From dab210a84ab99b29abc4a88245b9c062721a1be3 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 6 Sep 2026 12:59:33 -0500 Subject: [PATCH 1/5] feat(ui): show status bar icons beside their toggle keys in help The help overlay listed every toggle key but never the glyph the status bar renders for it, so the icon legend lived only in the README table. Each View and Search row now carries its glyph in front of the description, resolved through the same live keymap as the key column so custom bindings and chords stay correct. Rows in those sections without a glyph are indented to keep the description column straight. Related to #351 --- .../skills/revdiff/references/usage.md | 2 +- .claude/rules/gotchas.md | 2 +- README.md | 2 +- app/ui/handlers.go | 25 ++++++++-- app/ui/handlers_test.go | 46 +++++++++++++++++++ app/ui/view.go | 19 ++++++++ .../codex/skills/revdiff/references/usage.md | 2 +- site/docs.html | 2 +- 8 files changed, 92 insertions(+), 8 deletions(-) diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 47d9842d..07f9ea41 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -190,7 +190,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ## Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index e786c306..282294cc 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -32,7 +32,7 @@ - **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 thumbs: `applyScrollbar()` / `applyNavigationScrollbar()` in `app/ui/scrollbar.go` are **post-`lipgloss.Render()`** transforms on rendered pane strings (distinct from `padContentBg`, which runs *pre-render* on assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). They replace the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. Diff uses Bubble viewport state; navigation uses `sidepane.ScrollState` from file tree / markdown TOC after rendering. No-op when content fits, viewport height is zero, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). The slice replacement preserves the surrounding ANSI envelope (border-fg + optional `BorderBackground`) because it swaps only the rune itself — prefix/suffix bytes are kept intact regardless of the thumb's added SGR wrap. Glyphs `┃` and `│` are both 1 cell wide so display geometry stays unchanged. `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: diff pane assumes top-border row, **single-line** header, `vh` viewport rows, bottom-border — diff rows start at `diffScrollbarFirstViewportRow = 2`; navigation pane assumes top-border row, content rows, bottom-border — navigation rows start at `navigationScrollbarFirstViewportRow = 1`. The diff single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `style.SanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `style.TruncateLeftToWidth` 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 pane pre-content row counts (multi-line diff header, tree/TOC header, status pill above viewport, etc.) must update the matching scrollbar row offset in lockstep. -- Status bar mode icons: `▼◉↩≋⊟⊂#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. +- Status bar mode icons: `▼◉↩≋⊟⊂#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The help overlay shows each glyph beside its toggle key by reading `statusIconForAction` (same file, keyed by `keymap.Action`); a new icon needs an entry there or `?` never mentions it. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the mapping. - Search and hunk navigation use `centerViewportOnCursor()` (cursor ends up in the middle of the page). `syncViewportToCursor()` is the general "keep cursor visible" path and is called from cursor moves (j/k, g/G, page up/down), content mutations (annotation save/delete, blame load), and layout changes (tree/wrap/blame/line-number toggles, resize, file load). It uses `cursorVisualRange()` so the cursor's full logical line — wrap-continuation rows plus any injected annotation rows — stays visible, not just the cursor's top row. - **Viewport scroll after content mutation**: `syncViewportToCursor()` calls `SetContent(renderDiff())` itself before setting `YOffset` — callers must not pre-`SetContent` around it, and any code that injects rows below a diff line (wrap continuations, multi-line annotations, future overlays) must route scroll through this function so visual-height math stays consistent with `cursorVisualRange()` / `hunkLineHeight()`. - **Page/half-page cursor walks and `cursorOnAnnotation`**: `moveDiffCursorDownBy` / `moveDiffCursorUpBy` (`app/ui/diffnav.go`) detect "no movement possible" by comparing `diffCursor` **and** `cursorOnAnnotation` against the previous **single** step. So `moveDiffCursorDownWithHunks` may clear `cursorOnAnnotation` **only** together with an advancing `diffCursor` — clearing it on the last navigable line's annotation, where no next line exists, reads as progress forever and freezes the TUI (the visual delta oscillates by the diff line's own wrapped height — `cursorViewportYFromOffsets` adds `wrappedLineCount(diffCursor)` when the flag is set, not the annotation's row count — and both ends of the oscillation are fixed relative to the `startY` captured at loop entry, so the walk cycles forever whenever both ends stay below `startY + rows`). This freeze is unrecoverable from inside the TUI: it wedges the bubbletea event loop inside `Update`, so `shutdownGuard`'s `p.Quit()` blocks forever sending on the unbuffered `msgs` channel that only the wedged `eventLoop` drains (nothing cancels `p.ctx` — no `Kill()` caller, and `defer p.cancel()` runs only when `Run` returns). `signal.Notify` (`app/signal.go`) has already taken over SIGHUP/SIGTERM/SIGINT, and its `signal.Stop` restore only runs after `p.Run()` returns, which it never does — so `kill -TERM` does NOT reach the signal-safe-save path, it just parks the handler goroutine on the same blocked send. Only a signal outside that set ends it: `kill -QUIT` (Go runtime dumps goroutine stacks and dies, leaving the terminal in raw mode — run `reset` after) or `kill -9`. Neither runs `finalize`: no history auto-save and no `-o` write — but an earlier `O` flush's snapshot survives on disk. The up direction is deliberately asymmetric: the annotation sub-row renders *below* its diff line (`rowOnAnnotationSubLine` in `app/ui/annotate.go`), so clearing the flag without moving `diffCursor` IS the upward move, and that cursor strictly decreases so it cannot cycle. Absolute placements (H/M/L in `diffnav.go`, mouse click in `mouse.go`) are one-shot assignments outside these loops. Both walks also **undo** the step that carries the delta past `rows`, restoring `diffCursor` and `cursorOnAnnotation` (the only two fields `moveDiffCursor*WithHunks` mutates) and breaking immediately: one cursor step can span many rows via `hunkLineHeight`, and scrolling by that whole height moved the viewport further than a page, past rows it never rendered. The rollback cannot cycle — it is always followed by `break`. It is gated on `worthRollingBack(walked, rows)` (`walked*2 >= rows`) and that gate is **not** cosmetic: a block taller than the page has no selectable position inside it, so rolling back to the row or two walked before it scrolls the pane by almost nothing and the next press takes the same oversized step anyway. Gating on "has moved at all" instead produced exactly that one-row collapse. `walked` is 0 on the first step, so the walk can never refuse to move. A single step taller than the whole page still scrolls by its full height and still skips — unavoidable while the viewport follows the cursor, and the reason `pgdown` then `pgup` is only reversible for uniform-height content. The two directions accumulate differently and the comments say so: walking **down** the delta grows by the height of the line being *left* (`offsets[i]` is that line's top row), walking **up** by the height of the line *arrived at*, including the annotation block rendered below it. diff --git a/README.md b/README.md index 91768056..76f347d6 100644 --- a/README.md +++ b/README.md @@ -822,7 +822,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ### Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/app/ui/handlers.go b/app/ui/handlers.go index 9691409f..7ff0a194 100644 --- a/app/ui/handlers.go +++ b/app/ui/handlers.go @@ -61,17 +61,18 @@ func (m Model) buildHelpSpec() overlay.HelpSpec { sections := m.keymap.HelpSections() var result []overlay.HelpSection for _, sec := range sections { + pad := m.helpIconPad(sec) var entries []overlay.HelpEntry for _, e := range sec.Entries { entries = append(entries, overlay.HelpEntry{ Keys: m.formatKeysForHelp(e.Action), - Description: e.Description, + Description: m.helpDescriptionWithIcon(e, pad), }) } if sec.Name == "Search" { entries = append(entries, - overlay.HelpEntry{Keys: "↑ / Ctrl+P", Description: "recall previous search query (in search prompt)"}, - overlay.HelpEntry{Keys: "↓ / Ctrl+N", Description: "recall next search query / clear (in search prompt)"}, + overlay.HelpEntry{Keys: "↑ / Ctrl+P", Description: pad + "recall previous search query (in search prompt)"}, + overlay.HelpEntry{Keys: "↓ / Ctrl+N", Description: pad + "recall next search query / clear (in search prompt)"}, ) } result = append(result, overlay.HelpSection{Title: sec.Name, Entries: entries}) @@ -86,6 +87,24 @@ func (m Model) buildHelpSpec() overlay.HelpSpec { return overlay.HelpSpec{Sections: result} } +// helpIconPad returns the indent that keeps a section's description column straight +// once some of its rows carry a glyph; a section with no glyph rows gets none. +func (m Model) helpIconPad(sec keymap.HelpSection) string { + for _, e := range sec.Entries { + if _, ok := statusIconForAction[e.Action]; ok { + return " " + } + } + return "" +} + +func (m Model) helpDescriptionWithIcon(e keymap.HelpEntryWithKeys, pad string) string { + if icon, ok := statusIconForAction[e.Action]; ok { + return icon + " " + e.Description + } + return pad + e.Description +} + // buildVimMotionHelpSection returns the synthetic help section for the // vim-motion preset. Keys are hardcoded because these bindings are not part // of the configurable keymap — they are driven by the interceptor in diff --git a/app/ui/handlers_test.go b/app/ui/handlers_test.go index 74be32dd..38b2f3a7 100644 --- a/app/ui/handlers_test.go +++ b/app/ui/handlers_test.go @@ -1051,6 +1051,52 @@ func TestBuildHelpSpec_SearchPromptHistoryEntries(t *testing.T) { assert.Contains(t, downEntry.Description, "next", "Down entry description must mention next query") } +func TestBuildHelpSpec_StatusIconsOnToggleRows(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + spec := m.buildHelpSpec() + + sections := map[string]overlay.HelpSection{} + for _, sec := range spec.Sections { + sections[sec.Title] = sec + } + byDesc := func(sec overlay.HelpSection, suffix string) string { + for _, e := range sec.Entries { + if strings.HasSuffix(e.Description, suffix) { + return e.Description + } + } + return "" + } + + tests := []struct { + section, suffix, want string + }{ + {"View", "toggle collapsed view", "▼ toggle collapsed view"}, + {"View", "toggle compact diff view", "⊂ toggle compact diff view"}, + {"View", "filter files", "◉ filter files"}, + {"View", "toggle word wrap", "↩ toggle word wrap"}, + {"View", "toggle tree pane", "⊟ toggle tree pane"}, + {"View", "toggle line numbers", "# toggle line numbers"}, + {"View", "toggle blame gutter", "b toggle blame gutter"}, + {"View", "toggle word-diff highlighting", "± toggle word-diff highlighting"}, + {"View", "mark file as reviewed", "✓ mark file as reviewed"}, + {"View", "show unreviewed files", "○ show unreviewed files"}, + {"View", "show/hide untracked files", "∅ show/hide untracked files"}, + {"View", "toggle hunk in collapsed", " toggle hunk in collapsed"}, + {"View", "show review info popup", " show review info popup"}, + {"Search", "search in diff", "≋ search in diff"}, + {"Search", "recall previous search query (in search prompt)", " recall previous search query (in search prompt)"}, + {"Navigation", "move cursor down", "move cursor down"}, + } + for _, tc := range tests { + t.Run(tc.want, func(t *testing.T) { + sec, ok := sections[tc.section] + require.True(t, ok, "section %q missing", tc.section) + assert.Equal(t, tc.want, byDesc(sec, tc.suffix)) + }) + } +} + func TestBuildHelpSpec_VimMotionSectionOff(t *testing.T) { m := testModel([]string{"a.go"}, nil) m.modes.vimMotion = false diff --git a/app/ui/view.go b/app/ui/view.go index 80c9e70c..fee92fd3 100644 --- a/app/ui/view.go +++ b/app/ui/view.go @@ -7,6 +7,7 @@ import ( "github.com/charmbracelet/lipgloss" "github.com/umputun/revdiff/app/diff" + "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/ui/overlay" "github.com/umputun/revdiff/app/ui/sidepane" "github.com/umputun/revdiff/app/ui/style" @@ -372,6 +373,24 @@ func (m Model) padContentBg(content string, targetWidth int, bg style.Color) str return strings.Join(lines, "\n") } +// statusIconForAction maps each view toggle to the glyph statusModeIcons renders for it, +// so the help overlay can show the icon beside the key that drives it. The reviewed slot +// is one glyph position with two actions behind it: ✓ for marking, ○ for the unreviewed filter. +var statusIconForAction = map[keymap.Action]string{ + keymap.ActionToggleCollapsed: "▼", + keymap.ActionToggleCompact: "⊂", + keymap.ActionFilter: "◉", + keymap.ActionToggleWrap: "↩", + keymap.ActionSearch: "≋", + keymap.ActionToggleTree: "⊟", + keymap.ActionToggleLineNums: "#", + keymap.ActionToggleBlame: "b", + keymap.ActionToggleWordDiff: "±", + keymap.ActionMarkReviewed: "✓", + keymap.ActionFilterUnreviewed: "○", + keymap.ActionToggleUntracked: "∅", +} + // statusModeIcons returns combined mode indicator icons (one per view toggle). // all icons are always shown; active modes use status foreground, inactive use muted color. func (m Model) statusModeIcons() string { diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index 1fe3b4f3..31825645 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -184,7 +184,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ## Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/site/docs.html b/site/docs.html index 6e7ace8c..24795132 100644 --- a/site/docs.html +++ b/site/docs.html @@ -598,7 +598,7 @@

