feat: improve OIDC connection behavior#2789
Open
wilsonrivera wants to merge 7 commits intomainfrom
Open
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
Contributor
WalkthroughExtracted inline OIDC provider UI from the settings page into modular components, added a shared absolute URL validator, updated a control-plane error message text, and replaced inline URL validation with the shared validator in one component. Settings now uses the new OIDCCard and manual refetching for provider data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
studio/src/lib/zod.ts (1)
8-14: Redundant empty string check.The
if (!val)block is unreachable because.min(1)on line 6 already validates that the string is non-empty beforesuperRefineruns.Proposed simplification
.superRefine((val, ctx) => { - if (!val) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: 'Must be a valid absolute URL starting with https://', - }); - return; - } - try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/lib/zod.ts` around lines 8 - 14, Remove the redundant empty-string guard inside the superRefine since .min(1) already ensures val is non-empty; delete the if (!val) { ctx.addIssue({ code: z.ZodIssueCode.custom, ... }); return; } block and let the remaining URL validation (inside the same superRefine) handle invalid formats while keeping the existing ctx.addIssue usage for non-https/absolute URL errors (referencing the superRefine callback, ctx.addIssue, and z.ZodIssueCode.custom).studio/src/components/oidc/oidc-info-dialog.tsx (2)
37-41: Dialog lacks a standard close/dismiss option.Users can only dismiss this dialog by clicking "Update Mappers," which forces them into the mappers flow. Consider adding a secondary close action or allowing the dialog to be dismissed via the standard close button for better UX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-info-dialog.tsx` around lines 37 - 41, The dialog currently only exposes a single action ("Update Mappers") in the DialogFooter and forces navigation; update the UI to provide a standard dismiss option by adding a secondary Button that calls the existing onClose handler (or enable the dialog's built-in close control) so users can close without entering mappers; adjust the DialogFooter to include both the primary action Button for "Update Mappers" and a secondary "Cancel" or "Close" Button that invokes onClose(), and ensure any modal prop that controls ESC/background click (if present) allows dismissal.
14-15: EmptyDialogTriggermay cause accessibility issues.The
DialogTriggeris empty because the dialog is externally controlled. Consider removing it entirely since theopenprop already controls visibility.Proposed fix
<Dialog open={open}> - <DialogTrigger /> <DialogContent>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-info-dialog.tsx` around lines 14 - 15, Remove the empty DialogTrigger element from the Dialog component because the dialog is externally controlled via the open prop; locate the Dialog/ DialogTrigger usage (DialogTrigger) in oidc-info-dialog.tsx and delete the self-closing <DialogTrigger /> line, then verify no other code expects a trigger—if focus management is required, handle it via the Dialog's props (e.g., initialFocus or aria attributes) rather than an empty trigger.studio/src/pages/[organizationSlug]/settings.tsx (1)
591-593: Consider enabling the query directly instead of manual refetch.The query is disabled and then manually triggered via
refetchOIDCProvider()in auseEffect. This pattern works, but you could simplify by just letting the query run when the user is available.Alternative approach
const { data: providerData, refetch: refetchOIDCProvider, isLoading: fetchingOIDCProvider, - } = useQuery(getOIDCProvider, { - enabled: false, - }); + } = useQuery(getOIDCProvider, { + enabled: !!user?.currentOrganization?.slug, + });This would eliminate the need for the manual
useEffectrefetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/`[organizationSlug]/settings.tsx around lines 591 - 593, The current useQuery call for getOIDCProvider is initialized with enabled: false and then manually triggered via refetchOIDCProvider inside a useEffect when the user is present; change this to let the query run automatically by removing enabled: false and instead set enabled: !!user (or equivalent truthy check) on the useQuery call so getOIDCProvider runs when user becomes available, and remove the corresponding useEffect that calls refetchOIDCProvider to avoid redundant manual refetching.studio/src/components/oidc/connect-oidc-provider-dialog.tsx (1)
54-56: Minor: Consider renaming the callback parameter to avoid shadowing.The parameter
datainonSuccess(data)shadows the outerdatafromhandleSubmit(data: OIDCProviderInput). While this works correctly, using a distinct name likeresponseorresultwould improve readability.💡 Suggested improvement
- onSuccess(data) { - if (data.response?.code === EnumStatusCode.OK) { + onSuccess(result) { + if (result.response?.code === EnumStatusCode.OK) { refetch().finally(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/connect-oidc-provider-dialog.tsx` around lines 54 - 56, The onSuccess callback in the mutate call is shadowing the outer handleSubmit(data: OIDCProviderInput) parameter by using the same name `data`; rename the inner parameter (e.g., to `response` or `result`) in the onSuccess signature and update all references inside that callback (e.g., change `data.response?.code` to `response.response?.code`) to avoid variable shadowing while preserving existing logic that checks EnumStatusCode.OK.studio/src/components/oidc/update-mappers-dialog.tsx (1)
194-196: Consider using a more robust ID generator.Using
Date.now().toString()alone for new mapper IDs could cause collisions if a user clicks "Add" rapidly. Consider using a counter or appending a random suffix.💡 Suggested improvement
onClick={() => { - setMappers((current) => [...current, { id: Date.now().toString(), groupId: '', ssoGroup: '' }]); + setMappers((current) => [ + ...current, + { id: `${Date.now()}-${Math.random().toString(36).slice(2, 9)}`, groupId: '', ssoGroup: '' }, + ]); }}Alternatively, use a library like
nanoidorcrypto.randomUUID()for guaranteed uniqueness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/update-mappers-dialog.tsx` around lines 194 - 196, The current onClick handler for adding a mapper uses Date.now().toString() for the id (in the setMappers([... , { id: Date.now().toString(), ... }]) call), which can collide on rapid clicks; replace that id generation with a robust unique ID strategy such as using a module-level counter, appending a random suffix, or calling a UUID generator (e.g., crypto.randomUUID() or nanoid) so each new mapper gets a guaranteed-unique id; update the onClick handler to produce the new id with the chosen method and leave the rest of the setMappers logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/bufservices/sso/deleteOIDCProvider.ts`:
- Line 51: In deleteOIDCProvider.ts update the error details string that
currently reads `details: \`Organization ${authContext.organizationSlug} doesn't
have an OIDC provider \`,` to remove the trailing space—make the details value
`Organization ${authContext.organizationSlug} doesn't have an OIDC provider` so
the error message has no trailing whitespace.
In `@studio/src/components/oidc/oidc-form.tsx`:
- Around line 83-84: The FormLabel for the clientSecret field is incorrect (it
reads "Client ID"); update the label inside the FormItem that renders the
clientSecret input in oidc-form.tsx so it correctly reads "Client Secret"
(locate the FormItem/FormLabel pair associated with the clientSecret field and
change its text).
In `@studio/src/components/oidc/update-mappers-dialog.tsx`:
- Line 164: The mappers list container in UpdateMappersDialog currently sets a
max height via the div with className "max-h-[35vh] space-y-3" but doesn't allow
scrolling, so add overflow handling (e.g., add "overflow-y-auto" or
"overflow-auto") to that div so excess mapper items become scrollable; update
the div in the UpdateMappersDialog component accordingly to include the overflow
utility class.
- Around line 59-68: The close-prevention in onOpenChangeCallback is too strict:
currently it blocks closing when mappers.length === 0 or when there's one
invalid mapper; change the logic to only block closing while an operation is
pending. Update onOpenChangeCallback to remove the mappers.length and
hasInvalidMappers checks and only return early when isPending && !open, then
call setOpen(open) and onClose() as before (references: onOpenChangeCallback,
isPending, mappers, hasInvalidMappers, setOpen, onClose).
---
Nitpick comments:
In `@studio/src/components/oidc/connect-oidc-provider-dialog.tsx`:
- Around line 54-56: The onSuccess callback in the mutate call is shadowing the
outer handleSubmit(data: OIDCProviderInput) parameter by using the same name
`data`; rename the inner parameter (e.g., to `response` or `result`) in the
onSuccess signature and update all references inside that callback (e.g., change
`data.response?.code` to `response.response?.code`) to avoid variable shadowing
while preserving existing logic that checks EnumStatusCode.OK.
In `@studio/src/components/oidc/oidc-info-dialog.tsx`:
- Around line 37-41: The dialog currently only exposes a single action ("Update
Mappers") in the DialogFooter and forces navigation; update the UI to provide a
standard dismiss option by adding a secondary Button that calls the existing
onClose handler (or enable the dialog's built-in close control) so users can
close without entering mappers; adjust the DialogFooter to include both the
primary action Button for "Update Mappers" and a secondary "Cancel" or "Close"
Button that invokes onClose(), and ensure any modal prop that controls
ESC/background click (if present) allows dismissal.
- Around line 14-15: Remove the empty DialogTrigger element from the Dialog
component because the dialog is externally controlled via the open prop; locate
the Dialog/ DialogTrigger usage (DialogTrigger) in oidc-info-dialog.tsx and
delete the self-closing <DialogTrigger /> line, then verify no other code
expects a trigger—if focus management is required, handle it via the Dialog's
props (e.g., initialFocus or aria attributes) rather than an empty trigger.
In `@studio/src/components/oidc/update-mappers-dialog.tsx`:
- Around line 194-196: The current onClick handler for adding a mapper uses
Date.now().toString() for the id (in the setMappers([... , { id:
Date.now().toString(), ... }]) call), which can collide on rapid clicks; replace
that id generation with a robust unique ID strategy such as using a module-level
counter, appending a random suffix, or calling a UUID generator (e.g.,
crypto.randomUUID() or nanoid) so each new mapper gets a guaranteed-unique id;
update the onClick handler to produce the new id with the chosen method and
leave the rest of the setMappers logic intact.
In `@studio/src/lib/zod.ts`:
- Around line 8-14: Remove the redundant empty-string guard inside the
superRefine since .min(1) already ensures val is non-empty; delete the if (!val)
{ ctx.addIssue({ code: z.ZodIssueCode.custom, ... }); return; } block and let
the remaining URL validation (inside the same superRefine) handle invalid
formats while keeping the existing ctx.addIssue usage for non-https/absolute URL
errors (referencing the superRefine callback, ctx.addIssue, and
z.ZodIssueCode.custom).
In `@studio/src/pages/`[organizationSlug]/settings.tsx:
- Around line 591-593: The current useQuery call for getOIDCProvider is
initialized with enabled: false and then manually triggered via
refetchOIDCProvider inside a useEffect when the user is present; change this to
let the query run automatically by removing enabled: false and instead set
enabled: !!user (or equivalent truthy check) on the useQuery call so
getOIDCProvider runs when user becomes available, and remove the corresponding
useEffect that calls refetchOIDCProvider to avoid redundant manual refetching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb38ef30-f635-4abd-af18-5c586880958e
📒 Files selected for processing (11)
controlplane/src/core/bufservices/sso/deleteOIDCProvider.tsstudio/src/components/check-extensions/check-extensions-config.tsxstudio/src/components/oidc/connect-oidc-provider-dialog.tsxstudio/src/components/oidc/disconnect-oidc-provider-dialog.tsxstudio/src/components/oidc/oidc-card.tsxstudio/src/components/oidc/oidc-form.tsxstudio/src/components/oidc/oidc-group-mapper.tsxstudio/src/components/oidc/oidc-info-dialog.tsxstudio/src/components/oidc/update-mappers-dialog.tsxstudio/src/lib/zod.tsstudio/src/pages/[organizationSlug]/settings.tsx
| response: { | ||
| code: EnumStatusCode.ERR_NOT_FOUND, | ||
| details: `Organization ${authContext.organizationSlug} doesn't have an oidc identity provider `, | ||
| details: `Organization ${authContext.organizationSlug} doesn't have an OIDC provider `, |
Contributor
There was a problem hiding this comment.
Trailing space in error message.
There's a trailing space at the end of the error details string.
Proposed fix
- details: `Organization ${authContext.organizationSlug} doesn't have an OIDC provider `,
+ details: `Organization ${authContext.organizationSlug} doesn't have an OIDC provider`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| details: `Organization ${authContext.organizationSlug} doesn't have an OIDC provider `, | |
| details: `Organization ${authContext.organizationSlug} doesn't have an OIDC provider`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controlplane/src/core/bufservices/sso/deleteOIDCProvider.ts` at line 51, In
deleteOIDCProvider.ts update the error details string that currently reads
`details: \`Organization ${authContext.organizationSlug} doesn't have an OIDC
provider \`,` to remove the trailing space—make the details value `Organization
${authContext.organizationSlug} doesn't have an OIDC provider` so the error
message has no trailing whitespace.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/pages/`[organizationSlug]/settings.tsx:
- Around line 589-591: The OIDCCard is rendering with undefined/stale data
because useQuery(getOIDCProvider, { enabled: false }) prevents the initial fetch
and the component only checks fetchingOIDCProvider; add a per-organization
"first-loaded" flag and gate rendering until the first fetch for the current org
finishes: keep useQuery with enabled: false and refetchOIDCProvider as-is, add a
state like hasLoadedOIDCForOrg (keyed by organizationSlug or reset on org
change), kick off refetchOIDCProvider() in a useEffect when the org
mounts/changes, set hasLoadedOIDCForOrg to true when that refetch resolves (or
errors), and update the OIDCCard render condition to require hasLoadedOIDCForOrg
&& !fetchingOIDCProvider so the card only renders after the first fetch for that
org completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d83bb24a-44e8-4211-b8a1-855ce50365e8
📒 Files selected for processing (1)
studio/src/pages/[organizationSlug]/settings.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
studio/src/components/oidc/update-mappers-dialog.tsx (2)
164-164: Preferoverflow-y-autooveroverflow-scroll.A past review suggested adding overflow handling, which was marked as addressed. However,
overflow-scrollforces scrollbars to always appear regardless of content size, whileoverflow-y-autoonly shows scrollbars when content actually overflows the container. This provides a cleaner UI experience.🔧 Suggested change
- <div className="max-h-[35vh] space-y-3 overflow-scroll"> + <div className="max-h-[35vh] space-y-3 overflow-y-auto">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/update-mappers-dialog.tsx` at line 164, In update-mappers-dialog.tsx, update the scrolling container inside the dialog (the div with class "max-h-[35vh] space-y-3 overflow-scroll") to use "overflow-y-auto" instead of "overflow-scroll" so scrollbars only appear when content exceeds the max height; locate the div in the UpdateMappersDialog component and replace the overflow utility accordingly.
77-87: Potential stale state whencurrentMappersreference changes while dialog is open.The
useEffectreinitializesmapperswhenevercurrentMapperschanges, even when the dialog is already open and the user might be editing. If the parent component causescurrentMappersto get a new reference (e.g., from a refetch), any unsaved user edits would be lost.Consider removing
currentMappersfrom the dependency array and only initializing whenopentransitions fromfalsetotrue:🔧 Suggested approach using a ref to track previous open state
+import { useEffect, useRef, useState } from 'react'; -import { useEffect, useState } from 'react'; // Inside component: + const prevOpenRef = useRef(false); + useEffect(() => { - if (open) { + if (open && !prevOpenRef.current) { setPending(false); setMappers( currentMappers.map((mapper, index) => ({ id: `${Date.now().toString()}-${index}`, ...mapper, })), ); } - }, [open, currentMappers]); + prevOpenRef.current = open; + }, [open, currentMappers]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/update-mappers-dialog.tsx` around lines 77 - 87, The effect that initializes mappers currently depends on both open and currentMappers, so if currentMappers gets a new reference while the dialog is open user edits are clobbered; change the logic in the useEffect that sets mappers (the block using setMappers and setPending) so it only runs when open transitions from false to true (detect the transition with a useRef previousOpen or similar) and remove currentMappers from the dependency list, ensuring you still read the latest currentMappers when opening but avoid reinitializing mid-edit.studio/src/components/oidc/oidc-form.tsx (2)
8-8: Rename schema constant to UPPER_SNAKE_CASE for repo guideline compliance.
OIDCProviderInputSchemais a constant and should follow the configured constant naming rule.Proposed fix
-const OIDCProviderInputSchema = z.object({ +const OIDC_PROVIDER_INPUT_SCHEMA = z.object({ name: z.string().min(1), discoveryEndpoint: absoluteUrlValidator, clientID: z.string().min(1), clientSecret: z.string().min(1), }); @@ -export type OIDCProviderInput = z.infer<typeof OIDCProviderInputSchema>; +export type OIDCProviderInput = z.infer<typeof OIDC_PROVIDER_INPUT_SCHEMA>; @@ const form = useZodForm<OIDCProviderInput>({ - schema: OIDCProviderInputSchema, + schema: OIDC_PROVIDER_INPUT_SCHEMA, mode: 'onChange', });As per coding guidelines:
Use UPPER_SNAKE_CASE for constants.Also applies to: 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-form.tsx` at line 8, Rename the constant OIDCProviderInputSchema to follow UPPER_SNAKE_CASE (e.g., OIDC_PROVIDER_INPUT_SCHEMA) throughout the file and any other occurrences; update its declaration (z.object(...) bound to OIDC_PROVIDER_INPUT_SCHEMA), all internal usages in oidc-form.tsx (including the second occurrence referenced), and any exports/imports that reference OIDCProviderInputSchema so identifiers remain consistent.
23-23: Add explicit return type annotation on the component.The component currently relies on inference for return type; this conflicts with the TypeScript rule configured for explicit function return types.
Proposed fix
+import type { ReactElement } from 'react'; import { z } from 'zod'; @@ -export function OIDCForm({ isPending, handleSubmit, onCancel }: OIDCFormProps) { +export function OIDCForm({ isPending, handleSubmit, onCancel }: OIDCFormProps): ReactElement {As per coding guidelines:
Use explicit type annotations for function parameters and return types in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-form.tsx` at line 23, The OIDCForm component lacks an explicit return type; update the function signature for OIDCForm to include a JSX return type (e.g., : JSX.Element or : React.ReactElement) so it satisfies the TypeScript explicit function return type rule, leaving the existing OIDCFormProps parameter types unchanged and ensuring the function still returns the same JSX element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@studio/src/components/oidc/oidc-form.tsx`:
- Line 56: The OIDC discovery placeholder string in oidc-form.tsx is incorrect
(it uses ".wellknown"); update the placeholder value used in the form input (the
placeholder prop shown on line with
placeholder="https://hostname/auth/realms/master/.wellknown/openid-configuration")
to the correct path by replacing ".wellknown" with ".well-known" so it reads
".../auth/realms/master/.well-known/openid-configuration".
---
Nitpick comments:
In `@studio/src/components/oidc/oidc-form.tsx`:
- Line 8: Rename the constant OIDCProviderInputSchema to follow UPPER_SNAKE_CASE
(e.g., OIDC_PROVIDER_INPUT_SCHEMA) throughout the file and any other
occurrences; update its declaration (z.object(...) bound to
OIDC_PROVIDER_INPUT_SCHEMA), all internal usages in oidc-form.tsx (including the
second occurrence referenced), and any exports/imports that reference
OIDCProviderInputSchema so identifiers remain consistent.
- Line 23: The OIDCForm component lacks an explicit return type; update the
function signature for OIDCForm to include a JSX return type (e.g., :
JSX.Element or : React.ReactElement) so it satisfies the TypeScript explicit
function return type rule, leaving the existing OIDCFormProps parameter types
unchanged and ensuring the function still returns the same JSX element.
In `@studio/src/components/oidc/update-mappers-dialog.tsx`:
- Line 164: In update-mappers-dialog.tsx, update the scrolling container inside
the dialog (the div with class "max-h-[35vh] space-y-3 overflow-scroll") to use
"overflow-y-auto" instead of "overflow-scroll" so scrollbars only appear when
content exceeds the max height; locate the div in the UpdateMappersDialog
component and replace the overflow utility accordingly.
- Around line 77-87: The effect that initializes mappers currently depends on
both open and currentMappers, so if currentMappers gets a new reference while
the dialog is open user edits are clobbered; change the logic in the useEffect
that sets mappers (the block using setMappers and setPending) so it only runs
when open transitions from false to true (detect the transition with a useRef
previousOpen or similar) and remove currentMappers from the dependency list,
ensuring you still read the latest currentMappers when opening but avoid
reinitializing mid-edit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4975e284-1953-4c20-865e-f80a18c1a258
📒 Files selected for processing (2)
studio/src/components/oidc/oidc-form.tsxstudio/src/components/oidc/update-mappers-dialog.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
studio/src/components/oidc/oidc-form.tsx (3)
23-23: Add explicit return type forOIDCForm.The component function is missing an explicit return type annotation.
Proposed fix
-export function OIDCForm({ isPending, handleSubmit, onCancel }: OIDCFormProps) { +export function OIDCForm({ isPending, handleSubmit, onCancel }: OIDCFormProps): JSX.Element {As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-form.tsx` at line 23, The OIDCForm component lacks an explicit return type; update the function signature for OIDCForm to declare its return type (e.g., JSX.Element or React.ReactElement) while keeping the existing parameter type OIDCFormProps for isPending, handleSubmit, and onCancel so the signature reads OIDCForm(props: OIDCFormProps): JSX.Element (or React.ReactElement), ensuring the exported function has an explicit return annotation.
8-8: Rename schema constant to UPPER_SNAKE_CASE.
OIDCProviderInputSchemais declared as a constant but does not follow the repository naming rule for constants.Proposed fix
-const OIDCProviderInputSchema = z.object({ +const OIDC_PROVIDER_INPUT_SCHEMA = z.object({ name: z.string().min(1), discoveryEndpoint: absoluteUrlValidator, clientID: z.string().min(1), clientSecret: z.string().min(1), }); -export type OIDCProviderInput = z.infer<typeof OIDCProviderInputSchema>; +export type OIDCProviderInput = z.infer<typeof OIDC_PROVIDER_INPUT_SCHEMA>; @@ const form = useZodForm<OIDCProviderInput>({ - schema: OIDCProviderInputSchema, + schema: OIDC_PROVIDER_INPUT_SCHEMA, mode: 'onChange', });As per coding guidelines: "Use UPPER_SNAKE_CASE for constants".
Also applies to: 15-15, 25-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-form.tsx` at line 8, The schema constant OIDCProviderInputSchema should be renamed to follow UPPER_SNAKE_CASE: change its declaration to OIDC_PROVIDER_INPUT_SCHEMA and update every usage/export/import that references OIDCProviderInputSchema (including the other occurrences noted around the same file). Ensure the z.object(...) assignment, any exported name, and all places that consume this schema (validation calls, type inferences, or props) are updated to the new identifier to avoid build errors.
9-13: Harden required-field validation against whitespace-only values.
.min(1)allows" "inputs; trim before length checks forname,clientID, andclientSecret.Proposed fix
const OIDC_PROVIDER_INPUT_SCHEMA = z.object({ - name: z.string().min(1), + name: z.string().trim().min(1), discoveryEndpoint: absoluteUrlValidator, - clientID: z.string().min(1), - clientSecret: z.string().min(1), + clientID: z.string().trim().min(1), + clientSecret: z.string().trim().min(1), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/oidc/oidc-form.tsx` around lines 9 - 13, The current zod schema uses .min(1) for the fields name, clientID, and clientSecret which permits whitespace-only values; update those fields (name, clientID, clientSecret) to trim before length checks (e.g., use z.string().trim().min(1) or an equivalent refine) so inputs like " " are rejected, keeping discoveryEndpoint as absoluteUrlValidator unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@studio/src/components/oidc/oidc-form.tsx`:
- Line 23: The OIDCForm component lacks an explicit return type; update the
function signature for OIDCForm to declare its return type (e.g., JSX.Element or
React.ReactElement) while keeping the existing parameter type OIDCFormProps for
isPending, handleSubmit, and onCancel so the signature reads OIDCForm(props:
OIDCFormProps): JSX.Element (or React.ReactElement), ensuring the exported
function has an explicit return annotation.
- Line 8: The schema constant OIDCProviderInputSchema should be renamed to
follow UPPER_SNAKE_CASE: change its declaration to OIDC_PROVIDER_INPUT_SCHEMA
and update every usage/export/import that references OIDCProviderInputSchema
(including the other occurrences noted around the same file). Ensure the
z.object(...) assignment, any exported name, and all places that consume this
schema (validation calls, type inferences, or props) are updated to the new
identifier to avoid build errors.
- Around line 9-13: The current zod schema uses .min(1) for the fields name,
clientID, and clientSecret which permits whitespace-only values; update those
fields (name, clientID, clientSecret) to trim before length checks (e.g., use
z.string().trim().min(1) or an equivalent refine) so inputs like " " are
rejected, keeping discoveryEndpoint as absoluteUrlValidator unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7a4d048-1d46-47a0-99d6-79d9af6367bf
📒 Files selected for processing (1)
studio/src/components/oidc/oidc-form.tsx
…ng-and-updating-oidc-connections
…ng-and-updating-oidc-connections
…ng-and-updating-oidc-connections
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-04-22.at.2.14.27.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
The goal of this PR is to improve how users connect OIDC providers to their organization with better error handling. The code have also been moved to dedicated componets for a cleaner structure.
These changes will allow us further evolve how users manage their OIDC integration in the future.