detached 8df2acd

fix: address Copilot review feedback on localhost and download handling

commit
8df2acdcb2401a37c4c7b1e64381e8739e0646f2
author
brice · · 2026-01-24 16:07 UTC
parent
92eeb35
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
7 files · +173 -4
Split
UnifiedSplit
M internal/application/usecase/prepare_download.go +6 -1
@@ -61,7 +61,12 @@ func (u *PrepareDownloadUseCase) Execute(ctx context.Context, input PrepareDownl
6161 if u.fs != nil {
6262 safeName = download.MakeUniqueFilename(input.DownloadDir, safeName, func(path string) bool {
6363 exists, err := u.fs.Exists(ctx, path)
64- return err == nil && exists
64+ if err != nil {
65+ log.Warn().Err(err).Str("path", path).
66+ Msg("failed to check file existence; assuming exists")
67+ return true // Conservative: assume exists to avoid overwrite
68+ }
69+ return exists
6570 })
6671 }
6772
@@ -61,7 +61,12 @@ func (u *PrepareDownloadUseCase) Execute(ctx context.Context, input PrepareDownl
61 if u.fs != nil {61 if u.fs != nil {
62 safeName = download.MakeUniqueFilename(input.DownloadDir, safeName, func(path string) bool {62 safeName = download.MakeUniqueFilename(input.DownloadDir, safeName, func(path string) bool {
63 exists, err := u.fs.Exists(ctx, path)63 exists, err := u.fs.Exists(ctx, path)
64- return err == nil && exists64+ if err != nil {
65+ log.Warn().Err(err).Str("path", path).
66+ Msg("failed to check file existence; assuming exists")
67+ return true // Conservative: assume exists to avoid overwrite
68+ }
69+ return exists
65 })70 })
66 }71 }
67 72
M internal/application/usecase/prepare_download_test.go +102 -0
@@ -2,6 +2,7 @@ package usecase
22
33 import (
44 "context"
5+ "errors"
56 "testing"
67
78 "github.com/stretchr/testify/assert"
@@ -18,6 +19,31 @@ func (m *mockDownloadResponse) GetMimeType() string { return m.mimeType
1819 func (m *mockDownloadResponse) GetSuggestedFilename() string { return m.suggestedFilename }
1920 func (m *mockDownloadResponse) GetUri() string { return m.uri }
2021
22+// mockFileSystem implements port.FileSystem for testing.
23+type mockFileSystem struct {
24+ existingFiles map[string]bool
25+ existsErr error
26+}
27+
28+func (m *mockFileSystem) Exists(_ context.Context, path string) (bool, error) {
29+ if m.existsErr != nil {
30+ return false, m.existsErr
31+ }
32+ return m.existingFiles[path], nil
33+}
34+
35+func (*mockFileSystem) IsDirectory(_ context.Context, _ string) (bool, error) {
36+ return false, nil
37+}
38+
39+func (*mockFileSystem) GetSize(_ context.Context, _ string) (int64, error) {
40+ return 0, nil
41+}
42+
43+func (*mockFileSystem) RemoveAll(_ context.Context, _ string) error {
44+ return nil
45+}
46+
2147 func TestPrepareDownloadUseCase_Execute(t *testing.T) {
2248 ctx := context.Background()
2349 uc := NewPrepareDownloadUseCase(nil) // nil FileSystem disables deduplication
@@ -169,3 +195,79 @@ func TestPrepareDownloadUseCase_ResolvePriority(t *testing.T) {
169195 assert.Equal(t, "response.pdf", result.Filename)
170196 })
171197 }
198+
199+func TestPrepareDownloadUseCase_Deduplication(t *testing.T) {
200+ ctx := context.Background()
201+
202+ t.Run("existing file triggers _(1) suffix", func(t *testing.T) {
203+ fs := &mockFileSystem{
204+ existingFiles: map[string]bool{
205+ "/tmp/downloads/document.pdf": true,
206+ },
207+ }
208+ uc := NewPrepareDownloadUseCase(fs)
209+
210+ input := PrepareDownloadInput{
211+ SuggestedFilename: "document.pdf",
212+ DownloadDir: "/tmp/downloads",
213+ }
214+
215+ result := uc.Execute(ctx, input)
216+ assert.Equal(t, "document_(1).pdf", result.Filename)
217+ assert.Equal(t, "/tmp/downloads/document_(1).pdf", result.DestinationPath)
218+ })
219+
220+ t.Run("multiple existing files increment suffix", func(t *testing.T) {
221+ fs := &mockFileSystem{
222+ existingFiles: map[string]bool{
223+ "/tmp/downloads/document.pdf": true,
224+ "/tmp/downloads/document_(1).pdf": true,
225+ },
226+ }
227+ uc := NewPrepareDownloadUseCase(fs)
228+
229+ input := PrepareDownloadInput{
230+ SuggestedFilename: "document.pdf",
231+ DownloadDir: "/tmp/downloads",
232+ }
233+
234+ result := uc.Execute(ctx, input)
235+ assert.Equal(t, "document_(2).pdf", result.Filename)
236+ assert.Equal(t, "/tmp/downloads/document_(2).pdf", result.DestinationPath)
237+ })
238+
239+ t.Run("fs.Exists error assumes file exists (conservative)", func(t *testing.T) {
240+ fs := &mockFileSystem{
241+ existingFiles: map[string]bool{},
242+ existsErr: errors.New("permission denied"),
243+ }
244+ uc := NewPrepareDownloadUseCase(fs)
245+
246+ input := PrepareDownloadInput{
247+ SuggestedFilename: "document.pdf",
248+ DownloadDir: "/tmp/downloads",
249+ }
250+
251+ result := uc.Execute(ctx, input)
252+ // When fs.Exists returns an error, we conservatively assume file exists
253+ // This triggers deduplication to use timestamp fallback (after 1000 attempts)
254+ assert.NotEqual(t, "document.pdf", result.Filename)
255+ assert.Contains(t, result.Filename, "document_")
256+ })
257+
258+ t.Run("non-existing file uses original name", func(t *testing.T) {
259+ fs := &mockFileSystem{
260+ existingFiles: map[string]bool{},
261+ }
262+ uc := NewPrepareDownloadUseCase(fs)
263+
264+ input := PrepareDownloadInput{
265+ SuggestedFilename: "document.pdf",
266+ DownloadDir: "/tmp/downloads",
267+ }
268+
269+ result := uc.Execute(ctx, input)
270+ assert.Equal(t, "document.pdf", result.Filename)
271+ assert.Equal(t, "/tmp/downloads/document.pdf", result.DestinationPath)
272+ })
273+}
@@ -2,6 +2,7 @@ package usecase
2 2
3 import (3 import (
4 "context"4 "context"
5+ "errors"
5 "testing"6 "testing"
6 7
7 "github.com/stretchr/testify/assert"8 "github.com/stretchr/testify/assert"
@@ -18,6 +19,31 @@ func (m *mockDownloadResponse) GetMimeType() string { return m.mimeType
18 func (m *mockDownloadResponse) GetSuggestedFilename() string { return m.suggestedFilename }19 func (m *mockDownloadResponse) GetSuggestedFilename() string { return m.suggestedFilename }
19 func (m *mockDownloadResponse) GetUri() string { return m.uri }20 func (m *mockDownloadResponse) GetUri() string { return m.uri }
20 21
22+// mockFileSystem implements port.FileSystem for testing.
23+type mockFileSystem struct {
24+ existingFiles map[string]bool
25+ existsErr error
26+}
27+
28+func (m *mockFileSystem) Exists(_ context.Context, path string) (bool, error) {
29+ if m.existsErr != nil {
30+ return false, m.existsErr
31+ }
32+ return m.existingFiles[path], nil
33+}
34+
35+func (*mockFileSystem) IsDirectory(_ context.Context, _ string) (bool, error) {
36+ return false, nil
37+}
38+
39+func (*mockFileSystem) GetSize(_ context.Context, _ string) (int64, error) {
40+ return 0, nil
41+}
42+
43+func (*mockFileSystem) RemoveAll(_ context.Context, _ string) error {
44+ return nil
45+}
46+
21 func TestPrepareDownloadUseCase_Execute(t *testing.T) {47 func TestPrepareDownloadUseCase_Execute(t *testing.T) {
22 ctx := context.Background()48 ctx := context.Background()
23 uc := NewPrepareDownloadUseCase(nil) // nil FileSystem disables deduplication49 uc := NewPrepareDownloadUseCase(nil) // nil FileSystem disables deduplication
@@ -169,3 +195,79 @@ func TestPrepareDownloadUseCase_ResolvePriority(t *testing.T) {
169 assert.Equal(t, "response.pdf", result.Filename)195 assert.Equal(t, "response.pdf", result.Filename)
170 })196 })
171 }197 }
198+
199+func TestPrepareDownloadUseCase_Deduplication(t *testing.T) {
200+ ctx := context.Background()
201+
202+ t.Run("existing file triggers _(1) suffix", func(t *testing.T) {
203+ fs := &mockFileSystem{
204+ existingFiles: map[string]bool{
205+ "/tmp/downloads/document.pdf": true,
206+ },
207+ }
208+ uc := NewPrepareDownloadUseCase(fs)
209+
210+ input := PrepareDownloadInput{
211+ SuggestedFilename: "document.pdf",
212+ DownloadDir: "/tmp/downloads",
213+ }
214+
215+ result := uc.Execute(ctx, input)
216+ assert.Equal(t, "document_(1).pdf", result.Filename)
217+ assert.Equal(t, "/tmp/downloads/document_(1).pdf", result.DestinationPath)
218+ })
219+
220+ t.Run("multiple existing files increment suffix", func(t *testing.T) {
221+ fs := &mockFileSystem{
222+ existingFiles: map[string]bool{
223+ "/tmp/downloads/document.pdf": true,
224+ "/tmp/downloads/document_(1).pdf": true,
225+ },
226+ }
227+ uc := NewPrepareDownloadUseCase(fs)
228+
229+ input := PrepareDownloadInput{
230+ SuggestedFilename: "document.pdf",
231+ DownloadDir: "/tmp/downloads",
232+ }
233+
234+ result := uc.Execute(ctx, input)
235+ assert.Equal(t, "document_(2).pdf", result.Filename)
236+ assert.Equal(t, "/tmp/downloads/document_(2).pdf", result.DestinationPath)
237+ })
238+
239+ t.Run("fs.Exists error assumes file exists (conservative)", func(t *testing.T) {
240+ fs := &mockFileSystem{
241+ existingFiles: map[string]bool{},
242+ existsErr: errors.New("permission denied"),
243+ }
244+ uc := NewPrepareDownloadUseCase(fs)
245+
246+ input := PrepareDownloadInput{
247+ SuggestedFilename: "document.pdf",
248+ DownloadDir: "/tmp/downloads",
249+ }
250+
251+ result := uc.Execute(ctx, input)
252+ // When fs.Exists returns an error, we conservatively assume file exists
253+ // This triggers deduplication to use timestamp fallback (after 1000 attempts)
254+ assert.NotEqual(t, "document.pdf", result.Filename)
255+ assert.Contains(t, result.Filename, "document_")
256+ })
257+
258+ t.Run("non-existing file uses original name", func(t *testing.T) {
259+ fs := &mockFileSystem{
260+ existingFiles: map[string]bool{},
261+ }
262+ uc := NewPrepareDownloadUseCase(fs)
263+
264+ input := PrepareDownloadInput{
265+ SuggestedFilename: "document.pdf",
266+ DownloadDir: "/tmp/downloads",
267+ }
268+
269+ result := uc.Execute(ctx, input)
270+ assert.Equal(t, "document.pdf", result.Filename)
271+ assert.Equal(t, "/tmp/downloads/document.pdf", result.DestinationPath)
272+ })
273+}
M internal/domain/download/filename.go +9 -1
@@ -48,12 +48,20 @@ func SanitizeFilenameWithExtension(name, mimeType string) string {
4848
4949 // GetExtensionFromMimeType returns a file extension for a given MIME type.
5050 // Returns empty string if MIME type is unknown or empty.
51+// Handles MIME types with parameters (e.g., "application/pdf; charset=binary").
5152 func GetExtensionFromMimeType(mimeType string) string {
5253 if mimeType == "" {
5354 return ""
5455 }
5556
56- exts, err := mime.ExtensionsByType(mimeType)
57+ // Parse media type to extract bare type without parameters.
58+ // mime.ExtensionsByType expects "application/pdf", not "application/pdf; charset=binary".
59+ mediaType, _, err := mime.ParseMediaType(mimeType)
60+ if err != nil || mediaType == "" {
61+ return ""
62+ }
63+
64+ exts, err := mime.ExtensionsByType(mediaType)
5765 if err != nil || len(exts) == 0 {
5866 return ""
5967 }
@@ -48,12 +48,20 @@ func SanitizeFilenameWithExtension(name, mimeType string) string {
48 48
49 // GetExtensionFromMimeType returns a file extension for a given MIME type.49 // GetExtensionFromMimeType returns a file extension for a given MIME type.
50 // Returns empty string if MIME type is unknown or empty.50 // Returns empty string if MIME type is unknown or empty.
51+// Handles MIME types with parameters (e.g., "application/pdf; charset=binary").
51 func GetExtensionFromMimeType(mimeType string) string {52 func GetExtensionFromMimeType(mimeType string) string {
52 if mimeType == "" {53 if mimeType == "" {
53 return ""54 return ""
54 }55 }
55 56
56- exts, err := mime.ExtensionsByType(mimeType)57+ // Parse media type to extract bare type without parameters.
58+ // mime.ExtensionsByType expects "application/pdf", not "application/pdf; charset=binary".
59+ mediaType, _, err := mime.ParseMediaType(mimeType)
60+ if err != nil || mediaType == "" {
61+ return ""
62+ }
63+
64+ exts, err := mime.ExtensionsByType(mediaType)
57 if err != nil || len(exts) == 0 {65 if err != nil || len(exts) == 0 {
58 return ""66 return ""
59 }67 }
M internal/domain/download/filename_test.go +20 -0
@@ -139,6 +139,26 @@ func TestGetExtensionFromMimeType(t *testing.T) {
139139 input: "application/pdf",
140140 expected: ".pdf",
141141 },
142+ {
143+ name: "pdf mime type with charset param",
144+ input: "application/pdf; charset=binary",
145+ expected: ".pdf",
146+ },
147+ {
148+ name: "text html with charset",
149+ input: "text/html; charset=utf-8",
150+ expected: ".htm",
151+ },
152+ {
153+ name: "mime type with multiple params",
154+ input: "application/json; charset=utf-8; boundary=something",
155+ expected: ".json",
156+ },
157+ {
158+ name: "invalid mime type",
159+ input: "not-a-valid-mime",
160+ expected: "",
161+ },
142162 }
143163
144164 for _, tt := range tests {
@@ -139,6 +139,26 @@ func TestGetExtensionFromMimeType(t *testing.T) {
139 input: "application/pdf",139 input: "application/pdf",
140 expected: ".pdf",140 expected: ".pdf",
141 },141 },
142+ {
143+ name: "pdf mime type with charset param",
144+ input: "application/pdf; charset=binary",
145+ expected: ".pdf",
146+ },
147+ {
148+ name: "text html with charset",
149+ input: "text/html; charset=utf-8",
150+ expected: ".htm",
151+ },
152+ {
153+ name: "mime type with multiple params",
154+ input: "application/json; charset=utf-8; boundary=something",
155+ expected: ".json",
156+ },
157+ {
158+ name: "invalid mime type",
159+ input: "not-a-valid-mime",
160+ expected: "",
161+ },
142 }162 }
143 163
144 for _, tt := range tests {164 for _, tt := range tests {
M internal/domain/url/normalize.go +2 -2
@@ -82,7 +82,7 @@ func Normalize(input string) string {
8282 }
8383
8484 // Looks like a URL (contains . and no spaces)
85- if strings.HasPrefix(input, "localhost") {
85+ if input == "localhost" || strings.HasPrefix(input, "localhost:") || strings.HasPrefix(input, "localhost/") {
8686 return "http://" + input
8787 }
8888 if strings.Contains(input, ".") && !strings.Contains(input, " ") {
@@ -115,7 +115,7 @@ func LooksLikeURL(input string) bool {
115115 }
116116
117117 // Contains a dot and no spaces = likely a URL
118- if strings.HasPrefix(input, "localhost") {
118+ if input == "localhost" || strings.HasPrefix(input, "localhost:") || strings.HasPrefix(input, "localhost/") {
119119 return true
120120 }
121121 return strings.Contains(input, ".") && !strings.Contains(input, " ")
@@ -82,7 +82,7 @@ func Normalize(input string) string {
82 }82 }
83 83
84 // Looks like a URL (contains . and no spaces)84 // Looks like a URL (contains . and no spaces)
85- if strings.HasPrefix(input, "localhost") {85+ if input == "localhost" || strings.HasPrefix(input, "localhost:") || strings.HasPrefix(input, "localhost/") {
86 return "http://" + input86 return "http://" + input
87 }87 }
88 if strings.Contains(input, ".") && !strings.Contains(input, " ") {88 if strings.Contains(input, ".") && !strings.Contains(input, " ") {
@@ -115,7 +115,7 @@ func LooksLikeURL(input string) bool {
115 }115 }
116 116
117 // Contains a dot and no spaces = likely a URL117 // Contains a dot and no spaces = likely a URL
118- if strings.HasPrefix(input, "localhost") {118+ if input == "localhost" || strings.HasPrefix(input, "localhost:") || strings.HasPrefix(input, "localhost/") {
119 return true119 return true
120 }120 }
121 return strings.Contains(input, ".") && !strings.Contains(input, " ")121 return strings.Contains(input, ".") && !strings.Contains(input, " ")
M internal/domain/url/normalize_test.go +30 -0
@@ -77,6 +77,21 @@ func TestNormalize(t *testing.T) {
7777 input: "localhost:8080",
7878 want: "http://localhost:8080",
7979 },
80+ {
81+ name: "localhost with path",
82+ input: "localhost/api/v1",
83+ want: "http://localhost/api/v1",
84+ },
85+ {
86+ name: "localhost.com is a domain not localhost",
87+ input: "localhost.com",
88+ want: "https://localhost.com",
89+ },
90+ {
91+ name: "localhostevil.com is a domain not localhost",
92+ input: "localhostevil.com",
93+ want: "https://localhostevil.com",
94+ },
8095 }
8196
8297 for _, tt := range tests {
@@ -287,6 +302,21 @@ func TestLooksLikeURL(t *testing.T) {
287302 input: "localhost:8080",
288303 want: true,
289304 },
305+ {
306+ name: "localhost with path",
307+ input: "localhost/api/v1",
308+ want: true,
309+ },
310+ {
311+ name: "localhost.com is a domain not localhost",
312+ input: "localhost.com",
313+ want: true,
314+ },
315+ {
316+ name: "localhostevil.com is a domain not localhost",
317+ input: "localhostevil.com",
318+ want: true,
319+ },
290320 }
291321
292322 for _, tt := range tests {
@@ -77,6 +77,21 @@ func TestNormalize(t *testing.T) {
77 input: "localhost:8080",77 input: "localhost:8080",
78 want: "http://localhost:8080",78 want: "http://localhost:8080",
79 },79 },
80+ {
81+ name: "localhost with path",
82+ input: "localhost/api/v1",
83+ want: "http://localhost/api/v1",
84+ },
85+ {
86+ name: "localhost.com is a domain not localhost",
87+ input: "localhost.com",
88+ want: "https://localhost.com",
89+ },
90+ {
91+ name: "localhostevil.com is a domain not localhost",
92+ input: "localhostevil.com",
93+ want: "https://localhostevil.com",
94+ },
80 }95 }
81 96
82 for _, tt := range tests {97 for _, tt := range tests {
@@ -287,6 +302,21 @@ func TestLooksLikeURL(t *testing.T) {
287 input: "localhost:8080",302 input: "localhost:8080",
288 want: true,303 want: true,
289 },304 },
305+ {
306+ name: "localhost with path",
307+ input: "localhost/api/v1",
308+ want: true,
309+ },
310+ {
311+ name: "localhost.com is a domain not localhost",
312+ input: "localhost.com",
313+ want: true,
314+ },
315+ {
316+ name: "localhostevil.com is a domain not localhost",
317+ input: "localhostevil.com",
318+ want: true,
319+ },
290 }320 }
291 321
292 for _, tt := range tests {322 for _, tt := range tests {
M internal/infrastructure/webkit/download_handler.go +4 -0
@@ -27,11 +27,15 @@ type DownloadHandler struct {
2727 }
2828
2929 // NewDownloadHandler creates a new download handler.
30+// Panics if prepareDownloadUC is nil (fail fast on misconfiguration).
3031 func NewDownloadHandler(
3132 downloadPath string,
3233 handler port.DownloadEventHandler,
3334 prepareDownloadUC *usecase.PrepareDownloadUseCase,
3435 ) *DownloadHandler {
36+ if prepareDownloadUC == nil {
37+ panic("prepareDownloadUC is required")
38+ }
3539 return &DownloadHandler{
3640 downloadPath: downloadPath,
3741 eventHandler: handler,
@@ -27,11 +27,15 @@ type DownloadHandler struct {
27 }27 }
28 28
29 // NewDownloadHandler creates a new download handler.29 // NewDownloadHandler creates a new download handler.
30+// Panics if prepareDownloadUC is nil (fail fast on misconfiguration).
30 func NewDownloadHandler(31 func NewDownloadHandler(
31 downloadPath string,32 downloadPath string,
32 handler port.DownloadEventHandler,33 handler port.DownloadEventHandler,
33 prepareDownloadUC *usecase.PrepareDownloadUseCase,34 prepareDownloadUC *usecase.PrepareDownloadUseCase,
34 ) *DownloadHandler {35 ) *DownloadHandler {
36+ if prepareDownloadUC == nil {
37+ panic("prepareDownloadUC is required")
38+ }
35 return &DownloadHandler{39 return &DownloadHandler{
36 downloadPath: downloadPath,40 downloadPath: downloadPath,
37 eventHandler: handler,41 eventHandler: handler,

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 8

Tags 42

↑↓ moveenter openesc close