gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-09-22 02:23:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(web): subscribe item detail page to live SSE updates (TASK-1243) (#446)

Browse Source 
* 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
This commit is contained in:
xarmian
2026-05-08 08:47:41 -04:00
committed by GitHub
parent 578494dc43
commit 07e47eba57
1 changed files with 109 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte
+109 -13
View File
@@ -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);