From b07c132e337f557c269b5d7acf3b9b7cdf0905a2 Mon Sep 17 00:00:00 2001 From: Umputun Date: Wed, 1 Apr 2026 21:08:25 -0500 Subject: [PATCH 1/2] feat: add discard-quit confirmation flow with Q keypress add Q hotkey to discard all annotations and quit without output. when annotations exist, show inline confirmation in status bar. confirmation is skippable via --no-confirm-discard flag. --- .../skills/revdiff/references/config.md | 1 + .../skills/revdiff/references/usage.md | 1 + README.md | 2 + cmd/revdiff/main.go | 37 +-- cmd/revdiff/main_test.go | 32 ++- docs/plans/completed/20260401-discard-quit.md | 124 ++++++++++ ui/model.go | 82 +++++-- ui/model_test.go | 223 ++++++++++++++++++ 8 files changed, 463 insertions(+), 39 deletions(-) create mode 100644 docs/plans/completed/20260401-discard-quit.md diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index 6b6dce74..275510b9 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -23,6 +23,7 @@ Then uncomment and edit the values you want to change. | `--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` | +| `--no-confirm-discard` | `REVDIFF_NO_CONFIRM_DISCARD` | Skip confirmation when discarding annotations with Q | `false` | | `--chroma-style` | `REVDIFF_CHROMA_STYLE` | Chroma color theme for syntax highlighting | `monokai` | | `-o`, `--output` | `REVDIFF_OUTPUT` | Write annotations to file instead of stdout | | | `--config` | `REVDIFF_CONFIG` | Path to config file | `~/.config/revdiff/config` | diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 2eb2c20d..1718ba9d 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -45,6 +45,7 @@ revdiff HEAD~1 # review last commit |-----|--------| | `f` | Toggle filter: all files / annotated only | | `q` | Quit, output annotations to stdout | +| `Q` | Discard all annotations and quit (confirms if annotations exist) | ## Output Format diff --git a/README.md b/README.md index 091a1cda..8b4c980a 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,7 @@ revdiff [OPTIONS] [ref] | `--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` | +| `--no-confirm-discard` | Skip confirmation when discarding annotations with Q, env: `REVDIFF_NO_CONFIRM_DISCARD` | `false` | | `--chroma-style` | Chroma color theme for syntax highlighting, env: `REVDIFF_CHROMA_STYLE` | `monokai` | | `-o`, `--output` | Write annotations to file instead of stdout, env: `REVDIFF_OUTPUT` | | | `--config` | Path to config file, env: `REVDIFF_CONFIG` | `~/.config/revdiff/config` | @@ -228,6 +229,7 @@ revdiff HEAD~1 |-----|--------| | `f` | Toggle filter: all files / annotated only (shown when annotations exist) | | `q` | Quit, output annotations to stdout | +| `Q` | Discard all annotations and quit (confirms if annotations exist) | ### Output Format diff --git a/cmd/revdiff/main.go b/cmd/revdiff/main.go index 11cec213..0827e44f 100644 --- a/cmd/revdiff/main.go +++ b/cmd/revdiff/main.go @@ -24,16 +24,17 @@ type options struct { Ref string `positional-arg-name:"ref" description:"git ref to diff against (default: uncommitted changes)"` } `positional-args:"yes"` - Staged bool `long:"staged" ini-name:"staged" env:"REVDIFF_STAGED" description:"show staged changes"` - 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)"` - 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"` - ChromaStyle string `long:"chroma-style" ini-name:"chroma-style" env:"REVDIFF_CHROMA_STYLE" default:"monokai" description:"chroma style for syntax highlighting"` - Output string `long:"output" short:"o" env:"REVDIFF_OUTPUT" no-ini:"true" description:"write annotations to file instead of stdout"` - Config string `long:"config" env:"REVDIFF_CONFIG" no-ini:"true" description:"path to config file"` - DumpConfig bool `long:"dump-config" no-ini:"true" description:"print default config to stdout and exit"` - Version bool `short:"V" long:"version" no-ini:"true" description:"show version info"` + Staged bool `long:"staged" ini-name:"staged" env:"REVDIFF_STAGED" description:"show staged changes"` + 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)"` + 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"` + NoConfirmDiscard bool `long:"no-confirm-discard" ini-name:"no-confirm-discard" env:"REVDIFF_NO_CONFIRM_DISCARD" description:"skip confirmation prompt when discarding annotations with Q"` + ChromaStyle string `long:"chroma-style" ini-name:"chroma-style" env:"REVDIFF_CHROMA_STYLE" default:"monokai" description:"chroma style for syntax highlighting"` + Output string `long:"output" short:"o" env:"REVDIFF_OUTPUT" no-ini:"true" description:"write annotations to file instead of stdout"` + Config string `long:"config" env:"REVDIFF_CONFIG" no-ini:"true" description:"path to config file"` + DumpConfig bool `long:"dump-config" no-ini:"true" description:"print default config to stdout and exit"` + Version bool `short:"V" long:"version" no-ini:"true" description:"show version info"` Colors struct { Accent string `long:"color-accent" ini-name:"color-accent" env:"REVDIFF_COLOR_ACCENT" default:"#5f87ff" description:"active pane borders and directory names"` @@ -171,12 +172,13 @@ func run(opts options) error { store := annotation.NewStore() hl := highlight.New(opts.ChromaStyle, !opts.NoColors) model := ui.NewModel(renderer, store, hl, ui.ModelConfig{ - NoColors: opts.NoColors, - NoStatusBar: opts.NoStatusBar, - TabWidth: opts.TabWidth, - Ref: opts.Ref.Ref, - Staged: opts.Staged, - TreeWidthRatio: opts.TreeWidth, + NoColors: opts.NoColors, + NoStatusBar: opts.NoStatusBar, + NoConfirmDiscard: opts.NoConfirmDiscard, + TabWidth: opts.TabWidth, + Ref: opts.Ref.Ref, + Staged: opts.Staged, + TreeWidthRatio: opts.TreeWidth, Colors: ui.Colors{ Accent: opts.Colors.Accent, Border: opts.Colors.Border, @@ -209,6 +211,9 @@ func run(opts options) error { if !ok { return nil } + if m.Discarded() { + return nil + } output := m.Store().FormatOutput() if output == "" { return nil diff --git a/cmd/revdiff/main_test.go b/cmd/revdiff/main_test.go index eef51d24..ddc33666 100644 --- a/cmd/revdiff/main_test.go +++ b/cmd/revdiff/main_test.go @@ -11,11 +11,9 @@ import ( // noConfigArgs returns args that point to a nonexistent config file, // isolating the test from user's real config. -func noConfigArgs(t *testing.T, extra ...string) []string { +func noConfigArgs(t *testing.T) []string { t.Helper() - args := make([]string, 0, 2+len(extra)) - args = append(args, "--config", filepath.Join(t.TempDir(), "none")) - return append(args, extra...) + return []string{"--config", filepath.Join(t.TempDir(), "none")} } func TestParseArgs_Defaults(t *testing.T) { @@ -27,10 +25,36 @@ func TestParseArgs_Defaults(t *testing.T) { assert.False(t, opts.Staged) assert.False(t, opts.NoColors) assert.False(t, opts.NoStatusBar) + assert.False(t, opts.NoConfirmDiscard) assert.Empty(t, opts.Output) assert.Empty(t, opts.Ref.Ref) } +func TestParseArgs_NoConfirmDiscard(t *testing.T) { + t.Run("flag", func(t *testing.T) { + opts, err := parseArgs([]string{"--no-confirm-discard"}) + require.NoError(t, err) + assert.True(t, opts.NoConfirmDiscard) + }) + + t.Run("env", func(t *testing.T) { + t.Setenv("REVDIFF_NO_CONFIRM_DISCARD", "true") + opts, err := parseArgs(noConfigArgs(t)) + require.NoError(t, err) + assert.True(t, opts.NoConfirmDiscard) + }) + + t.Run("config file", func(t *testing.T) { + cfgDir := t.TempDir() + cfgPath := filepath.Join(cfgDir, "config") + err := os.WriteFile(cfgPath, []byte("[Application Options]\nno-confirm-discard = true\n"), 0o600) + require.NoError(t, err) + opts, err := parseArgs([]string{"--config", cfgPath}) + require.NoError(t, err) + assert.True(t, opts.NoConfirmDiscard) + }) +} + func TestParseArgs_OutputFlag(t *testing.T) { opts, err := parseArgs([]string{"-o", "/tmp/out.txt"}) require.NoError(t, err) diff --git a/docs/plans/completed/20260401-discard-quit.md b/docs/plans/completed/20260401-discard-quit.md new file mode 100644 index 00000000..0d04753b --- /dev/null +++ b/docs/plans/completed/20260401-discard-quit.md @@ -0,0 +1,124 @@ +# Discard and Quit (Q hotkey) + +## Overview +- Add a "discard and quit" hotkey (`Q`) that exits revdiff without outputting any annotations +- When annotations exist, show an inline confirmation prompt in the status bar before discarding +- When no annotations exist, `Q` behaves identically to `q` (just exits, nothing to discard) +- The confirmation prompt is suppressible via `--no-confirm-discard` CLI flag / env / config +- Solves the problem of accidentally sending annotations back to the calling process when the user just wants to exit + +## Context (from discovery) +- Key files: `ui/model.go` (Model struct, handleKey, statusBarText), `cmd/revdiff/main.go` (options, run) +- `Store.Count()` already exists for checking annotation count +- Existing accessor pattern: `Store()` returns private field, same pattern for `Discarded()` +- Status bar already has context-sensitive text via `statusBarText()` +- Existing `--no-*` flag pattern: `--no-colors`, `--no-status-bar` + +## Solution Overview +- `Q` (shift+q) quits without annotations; `q` continues to quit with annotations +- If annotations exist and `--no-confirm-discard` is not set, status bar shows `"discard N annotations? [y/n]"` +- `y` or second `Q` confirms discard, `n`/`Esc` cancels back to normal mode +- If no annotations or `--no-confirm-discard` is set, `Q` quits immediately +- `main.go` checks `m.Discarded()` before calling `FormatOutput()` + +## Technical Details +- New Model fields: `confirmingDiscard bool`, `discarded bool`, `noConfirmDiscard bool` +- New ModelConfig field: `NoConfirmDiscard bool` +- New CLI option: `--no-confirm-discard` (env: `REVDIFF_NO_CONFIRM_DISCARD`, ini: `no-confirm-discard`) +- New accessor: `func (m Model) Discarded() bool` +- Status bar in confirming state: `"discard N annotations? [y/n]"` (replaces normal hints) +- `Q` during annotation input mode is ignored (same as other navigation keys) + +## Development Approach +- **testing approach**: Regular (code first, then tests) +- complete each task fully before moving to the next +- make small, focused changes +- **CRITICAL: every task MUST include new/updated tests** for code changes in that task +- **CRITICAL: all tests must pass before starting next task** +- **CRITICAL: update this plan file when scope changes during implementation** +- run tests after each change +- maintain backward compatibility + +## Testing Strategy +- **unit tests**: required for every task +- Test Q with no annotations (immediate quit, discarded=true) +- Test Q with annotations and confirmation (y confirms, n/Esc cancels) +- Test Q with annotations and noConfirmDiscard (immediate quit) +- Test Q during annotation input (ignored) +- Test Discarded() accessor +- Test main.go skips output when discarded +- Test status bar text during confirmation + +## Progress Tracking +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with ➕ prefix +- document issues/blockers with ⚠️ prefix +- update plan if implementation deviates from original scope + +## Implementation Steps + +### Task 1: Add discard state to Model and CLI option + +**Files:** +- Modify: `ui/model.go` +- Modify: `cmd/revdiff/main.go` + +- [x] add `discarded bool`, `noConfirmDiscard bool` fields to Model struct (`confirmingDiscard` deferred to Task 2 to avoid unused-field lint error) +- [x] add `NoConfirmDiscard bool` to ModelConfig +- [x] add `Discarded() bool` accessor method on Model +- [x] wire `noConfirmDiscard` in NewModel from ModelConfig +- [x] add `NoConfirmDiscard` option to `options` struct in main.go (`--no-confirm-discard`, env `REVDIFF_NO_CONFIRM_DISCARD`, ini `no-confirm-discard`) +- [x] pass `NoConfirmDiscard` from options to ModelConfig in `run()` +- [x] write tests in `ui/model_test.go` for Discarded() accessor (default false, set true) +- [x] write test in `cmd/revdiff/main_test.go` for `--no-confirm-discard` flag parsing +- [x] run `go test ./...` - must pass before task 2 + +### Task 2: Handle Q keypress and confirmation flow + +**Files:** +- Modify: `ui/model.go` + +- [x] add `Q` case in handleKey: if no annotations or noConfirmDiscard, set `discarded=true` and return `tea.Quit` +- [x] if annotations exist and confirm required, set `confirmingDiscard=true` and return +- [x] ignore `Q` when `m.annotating` is true (annotation input mode) +- [x] add confirmation key handling: when `confirmingDiscard` is true, `y` or `Q` sets `discarded=true` and returns `tea.Quit`, `n`/`Esc` sets `confirmingDiscard=false` +- [x] block other keys while `confirmingDiscard` is true (only y/Q/n/Esc accepted); confirmation blocking applies only in `handleKey`, non-key messages (WindowSizeMsg etc.) are handled normally +- [x] write tests for Q with no annotations (immediate quit, discarded=true) +- [x] write tests for Q with annotations (enters confirming state) +- [x] write tests for y during confirmation (quits with discarded=true) +- [x] write tests for n and Esc during confirmation (cancels back to normal) +- [x] write test for second Q during confirmation (confirms discard) +- [x] write test for Q during annotation input (ignored) +- [x] write test for Q with noConfirmDiscard and annotations (immediate quit) +- [x] run `go test ./...` - must pass before task 3 + +### Task 3: Update status bar and output handling + +**Files:** +- Modify: `ui/model.go` +- Modify: `cmd/revdiff/main.go` + +- [x] update `statusBarText()` to show `"discard N annotations? [y/n]"` when `confirmingDiscard` is true +- [x] add `[Q] discard` hint to normal status bar text (both tree and diff pane hints) +- [x] in main.go `run()`, check `m.Discarded()` before `FormatOutput()` — if true, return nil (skip output) +- [x] write test for status bar text during confirmation state +- [x] write test for status bar showing Q hint in normal mode +- [x] write test verifying main.go skips output when discarded (if feasible with existing test patterns) +- [x] run `go test ./...` - must pass before task 4 + +### Task 4: Verify acceptance criteria +- [x] verify Q with no annotations exits silently (no output) +- [x] verify Q with annotations shows confirmation, y discards, n cancels +- [x] verify --no-confirm-discard skips prompt +- [x] verify q still works as before (outputs annotations) +- [x] verify Q is ignored during annotation text input +- [x] run full test suite: `go test ./...` +- [x] run linter: `golangci-lint run` +- [x] run formatters: `~/.claude/format.sh` + +### Task 5: [Final] Update documentation +- [x] update README.md key bindings table (add Q) +- [x] update README.md options table (add --no-confirm-discard) +- [x] update CLAUDE.md if needed +- [x] update plugin reference docs (`.claude-plugin/skills/revdiff/references/usage.md`, `config.md`) +- [x] move this plan to `docs/plans/completed/` diff --git a/ui/model.go b/ui/model.go index 2f56f544..8913513c 100644 --- a/ui/model.go +++ b/ui/model.go @@ -67,6 +67,10 @@ type Model struct { fileAnnotating bool // true when annotating at file level (Line=0) cursorOnAnnotation bool // true when cursor is on the annotation sub-line (not the diff line) annotateInput textinput.Model // text input for annotations + + discarded bool // true when user chose to discard annotations and quit + inConfirmDiscard bool // true when showing discard confirmation prompt + noConfirmDiscard bool // skip confirmation prompt on discard quit } // fileLoadedMsg is sent when a file's diff has been loaded. @@ -85,13 +89,14 @@ type filesLoadedMsg struct { // ModelConfig holds configuration options for NewModel. type ModelConfig struct { - Ref string - Staged bool - TreeWidthRatio int - TabWidth int // number of spaces per tab character - NoColors bool // disable all colors including syntax highlighting - NoStatusBar bool // hide the status bar - Colors Colors + Ref string + Staged bool + TreeWidthRatio int + TabWidth int // number of spaces per tab character + NoColors bool // disable all colors including syntax highlighting + NoStatusBar bool // hide the status bar + NoConfirmDiscard bool // skip confirmation prompt when discarding annotations + Colors Colors } // NewModel creates a new Model with the given renderer, store, highlighter and configuration. @@ -107,16 +112,17 @@ func NewModel(renderer Renderer, store *annotation.Store, highlighter SyntaxHigh s = plainStyles() } return Model{ - styles: s, - store: store, - renderer: renderer, - highlighter: highlighter, - ref: cfg.Ref, - staged: cfg.Staged, - noStatusBar: cfg.NoStatusBar, - focus: paneTree, - treeWidthRatio: cfg.TreeWidthRatio, - tabSpaces: strings.Repeat(" ", cfg.TabWidth), + styles: s, + store: store, + renderer: renderer, + highlighter: highlighter, + ref: cfg.Ref, + staged: cfg.Staged, + noStatusBar: cfg.NoStatusBar, + noConfirmDiscard: cfg.NoConfirmDiscard, + focus: paneTree, + treeWidthRatio: cfg.TreeWidthRatio, + tabSpaces: strings.Repeat(" ", cfg.TabWidth), } } @@ -125,6 +131,11 @@ func (m Model) Store() *annotation.Store { return m.store } +// Discarded returns true when the user chose to discard annotations and quit. +func (m Model) Discarded() bool { + return m.discarded +} + // Init initializes the model by loading changed files. func (m Model) Init() tea.Cmd { return m.loadFiles() @@ -149,6 +160,9 @@ func (m Model) loadFileDiff(file string) tea.Cmd { func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.KeyMsg: + if m.inConfirmDiscard { + return m.handleConfirmDiscardKey(msg) + } return m.handleKey(msg) case tea.WindowSizeMsg: return m.handleResize(msg) @@ -176,6 +190,9 @@ func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } switch { + case msg.String() == "Q": + return m.handleDiscardQuit() + case msg.String() == "q": return m, tea.Quit @@ -451,6 +468,10 @@ func (m Model) View() string { // statusBarText returns context-sensitive status bar hints. func (m Model) statusBarText(annotated map[string]bool) string { + if m.inConfirmDiscard { + return fmt.Sprintf("discard %d annotations? [y/n]", m.store.Count()) + } + if m.annotating { return "[enter] save [esc] cancel" } @@ -473,7 +494,7 @@ func (m Model) statusBarText(annotated map[string]bool) string { var hints string switch m.focus { case paneTree: - hints = "[j/k] navigate [enter] select [l/tab] diff" + filterHint + " [n/p] next/prev [q] quit" + hints = "[j/k] navigate [enter] select [l/tab] diff" + filterHint + " [n/p] next/prev [Q] discard [q] quit" case paneDiff: deleteHint := "" if m.cursorLineHasAnnotation() { @@ -483,7 +504,7 @@ func (m Model) statusBarText(annotated map[string]bool) string { if cur, total := m.currentHunk(); total > 0 { hunkHint = fmt.Sprintf(" [ ] hunk %d/%d", cur, total) } - hints = "[j/k] scroll [h/tab] files [enter/a] annotate" + deleteHint + hunkHint + filterHint + fileNoteHint + " [n/p] next/prev [q] quit" + hints = "[j/k] scroll [h/tab] files [enter/a] annotate" + deleteHint + hunkHint + filterHint + fileNoteHint + " [n/p] next/prev [Q] discard [q] quit" } if countHint != "" { @@ -498,6 +519,29 @@ func (m Model) statusBarText(annotated map[string]bool) string { return hints } +// handleDiscardQuit handles the Q key press for discard-and-quit. +func (m Model) handleDiscardQuit() (tea.Model, tea.Cmd) { + if m.store.Count() == 0 || m.noConfirmDiscard || m.noStatusBar { + m.discarded = true + return m, tea.Quit + } + m.inConfirmDiscard = true + return m, nil +} + +// handleConfirmDiscardKey handles keys during discard confirmation prompt. +func (m Model) handleConfirmDiscardKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "y", "Q": + m.discarded = true + return m, tea.Quit + case "n", "esc": + m.inConfirmDiscard = false + return m, nil + } + return m, nil +} + // annotatedFiles returns a set of files that have annotations. func (m Model) annotatedFiles() map[string]bool { result := make(map[string]bool) diff --git a/ui/model_test.go b/ui/model_test.go index c1e0df20..f7d2122d 100644 --- a/ui/model_test.go +++ b/ui/model_test.go @@ -2670,3 +2670,226 @@ func TestModel_ViewNoStatusBar(t *testing.T) { assert.NotContains(t, view, "quit", "status bar should be hidden") assert.Contains(t, view, "a.go", "tree content should still appear") } + +func TestModel_DiscardedAccessor(t *testing.T) { + t.Run("default is false", func(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + assert.False(t, m.Discarded()) + }) + + t.Run("true when set", func(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.discarded = true + assert.True(t, m.Discarded()) + }) +} + +func TestModel_NoConfirmDiscardWired(t *testing.T) { + renderer := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]string, error) { return nil, nil }, + FileDiffFunc: func(string, string, bool) ([]diff.DiffLine, error) { return nil, nil }, + } + store := annotation.NewStore() + m := NewModel(renderer, store, noopHighlighter(), ModelConfig{NoConfirmDiscard: true, TreeWidthRatio: 3}) + assert.True(t, m.noConfirmDiscard, "noConfirmDiscard should be wired from ModelConfig") +} + +func TestModel_QKeyDiscardNoAnnotations(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + require.NotNil(t, cmd) + + model := result.(Model) + assert.True(t, model.Discarded(), "should be discarded when no annotations") + msg := cmd() + _, ok := msg.(tea.QuitMsg) + assert.True(t, ok, "should quit") +} + +func TestModel_QKeyWithAnnotationsEntersConfirming(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + assert.Nil(t, cmd, "should not quit yet") + + model := result.(Model) + assert.True(t, model.inConfirmDiscard, "should enter confirming state") + assert.False(t, model.Discarded(), "should not be discarded yet") +} + +func TestModel_ConfirmDiscardY(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'y'}}) + require.NotNil(t, cmd) + + model := result.(Model) + assert.True(t, model.Discarded(), "y should confirm discard") + msg := cmd() + _, ok := msg.(tea.QuitMsg) + assert.True(t, ok, "should quit after y") +} + +func TestModel_ConfirmDiscardN(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'n'}}) + assert.Nil(t, cmd, "n should not quit") + + model := result.(Model) + assert.False(t, model.inConfirmDiscard, "n should cancel confirmation") + assert.False(t, model.Discarded(), "should not be discarded") +} + +func TestModel_ConfirmDiscardEsc(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEscape}) + assert.Nil(t, cmd, "esc should not quit") + + model := result.(Model) + assert.False(t, model.inConfirmDiscard, "esc should cancel confirmation") + assert.False(t, model.Discarded()) +} + +func TestModel_ConfirmDiscardSecondQ(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + require.NotNil(t, cmd) + + model := result.(Model) + assert.True(t, model.Discarded(), "second Q should confirm discard") + msg := cmd() + _, ok := msg.(tea.QuitMsg) + assert.True(t, ok, "should quit after second Q") +} + +func TestModel_QKeyDuringAnnotationIgnored(t *testing.T) { + lines := []diff.DiffLine{{NewNum: 1, Content: "line1", ChangeType: diff.ChangeAdd}} + m := testModel([]string{"a.go"}, map[string][]diff.DiffLine{"a.go": lines}) + + // load file + result, _ := m.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + model := result.(Model) + result, _ = model.Update(fileLoadedMsg{file: "a.go", lines: lines}) + model = result.(Model) + model.focus = paneDiff + model.diffCursor = 0 + + // enter annotation mode + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'a'}}) + model = result.(Model) + require.True(t, model.annotating) + + // press Q - should be handled as text input, not discard + result, _ = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + model = result.(Model) + assert.True(t, model.annotating, "should still be annotating") + assert.False(t, model.Discarded(), "should not be discarded") + assert.False(t, model.inConfirmDiscard, "should not enter confirming") + assert.Contains(t, model.annotateInput.Value(), "Q", "Q should be typed into input") +} + +func TestModel_QKeyNoConfirmDiscardWithAnnotations(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.noConfirmDiscard = true + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + require.NotNil(t, cmd) + + model := result.(Model) + assert.True(t, model.Discarded(), "should immediately discard with noConfirmDiscard") + assert.False(t, model.inConfirmDiscard, "should not enter confirming state") + msg := cmd() + _, ok := msg.(tea.QuitMsg) + assert.True(t, ok, "should quit immediately") +} + +func TestModel_QKeyNoStatusBarSkipsConfirmation(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.noStatusBar = true + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}) + require.NotNil(t, cmd) + + model := result.(Model) + assert.True(t, model.Discarded(), "should immediately discard when status bar is hidden") + assert.False(t, model.inConfirmDiscard, "should not enter confirming state without status bar") + msg := cmd() + _, ok := msg.(tea.QuitMsg) + assert.True(t, ok, "should quit immediately") +} + +func TestModel_ConfirmDiscardBlocksOtherKeys(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + // pressing j (navigation) should be blocked + result, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'j'}}) + assert.Nil(t, cmd, "j should be blocked during confirmation") + model := result.(Model) + assert.True(t, model.inConfirmDiscard, "should still be confirming") + + // pressing q should be blocked too + result, cmd = model.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) + assert.Nil(t, cmd, "q should be blocked during confirmation") + model = result.(Model) + assert.True(t, model.inConfirmDiscard, "should still be confirming") +} + +func TestModel_ConfirmDiscardAllowsNonKeyMessages(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "test"}) + m.inConfirmDiscard = true + + // WindowSizeMsg should still be handled + result, _ := m.Update(tea.WindowSizeMsg{Width: 100, Height: 30}) + model := result.(Model) + assert.Equal(t, 100, model.width, "resize should be handled during confirmation") + assert.True(t, model.inConfirmDiscard, "should still be confirming after resize") +} + +func TestModel_StatusBarDiscardConfirmation(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.width = 120 + m.store.Add(annotation.Annotation{File: "a.go", Line: 1, Type: "+", Comment: "note"}) + m.store.Add(annotation.Annotation{File: "b.go", Line: 5, Type: " ", Comment: "other"}) + m.inConfirmDiscard = true + + annotated := m.annotatedFiles() + status := m.statusBarText(annotated) + assert.Equal(t, "discard 2 annotations? [y/n]", status) +} + +func TestModel_StatusBarShowsDiscardHint(t *testing.T) { + m := testModel([]string{"a.go"}, nil) + m.width = 120 + + t.Run("tree pane", func(t *testing.T) { + m.focus = paneTree + status := m.statusBarText(m.annotatedFiles()) + assert.Contains(t, status, "[Q] discard") + assert.Contains(t, status, "[q] quit") + }) + + t.Run("diff pane", func(t *testing.T) { + m.focus = paneDiff + status := m.statusBarText(m.annotatedFiles()) + assert.Contains(t, status, "[Q] discard") + assert.Contains(t, status, "[q] quit") + }) +} From 58a602c9f938c619bc84b0c5f9b5bc4c8224f4a4 Mon Sep 17 00:00:00 2001 From: Umputun Date: Wed, 1 Apr 2026 21:34:16 -0500 Subject: [PATCH 2/2] fix: use noConfigArgs in flag parsing test to isolate from user config --- cmd/revdiff/main_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/revdiff/main_test.go b/cmd/revdiff/main_test.go index ddc33666..63e8335a 100644 --- a/cmd/revdiff/main_test.go +++ b/cmd/revdiff/main_test.go @@ -32,7 +32,7 @@ func TestParseArgs_Defaults(t *testing.T) { func TestParseArgs_NoConfirmDiscard(t *testing.T) { t.Run("flag", func(t *testing.T) { - opts, err := parseArgs([]string{"--no-confirm-discard"}) + opts, err := parseArgs(append(noConfigArgs(t), "--no-confirm-discard")) require.NoError(t, err) assert.True(t, opts.NoConfirmDiscard) })