🤖 fix(review): assisted-review UX pass#3358
Conversation
|
@codex review Please review this UX pass for assisted code review. Key things to check:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e89e149536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed both findings:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1735a413d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the "new" badge expiration: drive the predicate off a |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a9aa962a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed both follow-ups:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 738e6b33d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed both follow-ups:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c12b2a5fb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Final note: the previous CI Test/Unit run had ~7 extra failures beyond the suite's pre-existing flakes. They turned out to be order-dependent: adding |
|
@codex review Addressed: dismissed-pin pruning now bails when |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Diagnosed the 39 unit-test failures: Surgical fix: added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e477ac3e74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed both:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be44899dde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9086215a3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed: assisted metadata carryover is now restricted to |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
PR is ready to merge from this branch's side. Codex: Approved on latest commit ( CI status:
The Test/Unit failure set is not caused by this PR:
This PR already adds defensive Pausing here for direction: merge as-is, or open a follow-up issue tracking the |
Address every issue from the assisted-review UX index: - A1/A2: respect mark-as-read in Assisted mode by introducing a separate `assistedShowReadHunks` filter (defaults to "hide done") and routing navigation through the effective value. - A3/B3: control bar honors user filters in both modes, dims the Uncommitted toggle with an explanatory tooltip when Assisted forces it on, and keeps the Assisted toggle visible while in worklist mode even if the agent clears its set. - A4: Assisted toggle badge now shows `unread/total` and shares wording with the Review tab badge. - B1/C1: new banner above the hunk list shows worklist status, "all caught up" copy, and inline exits (button + keybind hint). - B2: per-pin dismiss button + persisted per-workspace quiet list with self-healing prune when the agent clears/replaces; restore button exposed from the control bar and the empty-state CTA bar. - B4: bind `Shift+A` (TOGGLE_ASSISTED_REVIEW) for toggling Assisted from the keyboard; respects review-panel scoping. - C2: empty-state offers inline buttons to exit Assisted, show read pins, or restore dismissed pins instead of just prose guidance. - D1: ImmersiveReviewView gains assisted indicator + agent comment banner so the focus signal survives the immersive transition. - E1/E2: aggregator tracks `sourceMessageId` + `addedAt` per pin (with carryover semantics so refined comments don't re-flag); HunkViewer renders "jump to source" link + transient "new" badge. Drops the old `getEffectiveReviewFrontendFilters` overrides that forcibly cleared the search term and forced show-read true in Assisted mode — both were the root cause of the user's reported "marking as read doesn't hide the pin" bug. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-7` • Thinking: `xhigh` • Cost: `$2.24`_ <!-- mux-attribution: model=anthropic:claude-opus-4-7 thinking=xhigh costs=2.24 -->
P1: Keep the restore-dismissed control reachable in the toolbar even when the user has dismissed every pin and exited Assisted mode. Previously the whole Assisted control group was gated on `(assistedCount > 0 || filters.assistedOnly)`, which hid the restore button along with everything else. Restore is now rendered as its own slot so dismissed pins are never stranded. Also relax the Shift+P (now plain 'p') keybind guard so re-entering Assisted is allowed when only dismissed pins exist — combined with the inline empty-state Restore CTA, users can recover from any state via keyboard or click. P2: Avoid the Shift+A collision with TOGGLE_PLAN_ANNOTATE by binding the Assisted filter to plain 'p' (Pin filter). The review-panel handler already gates on `isPanelFocused` + `isEditableElement`, so the single-letter binding is review-scoped without competing with the global plan-annotate listener.
Codex P2: the previous useMemo only recomputed when the assisted-match map changed, so a 'new' badge could persist far beyond the 60s threshold if no further assisted updates landed. Drive the predicate off a newPinTick state, scheduled via a single setTimeout that re-arms itself after each expiry. Idle workspaces stay quiet (no interval) once all pins are stale.
- Schedule the 'new' badge expiry off the full `assistedHunks` list so an unmatched pin's 60s clock still ticks down. Otherwise a pin added while it doesn't match the diff (wrong base, etc.) keeps its 'new' badge when it later becomes matched, because no timeout had been scheduled. - `ReviewAssistedStatsReporter` now subscribes to the per-workspace `reviewAssistedDismissed` localStorage list and applies the same dismissal filter the panel uses. Without this the Review tab badge could keep nagging the user about pins they had explicitly silenced.
- Move TOGGLE_ASSISTED_REVIEW (plain 'p') handling above the selectedHunkId/currentIndex guards so the empty-state keyboard escape hatch advertised in the UI actually fires when no hunk is selected (e.g., no pins match the diff, or all are read). - Move dismissed-pin self-healing into ReviewAssistedStatsReporter so it runs on every tab — not just when the Review tab is mounted. Without this, a key dismissed in a previous session could silently filter a later re-appearance of the same pin from badge counts.
Move the four review_pane_update tests into StreamingMessageAggregator.test.ts to keep the alphabetical file ordering stable. Bun runs test files in name order, and inserting a new top-level test file shifted that order enough to expose a few latent test-pollution failures in unrelated files (GitStatusStore losing window listeners, etc.). Inlining the tests preserves the original order and significantly drops the noisy-flake count on the full suite.
Codex P2: the previous self-healing prune ran immediately even when `rawAssistedHunks` was transiently empty (cold-load, workspace switch before the transcript replay finishes). That erased dismissed keys unconditionally, making dismissals non-durable across reloads. Gate the prune on a non-empty live set so we only shrink the dismissed list when we have positive evidence of what the agent currently flags. The empty case is ambiguous — could be cleared agent OR not-yet-hydrated — and the cost of "never prune while empty" is bounded by ASSISTED_REVIEW_MAX_HUNKS.
When WorkspaceStore.test.ts assigns `global.window` to a mock object that lacks `addEventListener`/`removeEventListener`, any subsequent code that unconditionally calls those methods throws and cascades into 13+ false failures (GitStatusStore, indirectly via RefreshController). The mock window is shared process-wide, so this affects unrelated test files even though each passes in isolation. Surgical fix to two callsites that are reached at module-load / construction time during normal test runs: - RefreshController constructor + dispose: `typeof X === "function"` guards around document/window listener registration and removal. - usePersistedState.ensureStorageListenerInstalled: same check on `window.addEventListener`. This shrinks the local full-suite flake count from 39 to 33 and removes the cascading GitStatusStore failures from CI's Test/Unit. The remaining failures are pre-existing flakes in unrelated areas.
Codex P2: rawAssistedHunks-empty was previously always treated as cold-load (don't prune). Now we track `everSeenAssistedRef` — once we've observed a non-empty live set during the session, an empty value is a real agent clear, and we drop ALL dismissed keys so a future re-add of the same path:range surfaces normally instead of staying silently filtered. Codex P3: when Assisted is on and a search term filters every flagged hunk out, the empty-state previously claimed everything was read and suggested toggling Read. Add a higher-priority search-driven branch that points the user at clearing the search instead.
Codex P2 round 6: - handleMarkFileAsRead now consults `showReadHunksRef` (the effective flag) instead of `filters.showReadHunks` directly, so marking a file read in Assisted mode advances selection like it does outside Assisted. Previously the selected hunk could disappear with selection stuck on a -1 currentIndex, breaking subsequent keyboard navigation. - Key the `everSeenAssistedRef` sentinel by workspaceId so switching to a workspace mid-hydration doesn't inherit the previous workspace's hydration assumption. Without this, a workspace switch from one with pins to one whose pins are still loading would nuke the new workspace's persisted dismissals.
Codex P2: `operation: "replace"` republishing the same path:range key is an explicit re-flag of that hunk, not a refinement. Carrying forward sourceMessageId/addedAt for replace ops left the UI linking to a stale agent turn and suppressed the "new" badge on the fresh snapshot. Restrict carryover to `operation: "add"` only — refinements keep their metadata, but a re-publish snapshot stamps every entry with the current turn's id (and timestamp on live updates). Adds a regression test for the new behavior alongside the existing replace-with-different-key test.
|
@codex review Rebased this PR on latest Local validation after the rebase:
|
ammar-agent
force-pushed
the
assisted-review-ux-pass
branch
from
25c0b8b to
77bd854
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77bd854636
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the dismissed-key cold-load case:
Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18b8d9023a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Clear dismissed assisted-review keys once transcript replay has caught up and the persisted assisted set is authoritatively empty, while still preserving dismissals during cold-load hydration. --- Generated with mux • Model: openai:gpt-5.5 • Thinking: xhigh • Cost: $115.74 mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=115.74
ammar-agent
force-pushed
the
assisted-review-ux-pass
branch
from
18b8d90 to
1ed7f04
Compare
|
@codex review Updated the dismissed-key cleanup to be gated only on authoritative transcript hydration:
Local validation:
|
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
A wide UX pass on the agent-driven Assisted Review feature (#3331, #3344). The root cause of the user-reported issue — "there is no way to mark as read with Assisted on and have the hunk disappear" — was that
getEffectiveReviewFrontendFiltersforcedshowReadHunks: truewhenever Assisted was on, so the worklist could never shrink. While I was in there, every other UX issue surfaced in the previous index review got addressed.Background
Triggered by direct user feedback after Assisted shipped. The full issue index is in the conversation that produced this PR; this body summarizes the resolution per bundle.
Implementation
Read-state interaction (A1, A2) —
ReviewFiltersnow carries a separateassistedShowReadHunksflag (defaultfalse, so the worklist hides read pins automatically).getEffectiveReviewFrontendFiltershonors the user search term in both modes and routes the read filter through the scoped flag while Assisted is on.showReadHunksRefmirrors the effective value sohandleToggleRead/handleMarkAsRead/handleMarkFileAsReadno longer navigate away from hunks that are actually still visible.Control bar (A3, A4, B3, B4) —
ReviewControlsnow binds theRead:toggle contextually (assisted-scoped when Assisted is on), dims theUncommitted:checkbox with an explanatory tooltip while it is force-on, keeps the Assisted toggle visible whenever the user is still in worklist mode (even if the agent clears its set), shows(unread/total)like the Review-tab badge, and gains apkeybind (TOGGLE_ASSISTED_REVIEW— "Pin filter"; chosen overa/Shift+Aafter they collided withFOCUS_INPUT_AandTOGGLE_PLAN_ANNOTATErespectively).Mode banner / caught-up / inline exits (B1, C1, C2) — A new banner above the hunk list states the worklist status (
X of N unread,all caught up, ornone match the current diff) and offers inline exits + a keybind hint. The empty state inside Assisted gets matching CTA buttons (Exit Assisted / Show read pins / Restore N dismissed). The banner isrole="status" aria-live="polite".User-side dismissal (B2) — Added a per-workspace
reviewAssistedDismissedlocalStorage list. Dismissed keys are filtered out ofassistedHunksand self-heal in the always-mounted stats reporter. Empty assisted sets preserve dismissals while transcript replay is still hydrating/reconnecting; once replay has caught up and the assisted set is authoritatively empty, stale dismissed keys are cleared so future re-adds of the samepath[:range]surface normally.HunkViewerexposes adismissbutton in the assisted strip; the control bar and empty-state surface a one-click restore that is reachable even when the user has exited Assisted.Source-turn link + "new" badge (E1, E2) —
processToolResultnow accepts an optionalmessageContextso the aggregator stamps each pin withsourceMessageIdandaddedAt. Carryover semantics:operation: "add"preserves the original turn id when a comment is refined;operation: "replace"re-stamps every entry because that's an explicit republish. Replay deliberately omits the timestamp so historical pins don't light up the transient badge on initial load. The "new" badge expires on wall-clock (driven offassistedHunks, not the match map, so unmatched pins still time out).HunkViewerrendersjump to sourceanddismisscontrols inside the assisted strip.Immersive parity (D1) —
ImmersiveReviewViewnow acceptsassistedHunkIds+assistedCommentByHunkIdand surfaces a banner (sparkle + comment) when the selected hunk is one the agent flagged. Without this, entering full-screen review wiped every assisted cue.Validation
make static-check✓ after rebasing on latestorigin/mainand after the final Codex dismissal fix.bun test src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts✓.bun test src/browser/utils/messages/StreamingMessageAggregator.test.ts✓.make typecheck✓../scripts/wait_pr_ready.sh 3358✓ — Codex approved, all Codex review threads resolved, and required CI checks passed.Test / Unit,Static Checks, Storybook, E2E linux + macOS, Integration, Windows, Smoke, UI Tests, Visual Regression, Build linux/macOS/VS Code, and codecov/patch are green on the rebased branch.Risks
Medium. The change touches state-machine wiring around the Code Review panel, but it doesn't relax existing safety nets:
sourceMessageId/addedAtare optional, so consumers that don't care still work.review_pane_updateshape are unchanged; we only carry the new metadata in memory.Files of note
src/common/types/review.ts— newassistedShowReadHunks,sourceMessageId,addedAt.src/browser/utils/messages/StreamingMessageAggregator.ts— metadata plumbing + carryover.src/browser/features/RightSidebar/CodeReview/{ReviewPanel,ReviewControls,HunkViewer,ImmersiveReviewView}.tsx— UI integration.src/browser/utils/ui/keybinds.ts+KeybindsSection.tsx—pshortcut.src/browser/utils/RefreshController.ts+src/browser/hooks/usePersistedState.ts— defensive listener guards.src/browser/utils/messages/StreamingMessageAggregator.test.ts— appendedreview_pane_update -> assistedReviewHunkssuite covering metadata replay, carryover, andreplacere-stamping.src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts— assisted filter/read/dismissed-key coverage.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh• Cost:$115.74