[WEB-5324] refactor: add Unified OAuth Configuration and Missing Gitea Options #8050
+308
−245
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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.
Contributor
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 mode
sequenceDiagram
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 oAuthOptions
Estimated 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 the high-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 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}
Contributor
There was a problem hiding this comment.
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: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T22:12:26.424Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/core/components/new-user-popup.tsx:4-6
Timestamp: 2025-10-09T20:43:07.762Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: 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: lifeiscontent Repo: makeplane/plane PR: 7922 File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19 Timestamp: 2025-10-09T22:12:26.424Z Learning: 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: lifeiscontent Repo: makeplane/plane PR: 7922 File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19 Timestamp: 2025-10-09T20:42:31.843Z Learning: 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: lifeiscontent Repo: makeplane/plane PR: 7888 File: packages/propel/src/avatar/avatar.stories.tsx:2-3 Timestamp: 2025-10-01T15:30:17.605Z Learning: 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.
aaryan610
requested changes
Oct 31, 2025
- 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 by Plane |
Merge branch 'preview' of github.com:makeplane/plane into refactor-oauth-options
Contributor
There was a problem hiding this comment.
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: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: 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: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/core/components/new-user-popup.tsx:4-6
Timestamp: 2025-10-09T20:43:07.762Z
Learning: 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: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: 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.
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+5
to
+19
| 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; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ 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 to getCoreAuthenticationModesMap, 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 currently
builds a hardcoded array from the authenticationModes map; replace that with a
dynamic derivation using Object.values(authenticationModes) (or
Object.keys/values + filtering to guarantee TInstanceAuthenticationModes type)
and wrap the result with React's useMemo so the array is recomputed only when
the authenticationModes object changes; add the useMemo import from 'react' and
ensure the returned value is typed as TInstanceAuthenticationModes[] and filters
out any undefined entries.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Summary by CodeRabbit
New Features
Refactoring
UI Change
✏️ Tip: You can customize this high-level summary in your review settings.