fix(ai): contain provider setup errors#2411
Open
aqilaziz wants to merge 8 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRemoves a separator in the AI settings panel, updates tests to make ChangesAI settings UI & tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/components/settings/panels/AIPanel.tsx (2)
205-225: ⚡ Quick winInternationalize the fallback error message.
The fallback message on line 207 is hardcoded in English. For consistency with the rest of the codebase (which uses the
t()function for i18n), this should be internationalized.Suggested change
function formatProviderSetupError(err: unknown): ProviderSetupError { const raw = err instanceof Error ? err.message : String(err ?? ''); - const fallback = 'Provider setup failed. Check the provider settings and try again.'; + const fallback = t('settings.ai.providerSetupFallbackError'); if (!raw.trim()) return { summary: fallback };You'll need to add the translation key to your i18n resources and accept
tas a parameter or use theuseT()hook if this function is called within a component context. Alternatively, keep the function pure and handle the fallback message at the call sites.🤖 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 `@app/src/components/settings/panels/AIPanel.tsx` around lines 205 - 225, formatProviderSetupError currently uses a hardcoded English fallback string; update it to use i18n by either (A) adding a translation key (e.g. "provider.setup_failed") to your i18n resources and changing formatProviderSetupError to accept a t function parameter (e.g. formatProviderSetupError(err: unknown, t: TFunction)) and call t('provider.setup_failed') instead of the literal, or (B) keep formatProviderSetupError pure and remove the fallback here, returning undefined/detail only and let callers (components using useT()) supply the localized fallback; ensure ProviderSetupError consumers are updated accordingly to use t() or useT() and add the new translation key to the locale files.
673-691: ⚡ Quick winInternationalize the "Details" label.
The "Details" label on line 682 is hardcoded in English. This should use the
t()function for internationalization, consistent with other user-facing strings in the component (e.g., line 619 usest('settings.ai.connectProvider')).Suggested change
-function ProviderSetupErrorMessage({ error }: { error: ProviderSetupError }) { +function ProviderSetupErrorMessage({ error }: { error: ProviderSetupError }) { + const { t } = useT(); return ( <div className="min-w-0 rounded-md border border-red-200 dark:border-red-500/30 bg-red-50 dark:bg-red-500/10 px-3 py-2 text-xs text-red-700 dark:text-red-300"> <p role="alert" className="font-medium leading-relaxed break-words [overflow-wrap:anywhere]"> {error.summary} </p> {error.detail && ( <details className="mt-2"> - <summary className="cursor-pointer select-none text-[11px] font-medium text-red-700 dark:text-red-200"> - Details - </summary> + <summary className="cursor-pointer select-none text-[11px] font-medium text-red-700 dark:text-red-200"> + {t('settings.ai.errorDetails')} + </summary>🤖 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 `@app/src/components/settings/panels/AIPanel.tsx` around lines 673 - 691, ProviderSetupErrorMessage currently hardcodes the "Details" summary text; replace that literal with a call to the i18n function (t) to match the rest of the component. In the ProviderSetupErrorMessage component, change the <summary> content from "Details" to t('<appropriate.key>') (e.g., t('settings.ai.details') or whatever existing localization key you prefer), import/use the same t function/context as the parent component so the label is internationalized consistently.app/src/components/settings/panels/__tests__/AIPanel.test.tsx (1)
344-366: ⚡ Quick winConsider explicitly testing the expand/collapse behavior.
The test verifies that the friendly message appears in the alert and the raw error text exists in the dialog, which covers the core functionality. However, line 365 searches the entire dialog for the raw error text without explicitly verifying it's hidden before the user expands the "Details" section.
To more thoroughly test the "raw detail behind details" behavior described in the test name, consider:
- Asserting the raw text is not visible before expanding (e.g., checking the
<details>element is closed or using a visibility query)- Clicking the "Details" button/control to expand it
- Then asserting the raw error text is visible
This would more accurately verify the hide/show interaction rather than just checking that the content exists in the DOM.
✨ Example enhancement
const alert = await within(dialog).findByRole('alert'); expect(alert).toHaveTextContent(/Could not reach OpenAI\./i); expect(alert).not.toHaveTextContent('invalid_api_key'); - expect(within(dialog).getByText('Details')).toBeInTheDocument(); - expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).toBeInTheDocument(); + + // Verify raw error is not visible initially (hidden behind details) + expect(screen.queryByText(/invalid_api_key_invalid_api_key/)).not.toBeVisible(); + + // Expand the details section + const detailsToggle = within(dialog).getByText('Details'); + fireEvent.click(detailsToggle); + + // Now raw error should be visible + await waitFor(() => + expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).toBeVisible() + );Note: Adjust based on how
ProviderSetupErrorMessageimplements the details disclosure (e.g.,<details>/<summary>vs. custom toggle).🤖 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 `@app/src/components/settings/panels/__tests__/AIPanel.test.tsx` around lines 344 - 366, Update the test in AIPanel.test.tsx to explicitly verify the expand/collapse behavior: after confirming the friendly alert text, assert the raw error text is not visible (e.g., query the <details> disclosure or use a visibility assertion on the raw string within(dialog).queryByText(/invalid_api_key_invalid_api_key/)). Then simulate the user expanding the Details control (click the "Details" summary/button found via within(dialog).getByText('Details') or getByRole) and finally assert the raw error text is visible; reference the AIPanel render and the ProviderSetupErrorMessage/Details control used in the dialog when updating the assertions.
🤖 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 `@app/src/components/settings/panels/__tests__/AIPanel.test.tsx`:
- Around line 344-366: Update the test in AIPanel.test.tsx to explicitly verify
the expand/collapse behavior: after confirming the friendly alert text, assert
the raw error text is not visible (e.g., query the <details> disclosure or use a
visibility assertion on the raw string
within(dialog).queryByText(/invalid_api_key_invalid_api_key/)). Then simulate
the user expanding the Details control (click the "Details" summary/button found
via within(dialog).getByText('Details') or getByRole) and finally assert the raw
error text is visible; reference the AIPanel render and the
ProviderSetupErrorMessage/Details control used in the dialog when updating the
assertions.
In `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 205-225: formatProviderSetupError currently uses a hardcoded
English fallback string; update it to use i18n by either (A) adding a
translation key (e.g. "provider.setup_failed") to your i18n resources and
changing formatProviderSetupError to accept a t function parameter (e.g.
formatProviderSetupError(err: unknown, t: TFunction)) and call
t('provider.setup_failed') instead of the literal, or (B) keep
formatProviderSetupError pure and remove the fallback here, returning
undefined/detail only and let callers (components using useT()) supply the
localized fallback; ensure ProviderSetupError consumers are updated accordingly
to use t() or useT() and add the new translation key to the locale files.
- Around line 673-691: ProviderSetupErrorMessage currently hardcodes the
"Details" summary text; replace that literal with a call to the i18n function
(t) to match the rest of the component. In the ProviderSetupErrorMessage
component, change the <summary> content from "Details" to t('<appropriate.key>')
(e.g., t('settings.ai.details') or whatever existing localization key you
prefer), import/use the same t function/context as the parent component so the
label is internationalized consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5a30f5c-cc4f-4577-926b-52ccced84f1d
📒 Files selected for processing (2)
app/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/__tests__/AIPanel.test.tsx
aqilaziz
force-pushed
the
codex/2363-ai-provider-error-wrap
branch
from
2c3bbd2 to
03194b2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/components/settings/panels/__tests__/AIPanel.test.tsx (1)
344-366: ⚡ Quick winStrengthen this test to verify disclosure behavior, not just DOM presence.
This currently checks detail text exists without expanding “Details”, so it doesn’t prove the raw payload is hidden until user action.
✅ Suggested assertion shape
const alert = await within(dialog).findByRole('alert'); expect(alert).toHaveTextContent(/Could not reach OpenAI\./i); expect(alert).not.toHaveTextContent('invalid_api_key'); - expect(within(dialog).getByText('Details')).toBeInTheDocument(); - expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).toBeInTheDocument(); + const detailsSummary = within(dialog).getByText('Details'); + const details = detailsSummary.closest('details') as HTMLDetailsElement; + expect(details.open).toBe(false); + fireEvent.click(detailsSummary); + expect(details.open).toBe(true); + expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).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 `@app/src/components/settings/panels/__tests__/AIPanel.test.tsx` around lines 344 - 366, The test currently only checks that the details text exists in the DOM but doesn't verify that it's initially hidden and only revealed after user action; update the 'keeps long provider setup errors contained with raw detail behind details' test to assert that the raw payload string (e.g., 'invalid_api_key_invalid_api_key') is not visible inside the alert element initially (use within(dialog).findByRole('alert') and expect(...).not.toHaveTextContent(...)), then simulate expanding the Details control (fireEvent.click(on the element returned by within(dialog).getByText('Details') or getByRole if available) and assert afterwards that the raw payload is visible (expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).toBeInTheDocument()). Ensure you still keep the existing checks for the generic error message and that Details exists.
🤖 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 `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 223-224: The current code sets detail to the full raw provider
payload which can bloat React state; modify the assignment around const detail =
raw.trim() !== summary ? raw.trim() : undefined to enforce a maximum length
(e.g., const MAX_DETAIL_LENGTH = 1000) and if raw.trim() exceeds that limit,
truncate it (append an ellipsis) before setting detail, while preserving the
existing equality check with summary so detail remains undefined when identical
to summary; update any references that expect full raw to use the truncated
value or expose the full payload only on demand (e.g., via "view full error"
action).
---
Nitpick comments:
In `@app/src/components/settings/panels/__tests__/AIPanel.test.tsx`:
- Around line 344-366: The test currently only checks that the details text
exists in the DOM but doesn't verify that it's initially hidden and only
revealed after user action; update the 'keeps long provider setup errors
contained with raw detail behind details' test to assert that the raw payload
string (e.g., 'invalid_api_key_invalid_api_key') is not visible inside the alert
element initially (use within(dialog).findByRole('alert') and
expect(...).not.toHaveTextContent(...)), then simulate expanding the Details
control (fireEvent.click(on the element returned by
within(dialog).getByText('Details') or getByRole if available) and assert
afterwards that the raw payload is visible
(expect(within(dialog).getByText(/invalid_api_key_invalid_api_key/)).toBeInTheDocument()).
Ensure you still keep the existing checks for the generic error message and that
Details exists.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36ad4671-621f-499f-9353-44fd38a2ace1
📒 Files selected for processing (2)
app/src/components/settings/panels/AIPanel.tsxapp/src/components/settings/panels/__tests__/AIPanel.test.tsx
aqilaziz
force-pushed
the
codex/2363-ai-provider-error-wrap
branch
from
03194b2 to
266a570
Compare
# Conflicts: # app/src/components/settings/panels/AIPanel.tsx # app/src/components/settings/panels/__tests__/AIPanel.test.tsx
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
2 participants
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.
Summary
Problem
Provider setup failures can include long URLs or JSON-like API payloads that overflow the LLM settings modal.
Solution
Submission Checklist
Required
pnpm formator an equivalent focused formatter checkpnpm lintor verified the touched code path with focused checksCoverage
Release / Manual Validation
Linear Metadata
Validation
prettier --check src/components/settings/panels/AIPanel.tsx src/components/settings/panels/__tests__/AIPanel.test.tsxpnpm test:unit src/components/settings/panels/__tests__/AIPanel.test.tsxgit diff --checkRelated
Closes #2363
AI Authored PR Metadata
Summary by CodeRabbit
Style
Tests
Documentation