✨(error) display specific error when LLM provider is down#495
Open
maxenceh wants to merge 2 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBackend streaming maps provider/HTTP/SDK exceptions to deterministic ErrorPart events; a STATUS_PAGE_URL setting is added and exposed by the config API; frontend classifies errors as provider-specific or generic, renders type-specific messages/placeholders with optional status-page links, and disables input for provider failures. Tests and docs updated. ChangesLLM Provider Error Handling and Status Page Integration
Sequence Diagram(s)sequenceDiagram
participant Chat
participant onErrorChat
participant ChatError
participant ConfigAPI
Chat->>onErrorChat: catch error (code)
onErrorChat->>Chat: set chatErrorType
Chat->>ChatError: render with errorType
ChatError->>ConfigAPI: read STATUS_PAGE_URL
alt Provider error
ChatError->>ChatError: show provider message + status link
else Generic error
ChatError->>ChatError: show retry / new conversation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
3 times, most recently
from
2dcd509 to
f43b8e2
Compare
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
4 times, most recently
from
8999f50 to
bdb4258
Compare
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
from
bdb4258 to
cf91b3f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/backend/chat/clients/pydantic_ai.py (1)
373-378: ⚡ Quick winLog caught provider exceptions with traceback context.
Use
logger.exception(...)in theseexceptblocks so stack traces are retained for incident debugging.♻️ Proposed refactor
- logger.error( + logger.exception( "LLM provider HTTP error (status=%s) for conversation %s: %s", exc.status_code, self.conversation.pk, exc, ) @@ - logger.error( + logger.exception( "LLM provider connection error for conversation %s: %s", self.conversation.pk, exc, )Also applies to: 383-387
🤖 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 `@src/backend/chat/clients/pydantic_ai.py` around lines 373 - 378, The caught provider exceptions logged with logger.error in PydanticAIClient (see the except blocks that reference exc and the conversation via self.conversation.pk around the logger.error calls) should use logger.exception(...) so the stack trace is recorded; update those except handlers (the blocks emitting the "LLM provider HTTP error..." message and the similar block at lines ~383-387) to call logger.exception with the same formatted message and parameters (including exc/context) instead of logger.error.src/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsx (1)
148-156: ⚡ Quick winAdd coverage for provider-specific error behavior.
This update validates generic error handling, but the new provider branch (
errorType !== 'generic') should also be asserted (disabled textarea, provider placeholder, no suggestion carousel).Suggested additional test
+ it('should disable textarea and hide suggestions for provider errors', () => { + render( + <InputChat + {...defaultProps} + status="error" + errorType="model_unavailable" + />, + { wrapper: AppWrapper }, + ); + + expect( + screen.getByRole('textbox', { name: 'Enter your message or a question' }), + ).toBeDisabled(); + expect( + screen.getByPlaceholderText( + 'The assistant is temporarily unavailable. Please try again later.', + ), + ).toBeInTheDocument(); + expect(screen.queryByTestId('suggestion-carousel')).not.toBeInTheDocument(); + });🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsx` around lines 148 - 156, Add a new test in InputChat.test.tsx that covers the provider-specific error branch by rendering <InputChat {...defaultProps} status="error" errorType="provider" /> (or the non-'generic' value used in the component) and assert that the textbox returned by getByRole('textbox', { name: 'Enter your message or a question' }) is disabled, the placeholder or visible label shows the provider-specific message, and that the suggestion carousel component (the carousel test id or role used in the suite) is not present (use queryByTestId/queryByRole and expect(...).toBeNull()/not.toBeInTheDocument()). Ensure you reuse defaultProps and AppWrapper like the existing tests and name the spec to reflect "provider-specific error behavior".src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx (1)
247-260: ⚡ Quick winExtract provider error messages into a shared constant.
These strings duplicate the message mapping in
ChatError.tsx; they can drift over time. A sharedChatErrorType -> messagemap would keep error banner and input placeholder copy consistent.🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx` around lines 247 - 260, The PROVIDER_ERROR_PLACEHOLDERS map and its use to compute textareaPlaceholder in InputChat.tsx duplicate the mapping in ChatError.tsx; extract that mapping into a shared exported constant (e.g., PROVIDER_ERROR_MESSAGES or getProviderErrorMessage) in the module that currently owns the canonical copy (or a new shared utilities file), export it, and then import and use it in InputChat.tsx by replacing the local PROVIDER_ERROR_PLACEHOLDERS and the conditional that sets textareaPlaceholder so both the banner (ChatError.tsx) and input placeholder (InputChat.tsx) use the same source of truth.
🤖 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 `@CHANGELOG.md`:
- Around line 20-21: Remove the duplicated release heading by keeping a single
"## [0.0.16] - 2026-05-21" section and deleting the extra identical heading; if
both headings have content, merge their content under the one remaining "##
[0.0.16] - 2026-05-21" section so no release notes are lost and the changelog
headings remain unique and parser-friendly.
In `@docs/errors.md`:
- Around line 7-17: The docs/errors.md table is out of sync with the
implementation: add the missing backend error codes (`model_busy`,
`model_not_found`, `model_wrong_type`) into the mapping rows (with their
corresponding pydantic-ai exceptions and user-facing messages) and update the
"Status page link" rule for STATUS_PAGE_URL to match frontend behavior (show the
"Check service status" link for all provider errors except `generic`, i.e., any
non-`generic` provider error). Ensure the table entries use the same error-code
identifiers (`model_unavailable`, `model_rate_limited`,
`model_connection_error`, `model_busy`, `model_not_found`, `model_wrong_type`,
`generic`) so they align with the backend/front-end implementation.
In `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 379-381: The error mapping in _stream_content always uses
self.event_encoder.encode(error_event), which can emit data-protocol frames even
when the selected stream protocol is text-only; change the code to branch on the
active stream protocol (e.g., check self.stream_protocol or similar flag used
elsewhere) and encode using the appropriate encoder: use self.text_encoder (or
the text-path encoder) to produce plain text frames for the text-stream path and
use self.event_encoder for the data-protocol path; apply the same fix for the
other occurrence that maps ErrorPart (the block at the later lines) so both
places respect the selected protocol.
In `@src/frontend/apps/conversations/src/features/chat/components/ChatError.tsx`:
- Around line 56-58: The status-page link in the ChatError component is
icon-only and needs an accessible name; update the anchor wrapping the Icon (the
<a href={statusPageUrl} ...> that contains <Icon iconName="info" ... />) to
include a descriptive accessible label (e.g., add aria-label="Open status page"
or aria-label={`Open status page (${serviceName})`}) or include visually hidden
text inside the link, so screen readers announce the link purpose.
---
Nitpick comments:
In `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 373-378: The caught provider exceptions logged with logger.error
in PydanticAIClient (see the except blocks that reference exc and the
conversation via self.conversation.pk around the logger.error calls) should use
logger.exception(...) so the stack trace is recorded; update those except
handlers (the blocks emitting the "LLM provider HTTP error..." message and the
similar block at lines ~383-387) to call logger.exception with the same
formatted message and parameters (including exc/context) instead of
logger.error.
In
`@src/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsx`:
- Around line 148-156: Add a new test in InputChat.test.tsx that covers the
provider-specific error branch by rendering <InputChat {...defaultProps}
status="error" errorType="provider" /> (or the non-'generic' value used in the
component) and assert that the textbox returned by getByRole('textbox', { name:
'Enter your message or a question' }) is disabled, the placeholder or visible
label shows the provider-specific message, and that the suggestion carousel
component (the carousel test id or role used in the suite) is not present (use
queryByTestId/queryByRole and expect(...).toBeNull()/not.toBeInTheDocument()).
Ensure you reuse defaultProps and AppWrapper like the existing tests and name
the spec to reflect "provider-specific error behavior".
In `@src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx`:
- Around line 247-260: The PROVIDER_ERROR_PLACEHOLDERS map and its use to
compute textareaPlaceholder in InputChat.tsx duplicate the mapping in
ChatError.tsx; extract that mapping into a shared exported constant (e.g.,
PROVIDER_ERROR_MESSAGES or getProviderErrorMessage) in the module that currently
owns the canonical copy (or a new shared utilities file), export it, and then
import and use it in InputChat.tsx by replacing the local
PROVIDER_ERROR_PLACEHOLDERS and the conditional that sets textareaPlaceholder so
both the banner (ChatError.tsx) and input placeholder (InputChat.tsx) use the
same source of truth.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43d36acb-7a05-49c4-9ed8-6a525fe4dfe3
📒 Files selected for processing (15)
CHANGELOG.mddocs/errors.mdsrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/tests/clients/pydantic_ai/test_stream_methods.pysrc/backend/chat/vercel_ai_sdk/encoder/encoder.pysrc/backend/conversations/settings.pysrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/frontend/apps/conversations/src/core/config/api/useConfig.tsxsrc/frontend/apps/conversations/src/features/chat/components/Chat.tsxsrc/frontend/apps/conversations/src/features/chat/components/ChatError.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChat.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ChatError.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/apps/conversations/src/features/chat/components/Chat.tsx (1)
222-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
chatErrorTypebefore early return to avoid stale provider state.If this branch returns early,
chatErrorTypemay keep a previous provider value, and the input can remain incorrectly disabled during a non-provider error flow.Proposed fix
if (error.message === 'attachment_summary_not_supported') { + setChatErrorType('generic'); setChatErrorModal({ title: t('Attachment summary not supported'), message: t('The summary feature is not supported yet.'), }); return; }🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/Chat.tsx` around lines 222 - 228, In the attachment-summary error branch inside Chat.tsx (the if (error.message === 'attachment_summary_not_supported') block), reset the chatErrorType state to null or its default via the same setter used elsewhere (chatErrorType / setChatErrorType) before calling setChatErrorModal and returning so the provider-related state is cleared; locate the chatErrorType state and its setter in this component and ensure you call setChatErrorType(null) (or the default) immediately prior to setChatErrorModal(...) to avoid leaving stale provider state that disables the input.
🧹 Nitpick comments (1)
src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py (1)
161-161: ⚡ Quick winRemove unreachable
yieldstatements in async mocks.At Line 161, Line 179, and Line 243, the post-
raiseyieldis dead code and triggers static-analysis noise. Prefer a small reusable async-iterator helper that raises on__anext__so these tests stay clean.Proposed cleanup
+class _RaisingAsyncIterator: + def __init__(self, exc: Exception): + self.exc = exc + + def __aiter__(self): + return self + + async def __anext__(self): + raise self.exc + @@ - async def mock_run_agent(*args, **kwargs): - raise ModelHTTPError(status_code=429, model_name="test-model") - yield # pylint: disable=unreachable - - with patch.object(service, "_run_agent", side_effect=mock_run_agent): + with patch.object( + service, + "_run_agent", + return_value=_RaisingAsyncIterator( + ModelHTTPError(status_code=429, model_name="test-model") + ), + ): results = [] async for result in service.stream_data_async([]): results.append(result)Also applies to: 179-179, 243-243
🤖 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 `@src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py` at line 161, Tests contain async mock generators with a post-raise "yield # pylint: disable=unreachable" that is dead code; remove those unreachable yields and replace repeated patterns with a small reusable async-iterator helper (e.g., an AsyncRaiseIterator that implements __aiter__ and an __anext__ that immediately raises the intended exception) and use that helper in the tests instead of the in-place generators; search for the literal "yield # pylint: disable=unreachable" occurrences in test_stream_methods.py and replace each generator/mock with the new AsyncRaiseIterator to eliminate static-analysis noise.
🤖 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 `@src/frontend/apps/conversations/src/core/config/api/useConfig.tsx`:
- Line 33: The config type currently declares STATUS_PAGE_URL?: string but the
backend can return null; update the type definition for STATUS_PAGE_URL in
useConfig (where the interface/type is declared) to allow null (e.g., string |
null or string | null | undefined if optional) so TypeScript reflects API
responses and handles null safely in code that reads STATUS_PAGE_URL.
---
Outside diff comments:
In `@src/frontend/apps/conversations/src/features/chat/components/Chat.tsx`:
- Around line 222-228: In the attachment-summary error branch inside Chat.tsx
(the if (error.message === 'attachment_summary_not_supported') block), reset the
chatErrorType state to null or its default via the same setter used elsewhere
(chatErrorType / setChatErrorType) before calling setChatErrorModal and
returning so the provider-related state is cleared; locate the chatErrorType
state and its setter in this component and ensure you call
setChatErrorType(null) (or the default) immediately prior to
setChatErrorModal(...) to avoid leaving stale provider state that disables the
input.
---
Nitpick comments:
In `@src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py`:
- Line 161: Tests contain async mock generators with a post-raise "yield #
pylint: disable=unreachable" that is dead code; remove those unreachable yields
and replace repeated patterns with a small reusable async-iterator helper (e.g.,
an AsyncRaiseIterator that implements __aiter__ and an __anext__ that
immediately raises the intended exception) and use that helper in the tests
instead of the in-place generators; search for the literal "yield # pylint:
disable=unreachable" occurrences in test_stream_methods.py and replace each
generator/mock with the new AsyncRaiseIterator to eliminate static-analysis
noise.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c08e09a-20ed-4fe0-aa9a-a538df0de95e
📒 Files selected for processing (9)
docs/errors.mdsrc/backend/chat/tests/clients/pydantic_ai/test_stream_methods.pysrc/frontend/apps/conversations/src/core/config/api/useConfig.tsxsrc/frontend/apps/conversations/src/features/chat/components/Chat.tsxsrc/frontend/apps/conversations/src/features/chat/components/ChatError.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChat.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ChatError.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsx
✅ Files skipped from review due to trivial changes (1)
- docs/errors.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/frontend/apps/conversations/src/features/chat/components/tests/InputChat.test.tsx
- src/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsx
- src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx
- src/frontend/apps/conversations/src/features/chat/components/tests/ChatError.test.tsx
- src/frontend/apps/conversations/src/features/chat/components/ChatError.tsx
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
from
cf91b3f to
b684879
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 379-381: The text-stream path in stream_text_async (using
encoder_fn / encode_text) currently constructs an events_v4.ErrorPart
(error_event) but if encoder_fn(error_event) returns None the code yields
nothing and silently swallows the provider exception; change the logic in the
error handling block around error_event so that if encoder_fn(error_event)
returns falsy (None) you re-raise the original exception (or wrap and raise it)
instead of returning/continuing—update the handling in
encode_text/stream_text_async to detect encoder_fn(error_event) == None and
raise the original provider error to preserve failure semantics for text
protocol consumers.
- Around line 373-378: Replace the two logger.error calls in
src/backend/chat/clients/pydantic_ai.py (the error handlers around the LLM
provider HTTP/other exception blocks that currently call logger.error with
exc/status_code and conversation info) with logger.exception so the stack trace
is recorded; keep the same message text and interpolation of
self.conversation.pk and status/exception details but use logger.exception (or
logger.error(..., exc_info=True)) in the except blocks to preserve full
traceback for debugging.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be6127dd-87ef-4f84-82eb-a6d17710f233
📒 Files selected for processing (17)
CHANGELOG.mddocs/errors.mdsrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/tests/clients/pydantic_ai/test_stream_methods.pysrc/backend/chat/vercel_ai_sdk/encoder/encoder.pysrc/backend/conversations/settings.pysrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/frontend/apps/conversations/src/core/config/api/useConfig.tsxsrc/frontend/apps/conversations/src/features/chat/components/Chat.tsxsrc/frontend/apps/conversations/src/features/chat/components/ChatError.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChat.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ChatError.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsxsrc/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/projects.spec.ts
✅ Files skipped from review due to trivial changes (4)
- CHANGELOG.md
- src/backend/chat/vercel_ai_sdk/encoder/encoder.py
- src/frontend/apps/conversations/src/core/config/api/useConfig.tsx
- docs/errors.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/frontend/apps/conversations/src/features/chat/components/tests/InputChat.test.tsx
- src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx
- src/backend/conversations/settings.py
- src/backend/core/api/viewsets.py
- src/backend/core/tests/test_api_config.py
- src/frontend/apps/conversations/src/features/chat/components/tests/ChatError.test.tsx
- src/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsx
- src/frontend/apps/conversations/src/features/chat/components/ChatError.tsx
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
from
b684879 to
8b3d564
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/frontend/apps/conversations/src/features/chat/components/Chat.tsx (2)
42-49: ⚡ Quick winConsider using a Set for O(1) membership checks.
The array is used with
.includes()for membership checks. Converting to a Set provides better semantics and O(1) lookup performance.♻️ Proposed refactor to use Set
-const PROVIDER_ERROR_CODES: ChatErrorType[] = [ +const PROVIDER_ERROR_CODES = new Set<ChatErrorType>([ 'model_unavailable', 'model_rate_limited', 'model_connection_error', 'model_not_found', 'model_wrong_type', 'model_busy', -]; +]);Then update line 230:
- if (PROVIDER_ERROR_CODES.includes(error.message as ChatErrorType)) { + if (PROVIDER_ERROR_CODES.has(error.message as ChatErrorType)) {🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/Chat.tsx` around lines 42 - 49, Replace the PROVIDER_ERROR_CODES array with an immutable Set (e.g., const PROVIDER_ERROR_CODES: ReadonlySet<ChatErrorType> = new Set([...])) and update all membership checks that currently use PROVIDER_ERROR_CODES.includes(...) to use PROVIDER_ERROR_CODES.has(...); ensure the Set is initialized with the same ChatErrorType values ('model_unavailable', 'model_rate_limited', etc.) and that any type annotations or exports referencing PROVIDER_ERROR_CODES reflect the Set type.
221-237: ⚡ Quick winRemove redundant type assertion.
Line 231's type assertion
as ChatErrorTypeis unnecessary because theincludes()check on line 230 already proves thaterror.messageis a validChatErrorTypewhen the condition is true.♻️ Simplified version
- if (PROVIDER_ERROR_CODES.includes(error.message as ChatErrorType)) { - setChatErrorType(error.message as ChatErrorType); + if (PROVIDER_ERROR_CODES.includes(error.message as ChatErrorType)) { + setChatErrorType(error.message); } else { setChatErrorType('generic'); }Note: If you adopt the Set refactor from the previous comment, TypeScript may require a type guard helper or keeping the assertion in the
has()call.🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/Chat.tsx` around lines 221 - 237, In onErrorChat, remove the redundant type assertion when calling setChatErrorType—since PROVIDER_ERROR_CODES.includes(error.message as ChatErrorType) already guarantees the value is a ChatErrorType, change setChatErrorType(error.message as ChatErrorType) to setChatErrorType(error.message); update the onErrorChat handler (references: onErrorChat, setChatErrorType, PROVIDER_ERROR_CODES, ChatErrorType) accordingly so TypeScript infers the correct type without the extra cast.src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py (1)
130-246: ⚡ Quick winAdd tests for the remaining HTTP mapping edge cases.
Line 185 in
src/backend/chat/clients/pydantic_ai.pymaps404tomodel_not_found, but this suite doesn’t assert that branch (or an unmapped 4xx re-raise). Adding both would fully lock the error-code contract.✅ Suggested test additions
+@pytest.mark.asyncio +async def test_stream_data_async_emits_model_not_found_on_404(): + conversation = await sync_to_async(ChatConversationFactory)() + service = AIAgentService(conversation, user=conversation.owner) + + def mock_run_agent(*args, **kwargs): + return AsyncRaiseIterator(ModelHTTPError(status_code=404, model_name="test-model")) + + with patch.object(service, "_run_agent", side_effect=mock_run_agent): + results = [] + async for result in service.stream_data_async([]): + results.append(result) + + assert results == ['3:"model_not_found"\n'] + + +@pytest.mark.asyncio +async def test_stream_data_async_reraises_unmapped_http_error(): + conversation = await sync_to_async(ChatConversationFactory)() + service = AIAgentService(conversation, user=conversation.owner) + + def mock_run_agent(*args, **kwargs): + return AsyncRaiseIterator(ModelHTTPError(status_code=401, model_name="test-model")) + + with patch.object(service, "_run_agent", side_effect=mock_run_agent): + with pytest.raises(ModelHTTPError): + async for _ in service.stream_data_async([]): + pass🤖 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 `@src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py` around lines 130 - 246, Add two tests for stream_data_async on AIAgentService: one that mocks _run_agent to raise ModelHTTPError(status_code=404, model_name="test-model") and asserts the streamed result equals ['3:"model_not_found"\n'], and another that mocks _run_agent to raise an unmapped 4xx (e.g., ModelHTTPError(status_code=400)) and asserts the error is re-raised (use pytest.raises around iterating service.stream_data_async([])). Locate the test module test_stream_methods.py and mirror the existing test patterns (use AsyncRaiseIterator, patch.object(service, "_run_agent", ...), and building conversation via ChatConversationFactory) so these new tests follow the same structure as test_stream_data_async_emits_model_busy_on_503 and the other cases.
🤖 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.
Nitpick comments:
In `@src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py`:
- Around line 130-246: Add two tests for stream_data_async on AIAgentService:
one that mocks _run_agent to raise ModelHTTPError(status_code=404,
model_name="test-model") and asserts the streamed result equals
['3:"model_not_found"\n'], and another that mocks _run_agent to raise an
unmapped 4xx (e.g., ModelHTTPError(status_code=400)) and asserts the error is
re-raised (use pytest.raises around iterating service.stream_data_async([])).
Locate the test module test_stream_methods.py and mirror the existing test
patterns (use AsyncRaiseIterator, patch.object(service, "_run_agent", ...), and
building conversation via ChatConversationFactory) so these new tests follow the
same structure as test_stream_data_async_emits_model_busy_on_503 and the other
cases.
In `@src/frontend/apps/conversations/src/features/chat/components/Chat.tsx`:
- Around line 42-49: Replace the PROVIDER_ERROR_CODES array with an immutable
Set (e.g., const PROVIDER_ERROR_CODES: ReadonlySet<ChatErrorType> = new
Set([...])) and update all membership checks that currently use
PROVIDER_ERROR_CODES.includes(...) to use PROVIDER_ERROR_CODES.has(...); ensure
the Set is initialized with the same ChatErrorType values ('model_unavailable',
'model_rate_limited', etc.) and that any type annotations or exports referencing
PROVIDER_ERROR_CODES reflect the Set type.
- Around line 221-237: In onErrorChat, remove the redundant type assertion when
calling setChatErrorType—since PROVIDER_ERROR_CODES.includes(error.message as
ChatErrorType) already guarantees the value is a ChatErrorType, change
setChatErrorType(error.message as ChatErrorType) to
setChatErrorType(error.message); update the onErrorChat handler (references:
onErrorChat, setChatErrorType, PROVIDER_ERROR_CODES, ChatErrorType) accordingly
so TypeScript infers the correct type without the extra cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98d0b1d9-26cc-4f5e-9a9b-8e9ebe14b72b
📒 Files selected for processing (17)
CHANGELOG.mddocs/errors.mdsrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/tests/clients/pydantic_ai/test_stream_methods.pysrc/backend/chat/vercel_ai_sdk/encoder/encoder.pysrc/backend/conversations/settings.pysrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/frontend/apps/conversations/src/core/config/api/useConfig.tsxsrc/frontend/apps/conversations/src/features/chat/components/Chat.tsxsrc/frontend/apps/conversations/src/features/chat/components/ChatError.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChat.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ChatError.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsxsrc/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/projects.spec.ts
✅ Files skipped from review due to trivial changes (3)
- src/backend/chat/vercel_ai_sdk/encoder/encoder.py
- CHANGELOG.md
- docs/errors.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsx
- src/frontend/apps/conversations/src/features/chat/components/tests/InputChat.test.tsx
- src/backend/core/api/viewsets.py
- src/backend/conversations/settings.py
- src/backend/core/tests/test_api_config.py
- src/frontend/apps/conversations/src/features/chat/components/tests/ChatError.test.tsx
- src/frontend/apps/conversations/src/features/chat/components/ChatError.tsx
- src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx
- src/frontend/apps/conversations/src/core/config/api/useConfig.tsx
Add AlbertAPI status page to settings Send event when http error is raised
Display specific message based on http error code Show link to albert api status page
maxenceh
force-pushed
the
maxenceh/error-model-down
branch
from
8b3d564 to
660a380
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.ts (1)
19-32: ⚡ Quick winConsider extracting the duplicated cleanup logic to a shared helper.
The
afterEachcleanup block (lines 19-32) is identical in bothleft-panel.spec.tsandprojects.spec.ts. Extracting this to a shared helper function inhelpers.tswould reduce duplication and centralize the cleanup pattern.♻️ Suggested extraction
In
helpers.ts:export async function cleanupTestProjects( request: APIRequestContext, browserName: string, ): Promise<void> { const response = await request.get('/api/v1.0/projects/', { params: { title: `${browserName}-`, page_size: 200 }, }); if (!response.ok()) return; const body = (await response.json()) as { results?: Array<{ id: string; title: string }>; }; const prefix = new RegExp(`^${browserName}-\\d+-\\d+-`); for (const project of body.results ?? []) { if (!prefix.test(project.title)) continue; await request.delete(`/api/v1.0/projects/${project.id}/`); } }Then in both test files:
test.afterEach(async ({ request, browserName }) => { await cleanupTestProjects(request, browserName); });🤖 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 `@src/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.ts` around lines 19 - 32, Extract the duplicated afterEach cleanup into a shared helper function (e.g., cleanupTestProjects) that accepts the APIRequestContext and browserName, moves the GET/JSON/parsing/regex/delete logic into that function, export it from helpers.ts, and replace the inline test.afterEach in left-panel.spec.ts (and projects.spec.ts) with a single call to cleanupTestProjects(request, browserName) to remove duplication and centralize cleanup logic.
🤖 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 `@src/backend/chat/clients/pydantic_ai.py`:
- Around line 367-371: The except block catching (ModelHTTPError,
HTTPValidationError, SDKError) assumes exc.status_code exists and can raise
AttributeError for SDKError/HTTPValidationError; update the handler in
pydantic_ai.py to guard access by using getattr(exc, "status_code", None) (or
branch by exception type) before calling _resolve_http_error_code so error_code
is computed only when a status_code is present, and ensure downstream logic
handles a None error_code without masking the original exception.
---
Nitpick comments:
In `@src/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.ts`:
- Around line 19-32: Extract the duplicated afterEach cleanup into a shared
helper function (e.g., cleanupTestProjects) that accepts the APIRequestContext
and browserName, moves the GET/JSON/parsing/regex/delete logic into that
function, export it from helpers.ts, and replace the inline test.afterEach in
left-panel.spec.ts (and projects.spec.ts) with a single call to
cleanupTestProjects(request, browserName) to remove duplication and centralize
cleanup logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 83e34e58-1f7a-421a-9207-b02160c96447
📒 Files selected for processing (17)
CHANGELOG.mddocs/errors.mdsrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/tests/clients/pydantic_ai/test_stream_methods.pysrc/backend/chat/vercel_ai_sdk/encoder/encoder.pysrc/backend/conversations/settings.pysrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/frontend/apps/conversations/src/core/config/api/useConfig.tsxsrc/frontend/apps/conversations/src/features/chat/components/Chat.tsxsrc/frontend/apps/conversations/src/features/chat/components/ChatError.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChat.tsxsrc/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/ChatError.test.tsxsrc/frontend/apps/conversations/src/features/chat/components/__tests__/InputChat.test.tsxsrc/frontend/apps/e2e/__tests__/app-conversations/left-panel.spec.tssrc/frontend/apps/e2e/__tests__/app-conversations/projects.spec.ts
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- src/backend/chat/vercel_ai_sdk/encoder/encoder.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/frontend/apps/conversations/src/features/chat/components/tests/InputChat.test.tsx
- src/backend/conversations/settings.py
- src/frontend/apps/conversations/src/features/chat/components/InputChatAction.tsx
- docs/errors.md
- src/backend/core/api/viewsets.py
- src/frontend/apps/conversations/src/features/chat/components/Chat.tsx
- src/frontend/apps/conversations/src/features/chat/components/tests/ChatError.test.tsx
- src/frontend/apps/conversations/src/core/config/api/useConfig.tsx
- src/backend/core/tests/test_api_config.py
- src/frontend/apps/conversations/src/features/chat/components/InputChat.tsx
- src/frontend/apps/conversations/src/features/chat/components/ChatError.tsx
- src/backend/chat/tests/clients/pydantic_ai/test_stream_methods.py
Comment on lines
+367
to
+371
| except (ModelHTTPError, HTTPValidationError, SDKError) as exc: | ||
| # HTTPValidationError and SDKError are mistral-specific exceptions not | ||
| # wrapped by pydantic_ai into ModelHTTPError. | ||
| error_code = _resolve_http_error_code(exc.status_code) | ||
| if error_code is None: |
There was a problem hiding this comment.
Guard status_code access for SDKError/HTTPValidationError.
Line 370 assumes every caught exception has status_code. If SDKError lacks it, this handler raises AttributeError and masks the original provider failure path.
💡 Minimal hardening patch
- except (ModelHTTPError, HTTPValidationError, SDKError) as exc:
+ except (ModelHTTPError, HTTPValidationError, SDKError) as exc:
# HTTPValidationError and SDKError are mistral-specific exceptions not
# wrapped by pydantic_ai into ModelHTTPError.
- error_code = _resolve_http_error_code(exc.status_code)
+ status_code = getattr(exc, "status_code", None)
+ if status_code is None:
+ error_code = "model_connection_error"
+ else:
+ error_code = _resolve_http_error_code(status_code)
if error_code is None:
raise
logger.exception(
"LLM provider HTTP error (status=%s) for conversation %s: %s",
- exc.status_code,
+ status_code,
self.conversation.pk,
exc,
)In the current mistralai Python SDK, do `SDKError` and `HTTPValidationError` always expose a `status_code` attribute? Please provide the exception class definitions or official docs link.
🤖 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 `@src/backend/chat/clients/pydantic_ai.py` around lines 367 - 371, The except
block catching (ModelHTTPError, HTTPValidationError, SDKError) assumes
exc.status_code exists and can raise AttributeError for
SDKError/HTTPValidationError; update the handler in pydantic_ai.py to guard
access by using getattr(exc, "status_code", None) (or branch by exception type)
before calling _resolve_http_error_code so error_code is computed only when a
status_code is present, and ensure downstream logic handles a None error_code
without masking the original exception.
Collaborator
Author
There was a problem hiding this comment.
SDKError and HTTPValidationError inherit from MistralError
@dataclass(unsafe_hash=True)
class MistralError(Exception):
"""The base class for all HTTP error responses."""
message: str
status_code: int
body: str
headers: httpx.Headers = field(hash=False)
raw_response: httpx.Response = field(hash=False)
def __init__(
self, message: str, raw_response: httpx.Response, body: Optional[str] = None
):
object.__setattr__(self, "message", message)
object.__setattr__(self, "status_code", raw_response.status_code)
object.__setattr__(
self, "body", body if body is not None else raw_response.text
)
object.__setattr__(self, "headers", raw_response.headers)
object.__setattr__(self, "raw_response", raw_response)
def __str__(self):
return self.message
There was a problem hiding this comment.
@maxenceh, thanks for sharing the MistralError base class definition. Since status_code is set unconditionally from raw_response.status_code in __init__, it is always present on both SDKError and HTTPValidationError. My concern doesn't apply here — the current direct access to exc.status_code is safe.
(ᵕ—ᴗ—) 🐇
🧠 Learnings used
Learnt from: maxenceh
Repo: suitenumerique/conversations PR: 413
File: src/backend/core/models.py:197-207
Timestamp: 2026-04-16T09:18:10.063Z
Learning: In Django projects that use `django-solo` and `SingletonModelAdmin`, do not redundantly set `verbose_name_plural` on a `SingletonModel` subclass just to fix admin display. `SingletonModelAdmin` already overrides the admin rendering for the app index/breadcrumbs to use the singular `verbose_name` (and it suppresses the "+ Add" button), so `verbose_name_plural` is unnecessary for correct `django-solo` admin behavior. Only add `verbose_name_plural` if you have a non-admin use case that requires a plural label.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
When an LLM provider is temporarily down, rate-limited, or unreachable, users currently see a generic error with no context. This PR surfaces specific, actionable error messages so users understand what happened and where to check for status updates.
Proposal
Backend
Frontend
UX
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests