Skip to content

fix(board): sheet @-mentions + subpages on local boards - #265

Open
winlp4ever wants to merge 6 commits into
mainfrom
fix/sheet-page-provider-local
Open

fix(board): sheet @-mentions + subpages on local boards#265
winlp4ever wants to merge 6 commits into
mainfrom
fix/sheet-page-provider-local

Conversation

@winlp4ever

Copy link
Copy Markdown
Contributor

The bug

In sheet notes, the @-mention (page reference) syntax and the /subpage syntax stopped working on local boards — no autocomplete, existing @ chips wouldn't resolve, and creating a subpage did nothing.

Root cause

createBoardPageProvider (which backs both syntaxes) was still calling the REST board APIlistBoardContents (/boards/{id}/contents), getNote (/boards/{id}/notes/{id}), addNotes. When boards became offline-first, those routes have no local-board counterpart, so apiFetch throws (src/api.ts:236). Each method fails:

  • list (autocomplete) throws → no suggestions,
  • get (chip resolution, via page-cache) → caught → null → chips can't resolve,
  • create (/subpage, "Create new") throws → nothing created.

Pre-existing, not from recent work. The provider was last touched in #100 (page references); the on-device contents path arrived in #190/#191 (the offline-first sidebar migration), which moved the sidebar to useLocalBoardContents but left the PageProvider on the dead REST path. list-board-contents.ts even notes "no component consumes this anymore." Our recent PRs never touch this file.

The fix

Migrate all three methods to the on-device replica (no call-site changes — both providers just pass boardId/parentNoteId/onNavigate):

  • listlistLocalBoardContents(boardId), filtered to sheet-kind nodes.
  • get → the live store first (current layer), else the whole-board persistence replica; maps the harness node → Page.
  • create → builds a real sheet via createDefaultNote({nodeType:"sheet"}) → noteToNode (which sets styleType, so it shows up in list), then writes sync-correctly: same layer as the view → store.addNode (renders + syncs); a subpage's child layer (always off-scene) → the S7 headless intake (record + submitLocalBatch(scene:false)), so the user's view never moves. Invalidates ["localBoardContents", boardId].

Test plan

  • noteToPage unit tests unchanged (still green).
  • New end-to-end provider tests over the on-device store: list returns sheets and excludes non-sheets; list(query) filters by title; get resolves title + snippet from the whole-board replica (and null for missing); create adds a top-level sheet in-scene with an empty body and lists it; create with a parent writes the subpage off-scene (not in the visible store) under the right parent.
  • npm run check-all + a board sweep (240 tests) pass.

Follow-up (out of scope)

Agent-created sheets (via the mutator) set node.type but not data.styleType, so they don't yet appear in listLocalBoardContents (neither the @-page picker nor the sidebar). Worth a separate small fix so agent-authored sheets are referenceable.

The sheet editor's PageProvider still called the REST board API
(listBoardContents / getNote / addNotes), but boards went offline-first — those
routes have no local-board counterpart, so apiFetch threw. Result: @ autocomplete
returned nothing, existing @ chips couldn't resolve, and /subpage created nothing.
(The sidebar migrated to the on-device store in #190/#191; the PageProvider was
missed.)

Migrate all three methods to the local replica:
- list  -> listLocalBoardContents (sheet-kind nodes)
- get   -> live store, else the whole-board persistence replica
- create -> build a real sheet (createDefaultNote + noteToNode, sets styleType so
  it lists) and write sync-correctly: same layer as the view -> the store; a
  subpage's child layer -> the S7 headless intake (record + submitLocalBatch
  scene:false), so the user's view never moves.

No call-site changes (both providers just pass boardId/parentNoteId/onNavigate).
…o dangling chip)

Address code-review findings on the on-device page provider:
- create() awaits persistence.flush() before invalidating, so an off-scene
  subpage (no store 'change' to flush-chain) is durable before the contents
  index reloads — otherwise the picker/sidebar stayed stale until an unrelated
  edit.
- writeSheetNode returns a boolean; create() throws when there's no live board
  to write into, so the editor never inserts a chip for a non-existent page.
- get() filters to sheet-kind on both the live-store and replica paths (a page
  is a sheet), matching list().
