Configure Identity Providers in the UI #523
Conversation
📝 WalkthroughWalkthroughAdds embedded Identity Provider management (types, icons, list/modal CRUD and IdP badges), invite-success modal with copyable password, an instance-setup wizard and provider with unauthenticated API, password visibility toggle, updated redirect/retry logic, and related layout/auth integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User as End User
participant UI as Browser UI
participant API as Server API
participant Cache as SWR Cache
participant Notif as Notifications
rect rgb(236,247,255)
Note left of UI: Identity Provider create/edit flow
User->>UI: Open Settings → Identity Providers tab
UI->>API: GET /identity-providers
API-->>UI: 200 OK (list)
UI->>User: Render list and "Add Identity Provider"
User->>UI: Open modal, fill form, submit
UI->>API: POST/PUT /identity-providers
API-->>UI: 200 OK (resource + redirect_url)
UI->>Cache: mutate('/identity-providers')
UI->>Notif: show success toast
UI->>User: Show redirect URL success modal (copyable)
end
sequenceDiagram
participant Browser as Browser UI
participant InstanceCtx as InstanceSetupProvider
participant API as Server API
participant Router as Router
rect rgb(250,240,245)
Note left of InstanceCtx: Instance setup check on app load
Browser->>InstanceCtx: mount → evaluate bypass/routes
alt not bypass
InstanceCtx->>API: GET /instance (unauthenticated)
API-->>InstanceCtx: 200 OK (setup_required: boolean)
alt setup_required = true and not on /setup
InstanceCtx->>Router: redirect to /setup (replace)
else
InstanceCtx-->>Browser: allow normal render
end
else bypass
InstanceCtx-->>Browser: skip check
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/modules/users/UsersTable.tsx (1)
18-18: Unused import:isLocalDev.The
isLocalDevimport appears unused after removing the environment gating. Consider removing it to keep the imports clean.🔎 Proposed fix
-import { isLocalDev, isNetBirdHosted } from "@utils/netbird"; +import { isNetBirdHosted } from "@utils/netbird";src/modules/users/table-cells/UserNameCell.tsx (1)
17-79: Consider extracting shared IdP icons to a common module.The same SVG icon components (
GoogleIcon,MicrosoftIcon,EntraIcon, etc.) and theidpIconsmapping are duplicated across three files:
UserNameCell.tsxIdentityProviderModal.tsxIdentityProvidersTab.tsxExtracting these to a shared module (e.g.,
@/components/icons/IdentityProviderIcons.tsx) would improve maintainability and ensure consistency if icons need updates.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/app/(dashboard)/settings/page.tsxsrc/interfaces/IdentityProvider.tssrc/interfaces/User.tssrc/modules/settings/IdentityProviderModal.tsxsrc/modules/settings/IdentityProvidersTab.tsxsrc/modules/users/UserInviteModal.tsxsrc/modules/users/UsersTable.tsxsrc/modules/users/table-cells/UserNameCell.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
src/modules/settings/IdentityProvidersTab.tsx (7)
src/interfaces/IdentityProvider.ts (2)
IdentityProviderType(34-41)IdentityProvider(43-50)src/contexts/DialogProvider.tsx (1)
useDialog(132-132)src/utils/api.tsx (2)
useApiCall(169-212)useFetchApi(120-167)src/contexts/PermissionsProvider.tsx (1)
usePermissions(39-39)src/components/DropdownMenu.tsx (4)
DropdownMenu(207-207)DropdownMenuTrigger(221-221)DropdownMenuContent(209-209)DropdownMenuItem(211-211)src/hooks/useLocalStorage.tsx (1)
useLocalStorage(20-124)src/modules/settings/IdentityProviderModal.tsx (1)
IdentityProviderModal(117-391)
src/modules/users/UserInviteModal.tsx (3)
src/interfaces/Group.ts (1)
Group(1-12)src/interfaces/User.ts (1)
User(3-18)src/utils/netbird.ts (1)
isNetBirdHosted(18-23)
src/modules/users/UsersTable.tsx (2)
src/modules/users/UserInviteModal.tsx (1)
UserInviteModal(40-121)src/utils/netbird.ts (1)
isNetBirdHosted(18-23)
src/modules/users/table-cells/UserNameCell.tsx (4)
src/interfaces/IdentityProvider.ts (2)
IdentityProviderType(34-41)IdentityProvider(43-50)src/interfaces/User.ts (1)
User(3-18)src/utils/api.tsx (1)
useFetchApi(120-167)src/components/Tooltip.tsx (4)
TooltipProvider(68-68)Tooltip(68-68)TooltipTrigger(68-68)TooltipContent(68-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (18)
src/interfaces/User.ts (1)
16-17: LGTM with a security note.The new optional fields align well with the identity provider integration. The
passwordfield is appropriately optional for the create-user flow.Consider ensuring that the
passwordfield is never persisted to localStorage, logged, or included in error reports. If using serialization helpers or debugging tools, verify they exclude this field.src/modules/users/UsersTable.tsx (1)
280-291: LGTM!The dynamic button label appropriately reflects the different user flows: invitation-based for NetBird-hosted environments and password-based creation for self-hosted deployments.
src/app/(dashboard)/settings/page.tsx (2)
58-61: LGTM!The new Identity Providers tab trigger is correctly placed within the
permission.settings.readconditional block, consistent with other settings tabs.
89-89: Verify ifIdentityProvidersTabshould be guarded byaccount.Unlike other tabs (e.g.,
AuthenticationTab,PermissionsTab),IdentityProvidersTabis rendered unconditionally without theaccount &&guard. If the component relies on account data or should only render when account is available, consider adding the guard for consistency:-<IdentityProvidersTab /> +{account && <IdentityProvidersTab />}If the tab intentionally doesn't depend on account data, this is fine as-is.
src/modules/users/table-cells/UserNameCell.tsx (2)
85-92: LGTM!The
useFetchApihook benefits from SWR's caching, so multipleUserNameCellinstances will share the same cached response rather than making redundant API calls. TheuseMemocorrectly derives the user's identity provider with appropriate null checks.
142-155: LGTM!The IdP badge rendering logic correctly:
- Only displays when
userIdpexists and user status is not "invited" or "blocked"- Uses proper tooltip implementation with accessible trigger
- Falls back gracefully via the
idpIconsmappingsrc/modules/settings/IdentityProviderModal.tsx (4)
179-186: Consider omittingissuerfor providers that don't require it.The payload always includes
issuer: trim(issuer), but formicrosoftproviders, the issuer field is hidden and will be an empty string. If the API expects the issuer to be omitted (rather than empty) for these providers, consider conditionally including it:const payload: IdentityProviderRequest = { type, name: trim(name), - issuer: trim(issuer), + ...(requiresIssuer && { issuer: trim(issuer) }), client_id: trim(clientId), client_secret: trim(clientSecret), };If the API handles empty strings correctly, this is fine as-is.
140-154: LGTM!The
useEffectcorrectly resets form state when the modal opens or the provider changes. SettingclientSecretto empty on edit is appropriate since secrets shouldn't be pre-filled for security reasons.
158-170: LGTM!The validation logic is sound:
- Name is always required
- Issuer is required only when
requiresIssueris true- Client ID is always required
- Client secret is required only for new providers (not edits)
343-388: LGTM!The success modal correctly prevents dismissal via escape key or clicking outside, ensuring users don't accidentally close it before copying the redirect URL. The
handleCopyAndClosefunction ensures the URL is copied before closing.src/interfaces/IdentityProvider.ts (1)
34-58: LGTM!Well-designed type definitions with proper separation of concerns:
IdentityProviderTypeprovides type-safe provider selectionIdentityProvider(response) correctly excludesclient_secretand includesredirect_urlIdentityProviderRequest(request) correctly includesclient_secretand omits server-generated fieldssrc/modules/settings/IdentityProvidersTab.tsx (3)
119-177: LGTM!The
ActionCellcomponent correctly:
- Confirms deletion with a danger-typed dialog
- Uses
notifyto handle async feedback with loading state- Disables actions based on
permission.settings.update- Triggers SWR mutation after successful deletion
210-243: LGTM!The column definitions are well-structured:
- Name column includes provider icon for visual identification
- Type column displays human-readable labels via
idpTypeLabels- Falls back gracefully to raw type value or default icon for unknown types
278-340: LGTM!The
DataTableintegration is comprehensive:
- Row click triggers edit modal
- GetStarted card provides clear onboarding
- Add button visibility and disabled state are properly controlled
- Refresh and rows-per-page controls are correctly wired
src/modules/users/UserInviteModal.tsx (4)
47-64: LGTM!The success handling flow is well-designed:
- Password-based creation shows the success modal for secure credential handoff
- Non-password flow (hosted) closes the modal directly
- Delayed mutation ensures the user list updates after the UI transitions
73-118: LGTM!The success modal implementation correctly:
- Blocks escape key and click-outside to prevent accidental dismissal before copying
- Syncs its open state with the parent modal
- Provides clear messaging about password visibility being one-time only
- Uses the
Codecomponent for consistent password display with copy functionality
193-197: LGTM!The dynamic text appropriately differentiates between hosted (invitation-based) and self-hosted (password-based) user creation flows, providing clear context to administrators.
123-126: No action needed.UserInviteModalContentis not consumed by external components—it's used only internally withinUserInviteModal. The callback signature change is an internal implementation detail and does not affect any external consumers.
# Conflicts: # src/interfaces/Account.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/modules/users/UserInviteModal.tsx (1)
163-163: Remove redundant check.The
onSuccess &&check is unnecessary sinceonSuccessis a required parameter in theModalPropsinterface (line 128). The callback is guaranteed to be defined.🔎 Suggested simplification
- onSuccess && onSuccess(user); + onSuccess(user);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(dashboard)/settings/page.tsxsrc/interfaces/Account.tssrc/modules/users/UserInviteModal.tsxsrc/modules/users/UsersTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/(dashboard)/settings/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/users/UsersTable.tsx (3)
src/modules/account/useAccount.tsx (1)
useAccount(6-21)src/utils/netbird.ts (1)
isNetBirdHosted(18-23)src/modules/users/UserInviteModal.tsx (1)
UserInviteModal(40-125)
src/modules/users/UserInviteModal.tsx (2)
src/interfaces/User.ts (1)
User(3-18)src/utils/netbird.ts (1)
isNetBirdHosted(18-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (11)
src/interfaces/Account.ts (1)
25-25: LGTM!The optional
embedded_idp_enabledfield is a clean interface extension that enables feature-flagging for identity provider functionality without breaking existing code.src/modules/users/UserInviteModal.tsx (7)
2-2: LGTM!The new imports (
Code,CopyIcon,useCopyToClipboard,isNetBirdHosted) and thecopyMessageconstant are appropriately added to support the password display and copy functionality.Also applies to: 18-18, 22-22, 31-31, 38-38
47-57: LGTM!The
handleOnSuccesslogic correctly handles both scenarios:
- Self-hosted (with password): displays success modal
- Cloud/hosted (no password): closes modal immediately
The 1-second timeout for mutation allows proper modal close animation.
59-65: LGTM!The
handleCopyAndClosefunction properly sequences clipboard copy, state cleanup, and modal closure.
74-122: LGTM - Intentional UX constraint for security.The success modal prevents closing via ESC, outside clicks, and hides the close button, forcing users to use "Copy & Close". This ensures the password is copied before the modal can be dismissed, which is appropriate given the security-sensitive nature of password display and the warning that it won't be shown again.
197-201: Verify backend behavior aligns with conditional text.The conditional text distinguishes between "Invite User" (cloud) and "Create User" (self-hosted). Ensure the backend consistently returns a
passwordfield only for self-hosted instances, since the success modal flow depends on this distinction.
128-128: No action needed - the breaking signature change does not affect external consumers.
UserInviteModalContentis exported but has no external consumers in the codebase. The only usage is internal withinUserInviteModal(the wrapper component at line 71), which already passeshandleOnSuccesswith the correct signature(user: User) => void. External consumers import only the default exportUserInviteModal, which remains unchanged.
45-45: No changes needed. TheuseCopyToClipboardhook properly handles dynamic password values through React's re-render mechanism. WhencreatedUserstate updates, the component re-renders and the hook is re-invoked with the new password value, creating a new closure that captures the updated value. The returnedcopyToClipboardfunction will read the current password from the closure when called.Likely an incorrect or invalid review comment.
src/modules/users/UsersTable.tsx (3)
18-18: LGTM!The new imports (
isNetBirdHosted,useAccount) support the enhanced conditional rendering logic for the InviteUserButton.Also applies to: 38-38
278-287: LGTM - Conditional rendering logic is correct.The button visibility logic correctly handles both scenarios:
- Cloud (
isNetBirdHosted()): always shows "Invite User"- Self-hosted: shows "Create User" only when
embedded_idp_enabledis trueNote: On self-hosted instances, the button will be hidden while
accountis loading (whenembeddedIdpEnabledisundefined). This may cause a brief flash but is acceptable behavior.
290-299: LGTM!The conditional button text ("Invite User" for cloud, "Create User" for self-hosted) is consistent with the terminology used in
UserInviteModaland accurately reflects the different workflows.
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
Co-authored-by: Eduard Gert <kontakt@eduardgert.de>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/users/UserInviteModal.tsx (1)
161-164: Optional: Remove redundant null check.Since
onSuccessis a required prop (line 128), the null check on line 163 is unnecessary.🔎 Suggested simplification
.then((user) => { mutate("/users?service_user=false"); - onSuccess && onSuccess(user); + onSuccess(user); }),src/modules/users/table-cells/UserNameCell.tsx (1)
18-70: Consider extracting icon components to a separate file.The inline SVG icon components are functional but add significant length to this file. Consider extracting them to a dedicated icons module (e.g.,
IdpIcons.tsx) to improve maintainability and enable potential reuse across the application.💡 Suggested structure
Create a new file
src/components/icons/IdpIcons.tsx:export const GoogleIcon = () => ( <svg width="14" height="14" viewBox="0 0 48 48"> {/* SVG paths */} </svg> ); export const MicrosoftIcon = () => ( {/* ... */} ); // Export all icons export const idpIcons: Record<IdentityProviderType, React.ReactNode> = { google: <GoogleIcon />, microsoft: <MicrosoftIcon />, // ... };Then import in this file:
import { idpIcons } from "@components/icons/IdpIcons";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/users/UserInviteModal.tsxsrc/modules/users/table-cells/UserNameCell.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/users/table-cells/UserNameCell.tsx (5)
src/interfaces/IdentityProvider.ts (2)
IdentityProviderType(34-41)IdentityProvider(43-50)src/interfaces/User.ts (1)
User(3-18)src/modules/account/useAccount.tsx (1)
useAccount(6-21)src/utils/api.tsx (1)
useFetchApi(120-167)src/components/Tooltip.tsx (4)
TooltipProvider(68-68)Tooltip(68-68)TooltipTrigger(68-68)TooltipContent(68-68)
src/modules/users/UserInviteModal.tsx (5)
src/interfaces/Group.ts (1)
Group(1-12)src/interfaces/User.ts (1)
User(3-18)src/components/modal/Modal.tsx (4)
Modal(209-209)ModalTrigger(217-217)ModalContent(211-211)ModalFooter(213-213)src/utils/netbird.ts (1)
isNetBirdHosted(18-23)src/utils/helpers.ts (1)
cn(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (9)
src/modules/users/UserInviteModal.tsx (5)
2-2: LGTM! Clean import additions.The new imports (Code, CopyIcon, useCopyToClipboard, isNetBirdHosted) are all properly utilized in the implementation.
Also applies to: 18-18, 22-22, 31-31
42-65: Well-structured dual-modal flow for password handling.The state management correctly handles both scenarios:
- Users with generated passwords display the success modal
- Regular invites (no password) close immediately
The 1-second setTimeout for cache invalidation (lines 54-56) is a reasonable approach to avoid race conditions with the API response.
67-124: Secure password display modal with appropriate constraints.The success modal implementation follows good security practices:
- Prevents accidental closure via ESC key or outside clicks (lines 85-87)
- Hides the close button (line 90)
- Forces user to acknowledge by copying the password
- Clear warning that password won't be shown again (lines 99-100)
The modal state synchronization (lines 76-82) correctly ensures both modals close together.
128-128: Breaking change: onSuccess now requires User parameter.The callback signature changed from
() => voidto(user: User) => void. Ensure all call sites ofUserInviteModalContentare updated accordingly (though this appears to only be used within this file's main component).
196-201: Clear conditional UI text for hosted vs self-hosted scenarios.The UI appropriately distinguishes between:
- Hosted (NetBird Cloud): "Invite User" with email invitation flow
- Self-hosted: "Create User" with password generation
The conditional rendering using
isNetBirdHosted()is clear and maintainable.Also applies to: 257-258
src/modules/users/table-cells/UserNameCell.tsx (4)
1-16: LGTM! All necessary imports added.The imports are well-organized and include all the required dependencies for the identity provider integration feature.
72-80: LGTM! Complete type-safe mapping.The
idpIconsmapping covers all identity provider types defined inIdentityProviderType, ensuring type-safe icon rendering throughout the component.
93-96: LGTM! Well-optimized with proper null handling.The
useMemohook correctly memoizes the user's identity provider lookup with appropriate dependencies. The graceful null handling ensures the UI degrades properly when an IdP is not found.
146-159: LGTM! Correct conditional rendering with good UX.The IdP badge rendering logic properly handles all edge cases:
- Gracefully degrades when IdP data is unavailable
- Correctly prioritizes status badges for invited/blocked users
- Provides accessible tooltips with IdP names
- Maintains visual consistency with existing status badge patterns
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/modules/settings/IdentityProvidersTab.tsx (1)
313-327: Consider simplifying the rightSide render function.The current implementation is correct, but you could slightly simplify by moving the conditional logic:
🔎 Optional refactor
-rightSide={() => ( - <> - {providers && providers.length > 0 && ( - <Button - variant={"primary"} - className={"ml-auto"} - onClick={() => setModalOpen(true)} - disabled={!permission.settings.update} - > - <PlusCircle size={16} /> - Add Identity Provider - </Button> - )} - </> -)} +rightSide={() => + providers && providers.length > 0 ? ( + <Button + variant={"primary"} + className={"ml-auto"} + onClick={() => setModalOpen(true)} + disabled={!permission.settings.update} + > + <PlusCircle size={16} /> + Add Identity Provider + </Button> + ) : null +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/(dashboard)/settings/page.tsxsrc/modules/settings/IdentityProviderModal.tsxsrc/modules/settings/IdentityProvidersTab.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/(dashboard)/settings/page.tsx
- src/modules/settings/IdentityProviderModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (7)
src/modules/settings/IdentityProvidersTab.tsx (7)
1-92: LGTM! Clean imports and icon definitions.The imports are well-organized and the inline SVG icon components are appropriate for this use case.
94-112: LGTM! Exported records enable reuse across components.The exported
idpIconsandidpTypeLabelsrecords provide centralized mappings that can be reused in IdentityProviderModal.tsx and UsersNameCell, as suggested in previous review.
119-177: LGTM! Well-implemented action cell with proper safeguards.The component correctly implements:
- Delete confirmation dialog to prevent accidental deletions
- Permission checks for both edit and delete actions
- Cache invalidation via SWR mutation after successful delete
- User feedback through toast notifications
185-208: LGTM! Clean modal state management.The edit and close handlers properly manage modal state, and the
editProviderstate is correctly cleared when closing.
272-277: LGTM! Modal key pattern correctly implemented.The key prop pattern (line 274) ensures the modal component remounts on open/close, resetting internal state for create/edit flows, as suggested in the previous review.
210-243: LGTM! Solid column definitions with defensive fallbacks.The column definitions are well-structured. The fallback icon (line 219) and label (line 231) provide defensive handling for unexpected provider types, which is good practice even though the TypeScript types should prevent this at compile time.
279-341: LGTM! Well-integrated DataTable with good UX patterns.The implementation includes:
- Appropriate empty state with GetStartedCard
- Row click for quick edit access
- Conditional Add button rendering to avoid duplicates
- Proper permission checks on action buttons
- SWR cache invalidation on refresh
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/modules/instance-setup/InstanceSetupWizard.tsx (1)
89-92: Potential memory leak:setTimeoutnot cleared on unmount.If the component unmounts before the 2-second redirect completes, the timeout will still fire and attempt to navigate. This issue was previously identified and remains unresolved.
🧹 Nitpick comments (2)
src/modules/instance-setup/InstanceSetupWizard.tsx (2)
28-28: Consider using a validation library for email.The regex is adequate for basic email validation but may not cover all edge cases defined in RFC 5322. For production use, consider a well-tested library like
validator.jsoremail-validatorfor more comprehensive validation.
50-54: Consider adding password complexity validation.Currently only enforces minimum 8 characters. For better security posture, consider validating password complexity (e.g., requiring a mix of uppercase, lowercase, numbers, and special characters).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Input.tsxsrc/modules/instance-setup/InstanceSetupWizard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Input.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/instance-setup/InstanceSetupWizard.tsx (8)
src/interfaces/Instance.ts (2)
SetupRequest(5-9)ApiError(16-19)src/utils/unauthenticatedApi.ts (1)
submitSetup(52-54)src/components/NetBirdLogo.tsx (1)
NetBirdLogo(23-42)src/components/Label.tsx (1)
Label(25-25)src/components/Input.tsx (1)
Input(182-182)src/components/HelpText.tsx (1)
HelpText(9-25)src/utils/helpers.ts (1)
cn(6-8)src/components/ui/GradientFadedBackground.tsx (1)
GradientFadedBackground(8-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (5)
src/modules/instance-setup/InstanceSetupWizard.tsx (5)
110-126: LGTM: Proper cleanup in countdown effect.The countdown effect correctly clears the interval both when the countdown completes and when the component unmounts. Good implementation.
128-134: LGTM: Clean input change handler.The curried function pattern is clean, and clearing field-specific errors as users type provides good UX.
136-170: LGTM: Clean success UI.The success state provides clear feedback with a countdown and manual redirect option. Implementation is consistent with the rest of the component.
172-264: LGTM: Well-structured accessible form.The form implementation is clean with proper accessibility features:
- Proper label/input associations
- AutoFocus on the first field
- Password visibility toggle for better UX
- Clear loading states and error messaging
- Disabled inputs during submission
267-294: LGTM: Clean helper components.Both the
CardandErrorMessagecomponents are simple, focused, and properly styled. The use ofwhitespace-break-spacesinErrorMessageensures error text formatting is preserved.
| await submitSetup(request); | ||
| setIsSuccess(true); | ||
| } catch (err) { | ||
| const error = err as ApiError; |
There was a problem hiding this comment.
Unsafe type assertion on caught error.
The type assertion err as ApiError assumes the error always has this shape. If the API or network layer throws a different error type (e.g., network failure, CORS error), accessing error.code or error.message could fail.
🔎 Suggested fix to safely handle error types
} catch (err) {
- const error = err as ApiError;
+ const error = err as Partial<ApiError>;
let message = "An error occurred. Please try again.";
- switch (error.code) {
+ switch (error?.code) {
case 400:
message = "Invalid request. Please check your input.";
break;
case 412:
message = "Setup has already been completed. Redirecting to login...";
setTimeout(() => (window.location.href = "/"), 2000);
break;
case 422:
message =
error.message || "Validation error. Please check your input.";
break;
case 500:
message = "An error occurred. Please try again.";
break;
default:
- message = error.message || message;
+ message = error?.message || message;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/instance-setup/InstanceSetupWizard.tsx around line 82, the caught
error is unsafely asserted to ApiError which can throw if the thrown value isn't
that shape; update the handler to avoid a direct type assertion by narrowing the
error first (use typeof checks, instanceof, or a type-guard like isApiError that
checks for expected properties such as code/message), then safely read
error.code and error.message with fallbacks (e.g., default message or stringify
the error) so non-ApiError exceptions (network/CORS/unknown) are handled without
runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/assets/icons/IdentityProviderIcons.tsx (2)
15-23: Move the icons mapping outside the function to avoid recreation.The
iconsRecord is recreated on every function call, which is inefficient. Since the mapping is static, move it outside the function body as a constant.🔎 Proposed refactor
+const IDP_ICONS: Record<SSOIdentityProviderType, (size: number) => React.ReactNode> = { + google: (size) => <GoogleIcon size={size} />, + microsoft: (size) => <MicrosoftIcon size={size} />, + entra: (size) => <EntraIcon size={size} />, + okta: (size) => <OktaIcon size={size} className="text-nb-gray-300" />, + pocketid: (size) => <PocketIdIcon size={size} />, + zitadel: (size) => <ZitadelIcon size={size} />, + oidc: (size) => <KeyRound size={size} className="text-nb-gray-400" />, +}; + export const idpIcon = ( type: SSOIdentityProviderType, size: number = 16, ): React.ReactNode => { - const icons: Record<SSOIdentityProviderType, React.ReactNode> = { - google: <GoogleIcon size={size} />, - microsoft: <MicrosoftIcon size={size} />, - entra: <EntraIcon size={size} />, - okta: <OktaIcon size={size} className="text-nb-gray-300" />, - pocketid: <PocketIdIcon size={size} />, - zitadel: <ZitadelIcon size={size} />, - oidc: <KeyRound size={size} className="text-nb-gray-400" />, - }; - - return icons[type]; + return IDP_ICONS[type]?.(size); };
25-25: Consider adding a fallback for invalid provider types.While TypeScript should prevent invalid types at compile time, adding a fallback (e.g., returning
nullor a default icon) would make the function more defensive against runtime errors.🔎 Suggested defensive handling
- return icons[type]; + return icons[type] ?? null;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/assets/icons/IdentityProviderIcons.tsxsrc/interfaces/IdentityProvider.tssrc/modules/settings/IdentityProviderModal.tsxsrc/modules/settings/IdentityProvidersTab.tsxsrc/modules/users/table-cells/UserNameCell.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interfaces/IdentityProvider.ts
- src/modules/settings/IdentityProvidersTab.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/assets/icons/IdentityProviderIcons.tsx (7)
src/interfaces/IdentityProvider.ts (1)
SSOIdentityProviderType(34-41)src/assets/icons/GoogleIcon.tsx (1)
GoogleIcon(3-31)src/assets/icons/MicrosoftIcon.tsx (1)
MicrosoftIcon(3-16)src/assets/icons/EntraIcon.tsx (1)
EntraIcon(3-39)src/assets/icons/OktaIcon.tsx (1)
OktaIcon(3-18)src/assets/icons/PocketIdIcon.tsx (1)
PocketIdIcon(3-17)src/assets/icons/ZitadelIcon.tsx (1)
ZitadelIcon(3-32)
src/modules/settings/IdentityProviderModal.tsx (3)
src/interfaces/IdentityProvider.ts (4)
SSOIdentityProvider(65-72)SSOIdentityProviderType(34-41)SSOIdentityProviderRequest(74-80)SSOIdentityProviderOptions(43-54)src/utils/api.tsx (1)
useApiCall(169-212)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(11-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (9)
src/modules/users/table-cells/UserNameCell.tsx (3)
21-29: LGTM! Identity provider fetching is correctly gated.The conditional fetching based on
embeddedIdpEnabledis implemented correctly using theenabledparameter. This prevents unnecessary API calls when the feature is disabled.
31-34: LGTM! userIdp computation is efficient and correct.The
useMemohook properly memoizes the IdP lookup with correct dependencies, and the early return handles edge cases appropriately.
84-97: LGTM! IdP badge rendering is well-implemented.The conditional rendering logic correctly excludes invited/blocked users, and the tooltip provides good UX. The styling is consistent with the existing status badge implementation.
src/modules/settings/IdentityProviderModal.tsx (6)
67-73: LGTM! State initialization correctly handles create and edit modes.The client secret is properly initialized as empty for security (not exposing existing secrets during edit), while other fields correctly fallback to provider values or sensible defaults.
79-79: LGTM! Conditional issuer requirement is correct.Google and Microsoft use well-known discovery endpoints, so they don't require manual issuer configuration. This logic correctly identifies which providers need explicit issuer URLs.
81-93: LGTM! Form validation is comprehensive and correct.The validation logic properly:
- Trims whitespace to prevent invalid inputs
- Conditionally requires issuer based on provider type
- Allows empty client secret during edit (preserves existing secret)
- Uses
useMemowith correct dependencies for performance
102-137: LGTM! Submit handler correctly manages create/edit flows.The handler properly:
- Trims all input fields before submission
- Uses appropriate HTTP methods (POST for create, PUT for edit)
- Provides clear user feedback via notifications
- Invalidates the SWR cache to refresh the list
- Shows the redirect URL modal only when relevant
162-254: LGTM! Form fields are well-structured with good UX.The form implementation demonstrates good practices:
- Clear labels and contextual help text for each field
- Icons from the centralized
idpIconutility- Password masking for client secret
- Contextual placeholders that differ between create and edit modes
- Conditional rendering of issuer field based on provider type
- Display of redirect URL during edit for user reference
284-329: LGTM! Success modal enforces acknowledgment of redirect URL.The modal implementation uses appropriate UX patterns:
- Prevents dismissal via escape key or outside clicks
- Hides the close button to ensure user interaction
- Provides a clear call-to-action that copies and closes in one step
- Uses the
Codecomponent for easy copying of the redirect URLThis ensures users don't miss the critical redirect URL configuration step.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/settings/IdentityProviderModal.tsx (2)
62-65: Consider safer URL construction for the update request.While not a runtime issue (since
updateRequestis only called whenisEditingis true), constructing the URL withprovider?.idwhen provider might be null/undefined is fragile.🔎 Alternative: conditionally create the hook or use empty string fallback
const createRequest = useApiCall<SSOIdentityProvider>("/identity-providers"); const updateRequest = useApiCall<SSOIdentityProvider>( - "/identity-providers/" + provider?.id, + `/identity-providers/${provider?.id ?? ""}`, );
79-79: Consider centralizing provider-specific configuration.The
requiresIssuerlogic is hardcoded here, which means any new provider types that don't require an issuer would need updates in this file.💡 Consider moving this to the provider type definition
In
src/interfaces/IdentityProvider.ts, you could add:export const ProviderConfig: Record<SSOIdentityProviderType, { requiresIssuer: boolean }> = { google: { requiresIssuer: false }, microsoft: { requiresIssuer: false }, entra: { requiresIssuer: true }, okta: { requiresIssuer: true }, // ... };Then use:
const requiresIssuer = ProviderConfig[type].requiresIssuer;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/modules/settings/IdentityProviderModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/settings/IdentityProviderModal.tsx (10)
src/interfaces/IdentityProvider.ts (4)
SSOIdentityProvider(65-72)SSOIdentityProviderType(34-41)SSOIdentityProviderRequest(74-80)SSOIdentityProviderOptions(43-54)src/utils/api.tsx (1)
useApiCall(169-212)src/components/Notification.tsx (1)
notify(151-155)src/components/modal/Modal.tsx (4)
Modal(209-209)ModalContent(211-211)ModalFooter(213-213)ModalClose(210-210)src/components/modal/ModalHeader.tsx (1)
ModalHeader(16-53)src/components/Separator.tsx (1)
Separator(1-3)src/components/Label.tsx (1)
Label(25-25)src/components/HelpText.tsx (1)
HelpText(9-25)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(11-26)src/components/Input.tsx (1)
Input(182-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (4)
src/modules/settings/IdentityProviderModal.tsx (4)
102-137: Submit logic is well-structured.The handler correctly:
- Trims all inputs before submission
- Differentiates between create and update flows
- Uses the notification system for async feedback
- Mutates the SWR cache to refresh the provider list
- Conditionally shows the success modal only when a redirect URL is returned
162-254: Form UI is well-structured with good UX.The form implementation includes:
- Clear labels and help text for each field
- Conditional rendering of the issuer field based on provider type
- Visual icons prefixing each input for clarity
- Proper readonly display of redirect URL during edit
- Responsive validation that disables submission until requirements are met
284-329: Success modal design effectively ensures users capture the redirect URL.The modal intentionally:
- Prevents dismissal via escape key, outside clicks, or the close button (lines 292-294, 297)
- Forces explicit interaction through the "Copy & Close" button
- Provides clear instructions on what to do with the redirect URL
This is good UX for critical post-creation information that users must not miss.
81-93: Verify backend handling of emptyclient_secretduring updates.The frontend allows empty
client_secretwhen editing (validation at line 90) and the help text states that leaving it empty preserves the existing secret. However, line 108 always includesclient_secretin the payload as an empty string, even during updates. The backend must be checked to confirm whether an emptyclient_secretis interpreted as "no change" rather than "clear the secret," as the help text promises.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/modules/settings/IdentityProvidersTab.tsx (2)
42-52: Consider removing duplicate label mapping.
idpTypeLabelsis exported but unused in this file. TheTypecolumn (line 174) usesgetSSOIdentityProviderLabelByTypefrom@/interfaces/IdentityProviderinstead. Having two sources of truth for the same labels could lead to inconsistencies if one is updated without the other.Either consolidate by removing this export and using
getSSOIdentityProviderLabelByTypeeverywhere, or replace the usage at line 174 with this exported mapping.
158-165: Redundant fallback icon.The
idpIconfunction already handles allSSOIdentityProviderTypevalues includingoidc(which returnsKeyRound). Since the type system ensuresrow.original.typeis always a validSSOIdentityProviderType, the fallback|| (<KeyRound .../>)will never be reached.Consider simplifying:
- {idpIcon(row.original.type) || ( - <KeyRound size={16} className="text-nb-gray-400" /> - )} + {idpIcon(row.original.type)}src/assets/icons/IdentityProviderIcons.tsx (2)
13-30: Consider memoizing icon instances or hoisting the map.The
iconsRecord is recreated on every call toidpIcon, instantiating new React elements each time. For frequent renders (e.g., tables with many rows), this creates unnecessary object allocations.Consider hoisting the map outside or memoizing:
🔎 Suggested optimization
+const createIconMap = (size: number): Record<SSOIdentityProviderType, React.ReactNode> => ({ + google: <GoogleIcon size={size} />, + microsoft: <MicrosoftIcon size={size} />, + entra: <EntraIcon size={size} />, + okta: <OktaIcon size={size} className="text-nb-gray-300" />, + pocketid: <PocketIdIcon size={size} />, + zitadel: <ZitadelIcon size={size} />, + authentik: <AuthentikIcon size={size} />, + keycloak: <KeycloakIcon size={size} />, + oidc: <KeyRound size={size} className="text-nb-gray-400" />, +}); + +// Cache for common sizes +const iconCache = new Map<number, Record<SSOIdentityProviderType, React.ReactNode>>(); + export const idpIcon = ( type: SSOIdentityProviderType, size: number = 16, ): React.ReactNode => { - const icons: Record<SSOIdentityProviderType, React.ReactNode> = { - google: <GoogleIcon size={size} />, - microsoft: <MicrosoftIcon size={size} />, - entra: <EntraIcon size={size} />, - okta: <OktaIcon size={size} className="text-nb-gray-300" />, - pocketid: <PocketIdIcon size={size} />, - zitadel: <ZitadelIcon size={size} />, - authentik: <AuthentikIcon size={size} />, - keycloak: <KeycloakIcon size={size} />, - oidc: <KeyRound size={size} className="text-nb-gray-400" />, - }; - - return icons[type]; + if (!iconCache.has(size)) { + iconCache.set(size, createIconMap(size)); + } + return iconCache.get(size)![type]; };
21-21: Inconsistent icon styling.
OktaIconappliesclassName="text-nb-gray-300"while other branded icons (Google, Microsoft, Entra, etc.) don't have any className applied. If this is intentional due to OktaIcon's design, consider adding a comment explaining why. Otherwise, consider removing it for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/assets/icons/AuthentikIcon.tsxsrc/assets/icons/IdentityProviderIcons.tsxsrc/assets/icons/KeycloakIcon.tsxsrc/interfaces/IdentityProvider.tssrc/modules/settings/IdentityProvidersTab.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/interfaces/IdentityProvider.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/assets/icons/AuthentikIcon.tsx (1)
src/assets/icons/IconProperties.tsx (2)
IconProps(1-5)iconProperties(14-26)
src/modules/settings/IdentityProvidersTab.tsx (8)
src/interfaces/IdentityProvider.ts (3)
SSOIdentityProviderType(34-43)SSOIdentityProvider(69-76)getSSOIdentityProviderLabelByType(60-67)src/contexts/DialogProvider.tsx (1)
useDialog(132-132)src/utils/api.tsx (1)
useApiCall(169-212)src/contexts/PermissionsProvider.tsx (1)
usePermissions(39-39)src/components/Notification.tsx (1)
notify(151-155)src/components/DropdownMenu.tsx (4)
DropdownMenu(207-207)DropdownMenuTrigger(221-221)DropdownMenuContent(209-209)DropdownMenuItem(211-211)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(13-30)src/modules/settings/IdentityProviderModal.tsx (1)
IdentityProviderModal(54-332)
src/assets/icons/KeycloakIcon.tsx (1)
src/assets/icons/IconProperties.tsx (2)
IconProps(1-5)iconProperties(14-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (5)
src/modules/settings/IdentityProvidersTab.tsx (3)
59-117: LGTM!The
ActionCellcomponent is well-implemented with proper permission guards on both Edit and Delete actions, a confirmation dialog for destructive operations, and toast notifications for user feedback.
215-220: LGTM!The modal integration with
key={modalOpen ? 1 : 0}correctly ensures the component re-renders and resets state when the modal opens/closes, addressing the editing flow requirements.
229-230: Verify search functionality matches placeholder text.The placeholder says "Search by name or type..." but typically
DataTableonly searches on theaccessorKeyfields. Verify that the search actually filters by bothnameandtypecolumns, or update the placeholder to accurately reflect the search behavior.src/assets/icons/AuthentikIcon.tsx (1)
3-27: LGTM!The icon component follows the established pattern using
iconPropertiesfor consistent sizing and styling. Clean implementation.src/assets/icons/KeycloakIcon.tsx (1)
3-87: LGTM!The Keycloak icon implementation correctly follows the established pattern with
iconProperties. The SVG uses inline fills without gradient IDs, avoiding the collision issues noted in other icon components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/settings/IdentityProviderModal.tsx (2)
123-130: Consider conditionally omitting issuer for Google/Microsoft.The payload always includes
issuer(Line 127), even for Google and Microsoft where it's not displayed or required. If a user selects a different provider type, enters an issuer, then switches to Google, the old issuer value is still sent. While the backend should handle this, it's cleaner to conditionally omit it.🔎 Conditionally construct payload based on requiresIssuer
const submit = () => { const payload: SSOIdentityProviderRequest = { type, name: trim(name), - issuer: trim(issuer), + issuer: requiresIssuer ? trim(issuer) : "", client_id: trim(clientId), client_secret: trim(clientSecret), };
259-267: Consider adding a password visibility toggle.Client secrets can be long and complex. Adding a visibility toggle (show/hide button) would help users verify they've entered the correct value, reducing errors.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/modules/settings/IdentityProviderModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/settings/IdentityProviderModal.tsx (8)
src/interfaces/IdentityProvider.ts (4)
SSOIdentityProviderType(34-43)SSOIdentityProvider(69-76)SSOIdentityProviderRequest(78-84)SSOIdentityProviderOptions(45-58)src/utils/api.tsx (1)
useApiCall(169-212)src/components/Notification.tsx (1)
notify(151-155)src/components/modal/ModalHeader.tsx (1)
ModalHeader(16-53)src/components/Separator.tsx (1)
Separator(1-3)src/components/Label.tsx (1)
Label(25-25)src/components/HelpText.tsx (1)
HelpText(9-25)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(13-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (6)
src/modules/settings/IdentityProviderModal.tsx (6)
46-65: LGTM: Well-structured provider metadata.The
issuerHintsanddefaultNamesconstants provide clear guidance and defaults for different provider types, improving the user experience.
102-114: LGTM: Robust validation logic.The validation correctly handles all required fields, conditionally checks the issuer based on provider type, and appropriately makes client secret optional during edits to preserve existing secrets.
132-158: LGTM: Well-structured async flows with proper cache invalidation.Both create and edit flows use the notification system correctly, invalidate the SWR cache on success, and the create flow thoughtfully presents the redirect URL in a success modal when available.
189-195: LGTM: Smart default naming behavior.Auto-filling the name with the default when changing provider type (only in create mode) provides a good starting point while preserving user customization in edit mode.
311-356: LGTM: Effective forced-copy flow for critical setup data.The success modal correctly prevents accidental dismissal, ensuring users copy the redirect URL before proceeding. The copy-then-close flow is well-implemented.
226-239: LGTM: Clean conditional rendering with helpful placeholders.The issuer field is appropriately hidden for Google and Microsoft, and shown with provider-specific example URLs for others, streamlining the user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/modules/settings/IdentityProviderModal.tsx (1)
90-96: State persists when editing different providers.The
key={open ? 1 : 0}approach on Line 167 doesn't fully solve state reset. When the modal reopens with a different provider (e.g., user clicks Edit on provider B after closing the modal from provider A), both timesopenistrue, so the key remains1and the component doesn't remount. The form state from the previous provider persists.🔎 Add useEffect to reset state when provider changes
+ React.useEffect(() => { + if (open) { + setType(provider?.type ?? "oidc"); + setName(provider?.name ?? ""); + setIssuer(provider?.issuer ?? ""); + setClientId(provider?.client_id ?? ""); + setClientSecret(""); + } + }, [open, provider]); + const [type, setType] = useState<SSOIdentityProviderType>( provider?.type ?? "oidc", );Then remove the
keyprop at Line 167:<Modal open={open} onOpenChange={(state) => !state && onClose()} - key={open ? 1 : 0} >Also applies to: 167-167
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/(dashboard)/settings/page.tsxsrc/interfaces/Permission.tssrc/modules/settings/IdentityProviderModal.tsxsrc/modules/settings/IdentityProvidersTab.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/app/(dashboard)/settings/page.tsx (1)
src/modules/settings/IdentityProvidersTab.tsx (1)
IdentityProvidersTab(119-287)
src/modules/settings/IdentityProviderModal.tsx (11)
src/interfaces/IdentityProvider.ts (4)
SSOIdentityProviderType(34-43)SSOIdentityProvider(69-76)SSOIdentityProviderRequest(78-84)SSOIdentityProviderOptions(45-58)src/contexts/PermissionsProvider.tsx (1)
usePermissions(39-39)src/utils/api.tsx (1)
useApiCall(169-212)src/components/Notification.tsx (1)
notify(151-155)src/components/modal/Modal.tsx (4)
Modal(209-209)ModalContent(211-211)ModalFooter(213-213)ModalClose(210-210)src/components/modal/ModalHeader.tsx (1)
ModalHeader(16-53)src/components/Separator.tsx (1)
Separator(1-3)src/components/Label.tsx (1)
Label(25-25)src/components/HelpText.tsx (1)
HelpText(9-25)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(13-30)src/components/Input.tsx (1)
Input(182-182)
src/modules/settings/IdentityProvidersTab.tsx (4)
src/interfaces/IdentityProvider.ts (3)
SSOIdentityProviderType(34-43)SSOIdentityProvider(69-76)getSSOIdentityProviderLabelByType(60-67)src/contexts/DialogProvider.tsx (1)
useDialog(132-132)src/contexts/PermissionsProvider.tsx (1)
usePermissions(39-39)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(13-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (15)
src/interfaces/Permission.ts (1)
25-25: LGTM!The addition of the
identity_providerspermission field is consistent with the existing permission structure and properly integrates with the broader Identity Provider management feature.src/app/(dashboard)/settings/page.tsx (3)
7-7: LGTM!The imports for
FingerprintIconandIdentityProvidersTabare correctly added to support the new Identity Providers tab functionality.Also applies to: 23-23
58-64: LGTM!The conditional rendering of the Identity Providers tab trigger is properly guarded by both the account's embedded IdP feature flag and the appropriate read permission.
92-93: LGTM!The conditional rendering of the
IdentityProvidersTabcontent uses the same guards as the tab trigger, ensuring consistent access control.src/modules/settings/IdentityProviderModal.tsx (7)
47-66: LGTM!The
issuerHintsanddefaultNamesconstants provide helpful UX defaults for each provider type. The use ofPartial<Record>forissuerHintscorrectly reflects that not all provider types require issuer URLs (e.g., Google and Microsoft).
102-116: LGTM!The form validation logic is well-structured:
- Properly trims input values before validation
- Correctly requires issuer only for provider types that need it
- Appropriately handles client secret as optional during edit operations
- Uses
useMemowith correct dependencies for performance
125-160: LGTM!The submit handler is well-implemented:
- Properly trims all input values before submission
- Correctly branches between create and update operations
- Invalidates the SWR cache after successful operations
- Appropriately shows the success modal only for create operations when a redirect URL is returned
185-213: LGTM!The provider type selector provides excellent UX by automatically setting a default name when the type changes during creation (Lines 194-196). This reduces manual input while still allowing customization.
228-241: LGTM!The conditional rendering of the issuer field based on provider type is correct. Google and Microsoft don't require manual issuer configuration, which aligns with the standard OAuth2 implementations for these providers.
272-282: LGTM!Displaying the redirect URL during edit operations allows users to verify the configuration without re-creating the provider. The copyable code component provides good UX.
318-363: LGTM!The success modal implementation ensures users don't miss the critical redirect URL by preventing dismissal via escape key or outside clicks (Lines 326-328). The "Copy & Close" workflow is intuitive and reduces the chance of configuration errors.
src/modules/settings/IdentityProvidersTab.tsx (4)
42-52: LGTM!The exported
idpTypeLabelsmapping provides human-readable labels for each identity provider type and can be reused across components for consistent type display throughout the application.
59-117: LGTM!The
ActionCellcomponent properly implements:
- Edit and delete actions with appropriate icons
- Permission checks preventing unauthorized operations (Lines 100, 107)
- Confirmation dialog before deletion with clear messaging
- SWR cache invalidation after successful deletion
- User feedback via toast notifications
151-186: LGTM!The table column definitions provide a clean, informative display:
- Name column with provider icon for visual identification
- Type column using localized labels via
getSSOIdentityProviderLabelByType- Actions column with edit/delete functionality
- Fallback icon (KeyRound) for unknown provider types
222-284: LGTM!The DataTable implementation is comprehensive:
- Loading states handled properly
- Sorting persistence via localStorage with a namespace-specific key
- Click-to-edit workflow on table rows
- GetStarted card for empty state with clear call-to-action
- Right-side "Add" button for non-empty states
- Refresh functionality with cache invalidation
- Permission checks on creation actions (Lines 248, 263)
| <IdentityProviderModal | ||
| open={modalOpen} | ||
| key={modalOpen ? 1 : 0} | ||
| onClose={handleCloseModal} | ||
| provider={editProvider} | ||
| /> |
There was a problem hiding this comment.
State persists when editing different providers.
The same key={modalOpen ? 1 : 0} pattern used here has the limitation mentioned in the IdentityProviderModal review. When switching between different providers while the modal is open (or reopening after close with a different provider), the key remains 1 and doesn't trigger a remount, so stale form state may persist.
The fix should be applied in the IdentityProviderModal component itself (add a useEffect to reset state when provider changes), rather than relying on the key prop.
🤖 Prompt for AI Agents
In src/modules/settings/IdentityProvidersTab.tsx around lines 215 to 220, the
modal is keyed with modalOpen which prevents remount when switching providers
and causes stale form state; instead modify the IdentityProviderModal component
to watch its provider prop and reset its internal form/state when provider
changes: add a useEffect that runs on provider change to reinitialize form
values (or call your existing reset/initialize function) and clear any transient
state/errors so the modal shows the new provider fresh without relying on parent
key toggling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/netbird.ts (1)
18-22: Consider case-insensitive matching for "selfhosted".The
includes()method is case-sensitive, so hostnames like "SelfHosted.example.com" would not be caught by this check. This could lead to self-hosted instances being incorrectly identified as hosted, affecting UI behavior in UserInviteModal and UsersTable.🔎 Proposed fix for case-insensitive matching
export const isNetBirdHosted = () => { const hostname = window.location.hostname; - if (hostname.includes("selfhosted")) return false; + if (hostname.toLowerCase().includes("selfhosted")) return false; return hostname.endsWith(".netbird.io") || hostname.endsWith(".wiretrustee.com"); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/netbird.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/hooks/useRedirect.tsx:
- Around line 41-48: The retry logic in performRedirect() can loop forever
because the setTimeout callback calls performRedirect() unbounded; add a maximum
retry limit constant (e.g., MAX_RETRIES) and a retry counter (use a ref like
retryCountRef) that increments each attempt; before scheduling or calling
performRedirect() again check retryCountRef.current < MAX_RETRIES, and if the
limit is reached clear timeoutRef, stop retrying and optionally call a failure
handler or log the error using processLogger or console; update places
referencing timeoutRef, performRedirect, RETRY_DELAY and targetPath to use the
new retry guard so retries terminate.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/not-found.tsxsrc/app/page.tsxsrc/auth/OIDCProvider.tsxsrc/hooks/useRedirect.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/app/page.tsx (1)
src/hooks/useRedirect.tsx (1)
useRedirect(9-58)
src/auth/OIDCProvider.tsx (1)
src/auth/OIDCError.tsx (1)
OIDCError(13-98)
src/app/not-found.tsx (1)
src/hooks/useRedirect.tsx (1)
useRedirect(9-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (7)
src/app/not-found.tsx (1)
37-41: LGTM!The redirect logic correctly passes
truefor thereplaceparameter, which usesrouter.replaceto avoid polluting the browser history stack. Theenableparameter correctly defaults totrue.src/auth/OIDCProvider.tsx (3)
74-80: Usingwindow.location.replacebypasses Next.js router entirely.This performs a hard navigation which will cause a full page reload. This may be intentional for the OIDC callback flow to ensure clean state, but it differs from the soft navigation provided by Next.js router.
Please confirm this behavior is intentional - using
window.location.replacewill trigger a full page reload rather than a client-side navigation.
104-107: LGTM!The
/setuppath bypass is appropriate for the first-time instance setup wizard, which needs to run before authentication is configured.
110-117: LGTM!Enabling
withCustomHistoryand usingFullScreenLoadingfor the callback success component provides a consistent loading experience during the OIDC callback flow.src/hooks/useRedirect.tsx (2)
19-32: LGTM!The path parsing and early exit conditions are well-designed - comparing against both
window.location.pathnameand the Next.jsusePathnamehook provides robust path detection and avoids unnecessary redirects.
52-57: LGTM!The cleanup function properly clears the timeout when the component unmounts or dependencies change, preventing stale redirect attempts.
src/app/page.tsx (1)
38-42: LGTM!Consistent with the pattern in
not-found.tsx- usingreplace=truefor history replacement navigation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hooks/useRedirect.tsx (1)
36-54: Previous concern about unbounded retries is now addressed.The addition of
MAX_RETRIESresolves the infinite loop issue flagged in the prior review.Consider adding logging when retries exhaust. When the maximum retry count is reached without successful navigation, the redirect silently stops. Adding a warning log would help with debugging navigation issues in production.
🔎 Optional: Add logging when retries exhaust
// Retry if navigation hasn't occurred and we haven't exceeded max retries if (retryCountRef.current < MAX_RETRIES) { timeoutRef.current = setTimeout(() => { // Check again if we're still not on the target path if (window.location.pathname !== targetPath) { performRedirect(); } }, RETRY_DELAY); + } else { + console.warn(`[useRedirect] Failed to navigate to ${url} after ${MAX_RETRIES} attempts`); } };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/hooks/useRedirect.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (1)
src/hooks/useRedirect.tsx (1)
58-64: LGTM!Cleanup properly clears the timeout and resets the retry counter, preventing memory leaks and ensuring fresh state when dependencies change. The dependency array is complete.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/modules/settings/IdentityProviderModal.tsx:
- Around line 90-96: The component's local state (type, name, issuer, clientId,
clientSecret) is initialized from provider via useState but never updated when
provider changes while the modal is open; add a useEffect that watches provider
and updates setType(provider?.type ?? "oidc"), setName(provider?.name ?? ""),
setIssuer(provider?.issuer ?? ""), setClientId(provider?.client_id ?? ""), and
setClientSecret("") (or provider secret behavior as appropriate) to resync state
whenever provider changes, and remove the key={open ? 1 : 0} remount workaround
so the modal no longer relies on remounting to reset state.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/modules/settings/IdentityProviderModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/modules/settings/IdentityProviderModal.tsx (7)
src/interfaces/IdentityProvider.ts (4)
SSOIdentityProviderType(34-43)SSOIdentityProvider(69-76)SSOIdentityProviderRequest(78-84)SSOIdentityProviderOptions(45-58)src/contexts/PermissionsProvider.tsx (1)
usePermissions(39-39)src/utils/api.tsx (1)
useApiCall(169-212)src/components/modal/Modal.tsx (4)
Modal(209-209)ModalContent(211-211)ModalFooter(213-213)ModalClose(210-210)src/components/Label.tsx (1)
Label(25-25)src/assets/icons/IdentityProviderIcons.tsx (1)
idpIcon(13-30)src/components/Input.tsx (1)
Input(182-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_n_push
🔇 Additional comments (1)
src/modules/settings/IdentityProviderModal.tsx (1)
118-126: Ensure backend API accepts emptyissuerfield for Google and Microsoft provider types.The UI correctly excludes the issuer field requirement for Google and Microsoft providers (line 98), but the form still sends an empty string for
issuerin the payload (line 122). Verify the backend API validates or accepts empty issuer values for these provider types. Additionally, consider updating theSSOIdentityProviderRequestinterface to makeissueroptional to clarify the contract.
| const [type, setType] = useState<SSOIdentityProviderType>( | ||
| provider?.type ?? "oidc", | ||
| ); | ||
| const [name, setName] = useState(provider?.name ?? ""); | ||
| const [issuer, setIssuer] = useState(provider?.issuer ?? ""); | ||
| const [clientId, setClientId] = useState(provider?.client_id ?? ""); | ||
| const [clientSecret, setClientSecret] = useState(""); |
There was a problem hiding this comment.
State doesn't reset when the provider prop changes while the modal remains open.
The key={open ? 1 : 0} workaround on line 155 only forces a remount when open toggles from false to true. If the provider prop changes while the modal is already open, the key remains 1 and the component doesn't remount, causing stale state to persist.
🔎 Add useEffect to sync state with provider changes
+ React.useEffect(() => {
+ if (open) {
+ setType(provider?.type ?? "oidc");
+ setName(provider?.name ?? "");
+ setIssuer(provider?.issuer ?? "");
+ setClientId(provider?.client_id ?? "");
+ setClientSecret("");
+ }
+ }, [open, provider]);
+
const [type, setType] = useState<SSOIdentityProviderType>(
provider?.type ?? "oidc",
);Then remove the key prop from line 155:
<Modal
open={open}
onOpenChange={(state) => !state && onClose()}
- key={open ? 1 : 0}
>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/modules/settings/IdentityProviderModal.tsx around lines 90 - 96, The
component's local state (type, name, issuer, clientId, clientSecret) is
initialized from provider via useState but never updated when provider changes
while the modal is open; add a useEffect that watches provider and updates
setType(provider?.type ?? "oidc"), setName(provider?.name ?? ""),
setIssuer(provider?.issuer ?? ""), setClientId(provider?.client_id ?? ""), and
setClientSecret("") (or provider secret behavior as appropriate) to resync state
whenever provider changes, and remove the key={open ? 1 : 0} remount workaround
so the modal no longer relies on remounting to reset state.
2 participants
Summary by CodeRabbit
New Features
Behavioral Changes
✏️ Tip: You can customize this high-level summary in your review settings.