Collaborative script editing: Vue 3 Yjs foundation (Phase 2)

[Tim020]

Summary

Phase 2 of the collaborative script editing rebuild (issue #889), following Phase 1's backend port (#1359, merged to dev). Full design/rationale: plans/COLLABORATIVE_EDITING_V3_PLAN.md (gitignored — ask if you want it pasted).

This phase builds the isolated Vue 3 / Yjs foundation client-v3 needs before any editor UI wires into it. Per the plan's exit criteria: a Y.Doc can be joined, synced, and observed reactively — no script editor UI wired up yet. That's deliberate, not incomplete — wiring it into ScriptEditor.vue is Phase 3.

New files under client-v3/src/:

• js/yjs/base64.ts — base64⇄Uint8Array for the WS protocol's binary-in-JSON transport
• js/yjs/ScriptDocProvider.ts — thin wrapper over a caller-supplied send function; applies incoming YJS_SYNC/YJS_UPDATE with a SERVER_ORIGIN echo guard so server-applied updates are never re-sent back to the server (a save's ID-patch broadcast goes to all clients including the saver — without this guard that loops)
• js/yjs/yjsSnapshot.ts — one-way Y.Doc → plain-object snapshot builder for read paths
• stores/scriptDraft.ts — new Pinia store; the Y.Doc/provider live in a plain module-level holder entirely outside state(), so nothing needs markRaw — Pinia only wraps what state() returns
• composables/useScriptDraft.ts — join/leave lifecycle via onMounted/onBeforeUnmount, with a join refcount so multiple simultaneous consumers share one room

All 9 collab WS actions are camelCase methods on scriptDraft.ts, picked up automatically by useWebSocket's existing dispatch convention (no changes to that file). GET_SCRIPT_CONFIG_STATUS/REQUEST_EDIT_FAILURE stay owned by scriptConfig.ts (extended to keep editors/cutters/hasDraft instead of discarding them) — dispatch is first-match-wins across every instantiated store, so scriptDraft.ts exposes editors/cutters as getters reading scriptConfig rather than duplicating the fetch.

Bugs caught by this phase's own tests, pre-push

• parseDbId used parseInt, which reads a numeric prefix out of a UUID like "3fa85f64-..." and misreports a brand-new line as DB id 3. Fixed with Number(), matching the server's int(float(str(line_id))).
• The snapshot refresh was pages-only (observeDeep on the pages map), but deleted_line_ids/meta are separate top-level shared types a pages-only observer can't see. Switched to a doc-wide doc.on('update', ...) listener.
• Two same-store naming collisions between a state field and a WS-dispatched action sharing a name (saveProgress, collabError) — Pinia silently breaks on this and tsc doesn't catch it, only Vitest did. Renamed the state fields (pageSaveProgress, lastCollabError).

Automated review response (2026-09-08)

A pr-review-toolkit bot review found 4 Critical + 8 Important issues — every single one checked out as real on verification (unlike prior rounds on this feature, no false positives this time). Full ledger in plans/collab_v3_pr1366_review_response.md (gitignored — ask if you want it pasted). Fixed:

• draftYdoc was a broken Pinia getter. It read a plain module-level variable with no reactive dependency, so as a computed() it cached its first value forever — verified empirically with a throwaway test before fixing. Converted to a plain action getDraftYdoc().
• The module-level Y.Doc holder could leak across store instances. The re-entry guard checked per-instance state, not the actual shared resource. Fixed with a 3-case guard — explicitly not the reviewer's own suggested "always tear down and rejoin," which would have discarded un-checkpointed edits.
• A rejected join left the store stuck "active" forever, since the error handler never rolled back the optimistic state joinScriptRoom sets. Now tears down, but only when genuinely mid-join.
• A corrupt sync payload was reported as "synced" anyway. Apply methods now return success/failure; the store only marks itself synced on genuine success.
• Plus: a join refcount for useScriptDraft.ts (two simultaneous consumers previously meant one's unmount could kill the room for both), useScriptDraft.ts going from 0 to 6 tests, a dirty-flag race where an edit made during a save could be silently marked saved and lost, and two of this phase's own claimed "regression tests" (doc-wide listener, save-ordering) turning out not to actually pin what they claimed — both now do.

I also corrected two overclaims this phase's own commit/plan doc made before this review landed, rather than leave them standing.

Test plan

• 90/90 client-v3 Vitest passing (was 51 → 75 → 90 across the two rounds above)
• tsc --noEmit / eslint: clean
• npm run build: succeeds; yjs is a new dependency but nothing in the entry graph imports these files yet, so it tree-shakes out of the production build entirely (bundle size unchanged) — expected per this phase's exit criteria, not an oversight
• Full chromium E2E suite (221/221) as an inert-change confidence check; firefox E2E green via CI
• Full backend suite — not re-run since no backend files changed this phase; will run before Phase 3 lands actual UI wiring

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[github-actions]

Client V3 Test Results

90 tests   90 ✅  0s ⏱️
 8 suites   0 💤
 1 files     0 ❌

Results for commit ac5c6e0.

♻️ This comment has been updated with latest results.

[github-actions]

Client Test Results

132 tests   132 ✅  0s ⏱️
  7 suites    0 💤
  1 files      0 ❌

Results for commit ac5c6e0.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

  1 files    1 suites   2m 20s ⏱️
863 tests 863 ✅ 0 💤 0 ❌
868 runs  868 ✅ 0 💤 0 ❌

Results for commit ac5c6e0.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

221 tests   221 ✅  2m 11s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit ac5c6e0.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

221 tests   221 ✅  2m 18s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit ac5c6e0.

♻️ This comment has been updated with latest results.

[Tim020]

🤖 Automated PR Review (Claude Code)

This is an automated review generated by Claude Code's pr-review-toolkit — five specialized review agents (general code review, test coverage, comments, silent failures, type design) independently analyzed this PR's diff and cross-checked claims against the merged backend. Findings below are aggregated and de-duplicated across agents.

Critical Issues

1. draftYdoc getter caches its value forever — client-v3/src/stores/scriptDraft.ts:43
draftYdoc: () => ydoc is a Pinia option-store getter (a computed()) over a plain module-level let with no reactive dependency. It evaluates once and never re-runs. Verified empirically: three reads across a join/clear lifecycle on one store instance return the first value each time — null before any join, then a stale/destroyed doc forever after. The doc comment right above it ("Never store this — read it fresh") describes exactly the usage this bug breaks. Existing tests pass only because none reads draftYdoc both before and after a lifecycle transition on the same instance.
   Fix: make it an action/plain function (getYdoc(): Y.Doc | null { return ydoc }), not a getter.

2. The module-level ydoc/provider/unsubscribeDocUpdate holder is a genuine global leak, not a per-store singleton — client-v3/src/stores/scriptDraft.ts:23-25
   These are module-scope, so they're shared across every useScriptDraftStore() call/Pinia instance — confirmed by the test file's own comment and its unconditional afterEach teardown workaround. The re-entry guard in joinScriptRoom (line ~59) checks isDraftActive, which is per-instance state, while the resource it's guarding is global — a new Pinia instance (HMR, remount, a test that skips teardown) can see isDraftActive === false while ydoc/provider still hold a live doc, and joinScriptRoom will silently overwrite them without calling destroy(), orphaning a live doc.on('update') listener closed over the previous instance.
   Fix: guard on the actual resource (if (ydoc) { ... } / defensive _teardown() at the top of joinScriptRoom), or better, key the holder off the store instance (e.g. a WeakMap) instead of module scope.

3. A rejected JOIN_SCRIPT_ROOM (COLLAB_ERROR) leaves isDraftActive stuck true forever — client-v3/src/stores/scriptDraft.ts (joinScriptRoom/collabError)
   joinScriptRoom optimistically sets isDraftActive = true and allocates the Y.Doc/provider before server ack. The backend can reject the join for four separate reasons (no show, no revision, live session active, room-build exception) via a generic COLLAB_ERROR. The collabError handler only sets lastCollabError/toasts — it never calls _teardown() or resets isDraftActive. Result: the store believes a draft room is active forever, isDraftSynced never becomes true (nothing will ever set it), and the orphaned doc/provider keep listening. Zero test coverage for collabError fired after joinScriptRoom().
   Fix: collabError should tear down state when it fires while isDraftActive && !isDraftSynced.

4. Corrupt/incomplete Yjs sync payloads cause silent, permanent document divergence with no recovery path — client-v3/src/js/yjs/ScriptDocProvider.ts:88-95, client-v3/src/stores/scriptDraft.ts:132-138
applyRemote's catch block only logs — it never signals failure to the caller. Worse, scriptDraft.ts's yjsSync unconditionally sets isDraftSynced = true / isDraftDirty = false whenever step === 0, regardless of whether the apply actually succeeded. A malformed initial full-sync payload leaves the UI reporting "synced" over an empty/stale doc, with no toast, no store flag, and no self-heal. Compounding this: ScriptDocProvider.requestSync() is fully implemented and unit-tested but has zero production call sites — nothing wires it into the WS reconnect path, so even a detected desync has no trigger to recover from.
   Fix: on apply failure, set a visible error flag and don't mark isDraftSynced = true; wire requestSync() into reconnect handling.

Important Issues

5. Fire-and-forget WS sends are dropped silently when the socket isn't OPEN — relies on pre-existing useWebSocket.ts sendObj, exercised by all new scriptDraft.ts actions.
   sendObj just log.warns and drops the frame if the socket isn't open — no exception, no return value. This breaks saveDraft() (sets isDraftSaving = true then may never get a server reply → permanent stuck "Saving…" spinner), joinScriptRoom() (sets isDraftActive = true with zero chance of any error ever arriving, not even COLLAB_ERROR, since the server never saw the request), and discardDraft() (silent no-op on click).

6. useScriptDraft composable has no join refcount — client-v3/src/composables/useScriptDraft.ts:28-34
   Two components mounting the composable simultaneously (Phase 3's planned editor + presence panel shape): the second onMounted join is warn-and-ignore, but the first onBeforeUnmount tears the room down for both.

7. useScriptDraft.ts has no tests at all — this is the actual integration surface Phase 3 will build on (mount/unmount lifecycle, double-mount behavior, onBeforeUnmount actually firing leaveScriptRoom). Highest-value gap to close before Phase 3 lands.

8. The claimed regression test for "doc-wide vs. pages-scoped listener" doesn't actually pin that fix — every test exercising the doc.on('update', ...) listener only mutates pages; none mutates deleted_line_ids/meta alone. Reverting to a pages-scoped observeDeep would still pass all current tests.

9. The save-ordering "regression test" is tautological — it calls yjsUpdate then scriptSaved in the literal order the test author chose, rather than exercising anything that could deliver them differently. It also doesn't cover the actually risky case: a local edit made by the user after saveDraft() fires but before scriptSaved arrives — under the current unconditional-clear logic in scriptSaved, that edit's dirty flag would be silently wiped.

10. yjsSync/yjsUpdate silently no-op with zero logging when provider is null (e.g., a message arriving in the race window around teardown) — no log.warn/log.debug, making a real occurrence undiagnosable.

11. Dead/incomplete API types — client-v3/src/types/api/scriptDraft.ts: RequestEditFailureMessage models a {reason} payload that nothing consumes (scriptConfig.ts's real handler takes no parameter and discards the server's reason for a generic toast); SaveProgressMessage is missing percent, which the server actually sends.

12. Comments citing unreachable sources — several comments in scriptDraft.ts/scriptConfig.ts/ScriptDocProvider.ts point to plans/COLLABORATIVE_EDITING_V3_PLAN.md (gitignored, not in the repo for any other reader) or, in two spots, a private local Claude Code memory file (feedback_v3_ws_dispatch) that exists only on the author's machine. The WHY content of these comments is otherwise accurate and worth keeping — just drop the dead pointer.

Suggestions

• ScriptDocProvider.ts:13 — SERVER_ORIGIN = 'server' is a bare string sentinel; a Symbol would prevent accidental collision with any other code using 'server' as a transaction origin.
• ScriptDocProvider has no internal state machine — nothing prevents calling sendAwareness()/applyUpdate() before join() or after destroy(). A destroyed flag with early-return/throw would fail loudly instead of silently operating on a torn-down doc.
• YjsSyncMessage.step: number could be narrowed to a literal union (0 | 1 | 2, or split incoming/outgoing) to catch the receive/send direction asymmetry at compile time instead of via a runtime log.warn.
• _teardown doesn't clear draftLastSavedAt, so switching revisions can briefly show revision A's "last saved" timestamp under revision B.
• yjsSnapshot.ts reads off Y.Map<unknown> via unchecked as number/as string casts — a malformed doc produces a silent undefined/NaN rather than a loud fallback. SnapshotLine/SnapshotLinePart could also be marked Readonly<> to make the documented "never write back" contract compiler-enforced.
• scriptConfig.ts's new editors/cutters/hasDraft fields (including their ?? []/?? false fallbacks) have zero test coverage.
• Minor test duplication: two parseDbId UUID test cases in yjsSnapshot.test.ts assert the identical input/expectation.
• Redundant comment in scriptDraft.ts (saveProgress handler) restates the same convention already stated generically a few lines above.

Strengths

• The echo-guard mechanism (SERVER_ORIGIN sentinel preventing server-applied updates from being re-sent) is sound and verified correct against Yjs transaction-origin semantics.
• All claimed Pinia naming-collision fixes (saveProgress→pageSaveProgress, collabError→lastCollabError) were verified with no remaining collisions anywhere in client-v3/src/stores/*.ts.
• The parseDbId parseInt→Number fix is correct and is the one claimed bugfix with a real regression test that would fail if reverted.
• The server contract (camelCase editors/cutters/hasDraft, role: 'editor' | 'viewer') was cross-checked against server/controllers/api/v1/show/script/config.py and ws_controller.py and matches exactly.
• Test style is genuinely behavioral: using a real second Y.Doc to simulate remote peers rather than mocking Yjs internals, and asserting on send calls / doc state rather than internal wiring.
• Comments are consistently WHY-not-WHAT and (aside from the dead references noted above) accurate against the backend they describe — better invariant documentation than most PRs at this stage.
• Nothing from this phase is wired into any route/component yet, consistent with the PR's stated exit criteria; E2E coverage is correctly absent for this reason.

Recommended Action

1. Fix items 1–4 (Critical) before merge — items 1 and 3 in particular mean the phase's stated exit criterion ("a Y.Doc can be joined, synced, and observed reactively") doesn't actually hold up across a full lifecycle or an error path yet.
2. Address items 5–10 (Important), especially the useScriptDraft.ts test gap, before Phase 3 wires real UI into this foundation — several of these become much harder to debug once there's a live editor on top.
3. Consider the Suggestions opportunistically.
4. Re-run this review after fixes to confirm.

---

Generated by Claude Code's PR review toolkit (5 parallel specialized review agents). Not a human review — please verify findings before acting on them.

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Thanks — this was a much sharper review than the earlier round on this feature. Went through all 4 Critical and 8 Important findings; every one checked out as real once verified against the code, no false positives this time. Fixed in ac5c6e0:

Fixed (verified real):

• DIGI-1: Add basic server app #1 (draftYdoc caches forever): Confirmed empirically before fixing it — wrote a throwaway test (a bare defineStore with a getter over a plain module variable, mutate, re-read) and reproduced the stale-cache behavior first. A Pinia getter is a computed(); reading a non-reactive module variable gives it zero dependencies to track, so it evaluates once and never re-runs. Converted to a plain action getDraftYdoc(). Confirmed editors/cutters are not affected — they read real reactive Pinia state (useScriptConfigStore()), so those getters track correctly.
• DIGI-2: Add basic client boilerplate #2 (module-level holder leak): Real, but I didn't take the suggested fix as-is — "always tear down and rejoin" would silently discard un-checkpointed CRDT state on a genuine duplicate call, and conflicts with the refcount added for DIGI-6: Add logging to server #6. Landed a narrower 3-case guard: duplicate call while active → ignore (unchanged); a doc exists but this instance isn't marked active (the actual leak scenario) → tear down and retry; nothing yet → normal path.
• DIGI-3: Add GitHub labeler action #3 (stuck-active on rejected join): collabError now tears down, but only when isDraftActive && !isDraftSynced — narrowly targeting "the join itself failed," so a post-join error (e.g. an edit rejected for permissions) doesn't tear down an otherwise-healthy room.
• DIGI-4: Add node CV action #4 (corrupt sync payload reported as synced): applySync/applyUpdate now return success booleans; yjsSync only marks synced on success. requestSync()'s reconnect wiring stays deferred to Phase 5 as the plan already scopes it there — wiring it now without that phase's reconnection design would be premature, not a rejection of the finding.
• DIGI-5: Update readme with setup instructions #5 (fire-and-forget sends): Landed the minimal fix — bail early with a toast when the socket isn't connected. The full stall-watchdog is already explicitly Phase 3 scope in the plan doc (SAVE_SCRIPT_DRAFT with a stall watchdog), so didn't pull that forward.
• DIGI-6: Add logging to server #6 (no join refcount): Added one in useScriptDraft.ts — join on 0→1, leave on 1→0.
• Revert "DIGI-6: Add logging to server" #7 (zero tests on useScriptDraft.ts): Added 6, using @vue/test-utils.
• DIGI-6: Add logging to server #8 (doc-wide listener fix wasn't pinned): Correct — every existing test only mutated pages. Added a test mutating deleted_line_ids alone.
• DIGI-7: Add file based config parsing #9 (save-ordering test was tautological, missed the real risk): Agreed this was the most substantive finding. Fixed the actual race: a local edit landing between saveDraft() and scriptSaved now correctly keeps isDraftDirty true instead of being silently wiped, tracked via a local-edit counter that ignores server-originated updates (the save's own ID-patch broadcast).
• DIGI-8: Add basic front end routing #10, DIGI-9: Do not reroute API calls on the server side #11, DIGI-10: Add 404 page #12: All fixed as suggested — added logging for the null-provider no-op paths, added the missing percent field and removed the dead RequestEditFailureMessage type, and rewrote comments that pointed at the gitignored plan doc or a local-machine-only memory file to be self-contained.

Suggestions landed: _teardown now clears draftLastSavedAt; trimmed the duplicate parseDbId UUID test case; removed the redundant saveProgress comment.

Suggestions deferred, with reasons (Symbol sentinel, a destroyed state machine, literal-union step typing, Readonly<> snapshot types, hardening unchecked casts, test coverage for scriptConfig's passthrough fields): see the ledger doc for why each isn't worth the churn right now.

Also corrected two overclaims my own Phase 2 commit/plan-doc text made before this review — I'd described the doc-wide-listener and save-ordering fixes as "verified"/"locked in with a regression test" when neither test actually exercised the failing scenario. Walked the plan doc back to describe what was actually true at each point.

90/90 Vitest passing (was 75), tsc/eslint clean, build still tree-shakes to zero bytes (nothing wired in yet). Full ledger: plans/collab_v3_pr1366_review_response.md.