From 70b6ab931f65bd46420aaa5b98ad76582960e8a3 Mon Sep 17 00:00:00 2001 From: Adam Bowker Date: Mon, 6 Jul 2026 10:13:36 -0400 Subject: [PATCH 1/2] fix(workspace): make branch mismatch "Switch branch" actually send the message The Switch button was wrapped in AlertDialog.Action, which closes the dialog on click. Closing fires onOpenChange(false) -> onCancel, which discarded the pending message and clear-editor refs while the checkout mutation was still in flight, so on success the user's message was never sent (and checkout errors rendered into an already-closed dialog, failing silently). It also double-fired the "Branch mismatch action" analytics event: every "switch" also emitted a "cancel", which is visible in production data (60d: 1,249 showns vs 1,513 actions; the 264-event excess matches the 271 switches). Unwrap the button so the dialog stays open through the mutation (loading spinner now actually visible, errors surface inline), ignore close requests while switching so Escape can't wipe the pending message mid-checkout, and clear a stale switch error on success. Generated-By: PostHog Code Task-Id: adec8438-4ca0-42f0-9d9d-948de60c3b23 --- .../task-detail/BranchMismatchDialog.test.tsx | 90 +++++++++++++++++++ .../task-detail/BranchMismatchDialog.tsx | 26 +++--- .../workspace/useBranchMismatchDialog.ts | 1 + 3 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx diff --git a/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx b/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx new file mode 100644 index 0000000000..7076106a3c --- /dev/null +++ b/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx @@ -0,0 +1,90 @@ +import { Theme } from "@radix-ui/themes"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi } from "vitest"; +import { BranchMismatchDialog } from "./BranchMismatchDialog"; + +function renderDialog( + overrides?: Partial[0]>, +) { + const handlers = { + onSwitch: vi.fn(), + onContinue: vi.fn(), + onCancel: vi.fn(), + }; + render( + + + , + ); + return handlers; +} + +describe("BranchMismatchDialog", () => { + it("clicking Switch branch does not fire onCancel", async () => { + // Regression: the Switch button was wrapped in AlertDialog.Action, whose + // auto-close fired onOpenChange -> onCancel, discarding the pending + // message so it was never sent after the checkout succeeded. + const user = userEvent.setup(); + const { onSwitch, onCancel } = renderDialog(); + + await user.click(screen.getByRole("button", { name: "Switch branch" })); + + expect(onSwitch).toHaveBeenCalledTimes(1); + expect(onCancel).not.toHaveBeenCalled(); + }); + + it("clicking Cancel fires onCancel only", async () => { + const user = userEvent.setup(); + const { onSwitch, onContinue, onCancel } = renderDialog(); + + await user.click(screen.getByRole("button", { name: "Cancel" })); + + expect(onCancel).toHaveBeenCalledTimes(1); + expect(onSwitch).not.toHaveBeenCalled(); + expect(onContinue).not.toHaveBeenCalled(); + }); + + it("clicking Continue anyway fires onContinue only", async () => { + const user = userEvent.setup(); + const { onSwitch, onContinue, onCancel } = renderDialog(); + + await user.click(screen.getByRole("button", { name: "Continue anyway" })); + + expect(onContinue).toHaveBeenCalledTimes(1); + expect(onSwitch).not.toHaveBeenCalled(); + expect(onCancel).not.toHaveBeenCalled(); + }); + + it("Escape closes and fires onCancel when idle", async () => { + const user = userEvent.setup(); + const { onCancel } = renderDialog(); + + await user.keyboard("{Escape}"); + + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + it("Escape does not fire onCancel while switching", async () => { + const user = userEvent.setup(); + const { onCancel } = renderDialog({ isSwitching: true }); + + await user.keyboard("{Escape}"); + + expect(onCancel).not.toHaveBeenCalled(); + }); + + it("shows the switch error", () => { + renderDialog({ switchError: "dirty worktree" }); + + expect(screen.getByText("dirty worktree")).toBeInTheDocument(); + }); +}); diff --git a/packages/ui/src/features/task-detail/BranchMismatchDialog.tsx b/packages/ui/src/features/task-detail/BranchMismatchDialog.tsx index 23c5cb5b78..04b343cae1 100644 --- a/packages/ui/src/features/task-detail/BranchMismatchDialog.tsx +++ b/packages/ui/src/features/task-detail/BranchMismatchDialog.tsx @@ -55,7 +55,9 @@ export function BranchMismatchDialog({ { - if (!isOpen) onCancel(); + // While the checkout runs the dialog must stay up: onCancel discards + // the pending message the switch handler sends on success. + if (!isOpen && !isSwitching) onCancel(); }} > @@ -116,16 +118,18 @@ export function BranchMismatchDialog({ Continue anyway - - - + {/* Not an AlertDialog.Action: Action closes the dialog on click, + which fires onOpenChange -> onCancel and discards the pending + message before the checkout finishes. The dialog closes (or + shows the error) when the mutation settles. */} + diff --git a/packages/ui/src/features/workspace/useBranchMismatchDialog.ts b/packages/ui/src/features/workspace/useBranchMismatchDialog.ts index bc76a9255b..423688cc22 100644 --- a/packages/ui/src/features/workspace/useBranchMismatchDialog.ts +++ b/packages/ui/src/features/workspace/useBranchMismatchDialog.ts @@ -74,6 +74,7 @@ export function useBranchMismatchDialog({ if (message) onSendPromptRef.current(message); setPendingMessage(null); pendingMessageRef.current = null; + setSwitchError(null); }, onError: (error) => { log.error("Failed to switch branch", error); From 9d164431e7fad4f90f70f803c91ba02db5474049 Mon Sep 17 00:00:00 2001 From: Adam Bowker Date: Mon, 6 Jul 2026 10:52:38 -0400 Subject: [PATCH 2/2] review: parameterise dialog click tests, drop redundant error clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile review feedback: the three button-click tests shared one shape (click a button, exactly one handler fires), so they collapse into an it.each — the Switch case still asserts onCancel never fires, which is the regression this PR guards. Also removes the setSwitchError(null) in the checkout onSuccess: handleSwitch clears the error synchronously before every mutate and onError is the only other setter, so success can never observe a stale error. Generated-By: PostHog Code Task-Id: adec8438-4ca0-42f0-9d9d-948de60c3b23 --- .../task-detail/BranchMismatchDialog.test.tsx | 55 +++++++------------ .../workspace/useBranchMismatchDialog.ts | 1 - 2 files changed, 21 insertions(+), 35 deletions(-) diff --git a/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx b/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx index 7076106a3c..6c2d317ec2 100644 --- a/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx +++ b/packages/ui/src/features/task-detail/BranchMismatchDialog.test.tsx @@ -29,40 +29,27 @@ function renderDialog( } describe("BranchMismatchDialog", () => { - it("clicking Switch branch does not fire onCancel", async () => { - // Regression: the Switch button was wrapped in AlertDialog.Action, whose - // auto-close fired onOpenChange -> onCancel, discarding the pending - // message so it was never sent after the checkout succeeded. - const user = userEvent.setup(); - const { onSwitch, onCancel } = renderDialog(); - - await user.click(screen.getByRole("button", { name: "Switch branch" })); - - expect(onSwitch).toHaveBeenCalledTimes(1); - expect(onCancel).not.toHaveBeenCalled(); - }); - - it("clicking Cancel fires onCancel only", async () => { - const user = userEvent.setup(); - const { onSwitch, onContinue, onCancel } = renderDialog(); - - await user.click(screen.getByRole("button", { name: "Cancel" })); - - expect(onCancel).toHaveBeenCalledTimes(1); - expect(onSwitch).not.toHaveBeenCalled(); - expect(onContinue).not.toHaveBeenCalled(); - }); - - it("clicking Continue anyway fires onContinue only", async () => { - const user = userEvent.setup(); - const { onSwitch, onContinue, onCancel } = renderDialog(); - - await user.click(screen.getByRole("button", { name: "Continue anyway" })); - - expect(onContinue).toHaveBeenCalledTimes(1); - expect(onSwitch).not.toHaveBeenCalled(); - expect(onCancel).not.toHaveBeenCalled(); - }); + // The Switch case is the regression: the button was wrapped in + // AlertDialog.Action, whose auto-close fired onOpenChange -> onCancel, + // discarding the pending message so it was never sent after the checkout + // succeeded. + it.each([ + { button: "Switch branch", fires: "onSwitch" }, + { button: "Continue anyway", fires: "onContinue" }, + { button: "Cancel", fires: "onCancel" }, + ] as const)( + "clicking $button fires $fires only", + async ({ button, fires }) => { + const user = userEvent.setup(); + const handlers = renderDialog(); + + await user.click(screen.getByRole("button", { name: button })); + + for (const [name, handler] of Object.entries(handlers)) { + expect(handler, name).toHaveBeenCalledTimes(name === fires ? 1 : 0); + } + }, + ); it("Escape closes and fires onCancel when idle", async () => { const user = userEvent.setup(); diff --git a/packages/ui/src/features/workspace/useBranchMismatchDialog.ts b/packages/ui/src/features/workspace/useBranchMismatchDialog.ts index 423688cc22..bc76a9255b 100644 --- a/packages/ui/src/features/workspace/useBranchMismatchDialog.ts +++ b/packages/ui/src/features/workspace/useBranchMismatchDialog.ts @@ -74,7 +74,6 @@ export function useBranchMismatchDialog({ if (message) onSendPromptRef.current(message); setPendingMessage(null); pendingMessageRef.current = null; - setSwitchError(null); }, onError: (error) => { log.error("Failed to switch branch", error);