feat(diff): render the diff like GitHub, unified and side by side - #50
Merged
Conversation
The reported problem is the `+`/`-` at the head of every line. Investigation found a second cause behind it, and a larger one: `TextDecoration` paints a background from the first glyph of a range to the last, so the addition and deletion tints follow the ragged edge of the text instead of filling the row, and a blank added line gets no background at all. The markers carry the signal because the colour only half does. Both follow from the diff being one text document in a code editor rather than a list of elements, and that cannot be worked around: at the pinned gpui-component revision the gutter is fixed to `buffer_line + 1` in a single column, there is no full-width row background outside the cursor line, and there is no per-line element injection. Two of the three things the editor bought back in `5cc1caa` are recoverable — `virtual_list` for virtualisation, and `gpui-base`'s window-level selection, already mounted through `Root`, for selection. The spec records the evidence for each so the next person does not re-derive it.
Reading the real signatures overturned two claims made from the shape of the problem rather than from the code. Virtualisation cannot be delegated to `virtual_list`. That list yields one element per row, while a selection participant declares every run in a single `update_runs` call and gets one range back per run. Per-row elements on a shared handle would each overwrite the last; a handle per row is an `Entity` per visible line, rebuilt on every scroll. The body becomes one element that windows and paints its own rows. Dropping soft wrap was justified by virtualisation, and that was wrong: `v_virtual_list` takes one `Size` per item and accepts varying heights. The choice survives on fidelity to GitHub and on the cost of measuring a wrapped height per row per viewport width, but it is a product decision now and reversible on its own. Also records the panic hazard on `TextLayout`: bounds, line_height, len, position_for_index and index_for_position unwrap an inner cell filled during prepaint, so they are callable from paint and nowhere earlier.
Seven tasks. Four are pure and carry real tests — row derivation, left/right pairing, the palette, and the persisted view mode. Three touch gpui and are verified by running the app, because this crate has no element-tree tests and the plan does not add that machinery. Ordering follows the spec's risk: the selectable body element comes before the split view, so that if the participant-and-runs model does not behave as read, it fails before a second view is written on top of it. The self-review caught one requirement with no task behind it. The spec calls for horizontal scrolling instead of soft wrap, and nothing enforced it — `StyledText` wraps to whatever bounds it is handed, and a wrapped row would also break the fixed row height the windowing arithmetic depends on. That is now a step of its own, including the `restrict_scroll_to_axis` flag whose default sends vertical gestures sideways.
The editor fixed three things this view has to control: its gutter is always `buffer_line + 1` in one column, so a GitHub old/new pair is unreachable; its only full-width row fill is the cursor line, so an addition's tint hugged the glyphs and a blank added line got none; and it has no per-line element hook to work around either. The `+`/`-` markers were carrying the signal the colour only half carried. All three follow from the diff being one text document, so `DiffBody` paints the rows itself. Selection comes back from gpui-base's window-level participant system, which `Root` already mounts. The body registers one participant and declares one run per row, and only the code text becomes a run — the gutters and the marker are shaped and painted directly, which is what keeps line numbers and markers out of the clipboard. Two things that look like details decide whether it works at all. A participant's runs are concatenated with *no* separator when the window resolves a copy; only whole participants are joined by a newline. One participant per row would give the right separator and cost an `Entity` per line, so the join is done here and handed back through `set_fallback_copy_text`. `update_runs` is what sets the projection; `set_fallback_copy_text` clears it right back off, which is what lets `copy_item` fall through to our joined text instead of the unseparated one. The span runs from the first selected row to the last rather than over the selected rows alone, because a blank row inside a selection projects to no range and dropping it would close a gap the user can see. And every row is laid out against the widest row's measured width, not the viewport's. `StyledText` wraps to the bounds it is given, and a wrapped row is taller than `ROW_HEIGHT` — which every position in this element is derived from. The container scrolls both axes over that width. `restrict_scroll_to_axis` earns its place for a narrower reason than it looks: both axes are already `Overflow::Scroll` here, so gpui's vertical-onto-horizontal remap never applies with or without the flag; what the flag does is axis-lock a precise trackpad gesture that would otherwise drift diagonally. `TextLayout::bounds`, `line_height`, `position_for_index` and `index_for_position` all unwrap a cell filled during prepaint, so they are called from `paint` and nowhere earlier. `format`'s text reconstruction and the decorations it fed go with the editor.
The body built a StyledText for every row of the patch on every frame, then declared a TextSelectionRun for every one of them. On a commit touching hundreds of files that is thousands of laid-out lines per frame for a viewport that shows forty. The window is decided in request_layout rather than in paint, because laying a row out is the expensive half and nothing downstream can be narrowed without it. The ScrollHandle is the only input available before the frame hands the element any bounds; the range is stored, so request_layout, prepaint and paint cannot disagree when the container clamps the offset in its own prepaint. Narrowing layout forces the runs to narrow too: selection_range_for_run reads layout.len() on every run it is handed, and TextLayout::len expects on a cell the measure closure fills, so a row that was skipped this frame cannot be declared at all. Reading the copied text off that projection would then return only the rows that happened to be on screen — a selection dragged past the bottom edge would come back cut short with nothing to say it had been. So the copy is derived from the selection's own window points instead. Those survive scrolling: gpui-base stores an endpoint as position - bounds.origin, and this element's bounds.origin already carries the scroll, so a point above the viewport is a negative y rather than a lost one. The row span is then arithmetic, every row between the two ends is whole, and only the two rows the selection cuts through need shaping. A row already on screen keeps the projection's own range, so the highlight and the clipboard cannot drift apart. TextSelectionContentKey was the obvious lead and turns out to be a channel rather than a mechanism: gpui-base resolves it once from a participant callback and hands it straight back on the snapshot, taking no part in hit-testing, projection or copying. With a fixed ROW_HEIGHT a row index is y / ROW_HEIGHT, which is the identity a key would have carried. content_width still shapes every row. It has to: the horizontal scroll extent must consider rows that are off screen or the scrollbar resizes as the view scrolls vertically, and the children are laid out against a width that provably exceeds every row's natural width, which is what keeps a row one line tall. After the first frame those are hits in gpui's line-layout cache.
selected_range took its ShapedLine by value, so pen.measure ran for every off-screen row in the selection span — including the whole-row middles whose shaped result was then thrown away in favour of 0..text.len(). The claim that only the two rows the selection cuts through get shaped was made in four places and was true in none of them. Taking the line as FnOnce and answering Band::Whole before calling it makes the claim true rather than requiring four retractions, and roughly halves the per-frame cost while a large selection is live. The (f32::MIN, f32::MAX) sentinel pair went with it. A band is now an enum over the four cases the rule actually has, so the whole-row case is something to match on rather than something to recognise by its bounds, and Band::holds is exhaustive instead of relying on the sentinels behaving as open bounds. Also corrects the panic citation in the module doc and the design spec. The four TextLayout accessors do not agree on either the mechanism or the cell: len and line_height unwrap the cell the measure closure fills, while bounds, position_for_index and index_for_position need the one prepaint fills as well. Only position_for_index lives at text.rs:864-871, which the doc cited for all four. That len needs only measurement does not soften the constraint, because selection_range_for_run reads it before any geometry and a row skipped at request_layout has no measure cell either. Three tests, all covering branches nothing reached: an upward drag, which is the arm that sorts the endpoints by y and was the failure mode the review was asked to chase; a view over-scrolled past the top, which is the .max(0.) clamp; and a whole row, whose shaper is an unreachable! so removing the short-circuit fails the suite rather than quietly costing a frame.
DiffBody now paints a row as N cells rather than as one string, with N fixed per view — one for unified, two for split. That uniformity is the whole design: a cell is row * columns + column, so every index the element already kept over rows converts to cells by arithmetic instead of by a lookup table, and the three phases that had to agree on a visible range still agree on one range derived one way. A file, hunk or placeholder row keeps a single full-width cell in either view rather than being split into a text half and an empty half, which is what makes the column count uniform in the first place; content_width measures every cell and divides by the column count, so a full-width header still fits inside the half it is drawn in and a row stays one line tall. The copy path needed a decision, because a row now has two texts. It joins columns with a tab and rows with a newline. Newlines between columns were rejected: gpui-base's band rule takes both cells of every row a selection passes through whole, so a drag down the right column copies the left one too, and interleaving them would repeat every context line. Copying only the column the drag started in was rejected for a harder reason — the highlight comes from the projection, so dropping a cell the projection selected is exactly the drift between clipboard and screen that deriving the copy from geometry exists to prevent. Neither separator yields code you can paste into a file; the tab at least yields the table on screen. The arithmetic and the projection still agree cell for cell. A selection band is a property of the row, so both cells of a row share it and only the cell's own column offset turns it into a byte range — which is what point_in_selection_band does to two runs that share a y, since it tests a character's midpoint against a band derived from that character's own line. Band::Whole still short-circuits before the shaper. The toggle is a second segmented bar beside the existing one, shown only on the Diff tab. save_diff_view_mode blocks on file I/O, so it runs on the background executor the way the theme preference does; the mode is read once in DetailPanel::new. The bar's container gains .flex() — it already carried items_center and gap_2, which were inert under the default block display. split_rows is the only new pure logic and carries the tests. model.rs gives up its file header, placeholder and hunk header construction to functions both row builders call, so the two views cannot drift on what a header says.
A selection endpoint is stored relative to bounds.origin (text_selection.rs:1336), which is what makes it survive scrolling — and also what makes it survive the content being replaced. Toggling to split view or picking a different commit left the stored y resolving onto whatever row now sits at that offset: a highlight over lines nobody dragged across, and a Cmd-C that copies them. Highlight and clipboard still agreed with each other, so this was not the drift the geometry-derived copy guards against, but it is surprising in a way nothing on screen explains. set_detail and set_diff_view_mode now share reset_diff_view, which zeroes the diff scroll offset and calls TextSelection::clear. Both take a &mut Window for it, which is why sync_panels_from_repository takes one too; both of its callers already had one in scope. The clear is window-wide rather than per-participant because TextSelectionHandle exposes no clear of its own and WindowSelectionState is private — which costs nothing here, since the diff body is the only participant this crate registers. Also stops the column rule crossing full-width rows. The loop ran outside the match on whether a row has one cell or two, so a 1px border cut through every file and hunk header; GitHub draws those bands unbroken. Moving it into the two-cell arm is the whole fix. The split-view mirror case: a pure deletion leaving the right column empty was the only shape of pairing.rs's fan-out that split_rows did not assert for itself.
…frame The spec's data model says both row lists are derived when the patch or the view mode changes, never per frame. `render` did the opposite: it ran `rows` or `split_rows` on every frame and `body` then built one `SharedString` per cell from the result. That is not a rare frame — `DetailPanel::new` attaches `refresh_window_on_change`, so a selection drag repaints on every mouse move and copied every line of the patch two or three times over each time. `DiffContent` holds the rows and their cell text together, `DetailPanel` rebuilds it in `set_detail` and `set_diff_view_mode` alone, and the element takes it by `Rc`. That is also what `content_width`'s justification wanted all along: the per-frame cost is now a hash against gpui's line-layout cache over bytes that are already there, with no allocation behind it. Three more things follow the rows: `Row::FileHeader` carries the file's `FileStatus` and its two line counts, as the spec wrote it. Dropping the status made a rename render as `new.rs +0 -0` above "No content changes." with no way to see what the old name was; the header now reads `old.rs -> new.rs renamed 87% +0 -0`, on one line. The unmeasured-viewport window is clamped to a screenful. `visible_rows` reads `ScrollHandle::bounds`, written during the container's prepaint and so still unset when this element's `request_layout` first asks, and answering `0..rows` there laid out, prepainted and painted every row of the patch on the first frame the Diff tab is shown. Frame two corrects it either way. `bounds_for_cell`, `column_left` and `column_width` become free functions over `(bounds, columns, code_left, cell)`. They were methods on an element that holds a `TextSelectionHandle`, so nothing could reach them without an `App`, and neither could `cell_text`, `row_text`, `marker`, `row_background` or the `Rows` accessors — the unified/split cell mapping added last, untested. All of it is tested now.
Three things were lost with the editor and none of them failed loudly. Nothing handled `SelectAll` any more, so the menu item was inert over the diff. `DetailPanel` registers one, and marks the participant as locally selected (`set_local_selection`) rather than faking a geometric selection: there are no window points behind Cmd-A, so `copy_selection` answers the whole document directly and every visible cell is highlighted whole. A drag clears it through `TextSelectionEvent::Cleared`, which `begin_impl` raises before it takes a new anchor, and so does the frame sweep when the Diff tab goes away. The event rather than `clear_with`, because `clear_with` fires synchronously from inside `TextSelection::clear` — which `reset_diff_view` calls while the panel is already leased, so updating the panel from there would panic. `Copy` is registered here too, and has to be: `gpui_component::Root`'s handler trims the string it copies (`root.rs:552-555`), which silently eats the indentation of the first selected line. A descendant of `Root` in the dispatch tree wins over it, which is what `track_focus` on the panel's own div buys — `TabPanel` tracks this panel's focus handle on a node above it, so without one of our own the panel is never on the focus path. Nothing drove `TextSelectionEvent::AutoScroll`, so a drag past the viewport edge stopped instead of scrolling. Subscribing is half the fix: the registered rectangle is what `AutoScroll::compute_delta` measures the pointer against, and this element registered its own bounds, which are the whole diff rather than the part of it on screen, so the trigger zone sat off-screen and no delta was ever produced. It now registers the viewport and reports `bounds.origin - viewport.origin` as the scroll offset. `gpui-base` stores an endpoint as `position - bounds.origin - scroll_offset` and resolves it by adding both back, so the sum is what matters and this sum is the element's own origin by construction — every stored endpoint is bit-identical to before.
…the code `LineColors::foreground` was `theme.foreground` for every origin, so the field and the `line_colors(origin, ..)` signature promised something that did not exist and the `+` and `-` markers were painted in the code's own colour. They now carry `theme.green` and `theme.red`, the tokens the reference badges use, with `theme.muted_foreground` for context; the code stays `theme.foreground` whatever its origin, which is where the marker colour lives in a diff that no longer highlights by role. That also fixes what the two dark constants were justified against. The note argued their legibility against Catppuccin Frappé's addition and deletion syntax colours, and no diff line has been painted in those since the editor went. Measured against `#c6d0f5`, the foreground the code is actually drawn in, the plates give 5.11:1 and 6.20:1 — both better than the numbers the note claimed, and both true. The visibility half of the argument, 1.58:1 and 1.30:1 against Frappé's `#303446`, never depended on the text and stands. `light_mode_uses_the_exact_given_hex_values` comes back with them. It pinned GitHub's two values and was dropped along with the vacuous test beside it when `decorations.rs` was replaced.
`metadata.rs` still explained its header/body split by an editor that had to fit beneath the body, and still called the commit body's code rendering the editor's own. Neither exists: the two tabs are separate, both halves scroll together in the General tab's one scroll region, and the split survives only because `render_description` answers `None` for a commit with nothing beyond its subject line.
The status still read "approved, not yet implemented". The module table still listed `diff/unified.rs`, which was never created, and called the element `row.rs` where the rest of the document calls it `body.rs` — with six files in the directory against seven in the table. And the selection section still argued that `with_scroll_offset` must not be called, which stopped being true when the participant started registering the viewport so that auto-scroll has a trigger zone the pointer can reach.
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
1 participant
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.
Reading a diff was hard. The reported cause was the
+/-at the head of every line, which shifts the code one column. Investigation found a second cause behind it, and a larger one.Why it was unreadable
TextDecorationpaints a background from a range's first glyph to its last (gpui/src/text_system/line.rs:689-701), so the addition and deletion tints — which already carried GitHub's exact values — followed the ragged right edge of the text instead of filling the row, and a blank added line got no background at all. The markers were carrying the signal because the colour only did half of it.Both followed from the diff being one text document fed to a code editor rather than a list of elements, and that could not be worked around: at the pinned gpui-component revision the gutter is fixed to
buffer_line + 1in a single column, there is no full-width row background outside the cursor line, and there is no per-line element injection.What this does
The diff is now a derived row model painted by a custom
Element:+/-in its own 16px column, tinted by origin, so the code starts at the same column on every line and the markers never reach the clipboarddiff-view-preference.jsongpui-base's window-level selection — already mounted throughRootTwo of the three things the editor had bought back are therefore recovered rather than lost: virtualisation is hand-rolled (
virtual_listcould not be used — it yields one element per row, while a selection participant declares every run in a single call), and selection comes fromgpui-base. Syntax highlighting by language is deliberately deferred.Design notes
docs/superpowers/specs/2026-08-29-github-style-diff-view-design.mdrecords the reasoning, including three findings that overturned earlier decisions mid-flight:with_scroll_offsetmust not be reported for a participant inside a scroll container — gpui already folds the offset intobounds.origin, so reporting it double-counts. The auto-scroll fix later reintroduced it deliberately, against the viewport's registered bounds, where the sum is what gpui-base consumes.set_fallback_copy_text, which works only because that setter clears the projection — undocumented on the setter, and pinned only byCargo.lock. Recorded in the module doc.prepaintand the run set must narrow together: narrowing prepaint alone panics, narrowing runs alone silently truncates a copy.Testing
383 tests pass;
cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --all --checkclean. The row model, the left/right pairing, the palette, the cell mapping and the persisted mode are pure and carry the tests.Not verified — please look before merging
Nothing here has been seen on screen. The three criteria the design set for itself have never been exercised by anyone:
+/-Plus, added later and equally unexercised: the tinted markers, the split columns, the toggle surviving a restart, and the one path no test reaches — drag, wheel-scroll while still holding the button, then copy, which is the only route through the off-screen copy branch.
Four residuals, parked deliberately
Left for you to rule on rather than fixed, since the review process allows one fix wave:
///block onstruct LineColors— it contravenes the no-comments rule and is a five-line deletionpalette.rssays6.20:1where the measurement is6.19:1Copyhandler sits above the metadata views on the bubble path