[WEB-5798] refactor: web and admin auth related components and update admin designs #8431
Conversation
📝 WalkthroughWalkthroughIntroduces a PageWrapper layout across many admin pages, replaces inline OAuth/OAuth-config construction with hook-driven config (core + extended), reorganizes providers and sidebar/menu hooks, adds header label mappings and PageWrapper component, and applies numerous presentational class and minor API surface adjustments (type additions, removed barrel exports). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/auth-screens/header.tsx (1)
11-24: Fix inverted page titles inauthContentMap.The
pageTitlevalues are swapped: whentypeisEAuthModes.SIGN_IN(user is on the sign-in page), the page title should be "Sign in", not "Sign up". Similarly, whentypeisEAuthModes.SIGN_UP, the title should be "Sign up", not "Sign in". This causes incorrect browser tab titles.🔎 Proposed fix
const authContentMap = { [EAuthModes.SIGN_IN]: { - pageTitle: "Sign up", + pageTitle: "Sign in", text: "auth.common.new_to_plane", linkText: "Sign up", linkHref: "/sign-up", }, [EAuthModes.SIGN_UP]: { - pageTitle: "Sign in", + pageTitle: "Sign up", text: "auth.common.already_have_an_account", linkText: "Sign in", linkHref: "/sign-in", }, };
🧹 Nitpick comments (17)
apps/web/core/components/account/auth-forms/auth-banner.tsx (1)
10-10: Simplify the callback signature.The parameter name
bannerDatawith typeundefinedis semantically confusing—it suggests data is being passed, but the type only allowsundefined. Since the callback is always invoked withundefinedto clear state, consider one of these alternatives:
- Remove the parameter entirely:
handleBannerData?: () => voidand call it ashandleBannerData?.()- Rename to match intent:
onClose?: () => void- Make it more flexible:
handleBannerData?: (value: TAuthErrorInfo | undefined) => voidif you anticipate future use cases🔎 Proposed refactor to remove the parameter
type TAuthBanner = { message: React.ReactNode; - handleBannerData?: (bannerData: undefined) => void; + handleBannerData?: () => void; };And update the call site on line 31:
- onClick={() => handleBannerData?.(undefined)} + onClick={() => handleBannerData?.()}apps/admin/core/providers/extended.tsx (1)
2-4: Placeholder pattern for future provider extensions.ExtendedProviders currently acts as a passthrough component. This is a common scaffolding pattern for establishing extension points in the provider hierarchy where additional providers can be added later without modifying the core composition.
apps/admin/app/(all)/(dashboard)/sidebar-help-section.tsx (1)
110-110: Inconsistent icon sizing approach.Line 110 still uses explicit dimension classes (
h-3.5 w-3.5) while other icons in this file were updated to use size tokens (e.g.,size-4on lines 71 and 82). Consider updating for consistency.🔎 Proposed fix for consistency
- <Icon className="h-3.5 w-3.5 text-secondary" /> + <Icon className="size-3.5 text-secondary" />apps/admin/core/hooks/use-sidebar-menu/index.ts (1)
5-14: Consider usingas const satisfiesfor stricter type inference.While the current implementation is correct, you could leverage TypeScript 5.0+ features for more precise type inference:
export function useSidebarMenu(): TSidebarMenuItem[] { return [ coreSidebarMenuLinks.general, coreSidebarMenuLinks.email, coreSidebarMenuLinks.authentication, coreSidebarMenuLinks.workspace, coreSidebarMenuLinks.ai, coreSidebarMenuLinks.image, ] as const satisfies readonly TSidebarMenuItem[]; }This ensures the array is treated as a readonly tuple with exact types, catching any type mismatches at compile time. However, the current implementation is acceptable if the consuming code requires a mutable array.
As per coding guidelines for TypeScript 5.0+.
apps/admin/core/hooks/use-sidebar-menu/core.ts (1)
9-46: Consider usingas constfor more precise literal types.For stricter type inference of the string literals and improved type safety, consider:
export const coreSidebarMenuLinks = { general: { Icon: Cog, name: "General", description: "Identify your instances and get key details.", href: `/general/`, }, // ... other entries } as const satisfies Record<TCoreSidebarMenuKey, TSidebarMenuItem>;This would make
name,description, andhrefliteral types rather than juststring, enabling better type checking and autocomplete. However, the current implementation is acceptable if you prefer the flexibility of regular string types.As per coding guidelines for TypeScript 5.0+.
apps/admin/app/(all)/(dashboard)/sidebar-menu.tsx (1)
28-28: Optional chaining may be unnecessary.The
usePathname()hook from Next.js App Router always returns a string (the current pathname), so the optional chaining on line 28 is likely unnecessary:const isActive = item.href === pathName || pathName.includes(item.href);However, the defensive coding doesn't hurt and the current implementation is safe.
apps/web/app/routes/helper.ts (1)
30-33: Optional: RedundantArray.isArraychecks.Since
RouteConfigEntry.childrenis typed asRouteConfigEntry[] | undefined, and the outer condition already confirms both are truthy, theArray.isArraychecks are redundant. The children will always be arrays when defined.🔎 Simplified version
// Deep merge: recursively merge children - const mergedChildren = mergeRoutes( - Array.isArray(coreRoute.children) ? coreRoute.children : [], - Array.isArray(extendedRoute.children) ? extendedRoute.children : [] - ); + const mergedChildren = mergeRoutes(coreRoute.children, extendedRoute.children);apps/admin/core/components/common/page-wrapper.tsx (1)
16-44: LGTM: Well-structured page wrapper component.The component provides a consistent layout container with responsive sizing, optional headers, and scrollable content. The conditional layout logic on Line 31 appropriately handles the presence of actions.
Optional: Consider simplifying the header layout logic
The conditional className on Line 31 could potentially be simplified:
- <div className={header.actions ? "flex flex-col gap-1" : "space-y-1"}> + <div className="space-y-1">Both
flex flex-col gap-1andspace-y-1achieve similar vertical spacing. Usingspace-y-1consistently would simplify the code, though the current approach may have specific alignment requirements when actions are present.apps/space/core/hooks/oauth/extended.tsx (1)
4-7: Consider extracting shared OAuth config pattern.The
useExtendedOAuthConfighook is duplicated in bothapps/spaceandapps/webwith identical implementations. While this pattern provides extensibility, consider extracting it to a shared location if the implementation is intended to remain identical across apps.If app-specific customization is anticipated, the current duplication is acceptable for maintaining independence.
apps/admin/core/hooks/oauth/index.ts (1)
5-19: LGTM! Consider eliminating intermediate variable.The implementation correctly derives authentication modes from the core map and returns them in a specific order. The explicit array construction ensures type safety and makes the selected modes clear.
Optional refactor to eliminate intermediate variable
export const useAuthenticationModes = (props: TGetAuthenticationModeProps): TInstanceAuthenticationModes[] => { // derived values const authenticationModes = getCoreAuthenticationModesMap(props); - const availableAuthenticationModes: TInstanceAuthenticationModes[] = [ + return [ authenticationModes["unique-codes"], authenticationModes["passwords-login"], authenticationModes["google"], authenticationModes["github"], authenticationModes["gitlab"], authenticationModes["gitea"], ]; - - return availableAuthenticationModes; };apps/space/core/hooks/oauth/core.tsx (3)
15-31: Consider memoizing derived values for performance.The
isOAuthEnabledboolean andnext_pathare recomputed on every render. For a frequently rendered auth component, consider wrapping them inuseMemo:🔎 Optional performance optimization
export const useCoreOAuthConfig = (oauthActionText: string): TOAuthConfigs => { //router const searchParams = useSearchParams(); - // query params - const next_path = searchParams.get("next_path"); // theme const { resolvedTheme } = useTheme(); // store hooks const { config } = useInstance(); + + const next_path = useMemo(() => searchParams.get("next_path"), [searchParams]); + // derived values - const isOAuthEnabled = + const isOAuthEnabled = useMemo(() => (config && (config?.is_google_enabled || config?.is_github_enabled || config?.is_gitlab_enabled || config?.is_gitea_enabled)) || - false; + false, [config]);
32-76: Memoize oAuthOptions array to prevent unnecessary re-renders.The
oAuthOptionsarray is reconstructed on every render, creating new object references and function instances. This can cause unnecessary re-renders of child components consuming these options.🔎 Recommended memoization
+ const oAuthOptions: TOAuthOption[] = useMemo(() => [ - const oAuthOptions: TOAuthOption[] = [ { id: "google", text: `${oauthActionText} with Google`, icon: <img src={googleLogo} height={18} width={18} alt="Google Logo" />, onClick: () => { window.location.assign(`${API_BASE_URL}/auth/google/${next_path ? `?next_path=${next_path}` : ``}`); }, enabled: config?.is_google_enabled, }, // ... other providers - ]; + ], [oauthActionText, next_path, config, resolvedTheme]);
45-52: Handle undefined theme during hydration.The
resolvedThemefromnext-themescan beundefinedduring SSR or initial hydration, which defaults to thegithubDarkLogo. Consider adding explicit handling to prevent hydration mismatches:🔎 Safer theme handling
icon: ( <img - src={resolvedTheme === "dark" ? githubLightLogo : githubDarkLogo} + src={resolvedTheme === "dark" ? githubLightLogo : githubDarkLogo} height={18} width={18} alt="GitHub Logo" /> ),Alternatively, provide a fallback:
icon: ( <img - src={resolvedTheme === "dark" ? githubLightLogo : githubDarkLogo} + src={!resolvedTheme || resolvedTheme === "light" ? githubDarkLogo : githubLightLogo} height={18} width={18} alt="GitHub Logo" /> ),apps/web/core/hooks/oauth/core.tsx (3)
15-31: Consider memoizing derived values for performance.Similar to the space implementation,
isOAuthEnabledandnext_pathare recomputed on every render. Consider memoization for better performance in frequently rendered auth flows.🔎 Optional performance optimization
+ const next_path = useMemo(() => searchParams.get("next_path"), [searchParams]); + const isOAuthEnabled = useMemo(() => (config && (config?.is_google_enabled || config?.is_github_enabled || config?.is_gitlab_enabled || config?.is_gitea_enabled)) || - false; + false, [config]);
32-82: Memoize oAuthOptions to prevent unnecessary re-renders.The
oAuthOptionsarray is recreated on every render, which can cause performance issues in child components. ApplyuseMemoto maintain stable references:🔎 Recommended memoization
+ const oAuthOptions: TOAuthOption[] = useMemo(() => [ - const oAuthOptions: TOAuthOption[] = [ { id: "google", text: `${oauthActionText} with Google`, // ... options }, // ... other providers - ]; + ], [oauthActionText, next_path, config, resolvedTheme]);
45-52: Handle undefined theme during hydration.The
resolvedThemecan beundefinedduring initial render, which may cause hydration mismatches. Consider explicit handling:🔎 Safer theme handling
icon: ( <img - src={resolvedTheme === "dark" ? GithubDarkLogo : GithubLightLogo} + src={!resolvedTheme || resolvedTheme === "light" ? GithubLightLogo : GithubDarkLogo} height={18} width={18} alt="GitHub Logo" /> ),apps/admin/app/(all)/(dashboard)/authentication/page.tsx (1)
84-95: Simplify the toggle value parsing logic.The
Boolean(parseInt(enableSignUpConfig))pattern is repeated and verbose. Consider extracting this to a derived variable for clarity and consistency.🔎 Proposed simplification
// derived values const enableSignUpConfig = formattedConfig?.ENABLE_SIGNUP ?? ""; + const isSignUpEnabled = enableSignUpConfig === "1"; const resolvedTheme = resolveGeneralTheme(resolvedThemeAdmin);Then use in the ToggleSwitch:
<ToggleSwitch - value={Boolean(parseInt(enableSignUpConfig))} + value={isSignUpEnabled} onChange={() => { - if (Boolean(parseInt(enableSignUpConfig)) === true) { - updateConfig("ENABLE_SIGNUP", "0"); - } else { - updateConfig("ENABLE_SIGNUP", "1"); - } + updateConfig("ENABLE_SIGNUP", isSignUpEnabled ? "0" : "1"); }} size="sm" disabled={isSubmitting} />
| const enableIntercomConfig = () => { | ||
| submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" }); | ||
| void submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" }); | ||
| }; |
There was a problem hiding this comment.
Good use of void prefix, but missing loading state management.
The void prefix correctly signals that the promise is intentionally ignored, which is good practice for linting rules.
However, there's a pre-existing issue: the isSubmitting state (line 19) is checked on line 73 to disable the toggle, but setIsSubmitting(true) is never called before invoking submitInstanceConfigurations. This means:
- The loading state never displays
- Users can rapidly toggle multiple times, triggering concurrent requests
- The UX lacks feedback during submission
🔎 Proposed fix to add loading state management
const enableIntercomConfig = () => {
+ setIsSubmitting(true);
void submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const enableIntercomConfig = () => { | |
| submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" }); | |
| void submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" }); | |
| }; | |
| const enableIntercomConfig = () => { | |
| setIsSubmitting(true); | |
| void submitInstanceConfigurations({ IS_INTERCOM_ENABLED: isIntercomEnabled ? "0" : "1" }); | |
| }; |
🤖 Prompt for AI Agents
In apps/admin/app/(all)/(dashboard)/general/intercom.tsx around lines 46 to 48,
the submit handler uses void submitInstanceConfigurations but never sets
isSubmitting, so the loading state is never shown and multiple rapid toggles can
fire concurrent requests; update enableIntercomConfig to setIsSubmitting(true)
before calling submitInstanceConfigurations, await the promise (or attach
then/catch) and in a finally block setIsSubmitting(false), and ensure errors are
caught and surfaced (e.g., via existing error handler) so the toggle is disabled
while the request is in-flight and re-enabled afterwards.
| useEffect(() => { | ||
| if (csrfToken === undefined) | ||
| authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token)); | ||
| void authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token)); | ||
| }, [csrfToken]); |
There was a problem hiding this comment.
Add error handling to prevent infinite retry loop.
The promise rejection is not handled, which means if requestCSRFToken() fails, csrfToken remains undefined and the effect will continuously retry on every render, creating an infinite loop of failed requests.
🔎 Proposed fix with error handling
useEffect(() => {
if (csrfToken === undefined)
- void authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token));
+ void authService
+ .requestCSRFToken()
+ .then((data) => {
+ if (data?.csrf_token) {
+ setCsrfToken(data.csrf_token);
+ }
+ })
+ .catch((error) => {
+ console.error("Failed to fetch CSRF token:", error);
+ // Set empty string to prevent infinite retries
+ setCsrfToken("");
+ });
}, [csrfToken]);As per coding guidelines, proper error handling with logging is required for promise-based operations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (csrfToken === undefined) | |
| authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token)); | |
| void authService.requestCSRFToken().then((data) => data?.csrf_token && setCsrfToken(data.csrf_token)); | |
| }, [csrfToken]); | |
| useEffect(() => { | |
| if (csrfToken === undefined) | |
| void authService | |
| .requestCSRFToken() | |
| .then((data) => { | |
| if (data?.csrf_token) { | |
| setCsrfToken(data.csrf_token); | |
| } | |
| }) | |
| .catch((error) => { | |
| console.error("Failed to fetch CSRF token:", error); | |
| // Set empty string to prevent infinite retries | |
| setCsrfToken(""); | |
| }); | |
| }, [csrfToken]); |
🤖 Prompt for AI Agents
In apps/admin/app/(all)/(dashboard)/sidebar-dropdown.tsx around lines 72-75 the
call to authService.requestCSRFToken() has no error handling so a rejection
leaves csrfToken undefined and the effect repeatedly retries; fix by wrapping
the request in an async inner function or attaching .catch(), log the error
(console.error or app logger), and set a sentinel state to stop retries (e.g.,
setCsrfToken(null) or set a separate csrfFailed boolean) so the effect doesn't
loop indefinitely; ensure the effect still only triggers when needed (keep
dependency logic) and avoid swallowing the error.
| })} | ||
| </div> | ||
| <div className="px-2 pb-1 pt-2 text-10">Version: v{packageJson.version}</div> | ||
| <div className="px-2 pb-1 pt-2 text-10">Version: v{instance?.current_version}</div> |
There was a problem hiding this comment.
Handle undefined version gracefully.
The version display uses optional chaining but doesn't handle the undefined case. If instance or current_version is undefined, this will render "vundefined" or "v".
🔎 Proposed fix
- <div className="px-2 pb-1 pt-2 text-10">Version: v{instance?.current_version}</div>
+ <div className="px-2 pb-1 pt-2 text-10">
+ Version: {instance?.current_version ? `v${instance.current_version}` : "N/A"}
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="px-2 pb-1 pt-2 text-10">Version: v{instance?.current_version}</div> | |
| <div className="px-2 pb-1 pt-2 text-10"> | |
| Version: {instance?.current_version ? `v${instance.current_version}` : "N/A"} | |
| </div> |
🤖 Prompt for AI Agents
In apps/admin/app/(all)/(dashboard)/sidebar-help-section.tsx around line 131,
the version line uses optional chaining but will render "vundefined" or "v" when
instance or instance.current_version is missing; change it to handle undefined
by using a safe fallback or conditional render — e.g., compute a display string
like const version = instance?.current_version ? `v${instance.current_version}`
: 'Version unknown' (or render nothing) and use that variable in the div so the
UI shows a sensible fallback instead of "vundefined".
| const lastNameParam = searchParams?.get("last_name") || undefined; | ||
| const companyParam = searchParams?.get("company") || undefined; | ||
| const emailParam = searchParams?.get("email") || undefined; | ||
| const isTelemetryEnabledParam = (searchParams?.get("is_telemetry_enabled") === "True" ? true : false) || true; |
There was a problem hiding this comment.
Critical: Telemetry parameter logic always returns true.
The || true fallback at the end of Line 61 makes isTelemetryEnabledParam always evaluate to true, regardless of the parameter value. This prevents users from opting out of telemetry via the URL parameter.
Current behavior:
?is_telemetry_enabled=False→true(incorrect)?is_telemetry_enabled=True→true(correct)- No parameter →
true(correct as default)
Expected behavior:
?is_telemetry_enabled=False→false?is_telemetry_enabled=True→true- No parameter →
true(default)
🔎 Proposed fix
- const isTelemetryEnabledParam = (searchParams?.get("is_telemetry_enabled") === "True" ? true : false) || true;
+ const isTelemetryEnabledParam = searchParams?.get("is_telemetry_enabled") === "False" ? false : true;This correctly defaults to true when the parameter is absent or undefined, but allows "False" to set it to false.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isTelemetryEnabledParam = (searchParams?.get("is_telemetry_enabled") === "True" ? true : false) || true; | |
| const isTelemetryEnabledParam = searchParams?.get("is_telemetry_enabled") === "False" ? false : true; |
🤖 Prompt for AI Agents
In apps/admin/core/components/instance/setup-form.tsx around line 61, the
current expression ends with "|| true" which forces isTelemetryEnabledParam to
always be true; replace it with logic that returns true when the URL param is
absent but respects explicit "False" values — e.g. read the param value, if it's
null/undefined default to "True" (or return true), otherwise compare the param
string to "True" (optionally case-insensitive) to determine the boolean.
| type TAuthHeaderBase = { | ||
| header: React.ReactNode; | ||
| subHeader: string; | ||
| }; | ||
|
|
||
| export function AuthHeaderBase(props: TAuthHeaderBase) { | ||
| return ( | ||
| <div className="flex flex-col gap-1"> | ||
| <span className="text-h4-semibold text-primary">{typeof header === "string" ? t(header) : header}</span> | ||
| <span className="text-h4-semibold text-placeholder">{subHeader}</span> | ||
| <span className="text-h4-semibold text-primary">{props.header}</span> | ||
| <span className="text-h4-semibold text-placeholder">{props.subHeader}</span> | ||
| </div> | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Naming collision: AuthHeaderBase exported from multiple files with different purposes.
This file exports AuthHeaderBase (lines 111-118) that renders form header content (header + subHeader), while apps/web/core/components/auth-screens/header.tsx also exports AuthHeaderBase (lines 63-76) with a different signature and purpose (page navigation with logo and actions). This creates ambiguity and potential import conflicts.
Consider renaming one or both to clarify their distinct roles:
- This component:
AuthFormHeaderBaseorAuthContentHeader - The other:
AuthPageHeaderBaseorAuthNavigationHeader
🔎 Proposed fix to rename this component
-type TAuthHeaderBase = {
+type TAuthFormHeaderBase = {
header: React.ReactNode;
subHeader: string;
};
-export function AuthHeaderBase(props: TAuthHeaderBase) {
+export function AuthFormHeaderBase(props: TAuthFormHeaderBase) {
return (
<div className="flex flex-col gap-1">
<span className="text-h4-semibold text-primary">{props.header}</span>
<span className="text-h4-semibold text-placeholder">{props.subHeader}</span>
</div>
);
}And update the call site at line 103:
- return <AuthHeaderBase subHeader={subHeader} header={header} />;
+ return <AuthFormHeaderBase subHeader={subHeader} header={header} />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type TAuthHeaderBase = { | |
| header: React.ReactNode; | |
| subHeader: string; | |
| }; | |
| export function AuthHeaderBase(props: TAuthHeaderBase) { | |
| return ( | |
| <div className="flex flex-col gap-1"> | |
| <span className="text-h4-semibold text-primary">{typeof header === "string" ? t(header) : header}</span> | |
| <span className="text-h4-semibold text-placeholder">{subHeader}</span> | |
| <span className="text-h4-semibold text-primary">{props.header}</span> | |
| <span className="text-h4-semibold text-placeholder">{props.subHeader}</span> | |
| </div> | |
| ); | |
| }); | |
| } | |
| type TAuthFormHeaderBase = { | |
| header: React.ReactNode; | |
| subHeader: string; | |
| }; | |
| export function AuthFormHeaderBase(props: TAuthFormHeaderBase) { | |
| return ( | |
| <div className="flex flex-col gap-1"> | |
| <span className="text-h4-semibold text-primary">{props.header}</span> | |
| <span className="text-h4-semibold text-placeholder">{props.subHeader}</span> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/web/core/components/account/auth-forms/auth-header.tsx around lines 106
to 118, the exported component AuthHeaderBase collides with another
AuthHeaderBase elsewhere; rename this component to a clearer name (e.g.,
AuthFormHeaderBase or AuthContentHeader), update the export declaration
accordingly, and update all local call sites (notably the caller at line 103 in
this file) and any imports in other files to use the new name so there is no
ambiguity with apps/web/core/components/auth-screens/header.tsx.
| type TAuthHeaderBase = { | ||
| pageTitle: string; | ||
| additionalAction?: React.ReactNode; | ||
| }; | ||
|
|
||
| export function AuthHeaderBase(props: TAuthHeaderBase) { | ||
| const { pageTitle, additionalAction } = props; | ||
| return ( | ||
| <> | ||
| <PageHead title={pageTitle + " - Plane"} /> | ||
| <div className="flex items-center justify-between gap-6 w-full flex-shrink-0 sticky top-0"> | ||
| <Link href="/"> | ||
| <PlaneLockup height={20} width={95} className="text-primary" /> | ||
| </Link> | ||
| {additionalAction} | ||
| </div> | ||
| </> | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Naming collision: See comment on apps/web/core/components/account/auth-forms/auth-header.tsx.
This AuthHeaderBase export has the same name as a different component exported from apps/web/core/components/account/auth-forms/auth-header.tsx (lines 111-118). Consider renaming this to AuthPageHeaderBase or AuthNavigationHeader to distinguish it from the form-content header.
🤖 Prompt for AI Agents
In apps/web/core/components/auth-screens/header.tsx around lines 58 to 76, the
exported component name AuthHeaderBase collides with another export of the same
name in apps/web/core/components/account/auth-forms/auth-header.tsx; rename this
component (and its type if needed) to a distinct name such as AuthPageHeaderBase
or AuthNavigationHeader, update the function export and any imports/uses across
the codebase to the new name, and run a quick search/replace to ensure there are
no remaining references to the old name.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/admin/app/(all)/(dashboard)/authentication/google/page.tsx (3)
51-58: Consider using try/catch/finally instead of mixing await with .then()/.catch().The current pattern is redundant—
awaitalready handles the promise. Usingtry/catch/finallyis more idiomatic and consolidates the duplicatedsetIsSubmitting(false)call.🔎 Proposed refactor
- await updateConfigPromise - .then(() => { - setIsSubmitting(false); - }) - .catch((err) => { - console.error(err); - setIsSubmitting(false); - }); + try { + await updateConfigPromise; + } catch (err) { + console.error(err); + } finally { + setIsSubmitting(false); + }
65-66: Multiline string may introduce unwanted whitespace.The description string split across lines will include the newline and leading spaces in the rendered text.
🔎 Proposed fix
- description="Allow members to login or sign up to plane with their Google - accounts." + description="Allow members to login or sign up to plane with their Google accounts."
69-77: Simplify toggle logic and add radix to parseInt.The
parseIntcall should include a radix parameter to avoid ambiguity. Additionally, the redundant=== truecomparison and repeated parsing can be simplified.🔎 Proposed refactor
+ const isGoogleEnabled = Boolean(parseInt(enableGoogleConfig, 10)); <ToggleSwitch - value={Boolean(parseInt(enableGoogleConfig))} - onChange={() => { - if (Boolean(parseInt(enableGoogleConfig)) === true) { - updateConfig("IS_GOOGLE_ENABLED", "0"); - } else { - updateConfig("IS_GOOGLE_ENABLED", "1"); - } - }} + value={isGoogleEnabled} + onChange={() => updateConfig("IS_GOOGLE_ENABLED", isGoogleEnabled ? "0" : "1")} size="sm" disabled={isSubmitting || !formattedConfig} />Note: Extract
isGoogleEnabledas a derived variable near line 26 to avoid repetition.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin/app/(all)/(dashboard)/authentication/google/page.tsxapps/admin/core/providers/extended.tsxpackages/editor/src/core/hooks/use-yjs-setup.tspackages/types/src/instance/auth-ee.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/types/src/instance/auth-ee.ts
- apps/admin/core/providers/extended.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/editor/src/core/hooks/use-yjs-setup.tsapps/admin/app/(all)/(dashboard)/authentication/google/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
packages/editor/src/core/hooks/use-yjs-setup.tsapps/admin/app/(all)/(dashboard)/authentication/google/page.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
packages/editor/src/core/hooks/use-yjs-setup.tsapps/admin/app/(all)/(dashboard)/authentication/google/page.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
packages/editor/src/core/hooks/use-yjs-setup.tsapps/admin/app/(all)/(dashboard)/authentication/google/page.tsx
🧠 Learnings (5)
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Use `const` type parameters for more precise literal inference in TypeScript 5.0+
Applied to files:
packages/editor/src/core/hooks/use-yjs-setup.ts
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{js,mjs,cjs} : Use `import` tags in JSDoc for cleaner type imports in JavaScript files when working in a mixed codebase (TypeScript 5.5+)
Applied to files:
packages/editor/src/core/hooks/use-yjs-setup.ts
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Use `with { type: "json" }` for import attributes; avoid deprecated `assert` syntax (TypeScript 5.3/5.8+)
Applied to files:
packages/editor/src/core/hooks/use-yjs-setup.ts
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Use `NoInfer<T>` utility to block inference for specific type arguments when they should be determined by other arguments
Applied to files:
packages/editor/src/core/hooks/use-yjs-setup.ts
📚 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/admin/app/(all)/(dashboard)/authentication/google/page.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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/admin/app/(all)/(dashboard)/authentication/google/page.tsx (1)
61-98: PageWrapper integration looks good.The refactor to use
PageWrapperwithcustomHeaderis clean and maintains the existing functionality while improving layout consistency. The conditional rendering ofInstanceGoogleConfigFormvsLoaderis preserved correctly.packages/editor/src/core/hooks/use-yjs-setup.ts (1)
190-190: Type constraint ensures safety—removal of explicit assertion is correct.The
useStatetype parameter (line 39) explicitly constrainsydoc: Y.Doc, so TypeScript will enforce thatprovider.documentis assignable toY.Docat compile time. This refactoring safely removes the unnecessary assertion.Note: Library type verification for
@hocuspocus/providerv2.15.2 could not be completed in this environment, but the code structure and usage pattern (e.g., passingprovider.documenttoIndexeddbPersistence) are consistent with correct typing.
Description
This PR includes refactoring of auth-related components for both the admin and web applications, as well as some updates to the new design system.
Type of Change
Note
Modernizes admin and web authentication UX and shared infrastructure.
PageWrapperand newAdminHeader(breadcrumbs), standardizes loaders, spacing, and copy (e.g., “Saving...” → “Saving”).useSidebarMenu; tweaks help/user sections and shows instance version.useAuthenticationModes; updates GitHub/GitLab/Google/Gitea pages to useAuthenticationMethodCardand consistent toggles.useOAuthConfighooks (core/extended) to generate provider options; refactors auth screens (AuthRoot, banners/headers) to consume them.searchParamsaccess.AppProviders).Written by Cursor Bugbot for commit 96aa04b. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.