From 07e47eba5754baa4aeb2716a44491d3ad65367f3 Mon Sep 17 00:00:00 2001 From: xarmian Date: Fri, 8 May 2026 08:47:41 -0400 Subject: [PATCH] fix(web): subscribe item detail page to live SSE updates (TASK-1243) (#446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): subscribe item detail page to live SSE updates (TASK-1243) The item detail page (+page.svelte under [collection]/[slug]) never called sseService.onItemEvent, so live changes to the parent item's title / fields / archive state didn't propagate from the server until a manual refresh. Comments, reactions, timeline events, and child-item updates all worked because their respective child components (CommentThread, ItemTimeline, ChildItems) carry their own SSE subscriptions — the gap was only on the parent item itself. Discovered while manually verifying TASK-1242 (callback inversion): two tabs on the same item showed divergent state until refresh. Fix mirrors the existing onSync handler that lives a few lines below: • Subscribe to sseService.onItemEvent in onMount, store the unsubscriber alongside unsubscribeSync / unsubscribeBeforePrint • Filter to event.item_id === item.id so cross-item navigation inside the same component instance is handled cleanly (the handler reads the *current* $state value of `item` on each fire, no stale-closure bug) • Same edit-conflict guards (saveStatus === 'saving' || editingTitle) so SSE pushes don't clobber in-flight edits • Same content-preservation pattern (`content: item.content`) — the Tiptap editor owns the document state; replacing item.content while it's mounted would clobber the user's local edits. Title, fields, and metadata propagate; content stays put. This is a deliberate conservative behavior identical to onSync's behavior for the same reason. • Handle item_archived (bounce back to collection list, matches onSync's deletion path) and item_restored • Tear down in onDestroy alongside the existing unsubscribers Lifecycle: SvelteKit reuses the +page.svelte component instance when navigating between items in the same collection, so onMount runs once per route entry and onDestroy runs once per route exit. The single subscription handles cross-item navigation correctly via the `event.item_id !== item.id` filter; closure reads of $state / $derived values pick up the new item on each event fire. No re-subscription per navigation needed (collection page does that because its closure captures wsSlug/collSlug as `const`s — different pattern, same correctness). KNOWN LIMITATION: live content sync is intentionally NOT handled. For Pad's current "snapshot save on debounce" model, replacing the editor's mounted document mid-edit would either drop user keystrokes or fight the editor's internal state machine. A proper fix needs either editor-dirty-state integration (acceptable for the "I'm editing my own doc on two tabs" case) or a true CRDT-based collab- edit refactor (Yjs + @tiptap/extension-collaboration) for the "two users typing simultaneously" case. The CRDT direction is the forward-looking plan; tracked separately. * fix(web): guard SSE/sync handlers against stale-resolution race Per Codex review on PR #446. The SSE handler I added in the previous commit checks `event.item_id === item.id` *before* `await api.items.get(...)`, but assigns to `item` *after* the resolution without re-checking. If the user navigates to a different item while the fetch is in flight, the resolved old-item data clobbers the newly-loaded current item. Same latent bug exists in the existing `onSync` handler's full- refresh path, and Codex correctly flagged it as the same shape. The loadData() function already guards against this same race in its catch-block (TASK-754 round-2 race guard, see comment at line 224). Fix mirrors that established pattern: • Capture `item.id`, `wsSlug`, `itemSlug` into `reqItemId` / `reqWsSlug` / `reqItemSlug` before the first await • After every await (api.items.get, api.links.list), bail with `if (!item || item.id !== reqItemId) return` before assigning • Use the captured values for the requests themselves so cross- navigation can't change which item we're fetching mid-flight Applied to: • SSE handler — item_updated and item_restored cases (item_archived has no await, so it's already safe) • onSync handler — incremental path (api.links.list was unguarded) and full-refresh path (api.items.get was unguarded) * fix(web): exempt destructive events from edit-conflict guard Per Codex review round 2 on PR #446. The edit-conflict guard (saveStatus === 'saving' || editingTitle) was placed BEFORE the event-type switch, which meant `item_archived` (SSE) and the `changes.deleted` branch (onSync) were also gated. Result: if another client archived the item while the user was editing, the destructive event was dropped and the user kept editing a non-existent item until the next event or tab-resume sync. Fix: hoist the destructive cases above the edit-conflict guard. The user's in-flight save will fail against the archived row anyway, so silently keeping them on a deleted item is strictly worse than discarding the edit and bouncing them to the collection list. Applied to both handlers: • SSE: `item_archived` runs before `saveStatus`/`editingTitle` guard, exits early after `goto()` • onSync: `result.changes.deleted.includes(item.id)` checked before the guard for the same reason --- .../[collection]/[slug]/+page.svelte | 122 ++++++++++++++++-- 1 file changed, 109 insertions(+), 13 deletions(-) diff --git a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte index e6b57e8d..ac68dc2a 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte @@ -4,6 +4,7 @@ import { api } from '$lib/api/client'; import { collectionStore } from '$lib/stores/collections.svelte'; import { syncService } from '$lib/services/sync.svelte'; + import { sseService } from '$lib/services/sse.svelte'; import Editor from '$lib/components/editor/Editor.svelte'; import EditorBubbleMenu from '$lib/components/editor/EditorBubbleMenu.svelte'; import EditorLinkPopover from '$lib/components/editor/EditorLinkPopover.svelte'; @@ -146,6 +147,7 @@ // Sync coordinator — refresh item data on tab resume let unsubscribeSync: (() => void) | null = null; + let unsubscribeSSE: (() => void) | null = null; let unsubscribeBeforePrint: (() => void) | null = null; // Print header/footer state (PLAN-620 / TASK-623). Initialized on mount @@ -170,40 +172,133 @@ window.addEventListener('beforeprint', handler); unsubscribeBeforePrint = () => window.removeEventListener('beforeprint', handler); + // Live SSE updates for THIS item's title / fields / archive state. + // Mirrors the onSync handler below — same edit-conflict guards + // (saveStatus / editingTitle) and same content-preservation + // pattern (the editor owns content; replacing it would clobber + // in-flight edits). The detail page previously had no live + // subscription, so title/field changes from another client (or + // session) didn't propagate until a manual refresh — TASK-1243. + // + // Comments, reactions, timeline events, and child-item updates + // already have their own subscriptions inside CommentThread.svelte, + // ItemTimeline.svelte, and ChildItems.svelte respectively — we + // only handle item_updated / item_archived / item_restored for + // the parent item itself here. + // + // KNOWN LIMITATION: live content-sync is intentionally NOT handled. + // Replacing item.content while the editor is mounted would clobber + // the user's in-flight document. A proper fix needs editor-dirty- + // state integration; tracked separately. + unsubscribeSSE = sseService.onItemEvent(async (event) => { + if (!item || event.item_id !== item.id) return; + + // Archive is destructive and must NOT be gated by the + // edit-conflict guard below — a user editing a since-archived + // item should be redirected immediately. Their in-flight save + // will fail against the archived row, and silently keeping + // them on a non-existent item is worse than discarding the + // edit. Per Codex review round 2. + if (event.type === 'item_archived') { + goto(`/${username}/${wsSlug}/${collSlug}`); + return; + } + + // Non-destructive updates: skip if the user is actively + // editing the title or has a pending content save in flight. + // They'll catch up on the next idle event (and the + // syncService onTabResume path also covers anything missed). + if (saveStatus === 'saving' || editingTitle) return; + + // Capture the item this event was scoped to *before* awaiting. + // Otherwise a navigation that completes during the in-flight + // request would let the resolved fetch clobber the new item + // (TASK-754-style race guard, mirrored from loadData()). + const reqItemId = item.id; + const reqWsSlug = wsSlug; + const reqItemSlug = itemSlug; + + switch (event.type) { + case 'item_updated': { + try { + const updated = await api.items.get(reqWsSlug, reqItemSlug); + // Bail if the user navigated away before this resolved. + if (!item || item.id !== reqItemId) return; + item = { ...updated, content: item.content }; + const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); + if (!item || item.id !== reqItemId) return; + itemLinks = links; + } catch { + // Ignore — will catch up on next event + } + break; + } + case 'item_restored': { + try { + const updated = await api.items.get(reqWsSlug, reqItemSlug); + if (!item || item.id !== reqItemId) return; + item = { ...updated, content: item.content }; + } catch { + // Ignore — will catch up on next event + } + break; + } + } + }); + unsubscribeSync = syncService.onSync(async (result) => { if (!wsSlug || !itemSlug || !item) return; - // Don't refresh if the user is actively editing - if (saveStatus === 'saving' || editingTitle) return; if (result.type === 'caught_up') return; + // Deletion is destructive and must run even if the user is + // editing — same reasoning as the SSE handler's archive case. + // Check this BEFORE the edit-conflict guard so a deleted item + // doesn't sit there gated by an in-flight save. + if (result.type === 'incremental' && result.changes.deleted.includes(item.id)) { + goto(`/${username}/${wsSlug}/${collSlug}`); + return; + } + + // Don't refresh non-destructive updates if the user is actively editing + if (saveStatus === 'saving' || editingTitle) return; + + // Capture the item this sync was scoped to *before* awaiting. + // Same race guard as the SSE handler above and loadData() — + // a navigation that completes mid-flight must not let a stale + // resolution clobber the newly-loaded item. + const reqItemId = item.id; + const reqWsSlug = wsSlug; + const reqItemSlug = itemSlug; + if (result.type === 'incremental') { // Check if our item is in the changed set - const updated = result.changes.updated.find(i => i.id === item!.id); + const updated = result.changes.updated.find(i => i.id === reqItemId); if (updated) { // Merge server state without disrupting the editor + if (!item || item.id !== reqItemId) return; item = { ...updated, - content: item!.content + content: item.content }; - itemLinks = await api.links.list(wsSlug, updated.slug).catch(() => []); + const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); + if (!item || item.id !== reqItemId) return; + itemLinks = links; } - // Check if our item was deleted - if (result.changes.deleted.includes(item!.id)) { - // Item was deleted — navigate back to collection - goto(`/${username}/${wsSlug}/${collSlug}`); -} return; } // Full refresh fallback try { - const updated = await api.items.get(wsSlug, itemSlug); + const updated = await api.items.get(reqWsSlug, reqItemSlug); + if (!item || item.id !== reqItemId) return; item = { ...updated, - content: item!.content + content: item.content }; - itemLinks = await api.links.list(wsSlug, updated.slug).catch(() => []); + const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); + if (!item || item.id !== reqItemId) return; + itemLinks = links; syncService.markSynced(); // Advance cursor now that reload succeeded } catch { // Ignore — will catch up on next event @@ -213,6 +308,7 @@ onDestroy(() => { unsubscribeSync?.(); + unsubscribeSSE?.(); unsubscribeBeforePrint?.(); editorStore.resetForDoc(); collectionStore.setActiveItem(null);