From feb068a91f367d8bfd795379b1e6e7dd6d333f82 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 30 May 2026 01:58:17 -0400 Subject: [PATCH] feat(tags): tag chip editor on the item detail page (TASK-1654) (#659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(tags): tag chip editor on the item detail page (TASK-1654) Tags live on item.tags (a JSON-array string), not the collection schema, so this adds a TagInput sibling to FieldEditor rather than a field type. - TagInput.svelte: chip editor — Enter/comma to add, Backspace/× to remove, case-insensitive dedupe (stored as typed), autocomplete dropdown sourced from the workspace tag set; readonly mode renders plain chips. - Item detail page: derive `tags` from item.tags (defensive parse), load `tagSuggestions` via a workspace-keyed $effect kept separate from the item-load path (Svelte 5 effect-splitting convention), and updateTags() mirrors updateField() — optimistic with revert-on-failure, PATCHing `tags`. The Tags row renders between the schema fields and the Assignment section. api.items.update already accepted `tags` via ItemUpdate, so no client change was needed there. Parent: PLAN-1652. * fix(tags): guard overlapping tag saves with a sequence counter per Codex review (round 1) Rapid chip edits can issue overlapping PATCHes; a late-resolving older request could clobber the newer tag set with stale data or an errant revert. Only the latest save (by monotonic seq) applies its result or reverts. * fix(tags): drop stale tag-suggestion results across workspace navigation per Codex review (round 2) loadTagSuggestions now only assigns when the in-flight workspace still matches the current one, so a slower old-workspace /tags response can't overwrite the new workspace's autocomplete. * fix(tags): dedupe tags + key chips by index per Codex review (round 3) An item can carry duplicate tags (e.g. ["ux","ux"]) since the write path doesn't enforce per-item uniqueness, which would collide value-based Svelte keys. Key chips by index, and dedupe case-insensitively at the source so the cleaned set persists on the next save. * fix(tags): gate tag-save completion UI on item freshness per Codex review (round 4) If the user navigates to another item while a tag save is in flight (no further edit, so the seq guard doesn't trip), skip showSaved()/toast/refresh so completion UI can't fire on an unrelated page. * fix(tags): serialize+coalesce tag saves, revert to last confirmed per Codex review (round 5) Replace the concurrent-PATCH-with-seq-guard approach with a single in-flight, coalescing saver scoped per item. Eliminates the overlap class structurally: no stale completion clobbers a newer set, and `confirmed` tracks the last server-acknowledged tags so a failed save reverts to server truth rather than an optimistic unconfirmed value. Subsumes the round-1 race guard and round-4 navigation gate. * fix(tags): key tag savers by item id to prevent cross-navigation concurrency per Codex review (round 6) A single saver slot let navigating away from an item mid-save and back spawn a second concurrent saver for it. Hold savers in a Map keyed by item id so edits coalesce into the existing in-flight saver; evict on drain. * fix(tags): reapply in-flight desired tags after item reload per Codex review (round 7) Navigating away and back mid-save reloaded stale server tags; a follow-up edit computed from that stale set could drop the in-flight edit. The saver now tracks the latest desired set and loadData reapplies it when a save is still in flight for the reloaded item. * fix(tags): keep save indicator active across reload so refresh guards hold per Codex review (round 8) loadData reset saveStatus to idle while a tag PATCH was still in flight, letting SSE/sync snapshot adoption bypass the saveStatus==='saving' guard and land stale tags. Restore 'saving' when reapplying an in-flight saver so the existing refresh guards keep skipping until the save drains. * fix(tags): overlay in-flight tags at every server-snapshot assignment per Codex review (round 9) The saveStatus guard is racy (checked before the refresh handlers' own await, not after), so a concurrent snapshot could still drop optimistic tags. Extract withInflightTags() and route ALL item = sites through it (realtime SSE/sync, initial load, content-save echoes, title/field/assignment/ role update echoes, post-action refresh, version restore). Overlaying the saver's desired set at assignment time is race-free regardless of the guard. * fix(tags): overlay tags on field-save + forced-retry echoes per Codex review (round 10) updateField's success echo (item = fresh) and the forced open-children retry (item = forced) were the last two un-overlaid server-snapshot assignments; route both through withInflightTags so a concurrent tag save isn't clobbered. * fix(tags): preserve unsaved content when reconciling tag-save echo per Codex review (round 11) flushTagSaver adopted the full tag PATCH response (item = fresh), which carries server content and could clobber unsaved editor edits. Route it through adoptServerItem so local content is preserved (non-collab) like the other snapshot adoption sites. --- web/src/lib/components/fields/TagInput.svelte | 199 +++++++++++++++ .../[collection]/[slug]/+page.svelte | 237 +++++++++++++++--- 2 files changed, 401 insertions(+), 35 deletions(-) create mode 100644 web/src/lib/components/fields/TagInput.svelte diff --git a/web/src/lib/components/fields/TagInput.svelte b/web/src/lib/components/fields/TagInput.svelte new file mode 100644 index 00000000..6ffa74b3 --- /dev/null +++ b/web/src/lib/components/fields/TagInput.svelte @@ -0,0 +1,199 @@ + + +{#if readonly} +
+ {#if tags.length === 0} + No tags + {:else} + + {#each tags as tag, i (i)} + {tag} + {/each} + {/if} +
+{:else} +
+
+ {#each tags as tag, i (i)} + + {tag} + + + {/each} + (showSuggestions = true)} + onblur={() => setTimeout(() => (showSuggestions = false), 120)} + /> +
+ {#if showSuggestions && filteredSuggestions.length > 0} +
+ {#each filteredSuggestions as s (s)} + + {/each} +
+ {/if} +
+{/if} + + diff --git a/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/[slug]/+page.svelte index a8f6452e..792780d5 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 { userColor } from '$lib/collab/cursorColor'; import { authStore } from '$lib/stores/auth.svelte'; import FieldEditor from '$lib/components/fields/FieldEditor.svelte'; + import TagInput from '$lib/components/fields/TagInput.svelte'; import ItemTimeline from '$lib/components/timeline/ItemTimeline.svelte'; import ChildItems from '$lib/components/ChildItems.svelte'; import BacklinksPanel from '$lib/components/BacklinksPanel.svelte'; @@ -97,6 +98,32 @@ let titleInputEl = $state(); let fields = $derived>(item ? parseFields(item) : {}); + // Tags live on item.tags (a JSON-array string), NOT in the schema. Parse + // defensively — the column defaults to "[]" but tolerate empty/garbage. + let tags = $derived.by(() => { + if (!item?.tags) return []; + try { + const parsed = JSON.parse(item.tags); + if (!Array.isArray(parsed)) return []; + // Dedupe case-insensitively (keep first as typed). The write path + // doesn't enforce per-item uniqueness, so an API/imported item can + // carry ["ux","ux"]; cleaning here keeps rendering keys unique and + // the dedupe gets persisted on the next save. + const seen = new Set(); + const out: string[] = []; + for (const t of parsed) { + if (typeof t !== 'string') continue; + const key = t.toLowerCase(); + if (seen.has(key)) continue; + seen.add(key); + out.push(t); + } + return out; + } catch { + return []; + } + }); + let tagSuggestions = $state([]); let schema = $derived(collection ? parseSchema(collection) : { fields: [] }); let settings = $derived(collection ? parseSettings(collection) : { layout: 'balanced', default_view: 'list' }); let layout = $derived(settings.layout); @@ -284,11 +311,7 @@ // a debounced save is in flight, but a user // mid-keystroke with no save yet pending would // still lose chars without this branch. - if (collabProvider) { - item = updated; - } else { - item = { ...updated, content: item.content }; - } + item = adoptServerItem(updated); const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); if (!item || item.id !== reqItemId) return; itemLinks = links; @@ -301,11 +324,7 @@ try { const updated = await api.items.get(reqWsSlug, reqItemSlug); if (!item || item.id !== reqItemId) return; - if (collabProvider) { - item = updated; - } else { - item = { ...updated, content: item.content }; - } + item = adoptServerItem(updated); } catch { // Ignore — will catch up on next event } @@ -347,11 +366,7 @@ // Same collab-aware adoption rule as the SSE // handler above (TASK-1262). if (!item || item.id !== reqItemId) return; - if (collabProvider) { - item = updated; - } else { - item = { ...updated, content: item.content }; - } + item = adoptServerItem(updated); const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); if (!item || item.id !== reqItemId) return; itemLinks = links; @@ -363,11 +378,7 @@ try { const updated = await api.items.get(reqWsSlug, reqItemSlug); if (!item || item.id !== reqItemId) return; - if (collabProvider) { - item = updated; - } else { - item = { ...updated, content: item.content }; - } + item = adoptServerItem(updated); const links = await api.links.list(reqWsSlug, updated.slug).catch(() => []); if (!item || item.id !== reqItemId) return; itemLinks = links; @@ -447,7 +458,10 @@ api.collections.get(wsSlug, collSlug), itemsPromise ]); - item = itemData; + // Overlay any in-flight optimistic tag edit so navigating away and + // back mid-save can't show (and then let a follow-up edit overwrite + // with) stale server tags. See withInflightTags. Per Codex PR #659. + item = withInflightTags(itemData); collection = collData; collectionStore.setActiveItem(itemData); editorStore.resetForDoc(); @@ -800,7 +814,7 @@ // item; a navigation away should not stamp // fresh content into the OTHER item's slot. if (item && item.id === refreshCtx.itemId) { - item = fresh; + item = withInflightTags(fresh); } // Refetch succeeded → safe to rebuild. forceRefreshNonce += 1; @@ -1030,7 +1044,7 @@ if (!item || titleDraft.trim() === item.title) return; saveStatus = 'saving'; try { - item = await api.items.update(wsSlug, item.id, { title: titleDraft.trim() }); + item = withInflightTags(await api.items.update(wsSlug, item.id, { title: titleDraft.trim() })); showSaved(); } catch { saveStatus = 'idle'; @@ -1065,7 +1079,7 @@ try { const fresh = await doUpdate(false); - if (item && item.id === targetItem.id) item = fresh; + if (item && item.id === targetItem.id) item = withInflightTags(fresh); showSaved(); } catch (e) { // BUG-1538 / TASK-1539: same open-children-guard recovery @@ -1089,7 +1103,7 @@ return; } if (forced) { - if (item && item.id === targetItem.id) item = forced; + if (item && item.id === targetItem.id) item = withInflightTags(forced); showSaved(); return; } @@ -1111,6 +1125,146 @@ } } + // Serialized, coalescing tag saver. Tags are a top-level item column + // (item.tags), not a schema field, and the chip input makes rapid edits + // easy — so instead of firing concurrent PATCHes and guarding the races, + // we keep ONE save in flight per item and coalesce to the latest desired + // set. This structurally removes the overlap class: no stale completion + // can clobber a newer set, and `confirmed` always tracks the last + // server-acknowledged tags so a failed save reverts to server truth (not + // to an optimistic, unconfirmed value). Scoped by item id so a save still + // draining for a previous item can't touch the current one. Per Codex + // PR #659 rounds 1/4/5. + type TagSaver = { + itemId: string; + ws: string; + pending: string[] | null; // latest desired set not yet sent (coalesced) + desired: string[]; // latest desired set (in-flight OR pending) for reload reapply + running: boolean; + confirmed: string; // last server-acknowledged tags JSON (revert target) + }; + // Keyed by item id so each item's in-flight saver stays discoverable across + // navigation — navigating away from A and back must find A's running saver + // and coalesce into it, not spawn a second concurrent A saver. Per Codex + // PR #659 round 6. + const tagSavers = new Map(); + + function updateTags(newTags: string[]) { + if (!item) return; + const targetItem = item; + const targetWs = wsSlug; + // Optimistic so chips react instantly. + item = { ...item, tags: JSON.stringify(newTags) }; + + // Coalesce into this item's running saver if one exists; otherwise + // start a fresh one. The currently-displayed item is the only one whose + // tags can be edited, so targetItem.id keys the right saver. + const existing = tagSavers.get(targetItem.id); + if (existing && existing.running) { + existing.pending = newTags; + existing.desired = newTags; + return; + } + const saver: TagSaver = { + itemId: targetItem.id, + ws: targetWs, + pending: newTags, + desired: newTags, + running: false, + confirmed: targetItem.tags // confirmed baseline captured at burst start + }; + tagSavers.set(targetItem.id, saver); + void flushTagSaver(saver); + } + + async function flushTagSaver(saver: TagSaver) { + saver.running = true; + saveStatus = 'saving'; + try { + while (saver.pending !== null) { + const toSave = saver.pending; + saver.pending = null; + const fresh = await api.items.update(saver.ws, saver.itemId, { + tags: JSON.stringify(toSave) + }); + saver.confirmed = fresh.tags; + // Reconcile the UI to server truth only when nothing newer is + // queued (avoids flicker) and we're still on this item. Route + // through adoptServerItem so the tag PATCH echo can't clobber + // unsaved editor content — its response carries the server's + // `content`, and non-collab editors mirror item.content. Per + // Codex PR #659 round 11. + if (saver.pending === null && item && item.id === saver.itemId) { + item = adoptServerItem(fresh); + } + } + if (item && item.id === saver.itemId) showSaved(); + // A newly-created tag should appear in autocomplete next time. + void loadTagSuggestions(saver.ws); + } catch (e) { + console.error('Failed to save tags:', e); + saver.pending = null; + if (item && item.id === saver.itemId) { + // Revert to the last server-confirmed tags, not the optimistic set. + item = { ...item, tags: saver.confirmed }; + saveStatus = 'idle'; + toastStore.show('Failed to save', 'error'); + } + } finally { + saver.running = false; + // Drop the entry once fully drained so the map doesn't accumulate + // stale savers; guard on identity so a newer saver isn't evicted. + if (saver.pending === null && tagSavers.get(saver.itemId) === saver) { + tagSavers.delete(saver.itemId); + } + } + } + + // Overlay an in-flight optimistic tag edit onto any server snapshot of an + // item before it's assigned to `item`. EVERY `item = ` + // assignment (realtime SSE/sync, content-save echo, post-action refresh, + // initial load) must route through this: a tag PATCH owns `item.tags` until + // it drains, and the refresh handlers' `saveStatus === 'saving'` guard is + // racy (checked before their own await, not after), so overlaying the + // saver's latest desired set at assignment time is the only race-free + // guarantee that a concurrent snapshot can't drop the unsaved tags. Per + // Codex PR #659 rounds 8/9. + function withInflightTags(next: Item): Item { + const saver = tagSavers.get(next.id); + return saver?.running ? { ...next, tags: JSON.stringify(saver.desired) } : next; + } + + // Realtime-refresh convenience: applies the content-adoption rule (under + // collab the editor reads Y.Doc so adopt server content verbatim; otherwise + // preserve the live local content) and the tag overlay. + function adoptServerItem(updated: Item): Item { + return withInflightTags( + collabProvider ? updated : { ...updated, content: item?.content ?? updated.content } + ); + } + + // Load the workspace's distinct tags for autocomplete. Pure data-fetch + // keyed on the workspace slug (see the $effect below) — kept separate from + // the item-load path per the Svelte 5 effect-splitting convention. + async function loadTagSuggestions(ws: string) { + if (!ws) return; + try { + const all = await api.tags.list(ws); + // Drop stale results: if the workspace changed while this request + // was in flight (page instance reused across navigation, or a + // post-save reload for a now-previous workspace), don't overwrite + // the current workspace's suggestions. Per Codex PR #659 round 2. + if (ws !== wsSlug) return; + tagSuggestions = all.map((t) => t.tag); + } catch { + if (ws === wsSlug) tagSuggestions = []; + } + } + + $effect(() => { + loadTagSuggestions(wsSlug); + }); + // stampSourceUrl writes the pad_source_url + pad_imported_at orphan // keys into the item's `fields` JSON. The keys aren't declared in // any collection schema — `internal/items/validate.go` only iterates @@ -1157,7 +1311,7 @@ fields: JSON.stringify(merged) }); if (item && item.id === targetItem.id) { - item = fresh; + item = withInflightTags(fresh); } } catch (err) { // Non-fatal: the content was inserted regardless of whether @@ -1270,7 +1424,7 @@ } else { update.clear_assigned_user = true; } - item = await api.items.update(wsSlug, item.id, update); + item = withInflightTags(await api.items.update(wsSlug, item.id, update)); showSaved(); } catch { saveStatus = 'idle'; @@ -1288,7 +1442,7 @@ } else { update.clear_agent_role = true; } - item = await api.items.update(wsSlug, item.id, update); + item = withInflightTags(await api.items.update(wsSlug, item.id, update)); showSaved(); } catch { saveStatus = 'idle'; @@ -1577,7 +1731,7 @@ // drop the queued edit. Mirrors the Round 8 fix in // flushRawIfPending. Per Codex review round 9. if (rawPendingMarkdown === toSave) { - item = updated; + item = withInflightTags(updated); rawPendingMarkdown = null; editorStore.setDirty(false); showSaved(); @@ -1585,7 +1739,7 @@ // Newer pending edit; keep local content, adopt // server-side metadata only. The next debounce // cycle will land the queued edit. - item = { ...updated, content: item.content }; + item = withInflightTags({ ...updated, content: item.content }); } }).catch(() => { if (!item || item.id !== reqItemId) return; @@ -1666,13 +1820,13 @@ // from under the user's keystrokes and lose the // queued edit. Per Codex review round 8. if (rawPendingMarkdown === markdown) { - item = updated; + item = withInflightTags(updated); rawPendingMarkdown = null; } else { // Newer edit pending — keep our local // content but adopt server-side metadata // (timestamps, version, modified_by). - item = { ...updated, content: item.content }; + item = withInflightTags({ ...updated, content: item.content }); } } catch { saveStatus = 'idle'; @@ -1838,7 +1992,7 @@ } function handleVersionRestore(updatedItem: Item) { - item = updatedItem; + item = withInflightTags(updatedItem); } async function handleDelete() { @@ -1865,7 +2019,7 @@ itemLinks = itemLinks.filter(l => l.id !== linkId); // Refresh item to update parent info const refreshed = await api.items.get(wsSlug, itemSlug); - item = { ...refreshed, content: item.content }; + item = withInflightTags({ ...refreshed, content: item.content }); toastStore.show('Relationship removed', 'success'); } catch (e: any) { toastStore.show(e.message ?? 'Failed to remove relationship', 'error'); @@ -1913,7 +2067,7 @@ addLinkResults = []; // Refresh item to update parent info const refreshed = await api.items.get(wsSlug, itemSlug); - item = { ...refreshed, content: item.content }; + item = withInflightTags({ ...refreshed, content: item.content }); toastStore.show('Relationship added', 'success'); } catch (e: any) { toastStore.show(e.message ?? 'Failed to add relationship', 'error'); @@ -2343,6 +2497,19 @@ {/if} {/each} + +
+ Tags +
+ +
+
+ {#if workspaceMembers.length > 0 || agentRoles.length > 0}
Assignment