From 7d76fc2b8a9c7bbc39a518479244fce0187f249b Mon Sep 17 00:00:00 2001 From: xarmian Date: Thu, 9 Jul 2026 16:51:55 -0400 Subject: [PATCH] refactor(web): extract contentSaver + shared progress-merge from item/collection monoliths (TASK-2029) (#885) --- web/src/lib/collections/progressMerge.test.ts | 76 ++++++++ web/src/lib/collections/progressMerge.ts | 93 +++++++++ web/src/lib/items/contentSaver.svelte.ts | 107 +++++++++++ web/src/lib/items/contentSaver.test.ts | 107 +++++++++++ .../[workspace]/[collection]/+page.svelte | 92 ++------- .../[collection]/[slug]/+page.svelte | 178 ++++++++++-------- 6 files changed, 491 insertions(+), 162 deletions(-) create mode 100644 web/src/lib/collections/progressMerge.test.ts create mode 100644 web/src/lib/collections/progressMerge.ts create mode 100644 web/src/lib/items/contentSaver.svelte.ts create mode 100644 web/src/lib/items/contentSaver.test.ts diff --git a/web/src/lib/collections/progressMerge.test.ts b/web/src/lib/collections/progressMerge.test.ts new file mode 100644 index 00000000..52c1c610 --- /dev/null +++ b/web/src/lib/collections/progressMerge.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect } from 'vitest'; +import { + plansProgressToMap, + mergeChildAndCheckboxProgress, + type ProgressRow, +} from './progressMerge'; + +describe('plansProgressToMap', () => { + it('maps rows by item_id without a label', () => { + const rows: ProgressRow[] = [ + { item_id: 'a', total: 4, done: 2 }, + { item_id: 'b', total: 0, done: 0 }, + ]; + expect(plansProgressToMap(rows)).toEqual({ + a: { total: 4, done: 2 }, + b: { total: 0, done: 0 }, + }); + }); + + it('returns an empty map for no rows', () => { + expect(plansProgressToMap([])).toEqual({}); + }); +}); + +describe('mergeChildAndCheckboxProgress', () => { + it('prefers linked children (label "tasks") when total > 0', () => { + const child: ProgressRow[] = [{ item_id: 'a', total: 3, done: 1 }]; + const checkbox: ProgressRow[] = [{ item_id: 'a', total: 9, done: 9 }]; + expect(mergeChildAndCheckboxProgress(child, checkbox)).toEqual({ + a: { total: 3, done: 1, label: 'tasks' }, + }); + }); + + it('falls back to checkbox progress (label "done") when no linked children', () => { + const child: ProgressRow[] = [{ item_id: 'a', total: 0, done: 0 }]; + const checkbox: ProgressRow[] = [{ item_id: 'a', total: 5, done: 2 }]; + expect(mergeChildAndCheckboxProgress(child, checkbox)).toEqual({ + a: { total: 5, done: 2, label: 'done' }, + }); + }); + + it('omits items with no children and no checkboxes', () => { + const child: ProgressRow[] = [{ item_id: 'a', total: 0, done: 0 }]; + const checkbox: ProgressRow[] = []; + expect(mergeChildAndCheckboxProgress(child, checkbox)).toEqual({}); + }); + + it('defensively covers items present only in checkbox rows', () => { + const child: ProgressRow[] = []; + const checkbox: ProgressRow[] = [{ item_id: 'z', total: 2, done: 1 }]; + expect(mergeChildAndCheckboxProgress(child, checkbox)).toEqual({ + z: { total: 2, done: 1, label: 'done' }, + }); + }); + + it('merges a mixed collection across all three branches', () => { + const child: ProgressRow[] = [ + { item_id: 'withKids', total: 2, done: 2 }, + { item_id: 'onlyBoxes', total: 0, done: 0 }, + { item_id: 'empty', total: 0, done: 0 }, + ]; + const checkbox: ProgressRow[] = [ + { item_id: 'onlyBoxes', total: 4, done: 1 }, + { item_id: 'orphanBoxes', total: 1, done: 0 }, + ]; + expect(mergeChildAndCheckboxProgress(child, checkbox)).toEqual({ + withKids: { total: 2, done: 2, label: 'tasks' }, + onlyBoxes: { total: 4, done: 1, label: 'done' }, + orphanBoxes: { total: 1, done: 0, label: 'done' }, + }); + }); + + it('returns an empty map when both inputs are empty', () => { + expect(mergeChildAndCheckboxProgress([], [])).toEqual({}); + }); +}); diff --git a/web/src/lib/collections/progressMerge.ts b/web/src/lib/collections/progressMerge.ts new file mode 100644 index 00000000..616a7a57 --- /dev/null +++ b/web/src/lib/collections/progressMerge.ts @@ -0,0 +1,93 @@ +// progressMerge — the collection-page progress fetch + merge, extracted from +// [collection]/+page.svelte as part of TASK-2029. That page fetched and merged +// child-item + markdown-checkbox progress into per-item badge data in TWO +// places (the `refreshProgress` helper and inline in `loadCollection`) with +// byte-identical merge logic. This is the single shared implementation both +// call sites now use. +// +// The merge functions are pure (no IO, no Svelte) and unit-tested; +// `fetchCollectionProgress` wraps them with the parallel API fetch. Each fetch +// swallows its own error into empty rows, matching the call sites' existing +// `.catch(() => [])` behaviour. + +import { api } from '$lib/api/client'; + +export interface ProgressRow { + item_id: string; + total: number; + done: number; +} + +export interface ProgressEntry { + total: number; + done: number; + label?: string; +} + +export type ProgressMap = Record; + +/** Build the badge map for a `plans` collection from plans-progress rows. */ +export function plansProgressToMap(rows: ProgressRow[]): ProgressMap { + const map: ProgressMap = {}; + for (const p of rows) { + map[p.item_id] = { total: p.total, done: p.done }; + } + return map; +} + +/** + * Merge child-item progress with markdown-checkbox progress into the per-item + * badge map (BUG-1509). Preference order per item: + * 1. real linked children (total > 0) → label "tasks" + * 2. else markdown checkboxes, if any → label "done" + * child-progress returns ALL items (total=0 for those with no linked + * children), so it drives the has-children decision; the final loop is a + * defensive catch for any item present only in checkbox-progress. + */ +export function mergeChildAndCheckboxProgress( + childRows: ProgressRow[], + checkboxRows: ProgressRow[], +): ProgressMap { + const checkboxMap: Record = {}; + for (const p of checkboxRows) { + checkboxMap[p.item_id] = { total: p.total, done: p.done }; + } + + const map: ProgressMap = {}; + for (const p of childRows) { + if (p.total > 0) { + map[p.item_id] = { total: p.total, done: p.done, label: 'tasks' }; + } else if (checkboxMap[p.item_id]) { + map[p.item_id] = { ...checkboxMap[p.item_id], label: 'done' }; + } + } + // Defensive: cover any items only in checkbox-progress (shouldn't happen + // since child-progress covers all items, but be safe). + for (const p of checkboxRows) { + if (!map[p.item_id]) { + map[p.item_id] = { total: p.total, done: p.done, label: 'done' }; + } + } + return map; +} + +/** + * Fetch child + checkbox progress for a non-plans collection in parallel and + * merge them into the badge map. Each fetch resolves to `[]` on error so this + * never rejects — mirroring the call sites' pre-extraction `.catch(() => [])`. + */ +export async function fetchCollectionProgress( + ws: string, + coll: string, + opts: { includeArchived: boolean }, +): Promise { + const [childRows, checkboxRows] = await Promise.all([ + api.items + .collectionChildProgress(ws, coll, { includeArchived: opts.includeArchived }) + .catch(() => [] as ProgressRow[]), + api.items + .collectionCheckboxProgress(ws, coll, { includeArchived: opts.includeArchived }) + .catch(() => [] as ProgressRow[]), + ]); + return mergeChildAndCheckboxProgress(childRows, checkboxRows); +} diff --git a/web/src/lib/items/contentSaver.svelte.ts b/web/src/lib/items/contentSaver.svelte.ts new file mode 100644 index 00000000..8cc8584a --- /dev/null +++ b/web/src/lib/items/contentSaver.svelte.ts @@ -0,0 +1,107 @@ +// contentSaver — the debounced raw-markdown content saver extracted from the +// item detail page ([collection]/[slug]/+page.svelte) as part of TASK-2029. +// +// It owns exactly three things that used to be inlined (and tangled) in the +// monolith's raw-mode save path: +// 1. the debounce timer that coalesces keystrokes into one PATCH, +// 2. the "pending markdown" dirty flag (was `rawPendingMarkdown`), and +// 3. flush-now — fire the pending save IMMEDIATELY, cancelling the debounce. +// This is what the BUG-2024 keepalive-on-unload path and the +// rich↔raw toggle drain (flushRawIfPending) both need. +// +// The actual PATCH (api.items.update + all the reactive item/saveStatus/ +// editorStore bookkeeping + stale-response guards) stays in the page and is +// injected as the `save` callback so this module has no Svelte / API +// dependencies and is unit-testable in plain node. +// +// NOTE (CONVE-1688): `pending` is a plain closure variable, NOT `$state`. +// It is a handler-only dirty tracker — the page's *reactive* dirty flag lives +// in `editorStore`. A `$state` written inside the page's save $effect/handlers +// that an effect also read would silently wedge the effect scheduler in PROD. +// Keeping it a plain `let` is deliberate; this file is `.svelte.ts` for +// colocation with the item-page module conventions, but uses no runes. + +export interface ContentSaverConfig { + /** Debounce window in ms for queued keystroke saves (default 1200). */ + debounceMs?: number; + /** + * Perform the actual content save. `keepalive` is true when flushing on + * page unload (fetch keepalive) or any other immediate flush that must + * outlive teardown. The return value is ignored (fire-and-forget on the + * unload path). The page owns all reactive bookkeeping inside this + * callback and clears the pending flag via `clearPending()` when the + * PATCH lands and no newer edit superseded it. + */ + save: (markdown: string, ctx: { keepalive: boolean }) => void | Promise; +} + +export interface ContentSaver { + /** + * Queue a save for the given markdown: records it as the pending (dirty) + * content and (re)arms the debounce. Rapid successive calls coalesce — + * only the last markdown is saved when the debounce fires. + */ + queue(markdown: string): void; + /** + * Fire the pending save immediately, cancelling any queued debounce so it + * can't fire a second, older-content PATCH. No-op (returns false) when + * there's nothing pending. Returns true when a save was dispatched. + */ + flushNow(opts?: { keepalive?: boolean }): boolean; + /** Cancel the pending debounce without saving. Leaves the dirty flag set. */ + cancel(): void; + /** Clear the pending (dirty) markdown — call once a save has landed. */ + clearPending(): void; + /** The markdown awaiting a save, or null when clean. */ + readonly pending: string | null; + /** True when there is unsaved pending markdown. */ + readonly dirty: boolean; +} + +const DEFAULT_DEBOUNCE_MS = 1200; + +export function createContentSaver(config: ContentSaverConfig): ContentSaver { + const debounceMs = config.debounceMs ?? DEFAULT_DEBOUNCE_MS; + let timer: ReturnType | undefined; + let pending: string | null = null; + + function cancel(): void { + if (timer !== undefined) { + clearTimeout(timer); + timer = undefined; + } + } + + function queue(markdown: string): void { + cancel(); + pending = markdown; + timer = setTimeout(() => { + timer = undefined; + void config.save(markdown, { keepalive: false }); + }, debounceMs); + } + + function flushNow(opts: { keepalive?: boolean } = {}): boolean { + cancel(); + if (pending === null) return false; + void config.save(pending, { keepalive: opts.keepalive ?? false }); + return true; + } + + function clearPending(): void { + pending = null; + } + + return { + queue, + flushNow, + cancel, + clearPending, + get pending() { + return pending; + }, + get dirty() { + return pending !== null; + }, + }; +} diff --git a/web/src/lib/items/contentSaver.test.ts b/web/src/lib/items/contentSaver.test.ts new file mode 100644 index 00000000..b8d8217d --- /dev/null +++ b/web/src/lib/items/contentSaver.test.ts @@ -0,0 +1,107 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { createContentSaver } from './contentSaver.svelte'; + +describe('createContentSaver', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('debounces and coalesces rapid keystrokes into one save', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + saver.queue('a'); + vi.advanceTimersByTime(500); + saver.queue('ab'); // supersedes 'a' before it fires + vi.advanceTimersByTime(500); + saver.queue('abc'); + + // Nothing fires until a full debounce window elapses after the last edit. + expect(save).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1200); + + expect(save).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledWith('abc', { keepalive: false }); + }); + + it('tracks the dirty flag: set on queue, clearable via clearPending', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + expect(saver.dirty).toBe(false); + expect(saver.pending).toBeNull(); + + saver.queue('hello'); + expect(saver.dirty).toBe(true); + expect(saver.pending).toBe('hello'); + + saver.clearPending(); + expect(saver.dirty).toBe(false); + expect(saver.pending).toBeNull(); + }); + + it('flushNow fires the pending save immediately and cancels the debounce', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + saver.queue('draft'); + const fired = saver.flushNow(); + + expect(fired).toBe(true); + expect(save).toHaveBeenCalledTimes(1); + expect(save).toHaveBeenCalledWith('draft', { keepalive: false }); + + // The queued debounce must NOT fire a second, racing save. + vi.advanceTimersByTime(5000); + expect(save).toHaveBeenCalledTimes(1); + }); + + it('plumbs the keepalive flag through flushNow (BUG-2024 unload path)', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + saver.queue('unsaved edit'); + const fired = saver.flushNow({ keepalive: true }); + + expect(fired).toBe(true); + expect(save).toHaveBeenCalledWith('unsaved edit', { keepalive: true }); + }); + + it('does not save when clean', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + const fired = saver.flushNow({ keepalive: true }); + + expect(fired).toBe(false); + expect(save).not.toHaveBeenCalled(); + }); + + it('cancel stops the debounce but leaves the dirty flag set', () => { + const save = vi.fn(); + const saver = createContentSaver({ debounceMs: 1200, save }); + + saver.queue('typing'); + saver.cancel(); + vi.advanceTimersByTime(5000); + + expect(save).not.toHaveBeenCalled(); + // Still dirty — cancel only stops the timer, it doesn't discard content. + expect(saver.dirty).toBe(true); + expect(saver.pending).toBe('typing'); + }); + + it('uses the default debounce window when none is supplied', () => { + const save = vi.fn(); + const saver = createContentSaver({ save }); + + saver.queue('x'); + vi.advanceTimersByTime(1199); + expect(save).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(save).toHaveBeenCalledTimes(1); + }); +}); diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index 7c392c5a..95896096 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -4,6 +4,7 @@ import { api, PadApiError, isPlanLimitError, planLimitMessage } from '$lib/api/client'; import type { BulkItemsRequest, Collection, Item, QuickAction, View, ViewConfig } from '$lib/types'; import { parseSettings, parseFields, parseSchema, parseTags, getStatusOptions, itemUrlId, formatItemRef } from '$lib/types'; + import { plansProgressToMap, fetchCollectionProgress } from '$lib/collections/progressMerge'; import BoardView from '$lib/components/collections/BoardView.svelte'; import ListView from '$lib/components/collections/ListView.svelte'; import TableView from '$lib/components/collections/TableView.svelte'; @@ -463,50 +464,14 @@ async function refreshProgress(ws: string, coll: string, itemList: typeof items) { if (coll === 'plans') { const progress = await api.items.plansProgress(ws).catch(() => []); - const map: Record = {}; - for (const p of progress) { - map[p.item_id] = { total: p.total, done: p.done }; - } - itemProgress = map; + itemProgress = plansProgressToMap(progress); progressLabel = 'tasks'; } else { // Non-plans collections: prefer child-item progress (real linked - // children) for items that have them; fall back to markdown-checkbox - // progress for items that don't. Fetched in parallel (BUG-1509). - // - // child-progress returns ALL items in the collection with total=0 - // for those with no linked children, so we can distinguish "has - // children" from "no children" per item. - // - // Pass `includeArchived` to both endpoints so the archived-items - // toggle keeps progress badges on archived items (PR #491 [P2]). - const [childRows, checkboxRows] = await Promise.all([ - api.items.collectionChildProgress(ws, coll, { includeArchived: showArchived }).catch(() => [] as {item_id: string; total: number; done: number}[]), - api.items.collectionCheckboxProgress(ws, coll, { includeArchived: showArchived }).catch(() => [] as {item_id: string; total: number; done: number}[]), - ]); - - const checkboxMap: Record = {}; - for (const p of checkboxRows) { - checkboxMap[p.item_id] = { total: p.total, done: p.done }; - } - - const map: Record = {}; - for (const p of childRows) { - if (p.total > 0) { - map[p.item_id] = { total: p.total, done: p.done, label: 'tasks' }; - } else if (checkboxMap[p.item_id]) { - map[p.item_id] = { ...checkboxMap[p.item_id], label: 'done' }; - } - } - // Items that only have checkboxes (not in child-progress rows at - // all — shouldn't happen since child-progress covers all items — - // but be defensive). - for (const p of checkboxRows) { - if (!map[p.item_id]) { - map[p.item_id] = { total: p.total, done: p.done, label: 'done' }; - } - } - itemProgress = map; + // children) per item; fall back to markdown-checkbox progress for + // items with none (BUG-1509). `showArchived` keeps badges on + // archived items (PR #491 [P2]). Shared fetch+merge (TASK-2029). + itemProgress = await fetchCollectionProgress(ws, coll, { includeArchived: showArchived }); } } @@ -542,16 +507,14 @@ workspaceMembers = membersData.members ?? []; activeViewId = null; - // Fetch progress badges for the collection's items. + // Fetch progress badges for the collection's items. Shared + // fetch+merge helpers (TASK-2029); the seq-guard, label, and + // error handling stay here (per call site). if (coll === 'plans') { try { const progress = await api.items.plansProgress(ws); if (seq !== loadSeq) return; - const map: Record = {}; - for (const p of progress) { - map[p.item_id] = { total: p.total, done: p.done }; - } - itemProgress = map; + itemProgress = plansProgressToMap(progress); progressLabel = 'tasks'; } catch { // Don't clear a newer load's progress badges if this @@ -563,40 +526,11 @@ // Non-plans collections: prefer child-item progress (real // linked children, label "tasks") per item; fall back to // markdown-checkbox progress (label "done") for items that - // have no linked children (BUG-1509). Fetched in parallel. - // - // child-progress returns ALL items including those with - // total=0 (no linked children), so we can make the - // per-item decision without a second fetch. - // - // `includeArchived` is passed to checkbox-progress so the - // archived-items toggle keeps its badges (PR #491 [P2]). + // have no linked children (BUG-1509). `includeArchived` + // keeps the archived-items toggle's badges (PR #491 [P2]). try { - const [childRows, checkboxRows] = await Promise.all([ - api.items.collectionChildProgress(ws, coll, { includeArchived }).catch(() => [] as {item_id: string; total: number; done: number}[]), - api.items.collectionCheckboxProgress(ws, coll, { includeArchived }).catch(() => [] as {item_id: string; total: number; done: number}[]), - ]); + const map = await fetchCollectionProgress(ws, coll, { includeArchived }); if (seq !== loadSeq) return; - - const checkboxMap: Record = {}; - for (const p of checkboxRows) { - checkboxMap[p.item_id] = { total: p.total, done: p.done }; - } - - const map: Record = {}; - for (const p of childRows) { - if (p.total > 0) { - map[p.item_id] = { total: p.total, done: p.done, label: 'tasks' }; - } else if (checkboxMap[p.item_id]) { - map[p.item_id] = { ...checkboxMap[p.item_id], label: 'done' }; - } - } - // Defensive: cover any items only in checkbox-progress. - for (const p of checkboxRows) { - if (!map[p.item_id]) { - map[p.item_id] = { total: p.total, done: p.done, label: 'done' }; - } - } itemProgress = map; progressLabel = 'done'; } catch { diff --git a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte index ca1ac591..17e33753 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte @@ -17,6 +17,7 @@ import { CollabProvider, type CollabConnectionState } from '$lib/collab/wsProvider.svelte'; import { userColor } from '$lib/collab/cursorColor'; import { shouldDedupeEditorSpace } from '$lib/collab/flushDedupe'; + import { createContentSaver } from '$lib/items/contentSaver.svelte'; import { authStore } from '$lib/stores/auth.svelte'; import FieldEditor from '$lib/components/fields/FieldEditor.svelte'; import TagInput from '$lib/components/fields/TagInput.svelte'; @@ -530,7 +531,10 @@ clearTimeout(collabFlushTimer); collabFlushTimer = undefined; rawSeedMarkdown = null; - rawPendingMarkdown = null; + // Cancel the raw saver's debounce and drop its pending edit so a + // stale queued markdown from item A can't PATCH into item B. + rawContentSaver.cancel(); + rawContentSaver.clearPending(); // lastFlushedContent is per-item; resetting prevents the // dedupe from incorrectly suppressing the first flush on // the next item (which happens to share the same markdown @@ -1110,39 +1114,17 @@ const ctx = activeCollabContext; if (ctx) flushCollabNow(ctx, true); - // Raw-markdown path (BUG-2024). rawPendingMarkdown holds the - // exact debounced-but-unsaved markdown; when non-null there - // is up to ~1.2s of typing the collab flush above never - // sees. Only when actually dirty — never warn on a clean page. - const pendingRaw = rawPendingMarkdown; - if (pendingRaw !== null && item) { - const reqItemId = item.id; - // Cancel any queued debounce so it can't fire a second, - // older-content PATCH racing the keepalive one below — - // the keepalive request already carries the latest - // markdown. (An already-in-flight debounced PATCH would - // carry older content, but the debounce spacing makes - // one being airborne at unload vanishingly narrow, and - // the server's un-versioned content PATCH has never - // guarded that ordering; this is strictly better than - // the pre-fix total loss of the debounced edit.) - clearTimeout(contentDebounceTimer); - contentDebounceTimer = undefined; - // Fire an immediate keepalive PATCH so the edit survives - // page teardown (keepalive holds the request open past - // unload — that's the point). If the user cancels the - // navigation ("Stay"), clear the dirty state once it - // lands so a later unload doesn't re-prompt / re-PATCH - // already-saved content. - void api.items - .update(wsSlug, reqItemId, { content: pendingRaw }, { keepalive: true }) - .then(() => { - if (item && item.id === reqItemId && rawPendingMarkdown === pendingRaw) { - rawPendingMarkdown = null; - editorStore.setDirty(false); - } - }) - .catch(() => {}); + // Raw-markdown path (BUG-2024). The saver's pending markdown is + // the exact debounced-but-unsaved edit; when dirty there is up + // to ~1.2s of typing the collab flush above never sees. Only + // when actually dirty — never warn on a clean page. flushNow + // cancels the queued debounce (so it can't fire a second, + // older-content PATCH) and fires an immediate keepalive PATCH + // that survives page teardown. The saver's `save` callback + // clears the dirty state once it lands (covers the user + // cancelling the navigation via "Stay"). See rawContentSaver. + if (rawContentSaver.dirty && item) { + rawContentSaver.flushNow({ keepalive: true }); // Native "unsaved changes" prompt. event.preventDefault(); event.returnValue = ''; @@ -1731,6 +1713,11 @@ return; } clearTimeout(contentDebounceTimer); + // Cancel any pending RAW-mode saver debounce too — this legacy + // non-collab path and the raw saver shared one timer before + // TASK-2029 split them; cancelling both preserves the mode-toggle + // non-trample the single shared timer guaranteed. + rawContentSaver.cancel(); editorStore.setDirty(true); contentDebounceTimer = setTimeout(() => { if (!item) return; @@ -1977,41 +1964,48 @@ return true; } - // Latest raw markdown that hasn't yet been PATCHed. Tracked - // alongside contentDebounceTimer so toggling out of raw mode - // (via flushRawIfPending below) can synchronously land the - // pending edit BEFORE the collab provider mints — otherwise the - // debounced PATCH fires after the provider is up, gets routed - // through the applier path, and races peer state. Per Codex - // review round 5. - let rawPendingMarkdown: string | null = null; - - // One-shot seed for the raw editor when toggling from rich+collab. - // items.content stays stale under collab (handleContentUpdate is - // suppressed when the provider is active); without this seed, - // RawMarkdownEditor would mount with the pre-collab markdown and - // any subsequent save would silently lose the live Y.Doc state. - // Reset to null on rich-mode toggle and on item swap. Per Codex - // review round 9. - let rawSeedMarkdown = $state(null); - - function handleRawContentUpdate(markdown: string) { - clearTimeout(contentDebounceTimer); - editorStore.setDirty(true); - rawPendingMarkdown = markdown; - contentDebounceTimer = setTimeout(() => { + // The raw-markdown content saver (TASK-2029, extracted from this page). + // Owns the 1.2s debounce, the "pending markdown" dirty tracker (was + // `rawPendingMarkdown`), and flush-now for the keepalive-on-unload + // (BUG-2024) + rich↔raw toggle-drain paths. `saver.pending` is tracked + // alongside contentDebounceTimer so toggling out of raw mode (via + // flushRawIfPending below) can synchronously land the pending edit + // BEFORE the collab provider mints — otherwise the debounced PATCH + // fires after the provider is up, gets routed through the applier path, + // and races peer state. Per Codex review round 5. The PATCH + all + // reactive bookkeeping stays here as the injected `save`. + const rawContentSaver = createContentSaver({ + debounceMs: 1200, + save: (markdown, { keepalive }) => { if (!item) return; - saveStatus = 'saving'; - editorStore.setLastSaveTime(Date.now()); // Capture the item id this PATCH was scoped to so a // late-arriving response after navigation to a new item - // can't apply the old item's snapshot to the new - // page state (cross-item bleed). Per Codex review round - // 11. + // can't apply the old item's snapshot to the new page + // state (cross-item bleed). Per Codex review round 11. const reqItemId = item.id; + if (keepalive) { + // BUG-2024 keepalive-on-unload path. Fire an immediate + // keepalive PATCH so the edit survives page teardown + // (keepalive holds the request open past unload). If the + // user cancels the navigation ("Stay"), clear the dirty + // state once it lands so a later unload doesn't re-prompt + // / re-PATCH already-saved content. + return api.items + .update(wsSlug, reqItemId, { content: markdown }, { keepalive: true }) + .then(() => { + if (item && item.id === reqItemId && rawContentSaver.pending === markdown) { + rawContentSaver.clearPending(); + editorStore.setDirty(false); + } + }) + .catch(() => {}); + } + // Debounced raw save. + saveStatus = 'saving'; + editorStore.setLastSaveTime(Date.now()); // Raw mode: content is already in storage format (with [[wiki links]]) const toSave = markdown; - api.items.update(wsSlug, reqItemId, { content: toSave }).then((updated) => { + return api.items.update(wsSlug, reqItemId, { content: toSave }).then((updated) => { if (!item || item.id !== reqItemId) return; editorStore.setLastSaveTime(Date.now()); // Raw saves change items.content via a path the @@ -2028,9 +2022,9 @@ // mirror would reset the textarea mid-keystroke and // drop the queued edit. Mirrors the Round 8 fix in // flushRawIfPending. Per Codex review round 9. - if (rawPendingMarkdown === toSave) { + if (rawContentSaver.pending === toSave) { item = withInflightTags(updated); - rawPendingMarkdown = null; + rawContentSaver.clearPending(); editorStore.setDirty(false); showSaved(); } else { @@ -2044,7 +2038,26 @@ saveStatus = 'idle'; toastStore.show('Failed to save content', 'error'); }); - }, 1200); + }, + }); + + // One-shot seed for the raw editor when toggling from rich+collab. + // items.content stays stale under collab (handleContentUpdate is + // suppressed when the provider is active); without this seed, + // RawMarkdownEditor would mount with the pre-collab markdown and + // any subsequent save would silently lose the live Y.Doc state. + // Reset to null on rich-mode toggle and on item swap. Per Codex + // review round 9. + let rawSeedMarkdown = $state(null); + + function handleRawContentUpdate(markdown: string) { + // Cancel any pending LEGACY (non-collab rich) debounce so it can't + // fire a stale save over this raw edit — preserves the shared-timer + // non-trample the two paths had before TASK-2029 split the raw + // debounce into the saver. + clearTimeout(contentDebounceTimer); + editorStore.setDirty(true); + rawContentSaver.queue(markdown); } // True while flushRawIfPending is awaiting a PATCH response. @@ -2053,21 +2066,21 @@ let rawFlushInFlight = false; // Cap on flushRawIfPending's drain loop. If the user is typing - // fast enough to keep rawPendingMarkdown non-null across this - // many PATCH round-trips, return false and force them to click + // fast enough to keep the saver's pending markdown non-null across + // this many PATCH round-trips, return false and force them to click // again — better than spinning indefinitely. const RAW_FLUSH_DRAIN_CAP = 5; // flushRawIfPending drains every queued raw edit SYNCHRONOUSLY // (one PATCH per drained snapshot, awaited) and returns true - // only when rawPendingMarkdown is null on exit. Callers are - // expected to gate state transitions (e.g. enabling collab) on - // the return value — a stale rawPendingMarkdown left over from - // a fast typist or a failed PATCH would otherwise re-introduce - // the "collab active with unsaved raw edit" race the guard is - // meant to prevent. Per Codex review round 7. + // only when the saver's pending markdown is null on exit. Callers + // are expected to gate state transitions (e.g. enabling collab) on + // the return value — a stale pending edit left over from a fast + // typist or a failed PATCH would otherwise re-introduce the + // "collab active with unsaved raw edit" race the guard is meant to + // prevent. Per Codex review round 7. async function flushRawIfPending(): Promise { - if (!item) return rawPendingMarkdown === null; + if (!item) return rawContentSaver.pending === null; // Re-entrancy: another flush is already running. Wait for // it to settle, then re-evaluate from scratch. @@ -2075,10 +2088,10 @@ while (rawFlushInFlight) { await new Promise((r) => setTimeout(r, 50)); } - return rawPendingMarkdown === null; + return rawContentSaver.pending === null; } - if (rawPendingMarkdown === null) return true; + if (rawContentSaver.pending === null) return true; rawFlushInFlight = true; // Capture the item id once for the entire drain. If the user @@ -2089,10 +2102,9 @@ let lastError = false; try { for (let i = 0; i < RAW_FLUSH_DRAIN_CAP; i++) { - const markdown: string | null = rawPendingMarkdown; + const markdown: string | null = rawContentSaver.pending; if (markdown === null) break; - clearTimeout(contentDebounceTimer); - contentDebounceTimer = undefined; + rawContentSaver.cancel(); try { saveStatus = 'saving'; editorStore.setLastSaveTime(Date.now()); @@ -2117,9 +2129,9 @@ // stale snapshot here would reset the textarea // from under the user's keystrokes and lose the // queued edit. Per Codex review round 8. - if (rawPendingMarkdown === markdown) { + if (rawContentSaver.pending === markdown) { item = withInflightTags(updated); - rawPendingMarkdown = null; + rawContentSaver.clearPending(); } else { // Newer edit pending — keep our local // content but adopt server-side metadata @@ -2133,7 +2145,7 @@ break; } } - if (!lastError && rawPendingMarkdown === null) { + if (!lastError && rawContentSaver.pending === null) { editorStore.setDirty(false); showSaved(); return true;