Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude-plugin/skills/revdiff/references/usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 controls it.

| Icon | Toggle | Meaning |
|------|--------|---------|
Expand Down
2 changes: 1 addition & 1 deletion .claude/rules/gotchas.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 controls it.

| Icon | Toggle | Meaning |
|------|--------|---------|
Expand Down
23 changes: 19 additions & 4 deletions app/fsutil/fsutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,20 @@ package fsutil

import (
"fmt"
"io"
"os"
"path/filepath"
)

//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.
// *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 {
Expand All @@ -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)
}
Expand Down
32 changes: 32 additions & 0 deletions app/fsutil/fsutil_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fsutil

import (
"errors"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -76,6 +77,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 := &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 := &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")
Expand Down
107 changes: 107 additions & 0 deletions app/fsutil/temp_file_moq_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 22 additions & 3 deletions app/ui/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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
Expand Down
46 changes: 46 additions & 0 deletions app/ui/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading