fix(console): say why a chat surface failed, not just that it did - #156
Conversation
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.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughWalkthroughChat failures now retain upstream reasons. The degradation system translates mapped wire codes, preserves prose, limits reason length, and appends reasons to rendered failure details. Tests cover these behaviors and chat-route mappings. ChangesChat failure reason rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPage
participant degradationFor
participant describeOrigin
ChatPage->>degradationFor: pass failure code and reason
degradationFor->>describeOrigin: build degradation details
describeOrigin-->>ChatPage: return bounded readable reason
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/console/src/components/chat/ChatPage.tsx`:
- Around line 65-81: Sanitize the reason produced by the error-mapping flow
around the visible wire-error handling before exposing it to the UI: retain only
server-authored display-safe reasons or mapped wire-code text, never raw caught
Error.message values. At apps/console/src/components/chat/ChatPage.tsx lines
65-81, update the mapping to exclude arbitrary server exception text; at line
471, pass only that sanitized reason into degradationFor.
In `@apps/console/src/lib/degradation.test.ts`:
- Around line 180-194: Update the test named “every chat route error code has a
sentence of its own” to collect each emitted code’s cause and assert that the
set of causes has size equal to emitted.length. Keep the existing
generic-fallback assertion, but add the uniqueness check so duplicate sentences
among mapped codes fail.
In `@apps/console/src/lib/degradation.ts`:
- Around line 301-311: The fallback in readableReason for unmapped bare
identifiers must return safe generic prose instead of exposing trimmed. Update
readableReason while preserving mapped-code and sentence-like reason handling,
then add coverage for an unmapped bare origin.reason through describeOrigin or
the relevant public path, asserting the identifier is not rendered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22224223-b8be-4b2e-a527-95b507358ea4
📒 Files selected for processing (3)
apps/console/src/components/chat/ChatPage.tsxapps/console/src/lib/degradation.test.tsapps/console/src/lib/degradation.ts
| 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, | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not render arbitrary server exception text.
The route contract at apps/console/src/app/api/chat/projects/route.ts:12-20 returns raw caught Error.message. The new flow stores and displays it. The 160-character limit restricts size but does not remove internal or sensitive content.
apps/console/src/components/chat/ChatPage.tsx#L65-L81: retain only a server-authored, display-safe reason or a mapped wire code.apps/console/src/components/chat/ChatPage.tsx#L471-L471: pass only the sanitized reason intodegradationFor.
📍 Affects 1 file
apps/console/src/components/chat/ChatPage.tsx#L65-L81(this comment)apps/console/src/components/chat/ChatPage.tsx#L471-L471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/src/components/chat/ChatPage.tsx` around lines 65 - 81, Sanitize
the reason produced by the error-mapping flow around the visible wire-error
handling before exposing it to the UI: retain only server-authored display-safe
reasons or mapped wire-code text, never raw caught Error.message values. At
apps/console/src/components/chat/ChatPage.tsx lines 65-81, update the mapping to
exclude arbitrary server exception text; at line 471, pass only that sanitized
reason into degradationFor.
| 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); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test uniqueness between emitted chat failure sentences.
Line 190 compares each cause only with the unmapped fallback. Two emitted codes can share the same cause and this test still passes. Collect the emitted causes and assert that their set size equals emitted.length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/src/lib/degradation.test.ts` around lines 180 - 194, Update the
test named “every chat route error code has a sentence of its own” to collect
each emitted code’s cause and assert that the set of causes has size equal to
emitted.length. Keep the existing generic-fallback assertion, but add the
uniqueness check so duplicate sentences among mapped codes fail.
| 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; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep unknown wire identifiers out of the rendered detail.
Line 307 returns trimmed when a bare reason is not in WIRE_MAP. describeOrigin then displays that identifier to the user. This breaks the documented CS15 behavior and the PR requirement for unmapped codes.
Replace this fallback with safe generic prose. Add a test with an unmapped bare origin.reason.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/console/src/lib/degradation.ts` around lines 301 - 311, The fallback in
readableReason for unmapped bare identifiers must return safe generic prose
instead of exposing trimmed. Update readableReason while preserving mapped-code
and sentence-like reason handling, then add coverage for an unmapped bare
origin.reason through describeOrigin or the relevant public path, asserting the
identifier is not rendered.
There was a problem hiding this comment.
Pull request overview
This PR improves console degraded-state messaging for chat surfaces by preserving and rendering the upstream failure reason (in addition to door + status), and by mapping previously-unmapped chat route error codes to actionable sentences. It also adds tests to prevent future regressions where new chat error codes silently fall back to the generic unavailable banner.
Changes:
- Extend
DegradationOriginwithreasonand render it via a boundedreadableReasonhelper. - Expand
WIRE_MAPwith per-route chat failure codes so chat failures produce specific, actionable sentences. - Add vitest coverage to ensure upstream reasons render and chat route codes don’t regress to the generic sentence; update
ChatPageto propagate the reason field.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/console/src/lib/degradation.ts | Adds origin.reason, introduces readableReason, and expands WIRE_MAP for chat error codes so details include cause context. |
| apps/console/src/lib/degradation.test.ts | Adds tests asserting upstream reasons surface, reasons are bounded, and chat codes don’t fall through to the generic sentence. |
| apps/console/src/components/chat/ChatPage.tsx | Preserves route-provided reason text in ChatFailure and passes it into degradationFor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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', | ||
| ]; |
| 'web_search_requires_principal', 'console_chat_wire_failed', | ||
| 'web_search_unavailable', | ||
| ]; | ||
| const generic = degradationFor('a_code_that_is_not_mapped_at_all').cause; |
| 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', | ||
| }, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469d8a9af6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) |
There was a problem hiding this comment.
When an inner API returns a newly introduced or otherwise unmapped code as its reason, this fallback returns the identifier verbatim, and describeOrigin appends it directly to the user-facing banner. That contradicts this module's CS15 invariant that unmapped wire codes use generic prose and are reported only in development; fall back to the generic sentence or omit the reason instead of exposing trimmed.
Useful? React with 👍 / 👎.
| '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', |
There was a problem hiding this comment.
The asserted list omits activeProjectId_required, which /api/chat/projects emits at apps/console/src/app/api/chat/projects/route.ts:53 when the PUT body has no active project ID. That code is also absent from WIRE_MAP, so this case still falls through to the generic degradation sentence while the new test claiming to cover every chat route code passes; include this emitted code and its user-facing mapping, or derive the test inputs from a shared exhaustive source.
Useful? React with 👍 / 👎.
The previous fix worked: the banner went from "The data API is unreachable." to
That names the request. It does not name the cause — and the route knew the cause the whole time. Two gaps, both mine.
1. The reason was computed, sent, and thrown away
/api/chat/projectsreturns{ error, message }wheremessageholds the inner reason.chatFailurekept the code and dropped the message.DegradationOriginnow carriesreason, rendered after the door and status, never instead of them:/api/chat/projects answered 502./api/chat/projects answered 502. objects/query failed: 5022. Thirteen of fifteen chat error codes had no sentence
Only
console_chat_wire_failedandweb_search_unavailablewere mapped. These all fell through to the generic sentence:That is why you saw "This surface cannot render right now" rather than something specific:
project_catalog_failedwas unmapped. An unmapped code is not a neutral default, it is a sentence nobody can act on.A test now asserts every emitted code has a sentence distinct from the generic one, so a new route code cannot silently join them.
CS15 still holds
readableReasonrenders a bare wire code as its sentence, not the identifier:An unmapped bare code passes through verbatim, deliberately: inventing a sentence would hide the only identifier the reader could search for.
A real hole its own test caught
The length bound was inside one branch. The code-shaped check
/^[a-z][a-z0-9_]*$/matches any run of lowercase, so a 500-character token took that path and returned before the cap — 533 characters into the banner. Bounding at the exit removes the class rather than the instance.Verification
vitest run src/lib/degradation.test.tspnpm --filter @commonplace/console run build:railwayTwo notes for reviewers
Rebased, not stashed. This keeps the
onTransportrefinement that arrived with #155 —lastTransportis cleared unless the connection is actuallydisconnected, which is a stronger version of the correlation guarantee than what I wrote. Verified present after the rebase.Local build needs an install. #153 made
build:railwayrunpnpm --filter twenty-ui run buildfirst, andtwenty-uineeds its ownnode_modules. A stale checkout fails withERR_MODULE_NOT_FOUND: '@vitejs/plugin-react-swc'frompackages/twenty-ui/vite.config.ts, which reads like a broken config and is not. Runpnpm install; the lockfile does not move.Summary by CodeRabbit