feat(FN-1106): preserve planning sessions when closing modal

- Stop canceling active planning sessions on modal close and keep server-side progress intact - Remove progress-loss confirmation prompts and simplify close/escape/unload handling to just reset local UI state - Ensure close flows always disconnect the SSE stream connection without clearing the backend session - Rewrite PlanningModeModal close-behavior tests to verify no confirmation/cancel calls across initial, loading, question, summary, and overlay dismiss paths
2026-04-07 22:14:14 -07:00
parent 5b441ca86c
commit 1982362fb5
2 changed files with 128 additions and 149 deletions
--- a/packages/dashboard/app/components/PlanningModeModal.test.tsx
+++ b/packages/dashboard/app/components/PlanningModeModal.test.tsx
@@ -692,76 +692,9 @@ describe("PlanningModeModal", () => {
    });
  });

-  describe("Dismiss warning", () => {
-    it("shows confirmation when clicking X button with progress", async () => {
-      const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
-
-      render(
-        <PlanningModeModal
-          isOpen={true}
-          onClose={mockOnClose}
-          onTaskCreated={mockOnTaskCreated}
-          tasks={mockTasks}
-        />
-      );
-
-      const textarea = screen.getByPlaceholderText(/e.g., Build a user authentication/);
-      fireEvent.change(textarea, { target: { value: "Build auth system" } });
-      fireEvent.click(screen.getByText("Start Planning"));
-
-      // Wait for question view (progress made)
-      await waitFor(() => {
-        expect(screen.getByText("What is the scope?")).toBeDefined();
-      });
-
-      // Click X button
-      const closeButton = screen.getByLabelText("Close");
-      fireEvent.click(closeButton);
-
-      // Should show confirmation dialog
-      expect(confirmSpy).toHaveBeenCalledWith("Are you sure you want to close? Your planning progress will be lost.");
-      // onClose should NOT be called since confirm returned false
-      expect(mockOnClose).not.toHaveBeenCalled();
-
-      confirmSpy.mockRestore();
-    });
-
-    it("shows confirmation when clicking overlay with progress", async () => {
-      const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
-
-      const { container } = render(
-        <PlanningModeModal
-          isOpen={true}
-          onClose={mockOnClose}
-          onTaskCreated={mockOnTaskCreated}
-          tasks={mockTasks}
-        />
-      );
-
-      const textarea = screen.getByPlaceholderText(/e.g., Build a user authentication/);
-      fireEvent.change(textarea, { target: { value: "Build auth system" } });
-      fireEvent.click(screen.getByText("Start Planning"));
-
-      // Wait for question view (progress made)
-      await waitFor(() => {
-        expect(screen.getByText("What is the scope?")).toBeDefined();
-      });
-
-      // Click overlay (modal-overlay)
-      const overlay = container.querySelector(".modal-overlay");
-      expect(overlay).not.toBeNull();
-      fireEvent.click(overlay!);
-
-      // Should show confirmation dialog
-      expect(confirmSpy).toHaveBeenCalledWith("Are you sure you want to close? Your planning progress will be lost.");
-      // onClose should NOT be called since confirm returned false
-      expect(mockOnClose).not.toHaveBeenCalled();
-
-      confirmSpy.mockRestore();
-    });
-
-    it("no confirmation shown when no progress made (initial state)", async () => {
-      const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true);
+  describe("Modal close behavior", () => {
+    it("no confirmation shown when no progress made (initial state)", () => {
+      const confirmSpy = vi.spyOn(window, "confirm");

      render(
        <PlanningModeModal
@@ -776,16 +709,13 @@ describe("PlanningModeModal", () => {
      const closeButton = screen.getByLabelText("Close");
      fireEvent.click(closeButton);

-      // Should NOT show confirmation dialog
      expect(confirmSpy).not.toHaveBeenCalled();
-      // onClose should be called immediately
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
      expect(mockOnClose).toHaveBeenCalled();
-
-      confirmSpy.mockRestore();
    });

-    it("closes without confirmation after confirming dismiss", async () => {
-      const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(true);
+    it("closes active question session without canceling server session", async () => {
+      const confirmSpy = vi.spyOn(window, "confirm");

      render(
        <PlanningModeModal
@@ -796,44 +726,98 @@ describe("PlanningModeModal", () => {
        />
      );

-      const textarea = screen.getByPlaceholderText(/e.g., Build a user authentication/);
-      fireEvent.change(textarea, { target: { value: "Build auth system" } });
+      fireEvent.change(screen.getByPlaceholderText(/e.g., Build a user authentication/), {
+        target: { value: "Build auth system" },
+      });
      fireEvent.click(screen.getByText("Start Planning"));

-      // Wait for question view (progress made)
      await waitFor(() => {
        expect(screen.getByText("What is the scope?")).toBeDefined();
      });

-      // Click X button
-      const closeButton = screen.getByLabelText("Close");
-      fireEvent.click(closeButton);
+      fireEvent.click(screen.getByLabelText("Close"));

-      // Should show confirmation dialog
-      expect(confirmSpy).toHaveBeenCalledWith("Are you sure you want to close? Your planning progress will be lost.");
-      
-      // Wait for async handleCancel to complete
-      await waitFor(() => {
-        expect(mockOnClose).toHaveBeenCalled();
-      });
-
-      confirmSpy.mockRestore();
+      expect(confirmSpy).not.toHaveBeenCalled();
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
+      expect(mockOnClose).toHaveBeenCalled();
    });

-    it("shows confirmation in summary view", async () => {
-      // Override mock to return summary
+    it("closes summary view without canceling server session", async () => {
+      const confirmSpy = vi.spyOn(window, "confirm");
+
      mockConnectPlanningStream.mockImplementationOnce((_sessionId: string, _projectId: string | undefined, handlers: any) => {
        setTimeout(() => {
          handlers.onSummary?.(mockSummary);
        }, 10);
-        
+
        return {
          close: vi.fn(),
          isConnected: vi.fn().mockReturnValue(true),
        };
      });

-      const confirmSpy = vi.spyOn(window, "confirm").mockReturnValue(false);
+      render(
+        <PlanningModeModal
+          isOpen={true}
+          onClose={mockOnClose}
+          onTaskCreated={mockOnTaskCreated}
+          tasks={mockTasks}
+        />
+      );
+
+      fireEvent.change(screen.getByPlaceholderText(/e.g., Build a user authentication/), {
+        target: { value: "Build auth system" },
+      });
+      fireEvent.click(screen.getByText("Start Planning"));
+
+      await waitFor(() => {
+        expect(screen.getByText("Planning Complete!")).toBeDefined();
+      });
+
+      fireEvent.click(screen.getByLabelText("Close"));
+
+      expect(confirmSpy).not.toHaveBeenCalled();
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
+      expect(mockOnClose).toHaveBeenCalled();
+    });
+
+    it("closes via overlay without canceling server session", async () => {
+      const confirmSpy = vi.spyOn(window, "confirm");
+
+      const { container } = render(
+        <PlanningModeModal
+          isOpen={true}
+          onClose={mockOnClose}
+          onTaskCreated={mockOnTaskCreated}
+          tasks={mockTasks}
+        />
+      );
+
+      fireEvent.change(screen.getByPlaceholderText(/e.g., Build a user authentication/), {
+        target: { value: "Build auth system" },
+      });
+      fireEvent.click(screen.getByText("Start Planning"));
+
+      await waitFor(() => {
+        expect(screen.getByText("What is the scope?")).toBeDefined();
+      });
+
+      const overlay = container.querySelector(".modal-overlay");
+      expect(overlay).not.toBeNull();
+      fireEvent.click(overlay!);
+
+      expect(confirmSpy).not.toHaveBeenCalled();
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
+      expect(mockOnClose).toHaveBeenCalled();
+    });
+
+    it("closes during loading state without canceling server session", async () => {
+      const confirmSpy = vi.spyOn(window, "confirm");
+
+      mockConnectPlanningStream.mockImplementationOnce(() => ({
+        close: vi.fn(),
+        isConnected: vi.fn().mockReturnValue(true),
+      }));

      render(
        <PlanningModeModal
@@ -844,24 +828,53 @@ describe("PlanningModeModal", () => {
        />
      );

-      const textarea = screen.getByPlaceholderText(/e.g., Build a user authentication/);
-      fireEvent.change(textarea, { target: { value: "Build auth system" } });
+      fireEvent.change(screen.getByPlaceholderText(/e.g., Build a user authentication/), {
+        target: { value: "Build auth system" },
+      });
      fireEvent.click(screen.getByText("Start Planning"));

-      // Wait for summary view (progress made)
      await waitFor(() => {
-        expect(screen.getByText("Planning Complete!")).toBeDefined();
+        expect(screen.getByText("Generating next question...")).toBeDefined();
      });

-      // Click X button
-      const closeButton = screen.getByLabelText("Close");
-      fireEvent.click(closeButton);
+      fireEvent.click(screen.getByLabelText("Close"));

-      // Should show confirmation dialog
-      expect(confirmSpy).toHaveBeenCalledWith("Are you sure you want to close? Your planning progress will be lost.");
-      expect(mockOnClose).not.toHaveBeenCalled();
+      expect(confirmSpy).not.toHaveBeenCalled();
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
+      expect(mockOnClose).toHaveBeenCalled();
+    });

-      confirmSpy.mockRestore();
+    it("disconnects SSE stream on close", async () => {
+      const closeSpy = vi.fn();
+
+      mockConnectPlanningStream.mockImplementationOnce(() => ({
+        close: closeSpy,
+        isConnected: vi.fn().mockReturnValue(true),
+      }));
+
+      render(
+        <PlanningModeModal
+          isOpen={true}
+          onClose={mockOnClose}
+          onTaskCreated={mockOnTaskCreated}
+          tasks={mockTasks}
+        />
+      );
+
+      fireEvent.change(screen.getByPlaceholderText(/e.g., Build a user authentication/), {
+        target: { value: "Build auth system" },
+      });
+      fireEvent.click(screen.getByText("Start Planning"));
+
+      await waitFor(() => {
+        expect(screen.getByText("Generating next question...")).toBeDefined();
+      });
+
+      fireEvent.click(screen.getByLabelText("Close"));
+
+      expect(closeSpy).toHaveBeenCalledTimes(1);
+      expect(mockCancelPlanning).not.toHaveBeenCalled();
+      expect(mockOnClose).toHaveBeenCalled();
    });
  });
 });
--- a/packages/dashboard/app/components/PlanningModeModal.tsx
+++ b/packages/dashboard/app/components/PlanningModeModal.tsx
@@ -4,7 +4,6 @@ import {
  startPlanning,
  startPlanningStreaming,
  respondToPlanning,
-  cancelPlanning,
  createTaskFromPlanning,
  connectPlanningStream,
  fetchAiSession,
@@ -57,7 +56,6 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
  const [error, setError] = useState<string | null>(null);
  const [responseHistory, setResponseHistory] = useState<QuestionResponse[]>([]);
  const [editedSummary, setEditedSummary] = useState<PlanningSummary | null>(null);
-  const [hasProgress, setHasProgress] = useState(false);
  // Use ref instead of state for hasAutoStarted to handle React StrictMode double-render.
  // In StrictMode, components render twice but state persists across renders,
  // which would skip auto-start on the second (committed) render. Refs are
@@ -94,7 +92,6 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
            session: { sessionId, currentQuestion: question, summary: null },
          });
          setStreamingOutput("");
-          setHasProgress(true);
        },
        onSummary: (summary) => {
          clearPlanningDescription();
@@ -105,7 +102,6 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
          });
          setEditedSummary(summary);
          setStreamingOutput("");
-          setHasProgress(true);
        },
        onError: (message) => {
          setError(message);
@@ -170,13 +166,11 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
          const question = JSON.parse(session.currentQuestion);
          setView({ type: "question", session: { sessionId: resumeSessionId, currentQuestion: question, summary: null } });
          if (session.thinkingOutput) setStreamingOutput(session.thinkingOutput);
-          setHasProgress(true);
        } else if (session.status === "complete" && session.result) {
          clearPlanningDescription();
          const summary = JSON.parse(session.result);
          setView({ type: "summary", session: { sessionId: resumeSessionId, currentQuestion: null, summary }, summary });
          setEditedSummary(summary);
-          setHasProgress(true);
        } else if (session.status === "generating") {
          setView({ type: "loading" });
          if (session.thinkingOutput) setStreamingOutput(session.thinkingOutput);
@@ -187,14 +181,12 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
              clearPlanningDescription();
              setView({ type: "question", session: { sessionId: resumeSessionId, currentQuestion: question, summary: null } });
              setStreamingOutput("");
-              setHasProgress(true);
            },
            onSummary: (summary) => {
              clearPlanningDescription();
              setView({ type: "summary", session: { sessionId: resumeSessionId, currentQuestion: null, summary }, summary });
              setEditedSummary(summary);
              setStreamingOutput("");
-              setHasProgress(true);
            },
            onError: (message) => { setError(message); setView({ type: "initial" }); },
            onComplete: () => { currentSessionIdRef.current = null; },
@@ -226,57 +218,39 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
    };
  }, []);

-  // Handle browser unload during active session
+  // Handle browser unload while modal is open
  useEffect(() => {
    if (!isOpen) return;

-    const handleBeforeUnload = (e: BeforeUnloadEvent) => {
-      if (view.type === "question" || view.type === "summary") {
-        e.preventDefault();
-        e.returnValue = "";
-      }
-      // Close stream connection
+    const handleBeforeUnload = () => {
+      // Session is preserved server-side; just disconnect the local stream.
      streamConnectionRef.current?.close();
+      streamConnectionRef.current = null;
    };

    window.addEventListener("beforeunload", handleBeforeUnload);
    return () => window.removeEventListener("beforeunload", handleBeforeUnload);
-  }, [isOpen, view]);
+  }, [isOpen]);

-  const handleCancel = useCallback(async () => {
+  const handleCancel = useCallback(() => {
    // Save to localStorage BEFORE any cleanup (preserve for re-entry)
    if (initialPlan) {
      savePlanningDescription(initialPlan);
    }

-    // Show confirmation if user has made progress
-    if (hasProgress) {
-      if (!confirm("Are you sure you want to close? Your planning progress will be lost.")) {
-        return;
-      }
-    }
-
    // Always close the stream connection
    streamConnectionRef.current?.close();
    streamConnectionRef.current = null;

-    if (view.type === "question" || view.type === "summary") {
-      try {
-        await cancelPlanning(view.session.sessionId, projectId);
-      } catch {
-        // Ignore errors on cancel
-      }
-    }
    setInitialPlan("");
    setView({ type: "initial" });
    setError(null);
    setResponseHistory([]);
    setEditedSummary(null);
    setStreamingOutput("");
-    setHasProgress(false);
    currentSessionIdRef.current = null;
    onClose();
-  }, [initialPlan, hasProgress, view, onClose]);
+  }, [initialPlan, onClose]);

  // Handle escape key to close
  useEffect(() => {
@@ -284,19 +258,13 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat

    const handleKeyDown = (e: KeyboardEvent) => {
      if (e.key === "Escape") {
-        if (hasProgress) {
-          if (confirm("Are you sure you want to close? Your planning progress will be lost.")) {
-            handleCancel();
-          }
-        } else {
-          handleCancel();
-        }
+        handleCancel();
      }
    };

    document.addEventListener("keydown", handleKeyDown);
    return () => document.removeEventListener("keydown", handleKeyDown);
-  }, [isOpen, hasProgress, handleCancel]);
+  }, [isOpen, handleCancel]);

  const handleSubmitResponse = useCallback(
    async (responses: QuestionResponse) => {
@@ -319,7 +287,6 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
        // Submit response - AI will broadcast events via the already-connected stream
        await respondToPlanning(sessionId, responses, projectId);
        setResponseHistory((prev) => [...prev, responses]);
-        setHasProgress(true);
        // Events (question/summary) will arrive via the existing SSE stream
      } catch (err: any) {
        setError(err.message || "Failed to submit response");
@@ -381,7 +348,6 @@ export function PlanningModeModal({ isOpen, onClose, onTaskCreated, onTasksCreat
      setResponseHistory([]);
      setEditedSummary(null);
      setStreamingOutput("");
-      setHasProgress(false);
      currentSessionIdRef.current = null;
      onClose();
    } catch (err: any) {