b04eecd fix(download): apply review follow-ups · dumber

M internal/infrastructure/cef/download_handler.go +9 -4


@@ -197,17 +197,22 @@ func (h *downloadHandler) currentState(id uint32, item purecef.DownloadItem) cef
197	197		h.mu.Lock()
198	198		defer h.mu.Unlock()
199	199
200		-	state, ok := h.active[id]
201		-	if !ok {
	200	+	state, exists := h.active[id]
	201	+	if !exists {
202	202		state = cefDownloadState{}
203	203		}
	204	+	updated := false
204	205		if state.destination == "" {
205	206		state.destination = item.GetFullPath()
	207	+	updated = true
206	208		}
207	209		if state.filename == "" {
208	210		state.filename = downloadFilenameFromItem(item, state.destination)
	211	+	updated = true
	212	+	}
	213	+	if exists && updated {
	214	+	h.active[id] = state
209	215		}
210		-	h.active[id] = state
211	216		return state
212	217		}
213	218

@@ -269,10 +274,10 @@ func (h *downloadHandler) markFinished(id uint32) bool {
269	274		h.mu.Lock()
270	275		defer h.mu.Unlock()
271	276
	277	+	delete(h.active, id)
272	278		if _, seen := h.finished[id]; seen {
273	279		return false
274	280		}
275		-	delete(h.active, id)
276	281		if len(h.finished) >= maxFinishedEntries {
277	282		clear(h.finished)
278	283		}

 	h.mu.Lock()
 	defer h.mu.Unlock()
-	state, ok := h.active[id]
+	state, exists := h.active[id]
-	if !ok {
+	if !exists {
 		state = cefDownloadState{}
 	}
+	updated := false
 	if state.destination == "" {
 		state.destination = item.GetFullPath()
+		updated = true
 	}
 	if state.filename == "" {
 		state.filename = downloadFilenameFromItem(item, state.destination)
+		updated = true
+	}
+	if exists && updated {
+		h.active[id] = state
 	}
-	h.active[id] = state
 	return state
 }
 	h.mu.Lock()
 	defer h.mu.Unlock()
+	delete(h.active, id)
 	if _, seen := h.finished[id]; seen {
 		return false
 	}
-	delete(h.active, id)
 	if len(h.finished) >= maxFinishedEntries {
 		clear(h.finished)
 	}

M internal/infrastructure/cef/download_handler_test.go +9 -1


@@ -2,6 +2,7 @@ package cef
2	2
3	3		import (
4	4		"context"
	5	+	"fmt"
5	6		"testing"
6	7
7	8		purecef "github.com/bnema/purego-cef/cef"

@@ -127,6 +128,12 @@ func TestMarkFinished_SuppressesDuplicatesAfterCleanup(t *testing.T) {
127	128		handler.mu.Unlock()
128	129
129	130		require.True(t, handler.markFinished(42), "first terminal event should succeed")
	131	+
	132	+	// Simulate a concurrent update re-populating active before a duplicate terminal update.
	133	+	handler.mu.Lock()
	134	+	handler.active[42] = cefDownloadState{filename: "test.bin", destination: "/tmp/test.bin", lastProgressPercent: -1}
	135	+	handler.mu.Unlock()
	136	+
130	137		require.False(t, handler.markFinished(42), "second terminal event should be suppressed")
131	138
132	139		// Active map should not contain the entry.

@@ -194,6 +201,7 @@ func TestCEFDownloadInterruptedErrorIncludesCodeAndReason(t *testing.T) {
194	201
195	202		require.Len(t, events.events, 2)
196	203		require.Error(t, events.events[1].Error)
197		-	require.ErrorContains(t, events.events[1].Error, "code=33")
	204	+	expectedCode := fmt.Sprintf("code=%d", purecef.DownloadInterruptReasonServerBadContent)
	205	+	require.ErrorContains(t, events.events[1].Error, expectedCode)
198	206		require.ErrorContains(t, events.events[1].Error, "reason=server_bad_content")
199	207		}

 import (
 	"context"
+	"fmt"
 	"testing"
 	purecef "github.com/bnema/purego-cef/cef"
 	handler.mu.Unlock()
 	require.True(t, handler.markFinished(42), "first terminal event should succeed")
+	// Simulate a concurrent update re-populating active before a duplicate terminal update.
+	handler.mu.Lock()
+	handler.active[42] = cefDownloadState{filename: "test.bin", destination: "/tmp/test.bin", lastProgressPercent: -1}
+	handler.mu.Unlock()
 	require.False(t, handler.markFinished(42), "second terminal event should be suppressed")
 	// Active map should not contain the entry.
 	require.Len(t, events.events, 2)
 	require.Error(t, events.events[1].Error)
-	require.ErrorContains(t, events.events[1].Error, "code=33")
+	expectedCode := fmt.Sprintf("code=%d", purecef.DownloadInterruptReasonServerBadContent)
+	require.ErrorContains(t, events.events[1].Error, expectedCode)
 	require.ErrorContains(t, events.events[1].Error, "reason=server_bad_content")
 }

M internal/infrastructure/cef/webview_handlers_test.go +7 -2


@@ -6,7 +6,6 @@ import (
6	6		"testing"
7	7		"time"
8	8
9		-	"github.com/bnema/dumber/internal/application/usecase"
10	9		purecef "github.com/bnema/purego-cef/cef"
11	10		cefmocks "github.com/bnema/purego-cef/cef/mocks"
12	11		"github.com/stretchr/testify/assert"

@@ -24,6 +23,12 @@ type clipboardOrchestratorRecorder struct {
24	23		selectionCalls int
25	24		}
26	25
	26	+	type stubDownloadPreparer struct{}
	27	+
	28	+	func (stubDownloadPreparer) Execute(context.Context, port.DownloadPrepareInput) *port.DownloadPrepareOutput {
	29	+	return &port.DownloadPrepareOutput{}
	30	+	}
	31	+
27	32		type controlledSelectionTimer struct {
28	33		stopped bool
29	34		}

@@ -377,7 +382,7 @@ func TestOptionalHandlersAreAlwaysEnabled(t *testing.T) {
377	382
378	383		func TestGetDownloadHandler_EnabledWhenEngineConfigured(t *testing.T) {
379	384		eng := &Engine{}
380		-	require.NoError(t, eng.ConfigureDownloads(context.Background(), "/tmp/downloads", nil, usecase.NewPrepareDownloadUseCase(nil)))
	385	+	require.NoError(t, eng.ConfigureDownloads(context.Background(), "/tmp/downloads", nil, stubDownloadPreparer{}))
381	386
382	387		h := &handlerSet{
383	388		wv: &WebView{

 	"testing"
 	"time"
-	"github.com/bnema/dumber/internal/application/usecase"
 	purecef "github.com/bnema/purego-cef/cef"
 	cefmocks "github.com/bnema/purego-cef/cef/mocks"
 	"github.com/stretchr/testify/assert"
 	selectionCalls int
 }
+type stubDownloadPreparer struct{}
+func (stubDownloadPreparer) Execute(context.Context, port.DownloadPrepareInput) *port.DownloadPrepareOutput {
+	return &port.DownloadPrepareOutput{}
+}
 type controlledSelectionTimer struct {
 	stopped bool
 }
 func TestGetDownloadHandler_EnabledWhenEngineConfigured(t *testing.T) {
 	eng := &Engine{}
-	require.NoError(t, eng.ConfigureDownloads(context.Background(), "/tmp/downloads", nil, usecase.NewPrepareDownloadUseCase(nil)))
+	require.NoError(t, eng.ConfigureDownloads(context.Background(), "/tmp/downloads", nil, stubDownloadPreparer{}))
 	h := &handlerSet{
 		wv: &WebView{

M internal/infrastructure/downloadruntime/runtime.go +7 -1


@@ -2,6 +2,7 @@ package downloadruntime
2	2
3	3		import (
4	4		"context"
	5	+	"errors"
5	6		"fmt"
6	7		"os"
7	8		"sync"

@@ -51,6 +52,10 @@ func (r *Runtime) ResolveDestination(
51	52		preparer := r.preparer
52	53		r.mu.RUnlock()
53	54
	55	+	if preparer == nil {
	56	+	return nil, fmt.Errorf("download preparer is required")
	57	+	}
	58	+
54	59		if err := os.MkdirAll(downloadPath, dirPerm); err != nil {
55	60		return nil, fmt.Errorf("failed to create download directory %q: %w", downloadPath, err)
56	61		}

@@ -149,7 +154,8 @@ func (r *Runtime) EmitFailed(ctx context.Context, filename, destination string,
149	154		Err(err).
150	155		Str("filename", filename).
151	156		Str("destination", destination)
152		-	if codedErr, ok := err.(codedDownloadError); ok {
	157	+	var codedErr codedDownloadError
	158	+	if errors.As(err, &codedErr) {
153	159		log = log.
154	160		Int("error_code", codedErr.DownloadErrorCode()).
155	161		Str("error_reason", codedErr.DownloadErrorReason())

 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"sync"
 	preparer := r.preparer
 	r.mu.RUnlock()
+	if preparer == nil {
+		return nil, fmt.Errorf("download preparer is required")
+	}
 	if err := os.MkdirAll(downloadPath, dirPerm); err != nil {
 		return nil, fmt.Errorf("failed to create download directory %q: %w", downloadPath, err)
 	}
 		Err(err).
 		Str("filename", filename).
 		Str("destination", destination)
-	if codedErr, ok := err.(codedDownloadError); ok {
+	var codedErr codedDownloadError
+	if errors.As(err, &codedErr) {
 		log = log.
 			Int("error_code", codedErr.DownloadErrorCode()).
 			Str("error_reason", codedErr.DownloadErrorReason())

M internal/infrastructure/downloadruntime/runtime_test.go +11 -1


@@ -30,7 +30,7 @@ func (c *captureEvents) OnDownloadEvent(_ context.Context, event port.DownloadEv
30	30		func TestRuntimeResolveDestinationAndEvents(t *testing.T) {
31	31		ctx := context.Background()
32	32		events := &captureEvents{}
33		-	runtime := New("/tmp/downloads", events, usecase.NewPrepareDownloadUseCase(nil))
	33	+	runtime := New(t.TempDir(), events, usecase.NewPrepareDownloadUseCase(nil))
34	34
35	35		output, err := runtime.ResolveDestination(ctx, "artifact", stubResponse{mimeType: "application/pdf"})
36	36

@@ -54,3 +54,13 @@ func TestNewRuntime_NilPreparer_NoPanic(t *testing.T) {
54	54		_ = New("/tmp/downloads", nil, nil)
55	55		})
56	56		}
	57	+
	58	+	func TestRuntimeResolveDestination_NilPreparerReturnsError(t *testing.T) {
	59	+	runtime := New(t.TempDir(), nil, nil)
	60	+
	61	+	output, err := runtime.ResolveDestination(context.Background(), "artifact", stubResponse{})
	62	+
	63	+	require.Nil(t, output)
	64	+	require.Error(t, err)
	65	+	require.ErrorContains(t, err, "download preparer is required")
	66	+	}

 func TestRuntimeResolveDestinationAndEvents(t *testing.T) {
 	ctx := context.Background()
 	events := &captureEvents{}
-	runtime := New("/tmp/downloads", events, usecase.NewPrepareDownloadUseCase(nil))
+	runtime := New(t.TempDir(), events, usecase.NewPrepareDownloadUseCase(nil))
 	output, err := runtime.ResolveDestination(ctx, "artifact", stubResponse{mimeType: "application/pdf"})
 		_ = New("/tmp/downloads", nil, nil)
 	})
 }
+func TestRuntimeResolveDestination_NilPreparerReturnsError(t *testing.T) {
+	runtime := New(t.TempDir(), nil, nil)
+	output, err := runtime.ResolveDestination(context.Background(), "artifact", stubResponse{})
+	require.Nil(t, output)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "download preparer is required")
+}