From 469d8a9af61557399a5fa34049c55d28bf724349 Mon Sep 17 00:00:00 2001 From: Travis Gilbert <1travisgilbert@gmail.com> Date: Sun, 2 Aug 2026 11:38:12 -0400 Subject: [PATCH] fix(console): say why a chat surface failed, not just that it did Travis' second screenshot showed the previous fix working: "/api/chat/projects answered 502." That named the request. It did not name the cause, so it still took a round trip to learn anything, and the route had known the cause the whole time. Two gaps, both mine. The route returns { error, message } and the message holds the inner reason. chatFailure kept the code and dropped the message, so the only part naming what actually broke was computed, sent over the wire, and discarded on arrival. DegradationOrigin now carries `reason` and it is rendered after the door and status, never instead of them. Second, 13 of the 15 error codes the /api/chat/* routes can emit were missing from WIRE_MAP: project_catalog_failed, project_write_failed, project_select_failed, thread_catalog_failed, thread_read_failed, thread_create_failed, thread_update_failed, thread_not_found, attachment_upload_failed, file_required, invalid_body, tenant_connector_unavailable, web_search_requires_principal. Every one fell through to "This surface cannot render right now." An unmapped code is not a neutral default; it is a sentence nobody can act on. A test now asserts each emitted code has a sentence of its own, so a new route code cannot silently join them. readableReason keeps CS15 intact: a bare wire code renders as its sentence rather than the identifier, and anything else is prose already. An unmapped bare code passes through, because inventing a sentence would hide the only identifier the reader could search for. The length bound had a real hole, caught by its own test. The code-shaped check matches any run of lowercase, so a 500-character token took that branch and returned before the cap. Bounding at the exit instead of inside one branch removes the class, not the instance. vitest run src/lib/degradation.test.ts 18 passed (+4) pnpm --filter @commonplace/console run build:railway exit 0 Rebased onto main rather than stashed, to keep the onTransport refinement that came in with #155: lastTransport is cleared unless the connection is actually disconnected, which is a stronger version of the correlation guarantee than what I wrote. Note for anyone building locally: #153 made build:railway run `pnpm --filter twenty-ui run build` first, and twenty-ui needs its own node_modules. A stale checkout fails with ERR_MODULE_NOT_FOUND '@vitejs/plugin-react-swc' from packages/twenty-ui/vite.config.ts. Run pnpm install; the lockfile does not move. --- apps/console/src/components/chat/ChatPage.tsx | 16 ++- apps/console/src/lib/degradation.test.ts | 53 +++++++ apps/console/src/lib/degradation.ts | 133 +++++++++++++++++- 3 files changed, 193 insertions(+), 9 deletions(-) diff --git a/apps/console/src/components/chat/ChatPage.tsx b/apps/console/src/components/chat/ChatPage.tsx index 70ce6050..50c1dc9f 100644 --- a/apps/console/src/components/chat/ChatPage.tsx +++ b/apps/console/src/components/chat/ChatPage.tsx @@ -49,7 +49,7 @@ const MESSAGE_PERSIST_DEBOUNCE_MS = 500; const EMPTY_CAPABILITIES: readonly CapabilityItem[] = []; /** A failed chat request, kept with the evidence needed to describe it. */ -type ChatFailure = { code: string; door?: string; status?: number }; +type ChatFailure = { code: string; door?: string; status?: number; reason?: string }; /** * Classify a rejected chat request. @@ -62,14 +62,23 @@ type ChatFailure = { code: string; door?: string; status?: number }; */ function chatFailure(error: unknown, door: string): ChatFailure { if (error instanceof ChatWireError) { + const code = error.wireCode ?? 'console_chat_wire_failed'; return { - code: error.wireCode ?? 'console_chat_wire_failed', + code, door: error.door, status: error.status ?? undefined, + // These routes wrap an inner failure and put its reason in `message`. + // That reason is the only part naming what actually broke, so keep it + // unless it just repeats the code the sentence already covers. + reason: error.message && error.message !== code ? error.message : undefined, }; } // fetch itself rejected, so there is no status: the request never landed. - return { code: 'console_chat_wire_failed', door }; + return { + code: 'console_chat_wire_failed', + door, + reason: error instanceof Error ? error.message : undefined, + }; } function connectionFor(status: number | null, error?: string | null): ConnectionState { @@ -459,6 +468,7 @@ export function ChatPage({ ? degradationFor(loadError.code, { door: loadError.door, status: loadError.status, + reason: loadError.reason, }) : connection === 'disconnected' ? degradationFor('console_data_api_unreachable', transportOrigin) diff --git a/apps/console/src/lib/degradation.test.ts b/apps/console/src/lib/degradation.test.ts index cc8bb76c..726064e0 100644 --- a/apps/console/src/lib/degradation.test.ts +++ b/apps/console/src/lib/degradation.test.ts @@ -140,6 +140,59 @@ describe('degradationFor origin evidence', () => { expect(chat.detail).toContain('502'); }); + // The failing route knows why it failed and says so in its body. Dropping + // that left the reader with a category ("answered 502") and no cause, which + // is what turned a real 502 into another round trip. + it('surfaces the upstream reason alongside the door and status', () => { + const result = degradationFor('project_catalog_failed', { + door: '/api/chat/projects', + status: 502, + reason: 'objects/query failed: 502', + }); + expect(result.cause).toBe('The chat project list could not be read.'); + expect(result.detail).toContain('/api/chat/projects'); + expect(result.detail).toContain('502'); + expect(result.detail).toContain('objects/query failed'); + }); + + // CS15 still holds: a bare wire code is not prose, so it renders as its + // sentence rather than as the identifier. + it('translates a wire-code reason into its sentence', () => { + const result = degradationFor('project_catalog_failed', { + door: '/api/chat/projects', + status: 502, + reason: 'console_data_api_unreachable', + }); + expect(result.detail).toContain('The data API is unreachable.'); + expect(result.detail).not.toContain('console_data_api_unreachable'); + }); + + it('bounds a runaway reason so a stack trace cannot become the banner', () => { + const result = degradationFor('project_catalog_failed', { + door: '/api/chat/projects', + status: 502, + reason: 'x'.repeat(500), + }); + expect(result.detail!.length).toBeLessThan(320); + expect(result.detail).toContain('...'); + }); + + it('every chat route error code has a sentence of its own', () => { + const emitted = [ + 'project_catalog_failed', 'project_write_failed', 'project_select_failed', + 'thread_catalog_failed', 'thread_read_failed', 'thread_create_failed', + 'thread_update_failed', 'thread_not_found', 'attachment_upload_failed', + 'file_required', 'invalid_body', 'tenant_connector_unavailable', + 'web_search_requires_principal', 'console_chat_wire_failed', + 'web_search_unavailable', + ]; + const generic = degradationFor('a_code_that_is_not_mapped_at_all').cause; + for (const code of emitted) { + expect(degradationFor(code).cause, `${code} fell through to the generic sentence`) + .not.toBe(generic); + } + }); + it('still keeps the wire code out of what the user sees', () => { const result = degradationFor('console_data_api_unreachable', { door: '/api/objects/views', diff --git a/apps/console/src/lib/degradation.ts b/apps/console/src/lib/degradation.ts index 8a4dad19..7217ea45 100644 --- a/apps/console/src/lib/degradation.ts +++ b/apps/console/src/lib/degradation.ts @@ -29,6 +29,14 @@ export type DegradationOrigin = { host?: string; /** HTTP status, when the request was answered at all. Omit for no answer. */ status?: number; + /** + * What the failing route said went wrong, when it said anything. + * + * A route that wraps an inner failure knows the reason and puts it in its + * body; dropping it leaves the reader with a category ("answered 502") and + * no cause. Rendered through `readableReason`, which keeps CS15 intact. + */ + reason?: string; }; type DegradationTemplate = @@ -104,6 +112,86 @@ const WIRE_MAP: Record = { actionLabel: 'Retry', door: 'The chat wire', }, + // Every code the /api/chat/* routes can emit. Before these, all of them fell + // through to GENERIC_UNAVAILABLE, so a failed project read, a failed thread + // write and a missing attachment all said "This surface cannot render right + // now." An unmapped code is not a neutral default: it is a sentence that + // cannot be acted on. + project_catalog_failed: { + level: 'unavailable', + cause: 'The chat project list could not be read.', + actionLabel: 'Retry', + door: 'The chat project catalog', + }, + project_write_failed: { + level: 'unavailable', + cause: 'That chat project could not be saved.', + actionLabel: 'Retry', + door: 'The chat project catalog', + }, + project_select_failed: { + level: 'unavailable', + cause: 'That project could not be made active.', + actionLabel: 'Retry', + door: 'The chat project catalog', + }, + thread_catalog_failed: { + level: 'unavailable', + cause: 'The thread list could not be read.', + actionLabel: 'Retry', + door: 'The chat thread catalog', + }, + thread_read_failed: { + level: 'unavailable', + cause: 'That thread could not be read.', + actionLabel: 'Retry', + door: 'The chat thread catalog', + }, + thread_create_failed: { + level: 'unavailable', + cause: 'That thread could not be created.', + actionLabel: 'Retry', + door: 'The chat thread catalog', + }, + thread_update_failed: { + level: 'unavailable', + cause: 'That thread could not be saved.', + actionLabel: 'Retry', + door: 'The chat thread catalog', + }, + thread_not_found: { + level: 'unavailable', + cause: 'That thread no longer exists.', + door: 'The chat thread catalog', + }, + attachment_upload_failed: { + level: 'unavailable', + cause: 'That attachment could not be uploaded.', + actionLabel: 'Retry', + door: 'The chat attachment upload', + }, + file_required: { + level: 'unavailable', + cause: 'That upload arrived without a file.', + door: 'The chat attachment upload', + }, + invalid_body: { + level: 'unavailable', + cause: 'The console sent a request this route could not read.', + door: 'The chat wire', + }, + tenant_connector_unavailable: { + level: 'unavailable', + cause: 'No connector is available for this tenant.', + actionLabel: 'Open Account', + door: 'The chat wire', + }, + web_search_requires_principal: { + level: 'unavailable', + cause: 'Web search needs a signed-in principal.', + actionLabel: 'Open Account', + door: 'Web search', + }, web_search_unavailable: { level: 'reduced', cause: 'Web search is unavailable.', @@ -202,18 +290,47 @@ export function degradationFor( * credential problem rather than an outage. Returns undefined when there is * nothing concrete to say, so no caller is forced to show an empty line. */ +/** + * Render an upstream reason without leaking a wire code to the reader. + * + * Inner failures often surface as a bare code (`console_data_api_unreachable`) + * rather than prose. CS15 keeps codes off the screen, so a code is translated + * to its sentence; anything else is real prose already and passes through, + * bounded so a stack trace cannot become the banner. + */ +function readableReason(reason: string | undefined): string | undefined { + const trimmed = reason?.trim(); + if (!trimmed) return undefined; + const rendered = /^[a-z][a-z0-9_]*$/.test(trimmed) + // An unmapped bare code has no sentence to show, and inventing one would + // hide the only identifier the reader could search for. + ? (WIRE_MAP[trimmed]?.cause ?? trimmed) + : trimmed; + // Bound at the exit, not inside one branch. The code-shaped test matches any + // run of lowercase, so a long token took the other path and escaped the cap. + return rendered.length > 160 ? `${rendered.slice(0, 157)}...` : rendered; +} + export function describeOrigin(origin: DegradationOrigin | undefined): string | undefined { if (!origin) return undefined; const { door, host, status } = origin; - if (!door && !host && status === undefined) return undefined; + const reason = readableReason(origin.reason); + if (!door && !host && status === undefined) return reason; const subject = [door ?? 'The request', host ? `at ${host}` : null].filter(Boolean).join(' '); + // The reason is the innermost thing known about the failure, so it goes last + // and never replaces the door and status a reader needs to locate it. + const withReason = (sentence: string) => (reason ? `${sentence} ${reason}` : sentence); if (status === undefined) { - return `${subject} did not answer. That is DNS, the network, or a blocked origin, not a status code.`; + return withReason( + `${subject} did not answer. That is DNS, the network, or a blocked origin, not a status code.`, + ); } if (status === 401) { - return `${subject} answered 401. The request was not authenticated, which is a credential problem rather than an outage.`; + return withReason( + `${subject} answered 401. The request was not authenticated, which is a credential problem rather than an outage.`, + ); } if (status === 403) { // Not the same failure as 401, and saying so matters. connectionFor maps @@ -221,12 +338,16 @@ export function describeOrigin(origin: DegradationOrigin | undefined): string | // active_workspace_claim_required and active_workspace_membership_refused. // In those cases the credential is fine and simply does not reach this // workspace, so "fix your credential" would send the reader the wrong way. - return `${subject} answered 403. The credential was accepted but refused for this workspace, which is not an outage.`; + return withReason( + `${subject} answered 403. The credential was accepted but refused for this workspace, which is not an outage.`, + ); } if (status === 404) { - return `${subject} answered 404. Check the configured URL before assuming the service is down.`; + return withReason( + `${subject} answered 404. Check the configured URL before assuming the service is down.`, + ); } - return `${subject} answered ${status}.`; + return withReason(`${subject} answered ${status}.`); } /** Collapse a list of missing capability codes into one reduced marker. */