feat(ui): vertical scrollbar thumb on diff pane right border#151
Merged
feat(ui): vertical scrollbar thumb on diff pane right border#151
Conversation
Replace the right-border │ with a heavy ┃ on rows that map to the visible viewport portion, giving an at-a-glance indicator of scroll position and content length. Implemented as a post-process pass on the lipgloss-rendered pane (same pattern as padContentBg/extendLineBg/ applyHorizontalScroll), so the surrounding ANSI envelope — including focused-vs-inactive border color and BorderBackground — is preserved. Thumb is bold-wrapped via \x1b[1m...\x1b[22m to brighten the accent color without resetting the border background. No-op when content fits the viewport. Diff pane only; tree pane untouched.
- [critical] [app/ui/view.go,scrollbar.go] truncate diff header so long filenames don't soft-wrap and push viewport rows past scrollbarFirstViewportRow=2 - [doc] [app/ui/scrollbar.go:22] godoc said "half-block thumb glyph" — rune is heavy-vertical; updated wording - [doc] [app/ui/scrollbar.go:23] godoc no-op clause now mentions vh<=0 case - [doc] [app/ui/scrollbar.go:5-22] document intentional bold-only --no-colors behavior + scrollbarFirstViewportRow coupling warning - [doc] [CLAUDE.md] clarify post-pane vs per-line transform; document single-line header invariant via truncateHeaderTitle - [doc] [docs/ARCHITECTURE.md] add scrollbar.go to app/ui per-file table - [minor] [app/ui/scrollbar.go:38-41] drop unreachable maxStart>0 guard - [minor] [app/ui/scrollbar_test.go:217] fix comment "5 lines total" → 6 - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_ThumbWrappedInBoldSGR to lock in the bold SGR contract - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_RowWithoutTrackRune to cover the idx<0 branch - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_BodyContainingTrackRune to cover content rows containing literal │ - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_AgainstRealLipglossOutput integration test against real lipgloss border render - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_ViewportHeightOne smallest-viewport edge case - [test] [app/ui/scrollbar_test.go] tighten ThumbProportionalSize to assert exact row positions (not just count) - [test] [app/ui/view_test.go] tighten thumb-position-shifts test to assert exact positions instead of loose Less ordering - [test] [app/ui/view_test.go] add paneTree-focus subtest, markdown-TOC subtest, and long-filename C1 regression - [test] [app/ui/view_test.go] add TestModel_TruncateHeaderTitle table-driven coverage of the new helper
- [critical] [app/ui/view.go] sanitize control characters (C0/C1/DEL/RuneError) in filenames before width budgeting; without this, paths containing \n / ESC etc. could re-wrap the diff header and re-break the scrollbar invariant the iter-1 fix was meant to enforce - [major] [app/ui/scrollbar.go] add defensive line-count range check; bail when the rendered pane has more rows than paneHeight()+2 (catches body rows wrapping in narrow terminals where applyHorizontalScroll's cutWidth goes non-positive due to gutters consuming all content width) - [code-quality] [app/ui/view.go] extract truncateLeftToWidth shared helper; status-bar inline truncation and truncateHeaderTitle now use the same algorithm (was a verbatim copy) - [doc] [app/ui/scrollbar.go:58-61] correct comment that incorrectly claimed thumbSize clamps to vh-1; the actual non-zero-divisor argument is that total>vh holds after the early-return - [doc] [CLAUDE.md] correct mis-claim that applyScrollbar runs at "the same layer as padContentBg" (padContentBg is pre-render, applyScrollbar is post-render); add upstream coupling warning that paneW must match lipgloss Width() - [doc] [docs/ARCHITECTURE.md] add post-pane stage (truncateHeaderTitle, padContentBg, lipgloss.Render, applyScrollbar) to the rendering pipeline diagram - [test] [app/ui/scrollbar_test.go] strengthen NeverModifiesCorners with positive thumb-position assertion (would have passed even with a no-op) - [test] [app/ui/scrollbar_test.go] table-drive ThumbProportionalSize across top/midway/bottom YOffsets (was only YOffset=0) - [test] [app/ui/scrollbar_test.go] add TestApplyScrollbar_BailsOnUnexpectedLineCount for the new defensive check - [test] [app/ui/scrollbar_test.go] add TestSanitizeFilenameForDisplay covering newline/CR/tab/ESC/DEL/C1/CJK/RuneError - [test] [app/ui/scrollbar_test.go] add TestTruncateLeftToWidth covering the new shared helper with budget edge cases - [test] [app/ui/view_test.go] extend TestModel_TruncateHeaderTitle with paneW=2/3/4 boundaries, negative paneW, and control-char-in-title cases; invariant assertion that result never contains a newline
- [critical] [app/ui/view.go] apply sanitizeFilenameForDisplay to status-bar filename segments (statusBarText, statusSegmentsNoSearch, statusSegmentsMinimal). previously the sanitizer was scoped to the diff header only — a crafted filename with newline/ESC could still break or spoof the bottom-of-screen layout - [major] [app/ui/view.go] extend sanitizeFilenameForDisplay to strip Unicode bidi/format controls (RTL/LTR overrides U+202A–U+202E, isolates U+2066–U+2069, ZWJ/ZWNJ/ZWSP U+200B–U+200D, BOM U+FEFF) — defense in depth against filename spoofing - [code-quality] [app/ui/view.go] convert sanitizeFilenameForDisplay and truncateLeftToWidth from package-level functions to Model methods, per project rule preferring methods over standalone helpers - [code-quality] [app/ui/view.go] pass diffPaneW into renderTwoPaneLayout instead of re-deriving the formula inline — single source of truth so a future change to one side cannot drift from the other - [code-quality] [app/ui/view.go] simplify truncateHeaderTitle: paneW<=1 now delegates to truncateLeftToWidth instead of mirroring its boundary cases inline - [test] [app/ui/scrollbar_test.go] strengthen TestApplyScrollbar_SafeWhenLinesShorterThanExpected with Equal+NotContains assertions (was only asserting line count preserved, would have passed with a thumb mistakenly applied) - [test] [app/ui/scrollbar_test.go] add bidi/format char cases to TestSanitizeFilenameForDisplay (RTL override, LRI, PDI, ZWJ, ZWSP, BOM) - [test] [app/ui/view_test.go] add TestModel_StatusBarSanitizesFilename regression covering newline/CR/tab/ESC/RTL/BOM in the status bar
There was a problem hiding this comment.
Pull request overview
Adds a vertical scroll-position indicator to the diff pane by rendering a “thumb” on the pane’s right border, while hardening filename rendering to preserve single-line header/status layouts.
Changes:
- Add
applyScrollbar()post-processing to replace the diff pane’s right border│with a bold┃over rows corresponding to the visible viewport slice. - Enforce the “single-line diff header” invariant via filename sanitization + left-truncation (
sanitizeFilenameForDisplay,truncateLeftToWidth,truncateHeaderTitle), and apply the same sanitization to status bar filename segments. - Add extensive unit + integration-style tests covering thumb sizing/positioning, ANSI envelope behavior, and filename sanitization/truncation edge cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/ARCHITECTURE.md | Documents the new post-render scrollbar stage in the rendering pipeline. |
| app/ui/view.go | Integrates scrollbar post-processing into diff pane rendering; adds filename sanitization + truncation helpers and applies them to header/status bar. |
| app/ui/scrollbar.go | Implements the scrollbar thumb substitution logic with defensive shape checks. |
| app/ui/view_test.go | Adds view-level tests validating thumb presence/position and status bar filename sanitization. |
| app/ui/scrollbar_test.go | Adds focused unit tests for thumb math, row targeting, ANSI behavior, and edge cases. |
| README.md | Documents the new vertical scrollbar thumb feature. |
| CLAUDE.md | Adds a gotcha entry describing the scrollbar’s post-render transform and layout coupling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+35
| // rune since both track and thumb share the same UTF-8 byte width and | ||
| // lipgloss renders the right border as the line's last │ rune. |
There was a problem hiding this comment.
The applyScrollbar doc comment says “both track and thumb share the same UTF-8 byte width”. That’s not true for scrollbarThumbRune because it includes SGR escape sequences (so its byte length is larger than scrollbarTrackRune). Consider rewording to clarify that the glyph ┃ is the same 3-byte UTF-8 rune / 1-cell width as │, and that the replacement works because it targets the last border rune and keeps the existing surrounding ANSI sequences intact.
Suggested change
| // rune since both track and thumb share the same UTF-8 byte width and | |
| // lipgloss renders the right border as the line's last │ rune. | |
| // rune by replacing only the line's last border │ rune; the visible thumb | |
| // glyph ┃ has the same 1-cell width (and same 3-byte UTF-8 rune width) as | |
| // │, while the added bold SGR sequences remain localized to the inserted | |
| // thumb string. |
| - **ANSI nesting with lipgloss**: `lipgloss.Render()` emits `\033[0m` (full reset) which breaks outer style backgrounds. For styled substrings inside a lipgloss container (status bar separators, search highlights, diff cursor, annotation lines), use raw ANSI sequences via the style sub-package (`style.AnsiFg`, `resolver.Color`), or dedicated `Renderer` methods. Never use `lipgloss.NewStyle().Render()` for inline elements within a lipgloss-rendered parent. | ||
| - **Background fill for themed panes**: lipgloss pane `Render()` and viewport internal padding emit plain spaces after reset, causing terminal default bg. Workarounds: (1) `extendLineBg()` pads lines to full width, (2) `padContentBg()` re-pads pane content, (3) `BorderBackground()` on border styles. **Ordering**: `extendLineBg()` must be called AFTER `applyHorizontalScroll()`. | ||
| - Horizontal scroll indicators (`«`/`»`): see `applyHorizontalScroll()` in `diffview.go`. `«` replaces first visible column when scrolled past hidden content. `»` extends 1 col into right padding. Bg split: `«` and separator space use line bg via `indicatorBg()`, `»` glyph uses `DiffBg`. Only in unwrapped mode. | ||
| - Vertical scrollbar thumb on diff pane: `applyScrollbar()` in `app/ui/scrollbar.go` is the only **post-`lipgloss.Render()`** transform on the diff pane string (distinct from `padContentBg`, which runs *pre-render* on the assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). Replaces the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. No-op when `viewport.TotalLineCount() <= viewport.Height` (content fits), `viewport.Height <= 0`, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). Both glyphs are 3-byte UTF-8 so the slice replacement preserves the surrounding ANSI envelope (border-fg + optional `BorderBackground`); `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: the rendered pane is assumed to be top-border row, **single-line** header, `vh` viewport rows, bottom-border — viewport rows start at index `scrollbarFirstViewportRow = 2`. The single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `sanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `…` so lipgloss never soft-wraps the header). The `paneW` passed to `truncateHeaderTitle` MUST match the lipgloss `Width()` later applied to the pane — both branches of `View()` derive it via `width-2` (tree hidden) / `width-treeWidth-4` (two-pane). Changing one side without the other re-introduces the wrap regression. Any change to the diff pane's pre-viewport row count (multi-line header, status pill above viewport, etc.) must update `scrollbarFirstViewportRow` in lockstep. Tree pane is intentionally untouched. |
There was a problem hiding this comment.
This gotcha entry states “Both glyphs are 3-byte UTF-8 so the slice replacement preserves the surrounding ANSI envelope”. scrollbarThumbRune includes SGR escape sequences, so it’s not 3 bytes; only the ┃ glyph is. Suggest rephrasing to avoid implying the replacement depends on equal string byte lengths, and instead note that the code replaces the last │ rune and relies on ┃ being 1 cell / 3-byte UTF-8.
Suggested change
| - Vertical scrollbar thumb on diff pane: `applyScrollbar()` in `app/ui/scrollbar.go` is the only **post-`lipgloss.Render()`** transform on the diff pane string (distinct from `padContentBg`, which runs *pre-render* on the assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). Replaces the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. No-op when `viewport.TotalLineCount() <= viewport.Height` (content fits), `viewport.Height <= 0`, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). Both glyphs are 3-byte UTF-8 so the slice replacement preserves the surrounding ANSI envelope (border-fg + optional `BorderBackground`); `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: the rendered pane is assumed to be top-border row, **single-line** header, `vh` viewport rows, bottom-border — viewport rows start at index `scrollbarFirstViewportRow = 2`. The single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `sanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `…` so lipgloss never soft-wraps the header). The `paneW` passed to `truncateHeaderTitle` MUST match the lipgloss `Width()` later applied to the pane — both branches of `View()` derive it via `width-2` (tree hidden) / `width-treeWidth-4` (two-pane). Changing one side without the other re-introduces the wrap regression. Any change to the diff pane's pre-viewport row count (multi-line header, status pill above viewport, etc.) must update `scrollbarFirstViewportRow` in lockstep. Tree pane is intentionally untouched. | |
| - Vertical scrollbar thumb on diff pane: `applyScrollbar()` in `app/ui/scrollbar.go` is the only **post-`lipgloss.Render()`** transform on the diff pane string (distinct from `padContentBg`, which runs *pre-render* on the assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). Replaces the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. No-op when `viewport.TotalLineCount() <= viewport.Height` (content fits), `viewport.Height <= 0`, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). The replacement targets the last `│` rune on each affected rendered row; it relies on `┃` being a single-cell glyph with 3-byte UTF-8 encoding, while the surrounding ANSI envelope (border-fg + optional `BorderBackground`) remains outside that replaced rune. `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: the rendered pane is assumed to be top-border row, **single-line** header, `vh` viewport rows, bottom-border — viewport rows start at index `scrollbarFirstViewportRow = 2`. The single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `sanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `…` so lipgloss never soft-wraps the header). The `paneW` passed to `truncateHeaderTitle` MUST match the lipgloss `Width()` later applied to the pane — both branches of `View()` derive it via `width-2` (tree hidden) / `width-treeWidth-4` (two-pane). Changing one side without the other re-introduces the wrap regression. Any change to the diff pane's pre-viewport row count (multi-line header, status pill above viewport, etc.) must update `scrollbarFirstViewportRow` in lockstep. Tree pane is intentionally untouched. |
Copilot review on PR #151 flagged the byte-width claim as misleading. The current text said "both track and thumb share the same UTF-8 byte width" / "Both glyphs are 3-byte UTF-8 so the slice replacement preserves the surrounding ANSI envelope" — but scrollbarThumbRune is the bold-wrapped string "\x1b[1m┃\x1b[22m" (12 bytes), not 3, so the implied byte-equality argument was wrong. Reword both spots (godoc on applyScrollbar, CLAUDE.md gotcha entry) to state the actual mechanism: the slice operation finds the rune via LastIndex and replaces only that rune, leaving prefix/suffix bytes (border fg/bg ANSI) intact regardless of the thumb's added SGR wrap. The 1-cell display width of ┃ and │ is what keeps geometry stable — that's separate from envelope preservation. Related to #151.
Deploying revdiff with
|
| Latest commit: |
052e7c0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://330da62e.revdiff.pages.dev |
| Branch Preview URL: | https://feat-scrollbar-thumb.revdiff.pages.dev |
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
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.
Adds an at-a-glance scroll-position indicator to the diff pane: the right
border
│is replaced with a thicker┃(heavy-vertical, bold) on therows that map to the currently visible portion of the diff. Thumb size and
position track viewport state automatically. Diff pane only; tree pane is
intentionally left untouched.
Implementation
applyScrollbar()runs afterlipgloss.Render()on the diff panestring — same byte-level post-processing pattern as
padContentBg.Both glyphs are 3-byte UTF-8 so the slice replacement preserves the
surrounding ANSI envelope (border-fg + optional
BorderBackground).\x1b[22mresets only intensity to keep the border bg intact.filenames are sanitized via
sanitizeFilenameForDisplay(strips C0/C1controls, DEL, RuneError, and Unicode bidi/format chars: RTL/LTR
overrides, isolates, ZWJ/ZWNJ/ZWSP, BOM) and then left-truncated with
truncateLeftToWidthso a long filename cannot soft-wrap the header.bar — so crafted paths cannot break or spoof either layout.
applyScrollbarsilently no-opswhen the rendered pane has more rows than
paneHeight()+2. Catchesbody-row wrapping in narrow terminals where
applyHorizontalScrollcannot truncate because gutters consume the full content width.
line-numbers, word-diff, search, focus, --no-colors, --stdin).
Tests
23 dedicated scrollbar unit tests + 7 view-level subtests + dedicated
coverage of
truncateHeaderTitle,truncateLeftToWidth,sanitizeFilenameForDisplay(incl. CJK, bidi, BOM, control-char cases),status-bar sanitization regression, paneTree-focus subtest, markdown-TOC
layout subtest, long-filename regression, vh=1 minimum-viewport edge
case, real-lipgloss integration test, content-row-with-
│case, idx<0defensive branch.
Docs
header invariant, paneW upstream coupling, and
scrollbarFirstViewportRowlayout coupling
scrollbar.goadded to per-file table; renderingpipeline diagram extended with the post-pane stage