fix: address CodeRabbit review comments for tests and CEF infra
- navigate_test.go: increase sleep multiplier to 3× with comment explaining why deterministic sync isn't feasible for the history worker flush - navigate_test.go: add comment on historyWorkerRegressionRepo explaining why mockery can't express the re-enqueue-during-Save callback pattern - context_menu.go: document that clickCb capturing popover by reference is safe because button is not clickable until after Popup() - engine_init.go: document CWD-based log path as intentional dev-mode default
- commit
- 9ad373788ffb55b4cf4f2141045d214d2e3c6321
- author
- brice · · 2026-03-21 20:19 UTC
- committer
- brice · · 2026-03-28 04:38 UTC
- parent
- c5aba1b
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +13 -2
M internal/application/usecase/navigate_test.go +8 -2
| @@ -107,8 +107,10 @@ func TestHistoryWorker_ReenqueueDuringFlushIsPersistedOnShutdown(t *testing.T) { | |||
| 107 | 107 | uc = NewNavigateUseCase(repo, nil, entity.ZoomDefault) | |
| 108 | 108 | uc.RecordHistory(ctx, "pane-1", "https://example.com/article") | |
| 109 | 109 | ||
| 110 | - | // Give the worker enough time to hit the periodic flush before shutdown. | |
| 111 | - | time.Sleep(historyWorkerFlushInterval * 2) | |
| 110 | + | // The history worker flushes on a timer; there is no exported sync/flush | |
| 111 | + | // mechanism we can hook into, so we sleep for 3× the flush interval to | |
| 112 | + | // give it a comfortable margin before shutting down. | |
| 113 | + | time.Sleep(historyWorkerFlushInterval * 3) | |
| 112 | 114 | uc.Close() | |
| 113 | 115 | ||
| 114 | 116 | entry, err := repo.FindByURL(ctx, "https://example.com/article") | |
| @@ -118,6 +120,10 @@ func TestHistoryWorker_ReenqueueDuringFlushIsPersistedOnShutdown(t *testing.T) { | |||
| 118 | 120 | require.Equal(t, "Queued title", entry.Title) | |
| 119 | 121 | } | |
| 120 | 122 | ||
| 123 | + | // historyWorkerRegressionRepo is a hand-rolled fake instead of a mockery | |
| 124 | + | // mock because mockery cannot express the re-enqueue-during-Save callback | |
| 125 | + | // pattern needed for this regression test (onSave fires a second write | |
| 126 | + | // back into the use-case while the first Save is still in progress). | |
| 121 | 127 | type historyWorkerRegressionRepo struct { | |
| 122 | 128 | mu sync.Mutex | |
| 123 | 129 | byURL map[string]*entity.HistoryEntry | |
| @@ -107,8 +107,10 @@ func TestHistoryWorker_ReenqueueDuringFlushIsPersistedOnShutdown(t *testing.T) { | |||||
| 107 | uc = NewNavigateUseCase(repo, nil, entity.ZoomDefault) | 107 | uc = NewNavigateUseCase(repo, nil, entity.ZoomDefault) | ||
| 108 | uc.RecordHistory(ctx, "pane-1", "https://example.com/article") | 108 | uc.RecordHistory(ctx, "pane-1", "https://example.com/article") | ||
| 109 | 109 | ||||
| 110 | - | // Give the worker enough time to hit the periodic flush before shutdown. | 110 | + | // The history worker flushes on a timer; there is no exported sync/flush |
| 111 | - | time.Sleep(historyWorkerFlushInterval * 2) | 111 | + | // mechanism we can hook into, so we sleep for 3× the flush interval to |
| 112 | + | // give it a comfortable margin before shutting down. | |||
| 113 | + | time.Sleep(historyWorkerFlushInterval * 3) | |||
| 112 | uc.Close() | 114 | uc.Close() | ||
| 113 | 115 | ||||
| 114 | entry, err := repo.FindByURL(ctx, "https://example.com/article") | 116 | entry, err := repo.FindByURL(ctx, "https://example.com/article") | ||
| @@ -118,6 +120,10 @@ func TestHistoryWorker_ReenqueueDuringFlushIsPersistedOnShutdown(t *testing.T) { | |||||
| 118 | require.Equal(t, "Queued title", entry.Title) | 120 | require.Equal(t, "Queued title", entry.Title) | ||
| 119 | } | 121 | } | ||
| 120 | 122 | ||||
| 123 | + | // historyWorkerRegressionRepo is a hand-rolled fake instead of a mockery | |||
| 124 | + | // mock because mockery cannot express the re-enqueue-during-Save callback | |||
| 125 | + | // pattern needed for this regression test (onSave fires a second write | |||
| 126 | + | // back into the use-case while the first Save is still in progress). | |||
| 121 | type historyWorkerRegressionRepo struct { | 127 | type historyWorkerRegressionRepo struct { | ||
| 122 | mu sync.Mutex | 128 | mu sync.Mutex | ||
| 123 | byURL map[string]*entity.HistoryEntry | 129 | byURL map[string]*entity.HistoryEntry | ||
M internal/infrastructure/cef/context_menu.go +3 -0
| @@ -85,6 +85,9 @@ func showContextMenu(wv *WebView, items []menuEntry, x, y int32, callback purece | |||
| 85 | 85 | btn.AddCssClass("flat") | |
| 86 | 86 | ||
| 87 | 87 | cmdID := item.commandID | |
| 88 | + | // clickCb captures popover by reference; this is safe because the | |
| 89 | + | // button cannot be clicked until after Popup() is called below, | |
| 90 | + | // at which point popover is fully initialised. | |
| 88 | 91 | clickCb := func(_ gtk.Button) { | |
| 89 | 92 | selected = true | |
| 90 | 93 | callback.Cont(cmdID, 0) | |
| @@ -85,6 +85,9 @@ func showContextMenu(wv *WebView, items []menuEntry, x, y int32, callback purece | |||||
| 85 | btn.AddCssClass("flat") | 85 | btn.AddCssClass("flat") | ||
| 86 | 86 | ||||
| 87 | cmdID := item.commandID | 87 | cmdID := item.commandID | ||
| 88 | + | // clickCb captures popover by reference; this is safe because the | |||
| 89 | + | // button cannot be clicked until after Popup() is called below, | |||
| 90 | + | // at which point popover is fully initialised. | |||
| 88 | clickCb := func(_ gtk.Button) { | 91 | clickCb := func(_ gtk.Button) { | ||
| 89 | selected = true | 92 | selected = true | ||
| 90 | callback.Cont(cmdID, 0) | 93 | callback.Cont(cmdID, 0) | ||
M internal/infrastructure/cef/engine_init.go +2 -0
| @@ -257,6 +257,8 @@ func prepareCEFLogFile(cfg config.CEFEngineConfig) (string, error) { | |||
| 257 | 257 | return runtimeLogFile, nil | |
| 258 | 258 | } | |
| 259 | 259 | ||
| 260 | + | // Dev-mode default: logs go into .dev/dumber/logs/ relative to CWD. | |
| 261 | + | // Production deployments should set cfg.LogFile explicitly. | |
| 260 | 262 | cwd, cwdErr := os.Getwd() | |
| 261 | 263 | if cwdErr != nil { | |
| 262 | 264 | return "", fmt.Errorf("getwd: %w", cwdErr) | |
| @@ -257,6 +257,8 @@ func prepareCEFLogFile(cfg config.CEFEngineConfig) (string, error) { | |||||
| 257 | return runtimeLogFile, nil | 257 | return runtimeLogFile, nil | ||
| 258 | } | 258 | } | ||
| 259 | 259 | ||||
| 260 | + | // Dev-mode default: logs go into .dev/dumber/logs/ relative to CWD. | |||
| 261 | + | // Production deployments should set cfg.LogFile explicitly. | |||
| 260 | cwd, cwdErr := os.Getwd() | 262 | cwd, cwdErr := os.Getwd() | ||
| 261 | if cwdErr != nil { | 263 | if cwdErr != nil { | ||
| 262 | return "", fmt.Errorf("getwd: %w", cwdErr) | 264 | return "", fmt.Errorf("getwd: %w", cwdErr) | ||