From a09f6a87c3a7829b83e604c55be7210fa88bd0c7 Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:57:44 +1000 Subject: [PATCH] refactor(app): drop redundant server keying from session route boundary --- packages/app/src/pages/session.tsx | 7 ++-- .../app/src/pages/session/route-boundary.ts | 42 +++++++------------ .../session-route-boundary.test.ts | 23 ++++------ 3 files changed, 27 insertions(+), 45 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index d6ef37c5fa..899490b8fa 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -89,7 +89,7 @@ import { formatServerError, isSessionNotFoundError } from "@/utils/server-errors import { legacySessionHref, requireServerKey, sessionHref } from "@/utils/session-route" import { useUsageExceededDialogs } from "./session/usage-exceeded-dialogs" import { createSessionOwnership } from "./session/session-ownership" -import { SessionRouteBoundary } from "./session/route-boundary" +import { SessionRouteErrorBoundary } from "./session/route-boundary" type FollowupItem = FollowupDraft & { id: string } type FollowupEdit = Pick @@ -147,8 +147,7 @@ export function SessionPage() { export function TargetSessionRoute() { const params = useParams<{ serverKey: string; id: string }>() return ( - ( - + ) } diff --git a/packages/app/src/pages/session/route-boundary.ts b/packages/app/src/pages/session/route-boundary.ts index 9df8d8e8c2..926a17099e 100644 --- a/packages/app/src/pages/session/route-boundary.ts +++ b/packages/app/src/pages/session/route-boundary.ts @@ -1,37 +1,27 @@ -import { ErrorBoundary, Show, createComponent, createEffect, on } from "solid-js" +import { ErrorBoundary, createComponent, createEffect, on } from "solid-js" import type { JSX } from "solid-js" -// Structural boundary for the target session route: decides when the route -// subtree remounts and how route-level errors are scoped. Kept free of app -// contexts (and JSX) so the remount semantics can be tested directly. -// -// Keyed by server only. Workspace-scoped state (notably TerminalProvider and -// its PTY WebSockets) lives inside the route subtree, so switching session -// tabs within the same workspace must not remount it; session changes are -// handled reactively below (TargetSessionPage re-keys per workspace). -export function SessionRouteBoundary(props: { - serverKey: string | undefined +// Error scope for the target session route. All session tabs on a server share +// one route instance, so this must NOT key or remount per session: the subtree +// holds workspace-scoped state (notably TerminalProvider and its PTY +// WebSockets) that has to survive switching tabs within the same workspace. +// Remount boundaries live elsewhere: app.tsx keys the route per server around +// the server-scoped providers, and TargetSessionPage re-keys per workspace. +// Kept free of app contexts (and JSX) so these semantics are directly testable. +export function SessionRouteErrorBoundary(props: { sessionID: string | undefined fallback: (error: unknown) => JSX.Element children: JSX.Element }) { - return createComponent(Show, { - get when() { - return props.serverKey + return createComponent(ErrorBoundary, { + fallback: (error: unknown, reset: () => void) => { + // A stale error (e.g. session not found) must clear when navigating to a + // different session tab; mirrors the panel boundary reset inside Page. + createEffect(on(() => props.sessionID, reset, { defer: true })) + return props.fallback(error) }, - keyed: true, get children() { - return createComponent(ErrorBoundary, { - fallback: (error: unknown, reset: () => void) => { - // Without a per-session remount, a stale error (e.g. session not - // found) must clear when navigating to a different session. - createEffect(on(() => props.sessionID, reset, { defer: true })) - return props.fallback(error) - }, - get children() { - return props.children - }, - }) + return props.children }, }) } diff --git a/packages/app/test-browser/session-route-boundary.test.ts b/packages/app/test-browser/session-route-boundary.test.ts index 13f35d43dc..bd7aaa0d38 100644 --- a/packages/app/test-browser/session-route-boundary.test.ts +++ b/packages/app/test-browser/session-route-boundary.test.ts @@ -1,12 +1,13 @@ import { expect, test } from "bun:test" import { createComponent, createSignal, onCleanup } from "solid-js" import { render } from "solid-js/web" -import { SessionRouteBoundary } from "@/pages/session/route-boundary" +import { SessionRouteErrorBoundary } from "@/pages/session/route-boundary" -// Terminals (and other workspace-scoped state) live inside the route subtree, -// so switching session tabs within the same server must not remount it. -test("switching sessions on the same server does not remount the route subtree", () => { - const [server, setServer] = createSignal("srv_1") +// All session tabs on a server share one route instance, and the subtree holds +// workspace-scoped state (notably the terminal and its PTY WebSockets), so +// switching session tabs must not remount it. Remounting is owned elsewhere: +// per server in app.tsx and per workspace in TargetSessionPage. +test("switching sessions does not remount the route subtree", () => { const [session, setSession] = createSignal("ses_a") let mounts = 0 let disposals = 0 @@ -20,10 +21,7 @@ test("switching sessions on the same server does not remount the route subtree", const dispose = render( () => - createComponent(SessionRouteBoundary, { - get serverKey() { - return server() - }, + createComponent(SessionRouteErrorBoundary, { get sessionID() { return session() }, @@ -42,10 +40,6 @@ test("switching sessions on the same server does not remount the route subtree", expect(mounts).toBe(initialMounts) expect(disposals).toBe(0) - setServer("srv_2") - expect(mounts).toBeGreaterThan(initialMounts) - expect(disposals).toBeGreaterThan(0) - dispose() }) @@ -62,8 +56,7 @@ test("route error clears when navigating to a different session", () => { const container = document.createElement("div") const dispose = render( () => - createComponent(SessionRouteBoundary, { - serverKey: "srv_1", + createComponent(SessionRouteErrorBoundary, { get sessionID() { return session() },