refactor: updates prompt model parameters UI#2548
Conversation
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR restructures prompt deployment UI components by introducing an accordion-based layout. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
5f923f9 to
face6b2
Compare
77478f2 to
f5f03a7
Compare
face6b2 to
44734e7
Compare
f5f03a7 to
c1cd6b8
Compare
44734e7 to
369f154
Compare
c1cd6b8 to
729fa28
Compare
369f154 to
56330d2
Compare
729fa28 to
bf18504
Compare
56330d2 to
548ddba
Compare
bf18504 to
36cfa1c
Compare
4f1cda0 to
5c75f3d
Compare
36cfa1c to
32633c8
Compare
Confidence Score: 4/5Safe to merge after addressing the stale One P1 issue: when ui/components/prompts/fragments/settingsPanel.tsx — needs a Important Files Changed
Reviews (15): Last reviewed commit: "refactor: updates prompt model parameter..." | Re-trigger Greptile |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ui/components/prompts/fragments/settingsPanel.tsx (1)
122-123: Prefer reusing the shared section type to avoid drift.
SettingsSectionduplicatesSettingsSidebarSectionfromPromptDeploymentsAccordionItem. Reusing one type will keep this stack safer as sections evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/fragments/settingsPanel.tsx` around lines 122 - 123, The local SettingsSection type duplicates PromptDeploymentsAccordionItem's SettingsSidebarSection; remove the duplicate type and import/reuse PromptDeploymentsAccordionItem's SettingsSidebarSection (or the shared exported type if available), then update the useState generic and initial value to use that imported type (e.g., useState<SettingsSidebarSection | undefined>("parameters")). If the type isn't exported yet, export it from PromptDeploymentsAccordionItem (or create a single shared type file) and update both places to reference that shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ui/app/_fallbacks/enterprise/components/prompt-deployments/promptDeploymentsAccordionItem.tsx`:
- Around line 30-32: The new interactive AccordionTrigger for "Deployments" is
missing a stable test selector; update the AccordionTrigger element (the JSX
component named AccordionTrigger in promptDeploymentsAccordionItem.tsx) to
include a data-testid attribute following the project convention like
data-testid="prompt-deployments-trigger" (pattern:
<entity>-<element>-<qualifier>), ensuring the value uniquely identifies this
Deployments trigger for tests.
In
`@ui/app/_fallbacks/enterprise/components/prompt-deployments/promptDeploymentView.tsx`:
- Line 4: The omitTitle prop on PromptDeploymentView is declared but unused, so
passing omitTitle has no effect; update the PromptDeploymentView function to
accept and destructure the prop (omitTitle) and conditionally skip rendering the
title element (the JSX element that renders the deployment prompt title inside
PromptDeploymentView) when omitTitle is true—locate the title markup in
PromptDeploymentView and wrap it with a conditional that renders it only when
!omitTitle.
In `@ui/components/prompts/fragments/settingsPanel.tsx`:
- Around line 165-167: Add a stable test selector to the new accordion trigger
by adding a data-testid attribute to the AccordionTrigger element in
settingsPanel.tsx; set it to the project convention value (for example
"prompts-configuration-trigger") so the element can be reliably selected in
tests and QA.
---
Nitpick comments:
In `@ui/components/prompts/fragments/settingsPanel.tsx`:
- Around line 122-123: The local SettingsSection type duplicates
PromptDeploymentsAccordionItem's SettingsSidebarSection; remove the duplicate
type and import/reuse PromptDeploymentsAccordionItem's SettingsSidebarSection
(or the shared exported type if available), then update the useState generic and
initial value to use that imported type (e.g., useState<SettingsSidebarSection |
undefined>("parameters")). If the type isn't exported yet, export it from
PromptDeploymentsAccordionItem (or create a single shared type file) and update
both places to reference that shared symbol.
🪄 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: efb40927-f5cf-4541-ab54-112de6fa5570
📒 Files selected for processing (5)
ui/app/_fallbacks/enterprise/components/prompt-deployments/promptDeploymentView.tsxui/app/_fallbacks/enterprise/components/prompt-deployments/promptDeploymentsAccordionItem.tsxui/app/workspace/routing-rules/views/routingRuleSheet.tsxui/components/prompts/fragments/settingsPanel.tsxui/components/ui/accordion.tsx
5c75f3d to
26f752b
Compare
32633c8 to
7dca2fa
Compare
e335344 to
43002ba
Compare
8b33aec to
014fd47
Compare
43002ba to
e93673c
Compare
7eb2cbc to
7cd2a3d
Compare
e93673c to
b837551
Compare
7cd2a3d to
fe6f971
Compare
c1b9df5 to
58be647
Compare
fe6f971 to
272618a
Compare
58be647 to
6b59828
Compare
272618a to
53dd746
Compare
6b59828 to
f13c53d
Compare
53dd746 to
29a6458
Compare
29a6458 to
9cd0a3e
Compare
31196ed to
7462cc9
Compare
423ec88 to
944dd72
Compare
7462cc9 to
2a1ede0
Compare
Merge activity
|
The base branch was changed.
2a1ede0 to
f49aac1
Compare
| @@ -131,66 +140,98 @@ export function SettingsPanel() { | |||
| } | |||
|
|
|||
| return ( | |||
| <div className="flex h-full flex-col"> | |||
| <ScrollArea className="grow overflow-y-auto" viewportClassName="no-table"> | |||
| <div className="space-y-6 p-4"> | |||
| <div className="flex flex-col gap-2" data-testid="settings-provider"> | |||
| <Label className="text-muted-foreground text-xs font-medium uppercase">Provider</Label> | |||
| <ComboboxSelect | |||
| options={providerOptions} | |||
| value={provider} | |||
| onValueChange={(v) => v && onProviderChange(v)} | |||
| placeholder="Select provider" | |||
| hideClear | |||
| /> | |||
| </div> | |||
|
|
|||
| <div className="flex flex-col gap-2" data-testid="settings-model"> | |||
| <Label className="text-muted-foreground text-xs font-medium uppercase">Model</Label> | |||
| <ModelMultiselect | |||
| provider={provider} | |||
| keys={filterKeys && filterKeys.length > 0 ? filterKeys : undefined} | |||
| vks={filterVks} | |||
| value={model} | |||
| onChange={(v) => onModelChange(v)} | |||
| isSingleSelect | |||
| unfiltered | |||
| placeholder={!provider ? "Select a provider first" : "Select model"} | |||
| disabled={!provider} | |||
| /> | |||
| </div> | |||
|
|
|||
| {(providerKeys.length > 0 || providerVirtualKeys.length > 0) && !!provider && ( | |||
| <ApiKeySelectorView | |||
| providerKeys={providerKeys} | |||
| virtualKeys={providerVirtualKeys} | |||
| value={apiKeyId} | |||
| onValueChange={(v) => onApiKeyIdChange(v ?? "__auto__")} | |||
| disabled={!provider} | |||
| /> | |||
| )} | |||
|
|
|||
| {Object.keys(variables).length > 0 && ( | |||
| <> | |||
| <Separator /> | |||
| <VariablesTableView variables={variables} onChange={setVariables} /> | |||
| </> | |||
| )} | |||
|
|
|||
| {model && ( | |||
| <> | |||
| <Separator /> | |||
|
|
|||
| <div className="flex flex-col gap-4"> | |||
| <Label className="text-muted-foreground text-xs font-medium uppercase">Model Parameters</Label> | |||
| <ModelParameters model={model} config={modelParams} onChange={handleModelParamsChange} hideFields={["promptTools"]} /> | |||
| <div className="flex h-full min-h-0 flex-col"> | |||
| <div className="flex min-h-0 flex-1 flex-col px-4 pb-4 pt-2"> | |||
| <Accordion | |||
| type="single" | |||
| collapsible | |||
| value={openSection ?? ""} | |||
| onValueChange={(v) => { | |||
| if (v === "parameters" || v === "deployments") { | |||
| setOpenSection(v); | |||
| } else { | |||
| setOpenSection(undefined); | |||
| } | |||
| }} | |||
| className="flex min-h-0 flex-1 flex-col" | |||
There was a problem hiding this comment.
Stale
openSection state when a prompt is deselected
When the user has the "Deployments" section active (openSection === "deployments") and then deselects the current prompt, selectedPromptId becomes falsy and PromptDeploymentsAccordionItem unmounts — but openSection stays "deployments". Because the AccordionItem for "Configuration" computes its className using openSection:
openSection === "parameters" ? "flex-1" : "shrink-0 overflow-hidden"…it gets "shrink-0 overflow-hidden" even though nothing is actually open, leaving the panel showing only a shrunken collapsed trigger bar. The user has to manually click "Configuration" to restore a usable layout.
Consider resetting openSection back to "parameters" when selectedPromptId is cleared:
useEffect(() => {
if (!selectedPromptId && openSection === "deployments") {
setOpenSection("parameters");
}
}, [selectedPromptId, openSection]);
Summary
Refactored the prompt settings panel to use an accordion layout, separating configuration and deployments into collapsible sections for improved organization and space utilization.
Changes
PromptDeploymentsAccordionItemcomponent to handle the deployments section within the accordionAccordionContentcomponent with acontainerClassNameprop for better layout controlPromptDeploymentViewto accept an optionalomitTitleprop for cleaner integrationType of change
Affected areas
How to test
Validate the accordion functionality and layout in the prompt settings panel:
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
No security implications - this is a UI layout refactor that doesn't affect authentication, data handling, or API interactions.
Checklist
docs/contributing/README.mdand followed the guidelines