fix(history): harden recorder shutdown and sidebar timers
- commit
- 5f2a4b4628bf75cda2db14ccd25c1acfb70302ee
- author
- brice · · 2026-06-25 04:58 UTC
- parent
- 1708967
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
6 files · +245 -24
M internal/application/usecase/history_recorder.go +87 -16
| @@ -24,6 +24,12 @@ const ( | |||
| 24 | 24 | ||
| 25 | 25 | // historyWorkerFlushInterval coalesces bursts into fewer persistence writes. | |
| 26 | 26 | historyWorkerFlushInterval = 100 * time.Millisecond | |
| 27 | + | ||
| 28 | + | // historyWorkerShutdownFlushTimeout bounds final persistence attempts during shutdown. | |
| 29 | + | historyWorkerShutdownFlushTimeout = 2 * time.Second | |
| 30 | + | ||
| 31 | + | // historyWorkerShutdownWaitTimeout bounds Close even if a repository call ignores context cancellation. | |
| 32 | + | historyWorkerShutdownWaitTimeout = historyWorkerShutdownFlushTimeout + 500*time.Millisecond | |
| 27 | 33 | ) | |
| 28 | 34 | ||
| 29 | 35 | // historyDeduplicationWindow is the time window for deduplicating history visits. | |
| @@ -108,12 +114,9 @@ func (p *pendingHistoryRecords) empty() bool { | |||
| 108 | 114 | return len(p.visits) == 0 && len(p.titles) == 0 | |
| 109 | 115 | } | |
| 110 | 116 | ||
| 111 | - | func (p *pendingHistoryRecords) reset() (map[string]int, map[string]string) { | |
| 112 | - | visits := p.visits | |
| 113 | - | titles := p.titles | |
| 117 | + | func (p *pendingHistoryRecords) reset() { | |
| 114 | 118 | p.visits = make(map[string]int) | |
| 115 | 119 | p.titles = make(map[string]string) | |
| 116 | - | return visits, titles | |
| 117 | 120 | } | |
| 118 | 121 | ||
| 119 | 122 | // HistoryRecorderUseCase records persisted history visits and title updates asynchronously. | |
| @@ -133,11 +136,13 @@ type HistoryRecorderUseCase struct { | |||
| 133 | 136 | done chan struct{} | |
| 134 | 137 | wg sync.WaitGroup | |
| 135 | 138 | ctx context.Context // Base context for background worker | |
| 139 | + | cancel context.CancelFunc | |
| 136 | 140 | } | |
| 137 | 141 | ||
| 138 | 142 | // NewHistoryRecorderUseCase creates a new history recorder use case. | |
| 139 | 143 | func NewHistoryRecorderUseCase(historyRepo repository.HistoryRepository, changeSink port.HistoryChangeSink) *HistoryRecorderUseCase { | |
| 140 | 144 | changeSink = normalizeHistoryChangeSink(changeSink) | |
| 145 | + | ctx, cancel := context.WithCancel(context.Background()) | |
| 141 | 146 | ||
| 142 | 147 | uc := &HistoryRecorderUseCase{ | |
| 143 | 148 | historyRepo: historyRepo, | |
| @@ -146,7 +151,8 @@ func NewHistoryRecorderUseCase(historyRepo repository.HistoryRepository, changeS | |||
| 146 | 151 | historyQueue: make(chan historyRecord, historyQueueSize), | |
| 147 | 152 | controlQueue: make(chan historyControlRequest), | |
| 148 | 153 | done: make(chan struct{}), | |
| 149 | - | ctx: context.Background(), | |
| 154 | + | ctx: ctx, | |
| 155 | + | cancel: cancel, | |
| 150 | 156 | } | |
| 151 | 157 | ||
| 152 | 158 | uc.wg.Add(1) | |
| @@ -168,13 +174,57 @@ func (uc *HistoryRecorderUseCase) SetHistoryChangeSink(changeSink port.HistoryCh | |||
| 168 | 174 | ||
| 169 | 175 | // Close shuts down the background history worker and drains any pending records. | |
| 170 | 176 | func (uc *HistoryRecorderUseCase) Close() { | |
| 177 | + | if uc == nil { | |
| 178 | + | return | |
| 179 | + | } | |
| 171 | 180 | uc.closeOnce.Do(func() { | |
| 172 | 181 | uc.enqueueMu.Lock() | |
| 173 | 182 | uc.closing = true | |
| 174 | 183 | uc.enqueueMu.Unlock() | |
| 175 | - | close(uc.done) | |
| 184 | + | uc.cancelWorkerContext() | |
| 185 | + | if uc.done != nil { | |
| 186 | + | close(uc.done) | |
| 187 | + | } | |
| 176 | 188 | }) | |
| 177 | - | uc.wg.Wait() | |
| 189 | + | if uc.waitForHistoryWorker(historyWorkerShutdownWaitTimeout) { | |
| 190 | + | return | |
| 191 | + | } | |
| 192 | + | logging.FromContext(uc.workerContext()).Warn(). | |
| 193 | + | Dur("timeout", historyWorkerShutdownWaitTimeout). | |
| 194 | + | Msg("history worker shutdown timed out") | |
| 195 | + | } | |
| 196 | + | ||
| 197 | + | func (uc *HistoryRecorderUseCase) workerContext() context.Context { | |
| 198 | + | if uc == nil || uc.ctx == nil { | |
| 199 | + | return context.Background() | |
| 200 | + | } | |
| 201 | + | return uc.ctx | |
| 202 | + | } | |
| 203 | + | ||
| 204 | + | func (uc *HistoryRecorderUseCase) cancelWorkerContext() { | |
| 205 | + | if uc != nil && uc.cancel != nil { | |
| 206 | + | uc.cancel() | |
| 207 | + | } | |
| 208 | + | } | |
| 209 | + | ||
| 210 | + | func (uc *HistoryRecorderUseCase) waitForHistoryWorker(timeout time.Duration) bool { | |
| 211 | + | done := make(chan struct{}) | |
| 212 | + | go func() { | |
| 213 | + | uc.wg.Wait() | |
| 214 | + | close(done) | |
| 215 | + | }() | |
| 216 | + | if timeout <= 0 { | |
| 217 | + | <-done | |
| 218 | + | return true | |
| 219 | + | } | |
| 220 | + | timer := time.NewTimer(timeout) | |
| 221 | + | defer timer.Stop() | |
| 222 | + | select { | |
| 223 | + | case <-done: | |
| 224 | + | return true | |
| 225 | + | case <-timer.C: | |
| 226 | + | return false | |
| 227 | + | } | |
| 178 | 228 | } | |
| 179 | 229 | ||
| 180 | 230 | // RecordHistory queues a history entry for async recording. | |
| @@ -248,6 +298,8 @@ func (uc *HistoryRecorderUseCase) ClearPaneHistory(paneID string) { | |||
| 248 | 298 | return | |
| 249 | 299 | } | |
| 250 | 300 | paneID = normalizePaneID(paneID) | |
| 301 | + | uc.mutationMu.Lock() | |
| 302 | + | defer uc.mutationMu.Unlock() | |
| 251 | 303 | uc.recentMu.Lock() | |
| 252 | 304 | delete(uc.recentVisits, paneID) | |
| 253 | 305 | uc.recentMu.Unlock() | |
| @@ -275,7 +327,7 @@ func (uc *HistoryRecorderUseCase) UpdateHistoryTitle(_ context.Context, historyU | |||
| 275 | 327 | defer uc.mutationMu.Unlock() | |
| 276 | 328 | ||
| 277 | 329 | if !uc.enqueueHistoryRecord(historyRecord{url: historyURL, title: title}) { | |
| 278 | - | logging.FromContext(uc.ctx).Warn(). | |
| 330 | + | logging.FromContext(uc.workerContext()).Warn(). | |
| 279 | 331 | Str("url", logging.RedactURL(historyURL)). | |
| 280 | 332 | Msg("history queue full or recorder closed, title update dropped") | |
| 281 | 333 | } | |
| @@ -302,6 +354,7 @@ func (uc *HistoryRecorderUseCase) historyWorker() { | |||
| 302 | 354 | ||
| 303 | 355 | ticker := time.NewTicker(historyWorkerFlushInterval) | |
| 304 | 356 | defer ticker.Stop() | |
| 357 | + | workerCtx := uc.workerContext() | |
| 305 | 358 | ||
| 306 | 359 | pending := newPendingHistoryRecords() | |
| 307 | 360 | for { | |
| @@ -309,19 +362,22 @@ func (uc *HistoryRecorderUseCase) historyWorker() { | |||
| 309 | 362 | case record := <-uc.historyQueue: | |
| 310 | 363 | pending.add(record) | |
| 311 | 364 | case <-ticker.C: | |
| 312 | - | _ = uc.flushPendingHistory(uc.ctx, pending) | |
| 365 | + | _ = uc.flushPendingHistory(workerCtx, pending) | |
| 313 | 366 | case req := <-uc.controlQueue: | |
| 314 | 367 | uc.handleHistoryControl(pending, req) | |
| 315 | 368 | case <-uc.done: | |
| 316 | 369 | uc.shutdownHistoryWorker(pending) | |
| 317 | 370 | return | |
| 371 | + | case <-workerCtx.Done(): | |
| 372 | + | uc.shutdownHistoryWorker(pending) | |
| 373 | + | return | |
| 318 | 374 | } | |
| 319 | 375 | } | |
| 320 | 376 | } | |
| 321 | 377 | ||
| 322 | 378 | func (uc *HistoryRecorderUseCase) flushPendingHistory(ctx context.Context, pending *pendingHistoryRecords) error { | |
| 323 | 379 | if ctx == nil { | |
| 324 | - | ctx = uc.ctx | |
| 380 | + | ctx = uc.workerContext() | |
| 325 | 381 | } | |
| 326 | 382 | if pending.empty() { | |
| 327 | 383 | return nil | |
| @@ -409,14 +465,23 @@ func (uc *HistoryRecorderUseCase) drainHistoryQueue(pending *pendingHistoryRecor | |||
| 409 | 465 | } | |
| 410 | 466 | ||
| 411 | 467 | func (uc *HistoryRecorderUseCase) shutdownHistoryWorker(pending *pendingHistoryRecords) { | |
| 412 | - | log := logging.FromContext(uc.ctx).With(). | |
| 468 | + | log := logging.FromContext(uc.workerContext()).With(). | |
| 413 | 469 | Str("component", "history-worker"). | |
| 414 | 470 | Logger() | |
| 471 | + | shutdownCtx, cancel := context.WithTimeout(context.Background(), historyWorkerShutdownFlushTimeout) | |
| 472 | + | defer cancel() | |
| 415 | 473 | ||
| 416 | 474 | for { | |
| 475 | + | if err := shutdownCtx.Err(); err != nil { | |
| 476 | + | if !pending.empty() { | |
| 477 | + | log.Warn().Err(err).Msg("dropping unflushed history during shutdown after timeout") | |
| 478 | + | pending.reset() | |
| 479 | + | } | |
| 480 | + | return | |
| 481 | + | } | |
| 417 | 482 | log.Debug().Int("remaining", len(uc.historyQueue)).Msg("draining history queue") | |
| 418 | 483 | uc.drainHistoryQueue(pending) | |
| 419 | - | uc.flushPendingHistoryForShutdown(log, pending) | |
| 484 | + | uc.flushPendingHistoryForShutdown(shutdownCtx, log, pending) | |
| 420 | 485 | ||
| 421 | 486 | uc.enqueueMu.Lock() | |
| 422 | 487 | uc.drainHistoryQueue(pending) | |
| @@ -430,10 +495,10 @@ func (uc *HistoryRecorderUseCase) shutdownHistoryWorker(pending *pendingHistoryR | |||
| 430 | 495 | } | |
| 431 | 496 | } | |
| 432 | 497 | ||
| 433 | - | func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(log zerolog.Logger, pending *pendingHistoryRecords) { | |
| 498 | + | func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(ctx context.Context, log zerolog.Logger, pending *pendingHistoryRecords) { | |
| 434 | 499 | const shutdownFlushAttempts = 3 | |
| 435 | 500 | for attempt := 1; attempt <= shutdownFlushAttempts; attempt++ { | |
| 436 | - | err := uc.flushPendingHistory(uc.ctx, pending) | |
| 501 | + | err := uc.flushPendingHistory(ctx, pending) | |
| 437 | 502 | if err == nil || pending.empty() { | |
| 438 | 503 | return | |
| 439 | 504 | } | |
| @@ -443,7 +508,13 @@ func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(log zerolog.Log | |||
| 443 | 508 | return | |
| 444 | 509 | } | |
| 445 | 510 | log.Warn().Err(err).Int("attempt", attempt).Msg("retrying unflushed history during shutdown") | |
| 446 | - | time.Sleep(historyWorkerFlushInterval) | |
| 511 | + | select { | |
| 512 | + | case <-ctx.Done(): | |
| 513 | + | log.Warn().Err(ctx.Err()).Msg("dropping unflushed history during shutdown after timeout") | |
| 514 | + | pending.reset() | |
| 515 | + | return | |
| 516 | + | case <-time.After(historyWorkerFlushInterval): | |
| 517 | + | } | |
| 447 | 518 | } | |
| 448 | 519 | } | |
| 449 | 520 | ||
| @@ -527,7 +598,7 @@ func (uc *HistoryRecorderUseCase) publishHistoryChange(successfulVisits, success | |||
| 527 | 598 | reasons = append(reasons, dto.HistoryChangeReasonTitle) | |
| 528 | 599 | } | |
| 529 | 600 | sink := uc.historyChangeSink() | |
| 530 | - | sink.OnHistoryChanged(uc.ctx, dto.HistoryChange{ | |
| 601 | + | sink.OnHistoryChanged(uc.workerContext(), dto.HistoryChange{ | |
| 531 | 602 | Reasons: reasons, | |
| 532 | 603 | VisitCount: successfulVisits, | |
| 533 | 604 | TitleCount: successfulTitles, | |
| @@ -24,6 +24,12 @@ const ( | |||||
| 24 | 24 | ||||
| 25 | // historyWorkerFlushInterval coalesces bursts into fewer persistence writes. | 25 | // historyWorkerFlushInterval coalesces bursts into fewer persistence writes. | ||
| 26 | historyWorkerFlushInterval = 100 * time.Millisecond | 26 | historyWorkerFlushInterval = 100 * time.Millisecond | ||
| 27 | + | ||||
| 28 | + | // historyWorkerShutdownFlushTimeout bounds final persistence attempts during shutdown. | |||
| 29 | + | historyWorkerShutdownFlushTimeout = 2 * time.Second | |||
| 30 | + | ||||
| 31 | + | // historyWorkerShutdownWaitTimeout bounds Close even if a repository call ignores context cancellation. | |||
| 32 | + | historyWorkerShutdownWaitTimeout = historyWorkerShutdownFlushTimeout + 500*time.Millisecond | |||
| 27 | ) | 33 | ) | ||
| 28 | 34 | ||||
| 29 | // historyDeduplicationWindow is the time window for deduplicating history visits. | 35 | // historyDeduplicationWindow is the time window for deduplicating history visits. | ||
| @@ -108,12 +114,9 @@ func (p *pendingHistoryRecords) empty() bool { | |||||
| 108 | return len(p.visits) == 0 && len(p.titles) == 0 | 114 | return len(p.visits) == 0 && len(p.titles) == 0 | ||
| 109 | } | 115 | } | ||
| 110 | 116 | ||||
| 111 | - | func (p *pendingHistoryRecords) reset() (map[string]int, map[string]string) { | 117 | + | func (p *pendingHistoryRecords) reset() { |
| 112 | - | visits := p.visits | |||
| 113 | - | titles := p.titles | |||
| 114 | p.visits = make(map[string]int) | 118 | p.visits = make(map[string]int) | ||
| 115 | p.titles = make(map[string]string) | 119 | p.titles = make(map[string]string) | ||
| 116 | - | return visits, titles | |||
| 117 | } | 120 | } | ||
| 118 | 121 | ||||
| 119 | // HistoryRecorderUseCase records persisted history visits and title updates asynchronously. | 122 | // HistoryRecorderUseCase records persisted history visits and title updates asynchronously. | ||
| @@ -133,11 +136,13 @@ type HistoryRecorderUseCase struct { | |||||
| 133 | done chan struct{} | 136 | done chan struct{} | ||
| 134 | wg sync.WaitGroup | 137 | wg sync.WaitGroup | ||
| 135 | ctx context.Context // Base context for background worker | 138 | ctx context.Context // Base context for background worker | ||
| 139 | + | cancel context.CancelFunc | |||
| 136 | } | 140 | } | ||
| 137 | 141 | ||||
| 138 | // NewHistoryRecorderUseCase creates a new history recorder use case. | 142 | // NewHistoryRecorderUseCase creates a new history recorder use case. | ||
| 139 | func NewHistoryRecorderUseCase(historyRepo repository.HistoryRepository, changeSink port.HistoryChangeSink) *HistoryRecorderUseCase { | 143 | func NewHistoryRecorderUseCase(historyRepo repository.HistoryRepository, changeSink port.HistoryChangeSink) *HistoryRecorderUseCase { | ||
| 140 | changeSink = normalizeHistoryChangeSink(changeSink) | 144 | changeSink = normalizeHistoryChangeSink(changeSink) | ||
| 145 | + | ctx, cancel := context.WithCancel(context.Background()) | |||
| 141 | 146 | ||||
| 142 | uc := &HistoryRecorderUseCase{ | 147 | uc := &HistoryRecorderUseCase{ | ||
| 143 | historyRepo: historyRepo, | 148 | historyRepo: historyRepo, | ||
| @@ -146,7 +151,8 @@ func NewHistoryRecorderUseCase(historyRepo repository.HistoryRepository, changeS | |||||
| 146 | historyQueue: make(chan historyRecord, historyQueueSize), | 151 | historyQueue: make(chan historyRecord, historyQueueSize), | ||
| 147 | controlQueue: make(chan historyControlRequest), | 152 | controlQueue: make(chan historyControlRequest), | ||
| 148 | done: make(chan struct{}), | 153 | done: make(chan struct{}), | ||
| 149 | - | ctx: context.Background(), | 154 | + | ctx: ctx, |
| 155 | + | cancel: cancel, | |||
| 150 | } | 156 | } | ||
| 151 | 157 | ||||
| 152 | uc.wg.Add(1) | 158 | uc.wg.Add(1) | ||
| @@ -168,13 +174,57 @@ func (uc *HistoryRecorderUseCase) SetHistoryChangeSink(changeSink port.HistoryCh | |||||
| 168 | 174 | ||||
| 169 | // Close shuts down the background history worker and drains any pending records. | 175 | // Close shuts down the background history worker and drains any pending records. | ||
| 170 | func (uc *HistoryRecorderUseCase) Close() { | 176 | func (uc *HistoryRecorderUseCase) Close() { | ||
| 177 | + | if uc == nil { | |||
| 178 | + | return | |||
| 179 | + | } | |||
| 171 | uc.closeOnce.Do(func() { | 180 | uc.closeOnce.Do(func() { | ||
| 172 | uc.enqueueMu.Lock() | 181 | uc.enqueueMu.Lock() | ||
| 173 | uc.closing = true | 182 | uc.closing = true | ||
| 174 | uc.enqueueMu.Unlock() | 183 | uc.enqueueMu.Unlock() | ||
| 175 | - | close(uc.done) | 184 | + | uc.cancelWorkerContext() |
| 185 | + | if uc.done != nil { | |||
| 186 | + | close(uc.done) | |||
| 187 | + | } | |||
| 176 | }) | 188 | }) | ||
| 177 | - | uc.wg.Wait() | 189 | + | if uc.waitForHistoryWorker(historyWorkerShutdownWaitTimeout) { |
| 190 | + | return | |||
| 191 | + | } | |||
| 192 | + | logging.FromContext(uc.workerContext()).Warn(). | |||
| 193 | + | Dur("timeout", historyWorkerShutdownWaitTimeout). | |||
| 194 | + | Msg("history worker shutdown timed out") | |||
| 195 | + | } | |||
| 196 | + | ||||
| 197 | + | func (uc *HistoryRecorderUseCase) workerContext() context.Context { | |||
| 198 | + | if uc == nil || uc.ctx == nil { | |||
| 199 | + | return context.Background() | |||
| 200 | + | } | |||
| 201 | + | return uc.ctx | |||
| 202 | + | } | |||
| 203 | + | ||||
| 204 | + | func (uc *HistoryRecorderUseCase) cancelWorkerContext() { | |||
| 205 | + | if uc != nil && uc.cancel != nil { | |||
| 206 | + | uc.cancel() | |||
| 207 | + | } | |||
| 208 | + | } | |||
| 209 | + | ||||
| 210 | + | func (uc *HistoryRecorderUseCase) waitForHistoryWorker(timeout time.Duration) bool { | |||
| 211 | + | done := make(chan struct{}) | |||
| 212 | + | go func() { | |||
| 213 | + | uc.wg.Wait() | |||
| 214 | + | close(done) | |||
| 215 | + | }() | |||
| 216 | + | if timeout <= 0 { | |||
| 217 | + | <-done | |||
| 218 | + | return true | |||
| 219 | + | } | |||
| 220 | + | timer := time.NewTimer(timeout) | |||
| 221 | + | defer timer.Stop() | |||
| 222 | + | select { | |||
| 223 | + | case <-done: | |||
| 224 | + | return true | |||
| 225 | + | case <-timer.C: | |||
| 226 | + | return false | |||
| 227 | + | } | |||
| 178 | } | 228 | } | ||
| 179 | 229 | ||||
| 180 | // RecordHistory queues a history entry for async recording. | 230 | // RecordHistory queues a history entry for async recording. | ||
| @@ -248,6 +298,8 @@ func (uc *HistoryRecorderUseCase) ClearPaneHistory(paneID string) { | |||||
| 248 | return | 298 | return | ||
| 249 | } | 299 | } | ||
| 250 | paneID = normalizePaneID(paneID) | 300 | paneID = normalizePaneID(paneID) | ||
| 301 | + | uc.mutationMu.Lock() | |||
| 302 | + | defer uc.mutationMu.Unlock() | |||
| 251 | uc.recentMu.Lock() | 303 | uc.recentMu.Lock() | ||
| 252 | delete(uc.recentVisits, paneID) | 304 | delete(uc.recentVisits, paneID) | ||
| 253 | uc.recentMu.Unlock() | 305 | uc.recentMu.Unlock() | ||
| @@ -275,7 +327,7 @@ func (uc *HistoryRecorderUseCase) UpdateHistoryTitle(_ context.Context, historyU | |||||
| 275 | defer uc.mutationMu.Unlock() | 327 | defer uc.mutationMu.Unlock() | ||
| 276 | 328 | ||||
| 277 | if !uc.enqueueHistoryRecord(historyRecord{url: historyURL, title: title}) { | 329 | if !uc.enqueueHistoryRecord(historyRecord{url: historyURL, title: title}) { | ||
| 278 | - | logging.FromContext(uc.ctx).Warn(). | 330 | + | logging.FromContext(uc.workerContext()).Warn(). |
| 279 | Str("url", logging.RedactURL(historyURL)). | 331 | Str("url", logging.RedactURL(historyURL)). | ||
| 280 | Msg("history queue full or recorder closed, title update dropped") | 332 | Msg("history queue full or recorder closed, title update dropped") | ||
| 281 | } | 333 | } | ||
| @@ -302,6 +354,7 @@ func (uc *HistoryRecorderUseCase) historyWorker() { | |||||
| 302 | 354 | ||||
| 303 | ticker := time.NewTicker(historyWorkerFlushInterval) | 355 | ticker := time.NewTicker(historyWorkerFlushInterval) | ||
| 304 | defer ticker.Stop() | 356 | defer ticker.Stop() | ||
| 357 | + | workerCtx := uc.workerContext() | |||
| 305 | 358 | ||||
| 306 | pending := newPendingHistoryRecords() | 359 | pending := newPendingHistoryRecords() | ||
| 307 | for { | 360 | for { | ||
| @@ -309,19 +362,22 @@ func (uc *HistoryRecorderUseCase) historyWorker() { | |||||
| 309 | case record := <-uc.historyQueue: | 362 | case record := <-uc.historyQueue: | ||
| 310 | pending.add(record) | 363 | pending.add(record) | ||
| 311 | case <-ticker.C: | 364 | case <-ticker.C: | ||
| 312 | - | _ = uc.flushPendingHistory(uc.ctx, pending) | 365 | + | _ = uc.flushPendingHistory(workerCtx, pending) |
| 313 | case req := <-uc.controlQueue: | 366 | case req := <-uc.controlQueue: | ||
| 314 | uc.handleHistoryControl(pending, req) | 367 | uc.handleHistoryControl(pending, req) | ||
| 315 | case <-uc.done: | 368 | case <-uc.done: | ||
| 316 | uc.shutdownHistoryWorker(pending) | 369 | uc.shutdownHistoryWorker(pending) | ||
| 317 | return | 370 | return | ||
| 371 | + | case <-workerCtx.Done(): | |||
| 372 | + | uc.shutdownHistoryWorker(pending) | |||
| 373 | + | return | |||
| 318 | } | 374 | } | ||
| 319 | } | 375 | } | ||
| 320 | } | 376 | } | ||
| 321 | 377 | ||||
| 322 | func (uc *HistoryRecorderUseCase) flushPendingHistory(ctx context.Context, pending *pendingHistoryRecords) error { | 378 | func (uc *HistoryRecorderUseCase) flushPendingHistory(ctx context.Context, pending *pendingHistoryRecords) error { | ||
| 323 | if ctx == nil { | 379 | if ctx == nil { | ||
| 324 | - | ctx = uc.ctx | 380 | + | ctx = uc.workerContext() |
| 325 | } | 381 | } | ||
| 326 | if pending.empty() { | 382 | if pending.empty() { | ||
| 327 | return nil | 383 | return nil | ||
| @@ -409,14 +465,23 @@ func (uc *HistoryRecorderUseCase) drainHistoryQueue(pending *pendingHistoryRecor | |||||
| 409 | } | 465 | } | ||
| 410 | 466 | ||||
| 411 | func (uc *HistoryRecorderUseCase) shutdownHistoryWorker(pending *pendingHistoryRecords) { | 467 | func (uc *HistoryRecorderUseCase) shutdownHistoryWorker(pending *pendingHistoryRecords) { | ||
| 412 | - | log := logging.FromContext(uc.ctx).With(). | 468 | + | log := logging.FromContext(uc.workerContext()).With(). |
| 413 | Str("component", "history-worker"). | 469 | Str("component", "history-worker"). | ||
| 414 | Logger() | 470 | Logger() | ||
| 471 | + | shutdownCtx, cancel := context.WithTimeout(context.Background(), historyWorkerShutdownFlushTimeout) | |||
| 472 | + | defer cancel() | |||
| 415 | 473 | ||||
| 416 | for { | 474 | for { | ||
| 475 | + | if err := shutdownCtx.Err(); err != nil { | |||
| 476 | + | if !pending.empty() { | |||
| 477 | + | log.Warn().Err(err).Msg("dropping unflushed history during shutdown after timeout") | |||
| 478 | + | pending.reset() | |||
| 479 | + | } | |||
| 480 | + | return | |||
| 481 | + | } | |||
| 417 | log.Debug().Int("remaining", len(uc.historyQueue)).Msg("draining history queue") | 482 | log.Debug().Int("remaining", len(uc.historyQueue)).Msg("draining history queue") | ||
| 418 | uc.drainHistoryQueue(pending) | 483 | uc.drainHistoryQueue(pending) | ||
| 419 | - | uc.flushPendingHistoryForShutdown(log, pending) | 484 | + | uc.flushPendingHistoryForShutdown(shutdownCtx, log, pending) |
| 420 | 485 | ||||
| 421 | uc.enqueueMu.Lock() | 486 | uc.enqueueMu.Lock() | ||
| 422 | uc.drainHistoryQueue(pending) | 487 | uc.drainHistoryQueue(pending) | ||
| @@ -430,10 +495,10 @@ func (uc *HistoryRecorderUseCase) shutdownHistoryWorker(pending *pendingHistoryR | |||||
| 430 | } | 495 | } | ||
| 431 | } | 496 | } | ||
| 432 | 497 | ||||
| 433 | - | func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(log zerolog.Logger, pending *pendingHistoryRecords) { | 498 | + | func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(ctx context.Context, log zerolog.Logger, pending *pendingHistoryRecords) { |
| 434 | const shutdownFlushAttempts = 3 | 499 | const shutdownFlushAttempts = 3 | ||
| 435 | for attempt := 1; attempt <= shutdownFlushAttempts; attempt++ { | 500 | for attempt := 1; attempt <= shutdownFlushAttempts; attempt++ { | ||
| 436 | - | err := uc.flushPendingHistory(uc.ctx, pending) | 501 | + | err := uc.flushPendingHistory(ctx, pending) |
| 437 | if err == nil || pending.empty() { | 502 | if err == nil || pending.empty() { | ||
| 438 | return | 503 | return | ||
| 439 | } | 504 | } | ||
| @@ -443,7 +508,13 @@ func (uc *HistoryRecorderUseCase) flushPendingHistoryForShutdown(log zerolog.Log | |||||
| 443 | return | 508 | return | ||
| 444 | } | 509 | } | ||
| 445 | log.Warn().Err(err).Int("attempt", attempt).Msg("retrying unflushed history during shutdown") | 510 | log.Warn().Err(err).Int("attempt", attempt).Msg("retrying unflushed history during shutdown") | ||
| 446 | - | time.Sleep(historyWorkerFlushInterval) | 511 | + | select { |
| 512 | + | case <-ctx.Done(): | |||
| 513 | + | log.Warn().Err(ctx.Err()).Msg("dropping unflushed history during shutdown after timeout") | |||
| 514 | + | pending.reset() | |||
| 515 | + | return | |||
| 516 | + | case <-time.After(historyWorkerFlushInterval): | |||
| 517 | + | } | |||
| 447 | } | 518 | } | ||
| 448 | } | 519 | } | ||
| 449 | 520 | ||||
| @@ -527,7 +598,7 @@ func (uc *HistoryRecorderUseCase) publishHistoryChange(successfulVisits, success | |||||
| 527 | reasons = append(reasons, dto.HistoryChangeReasonTitle) | 598 | reasons = append(reasons, dto.HistoryChangeReasonTitle) | ||
| 528 | } | 599 | } | ||
| 529 | sink := uc.historyChangeSink() | 600 | sink := uc.historyChangeSink() | ||
| 530 | - | sink.OnHistoryChanged(uc.ctx, dto.HistoryChange{ | 601 | + | sink.OnHistoryChanged(uc.workerContext(), dto.HistoryChange{ |
| 531 | Reasons: reasons, | 602 | Reasons: reasons, | ||
| 532 | VisitCount: successfulVisits, | 603 | VisitCount: successfulVisits, | ||
| 533 | TitleCount: successfulTitles, | 604 | TitleCount: successfulTitles, | ||
M internal/application/usecase/history_recorder_test.go +72 -0
| @@ -204,6 +204,56 @@ func TestHistoryRecorder_CloseDrainsPendingAndPublishes(t *testing.T) { | |||
| 204 | 204 | require.Equal(t, 1, changes[0].VisitCount) | |
| 205 | 205 | } | |
| 206 | 206 | ||
| 207 | + | func TestHistoryRecorder_CloseCancelsInFlightWorkerFlushAndDrains(t *testing.T) { | |
| 208 | + | ctx := context.Background() | |
| 209 | + | sink := &recordingHistoryChangeSink{} | |
| 210 | + | repo := repomocks.NewMockHistoryRepository(t) | |
| 211 | + | const historyURL = "https://example.com/cancel-close" | |
| 212 | + | ||
| 213 | + | findStarted := make(chan struct{}) | |
| 214 | + | var findOnce sync.Once | |
| 215 | + | repo.EXPECT().FindByURL(mock.Anything, historyURL).RunAndReturn( | |
| 216 | + | func(ctx context.Context, _ string) (*entity.HistoryEntry, error) { | |
| 217 | + | first := false | |
| 218 | + | findOnce.Do(func() { first = true }) | |
| 219 | + | if first { | |
| 220 | + | close(findStarted) | |
| 221 | + | <-ctx.Done() | |
| 222 | + | return nil, ctx.Err() | |
| 223 | + | } | |
| 224 | + | return nil, nil | |
| 225 | + | }, | |
| 226 | + | ).Twice() | |
| 227 | + | repo.EXPECT().Save(mock.Anything, mock.MatchedBy(func(entry *entity.HistoryEntry) bool { | |
| 228 | + | return entry.URL == historyURL && entry.VisitCount == 1 | |
| 229 | + | })).Return(nil).Once() | |
| 230 | + | ||
| 231 | + | uc := NewHistoryRecorderUseCase(repo, sink) | |
| 232 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |
| 233 | + | ||
| 234 | + | select { | |
| 235 | + | case <-findStarted: | |
| 236 | + | case <-time.After(time.Second): | |
| 237 | + | t.Fatal("timed out waiting for periodic history flush to start") | |
| 238 | + | } | |
| 239 | + | ||
| 240 | + | closed := make(chan struct{}) | |
| 241 | + | go func() { | |
| 242 | + | uc.Close() | |
| 243 | + | close(closed) | |
| 244 | + | }() | |
| 245 | + | ||
| 246 | + | select { | |
| 247 | + | case <-closed: | |
| 248 | + | case <-time.After(time.Second): | |
| 249 | + | t.Fatal("timed out waiting for history recorder shutdown") | |
| 250 | + | } | |
| 251 | + | ||
| 252 | + | changes := sink.snapshot() | |
| 253 | + | require.Len(t, changes, 1) | |
| 254 | + | require.Equal(t, 1, changes[0].VisitCount) | |
| 255 | + | } | |
| 256 | + | ||
| 207 | 257 | func TestHistoryRecorder_FailedPersistencePublishesNoChange(t *testing.T) { | |
| 208 | 258 | ctx := context.Background() | |
| 209 | 259 | sink := &recordingHistoryChangeSink{} | |
| @@ -328,6 +378,28 @@ func TestHistoryRecorder_RecordHistory_DedupIsPerPane(t *testing.T) { | |||
| 328 | 378 | require.Equal(t, 2, changes[0].VisitCount) | |
| 329 | 379 | } | |
| 330 | 380 | ||
| 381 | + | func TestHistoryRecorder_ClearPaneHistorySerializesWithRecorderAndAllowsPaneReuse(t *testing.T) { | |
| 382 | + | ctx := context.Background() | |
| 383 | + | sink := &recordingHistoryChangeSink{} | |
| 384 | + | repo := repomocks.NewMockHistoryRepository(t) | |
| 385 | + | const historyURL = "https://example.com/reused-pane" | |
| 386 | + | ||
| 387 | + | repo.EXPECT().FindByURL(mock.Anything, historyURL).Return(nil, nil).Once() | |
| 388 | + | repo.EXPECT().Save(mock.Anything, mock.MatchedBy(func(entry *entity.HistoryEntry) bool { | |
| 389 | + | return entry.URL == historyURL && entry.VisitCount == 2 | |
| 390 | + | })).Return(nil).Once() | |
| 391 | + | ||
| 392 | + | uc := NewHistoryRecorderUseCase(repo, sink) | |
| 393 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |
| 394 | + | uc.ClearPaneHistory("pane-1") | |
| 395 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |
| 396 | + | uc.Close() | |
| 397 | + | ||
| 398 | + | changes := sink.snapshot() | |
| 399 | + | require.Len(t, changes, 1) | |
| 400 | + | require.Equal(t, 2, changes[0].VisitCount) | |
| 401 | + | } | |
| 402 | + | ||
| 331 | 403 | func TestHistoryRecorder_FlushPendingHistoryTracksPartialFallbackVisitWrites(t *testing.T) { | |
| 332 | 404 | ctx := context.Background() | |
| 333 | 405 | sink := &recordingHistoryChangeSink{} | |
| @@ -204,6 +204,56 @@ func TestHistoryRecorder_CloseDrainsPendingAndPublishes(t *testing.T) { | |||||
| 204 | require.Equal(t, 1, changes[0].VisitCount) | 204 | require.Equal(t, 1, changes[0].VisitCount) | ||
| 205 | } | 205 | } | ||
| 206 | 206 | ||||
| 207 | + | func TestHistoryRecorder_CloseCancelsInFlightWorkerFlushAndDrains(t *testing.T) { | |||
| 208 | + | ctx := context.Background() | |||
| 209 | + | sink := &recordingHistoryChangeSink{} | |||
| 210 | + | repo := repomocks.NewMockHistoryRepository(t) | |||
| 211 | + | const historyURL = "https://example.com/cancel-close" | |||
| 212 | + | ||||
| 213 | + | findStarted := make(chan struct{}) | |||
| 214 | + | var findOnce sync.Once | |||
| 215 | + | repo.EXPECT().FindByURL(mock.Anything, historyURL).RunAndReturn( | |||
| 216 | + | func(ctx context.Context, _ string) (*entity.HistoryEntry, error) { | |||
| 217 | + | first := false | |||
| 218 | + | findOnce.Do(func() { first = true }) | |||
| 219 | + | if first { | |||
| 220 | + | close(findStarted) | |||
| 221 | + | <-ctx.Done() | |||
| 222 | + | return nil, ctx.Err() | |||
| 223 | + | } | |||
| 224 | + | return nil, nil | |||
| 225 | + | }, | |||
| 226 | + | ).Twice() | |||
| 227 | + | repo.EXPECT().Save(mock.Anything, mock.MatchedBy(func(entry *entity.HistoryEntry) bool { | |||
| 228 | + | return entry.URL == historyURL && entry.VisitCount == 1 | |||
| 229 | + | })).Return(nil).Once() | |||
| 230 | + | ||||
| 231 | + | uc := NewHistoryRecorderUseCase(repo, sink) | |||
| 232 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |||
| 233 | + | ||||
| 234 | + | select { | |||
| 235 | + | case <-findStarted: | |||
| 236 | + | case <-time.After(time.Second): | |||
| 237 | + | t.Fatal("timed out waiting for periodic history flush to start") | |||
| 238 | + | } | |||
| 239 | + | ||||
| 240 | + | closed := make(chan struct{}) | |||
| 241 | + | go func() { | |||
| 242 | + | uc.Close() | |||
| 243 | + | close(closed) | |||
| 244 | + | }() | |||
| 245 | + | ||||
| 246 | + | select { | |||
| 247 | + | case <-closed: | |||
| 248 | + | case <-time.After(time.Second): | |||
| 249 | + | t.Fatal("timed out waiting for history recorder shutdown") | |||
| 250 | + | } | |||
| 251 | + | ||||
| 252 | + | changes := sink.snapshot() | |||
| 253 | + | require.Len(t, changes, 1) | |||
| 254 | + | require.Equal(t, 1, changes[0].VisitCount) | |||
| 255 | + | } | |||
| 256 | + | ||||
| 207 | func TestHistoryRecorder_FailedPersistencePublishesNoChange(t *testing.T) { | 257 | func TestHistoryRecorder_FailedPersistencePublishesNoChange(t *testing.T) { | ||
| 208 | ctx := context.Background() | 258 | ctx := context.Background() | ||
| 209 | sink := &recordingHistoryChangeSink{} | 259 | sink := &recordingHistoryChangeSink{} | ||
| @@ -328,6 +378,28 @@ func TestHistoryRecorder_RecordHistory_DedupIsPerPane(t *testing.T) { | |||||
| 328 | require.Equal(t, 2, changes[0].VisitCount) | 378 | require.Equal(t, 2, changes[0].VisitCount) | ||
| 329 | } | 379 | } | ||
| 330 | 380 | ||||
| 381 | + | func TestHistoryRecorder_ClearPaneHistorySerializesWithRecorderAndAllowsPaneReuse(t *testing.T) { | |||
| 382 | + | ctx := context.Background() | |||
| 383 | + | sink := &recordingHistoryChangeSink{} | |||
| 384 | + | repo := repomocks.NewMockHistoryRepository(t) | |||
| 385 | + | const historyURL = "https://example.com/reused-pane" | |||
| 386 | + | ||||
| 387 | + | repo.EXPECT().FindByURL(mock.Anything, historyURL).Return(nil, nil).Once() | |||
| 388 | + | repo.EXPECT().Save(mock.Anything, mock.MatchedBy(func(entry *entity.HistoryEntry) bool { | |||
| 389 | + | return entry.URL == historyURL && entry.VisitCount == 2 | |||
| 390 | + | })).Return(nil).Once() | |||
| 391 | + | ||||
| 392 | + | uc := NewHistoryRecorderUseCase(repo, sink) | |||
| 393 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |||
| 394 | + | uc.ClearPaneHistory("pane-1") | |||
| 395 | + | uc.RecordHistory(ctx, "pane-1", historyURL) | |||
| 396 | + | uc.Close() | |||
| 397 | + | ||||
| 398 | + | changes := sink.snapshot() | |||
| 399 | + | require.Len(t, changes, 1) | |||
| 400 | + | require.Equal(t, 2, changes[0].VisitCount) | |||
| 401 | + | } | |||
| 402 | + | ||||
| 331 | func TestHistoryRecorder_FlushPendingHistoryTracksPartialFallbackVisitWrites(t *testing.T) { | 403 | func TestHistoryRecorder_FlushPendingHistoryTracksPartialFallbackVisitWrites(t *testing.T) { | ||
| 332 | ctx := context.Background() | 404 | ctx := context.Background() | ||
| 333 | sink := &recordingHistoryChangeSink{} | 405 | sink := &recordingHistoryChangeSink{} | ||
M internal/ui/browser_window_history_sidebar_test.go +55 -5
| @@ -2,7 +2,10 @@ package ui | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "reflect" | |
| 6 | + | "sync" | |
| 5 | 7 | "testing" | |
| 8 | + | "unsafe" | |
| 6 | 9 | ||
| 7 | 10 | "github.com/bnema/dumber/internal/application/dto" | |
| 8 | 11 | "github.com/bnema/dumber/internal/application/usecase" | |
| @@ -15,6 +18,7 @@ import ( | |||
| 15 | 18 | "github.com/bnema/dumber/internal/ui/dispatcher" | |
| 16 | 19 | "github.com/bnema/dumber/internal/ui/input" | |
| 17 | 20 | "github.com/bnema/dumber/internal/ui/window" | |
| 21 | + | "github.com/bnema/puregotk/v4/glib" | |
| 18 | 22 | "github.com/stretchr/testify/assert" | |
| 19 | 23 | "github.com/stretchr/testify/require" | |
| 20 | 24 | ) | |
| @@ -957,12 +961,29 @@ func TestApp_RefreshVisibleHistorySidebars_OnlyVisibleInitializedWindows(t *test | |||
| 957 | 961 | visibleOne := &recordingHistorySidebarReloader{} | |
| 958 | 962 | visibleTwo := &recordingHistorySidebarReloader{} | |
| 959 | 963 | hidden := &recordingHistorySidebarReloader{} | |
| 964 | + | fallbackSidebar := &component.HistorySidebar{} | |
| 965 | + | var fallbackMu sync.Mutex | |
| 966 | + | var fallbackIdle []glib.SourceFunc | |
| 967 | + | var fallbackTimers []glib.SourceFunc | |
| 968 | + | setComponentHistorySidebarField(t, fallbackSidebar, "visible", true) | |
| 969 | + | setComponentHistorySidebarField(t, fallbackSidebar, "idleScheduler", func(cb glib.SourceFunc) { | |
| 970 | + | fallbackMu.Lock() | |
| 971 | + | defer fallbackMu.Unlock() | |
| 972 | + | fallbackIdle = append(fallbackIdle, cb) | |
| 973 | + | }) | |
| 974 | + | setComponentHistorySidebarField(t, fallbackSidebar, "timeoutAdd", func(_ uint, cb glib.SourceFunc) uint { | |
| 975 | + | fallbackMu.Lock() | |
| 976 | + | defer fallbackMu.Unlock() | |
| 977 | + | fallbackTimers = append(fallbackTimers, cb) | |
| 978 | + | return uint(100 + len(fallbackTimers)) | |
| 979 | + | }) | |
| 960 | 980 | app := &App{browserWindows: map[string]*browserWindow{ | |
| 961 | - | "visible-1": {id: "visible-1", sidebarVisible: true, historySidebarReloader: visibleOne}, | |
| 962 | - | "hidden": {id: "hidden", sidebarVisible: false, historySidebarReloader: hidden}, | |
| 963 | - | "uninitialized": {id: "uninitialized", sidebarVisible: true}, | |
| 964 | - | "visible-2": {id: "visible-2", sidebarVisible: true, historySidebarReloader: visibleTwo}, | |
| 965 | - | "destroyed-no-op": nil, | |
| 981 | + | "visible-1": {id: "visible-1", sidebarVisible: true, historySidebarReloader: visibleOne}, | |
| 982 | + | "hidden": {id: "hidden", sidebarVisible: false, historySidebarReloader: hidden}, | |
| 983 | + | "uninitialized": {id: "uninitialized", sidebarVisible: true}, | |
| 984 | + | "visible-2": {id: "visible-2", sidebarVisible: true, historySidebarReloader: visibleTwo}, | |
| 985 | + | "visible-fallback": {id: "visible-fallback", sidebarVisible: true, historySidebar: fallbackSidebar}, | |
| 986 | + | "destroyed-no-op": nil, | |
| 966 | 987 | }} | |
| 967 | 988 | ||
| 968 | 989 | require.NotPanics(t, func() { | |
| @@ -974,4 +995,33 @@ func TestApp_RefreshVisibleHistorySidebars_OnlyVisibleInitializedWindows(t *test | |||
| 974 | 995 | assert.Zero(t, hidden.calls) | |
| 975 | 996 | assert.Equal(t, []string{"clear"}, visibleOne.reasons) | |
| 976 | 997 | assert.Equal(t, []string{"clear"}, visibleTwo.reasons) | |
| 998 | + | ||
| 999 | + | fallbackMu.Lock() | |
| 1000 | + | require.Len(t, fallbackIdle, 1) | |
| 1001 | + | idleCb := fallbackIdle[0] | |
| 1002 | + | fallbackMu.Unlock() | |
| 1003 | + | assert.False(t, idleCb(0)) | |
| 1004 | + | ||
| 1005 | + | fallbackMu.Lock() | |
| 1006 | + | require.Len(t, fallbackTimers, 1) | |
| 1007 | + | timerCb := fallbackTimers[0] | |
| 1008 | + | fallbackMu.Unlock() | |
| 1009 | + | assert.False(t, timerCb(0)) | |
| 1010 | + | assert.True(t, getComponentHistorySidebarBoolField(t, fallbackSidebar, "loadStarted")) | |
| 1011 | + | } | |
| 1012 | + | ||
| 1013 | + | func setComponentHistorySidebarField(t *testing.T, hs *component.HistorySidebar, name string, value any) { | |
| 1014 | + | t.Helper() | |
| 1015 | + | rv := reflect.ValueOf(hs).Elem() | |
| 1016 | + | field := rv.FieldByName(name) | |
| 1017 | + | require.Truef(t, field.IsValid(), "component.HistorySidebar.%s field missing", name) | |
| 1018 | + | reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Set(reflect.ValueOf(value)) | |
| 1019 | + | } | |
| 1020 | + | ||
| 1021 | + | func getComponentHistorySidebarBoolField(t *testing.T, hs *component.HistorySidebar, name string) bool { | |
| 1022 | + | t.Helper() | |
| 1023 | + | rv := reflect.ValueOf(hs).Elem() | |
| 1024 | + | field := rv.FieldByName(name) | |
| 1025 | + | require.Truef(t, field.IsValid(), "component.HistorySidebar.%s field missing", name) | |
| 1026 | + | return reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Bool() | |
| 977 | 1027 | } | |
| @@ -2,7 +2,10 @@ package ui | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "reflect" | |||
| 6 | + | "sync" | |||
| 5 | "testing" | 7 | "testing" | ||
| 8 | + | "unsafe" | |||
| 6 | 9 | ||||
| 7 | "github.com/bnema/dumber/internal/application/dto" | 10 | "github.com/bnema/dumber/internal/application/dto" | ||
| 8 | "github.com/bnema/dumber/internal/application/usecase" | 11 | "github.com/bnema/dumber/internal/application/usecase" | ||
| @@ -15,6 +18,7 @@ import ( | |||||
| 15 | "github.com/bnema/dumber/internal/ui/dispatcher" | 18 | "github.com/bnema/dumber/internal/ui/dispatcher" | ||
| 16 | "github.com/bnema/dumber/internal/ui/input" | 19 | "github.com/bnema/dumber/internal/ui/input" | ||
| 17 | "github.com/bnema/dumber/internal/ui/window" | 20 | "github.com/bnema/dumber/internal/ui/window" | ||
| 21 | + | "github.com/bnema/puregotk/v4/glib" | |||
| 18 | "github.com/stretchr/testify/assert" | 22 | "github.com/stretchr/testify/assert" | ||
| 19 | "github.com/stretchr/testify/require" | 23 | "github.com/stretchr/testify/require" | ||
| 20 | ) | 24 | ) | ||
| @@ -957,12 +961,29 @@ func TestApp_RefreshVisibleHistorySidebars_OnlyVisibleInitializedWindows(t *test | |||||
| 957 | visibleOne := &recordingHistorySidebarReloader{} | 961 | visibleOne := &recordingHistorySidebarReloader{} | ||
| 958 | visibleTwo := &recordingHistorySidebarReloader{} | 962 | visibleTwo := &recordingHistorySidebarReloader{} | ||
| 959 | hidden := &recordingHistorySidebarReloader{} | 963 | hidden := &recordingHistorySidebarReloader{} | ||
| 964 | + | fallbackSidebar := &component.HistorySidebar{} | |||
| 965 | + | var fallbackMu sync.Mutex | |||
| 966 | + | var fallbackIdle []glib.SourceFunc | |||
| 967 | + | var fallbackTimers []glib.SourceFunc | |||
| 968 | + | setComponentHistorySidebarField(t, fallbackSidebar, "visible", true) | |||
| 969 | + | setComponentHistorySidebarField(t, fallbackSidebar, "idleScheduler", func(cb glib.SourceFunc) { | |||
| 970 | + | fallbackMu.Lock() | |||
| 971 | + | defer fallbackMu.Unlock() | |||
| 972 | + | fallbackIdle = append(fallbackIdle, cb) | |||
| 973 | + | }) | |||
| 974 | + | setComponentHistorySidebarField(t, fallbackSidebar, "timeoutAdd", func(_ uint, cb glib.SourceFunc) uint { | |||
| 975 | + | fallbackMu.Lock() | |||
| 976 | + | defer fallbackMu.Unlock() | |||
| 977 | + | fallbackTimers = append(fallbackTimers, cb) | |||
| 978 | + | return uint(100 + len(fallbackTimers)) | |||
| 979 | + | }) | |||
| 960 | app := &App{browserWindows: map[string]*browserWindow{ | 980 | app := &App{browserWindows: map[string]*browserWindow{ | ||
| 961 | - | "visible-1": {id: "visible-1", sidebarVisible: true, historySidebarReloader: visibleOne}, | 981 | + | "visible-1": {id: "visible-1", sidebarVisible: true, historySidebarReloader: visibleOne}, |
| 962 | - | "hidden": {id: "hidden", sidebarVisible: false, historySidebarReloader: hidden}, | 982 | + | "hidden": {id: "hidden", sidebarVisible: false, historySidebarReloader: hidden}, |
| 963 | - | "uninitialized": {id: "uninitialized", sidebarVisible: true}, | 983 | + | "uninitialized": {id: "uninitialized", sidebarVisible: true}, |
| 964 | - | "visible-2": {id: "visible-2", sidebarVisible: true, historySidebarReloader: visibleTwo}, | 984 | + | "visible-2": {id: "visible-2", sidebarVisible: true, historySidebarReloader: visibleTwo}, |
| 965 | - | "destroyed-no-op": nil, | 985 | + | "visible-fallback": {id: "visible-fallback", sidebarVisible: true, historySidebar: fallbackSidebar}, |
| 986 | + | "destroyed-no-op": nil, | |||
| 966 | }} | 987 | }} | ||
| 967 | 988 | ||||
| 968 | require.NotPanics(t, func() { | 989 | require.NotPanics(t, func() { | ||
| @@ -974,4 +995,33 @@ func TestApp_RefreshVisibleHistorySidebars_OnlyVisibleInitializedWindows(t *test | |||||
| 974 | assert.Zero(t, hidden.calls) | 995 | assert.Zero(t, hidden.calls) | ||
| 975 | assert.Equal(t, []string{"clear"}, visibleOne.reasons) | 996 | assert.Equal(t, []string{"clear"}, visibleOne.reasons) | ||
| 976 | assert.Equal(t, []string{"clear"}, visibleTwo.reasons) | 997 | assert.Equal(t, []string{"clear"}, visibleTwo.reasons) | ||
| 998 | + | ||||
| 999 | + | fallbackMu.Lock() | |||
| 1000 | + | require.Len(t, fallbackIdle, 1) | |||
| 1001 | + | idleCb := fallbackIdle[0] | |||
| 1002 | + | fallbackMu.Unlock() | |||
| 1003 | + | assert.False(t, idleCb(0)) | |||
| 1004 | + | ||||
| 1005 | + | fallbackMu.Lock() | |||
| 1006 | + | require.Len(t, fallbackTimers, 1) | |||
| 1007 | + | timerCb := fallbackTimers[0] | |||
| 1008 | + | fallbackMu.Unlock() | |||
| 1009 | + | assert.False(t, timerCb(0)) | |||
| 1010 | + | assert.True(t, getComponentHistorySidebarBoolField(t, fallbackSidebar, "loadStarted")) | |||
| 1011 | + | } | |||
| 1012 | + | ||||
| 1013 | + | func setComponentHistorySidebarField(t *testing.T, hs *component.HistorySidebar, name string, value any) { | |||
| 1014 | + | t.Helper() | |||
| 1015 | + | rv := reflect.ValueOf(hs).Elem() | |||
| 1016 | + | field := rv.FieldByName(name) | |||
| 1017 | + | require.Truef(t, field.IsValid(), "component.HistorySidebar.%s field missing", name) | |||
| 1018 | + | reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Set(reflect.ValueOf(value)) | |||
| 1019 | + | } | |||
| 1020 | + | ||||
| 1021 | + | func getComponentHistorySidebarBoolField(t *testing.T, hs *component.HistorySidebar, name string) bool { | |||
| 1022 | + | t.Helper() | |||
| 1023 | + | rv := reflect.ValueOf(hs).Elem() | |||
| 1024 | + | field := rv.FieldByName(name) | |||
| 1025 | + | require.Truef(t, field.IsValid(), "component.HistorySidebar.%s field missing", name) | |||
| 1026 | + | return reflect.NewAt(field.Type(), unsafe.Pointer(field.UnsafeAddr())).Elem().Bool() | |||
| 977 | } | 1027 | } | ||
M internal/ui/component/history_sidebar.go +5 -2
| @@ -304,7 +304,7 @@ func (hs *HistorySidebar) Show() { | |||
| 304 | 304 | // Hide hides the sidebar. | |
| 305 | 305 | func (hs *HistorySidebar) Hide() { | |
| 306 | 306 | hs.mu.Lock() | |
| 307 | - | if hs.outerBox == nil || hs.destroyed { | |
| 307 | + | if hs.destroyed { | |
| 308 | 308 | hs.mu.Unlock() | |
| 309 | 309 | return | |
| 310 | 310 | } | |
| @@ -312,7 +312,10 @@ func (hs *HistorySidebar) Hide() { | |||
| 312 | 312 | reloadTimerID := hs.cancelReloadDebounceLocked() | |
| 313 | 313 | tickerID := hs.relativeTimeTicker | |
| 314 | 314 | hs.relativeTimeTicker = 0 | |
| 315 | - | hs.outerBox.SetVisible(false) | |
| 315 | + | hs.clearRelativeTimeBindingsLocked() | |
| 316 | + | if hs.outerBox != nil { | |
| 317 | + | hs.outerBox.SetVisible(false) | |
| 318 | + | } | |
| 316 | 319 | hs.visible = false | |
| 317 | 320 | hs.mu.Unlock() | |
| 318 | 321 | ||
| @@ -304,7 +304,7 @@ func (hs *HistorySidebar) Show() { | |||||
| 304 | // Hide hides the sidebar. | 304 | // Hide hides the sidebar. | ||
| 305 | func (hs *HistorySidebar) Hide() { | 305 | func (hs *HistorySidebar) Hide() { | ||
| 306 | hs.mu.Lock() | 306 | hs.mu.Lock() | ||
| 307 | - | if hs.outerBox == nil || hs.destroyed { | 307 | + | if hs.destroyed { |
| 308 | hs.mu.Unlock() | 308 | hs.mu.Unlock() | ||
| 309 | return | 309 | return | ||
| 310 | } | 310 | } | ||
| @@ -312,7 +312,10 @@ func (hs *HistorySidebar) Hide() { | |||||
| 312 | reloadTimerID := hs.cancelReloadDebounceLocked() | 312 | reloadTimerID := hs.cancelReloadDebounceLocked() | ||
| 313 | tickerID := hs.relativeTimeTicker | 313 | tickerID := hs.relativeTimeTicker | ||
| 314 | hs.relativeTimeTicker = 0 | 314 | hs.relativeTimeTicker = 0 | ||
| 315 | - | hs.outerBox.SetVisible(false) | 315 | + | hs.clearRelativeTimeBindingsLocked() |
| 316 | + | if hs.outerBox != nil { | |||
| 317 | + | hs.outerBox.SetVisible(false) | |||
| 318 | + | } | |||
| 316 | hs.visible = false | 319 | hs.visible = false | ||
| 317 | hs.mu.Unlock() | 320 | hs.mu.Unlock() | ||
| 318 | 321 | ||||
M internal/ui/component/history_sidebar_loading_search.go +1 -1
| @@ -287,7 +287,7 @@ func (hs *HistorySidebar) onSearchChanged() { | |||
| 287 | 287 | hs.applyFilter() | |
| 288 | 288 | return false | |
| 289 | 289 | }) | |
| 290 | - | timerID := glib.TimeoutAdd(uint(sidebarSearchDebounceMs), &filterCb, 0) | |
| 290 | + | timerID := hs.addTimeout(sidebarSearchDebounceMs, filterCb) | |
| 291 | 291 | ||
| 292 | 292 | hs.mu.Lock() | |
| 293 | 293 | if hs.destroyed { | |
| @@ -287,7 +287,7 @@ func (hs *HistorySidebar) onSearchChanged() { | |||||
| 287 | hs.applyFilter() | 287 | hs.applyFilter() | ||
| 288 | return false | 288 | return false | ||
| 289 | }) | 289 | }) | ||
| 290 | - | timerID := glib.TimeoutAdd(uint(sidebarSearchDebounceMs), &filterCb, 0) | 290 | + | timerID := hs.addTimeout(sidebarSearchDebounceMs, filterCb) |
| 291 | 291 | ||||
| 292 | hs.mu.Lock() | 292 | hs.mu.Lock() | ||
| 293 | if hs.destroyed { | 293 | if hs.destroyed { | ||
M internal/ui/component/history_sidebar_reactivity_test.go +25 -0
| @@ -166,6 +166,31 @@ func TestHistorySidebar_DestroyCancelsReloadTickerAndClearsBindings(t *testing.T | |||
| 166 | 166 | assert.Contains(t, h.removed, uint(52)) | |
| 167 | 167 | } | |
| 168 | 168 | ||
| 169 | + | func TestHistorySidebar_HideCancelsReloadTickerAndClearsBindingsWithoutDestroying(t *testing.T) { | |
| 170 | + | h := newSidebarReactivityHarness() | |
| 171 | + | h.hs.visible = true | |
| 172 | + | h.hs.reloadDebounceTimer = 61 | |
| 173 | + | h.hs.relativeTimeTicker = 62 | |
| 174 | + | h.hs.relativeTimeLabelBinds = []relativeTimeLabelBinding{{lastVisited: h.hs.currentTime()}} | |
| 175 | + | ||
| 176 | + | h.hs.Hide() | |
| 177 | + | ||
| 178 | + | h.hs.mu.RLock() | |
| 179 | + | destroyed := h.hs.destroyed | |
| 180 | + | visible := h.hs.visible | |
| 181 | + | reloadTimer := h.hs.reloadDebounceTimer | |
| 182 | + | ticker := h.hs.relativeTimeTicker | |
| 183 | + | bindings := h.hs.relativeTimeLabelBinds | |
| 184 | + | h.hs.mu.RUnlock() | |
| 185 | + | assert.False(t, destroyed) | |
| 186 | + | assert.False(t, visible) | |
| 187 | + | assert.Zero(t, reloadTimer) | |
| 188 | + | assert.Zero(t, ticker) | |
| 189 | + | assert.Empty(t, bindings) | |
| 190 | + | assert.Contains(t, h.removed, uint(61)) | |
| 191 | + | assert.Contains(t, h.removed, uint(62)) | |
| 192 | + | } | |
| 193 | + | ||
| 169 | 194 | func TestHistorySidebar_RelativeTimeBindingsLifecycleAndTicker(t *testing.T) { | |
| 170 | 195 | h := newSidebarReactivityHarness() | |
| 171 | 196 | h.hs.visible = true | |
| @@ -166,6 +166,31 @@ func TestHistorySidebar_DestroyCancelsReloadTickerAndClearsBindings(t *testing.T | |||||
| 166 | assert.Contains(t, h.removed, uint(52)) | 166 | assert.Contains(t, h.removed, uint(52)) | ||
| 167 | } | 167 | } | ||
| 168 | 168 | ||||
| 169 | + | func TestHistorySidebar_HideCancelsReloadTickerAndClearsBindingsWithoutDestroying(t *testing.T) { | |||
| 170 | + | h := newSidebarReactivityHarness() | |||
| 171 | + | h.hs.visible = true | |||
| 172 | + | h.hs.reloadDebounceTimer = 61 | |||
| 173 | + | h.hs.relativeTimeTicker = 62 | |||
| 174 | + | h.hs.relativeTimeLabelBinds = []relativeTimeLabelBinding{{lastVisited: h.hs.currentTime()}} | |||
| 175 | + | ||||
| 176 | + | h.hs.Hide() | |||
| 177 | + | ||||
| 178 | + | h.hs.mu.RLock() | |||
| 179 | + | destroyed := h.hs.destroyed | |||
| 180 | + | visible := h.hs.visible | |||
| 181 | + | reloadTimer := h.hs.reloadDebounceTimer | |||
| 182 | + | ticker := h.hs.relativeTimeTicker | |||
| 183 | + | bindings := h.hs.relativeTimeLabelBinds | |||
| 184 | + | h.hs.mu.RUnlock() | |||
| 185 | + | assert.False(t, destroyed) | |||
| 186 | + | assert.False(t, visible) | |||
| 187 | + | assert.Zero(t, reloadTimer) | |||
| 188 | + | assert.Zero(t, ticker) | |||
| 189 | + | assert.Empty(t, bindings) | |||
| 190 | + | assert.Contains(t, h.removed, uint(61)) | |||
| 191 | + | assert.Contains(t, h.removed, uint(62)) | |||
| 192 | + | } | |||
| 193 | + | ||||
| 169 | func TestHistorySidebar_RelativeTimeBindingsLifecycleAndTicker(t *testing.T) { | 194 | func TestHistorySidebar_RelativeTimeBindingsLifecycleAndTicker(t *testing.T) { | ||
| 170 | h := newSidebarReactivityHarness() | 195 | h := newSidebarReactivityHarness() | ||
| 171 | h.hs.visible = true | 196 | h.hs.visible = true | ||