Collaborative script editing: read-only lifecycle and browse path (Phase 3, Step 2)

[Tim020]

Summary

Step 2 of Phase 3 of the collaborative script editing rebuild (issue #889; Step 1 is merged as 42d7844). Read-only lifecycle: with collaborative_script_editing on, an editor opens the shared draft and browses it. Editing itself arrives in Step 3. The classic ScriptEditor.vue is untouched — a new CollabScriptEditor.vue is chosen instead by a store getter, so nothing changes for anyone while the setting stays off (default, and still hidden from the UI).

Design/rationale: plans/COLLABORATIVE_EDITING_V3_PLAN.md (gitignored — ask if you want it pasted).

What's new

• ConfigScript.vue renders CollabScriptEditor.vue when systemStore.isCollabScriptEditing is true, ScriptEditor.vue otherwise.
• Join follows the server's editors list containing our own id (the server commits is_editor before broadcasting GET_SCRIPT_CONFIG_STATUS, so this is also the trigger on mount for a client that's already an editor).
• Failure handling: an editor whose room is gone (rejected join, ROOM_CLOSED) gets an alert with Retry instead of a dead end. requestEditFailure now shows the server's actual reason instead of a fixed string.
• Reads: lines come from the draft's per-page snapshots through draftAdapter, rendered with the existing ScriptLineViewer (read-only), keyed by _uid — never index, never _id — so a save's id-patch or another editor's insert never remounts a row. Non-editors keep reading over REST exactly as today.
• Navigation (D13): Next Page and Go to Page stop at the last page that actually exists (the server's empty trailing page); a client never creates a page.
• Cuts is disabled for now (REQUEST_SCRIPT_CUTS cutover is Step 5).
• New useDraftPages composable: page numbers, max page, a page's lines, and the previous line across a page boundary.

Found while writing this step's E2E — not a regression, filed separately

Reloading the page while editing does not currently keep is_editor server-side. Traced to shared WS session-lifecycle code (REFRESH_CLIENT reads the wrong Session row; on_close deletes the old row, usually before the new connection can resume it) that predates this phase and had zero test coverage anywhere — genuinely never exercised before this step's spec reloaded mid-edit. No draft content is lost (the room rebuilds from its checkpoint on the next join); the user just sees "Edit" again and needs to click it to resume. Filed as #1419 and background-investigated: confirmed empirically via two real-WebSocket integration tests, which also found a second, independent race the initial write-up hadn't identified. Correctly left unfixed pending a real design decision (a resumable-trace mechanism, not a one-line patch) rather than a guessed change. CollabScriptEditor.vue's reload-rejoin code path is already written for the fixed behaviour and needs no client change once #1419 lands.

Also fixed two pre-existing E2E locator collisions surfaced by the new spec (not new duplication): button:has-text("Edit") also matches the Stage Direction Styles sub-tab's own Edit button; #page-input isn't unique app-wide since App.vue mounts its own hidden global "Go to Page" modal on every route. Both now scope to the element that's actually open/visible, matching the pattern spec 10 already uses for the classic editor's identical modal.

Test plan

• client-v3: 231 Vitest (+22 for this step — composable, store, and a component test suite that includes a mutation check confirming an index-based key would have failed the _uid test), tsc --noEmit, eslint, prettier, npm run build: clean.
• Full Playwright E2E, chromium and firefox as separate runs, after merging the latest dev (which includes the Electron removal and the Flaky Firefox E2E: 03-system-config 'can configure RBAC permissions for testuser' fails first attempt #1412 Firefox-flake fix): 229/229 in both.
• Legacy specs 01–14 (setting off) unaffected.

Notes for reviewers

• No server changes in this step.
• requestEditFailure's new optional reason param is a compatible change to a shared action; harmless for the legacy editor, which doesn't pass one.
• WS reconnect loses session state on reload: is_editor/is_cutting and live-show leadership (on_close finalises before REFRESH_CLIENT can reclaim) #1419 (session-continuity gap) and Flaky Firefox E2E: 03-system-config 'can configure RBAC permissions for testuser' fails first attempt #1412-adjacent flake investigation are both tracked separately and don't block this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[github-actions]

Client V3 Test Results

243 tests   243 ✅  1s ⏱️
 17 suites    0 💤
  1 files      0 ❌

Results for commit 86b268f.

♻️ This comment has been updated with latest results.

[github-actions]

Client Test Results

95 tests   95 ✅  0s ⏱️
 6 suites   0 💤
 1 files     0 ❌

Results for commit 86b268f.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

  1 files    1 suites   49s ⏱️
934 tests 934 ✅ 0 💤 0 ❌
939 runs  939 ✅ 0 💤 0 ❌

Results for commit 86b268f.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

230 tests   230 ✅  2m 21s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit 86b268f.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

230 tests   230 ✅  2m 21s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit 86b268f.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head reviewed: 526bdb2d5ef541a82051baba38b280e519125bc4

Verdict: changes needed. One lifecycle bug on Stop Editing (the server never closes or checkpoints the room) and one stuck-spinner failure path should be fixed before merge. The rest is test hardening and comment accuracy.

Previous round follow-up

First round: no earlier threads.

SonarQube

Source: the SonarCloud check run and bot comment on this PR. Quality Gate passed: 0 new issues, 0 accepted, 0 security hotspots, 0.0% duplication on new code. Coverage on new code shows 0.0% but passes, so the gate appears not to enforce coverage for client-v3. Nothing is needed to pass.

CI note

E2E Tests (firefox) failed, but not because of this PR. The failure is in 13-live-show.spec.ts ("can add an individual cue from the live view"), a known leader/follower race fixed by #1425, which was merged to dev after this branch was pushed. Because the suite is serial, the new spec 15 was skipped on Firefox in CI, so it has not yet run in Firefox CI. Merging dev and re-running should give a real Firefox result for spec 15.

New findings

• Critical (1): stopEditing sends LEAVE before STOP, so the server skips checkpoint and close_active_room for the last editor. It also flashes the "Could not open the script draft" alert.
• Important (6):
  • A failed initial yjsSync apply (or a join that never gets an answer) hangs on the spinner with Retry dead.
  • requestEdit ignores sendObj's false return.
  • The e2e afterAll can't restore the setting after a mid-edit failure, and its comment is inaccurate.
  • The reload e2e test pins #1419 and doesn't check "same draft".
  • The two-browser e2e test doesn't prove the browsers share a draft.
  • The unit tests don't assert the leave message or drive the real failure handlers.
• Suggestion (5): silent Go to Page failures; "Step 2" comment rot; previousLineOf allocating per render; brittle Prev/Escape e2e steps plus the "buttonless" comment; the incomplete reason list in requestEditFailure.

Not anchored inline:

• No user docs were added under docs/. That's fine while the setting stays hide_from_ui, but it will need docs and screenshots before it's exposed.
• onMounted has no try/catch around its loads, so a network-level fetch rejection leaves the loading spinner up forever. This is minor because the store loaders already handle non-OK responses.

Strengths

• Choosing the editor with a store getter keeps ScriptEditor.vue untouched; the only change the classic editor sees is the more helpful requestEditFailure reason.
• Rows are keyed by _uid, and a unit test proves an index key would have failed. That's the right call ahead of Step 3's id-patching.
• Double joins are guarded: every join path goes through retryJoin(), which checks isDraftActive first, so the watcher and onMounted can't both join.
• Next Page and Go to Page stop at the server's trailing page instead of letting a client create a page.
• The #1419 write-up in the PR and the inline comment accurately describe the server behaviour; both were verified against ws_controller.py.

🤖 Generated with Claude Code

[Tim020]

Critical: Stop Editing leaves the room before telling the server, so the server never checkpoints or closes the room.

leaveScriptRoom() sends LEAVE_SCRIPT_ROOM first. The server's handler (ws_controller.py, LEAVE_SCRIPT_ROOM) only calls room.remove_client(self). When STOP_SCRIPT_EDIT arrives next, room_manager.get_room_for_client(self) returns None, because get_room_for_client only matches clients still in room.clients. So this whole block is skipped:

if not room.has_editors:
    if room._dirty: await room_manager._checkpoint_room(room); ... GET_SCRIPT_REVISIONS
    await room_manager.close_active_room()

Scenario: A and B edit, B stops, then A (the last editor) clicks Stop Editing. The room stays open on the server with no members. The dirty state isn't checkpointed at stop time, and ROOM_CLOSED / GET_SCRIPT_REVISIONS are never broadcast. Only the periodic maintenance checkpoint protects the draft until someone rejoins. This will also matter for keeping the draft on stop in Step 4.

It also causes a visible glitch: status goes 'idle' immediately while isEditor is still true, so the red "Could not open the script draft" + Retry alert (joinFailed) flashes until GET_SCRIPT_CONFIG_STATUS arrives.

Fix: send STOP only and let the existing paths tear down locally:

• If we were the last editor, the server sends ROOM_CLOSED, which calls _teardown().
• Otherwise the isEditor watcher (L256) calls leaveScriptRoom(), which is safe when already inactive.

Also check the send result (see the requestEdit comment). This needs import { toast } from '@/js/toast'.

Suggested change

	function stopEditing(): void {
	draftStore.leaveScriptRoom();
	sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} });
	}
	function stopEditing(): void {
	// STOP first: the server only demotes, checkpoints and closes the room for a client that is
	// still in it. ROOM_CLOSED (last editor) or the isEditor watcher then tears down locally.
	if (!sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} })) {
	toast.error('Cannot stop editing: not connected to the server');
	}
	}

