adds dynamic ui #655
adds dynamic ui #655
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughReworked sidebar navigation and added Plugins, Governance, Prompt Repository, and Alert Channels; introduced plugin path/isCustom fields with DB migration and server-side plugin status tracking; converted several Dialogs to Sheets; standardized centered max-width page wrappers; promo-card empty-state now informs sidebar layout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI
participant PluginsAPI
participant Server
User->>UI: Open Plugins page
UI->>PluginsAPI: useGetPluginsQuery()
PluginsAPI-->>UI: plugins list (includes path,isCustom)
User->>UI: Click "Install New Plugin" / "Edit"
UI->>UI: Open AddNewPluginSheet (prefill if edit)
User->>UI: Submit plugin form (name,path,config)
UI->>PluginsAPI: create/update plugin (payload includes path)
PluginsAPI->>Server: persist plugin, call ReloadPlugin if enabled
Server-->>PluginsAPI: success/error (and plugin status updates)
PluginsAPI-->>UI: response
UI-->>User: close sheet & refresh list
sequenceDiagram
autonumber
participant Bootstrap
participant Server
participant ConfigStore
Bootstrap->>Server: LoadPlugins(ctx, config)
Server->>ConfigStore: read plugin configs
Server-->>Server: initialize pluginStatus entries (loading/disabled/loaded/error)
Server->>Server: set pluginStatus mutex and store statuses
Server-->>Bootstrap: return plugins + pluginStatus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/components/sidebar.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
ui/components/sidebar.tsx (2)
15-15: LGTM!The icon imports are correctly added and used by the new navigation items.
Also applies to: 19-19
102-107: No issues found—the /plugins route is properly implemented.The verification confirms that
ui/app/plugins/page.tsxexists, so the new "Plugins" navigation item at lines 102-107 correctly points to an existing route. The navigation item follows the established pattern and is ready for merge.
|
|
||
| { | ||
| title: "MCP Clients", | ||
| title: "MCP Gateway", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify naming consistency across the codebase.
The navigation item has been renamed from "MCP Clients" to "MCP Gateway", but the URL remains /mcp-clients. Ensure this terminology change is consistent across:
- Page titles and headers
- Documentation
- Other UI references
- API endpoint names
Run the following script to check for references to the old name:
🏁 Script executed:
#!/bin/bash
# Description: Check for "MCP Clients" references that may need updating
# Search for "MCP Clients" in UI files (case-insensitive to catch variations)
echo "=== Searching for 'MCP Clients' references ==="
rg -i "mcp\s+clients?" ui/ --type tsx --type ts --type jsx --type js -C 2
# Search for page titles or headers that might still use old terminology
echo -e "\n=== Checking mcp-clients page files ==="
fd -e tsx -e ts "page|layout|index" ui/app/mcp-clients/ ui/src/app/mcp-clients/ 2>/dev/null | xargs cat 2>/dev/null || echo "No files found"Length of output: 864
Let me search more broadly for inconsistencies:
---
🏁 Script executed:
```shell
#!/bin/bash
# Corrected search without type specifiers and broader scope
echo "=== Searching for 'MCP Clients' (all files) ==="
rg -i "mcp clients|mcp_clients" ui/ -C 2 | head -50
echo -e "\n=== Searching for 'MCP Gateway' references ==="
rg -i "mcp gateway|mcp.gateway" ui/ -C 2
echo -e "\n=== Checking sidebar.tsx for context ==="
rg -n "MCP Gateway|MCP Clients" ui/components/sidebar.tsx -B 3 -A 3
echo -e "\n=== Checking table and hook files ==="
fd -type f "table|hook" ui/ | rg -i "mcp" | head -20
Length of output: 4664
🏁 Script executed:
# Also check imports and related queries
echo "=== Files importing mcp-clients related items ==="
rg "import.*[Mm]cp.*lient" ui/ -l | head -20Length of output: 166
Incomplete rename: "MCP Clients" to "MCP Gateway" only applied to sidebar navigation.
The sidebar title was changed to "MCP Gateway" (line 110), but this terminology is not reflected elsewhere in the codebase, creating user-facing inconsistency. The underlying functionality, components, documentation, and UI elements still use "MCP Clients":
- UI Components:
mcpClientsTable.tsxdisplays "Registered MCP Clients" - Documentation:
README.mdreferences "MCP clients" multiple times - API/Hooks: All remain as
getMCPClients,MCPClientsTable, etc. - Icons:
sidebar.tsxSVG retains "MCP clients icon" label - Virtual Keys: Configuration UI still refers to "MCP clients"
Either complete the rename across the codebase or revert the sidebar change to "MCP Clients" for consistency.
🤖 Prompt for AI Agents
In ui/components/sidebar.tsx around line 110 the sidebar title was changed to
"MCP Gateway" but the rest of the codebase still uses "MCP Clients"; either
revert this line back to "MCP Clients" to restore consistency, or perform a
complete rename across files (UI labels, component names, API/hook names,
documentation, SVG aria/labels, and configuration keys) ensuring casing and
plurals match, update import/exports and tests accordingly, and run a
project-wide search/replace to update getMCPClients, MCPClientsTable, README
references, SVG labels and any virtual-key strings to the chosen term.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/components/sidebar.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
ui/components/sidebar.tsx (3)
15-15: LGTM!The new icon imports are appropriate for their respective menu items (Puzzle for plugins, Siren for alert channels).
Also applies to: 19-19
110-110: LGTM!The label change from "MCP Clients" to "MCP Gateway" improves clarity while maintaining backward compatibility by keeping the URL unchanged.
102-107: Route verified—no changes needed.The
/pluginsroute exists atui/app/plugins/page.tsxwith a fully implemented page component. The new Plugins menu item will function correctly without 404 errors.
akshaydeo
force-pushed
the
10-20-dynamic-plugins-ui
branch
from
abcd1d1 to
c6f4b95
Compare
akshaydeo
force-pushed
the
10-16-dynamic_plugin_architecture
branch
from
8c12ca7 to
ddd7b35
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
ui/app/providers/views/providerKeyForm.tsx (2)
107-107: Optimize: CallgetTooltipContent()only once.The function is called twice on this line—once to check if it's truthy and once to render the content. Store the result in a variable to avoid redundant calls.
Apply this diff:
+ const tooltipContent = getTooltipContent(); - {getTooltipContent() && <TooltipContent>{getTooltipContent()}</TooltipContent>} + {tooltipContent && <TooltipContent>{tooltipContent}</TooltipContent>}Or use a more compact approach by moving the logic inside the component:
- {getTooltipContent() && <TooltipContent>{getTooltipContent()}</TooltipContent>} + <TooltipContent>{getTooltipContent()}</TooltipContent>Note: The second approach renders
TooltipContenteven when the content isnull, but Tooltip components typically handle null/empty content gracefully by not displaying.
53-61: Remove unnecessary root error check that will never execute.The tooltip logic checks
form.formState.errors.root?.message, but this path is unreachable because your form never callsform.setError("root", ...). Root errors aren't set anywhere in this component or anywhere in the codebase—Zod validation only produces field-level errors.Simplify the tooltip logic to remove the dead code path:
const getTooltipContent = useCallback(() => { if (!form.formState.isDirty) { return "No changes made"; } return null; }, [form?.formState.isDirty]);If you need to display validation errors in the future, either implement
form.setError("root", ...)in your error handler or extract field-level errors fromform.formState.errorsdirectly.ui/app/logs/views/logsTable.tsx (1)
81-82: Height calculation adjusted for new page layout.The reduced offset (20rem → 15.5rem) increases the visible table area by 4.5rem, likely compensating for the centered page wrapper changes applied elsewhere. Both the container and Table maintain matching max-heights.
Consider documenting why 15.5rem was chosen, or use CSS variables to make the calculation more maintainable if the page layout changes again.
ui/app/cluster/page.tsx (1)
4-8: Layout wrapper applied consistently.This matches the centered container pattern used across multiple pages in this PR.
The layout wrapper pattern
<div className="mx-auto w-full max-w-7xl">is repeated across at least 7 pages in this PR. Consider extracting this into a reusable layout component (e.g.,PageContainerorCenteredLayout) to reduce duplication and simplify future layout changes.Example:
// components/layouts/PageContainer.tsx export function PageContainer({ children }: { children: React.ReactNode }) { return <div className="mx-auto w-full max-w-7xl">{children}</div>; }Then use it as:
export default async function ClusterPage() { return ( <PageContainer> <ClusterView /> </PageContainer> ); }ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
154-154: Remove redundant value prop.The
{...field}spread already includes thevalueproperty from react-hook-form. The explicitvalue={field.value || ""}is redundant and the empty string fallback is unnecessary since the form's default value is already an empty string.Apply this diff:
- <Input placeholder={"https://api.your-provider.com"} {...field} value={field.value || ""} /> + <Input placeholder="https://api.your-provider.com" {...field} />ui/app/providers/dialogs/addNewKeySheet.tsx (1)
14-17: Rename variable for consistency.The component was renamed from
AddNewKeyDialogtoAddNewKeySheet, but the variabledialogTitlestill references "dialog". Consider renaming it to match the Sheet pattern.Apply this diff to improve naming consistency:
export default function AddNewKeySheet({ show, onCancel, provider, keyIndex }: Props) { const isEditing = keyIndex < provider.keys.length; - const dialogTitle = isEditing ? "Edit key" : "Add new key"; + const title = isEditing ? "Edit key" : "Add new key"; const successMessage = isEditing ? "Key updated successfully" : "Key added successfully";And update the usage on line 28:
- {dialogTitle} + {title}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(1 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- ui/app/observability/page.tsx
- ui/app/user-groups/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/components/sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/app/providers/views/modelProviderConfig.tsx (3)
ui/lib/types/config.ts (1)
isKnownProvider(15-17)ui/app/providers/views/utils.ts (1)
keysRequired(1-1)ui/components/ui/accordion.tsx (1)
Accordion(47-47)
ui/app/audit-logs/page.tsx (1)
ui/app/_fallbacks/enterprise/components/audit-logs/auditLogsView.tsx (1)
AuditLogsView(4-16)
ui/app/virtual-keys/page.tsx (1)
ui/app/virtual-keys/views/virtualKeysTable.tsx (1)
VirtualKeysTable(34-230)
ui/app/adaptive-routing/page.tsx (1)
ui/app/_fallbacks/enterprise/components/adaptive-routing/adaptiveRoutingView.tsx (1)
AdaptiveRoutingView(4-16)
ui/app/providers/page.tsx (1)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
AddCustomProviderSheet(41-176)
ui/app/providers/views/providerKeyForm.tsx (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)
ui/app/alert-channels/page.tsx (1)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
AlertChannelsView(4-16)
ui/app/scim/page.tsx (1)
ui/app/_fallbacks/enterprise/components/scim/scimView.tsx (1)
SCIMView(4-16)
ui/app/logs/views/logsTable.tsx (1)
ui/components/ui/table.tsx (1)
Table(71-71)
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
ui/components/ui/alertDialog.tsx (1)
AlertDialogCancel(85-85)ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
ui/components/ui/input.tsx (1)
Input(15-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (19)
ui/app/providers/views/modelProviderConfig.tsx (3)
46-47: Good typo fix and state addition for controlled accordion.The typo fix (
isCustomProver→isCustomProvider) is correct, and addingaccordionValuestate enables a proper controlled component pattern for the Accordion.
52-52: LGTM: Proper controlled component initialization.The
showApiKeysconstant is correctly computed early and used in the initialization effect. The effect properly initializesaccordionValuebased on provider characteristics and will reset the accordion state when the provider changes, which is the expected behavior.Also applies to: 58-60
64-64: Excellent refactoring to a fully controlled accordion.The accordion now uses a proper controlled component pattern with explicit state management. This change also improves UX consistency—the accordion is now always collapsible (previously, providers without API key requirements like Ollama/SGL had non-collapsible accordions). This aligns well with the broader UI standardization effort mentioned in the PR context.
ui/app/providers/views/providerKeyForm.tsx (1)
88-88: Good catch on the CSS class fix!The correction from
bg:whitetobg-whitefixes invalid Tailwind syntax.ui/app/adaptive-routing/page.tsx (1)
4-8: Layout wrapper applied consistently.The centered, constrained-width container follows the same pattern used across other pages in this PR.
ui/app/audit-logs/page.tsx (1)
4-6: LGTM!Layout wrapper applied consistently with the broader refactor pattern.
ui/app/scim/page.tsx (1)
5-9: LGTM!Layout wrapper applied consistently.
ui/app/config/page.tsx (1)
64-64: Layout constraints added to existing flex container.The
max-w-7xl mx-autoclasses center and constrain the config page width, consistent with other pages in this PR.ui/app/virtual-keys/page.tsx (2)
6-6: LGTM!The import is properly used in the governance configuration check below.
73-80: Layout wrapper applied consistently.The centered container follows the same pattern as other pages in this PR.
ui/app/mcp-clients/page.tsx (1)
29-31: LGTM!Layout wrapper applied consistently with the broader refactor.
ui/app/layout.tsx (1)
41-41: LGTM!The layout refactor appropriately delegates width constraints to individual pages, which now consistently apply
max-w-7xl mx-autowrappers. This creates a more flexible and maintainable layout structure.ui/app/guardrails/page.tsx (1)
4-8: LGTM!The layout wrapper is consistent with the pattern applied across other pages in this PR.
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
5-5: LGTM!The migration from Dialog to Sheet components is well-executed and maintains the same functionality while updating the UI pattern.
Also applies to: 41-41, 97-174
ui/app/providers/page.tsx (1)
24-24: LGTM!The import and usage are correctly updated to use the new
AddCustomProviderSheetcomponent, and the layout wrapper is consistent with the pattern applied across the codebase.Also applies to: 94-94, 113-113
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
1-16: LGTM!The AlertChannelsView component follows the established pattern for enterprise feature fallbacks, using ContactUsView with appropriate messaging and links.
ui/app/alert-channels/page.tsx (1)
1-9: LGTM!The AlertChannelsPage correctly wraps the AlertChannelsView with the consistent layout container pattern used throughout the codebase.
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
42-51: Good: Explicit state management for dialog dismissal.The explicit
onClickhandler onAlertDialogCancelcorrectly manages the dialog state. Since theAlertDialogdoesn't have anonOpenChangehandler, this explicit state reset is necessary for the Cancel button to work properly.
78-83: LGTM: AddNewKeySheet integration looks correct.The component is properly integrated with the correct props (
show,onCancel,provider,keyIndex) and the state management is consistent with the delete dialog pattern.
| <EmptyState isSocketConnected={isSocketConnected} error={error} /> | ||
| ) : ( | ||
| <div className="space-y-6"> | ||
| <div className="space-y-6 max-w-7xl mx-auto"> |
There was a problem hiding this comment.
Fix whitespace in className.
There's a tab character between max-w-7xl and mx-auto in the className. This should be a single space for consistency.
Apply this diff:
- <div className="space-y-6 max-w-7xl mx-auto">
+ <div className="space-y-6 max-w-7xl mx-auto">📝 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.
| <div className="space-y-6 max-w-7xl mx-auto"> | |
| <div className="space-y-6 max-w-7xl mx-auto"> |
🤖 Prompt for AI Agents
In ui/app/logs/page.tsx around line 352, the className value contains a tab
character between "max-w-7xl" and "mx-auto"; replace the tab with a single space
so the className reads "space-y-6 max-w-7xl mx-auto" to ensure consistent
whitespace and proper Tailwind parsing.
| <DialogHeader> | ||
| <DialogTitle> | ||
| <Sheet open={show} onOpenChange={onCancel}> | ||
| <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> |
There was a problem hiding this comment.
Fixed min-width may cause responsive issues.
The min-w-[600px] will cause horizontal overflow on mobile devices (typical viewport: 360-428px). Consider using a responsive approach.
Apply this diff to make it mobile-friendly:
- <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4">
+ <SheetContent className="custom-scrollbar bg-white dark:bg-card w-full sm:min-w-[600px] sm:max-w-[600px] py-4">This ensures full width on mobile and fixed 600px on larger screens.
📝 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.
| <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> | |
| <SheetContent className="custom-scrollbar bg-white dark:bg-card w-full sm:min-w-[600px] sm:max-w-[600px] py-4"> |
🤖 Prompt for AI Agents
In ui/app/providers/dialogs/addNewKeySheet.tsx around line 21, the fixed
min-w-[600px] causes horizontal overflow on small viewports; update the
SheetContent class to be full width on mobile and only enforce the 600px minimum
at larger breakpoints (e.g., use a mobile w-full and apply min-w-[600px]
starting at the sm breakpoint) so the sheet fits narrow screens without
overflow.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
ui/app/observability/page.tsx (1)
4-8: Class order inconsistency (minor).The wrapper uses
max-w-7xl mx-auto w-fullwhile other pages usemx-auto w-full max-w-7xl. Both are functionally identical, but consistency would improve maintainability.Apply this diff for consistency:
- <div className="max-w-7xl mx-auto w-full"> + <div className="mx-auto w-full max-w-7xl">ui/app/providers/views/providerKeyForm.tsx (1)
107-107: Optimize: getTooltipContent() is called twice.The function is called once in the condition and once in the content, which is inefficient. Consider storing the result in a variable.
Apply this diff to optimize the tooltip rendering:
- {getTooltipContent() && <TooltipContent>{getTooltipContent()}</TooltipContent>} + {(() => { + const content = getTooltipContent(); + return content && <TooltipContent>{content}</TooltipContent>; + })()}ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
97-97: Consider simplifying onOpenChange for consistency.The onOpenChange handler uses
(open) => !open && onClose()which works but differs from the simpler pattern used in addNewKeySheet.tsx at line 20:onOpenChange={onCancel}. Both Sheet components close on user dismissal, but the inconsistent pattern could be confusing.For consistency with addNewKeySheet.tsx, consider this simpler pattern:
- <Sheet open={show} onOpenChange={(open) => !open && onClose()}> + <Sheet open={show} onOpenChange={onClose}>However, if you prefer the explicit check to only call onClose when the sheet is being closed (not opened), both approaches work correctly.
ui/app/plugins/page.tsx (1)
1-1: Remove unnecessary "use client"; convert to server component and add metadata.The page contains no client-side hooks or interactivity. Route wiring to "/plugins" is confirmed in
ui/components/sidebar.tsx(line 104). This change reduces JS bundle and enables metadata export.-"use client"; +export const metadata = { + title: "Plugins", + description: "Manage custom plugins", +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(1 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/components/sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
ui/app/providers/views/providerKeyForm.tsx (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)
ui/app/adaptive-routing/page.tsx (1)
ui/app/_fallbacks/enterprise/components/adaptive-routing/adaptiveRoutingView.tsx (1)
AdaptiveRoutingView(4-16)
ui/app/audit-logs/page.tsx (1)
ui/app/_fallbacks/enterprise/components/audit-logs/auditLogsView.tsx (1)
AuditLogsView(4-16)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/app/scim/page.tsx (1)
ui/app/_fallbacks/enterprise/components/scim/scimView.tsx (1)
SCIMView(4-16)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
ui/components/ui/form.tsx (1)
Form(160-160)ui/components/ui/input.tsx (1)
Input(15-69)
ui/app/alert-channels/page.tsx (1)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
AlertChannelsView(4-16)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/logs/views/logsTable.tsx (1)
ui/components/ui/table.tsx (1)
Table(71-71)
ui/app/providers/page.tsx (1)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
AddCustomProviderSheet(41-176)
ui/app/virtual-keys/page.tsx (1)
ui/app/virtual-keys/views/virtualKeysTable.tsx (1)
VirtualKeysTable(34-230)
ui/app/providers/views/modelProviderConfig.tsx (2)
ui/lib/types/config.ts (1)
isKnownProvider(15-17)ui/app/providers/views/utils.ts (1)
keysRequired(1-1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (23)
ui/app/adaptive-routing/page.tsx (1)
4-8: LGTM!The centered max-width container follows the consistent layout pattern applied across all pages in this PR.
ui/app/cluster/page.tsx (1)
4-8: LGTM!The centered max-width container follows the consistent layout pattern applied across all pages in this PR.
ui/app/config/page.tsx (1)
63-91: LGTM!The max-width constraint and centering have been correctly added to the existing flex container, maintaining the layout structure while applying the consistent page width pattern.
ui/app/mcp-clients/page.tsx (1)
28-32: LGTM!The centered max-width container follows the consistent layout pattern applied across all pages in this PR.
ui/app/audit-logs/page.tsx (1)
4-6: LGTM!The centered max-width container follows the consistent layout pattern applied across all pages in this PR.
ui/app/logs/views/logsTable.tsx (1)
81-82: Verify the height calculation adjustment across viewports.The height calculation has been reduced from
calc(100vh-20rem)tocalc(100vh-15.5rem), allocating 4.5rem more vertical space for the table. While this aligns with the page layout changes, ensure this doesn't cause overflow issues on smaller screens or with varying header heights.Test the logs page on different viewport sizes to confirm the table height adjusts correctly without causing layout issues.
ui/app/guardrails/page.tsx (1)
4-8: LGTM! Consistent layout wrapper applied.The centered max-width container aligns with the broader layout standardization across the UI.
ui/app/scim/page.tsx (1)
5-9: LGTM! Consistent layout wrapper applied.The centered max-width container follows the same pattern used across other pages in this PR.
ui/app/user-groups/page.tsx (1)
105-105: LGTM! Consistent layout constraint added.Adding
max-w-7xl mx-autoto the existing flex container maintains the layout while applying the standard width constraint used across the UI.ui/app/virtual-keys/page.tsx (1)
73-80: LGTM! Consistent layout wrapper applied.The centered max-width container aligns with the layout standardization across the application.
ui/app/layout.tsx (1)
41-41: Layout refactor moves width control to individual pages.This change removes the responsive width constraints (
w-5xl,2xl:w-7xl) from the main container, allowing individual pages to control their own max-width viamax-w-7xlwrappers. The reduced padding (py-12→p-4) appears intentional but should be verified across all pages to ensure consistent spacing.Please verify that the reduced padding and flex layout work correctly across all pages, especially on mobile viewports.
ui/app/providers/views/providerKeyForm.tsx (2)
53-61: LGTM! Tooltip logic extracted into reusable function.The
useCallbackhook properly memoizes the tooltip content logic with the correct dependencies.
88-88: Good catch! Typo fixed.Corrected
bg:whitetobg-white.ui/app/providers/views/modelProviderConfig.tsx (3)
46-47: LGTM! Accordion state management added.Adding controlled accordion state allows better control over the accordion's open/closed state. The variable rename from
isCustomProvertoisCustomProviderfixes a typo.
52-60: LGTM! Smart accordion initialization logic.The effect automatically opens the accordion for custom providers or when API keys are not required, improving the user experience.
64-64: LGTM! Accordion is now controlled.Converting to a controlled accordion with
valueandonValueChangeprovides better state management.ui/app/alert-channels/page.tsx (1)
1-9: LGTM! New alert channels page follows established patterns.The page structure is consistent with other pages in the application, using the same centered max-width container layout.
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
1-16: LGTM!The AlertChannelsView component is well-structured and correctly integrates with ContactUsView to provide an enterprise feature placeholder. The props are properly configured to direct users to the alert channels documentation and demo booking.
ui/app/providers/page.tsx (1)
24-24: LGTM! Consistent migration to Sheet-based UI.The changes successfully migrate from Dialog to Sheet components and apply consistent page-width constraints. The component wiring (show, onSave, onClose) remains intact.
Also applies to: 94-94, 113-124
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
13-23: LGTM! Sheet migration and keyIndex prop support added correctly.The migration from AddNewKeyDialog to AddNewKeySheet is consistent, and the keyIndex prop is now properly passed for per-key edit operations. The explicit onClick handler on AlertDialogCancel (lines 49-51) is redundant since the disabled prop already prevents interaction, but it's harmless and may improve code clarity.
Also applies to: 49-51, 78-83
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
1-46: LGTM! Clean Dialog-to-Sheet migration.The component has been successfully migrated from Dialog to Sheet with appropriate styling and structure. The onOpenChange handler directly calls onCancel, which is simple and effective.
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
154-154: Good practice: Explicit controlled component value.The explicit
value={field.value || ""}ensures the Input remains a controlled component even when field.value is undefined, preventing React warnings about switching between controlled and uncontrolled states.
1-176: LGTM! Successful Dialog-to-Sheet migration with proper form handling.The component has been successfully migrated from Dialog to Sheet with appropriate structure, styling, and form integration. The Sheet-based UI provides better UX for complex forms.
| <EmptyState isSocketConnected={isSocketConnected} error={error} /> | ||
| ) : ( | ||
| <div className="space-y-6"> | ||
| <div className="space-y-6 max-w-7xl mx-auto"> |
There was a problem hiding this comment.
Fix whitespace in className.
There appears to be a tab character between max-w-7xl and mx-auto instead of a regular space. This could cause formatting inconsistencies.
Apply this diff:
- <div className="space-y-6 max-w-7xl mx-auto">
+ <div className="space-y-6 mx-auto w-full max-w-7xl">📝 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.
| <div className="space-y-6 max-w-7xl mx-auto"> | |
| <div className="space-y-6 mx-auto w-full max-w-7xl"> |
🤖 Prompt for AI Agents
In ui/app/logs/page.tsx around line 352, the className string contains a tab
character between "max-w-7xl" and "mx-auto"; replace the tab with a regular
space so the class list reads "space-y-6 max-w-7xl mx-auto" to ensure consistent
whitespace and proper Tailwind class parsing.
| }); | ||
| }; | ||
|
|
||
| console.log(form.formState.isDirty); |
There was a problem hiding this comment.
Remove debug console.log statement.
This debug statement should be removed before merging to production.
Apply this diff to remove the debug statement:
- console.log(form.formState.isDirty);
-📝 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.
| console.log(form.formState.isDirty); |
🤖 Prompt for AI Agents
In ui/app/providers/views/providerKeyForm.tsx around line 81, remove the debug
console.log(form.formState.isDirty); statement — delete that line (or the entire
debug call) so no console output remains in production, then run lint/format to
ensure no leftover whitespace or unused variables warnings.
0536336 to
a3a156b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
ui/app/providers/views/modelProviderConfig.tsx (1)
46-46: Verify the behavioral change: accordion is now always collapsible.The accordion has been converted from an uncontrolled to a controlled component, which is a reasonable refactor. However, the
collapsibleprop appears to have changed fromcollapsible={showApiKeys}(conditional) tocollapsible={true}(always), meaning users can now collapse the accordion even when API keys are not required. Please confirm this is the intended behavior.Optional refactor: Simplify useEffect dependencies.
The useEffect dependencies
[showApiKeys, isCustomProvider]are derived fromprovider.name. Consider depending directly on the source:useEffect(() => { setAccordionValue(!showApiKeys || isCustomProvider ? "item-1" : undefined); -}, [showApiKeys, isCustomProvider]); +}, [provider.name]);This makes the dependency chain more explicit and easier to understand.
Also applies to: 58-64
ui/components/sidebar.tsx (1)
260-278: Improve type safety for sub-item rendering.The sub-item mapping uses
anytype for thesubItemparameter (line 311), which bypasses TypeScript's type checking.Consider defining a proper type for sub-items:
+interface SubItem { + title: string; + url: string; + icon: React.ComponentType<{ className?: string }>; + description: string; +} + interface DataTableProps { - item: (typeof items)[0]; + item: (typeof items)[0] & { subItems?: SubItem[] };Then update line 311:
-{item.subItems?.map((subItem: any) => { +{item.subItems?.map((subItem) => {framework/configstore/migrations.go (1)
600-634: Migration name doesn't reflect all changes.The migration ID is
"add_plugin_path_column"but it also adds theis_customcolumn (lines 611-613). This naming mismatch could cause confusion during maintenance.Consider renaming the migration to reflect both changes:
-ID: "add_plugin_path_column", +ID: "add_plugin_path_and_is_custom_columns",Or update the comment on line 600 to clarify:
-// migrationAddPluginPathColumn adds the path column to the plugin table +// migrationAddPluginPathColumn adds the path and is_custom columns to the plugin tableui/app/plugins/page.tsx (2)
10-12: Loading state is fetched but unused.Render a loader/skeleton while plugins load, and handle errors if exposed by the hook.
-const { data: plugins, isLoading } = useGetPluginsQuery(); +const { data: plugins, isLoading, isError, error } = useGetPluginsQuery(); @@ - return ( + if (isLoading) return <div className="text-sm text-muted-foreground">Loading plugins…</div>; + if (isError) return <div className="text-sm text-destructive">Failed to load plugins.</div>; + return (
14-20: Add a page heading for a11y/navigation.Provide a landmark title so screen readers can identify the page.
-<div className="mx-auto w-full max-w-7xl"> +<div className="mx-auto w-full max-w-7xl"> + <h1 className="sr-only">Plugins</h1>ui/app/providers/page.tsx (1)
113-124: Avoid timeout for provider selection after save.Using
setTimeout(300)is brittle. Prefer updating selection immediately ononSave, and let the sheet close independently (animations won’t require a delay).- onSave={(id) => { - setTimeout(() => { - setProvider(id); - }, 300); - setShowCustomProviderDialog(false); - }} + onSave={(id) => { + setProvider(id); + setShowCustomProviderDialog(false); + }}Optionally rename state to
showCustomProviderSheetfor clarity.ui/app/providers/views/modelProviderKeysTableView.tsx (2)
88-95: Add accessible label to the primary action.Give the “Add new key” button an
aria-labelfor clarity.-<Button +<Button aria-label="Add new API key" onClick={() => { handleAddKey(provider.keys.length); }} >
132-134: Ellipsis menu trigger: add an accessible label.Screen readers need a descriptive label for the actions menu button.
-<Button onClick={(e) => e.stopPropagation()} variant="ghost"> +<Button aria-label="Open key actions menu" onClick={(e) => e.stopPropagation()} variant="ghost">ui/app/layout.tsx (1)
41-43: Remove redundantcustom-scrollbarand ineffectivemx-autofrom main element.Verified: the parent
<div>withoverflow-autohandles scrolling, making thecustom-scrollbarclass on<main>redundant. Themx-autoon<main>is ineffective—all 14 pages consistently applymax-w-7xl(andmx-auto) at their own level, not relying on the layout's main container. Trimming these unused classes improves clarity.ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
154-154: Redundant value prop in Input field.The explicit
value={field.value || ""}is unnecessary because{...field}already spreads all field props includingvalue. The fallback to empty string is also redundant sincebase_urlis initialized to""in defaultValues (line 48).Apply this diff to simplify:
-<Input placeholder={"https://api.your-provider.com"} {...field} value={field.value || ""} /> +<Input placeholder="https://api.your-provider.com" {...field} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(11 hunks)ui/lib/types/plugins.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/config.go
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/app/scim/page.tsx
- ui/app/mcp-clients/page.tsx
- ui/app/guardrails/page.tsx
- ui/app/logs/page.tsx
- ui/app/alert-channels/page.tsx
- ui/app/user-groups/page.tsx
- ui/app/providers/dialogs/addNewKeySheet.tsx
- ui/app/adaptive-routing/page.tsx
- ui/app/providers/views/providerKeyForm.tsx
- ui/app/cluster/page.tsx
- ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx
- ui/app/virtual-keys/page.tsx
- ui/app/audit-logs/page.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
ui/components/ui/alertDialog.tsx (1)
AlertDialogCancel(85-85)ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/providers/views/modelProviderConfig.tsx (2)
ui/lib/types/config.ts (1)
isKnownProvider(15-17)ui/app/providers/views/utils.ts (1)
keysRequired(1-1)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
ui/components/sidebar.tsx (1)
ui/components/ui/sidebar.tsx (5)
SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)SidebarContent(598-598)SidebarGroup(600-600)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
ui/components/ui/input.tsx (1)
Input(15-69)
ui/app/providers/page.tsx (1)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
AddCustomProviderSheet(41-176)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (17)
ui/app/providers/views/modelProviderConfig.tsx (2)
47-47: LGTM: Typo fix improves code clarity.Good catch on the typo! The variable name
isCustomProvideris now consistent with its purpose and more readable than the previousisCustomProver.
52-52: LGTM: Good extraction for clarity.Extracting
keysRequired(provider.name)into a named constantshowApiKeysimproves readability and makes the intent clearer throughout the component.ui/app/logs/views/logsTable.tsx (1)
81-82: LGTM! Vertical sizing adjustment aligns with layout standardization.The height increase from
calc(100vh-20rem)tocalc(100vh-16.5rem)provides 3.5rem additional vertical space, which aligns with the PR's broader layout changes that add centered, max-width containers across pages.framework/configstore/tables/plugin.go (1)
20-20: LGTM! New IsCustom field properly integrated.The
IsCustomfield addition is well-structured and aligns with the database migration inframework/configstore/migrations.go(lines 611-613) that adds theis_customcolumn. The field is properly exported with a JSON tag for API serialization.ui/components/sidebar.tsx (3)
74-92: LGTM! Observability restructured with logical sub-navigation.The conversion of Observability into a parent item with "Logs" and "Connectors" sub-items provides better organization. The icon choices (Telescope for parent, Logs and ChevronsLeftRightEllipsis for children) are appropriate.
106-136: Well-structured Governance section with comprehensive sub-items.The new Governance section effectively groups access control features (Virtual Keys, Users & Groups, User Provisioning, Audit Logs) under a single parent with the Landmark icon. The structure improves discoverability and navigation hierarchy.
139-143: ✓ Verified: /plugins route existsThe Plugins menu item is properly configured. The
/pluginsroute page component has been confirmed atui/app/plugins/page.tsx.framework/configstore/migrations.go (1)
611-625: LGTM! Migration correctly handles both columns in forward and rollback.The migration properly adds both
pathandis_customcolumns together in the Migrate step and removes both in the Rollback step, maintaining database consistency.ui/app/observability/page.tsx (1)
4-8: LGTM! Layout wrapper aligns with PR-wide standardization.The
max-w-7xl mx-auto w-fullwrapper provides consistent centering and width constraints, matching the pattern applied across other pages (logs, audit-logs, config, etc.) in this PR.ui/app/config/page.tsx (1)
64-64: LGTM! Layout constraint added for consistency.Adding
max-w-7xl mx-autoto the config page maintains visual consistency with other pages updated in this PR while preserving the existing two-column layout structure.ui/lib/types/plugins.ts (1)
10-11: LGTM! Frontend types properly synchronized with backend.The
isCustomandpathfields correctly match the backendTablePluginstruct inframework/configstore/tables/plugin.go(lines 16, 20), including JSON tag naming conventions.ui/app/providers/page.tsx (2)
24-24: Switch to Sheet looks good.Import aligns with the default export in the new component. No issues spotted.
94-94: Layout wrapper update LGTM.Constrained, centered content improves consistency with the new global layout.
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
49-51: Cancel handler improvement LGTM.
AlertDialogCancelnow resets local state; good UX and state hygiene.
78-83: Sheet migration LGTM.
AddNewKeySheetwiring (show/cancel/provider/keyIndex) mirrors prior dialog behavior.ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
5-5: LGTM! Clean Dialog → Sheet conversion.The import changes and component renaming correctly implement the Dialog to Sheet migration pattern. The changes align with the broader UI refactoring mentioned in the PR objectives.
Also applies to: 41-41
163-170: LGTM! Appropriate footer button alignment.The nested div with
ml-autocorrectly aligns the Cancel/Add buttons to the right side of the SheetFooter while maintaining consistent spacing. This is a standard pattern for sheet footers in shadcn/ui.
| // Mark plugin as custom if path is not empty | ||
| if plugin.Path != nil && *plugin.Path != "" { | ||
| plugin.IsCustom = true | ||
| } |
There was a problem hiding this comment.
Derive IsCustom deterministically (also when path is cleared).
Currently only sets true; if a plugin’s path becomes empty, IsCustom may stay stale. Compute from Path every time.
-// Mark plugin as custom if path is not empty
-if plugin.Path != nil && *plugin.Path != "" {
- plugin.IsCustom = true
-}
+// Derive IsCustom from Path (trimmed) on every write
+if plugin.Path != nil && strings.TrimSpace(*plugin.Path) != "" {
+ plugin.IsCustom = true
+} else {
+ plugin.IsCustom = false
+}Add import:
import (
"context"
"encoding/json"
"errors"
"fmt"
+ "strings"
)📝 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.
| // Mark plugin as custom if path is not empty | |
| if plugin.Path != nil && *plugin.Path != "" { | |
| plugin.IsCustom = true | |
| } | |
| // Derive IsCustom from Path (trimmed) on every write | |
| if plugin.Path != nil && strings.TrimSpace(*plugin.Path) != "" { | |
| plugin.IsCustom = true | |
| } else { | |
| plugin.IsCustom = false | |
| } |
🤖 Prompt for AI Agents
In framework/configstore/rdb.go around lines 854 to 857, the code only sets
plugin.IsCustom true when Path is non-empty, leaving it stale if Path is later
cleared; change the logic to deterministically compute IsCustom from Path on
every update (e.g., set plugin.IsCustom to the boolean expression (plugin.Path
!= nil && *plugin.Path != "")) so it becomes false when Path is cleared; update
any related code paths accordingly and add any necessary import if you reference
helpers for nil/empty checks.
| // Mark plugin as custom if path is not empty | ||
| if plugin.Path != nil && *plugin.Path != "" { | ||
| plugin.IsCustom = true | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Apply the same IsCustom derivation in UpdatePlugin.
Mirror the deterministic computation here as well to avoid divergence.
-// Mark plugin as custom if path is not empty
-if plugin.Path != nil && *plugin.Path != "" {
- plugin.IsCustom = true
-}
+// Derive IsCustom from Path consistently
+if plugin.Path != nil && strings.TrimSpace(*plugin.Path) != "" {
+ plugin.IsCustom = true
+} else {
+ plugin.IsCustom = false
+}📝 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.
| // Mark plugin as custom if path is not empty | |
| if plugin.Path != nil && *plugin.Path != "" { | |
| plugin.IsCustom = true | |
| } | |
| // Derive IsCustom from Path consistently | |
| if plugin.Path != nil && strings.TrimSpace(*plugin.Path) != "" { | |
| plugin.IsCustom = true | |
| } else { | |
| plugin.IsCustom = false | |
| } |
🤖 Prompt for AI Agents
In framework/configstore/rdb.go around lines 873-877, the code sets
plugin.IsCustom = true when plugin.Path is non-nil and non-empty; apply the same
deterministic derivation inside UpdatePlugin so updates can't diverge: in the
UpdatePlugin implementation recompute plugin.IsCustom = (plugin.Path != nil &&
*plugin.Path != "") before saving (and handle the false case explicitly),
ensuring the updated record persists the derived IsCustom value consistently.
| Name string `json:"name"` | ||
| Enabled bool `json:"enabled"` | ||
| Config map[string]any `json:"config"` | ||
| Path *string `json:"path"` |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Path field usage in create handler.
The Path field is added to CreatePluginRequest but is not used in the createPlugin handler (lines 134-138). The Path is not passed to h.configStore.CreatePlugin() or to h.pluginsLoader.ReloadPlugin() at line 153.
Check if Path handling is implemented elsewhere or if this is incomplete:
🏁 Script executed:
#!/bin/bash
# Description: Check if Path is used in plugin creation logic
# Check CreatePlugin implementation in configstore
ast-grep --pattern 'func $_ CreatePlugin($$$) $_ {
$$$
}'
# Check if Path is passed anywhere in the handlers
rg -n "request\.Path" transports/bifrost-http/handlers/plugins.go -A 3 -B 3
# Check rdb.go for Path handling in CreatePlugin
rg -n "Path" framework/configstore/rdb.go -A 3 -B 3 | head -30Length of output: 858
I need to examine the createPlugin handler implementation to understand the complete flow of how the Path field is handled.
🏁 Script executed:
#!/bin/bash
# Description: Examine the createPlugin handler and how Path is handled
# Show the createPlugin handler
rg -n "func.*createPlugin" transports/bifrost-http/handlers/plugins.go -A 30
# Search for all Path references in the entire handler file
rg -n "Path" transports/bifrost-http/handlers/plugins.go -B 2 -A 2
# Check if there's a mapping or assignment from request to the plugin object
rg -n "CreatePluginRequest\|CreatePlugin" transports/bifrost-http/handlers/plugins.go -B 3 -A 3Length of output: 1303
Path field is not extracted from request in createPlugin handler.
The review comment is verified as correct. The CreatePluginRequest struct includes the Path field (line 44), but the createPlugin handler (lines 114-142) does not extract it from the request. When calling h.configStore.CreatePlugin() at lines 134-138, only Name, Enabled, and Config are passed to the TablePlugin struct—Path is omitted entirely.
However, the underlying configstore implementation in rdb.go (lines 855, 874) expects and uses the Path field to determine if a plugin should be marked as custom. This creates an incomplete implementation where the field is defined but never passed through the handler layer.
The Path field must be extracted from the request and included in the TablePlugin struct passed to configstore.CreatePlugin() at lines 134-138.
🤖 Prompt for AI Agents
transports/bifrost-http/handlers/plugins.go lines ~44 and 114-142: the
CreatePluginRequest defines Path at line 44 but the createPlugin handler (lines
~114-142) never reads that field and omits it when calling
h.configStore.CreatePlugin() (call around lines 134-138). Update the handler to
extract req.Path from the parsed request and set the TablePlugin.Path field when
constructing the object passed to h.configStore.CreatePlugin(); preserve
existing behavior for nil/empty (pass nil or empty string as-is) and include any
necessary validation or sanitization consistent with other fields before calling
CreatePlugin.
| Read our{" "} | ||
| <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> | ||
| documentation | ||
| </Link>{" "} | ||
| to learn more about plugins. | ||
| </div> |
There was a problem hiding this comment.
External link: add rel to avoid reverse‑tabnabbing.
Add rel="noopener noreferrer" with target="_blank".
-<Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank">
+<Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">📝 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.
| Read our{" "} | |
| <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> | |
| documentation | |
| </Link>{" "} | |
| to learn more about plugins. | |
| </div> | |
| Read our{" "} | |
| <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer"> | |
| documentation | |
| </Link>{" "} | |
| to learn more about plugins. | |
| </div> |
🤖 Prompt for AI Agents
In ui/app/plugins/page.tsx around lines 36 to 41, the external Link opens with
target="_blank" but is missing rel attributes; add rel="noopener noreferrer" to
the Link component (alongside the existing target="_blank") so the rendered
anchor includes rel="noopener noreferrer" to prevent reverse-tabnabbing.
| <DialogDescription>Enter the details of your custom provider.</DialogDescription> | ||
| </DialogHeader> | ||
| <Sheet open={show} onOpenChange={(open) => !open && onClose()}> | ||
| <SheetContent className="custom-scrollbar dark:bg-card bg-white py-4 min-w-[600px]"> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify responsive behavior with fixed minimum width.
The min-w-[600px] could cause horizontal overflow on smaller viewports (tablets in portrait, mobile landscape). Consider using responsive breakpoints or a max-width constraint instead.
Run this script to check if other Sheet usages in the codebase have similar fixed widths:
🏁 Script executed:
#!/bin/bash
# Description: Check for fixed width patterns in Sheet components across the codebase
# Search for SheetContent with min-w or w- classes
rg -n "SheetContent.*min-w-\[" --type tsx --type ts -C2
# Also check for responsive patterns in Sheet components
rg -n "SheetContent.*className" --type tsx --type ts | head -20Length of output: 55
🏁 Script executed:
#!/bin/bash
# Description: Check for fixed width patterns in Sheet components across the codebase
# Using corrected ripgrep syntax with glob patterns
# Search for SheetContent with min-w or w- classes using glob patterns
rg -n "SheetContent.*min-w-\[" --include="*.tsx" --include="*.ts" -C2
# Check all SheetContent usages to see width patterns
echo "=== All SheetContent className usages ==="
rg -n "SheetContent.*className" --include="*.tsx" --include="*.ts" -B1 -A1
# Also look at the specific file mentioned in the review
echo "=== Checking addNewCustomProviderSheet.tsx ==="
rg -n "SheetContent" ui/app/providers/dialogs/addNewCustomProviderSheet.tsx -B2 -A2Length of output: 1728
🏁 Script executed:
#!/bin/bash
# Description: Find all SheetContent usages to compare width patterns
# Find all files containing SheetContent
echo "=== All SheetContent usages across codebase ==="
fd -e tsx -e ts | xargs rg -n "SheetContent" -B1 -A1 | head -100
# Also check what the Sheet component itself expects/provides
echo ""
echo "=== Sheet component definition ==="
fd -name "sheet*" -e tsx -e ts | head -5Length of output: 6780
🏁 Script executed:
#!/bin/bash
# Description: Examine the Sheet component definition to understand constraints
# View the sheet component source
cat ui/components/ui/sheet.tsxLength of output: 3791
Inconsistent responsive width: use responsive max-width instead of fixed minimum.
The min-w-[600px] causes horizontal overflow on tablets and mobile devices. Replace with the responsive pattern used throughout the codebase: w-full flex-col overflow-x-hidden bg-white p-8 sm:max-w-2xl.
Note: addNewKeySheet.tsx (line 21) has the same issue and should be updated similarly.
🤖 Prompt for AI Agents
In ui/app/providers/dialogs/addNewCustomProviderSheet.tsx around line 98,
replace the fixed CSS class min-w-[600px] which causes horizontal overflow on
smaller screens with the responsive pattern used elsewhere: use full width with
responsive max-width and padding (e.g. w-full flex-col overflow-x-hidden
bg-white p-8 sm:max-w-2xl) to ensure it scales on mobile/tablet; also update
addNewKeySheet.tsx (around line 21) the same way to keep consistency.
| useEffect(() => { | ||
| const newExpandedItems = new Set<string>(); | ||
|
|
||
| enterpriseItems.forEach(item => { | ||
| if ('subItems' in item && item.subItems && Array.isArray(item.subItems)) { | ||
| const hasActiveSubItem = item.subItems.some((subItem: { url: string }) => pathname.startsWith(subItem.url)); | ||
| if (hasActiveSubItem) { | ||
| newExpandedItems.add(item.title); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
|
|
||
| if (newExpandedItems.size > 0) { | ||
| setExpandedItems(prev => new Set([...prev, ...newExpandedItems])); | ||
| setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); | ||
| } | ||
| }, [pathname]); |
There was a problem hiding this comment.
Auto-expansion logic appears incomplete.
The auto-expand effect creates a newExpandedItems Set but never populates it. The logic that should identify which items to auto-expand based on active subitems is missing.
Apply this diff to restore auto-expansion when subitems are active:
useEffect(() => {
const newExpandedItems = new Set<string>();
+ items.forEach((item) => {
+ if (item.subItems?.some((subItem) => pathname.startsWith(subItem.url))) {
+ newExpandedItems.add(item.title);
+ }
+ });
if (newExpandedItems.size > 0) {
setExpandedItems((prev) => new Set([...prev, ...newExpandedItems]));
}
}, [pathname]);📝 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.
| useEffect(() => { | |
| const newExpandedItems = new Set<string>(); | |
| enterpriseItems.forEach(item => { | |
| if ('subItems' in item && item.subItems && Array.isArray(item.subItems)) { | |
| const hasActiveSubItem = item.subItems.some((subItem: { url: string }) => pathname.startsWith(subItem.url)); | |
| if (hasActiveSubItem) { | |
| newExpandedItems.add(item.title); | |
| } | |
| } | |
| }); | |
| if (newExpandedItems.size > 0) { | |
| setExpandedItems(prev => new Set([...prev, ...newExpandedItems])); | |
| setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); | |
| } | |
| }, [pathname]); | |
| useEffect(() => { | |
| const newExpandedItems = new Set<string>(); | |
| items.forEach((item) => { | |
| if (item.subItems?.some((subItem) => pathname.startsWith(subItem.url))) { | |
| newExpandedItems.add(item.title); | |
| } | |
| }); | |
| if (newExpandedItems.size > 0) { | |
| setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); | |
| } | |
| }, [pathname]); |
🤖 Prompt for AI Agents
In ui/components/sidebar.tsx around lines 404 to 410, the effect creates a
newExpandedItems Set but never fills it, so auto-expansion for active subitems
is missing; update the effect to iterate your sidebar data (e.g., sidebarItems
or nav structure) and add the parent item IDs to newExpandedItems when any of
their child/subitem paths match the current pathname (or pathname startsWith the
child path), then call setExpandedItems(prev => new Set([...prev,
...newExpandedItems])) as already done; ensure you handle edge cases (no
children, trailing slashes) and keep the dependency on pathname.
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 (4)
framework/configstore/migrations.go (1)
607-614: Critical: Independent column checks needed to prevent schema inconsistency.The
is_customcolumn addition is nested inside thepathcolumn existence check. If thepathcolumn already exists (e.g., from a partial migration or earlier version), theis_customcolumn won't be added, creating schema inconsistency across installations.This will cause runtime failures when the application expects the
is_customcolumn but it's missing.Apply this diff to check each column independently:
if !migrator.HasColumn(&tables.TablePlugin{}, "path") { if err := migrator.AddColumn(&tables.TablePlugin{}, "path"); err != nil { return err } + } + if !migrator.HasColumn(&tables.TablePlugin{}, "is_custom") { if err := migrator.AddColumn(&tables.TablePlugin{}, "is_custom"); err != nil { return err } }transports/bifrost-http/handlers/plugins.go (2)
134-138: Critical: Path field not being passed to database.The
Pathfield is accepted inCreatePluginRequest(line 44) but is not included when creating theTablePluginat lines 134-138. This means the Path will always be nil in the database, andIsCustomwill never be set to true in the creation flow.Apply this diff to pass the Path field:
if err := h.configStore.CreatePlugin(ctx, &configstoreTables.TablePlugin{ Name: request.Name, Enabled: request.Enabled, Config: request.Config, + Path: request.Path, }); err != nil {
152-153: Pass Path to plugin loader.The
ReloadPlugincall should include the path from the request to properly load custom plugins.Apply this diff:
-if err := h.pluginsLoader.ReloadPlugin(ctx, request.Name, nil, request.Config); err != nil { +if err := h.pluginsLoader.ReloadPlugin(ctx, request.Name, request.Path, request.Config); err != nil {ui/app/plugins/page.tsx (1)
10-47: Missing loading and error states.The component fetches plugin data but doesn't display loading state or handle errors, which could lead to poor UX when the API is slow or fails.
Apply this diff to add loading and error handling:
export default function PluginsPage() { - const { data: plugins, isLoading } = useGetPluginsQuery(); + const { data: plugins, isLoading, error } = useGetPluginsQuery(); const customPlugins = useMemo(() => plugins?.filter((plugin) => plugin.isCustom), [plugins]); + if (isLoading) { + return ( + <div className="mx-auto w-full max-w-7xl"> + <div className="flex items-center justify-center p-8"> + <span className="text-muted-foreground">Loading plugins...</span> + </div> + </div> + ); + } + + if (error) { + return ( + <div className="mx-auto w-full max-w-7xl"> + <div className="flex items-center justify-center p-8"> + <span className="text-destructive">Failed to load plugins</span> + </div> + </div> + ); + } + return (
♻️ Duplicate comments (2)
ui/app/providers/views/providerKeyForm.tsx (1)
81-81: Debug console.log still present.This debug statement was already flagged in previous reviews and marked as addressed in commit 0536336, but it still appears in the code. Please remove it before merging.
ui/components/sidebar.tsx (1)
146-146: Incomplete rename creates user-facing inconsistency.Per the previous review, renaming "MCP Clients" to "MCP Gateway" only in the sidebar creates inconsistency with the rest of the codebase (table headers, documentation, API hooks, etc. still use "MCP Clients"). Either complete the rename across all user-facing surfaces or revert to "MCP Clients" for consistency.
🧹 Nitpick comments (2)
framework/configstore/rdb.go (2)
854-857: Consider explicit false assignment for IsCustom.The logic correctly sets
IsCustom = truewhen Path exists, but relies on Go's zero value for the false case. While functionally correct, explicitly settingIsCustom = falsewhen Path is nil/empty would make the intent clearer.Apply this diff to make the logic explicit:
// Mark plugin as custom if path is not empty if plugin.Path != nil && *plugin.Path != "" { plugin.IsCustom = true +} else { + plugin.IsCustom = false }
854-857: Consider extracting duplicate IsCustom logic.The same logic for setting
IsCustombased onPathappears in bothCreatePluginandUpdatePlugin. Consider extracting this to a helper method onTablePluginto follow DRY principles.Add a helper method in
framework/configstore/tables/plugin.go:// SetIsCustomFromPath updates IsCustom based on whether Path is set func (p *TablePlugin) SetIsCustomFromPath() { p.IsCustom = p.Path != nil && *p.Path != "" }Then use it in both locations:
-// Mark plugin as custom if path is not empty -if plugin.Path != nil && *plugin.Path != "" { - plugin.IsCustom = true -} +plugin.SetIsCustomFromPath()Also applies to: 873-877
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(11 hunks)ui/lib/types/plugins.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/config.go
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/app/observability/page.tsx
- ui/app/guardrails/page.tsx
- ui/app/logs/views/logsTable.tsx
- ui/app/user-groups/page.tsx
- ui/app/scim/page.tsx
- ui/app/alert-channels/page.tsx
- ui/app/mcp-clients/page.tsx
- ui/app/providers/page.tsx
- ui/app/providers/dialogs/addNewKeySheet.tsx
- ui/app/logs/page.tsx
- ui/app/audit-logs/page.tsx
- ui/app/virtual-keys/page.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
ui/app/providers/views/providerKeyForm.tsx (2)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)ui/components/ui/tooltip.tsx (1)
TooltipContent(43-43)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/adaptive-routing/page.tsx (1)
ui/app/_fallbacks/enterprise/components/adaptive-routing/adaptiveRoutingView.tsx (1)
AdaptiveRoutingView(4-16)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
ui/components/sidebar.tsx (1)
ui/components/ui/sidebar.tsx (5)
SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)SidebarContent(598-598)SidebarGroup(600-600)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/app/providers/views/modelProviderConfig.tsx (2)
ui/lib/types/config.ts (1)
isKnownProvider(15-17)ui/app/providers/views/utils.ts (1)
keysRequired(1-1)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
ui/components/ui/sheet.tsx (6)
Sheet(90-90)SheetContent(90-90)SheetHeader(90-90)SheetTitle(90-90)SheetDescription(90-90)SheetFooter(90-90)ui/components/ui/input.tsx (1)
Input(15-69)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (20)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (2)
1-2: LGTM! Clean imports.The Siren icon is semantically appropriate for alert channels, and the relative import path correctly references the ContactUsView component.
4-16: LGTM! Well-structured enterprise fallback component.The implementation is clean and follows the expected pattern for enterprise feature gates. All required props are correctly passed to ContactUsView, the content is clear and appropriate, and the layout ensures proper centering and spacing.
ui/app/cluster/page.tsx (1)
4-8: LGTM! Consistent layout pattern applied.The centered max-width wrapper aligns with the layout standardization across the PR.
ui/app/adaptive-routing/page.tsx (1)
4-8: LGTM! Consistent layout pattern applied.Same centered max-width wrapper as other pages in this PR.
ui/app/config/page.tsx (1)
64-64: LGTM! Layout standardization applied.Adding max-width constraint aligns with the consistent layout approach across the PR.
ui/app/providers/views/providerKeyForm.tsx (2)
53-61: LGTM! Well-structured tooltip logic.The memoized callback efficiently computes tooltip content based on form state with correct dependencies.
88-88: LGTM! Typo fix and improved conditional rendering.The className typo correction and conditional TooltipContent rendering improve code quality.
Also applies to: 107-107
ui/app/providers/views/modelProviderConfig.tsx (1)
46-60: LGTM! Improved accordion control and typo fix.The controlled accordion state with proper initialization based on provider type and API key requirements is well-implemented. The variable rename from
isCustomProvertoisCustomProvidercorrects a typo.Also applies to: 64-64
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
23-23: LGTM! Clean migration to Sheet component.The transition from
AddNewKeyDialogtoAddNewKeySheetwith properkeyIndexprop passing supports the editing functionality correctly.Also applies to: 78-83
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
5-5: LGTM! Complete migration to Sheet pattern.The component has been successfully migrated from Dialog to Sheet, maintaining all functionality while adopting the consistent Sheet-based UI pattern used throughout the PR.
Also applies to: 41-41, 97-174
framework/configstore/tables/plugin.go (1)
20-20: LGTM! IsCustom field properly integrated.The new
IsCustomboolean field is correctly added with appropriate JSON serialization. The field integrates well with the existing serialization hooks and will be automatically persisted by GORM asis_customcolumn.ui/app/layout.tsx (1)
41-41: Layout refactor aligns with page-level container pattern.Switching from fixed-width responsive classes to full-width flexible layout is consistent with the PR's pattern of applying centered max-width containers at the page level (e.g.,
mx-auto max-w-7xlin various pages).ui/lib/types/plugins.ts (1)
10-11: LGTM! Type definitions match backend schema.The new
isCustomandpathfields correctly align with the backendTablePluginstruct, using appropriate TypeScript types (boolean and nullable string).transports/bifrost-http/handlers/plugins.go (1)
44-44: Path field added to request type.The new
Pathfield enables custom plugin installations via path specification.ui/app/plugins/page.tsx (1)
10-11: Good use of memoization for custom plugin filtering.The
useMemohook efficiently filters plugins by theisCustomflag, preventing unnecessary re-computations.ui/components/sidebar.tsx (5)
74-92: Well-structured Observability section with subItems.The restructured Observability navigation with Logs and Connectors as subItems provides better organization. The icon choices (Telescope for parent, Logs and ChevronsLeftRightEllipsis for children) are appropriate.
106-136: Governance section properly organized.The new Governance section with Virtual Keys, Users & Groups, User Provisioning, and Audit Logs as subItems follows a logical structure. The Landmark icon appropriately represents governance concepts.
139-143: Plugins navigation item added correctly.The new Plugins menu item is properly structured with appropriate icon (Puzzle) and description. The
/pluginsroute corresponds to the page component added in this PR.
260-336: SubItem implementation is robust.The enhanced
SidebarItemcomponent properly handles:
- SubItem expansion/collapse with chevron rotation
- Active state detection for both parent and sub-items
- Click handling that distinguishes between navigation and expansion
- Proper styling for hover and active states
The implementation follows good React patterns with appropriate state management.
310-333: SubItem rendering follows consistent styling patterns.The
SidebarMenuSubrendering logic correctly:
- Applies indentation with left border
- Renders sub-item icons and titles
- Handles active states
- Uses proper spacing and transitions
The use of
anytype forsubItemat line 311 is acceptable given the dynamic nature of the items array.
akshaydeo
force-pushed
the
10-16-dynamic_plugin_architecture
branch
from
ddd7b35 to
bce70c7
Compare
akshaydeo
force-pushed
the
10-20-dynamic-plugins-ui
branch
from
a3a156b to
de194ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/migrations.go (1)
611-625: Addis_customvia a fresh migration, not by editing this one.
add_plugin_path_columnhas already been executed everywhere and its ID is stored in the migration ledger. Updating the existing migration means live databases will not rerun this code, leavingconfig_plugins.is_customabsent while the rest of the stack now expects it—resulting in runtime failures. Even if the migration were forced to rerun, the guardif !migrator.HasColumn(&tables.TablePlugin{}, "path")short-circuits oncepathexists, sois_customwould still never be created. Please revert the new column work here and introduce a brand-new migration (with a new ID) that independently checks for/createsis_customand drops it on rollback.- if err := migrator.AddColumn(&tables.TablePlugin{}, "is_custom"); err != nil { - return err - } @@ - if err := migrator.DropColumn(&tables.TablePlugin{}, "is_custom"); err != nil { - return err - }Add a follow-up migration (hook it in
triggerMigrations) like:func migrationAddPluginIsCustomColumn(ctx context.Context, db *gorm.DB) error { m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ ID: "add_plugin_is_custom_column", Migrate: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() if !migrator.HasColumn(&tables.TablePlugin{}, "is_custom") { if err := migrator.AddColumn(&tables.TablePlugin{}, "is_custom"); err != nil { return err } } return nil }, Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) return tx.Migrator().DropColumn(&tables.TablePlugin{}, "is_custom") }, }}) return wrapMigrateError(m.Migrate()) }
♻️ Duplicate comments (6)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
98-170: Fix the sheet width for small screens.Line 99 still forces
min-w-[600px], which overflows on phones and small tablets. Please switch to a responsive pattern (full width on mobile with a max width atsm+) so the sheet renders without horizontal scrolling.- <SheetContent className="custom-scrollbar dark:bg-card bg-white py-4 min-w-[600px]"> + <SheetContent className="custom-scrollbar dark:bg-card bg-white py-4 w-full sm:min-w-[600px] sm:max-w-[600px]">ui/app/providers/dialogs/addNewKeySheet.tsx (1)
20-44: Make the key sheet responsive.Line 21 keeps
min-w-[600px], so the sheet still breaks on sub-600px viewports. Please mirror the responsive width pattern (mobilew-full, constrained width onsmand up) to prevent overflow.- <Sheet open={show} onOpenChange={onCancel}> - <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> + <Sheet open={show} onOpenChange={onCancel}> + <SheetContent className="custom-scrollbar bg-white dark:bg-card w-full sm:min-w-[600px] sm:max-w-[600px] py-4">ui/components/sidebar.tsx (2)
139-143: Rename consistency: “MCP Gateway” vs legacy “MCP Clients” artifacts.URL remains /mcp-clients, and the custom SVG still has “MCP clients icon” title/aria-label. Either complete the rename (route, component titles, a11y labels) or revert the sidebar label for consistency.
- aria-label="MCP clients icon" - <title>MCP clients icon</title> + aria-label="MCP gateway icon" + <title>MCP gateway icon</title>
397-403: Auto‑expand is broken: effect never adds parents with active subitems.Populate newExpandedItems based on pathname and subItems.
useEffect(() => { const newExpandedItems = new Set<string>(); + items.forEach((it) => { + if (it.subItems?.some((s) => pathname.startsWith(s.url))) { + newExpandedItems.add(it.title); + } + }); if (newExpandedItems.size > 0) { setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); } }, [pathname]);ui/app/plugins/page.tsx (2)
22-33: “Install New Plugin” button is inert; wire it or link docs.Make it navigate (e.g., /plugins/new) or open docs. Remove preventDefault/stopPropagation.
- <Button - variant="outline" - size="sm" - className="w-full justify-start" - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - }} - > - <PlusIcon className="h-4 w-4" /> - <div className="text-xs">Install New Plugin</div> - </Button> + <Link href="/plugins/new"> + <Button variant="outline" size="sm" className="w-full justify-start"> + <PlusIcon className="h-4 w-4" /> + <div className="text-xs">Install New Plugin</div> + </Button> + </Link>
36-40: External link: add rel to prevent reverse‑tabnabbing.Include rel="noopener noreferrer" with target="_blank".
- <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> + <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">
🧹 Nitpick comments (3)
ui/app/plugins/page.tsx (1)
10-12: Handle loading/empty states.Currently renders an empty list while fetching. Show a loader/skeleton or an empty-state message.
- const { data: plugins, isLoading } = useGetPluginsQuery(); + const { data: plugins, isLoading } = useGetPluginsQuery(); const customPlugins = useMemo(() => plugins?.filter((plugin) => plugin.isCustom), [plugins]); - return ( + if (isLoading) return <div className="p-4 text-sm text-muted-foreground">Loading plugins…</div>; + const hasCustom = (customPlugins?.length ?? 0) > 0; + return ( <div className="mx-auto w-full max-w-7xl"> <div className="flex w-[250px] flex-col gap-2 pb-10"> <div className="rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20"> <div className="mb-4"> - {customPlugins?.map((plugin) => ( + {!hasCustom && <div className="text-xs text-muted-foreground">No custom plugins yet.</div>} + {hasCustom && customPlugins!.map((plugin) => ( <div key={plugin.name}>{plugin.name}</div> ))}ui/components/sidebar.tsx (2)
73-91: Observability item/URLs: confirm intended default and active states.Top-level “Observability” points to /logs while a “Logs” subitem also points to /logs; ensure this duplication is intentional. If the intended hub is /observability, consider setting the parent url to /observability and keep “Logs” as a subitem.
- { title: "Observability", url: "/logs", ... subItems: [ + { title: "Observability", url: "/observability", ... subItems: [ { title: "Logs", url: "/logs", ... }, { title: "Connectors", url: "/observability", ... }, ] }
303-326: Type subItems instead of using any.Avoid any for subItem; define a SubItem type and use it in items and map.
- { item.subItems?.map((subItem: any) => { + type SubItem = { title: string; url: string; icon?: React.ComponentType<{ className?: string }>; description?: string }; + // items: Array<{ title: string; url: string; icon: any; description?: string; subItems?: SubItem[] }> + { item.subItems?.map((subItem: SubItem) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(11 hunks)ui/lib/types/plugins.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- ui/app/providers/views/providerKeyForm.tsx
- ui/app/audit-logs/page.tsx
- framework/configstore/tables/plugin.go
- ui/app/layout.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
- ui/app/providers/views/modelProviderConfig.tsx
- ui/lib/types/plugins.ts
- ui/app/scim/page.tsx
- transports/bifrost-http/handlers/plugins.go
- ui/app/providers/page.tsx
- ui/app/logs/page.tsx
- ui/app/virtual-keys/page.tsx
- ui/app/logs/views/logsTable.tsx
- ui/app/user-groups/page.tsx
- ui/app/cluster/page.tsx
- ui/app/config/page.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/observability/page.tsx (1)
ui/app/observability/views/observabilityView.tsx (1)
ObservabilityView(67-168)
ui/app/adaptive-routing/page.tsx (1)
ui/app/_fallbacks/enterprise/components/adaptive-routing/adaptiveRoutingView.tsx (1)
AdaptiveRoutingView(4-16)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
ui/components/ui/input.tsx (1)
Input(15-69)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/components/sidebar.tsx (1)
ui/components/ui/sidebar.tsx (5)
SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)SidebarContent(598-598)SidebarGroup(600-600)
ui/app/alert-channels/page.tsx (1)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
AlertChannelsView(4-16)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
ui/app/adaptive-routing/page.tsx (1)
4-8: LGTM: layout wrapper added.Centering with max-w-7xl is consistent with other pages.
ui/app/guardrails/page.tsx (1)
4-8: LGTM: layout wrapper added.Matches the app-wide container pattern.
ui/app/mcp-clients/page.tsx (1)
29-31: LGTM: width-constrained wrapper.No functional changes; table still receives a safe fallback array.
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 (2)
ui/components/sidebar.tsx (2)
268-271: Governance gating bypass: sub-items still navigate when disabled.Clicking a sub-item routes even if governance is disabled. Block navigation and visually disable sub-items when
!isAllowed.const handleSubItemClick = (url: string) => { - router.push(url); + if (!isAllowed) return; + router.push(url); }; @@ - <SidebarMenuSubButton - className={`h-7 cursor-pointer rounded-md px-2 transition-all duration-200 ${ - isSubItemActive - ? "bg-sidebar-accent text-primary font-medium" - : "hover:bg-sidebar-accent hover:text-accent-foreground text-slate-500 dark:text-zinc-400" - }`} - onClick={() => handleSubItemClick(subItem.url)} - > + <SidebarMenuSubButton + aria-disabled={!isAllowed || undefined} + className={`h-7 rounded-md px-2 transition-all duration-200 ${ + isSubItemActive + ? "bg-sidebar-accent text-primary font-medium" + : "hover:bg-sidebar-accent hover:text-accent-foreground text-slate-500 dark:text-zinc-400" + } ${!isAllowed ? "cursor-not-allowed opacity-50 pointer-events-none" : "cursor-pointer"}`} + onClick={() => handleSubItemClick(subItem.url)} + >Also applies to: 304-320
441-447: External link: add rel with target="_blank".Prevent reverse‑tabnabbing on release notes link.
-<Link +<Link href={`https://docs.getbifrost.ai/changelogs/${latestRelease.name}`} target="_blank" - className="text-primary mt-auto pb-1 font-medium underline" + rel="noopener noreferrer" + className="text-primary mt-auto pb-1 font-medium underline" >
♻️ Duplicate comments (6)
ui/app/logs/page.tsx (1)
352-352: Replace the tab separator in the Tailwind class listThere’s still a tab character between
max-w-7xlandmx-auto; please swap it for a normal space to keep Tailwind class parsing consistent.- <div className="space-y-6 max-w-7xl mx-auto"> + <div className="space-y-6 max-w-7xl mx-auto">ui/app/providers/views/providerKeyForm.tsx (1)
81-81: Remove debug logging before shipping.This
console.logreintroduces the debug output we asked to drop earlier; please delete it so production logs stay clean.- console.log(form.formState.isDirty);ui/components/sidebar.tsx (2)
62-66: ARIA text still says “MCP clients”; update to “MCP gateway”.Aligns with the sidebar label rename.
- aria-label="MCP clients icon" + aria-label="MCP gateway icon" ... - <title>MCP clients icon</title> + <title>MCP gateway icon</title>
397-403: Auto‑expand logic incomplete; never populates the set.Populate
newExpandedItemsfor parents whose subItem matches the current path.useEffect(() => { const newExpandedItems = new Set<string>(); + items.forEach((item) => { + if (item.subItems?.some((sub) => pathname.startsWith(sub.url))) { + newExpandedItems.add(item.title); + } + }); if (newExpandedItems.size > 0) { setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); } }, [pathname]);ui/app/plugins/page.tsx (2)
22-33: “Install New Plugin” button is inert; wire it or make it a link.Route to a creation flow or docs instead of preventing default.
-<Button - variant="outline" - size="sm" - className="w-full justify-start" - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - }} -> - <PlusIcon className="h-4 w-4" /> - <div className="text-xs">Install New Plugin</div> -</Button> +<Link href="/plugins/new"> + <Button variant="outline" size="sm" className="w-full justify-start"> + <PlusIcon className="h-4 w-4" /> + <div className="text-xs">Install New Plugin</div> + </Button> +</Link>Alternative (temporary):
-<Button ... onClick={...}> +<a href="https://docs.getbifrost.ai/plugins#install" target="_blank" rel="noopener noreferrer"> + <Button variant="outline" size="sm" className="w-full justify-start"> ... </Button> +</a>
36-41: External link: add rel to avoid reverse‑tabnabbing.Add rel="noopener noreferrer" with target="_blank".
-<Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> +<Link + className="text-primary hover:underline dark:text-green-400" + href="https://docs.getbifrost.ai/plugins" + target="_blank" + rel="noopener noreferrer" +>
🧹 Nitpick comments (4)
ui/app/plugins/page.tsx (2)
13-16: Add a page title for a11y.Include a minimal h1 (can be sr-only) so screen readers have a landmark.
return ( <div className="mx-auto w-full max-w-7xl"> + <h1 className="sr-only">Plugins</h1>
10-11: Handle loading/empty states.Render a small placeholder while fetching and when no custom plugins exist.
-const { data: plugins, isLoading } = useGetPluginsQuery(); +const { data: plugins, isLoading } = useGetPluginsQuery(); -const customPlugins = useMemo(() => plugins?.filter((plugin) => plugin.isCustom), [plugins]); +const customPlugins = useMemo(() => plugins?.filter((plugin) => plugin.isCustom), [plugins]); @@ - <div className="flex w-[250px] flex-col gap-2 pb-10"> + <div className="flex w-[250px] flex-col gap-2 pb-10"> <div className="rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20"> <div className="mb-4"> - {customPlugins?.map((plugin) => ( - <div key={plugin.name}>{plugin.name}</div> - ))} + {isLoading && <div className="text-xs text-muted-foreground">Loading plugins…</div>} + {!isLoading && (!customPlugins || customPlugins.length === 0) && ( + <div className="text-xs text-muted-foreground">No custom plugins installed yet.</div> + )} + {!isLoading && + customPlugins?.map((plugin) => <div key={plugin.name}>{plugin.name}</div>)}ui/components/sidebar.tsx (1)
292-295: Optional: logs live‑pulse never shows for Observability (has subItems).The pulse renders only when
!hasSubItems && item.url === "/logs". Consider moving the indicator to the “Logs” sub‑item.ui/app/observability/views/observabilityView.tsx (1)
82-90: Usenullfor missing plugin paths and guard redundant dispatch
- The
Plugin.pathfield isstring | null, so replacepath: ""withpath: nullin both fallback objects.- Wrap
dispatch(setSelectedPlugin(plugin))in a check (selectedPlugin?.name !== plugin.name) to avoid unnecessary state updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(11 hunks)ui/lib/types/plugins.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/app/cluster/page.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- ui/app/adaptive-routing/page.tsx
- ui/app/audit-logs/page.tsx
- ui/app/providers/page.tsx
- ui/app/user-groups/page.tsx
- ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx
- framework/configstore/tables/plugin.go
- framework/configstore/rdb.go
- ui/app/guardrails/page.tsx
- ui/app/alert-channels/page.tsx
- ui/app/virtual-keys/page.tsx
- transports/bifrost-http/lib/config.go
- ui/app/config/page.tsx
- ui/app/providers/dialogs/addNewKeySheet.tsx
- transports/bifrost-http/handlers/plugins.go
- ui/lib/types/plugins.ts
- ui/app/scim/page.tsx
- framework/configstore/migrations.go
- ui/app/logs/views/logsTable.tsx
- ui/app/providers/dialogs/addNewCustomProviderSheet.tsx
- ui/app/layout.tsx
- ui/app/mcp-clients/page.tsx
- ui/app/providers/views/modelProviderConfig.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
ui/app/observability/page.tsx (1)
ui/app/observability/views/observabilityView.tsx (1)
ObservabilityView(67-168)
ui/app/providers/views/providerKeyForm.tsx (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)
ui/components/sidebar.tsx (1)
ui/components/ui/sidebar.tsx (5)
SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)SidebarContent(598-598)SidebarGroup(600-600)
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
ui/components/ui/alertDialog.tsx (1)
AlertDialogCancel(85-85)ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
ui/app/observability/page.tsx (1)
4-8: LGTM — layout wrapper is appropriate.
akshaydeo
force-pushed
the
10-16-dynamic_plugin_architecture
branch
from
bce70c7 to
127675d
Compare
akshaydeo
force-pushed
the
10-20-dynamic-plugins-ui
branch
from
de194ef to
e14cdec
Compare
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 (4)
transports/bifrost-http/handlers/inference.go (2)
832-900: SSE “[DONE]” emission is incorrect; requestType is never set.[DONE] is sent for Responses API too, diverging from OpenAI semantics. Track whether the stream is “responses” and skip [DONE].
Proposed fix:
- var requestType schemas.RequestType + isResponses := false // Use streaming response writer ctx.Response.SetBodyStreamWriter(func(w *bufio.Writer) { @@ - includeEventType := false + includeEventType := false if chunk.BifrostResponsesStreamResponse != nil || (chunk.BifrostError != nil && chunk.BifrostError.ExtraFields.RequestType == schemas.ResponsesStreamRequest) { includeEventType = true } + if includeEventType { + isResponses = true + } @@ - if requestType != schemas.ResponsesStreamRequest { + if !isResponses { // Send the [DONE] marker to indicate the end of the stream (only for non-responses APIs) if _, err := fmt.Fprint(w, "data: [DONE]\n\n"); err != nil { h.logger.Warn(fmt.Sprintf("Failed to write SSE done marker: %v", err)) } }
685-690: Guard file reads and fix int64→int mismatch; re-enable size/type validation.Use a LimitedReader to cap memory and avoid using int64 directly in make(); restore MIME/size validation.
Suggested changes:
- // // Validate file size and format - // if err := h.validateAudioFile(fileHeader); err != nil { - // SendError(ctx, fasthttp.StatusBadRequest, err.Error(), h.logger) - // return - // } + // Validate file size and format + if err := h.validateAudioFile(fileHeader); err != nil { + SendError(ctx, fasthttp.StatusBadRequest, err.Error(), h.logger) + return + } @@ - // Read file data - fileData := make([]byte, fileHeader.Size) - if _, err := file.Read(fileData); err != nil { - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to read uploaded file: %v", err), h.logger) - return - } + // Read file data with hard cap to prevent OOM + lr := &io.LimitedReader{R: file, N: MaxFileSize + 1} + fileData, err := io.ReadAll(lr) + if err != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to read uploaded file: %v", err), h.logger) + return + } + if len(fileData) > MaxFileSize { + SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("File exceeds maximum size of %d MB", MaxFileSize/1024/1024), h.logger) + return + }Also applies to: 699-703
ui/components/sidebar.tsx (2)
263-269: Governance gating bypass: prevent toggle and subitem navigation when not allowed.Currently, users can expand Governance and navigate its subitems even when disallowed.
Apply:
const handleClick = (e: React.MouseEvent) => { - if (hasSubItems && onToggle) { + if (!isAllowed) { + e.preventDefault(); + return; + } + if (hasSubItems && onToggle) { e.preventDefault(); onToggle(); } }; const handleSubItemClick = (url: string) => { - router.push(url); + if (!isAllowed) return; + router.push(url); }; ... <SidebarItem key={item.title} item={item} isActive={isActive} isAllowed={isAllowed} isWebSocketConnected={isWebSocketConnected} isExpanded={expandedItems.has(item.title)} - onToggle={() => toggleItem(item.title)} + onToggle={() => isAllowed && toggleItem(item.title)} pathname={pathname} router={router} /> ... <SidebarMenuSubButton className={`h-7 cursor-pointer rounded-md px-2 transition-all duration-200 ${/* unchanged */""}`} - onClick={() => handleSubItemClick(subItem.url)} + onClick={() => handleSubItemClick(subItem.url)} + disabled={!isAllowed} >Also applies to: 275-277, 496-507, 317-323
449-456: Add rel="noopener noreferrer" to “View release notes” link.External link opens in a new tab without rel.
Apply:
- <Link + <Link href={`https://docs.getbifrost.ai/changelogs/${latestRelease.name}`} target="_blank" + rel="noopener noreferrer" className="text-primary mt-auto pb-1 font-medium underline" >
♻️ Duplicate comments (3)
ui/app/plugins/page.tsx (1)
58-58: Security: Add rel attribute to external link.The external link opens with
target="_blank"but is missingrel="noopener noreferrer", which prevents reverse tabnabbing attacks and referrer leakage.Apply this diff:
-<Link className="text-primary hover:underline" href="https://docs.getbifrost.ai/plugins" target="_blank"> +<Link className="text-primary hover:underline" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">ui/components/sidebar.tsx (2)
405-411: Auto-expand logic incomplete: populate expanded parents for active subitems.Apply:
useEffect(() => { const newExpandedItems = new Set<string>(); + items.forEach((item) => { + if (item.subItems?.some((sub) => pathname.startsWith(sub.url))) { + newExpandedItems.add(item.title); + } + }); if (newExpandedItems.size > 0) { setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); } }, [pathname]);
146-150: MCP naming consistency (label vs icon labels/route).Sidebar label is “MCP Gateway”, icon title/aria says “MCP clients icon” and route remains “/mcp-clients”. Either revert label to “MCP Clients” or align icon labels and route to “mcp-gateway” for consistency across UI/docs.
Also applies to: 63-66
🧹 Nitpick comments (5)
ui/app/config/page.tsx (1)
65-81: Add basic tab semantics for accessibility.Consider role="tablist"/role="tab" and aria-selected for keyboard/screen-reader support.
Example diff:
- <div className="flex min-w-[250px] flex-col gap-1 rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20"> + <div role="tablist" aria-label="Config sections" className="flex min-w-[250px] flex-col gap-1 rounded-md bg-zinc-50/50 p-4 dark:bg-zinc-800/20"> - <button + <button + role="tab" + aria-selected={activeTab === tab.id} key={tab.id} className={cn( "mb-1 flex w-full items-center gap-2 rounded-sm border px-3 py-1.5 text-sm", activeTab === tab.id ? "bg-secondary opacity-100 hover:opacity-100" : "hover:bg-secondary cursor-pointer border-transparent opacity-100 hover:border", )} onClick={() => setActiveTab(tab.id)} type="button" >transports/bifrost-http/handlers/inference.go (1)
599-606: Remove duplicate nil-check for SpeechParameters.The second block repeats the same initialization.
Apply:
- // Extract extra params - if req.SpeechParameters == nil { - req.SpeechParameters = &schemas.SpeechParameters{} - }framework/configstore/tables/plugin.go (1)
13-21: JSON casing inconsistency: isCustom vs created_at/updated_at.API fields mix camelCase and snake_case. If external clients consume this JSON, consider aligning to a single convention (e.g., json:"is_custom") or confirm consumers expect isCustom.
ui/app/layout.tsx (1)
40-42: Avoid duplicate custom-scrollbar classes.Parent div already controls scrolling; remove custom-scrollbar from main to prevent style duplication.
- <main className="custom-scrollbar relative mx-auto flex flex-col p-4"> + <main className="relative mx-auto flex flex-col p-4">ui/app/plugins/page.tsx (1)
30-44: Replace window.confirm() with AlertDialog component.The native
window.confirm()provides poor UX and doesn't match the app's design system. Consider using a consistentAlertDialogcomponent like the one inmodelProviderKeysTableView.tsxfor better user experience and styling consistency.Example pattern from
ui/app/providers/views/modelProviderKeysTableView.tsx(lines 41-76):const [showDeleteDialog, setShowDeleteDialog] = useState<{show: boolean; pluginName: string} | undefined>(); // In render: {showDeleteDialog && ( <AlertDialog open={showDeleteDialog.show}> <AlertDialogContent> <AlertDialogHeader> <AlertDialogTitle>Delete Plugin</AlertDialogTitle> <AlertDialogDescription> Are you sure you want to delete "{showDeleteDialog.pluginName}"? This action cannot be undone. </AlertDialogDescription> </AlertDialogHeader> <AlertDialogFooter> <AlertDialogCancel onClick={() => setShowDeleteDialog(undefined)}> Cancel </AlertDialogCancel> <AlertDialogAction onClick={() => { /* delete logic */ }}> Delete </AlertDialogAction> </AlertDialogFooter> </AlertDialogContent> </AlertDialog> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/fragments/pluginFormFragement.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/plugins/sheets/addNewPluginSheet.tsx(1 hunks)ui/app/prompt-repo/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(13 hunks)ui/components/ui/promoCardStack.tsx(3 hunks)ui/lib/types/plugins.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- ui/app/logs/page.tsx
- ui/app/user-groups/page.tsx
- ui/app/adaptive-routing/page.tsx
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/observability/page.tsx
- ui/app/audit-logs/page.tsx
- ui/app/scim/page.tsx
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/handlers/plugins.go
- ui/app/providers/dialogs/addNewKeySheet.tsx
- ui/app/logs/views/logsTable.tsx
- ui/app/providers/views/providerKeyForm.tsx
- ui/app/observability/views/observabilityView.tsx
- framework/configstore/rdb.go
- ui/app/cluster/page.tsx
- ui/app/mcp-clients/page.tsx
- ui/app/providers/dialogs/addNewCustomProviderSheet.tsx
- ui/app/virtual-keys/page.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
ui/app/alert-channels/page.tsx (1)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
AlertChannelsView(4-16)
ui/app/prompt-repo/page.tsx (1)
ui/lib/utils.ts (1)
cn(4-6)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
ui/app/providers/views/modelProviderKeysTableView.tsx (2)
ui/components/ui/alertDialog.tsx (1)
AlertDialogCancel(85-85)ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/plugins/fragments/pluginFormFragement.tsx (2)
ui/components/ui/input.tsx (1)
Input(15-69)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/providers/page.tsx (1)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
AddCustomProviderSheet(41-176)
ui/app/plugins/sheets/addNewPluginSheet.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/fragments/pluginFormFragement.tsx (1)
PluginFormFragment(23-131)
ui/app/plugins/page.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/sheets/addNewPluginSheet.tsx (1)
AddNewPluginSheet(51-178)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/components/sidebar.tsx (1)
ui/components/ui/promoCardStack.tsx (1)
PromoCardStack(21-120)
🪛 Biome (2.1.2)
ui/app/plugins/fragments/pluginFormFragement.tsx
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (15)
ui/app/alert-channels/page.tsx (1)
1-9: LGTM! Clean page component implementation.The page component correctly wraps AlertChannelsView with a centered, max-width container. This follows Next.js conventions and aligns with the PR's goal of standardizing layout wrappers across UI pages.
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (2)
4-16: LGTM! Well-structured enterprise fallback component.The component correctly renders a ContactUsView with appropriate enterprise upsell messaging. The
min-h-[80vh]ensures good visual presentation, and the Siren icon aligns with the PR's stated icon additions.
12-12: Documentation URL is accessible and functional.The URL
https://docs.getbifrost.ai/enterprise/alert-channelsreturns HTTP 200, confirming the documentation link is live and will work for users clicking "Read more" in the alert channels view.ui/app/config/page.tsx (1)
64-64: Layout centering LGTM.Adding max-w-7xl mx-auto keeps content readable and consistent with other routes.
transports/bifrost-http/handlers/inference.go (1)
354-356: No-op formatting change.Whitespace-only; safe to merge.
ui/app/guardrails/page.tsx (1)
4-8: Centered wrapper LGTM.Consistent with app-wide max-w-7xl layout.
ui/app/layout.tsx (1)
41-41: Main wrapper update LGTM.Flexible, centered base pairs well with page-level max-w-7xl wrappers.
ui/app/providers/page.tsx (1)
24-24: LGTM: Clean migration from Dialog to Sheet component.The transition from
AddCustomProviderDialogtoAddCustomProviderSheetmaintains the same prop interface and component wiring. The Sheet-based UI pattern is consistent with similar changes across the PR.Also applies to: 113-124
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
78-83: LGTM: Sheet migration with proper keyIndex propagation.The transition to
AddNewKeySheetcorrectly passes thekeyIndexprop for per-key edit/add operations. The component usage aligns with the Sheet-based pattern adopted across the PR.ui/app/config/views/pluginsForm.tsx (1)
119-124: LGTM: Empty path correctly identifies built-in plugin.The
path: ""field is appropriately set for the semantic cache built-in plugin. The backend will deriveisCustom: falsefrom the empty path, correctly distinguishing it from custom plugins.framework/configstore/migrations.go (1)
611-613: LGTM: Migration correctly adds and removes is_custom column.The
is_customcolumn is properly added alongside thepathcolumn in the migration, and both are correctly removed in the rollback. This supports the custom plugin schema changes across the codebase.Also applies to: 623-625
ui/lib/types/plugins.ts (1)
10-11: LGTM: Type extensions support custom plugin feature.The
isCustomandpathfields properly extend the plugin model to distinguish and locate custom plugins. The changes align with backend schema updates and migration logic.Also applies to: 21-21
ui/components/ui/promoCardStack.tsx (2)
18-18: LGTM: onCardsEmpty callback properly implemented.The optional
onCardsEmptyprop is correctly typed and invoked when the items list becomes empty. The effect dependencies are appropriate, enabling dynamic UI adjustments based on card presence.Also applies to: 21-21, 41-45
62-64: LGTM: Early return guards against empty visible slice.The updated condition checks both the input
cardsand the computedvisibleCards, ensuring no render when the visible slice is empty. This prevents rendering an empty container.ui/app/plugins/page.tsx (1)
13-131: LGTM: Comprehensive plugin CRUD implementation.The plugins page now provides full functionality with proper data fetching, state management, and CRUD operations. The implementation successfully addresses past review concerns about the placeholder page. The integration with
AddNewPluginSheetand mutation hooks follows established patterns.
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> | ||
| </p> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external link.
Opening in a new tab without rel is a security risk.
Apply:
- } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a>
+ } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a>Based on static analysis hints.
📝 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.
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> | |
| </p> | |
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a> | |
| </p> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In ui/app/plugins/fragments/pluginFormFragement.tsx around lines 34 to 35, the
external anchor opens in a new tab without rel attributes; update the <a> tag to
include rel="noopener noreferrer" in addition to target="_blank" to mitigate
security and performance risks when linking to external sites.
| const form = useForm<PluginFormData>({ | ||
| resolver: zodResolver(pluginFormSchema), | ||
| defaultValues: { | ||
| name: '', | ||
| path: '', | ||
| hasConfig: false, | ||
| config: undefined, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Submit stays disabled: enable RHF validation on change or relax the guard.
isValid won’t become true before submit with RHF’s default mode ("onSubmit"), so the submit button remains disabled.
Apply one of:
- Preferred: turn on change validation.
const form = useForm<PluginFormData>({
resolver: zodResolver(pluginFormSchema),
+ mode: "onChange",
+ reValidateMode: "onChange",
defaultValues: {- Or relax the button guard (less ideal):
- disabled={isLoading || !form.formState.isValid}
+ disabled={isLoading || !form.formState.isSubmitting && !form.formState.isValid}Also applies to: 165-169
🤖 Prompt for AI Agents
In ui/app/plugins/sheets/addNewPluginSheet.tsx around lines 58-66 (and similarly
at 165-169) the form uses React Hook Form's default "onSubmit" mode so
formState.isValid stays false until submit, leaving the submit button disabled;
update the useForm call to enable validation on change (set mode to "onChange"
and optionally reValidateMode to "onChange") so isValid updates as fields
change, or alternatively loosen the submit-button guard to not rely on isValid
(preferred: enable onChange validation).
| const handleClose = () => { | ||
| form.reset() | ||
| onClose() | ||
| } | ||
|
|
||
| return ( | ||
| <Sheet open={open} onOpenChange={handleClose}> | ||
| <SheetContent className="dark:bg-card flex w-full flex-col overflow-x-hidden bg-white p-8 sm:max-w-2xl"> | ||
| <SheetHeader className="p-0"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix onOpenChange: handle the boolean and reset only on close.
Passing a no‑arg handler risks closing on open toggles and ignores the next state.
Apply:
- const handleClose = () => {
- form.reset()
- onClose()
- }
+ const handleClose = () => {
+ form.reset();
+ onClose();
+ }
+
+ const handleOpenChange = (nextOpen: boolean) => {
+ if (!nextOpen) {
+ handleClose();
+ }
+ }
...
- <Sheet open={open} onOpenChange={handleClose}>
+ <Sheet open={open} onOpenChange={handleOpenChange}>🤖 Prompt for AI Agents
In ui/app/plugins/sheets/addNewPluginSheet.tsx around lines 132 to 140, the
Sheet onOpenChange is wired to a no-arg handler which can't tell open vs close
and may reset on open; change handleClose to accept the boolean nextOpen (or
isOpen) parameter, call form.reset() and onClose() only when nextOpen === false
(closing), and pass that handler to onOpenChange so open toggles won't
incorrectly reset the form.
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | ||
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | ||
| Prompt repositories is coming soon. Please check back soon for updates. | ||
| </div> |
There was a problem hiding this comment.
Fix grammatical error and inconsistency.
Line 11 uses singular "repository" but line 13 uses plural "repositories" with incorrect verb agreement. Use singular throughout for consistency.
Apply this diff:
<h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1>
<div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal">
- Prompt repositories is coming soon. Please check back soon for updates.
+ Prompt repository is coming soon. Please check back soon for updates.
</div>📝 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.
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | |
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | |
| Prompt repositories is coming soon. Please check back soon for updates. | |
| </div> | |
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | |
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | |
| Prompt repository is coming soon. Please check back soon for updates. | |
| </div> |
🤖 Prompt for AI Agents
In ui/app/prompt-repo/page.tsx around lines 11 to 14, the first line uses
singular "repository" while the following line uses plural "repositories" with
incorrect verb agreement; update the second line to use singular to match the
heading and correct the verb (e.g., "Prompt repository is coming soon. Please
check back soon for updates." or more natural: "The prompt repository is coming
soon. Please check back for updates.").
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
ui/components/sidebar.tsx (2)
146-150: “MCP Gateway” label conflicts with /mcp-clients and icon aria text.Either keep label “MCP Clients” or complete the rename (route, MCPIcon aria/title, page headings, docs).
405-411: Auto-expand logic is incomplete — populate the Set.newExpandedItems is never filled, so parents don’t auto-expand when a sub-route is active.
useEffect(() => { - const newExpandedItems = new Set<string>(); - - if (newExpandedItems.size > 0) { - setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); - } + const newExpandedItems = new Set<string>(); + items.forEach((item) => { + if (item.subItems?.some((sub) => pathname.startsWith(sub.url))) { + newExpandedItems.add(item.title); + } + }); + if (newExpandedItems.size > 0) { + setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); + } }, [pathname]);transports/bifrost-http/handlers/plugins.go (1)
114-168: Path field is not extracted from request or passed to CreatePlugin.The
Pathfield is defined inCreatePluginRequest(line 44) but is never used in thecreatePluginhandler. It must be extracted from the request and passed to bothCreatePlugin(lines 134-138) andReloadPlugin(line 153).Apply this diff:
if err := h.configStore.CreatePlugin(ctx, &configstoreTables.TablePlugin{ Name: request.Name, Enabled: request.Enabled, Config: request.Config, + Path: request.Path, }); err != nil {Also update the
ReloadPlugincall to userequest.Path:if request.Enabled { - if err := h.pluginsLoader.ReloadPlugin(ctx, request.Name, nil, request.Config); err != nil { + if err := h.pluginsLoader.ReloadPlugin(ctx, request.Name, request.Path, request.Config); err != nil {ui/app/providers/views/providerKeyForm.tsx (1)
81-81: Remove debug console.log statement.This debug statement should be removed before merging to production.
Apply this diff:
- console.log(form.formState.isDirty); -ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
98-98: Fixed minimum width causes responsive issues on mobile.The
min-w-[600px]will cause horizontal overflow on mobile devices (viewport: 360-428px). Use a responsive approach instead.Apply this diff:
- <SheetContent className="custom-scrollbar dark:bg-card bg-white py-4 min-w-[600px]"> + <SheetContent className="custom-scrollbar dark:bg-card bg-white py-4 w-full sm:max-w-2xl">Note: The same issue exists in
addNewKeySheet.tsxat line 21 and should be fixed similarly.ui/app/providers/dialogs/addNewKeySheet.tsx (1)
21-21: Fixed minimum width causes responsive issues on mobile.The
min-w-[600px]will cause horizontal overflow on mobile devices (viewport: 360-428px).Apply this diff:
- <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> + <SheetContent className="custom-scrollbar bg-white dark:bg-card w-full sm:max-w-2xl py-4">ui/app/plugins/page.tsx (1)
58-60: External link: add rel to prevent reverse‑tabnabbing.Add
rel="noopener noreferrer"alongsidetarget="_blank".-<Link className="text-primary hover:underline" href="https://docs.getbifrost.ai/plugins" target="_blank"> +<Link className="text-primary hover:underline" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">
🧹 Nitpick comments (8)
ui/app/observability/page.tsx (1)
4-8: Inconsistent class ordering across similar page layouts.The wrapper div uses the class order
max-w-7xl mx-auto w-full, whileui/app/audit-logs/page.tsx(Line 4) usesmx-auto w-full max-w-7xl. Although Tailwind is order-independent for these utilities, consistent ordering improves maintainability across the codebase.Apply this diff to align with the ordering in
ui/app/audit-logs/page.tsx:- <div className="max-w-7xl mx-auto w-full"> + <div className="mx-auto w-full max-w-7xl">Alternatively, update all similar pages to use a consistent order (e.g.,
max-w-7xl mx-auto w-full).ui/components/ui/promoCardStack.tsx (1)
15-22: Prop addition is fine; consider documenting semantics.onCardsEmpty is useful; clarify in a brief JSDoc whether it should fire only after user dismissals vs. also when initial cards is empty. This avoids divergent parent expectations.
ui/components/sidebar.tsx (2)
311-327: Avoid any for subItems; add simple types.Strong typing improves DX and prevents runtime mismatches.
+// Types for sidebar items +type SidebarSubItem = { + title: string; + url: string; + icon?: React.ElementType; + description?: string; +}; +type SidebarItemType = { + title: string; + url: string; + icon: React.ElementType; + description?: string; + subItems?: SidebarSubItem[]; +}; - -const items = [ +const items: SidebarItemType[] = [ // ... ]; -const SidebarItem = ({ +const SidebarItem = ({ item, isActive, isAllowed, isWebSocketConnected, isExpanded, onToggle, pathname, router, }: { - item: (typeof items)[0]; + item: SidebarItemType; isActive: boolean; isAllowed: boolean; isWebSocketConnected: boolean; isExpanded?: boolean; onToggle?: () => void; pathname: string; router: ReturnType<typeof useRouter>; }) => { // ... - {item.subItems?.map((subItem: any) => { + {item.subItems?.map((subItem: SidebarSubItem) => {
476-479: Stabilize handleCardsEmpty with useCallback.Prevents re-creating the handler every render and reduces redundant effects downstream.
-import { useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useState } from "react"; @@ -const handleCardsEmpty = () => { - setAreCardsEmpty(true); -}; +const handleCardsEmpty = useCallback(() => { + setAreCardsEmpty(true); +}, []);ui/app/plugins/sheets/addNewPluginSheet.tsx (2)
138-138: Only close the sheet when it changes to closed.
onOpenChangepasses the next boolean state. CallinghandleClose()unconditionally can fight open transitions. Gate on!open.- <Sheet open={open} onOpenChange={handleClose}> + <Sheet open={open} onOpenChange={(next) => { if (!next) handleClose() }}>
15-26: Trim inputs before validation to reduce user error.Minor polish: trim
nameandpathbefore checks to avoid leading/trailing space issues.-const pluginFormSchema = z.object({ - name: z.string().min(1, 'Plugin name is required').regex(/^[a-z0-9-_]+$/, 'Plugin name must contain only lowercase letters, numbers, hyphens, and underscores'), - path: z.string().min(1, 'Plugin path/URL is required').refine( +const pluginFormSchema = z.object({ + name: z.string().transform((v) => v.trim()).min(1, 'Plugin name is required').regex(/^[a-z0-9-_]+$/, 'Plugin name must contain only lowercase letters, numbers, hyphens, and underscores'), + path: z.string().transform((v) => v.trim()).min(1, 'Plugin path/URL is required').refine(ui/app/plugins/page.tsx (2)
76-83: Add an accessible label for the status dot.The colored dot alone isn’t announced by screen readers. Include an sr-only label.
- <CardTitle className="flex items-center justify-between"> - <span>{plugin.name}</span> - <div className={`h-2 w-2 rounded-full ${plugin.enabled ? 'bg-green-500' : 'bg-gray-400'}`} /> - </CardTitle> + <CardTitle className="flex items-center justify-between"> + <span>{plugin.name}</span> + <div className="flex items-center gap-2"> + <span className="sr-only">{plugin.enabled ? 'Enabled' : 'Disabled'}</span> + <span + aria-hidden + className={`h-2 w-2 rounded-full ${plugin.enabled ? 'bg-green-500' : 'bg-gray-400'}`} + /> + </div> + </CardTitle>
95-101: Disable only the row being deleted (better UX).Global
isDeletingdisables all delete buttons. Track the deleting plugin name to disable just one.- const [deletePlugin, { isLoading: isDeleting }] = useDeletePluginMutation(); + const [deletePlugin, { isLoading: isDeleting }] = useDeletePluginMutation(); + const [deletingName, setDeletingName] = useState<string | null>(null); @@ - const handleDelete = async (pluginName: string) => { + const handleDelete = async (pluginName: string) => { if (!confirm(`Are you sure you want to delete the plugin "${pluginName}"?`)) { return; } try { + setDeletingName(pluginName); await deletePlugin(pluginName).unwrap(); toast.success('Plugin deleted successfully'); } catch (error) { const errorMessage = error && typeof error === 'object' && 'data' in error ? (error.data as any)?.error || 'Failed to delete plugin' : 'Failed to delete plugin'; toast.error(errorMessage); } finally { + setDeletingName(null); } }; @@ - disabled={isDeleting} + disabled={isDeleting && deletingName === plugin.name}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
plugins/governance/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/plugins.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/fragments/pluginFormFragement.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/plugins/sheets/addNewPluginSheet.tsx(1 hunks)ui/app/prompt-repo/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(3 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(13 hunks)ui/components/ui/promoCardStack.tsx(3 hunks)ui/lib/types/plugins.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- transports/bifrost-http/handlers/inference.go
- ui/app/mcp-clients/page.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- ui/app/logs/page.tsx
- ui/lib/types/plugins.ts
- transports/bifrost-http/lib/config.go
- ui/app/layout.tsx
- ui/app/adaptive-routing/page.tsx
- ui/app/providers/page.tsx
- ui/app/user-groups/page.tsx
- framework/configstore/tables/plugin.go
- ui/app/scim/page.tsx
- framework/configstore/rdb.go
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/app/logs/views/logsTable.tsx
- ui/app/observability/views/observabilityView.tsx
- ui/app/alert-channels/page.tsx
- ui/app/cluster/page.tsx
- ui/app/config/page.tsx
- ui/app/virtual-keys/page.tsx
- ui/app/guardrails/page.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
ui/app/providers/views/providerKeyForm.tsx (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)
ui/app/audit-logs/page.tsx (1)
ui/app/_fallbacks/enterprise/components/audit-logs/auditLogsView.tsx (1)
AuditLogsView(4-16)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
ui/app/plugins/sheets/addNewPluginSheet.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/fragments/pluginFormFragement.tsx (1)
PluginFormFragment(23-131)
ui/app/providers/views/modelProviderConfig.tsx (2)
ui/lib/types/config.ts (1)
isKnownProvider(15-17)ui/app/providers/views/utils.ts (1)
keysRequired(1-1)
ui/app/observability/page.tsx (1)
ui/app/observability/views/observabilityView.tsx (1)
ObservabilityView(67-168)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
ui/components/ui/sheet.tsx (6)
Sheet(90-90)SheetContent(90-90)SheetHeader(90-90)SheetTitle(90-90)SheetDescription(90-90)SheetFooter(90-90)ui/components/ui/input.tsx (1)
Input(15-69)
ui/app/plugins/page.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/sheets/addNewPluginSheet.tsx (1)
AddNewPluginSheet(51-178)
ui/app/plugins/fragments/pluginFormFragement.tsx (2)
ui/components/ui/input.tsx (1)
Input(15-69)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/components/sidebar.tsx (1)
ui/components/ui/promoCardStack.tsx (1)
PromoCardStack(21-120)
ui/app/prompt-repo/page.tsx (1)
ui/lib/utils.ts (1)
cn(4-6)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
🪛 Biome (2.1.2)
ui/app/plugins/fragments/pluginFormFragement.tsx
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (11)
ui/app/audit-logs/page.tsx (1)
4-6: LGTM!The centered max-width container correctly applies the consistent layout pattern across pages.
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
1-16: LGTM! Clean enterprise fallback implementation.The component follows the established pattern for enterprise feature promotional views, correctly reuses the
ContactUsViewcomponent, and provides appropriate marketing content for the Alert Channels feature. The implementation is straightforward with no concerns.ui/components/ui/promoCardStack.tsx (1)
62-65: Render guard looks good.Returning null when there are no input cards or no visibleCards is correct and prevents stray containers.
ui/components/sidebar.tsx (2)
513-516: PromoCardStack integration looks good; confirm desired empty-state layout shift.The height calculation toggles from 13rem to 8rem when cards empty. Verify this matches design specs and doesn’t cause jarring layout jumps.
79-91: Verify routes for subitems (“Connectors” → /observability) exist.Ensure /observability resolves, alongside /logs.
framework/configstore/migrations.go (1)
600-634: LGTM!The migration correctly adds both
pathandis_customcolumns together, and the rollback properly removes both. This aligns with the schema changes inframework/configstore/tables/plugin.go.ui/app/providers/views/modelProviderConfig.tsx (1)
46-60: LGTM!The controlled accordion pattern is implemented correctly. The
useEffectappropriately initializes the accordion to expand when API keys are not required or when the provider is custom, improving the user experience.ui/app/providers/views/providerKeyForm.tsx (1)
53-61: Good use of useCallback for tooltip content.The
getTooltipContenthelper is correctly memoized and provides clear conditional logic for tooltip display.ui/app/config/views/pluginsForm.tsx (1)
119-124: LGTM!The addition of
path: ""to thecreatePluginpayload correctly aligns with the extendedCreatePluginRequesttype that now includes thepathfield.ui/app/plugins/sheets/addNewPluginSheet.tsx (1)
101-119: Confirm create/update payloads and tag invalidation.Sanity check that:
- create sets whatever flags backend expects to mark a plugin “custom” (if required), and
- update/create/delete invalidate the plugins list so the grid refreshes.
ui/app/plugins/page.tsx (1)
14-19: Verify data refresh after mutations.Ensure
useGetPluginsQueryis invalidated by create/update/delete so the list reflects changes immediately.Also applies to: 124-128
| {isEditMode | ||
| ? 'Update your plugin configuration. Plugin name and path are read-only.' | ||
| : 'Install a custom plugin by providing an absolute file path or HTTP URL accessible to Bifrost deployment (.so).' | ||
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external link.
Opening external links with target="_blank" without rel="noopener noreferrer" is a security risk that can allow the opened page to access the window.opener object.
Apply this diff:
- } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a>
+ } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a>📝 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.
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> | |
| } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🤖 Prompt for AI Agents
In ui/app/plugins/fragments/pluginFormFragement.tsx around line 34, the external
anchor uses target="_blank" but is missing rel="noopener noreferrer"; update the
<a> tag to include rel="noopener noreferrer" so the external link opens in a new
tab safely by preventing access to window.opener and avoiding potential security
vulnerabilities.
| const form = useForm<PluginFormData>({ | ||
| resolver: zodResolver(pluginFormSchema), | ||
| defaultValues: { | ||
| name: '', | ||
| path: '', | ||
| hasConfig: false, | ||
| config: undefined, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Submit button will stay disabled (RHF isValid deadlock).
With default RHF mode ("onSubmit"), form.formState.isValid remains false until a submit occurs, but the submit button is disabled when !isValid, so users can’t submit at all. Enable validation on change.
Apply this diff:
const form = useForm<PluginFormData>({
resolver: zodResolver(pluginFormSchema),
+ mode: 'onChange',
defaultValues: {
name: '',
path: '',
hasConfig: false,
config: undefined,
},
})Also applies to: 166-169
🤖 Prompt for AI Agents
In ui/app/plugins/sheets/addNewPluginSheet.tsx around lines 58-66 (and similarly
at lines 166-169), the form is initialized with the default React Hook Form mode
("onSubmit") so form.formState.isValid stays false until a submit and the submit
button remains disabled; change the useForm initialization to enable validation
on change (e.g., set mode to "onChange" or "all") so isValid updates as the user
types, keeping the submit button enabled when the form is valid.
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | ||
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | ||
| Prompt repositories is coming soon. Please check back soon for updates. |
There was a problem hiding this comment.
Fix grammatical error and reduce redundancy in user-facing text.
Two text quality issues:
- Line 13: "Prompt repositories is" has subject-verb disagreement. Should be "Prompt repository is" (singular, matching the title) or "Prompt repositories are" (plural).
- The description repeats "coming soon" information from the title without adding new details.
Apply this diff to fix the grammatical error and improve the messaging:
<div className="flex flex-col gap-1">
<h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1>
<div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal">
- Prompt repositories is coming soon. Please check back soon for updates.
+ This feature will allow you to manage and version your prompts. Please check back soon for updates.
</div>
</div>📝 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.
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | |
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | |
| Prompt repositories is coming soon. Please check back soon for updates. | |
| <div className="flex flex-col gap-1"> | |
| <h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> | |
| <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> | |
| This feature will allow you to manage and version your prompts. Please check back soon for updates. | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In ui/app/prompt-repo/page.tsx around lines 11 to 13, fix the subject-verb
disagreement and remove redundant phrasing by making the description complement
the title instead of repeating it; change the second line so it matches the
singular title ("Prompt repository") and replaces the repeated "coming soon"
with a concise follow-up such as "Check back for updates" or "Stay tuned for
updates."
| useEffect(() => { | ||
| if (items.length === 0 && onCardsEmpty) { | ||
| onCardsEmpty(); | ||
| } | ||
| }, [items.length, onCardsEmpty]); | ||
|
|
There was a problem hiding this comment.
Fire onCardsEmpty only on transition to empty; prevent repeated callbacks.
Current effect re-fires whenever the parent re-renders with a new onCardsEmpty (common with inline lambdas), causing multiple calls when items.length is 0. Guard on transition (>0 → 0) and stabilize deps.
- useEffect(() => {
- if (items.length === 0 && onCardsEmpty) {
- onCardsEmpty();
- }
- }, [items.length, onCardsEmpty]);
+ // Call once when the stack transitions from non-empty to empty
+ const prevLenRef = React.useRef(items.length);
+ useEffect(() => {
+ if (prevLenRef.current > 0 && items.length === 0) {
+ onCardsEmpty?.();
+ }
+ prevLenRef.current = items.length;
+ }, [items.length]);Also add useRef to the import:
-import React, { useEffect, useState } from "react";
+import React, { useEffect, useRef, useState } from "react";📝 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.
| useEffect(() => { | |
| if (items.length === 0 && onCardsEmpty) { | |
| onCardsEmpty(); | |
| } | |
| }, [items.length, onCardsEmpty]); | |
| // Call once when the stack transitions from non-empty to empty | |
| const prevLenRef = React.useRef(items.length); | |
| useEffect(() => { | |
| if (prevLenRef.current > 0 && items.length === 0) { | |
| onCardsEmpty?.(); | |
| } | |
| prevLenRef.current = items.length; | |
| }, [items.length]); |
🤖 Prompt for AI Agents
In ui/components/ui/promoCardStack.tsx around lines 41–46, import useRef and add
a ref to track the previous items.length and a ref to hold the latest
onCardsEmpty callback; in the effect compare prevLengthRef.current > 0 &&
items.length === 0 and only then call onCardsEmptyRef.current(), update
prevLengthRef.current = items.length, and change the effect dependency to
[items.length] (no onCardsEmpty) so the callback only fires on the transition
from non-empty to empty and not on parent re-renders or changing inline lambdas.
127675d to
285f004
Compare
akshaydeo
force-pushed
the
10-20-dynamic-plugins-ui
branch
from
e14cdec to
1c19b16
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugins/otel/docker-compose.yml (1)
29-29: Fix port configuration mismatch in Tempo service.The configuration splits gRPC (port 3201) and HTTP (port 3200) across different ports, but only port 3200 is exposed in the service ports. The comment on line 29 labels it as "multiplexed," which no longer reflects the actual configuration.
Choose one approach:
- If gRPC should be accessible externally, add
- "3201:3201"to line 29 and update the comment to reflect separate ports.- If multiplexed behavior is intended, change line 143 to
grpc_listen_port: 3200to align with the exposed port.Currently, external gRPC connections to Tempo will fail since port 3201 is not exposed.
ui/components/sidebar.tsx (3)
3-23: Import Siren icon for “Alert channels”.PR objective mentions Siren; it’s not imported.
Layers, LogOut, Logs, Puzzle, + Siren, ScrollText,
263-277: Block navigation when Governance is disabled.Currently, disabled items can expand and subitems can navigate.
const handleClick = (e: React.MouseEvent) => { - if (hasSubItems && onToggle) { + if (!isAllowed) { + e.preventDefault(); + return; + } + if (hasSubItems && onToggle) { e.preventDefault(); onToggle(); } }; const handleSubItemClick = (url: string) => { - router.push(url); + if (!isAllowed) return; + router.push(url); };- className={`h-7 cursor-pointer rounded-md px-2 transition-all duration-200 ${ + className={`h-7 rounded-md px-2 transition-all duration-200 ${ + !isAllowed + ? "text-muted-foreground cursor-not-allowed" + : isSubItemActive ? "bg-sidebar-accent text-primary font-medium" : "hover:bg-sidebar-accent hover:text-accent-foreground text-slate-500 dark:text-zinc-400" }`} + aria-disabled={!isAllowed}Also applies to: 317-323
53-69: A11y: update MCP icon labels to new naming.ARIA/title still say “MCP clients”.
- aria-label="MCP clients icon" + aria-label="MCP gateway icon" ... - <title>MCP clients icon</title> + <title>MCP gateway icon</title>
♻️ Duplicate comments (11)
ui/app/providers/views/providerKeyForm.tsx (1)
81-81: Remove debug console.log statement.This debug statement should be removed before merging to production.
Apply this diff to remove the debug statement:
- console.log(form.formState.isDirty); -ui/app/plugins/fragments/pluginFormFragement.tsx (1)
34-34: Add rel="noopener noreferrer" to external link.Opening external links with
target="_blank"withoutrel="noopener noreferrer"is a security risk that can allow the opened page to access thewindow.openerobject.Apply this diff:
- } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> + } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a>Based on static analysis hints.
ui/components/ui/promoCardStack.tsx (1)
41-45: Fire onCardsEmpty only on transition to empty; prevent repeated callbacks.The effect re-fires whenever the parent re-renders with a new
onCardsEmptyreference (common with inline lambdas), causing multiple calls whenitems.lengthis 0. Guard on transition (>0 → 0) and stabilize deps.Apply this diff:
+ const prevLenRef = React.useRef(items.length); useEffect(() => { - if (items.length === 0 && onCardsEmpty) { - onCardsEmpty(); + if (prevLenRef.current > 0 && items.length === 0) { + onCardsEmpty?.(); } - }, [items.length, onCardsEmpty]); + prevLenRef.current = items.length; + }, [items.length, onCardsEmpty]);Also add
useRefto the import:-import React, { useEffect, useState } from "react"; +import React, { useEffect, useRef, useState } from "react";Based on past review comments.
ui/app/prompt-repo/page.tsx (1)
11-14: Fix grammatical error and reduce redundancy in user-facing text.Line 13 has subject-verb disagreement ("repositories is") and repeats "coming soon" without adding value.
Apply this diff:
<h1 className="text-muted-foreground text-xl font-medium">Prompt repository is coming soon</h1> <div className="text-muted-foreground mt-2 max-w-[600px] text-sm font-normal"> - Prompt repositories is coming soon. Please check back soon for updates. + This feature will allow you to manage and version your prompts. Check back soon for updates. </div>Based on past review comments.
ui/app/plugins/page.tsx (1)
52-52: Add rel attribute to external link for security.The external documentation link opens in a new tab but is missing
rel="noopener noreferrer"to prevent reverse-tabnabbing.Apply this diff:
- <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> + <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">Based on past review comments.
ui/app/plugins/sheets/addNewPluginSheet.tsx (2)
58-66: Enable validation on change to prevent submit button deadlock.With the default React Hook Form mode ("onSubmit"),
form.formState.isValidremains false until a submit attempt, but the submit button is disabled when!isValid, creating a deadlock where users can't submit valid forms.Apply this diff:
const form = useForm<PluginFormData>({ resolver: zodResolver(pluginFormSchema), + mode: 'onChange', defaultValues: { name: '', path: '', hasConfig: false, config: undefined, }, })Based on past review comments.
132-138: Fix onOpenChange to handle the boolean parameter correctly.The
onOpenChangehandler receives a boolean indicating the next open state, buthandleClosedoesn't accept parameters. This can cause the form to reset when opening the sheet.Apply this diff:
const handleClose = () => { form.reset() onClose() } + + const handleOpenChange = (nextOpen: boolean) => { + if (!nextOpen) { + handleClose(); + } + } return ( - <Sheet open={open} onOpenChange={handleClose}> + <Sheet open={open} onOpenChange={handleOpenChange}>Based on past review comments.
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
21-21: Prevent mobile overflow: make sheet width responsive.Use full width on mobile, fixed width from sm+.
- <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> + <SheetContent className="custom-scrollbar bg-white dark:bg-card w-full sm:min-w-[600px] sm:max-w-[600px] py-4">ui/components/sidebar.tsx (3)
405-411: Restore auto‑expand for active subitems.Set parents expanded when a subitem matches pathname.
useEffect(() => { const newExpandedItems = new Set<string>(); + items.forEach((item) => { + if (item.subItems?.some((subItem) => pathname.startsWith(subItem.url))) { + newExpandedItems.add(item.title); + } + }); if (newExpandedItems.size > 0) { setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); } }, [pathname]);
146-150: Align label and route: “MCP Gateway” vs “/mcp-clients”.Either revert label to “MCP Clients” or rename the route consistently.
Option A (revert label):
- title: "MCP Gateway", + title: "MCP Clients",Option B (rename route – broader scope): update path to /mcp-gateway and adjust all references, pages, docs, and tests.
#!/bin/bash # Find remaining "MCP Clients" vs "MCP Gateway" references rg -nI "MCP Clients" ui/ -C1 rg -nI "MCP Gateway" ui/ -C1
138-143: Add missing "Alert channels" nav item.The objective requires this navigation item, but it's not present in the sidebar. The route and icon are both available.
{ title: "Plugins", url: "/plugins", icon: Puzzle, description: "Manage custom plugins", }, + { + title: "Alert channels", + url: "/alert-channels", + icon: Siren, + description: "Manage alert notifications", + },Add
Sirento the lucide-react import on line 3.
🧹 Nitpick comments (5)
ui/app/logs/views/logsTable.tsx (1)
81-82: Height adjustment looks correct for sidebar changes.The reduction from 20rem to 16.5rem gives the logs table more vertical space, which aligns with the sidebar navigation reorganization mentioned in the PR objectives.
Consider extracting the magic number
16.5remas a constant or CSS variable to improve maintainability:// At the top of the file or in a constants file const LOGS_TABLE_VERTICAL_OFFSET = "16.5rem";Then use it in both places:
-<div className="rounded-sm border max-h-[calc(100vh-16.5rem)]"> - <Table containerClassName="max-h-[calc(100vh-16.5rem)]"> +<div className={`rounded-sm border max-h-[calc(100vh-${LOGS_TABLE_VERTICAL_OFFSET})]`}> + <Table containerClassName={`max-h-[calc(100vh-${LOGS_TABLE_VERTICAL_OFFSET})]`}>This makes it easier to adjust the offset if sidebar dimensions change in the future.
ui/app/providers/views/providerKeyForm.tsx (1)
107-107: Consider storing tooltip content to avoid double invocation.
getTooltipContent()is called twice on this line—once for the condition check and once to render the content. While not a performance concern given the simple logic, storing the result would be cleaner.You could refactor by computing it once within an IIFE or extract it earlier in the JSX:
- {getTooltipContent() && <TooltipContent>{getTooltipContent()}</TooltipContent>} + {(() => { + const content = getTooltipContent(); + return content && <TooltipContent>{content}</TooltipContent>; + })()}ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
6-6: Consider simplifying the wrapper div.The wrapper div with
h-full w-fullmay be redundant since:
- The parent page already applies
mx-auto w-full max-w-7xl(based on the summary)ContactUsViewhas its ownmin-h-[80vh]stylingIf no specific layout constraint requires this wrapper, you could simplify to:
export default function AlertChannelsView() { - return ( - <div className="h-full w-full"> - <ContactUsView + return ( + <ContactUsView className="mx-auto min-h-[80vh]" icon={<Siren className="h-[5.5rem] w-[5.5rem]" strokeWidth={1} />} title="Unlock alert channels for better observability" description="This feature is a part of the Bifrost enterprise license. We would love to know more about your use case and how we can help you." readmeLink="https://docs.getbifrost.ai/enterprise/alert-channels" /> - </div> ); }ui/app/observability/views/observabilityView.tsx (1)
82-89: Use null instead of empty string for path in fallback plugin.The
Plugintype definespath: string | null, but the fallback object uses an empty string. For consistency and to represent "no path", usenullinstead.Apply this diff:
const plugin = plugins.find((plugin) => plugin.name === selectedPluginId) ?? { name: selectedPluginId, enabled: false, config: {}, isCustom: false, - path: "", + path: null, }; dispatch(setSelectedPlugin(plugin));Also applies to: 96-102
ui/app/plugins/page.tsx (1)
12-13: Consider handling loading state for better UX.The
isLoadingstate fromuseGetPluginsQueryis destructured but not used. Consider showing a loading indicator while plugins are being fetched.Example:
if (isLoading) { return <div className="mx-auto w-full max-w-7xl">Loading plugins...</div>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
plugins/governance/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sum
📒 Files selected for processing (38)
framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)plugins/otel/docker-compose.yml(1 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/plugins.go(3 hunks)transports/bifrost-http/handlers/server.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/fragments/pluginFormFragement.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/plugins/sheets/addNewPluginSheet.tsx(1 hunks)ui/app/prompt-repo/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(2 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(13 hunks)ui/components/ui/promoCardStack.tsx(3 hunks)ui/lib/types/plugins.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/app/config/page.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- ui/app/providers/dialogs/addNewCustomProviderSheet.tsx
- transports/bifrost-http/handlers/plugins.go
- ui/app/audit-logs/page.tsx
- framework/configstore/rdb.go
- ui/app/guardrails/page.tsx
- ui/app/mcp-clients/page.tsx
- ui/app/observability/page.tsx
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/logs/page.tsx
- ui/app/providers/page.tsx
- transports/bifrost-http/lib/config.go
- ui/app/config/views/pluginsForm.tsx
- ui/app/alert-channels/page.tsx
- framework/configstore/migrations.go
- ui/app/layout.tsx
🧰 Additional context used
🧬 Code graph analysis (12)
ui/app/prompt-repo/page.tsx (1)
ui/lib/utils.ts (1)
cn(4-6)
ui/app/scim/page.tsx (1)
ui/app/_fallbacks/enterprise/components/scim/scimView.tsx (1)
SCIMView(4-16)
ui/app/adaptive-routing/page.tsx (1)
ui/app/_fallbacks/enterprise/components/adaptive-routing/adaptiveRoutingView.tsx (1)
AdaptiveRoutingView(4-16)
ui/app/plugins/sheets/addNewPluginSheet.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/fragments/pluginFormFragement.tsx (1)
PluginFormFragment(23-131)
ui/app/virtual-keys/page.tsx (1)
ui/app/virtual-keys/views/virtualKeysTable.tsx (1)
VirtualKeysTable(34-230)
transports/bifrost-http/handlers/server.go (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)core/schemas/plugin.go (1)
Plugin(45-71)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/app/plugins/page.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/sheets/addNewPluginSheet.tsx (1)
AddNewPluginSheet(51-178)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/components/sidebar.tsx (1)
ui/components/ui/promoCardStack.tsx (1)
PromoCardStack(21-120)
ui/app/providers/views/providerKeyForm.tsx (1)
ui/app/providers/fragments/apiKeysFormFragment.tsx (1)
ApiKeyFormFragment(21-445)
ui/app/plugins/fragments/pluginFormFragement.tsx (2)
ui/components/ui/input.tsx (1)
Input(15-69)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
🪛 Biome (2.1.2)
ui/app/plugins/fragments/pluginFormFragement.tsx
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (15)
plugins/otel/docker-compose.yml (1)
1-229: Scope mismatch: infrastructure change unrelated to PR objectives.This PR is titled "adds dynamic ui" and focuses on updating the admin interface sidebar (menu items, icons). However, this infrastructure/DevOps file appears to have been included unintentionally. The Tempo gRPC port change has no documented relationship to the UI feature.
Verify: Was this file change intentional, or should it be excluded from this PR?
ui/app/providers/views/providerKeyForm.tsx (3)
46-51: Good addition for immediate validation feedback.Triggering validation on mount when editing existing data provides immediate feedback to users about any validation issues with the current data.
53-61: Good abstraction for tooltip content logic.The
getTooltipContentcallback centralizes the tooltip logic and makes it easier to understand the different disabled states. The memoization withuseCallbackis appropriate here.
88-88: Good fix for Tailwind class syntax.Corrected the invalid
bg:whiteto the properbg-whiteTailwind class.ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (2)
1-2: LGTM!The imports are correct and align with the PR objectives to add a Siren icon for the Alert Channels feature.
7-13: Pattern verification complete—code approved.The alertChannelsView.tsx implementation follows the well-established enterprise fallback pattern across all 10 similar components in the codebase. Icon sizing, prop structure, documentation links, and accessibility handling are all consistent and correct. No issues found.
ui/app/user-groups/page.tsx (1)
105-105: LGTM! Layout standardization.The centered, width-constrained container aligns with the layout pattern applied consistently across multiple pages in this PR.
ui/app/scim/page.tsx (1)
5-9: LGTM! Layout standardization.The centered, width-constrained container aligns with the layout pattern applied consistently across multiple pages in this PR.
ui/app/adaptive-routing/page.tsx (1)
4-8: LGTM! Layout standardization.The centered, width-constrained container aligns with the layout pattern applied consistently across multiple pages in this PR.
ui/app/cluster/page.tsx (1)
4-8: LGTM! Layout standardization.The centered, width-constrained container aligns with the layout pattern applied consistently across multiple pages in this PR.
framework/configstore/tables/plugin.go (1)
20-20: LGTM! IsCustom field addition.The new
IsCustomfield is properly defined with appropriate GORM and JSON tags, integrates cleanly with existing serialization hooks, and supports the PR's custom plugin functionality.ui/app/virtual-keys/page.tsx (1)
73-80: LGTM! Consistent centered layout pattern.The addition of the centered container wrapper aligns with the standardized layout approach applied across multiple pages in this PR.
ui/lib/types/plugins.ts (2)
6-12: LGTM! Type extensions support custom plugin functionality.The addition of
isCustomandpathfields properly extends the Plugin interface to support the dynamic plugin architecture introduced in this PR.
19-24: LGTM! CreatePluginRequest appropriately requires path.The
pathfield is correctly required for plugin creation, aligning with the validation schema inaddNewPluginSheet.tsxwhere path is mandatory.ui/app/providers/views/modelProviderKeysTableView.tsx (1)
78-83: LGTM! Clean migration from Dialog to Sheet.The replacement of
AddNewKeyDialogwithAddNewKeySheetis implemented correctly, with all necessary props including thekeyIndexproperly passed through.
| func (s *BifrostHTTPServer) GetPluginsWithStatus() ([]schemas.Plugin, error) { | ||
| for _, plugin := range s.Plugins { | ||
| pluginStatus := map[string]any{ | ||
| Name: plugin.GetName(), | ||
| Status: "active", | ||
| } | ||
| } | ||
| return pluginStatus, nil | ||
| } |
There was a problem hiding this comment.
Fix critical issues in GetPluginsWithStatus method.
This method has multiple critical problems that will prevent compilation or cause runtime errors:
- Line 377:
Nameis used as an undefined identifier; it should be the string literal"Name" - Lines 376-379:
pluginStatusis created inside the loop but never accumulated—each iteration overwrites it - Line 381: Returns
pluginStatus(typemap[string]any) but the function signature declares([]schemas.Plugin, error)
Apply this diff to fix the issues:
-func (s *BifrostHTTPServer) GetPluginsWithStatus() ([]schemas.Plugin, error) {
+func (s *BifrostHTTPServer) GetPluginsWithStatus() ([]map[string]any, error) {
+ pluginStatuses := make([]map[string]any, 0, len(s.Plugins))
for _, plugin := range s.Plugins {
pluginStatus := map[string]any{
- Name: plugin.GetName(),
- Status: "active",
+ "name": plugin.GetName(),
+ "status": "active",
}
+ pluginStatuses = append(pluginStatuses, pluginStatus)
}
- return pluginStatus, nil
+ return pluginStatuses, nil
}| <Sheet open={show} onOpenChange={onCancel}> | ||
| <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> |
There was a problem hiding this comment.
Fix onOpenChange typing to avoid TS error and only close on dismiss.
onOpenChange expects (open: boolean) => void; passing onCancel directly can fail typing and reopen-close loops.
- <Sheet open={show} onOpenChange={onCancel}>
+ <Sheet
+ open={show}
+ onOpenChange={(open) => {
+ if (!open) onCancel();
+ }}
+ >📝 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.
| <Sheet open={show} onOpenChange={onCancel}> | |
| <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> | |
| <Sheet | |
| open={show} | |
| onOpenChange={(open) => { | |
| if (!open) onCancel(); | |
| }} | |
| > | |
| <SheetContent className="custom-scrollbar bg-white dark:bg-card min-w-[600px] py-4"> |
🤖 Prompt for AI Agents
In ui/app/providers/dialogs/addNewKeySheet.tsx around lines 20-21, onOpenChange
currently passes onCancel directly which mismatches the expected (open: boolean)
=> void signature and can cause reopen/close loops; change it to a properly
typed handler such as onOpenChange={(open: boolean) => { if (!open) onCancel();
}} so the sheet only calls onCancel when it is being dismissed (open === false)
and the handler matches the required boolean parameter type.
| <div className="font-lg flex items-center gap-2"> | ||
| <div className={"flex items-center"}> | ||
| <Provider provider={provider.name} size={24} />: | ||
| </div> | ||
| {dialogTitle} | ||
| </div> | ||
| </DialogTitle> | ||
| </DialogHeader> | ||
| <div> | ||
| </SheetTitle> |
There was a problem hiding this comment.
Tailwind class typo: use text-lg (and optional font weight).
font-lg isn’t a Tailwind class.
- <div className="font-lg flex items-center gap-2">
+ <div className="text-lg font-medium flex items-center gap-2">📝 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.
| <div className="font-lg flex items-center gap-2"> | |
| <div className={"flex items-center"}> | |
| <Provider provider={provider.name} size={24} />: | |
| </div> | |
| {dialogTitle} | |
| </div> | |
| </DialogTitle> | |
| </DialogHeader> | |
| <div> | |
| </SheetTitle> | |
| <div className="text-lg font-medium flex items-center gap-2"> | |
| <div className={"flex items-center"}> | |
| <Provider provider={provider.name} size={24} />: | |
| </div> | |
| {dialogTitle} | |
| </div> | |
| </SheetTitle> |
🤖 Prompt for AI Agents
In ui/app/providers/dialogs/addNewKeySheet.tsx around lines 24 to 30 the
Tailwind class "font-lg" is invalid; replace it with the correct class "text-lg"
(optionally add a weight like "font-medium" or "font-semibold" if desired) by
updating the div's className to "text-lg" (or "text-lg font-medium") so the
title size renders correctly.
akshaydeo
force-pushed
the
10-20-dynamic-plugins-ui
branch
from
1c19b16 to
7af0d58
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/configstore/migrations.go (1)
619-629: Add defensive checks in rollback to prevent errors.The rollback attempts to drop both columns without checking if they exist first. If the migration was partially applied or never run, the rollback will fail when trying to drop non-existent columns.
Apply this diff to add defensive checks:
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() - if err := migrator.DropColumn(&tables.TablePlugin{}, "path"); err != nil { - return err + if migrator.HasColumn(&tables.TablePlugin{}, "path") { + if err := migrator.DropColumn(&tables.TablePlugin{}, "path"); err != nil { + return err + } } - if err := migrator.DropColumn(&tables.TablePlugin{}, "is_custom"); err != nil { - return err + if migrator.HasColumn(&tables.TablePlugin{}, "is_custom") { + if err := migrator.DropColumn(&tables.TablePlugin{}, "is_custom"); err != nil { + return err + } } return nil },ui/next.config.ts (1)
23-31: Fix path mismatch in tsconfig and consolidate duplicate alias logic.Two issues found:
- Duplication: Both webpack and turbopack (lines 23-31, 39-46) define identical alias mappings—extract to constants to prevent drift.
- TypeScript path misalignment:
ui/tsconfig.jsondefines@schemasfallback as./app/_fallbacks/enterprise/lib/schemas/*, but this directory doesn't exist. The actual structure only haslib/withstore/subdirectories. Update tsconfig to match the actual directory structure or align next.config.ts accordingly.+const enterprisePath = haveEnterprise + ? path.join(__dirname, "app", "enterprise") + : path.join(__dirname, "app", "_fallbacks", "enterprise"); +const schemasPath = haveEnterprise + ? path.join(__dirname, "app", "enterprise", "lib", "schemas") + : path.join(__dirname, "app", "_fallbacks", "enterprise", "lib"); + webpack: (config) => { config.resolve = config.resolve || {}; config.resolve.alias = config.resolve.alias || {}; - config.resolve.alias["@enterprise"] = haveEnterprise - ? path.join(__dirname, "app", "enterprise") - : path.join(__dirname, "app", "_fallbacks", "enterprise"); - config.resolve.alias["@schemas"] = haveEnterprise - ? path.join(__dirname, "app", "enterprise", "lib", "schemas") - : path.join(__dirname, "app", "_fallbacks", "enterprise", "lib"); + config.resolve.alias["@enterprise"] = enterprisePath; + config.resolve.alias["@schemas"] = schemasPath; -turbopack: { - resolveAlias: { - "@enterprise": haveEnterprise ? path.join(__dirname, "app", "enterprise") : path.join(__dirname, "app", "_fallbacks", "enterprise"), - "@schemas": haveEnterprise - ? path.join(__dirname, "app", "enterprise", "lib", "schemas") - : path.join(__dirname, "app", "_fallbacks", "enterprise", "lib"), - }, -}, +turbopack: { + resolveAlias: { + "@enterprise": enterprisePath, + "@schemas": schemasPath, + }, +},Also verify and fix
ui/tsconfig.jsonpaths—the@schemasfallback references a non-existent/schemassubdirectory.
♻️ Duplicate comments (8)
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (1)
98-98: Fixed minimum width still causes mobile overflow.The
min-w-[600px]continues to cause horizontal overflow on mobile devices (360-428px viewports). The past review comment recommended using the responsive patternw-full flex-col overflow-x-hidden bg-white p-8 sm:max-w-2xlfound elsewhere in the codebase.ui/app/providers/dialogs/addNewKeySheet.tsx (3)
20-20: Incorrect onOpenChange handler type (previously flagged).The
onOpenChange={onCancel}has a type mismatch:onCancelis() => voidbutonOpenChangeexpects(open: boolean) => void. This can cause TypeScript errors and unexpected behavior.Note that
addNewCustomProviderSheet.tsx(line 97) uses the correct pattern:onOpenChange={(open) => !open && onClose()}. Apply the same fix here for consistency.
21-21: Fixed minimum width causes mobile overflow (previously flagged).The
min-w-[600px]will cause horizontal overflow on mobile devices (typical viewport: 360-428px). Past review suggested:w-full sm:min-w-[600px] sm:max-w-[600px]to ensure full width on mobile and fixed 600px on larger screens.
24-24: Invalid Tailwind class (previously flagged).
font-lgis not a valid Tailwind class. The correct class istext-lg(optionally withfont-mediumorfont-semiboldfor weight).Apply this diff:
- <div className="font-lg flex items-center gap-2"> + <div className="text-lg font-medium flex items-center gap-2">ui/app/plugins/fragments/pluginFormFragement.tsx (1)
34-34: Add rel="noopener noreferrer" to external link.Security risk with target="_blank" without rel. Patch below.
- } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" className="text-primary hover:underline">Learn more</a> + } <a href="https://docs.getbifrost.ai/plugins/building-bifrost-plugin" target="_blank" rel="noopener noreferrer" className="text-primary hover:underline">Learn more</a>Based on static analysis hints.
ui/app/plugins/page.tsx (1)
51-55: External link: add rel to prevent reverse‑tabnabbing.- <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank"> + <Link className="text-primary hover:underline dark:text-green-400" href="https://docs.getbifrost.ai/plugins" target="_blank" rel="noopener noreferrer">Based on static analysis hints.
ui/components/sidebar.tsx (2)
146-150: MCP naming mismatch: label vs route.Title “MCP Gateway” points to
/mcp-clients. Either keep label “MCP Clients” or rename the route/codebase consistently.- title: "MCP Gateway", + title: "MCP Clients",Or plan a full rename: route folder, URLs, headings, docs, tests.
405-411: Auto‑expand logic is a no‑op.
newExpandedItemsis never populated; parent sections won’t open on active subroutes.useEffect(() => { const newExpandedItems = new Set<string>(); + items.forEach((it) => { + if (it.subItems?.some((s) => pathname.startsWith(s.url))) { + newExpandedItems.add(it.title); + } + }); if (newExpandedItems.size > 0) { setExpandedItems((prev) => new Set([...prev, ...newExpandedItems])); } }, [pathname]);
🧹 Nitpick comments (17)
framework/configstore/migrations.go (1)
607-616: Minor: Clean up inconsistent spacing.Line 610 has unusual trailing whitespace or spacing. While this doesn't affect functionality, consistent formatting improves code readability.
Apply this diff to fix the spacing:
if !migrator.HasColumn(&tables.TablePlugin{}, "path") { if err := migrator.AddColumn(&tables.TablePlugin{}, "path"); err != nil { return err - } + } }ui/app/plugins/page.tsx (2)
36-44: Remove unnecessary preventDefault/stopPropagation on the button.This Button isn’t inside a Link/form; the handlers add noise.
- onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - handleAddNew(); - }} + onClick={handleAddNew}
31-35: Show a lightweight loading/empty state.Improve UX while plugins load or when none exist.
- {customPlugins?.map((plugin) => ( - <div key={plugin.name}>{plugin.name}</div> - ))} + {isLoading ? ( + <div className="text-xs text-muted-foreground">Loading plugins…</div> + ) : customPlugins && customPlugins.length > 0 ? ( + customPlugins.map((plugin) => <div key={plugin.name}>{plugin.name}</div>) + ) : ( + <div className="text-xs text-muted-foreground">No custom plugins yet.</div> + )}ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
9-13: Mark decorative icon as aria-hidden.Icon is purely visual; hide from assistive tech.
- icon={<Siren className="h-[5.5rem] w-[5.5rem]" strokeWidth={1} />} + icon={<Siren aria-hidden className="h-[5.5rem] w-[5.5rem]" strokeWidth={1} />}ui/app/alert-channels/page.tsx (1)
3-8: LGTM on route and wrapper.Optional: add page metadata for title to improve UX/SEO.
+export const metadata = { title: "Alert channels • Bifrost" };ui/components/sidebar.tsx (3)
63-66: Update icon a11y text to match label or make decorative.Current aria/title say “MCP clients icon” while label is “MCP Gateway”. Prefer decorative icon or synced text.
- xmlns="http://www.w3.org/2000/svg" - aria-label="MCP clients icon" + xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" @@ - <title>MCP clients icon</title> + {/* decorative */}
311-327: Avoidanyfor subitems; add a minimal SubItem type.Improves safety and IntelliSense.
- {item.subItems?.map((subItem: any) => { + type SubItem = { title: string; url: string; icon?: React.ComponentType<{ className?: string }>; description?: string }; + {item.subItems?.map((subItem: SubItem) => {Optionally type
itemswith this shape to propagate types.
284-294: Expose disabled state to AT.Add
aria-disabledwhen navigation is disallowed.- <SidebarMenuButton + <SidebarMenuButton className={`relative h-7.5 cursor-pointer rounded-md border px-3 transition-all duration-200 ${ ... } `} - onClick={hasSubItems ? handleClick : () => handleNavigation(item.url)} + aria-disabled={!isAllowed} + onClick={hasSubItems ? handleClick : () => handleNavigation(item.url)} >ui/app/virtual-keys/page.tsx (2)
26-39: Handle config fetch errors explicitly.Use unwrap + catch to surface failures; current code ignores network/500s.
- useEffect(() => { - triggerGetConfig({ fromDB: true }).then((res) => { - if (res.data && res.data.client_config.enable_governance) { + useEffect(() => { + triggerGetConfig({ fromDB: true }) + .unwrap() + .then((data) => { + if (data?.client_config?.enable_governance) { setGovernanceEnabled(true); triggerGetVirtualKeys(); triggerGetTeams({}); triggerGetCustomers(); - } else { + } else { setGovernanceEnabled(false); toast.error("Governance is not enabled. Please enable it in the config."); - } - }); + } + }) + .catch((e) => toast.error(getErrorMessage(e))); }, [triggerGetConfig, triggerGetVirtualKeys, triggerGetTeams, triggerGetCustomers]);
72-81: Show a clear empty‑state when governance is disabled.Rendering an empty table after a toast is confusing. Gate the view.
-return ( - <div className="mx-auto w-full max-w-7xl"> - <VirtualKeysTable - virtualKeys={virtualKeysData?.virtual_keys || []} - teams={teamsData?.teams || []} - customers={customersData?.customers || []} - onRefresh={handleRefresh} - /> - </div> -); +return ( + <div className="mx-auto w-full max-w-7xl"> + {governanceEnabled ? ( + <VirtualKeysTable + virtualKeys={virtualKeysData?.virtual_keys || []} + teams={teamsData?.teams || []} + customers={customersData?.customers || []} + onRefresh={handleRefresh} + /> + ) : ( + <div className="text-muted-foreground rounded-md border p-6"> + Governance is not enabled. Enable it in Settings to manage Virtual Keys. + </div> + )} + </div> +);core/schemas/plugin.go (1)
14-23: Type the status enum and omit empty logs.Improves compile‑time safety and JSON ergonomics.
-// PluginStatus constants -const ( - PluginStatusActive = "active" - PluginStatusError = "error" - PluginStatusDisabled = "disabled" - PluginStatusLoading = "loading" - PluginStatusUninitialized = "uninitialized" - PluginStatusUnloaded = "unloaded" - PluginStatusLoaded = "loaded" -) +// PluginStatusValue is the canonical status type for plugins. +type PluginStatusValue string +const ( + PluginStatusActive PluginStatusValue = "active" + PluginStatusError PluginStatusValue = "error" + PluginStatusDisabled PluginStatusValue = "disabled" + PluginStatusLoading PluginStatusValue = "loading" + PluginStatusUninitialized PluginStatusValue = "uninitialized" + PluginStatusUnloaded PluginStatusValue = "unloaded" + PluginStatusLoaded PluginStatusValue = "loaded" +) // PluginStatus represents the status of a plugin. type PluginStatus struct { Name string `json:"name"` - Status string `json:"status"` - Logs []string `json:"logs"` + Status PluginStatusValue `json:"status"` + Logs []string `json:"logs,omitempty"` }Also applies to: 27-30
ui/app/observability/views/observabilityView.tsx (1)
82-90: DRY up default plugin construction.Build a small helper to avoid duplicating the fallback object.
function fallbackPlugin(name: string) { return { name, enabled: false, config: {}, isCustom: false, path: "" }; }Then:
- const plugin = plugins.find((p) => p.name === selectedPluginId) ?? { - name: selectedPluginId, enabled: false, config: {}, isCustom: false, path: "", - }; + const plugin = plugins.find((p) => p.name === selectedPluginId) ?? fallbackPlugin(selectedPluginId);Also applies to: 96-104
transports/bifrost-http/handlers/server.go (5)
663-666: Move the lock to cover only assignment, not plugin loading.Holding the write lock during LoadPlugins is unnecessary and blocks readers longer.
- s.pluginStatusMutex.Lock() - defer s.pluginStatusMutex.Unlock() - s.Plugins, s.pluginStatus, err = LoadPlugins(ctx, s.Config) + plugins, statuses, err := LoadPlugins(ctx, s.Config) if err != nil { return fmt.Errorf("failed to load plugins %v", err) } + s.pluginStatusMutex.Lock() + s.Plugins, s.pluginStatus = plugins, statuses + s.pluginStatusMutex.Unlock()
650-651: Mutex re-init is unnecessary.
sync.RWMutex{}zero value is already valid; reassigning can surprise future readers.- s.pluginStatusMutex = sync.RWMutex{} + // no-op: zero value of sync.RWMutex is ready to use
436-454: Validate status input and keep one entry per plugin.Add a quick guard to reject invalid statuses.
func (s *BifrostHTTPServer) UpdatePluginStatus(name string, status string, logs []string) error { + switch status { + case string(schemas.PluginStatusActive), string(schemas.PluginStatusError), + string(schemas.PluginStatusDisabled), string(schemas.PluginStatusLoading), + string(schemas.PluginStatusUninitialized), string(schemas.PluginStatusUnloaded), + string(schemas.PluginStatusLoaded): + // ok + default: + return fmt.Errorf("invalid plugin status: %s", status) + } s.pluginStatusMutex.Lock() defer s.pluginStatusMutex.Unlock() // Remove plugin status if already exists
493-499: Mark plugin as active after successful reload.Reflect success in status to keep UI in sync.
- s.Plugins = newPlugins // Keep BifrostHTTPServer.Plugins in sync - return nil + s.Plugins = newPlugins // Keep BifrostHTTPServer.Plugins in sync + _ = s.UpdatePluginStatus(name, schemas.PluginStatusActive, []string{"plugin reloaded successfully"}) + return nil
60-66: Provide a read-only getter for plugin statuses.Expose a copy under RLock to avoid races and slice aliasing.
Add this method somewhere in this file:
// GetPluginStatuses returns a snapshot copy of plugin statuses. func (s *BifrostHTTPServer) GetPluginStatuses() []schemas.PluginStatus { s.pluginStatusMutex.RLock() defer s.pluginStatusMutex.RUnlock() out := make([]schemas.PluginStatus, len(s.pluginStatus)) copy(out, s.pluginStatus) return out }If helpful, I can thread this into the Plugins handler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
framework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
core/schemas/plugin.go(1 hunks)framework/configstore/migrations.go(2 hunks)framework/configstore/rdb.go(2 hunks)framework/configstore/tables/plugin.go(1 hunks)framework/go.mod(1 hunks)plugins/logging/go.mod(2 hunks)plugins/otel/docker-compose.yml(1 hunks)plugins/otel/go.mod(1 hunks)transports/bifrost-http/handlers/inference.go(1 hunks)transports/bifrost-http/handlers/plugins.go(3 hunks)transports/bifrost-http/handlers/server.go(8 hunks)transports/bifrost-http/lib/config.go(1 hunks)ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx(1 hunks)ui/app/adaptive-routing/page.tsx(1 hunks)ui/app/alert-channels/page.tsx(1 hunks)ui/app/audit-logs/page.tsx(1 hunks)ui/app/cluster/page.tsx(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/layout.tsx(1 hunks)ui/app/logs/page.tsx(1 hunks)ui/app/logs/views/logsTable.tsx(2 hunks)ui/app/mcp-clients/page.tsx(1 hunks)ui/app/observability/page.tsx(1 hunks)ui/app/observability/views/observabilityView.tsx(2 hunks)ui/app/plugins/fragments/pluginFormFragement.tsx(1 hunks)ui/app/plugins/page.tsx(1 hunks)ui/app/plugins/sheets/addNewPluginSheet.tsx(1 hunks)ui/app/prompt-repo/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderSheet.tsx(5 hunks)ui/app/providers/dialogs/addNewKeySheet.tsx(3 hunks)ui/app/providers/page.tsx(3 hunks)ui/app/providers/views/modelProviderConfig.tsx(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(2 hunks)ui/app/providers/views/providerKeyForm.tsx(5 hunks)ui/app/scim/page.tsx(1 hunks)ui/app/user-groups/page.tsx(1 hunks)ui/app/virtual-keys/page.tsx(2 hunks)ui/components/sidebar.tsx(13 hunks)ui/components/ui/promoCardStack.tsx(3 hunks)ui/lib/types/plugins.ts(2 hunks)ui/next.config.ts(1 hunks)ui/package.json(1 hunks)ui/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (22)
- ui/app/logs/page.tsx
- ui/app/adaptive-routing/page.tsx
- framework/configstore/rdb.go
- ui/app/guardrails/page.tsx
- transports/bifrost-http/lib/config.go
- ui/app/providers/page.tsx
- transports/bifrost-http/handlers/plugins.go
- ui/app/user-groups/page.tsx
- ui/lib/types/plugins.ts
- ui/components/ui/promoCardStack.tsx
- ui/app/providers/views/providerKeyForm.tsx
- ui/app/audit-logs/page.tsx
- framework/configstore/tables/plugin.go
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/prompt-repo/page.tsx
- ui/app/config/views/pluginsForm.tsx
- ui/app/scim/page.tsx
- ui/app/cluster/page.tsx
- ui/app/logs/views/logsTable.tsx
- plugins/otel/docker-compose.yml
- ui/app/config/page.tsx
- ui/app/plugins/sheets/addNewPluginSheet.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
ui/app/plugins/page.tsx (2)
ui/lib/types/plugins.ts (1)
Plugin(6-12)ui/app/plugins/sheets/addNewPluginSheet.tsx (1)
AddNewPluginSheet(60-174)
ui/components/sidebar.tsx (1)
ui/components/ui/promoCardStack.tsx (1)
PromoCardStack(21-120)
ui/app/virtual-keys/page.tsx (1)
ui/app/virtual-keys/views/virtualKeysTable.tsx (1)
VirtualKeysTable(34-230)
framework/configstore/migrations.go (1)
framework/configstore/tables/plugin.go (2)
TablePlugin(12-24)TablePlugin(27-27)
transports/bifrost-http/handlers/server.go (6)
core/schemas/plugin.go (5)
Plugin(63-89)PluginStatus(26-30)PluginStatusError(17-17)PluginStatusActive(16-16)PluginStatusDisabled(18-18)core/bifrost.go (1)
Bifrost(34-51)transports/bifrost-http/lib/config.go (1)
Config(136-166)plugins/governance/main.go (2)
Config(29-31)PluginName(20-20)plugins/telemetry/main.go (2)
PrometheusPlugin(30-44)PluginName(18-18)plugins/logging/main.go (1)
PluginName(21-21)
ui/app/plugins/fragments/pluginFormFragement.tsx (2)
ui/components/ui/input.tsx (1)
Input(15-69)ui/app/logs/views/codeEditor.tsx (1)
CodeEditor(82-245)
ui/app/observability/page.tsx (1)
ui/app/observability/views/observabilityView.tsx (1)
ObservabilityView(67-168)
ui/app/alert-channels/page.tsx (1)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
AlertChannelsView(4-16)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/app/providers/dialogs/addNewKeySheet.tsx (1)
AddNewKeySheet(14-46)
ui/app/_fallbacks/enterprise/components/alert-channels/alertChannelsView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
🪛 Biome (2.1.2)
ui/app/plugins/fragments/pluginFormFragement.tsx
[error] 34-34: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (15)
plugins/logging/go.mod (1)
8-8: Low-risk dependency updates; clarify alignment with PR objectives.The sonic v1.14.1 release (Sep 2, 2025) includes bug fixes for boundary pointer handling, TextMarshaler key encoding, UTF-8 validation, and JSON decoding with null characters. Sonic shows 0 unresolved vulnerabilities and no reported dependency vulnerabilities. The new indirect dependency on gopkg v0.1.3 (published Aug 13, 2025) is Apache-2.0 licensed.
Both changes are low-risk and appear appropriate for dependency maintenance. However, the PR objectives describe UI/Next.js changes for the admin sidebar navigation. It's unclear why a Go module update for the logging plugin is included here. Consider clarifying whether this change:
- Is part of a broader plugin architecture refactor (mentioned in the PR stack context),
- Was included accidentally, or
- Needs to be merged as part of the Graphite stack.
Also applies to: 33-33
ui/tsconfig.json (2)
44-45: Generated types path addition is well-motivated.The inclusion of
"out/dev/types/**/*.ts"aligns with the PR's new plugin architecture and dynamic type generation. Ensure the build process creates this directory and that no unintended files are included in type checking.
18-18: All concerns verified—no action needed.React 19.1.1 fully supports the
react-jsxruntime (required React 17+), and no legacyimport React from 'react'statements were found in the codebase. The JSX configuration change is safe and compatible.framework/go.mod (2)
1-17: Significant mismatch between PR objectives and file changes.The PR is described as a UI-only feature for Next.js sidebar navigation (Plugins menu, Alert channels, icons), but this file contains backend Go dependency version upgrades. The AI summary mentions plugin path/isCustom DB fields and server-side tracking, suggesting infrastructure changes beyond UI.
This requires clarification on whether the backend changes are intentional and properly scoped.
Can you confirm whether this PR should include backend infrastructure changes, or if this file was inadvertently included?
10-10: I need to verify if these dependency updates introduce breaking changes. Let me search for release information.Dependencies are compatible and secure. Critical breaking change in weaviate-go-client v5 has been addressed.
The redis/go-redis update adjusts buffer sizes from 512KiB to 256KiB with no API breaking changes. Weaviate v1.31.0 has no breaking changes, and v1.33.1 is a minor release.
For weaviate-go-client: A breaking change exists in v5.0+ where models.Vector changed from []float32 to interface{} to support multi-dimensional vectors. Since the PR uses v5.5.0, this change is already past that breaking change. Verify that your codebase handles the Vector type correctly if you have direct Vector type usage.
Security status: redis/go-redis v9.14.0 is outside all reported vulnerable ranges; weaviate v1.33.1 has no known vulnerabilities in the queried advisories; weaviate-go-client shows no security advisories.
ui/app/providers/dialogs/addNewCustomProviderSheet.tsx (2)
5-5: LGTM: Clean migration to Sheet-based component.The import changes and function renaming are consistent with the Dialog → Sheet migration pattern seen across the codebase.
Also applies to: 37-37, 41-41
104-104: LGTM: Proper Sheet layout structure.The padding and footer layout align with standard Sheet component patterns.
Also applies to: 163-170
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
13-23: LGTM: Correct integration of AddNewKeySheet.The component is properly integrated with all required props (
show,onCancel,provider,keyIndex) correctly passed. The state management at line 32 appropriately tracks the keyIndex for both add and edit operations.Also applies to: 78-83
plugins/otel/go.mod (1)
10-10: Clarify whether the gRPC dependency bump belongs in this PR.The gRPC v1.76.0 bump is compatible with your project's Go version requirement (Go 1.24.1 already satisfies the v1.76.0 minimum Go 1.24 requirement), and the release introduces no critical breaking changes—only new JWT credentials, bug fixes, and performance improvements.
However, this PR's objectives describe UI-only changes (sidebar navigation, icons from lucide-react), yet line 10 modifies a Go module dependency. Clarify whether:
- This is part of a larger stack change managed by Graphite, or
- This dependency update should be in a separate PR.
ui/app/plugins/fragments/pluginFormFragement.tsx (1)
7-7: Remove review comment — PlusIcon is a valid lucide-react export.Lucide includes both prefixed and suffixed names to use within your project to prevent import name collisions with other libraries or your own code. Both
import { Camera } from "lucide-react"andimport { CameraIcon } from "lucide-react"reference the same icon file. The import statement usingPlusIconwill not cause build failures.ui/package.json (1)
49-49: Verify Node.js version meets minimum requirements (>= 20.9.0) and test build/dev server locally before merging.The upgrade to Next.js 16.0.0 appears compatible with your current dependency tree:
- React 19.1.1 is officially supported by Next.js 16
- Radix UI has official React 19 support, so all radix-ui components in your dependencies are compatible
- Your
next.config.tsuses static export mode (output: "export"), which avoids most breaking changes related to runtime APIs and middleware- No deprecated patterns detected (no
serverRuntimeConfig/publicRuntimeConfig, no legacy middleware)- No usage of APIs requiring async/await refactoring (cookies, headers, params, draftMode)
However, verify:
- Node.js >= 20.9.0 and TypeScript >= 5.1.0 are enforced in your CI/deployment environment (not pinned in the codebase)
- Run
npm installand test the dev server and build locally to catch any transitive dependency or configuration issuesui/app/observability/page.tsx (1)
4-8: LGTM.Consistent container wrapper with other pages.
ui/next.config.ts (1)
39-46: Turbopack config is supported — no changes needed.Next.js supports a top-level
turbopack.resolveAliasin next.config.js/ts, which accepts string replacements. The code correctly maps aliases to strings viapath.join(), matching the expected shape. No validation required.ui/app/mcp-clients/page.tsx (1)
29-31: LGTM: centered container wrapper aligns with new layout system.ui/app/observability/views/observabilityView.tsx (1)
7-9: All referenced images are present and accessible.Verification confirms that all image assets referenced in observabilityView.tsx exist in the public directory:
/maxim-logo.png✓/maxim-logo-dark.png✓/images/datadog-logo.png✓No runtime 404 risks for the Image component usage on lines 50 and 54.
| func migrationAddPluginPathColumn(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| ID: "add_plugin_path_column", | ||
| ID: "update_plugins_table_for_custom_plugins", |
There was a problem hiding this comment.
🧩 Analysis chain
CRITICAL: Verify migration ID change impact before merging.
Changing the migration ID from "add_plugin_path_column" to "update_plugins_table_for_custom_plugins" will cause the migration system to treat this as a completely new migration. If the old ID was already executed in any environment (dev, staging, production), the system will attempt to run this migration again, potentially causing conflicts even with the HasColumn checks.
Please verify:
- Has the migration with ID
"add_plugin_path_column"been deployed to any environment? - If yes, you'll need a different approach (e.g., a separate additive migration for the
is_customcolumn) - If no, document that this change is safe because the old ID was never released
Run this script to check if the old migration ID exists in the migration history:
🏁 Script executed:
#!/bin/bash
# Search for any references to the old migration ID in the codebase and git history
echo "=== Checking for old migration ID in current codebase ==="
rg -n "add_plugin_path_column" --type go
echo -e "\n=== Checking git history for when old migration ID was introduced ==="
git log --all --oneline --grep="add_plugin_path_column" || echo "No commits found with old migration ID in message"
echo -e "\n=== Checking if old migration ID exists in git history ==="
git log -p --all -S "add_plugin_path_column" -- "*.go" | head -50Length of output: 188
🏁 Script executed:
#!/bin/bash
# Check the git history of the migrations.go file
echo "=== Git history of migrations.go file ==="
git log --oneline framework/configstore/migrations.go | head -20
echo -e "\n=== When was this specific migration function added? ==="
git log -p --all -- framework/configstore/migrations.go | grep -A5 -B5 "migrationAddPluginPathColumn" | head -40
echo -e "\n=== Current state of the function around line 600-630 ==="
sed -n '600,630p' framework/configstore/migrations.goLength of output: 306
🏁 Script executed:
#!/bin/bash
# Check the git history of the migrations.go file
echo "=== Git history of migrations.go file ==="
git log --oneline framework/configstore/migrations.go | head -20
echo -e "\n=== When was this specific migration function added? ==="
git log -p --all -- framework/configstore/migrations.go | grep -A10 -B5 "migrationAddPluginPathColumn" | head -50
echo -e "\n=== Current state of the function around line 600-636 ==="
sed -n '600,636p' framework/configstore/migrations.go
echo -e "\n=== Check for rollback defensive checks ==="
sed -n '619,629p' framework/configstore/migrations.goLength of output: 2291
🏁 Script executed:
#!/bin/bash
# Search for the old migration ID across entire git history
echo "=== Search for any migration with old ID 'add_plugin_path_column' across all history ==="
git log -p --all -S "add_plugin_path_column" -- framework/configstore/migrations.go | head -100
echo -e "\n=== Check when migrationAddPluginPathColumn was first added ==="
git log --all --source --remotes --branches -p -- framework/configstore/migrations.go | grep -A30 "func migrationAddPluginPathColumn" | head -50
echo -e "\n=== Check if this function exists in main/master branch ==="
git show origin/main:framework/configstore/migrations.go 2>/dev/null | grep -A5 "migrationAddPluginPathColumn" || echo "Function not found in main branch"Length of output: 2118
CRITICAL: Migration ID change will re-execute already-deployed migration in production.
The git history confirms that the migration ID "add_plugin_path_column" was already deployed. Changing it to "update_plugins_table_for_custom_plugins" will cause the migration system to treat this as a completely new migration and attempt to execute it again across all environments, even though the HasColumn checks exist.
Action required: Either:
- Revert the ID change to
"add_plugin_path_column"and create a separate new migration for theis_customcolumn, OR - Confirm this migration was never actually deployed and document why the ID change is safe
Secondary issue: The rollback logic lacks defensive checks. Add HasColumn guards before DropColumn calls (lines 625–627) to prevent failures if the migration partially executed.
🤖 Prompt for AI Agents
In framework/configstore/migrations.go around line 603 (where ID:
"update_plugins_table_for_custom_plugins" is set) revert the migration ID back
to the original "add_plugin_path_column" unless you can prove this migration was
never deployed and document that justification; if you need to add the new
is_custom column, create a separate migration file with a new unique ID instead
of renaming the existing one. Additionally, in the rollback section around lines
625–627, add defensive HasColumn checks before each DropColumn call so the
rollback only attempts to drop columns that exist, preventing failures from
partially-applied migrations.
| if !migrator.HasColumn(&tables.TablePlugin{}, "is_custom") { | ||
| if err := migrator.AddColumn(&tables.TablePlugin{}, "is_custom"); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify default value is applied to existing rows.
When adding the is_custom column, existing plugin rows should default to false. The GORM struct tag gorm:"default:false" should handle this, but the behavior can vary across database engines.
Consider running this verification script after the migration to confirm existing rows have is_custom = false:
🏁 Script executed:
#!/bin/bash
# Verify that all existing plugins have is_custom set to false after migration
# This requires database access credentials to be configured
echo "This script requires manual execution with database access"
echo "Example SQL to verify after migration:"
echo ""
echo "SELECT name, is_custom, path FROM config_plugins WHERE is_custom IS NULL OR is_custom = true;"
echo ""
echo "Expected: No rows returned (all existing plugins should have is_custom = false)"Length of output: 273
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the is_custom field definition in tables/plugin.go
fd -type f -name "plugin.go" --path "*/tables/*" --exec cat {} \; | grep -A5 -B5 "is_custom"
Length of output: 170
🏁 Script executed:
#!/bin/bash
# View the migration code context around lines 612-616
cat framework/configstore/migrations.go | sed -n '605,625p'Length of output: 694
🏁 Script executed:
#!/bin/bash
# Check if there are any explicit ALTER statements or default value assignments in the migration
rg -n "is_custom" framework/configstore/migrations.go -B3 -A3Length of output: 483
---
🏁 Script executed:
```shell
#!/bin/bash
# Find plugin.go in tables directory
fd -type f -name "plugin.go"
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Once found, search for is_custom field definition with GORM tags
rg -n "is_custom" --type go -B2 -A2 | grep -A2 -B2 "is_custom"Length of output: 750
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for TablePlugin struct definition in Go files
rg -n "type TablePlugin" -A30 | head -60
Length of output: 2320
🏁 Script executed:
#!/bin/bash
# Search for is_custom across all files to find the struct definition
find . -name "*.go" -type f | xargs grep -l "is_custom" | head -10Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look for tables directory and plugin definitions
find . -path "*/tables/*.go" -type f 2>/dev/null | head -20Length of output: 802
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the full migration block around is_custom to see if there's an UPDATE statement
rg -n "is_custom" framework/configstore/migrations.go -B10 -A10
Length of output: 1157
🏁 Script executed:
#!/bin/bash
# Look for UPDATE statements in the migration function around line 612
sed -n '600,635p' framework/configstore/migrations.goLength of output: 1170
| test, _ := sonic.Marshal(config.PluginConfigs) | ||
| logger.Debug("loading plugins from config %s", string(test)) | ||
| var err error |
There was a problem hiding this comment.
Don’t log raw plugin configs; may leak secrets.
sonic.Marshal(config.PluginConfigs) and the debug log can expose credentials/paths.
- test, _ := sonic.Marshal(config.PluginConfigs)
- logger.Debug("loading plugins from config %s", string(test))
+ // Avoid logging raw plugin configs to prevent credential leakage.
+ logger.Debug("loading plugins from config (redacted)")📝 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.
| test, _ := sonic.Marshal(config.PluginConfigs) | |
| logger.Debug("loading plugins from config %s", string(test)) | |
| var err error | |
| // Avoid logging raw plugin configs to prevent credential leakage. | |
| logger.Debug("loading plugins from config (redacted)") | |
| var err error |
🤖 Prompt for AI Agents
In transports/bifrost-http/handlers/server.go around lines 284 to 286, the code
marshals and debug-logs the entire PluginConfigs which can leak secrets; remove
the raw marshal-and-log or replace it with a safe redaction step: build a
shallow copy of config.PluginConfigs that omits or masks known sensitive keys
(e.g., "password", "secret", "token", "key", "credentials", "access_key",
"secret_key"), marshal and log that redacted copy or simply log only plugin
names/keys, and ensure no raw credential values are printed.
| <Sidebar /> | ||
| <div className="dark:bg-card custom-scrollbar my-[1rem] h-[calc(100dvh-2rem)] w-full overflow-auto rounded-md border border-gray-200 bg-white dark:border-zinc-800"> | ||
| <main className="custom-scrollbar relative mx-auto flex w-5xl flex-col px-4 py-12 2xl:w-7xl"> | ||
| <main className="custom-scrollbar relative mx-auto flex flex-col p-4"> |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid nested scroll areas; remove duplicate class on
.Both wrapper and main have custom-scrollbar. Keep it on the scroll container only.
- <main className="custom-scrollbar relative mx-auto flex flex-col p-4">
+ <main className="relative mx-auto flex flex-col p-4">Quick check to ensure every page is now self-wrapping (so removing old global max-width won’t make content edge-to-edge):
🏁 Script executed:
#!/bin/bash
# Lists app pages missing the max-w-7xl wrapper
fd -a -t f 'page.tsx' ui/app | while read -r f; do
if ! rg -n 'max-w-7xl' "$f" >/dev/null; then
echo "Missing wrapper: $f"
fi
doneLength of output: 259
Avoid nested scroll areas AND wrap these 4 pages with max-w-7xl.
Removing custom-scrollbar from <main> is correct (keep it only on scroll container). However, verification found 4 pages still missing the max-w-7xl wrapper required after global max-width removal:
ui/app/guardrails/configuration/page.tsxui/app/guardrails/providers/page.tsxui/app/login/page.tsxui/app/page.tsx
Add max-w-7xl mx-auto wrapper to each page's content container before merging.
- <main className="custom-scrollbar relative mx-auto flex flex-col p-4">
+ <main className="relative mx-auto flex flex-col p-4">📝 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.
| <main className="custom-scrollbar relative mx-auto flex flex-col p-4"> | |
| <main className="relative mx-auto flex flex-col p-4"> |
🤖 Prompt for AI Agents
In ui/app/layout.tsx around line 41, remove the custom-scrollbar class from the
<main> element so the global scroll container is the only scroll area (avoid
nested scrollbars); then, for each of the four pages listed
(ui/app/guardrails/configuration/page.tsx, ui/app/guardrails/providers/page.tsx,
ui/app/login/page.tsx, ui/app/page.tsx) wrap the page content container with a
div that adds the layout constraint classes max-w-7xl mx-auto (place this
wrapper as the outermost content container inside the page so content is
centered and constrained to max width).
| @@ -0,0 +1,131 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Rename file to pluginFormFragment.tsx for consistency.
Typo in filename (“Fragement”). Rename file and update imports to improve discoverability and avoid future import mistakes.
🏁 Script executed:
#!/bin/bash
# Find all imports to update in a rename
rg -n "pluginFormFragement" -SLength of output: 117
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any other references to the file (including re-exports, test files, etc.)
rg -n "pluginFormFragement" --type-list | head -20
rg -n "pluginFormFragement" -g '!node_modules' 2>/dev/null || rg -n "pluginFormFragement"
Length of output: 507
🏁 Script executed:
#!/bin/bash
# Alternative broader search for the filename or component
rg "pluginForm" -nLength of output: 393
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for test files or other file extensions that might reference the fragment
rg "pluginFormFragement" -g '*.test.*' -g '*.spec.*' 2>/dev/null || echo "No test files found with reference"
# Also check for barrel exports or index files
fd "index\.(ts|tsx|js|jsx)" -x rg -l "pluginFormFragement" {} \;
Length of output: 34
Rename file to pluginFormFragment.tsx to fix typo and update import.
Confirmed one import location that requires update:
ui/app/plugins/sheets/addNewPluginSheet.tsx:13: Change import path from"../fragments/pluginFormFragement"to"../fragments/pluginFormFragment"
🤖 Prompt for AI Agents
In ui/app/plugins/fragments/pluginFormFragement.tsx around line 1 the filename
contains a typo; rename the file to pluginFormFragment.tsx and update its
default/relative export path; then update the import in
ui/app/plugins/sheets/addNewPluginSheet.tsx line 13 to import from
"../fragments/pluginFormFragment" (ensure any other imports/exports referencing
the old name are updated accordingly).
| const [showConfig, setShowConfig] = useState(form.getValues("hasConfig") || false); | ||
|
|
There was a problem hiding this comment.
Fix state drift between showConfig and form.hasConfig.
showConfig is initialized once from form and won’t reflect later form.reset()/watch updates (e.g., editing a plugin with existing config). This can hide the editor incorrectly. Minimal sync fix below; preferred approach removes duplicate state.
Option A — minimal sync (keep state):
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
-const [showConfig, setShowConfig] = useState(form.getValues("hasConfig") || false);
+const [showConfig, setShowConfig] = useState(form.getValues("hasConfig") || false);
+const hasConfig = form.watch("hasConfig");
+useEffect(() => {
+ setShowConfig(!!hasConfig);
+}, [hasConfig]);Option B — preferred (remove local state; derive from form):
-import { useState } from "react";
+// no local state needed
@@
-const [showConfig, setShowConfig] = useState(form.getValues("hasConfig") || false);
+const showConfig = !!form.watch("hasConfig");
@@
- onClick={() => {
- setShowConfig(true);
- form.setValue("hasConfig", true);
- if (!form.getValues("config")) {
- form.setValue("config", "{}");
- }
- }}
+ onClick={() => {
+ form.setValue("hasConfig", true, { shouldDirty: true });
+ if (!form.getValues("config")) form.setValue("config", "{}", { shouldDirty: true });
+ }}
@@
- onClick={() => {
- setShowConfig(false);
- form.setValue("hasConfig", false);
- form.setValue("config", undefined);
- }}
+ onClick={() => {
+ form.setValue("hasConfig", false, { shouldDirty: true });
+ form.setValue("config", undefined, { shouldDirty: true });
+ }}Also applies to: 66-83, 91-103
| "moment": "2.30.1", | ||
| "monaco-editor": "0.52.2", | ||
| "next": "15.5.3", | ||
| "next": "16.0.0", |
There was a problem hiding this comment.
Fix version mismatch between next and eslint-config-next.
The next package has been upgraded to 16.0.0, but eslint-config-next on line 76 is still pinned to 15.5.2. This version mismatch can lead to linting inconsistencies and compatibility issues.
Update eslint-config-next to match the Next.js version:
- "eslint-config-next": "15.5.2",
+ "eslint-config-next": "16.0.0",Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ui/package.json around lines 49 and 76, Next.js is upgraded to 16.0.0 but
eslint-config-next remains at 15.5.2; update the eslint-config-next dependency
to match Next.js (set it to 16.0.0 or a matching semver range), save the
package.json change and reinstall dependencies (npm/yarn install) so the
lockfile is updated and the linter uses the compatible config.
1 participant
Summary
Updated the sidebar navigation with new menu items and renamed an existing one to improve the admin interface organization.
Changes
Type of change
Affected areas
How to test
Screenshots/Recordings
[Add screenshots of the updated sidebar navigation]
Breaking changes
Related issues
[Link to related issue if applicable]
Security considerations
No security implications as this is a UI-only change affecting navigation labels and icons.
Checklist
docs/contributing/README.mdand followed the guidelines