From 578494dc435ae9acbc440eca2072bea471d87a86 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 8 May 2026 08:15:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(web):=20break=20sse=E2=86=94sync=20circular?= =?UTF-8?q?=20dep=20via=20callback=20inversion=20(TASK-1242)=20(#445)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rolldown's stricter import diagnostics (introduced in TASK-1238 via Vite 8) flagged that `sync.svelte.ts` was both statically imported from 5 routes/components AND dynamically imported from `sse.svelte.ts`. Rolldown's warning: [INEFFECTIVE_DYNAMIC_IMPORT] sync.svelte.ts is dynamically imported by sse.svelte.ts but also statically imported by [5 files], dynamic import will not move module into another chunk. The original task body's first-cut fix ("convert the dynamic import to static") was wrong: the dynamic import wasn't there for code- splitting — the inline comment said "to avoid circular dependency", and indeed sync.svelte.ts statically imports `sseService`, so a reverse static import would close the cycle. Real fix: callback inversion. Mirror the existing `onItemEvent` pattern by adding `onSyncRequired(callback)` to sseService. Have syncService subscribe in its `init()` instead of sseService calling syncService directly. Net result: Before: sse →(dynamic import)→ sync ──╮ sync ─(static import)─→ sse ←──╯ (circular, papered over) After: sse exposes onSyncRequired() sync subscribes on init(), receives sync_required pings sse has zero imports of sync.svelte.ts (static or dynamic) Behavior is identical: • sse_required server event still triggers `syncService.triggerSync()` • sync.svelte.ts still owns the sync coordination decision tree • Init order is fine — both modules are evaluated as singletons at module-load time; subscription happens during syncService.init() which workspace +layout.svelte calls in onMount, well after both modules have settled Verified: • `npm run build` — INEFFECTIVE_DYNAMIC_IMPORT warning is gone; Rolldown bundle build went from 5.91s → 2.90s as a bonus • `make check` — golangci-lint + go test + npm run build + svelte-check, 0 errors, same 6 pre-existing warnings • Manual UI verify — SSE still works (collection page real-time updates, child item progress, comments/reactions, timeline) Spawned [[TASK-1243]] for a separate pre-existing bug surfaced during this manual verify: the item DETAIL page never subscribed to sseService.onItemEvent, so live title/field updates from other clients don't propagate until manual refresh. Out of scope for this PR. --- web/src/lib/services/sse.svelte.ts | 34 ++++++++++++++++++++++++----- web/src/lib/services/sync.svelte.ts | 12 ++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/web/src/lib/services/sse.svelte.ts b/web/src/lib/services/sse.svelte.ts index 77d4e4cb..937ed1eb 100644 --- a/web/src/lib/services/sse.svelte.ts +++ b/web/src/lib/services/sse.svelte.ts @@ -16,6 +16,7 @@ export interface ItemEvent { } type ItemEventCallback = (event: ItemEvent) => void; +type SyncRequiredCallback = () => void; const ITEM_EVENTS = [ 'item_created', @@ -36,6 +37,7 @@ function createSSEService() { let eventSource: EventSource | null = null; let currentWorkspace: string = ''; const callbacks = new SvelteSet(); + const syncRequiredCallbacks = new SvelteSet(); function connect(workspaceSlug: string) { // If already connected to the same workspace, don't reconnect. @@ -74,13 +76,18 @@ function createSSEService() { // Handle sync_required: server's replay buffer couldn't cover the gap. // Trigger an immediate sync rather than waiting for a visibility change, - // so the UI stays fresh even when the tab is actively visible. + // so the UI stays fresh even when the tab is actively visible. Fires + // out via onSyncRequired() subscribers (currently syncService) — the + // callback inversion keeps this module free of any sync.svelte import, + // breaking the circular dep that previously required a dynamic import + // here. (Rolldown flagged the dynamic import as ineffective because + // sync.svelte is statically imported from 5 routes/components anyway, + // so it's always in the main chunk — see TASK-1242.) eventSource.addEventListener('sync_required', () => { needsSync = true; - // Dynamic import to avoid circular dependency - import('./sync.svelte').then(({ syncService }) => { - syncService.triggerSync(); - }); + for (const cb of syncRequiredCallbacks) { + cb(); + } }); // Handle unauthorized: the server's periodic membership revalidation @@ -135,6 +142,22 @@ function createSSEService() { }; } + /** + * Subscribe to `sync_required` events from the server. Fires when the + * server's replay buffer couldn't cover a reconnect gap and the client + * needs to do a fresh sync. Returns an unsubscribe function. + * + * Used by syncService to drive `triggerSync()` without sse.svelte + * having to import sync.svelte (which would form a circular dep — + * sync.svelte already statically imports sseService). + */ + function onSyncRequired(callback: SyncRequiredCallback): () => void { + syncRequiredCallbacks.add(callback); + return () => { + syncRequiredCallbacks.delete(callback); + }; + } + function clearSyncFlag() { needsSync = false; } @@ -153,6 +176,7 @@ function createSSEService() { disconnect, reconnect, onItemEvent, + onSyncRequired, clearSyncFlag }; } diff --git a/web/src/lib/services/sync.svelte.ts b/web/src/lib/services/sync.svelte.ts index 2ea2b539..29cb65fa 100644 --- a/web/src/lib/services/sync.svelte.ts +++ b/web/src/lib/services/sync.svelte.ts @@ -64,6 +64,18 @@ function createSyncService() { onTabResume(); } }); + + // Subscribe to server-driven sync_required events. The callback + // inversion (sync subscribes via sseService.onSyncRequired instead + // of sse calling syncService.triggerSync directly) is what lets + // sse.svelte.ts stay free of any sync.svelte import — sync already + // imports sseService statically, so a reverse import would create + // a cycle. Previously broken with a dynamic `import('./sync.svelte')` + // call inside sse, which Rolldown correctly flagged as ineffective + // (see TASK-1242). + sseService.onSyncRequired(() => { + triggerSync(); + }); } async function setWorkspace(slug: string) {