From 7a2106e466ea2e9b4245ea185e8137b8eb1637ff Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Wed, 15 Jul 2026 18:36:49 -0700 Subject: [PATCH 1/6] feat(ui): configure tree pane position --- .claude-plugin/skills/revdiff/SKILL.md | 2 + .../skills/revdiff/references/config.md | 3 +- .../skills/revdiff/references/usage.md | 2 +- README.md | 11 +++- app/config.go | 1 + app/config_test.go | 32 ++++++++++ app/main.go | 1 + app/ui/doc.go | 2 +- app/ui/model.go | 3 + app/ui/model_test.go | 5 ++ app/ui/mouse.go | 22 ++++--- app/ui/mouse_test.go | 38 ++++++++++++ app/ui/view.go | 19 +++--- app/ui/view_test.go | 59 +++++++++++++++++++ docs/ARCHITECTURE.md | 4 +- plugins/codex/skills/revdiff/SKILL.md | 2 + .../codex/skills/revdiff/references/config.md | 3 +- .../codex/skills/revdiff/references/usage.md | 2 +- plugins/pi/skills/revdiff/SKILL.md | 2 + site/docs.html | 4 +- 20 files changed, 189 insertions(+), 28 deletions(-) diff --git a/.claude-plugin/skills/revdiff/SKILL.md b/.claude-plugin/skills/revdiff/SKILL.md index 65333bbd..55d3ecc7 100644 --- a/.claude-plugin/skills/revdiff/SKILL.md +++ b/.claude-plugin/skills/revdiff/SKILL.md @@ -122,6 +122,8 @@ When you are launching revdiff for the user (e.g., right after a refactor or ana **When the recent change likely created new untracked files** (new packages, new test files, new docs, new scripts that haven't been `git add`-ed yet), pass `--untracked` so those files appear in the tree. Use this in working-tree mode (no ref, no `--staged`); skip it for ref-to-ref reviews where untracked files are not part of the historical diff. +**When the user explicitly requests the file tree on a side**, pass `--tree-position=left` or `--tree-position=right`. Otherwise omit it and respect the user's config. + Run the launcher through the override-chain resolver: ```bash diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index 0f5e825d..3fcc696f 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -21,6 +21,7 @@ Then uncomment and edit the values you want to change. | `--staged` | `REVDIFF_STAGED` | Show staged changes | `false` | | `--untracked` | `REVDIFF_UNTRACKED` | Show untracked files in the tree | `false` | | `--tree-width` | `REVDIFF_TREE_WIDTH` | File tree panel width in units (1-10) | `2` | +| `--tree-position` | `REVDIFF_TREE_POSITION` | File tree and markdown TOC position (`left` or `right`) | `left` | | `--tab-width` | `REVDIFF_TAB_WIDTH` | Spaces per tab character | `4` | | `--no-colors` | `REVDIFF_NO_COLORS` | Disable all colors including syntax highlighting | `false` | | `--no-status-bar` | `REVDIFF_NO_STATUS_BAR` | Hide the status bar | `false` | @@ -60,7 +61,7 @@ Then uncomment and edit the values you want to change. | `--config` | `REVDIFF_CONFIG` | Path to config file | `~/.config/revdiff/config` | | `--dump-config` | | Print default config to stdout and exit | | -Config-backed options use long flag names without leading `--`; for annotation exit status use `exit-code-on-annotations = true`. +Config-backed options use long flag names without leading `--`; for example, use `tree-position = right` to put the file tree and markdown TOC on the right. For annotation exit status use `exit-code-on-annotations = true`. ## Popup Size (Claude Code plugin) diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index c3bf536a..12bc8f48 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -35,7 +35,7 @@ When a diff contains exactly one file, revdiff automatically hides the file tree ## Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the left listing all markdown headers with indentation by level. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Set `tree-position = right` in config, pass `--tree-position=right`, or set `REVDIFF_TREE_POSITION=right` to move both the file tree and TOC to the right. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. ## All-Files Mode diff --git a/README.md b/README.md index 9d0d79be..2c6929fc 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ Built for a specific use case: reviewing code changes, plans, and documents with - Blame gutter: shows author name and commit age per line, toggle with `B` - Annotate any line in the diff (added, removed, or context) plus file-level notes - Single-file auto-detection: when a diff contains exactly one file, hides the tree pane and gives full terminal width to the diff view -- Two-pane TUI: file tree (left) + colorized diff viewport (right) +- Two-pane TUI: file tree and colorized diff viewport, with configurable tree position (left by default) - Vim-style `/` search within diff with `n`/`N` match navigation - Hunk navigation to jump between change groups - Annotation list popup (`@`): browse all annotations across files, jump to any annotation @@ -354,6 +354,7 @@ Positional arguments support several forms: | `--staged` | Show staged changes, env: `REVDIFF_STAGED` | `false` | | `--untracked` | Show untracked files in the tree, env: `REVDIFF_UNTRACKED` | `false` | | `--tree-width` | File tree panel width in units (1-10), env: `REVDIFF_TREE_WIDTH` | `2` | +| `--tree-position` | File tree and markdown TOC position (`left` or `right`), env: `REVDIFF_TREE_POSITION` | `left` | | `--tab-width` | Number of spaces per tab character, env: `REVDIFF_TAB_WIDTH` | `4` | | `--no-colors` | Disable all colors including syntax highlighting, env: `REVDIFF_NO_COLORS` | `false` | | `--no-status-bar` | Hide the status bar, env: `REVDIFF_NO_STATUS_BAR` | `false` | @@ -412,7 +413,11 @@ mkdir -p ~/.config/revdiff revdiff --dump-config > ~/.config/revdiff/config ``` -Then uncomment and edit the values you want to change. +Then uncomment and edit the values you want to change. For example, place the file tree and markdown TOC on the right: + +```ini +tree-position = right +``` ### Themes @@ -665,7 +670,7 @@ revdiff HEAD~3 --description-file=.review-description.md ### Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md` or `printf '# title\n' | revdiff --stdin --stdin-name plan.md`), revdiff shows a table-of-contents pane on the left listing all markdown headers. Use `Tab` to switch focus between the TOC and diff panes, `j`/`k` to navigate headers, and `Enter` to jump to a header in the diff. The TOC automatically highlights the current section as you scroll through the file. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md` or `printf '# title\n' | revdiff --stdin --stdin-name plan.md`), revdiff shows a table-of-contents pane on the configured tree side (left by default) listing all markdown headers. Use `Tab` to switch focus between the TOC and diff panes, `j`/`k` to navigate headers, and `Enter` to jump to a header in the diff. The TOC automatically highlights the current section as you scroll through the file. This mode activates when all three conditions are met: single file, markdown extension (`.md`/`.markdown`), and all lines are context (no diff changes). Headers inside fenced code blocks are excluded from the TOC. diff --git a/app/config.go b/app/config.go index a7327185..cc63b4a9 100644 --- a/app/config.go +++ b/app/config.go @@ -20,6 +20,7 @@ type options struct { Staged bool `long:"staged" ini-name:"staged" env:"REVDIFF_STAGED" description:"show staged changes"` Untracked bool `long:"untracked" ini-name:"untracked" env:"REVDIFF_UNTRACKED" description:"show untracked files in the tree"` TreeWidth int `long:"tree-width" ini-name:"tree-width" env:"REVDIFF_TREE_WIDTH" default:"2" description:"file tree panel width in units (1-10, default 2 of 10)"` + TreePosition string `long:"tree-position" ini-name:"tree-position" env:"REVDIFF_TREE_POSITION" choice:"left" choice:"right" default:"left" description:"file tree and markdown TOC position"` TabWidth int `long:"tab-width" ini-name:"tab-width" env:"REVDIFF_TAB_WIDTH" default:"4" description:"number of spaces per tab character"` NoColors bool `long:"no-colors" ini-name:"no-colors" env:"REVDIFF_NO_COLORS" description:"disable all colors including syntax highlighting"` NoStatusBar bool `long:"no-status-bar" ini-name:"no-status-bar" env:"REVDIFF_NO_STATUS_BAR" description:"hide the status bar"` diff --git a/app/config_test.go b/app/config_test.go index a3ec94db..bb1f0e9e 100644 --- a/app/config_test.go +++ b/app/config_test.go @@ -21,6 +21,7 @@ func TestParseArgs_Defaults(t *testing.T) { opts, err := parseArgs(noConfigArgs(t)) require.NoError(t, err) assert.Equal(t, 2, opts.TreeWidth) + assert.Equal(t, "left", opts.TreePosition) assert.Equal(t, 4, opts.TabWidth) assert.Equal(t, "catppuccin-macchiato", opts.ChromaStyle) assert.Equal(t, "πŸ’¬", opts.AnnotationMarker) @@ -48,6 +49,36 @@ func TestParseArgs_Defaults(t *testing.T) { assert.Equal(t, "catppuccin-latte", opts.AutoThemeLight) } +func TestParseArgs_TreePosition(t *testing.T) { + t.Run("flag", func(t *testing.T) { + opts, err := parseArgs(append(noConfigArgs(t), "--tree-position=right")) + require.NoError(t, err) + assert.Equal(t, "right", opts.TreePosition) + }) + + t.Run("env", func(t *testing.T) { + t.Setenv("REVDIFF_TREE_POSITION", "right") + opts, err := parseArgs(noConfigArgs(t)) + require.NoError(t, err) + assert.Equal(t, "right", opts.TreePosition) + }) + + t.Run("config file", func(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), "config") + err := os.WriteFile(cfgPath, []byte("[Application Options]\ntree-position = right\n"), 0o600) + require.NoError(t, err) + + opts, err := parseArgs([]string{"--config", cfgPath}) + require.NoError(t, err) + assert.Equal(t, "right", opts.TreePosition) + }) + + t.Run("invalid", func(t *testing.T) { + _, err := parseArgs(append(noConfigArgs(t), "--tree-position=center")) + require.Error(t, err) + }) +} + func TestParseArgs_NoConfirmDiscard(t *testing.T) { t.Run("flag", func(t *testing.T) { opts, err := parseArgs(append(noConfigArgs(t), "--no-confirm-discard")) @@ -855,6 +886,7 @@ func TestDumpConfig(t *testing.T) { assert.Contains(t, output, "[Application Options]") assert.Contains(t, output, "chroma-style = catppuccin-macchiato") + assert.Contains(t, output, "tree-position = left") assert.Contains(t, output, "cross-file-hunks = false") assert.Contains(t, output, "exit-code-on-annotations = false") assert.Contains(t, output, "no-mouse = false") diff --git a/app/main.go b/app/main.go index be91347d..4a6bf032 100644 --- a/app/main.go +++ b/app/main.go @@ -231,6 +231,7 @@ func run(opts options) (int, error) { Ref: opts.ref(), Staged: opts.Staged, TreeWidthRatio: opts.TreeWidth, + TreeOnRight: opts.TreePosition == "right", Only: opts.Only, WorkDir: workDir, SourceEditor: sourceEditorPolicy(opts, workDir), diff --git a/app/ui/doc.go b/app/ui/doc.go index 4e66f160..f7870845 100644 --- a/app/ui/doc.go +++ b/app/ui/doc.go @@ -59,7 +59,7 @@ // (styleResolver, styleRenderer, sgrProcessor) defined in model.go; concrete // implementations live in the style sub-package. // -// Left-pane navigation components live in the [sidepane] sub-package (app/ui/sidepane/). +// Navigation-pane components live in the [sidepane] sub-package (app/ui/sidepane/). // It owns the file tree (FileTree) and markdown table-of-contents (TOC) types, // including cursor/offset management, entry parsing, and rendering logic. // Model holds sidepane types through consumer-side interfaces (FileTreeComponent, diff --git a/app/ui/model.go b/app/ui/model.go index 64f1972a..30a80771 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -327,6 +327,7 @@ type modelConfigState struct { noConfirmReload bool // skip confirmation prompt on reload (R) crossFileHunks bool // allow [ and ] to jump across file boundaries treeWidthRatio int // 1-10 units for file tree panel + treeOnRight bool // render the file tree or markdown TOC to the right of the diff tabSpaces string // spaces to replace tabs with wrapIndent int // extra indent (in columns) for wrap continuation rows; 0 disables annotPrefix string // cached: marker + " " @@ -728,6 +729,7 @@ type ModelConfig struct { Ref string Staged bool TreeWidthRatio int + TreeOnRight bool // render the file tree or markdown TOC to the right of the diff TabWidth int // number of spaces per tab character NoColors bool // disable all colors including syntax highlighting MouseTracking bool // enable mouse tracking for clicks and wheel events @@ -887,6 +889,7 @@ func NewModel(cfg ModelConfig) (Model, error) { noConfirmReload: cfg.NoConfirmReload, crossFileHunks: cfg.CrossFileHunks, treeWidthRatio: cfg.TreeWidthRatio, + treeOnRight: cfg.TreeOnRight, tabSpaces: strings.Repeat(" ", cfg.TabWidth), wrapIndent: max(0, cfg.WrapIndent), annotPrefix: cfg.AnnotationMarker + " ", diff --git a/app/ui/model_test.go b/app/ui/model_test.go index 5219699f..7ab7d478 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -260,6 +260,11 @@ func TestNewModel_OptionalDefaults(t *testing.T) { m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreeWidthRatio: 5}) assert.Equal(t, 5, m.cfg.treeWidthRatio) }) + + t.Run("TreeOnRight is kept", func(t *testing.T) { + m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreeOnRight: true}) + assert.True(t, m.cfg.treeOnRight) + }) } func TestModel_Init(t *testing.T) { diff --git a/app/ui/mouse.go b/app/ui/mouse.go index d686e7fa..f3fba0cf 100644 --- a/app/ui/mouse.go +++ b/app/ui/mouse.go @@ -93,8 +93,8 @@ func (m Model) treeTopRow() int { // hitTest classifies a screen coordinate into a hitZone for mouse-event routing. // the classification is pure arithmetic over m.layout state and does not // inspect any dynamic UI content. ordering matters: status bar is checked -// first (y at bottom), then x is used to split tree vs diff columns, and -// finally y is used within each column to reject the diff header row or tree +// first (y at bottom), then x is used to identify the configured tree side, +// and finally y is used within each pane to reject the diff header row or tree // top border. func (m Model) hitTest(x, y int) hitZone { if x < 0 || y < 0 || x >= m.layout.width || y >= m.layout.height { @@ -111,14 +111,18 @@ func (m Model) hitTest(x, y int) hitZone { return hitNone } - // tree block spans columns [0, treeWidth+1] when visible: left border + - // treeWidth content columns + right border = treeWidth+2 columns total. - // diff block picks up at column treeWidth+2. - if !m.treePaneHidden() && x < m.layout.treeWidth+2 { - if y < m.treeTopRow() { - return hitNone + if !m.treePaneHidden() { + treeStart := 0 + if m.cfg.treeOnRight { + // pane width includes the content plus its left and right borders. + treeStart = m.layout.width - m.layout.treeWidth - 2 + } + if x >= treeStart && x < treeStart+m.layout.treeWidth+2 { + if y < m.treeTopRow() { + return hitNone + } + return hitTree } - return hitTree } if y == 0 { diff --git a/app/ui/mouse_test.go b/app/ui/mouse_test.go index 7d3c7e83..4a7d246d 100644 --- a/app/ui/mouse_test.go +++ b/app/ui/mouse_test.go @@ -93,6 +93,34 @@ func TestModel_hitTest(t *testing.T) { {name: "status bar at x=0", setup: func(m *Model) {}, x: 0, y: 39, want: hitStatus}, {name: "tree-diff boundary: last tree column", setup: func(m *Model) {}, x: 37, y: 10, want: hitTree}, {name: "tree-diff boundary: first diff column", setup: func(m *Model) {}, x: 38, y: 10, want: hitDiff}, + { + name: "right tree: last diff column", + setup: func(m *Model) { + m.cfg.treeOnRight = true + }, + x: 81, y: 10, want: hitDiff, + }, + { + name: "right tree: first tree column", + setup: func(m *Model) { + m.cfg.treeOnRight = true + }, + x: 82, y: 10, want: hitTree, + }, + { + name: "right tree: tree top border", + setup: func(m *Model) { + m.cfg.treeOnRight = true + }, + x: 100, y: 0, want: hitNone, + }, + { + name: "right tree: diff header on left", + setup: func(m *Model) { + m.cfg.treeOnRight = true + }, + x: 5, y: 1, want: hitHeader, + }, {name: "x negative", setup: func(m *Model) {}, x: -1, y: 10, want: hitNone}, {name: "y negative", setup: func(m *Model) {}, x: 60, y: -1, want: hitNone}, {name: "x out of bounds (= width)", setup: func(m *Model) {}, x: 120, y: 10, want: hitNone}, @@ -432,6 +460,16 @@ func TestModel_HandleMouse_WheelInTreeMovesTreeCursor(t *testing.T) { assert.Equal(t, "ab.go", model.tree.SelectedFile(), "single wheel-down notch must advance tree cursor by exactly one entry") }) + t.Run("right-positioned tree receives wheel events on the right", func(t *testing.T) { + m := mouseTestModel(t, files, diffs) + m.cfg.treeOnRight = true + require.Equal(t, "aa.go", m.tree.SelectedFile()) + + result, _ := m.Update(wheelMsg(tea.MouseButtonWheelDown, 100, 3, false)) + model := result.(Model) + assert.Equal(t, "ab.go", model.tree.SelectedFile()) + }) + t.Run("plain wheel-up retreats exactly one entry", func(t *testing.T) { m := mouseTestModel(t, files, diffs) m.tree.SelectByPath("ac.go") // cursor at entry 2 diff --git a/app/ui/view.go b/app/ui/view.go index 80c9e70c..ae88ab5d 100644 --- a/app/ui/view.go +++ b/app/ui/view.go @@ -62,7 +62,7 @@ func (m Model) View() string { mainView = m.applyScrollbar(diffPane) case m.file.singleFile && m.file.mdTOC != nil: - // single-file markdown with TOC: two-pane layout with TOC in left pane + // single-file markdown with TOC: use the configured navigation pane side tocContent := m.file.mdTOC.Render(sidepane.TOCRender{Width: m.layout.treeWidth, Height: ph, Focused: m.layout.focus == paneTree, Resolver: m.resolver}) mainView = m.renderTwoPaneLayout(tocContent, diffContent, m.file.mdTOC.ScrollState(), ph, diffPaneW) @@ -82,12 +82,12 @@ func (m Model) View() string { return lipgloss.JoinVertical(lipgloss.Left, mainView, status) } -// renderTwoPaneLayout renders a two-pane layout with left (tree/TOC) and right (diff) content. +// renderTwoPaneLayout renders the navigation and diff panes in the configured order. // applies focus-based pane styles, background padding, scrollbars, and joins horizontally. // diffPaneW is the inner width caller passed to truncateHeaderTitle and must // match the lipgloss Width() applied here β€” single source of truth for the // scrollbar's single-line-header invariant. -func (m Model) renderTwoPaneLayout(leftContent, diffContent string, leftScroll sidepane.ScrollState, ph, diffPaneW int) string { +func (m Model) renderTwoPaneLayout(navigationContent, diffContent string, navigationScroll sidepane.ScrollState, ph, diffPaneW int) string { treeStyle := m.resolver.Style(style.StyleKeyTreePane) diffStyle := m.resolver.Style(style.StyleKeyDiffPane) if m.layout.focus == paneTree { @@ -96,14 +96,14 @@ func (m Model) renderTwoPaneLayout(leftContent, diffContent string, leftScroll s diffStyle = m.resolver.Style(style.StyleKeyDiffPaneActive) } - leftContent = m.padContentBg(leftContent, m.layout.treeWidth, m.resolver.Color(style.ColorKeyTreePaneBg)) + navigationContent = m.padContentBg(navigationContent, m.layout.treeWidth, m.resolver.Color(style.ColorKeyTreePaneBg)) diffContent = m.padContentBg(diffContent, diffPaneW, m.resolver.Color(style.ColorKeyDiffPaneBg)) - leftPane := treeStyle. + navigationPane := treeStyle. Width(m.layout.treeWidth). Height(ph). - Render(leftContent) - leftPane = m.applyNavigationScrollbar(leftPane, leftScroll) + Render(navigationContent) + navigationPane = m.applyNavigationScrollbar(navigationPane, navigationScroll) diffPane := diffStyle. Width(diffPaneW). @@ -111,7 +111,10 @@ func (m Model) renderTwoPaneLayout(leftContent, diffContent string, leftScroll s Render(diffContent) diffPane = m.applyScrollbar(diffPane) - return lipgloss.JoinHorizontal(lipgloss.Top, leftPane, diffPane) + if m.cfg.treeOnRight { + return lipgloss.JoinHorizontal(lipgloss.Top, diffPane, navigationPane) + } + return lipgloss.JoinHorizontal(lipgloss.Top, navigationPane, diffPane) } // truncateHeaderTitle returns the diff pane header text shortened to fit diff --git a/app/ui/view_test.go b/app/ui/view_test.go index 245e9b3f..b1336ba3 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -607,6 +607,65 @@ func TestModel_ViewSingleFileMode(t *testing.T) { }) } +func TestModel_ViewTreePosition(t *testing.T) { + assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, navigationOnRight bool) { + t.Helper() + labelColumn := func(lines []string, label string) int { + for _, line := range lines { + if idx := strings.Index(line, label); idx >= 0 { + return idx + } + } + return -1 + } + + lines := strings.Split(ansi.Strip(view), "\n") + diffIdx := labelColumn(lines, diffLabel) + navigationIdx := labelColumn(lines, navigationLabel) + require.NotEqual(t, -1, diffIdx, "diff label must appear in the rendered view") + require.NotEqual(t, -1, navigationIdx, "navigation label must appear in the rendered view") + if navigationOnRight { + assert.Less(t, diffIdx, navigationIdx) + return + } + assert.Less(t, navigationIdx, diffIdx) + } + + for _, tc := range []struct { + name string + navigationOnRight bool + }{ + {name: "file tree on left"}, + {name: "file tree on right", navigationOnRight: true}, + } { + t.Run(tc.name, func(t *testing.T) { + m := testModel([]string{"tree.go", "other.go"}, nil) + m.tree = testNewFileTree([]string{"tree.go", "other.go"}) + m.file.name = "current.go" + m.cfg.treeOnRight = tc.navigationOnRight + m.cfg.noStatusBar = true + + assertOrder(t, m.View(), "current.go", "tree.go", tc.navigationOnRight) + }) + } + + t.Run("markdown TOC follows tree position", func(t *testing.T) { + m := testModel([]string{"plan.md"}, nil) + m.tree = testNewFileTree([]string{"plan.md"}) + m.file.singleFile = true + m.file.mdTOC = sidepane.ParseTOC( + []diff.DiffLine{{NewNum: 1, Content: "# Navigation section", ChangeType: diff.ChangeContext}}, + "plan.md", + ) + require.NotNil(t, m.file.mdTOC) + m.file.name = "plan.md" + m.cfg.treeOnRight = true + m.cfg.noStatusBar = true + + assertOrder(t, m.View(), "plan.md", "Navigation section", true) + }) +} + func TestModel_ViewRenameHeader(t *testing.T) { t.Run("renamed file header shows old to new", func(t *testing.T) { m := testModel([]string{"new.go"}, nil) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f82cea8c..2b85a2f2 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -146,13 +146,13 @@ Three main types: `display.go` holds two package-level functions rather than methods on those types: `SanitizeFilenameForDisplay()` strips control, ANSI/OSC, and bidi sequences out of repository-supplied filenames, and `TruncateLeftToWidth()` left-truncates with an ellipsis. Both are shared by the diff-pane header, the status bar, and the file picker; any new filename-rendering surface must route through them. -### app/ui/sidepane/ β€” left-pane navigation +### app/ui/sidepane/ β€” navigation pane Two independent component types, both with cursor/offset management, rendering, and keyboard navigation: - **`FileTree`** β€” file tree sidebar. Supports navigation (`Move`/`StepFile`), filtering (annotated-only), semantic-fingerprint reviewed tracking, directory grouping. File-list reloads revalidate only paths reviewed before the load; marks added during the load are reconciled when that file's refreshed diff arrives. `VisibleFiles()` exposes file paths in rendered order after active filters for consumers such as the file picker. - **`TOC`** β€” markdown table-of-contents. Activated for single-file full-context markdown. Active section tracking, header-level navigation -Both constructed via factory closures in `main.go`, consumed through `FileTreeComponent`/`TOCComponent` interfaces. +Both are constructed via factory closures in `main.go`, consumed through `FileTreeComponent`/`TOCComponent` interfaces, and rendered on the left or right according to `tree-position`. ### app/ui/overlay/ β€” popup layers diff --git a/plugins/codex/skills/revdiff/SKILL.md b/plugins/codex/skills/revdiff/SKILL.md index 54a717c1..b73a8699 100644 --- a/plugins/codex/skills/revdiff/SKILL.md +++ b/plugins/codex/skills/revdiff/SKILL.md @@ -138,6 +138,8 @@ When you are launching revdiff for the user (e.g., right after a refactor or ana **When the recent change likely created new untracked files** (new packages, new test files, new docs, new scripts that haven't been `git add`-ed yet), pass `--untracked` so those files appear in the tree. Use this in working-tree mode (no ref, no `--staged`); skip it for ref-to-ref reviews where untracked files are not part of the historical diff. +**When the user explicitly requests the file tree on a side**, pass `--tree-position=left` or `--tree-position=right`. Otherwise omit it and respect the user's config. + Run the launcher script: ```bash diff --git a/plugins/codex/skills/revdiff/references/config.md b/plugins/codex/skills/revdiff/references/config.md index baa541d2..89136e99 100644 --- a/plugins/codex/skills/revdiff/references/config.md +++ b/plugins/codex/skills/revdiff/references/config.md @@ -21,6 +21,7 @@ Then uncomment and edit the values you want to change. | `--staged` | `REVDIFF_STAGED` | Show staged changes | `false` | | `--untracked` | `REVDIFF_UNTRACKED` | Show untracked files in the tree | `false` | | `--tree-width` | `REVDIFF_TREE_WIDTH` | File tree panel width in units (1-10) | `2` | +| `--tree-position` | `REVDIFF_TREE_POSITION` | File tree and markdown TOC position (`left` or `right`) | `left` | | `--tab-width` | `REVDIFF_TAB_WIDTH` | Spaces per tab character | `4` | | `--no-colors` | `REVDIFF_NO_COLORS` | Disable all colors including syntax highlighting | `false` | | `--no-status-bar` | `REVDIFF_NO_STATUS_BAR` | Hide the status bar | `false` | @@ -59,7 +60,7 @@ Then uncomment and edit the values you want to change. | `--config` | `REVDIFF_CONFIG` | Path to config file | `~/.config/revdiff/config` | | `--dump-config` | | Print default config to stdout and exit | | -Config-backed options use long flag names without leading `--`; for annotation exit status use `exit-code-on-annotations = true`. +Config-backed options use long flag names without leading `--`; for example, use `tree-position = right` to put the file tree and markdown TOC on the right. For annotation exit status use `exit-code-on-annotations = true`. ## Popup Size (Claude Code plugin) diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index fe2a63fb..a3fc751d 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -35,7 +35,7 @@ When a diff contains exactly one file, revdiff automatically hides the file tree ## Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the left listing all markdown headers with indentation by level. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Set `tree-position = right` in config, pass `--tree-position=right`, or set `REVDIFF_TREE_POSITION=right` to move both the file tree and TOC to the right. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. ## All-Files Mode diff --git a/plugins/pi/skills/revdiff/SKILL.md b/plugins/pi/skills/revdiff/SKILL.md index e8e0c351..9ad2e8ef 100644 --- a/plugins/pi/skills/revdiff/SKILL.md +++ b/plugins/pi/skills/revdiff/SKILL.md @@ -23,6 +23,7 @@ Reference resolution rules: - For date requests, resolve the commit first. Examples: `2 weeks ago`, `yesterday`, `last Friday` β†’ run `git rev-list -1 --before= HEAD`, then pass the resulting commit hash as `args`. - For file targets, use `args: "--only "`. - For all-files requests, map excludes explicitly. Example: `all files exclude vendor and dist` β†’ `args: "--all-files --exclude=vendor --exclude=dist"`. +- When the user explicitly requests the file tree on a side, pass `--tree-position=left` or `--tree-position=right`; otherwise respect their config. - For explicit refs, ranges, flags, or two-ref requests, pass them through as revdiff args. - If the requested natural language target cannot be resolved, say what failed and ask for a concrete ref/path. Do not guess silently. @@ -34,6 +35,7 @@ Tool examples: - `args: "--untracked"`: review untracked files with working-tree changes - `args: "--only README.md"`: review one standalone file - `args: "--all-files --exclude vendor"`: review all tracked files except vendor +- `args: "--tree-position=right main"`: review with the file tree on the right - `args: "--description='why this refactor matters' main"`: include review context in the info popup - `args: "--description-file=/tmp/revdiff-desc.md main"`: include longer markdown review context - `args: "--annotations=/tmp/revdiff-review.md main"`: preload in-session review notes diff --git a/site/docs.html b/site/docs.html index 4a9daf1f..23abfbde 100644 --- a/site/docs.html +++ b/site/docs.html @@ -289,7 +289,7 @@

Review description

--description and --description-file are mutually exclusive. The description section is hidden when neither is set, so the flag is purely additive.

Markdown TOC navigation

-

When reviewing a single markdown file in context-only mode, a table-of-contents pane appears on the left listing all markdown headers. This works for --only files and for --stdin when --stdin-name ends with .md or .markdown. Use Tab to switch focus, j/k to navigate, Enter to jump. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded.

+

When reviewing a single markdown file in context-only mode, a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers. This works for --only files and for --stdin when --stdin-name ends with .md or .markdown. Use Tab to switch focus, j/k to navigate, Enter to jump. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded.

Beyond code review

The --only and --stdin flags enable use cases beyond git diffs. Files and ephemeral command output can both be loaded for annotation, no git repo required.

@@ -388,6 +388,7 @@

Config file

Precedence: CLI flags > env vars > config file > built-in defaults. For annotation exit status, use REVDIFF_EXIT_CODE_ON_ANNOTATIONS or exit-code-on-annotations = true.

mkdir -p ~/.config/revdiff revdiff --dump-config > ~/.config/revdiff/config
+

Set tree-position = right to place the file tree and markdown TOC on the right.

Options

@@ -396,6 +397,7 @@

Options

+ From 1271ba020ed77201513d162a6ac7ec613e5a86d1 Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Sat, 18 Jul 2026 20:58:58 -0700 Subject: [PATCH 2/6] feat(keymap): support dynamic h/l bindings based on tree position - update default keymap generation to swap h/l keys when tree is on the right - add DefaultForTreePosition and LoadOrDefaultForTreePosition helpers - pass tree position config through to keymap loaders - update UI model and navigation tests to verify correct pane switching for both tree positions --- app/keymap/keymap.go | 40 ++++++++++++++++++++++++++++++--------- app/keymap/keymap_test.go | 21 ++++++++++++++++++++ app/main.go | 7 +++++-- app/ui/model.go | 2 +- app/ui/model_test.go | 40 ++++++++++++++++++++++++++------------- app/ui/view_test.go | 16 ++++++++++++++++ 6 files changed, 101 insertions(+), 25 deletions(-) diff --git a/app/keymap/keymap.go b/app/keymap/keymap.go index c3baa85f..954bf50e 100644 --- a/app/keymap/keymap.go +++ b/app/keymap/keymap.go @@ -254,8 +254,9 @@ func defaultDescriptions() []HelpEntry { } // defaultBindings returns the default key-to-action mapping. -func defaultBindings() map[string]Action { - return map[string]Action{ +// h/l follow the visual pane order while the actions remain semantic. +func defaultBindings(treeOnRight bool) map[string]Action { + bindings := map[string]Action{ "j": ActionDown, "k": ActionUp, "down": ActionDown, @@ -310,12 +311,23 @@ func defaultBindings() map[string]Action { "R": ActionReload, "esc": ActionDismiss, } + if treeOnRight { + bindings["h"] = ActionFocusDiff + bindings["l"] = ActionFocusTree + } + return bindings } -// Default returns a Keymap with all default bindings. +// Default returns a Keymap with the default left-positioned tree bindings. func Default() *Keymap { + return DefaultForTreePosition(false) +} + +// DefaultForTreePosition returns a Keymap whose h/l pane bindings follow the +// visual pane order for the configured tree position. +func DefaultForTreePosition(treeOnRight bool) *Keymap { return &Keymap{ - bindings: defaultBindings(), + bindings: defaultBindings(treeOnRight), descriptions: defaultDescriptions(), } } @@ -633,6 +645,10 @@ func parse(r io.Reader) (maps []mapEntry, unmaps []string, err error) { // Load reads a keybindings file from path and returns a Keymap with defaults // overridden by the file contents. Returns error if the file cannot be opened or parsed. func Load(path string) (*Keymap, error) { + return load(path, false) +} + +func load(path string, treeOnRight bool) (*Keymap, error) { f, err := os.Open(path) //nolint:gosec // path is user-provided config file location if err != nil { return nil, fmt.Errorf("opening keybindings file: %w", err) @@ -644,7 +660,7 @@ func Load(path string) (*Keymap, error) { return nil, err } - km := Default() + km := DefaultForTreePosition(treeOnRight) // apply unmaps first, then maps (so "unmap q" + "map x quit" works) for _, key := range unmaps { @@ -680,16 +696,22 @@ func (km *Keymap) resolveConflicts() { // LoadOrDefault loads keybindings from path if the file exists, otherwise returns // Default(). Parse errors are logged as warnings and Default() is returned. func LoadOrDefault(path string) *Keymap { + return LoadOrDefaultForTreePosition(path, false) +} + +// LoadOrDefaultForTreePosition loads user overrides on top of defaults whose +// h/l pane bindings follow the configured tree position. +func LoadOrDefaultForTreePosition(path string, treeOnRight bool) *Keymap { if path == "" { - return Default() + return DefaultForTreePosition(treeOnRight) } if _, err := os.Stat(path); os.IsNotExist(err) { - return Default() + return DefaultForTreePosition(treeOnRight) } - km, err := Load(path) + km, err := load(path, treeOnRight) if err != nil { log.Printf("[WARN] failed to load keybindings from %s: %v, using defaults", path, err) - return Default() + return DefaultForTreePosition(treeOnRight) } return km } diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index 31b7f880..9ab17fc1 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -18,6 +18,17 @@ func TestDefault(t *testing.T) { assert.NotEmpty(t, km.descriptions) } +func TestDefaultForTreePosition(t *testing.T) { + left := DefaultForTreePosition(false) + assert.Equal(t, ActionFocusTree, left.Resolve("h")) + assert.Equal(t, ActionFocusDiff, left.Resolve("l")) + + right := DefaultForTreePosition(true) + assert.Equal(t, ActionFocusDiff, right.Resolve("h")) + assert.Equal(t, ActionFocusTree, right.Resolve("l")) + assert.Equal(t, ActionDown, right.Resolve("j"), "unrelated defaults must stay unchanged") +} + func TestDefault_allExpectedBindings(t *testing.T) { km := Default() tests := []struct { @@ -759,6 +770,16 @@ func TestLoadOrDefault_withFile(t *testing.T) { assert.Equal(t, ActionDown, km.Resolve("j")) // defaults still present } +func TestLoadOrDefaultForTreePosition_UserOverridesDefaults(t *testing.T) { + tmpFile := t.TempDir() + "/keybindings" + err := os.WriteFile(tmpFile, []byte("map h quit\nunmap l\n"), 0o600) + require.NoError(t, err) + + km := LoadOrDefaultForTreePosition(tmpFile, true) + assert.Equal(t, ActionQuit, km.Resolve("h")) + assert.Empty(t, km.Resolve("l")) +} + func TestLoad_unmapOfUnboundKey(t *testing.T) { tmpFile := t.TempDir() + "/keybindings" err := os.WriteFile(tmpFile, []byte("unmap z\n"), 0o600) diff --git a/app/main.go b/app/main.go index 4a6bf032..0c4e3061 100644 --- a/app/main.go +++ b/app/main.go @@ -55,7 +55,10 @@ func main() { } if opts.DumpKeys { - km := keymap.LoadOrDefault(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath)) + km := keymap.LoadOrDefaultForTreePosition( + resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), + opts.TreePosition == "right", + ) if err := km.Dump(os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) @@ -108,7 +111,7 @@ func run(opts options) (int, error) { store := annotation.NewStore() hl := highlight.New(opts.ChromaStyle, !opts.NoColors) - km := keymap.LoadOrDefault(resolveKeysPath(opts)) + km := keymap.LoadOrDefaultForTreePosition(resolveKeysPath(opts), opts.TreePosition == "right") var ( renderer ui.Renderer diff --git a/app/ui/model.go b/app/ui/model.go index 30a80771..71a32d42 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -847,7 +847,7 @@ func NewModel(cfg ModelConfig) (Model, error) { } km := cfg.Keymap if km == nil { - km = keymap.Default() + km = keymap.DefaultForTreePosition(cfg.TreeOnRight) } ed := cfg.Editor if ed == nil || isNilValue(ed) { diff --git a/app/ui/model_test.go b/app/ui/model_test.go index 7ab7d478..524b3d22 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -261,9 +261,11 @@ func TestNewModel_OptionalDefaults(t *testing.T) { assert.Equal(t, 5, m.cfg.treeWidthRatio) }) - t.Run("TreeOnRight is kept", func(t *testing.T) { + t.Run("TreeOnRight is kept and default pane keys follow layout", func(t *testing.T) { m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreeOnRight: true}) assert.True(t, m.cfg.treeOnRight) + assert.Equal(t, keymap.ActionFocusDiff, m.keymap.Resolve("h")) + assert.Equal(t, keymap.ActionFocusTree, m.keymap.Resolve("l")) }) } @@ -506,20 +508,32 @@ func TestModel_TreeNavigation(t *testing.T) { } func TestModel_FocusSwitching(t *testing.T) { - m := testModel([]string{"a.go"}, nil) - m.tree = testNewFileTree([]string{"a.go"}) - m.file.name = "a.go" // pretend a file is loaded - m.layout.focus = paneTree + for _, tc := range []struct { + name string + treeOnRight bool + toDiffKey rune + toTreeKey rune + }{ + {name: "tree on left", toDiffKey: 'l', toTreeKey: 'h'}, + {name: "tree on right", treeOnRight: true, toDiffKey: 'h', toTreeKey: 'l'}, + } { + t.Run(tc.name, func(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.tree = testNewFileTree([]string{"a.go"}) + m.keymap = keymap.DefaultForTreePosition(tc.treeOnRight) + m.cfg.treeOnRight = tc.treeOnRight + m.file.name = "a.go" // pretend a file is loaded + m.layout.focus = paneTree - // l switches to diff pane - result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) - model := result.(Model) - assert.Equal(t, paneDiff, model.layout.focus) + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toDiffKey}}) + model := result.(Model) + assert.Equal(t, paneDiff, model.layout.focus) - // h switches back to tree - result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) - model = result.(Model) - assert.Equal(t, paneTree, model.layout.focus) + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toTreeKey}}) + model = result.(Model) + assert.Equal(t, paneTree, model.layout.focus) + }) + } } func TestModel_WindowResize(t *testing.T) { diff --git a/app/ui/view_test.go b/app/ui/view_test.go index b1336ba3..285c01c8 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -1095,6 +1095,22 @@ func TestModel_HKeySwitchesToTOC(t *testing.T) { assert.Equal(t, paneTree, model.layout.focus, "h key should switch to TOC pane") }) + t.Run("l key in diff pane switches to right-positioned TOC", func(t *testing.T) { + m := testModel([]string{"README.md"}, map[string][]diff.DiffLine{"README.md": mdLines}) + m.file.singleFile = true + m.file.mdTOC = sidepane.ParseTOC(mdLines, "README.md") + require.NotNil(t, m.file.mdTOC) + m.file.name = "README.md" + m.file.lines = mdLines + m.layout.focus = paneDiff + m.cfg.treeOnRight = true + m.keymap = keymap.DefaultForTreePosition(true) + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) + model := result.(Model) + assert.Equal(t, paneTree, model.layout.focus, "l key should switch to the right-positioned TOC pane") + }) + t.Run("h key no-op in single-file without TOC", func(t *testing.T) { m := testModel([]string{"main.go"}, nil) m.file.singleFile = true From 897c9fb7775d1930a028129509b62d3685eb0be8 Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Sat, 18 Jul 2026 21:26:35 -0700 Subject: [PATCH 3/6] refactor(keymap): introduce TreePosition type to replace boolean toggle - add TreePosition type and constants (TreePositionLeft, TreePositionRight) - replace boolean treeOnRight usage in keymap and UI with the new typed enum - update option parsing to resolve string flags to the new TreePosition type - simplify Load/Default signatures in keymap package to use TreePosition - update all tests and callers to reflect the improved type safety --- app/config.go | 10 ++++ app/keymap/keymap.go | 52 ++++++++--------- app/keymap/keymap_test.go | 116 +++++++++++++++++++------------------- app/keymap/layout_test.go | 8 +-- app/main.go | 9 +-- app/ui/annotnav_test.go | 2 +- app/ui/diffnav_test.go | 2 +- app/ui/model.go | 8 +-- app/ui/model_test.go | 58 +++++++++---------- app/ui/mouse.go | 3 +- app/ui/mouse_test.go | 11 ++-- app/ui/view.go | 3 +- app/ui/view_test.go | 22 ++++---- 13 files changed, 154 insertions(+), 150 deletions(-) diff --git a/app/config.go b/app/config.go index cc63b4a9..ebf495fb 100644 --- a/app/config.go +++ b/app/config.go @@ -9,6 +9,8 @@ import ( "strings" "github.com/jessevdk/go-flags" + + "github.com/umputun/revdiff/app/keymap" ) type options struct { @@ -120,6 +122,14 @@ func (o options) startupUntracked() bool { return true } +// treePosition resolves the --tree-position flag value to its typed position. +func (o options) treePosition() keymap.TreePosition { + if o.TreePosition == "right" { + return keymap.TreePositionRight + } + return keymap.TreePositionLeft +} + // parseArgs parses CLI arguments with config file support. // config file is loaded first, then CLI args override. // precedence: CLI flags > env vars > config file > built-in defaults. diff --git a/app/keymap/keymap.go b/app/keymap/keymap.go index 954bf50e..4a5e338a 100644 --- a/app/keymap/keymap.go +++ b/app/keymap/keymap.go @@ -253,9 +253,18 @@ func defaultDescriptions() []HelpEntry { } } +// TreePosition is the side of the screen the file tree (or markdown TOC) +// pane renders on. The zero value is TreePositionLeft, the default layout. +type TreePosition int + +const ( + TreePositionLeft TreePosition = iota + TreePositionRight +) + // defaultBindings returns the default key-to-action mapping. // h/l follow the visual pane order while the actions remain semantic. -func defaultBindings(treeOnRight bool) map[string]Action { +func defaultBindings(pos TreePosition) map[string]Action { bindings := map[string]Action{ "j": ActionDown, "k": ActionUp, @@ -311,23 +320,18 @@ func defaultBindings(treeOnRight bool) map[string]Action { "R": ActionReload, "esc": ActionDismiss, } - if treeOnRight { + if pos == TreePositionRight { bindings["h"] = ActionFocusDiff bindings["l"] = ActionFocusTree } return bindings } -// Default returns a Keymap with the default left-positioned tree bindings. -func Default() *Keymap { - return DefaultForTreePosition(false) -} - -// DefaultForTreePosition returns a Keymap whose h/l pane bindings follow the -// visual pane order for the configured tree position. -func DefaultForTreePosition(treeOnRight bool) *Keymap { +// Default returns a Keymap with all default bindings; h/l pane bindings +// follow the visual pane order for the given tree position. +func Default(pos TreePosition) *Keymap { return &Keymap{ - bindings: defaultBindings(treeOnRight), + bindings: defaultBindings(pos), descriptions: defaultDescriptions(), } } @@ -644,11 +648,7 @@ func parse(r io.Reader) (maps []mapEntry, unmaps []string, err error) { // Load reads a keybindings file from path and returns a Keymap with defaults // overridden by the file contents. Returns error if the file cannot be opened or parsed. -func Load(path string) (*Keymap, error) { - return load(path, false) -} - -func load(path string, treeOnRight bool) (*Keymap, error) { +func Load(path string, pos TreePosition) (*Keymap, error) { f, err := os.Open(path) //nolint:gosec // path is user-provided config file location if err != nil { return nil, fmt.Errorf("opening keybindings file: %w", err) @@ -660,7 +660,7 @@ func load(path string, treeOnRight bool) (*Keymap, error) { return nil, err } - km := DefaultForTreePosition(treeOnRight) + km := Default(pos) // apply unmaps first, then maps (so "unmap q" + "map x quit" works) for _, key := range unmaps { @@ -694,24 +694,18 @@ func (km *Keymap) resolveConflicts() { } // LoadOrDefault loads keybindings from path if the file exists, otherwise returns -// Default(). Parse errors are logged as warnings and Default() is returned. -func LoadOrDefault(path string) *Keymap { - return LoadOrDefaultForTreePosition(path, false) -} - -// LoadOrDefaultForTreePosition loads user overrides on top of defaults whose -// h/l pane bindings follow the configured tree position. -func LoadOrDefaultForTreePosition(path string, treeOnRight bool) *Keymap { +// Default(pos). Parse errors are logged as warnings and defaults are returned. +func LoadOrDefault(path string, pos TreePosition) *Keymap { if path == "" { - return DefaultForTreePosition(treeOnRight) + return Default(pos) } if _, err := os.Stat(path); os.IsNotExist(err) { - return DefaultForTreePosition(treeOnRight) + return Default(pos) } - km, err := load(path, treeOnRight) + km, err := Load(path, pos) if err != nil { log.Printf("[WARN] failed to load keybindings from %s: %v, using defaults", path, err) - return DefaultForTreePosition(treeOnRight) + return Default(pos) } return km } diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index 9ab17fc1..26b0a4ce 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -12,25 +12,25 @@ import ( ) func TestDefault(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) require.NotNil(t, km) assert.NotEmpty(t, km.bindings) assert.NotEmpty(t, km.descriptions) } -func TestDefaultForTreePosition(t *testing.T) { - left := DefaultForTreePosition(false) +func TestDefault_TreePosition(t *testing.T) { + left := Default(TreePositionLeft) assert.Equal(t, ActionFocusTree, left.Resolve("h")) assert.Equal(t, ActionFocusDiff, left.Resolve("l")) - right := DefaultForTreePosition(true) + right := Default(TreePositionRight) assert.Equal(t, ActionFocusDiff, right.Resolve("h")) assert.Equal(t, ActionFocusTree, right.Resolve("l")) assert.Equal(t, ActionDown, right.Resolve("j"), "unrelated defaults must stay unchanged") } func TestDefault_allExpectedBindings(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) tests := []struct { key string action Action @@ -83,7 +83,7 @@ func TestDefault_specialKeysMatchBubbletea(t *testing.T) { {tea.KeyTab, "tab"}, } - km := Default() + km := Default(TreePositionLeft) for _, tt := range tests { msg := tea.KeyMsg{Type: tt.keyType} actual := msg.String() @@ -100,7 +100,7 @@ func TestDefault_ctrlKeysMatchBubbletea(t *testing.T) { ctrlU := tea.KeyMsg{Type: tea.KeyCtrlU} ctrlP := tea.KeyMsg{Type: tea.KeyCtrlP} - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionHalfPageDown, km.Resolve(ctrlD.String())) assert.Equal(t, ActionHalfPageUp, km.Resolve(ctrlU.String())) assert.Equal(t, ActionJumpFile, km.Resolve(ctrlP.String())) @@ -141,7 +141,7 @@ func TestActionJumpFile_CustomConfiguration(t *testing.T) { } func TestResolve(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) t.Run("existing key", func(t *testing.T) { assert.Equal(t, ActionDown, km.Resolve("j")) @@ -158,7 +158,7 @@ func TestResolve(t *testing.T) { } func TestKeysFor(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) t.Run("single key action", func(t *testing.T) { keys := km.KeysFor(ActionSearch) @@ -186,7 +186,7 @@ func TestKeysFor(t *testing.T) { } func TestBind(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x")) // original binding still works @@ -194,7 +194,7 @@ func TestBind(t *testing.T) { } func TestUnbind(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Unbind("q") assert.Equal(t, Action(""), km.Resolve("q")) // other bindings unaffected @@ -202,13 +202,13 @@ func TestUnbind(t *testing.T) { } func TestUnbind_noop(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Unbind("nonexistent") // should not panic assert.Equal(t, ActionDown, km.Resolve("j")) } func TestHelpSections(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) sections := km.HelpSections() require.NotEmpty(t, sections) @@ -236,7 +236,7 @@ func TestHelpSections(t *testing.T) { } func TestHelpSections_unmappedActionOmitted(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) // unbind all keys for quit km.Unbind("q") sections := km.HelpSections() @@ -251,7 +251,7 @@ func TestHelpSections_unmappedActionOmitted(t *testing.T) { } func TestHelpSections_customBindingReflected(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("x", ActionQuit) sections := km.HelpSections() @@ -281,7 +281,7 @@ func TestActionToggleCompact_IsValid(t *testing.T) { } func TestActionToggleCompact_DefaultBinding(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionToggleCompact, km.Resolve("C")) } @@ -304,7 +304,7 @@ func TestActionOpenEditor_IsValid(t *testing.T) { } func TestActionOpenEditor_DefaultBinding(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionOpenEditor, km.Resolve("ctrl+e")) } @@ -327,7 +327,7 @@ func TestActionOpenFileInEditor_IsValid(t *testing.T) { } func TestActionOpenFileInEditor_DefaultBinding(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionOpenFileInEditor, km.Resolve("e")) } @@ -350,7 +350,7 @@ func TestActionFlushOutput_IsValid(t *testing.T) { } func TestActionFlushOutput_DefaultBinding(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionFlushOutput, km.Resolve("O")) } @@ -369,7 +369,7 @@ func TestActionFlushOutput_HelpEntry(t *testing.T) { } func TestActionFlushOutput_DumpEntry(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) var buf strings.Builder require.NoError(t, km.Dump(&buf)) assert.Contains(t, buf.String(), "map O flush_output") @@ -406,7 +406,7 @@ func TestActionScrollConstants_InHelpEntries(t *testing.T) { func TestActionScrollConstants_NoDefaultBindings(t *testing.T) { // vim-motion interceptor is the only way to reach these actions by default; // there must be NO single-key bindings in defaultBindings. - km := Default() + km := Default(TreePositionLeft) for _, a := range []Action{ActionScrollCenter, ActionScrollTop, ActionScrollBottom} { assert.Empty(t, km.KeysFor(a), "action %q must have no default bindings", a) } @@ -451,7 +451,7 @@ func TestParse_acceptsDeprecatedCommitInfoAlias(t *testing.T) { func TestInfo_roundTrip(t *testing.T) { // default binding resolves correctly - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionInfo, km.Resolve("i")) // action appears in help sections @@ -485,7 +485,7 @@ func TestInfo_roundTrip(t *testing.T) { } func TestKeysFor_sorted(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) keys := km.KeysFor(ActionDown) // should be sorted: "down" before "j" assert.Equal(t, []string{"down", "j"}, keys) @@ -714,7 +714,7 @@ func TestLoad_withOverrides(t *testing.T) { err := os.WriteFile(tmpFile, []byte("map x quit\nunmap j\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) // new binding assert.Equal(t, ActionQuit, km.Resolve("q")) // default still works @@ -727,14 +727,14 @@ func TestLoad_unmapThenRemap(t *testing.T) { err := os.WriteFile(tmpFile, []byte("unmap q\nmap x quit\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) assert.Equal(t, Action(""), km.Resolve("q")) // unmapped assert.Equal(t, ActionQuit, km.Resolve("x")) // remapped } func TestLoad_missingFile(t *testing.T) { - _, err := Load("/nonexistent/path/keybindings") + _, err := Load("/nonexistent/path/keybindings", TreePositionLeft) assert.Error(t, err) } @@ -743,20 +743,20 @@ func TestLoad_malformedLines(t *testing.T) { err := os.WriteFile(tmpFile, []byte("garbage line\nmap x quit\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) // valid line still applied } func TestLoadOrDefault_noFile(t *testing.T) { - km := LoadOrDefault("/nonexistent/path/keybindings") + km := LoadOrDefault("/nonexistent/path/keybindings", TreePositionLeft) // should return defaults assert.Equal(t, ActionDown, km.Resolve("j")) assert.Equal(t, ActionQuit, km.Resolve("q")) } func TestLoadOrDefault_emptyPath(t *testing.T) { - km := LoadOrDefault("") + km := LoadOrDefault("", TreePositionLeft) assert.Equal(t, ActionDown, km.Resolve("j")) } @@ -765,17 +765,17 @@ func TestLoadOrDefault_withFile(t *testing.T) { err := os.WriteFile(tmpFile, []byte("map x quit\n"), 0o600) require.NoError(t, err) - km := LoadOrDefault(tmpFile) + km := LoadOrDefault(tmpFile, TreePositionLeft) assert.Equal(t, ActionQuit, km.Resolve("x")) assert.Equal(t, ActionDown, km.Resolve("j")) // defaults still present } -func TestLoadOrDefaultForTreePosition_UserOverridesDefaults(t *testing.T) { +func TestLoadOrDefault_TreePositionUserOverrides(t *testing.T) { tmpFile := t.TempDir() + "/keybindings" err := os.WriteFile(tmpFile, []byte("map h quit\nunmap l\n"), 0o600) require.NoError(t, err) - km := LoadOrDefaultForTreePosition(tmpFile, true) + km := LoadOrDefault(tmpFile, TreePositionRight) assert.Equal(t, ActionQuit, km.Resolve("h")) assert.Empty(t, km.Resolve("l")) } @@ -785,14 +785,14 @@ func TestLoad_unmapOfUnboundKey(t *testing.T) { err := os.WriteFile(tmpFile, []byte("unmap z\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) // should not panic, defaults should be intact assert.Equal(t, ActionDown, km.Resolve("j")) } func TestDump_format(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) var buf strings.Builder require.NoError(t, km.Dump(&buf)) output := buf.String() @@ -816,7 +816,7 @@ func TestDump_format(t *testing.T) { } func TestDump_roundTrip(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) var buf strings.Builder require.NoError(t, km.Dump(&buf)) @@ -843,7 +843,7 @@ func TestDump_roundTrip(t *testing.T) { } func TestDump_customBindings(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Unbind("q") km.Bind("x", ActionQuit) @@ -882,7 +882,7 @@ func TestDump_chordWithSpaceSecondStageRoundTrip(t *testing.T) { } func TestDump_spaceKeyRoundTrip(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind(" ", ActionPageDown) // bind space to an action var buf strings.Builder @@ -905,7 +905,7 @@ func TestDump_spaceKeyRoundTrip(t *testing.T) { } func TestDump_unmappedActionOmitted(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Unbind("/") // search only has one key var buf strings.Builder @@ -917,7 +917,7 @@ func TestDump_unmappedActionOmitted(t *testing.T) { } func TestDump_failingWriter(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) w := &failWriter{errAfter: 0} err := km.Dump(w) require.Error(t, err) @@ -925,7 +925,7 @@ func TestDump_failingWriter(t *testing.T) { } func TestDump_failingWriterAfterSomeOutput(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) w := &failWriter{errAfter: 5} // fail after 5 successful writes err := km.Dump(w) require.Error(t, err) @@ -950,7 +950,7 @@ func (w *failWriter) Write(p []byte) (int, error) { func TestAcceptance_defaultKeymapPreservesAllBindings(t *testing.T) { // no keybindings file β†’ identical behavior to current defaults - km := Default() + km := Default(TreePositionLeft) assert.Equal(t, ActionDown, km.Resolve("j")) assert.Equal(t, ActionUp, km.Resolve("k")) assert.Equal(t, ActionQuit, km.Resolve("q")) @@ -964,7 +964,7 @@ func TestAcceptance_defaultKeymapPreservesAllBindings(t *testing.T) { func TestAcceptance_additiveBinding(t *testing.T) { // map x quit β†’ x quits, q still quits (additive, not replacement) - km := Default() + km := Default(TreePositionLeft) km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x"), "x should quit after binding") assert.Equal(t, ActionQuit, km.Resolve("q"), "q should still quit (additive)") @@ -972,7 +972,7 @@ func TestAcceptance_additiveBinding(t *testing.T) { func TestAcceptance_unmapThenRemap(t *testing.T) { // unmap q + map x quit β†’ only x quits - km := Default() + km := Default(TreePositionLeft) km.Unbind("q") km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x"), "x should quit") @@ -981,7 +981,7 @@ func TestAcceptance_unmapThenRemap(t *testing.T) { func TestAcceptance_dumpKeysShowsEffective(t *testing.T) { // --dump-keys prints all effective bindings in parseable format - km := Default() + km := Default(TreePositionLeft) var buf strings.Builder require.NoError(t, km.Dump(&buf)) output := buf.String() @@ -999,7 +999,7 @@ func TestAcceptance_loadCustomFile(t *testing.T) { require.NoError(t, err) require.NoError(t, tmp.Close()) - km, err := Load(tmp.Name()) + km, err := Load(tmp.Name(), TreePositionLeft) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) assert.Equal(t, Action(""), km.Resolve("q")) @@ -1007,7 +1007,7 @@ func TestAcceptance_loadCustomFile(t *testing.T) { func TestAcceptance_helpReflectsCustomBindings(t *testing.T) { // help overlay reflects custom bindings - km := Default() + km := Default(TreePositionLeft) km.Bind("x", ActionQuit) sections := km.HelpSections() @@ -1035,7 +1035,7 @@ func TestAcceptance_invalidActionWarnsNoCrash(t *testing.T) { } func TestIsChordLeader(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>x", ActionQuit) assert.True(t, km.IsChordLeader("ctrl+w"), "ctrl+w should be a chord leader") @@ -1046,13 +1046,13 @@ func TestIsChordLeader(t *testing.T) { func TestIsChordLeader_standaloneIsNotLeader(t *testing.T) { // standalone ctrl+w without any ctrl+w>* chord β†’ not a leader - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w", ActionQuit) assert.False(t, km.IsChordLeader("ctrl+w"), "standalone-only binding should not be a chord leader") } func TestIsChordLeader_LazyAndInvalidated(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) // no chord bindings yet assert.False(t, km.IsChordLeader("ctrl+w")) @@ -1076,7 +1076,7 @@ func TestLoad_ConflictDropsStandalone(t *testing.T) { content := "map ctrl+w quit\nmap ctrl+w>x help\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) // the chord binding survives @@ -1092,7 +1092,7 @@ func TestLoad_NoConflictKeepsBoth(t *testing.T) { content := "map ctrl+w>x help\nmap ctrl+t quit\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) // chord survives @@ -1112,7 +1112,7 @@ func TestLoad_ConflictInvalidatesChordCache(t *testing.T) { content := "map ctrl+d>x help\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile) + km, err := Load(tmpFile, TreePositionLeft) require.NoError(t, err) // the default ctrl+d standalone binding was dropped by resolveConflicts @@ -1155,7 +1155,7 @@ func TestNormalizeKey_UnmappedRunePassThrough(t *testing.T) { } func TestResolveChord_Direct(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>x", ActionQuit) assert.Equal(t, ActionQuit, km.ResolveChord("ctrl+w", "x")) } @@ -1163,13 +1163,13 @@ func TestResolveChord_Direct(t *testing.T) { func TestResolveChord_LayoutFallback(t *testing.T) { // Ρ‡ (Cyrillic che) sits on the same physical key as x on QWERTY. // chord bound under the latin "x" must still resolve when user presses Ρ‡. - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>x", ActionHelp) assert.Equal(t, ActionHelp, km.ResolveChord("ctrl+w", "Ρ‡")) } func TestResolveChord_Unbound(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>x", ActionQuit) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "q")) assert.Equal(t, Action(""), km.ResolveChord("ctrl+t", "x")) @@ -1177,7 +1177,7 @@ func TestResolveChord_Unbound(t *testing.T) { func TestResolveChord_PrefixOnly(t *testing.T) { // only the leader is bound (no chord under it) β†’ ResolveChord returns empty - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w", ActionQuit) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "x")) } @@ -1185,7 +1185,7 @@ func TestResolveChord_PrefixOnly(t *testing.T) { func TestResolveChord_LayoutFallbackMissingForMultiRuneSecond(t *testing.T) { // layout fallback only applies when second is a single rune; multi-rune // strings like "esc" should not trigger a translation attempt - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>esc", ActionDismiss) assert.Equal(t, ActionDismiss, km.ResolveChord("ctrl+w", "esc")) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "tab")) @@ -1234,7 +1234,7 @@ func TestDump_RoundTripsChords(t *testing.T) { } func TestKeysFor_IncludesChordKeys(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) km.Bind("ctrl+w>x", ActionQuit) keys := km.KeysFor(ActionQuit) diff --git a/app/keymap/layout_test.go b/app/keymap/layout_test.go index bec50884..c189f6cb 100644 --- a/app/keymap/layout_test.go +++ b/app/keymap/layout_test.go @@ -121,7 +121,7 @@ func TestLayoutResolve(t *testing.T) { } func TestKeymap_ResolveLayoutFallback(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) t.Run("russian: Π³ triggers toggle_untracked (u)", func(t *testing.T) { assert.Equal(t, ActionToggleUntracked, km.Resolve("Π³")) @@ -155,7 +155,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("direct binding takes precedence over alias", func(t *testing.T) { - customKm := Default() + customKm := Default(TreePositionLeft) // bind a Cyrillic character directly to a different action customKm.Bind("Π³", ActionQuit) // should use the direct binding, not the layout alias to ActionToggleUntracked @@ -176,7 +176,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("layout alias does not pollute KeysFor", func(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) keys := km.KeysFor(ActionDown) assert.NotContains(t, keys, "ш") assert.NotContains(t, keys, "ΞΎ") @@ -186,7 +186,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("layout alias does not pollute HelpSections", func(t *testing.T) { - km := Default() + km := Default(TreePositionLeft) sections := km.HelpSections() for _, sec := range sections { for _, entry := range sec.Entries { diff --git a/app/main.go b/app/main.go index 0c4e3061..fdc9dc96 100644 --- a/app/main.go +++ b/app/main.go @@ -55,10 +55,7 @@ func main() { } if opts.DumpKeys { - km := keymap.LoadOrDefaultForTreePosition( - resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), - opts.TreePosition == "right", - ) + km := keymap.LoadOrDefault(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), opts.treePosition()) if err := km.Dump(os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) @@ -111,7 +108,7 @@ func run(opts options) (int, error) { store := annotation.NewStore() hl := highlight.New(opts.ChromaStyle, !opts.NoColors) - km := keymap.LoadOrDefaultForTreePosition(resolveKeysPath(opts), opts.TreePosition == "right") + km := keymap.LoadOrDefault(resolveKeysPath(opts), opts.treePosition()) var ( renderer ui.Renderer @@ -234,7 +231,7 @@ func run(opts options) (int, error) { Ref: opts.ref(), Staged: opts.Staged, TreeWidthRatio: opts.TreeWidth, - TreeOnRight: opts.TreePosition == "right", + TreePosition: opts.treePosition(), Only: opts.Only, WorkDir: workDir, SourceEditor: sourceEditorPolicy(opts, workDir), diff --git a/app/ui/annotnav_test.go b/app/ui/annotnav_test.go index f293682c..7d1a546b 100644 --- a/app/ui/annotnav_test.go +++ b/app/ui/annotnav_test.go @@ -795,7 +795,7 @@ func TestModel_HandleAnnotNav_FileLevelDoesNotSetCursorOnAnnotation(t *testing.T } func TestModel_HandleAnnotNav_DefaultBindings(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) assert.Equal(t, keymap.ActionNextAnnotation, km.Resolve("}")) assert.Equal(t, keymap.ActionPrevAnnotation, km.Resolve("{")) } diff --git a/app/ui/diffnav_test.go b/app/ui/diffnav_test.go index 54947f71..e1b5ef54 100644 --- a/app/ui/diffnav_test.go +++ b/app/ui/diffnav_test.go @@ -2212,7 +2212,7 @@ func TestModel_ActiveSectionTrackingOnScroll(t *testing.T) { } func TestModel_CustomKeymapDiffNavNextHunk(t *testing.T) { // map "x" to next_hunk, unbind "]" β€” verify "x" jumps to next hunk and "]" does not - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("x", keymap.ActionNextHunk) km.Unbind("]") diff --git a/app/ui/model.go b/app/ui/model.go index 71a32d42..60f9dc63 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -327,7 +327,7 @@ type modelConfigState struct { noConfirmReload bool // skip confirmation prompt on reload (R) crossFileHunks bool // allow [ and ] to jump across file boundaries treeWidthRatio int // 1-10 units for file tree panel - treeOnRight bool // render the file tree or markdown TOC to the right of the diff + treePosition keymap.TreePosition // side the file tree or markdown TOC renders on tabSpaces string // spaces to replace tabs with wrapIndent int // extra indent (in columns) for wrap continuation rows; 0 disables annotPrefix string // cached: marker + " " @@ -729,7 +729,7 @@ type ModelConfig struct { Ref string Staged bool TreeWidthRatio int - TreeOnRight bool // render the file tree or markdown TOC to the right of the diff + TreePosition keymap.TreePosition // side the file tree or markdown TOC renders on TabWidth int // number of spaces per tab character NoColors bool // disable all colors including syntax highlighting MouseTracking bool // enable mouse tracking for clicks and wheel events @@ -847,7 +847,7 @@ func NewModel(cfg ModelConfig) (Model, error) { } km := cfg.Keymap if km == nil { - km = keymap.DefaultForTreePosition(cfg.TreeOnRight) + km = keymap.Default(cfg.TreePosition) } ed := cfg.Editor if ed == nil || isNilValue(ed) { @@ -889,7 +889,7 @@ func NewModel(cfg ModelConfig) (Model, error) { noConfirmReload: cfg.NoConfirmReload, crossFileHunks: cfg.CrossFileHunks, treeWidthRatio: cfg.TreeWidthRatio, - treeOnRight: cfg.TreeOnRight, + treePosition: cfg.TreePosition, tabSpaces: strings.Repeat(" ", cfg.TabWidth), wrapIndent: max(0, cfg.WrapIndent), annotPrefix: cfg.AnnotationMarker + " ", diff --git a/app/ui/model_test.go b/app/ui/model_test.go index 524b3d22..726deb9f 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -230,7 +230,7 @@ func TestNewModel_OptionalDefaults(t *testing.T) { FileDiffFunc: func(diff.FileDiffRequest) ([]diff.DiffLine, error) { return nil, nil }, } - t.Run("nil keymap defaults to keymap.Default()", func(t *testing.T) { + t.Run("nil keymap defaults to keymap.Default(keymap.TreePositionLeft)", func(t *testing.T) { m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{}) require.NotNil(t, m.keymap) // verify a known default binding works @@ -239,7 +239,7 @@ func TestNewModel_OptionalDefaults(t *testing.T) { }) t.Run("custom keymap is used when provided", func(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Unbind("q") m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{Keymap: km}) action := m.keymap.Resolve("q") @@ -261,9 +261,9 @@ func TestNewModel_OptionalDefaults(t *testing.T) { assert.Equal(t, 5, m.cfg.treeWidthRatio) }) - t.Run("TreeOnRight is kept and default pane keys follow layout", func(t *testing.T) { - m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreeOnRight: true}) - assert.True(t, m.cfg.treeOnRight) + t.Run("TreePosition is kept and default pane keys follow layout", func(t *testing.T) { + m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreePosition: keymap.TreePositionRight}) + assert.Equal(t, keymap.TreePositionRight, m.cfg.treePosition) assert.Equal(t, keymap.ActionFocusDiff, m.keymap.Resolve("h")) assert.Equal(t, keymap.ActionFocusTree, m.keymap.Resolve("l")) }) @@ -509,19 +509,19 @@ func TestModel_TreeNavigation(t *testing.T) { func TestModel_FocusSwitching(t *testing.T) { for _, tc := range []struct { - name string - treeOnRight bool - toDiffKey rune - toTreeKey rune + name string + treePos keymap.TreePosition + toDiffKey rune + toTreeKey rune }{ {name: "tree on left", toDiffKey: 'l', toTreeKey: 'h'}, - {name: "tree on right", treeOnRight: true, toDiffKey: 'h', toTreeKey: 'l'}, + {name: "tree on right", treePos: keymap.TreePositionRight, toDiffKey: 'h', toTreeKey: 'l'}, } { t.Run(tc.name, func(t *testing.T) { m := testModel([]string{"a.go"}, nil) m.tree = testNewFileTree([]string{"a.go"}) - m.keymap = keymap.DefaultForTreePosition(tc.treeOnRight) - m.cfg.treeOnRight = tc.treeOnRight + m.keymap = keymap.Default(tc.treePos) + m.cfg.treePosition = tc.treePos m.file.name = "a.go" // pretend a file is loaded m.layout.focus = paneTree @@ -579,7 +579,7 @@ func TestModel_TreeWidthRatio(t *testing.T) { func TestModel_CustomKeymapQuitOverride(t *testing.T) { // map "x" to quit, unbind "q" β€” verify "x" quits and "q" does not - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("x", keymap.ActionQuit) km.Unbind("q") @@ -600,7 +600,7 @@ func TestModel_CustomKeymapQuitOverride(t *testing.T) { func TestModel_CustomKeymapViewToggle(t *testing.T) { // map "x" to toggle_wrap β€” verify "x" toggles wrap and "w" still works - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("x", keymap.ActionToggleWrap) lines := []diff.DiffLine{{NewNum: 1, Content: "ctx", ChangeType: diff.ChangeContext}} @@ -625,7 +625,7 @@ func TestModel_CustomKeymapViewToggle(t *testing.T) { func TestModel_CustomKeymapTreeNav(t *testing.T) { // map "x" to down, unbind "j" β€” verify "x" moves tree cursor and "j" does not - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("x", keymap.ActionDown) km.Unbind("j") @@ -664,7 +664,7 @@ func TestModel_CustomKeymapTreeFocusDiff(t *testing.T) { func TestModel_AcceptanceAdditiveQuitBinding(t *testing.T) { // map x quit (additive) β€” both x and q should quit - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) @@ -688,7 +688,7 @@ func TestModel_AcceptanceAdditiveQuitBinding(t *testing.T) { func TestModel_AcceptanceDefaultBehaviorNoKeybindingsFile(t *testing.T) { // no keybindings file β†’ identical behavior to current defaults m := testModel([]string{"a.go"}, nil) - // m.keymap is set to Default() in testModel via NewModel + // m.keymap is set to Default(false) in testModel via NewModel // q should quit _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) @@ -949,7 +949,7 @@ func TestDispatchAction_PaneNavFallback_Tree(t *testing.T) { } func TestHandleChordSecond_ResolvedDispatches(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -967,7 +967,7 @@ func TestHandleChordSecond_ResolvedDispatches(t *testing.T) { } func TestHandleChordSecond_UnboundShowsHint(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -983,7 +983,7 @@ func TestHandleChordSecond_UnboundShowsHint(t *testing.T) { } func TestHandleChordSecond_EscCancels(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -999,7 +999,7 @@ func TestHandleChordSecond_EscCancels(t *testing.T) { } func TestHandleChordSecond_LayoutFallback(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1029,7 +1029,7 @@ func TestHandleChordSecond_DispatchesToTOCWhenFocused(t *testing.T) { {NewNum: 2, Content: "text", ChangeType: diff.ChangeContext}, {NewNum: 3, Content: "## Second", ChangeType: diff.ChangeContext}, } - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionDown) m := testModel([]string{"README.md"}, map[string][]diff.DiffLine{"README.md": mdLines}) @@ -1118,7 +1118,7 @@ func TestTransientHint_OutputPriority(t *testing.T) { } func TestHandleKey_EntersChordPending(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1132,7 +1132,7 @@ func TestHandleKey_EntersChordPending(t *testing.T) { } func TestHandleKey_ChordSecondCoexistenceGuard(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1155,7 +1155,7 @@ func TestHandleKey_ChordSecondCoexistenceGuard(t *testing.T) { } func TestHandleKey_ChordIgnoredWhenPendingReload(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1175,7 +1175,7 @@ func TestHandleKey_ChordIgnoredWhenPendingReload(t *testing.T) { } func TestHandleKey_LeaderWithStandaloneActionDoesNotEnterChord(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) // bind ctrl+w as a standalone action (no chord binding for ctrl+w>*) km.Bind("ctrl+w", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) @@ -1560,7 +1560,7 @@ func TestHandleKey_ChordPrecedence(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1599,7 +1599,7 @@ func TestHandleKey_VimMotionOn_DigitAccumulates(t *testing.T) { } func TestHandleKey_VimMotionOn_ChordSecondWins(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1709,7 +1709,7 @@ func TestHandleKey_NonKeyMessagesPreserveChordState(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - km := keymap.Default() + km := keymap.Default(keymap.TreePositionLeft) km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km diff --git a/app/ui/mouse.go b/app/ui/mouse.go index f3fba0cf..7945154f 100644 --- a/app/ui/mouse.go +++ b/app/ui/mouse.go @@ -5,6 +5,7 @@ import ( tea "github.com/charmbracelet/bubbletea" + "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/ui/overlay" "github.com/umputun/revdiff/app/ui/sidepane" ) @@ -113,7 +114,7 @@ func (m Model) hitTest(x, y int) hitZone { if !m.treePaneHidden() { treeStart := 0 - if m.cfg.treeOnRight { + if m.cfg.treePosition == keymap.TreePositionRight { // pane width includes the content plus its left and right borders. treeStart = m.layout.width - m.layout.treeWidth - 2 } diff --git a/app/ui/mouse_test.go b/app/ui/mouse_test.go index 4a7d246d..1e4bbe07 100644 --- a/app/ui/mouse_test.go +++ b/app/ui/mouse_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/umputun/revdiff/app/annotation" + "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/diff" "github.com/umputun/revdiff/app/ui/overlay" "github.com/umputun/revdiff/app/ui/sidepane" @@ -96,28 +97,28 @@ func TestModel_hitTest(t *testing.T) { { name: "right tree: last diff column", setup: func(m *Model) { - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight }, x: 81, y: 10, want: hitDiff, }, { name: "right tree: first tree column", setup: func(m *Model) { - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight }, x: 82, y: 10, want: hitTree, }, { name: "right tree: tree top border", setup: func(m *Model) { - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight }, x: 100, y: 0, want: hitNone, }, { name: "right tree: diff header on left", setup: func(m *Model) { - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight }, x: 5, y: 1, want: hitHeader, }, @@ -462,7 +463,7 @@ func TestModel_HandleMouse_WheelInTreeMovesTreeCursor(t *testing.T) { t.Run("right-positioned tree receives wheel events on the right", func(t *testing.T) { m := mouseTestModel(t, files, diffs) - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight require.Equal(t, "aa.go", m.tree.SelectedFile()) result, _ := m.Update(wheelMsg(tea.MouseButtonWheelDown, 100, 3, false)) diff --git a/app/ui/view.go b/app/ui/view.go index ae88ab5d..c726c18f 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" @@ -111,7 +112,7 @@ func (m Model) renderTwoPaneLayout(navigationContent, diffContent string, naviga Render(diffContent) diffPane = m.applyScrollbar(diffPane) - if m.cfg.treeOnRight { + if m.cfg.treePosition == keymap.TreePositionRight { return lipgloss.JoinHorizontal(lipgloss.Top, diffPane, navigationPane) } return lipgloss.JoinHorizontal(lipgloss.Top, navigationPane, diffPane) diff --git a/app/ui/view_test.go b/app/ui/view_test.go index 285c01c8..d71ad5ed 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -608,7 +608,7 @@ func TestModel_ViewSingleFileMode(t *testing.T) { } func TestModel_ViewTreePosition(t *testing.T) { - assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, navigationOnRight bool) { + assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, treePos keymap.TreePosition) { t.Helper() labelColumn := func(lines []string, label string) int { for _, line := range lines { @@ -624,7 +624,7 @@ func TestModel_ViewTreePosition(t *testing.T) { navigationIdx := labelColumn(lines, navigationLabel) require.NotEqual(t, -1, diffIdx, "diff label must appear in the rendered view") require.NotEqual(t, -1, navigationIdx, "navigation label must appear in the rendered view") - if navigationOnRight { + if treePos == keymap.TreePositionRight { assert.Less(t, diffIdx, navigationIdx) return } @@ -632,20 +632,20 @@ func TestModel_ViewTreePosition(t *testing.T) { } for _, tc := range []struct { - name string - navigationOnRight bool + name string + treePos keymap.TreePosition }{ {name: "file tree on left"}, - {name: "file tree on right", navigationOnRight: true}, + {name: "file tree on right", treePos: keymap.TreePositionRight}, } { t.Run(tc.name, func(t *testing.T) { m := testModel([]string{"tree.go", "other.go"}, nil) m.tree = testNewFileTree([]string{"tree.go", "other.go"}) m.file.name = "current.go" - m.cfg.treeOnRight = tc.navigationOnRight + m.cfg.treePosition = tc.treePos m.cfg.noStatusBar = true - assertOrder(t, m.View(), "current.go", "tree.go", tc.navigationOnRight) + assertOrder(t, m.View(), "current.go", "tree.go", tc.treePos) }) } @@ -659,10 +659,10 @@ func TestModel_ViewTreePosition(t *testing.T) { ) require.NotNil(t, m.file.mdTOC) m.file.name = "plan.md" - m.cfg.treeOnRight = true + m.cfg.treePosition = keymap.TreePositionRight m.cfg.noStatusBar = true - assertOrder(t, m.View(), "plan.md", "Navigation section", true) + assertOrder(t, m.View(), "plan.md", "Navigation section", keymap.TreePositionRight) }) } @@ -1103,8 +1103,8 @@ func TestModel_HKeySwitchesToTOC(t *testing.T) { m.file.name = "README.md" m.file.lines = mdLines m.layout.focus = paneDiff - m.cfg.treeOnRight = true - m.keymap = keymap.DefaultForTreePosition(true) + m.cfg.treePosition = keymap.TreePositionRight + m.keymap = keymap.Default(keymap.TreePositionRight) result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) model := result.(Model) From 10f22cc974abe1982640dc6e6e6f3bfb731cc09b Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Sat, 18 Jul 2026 21:45:06 -0700 Subject: [PATCH 4/6] refactor(ui): remove support for configurable tree position - remove `tree-position` option from configuration and CLI - simplify `keymap` API to default to left-side pane navigation - update documentation to remove references to tree-position settings - refactor `ModelConfig` to remove unused tree position field BREAKING CHANGE: The `tree-position` configuration option and CLI flag have been removed; the file tree and markdown TOC will now always be displayed on the left. --- .../skills/revdiff/references/config.md | 2 +- .../skills/revdiff/references/usage.md | 2 +- README.md | 10 +- app/keymap/keymap.go | 43 +++++-- app/keymap/keymap_test.go | 118 +++++++++--------- app/keymap/layout_test.go | 8 +- app/main.go | 4 +- app/ui/annotnav_test.go | 2 +- app/ui/diffnav_test.go | 2 +- app/ui/model.go | 8 +- app/ui/model_test.go | 97 +++++++------- app/ui/view_test.go | 2 +- docs/ARCHITECTURE.md | 2 +- .../codex/skills/revdiff/references/config.md | 2 +- .../codex/skills/revdiff/references/usage.md | 2 +- plugins/pi/skills/revdiff/SKILL.md | 1 - site/docs.html | 3 +- 17 files changed, 158 insertions(+), 150 deletions(-) diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index 3fcc696f..c4fc139a 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -61,7 +61,7 @@ Then uncomment and edit the values you want to change. | `--config` | `REVDIFF_CONFIG` | Path to config file | `~/.config/revdiff/config` | | `--dump-config` | | Print default config to stdout and exit | | -Config-backed options use long flag names without leading `--`; for example, use `tree-position = right` to put the file tree and markdown TOC on the right. For annotation exit status use `exit-code-on-annotations = true`. +Config-backed options use long flag names without leading `--`; for annotation exit status use `exit-code-on-annotations = true`. ## Popup Size (Claude Code plugin) diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 12bc8f48..495e8968 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -35,7 +35,7 @@ When a diff contains exactly one file, revdiff automatically hides the file tree ## Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Set `tree-position = right` in config, pass `--tree-position=right`, or set `REVDIFF_TREE_POSITION=right` to move both the file tree and TOC to the right. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. ## All-Files Mode diff --git a/README.md b/README.md index 2c6929fc..96f121ce 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ Built for a specific use case: reviewing code changes, plans, and documents with - Blame gutter: shows author name and commit age per line, toggle with `B` - Annotate any line in the diff (added, removed, or context) plus file-level notes - Single-file auto-detection: when a diff contains exactly one file, hides the tree pane and gives full terminal width to the diff view -- Two-pane TUI: file tree and colorized diff viewport, with configurable tree position (left by default) +- Two-pane TUI: file tree + colorized diff viewport - Vim-style `/` search within diff with `n`/`N` match navigation - Hunk navigation to jump between change groups - Annotation list popup (`@`): browse all annotations across files, jump to any annotation @@ -413,11 +413,7 @@ mkdir -p ~/.config/revdiff revdiff --dump-config > ~/.config/revdiff/config ``` -Then uncomment and edit the values you want to change. For example, place the file tree and markdown TOC on the right: - -```ini -tree-position = right -``` +Then uncomment and edit the values you want to change. ### Themes @@ -670,7 +666,7 @@ revdiff HEAD~3 --description-file=.review-description.md ### Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md` or `printf '# title\n' | revdiff --stdin --stdin-name plan.md`), revdiff shows a table-of-contents pane on the configured tree side (left by default) listing all markdown headers. Use `Tab` to switch focus between the TOC and diff panes, `j`/`k` to navigate headers, and `Enter` to jump to a header in the diff. The TOC automatically highlights the current section as you scroll through the file. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md` or `printf '# title\n' | revdiff --stdin --stdin-name plan.md`), revdiff shows a table-of-contents pane on the configured side listing all markdown headers. Use `Tab` to switch focus between the TOC and diff panes, `j`/`k` to navigate headers, and `Enter` to jump to a header in the diff. The TOC automatically highlights the current section as you scroll through the file. This mode activates when all three conditions are met: single file, markdown extension (`.md`/`.markdown`), and all lines are context (no diff changes). Headers inside fenced code blocks are excluded from the TOC. diff --git a/app/keymap/keymap.go b/app/keymap/keymap.go index 4a5e338a..05a662b7 100644 --- a/app/keymap/keymap.go +++ b/app/keymap/keymap.go @@ -327,9 +327,14 @@ func defaultBindings(pos TreePosition) map[string]Action { return bindings } -// Default returns a Keymap with all default bindings; h/l pane bindings -// follow the visual pane order for the given tree position. -func Default(pos TreePosition) *Keymap { +// Default returns a Keymap with all default bindings. +func Default() *Keymap { + return DefaultForTreePosition(TreePositionLeft) +} + +// DefaultForTreePosition returns the default keymap with h/l following the +// visual pane order for the given tree position. +func DefaultForTreePosition(pos TreePosition) *Keymap { return &Keymap{ bindings: defaultBindings(pos), descriptions: defaultDescriptions(), @@ -648,7 +653,11 @@ func parse(r io.Reader) (maps []mapEntry, unmaps []string, err error) { // Load reads a keybindings file from path and returns a Keymap with defaults // overridden by the file contents. Returns error if the file cannot be opened or parsed. -func Load(path string, pos TreePosition) (*Keymap, error) { +func Load(path string) (*Keymap, error) { + return load(path, TreePositionLeft) +} + +func load(path string, pos TreePosition) (*Keymap, error) { f, err := os.Open(path) //nolint:gosec // path is user-provided config file location if err != nil { return nil, fmt.Errorf("opening keybindings file: %w", err) @@ -660,7 +669,7 @@ func Load(path string, pos TreePosition) (*Keymap, error) { return nil, err } - km := Default(pos) + km := DefaultForTreePosition(pos) // apply unmaps first, then maps (so "unmap q" + "map x quit" works) for _, key := range unmaps { @@ -693,19 +702,29 @@ func (km *Keymap) resolveConflicts() { km.chordPrefixCache = nil } -// LoadOrDefault loads keybindings from path if the file exists, otherwise returns -// Default(pos). Parse errors are logged as warnings and defaults are returned. -func LoadOrDefault(path string, pos TreePosition) *Keymap { +// LoadOrDefault loads keybindings from path if it exists, otherwise returns +// Default. Parse errors are logged as warnings and defaults are returned. +func LoadOrDefault(path string) *Keymap { + return loadOrDefault(path, TreePositionLeft) +} + +// LoadOrDefaultForTreePosition loads keybindings over defaults whose h/l +// bindings follow the visual pane order for the given tree position. +func LoadOrDefaultForTreePosition(path string, pos TreePosition) *Keymap { + return loadOrDefault(path, pos) +} + +func loadOrDefault(path string, pos TreePosition) *Keymap { if path == "" { - return Default(pos) + return DefaultForTreePosition(pos) } if _, err := os.Stat(path); os.IsNotExist(err) { - return Default(pos) + return DefaultForTreePosition(pos) } - km, err := Load(path, pos) + km, err := load(path, pos) if err != nil { log.Printf("[WARN] failed to load keybindings from %s: %v, using defaults", path, err) - return Default(pos) + return DefaultForTreePosition(pos) } return km } diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index 26b0a4ce..f49e92f5 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -12,25 +12,21 @@ import ( ) func TestDefault(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() require.NotNil(t, km) assert.NotEmpty(t, km.bindings) assert.NotEmpty(t, km.descriptions) } -func TestDefault_TreePosition(t *testing.T) { - left := Default(TreePositionLeft) - assert.Equal(t, ActionFocusTree, left.Resolve("h")) - assert.Equal(t, ActionFocusDiff, left.Resolve("l")) - - right := Default(TreePositionRight) +func TestDefaultForTreePosition(t *testing.T) { + right := DefaultForTreePosition(TreePositionRight) assert.Equal(t, ActionFocusDiff, right.Resolve("h")) assert.Equal(t, ActionFocusTree, right.Resolve("l")) assert.Equal(t, ActionDown, right.Resolve("j"), "unrelated defaults must stay unchanged") } func TestDefault_allExpectedBindings(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() tests := []struct { key string action Action @@ -83,7 +79,7 @@ func TestDefault_specialKeysMatchBubbletea(t *testing.T) { {tea.KeyTab, "tab"}, } - km := Default(TreePositionLeft) + km := Default() for _, tt := range tests { msg := tea.KeyMsg{Type: tt.keyType} actual := msg.String() @@ -100,7 +96,7 @@ func TestDefault_ctrlKeysMatchBubbletea(t *testing.T) { ctrlU := tea.KeyMsg{Type: tea.KeyCtrlU} ctrlP := tea.KeyMsg{Type: tea.KeyCtrlP} - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionHalfPageDown, km.Resolve(ctrlD.String())) assert.Equal(t, ActionHalfPageUp, km.Resolve(ctrlU.String())) assert.Equal(t, ActionJumpFile, km.Resolve(ctrlP.String())) @@ -141,7 +137,7 @@ func TestActionJumpFile_CustomConfiguration(t *testing.T) { } func TestResolve(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() t.Run("existing key", func(t *testing.T) { assert.Equal(t, ActionDown, km.Resolve("j")) @@ -158,7 +154,7 @@ func TestResolve(t *testing.T) { } func TestKeysFor(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() t.Run("single key action", func(t *testing.T) { keys := km.KeysFor(ActionSearch) @@ -186,7 +182,7 @@ func TestKeysFor(t *testing.T) { } func TestBind(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x")) // original binding still works @@ -194,7 +190,7 @@ func TestBind(t *testing.T) { } func TestUnbind(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Unbind("q") assert.Equal(t, Action(""), km.Resolve("q")) // other bindings unaffected @@ -202,13 +198,13 @@ func TestUnbind(t *testing.T) { } func TestUnbind_noop(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Unbind("nonexistent") // should not panic assert.Equal(t, ActionDown, km.Resolve("j")) } func TestHelpSections(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() sections := km.HelpSections() require.NotEmpty(t, sections) @@ -236,7 +232,7 @@ func TestHelpSections(t *testing.T) { } func TestHelpSections_unmappedActionOmitted(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() // unbind all keys for quit km.Unbind("q") sections := km.HelpSections() @@ -251,7 +247,7 @@ func TestHelpSections_unmappedActionOmitted(t *testing.T) { } func TestHelpSections_customBindingReflected(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("x", ActionQuit) sections := km.HelpSections() @@ -281,7 +277,7 @@ func TestActionToggleCompact_IsValid(t *testing.T) { } func TestActionToggleCompact_DefaultBinding(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionToggleCompact, km.Resolve("C")) } @@ -304,7 +300,7 @@ func TestActionOpenEditor_IsValid(t *testing.T) { } func TestActionOpenEditor_DefaultBinding(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionOpenEditor, km.Resolve("ctrl+e")) } @@ -327,7 +323,7 @@ func TestActionOpenFileInEditor_IsValid(t *testing.T) { } func TestActionOpenFileInEditor_DefaultBinding(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionOpenFileInEditor, km.Resolve("e")) } @@ -350,7 +346,7 @@ func TestActionFlushOutput_IsValid(t *testing.T) { } func TestActionFlushOutput_DefaultBinding(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionFlushOutput, km.Resolve("O")) } @@ -369,7 +365,7 @@ func TestActionFlushOutput_HelpEntry(t *testing.T) { } func TestActionFlushOutput_DumpEntry(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() var buf strings.Builder require.NoError(t, km.Dump(&buf)) assert.Contains(t, buf.String(), "map O flush_output") @@ -406,7 +402,7 @@ func TestActionScrollConstants_InHelpEntries(t *testing.T) { func TestActionScrollConstants_NoDefaultBindings(t *testing.T) { // vim-motion interceptor is the only way to reach these actions by default; // there must be NO single-key bindings in defaultBindings. - km := Default(TreePositionLeft) + km := Default() for _, a := range []Action{ActionScrollCenter, ActionScrollTop, ActionScrollBottom} { assert.Empty(t, km.KeysFor(a), "action %q must have no default bindings", a) } @@ -451,7 +447,7 @@ func TestParse_acceptsDeprecatedCommitInfoAlias(t *testing.T) { func TestInfo_roundTrip(t *testing.T) { // default binding resolves correctly - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionInfo, km.Resolve("i")) // action appears in help sections @@ -485,7 +481,7 @@ func TestInfo_roundTrip(t *testing.T) { } func TestKeysFor_sorted(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() keys := km.KeysFor(ActionDown) // should be sorted: "down" before "j" assert.Equal(t, []string{"down", "j"}, keys) @@ -714,7 +710,7 @@ func TestLoad_withOverrides(t *testing.T) { err := os.WriteFile(tmpFile, []byte("map x quit\nunmap j\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) // new binding assert.Equal(t, ActionQuit, km.Resolve("q")) // default still works @@ -727,14 +723,14 @@ func TestLoad_unmapThenRemap(t *testing.T) { err := os.WriteFile(tmpFile, []byte("unmap q\nmap x quit\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) assert.Equal(t, Action(""), km.Resolve("q")) // unmapped assert.Equal(t, ActionQuit, km.Resolve("x")) // remapped } func TestLoad_missingFile(t *testing.T) { - _, err := Load("/nonexistent/path/keybindings", TreePositionLeft) + _, err := Load("/nonexistent/path/keybindings") assert.Error(t, err) } @@ -743,20 +739,20 @@ func TestLoad_malformedLines(t *testing.T) { err := os.WriteFile(tmpFile, []byte("garbage line\nmap x quit\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) // valid line still applied } func TestLoadOrDefault_noFile(t *testing.T) { - km := LoadOrDefault("/nonexistent/path/keybindings", TreePositionLeft) + km := LoadOrDefault("/nonexistent/path/keybindings") // should return defaults assert.Equal(t, ActionDown, km.Resolve("j")) assert.Equal(t, ActionQuit, km.Resolve("q")) } func TestLoadOrDefault_emptyPath(t *testing.T) { - km := LoadOrDefault("", TreePositionLeft) + km := LoadOrDefault("") assert.Equal(t, ActionDown, km.Resolve("j")) } @@ -765,17 +761,17 @@ func TestLoadOrDefault_withFile(t *testing.T) { err := os.WriteFile(tmpFile, []byte("map x quit\n"), 0o600) require.NoError(t, err) - km := LoadOrDefault(tmpFile, TreePositionLeft) + km := LoadOrDefault(tmpFile) assert.Equal(t, ActionQuit, km.Resolve("x")) assert.Equal(t, ActionDown, km.Resolve("j")) // defaults still present } -func TestLoadOrDefault_TreePositionUserOverrides(t *testing.T) { +func TestLoadOrDefaultForTreePosition_UserOverrides(t *testing.T) { tmpFile := t.TempDir() + "/keybindings" err := os.WriteFile(tmpFile, []byte("map h quit\nunmap l\n"), 0o600) require.NoError(t, err) - km := LoadOrDefault(tmpFile, TreePositionRight) + km := LoadOrDefaultForTreePosition(tmpFile, TreePositionRight) assert.Equal(t, ActionQuit, km.Resolve("h")) assert.Empty(t, km.Resolve("l")) } @@ -785,14 +781,14 @@ func TestLoad_unmapOfUnboundKey(t *testing.T) { err := os.WriteFile(tmpFile, []byte("unmap z\n"), 0o600) require.NoError(t, err) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) // should not panic, defaults should be intact assert.Equal(t, ActionDown, km.Resolve("j")) } func TestDump_format(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() var buf strings.Builder require.NoError(t, km.Dump(&buf)) output := buf.String() @@ -816,7 +812,7 @@ func TestDump_format(t *testing.T) { } func TestDump_roundTrip(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() var buf strings.Builder require.NoError(t, km.Dump(&buf)) @@ -843,7 +839,7 @@ func TestDump_roundTrip(t *testing.T) { } func TestDump_customBindings(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Unbind("q") km.Bind("x", ActionQuit) @@ -882,7 +878,7 @@ func TestDump_chordWithSpaceSecondStageRoundTrip(t *testing.T) { } func TestDump_spaceKeyRoundTrip(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind(" ", ActionPageDown) // bind space to an action var buf strings.Builder @@ -905,7 +901,7 @@ func TestDump_spaceKeyRoundTrip(t *testing.T) { } func TestDump_unmappedActionOmitted(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Unbind("/") // search only has one key var buf strings.Builder @@ -917,7 +913,7 @@ func TestDump_unmappedActionOmitted(t *testing.T) { } func TestDump_failingWriter(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() w := &failWriter{errAfter: 0} err := km.Dump(w) require.Error(t, err) @@ -925,7 +921,7 @@ func TestDump_failingWriter(t *testing.T) { } func TestDump_failingWriterAfterSomeOutput(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() w := &failWriter{errAfter: 5} // fail after 5 successful writes err := km.Dump(w) require.Error(t, err) @@ -950,7 +946,7 @@ func (w *failWriter) Write(p []byte) (int, error) { func TestAcceptance_defaultKeymapPreservesAllBindings(t *testing.T) { // no keybindings file β†’ identical behavior to current defaults - km := Default(TreePositionLeft) + km := Default() assert.Equal(t, ActionDown, km.Resolve("j")) assert.Equal(t, ActionUp, km.Resolve("k")) assert.Equal(t, ActionQuit, km.Resolve("q")) @@ -964,7 +960,7 @@ func TestAcceptance_defaultKeymapPreservesAllBindings(t *testing.T) { func TestAcceptance_additiveBinding(t *testing.T) { // map x quit β†’ x quits, q still quits (additive, not replacement) - km := Default(TreePositionLeft) + km := Default() km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x"), "x should quit after binding") assert.Equal(t, ActionQuit, km.Resolve("q"), "q should still quit (additive)") @@ -972,7 +968,7 @@ func TestAcceptance_additiveBinding(t *testing.T) { func TestAcceptance_unmapThenRemap(t *testing.T) { // unmap q + map x quit β†’ only x quits - km := Default(TreePositionLeft) + km := Default() km.Unbind("q") km.Bind("x", ActionQuit) assert.Equal(t, ActionQuit, km.Resolve("x"), "x should quit") @@ -981,7 +977,7 @@ func TestAcceptance_unmapThenRemap(t *testing.T) { func TestAcceptance_dumpKeysShowsEffective(t *testing.T) { // --dump-keys prints all effective bindings in parseable format - km := Default(TreePositionLeft) + km := Default() var buf strings.Builder require.NoError(t, km.Dump(&buf)) output := buf.String() @@ -999,7 +995,7 @@ func TestAcceptance_loadCustomFile(t *testing.T) { require.NoError(t, err) require.NoError(t, tmp.Close()) - km, err := Load(tmp.Name(), TreePositionLeft) + km, err := Load(tmp.Name()) require.NoError(t, err) assert.Equal(t, ActionQuit, km.Resolve("x")) assert.Equal(t, Action(""), km.Resolve("q")) @@ -1007,7 +1003,7 @@ func TestAcceptance_loadCustomFile(t *testing.T) { func TestAcceptance_helpReflectsCustomBindings(t *testing.T) { // help overlay reflects custom bindings - km := Default(TreePositionLeft) + km := Default() km.Bind("x", ActionQuit) sections := km.HelpSections() @@ -1035,7 +1031,7 @@ func TestAcceptance_invalidActionWarnsNoCrash(t *testing.T) { } func TestIsChordLeader(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>x", ActionQuit) assert.True(t, km.IsChordLeader("ctrl+w"), "ctrl+w should be a chord leader") @@ -1046,13 +1042,13 @@ func TestIsChordLeader(t *testing.T) { func TestIsChordLeader_standaloneIsNotLeader(t *testing.T) { // standalone ctrl+w without any ctrl+w>* chord β†’ not a leader - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w", ActionQuit) assert.False(t, km.IsChordLeader("ctrl+w"), "standalone-only binding should not be a chord leader") } func TestIsChordLeader_LazyAndInvalidated(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() // no chord bindings yet assert.False(t, km.IsChordLeader("ctrl+w")) @@ -1076,7 +1072,7 @@ func TestLoad_ConflictDropsStandalone(t *testing.T) { content := "map ctrl+w quit\nmap ctrl+w>x help\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) // the chord binding survives @@ -1092,7 +1088,7 @@ func TestLoad_NoConflictKeepsBoth(t *testing.T) { content := "map ctrl+w>x help\nmap ctrl+t quit\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) // chord survives @@ -1112,7 +1108,7 @@ func TestLoad_ConflictInvalidatesChordCache(t *testing.T) { content := "map ctrl+d>x help\n" require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0o600)) - km, err := Load(tmpFile, TreePositionLeft) + km, err := Load(tmpFile) require.NoError(t, err) // the default ctrl+d standalone binding was dropped by resolveConflicts @@ -1155,7 +1151,7 @@ func TestNormalizeKey_UnmappedRunePassThrough(t *testing.T) { } func TestResolveChord_Direct(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>x", ActionQuit) assert.Equal(t, ActionQuit, km.ResolveChord("ctrl+w", "x")) } @@ -1163,13 +1159,13 @@ func TestResolveChord_Direct(t *testing.T) { func TestResolveChord_LayoutFallback(t *testing.T) { // Ρ‡ (Cyrillic che) sits on the same physical key as x on QWERTY. // chord bound under the latin "x" must still resolve when user presses Ρ‡. - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>x", ActionHelp) assert.Equal(t, ActionHelp, km.ResolveChord("ctrl+w", "Ρ‡")) } func TestResolveChord_Unbound(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>x", ActionQuit) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "q")) assert.Equal(t, Action(""), km.ResolveChord("ctrl+t", "x")) @@ -1177,7 +1173,7 @@ func TestResolveChord_Unbound(t *testing.T) { func TestResolveChord_PrefixOnly(t *testing.T) { // only the leader is bound (no chord under it) β†’ ResolveChord returns empty - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w", ActionQuit) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "x")) } @@ -1185,7 +1181,7 @@ func TestResolveChord_PrefixOnly(t *testing.T) { func TestResolveChord_LayoutFallbackMissingForMultiRuneSecond(t *testing.T) { // layout fallback only applies when second is a single rune; multi-rune // strings like "esc" should not trigger a translation attempt - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>esc", ActionDismiss) assert.Equal(t, ActionDismiss, km.ResolveChord("ctrl+w", "esc")) assert.Equal(t, Action(""), km.ResolveChord("ctrl+w", "tab")) @@ -1234,7 +1230,7 @@ func TestDump_RoundTripsChords(t *testing.T) { } func TestKeysFor_IncludesChordKeys(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() km.Bind("ctrl+w>x", ActionQuit) keys := km.KeysFor(ActionQuit) diff --git a/app/keymap/layout_test.go b/app/keymap/layout_test.go index c189f6cb..bec50884 100644 --- a/app/keymap/layout_test.go +++ b/app/keymap/layout_test.go @@ -121,7 +121,7 @@ func TestLayoutResolve(t *testing.T) { } func TestKeymap_ResolveLayoutFallback(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() t.Run("russian: Π³ triggers toggle_untracked (u)", func(t *testing.T) { assert.Equal(t, ActionToggleUntracked, km.Resolve("Π³")) @@ -155,7 +155,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("direct binding takes precedence over alias", func(t *testing.T) { - customKm := Default(TreePositionLeft) + customKm := Default() // bind a Cyrillic character directly to a different action customKm.Bind("Π³", ActionQuit) // should use the direct binding, not the layout alias to ActionToggleUntracked @@ -176,7 +176,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("layout alias does not pollute KeysFor", func(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() keys := km.KeysFor(ActionDown) assert.NotContains(t, keys, "ш") assert.NotContains(t, keys, "ΞΎ") @@ -186,7 +186,7 @@ func TestKeymap_ResolveLayoutFallback(t *testing.T) { }) t.Run("layout alias does not pollute HelpSections", func(t *testing.T) { - km := Default(TreePositionLeft) + km := Default() sections := km.HelpSections() for _, sec := range sections { for _, entry := range sec.Entries { diff --git a/app/main.go b/app/main.go index fdc9dc96..e6f10dd3 100644 --- a/app/main.go +++ b/app/main.go @@ -55,7 +55,7 @@ func main() { } if opts.DumpKeys { - km := keymap.LoadOrDefault(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), opts.treePosition()) + km := keymap.LoadOrDefaultForTreePosition(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), opts.treePosition()) if err := km.Dump(os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) @@ -108,7 +108,7 @@ func run(opts options) (int, error) { store := annotation.NewStore() hl := highlight.New(opts.ChromaStyle, !opts.NoColors) - km := keymap.LoadOrDefault(resolveKeysPath(opts), opts.treePosition()) + km := keymap.LoadOrDefaultForTreePosition(resolveKeysPath(opts), opts.treePosition()) var ( renderer ui.Renderer diff --git a/app/ui/annotnav_test.go b/app/ui/annotnav_test.go index 7d1a546b..f293682c 100644 --- a/app/ui/annotnav_test.go +++ b/app/ui/annotnav_test.go @@ -795,7 +795,7 @@ func TestModel_HandleAnnotNav_FileLevelDoesNotSetCursorOnAnnotation(t *testing.T } func TestModel_HandleAnnotNav_DefaultBindings(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() assert.Equal(t, keymap.ActionNextAnnotation, km.Resolve("}")) assert.Equal(t, keymap.ActionPrevAnnotation, km.Resolve("{")) } diff --git a/app/ui/diffnav_test.go b/app/ui/diffnav_test.go index e1b5ef54..54947f71 100644 --- a/app/ui/diffnav_test.go +++ b/app/ui/diffnav_test.go @@ -2212,7 +2212,7 @@ func TestModel_ActiveSectionTrackingOnScroll(t *testing.T) { } func TestModel_CustomKeymapDiffNavNextHunk(t *testing.T) { // map "x" to next_hunk, unbind "]" β€” verify "x" jumps to next hunk and "]" does not - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("x", keymap.ActionNextHunk) km.Unbind("]") diff --git a/app/ui/model.go b/app/ui/model.go index 60f9dc63..703a82f5 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -327,12 +327,13 @@ type modelConfigState struct { noConfirmReload bool // skip confirmation prompt on reload (R) crossFileHunks bool // allow [ and ] to jump across file boundaries treeWidthRatio int // 1-10 units for file tree panel - treePosition keymap.TreePosition // side the file tree or markdown TOC renders on tabSpaces string // spaces to replace tabs with wrapIndent int // extra indent (in columns) for wrap continuation rows; 0 disables annotPrefix string // cached: marker + " " annotFilePrefix string // cached: marker + " file: " outputPath string // --output destination for the O in-session flush; empty disables it + + treePosition keymap.TreePosition // side the file tree or markdown TOC renders on } // layoutState holds viewport and layout concerns that change on resize and pane toggles. @@ -729,7 +730,6 @@ type ModelConfig struct { Ref string Staged bool TreeWidthRatio int - TreePosition keymap.TreePosition // side the file tree or markdown TOC renders on TabWidth int // number of spaces per tab character NoColors bool // disable all colors including syntax highlighting MouseTracking bool // enable mouse tracking for clicks and wheel events @@ -790,6 +790,8 @@ type ModelConfig struct { // disables the flush (there is no file to write to); a non-empty path enables // it. Copied into modelConfigState.outputPath as a plain value. OutputPath string + + TreePosition keymap.TreePosition // side the file tree or markdown TOC renders on } // NewModel creates a new Model from the given configuration. All dependencies @@ -847,7 +849,7 @@ func NewModel(cfg ModelConfig) (Model, error) { } km := cfg.Keymap if km == nil { - km = keymap.Default(cfg.TreePosition) + km = keymap.DefaultForTreePosition(cfg.TreePosition) } ed := cfg.Editor if ed == nil || isNilValue(ed) { diff --git a/app/ui/model_test.go b/app/ui/model_test.go index 726deb9f..a49f17ca 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -230,7 +230,7 @@ func TestNewModel_OptionalDefaults(t *testing.T) { FileDiffFunc: func(diff.FileDiffRequest) ([]diff.DiffLine, error) { return nil, nil }, } - t.Run("nil keymap defaults to keymap.Default(keymap.TreePositionLeft)", func(t *testing.T) { + t.Run("nil keymap defaults to keymap.Default()", func(t *testing.T) { m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{}) require.NotNil(t, m.keymap) // verify a known default binding works @@ -239,7 +239,7 @@ func TestNewModel_OptionalDefaults(t *testing.T) { }) t.Run("custom keymap is used when provided", func(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Unbind("q") m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{Keymap: km}) action := m.keymap.Resolve("q") @@ -260,13 +260,6 @@ func TestNewModel_OptionalDefaults(t *testing.T) { m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreeWidthRatio: 5}) assert.Equal(t, 5, m.cfg.treeWidthRatio) }) - - t.Run("TreePosition is kept and default pane keys follow layout", func(t *testing.T) { - m := testNewModel(t, renderer, annotation.NewStore(), noopHighlighter(), ModelConfig{TreePosition: keymap.TreePositionRight}) - assert.Equal(t, keymap.TreePositionRight, m.cfg.treePosition) - assert.Equal(t, keymap.ActionFocusDiff, m.keymap.Resolve("h")) - assert.Equal(t, keymap.ActionFocusTree, m.keymap.Resolve("l")) - }) } func TestModel_Init(t *testing.T) { @@ -508,32 +501,36 @@ func TestModel_TreeNavigation(t *testing.T) { } func TestModel_FocusSwitching(t *testing.T) { - for _, tc := range []struct { - name string - treePos keymap.TreePosition - toDiffKey rune - toTreeKey rune - }{ - {name: "tree on left", toDiffKey: 'l', toTreeKey: 'h'}, - {name: "tree on right", treePos: keymap.TreePositionRight, toDiffKey: 'h', toTreeKey: 'l'}, - } { - t.Run(tc.name, func(t *testing.T) { - m := testModel([]string{"a.go"}, nil) - m.tree = testNewFileTree([]string{"a.go"}) - m.keymap = keymap.Default(tc.treePos) - m.cfg.treePosition = tc.treePos - m.file.name = "a.go" // pretend a file is loaded - m.layout.focus = paneTree + m := testModel([]string{"a.go"}, nil) + m.tree = testNewFileTree([]string{"a.go"}) + m.file.name = "a.go" // pretend a file is loaded + m.layout.focus = paneTree - result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toDiffKey}}) - model := result.(Model) - assert.Equal(t, paneDiff, model.layout.focus) + // l switches to diff pane + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) + model := result.(Model) + assert.Equal(t, paneDiff, model.layout.focus) - result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toTreeKey}}) - model = result.(Model) - assert.Equal(t, paneTree, model.layout.focus) - }) - } + // h switches back to tree + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) + model = result.(Model) + assert.Equal(t, paneTree, model.layout.focus) +} + +func TestModel_FocusSwitching_RightTree(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.tree = testNewFileTree([]string{"a.go"}) + m.keymap = keymap.DefaultForTreePosition(keymap.TreePositionRight) + m.file.name = "a.go" + m.layout.focus = paneTree + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) + model := result.(Model) + assert.Equal(t, paneDiff, model.layout.focus) + + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) + model = result.(Model) + assert.Equal(t, paneTree, model.layout.focus) } func TestModel_WindowResize(t *testing.T) { @@ -579,7 +576,7 @@ func TestModel_TreeWidthRatio(t *testing.T) { func TestModel_CustomKeymapQuitOverride(t *testing.T) { // map "x" to quit, unbind "q" β€” verify "x" quits and "q" does not - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("x", keymap.ActionQuit) km.Unbind("q") @@ -600,7 +597,7 @@ func TestModel_CustomKeymapQuitOverride(t *testing.T) { func TestModel_CustomKeymapViewToggle(t *testing.T) { // map "x" to toggle_wrap β€” verify "x" toggles wrap and "w" still works - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("x", keymap.ActionToggleWrap) lines := []diff.DiffLine{{NewNum: 1, Content: "ctx", ChangeType: diff.ChangeContext}} @@ -625,7 +622,7 @@ func TestModel_CustomKeymapViewToggle(t *testing.T) { func TestModel_CustomKeymapTreeNav(t *testing.T) { // map "x" to down, unbind "j" β€” verify "x" moves tree cursor and "j" does not - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("x", keymap.ActionDown) km.Unbind("j") @@ -664,7 +661,7 @@ func TestModel_CustomKeymapTreeFocusDiff(t *testing.T) { func TestModel_AcceptanceAdditiveQuitBinding(t *testing.T) { // map x quit (additive) β€” both x and q should quit - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) @@ -688,7 +685,7 @@ func TestModel_AcceptanceAdditiveQuitBinding(t *testing.T) { func TestModel_AcceptanceDefaultBehaviorNoKeybindingsFile(t *testing.T) { // no keybindings file β†’ identical behavior to current defaults m := testModel([]string{"a.go"}, nil) - // m.keymap is set to Default(false) in testModel via NewModel + // m.keymap is set to Default() in testModel via NewModel // q should quit _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) @@ -949,7 +946,7 @@ func TestDispatchAction_PaneNavFallback_Tree(t *testing.T) { } func TestHandleChordSecond_ResolvedDispatches(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -967,7 +964,7 @@ func TestHandleChordSecond_ResolvedDispatches(t *testing.T) { } func TestHandleChordSecond_UnboundShowsHint(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -983,7 +980,7 @@ func TestHandleChordSecond_UnboundShowsHint(t *testing.T) { } func TestHandleChordSecond_EscCancels(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -999,7 +996,7 @@ func TestHandleChordSecond_EscCancels(t *testing.T) { } func TestHandleChordSecond_LayoutFallback(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1029,7 +1026,7 @@ func TestHandleChordSecond_DispatchesToTOCWhenFocused(t *testing.T) { {NewNum: 2, Content: "text", ChangeType: diff.ChangeContext}, {NewNum: 3, Content: "## Second", ChangeType: diff.ChangeContext}, } - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionDown) m := testModel([]string{"README.md"}, map[string][]diff.DiffLine{"README.md": mdLines}) @@ -1118,7 +1115,7 @@ func TestTransientHint_OutputPriority(t *testing.T) { } func TestHandleKey_EntersChordPending(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1132,7 +1129,7 @@ func TestHandleKey_EntersChordPending(t *testing.T) { } func TestHandleKey_ChordSecondCoexistenceGuard(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1155,7 +1152,7 @@ func TestHandleKey_ChordSecondCoexistenceGuard(t *testing.T) { } func TestHandleKey_ChordIgnoredWhenPendingReload(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1175,7 +1172,7 @@ func TestHandleKey_ChordIgnoredWhenPendingReload(t *testing.T) { } func TestHandleKey_LeaderWithStandaloneActionDoesNotEnterChord(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() // bind ctrl+w as a standalone action (no chord binding for ctrl+w>*) km.Bind("ctrl+w", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) @@ -1560,7 +1557,7 @@ func TestHandleKey_ChordPrecedence(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1599,7 +1596,7 @@ func TestHandleKey_VimMotionOn_DigitAccumulates(t *testing.T) { } func TestHandleKey_VimMotionOn_ChordSecondWins(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km @@ -1709,7 +1706,7 @@ func TestHandleKey_NonKeyMessagesPreserveChordState(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - km := keymap.Default(keymap.TreePositionLeft) + km := keymap.Default() km.Bind("ctrl+w>x", keymap.ActionQuit) m := testModel([]string{"a.go"}, nil) m.keymap = km diff --git a/app/ui/view_test.go b/app/ui/view_test.go index d71ad5ed..26e699ba 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -1104,7 +1104,7 @@ func TestModel_HKeySwitchesToTOC(t *testing.T) { m.file.lines = mdLines m.layout.focus = paneDiff m.cfg.treePosition = keymap.TreePositionRight - m.keymap = keymap.Default(keymap.TreePositionRight) + m.keymap = keymap.DefaultForTreePosition(keymap.TreePositionRight) result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) model := result.(Model) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2b85a2f2..78047d3f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -152,7 +152,7 @@ Two independent component types, both with cursor/offset management, rendering, - **`FileTree`** β€” file tree sidebar. Supports navigation (`Move`/`StepFile`), filtering (annotated-only), semantic-fingerprint reviewed tracking, directory grouping. File-list reloads revalidate only paths reviewed before the load; marks added during the load are reconciled when that file's refreshed diff arrives. `VisibleFiles()` exposes file paths in rendered order after active filters for consumers such as the file picker. - **`TOC`** β€” markdown table-of-contents. Activated for single-file full-context markdown. Active section tracking, header-level navigation -Both are constructed via factory closures in `main.go`, consumed through `FileTreeComponent`/`TOCComponent` interfaces, and rendered on the left or right according to `tree-position`. +Both constructed via factory closures in `main.go`, consumed through `FileTreeComponent`/`TOCComponent` interfaces. ### app/ui/overlay/ β€” popup layers diff --git a/plugins/codex/skills/revdiff/references/config.md b/plugins/codex/skills/revdiff/references/config.md index 89136e99..d8999075 100644 --- a/plugins/codex/skills/revdiff/references/config.md +++ b/plugins/codex/skills/revdiff/references/config.md @@ -60,7 +60,7 @@ Then uncomment and edit the values you want to change. | `--config` | `REVDIFF_CONFIG` | Path to config file | `~/.config/revdiff/config` | | `--dump-config` | | Print default config to stdout and exit | | -Config-backed options use long flag names without leading `--`; for example, use `tree-position = right` to put the file tree and markdown TOC on the right. For annotation exit status use `exit-code-on-annotations = true`. +Config-backed options use long flag names without leading `--`; for annotation exit status use `exit-code-on-annotations = true`. ## Popup Size (Claude Code plugin) diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index a3fc751d..eadec9e3 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -35,7 +35,7 @@ When a diff contains exactly one file, revdiff automatically hides the file tree ## Markdown TOC Navigation -When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Set `tree-position = right` in config, pass `--tree-position=right`, or set `REVDIFF_TREE_POSITION=right` to move both the file tree and TOC to the right. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. +When reviewing a single markdown file in context-only mode (e.g., `revdiff --only=README.md`), a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers with indentation by level. Use `Tab` to switch between TOC and diff, `j`/`k` to navigate headers, `n`/`p` to jump to next/prev header from either pane, `Enter` to jump to a header. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded. ## All-Files Mode diff --git a/plugins/pi/skills/revdiff/SKILL.md b/plugins/pi/skills/revdiff/SKILL.md index 9ad2e8ef..97468357 100644 --- a/plugins/pi/skills/revdiff/SKILL.md +++ b/plugins/pi/skills/revdiff/SKILL.md @@ -35,7 +35,6 @@ Tool examples: - `args: "--untracked"`: review untracked files with working-tree changes - `args: "--only README.md"`: review one standalone file - `args: "--all-files --exclude vendor"`: review all tracked files except vendor -- `args: "--tree-position=right main"`: review with the file tree on the right - `args: "--description='why this refactor matters' main"`: include review context in the info popup - `args: "--description-file=/tmp/revdiff-desc.md main"`: include longer markdown review context - `args: "--annotations=/tmp/revdiff-review.md main"`: preload in-session review notes diff --git a/site/docs.html b/site/docs.html index 23abfbde..4ebf1368 100644 --- a/site/docs.html +++ b/site/docs.html @@ -289,7 +289,7 @@

Review description

--description and --description-file are mutually exclusive. The description section is hidden when neither is set, so the flag is purely additive.

Markdown TOC navigation

-

When reviewing a single markdown file in context-only mode, a table-of-contents pane appears on the configured tree side (left by default) listing all markdown headers. This works for --only files and for --stdin when --stdin-name ends with .md or .markdown. Use Tab to switch focus, j/k to navigate, Enter to jump. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded.

+

When reviewing a single markdown file in context-only mode, a table-of-contents pane appears on the configured side listing all markdown headers. This works for --only files and for --stdin when --stdin-name ends with .md or .markdown. Use Tab to switch focus, j/k to navigate, Enter to jump. The TOC highlights the current section as you scroll. Headers inside fenced code blocks are excluded.

Beyond code review

The --only and --stdin flags enable use cases beyond git diffs. Files and ephemeral command output can both be loaded for annotation, no git repo required.

@@ -388,7 +388,6 @@

Config file

Precedence: CLI flags > env vars > config file > built-in defaults. For annotation exit status, use REVDIFF_EXIT_CODE_ON_ANNOTATIONS or exit-code-on-annotations = true.

mkdir -p ~/.config/revdiff revdiff --dump-config > ~/.config/revdiff/config
-

Set tree-position = right to place the file tree and markdown TOC on the right.

Options

--stagedShow staged changesfalse
--untrackedShow untracked files in the treefalse
--tree-widthFile tree width in units (1-10)2
--tree-positionFile tree and markdown TOC position (left or right)left
--tab-widthSpaces per tab character4
--no-colorsDisable all colorsfalse
--no-status-barHide the status barfalse
From 30b56cbd2297464bc35f3a3ccdcceeea84a3efc4 Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Sat, 18 Jul 2026 22:03:38 -0700 Subject: [PATCH 5/6] feat(ui): add directional focus actions and decouple keymap from layout - add `focus_left` and `focus_right` actions to `keymap` - implement `resolveDirectionalFocus` to map spatial focus to semantic panes based on `treePosition` - remove `TreePosition` dependency from `keymap` package to simplify configuration and decoupling - update default keybindings to use `focus_left`/`focus_right` instead of fixed `h`/`l` logic - update documentation, examples, and tests to reflect the new focus logic --- .../skills/revdiff/references/config.md | 2 +- README.md | 2 +- app/config.go | 8 +-- app/keymap/keymap.go | 66 +++++-------------- app/keymap/keymap_test.go | 23 ++----- app/main.go | 4 +- app/ui/model.go | 34 +++++++++- app/ui/model_test.go | 15 ++++- app/ui/mouse.go | 3 +- app/ui/mouse_test.go | 11 ++-- app/ui/view.go | 3 +- app/ui/view_test.go | 15 ++--- .../codex/skills/revdiff/references/config.md | 2 +- site/docs.html | 2 +- 14 files changed, 92 insertions(+), 98 deletions(-) diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index c4fc139a..87e7240b 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -153,7 +153,7 @@ unmap q map ctrl+d half_page_down ``` -Available actions: `down`, `up`, `page_down`, `page_up`, `half_page_down`, `half_page_up`, `home`, `end`, `scroll_left`, `scroll_right`, `scroll_center`, `scroll_top`, `scroll_bottom`, `scroll_diff_down`, `scroll_diff_up`, `next_item`, `prev_item`, `jump_file`, `next_hunk`, `prev_hunk`, `open_file_in_editor`, `toggle_pane`, `focus_tree`, `focus_diff`, `search`, `confirm`, `annotate_file`, `delete_annotation`, `annot_list`, `open_editor`, `next_annotation`, `prev_annotation`, `flush_output`, `toggle_collapsed`, `toggle_compact`, `toggle_wrap`, `toggle_tree`, `toggle_line_numbers`, `toggle_blame`, `toggle_word_diff`, `toggle_hunk`, `toggle_untracked`, `mark_reviewed`, `theme_select`, `filter`, `info`, `reload`, `quit`, `discard_quit`, `help`, `dismiss` +Available actions: `down`, `up`, `page_down`, `page_up`, `half_page_down`, `half_page_up`, `home`, `end`, `scroll_left`, `scroll_right`, `scroll_center`, `scroll_top`, `scroll_bottom`, `scroll_diff_down`, `scroll_diff_up`, `next_item`, `prev_item`, `jump_file`, `next_hunk`, `prev_hunk`, `open_file_in_editor`, `toggle_pane`, `focus_left`, `focus_right`, `focus_tree`, `focus_diff`, `search`, `confirm`, `annotate_file`, `delete_annotation`, `annot_list`, `open_editor`, `next_annotation`, `prev_annotation`, `flush_output`, `toggle_collapsed`, `toggle_compact`, `toggle_wrap`, `toggle_tree`, `toggle_line_numbers`, `toggle_blame`, `toggle_word_diff`, `toggle_hunk`, `toggle_untracked`, `mark_reviewed`, `theme_select`, `filter`, `info`, `reload`, `quit`, `discard_quit`, `help`, `dismiss` Fixed modal keys (Enter, Esc in annotation/search input, confirm discard) are not remappable. Keymap-resolved actions like `open_editor` work during annotation input and can be rebound. Chord bindings do not fire during text input β€” use single-key `ctrl+*` bindings for actions that need to work during annotation input. diff --git a/README.md b/README.md index 96f121ce..7f01a23d 100644 --- a/README.md +++ b/README.md @@ -886,7 +886,7 @@ When the leader is pressed, the status bar shows `Pending: ctrl+w, esc to cancel **File/Hunk:** `next_item`, `prev_item`, `jump_file`, `next_hunk`, `prev_hunk`, `open_file_in_editor` -**Pane:** `toggle_pane`, `focus_tree`, `focus_diff` +**Pane:** `toggle_pane`, `focus_left`, `focus_right`, `focus_tree`, `focus_diff` **Search:** `search` diff --git a/app/config.go b/app/config.go index ebf495fb..d481e004 100644 --- a/app/config.go +++ b/app/config.go @@ -10,7 +10,7 @@ import ( "github.com/jessevdk/go-flags" - "github.com/umputun/revdiff/app/keymap" + "github.com/umputun/revdiff/app/ui" ) type options struct { @@ -123,11 +123,11 @@ func (o options) startupUntracked() bool { } // treePosition resolves the --tree-position flag value to its typed position. -func (o options) treePosition() keymap.TreePosition { +func (o options) treePosition() ui.TreePosition { if o.TreePosition == "right" { - return keymap.TreePositionRight + return ui.TreePositionRight } - return keymap.TreePositionLeft + return ui.TreePositionLeft } // parseArgs parses CLI arguments with config file support. diff --git a/app/keymap/keymap.go b/app/keymap/keymap.go index 05a662b7..35085af3 100644 --- a/app/keymap/keymap.go +++ b/app/keymap/keymap.go @@ -40,6 +40,8 @@ const ( ActionNextHunk Action = "next_hunk" ActionPrevHunk Action = "prev_hunk" ActionTogglePane Action = "toggle_pane" + ActionFocusLeft Action = "focus_left" + ActionFocusRight Action = "focus_right" ActionFocusTree Action = "focus_tree" ActionFocusDiff Action = "focus_diff" ActionSearch Action = "search" @@ -85,7 +87,8 @@ var validActions = map[Action]bool{ ActionScrollDiffDown: true, ActionScrollDiffUp: true, ActionNextItem: true, ActionPrevItem: true, ActionJumpFile: true, ActionNextHunk: true, ActionPrevHunk: true, - ActionTogglePane: true, ActionFocusTree: true, ActionFocusDiff: true, + ActionTogglePane: true, ActionFocusLeft: true, ActionFocusRight: true, + ActionFocusTree: true, ActionFocusDiff: true, ActionSearch: true, ActionConfirm: true, ActionAnnotateFile: true, ActionDeleteAnnotation: true, ActionAnnotList: true, ActionNextAnnotation: true, ActionPrevAnnotation: true, @@ -212,6 +215,8 @@ func defaultDescriptions() []HelpEntry { // pane {ActionTogglePane, "toggle pane focus", SectionPane}, + {ActionFocusLeft, "focus left pane", SectionPane}, + {ActionFocusRight, "focus right pane", SectionPane}, {ActionFocusTree, "focus tree pane", SectionPane}, {ActionFocusDiff, "focus diff pane", SectionPane}, @@ -253,19 +258,9 @@ func defaultDescriptions() []HelpEntry { } } -// TreePosition is the side of the screen the file tree (or markdown TOC) -// pane renders on. The zero value is TreePositionLeft, the default layout. -type TreePosition int - -const ( - TreePositionLeft TreePosition = iota - TreePositionRight -) - // defaultBindings returns the default key-to-action mapping. -// h/l follow the visual pane order while the actions remain semantic. -func defaultBindings(pos TreePosition) map[string]Action { - bindings := map[string]Action{ +func defaultBindings() map[string]Action { + return map[string]Action{ "j": ActionDown, "k": ActionUp, "down": ActionDown, @@ -288,8 +283,8 @@ func defaultBindings(pos TreePosition) map[string]Action { "[": ActionPrevHunk, "e": ActionOpenFileInEditor, "tab": ActionTogglePane, - "h": ActionFocusTree, - "l": ActionFocusDiff, + "h": ActionFocusLeft, + "l": ActionFocusRight, "/": ActionSearch, "a": ActionConfirm, "enter": ActionConfirm, @@ -320,23 +315,12 @@ func defaultBindings(pos TreePosition) map[string]Action { "R": ActionReload, "esc": ActionDismiss, } - if pos == TreePositionRight { - bindings["h"] = ActionFocusDiff - bindings["l"] = ActionFocusTree - } - return bindings } // Default returns a Keymap with all default bindings. func Default() *Keymap { - return DefaultForTreePosition(TreePositionLeft) -} - -// DefaultForTreePosition returns the default keymap with h/l following the -// visual pane order for the given tree position. -func DefaultForTreePosition(pos TreePosition) *Keymap { return &Keymap{ - bindings: defaultBindings(pos), + bindings: defaultBindings(), descriptions: defaultDescriptions(), } } @@ -654,10 +638,6 @@ func parse(r io.Reader) (maps []mapEntry, unmaps []string, err error) { // Load reads a keybindings file from path and returns a Keymap with defaults // overridden by the file contents. Returns error if the file cannot be opened or parsed. func Load(path string) (*Keymap, error) { - return load(path, TreePositionLeft) -} - -func load(path string, pos TreePosition) (*Keymap, error) { f, err := os.Open(path) //nolint:gosec // path is user-provided config file location if err != nil { return nil, fmt.Errorf("opening keybindings file: %w", err) @@ -669,7 +649,7 @@ func load(path string, pos TreePosition) (*Keymap, error) { return nil, err } - km := DefaultForTreePosition(pos) + km := Default() // apply unmaps first, then maps (so "unmap q" + "map x quit" works) for _, key := range unmaps { @@ -702,29 +682,19 @@ func (km *Keymap) resolveConflicts() { km.chordPrefixCache = nil } -// LoadOrDefault loads keybindings from path if it exists, otherwise returns -// Default. Parse errors are logged as warnings and defaults are returned. +// LoadOrDefault loads keybindings from path if the file exists, otherwise returns +// Default(). Parse errors are logged as warnings and Default() is returned. func LoadOrDefault(path string) *Keymap { - return loadOrDefault(path, TreePositionLeft) -} - -// LoadOrDefaultForTreePosition loads keybindings over defaults whose h/l -// bindings follow the visual pane order for the given tree position. -func LoadOrDefaultForTreePosition(path string, pos TreePosition) *Keymap { - return loadOrDefault(path, pos) -} - -func loadOrDefault(path string, pos TreePosition) *Keymap { if path == "" { - return DefaultForTreePosition(pos) + return Default() } if _, err := os.Stat(path); os.IsNotExist(err) { - return DefaultForTreePosition(pos) + return Default() } - km, err := load(path, pos) + km, err := Load(path) if err != nil { log.Printf("[WARN] failed to load keybindings from %s: %v, using defaults", path, err) - return DefaultForTreePosition(pos) + return Default() } return km } diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index f49e92f5..80e57d24 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -18,13 +18,6 @@ func TestDefault(t *testing.T) { assert.NotEmpty(t, km.descriptions) } -func TestDefaultForTreePosition(t *testing.T) { - right := DefaultForTreePosition(TreePositionRight) - assert.Equal(t, ActionFocusDiff, right.Resolve("h")) - assert.Equal(t, ActionFocusTree, right.Resolve("l")) - assert.Equal(t, ActionDown, right.Resolve("j"), "unrelated defaults must stay unchanged") -} - func TestDefault_allExpectedBindings(t *testing.T) { km := Default() tests := []struct { @@ -40,7 +33,7 @@ func TestDefault_allExpectedBindings(t *testing.T) { {"n", ActionNextItem}, {"N", ActionPrevItem}, {"p", ActionPrevItem}, {"ctrl+p", ActionJumpFile}, {"]", ActionNextHunk}, {"[", ActionPrevHunk}, {"e", ActionOpenFileInEditor}, - {"tab", ActionTogglePane}, {"h", ActionFocusTree}, {"l", ActionFocusDiff}, + {"tab", ActionTogglePane}, {"h", ActionFocusLeft}, {"l", ActionFocusRight}, {"/", ActionSearch}, {"a", ActionConfirm}, {"enter", ActionConfirm}, {"A", ActionAnnotateFile}, {"d", ActionDeleteAnnotation}, {"@", ActionAnnotList}, {"ctrl+e", ActionOpenEditor}, @@ -411,6 +404,10 @@ func TestActionScrollConstants_NoDefaultBindings(t *testing.T) { func TestIsValidAction(t *testing.T) { assert.True(t, IsValidAction(ActionQuit)) assert.True(t, IsValidAction(ActionDown)) + assert.True(t, IsValidAction(ActionFocusLeft)) + assert.True(t, IsValidAction(ActionFocusRight)) + assert.True(t, IsValidAction(ActionFocusTree)) + assert.True(t, IsValidAction(ActionFocusDiff)) assert.True(t, IsValidAction(ActionInfo)) assert.True(t, IsValidAction(Action("commit_info")), "deprecated alias must validate") assert.False(t, IsValidAction(Action("nonexistent"))) @@ -766,16 +763,6 @@ func TestLoadOrDefault_withFile(t *testing.T) { assert.Equal(t, ActionDown, km.Resolve("j")) // defaults still present } -func TestLoadOrDefaultForTreePosition_UserOverrides(t *testing.T) { - tmpFile := t.TempDir() + "/keybindings" - err := os.WriteFile(tmpFile, []byte("map h quit\nunmap l\n"), 0o600) - require.NoError(t, err) - - km := LoadOrDefaultForTreePosition(tmpFile, TreePositionRight) - assert.Equal(t, ActionQuit, km.Resolve("h")) - assert.Empty(t, km.Resolve("l")) -} - func TestLoad_unmapOfUnboundKey(t *testing.T) { tmpFile := t.TempDir() + "/keybindings" err := os.WriteFile(tmpFile, []byte("unmap z\n"), 0o600) diff --git a/app/main.go b/app/main.go index e6f10dd3..f03918a6 100644 --- a/app/main.go +++ b/app/main.go @@ -55,7 +55,7 @@ func main() { } if opts.DumpKeys { - km := keymap.LoadOrDefaultForTreePosition(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath), opts.treePosition()) + km := keymap.LoadOrDefault(resolveFlagPath(os.Args[1:], "keys", "REVDIFF_KEYS", defaultKeysPath)) if err := km.Dump(os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) @@ -108,7 +108,7 @@ func run(opts options) (int, error) { store := annotation.NewStore() hl := highlight.New(opts.ChromaStyle, !opts.NoColors) - km := keymap.LoadOrDefaultForTreePosition(resolveKeysPath(opts), opts.treePosition()) + km := keymap.LoadOrDefault(resolveKeysPath(opts)) var ( renderer ui.Renderer diff --git a/app/ui/model.go b/app/ui/model.go index 703a82f5..202dac07 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -278,6 +278,14 @@ type TOCComponent interface { Render(r sidepane.TOCRender) string } +// TreePosition is the side of the screen where the file tree or markdown TOC renders. +type TreePosition int + +const ( + TreePositionLeft TreePosition = iota + TreePositionRight +) + // pane identifies which pane has focus. type pane int @@ -333,7 +341,7 @@ type modelConfigState struct { annotFilePrefix string // cached: marker + " file: " outputPath string // --output destination for the O in-session flush; empty disables it - treePosition keymap.TreePosition // side the file tree or markdown TOC renders on + treePosition TreePosition // side the file tree or markdown TOC renders on } // layoutState holds viewport and layout concerns that change on resize and pane toggles. @@ -791,7 +799,7 @@ type ModelConfig struct { // it. Copied into modelConfigState.outputPath as a plain value. OutputPath string - TreePosition keymap.TreePosition // side the file tree or markdown TOC renders on + TreePosition TreePosition // side the file tree or markdown TOC renders on } // NewModel creates a new Model from the given configuration. All dependencies @@ -849,7 +857,7 @@ func NewModel(cfg ModelConfig) (Model, error) { } km := cfg.Keymap if km == nil { - km = keymap.DefaultForTreePosition(cfg.TreePosition) + km = keymap.Default() } ed := cfg.Editor if ed == nil || isNilValue(ed) { @@ -1062,11 +1070,31 @@ func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m.dispatchAction(action) } +// resolveDirectionalFocus maps spatial focus actions to semantic pane actions. +// Explicit user bindings to focus_tree or focus_diff bypass this mapping. +func (m Model) resolveDirectionalFocus(action keymap.Action) keymap.Action { + switch action { + case keymap.ActionFocusLeft: + if m.cfg.treePosition == TreePositionRight { + return keymap.ActionFocusDiff + } + return keymap.ActionFocusTree + case keymap.ActionFocusRight: + if m.cfg.treePosition == TreePositionRight { + return keymap.ActionFocusTree + } + return keymap.ActionFocusDiff + default: + return action + } +} + // dispatchAction routes a resolved keymap action through overlay-open, the // global action switch, and the pane-specific nav fallback. It is the unified // dispatch path shared by keymap-resolved single keys (handleKey) and by // chord-resolved actions (handleChordSecond). func (m Model) dispatchAction(action keymap.Action) (tea.Model, tea.Cmd) { + action = m.resolveDirectionalFocus(action) if model, cmd, ok := m.handleOverlayOpen(action); ok { return model, cmd } diff --git a/app/ui/model_test.go b/app/ui/model_test.go index a49f17ca..ec1b42c7 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -520,7 +520,7 @@ func TestModel_FocusSwitching(t *testing.T) { func TestModel_FocusSwitching_RightTree(t *testing.T) { m := testModel([]string{"a.go"}, nil) m.tree = testNewFileTree([]string{"a.go"}) - m.keymap = keymap.DefaultForTreePosition(keymap.TreePositionRight) + m.cfg.treePosition = TreePositionRight m.file.name = "a.go" m.layout.focus = paneTree @@ -533,6 +533,19 @@ func TestModel_FocusSwitching_RightTree(t *testing.T) { assert.Equal(t, paneTree, model.layout.focus) } +func TestModel_FocusSwitching_UserSemanticBinding(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.tree = testNewFileTree([]string{"a.go"}) + m.cfg.treePosition = TreePositionRight + m.keymap.Bind("h", keymap.ActionFocusTree) + m.file.name = "a.go" + m.layout.focus = paneDiff + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) + model := result.(Model) + assert.Equal(t, paneTree, model.layout.focus) +} + func TestModel_WindowResize(t *testing.T) { m := testModel([]string{"a.go"}, nil) m.ready = false diff --git a/app/ui/mouse.go b/app/ui/mouse.go index 7945154f..b302f6cb 100644 --- a/app/ui/mouse.go +++ b/app/ui/mouse.go @@ -5,7 +5,6 @@ import ( tea "github.com/charmbracelet/bubbletea" - "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/ui/overlay" "github.com/umputun/revdiff/app/ui/sidepane" ) @@ -114,7 +113,7 @@ func (m Model) hitTest(x, y int) hitZone { if !m.treePaneHidden() { treeStart := 0 - if m.cfg.treePosition == keymap.TreePositionRight { + if m.cfg.treePosition == TreePositionRight { // pane width includes the content plus its left and right borders. treeStart = m.layout.width - m.layout.treeWidth - 2 } diff --git a/app/ui/mouse_test.go b/app/ui/mouse_test.go index 1e4bbe07..0704e027 100644 --- a/app/ui/mouse_test.go +++ b/app/ui/mouse_test.go @@ -12,7 +12,6 @@ import ( "github.com/stretchr/testify/require" "github.com/umputun/revdiff/app/annotation" - "github.com/umputun/revdiff/app/keymap" "github.com/umputun/revdiff/app/diff" "github.com/umputun/revdiff/app/ui/overlay" "github.com/umputun/revdiff/app/ui/sidepane" @@ -97,28 +96,28 @@ func TestModel_hitTest(t *testing.T) { { name: "right tree: last diff column", setup: func(m *Model) { - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight }, x: 81, y: 10, want: hitDiff, }, { name: "right tree: first tree column", setup: func(m *Model) { - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight }, x: 82, y: 10, want: hitTree, }, { name: "right tree: tree top border", setup: func(m *Model) { - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight }, x: 100, y: 0, want: hitNone, }, { name: "right tree: diff header on left", setup: func(m *Model) { - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight }, x: 5, y: 1, want: hitHeader, }, @@ -463,7 +462,7 @@ func TestModel_HandleMouse_WheelInTreeMovesTreeCursor(t *testing.T) { t.Run("right-positioned tree receives wheel events on the right", func(t *testing.T) { m := mouseTestModel(t, files, diffs) - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight require.Equal(t, "aa.go", m.tree.SelectedFile()) result, _ := m.Update(wheelMsg(tea.MouseButtonWheelDown, 100, 3, false)) diff --git a/app/ui/view.go b/app/ui/view.go index c726c18f..d150c5ac 100644 --- a/app/ui/view.go +++ b/app/ui/view.go @@ -7,7 +7,6 @@ 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" @@ -112,7 +111,7 @@ func (m Model) renderTwoPaneLayout(navigationContent, diffContent string, naviga Render(diffContent) diffPane = m.applyScrollbar(diffPane) - if m.cfg.treePosition == keymap.TreePositionRight { + if m.cfg.treePosition == TreePositionRight { return lipgloss.JoinHorizontal(lipgloss.Top, diffPane, navigationPane) } return lipgloss.JoinHorizontal(lipgloss.Top, navigationPane, diffPane) diff --git a/app/ui/view_test.go b/app/ui/view_test.go index 26e699ba..6c14aa41 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -608,7 +608,7 @@ func TestModel_ViewSingleFileMode(t *testing.T) { } func TestModel_ViewTreePosition(t *testing.T) { - assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, treePos keymap.TreePosition) { + assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, treePos TreePosition) { t.Helper() labelColumn := func(lines []string, label string) int { for _, line := range lines { @@ -624,7 +624,7 @@ func TestModel_ViewTreePosition(t *testing.T) { navigationIdx := labelColumn(lines, navigationLabel) require.NotEqual(t, -1, diffIdx, "diff label must appear in the rendered view") require.NotEqual(t, -1, navigationIdx, "navigation label must appear in the rendered view") - if treePos == keymap.TreePositionRight { + if treePos == TreePositionRight { assert.Less(t, diffIdx, navigationIdx) return } @@ -633,10 +633,10 @@ func TestModel_ViewTreePosition(t *testing.T) { for _, tc := range []struct { name string - treePos keymap.TreePosition + treePos TreePosition }{ {name: "file tree on left"}, - {name: "file tree on right", treePos: keymap.TreePositionRight}, + {name: "file tree on right", treePos: TreePositionRight}, } { t.Run(tc.name, func(t *testing.T) { m := testModel([]string{"tree.go", "other.go"}, nil) @@ -659,10 +659,10 @@ func TestModel_ViewTreePosition(t *testing.T) { ) require.NotNil(t, m.file.mdTOC) m.file.name = "plan.md" - m.cfg.treePosition = keymap.TreePositionRight + m.cfg.treePosition = TreePositionRight m.cfg.noStatusBar = true - assertOrder(t, m.View(), "plan.md", "Navigation section", keymap.TreePositionRight) + assertOrder(t, m.View(), "plan.md", "Navigation section", TreePositionRight) }) } @@ -1103,8 +1103,7 @@ func TestModel_HKeySwitchesToTOC(t *testing.T) { m.file.name = "README.md" m.file.lines = mdLines m.layout.focus = paneDiff - m.cfg.treePosition = keymap.TreePositionRight - m.keymap = keymap.DefaultForTreePosition(keymap.TreePositionRight) + m.cfg.treePosition = TreePositionRight result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) model := result.(Model) diff --git a/plugins/codex/skills/revdiff/references/config.md b/plugins/codex/skills/revdiff/references/config.md index d8999075..48c91b24 100644 --- a/plugins/codex/skills/revdiff/references/config.md +++ b/plugins/codex/skills/revdiff/references/config.md @@ -148,7 +148,7 @@ unmap q map ctrl+d half_page_down ``` -Available actions: `down`, `up`, `page_down`, `page_up`, `half_page_down`, `half_page_up`, `home`, `end`, `scroll_left`, `scroll_right`, `scroll_center`, `scroll_top`, `scroll_bottom`, `scroll_diff_down`, `scroll_diff_up`, `next_item`, `prev_item`, `jump_file`, `next_hunk`, `prev_hunk`, `open_file_in_editor`, `toggle_pane`, `focus_tree`, `focus_diff`, `search`, `confirm`, `annotate_file`, `delete_annotation`, `annot_list`, `open_editor`, `next_annotation`, `prev_annotation`, `flush_output`, `toggle_collapsed`, `toggle_compact`, `toggle_wrap`, `toggle_tree`, `toggle_line_numbers`, `toggle_blame`, `toggle_word_diff`, `toggle_hunk`, `toggle_untracked`, `mark_reviewed`, `theme_select`, `filter`, `info`, `reload`, `quit`, `discard_quit`, `help`, `dismiss` +Available actions: `down`, `up`, `page_down`, `page_up`, `half_page_down`, `half_page_up`, `home`, `end`, `scroll_left`, `scroll_right`, `scroll_center`, `scroll_top`, `scroll_bottom`, `scroll_diff_down`, `scroll_diff_up`, `next_item`, `prev_item`, `jump_file`, `next_hunk`, `prev_hunk`, `open_file_in_editor`, `toggle_pane`, `focus_left`, `focus_right`, `focus_tree`, `focus_diff`, `search`, `confirm`, `annotate_file`, `delete_annotation`, `annot_list`, `open_editor`, `next_annotation`, `prev_annotation`, `flush_output`, `toggle_collapsed`, `toggle_compact`, `toggle_wrap`, `toggle_tree`, `toggle_line_numbers`, `toggle_blame`, `toggle_word_diff`, `toggle_hunk`, `toggle_untracked`, `mark_reviewed`, `theme_select`, `filter`, `info`, `reload`, `quit`, `discard_quit`, `help`, `dismiss` Fixed modal keys (Enter, Esc in annotation/search input, confirm discard) are not remappable. Keymap-resolved actions like `open_editor` work during annotation input and can be rebound. Chord bindings do not fire during text input β€” use single-key `ctrl+*` bindings for actions that need to work during annotation input. diff --git a/site/docs.html b/site/docs.html index 4ebf1368..1ddcbe16 100644 --- a/site/docs.html +++ b/site/docs.html @@ -653,7 +653,7 @@

Chord bindings (ctrl/alt leader)

Available actions

Navigation: down, up, page_down, page_up, half_page_down, half_page_up, home, end, scroll_left, scroll_right, scroll_center, scroll_top, scroll_bottom, scroll_diff_down, scroll_diff_up

File/Hunk: next_item, prev_item, jump_file, next_hunk, prev_hunk, open_file_in_editor

-

Pane: toggle_pane, focus_tree, focus_diff

+

Pane: toggle_pane, focus_left, focus_right, focus_tree, focus_diff

Search: search

Annotations: confirm, annotate_file, delete_annotation, annot_list, open_editor, next_annotation, prev_annotation, flush_output

View: toggle_collapsed, toggle_compact, toggle_wrap, toggle_tree, toggle_line_numbers, toggle_blame, toggle_word_diff, toggle_hunk, toggle_untracked, mark_reviewed, theme_select, filter, info, reload

From fc216e408da771699cca58f570efec4d057088a1 Mon Sep 17 00:00:00 2001 From: Win Cheng Date: Wed, 22 Jul 2026 11:07:48 -0700 Subject: [PATCH 6/6] refactor(ui): clean up tree navigation and layout logic - refactor `resolveDirectionalFocus` to use a dynamic mapping of focus actions - consolidate tree pane X-range calculation into a helper method - unify `TestModel_FocusSwitching` using a table-driven approach - simplify tree positioning tests by explicitly asserting horizontal order --- app/keymap/keymap_test.go | 2 -- app/ui/model.go | 14 +++++------ app/ui/model_test.go | 51 ++++++++++++++++++--------------------- app/ui/mouse.go | 25 ++++++++++--------- app/ui/view_test.go | 34 ++++++++++++-------------- 5 files changed, 59 insertions(+), 67 deletions(-) diff --git a/app/keymap/keymap_test.go b/app/keymap/keymap_test.go index 80e57d24..24afab72 100644 --- a/app/keymap/keymap_test.go +++ b/app/keymap/keymap_test.go @@ -406,8 +406,6 @@ func TestIsValidAction(t *testing.T) { assert.True(t, IsValidAction(ActionDown)) assert.True(t, IsValidAction(ActionFocusLeft)) assert.True(t, IsValidAction(ActionFocusRight)) - assert.True(t, IsValidAction(ActionFocusTree)) - assert.True(t, IsValidAction(ActionFocusDiff)) assert.True(t, IsValidAction(ActionInfo)) assert.True(t, IsValidAction(Action("commit_info")), "deprecated alias must validate") assert.False(t, IsValidAction(Action("nonexistent"))) diff --git a/app/ui/model.go b/app/ui/model.go index 202dac07..9a5c751b 100644 --- a/app/ui/model.go +++ b/app/ui/model.go @@ -1073,17 +1073,15 @@ func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { // resolveDirectionalFocus maps spatial focus actions to semantic pane actions. // Explicit user bindings to focus_tree or focus_diff bypass this mapping. func (m Model) resolveDirectionalFocus(action keymap.Action) keymap.Action { + left, right := keymap.ActionFocusTree, keymap.ActionFocusDiff + if m.cfg.treePosition == TreePositionRight { + left, right = right, left + } switch action { case keymap.ActionFocusLeft: - if m.cfg.treePosition == TreePositionRight { - return keymap.ActionFocusDiff - } - return keymap.ActionFocusTree + return left case keymap.ActionFocusRight: - if m.cfg.treePosition == TreePositionRight { - return keymap.ActionFocusTree - } - return keymap.ActionFocusDiff + return right default: return action } diff --git a/app/ui/model_test.go b/app/ui/model_test.go index ec1b42c7..221335b8 100644 --- a/app/ui/model_test.go +++ b/app/ui/model_test.go @@ -501,36 +501,31 @@ func TestModel_TreeNavigation(t *testing.T) { } func TestModel_FocusSwitching(t *testing.T) { - m := testModel([]string{"a.go"}, nil) - m.tree = testNewFileTree([]string{"a.go"}) - m.file.name = "a.go" // pretend a file is loaded - m.layout.focus = paneTree - - // l switches to diff pane - result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) - model := result.(Model) - assert.Equal(t, paneDiff, model.layout.focus) - - // h switches back to tree - result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) - model = result.(Model) - assert.Equal(t, paneTree, model.layout.focus) -} - -func TestModel_FocusSwitching_RightTree(t *testing.T) { - m := testModel([]string{"a.go"}, nil) - m.tree = testNewFileTree([]string{"a.go"}) - m.cfg.treePosition = TreePositionRight - m.file.name = "a.go" - m.layout.focus = paneTree + for _, tc := range []struct { + name string + treePos TreePosition + toDiff rune // key that moves focus tree→diff + toTree rune // key that moves focus diff→tree + }{ + {name: "left tree", toDiff: 'l', toTree: 'h'}, + {name: "right tree", treePos: TreePositionRight, toDiff: 'h', toTree: 'l'}, + } { + t.Run(tc.name, func(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.tree = testNewFileTree([]string{"a.go"}) + m.cfg.treePosition = tc.treePos + m.file.name = "a.go" // pretend a file is loaded + m.layout.focus = paneTree - result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'h'}}) - model := result.(Model) - assert.Equal(t, paneDiff, model.layout.focus) + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toDiff}}) + model := result.(Model) + assert.Equal(t, paneDiff, model.layout.focus) - result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'l'}}) - model = result.(Model) - assert.Equal(t, paneTree, model.layout.focus) + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{tc.toTree}}) + model = result.(Model) + assert.Equal(t, paneTree, model.layout.focus) + }) + } } func TestModel_FocusSwitching_UserSemanticBinding(t *testing.T) { diff --git a/app/ui/mouse.go b/app/ui/mouse.go index b302f6cb..d5988c94 100644 --- a/app/ui/mouse.go +++ b/app/ui/mouse.go @@ -90,6 +90,16 @@ func (m Model) treeTopRow() int { return 1 } +// treePaneXRange returns the half-open screen column range [start, end) of the +// tree pane block: left border + treeWidth content columns + right border. +// the block hugs the right edge when the tree renders on the right. +func (m Model) treePaneXRange() (start, end int) { + if m.cfg.treePosition == TreePositionRight { + start = m.layout.width - m.layout.treeWidth - 2 + } + return start, start + m.layout.treeWidth + 2 +} + // hitTest classifies a screen coordinate into a hitZone for mouse-event routing. // the classification is pure arithmetic over m.layout state and does not // inspect any dynamic UI content. ordering matters: status bar is checked @@ -111,18 +121,11 @@ func (m Model) hitTest(x, y int) hitZone { return hitNone } - if !m.treePaneHidden() { - treeStart := 0 - if m.cfg.treePosition == TreePositionRight { - // pane width includes the content plus its left and right borders. - treeStart = m.layout.width - m.layout.treeWidth - 2 - } - if x >= treeStart && x < treeStart+m.layout.treeWidth+2 { - if y < m.treeTopRow() { - return hitNone - } - return hitTree + if start, end := m.treePaneXRange(); !m.treePaneHidden() && x >= start && x < end { + if y < m.treeTopRow() { + return hitNone } + return hitTree } if y == 0 { diff --git a/app/ui/view_test.go b/app/ui/view_test.go index 6c14aa41..bb28fe0f 100644 --- a/app/ui/view_test.go +++ b/app/ui/view_test.go @@ -608,9 +608,12 @@ func TestModel_ViewSingleFileMode(t *testing.T) { } func TestModel_ViewTreePosition(t *testing.T) { - assertOrder := func(t *testing.T, view, diffLabel, navigationLabel string, treePos TreePosition) { + // assertLeftOf makes each call site state the expected pane order explicitly + // instead of mirroring the production treePosition branch + assertLeftOf := func(t *testing.T, view, leftLabel, rightLabel string) { t.Helper() - labelColumn := func(lines []string, label string) int { + lines := strings.Split(ansi.Strip(view), "\n") + labelColumn := func(label string) int { for _, line := range lines { if idx := strings.Index(line, label); idx >= 0 { return idx @@ -619,24 +622,19 @@ func TestModel_ViewTreePosition(t *testing.T) { return -1 } - lines := strings.Split(ansi.Strip(view), "\n") - diffIdx := labelColumn(lines, diffLabel) - navigationIdx := labelColumn(lines, navigationLabel) - require.NotEqual(t, -1, diffIdx, "diff label must appear in the rendered view") - require.NotEqual(t, -1, navigationIdx, "navigation label must appear in the rendered view") - if treePos == TreePositionRight { - assert.Less(t, diffIdx, navigationIdx) - return - } - assert.Less(t, navigationIdx, diffIdx) + leftIdx, rightIdx := labelColumn(leftLabel), labelColumn(rightLabel) + require.NotEqual(t, -1, leftIdx, "label %q must appear in the rendered view", leftLabel) + require.NotEqual(t, -1, rightIdx, "label %q must appear in the rendered view", rightLabel) + assert.Less(t, leftIdx, rightIdx) } for _, tc := range []struct { - name string - treePos TreePosition + name string + treePos TreePosition + leftLabel, rightLabel string }{ - {name: "file tree on left"}, - {name: "file tree on right", treePos: TreePositionRight}, + {name: "file tree on left", leftLabel: "tree.go", rightLabel: "current.go"}, + {name: "file tree on right", treePos: TreePositionRight, leftLabel: "current.go", rightLabel: "tree.go"}, } { t.Run(tc.name, func(t *testing.T) { m := testModel([]string{"tree.go", "other.go"}, nil) @@ -645,7 +643,7 @@ func TestModel_ViewTreePosition(t *testing.T) { m.cfg.treePosition = tc.treePos m.cfg.noStatusBar = true - assertOrder(t, m.View(), "current.go", "tree.go", tc.treePos) + assertLeftOf(t, m.View(), tc.leftLabel, tc.rightLabel) }) } @@ -662,7 +660,7 @@ func TestModel_ViewTreePosition(t *testing.T) { m.cfg.treePosition = TreePositionRight m.cfg.noStatusBar = true - assertOrder(t, m.View(), "plan.md", "Navigation section", TreePositionRight) + assertLeftOf(t, m.View(), "plan.md", "Navigation section") }) }