refactor(platform): provider drawer — headers outside Card, ghost buttons, collapsible options guide#1732
Open
Israeltheminer wants to merge 2 commits into
Open
refactor(platform): provider drawer — headers outside Card, ghost buttons, collapsible options guide#1732Israeltheminer wants to merge 2 commits into
Israeltheminer wants to merge 2 commits into
Conversation
…tons, collapsible options guide - Move section headers (General, API Key, Default Models, Provider Options, Models) outside the Card so each section is a header + content pair instead of an enclosed Card with internal `SectionHeader`. Drops the bespoke `SectionHeader` helper. - Switch inline Edit/Test/Add Model/Fetch from Provider actions to ghost `Button` for visual consistency. - Tighten InfoRow padding (px-5 py-3.5 → px-4 py-2.5) and align="start" so multi-line values wrap cleanly; rebuild the skeleton to mirror the new layout. - Render the long provider-options copy through `CollapsibleGuide` with markdown — bold headings, bullet lists, fenced ```json blocks — and add `providers.providerOptions.guideLabel` in en/de/fr. Update `CollapsibleGuide` to style `<pre>` blocks. - Fix row-level Edit on the providers table: wrap `ProviderEditPanel` in a `ProviderEditPanelLoader` that fetches config + hash and supplies `ProviderConfigProvider`, otherwise `useProviderConfig` throws. - Minor: `Card` radius `rounded-xl` → `rounded-lg`; tabs list `min-w-full justify-around` so the underline spans the container; `provider-add-panel` Models header matches drawer styling.
📝 WalkthroughWalkthroughThis PR refactors the provider options documentation system to use collapsible guides with markdown formatting, removes a shared SectionHeader helper from ProviderDetailDrawer in favor of per-section header management, adds localization support with improved help text across English, German, and French, introduces a ProviderEditPanelLoader to provide configuration context during provider editing, and applies minor styling updates to several UI components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/app/features/settings/providers/components/provider-options-editor.tsx (1)
9-23: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep imports contiguous at the top of the module.
providerOptionsClientSchemacurrently splits two import groups. Move all imports above declarations to match repository conventions.Proposed fix
import { JsonInput } from '`@/app/components/ui/forms/json-input`'; import { Textarea } from '`@/app/components/ui/forms/textarea`'; +import { Card } from '`@/app/components/ui/layout/card`'; +import { HStack, Stack } from '`@/app/components/ui/layout/layout`'; +import { Sheet } from '`@/app/components/ui/overlays/sheet`'; +import { Text } from '`@/app/components/ui/typography/text`'; +import { toast } from '`@/app/hooks/use-toast`'; +import { useT } from '`@/lib/i18n/client`'; // Permissive client-side schema: server-side `providerJsonSchema.parse` is the // authoritative gate (it carries the deny-list). This schema only catches // "is JSON, is an object" so we don't ship obviously malformed input. const providerOptionsClientSchema = z.record(z.string(), z.unknown()); -import { Card } from '`@/app/components/ui/layout/card`'; -import { HStack, Stack } from '`@/app/components/ui/layout/layout`'; -import { Sheet } from '`@/app/components/ui/overlays/sheet`'; -import { Text } from '`@/app/components/ui/typography/text`'; -import { toast } from '`@/app/hooks/use-toast`'; -import { useT } from '`@/lib/i18n/client`';As per coding guidelines: "Imports at the top, exports at the bottom, both sorted."
🤖 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 `@services/platform/app/features/settings/providers/components/provider-options-editor.tsx` around lines 9 - 23, The declaration providerOptionsClientSchema is placed between import statements, breaking the "imports contiguous at top" rule; move the line declaring providerOptionsClientSchema (and its z.record(...) definition) so that all import statements (e.g. CollapsibleGuide, ConfirmDialog, JsonInput, Textarea, Card, HStack, Stack, Sheet, Text, toast, useT) appear first in the file, then add the providerOptionsClientSchema declaration after the imports (and before any exports), ensuring imports remain grouped at the top and declarations follow.
🤖 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 `@services/platform/app/components/ui/data-display/collapsible-guide.tsx`:
- Around line 53-60: The custom react-markdown pre renderer is currently
spreading all incoming props and then overwriting className; update the renderer
in collapsible-guide.tsx to destructure className (e.g. ({ children, className,
...props }) => ...) and merge it with the component's default classes using a
classname helper (clsx/cn) so incoming className is preserved, and only spread
the remaining DOM-safe props (e.g. ...props) onto <pre> to avoid leaking non-DOM
props.
In
`@services/platform/app/features/settings/providers/components/provider-detail-drawer.tsx`:
- Around line 168-243: Remove the JSX section-structure comments in the skeleton
render block (the comment nodes like {/* GeneralSection: header outside, Card
with 3 InfoRows */}, {/* ApiKeySection: ... */}, {/* DefaultModelsSection: ...
*/}, {/* ProviderOptionsSection: ... */}, {/* ModelsSection: ... */}) so only
the actual JSX elements (Stack, HStack, Card, Skeleton, Array.from mappings
keyed by i, and cn usages) remain; do not change any component structure, props,
or keys (e.g., the Array.from maps, key={i}, className uses of cn), just delete
those explanatory "what" comments to comply with the comment guideline.
- Around line 199-203: The skeleton header HStack currently uses className "px-5
py-3.5" which causes a layout jump compared to the loaded API key rows that use
"px-4 py-2.5"; update the skeleton HStack (in provider-detail-drawer.tsx) to use
the same padding classes as the API key rows (replace "px-5 py-3.5" with "px-4
py-2.5") so the loading and resolved states share identical spacing (look for
the HStack rendering the API key skeleton and the elements rendering the actual
API key rows to confirm both are updated).
In
`@services/platform/app/features/settings/providers/components/providers-table.tsx`:
- Around line 300-303: The component currently returns null when the
useReadProvider hook yields no ok data (the block checking data?.ok after
calling useReadProvider(orgSlug, providerName, { enabled: !!orgSlug })), which
gives no user feedback; change this to render a loading state while the query is
fetching (e.g., a spinner/skeleton inside the sheet when data is undefined or
isLoading from the hook) and render an error state when the query returns an
error or data.ok is false (show an inline error message and trigger a toast via
your existing toast utility) instead of returning null so users see progress and
failure reasons when they click Edit.
---
Outside diff comments:
In
`@services/platform/app/features/settings/providers/components/provider-options-editor.tsx`:
- Around line 9-23: The declaration providerOptionsClientSchema is placed
between import statements, breaking the "imports contiguous at top" rule; move
the line declaring providerOptionsClientSchema (and its z.record(...)
definition) so that all import statements (e.g. CollapsibleGuide, ConfirmDialog,
JsonInput, Textarea, Card, HStack, Stack, Sheet, Text, toast, useT) appear first
in the file, then add the providerOptionsClientSchema declaration after the
imports (and before any exports), ensuring imports remain grouped at the top and
declarations follow.
🪄 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: f1ad83d1-4294-4424-8f8b-c76b1a4844e9
📒 Files selected for processing (10)
services/platform/app/components/ui/data-display/collapsible-guide.tsxservices/platform/app/components/ui/layout/card.tsxservices/platform/app/components/ui/navigation/tabs.tsxservices/platform/app/features/settings/providers/components/provider-add-panel.tsxservices/platform/app/features/settings/providers/components/provider-detail-drawer.tsxservices/platform/app/features/settings/providers/components/provider-options-editor.tsxservices/platform/app/features/settings/providers/components/providers-table.tsxservices/platform/messages/de.jsonservices/platform/messages/en.jsonservices/platform/messages/fr.json
Comment on lines
+53
to
+60
| pre: ({ children, ...rest }) => ( | ||
| <pre | ||
| {...rest} | ||
| className="bg-muted/60 border-border my-2 overflow-x-auto rounded-md border p-3 font-mono text-[11px] leading-relaxed" | ||
| > | ||
| {children} | ||
| </pre> | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find react-markdown usage and likely custom component renderers
rg -n "react-markdown" -S .
rg -n "components=\{\{[^\}]*pre\s*:" -S --glob="*.{ts,tsx,js,jsx}" .
rg -n "pre:\s*\(\s*\{[^}]*className" -S --glob="*.{ts,tsx,js,jsx}" .
rg -n "node:\s*_" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components/ui/data-display || trueRepository: tale-project/tale
Length of output: 6166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Limit search scope to likely directories to avoid huge output
rg -n "react-markdown" -S services/platform/app/components --glob="*.{ts,tsx,js,jsx}"
rg -n "components=\{\{[^\}]*pre\s*:" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components
rg -n "pre:\s*\(\s*\{[^}]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components
# Also look for cn(., className) merge patterns in markdown pre/code renderers
rg -n "cn\([^)]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components | head -n 50Repository: tale-project/tale
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "react-markdown" -S services/platform/app/components --glob="*.{ts,tsx,js,jsx}"
rg -n "components=\{\{[^\}]*pre\s*:" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components || true
rg -n "pre:\s*\(\s*\{[^}]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components || true
rg -n "cn\([^)]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components | head -n 50Repository: tale-project/tale
Length of output: 6360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "react-markdown" -S services/platform/app/components --glob="*.{ts,tsx,js,jsx}"
rg -n "components=\{\{[^\}]*pre\s*:" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components || true
rg -n "pre:\s*\(\s*\{[^}]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components || true
rg -n "cn\([^)]*className" -S --glob="*.{ts,tsx,js,jsx}" services/platform/app/components | head -n 50Repository: tale-project/tale
Length of output: 6434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "react-markdown" -S . --glob="*.{ts,tsx,js,jsx}" | head -n 100Repository: tale-project/tale
Length of output: 5261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "react-markdown" -S . --glob="*.{ts,tsx,js,jsx}" | head -n 200Repository: tale-project/tale
Length of output: 5261
Preserve incoming className (and avoid leaking non-DOM props) in react-markdown pre: the current renderer spreads ...rest onto <pre> and then hard-sets className, overwriting any provided className.
Proposed fix
- pre: ({ children, ...rest }) => (
+ pre: ({ node: _node, className, children, ...rest }) => (
<pre
{...rest}
- className="bg-muted/60 border-border my-2 overflow-x-auto rounded-md border p-3 font-mono text-[11px] leading-relaxed"
+ className={cn(
+ 'bg-muted/60 border-border my-2 overflow-x-auto rounded-md border p-3 font-mono text-[11px] leading-relaxed',
+ className,
+ )}
>
{children}
</pre>
),🤖 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 `@services/platform/app/components/ui/data-display/collapsible-guide.tsx`
around lines 53 - 60, The custom react-markdown pre renderer is currently
spreading all incoming props and then overwriting className; update the renderer
in collapsible-guide.tsx to destructure className (e.g. ({ children, className,
...props }) => ...) and merge it with the component's default classes using a
classname helper (clsx/cn) so incoming className is preserved, and only spread
the remaining DOM-safe props (e.g. ...props) onto <pre> to avoid leaking non-DOM
props.
Comment on lines
168
to
243
| {/* GeneralSection: header outside, Card with 3 InfoRows */} | ||
| <Stack gap={2}> | ||
| <HStack justify="between" align="center"> | ||
| <Skeleton className="h-5 w-16" /> | ||
| <Skeleton className="h-5 w-32" /> | ||
| <Skeleton className="h-4 w-28" /> | ||
| <Skeleton className="h-8 w-20" /> | ||
| </HStack> | ||
| </Card> | ||
| <Card contentClassName="p-5"> | ||
| <Stack gap={4}> | ||
| <HStack justify="between" className="border-b pb-4"> | ||
| <Skeleton className="h-5 w-28" /> | ||
| <Skeleton className="h-4 w-20" /> | ||
| <Card contentClassName="p-0"> | ||
| {Array.from({ length: 3 }).map((_, i) => ( | ||
| <HStack | ||
| key={i} | ||
| gap={4} | ||
| align="start" | ||
| className={cn('px-4 py-2.5', i < 2 && 'border-b')} | ||
| > | ||
| <Skeleton className="h-4 w-28 shrink-0" /> | ||
| <Skeleton className="h-4 w-48" /> | ||
| </HStack> | ||
| ))} | ||
| </Card> | ||
| </Stack> | ||
|
|
||
| {/* ApiKeySection: header (title + 2 ghost buttons) outside, Card with badge row */} | ||
| <Stack gap={2}> | ||
| <HStack justify="between" align="center"> | ||
| <Skeleton className="h-5 w-16" /> | ||
| <HStack gap={1} align="center"> | ||
| <Skeleton className="h-8 w-28" /> | ||
| <Skeleton className="h-8 w-24" /> | ||
| </HStack> | ||
| <Stack gap={3} className="divide-y"> | ||
| {Array.from({ length: 3 }).map((_, i) => ( | ||
| <HStack key={i} gap={4} className="pt-3 first:pt-0"> | ||
| <Skeleton className="h-4 w-28" /> | ||
| <Skeleton className="h-4 w-32" /> | ||
| </HStack> | ||
| ))} | ||
| </HStack> | ||
| <Card contentClassName="p-0"> | ||
| <HStack gap={4} align="center" className="px-5 py-3.5"> | ||
| <Skeleton className="h-5 w-24 rounded-full" /> | ||
| <Skeleton className="h-4 w-32" /> | ||
| </HStack> | ||
| </Card> | ||
| </Stack> | ||
|
|
||
| {/* DefaultModelsSection: header (title + description + Edit) outside, Card with 5 InfoRows */} | ||
| <Stack gap={2}> | ||
| <HStack justify="between" align="start"> | ||
| <Stack gap={1} className="min-w-0"> | ||
| <Skeleton className="h-5 w-32" /> | ||
| <Skeleton className="h-4 w-64" /> | ||
| </Stack> | ||
| </Stack> | ||
| </Card> | ||
| <Skeleton className="h-8 w-20" /> | ||
| </HStack> | ||
| <Card contentClassName="p-0"> | ||
| {Array.from({ length: 5 }).map((_, i) => ( | ||
| <HStack | ||
| key={i} | ||
| gap={4} | ||
| align="start" | ||
| className={cn('px-4 py-2.5', i < 4 && 'border-b')} | ||
| > | ||
| <Skeleton className="h-4 w-28 shrink-0" /> | ||
| <Skeleton className="h-4 w-32" /> | ||
| </HStack> | ||
| ))} | ||
| </Card> | ||
| </Stack> | ||
|
|
||
| {/* ProviderOptionsSection: header outside, collapsible guide, Card with placeholder */} | ||
| <Stack gap={2}> | ||
| <HStack justify="between" align="center"> | ||
| <Skeleton className="h-5 w-48" /> | ||
| <Skeleton className="h-8 w-20" /> | ||
| </HStack> | ||
| <Skeleton className="h-9 w-full rounded-lg" /> | ||
| <Card contentClassName="p-5"> | ||
| <Skeleton className="h-4 w-64" /> | ||
| </Card> | ||
| </Stack> | ||
|
|
||
| {/* ModelsSection: header (title + 2 ghost buttons) outside, bordered list */} | ||
| <Stack gap={3}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove section-structure JSX comments in the skeleton block.
These comments describe layout “what” rather than intent “why,” and they add noise in a straightforward render tree.
As per coding guidelines: "Comments should explain why, rarely what."
🤖 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
`@services/platform/app/features/settings/providers/components/provider-detail-drawer.tsx`
around lines 168 - 243, Remove the JSX section-structure comments in the
skeleton render block (the comment nodes like {/* GeneralSection: header
outside, Card with 3 InfoRows */}, {/* ApiKeySection: ... */}, {/*
DefaultModelsSection: ... */}, {/* ProviderOptionsSection: ... */}, {/*
ModelsSection: ... */}) so only the actual JSX elements (Stack, HStack, Card,
Skeleton, Array.from mappings keyed by i, and cn usages) remain; do not change
any component structure, props, or keys (e.g., the Array.from maps, key={i},
className uses of cn), just delete those explanatory "what" comments to comply
with the comment guideline.
Comment on lines
+199
to
+203
| <HStack gap={4} align="center" className="px-5 py-3.5"> | ||
| <Skeleton className="h-5 w-24 rounded-full" /> | ||
| <Skeleton className="h-4 w-32" /> | ||
| </HStack> | ||
| </Card> |
There was a problem hiding this comment.
Align API key skeleton spacing with loaded content spacing.
Line 199 uses px-5 py-3.5, but the loaded API key rows use px-4 py-2.5 (Lines 603 and 612). This introduces a layout jump between loading and resolved states.
Proposed fix
- <HStack gap={4} align="center" className="px-5 py-3.5">
+ <HStack gap={4} align="center" className="px-4 py-2.5">Also applies to: 603-613
🤖 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
`@services/platform/app/features/settings/providers/components/provider-detail-drawer.tsx`
around lines 199 - 203, The skeleton header HStack currently uses className
"px-5 py-3.5" which causes a layout jump compared to the loaded API key rows
that use "px-4 py-2.5"; update the skeleton HStack (in
provider-detail-drawer.tsx) to use the same padding classes as the API key rows
(replace "px-5 py-3.5" with "px-4 py-2.5") so the loading and resolved states
share identical spacing (look for the HStack rendering the API key skeleton and
the elements rendering the actual API key rows to confirm both are updated).
Comment on lines
+300
to
+303
| const { data } = useReadProvider(orgSlug, providerName, { | ||
| enabled: !!orgSlug, | ||
| }); | ||
| if (!data?.ok) return null; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding loading and error feedback.
Returning null while data loads or on error leaves users without feedback after clicking Edit. A brief loading indicator (e.g., skeleton or spinner in a sheet) and error handling (toast or inline message) would improve reliability perception.
♻️ Suggested improvement
function ProviderEditPanelLoader({
orgSlug,
providerName,
organizationId,
onClose,
}: {
orgSlug: string;
providerName: string;
organizationId: string;
onClose: () => void;
}) {
- const { data } = useReadProvider(orgSlug, providerName, {
+ const { data, isLoading, isError } = useReadProvider(orgSlug, providerName, {
enabled: !!orgSlug,
});
- if (!data?.ok) return null;
+
+ if (isLoading) {
+ // Render a loading sheet/skeleton matching ProviderEditPanel layout
+ return <ProviderEditPanelSkeleton open onClose={onClose} />;
+ }
+
+ if (isError || !data?.ok) {
+ // Close panel and notify user
+ onClose();
+ return null;
+ }
+
return (You may also want to trigger a toast in the error branch to inform users why the panel closed.
🤖 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
`@services/platform/app/features/settings/providers/components/providers-table.tsx`
around lines 300 - 303, The component currently returns null when the
useReadProvider hook yields no ok data (the block checking data?.ok after
calling useReadProvider(orgSlug, providerName, { enabled: !!orgSlug })), which
gives no user feedback; change this to render a loading state while the query is
fetching (e.g., a spinner/skeleton inside the sheet when data is undefined or
isLoading from the hook) and render an error state when the query returns an
error or data.ok is false (show an inline error message and trigger a toast via
your existing toast utility) instead of returning null so users see progress and
failure reasons when they click Edit.
…factor - CollapsibleGuide: preserve incoming className on the markdown <pre> renderer (merge with cn) and strip the non-DOM `node` prop instead of overwriting className and leaking node onto the DOM element. - ProviderDetailSkeleton: drop the JSX section-structure comments — they describe what the layout is, not why. - ProviderDetailSkeleton: align API-key row padding with the loaded state (px-5 py-3.5 → px-4 py-2.5) so loading and resolved states share spacing.
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
Tighten the provider detail drawer layout and the provider-options copy.
SectionHeaderhelper.Buttons for visual consistency with the rest of the surface.px-5 py-3.5→px-4 py-2.5) andalign="start"so multi-line values wrap cleanly; skeleton rebuilt to mirror the new layout.CollapsibleGuidewith markdown: bold headings, bullet lists, fenced```jsonblocks. Newproviders.providerOptions.guideLabeltranslation added in en/de/fr.CollapsibleGuidenow styles<pre>blocks.ProviderEditPanelinProviderEditPanelLoaderthat fetches config + hash and suppliesProviderConfigProvider. Without ituseProviderConfigthrows when invoked from the row-level Edit action.Cardradiusrounded-xl→rounded-lg; tabs listmin-w-full justify-aroundso the underline spans the container; the provider-add-panel Models header matches the drawer styling.Test plan
Summary by CodeRabbit
Release Notes
Improvements
Documentation