From a52987547e8b834b1a8a18ab2674a0b64d74359f Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Fri, 12 Jun 2026 15:48:24 -0700 Subject: [PATCH 1/8] simultaneous-edit protection for workspace files --- e2e-tests/fixtures/WorkspaceSaveConflict.ts | 76 +++++++ .../tests/workspace-edit-protection.test.ts | 206 ++++++++++++++++++ e2e-tests/tests/workspace.test.ts | 24 +- package-lock.json | 14 ++ package.json | 1 + .../modals/WorkspaceSaveConflictModal.svelte | 175 +++++++++++++++ .../CommandPanel/CommandPanel.svelte | 2 + .../CommandPanel/SelectedCommand.svelte | 110 +++++----- .../sequencing/SequenceEditor.svelte | 17 +- .../sequencing/form/ArgEditor.svelte | 3 + .../sequencing/form/BooleanEditor.svelte | 2 +- .../sequencing/form/EnumEditor.svelte | 5 +- .../sequencing/form/NumEditor.svelte | 2 +- .../sequencing/form/StringEditor.svelte | 8 +- src/components/ui/TextEditor.svelte | 17 +- .../workspace/WorkspaceDiffViewer.svelte | 130 +++++++++++ .../workspace/WorkspaceRightPanel.svelte | 1 + .../workspaces/[workspaceId]/+page.svelte | 158 +++++++++++++- src/stores/activeDocument.test.ts | 90 ++++++++ src/stores/activeDocument.ts | 53 ++++- src/utilities/codemirror/readOnly.test.ts | 46 ++++ src/utilities/codemirror/readOnly.ts | 19 ++ src/utilities/effects.ts | 52 +++-- src/utilities/modal.ts | 64 ++++++ src/utilities/requests.test.ts | 147 +++++++++++++ src/utilities/requests.ts | 119 ++++++++++ src/utilities/workspaces.test.ts | 15 +- src/utilities/workspaces.ts | 31 ++- 28 files changed, 1486 insertions(+), 101 deletions(-) create mode 100644 e2e-tests/fixtures/WorkspaceSaveConflict.ts create mode 100644 e2e-tests/tests/workspace-edit-protection.test.ts create mode 100644 src/components/modals/WorkspaceSaveConflictModal.svelte create mode 100644 src/components/workspace/WorkspaceDiffViewer.svelte create mode 100644 src/stores/activeDocument.test.ts create mode 100644 src/utilities/codemirror/readOnly.test.ts create mode 100644 src/utilities/codemirror/readOnly.ts create mode 100644 src/utilities/requests.test.ts diff --git a/e2e-tests/fixtures/WorkspaceSaveConflict.ts b/e2e-tests/fixtures/WorkspaceSaveConflict.ts new file mode 100644 index 0000000000..d3ea57ae2e --- /dev/null +++ b/e2e-tests/fixtures/WorkspaceSaveConflict.ts @@ -0,0 +1,76 @@ +import type { Locator, Page } from '@playwright/test'; +import { expect } from '@playwright/test'; + +/** + * Page Object Model for the workspace save-conflict modal (`WorkspaceSaveConflictModal`), + * shown when a save is rejected by the optimistic-concurrency check (`412`). Covers both + * the "conflict" variant (the file changed underneath) and the "deleted" variant (the file + * was deleted/moved underneath). Selectors are scoped to the modal root. + */ +export class WorkspaceSaveConflict { + cancelButton!: Locator; + conflictTitle!: Locator; + deletedTitle!: Locator; + discardButton!: Locator; + mineHeader!: Locator; + modal!: Locator; + recreateButton!: Locator; + takeMineButton!: Locator; + takeTheirsButton!: Locator; + theirsHeader!: Locator; + + constructor(public page: Page) { + this.updatePage(page); + } + + async cancel(): Promise { + await this.cancelButton.click(); + await expect(this.modal).toBeHidden(); + } + + async discardAndClose(): Promise { + await this.discardButton.click(); + await expect(this.modal).toBeHidden(); + } + + async recreate(): Promise { + await this.recreateButton.click(); + await expect(this.modal).toBeHidden(); + } + + async takeMine(): Promise { + await this.takeMineButton.click(); + await expect(this.modal).toBeHidden(); + } + + async takeTheirs(): Promise { + await this.takeTheirsButton.click(); + await expect(this.modal).toBeHidden(); + } + + updatePage(page: Page): void { + this.page = page; + this.modal = page.locator('#modal-container'); + this.conflictTitle = this.modal.getByText('This file changed since you opened it'); + this.deletedTitle = this.modal.getByText('File deleted or moved'); + this.theirsHeader = this.modal.getByText('Theirs (server)'); + this.mineHeader = this.modal.getByText('Mine (your edits)'); + this.takeTheirsButton = this.modal.getByRole('button', { name: 'Take theirs (discard mine)' }); + this.takeMineButton = this.modal.getByRole('button', { name: 'Take mine (overwrite)' }); + this.cancelButton = this.modal.getByRole('button', { name: 'Cancel' }); + this.recreateButton = this.modal.getByRole('button', { name: 'Recreate file' }); + this.discardButton = this.modal.getByRole('button', { name: 'Discard & close' }); + } + + /** Wait for the "conflict" variant (read-only diff of theirs vs mine). */ + async waitForConflict(): Promise { + await expect(this.conflictTitle).toBeVisible({ timeout: 15000 }); + await expect(this.theirsHeader).toBeVisible(); + await expect(this.mineHeader).toBeVisible(); + } + + /** Wait for the "deleted/moved" variant (single read-only pane of mine). */ + async waitForDeleted(): Promise { + await expect(this.deletedTitle).toBeVisible({ timeout: 15000 }); + } +} diff --git a/e2e-tests/tests/workspace-edit-protection.test.ts b/e2e-tests/tests/workspace-edit-protection.test.ts new file mode 100644 index 0000000000..34457e2a52 --- /dev/null +++ b/e2e-tests/tests/workspace-edit-protection.test.ts @@ -0,0 +1,206 @@ +import test, { expect } from '@playwright/test'; +import { Dictionaries } from '../fixtures/Dictionaries.js'; +import { Parcels } from '../fixtures/Parcels.js'; +import { Workspace } from '../fixtures/Workspace.js'; +import { WorkspaceSaveConflict } from '../fixtures/WorkspaceSaveConflict.js'; +import { Workspaces } from '../fixtures/Workspaces.js'; +import { setupTest, teardownTest, type BrowserSetupResult } from '../utilities/api.js'; +import { generateRandomName } from '../utilities/helpers.js'; + +// Simultaneous-edit protection: a save is rejected (412) when the file changed underneath the +// editor since it was opened. We simulate the "other editor" with a second browser context for +// the same 'test' user editing the same workspace file. + +let setup: BrowserSetupResult; // primary editor — the one under test +let setupOther: BrowserSetupResult; // concurrent editor — changes the file first + +let dictionaries: Dictionaries; +let parcels: Parcels; +let workspaces: Workspaces; +let workspace: Workspace; // primary page +let otherWorkspace: Workspace; // concurrent page (same workspace) +let conflict: WorkspaceSaveConflict; +let workspaceId: string; +let workspaceName: string; + +test.beforeAll(async ({ baseURL, browser }) => { + test.setTimeout(120000); + + setup = await setupTest(browser, { model: false }); + dictionaries = new Dictionaries(setup.page); + parcels = new Parcels(setup.page); + workspaces = new Workspaces(setup.page, parcels, baseURL); + + await dictionaries.goto(); + await dictionaries.createCommandDictionary(); + await parcels.goto(); + await parcels.createParcel(dictionaries.commandDictionaryName, baseURL); + + await workspaces.goto(); + workspaceId = await workspaces.createWorkspace(); + workspaceName = workspaces.workspaceName; + workspace = new Workspace(setup.page, workspaceId, workspaceName, baseURL); + conflict = new WorkspaceSaveConflict(setup.page); + + // Second context (same 'test' user) editing the same workspace concurrently. + setupOther = await setupTest(browser, { model: false }); + otherWorkspace = new Workspace(setupOther.page, workspaceId, workspaceName, baseURL); + + await workspace.goto(); +}); + +test.afterAll(async () => { + await workspaces.goto(); + await workspaces.deleteWorkspace(workspaceName); + await parcels.goto(); + await parcels.deleteParcel(); + await dictionaries.goto(); + await dictionaries.deleteCommandDictionary(); + + await teardownTest(setup); + await teardownTest(setupOther); +}); + +/** Create a sequence file on the primary page and open it (capturing its base version). */ +async function createAndOpenSequence(): Promise { + const { sequenceName } = await workspace.createSequence(undefined, `${generateRandomName()}.seq`); + await workspace.searchForFileAndWait(sequenceName); + await workspace.clickFile(sequenceName); + await expect(workspace.saveSequenceButton).toBeVisible({ timeout: 10000 }); + return sequenceName; +} + +/** The concurrent editor opens the same file and saves a change, advancing the server version. */ +async function otherEditorSaves(sequenceName: string, content: string): Promise { + await otherWorkspace.goto(); + await otherWorkspace.searchForFileAndWait(sequenceName); + await otherWorkspace.clickFile(sequenceName); + await expect(otherWorkspace.saveSequenceButton).toBeVisible({ timeout: 10000 }); + await otherWorkspace.fillSequenceContent(content); + await otherWorkspace.saveSequence(); +} + +/** Edit on the primary page and click Save, expecting the concurrency check to reject it. */ +async function editAndSaveExpectingConflict(content: string): Promise { + await workspace.fillSequenceContent(content); + await expect(workspace.saveSequenceButton).toBeEnabled(); + await workspace.saveSequenceButton.click(); +} + +test.describe.serial('Workspace simultaneous-edit protection', () => { + test('consecutive saves of the same file succeed (token round-trips on PUT)', async () => { + // No concurrent editor here: this confirms a save advances the stored token so the next + // save against the same editor does not spuriously conflict. + await createAndOpenSequence(); + + await workspace.fillSequenceContent('// first change'); + await workspace.saveSequence(); + await expect(workspace.saveSequenceButton).toBeDisabled(); + + await workspace.fillSequenceContent('// second change'); + await workspace.saveSequence(); + await expect(workspace.saveSequenceButton).toBeDisabled(); + }); + + test('a stale save shows the conflict modal with a diff', async () => { + const sequenceName = await createAndOpenSequence(); + await otherEditorSaves(sequenceName, '// their change'); + + await editAndSaveExpectingConflict('// my change'); + + await conflict.waitForConflict(); + + // Cancel leaves the document dirty (Save still enabled) and keeps the stale token. + await conflict.cancel(); + await expect(workspace.saveSequenceButton).toBeEnabled(); + + // Resolve the still-dirty document so the next test starts from a clean editor: + // saving again re-checks the (still stale) token, then take theirs to rebase clean. + await workspace.saveSequenceButton.click(); + await conflict.waitForConflict(); + await conflict.takeTheirs(); + await expect(workspace.saveSequenceButton).toBeDisabled(); + }); + + test('"Take theirs" rebases the editor and the next save succeeds', async () => { + const sequenceName = await createAndOpenSequence(); + await otherEditorSaves(sequenceName, '// their change'); + + await editAndSaveExpectingConflict('// my change'); + await conflict.waitForConflict(); + await conflict.takeTheirs(); + + // Editor now reflects the server version; saving a fresh edit succeeds (token advanced, + // no immediate re-conflict). + await workspace.fillSequenceContent('// after taking theirs'); + await workspace.saveSequence(); + await expect(workspace.saveSequenceButton).toBeDisabled(); + }); + + test('"Take mine (overwrite)" saves the local version and the next save succeeds', async () => { + const sequenceName = await createAndOpenSequence(); + await otherEditorSaves(sequenceName, '// their change'); + + await editAndSaveExpectingConflict('// my change'); + await conflict.waitForConflict(); + await conflict.takeMine(); + + // Force-overwrite saved successfully and advanced the token. + await workspace.waitForToast('Workspace File Saved Successfully'); + await expect(workspace.saveSequenceButton).toBeDisabled(); + + await workspace.fillSequenceContent('// after taking mine'); + await workspace.saveSequence(); + await expect(workspace.saveSequenceButton).toBeDisabled(); + }); + + test('Ctrl/Cmd+S is ignored while the conflict modal is open (no stacked save)', async () => { + const sequenceName = await createAndOpenSequence(); + await otherEditorSaves(sequenceName, '// their change'); + + await editAndSaveExpectingConflict('// my change'); + await conflict.waitForConflict(); + + // The global save shortcut must not fire another save / stack a second modal. + await setup.page.keyboard.press('ControlOrMeta+s'); + await expect(conflict.takeMineButton).toHaveCount(1); + await expect(conflict.conflictTitle).toBeVisible(); + + await conflict.takeTheirs(); + }); + + test('a file deleted underneath shows the deleted/moved variant — Recreate restores it', async () => { + const sequenceName = await createAndOpenSequence(); + + // Concurrent editor deletes the file out from under the open editor. + await otherWorkspace.goto(); + await otherWorkspace.searchForFileAndWait(sequenceName); + await otherWorkspace.deleteFile(sequenceName); + + await editAndSaveExpectingConflict('// my change after delete'); + await conflict.waitForDeleted(); + await conflict.recreate(); + + await workspace.waitForToast('Workspace File Saved Successfully'); + // The recreated file is back in the tree. + await workspace.searchForFileAndWait(sequenceName); + await expect(workspace.getFileRow(sequenceName)).toBeVisible(); + }); + + test('the deleted/moved variant — "Discard & close" closes the document', async () => { + const sequenceName = await createAndOpenSequence(); + + await otherWorkspace.goto(); + await otherWorkspace.searchForFileAndWait(sequenceName); + await otherWorkspace.deleteFile(sequenceName); + + await editAndSaveExpectingConflict('// my change after delete'); + await conflict.waitForDeleted(); + await conflict.discardAndClose(); + + // The unsaved edits are discarded — the editor is no longer dirty (Save disabled) and + // the discarded content is gone. + await expect(workspace.saveSequenceButton).toBeDisabled(); + await expect(setup.page.locator('.cm-content').first()).not.toContainText('my change after delete'); + }); +}); diff --git a/e2e-tests/tests/workspace.test.ts b/e2e-tests/tests/workspace.test.ts index b57a672bd6..7fa73aa031 100644 --- a/e2e-tests/tests/workspace.test.ts +++ b/e2e-tests/tests/workspace.test.ts @@ -603,17 +603,31 @@ test.describe.serial('Workspace', () => { }); test('Toggle file read-only and verify editor is locked', async () => { - // Create a sequence file to test with - const { sequenceName } = await workspace.createSequence(undefined, `${generateRandomName()}.seq`); + // Use the adaptation's input-sequence extension so the file opens as a sequence with the + // Selected Command panel (a plain `.seq` would open as generic text — no command panel). + const { sequenceName } = await workspace.createSequence(undefined, `${generateRandomName()}.seqN.txt`); await workspace.searchForFileAndWait(sequenceName); await workspace.clickFile(sequenceName); // Wait for the editor to load and the file metadata banner to appear await expect(workspace.readOnlyCheckbox).toBeVisible({ timeout: 10000 }); + // Add a command so the Selected Command panel renders editable argument inputs. + await workspace.fillSequenceContent('C FSW_CMD_0 "ON" true 0.5'); + + // The Selected Command panel (right side) shows the command's argument editors; + // float_arg_0 renders as a numeric input (spinbutton) labeled by its arg name. + const commandArgInput = setup.page.getByRole('spinbutton', { name: 'float_arg_0' }); + await expect(commandArgInput).toBeVisible({ timeout: 10000 }); + // Verify the file is initially editable — title should NOT contain "(Read only)" await expect(setup.page.getByText('(Read only)')).not.toBeVisible(); await expect(workspace.saveSequenceButton).toBeVisible(); + // ...and the command argument inputs are interactive. + await expect(commandArgInput).toBeEnabled(); + + // Persist so the document is clean before toggling read-only / deleting later. + await workspace.saveSequence(); // Toggle read-only ON await workspace.readOnlyCheckbox.click(); @@ -625,6 +639,10 @@ test.describe.serial('Workspace', () => { // Verify the Save button is hidden (read-only files can't be saved) await expect(workspace.saveSequenceButton).not.toBeVisible(); + // Verify the Selected Command panel argument inputs are disabled while read-only — + // `EditorState.readOnly` alone wouldn't stop the form-builder from editing the document. + await expect(commandArgInput).toBeDisabled(); + // Verify the editor rejects input — type something and confirm content didn't change await workspace.sequenceEditor.click(); await setup.page.keyboard.type('this should not appear'); @@ -639,6 +657,8 @@ test.describe.serial('Workspace', () => { // Verify the Save button reappears await expect(workspace.saveSequenceButton).toBeVisible(); + // ...and the command argument inputs are interactive again. + await expect(commandArgInput).toBeEnabled(); // Cleanup await workspace.searchForFileAndWait(sequenceName); diff --git a/package-lock.json b/package-lock.json index 169670a047..8c5cfd896d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,6 +13,7 @@ "@codemirror/lang-json": "^6.0.1", "@codemirror/language": "^6.10.1", "@codemirror/lint": "^6.5.0", + "@codemirror/merge": "^6.12.2", "@codemirror/view": "^6.38.2", "@fontsource/jetbrains-mono": "^5.0.19", "@lezer/common": "^1.2.3", @@ -313,6 +314,19 @@ "crelt": "^1.0.5" } }, + "node_modules/@codemirror/merge": { + "version": "6.12.2", + "resolved": "https://registry.npmjs.org/@codemirror/merge/-/merge-6.12.2.tgz", + "integrity": "sha512-V8JvyAPjHbPupqP7BeMcsdsYCbyPij74jxIbaIJDORI+VZzW44zFmon8bF+oxGWvOKhcRmkiUMXd8MxHr3YA2w==", + "license": "MIT", + "dependencies": { + "@codemirror/language": "^6.0.0", + "@codemirror/state": "^6.0.0", + "@codemirror/view": "^6.17.0", + "@lezer/highlight": "^1.0.0", + "style-mod": "^4.1.0" + } + }, "node_modules/@codemirror/search": { "version": "6.5.10", "resolved": "https://registry.npmjs.org/@codemirror/search/-/search-6.5.10.tgz", diff --git a/package.json b/package.json index 4f032a089d..240b5e9c77 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "@codemirror/lang-json": "^6.0.1", "@codemirror/language": "^6.10.1", "@codemirror/lint": "^6.5.0", + "@codemirror/merge": "^6.12.2", "@codemirror/view": "^6.38.2", "@fontsource/jetbrains-mono": "^5.0.19", "@lezer/common": "^1.2.3", diff --git a/src/components/modals/WorkspaceSaveConflictModal.svelte b/src/components/modals/WorkspaceSaveConflictModal.svelte new file mode 100644 index 0000000000..4e6c778dd4 --- /dev/null +++ b/src/components/modals/WorkspaceSaveConflictModal.svelte @@ -0,0 +1,175 @@ + + + + + + + {variant === 'deleted' && !loadError ? 'File deleted or moved' : 'This file changed since you opened it'} + + +
+ {#if loadError} + {fileName} was changed by user @{editedByText} + {whenText ? ` ${whenText}` : ''}, but the latest version couldn't be loaded to compare. + {:else if variant === 'deleted'} + {fileName} was deleted or moved by user @{editedByText} + {whenText ? ` ${whenText}` : ''}. Your unsaved changes are shown below. + {:else if allowMerge} + {fileName} was changed by user @{editedByText} + {whenText ? ` ${whenText}` : ''}. Edit your version on the right — use the arrows to pull in theirs — then + Save, or take theirs to discard your changes. + {:else} + {fileName} was changed by user @{editedByText} + {whenText ? ` ${whenText}` : ''}. Review the differences, then choose how to resolve. + {/if} +
+ +
+ {#if isLoading} +
+ + Loading the latest version… +
+ {:else if loadError} +
+ + Couldn't load the latest version of this file. Your unsaved changes are preserved — retry, or cancel and + try saving again. +
+ {:else if variant === 'deleted'} + + {:else} + + {/if} +
+ + +
+ {#if loadError} + + + {:else if variant === 'deleted'} + + + {:else} + + + {#if allowMerge} + + {:else} + + {/if} + {/if} +
+ + diff --git a/src/components/sequencing/CommandPanel/CommandPanel.svelte b/src/components/sequencing/CommandPanel/CommandPanel.svelte index c777a1f5e2..948f656c05 100644 --- a/src/components/sequencing/CommandPanel/CommandPanel.svelte +++ b/src/components/sequencing/CommandPanel/CommandPanel.svelte @@ -17,6 +17,7 @@ export let phoenixContext: PhoenixContext; export let commandInfoMapper: CommandInfoMapper; export let editorSequenceView: EditorView; + export let readOnly: boolean = false; enum CommandPanelTabs { COMMAND = 'command', @@ -126,6 +127,7 @@ {commandDictionary} {commandInfoMapper} {editorSequenceView} + {readOnly} {timeTagNode} on:selectCommandDefinition={onSelectCommandDefinition} /> diff --git a/src/components/sequencing/CommandPanel/SelectedCommand.svelte b/src/components/sequencing/CommandPanel/SelectedCommand.svelte index 822efa62cc..460ce0aef7 100644 --- a/src/components/sequencing/CommandPanel/SelectedCommand.svelte +++ b/src/components/sequencing/CommandPanel/SelectedCommand.svelte @@ -28,6 +28,7 @@ export let commandNameNode: SyntaxNode | null = null; export let commandNode: SyntaxNode | null = null; export let editorSequenceView: EditorView; + export let readOnly: boolean = false; export let timeTagNode: TimeTagInfo = null; export let variablesInScope: string[] = []; @@ -117,65 +118,70 @@ {/if} {#if !!commandNode} - {#if commandInfoMapper.nodeTypeHasArguments(commandNode)} - {#if !!commandDef} - {#each editorArgInfoArray as argInfo} - setInEditor(editorSequenceView, token, val), 250)} - addDefaultArgs={(commandNodeToAddArgs, missingArgDefs) => - addDefaultArgs( - commandDictionary, - editorSequenceView, - commandNodeToAddArgs, - missingArgDefs, - commandInfoMapper, - )} - /> - {/each} - - {#if missingArgDefArray.length} -
- { - if (commandNode) { - addDefaultArgs( - commandDictionary, - editorSequenceView, - commandNode, - missingArgDefArray, - commandInfoMapper, - ); - } - }} + +
+ {#if commandInfoMapper.nodeTypeHasArguments(commandNode)} + {#if !!commandDef} + {#each editorArgInfoArray as argInfo} + setInEditor(editorSequenceView, token, val), 250)} + addDefaultArgs={(commandNodeToAddArgs, missingArgDefs) => + addDefaultArgs( + commandDictionary, + editorSequenceView, + commandNodeToAddArgs, + missingArgDefs, + commandInfoMapper, + )} /> + {/each} + + {#if missingArgDefArray.length} +
+ { + if (commandNode) { + addDefaultArgs( + commandDictionary, + editorSequenceView, + commandNode, + missingArgDefArray, + commandInfoMapper, + ); + } + }} + /> +
+ {/if} + {:else} +
+
{commandName ?? ''}
+
Command type is not present in dictionary
{/if} {:else}
-
{commandName ?? ''}
+
{`${formatTypeName(commandNode.name)} Name`}
+
+ { + if (commandNameNode) { + setInEditor(editorSequenceView, commandNameNode, val); + } + }} + /> +
-
Command type is not present in dictionary
{/if} - {:else} -
-
{`${formatTypeName(commandNode.name)} Name`}
-
- { - if (commandNameNode) { - setInEditor(editorSequenceView, commandNameNode, val); - } - }} - /> -
-
- {/if} +
{:else}
Select a command or open the diff --git a/src/components/sequencing/SequenceEditor.svelte b/src/components/sequencing/SequenceEditor.svelte index a55d5b515b..99a890d7de 100644 --- a/src/components/sequencing/SequenceEditor.svelte +++ b/src/components/sequencing/SequenceEditor.svelte @@ -22,6 +22,7 @@ import type { LintDiagnostic } from '../../types/errors'; import type { WorkspaceFileMetadata } from '../../types/workspace-tree-view'; import { getLintDiagnostics } from '../../utilities/codemirror/lint'; + import { readOnlyChangeGuard } from '../../utilities/codemirror/readOnly'; import { blockTheme } from '../../utilities/codemirror/themes/block'; import { phoenixResources } from '../../utilities/sequence-editor/adaptation-resources'; import { showFailureToast, showSuccessToast } from '../../utilities/toast'; @@ -160,6 +161,9 @@ effects: compartmentReadonly.reconfigure([ EditorState.readOnly.of(!isEditable), EditorView.editable.of(isEditable), + // Block programmatic edits (lint fixes, command panel, sanitizer) that readOnly + // misses, but allow the editor's own 'file.open' content sync. + ...(isEditable ? [] : [readOnlyChangeGuard(['file.open'])]), ]), }); } @@ -385,7 +389,11 @@ EditorView.updateListener.of(viewUpdate => dispatchLintChange(viewUpdate.view)), blockTheme, compartmentAdaptation.of(inputEditorExtension), - compartmentReadonly.of([EditorState.readOnly.of(readOnly || previewOnly || isLoading)]), + compartmentReadonly.of( + readOnly || previewOnly || isLoading + ? [EditorState.readOnly.of(true), readOnlyChangeGuard(['file.open'])] + : [EditorState.readOnly.of(false)], + ), EditorView.updateListener.of(viewUpdate => { for (const tr of viewUpdate.transactions) { if (tr.annotation(Transaction.userEvent) === 'sanitize.smartQuotes') { @@ -496,7 +504,12 @@ {#if showCommandFormBuilder} {#if phoenixContext && phoenixContext.commandDictionary !== null} - + {:else} diff --git a/src/components/sequencing/form/ArgEditor.svelte b/src/components/sequencing/form/ArgEditor.svelte index 933763673b..efea7f217a 100644 --- a/src/components/sequencing/form/ArgEditor.svelte +++ b/src/components/sequencing/form/ArgEditor.svelte @@ -28,6 +28,7 @@ export let setInEditor: (token: SyntaxNode, val: string) => void; export let addDefaultArgs: (commandNode: SyntaxNode, argDefs: FswCommandArgument[]) => void; export let commandInfoMapper: CommandInfoMapper; + export let disabled: boolean = false; export let variablesInScope: string[]; let argDef: FswCommandArgument | undefined = undefined; @@ -98,6 +99,7 @@
Reference
{ if (argInfo.node) { @@ -110,6 +112,7 @@ { if (argInfo.node) { diff --git a/src/components/sequencing/form/BooleanEditor.svelte b/src/components/sequencing/form/BooleanEditor.svelte index 3b4d02f675..902c444f72 100644 --- a/src/components/sequencing/form/BooleanEditor.svelte +++ b/src/components/sequencing/form/BooleanEditor.svelte @@ -15,7 +15,7 @@
- {#each ['false', 'true'] as ev} {/each} diff --git a/src/components/sequencing/form/EnumEditor.svelte b/src/components/sequencing/form/EnumEditor.svelte index 6a5f536f1b..77066137fe 100644 --- a/src/components/sequencing/form/EnumEditor.svelte +++ b/src/components/sequencing/form/EnumEditor.svelte @@ -9,6 +9,7 @@ export let argDef: FswCommandArgumentEnum; export let commandDictionary: CommandDictionary | null = null; + export let disabled: boolean = false; export let initVal: string; export let setInEditor: (val: string) => void; @@ -37,14 +38,16 @@
{#if enumValues.length > SEARCH_THRESHOLD} {:else} - {#if !isValueInEnum} {/if} diff --git a/src/components/sequencing/form/NumEditor.svelte b/src/components/sequencing/form/NumEditor.svelte index 33d9ca07bf..6ab1ca7503 100644 --- a/src/components/sequencing/form/NumEditor.svelte +++ b/src/components/sequencing/form/NumEditor.svelte @@ -31,7 +31,7 @@
- + {#if typeof min === 'number' && typeof max === 'number' && (valFloat < min || valFloat > max)}
{/if} {#if !!commandNode} -
{#if commandInfoMapper.nodeTypeHasArguments(commandNode)} {#if !!commandDef} diff --git a/src/components/workspace/WorkspaceDiffViewer.svelte b/src/components/workspace/WorkspaceDiffViewer.svelte index 23531cb511..c1194ca50a 100644 --- a/src/components/workspace/WorkspaceDiffViewer.svelte +++ b/src/components/workspace/WorkspaceDiffViewer.svelte @@ -26,12 +26,13 @@ export let type: WorkspaceContentType | null = null; let container: HTMLDivElement; + let isSinglePane: boolean = true; let mergeView: MergeView | null = null; let singleView: EditorView | null = null; $: isSinglePane = theirs === null; - /** Current content of the editable "Mine" pane (for the merge flow). */ + /** Get the current contents of the editable "Mine" pane (for the merge flow). */ export function getMergedContent(): string { return mergeView?.b.state.doc.toString() ?? mine; } @@ -46,9 +47,9 @@ return []; } - // A truly read-only pane. The guard blocks programmatic edits that `readOnly` misses - // (lint quick-fixes, sanitizers), keeping the diff faithful to the bytes; the theme hides - // the now-dead lint quick-fix buttons. + // Blocks programmatic edits that `readOnly` codemirror flag misses + // (lint quick-fixes, sanitizers), keeping the diff faithful to the bytes. + // The theme hides the irrelevant lint quick-fix buttons. function readOnlyExtensions(): Extension[] { return [ lineNumbers(), @@ -89,7 +90,6 @@ return; } - // Divider between the two panes (matches the header divider). const dividerTheme = EditorView.theme({ '&': { borderLeft: '1px solid hsl(var(--border))' } }); // `a` = Theirs (read-only), `b` = Mine (editable in merge mode). revertControls are the diff --git a/src/utilities/modal.ts b/src/utilities/modal.ts index de71356485..4920af87f5 100644 --- a/src/utilities/modal.ts +++ b/src/utilities/modal.ts @@ -1719,17 +1719,12 @@ export async function showExpansionPanelModal(user: User | null): Promise finish()); conflictModal.$on('takeMine', (e: CustomEvent<{ content: string; token: string | null }>) => finish({ action: 'take-mine', content: e.detail.content, token: e.detail.token }), From 02b48ac716be128194ed26567cb23b481dc1bebf Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Wed, 8 Jul 2026 09:41:44 -0700 Subject: [PATCH 3/8] refactor: address save-conflict review feedback and redesign the conflict modal - Rename reqWorkspaceWithMeta -> reqWorkspaceWithEtag (+ return type) to avoid confusion with reqWorkspaceMetadata - Rename the opaque "token" vocabulary to "etag" across the activeDocument store (baseToken -> baseEtag), modal resolution, effects, and consumers - Redesign WorkspaceSaveConflictModal: gray footer bar with white "Keep theirs / Keep mine" buttons, per-action consequence sublines, and a "Keep editing" escape; unify the conflict / deleted / load-error footers - Show an "N changed lines" summary in the diff header (emitted from the diff viewer) - Deleted variant: destructive text cue on "Discard & close" + "Recreate file", each with a consequence subline - Re-fetch the latest server version when the user picks "Keep theirs" so it never applies a stale snapshot from when the modal opened - Make "Keep theirs" undoable: rebaseContent() on both editors records the rebase in the undo history so Cmd-Z restores the discarded edits - Add e2e coverage for the undo behavior; update conflict fixture selectors/labels --- e2e-tests/fixtures/WorkspaceSaveConflict.ts | 8 +- .../tests/workspace-edit-protection.test.ts | 24 +++- .../modals/WorkspaceSaveConflictModal.svelte | 106 ++++++++++++------ .../sequencing/SequenceEditor.svelte | 14 +++ src/components/ui/TextEditor.svelte | 14 +++ .../workspace/WorkspaceDiffViewer.svelte | 19 +++- .../workspaces/[workspaceId]/+page.svelte | 19 +++- src/stores/activeDocument.test.ts | 32 +++--- src/stores/activeDocument.ts | 28 ++--- src/utilities/effects.ts | 4 +- src/utilities/modal.ts | 12 +- src/utilities/requests.test.ts | 18 +-- src/utilities/requests.ts | 10 +- src/utilities/workspaces.test.ts | 8 +- src/utilities/workspaces.ts | 6 +- 15 files changed, 218 insertions(+), 104 deletions(-) diff --git a/e2e-tests/fixtures/WorkspaceSaveConflict.ts b/e2e-tests/fixtures/WorkspaceSaveConflict.ts index d3ea57ae2e..b2bb493e85 100644 --- a/e2e-tests/fixtures/WorkspaceSaveConflict.ts +++ b/e2e-tests/fixtures/WorkspaceSaveConflict.ts @@ -51,13 +51,13 @@ export class WorkspaceSaveConflict { updatePage(page: Page): void { this.page = page; this.modal = page.locator('#modal-container'); - this.conflictTitle = this.modal.getByText('This file changed since you opened it'); + this.conflictTitle = this.modal.getByText('This file was updated by someone else'); this.deletedTitle = this.modal.getByText('File deleted or moved'); this.theirsHeader = this.modal.getByText('Theirs (server)'); this.mineHeader = this.modal.getByText('Mine (your edits)'); - this.takeTheirsButton = this.modal.getByRole('button', { name: 'Take theirs (discard mine)' }); - this.takeMineButton = this.modal.getByRole('button', { name: 'Take mine (overwrite)' }); - this.cancelButton = this.modal.getByRole('button', { name: 'Cancel' }); + this.takeTheirsButton = this.modal.getByRole('button', { name: 'Keep theirs' }); + this.takeMineButton = this.modal.getByRole('button', { name: 'Keep mine' }); + this.cancelButton = this.modal.getByRole('button', { name: 'Keep editing' }); this.recreateButton = this.modal.getByRole('button', { name: 'Recreate file' }); this.discardButton = this.modal.getByRole('button', { name: 'Discard & close' }); } diff --git a/e2e-tests/tests/workspace-edit-protection.test.ts b/e2e-tests/tests/workspace-edit-protection.test.ts index 34457e2a52..50b76cd2e1 100644 --- a/e2e-tests/tests/workspace-edit-protection.test.ts +++ b/e2e-tests/tests/workspace-edit-protection.test.ts @@ -122,7 +122,7 @@ test.describe.serial('Workspace simultaneous-edit protection', () => { await expect(workspace.saveSequenceButton).toBeDisabled(); }); - test('"Take theirs" rebases the editor and the next save succeeds', async () => { + test('"Keep theirs" rebases the editor and the next save succeeds', async () => { const sequenceName = await createAndOpenSequence(); await otherEditorSaves(sequenceName, '// their change'); @@ -137,7 +137,27 @@ test.describe.serial('Workspace simultaneous-edit protection', () => { await expect(workspace.saveSequenceButton).toBeDisabled(); }); - test('"Take mine (overwrite)" saves the local version and the next save succeeds', async () => { + test('"Keep theirs" is undoable — Ctrl/Cmd+Z restores the discarded edits', async () => { + const sequenceName = await createAndOpenSequence(); + await otherEditorSaves(sequenceName, '// their change'); + + await editAndSaveExpectingConflict('// my change'); + await conflict.waitForConflict(); + await conflict.takeTheirs(); + + // Right after taking theirs the editor shows the server version. + const editor = setup.page.locator('.cm-content').first(); + await expect(editor).toContainText('// their change'); + + // The rebase is recorded in the undo history, so a single undo restores the discarded edits + // and the document goes dirty again (restored content differs from the new server baseline). + await editor.click(); + await setup.page.keyboard.press('ControlOrMeta+z'); + await expect(editor).toContainText('// my change'); + await expect(workspace.saveSequenceButton).toBeEnabled(); + }); + + test('"Keep mine" saves the local version and the next save succeeds', async () => { const sequenceName = await createAndOpenSequence(); await otherEditorSaves(sequenceName, '// their change'); diff --git a/src/components/modals/WorkspaceSaveConflictModal.svelte b/src/components/modals/WorkspaceSaveConflictModal.svelte index ea9bd429f6..c9c7bd7ac5 100644 --- a/src/components/modals/WorkspaceSaveConflictModal.svelte +++ b/src/components/modals/WorkspaceSaveConflictModal.svelte @@ -13,7 +13,6 @@ import WorkspaceDiffViewer from '../workspace/WorkspaceDiffViewer.svelte'; import Modal from './Modal.svelte'; import ModalContent from './ModalContent.svelte'; - import ModalFooter from './ModalFooter.svelte'; import ModalHeader from './ModalHeader.svelte'; export let allowMerge: boolean = false; @@ -32,21 +31,23 @@ close: void; discard: void; recreate: void; - takeMine: { content: string; token: string | null }; - takeTheirs: { content: string; token: string | null }; + takeMine: { content: string; etag: string | null }; + takeTheirs: { content: string; etag: string | null }; }>(); + let changedLines: number = 0; let diffViewer: WorkspaceDiffViewer | undefined; let editedByText: string = ''; let whenText: string = ''; let isLoading: boolean = true; let loadError: boolean = false; let theirsContent: string | null = null; - let theirsToken: string | null = null; + let theirsEtag: string | null = null; let variant: WorkspaceSaveConflictReason = reason === 'deleted' ? 'deleted' : 'conflict'; $: editedByText = lastEditedBy ?? 'another user'; $: whenText = lastEditedAt ? getTimeAgo(new Date(lastEditedAt)) : ''; + $: changedLinesText = changedLines > 0 ? ` · ${changedLines} changed ${changedLines === 1 ? 'line' : 'lines'}` : ''; async function loadTheirs() { // Re-fetch the server's version for the diff. A 404 means it was deleted. Any other @@ -57,7 +58,7 @@ try { const { content, etag } = await WorkspaceApi.getFileContent(workspaceId, path, user); theirsContent = content; - theirsToken = etag; + theirsEtag = etag; variant = 'conflict'; } catch (e) { if (e instanceof WorkspaceRequestError && e.status === 404) { @@ -75,13 +76,30 @@ }); function onTakeMine() { - // Save the "Mine" pane against the version shown (theirsToken). If the file moved again, + // Save the "Mine" pane against the version shown (theirsEtag). If the file moved again, // the save 412s and the diff re-opens rather than overwriting unseen changes. - dispatch('takeMine', { content: diffViewer?.getMergedContent() ?? mineContent, token: theirsToken }); + dispatch('takeMine', { content: diffViewer?.getMergedContent() ?? mineContent, etag: theirsEtag }); } - function onTakeTheirs() { - dispatch('takeTheirs', { content: theirsContent ?? '', token: theirsToken }); + async function onTakeTheirs() { + // Re-fetch so "take theirs" applies the *latest* server version, not the snapshot from when + // the modal opened — someone may have saved again while it was open. A 404 means it was since + // deleted, so switch to the deleted flow instead of loading empty content; any other failure + // is transient, so show the retry prompt and keep the doc dirty. + isLoading = true; + loadError = false; + try { + const { content, etag } = await WorkspaceApi.getFileContent(workspaceId, path, user); + dispatch('takeTheirs', { content, etag }); + } catch (e) { + if (e instanceof WorkspaceRequestError && e.status === 404) { + theirsContent = null; + variant = 'deleted'; + } else { + loadError = true; + } + isLoading = false; + } } function onRecreate() { @@ -93,9 +111,9 @@ } - + - {variant === 'deleted' && !loadError ? 'File deleted or moved' : 'This file changed since you opened it'} + {variant === 'deleted' && !loadError ? 'File deleted or moved' : 'This file was updated by someone else'}
@@ -111,14 +129,13 @@ {:else if allowMerge} - {fileName} was changed by user @{editedByText} - {whenText ? ` ${whenText}` : ''}. Edit your version on the right — use the arrows to pull in theirs — then - Save, or take theirs to discard your changes. + {fileName} · edited by @{editedByText}{whenText ? ` ${whenText}` : ''}{changedLinesText}. Edit your + version on the right — use the arrows to pull in theirs — then Save, or keep theirs to discard your changes. {:else} - {fileName} was changed by user @{editedByText} - {whenText ? ` ${whenText}` : ''}. Review the differences, then choose how to resolve. + + {fileName} · edited by @{editedByText}{whenText ? ` ${whenText}` : ''}{changedLinesText}. Choose which + version to keep. {/if}
@@ -149,27 +166,52 @@ {type} {languageExtension} editable={allowMerge} + on:stats={e => (changedLines = e.detail.changedLines)} /> {/if} - -
- {#if loadError} - - - {:else if variant === 'deleted'} - - - {:else} - - + +
+ {#if loadError} +
+ +
+ {:else if variant === 'deleted'} +
+ + +
+ {:else if !isLoading} +
+ {#if allowMerge} - + {:else} - + {/if} - {/if} +
+ {/if} +
+
- +
diff --git a/src/components/sequencing/SequenceEditor.svelte b/src/components/sequencing/SequenceEditor.svelte index 99a890d7de..f72949e9e8 100644 --- a/src/components/sequencing/SequenceEditor.svelte +++ b/src/components/sequencing/SequenceEditor.svelte @@ -351,6 +351,20 @@ return true; } + /** + * Replace the editor content and DO record it in the undo history — unlike the file-open sync, + * which is excluded from history. Used by "take theirs" so the user can Cmd-Z back to the edits + * they discarded (the full-doc replace and its inverse are exact, so undo/redo stay clean). + */ + export function rebaseContent(content: string): void { + if (editorSequenceView && editorSequenceView.state.doc.toString() !== content) { + editorSequenceView.dispatch({ + changes: { from: 0, insert: content, to: editorSequenceView.state.doc.length }, + userEvent: 'file.rebase', + }); + } + } + // Exported function to allow parent to navigate to a specific line/column export function gotoLine(line: number, column: number = 0): void { if (editorSequenceView) { diff --git a/src/components/ui/TextEditor.svelte b/src/components/ui/TextEditor.svelte index 6b7d4b98ba..cdcc6d7941 100644 --- a/src/components/ui/TextEditor.svelte +++ b/src/components/ui/TextEditor.svelte @@ -166,6 +166,20 @@ dispatch('textContentUpdated', { filePath: textFilePath, input: updatedText }); } + /** + * Replace the editor content and DO record it in the undo history — unlike the file-open sync, + * which is excluded from history. Used by "take theirs" so the user can Cmd-Z back to the edits + * they discarded (the full-doc replace and its inverse are exact, so undo/redo stay clean). + */ + export function rebaseContent(content: string): void { + if (editorView && editorView.state.doc.toString() !== content) { + editorView.dispatch({ + changes: { from: 0, insert: content, to: editorView.state.doc.length }, + userEvent: 'file.rebase', + }); + } + } + function downloadInputFormat(): void { dispatch('download', { filePath: textFilePath }); } diff --git a/src/components/workspace/WorkspaceDiffViewer.svelte b/src/components/workspace/WorkspaceDiffViewer.svelte index c1194ca50a..4b72973d42 100644 --- a/src/components/workspace/WorkspaceDiffViewer.svelte +++ b/src/components/workspace/WorkspaceDiffViewer.svelte @@ -7,7 +7,7 @@ import { EditorState, type Extension } from '@codemirror/state'; import { EditorView, lineNumbers } from '@codemirror/view'; import { basicSetup } from 'codemirror'; - import { onDestroy, onMount } from 'svelte'; + import { createEventDispatcher, onDestroy, onMount } from 'svelte'; import { WorkspaceContentType } from '../../enums/workspace'; import { readOnlyChangeGuard } from '../../utilities/codemirror/readOnly'; @@ -25,6 +25,8 @@ /** File type, used to pick the fallback language extension. */ export let type: WorkspaceContentType | null = null; + const dispatch = createEventDispatcher<{ stats: { changedLines: number } }>(); + let container: HTMLDivElement; let isSinglePane: boolean = true; let mergeView: MergeView | null = null; @@ -75,6 +77,19 @@ singleView = null; } + /** Total lines involved in changes (larger side of each chunk), for the header summary. */ + function countChangedLines(view: MergeView): number { + const aDoc = view.a.state.doc; + const bDoc = view.b.state.doc; + let total = 0; + for (const chunk of view.chunks) { + const aLines = chunk.toA > chunk.fromA ? aDoc.lineAt(chunk.endA).number - aDoc.lineAt(chunk.fromA).number + 1 : 0; + const bLines = chunk.toB > chunk.fromB ? bDoc.lineAt(chunk.endB).number - bDoc.lineAt(chunk.fromB).number + 1 : 0; + total += Math.max(aLines, bLines); + } + return total; + } + function buildViews() { destroyViews(); if (!container) { @@ -103,6 +118,8 @@ parent: container, revertControls: editable ? 'a-to-b' : undefined, }); + + dispatch('stats', { changedLines: countChangedLines(mergeView) }); } onMount(() => { diff --git a/src/routes/workspaces/[workspaceId]/+page.svelte b/src/routes/workspaces/[workspaceId]/+page.svelte index 6144764a59..e0a22e0a55 100644 --- a/src/routes/workspaces/[workspaceId]/+page.svelte +++ b/src/routes/workspaces/[workspaceId]/+page.svelte @@ -190,6 +190,7 @@ let selectedConsoleTab: WorkspaceConsoleTab = 'actions'; let activeEditorView: EditorView | null = null; let sequenceEditorRef: SequenceEditor; + let textEditorRef: TextEditor; let showLoadingSpinner: boolean = false; let librarySequences: LibrarySequenceSignature[] = []; let loadingSpinnerTimeout: ReturnType | null = null; @@ -665,7 +666,7 @@ return; } - // open() does the stale check and stores the ETag as baseToken for the next save. + // open() does the stale check and stores the ETag as baseEtag for the next save. activeDocument.open(filePath, content, etag); // Fetch fresh metadata so readOnly/user fields are current when the user opens the file @@ -803,7 +804,7 @@ // Save the file before the operation const path = $activeDocumentPath!; const content = $activeDocument.currentContent; - const ifMatch = $activeDocument.baseToken ?? '*'; + const ifMatch = $activeDocument.baseEtag ?? '*'; try { const result = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); if (!result) { @@ -1029,8 +1030,8 @@ async function saveCurrentFile(content: string) { if ($activeDocumentPath) { const path = $activeDocumentPath; - // baseToken runs the concurrency check; '*' forces (when no token was captured). - const ifMatch = $activeDocument.baseToken ?? '*'; + // baseEtag runs the concurrency check; '*' forces (when no etag was captured). + const ifMatch = $activeDocument.baseEtag ?? '*'; try { const result = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); if (result) { @@ -1081,14 +1082,19 @@ } if (value.action === 'take-theirs') { - activeDocument.replaceWithServer(path, value.content, value.token); + // Push the rebase INTO the editor's undo history (before replaceWithServer updates + // originalContent, so the non-undoable prop-sync then no-ops) so the user can Cmd-Z back to + // the edits they discarded. + const editorRef = activeFileIsSequence ? sequenceEditorRef : textEditorRef; + editorRef?.rebaseContent(value.content); + activeDocument.replaceWithServer(path, value.content, value.etag); showSuccessToast('Loaded the latest version of the file'); return true; } if (value.action === 'take-mine') { // Save against the version shown; '*' forces if no token. A re-conflict reopens the diff. - return persistMine(path, value.content, value.token ?? '*'); + return persistMine(path, value.content, value.etag ?? '*'); } if (value.action === 'recreate') { @@ -1711,6 +1717,7 @@ {:else if isTextOrEmpty}
{ +describe('activeDocument store — baseEtag (simultaneous-edit protection)', () => { afterEach(() => { activeDocument.reset(); }); - test('open stores the server etag as baseToken when the path matches loadingPath', () => { + test('open stores the server etag as baseEtag when the path matches loadingPath', () => { activeDocument.startLoad('foo/bar.seq', 'bar.seq', WorkspaceContentType.Sequence); - expect(get(activeDocument).baseToken).toBeNull(); + expect(get(activeDocument).baseEtag).toBeNull(); const opened = activeDocument.open('foo/bar.seq', 'hello', '"tok-1"'); expect(opened).toBe(true); const state = get(activeDocument); - expect(state.baseToken).toBe('"tok-1"'); + expect(state.baseEtag).toBe('"tok-1"'); expect(state.currentContent).toBe('hello'); expect(state.originalContent).toBe('hello'); expect(get(activeDocumentIsDirty)).toBe(false); }); - test('open ignores a stale path and leaves baseToken untouched', () => { + test('open ignores a stale path and leaves baseEtag untouched', () => { activeDocument.startLoad('foo/bar.seq', 'bar.seq', WorkspaceContentType.Sequence); const opened = activeDocument.open('other/file.seq', 'nope', '"tok-x"'); expect(opened).toBe(false); - expect(get(activeDocument).baseToken).toBeNull(); + expect(get(activeDocument).baseEtag).toBeNull(); }); - test('startLoad clears a previously stored baseToken', () => { + test('startLoad clears a previously stored baseEtag', () => { activeDocument.startLoad('a.seq', 'a.seq', WorkspaceContentType.Sequence); activeDocument.open('a.seq', 'x', '"tok-a"'); - expect(get(activeDocument).baseToken).toBe('"tok-a"'); + expect(get(activeDocument).baseEtag).toBe('"tok-a"'); activeDocument.startLoad('b.seq', 'b.seq', WorkspaceContentType.Sequence); - expect(get(activeDocument).baseToken).toBeNull(); + expect(get(activeDocument).baseEtag).toBeNull(); }); - test('markClean advances baseToken when a new token is given, and preserves it otherwise', () => { + test('markClean advances baseEtag when a new etag is given, and preserves it otherwise', () => { activeDocument.startLoad('a.seq', 'a.seq', WorkspaceContentType.Sequence); activeDocument.open('a.seq', 'x', '"tok-a"'); activeDocument.updateContent('x edited'); @@ -50,18 +50,18 @@ describe('activeDocument store — baseToken (simultaneous-edit protection)', () activeDocument.markClean('x edited', '"tok-b"'); let state = get(activeDocument); - expect(state.baseToken).toBe('"tok-b"'); + expect(state.baseEtag).toBe('"tok-b"'); expect(state.originalContent).toBe('x edited'); expect(get(activeDocumentIsDirty)).toBe(false); - // No token argument -> baseToken is left unchanged. + // No etag argument -> baseEtag is left unchanged. activeDocument.updateContent('x edited 2'); activeDocument.markClean('x edited 2'); state = get(activeDocument); - expect(state.baseToken).toBe('"tok-b"'); + expect(state.baseEtag).toBe('"tok-b"'); }); - test('replaceWithServer rebases content + token when the path still matches', () => { + test('replaceWithServer rebases content + etag when the path still matches', () => { activeDocument.startLoad('a.seq', 'a.seq', WorkspaceContentType.Sequence); activeDocument.open('a.seq', 'mine', '"tok-a"'); activeDocument.updateContent('mine edited'); @@ -72,7 +72,7 @@ describe('activeDocument store — baseToken (simultaneous-edit protection)', () const state = get(activeDocument); expect(state.currentContent).toBe('theirs'); expect(state.originalContent).toBe('theirs'); - expect(state.baseToken).toBe('"tok-server"'); + expect(state.baseEtag).toBe('"tok-server"'); expect(get(activeDocumentIsDirty)).toBe(false); }); @@ -85,6 +85,6 @@ describe('activeDocument store — baseToken (simultaneous-edit protection)', () expect(applied).toBe(false); const state = get(activeDocument); expect(state.currentContent).toBe('mine'); - expect(state.baseToken).toBe('"tok-a"'); + expect(state.baseEtag).toBe('"tok-a"'); }); }); diff --git a/src/stores/activeDocument.ts b/src/stores/activeDocument.ts index f79970fb7c..767083b9a1 100644 --- a/src/stores/activeDocument.ts +++ b/src/stores/activeDocument.ts @@ -8,7 +8,7 @@ export interface ActiveDocumentState { * The server's ETag for the version the editor is based on. Sent back on save so the * server can reject it if the file changed underneath. Opaque — only stored and echoed. */ - baseToken: string | null; + baseEtag: string | null; currentContent: string; fileName: string | null; isLoading: boolean; @@ -26,22 +26,22 @@ export interface ActiveDocumentStore { close: () => void; /** - * Mark the document clean after save (syncs originalContent). Pass `newToken` to also - * advance `baseToken`. + * Mark the document clean after save (syncs originalContent). Pass `newEtag` to also + * advance `baseEtag`. */ - markClean: (savedContent?: string, newToken?: string | null) => void; + markClean: (savedContent?: string, newEtag?: string | null) => void; /** - * Open a loaded document and store its `etag` as `baseToken`. Skips stale loads (only + * Open a loaded document and store its `etag` as `baseEtag`. Skips stale loads (only * applies when `path` matches `loadingPath`). Returns false if stale. */ open: (path: string, content: string, etag: string | null) => boolean; /** - * Rebase the document onto `content` + `token` and mark it clean (e.g. "take theirs", or + * Rebase the document onto `content` + `etag` and mark it clean (e.g. "take theirs", or * after a merged save). Only applies if `path` is still active. Returns false otherwise. */ - replaceWithServer: (path: string, content: string, token: string | null) => boolean; + replaceWithServer: (path: string, content: string, etag: string | null) => boolean; /** Reset the store to initial state. */ reset: () => void; @@ -65,7 +65,7 @@ export interface ActiveDocumentStore { /* Constants */ const initialState: ActiveDocumentState = { - baseToken: null, + baseEtag: null, currentContent: '', fileName: null, isLoading: false, @@ -97,10 +97,10 @@ function createActiveDocumentStore(): ActiveDocumentStore { set(initialState); }, - markClean(savedContent?: string, newToken?: string | null): void { + markClean(savedContent?: string, newEtag?: string | null): void { update(state => ({ ...state, - baseToken: newToken !== undefined ? newToken : state.baseToken, + baseEtag: newEtag !== undefined ? newEtag : state.baseEtag, originalContent: savedContent ?? state.currentContent, })); }, @@ -115,7 +115,7 @@ function createActiveDocumentStore(): ActiveDocumentStore { update(state => ({ ...state, - baseToken: etag, + baseEtag: etag, currentContent: content, isLoading: false, loadingPath: null, @@ -125,7 +125,7 @@ function createActiveDocumentStore(): ActiveDocumentStore { return true; }, - replaceWithServer(path: string, content: string, token: string | null): boolean { + replaceWithServer(path: string, content: string, etag: string | null): boolean { const currentState = get(internalStore); // Only replace if this is still the active document (no in-flight load to guard on). @@ -135,7 +135,7 @@ function createActiveDocumentStore(): ActiveDocumentStore { update(state => ({ ...state, - baseToken: token, + baseEtag: etag, currentContent: content, isLoading: false, loadingPath: null, @@ -151,7 +151,7 @@ function createActiveDocumentStore(): ActiveDocumentStore { startLoad(path: string, fileName: string | null, type: WorkspaceContentType | null): void { set({ - baseToken: null, + baseEtag: null, currentContent: '', fileName, isLoading: true, diff --git a/src/utilities/effects.ts b/src/utilities/effects.ts index 63b0be2d82..078a4e2d11 100644 --- a/src/utilities/effects.ts +++ b/src/utilities/effects.ts @@ -7197,8 +7197,8 @@ const effects = { }, /** - * Saves a workspace file. Pass `ifMatch` (the `baseToken`) to run the concurrency check, - * or `'*'` to force. Returns `{ etag }` (the new token) on success. Re-throws a + * Saves a workspace file. Pass `ifMatch` (the `baseEtag`) to run the concurrency check, + * or `'*'` to force. Returns `{ etag }` (the new etag) on success. Re-throws a * {@link WorkspaceSaveConflictError} on `412` so the caller can show the conflict modal; * other errors become a failure toast and return `null`. */ diff --git a/src/utilities/modal.ts b/src/utilities/modal.ts index 4920af87f5..093bfc63b7 100644 --- a/src/utilities/modal.ts +++ b/src/utilities/modal.ts @@ -1720,8 +1720,8 @@ export async function showExpansionPanelModal(user: User | null): Promise finish()); - conflictModal.$on('takeMine', (e: CustomEvent<{ content: string; token: string | null }>) => - finish({ action: 'take-mine', content: e.detail.content, token: e.detail.token }), + conflictModal.$on('takeMine', (e: CustomEvent<{ content: string; etag: string | null }>) => + finish({ action: 'take-mine', content: e.detail.content, etag: e.detail.etag }), ); - conflictModal.$on('takeTheirs', (e: CustomEvent<{ content: string; token: string | null }>) => - finish({ action: 'take-theirs', content: e.detail.content, token: e.detail.token }), + conflictModal.$on('takeTheirs', (e: CustomEvent<{ content: string; etag: string | null }>) => + finish({ action: 'take-theirs', content: e.detail.content, etag: e.detail.etag }), ); conflictModal.$on('recreate', () => finish({ action: 'recreate' })); conflictModal.$on('discard', () => finish({ action: 'discard' })); diff --git a/src/utilities/requests.test.ts b/src/utilities/requests.test.ts index 208c17e717..8bb935914e 100644 --- a/src/utilities/requests.test.ts +++ b/src/utilities/requests.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test, vi } from 'vitest'; -import { reqWorkspaceWithMeta, WorkspaceRequestError, WorkspaceSaveConflictError } from './requests'; +import { reqWorkspaceWithEtag, WorkspaceRequestError, WorkspaceSaveConflictError } from './requests'; vi.mock('$env/dynamic/public', () => ({ env: { PUBLIC_WORKSPACE_CLIENT_URL: 'http://workspace' }, @@ -22,7 +22,7 @@ function mockResponse(opts: { } as unknown as Response; } -describe('reqWorkspaceWithMeta', () => { +describe('reqWorkspaceWithEtag', () => { afterEach(() => { vi.restoreAllMocks(); }); @@ -33,7 +33,7 @@ describe('reqWorkspaceWithMeta', () => { vi.fn().mockResolvedValue(mockResponse({ headers: { ETag: '"tok-1"' }, status: 200, textBody: 'file contents' })), ); - const result = await reqWorkspaceWithMeta('1/foo.seq', 'GET', null, null); + const result = await reqWorkspaceWithEtag('1/foo.seq', 'GET', null, null); expect(result).toEqual({ data: 'file contents', etag: '"tok-1"', status: 200 }); }); @@ -41,7 +41,7 @@ describe('reqWorkspaceWithMeta', () => { test('returns a null etag when the ETag header is absent (CORS not exposed)', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(mockResponse({ status: 200, textBody: 'x' }))); - const result = await reqWorkspaceWithMeta('1/foo.seq', 'GET', null, null); + const result = await reqWorkspaceWithEtag('1/foo.seq', 'GET', null, null); expect(result.etag).toBeNull(); }); @@ -66,7 +66,7 @@ describe('reqWorkspaceWithMeta', () => { ), ); - await expect(reqWorkspaceWithMeta('1/foo.seq', 'PUT', null, null)).rejects.toMatchObject({ + await expect(reqWorkspaceWithEtag('1/foo.seq', 'PUT', null, null)).rejects.toMatchObject({ currentETag: '"server-tok"', lastEditedAt: '2026-06-11T12:00:00Z', lastEditedBy: 'alice@example.com', @@ -85,7 +85,7 @@ describe('reqWorkspaceWithMeta', () => { let error: unknown; try { - await reqWorkspaceWithMeta('1/foo.seq', 'PUT', null, null); + await reqWorkspaceWithEtag('1/foo.seq', 'PUT', null, null); } catch (e) { error = e; } @@ -111,7 +111,7 @@ describe('reqWorkspaceWithMeta', () => { let error: unknown; try { - await reqWorkspaceWithMeta('1/foo.seq', 'PUT', null, null); + await reqWorkspaceWithEtag('1/foo.seq', 'PUT', null, null); } catch (e) { error = e; } @@ -127,7 +127,7 @@ describe('reqWorkspaceWithMeta', () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(mockResponse({ status: 404 }))); let notFound: unknown; try { - await reqWorkspaceWithMeta('1/foo.seq', 'GET', null, null); + await reqWorkspaceWithEtag('1/foo.seq', 'GET', null, null); } catch (e) { notFound = e; } @@ -138,7 +138,7 @@ describe('reqWorkspaceWithMeta', () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(mockResponse({ status: 500 }))); let serverError: unknown; try { - await reqWorkspaceWithMeta('1/foo.seq', 'GET', null, null); + await reqWorkspaceWithEtag('1/foo.seq', 'GET', null, null); } catch (e) { serverError = e; } diff --git a/src/utilities/requests.ts b/src/utilities/requests.ts index b4ec8a6379..4e3087432b 100644 --- a/src/utilities/requests.ts +++ b/src/utilities/requests.ts @@ -358,8 +358,8 @@ export class WorkspaceRequestError extends Error { } } -/** A workspace response plus its `ETag` token and HTTP status. */ -export interface WorkspaceResponseWithMeta { +/** A workspace response plus its `ETag` and HTTP status. */ +export interface WorkspaceResponseWithEtag { data: T; etag: string | null; status: number; @@ -368,9 +368,9 @@ export interface WorkspaceResponseWithMeta { /** * Like {@link reqWorkspace}, but returns the response `ETag` + status and, on `412`, * throws a typed {@link WorkspaceSaveConflictError}. Used for the editor's file GET/save - * so the save token can be threaded through; `reqWorkspace` is left unchanged. + * so the save etag can be threaded through; `reqWorkspace` is left unchanged. */ -export async function reqWorkspaceWithMeta( +export async function reqWorkspaceWithEtag( url: string, method: string, body: any | null, @@ -378,7 +378,7 @@ export async function reqWorkspaceWithMeta( signal?: AbortSignal, asJson: boolean = false, headerOverrides: HeadersInit = {}, -): Promise> { +): Promise> { const WORKSPACE_URL = env.PUBLIC_WORKSPACE_CLIENT_URL; const headers: HeadersInit = { diff --git a/src/utilities/workspaces.test.ts b/src/utilities/workspaces.test.ts index fe9109efa5..cc4e2e1fc4 100644 --- a/src/utilities/workspaces.test.ts +++ b/src/utilities/workspaces.test.ts @@ -38,8 +38,8 @@ const mockNavigator = { const reqWorkspaceMock = vi.spyOn(requests, 'reqWorkspace').mockResolvedValue({}); const reqWorkspaceMetadataMock = vi.spyOn(requests, 'reqWorkspaceMetadata').mockResolvedValue({}); -const reqWorkspaceWithMetaMock = vi - .spyOn(requests, 'reqWorkspaceWithMeta') +const reqWorkspaceWithEtagMock = vi + .spyOn(requests, 'reqWorkspaceWithEtag') .mockResolvedValue({ data: '', etag: null, status: 200 }); vi.stubGlobal('navigator', mockNavigator); vi.mock('$env/dynamic/public', () => { @@ -270,7 +270,7 @@ describe('Workspace utility function tests', () => { test('getFileContent', async () => { await WorkspaceApi.getFileContent(1, 'foo/bar/bazz.seq', null); - expect(reqWorkspaceWithMetaMock).toHaveBeenLastCalledWith( + expect(reqWorkspaceWithEtagMock).toHaveBeenLastCalledWith( '1/foo/bar/bazz.seq', 'GET', null, @@ -587,7 +587,7 @@ describe('Workspace utility function tests', () => { body.append('file', file, file.name); await WorkspaceApi.saveFile(1, 'foo/bar/bazz.seq', 'sequence contents', true, null); - expect(reqWorkspaceWithMetaMock).toHaveBeenLastCalledWith( + expect(reqWorkspaceWithEtagMock).toHaveBeenLastCalledWith( '1/foo/bar/bazz.seq?type=file&overwrite=true', 'PUT', body, diff --git a/src/utilities/workspaces.ts b/src/utilities/workspaces.ts index c13d67e85b..dd58083f10 100644 --- a/src/utilities/workspaces.ts +++ b/src/utilities/workspaces.ts @@ -14,7 +14,7 @@ import type { } from '../types/workspace-tree-view'; import { filterEmpty } from './generic'; import { pathMatchesExtensionPattern } from './parameters'; -import { reqWorkspace, reqWorkspaceMetadata, reqWorkspaceWithMeta } from './requests'; +import { reqWorkspace, reqWorkspaceMetadata, reqWorkspaceWithEtag } from './requests'; import { pluralize } from './text'; export function mapWorkspaceTreePaths(nodes: WorkspaceTreeNode[], currentPath: string[] = []): WorkspaceTreeMap { @@ -494,7 +494,7 @@ export const WorkspaceApi = { user: User | null, ): Promise<{ content: string; etag: string | null }> { // Throws a WorkspaceRequestError (with status) on any non-2xx — content is never null. - const { data, etag } = await reqWorkspaceWithMeta( + const { data, etag } = await reqWorkspaceWithEtag( joinPath([workspaceId, filePath]), 'GET', null, @@ -631,7 +631,7 @@ export const WorkspaceApi = { ifMatch?: string | '*', ): Promise { const body = createFormDataWithFile(filePath, fileContent); - const { etag } = await reqWorkspaceWithMeta( + const { etag } = await reqWorkspaceWithEtag( `${workspaceId}/${filePath}?type=file${shouldOverwrite ? '&overwrite=true' : ''}`, 'PUT', body, From 3e9deeea19b4e1d3973ec805f8f9b588256f087e Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Wed, 8 Jul 2026 10:28:14 -0700 Subject: [PATCH 4/8] Refactor --- .../workspaces/[workspaceId]/+page.svelte | 30 +++++++------------ src/utilities/effects.ts | 20 ++++++------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/src/routes/workspaces/[workspaceId]/+page.svelte b/src/routes/workspaces/[workspaceId]/+page.svelte index e0a22e0a55..2b15f3b7d0 100644 --- a/src/routes/workspaces/[workspaceId]/+page.svelte +++ b/src/routes/workspaces/[workspaceId]/+page.svelte @@ -806,18 +806,15 @@ const content = $activeDocument.currentContent; const ifMatch = $activeDocument.baseEtag ?? '*'; try { - const result = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); - if (!result) { - // Save failed (a failure toast was already shown) — don't proceed. - return false; - } - activeDocument.markClean(content, result.etag); + const { etag } = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); + activeDocument.markClean(content, etag); return true; } catch (e) { if (e instanceof WorkspaceSaveConflictError) { // Proceed with the operation only if the conflict resolution actually saved. return await resolveSaveConflict(e, path, content); } + // Any other failure was already toasted by the effect — just don't proceed. return false; } } @@ -1033,11 +1030,9 @@ // baseEtag runs the concurrency check; '*' forces (when no etag was captured). const ifMatch = $activeDocument.baseEtag ?? '*'; try { - const result = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); - if (result) { - activeDocument.markClean(content, result.etag); - refreshWorkspaceContents(); - } + const { etag } = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); + activeDocument.markClean(content, etag); + refreshWorkspaceContents(); } catch (e) { if (e instanceof WorkspaceSaveConflictError) { await resolveSaveConflict(e, path, content); @@ -1118,14 +1113,11 @@ */ async function persistMine(path: string, content: string, ifMatch: string): Promise { try { - const result = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); - if (result) { - // Rebase the editor on the saved content (no-ops if the user navigated away). - activeDocument.replaceWithServer(path, content, result.etag); - refreshWorkspaceContents(); - return true; - } - return false; + const { etag } = await effects.saveWorkspaceFile($workspaceId, path, content, $user, ifMatch); + // Rebase the editor on the saved content (no-ops if the user navigated away). + activeDocument.replaceWithServer(path, content, etag); + refreshWorkspaceContents(); + return true; } catch (e) { if (e instanceof WorkspaceSaveConflictError) { return resolveSaveConflict(e, path, content); diff --git a/src/utilities/effects.ts b/src/utilities/effects.ts index 078a4e2d11..f23867664e 100644 --- a/src/utilities/effects.ts +++ b/src/utilities/effects.ts @@ -7198,9 +7198,9 @@ const effects = { /** * Saves a workspace file. Pass `ifMatch` (the `baseEtag`) to run the concurrency check, - * or `'*'` to force. Returns `{ etag }` (the new etag) on success. Re-throws a - * {@link WorkspaceSaveConflictError} on `412` so the caller can show the conflict modal; - * other errors become a failure toast and return `null`. + * or `'*'` to force. Returns `{ etag }` (the new etag) on success. Always throws on failure so + * callers have a single failure path: a {@link WorkspaceSaveConflictError} on `412` (the caller + * shows the conflict modal), or any other error (already surfaced as a failure toast here). */ async saveWorkspaceFile( workspaceId: number, @@ -7208,7 +7208,7 @@ const effects = { fileContent: string, user: User | null = null, ifMatch?: string | '*', - ): Promise<{ etag: string | null } | null> { + ): Promise<{ etag: string | null }> { try { const etag = await WorkspaceApi.saveFile(workspaceId, filePath, fileContent, true, user, ifMatch); @@ -7216,13 +7216,13 @@ const effects = { logMessage(`Saved workspace file "${filePath}".`); return { etag }; } catch (e) { - if (e instanceof WorkspaceSaveConflictError) { - // Let the caller resolve the conflict (modal); don't swallow it in a toast. - throw e; + // Conflict is the caller's to resolve (modal), so don't toast it. Everything else is a real + // failure we surface here before rethrowing, so callers just bail in their catch. + if (!(e instanceof WorkspaceSaveConflictError)) { + catchError('Workspace file was unable to be saved', e as Error); + showFailureToast('Workspace File Save Failed'); } - catchError('Workspace file was unable to be saved', e as Error); - showFailureToast('Workspace File Save Failed'); - return null; + throw e; } }, From 065124219c66668011652c63f9c11d0cff487d29 Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Wed, 8 Jul 2026 10:35:53 -0700 Subject: [PATCH 5/8] Language: Updated -> Changed in WorkspaceSaveConflictModal --- e2e-tests/fixtures/WorkspaceSaveConflict.ts | 2 +- src/components/modals/WorkspaceSaveConflictModal.svelte | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e-tests/fixtures/WorkspaceSaveConflict.ts b/e2e-tests/fixtures/WorkspaceSaveConflict.ts index b2bb493e85..87aa56959f 100644 --- a/e2e-tests/fixtures/WorkspaceSaveConflict.ts +++ b/e2e-tests/fixtures/WorkspaceSaveConflict.ts @@ -51,7 +51,7 @@ export class WorkspaceSaveConflict { updatePage(page: Page): void { this.page = page; this.modal = page.locator('#modal-container'); - this.conflictTitle = this.modal.getByText('This file was updated by someone else'); + this.conflictTitle = this.modal.getByText('This file was changed by someone else'); this.deletedTitle = this.modal.getByText('File deleted or moved'); this.theirsHeader = this.modal.getByText('Theirs (server)'); this.mineHeader = this.modal.getByText('Mine (your edits)'); diff --git a/src/components/modals/WorkspaceSaveConflictModal.svelte b/src/components/modals/WorkspaceSaveConflictModal.svelte index c9c7bd7ac5..c16e749453 100644 --- a/src/components/modals/WorkspaceSaveConflictModal.svelte +++ b/src/components/modals/WorkspaceSaveConflictModal.svelte @@ -113,7 +113,7 @@ - {variant === 'deleted' && !loadError ? 'File deleted or moved' : 'This file was updated by someone else'} + {variant === 'deleted' && !loadError ? 'File deleted or moved' : 'This file was changed by someone else'}
From 4f926a33d50a7a990ff872f39adc74e57f6b537f Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Wed, 8 Jul 2026 10:40:59 -0700 Subject: [PATCH 6/8] Language fix --- src/components/modals/WorkspaceSaveConflictModal.svelte | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/modals/WorkspaceSaveConflictModal.svelte b/src/components/modals/WorkspaceSaveConflictModal.svelte index c16e749453..9a4209af48 100644 --- a/src/components/modals/WorkspaceSaveConflictModal.svelte +++ b/src/components/modals/WorkspaceSaveConflictModal.svelte @@ -124,8 +124,7 @@ {:else if variant === 'deleted'} - {fileName} was deleted or moved by user @{editedByText} - {whenText ? ` ${whenText}` : ''}. Your unsaved changes are shown below. + {fileName} was deleted or moved{whenText ? ` ${whenText}` : ''}. Your unsaved changes are shown below. {:else if allowMerge} From a093d3f0fc3e719eda73ee93469d9bc6fc1602ce Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Wed, 8 Jul 2026 10:54:03 -0700 Subject: [PATCH 7/8] Clear url + selected file path + refresh workspace upon discarding file on conflict --- src/routes/workspaces/[workspaceId]/+page.svelte | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/routes/workspaces/[workspaceId]/+page.svelte b/src/routes/workspaces/[workspaceId]/+page.svelte index 2b15f3b7d0..091ba00f45 100644 --- a/src/routes/workspaces/[workspaceId]/+page.svelte +++ b/src/routes/workspaces/[workspaceId]/+page.svelte @@ -1100,6 +1100,9 @@ if (value.action === 'discard') { if ($activeDocumentPath === path) { activeDocument.close(); + selectedFilePath = null; + confirmAndNavigate(null); + refreshWorkspaceContents(); } return false; } From c85e1cd7ca981273c365b1860304f932170d1010 Mon Sep 17 00:00:00 2001 From: AaronPlave Date: Thu, 9 Jul 2026 09:55:37 -0700 Subject: [PATCH 8/8] test: fix serial-suite state leak in workspace edit-protection e2e --- e2e-tests/tests/workspace-edit-protection.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/e2e-tests/tests/workspace-edit-protection.test.ts b/e2e-tests/tests/workspace-edit-protection.test.ts index 50b76cd2e1..5b896d5455 100644 --- a/e2e-tests/tests/workspace-edit-protection.test.ts +++ b/e2e-tests/tests/workspace-edit-protection.test.ts @@ -155,6 +155,13 @@ test.describe.serial('Workspace simultaneous-edit protection', () => { await setup.page.keyboard.press('ControlOrMeta+z'); await expect(editor).toContainText('// my change'); await expect(workspace.saveSequenceButton).toBeEnabled(); + + // Leave a clean editor for the next serial test. "Keep theirs" already advanced the stored + // token to the server version, so this save succeeds without re-conflicting. Without it the + // dirty doc leaks: the next test's createAndOpenSequence auto-selects its new file, which pops + // a "Navigate Away" modal whose backdrop blocks the file-row click. + await workspace.saveSequence(); + await expect(workspace.saveSequenceButton).toBeDisabled(); }); test('"Keep mine" saves the local version and the next save succeeds', async () => {