feat: adds governance frontend for models and providers #1170
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
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds governance support: RBAC resource, UI for Model Limits and provider governance (views, table, form, sheet), RTK Query endpoints/types for model-configs and provider-governance, reset-duration labels constant, and a sidebar Model Limits entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser UI
participant RTK as RTK Query (governanceApi)
participant API as Backend / Governance API
participant Toast as Toasts
rect rgb(245,250,255)
Note over Browser,RTK: Create or Update Model Config flow
end
Browser->>Browser: Open Model Limit Sheet (form)
Browser->>RTK: call createModelConfig / updateModelConfig mutation
activate RTK
RTK->>API: POST/PUT /governance/model-configs
API-->>RTK: 200 { model_config }
RTK-->>Browser: mutation fulfilled
deactivate RTK
Browser->>Toast: show success
Browser->>RTK: invalidate/refetch useGetModelConfigsQuery
activate RTK
RTK->>API: GET /governance/model-configs
API-->>RTK: 200 { model_configs }
RTK-->>Browser: updated model configs (re-render table)
deactivate RTK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 4
🧹 Nitpick comments (8)
ui/app/workspace/model-limits/layout.tsx (1)
1-3: Consider removing this pass-through layout.The current layout implementation is a simple pass-through that renders
childreninside a fragment. In Next.js App Router, layout files can be omitted entirely if they don't add any wrapping logic. Unless you plan to add shared UI or logic here in the future, this file could be removed to simplify the codebase.Alternative: Remove the file entirely
Next.js will use the default layout behavior if this file is absent, achieving the same result with less code.
ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
20-35: Duplicate helper function across files.
formatResetDurationis duplicated inmodelLimitsTable.tsx(line 33-35). Consider extracting this to a shared utility in@/lib/utils/governancealongsideformatCurrency.ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
59-63: Consider checkinghasGovernanceAccessbefore refetching.The
handleRefreshfunction only checksgovernanceEnabledbut the query also depends onhasGovernanceAccess. For consistency, both should be checked, though the query'sskipoption would prevent actual fetching anyway.🔎 Proposed fix
const handleRefresh = () => { - if (governanceEnabled) { + if (governanceEnabled && hasGovernanceAccess) { refetchModelConfigs(); } };ui/app/workspace/model-limits/views/modelLimitSheet.tsx (1)
34-43: Consider adding validation for at least one limit type.The schema allows submitting a model limit with only a model name and no budget or rate limits configured. This might create a governance config that doesn't actually govern anything.
🔎 Proposed validation enhancement
const formSchema = z.object({ modelName: z.string().min(1, "Model name is required"), provider: z.string().optional(), budgetMaxLimit: z.string().optional(), budgetResetDuration: z.string().optional(), tokenMaxLimit: z.string().optional(), tokenResetDuration: z.string().optional(), requestMaxLimit: z.string().optional(), requestResetDuration: z.string().optional(), -}); +}).refine( + (data) => data.budgetMaxLimit || data.tokenMaxLimit || data.requestMaxLimit, + { message: "At least one limit (budget, token, or request) must be configured", path: ["budgetMaxLimit"] } +);ui/app/workspace/model-limits/views/modelLimitsTable.tsx (2)
32-35: Duplicate helper function — same as in providerGovernanceTable.tsx.This is the second instance of
formatResetDuration. Extract to a shared utility.
314-353: Delete button remains enabled during deletion of other items.The
isDeletingstate is shared across all rows, but the disabled state only applies to the action button of the item being deleted. If a user clicks delete on one item, then quickly clicks delete on another, the second request could fire before the first completes.Consider tracking which specific item is being deleted.
🔎 Proposed approach
-const [deleteModelConfig, { isLoading: isDeleting }] = useDeleteModelConfigMutation(); +const [deleteModelConfig] = useDeleteModelConfigMutation(); +const [deletingId, setDeletingId] = useState<string | null>(null); const handleDelete = async (id: string) => { + setDeletingId(id); try { await deleteModelConfig(id).unwrap(); toast.success("Model limit deleted successfully"); onRefresh(); } catch (error) { toast.error(getErrorMessage(error)); + } finally { + setDeletingId(null); } }; // In the AlertDialogAction: -disabled={isDeleting} +disabled={deletingId === config.id}ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
101-127: TwouseEffecthooks both reset the form, which may cause conflicts.The first effect (lines 102-113) resets the form when
providerGovernancechanges. The second effect (lines 116-127) resets the form whenprovider.nameorproviderGovernanceDatachanges. SinceproviderGovernanceis derived fromproviderGovernanceData, both effects may fire in sequence, causing double resets.Consider consolidating into a single effect that handles both the provider change and data loading scenarios.
🔎 Proposed consolidation
-// Update form values when provider governance data is loaded -useEffect(() => { - if (providerGovernance) { - form.reset({ - budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", - budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", - tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", - tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", - requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", - }); - } -}, [providerGovernance, form]); - -// Reset form and creation state when provider changes -useEffect(() => { - setIsCreating(false); - const newProvGov = providerGovernanceData?.providers?.find((p) => p.provider === provider.name); - form.reset({ - budgetMaxLimit: newProvGov?.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", - budgetResetDuration: newProvGov?.budget?.reset_duration || "1M", - tokenMaxLimit: newProvGov?.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", - tokenResetDuration: newProvGov?.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: newProvGov?.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", - requestResetDuration: newProvGov?.rate_limit?.request_reset_duration || "1h", - }); -}, [provider.name, providerGovernanceData, form]); +// Reset form when provider changes or governance data loads +useEffect(() => { + setIsCreating(false); + form.reset({ + budgetMaxLimit: providerGovernance?.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", + budgetResetDuration: providerGovernance?.budget?.reset_duration || "1M", + tokenMaxLimit: providerGovernance?.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", + tokenResetDuration: providerGovernance?.rate_limit?.token_reset_duration || "1h", + requestMaxLimit: providerGovernance?.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", + requestResetDuration: providerGovernance?.rate_limit?.request_reset_duration || "1h", + }); +}, [provider.name, providerGovernance, form]);
484-494: Tooltip condition may show incorrect message.The tooltip shows when
!form.formState.isDirty || !form.formState.isValid, but the message logic at line 487 checks both conditions together first. IfisDirtyis true butisValidis false, the message correctly says "Please fix validation errors". However, the first condition!isDirty && !isValidwould show the combined message when both are false, which could be confusing since there are no "validation errors" if the form hasn't been touched.🔎 Proposed fix for clearer messaging
<p> - {!form.formState.isDirty && !form.formState.isValid - ? "No changes made and validation errors present" - : !form.formState.isDirty - ? "No changes made" - : "Please fix validation errors"} + {!form.formState.isDirty + ? "No changes made" + : !form.formState.isValid + ? "Please fix validation errors" + : ""} </p>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/model-limits/layout.tsxui/lib/constants/governance.tsui/app/workspace/providers/fragments/index.tsui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/lib/store/apis/baseApi.tsui/app/workspace/providers/views/providerGovernanceTable.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/components/sidebar.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/lib/types/governance.tsui/app/workspace/providers/fragments/governanceFormFragment.tsxui/lib/store/apis/governanceApi.tsui/app/workspace/providers/views/modelProviderConfig.tsx
🧬 Code graph analysis (7)
ui/app/workspace/model-limits/page.tsx (1)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
ModelLimitsView(10-74)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (13)
ui/lib/constants/governance.ts (1)
resetDurationLabels(23-33)ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/app/workspace/model-limits/views/modelLimitSheet.tsx (1)
ModelLimitSheet(47-461)ui/components/ui/button.tsx (1)
Button(70-70)ui/components/ui/table.tsx (6)
Table(71-71)TableHeader(71-71)TableRow(71-71)TableHead(71-71)TableBody(71-71)TableCell(71-71)ui/components/ui/badge.tsx (1)
Badge(37-37)ui/lib/constants/icons.tsx (2)
RenderProviderIcon(611-615)ProviderIconType(617-617)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)ui/lib/utils/governance.ts (1)
formatCurrency(27-29)ui/components/ui/progress.tsx (1)
Progress(24-24)ui/components/ui/alertDialog.tsx (9)
AlertDialog(83-83)AlertDialogTrigger(93-93)AlertDialogContent(86-86)AlertDialogHeader(89-89)AlertDialogTitle(92-92)AlertDialogDescription(87-87)AlertDialogFooter(88-88)AlertDialogCancel(85-85)AlertDialogAction(84-84)
ui/components/sidebar.tsx (1)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)
ui/lib/types/governance.ts (1)
transports/bifrost-http/handlers/governance.go (7)
CreateModelConfigRequest(161-166)CreateBudgetRequest(107-110)CreateRateLimitRequest(119-124)UpdateModelConfigRequest(169-174)UpdateBudgetRequest(113-116)UpdateRateLimitRequest(127-132)UpdateProviderGovernanceRequest(177-180)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
ui/app/workspace/providers/views/modelProviderConfig.tsx (4)
ui/components/ui/tabs.tsx (1)
TabsContent(39-39)ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
GovernanceFormFragment(56-502)ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
ProviderGovernanceTable(160-274)
⏰ 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). (18)
- 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/lib/constants/governance.ts (1)
22-33: LGTM! Well-structured duration labels mapping.The
resetDurationLabelsconstant provides clear, user-friendly display labels for all duration options used in the governance UI. The keys comprehensively cover all values from bothresetDurationOptionsandbudgetDurationOptions.ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
25-25: LGTM! Governance resource added to RBAC enum.The addition of
Governanceto theRbacResourceenum is consistent with the pattern and enables RBAC checks for governance-related features across the UI.ui/app/workspace/model-limits/page.tsx (1)
1-7: LGTM! Clean page component following Next.js conventions.The page component properly delegates rendering to
ModelLimitsView, maintaining a clean separation between the page route and the view logic. The"use client"directive is appropriately placed for client-side interactivity.ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM! Export added consistently.The
GovernanceFormFragmentexport follows the established pattern and maintains alphabetical ordering within the barrel file.ui/components/sidebar.tsx (4)
327-327: LGTM! Governance RBAC check added correctly.The
hasGovernanceAccesscheck is properly initialized usinguseRbacand follows the same pattern as other resource checks in the component.
392-393: LGTM! Governance menu access properly extended.The
hasAccesscondition for the Governance menu item now correctly includeshasGovernanceAccess, ensuring users with governance permissions can access the menu even if they don't have other governance-related permissions (virtual keys, customers, etc.).
402-408: LGTM! Model Limits sub-item properly configured.The new "Model Limits" sub-item under Governance is well-structured with:
- Appropriate URL (
/workspace/model-limits)- Relevant icon (Gauge)
- Clear description
- Proper RBAC gating via
hasGovernanceAccess
727-727: Trivial formatting change.Removed extraneous whitespace in the
routerprop. This is a minor cleanup with no functional impact.ui/lib/store/apis/baseApi.ts (2)
158-159: LGTM! Tag types added for governance features.The new
ModelConfigsandProviderGovernancetag types enable proper cache invalidation for governance-related API queries and mutations. The naming follows the established camelCase convention used by other tag types in the array.
177-177: Trivial formatting improvement.Added a space after
iffor consistency with standard JavaScript formatting conventions.ui/app/workspace/providers/views/modelProviderConfig.tsx (4)
5-5: LGTM! Governance component imports added.The imports for
GovernanceFormFragmentandProviderGovernanceTableare properly added to integrate governance features into the provider configuration view.Also applies to: 9-9
42-46: LGTM! Governance tab added to provider configuration.The Governance tab is now available for all providers, enabling per-provider budget and rate limit configuration. This is appropriately placed after the Performance tuning tab.
106-108: LGTM! Governance tab content properly wired.The
GovernanceFormFragmentis correctly rendered within the governance tab's content area, following the same pattern as other tabs in the configuration UI.
120-120: LGTM! Governance metrics table added.The
ProviderGovernanceTableis rendered below the provider keys section to display governance metrics. Based on the component's implementation (from relevant code snippets), it includes internal checks for RBAC permissions and governance enablement, so unconditional rendering here is safe and appropriate.ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
160-207: LGTM: Well-structured component with proper loading and access control.The component correctly:
- Guards data fetching with RBAC and governance-enabled checks
- Uses lazy config query to determine governance state
- Shows loading state while checking governance enablement
- Returns null gracefully when governance is disabled or unconfigured
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (2)
186-191: Sheet blocks escape and outside clicks — verify this is intentional.Preventing
onInteractOutsideandonEscapeKeyDownmay frustrate users who expect standard sheet/modal behavior. If this is to prevent accidental data loss, consider showing a confirmation dialog instead when the form is dirty.
99-183: LGTM: Form submission logic is well-structured.The
onSubmithandler correctly:
- Checks permissions before proceeding
- Handles both create and update paths
- Builds conditional payloads for budget and rate limits
- Properly handles the "removal" case with empty objects
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
79-97: LGTM: Header and CTA properly gated by RBAC.The "Add Model Limit" button is correctly disabled when
hasCreateAccessis false, both in the header and the empty state.ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
129-185: LGTM: Submission logic correctly handles create/update semantics.The
onSubmithandler properly:
- Determines whether fields were added or removed
- Sends empty objects to signal removal of budget/rate limit
- Distinguishes between creating new vs updating existing governance
ui/lib/types/governance.ts (2)
209-214: LGTM: Null union types correctly enable clearing semantics.The addition of
| nulltoUpdateRateLimitRequestfields properly aligns with the backend's pointer semantics wherenullsignals explicit clearing.
280-332: LGTM: New governance types are well-structured and consistent with backend.The
ModelConfig,ProviderGovernance, and related request/response interfaces correctly mirror the Go backend structures provided in the context snippets. The optional fields and relationships are properly defined.ui/lib/store/apis/governanceApi.ts (2)
234-296: LGTM: API endpoints are well-implemented.The new endpoints correctly:
- Use consistent patterns with existing endpoints
- Apply
encodeURIComponentfor provider names in URLs (lines 283, 292)- Set up proper tag invalidation for cache management
- Follow the established RTK Query builder conventions
236-239: BothModelConfigsandProviderGovernanceare already registered as tag types in baseApi.ts (lines 158–159), so no action is required.Likely an incorrect or invalid review comment.
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
a063ede to
7b90953
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
723435a to
3c984ca
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
7b90953 to
58f1ddc
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
3c984ca to
548cfc9
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
58f1ddc to
a9894c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
12-12: Remove unused imports.
ShieldandClockicons are imported but not used in this component.🔎 Proposed fix
-import { AlertTriangle, Clock, DollarSign, Gauge, RefreshCw, Shield, Zap } from "lucide-react"; +import { AlertTriangle, DollarSign, Gauge, RefreshCw, Zap } from "lucide-react";ui/app/workspace/model-limits/views/modelLimitsView.tsx (2)
36-39: Consider showing an inline message instead of a toast for disabled governance.The toast at line 38 will fire every time the component mounts when governance is disabled. Consider rendering an inline disabled state UI instead of a toast, which would provide a better user experience for users who navigate to this page when governance is turned off.
🔎 Proposed approach
+ if (governanceEnabled === false) { + return ( + <div className="mx-auto w-full max-w-7xl"> + <div className="flex flex-col items-center justify-center py-12 text-center"> + <p className="text-muted-foreground">Governance is not enabled. Please enable it in the config.</p> + </div> + </div> + ); + } if (isLoading) { return <FullPageLoader />; }And remove the toast from the config fetch:
.then((res) => { if (res.data?.client_config?.enable_governance) { setGovernanceEnabled(true); } else { setGovernanceEnabled(false); - toast.error("Governance is not enabled. Please enable it in the config."); } })
48-53: Toast spam during polling errors.Without deduplication, the error toast will fire repeatedly during polling if the model configs endpoint consistently fails. Consider either:
- Re-adding error deduplication using the error message as the key
- Disabling polling when an error occurs
🔎 Proposed fix using error message as key
+import { useRef } from "react"; + export default function ModelLimitsView() { + const shownErrorsRef = useRef<Set<string>>(new Set()); const [governanceEnabled, setGovernanceEnabled] = useState<boolean | null>(null); // ... // Handle query errors useEffect(() => { - if (modelConfigsError) { - toast.error(`Failed to load model configs: ${getErrorMessage(modelConfigsError)}`); - } + if (!modelConfigsError) return; + const errorKey = getErrorMessage(modelConfigsError); + if (shownErrorsRef.current.has(errorKey)) return; + shownErrorsRef.current.add(errorKey); + toast.error(`Failed to load model configs: ${errorKey}`); }, [modelConfigsError]);ui/app/workspace/providers/fragments/governanceFormFragment.tsx (5)
42-52: Consider adding schema-level validation for numeric fields.The Zod schema accepts any string for numeric limit fields. While parsing happens in
onSubmit, adding validation at the schema level would provide earlier feedback and prevent form submission with invalid data.🔎 Proposed schema enhancement
const formSchema = z.object({ // Budget - budgetMaxLimit: z.string().optional(), + budgetMaxLimit: z.string().optional().refine( + (val) => !val || (!isNaN(parseFloat(val)) && parseFloat(val) > 0), + { message: "Must be a positive number" } + ), budgetResetDuration: z.string().optional(), // Token limits - tokenMaxLimit: z.string().optional(), + tokenMaxLimit: z.string().optional().refine( + (val) => !val || (!isNaN(parseInt(val)) && parseInt(val) > 0), + { message: "Must be a positive integer" } + ), tokenResetDuration: z.string().optional(), // Request limits - requestMaxLimit: z.string().optional(), + requestMaxLimit: z.string().optional().refine( + (val) => !val || (!isNaN(parseInt(val)) && parseInt(val) > 0), + { message: "Must be a positive integer" } + ), requestResetDuration: z.string().optional(), });
63-71: Consider logging errors when checking governance status.The catch block silently sets
governanceEnabledtofalse, which may hide actual errors (network issues, auth problems, etc.) from administrators who need to troubleshoot.🔎 Proposed improvement
useEffect(() => { triggerGetConfig({ fromDB: true }) .then((res) => { setGovernanceEnabled(!!res.data?.client_config?.enable_governance); }) .catch((err) => { + console.error("Failed to check governance status:", err); setGovernanceEnabled(false); }); }, [triggerGetConfig]);
73-78: Consider reducing polling frequency for governance data.A 10-second polling interval may be excessive for governance data that typically changes infrequently. Consider increasing to 30-60 seconds to reduce unnecessary API calls.
🔎 Proposed change
const { data: providerGovernanceData, isLoading: isLoadingGovernance } = useGetProviderGovernanceQuery(undefined, { skip: !hasViewAccess || !governanceEnabled, - pollingInterval: hasViewAccess && governanceEnabled ? 10000 : 0, + pollingInterval: hasViewAccess && governanceEnabled ? 30000 : 0, refetchOnFocus: true, skipPollingIfUnfocused: true, });
102-113: Removeformfrom useEffect dependency array.Including the entire
formobject in the dependency array is unnecessary and may cause extra re-renders. React Hook Form's methods likeresetare stable references.🔎 Proposed fix
useEffect(() => { if (providerGovernance) { form.reset({ budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", }); } - }, [providerGovernance, form]); + }, [providerGovernance, form.reset]);
115-127: Consolidate form reset logic and fix dependencies.This useEffect duplicates the form reset logic from the previous effect and includes the unstable
formreference. Consider consolidating the reset logic or ensuring only one effect handles form synchronization.🔎 Proposed consolidation
+ // Reset form and creation state when provider changes + useEffect(() => { + setIsCreating(false); + }, [provider.name]); + // Reset form and creation state when provider changes useEffect(() => { - setIsCreating(false); const newProvGov = providerGovernanceData?.providers?.find((p) => p.provider === provider.name); form.reset({ budgetMaxLimit: newProvGov?.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", budgetResetDuration: newProvGov?.budget?.reset_duration || "1M", tokenMaxLimit: newProvGov?.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", tokenResetDuration: newProvGov?.rate_limit?.token_reset_duration || "1h", requestMaxLimit: newProvGov?.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", requestResetDuration: newProvGov?.rate_limit?.request_reset_duration || "1h", }); - }, [provider.name, providerGovernanceData, form]); + }, [provider.name, providerGovernanceData, form.reset]);Or merge with the previous effect by adding
provider.nameto its dependencies.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/app/workspace/providers/fragments/index.ts
- ui/app/workspace/model-limits/page.tsx
- ui/app/workspace/model-limits/layout.tsx
- ui/lib/constants/governance.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/lib/store/apis/baseApi.tsui/app/workspace/providers/views/providerGovernanceTable.tsxui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/components/sidebar.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/lib/types/governance.tsui/lib/store/apis/governanceApi.tsui/app/workspace/model-limits/views/modelLimitsView.tsx
🧬 Code graph analysis (6)
ui/components/sidebar.tsx (1)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (5)
ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/types/config.ts (1)
KnownProvider(6-6)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/app/workspace/providers/views/modelProviderConfig.tsx (4)
ui/components/ui/tabs.tsx (1)
TabsContent(39-39)ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
GovernanceFormFragment(56-502)ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
ProviderGovernanceTable(164-278)
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (6)
ui/lib/constants/governance.ts (1)
resetDurationLabels(23-33)ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/icons.tsx (2)
RenderProviderIcon(611-615)ProviderIconType(617-617)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
ModelLimitsTable(42-374)
⏰ 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 (25)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
25-25: LGTM!The new
Governanceresource correctly extends the RBAC enum to support the governance features introduced in this PR. The naming convention follows the existing pattern.ui/components/sidebar.tsx (1)
327-327: LGTM!The governance RBAC check and Model Limits sub-item are properly integrated:
hasGovernanceAccessfollows the established pattern for other RBAC checks- The Governance menu's
hasAccesscorrectly includes the new permission alongside existing checks- The Model Limits sub-item is well-configured with appropriate icon, URL, and access control
Also applies to: 392-408
ui/lib/store/apis/baseApi.ts (1)
158-159: LGTM!The new RTK Query tag types
ModelConfigsandProviderGovernancecorrectly support cache invalidation for the governance API endpoints introduced in this PR.ui/app/workspace/providers/views/modelProviderConfig.tsx (1)
42-46: LGTM!The governance tab integration is well-structured:
- The tab definition follows the existing pattern
GovernanceFormFragmenthandles the configuration UI within the tabProviderGovernanceTablerenders separately to display metrics and internally manages its visibility based on governance enablementAlso applies to: 106-108, 120-120
ui/app/workspace/providers/views/providerGovernanceTable.tsx (2)
101-103: LGTM! Division by zero issue properly addressed.The
clampedPercentagecomputation correctly guards against division by zero and clamps the value between 0-100, which is then used consistently in the tooltip content.Also applies to: 149-152
164-211: LGTM!The component properly:
- Performs RBAC checks before fetching data
- Uses lazy query for config check with proper error handling
- Conditionally polls governance data only when enabled and accessible
- Returns null when governance is disabled or unconfigured (avoiding empty UI)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (2)
84-101: LGTM! Form reset properly guards against discarding unsaved changes.The
isDirtycheck at line 87 correctly prevents the form from being reset when the user has unsaved changes, addressing the potential data loss issue from polling updates.
103-187: LGTM!The submit handler properly:
- Validates RBAC permissions before mutations
- Handles both create and update flows with appropriate payloads
- Correctly builds payloads to clear budget/rate limits when values are removed
- Provides appropriate success/error feedback via toasts
ui/lib/types/governance.ts (2)
209-214: LGTM!Allowing
nullinUpdateRateLimitRequestfields enables explicit clearing of rate limit values, which aligns with the update logic inmodelLimitSheet.tsxwhere removed values need to be cleared on the backend.
280-332: LGTM!The new governance type definitions are well-structured:
ModelConfigproperly models per-model budgeting with optional provider scoping- Request/response types follow the established patterns in this file
ProviderGovernanceextends providers with budget/rate limit capabilities- All interfaces align with the backend structures referenced in the PR
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (5)
129-185: LGTM! Well-structured submission handler.The form submission logic correctly handles:
- Parsing string inputs to numbers with proper undefined handling
- Distinguishing between "never had" vs "had but now removed" states via empty objects
- Explicit null values for partial rate limit removal
- Appropriate success/error feedback
219-240: LGTM! Clear loading and disabled states.The loading and governance-disabled states provide appropriate feedback to users with clear messaging and visual indicators.
242-293: LGTM! Well-designed empty state with proper RBAC gating.The empty state provides clear call-to-action with appropriate permission checks and helpful tooltips for users without access.
295-502: LGTM! Comprehensive form UI with proper validation and RBAC.The form rendering handles all states correctly:
- Conditional header based on create vs edit mode
- Proper form field bindings with React Hook Form
- Appropriate RBAC gating for actions
- Clear user feedback via tooltips and disabled states
400-436: No action required. The code already handles null safety correctly through TypeScript's type system—current_usagefields are required properties in theBudgetandRateLimitinterfaces, so they are guaranteed to exist when their parent objects are present. The existing optional chaining (?.) checks are sufficient.ui/app/workspace/model-limits/views/modelLimitsTable.tsx (7)
1-40: LGTM! Clean imports and helper function.The imports are well-organized, and the
formatResetDurationhelper provides a sensible fallback for unknown duration formats.
42-77: LGTM! Well-structured component initialization and handlers.The handlers properly manage state, RBAC checks, and provide appropriate user feedback. The
stopPropagationin edit handler prevents event bubbling issues.
79-133: LGTM! Clear header and engaging empty state.The empty state provides excellent user guidance with proper RBAC gating on the CTA button.
134-171: LGTM! Division by zero issue properly addressed.The safe percentage calculations correctly guard against division by zero (addressing the previous review comment). The exhaustion logic properly checks both budget and rate limit states.
172-193: LGTM! Clear model and provider display.The conditional styling for exhausted states and the handling of optional provider fields work well. Using
font-monofor model names improves readability of technical identifiers.
194-303: LGTM! Comprehensive budget and rate limit displays.The budget and rate limit columns provide excellent visual feedback with:
- Safe progress bar calculations using pre-computed percentages
- Clear tooltips with detailed usage information
- Appropriate red styling when limits are reached
- Graceful handling of missing data
304-364: LGTM! Well-implemented action buttons with proper safeguards.The action column correctly implements:
- Hover-based visibility for a clean UI
- RBAC gating for edit and delete operations
- Confirmation dialog to prevent accidental deletions
- Loading states during deletion
- Truncation of long model names in confirmation messages
ui/lib/store/apis/governanceApi.ts (3)
1-31: LGTM! Clean type imports.All new governance types are properly imported and maintain alphabetical ordering for maintainability.
235-296: LGTM! Well-structured API endpoints with proper URL encoding.The new endpoints follow RTK Query best practices and maintain consistency with existing patterns. The use of
encodeURIComponentfor provider names (lines 283, 292) is essential for handling special characters correctly.
300-370: LGTM! Complete hook exports.All new endpoints properly export their corresponding hooks, including lazy query variants where appropriate, maintaining consistency with the existing API structure.
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
a9894c8 to
e6e77b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (4)
68-68: Remove unnecessarystopPropagationcalls.The
stopPropagationcalls on lines 68, 304, and 331 appear unnecessary since there's no parent click handler on the table rows. These calls add no value and can be safely removed for cleaner code.🔎 Proposed cleanup
-const handleEditModelLimit = (config: ModelConfig, e: React.MouseEvent) => { - e.stopPropagation(); +const handleEditModelLimit = (config: ModelConfig) => { setEditingModelConfig(config); setShowModelLimitSheet(true); };And update the call site at line 313:
-onClick={(e) => handleEditModelLimit(config, e)} +onClick={() => handleEditModelLimit(config)}Also remove the stopPropagation from line 304:
-<TableCell onClick={(e) => e.stopPropagation()}> +<TableCell>And from line 331:
-onClick={(e) => e.stopPropagation()} +disabled={!hasDeleteAccess}Also applies to: 304-304, 331-331
100-100: Remove unnecessary optional chaining.The
modelConfigsprop is typed asModelConfig[](non-optional), so the optional chaining operator (?.) on lines 100 and 146 is unnecessary. While harmless, removing it improves code clarity.🔎 Proposed fix
-{modelConfigs?.length === 0 ? ( +{modelConfigs.length === 0 ? (And at line 146:
-{modelConfigs?.map((config) => { +{modelConfigs.map((config) => {Also applies to: 146-146
42-374: Consider extracting table row logic into a separate component.This component is 374 lines with complex inline JSX for rendering table rows (lines 172-365). Extracting the row rendering logic into a separate
ModelLimitRowcomponent would improve maintainability and testability.This would:
- Reduce the cognitive complexity of the main component
- Make the table row logic independently testable
- Improve code organization and reusability
- Allow React to better optimize re-renders at the row level
Example structure:
// Separate file: modelLimitRow.tsx interface ModelLimitRowProps { config: ModelConfig; onEdit: (config: ModelConfig) => void; onDelete: (id: string) => Promise<void>; hasUpdateAccess: boolean; hasDeleteAccess: boolean; isDeleting: boolean; } function ModelLimitRow({ config, onEdit, onDelete, hasUpdateAccess, hasDeleteAccess, isDeleting }: ModelLimitRowProps) { // Row rendering logic here }
196-223: Consider extracting repeatedTooltipProviderwrapping pattern.The same
TooltipProvider→Tooltip→TooltipTrigger→ content structure is repeated three times for budget and rate limit displays. While functional, this repetition could be reduced by movingTooltipProviderto a higher level or creating a reusable wrapper component.💡 Example approach
Option 1: Move TooltipProvider to wrap the entire table (lines 134-369):
+<TooltipProvider> <div className="rounded-lg border"> <Table> <!-- ... --> </Table> </div> +</TooltipProvider>Then remove individual
<TooltipProvider>wrappers from lines 196, 232, and 266.Option 2: Create a reusable component:
function MetricDisplay({ children, tooltip }: { children: React.ReactNode; tooltip: React.ReactNode }) { return ( <Tooltip> <TooltipTrigger asChild> {children} </TooltipTrigger> <TooltipContent> {tooltip} </TooltipContent> </Tooltip> ); }According to the Radix UI documentation (which these components are based on), a single
TooltipProvidercan wrap multiple tooltips to share state and reduce provider overhead.Also applies to: 232-263, 266-297
ui/lib/store/apis/governanceApi.ts (1)
236-239: Consider adding pagination support for list endpoints.The
getModelConfigsandgetProviderGovernanceendpoints fetch all records without pagination parameters. If the number of model configurations or providers grows large, this could impact performance and user experience.💡 Suggested enhancement
Consider adding optional pagination and filtering parameters similar to how
getTeamsaccepts acustomerIdfilter (line 74):getModelConfigs: builder.query<GetModelConfigsResponse, { page?: number; limit?: number; provider?: string }>({ query: ({ page, limit, provider } = {}) => ({ url: "/governance/model-configs", params: { ...(page !== undefined && { page }), ...(limit !== undefined && { limit }), ...(provider && { provider }), }, }), providesTags: ["ModelConfigs"], }),This would require backend API support for these parameters, so coordinate with the backend team if this enhancement is needed. Given the PR stack mentions backend Go functions were added, verify if pagination is already supported in the API contract.
Also applies to: 273-276
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/app/workspace/model-limits/page.tsx
- ui/lib/constants/governance.ts
- ui/app/workspace/model-limits/views/modelLimitSheet.tsx
- ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx
- ui/lib/types/governance.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/model-limits/layout.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/lib/store/apis/baseApi.tsui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/components/sidebar.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/lib/store/apis/governanceApi.ts
🧬 Code graph analysis (4)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/components/sidebar.tsx (1)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)
ui/app/workspace/providers/views/modelProviderConfig.tsx (3)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
GovernanceFormFragment(65-490)ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
ProviderGovernanceTable(164-278)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
⏰ 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). (23)
- 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 (26)
ui/app/workspace/model-limits/layout.tsx (1)
1-3: LGTM!Clean layout component following Next.js conventions. The fragment wrapper is appropriate when no additional layout markup is needed for the model-limits route group.
ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM!The export follows the established barrel file pattern consistently.
ui/lib/store/apis/baseApi.ts (1)
158-159: LGTM!New tag types
ModelConfigsandProviderGovernanceare correctly added to support cache invalidation for the governance API endpoints. The naming follows the existing convention.ui/components/sidebar.tsx (2)
327-327: LGTM!The governance RBAC check follows the established pattern for other resource access checks in this component.
392-408: LGTM!The governance menu integration is well-structured:
- Access control properly aggregates multiple permissions including the new
hasGovernanceAccess- The "Model Limits" sub-item follows the existing sub-item pattern with appropriate icon, URL, and description
- RBAC gating is correctly applied at the sub-item level
ui/app/workspace/providers/views/modelProviderConfig.tsx (3)
42-47: LGTM!The governance tab is correctly integrated into the available tabs. The tab definition follows the existing pattern, and the internal components (GovernanceFormFragment) handle their own access/enablement checks.
106-108: LGTM!The governance TabsContent correctly renders the GovernanceFormFragment with the provider prop, consistent with other tab content implementations.
120-120: LGTM!Placing
ProviderGovernanceTableat the root level (outside the accordion) provides good visibility for governance metrics. The component internally handles its own visibility logic based on governance enablement.ui/app/workspace/providers/views/providerGovernanceTable.tsx (4)
26-81: LGTM!The
CircularProgresscomponent is well-implemented with proper guards against division by zero (line 39) and smooth visual transitions. The color scheme correctly reflects usage thresholds (green < 80%, amber 80-99%, red at exhaustion).
84-162: LGTM!The
MetricCardcomponent handles edge cases correctly:
- Division by zero protected at line 102
- Percentage clamped to 0-100 at line 103
- Tooltip uses the safe
clampedPercentagevalueThe exhaustion state styling and tooltips provide good user feedback.
164-211: LGTM!The data fetching logic is well-structured:
- RBAC check gates the governance view access
- Lazy core config query determines governance enablement
- Provider governance query correctly uses
skipand conditional polling- Loading state handles both data fetch and initial enablement check
213-277: LGTM!The exhaustion logic correctly verifies both that limits exist (
max_limit > 0) and that usage meets/exceeds the limit before flagging exhaustion. The conditional rendering of metric cards based on configured limits is appropriate.ui/app/workspace/model-limits/views/modelLimitsView.tsx (3)
10-29: LGTM!The query setup correctly:
- Uses RBAC access check to gate governance features
- Combines
skipcondition withgovernanceEnabledandhasGovernanceAccess- Enables polling only when both conditions are met
- Uses
skipPollingIfUnfocusedto conserve resources
31-53: LGTM!The governance enablement check and error handling are well-structured. The decision to remove error deduplication (per past discussion) is acceptable since React's
useEffectdependency tracking prevents duplicate toasts for the same error reference.
55-69: LGTM!The
handleRefreshguard and the render logic are correct. The component appropriately shows a loader during initialization and passes an empty array as fallback when no model configs exist.ui/app/workspace/providers/fragments/governanceFormFragment.tsx (6)
42-63: LGTM!The Zod schema with optional string fields is appropriate for a form where all limits are optional. The
DEFAULT_GOVERNANCE_FORM_VALUESconstant properly addresses the previous DRY concern and provides a single source of truth for form defaults.
131-187: LGTM!The payload construction logic correctly handles the create/update/remove scenarios:
- Empty objects signal removal of previously existing configurations
- The
hadBudget/hadRateLimittracking ensures proper cleanup signals to the API- The implicit NaN-to-falsy conversion from
parseFloat/parseIntgracefully handles invalid input as "no limit configured"
189-205: LGTM!The delete handler correctly resets the form to defaults using the extracted constant, addressing the previous review feedback about DRY violations.
207-281: LGTM!The conditional rendering states are well-structured:
- Loading spinner during initial data fetch
- Clear "Governance Not Enabled" state with helpful message
- Empty state with permission-gated CTA button and feature hints
The tooltip for disabled state provides good UX feedback about missing permissions.
284-424: LGTM!The form structure is clean with proper sections for budget and rate limiting configuration. The "Current Usage" display provides valuable feedback when editing existing governance. The conditional rendering based on
isCreatingandhasExistingGovernancecorrectly differentiates between create and edit modes.
426-489: LGTM!The form actions are well-implemented:
- Delete button with confirmation dialog prevents accidental data loss
- Cancel button only shown in create mode
- Submit button properly disabled based on form state and permissions
- Tooltip feedback explains why submit is disabled
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (2)
147-170: LGTM - Safe percentage calculations properly implemented.The division-by-zero guards are correctly implemented for all three percentage calculations (budget, token rate, request rate). Each calculation checks that the limit exists and is greater than zero before dividing, with a safe fallback to 0. This addresses the previous review comment appropriately.
46-48: LGTM - RBAC enforcement is properly implemented.The component correctly uses
useRbachooks to check permissions for Create, Update, and Delete operations, and enforces these checks by disabling buttons when access is not granted. This provides proper authorization controls for the governance features.Also applies to: 93-96, 314-315, 332-333
ui/lib/store/apis/governanceApi.ts (3)
235-296: LGTM - Endpoints follow established patterns.The new Model Configs and Provider Governance endpoints are well-structured and consistent with existing endpoints in the file. Key strengths:
- Proper cache tag invalidation on mutations
- Type-safe request/response definitions
- Safe URL encoding for provider names using
encodeURIComponent- Follows the established builder pattern
272-296: Verify Provider Governance API contract matches implementation.The Provider Governance endpoints include Update and Delete operations but no Create endpoint, and no "get single provider" query. This suggests providers are pre-defined entities.
Please confirm:
- Is the absence of a
createProviderGovernanceendpoint intentional because providers are managed elsewhere?- Would a "get single provider governance" query (e.g.,
getProviderGovernance: builder.query<{ provider: ProviderGovernance }, string>) be useful for the UI, or is fetching the full list always acceptable?This helps ensure the API surface matches the intended governance workflow. If providers can number in the dozens, a single-item query might improve performance when viewing/editing a specific provider's governance settings.
1-31: LGTM - Clean type imports.The new governance types are properly imported and integrate cleanly with the existing import structure. The types are well-organized and follow the established naming conventions (
GetModelConfigsResponse,CreateModelConfigRequest, etc.).
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
e6e77b9 to
5e746b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
104-129: Consider consolidating form reset logic.There are two useEffect hooks that both reset the form with provider-specific governance data:
- Effect 1 (lines 104-115): Resets form when
providerGovernancechanges- Effect 2 (lines 118-129): Resets form when
provider.nameorproviderGovernanceDatachangesSince
providerGovernanceis derived fromproviderGovernanceDataandprovider.name(line 95), changes to either dependency in Effect 2 will causeproviderGovernanceto change, which then triggers Effect 1. This could result in the form being reset twice in quick succession.However, Effect 2 also manages
isCreatingstate (line 119), which Effect 1 doesn't handle. If this dual-reset pattern is intentional for state consistency, consider adding a comment explaining the reasoning. Otherwise, consolidating into a single effect that handles both form reset andisCreatingstate would reduce redundancy.🔎 Potential consolidation approach
- // Update form values when provider governance data is loaded - useEffect(() => { - if (providerGovernance) { - form.reset({ - budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", - budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", - tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", - tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", - requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", - }); - } - }, [providerGovernance, form]); - // Reset form and creation state when provider changes useEffect(() => { setIsCreating(false); const newProvGov = providerGovernanceData?.providers?.find((p) => p.provider === provider.name); form.reset({ budgetMaxLimit: newProvGov?.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", budgetResetDuration: newProvGov?.budget?.reset_duration || "1M", tokenMaxLimit: newProvGov?.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", tokenResetDuration: newProvGov?.rate_limit?.token_reset_duration || "1h", requestMaxLimit: newProvGov?.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", requestResetDuration: newProvGov?.rate_limit?.request_reset_duration || "1h", }); }, [provider.name, providerGovernanceData, form]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/workspace/model-limits/page.tsx
- ui/app/workspace/model-limits/layout.tsx
- ui/lib/constants/governance.ts
- ui/lib/store/apis/baseApi.ts
- ui/lib/types/governance.ts
- ui/components/sidebar.tsx
- ui/app/workspace/providers/views/modelProviderConfig.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/providerGovernanceTable.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/lib/store/apis/governanceApi.ts
🧬 Code graph analysis (4)
ui/app/workspace/providers/views/providerGovernanceTable.tsx (6)
ui/lib/constants/governance.ts (1)
resetDurationLabels(23-33)ui/components/ui/badge.tsx (1)
Badge(37-37)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)ui/lib/utils/governance.ts (1)
formatCurrency(27-29)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/components/ui/card.tsx (2)
CardHeader(50-50)CardTitle(50-50)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (13)
npx/bin.js (1)
data(172-172)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/sheet.tsx (5)
Sheet(137-137)SheetContent(137-137)SheetHeader(137-137)SheetTitle(137-137)SheetDescription(137-137)ui/components/ui/form.tsx (5)
Form(160-160)FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(15-69)ui/components/ui/popover.tsx (3)
Popover(37-37)PopoverTrigger(37-37)PopoverContent(37-37)ui/components/ui/button.tsx (1)
Button(70-70)ui/lib/types/config.ts (1)
KnownProvider(6-6)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/components/ui/separator.tsx (1)
DottedSeparator(43-43)ui/components/ui/label.tsx (1)
Label(21-21)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
ModelLimitsTable(42-395)
⏰ 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/lib/contexts/rbacContext.tsx (1)
25-25: LGTM! Clean addition of Governance RBAC resource.The new
Governanceresource enum value properly extends the RBAC surface to gate governance-related UI features across the application. The placement is consistent with the existing enum structure.ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM! Clean export addition.The export follows the established pattern and maintains consistency with other fragment exports.
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
10-70: LGTM! Well-structured governance-enabled view.The component properly:
- Gates data fetching behind governance enablement and RBAC checks
- Implements conditional polling (10s interval) with skip logic
- Enables refetch-on-focus for better UX
- Handles loading and error states appropriately
- Provides a clean interface to the ModelLimitsTable component
The governance enablement check flow (lines 31-46) ensures the feature gracefully handles cases where governance is disabled, with clear user feedback via toast.
ui/app/workspace/providers/views/providerGovernanceTable.tsx (3)
26-81: LGTM! Robust circular progress implementation with proper guards.The
CircularProgresscomponent (lines 26-81) properly handles edge cases:
- Line 39: Guards against division by zero with
max > 0check- Clamps percentage to 0-100 range
- Applies appropriate color coding based on exhaustion state and percentage thresholds
- Smooth transitions with duration-500
This addresses the previously flagged division by zero concern.
84-162: LGTM! Well-structured metric card with safe calculations.The
MetricCardcomponent properly:
- Computes safe percentages with guards (lines 102-103) preventing division by zero
- Clamps values to valid 0-100 range
- Provides rich tooltips with reset duration information
- Uses appropriate visual indicators for exhausted states
- Formats currency and numbers correctly
164-278: LGTM! Comprehensive provider governance display with proper gating.The main component properly:
- Checks both RBAC view access and governance enablement before fetching/displaying data
- Implements conditional polling (10s) with skip logic and refetch-on-focus
- Returns
nullwhen governance is disabled or not configured (clean UX pattern)- Handles loading states with appropriate skeleton UI
- Displays budget, token limits, and request limits with visual progress indicators
- Properly detects exhaustion states for each metric
The governance gating pattern is consistent with other governance components in the PR.
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (3)
84-101: LGTM! Proper form reset protection implemented.The
isDirtycheck (lines 87-89) prevents the form from being reset when external data updates while the user has unsaved changes. This properly addresses the previously flagged concern about discarding user edits during polling updates.
103-187: LGTM! Solid form submission with proper RBAC and payload construction.The submission logic properly:
- Enforces RBAC permissions before submitting (lines 104-107)
- Parses numeric values appropriately
- Differentiates between create and update flows
- Constructs payloads that signal removal via empty objects when limits are cleared (lines 121-148)
- Handles partial rate limits (only tokens or only requests) correctly with null values
- Provides clear success/error feedback via toasts
The create/update payload logic is well thought out to handle all combinations of budget and rate limit configurations.
226-310: LGTM! Excellent provider selector UX with search and clear functionality.The provider field implementation (lines 226-310) provides:
- Searchable popover with Command component for easy provider selection
- Visual provider icons for quick recognition
- Clear button to reset selection to "All Providers"
- Proper event handling to prevent bubbling
- Helpful hint text explaining the "All Providers" option
The UX is intuitive and aligns with modern design patterns.
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
56-63: LGTM! Default values properly extracted to constant.The
DEFAULT_GOVERNANCE_FORM_VALUESconstant eliminates the duplication that was previously flagged, providing a single source of truth for default form values used in initialization (line 100), deletion (line 194), and cancellation (line 204).
131-187: LGTM! Robust payload construction for create and update flows.The submission logic properly:
- Differentiates between new limits and removal of existing limits
- Uses empty objects
{}to signal removal of budget or rate_limit sections (lines 128, 147)- Handles partial rate limits with
nullvalues for unused fields- Maintains backward compatibility with optional provider governance
- Provides appropriate success messages for create vs update flows
230-281: LGTM! Polished empty state with clear call-to-action.The empty state (lines 230-281) provides:
- Visually appealing gradient background with decorative elements
- Clear explanation of the feature's value proposition
- Permission-gated "Configure Governance" button with tooltip feedback
- Feature hints highlighting key capabilities (Budget Limits, Token Rate Limiting, Request Throttling)
The empty state encourages adoption while respecting RBAC permissions.
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (5)
147-170: LGTM! Safe percentage calculations prevent division by zero.The percentage calculations (lines 159-170) properly guard against division by zero with explicit checks:
- budgetPercentage (lines 159-162): Checks
config.budget?.max_limit && config.budget.max_limit > 0- tokenPercentage (lines 163-166): Checks
config.rate_limit?.token_max_limit && config.rate_limit.token_max_limit > 0- requestPercentage (lines 167-170): Checks
config.rate_limit?.request_max_limit && config.rate_limit.request_max_limit > 0Each calculation clamps the result to 100 and defaults to 0 when the limit is absent or zero. This properly addresses the previously flagged division by zero concern.
42-77: LGTM! Comprehensive RBAC-gated CRUD operations.The component properly:
- Checks create/update/delete permissions separately for fine-grained access control (lines 46-48)
- Handles mutations with appropriate success/error feedback via toasts
- Triggers refresh callback after successful operations to sync UI state
- Manages modal state for add/edit flows cleanly
The CRUD operation handlers are well-structured and maintain proper separation of concerns.
100-133: LGTM! Engaging empty state with clear value proposition.The empty state provides:
- Visually appealing design with gradient background and decorative elements
- Clear messaging about the feature's purpose
- Permission-gated CTA button
- Feature hints highlighting key capabilities
- Consistent styling with other governance empty states in the PR
The empty state effectively encourages users to adopt the feature.
194-236: LGTM! Rich budget display with progress visualization.The budget cell implementation (lines 194-236):
- Shows maximum limit with currency formatting and reset duration
- Displays progress bar with dynamic color coding (emerald→amber→red based on usage)
- Provides detailed tooltip with current usage and reset schedule
- Gracefully handles missing budget with a dash
The visual feedback clearly communicates budget status at a glance.
325-384: LGTM! Well-designed action buttons with proper gating and confirmation.The actions column (lines 325-384):
- Uses group-hover pattern to reveal actions on row hover (line 326)
- Provides tooltips for Edit and Delete buttons
- Gates Edit button by update permission (line 335)
- Gates Delete button by delete permission (line 353)
- Requires confirmation via AlertDialog before deletion with clear, descriptive messaging
- Truncates long model names in confirmation dialog for better UX (line 367)
- Shows loading state during deletion ("Deleting...")
The action handling is thorough and user-friendly with appropriate safeguards.
ui/lib/store/apis/governanceApi.ts (3)
235-270: LGTM! Complete CRUD endpoints for model configs.The model configs endpoints (lines 235-270) properly:
- Implement all CRUD operations (GET collection, GET single, POST, PUT, DELETE)
- Use consistent tag names ("ModelConfigs") for cache management
- Provide item-level tags for granular invalidation (lines 243, 261)
- Invalidate the collection tag after mutations to refresh lists
- Follow the established patterns from other governance endpoints
The endpoint structure is clean and maintainable.
272-296: LGTM! Provider governance endpoints with proper URL encoding.The provider governance endpoints (lines 272-296) properly:
- Implement GET, PUT, and DELETE operations (no POST needed since PUT creates if not exists)
- Use
encodeURIComponentfor provider names in URLs (lines 283, 292) to handle special characters correctly- Invalidate the "ProviderGovernance" tag after mutations
- Return consistent response shapes with message and data
The URL encoding is particularly important since provider names can contain characters that need escaping in URL paths.
300-370: LGTM! Comprehensive hook exports for new endpoints.The hook exports properly:
- Include all new model config hooks (lines 342-347)
- Include all new provider governance hooks (lines 349-352)
- Add lazy query variants for async usage (lines 368-369)
- Follow the naming conventions established by RTK Query
- Maintain alphabetical grouping by feature area
The exported hooks provide a complete and consistent API surface for consuming components.
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
5e746b9 to
4ad031e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
103-129: MissingisDirtyguard may discard user edits during polling.Unlike
ModelLimitSheetwhich guards form resets withform.formState.isDirty, this component unconditionally resets the form whenproviderGovernanceorprovider.namechanges. With 10-second polling enabled, any unsaved user edits could be overwritten.🔎 Proposed fix
// Update form values when provider governance data is loaded useEffect(() => { if (providerGovernance) { + // Don't reset if user has unsaved changes + if (form.formState.isDirty) { + return; + } form.reset({ budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", }); } }, [providerGovernance, form]);
429-453: Delete button is not gated by RBAC.The delete button and
AlertDialogActionare not disabled based on permissions. WhilehasUpdateProviderAccessgates the save button, there's no corresponding delete permission check. If delete requires a separate permission (or the same update permission), consider adding a disabled state.🔎 Proposed fix (if delete should require update permission)
- <AlertDialogAction onClick={handleDelete} className="bg-red-600 text-white hover:bg-red-700" disabled={isDeleting}> + <AlertDialogAction onClick={handleDelete} className="bg-red-600 text-white hover:bg-red-700" disabled={isDeleting || !hasUpdateProviderAccess}> {isDeleting ? "Removing..." : "Remove Governance"} </AlertDialogAction>Also consider disabling the trigger button:
- <Button type="button" variant="ghost" size="sm" className="text-red-500 hover:bg-red-500/10 hover:text-red-500"> + <Button type="button" variant="ghost" size="sm" className="text-red-500 hover:bg-red-500/10 hover:text-red-500" disabled={!hasUpdateProviderAccess}>ui/app/workspace/model-limits/views/modelLimitsTable.tsx (2)
343-361: Verify the nested Tooltip/AlertDialog pattern.The Delete button uses a complex nesting structure where
AlertDialogTriggerwrapsTooltipTrigger, both withasChild. While React 19's improved ref handling should support this, the more conventional pattern isTooltipTriggerwrappingAlertDialogTrigger.This structure may cause conflicts in event handling or focus management. Please verify that:
- The tooltip displays correctly on hover
- The confirmation dialog opens on click
- Focus management works as expected
- Keyboard navigation functions properly
💡 Alternative pattern to consider
<AlertDialog> <TooltipProvider> <Tooltip> - <AlertDialogTrigger asChild> - <TooltipTrigger asChild> + <TooltipTrigger asChild> + <AlertDialogTrigger asChild> <Button variant="ghost" size="icon" className="h-8 w-8 text-red-500 hover:bg-red-500/10 hover:text-red-500" onClick={(e) => e.stopPropagation()} disabled={!hasDeleteAccess} > <Trash2 className="h-4 w-4" /> </Button> - </TooltipTrigger> - </AlertDialogTrigger> + </AlertDialogTrigger> + </TooltipTrigger> <TooltipContent>Delete</TooltipContent> </Tooltip> </TooltipProvider> <AlertDialogContent> {/* ... */} </AlertDialogContent> </AlertDialog>
330-357: Consider adding explicit aria-labels for icon-only buttons.The Edit and Delete buttons use icons without visible text labels. While tooltips provide labels on hover, screen readers may not announce them properly, affecting accessibility.
💡 Accessibility enhancement
<Button variant="ghost" size="icon" className="h-8 w-8" onClick={(e) => handleEditModelLimit(config, e)} disabled={!hasUpdateAccess} + aria-label="Edit model limit" > <Edit className="h-4 w-4" /> </Button><Button variant="ghost" size="icon" className="h-8 w-8 text-red-500 hover:bg-red-500/10 hover:text-red-500" onClick={(e) => e.stopPropagation()} disabled={!hasDeleteAccess} + aria-label="Delete model limit" > <Trash2 className="h-4 w-4" /> </Button>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx
- ui/app/workspace/model-limits/layout.tsx
- ui/lib/constants/governance.ts
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/app/workspace/providers/views/providerGovernanceTable.tsx
- ui/lib/store/apis/governanceApi.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/providers/fragments/index.tsui/app/workspace/model-limits/views/modelLimitsView.tsxui/components/sidebar.tsxui/lib/types/governance.tsui/lib/store/apis/baseApi.tsui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/page.tsx
🧬 Code graph analysis (6)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)
ui/components/sidebar.tsx (1)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)
ui/lib/types/governance.ts (1)
transports/bifrost-http/handlers/governance.go (7)
CreateModelConfigRequest(161-166)CreateBudgetRequest(107-110)CreateRateLimitRequest(119-124)UpdateModelConfigRequest(169-174)UpdateBudgetRequest(113-116)UpdateRateLimitRequest(127-132)UpdateProviderGovernanceRequest(177-180)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (11)
ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/sheet.tsx (5)
Sheet(137-137)SheetContent(137-137)SheetHeader(137-137)SheetTitle(137-137)SheetDescription(137-137)ui/components/ui/form.tsx (5)
Form(160-160)FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(15-69)ui/components/ui/popover.tsx (3)
Popover(37-37)PopoverTrigger(37-37)PopoverContent(37-37)ui/components/ui/button.tsx (1)
Button(70-70)ui/lib/types/config.ts (1)
KnownProvider(6-6)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/components/ui/separator.tsx (1)
DottedSeparator(43-43)ui/components/ui/label.tsx (1)
Label(21-21)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/app/workspace/model-limits/page.tsx (1)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
ModelLimitsView(10-70)
⏰ 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). (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (20)
ui/lib/types/governance.ts (2)
209-214: LGTM! Union types correctly support clearing values.The
| nulladditions toUpdateRateLimitRequestproperly allow signaling removal of rate limit fields, aligning with the backend'somitemptyhandling wherenullcan be explicitly sent to clear values.
279-332: Type definitions align with Go backend structures.The new interfaces (
ModelConfig,CreateModelConfigRequest,UpdateModelConfigRequest,ProviderGovernance, etc.) correctly mirror the Go backend request/response structures fromtransports/bifrost-http/handlers/governance.go. The requiredmodel_namefield inCreateModelConfigRequestmatches the backend'svalidate:"required"tag.ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM!Clean addition to the barrel exports, maintaining consistent export patterns with the other fragments.
ui/app/workspace/model-limits/page.tsx (1)
1-7: LGTM!Clean page component following Next.js App Router conventions with proper client directive and simple delegation to the view component.
ui/components/sidebar.tsx (2)
327-327: LGTM!Correct RBAC hook usage for the new Governance resource.
392-408: LGTM! Governance menu properly extended.The parent Governance menu correctly includes
hasGovernanceAccessin its access check, and the new "Model Limits" sub-item is properly gated with the same RBAC permission. The URL matches the new page route.ui/lib/store/apis/baseApi.ts (1)
158-159: LGTM!New cache tag types properly added to support cache invalidation for model configs and provider governance endpoints.
ui/app/workspace/model-limits/views/modelLimitsView.tsx (2)
31-46: Consider handling promise rejection more robustly.The
triggerGetConfigcall uses.then()/.catch()pattern. If the promise resolves butres.datais undefined (network issue or malformed response),res.data?.client_config?.enable_governancewould be falsy, setting governance to false without showing an error toast. This might be acceptable, but worth noting.
10-69: LGTM! Well-structured view component.The component properly handles:
- RBAC gating with
hasGovernanceAccess- Conditional query execution with
skip- Polling with appropriate intervals
- Loading states
- Error notifications
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (3)
160-179: Create payload correctly uses undefined for optional fields.The create flow properly sends
undefinedfor missing budget/rate_limit, which will be omitted from the JSON request due to standard serialization behavior. This aligns with the backend'somitemptytags.
47-101: LGTM! Form initialization and reset logic is well-implemented.The
isDirtyguard in the useEffect (lines 87-89) correctly prevents overwriting user edits whenmodelConfigupdates from polling, addressing the previous review comment.
115-158: The backend correctly interprets empty object payloads as removal signals. Whenbudget: {}orrate_limit: {}is sent, the empty fields unmarshal as nil pointers, which the handler detects withMaxLimit == nilandTokenMaxLimit == nil && RequestMaxLimit == nilchecks respectively, then marks the configuration for deletion. No changes needed.ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
56-63: LGTM! DRY principle properly applied.The
DEFAULT_GOVERNANCE_FORM_VALUESconstant eliminates duplication and addresses the previous review comment.
143-170: Same payload pattern as ModelLimitSheet - verify empty object behavior.The rate limit and budget payload construction mirrors
ModelLimitSheet. Ensure the backend correctly handles empty objects ({}) as removal signals for provider governance as well.
283-489: LGTM! Form rendering is comprehensive and well-structured.The form properly handles:
- Create vs edit modes with appropriate headers
- Budget and rate limit configuration sections
- Current usage display for existing governance
- Proper button states with tooltips explaining why actions are disabled
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (5)
1-40: LGTM! Clean setup with proper Next.js 15 client-side configuration.The imports, helper function, and interface are well-structured. The
formatResetDurationhelper provides a clean abstraction for displaying duration labels.
42-77: LGTM! Robust state management and RBAC integration.The component properly:
- Gates actions with RBAC checks for Governance resource
- Handles delete operations with async mutation, error handling, and user feedback
- Manages add/edit flows with clear state separation
- Prevents event propagation in edit handler
79-132: LGTM! Excellent empty state UX.The header and empty state provide clear guidance and visual appeal. RBAC controls are consistently applied to both "Add Model Limit" buttons, ensuring proper access control throughout the user journey.
146-170: LGTM! Division-by-zero guards properly implemented.The exhaustion state and percentage calculations correctly guard against division by zero (lines 159-170). The past review concern has been fully addressed with checks ensuring
max_limit > 0before division.
172-324: LGTM! Rich table UI with excellent visual feedback.The table cells effectively display:
- Model names with exhaustion badges
- Provider icons or "All Providers" fallback
- Budget and rate limit metrics with progress bars
- Contextual color coding (red/amber/emerald) based on usage levels
- Detailed tooltips for usage insights
The implementation handles optional data gracefully and provides clear visual cues for limit exhaustion.
akshaydeo
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
548cfc9 to
d9d7bcc
Compare
akshaydeo
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
4ad031e to
53e1735
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
d9d7bcc to
f2e6066
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
53e1735 to
5de62d8
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
f2e6066 to
ed6660d
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
5de62d8 to
04a0216
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
31-46: Consider gating the core config fetch with RBAC check.The
triggerGetConfigruns unconditionally on mount, but if the user doesn't have governance access (hasGovernanceAccess), the subsequent model configs query is skipped anyway. While this doesn't cause functional issues (no sensitive data is exposed by the core config call), you could short-circuit the config fetch when!hasGovernanceAccessfor consistency.🔎 Optional improvement
useEffect(() => { + if (!hasGovernanceAccess) { + setGovernanceEnabled(false); + return; + } triggerGetConfig({ fromDB: true }) .then((res) => { // ... }) // ... -}, [triggerGetConfig]); +}, [triggerGetConfig, hasGovernanceAccess]);ui/app/workspace/model-limits/views/modelLimitSheet.tsx (1)
109-113: Consider adding NaN validation for numeric inputs.While
NumberAndSelectlikely constrains input to valid numbers,parseFloatandparseIntwill returnNaNfor invalid strings (e.g., "abc"), which could propagate to the API. Adding a guard ensures robustness.🔎 Defensive validation
-const budgetMaxLimit = data.budgetMaxLimit ? parseFloat(data.budgetMaxLimit) : undefined; -const tokenMaxLimit = data.tokenMaxLimit ? parseInt(data.tokenMaxLimit) : undefined; -const requestMaxLimit = data.requestMaxLimit ? parseInt(data.requestMaxLimit) : undefined; +const budgetMaxLimit = data.budgetMaxLimit ? parseFloat(data.budgetMaxLimit) : undefined; +const tokenMaxLimit = data.tokenMaxLimit ? parseInt(data.tokenMaxLimit, 10) : undefined; +const requestMaxLimit = data.requestMaxLimit ? parseInt(data.requestMaxLimit, 10) : undefined; + +// Guard against NaN from invalid input +if ((budgetMaxLimit !== undefined && isNaN(budgetMaxLimit)) || + (tokenMaxLimit !== undefined && isNaN(tokenMaxLimit)) || + (requestMaxLimit !== undefined && isNaN(requestMaxLimit))) { + toast.error("Please enter valid numeric values"); + return; +}ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
103-129: Consider consolidating the two form reset effects.Both
useEffectblocks reset the form based on governance data changes, but they can fire redundantly:
- First effect (lines 104-115) triggers when
providerGovernancechanges- Second effect (lines 118-129) triggers when
provider.nameorproviderGovernanceDatachangesSince
providerGovernanceis derived fromproviderGovernanceData, both effects fire when data updates. Consider consolidating into a single effect keyed onprovider.nameandproviderGovernanceData.🔎 Consolidated effect
-// Update form values when provider governance data is loaded -useEffect(() => { - if (providerGovernance) { - form.reset({ - budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", - budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", - tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", - tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", - requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", - }); - } -}, [providerGovernance, form]); - // Reset form and creation state when provider changes useEffect(() => { setIsCreating(false); const newProvGov = providerGovernanceData?.providers?.find((p) => p.provider === provider.name); - form.reset({ - budgetMaxLimit: newProvGov?.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", - budgetResetDuration: newProvGov?.budget?.reset_duration || "1M", - tokenMaxLimit: newProvGov?.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", - tokenResetDuration: newProvGov?.rate_limit?.token_reset_duration || "1h", - requestMaxLimit: newProvGov?.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", - requestResetDuration: newProvGov?.rate_limit?.request_reset_duration || "1h", - }); + if (newProvGov) { + form.reset({ + budgetMaxLimit: newProvGov.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", + budgetResetDuration: newProvGov.budget?.reset_duration || "1M", + tokenMaxLimit: newProvGov.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", + tokenResetDuration: newProvGov.rate_limit?.token_reset_duration || "1h", + requestMaxLimit: newProvGov.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", + requestResetDuration: newProvGov.rate_limit?.request_reset_duration || "1h", + }); + } else { + form.reset(DEFAULT_GOVERNANCE_FORM_VALUES); + } }, [provider.name, providerGovernanceData, form]);
131-136: Add NaN validation for numeric inputs (same pattern as modelLimitSheet).Similar to the model limit sheet, consider adding NaN guards for
parseFloat/parseIntresults to handle edge cases where invalid input bypasses UI validation.🔎 Defensive validation
const budgetMaxLimit = data.budgetMaxLimit ? parseFloat(data.budgetMaxLimit) : undefined; -const tokenMaxLimit = data.tokenMaxLimit ? parseInt(data.tokenMaxLimit) : undefined; -const requestMaxLimit = data.requestMaxLimit ? parseInt(data.requestMaxLimit) : undefined; +const tokenMaxLimit = data.tokenMaxLimit ? parseInt(data.tokenMaxLimit, 10) : undefined; +const requestMaxLimit = data.requestMaxLimit ? parseInt(data.requestMaxLimit, 10) : undefined; + +if ((budgetMaxLimit !== undefined && isNaN(budgetMaxLimit)) || + (tokenMaxLimit !== undefined && isNaN(tokenMaxLimit)) || + (requestMaxLimit !== undefined && isNaN(requestMaxLimit))) { + toast.error("Please enter valid numeric values"); + return; +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/lib/store/apis/baseApi.ts
- ui/app/workspace/model-limits/layout.tsx
- ui/app/workspace/model-limits/page.tsx
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/lib/constants/governance.ts
- ui/app/workspace/providers/views/providerGovernanceTable.tsx
- ui/lib/store/apis/governanceApi.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/components/sidebar.tsxui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/lib/types/governance.tsui/app/workspace/providers/fragments/index.tsui/app/workspace/model-limits/views/modelLimitsTable.tsx
🧬 Code graph analysis (3)
ui/components/sidebar.tsx (1)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/lib/types/governance.ts (1)
transports/bifrost-http/handlers/governance.go (7)
CreateModelConfigRequest(161-166)CreateBudgetRequest(107-110)CreateRateLimitRequest(119-124)UpdateModelConfigRequest(169-174)UpdateBudgetRequest(113-116)UpdateRateLimitRequest(127-132)UpdateProviderGovernanceRequest(177-180)
⏰ 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). (6)
- 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 (9)
ui/lib/types/governance.ts (1)
279-332: Well-structured governance types that align with backend.The new
ModelConfig,ProviderGovernance, and related request/response interfaces properly mirror the Go backend structures. The use of optional fields (provider?,budget?,rate_limit?) correctly matches the backend's nullable/omitempty semantics.ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
25-25: LGTM!The new
Governanceresource follows the existing enum pattern and enables RBAC gating for governance-related UI features.ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM!Clean addition of the
GovernanceFormFragmentexport following the existing barrel export pattern.ui/components/sidebar.tsx (1)
327-327: LGTM!The governance RBAC check and new "Model Limits" sub-item follow the established sidebar patterns. The access control logic correctly gates the menu item visibility based on governance permissions.
Also applies to: 392-408
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
61-69: LGTM!Clean loading state handling and component rendering. The default empty array for
model_configsprevents null reference issues.ui/app/workspace/model-limits/views/modelLimitSheet.tsx (1)
84-101: LGTM!The
isDirtyguard correctly prevents form reset when the user has unsaved changes, addressing the previous review concern.ui/app/workspace/model-limits/views/modelLimitsTable.tsx (2)
158-170: LGTM!The safe percentage calculations correctly guard against division by zero, implementing the fix from the previous review. The comment explaining the purpose is helpful.
42-60: LGTM!Clean component setup with proper RBAC checks and mutation hooks. The delete handler correctly calls
onRefresh()after successful deletion to update the parent view.ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
56-63: LGTM!Good implementation of the DRY refactor from the previous review. The
DEFAULT_GOVERNANCE_FORM_VALUESconstant is now used consistently acrossdefaultValues,handleDelete, andhandleCancel.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/workspace/model-limits/views/modelLimitsView.tsx
- ui/components/sidebar.tsx
- ui/app/workspace/model-limits/page.tsx
- ui/app/workspace/providers/views/providerGovernanceTable.tsx
- ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx
- ui/app/workspace/model-limits/layout.tsx
- ui/lib/store/apis/baseApi.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/providers/fragments/index.tsui/lib/constants/governance.tsui/app/workspace/model-limits/views/modelLimitSheet.tsxui/lib/store/apis/governanceApi.tsui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/providers/views/modelProviderConfig.tsxui/lib/types/governance.tsui/app/workspace/providers/fragments/governanceFormFragment.tsx
🧬 Code graph analysis (4)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (13)
npx/bin.js (1)
data(172-172)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/sheet.tsx (5)
Sheet(137-137)SheetContent(137-137)SheetHeader(137-137)SheetTitle(137-137)SheetDescription(137-137)ui/components/ui/form.tsx (5)
Form(160-160)FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/input.tsx (1)
Input(15-69)ui/components/ui/popover.tsx (3)
Popover(37-37)PopoverTrigger(37-37)PopoverContent(37-37)ui/components/ui/button.tsx (1)
Button(70-70)ui/lib/types/config.ts (1)
KnownProvider(6-6)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/components/ui/separator.tsx (1)
DottedSeparator(43-43)ui/components/ui/label.tsx (1)
Label(21-21)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
ui/lib/types/governance.ts (1)
transports/bifrost-http/handlers/governance.go (7)
CreateModelConfigRequest(161-166)CreateBudgetRequest(107-110)CreateRateLimitRequest(119-124)UpdateModelConfigRequest(169-174)UpdateBudgetRequest(113-116)UpdateRateLimitRequest(127-132)UpdateProviderGovernanceRequest(177-180)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (6)
ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)ui/components/ui/separator.tsx (1)
DottedSeparator(43-43)
⏰ 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 (15)
ui/lib/constants/governance.ts (1)
22-33: LGTM! Clean constant addition for duration label mapping.The
resetDurationLabelsmap provides a convenient lookup that complements the existingresetDurationOptionsarray. All duration keys are consistent between the two constants, ensuring reliable label resolution across the governance UI.ui/app/workspace/model-limits/views/modelLimitSheet.tsx (2)
84-101: LGTM! Form reset guard properly implemented.The
isDirtycheck prevents the form from being reset when polling updates arrive while the user has unsaved changes. This addresses the previous review concern effectively.
115-158: Complex payload logic is well-structured.The update payload construction correctly handles partial updates and signals removal by sending empty objects when limits are cleared. The logic differentiates between "had a value before" and "has a value now" to determine the appropriate payload shape.
ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM! Standard fragment re-export.The export follows the established pattern for fragment components in this module.
ui/app/workspace/providers/views/modelProviderConfig.tsx (2)
42-46: LGTM! Governance tab properly integrated.The governance tab is added consistently with the existing tab structure, maintaining the UI pattern.
106-108: LGTM! Governance content properly wired.The
GovernanceFormFragmentis correctly rendered within the governance tab's content area.ui/app/workspace/model-limits/views/modelLimitsTable.tsx (2)
158-170: LGTM! Safe percentage calculations properly implemented.The division-by-zero guards ensure that percentage calculations never produce
Infinity. The pattern is consistently applied across budget, token, and request limits.
147-156: LGTM! Exhaustion detection logic is comprehensive.The logic correctly identifies when budgets or rate limits are exhausted by checking both budget limits and either token or request limits. The early guards for existence and non-zero values prevent false positives.
ui/lib/types/governance.ts (2)
210-214: LGTM! Nullable fields enable explicit limit removal.The addition of
nullto the union types allows the UI to explicitly clear rate limit fields, which is necessary for the delete/update flows where limits are removed.
280-332: LGTM! Comprehensive governance type additions.The new interfaces for
ModelConfigandProviderGovernanceare well-structured:
- Include appropriate metadata fields (id, created_at, updated_at)
- Support optional provider field with clear documentation
- Define separate request/response types for API operations
- Include populated relationship fields (budget, rate_limit)
These types align with the backend Go structures and support the full CRUD workflow.
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
56-63: LGTM! Constant extraction addresses previous duplication.The
DEFAULT_GOVERNANCE_FORM_VALUESconstant eliminates the duplication that existed inhandleDeleteandhandleCancel, following the DRY principle effectively.
131-187: LGTM! Payload construction logic is sound.The submit handler correctly determines whether to send empty objects to signal removal of governance configuration. The logic mirrors the approach in
modelLimitSheet.tsxand properly handles partial updates.ui/lib/store/apis/governanceApi.ts (3)
235-270: LGTM! Model config endpoints follow RTK Query best practices.The new endpoints:
- Use appropriate HTTP methods (GET/POST/PUT/DELETE)
- Include proper tag-based cache invalidation
- Return typed responses aligned with governance types
- Follow the existing pattern established in this file
272-296: LGTM! Provider governance endpoints properly implemented.The endpoints correctly use
encodeURIComponentfor the provider name in URLs (lines 283, 292), preventing issues with provider names that contain special characters. Cache invalidation strategy is appropriate.
342-369: LGTM! Hook exports are complete.All new endpoints export both regular and lazy query hooks, maintaining consistency with the existing API surface.
ui/app/workspace/providers/fragments/governanceFormFragment.tsx
Outdated
Show resolved
Hide resolved
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
04a0216 to
b880611
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
ed6660d to
54081dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
104-115: Add isDirty guard to prevent polling from overwriting user edits.This concern was raised in a previous review and remains unaddressed. With 10-second polling enabled, the form will reset whenever
providerGovernanceupdates, potentially discarding user changes in progress.🔎 Recommended fix
useEffect(() => { if (providerGovernance) { + // Don't reset the form if the user has unsaved changes + if (form.formState.isDirty) { + return; + } form.reset({ budgetMaxLimit: providerGovernance.budget?.max_limit ? String(providerGovernance.budget.max_limit) : "", budgetResetDuration: providerGovernance.budget?.reset_duration || "1M", tokenMaxLimit: providerGovernance.rate_limit?.token_max_limit ? String(providerGovernance.rate_limit.token_max_limit) : "", tokenResetDuration: providerGovernance.rate_limit?.token_reset_duration || "1h", requestMaxLimit: providerGovernance.rate_limit?.request_max_limit ? String(providerGovernance.rate_limit.request_max_limit) : "", requestResetDuration: providerGovernance.rate_limit?.request_reset_duration || "1h", }); } }, [providerGovernance, form]);
118-129: Consider isDirty guard for provider change reset.This concern from the previous review remains unaddressed. When the provider changes, unsaved form edits are discarded. Consider adding an
isDirtycheck and either preserving changes or showing a confirmation dialog before resetting.🔎 Suggested approach
useEffect(() => { setIsCreating(false); + // Consider preserving unsaved changes or warning the user + if (form.formState.isDirty) { + // Option 1: Skip the reset (may cause confusion) + // return; + // Option 2: Show a warning dialog (better UX) + // (implementation would require additional state/dialog component) + } const newProvGov = providerGovernanceData?.providers?.find((p) => p.provider === provider.name); form.reset({ budgetMaxLimit: newProvGov?.budget?.max_limit ? String(newProvGov.budget.max_limit) : "", budgetResetDuration: newProvGov?.budget?.reset_duration || "1M", tokenMaxLimit: newProvGov?.rate_limit?.token_max_limit ? String(newProvGov.rate_limit.token_max_limit) : "", tokenResetDuration: newProvGov?.rate_limit?.token_reset_duration || "1h", requestMaxLimit: newProvGov?.rate_limit?.request_max_limit ? String(newProvGov.rate_limit.request_max_limit) : "", requestResetDuration: newProvGov?.rate_limit?.request_reset_duration || "1h", }); }, [provider.name, providerGovernanceData, form]);
🧹 Nitpick comments (1)
ui/lib/store/apis/governanceApi.ts (1)
272-296: Consider granular cache tags for provider governance.The provider governance endpoints use list-level cache invalidation only. While this is functionally correct, adding granular tags like
{ type: "ProviderGovernance", id: provider }(similar to the Model Configs pattern) would enable more efficient cache updates when editing a single provider.Current impact is minimal since provider lists are typically small, but the pattern would align with the Model Configs implementation.
🔎 Optional enhancement
getProviderGovernance: builder.query<GetProviderGovernanceResponse, void>({ query: () => "/governance/providers", - providesTags: ["ProviderGovernance"], + providesTags: (result) => + result + ? [ + "ProviderGovernance", + ...result.providers.map((p) => ({ type: "ProviderGovernance" as const, id: p.provider })), + ] + : ["ProviderGovernance"], }), updateProviderGovernance: builder.mutation< { message: string; provider: ProviderGovernance }, { provider: string; data: UpdateProviderGovernanceRequest } >({ query: ({ provider, data }) => ({ url: `/governance/providers/${encodeURIComponent(provider)}`, method: "PUT", body: data, }), - invalidatesTags: ["ProviderGovernance"], + invalidatesTags: (result, error, { provider }) => [ + "ProviderGovernance", + { type: "ProviderGovernance", id: provider }, + ], }), deleteProviderGovernance: builder.mutation<{ message: string }, string>({ query: (provider) => ({ url: `/governance/providers/${encodeURIComponent(provider)}`, method: "DELETE", }), - invalidatesTags: ["ProviderGovernance"], + invalidatesTags: (result, error, provider) => [ + "ProviderGovernance", + { type: "ProviderGovernance", id: provider }, + ], }),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/app/workspace/model-limits/layout.tsx
- ui/components/sidebar.tsx
- ui/app/workspace/providers/views/providerGovernanceTable.tsx
- ui/lib/constants/governance.ts
- ui/app/workspace/model-limits/views/modelLimitSheet.tsx
- ui/lib/types/governance.ts
- ui/app/workspace/model-limits/page.tsx
- ui/lib/store/apis/baseApi.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/lib/store/apis/governanceApi.ts
🧬 Code graph analysis (4)
ui/app/workspace/providers/views/modelProviderConfig.tsx (4)
ui/components/ui/tabs.tsx (1)
TabsContent(39-39)ui/app/workspace/providers/fragments/governanceFormFragment.tsx (1)
GovernanceFormFragment(65-490)ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
ProviderGovernanceTable(164-278)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
ModelLimitsTable(42-395)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (2)
ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
⏰ 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). (7)
- 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 (12)
ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM!The export is clean and consistent with the existing barrel export pattern in this module.
ui/lib/store/apis/governanceApi.ts (3)
1-31: LGTM!The new governance type imports are properly organized and align with the types defined in
ui/lib/types/governance.ts.
235-270: Well-structured Model Configs endpoints.The CRUD endpoints follow RTK Query best practices with proper cache invalidation strategies. The granular tag approach (list + individual item tags) ensures efficient cache updates.
300-370: LGTM!The exported hooks are correctly generated from the RTK Query endpoints and follow the established naming conventions.
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
25-25: LGTM!The
GovernanceRBAC resource addition is correctly placed and follows the enum pattern. Based on the PR stack context, the backend definition should already exist in the downstack PRs.ui/app/workspace/providers/views/modelProviderConfig.tsx (2)
5-5: LGTM!The governance tab integration is clean and follows the existing pattern for other provider configuration tabs.
Also applies to: 9-9, 42-46
106-108: Well-integrated governance components.The governance tab content and provider governance table are properly wired into the existing provider configuration layout. The placement of
ProviderGovernanceTableoutside the accordion makes sense for displaying summary metrics.Also applies to: 120-120
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
10-70: Well-structured governance-enabled view.The component properly gates data fetching based on governance enablement and RBAC permissions, handles loading and error states appropriately, and provides a clean integration point for the model limits table.
Key strengths:
- Defensive state management with
governanceEnabled: boolean | null- Proper query skip conditions and polling configuration
- Clear separation of concerns between governance config check and data fetching
- User-friendly error messaging
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
1-395: Comprehensive and well-implemented model limits table.The component demonstrates excellent attention to detail:
- Safe calculations: Division by zero guards are properly implemented for all progress percentages (confirmed from past review)
- RBAC integration: All actions appropriately gated with disabled states when no permission
- User experience: Polished empty state, confirmation dialogs, tooltips, and visual indicators for limit exhaustion
- Error handling: Proper error messages and refresh callbacks
- Formatting: Currency, numbers, and durations properly formatted throughout
The past review concerns have been addressed, and the current implementation is solid.
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (3)
56-63: LGTM!The
DEFAULT_GOVERNANCE_FORM_VALUESconstant properly centralizes the default form values, eliminating the duplication issue identified in the previous review.
131-205: Well-structured mutation handlers.The submit, delete, and cancel handlers properly manage form state, API interactions, and user feedback. The payload construction logic correctly handles creation, updates, and removal scenarios by sending empty objects when needed.
207-490: Excellent conditional rendering logic.The component handles multiple UI states gracefully:
- Loading states during data fetches
- Clear messaging when governance is disabled
- Polished empty state with RBAC-aware CTA
- Comprehensive form with budget and rate limit configuration
- Current usage metrics display
- Proper permission checks throughout
The form validation and button states ensure good UX by preventing invalid submissions and providing helpful tooltips.
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
54081dd to
5ff01f7
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
b880611 to
cc0611d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ui/lib/constants/governance.ts (1)
22-33: Consider derivingresetDurationLabelsfromresetDurationOptionsto avoid duplication.The mapping duplicates the label-value pairs already defined in
resetDurationOptions. This creates a maintenance burden where changes must be synchronized in two places.🔎 Proposed fix
-// Map of duration values to short labels for display -export const resetDurationLabels: Record<string, string> = { - "1m": "Every Minute", - "5m": "Every 5 Minutes", - "15m": "Every 15 Minutes", - "30m": "Every 30 Minutes", - "1h": "Hourly", - "6h": "Every 6 Hours", - "1d": "Daily", - "1w": "Weekly", - "1M": "Monthly", -}; +// Map of duration values to labels for display (derived from resetDurationOptions) +export const resetDurationLabels: Record<string, string> = Object.fromEntries( + resetDurationOptions.map(({ value, label }) => [value, label]) +);ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
61-69: Consider showing a disabled state UI when governance is disabled.When
governanceEnabledisfalse, the component rendersModelLimitsTablewith an empty array, but the user only sees a toast about governance being disabled. The table will show the empty state, which may be confusing. Consider adding an explicit disabled state UI.🔎 Proposed enhancement
if (isLoading) { return <FullPageLoader />; } +if (!governanceEnabled) { + return ( + <div className="mx-auto w-full max-w-7xl"> + <div className="flex flex-col items-center justify-center py-12 text-center"> + <p className="text-muted-foreground">Governance is not enabled. Please enable it in the configuration.</p> + </div> + </div> + ); +} + return ( <div className="mx-auto w-full max-w-7xl"> <ModelLimitsTable modelConfigs={modelConfigsData?.model_configs || []} onRefresh={handleRefresh} /> </div> );ui/app/workspace/providers/views/providerGovernanceTable.tsx (2)
12-12: Remove unusedShieldimport.The
Shieldicon is imported but not used in the component.🔎 Proposed fix
-import { AlertTriangle, Clock, DollarSign, Gauge, RefreshCw, Shield, Zap } from "lucide-react"; +import { AlertTriangle, Clock, DollarSign, Gauge, RefreshCw, Zap } from "lucide-react";
170-178: Silent error handling may confuse users.When the config fetch fails, the error is silently swallowed and
governanceEnabledis set tofalse. UnlikemodelLimitsView.tsxwhich shows a toast, this provides no feedback to the user. Consider adding error handling for consistency.🔎 Proposed fix
useEffect(() => { triggerGetConfig({ fromDB: true }) .then((res) => { setGovernanceEnabled(!!res.data?.client_config?.enable_governance); }) - .catch(() => { + .catch((err) => { + console.error("Failed to fetch config:", err); setGovernanceEnabled(false); }); }, [triggerGetConfig]);ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
32-35: Consider extractingformatResetDurationto a shared utility.This helper is duplicated in both
modelLimitsTable.tsxandproviderGovernanceTable.tsx. Consider moving it to a shared utilities file (e.g.,ui/lib/utils/governance.tsalongsideformatCurrency).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- ui/app/workspace/model-limits/page.tsx
- ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx
- ui/lib/store/apis/baseApi.ts
- ui/components/sidebar.tsx
- ui/app/workspace/providers/fragments/governanceFormFragment.tsx
- ui/app/workspace/model-limits/views/modelLimitSheet.tsx
- ui/app/workspace/providers/views/modelProviderConfig.tsx
- ui/lib/store/apis/governanceApi.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/providerGovernanceTable.tsxui/lib/constants/governance.tsui/lib/types/governance.tsui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/layout.tsx
🧬 Code graph analysis (2)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (2)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (5)
ui/lib/constants/governance.ts (1)
resetDurationLabels(23-33)ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/utils/governance.ts (1)
formatCurrency(27-29)
⏰ 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). (11)
- 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 (8)
ui/app/workspace/providers/fragments/index.ts (1)
4-4: Export correctly extends the module's public API. TheGovernanceFormFragmentre-export follows the established pattern, maintains alphabetical ordering with other exports, and properly references the source component ingovernanceFormFragment.tsx.ui/app/workspace/model-limits/layout.tsx (1)
1-3: LGTM!Simple pass-through layout that provides a clean extension point for future additions (e.g., shared headers or navigation).
ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
26-81: Well-implemented progress visualization components.
CircularProgressandMetricCardare well-structured with proper division-by-zero guards and clean visual feedback for different exhaustion states.Also applies to: 84-162
ui/lib/types/governance.ts (2)
209-214: Good use of nullable types for explicit field clearing.The
| nullunion types allow the frontend to explicitly clear optional fields (as opposed to just omitting them), which is the correct approach for PATCH-style updates.
280-332: Well-structured type definitions.The new governance types follow existing patterns in the file and properly model the relationships between entities. The separation of create/update request types with optional fields for updates is consistent with the rest of the codebase.
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (3)
42-60: Clean RBAC integration and error handling.The component properly gates actions with RBAC checks and provides user feedback via toasts on success and failure. The mutation hook's
isDeletingstate correctly prevents concurrent delete operations.
146-170: Well-implemented exhaustion detection and safe percentage calculations.The exhaustion checks properly handle null, undefined, and zero values. Pre-computing the percentages with guards against division by zero is the correct approach, addressing the previously reviewed concern.
99-132: Nice empty state design.The empty state provides clear guidance with feature hints and a prominent CTA. The decorative blur elements add visual polish without impacting functionality.
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
5ff01f7 to
80f2779
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
cc0611d to
10dc657
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
danpiths
force-pushed
the
feature/12-24-feat_adds_governance_frontend_for_models_and_providers
branch
from
10dc657 to
cad0361
Compare
danpiths
force-pushed
the
feature/12-24-feat_adds_backend_go_functions_for_model_provider_governance_configuration
branch
from
80f2779 to
5c8aba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
48-53: Potential repeated error toasts during polling.If the API continues to fail during polling (e.g., network issue), this
useEffectwill show a toast every timemodelConfigsErrorchanges. RTK Query may create new error object references on each failed poll, potentially causing repeated toasts every 10 seconds.Consider adding deduplication based on the error message, or disabling polling when an error occurs.
🔎 Optional: Error deduplication approach
+const shownErrorsRef = useRef(new Set<string>()); + useEffect(() => { - if (modelConfigsError) { - toast.error(`Failed to load model configs: ${getErrorMessage(modelConfigsError)}`); - } + if (!modelConfigsError) return; + const errorKey = getErrorMessage(modelConfigsError); + if (shownErrorsRef.current.has(errorKey)) return; + shownErrorsRef.current.add(errorKey); + toast.error(`Failed to load model configs: ${errorKey}`); }, [modelConfigsError]);ui/app/workspace/model-limits/views/modelLimitSheet.tsx (1)
34-45: Consider validating that at least one limit is configured.The schema allows creating a model config with only a model name and no budget or rate limits. This might create governance entries that don't actually govern anything.
🔎 Optional: Add cross-field validation
const formSchema = z.object({ modelName: z.string().min(1, "Model name is required"), provider: z.string().optional(), budgetMaxLimit: z.string().optional(), budgetResetDuration: z.string().optional(), tokenMaxLimit: z.string().optional(), tokenResetDuration: z.string().optional(), requestMaxLimit: z.string().optional(), requestResetDuration: z.string().optional(), }).refine( (data) => data.budgetMaxLimit || data.tokenMaxLimit || data.requestMaxLimit, { message: "At least one limit (budget, token, or request) must be configured", path: ["modelName"] } );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsxui/app/workspace/model-limits/layout.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/providers/views/providerGovernanceTable.tsxui/components/sidebar.tsxui/lib/constants/governance.tsui/lib/store/apis/baseApi.tsui/lib/store/apis/governanceApi.tsui/lib/types/governance.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/workspace/model-limits/layout.tsx
- ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx
- ui/components/sidebar.tsx
- ui/app/workspace/model-limits/page.tsx
- ui/lib/types/governance.ts
- ui/lib/constants/governance.ts
- ui/app/workspace/providers/views/providerGovernanceTable.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/lib/store/apis/baseApi.tsui/app/workspace/providers/views/modelProviderConfig.tsxui/app/workspace/model-limits/views/modelLimitSheet.tsxui/app/workspace/providers/fragments/index.tsui/app/workspace/providers/fragments/governanceFormFragment.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/lib/store/apis/governanceApi.tsui/app/workspace/model-limits/views/modelLimitsTable.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-24T18:54:01.100Z
Learnt from: danpiths
Repo: maximhq/bifrost PR: 1170
File: ui/app/workspace/providers/fragments/governanceFormFragment.tsx:0-0
Timestamp: 2025-12-24T18:54:01.100Z
Learning: In the Bifrost UI, when an external state change may affect a form (e.g., provider changes), do not reset the form or trigger updates if the user has unsaved edits. Check form.formState.isDirty and preserve user edits; only update underlying information or navigate/redirect when the form is clean. Apply this guideline to similar form fragments under ui/app/workspace/providers/fragments (and related forms) to avoid overwriting user input.
Applied to files:
ui/app/workspace/providers/fragments/governanceFormFragment.tsx
🧬 Code graph analysis (6)
ui/app/workspace/providers/views/modelProviderConfig.tsx (2)
ui/app/workspace/providers/fragments/index.ts (1)
GovernanceFormFragment(4-4)ui/app/workspace/providers/views/providerGovernanceTable.tsx (1)
ProviderGovernanceTable(164-278)
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (6)
ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/input.tsx (1)
Input(15-69)ui/lib/types/config.ts (1)
KnownProvider(6-6)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (4)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)ui/lib/constants/governance.ts (1)
resetDurationOptions(3-13)
ui/app/workspace/model-limits/views/modelLimitsView.tsx (3)
ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/app/workspace/model-limits/views/modelLimitsTable.tsx (1)
ModelLimitsTable(42-395)
ui/lib/store/apis/governanceApi.ts (1)
ui/lib/types/governance.ts (7)
GetModelConfigsResponse(310-313)ModelConfig(281-292)CreateModelConfigRequest(295-300)UpdateModelConfigRequest(302-307)GetProviderGovernanceResponse(329-332)ProviderGovernance(316-322)UpdateProviderGovernanceRequest(324-327)
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (7)
ui/lib/constants/governance.ts (1)
resetDurationLabels(23-33)ui/lib/types/governance.ts (1)
ModelConfig(281-292)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(64-66)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(176-201)ui/lib/constants/icons.tsx (1)
RenderProviderIcon(611-615)ui/lib/constants/logs.ts (2)
ProviderLabels(44-63)ProviderName(24-24)ui/lib/utils/governance.ts (1)
formatCurrency(27-29)
⏰ 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). (19)
- 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 (19)
ui/lib/store/apis/baseApi.ts (1)
158-159: LGTM!The new
"ModelConfigs"and"ProviderGovernance"tag types are correctly added to support cache invalidation for the new governance API endpoints. This aligns well with the RTK Query patterns used throughout the codebase.ui/lib/store/apis/governanceApi.ts (2)
234-296: LGTM! Well-structured API endpoints.The new Model Configs and Provider Governance endpoints follow consistent patterns with the existing codebase:
- Proper tag provisioning (
providesTags) for queries- Appropriate cache invalidation (
invalidatesTags) for mutations- Good use of
encodeURIComponentfor provider names in URLs to handle special characters safelyThe endpoint structure aligns well with the governance types defined in
ui/lib/types/governance.ts.
342-369: Hooks exported correctly.All new query and mutation hooks, including their lazy variants, are properly exported for consumption by the UI components.
ui/app/workspace/providers/fragments/index.ts (1)
4-4: LGTM!The
GovernanceFormFragmentexport follows the existing barrel export pattern.ui/app/workspace/providers/views/modelProviderConfig.tsx (2)
42-46: LGTM!The Governance tab is correctly added to the available tabs. The
GovernanceFormFragmenthandles the "governance not enabled" state internally, so no conditional check is needed here.
106-108: Clear separation of concerns between form and metrics.The design appropriately separates:
GovernanceFormFragment(inside the tab): configuration/editing formProviderGovernanceTable(outside the accordion): read-only metrics displayBoth components handle their own loading states and governance-enabled checks internally, returning null when appropriate.
Also applies to: 120-120
ui/app/workspace/model-limits/views/modelLimitsView.tsx (1)
10-27: LGTM! Well-structured view component.The component correctly:
- Guards data fetching with RBAC and governance-enabled checks
- Uses appropriate polling settings with
skipPollingIfUnfocused- Shows loading state until governance status is determined
- Provides refresh capability to child components
Also applies to: 55-69
ui/app/workspace/model-limits/views/modelLimitSheet.tsx (3)
84-101: LGTM! Properly guards against overwriting user edits.The
isDirtycheck prevents polling updates from resetting the form while the user is editing, which was correctly identified and addressed in previous reviews. Based on learnings, this aligns with the codebase pattern.
103-187: LGTM! Clean submit logic with proper payload construction.The
onSubmithandler correctly:
- Checks permissions before proceeding
- Distinguishes between create and update flows
- Signals removal of budget/rate limits with empty objects
- Handles nullable fields in rate limit payload
189-460: LGTM! Well-designed sheet UI.The UI provides:
- Clear create/edit distinction in title and description
- Searchable provider picker with clear button
- Organized sections for budget and rate limits
- Current usage display when editing
- Informative tooltips explaining disabled states
ui/app/workspace/model-limits/views/modelLimitsTable.tsx (4)
42-77: LGTM! Clean component structure.The component correctly:
- Uses RBAC hooks for create/update/delete permissions
- Manages sheet state for add/edit flows
- Provides proper handlers for CRUD operations with success/error toasts
158-170: LGTM! Safe percentage calculations.The division-by-zero guards are correctly implemented with
max_limit > 0checks before computing percentages, addressing the previous review feedback.
194-236: LGTM! Well-crafted table cells with rich UI.The budget and rate limit cells provide:
- Clear value display with icons
- Reset duration indicators
- Progress bars with color-coded thresholds (green < 80%, amber 80-100%, red at 100%)
- Informative tooltips with exact usage values
Also applies to: 237-324
343-382: LGTM! Proper action handling.The delete action correctly:
- Uses confirmation dialog
- Shows loading state during deletion
- Disables button based on RBAC
- Handles error with toast
ui/app/workspace/providers/fragments/governanceFormFragment.tsx (5)
56-63: LGTM! Default values extracted to constant.The
DEFAULT_GOVERNANCE_FORM_VALUESconstant eliminates duplication acrossdefaultValues,handleDelete, andhandleCancel, addressing the previous review feedback.
103-116: LGTM! Polling-safe form reset.The
isDirtyguard correctly prevents form resets during polling when the user has unsaved changes, as discussed in previous reviews and learnings.
136-192: LGTM! Comprehensive submit logic.The
onSubmithandler correctly:
- Determines create vs update based on existing data
- Constructs appropriate payloads for budget and rate limits
- Signals removal with empty objects when limits are cleared
- Shows contextual success messages
288-494: LGTM! Well-organized form UI.The form UI provides:
- Clear header with context (creating vs editing)
- Budget and rate limit configuration sections
- Current usage display when editing existing governance
- Proper action buttons with RBAC-gated submit
- Delete confirmation for removing governance
66-67: No action needed—RBAC resource choice is intentionally correct.The use of
RbacResource.ModelProviderforhasUpdateProviderAccessis intentional and distinct by design. The component distinguishes between:
- ModelProvider.Update: Permission to configure governance settings on a specific provider (provider-level operation, shown in button/form disable state)
- Governance.View: Permission to view governance information (general governance access)
This mirrors the broader codebase pattern where governance CRUD operations in the model-limits section use
RbacResource.Governance, while provider-specific configuration in the providers directory usesRbacResource.ModelProvider.
Summary
This PR adds model-level governance capabilities to the platform, allowing
administrators to set budget and rate limits for specific models across
providers. It introduces a new "Model Limits" page and enhances provider
settings with governance controls.
Changes
features
limits
Type of change
Affected areas
How to test
Screenshots/Recordings
Breaking changes
Related issues
N/A
Security considerations
This feature is protected by RBAC controls to ensure only authorized users can
view and modify governance settings.
Checklist
docs/contributing/README.mdand followed the guidelines