Repository navigation
fix(github): keep the PR panel's view and unsent review across channel switches - #3060
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot coverage.
One P2 finding inline: submit completion in a still-mounted tab can delete or overwrite a newer sibling-tab draft.
Optional documentation correction: docs/buzz-help.md:976–977 says closing Submit review after the PR closes/merges also discards the draft. SubmitReview.tsx:110 only hides the control, so the draft can return if a closed PR reopens during the session. Dropping “and the draft with it” would match the implemented behavior and the PR’s known limits.
Reviewed head 639e8f9767f970a85d7d037bb52e430a43e93167 against base/merge-base a905c4865f1ebc6672f71e1bc65cc94cfc84054d. Source-only: no tests, builds or app execution; desktop GitHub integration and live focus behavior remain unverified. Inspected the PR text, screenshot and video frames sampled at one-second intervals; no publication-boundary issue identified in those samples. The supplied recording uses GitHub stubs and shows an earlier fix commit, so it is not runtime validation of this head.
639e8f9 to
942bfce
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Follow-up source review at 942bfceaae55fc6860828e10c2c951febd3801d4 against base/merge-base 40e6ccb597cf9d8b47f9692de8285458cc304017.
The previously reported submission-settlement race is fixed: edit-only persistence lets a newer sibling draft survive either success or failure. The help-text correction also resolves the earlier wording mismatch.
One residual P2 remains in the same ownership flow: retrying the failed tab without editing deletes the sibling's newer draft, although the retry submits only the failed tab's older text. The inline comment gives the sequence and repair. This unconditional clear existed in the previous reviewed revision; we missed the retry ordering in the first review. It is newly identified evidence, not a regression introduced by this fix.
Rocket, Mantis and Groot independently covered persistence/lifecycle, failure/ownership/stale-head behavior, and recovery/focus/docs/test contracts; Star Lord integrated the findings. Earlier unchanged feature coverage is retained. Source inspection only: no tests, builds or apps were run. Current image/video bytes match the previously inspected material; the earlier recording uses GitHub stubs and predates this head, so it does not establish current desktop behavior.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source follow-up at e220b1ec69c8d272603fb5e6ce6f51044a98e2b6 against base/merge-base 40e6ccb597cf9d8b47f9692de8285458cc304017.
No new actionable findings in this bounded follow-up. The remaining retry P2 from review 5824243604 is repaired: submit clears shared memory only when the saved event/body/head matches the payload being sent, so a different sibling draft survives an unedited retry whether it succeeds or fails. The changed regression cases exercise both outcomes and check the saved draft and reopened text/decision. The earlier settlement repair remains intact; coverage of the five unchanged patches is retained.
Rocket, Mantis and Groot independently checked ownership/lifecycle, conditional clearing/restoration and stale-head safety, and regression ordering/recovery/focus contracts; Star Lord verified and integrated their results.
Source inspection only: no tests, builds or apps were run. The PR body and freshly downloaded image/video are unchanged from previously inspected material. That recording predates this head and uses GitHub stubs, so current desktop behavior and runtime focus remain unverified.
…s channel switches
Leaving a channel unmounts its GitHub panel tab, so coming back started over:
Summary instead of Changes, and a Submit review draft (decision, text and the
head it was written against) was lost.
The plugin now keeps a PanelMemory, created once in apply beside the
pull-state store and keyed by pull request URL. A standalone GitHubPanel keeps
its own, as it does for states. ObjectPanel seeds the Summary/Changes view
from it (and mounts the diff if Changes was showing) and writes the view back
when the tab changes. SubmitReview seeds its event, body and head from the
remembered draft and writes them back as you type. An empty Comment is never
remembered. The draft is cleared once the review mutation succeeds, even if
the panel unmounted mid-submit, so a sent review never comes back.
The memory is in-process only: it lasts for the app session, survives
closing the tab, is shared by panels on the same pull request from any
channel (GitHub keeps one pending review per pull request), and does not
survive a restart or reload. A remembered head older than the pull request's
current head shows the existing "New commits were pushed" notice. Scroll
position is not restored; that needs a host change.
Evidence: on unmodified main the new remount case fails at its first
assertion, expect(Changes tab).toHaveAttribute("aria-selected", "true")
receiving "false". With only SubmitReview reverted it fails at the Request
changes radio's toBeChecked(). A second case, a review that goes in after its
panel unmounted, fails without the submit-time clear. GitHubPanel.test.tsx
52 -> 54 passing; GitHub plugin suite 345 -> 347 passing in 20 files;
tsc --noEmit clean; Biome clean on changed files.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: mtoohey <mtoohey@squareup.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ew per channel Follow-up to a5ad4ba from review. Duplicate review: the remembered draft was cleared only when GitHub answered, so a panel remounted mid-submit (channel switch and back, or a second tab on the same pull request) seeded the draft and could send it again. SubmitReview now moves the draft out of memory when the submit starts and puts it back only if the submit fails (unless another panel has started a new one since). Opening the popover re-reads memory: a closed popover's draft is memory's, so a draft another panel sent or changed is dropped or taken over. A decision with no text stays in the mounted panel. View scope: the Summary/Changes view is now remembered per channel and pull request, falling back to the URL alone without a channel, so a first open from another channel starts on Summary with the title. The unsent review stays per pull request from any channel, as GitHub keeps one pending review per pull request. Empty draft: a review is remembered only once its text is non-blank (isEmptyDraft, shared by the write effect, onOpenChange and the failure path). A text-less Request changes or Approve is no longer kept, so it no longer shows "New commits were pushed" after a push. Store: the raw Map becomes createPanelMemory() in panel-memory.ts, which also holds ReviewDraft so SubmitReview and the store don't import each other. get() reads without inserting, writes key by the lowercased pull request URL as the pull-state store does, and only pull requests are kept. Nothing is written during render. Lifetime is unchanged: session only, per plugin activation, so a reload or turning the plugin off and on forgets it. Scroll position is not kept. docs/buzz-help.md gets its own bullet saying so, and that Changes and Submit review need the desktop app with gh signed in. Tests: the remount case ends on a whole-entry assertion; new cases for a review whose panel unmounts and remounts before GitHub answers (sent, and refused), a fresh open from another channel, and a remembered head older than the fresh one alongside a whitespace-only draft; store unit tests for casing and non-pull URLs. Each fix was broken in turn and its case fails: clearing on resolve instead of at submit start, no re-read on open, no put-back on failure, the old empty rule, and a view keyed by URL alone. GitHubPanel.test.tsx 54 -> 57 passing; GitHub plugin suite 347 -> 352 passing in 21 files; tsc --noEmit clean; Biome clean on changed files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to 56d713e from its review. Regression: re-reading memory on opening Submit review refreshed the head whenever memory held nothing, whatever the mounted panel held. A text-less Approve, which isn't remembered and so stays in the panel, then silently moved to commits pushed since it was chosen, with no "New commits were pushed" notice and Submit enabled. Main refreshed the head only for an empty Comment, and that rule is back: a saved draft is taken whole (event, body, head); text with nothing saved, because another panel sent or cleared it, resets to an empty Comment at the latest head; otherwise only a Comment follows the latest head. Mid-submit write: "Use latest commit" stayed enabled while a review was going in, and clicking it re-ran the draft's write effect, putting the in-flight draft back into memory, where a later panel could offer it again. The button is now disabled while busy, and the write effect skips while busy, so either guard alone keeps the draft out until GitHub answers. Docs: a draft on a pull request that merges or closes first isn't kept "until you submit it"; Submit review goes away once closed, and the draft with it. Tests: an Approve variant of the mid-draft push case (no text, Escape, poll brings a new head, reopen: notice and aria-disabled Submit) passes on main's index.tsx and SubmitReview.tsx and fails on 56d713e (no status). A two-panel case on one memory (draft in the first, open and close the second, submit from the first, reopen the second: empty Comment) fails when the re-read's text-but-nothing-saved branch is dropped, leaving only `if (saved)`. GitHubPanel.test.tsx 57 -> 59 passing; GitHub plugin suite 352 -> 354 passing in 21 files; tsc --noEmit clean; Biome clean on changed files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… push Follow-up to 04027a7 from its final review. Test and comment only. Adds the case for the mid-submit guards: with the review mutation held, a poll brings a new head and "Use latest commit" is clicked; the panel then unmounts and a second panel on the same memory opens Submit review on an empty Comment, so it can't send the review again. Fail-then-pass: the case fails on 56d713e's SubmitReview.tsx and with both guards removed (the write effect's `if (busy) return` and the button's `disabled={busy}`), and passes with them. It also fails with only the early return removed: with `busy` among the effect's dependencies, the effect re-runs as the submit starts and writes the draft straight back. So the early return is what keeps the draft out on its own, and the disabled button is the second guard, enough by itself only with 56d713e's effect (`busy` not a dependency), where the case passes. This corrects 04027a7's message, which said either guard alone holds. The comment on the failure path's put-back now says its "unless another panel has started a draft since" check only matters once the panel has unmounted: while mounted, the write effect saves the draft again when busy ends. GitHubPanel.test.tsx 59 -> 60 passing; GitHub plugin suite 354 -> 355 passing in 21 files; tsc --noEmit clean; Biome clean on changed files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mtoohey <mtoohey@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-up to 639e8f9 from wesbillman's review of PR 3060 (the P2 thread on SubmitReview.tsx line 63). Two tabs on one pull request stay mounted once shown. Submitting from the first took the draft out of memory, but when GitHub answered, the first tab's reset (success) or recovery (failure) re-ran the draft write effect, which wrote the first tab's state back: an empty body deleted a draft the second tab had written meanwhile, and the older text replaced it, bypassing the failure path's no-newer-draft check. Reopening the second tab then took that missing or older draft and lost the new review. Memory now follows the person's edits only. The write effect is gone; the decision, comment and "Use latest commit" handlers write through one edit() that sets state and memory together. Submit still takes the draft out as it starts, and neither the success reset nor the failure path writes anything beyond the existing conditional put-back, which stays for a panel that unmounted mid-submit and remains the only restore while mounted. Inputs are disabled while busy, so nothing is written mid-submit and the earlier busy guards on the effect are no longer needed. Docs: the same review's optional note. Closing Submit review after the pull request merges or closes hides the control; the draft stays in memory for the session, so "and the draft with it" is dropped. Tests: a two-tab case in two variants, both tabs mounted, the first's review held in flight while the second writes a Request changes draft and cancels out; the first is then sent or refused, and the second reopens on its own draft, with memory holding it. On 639e8f9's SubmitReview.tsx the sent variant fails with memory empty and the refused variant with the first tab's Comment "Looks right" in memory. GitHubPanel.test.tsx 60 -> 62 passing; GitHub plugin suite 355 -> 357 passing in 21 files; tsc --noEmit clean; Biome clean on changed files. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d unedited Follow-up to 942bfce from wesbillman's review of PR 3060 (the P2 thread on SubmitReview.tsx line 85). Submit took the draft out of memory unconditionally as it started. With two tabs on one pull request, the first's review refused while the second wrote a different draft meanwhile, the first tab still shows its older text; retrying it as it stands cleared the second tab's draft before sending only the first's. A retry that went in left memory empty, and one refused again put the first's draft back into the empty slot, so reopening the second tab lost its unsent text either way, though it was the last editor. Submit now clears memory only when the saved draft matches the one going in (event, body and head) or memory is already empty. A different saved draft is another panel's, written since this one was, and stays. The conditional put-back on failure is unchanged, so it still never replaces a newer draft, and memory still follows the person's edits only through edit(). Tests: the two-tab refused case gains two variants that retry the first tab's refused review unedited, once sent and once refused again, and assert the second tab's draft is still in memory and that reopening the second tab shows its Request changes text. On 942bfce's SubmitReview.tsx the sent variant fails with memory empty and the refused variant with the first tab's Comment "Looks right" in memory; with the memory assertion removed, both fail at the reopened tab's Request changes radio. The retry follows the refusal directly: reopening the second tab first is an outside press that closes the first tab's popover once it is no longer busy. GitHubPanel.test.tsx 62 -> 63 passing; GitHub plugin suite 357 -> 358 passing in 21 files; tsc --noEmit clean; Biome clean on changed files. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
e220b1e to
80ca33b
Compare
kazemek21
left a comment
There was a problem hiding this comment.
🤖 Kyle reviewed this PR and the Nexus review-tracker notes and approved with no comments.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
Why
Since #2838 keeps a channel's side panels open when you return to it, the GitHub tab survives a channel switch but its panel is unmounted and remounted. Coming back to the channel therefore started the pull request panel over: Changes fell back to Summary, and a Submit review draft (decision, text, and the head it was written against) was lost.
What changes
The GitHub plugin owns a small per-session panel memory, created once in
applybeside the pull-state store (src/bundled/github/panel-memory.ts):Use latest commitis disabled while a submit is in flight and the write effect skips while busy.docs/buzz-help.mdgains a bullet describing this.#1793 (restore tab workspaces across refresh) is rebase-only against this; there is no behavioural interaction.
Before and after
Before is on the left at
origin/maina905c4865f(the fix commit's parent); After is on the right ata5ad4ba667. Watch the GitHub panel on the right of each half.github-panel-memory-before-after.mp4
a905c4865f)a5ad4ba667)The recording is the production browser build with the plugin's
ghclient and diff reader swapped at build time for an in-memory stub so Changes and Submit review are available outside the desktop app; nothing else is faked.Test plan
GitHubPanel.test.tsx52 → 60 passing; GitHub plugin suite 345 → 355 passing across 21 files.tsc --noEmitclean; Biome clean on changed files.ghsigned in.Known limits
🤖 Generated with Claude Code