[Tim020]

Summary

Phase 3, Step 4 of the client-v3-only collaborative script editing rebuild (issue #889): save, stop-with-choice, discard, autosave, and bulk edit for CollabScriptEditor.vue, building on Step 3's line/text editing (#1479, merged).

• Save: canSave/saveDisabledReason mirror server validation client-side (D1) so Save disables itself with a specific reason before the round trip. A SaveProgressModal.vue (extracted and shared with the classic editor) shows progress/result.
• Stop Editing: stops immediately when nothing is unsaved; otherwise offers Keep Draft / Discard / Keep Editing (D9), warning when other editors are present and will lose their work on Discard.
• Autosave: a new useAutoSave composable (also shared with the classic editor), lighter-touch than classic's blocking modal — silently skips a tick if the snapshot currently fails validation, per Tim's decision.
• D8 fix: scriptDraft.ts's canSaveDraft now covers isDraftDirty || scriptConfigStore.hasDraft, fixing a rejoin bug where resuming a kept draft left Save stuck disabled until a fresh edit.
• Bulk edit: reuses BulkEditModal.vue as-is; selection is stored as {page, lineUid} (D6), resolved to a position only at apply time, aborting with a toast if a boundary line vanished; the whole range-apply is one reentrant doc.transact(..., LOCAL_EDIT_ORIGIN).
• Branded LineUid/PartUid types throughout the draft-writing stack.

Two real backend bugs found and fixed by the new E2E coverage (the first time this suite exercises Save/Stop/Discard/bulk edit through the UI):

• _op_save_script_draft never broadcast GET_SCRIPT_CONFIG_STATUS after a successful save, so the "Unsaved draft" indicator never cleared client-side.
• discard_room/discard_active_room deleted the draft, then immediately re-checkpointed the still-dirty in-memory doc straight back to a new draft file — silently undoing the discard. Fixed with room.mark_checkpointed() before close_active_room().

Both backend fixes have their own regression tests, verified via mutation testing (temporarily reverted the fix, confirmed the new test fails, restored).

Test plan

• Vitest: 360/360 passing (client-v3), including new coverage for Save/Stop/Discard/autosave and bulk edit, each mutation-tested
• Backend pytest: 1275/1275 passing, including new regression tests for both bugs found
• Playwright E2E: 243/243 passing on both chromium and firefox, including new Step 4 coverage (save commits to DB, D8 resumed-draft proof, bulk edit, discard closes the room for every editor in it)
• typecheck and lint clean (client-v3); ruff check/ruff format --check clean (server)

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[github-actions]

Client Test Results

102 tests   102 ✅  0s ⏱️
  9 suites    0 💤
  1 files      0 ❌

Results for commit b803c7f.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

375 tests   375 ✅  3s ⏱️
 29 suites    0 💤
  1 files      0 ❌

Results for commit b803c7f.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 29s ⏱️
1 281 tests 1 281 ✅ 0 💤 0 ❌
1 312 runs  1 312 ✅ 0 💤 0 ❌

Results for commit b803c7f.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

243 tests   243 ✅  2m 34s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit b803c7f.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

243 tests   243 ✅  2m 24s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit b803c7f.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Reviewed at head SHA 08cfc4d22932e1a22b4585a3ee968918d8d8e278.

Verdict: changes needed. The SonarCloud quality gate is failing and there are several real correctness bugs in the new Save/Stop/Discard and bulk-edit code, concentrated around error handling on failure/race paths rather than the happy path (which is well covered by the new tests).

SonarQube

Quality gate: FAILED — one condition out of five fails:

Metric	Actual	Required	Result
New duplicated lines density	4.64%	≤ 3%	FAIL
New reliability/security/maintainability rating	A	≤ A	pass
New security hotspots reviewed	100%	100%	pass

All 89 duplicated new-code lines are in 3 files, 6 blocks:

• CollabScriptEditor.vue:744-764 ⇄ ScriptEditor.vue:744-764 (20 lines) — the autosave-wiring watch/onMounted boilerplate; worth a shared useScriptEditorSetup composable, but note only ~5 of these lines are actually new in this PR (the rest is pre-existing context in both files), so this block isn't independently fixable without touching unrelated code.
• CollabScriptEditor.test.ts (69 of the 89 lines, self-duplicated across the Stop-Editing and bulk-edit test blocks) — extracting shared setup helpers here alone brings density to roughly 1.0%, comfortably under the 3% gate.
• e2e/tests/15-collaborative-script-editing.spec.ts:225-242 ⇄ 308-325 (shared "add dialogue line" sequence) — candidate for a new e2e/helpers.ts helper.

Fixing the test file is the highest-leverage single change for the gate. 4 Sonar-flagged code issues are posted inline above (2× typescript:S3776 cognitive complexity, 1× typescript:S1128 unused import, 1× python:S7504 redundant list()). No security hotspots were found (0 returned, not an access failure).

New findings (9 inline comments)

• Important (4): unguarded DB commit in close_active_room()'s new demote block that can leave a room wedged and a discarding client with no feedback; no lock between save_room/discard_room/discard_active_room (a real race, not just a test gap); discardAndStop() silently no-ops from the join-failed/idle state; the save-progress modal can show "Finished saving script." when the save never actually sent.
• Suggestions (5): two SonarQube cognitive-complexity flags (onBulkApply, validateDialogue), onBulkApply reaching past useDraftWriter's documented encapsulation with its docstrings not updated to say so, a stale "bulk edit not yet implemented" line left in a docstring this PR itself edited, a non-discriminated-union validation result type, an unused import, and an unbranded line_type field that defeats exhaustiveness checking.

Pre-existing bug surfaced during review (not in this PR's diff, so not postable inline)

client-v3/src/stores/scriptDraft.ts's scriptSaved() (around line 421-439, untouched by this PR) computes wasOurSave = this.isDraftSaving. _armSaveWatchdog() (lines 302-311) independently flips isDraftSaving to false after a 30s stall timeout, regardless of whether the save actually completes later. If the real SCRIPT_SAVED for our own save arrives after the watchdog has already fired, wasOurSave reads as false, and the code takes the "someone else's save" branch, clearing isDraftDirty unconditionally even if edits were made during the stall window — those edits would show as "saved" without actually being covered by the save that completed. Worth a follow-up: give each save request its own identity (a token/counter) instead of reusing isDraftSaving, which a timeout can mutate out from under this check. This predates Step 4, but Step 4's new Stop-Editing-disabled-during-save and Save button make the save flow more load-bearing, so it's worth fixing in this area.

Test coverage (not anchored to a diff line — flagging here)

• No test exercises the save-vs-discard race described above (would belong in server/test/utils/script/test_script_room.py, next to the existing discard/checkpoint regression tests).
• The only two-browser E2E test has the second client idle (no concurrent typing, no concurrent CRDT merge check), and the new Discard test's "second editor" has no real unsaved edits of its own — so the PR's own "other editors will lose their work too" warning is only verified as label text in a unit test, never as an actual behavior end-to-end.
• Bulk edit: no test for a line inside the range disappearing mid-apply (only the start/end boundary case is covered), and no test for an empty-selection no-op.
• No test asserts the new :disabled="draftStore.isDraftSaving" on the Stop Editing button.
• The classic ScriptEditor.vue's autosave/save-modal refactor (swapping to the shared useAutoSave/SaveProgressModal) has no dedicated unit test, and SaveProgressModal.vue has no component test of its own.

Strengths

• LineUid/PartUid branded types threaded through the whole Yjs write path are a genuine compile-time win — a PartUid can no longer be passed where a LineUid is expected, at zero runtime cost.
• Bulk edit's {page, lineUid} boundaries, resolved only at apply time and aborting cleanly if a boundary line vanished, correctly handle concurrent inserts/deletes.
• save_room()'s existing error handling (separate from the bugs flagged above) is genuinely careful — each risky step has its own narrow try/except with a specific client-facing error.
• The two real backend regressions this PR fixes (discard not reverting dirty content, save not broadcasting a config-status refresh) each get a focused, well-documented regression test.
• scriptDraftValidation.ts mirrors the server's line_type_validator.py accurately, including the one deliberate client/server difference, and is itself thoroughly tested.

🤖 Generated with Claude Code

[Tim020]

Round 1 fixes — 8d3b458

All 9 inline findings replied to in their own threads with the fix (or, for the two suggestions where I didn't take the exact approach proposed, a written reason — BulkEditModal's emit shape and ScriptLine.line_type's type, both shared with the classic editor).

Summary-level items from the review

Pre-existing wasOurSave/watchdog bug (flagged as not anchorable to this PR's diff, but fixed per project direction to fix rather than defer): scriptSaved() read isDraftSaving to tell our own save from someone else's, but the stall watchdog resets isDraftSaving on a timeout purely for the UI (so Save/Stop Editing unstick themselves) — a late reply to a save we actually sent could then be misread as someone else's, and unconditionally clear isDraftDirty. Added a separate ownSaveInFlight flag in scriptDraft.ts, set when saveDraft() actually sends and cleared only by scriptSaved()/saveError() resolving it — never by the watchdog. New test: scriptDraft.test.ts's "keeps isDraftDirty true even if the stall watchdog fires before a late scriptSaved for our own save arrives", mutation-tested.

SonarCloud duplication gate: extracted mountAsEditor()/mountSyncedDraft() helpers in CollabScriptEditor.test.ts, removing the ~4-line becomeEditor(); vi.spyOn(...); const w = await mountEditor(); syncDraft(...); prefix that was repeated across ~20 tests in the save/stop/discard block (and hoisted mountAsEditor() to file scope so the bulk-edit block's own helper reuses it too, instead of repeating its own copy of the same 3 lines). Added addDialogueLine() to e2e/helpers.ts for the duplicated add-dialogue-line sequence flagged in spec 15, used at both call sites.

Additional coverage flagged as missing but not strictly required: a "Stop Editing is disabled while saving" test, a new SaveProgressModal.test.ts (4 tests — progress text, success/error states, exposed show/hide), two new bulk-edit tests (an empty selection is a no-op; a line inside the range — not a boundary — vanishing before apply still applies correctly to the surviving boundary lines), and the E2E Discard test now has the second editor make a real unsaved edit of its own before the discard, asserting it's gone on both clients afterward — actually exercising the "other editors will lose their work too" warning end-to-end, not just as label text in a unit test.

Verification

• Backend: 1280/1280 pytest passing, ruff check/ruff format --check clean (whole repo)
• Frontend: 374/374 Vitest passing, typecheck/lint clean
• E2E: 243/243 passing on both chromium and firefox
• Every fix (backend and frontend) has a new or updated regression test, each verified by mutation testing: revert the fix, confirm the test fails, restore.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[Tim020]

Review round 2

Reviewed at head SHA 8d3b4580bde4e367d9aafa3c0e188dc2fd948ab3.

Verdict: changes needed (minor). This is a strong fix commit — 8 of the 9 round-1 findings are fully and correctly resolved, with real regression tests for each, and the SonarCloud gate now passes. One new, non-blocking correctness bug was introduced by the fix for finding #6, flagged below.

Previous round follow-up

• Verified fixed (7): close_active_room's unguarded demote block now wrapped in try/except, with the redundant list() (Sonar S7504) dropped too; no lock between save/discard/close now serialized on _room_lock via a clean locked/unlocked method split that avoids deadlocking the non-reentrant lock; discardAndStop() silent no-op from idle; save modal false-success; stale "bulk edit not yet implemented" docstring; non-discriminated-union LineValidationResult; unused parseDbId import.
• Verified fixed with a reasoned alternative (1): validateDialogue's complexity split — the author declined the exact LineType narrowing suggested (would ripple into the classic editor's shared type) but changed the default: branch to reject rather than silently accept an unknown line type, which addresses the actual risk. Accepted.
• Fix incomplete (1): onBulkApply's Sonar complexity — the encapsulation-boundary fix (useDraftWriter.transact()) is fully correct and well-documented, but cognitive complexity is still 16 vs. the 15 allowed (down from 20). Doesn't block the gate; left open.
• Needs human decision (1): BulkEditModal's emit shape was left as five independently-nullable fields rather than the typed union suggested, with the stated reason that it's shared with the classic editor and out of this PR's scope — reasonable, but a project-direction call rather than something to adjudicate in review.

SonarQube

Quality gate: PASSED (was failing in round 1 on duplication; now 2.9% vs ≤3% required — all other conditions pass). 1 new open issue, already covered above: typescript:S3776 on CollabScriptEditor.vue:692 (onBulkApply, complexity 16 vs 15), non-gate-blocking.

New findings

• Important (1): a new ownSaveInFlight leak via the COLLAB_ERROR save-rejection path, posted inline above — introduced by (and only possible because of) this round's fix for the pre-existing wasOurSave race.
• Test gap (not anchored to a diff line): the demote-block try/except fix (round-1 finding #1) has no test — nothing simulates a session.commit()/session.get() failure inside _close_active_room_locked's demote loop to confirm ROOM_CLOSED still broadcasts and the room still tears down. Worth a follow-up test in test_script_room.py.
• Minor note: the concurrency test for the new _room_lock (round-1 finding #2) lives in test_collab_editing_mode.py rather than test_script_room.py, and proves no-deadlock rather than interleaving correctness — acceptable, not a blocker.
• Minor note: save_room() now holds _room_lock for the duration of its broadcast write_message/ws_send_to_all awaits to every room client, so a slow client can delay joins, discards, closes and maintenance checkpoints for as long as that write takes. Not a bug today (rooms are small), but worth knowing if this ever needs to scale — gathering broadcast payloads under the lock and sending after release would remove it.

Strengths

• The lock refactor (_room_lock + locked/unlocked method split) is genuinely well-designed: every caller of the lock-free _close_active_room_locked() was traced and confirmed to already hold the lock, and checkpoint_if_dirty's re-check of self._room is room and room._dirty after acquiring the lock closes the narrower maintenance-checkpoint race flagged alongside the main one.
• Every fix has a real, specific regression test (7 of 9 confirmed non-superficial by independent test-coverage review), several explicitly mutation-tested per the author's own summary.
• The three "additional coverage" items volunteered beyond what was asked (Stop-Editing-disabled-during-save, bulk-edit empty-selection/mid-range-deletion, and the E2E discard test now using a real unsaved edit) are all present and genuinely exercise the scenarios, not just happy-path labels.
• Where the author disagreed with a suggestion, they gave a specific, scoped reason rather than silently diverging or over-applying the suggestion into unrelated code.

🤖 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
2.8% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Round 2 fixes — `b803c7f`

Replied inline in both threads (the new ownSaveInFlight leak via COLLAB_ERROR, and the remaining onBulkApply complexity).

Also addressed the non-inline test gap you flagged: added a test in test_script_room.py (TestRoomManagerCloseRoomDemotesEditors::test_a_db_failure_while_demoting_does_not_block_room_closed_or_teardown) that injects a session.get() failure into the demote block and confirms ROOM_CLOSED still broadcasts and the room still tears down. Mutation-tested (reverted the try/except, confirmed the test fails with the injected RuntimeError propagating, restored).

Left the three "minor note" items as-is per your own assessment (not blockers): the concurrency test living in test_collab_editing_mode.py rather than test_script_room.py, save_room() holding the lock across broadcast writes (a scaling consideration, not a bug today), and the BulkEditModal emit-shape item remains open as a project-direction call for Tim.

Verification

• Backend: 1281/1281 pytest passing, ruff check/ruff format --check clean
• Frontend: 375/375 Vitest passing, typecheck/lint clean
• E2E: 243/243 passing on both chromium and firefox
• All three round-2 fixes mutation-tested

🤖 Generated with Claude Code

https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8

[Tim020]

Review round 3

Reviewed at head SHA b803c7fb025c45f9759aaf8cc602e8a67f25aba2.

Verdict: ready to merge. Both remaining findings from round 2 are now fully and correctly fixed, with no new bugs introduced. All 11 findings raised across 3 rounds are resolved or settled. SonarCloud is fully clean.

Previous round follow-up

• Verified fixed (2): onBulkApply's remaining cognitive-complexity issue — the applyBulkEditToLine extraction preserves identical behavior (same calls, same order, same vanished-line guard), and SonarCloud now reports 0 open issues for this PR. The ownSaveInFlight leak via COLLAB_ERROR — collabError() now resolves it correctly scoped to only fire when a save was actually in flight, with a real regression test.
• Also addressed (not inline, from round 2's summary-level test gap): a new backend test (test_a_db_failure_while_demoting_does_not_block_room_closed_or_teardown) injects a DB failure into the demote block and confirms ROOM_CLOSED still broadcasts and the room still tears down — verified correct and using the project's standard test-case base class.
• Needs human decision, still open (1): BulkEditModal's emit shape (five independently-nullable fields vs. a typed union) remains a deliberate project-direction call, out of this PR's scope per the author. Not something for this review to adjudicate further.
• Noted, not a blocker (1): a narrow residual edge case in the ownSaveInFlight/collabError() fix — a same-client YJS_UPDATE apply failure arriving just after a save was sent could still clear ownSaveInFlight prematurely, causing a later real save's SCRIPT_SAVED to be misattributed. Fully closing this needs the server to send SAVE_ERROR (not COLLAB_ERROR) for _op_save_script_draft's own rejection paths. Flagged on the thread as an optional follow-up, not blocking.

SonarQube

Quality gate: PASSED. All conditions green, 0 open issues, 2.8% duplication on new code (≤3% required), 0 security hotspots. Confirmed via anonymous SonarCloud API against the exact head commit.

New findings

None. The round-3 diff (108 lines across 4 files) was reviewed in full; no new bugs found at reportable confidence.

Strengths

• Both fixes were verified not just by re-reading the diff but by checking they don't double-fire against the other code paths that also touch the same state (saveError(), scriptSaved(), _teardown()), and by confirming the new/existing tests actually pass locally.
• The author continues to volunteer regression tests beyond what was strictly asked (the DB-failure demote test from round 2's non-blocking summary note), each mutation-tested per their own process.
• Across all 3 rounds, every fix was either fully correct on the first attempt or converged to fully correct within one follow-up — no finding required more than two rounds to resolve.

🤖 Generated with Claude Code