diff --git a/backend/explore/emit_test.go b/backend/explore/emit_test.go new file mode 100644 index 0000000..78c95ae --- /dev/null +++ b/backend/explore/emit_test.go @@ -0,0 +1,184 @@ +package explore + +import ( + "context" + "log/slog" + "testing" + + "yellowjacket/backend/database" + "yellowjacket/backend/events" +) + +// The index status used to be pushed on a 3 s ticker for the life of +// the process, with a byte-identical payload once the index was ready. +// The frontend's handler assigns it to a @state field, so every tick +// re-rendered the whole settings page — a cached view that never +// unmounts — saying nothing (`perf.M6` / `H-14`). +// +// The ticker is gone and emitStatus suppresses an unchanged payload, so +// what these cover is the pair of properties that replaced it: a change +// still gets through, and a non-change does not. + +// setupRecordedIndex builds a SearchIndex with an event sink installed, +// so what the frontend would receive is assertable in-process. +func setupRecordedIndex(t *testing.T) (*SearchIndex, *events.Recorder) { + t.Helper() + + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, slog.Default()) + + rec := events.NewRecorder() + si.SetContext(events.WithSink(context.Background(), rec)) + + return si, rec +} + +func TestEmitStatus_SuppressesAnUnchangedPayload(t *testing.T) { + si, rec := setupRecordedIndex(t) + + // SetContext emits once via refreshStatusCounts; everything after + // this describes the same state. + rec.Reset() + + for range 5 { + si.emitStatus() + } + + if got := rec.Count(events.IndexStatusChanged); got != 0 { + t.Fatalf( + "emitted %d IndexStatusChanged for an unchanged status, want 0", + got, + ) + } +} + +func TestEmitStatus_EmitsOnChange(t *testing.T) { + si, rec := setupRecordedIndex(t) + rec.Reset() + + si.setTierStatus("artists", "running", 100, 10) + + if got := rec.Count(events.IndexStatusChanged); got != 1 { + t.Fatalf("emitted %d IndexStatusChanged for a new tier, want 1", got) + } + + // Progress within the tier is a change too — this is what a build + // reports, and dropping it would freeze the progress bar. + si.setTierStatus("artists", "running", 100, 20) + + if got := rec.Count(events.IndexStatusChanged); got != 2 { + t.Fatalf("emitted %d after progress moved, want 2", got) + } + + // The same call again is not. + si.setTierStatus("artists", "running", 100, 20) + + if got := rec.Count(events.IndexStatusChanged); got != 2 { + t.Fatalf("emitted %d after a repeated status, want 2", got) + } +} + +// The status carries a slice, so a snapshot that aliases it would +// compare equal to a later mutation of the same backing array and the +// change would never be emitted. +func TestEmitStatus_SnapshotDoesNotAliasTiers(t *testing.T) { + si, rec := setupRecordedIndex(t) + + si.setTierStatus("artists", "running", 100, 10) + rec.Reset() + + si.setTierStatus("artists", "complete", 100, 100) + + if got := rec.Count(events.IndexStatusChanged); got != 1 { + t.Fatalf( + "emitted %d when a tier completed in place, want 1", got, + ) + } + + status, ok := rec.Last(events.IndexStatusChanged) + if !ok { + t.Fatal("no IndexStatusChanged recorded") + } + + payload, ok := status.Payload().(IndexStatus) + if !ok { + t.Fatalf("payload is %T, want explore.IndexStatus", status.Payload()) + } + + if len(payload.Tiers) != 1 || payload.Tiers[0].State != "complete" { + t.Fatalf("payload tiers = %+v, want one complete tier", payload.Tiers) + } +} + +// `Building` and `Ready` are derived inside emitStatus from fields the +// dedupe never sees directly, so a transition in either has to survive +// it. A build ending is the case that matters: syncIndexJob only +// resolves the job in the registry on a sync reporting Building false, +// and suppressing that left the header badge saying "Building search +// index" over an index the settings page called ready. +func TestEmitStatus_BuildingTransitionsAreNotSuppressed(t *testing.T) { + si, rec := setupRecordedIndex(t) + + si.mu.Lock() + _, si.cancel = context.WithCancel(context.Background()) + si.mu.Unlock() + + si.emitStatus() + rec.Reset() + + // Nothing else changed, so this one is noise. + si.emitStatus() + + if got := rec.Count(events.IndexStatusChanged); got != 0 { + t.Fatalf("emitted %d while still building unchanged, want 0", got) + } + + si.mu.Lock() + si.cancel = nil + si.mu.Unlock() + + si.emitStatus() + + ev, ok := rec.Last(events.IndexStatusChanged) + if !ok { + t.Fatalf("a build ending emitted nothing; got %v", rec.Names()) + } + + payload, ok := ev.Payload().(IndexStatus) + if !ok { + t.Fatalf("payload is %T, want explore.IndexStatus", ev.Payload()) + } + + if payload.Building { + t.Fatal("payload still reports building after the build ended") + } +} + +// Becoming ready used to be the one status mutation with no emit behind +// it; the ticker carried it, so removing the ticker without an explicit +// emit would have left the settings page reading "not ready" forever +// over a fully built index. +func TestEmitStatus_ReadyIsAChange(t *testing.T) { + si, rec := setupRecordedIndex(t) + rec.Reset() + + si.mu.Lock() + si.ready = true + si.mu.Unlock() + + si.emitStatus() + + ev, ok := rec.Last(events.IndexStatusChanged) + if !ok { + t.Fatalf("becoming ready emitted nothing; got %v", rec.Names()) + } + + payload, ok := ev.Payload().(IndexStatus) + if !ok { + t.Fatalf("payload is %T, want explore.IndexStatus", ev.Payload()) + } + + if !payload.Ready { + t.Fatal("payload reports not ready after si.ready was set") + } +} diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 7cd54fa..37d601e 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -199,6 +199,13 @@ type SearchIndex struct { // Build status tracking — read by GetIndexStatus for the UI. buildStatus IndexStatus + // lastEmitted is the status most recently pushed to the frontend, so + // an unchanged one can be dropped rather than re-rendering the whole + // settings page for nothing. It has its own mutex: emitStatus is + // called from paths that already hold mu for reading. + emitMu sync.Mutex + lastEmitted *IndexStatus + // jobs is the background job registry; buildPaused records that the // user paused the build, distinguishing a deliberate stop from a // build that merely finished. Both are protected by mu. @@ -267,24 +274,16 @@ func (si *SearchIndex) SetContext(ctx context.Context) { si.mu.Unlock() // Load current row counts + last-built timestamp from DB. + // + // There is deliberately no ticker here. This used to emit the status + // every 3 seconds for the life of the process, with a byte-identical + // payload once the index was ready — which re-rendered the whole of + // `config-page` on every tick, since it is a cached view that never + // unmounts. Every path that mutates the status already calls + // emitStatus, that call now suppresses an unchanged payload, and the + // frontend seeds itself with GetIndexStatus() on connect rather than + // waiting for the next tick. si.refreshStatusCounts() - - // Start a background ticker that emits status every 3 seconds. - // This replaces frontend polling — the Wails binding dispatcher - // can be blocked by other calls, but EventsEmit bypasses it. - go func() { - ticker := time.NewTicker(3 * time.Second) - defer ticker.Stop() - - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - si.emitStatus() - } - } - }() } // EnsureArtistDiscography lazily fetches an artist's top release groups @@ -522,6 +521,14 @@ func (si *SearchIndex) StartBuild(ctx context.Context) { si.cancel = nil si.mu.Unlock() + // `Building` is derived from si.cancel, so clearing it is a + // status change and has to say so. This is what resolves the + // job in the registry — syncIndexJob only finishes a job on a + // sync that reports Building false, and without this line the + // header badge reads "Building search index" over an index the + // settings page calls ready. + si.emitStatus() + close(si.done) // The index rows (and their popularities) may have changed, so @@ -677,7 +684,38 @@ func (si *SearchIndex) setTierDetail(name, state string, total, completed int, d si.emitStatus() } -// emitStatus pushes the current index status to the frontend via Wails event. +// sameStatusAs reports whether two statuses would render identically. +// IndexStatus holds a slice, so it is not comparable with ==. +func (s IndexStatus) sameStatusAs(o IndexStatus) bool { + if s.Building != o.Building || + s.Ready != o.Ready || + s.LastBuilt != o.LastBuilt || + s.Artists != o.Artists || + s.Recordings != o.Recordings || + s.ReleaseGroups != o.ReleaseGroups || + s.TotalRows != o.TotalRows || + len(s.Tiers) != len(o.Tiers) { + return false + } + + for i := range s.Tiers { + if s.Tiers[i] != o.Tiers[i] { + return false + } + } + + return true +} + +// emitStatus pushes the current index status to the frontend via Wails +// event — but only when it differs from the last one pushed. +// +// The status is emitted from every path that touches it, several of +// which report progress in a tight loop, and the frontend's handler +// assigns to a @state field: an identical payload is therefore a full +// re-render of a 2 000-line template saying nothing. Deduplicating +// here rather than at the call sites means no future emitter has to +// remember (`perf.M6` / `H-14`). func (si *SearchIndex) emitStatus() { if si.runtimeCtx == nil { return @@ -689,6 +727,23 @@ func (si *SearchIndex) emitStatus() { status.Building = si.cancel != nil si.mu.RUnlock() + si.emitMu.Lock() + unchanged := si.lastEmitted != nil && + si.lastEmitted.sameStatusAs(status) + + if !unchanged { + snapshot := status + snapshot.Tiers = append( + []TierStatus(nil), status.Tiers..., + ) + si.lastEmitted = &snapshot + } + si.emitMu.Unlock() + + if unchanged { + return + } + events.Emit(si.runtimeCtx, events.IndexStatusChanged, status) // Mirror into the shared job registry. Every status mutation goes @@ -2644,6 +2699,13 @@ func (si *SearchIndex) MarkReadyIfPopulated() { si.logger.Info("search index: using existing index", "entries", count) + // Becoming ready is a change the UI has to see, and this was the one + // path that mutated the status without saying so — the 3 s ticker + // carried it, invisibly, which is why removing the ticker without + // this line would have left the settings page reading "not ready" + // over a fully built index. + si.emitStatus() + if !championBuilt { si.scheduleChampionRebuild() }