- Notifications
You must be signed in to change notification settings - Fork3k
[WEB-5324] refactor: add Unified OAuth Configuration and Missing Gitea Options#8050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:preview
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- Replaced the AuthenticationModes component with a more streamlined implementation using AuthenticationMethodCard.- Removed obsolete authentication modes files from the codebase.- Enhanced the AuthRoot component to utilize the new OAuth configuration hook for better management of authentication options.- Updated type definitions for instance authentication modes to reflect the new structure.
coderabbitaibot commentedOct 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughAuthentication handling was refactored: admin auth modes moved from an exported array/component to a map + hook; UI now renders AuthenticationMethodCard list after theme resolution. OAuth configuration for Web/Space was moved to composable hooks (core + extended + composer). Several barrel re-exports and a UX UpgradeButton file were removed. Types were tightened to a finite set of core auth keys and new OAuth config types were added. Changes
Sequence Diagram(s)sequenceDiagram autonumber participant Page as Admin auth page participant Theme as Theme resolver participant Hook as useAuthenticationModes participant Core as getCoreAuthenticationModesMap participant UI as AuthenticationMethodCard... Page->>Theme: resolveGeneralTheme(resolvedThemeAdmin) Page->>Hook: useAuthenticationModes({ disabled,isSubmitting,updateConfig,resolvedTheme }) activate Hook Hook->>Core: getCoreAuthenticationModesMap(props) Core-->>Hook: modes map (keys: unique-codes, passwords-login, google, github, gitlab, gitea) Hook-->>Page: ordered modes array deactivate Hook Page->>UI: render AuthenticationMethodCard for each modesequenceDiagram autonumber participant AuthRoot as auth-root.tsx participant UseOAuth as useOAuthConfig("Continue") participant Core as useCoreOAuthConfig participant Ext as useExtendedOAuthConfig participant Instance as useInstance() AuthRoot->>UseOAuth: invoke activate UseOAuth UseOAuth->>Core: useCoreOAuthConfig("Continue") activate Core Core->>Instance: read provider flags Instance-->>Core: config (google, github, gitlab, gitea) Core-->>UseOAuth: { isOAuthEnabled, oAuthOptions } deactivate Core UseOAuth->>Ext: useExtendedOAuthConfig("Continue") activate Ext Ext-->>UseOAuth: { isOAuthEnabled:false, oAuthOptions:[] } deactivate Ext rect rgb(230,245,230) Note over UseOAuth: compose -> isOAuthEnabled = Core.isOAuthEnabled OR Ext.isOAuthEnabled Note over UseOAuth: oAuthOptions = Core.oAuthOptions ++ Ext.oAuthOptions end UseOAuth-->>AuthRoot: merged configs deactivate UseOAuth AuthRoot->>AuthRoot: render OAuthOptions based on merged oAuthOptionsEstimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to pay extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates thehigh-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx(4 hunks)apps/admin/ce/components/authentication/index.ts(0 hunks)apps/admin/ce/components/common/upgrade-button.tsx(1 hunks)apps/admin/core/hooks/oauth/core.tsx(3 hunks)apps/admin/core/hooks/oauth/index.ts(1 hunks)apps/admin/core/hooks/oauth/types.ts(1 hunks)apps/admin/ee/components/authentication/authentication-modes.tsx(0 hunks)apps/admin/ee/components/authentication/index.ts(0 hunks)apps/space/core/components/account/auth-forms/auth-root.tsx(3 hunks)apps/space/core/hooks/oauth/core.tsx(1 hunks)apps/space/core/hooks/oauth/extended.tsx(1 hunks)apps/space/core/hooks/oauth/index.ts(1 hunks)apps/space/core/hooks/oauth/types.ts(1 hunks)apps/web/core/components/account/auth-forms/auth-root.tsx(3 hunks)apps/web/core/hooks/oauth/core.tsx(1 hunks)apps/web/core/hooks/oauth/extended.tsx(1 hunks)apps/web/core/hooks/oauth/index.ts(1 hunks)apps/web/core/hooks/oauth/types.ts(1 hunks)packages/types/src/instance/auth.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/admin/ee/components/authentication/authentication-modes.tsx
- apps/admin/ee/components/authentication/index.ts
- apps/admin/ce/components/authentication/index.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshitaRepo: makeplane/plane PR: 7393File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104Timestamp: 2025-07-14T11:22:43.964ZLearning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.Applied to files:
apps/admin/app/(all)/(dashboard)/authentication/page.tsxapps/space/core/components/account/auth-forms/auth-root.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/web/core/components/account/auth-forms/auth-root.tsxapps/admin/core/hooks/oauth/core.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7989File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46Timestamp: 2025-10-21T17:22:05.204ZLearning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/web/core/components/account/auth-forms/auth-root.tsx
📚 Learning: 2025-10-09T22:12:26.424Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T22:12:26.424ZLearning: When `types/react` is installed in a TypeScript project (which is standard for React + TypeScript codebases), React types (React.FC, React.ReactNode, React.ComponentProps, etc.) are globally available by design. These type annotations can and should be used without explicitly importing the React namespace. This is a TypeScript/DefinitelyTyped feature, not codebase-specific configuration.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/web/core/components/account/auth-forms/auth-root.tsx
📚 Learning: 2025-10-09T20:43:07.762Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/core/components/new-user-popup.tsx:4-6Timestamp: 2025-10-09T20:43:07.762ZLearning: The `next-themes` library is React-compatible and can be used outside of Next.js applications. It's not Next.js-specific despite its name.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/web/core/components/account/auth-forms/auth-root.tsxapps/admin/core/hooks/oauth/core.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7888File: packages/propel/src/avatar/avatar.stories.tsx:2-3Timestamp: 2025-10-01T15:30:17.605ZLearning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.Applied to files:
apps/web/core/components/account/auth-forms/auth-root.tsx
🧬 Code graph analysis (14)
apps/web/core/hooks/oauth/types.ts (2)
packages/ui/src/oauth/oauth-options.tsx (1)
TOAuthOption(5-11)apps/space/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/admin/core/hooks/oauth/index.ts (3)
apps/admin/core/hooks/oauth/types.ts (1)
TGetAuthenticationModeProps(3-7)packages/types/src/instance/auth.ts (1)
TInstanceAuthenticationModes(1-8)apps/admin/core/hooks/oauth/core.tsx (1)
getCoreAuthenticationModesMap(26-107)
apps/admin/core/hooks/oauth/types.ts (1)
packages/types/src/instance/auth.ts (1)
TInstanceAuthenticationMethodKeys(10-17)
apps/web/core/hooks/oauth/extended.tsx (2)
apps/space/core/hooks/oauth/extended.tsx (1)
useExtendedOAuthConfig(4-7)apps/web/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/space/core/hooks/oauth/extended.tsx (2)
apps/web/core/hooks/oauth/extended.tsx (1)
useExtendedOAuthConfig(4-9)apps/space/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx (3)
apps/space/helpers/common.helper.ts (1)
resolveGeneralTheme(9-10)apps/admin/core/hooks/oauth/index.ts (1)
useAuthenticationModes(5-22)apps/admin/core/components/authentication/authentication-method-card.tsx (1)
AuthenticationMethodCard(17-56)
apps/space/core/hooks/oauth/types.ts (2)
packages/ui/src/oauth/oauth-options.tsx (1)
TOAuthOption(5-11)apps/web/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/web/core/hooks/oauth/core.tsx (2)
apps/space/core/hooks/oauth/core.tsx (1)
useCoreOAuthConfig(17-84)apps/web/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/space/core/hooks/oauth/core.tsx (2)
apps/web/core/hooks/oauth/core.tsx (1)
useCoreOAuthConfig(17-84)apps/space/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)
apps/web/core/hooks/oauth/index.ts (3)
apps/web/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)apps/web/core/hooks/oauth/core.tsx (1)
useCoreOAuthConfig(17-84)apps/web/core/hooks/oauth/extended.tsx (1)
useExtendedOAuthConfig(4-9)
apps/space/core/components/account/auth-forms/auth-root.tsx (2)
apps/space/core/hooks/oauth/index.ts (1)
useOAuthConfig(6-13)packages/ui/src/oauth/oauth-options.tsx (1)
OAuthOptions(21-57)
apps/web/core/components/account/auth-forms/auth-root.tsx (2)
apps/web/core/hooks/oauth/index.ts (1)
useOAuthConfig(6-13)packages/ui/src/oauth/oauth-options.tsx (1)
OAuthOptions(21-57)
apps/space/core/hooks/oauth/index.ts (3)
apps/space/core/hooks/oauth/types.ts (1)
TOAuthConfigs(9-12)apps/space/core/hooks/oauth/core.tsx (1)
useCoreOAuthConfig(17-84)apps/space/core/hooks/oauth/extended.tsx (1)
useExtendedOAuthConfig(4-7)
apps/admin/core/hooks/oauth/core.tsx (2)
packages/types/src/instance/auth.ts (2)
TGetBaseAuthenticationModeProps(41-45)TInstanceAuthenticationModes(1-8)apps/admin/ce/components/common/upgrade-button.tsx (1)
UpgradeButton(14-19)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/web/core/hooks/oauth/types.ts (1)
1-7:Import the React types to keep the build passing.
React.ReactNodeis referenced without importing theReactnamespace (orReactNode) in this module. TypeScript will fail with “Cannot find namespace 'React'” once this file is compiled. Pull in the React types explicitly and use them instead.+import type { ReactNode } from "react";+-export type TOAuthOption = {+export type TOAuthOption = { id: string; text: string;- icon: React.ReactNode;+ icon: ReactNode; onClick: () => void; enabled?: boolean; };⛔ Skipped due to learnings
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T22:12:26.424ZLearning: When `types/react` is installed in a TypeScript project (which is standard for React + TypeScript codebases), React types (React.FC, React.ReactNode, React.ComponentProps, etc.) are globally available by design. These type annotations can and should be used without explicitly importing the React namespace. This is a TypeScript/DefinitelyTyped feature, not codebase-specific configuration.Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.Learnt from: lifeiscontentRepo: makeplane/plane PR: 7888File: packages/propel/src/avatar/avatar.stories.tsx:2-3Timestamp: 2025-10-01T15:30:17.605ZLearning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
- Replaced local type imports with centralized imports from @plane/types in core, extended, and index OAuth hooks.- Removed the now redundant types.ts file as its definitions have been migrated.- Enhanced type definitions for OAuth options to improve consistency across the application.
Linked to Plane Work Item(s) This comment was auto-generated byPlane |
Merge branch 'preview' of github.com:makeplane/plane into refactor-oauth-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx (1)
30-30:Consider adding explicit error handling for configuration loading.While SWR provides built-in error handling, the component doesn't display an error state if
fetchInstanceConfigurationsfails. Consider adding error UI to improve user experience when the fetch fails.Example enhancement:
+ const { error } = useSWR("INSTANCE_CONFIGURATIONS", () => fetchInstanceConfigurations());Then add an error state in the render block to show a message when
erroris truthy.apps/admin/core/hooks/oauth/core.tsx (2)
3-13:Typed map factory for auth modes looks correct; only minor style nitThe switch to a keyed map with
getCoreAuthenticationModesMapis well-typed and ensures allTInstanceAuthenticationModes["key"]entries are covered. The explicit function type on the const plus arrow implementation is a bit verbose; if you ever want to trim it, you could instead annotate the parameter and return type directly on the arrow function, but this is purely stylistic and not required.Also applies to: 25-31
68-81:Minor copy consistency nit for GitLab/Gitea descriptionsThe GitLab and Gitea descriptions use
sign up to plane(lowercase “plane”), whereas other entries use “Plane” as a proper noun. Consider normalizing these strings for brand and copy consistency, e.g. “…sign up for Plane with their GitLab/Gitea accounts.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx(3 hunks)apps/admin/core/hooks/oauth/core.tsx(3 hunks)apps/space/core/components/account/auth-forms/auth-root.tsx(3 hunks)apps/space/core/hooks/oauth/core.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/space/core/hooks/oauth/core.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7989File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46Timestamp: 2025-10-21T17:22:05.204ZLearning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/admin/app/(all)/(dashboard)/authentication/page.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19Timestamp: 2025-10-09T20:42:31.843ZLearning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsx
📚 Learning: 2025-10-09T20:43:07.762Z
Learnt from: lifeiscontentRepo: makeplane/plane PR: 7922File: apps/admin/core/components/new-user-popup.tsx:4-6Timestamp: 2025-10-09T20:43:07.762ZLearning: The `next-themes` library is React-compatible and can be used outside of Next.js applications. It's not Next.js-specific despite its name.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsxapps/admin/core/hooks/oauth/core.tsx
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshitaRepo: makeplane/plane PR: 7393File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104Timestamp: 2025-07-14T11:22:43.964ZLearning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.Applied to files:
apps/space/core/components/account/auth-forms/auth-root.tsx
🧬 Code graph analysis (3)
apps/space/core/components/account/auth-forms/auth-root.tsx (2)
apps/space/core/hooks/oauth/index.ts (1)
useOAuthConfig(7-14)packages/ui/src/oauth/oauth-options.tsx (1)
OAuthOptions(21-57)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx (5)
apps/admin/core/hooks/store/use-instance.tsx (1)
useInstance(6-10)apps/admin/core/store/instance.store.ts (1)
formattedConfig(84-90)apps/space/helpers/common.helper.ts (1)
resolveGeneralTheme(1-2)apps/admin/core/hooks/oauth/index.ts (1)
useAuthenticationModes(5-22)apps/admin/core/components/authentication/authentication-method-card.tsx (1)
AuthenticationMethodCard(16-55)
apps/admin/core/hooks/oauth/core.tsx (2)
packages/types/src/instance/auth.ts (2)
TGetBaseAuthenticationModeProps(41-45)TInstanceAuthenticationModes(1-8)apps/admin/ce/components/common/upgrade-button.tsx (1)
UpgradeButton(14-19)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/admin/app/(all)/(dashboard)/authentication/page.tsx (2)
20-21:Theme resolution logic looks good.The two-step theme resolution (useTheme → resolveGeneralTheme) correctly normalizes the theme for consistent use in authentication mode rendering.
Also applies to: 28-28
103-113:Dynamic authentication mode rendering looks good.The mapping over
authenticationModescorrectly passes all required props toAuthenticationMethodCard, including proper key assignment and disabled state handling.apps/space/core/components/account/auth-forms/auth-root.tsx (2)
14-14:LGTM! Clean hook-driven OAuth refactoring.The new
useOAuthConfighook integration is well-implemented:
- The action text is correctly derived from
authMode(handles both sign-up and sign-in flows)- Hook usage follows React best practices (top-level, unconditional)
- The hook's default parameter
"Continue"provides a sensible fallback- This approach centralizes OAuth configuration and improves maintainability over static definitions
Also applies to: 83-84
151-151:LGTM! Proper conditional rendering with good UX consideration.The OAuth options rendering is correctly implemented:
- The
isOAuthEnabledguard ensures OAuth controls only appear when appropriate- The
compactprop dynamically adjusts layout based on auth step (compact when showing password form) — nice UX touch- The
OAuthOptionscomponent handles edge cases (filters disabled options, returns null if empty)This maintains clean separation between OAuth configuration (hook) and presentation (component).
apps/admin/core/hooks/oauth/core.tsx (2)
32-67:Core email/password + Google/GitHub modes are wired consistently
disabled/updateConfigare passed through correctly to the configuration components, and the icon setup (including theme-sensitive GitHub logo selection) is coherent with the types. No functional issues stand out here.
82-105:OIDC/SAML/LDAP gating and LDAP addition look aligned with the new modelUsing
UpgradeButtonplusunavailable: truefor OIDC/SAML and the new LDAP mode fits the unifiedTInstanceAuthenticationModesshape, and the LDAP entry is correctly added to the map with aninstance-level upgrade CTA. The icons, descriptions, and keys all look consistent with the rest of the configuration.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/types/src/instance/auth-ee.ts (1)
1-1:Consider adding documentation for the extensibility pattern.The
nevertype indicates no extended authentication modes in the base build, but this pattern may not be immediately clear to contributors.Add a JSDoc comment to clarify the intent:
+/**+ * Extended authentication mode keys for enterprise builds.+ * In the base build this is `never`, but enterprise editions can override+ * this module to add custom authentication modes.+ */ export type TExtendedInstanceAuthenticationModeKeys = never;packages/types/src/instance/auth.ts (1)
61-72:Consider documenting theenabledfield behavior.The optional
enabledfield inTOAuthOptionlacks documentation about its default behavior when omitted.Add JSDoc to clarify the semantics:
+/**+ * Configuration for an OAuth authentication option.+ * @property enabled - Whether the option is enabled. If omitted, defaults to true.+ */ export type TOAuthOption = { id: string; text: string; icon: React.ReactNode; onClick: () => void; enabled?: boolean; };apps/admin/core/hooks/oauth/core.tsx (1)
42-76:Icon sizing inconsistency between authentication modes.The Lucide icons (Mails, KeyRound) use
h-6 w-6classes (24px), while the OAuth provider logos useheight={20} width={20}(20px). This creates a 4px size difference that may cause visual misalignment in the UI.Standardize icon sizing to ensure visual consistency:
google: { key: "google", name: "Google", description: "Allow members to log in or sign up for Plane with their Google accounts.",- icon: <img src={googleLogo} height={20} width={20} alt="Google Logo" />,+ icon: <img src={googleLogo} height={24} width={24} alt="Google Logo" />, config: <GoogleConfiguration disabled={disabled} updateConfig={updateConfig} />, }, github: { key: "github", name: "GitHub", description: "Allow members to log in or sign up for Plane with their GitHub accounts.", icon: ( <img src={resolveGeneralTheme(resolvedTheme) === "dark" ? githubDarkModeImage : githubLightModeImage}- height={20}- width={20}+ height={24}+ width={24} alt="GitHub Logo" /> ), config: <GithubConfiguration disabled={disabled} updateConfig={updateConfig} />, }, gitlab: { key: "gitlab", name: "GitLab", description: "Allow members to log in or sign up to plane with their GitLab accounts.",- icon: <img src={gitlabLogo} height={20} width={20} alt="GitLab Logo" />,+ icon: <img src={gitlabLogo} height={24} width={24} alt="GitLab Logo" />, config: <GitlabConfiguration disabled={disabled} updateConfig={updateConfig} />, }, gitea: { key: "gitea", name: "Gitea", description: "Allow members to log in or sign up to plane with their Gitea accounts.",- icon: <img src={giteaLogo} height={20} width={20} alt="Gitea Logo" />,+ icon: <img src={giteaLogo} height={24} width={24} alt="Gitea Logo" />, config: <GiteaConfiguration disabled={disabled} updateConfig={updateConfig} />, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/admin/app/assets/logos/oidc-logo.svgis excluded by!**/*.svgapps/admin/app/assets/logos/saml-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
apps/admin/ce/components/common/index.ts(0 hunks)apps/admin/ce/components/common/upgrade-button.tsx(0 hunks)apps/admin/core/hooks/oauth/core.tsx(2 hunks)apps/admin/core/hooks/oauth/index.ts(1 hunks)packages/types/src/instance/auth-ee.ts(1 hunks)packages/types/src/instance/auth.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/admin/ce/components/common/index.ts
- apps/admin/ce/components/common/upgrade-button.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/admin/core/hooks/oauth/index.ts (3)
apps/admin/core/hooks/oauth/types.ts (1)
TGetAuthenticationModeProps(3-7)packages/types/src/instance/auth.ts (1)
TInstanceAuthenticationModes(15-22)apps/admin/core/hooks/oauth/core.tsx (1)
getCoreAuthenticationModesMap(20-77)
apps/admin/core/hooks/oauth/core.tsx (5)
packages/types/src/instance/auth.ts (2)
TGetBaseAuthenticationModeProps(55-59)TInstanceAuthenticationModes(15-22)apps/admin/core/components/authentication/email-config-switch.tsx (1)
EmailCodesConfiguration(17-35)apps/admin/core/components/authentication/password-config-switch.tsx (1)
PasswordLoginConfiguration(17-35)apps/admin/core/components/authentication/google-config.tsx (1)
GoogleConfiguration(20-56)apps/admin/core/components/authentication/gitlab-config.tsx (1)
GitlabConfiguration(20-56)
packages/types/src/instance/auth.ts (1)
packages/types/src/instance/auth-ee.ts (1)
TExtendedInstanceAuthenticationModeKeys(1-1)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (4)
packages/types/src/instance/auth.ts (2)
3-13:LGTM! Strong typing for authentication mode keys.The typed union approach improves type safety and enables extensibility for enterprise builds while maintaining a clear set of core authentication modes.
16-16:LGTM! Improved type safety for the key field.Narrowing the
keyfield fromstringtoTInstanceAuthenticationModeKeysprevents invalid authentication mode keys and improves type checking.apps/admin/core/hooks/oauth/core.tsx (2)
20-26:LGTM! Clean refactor to map-based authentication modes.The function signature clearly defines the return type as a keyed record, improving access patterns and type safety compared to array-based approaches.
27-41:LGTM! Email and password authentication modes are well-defined.The configuration components are correctly integrated, and icon sizing is consistent.
| exportconstuseAuthenticationModes=(props:TGetAuthenticationModeProps):TInstanceAuthenticationModes[]=>{ | ||
| // derived values | ||
| constauthenticationModes=getCoreAuthenticationModesMap(props); | ||
| constavailableAuthenticationModes:TInstanceAuthenticationModes[]=[ | ||
| authenticationModes["unique-codes"], | ||
| authenticationModes["passwords-login"], | ||
| authenticationModes["google"], | ||
| authenticationModes["github"], | ||
| authenticationModes["gitlab"], | ||
| authenticationModes["gitea"], | ||
| ]; | ||
| returnavailableAuthenticationModes; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🛠️ Refactor suggestion |🟠 Major
Refactor to derive the array dynamically from the map.
The hardcoded array construction defeats the purpose of the map-based approach and creates a maintenance burden. If new authentication modes are added togetCoreAuthenticationModesMap, this hook must be manually updated.
Apply this diff to derive the array dynamically and add memoization:
+import { useMemo } from "react"; import type { TInstanceAuthenticationModes } from "@plane/types"; import { getCoreAuthenticationModesMap } from "./core"; import type { TGetAuthenticationModeProps } from "./types"; export const useAuthenticationModes = (props: TGetAuthenticationModeProps): TInstanceAuthenticationModes[] => {- // derived values- const authenticationModes = getCoreAuthenticationModesMap(props);-- const availableAuthenticationModes: TInstanceAuthenticationModes[] = [- authenticationModes["unique-codes"],- authenticationModes["passwords-login"],- authenticationModes["google"],- authenticationModes["github"],- authenticationModes["gitlab"],- authenticationModes["gitea"],- ];-- return availableAuthenticationModes;+ return useMemo(() => {+ const authenticationModes = getCoreAuthenticationModesMap(props);+ return Object.values(authenticationModes);+ }, [props]); };
This approach:
- Automatically includes all modes from the map
- Reduces duplication and maintenance overhead
- Adds memoization to prevent unnecessary re-renders
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/admin/core/hooks/oauth/index.ts around lines 5 to 19, the hook currentlybuilds a hardcoded array from the authenticationModes map; replace that with adynamic derivation using Object.values(authenticationModes) (orObject.keys/values + filtering to guarantee TInstanceAuthenticationModes type)and wrap the result with React's useMemo so the array is recomputed only whenthe authenticationModes object changes; add the useMemo import from 'react' andensure the returned value is typed as TInstanceAuthenticationModes[] and filtersout any undefined entries.
Uh oh!
There was an error while loading.Please reload this page.
Description
Type of Change
Summary by CodeRabbit
New Features
Refactoring
UI Change
✏️ Tip: You can customize this high-level summary in your review settings.