From 7b08c13baa83cc291209ddd5579510b4734afe89 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 21 Sep 2026 22:21:07 +0100 Subject: [PATCH 1/6] Collaborative script editing: read-only lifecycle and browse path (Phase 3, Step 2) With collaborative_script_editing on, ConfigScript renders a new CollabScriptEditor instead of the classic ScriptEditor (which is untouched). An editor joins the shared draft and browses it; everyone else reads saved pages over REST as before. - Join follows the server's editors list containing our own id (the server commits is_editor before broadcasting GET_SCRIPT_CONFIG_STATUS), so it also covers a reload while editing. Stop Editing leaves the room and sends STOP_SCRIPT_EDIT. - An editor with no room (rejected join, ROOM_CLOSED) gets an alert with Retry instead of a dead end. - Lines come from the store's page snapshots through the draft adapter, keyed by _uid so a save's id patch (or another editor's insert) never remounts a row. - Navigation stops at the server's empty trailing page: Next Page is disabled there and Go to Page past it is refused with a message. A remembered page past the end is clamped. - useDraftPages composable (page numbers, max page, lines, previous line across pages); typed setting and isCollabScriptEditing getter; requestEditFailure now shows the server's reason. - Cuts is disabled in collaborative mode until Step 5. Tests: 22 new Vitest (composable, store, component with a mutation check that index keys fail the _uid test) and a new Playwright spec 15 (setting driven through the API, restored in afterAll): 400 on a non-boolean, 409 while editing, open draft, last-page navigation, reload rejoin, second browser, stop editing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- client-v3/e2e/README.md | 38 +-- .../15-collaborative-script-editing.spec.ts | 138 ++++++++ .../config/script/CollabScriptEditor.test.ts | 308 +++++++++++++++++ .../show/config/script/CollabScriptEditor.vue | 315 ++++++++++++++++++ .../src/composables/useDraftPages.test.ts | 97 ++++++ client-v3/src/composables/useDraftPages.ts | 52 +++ client-v3/src/stores/scriptConfig.test.ts | 36 +- client-v3/src/stores/scriptConfig.ts | 6 +- client-v3/src/stores/system.ts | 4 + client-v3/src/types/api/settings.ts | 2 + .../src/views/show/config/ConfigScript.vue | 9 +- 11 files changed, 981 insertions(+), 24 deletions(-) create mode 100644 client-v3/e2e/tests/15-collaborative-script-editing.spec.ts create mode 100644 client-v3/src/components/show/config/script/CollabScriptEditor.test.ts create mode 100644 client-v3/src/components/show/config/script/CollabScriptEditor.vue create mode 100644 client-v3/src/composables/useDraftPages.test.ts create mode 100644 client-v3/src/composables/useDraftPages.ts diff --git a/client-v3/e2e/README.md b/client-v3/e2e/README.md index c0a47df23..94fcf4ff3 100644 --- a/client-v3/e2e/README.md +++ b/client-v3/e2e/README.md @@ -5,9 +5,9 @@ Tests run against a real backend instance with a clean SQLite database, covering ## Prerequisites -* Node 24.x with dependencies installed (`npm ci` in `client-v3/`) -* Python 3.13.x with dependencies installed (`pip install -r requirements.txt` in `server/`) -* A production build of the frontend (`npm run build` in `client-v3/`) +- Node 24.x with dependencies installed (`npm ci` in `client-v3/`) +- Python 3.13.x with dependencies installed (`pip install -r requirements.txt` in `server/`) +- A production build of the frontend (`npm run build` in `client-v3/`) The global setup script starts and stops the backend automatically — no manual server management required. @@ -38,22 +38,22 @@ and cleans up the database file. All tests share this single server instance and Tests are numbered to enforce a specific run order. Each spec depends on the database state left by earlier specs. -| File | Area | -|------|------| -| `01-first-run.spec.ts` | Initial server setup wizard | -| `02-auth.spec.ts` | Login, logout, session handling | -| `03-system-config.spec.ts` | System Config: show creation, users, settings, system info, logs, backups | -| `04-show-config-show.spec.ts` | Show Config: show details | -| `05-show-config-acts-scenes.spec.ts` | Show Config: acts and scenes CRUD | -| `06-show-config-characters.spec.ts` | Show Config: characters CRUD | -| `07-show-config-stage.spec.ts` | Show Config: props, scenery, crew, stage manager allocations | -| `08-show-config-cues.spec.ts` | Show Config: cue types CRUD | -| `09-show-config-mics.spec.ts` | Show Config: microphones, allocations, timeline | -| `10-show-config-script.spec.ts` | Show Config: script editing, saving, stage direction styles, cue add/edit/delete | -| `11-show-config-revisions.spec.ts` | Show Config: script revisions | -| `12-show-config-sessions.spec.ts` | Show Config: live show sessions | -| `13-live-show.spec.ts` | Live show display and cue triggering | -| `14-user-settings.spec.ts` | User settings and profile | +| File | Area | +| ------------------------------------ | -------------------------------------------------------------------------------- | +| `01-first-run.spec.ts` | Initial server setup wizard | +| `02-auth.spec.ts` | Login, logout, session handling | +| `03-system-config.spec.ts` | System Config: show creation, users, settings, system info, logs, backups | +| `04-show-config-show.spec.ts` | Show Config: show details | +| `05-show-config-acts-scenes.spec.ts` | Show Config: acts and scenes CRUD | +| `06-show-config-characters.spec.ts` | Show Config: characters CRUD | +| `07-show-config-stage.spec.ts` | Show Config: props, scenery, crew, stage manager allocations | +| `08-show-config-cues.spec.ts` | Show Config: cue types CRUD | +| `09-show-config-mics.spec.ts` | Show Config: microphones, allocations, timeline | +| `10-show-config-script.spec.ts` | Show Config: script editing, saving, stage direction styles, cue add/edit/delete | +| `11-show-config-revisions.spec.ts` | Show Config: script revisions | +| `12-show-config-sessions.spec.ts` | Show Config: live show sessions | +| `13-live-show.spec.ts` | Live show display and cue triggering | +| `14-user-settings.spec.ts` | User settings and profile | > **Important**: specs must always be run as a full suite. Running individual spec files in > isolation will fail because each spec relies on database state created by earlier specs. diff --git a/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts new file mode 100644 index 000000000..18ebaef3d --- /dev/null +++ b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts @@ -0,0 +1,138 @@ +/** + * Collaborative script editing (setting on): open the shared draft and browse it. + * + * Step 2 is read-only. Runs last on purpose — it flips the server-wide + * `collaborative_script_editing` setting, which every earlier spec needs OFF, and puts + * it back in afterAll. (The retry hooks also restore config, covering a failed run.) + * + * The setting is hidden from the Settings page until the editor ships, so it is driven + * through the API with the admin's own token. + */ +import { test, expect, type BrowserContext, type Page } from '@playwright/test'; +import { UI_BASE, loginAsAdmin, waitForAppReady, confirmModal } from '../helpers.js'; +import { registerRetryHooks } from '../db-snapshot.js'; + +test.describe.configure({ mode: 'serial' }); + +registerRetryHooks(); + +const MODE = 'collaborative_script_editing'; +const SCRIPT_URL = `${UI_BASE}/show-config/script`; + +let ctx: BrowserContext; +let page: Page; +let ctx2: BrowserContext | null = null; + +async function token(p: Page): Promise { + const t = await p.evaluate(() => localStorage.getItem('digiscript_auth_token')); + if (!t) throw new Error('no auth token in localStorage'); + return t; +} + +async function patchSettings(p: Page, body: Record) { + return p.request.patch(`${UI_BASE}/api/v1/settings`, { + headers: { Authorization: `Bearer ${await token(p)}` }, + data: body, + }); +} + +/** Wait until the collaborative editor (not the classic one) is what is on screen. */ +async function openScriptEditor(p: Page): Promise { + await p.goto(SCRIPT_URL); + await waitForAppReady(p); + await expect(p.locator('button:has-text("Go to Page")')).toBeVisible({ timeout: 15_000 }); +} + +const stopEditing = (p: Page) => p.locator('button:has-text("Stop Editing")'); +const firstRow = (p: Page) => p.locator('.script-line-row').first(); + +test.beforeAll(async ({ browser }) => { + ctx = await browser.newContext(); + page = await ctx.newPage(); + await loginAsAdmin(page); + const response = await patchSettings(page, { [MODE]: true }); + expect(response.status()).toBe(200); + await openScriptEditor(page); +}); + +test.afterAll(async () => { + // Nobody is editing any more, so the server allows the switch back. The room closes + // asynchronously after the last editor leaves, so allow it a moment. + await expect + .poll(async () => (await patchSettings(page, { [MODE]: false })).status(), { + timeout: 15_000, + }) + .toBe(200); + await ctx2?.close(); + await ctx.close(); +}); + +test('the mode setting rejects a non-boolean value', async () => { + const response = await patchSettings(page, { [MODE]: 'yes' }); + + expect(response.status()).toBe(400); +}); + +test('shows the collaborative editor, with cut mode not yet available', async () => { + await expect(page.locator('button:has-text("Edit")')).toBeVisible(); + await expect(page.locator('button:has-text("Cuts")')).toBeDisabled(); +}); + +test('Edit opens the shared draft and its lines appear', async () => { + await page.click('button:has-text("Edit")'); + + await expect(stopEditing(page)).toBeVisible({ timeout: 10_000 }); + await expect(firstRow(page)).toBeVisible({ timeout: 15_000 }); +}); + +test('the mode cannot be changed while someone is editing', async () => { + const response = await patchSettings(page, { [MODE]: false }); + + expect(response.status()).toBe(409); +}); + +test('Next Page stops at the last page that exists', async () => { + const next = page.locator('button:has-text("Next Page")'); + for (let i = 0; i < 20 && (await next.isEnabled()); i += 1) { + await next.click(); + } + + await expect(next).toBeDisabled(); + // Go to Page past the end is refused with a message rather than creating a page. + await page.click('button:has-text("Go to Page")'); + await page.fill('#page-input', '999'); + await confirmModal(page); + await expect(page.getByTestId('page-error')).toContainText('last page is'); + await page.keyboard.press('Escape'); +}); + +test('reloading during a draft rejoins it', async () => { + await page.reload(); + await waitForAppReady(page); + + await expect(stopEditing(page)).toBeVisible({ timeout: 15_000 }); + await expect(firstRow(page)).toBeVisible({ timeout: 15_000 }); +}); + +test('a second browser can edit the same script at the same time', async ({ browser }) => { + ctx2 = await browser.newContext(); + const page2 = await ctx2.newPage(); + await loginAsAdmin(page2); + await openScriptEditor(page2); + + await page2.click('button:has-text("Edit")'); + + await expect(stopEditing(page2)).toBeVisible({ timeout: 10_000 }); + await expect(firstRow(page2)).toBeVisible({ timeout: 15_000 }); + // The first browser is still an editor with its draft open. + await expect(stopEditing(page)).toBeVisible(); + + await stopEditing(page2).click(); + await expect(page2.locator('button:has-text("Edit")')).toBeVisible({ timeout: 10_000 }); +}); + +test('Stop Editing returns to Edit', async () => { + await stopEditing(page).click(); + + await expect(page.locator('button:has-text("Edit")')).toBeVisible({ timeout: 10_000 }); +}); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts new file mode 100644 index 000000000..81482c4a6 --- /dev/null +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts @@ -0,0 +1,308 @@ +/* eslint-disable vue/one-component-per-file -- small in-file stubs */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import { flushPromises, mount, type VueWrapper } from '@vue/test-utils'; +import { defineComponent, h } from 'vue'; +import CollabScriptEditor from './CollabScriptEditor.vue'; +import { useScriptDraftStore } from '@/stores/scriptDraft'; +import { useScriptConfigStore } from '@/stores/scriptConfig'; +import { useScriptStore } from '@/stores/script'; +import { useShowStore } from '@/stores/show'; +import { useSystemStore } from '@/stores/system'; +import { useWebSocketStore } from '@/stores/websocket'; +import type { SnapshotLine } from '@/js/yjs/yjsSnapshot'; +import type { ScriptLine } from '@/types/api/script'; + +const sendObj = vi.fn((_data: object) => true); +vi.mock('@/composables/useWebSocket', () => ({ + useWebSocket: () => ({ sendObj, connect: vi.fn() }), +})); +vi.mock('@/js/toast', () => ({ + toast: { info: vi.fn(), error: vi.fn(), success: vi.fn() }, +})); + +// The viewer is stubbed to a bare row so a row's DOM identity can be tracked. +const ViewerStub = defineComponent({ + props: { line: { type: Object, required: true }, lineIndex: { type: Number, default: 0 } }, + render() { + return h('div', { class: 'row-stub' }, (this.line as { _uid?: string })._uid ?? 'rest'); + }, +}); +// bootstrap-vue-next components are not registered in unit tests; only the alert needs +// real show/hide behaviour, the rest can render as plain elements. +const AlertStub = defineComponent({ + props: { modelValue: { type: Boolean, default: false } }, + render() { + return this.modelValue ? h('div', { class: 'alert-stub' }, this.$slots.default?.()) : null; + }, +}); + +/** Plain-element stand-ins for the Bootstrap components (not registered in unit tests). */ +function passthrough(tag: string) { + return defineComponent({ + inheritAttrs: true, + render() { + return h(tag, this.$slots.default?.()); + }, + }); +} +const bootstrapStubs = { + BContainer: passthrough('div'), + BRow: passthrough('div'), + BCol: passthrough('div'), + BButton: passthrough('button'), + BButtonGroup: passthrough('div'), + BSpinner: passthrough('span'), + BModal: passthrough('div'), + BForm: passthrough('form'), + BFormGroup: passthrough('div'), + BFormInput: passthrough('span'), +}; + +function snap(uid: string, id: number | null = null): SnapshotLine { + return { + _id: id === null ? uid : String(id), + _uid: uid, + id, + act_id: 1, + scene_id: 1, + line_type: 1, + stage_direction_style_id: null, + parts: [], + }; +} + +function restLine(id: number): ScriptLine { + return { + id, + act_id: 1, + scene_id: 1, + page: 1, + line_type: 1, + stage_direction_style_id: null, + line_parts: [], + }; +} + +const sentOps = (): string[] => sendObj.mock.calls.map((c) => (c[0] as { OP: string }).OP); +const buttons = (w: VueWrapper) => w.findAll('button'); +const button = (w: VueWrapper, text: string) => + buttons(w).find((b) => b.text().trim() === text || b.text().includes(text)); + +describe('CollabScriptEditor', () => { + let wrapper: VueWrapper | null = null; + + beforeEach(() => { + setActivePinia(createPinia()); + localStorage.clear(); + const ws = useWebSocketStore(); + ws.isConnected = true; + ws.authenticated = true; + ws.internalUUID = 'me'; + useSystemStore().settings = { collaborative_script_editing: true } as never; + useSystemStore().rbacRoles = []; + for (const m of [ + 'getActList', + 'getSceneList', + 'getCharacterList', + 'getCharacterGroupList', + ] as const) { + vi.spyOn(useShowStore(), m).mockResolvedValue(); + } + vi.spyOn(useScriptStore(), 'getStageDirectionStyles').mockResolvedValue(); + vi.spyOn(useScriptStore(), 'loadScriptPage').mockResolvedValue(); + vi.spyOn(useScriptConfigStore(), 'getScriptConfigStatus').mockResolvedValue(); + }); + + afterEach(() => { + wrapper?.unmount(); + wrapper = null; + useScriptDraftStore()._teardown(); + }); + + async function mountEditor(): Promise { + wrapper = mount(CollabScriptEditor, { + global: { + components: bootstrapStubs, + stubs: { ScriptLineViewer: ViewerStub, BAlert: AlertStub }, + }, + }); + await flushPromises(); + return wrapper; + } + + function becomeEditor(): void { + useScriptConfigStore().editors = [{ internal_id: 'me', username: 'admin' }]; + } + + function syncDraft(pages: Record): void { + const draft = useScriptDraftStore(); + draft.status = 'synced'; + draft.pageSnapshots = pages; + } + + it('reads saved pages over REST when not an editor and holds no room', async () => { + useScriptStore().script = { '1': [restLine(1), restLine(2)] }; + useSystemStore().rbacRoles = []; + const w = await mountEditor(); + + expect(w.findAll('.row-stub')).toHaveLength(2); + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('sends the collab flag when Edit is pressed', async () => { + const system = useSystemStore(); + vi.spyOn(system, 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + + await button(w, 'Edit')!.trigger('click'); + + expect(sendObj).toHaveBeenCalledWith({ OP: 'REQUEST_SCRIPT_EDIT', DATA: { collab: true } }); + }); + + it('joins the draft on mount when already an editor (a reload), showing a spinner until synced', async () => { + becomeEditor(); + const w = await mountEditor(); + + expect(sentOps()).toContain('JOIN_SCRIPT_ROOM'); + expect(useScriptDraftStore().status).toBe('joining'); + expect(w.find('.row-stub').exists()).toBe(false); + expect(w.find('.alert-stub').exists()).toBe(false); + }); + + it('joins when the server adds us to the editors after mount, and leaves when removed', async () => { + const w = await mountEditor(); + expect(useScriptDraftStore().isDraftActive).toBe(false); + + becomeEditor(); + await flushPromises(); + expect(useScriptDraftStore().status).toBe('joining'); + + useScriptConfigStore().editors = []; + await flushPromises(); + expect(useScriptDraftStore().isDraftActive).toBe(false); + expect(w.exists()).toBe(true); + }); + + it('renders the synced draft from snapshots, keyed by _uid', async () => { + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a'), snap('b')], '2': [] }); + await flushPromises(); + + expect(w.findAll('.row-stub').map((r) => r.text())).toEqual(['a', 'b']); + }); + + it('keeps a row mounted when a save rewrites its _id (identity is _uid)', async () => { + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a'), snap('b')], '2': [] }); + await flushPromises(); + const rowFor = (uid: string) => w.findAll('.row-stub').find((r) => r.text() === uid)!.element; + const before = rowFor('a'); + + // A save's id patch (same _uid, new DB id) while another editor inserted a line + // above it: an index key would hand row `a` to the new line. + useScriptDraftStore().pageSnapshots = { + '1': [snap('new'), snap('a', 501), snap('b', 502)], + '2': [], + }; + await flushPromises(); + + expect(rowFor('a')).toBe(before); + expect(w.findAll('.row-stub').map((r) => r.text())).toEqual(['new', 'a', 'b']); + }); + + it('stops Next Page at the last page that exists (the server-made trailing page)', async () => { + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + + const next = button(w, 'Next Page')!; + expect(next.attributes('disabled')).toBeUndefined(); + await next.trigger('click'); + await flushPromises(); + expect(w.text()).toContain('Current Page: 2'); + expect(button(w, 'Next Page')!.attributes('disabled')).toBeDefined(); + + // ...and grows with the doc when the server adds a page. + useScriptDraftStore().pageSnapshots['3'] = []; + await flushPromises(); + expect(button(w, 'Next Page')!.attributes('disabled')).toBeUndefined(); + }); + + it('refuses Go to Page past the last page with a message, and does not navigate', async () => { + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + const vm = w.vm as unknown as { pageInputNo: number; goToPage: () => Promise }; + + vm.pageInputNo = 9; + await vm.goToPage(); + await flushPromises(); + + expect(w.find('[data-testid="page-error"]').text()).toContain('last page is 2'); + expect(w.text()).toContain('Current Page: 1'); + }); + + it('clamps a remembered page that is past the end of the draft', async () => { + localStorage.setItem('scriptEditPage', '40'); + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + + expect(w.text()).toContain('Current Page: 2'); + }); + + it('offers Retry when an editor has no room, and Retry rejoins', async () => { + becomeEditor(); + const w = await mountEditor(); + // A rejected join tears the room down: still an editor, but no room. + useScriptDraftStore()._teardown(); + await flushPromises(); + + expect(w.find('.alert-stub').exists()).toBe(true); + sendObj.mockClear(); + await w.find('.alert-stub button').trigger('click'); + + expect(sentOps()).toContain('JOIN_SCRIPT_ROOM'); + expect(useScriptDraftStore().status).toBe('joining'); + }); + + it('Stop Editing leaves the room and tells the server', async () => { + becomeEditor(); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + sendObj.mockClear(); + + await button(w, 'Stop Editing')!.trigger('click'); + + expect(sentOps()).toContain('STOP_SCRIPT_EDIT'); + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('leaves the room when the component unmounts', async () => { + becomeEditor(); + await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + + wrapper!.unmount(); + wrapper = null; + + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('is chosen by the server setting', () => { + const system = useSystemStore(); + expect(system.isCollabScriptEditing).toBe(true); + system.settings = {} as never; + expect(system.isCollabScriptEditing).toBe(false); + system.settings = { collaborative_script_editing: false } as never; + expect(system.isCollabScriptEditing).toBe(false); + }); +}); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue new file mode 100644 index 000000000..a236bb79c --- /dev/null +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -0,0 +1,315 @@ + + + + + diff --git a/client-v3/src/composables/useDraftPages.test.ts b/client-v3/src/composables/useDraftPages.test.ts new file mode 100644 index 000000000..5b130b614 --- /dev/null +++ b/client-v3/src/composables/useDraftPages.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import { useDraftPages } from './useDraftPages'; +import { useScriptDraftStore } from '@/stores/scriptDraft'; +import type { SnapshotLine } from '@/js/yjs/yjsSnapshot'; + +function line(uid: string, id: number | null = null, text = uid): SnapshotLine { + return { + _id: id === null ? uid : String(id), + _uid: uid, + id, + act_id: 1, + scene_id: 1, + line_type: 1, + stage_direction_style_id: null, + parts: [ + { + _id: `${uid}-p`, + _uid: `${uid}-p`, + id: null, + part_index: 0, + character_id: 1, + character_group_id: null, + line_text: text, + }, + ], + }; +} + +function setPages(pages: Record): void { + useScriptDraftStore().pageSnapshots = pages; +} + +describe('useDraftPages', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + it('defaults to a single page before anything has synced', () => { + const { pageNumbers, maxPage } = useDraftPages(); + + expect(pageNumbers.value).toEqual([]); + expect(maxPage.value).toBe(1); + }); + + it('lists existing pages ascending and ignores non-page keys', () => { + setPages({ '10': [], '2': [line('a')], '1': [], notes: [], '01': [], '0': [] }); + const { pageNumbers, maxPage } = useDraftPages(); + + expect(pageNumbers.value).toEqual([1, 2, 10]); + expect(maxPage.value).toBe(10); + }); + + it('follows the server adding a new trailing page', () => { + setPages({ '1': [line('a')], '2': [] }); + const { maxPage } = useDraftPages(); + expect(maxPage.value).toBe(2); + + useScriptDraftStore().pageSnapshots['3'] = []; + + expect(maxPage.value).toBe(3); + }); + + it('presents a page in the ScriptLine shape, carrying _uid', () => { + setPages({ '1': [line('a', 7, 'Hello')] }); + const { linesFor } = useDraftPages(); + + const [first] = linesFor(1); + + expect(first).toMatchObject({ _uid: 'a', id: 7, page: 1, line_type: 1 }); + expect(first.line_parts[0].line_text).toBe('Hello'); + expect(linesFor(9)).toEqual([]); + }); + + describe('previousLineOf', () => { + it('returns the line above on the same page', () => { + setPages({ '1': [line('a'), line('b')] }); + const { previousLineOf } = useDraftPages(); + + expect(previousLineOf(1, 1)?._uid).toBe('a'); + }); + + it('is null at the very start of the script', () => { + setPages({ '1': [line('a')] }); + const { previousLineOf } = useDraftPages(); + + expect(previousLineOf(1, 0)).toBeNull(); + }); + + it('crosses to the last line of the nearest earlier non-empty page', () => { + setPages({ '1': [line('a'), line('b')], '2': [], '3': [line('c')] }); + const { previousLineOf } = useDraftPages(); + + expect(previousLineOf(3, 0)?._uid).toBe('b'); + }); + }); +}); diff --git a/client-v3/src/composables/useDraftPages.ts b/client-v3/src/composables/useDraftPages.ts new file mode 100644 index 000000000..b5fc7ff18 --- /dev/null +++ b/client-v3/src/composables/useDraftPages.ts @@ -0,0 +1,52 @@ +import { computed } from 'vue'; +import { useScriptDraftStore } from '@/stores/scriptDraft'; +import { snapshotPageToScriptLines, type DraftScriptLine } from '@/js/yjs/draftAdapter'; + +/** + * The draft's pages as the script components consume them. Pure reads over the store's + * per-page snapshots (which are only ever replaced for a page that changed), so nothing + * here holds a Yjs object. + * + * The server keeps the doc ending in an empty trailing page, so `maxPage` is the highest + * page that *exists* — navigation must stop there, because a client can't create a page + * (two editors creating the same new page would silently lose one editor's lines). + */ +export function useDraftPages() { + const store = useScriptDraftStore(); + + /** Existing page numbers, ascending. Non-page keys are ignored. */ + const pageNumbers = computed(() => + Object.keys(store.pageSnapshots) + .filter((key) => /^[1-9]\d*$/.test(key)) + .map(Number) + .sort((a, b) => a - b) + ); + + /** The highest existing page (the empty trailing page, once synced); 1 before any sync. */ + const maxPage = computed(() => pageNumbers.value.at(-1) ?? 1); + + function linesFor(page: number): DraftScriptLine[] { + return snapshotPageToScriptLines(page, store.getPageSnapshot(page)); + } + + /** + * The line before `index` on `page`, walking back across pages (skipping empty ones) + * so act/scene headings are suppressed correctly at a page boundary. Null at the very + * start of the script. + */ + function previousLineOf(page: number, index: number): DraftScriptLine | null { + const onPage = store.getPageSnapshot(page); + if (index > 0 && onPage[index - 1]) { + return snapshotPageToScriptLines(page, [onPage[index - 1]])[0]; + } + for (let p = page - 1; p >= 1; p -= 1) { + const lines = store.getPageSnapshot(p); + if (lines.length > 0) { + return snapshotPageToScriptLines(p, [lines[lines.length - 1]])[0]; + } + } + return null; + } + + return { pageNumbers, maxPage, linesFor, previousLineOf }; +} diff --git a/client-v3/src/stores/scriptConfig.test.ts b/client-v3/src/stores/scriptConfig.test.ts index f421c1f17..41d3bb0b9 100644 --- a/client-v3/src/stores/scriptConfig.test.ts +++ b/client-v3/src/stores/scriptConfig.test.ts @@ -1,6 +1,12 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; import type { ScriptLine } from '@/types/api/script'; -import { computePageStatus } from './scriptConfig'; +import { computePageStatus, useScriptConfigStore } from './scriptConfig'; +import { toast } from '@/js/toast'; + +vi.mock('@/js/toast', () => ({ + toast: { info: vi.fn(), error: vi.fn(), success: vi.fn() }, +})); function makeLine(id: number | null, numParts: number): ScriptLine { return { @@ -98,3 +104,29 @@ describe('computePageStatus', () => { expect(status.inserted).toHaveLength(0); }); }); + +describe('requestEditFailure', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + it('shows the server reason when there is one', async () => { + const store = useScriptConfigStore(); + vi.spyOn(store, 'getScriptConfigStatus').mockResolvedValue(); + + await store.requestEditFailure({ reason: 'Collaborative editing is enabled' }); + + expect(toast.error).toHaveBeenCalledWith( + 'Unable to edit script: Collaborative editing is enabled' + ); + }); + + it('falls back to a generic message with no reason', async () => { + const store = useScriptConfigStore(); + vi.spyOn(store, 'getScriptConfigStatus').mockResolvedValue(); + + await store.requestEditFailure(); + + expect(toast.error).toHaveBeenCalledWith('Unable to edit script'); + }); +}); diff --git a/client-v3/src/stores/scriptConfig.ts b/client-v3/src/stores/scriptConfig.ts index 85a5fdeb6..fcc5bcb4e 100644 --- a/client-v3/src/stores/scriptConfig.ts +++ b/client-v3/src/stores/scriptConfig.ts @@ -173,8 +173,10 @@ export const useScriptConfigStore = defineStore('scriptConfig', { } }, - async requestEditFailure(): Promise { - toast.error('Unable to edit script'); + async requestEditFailure(data?: { reason?: string }): Promise { + // The server says *why* (wrong editing mode, another editor, a live show, missing + // permission); show that rather than a bare "unable". + toast.error(data?.reason ? `Unable to edit script: ${data.reason}` : 'Unable to edit script'); await this.getScriptConfigStatus(); this.cutMode = false; }, diff --git a/client-v3/src/stores/system.ts b/client-v3/src/stores/system.ts index b6cd72f62..a03efcd09 100644 --- a/client-v3/src/stores/system.ts +++ b/client-v3/src/stores/system.ts @@ -82,6 +82,10 @@ export const useSystemStore = defineStore('system', { if (!userRbac?.shows) return false; return (userRbac.shows[0][1] & getRbacMask(this.rbacRoles, 'EXECUTE')) !== 0; }, + /** True when the server is in collaborative script editing mode (see `CollabScriptEditor`). */ + isCollabScriptEditing(): boolean { + return this.settings.collaborative_script_editing === true; + }, isScriptEditor(): boolean { if (this.isAdminUser) return true; if (this.rbacRoles.length === 0) return false; diff --git a/client-v3/src/types/api/settings.ts b/client-v3/src/types/api/settings.ts index 05d6fa9ca..ba9f89dee 100644 --- a/client-v3/src/types/api/settings.ts +++ b/client-v3/src/types/api/settings.ts @@ -7,5 +7,7 @@ export interface SystemSettings { current_show: number | null; client_log_enabled: boolean | null; client_log_level: string | null; + /** Server-wide switch to the collaborative script editor (admin setting, default off). */ + collaborative_script_editing?: boolean; [key: string]: unknown; } diff --git a/client-v3/src/views/show/config/ConfigScript.vue b/client-v3/src/views/show/config/ConfigScript.vue index 9ffbfe8d9..4bb039792 100644 --- a/client-v3/src/views/show/config/ConfigScript.vue +++ b/client-v3/src/views/show/config/ConfigScript.vue @@ -3,7 +3,10 @@ - + + + + @@ -13,5 +16,9 @@ From 04360c9f836b8fabeebaa6a35d5e0db3f98e28b9 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Tue, 22 Sep 2026 23:23:59 +0100 Subject: [PATCH 2/6] Step 2: fix E2E locator collisions, adjust reload test to reality (#1419) E2E verification of the previous commit surfaced two real issues, both fixed here: - Two locator collisions with app-wide elements, not caused by duplication I introduced: button:has-text("Edit") also matches the Stage Direction Styles sub-tab's own Edit button (scope to .sticky-header); #page-input is not unique app-wide since App.vue always mounts its own hidden global "Go to Page" modal on every route (scope to .modal.show, the pattern spec 10 already uses for the classic editor's identical modal). - A full page reload does not currently preserve is_editor/is_cutting server-side. Traced to shared, pre-existing WS session-lifecycle code (REFRESH_CLIENT reads the wrong Session row; on_close deletes the old row before the new connection's REFRESH_CLIENT can resume it) with zero prior test coverage anywhere in the codebase - genuinely never exercised before this spec reloaded mid-edit. No draft data is lost (the room rebuilds from its checkpoint on the next join), only the UI's editor status, so the user needs to click Edit again. Filed as #1419 and background-investigated (confirmed empirically via two new xfail backend integration tests, revealing a second independent race the issue hadn't identified; correctly left unfixed pending a real design decision rather than a guessed patch). The spec's reload test and CollabScriptEditor's onMounted comment now describe the real current behaviour instead of the originally assumed automatic rejoin; the rejoin code path itself is untouched and will start working with no client change once #1419 lands. Verified: client-v3 231 Vitest, tsc, eslint, prettier clean; full Playwright suite green in both chromium (229/229) and firefox (229/229, after one unrelated flaky retry of the pre-existing #1412 RBAC spec). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- .../15-collaborative-script-editing.spec.ts | 39 ++++++++++++++----- .../show/config/script/CollabScriptEditor.vue | 9 ++++- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts index 18ebaef3d..4d02407aa 100644 --- a/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts +++ b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts @@ -9,7 +9,7 @@ * through the API with the admin's own token. */ import { test, expect, type BrowserContext, type Page } from '@playwright/test'; -import { UI_BASE, loginAsAdmin, waitForAppReady, confirmModal } from '../helpers.js'; +import { UI_BASE, loginAsAdmin, waitForAppReady, waitForModal, confirmModal } from '../helpers.js'; import { registerRetryHooks } from '../db-snapshot.js'; test.describe.configure({ mode: 'serial' }); @@ -43,6 +43,8 @@ async function openScriptEditor(p: Page): Promise { await expect(p.locator('button:has-text("Go to Page")')).toBeVisible({ timeout: 15_000 }); } +const editButton = (p: Page) => + p.locator('.sticky-header').getByRole('button', { name: 'Edit', exact: true }); const stopEditing = (p: Page) => p.locator('button:has-text("Stop Editing")'); const firstRow = (p: Page) => p.locator('.script-line-row').first(); @@ -74,12 +76,12 @@ test('the mode setting rejects a non-boolean value', async () => { }); test('shows the collaborative editor, with cut mode not yet available', async () => { - await expect(page.locator('button:has-text("Edit")')).toBeVisible(); + await expect(editButton(page)).toBeVisible(); await expect(page.locator('button:has-text("Cuts")')).toBeDisabled(); }); test('Edit opens the shared draft and its lines appear', async () => { - await page.click('button:has-text("Edit")'); + await editButton(page).click(); await expect(stopEditing(page)).toBeVisible({ timeout: 10_000 }); await expect(firstRow(page)).toBeVisible({ timeout: 15_000 }); @@ -99,18 +101,35 @@ test('Next Page stops at the last page that exists', async () => { await expect(next).toBeDisabled(); // Go to Page past the end is refused with a message rather than creating a page. + // App.vue mounts its own (hidden, buttonless) Go to Page modal on every route, so + // #page-input is not unique app-wide — scope to the modal that is actually open, as + // spec 10 does for the classic editor's identical modal. await page.click('button:has-text("Go to Page")'); - await page.fill('#page-input', '999'); + await waitForModal(page, 'Go to Page'); + await page.fill('.modal.show input[type="number"]', '999'); await confirmModal(page); - await expect(page.getByTestId('page-error')).toContainText('last page is'); + await expect(page.locator('.modal.show').getByTestId('page-error')).toContainText('last page is'); await page.keyboard.press('Escape'); + + // Back to page 1 (which has content) so later tests aren't left looking at the + // empty trailing page. + await page.click('button:has-text("Prev Page")'); + await expect(page.locator('p:has-text("Current Page: 1")')).toBeVisible(); }); -test('reloading during a draft rejoins it', async () => { +test('reloading loses editor status (known gap, issue #1419) but not the draft', async () => { + // A full reload closes and reopens the WebSocket, and REFRESH_CLIENT does not + // currently preserve is_editor across that (see issue #1419) — this is a pre-existing + // gap in shared session-lifecycle code, not specific to the collaborative editor. + // What must still hold: no draft content is lost, and clicking Edit again resumes the + // same draft rather than starting a fresh one. await page.reload(); await waitForAppReady(page); - await expect(stopEditing(page)).toBeVisible({ timeout: 15_000 }); + await expect(editButton(page)).toBeVisible({ timeout: 15_000 }); + + await editButton(page).click(); + await expect(stopEditing(page)).toBeVisible({ timeout: 10_000 }); await expect(firstRow(page)).toBeVisible({ timeout: 15_000 }); }); @@ -120,7 +139,7 @@ test('a second browser can edit the same script at the same time', async ({ brow await loginAsAdmin(page2); await openScriptEditor(page2); - await page2.click('button:has-text("Edit")'); + await editButton(page2).click(); await expect(stopEditing(page2)).toBeVisible({ timeout: 10_000 }); await expect(firstRow(page2)).toBeVisible({ timeout: 15_000 }); @@ -128,11 +147,11 @@ test('a second browser can edit the same script at the same time', async ({ brow await expect(stopEditing(page)).toBeVisible(); await stopEditing(page2).click(); - await expect(page2.locator('button:has-text("Edit")')).toBeVisible({ timeout: 10_000 }); + await expect(editButton(page2)).toBeVisible({ timeout: 10_000 }); }); test('Stop Editing returns to Edit', async () => { await stopEditing(page).click(); - await expect(page.locator('button:has-text("Edit")')).toBeVisible({ timeout: 10_000 }); + await expect(editButton(page)).toBeVisible({ timeout: 10_000 }); }); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue index a236bb79c..f3e11f232 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.vue +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -289,8 +289,13 @@ onMounted(async () => { await loadViewerPage(currentPage.value); loaded.value = true; - // A reload while editing: the server kept our editor flag under the new client id, so - // we are an editor again before this component ever saw the change. + // Covers a reconnect that *does* keep our editor flag (a brief drop, not a full + // reload). A full page reload currently does not preserve is_editor server-side + // (issue #1419 — REFRESH_CLIENT reads the wrong session row, and on_close deletes + // the old one before it can be resumed), so isEditor is false here after a reload + // and the user sees Edit again rather than being silently rejoined; no draft data is + // lost, since the room rebuilds from the checkpoint on the next join. Once #1419 is + // fixed server-side this line needs no client change to also cover the reload case. if (isEditor.value) retryJoin(); }); From 86b268f9fa68a35171c1059700b122de7ac0a94d Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Thu, 24 Sep 2026 22:57:54 +0100 Subject: [PATCH 3/6] Address PR #1421 review: Stop Editing lifecycle bug, stuck-spinner failures, test hardening - Critical: stopEditing() sent LEAVE_SCRIPT_ROOM before STOP_SCRIPT_EDIT, so the server's get_room_for_client(self) returned None on STOP and skipped checkpoint + close_active_room for the last editor entirely - the room stayed open, dirty, and unannounced. Now sends STOP only; ROOM_CLOSED (last editor) or the isEditor watcher (another editor still present) tears the local draft down, matching what the server actually does. Also fixed the same silently-ignored-sendObj-return bug in requestEdit. - Important: a failed initial yjsSync apply left status stuck at 'joining' forever - joinFailed only fires on 'idle' and retryJoin() is a no-op while isDraftActive, so there was no way out but Stop Editing. yjsSync now unwinds exactly as collabError already does for a rejected join. Added a 15s join watchdog (mirrors the existing save watchdog) covering a JOIN_SCRIPT_ROOM the server never answers at all. - Suggestion: Go to Page now sets pageError for an invalid/out-of-range number instead of doing nothing, and a rejected navigation (network failure) sets an error instead of leaving an unhandled rejection with the modal stuck open. "Step 2: read-only" reworded so it won't go stale when Step 3 lands. - E2E (15-collaborative-script-editing.spec.ts): afterAll now stops editing on both browser contexts before polling the settings PATCH, since the server refuses the mode change while anyone is still an editor; header comment corrected (retry hooks only restore state ahead of a retry, not a final failed attempt). The reload test captures and compares draft content before/after instead of only checking a row exists, and is commented as a deliberate tripwire for #1419 (also noted there). The two-browser test now proves the browsers share a room (matching content, matching last-page) rather than just that each renders something. The Next-Page test waits for the modal to actually close and navigates back to page 1 via Go to Page instead of assuming the fixture is exactly two pages long. - Unit tests: Stop Editing is now asserted to send STOP only (would have caught the ordering bug), plus its two real follow-up paths (ROOM_CLOSED, losing editor status) and the not-connected case; unmount asserts LEAVE_SCRIPT_ROOM is actually sent; the Retry alert is now driven through the real collabError/yjsSync-failure/ roomClosed handlers rather than a forced _teardown(); new scriptDraft.ts tests for the join watchdog and the failed-sync unwind; new ConfigScript.test.ts asserting which editor renders for each setting value. - requestEditFailure's comment corrected: it also fires for REQUEST_SCRIPT_CUTS failures, not just edit ones. Verified: 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), after merging the latest dev (E2E port isolation + the #1425 Firefox leader/follower fix). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- client-v3/e2e/README.md | 2 +- .../15-collaborative-script-editing.spec.ts | 88 ++++++++++++--- .../config/script/CollabScriptEditor.test.ts | 101 +++++++++++++++++- .../show/config/script/CollabScriptEditor.vue | 32 ++++-- client-v3/src/stores/scriptConfig.ts | 6 +- client-v3/src/stores/scriptDraft.test.ts | 46 +++++++- client-v3/src/stores/scriptDraft.ts | 40 ++++++- .../views/show/config/ConfigScript.test.ts | 43 ++++++++ 8 files changed, 330 insertions(+), 28 deletions(-) create mode 100644 client-v3/src/views/show/config/ConfigScript.test.ts diff --git a/client-v3/e2e/README.md b/client-v3/e2e/README.md index 27828eb77..ce86ae452 100644 --- a/client-v3/e2e/README.md +++ b/client-v3/e2e/README.md @@ -40,7 +40,7 @@ The backend port is **not** a fixed 8888. `e2e/env.ts` derives a stable port fro `process.cwd()`, so each checkout (e.g. separate git worktrees driven by parallel CI/agent jobs) gets its own port automatically and runs never collide — set `E2E_PORT=` to pin a specific one instead (e.g. to reproduce CI's port locally). Setup only ever kills a server it can positively -identify as its *own* leftover, by matching a live process's command line against +identify as its _own_ leftover, by matching a live process's command line against `main.py ... --port=` — not by trusting a PID pointer file alone, which a crashed run could leave stale or missing; if the resolved port is held by something else, the suite fails fast with a message telling you to set `E2E_PORT`, rather than killing a process it diff --git a/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts index 4d02407aa..36fe5d053 100644 --- a/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts +++ b/client-v3/e2e/tests/15-collaborative-script-editing.spec.ts @@ -1,15 +1,25 @@ /** * Collaborative script editing (setting on): open the shared draft and browse it. * - * Step 2 is read-only. Runs last on purpose — it flips the server-wide + * Currently read-only: editors join and browse the draft; editing is not yet + * implemented. Runs last on purpose — it flips the server-wide * `collaborative_script_editing` setting, which every earlier spec needs OFF, and puts - * it back in afterAll. (The retry hooks also restore config, covering a failed run.) + * it back in afterAll. A retry's own `beforeAll` restores the DB and config from the + * spec-start snapshot (see `registerRetryHooks`), but that does not help a *final* + * failed attempt — `afterAll` below is what must actually put the setting back. * * The setting is hidden from the Settings page until the editor ships, so it is driven * through the API with the admin's own token. */ import { test, expect, type BrowserContext, type Page } from '@playwright/test'; -import { UI_BASE, loginAsAdmin, waitForAppReady, waitForModal, confirmModal } from '../helpers.js'; +import { + UI_BASE, + loginAsAdmin, + waitForAppReady, + waitForModal, + waitForModalClosed, + confirmModal, +} from '../helpers.js'; import { registerRetryHooks } from '../db-snapshot.js'; test.describe.configure({ mode: 'serial' }); @@ -58,14 +68,36 @@ test.beforeAll(async ({ browser }) => { }); test.afterAll(async () => { - // Nobody is editing any more, so the server allows the switch back. The room closes - // asynchronously after the last editor leaves, so allow it a moment. + // Everyone must actually stop editing before the mode can switch back — the server + // refuses while any session is still an editor (409), regardless of which test in + // this file failed. Closing a context does not itself send STOP_SCRIPT_EDIT, so do + // that first; then poll, since the room closes asynchronously after the last editor + // leaves (checkpoint, then ROOM_CLOSED). + if (ctx2) { + const page2 = ctx2.pages()[0]; + if ( + page2 && + (await stopEditing(page2) + .isVisible() + .catch(() => false)) + ) { + await stopEditing(page2).click(); + } + await ctx2.close(); + } + if ( + await stopEditing(page) + .isVisible() + .catch(() => false) + ) { + await stopEditing(page).click(); + } + await expect .poll(async () => (await patchSettings(page, { [MODE]: false })).status(), { timeout: 15_000, }) .toBe(200); - await ctx2?.close(); await ctx.close(); }); @@ -101,8 +133,9 @@ test('Next Page stops at the last page that exists', async () => { await expect(next).toBeDisabled(); // Go to Page past the end is refused with a message rather than creating a page. - // App.vue mounts its own (hidden, buttonless) Go to Page modal on every route, so - // #page-input is not unique app-wide — scope to the modal that is actually open, as + // App.vue mounts its own (closed) live-view "Go to Page" modal on every route — + // opened from the navbar's "Jump To Page" item, not by a visible button here — so + // #page-input is not unique app-wide; scope to the modal that is actually open, as // spec 10 does for the classic editor's identical modal. await page.click('button:has-text("Go to Page")'); await waitForModal(page, 'Go to Page'); @@ -110,10 +143,15 @@ test('Next Page stops at the last page that exists', async () => { await confirmModal(page); await expect(page.locator('.modal.show').getByTestId('page-error')).toContainText('last page is'); await page.keyboard.press('Escape'); + await waitForModalClosed(page); - // Back to page 1 (which has content) so later tests aren't left looking at the - // empty trailing page. - await page.click('button:has-text("Prev Page")'); + // Back to page 1 (which has content) via Go to Page rather than a single Prev Page + // click, so this doesn't assume the fixture is exactly two pages long. + await page.click('button:has-text("Go to Page")'); + await waitForModal(page, 'Go to Page'); + await page.fill('.modal.show input[type="number"]', '1'); + await confirmModal(page); + await waitForModalClosed(page); await expect(page.locator('p:has-text("Current Page: 1")')).toBeVisible(); }); @@ -122,15 +160,26 @@ test('reloading loses editor status (known gap, issue #1419) but not the draft', // currently preserve is_editor across that (see issue #1419) — this is a pre-existing // gap in shared session-lifecycle code, not specific to the collaborative editor. // What must still hold: no draft content is lost, and clicking Edit again resumes the - // same draft rather than starting a fresh one. + // *same* draft rather than starting a fresh one, so this captures the content before + // reloading and compares it after — a fresh/empty draft would also show a first row, + // so "a row is visible" alone wouldn't prove that. + const before = await firstRow(page).textContent(); + await page.reload(); await waitForAppReady(page); + // Tripwire, not the desired behaviour: this asserts the #1419 bug (Edit shown again, + // not silently rejoined). It will start failing the moment #1419 is fixed server-side + // — see the note left on that issue — at which point invert this to assert + // `stopEditing(page)` is visible instead, since CollabScriptEditor's reload-rejoin + // code path (onMounted's `if (isEditor.value) retryJoin()`) needs no change to cover + // that case. await expect(editButton(page)).toBeVisible({ timeout: 15_000 }); await editButton(page).click(); await expect(stopEditing(page)).toBeVisible({ timeout: 10_000 }); await expect(firstRow(page)).toBeVisible({ timeout: 15_000 }); + await expect(firstRow(page)).toHaveText(before ?? ''); }); test('a second browser can edit the same script at the same time', async ({ browser }) => { @@ -146,6 +195,21 @@ test('a second browser can edit the same script at the same time', async ({ brow // The first browser is still an editor with its draft open. await expect(stopEditing(page)).toBeVisible(); + // Prove the two browsers actually share the draft room, not just that each renders + // *a* row: matching content on both, and — since a client can't create a page — the + // same server-owned trailing page stops Next Page at the same page number for both. + await expect(firstRow(page2)).toHaveText((await firstRow(page).textContent()) ?? ''); + + async function lastPageReached(p: Page): Promise { + const next = p.locator('button:has-text("Next Page")'); + for (let i = 0; i < 20 && (await next.isEnabled()); i += 1) { + await next.click(); + } + await expect(next).toBeDisabled(); + return p.locator('p:has-text("Current Page:")').textContent(); + } + expect(await lastPageReached(page2)).toBe(await lastPageReached(page)); + await stopEditing(page2).click(); await expect(editButton(page2)).toBeVisible({ timeout: 10_000 }); }); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts index 81482c4a6..8c5b92d01 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts @@ -272,7 +272,43 @@ describe('CollabScriptEditor', () => { expect(useScriptDraftStore().status).toBe('joining'); }); - it('Stop Editing leaves the room and tells the server', async () => { + it('a rejected join (COLLAB_ERROR while joining) shows the real server reason in the Retry alert', async () => { + becomeEditor(); + const w = await mountEditor(); + expect(useScriptDraftStore().status).toBe('joining'); + + useScriptDraftStore().collabError({ error: 'No show loaded' }); + await flushPromises(); + + expect(w.find('.alert-stub').text()).toContain('No show loaded'); + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('a failed initial sync unwinds to the Retry alert instead of hanging on the spinner', async () => { + becomeEditor(); + const w = await mountEditor(); + expect(useScriptDraftStore().status).toBe('joining'); + + useScriptDraftStore().yjsSync({ step: 0, payload: 'not-valid-base64!!' }); + await flushPromises(); + + expect(w.find('.alert-stub').exists()).toBe(true); + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('ROOM_CLOSED after a successful sync also lands on Retry, not a stuck view', async () => { + becomeEditor(); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + + useScriptDraftStore().roomClosed(); + await flushPromises(); + + expect(w.find('.alert-stub').exists()).toBe(true); + }); + + it('Stop Editing sends STOP only — never LEAVE first, which would make the server skip checkpoint/close', async () => { becomeEditor(); vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); const w = await mountEditor(); @@ -282,18 +318,79 @@ describe('CollabScriptEditor', () => { await button(w, 'Stop Editing')!.trigger('click'); - expect(sentOps()).toContain('STOP_SCRIPT_EDIT'); + // LEAVE_SCRIPT_ROOM first would remove us from room.clients before STOP_SCRIPT_EDIT + // arrives, so the server's get_room_for_client(self) would return None and skip + // checkpoint/close entirely — the local draft must not be torn down client-side + // either, until the server actually says so (ROOM_CLOSED, or editors no longer + // listing us). + expect(sentOps()).toEqual(['STOP_SCRIPT_EDIT']); + expect(useScriptDraftStore().isDraftActive).toBe(true); + }); + + it('Stop Editing as the last editor: ROOM_CLOSED (from the server) tears the draft down', async () => { + becomeEditor(); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + + await button(w, 'Stop Editing')!.trigger('click'); + useScriptDraftStore().roomClosed(); + await flushPromises(); + + expect(useScriptDraftStore().isDraftActive).toBe(false); + }); + + it('Stop Editing with another editor still present: losing editor status tears the draft down', async () => { + becomeEditor(); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + + await button(w, 'Stop Editing')!.trigger('click'); + // The server's GET_SCRIPT_CONFIG_STATUS reply no longer lists us. + useScriptConfigStore().editors = []; + await flushPromises(); + expect(useScriptDraftStore().isDraftActive).toBe(false); }); + it('requestEdit tells the user when the frame could not be sent', async () => { + sendObj.mockReturnValueOnce(false); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + + await button(w, 'Edit')!.trigger('click'); + + const { toast } = await import('@/js/toast'); + expect(toast.error).toHaveBeenCalledWith('Cannot edit script: not connected to the server'); + }); + + it('stopEditing tells the user when the frame could not be sent', async () => { + becomeEditor(); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); + sendObj.mockReturnValueOnce(false); + + await button(w, 'Stop Editing')!.trigger('click'); + + const { toast } = await import('@/js/toast'); + expect(toast.error).toHaveBeenCalledWith('Cannot stop editing: not connected to the server'); + }); + it('leaves the room when the component unmounts', async () => { becomeEditor(); await mountEditor(); syncDraft({ '1': [snap('a')], '2': [] }); + sendObj.mockClear(); wrapper!.unmount(); wrapper = null; + expect(sentOps()).toContain('LEAVE_SCRIPT_ROOM'); expect(useScriptDraftStore().isDraftActive).toBe(false); }); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue index f3e11f232..c832750a0 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.vue +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -135,14 +135,16 @@ import { useUserStore } from '@/stores/user'; import { useWebSocketStore } from '@/stores/websocket'; import { useWebSocket } from '@/composables/useWebSocket'; import { useDraftPages } from '@/composables/useDraftPages'; +import { toast } from '@/js/toast'; import ScriptLineViewer from './ScriptLineViewer.vue'; import type { ScriptLine } from '@/types/api/script'; /** - * The script editor for collaborative mode (`collaborative_script_editing` on). Step 2: - * read-only. An editor joins the shared draft and browses it; everyone else reads saved - * pages over REST exactly as the classic editor's viewer does. The classic - * `ScriptEditor.vue` is untouched and is what renders when the setting is off. + * The script editor for collaborative mode (`collaborative_script_editing` on). + * Currently read-only: an editor joins the shared draft and browses it; editing is not + * yet implemented. Everyone else reads saved pages over REST exactly as the classic + * editor's viewer does. The classic `ScriptEditor.vue` is untouched and is what renders + * when the setting is off. */ const showStore = useShowStore(); @@ -217,7 +219,10 @@ function openGoToPage(): void { async function goToPage(): Promise { const target = pageInputNo.value; - if (!Number.isInteger(target) || target < 1) return; + if (!Number.isInteger(target) || target < 1) { + pageError.value = 'Enter a page number of 1 or more.'; + return; + } if (draftReady.value && target > maxPage.value) { pageError.value = `Page ${target} does not exist yet — the last page is ${maxPage.value}.`; return; @@ -226,19 +231,28 @@ async function goToPage(): Promise { changingPage.value = true; try { await navigateTo(target); + goToPageModal.value?.hide(); + } catch { + pageError.value = 'Could not load that page — check the connection and try again.'; } finally { changingPage.value = false; } - goToPageModal.value?.hide(); } function requestEdit(): void { - sendObj({ OP: 'REQUEST_SCRIPT_EDIT', DATA: { collab: true } }); + if (!sendObj({ OP: 'REQUEST_SCRIPT_EDIT', DATA: { collab: true } })) { + toast.error('Cannot edit script: not connected to the server'); + } } function stopEditing(): void { - draftStore.leaveScriptRoom(); - sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} }); + // STOP only: the server only demotes, checkpoints and closes the room for a client + // that is still in it (LEAVE_SCRIPT_ROOM first would remove us before STOP arrives, + // so the server would skip both). ROOM_CLOSED (if we were the last editor) or the + // isEditor watcher below then tears down the local draft. + if (!sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} })) { + toast.error('Cannot stop editing: not connected to the server'); + } } function retryJoin(): void { diff --git a/client-v3/src/stores/scriptConfig.ts b/client-v3/src/stores/scriptConfig.ts index fcc5bcb4e..f112e7832 100644 --- a/client-v3/src/stores/scriptConfig.ts +++ b/client-v3/src/stores/scriptConfig.ts @@ -174,8 +174,10 @@ export const useScriptConfigStore = defineStore('scriptConfig', { }, async requestEditFailure(data?: { reason?: string }): Promise { - // The server says *why* (wrong editing mode, another editor, a live show, missing - // permission); show that rather than a bare "unable". + // Also fires for a rejected REQUEST_SCRIPT_CUTS (same ACTION, e.g. a cutter or + // editor already in the way, or a live show) — "Unable to edit script" is the + // closest one-size-fits-all label. The server sends a reason (mode mismatch, a + // live show, a cutter/editor in the way, missing permission); show it. toast.error(data?.reason ? `Unable to edit script: ${data.reason}` : 'Unable to edit script'); await this.getScriptConfigStatus(); this.cutMode = false; diff --git a/client-v3/src/stores/scriptDraft.test.ts b/client-v3/src/stores/scriptDraft.test.ts index 8f43b9459..2f25fdd20 100644 --- a/client-v3/src/stores/scriptDraft.test.ts +++ b/client-v3/src/stores/scriptDraft.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { createPinia, setActivePinia } from 'pinia'; import * as Y from 'yjs'; import { nextTick } from 'vue'; -import { useScriptDraftStore, SAVE_STALL_TIMEOUT_MS } from './scriptDraft'; +import { useScriptDraftStore, SAVE_STALL_TIMEOUT_MS, JOIN_STALL_TIMEOUT_MS } from './scriptDraft'; import { useScriptConfigStore } from './scriptConfig'; import { useWebSocketStore } from './websocket'; import { bytesToBase64 } from '@/js/yjs/base64'; @@ -513,6 +513,7 @@ describe('scriptDraft store', () => { vi.useFakeTimers(); const store = useScriptDraftStore(); store.joinScriptRoom(); + store.yjsSync({ step: 0, payload: makeRemoteUpdate(() => {}) }); store.saveDraft(); vi.advanceTimersByTime(SAVE_STALL_TIMEOUT_MS + 1); @@ -526,6 +527,7 @@ describe('scriptDraft store', () => { vi.useFakeTimers(); const store = useScriptDraftStore(); store.joinScriptRoom(); + store.yjsSync({ step: 0, payload: makeRemoteUpdate(() => {}) }); store.saveDraft(); vi.advanceTimersByTime(SAVE_STALL_TIMEOUT_MS - 1000); @@ -541,6 +543,7 @@ describe('scriptDraft store', () => { vi.useFakeTimers(); const store = useScriptDraftStore(); store.joinScriptRoom(); + store.yjsSync({ step: 0, payload: makeRemoteUpdate(() => {}) }); store.saveDraft(); store.scriptSaved({ last_saved_at: 't' }); @@ -550,6 +553,47 @@ describe('scriptDraft store', () => { expect(toast.error).not.toHaveBeenCalled(); }); + it('unwinds a stuck join when the server never replies (join watchdog)', () => { + vi.useFakeTimers(); + const store = useScriptDraftStore(); + store.joinScriptRoom(); + + vi.advanceTimersByTime(JOIN_STALL_TIMEOUT_MS + 1); + + // Torn down, not left "joining" forever: isDraftActive false means retryJoin() (a + // no-op while active) can actually rejoin, and joinFailed (which only fires on + // 'idle') can show the alert. + expect(store.status).toBe('idle'); + expect(store.isDraftActive).toBe(false); + expect(store.lastCollabError).not.toBeNull(); + expect(toast.error).toHaveBeenCalled(); + }); + + it('a successful sync cancels the join watchdog', () => { + vi.useFakeTimers(); + const store = useScriptDraftStore(); + store.joinScriptRoom(); + store.yjsSync({ step: 0, payload: makeRemoteUpdate(() => {}) }); + + vi.advanceTimersByTime(JOIN_STALL_TIMEOUT_MS * 2); + + expect(store.status).toBe('synced'); + expect(store.lastCollabError).toBeNull(); + }); + + it('a failed initial sync unwinds the join instead of hanging on the spinner', () => { + const store = useScriptDraftStore(); + store.joinScriptRoom(); + + store.yjsSync({ step: 0, payload: 'not-valid-base64!!' }); + + // Same shape as a rejected join (collabError while joining): torn down so Retry + // has something to retry, rather than left "joining" with no way out. + expect(store.status).toBe('idle'); + expect(store.isDraftActive).toBe(false); + expect(store.lastCollabError).not.toBeNull(); + }); + it('accepts a YJS_UPDATE before the initial sync without reporting synced', () => { const store = useScriptDraftStore(); store.joinScriptRoom(); diff --git a/client-v3/src/stores/scriptDraft.ts b/client-v3/src/stores/scriptDraft.ts index ae5172b9b..e19e9ffe3 100644 --- a/client-v3/src/stores/scriptDraft.ts +++ b/client-v3/src/stores/scriptDraft.ts @@ -38,6 +38,15 @@ export type DraftStatus = 'idle' | 'joining' | 'synced'; */ export const SAVE_STALL_TIMEOUT_MS = 30_000; +/** + * How long a join may go without reaching `synced` before we give up — covers both a + * `JOIN_SCRIPT_ROOM` the server never answers, and an initial sync that arrives but + * fails to apply (see `yjsSync`). Without this, either leaves the spinner up forever: + * `joinFailed` only fires on `status === 'idle'`, and `retryJoin()` is a no-op while + * `isDraftActive` (which `'joining'` counts as) is still true. + */ +export const JOIN_STALL_TIMEOUT_MS = 15_000; + // The Y.Doc/provider must never live inside Pinia's reactive state — Vue 3's Proxy // walks nested objects the same way Vue 2's defineProperty did, and Yjs's internal // bookkeeping (_item, _map, doc, ...) breaks under that walk. A plain module-level @@ -49,6 +58,7 @@ let provider: ScriptDocProvider | null = null; let unsubscribeDoc: (() => void) | null = null; let stopReconnectWatch: (() => void) | null = null; let saveWatchdog: ReturnType | null = null; +let joinWatchdog: ReturnType | null = null; // Count of local-origin doc updates since the last saveDraft() call, used to decide // whether a following scriptSaved is allowed to clear isDraftDirty (see scriptSaved). let localEditsSinceSaveRequest = 0; @@ -209,7 +219,9 @@ export const useScriptDraftStore = defineStore('scriptDraft', { if (!provider.join()) { this._teardown(); toast.error('Cannot open script draft: not connected to the server'); + return; } + this._armJoinWatchdog(); }, /** Leave the draft room and tear down the Y.Doc. Safe to call when not active. */ @@ -225,6 +237,7 @@ export const useScriptDraftStore = defineStore('scriptDraft', { stopReconnectWatch?.(); stopReconnectWatch = null; this._clearSaveWatchdog(); + this._clearJoinWatchdog(); provider?.destroy(); provider = null; ydoc?.destroy(); @@ -289,6 +302,24 @@ export const useScriptDraftStore = defineStore('scriptDraft', { } }, + _armJoinWatchdog(): void { + this._clearJoinWatchdog(); + joinWatchdog = setTimeout(() => { + joinWatchdog = null; + if (this.status !== 'joining') return; + this.lastCollabError = 'Joining the script draft timed out'; + toast.error('Joining the script draft timed out — check the connection and try again'); + this._teardown(); + }, JOIN_STALL_TIMEOUT_MS); + }, + + _clearJoinWatchdog(): void { + if (joinWatchdog !== null) { + clearTimeout(joinWatchdog); + joinWatchdog = null; + } + }, + saveDraft(): void { if (!this.isDraftActive) return; // A second request while one is in flight would race two saves of the same doc. @@ -325,11 +356,18 @@ export const useScriptDraftStore = defineStore('scriptDraft', { if (!applied) { // Covers step 0 (initial full state) and step 2 (a resync diff): a failed apply // leaves the local doc missing server state, so never report synced over it, - // and surface the failure rather than diverging silently. + // and surface the failure rather than diverging silently. While still joining, + // there is no other way out of this for the caller (retryJoin() is a no-op + // while isDraftActive, and joinFailed only fires on 'idle') — unwind exactly as + // collabError does for a rejected join, so the Retry alert can appear. this.lastCollabError = 'Failed to apply a script sync from the server'; toast.error('Failed to load the script draft — try rejoining'); + if (this.status === 'joining') { + this._teardown(); + } return; } + this._clearJoinWatchdog(); this.status = 'synced'; if (data.step === 0) { this.isDraftDirty = false; diff --git a/client-v3/src/views/show/config/ConfigScript.test.ts b/client-v3/src/views/show/config/ConfigScript.test.ts new file mode 100644 index 000000000..886a113ea --- /dev/null +++ b/client-v3/src/views/show/config/ConfigScript.test.ts @@ -0,0 +1,43 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { createPinia, setActivePinia } from 'pinia'; +import { mount } from '@vue/test-utils'; +import ConfigScript from './ConfigScript.vue'; +import { useSystemStore } from '@/stores/system'; +import ScriptEditor from '@/components/show/config/script/ScriptEditor.vue'; +import CollabScriptEditor from '@/components/show/config/script/CollabScriptEditor.vue'; + +describe('ConfigScript', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + function mountView() { + return mount(ConfigScript, { + global: { + stubs: { + ScriptEditor: true, + CollabScriptEditor: true, + StageDirectionStyles: true, + BTabs: { template: '
' }, + BTab: { template: '
' }, + }, + }, + }); + } + + it('renders the classic editor when collaborative_script_editing is off', () => { + useSystemStore().settings = { collaborative_script_editing: false } as never; + const w = mountView(); + + expect(w.findComponent(ScriptEditor).exists()).toBe(true); + expect(w.findComponent(CollabScriptEditor).exists()).toBe(false); + }); + + it('renders the collaborative editor when collaborative_script_editing is on', () => { + useSystemStore().settings = { collaborative_script_editing: true } as never; + const w = mountView(); + + expect(w.findComponent(CollabScriptEditor).exists()).toBe(true); + expect(w.findComponent(ScriptEditor).exists()).toBe(false); + }); +}); From 9744cf99813c1fb9e767c4526b04469fcdbc114e Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 26 Oct 2026 09:55:07 +0000 Subject: [PATCH 4/6] Address PR #1421 review round 2: suppress the Retry-alert flash on Stop Editing Round 2 confirmed the critical lifecycle fix and every Important/Suggestion item from round 1, with one optional UX item left open: when the last editor stops, the server's ROOM_CLOSED tears the local draft down to 'idle' before its own GET_SCRIPT_CONFIG_STATUS broadcast clears isEditor, so joinFailed's "Could not open the script draft" + Retry alert flashed for that one round trip - and clicking Retry in that window would rejoin the room just asked to leave. Fixed with the suggested stopRequested flag: set on a successful STOP_SCRIPT_EDIT send, suppresses joinFailed until the isEditor watcher confirms we're no longer an editor (which also resets it). requestEdit() also resets it defensively, since a stale flag must never suppress a later join's own failure, even though the normal UI flow can't actually reach that state (Edit only shows once isEditor is already false, which the watcher's reset already covers). Tests: the existing "Stop Editing as the last editor" test now also asserts no alert shows after roomClosed() - confirmed it fails without the fix, passes with it. New test drives the defensive reset in requestEdit() directly (forces the flag on via the component's exposed setup state, since the normal flow can never leave it stale). Verified: client-v3 262 Vitest (+1 net), tsc/eslint/prettier/build clean, full Playwright green in both chromium and firefox (235/235 each, no retries). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- .../config/script/CollabScriptEditor.test.ts | 24 +++++++++++++++++++ .../show/config/script/CollabScriptEditor.vue | 24 ++++++++++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts index 8c5b92d01..aac623165 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts @@ -339,6 +339,30 @@ describe('CollabScriptEditor', () => { await flushPromises(); expect(useScriptDraftStore().isDraftActive).toBe(false); + // ROOM_CLOSED (sent before the server's own GET_SCRIPT_CONFIG_STATUS broadcast) + // tears the draft down to 'idle' while we are still an editor on paper — without + // stopRequested suppressing it, that's exactly joinFailed's shape, and Retry would + // rejoin the room we just asked to leave. + expect(w.find('.alert-stub').exists()).toBe(false); + }); + + it('requestEdit defensively clears stopRequested, so a stale flag can never suppress a later join failure', async () => { + // The isEditor watcher already clears stopRequested whenever isEditor goes + // false, which is the only way to reach the Edit button in the first place — + // so this can't happen through the UI. requestEdit() resets it anyway, as + // belt-and-braces; this test drives that line directly, by forcing the flag + // on before Edit is pressed, bypassing the normal (always-already-reset) flow. + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + (w.vm as unknown as { stopRequested: boolean }).stopRequested = true; + + await button(w, 'Edit')!.trigger('click'); + becomeEditor(); + await flushPromises(); + useScriptDraftStore()._teardown(); // this join is rejected + + await flushPromises(); + expect(w.find('.alert-stub').exists()).toBe(true); }); it('Stop Editing with another editor still present: losing editor status tears the draft down', async () => { diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue index 15223d2b2..079ff80d2 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.vue +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -164,6 +164,12 @@ const changingPage = ref(false); const pageInputNo = ref(1); const pageError = ref(null); const goToPageModal = ref>(); +// True from a successful `stopEditing()` send until the server confirms we are no +// longer an editor. STOP_SCRIPT_EDIT's own ROOM_CLOSED (for the last editor) arrives +// before its GET_SCRIPT_CONFIG_STATUS broadcast, so without this flag `joinFailed` +// would show the "could not open the script draft" alert — with a Retry that would +// rejoin the room we just asked to leave — for that one round trip. +const stopRequested = ref(false); // We are an editor when the server lists us among the editors. The server commits // `is_editor` before it broadcasts GET_SCRIPT_CONFIG_STATUS, so this is also the moment @@ -177,8 +183,11 @@ const hasCutters = computed(() => scriptConfigStore.cutters.length > 0); const draftReady = computed(() => isEditor.value && draftStore.isDraftSynced); // Editor whose room is gone: a rejected join, ROOM_CLOSED, or the socket not up. Without -// this the user is an editor with no room and no way out but Stop Editing. -const joinFailed = computed(() => loaded.value && isEditor.value && draftStore.status === 'idle'); +// this the user is an editor with no room and no way out but Stop Editing. Suppressed +// while our own stopEditing() is still awaiting confirmation (see stopRequested). +const joinFailed = computed( + () => loaded.value && isEditor.value && !stopRequested.value && draftStore.status === 'idle' +); const maxPage = draftPages.maxPage; const atLastDraftPage = computed(() => draftReady.value && currentPage.value >= maxPage.value); @@ -240,6 +249,10 @@ async function goToPage(): Promise { } function requestEdit(): void { + // A fresh edit request supersedes any stop still awaiting confirmation (there is + // no realistic way to reach this while stopRequested is true — Edit isn't shown + // while isEditor is — but a stale flag must never suppress this join's own alert). + stopRequested.value = false; if (!sendObj({ OP: 'REQUEST_SCRIPT_EDIT', DATA: { collab: true } })) { toast.error('Cannot edit script: not connected to the server'); } @@ -250,7 +263,9 @@ function stopEditing(): void { // that is still in it (LEAVE_SCRIPT_ROOM first would remove us before STOP arrives, // so the server would skip both). ROOM_CLOSED (if we were the last editor) or the // isEditor watcher below then tears down the local draft. - if (!sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} })) { + if (sendObj({ OP: 'STOP_SCRIPT_EDIT', DATA: {} })) { + stopRequested.value = true; + } else { toast.error('Cannot stop editing: not connected to the server'); } } @@ -268,6 +283,9 @@ watch(isEditor, async (now, was) => { if (now && !was) { retryJoin(); } else if (!now && was) { + // The server has confirmed we're no longer an editor (our own stop, or losing + // the lock some other way) — stopRequested has done its job, if it was set. + stopRequested.value = false; draftStore.leaveScriptRoom(); await loadViewerPage(currentPage.value); } From 617258a7daa9846f3c10728afe150f4a46b911d5 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 26 Oct 2026 10:15:33 +0000 Subject: [PATCH 5/6] Address PR #1421 review round 3: test the failed-send branch, harden stopRequested Round 3 confirmed the stopRequested fix from round 2 and found two more issues in the same area: - Important: the failed-STOP-send test only asserted the toast, never that stopRequested stayed false - a mutant that sets the flag unconditionally would have passed every test in the file. Added the assertion; confirmed it catches that exact mutant. - Suggestion: stopRequested could get stuck true forever if the STOP frame is sent but never actually registers server-side (a dropped socket, or the #1427 reconnect grace window keeping is_editor alive across it) - the isEditor watcher that normally clears it would then never fire, silently suppressing any later real join failure. Replaced the narrower defensive reset in requestEdit() (which the reviewer correctly noted is unreachable via the UI) with a broader one: any fresh join (status transitioning to 'joining', whether from requestEdit, a retry, or a reconnect) now clears the flag, since an earlier stop is moot once a new join starts either way. Tests: a new test forces the stuck-true state directly and confirms a fresh join clears it, checked to fail without the fix and pass with it. Verified: client-v3 262 Vitest (net unchanged - one test replaced), tsc/eslint/ prettier/build clean, full Playwright green in both chromium and firefox (235/235 each, no retries). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- .../config/script/CollabScriptEditor.test.ts | 27 ++++++++++++------- .../show/config/script/CollabScriptEditor.vue | 17 +++++++++--- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts index aac623165..aca361703 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts @@ -346,21 +346,26 @@ describe('CollabScriptEditor', () => { expect(w.find('.alert-stub').exists()).toBe(false); }); - it('requestEdit defensively clears stopRequested, so a stale flag can never suppress a later join failure', async () => { - // The isEditor watcher already clears stopRequested whenever isEditor goes - // false, which is the only way to reach the Edit button in the first place — - // so this can't happen through the UI. requestEdit() resets it anyway, as - // belt-and-braces; this test drives that line directly, by forcing the flag - // on before Edit is pressed, bypassing the normal (always-already-reset) flow. + it('a fresh join clears a stopRequested flag stuck true by a STOP that never registered', async () => { + // sendObj returning true only means the frame reached the socket, not that the + // server processed it (a drop right after, or the #1427 reconnect grace window + // keeping is_editor alive, would leave isEditor never flipping false) — in that + // case the isEditor watcher's own reset never fires, and stopRequested would + // stay stuck true, silently eating any later real join failure. Force exactly + // that stuck state, then prove a fresh join (ours or a resumed one) clears it + // regardless, since that earlier stop is moot once a new join starts. + becomeEditor(); vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); const w = await mountEditor(); + syncDraft({ '1': [snap('a')], '2': [] }); + await flushPromises(); (w.vm as unknown as { stopRequested: boolean }).stopRequested = true; - await button(w, 'Edit')!.trigger('click'); - becomeEditor(); + useScriptDraftStore().status = 'joining'; // a fresh join starts await flushPromises(); - useScriptDraftStore()._teardown(); // this join is rejected + expect((w.vm as unknown as { stopRequested: boolean }).stopRequested).toBe(false); + useScriptDraftStore()._teardown(); // ...and this one is rejected await flushPromises(); expect(w.find('.alert-stub').exists()).toBe(true); }); @@ -403,6 +408,10 @@ describe('CollabScriptEditor', () => { const { toast } = await import('@/js/toast'); expect(toast.error).toHaveBeenCalledWith('Cannot stop editing: not connected to the server'); + // A mutant that sets stopRequested unconditionally (not just on a successful + // send) would pass every other test in this file — nothing else exercises the + // failed-send branch's effect on the flag. + expect((w.vm as unknown as { stopRequested: boolean }).stopRequested).toBe(false); }); it('leaves the room when the component unmounts', async () => { diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue index 079ff80d2..73f010769 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.vue +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -249,10 +249,6 @@ async function goToPage(): Promise { } function requestEdit(): void { - // A fresh edit request supersedes any stop still awaiting confirmation (there is - // no realistic way to reach this while stopRequested is true — Edit isn't shown - // while isEditor is — but a stale flag must never suppress this join's own alert). - stopRequested.value = false; if (!sendObj({ OP: 'REQUEST_SCRIPT_EDIT', DATA: { collab: true } })) { toast.error('Cannot edit script: not connected to the server'); } @@ -291,6 +287,19 @@ watch(isEditor, async (now, was) => { } }); +// Belt-and-braces: if a STOP frame is sent but never actually registers server-side +// (e.g. the socket drops right after, or the #1427 reconnect grace window keeps +// is_editor alive across it), isEditor never flips false and the watcher above never +// resets stopRequested — it would stay stuck true, silently eating any later real +// join failure. Any fresh join (ours or a resumed one) means that earlier stop, if it +// was ever in flight, is moot, so clear the flag whenever one starts. +watch( + () => draftStore.status, + (status) => { + if (status === 'joining') stopRequested.value = false; + } +); + // Pages are never removed, but the stored page (or a page from before a rejoin) may be // past the last one that exists. watch([draftReady, maxPage], () => { From 3854cf0f0df87bb60738daca3b41f3b803230d94 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Mon, 26 Oct 2026 10:35:55 +0000 Subject: [PATCH 6/6] Address PR #1421 review round 4: stopRequested watch needs flush: 'sync' The round-3 watch(() => draftStore.status, ...) that clears stopRequested on a fresh join missed one reachable case: _rejoinAfterReconnect() calls _teardown() ('idle') then joinScriptRoom() ('joining') synchronously in one tick. If status was already 'joining' when that happens (Stop Editing clicked mid-join, the frame lost, a fast reconnect before the join watchdog fires), the net change over the tick is 'joining' -> 'joining'. Vue's default flush: 'pre' batches same-tick mutations and only invokes the callback if the net value differs from the value at the last invocation, so the watcher never sees the intermediate 'idle' and never fires - stopRequested stays stuck true, the exact failure mode this round-2/3 work set out to close. Fix: flush: 'sync' on that watch, so it sees every assignment rather than just the net result. Verified with the review's own repro: forcing stopRequested=true then assigning status='idle' then status='joining' in the same tick left the flag stuck true before this fix, and clears it after. New test pins this, alongside the existing test for the simpler (non-same-tick) case. Verified: client-v3 263 Vitest (+1), tsc/eslint/prettier/build clean, full Playwright green in both chromium and firefox (235/235 each, no retries). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TjTnfchFvvQvKZUZv2jpT8 --- .../config/script/CollabScriptEditor.test.ts | 24 +++++++++++++++++++ .../show/config/script/CollabScriptEditor.vue | 11 +++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts index aca361703..937112d02 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.test.ts @@ -370,6 +370,30 @@ describe('CollabScriptEditor', () => { expect(w.find('.alert-stub').exists()).toBe(true); }); + it('clears stopRequested even when a same-tick reconnect nets to no status change', async () => { + // _rejoinAfterReconnect() does _teardown() ('idle') then joinScriptRoom() + // ('joining') synchronously in one tick. If status was already 'joining' when + // that happens (Stop Editing clicked mid-join, the frame lost, a fast reconnect + // before the join watchdog fires), the net change over the tick is + // 'joining' -> 'joining' — a default pre-flush watcher never sees the + // intermediate 'idle' and so never fires, leaving the flag stuck exactly as + // before. The watch needs flush: 'sync' to see every assignment, not just the + // net result. + becomeEditor(); + vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); + const w = await mountEditor(); + const draft = useScriptDraftStore(); + draft.status = 'joining'; + await flushPromises(); + (w.vm as unknown as { stopRequested: boolean }).stopRequested = true; + + draft.status = 'idle'; + draft.status = 'joining'; // same-tick rejoin, as _rejoinAfterReconnect does + await flushPromises(); + + expect((w.vm as unknown as { stopRequested: boolean }).stopRequested).toBe(false); + }); + it('Stop Editing with another editor still present: losing editor status tears the draft down', async () => { becomeEditor(); vi.spyOn(useSystemStore(), 'isScriptEditor', 'get').mockReturnValue(true); diff --git a/client-v3/src/components/show/config/script/CollabScriptEditor.vue b/client-v3/src/components/show/config/script/CollabScriptEditor.vue index 73f010769..5ccd2dcfc 100644 --- a/client-v3/src/components/show/config/script/CollabScriptEditor.vue +++ b/client-v3/src/components/show/config/script/CollabScriptEditor.vue @@ -292,12 +292,19 @@ watch(isEditor, async (now, was) => { // is_editor alive across it), isEditor never flips false and the watcher above never // resets stopRequested — it would stay stuck true, silently eating any later real // join failure. Any fresh join (ours or a resumed one) means that earlier stop, if it -// was ever in flight, is moot, so clear the flag whenever one starts. +// was ever in flight, is moot, so clear the flag whenever one starts. flush: 'sync' is +// required, not cosmetic: _rejoinAfterReconnect() does _teardown() (-> 'idle') then +// joinScriptRoom() (-> 'joining') synchronously in one tick, and if status was already +// 'joining' when that happens (STOP clicked mid-join, frame lost, fast reconnect before +// the join watchdog fires), the net change over the tick is 'joining' -> 'joining' — +// the default pre-flush watcher would never see an intermediate value and so never +// fire, leaving the flag stuck exactly as before. watch( () => draftStore.status, (status) => { if (status === 'joining') stopRequested.value = false; - } + }, + { flush: 'sync' } ); // Pages are never removed, but the stored page (or a page from before a rejoin) may be