Skip to content

Commit 96b827c

Browse files
committed
fix: address round 2 review findings
1 parent 5238590 commit 96b827c

12 files changed

Lines changed: 218 additions & 93 deletions

File tree

go.mod

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,24 @@ module github.com/Astro-Han/diffpane
22

33
go 1.26.1
44

5+
require (
6+
github.com/charmbracelet/bubbletea v1.3.10
7+
github.com/charmbracelet/lipgloss v1.1.0
8+
github.com/fsnotify/fsnotify v1.9.0
9+
github.com/mattn/go-runewidth v0.0.21
10+
)
11+
512
require (
613
github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect
7-
github.com/charmbracelet/bubbletea v1.3.10 // indirect
814
github.com/charmbracelet/colorprofile v0.2.3-0.20250311203215-f60798e515dc // indirect
9-
github.com/charmbracelet/lipgloss v1.1.0 // indirect
1015
github.com/charmbracelet/x/ansi v0.10.1 // indirect
1116
github.com/charmbracelet/x/cellbuf v0.0.13-0.20250311204145-2c3ea96c31dd // indirect
1217
github.com/charmbracelet/x/term v0.2.1 // indirect
1318
github.com/clipperhouse/uax29/v2 v2.2.0 // indirect
1419
github.com/erikgeiser/coninput v0.0.0-20211004153227-1c3628e74d0f // indirect
15-
github.com/fsnotify/fsnotify v1.9.0 // indirect
1620
github.com/lucasb-eyer/go-colorful v1.2.0 // indirect
1721
github.com/mattn/go-isatty v0.0.20 // indirect
1822
github.com/mattn/go-localereader v0.0.1 // indirect
19-
github.com/mattn/go-runewidth v0.0.21 // indirect
2023
github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect
2124
github.com/muesli/cancelreader v0.2.2 // indirect
2225
github.com/muesli/termenv v0.16.0 // indirect

go.sum

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,6 @@ github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWE
2424
github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
2525
github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2JC/oIi4=
2626
github.com/mattn/go-localereader v0.0.1/go.mod h1:8fBrzywKY7BI3czFoHkuzRoWE9C+EiG4R1k4Cjx5p88=
27-
github.com/mattn/go-runewidth v0.0.16 h1:E5ScNMtiwvlvB5paMFdw9p4kSQzbXFikJ5SQO6TULQc=
28-
github.com/mattn/go-runewidth v0.0.16/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w=
2927
github.com/mattn/go-runewidth v0.0.21 h1:jJKAZiQH+2mIinzCJIaIG9Be1+0NR+5sz/lYEEjdM8w=
3028
github.com/mattn/go-runewidth v0.0.21/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs=
3129
github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 h1:ZK8zHtRHOkbHy6Mmr5D264iyp3TiX5OmNcI5cIARiQI=
@@ -34,11 +32,12 @@ github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELU
3432
github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo=
3533
github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc=
3634
github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk=
37-
github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc=
3835
github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ=
3936
github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88=
4037
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no=
4138
github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM=
39+
golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 h1:MDc5xs78ZrZr3HMQugiXOAkSZtfTpbJLDr/lwfgO53E=
40+
golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE=
4241
golang.org/x/sys v0.0.0-20210809222454-d867a43fc93e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
4342
golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
4443
golang.org/x/sys v0.36.0 h1:KVRy2GtZBrk1cBYA7MKu5bEZFxQk4NIDV6RLVcC8o0k=

internal/git/diff.go

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package git
22

33
import (
44
"fmt"
5-
"io/fs"
65
"os"
76
"os/exec"
87
"path/filepath"
@@ -64,45 +63,20 @@ func getTrackedDiff(repoDir, baselineSHA string) ([]internal.FileDiff, error) {
6463

6564
// getUntrackedDiff expands untracked files and directories to synthetic added diffs.
6665
func getUntrackedDiff(repoDir string) ([]internal.FileDiff, error) {
67-
cmd := exec.Command("git", "status", "--porcelain")
66+
cmd := exec.Command("git", "status", "--porcelain", "-z", "--untracked-files=all")
6867
cmd.Dir = repoDir
6968
out, err := cmd.Output()
7069
if err != nil {
7170
return nil, err
7271
}
7372

7473
var files []internal.FileDiff
75-
for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") {
76-
if line == "" || !strings.HasPrefix(line, "?? ") {
77-
continue
78-
}
79-
80-
path := strings.TrimPrefix(line, "?? ")
81-
if strings.HasSuffix(path, "/") {
82-
dirPath := filepath.Join(repoDir, strings.TrimSuffix(path, "/"))
83-
walkErr := filepath.WalkDir(dirPath, func(current string, d fs.DirEntry, err error) error {
84-
if err != nil || d.IsDir() {
85-
return nil
86-
}
87-
88-
rel, relErr := filepath.Rel(repoDir, current)
89-
if relErr != nil {
90-
return nil
91-
}
92-
// #nosec G304,G122 -- current comes from walking inside the repository root.
93-
data, readErr := os.ReadFile(current)
94-
if readErr != nil {
95-
return nil
96-
}
97-
files = append(files, buildNewFileDiff(rel, string(data)))
98-
return nil
99-
})
100-
if walkErr != nil {
101-
return nil, walkErr
102-
}
74+
for _, entry := range strings.Split(string(out), "\x00") {
75+
if entry == "" || !strings.HasPrefix(entry, "?? ") {
10376
continue
10477
}
10578

79+
path := strings.TrimPrefix(entry, "?? ")
10680
// #nosec G304 -- path comes from git status output scoped to the repository.
10781
data, readErr := os.ReadFile(filepath.Join(repoDir, path))
10882
if readErr != nil {

internal/git/diff_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,33 @@ func TestComputeDiffUntrackedBinaryFile(t *testing.T) {
192192
}
193193
}
194194

195+
// TestComputeDiffUntrackedFileWithSpaces verifies porcelain quoting does not
196+
// leak into reported untracked paths.
197+
func TestComputeDiffUntrackedFileWithSpaces(t *testing.T) {
198+
root := initGitRepo(t)
199+
runGit(t, root, "commit", "--allow-empty", "-m", "init")
200+
201+
baseline, err := GetHeadSHA(root)
202+
if err != nil {
203+
t.Fatalf("GetHeadSHA returned error: %v", err)
204+
}
205+
const name = "two words.txt"
206+
if err := os.WriteFile(filepath.Join(root, name), []byte("hello\n"), 0o600); err != nil {
207+
t.Fatalf("write file: %v", err)
208+
}
209+
210+
files, err := ComputeDiff(root, baseline)
211+
if err != nil {
212+
t.Fatalf("ComputeDiff returned error: %v", err)
213+
}
214+
if len(files) != 1 {
215+
t.Fatalf("expected 1 file, got %d", len(files))
216+
}
217+
if files[0].Path != name {
218+
t.Fatalf("path = %q, want %q", files[0].Path, name)
219+
}
220+
}
221+
195222
// TestComputeDiffNoChanges verifies a clean worktree produces no diffs.
196223
func TestComputeDiffNoChanges(t *testing.T) {
197224
root := initGitRepo(t)

internal/ui/diffview.go

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,38 @@ import (
99

1010
// RenderDiffView renders the current file diff within the viewport.
1111
func RenderDiffView(file *internal.FileDiff, scrollOffset, width, height int) string {
12-
if file == nil {
12+
lines := diffDisplayLines(file, width)
13+
14+
if scrollOffset < 0 {
15+
scrollOffset = 0
16+
}
17+
if scrollOffset >= len(lines) {
18+
scrollOffset = max(0, len(lines)-1)
19+
}
20+
21+
end := scrollOffset + height
22+
if end > len(lines) {
23+
end = len(lines)
24+
}
25+
if scrollOffset >= end {
1326
return ""
1427
}
28+
29+
return strings.Join(lines[scrollOffset:end], "\n")
30+
}
31+
32+
// countVisualDiffLines returns how many terminal rows the diff content occupies.
33+
func countVisualDiffLines(file *internal.FileDiff, width int) int {
34+
return len(diffDisplayLines(file, width))
35+
}
36+
37+
// diffDisplayLines expands one file diff into the exact visual lines shown in the viewport.
38+
func diffDisplayLines(file *internal.FileDiff, width int) []string {
39+
if file == nil {
40+
return nil
41+
}
1542
if file.IsBinary {
16-
return StyleDim.Render("Binary file changed")
43+
return []string{StyleDim.Render("Binary file changed")}
1744
}
1845

1946
var lines []string
@@ -35,22 +62,7 @@ func RenderDiffView(file *internal.FileDiff, scrollOffset, width, height int) st
3562
}
3663
}
3764

38-
if scrollOffset < 0 {
39-
scrollOffset = 0
40-
}
41-
if scrollOffset >= len(lines) {
42-
scrollOffset = max(0, len(lines)-1)
43-
}
44-
45-
end := scrollOffset + height
46-
if end > len(lines) {
47-
end = len(lines)
48-
}
49-
if scrollOffset >= end {
50-
return ""
51-
}
52-
53-
return strings.Join(lines[scrollOffset:end], "\n")
65+
return lines
5466
}
5567

5668
// wrapLine wraps one rendered line by terminal cell width.

internal/ui/header.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ func RenderHeader(dirName string, files []internal.FileDiff, currentIdx, newCoun
1313
return StyleDim.Render(fmt.Sprintf("%s · watching", dirName))
1414
}
1515

16+
currentIdx = min(max(currentIdx, 0), len(files)-1)
1617
file := files[currentIdx]
1718
paths := make([]string, len(files))
1819
for i, item := range files {
@@ -21,12 +22,24 @@ func RenderHeader(dirName string, files []internal.FileDiff, currentIdx, newCoun
2122
shortPaths := ShortestUniquePaths(paths)
2223
displayPath := shortPaths[currentIdx]
2324

24-
var stats string
25+
result := fmt.Sprintf("%s %s", displayPath, renderFileStats(file))
26+
if len(files) > 1 {
27+
result += StyleDim.Render(fmt.Sprintf(" ‹ %d/%d ›", currentIdx+1, len(files)))
28+
}
29+
if newCount > 0 {
30+
result += " " + StyleAdd.Render(fmt.Sprintf("+%d new", newCount))
31+
}
32+
33+
return result
34+
}
35+
36+
// renderFileStats formats one file's change summary for header and overlay views.
37+
func renderFileStats(file internal.FileDiff) string {
2538
switch {
2639
case file.IsBinary:
27-
stats = StyleDim.Render("[binary]")
40+
return StyleDim.Render("[binary]")
2841
case file.Status == internal.StatusDeleted:
29-
stats = StyleDel.Render("[deleted]")
42+
return StyleDel.Render("[deleted]")
3043
default:
3144
var parts []string
3245
if file.AddCount > 0 {
@@ -35,16 +48,6 @@ func RenderHeader(dirName string, files []internal.FileDiff, currentIdx, newCoun
3548
if file.DelCount > 0 {
3649
parts = append(parts, StyleDel.Render(fmt.Sprintf("-%d", file.DelCount)))
3750
}
38-
stats = strings.Join(parts, " ")
51+
return strings.Join(parts, " ")
3952
}
40-
41-
result := fmt.Sprintf("%s %s", displayPath, stats)
42-
if len(files) > 1 {
43-
result += StyleDim.Render(fmt.Sprintf(" ‹ %d/%d ›", currentIdx+1, len(files)))
44-
}
45-
if newCount > 0 {
46-
result += " " + StyleAdd.Render(fmt.Sprintf("+%d new", newCount))
47-
}
48-
49-
return result
5053
}

internal/ui/header_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,19 @@ func TestRenderHeaderNewCount(t *testing.T) {
4444
t.Fatalf("header = %q, want +3 new", header)
4545
}
4646
}
47+
48+
// TestRenderHeaderOutOfRangeIndex verifies defensive index clamping avoids panics.
49+
func TestRenderHeaderOutOfRangeIndex(t *testing.T) {
50+
files := []internal.FileDiff{{Path: "a.ts", AddCount: 1}}
51+
52+
defer func() {
53+
if r := recover(); r != nil {
54+
t.Fatalf("RenderHeader panicked for out-of-range index: %v", r)
55+
}
56+
}()
57+
58+
header := RenderHeader("myproject", files, 9, 0)
59+
if !strings.Contains(header, "a.ts") {
60+
t.Fatalf("header = %q, want file path", header)
61+
}
62+
}

internal/ui/model.go

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
5555
case tea.WindowSizeMsg:
5656
m.Width = msg.Width
5757
m.Height = msg.Height
58+
m.clampScrollOffset()
5859
return m, nil
5960
case FilesUpdatedMsg:
6061
return m.handleFilesUpdated(msg)
@@ -107,13 +108,16 @@ func (m Model) applyFilesUpdate(msg FilesUpdatedMsg) Model {
107108

108109
if m.FollowOn && len(m.Files) > 0 {
109110
target := len(m.Files) - 1
110-
for i, file := range m.Files {
111-
for _, changedPath := range msg.ChangedPaths {
111+
for i := len(msg.ChangedPaths) - 1; i >= 0; i-- {
112+
changedPath := msg.ChangedPaths[i]
113+
for j, file := range m.Files {
112114
if file.Path == changedPath {
113-
target = i
115+
target = j
116+
goto followTargetFound
114117
}
115118
}
116119
}
120+
followTargetFound:
117121
m.CurrentIdx = target
118122
m.ScrollOffset = 0
119123
m.NewFiles = make(map[string]bool)
@@ -152,6 +156,7 @@ func (m Model) applyFilesUpdate(msg FilesUpdatedMsg) Model {
152156
m.NewCount = 0
153157
}
154158

159+
m.clampScrollOffset()
155160
return m
156161
}
157162

@@ -161,11 +166,13 @@ func (m Model) handleKey(key string) (tea.Model, tea.Cmd) {
161166
return m, tea.Quit
162167
case "j", "down":
163168
m.ScrollOffset++
169+
m.clampScrollOffset()
164170
return m, nil
165171
case "k", "up":
166172
if m.ScrollOffset > 0 {
167173
m.ScrollOffset--
168174
}
175+
m.clampScrollOffset()
169176
return m, nil
170177
case "n":
171178
if len(m.Files) > 1 {
@@ -271,13 +278,41 @@ func (m *Model) selectLatestPendingFile() {
271278
}
272279
}
273280

281+
// clampScrollOffset keeps scroll state within the current diff viewport bounds.
282+
func (m *Model) clampScrollOffset() {
283+
if m.ScrollOffset < 0 {
284+
m.ScrollOffset = 0
285+
return
286+
}
287+
288+
maxOffset := m.maxScrollOffset()
289+
if m.ScrollOffset > maxOffset {
290+
m.ScrollOffset = maxOffset
291+
}
292+
}
293+
294+
// maxScrollOffset returns the furthest scroll position that can show diff content.
295+
func (m Model) maxScrollOffset() int {
296+
if len(m.Files) == 0 || m.CurrentIdx < 0 || m.CurrentIdx >= len(m.Files) {
297+
return 0
298+
}
299+
300+
diffHeight := max(0, m.Height-2)
301+
if diffHeight == 0 {
302+
return 0
303+
}
304+
305+
totalLines := countVisualDiffLines(&m.Files[m.CurrentIdx], m.Width)
306+
return max(0, totalLines-diffHeight)
307+
}
308+
274309
// View renders header, content, and footer into the terminal viewport.
275310
func (m Model) View() string {
276311
if m.Width == 0 || m.Height == 0 {
277312
return ""
278313
}
279314

280-
diffHeight := m.Height - 2
315+
diffHeight := max(0, m.Height-2)
281316
header := RenderHeader(m.DirName, m.Files, m.CurrentIdx, m.NewCount)
282317
footer := RenderFooter(m.FollowOn, m.Notification)
283318

0 commit comments

Comments
 (0)