🤖 fix: surface assisted review hunks reliably#3344
Merged
Conversation
Ensure agent-pinned review hunks remain visible when Assisted mode is active, even with stale selected-file, read, search, or uncommitted-change filters. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `$6.49`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=6.49 -->
Collaborator
Author
|
@codex review |
![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: 6526a9b41d
ℹ️ 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".
Fetch assisted review diffs from each pinned path's owning repo root so multi-project pins remain visible even when the Review pane was rooted to another project. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `$6.49`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=6.49 -->
Collaborator
Author
|
@codex review Please take another look. I grouped Assisted-mode diff requests by each pinned path's owning repo root so multi-project pins are fetched from the correct checkout. |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
mux-bot Bot
pushed a commit
that referenced
this pull request
May 21, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
mux-bot Bot
pushed a commit
that referenced
this pull request
May 21, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
mux-bot Bot
pushed a commit
that referenced
this pull request
May 21, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
mux-bot Bot
pushed a commit
that referenced
this pull request
May 21, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
mux-bot Bot
pushed a commit
that referenced
this pull request
May 22, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
ammario
pushed a commit
that referenced
this pull request
May 22, 2026
## 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 `getEffectiveReviewFrontendFilters` forced `showReadHunks: true` whenever 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)** — `ReviewFilters` now carries a separate `assistedShowReadHunks` flag (default `false`, so the worklist hides read pins automatically). `getEffectiveReviewFrontendFilters` honors the user search term in both modes and routes the read filter through the scoped flag while Assisted is on. `showReadHunksRef` mirrors the effective value so `handleToggleRead`/`handleMarkAsRead`/`handleMarkFileAsRead` no longer navigate away from hunks that are actually still visible. **Control bar (A3, A4, B3, B4)** — `ReviewControls` now binds the `Read:` toggle contextually (assisted-scoped when Assisted is on), dims the `Uncommitted:` 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 a `p` keybind (`TOGGLE_ASSISTED_REVIEW` — "Pin filter"; chosen over `a`/`Shift+A` after they collided with `FOCUS_INPUT_A` and `TOGGLE_PLAN_ANNOTATE` respectively). **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`, or `none 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 is `role="status" aria-live="polite"`. **User-side dismissal (B2)** — Added a per-workspace `reviewAssistedDismissed` localStorage list. Dismissed keys are filtered out of `assistedHunks` and 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 same `path[:range]` surface normally. `HunkViewer` exposes a `dismiss` button 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)** — `processToolResult` now accepts an optional `messageContext` so the aggregator stamps each pin with `sourceMessageId` and `addedAt`. 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 off `assistedHunks`, not the match map, so unmatched pins still time out). `HunkViewer` renders `jump to source` and `dismiss` controls inside the assisted strip. **Immersive parity (D1)** — `ImmersiveReviewView` now accepts `assistedHunkIds` + `assistedCommentByHunkId` and 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 - Local: `make static-check` ✓ after rebasing on latest `origin/main` and after the final Codex dismissal fix. - Local targeted: `bun test src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts` ✓. - Local targeted: `bun test src/browser/utils/messages/StreamingMessageAggregator.test.ts` ✓. - Local: `make typecheck` ✓. - PR readiness: `./scripts/wait_pr_ready.sh 3358` ✓ — Codex approved, all Codex review threads resolved, and required CI checks passed. - Hosted CI: `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: - The aggregator change is additive — `sourceMessageId`/`addedAt` are optional, so consumers that don't care still work. - Tool persistence and the existing `review_pane_update` shape are unchanged; we only carry the new metadata in memory. - Dismissed pins live in localStorage with hydration-aware self-pruning so reconnect/full-replay placeholders don't drop user dismissals, while truly stale dismissed keys still clear after replay catches up. - The biggest behavioral shift is the read filter: marking an Assisted pin as read **will** now hide it by default. That is the explicit fix the user asked for; the worklist-scoped toggle is the escape hatch. ## Files of note - `src/common/types/review.ts` — new `assistedShowReadHunks`, `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` — `p` shortcut. - `src/browser/utils/RefreshController.ts` + `src/browser/hooks/usePersistedState.ts` — defensive listener guards. - `src/browser/utils/messages/StreamingMessageAggregator.test.ts` — appended `review_pane_update -> assistedReviewHunks` suite covering metadata replay, carryover, and `replace` re-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`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=115.74 -->
mux-bot Bot
pushed a commit
that referenced
this pull request
May 22, 2026
The single-result `buildReviewDiffPathFilter` wrapper was exported from ReviewPanel.tsx solely so two assertions in ReviewPanel.assistedStats.test.ts could grab the first spec's pathFilter. Production callers (ReviewPanel's diff-load effect and ReviewAssistedStatsReporter) already use the multi-spec `buildReviewDiffPathFilterSpecs` form directly so they can handle assisted multi-project pins. Move the wrapper into the test file as a private helper. Pure relocation — identical implementation, same two describe-block assertions, same buildReviewDiffPathFilterSpecs delegation — so the ReviewPanel module's exported surface drops one helper that no production code ever called. Picked from the `fix: surface assisted review hunks reliably` change in #3344, which introduced the test-only wrapper alongside the production multi-spec form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes Assisted Review visibility parity-by-construction: once
review_pane_updateaccepts a pin, the Review sidebar fetches the pinned paths from their owning repo roots and ignores stale user filters that could otherwise hide those hunks.Background
Agents can flag specific review hunks, but the sidebar could still render an empty Assisted view when unrelated UI state was active: a previously-selected file path, hidden read hunks, a search term, an off include-uncommitted toggle, or a multi-project workspace rooted to a different repository. That made successful tool calls appear as if they had no sidebar effect.
Implementation
Validation
bun test src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts src/common/utils/review/assistedReview.test.ts src/node/services/tools/review_pane.test.tsmake typecheckmake lintmake static-checkRisks
Low-to-moderate: the change is scoped to Review sidebar Assisted mode, but it intentionally overrides user visibility filters while Assisted is enabled so accepted agent pins cannot be hidden.
Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh• Cost:$6.49