fix(workspace): make branch mismatch "Switch branch" actually send the message#3179
fix(workspace): make branch mismatch "Switch branch" actually send the message#3179adboio wants to merge 2 commits into
Conversation
…e 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
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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); | ||
| }); | ||
|
|
There was a problem hiding this comment.
it.each for this pattern; consolidating them removes the duplicated assertion structure without losing any coverage.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
|
Closing unmerged: the branch-mismatch dialog this fixes is deleted wholesale by #3183 (the modal → banner replacement), so merging this would only add review load for code that dies two PRs up the stack. The stack is now #3180 → #3183, rebased so this PR's commits are no longer part of it. If the banner direction gets dropped, this fix stands alone and can be reopened. |
1 participant
Problem
In the branch-mismatch dialog, the Switch branch button was wrapped in
AlertDialog.Action, which auto-closes the dialog on click. Closing firesonOpenChange(false)→onCancel(), which:loadingstate on the button was never visible (dialog already gone);switchalso emitted acancel.The double-fire is visible in production: over the last 60 days,
Branch mismatch warning shownfired 1,249× while actions total 1,513 (continue 737 / cancel 505 / switch 271). The 264-event excess matches the 271 switches almost exactly — corrected, the real distribution is ~59% continue / ~22% switch / ~19% cancel.Fix
AlertDialog.Actionso clicking it no longer closes the dialog; it now stays open through the mutation (spinner visible, errors surface inline) and closes via state when the checkout succeeds.onOpenChangeclose requests whileisSwitching, so Escape can't wipe the pending message mid-checkout.switchErrorwhen a retry succeeds.Tests
New
BranchMismatchDialog.test.tsxcomponent test clicks the real Radix buttons and assertsonSwitchdoesn't co-fireonCancel(the regression), plus coverage for Cancel/Continue/Escape/error states. Existing hook tests unchanged and passing.Part of a stack reworking the branch-mismatch UX:
🤖 Generated with Claude Code