The unit test at CollabScriptEditor.test.ts:275 only asserts that STOP was sent. It should also assert that no LEAVE_SCRIPT_ROOM precedes it.

🤖 Generated with Claude Code

[Tim020]

Confirmed against ws_controller.py and fixed exactly as suggested, in 86b268f: stopEditing() now sends STOP_SCRIPT_EDIT only. LEAVE_SCRIPT_ROOM first really did make get_room_for_client return None on the follow-up STOP_SCRIPT_EDIT, skipping checkpoint/close/broadcast for the last editor — verified by reading both handlers. Local teardown now happens only via ROOM_CLOSED (last editor) or the isEditor watcher (another editor still present), both already in place. Also added the sendObj return check and toast you suggested.

Test added: "Stop Editing sends STOP only — never LEAVE first" asserts sentOps() equals exactly ['STOP_SCRIPT_EDIT'], which would have caught this. Two more cover the two real follow-up paths (ROOM_CLOSED, losing editor status via the config-status update).

[Tim020]

Verified: the server-side bug is fixed. stopEditing() (L248-256) sends only STOP_SCRIPT_EDIT, so get_room_for_client still finds us, and the last editor's stop now checkpoints and calls close_active_room(). The new test (sentOps() equals exactly ['STOP_SCRIPT_EDIT']) locks that in, and spec 15's afterAll now relies on it correctly.

