refactor(platform): convert provider dialogs to sidepanels#1410
Merged
Conversation
Replace FormDialog-based provider add/edit modals with Sheet-based sidepanels matching the integration panel pattern. Extract inline setup guide markup into a reusable CollapsibleGuide component. Add edit-form validation and clarify read-only provider name field.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR refactors the provider management UI by replacing dialog-based components (ProviderAddDialog, ProviderEditDialog) with new sheet-based panel components (ProviderAddPanel, ProviderEditPanel). Additionally, a new reusable CollapsibleGuide component is introduced to standardize the rendering of collapsible markdown content, which replaces inline details implementations in the integration credentials form. Supporting changes include reformatting the ai-image configuration guide text and adding localization strings for provider name read-only help text and BYOM guidance section titles. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/data-display/collapsible-guide.tsx`:
- Around line 22-25: The <details> is currently controlled via
open={defaultOpen}, which forces its state on every parent render; remove the
controlled prop and instead set the initial open state only on mount (e.g., use
a ref for the <details> element and in useEffect set detailsRef.current.open =
defaultOpen once). Update the component in collapsible-guide.tsx to stop passing
open={defaultOpen} and implement the useRef/useEffect initialization so user
toggles remain uncontrolled after mount.
In
`@services/platform/app/features/settings/providers/components/provider-add-panel.tsx`:
- Around line 289-293: The error message Text elements (e.g., the one rendering
errors.models?.root?.message using <Text variant="caption"
className="text-destructive text-sm">) need an accessibility attribute; add
role="alert" to these Text components so screen readers announce the error
immediately; apply the same change to the other error display (the Text
rendering the model-level error at the second location around lines 358-365) to
ensure both error messages include role="alert".
- Line 45: The component is accepting an organizationId prop (_organizationId)
but the calls to saveProvider and saveProviderSecret use the hardcoded string
'default'; either use the passed prop or remove it from the props/interface. Fix
option A: replace the hardcoded 'default' in the saveProvider and
saveProviderSecret calls with the received _organizationId (or a safe fallback
like _organizationId ?? 'default') so ProviderAddPanel actually uses the prop.
Fix option B: if multi-org is not needed, remove organizationId/_organizationId
from the component props/interface and any related tests/usage so it isn't
unused. Update references around the saveProvider and saveProviderSecret
invocations accordingly.
- Around line 70-74: The baseUrl zod schema in provider-add-panel.tsx currently
passes the "required" translation to .url(), causing invalid-URL errors to show
as "required"; change the chain to enforce an explicit empty-string/required
message and a separate URL-format message: call
.nonempty(tCommon('validation.required', { field: t('providers.baseUrl') })) (or
.min(1, ...)) and then .url(tCommon('validation.url', { field:
t('providers.baseUrl') })) on the baseUrl field so empty and malformed URLs
produce the correct localized messages.
- Around line 162-195: The current onSubmit flow calls saveProvider then
saveProviderSecret and treats any error the same, causing a silent partial
success if the secret save fails; update onSubmit to detect saveProviderSecret
failures and handle them explicitly: after calling saveProvider and if
saveProviderSecret throws, either call a cleanup API (e.g. deleteProvider with
the same orgSlug and providerName) to roll back the created provider or surface
a distinct user message that the provider was created but the API key was not
saved and they must add it via the edit flow; implement this by wrapping
saveProviderSecret in its own try/catch inside onSubmit and use
deleteProvider(providerName) or a descriptive toast (instead of the generic
createFailed toast), while keeping reset() and onOpenChange(false) only on full
success.
In
`@services/platform/app/features/settings/providers/components/provider-edit-panel.tsx`:
- Around line 73-74: The current isValid computation only checks non-empty
strings (const isValid) so invalid strings like "not-a-url" enable the save
button; update the client-side validation to verify form.baseUrl is a
well-formed URL (e.g., attempt new URL(form.baseUrl) or use a lightweight regex)
and incorporate that into isValid. Also surface a user-facing validation error
for baseUrl in the provider edit form (the same place that reads form.baseUrl
and sets form.displayName) so the Save button stays disabled when the URL parse
fails and the user sees feedback before submission. Ensure references to isValid
and form.baseUrl (in provider-edit-panel.tsx) are updated accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 733c8189-359c-48b5-9eea-1f5b0cbe3c6e
📒 Files selected for processing (10)
examples/integrations/ai-image/config.jsonservices/platform/app/components/ui/data-display/collapsible-guide.tsxservices/platform/app/features/settings/integrations/components/integration-manage/integration-credentials-form.tsxservices/platform/app/features/settings/providers/components/provider-add-dialog.tsxservices/platform/app/features/settings/providers/components/provider-add-panel.tsxservices/platform/app/features/settings/providers/components/provider-edit-dialog.tsxservices/platform/app/features/settings/providers/components/provider-edit-panel.tsxservices/platform/app/features/settings/providers/components/providers-table.tsxservices/platform/messages/de.jsonservices/platform/messages/en.json
💤 Files with no reviewable changes (2)
- services/platform/app/features/settings/providers/components/provider-add-dialog.tsx
- services/platform/app/features/settings/providers/components/provider-edit-dialog.tsx
Comment on lines
+22
to
+25
| <details | ||
| className="bg-muted/30 border-border rounded-lg border" | ||
| open={defaultOpen} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n services/platform/app/components/ui/data-display/collapsible-guide.tsxRepository: tale-project/tale
Length of output: 1407
🏁 Script executed:
rg -l "CollapsibleGuide" --type tsx --type tsRepository: tale-project/tale
Length of output: 88
🏁 Script executed:
rg "CollapsibleGuide" --type ts --type jsRepository: tale-project/tale
Length of output: 1000
🏁 Script executed:
rg -A 5 "CollapsibleGuide" services/platform/app/features/settings/providers/components/provider-add-panel.tsx | head -20Repository: tale-project/tale
Length of output: 640
🏁 Script executed:
rg -A 5 "CollapsibleGuide" services/platform/app/features/settings/integrations/components/integration-manage/integration-credentials-form.tsx | head -20Repository: tale-project/tale
Length of output: 606
defaultOpen is wired as a controlled open prop, causing user-toggled state to reset on parent rerender.
When a parent component rerenders, React will update the <details> element's open attribute to the defaultOpen value, overriding any user interactions (like closing the disclosure). This breaks the expected "default-only" behavior.
Proposed fix
import { Info } from 'lucide-react';
+import { useState } from 'react';
import ReactMarkdown from 'react-markdown';
import remarkGfm from 'remark-gfm';
@@
export function CollapsibleGuide({
label,
content,
defaultOpen,
}: CollapsibleGuideProps) {
+ const [isOpen, setIsOpen] = useState(Boolean(defaultOpen));
+
return (
<details
className="bg-muted/30 border-border rounded-lg border"
- open={defaultOpen}
+ open={isOpen}
+ onToggle={(event) => setIsOpen(event.currentTarget.open)}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/data-display/collapsible-guide.tsx`
around lines 22 - 25, The <details> is currently controlled via
open={defaultOpen}, which forces its state on every parent render; remove the
controlled prop and instead set the initial open state only on mount (e.g., use
a ref for the <details> element and in useEffect set detailsRef.current.open =
defaultOpen once). Update the component in collapsible-guide.tsx to stop passing
open={defaultOpen} and implement the useRef/useEffect initialization so user
toggles remain uncontrolled after mount.
| export function ProviderAddPanel({ | ||
| open, | ||
| onOpenChange, | ||
| organizationId: _organizationId, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused organizationId prop with hardcoded fallback.
The organizationId prop is received but not used—'default' is hardcoded in both saveProvider and saveProviderSecret calls (lines 165 and 178). If multi-org support is planned, consider using the prop; otherwise, remove it from the interface to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/providers/components/provider-add-panel.tsx`
at line 45, The component is accepting an organizationId prop (_organizationId)
but the calls to saveProvider and saveProviderSecret use the hardcoded string
'default'; either use the passed prop or remove it from the props/interface. Fix
option A: replace the hardcoded 'default' in the saveProvider and
saveProviderSecret calls with the received _organizationId (or a safe fallback
like _organizationId ?? 'default') so ProviderAddPanel actually uses the prop.
Fix option B: if multi-org is not needed, remove organizationId/_organizationId
from the component props/interface and any related tests/usage so it isn't
unused. Update references around the saveProvider and saveProviderSecret
invocations accordingly.
Comment on lines
+70
to
+74
| baseUrl: z.string().url( | ||
| tCommon('validation.required', { | ||
| field: t('providers.baseUrl'), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
URL validation uses incorrect error message.
The baseUrl field uses .url() validation but displays a "required" error message. When users enter an invalid URL format, they'll see "Base URL is required" instead of a URL format error, which is misleading.
🔧 Proposed fix
baseUrl: z.string().url(
- tCommon('validation.required', {
- field: t('providers.baseUrl'),
- }),
+ t('providers.invalidBaseUrl'),
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/providers/components/provider-add-panel.tsx`
around lines 70 - 74, The baseUrl zod schema in provider-add-panel.tsx currently
passes the "required" translation to .url(), causing invalid-URL errors to show
as "required"; change the chain to enforce an explicit empty-string/required
message and a separate URL-format message: call
.nonempty(tCommon('validation.required', { field: t('providers.baseUrl') })) (or
.min(1, ...)) and then .url(tCommon('validation.url', { field:
t('providers.baseUrl') })) on the baseUrl field so empty and malformed URLs
produce the correct localized messages.
Comment on lines
+162
to
+195
| const onSubmit = async (data: FormData) => { | ||
| try { | ||
| await saveProvider({ | ||
| orgSlug: 'default', | ||
| providerName: data.name, | ||
| config: { | ||
| displayName: data.displayName, | ||
| baseUrl: data.baseUrl, | ||
| models: data.models.map((m) => ({ | ||
| id: m.id, | ||
| displayName: m.displayName, | ||
| tags: m.tags, | ||
| })), | ||
| }, | ||
| }); | ||
| await saveProviderSecret({ | ||
| orgSlug: 'default', | ||
| providerName: data.name, | ||
| apiKey: data.apiKey, | ||
| }); | ||
| toast({ | ||
| title: t('providers.created'), | ||
| variant: 'success', | ||
| }); | ||
| reset(); | ||
| onOpenChange(false); | ||
| } catch (error) { | ||
| console.error(error); | ||
| toast({ | ||
| title: t('providers.createFailed'), | ||
| variant: 'destructive', | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Partial failure risk when saving provider secret.
If saveProviderSecret fails after saveProvider succeeds (lines 164-181), the provider exists without an API key. The user sees the "createFailed" toast but the provider was actually created. Consider either:
- Informing the user that the provider was created but the key wasn't saved, so they can add it via the edit flow
- Attempting to clean up the provider on secret save failure (if a delete API exists)
💡 Suggested improvement for clearer error messaging
} catch (error) {
console.error(error);
toast({
- title: t('providers.createFailed'),
+ title: t('providers.secretSaveFailed'),
+ description: t('providers.providerCreatedButKeyFailed'), // add this key
variant: 'destructive',
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/providers/components/provider-add-panel.tsx`
around lines 162 - 195, The current onSubmit flow calls saveProvider then
saveProviderSecret and treats any error the same, causing a silent partial
success if the secret save fails; update onSubmit to detect saveProviderSecret
failures and handle them explicitly: after calling saveProvider and if
saveProviderSecret throws, either call a cleanup API (e.g. deleteProvider with
the same orgSlug and providerName) to roll back the created provider or surface
a distinct user message that the provider was created but the API key was not
saved and they must add it via the edit flow; implement this by wrapping
saveProviderSecret in its own try/catch inside onSubmit and use
deleteProvider(providerName) or a descriptive toast (instead of the generic
createFailed toast), while keeping reset() and onOpenChange(false) only on full
success.
Comment on lines
+289
to
+293
| {errors.models?.root?.message && ( | ||
| <Text variant="caption" className="text-destructive text-sm"> | ||
| {errors.models.root.message} | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
Add role="alert" to error messages for screen reader announcements.
Per accessibility guidelines, error messages should use role="alert" so screen readers announce them immediately when they appear. This applies here and to the similar error display at lines 358-365.
♿ Proposed fix
{errors.models?.root?.message && (
- <Text variant="caption" className="text-destructive text-sm">
+ <Text variant="caption" className="text-destructive text-sm" role="alert">
{errors.models.root.message}
</Text>
)}As per coding guidelines: "USE role='alert' on error messages."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {errors.models?.root?.message && ( | |
| <Text variant="caption" className="text-destructive text-sm"> | |
| {errors.models.root.message} | |
| </Text> | |
| )} | |
| {errors.models?.root?.message && ( | |
| <Text variant="caption" className="text-destructive text-sm" role="alert"> | |
| {errors.models.root.message} | |
| </Text> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/providers/components/provider-add-panel.tsx`
around lines 289 - 293, The error message Text elements (e.g., the one rendering
errors.models?.root?.message using <Text variant="caption"
className="text-destructive text-sm">) need an accessibility attribute; add
role="alert" to these Text components so screen readers announce the error
immediately; apply the same change to the other error display (the Text
rendering the model-level error at the second location around lines 358-365) to
ensure both error messages include role="alert".
Comment on lines
+73
to
+74
| const isValid = | ||
| form.displayName.trim().length > 0 && form.baseUrl.trim().length > 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating URL format client-side.
The isValid check only verifies non-empty values, but baseUrl must be a valid URL (per the server-side schema). Users can enter invalid URLs like "not-a-url", the save button enables, but submission fails server-side. Adding a simple URL format check would provide earlier feedback.
💡 Suggested enhancement
+ const isValidUrl = (url: string) => {
+ try {
+ new URL(url);
+ return true;
+ } catch {
+ return false;
+ }
+ };
+
const isValid =
- form.displayName.trim().length > 0 && form.baseUrl.trim().length > 0;
+ form.displayName.trim().length > 0 &&
+ form.baseUrl.trim().length > 0 &&
+ isValidUrl(form.baseUrl.trim());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isValid = | |
| form.displayName.trim().length > 0 && form.baseUrl.trim().length > 0; | |
| const isValidUrl = (url: string) => { | |
| try { | |
| new URL(url); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }; | |
| const isValid = | |
| form.displayName.trim().length > 0 && | |
| form.baseUrl.trim().length > 0 && | |
| isValidUrl(form.baseUrl.trim()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/platform/app/features/settings/providers/components/provider-edit-panel.tsx`
around lines 73 - 74, The current isValid computation only checks non-empty
strings (const isValid) so invalid strings like "not-a-url" enable the save
button; update the client-side validation to verify form.baseUrl is a
well-formed URL (e.g., attempt new URL(form.baseUrl) or use a lightweight regex)
and incorporate that into isValid. Also surface a user-facing validation error
for baseUrl in the provider edit form (the same place that reads form.baseUrl
and sets form.displayName) so the Save button stays disabled when the URL parse
fails and the user sees feedback before submission. Ensure references to isValid
and form.baseUrl (in provider-edit-panel.tsx) are updated accordingly.
Use controlled state for CollapsibleGuide to preserve user toggle across rerenders. Add role="alert" to form error messages for screen reader announcements. Fix base URL validation to show proper URL format error instead of "required". Add URL format validation to edit panel.
This was referenced Apr 13, 2026
8 tasks
7 tasks
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.
Summary
FormDialog-based provider add/edit modals withSheet-based sidepanels matching the integration panel pattern (header bar, scrollable content, sticky footer)integration-credentials-forminto a reusableCollapsibleGuidecomponent withdefaultOpenprop### Configuration Guideheading from AI Image integration'ssetupGuidecontentCloses #1387 & #1382
Test plan
CollapsibleGuiderenders correctlySummary by CodeRabbit
New Features
Improvements
Localization