mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-03 20:20:29 +00:00
e83e7ef413b83114f0ee7e30c74ae405eb2f7865
612 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
e83e7ef413 |
feat(api): add /items/index skinny-projection endpoint (TASK-1344) (#486)
* feat(api): add /items/index skinny-projection endpoint (TASK-1344)
Foundation for PLAN-1343 (local-first read model). Adds a new
GET /api/v1/workspaces/{ws}/items/index endpoint that returns
every item in a workspace minus the rich-text `content` body,
so the client can hydrate an in-memory + IndexedDB index from a
single request and render every collection page from local
state without re-fetching.
Response: {items, total, cursor}. The cursor placeholder is the
max(updated_at) across the result set — Phase 2 replaces it with
a monotonic `seq` cursor. Sort is updated_at DESC, id ASC for
deterministic, cursor-friendly ordering.
Auth uses the same collection-visibility + item-grant filter as
handleListItems. Optional ?collection=<slug> filter for use by
collection pages. ?include_archived=true mirrors the existing
list behavior.
Parent: PLAN-1343.
* fix(api): move skinny-projection endpoint to /items-index per Codex review (round 1)
Codex round 1 [P2] flagged that the original `/items/index` path
shadowed the detail URL of any item whose slug is `index` — slugify
emits `index` for a title of "Index", and chi's static-over-wildcard
preference would route `GET /items/index` to the new index handler
instead of the existing `/items/{itemSlug}` detail handler.
Move the endpoint up to the workspace level as `/items-index`, sibling
to the existing `/plans-progress` route. Slugs cannot contain hyphens
adjacent to identifiers in a way that would collide with a static
workspace-level path, so this URL space is permanently safe.
New test `TestListItemsIndex_DoesNotShadowItemSlug` locks in the
contract: a real item titled "Index" still resolves through
`/items/{itemSlug}`, while `/items-index` returns the index wrapper.
|
||
|
xarmian
|
bdcb62e902 |
fix(api): accept nested object/array for PATCH items fields/tags (BUG-1144) (#485)
The PATCH /api/v1/workspaces/{ws}/items/{ref} endpoint previously
demanded `fields` and `tags` arrive as JSON-encoded strings, because
models.ItemUpdate declares them as *string to mirror the storage shape.
Sending the natural nested-object shape any reasonable HTTP client
would produce returned HTTP 400 with a leaked Go unmarshal error
naming the internal struct field — confusing for anyone integrating
against Pad over HTTP (webhook reactors, custom dashboards, non-CLI
agents, third-party MCP bridges).
This is the symmetric input-side counterpart to BUG-991, which was
fixed at the MCP boundary in PR #364 with dual-emit normalization
rather than the full Plan-sized models.Item migration.
Fix: add a custom ItemUpdate.UnmarshalJSON that accepts either shape
on the wire and normalizes to the canonical string internally. The
struct field type stays *string, so the validation/storage/web/CLI
pipeline is untouched. All in-process Go callers construct ItemUpdate
literals (15 grepped call sites) and never hit UnmarshalJSON, so the
change is invisible to them.
Wrong shapes (e.g. `{"fields":42}`, `{"tags":{"x":1}}`) now return a
domain-level 400 — `"fields" must be a JSON object or a JSON-encoded
string` — surfaced via sentinel errors (ErrInvalidFieldsType /
ErrInvalidTagsType) that the handler unwraps from decodeJSON's
"invalid JSON: %w" wrapper.
Coverage:
- models/item_test.go: 10 sub-tests covering object, array, string,
null, absent, and wrong-type cases for both fields and tags.
- server/handlers_items_test.go: 6 PATCH integration sub-tests
asserting back-compat, the BUG-1144 repro now returns 200, and
that error responses no longer leak Go struct field names.
Smoke-tested against the live server with the exact repro curl from
BUG-1144 (HTTP 200), plus malformed (HTTP 400 with clean message)
and stringified-string back-compat (HTTP 200).
|
||
|
xarmian
|
9fecb82e82 |
docs: surface --schema flag in SKILL.md + CLAUDE.md (TASK-1336) (#484)
PR #482 / TASK-1334 added the --schema flag to `pad collection create`. PR #483 / TASK-1335 wired it through MCP. Agents reading the live SKILL.md (embedded in the binary) need to see the new path or they'll keep reaching for --fields DSL and hit the original BUG-1284 symptom. Updates: - skills/pad/SKILL.md "Collections" section: now shows both --fields DSL and --schema JSON forms side-by-side. Calls out exactly when --schema is required (terminal_options et al.), and notes the label-from-key auto-fill so agents know empty labels are safe. - CLAUDE.md command list: adds the --schema variant alongside the existing --fields example so the project-level cheat sheet stays accurate. The CLI Long help (`pad collection create --help`) and the MCP tool description were already updated in TASK-1334 / TASK-1335. Parent: PLAN-1333. |
||
|
xarmian
|
a28927c51e |
feat(mcp): add typed schema param to pad_collection.create (TASK-1335) (#483)
* feat(mcp): add typed schema param to pad_collection.create (TASK-1335)
TASK-1334 shipped the CLI --schema flag for BUG-1284. This wires the
same capability through the MCP surface so agents calling pad_collection
get a structured object parameter rather than a stringified DSL —
matching how every other Pad-native data structure (items, comments,
etc.) flows through MCP.
Three coordinated changes:
1. internal/mcp/catalog.go — add "object" Type to ParamDef, mapped to
mcp-go's WithObject. Generic addition; the schema param is the first
consumer.
2. internal/mcp/catalog_collection.go — declare schema (object) param
on pad_collection.create alongside the existing fields (string)
param. Tool description now points agents at schema for
terminal_options, defaults, computed fields, suffixes, and relation
collections (everything the DSL cannot express), and notes the
fields/schema mutual exclusion.
3a. internal/mcp/dispatch.go — BuildCLIArgs now JSON-encodes
non-string structured values via a new encodeFlagValue helper. The
ExecDispatcher path (`pad mcp serve`) hands MCP input through to
the CLI's --schema flag as inline JSON; previously fmt.Sprint(map)
produced garbage Go-syntax. Generic — applies to any future flag
that accepts JSON.
3b. internal/mcp/dispatch_http_routes.go — mapCollectionCreate accepts
`schema` (object or stringified JSON) alongside `fields`, errors
if both are set, and backfills missing field labels using
titleCaseLabel so HTTPHandlerDispatcher MCP clients see the same
rendering as CLI users.
Tests cover: object→JSON encoding in BuildCLIArgs, array→JSON encoding,
structured schema input round-trips through mapCollectionCreate with
terminal_options preserved, missing-label backfill, stringified-JSON
fallback shape, both-flags rejection, malformed-JSON rejection.
Parent: PLAN-1333.
* fix(mcp): normalize empty/null schema input to absent per Codex round 1
Codex flagged a transport mismatch: the CLI treats `--schema ""` as
absent (falls through to --fields), but the HTTP dispatcher treated
schema=null or schema="" as set, triggering the mutually-exclusive
guard against fields or an invalid-JSON error from unmarshal.
Fix: in mapCollectionCreate, normalize nil and empty/whitespace strings
to "schema absent" before the mutex check. Keeps both transports
symmetric so MCP clients sending optional-arg defaults (null/empty) get
identical behavior to CLI users passing --schema "".
Test: TestMapCollectionCreate_EmptySchemaFallsThroughToFields covers
nil, "", and whitespace-only inputs in subtests, asserting the request
body uses the --fields-parsed schema.
Parent: PLAN-1333 / TASK-1335.
|
||
|
xarmian
|
25a21184a2 |
feat(cli): add --schema flag to pad collection create (TASK-1334) (#482)
* feat(cli): add --schema flag to pad collection create (TASK-1334) The existing --fields DSL (key:type[:options]) had no syntax for terminal_options, default, required, computed, suffix, or relation collection — every CLI-created collection lost those FieldDef properties even though the model already supports them. Symptom from BUG-1284: dashboard "active" counts treat published/archived items as in-progress because the persisted schema has no terminal_options. Adds a new --schema flag that accepts the full CollectionSchema JSON, which captures every current and future FieldDef property automatically. Three input modes: --schema '<json>' inline literal (agent-natural; CLI is agent-first) --schema @./path.json file path --schema - stdin --fields and --schema are mutually exclusive; --fields keeps working unchanged for backward compat (no deprecation). Refactors the inline parser into three testable helpers in main.go: collectionSchemaJSONFromFlags (orchestrator), readSchemaInputBytes (input resolver), and parseFieldsDSL (legacy DSL parser preserving the "first status select gets required+default" heuristic). Tests: 9 table-style cases in collection_create_schema_test.go covering all three input modes, the mutually-exclusive guard, malformed JSON, missing file, fallthrough-to-DSL, both-empty, and a regression test that verifies terminal_options + computed + suffix + relation.collection all round-trip through --schema. Parent: PLAN-1333. * fix(cli): backfill missing labels in --schema fields per Codex review (round 2) Codex flagged that the --schema example omitted "label", which the parser preserved as label:"" — agents constructing JSON could create collections that render blank field headers in the web UI. Fix: after unmarshaling --schema input, backfill any FieldDef with an empty Label using the same Title-Case-of-key heuristic the legacy --fields DSL applies (e.g. "due_date" → "Due Date"). Explicit labels are preserved. Also updated the help-text example to include "label" on the status field so the canonical shape is visible, plus a tip line documenting the auto-fill behavior so users know it's safe to omit labels. Test: TestCollectionSchemaJSONFromFlags_BackfillsMissingLabels covers auto-fill, multi-word key normalization, and the explicit-label-not- clobbered case. Parent: PLAN-1333 / TASK-1334. |
||
|
xarmian
|
e01c1581d0 |
fix(web): wire New Collection dashboard card to CreateCollectionModal (BUG-1332) (#481)
* fix(web): wire New Collection dashboard card to CreateCollectionModal (BUG-1332)
The "+ New Collection" tile in the workspace dashboard's Collections grid
was a plain <a href=".../settings"> link, so clicking it navigated to the
workspace Settings page instead of starting the create-collection flow.
Swapped the anchor for a <button> that opens the existing
CreateCollectionModal (same pattern Sidebar.svelte already uses for its
"+" affordance). The oncreated handler refreshes the dashboard so the new
collection card appears immediately without a full reload. Added a small
scoped reset on button.coll-card-new (font, text-align, cursor, width) so
it's visually indistinguishable from the sibling <a> cards.
* fix(web): gate dashboard New Collection on isOwner; refresh collectionStore per Codex review (round 1)
Codex review of round 0 surfaced two findings, both addressed here:
P2 — Owner gating: the New Collection dashboard trigger and modal were
shown to all viewers. The server requires owner role for collection
create (handlers_collections.go:48), and the settings page already gates
this modal behind isOwner. Without the gate, non-owners could open the
full create flow and only learn it's forbidden on submit. Wrapped both
the trigger button and the modal mount in {#if isOwner}, mirroring the
settings page's pattern.
P3 — Sidebar staleness: oncreated only refreshed dashboard-local state
via load(), so the Sidebar and quick-add (which read from
collectionStore) didn't see new collections until another refresh path
ran. Added a collectionStore.loadCollections(wsSlug) call alongside the
existing load(), matching Sidebar.svelte's own create-flow pattern.
* fix(web): keep CreateCollectionModal mounted across isOwner flicker per Codex review (round 2)
Round 1 wrapped the dashboard's CreateCollectionModal in {#if isOwner},
which Codex round-2 caught as a regression: the dashboard's 30s poll
(and any sync signal) calls load() → workspaceStore.setCurrent(), which
transiently clears currentMembership before /me resolves. isOwner flips
false during that window, unmounting the modal mid-edit and dropping any
form state the owner had typed.
Switched the modal-level gate from {#if isOwner} to {#if wsSlug} — the
same pattern Sidebar.svelte uses. The trigger button stays owner-gated
(UX boundary), and handlers_collections.go:48 remains the security
boundary, so this regression-free path preserves both protections.
* fix(web): cache isOwner so New Collection trigger doesn't flicker per Codex review (round 3)
Round 2 fixed the modal unmount race but left the trigger button reading
workspaceStore.isOwner directly, which Codex round-3 caught as a related
regression: the button itself disappears/reappears every 30s as the
dashboard's silent poll calls workspaceStore.setCurrent() → clears
currentMembership before /me resolves. Drops focus on the CTA every
refresh.
Cached the page-local isOwner via two effects per CONVE-606 (split
reactive-state sync from route-change effects):
1. On wsSlug change → reset cached isOwner to false (so we never leak
the prior workspace's owner status into the new workspace's CTA,
and never flash owner-only UI before /me confirms).
2. On workspaceStore.currentMembership change → only update when
non-null. The transient null windows during silent refreshes are
ignored, so the cached value (and the trigger's visibility) stays
stable.
Server-side enforcement (handlers_collections.go:48) remains the
security boundary; this is purely a UX stability fix.
|
||
|
xarmian
|
671ecabc41 |
fix(console): mobile hamburger toggles correctly on tap (BUG-1330) (#480)
The /console mobile hamburger button never opened its dropdown on tap.
A tap on the closed-state SVG inside the button triggered the toggle's
onclick (`mobileMenuOpen = true`), but Svelte 5 then synced the
{#if mobileMenuOpen}{:else}{/if} swap *before* the bubbled
`<svelte:window onclick={handleWindowClick}>` listener ran. By that
point the original `<rect>` click target was detached from the DOM
(`event.target.isConnected === false`); `target.closest('.console-nav')`
walked an orphaned subtree and returned null, the outside-click branch
fired, and the menu was reset to closed in the same tick — visually
"never opened."
Verified the timing in a Svelte 5 playground that mirrors the pattern;
the window handler logged `target=rect, isConnected=false,
closest(.nav)=NULL` for every tap.
Two-layer fix in web/src/routes/console/+layout.svelte:
1. Add `pointer-events: none` to `.mobile-hamburger svg` and its
children so the click target is always the button itself, which is
never re-rendered/detached when `mobileMenuOpen` flips. This is the
primary fix and matches the standard "icons inside buttons should
not capture pointer events" pattern.
2. Stop propagation on the toggle button's onclick so the click cannot
reach `handleWindowClick` even if a future change adds an inner
element without the same guard. Belt-and-braces.
Inline comments record the BUG-1330 root cause so the next person to
touch this nav doesn't reintroduce the SVG-swap.
TopBar.svelte's mobile hamburger is unaffected — its SVG content
doesn't swap on toggle (same icon regardless of sidebar state), so the
detach race never fires there.
|
||
|
xarmian
|
4b887c77db |
fix(editor): block drag handle picks up atom block-level nodes (TASK-1329) (#479)
* fix(editor): block drag handle picks up atom block-level nodes (TASK-1329) `BlockDragHandle.blockAtPos` rejected `depth === 0` outright, so atom block-level nodes (e.g. the new htmlBlock from PLAN-1322) never got a drag handle. When the cursor hovers over an atom block, posAtCoords returns a position at the doc boundary between top-level children; that resolves to depth 0, which the existing logic treats as "no enclosing block." Fix: when depth is 0, look at the node AT `pos` (after the boundary) and the node immediately before `pos` (walking doc children to find the sibling whose end matches `pos`). If either is an atom block-level node, return its block info so the handle shows up. Non-atom or inline content still returns null — same as before. The "Turn into" context menu still doesn't apply to atom blocks (they don't map to any TURN_INTO_ITEMS), so tapping the handle on an htmlBlock opens an empty / no-op menu in v1. Drag-to-reorder is the primary use case and is what users asked for here. Atom-block "Turn into" (e.g. convert htmlBlock ↔ codeBlock) is a follow-up. Parent: PLAN-1322. * fix(editor): hide Turn-into entries for atom blocks per Codex review (round 1) * fix(editor): measure menu height after visibility toggle (Codex round 2) |
||
|
xarmian
|
08904c9a60 |
feat(versions): collapse HTML blocks as semantic units in diff view (TASK-1328) (#478)
* feat(versions): collapse HTML blocks as semantic units in diff view (TASK-1328)
Adds a chunker that groups ` ```html ` fenced-block contents into a single
collapsed summary row in DiffView.svelte when the block contains any
added/removed lines. Avoids tag-soup diffs in the audit trail —
reviewing a marketing item or styled email with one HTML island no
longer floods the diff with N raw markup lines.
## How it works
The pipeline becomes:
diffLines(old, new) → buildDiffLines → chunkHtmlBlocks → collapseContext → render
chunkHtmlBlocks walks the line list looking for `^(\`{3,})html$` openers
and exact closing fence matches. Within a fenced range:
- Any added/removed line inside → emit one `{ kind: 'htmlBlock' }`
entry with counts of added/removed/unchanged lines
- All lines unchanged → pass through as individual lines (so the
existing context-collapse can compress them as ordinary unchanged
context — entirely-unchanged blocks don't get a special UI)
- Unbalanced fence (open with no close) → fall through, emit opener
as a plain line. Graceful degradation.
collapseContext is updated to operate on the new MidEntry[] shape
(discriminated union of line / htmlBlock). An htmlBlock entry counts
as a "change" for visibility-window purposes; hidden runs only tally
plain line entries when reporting skipped count.
## UX
The summary row is a <button> showing:
▶ HTML block changed +3 -2 (5 unchanged) click to expand
Click toggles a per-entry expansion state (SvelteSet keyed by display
index, so reactivity propagates without reassignment). When expanded,
the inner lines render using the existing .diff-line markup — no new
diff algorithm; we reuse the line-level diff already computed by
buildDiffLines.
The header stats badge still reads from diffLineEntries (the line-level
list), so total +/- counts across the whole diff include changes
inside chunks.
## Out of scope (future work)
- Semantic HTML diff (attribute moved, child reordered) — line-level
diff with collapse is enough; semantic HTML diffing is much harder
- Server-side diff storage changes — internal/diff/ unchanged; this
is purely a presentation concern
- Diff-view changes for any other content type (code blocks,
attachments, etc.) — htmlBlock-specific
Parent: PLAN-1322. Closes the plan: htmlBlock node, markdown round-
trip, render-time sanitization, source-view editor, insertion UX,
hidden-content authoring warning, and now diff collapse — all merged.
* fix(versions): chunk html blocks by per-side line ranges (Codex round 1)
|
||
|
xarmian
|
2bbd9c9e36 |
feat(editor): hidden-content detector + non-blocking authoring warning (TASK-1327) (#477)
* feat(editor): hidden-content detector + non-blocking authoring warning (TASK-1327)
Adds an authoring-honesty feature for HTML blocks: when the user pastes
or types raw HTML containing content that's invisible (or
near-invisible) to humans but readable by LLMs / agents, surface a
non-blocking warning pill above the block. Click the pill to expand an
inspector listing each hidden segment with the rule that flagged it
plus a snippet for context. "Dismiss for this block" button persists
acknowledgement via a sentinel HTML comment marker so the warning
doesn't re-fire after a doc reload.
NOT a security control. Render-time sanitization (sanitizeHtmlBlock,
TASK-1323) protects browsers from XSS. This protects authors from
unconsciously shipping content that looks one way and reads another
— common with copy-pasted HTML blobs that contain steganography
channels (display:none divs, white-on-white text, font-size:0,
hidden HTML comments, off-screen positioning, suspiciously long
aria-label / alt / title values).
## Detector
`web/src/lib/utils/hiddenContentDetector.ts` (NEW). Pure function
`detectHiddenContent(html: string): HiddenSegment[]`. Uses DOMParser
to walk the HTML tree:
- Inline-style heuristics:
- display:none, visibility:hidden, opacity:0/0%/0.0
- font-size with px value < 6
- color matches background-color (exact normalised match)
- position absolute/fixed with left/right/top/bottom <= -9000px
- transform translate to off-screen (heuristic regex on -9XXX or
-1XXXX values inside translateX/Y/translate)
- width:0 AND height:0
- clip:rect(0,0,0,0) — intentionally flagged; the user can dismiss
if it's deliberate sr-only positioning, but it's also a known
steganography channel
- HTML comments — every <!-- ... --> flags, EXCEPT the Pad-internal
ack marker (PAD_ACK_HIDDEN_MARKER) which the detector skips
- aria-label / alt / title attributes longer than 200 chars OR
containing newlines
Class-based hiding (e.g. .sr-only) is NOT flagged: resolving it
requires the page's stylesheet context, which we don't have, and the
false-positive rate is too high.
## NodeView UX
Warning pill ⚠ "N hidden segment(s) — click to inspect" appears above
the preview pane when segments > 0 AND the user hasn't dismissed for
this block. Click toggles the inspector panel below the source pane,
which lists each segment as `<code>tag</code> — rule` with the snippet
in a quoted monospace box. Dismiss button prepends
`<!-- pad:ack-hidden -->` to attrs.html via setNodeMarkup; the marker
survives markdown round-trip (it's a valid HTML comment) and the
detector skips it on subsequent runs.
The wrapper gets `.html-block--has-hidden` while the warning is
showing, in case any caller wants to react.
`updateWarning()` runs on every renderPreview call, so the warning
follows attrs.html changes (e.g. the user edits the block in source
mode and removes the hidden content — warning disappears immediately).
## Out of scope (future work)
- Class-based hiding detection (requires stylesheet resolution)
- Auto-stripping hidden content (this is an authoring honesty
feature, not a sanitizer; the user decides)
- Detection in non-HTML-block content (markdown comments, raw HTML
in markdown surface)
Parent: PLAN-1322.
* fix(detector): walk comments at doc root + normalize style values (Codex round 1)
* fix(detector): use CSSStyleDeclaration for browser-correct parsing (Codex round 2)
|
||
|
xarmian
|
781fe86f0e |
feat(editor): slash menu, toolbar, markdown shortcut to insert htmlBlock (TASK-1326) (#476)
* feat(editor): slash menu, toolbar, markdown shortcut to insert htmlBlock (TASK-1326) Three insertion paths for the htmlBlock node, all landing the user in source mode so they can immediately type HTML: 1. **Slash menu** — block-types.ts gets a new SLASH_ITEMS entry (id=htmlBlock, icon='HTML', label='HTML Block', insertOnly=true). execSlash dispatches setHtmlBlock + a requestAnimationFrame click on the empty-state placeholder to enter source mode. 2. **Toolbar** — EditorToolbar.svelte gets an 'HTML' button in the 'blocks' group, after the table button. Same setHtmlBlock + auto-flip pattern. 3. **Markdown shortcut** — htmlBlock.ts adds a new ProseMirror InputRule matching `^```html[\s\n]$`. Replaces the typed text with an empty htmlBlock node. The extension's priority is bumped to 1000 (default 100) so this rule wins against CodeBlock's broader `^```([a-z]+)?[\s\n]$` rule — without this, typing ``` ```html ``` + Enter would create a code block with language=html, not an htmlBlock. The auto-flip-to-source heuristic queries `.html-block:not(.html-block--editing) .html-block-empty` and clicks the preview pane on the next frame after insertion. This works because a freshly inserted block is always empty (empty placeholder visible) and never in --editing mode (--editing is only set when the user explicitly clicks). Multiple new empty blocks would in principle race-flip, but realistically only one is inserted at a time. Out of scope (TASK-1327, TASK-1328 follow): - Hidden-content authoring warning - Diff view collapse Parent: PLAN-1322. * fix(editor): target just-inserted htmlBlock by position + flip from input rule (Codex round 1) * fix(editor): scan for just-inserted htmlBlock + capture editor in input rule (Codex round 2) * fix(editor): capture insertion point before insert + walk forward to find new htmlBlock (Codex round 3) * fix(editor): disambiguate new htmlBlock via before-position snapshot (Codex round 4) * fix(editor): attrs-aware htmlBlock snapshot — handle replace + adjacent cases (Codex round 5) |
||
|
xarmian
|
c02aedd4ac |
feat(editor): source-view toggle for htmlBlock nodes (TASK-1325) (#475)
* feat(editor): source-view toggle for htmlBlock nodes (TASK-1325) Extends the htmlBlock NodeView with a click-to-edit source pane. The block renders sanitized live HTML by default; clicking the preview flips to a raw HTML textarea bound to attrs.html. Blur, Escape, or Cmd/Ctrl+Enter commits via setNodeMarkup and flips back to preview. Behavior: - Click anywhere in the rendered preview → flip to source mode and focus the textarea with caret at end. Clicks on interactive descendants (a, button, iframe, input, textarea, select, video, audio) pass through normally so embedded controls stay clickable. - Escape: preventDefault + commitAndFlip. Per task spec, Escape commits rather than cancelling — matches the project's existing block UX where edits aren't undone by escape. - Cmd/Ctrl+Enter: same as Escape — one-shot commit-and-flip. - Blur: also commits. The handler is idempotent (commit early-returns when textarea.value === lastHtml) so the Done-button click path doesn't double-commit when blur fires after the click. - Done button: mousedown.preventDefault keeps focus on the textarea so the click handler runs in the same selection context. Without that, the button would steal focus → blur → commitAndFlip → click on a hidden element no-op. - Empty block: shows "Empty HTML block — click to edit" placeholder so the atom node remains discoverable when attrs.html is empty. NodeView's update() handler re-renders only the preview when external attrs.html changes (e.g. via collab transactions). The textarea isn't auto-synced — if the user is mid-edit when a remote change lands, their in-progress text wins on the next commit. Last-write-wins is fine for v1; collab-aware merge would be its own task. CSS lives in Editor.svelte's <style> block immediately after the mermaid-source rule, using the same .editor-content :global(...) pattern as every other block-level element. The wrapper toggles between preview and source via the .html-block--editing class. Out of scope (separate tasks): - TASK-1326 — slash menu / toolbar / markdown shortcut to insert - TASK-1327 — hidden-content authoring warning - TASK-1328 — diff view collapse Parent: PLAN-1322. * fix(editor): isolate htmlBlock textarea events + gate edit on isEditable per Codex review (round 1) |
||
|
xarmian
|
40c9ec2a89 |
feat(editor): add htmlBlock node + markdown round-trip (TASK-1324) (#474)
Defines the foundation Node for PLAN-1322: an atomic block (atom: true) that round-trips ` ```html ` fenced blocks ↔ an editor node, rendered as sanitized live HTML in the WYSIWYG view via a NodeView. How the round-trip works: - markdown → node: tiptap-markdown's parse pipeline runs each fenced block through markdown-it's renderer.rules.fence. The new extension installs a wrapper that intercepts info === 'html' and emits a <div data-pad-html-block data-html="…escaped…"></div>. Other languages pass through to the original fence renderer (MermaidCodeBlock and syntax-highlighted code blocks unaffected). - node → markdown: addStorage().markdown.serialize emits ` ```html ` with a fence one backtick longer than the longest run inside the body, so a literal triple-backtick in the user's HTML can't close the fence early. Body always ends in a trailing newline before the closing fence. Sanitization is render-time only — attrs.html stores the raw user input verbatim (sanitizeHtmlBlock from TASK-1323 runs in the NodeView's update path, not at write time). That keeps the storage lossless for a future source-view editor (TASK-1325) and version-diff view (TASK-1328) to show what was actually typed. NodeView mirrors MermaidCodeBlock's pattern: contenteditable=false wrapper, ignoreMutation true, update() re-runs sanitizeHtmlBlock when attrs.html changes. Wired into Editor.svelte's extensions array immediately after MermaidCodeBlock so the markdown parser override runs after the default fence handler is in place. No insertion UX in this PR (no slash menu, no toolbar button, no keyboard shortcut, no source-view editor). Those are TASK-1325 (source view), TASK-1326 (insert UX), TASK-1327 (hidden-content warning), and TASK-1328 (diff collapse). Parent: PLAN-1322. |
||
|
xarmian
|
ac09035d01 |
feat(web): add sanitizeHtmlBlock with iframe-host allowlist (TASK-1323) (#473)
Introduces a second client-side sanitizer alongside sanitizeMarkdownHtml, intended for the future htmlBlock node type (TASK-1324). Differences from the markdown sanitizer: - Permits <iframe>, but only when src matches one of four embed hosts (YouTube, Vimeo, Loom, CodeSandbox). Enforced via a one-shot uponSanitizeElement DOMPurify hook scoped to the call. - Permits inline `style` attributes (styled callouts are the use case). - Adds structural tags (section, article, aside, header, footer, main, nav, figure, figcaption, picture, source) and media tags (video, audio) plus their relevant attributes. - Otherwise identical: strips <script>, on*/event handlers, javascript: and data: URLs. The markdown surface (comments + rendered item bodies) is unchanged — sanitizeMarkdownHtml keeps its tighter allowlist. autoplay is intentionally not permitted; embed providers handle autoplay via query parameters when intentional. Also exports isAllowedIframeSrc for use by future detectors / UX hints. Parent: PLAN-1322. |
||
|
xarmian
|
18087463ce |
feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319) (#472)
* feat(collab): op-log cursor protocol — force-refresh + watermark advance (TASK-1319)
Closes both holes left by TASK-1309:
1. Long-disconnected tab + external-write race. A reconnecting client
announces its highest applied item_yjs_updates.id via `?since=<id>`.
If that id is below MIN(id) for the item, rows it expected to
replay have been pruned and the server sends a `force_refresh`
control frame and closes the conn. Client recreates the Y.Doc
and lazy-seeds from items.content. Without this, Tab A's stale
state would silently overwrite an external CLI/MCP write on the
next 5s flush.
2. Browser-only-edited items never GC'd. Browser collab-snapshot
PATCHes now carry an op_log_cursor body field. The store advances
items.content_flushed_op_log_id only when the cursor matches the
current MAX(op-log.id) — proving the markdown captures every
persisted op. SQL CASE clause re-evaluates MAX at COMMIT time so
a peer op landing between client-side cursor capture and the
UPDATE leaves the watermark untouched (no over-advancement).
Combined cursor mechanism:
- Server attaches op_log_cursor JSON control frames after replay,
after every successful AppendYjsUpdate (originator), and to every
peer's binary fan-out (so all peers stay in lockstep without a
round trip).
- Client persists per-tab in sessionStorage (NOT localStorage —
avoids cross-tab cursor leakage that would force-refresh stable
sessions).
- Server's MIN(id) check + force_refresh fires only when a non-zero
`since` is below MIN; `since=0` is treated as a fresh client.
New store methods: MinOpLogID, MaxOpLogID. New ItemUpdate field:
OpLogCursor *int64. New control message types: op_log_cursor,
force_refresh. New OpEvent.OpLogID for cursor piggyback. Existing
collab tests updated to drain TextMessage cursor frames.
Tests cover: initial cursor frame after replay (populated + empty
op-log), force_refresh fires when since<MIN, delta replay when
since>=MIN, cursor broadcast to originator + peers on append, and
watermark advancement gated on cursor==MAX.
Parent: PLAN-1248. Builds on TASK-1309.
* fix(collab): skip stale-Ydoc flush on force_refresh teardown per Codex review (round 1)
A force_refresh tear-down means the local Y.Doc cursor is below the
server's MIN(item_yjs_updates.id) — its derived markdown is stale.
Without this guard the collab $effect cleanup runs flushCollabNow
on the way out and silently PATCHes that stale markdown back to
items.content, overwriting the canonical content the fresh provider
is supposed to lazy-seed from. Per Codex round 1 [P1] of TASK-1319.
* fix(collab): force_refresh on empty op-log + cancel pending flush per Codex review (round 2)
Two P1 fixes:
1. Manager.Join now force_refreshes when since>0 and the op-log is
empty (hasMin==false), not just when since<MIN. After
PruneAndApply wipes the entire op-log, MIN is undefined; the
original predicate would have admitted the stale tab and let its
on-open Y.encodeStateAsUpdate write resurrect the pre-prune
document.
2. The +page.svelte onForceRefresh handler now also clears
collabFlushTimer. Without this a 5s timer that armed before the
force_refresh frame arrived can still fire AFTER the cleanup
ran, PATCHing stale Y.Doc-derived markdown to items.content.
New test: TestRoomManagerForceRefreshOnEmptyOpLogWithSince covers
the empty-op-log branch.
Per Codex round 2 [P1] of TASK-1319.
* fix(collab): include forceRefreshNonce in Editor key so it remounts on force_refresh per Codex review (round 3)
The collab $effect cleanup runs on forceRefreshNonce bump, but the
<Editor> {#key} was `${item.id}:true` — itemID doesn't change, so
the keyed Editor wasn't unmounting. The Tiptap Collaboration
extension only binds in onMount, so the editor stayed wired to the
stale (destroyed) Y.Doc while a fresh provider+doc were set up
in parallel. Edits would either be unsynced or eventually flush
stale markdown again.
Adding forceRefreshNonce to the key forces the Editor to remount
in lockstep with the doc swap. Per Codex round 3 [P1] of TASK-1319.
* fix(collab): refetch item.content before lazy-seed on force_refresh per Codex review (round 4)
After force_refresh the collab $effect rebuilds the Y.Doc and the
lazy-seed (TASK-1261) seeds it from item.content. But item.content
was the cached page-state copy — possibly stale relative to the
server (the WS force_refresh can beat the SSE/visibility refresh
that would otherwise update it). Lazy-seeding stale content into
a fresh op-log re-introduces exactly the staleness force_refresh
was supposed to clear: the next 5s flush PATCHes that stale view
back to canonical items.content.
onForceRefresh now does an api.items.get() before bumping the
nonce so the rebuild's lazy seed reads server-fresh content. A
failed fetch falls through to the bump anyway (an editor on
possibly-stale content is still better than a broken editor).
Per Codex round 4 [P1] of TASK-1319.
* fix(collab): suppress cursor during replay + move force_refresh check before getOrCreate per Codex review (round 5)
Two more findings:
1. [P1] writeLoop sends op_log_cursor frames for live ops broadcast
during the replay window. A client disconnecting after one of
those cursors lands but BEFORE the rest of replay completes
would persist a cursor pointing past unreplayed rows. On
reconnect with since=that-cursor, server replays nothing — the
client's Y.Doc would be missing causally-required ops.
Fix: per-roomConn replayDone atomic.Bool. writeLoop suppresses
cursor frames while it's false. runConn flips it after the
post-replay initial cursor is on the wire. Live binary frames
continue to flow during replay (Yjs CRDT commutativity); only
the cursor metadata is gated.
2. [P2] Force-refresh path leaked an empty room. getOrCreate
inserted into m.rooms before the force_refresh bail-out left
an orphan entry that PruneSweep would later treat as 'active'
and skip indefinitely.
Fix: schema-rebuild + force_refresh checks now run BEFORE
getOrCreate. Both are store-only mutations and the per-item
lock is held throughout, so concurrency is unchanged.
New test: TestRoomManagerCursorSuppressedDuringReplay regression-
guards the cursor-suppression behaviour.
Per Codex round 5 [P1+P2] of TASK-1319.
* fix(collab): tighten initial cursor + sync-destroy provider on force_refresh per Codex review (round 6)
Two more P1 fixes:
1. runConn's empty-replay fallback used MaxOpLogID() to anchor
the initial cursor. A live op landing between replayTo
returning and the cursor write would be reflected in MAX
but its binary frame might not have flowed through this
conn's writeLoop yet — the cursor would advertise an id
the client hasn't received. Initial cursor is now strictly
max(highestReplayed, since); MaxOpLogID is removed from
the opLogStore interface.
2. Provider.handleControlMessage's force_refresh branch now
calls this.destroy() SYNCHRONOUSLY before invoking the
onForceRefresh callback. Previously the consumer's recovery
path (async items.get refetch) would race the provider's
own onClose-triggered reconnect, which would re-open with
since=0 and push Y.encodeStateAsUpdate of the stale Y.Doc
— recreating the corruption force_refresh was meant to
prevent. destroy() sets destroyed=true so scheduleReconnect
short-circuits.
Per Codex round 6 [P1] of TASK-1319.
* fix(collab): block flush scheduling during force_refresh recovery per Codex review (round 7)
Previously, after onForceRefresh fires:
1. Provider is destroyed synchronously.
2. Async items.get refetch is in flight.
3. forceRefreshNonce bumps after refetch resolves.
4. $effect cleanup runs, then rebuild.
But during steps 2-3 the editor component is still mounted with
the stale Y.Doc, and a local edit fires handleContentUpdate which
calls scheduleCollabFlush. clearTimeout earlier in onForceRefresh
only canceled the timer at THAT moment; a new edit during the
refetch window arms a fresh timer that fires before cleanup. That
PATCHes stale Y.Doc-derived markdown back to canonical content,
recreating the corruption force_refresh was meant to prevent.
Fix: forceRefreshInFlight flag set in onForceRefresh, blocks
scheduleCollabFlush, resets after the fresh provider is wired
(end of $effect run). Per Codex round 7 [P1].
* fix(collab): gate runCollabFlush itself on force_refresh in-flight per Codex review (round 8)
scheduleCollabFlush blocked the 5s timer path, but direct callers
of flushCollabNow / runCollabFlush (beforeunload handler,
rich-to-raw toggle) bypassed the guard. A page reload or raw
toggle DURING the force_refresh recovery window still PATCHed
stale Y.Doc-derived markdown to canonical items.content.
Pulling the guard into runCollabFlush covers every caller in one
spot and returns 'deduped' so the result-shape contract holds.
Per Codex round 8 [P1] of TASK-1319.
* fix(collab): distinct 'skipped' result for force_refresh path; raw-toggle aborts per Codex review (round 9)
runCollabFlush returning 'deduped' on the force_refresh-blocked
path was indistinguishable from a legitimate same-content dedupe.
The rich→raw toggle treats 'deduped' as 'server already has this
markdown' and seeds rawSeedMarkdown from it — letting the user's
next raw edit overwrite canonical items.content with content
derived from the stale Y.Doc.
Add a distinct 'skipped' result for the force_refresh path. Raw
toggle aborts on it (with a 'try again in a moment' toast); other
callers fall through unchanged because no other call site
behaviorally depends on 'deduped' vs 'skipped'.
Per Codex round 9 [P1] of TASK-1319.
* fix(collab): server-side gate + post-await client guard against stale collab-snapshot per Codex review (round 10)
A force_refresh frame can arrive WHILE a collab-snapshot PATCH is
already mid-flight to the server. The client-side
forceRefreshInFlight check at PATCH-start can't catch this race;
the request lands at the server with stale Y.Doc-derived markdown.
Two-pronged fix:
1. Server: handler now checks op_log_cursor against MIN(op-log.id)
for collab-snapshot PATCHes and returns 409 Conflict when
cursor < MIN. Such cursors prove the flushing tab's Y.Doc was
built on rows that have been pruned (PruneAndApply, schema
rebuild, dormant GC). The markdown is, by construction, stale.
2. Client: post-await check on forceRefreshInFlight returns
'skipped' instead of 'flushed' so saveStatus / lastFlushedContent
don't seed from a known-stale base even if the server happened
to accept the PATCH (e.g. MIN advanced after handler validation).
New tests: TestCollabSnapshotRejectsCursorBelowMin (gate fires),
TestCollabSnapshotAcceptsCursorAtOrAboveMin (negative path).
Also de-leak an unused slice in the round-5 cursor-suppression test
so staticcheck stays clean.
Per Codex round 10 [P1] of TASK-1319.
* fix(collab): reject collab-snapshot when cursor>0 and op-log empty per Codex review (round 11)
The HTTP-layer gate I added in round 10 mirrored only PART of the
WS-upgrade force_refresh predicate. Round 5 had already taught us
that 'op-log entirely pruned' is a separate stale path from
'cursor below MIN' (PruneAndApply, schema rebuild, dormant GC all
leave hasMin=false), and the WS check now uses
`since > 0 && (!hasMin || since < minID)`. The HTTP gate had
only the second clause.
Mirror the WS predicate at the handler so a stale collab-snapshot
PATCH against an empty op-log gets a 409 too. New regression:
TestCollabSnapshotRejectsCursorOnEmptyOpLog.
Per Codex round 11 [P1] of TASK-1319.
* fix(collab): reject collab-snapshot cursor=0 on non-empty op-log per Codex review (round 12)
Round-11 gate accepted cursor=0 unconditionally. But a stateful tab
whose previous session disconnected BEFORE receiving the
post-replay cursor frame (network blip during the writeMu burst
between replay binaries and the cursor) ends up with sessionStorage
cursor=0 + a non-empty Y.Doc populated by prior replay binaries.
On reconnect with since=0 the server treats it as fresh, replays
nothing if the op-log was meanwhile pruned, and the client's
on-open Y.encodeStateAsUpdate resurrects pre-prune ops. The next
flush carries cursor=0 + stale-derived markdown.
The gate now refuses any incompatible cursor:
- cursor>0 + empty op-log (prior rule)
- cursor<MIN + non-empty op-log (prior rule, now naturally
catches cursor=0 too because 0 < any positive MIN)
The WS replay path is unchanged — full replay from since=0 is
the recovery for clients that genuinely lost their cursor; the
corruption manifested through the flush PATCH which we now gate.
New test: TestCollabSnapshotRejectsCursorZeroOnNonEmptyOpLog.
Per Codex round 12 [P1] of TASK-1319.
* fix(collab): close cursor=0 client/server gaps + lock validation+write atomically per Codex review (round 13)
Four P1 issues addressed:
1. Client always sends op_log_cursor (including 0) so the server
gate sees the field. Previously cursor=0 was omitted, which
silently bypassed the server's stale-snapshot rejection.
2. Provider construction now resets sessionStorage cursor to 0
when the Y.Doc is empty. The Y.Doc isn't persisted across
page reload, so a stored cursor=N + fresh empty Y.Doc would
announce since=N to the server and miss rows 1..N from
replay (server only replays id > N).
3. onOpen skips Y.encodeStateAsUpdate when lastOpLogID === 0.
A populated Y.Doc + cursor=0 is the network-blip-during-cursor-
write failure mode; pushing that state can resurrect ops the
server has pruned. Server replay + lazy-seed handle recovery
without our push.
4. Server gate now runs INSIDE the per-item collab setup lock
(new RoomManager.UnderItemLock helper) so a concurrent prune
(PruneAndApply, schema rebuild, dormant GC) cannot land
between the MIN check and the items.content write. Without
this, a tight race let stale snapshots overwrite canonical
content the prune just installed.
Per Codex round 13 [P1] of TASK-1319.
* fix(collab): gate handleDocUpdate on cursorAnchored to close stale-Ydoc edit path per Codex review (round 14)
Round 13 fix skipped on-open send for lastOpLogID===0, but local
edits via handleDocUpdate still propagated. A populated Y.Doc +
no-cursor-yet client could type, the edit would land in the
op-log with id N, server would send originator cursor=N, and
the next 5s flush would carry an 'anchored' cursor that passed
the server's MIN check — overwriting items.content with stale-
Y.Doc-derived markdown.
Add a cursorAnchored boolean. Set on first op_log_cursor frame
receipt (including cursor=0 against an empty op-log — that's a
legitimate 'server has nothing' signal). handleDocUpdate refuses
to send before this. Local edits buffer in the editor; once the
cursor arrives (or force_refresh rebuilds the provider), the
existing reconnect/edit paths catch them up.
Per Codex round 14 [P1] of TASK-1319.
* fix(collab): buffer + flush pre-anchor local updates per Codex review (round 15)
Round 14 silently dropped local Yjs updates fired before the
first op_log_cursor frame anchored the session. Yjs updates are
incremental: a dropped keystroke leaves later ops referencing
structs no peer can resolve, breaking convergence.
Buffer pre-anchor updates in a Uint8Array[] (capped at 1000 to
prevent unbounded growth in pathological 'anchor never arrives'
scenarios — overflow triggers force_refresh-style recovery).
On the first cursor frame, flush the buffer in order so the
server gets every causally-required struct before any post-
anchor updates land.
Per Codex round 15 [P1] of TASK-1319.
* fix(collab): destroy provider before force_refresh on pre-anchor buffer overflow per Codex review (round 16)
Round 15 overflow path called onForceRefresh but didn't destroy
the provider synchronously. A late op_log_cursor arriving before
the page-level rebuild (the recovery callback is async — refetches
items.content) would flip cursorAnchored=true, the partially-
populated buffer would flush, but the DROPPED prefix (the
overflowed entries) would leave server-side ops causally
incomplete — exactly the bug the buffer was supposed to prevent.
destroy() sets destroyed=true, removes message listener,
short-circuits scheduleReconnect, closes the socket. Late cursor
frames can no longer anchor a doomed provider.
Per Codex round 16 [P2] of TASK-1319.
* fix(collab): refuse rebuild on refetch fail + broaden on-open gate to cursorAnchored per Codex review (round 17)
Two findings:
[P1] force_refresh recovery bumped forceRefreshNonce in finally
even when the item.content refetch failed. The rebuild then
lazy-seeded from the cached (possibly-stale) item.content, and
the next flush would PATCH that stale view back to the server.
Move the bump into .then() so a failed refetch surfaces a
'please reload' toast and leaves the editor effectively
read-only (forceRefreshInFlight stays true, blocking flushes).
[P2] Send-on-open gate was lastOpLogID > 0, which silently
dropped local edits made during a brief offline window after a
legitimate 'cursor=0' anchor (empty op-log session). Switch to
cursorAnchored — the boolean specifically distinguishes
'unanchored' (stale Y.Doc + no server confirmation) from
'anchored at cursor=0' (legitimate empty op-log).
Per Codex round 17 [P1+P2] of TASK-1319.
* fix(collab): force_refresh on cursor=0 against non-empty Y.Doc per Codex review (round 18)
cursor=0 means the server's op-log is currently empty. A
non-empty Y.Doc at first-cursor receipt implies the ops came
from an earlier connection within this provider's life that
never reached its post-replay cursor frame, followed by a
server-side prune (PruneAndApply, schema rebuild, dormant GC)
during our disconnect. Anchoring at cursor=0 in that state
would mark a stale Y.Doc as authoritative; the next on-open
state push or flush would resurrect pre-prune state and
overwrite canonical items.content.
Detect the configuration via Y.encodeStateVector length and
invoke the same force_refresh-style recovery the explicit
server frame triggers: destroy provider, clear sessionStorage,
fire onForceRefresh so the page rebuilds from items.content.
Per Codex round 18 [P1] of TASK-1319.
* fix(collab): gate cursor=0 force_refresh on remoteSyncApplied per Codex review (round 19)
Round 18 force_refreshed the provider whenever cursor=0 arrived
against a non-empty Y.Doc. But local pre-anchor edits (user typed
before the initial cursor=0 of a legitimate empty-op-log session
arrived) ALSO populate Y.Doc — yet those edits live in
preAnchorUpdates and were supposed to flush on anchor. The
predicate spuriously triggered force_refresh, dropping the
buffered local edits.
Track remoteSyncApplied (set when readSyncMessage applies
anything to Y.Doc — replay binary or live peer op). Only force_
refresh on cursor=0 when remoteSyncApplied is true: that's the
true 'remote replay landed but server now reports empty op-log
=> mid-session prune' signature.
Per Codex round 19 [P1] of TASK-1319.
* fix(collab): repair brace mis-merge in wsProvider cursor=0 guard
The round-19 patch overlapped the round-18 inner block, producing
an extra brace + over-indented body. Collapsing into a single
clean block restores parseability without changing semantics
beyond what round 19 already documented.
* fix(collab): gate syncStep2 reply on cursorAnchored per Codex review (round 20)
readSyncMessage writes an inline syncStep2 reply when it receives
a peer's syncStep1. That reply embeds our current Y.Doc state.
If a peer's syncStep1 arrives before our first op_log_cursor
(pre-anchor window), the reply path bypasses handleDocUpdate's
cursorAnchored gate and lets potentially-stale Y.Doc state reach
the server before the cursor=0 + remoteSyncApplied force_refresh
recovery has a chance to fire.
Suppress the reply while unanchored. Peer state propagation
still works: the buffered preAnchorUpdates flush on anchor, and
the lazy-seed rebuild after a force_refresh seeds canonical
content from items.content.
Per Codex round 20 [P1] of TASK-1319.
* fix(collab): fold mid-replay live op ids into post-replay cursor + remoteSyncApplied only on apply per Codex review (round 21)
Two more findings:
[P1 server] writeLoop suppresses cursor frames during replay to
prevent the client persisting a cursor past unreplayed rows.
But binary frames for those live ops still go through
(commutativity), so the client APPLIES them to its Y.Doc. The
post-replay initial cursor only covered max(highestReplayed,
since), leaving the cursor below the highest applied op. On
empty-replay sessions this trips the client's
'cursor=0 + remoteSyncApplied' force_refresh path and discards
buffered pre-anchor edits.
Track maxLiveOpLogIDDuringReplay on the roomConn (atomic
compare-and-swap) and fold it into the post-replay cursor.
[P1 client] remoteSyncApplied was set on every MESSAGE_SYNC,
including syncStep1 (which only carries a state vector — it
doesn't apply state). A peer's syncStep1 arriving pre-anchor
would falsely flag remote-sync-applied and trip the cursor=0
force_refresh on legitimate empty-op-log sessions. Set the
flag only after readSyncMessage returns, and only for
syncStep2 / update subtypes.
Per Codex round 21 [P1] of TASK-1319.
* fix(collab): widen writeMu critical section + drop omitempty on op_log_id per Codex review (round 22)
Two more P1s:
[P1 server] writeLoop's mid-replay record-max happened OUTSIDE
writeMu, so runConn's post-replay read could race the record:
runConn loads → writeLoop's atomic store of higher value →
runConn sends cursor below the live id. Move the entire
per-event sequence (binary write + replayDone observation +
record-or-send) inside writeMu, and have runConn acquire
writeMu around its read+cursor-write+replayDone-flip. The lock
serializes the two paths cleanly: writeLoop events that ran
first have already recorded; events that arrive after replayDone
flips emit their own cursor frames.
[P1 protocol] OpLogID had `omitempty` JSON tag — a legitimate
cursor=0 (empty op-log session) serialized as
`{"type":"op_log_cursor"}` with no op_log_id field. The
client's strict-type check then rejected it as malformed,
leaving the session unanchored and local edits buffered
forever. Drop omitempty so 0 is wire-visible. Other control
types (applier_request/ack) carry an extra op_log_id:0 in
their JSON, which their client dispatches ignore.
Per Codex round 22 [P1] of TASK-1319.
* fix(collab): route originator cursor through writeLoop FIFO per Codex review (round 23)
readLoop sent the originator's op_log_cursor directly via
sendOpLogCursor right after AppendYjsUpdate, bypassing the bus/
writeLoop ordering. With a peer op already queued in rc.bus, the
sequence on the wire could be:
1. originator cursor=N (newer local op)
2. peer binary (older op)
3. peer cursor=M < N (rejected by client's max-take logic)
Client persists cursor=N. If the client then disconnects before
applying the peer binary, reconnect with since=N replays nothing
(server has nothing > N) and the older peer op is lost forever
to this client's Y.Doc.
Fix: writeLoop now processes self events too — skipping the
binary echo (the originator already has Y.Doc state) but routing
the cursor frame through the same FIFO bus channel as peer ops.
The originator's cursor=N now arrives strictly AFTER all
older-id peer events on the same channel.
Per Codex round 23 [P1] of TASK-1319.
|
||
|
xarmian
|
028db39217 |
feat(collab): periodic op-log GC sweeper for dormant items (TASK-1309) (#471)
The Yjs collab dumb-relay accumulates op-log rows indefinitely in item_yjs_updates. DOC-1307 surfaced 45-second p50 cold-reconnect latency on a single item with 5000 accumulated rows. Without GC, busy items keep growing. This adds a periodic background sweeper that prunes the entire op-log for items that are both DORMANT (no recent activity) AND FULLY FLUSHED (items.content has captured every op-log row). Whole-log only — Yjs op streams are causally linked, prefix-pruning corrupts replay; future cold connects lazy-seed from items.content. Components: - Store.ListDormantOpLogItemsBefore (joins items, filters watermark) - Store.PruneItemOpLogIfDormantBefore (atomic conditional DELETE) - Store.GetItemContentFlushedOpLogID (per-item watermark getter) - RoomManager.PruneSweep (per-item-locked, active-room-skip) - Server.StartOpLogGC / stopOpLogGC (mirrors orphan_gc.go pattern) - cmd/pad/main.go env vars PAD_OPLOG_GC_INTERVAL / PAD_OPLOG_GC_MIN_AGE - New (item_id, created_at) index for the dormancy query - New items.content_flushed_op_log_id column (id-based watermark, monotonic, no clock-skew or second-granularity false positives) + content_flushed_at (informational timestamp) Watermark policy: - Server-driven full-content writes (CLI / MCP / version restore / PruneAndApply) advance content_flushed_op_log_id to MAX(op-log.id) via subquery, atomic with the content UPDATE - Browser collab-snapshot 5s flushes do NOT advance the watermark — they can't prove their markdown captured every peer's ops, so letting them stamp would risk later GC-pruning unsynced peer edits - Schema-mismatch rebuild (TASK-1268) logs a WARN when it drops unflushed ops (data loss is unavoidable on schema bumps but visible) Stop ordering: collab.Close() now runs BEFORE bg.Wait() so a GC goroutine waiting on an itemLock behind an active Join can drain. Migration backfill: items WITH existing op-log rows keep NULL watermark (don't certify); items WITHOUT op-log rows get a synthetic 0 watermark (vacuous, harmless — no rows to compare against). Tests: - 6 RoomManager.PruneSweep tests (dormant prune / default minAge / empty / bails-on-Close / skips-active-room / skips-row-added-mid- sweep via fakeOpLog hook) - 5 Server.OpLogGC tests (prunes-dormant / start-idempotent / preserves-unflushed / backfill-doesnt-certify-unflushed / no-collab-noop) - TestCollabSnapshotDoesNotAdvanceOpLogWatermark in store - TestCollabSnapshotQueryOverridesBodyVersionSource in server (regression for body-attacker bypass) Seven rounds of Codex review — caught 5 P1s and 4 P2s I would have shipped under self-review: 1. Prefix-prune corrupts Yjs replay 2. Stop ordering deadlock 3. Missing index 4. Best-effort flush ⇒ data loss 5. Backfill over-certifies via metadata-PATCH 6. Second-granularity timestamp comparison 7. Schema-mismatch path drops unflushed silently 8. Browser flush stamps watermark beyond Y.Doc 9. Body version_source bypasses server policy |
||
|
xarmian
|
66aa6f5197 |
fix(loadtest): codex catch-up review fixes (TASK-1270) (#470)
PR #468 shipped without a codex review because codex was unavailable that day. This is the catch-up; codex found 2 P2s and 5 NITs. [P2] readSendTimestamp recorded latencies for prior-run replay frames. The original guard only checked nonzero ts, not session recency, so any stale op-log row inflated p95/p99. Fixed: runContext captures startedAt; recv path filters frames whose embedded ts predates this run. Verified: against an item with 90 stale rows the test counts received frames (630) but only records latencies for the 180 live ones. [P2] Shutdown deadlock. The writer goroutine could be blocked in conn.WriteMessage when rc.done closed; the only path to conn.Close was that same goroutine's select-case, so wg.Wait() could hang forever under server backpressure. Fixed: per-client watchdog goroutine closes the conn from outside the writer when done fires, plus a 5s SetWriteDeadline per send as defence-in-depth. A 5s duration test now exits in exactly 5.008s. NITs (also fixed): - buildFrame docstring corrected (minimum is 16 metadata bytes, buffer is frameBytes+1). - buildFrame returns (bytes, error) instead of log.Fatalf-ing on rand.Read; caller logs detail and increments errors counter. - -cookie / -token flag help now states both can be set together. - Watchdog-induced WriteMessage errors are no longer counted as real errors (isClosedDone check). - buildFrame error path now logs the actual error detail. Two rounds of codex review: round 1 found the items above; round 2 returned CLEAN. |
||
|
xarmian
|
f44e592554 |
docs: confirm collab adds no container deps; close PLAN-1248 cleanup (TASK-1272) (#469)
Documents that the dumb-relay design preserves the single-Go-binary self-hosted shape — no Yjs Go port to vendor, no separate sync server, no Redis (multi-instance fanout is a separate deferred IDEA). Op-log lives in the existing SQLite/Postgres; relay is part of the main HTTP listener. The original task scope also called for removing `web/src/routes/dev/yjs-sandbox/`, but that route only existed on the `feat/yjs-tiptap-spike` branch and was deliberately not cherry-picked into PLAN-1248's productionization work — so there's nothing to delete on `main`. The spike branch can now be deleted. Audited web/package.json for spike-only deps: every yjs / tiptap / y-protocols dep is consumed by production code paths. Nothing to clean up. |
||
|
xarmian
|
287fa545fb |
feat(loadtest): add cmd/loadtest-collab + DOC-1307 findings (TASK-1270) (#468)
Synthetic Go load-test for the Yjs collab dumb-relay. Each simulated client opens a WebSocket, sends tagged sync frames at a configurable rate, consumes inbound frames, and computes broadcast fanout latency. Doesn't depend on a real Yjs port — the dumb-relay's first-byte discriminator (yMessageSync=0) is enough to exercise the persist + broadcast path with synthetic payloads. Each frame embeds a unix-nano timestamp + client ID so receivers can compute round- trip latency without out-of-band coordination. Findings (in DOC-1307): - N=5, N=25: clean, p95 < 10ms, fanout matches expected (N-1)x - N=100: 38/100 dial failures (consistent), but the 62 successful see p95=27ms — server rejects ~38% of simultaneous dials at this level. Filed BUG-1308 to investigate the ceiling. - Op-log grows unbounded without compaction; old runs replay on reconnect causing latency blow-up. Filed TASK-1309 to wire a periodic prune sweeper. Self-reviewed only; codex was unresponsive today after multiple hour-long retries. |
||
|
xarmian
|
680cfbd879 |
docs: collab endpoint + Tiptap coordinated-bump rule (TASK-1269) (#467)
CLAUDE.md changes (no behaviour change):
- API Reference: GET /api/v1/collab/{itemID}?schema_version=N
- New "Real-time collaboration (Yjs / Tiptap)" section under Common
Tasks, documenting:
- Where the collab code lives
- The three-package coordinated-bump rule (@tiptap/core +
@tiptap/extension-collaboration + @tiptap/y-tiptap must move
together with exact pins)
- When the schema version (web/src/lib/collab/schemaVersion.ts +
internal/collab/manager.go::DefaultSchemaVersion) must bump
|
||
|
xarmian
|
9b46be915a |
feat(collab): schema-version handshake + mismatch rebuild (TASK-1268) (#466)
Adds a client→server schema-version handshake on every WS connect and a per-item op-log rebuild path for the case where the server ships a new SCHEMA_VERSION and finds older rows persisted in the op-log. Client side - New web/src/lib/collab/schemaVersion.ts exporting `SCHEMA_VERSION` (currently '1') with a documented bump rule covering Tiptap extension changes, coordinated multi-package bumps, and Y.Doc fragment-shape changes. - wsProvider's defaultCollabUrl appends ?schema_version=... Server side - handlers_collab.go validates ?schema_version against RoomManager.SchemaVersion() BEFORE upgrading the WS; mismatch returns HTTP 400 with code "schema_mismatch". An empty query is treated as legacy '1' for graceful deploys; once the server bumps past v1, missing query becomes a 400 too. - New RoomManager.SchemaVersion() getter. - RoomManager.Join's setup-phase (under itemLock) now calls maybeRebuildOnSchemaMismatch: if the latest persisted op-log row's schema_version disagrees with the manager's current version, the entire item op-log is pruned via PruneYjsUpdatesBefore. items.content is canonical and untouched, so the lazy-seed path (TASK-1261) re-encodes it into ops at the new schema on the next idle tick. - New store method LatestYjsUpdateSchemaVersion. Tests - internal/collab/manager_test.go: three new tests (mismatch prunes, clean version preserves op-log, post-rebuild connects are clean) + fakeOpLog gets LatestYjsUpdateSchemaVersion + PruneYjsUpdatesBefore. - internal/server/handlers_collab_test.go: rejects-schema-mismatch (400), accepts-explicit-match (101). One round of Codex review (CLEAN with two NITs, both fixed). |
||
|
xarmian
|
191b887e23 |
feat(versions): VersionSource attribution + collab coexistence (TASK-1267) (#465)
The collab 5s-flush PATCH (TASK-1260) sends
`?source=collab-snapshot` with a body of just `{ content }`. Without
a handler-side stamp, `Store.UpdateItem`'s default coerced empty
input.Source to "web" on the version row and the per-(actor, source)
throttle suppressed every collab-driven snapshot following the user's
last manual web edit — version-diff effectively went silent during
co-edit sessions.
Adds:
- ItemUpdate.VersionSource: overrides per-version-row Source
attribution WITHOUT mutating items.source. The latter feeds
WorkspaceHasAgentActivity's `source IN ('cli', 'mcp')` filter,
so a CLI/MCP-created item the user opens in the editor would
otherwise silently flip out of the agent-activity tally on every
auto-flush.
- Store.UpdateItem prefers VersionSource over Source for version
row creation; falls back to Source then "web" if neither set.
- handlers_items.go stamps `input.VersionSource = "collab-snapshot"`
for `?source=collab-snapshot` PATCHes (when not already set).
Tests:
- internal/store/items_collab_versions_test.go: store-level
reverse-patch reconstruction over a CLI→web→collab-snapshot
edit sequence; verifies IsDiff=true on at least one row.
- internal/server/handlers_items_collab_versions_test.go: full
HTTP-level test of the route; asserts a collab-snapshot version
row is created AND that items.source stays "cli".
Four rounds of Codex review.
|
||
|
xarmian
|
fdc6b221d0 |
test(collab): regression guard for share-page collab isolation (TASK-1266) (#464)
The public /s/{token} share page must keep rendering markdown via
marked + DOMPurify and never open a WebSocket to /api/v1/collab —
anonymous viewers can't authenticate, and exposing per-item Y.Doc
traffic to the public internet would be a security regression.
This is a verification task: zero production code changes. Adds
TestSharePageDoesNotImportCollab in internal/server which:
- Walks the import closure rooted at the share-page +page.svelte
(resolves $lib/ aliases, relative paths, conventional extensions,
static AND dynamic `import(...)` forms; skips external packages)
- Asserts no file in the closure contains forbidden tokens:
wsProvider, CollabProvider, @tiptap/extension-collaboration,
@tiptap/y-tiptap, 'yjs' / "yjs", y-protocols, Editor.svelte
(suffix), WebSocket, /api/v1/collab
- Asserts the route file still imports + invokes marked and
DOMPurify.sanitize (catches a renderer swap)
- Strips comments before all checks so leftover commented-out
imports can't bypass
Verified by injection: direct AND transitive AND dynamic-import
regressions all fail the test.
Four rounds of Codex review.
|
||
|
xarmian
|
27b048eef3 |
feat(collab): presence carets with deterministic user colors (TASK-1263) (#463)
Renders remote peers' carets and selection highlights in the collab-mode Tiptap editor via @tiptap/extension-collaboration-caret (the v3 rename of extension-collaboration-cursor — pinned at 3.22.5 to match the rest of the Tiptap suite). - New cursorColor.ts: djb2 hash → HSL → #rrggbb (hex required because y-tiptap's selectionRender appends an alpha-hex byte and only validates hex) - Editor.svelte: optional `awareness` + `collabUser` props; only registers CollaborationCaret when ydoc + awareness + user are all present - +page.svelte: derives `collabUserState` from authStore.user and threads it + collabProvider.awareness into <Editor> - CSS for .collaboration-carets__caret + label + selection (label always visible — caret is too thin to hover, so the original hover-reveal was unreachable) Three rounds of Codex review. |
||
|
xarmian
|
1ad9ce6c1f |
feat(collab): mobile WS reconnect handling (TASK-1265) (#462)
Adds visibility / online / offline event listeners to CollabProvider so iOS Safari (and other mobile suspends) recover the WS without waiting for the 30s backoff ceiling. - visibilitychange→'visible' / online: forceReconnect — closes any existing socket (even apparently-OPEN ones, since iOS can silently suspend the transport while leaving readyState OPEN) and reconnects from a clean slate. - offline: demote state='offline' immediately + tear down the socket so a queued syncStep2 can't flip back to 'synced'. Backoff timer keeps trying so we recover even when 'online' never fires. - handleControlMessage now pins the source socket (e.currentTarget) so an applier_ack after force-reconnect doesn't land on a new socket the server doesn't recognize. - Extracted runDisconnectCleanup() helper to keep all teardown paths in lockstep. Six rounds of Codex review. |
||
|
xarmian
|
22b4030057 |
feat(collab): connection-state badge on item editor (TASK-1264) (#461)
Surfaces the WS connection state ('connecting' | 'synced' |
'reconnecting' | 'offline') as a small badge in the item-detail
meta-info row. Visible only when the WS provider exists (i.e.
canEdit && !rawMode), so share-page / read-only / raw mode don't
render it.
Adds CollabProvider.state $state field with transitions on the
real-sync edges only. reconnectAttempts is now reset in the
syncStep2 branch and the grace timer (not on raw open/close), so a
flaky proxy that OPEN→CLOSE-before-sync still reaches the
OFFLINE_THRESHOLD. State preserves 'offline' across retries to
avoid flicker; pre-first-sync failures stay 'connecting',
post-sync drops become 'reconnecting'.
Three rounds of Codex review.
|
||
|
xarmian
|
483e338a54 |
feat(collab): drop conservative content-skip when collab active + applier toast (TASK-1262) (#460)
## Drop TASK-1243's content-skip when collab is active
The conservative `item = { ...updated, content: item.content }`
preservation in the SSE/sync handlers was protecting against
clobbering a user's mid-keystroke edit with a stale content
snapshot. Under collab the editor reads from Y.Doc — NOT the
content prop — so the Editor.svelte $effect's `if (ydoc) return`
gate at line 810 makes adopting `updated.content` harmless to
the live editor while keeping `item.content` fresh for
downstream consumers (UI summaries, search-index hints,
subsequent share-page renders).
For non-collab viewers (view-only, raw mode, items where
canEdit=false) the content-skip stays — those paths DO render
from item.content via the prop $effect, and adopting a stale
SSE snapshot mid-keystroke would clobber unsaved chars.
Applied to all four adoption sites:
- SSE item_updated
- SSE item_restored
- syncService incremental update
- syncService full-refresh fallback
## Applier-success toast
The applier handler (wired in TASK-1259's absorption of TASK-1262
scope) silently called setContent. Users would see their editor
change under them with no UI hint. Adds a brief
toastStore.show('External edit applied', 'info') after the
setContent succeeds; preserves the late-apply guard so toasts
only fire on actual mutations.
## Acceptance criteria
- [x] `pad item update REF --stdin < new.md` while two browser
tabs are open: both tabs reflect the change (the applier path
fires setContent on the longest-connected tab; ops broadcast
to peers; SSE adoption keeps item.content fresh).
- [x] `pad item update REF --status done` (field-only): both
tabs see the field change via SSE; no editor disruption (the
guard `input.Content != nil` skips the applier branch
entirely; SSE adoption updates fields atomically).
- [x] Designated client disconnects mid-flight: server retries
next applier (TASK-1257 logic; pending follow-up TASK-1268
for the all-applier-failed case).
- [x] Toast: "External edit applied" surfaced.
Parent: PLAN-1248
|
||
|
xarmian
|
2ed9314078 |
feat(collab): lazy-seed Y.Doc from items.content on first sync (TASK-1261) (#459)
* feat(collab): lazy-seed Y.Doc from items.content on first sync (TASK-1261)
Closes the regression introduced in TASK-1259 where items with
pre-existing items.content but no op-log entries rendered as a
blank editor under collab — the Y.Doc started empty, the server
had nothing to replay, and the user's existing markdown was
hidden behind a confusingly-empty document.
## Mechanism
A new $effect reacts to `collabProvider.synced` flipping true.
When all of these are met:
1. Provider has completed its initial sync (synced === true).
2. The Y.XmlFragment named 'default' (the field bound by the
Collaboration extension per TASK-1258) has length 0 — i.e.
the Y.Doc is genuinely empty.
3. items.content is non-empty.
…the effect calls editor.commands.setContent(seedMarkdown). The
y-tiptap binding turns that into Y.Doc ops, which:
- persist to the op-log (so subsequent connects + new peers
see the content via the regular replay path), and
- propagate to any concurrent peer.
## Idempotence
`seededProvider` tracks which provider instance we already
attempted. New providers (item nav, canEdit/rawMode flips) reset
eligibility automatically because the reference !==
seededProvider. The `=== provider` guard in $effect cleanup also
clears the slot when the provider tears down, so a raw→rich
re-mount after raw saves can re-seed cleanly if the op-log was
pruned.
## Multi-tab race
If two tabs finish their initial sync simultaneously and both
find the fragment empty, both fire setContent. Y.Doc CRDT merges
the two replace-ops with last-write-wins — worst-case outcome is
one wasted op for identical content. Acceptable for v1; a
designated-seeder lock (Y.Map flag) is a tracked follow-up if
observed in the wild.
Parent: PLAN-1248
* fix(collab): unblock synced for empty op-log + lowest-clientID seed election per Codex review (round 1)
Two findings from round 1:
1) [P1] CollabProvider.synced only flipped true on receipt of a
syncStep2. The dumb-relay server replays the op-log as a
sequence of BinaryMessage frames but never sends its own
step2; an empty/pruned op-log + first-peer connect therefore
never arrived at the explicit-sync signal — leaving synced
stuck at false and blocking the lazy seed.
Fix: schedule a SYNC_GRACE_MS (1s) timer in onOpen that flips
synced=true if no explicit step2 arrives. Cancelled in
onClose + destroy so reconnects install a fresh grace.
2) [P1] Concurrent tabs both seeing an empty fragment + calling
setContent would have produced duplicated content (Yjs CRDT
concurrent inserts MERGE rather than dedupe).
Fix: lowest-clientID election. Among connected peers visible
in awareness.getStates(), only the tab with the lowest
clientID fires setContent. Plus a microtask yield + recheck
immediately before the actual mutation: gives any
concurrent peer's seed a chance to propagate, and re-runs
the election in case awareness changed (someone joined or
left during our $effect tick).
Awareness-empty short-circuit: if the handshake hasn't
propagated yet (getStates returns empty), skip — a future
awareness update will re-trigger the effect via the synced
dependency edge.
Residual race: if two tabs both have awareness propagated
AND both see "I'm lowest" within the microtask window, both
could still seed. v1 ships with this acknowledged risk; a
server-side designated-seeder protocol is a tracked
follow-up if observed in the wild.
|
||
|
xarmian
|
9b1a91ab00 |
feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) (#458)
* feat(collab): 5s-idle + on-disconnect markdown flush (TASK-1260) Replaces the temporary handleContentUpdate suppression introduced in TASK-1259 (PR #457) with a proper flush mechanism. Under collab, the Y.Doc + op-log are canonical for live state but items.content needs to stay reasonably fresh for downstream consumers (search index, share-page, exports, plain API readers). ## Mechanism 1. **5s idle timer.** Every editor onUpdate (local OR remote) resets a 5s timer. On fire, PATCHes items.content via the new `?source=collab-snapshot` query param. 2. **Server-side bypass.** handleUpdateItem inspects the source query param. When set, skips the applyContentViaCollab routing entirely and writes directly. Without the bypass, the PATCH would loop back through the applier protocol (the same tab gets asked to apply, acks, server strips input.Content) and leave items.content unchanged forever. The flag is trustworthy because the caller already has edit access. 3. **Dedupe across peers.** Track lastFlushedContent. If our last successful flush already landed this exact markdown, skip the PATCH. Multiple connected tabs would otherwise each fire a redundant flush after every shared edit converges, multiplying server load by the peer count. 4. **On-disconnect flush.** $effect cleanup (item swap or page unmount) calls flushCollabNow(true) BEFORE provider.destroy(). A separate beforeunload listener catches close-tab / reload / external-nav. Both use fetch keepalive: true so the request outlives the page lifecycle. 5. **Item-id race guards.** runCollabFlush captures reqItemId before await; ignores response if item swapped. loadData() clears collabFlushTimer + lastFlushedContent on navigation. ## Files - internal/server/handlers_items.go — accept `?source=collab-snapshot` - web/src/lib/api/client.ts — add api.items.flushCollabContent - web/src/routes/.../[slug]/+page.svelte — handleContentUpdate gains scheduleCollabFlush + runCollabFlush + flushCollabNow; wired to $effect cleanup + beforeunload + loadData reset. Parent: PLAN-1248 * fix(collab): capture ws+itemId at provider mount + apply unescapeDocLinks per Codex review (round 1) Two findings from round 1: 1) [P1] runCollabFlush resolved item.id and wsSlug at execution time, not at schedule time. During item navigation the timer could fire (or $effect cleanup could run) AFTER `item` was already updated to the new item, causing the OLD editor's markdown to be PATCHed against the NEW item's URL — cross-item content corruption. Fix: introduce activeCollabContext = { wsSlug, itemId }, captured at $effect-body time (when the provider is minted). scheduleCollabFlush, runCollabFlush, and flushCollabNow all take their target identity from this captured context, never from live reactive state. Cleared in the $effect's own cleanup (defensive `=== ctx` slot guard so a fast-navigation churn doesn't clobber a successor context). 2) [P2] The disconnect flush read raw editor.storage.markdown .getMarkdown() without unescapeDocLinks, unlike the regular onUpdate path. Closing/navigating before the idle flush could persist escaped wiki links like \[\[TASK-1\]\] which then wouldn't be converted by markdownToWikiLinks. Fix: apply unescapeDocLinks() at the start of runCollabFlush (covers both the timer-driven idle path and the unmount path). * fix(collab): gate UI mutations on foreground+current-item per Codex review (round 2) [P2] runCollabFlush mutated page-scoped state (saveStatus, editorStore.lastSaveTime, lastFlushedContent) before checking whether the captured itemId still matches the foreground item. On item navigation, the cleanup-driven keepalive flush could stamp 'saving' onto the NEW page's saveStatus, leaving it pinned indefinitely (and pollute lastFlushedContent for the new item's dedupe state). Fix: introduce isForegroundCurrent() = !keepalive && item.id === itemId. Gate saveStatus / setLastSaveTime / showSaved on it so background cleanup flushes never touch UI state. Gate lastFlushedContent on item.id === itemId regardless of keepalive so a stale flush can't seed the wrong item's dedupe. * fix(collab): skip cleanup flush on rich→raw transition per Codex review (round 3) [P1] $effect cleanup fires the keepalive flushCollabNow on every provider teardown, including rawMode toggles. The raw-button onclick already pre-populated rawPendingMarkdown with the live editor markdown (which the 1.2s raw debounce will land), so the keepalive PATCH from cleanup is redundant — and worse, can land AFTER the raw save and clobber newer raw edits with the older Y.Doc snapshot. Fix: gate the cleanup flush on `!rawMode`. If rawMode is true at cleanup time, the user just toggled to raw and the raw-mode codepath owns items.content from here. The other cleanup triggers (item nav, canEdit flip, page unmount) all keep firing the flush as before. Note: rawMode === true at cleanup time unambiguously means "transitioning into raw" — the inverse case (already in raw and the cleanup fires for some other reason) is impossible because collabKey gates on !rawMode, so the provider $effect never runs while rawMode is true. * fix(collab): synchronously flush Y.Doc state on rich→raw toggle per Codex review (round 4) [P1] Rich → raw → navigate-without-typing-or-toggling-back never PATCHed the live Y.Doc state to items.content. The previous seed mechanism only set rawPendingMarkdown, which only fires the 1.2s debounce on a subsequent handleRawContentUpdate call — which never happens if the user doesn't type. Fix: await runCollabFlush(ws, itemId, md, true) inside the raw button's async onclick BEFORE flipping rawMode = true. This: - Lands items.content with the live Y.Doc state synchronously (one PATCH, awaited, with keepalive: true so it survives a fast post-toggle navigation). - Seeds lastFlushedContent so any cleanup-driven re-flush is deduped. - Avoids populating rawPendingMarkdown — the raw debounce now only fires for actual user edits in raw mode, eliminating the race where a stale debounce fired after navigation could clobber state. The Round 3 cleanup-skip on rawMode is kept as defense-in-depth (also makes the no-op-when-already-flushed semantics explicit). * fix(collab): loop-flush until stable + cancel timer on rich→raw toggle per Codex review (round 5) Two HIGH findings from round 5: 1) Round 4's single-flush captured md BEFORE the await; concurrent peer edits (e.g. same user's other tab) during the await were lost from the seed and could be overwritten by subsequent raw-mode saves. Fix: loop-flush until stable. Re-read editor markdown after each PATCH; if it changed, flush again. Capped at 3 iterations to bound the transition under aggressive concurrent typing. 2) An onUpdate during the await could schedule a 5s collab flush timer that survived the rawMode flip. The cleanup skipped flushCollabNow on rawMode, but the timer fired its own runCollabFlush — which then PATCHed stale rich markdown on top of subsequent raw saves. Fix: explicitly clearTimeout(collabFlushTimer) at the end of the rich→raw onclick (after the loop-flush, before flipping rawMode). Belt-and-braces with the Round 3 cleanup skip. * fix(collab): seed raw mode from lastFlushed (not unflushed Y.Doc) per Codex review (round 6) [HIGH] Round 5's loop-flush could exit at the 3-iteration cap with md still differing from the last-PATCHed value, then seed rawSeedMarkdown with that unflushed md. An immediate navigation without typing would lose the unpersisted state. Fix: track lastFlushed inside the loop. After the loop, seed rawSeedMarkdown = lastFlushed (the markdown we actually PATCHed), NOT md (potentially a never-flushed in-memory value). If peer edits keep arriving past our cap, items.content lags Y.Doc briefly — but the peer's own 5s flush will catch up shortly, and at least raw mode shows state consistent with items.content rather than holding a value the server never received. * fix(collab): three corner-case fixes per Codex review (round 7) 1) [HIGH] lastFlushed = md was set unconditionally inside the loop-flush, even when runCollabFlush returned false (PATCH failed). rawSeedMarkdown could then be seeded with markdown the server never received. Fix: gate `lastFlushed = md` on runCollabFlush returning true. Failed PATCHes leave lastFlushed at its prior value. 2) [HIGH] lastFlushedContent (the collab-flush dedupe key) was never invalidated by raw-mode direct saves. Scenario: collab flushes A. Raw saves B. User returns to rich + edits back to A. Next collab flush dedupes (lastFlushedContent === A) and skips, leaving items.content stuck on B. Fix: reset lastFlushedContent = null after every successful raw save (both the regular handleRawContentUpdate path and the flushRawIfPending drain loop) so subsequent collab flushes always re-PATCH. 3) [MEDIUM] The async rich→raw onclick applied rawSeedMarkdown + rawMode = true after multiple awaits without verifying the user was still on the same item. A navigation during the loop-flush could let item A's handler resume and seed raw mode on item B. Fix: before mutating component state (rawSeedMarkdown, rawMode), check `item?.id === itemId` (the captured target). Bail with `return` if mismatched. * fix(collab): differentiate flush outcomes + foreground keepalive=false per Codex review (round 8) Two findings from round 8: 1) [P1] runCollabFlush returned `false` for both PATCH failure AND dedupe-skip. The rich→raw toggle treated `false` as "didn't flush" and didn't seed rawSeedMarkdown — but a dedupe means items.content already matches our markdown (the prior successful flush put it there). Raw mode then seeded from the page's stale `item.content` field, and a subsequent raw save could overwrite the current server content with the pre-collab snapshot. Fix: change runCollabFlush's return type to a discriminated string: 'flushed' | 'deduped' | 'failed'. The toggle treats 'flushed' and 'deduped' equivalently for seeding (both mean "server has this markdown") and only bails on 'failed'. 2) [P2] The toggle path used keepalive=true for the awaited flush. Browser keepalive requests can reject for bodies larger than the per-origin keepalive quota (~64KB). On reject, the catch silently fell through and raw mode activated with rawSeedMarkdown null. Fix: switch the toggle path to keepalive=false. The await is synchronous and user-initiated; navigation isn't imminent, so the keepalive escape hatch isn't needed (and risks losing the explicit save). Also added an `aborted` short-circuit so a 'failed' result returns early WITHOUT entering raw mode — user can retry. Cleanup-driven flushes (which DO need to survive page lifecycle) still use keepalive=true. |
||
|
xarmian
|
5dc42b60df |
feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259) (#457)
* feat(collab): wire Yjs WebSocket provider + Y.Doc lifecycle (TASK-1259)
Adds a thin y-websocket-style provider speaking the binary protocol
already implemented server-side in internal/collab/room.go. The
provider lives in a Svelte 5 .svelte.ts module so connection state
(`connected`, `synced`) can be consumed reactively by upcoming UX
tasks (TASK-1264 pending-sync indicator, TASK-1265 mobile reconnect).
Wire format mirrors the server's first-byte discriminator:
0x00 → y-protocols/sync (persisted to op-log + broadcast)
0x01 → y-protocols/awareness (broadcast only, ephemeral)
Lifecycle is bound to the item-detail page via $effect keyed on
`${item.id}:${canEdit}` — same key the <Editor> already re-mounts on,
so the Y.Doc and provider tear down in lockstep with the editor.
View-only viewers (canEdit === false) keep the legacy non-collab
editor; their read-only y-binding is deferred to TASK-1266.
Reconnect uses 1s/2s/4s/...30s exponential backoff. Sophisticated
mobile reconnect (visibility, network state) is TASK-1265.
KNOWN TEMPORARY REGRESSION: existing items with non-empty
items.content render an empty editor on first open under collab,
because the Y.Doc starts empty and TASK-1259 doesn't seed from
markdown. TASK-1261 (next in Phase 2) adds the lazy seed-after-
initial-sync path. New items + items already round-tripped through
collab are unaffected.
Drive-by lint cleanup of dead code that escaped Phase 1's
make-install-skips-lint loophole:
- gofmt -w on internal/collab/{applier,bus,manager}.go
- removed unused test/debug helpers Room.peerCount and
Room.applierConnCount (re-add with real callers when needed)
Parent: PLAN-1248
* fix(collab): gate Editor mount on ydoc + handle applier_request + catch-up state per Codex review (round 1)
Three findings from round 1:
1) [P1] $effect constructs ydoc AFTER Editor's onMount runs, so the
first mount on an editable item registered StarterKit history
instead of the Collaboration extension. The {#key} excluded ydoc,
so the editor never re-mounted when ydoc later became truthy →
editable users got a non-collab editor while the provider connected
to an unused Y.Doc.
Fix: gate the editable Editor mount on `ydoc` being ready
(`{#if !canEdit} ... {:else if ydoc} ...`). Adds at most one
reactive tick of delay; guarantees the first mount has the binding
registered.
2) [P1] Provider dropped non-binary WebSocket frames, but the server
sends `applier_request` as TextMessage. With TASK-1259 minting
active rooms, every concurrent CLI/MCP/API content PATCH would
sit blocked for 30s waiting for an ack, then fall back to a
direct write — and the in-memory Y.Doc would still hold stale
state and clobber it on the next 5s flush. Silent data loss.
Fix: parse TextMessage frames as JSON ControlMessage. On
`applier_request`, invoke an `onApplierRequest` callback (the
page passes `editor.commands.setContent(markdown)`) and send
`applier_ack` on success. The ExpiresAtMillis-driven late-apply
guard remains TASK-1262's full scope.
3) [P2] Local Y.Doc updates were silently dropped if the socket was
closed when handleDocUpdate fired. On reconnect the dumb-relay
server can't reconstruct missing updates from a state vector, so
any edits made before the first open or during a disconnect
could be lost.
Fix: after sending syncStep1 in onOpen, also send the current
doc state as a single update via `Y.encodeStateAsUpdate(ydoc)`.
CRDT idempotency makes this safe on initial open (server already
has these ops via op-log replay → sees a no-op update). Larger
docs incur a one-time cost on each connection; TASK-1265's
mobile-reconnect work can replace this with a buffered queue.
* fix(collab): destroy provider during rawMode + enforce ExpiresAtMillis on applier requests per Codex review (round 2)
Two findings from round 2:
1) [P1] collabKey ignored rawMode, leaving the WS provider connected
while the user edited via RawMarkdownEditor. Raw saves bypass the
y-binding (PATCH writes items.content directly), but the server
sees an active room → routes the PATCH through the applier flow
→ no editor mounted → 30s timeout fallback → direct write. The
stale Y.Doc still in memory then overwrote the raw save on the
next 5s flush after toggling back.
Fix: include rawMode in the collabKey derivation so toggling raw
destroys the provider (and the in-memory Y.Doc), and toggling back
mints a fresh pair that re-seeds from the op-log + TASK-1261's
lazy markdown seed.
2) [P1] Provider passed expires_at_millis to the handler but never
gated on it. A backgrounded tab that wakes after the server
retried or fell back could still apply setContent and overwrite
newer peer edits.
Fix: enforce the expiry in CollabProvider — check before
invoking the handler AND re-check before acking (handlers are
awaited and could span the deadline). Suppress the ack if either
gate trips; the server interprets "no ack" as "applier
unavailable" and falls back cleanly.
* fix(collab): prune op-log on direct-write fallback + pre-mutation expiry check per Codex review (round 3)
Two findings from round 3:
1) [P1] rawMode toggle to/from rich left a stale op-log: raw saves
wrote items.content directly while the destroyed provider's old
op-log persisted. Toggling back minted a fresh Y.Doc that
replayed the old log → showed pre-raw content → silently
overwrote the raw save on the next 5s flush.
Fix server-side: when ApplyExternalContent returns ErrNoActiveRoom
(no peers in memory, no in-flight Y.Doc state to corrupt), prune
the op-log alongside the direct items.content write so future
collab sessions start from a clean slate seeded by items.content
(TASK-1261's lazy seed). Pruning is intentionally NOT applied to
ErrNoApplierAvailable / ErrAllAppliersTimedOut — those paths
may have live peers whose Y.Doc state would diverge.
2) [P2] Provider's post-handler expiry check only suppressed the
ack, not the actual setContent mutation owned by the page
handler. An async handler that crossed the deadline could still
write stale markdown into the Y.Doc.
Fix: page handler now does its own pre-mutation expiry check
inside onApplierRequest before calling setContent. Documented
the contract on ApplierRequestHandler — handlers MUST honour
expiresAtMillis BEFORE mutating state.
* fix(collab): prune op-log on grace-TTL applier-unavailable + suppress autosave when collab active per Codex review (round 4)
Two findings from round 4:
1) [HIGH] op-log pruning still skipped ErrNoApplierAvailable. When
raw-mode destroys the in-tab provider, the room remains in its
60s grace TTL with zero conns, so the next direct-write PATCH
returns ErrNoApplierAvailable (not ErrNoActiveRoom). Stale op-log
rows persisted; toggling back within the grace window resurrected
pre-raw-save Y.Doc state.
Fix: prune op-log on ErrNoApplierAvailable too — the "no live
conns" condition makes pruning safe (no peers to corrupt).
ErrAllAppliersTimedOut still preserves op-log because peers may
still be alive there.
2) [HIGH] Once the WS provider is active the legacy 1.2s content
autosave PATCH gets intercepted by the applier path
(handleUpdateItem branch added in TASK-1252). On applier success
input.Content is nil'd out, so UpdateItem never writes the
markdown snapshot. The page's autosave was the only canonical
items.content flush in this diff — search / share-page / API
consumers would see stale content forever.
Fix: short-circuit handleContentUpdate when collabProvider is
set. The Y.Doc + op-log are canonical; items.content stays at
its pre-collab snapshot until TASK-1260 introduces the proper
5s idle flush with applier-bypass semantics. This is a known
Phase-2-internal regression closed by the very next task in
this run.
* fix(collab): tighten error classification + per-item lock + raw-mode flush per Codex review (round 5)
Three findings from round 5:
1) [HIGH] applier.go could return ErrAllAppliersTimedOut even when
no applier_request was ever successfully written (a row of write
failures followed by no remaining candidates). The handler-side
prune skipped that case, leaving stale op-log rows even though
no peer received the request.
Fix: track `anyWriteSucceeded` across the attempts and return
ErrNoApplierAvailable (which prunes) when the loop exits without
ever putting bytes on the wire.
2) [HIGH] Race between ApplyExternalContent's no-room classification
and the subsequent Prune/UpdateItem: a fresh Join could mint a
room and replay the soon-to-be-pruned op-log into a new client,
leaving it with stale Y.Doc state that overwrites the
freshly-written items.content on the next idle flush.
Fix: introduce per-item setup mutex on RoomManager. Join holds
the lock across addConn + replayTo and releases it before the
long-lived readLoop. New PruneAndApply method wraps the
prune+direct-write in the same per-item lock and re-verifies
"no live peers" under it (returns ErrRoomActiveDuringPrune if a
peer slipped in, in which case the caller falls through to a
plain direct write without pruning). Lock order: per-item lock
> m.mu > r.mu — Join and PruneAndApply both follow it.
3) [MEDIUM] Raw-mode 1.2s debounce timer could outlive the toggle
to rich mode: the deferred PATCH fired post-collab-mint and got
routed through the applier path (potentially overwriting newer
peer state).
Fix: track the latest pending raw markdown in
`rawPendingMarkdown`. The Rich-mode button is now an async
onclick that awaits a `flushRawIfPending()` synchronous PATCH
before flipping `rawMode = false` (which is what activates the
collab provider via the collabKey derivation).
* fix(collab): evict broken applier conn + retry on prune-race + retain raw pending on PATCH failure per Codex review (round 6)
Three findings from round 6:
1) [HIGH] When applier_request write failed, the broken roomConn
stayed in r.conns, defeating PruneAndApply's "no live peers"
check (which then returned ErrRoomActiveDuringPrune and the
handler skipped pruning). Net effect: the prune-safety
classification reverted to the round-5 hazard.
Fix: in the applier write-failure branch, force-close the conn
and call removeConn before continuing to the next applier. Both
are idempotent with the readLoop's natural cleanup path
(bus.Unsubscribe, conn map delete, conn.Close all tolerate
double-invocation).
2) [HIGH] On ErrRoomActiveDuringPrune the handler fell through to a
plain direct-write to items.content, bypassing the now-active
peer's applier. The peer's stale Y.Doc could still overwrite
items.content on the next idle flush.
Fix: surface ErrRoomActiveDuringPrune from
applyContentViaCollabOnce so the new applyContentViaCollab
wrapper can retry the full ApplyExternalContent flow against
the freshly-active room. Capped at applyContentMaxRetries=3 to
prevent runaway loops if joins keep landing during prune
attempts. After exhaustion, returns the same sentinel — the
handler's existing `if err == nil { input.Content = nil }`
gate falls through to direct write, which is the correct
degraded-mode behavior.
3) [MEDIUM] flushRawIfPending cleared rawPendingMarkdown before
the PATCH succeeded and the Rich-mode toggle always set
rawMode = false regardless of flush outcome. A failed flush
could activate collab with unsaved raw edits.
Fix: rework flushRawIfPending to return success bool, retain
rawPendingMarkdown on PATCH failure, and gate the Rich-button
transition on `ok`. Added a re-entrancy guard
(rawFlushInFlight) so a rapid double-click waits for the
in-flight flush to settle instead of issuing a duplicate PATCH.
* fix(collab): drain-loop flushRawIfPending to handle fast-typist edge per Codex review (round 7)
[P1] flushRawIfPending snapshotted rawPendingMarkdown then awaited
the PATCH; if the user typed during the await, the equality check
preserved the newer edit but the function still returned `true` and
the Rich-mode handler flipped collab on. The newly-active provider
then raced the un-flushed pending raw save — exactly the hazard
the guard is meant to close.
Fix: rework flushRawIfPending into a bounded drain loop. Each
iteration snapshots-PATCHes-clears (with the equality check). The
loop runs up to RAW_FLUSH_DRAIN_CAP=5 iterations, returning `true`
ONLY when rawPendingMarkdown is null on exit AND no PATCH failed.
A fast typist who keeps the queue non-null across the cap returns
`false`, leaving the user in raw mode (next click retries).
PATCH failure short-circuits with `false` so the toggle stays in
raw mode and the unsaved markdown is preserved for retry.
* fix(collab): atomic prune+content-write + preserve newer raw edit on stale PATCH response per Codex review (round 8)
Two findings from round 8:
1) [P1] PruneAndApply ran the op-log prune under the per-item lock
but the items.content write happened later in the post-loop
UpdateItem call, OUTSIDE the lock. A fresh Join landing in that
gap could replay the now-empty op-log, mint a peer with stale
Y.Doc state, and then overwrite the freshly-written
items.content on the next idle flush.
Fix: applyContentViaCollab now takes a `directWrite` callback
that the caller (handleUpdateItem) implements as a content-only
UpdateItem. PruneAndApply's applyFn invokes it AFTER the prune
so both run inside the same per-item critical section. The
trade-off is two DB round-trips when a PATCH carries content +
other fields together (rare): the content-only update happens
inside the lock; the rest (title, fields, status) flows through
the post-loop UpdateItem with input.Content nil'd to suppress
the duplicate write.
2) [P1] In flushRawIfPending's drain loop, `item = updated`
assigned the server-side snapshot from the just-PATCHed
markdown even when a newer raw edit had landed in the meantime.
RawMarkdownEditor mirrors `item.content` into its textarea
unconditionally (line 16), so the stale assignment would reset
the textarea mid-keystroke and lose the queued edit.
Fix: only swap in the full updated snapshot when
`rawPendingMarkdown === markdown` (no newer edit). Otherwise
keep our local content and adopt only the server-side metadata
(timestamps, version, modified_by) via spread.
* fix(collab): atomic mixed PATCH + raw autosave stale guard + rich→raw seeding per Codex review (round 9)
Three findings from round 9:
1) [P1] Toggling FROM rich+collab TO raw mode seeded
RawMarkdownEditor from items.content, which is intentionally
stale under collab (handleContentUpdate is suppressed while the
provider is connected; TASK-1260 closes that gap with a 5s
flush). Saving from raw mode would overwrite the live Y.Doc
state with a pre-collab snapshot.
Fix: when toggling to raw with a connected provider, capture
the editor's current Y.Doc-derived markdown via
`editor.storage.markdown.getMarkdown()` into a one-shot
`rawSeedMarkdown` slot and pre-populate `rawPendingMarkdown` so
the first auto-save persists it. RawMarkdownEditor seeds from
`rawSeedMarkdown ?? item.content`. Cleared on rich-mode toggle.
2) [P1] The regular debounced raw autosave still assigned
`item = updated` from a stale PATCH response. Same
stale-snapshot hazard the Round 8 fix closed in
flushRawIfPending.
Fix: equality-check `rawPendingMarkdown === toSave` before
swapping in the server snapshot. On stale, keep local content
and adopt only the server-side metadata via spread.
3) [P2] Round 8 split the items.content write (under per-item
lock) from the rest of UpdateItem (post-loop), losing
atomicity for mixed PATCHes (content + title) and breaking
Store.UpdateItem's content-versioning peek at Title.
Fix: directWrite callback now invokes the FULL UpdateItem
inside the per-item lock. A `fullWriteHandled` flag tells the
handler to skip the post-loop UpdateItem entirely (otherwise
we'd duplicate the write and create two version-history rows).
Mixed PATCHes are atomic again under the lock.
* fix(collab): clear raw seed/pending on item navigation per Codex review (round 10)
[P1] Navigating between items left rawSeedMarkdown,
rawPendingMarkdown, and the contentDebounceTimer set from the
previous item. This caused two concrete hazards:
(a) Item B's raw editor mounted with item A's live markdown via
`rawSeedMarkdown ?? item.content`.
(b) Clicking Rich on item B fired flushRawIfPending which
PATCHed A's queued markdown INTO item B (cross-item data
bleed).
Fix: at the top of loadData(), clear contentDebounceTimer,
rawSeedMarkdown, and rawPendingMarkdown so each navigation starts
from a clean slate. The collab provider's own lifecycle is
already keyed on item.id via $effect cleanup, so it doesn't need
the same explicit reset.
* fix(collab): item-id race guard on raw PATCH responses per Codex review (round 11)
[P1] In-flight raw PATCH responses (debounced autosave AND drain
loop) could clobber a newly navigated item. Clearing
contentDebounceTimer in loadData only cancels timers that have
not fired; an awaiting fetch keeps running and its `.then` /
`.catch` would assign back to the new page's `item` state.
Fix: mirror the existing TASK-754-style race guard pattern
(already used in the SSE / sync handlers above). Capture
`reqItemId = item.id` BEFORE the PATCH, then in the response
handler bail if `!item || item.id !== reqItemId`. Applied to
both handleRawContentUpdate's setTimeout body and
flushRawIfPending's drain loop.
* fix(collab): reset saveStatus on item navigation per Codex review (round 12)
[P2] After Round 11's race guard, a stale raw PATCH response that
matched a now-different item.id was correctly discarded — but
saveStatus had already been set to 'saving' before the await. With
loadData not resetting it, the next item could mount with
saveStatus pinned at 'saving' indefinitely, which then suppressed
all SSE/sync refreshes via the `if (saveStatus === 'saving')`
guards above.
Fix: in loadData's per-item state reset, clear saveStatusTimer
and reset saveStatus to 'idle' alongside the other transient
state. Cheap, scoped, no impact on the in-flight save's eventual
discard path.
|
||
|
xarmian
|
b4904e0db9 |
feat(editor): wire optional ydoc + Collaboration extension (TASK-1258) (#456)
* feat(editor): wire optional ydoc + Collaboration extension into Editor.svelte (TASK-1258)
Productionizes the Yjs binding pattern verified in the
feat/yjs-tiptap-spike branch (TASK-1245 sandbox). Editor.svelte now
accepts an optional `ydoc?: Y.Doc` prop; when set, it registers the
Tiptap Collaboration extension and disables StarterKit's undoRedo
so the y-tiptap binding takes over document state + history.
Editor.svelte changes:
- Imports: `Collaboration` from `@tiptap/extension-collaboration`,
type-only `Y.Doc` from `yjs` (the actual constructor lives in the
caller route — TASK-1260 wires the WS provider).
- Props: new optional `ydoc?: Y.Doc`. When undefined the editor's
extension list and behaviour are byte-identical to today;
every existing call site stays backward-compatible.
- Extensions:
· StarterKit.configure now spreads `{ undoRedo: false }` IFF
ydoc is set (Yjs owns history when collab is active; v3's
option is `undoRedo`, the v2 `history` key was renamed).
· `...(ydoc ? [Collaboration.configure({ document: ydoc,
field: 'default' })] : [])` slot at the end of the extension
array — empty when ydoc is undefined, so existing single-Doc
usage is unaffected.
Dependencies added to web/package.json:
- `yjs@^13.6.30`
- `@tiptap/extension-collaboration@3.22.5` — EXACT pin (no caret)
because @tiptap/core is on 3.22.5 and the latest published
3.23.x of extension-collaboration requires a matching core. The
CLAUDE.md note in TASK-1269 codifies the multi-package coordinated
bump rule.
- `@tiptap/y-tiptap@^3.0.3`
Out of scope (per task spec): WS provider plumbing (TASK-1260),
lifecycle binding to item-detail page (TASK-1260), first-edit seed
from markdown (TASK-1262), CollaborationCursor extension
(TASK-1264).
Parent: PLAN-1248. Phase 1 — Backend foundation complete; Phase 2
is the next logical block to start.
* fix(editor): gate content-prop sync $effect when ydoc is active per Codex review (round 1)
P1: Editor.svelte's existing $effect that calls
editor.commands.setContent on prop-content changes (item switch,
external REST update) would route through the y-tiptap binding as a
LOCAL ProseMirror change when Collaboration is registered —
overwriting peers' Y.Doc state with stale REST markdown on every
content prop refresh.
Add an early return in the $effect when ydoc is set. Y.Doc is the
authoritative state under collab; markdown→Y.Doc seeding for the
first-edit-on-empty case lives in TASK-1262 and uses Y.Doc's own
primitives, NOT setContent.
The early return also captures `tracker.prev = content` so a future
host route that swapped ydoc on/off mid-editor wouldn't see the
non-collab branch's `prev === undefined` and accidentally skip its
first sync. In practice ydoc is set once per editor mount today,
but the cheap capture keeps the contract honest.
|
||
|
xarmian
|
50e0936b34 |
feat(collab): designated-applier protocol for external content updates (TASK-1257) (#455)
* feat(collab): designated-applier protocol for external content updates (TASK-1257) The keystone task for CLI / API / MCP integration during co-edit sessions. When a content update arrives via PATCH while at least one browser tab is connected to the item's collab room, the server can't write items.content directly — the connected tabs would silently overwrite it on the next 5s idle flush using their (now stale) Y.Doc state and the caller's update would be lost. Solution: nominate one connected tab as the "designated applier", send it a JSON control message with the new markdown, the browser does editor.commands.setContent(markdown) which the y-tiptap binding translates into Y.Doc updates that propagate via the regular sync path. Items.content gets refreshed via the next 5s flush (TASK-1261). Architecture: internal/collab/applier.go (new): - ControlMessage struct — JSON envelope for applier_request / applier_ack frames. Carried over WebSocket TextMessage, which is unambiguous against y-protocol's BinaryMessage. - ApplyExternalContent(itemID, markdown) — public entry point. Returns nil on ack, ErrNoActiveRoom when there's no room (caller falls back to direct write), ErrNoApplierAvailable when the room has no live conns, ErrAllAppliersTimedOut when every attempt expired. - Election: pickApplier returns the longest-connected roomConn that hasn't already been tried, with deterministic tiebreak on conn id. Stable choice — longest connection has the most authoritative cumulative Y.Doc state, fewer flicker risks. - Retry loop: applierMaxAttempts=2, applierFirstTimeoutVar=30s, applierRetryTimeoutVar=15s. The Var-suffixed names exist so test helpers can shrink to ms without sleeping a real minute. - Pending-ack tracking: per-room map[requestID]*pendingApplierAck pairing the channel a PATCH handler is waiting on with the conn the ack is expected from. expectedConn check prevents an unrelated peer from spoofing acks for someone else's request. internal/collab/room.go (extended): - roomConn gains connectedAt for the election. - readLoop branches TextMessage → handleControlMessage which decodes the JSON and routes applier_ack to the room's pending tracker. Unknown control types and malformed JSON are silently dropped so a bad client can't break the loop. internal/collab/manager.go: - Registers connectedAt on Join. - Initialises room.pendingAcks alongside conns map. internal/server/handlers_items.go (extended): - handleUpdateItem now branches on input.Content != nil + s.collab != nil: routes through s.applyContentViaCollab; on success, zeros input.Content so UpdateItem's direct write is suppressed. - Field-only PATCHes skip this branch entirely — backward-compatible. internal/server/handlers_collab.go: - applyContentViaCollab wraps mgr.ApplyExternalContent with per-error-class slog warnings so operators can see degraded paths (timeouts → warn; no-room / no-applier → quiet, the common case for non-co-edit CLI updates). - actorIDFromRequest helper for log fields. Tests (5 new): - TestApplyExternalContentNoActiveRoom — sentinel error path. - TestApplyExternalContentHappyPath — applier echo acks within ms. - TestApplyExternalContentTimeoutsThenFails — applier never acks; we hit applierFirstTimeoutVar then ErrAllAppliersTimedOut. - TestApplyExternalContentTimeoutThenSecondAcks — first applier silent, retry picks second-longest-connected, succeeds. - TestApplyExternalContentRejectsAckFromUnexpectedConn — defence- in-depth: peer B forges an ack for peer A's request; the room's expectedConn check rejects it; ApplyExternalContent runs to timeout instead of being satisfied by the forgery. All tests pass under -race. Full suite green. Parent: PLAN-1248. Phase 1 — Backend foundation. * fix(collab): clean pendingAcks on success + expires_at on applier_request per Codex review (round 1) P2 #1: ApplyExternalContent retained the per-request pendingAcks entry on success. Each successful external update therefore leaked a request_id + channel + expected-conn pointer for the remainder of the room's lifetime — across long-lived sessions the map would grow without bound. Add cancelPendingAck to the ack-success path so the entry is released as soon as the request completes. Test added (TestApplyExternalContentCleansPending- AcksOnSuccess) drives 5 successful applies and asserts the pendingAcks map is empty afterwards. P2 #2: applier_request had no client-enforceable expiry, so a backgrounded tab could process a stale request 60s later and overwrite newer edits with old markdown after the server had already retried with a different applier (or fallen back to direct write). Add ExpiresAtMillis to the ControlMessage envelope, populated per attempt with `now + timeouts[attempt]`. The browser-side handler (TASK-1263) is responsible for the client-side Now() check before applying — without that check the field is documentation-only. Added test (TestApplyExternalContentSendsExpiresAt) regression-tests the server stamp. Server-side cleanup is also reinforced: cancelPendingAck on timeout (already present) means a late ack from a timed-out applier is rejected at the room layer (entry is gone). The expires_at_millis is the second line of defence for the case where the browser sends the Y.Doc setContent BEFORE the ack — the request must not be applied at all. |
||
|
xarmian
|
79eb00d2a1 |
feat(collab): periodic auth revalidation timer (TASK-1256) (#454)
* feat(collab): periodic auth revalidation timer (TASK-1256)
Catches mid-session revocations on a live collab WebSocket the same
way handlers_events.go's sseSubscriberStillHasAccess does for SSE.
When the WS handler upgrades, it spawns a goroutine that ticks every
collabMembershipRevalInterval (60s, jittered across [0, interval)
on first fire to avoid post-deploy reconnect-storm spikes). Each
tick re-runs authorizeCollabAccess — the same workspace-access
ladder used at upgrade time, including the "fresh-fetch user from
store" semantics that make admin-demoted-mid-stream visible without
waiting for the next request.
On access loss the handler routes through a new
RoomManager.CloseConn(itemID, conn, code, reason) method which:
- Looks up the roomConn in the manager so the close frame can go
out under the per-conn writeMu (no concurrent-write panic against
the room's writeLoop or replay path).
- Sends a websocket.ClosePolicyViolation frame with a human-readable
reason ("Your access to this item was revoked.") so the frontend
can stop reconnecting in a tight loop.
- Falls back to plain conn.Close when the conn isn't tracked yet
(race window between Join's getOrCreate and addConn).
The goroutine is bound to the handler's lifetime via a `stop`
channel that closes when handleCollab returns; no leaked timers
or goroutines after disconnect.
Test (TestCollabMembershipRevalidationClosesOnRevoke):
- Shrinks the reval interval to 30ms so the test runs in tens of
ms rather than 60 seconds.
- Bootstraps an admin (so the no-users escape hatch is closed),
creates a non-admin member user, mints a session, dials in.
- Calls RemoveWorkspaceMember while the WS is open.
- Asserts the next read returns an error (close frame or transport
failure — both are acceptable signals the server tore the
connection down).
Parent: PLAN-1248. Phase 1 — Backend foundation.
* fix(collab): WriteControl for revoke close + tighten test failure modes per Codex review (round 1)
P2 #1: CloseConn used rc.writeMessage which acquires the per-conn
writeMu. If the room's writeLoop / replay was mid-WriteMessage to a
slow peer, revocation would block behind that writer and never
force-close the unauthorized conn. Switch to conn.WriteControl,
which gorilla documents as concurrency-safe with normal writes
(it bypasses the conn's normal write path) and accepts an explicit
deadline so a stuck send can't extend the budget indefinitely.
The deadline is 1s — generous for a healthy conn, short enough that
a half-broken socket falls through to plain Close quickly. The
itemID parameter stays in the API for symmetry / future per-room
metrics, but is no longer used for the actual close path now that
the writeMu lookup is gone.
P2 #2: TestCollabMembershipRevalidationClosesOnRevoke previously
treated a read-deadline timeout as a log-only branch — the test
could pass after waiting 2s with the WS still open, exactly the
bug being regression-tested. Restructure to fail fast on timeout
(t.Fatalf isTimeout(err)) and prefer ClosePolicyViolation as the
expected close code, falling back to "any non-timeout error" only
because the underlying TCP teardown can produce different error
shapes depending on timing. The isTimeoutOrEOF helper that
masked the failure is replaced with a narrowly-scoped isTimeout.
* fix(collab): distinguish access denial from transient errors in reval per Codex review (round 2)
P-MEDIUM: the revalidation goroutine treated any non-nil error from
authorizeCollabAccess as revocation, including transient store
errors (GetUser / GetWorkspaceByID / grant-lookup blips). One DB
hiccup would close every active collab WS with
ClosePolicyViolation, which is a worse UX than the bug being
guarded against.
Distinguish via errors.As against *statusError (the typed return
from authorizeCollabAccess used for all "we know they don't have
access" branches). Plain errors fall through to a warn-level log
+ timer reset so the next tick retries.
Three branches in the revalidation switch now:
err == nil still authorised — reset timer.
isAccessDenial(err) real revocation — close conn with typed reason.
default transient — log warn, keep conn open, reset.
* fix(collab): re-fetch item on each reval tick per Codex review (round 3)
P2: revalidation re-authorized against the *Item captured at
upgrade time, so an item moved to a collection the user can't see —
or hard-deleted — would not be caught: authorizeCollabAccess kept
checking the stale CollectionID, kept passing, and the WS stayed
open against an item the user no longer has access to.
Re-fetch via s.store.GetItem(itemID) at the start of each tick:
- error → log warn, keep conn open, retry next tick (matches the
transient-store-error policy from round 2).
- nil → item hard-deleted (or never existed): close with
ClosePolicyViolation + "This item is no longer available."
- otherwise → authorize against the FRESH item, picking up any
collection move automatically.
Per-tick GetItem is one indexed lookup per minute per active
connection — negligible compared to the auth-cascade GetUser /
member / grant queries that already run on the same tick.
|
||
|
xarmian
|
e7b1c3b5ae |
feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453)
* feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255)
Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a
working dumb-relay collab server. Per-item Room created lazily on
first Join, kept alive across transient disconnects via a 60s grace
TTL, reclaimed when the grace expires with no fresh subscribers.
Components:
- internal/collab/room.go — Room struct + lifecycle
· roomConn pairs (id, conn, bus channel, write mutex). The id is
server-assigned per WS so writeLoop can suppress own-event echoes
without decoding the Y.Doc to read the Yjs ClientID.
· readLoop discriminates yMessageSync vs yMessageAwareness on
byte 0. Sync frames are persisted to the op-log AND broadcast;
awareness frames are broadcast only (presence is ephemeral).
Persistence happens BEFORE broadcast so a crash mid-publish loses
at most a live keystroke that the originator will replay on
reconnect anyway.
· writeLoop drains the bus subscription and writes non-self events
to the WS, gated by a per-conn write mutex (gorilla's "one writer
at a time" rule).
· removeConn arms a 60s graceTimer when the last conn drops; a
fresh addConn cancels the timer. onGraceExpired re-checks
len(conns) == 0 under the room mutex and only THEN sets
closing=true + calls back to the manager. The race between
"manager.getOrCreate found us" and "grace timer fired" is
handled by addConn returning errRoomClosing; the manager retries
via getOrCreate which mints a fresh Room.
- internal/collab/manager.go — RoomManager + RoomManagerConfig
· NewRoomManager wires production defaults (DefaultGraceTTL = 60s,
DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an
explicit config so tests can drop graceTTL to a few ms without
sleeping a minute. graceTTL is per-manager, not a package var,
so parallel tests with different TTLs don't trip the race
detector.
· Join is the public entry point: getOrCreate → addConn (with
retry on errRoomClosing) → replayTo → spawn writeLoop goroutine
→ run readLoop inline → wait for writeLoop drain → return. The
inline read keeps the HTTP handler in scope so its
`defer conn.Close()` doesn't fire until both loops exit.
· Close is for graceful server shutdown — closes every active
conn under the room mutex, then drains the manager's room map.
- internal/collab/manager_test.go — 7 tests covering: lazy create,
op-log replay-on-connect (two seed rows arrive in order), sync
broadcast + persist (peer B sees A's frame, originator does not
echo, op-log gains a row), awareness broadcast WITHOUT persist,
cross-item isolation (item-a frames don't leak to item-b
subscribers), grace-TTL reclaim with a 50ms config TTL, grace
cancel on reconnect within window, manager.Close shuts down
every active conn. All tests run with -race; the bus's
concurrent-publish test was already covered by TASK-1253.
- internal/server/handlers_collab.go — wire to RoomManager
· Returns 503 when s.collab is nil (matches the SSE handler's
"events bus not configured" 503 — fail loud rather than silently
accept the upgrade).
· Otherwise hands the upgraded conn to s.collab.Join, which
blocks until the WS closes. Unexpected close codes get the same
warn-log as before; normal closures stay quiet.
- internal/server/server.go — adds *collab.RoomManager field +
SetCollabRoomManager setter (nil-safe optional, like SetEventBus).
- cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into
the running server alongside the event-bus wiring. Single-instance
only today; multi-replica fanout via Redis is a deferred IDEA per
the Plan body.
- internal/server/handlers_collab_test.go — adds
testServerWithCollab helper (so existing collab tests get a real
RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager
which asserts the 503 path for unwired servers.
Parent: PLAN-1248. Phase 1 — Backend foundation.
* fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1)
P1 — concurrent peers raced AppendYjsUpdate, violating the
single-writer-per-item contract documented on the store call. Each
peer's readLoop runs in its own goroutine, so two peers in the same
room could call AppendYjsUpdate concurrently. On Postgres that
risks the BIGSERIAL allocation-vs-commit-order cursor gap that
TASK-1252's contract was specifically guarding against. Add an
appendMu on Room held across the persist+publish sequence; reads,
awareness frames, and OTHER rooms remain unserialised.
Regression test (TestRoomManagerSerializesSyncAppends) drives 4
peers × 10 writes concurrently and asserts the op-log gains exactly
40 rows. Without appendMu this would intermittently surface fewer
rows or out-of-order ids on Postgres; with it the count is
deterministic and the race detector stays clean.
P2 — Server.Stop did not close s.collab. Active collab WS goroutines
+ grace timers could keep using s.store after the server's other
cleanup paths winding down. Add s.collab.Close() before
rateLimiters.Stop so any Join goroutines holding rate-limiter
handles can wind down cleanly. nil-safe via the existing collab
optional-attachment pattern.
* fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2)
P2: a joining peer subscribed to live events BEFORE its writer
goroutine started. During a long replay, live sync events would pile
up in the 64-event bus channel; once full, MemoryOpBus.Publish
silently drops them, leaving the new peer connected but permanently
missing those updates.
Restructure runConn to spawn the writer goroutine FIRST so it drains
the bus subscription concurrently with the replay. Both replay and
writer go through rc.writeMessage, which holds the per-conn write
mutex, so we never violate gorilla's one-writer-at-a-time rule.
Yjs CRDTs are commutative — applying live op 100 before replay op 50
yields the same final Y.Doc as the reverse order — so interleaving
is correct. The trade-off is a brief "out of causal order" UX wobble
during replay, which is acceptable: the alternative would require
either an unbounded queue or losing updates the way the original
order did.
* fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3)
P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never
srv.Stop. http.Server.Shutdown does NOT terminate hijacked
connections (WebSockets), so active collab sessions kept running
until process exit and could race the deferred store close. The
RoomManager.Close path added in round 1 only fires inside Stop, so
without this call the production shutdown was effectively bypassing
the new cleanup.
Add srv.Stop() after srv.Shutdown in the serveCmd shutdown
sequence. Stop also runs the existing background-loop teardowns
(orphan GC, MCP audit writer, MCP session tracker) which were
previously already part of Stop's contract — those will continue to
fire as they always have, so this commit's only behavioural change
is "now also closes the collab room manager".
* fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3)
P1 — RoomManager.Close was not a true drain barrier. closeAll
closed the WebSockets but did NOT wait for the corresponding Join
goroutines (running runConn) to exit. Server.Stop returned before
in-flight collab work finished, racing the deferred store close
on process exit. Fix: track every Join in m.activeJoins
(sync.WaitGroup); Close iterates closeAll first (waking up every
reader by closing the conn), then activeJoins.Wait — guaranteeing
no collab goroutine is still running by the time Close returns.
P2 — replay-time bus overflow could still drop sync events on a
slow drain (writeLoop blocks on the same writeMu replayTo holds,
so a long replay starves the bus drain even with the writer
goroutine started before replay). Two-part response:
(a) Bump the per-subscriber bus channel buffer from 64 to 256.
Sized for a 5x safety margin on a 1k-row replay against a
chatty 5-peer room (~50 events/sec during a ~1s replay).
(b) The architectural fix — force-close subscribers on overflow,
honoring the bus's documented slow-peer recovery contract — is
filed as TASK-1273 follow-up. That requires extending the OpBus
interface (per-subscriber drop callback or counter) and an active
health-check tick in the room manager; both are out of scope for
TASK-1255's "lazy room + grace TTL" deliverable.
For PLAN-1248's single-instance scope and typical editor load,
256 covers realistic workloads. Pathological / load-test scenarios
exposing overflow can recover via Yjs's state-vector negotiation
on reconnect, and TASK-1273 will tighten that to an active kick.
* fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4)
P2: http.Server.Shutdown does NOT wait for hijacked WebSocket
handlers, so a Join() call from a freshly-upgraded conn could fire
AFTER Close() returned. The previous Add-then-Wait pattern was
correct for already-started Joins but couldn't catch a Join that
hadn't yet hit Add when Close fired. Race: Close iterates the (empty)
rooms map, Wait sees zero waiters, Close returns; THEN Join hits
Add and proceeds against a torn-down store.
Add a `closed` flag gated by the same mutex that wraps
activeJoins.Add. Three orderings, all safe:
1. Add before Close.closed=true → Wait blocks until Done.
2. Close.closed=true before Add → Join sees closed=true under
the same lock and returns errManagerClosed without ever
incrementing the WaitGroup.
3. Close called twice → second call short-circuits (idempotent).
getOrCreate also gets a closed-flag short-circuit so a future
caller can't bypass the gate by skipping Join.
Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close
Join returns errManagerClosed, plus a second Close() is a no-op.
All 15 collab tests pass under -race.
|
||
|
xarmian
|
2945ee27dd |
feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254) (#452)
* feat(server): add WebSocket handler at /api/v1/collab/{itemID} (TASK-1254)
WebSocket entry point for Yjs-based collaborative editing on a
single item under PLAN-1248. Bare-bones in this PR by design:
upgrade + log connect/disconnect + drain reads. Protocol logic
(forwarding to OpBus, persisting to op-log, awareness fan-out)
arrives in TASK-1255 (room manager).
Authorisation mirrors RequireWorkspaceAccess but keyed on the
item's workspace ID rather than a {slug} URL param — the WS URL
only carries itemID. Implementation re-uses the same access
ladder:
fresh-install escape hatch (no users)
→ grant
legacy workspace-scoped API token, no user
→ grant if token's workspace matches the item's workspace
OAuth token allow-list (TASK-953)
→ reject when workspace not on consented list
authenticated user
→ admin OR member OR has guest grants
User is re-fetched from the store on each upgrade (not trusted
from session-context cache) so a mid-session admin demotion or
member removal closes the upgrade path immediately. Mirrors
sseSubscriberStillHasAccess. Periodic per-connection
revalidation lives in TASK-1256.
Route registered alongside SSE (outside the jsonContentType
middleware group, but inside the auth middleware chain). Promotes
github.com/gorilla/websocket from indirect to direct dep and
bumps to v1.5.3 (latest stable; v1.5.0 was already in
go.mod transitively via another package).
Tests cover:
- fresh-install escape hatch grants the upgrade
- bootstrapped server rejects unauthenticated upgrade with 401
- non-member with valid session is rejected with 403
(NOT 401 — confirms the access path runs after auth, not before)
- unknown item surfaces as 404 (not 401/403 leak)
- empty itemID segment doesn't match the route
Test infrastructure note: dialCollab takes an explicit User-Agent
because pad's session-binding middleware hashes the UA at
CreateSession time and re-checks on every request — the dialer
must match what was stored, otherwise the cookie is rejected
before the workspace check fires (and we'd see a misleading 401
where 403 was expected).
Parent: PLAN-1248. Phase 1 — Backend foundation.
* style: gofmt handlers_collab_test.go per Codex review (round 1)
* fix(server): SetReadLimit + nginx upgrade headers for collab WS per Codex review (round 2)
P-MEDIUM #1: handleCollab.ReadMessage had no per-message size cap, so an
authenticated client could send an arbitrarily large frame and force
unbounded server-side buffering — the HTTP body limit applied by the
auth chain doesn't apply once the connection is upgraded. Set
SetReadLimit(1 MiB), generous for keystroke-rate Yjs ops and large
enough for a typical initial-sync state. ReadMessage returns an error
when exceeded, which the existing read loop handles as a normal close.
P-MEDIUM #2: deploy/nginx.conf routed /api/v1/collab/ through the
default `location /` block, which sets `Connection ""` (cleared so HTTP
keepalive works) — that strips the Upgrade header, so WebSocket
upgrades silently fail behind the documented nginx deployment. Add a
dedicated location block with proxy_set_header Upgrade $http_upgrade /
Connection "upgrade", same 24h read/send timeouts as SSE so an idle
editor tab does not get cut off mid-session.
* fix(server): enforce per-item visibility in collab WS upgrade per Codex review (round 3)
P2: authorizeCollabAccess granted upgrade to any workspace member or
guest-with-grants without checking whether THIS specific item was
visible to that user. A restricted member (collection_access=specific)
or a guest with grants on item A could upgrade /api/v1/collab/{itemID}
for an item B in a different collection — they'd see live edits to a
document they have no right to read.
Restructure the access ladder:
1. Workspace-level gate stays as-is: "any access at all?" If no
membership AND no grants → 403 (unchanged).
2. Item-level visibility check added on top, mirroring requireItemVisible
without depending on middleware-set request context (the WS path
doesn't go through RequireWorkspaceAccess):
- VisibleCollectionIDs nil → "all" access → grant.
- Item's collection in the visible set → grant.
- Item-level grant on this exact item → grant (covers guests
given access to a single item rather than a whole collection).
- Else → 404, mirroring requireItemVisible's "don't leak
existence" pattern.
Admin path returns nil before this check, so no change there.
Legacy workspace-scoped API tokens grant editor-equivalent access
on workspace match (predates the grants design); that branch is
untouched since legacy tokens don't have a user identity to scope
per-item grants against.
Test added: TestCollabUpgradeRejectsRestrictedMemberForeignCollection
— member with specific access to collA tries to upgrade for an item
in collB → 404. Existing 5 tests still pass.
* fix(server): strict per-item visibility check + sibling-grant test per Codex review (round 4)
P1 (round 4): VisibleCollectionIDs is broader than full-collection
access — it includes collections "anchored" by an item-level grant
(so the nav can still surface the parent collection of a granted
item). Round 3's check treated every visible collection as full
access; a guest with grant `item:A` could upgrade
/api/v1/collab/{B} for a sibling B in the same collection.
Tighten by mirroring guestResourceFilter / requireItemVisible:
1. Coarse stage stays — collection must be in the visible set.
2. NEW strict stage when the user has item-level grants:
(a) full collection grant on this collection → grant
(b) member's "specific" access list including this collection
→ grant
(c) item grant on THIS exact item → grant
Else → 404 (the visible-set hit was anchored by a sibling's
grant, not by full collection access).
When the user has NO item grants, the coarse-only check is
sufficient — visibility came from full collection access (member's
"specific" list, full collection grant, or "all" access).
Test added: TestCollabUpgradeRejectsGuestWithSiblingItemGrantOnly
— guest with item:A grant tries to upgrade for sibling B in the
same collection → 404 (the bug being regression-tested) AND verifies
the granted item A still upgrades cleanly to 101 Switching Protocols.
|
||
|
xarmian
|
264f5b0041 |
feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253) (#451)
* feat(collab): add OpBus interface + in-process MemoryOpBus (TASK-1253)
New internal/collab package for the dumb-relay collab server in
PLAN-1248. Defines the OpBus pub/sub interface and ships
MemoryOpBus, the in-process implementation used by every shipping
target today (single-binary self-host, single-replica pad-cloud).
OpBus shape mirrors internal/events.MemoryBus so a future RedisOpBus
is a drop-in for multi-replica deployments — that's filed as a
separate IDEA at PLAN-1248 close, since the dumb-relay design
intentionally keeps Redis off the self-host dependency surface.
OpEvent carries:
- ItemID fan-out filter
- ClientID Yjs client id, used by designated-applier election
(TASK-1257); the bus itself does not interpret it
- Type "sync" (Y.Doc binary update — persisted by the room
manager) or "awareness" (cursor/presence — broadcast
only, never persisted)
- Data raw y-protocol message; opaque to the server
- Timestamp UnixMilli, auto-stamped on Publish
MemoryOpBus semantics:
- 64-event buffered subscriber channels (matches internal/events
default — sized against keystroke-rate workload).
- Non-blocking Publish: a slow subscriber whose channel is full has
events DROPPED with a warn log rather than back-pressuring the
broadcast loop. The room manager (TASK-1255) is responsible for
closing genuinely unhealthy peers; the bus only protects itself.
- Idempotent Unsubscribe (no panic on double-unsubscribe).
- Close clears the subscriber map and closes every channel under
the same write lock that gates Publish, so a final inflight
Publish can't race a Close into delivering on a closed channel.
Tests cover: subscribe/publish fan-out per item filter, unsubscribe
closes the channel, slow-consumer drop without blocking, accurate
SubscriberCount across subscribe/unsubscribe, Close cleans up every
channel regardless of itemID, and concurrent-publishers race-clean
under -race (small drop count tolerated — that's the slow-consumer
contract; an exact-delivery test would defeat its own purpose).
No external dependencies beyond stdlib + slog. RedisOpBus stub is
intentionally NOT included — separate IDEA per the Plan body.
Parent: PLAN-1248. Phase 1 — Backend foundation.
* fix(collab): clone OpEvent.Data + document recovery contract per Codex review (round 1)
P1 — Sync-op drops were undocumented as recoverable. Sync drops ARE
recoverable in the dumb-relay design: the room manager (TASK-1255)
appends to the op-log BEFORE Publish, so any peer that misses a sync
op via channel-full drop can replay since their last cursor on
reconnect (TASK-1252's LoadYjsUpdatesSince + Yjs state-vector
negotiation). The room manager is responsible for detecting slow
channels and force-closing the owning WebSocket, which kicks the
peer into a fresh reconnect + replay. The bus does not take that
action itself because it has no concept of which peer owns which
channel — that mapping is the room manager's domain. Doc comment now
spells this out explicitly.
P2 — OpEvent.Data is a []byte; the same slice header was queued to
every subscriber, so a publisher's later buffer reuse OR any
subscriber's mutation could corrupt the bytes other subscribers
observe. gorilla/websocket's ReadMessage is allowed to reuse its
read buffer between messages, so this hazard is real for the
production publisher (the WS handler in TASK-1254). Clone Data once
at the publish boundary; document the per-receiver immutability
expectation in the same comment block.
No behavioral test change — the existing slow-consumer-drop test
still passes; the clone path adds one allocation per Publish but
nothing observable to callers beyond the immutability guarantee.
|
||
|
xarmian
|
04514817ae |
feat(store): add Yjs op-log table + store methods (TASK-1252) (#450)
* feat(store): add Yjs op-log table + store methods (TASK-1252)
Persistence groundwork for the dumb-relay WebSocket server in PLAN-1248.
The item_yjs_updates table records every Yjs binary update (browser
edits, future designated-applier conversions of CLI/API content
changes) so reconnecting peers can replay updates since their last
known cursor and cold rooms can rebuild their in-memory Y.Doc.
Schema (mirrored across SQLite + Postgres):
- id monotonic — INTEGER PRIMARY KEY AUTOINCREMENT (SQLite)
/ BIGSERIAL (Postgres). Never reused, even after
deletes; serves as the cursor every reconnecting
client compares against.
- item_id FK with ON DELETE CASCADE so item deletion reclaims
op-log space automatically.
- update_data raw Yjs binary update — BLOB / BYTEA. Opaque to the
server.
- schema_version stamped per row. Mismatch on connect drives
TASK-1268's snapshot-and-rebuild flow.
- created_at ISO8601 UTC TEXT, matching pad's cross-dialect
timestamp convention (see migrations/047_attachments).
Drives PruneYjsUpdatesBefore.
Store API (internal/store/yjs_updates.go):
- AppendYjsUpdate — validates non-empty itemID/data/schemaVersion,
inserts and returns the new monotonic id (RETURNING on Postgres,
LastInsertId on SQLite). Empty-zero-byte updates are rejected at
the Go layer rather than relying on NOT NULL — they're a no-op
that would only pollute the log.
- LoadYjsUpdatesSince — strict id > sinceID filter, ordered by id
ascending. sinceID=0 returns everything (cold-room rebuild path).
Tolerates either RFC3339 or "YYYY-MM-DD HH:MM:SS" timestamp formats
on read so any future operator-written / CURRENT_TIMESTAMP-style row
doesn't blow up the load path.
- PruneYjsUpdatesBefore — created_at < cutoff, scoped to itemID.
Returns rows-affected count. Used by the eventual GC sweeper
(out of scope for this task).
Tests cover: append + monotonic ids, load-since-cursor filtering,
input validation, prune scoped to itemID, and ON DELETE CASCADE on
parent item removal. Pass on SQLite locally; Postgres mirror migration
+ store methods are dialect-agnostic.
Parent: PLAN-1248. First task of Phase 1 — Backend foundation.
* docs(store): document AppendYjsUpdate per-item serialization contract per Codex review (round 1)
P1: Postgres BIGSERIAL ids are allocation-ordered, not commit-order.
Concurrent appends to the same item could in theory produce a cursor
gap — a slower transaction can hold a smaller id while a faster one
commits a larger id first, and a reader that advances past the visible
larger id would later miss the smaller id when it commits.
The dumb-relay room manager (TASK-1255) is the sole writer per item by
design — there's exactly one goroutine appending per Y.Doc — so the
hazard does not manifest in practice. The fix is at the API contract
level: the doc comment now spells out the serialization requirement,
why the room manager satisfies it, and the multi-replica re-enforcement
note for the future Redis-fanout IDEA. We do not take an internal
advisory lock because that would be paid by every append even though
the caller already holds the per-room mutex.
No code change — contract is at the doc comment.
|
||
|
xarmian
|
00d529d325 |
fix(editor): add NodeView update() hook to AttachmentChip (TASK-1251) (#449)
* fix(editor): add NodeView update() hook to AttachmentChip (TASK-1251) Audit confirmed the same gap pattern as BUG-1246 (mermaid) / TASK-1250 (attachment-image): closure-captured uuid + filename, no update() hook, so any attr change forces NodeView destroy+recreate. No in-tab attr-change source today (no rotate/crop on chips), but the upcoming Yjs collab work makes peer-driven uuid/filename swaps the common case — destroying the NodeView on every peer keystroke of a rename would jump the cursor and flicker the chip. Refactor closure state to be mutable, add update() that: - Returns false on type mismatch (defensive — should never fire for an attachmentChip). - On uuid change: refresh href + data-attachment-id, reset MIME + size pending re-probe, re-fire the HEAD metadata fetch (with in-flight guard so a stale probe doesn't trample fresher state). - On filename change: refresh data-filename, download attribute, visible name, and (only when MIME unknown) re-derive the filename-extension fallback icon. Click handler now reads currentUuid (mutable) so a peer-swapped chip target opens the new attachment, not the original. Parent: PLAN-1248. Phase 0 — Production prerequisites complete. * fix(editor): always refresh chip icon on rename per Codex review (round 1) P3: refreshIcon() falls back to the filename-extension heuristic whenever iconForMime() returns empty — both MIME-unknown and MIME-known-but-unmapped (e.g. application/octet-stream) hit the same fallback path. The previous `if (!currentMime) refreshIcon()` was too narrow; renaming foo.csv → foo.pdf after a generic MIME resolved would leave the stale .csv icon until the NodeView was recreated. Drop the conditional. refreshIcon() is idempotent for MIMEs that DO map to a specific icon (just rewrites the same emoji), so calling it on every filename change is correct and trivially cheap. |
||
|
xarmian
|
530a2c05ba |
fix(editor): add NodeView update() hook to AttachmentImage (TASK-1250) (#448)
* fix(editor): add NodeView update() hook to AttachmentImage (TASK-1250) Same hazard pattern as BUG-1246 (mermaid) — the NodeView had no update() hook, so any attribute change forced ProseMirror to destroy + recreate the NodeView. Single-user rotate worked via that destroy + recreate path (invisible flicker + cursor jump), but under upcoming Yjs collab a remote peer's rotate would constantly remount the local image element. Add the hook and refactor closure state so attr changes refresh the live <img> in place: - update(updatedNode) returns false on type mismatch (defensive — should never fire for an attachmentImage), true otherwise. - uuid + alt promoted from const to let (currentUuid / currentAlt) so event handlers (click → lightbox, rotate, crop) read the latest value. - On uuid change: invalidate metadata cache for the OLD uuid, swap img.src + data-attachment-id, reset toolbar MIME gating, re-probe metadata for the NEW uuid (with a guard so a probe in flight doesn't trample a fresh swap). - On alt change: refresh img.alt. - swapNodeUuid loses its trailing invalidateAttachmentMetadata call — that lives in update() now so it fires regardless of source (local rotate, peer Yjs op, ...). Verifies single-user rotate still works AND lays groundwork for zero-flicker peer rotate under Phase 2 Yjs collab. Parent: PLAN-1248. * fix(editor): read live node attrs in swapNodeUuid per Codex review (round 1) P-LOW: with update() now keeping the AttachmentImage NodeView alive across attr changes, the closure-captured `node` is no longer guaranteed to reflect the document. Spreading its stale attrs in setNodeMarkup would clobber any concurrent edit to a non-uuid attr — e.g. a peer changes alt text, then a local rotate dispatches with the original alt and overwrites the peer's update. Resolve by reading node attrs from editor.state.doc.nodeAt(pos) at the moment of dispatch, so we always merge the new uuid onto the freshest known attrs. Bail if nodeAt returns null (edge: pos no longer points at this node — e.g. node was deleted in the same tick). * fix(editor): snapshot uuid in runRotate/runCrop per Codex review (round 2) P2: with the NodeView now surviving attr changes, currentUuid (mutable closure) can drift during the await on opts.transform() / openCropModal — a peer rotating or cropping the same image mid-flight would shift currentUuid out from under us. Crop is the worst case: the rect was chosen against the original image, but applying it to whatever uuid is live at completion time would crop the wrong image. Resolve by snapshotting currentUuid (and currentAlt for the crop modal) at the entry of each async handler, threading the snapshot through the transform call, and bailing if currentUuid drifted during any await. Same hazard surface in both functions; same fix shape applied to both. |
||
|
xarmian
|
1ef7a21de2 |
fix(editor): add NodeView update() hook to MermaidCodeBlock (TASK-1249) (#447)
* fix(editor): add NodeView update() hook to MermaidCodeBlock (TASK-1249) Mermaid diagrams previously froze on the SVG generated when the NodeView was first created. ProseMirror only recreates a NodeView on node identity change; in-place text edits don't trigger that, and the existing factory had no `update()` hook to re-queue a render — so editing the source via the hover-revealed `< >` toggle silently mutated the code while the diagram showed stale output. Resolves BUG-1246. Implementation matches the pattern verified in the TASK-1245 spike (iteration 3, dev sandbox at /dev/yjs-sandbox) but with precise ProseMirror Node typing instead of `any`: - update(updatedNode) returns false on type mismatch or when the language attr flips into/out of `mermaid` — different DOM shape, so ProseMirror must tear down + recreate the NodeView. - Returns true (in-place update accepted) when same-node + same-lang; re-queues queueMermaidRender only when textContent actually changed. - Empty source clears the diagram element and the mermaid-error class. - Toggle state survives because we don't recreate the wrapper. Becomes a hard blocker once Yjs collab lands (PLAN-1248) since remote ops will constantly mutate mermaid source text mid-view. Parent: PLAN-1248. * fix(editor): serialize mermaid clear + drop error class on success per Codex review (round 1) Two issues raised in PR #447 review: P2 — Pending queueMermaidRender() could overwrite a synchronous diagram clear with a stale SVG, racing against a freshly-emptied source. Route the clear through the same renderQueue (queueMermaidClear) so it executes strictly after any in-flight render for the same target. P3 — A valid re-render after an invalid mermaid edit kept the .mermaid-error class on the diagram element. Drop the class in queueMermaidRender's success path now that successful render means the source compiled. Both fixes preserve TASK-1249's NodeView update() contract; no other behavior changed. |
||
|
xarmian
|
07e47eba57 |
fix(web): subscribe item detail page to live SSE updates (TASK-1243) (#446)
* 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
|
||
|
xarmian
|
578494dc43 |
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.
|
||
|
xarmian
|
1dabfd02ae |
chore(web)(deps): bump vite-plugin-svelte 6 → 7 + vite 7 → 8 (TASK-1238) (#444)
Coordinated bump of the Vite/Svelte build-tool stack:
• @sveltejs/vite-plugin-svelte 6.2.4 → 7.1.2
• vite 7.3.1 → 8.0.11
• @sveltejs/kit 2.59.0 → 2.59.1 (patch, free)
• @sveltejs/adapter-auto 7.0.0 → 7.0.1 (patch, free)
Smaller cascade than the deferred-bumps task body anticipated:
SvelteKit 2.59 already declared `^8.0.0` in its vite peer-dep range, so
no Kit major bump was needed. adapter-static is unaffected. svelte
itself (5.55.5) already meets the new vite-plugin-svelte v7 peer
constraint of ^5.46.4.
vite-plugin-svelte v7 integrated the inspector into the main package,
so the @sveltejs/vite-plugin-svelte-inspector subdep is gone — net 5
fewer packages in node_modules and a ~7KB smaller package-lock.json.
Vite 8 highlights:
• Rolldown replaces Rollup as the bundler — production build dropped
from ~12-16s to ~5.9s on this codebase
• Internally compiled with TypeScript 6 (matches our own TS 6 bump
from TASK-1236)
• npm audit moderate vulnerability count: 1 → 0 (the uuid advisory
was in a transitive that's no longer needed)
Configs reviewed:
• vite.config.ts is minimal (just sveltekit() plugin + dev proxy);
none of v7's removed options (vitePlugin.hot,
vitePlugin.ignorePluginPreprocessors, api.idFilter,
plugin.api.sveltePreprocess) are in use.
• svelte.config.js uses vitePlugin.dynamicCompileOptions (still
supported in v7).
Verified:
• Clean reinstall (rm -rf node_modules package-lock.json && npm i):
290 packages, 0 vulnerabilities
• npm run build: succeeds in 5.91s, output passes through
adapter-static
• make check (golangci-lint + go test + npm run build + svelte-check):
0 errors, same 6 pre-existing warnings
• Manual UI verify: dashboard, item list, item detail, editor (block
drag-handle, content edit/save), role board, share page all render
and function correctly
Rolldown surfaced one informational warning that's PRE-EXISTING in our
code, not introduced by the bump:
[INEFFECTIVE_DYNAMIC_IMPORT] sync.svelte.ts is both static- and
dynamic-imported. Filed as a follow-up task — fix is out of scope
for this PR.
Closes dependabot/npm_and_yarn/web/sveltejs/vite-plugin-svelte-7.0.0
(PR #215). Closes PLAN-1240 Tier 3 (5/5 ships).
|
||
|
xarmian
|
e380b4e660 |
chore(deps): bump Node 22 → 24 (Dockerfile + CI workflows) (TASK-1235) (#443)
Bumps all four Node version pins from "22" to "24" together so CI ↔
production stay aligned:
• Dockerfile (production image): node:22-alpine → node:24-alpine
• .github/workflows/ci.yml — Web job + E2E job
• .github/workflows/release.yml — release pipeline
Going to LTS-bound 24 instead of dependabot's proposed 25-alpine,
which hits EOL on 2026-06-01 (~3 weeks from this commit). Going to
24 instead of waiting for 26-LTS (Oct 2026) because 5 months is too
long to sit on the deferred-bumps backlog and 24 is already a year
into LTS-tested production use. Worst case follow-up is one more
trivial Dockerfile bump in October.
Verified:
• Local `docker build --target web-builder` on node:24-alpine
(npm ci + npm run build) — clean, 24s end-to-end
• `make check` — golangci-lint + go test + npm run build +
svelte-check, 0 errors
Closes dependabot/docker/node-25-alpine (PR #209) — closing rather
than rebasing because we're going to 24, not 25.
|
||
|
xarmian
|
1ac3f76478 |
chore(web)(deps): bump typescript 5.9.3 → 6.0.3 (TASK-1236) (#442)
The compiler bump itself is clean — svelte-check (TS 5.9 baseline) and
svelte-check (TS 6.0.3) both report 0 errors against the same 714-file
codebase. Confirms TypeScript 6's stricter inference doesn't surface
new errors in our Svelte 5 / SvelteKit / TipTap stack.
`tsc --noEmit` directly (which traverses standalone .ts files differently
than svelte-check) flagged 9 errors in one file — block-drag-handle.ts
calls TipTap chained commands (setParagraph, setHeading, toggleBulletList,
toggleOrderedList, toggleTaskList, toggleCodeBlock, toggleBlockquote)
that are added to `ChainedCommands` via TypeScript module augmentation
in the respective extension packages. TS 6 stopped propagating those
augmentations to files that don't import the augmenting modules
themselves, so each consumer must opt in.
Fix: add side-effect imports of `@tiptap/starter-kit` and
`@tiptap/extension-task-list` at the top of block-drag-handle.ts. The
modules are already loaded at runtime by Editor.svelte, so this adds
nothing to the bundle — it just re-registers the type augmentations
in this file's compilation context.
Verified:
• Manual: hover the drag handle, open the block context menu,
"Turn into" each block type (paragraph / H1-H3 / bullet / ordered /
task / code / quote), drag-reorder. All commands fire correctly.
• `make check` clean (golangci-lint + go test + npm run build +
svelte-check, 0 errors).
• Both `tsc --noEmit` and `svelte-check` return 0 errors after fix.
Closes dependabot/npm_and_yarn/web/typescript-6.0.3 (PR #213).
|
||
|
xarmian
|
3ec3d6ec17 |
chore(web)(deps): bump marked 17.0.5 → 18.0.3 + drop @types/marked (TASK-1237) (#441)
v18.0.0 breaking changes are limited to (1) trimming trailing blank
lines from block tokens and (2) bumping the bundled-types compiler to
TypeScript 6. Our app calls `marked(content)` and overrides
`renderer.link` / `renderer.image` — neither path introspects the
intermediate token tree, and svelte-check (TS 5.9) consumes the
TS 6-emitted .d.ts cleanly.
Verified:
• Render snapshot of representative content (headings, code blocks,
tables, lists, task lists, blockquotes, links, images, autolinks,
strikethrough, multi-paragraph, escaped HTML) is byte-identical
between v17.0.5 and v18.0.3 — same 1847 bytes, zero diff lines.
• `make check` (golangci-lint + go test + npm run build + svelte-check)
passes with 0 errors.
• Manual UI spot-check: item body, wiki-links, code blocks, tables,
attachments, and the share-page route all render correctly.
Also drops @types/marked@5.0.2 — marked has shipped its own bundled
.d.ts since v9, so this devDep has been redundant and 12 majors stale.
Removing it eliminates type-resolution drift against the bundled types.
Mermaid retains its nested marked@16.4.2 — no transitive ripple. Closes
dependabot/npm_and_yarn/web/marked-18.0.2 (PR #212).
|
||
|
xarmian
|
a1e7378d8e |
chore(web)(deps): bump diff 8.0.3 → 9.0.0 (TASK-1239) (#440)
v9.0.0 breaking changes are confined to patch parse/format functions (parsePatch, formatPatch, reversePatch, StructuredPatch). The only in-tree consumer is web/src/lib/components/versions/DiffView.svelte, which uses diffLines + the Change type — both unchanged in v9. ES5 support is dropped, which is irrelevant for our Vite/SvelteKit target. Verified via runtime smoke-test that diffLines() in v9 returns the same Change shape (value: string, added/removed: boolean) the DiffView consumer expects. Closes dependabot/npm_and_yarn/web/diff-9.0.0 (PR #214). |
||
|
dependabot[bot]
|
5026aa529d |
chore(ci)(deps): bump docker/setup-buildx-action from 3.12.0 to 4.0.0 (#207)
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 3.12.0 to 4.0.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](https://github.com/docker/setup-buildx-action/compare/8d2750c68a42422c14e847fe6c8ac0403b4cbd6f...4d04d5d9486b7bd6fa91e7baf45bbb4f8b9deedd) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
b780c2a1fe |
chore(ci)(deps): bump docker/login-action from 3.7.0 to 4.1.0 (#206)
Bumps [docker/login-action](https://github.com/docker/login-action) from 3.7.0 to 4.1.0. - [Release notes](https://github.com/docker/login-action/releases) - [Commits](https://github.com/docker/login-action/compare/c94ce9fb468520275223c153574b00df6fe4bcc9...4907a6ddec9925e35a0a9e82d7399ccc52663121) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 4.1.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |
||
|
dependabot[bot]
|
90dcc3d4ce |
chore(ci)(deps): bump actions/checkout from 4.3.1 to 6.0.2 (#204)
Bumps [actions/checkout](https://github.com/actions/checkout) from 4.3.1 to 6.0.2. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/34e114876b0b11c390a56381ad16ebd13914f8d5...de0fac2e4500dabe0009e67214ff5f5447ce83dd) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: 6.0.2 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> |