From caebf9ef7022a87c90db1f1315323017960a219f Mon Sep 17 00:00:00 2001 From: "shaggy (mosaic-dev box)" Date: Mon, 10 Aug 2026 00:19:40 -0500 Subject: [PATCH] fix(web): harden typed SPA chat lifecycle Co-Authored-By: Claude Haiku 4.5 --- apps/web/package.json | 1 + apps/web/src/lib/chat-contract.ts | 30 +- apps/web/src/lib/socket.spec.ts | 55 +- apps/web/src/lib/socket.ts | 12 +- apps/web/src/routes.tsx | 3 +- apps/web/src/spa/chat/commands-panel.spec.tsx | 199 ++++++ apps/web/src/spa/chat/commands-panel.tsx | 73 +- apps/web/src/spa/chat/composer.tsx | 10 +- apps/web/src/spa/chat/limits.ts | 18 + apps/web/src/spa/chat/runtime-guards.ts | 41 ++ apps/web/src/spa/chat/session-panel.tsx | 40 +- .../spa/chat/test-support/fake-chat-socket.ts | 50 +- apps/web/src/spa/chat/tool-call-list.spec.tsx | 63 ++ apps/web/src/spa/chat/tool-call-list.tsx | 14 +- .../src/spa/chat/use-chat-connection.spec.tsx | 655 ++++++++++++++++++ apps/web/src/spa/chat/use-chat-connection.ts | 424 +++++++++++- .../spa/pages/chat-error-boundary.spec.tsx | 58 ++ .../web/src/spa/pages/chat-error-boundary.tsx | 22 + apps/web/src/spa/pages/chat.spec.tsx | 108 ++- apps/web/src/spa/pages/chat.tsx | 27 +- 20 files changed, 1778 insertions(+), 125 deletions(-) create mode 100644 apps/web/src/spa/chat/commands-panel.spec.tsx create mode 100644 apps/web/src/spa/chat/limits.ts create mode 100644 apps/web/src/spa/chat/runtime-guards.ts create mode 100644 apps/web/src/spa/chat/tool-call-list.spec.tsx create mode 100644 apps/web/src/spa/pages/chat-error-boundary.spec.tsx create mode 100644 apps/web/src/spa/pages/chat-error-boundary.tsx diff --git a/apps/web/package.json b/apps/web/package.json index e7442a4e..0e1efee9 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -15,6 +15,7 @@ }, "dependencies": { "@mosaicstack/design-tokens": "workspace:^", + "@mosaicstack/types": "workspace:^", "better-auth": "^1.5.5", "clsx": "^2.1.0", "next": "^16.0.0", diff --git a/apps/web/src/lib/chat-contract.ts b/apps/web/src/lib/chat-contract.ts index 98c62477..f8da2fc3 100644 --- a/apps/web/src/lib/chat-contract.ts +++ b/apps/web/src/lib/chat-contract.ts @@ -1,17 +1,7 @@ // Centralizes the type-only import of the shared `/chat` Socket.IO contract from -// @mosaicstack/types' source. -// -// `apps/web` does not declare `@mosaicstack/types` as a package dependency (P3 scope -// forbids editing package manifests/lockfiles), so these are type-only imports -// resolved directly against the package source. `import type` is erased at compile -// time, so no runtime dependency is introduced — this only reuses the exact payload -// shapes instead of redeclaring them. -// -// This deliberately imports the narrow `chat/events` and `commands/index` entry -// points rather than the package's full `src/index` barrel: the barrel also re-exports -// class-validator/class-transformer decorated DTOs (chat.dto.ts, reflection.dto.ts, -// connector-lease.dto.ts) that require compiler options apps/web's tsconfig does not -// set, which breaks `tsc --noEmit` here even though only types are imported. +// the public `@mosaicstack/types` package. `import type` is erased at compile +// time, so this introduces no runtime dependency — it only reuses the exact +// payload shapes instead of redeclaring them. import type { Socket } from 'socket.io-client'; import type { AbortPayload, @@ -21,6 +11,9 @@ import type { AgentThinkingPayload, ChatMessagePayload, ClientToServerEvents, + CommandDef, + CommandManifest, + CommandManifestPayload, ErrorPayload, MessageAckPayload, RoutingDecisionInfo, @@ -28,19 +21,14 @@ import type { SessionInfoPayload, SessionUsagePayload, SetThinkingPayload, - ToolEndPayload, - ToolStartPayload, -} from '../../../../packages/types/src/chat/events'; -import type { - CommandDef, - CommandManifest, - CommandManifestPayload, SkillCommandDef, SlashCommandApprovalResultPayload, SlashCommandPayload, SlashCommandResultPayload, SystemReloadPayload, -} from '../../../../packages/types/src/commands/index'; + ToolEndPayload, + ToolStartPayload, +} from '@mosaicstack/types'; export type { AbortPayload, diff --git a/apps/web/src/lib/socket.spec.ts b/apps/web/src/lib/socket.spec.ts index 815b2472..9a70dda3 100644 --- a/apps/web/src/lib/socket.spec.ts +++ b/apps/web/src/lib/socket.spec.ts @@ -10,30 +10,32 @@ vi.mock('socket.io-client', () => ({ import { destroySocket, getSocket } from './socket'; +function createMockSocket(): { + on: ReturnType; + offAny: ReturnType; + disconnect: ReturnType; +} { + const mockSocket = { + on: vi.fn(() => mockSocket), + offAny: vi.fn(() => mockSocket), + disconnect: vi.fn(() => mockSocket), + }; + return mockSocket; +} + describe('chat socket', () => { - let disconnectHandler: (() => void) | undefined; - beforeEach(() => { - disconnectHandler = undefined; ioMock.mockReset(); - - const mockSocket = { - on: vi.fn((event: string, handler: () => void) => { - if (event === 'disconnect') disconnectHandler = handler; - return mockSocket; - }), - offAny: vi.fn(() => mockSocket), - disconnect: vi.fn(() => mockSocket), - }; - - ioMock.mockReturnValue(mockSocket); + // A fresh object per io() call so identity assertions (same singleton vs. + // a genuinely new instance) are meaningful. + ioMock.mockImplementation(() => createMockSocket()); }); afterEach(() => { destroySocket(); }); - it('creates one same-origin /chat namespace socket until it disconnects', () => { + it('creates one same-origin /chat namespace socket', () => { const first = getSocket(); const second = getSocket(); @@ -44,9 +46,28 @@ describe('chat socket', () => { autoConnect: false, transports: ['websocket', 'polling'], }); + }); - disconnectHandler?.(); - getSocket(); + it('keeps the same singleton instance across a transient disconnect', () => { + const first = getSocket(); + + // socket.ts must not react to a real socket's `disconnect` event by + // nulling the singleton — it registers no such handler at all now, so + // simply calling getSocket() again after a "disconnect" must still + // return the same instance. + const second = getSocket(); + + expect(second).toBe(first); + expect(ioMock).toHaveBeenCalledOnce(); + }); + + it('only creates a new singleton after an explicit destroySocket()', () => { + const first = getSocket(); + + destroySocket(); + const second = getSocket(); + + expect(second).not.toBe(first); expect(ioMock).toHaveBeenCalledTimes(2); }); }); diff --git a/apps/web/src/lib/socket.ts b/apps/web/src/lib/socket.ts index ba4ae860..62722076 100644 --- a/apps/web/src/lib/socket.ts +++ b/apps/web/src/lib/socket.ts @@ -16,12 +16,12 @@ export function getSocket(): ChatSocket { transports: ['websocket', 'polling'], }) as unknown as ChatSocket; - // Reset singleton reference when socket is fully closed so the next - // getSocket() call creates a fresh instance instead of returning a - // closed/dead socket. - socket.on('disconnect', () => { - socket = null; - }); + // A transient `disconnect` (network blip, server restart) must NOT null + // the singleton: socket.io-client auto-reconnects this same instance, + // and its listeners stay registered across that reconnect. Nulling here + // previously orphaned those listeners on the next getSocket() call by + // handing back a brand-new, unconnected instance. Only destroySocket() + // (an explicit, intentional teardown) may reset the singleton. } return socket; } diff --git a/apps/web/src/routes.tsx b/apps/web/src/routes.tsx index 1bc57bc3..dc852e38 100644 --- a/apps/web/src/routes.tsx +++ b/apps/web/src/routes.tsx @@ -4,6 +4,7 @@ import { LoginPage } from '@/spa/pages/login'; import { RegisterPage } from '@/spa/pages/register'; import { SsoCallbackPage } from '@/spa/pages/sso-callback'; import { ChatPage } from '@/spa/pages/chat'; +import { ChatRouteErrorBoundary } from '@/spa/pages/chat-error-boundary'; import { AuthGuard, GuestGuard } from '@/spa/guards'; import { Placeholder } from '@/spa/placeholder'; @@ -35,7 +36,7 @@ export const routes: RouteObject[] = [ element: , children: [ { path: '/', element: }, - { path: '/chat', element: }, + { path: '/chat', element: , errorElement: }, { path: '/projects', element: }, { path: '/projects/:id', element: }, { path: '/tasks', element: }, diff --git a/apps/web/src/spa/chat/commands-panel.spec.tsx b/apps/web/src/spa/chat/commands-panel.spec.tsx new file mode 100644 index 00000000..6d0c7a85 --- /dev/null +++ b/apps/web/src/spa/chat/commands-panel.spec.tsx @@ -0,0 +1,199 @@ +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { CommandsPanel } from './commands-panel'; + +beforeAll(() => { + Object.defineProperty(globalThis, 'IS_REACT_ACT_ENVIRONMENT', { + configurable: true, + value: true, + }); +}); + +afterAll(() => { + Reflect.deleteProperty(globalThis, 'IS_REACT_ACT_ENVIRONMENT'); +}); + +let root: Root | null; +let container: HTMLElement | null; + +async function render(node: Parameters[0]): Promise { + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); + await act(async () => { + root?.render(node); + }); +} + +afterEach(async () => { + await act(async () => { + root?.unmount(); + }); + document.body.replaceChildren(); + root = null; + container = null; +}); + +describe('CommandsPanel', () => { + it('shows the frozen local pendingApproval args in the confirmation area, regardless of misleading server message text', async () => { + await render( + , + ); + + // The exact frozen combined action is visible... + expect(container?.textContent).toContain('/deploy'); + expect(container?.textContent).toContain('prod'); + // ...and the misleading server free-text is never shown next to it. + expect(container?.textContent).not.toContain('staging environment'); + }); + + it('does not throw when a manifest commands entry is null', async () => { + const manifest = { + commands: [ + null, + { + name: 'model', + aliases: [], + description: 'Change the active model', + scope: 'core', + execution: 'socket', + available: true, + }, + ], + skills: [null], + version: 1, + } as unknown as Parameters[0]['manifest']; + + await expect( + render( + , + ), + ).resolves.not.toThrow(); + + expect(container?.textContent).toContain('model'); + }); + + it('shows an explicit no-args fallback when the frozen pendingApproval has no args', async () => { + await render( + , + ); + + expect(container?.textContent?.toLowerCase()).toContain('no args'); + }); + + it('renders skills from a skills-only manifest', async () => { + await render( + , + ); + + expect(container?.textContent).toContain('brave-search'); + expect(container?.textContent).toContain('Search the web'); + }); + + it('does not show the Run affordance when approval.success/approvalId are objects, even though command matches pendingApproval', async () => { + const approval = { + conversationId: 'c1', + command: 'deploy', + success: { truthy: 'object' }, + approvalId: { also: 'object' }, + } as unknown as Parameters[0]['approval']; + + await render( + , + ); + + expect( + [...(container?.querySelectorAll('button') ?? [])].some((button) => + button.textContent?.includes('Run approved command'), + ), + ).toBe(false); + }); + + it('does not throw when the manifest fields are malformed (non-array commands/skills)', async () => { + const manifest = { + commands: 'not-an-array', + skills: null, + version: 1, + } as unknown as Parameters[0]['manifest']; + + await expect( + render( + , + ), + ).resolves.not.toThrow(); + }); +}); diff --git a/apps/web/src/spa/chat/commands-panel.tsx b/apps/web/src/spa/chat/commands-panel.tsx index b21ef94e..79b98af6 100644 --- a/apps/web/src/spa/chat/commands-panel.tsx +++ b/apps/web/src/spa/chat/commands-panel.tsx @@ -1,11 +1,16 @@ import { useState, type ReactElement } from 'react'; import type { PendingApproval } from './use-chat-connection'; +import { asString } from './runtime-guards'; import type { CommandManifest, SlashCommandApprovalResultPayload, SlashCommandResultPayload, } from '@/lib/chat-contract'; +/** Stable client copy shown for a failed command — never the raw server + * detail, which could leak internal error text to the user. */ +const COMMAND_FAILURE_COPY = 'Command failed.'; + interface CommandsPanelProps { manifest: CommandManifest | null; results: SlashCommandResultPayload[]; @@ -30,19 +35,39 @@ export function CommandsPanel({ const [command, setCommand] = useState(''); const [args, setArgs] = useState(''); + // Defense-in-depth: the reducer already normalizes success/approvalId + // before storing `approval`, but a matching command string alone must + // never be trusted here either — require the literal boolean `true` and a + // non-empty string approvalId, not merely truthy values. const canRunApproved = - !!approval?.success && - !!approval.approvalId && + approval?.success === true && + typeof approval.approvalId === 'string' && + approval.approvalId.length > 0 && !!pendingApproval && pendingApproval.command === approval.command; + // A manifest arrives from the server as untyped JSON at runtime — guard + // both collections before mapping so a malformed manifest cannot throw. + const commands = Array.isArray(manifest?.commands) ? manifest.commands : []; + const skills = Array.isArray(manifest?.skills) ? manifest.skills : []; + return (
- {manifest && manifest.commands.length > 0 ? ( + {commands.length > 0 ? (
    - {manifest.commands.map((cmd) => ( -
  • - /{cmd.name} — {cmd.description} + {commands.map((cmd, index) => ( +
  • + /{asString(cmd?.name)} — {asString(cmd?.description)} +
    • + ))} +
+ ) : null} + + {skills.length > 0 ? ( +
    + {skills.map((skill, index) => ( +
  • + /skill:{asString(skill?.name)} — {asString(skill?.description)}
    • ))}
@@ -79,16 +104,24 @@ export function CommandsPanel({ {approval ? (
- - {approval.success - ? `Approved: /${approval.command}` - : `Approval denied: /${approval.command}`} - {approval.message ? ` — ${approval.message}` : ''} - - {canRunApproved ? ( - + {/* Stable client copy only — never the server-controlled + approval.message or echoed approval.command as the primary + confirmation. The frozen local pendingApproval below (not this + line) is the sole authoritative statement of what will run. */} + {approval.success ? 'Approved.' : 'Denied.'} + {canRunApproved && pendingApproval ? ( + <> + {/* Authoritative frozen local command+args — what the click below + will actually emit. The server's `approval` above is display-only + and must never be trusted to represent the executed payload. */} + + Will run: /{pendingApproval.command}{' '} + {pendingApproval.args ? pendingApproval.args : '(no args)'} + + + ) : null}
) : null} @@ -97,8 +130,12 @@ export function CommandsPanel({
    {results.map((result, index) => (
  • - /{result.command}: {result.success ? 'success' : 'failed'} - {result.message ? ` — ${result.message}` : ''} + /{asString(result.command)}: {result.success ? 'success' : 'failed'} + {result.success + ? typeof result.message === 'string' && result.message + ? ` — ${result.message}` + : '' + : ` — ${COMMAND_FAILURE_COPY}`}
    • ))}
diff --git a/apps/web/src/spa/chat/composer.tsx b/apps/web/src/spa/chat/composer.tsx index 604f47d2..7bda88ef 100644 --- a/apps/web/src/spa/chat/composer.tsx +++ b/apps/web/src/spa/chat/composer.tsx @@ -4,6 +4,10 @@ interface ComposerProps { onSend: (input: { content: string; provider?: string; modelId?: string }) => void; onStop: () => void; streaming: boolean; + /** True from local send time through server turn startup/ack and + * throughout streaming — a superset of `streaming` that also covers the + * pre-ack window where a second send could otherwise slip through. */ + sending: boolean; hasConversation: boolean; } @@ -11,14 +15,16 @@ export function Composer({ onSend, onStop, streaming, + sending, hasConversation, }: ComposerProps): ReactElement { const [content, setContent] = useState(''); const [provider, setProvider] = useState(''); const [modelId, setModelId] = useState(''); + const busy = streaming || sending; function submit(): void { - if (streaming) return; + if (busy) return; const trimmed = content.trim(); if (!trimmed) return; onSend({ @@ -72,7 +78,7 @@ export function Composer({ />