feat(review): select current-line sides and add named sidebar policies - #676
Draft
rupert648 wants to merge 20 commits into
Draft
feat(review): select current-line sides and add named sidebar policies#676rupert648 wants to merge 20 commits into
rupert648 wants to merge 20 commits into
Conversation
`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.
|
@rupert648 is attempting to deploy a commit to the Modem Team on Vercel. A member of the Team first needs to authorize it. |
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
3 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
h/lkeyboard selection for the old and new sides of the current split rowcanchor an exact line comment on the selected sideauto,shown, orhidden, with boolean config values retained as compatibility aliasesStacked dependencies
This draft intentionally preserves and integrates the commits from #662 and #648 so the combined behavior can be used and tested now. Original authorship is preserved. The new delta is commit
0d27fa8.This should be rebased after those pull requests land so the final review contains only the side-selection and named-policy delta.
Validation
script -f, which Applescriptdoes not support