diff --git a/src/app/document/io/create.ts b/src/app/document/io/create.ts index d445382ba..a1e863d45 100644 --- a/src/app/document/io/create.ts +++ b/src/app/document/io/create.ts @@ -93,6 +93,7 @@ export function createDocumentIOActions( openDOMFile, importDOMText, hasUnsavedChanges: sourceActions.hasUnsavedChanges, + markDocumentSaved: () => sourceActions.markDocumentSaved(), saveFigFile: sourceActions.saveFigFile, saveFigFileAs: sourceActions.saveFigFileAs } diff --git a/src/app/editor/active-store/index.ts b/src/app/editor/active-store/index.ts index 366248276..03e2fd9a6 100644 --- a/src/app/editor/active-store/index.ts +++ b/src/app/editor/active-store/index.ts @@ -1,4 +1,7 @@ -import { shallowRef, triggerRef, watch } from 'vue' +import { hasInjectionContext, inject, provide, shallowRef, triggerRef, watch } from 'vue' +import type { InjectionKey } from 'vue' + +import { provideEditor } from '@open-pencil/vue' import type { EditorStore } from '@/app/editor/session' @@ -30,8 +33,24 @@ const storeProxy = new Proxy({} as EditorStore, { } }) +const TAB_STORE_KEY: InjectionKey = Symbol('open-pencil-tab-store') + +/** + * Binds a tab's components to that tab's own store, for `useEditorStore` and the SDK's + * `useEditor`. Without it they would follow the active tab, so a closing tab's canvas would + * hand its renderers to the next document and never take them back. + */ +export function provideTabEditorStore(store: EditorStore) { + provide(TAB_STORE_KEY, store) + provideEditor(store) +} + +/** + * The store of the tab this component belongs to, or the active tab's store outside any tab + * and outside component setup. + */ export function useEditorStore(): EditorStore { - return storeProxy + return hasInjectionContext() ? inject(TAB_STORE_KEY, storeProxy) : storeProxy } /** diff --git a/src/app/editor/session/modules.ts b/src/app/editor/session/modules.ts index 12ba76935..3c318ed1c 100644 --- a/src/app/editor/session/modules.ts +++ b/src/app/editor/session/modules.ts @@ -78,6 +78,8 @@ export function createEditorStoreModules( setViewportSize: documentIO.setViewportSize, fitCurrentPageToViewport: documentIO.fitCurrentPageToViewport, hasUnsavedChanges: documentIO.hasUnsavedChanges, + /** Takes the document as it is now as its saved state, so closing it asks nothing. */ + markDocumentSaved: documentIO.markDocumentSaved, saveFigFile: documentIO.saveFigFile, saveFigFileAs: documentIO.saveFigFileAs, getDocumentFilePath: documentIO.getDocumentFilePath, diff --git a/src/components/editor/EditorWorkspace.vue b/src/components/editor/EditorWorkspace.vue index 899134f7a..96da8741f 100644 --- a/src/components/editor/EditorWorkspace.vue +++ b/src/components/editor/EditorWorkspace.vue @@ -3,13 +3,13 @@ import { SplitterGroup, SplitterPanel, SplitterResizeHandle } from 'reka-ui' import { tv } from 'tailwind-variants' import { computed } from 'vue' -import { formatShortcut, provideEditor, useI18n, useViewportKind } from '@open-pencil/vue' +import { formatShortcut, useI18n, useViewportKind } from '@open-pencil/vue' -import { useEditorStore } from '@/app/editor/active-store' +import { provideTabEditorStore } from '@/app/editor/active-store' import { appRuntimeConfig } from '@/app/runtime/config' import { loadEditorLayout, saveEditorLayout } from '@/app/shell/layout-storage' import { appMenuShortcut } from '@/app/shell/menu/shortcut' -import { activeTab } from '@/app/tabs' +import type { Tab } from '@/app/tabs' import CanvasSplitRoot from '@/components/canvas/CanvasSplitRoot.vue' import CollabPanel from '@/components/collab-panel/CollabPanel.vue' import ActiveRoomOverlay from '@/components/collab-room/ActiveRoomOverlay.vue' @@ -27,12 +27,14 @@ import splitterTheme from '@/theme/splitter' import WorkspacePill from './WorkspacePill.vue' const showChrome = appRuntimeConfig.showChrome -const store = useEditorStore() -// WorkspaceView keys this view by tab, so the tab's own store is fixed for its lifetime. Its -// editor UI stays bound to that document rather than the app-level editor, which follows the -// active tab and would move these subscriptions to the next document when this tab closes. -const tab = activeTab.value -if (tab) provideEditor(tab.store) +/** + * The document tab this workspace edits. WorkspaceView keys the workspace by tab, so the tab and + * its store are fixed for its lifetime, and its editor UI stays bound to that document rather + * than following the active tab to the next document when this one closes. + */ +const { tab } = defineProps<{ tab: Tab }>() +const store = tab.store +provideTabEditorStore(store) const { isMobile } = useViewportKind() const initialEditorLayout = loadEditorLayout() const horizontalSplitterStyles = tv(splitterTheme)({ direction: 'horizontal' }) @@ -44,17 +46,13 @@ const { pending: roomPending } = useRoomActions() diff --git a/tests/e2e/app/closed-documents.spec.ts b/tests/e2e/app/closed-documents.spec.ts index d0b2fb291..727366fef 100644 --- a/tests/e2e/app/closed-documents.spec.ts +++ b/tests/e2e/app/closed-documents.spec.ts @@ -1,4 +1,4 @@ -import { expect, test } from '@playwright/test' +import { expect, test, type Page } from '@playwright/test' import type * as AppTabs from '@/app/tabs' @@ -7,9 +7,36 @@ import { testPath } from '#tests/helpers/paths' const FIXTURE = 'gold-preview.fig' +async function openFixtureInNewTab(page: Page) { + await page.evaluate(async (fixture) => { + const tabsURL = '/src/app/tabs/index.ts' + const tabs: typeof AppTabs = await import(tabsURL) + const response = await fetch(`/__fixtures/${fixture}`) + await tabs.openFileInNewTab(new File([await response.arrayBuffer()], fixture)) + }, FIXTURE) + await expect + .poll(() => + page.evaluate(() => { + const store = window.openPencil?.getStore?.() + return Boolean(store && store.graph.nodes.size > 100 && !store.state.preparation) + }) + ) + .toBe(true) +} + +function activeTabRenderers(page: Page) { + return page.evaluate(async () => { + const tabsURL = '/src/app/tabs/index.ts' + const tabs: typeof AppTabs = await import(tabsURL) + return tabs.getActiveStore().canvasRenderers.length + }) +} + // A closed tab must release its document. CanvasKit's WebGL context table, an editor's global // text measurer, store effects created during a component's setup, and app-level event -// subscriptions each kept every closed document alive: store, graph, canvas, and UI tree. +// subscriptions each kept every closed document alive: store, graph, canvas, and UI tree. A +// closing tab's canvas also registered its renderers with the tab that became active, which +// kept every closed canvas and its whole UI tree alive in the remaining document. test('documents closed in tabs are released', async ({ page }) => { test.setTimeout(90_000) await page.route(`**/__fixtures/${FIXTURE}`, (route) => @@ -20,21 +47,13 @@ test('documents closed in tabs are released', async ({ page }) => { const cdp = await page.context().newCDPSession(page) await page.evaluate(() => Reflect.set(window, '__closedGraphs', [])) + // The first document replaces the empty startup tab and stays open while others close. + await openFixtureInNewTab(page) + await expect.poll(() => activeTabRenderers(page)).toBeGreaterThan(0) + const remainingRenderers = await activeTabRenderers(page) + for (let cycle = 0; cycle < 3; cycle++) { - await page.evaluate(async (fixture) => { - const tabsURL = '/src/app/tabs/index.ts' - const tabs: typeof AppTabs = await import(tabsURL) - const response = await fetch(`/__fixtures/${fixture}`) - await tabs.openFileInNewTab(new File([await response.arrayBuffer()], fixture)) - }, FIXTURE) - await expect - .poll(() => - page.evaluate(() => { - const store = window.openPencil?.getStore?.() - return Boolean(store && store.graph.nodes.size > 100 && !store.state.preparation) - }) - ) - .toBe(true) + await openFixtureInNewTab(page) await page.evaluate(async () => { const tabsURL = '/src/app/tabs/index.ts' const tabs: typeof AppTabs = await import(tabsURL) @@ -59,4 +78,6 @@ test('documents closed in tabs are released', async ({ page }) => { { timeout: 15_000 } ) .toBe(0) + // The remaining tab's canvas remounts when it becomes active again. + await expect.poll(() => activeTabRenderers(page)).toBe(remainingRenderers) }) diff --git a/tests/e2e/app/menu.spec.ts b/tests/e2e/app/menu.spec.ts index 470c92115..1d9e475be 100644 --- a/tests/e2e/app/menu.spec.ts +++ b/tests/e2e/app/menu.spec.ts @@ -2,6 +2,18 @@ import { expect, test, useEditorSetup } from '#tests/e2e/fixtures' const editor = useEditorSetup() +/** + * Opens a top-level menu. A menu that just closed stays in the page while it animates out, and a + * click on a trigger then is lost, so this waits for it to go before clicking. + */ +async function openMenu(name: string) { + await expect(editor.page.getByRole('menu')).toHaveCount(0) + await editor.page.getByRole('menubar').getByRole('menuitem', { name, exact: true }).click() + const menu = editor.page.getByRole('menu', { name }) + await expect(menu).toBeVisible() + return menu +} + test('menu bar is visible in browser mode', async () => { const menubar = editor.page.locator('[role="menubar"]') await expect(menubar).toBeVisible() @@ -14,9 +26,7 @@ test('menu bar has all top-level menus', async () => { }) test('File menu opens and shows items', async () => { - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'File' }).click() - const menu = editor.page.locator('[role="menu"]') - await expect(menu).toBeVisible() + const menu = await openMenu('File') const items = await menu.locator('[role="menuitem"]').allTextContents() expect(items.some((t) => t.includes('Open'))).toBe(true) @@ -28,9 +38,7 @@ test('File menu opens and shows items', async () => { }) test('Edit menu shows Undo/Redo/Delete', async () => { - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Edit' }).click() - const menu = editor.page.locator('[role="menu"]') - await expect(menu).toBeVisible() + const menu = await openMenu('Edit') const items = await menu.locator('[role="menuitem"]').allTextContents() expect(items.some((t) => t.includes('Undo'))).toBe(true) @@ -42,9 +50,7 @@ test('Edit menu shows Undo/Redo/Delete', async () => { }) test('View menu shows zoom options', async () => { - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'View' }).click() - const menu = editor.page.locator('[role="menu"]') - await expect(menu).toBeVisible() + const menu = await openMenu('View') const items = await menu.locator('[role="menuitem"]').allTextContents() expect(items.some((t) => t.includes('Zoom to fit'))).toBe(true) @@ -55,9 +61,7 @@ test('View menu shows zoom options', async () => { }) test('Object menu shows Group/Ungroup/Component', async () => { - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Object' }).click() - const menu = editor.page.locator('[role="menu"]') - await expect(menu).toBeVisible() + const menu = await openMenu('Object') const items = await menu.locator('[role="menuitem"]').allTextContents() expect(items.some((t) => t.includes('Group'))).toBe(true) @@ -86,13 +90,13 @@ test('Move to page is disabled without a selection', async () => { store.switchPage(store.graph.getPages()[0].id) }) - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Object' }).click() + await openMenu('Object') const moveToPage = editor.page.getByRole('menuitem', { name: 'Move to page' }) await expect(moveToPage).toHaveAttribute('data-disabled') await editor.page.keyboard.press('Escape') await editor.canvas.drawRect(200, 200, 100, 100) - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Object' }).click() + await openMenu('Object') await expect(editor.page.getByRole('menuitem', { name: 'Move to page' })).not.toHaveAttribute( 'data-disabled' ) @@ -111,7 +115,7 @@ test('Undo via Edit menu works', async () => { const beforeUndo = await getStoreStateNumber('selectedIds') expect(beforeUndo).toBe(1) - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Edit' }).click() + await openMenu('Edit') const undoItem = editor.page.getByRole('menuitem', { name: /^Undo\b/ }) await expect(undoItem).toBeEnabled() await undoItem.click() @@ -120,7 +124,7 @@ test('Undo via Edit menu works', async () => { const afterUndo = await getStoreStateNumber('selectedIds') expect(afterUndo).toBe(0) - await editor.page.getByRole('menuitem', { name: 'Edit', exact: true }).click() + await openMenu('Edit') await expect(editor.page.getByRole('menuitem', { name: /^Redo\b/ })).toBeEnabled() await editor.page.keyboard.press('Escape') }) @@ -134,8 +138,8 @@ test('Duplicate via Edit menu works', async () => { return store.graph.getChildren(store.state.currentPageId).length }) - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'Edit' }).click() - await editor.page.locator('[role="menu"] [role="menuitem"]', { hasText: 'Duplicate' }).click() + const editMenu = await openMenu('Edit') + await editMenu.locator('[role="menuitem"]', { hasText: 'Duplicate' }).click() await editor.canvas.waitForRender() const countAfter = await editor.page.evaluate(() => { @@ -148,15 +152,15 @@ test('Duplicate via Edit menu works', async () => { }) test('Zoom to fit via View menu works', async () => { - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'View' }).click() - await editor.page.locator('[role="menu"] [role="menuitem"]', { hasText: 'Zoom in' }).click() + const zoomInMenu = await openMenu('View') + await zoomInMenu.locator('[role="menuitem"]', { hasText: 'Zoom in' }).click() await editor.canvas.waitForRender() const zoomBefore = await getStoreStateNumber('zoom') expect(zoomBefore).toBeGreaterThan(1) - await editor.page.locator('[role="menubar"] [role="menuitem"]', { hasText: 'View' }).click() - await editor.page.locator('[role="menu"] [role="menuitem"]', { hasText: 'Zoom to fit' }).click() + const zoomToFitMenu = await openMenu('View') + await zoomToFitMenu.locator('[role="menuitem"]', { hasText: 'Zoom to fit' }).click() await editor.canvas.waitForRender() const zoomAfter = await getStoreStateNumber('zoom') diff --git a/tests/e2e/storage/workspace.spec.ts b/tests/e2e/storage/workspace.spec.ts index fe49e7a82..4428fd3a5 100644 --- a/tests/e2e/storage/workspace.spec.ts +++ b/tests/e2e/storage/workspace.spec.ts @@ -86,17 +86,13 @@ test('configured storage lists previews through ranges before opening the docume await page.keyboard.press('ControlOrMeta+KeyN') await page.getByRole('button', { name: 'Settings' }).last().click() await page.getByTestId('settings-section-storage').click() - await page.getByLabel('Endpoint').fill('https://s3.example.com') - await page.getByLabel('Bucket').fill('designs') - - for (const [field, value] of [ - ['access-key-id', 'access-key'], - ['secret-access-key', 'secret-key'] - ] as const) { - const container = page.locator(`[data-credential="${field}"]`) - await container.locator('input').fill(value) - await container.getByRole('button', { name: 'Save' }).click() - } + const panel = page.getByTestId('settings-storage-panel') + await panel.getByRole('button', { name: /S3 storage/ }).click() + await panel.getByLabel('Endpoint').fill('https://s3.example.com') + await panel.getByLabel('Bucket').fill('designs') + await panel.getByLabel('Access key ID').fill('access-key') + await panel.getByLabel('Secret access key').fill('secret-key') + await page.getByRole('button', { name: 'Save', exact: true }).click() await page.getByTestId('settings-storage-open-workspace').click() await expect(page.getByTestId('recent-files-home')).toBeVisible() diff --git a/tests/helpers/canvas.ts b/tests/helpers/canvas.ts index 9ab587225..cf08a00c6 100644 --- a/tests/helpers/canvas.ts +++ b/tests/helpers/canvas.ts @@ -39,7 +39,8 @@ export class CanvasHelper { /** * Canvas specs draw on Figma's light page. Playwright runs the app in its dark theme, where a new * document starts on Figma's dark page, so a fresh document (one empty page on that exact - * background) is switched back; any other document keeps its own. + * background) is switched back; any other document keeps its own. The switch is setup, not an + * edit, so the document stays saved and closing it asks nothing. * `tests/e2e/pages/theme-background.spec.ts` covers the theme. */ async useLightPage() { @@ -54,7 +55,9 @@ export class CanvasHelper { pages.length === 1 && first.childIds.length === 0 && [r, g, b].every((c) => c === darkDefault) - if (fresh) store.setPageColor({ r: 0.96, g: 0.96, b: 0.96, a: 1 }) + if (!fresh) return + store.setPageColor({ r: 0.96, g: 0.96, b: 0.96, a: 1 }) + store.markDocumentSaved() }) }