feat(review): mark the current line and anchor notes to it - #662
Conversation
|
@loganthomas is attempting to deploy a commit to the Modem Team on Vercel. A member of the Team first needs to authorize it. |
564481e to
c31e0bf
Compare
Examples
|
|
Added to menu bar as well (toggle without having to pass command line args) 
|
5b14f55 to
8c01f97
Compare
|
@loganthomas Dude, figuring out the UI for this has been my biggest struggle - I love how high quality this PR is, that you explored a bunch of different options, etc. |
|
This was AI generated. I checked this out and exercised it in tmux. The precise note anchoring and
I think the navigation model should come from the same rendered plan/geometry used for display and note placement, with note creation always revealing its target. This comment was generated by Pi using OpenAI GPT-5.3 Codex |
|
@benvinegar sound good! I'll pick this up Monday. Excited to help on this cool project, thanks for building! |
|
NP!
Aside, I'm thinking if you navigate by page we just have to choose a new line somewhere (e.g. middle of viewport). There's a bunch of UX considerations like this. (This is why I was scared to tackle it, aha.) |
4c65721 to
769a674
Compare
|
Thanks for digging into this, the tmux pass was really helpful. Fixed:
You were right about the root cause too. Navigation now comes from the measured render plan instead of a separate walk over hunkContent, so the ordering problems can't really come back. Three I couldn't reproduce on the current branch:
I think the branch got rewritten after your review ran, so you may have been looking at an earlier push. Not trying to wave them off. On your paging note, I went with snapping to the nearest visible line instead of the middle of the viewport. It still picks a line every page, and it has the nice side effect of leaving the marker alone when you nudge the wheel one line. One other change: note cards now show up below the line they're attached to instead of above. And a question, since you mentioned there are a lot of UX calls like this. Expanding a gap pushes the hunk and the marker off the bottom of the screen. That happens on main too, but the marker makes it obvious. Right now |
8cf9957 to
0b5a499
Compare
|
PR author is not in the allowed authors list. |
I think it's less confusing if the active line stays where it is (vs jumping). Or let's just start with that and see if it needs to change after? |
|
Actually let's just keep it the way you have it. If it's irritating we'll learn and find out. LGTM! |
`hunks.ts` flattens the review stream into hunks for `[` and `]`. This does the same one level down, so a current-line cursor and the line a note anchors to can come from one list. Targets are ordered to match the rows the active layout draws: split pairs a change block's two columns per row, stack renders one column then the other.
Hunk reveal biases its target a quarter screen from the top, which is right for a jump but yanks the viewport when it runs on every step key. This moves the shortest distance that puts a line on screen, and stays put when it already is.
The marker shifts luminance rather than hue. Blending toward one fixed highlight color barely moves a background that already shares its hue, which left the row invisible on additions, and reading the transparent sentinel as a color painted an opaque band on light themes. Cells now take one resolved highlight instead of a `selected` flag, a column range and a blender that had to be kept in agreement by hand. Copy selection and the current line resolve through the same path, so a drag keeps its exact extent and the cursor falls back to the row.
Line and hunk are two granularities of one review position, so the cursor lives next to `selectedHunkIndex` and the two follow each other: moving the line carries the selection, and `]` or the sidebar re-seeds the line. Reads go through a ref because a held key drains as one stdin chunk, and batched state would leave every press in the burst seeing the same row.
`row` marks the whole row, `number` marks only the line number, and `off` removes the marker. Saved with the other view preferences, and settable per run with `--cursor-line`.
Reviewers had no way to tell which line they were on, and `c` always anchored a note at the first line of the selected hunk, so a line-precise note needed the mouse. `j`/`k` now move the line and `c` anchors at it, with a live mouse hover still taking priority. `cursor_line = "off"`, or a changeset with no navigable lines, falls back to scrolling the viewport a row at a time. Closes modem-dev#553 Closes modem-dev#436
Asserts the painted spans for each style across themes, and drives a real PTY for the two things frame assertions missed: a held key advancing one row per press, and a note anchoring where the marker sits. Scroll guards that measured one row per step key now run with the marker off, which is the mode they were written for. Split pairs both sides of a change row, so loops that counted presses per row needed twice as many.
Row anchors were write-only: the plan wrote them, but nothing could read a source line back out of one. Line navigation needs that inverse so it can walk measured rows instead of re-deriving positions from the parsed diff.
Stops were enumerated from hunkContent, a second model of the same rows the pane draws. It disagreed with the render plan wherever the two were derived differently: lines revealed by expanding a collapsed gap were unreachable, so one step could skip an entire gap. Walking the measured rows removes the disagreement by construction, and lets every stop carry the plan anchor that rendering, reveal, and note placement already key on. The pane that measures the stream publishes the stops, mirroring how it already reports viewport-driven selection.
Paging and the wheel moved the viewport without the marker, so it could sit off screen; the next step then yanked the view back to wherever it had been stranded, and a note started there opened out of sight. The marker now follows only once it would otherwise leave the viewport, landing on the nearest row it left through, so a one-line wheel tick still leaves it alone. Selection follows the marker rather than the viewport centre, so the two cannot disagree about where the reviewer is.
…ions Extension file views measured their rows only under their own row ids, so a cursor addressed by source line resolved to nothing: stepping through such a file moved an invisible marker and scrolled nowhere. Presentation rows bound to a single source line now carry that line's anchor alongside their own, which is all the raw-diff machinery needs to walk them, paint them, and scroll to them. It also holds the marker in place when a draft note forces the file back to raw diff.
Starting a note scrolled to the first inline card in the hunk, which is a different card whenever the hunk already carries one, so on a tall hunk the editor could open off screen. Opening a draft also restores the viewport anchor to stop the code jumping, and that restore overrode the reveal entirely, leaving the bottom of the card cut off on a short terminal. The reveal now addresses the draft's own row, and the anchor restore gives up the minimum height needed to show all of the card while still holding the annotated line still.
Cards rendered above their anchor, so a note read as a preamble to the line it was about rather than a response to it. Every mainstream diff tool puts a comment under its line, and that is where reviewers look for it. Agent annotations, live comments, saved notes, and the open draft all move together in both the raw diff and alternate presentations, so a note does not jump the moment it is saved.
The current-line work reworded existing rows and sentences that were still accurate, which is churn in someone else's docs. Fold what the branch actually needs into the paragraph it adds and leave the rest as written.
Nothing reads the stop list with the marker disabled, but it was still rebuilt on every remeasure — one object per rendered row of the whole changeset, about 39 ms and 68k allocations on a 200-file diff, repeated on each resize.
Expanding a gap is a request to read what it hid, but the marker stayed on the hunk while the revealed rows pushed it off screen, so `z` cost you your place. It now moves to the first revealed line, and collapsing puts it back where it started. The restore only applies when the collapse actually retires the row the marker is on, so stepping clear of the gap first is not undone. Expansion remeasures before its source text loads, so the request is recorded by the toggle and resolved against the list that carries the revealed rows.
Merges the duplicates the feature accumulated: one cached row-background blend instead of two copies, one partition search instead of two, one cursor-row predicate shared by both renderers, one owner for the inline-note anchor format, and one pending-request ref instead of two that had to be kept mutually exclusive by hand. Stepping now resolves through an index rather than scanning every row in the changeset per keypress, and the gap toggle goes back to deriving state inside its updater instead of writing through the mirror. Drops the commentary that restated the code, keeping only what documents a constraint a later change could break.
6960bc6 to
47a28ce
Compare
2 participants
Closes #553. Closes #436.
Behavior
j/k(and↑/↓) move it. The view scrolls only far enough to keep it visible.canchors a note at that line instead of the top of the hunk. A live mouse hover still wins, so the pointer flow is unchanged.[/]and sidebar selection in both directions.cursor_line=row(default, whole row) |number(line-number gutter only) |off. Settable in config, with--cursor-line <style>, or from the View menu.offrestores today'sj/kscrolling exactly.On
j/kAGENTS.mdsays not to reintroducej/khunk navigation. This is line navigation, which is what #436 asks for; one commenter there cannot use a mouse.offopts out, and the PTY guards that asserted one-row step scrolling now run in that mode, keeping their original coverage.Say the word and I'll flip the default to
off.Implementation
Hunk already resolves a line-precise note target; it just expires ~2s after the pointer moves. This makes it persistent and keyboard-driven.
src/ui/lib/lineCursors.tsmirrorshunks.tsone level down, memoized per parsed file so a keypress does not reallocate the list.useReviewControllerowns the cursor beside the existing selection, minting a reveal id likeselectHunkdoes.RowHighlightper cell (blend plus optional column span) and hands it to the existing cell renderers, replacing the oldselected+selectionColRangepair. No new theme tokens. The cursor row is found by the render plan's ownline:<hunk>:<side>:<n>key, so windowing needs no special case.The marker shifts luminance, not hue: blending toward one fixed highlight color barely moves a background that already shares its hue, which left it invisible on added rows.
AppHost.cursor-line.test.tsxasserts the painted backgrounds, including that added stays green and removed stays red under the marker.Known gaps
Targets come from
hunkContentrather than the render plan, so lines revealed by expanding a collapsed gap are not reachable byj/k.zstill expands the gap at the cursor's hunk. Same for files shown through an extension file presentation, which have noline:anchors. Both are called out in the module docstring; happy to follow up with a plan-derived target list if you'd prefer that before merge.Verification
format:check,lint,typecheck,check:docs,test,test:integration,test:tty-smoke,build:npm,check:pack. Docs updated: keybindings table, keyboard reference, notes guide, display config, plus regenerated CLI/config reference.Pre-existing failures on
mainate0d1757, unchanged by this PR (verified by stashing it):AppHost.watch.test.tsxtheme resolution, and 5 PTY extension/file-view tests.No true interactive TTY run: no controlling terminal here. Instead the real painted spans are asserted via
captureSpans()acrossrow/number/off, split and stack, light and dark.