The alert flash I mentioned is still there (my round-1 comment wrongly implied the reordering would remove it). Fixing it is optional and not a merge blocker. For the common single-editor case, the server's STOP handler does this in order:

1. It calls close_active_room(), which sends ROOM_CLOSED.
2. Then it broadcasts GET_SCRIPT_CONFIG_STATUS, which the client turns into a REST fetch.

Between the two, roomClosed() has torn down to 'idle' while isEditor is still true. So joinFailed (L181) shows the red "Could not open the script draft" alert with Retry and no reason, next to the "Collaborative draft closed" toast, until the REST round-trip lands. Your own test "ROOM_CLOSED after a successful sync also lands on Retry" asserts exactly this state. If the user clicks Retry during that window, they rejoin a room they just asked to leave.

A small local flag would fix it:

const stopRequested = ref(false);
const joinFailed = computed(
  () => loaded.value && isEditor.value && !stopRequested.value && draftStore.status === 'idle'
);
// stopEditing(): on a successful send, set stopRequested.value = true
// isEditor watcher: when it turns false, reset stopRequested.value = false

Add an assertion to "Stop Editing as the last editor" that .alert-stub does not show after roomClosed(). I'm leaving this thread open for that last piece.

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 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]

Review round 1 addressed in 86b268f; each inline comment has a reply.

Critical, fixed: stopEditing() sent LEAVE_SCRIPT_ROOM before STOP_SCRIPT_EDIT, so the server's get_room_for_client returned None on the follow-up STOP and silently skipped checkpoint/close/broadcast for the last editor. Confirmed by reading both handlers; now sends STOP only, letting ROOM_CLOSED or the isEditor watcher tear the local draft down the way the server actually behaves.

Important, all fixed:

