fix(web): break sse↔sync circular dep via callback inversion (TASK-1242) (#445)

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.
This commit is contained in:
xarmian
2026-05-08 08:15:44 -04:00
committed by GitHub
parent 1dabfd02ae
commit 578494dc43
2 changed files with 41 additions and 5 deletions
+29 -5
View File
@@ -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<ItemEventCallback>();
const syncRequiredCallbacks = new SvelteSet<SyncRequiredCallback>();
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
};
}
+12
View File
@@ -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) {