View toggles

Status bar icons

-

The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on.

+

The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (?) shows each icon beside the key that toggles it.

From 0edaf34dea36abbf220ebf04a35afd28743b1a42 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 6 Sep 2026 13:04:21 -0500 Subject: [PATCH 2/5] test(fsutil): cover the write and close failure cleanups of AtomicWriteFile Split the fill-close-rename step into commitTemp behind a TempFile interface so a moq mock can fail the write or the close. Both paths now prove the temp file is removed and the target is left untouched. Package coverage goes from 70.6% to 100%. --- app/fsutil/fsutil.go | 23 ++++++-- app/fsutil/fsutil_test.go | 34 +++++++++++ app/fsutil/mocks/temp_file.go | 107 ++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 app/fsutil/mocks/temp_file.go diff --git a/app/fsutil/fsutil.go b/app/fsutil/fsutil.go index a8b3fc8d..abbc4a0a 100644 --- a/app/fsutil/fsutil.go +++ b/app/fsutil/fsutil.go @@ -3,10 +3,20 @@ package fsutil import ( "fmt" + "io" "os" "path/filepath" ) +//go:generate moq -out mocks/temp_file.go -pkg mocks -skip-ensure -fmt goimports . TempFile + +// TempFile is the open temp file AtomicWriteFile fills before renaming it into place. +// *os.File satisfies it. +type TempFile interface { + io.Writer + io.Closer +} + // AtomicWriteFile writes data to a temp file and renames it into place, // ensuring the write is atomic on POSIX filesystems. func AtomicWriteFile(path string, data []byte) error { @@ -16,13 +26,18 @@ func AtomicWriteFile(path string, data []byte) error { if err != nil { return fmt.Errorf("creating temp file: %w", err) } - tmp := f.Name() - if _, err := f.Write(data); err != nil { - _ = f.Close() + return commitTemp(f, f.Name(), path, data) +} + +// commitTemp writes data through w, closes it and renames tmp over path. The temp file is +// removed on every failure so a partial write never survives next to the target. +func commitTemp(w TempFile, tmp, path string, data []byte) error { + if _, err := w.Write(data); err != nil { + _ = w.Close() _ = os.Remove(tmp) return fmt.Errorf("writing temp file: %w", err) } - if err := f.Close(); err != nil { + if err := w.Close(); err != nil { _ = os.Remove(tmp) return fmt.Errorf("closing temp file: %w", err) } diff --git a/app/fsutil/fsutil_test.go b/app/fsutil/fsutil_test.go index 6200dfed..29ec1192 100644 --- a/app/fsutil/fsutil_test.go +++ b/app/fsutil/fsutil_test.go @@ -1,12 +1,15 @@ package fsutil import ( + "errors" "os" "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/umputun/revdiff/app/fsutil/mocks" ) func TestAtomicWriteFile(t *testing.T) { @@ -76,6 +79,37 @@ func TestAtomicWriteFile(t *testing.T) { assert.Len(t, entries, 1, "only the original directory should remain") }) + t.Run("removes temp file when write fails", func(t *testing.T) { + dir := t.TempDir() + tmp := filepath.Join(dir, "target.txt.tmp-1") + require.NoError(t, os.WriteFile(tmp, nil, 0o600)) + f := &mocks.TempFileMock{ + WriteFunc: func([]byte) (int, error) { return 0, errors.New("disk full") }, + CloseFunc: func() error { return nil }, + } + err := commitTemp(f, tmp, filepath.Join(dir, "target.txt"), []byte("data")) + require.Error(t, err) + assert.Contains(t, err.Error(), "writing temp file") + assert.Len(t, f.CloseCalls(), 1) + assert.NoFileExists(t, tmp) + assert.NoFileExists(t, filepath.Join(dir, "target.txt")) + }) + + t.Run("removes temp file when close fails", func(t *testing.T) { + dir := t.TempDir() + tmp := filepath.Join(dir, "target.txt.tmp-1") + require.NoError(t, os.WriteFile(tmp, nil, 0o600)) + f := &mocks.TempFileMock{ + WriteFunc: func(p []byte) (int, error) { return len(p), nil }, + CloseFunc: func() error { return errors.New("io error") }, + } + err := commitTemp(f, tmp, filepath.Join(dir, "target.txt"), []byte("data")) + require.Error(t, err) + assert.Contains(t, err.Error(), "closing temp file") + assert.NoFileExists(t, tmp) + assert.NoFileExists(t, filepath.Join(dir, "target.txt")) + }) + t.Run("fails when directory becomes read-only before write", func(t *testing.T) { dir := t.TempDir() sub := filepath.Join(dir, "ro") diff --git a/app/fsutil/mocks/temp_file.go b/app/fsutil/mocks/temp_file.go new file mode 100644 index 00000000..909c140d --- /dev/null +++ b/app/fsutil/mocks/temp_file.go @@ -0,0 +1,107 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package mocks + +import ( + "sync" +) + +// TempFileMock is a mock implementation of fsutil.TempFile. +// +// func TestSomethingThatUsesTempFile(t *testing.T) { +// +// // make and configure a mocked fsutil.TempFile +// mockedTempFile := &TempFileMock{ +// CloseFunc: func() error { +// panic("mock out the Close method") +// }, +// WriteFunc: func(p []byte) (int, error) { +// panic("mock out the Write method") +// }, +// } +// +// // use mockedTempFile in code that requires fsutil.TempFile +// // and then make assertions. +// +// } +type TempFileMock struct { + // CloseFunc mocks the Close method. + CloseFunc func() error + + // WriteFunc mocks the Write method. + WriteFunc func(p []byte) (int, error) + + // calls tracks calls to the methods. + calls struct { + // Close holds details about calls to the Close method. + Close []struct { + } + // Write holds details about calls to the Write method. + Write []struct { + // P is the p argument value. + P []byte + } + } + lockClose sync.RWMutex + lockWrite sync.RWMutex +} + +// Close calls CloseFunc. +func (mock *TempFileMock) Close() error { + if mock.CloseFunc == nil { + panic("TempFileMock.CloseFunc: method is nil but TempFile.Close was just called") + } + callInfo := struct { + }{} + mock.lockClose.Lock() + mock.calls.Close = append(mock.calls.Close, callInfo) + mock.lockClose.Unlock() + return mock.CloseFunc() +} + +// CloseCalls gets all the calls that were made to Close. +// Check the length with: +// +// len(mockedTempFile.CloseCalls()) +func (mock *TempFileMock) CloseCalls() []struct { +} { + var calls []struct { + } + mock.lockClose.RLock() + calls = mock.calls.Close + mock.lockClose.RUnlock() + return calls +} + +// Write calls WriteFunc. +func (mock *TempFileMock) Write(p []byte) (int, error) { + if mock.WriteFunc == nil { + panic("TempFileMock.WriteFunc: method is nil but TempFile.Write was just called") + } + callInfo := struct { + P []byte + }{ + P: p, + } + mock.lockWrite.Lock() + mock.calls.Write = append(mock.calls.Write, callInfo) + mock.lockWrite.Unlock() + return mock.WriteFunc(p) +} + +// WriteCalls gets all the calls that were made to Write. +// Check the length with: +// +// len(mockedTempFile.WriteCalls()) +func (mock *TempFileMock) WriteCalls() []struct { + P []byte +} { + var calls []struct { + P []byte + } + mock.lockWrite.RLock() + calls = mock.calls.Write + mock.lockWrite.RUnlock() + return calls +} From 8708ba31f2ea1ab5a901f87ae0767924107b924c Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 6 Sep 2026 15:01:05 -0500 Subject: [PATCH 3/5] refactor(ui): render status bar glyphs from the single statusIconForAction map statusModeIcons carried its own copy of the twelve glyphs, so an edit in one place and not the other would put the status bar and the help legend out of step. The status bar now reads each glyph from the map by action. --- .claude/rules/gotchas.md | 2 +- app/ui/view.go | 32 +++++++++++++++++--------------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index 282294cc..942e32e3 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -32,7 +32,7 @@ - **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 thumbs: `applyScrollbar()` / `applyNavigationScrollbar()` in `app/ui/scrollbar.go` are **post-`lipgloss.Render()`** transforms on rendered pane strings (distinct from `padContentBg`, which runs *pre-render* on assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). They replace the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. Diff uses Bubble viewport state; navigation uses `sidepane.ScrollState` from file tree / markdown TOC after rendering. No-op when content fits, viewport height is zero, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). The slice replacement preserves the surrounding ANSI envelope (border-fg + optional `BorderBackground`) because it swaps only the rune itself — prefix/suffix bytes are kept intact regardless of the thumb's added SGR wrap. Glyphs `┃` and `│` are both 1 cell wide so display geometry stays unchanged. `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: diff pane assumes top-border row, **single-line** header, `vh` viewport rows, bottom-border — diff rows start at `diffScrollbarFirstViewportRow = 2`; navigation pane assumes top-border row, content rows, bottom-border — navigation rows start at `navigationScrollbarFirstViewportRow = 1`. The diff single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `style.SanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `style.TruncateLeftToWidth` 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 pane pre-content row counts (multi-line diff header, tree/TOC header, status pill above viewport, etc.) must update the matching scrollbar row offset in lockstep. -- Status bar mode icons: `▼◉↩≋⊟⊂#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The help overlay shows each glyph beside its toggle key by reading `statusIconForAction` (same file, keyed by `keymap.Action`); a new icon needs an entry there or `?` never mentions it. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the mapping. +- Status bar mode icons: `▼⊂◉↩≋⊟#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The glyphs live in one place, `statusIconForAction` (same file, keyed by `keymap.Action`): the status bar reads them from there and the help overlay shows each beside its toggle key. A new icon needs an entry there plus an indicator row in `statusModeIcons`. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the help mapping. - Search and hunk navigation use `centerViewportOnCursor()` (cursor ends up in the middle of the page). `syncViewportToCursor()` is the general "keep cursor visible" path and is called from cursor moves (j/k, g/G, page up/down), content mutations (annotation save/delete, blame load), and layout changes (tree/wrap/blame/line-number toggles, resize, file load). It uses `cursorVisualRange()` so the cursor's full logical line — wrap-continuation rows plus any injected annotation rows — stays visible, not just the cursor's top row. - **Viewport scroll after content mutation**: `syncViewportToCursor()` calls `SetContent(renderDiff())` itself before setting `YOffset` — callers must not pre-`SetContent` around it, and any code that injects rows below a diff line (wrap continuations, multi-line annotations, future overlays) must route scroll through this function so visual-height math stays consistent with `cursorVisualRange()` / `hunkLineHeight()`. - **Page/half-page cursor walks and `cursorOnAnnotation`**: `moveDiffCursorDownBy` / `moveDiffCursorUpBy` (`app/ui/diffnav.go`) detect "no movement possible" by comparing `diffCursor` **and** `cursorOnAnnotation` against the previous **single** step. So `moveDiffCursorDownWithHunks` may clear `cursorOnAnnotation` **only** together with an advancing `diffCursor` — clearing it on the last navigable line's annotation, where no next line exists, reads as progress forever and freezes the TUI (the visual delta oscillates by the diff line's own wrapped height — `cursorViewportYFromOffsets` adds `wrappedLineCount(diffCursor)` when the flag is set, not the annotation's row count — and both ends of the oscillation are fixed relative to the `startY` captured at loop entry, so the walk cycles forever whenever both ends stay below `startY + rows`). This freeze is unrecoverable from inside the TUI: it wedges the bubbletea event loop inside `Update`, so `shutdownGuard`'s `p.Quit()` blocks forever sending on the unbuffered `msgs` channel that only the wedged `eventLoop` drains (nothing cancels `p.ctx` — no `Kill()` caller, and `defer p.cancel()` runs only when `Run` returns). `signal.Notify` (`app/signal.go`) has already taken over SIGHUP/SIGTERM/SIGINT, and its `signal.Stop` restore only runs after `p.Run()` returns, which it never does — so `kill -TERM` does NOT reach the signal-safe-save path, it just parks the handler goroutine on the same blocked send. Only a signal outside that set ends it: `kill -QUIT` (Go runtime dumps goroutine stacks and dies, leaving the terminal in raw mode — run `reset` after) or `kill -9`. Neither runs `finalize`: no history auto-save and no `-o` write — but an earlier `O` flush's snapshot survives on disk. The up direction is deliberately asymmetric: the annotation sub-row renders *below* its diff line (`rowOnAnnotationSubLine` in `app/ui/annotate.go`), so clearing the flag without moving `diffCursor` IS the upward move, and that cursor strictly decreases so it cannot cycle. Absolute placements (H/M/L in `diffnav.go`, mouse click in `mouse.go`) are one-shot assignments outside these loops. Both walks also **undo** the step that carries the delta past `rows`, restoring `diffCursor` and `cursorOnAnnotation` (the only two fields `moveDiffCursor*WithHunks` mutates) and breaking immediately: one cursor step can span many rows via `hunkLineHeight`, and scrolling by that whole height moved the viewport further than a page, past rows it never rendered. The rollback cannot cycle — it is always followed by `break`. It is gated on `worthRollingBack(walked, rows)` (`walked*2 >= rows`) and that gate is **not** cosmetic: a block taller than the page has no selectable position inside it, so rolling back to the row or two walked before it scrolls the pane by almost nothing and the next press takes the same oversized step anyway. Gating on "has moved at all" instead produced exactly that one-row collapse. `walked` is 0 on the first step, so the walk can never refuse to move. A single step taller than the whole page still scrolls by its full height and still skips — unavoidable while the viewport follows the cursor, and the reason `pgdown` then `pgup` is only reversible for uniform-height content. The two directions accumulate differently and the comments say so: walking **down** the delta grows by the height of the line being *left* (`offsets[i]` is that line's top row), walking **up** by the height of the line *arrived at*, including the annotation block rendered below it. diff --git a/app/ui/view.go b/app/ui/view.go index fee92fd3..3306be34 100644 --- a/app/ui/view.go +++ b/app/ui/view.go @@ -373,9 +373,10 @@ func (m Model) padContentBg(content string, targetWidth int, bg style.Color) str return strings.Join(lines, "\n") } -// statusIconForAction maps each view toggle to the glyph statusModeIcons renders for it, -// so the help overlay can show the icon beside the key that drives it. The reviewed slot -// is one glyph position with two actions behind it: ✓ for marking, ○ for the unreviewed filter. +// statusIconForAction is the single source of the status-bar glyphs: statusModeIcons renders +// them from here and the help overlay shows each beside the key that drives it. The reviewed +// slot is one glyph position with two actions behind it: ✓ for marking, ○ for the unreviewed +// filter. var statusIconForAction = map[keymap.Action]string{ keymap.ActionToggleCollapsed: "▼", keymap.ActionToggleCompact: "⊂", @@ -398,22 +399,23 @@ func (m Model) statusModeIcons() string { icon string active bool } - reviewIcon := "✓" + icon := func(a keymap.Action) string { return statusIconForAction[a] } + reviewIcon := icon(keymap.ActionMarkReviewed) if m.tree.UnreviewedFilterActive() { - reviewIcon = "○" + reviewIcon = icon(keymap.ActionFilterUnreviewed) } indicators := []indicator{ - {"▼", m.modes.collapsed.enabled}, - {"⊂", m.modes.compact}, - {"◉", m.tree.FilterActive()}, - {"↩", m.modes.wrap}, - {"≋", len(m.search.matches) > 0}, - {"⊟", m.layout.treeHidden}, - {"#", m.modes.lineNumbers}, - {"b", m.modes.showBlame}, - {"±", m.modes.wordDiff}, + {icon(keymap.ActionToggleCollapsed), m.modes.collapsed.enabled}, + {icon(keymap.ActionToggleCompact), m.modes.compact}, + {icon(keymap.ActionFilter), m.tree.FilterActive()}, + {icon(keymap.ActionToggleWrap), m.modes.wrap}, + {icon(keymap.ActionSearch), len(m.search.matches) > 0}, + {icon(keymap.ActionToggleTree), m.layout.treeHidden}, + {icon(keymap.ActionToggleLineNums), m.modes.lineNumbers}, + {icon(keymap.ActionToggleBlame), m.modes.showBlame}, + {icon(keymap.ActionToggleWordDiff), m.modes.wordDiff}, {reviewIcon, m.tree.ReviewedCount() > 0 || m.tree.UnreviewedFilterActive()}, - {"∅", m.modes.showUntracked}, + {icon(keymap.ActionToggleUntracked), m.modes.showUntracked}, } mutedSeq := string(m.resolver.Color(style.ColorKeyMutedFg)) From c74945d89ffd7f053fafd478a3f5de9386f58574 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 6 Sep 2026 15:01:05 -0500 Subject: [PATCH 4/5] refactor(fsutil): unexport the tempFile interface and mock it in-package TempFile had no caller outside the package; it was exported only so a sibling mocks package could name the generated type. Follow the app/signal.go precedent instead: the interface stays unexported and moq writes temp_file_moq_test.go into fsutil itself. --- app/fsutil/fsutil.go | 8 ++--- app/fsutil/fsutil_test.go | 6 ++-- .../temp_file.go => temp_file_moq_test.go} | 30 +++++++++---------- 3 files changed, 21 insertions(+), 23 deletions(-) rename app/fsutil/{mocks/temp_file.go => temp_file_moq_test.go} (70%) diff --git a/app/fsutil/fsutil.go b/app/fsutil/fsutil.go index abbc4a0a..f8451d24 100644 --- a/app/fsutil/fsutil.go +++ b/app/fsutil/fsutil.go @@ -8,11 +8,11 @@ import ( "path/filepath" ) -//go:generate moq -out mocks/temp_file.go -pkg mocks -skip-ensure -fmt goimports . TempFile +//go:generate moq -out temp_file_moq_test.go -pkg fsutil -skip-ensure -fmt goimports . tempFile -// TempFile is the open temp file AtomicWriteFile fills before renaming it into place. +// tempFile is the open temp file AtomicWriteFile fills before renaming it into place. // *os.File satisfies it. -type TempFile interface { +type tempFile interface { io.Writer io.Closer } @@ -31,7 +31,7 @@ func AtomicWriteFile(path string, data []byte) error { // commitTemp writes data through w, closes it and renames tmp over path. The temp file is // removed on every failure so a partial write never survives next to the target. -func commitTemp(w TempFile, tmp, path string, data []byte) error { +func commitTemp(w tempFile, tmp, path string, data []byte) error { if _, err := w.Write(data); err != nil { _ = w.Close() _ = os.Remove(tmp) diff --git a/app/fsutil/fsutil_test.go b/app/fsutil/fsutil_test.go index 29ec1192..99a05266 100644 --- a/app/fsutil/fsutil_test.go +++ b/app/fsutil/fsutil_test.go @@ -8,8 +8,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/umputun/revdiff/app/fsutil/mocks" ) func TestAtomicWriteFile(t *testing.T) { @@ -83,7 +81,7 @@ func TestAtomicWriteFile(t *testing.T) { dir := t.TempDir() tmp := filepath.Join(dir, "target.txt.tmp-1") require.NoError(t, os.WriteFile(tmp, nil, 0o600)) - f := &mocks.TempFileMock{ + f := &tempFileMock{ WriteFunc: func([]byte) (int, error) { return 0, errors.New("disk full") }, CloseFunc: func() error { return nil }, } @@ -99,7 +97,7 @@ func TestAtomicWriteFile(t *testing.T) { dir := t.TempDir() tmp := filepath.Join(dir, "target.txt.tmp-1") require.NoError(t, os.WriteFile(tmp, nil, 0o600)) - f := &mocks.TempFileMock{ + f := &tempFileMock{ WriteFunc: func(p []byte) (int, error) { return len(p), nil }, CloseFunc: func() error { return errors.New("io error") }, } diff --git a/app/fsutil/mocks/temp_file.go b/app/fsutil/temp_file_moq_test.go similarity index 70% rename from app/fsutil/mocks/temp_file.go rename to app/fsutil/temp_file_moq_test.go index 909c140d..8abe8688 100644 --- a/app/fsutil/mocks/temp_file.go +++ b/app/fsutil/temp_file_moq_test.go @@ -1,18 +1,18 @@ // Code generated by moq; DO NOT EDIT. // github.com/matryer/moq -package mocks +package fsutil import ( "sync" ) -// TempFileMock is a mock implementation of fsutil.TempFile. +// tempFileMock is a mock implementation of tempFile. // -// func TestSomethingThatUsesTempFile(t *testing.T) { +// func TestSomethingThatUsestempFile(t *testing.T) { // -// // make and configure a mocked fsutil.TempFile -// mockedTempFile := &TempFileMock{ +// // make and configure a mocked tempFile +// mockedtempFile := &tempFileMock{ // CloseFunc: func() error { // panic("mock out the Close method") // }, @@ -21,11 +21,11 @@ import ( // }, // } // -// // use mockedTempFile in code that requires fsutil.TempFile +// // use mockedtempFile in code that requires tempFile // // and then make assertions. // // } -type TempFileMock struct { +type tempFileMock struct { // CloseFunc mocks the Close method. CloseFunc func() error @@ -48,9 +48,9 @@ type TempFileMock struct { } // Close calls CloseFunc. -func (mock *TempFileMock) Close() error { +func (mock *tempFileMock) Close() error { if mock.CloseFunc == nil { - panic("TempFileMock.CloseFunc: method is nil but TempFile.Close was just called") + panic("tempFileMock.CloseFunc: method is nil but tempFile.Close was just called") } callInfo := struct { }{} @@ -63,8 +63,8 @@ func (mock *TempFileMock) Close() error { // CloseCalls gets all the calls that were made to Close. // Check the length with: // -// len(mockedTempFile.CloseCalls()) -func (mock *TempFileMock) CloseCalls() []struct { +// len(mockedtempFile.CloseCalls()) +func (mock *tempFileMock) CloseCalls() []struct { } { var calls []struct { } @@ -75,9 +75,9 @@ func (mock *TempFileMock) CloseCalls() []struct { } // Write calls WriteFunc. -func (mock *TempFileMock) Write(p []byte) (int, error) { +func (mock *tempFileMock) Write(p []byte) (int, error) { if mock.WriteFunc == nil { - panic("TempFileMock.WriteFunc: method is nil but TempFile.Write was just called") + panic("tempFileMock.WriteFunc: method is nil but tempFile.Write was just called") } callInfo := struct { P []byte @@ -93,8 +93,8 @@ func (mock *TempFileMock) Write(p []byte) (int, error) { // WriteCalls gets all the calls that were made to Write. // Check the length with: // -// len(mockedTempFile.WriteCalls()) -func (mock *TempFileMock) WriteCalls() []struct { +// len(mockedtempFile.WriteCalls()) +func (mock *tempFileMock) WriteCalls() []struct { P []byte } { var calls []struct { From 3440a4ddfc326b52314d1d6bb8a45fce366751a1 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 6 Sep 2026 17:24:58 -0500 Subject: [PATCH 5/5] docs: say the help overlay pairs each icon with the key that controls it "toggles" was wrong for two rows: / opens the search prompt and the search icon lights on matches present, Space marks a file and the reviewed icon reflects that state. --- .claude-plugin/skills/revdiff/references/usage.md | 2 +- .claude/rules/gotchas.md | 2 +- README.md | 2 +- plugins/codex/skills/revdiff/references/usage.md | 2 +- site/docs.html | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 07f9ea41..997577c6 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -190,7 +190,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ## Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that controls it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index 942e32e3..5ad3754e 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -32,7 +32,7 @@ - **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 thumbs: `applyScrollbar()` / `applyNavigationScrollbar()` in `app/ui/scrollbar.go` are **post-`lipgloss.Render()`** transforms on rendered pane strings (distinct from `padContentBg`, which runs *pre-render* on assembled multi-line content, and from `extendLineBg`/`applyHorizontalScroll`, which run *per line* before viewport assembly). They replace the right-border `│` with `┃` (heavy vertical, bold-wrapped via `\x1b[1m...\x1b[22m`) on rows that map to the visible viewport portion. Diff uses Bubble viewport state; navigation uses `sidepane.ScrollState` from file tree / markdown TOC after rendering. No-op when content fits, viewport height is zero, or the rendered pane's line count differs from the expected shape (defensive bail when wrapping breaks the layout invariant). The slice replacement preserves the surrounding ANSI envelope (border-fg + optional `BorderBackground`) because it swaps only the rune itself — prefix/suffix bytes are kept intact regardless of the thumb's added SGR wrap. Glyphs `┃` and `│` are both 1 cell wide so display geometry stays unchanged. `\x1b[22m` resets only intensity to keep the border bg intact (vs `\x1b[0m` which would kill it). Layout coupling: diff pane assumes top-border row, **single-line** header, `vh` viewport rows, bottom-border — diff rows start at `diffScrollbarFirstViewportRow = 2`; navigation pane assumes top-border row, content rows, bottom-border — navigation rows start at `navigationScrollbarFirstViewportRow = 1`. The diff single-line header invariant is enforced by `truncateHeaderTitle()` in `view.go` (filenames are sanitized via `style.SanitizeFilenameForDisplay` to drop control bytes, then left-truncated with `style.TruncateLeftToWidth` 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 pane pre-content row counts (multi-line diff header, tree/TOC header, status pill above viewport, etc.) must update the matching scrollbar row offset in lockstep. -- Status bar mode icons: `▼⊂◉↩≋⊟#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The glyphs live in one place, `statusIconForAction` (same file, keyed by `keymap.Action`): the status bar reads them from there and the help overlay shows each beside its toggle key. A new icon needs an entry there plus an indicator row in `statusModeIcons`. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the help mapping. +- Status bar mode icons: `▼⊂◉↩≋⊟#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The glyphs live in one place, `statusIconForAction` (same file, keyed by `keymap.Action`): the status bar reads them from there and the help overlay shows each beside the key that drives it. A new icon needs an entry there plus an indicator row in `statusModeIcons`. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the help mapping. - Search and hunk navigation use `centerViewportOnCursor()` (cursor ends up in the middle of the page). `syncViewportToCursor()` is the general "keep cursor visible" path and is called from cursor moves (j/k, g/G, page up/down), content mutations (annotation save/delete, blame load), and layout changes (tree/wrap/blame/line-number toggles, resize, file load). It uses `cursorVisualRange()` so the cursor's full logical line — wrap-continuation rows plus any injected annotation rows — stays visible, not just the cursor's top row. - **Viewport scroll after content mutation**: `syncViewportToCursor()` calls `SetContent(renderDiff())` itself before setting `YOffset` — callers must not pre-`SetContent` around it, and any code that injects rows below a diff line (wrap continuations, multi-line annotations, future overlays) must route scroll through this function so visual-height math stays consistent with `cursorVisualRange()` / `hunkLineHeight()`. - **Page/half-page cursor walks and `cursorOnAnnotation`**: `moveDiffCursorDownBy` / `moveDiffCursorUpBy` (`app/ui/diffnav.go`) detect "no movement possible" by comparing `diffCursor` **and** `cursorOnAnnotation` against the previous **single** step. So `moveDiffCursorDownWithHunks` may clear `cursorOnAnnotation` **only** together with an advancing `diffCursor` — clearing it on the last navigable line's annotation, where no next line exists, reads as progress forever and freezes the TUI (the visual delta oscillates by the diff line's own wrapped height — `cursorViewportYFromOffsets` adds `wrappedLineCount(diffCursor)` when the flag is set, not the annotation's row count — and both ends of the oscillation are fixed relative to the `startY` captured at loop entry, so the walk cycles forever whenever both ends stay below `startY + rows`). This freeze is unrecoverable from inside the TUI: it wedges the bubbletea event loop inside `Update`, so `shutdownGuard`'s `p.Quit()` blocks forever sending on the unbuffered `msgs` channel that only the wedged `eventLoop` drains (nothing cancels `p.ctx` — no `Kill()` caller, and `defer p.cancel()` runs only when `Run` returns). `signal.Notify` (`app/signal.go`) has already taken over SIGHUP/SIGTERM/SIGINT, and its `signal.Stop` restore only runs after `p.Run()` returns, which it never does — so `kill -TERM` does NOT reach the signal-safe-save path, it just parks the handler goroutine on the same blocked send. Only a signal outside that set ends it: `kill -QUIT` (Go runtime dumps goroutine stacks and dies, leaving the terminal in raw mode — run `reset` after) or `kill -9`. Neither runs `finalize`: no history auto-save and no `-o` write — but an earlier `O` flush's snapshot survives on disk. The up direction is deliberately asymmetric: the annotation sub-row renders *below* its diff line (`rowOnAnnotationSubLine` in `app/ui/annotate.go`), so clearing the flag without moving `diffCursor` IS the upward move, and that cursor strictly decreases so it cannot cycle. Absolute placements (H/M/L in `diffnav.go`, mouse click in `mouse.go`) are one-shot assignments outside these loops. Both walks also **undo** the step that carries the delta past `rows`, restoring `diffCursor` and `cursorOnAnnotation` (the only two fields `moveDiffCursor*WithHunks` mutates) and breaking immediately: one cursor step can span many rows via `hunkLineHeight`, and scrolling by that whole height moved the viewport further than a page, past rows it never rendered. The rollback cannot cycle — it is always followed by `break`. It is gated on `worthRollingBack(walked, rows)` (`walked*2 >= rows`) and that gate is **not** cosmetic: a block taller than the page has no selectable position inside it, so rolling back to the row or two walked before it scrolls the pane by almost nothing and the next press takes the same oversized step anyway. Gating on "has moved at all" instead produced exactly that one-row collapse. `walked` is 0 on the first step, so the walk can never refuse to move. A single step taller than the whole page still scrolls by its full height and still skips — unavoidable while the viewport follows the cursor, and the reason `pgdown` then `pgup` is only reversible for uniform-height content. The two directions accumulate differently and the comments say so: walking **down** the delta grows by the height of the line being *left* (`offsets[i]` is that line's top row), walking **up** by the height of the line *arrived at*, including the annotation block rendered below it. diff --git a/README.md b/README.md index 76f347d6..f0ae2abc 100644 --- a/README.md +++ b/README.md @@ -822,7 +822,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ### Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that controls it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index 31825645..fa381773 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -184,7 +184,7 @@ Press `Space` to mark the focused file reviewed. Press `F` to toggle the sidebar ## Status Bar Icons -The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that toggles it. +The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (`?`) shows each icon beside the key that controls it. | Icon | Toggle | Meaning | |------|--------|---------| diff --git a/site/docs.html b/site/docs.html index 24795132..d1f6348b 100644 --- a/site/docs.html +++ b/site/docs.html @@ -598,7 +598,7 @@

View toggles

IconToggleMeaning

Status bar icons

-

The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (?) shows each icon beside the key that toggles it.

+

The status bar shows a fixed row of mode indicators on the right side. All slots are always rendered — active modes use the status bar foreground color, inactive modes use muted gray, so the row occupies the same width regardless of what's toggled on. The help overlay (?) shows each icon beside the key that controls it.

IconToggleMeaning