• A failed initial yjsSync (or a join the server never answers) hung on the spinner forever. yjsSync now unwinds exactly as collabError does, and I added a 15s join watchdog.
• requestEdit (and, with the Stop Editing fix, stopEditing) now check sendObj's return value and toast on failure.
• The E2E afterAll now stops editing on both browser contexts before polling the settings PATCH, and its comments are corrected.
• The reload test now compares draft content before/after instead of only checking a row exists, and is explicitly commented as a WS reconnect loses session state on reload: is_editor/is_cutting and live-show leadership (on_close finalises before REFRESH_CLIENT can reclaim) #1419 tripwire (also noted on that issue, with instructions for whoever fixes it).
• The two-browser test now proves the browsers share a room: matching content and matching last-page, not just "each renders something."
• Unit tests now assert the actual send order for Stop Editing (would have caught the bug), the unmount LEAVE message, and drive the Retry alert through the real collabError/yjsSync-failure/roomClosed handlers.

Suggestions, all taken: Go to Page now reports invalid input and failed navigation instead of doing nothing; the "Step 2" comment is reworded so it won't go stale; the brittle Prev-Page/Escape E2E steps and the modal-comment wording are fixed; requestEditFailure's comment now covers cuts failures too. previousLineOf's per-render allocation is deliberately left alone, per the review's own framing — it matters once Step 3 makes every keystroke touch a snapshot, not for read-only browsing.

Verification: backend 934 (unchanged), client-v3 243 Vitest (+18), tsc/eslint/prettier/build clean, full Playwright green in both chromium and firefox — 230/230 each, no retries needed — after merging the latest dev (E2E port isolation from #1420, and the #1425 Firefox leader/follower fix, which is why CI's earlier firefox failure on this PR was unrelated to it).

[Tim020]

Review round 2

Head reviewed: 86b268f9fa68a35171c1059700b122de7ac0a94d. The incremental diff is from 526bdb2d. Round 1's SHA is an ancestor, so this is not a force-push. The range also includes a merge of dev (#1420, #1423, #1425), which was already reviewed on its own PRs and is not re-reviewed here.

Verdict: ready to merge. The Critical lifecycle bug and every Important finding are fixed and verified against the code. One optional UX fix is left open, and it doesn't block merging.

Previous round follow-up

• Verified fixed and resolved (10): stuck spinner and join watchdog (thread), requestEdit send check (thread), Go to Page errors (thread), "Step 2" comment (thread), afterAll ordering (thread), reload "same draft" check and #1419 tripwire (thread), two-browser shared room (thread), brittle e2e steps (thread), unit-test coverage (thread), requestEditFailure comment (thread).
• Fix incomplete (1, non-blocking): Stop Editing (thread). The server-side bug is fixed and verified: STOP-only, so the last editor's stop checkpoints and closes the room. The red "Could not open the script draft" alert still flashes when the last editor stops, because ROOM_CLOSED arrives before the GET_SCRIPT_CONFIG_STATUS refresh clears isEditor. My round-1 comment wrongly implied the reordering would remove this. The thread has a suggested fix using a stopRequested flag.
• Deferred by agreement (1): previousLineOf per-render allocation moves to Step 3 (thread).
• Still open without reply: 0. Needs human decision: 0.

SonarQube

Source: the SonarCloud check run and bot comment on 86b268f. Quality Gate passed: 0 new issues, 0 accepted, 0 security hotspots, 0.0% duplication. Nothing is needed to pass.

CI

Every check is green on 86b268f. Firefox E2E passes 230/230, so spec 15 has now actually run in Firefox. In round 1 it was skipped behind the spec-13 flake that #1425 fixed.

New findings

None. The fix commit's changes are small and well targeted. The new watchdog is armed on every join path, including the reconnect rejoin, and is cleared on sync and on teardown.

Strengths

• The fixes follow the store's existing patterns: the join watchdog mirrors the save watchdog, and the failed-sync unwind mirrors collabError.
• The regression tests target the exact failure modes, e.g. sentOps() equals ['STOP_SCRIPT_EDIT'] and a failed sync lands on Retry, and they drive the real handlers instead of _teardown().
• The #1419 tripwire is clearly labelled, with instructions for inverting it once #1419 is fixed.

🤖 Generated with Claude Code