- list()/get() share a short (1.5s) per-instance whole-board cache so typeahead
  and hover cards don't replay the snapshot+oplog on every call.

Tests: off-scene create is durable + listable without a manual flush; get() on a
non-sheet returns null; create() throws with no live board.
@winlp4ever

Copy link
Copy Markdown
Contributor Author

Review response (code-review high)

All 5 findings were valid; fixed in 48e4f98:

  • feat(*): first commit #1 + style(ui): minor ui board twists #5 (off-scene create → stale picker/sidebar): create() now awaits persistence.flush() before invalidating. An off-scene subpage emits no store change, so nothing else flush-chains the refresh; flushing first makes the contents index (fresh snapshot+oplog load) see the batch. This is the same flush-guard useSidebarContentsSync uses, applied at the one spot off-scene writes need it.
  • feat(*): setup semantic versioning #2 (create returns a Page even when the write no-ops): writeSheetNode now returns a boolean; create() throws when there's no live board. page-suggestion already wraps create in try/catch, so this surfaces as "no chip inserted" rather than a dangling reference.
  • ci(github): publish github releases from workflow #3 (full replay per keystroke): list/get share a short (1.5s) per-instance whole-board cache, cleared on create. Typeahead and hover cards no longer replay the snapshot+oplog every call; get's live-store-first path already covered the current-layer common case.
  • ci(github): push release tags explicitly #4 (get resolves any node type as a page): both the live-store and replica paths now filter to sheet-kind (isSheet), matching list.

Added tests: off-scene create is durable + listable without a manual flush; get on a non-sheet returns null; create throws with no live board. check-all + provider suite (13) green.

The contents index keyed surface detection purely on the display `styleType`,
which only the convert layer (createDefaultNote -> noteToNode) sets. Agent
surfaces built through the mutator carry the canonical `node.type` but no
`styleType`, so an agent-created sheet/folder never appeared in the sidebar tree
or the @ page picker (though it rendered fine on the canvas).

Fall back to `node.type` when `styleType` is absent, in both surface readers:
- listLocalBoardContents (sidebar tree + page picker source)
- affectsSurfaceTree (live sidebar refresh on create/remove)

styleType stays a NoteNodeData (display) field, not canonical DimNodeData, so the
fix is a tolerant reader, not stamping an off-model field on agent writes.

Tests: an agent-style sheet (node.type only) is indexed by listLocalBoardContents
+ the provider list; affectsSurfaceTree fires for it.
@winlp4ever

Copy link
Copy Markdown
Contributor Author

Added: agent-authored surfaces now index correctly (dcf4c3d)

Folded in the styleType follow-up rather than deferring it.

Problem: the contents index (sidebar tree + @ page picker source) detected surfaces purely via the display styleType, which only the convert layer (createDefaultNote → noteToNode) sets. Agent surfaces built through the mutator carry the canonical node.type but no styleType, so an agent-created sheet/folder rendered on the canvas yet was invisible to the sidebar and un-referenceable.

Fix: fall back to node.type when styleType is absent, in both surface readers — listLocalBoardContents (sidebar tree + picker source) and affectsSurfaceTree (live sidebar refresh on add/remove). styleType stays a NoteNodeData (display) field, not canonical DimNodeData, so this is a tolerant reader rather than stamping an off-model field onto agent writes.

Tests added; full board sweep (248) + check-all green.

The PR now covers: @/subpage working on local boards and agent-authored sheets/folders showing up in the sidebar + @ picker.

- get() live-store lookup now runs inside the try/catch, so a malformed id
  can't reject the promise — honors the 'return null on missing' contract.
- extract titleFromData(); nodeToPage + list() share it (no duplicated
  RichText-vs-string decoding).
- affectsSurfaceTree: node.update that patches the node-level `type` (a kind
  change) now counts as tree-affecting, symmetric with the add/remove node.type
  fallback.

Skipped by design: re-adding the ['boardContents'] invalidation (dead cache —
useBoardContents has no callers; the sidebar reads ['localBoardContents']), and
the REST get/list fallback (offline-first narrowing; self-heals on materialize,
and re-adding it would break local boards).
@winlp4ever

Copy link
Copy Markdown
Contributor Author

Second review response (code-review high, post-styleType patch)

Ran a second pass since dcf4c3d (styleType fallback) + 48e4f98 (first-round fixes) landed after the initial review. Findings addressed in a12ced7:

