From 0e3448140ffeb583224b998a9e4240cb5f0a1fbd Mon Sep 17 00:00:00 2001 From: yumosx Date: Mon, 28 Sep 2026 13:47:14 +0800 Subject: [PATCH] fix(diff): make review notes memory-only, drop .phi/review.json Notes now live only in the pane's memory and are dropped when the diff spec changes. Nothing is written to disk, so a later `a` can no longer re-send comments the agent already answered. --- CHANGELOG.md | 5 ++ README.md | 3 +- README.zh-CN.md | 2 +- doc/project-layout.md | 2 +- doc/tui.md | 2 +- internal/tui/diffpane/pane.go | 76 +++++++--------------- internal/tui/diffpane/pane_test.go | 35 +++++++++-- internal/util/diffreview/comment.go | 80 ++++-------------------- internal/util/diffreview/comment_test.go | 35 +---------- internal/util/diffreview/doc.go | 4 +- 10 files changed, 77 insertions(+), 167 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a4e1f4c..e2aea0a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- `diff` pane notes no longer persist. `i` / `x` / `a` keep working, but drafts + live in the overlay's memory instead of `.phi/review.json`, and switching + diffs drops them. The store was per project and outlived the session, so a + later `a` re-sent comments the agent had already answered. + - Compaction preserves the previous summary when the history bucket is empty, including mid-turn cuts that only summarize the current turn prefix. Inherited file-operation lists are refreshed once instead of accumulating duplicate blocks. diff --git a/README.md b/README.md index 5a549e0c..ac5dffa6 100644 --- a/README.md +++ b/README.md @@ -259,7 +259,8 @@ line notes, then hand them to the agent without leaving the terminal. Slash-picker Enter inserts `/diff` plus a trailing space into the composer; submit to open. Inside the overlay: `s` side-by-side, `i` add/edit a note, `x` delete, `a` send notes to the agent, -`?` help, `q` / `Esc` close. Notes persist under `.phi/review.json`. +`?` help, `q` / `Esc` close. Notes live in the overlay's memory only — switching +diffs drops them and nothing is written to disk. ## Code viewer diff --git a/README.zh-CN.md b/README.zh-CN.md index cfe39e31..d0e1eccd 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -251,7 +251,7 @@ permissions: 斜杠选择器里回车会把 `/diff` 连同一个空格填进输入框；再提交才打开。 审阅层内：`j`/`k` 移动，`s` 左右对照，`i` 添加/编辑批注，`x` 删除，`a` 发给代理， -`?` 帮助，`q` / `Esc` 关闭。批注保存在 `.phi/review.json`。 +`?` 帮助，`q` / `Esc` 关闭。批注只存在内存里，切 diff 就丢，不会落盘。 ## 代码查看器 diff --git a/doc/project-layout.md b/doc/project-layout.md index 9678dc9a..e091c38a 100644 --- a/doc/project-layout.md +++ b/doc/project-layout.md @@ -28,7 +28,7 @@ | `internal/tui/controller/` | Engine lifecycle, Bus/Msg, activity | | `internal/version/` | Build-time `Version` (splash / `phi update`) | | `internal/util/` | Shared helpers (diff, retry, SSE, file search, …) | -| `internal/util/diffreview/` | Unified-diff parse/render, review comments, git load | +| `internal/util/diffreview/` | Unified-diff parse/render, in-memory review notes, git load | | `internal/permission/` | Permission policy and ask gate | | `internal/extension/` | PXB extension discover/spawn/runner | | `internal/mcp/` | MCP config + stdio client + pool (meta-tool route) | diff --git a/doc/tui.md b/doc/tui.md index 20eba7d6..823455c3 100644 --- a/doc/tui.md +++ b/doc/tui.md @@ -58,7 +58,7 @@ internal/tui/ | `composer` | Keyboard routing for chat, `/` slash, `?` shortcuts, `@` mention, `!` shell completion, Ctrl+K palette | | `footer` | Composer status slot (activity ↔ tokens), bottom footer row (ext status, jobs, update hint) | | `overlays` | Modal permission / continue-ask panels; replaces composer when active | -| `diffpane` | Full-screen git diff review; comments persist under `.phi/review.json` | +| `diffpane` | Full-screen git diff review; notes stay in memory | | `codepane` | Full-screen source viewer: syntax highlight, caret, line selection | | `submit` | User submit path: agent prompt, slash commands, `!bash`, cancel | | `commands` | Slash/palette registry; session load/clear; extension command bridge | diff --git a/internal/tui/diffpane/pane.go b/internal/tui/diffpane/pane.go index 809bc325..fb5286f8 100644 --- a/internal/tui/diffpane/pane.go +++ b/internal/tui/diffpane/pane.go @@ -29,7 +29,6 @@ type Pane struct { rows []diffreview.Row drafts []diffreview.CommentDraft - commentPath string highlighted map[int][]components.Span cursor int @@ -53,7 +52,6 @@ type Pane struct { help bool status string - dirty bool picker listpicker.Picker @@ -103,14 +101,15 @@ func (p *Pane) OpenGit(cwd string, spec []string) { if cwd != "" { p.cwd = cwd } + keepNotes := slices.Equal(p.spec, spec) p.spec = append([]string(nil), spec...) p.label = diffreview.LabelForSpec(spec) text, err := diffreview.LoadGit(context.Background(), p.cwd, spec) if err != nil { - p.openParsed("", err.Error()) + p.openParsed("", err.Error(), keepNotes) return } - p.openParsed(text, "") + p.openParsed(text, "", keepNotes) } // OpenText shows a preloaded unified diff (tests / paste). @@ -118,12 +117,15 @@ func (p *Pane) OpenText(input string, spec []string) { if p == nil { return } + keepNotes := slices.Equal(p.spec, spec) p.spec = append([]string(nil), spec...) p.label = diffreview.LabelForSpec(spec) - p.openParsed(input, "") + p.openParsed(input, "", keepNotes) } -func (p *Pane) openParsed(input, loadErr string) { +// openParsed rebuilds rows for input. Notes are dropped unless keepNotes is set, +// so a note never outlives the diff it was written against. +func (p *Pane) openParsed(input, loadErr string, keepNotes bool) { p.active = true p.err = loadErr p.help = false @@ -132,11 +134,12 @@ func (p *Pane) openParsed(input, loadErr string) { p.picker.Hide() p.pendingG = false p.pendingBracket = 0 - p.commentPath = diffreview.CommentPath(p.cwd) + if !keepNotes { + p.drafts = nil + } if loadErr != "" { p.rows = nil - p.drafts = nil p.highlighted = nil p.cursor = 0 p.scroll = 0 @@ -151,13 +154,6 @@ func (p *Pane) openParsed(input, loadErr string) { return } p.rows = doc.Rows() - file, err := diffreview.LoadFile(p.commentPath) - if err != nil { - p.status = "comments: " + err.Error() - p.drafts = nil - } else { - p.drafts = file.Comments - } p.highlighted = diffview.HighlightRows(p.rows, p.theme) p.cursor = 0 p.scroll = 0 @@ -170,14 +166,11 @@ func (p *Pane) openParsed(input, loadErr string) { } } -// Close hides the overlay, saving comments first. +// Close hides the overlay. func (p *Pane) Close() { if p == nil { return } - if p.dirty { - _ = p.saveComments() - } p.active = false p.help = false p.searchMode = false @@ -457,34 +450,23 @@ func (p *Pane) submitComment() { p.commentEdit = false if body == "" { p.removeDraft(p.commentTarget) - p.dirty = true - if err := p.saveComments(); err != nil { - p.toast(err.Error()) - } + p.status = "note removed" return } - found := false for i, d := range p.drafts { if diffreview.SameTarget(d, p.commentTarget) || (d.ID != "" && d.ID == p.commentTarget.ID) { p.drafts[i].Body = body - found = true - break + p.status = "note updated" + return } } - if !found { - d := p.commentTarget - d.Body = body - if d.ID == "" { - d.ID = strconv.FormatInt(time.Now().UnixNano(), 36) - } - p.drafts = append(p.drafts, d) + d := p.commentTarget + d.Body = body + if d.ID == "" { + d.ID = strconv.FormatInt(time.Now().UnixNano(), 36) } - p.dirty = true - if err := p.saveComments(); err != nil { - p.toast(err.Error()) - return - } - p.status = "note saved" + p.drafts = append(p.drafts, d) + p.status = "note added" } func (p *Pane) deleteNote() { @@ -495,11 +477,6 @@ func (p *Pane) deleteNote() { return } p.removeDraft(drafts[0]) - p.dirty = true - if err := p.saveComments(); err != nil { - p.toast(err.Error()) - return - } p.status = "note deleted" } @@ -514,17 +491,6 @@ func (p *Pane) removeDraft(target diffreview.CommentDraft) { p.drafts = out } -func (p *Pane) saveComments() error { - if p.commentPath == "" { - return nil - } - err := diffreview.SaveFile(p.commentPath, diffreview.CommentFile{Version: 1, Comments: p.drafts}) - if err == nil { - p.dirty = false - } - return err -} - func (p *Pane) sendToAgent() { prompt := strings.TrimSpace(diffreview.FormatPrompt(p.drafts)) if prompt == "" { diff --git a/internal/tui/diffpane/pane_test.go b/internal/tui/diffpane/pane_test.go index b8ada327..e7dcfcc3 100644 --- a/internal/tui/diffpane/pane_test.go +++ b/internal/tui/diffpane/pane_test.go @@ -1,7 +1,6 @@ package diffpane import ( - "path/filepath" "testing" "github.com/pulseaiclub/xui" @@ -21,9 +20,8 @@ const sampleDiff = `diff --git a/main.go b/main.go ` func TestPaneOpenCommentSearchAndSend(t *testing.T) { - dir := t.TempDir() var submitted string - p := New(components.DefaultTheme(), dir, func(s string) { submitted = s }, nil, nil) + p := New(components.DefaultTheme(), t.TempDir(), func(s string) { submitted = s }, nil, nil) p.OpenText(sampleDiff, nil) require.True(t, p.Active()) require.NotEmpty(t, p.rows) @@ -47,7 +45,6 @@ func TestPaneOpenCommentSearchAndSend(t *testing.T) { p.Handle(ctx, xui.KeyEvent{Press: true, Code: xui.KeyEnter}) require.False(t, p.commentEdit) require.Len(t, p.drafts, 1) - assert.FileExists(t, filepath.Join(dir, ".phi", "review.json")) key('/') for _, r := range "new" { @@ -66,6 +63,36 @@ func TestPaneOpenCommentSearchAndSend(t *testing.T) { assert.Contains(t, submitted, "please rename") } +func TestPaneDropsNotesWhenDiffChanges(t *testing.T) { + p := New(components.DefaultTheme(), t.TempDir(), nil, nil, nil) + p.OpenText(sampleDiff, nil) + + ctx := &components.EventContext{} + key := func(r rune) { + p.Handle(ctx, xui.KeyEvent{Press: true, Code: xui.KeyRune, Rune: r}) + } + for i := 0; i < 20 && (p.cursor >= len(p.rows) || p.rows[p.cursor].Code != "new"); i++ { + key('j') + } + require.Equal(t, "new", p.rows[p.cursor].Code) + + key('i') + for _, r := range "stale note" { + key(r) + } + p.Handle(ctx, xui.KeyEvent{Press: true, Code: xui.KeyEnter}) + require.Len(t, p.drafts, 1) + + // Reopening the same spec keeps notes... + p.OpenText(sampleDiff, nil) + require.Len(t, p.drafts, 1) + + // ...but switching specs must not carry them over, or `a` would resend + // comments the agent already answered. + p.OpenText(sampleDiff, []string{"staged"}) + assert.Empty(t, p.drafts) +} + func TestPaneEscCloses(t *testing.T) { p := New(components.DefaultTheme(), t.TempDir(), nil, nil, nil) p.OpenText(sampleDiff, nil) diff --git a/internal/util/diffreview/comment.go b/internal/util/diffreview/comment.go index 5608bfbf..4119871b 100644 --- a/internal/util/diffreview/comment.go +++ b/internal/util/diffreview/comment.go @@ -1,18 +1,11 @@ package diffreview import ( - "encoding/json" - "errors" - "os" - "path/filepath" "sort" "strconv" "strings" ) -// DefaultFilePath is the project-local comment store. -const DefaultFilePath = ".phi/review.json" - // Side identifies which side of a unified diff a comment anchors to. type Side string @@ -23,70 +16,21 @@ const ( // Anchor locates one visible diff line for a comment. type Anchor struct { - Path string `json:"path"` - Line int `json:"line"` - Side Side `json:"side"` - CommitID string `json:"commit_id,omitempty"` + Path string + Line int + Side Side + CommitID string } -// CommentDraft is one review note on a single line. +// CommentDraft is one review note on a single line. Drafts live only in the +// pane's memory; nothing is written to disk. type CommentDraft struct { - ID string `json:"id,omitempty"` - Path string `json:"path"` - Body string `json:"body"` - CommitID string `json:"commit_id,omitempty"` - Line int `json:"line"` - Side Side `json:"side"` -} - -// CommentFile is the on-disk review store. -type CommentFile struct { - Version int `json:"version"` - Comments []CommentDraft `json:"comments"` -} - -// CommentPath returns cwd/.phi/review.json. -func CommentPath(cwd string) string { - if cwd == "" { - return DefaultFilePath - } - return filepath.Join(cwd, DefaultFilePath) -} - -// LoadFile reads comments; a missing file is an empty v1 store. -func LoadFile(path string) (CommentFile, error) { - data, err := os.ReadFile(path) - if errors.Is(err, os.ErrNotExist) { - return CommentFile{Version: 1}, nil - } - if err != nil { - return CommentFile{}, err - } - - var file CommentFile - if err := json.Unmarshal(data, &file); err != nil { - return CommentFile{}, err - } - if file.Version == 0 { - file.Version = 1 - } - return file, nil -} - -// SaveFile writes comments as indented JSON. -func SaveFile(path string, file CommentFile) error { - if file.Version == 0 { - file.Version = 1 - } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err - } - data, err := json.MarshalIndent(file, "", " ") - if err != nil { - return err - } - data = append(data, '\n') - return os.WriteFile(path, data, 0o644) //nolint:gosec // G306: review notes are meant to be user-readable + ID string + Path string + Body string + CommitID string + Line int + Side Side } // CommentIndex maps visible drafts to the rows that render them. diff --git a/internal/util/diffreview/comment_test.go b/internal/util/diffreview/comment_test.go index 463a19cf..d31fc264 100644 --- a/internal/util/diffreview/comment_test.go +++ b/internal/util/diffreview/comment_test.go @@ -1,41 +1,12 @@ package diffreview import ( - "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestSaveLoadFile(t *testing.T) { - path := filepath.Join(t.TempDir(), ".phi", "review.json") - file := CommentFile{ - Version: 1, - Comments: []CommentDraft{{ - Path: "tui/app.go", - Body: "comment", - Line: 10, - Side: SideRight, - }}, - } - - require.NoError(t, SaveFile(path, file)) - got, err := LoadFile(path) - require.NoError(t, err) - require.Len(t, got.Comments, 1) - assert.Equal(t, 1, got.Version) - assert.Equal(t, "comment", got.Comments[0].Body) - assert.Equal(t, 10, got.Comments[0].Line) -} - -func TestLoadFileMissingReturnsEmptyFile(t *testing.T) { - file, err := LoadFile(filepath.Join(t.TempDir(), ".phi", "review.json")) - require.NoError(t, err) - assert.Equal(t, 1, file.Version) - assert.Empty(t, file.Comments) -} - func TestBuildCommentIndexResolvesSingleLineComment(t *testing.T) { rows := []Row{ {Kind: RowAdd, Code: "one", Review: Anchor{Path: "main.go", Line: 1, Side: SideRight}}, @@ -69,6 +40,7 @@ func TestBuildCommentIndexSkipsUnmatchedComments(t *testing.T) { {Path: "other.go", Line: 1, Side: SideRight, Body: "wrong path"}, {Path: "main.go", Line: 2, Side: SideRight, Body: "wrong line"}, } + idx := BuildCommentIndex(rows, drafts) assert.Empty(t, idx.TargetRows()) } @@ -110,8 +82,3 @@ func TestEmptyNote(t *testing.T) { assert.Contains(t, EmptyNote([]string{"HEAD~2"}), "No changes vs HEAD~2") assert.Contains(t, EmptyNote([]string{"HEAD~2"}), "Untracked files are not shown") } - -func TestCommentPath(t *testing.T) { - assert.Equal(t, DefaultFilePath, CommentPath("")) - assert.Equal(t, filepath.Join("/tmp/proj", DefaultFilePath), CommentPath("/tmp/proj")) -} diff --git a/internal/util/diffreview/doc.go b/internal/util/diffreview/doc.go index 0e48b2bf..d7429212 100644 --- a/internal/util/diffreview/doc.go +++ b/internal/util/diffreview/doc.go @@ -1,3 +1,3 @@ -// Package diffreview parses unified diffs, renders review rows, and stores -// line comments under .phi/review.json. +// Package diffreview parses unified diffs, renders review rows, and holds the +// pane's in-memory line notes. Notes are never written to disk. package diffreview