diff --git a/src/workspaces/workspaces/src/react/WorkspaceView.tsx b/src/workspaces/workspaces/src/react/WorkspaceView.tsx index 7b80fbf7..8af2a847 100644 --- a/src/workspaces/workspaces/src/react/WorkspaceView.tsx +++ b/src/workspaces/workspaces/src/react/WorkspaceView.tsx @@ -1,4 +1,4 @@ -import { useState, useCallback, useEffect, useRef, useMemo, type ComponentRef } from 'react'; +import { useState, useCallback, useEffect, useRef, useMemo } from 'react'; import { flushSync } from 'react-dom'; import { ResizablePanel, ResizablePanelGroup, ResizableHandle } from './internal/resizable'; import { useIsMobile } from './internal/useIsMobile'; @@ -296,28 +296,29 @@ export const WorkspaceView = ({ return () => window.removeEventListener('keydown', onKeyDown); }, [swapSourceId, dragSourceId]); - const ephemeralPanelRef = useRef>(null); + // ── The ephemeral pane is open when it EXISTS, and that is its only representation ── + // + // It used to be a permanent panel starting at `defaultSize={0}`, opened by an effect calling + // `panel.resize()`. That put the state in two places — the prop said "closed", the effect was supposed + // to correct it — and a correction step is something that can be missed. It was: + // + // Landing on `/files/...?view=x.m4a` (a deep link, or just a refresh) made `isEphemeralOpen` true on + // the FIRST render. But `workspace.isLoaded` is false until `/dashboards` answers, and the early return + // below means nothing is rendered yet — so the effect ran with a null ref and returned. Its deps were + // `[isEphemeralOpen, defaultBaseSize]`, and by the time the group did render neither had changed: one + // was already true, the other is a constant. So it never ran again. The URL said the viewer was open, + // the pane sat at zero width, and double-clicking the file did nothing at all — `?view=` already held + // that name, so there was no state change to react to. Reported as "refresh loses the panel and then + // the file won't open". + // + // Rendering the panel conditionally cannot have that failure: there is no moment where the markup and + // the intent disagree, and a deep link is correct on first paint rather than corrected afterwards. + // `id` + `order` are what make a conditional panel safe in react-resizable-panels 2.x — the same pair + // `WorkspaceRenderer` already uses for the panel tree. const isEphemeralOpen = !!ephemeral; - useEffect(() => { - const panel = ephemeralPanelRef.current; - if (!panel) return; - - // `react-resizable-panels` asserts rather than no-ops when it is asked to resize a group it has not - // laid out yet — and an assert thrown from an effect aborts the commit, which here means the whole - // dashboard rather than the pane that could not be sized. The pane is a file preview; the panels - // behind it are a night's work. Only the ephemeral pane's own geometry is at stake, and the next - // render tries again. - try { - if (isEphemeralOpen) { - panel.resize(100 - (ephemeral.defaultBaseSize ?? 40)); - } else { - panel.collapse(); - } - } catch (err) { - console.warn('[workspace] could not size the ephemeral pane yet', err); - } - }, [isEphemeralOpen, ephemeral?.defaultBaseSize]); + // The workspace's share of the group when the ephemeral pane is open; the pane takes the remainder. + const baseSize = ephemeral?.defaultBaseSize ?? 40; const onMobileBack = useMemo(() => { if (!isMobile) return null; @@ -390,7 +391,7 @@ export const WorkspaceView = ({ /> ) : ( - + - - - {isEphemeralOpen ? ( - - ) : null} - + {isEphemeralOpen && ephemeral ? ( + <> + + + + + + ) : null} )} diff --git a/src/workspaces/workspaces/src/react/ephemeral-pane.test.tsx b/src/workspaces/workspaces/src/react/ephemeral-pane.test.tsx new file mode 100644 index 00000000..a40fdbc4 --- /dev/null +++ b/src/workspaces/workspaces/src/react/ephemeral-pane.test.tsx @@ -0,0 +1,111 @@ +import { describe, expect, test } from 'bun:test'; +import { render } from '@testing-library/react'; +import type { LayoutNode } from '../core/types'; +import type { AppRegistryMap, DashboardState, EphemeralPanels } from './types'; +import { WorkspaceView } from './WorkspaceView'; + +/** + * The ephemeral pane — the file viewer that opens beside a workspace — and whether it is OPEN. + * + * Every bug this has had looked identical from outside: the URL says a file is open, the pane is not + * there, and clicking the file again does nothing, because the URL already says what the click would say. + * The cause was the same shape each time — open/closed expressed twice, once in the markup and once in an + * effect meant to correct it, with the correction able to run at a moment where it could not take effect. + * + * So these assert the SIZE the group gives the pane, never merely that the element exists. A pane + * rendered at zero width is precisely the failure being guarded against, and "is it in the document" + * reports that as a pass. `data-panel-size` is react-resizable-panels' own computed answer. + * + * Each test renders its own tree: `@testing-library/react` unmounts between tests, so a render shared by + * a describe block is detached by the time the second test reads it. + */ + +const BASE_PANEL: LayoutNode = { type: 'panel', id: 'p1', appType: 'test/app' }; + +const dashboard = (isLoaded: boolean): DashboardState => ({ + key: 'screens/test', + value: BASE_PANEL, + setValue: () => {}, + isLoaded, +}); + +const registry = { + 'test/app': { name: 'T', icon: () => null, component: () =>
BASE
}, +} as unknown as AppRegistryMap; + +const viewer: EphemeralPanels = { + layout: { type: 'panel', id: 'files-viewer', appType: null }, + components: { 'files-viewer': { component: () =>
VIEWER
} }, + defaultBaseSize: 40, +}; + +/** The group's own computed share for one pane, or null when the pane is not in the tree. */ +const paneSize = (container: HTMLElement, id: string): number | null => { + const el = container.querySelector(`[data-panel-id="${id}"]`); + const size = el?.getAttribute('data-panel-size'); + return size == null ? null : Number(size); +}; + +describe('nothing open', () => { + test('the pane is not in the tree at all', () => { + const { container } = render(); + expect(paneSize(container, 'workspace-ephemeral')).toBeNull(); + }); + + test('the workspace has the whole group', () => { + const { container } = render(); + expect(paneSize(container, 'workspace-base')).toBe(100); + }); +}); + +describe('a deep link — open on the very first render', () => { + test('the pane is open on first paint rather than corrected afterwards', () => { + const { container } = render(); + expect(paneSize(container, 'workspace-ephemeral')).toBe(60); + expect(paneSize(container, 'workspace-base')).toBe(40); + }); +}); + +// THE REGRESSION, and the reason this file exists. `workspace.isLoaded` is false until /dashboards +// answers, and WorkspaceView renders nothing at all until then — so a `?view=` present at load time was +// already "open" before there was any panel group to size. The old imperative effect ran once against a +// null ref and returned; its dependencies (already-true `isEphemeralOpen`, a constant size) could not +// change afterwards, so it never ran again. URL open, pane at zero width, and clicking the file inert +// because the URL needed no change. +describe('open before the dashboard has finished loading', () => { + test('the pane opens when the group appears, with no state change to trigger it', () => { + const { container, rerender } = render( + , + ); + expect(paneSize(container, 'workspace-base')).toBeNull(); // nothing renders while loading + + rerender(); + + expect(paneSize(container, 'workspace-ephemeral')).toBe(60); + expect(paneSize(container, 'workspace-base')).toBe(40); + }); +}); + +describe('opening and closing while the page is live', () => { + test('opening gives the pane its share and closing gives it back', () => { + const { container, rerender } = render(); + expect(paneSize(container, 'workspace-base')).toBe(100); + + rerender(); + expect(paneSize(container, 'workspace-ephemeral')).toBe(60); + + rerender(); + expect(paneSize(container, 'workspace-ephemeral')).toBeNull(); + expect(paneSize(container, 'workspace-base')).toBe(100); + }); +}); + +describe('a pane that asks for a different split', () => { + test('defaultBaseSize is the WORKSPACE share and the pane takes the remainder', () => { + const { container } = render( + , + ); + expect(paneSize(container, 'workspace-base')).toBe(25); + expect(paneSize(container, 'workspace-ephemeral')).toBe(75); + }); +}); diff --git a/todo/url-state/_index.md b/todo/url-state/_index.md index ba47c787..1308ecd1 100644 --- a/todo/url-state/_index.md +++ b/todo/url-state/_index.md @@ -22,3 +22,17 @@ Four bugs came out of that work and are fixed: closing a pane sent you home; `?v every page load by a mount effect that had made deep links impossible since 2026-02-23; navigating kept a pane belonging to the folder you left; and the breadcrumb opted out of that last rule because it is a `` and never called the function enforcing it. + +**A fifth, fixed 2026-08-21.** The URL survived a refresh and the pane still did not open — reported as +"refresh loses the panel, and then the file won't open". Nothing to do with the query string this time: +`WorkspaceView` gave the ephemeral pane `defaultSize={0}` and opened it from an effect calling +`panel.resize()`, so open/closed lived in two places. On a deep link the effect ran before +`workspace.isLoaded` was true — when the early return means nothing is rendered and the ref is null — and +its deps could not change afterwards, so it never ran again. The second half is what made it look like a +click bug: with `?view=` already naming the file, double-clicking it changed no state, so nothing re-ran. +The pane is now rendered conditionally with `id` + `order` and sized declaratively; `ephemeral-pane.test.tsx` +asserts the SIZE rather than the element's presence, because a pane at zero width passes any +"is it in the document" check. Five of its six tests fail against the old implementation. + +The shape is worth keeping in mind for the rest of this theme: **state that the URL owns, mirrored into an +imperative correction, will eventually be read at a moment the correction cannot run.**