fix(ui): stabilize agent note navigation#251
fix(ui): stabilize agent note navigation#251kevinswiber wants to merge 2 commits intomodem-dev:mainfrom
Conversation
Keep agent-note row geometry stable across viewport changes, resolve comment navigation relative to the full review stream, and keep keyboard navigation callbacks current across selection changes. Use full Pierre hunk spans when matching annotation ranges so saved agent-context anchors inside visible hunk rows remain navigable. Refs modem-dev#233. Refs modem-dev#234. Refs modem-dev#235.
Greptile SummaryThis PR addresses three related agent-note navigation failures: scroll-height instability caused by a viewport-gated note-measurement path, incorrect comment navigation from non-annotated hunks, and annotation anchors falling outside hunk ranges when context rows were excluded.
Confidence Score: 4/5Safe to merge; all three bug fixes are well-targeted, covered by new unit and PTY integration tests, and the one acknowledged tradeoff (all-file note geometry computation) is intentional and bounded by existing windowing. The changes are correctness-oriented with good test coverage. The only non-trivial concerns are the partial ref-ization of keyboard callbacks (two callbacks protected, others left unguarded) and the silent fallback in hunkLineRange to the old narrower count if additionCount were ever absent — both are defensive gaps rather than active defects. src/ui/lib/hunks.ts and src/ui/hooks/useAppKeyboardShortcuts.ts deserve a second look for the navigation boundary semantics and the ref-ization pattern respectively. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User presses } or {"] --> B["useAppKeyboardShortcuts\nmoveToAnnotatedHunkRef.current"]
B --> C["useReviewController\nmoveToAnnotatedHunk"]
C --> D["findNextHunkCursor(annotatedCursors, delta, hunkCursors)"]
D --> E{"current hunk\nin annotatedCursors?"}
E -- Yes --> F["clamp currentIndex + delta\nto annotated list bounds"]
E -- No --> G["findNearestCursorIndex\nusing full stream position"]
G --> H{"currentFileId\nin streamCursors?"}
H -- No --> I["fallback: first or last\nannotated cursor"]
H -- Yes --> J["find nearest annotated cursor\nbefore/after stream position"]
F --> K["selectHunk with scrollToNote=true"]
J --> K
I --> K
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/ui/hooks/useAppKeyboardShortcuts.ts:84-95
**Partial ref-ization of keyboard callbacks**
`moveToAnnotatedHunk` and `moveToHunk` are now dispatched through refs, but the other action callbacks captured by the same keyboard handler closure — `scrollDiff`, `scrollCodeHorizontally`, `cycleTheme`, `toggleAgentNotes`, etc. — are still read from the stale closure directly. If the stated motivation (guarding against OpenTUI changing how it stores callbacks) ever becomes real, these would be vulnerable by the same logic. Either apply the ref pattern consistently to all callbacks or document exactly which ones OpenTUI is known to capture stably so future readers understand the asymmetry.
### Issue 2 of 3
src/core/liveComments.ts:13-14
**Fallback to `additionLines`/`deletionLines` appears unreachable**
`hunk.additionCount` and `hunk.deletionCount` are used without a nullish fallback throughout the codebase (`codeColumns.ts`, `pierre.ts`, `useHighlightedDiff.ts`), which implies Pierre always populates them. The `?? hunk.additionLines` arm silently falls back to the old changed-line-only count, so if `additionCount` were ever absent the function would quietly return a narrower range rather than an error — the same bug this PR is fixing. A comment explaining when `additionCount` can be absent, or an assertion/log, would make the intent clearer.
### Issue 3 of 3
src/ui/lib/hunks.ts:81-83
**Forward `}` from beyond the last annotated hunk jumps backward, not to first**
When the current hunk is not in `annotatedCursors` and its stream position is past all annotated hunks, `indexedCursors.find(...)` returns `undefined` and the fallback resolves to the last annotated cursor — effectively a backward jump. The old code would have jumped to `cursors[0]` (first annotated hunk) in this case. The new behaviour is arguably more useful (nearest rather than wrap-to-start), and the test suite covers it, but it's a visible UX change worth calling out explicitly in a comment for future maintainers.
Reviews (1): Last reviewed commit: "fix(ui): stabilize agent note navigation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Keep navigation shortcuts resilient if the keyboard hook changes how it stores callbacks. | ||
| const moveToAnnotatedHunkRef = useRef(moveToAnnotatedHunk); | ||
| const moveToHunkRef = useRef(moveToHunk); | ||
| const pagerModeRef = useRef(pagerMode); | ||
| const showHelpRef = useRef(showHelp); | ||
|
|
||
| activeMenuIdRef.current = activeMenuId; | ||
| focusAreaRef.current = focusArea; | ||
| moveToAnnotatedHunkRef.current = moveToAnnotatedHunk; | ||
| moveToHunkRef.current = moveToHunk; | ||
| pagerModeRef.current = pagerMode; | ||
| showHelpRef.current = showHelp; |
There was a problem hiding this comment.
moveToAnnotatedHunk and moveToHunk are now dispatched through refs, but the other action callbacks captured by the same keyboard handler closure — scrollDiff, scrollCodeHorizontally, cycleTheme, toggleAgentNotes, etc. — are still read from the stale closure directly. If the stated motivation (guarding against OpenTUI changing how it stores callbacks) ever becomes real, these would be vulnerable by the same logic. Either apply the ref pattern consistently to all callbacks or document exactly which ones OpenTUI is known to capture stably so future readers understand the asymmetry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/hooks/useAppKeyboardShortcuts.ts
Line: 84-95
Comment:
**Partial ref-ization of keyboard callbacks**
`moveToAnnotatedHunk` and `moveToHunk` are now dispatched through refs, but the other action callbacks captured by the same keyboard handler closure — `scrollDiff`, `scrollCodeHorizontally`, `cycleTheme`, `toggleAgentNotes`, etc. — are still read from the stale closure directly. If the stated motivation (guarding against OpenTUI changing how it stores callbacks) ever becomes real, these would be vulnerable by the same logic. Either apply the ref pattern consistently to all callbacks or document exactly which ones OpenTUI is known to capture stably so future readers understand the asymmetry.
How can I resolve this? If you propose a fix, please make it concise.| const additionSpan = hunk.additionCount ?? hunk.additionLines; | ||
| const deletionSpan = hunk.deletionCount ?? hunk.deletionLines; |
There was a problem hiding this comment.
additionLines/deletionLines appears unreachable
hunk.additionCount and hunk.deletionCount are used without a nullish fallback throughout the codebase (codeColumns.ts, pierre.ts, useHighlightedDiff.ts), which implies Pierre always populates them. The ?? hunk.additionLines arm silently falls back to the old changed-line-only count, so if additionCount were ever absent the function would quietly return a narrower range rather than an error — the same bug this PR is fixing. A comment explaining when additionCount can be absent, or an assertion/log, would make the intent clearer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/liveComments.ts
Line: 13-14
Comment:
**Fallback to `additionLines`/`deletionLines` appears unreachable**
`hunk.additionCount` and `hunk.deletionCount` are used without a nullish fallback throughout the codebase (`codeColumns.ts`, `pierre.ts`, `useHighlightedDiff.ts`), which implies Pierre always populates them. The `?? hunk.additionLines` arm silently falls back to the old changed-line-only count, so if `additionCount` were ever absent the function would quietly return a narrower range rather than an error — the same bug this PR is fixing. A comment explaining when `additionCount` can be absent, or an assertion/log, would make the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.| if (delta >= 0) { | ||
| const nextCursor = indexedCursors.find(({ streamIndex }) => streamIndex > currentStreamIndex); | ||
| return nextCursor?.index ?? indexedCursors[indexedCursors.length - 1]!.index; |
There was a problem hiding this comment.
} from beyond the last annotated hunk jumps backward, not to first
When the current hunk is not in annotatedCursors and its stream position is past all annotated hunks, indexedCursors.find(...) returns undefined and the fallback resolves to the last annotated cursor — effectively a backward jump. The old code would have jumped to cursors[0] (first annotated hunk) in this case. The new behaviour is arguably more useful (nearest rather than wrap-to-start), and the test suite covers it, but it's a visible UX change worth calling out explicitly in a comment for future maintainers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/lib/hunks.ts
Line: 81-83
Comment:
**Forward `}` from beyond the last annotated hunk jumps backward, not to first**
When the current hunk is not in `annotatedCursors` and its stream position is past all annotated hunks, `indexedCursors.find(...)` returns `undefined` and the fallback resolves to the last annotated cursor — effectively a backward jump. The old code would have jumped to `cursors[0]` (first annotated hunk) in this case. The new behaviour is arguably more useful (nearest rather than wrap-to-start), and the test suite covers it, but it's a visible UX change worth calling out explicitly in a comment for future maintainers.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
This is a combined fix for the agent-note navigation failures I hit while reviewing a live
--agent-contextsession:{/}andhunk session navigate --next-comment/--prev-commentrelative to the full review stream when the current hunk is not itself annotatedagent-contextanchors inside a hunk remain navigableFixes #234.
Fixes #235.
Refs #233.
Relationship to the other open PRs
There are three related open PRs. This PR overlaps with them, but it is not identical.
queueMicrotaskto avoid setState during OpenTUI scrollbar/layout event emission. This PR does not include that direct viewport-loop guard, so it should not close Maximum update depth exceeded under rapid viewport changes (scroll or hunk-jump) #233. What this PR does for Maximum update depth exceeded under rapid viewport changes (scroll or hunk-jump) #233 is indirect: it removes the agent-note height oscillation that can feed the same update loop.DiffPanechange: measure geometry with all notes, but keep rendering on the viewport-restricted note set. This PR fixes the same root cause by removing the split source entirely: geometry and rendered sections both receiveallAgentNotesByFile. That is simpler and matches the one-source-of-truth direction, but it has a possible rendering-work tradeoff for visible/prefetched sections with many notes.hunkLineRangeto use the full hunk header span. This PR includes that same category of fix, but also fixes a separate next/prev-comment keybinding jumps backward when selection is on a non-annotated hunk #235 path: when the current selected hunk is not annotated,{/}used to fall back to the first/last annotated hunk instead of resolving the next annotated hunk relative to the current full-stream position.What is worth keeping if the other PRs land first
If #242, #243, and #244 land before this PR, I can rebase and narrow this branch. The part that still appears worth merging from this PR is:
src/ui/lib/hunks.tsuseReviewController.ts,reviewState.ts)src/ui/lib/hunks.test.ts}navigation through anagent-contextsidecarThose pieces are not reflected in #242, #243, or #244 as currently written.
Optional pieces after a rebase:
DiffPanegeometry work should be dropped if maintainers prefer fix(ui): use full agent-note set for section geometry measurement #243's narrower measurement-only approach.hunkLineRangeconsolidation should be dropped if fix(core): use header counts for hunkLineRange so context lines are in range #244 lands first, unless maintainers prefer this branch's shared-helper shape.Repro Steps
Bottom-edge snap-back with notes (#234)
hunk diff <range>.Before this change, the viewport can snap upward by roughly the height of off-top note rows because total content height changes with the viewport.
Next/previous comment from a non-annotated hunk (#235)
}a couple of times to move through notes.]so the selected/centered hunk is not one of the annotated hunks.}again.Before this change, the fallback path in
findNextHunkCursortreats the current hunk as absent from the filtered annotated cursor list and jumps to the first annotated hunk instead of the next annotated hunk after the current stream position.Context-row anchors skipped by comment navigation
}orhunk session navigate --next-comment.Before this change,
hunkLineRangeusedadditionLines/deletionLines, which count changed lines only, not the full hunk header extent. Anchors on context-adjacent rows could then fall outside the hunk range and never enter the annotated cursor list.Fix
Stable agent-note geometry
DiffPanenow derivessectionGeometryfrom the full per-file note map rather than a viewport-restricted note map. That keepstotalContentHeightindependent ofscrollViewport.top, so bottom-edge clamping has a stable ceiling while scrolling.Stream-relative annotated navigation
findNextHunkCursornow accepts the full stream cursor list. If the current hunk is absent from the target subset, annotated navigation finds the nearest target cursor before/after the current stream position instead of falling back to the first/last annotated cursor.When the current stream position is outside the annotated span, comment navigation clamps to the nearest annotated edge instead of wrapping. That matches normal hunk navigation and is now documented/tested explicitly.
Both the React review controller and session command path pass the full stream into that helper.
Full hunk-span range matching
hunkLineRangenow uses Pierre's required header span counts (additionCount/deletionCount).agentAnnotationsuses that shared helper, so live comments, sidecar notes, and annotated-hunk discovery all agree on hunk extent.Review follow-up
After Greptile review, this branch drops the defensive keyboard callback refs rather than expanding that pattern. OpenTUI's current
useKeyboardalready keeps the latest handler viauseEffectEvent, and the refs were not part of the root fix.Tradeoffs
Testing
Passed on current branch head:
bun run typecheckcurrently fails onmainitself, independent of this branch:I left that unrelated base issue out of this PR. I can rebase once
mainis fixed.