From bd9d26199f43759495508e7bbcdc3a34ccca9fa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 21 Aug 2026 21:10:20 +0000 Subject: [PATCH] the ephemeral pane is open when it exists, not when an effect says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh on /files/...?view=x.m4a came back with the query string intact and no viewer, and then double-clicking the file did nothing at all. Two symptoms, one cause, and the second is what made it read as a click bug. WorkspaceView gave the pane defaultSize={0} and opened it from an effect calling panel.resize(), so open/closed was expressed twice — the markup said closed and the effect was meant to correct it. A correction step can be missed, and this one was: on a deep link isEphemeralOpen is true on the first render, but workspace.isLoaded is false until /dashboards answers and the early return means nothing is rendered, so the effect ran against a null ref and returned. Its deps were [isEphemeralOpen, defaultBaseSize] — one already true, the other a constant — so it never ran again. URL open, pane at zero width. Then the inert double-click follows for free: ?view= already held that name, so setSearchParams changed nothing, so there was no transition for the effect to react to. The URL being already correct is exactly why the click had nothing to say. Now the pane is rendered conditionally with id + order — the same pair WorkspaceRenderer already uses — and both panels take declarative sizes. There is no moment where markup and intent disagree, and a deep link is right on first paint rather than corrected after it. The try/catch that guarded resizing an unlaid-out group goes with it; nothing calls resize now. ephemeral-pane.test.tsx asserts the SIZE the group computes, not that the element exists — a pane at zero width satisfies any is-it-in-the-document check, which is the failure being guarded against. Five of its six tests fail against the old implementation, including the load-order one. Full suite: 1316 pass. Co-Authored-By: Claude Fable 5 --- .../workspaces/src/react/WorkspaceView.tsx | 75 ++++++------ .../src/react/ephemeral-pane.test.tsx | 111 ++++++++++++++++++ todo/url-state/_index.md | 14 +++ 3 files changed, 164 insertions(+), 36 deletions(-) create mode 100644 src/workspaces/workspaces/src/react/ephemeral-pane.test.tsx 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.**