feat(workspace): replace branch mismatch modal with non-blocking banner#3183
feat(workspace): replace branch mismatch modal with non-blocking banner#3183adboio wants to merge 4 commits into
Conversation
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
A task's linkedBranch is never cleaned up when its branch is deleted (the usual end state after a PR merge), so returning to a finished task and asking a follow-up from main nags with a wrong-branch warning on every send — a link like that can never match the working tree again. Production data suggests this drives the ~59% "continue anyway" rate on the mismatch dialog. getWorkspace now self-heals: when the linked branch mismatches the current branch, a new anyBranchRefExists git query checks for the branch as a local head or remote-tracking ref on any remote (no network; tags don't count). If nothing remains, the link is dropped with source "auto" on the existing Branch unlinked analytics event. Being on the linked branch proves it exists, so only mismatched reads pay the extra ref lookup, and an in-flight guard keeps concurrent reads from stacking git calls. Also makes the mock workspace repository's updateLinkedBranch persist instead of being a no-op, so service tests can exercise link state. Generated-By: PostHog Code Task-Id: adec8438-4ca0-42f0-9d9d-948de60c3b23
Greptile review feedback: the unlink/keep cases share one shape (seed, mock anyBranchRefExists, read, assert linkedBranch), so they collapse into an it.each on refExists -> outcome; the unlink side effects (event, analytics, persisted null) move to their own focused test. Generated-By: PostHog Code Task-Id: adec8438-4ca0-42f0-9d9d-948de60c3b23
adboio
force-pushed
the
posthog-code/auto-unlink-stale-branches
branch
from
287ea33 to
ed27d0a
Compare
The wrong-branch AlertDialog intercepted the user's send at the worst moment (the Enter keypress) and blocked it behind a modal. Production data says its premise is wrong most of the time: of ~1,250 dialogs in 60 days, only ~22% chose "Switch branch" while ~59% chose "Continue anyway" — and dismissal was session-scoped, so the same users re-hit the dialog ~4x per day. Replace it with a slim banner above the composer, shown as soon as the mismatch exists (before typing) via a new composerNotice slot on SessionView. Sending is never intercepted; the agent works on the current branch either way, which is what the majority already chose. Actions: - Switch branch: checkout the linked branch, loading and errors inline. - Use current branch: re-link the task to the current branch via workspace.linkBranch — a durable resolution that replaces the old per-session "Continue anyway" nag. - Dismiss: hides for the session (re-arms when the branch changes), as before. Analytics keep the same event names; the action union becomes switch | relink | dismiss, and "shown" now fires once per banner appearance instead of once per intercepted send. The old dialog, its hook, and the beforeSubmit interception are deleted; the banner-ineligible cloud case is unchanged (branchName is null there). Generated-By: PostHog Code Task-Id: adec8438-4ca0-42f0-9d9d-948de60c3b23
adboio
force-pushed
the
posthog-code/branch-mismatch-banner
branch
from
0044f13 to
b0577f3
Compare
|
Reviews (1): Last reviewed commit: "feat(workspace): replace branch mismatch..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { mutate: linkBranch, isPending: isRelinking } = useMutation( | ||
| trpc.workspace.linkBranch.mutationOptions({ | ||
| // The LinkedBranchChanged event invalidates the workspace query, which | ||
| // clears the mismatch and hides the banner. | ||
| onSuccess: () => setActionError(null), | ||
| onError: (error) => { | ||
| log.error("Failed to re-link branch", error); | ||
| setActionError( | ||
| resolveBranchMismatchError(error, "Failed to update the task branch"), | ||
| ); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
dismissWarning on relink success
After "Use current branch" succeeds, the banner stays visible until the LinkedBranchChanged event propagates through the workspace query. The checkout path avoids this gap by calling dismissWarning() on success for immediate hiding (onSuccess at line 85), but the linkBranch path only calls setActionError(null). If the real-time event is delayed or lost, the banner is permanently stuck in a state where the mismatch has already been resolved server-side.
| it("onUseCurrentBranch re-links the task and tracks relink", () => { | ||
| const { result } = renderBanner(); | ||
|
|
||
| mockTrack.mockClear(); | ||
| act(() => { | ||
| result.current?.onUseCurrentBranch(); | ||
| }); | ||
|
|
||
| expect(mutateSpies.link).toHaveBeenCalledWith({ | ||
| taskId: "task-1", | ||
| branchName: "main", | ||
| }); | ||
| expect(mockTrack).toHaveBeenCalledWith( | ||
| ANALYTICS_EVENTS.BRANCH_MISMATCH_ACTION, | ||
| { | ||
| task_id: "task-1", | ||
| action: "relink", | ||
| linked_branch: "feat/foo", | ||
| current_branch: "main", | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
linkBranch success and error callbacks are untested
The onSwitch block has two companion tests — one for success (captured.checkout.onSuccess?.()) and one for failure (captured.checkout.onError?.(...)) — but onUseCurrentBranch only verifies the mutation is invoked and the analytics event fires. The captured.link.onSuccess and captured.link.onError callbacks are never exercised, so the error display and error-clearing logic for the relink path have no coverage. Adding the parallel success/error cases would match the pattern already established for switch.
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: "Use current branch" success now calls dismissWarning() like the checkout path does, so the banner hides immediately instead of waiting on the LinkedBranchChanged round-trip (and can't stick around if that event is dropped). Adds the missing tests for the linkBranch success and error callbacks, mirroring the switch-path coverage. 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 |
1 participant
Problem
The wrong-branch
AlertDialogintercepts the user's send at the worst possible moment — the Enter keypress — and blocks it behind a modal. Production analytics say the modal's premise ("you probably want to switch") is wrong most of the time:The dialog's Switch action was also broken outright — wrapped in
AlertDialog.Action, its auto-close firedonCancel, wiped the pending message, and the message was never sent after checkout (errors were silent too). That bug dies here with the dialog itself (a standalone fix existed as #3179, closed unmerged in favor of this stack).Change
The modal, its hook, and the
onBeforeSubmitinterception are deleted. In their place: a slim banner above the composer, rendered via a newcomposerNoticeslot onSessionView, visible as soon as the mismatch exists — before the user has typed anything. Sending is never intercepted; the agent works on the current branch either way (which is what the majority already chose).Banner actions:
workspace.linkBranch(source: "user"). This replaces the old "Continue anyway", whose session-scoped dismissal was the main source of repeat nagging.Cloud tasks are unaffected (no live
branchName, banner never renders). The banner hides automatically once the mismatch clears — both actions propagate through the existingBranchChanged/LinkedBranchChanged→ workspace-query invalidation path.Analytics
Event names are unchanged (
Branch mismatch warning shown/Branch mismatch action) so existing dashboards keep working, with two semantic changes to note:actionvalues are nowswitch | relink | dismiss(continue/cancelretire with the modal).shownfires once per banner appearance rather than once per intercepted send, so its volume isn't directly comparable across the change.Tests
branchMismatchBanner.test.ts— analytics event building for all actions, checkout/link request builders, error resolution (13 tests).useBranchMismatchBanner.test.ts— shown-once-per-appearance, switch success/failure, relink, dismiss (6 tests).BranchMismatchBanner.test.tsx— branch rendering, action wiring, error display, busy state (4 tests).Stack:
🤖 Generated with Claude Code