From 9aeb0464ce4c63543b66aa492c0a6f17aac39871 Mon Sep 17 00:00:00 2001 From: Menghuan1918 Date: Fri, 14 Aug 2026 18:29:31 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20sidebar=20crash=20#31=20=E2=80=94=20layo?= =?UTF-8?q?ut-push=20CSS-variable=20leak=20+=20per-tab=20error=20containme?= =?UTF-8?q?nt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failure modes of a sidebar render crash, fixed together: 1. Layout-push leak (root cause): the layout-push effect wrote --dsh-sidebar-width/--dsh-sidebar-height on document.documentElement without a cleanup, so ANY unmount of the Sidebar (error-boundary swap, plugin disable, HMR) left the variables behind and layout.css kept squeezing #root with a stale margin — "the sidebar cannot be hidden" until a full reload. The effect now removes both properties on cleanup (the CSS fallback 0px restores the layout). 2. Crash containment (UX): the root error boundary previously replaced the WHOLE sidebar — toggle cluster included — with an error strip on any tab crash. A new shared RenderBoundary (src/client/RenderBoundary.tsx) now scopes containment per tab: a crashing viewer/editor shows a dismissible strip inside its own pane while the toggle cluster, the other tabs, and the panel stay alive. The descriptor is rendered as a real element (createElement) so even top-level component throws land in the boundary, not in TabContent's own render. The root boundary remains as the last resort for Workbench/shell errors. Tests (tests/sidebar-crash.spec.tsx, jsdom + real Sidebar/store harness): - unmount clears both CSS variables (red before the fix: 307px leaked) - a crashing tab is contained: strip in-pane, collapse button alive, layout push still live - the retry button recovers a tab whose crash has since been fixed Full suite: 425 tests pass; tsc --noEmit clean. --- src/client/RenderBoundary.tsx | 49 +++++++++++ src/client/Sidebar.tsx | 34 ++++++-- src/client/index.tsx | 43 ++-------- src/client/sidebar.module.css | 16 ++++ tests/sidebar-crash.spec.tsx | 150 ++++++++++++++++++++++++++++++++++ 5 files changed, 252 insertions(+), 40 deletions(-) create mode 100644 src/client/RenderBoundary.tsx create mode 100644 tests/sidebar-crash.spec.tsx diff --git a/src/client/RenderBoundary.tsx b/src/client/RenderBoundary.tsx new file mode 100644 index 0000000..f856adf --- /dev/null +++ b/src/client/RenderBoundary.tsx @@ -0,0 +1,49 @@ +/** + * The generic render error boundary for the sidebar tree: a render error in + * the wrapped subtree shows a dismissible error strip (retry re-renders the + * children) instead of blanking the shell. Used at two scopes: + * + * - ROOT (index.tsx, `css.boundaryError`): last-resort containment for + * errors in the sidebar shell itself (Workbench, drag layout, …) — a full + * swap keeps the page alive. + * - PER-TAB (Sidebar.tsx TabContent, `css.tabBoundaryError`): a crashing + * viewer/editor shows a strip inside ITS OWN pane; the toggle cluster, the + * other tabs, and the panel itself stay alive (issue #31 — a tab crash + * must never take down the whole sidebar). + * + * The className prop selects the strip's geometry: the root's full-height + * fixed rail vs. the tab's pane-filling block. + */ +import { Component, type ErrorInfo, type ReactNode } from 'react' +import { t } from './locales.ts' +import css from './sidebar.module.css' + +export class RenderBoundary extends Component<{ children?: ReactNode; className?: string }, { error: string | null }> { + state = { error: null as string | null } + + static getDerivedStateFromError(error: unknown): { error: string } { + return { error: error instanceof Error ? error.message : String(error) } + } + + componentDidCatch(error: Error, info: ErrorInfo): void { + console.error('[dsh-better-sidebar] render error:', error, info.componentStack) + } + + render(): ReactNode { + if (this.state.error !== null) { + return ( +
+ dsh-better-sidebar: {this.state.error} + +
+ ) + } + return this.props.children + } +} diff --git a/src/client/Sidebar.tsx b/src/client/Sidebar.tsx index 3a6f9e2..50b8c75 100644 --- a/src/client/Sidebar.tsx +++ b/src/client/Sidebar.tsx @@ -25,7 +25,7 @@ * drawer floats). Widening does not migrate back: the tabs keep living in * the right tree. */ -import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react' +import { createElement, useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react' import { useSyncExternalStore } from 'react' import clsx from 'clsx' import { IconCloseFill14, Tooltip } from '@deepseek-ai/dsh-client-ui-primitives' @@ -44,6 +44,7 @@ import type { NewTabOption } from './TabBar.tsx' import type { TabDragPayload } from './TabBar.tsx' import { relativeTo } from './paths.ts' import { OrphanedTab } from './OrphanedTab.tsx' +import { RenderBoundary } from './RenderBoundary.tsx' import { detectNewDirectSubagent } from './subagent-detect.ts' import { detectNewJob } from './subagent-jobs.ts' import { t } from './locales.ts' @@ -77,10 +78,22 @@ function TabContent(props: { if (descriptor === undefined) { return } - return descriptor.component({ - ctx, store, scope, tab, visible, expanded, - onToggleDir, onReferenceFile, onOpenDiff, onSubagentJump, - }) + // One boundary per tab: a render crash in a viewer/editor shows a strip in + // THIS tab's pane only — the toggle cluster, the other tabs, and the panel + // stay alive (issue #31). The tab strip (close button) lives outside, so a + // crashing tab stays closable; the root boundary in index.tsx remains the + // last resort for errors in the sidebar shell itself. The descriptor is + // rendered as a REAL element (not called directly): a direct call would + // throw inside TabContent's own render, which the boundary cannot catch — + // as a child fiber, every render error (top-level or deep) lands in it. + return createElement( + RenderBoundary, + { className: css.tabBoundaryError }, + createElement(descriptor.component, { + ctx, store, scope, tab, visible, expanded, + onToggleDir, onReferenceFile, onOpenDiff, onSubagentJump, + }), + ) } /** The + menu options for the current state, driven by the tab registry. @@ -512,6 +525,17 @@ export function Sidebar(props: { ctx: Context; store: SidebarStore }) { : 0 document.documentElement.style.setProperty('--dsh-sidebar-width', `${width}px`) document.documentElement.style.setProperty('--dsh-sidebar-height', `${height}px`) + // Unmount must release the push (issue #31): when the boundary swaps the + // whole sidebar after a render crash (or the plugin fiber is disposed / + // HMR), the CSS variables would otherwise stay on and layout.css + // keeps squeezing #root with a stale margin — "the sidebar cannot be + // hidden" until a full reload. removeProperty restores the CSS fallback + // (var(--dsh-sidebar-width, 0px)); React re-runs cleanup+setup in the + // same commit on state changes, so there is no visible flicker. + return () => { + document.documentElement.style.removeProperty('--dsh-sidebar-width') + document.documentElement.style.removeProperty('--dsh-sidebar-height') + } }, [narrow, snapshot.state?.panelOpen, snapshot.state?.width, snapshot.state?.bottomOpen, snapshot.state?.bottomHeight]) useEffect(() => { if (anyDragging) document.body.setAttribute('data-dsh-sidebar-dragging', '') diff --git a/src/client/index.tsx b/src/client/index.tsx index 4784028..b5fbdd3 100644 --- a/src/client/index.tsx +++ b/src/client/index.tsx @@ -8,7 +8,7 @@ * bundle itself is a module-table consumer only (react + ui-primitives + * xterm, all provided or inlined). */ -import { Component, createElement, type ErrorInfo, type ReactNode } from 'react' +import { createElement } from 'react' import { createRoot, type Root } from 'react-dom/client' import type { Context } from '../context-types.ts' import { createSidebarStore } from './state.ts' @@ -16,6 +16,7 @@ import { createBetterSidebarService } from './service.ts' import { resetChunks } from './chunk-loader.ts' import { registerBuiltins } from './builtins/index.ts' import { Sidebar } from './Sidebar.tsx' +import { RenderBoundary } from './RenderBoundary.tsx' import { registerOpenPathInterception, registerTurnTailInterception } from './intercept.tsx' import { registerLinkInterception } from './link-intercept.ts' import { registerImeGuard } from './ime-guard.ts' @@ -31,40 +32,12 @@ import './layout.css' export const inject = ['slots', 'sessions', 'connection', 'workspaces', 'locale'] /** - * Error boundary over the sidebar tree: a render error must never blank the - * whole panel silently — it shows a dismissible error strip and logs the - * stack for diagnosis. + * Error boundary over the sidebar tree (root scope): a render error in the + * sidebar SHELL itself must never blank the page silently — the shared + * RenderBoundary shows a dismissible error strip and logs the stack. The + * per-tab scope (Sidebar.tsx) catches viewer/editor crashes first; this root + * boundary stays as the last resort for Workbench/shell errors. */ -class SidebarBoundary extends Component<{ children: ReactNode }, { error: string | null }> { - state = { error: null as string | null } - - static getDerivedStateFromError(error: unknown): { error: string } { - return { error: error instanceof Error ? error.message : String(error) } - } - - componentDidCatch(error: Error, info: ErrorInfo): void { - console.error('[dsh-better-sidebar] render error:', error, info.componentStack) - } - - render(): ReactNode { - if (this.state.error !== null) { - return ( -
- dsh-better-sidebar: {this.state.error} - -
- ) - } - return this.props.children - } -} - /** * Client plugin body. * @param ctx - the client cordis context (slots, sessions). @@ -141,7 +114,7 @@ export function apply(ctx: Context): void { host.setAttribute('data-dsh-better-sidebar', '') document.body.appendChild(host) root = createRoot(host) - root.render(createElement(SidebarBoundary, null, createElement(Sidebar, { ctx, store: sidebarStore }))) + root.render(createElement(RenderBoundary, { className: css.boundaryError }, createElement(Sidebar, { ctx, store: sidebarStore }))) } catch (error) { fail('mount', error) } diff --git a/src/client/sidebar.module.css b/src/client/sidebar.module.css index c94436c..3414840 100644 --- a/src/client/sidebar.module.css +++ b/src/client/sidebar.module.css @@ -1370,6 +1370,22 @@ color: var(--dsw-alias-label-primary); } +/* The PER-TAB error strip: fills one tab's content area (unlike + .boundaryError, the full-height fixed rail for shell-level crashes), so a + crashing viewer/editor never hides the toggle cluster or the other tabs. */ +.tabBoundaryError { + flex: 1; + min-height: 0; + display: flex; + flex-direction: column; + gap: 8px; + align-items: flex-start; + padding: 12px 16px; + overflow: auto; + font: var(--dsw-font-xxs-12); + color: var(--dsw-alias-state-error-primary); +} + /* ── Git ───────────────────────────────────────────────────────────────── */ .git { diff --git a/tests/sidebar-crash.spec.tsx b/tests/sidebar-crash.spec.tsx new file mode 100644 index 0000000..a6c93f4 --- /dev/null +++ b/tests/sidebar-crash.spec.tsx @@ -0,0 +1,150 @@ +/** + * Sidebar crash tests — the two failure modes behind issue #31. + * + * 1. Layout-push leak: the layout-push effect writes + * `--dsh-sidebar-width/--dsh-sidebar-height` on document.documentElement. + * Unmounting the Sidebar for ANY reason (error-boundary swap, plugin + * disable, HMR) must clear them — otherwise layout.css keeps squeezing + * `#root` with a stale margin and "the sidebar cannot be hidden" until a + * full page reload. + * + * 2. Tab crash containment: a render error inside ONE tab's content must not + * take down the whole sidebar. The per-tab boundary shows a strip inside + * that tab's pane while the toggle cluster, the other tabs, and the panel + * itself stay alive; the retry button recovers a transient crash. + * + * Rendered with the REAL Sidebar shell + real store/service against a minimal + * fake context (createRoot + act(), the repo's jsdom pattern). + */ +// @vitest-environment jsdom +import { afterEach, describe, expect, it, vi } from 'vitest' +import { createElement } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { act } from 'react-dom/test-utils' + +// The act() environment flag (React 18.2 reads it before flushing effects). +;(globalThis as Record).IS_REACT_ACT_ENVIRONMENT = true + +import { Sidebar } from '../src/client/Sidebar.tsx' +import { createSidebarStore, type SidebarStore } from '../src/client/state.ts' +import { createBetterSidebarService, type BetterSidebarService } from '../src/client/service.ts' +import { t } from '../src/client/locales.ts' + +/** jsdom has no WebSocket; the agent-terminals push effect constructs one on mount. */ +class FakeWebSocket { + onmessage: ((event: { data: unknown }) => void) | null = null + onclose: (() => void) | null = null + onerror: (() => void) | null = null + close = (): void => {} + constructor(_url: string) {} +} + +interface MountedSidebar { + container: HTMLDivElement + store: SidebarStore + service: BetterSidebarService + unmount: () => void +} + +/** Mount the real Sidebar shell against a minimal context (real store + service). */ +function mountSidebar(): MountedSidebar { + vi.stubGlobal('WebSocket', FakeWebSocket) + const container = document.createElement('div') + document.body.append(container) + const store = createSidebarStore() + const service = createBetterSidebarService(store) + // Fresh-session seed: the panel starts OPEN (openByDefault default true). + store.setSession('s1') + // useSyncExternalStore requires STABLE snapshots across calls (the real DSH + // services return stable objects) — a fresh object per call loops forever. + const localeSnapshot = { active: 'en' } + const sessionsSnapshot = { + current: 's1', + // cwd present → api.sessionCwd is never called in these tests. + byId: { s1: { cwd: '/tmp' } }, + } + const ctx = { + locale: { subscribe: () => () => {}, getSnapshot: () => localeSnapshot }, + sessions: { list: { subscribe: () => () => {}, getSnapshot: () => sessionsSnapshot } }, + betterSidebar: service, + } + const root: Root = createRoot(container) + act(() => { root.render(createElement(Sidebar, { ctx: ctx as never, store })) }) + return { + container, + store, + service, + unmount: () => { + act(() => { root.unmount() }) + container.remove() + }, + } +} + +afterEach(() => { + document.body.innerHTML = '' + vi.unstubAllGlobals() +}) + +describe('layout-push variable cleanup', () => { + it('clears --dsh-sidebar-width/--dsh-sidebar-height when the sidebar unmounts', () => { + const { store, unmount } = mountSidebar() + const htmlStyle = document.documentElement.style + // The seeded session is open: the layout push is applied on mount. + const width = store.getSnapshot().state!.width + expect(htmlStyle.getPropertyValue('--dsh-sidebar-width')).toBe(`${width}px`) + expect(htmlStyle.getPropertyValue('--dsh-sidebar-height')).toBe('0px') + // Any unmount (boundary swap, plugin disable, HMR) must release the push. + unmount() + expect(htmlStyle.getPropertyValue('--dsh-sidebar-width')).toBe('') + expect(htmlStyle.getPropertyValue('--dsh-sidebar-height')).toBe('') + }) +}) + +describe('tab crash containment', () => { + it('a crashing tab shows an in-pane strip while the cluster and panel survive', () => { + const { container, service, store } = mountSidebar() + service.registerTab({ + id: 'crash', + title: 'Crash', + component: () => { throw new Error('boom') }, + }) + act(() => { service.openTab({ type: 'crash', title: 'Crash' }) }) + // The strip lives inside the tab's pane — the crash is contained. + expect(container.textContent).toContain('boom') + expect(container.textContent).toContain(t('terminalRetry')) + // The toggle cluster and the panel itself survived (no full-tree swap): + // the collapse button is still there and the layout push is still live. + expect(container.querySelector(`[aria-label="${t('collapse')}"]`)).not.toBeNull() + expect(document.documentElement.style.getPropertyValue('--dsh-sidebar-width')).toBe( + `${store.getSnapshot().state!.width}px`, + ) + }) + + it('the retry button recovers a tab whose crash has since been fixed', () => { + // A PERSISTENT render error reaches the boundary strip (React 18.2 + // auto-recovers transient throws — one bad render followed by a good + // retry is swallowed as a recoverable error and never shows a strip). + let shouldThrow = true + const { container, service } = mountSidebar() + service.registerTab({ + id: 'crash-until-fixed', + title: 'Crash until fixed', + component: () => { + if (shouldThrow) throw new Error('transient') + return createElement('div', null, 'recovered') + }, + }) + act(() => { service.openTab({ type: 'crash-until-fixed', title: 'Crash until fixed' }) }) + expect(container.textContent).toContain('transient') + const retry = [...container.querySelectorAll('button')] + .find(button => button.textContent === t('terminalRetry')) + expect(retry).toBeDefined() + // The crash condition is gone (e.g. the data arrived): the retry button + // remounts the tab's content and the strip clears. + shouldThrow = false + act(() => { retry!.click() }) + expect(container.textContent).toContain('recovered') + expect(container.textContent).not.toContain('transient') + }) +})