Skipped, by design (verified):

  • feat(*): first commit #1 (dropped ['boardContents'] invalidation): that cache is deaduseBoardContents has zero callers; the sidebar reads ['localBoardContents'], which create() does invalidate. The sibling flows still poking ['boardContents'] are vestigial. Re-adding it would cargo-cult a dead key.
  • ci(github): publish github releases from workflow #3 (no REST get/list fallback): intended offline-first narrowing — a not-yet-materialized synced page self-heals once the whole-board replica lands, and re-adding REST would break local boards (the original bug).

check-all + provider/sidebar suites green.

Completes local-board sub-pages: creating one worked (#265), but OPENING it
showed 'This sheet no longer exists'. The surface host resolved a note only via
the live store (current layer) or REST — a /subpage lives off-scene in a
sub-layer, so it was in neither (not on the canvas, not on the server for a local
board), and even a successful load would have lost edits (save was store-or-REST
too).

Add useOffSceneNote (+ testable openOffSceneNoteStore core): seed the note's
layer from the whole-board replica into a throwaway store and forward its edits
to the sync intake (record + submitLocalBatch scene:false) — the surface-host
analog of the agent's HeadlessMutator. sheet-panel now resolves the note as
live-store -> off-scene replica -> REST, and points `store` at whichever holds
it, so the existing store.updateNode save path works off-scene unchanged. REST is
gated to fire only after the off-scene load settles (synced-not-materialized).
Save callbacks read prevData from the live store node so multi-edit merges don't
clobber a prior off-scene edit.

Tests: off-scene sub-page loads into an editable store; an edit records + enters
the sync intake scene:false and lands in the oplog.
…ar, caches)

- Off-scene node is now LIVE: it re-reads on the off-scene store's changes, so a
  sub-page rename / icon edit shows in the panel instead of reverting to the
  load-time snapshot (edits were saved but looked reverted).
- Surface-relevant off-scene edits invalidate the sidebar's localBoardContents
  cache (the sidebar sync only watches the live store, so it never saw them).
- Optimistic rename/icon patch now targets localBoardContents (what the sidebar
  reads) instead of the dead boardContents key.
- openOffSceneNoteStore returns a null store when the note isn't in the replica,
  so a synced-not-materialized note falls back to REST without building a doomed
  store (no wasted whole-board replay + subscription).
- Page provider get() trusts the live store only when it IS this provider's board
  (guards a sub-graph id collision).

Skipped: the 1500ms @-picker loadBoard cache staleness — an accepted trade-off
against per-keystroke oplog replay.

Tests: not-in-replica returns a null store; existing off-scene load + edit-sync
tests updated for the boardId param.
@winlp4ever

Copy link
Copy Markdown
Contributor Author

Third review response (code-review high, off-scene surface)

Findings on the surface-host commit, addressed in 1510a93:

  • feat(*): first commit #1 (snapshot node → edits visually revert): the off-scene node is now live — useOffSceneNote re-reads it on the off-scene store's changes (mirrors useNode), so a sub-page rename/icon edit shows in the panel, not just persists.
  • feat(*): setup semantic versioning #2 (off-scene edits don't refresh the sidebar): surface-relevant off-scene edits now invalidate ["localBoardContents"] (flush-chained, gated by affectsSurfaceTree) — the sidebar sync only watches the live store, so it never saw them.
  • ci(github): publish github releases from workflow #3 (optimistic patch on dead cache): rename/icon optimism now patches ["localBoardContents"] (what the sidebar reads), not the dead ["boardContents"].
  • ci(github): push release tags explicitly #4 (get fast-path cross-graph): get() trusts the live store only when it IS this provider's board (boardId === active), else it loads the replica.
  • style(ui): minor ui board twists #5 (wasted store build): openOffSceneNoteStore returns a null store when the note isn't in the replica, so a synced-not-materialized note falls back to REST without building a doomed store (no wasted whole-board replay + subscription).

Skipped, noted: #6 — the 1500ms @-picker loadBoard cache can show a just-deleted/renamed page for up to 1.5s; an accepted trade-off against per-keystroke oplog replay.

Tests updated (+ not-in-replica returns a null store). check-all + 290-test board sweep green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant