fix: ui fixes for providers#478
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-provider allowed-request toggles and an AllowedRequestsFields component; threads allowed_requests into provider create/update payloads; makes restart-required alerts conditional on form dirtiness; enhances network config with extra_headers and retry backoff fields plus validation; updates schemas and types; removes Maxim plugin UI; adjusts dialogs and minor UX; bumps multiple version strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dialog as AddNewCustomProviderDialog
participant Form as react-hook-form
participant Fields as AllowedRequestsFields
participant API as Mutation
participant Backend
User->>Dialog: Open dialog
Dialog->>Form: Initialize (allowed_requests defaults = true)
User->>Fields: Toggle request types
Fields-->>Form: Update values
User->>Form: Submit
Form->>API: Send payload (includes custom_provider_config.allowed_requests)
API->>Backend: POST
Backend-->>API: Response
API-->>Dialog: Close / notify
sequenceDiagram
autonumber
actor User
participant Network as NetworkFormFragment
participant Textarea as ExtraHeadersTextarea
participant Schema as ZodSchema
User->>Textarea: Type JSON text
User->>Textarea: Blur
Textarea->>Network: Try JSON.parse
alt Valid JSON object
Network-->>Network: Set extra_headers = parsed object
else Invalid or empty
Network-->>Network: Keep string or set undefined
end
User->>Network: Submit
Network->>Schema: Validate (retry_backoff_initial <= retry_backoff_max)
Schema-->>Network: Result (ok / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (24)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/providers/fragments/networkFormFragment.tsx (1)
209-227: “Remove configuration” bypasses validation, doesn’t remove config, and blocks custom providers.
- Calls
onSubmit(getValues())(bypasses resolver).- Spreads existing
provider.network_config, so fields persist.- Fails for custom providers due to base URL check.
Apply this diff to truly remove the config and keep UX consistent:
- <Button - type="button" - variant="outline" - onClick={() => { - form.reset({ - network_config: undefined, - }); - onSubmit(form.getValues()); - }} - disabled={ - isUpdatingProvider || - !provider.network_config || - !provider.network_config.base_url || - provider.network_config.base_url.trim() === "" - } - > + <Button + type="button" + variant="outline" + onClick={() => { + updateProvider({ ...provider, network_config: undefined }) + .unwrap() + .then(() => { + form.reset({ network_config: undefined }); + toast.success("Network configuration removed."); + }) + .catch((err) => + toast.error("Failed to update provider configuration", { description: getErrorMessage(err) }), + ); + }} + disabled={isUpdatingProvider || !provider.network_config} + > Remove configuration </Button>Also applies to: 218-223
🧹 Nitpick comments (16)
ui/app/providers/dialogs/addNewKeyDialog.tsx (1)
20-21: onOpenChange handler should consume the boolean argumentPassing
onCancel(no-arg) toonOpenChange?: (open: boolean) => voidcan be a type mismatch in stricter TS configs. Forward the boolean and close only onfalse.Apply this diff:
- <Dialog open={show} onOpenChange={onCancel}> + <Dialog open={show} onOpenChange={(open) => !open && onCancel()}>ui/lib/types/schemas.ts (2)
97-99: Fix validation message for max_retriesMessage says “greater than 0” but
min(0)allows 0. Align the copy.- .min(0, "Max retries must be greater than 0") + .min(0, "Max retries must be at least 0")
100-106: Backoff constraints: copy does not match numeric minsThe messages (100ms/1000ms) don’t match the actual mins (500/5000). Align messages to avoid confusing users.
retry_backoff_initial: z.coerce .number("Retry backoff initial must be a number") - .min(500, "Initial backoff must be at least 100ms"), + .min(500, "Initial backoff must be at least 500ms"), retry_backoff_max: z.coerce .number("Retry backoff max must be a number") - .min(5000, "Max backoff must be at least 1000ms"), + .min(5000, "Max backoff must be at least 5000ms"),Also applies to: 103-106
ui/app/providers/fragments/allowedRequestsFields.tsx (1)
12-21: Preserve literal types for request keysAdding
as consttightens types and helps avoid typos at call sites.-const REQUEST_TYPES = [ +const REQUEST_TYPES = [ { key: "text_completion", label: "Text Completion" }, { key: "chat_completion", label: "Chat Completion" }, { key: "chat_completion_stream", label: "Chat Completion Stream" }, { key: "embedding", label: "Embedding" }, { key: "speech", label: "Speech" }, { key: "speech_stream", label: "Speech Stream" }, { key: "transcription", label: "Transcription" }, { key: "transcription_stream", label: "Transcription Stream" }, -]; +] as const;ui/app/providers/views/modelProviderConfig.tsx (1)
47-49: Memo dependency may miss tab changes for custom providers
availableTabs(provider)depends onprovider.custom_provider_configpresence, but the memo key only tracksprovider.name. Include the relevant bit to prevent stale tabs.- const tabs = useMemo(() => { - return availableTabs(provider); - }, [provider.name]); + const tabs = useMemo(() => availableTabs(provider), [provider.name, !!provider.custom_provider_config]);ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
16-31: Avoid schema duplication; reuse central schemas
allowedRequestsSchemais already defined in@/lib/types/schemas. Duplicating it risks drift. Also, validatebaseFormatagainstknownProviderSchemainstead of a free string.-import { z } from "zod"; +import { z } from "zod"; +import { allowedRequestsSchema, knownProviderSchema } from "@/lib/types/schemas"; @@ -const allowedRequestsSchema = z.object({ - text_completion: z.boolean(), - chat_completion: z.boolean(), - chat_completion_stream: z.boolean(), - embedding: z.boolean(), - speech: z.boolean(), - speech_stream: z.boolean(), - transcription: z.boolean(), - transcription_stream: z.boolean(), -}); +// Reuse centralized schemas to prevent drift. @@ -const formSchema = z.object({ - name: z.string().min(1), - baseFormat: z.string().min(1), - allowed_requests: allowedRequestsSchema, -}); +const formSchema = z.object({ + name: z.string().min(1), + baseFormat: knownProviderSchema, // restrict to known provider values + allowed_requests: allowedRequestsSchema, +});
83-85: onOpenChange should use the boolean argumentSame as the keys dialog: consume the
openflag and close onfalseto satisfy the prop type.- <Dialog open={show} onOpenChange={onClose}> + <Dialog open={show} onOpenChange={(open) => !open && onClose()}>
16-31: Optional: consolidate imports via fragments indexMinor import hygiene: prefer importing
AllowedRequestsFieldsfrom the fragments barrel to keep import style consistent.-import { AllowedRequestsFields } from "../fragments/allowedRequestsFields"; +import { AllowedRequestsFields } from "../fragments";Also applies to: 48-57, 61-68
ui/app/providers/fragments/networkFormFragment.tsx (5)
42-67: Coerceextra_headerson submit; clear dirty state on success.Guarantee an object shape for headers before sending and reset form after a successful save.
Apply this diff:
-const onSubmit = (data: NetworkOnlyFormSchema) => { +const onSubmit = (data: NetworkOnlyFormSchema) => { if (isCustomProvider && !(data.network_config?.base_url || "").trim()) { toast.error("Base URL is required for custom providers."); return; } - // Create updated provider configuration + // Coerce/validate extra_headers to an object + const rawHeaders = data.network_config?.extra_headers as unknown; + let extraHeaders: Record<string, string> | undefined; + if (typeof rawHeaders === "string") { + try { + const parsed = JSON.parse(rawHeaders); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed) || !Object.values(parsed).every((v) => typeof v === "string")) { + throw new Error("Invalid headers"); + } + extraHeaders = parsed as Record<string, string>; + } catch { + toast.error("Extra headers must be valid JSON with string values."); + form.setError("network_config.extra_headers", { type: "custom", message: "Invalid headers JSON." }); + return; + } + } else if (rawHeaders && typeof rawHeaders === "object") { + extraHeaders = rawHeaders as Record<string, string>; + } + + // Create updated provider configuration const updatedProvider: ModelProvider = { ...provider, network_config: { ...provider.network_config, base_url: data.network_config?.base_url || undefined, - extra_headers: data.network_config?.extra_headers || undefined, + extra_headers: extraHeaders, default_request_timeout_in_seconds: data.network_config?.default_request_timeout_in_seconds ?? 30, max_retries: data.network_config?.max_retries ?? 0, retry_backoff_initial: data.network_config?.retry_backoff_initial ?? 500, retry_backoff_max: data.network_config?.retry_backoff_max ?? 10000, }, }; - updateProvider(updatedProvider) - .unwrap() + updateProvider(updatedProvider) + .unwrap() + .then(() => { + form.reset({ network_config: updatedProvider.network_config }); + toast.success("Network configuration saved."); + }) .catch((err) => { toast.error("Failed to update provider configuration", { description: getErrorMessage(err), }); }); };
114-121: Number inputs: avoid NaN/controlled-state glitches.Typing clears to empty →
Number("")⇒ 0, orNaNmay render “NaN”. Normalize empty toundefinedand control the value.Apply this diff:
- <Input placeholder="30" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + placeholder="30" + {...field} + value={field.value ?? ""} + onChange={(e) => { + const v = e.target.value.trim(); + field.onChange(v === "" ? undefined : Number(v)); + }} + />- <Input placeholder="0" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + placeholder="0" + {...field} + value={field.value ?? ""} + onChange={(e) => { + const v = e.target.value.trim(); + field.onChange(v === "" ? undefined : Number(v)); + }} + />Also applies to: 124-136
181-189: Backoff inputs: same empty/NaN issue; align placeholder with default.Normalize input like above; also placeholder shows 5000 while default fallback is 10000.
Apply this diff:
- <Input placeholder="500" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + placeholder="500" + {...field} + value={field.value ?? ""} + onChange={(e) => { + const v = e.target.value.trim(); + field.onChange(v === "" ? undefined : Number(v)); + }} + />- <Input placeholder="5000" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + placeholder="10000" + {...field} + value={field.value ?? ""} + onChange={(e) => { + const v = e.target.value.trim(); + field.onChange(v === "" ? undefined : Number(v)); + }} + />Also applies to: 193-201
169-169: Non-standard class name.
wrap-anywhereisn’t a standard Tailwind class. Preferbreak-wordsorbreak-all(or confirm a custom utility exists).
29-36: Schema-level coercion can simplify handlers.Consider using
z.coerce.number()with.min(0)etc. innetworkOnlyFormSchemato drop manualNumber(...)parsing and edge cases.If you want, I can open a follow-up PR to update
ui/lib/types/schemas.ts.ui/app/providers/fragments/apiStructureFormFragment.tsx (3)
56-58: Reset preserves defaults poorly; merge with current values.Resetting to
provider.custom_provider_configalone can unset fields that rely on defaults. Merge with current values to keep defaults when provider omits keys.Apply this diff:
- useEffect(() => { - form.reset(provider.custom_provider_config); - }, [form, provider.name, provider.custom_provider_config]); + useEffect(() => { + form.reset({ + base_provider_type: + provider.custom_provider_config?.base_provider_type ?? form.getValues().base_provider_type, + allowed_requests: { + ...form.getValues().allowed_requests, + ...provider.custom_provider_config?.allowed_requests, + }, + }); + }, [form, provider.name, provider.custom_provider_config]);
69-73: Clear dirty state after successful save.Reset to current values so Save/alert states reflect reality.
Apply this diff:
- .then(() => { - toast.success("Provider configuration updated successfully"); - }) + .then(() => { + form.reset(form.getValues()); + toast.success("Provider configuration updated successfully"); + })
13-13: Remove unused imports.
usefrom React andfilocale aren’t used.-import { use, useEffect } from "react"; +import { useEffect } from "react";-import { fi } from "zod/v4/locales";Also applies to: 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/lib/types/schemas.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
ui/app/providers/fragments/allowedRequestsFields.tsx (3)
ui/app/providers/fragments/index.ts (1)
AllowedRequestsFields(1-1)ui/components/ui/form.tsx (3)
FormItem(161-161)FormLabel(162-162)FormControl(163-163)ui/components/ui/switch.tsx (1)
Switch(36-36)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (2)
ui/lib/types/schemas.ts (1)
allowedRequestsSchema(193-202)ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
ui/app/providers/fragments/apiStructureFormFragment.tsx (1)
ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
ui/app/providers/views/modelProviderConfig.tsx (5)
ui/app/providers/views/utils.ts (1)
keysRequired(1-1)ui/app/providers/fragments/apiStructureFormFragment.tsx (1)
ApiStructureFormFragment(31-147)ui/app/providers/fragments/networkFormFragment.tsx (1)
NetworkFormFragment(24-251)ui/app/providers/fragments/proxyFormFragment.tsx (1)
ProxyFormFragment(26-196)ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ModelProviderKeysTableView(31-174)
ui/app/providers/fragments/networkFormFragment.tsx (2)
ui/components/ui/textarea.tsx (1)
Textarea(18-18)ui/components/ui/input.tsx (1)
Input(7-21)
⏰ 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 (14)
ui/app/providers/fragments/proxyFormFragment.tsx (1)
84-92: Restart alert gated by dirty-state — LGTMShowing the alert only when
showRestartAlertand the form is dirty is correct and consistent with other fragments.ui/app/providers/dialogs/addNewKeyDialog.tsx (1)
15-18: Dynamic title/toast for add vs. edit — LGTM
isEditing,dialogTitle, andsuccessMessagecorrectly improve UX with minimal surface change.ui/app/providers/fragments/allowedRequestsFields.tsx (1)
23-75: Reusable allowed-requests fieldset — LGTMClean extraction, good RHF integration, and clear UI layout.
ui/app/providers/fragments/index.ts (1)
1-1: Re-export added — LGTMCentralizing the export of
AllowedRequestsFieldsimproves import ergonomics.ui/app/providers/views/modelProviderConfig.tsx (2)
78-85: Propagating restart-alert flag — LGTMPassing
showRestartAlert={true}to all fragments keeps behavior consistent across tabs.
94-99: Conditional API keys section — LGTMUsing
keysRequired(provider.name)to gate the keys table is a good UX improvement.ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
91-99: Restart alert on dirty state — LGTMGood consistency with other forms; clear copy.
48-57: Sensible defaults for allowed requests — LGTMStarting with all request types enabled reduces setup friction.
143-145: Component reuse — LGTMUsing
AllowedRequestsFieldskeeps forms DRY and consistent.ui/app/providers/fragments/networkFormFragment.tsx (2)
79-87: Conditional restart alert: good guard.Rendering the alert only when both
showRestartAlertandform.formState.isDirtyare true is correct and avoids noise.
53-58: Backoff defaults changed; confirm product intent + schema guard.You changed fallbacks to 500/10000 ms. Verify these align with backend/provider defaults and docs, and ensure cross‑field validation (
initial <= max) is enforced at schema level.Would you confirm that
networkOnlyFormSchemaenforcesretry_backoff_initial <= retry_backoff_maxand that the docs reflect 500/10000 ms?ui/app/providers/fragments/apiStructureFormFragment.tsx (3)
7-7: Extraction to AllowedRequestsFields: good consolidation.Removes duplication and centralizes allowed-requests UI.
83-91: Conditional restart alert: consistent and correct.Good alignment with other fragments; gated by
showRestartAlert && form.formState.isDirty.
121-123: Component usage is clean.
<AllowedRequestsFields control={form.control} />reads well and keeps the fragment lean.
TejasGhatte
changed the base branch from
09-13-enhancement_configuration_updates_in_maxim_plugin
to
graphite-base/478
TejasGhatte
force-pushed
the
graphite-base/478
branch
from
0bcba23 to
75bed46
Compare
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
f5b100b to
2748b9c
Compare
TejasGhatte
changed the base branch from
graphite-base/478
to
09-13-enhancement_configuration_updates_in_maxim_plugin
TejasGhatte
changed the base branch from
09-13-enhancement_configuration_updates_in_maxim_plugin
to
graphite-base/478
TejasGhatte
force-pushed
the
graphite-base/478
branch
from
75bed46 to
f99b9e3
Compare
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
2748b9c to
ef8079d
Compare
TejasGhatte
changed the base branch from
graphite-base/478
to
09-13-enhancement_configuration_updates_in_maxim_plugin
TejasGhatte
changed the base branch from
09-13-enhancement_configuration_updates_in_maxim_plugin
to
graphite-base/478
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/lib/types/schemas.ts (1)
121-129: URL is required even for “environment”/“none” proxy types.
proxyConfigSchemamarksurlas required, contradicting the conditional refinements and the UI flow for “Environment”/“None”.Apply:
export const proxyConfigSchema = z .object({ type: proxyTypeSchema, - url: z.url("Must be a valid URL"), + url: z.url("Must be a valid URL").optional(), username: z.string().optional(), password: z.string().optional(), }) .refine((data) => !(data.type === "http" || data.type === "socks5") || (data.url && data.url.trim().length > 0), { message: "Proxy URL is required when using HTTP or SOCKS5 proxy", path: ["url"], })Also applies to: 129-147
🧹 Nitpick comments (4)
ui/app/providers/fragments/proxyFormFragment.tsx (2)
33-41: Expose “None” in the proxy type UI and avoid empty-string values.Currently the Select shows a blank state when the value is
"none", which is confusing and makes the control’s value diverge from form state. Default to"none"and add an explicit option.Apply:
@@ - proxy_config: { - type: provider.proxy_config?.type, + proxy_config: { + type: provider.proxy_config?.type ?? "none", @@ - <Select onValueChange={field.onChange} value={field.value === "none" ? "" : field.value}> + <Select onValueChange={field.onChange} value={field.value ?? "none"}> @@ - <SelectContent> + <SelectContent> + <SelectItem value="none">None</SelectItem> <SelectItem value="http">HTTP</SelectItem> <SelectItem value="socks5">SOCKS5</SelectItem> <SelectItem value="environment">Environment</SelectItem> </SelectContent>Also applies to: 103-114
64-68: Avoid sending stale URL/credentials when type is “environment” or “none”.If a user switches from HTTP/SOCKS5 to Environment, hidden fields can retain values and be sent unintentionally.
Apply:
- proxy_config: { - type: data.proxy_config?.type ?? "none", - url: data.proxy_config?.url || undefined, - username: data.proxy_config?.username || undefined, - password: data.proxy_config?.password || undefined, - }, + // Only include URL/creds when the proxy type actually needs them + ...(() => { + const type = data.proxy_config?.type ?? "none"; + const needsAuth = type === "http" || type === "socks5"; + return { + proxy_config: { + type, + url: needsAuth ? data.proxy_config?.url || undefined : undefined, + username: needsAuth ? data.proxy_config?.username || undefined : undefined, + password: needsAuth ? data.proxy_config?.password || undefined : undefined, + }, + }; + })(),ui/lib/types/schemas.ts (1)
74-74: Align validation message with constraint for max_retries.Message says “greater than 0” but constraint is
min(0).Apply:
- max_retries: z.number().min(0, "Max retries must be greater than 0").max(10, "Max retries must be less than 10"), + max_retries: z.number().min(0, "Max retries must be greater than or equal to 0").max(10, "Max retries must be less than 10"),ui/app/config/page.tsx (1)
301-305: Fix label association and description for Max Request Body Size.The label’s
htmlForpoints to the wrong id, and the description is copy/pasted.Apply:
- <label htmlFor="initial-pool-size" className="text-sm font-medium"> - Max Request Body Size + <label htmlFor="max-request-body-size-mb" className="text-sm font-medium"> + Max Request Body Size (MB) </label> - <p className="text-muted-foreground text-sm">The initial connection pool size.</p> + <p className="text-muted-foreground text-sm"> + Maximum allowed request body size in megabytes for HTTP and WebSocket requests. + </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ui/app/config/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/lib/types/schemas.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/app/providers/fragments/index.ts
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/providers/fragments/allowedRequestsFields.tsx
- ui/app/providers/dialogs/addNewCustomProviderDialog.tsx
- ui/app/providers/fragments/apiStructureFormFragment.tsx
- ui/app/providers/fragments/networkFormFragment.tsx
- ui/app/providers/dialogs/addNewKeyDialog.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/config/page.tsx (1)
ui/lib/types/config.ts (1)
CoreConfig(210-221)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
ui/app/providers/fragments/proxyFormFragment.tsx (1)
84-92: Good: restart alert gated by form dirtiness.Rendering the alert only when both
showRestartAlertandform.formState.isDirtyare true is correct and matches the stated UX.ui/app/config/page.tsx (1)
18-29: LGTM: defaultConfig typed and updated with enable_maxim.Consistent with
CoreConfigand the schema changes.
TejasGhatte
force-pushed
the
graphite-base/478
branch
from
f99b9e3 to
d6b2896
Compare
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
ef8079d to
1d82802
Compare
TejasGhatte
changed the base branch from
graphite-base/478
to
09-13-enhancement_configuration_updates_in_maxim_plugin
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
2 times, most recently
from
5805660 to
07dde20
Compare
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
d6b2896 to
d659da9
Compare
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
2 times, most recently
from
5666888 to
1a69a17
Compare
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
1a69a17 to
a7859e0
Compare
Pratham-Mishra04
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
d659da9 to
729b729
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/providers/fragments/networkFormFragment.tsx (1)
220-224: “Remove configuration” doesn’t actually remove network_config and bypasses resolver.This path resets the form and then reuses onSubmit, which rebuilds network_config (with defaults) instead of deleting it. Call the mutation directly with network_config: undefined.
Apply:
- onClick={() => { - form.reset({ - network_config: undefined, - }); - onSubmit(form.getValues()); - }} + onClick={async () => { + form.reset({ network_config: undefined }); + try { + await updateProvider({ ...provider, network_config: undefined }).unwrap(); + toast.success("Removed network configuration"); + } catch (err) { + toast.error("Failed to remove provider configuration", { description: getErrorMessage(err) }); + } + }}Optionally relax the disabled condition to allow removal whenever provider.network_config exists, not only when base_url is set.
♻️ Duplicate comments (1)
ui/app/providers/fragments/networkFormFragment.tsx (1)
144-182: Extra headers: consider stricter onBlur validation (duplicate of earlier thread).Current onBlur accepts any JSON object (arrays included) and doesn’t surface field-level errors. If schema requires Record<string, string>, either enforce here or guarantee it in zod.
If you prefer schema-level enforcement, ensure networkOnlyFormSchema uses something like:
- z.record(z.string()) for values, and
- a preprocess to parse string → object safely.
🧹 Nitpick comments (9)
ui/app/providers/fragments/apiStructureFormFragment.tsx (5)
83-91: Restart alert gating looks good; add a11y tweaks.Add role and hide the icon from AT; set a warning variant if supported.
-{showRestartAlert && form.formState.isDirty && ( - <Alert> - <AlertTriangle className="h-4 w-4" /> +{showRestartAlert && form.formState.isDirty && ( + <Alert role="alert" variant="warning"> + <AlertTriangle className="h-4 w-4" aria-hidden="true" /> <AlertDescription> The settings below require a Bifrost service restart to take effect. Current connections will continue with existing settings until restart. </AlertDescription> </Alert> )}
56-58: Reset currently risks unsetting new boolean flags.Resetting with a partial
custom_provider_configcan turn newly added request-type flags intoundefined(rendering as false). Merge with defaults on reset.-useEffect(() => { - form.reset(provider.custom_provider_config); -}, [form, provider.name, provider.custom_provider_config]); +useEffect(() => { + form.reset({ + base_provider_type: provider.custom_provider_config?.base_provider_type ?? "openai", + allowed_requests: { + text_completion: provider.custom_provider_config?.allowed_requests?.text_completion ?? true, + chat_completion: provider.custom_provider_config?.allowed_requests?.chat_completion ?? true, + chat_completion_stream: provider.custom_provider_config?.allowed_requests?.chat_completion_stream ?? true, + embedding: provider.custom_provider_config?.allowed_requests?.embedding ?? true, + speech: provider.custom_provider_config?.allowed_requests?.speech ?? true, + speech_stream: provider.custom_provider_config?.allowed_requests?.speech_stream ?? true, + transcription: provider.custom_provider_config?.allowed_requests?.transcription ?? true, + transcription_stream: provider.custom_provider_config?.allowed_requests?.transcription_stream ?? true, + }, + }); +}, [form, provider.name, provider.custom_provider_config]);
52-55: Exhaustive-deps: include dispatch.Small cleanup to satisfy the linter and future-proof.
-useEffect(() => { +useEffect(() => { dispatch(setProviderFormDirtyState(form.formState.isDirty)); -}, [form.formState.isDirty]); +}, [dispatch, form.formState.isDirty]);
129-140: Tooltip on disabled Button won’t trigger.Radix Tooltip won’t fire on disabled elements. Wrap the Button in a focusable span.
-<Tooltip> - <TooltipTrigger asChild> - <Button type="submit" disabled={!form.formState.isDirty || !form.formState.isValid} isLoading={isUpdatingProvider}> - Save API Structure Configuration - </Button> - </TooltipTrigger> +<Tooltip> + <TooltipTrigger asChild> + <span tabIndex={0}> + <Button type="submit" disabled={!form.formState.isDirty || !form.formState.isValid} isLoading={isUpdatingProvider}> + Save API Structure Configuration + </Button> + </span> + </TooltipTrigger> {!form.formState.isValid && ( <TooltipContent> <p>{form.formState.errors.root?.message || "Please fix validation errors"}</p> </TooltipContent> )} </Tooltip>
65-65: Avoid double cast.Prefer a single cast (or better: type the Zod schema to
KnownProvider).- base_provider_type: data.base_provider_type as unknown as KnownProvider, + base_provider_type: data.base_provider_type as KnownProvider,ui/app/providers/dialogs/addNewKeyDialog.tsx (2)
16-17: Optional: route user‑visible strings through i18n.If the app uses localization, wrap these strings with your translation helper for consistency.
Also applies to: 28-28, 38-38
15-17: Fix isEditing guard to handle negative/out-of-range indices and missing provider.keys.modelProviderKeysTableView.tsx passes keyIndex as 0..len or len for add (no -1 sentinel found). Make AddNewKeyDialog robust by validating keyIndex and provider.keys. Apply:
-const isEditing = keyIndex < provider.keys.length; -const dialogTitle = isEditing ? "Edit key" : "Add new key"; -const successMessage = isEditing ? "Key updated successfully" : "Key added successfully"; +const keysLen = provider.keys?.length ?? 0; +const isEditing = Number.isInteger(keyIndex) && keyIndex >= 0 && keyIndex < keysLen; +const dialogTitle = isEditing ? "Edit Key" : "Add New Key"; +const successMessage = isEditing ? "Key updated successfully" : "Key added successfully";ui/app/providers/fragments/networkFormFragment.tsx (2)
72-79: Use nullish coalescing and align defaults with onSubmit.
- || will clobber valid 0 for default_request_timeout_in_seconds; use ??.
- Keep retry_backoff_max consistent with onSubmit (choose 5000 or 10000, but be consistent with schema).
Apply:
network_config: { base_url: provider.network_config?.base_url || "", extra_headers: provider.network_config?.extra_headers, - default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds || 30, - max_retries: provider.network_config?.max_retries || 0, - retry_backoff_initial: provider.network_config?.retry_backoff_initial || 500, - retry_backoff_max: provider.network_config?.retry_backoff_max || 5000, + default_request_timeout_in_seconds: provider.network_config?.default_request_timeout_in_seconds ?? 30, + max_retries: provider.network_config?.max_retries ?? 0, + retry_backoff_initial: provider.network_config?.retry_backoff_initial ?? 500, + retry_backoff_max: provider.network_config?.retry_backoff_max ?? 5000, },
183-210: Numeric inputs: use type=number and valueAsNumber with NaN guard.Prevents accidental “empty → 0” coercion and improves mobile UX.
Apply:
- <Input placeholder="500" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + type="number" + inputMode="numeric" + min={0} + step={100} + placeholder="500" + {...field} + onChange={(e) => { + const n = e.currentTarget.valueAsNumber; + field.onChange(Number.isNaN(n) ? undefined : n); + }} + />- <Input placeholder="5000" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input + type="number" + inputMode="numeric" + min={0} + step={100} + placeholder="5000" + {...field} + onChange={(e) => { + const n = e.currentTarget.valueAsNumber; + field.onChange(Number.isNaN(n) ? undefined : n); + }} + />For consistency, consider applying the same pattern to the Timeout and Max Retries fields above (lines 124 and 137).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/version(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/schemas.ts(3 hunks)
✅ Files skipped from review due to trivial changes (7)
- plugins/logging/version
- plugins/semanticcache/version
- transports/version
- plugins/telemetry/version
- plugins/governance/version
- plugins/mocker/version
- core/version
🚧 Files skipped from review as they are similar to previous changes (11)
- framework/version
- ui/app/providers/fragments/proxyFormFragment.tsx
- plugins/maxim/version
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/app/providers/fragments/allowedRequestsFields.tsx
- ui/lib/types/schemas.ts
- plugins/jsonparser/version
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/providers/fragments/index.ts
- ui/app/providers/dialogs/addNewCustomProviderDialog.tsx
- ui/app/config/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/providers/fragments/networkFormFragment.tsx (2)
ui/components/ui/textarea.tsx (1)
Textarea(18-18)ui/components/ui/input.tsx (1)
Input(7-21)
ui/app/providers/fragments/apiStructureFormFragment.tsx (2)
ui/components/ui/alert.tsx (2)
Alert(41-41)AlertDescription(41-41)ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (9)
ui/app/providers/fragments/apiStructureFormFragment.tsx (3)
7-7: Nice dedup via AllowedRequestsFields.Good move extracting the toggles into a shared fragment to remove duplication.
60-68: Confirm update semantics; avoid dropping unrelated config fields.If
updateProviderreplacescustom_provider_configserver‑side, this will discard fields owned by other tabs. Prefer shallow merge to preserve unknown keys.- custom_provider_config: { - base_provider_type: data.base_provider_type as unknown as KnownProvider, - allowed_requests: data.allowed_requests, - }, + custom_provider_config: { + ...(provider.custom_provider_config ?? {}), + base_provider_type: data.base_provider_type as KnownProvider, + allowed_requests: { + ...(provider.custom_provider_config?.allowed_requests ?? {}), + ...data.allowed_requests, + }, + },Would you confirm whether the API performs a merge vs. replace? If replace, the above change is safer.
122-122: LGTM on integration of AllowedRequestsFields.Good reuse and clear separation of concerns.
ui/app/providers/dialogs/addNewKeyDialog.tsx (3)
20-21: Confirm Dialog onOpenChange signature.If
Dialogexpects(open: boolean) => void, passingonCancel: () => voidis a type mismatch. Prefer:-<Dialog open={show} onOpenChange={onCancel}> +<Dialog open={show} onOpenChange={(open) => { if (!open) onCancel(); }}>
28-28: LGTM: dynamic title wired correctly.
38-38: LGTM: contextual success toast.ui/app/providers/fragments/networkFormFragment.tsx (3)
7-7: Import looks good.Textarea import is correct and scoped to the single usage below.
86-94: Nice: restart alert gated by isDirty.Better UX; avoids spurious banners.
53-58: Fix defaults drift & allow true removal of network_configFile: ui/app/providers/fragments/networkFormFragment.tsx (lines 53-58)
- If data.network_config is undefined this still constructs a network_config object (due to ...provider.network_config) so the remove flow doesn't actually remove it.
- retry_backoff_max default here (10000) disagrees with reset (5000); keep a single source of truth for defaults.
Apply:
- const updatedProvider: ModelProvider = { - ...provider, - network_config: { - ...provider.network_config, - base_url: data.network_config?.base_url || undefined, - extra_headers: data.network_config?.extra_headers || undefined, - default_request_timeout_in_seconds: data.network_config?.default_request_timeout_in_seconds ?? 30, - max_retries: data.network_config?.max_retries ?? 0, - retry_backoff_initial: data.network_config?.retry_backoff_initial ?? 500, - retry_backoff_max: data.network_config?.retry_backoff_max ?? 10000, - }, - }; + const nextNetworkConfig = + data.network_config === undefined + ? undefined + : { + ...provider.network_config, + base_url: (data.network_config.base_url || "").trim() || undefined, + extra_headers: data.network_config.extra_headers || undefined, + default_request_timeout_in_seconds: data.network_config.default_request_timeout_in_seconds ?? 30, + max_retries: data.network_config.max_retries ?? 0, + retry_backoff_initial: data.network_config.retry_backoff_initial ?? 500, + retry_backoff_max: data.network_config.retry_backoff_max ?? 5000, + }; + const updatedProvider: ModelProvider = { ...provider, network_config: nextNetworkConfig };Confirm the intended default for retry_backoff_max in the schema and make both places match.
TejasGhatte
changed the base branch from
09-13-enhancement_configuration_updates_in_maxim_plugin
to
graphite-base/478
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
2771a81 to
c5897d2
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/478
branch
from
9ab3d68 to
2156d53
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/478
to
09-13-enhancement_configuration_updates_in_maxim_plugin
Pratham-Mishra04
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
2156d53 to
91e2d09
Compare
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
c5897d2 to
2dc4ebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
ui/app/providers/views/modelProviderConfig.tsx (7)
9-9: keysRequired import: verify provider IDs stay in sync with backend enums.If backend identifiers change (e.g., "sgl" vs "sglx"), this UI gate could drift. Consider centralizing from a shared type or server‑driven capability flag.
47-53: useMemo dependency may be stale vs. whatavailableTabs(provider)actually uses.
availableTabsinspectsprovider.custom_provider_config, but the memo only depends onprovider.name. Depend onprovider(or include the specific fields) to avoid stale tabs.- const tabs = useMemo(() => { - return availableTabs(provider); - }, [provider.name]); + const tabs = useMemo(() => availableTabs(provider), [provider]);
66-66: Avoid mixing controlledvaluewithdefaultValueon Tabs.You’re fully controlling with
value; dropdefaultValueto reduce confusion.- <Tabs defaultValue={tabs[0]?.id} value={selectedTab} onValueChange={setSelectedTab} className="space-y-6"> + <Tabs value={selectedTab} onValueChange={setSelectedTab} className="space-y-6">Optionally initialize state with the first tab to avoid an initial undefined:
- const [selectedTab, setSelectedTab] = useState<string | undefined>(undefined); + const [selectedTab, setSelectedTab] = useState<string | undefined>(() => availableTabs(provider)[0]?.id);
68-70:+ 3in gridTemplateColumns likely leaves empty columns.Use
tabs.length(or auto-fit) to match triggers and avoid layout gaps.- style={{ gridTemplateColumns: `repeat(${tabs.length + 3}, 1fr)` }} + style={{ gridTemplateColumns: `repeat(${tabs.length}, 1fr)` }}Or:
- style={{ gridTemplateColumns: `repeat(${tabs.length + 3}, 1fr)` }} + style={{ gridTemplateColumns: `repeat(auto-fit, minmax(0, 1fr))` }}
78-85: PassingshowRestartAlertexplicitly is good; minor DRY nit.Consider a local const to avoid repetition and ease future toggling.
+ const showRestartAlert = true; ... - <ApiStructureFormFragment provider={provider} showRestartAlert={true} /> + <ApiStructureFormFragment provider={provider} showRestartAlert={showRestartAlert} /> ... - <NetworkFormFragment provider={provider} showRestartAlert={true} /> + <NetworkFormFragment provider={provider} showRestartAlert={showRestartAlert} /> ... - <ProxyFormFragment provider={provider} showRestartAlert={true} /> + <ProxyFormFragment provider={provider} showRestartAlert={showRestartAlert} />
4-6: Import consistency nit.Either re-export
NetworkFormFragmentfrom the fragments index or import all fragments directly to keep import style uniform.
94-99: API keys section conditional render: looks good; consider UX hint.Optional: add a brief caption explaining why keys are hidden for this provider to reduce user confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/version(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/schemas.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- transports/version
- framework/version
- plugins/governance/version
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/semanticcache/version
- plugins/logging/version
- plugins/mocker/version
- ui/app/providers/dialogs/addNewKeyDialog.tsx
- core/version
- ui/app/providers/fragments/proxyFormFragment.tsx
- ui/app/config/page.tsx
- ui/app/providers/views/modelProviderKeysTableView.tsx
- plugins/telemetry/version
- ui/app/providers/dialogs/addNewCustomProviderDialog.tsx
- ui/app/providers/fragments/networkFormFragment.tsx
- ui/app/providers/fragments/index.ts
- ui/app/providers/fragments/allowedRequestsFields.tsx
- ui/lib/types/schemas.ts
- plugins/jsonparser/version
- ui/app/providers/fragments/apiStructureFormFragment.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/providers/views/modelProviderConfig.tsx (4)
ui/app/providers/views/utils.ts (1)
keysRequired(1-1)ui/app/providers/fragments/apiStructureFormFragment.tsx (1)
ApiStructureFormFragment(31-147)ui/app/providers/fragments/networkFormFragment.tsx (1)
NetworkFormFragment(24-258)ui/app/providers/fragments/proxyFormFragment.tsx (1)
ProxyFormFragment(26-196)
⏰ 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). (12)
- 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 (2)
ui/app/providers/views/modelProviderConfig.tsx (1)
55-55: Gating API keys UI is correct.This keeps keys hidden for providers that don’t need them; aligns with PR goals.
plugins/maxim/version (1)
1-1: Version bump to 1.3.0 — verify release hygieneSanity check: plugins/maxim/version = 1.3.0; other version files found: core/version 1.1.34, framework/version 1.0.17, plugins/governance 1.2.10, plugins/jsonparser 1.2.9, plugins/logging 1.2.10, plugins/mocker 1.2.8, plugins/semanticcache 1.2.11, plugins/telemetry 1.2.9, transports/version 1.2.14. Confirm CHANGELOG/release notes updated and bump any dependent plugin/core/framework versions if this is intended as a coordinated stack release.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (6)
16-25: Don’t duplicate allowedRequestsSchema; import the shared schema to avoid drift.This inline schema mirrors ui/lib/types/schemas.ts. Centralize to the shared source.
-const allowedRequestsSchema = z.object({ - text_completion: z.boolean(), - chat_completion: z.boolean(), - chat_completion_stream: z.boolean(), - embedding: z.boolean(), - speech: z.boolean(), - speech_stream: z.boolean(), - transcription: z.boolean(), - transcription_stream: z.boolean(), -});Add this import near the other imports:
+import { allowedRequestsSchema } from "@/lib/types/schemas";
27-31: Constrain baseFormat and require at least one allowed request.
- Use a z.enum for base formats to prevent invalid values.
- Add a refinement ensuring at least one request type is enabled.
-const formSchema = z.object({ - name: z.string().min(1), - baseFormat: z.string().min(1), - allowed_requests: allowedRequestsSchema, -}); +const formSchema = z + .object({ + name: z.string().min(1), + baseFormat: z.enum(["openai", "anthropic", "gemini", "cohere", "bedrock"]), + allowed_requests: allowedRequestsSchema, + }) + .refine( + (v) => Object.values(v.allowed_requests).some(Boolean), + { message: "Select at least one request type.", path: ["allowed_requests"] } + );
124-128: Use controlled value instead of defaultValue for Select.defaultValue won’t update on form.reset(); use value for a controlled field.
-<Select onValueChange={field.onChange} defaultValue={field.value}> +<Select onValueChange={field.onChange} value={field.value || undefined}>
61-67: Avoid unsafe cast to ModelProviderName.
data.nameis free-form; casting toModelProviderNamebypasses type safety and can mis-shape the mutation payload if the union excludes custom names. Prefer widening the mutation type to accept a string for custom providers or validate the name against an allowed pattern.Would you like me to propose typings for
useCreateProviderMutationthat separate known vs. custom providers?
48-57: DRY up default allowed_requests.These defaults duplicate the schema fields. Consider a shared constant (e.g.,
ALL_REQUESTS_ENABLED) imported where needed to prevent drift.Example:
// ui/lib/types/constants.ts export const ALL_REQUESTS_ENABLED = { text_completion: true, chat_completion: true, chat_completion_stream: true, embedding: true, speech: true, speech_stream: true, transcription: true, transcription_stream: true, } as const;Then:
-allowed_requests: { ...inline... }, +allowed_requests: ALL_REQUESTS_ENABLED,
91-99: Confirm restart notice behavior and wording.Showing the “restart required” alert as soon as the form is dirty may confuse users on initial creation. If restarts are only required after saving, consider gating on a
showRestartAlertprop or displaying the notice post‑save.Do we require a restart immediately upon creating a provider, or only after persisting changes?
ui/app/providers/fragments/apiStructureFormFragment.tsx (1)
17-17: Remove unused import.The
fiimport fromzod/v4/localesappears to be unused in this file and should be removed to keep imports clean.Apply this diff to remove the unused import:
-import { fi } from "zod/v4/locales";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/version(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/schemas.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- plugins/mocker/version
- plugins/jsonparser/version
🚧 Files skipped from review as they are similar to previous changes (17)
- plugins/telemetry/version
- ui/app/config/page.tsx
- framework/version
- ui/app/providers/views/modelProviderKeysTableView.tsx
- core/version
- ui/app/providers/dialogs/addNewKeyDialog.tsx
- plugins/maxim/version
- ui/app/providers/fragments/proxyFormFragment.tsx
- plugins/semanticcache/version
- ui/lib/types/schemas.ts
- plugins/logging/version
- plugins/governance/version
- transports/version
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/providers/fragments/index.ts
- ui/app/providers/fragments/allowedRequestsFields.tsx
- ui/app/providers/fragments/networkFormFragment.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/providers/fragments/apiStructureFormFragment.tsx (1)
ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (2)
ui/lib/types/schemas.ts (1)
allowedRequestsSchema(193-202)ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (8)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (4)
84-84: DialogContent sizing/scrolling looks good.Max height + custom scrollbar is consistent with long forms.
143-145: AllowedRequestsFields integration is clean.Good reuse of the fragment with the default
namePrefix. No issues.
6-6: Alert components import is appropriate.Matches usage below; no issues.
13-15: New imports LGTM.Icon and fragment paths are correct and used.
ui/app/providers/fragments/apiStructureFormFragment.tsx (4)
7-7: Good refactoring for code reusability.The extraction of allowed requests fields into a dedicated component eliminates code duplication and improves maintainability across the provider configuration forms.
18-19: Good addition of proper Alert component.The imports for Alert components are correctly added and align with the UI component structure.
83-91: Excellent improvement to conditional alert display.The restart alert now correctly displays only when both
showRestartAlertis true AND the form has been modified (form.formState.isDirty). This prevents unnecessary alerts from appearing when users haven't made any changes, improving the user experience.
122-122: Clean component usage.The replacement of the duplicated allowed requests fields with the reusable
AllowedRequestsFieldscomponent significantly reduces code duplication and maintains consistent functionality.
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
2 times, most recently
from
f2a70ce to
ffff75a
Compare
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
ffff75a to
d37ab4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (1)
83-85: onOpenChange handler fix matches Dialog’s signatureGood catch; wraps onClose on false as previously requested.
🧹 Nitpick comments (3)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (2)
16-25: Deduplicate allowedRequestsSchema by importing the shared schemaThis local schema duplicates ui/lib/types/schemas.ts and risks drift.
Apply this diff to remove the local definition:
-const allowedRequestsSchema = z.object({ - text_completion: z.boolean(), - chat_completion: z.boolean(), - chat_completion_stream: z.boolean(), - embedding: z.boolean(), - speech: z.boolean(), - speech_stream: z.boolean(), - transcription: z.boolean(), - transcription_stream: z.boolean(), -});Then add this import (outside the changed hunk):
import { allowedRequestsSchema } from "@/lib/types/schemas";
48-57: Avoid inline defaults: extract a single source of truth for allowed_requestsDefine a DEFAULT_ALLOWED_REQUESTS constant (or use schema defaults) and reuse across forms/dialogs to prevent divergence.
Example (place near imports):
const DEFAULT_ALLOWED_REQUESTS: z.infer<typeof allowedRequestsSchema> = { text_completion: true, chat_completion: true, chat_completion_stream: true, embedding: true, speech: true, speech_stream: true, transcription: true, transcription_stream: true, };Then:
allowed_requests: DEFAULT_ALLOWED_REQUESTS,docs/features/observability.mdx (1)
230-230: Punctuation nitAdd a period for consistency with the surrounding bullets.
-* **Alerts & SLAs**: Threshold-based notifications to keep quality and latency in guardrails +* **Alerts & SLAs**: Threshold-based notifications to keep quality and latency in guardrails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/version(1 hunks)docs/features/observability.mdx(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/schemas.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- plugins/logging/version
- plugins/mocker/version
- framework/version
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/governance/version
- plugins/maxim/version
- ui/app/config/page.tsx
- plugins/telemetry/version
- ui/app/providers/fragments/proxyFormFragment.tsx
- plugins/jsonparser/version
- ui/app/providers/fragments/index.ts
- core/version
- ui/app/providers/views/modelProviderConfig.tsx
- plugins/semanticcache/version
- ui/app/providers/fragments/apiStructureFormFragment.tsx
- ui/lib/types/schemas.ts
- ui/app/providers/fragments/allowedRequestsFields.tsx
- transports/version
- ui/app/providers/dialogs/addNewKeyDialog.tsx
- ui/app/providers/fragments/networkFormFragment.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (2)
ui/lib/types/schemas.ts (1)
allowedRequestsSchema(193-202)ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
⏰ 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 (5)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
71-73: Success toast on delete looks goodNice UX touch; placement after unwrap() is correct and dialog closes reliably.
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
66-67: Payload wiring LGTMForwarding allowed_requests under custom_provider_config is correct.
91-97: Confirm restart requirement text for this dialogDoes creating a custom provider always require a Bifrost restart, or only for specific backends? If conditional, consider gating the alert behind a prop (like other fragments) or clarifying the message.
141-143: Reusable AllowedRequestsFields integration is cleanGood reuse; default namePrefix aligns with form shape.
docs/features/observability.mdx (1)
208-210: Clarify scope of supported request typesOnly “Text Completion” and “Chat Completion” are listed. If embeddings/speech/transcription aren’t observed by the Maxim plugin, add a note to avoid confusion with the provider UI toggles. If they are supported, list them here.
TejasGhatte
force-pushed
the
09-13-enhancement_configuration_updates_in_maxim_plugin
branch
from
37bc39a to
c81c8bb
Compare
TejasGhatte
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
d37ab4a to
6a67135
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/config/views/pluginsForm.tsx (1)
271-281: Numeric parsing bug: “0” becomes default due to||fallback.
thresholdcannot be set to 0 becauseparseFloat(...) || 0.8treats 0 as falsy; similar patterns exist in other fields. UseNumber.isNaNchecks.- onChange={(e) => debouncedUpdateCacheConfig({ threshold: parseFloat(e.target.value) || 0.8 })} + onChange={(e) => { + const v = parseFloat(e.target.value); + debouncedUpdateCacheConfig({ threshold: Number.isNaN(v) ? 0.8 : v }); + }}[ suggest_optional_refactor ]
Apply the same pattern to these handlers for consistency/correctness:- onChange={(e) => debouncedUpdateCacheConfig({ ttl_seconds: parseInt(e.target.value) || 300 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + debouncedUpdateCacheConfig({ ttl_seconds: Number.isNaN(v) ? 300 : v }); + }} - onChange={(e) => debouncedUpdateCacheConfig({ dimension: parseInt(e.target.value) || 0 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + debouncedUpdateCacheConfig({ dimension: Number.isNaN(v) ? 0 : v }); + }} - onChange={(e) => debouncedUpdateCacheConfig({ conversation_history_threshold: parseInt(e.target.value) || 3 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + debouncedUpdateCacheConfig({ conversation_history_threshold: Number.isNaN(v) ? 3 : v }); + }}Also applies to: 261-269, 283-291, 305-312
ui/app/providers/fragments/networkFormFragment.tsx (1)
42-47: “Remove configuration” can’t work for custom providers (hard base URL check).When clicking Remove, the form sets
network_configtoundefinedand callsonSubmit, but the base‑URL requirement blocks submission. Allow clearing config for custom providers by only enforcing base URL whennetwork_configexists.- if (isCustomProvider && !(data.network_config?.base_url || "").trim()) { + if (isCustomProvider && data.network_config !== undefined && !(data.network_config?.base_url || "").trim()) { toast.error("Base URL is required for custom providers."); return; }
🧹 Nitpick comments (5)
ui/app/config/views/pluginsForm.tsx (2)
200-209: Fix switch state: reflect actual plugin enablement even when vector store is disabled.Using
checked={isSemanticCacheEnabled && isVectorStoreEnabled}renders the toggle “off” when the plugin is enabled but the vector store is disabled, which misrepresents current state. Keep it checked based on plugin state; only disable interaction.-checked={isSemanticCacheEnabled && isVectorStoreEnabled} +checked={isSemanticCacheEnabled} disabled={!isVectorStoreEnabled || providersLoading || providers.length === 0}
124-160: Avoid success-toast spam on debounced updates.Saving on every keystroke shows a success toast each time. Gate toasts with an ID or switch to a silent save and a single “saved” toast on blur/settle.
- .then(() => { - toast.success("Cache configuration updated successfully"); - }) + .then(() => { + toast.success("Cache configuration updated", { id: "cache-config" }); + })docs/features/observability.mdx (1)
202-219: Consistent bullet punctuation.Some bullets end with periods, others don’t. Make punctuation consistent for a tighter doc.
ui/app/providers/fragments/networkFormFragment.tsx (2)
57-58: Align retry backoff defaults (5000 vs 10000).
onSubmitusesretry_backoff_max ?? 10000but the form resets to5000and the placeholder shows 5000. Use one default.- retry_backoff_max: data.network_config?.retry_backoff_max ?? 10000, + retry_backoff_max: data.network_config?.retry_backoff_max ?? 5000,Also applies to: 69-81
184-210: Avoid coercing empty input to 0 for numeric fields.
Number(e.target.value)turns empty string into 0, producing invalid states and surprising users. Parse and passundefinedfor empty to let Zod/RHF validate properly.- <Input placeholder="500" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input placeholder="500" {...field} onChange={(e) => { + const s = e.target.value; + field.onChange(s === "" ? undefined : Number(s)); + }} /> - <Input placeholder="5000" {...field} onChange={(e) => field.onChange(Number(e.target.value))} /> + <Input placeholder="5000" {...field} onChange={(e) => { + const s = e.target.value; + field.onChange(s === "" ? undefined : Number(s)); + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/version(1 hunks)docs/features/observability.mdx(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(3 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(0 hunks)ui/lib/types/schemas.ts(3 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (1)
- plugins/maxim/version
🚧 Files skipped from review as they are similar to previous changes (19)
- plugins/semanticcache/version
- plugins/jsonparser/version
- ui/app/providers/fragments/proxyFormFragment.tsx
- plugins/telemetry/version
- core/version
- transports/version
- plugins/governance/version
- framework/version
- ui/app/providers/fragments/index.ts
- plugins/mocker/version
- ui/app/providers/views/modelProviderConfig.tsx
- ui/app/providers/fragments/apiStructureFormFragment.tsx
- ui/app/config/page.tsx
- plugins/logging/version
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/app/providers/dialogs/addNewKeyDialog.tsx
- ui/lib/types/schemas.ts
- ui/app/providers/dialogs/addNewCustomProviderDialog.tsx
- ui/app/providers/fragments/allowedRequestsFields.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/config/views/pluginsForm.tsx (1)
ui/lib/types/config.ts (1)
ModelProviderName(12-12)
ui/app/providers/fragments/networkFormFragment.tsx (3)
ui/components/ui/form.tsx (5)
Form(160-160)FormItem(161-161)FormLabel(162-162)FormControl(163-163)FormMessage(165-165)ui/components/ui/textarea.tsx (1)
Textarea(18-18)ui/components/ui/input.tsx (1)
Input(7-21)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
docs/features/observability.mdx (1)
197-199: LGTM: clearer request types.Switching to a simple list improves readability.
ui/app/providers/fragments/networkFormFragment.tsx (1)
86-94: LGTM: restart alert gated by form dirtiness.Good UX—avoids noisy alerts unless there are changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ui/lib/types/schemas.ts (1)
121-146:proxyConfigSchema.urlis required even when type isnone/environment.As written,
urlis always required (z.url()), but later refines imply it’s only required forhttp/socks5. This blocks valid configs fornone/environment.export const proxyConfigSchema = z .object({ type: proxyTypeSchema, - url: z.url("Must be a valid URL"), + url: z.string().optional(), username: z.string().optional(), password: z.string().optional(), }) .refine((data) => !(data.type === "http" || data.type === "socks5") || (data.url && data.url.trim().length > 0), { message: "Proxy URL is required when using HTTP or SOCKS5 proxy", path: ["url"], }) .refine( (data) => { - if ((data.type === "http" || data.type === "socks5") && data.url?.trim()) { + if ((data.type === "http" || data.type === "socks5") && data.url?.trim()) { try { new URL(data.url); return true; } catch { return false; } } return true; }, { message: "Must be a valid URL (e.g., http://proxy.example.com:8080)", path: ["url"] }, );ui/app/config/views/pluginsForm.tsx (2)
261-268: Numeric parsing:||default masks valid 0 and intermediate input. Clamp explicitly.Using
|| 300treats0as falsy. Parse with radix 10 and clamp to min.Apply this diff:
- onChange={(e) => debouncedUpdateCacheConfig({ ttl_seconds: parseInt(e.target.value) || 300 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + debouncedUpdateCacheConfig({ ttl_seconds: Number.isNaN(v) ? 300 : Math.max(1, v) }); + }}
271-280: Numeric parsing:|| 0.8ignores user-entered 0 and doesn’t clamp to [0,1].Parse, validate NaN, then clamp 0–1.
Apply this diff:
- onChange={(e) => debouncedUpdateCacheConfig({ threshold: parseFloat(e.target.value) || 0.8 })} + onChange={(e) => { + const v = parseFloat(e.target.value); + const next = Number.isNaN(v) ? 0.8 : Math.min(1, Math.max(0, v)); + debouncedUpdateCacheConfig({ threshold: next }); + }}
♻️ Duplicate comments (1)
ui/lib/types/schemas.ts (1)
100-106: Backoff mins in form schema still don’t match base schema (regression).
networkFormConfigSchemauses 0/0, whilenetworkConfigSchemaenforces 100/1000. Align the form schema to avoid accepting values the persisted schema rejects. This mirrors an earlier comment; looks like it slipped back in.Apply:
- retry_backoff_initial: z.coerce - .number("Retry backoff initial must be a number") - .min(0, "Retry backoff initial must be greater than 0"), - retry_backoff_max: z.coerce - .number("Retry backoff max must be a number") - .min(0, "Retry backoff max must be greater than 0"), + retry_backoff_initial: z.coerce + .number("Retry backoff initial must be a number") + .min(100, "Initial backoff must be at least 100ms"), + retry_backoff_max: z.coerce + .number("Retry backoff max must be a number") + .min(1000, "Max backoff must be at least 1000ms"),Also applies to: 107-110
🧹 Nitpick comments (16)
ui/lib/types/schemas.ts (5)
74-74: Fix constraint/message mismatch for max_retries (allowing 0).Validators allow 0 but the message says “greater than 0”. Either make it
min(1)or update the message to “at least 0”. Recommending message change to preserve 0 as valid.- max_retries: z.number().min(0, "Max retries must be greater than 0").max(10, "Max retries must be less than 10"), + max_retries: z.number().min(0, "Max retries must be at least 0").max(10, "Max retries must be less than or equal to 10"), ... - .min(0, "Max retries must be greater than 0") - .max(10, "Max retries must be less than 10"), + .min(0, "Max retries must be at least 0") + .max(10, "Max retries must be less than or equal to 10"),Also applies to: 96-99
71-73: Clarify “≤” vs “<” in timeout max messages.
.max(300)is inclusive, but the message says “less than 300”. Update copy.- .max(300, "Timeout must be less than 300 seconds"), + .max(300, "Timeout must be at most 300 seconds"), ... - .max(300, "Timeout must be less than 300 seconds"), + .max(300, "Timeout must be at most 300 seconds"),Also applies to: 93-95
45-46: Weight upper bound message contradicts.max(1)(inclusive).Change copy to “≤ 1” to match the constraint.
- .max(1, "Weight must be less than 1"), + .max(1, "Weight must be less than or equal to 1"),
233-239: Form schema uses persistednetworkConfigSchemainstead of lenientnetworkFormConfigSchema.In forms we typically need coercion and relaxed URL rules. Using the persisted schema risks RHF failing on string inputs.
- network_config: networkConfigSchema.optional(), + network_config: networkFormConfigSchema.optional(),
68-69: Optional: treat empty base_url as undefined via preprocess.Avoid the union with
z.string().length(0)by preprocessing""→undefined, which simplifies consumers.- base_url: z.union([z.string().url("Must be a valid URL"), z.string().length(0)]).optional(), + base_url: z.preprocess( + (v) => (typeof v === "string" && v.trim() === "" ? undefined : v), + z.string().url("Must be a valid URL").optional(), + ),ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
16-25: DeduplicateallowedRequestsSchema– import from shared schemas.Defining it locally risks drift from
ui/lib/types/schemas.ts. Import the shared schema.-import { z } from "zod"; +import { z } from "zod"; +import { allowedRequestsSchema } from "@/lib/types/schemas"; ... -const allowedRequestsSchema = z.object({ - text_completion: z.boolean(), - chat_completion: z.boolean(), - chat_completion_stream: z.boolean(), - embedding: z.boolean(), - speech: z.boolean(), - speech_stream: z.boolean(), - transcription: z.boolean(), - transcription_stream: z.boolean(), -});
121-134: Make Select controlled (RHF + shadcn pattern).Use
value={field.value}instead ofdefaultValueto keep it controlled by RHF.- <Select onValueChange={field.onChange} defaultValue={field.value}> + <Select onValueChange={field.onChange} value={field.value}>
148-149: UX: disable submit while creating.Prevent double submits and show busy state.
- <Button type="submit">Add</Button> + <Button type="submit" disabled={isAddingProvider} aria-busy={isAddingProvider}> + {isAddingProvider ? "Adding…" : "Add"} + </Button>ui/app/providers/fragments/allowedRequestsFields.tsx (2)
23-26: Type the control generically for better DX.Expose a generic so consumers can pass their form type and get field name safety.
-interface AllowedRequestsFieldsProps { - control: Control<any>; +interface AllowedRequestsFieldsProps<TFieldValues = any> { + control: Control<TFieldValues>; namePrefix?: string; } -export function AllowedRequestsFields({ control, namePrefix = "allowed_requests" }: AllowedRequestsFieldsProps) { +export function AllowedRequestsFields<TFieldValues = any>({ + control, + namePrefix = "allowed_requests", +}: AllowedRequestsFieldsProps<TFieldValues>) {
47-47: CheckSwitchAPI: issize="md"supported?If not, drop it to avoid passing unknown props.
Also applies to: 66-66
ui/app/config/views/pluginsForm.tsx (6)
41-45: Avoidanycast when extracting error message.Prefer passing
unknown(or the concrete error type) togetErrorMessageto keep type-safety.Apply this diff:
- toast.error(`Failed to load providers: ${getErrorMessage(providersError as any)}`); + toast.error(`Failed to load providers: ${getErrorMessage(providersError)}`);
66-72: Handle stale/missing provider in existing configs.If an existing plugin config refers to a provider that no longer exists, the Select will render with an invalid value. Normalize to the first available provider when mismatch is detected.
Apply this diff:
-useEffect(() => { - if (providers.length > 0 && !semanticCachePlugin?.config) { - setCacheConfig((prev) => ({ - ...prev, - provider: providers[0].name as ModelProviderName, - })); - } -}, [providers, semanticCachePlugin?.config]); +useEffect(() => { + if (providers.length === 0) return; + const hasValidProvider = providers.some((p) => p.name === cacheConfig.provider); + if (!semanticCachePlugin?.config || !hasValidProvider) { + setCacheConfig((prev) => ({ + ...prev, + provider: providers[0].name as ModelProviderName, + })); + } +}, [providers, semanticCachePlugin?.config, cacheConfig.provider]);
212-220: Hide config panel when no providers are configured.Currently, config UI can render with an empty providers list. Gate the panel on providers length and show a small note instead.
Apply this diff:
- {isSemanticCacheEnabled && - isVectorStoreEnabled && - (providersLoading ? ( + {isSemanticCacheEnabled && + isVectorStoreEnabled && + (providersLoading ? ( <div className="flex items-center justify-center"> <Loader2 className="h-4 w-4 animate-spin" /> </div> - ) : ( - <div className="mt-4 space-y-4"> + ) : providers.length > 0 ? ( + <div className="mt-4 space-y-4"> + </div> + ) : ( + <div className="mt-4 text-sm text-destructive"> + Configure at least one provider to edit cache settings. </div> ))}
283-291: Numeric parsing: avoid|| 0; clamp to non-negative and keep radix.Prevents accidental fallback when user types “0” or during intermediate edits.
Apply this diff:
- onChange={(e) => debouncedUpdateCacheConfig({ dimension: parseInt(e.target.value) || 0 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + debouncedUpdateCacheConfig({ dimension: Number.isNaN(v) ? 0 : Math.max(0, v) }); + }}
303-312: Numeric parsing: same||default issue and no bounds enforcement.Clamp to 1–50 and preserve defaults only on NaN.
Apply this diff:
- onChange={(e) => debouncedUpdateCacheConfig({ conversation_history_threshold: parseInt(e.target.value) || 3 })} + onChange={(e) => { + const v = parseInt(e.target.value, 10); + const next = Number.isNaN(v) ? 3 : Math.min(50, Math.max(1, v)); + debouncedUpdateCacheConfig({ conversation_history_threshold: next }); + }}
147-151: Reduce toast noise on debounced saves.Frequent edits fire repeated success toasts. Consider collapsing to a single “Saving…” → “Saved” toast or suppressing success toasts for debounced background saves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/version(1 hunks)docs/features/observability.mdx(1 hunks)framework/version(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/version(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/version(1 hunks)transports/version(1 hunks)ui/app/config/page.tsx(1 hunks)ui/app/config/views/pluginsForm.tsx(3 hunks)ui/app/providers/dialogs/addNewCustomProviderDialog.tsx(5 hunks)ui/app/providers/dialogs/addNewKeyDialog.tsx(3 hunks)ui/app/providers/fragments/allowedRequestsFields.tsx(1 hunks)ui/app/providers/fragments/apiStructureFormFragment.tsx(4 hunks)ui/app/providers/fragments/index.ts(1 hunks)ui/app/providers/fragments/networkFormFragment.tsx(4 hunks)ui/app/providers/fragments/proxyFormFragment.tsx(1 hunks)ui/app/providers/views/modelProviderConfig.tsx(4 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)ui/lib/types/config.ts(0 hunks)ui/lib/types/schemas.ts(3 hunks)
💤 Files with no reviewable changes (1)
- ui/lib/types/config.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- ui/app/providers/fragments/index.ts
- ui/app/providers/fragments/proxyFormFragment.tsx
- plugins/semanticcache/version
- plugins/logging/version
- plugins/telemetry/version
- plugins/jsonparser/version
- core/version
- framework/version
- ui/app/providers/fragments/apiStructureFormFragment.tsx
- plugins/maxim/version
- ui/app/providers/dialogs/addNewKeyDialog.tsx
- plugins/governance/version
- ui/app/config/page.tsx
- transports/version
- plugins/mocker/version
- ui/app/providers/views/modelProviderKeysTableView.tsx
- ui/app/providers/views/modelProviderConfig.tsx
- docs/features/observability.mdx
- ui/app/providers/fragments/networkFormFragment.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
ui/app/config/views/pluginsForm.tsx (1)
ui/lib/types/config.ts (1)
ModelProviderName(12-12)
ui/app/providers/fragments/allowedRequestsFields.tsx (3)
ui/app/providers/fragments/index.ts (1)
AllowedRequestsFields(1-1)ui/components/ui/form.tsx (3)
FormItem(161-161)FormLabel(162-162)FormControl(163-163)ui/components/ui/switch.tsx (1)
Switch(36-36)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
ui/lib/types/schemas.ts (1)
allowedRequestsSchema(193-202)ui/components/ui/alert.tsx (2)
Alert(41-41)AlertDescription(41-41)ui/app/providers/fragments/allowedRequestsFields.tsx (1)
AllowedRequestsFields(23-76)
⏰ 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). (22)
- 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 (7)
ui/app/providers/dialogs/addNewCustomProviderDialog.tsx (3)
83-84: Dialog onOpenChange wiring is correct.Handler now matches
(open: boolean) => voidand only closes onfalse. Good fix.
61-69: Verify: backend acceptance ofkeys: []for custom providers.Type schema for add requests generally requires ≥1 key. If the API permits keyless creation, fine; else this will 400. Consider omitting
keysor gating via a separate “add keys” flow.Would you like me to scan usages of
useCreateProviderMutationand the API handler to confirm accepted payloads?
91-96: Nice touch: conditional restart notice tied to form dirtiness.Matches PR intent and keeps noise down.
ui/app/providers/fragments/allowedRequestsFields.tsx (1)
12-21: Good: keys/labels align with schema.The 8 toggles match
allowedRequestsSchema. Nice, balanced 2‑column layout.ui/app/config/views/pluginsForm.tsx (3)
11-12: Imports LGTM; types correctly scoped to config/plugins.No issues. Using ModelProviderName for provider values is appropriate here.
124-125: Debounce ref separation is good.Clear, single-responsibility ref for cache updates; cleanup handled below.
200-209: Confirm Switch checked state reflects intended UX.
checked={isSemanticCacheEnabled && isVectorStoreEnabled}will show “off” when the plugin is enabled but vector store is disabled. If that’s intentional, ignore; otherwise bind toisSemanticCacheEnabledand rely solely ondisabledto block interaction.
TejasGhatte
changed the base branch from
09-13-enhancement_configuration_updates_in_maxim_plugin
to
graphite-base/478
Merge activity
|
Pratham-Mishra04
force-pushed
the
09-16-fix_ui_fixes_for_providers
branch
from
6a67135 to
dbf1853
Compare
## Summary Improved error handling for request timeouts across all provider implementations, ensuring consistent error messages and better user experience when API requests time out. ## Changes - Added specific error handling for timeout scenarios (context.Canceled, context.DeadlineExceeded, fasthttp.ErrTimeout) across all providers - Created a dedicated error message for timeouts that guides users to adjust the timeout setting - Fixed validation in HTTP handlers for embeddings, speech, and text completion requests - Improved CORS wildcard pattern matching to support domain patterns like `*.example.com` - Fixed issues in the logging plugin to properly handle text completion responses - Enhanced UI form handling for network configuration with proper default values ## Type of change - [x] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (Next.js) - [ ] Docs ## How to test Test timeout handling by configuring a very short timeout and making requests: ```sh # Core/Transports go test ./core/providers/... -run TestProviderTimeouts # Test CORS with wildcard domains curl -H "Origin: https://test.example.com" -v http://localhost:8000/v1/chat/completions # Test UI network config form cd ui pnpm i pnpm dev # Navigate to Providers > [Any Provider] > Network Config ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #456 - Inconsistent timeout error handling Closes #478 - CORS wildcard pattern support ## Security considerations The CORS wildcard pattern implementation has been carefully designed to prevent regex-based attacks. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
## Summary Improved error handling for request timeouts across all provider implementations, ensuring consistent error messages and better user experience when API requests time out. ## Changes - Added specific error handling for timeout scenarios (context.Canceled, context.DeadlineExceeded, fasthttp.ErrTimeout) across all providers - Created a dedicated error message for timeouts that guides users to adjust the timeout setting - Fixed validation in HTTP handlers for embeddings, speech, and text completion requests - Improved CORS wildcard pattern matching to support domain patterns like `*.example.com` - Fixed issues in the logging plugin to properly handle text completion responses - Enhanced UI form handling for network configuration with proper default values ## Type of change - [x] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (Next.js) - [ ] Docs ## How to test Test timeout handling by configuring a very short timeout and making requests: ```sh # Core/Transports go test ./core/providers/... -run TestProviderTimeouts # Test CORS with wildcard domains curl -H "Origin: https://test.example.com" -v http://localhost:8000/v1/chat/completions # Test UI network config form cd ui pnpm i pnpm dev # Navigate to Providers > [Any Provider] > Network Config ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #456 - Inconsistent timeout error handling Closes #478 - CORS wildcard pattern support ## Security considerations The CORS wildcard pattern implementation has been carefully designed to prevent regex-based attacks. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
3 participants
Summary
Enhanced the custom provider configuration UI with granular request type controls and improved form handling for provider settings.
Solves #445
Changes
AllowedRequestsFieldscomponent to configure which request types a custom provider can handleType of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No significant security implications. The changes are UI enhancements for configuration options that already exist in the backend.
Checklist
docs/contributing/README.mdand followed the guidelines