From fccbde195c94c91efa6b8799fa97a6dc8d495f84 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 9 Aug 2026 14:26:29 -0700 Subject: [PATCH] CL-5848: Fix silent provision parse and canvas deep-link first paint Reject malformed JSON and schema-invalid provision bodies with 400 instead of treating them as membership probes. Seed canvas state from the path on first paint, drop the unused close helper, and cover path to canvas wiring in pure and SSR tests. --- apps/web/src/shell/app-shell.tsx | 13 ++-- .../web/src/shell/canvas-column-state.test.ts | 30 ++++++--- apps/web/src/shell/canvas-column-state.ts | 35 +++++----- apps/web/test/routes.test.tsx | 7 ++ packages/onboarding/src/routes.ts | 43 +++++++++--- packages/onboarding/test/routes.test.ts | 66 +++++++++++++++++++ 6 files changed, 158 insertions(+), 36 deletions(-) diff --git a/apps/web/src/shell/app-shell.tsx b/apps/web/src/shell/app-shell.tsx index edcf6c54d..a6b91cf45 100644 --- a/apps/web/src/shell/app-shell.tsx +++ b/apps/web/src/shell/app-shell.tsx @@ -14,6 +14,7 @@ import { canvasColumnAllowed, contextualPanelVisible } from "./breakpoints"; import { useShellFocusRescue } from "./focus-rescue"; import { useScrollReset } from "./use-scroll-reset"; import { + applyChannelPathToCanvas, initialCanvasColumnState, openChannelInCanvas, resolveCanvasVisibility, @@ -38,7 +39,11 @@ export function AppShell({ }) { const navigate = useNavigate(); const layoutMode = useShellLayoutMode(); - const [canvasState, setCanvasState] = useState(initialCanvasColumnState); + // Deep links seed canvas state on first paint (SSR and client) so a `/c/:id` + // URL is not effect-only. Later path changes re-apply through the effect. + const [canvasState, setCanvasState] = useState(() => + applyChannelPathToCanvas(initialCanvasColumnState(), path), + ); const canvasAllowed = canvasColumnAllowed(layoutMode); const canvasOpen = resolveCanvasVisibility(canvasState, canvasAllowed); const frameRef = useRef(null); @@ -52,13 +57,11 @@ export function AppShell({ // here — the toggle only flips open/closed so reopening lands on the // same conversation. useEffect(() => { - const channelId = channelIdFromPath(path); - if (channelId === null) return; - setCanvasState((state) => openChannelInCanvas(state, channelId)); + setCanvasState((state) => applyChannelPathToCanvas(state, path)); }, [path]); const handleChannelChange = (channelId: string) => { - setCanvasState((state) => openChannelInCanvas(state, channelId)); + setCanvasState(openChannelInCanvas(channelId)); if (!isChannelPath(path) || channelIdFromPath(path) !== channelId) { navigate(channelPath(channelId)); } diff --git a/apps/web/src/shell/canvas-column-state.test.ts b/apps/web/src/shell/canvas-column-state.test.ts index 593481376..fb43ec403 100644 --- a/apps/web/src/shell/canvas-column-state.test.ts +++ b/apps/web/src/shell/canvas-column-state.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { - closeCanvasColumn, + applyChannelPathToCanvas, initialCanvasColumnState, openChannelInCanvas, resolveCanvasVisibility, @@ -17,24 +17,38 @@ describe("canvas column state", () => { }); test("opening a channel loads it and opens the canvas", () => { - const next = openChannelInCanvas(initialCanvasColumnState(), "ch_1"); - expect(next).toEqual({ open: true, channelId: "ch_1" }); + expect(openChannelInCanvas("ch_1")).toEqual({ + open: true, + channelId: "ch_1", + }); }); test("toggle preserves the loaded channel", () => { - const open = openChannelInCanvas(initialCanvasColumnState(), "ch_1"); + const open = openChannelInCanvas("ch_1"); const closed = toggleCanvasColumn(open); expect(closed).toEqual({ open: false, channelId: "ch_1" }); expect(toggleCanvasColumn(closed)).toEqual(open); }); - test("close drops the channel", () => { - const open = openChannelInCanvas(initialCanvasColumnState(), "ch_1"); - expect(closeCanvasColumn(open)).toEqual({ open: false, channelId: null }); + test("a /c deep link opens the canvas onto that channel", () => { + expect( + applyChannelPathToCanvas(initialCanvasColumnState(), "/c/ch_deep"), + ).toEqual({ open: true, channelId: "ch_deep" }); + expect( + applyChannelPathToCanvas(initialCanvasColumnState(), "/chat/ch_legacy"), + ).toEqual({ open: true, channelId: "ch_legacy" }); + }); + + test("non-channel paths leave canvas state alone", () => { + const open = openChannelInCanvas("ch_1"); + expect(applyChannelPathToCanvas(open, "/agents")).toEqual(open); + expect(applyChannelPathToCanvas(initialCanvasColumnState(), "/")).toEqual( + initialCanvasColumnState(), + ); }); test("visibility is gated by the viewport allow flag", () => { - const open = openChannelInCanvas(initialCanvasColumnState(), "ch_1"); + const open = openChannelInCanvas("ch_1"); expect(resolveCanvasVisibility(open, true)).toBe(true); expect(resolveCanvasVisibility(open, false)).toBe(false); expect(resolveCanvasVisibility(initialCanvasColumnState(), true)).toBe( diff --git a/apps/web/src/shell/canvas-column-state.ts b/apps/web/src/shell/canvas-column-state.ts index e0162d6bc..5ba35ba66 100644 --- a/apps/web/src/shell/canvas-column-state.ts +++ b/apps/web/src/shell/canvas-column-state.ts @@ -1,13 +1,14 @@ -// The canvas column's state as a pure reducer, separate from `breakpoints.ts`'s -// allow/disallow rule — a user's toggle, a channel the user opened into the -// canvas, and the viewport's veto are three independent inputs, and -// `resolveCanvasVisibility` is the one place they combine. +// The canvas column's state as pure transitions, separate from +// `breakpoints.ts`'s allow/disallow rule — a user's toggle, a channel the +// user opened into the canvas, and the viewport's veto are three independent +// inputs, and `resolveCanvasVisibility` is the one place they combine. // // The canvas hosts the channel chat surface (the retired `/chat` page's // `ChatWorkspace`), so its state carries the active channel alongside // open/closed. A deep link (`/c/:channelId`) feeds the same `channelId` from -// the URL in `app-shell.tsx`; this reducer only owns the toggle-and-channel -// shape, never the URL. +// the URL; path→state lives here so the shell and tests share one contract. + +import { channelIdFromPath } from "../channel-path"; export type CanvasColumnState = { readonly open: boolean; @@ -27,19 +28,23 @@ export function toggleCanvasColumn( return { ...state, open: !state.open }; } -/** Open the canvas onto a specific channel (a channel-row click). */ -export function openChannelInCanvas( - _state: CanvasColumnState, - channelId: string, -): CanvasColumnState { +/** Open the canvas onto a specific channel (channel-row click or deep link). */ +export function openChannelInCanvas(channelId: string): CanvasColumnState { return { open: true, channelId }; } -/** Close the canvas and drop the loaded channel. */ -export function closeCanvasColumn( - _state: CanvasColumnState, +/** + * Apply a route path to canvas state. A `/c/:id` (or legacy `/chat/:id`) opens + * the canvas onto that channel; any other path leaves the previous state alone + * so navigating within the rest of the app does not drop an open conversation. + */ +export function applyChannelPathToCanvas( + state: CanvasColumnState, + path: string, ): CanvasColumnState { - return { open: false, channelId: null }; + const channelId = channelIdFromPath(path); + if (channelId === null) return state; + return openChannelInCanvas(channelId); } /** What actually renders: the user's toggle (or a deep-link channel), gated by diff --git a/apps/web/test/routes.test.tsx b/apps/web/test/routes.test.tsx index d5e0fc0ee..f43eda87e 100644 --- a/apps/web/test/routes.test.tsx +++ b/apps/web/test/routes.test.tsx @@ -124,4 +124,11 @@ describe("routes render", () => { expect(markup).toContain("Page not found"); expect(markup).not.toContain('aria-current="page"'); }); + + test("a /c/:channelId deep link opens the canvas on first paint", () => { + // Canvas state is seeded from the path (not effect-only), so static + // markup sees the open column without running useEffect. + const markup = renderApp("/c/ch_deep"); + expect(markup).toMatch(/class="shell-canvas-column"[^>]*data-open="true"/); + }); }); diff --git a/packages/onboarding/src/routes.ts b/packages/onboarding/src/routes.ts index ba929f01c..e0a7bee7f 100644 --- a/packages/onboarding/src/routes.ts +++ b/packages/onboarding/src/routes.ts @@ -75,14 +75,41 @@ export function createOnboardingRoutes( // Optional body: the naming wizard sends `{ name }`; the shell's // membership probe may POST with no body and only wants the read path. // Parse before rate-limiting so the read probe never burns a create slot. - const rawBody: unknown = await c.req.json().catch(() => null); - const body = - rawBody === null - ? undefined - : (() => { - const parsed = ProvisionBody(rawBody); - return parsed instanceof type.errors ? undefined : parsed; - })(); + // Empty body → probe. Present body that is not valid JSON or fails the + // schema → 400 (never silently treated as a probe). + const bodyText = await c.req.text(); + let body: { name?: string } | undefined; + if (bodyText.trim() === "") { + body = undefined; + } else { + let rawBody: unknown; + try { + rawBody = JSON.parse(bodyText) as unknown; + } catch { + return c.json( + { + error: { + code: "bad_request", + message: "Request body must be valid JSON", + }, + }, + 400, + ); + } + const parsed = ProvisionBody(rawBody); + if (parsed instanceof type.errors) { + return c.json( + { + error: { + code: "bad_request", + message: "Invalid provision body", + }, + }, + 400, + ); + } + body = parsed; + } const isCreateAttempt = body?.name !== undefined; // Rate-limit only named creates. The two-step first-login flow is diff --git a/packages/onboarding/test/routes.test.ts b/packages/onboarding/test/routes.test.ts index f7c1565f3..24094d696 100644 --- a/packages/onboarding/test/routes.test.ts +++ b/packages/onboarding/test/routes.test.ts @@ -183,6 +183,72 @@ describe("POST /provision", () => { } }); + test("malformed JSON on /provision is 400, not a silent membership probe", async () => { + const creates: unknown[] = []; + const hub = new Hono(); + hub.get("/api/me/principals", (c) => + c.json({ data: [], nextCursor: null }), + ); + hub.post("/api/tenants", async (c) => { + creates.push(await c.req.json()); + return c.json({ id: "tnt_x" }, 201); + }); + const server = Bun.serve({ port: 0, fetch: hub.fetch }); + try { + const routes = createOnboardingRoutes({ + hubUrl: `http://localhost:${server.port}`, + pushWorkflow: async () => "pushed", + log: () => undefined, + }); + const app = mountAuthenticated(routes); + + const response = await app.request("/provision", { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{not-json", + }); + + expect(response.status).toBe(400); + const body = (await response.json()) as { + error: { code: string; message: string }; + }; + expect(body.error.code).toBe("bad_request"); + expect(creates).toEqual([]); + } finally { + server.stop(true); + } + }); + + test("schema-invalid provision body is 400, not a silent membership probe", async () => { + const hub = new Hono(); + hub.get("/api/me/principals", (c) => + c.json({ data: [], nextCursor: null }), + ); + const server = Bun.serve({ port: 0, fetch: hub.fetch }); + try { + const routes = createOnboardingRoutes({ + hubUrl: `http://localhost:${server.port}`, + pushWorkflow: async () => "pushed", + log: () => undefined, + }); + const app = mountAuthenticated(routes); + + const response = await app.request("/provision", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: 12 }), + }); + + expect(response.status).toBe(400); + const body = (await response.json()) as { + error: { code: string; message: string }; + }; + expect(body.error.code).toBe("bad_request"); + } finally { + server.stop(true); + } + }); + test("an anonymous request is rejected before provisioning runs", async () => { const routes = createOnboardingRoutes({ hubUrl: "http://127.0.0.1:0",