[WEB-4542] feat: god mode auth revamp and code refactor #7563
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
WalkthroughThis update introduces new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthHeader
participant FormHeader
participant SignInForm
participant SetupForm
participant FailureView
User->>AuthHeader: Loads authentication page
AuthHeader-->>User: Renders sticky header with logo
User->>SignInForm: Navigates to sign-in
SignInForm->>FormHeader: Renders heading/subheading
SignInForm-->>User: Displays sign-in form
User->>SetupForm: Navigates to setup
SetupForm->>AuthHeader: Renders header
SetupForm->>FormHeader: Renders heading/subheading
SetupForm-->>User: Displays setup form
User->>FailureView: Encounter error
FailureView->>AuthHeader: Renders header
FailureView-->>User: Displays failure message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically 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: 5
🔭 Outside diff range comments (3)
apps/admin/core/components/common/logo-spinner.tsx (1)
1-3: Client component requiredThis component uses a hook (
useTheme) but lacks the "use client" directive.Apply:
+"use client"; + import Image from "next/image"; import { useTheme } from "next-themes"; // assetsapps/space/core/components/common/logo-spinner.tsx (1)
11-16: Harden against theme flicker and improve alt/sizesMirror the admin fix to avoid spinner swap on mount and improve a11y + responsiveness.
import { useTheme } from "next-themes"; +import { useEffect, useState } from "react"; // assets import LogoSpinnerDark from "@/public/images/logo-spinner-dark.gif"; import LogoSpinnerLight from "@/public/images/logo-spinner-light.gif"; export const LogoSpinner = () => { - const { resolvedTheme } = useTheme(); - - const logoSrc = resolvedTheme === "dark" ? LogoSpinnerLight : LogoSpinnerDark; + const { resolvedTheme } = useTheme(); + const [mounted, setMounted] = useState(false); + useEffect(() => setMounted(true), []); + const logoSrc = !mounted + ? LogoSpinnerDark + : resolvedTheme === "dark" + ? LogoSpinnerLight + : LogoSpinnerDark; return ( <div className="flex items-center justify-center"> - <Image src={logoSrc} alt="logo" className="h-6 w-auto sm:h-11" /> + <Image src={logoSrc} alt="Plane logo spinner" sizes="(min-width: 640px) 44px, 24px" className="h-6 w-auto sm:h-11" /> </div> ); };apps/admin/core/components/instance/setup-form.tsx (1)
63-63: Telemetry flag parsing always resolves to true (logic bug)The
|| trueforcesisTelemetryEnabledParamto always be true, even when the query param is "False".-const isTelemetryEnabledParam = (searchParams.get("is_telemetry_enabled") === "True" ? true : false) || true; +const rawTelemetry = searchParams.get("is_telemetry_enabled"); +const isTelemetryEnabledParam = rawTelemetry === null ? true : rawTelemetry === "True";
🧹 Nitpick comments (12)
apps/admin/core/components/instance/instance-not-ready.tsx (2)
15-15: Improve alt text for accessibilityMake the alt text descriptive of the image content or use empty alt if purely decorative.
- <Image src={PlaneTakeOffImage} alt="Plane Logo" /> + <Image src={PlaneTakeOffImage} alt="Plane taking off illustration" />
3-3: Optional: drop React.FC for simplicity and consistencyReact.FC adds little value here and may conflict with implicit children conventions. You can remove the type and import.
-import { FC } from "react"; +// (no FC import) -export const InstanceNotReady: FC = () => ( +export const InstanceNotReady = () => (Also applies to: 10-10
apps/admin/core/components/common/logo-spinner.tsx (1)
14-15: Alt text and responsive hintUse a more descriptive alt and include sizes for better image selection.
- <Image src={logoSrc} alt="logo" className="h-6 w-auto sm:h-11" /> + <Image src={logoSrc} alt="Plane logo spinner" sizes="(min-width: 640px) 44px, 24px" className="h-6 w-auto sm:h-11" />apps/admin/core/components/instance/form-header.tsx (1)
3-7: Improve semantics, flexibility, and typings
- Use semantic headings for a11y.
- Allow ReactNode for localization/formatting.
- Extract props type for readability.
-export const FormHeader = ({ heading, subHeading }: { heading: string; subHeading: string }) => ( - <div className="flex flex-col gap-1"> - <span className="text-2xl font-semibold text-custom-text-100 leading-7">{heading}</span> - <span className="text-lg font-semibold text-custom-text-400 leading-7">{subHeading}</span> - </div> -); +type FormHeaderProps = { heading: React.ReactNode; subHeading: React.ReactNode; as?: keyof JSX.IntrinsicElements }; +export const FormHeader = ({ heading, subHeading, as: HeadingTag = "h1" }: FormHeaderProps) => ( + <div className="flex flex-col gap-1"> + <HeadingTag className="text-2xl font-semibold text-custom-text-100 leading-7">{heading}</HeadingTag> + <p className="text-lg font-semibold text-custom-text-400 leading-7">{subHeading}</p> + </div> +);apps/admin/app/(all)/(home)/auth-header.tsx (1)
6-11: Use semantic header, add background/z-index, and label the linkImproves a11y and ensures the sticky header doesn’t visually merge with scrolled content.
-export const AuthHeader = () => ( - <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-custom-text-100" /> - </Link> - </div> -); +export const AuthHeader = () => ( + <header className="sticky top-0 z-20 w-full flex-shrink-0 bg-custom-background-100 flex items-center justify-between gap-6"> + <Link href="/" aria-label="Go to home"> + <PlaneLockup height={20} width={95} className="text-custom-text-100" /> + </Link> + </header> +);apps/admin/app/(all)/(home)/layout.tsx (1)
5-7: Prefer dvh/min-h and semantic mainAvoids mobile viewport issues and improves semantics.
- <div className="relative z-10 flex flex-col items-center w-screen h-screen overflow-hidden overflow-y-auto pt-6 pb-10 px-8"> - {children} - </div> + <main className="relative z-10 flex flex-col items-center w-full min-h-dvh overflow-hidden overflow-y-auto pt-6 pb-10 px-8"> + {children} + </main>If dvh support is a concern, fallback to
min-h-screen.apps/admin/core/components/instance/loading.tsx (1)
10-15: Prevent theme flicker and improve a11y on the loading spinnerresolvedTheme can be undefined briefly; this may flash the wrong spinner. Also add semantic status and a clearer alt.
export const InstanceLoading = () => { const { resolvedTheme } = useTheme(); - const logoSrc = resolvedTheme === "dark" ? LogoSpinnerLight : LogoSpinnerDark; + const isThemeResolved = resolvedTheme === "dark" || resolvedTheme === "light"; + const logoSrc = resolvedTheme === "dark" ? LogoSpinnerLight : LogoSpinnerDark; return ( - <div className="flex items-center justify-center"> - <Image src={logoSrc} alt="logo" className="h-6 w-auto sm:h-11" /> + <div className="flex items-center justify-center" role="status" aria-label="Loading instance"> + {isThemeResolved && ( + <Image src={logoSrc} alt="Plane logo spinner" className="h-6 w-auto sm:h-11" /> + )} </div> );apps/admin/core/components/instance/failure.tsx (1)
12-12: Drop unnecessary MobX observer wrapperThis component doesn’t consume observable state. Wrapping with observer adds overhead without benefit.
-import { observer } from "mobx-react"; +// import { observer } from "mobx-react"; ... -export const InstanceFailureView: FC = observer(() => { +export const InstanceFailureView: FC = () => { ... -}); +};apps/admin/app/(all)/(home)/sign-in-form.tsx (3)
13-13: Unify import paths with alias for consistencyUse the alias path like other files to keep imports consistent.
-import { FormHeader } from "../../../core/components/instance/form-header"; +import { FormHeader } from "@/components/instance/form-header";
135-146: Prefer semantic autocomplete and required on emailUse autocomplete tokens and required for better UX and password manager behavior.
<Input className="w-full border border-custom-border-100 !bg-custom-background-100 placeholder:text-custom-text-400" id="email" name="email" type="email" inputSize="md" placeholder="[email protected]" value={formData.email} onChange={(e) => handleFormChange("email", e.target.value)} - autoComplete="on" + autoComplete="email" + required autoFocus />
153-163: Improve password field UX: autocomplete and a11y for toggleUse autocomplete="current-password" and add accessible labels to the toggle buttons.
<Input className="w-full border border-custom-border-100 !bg-custom-background-100 placeholder:text-custom-text-400" id="password" name="password" type={showPassword ? "text" : "password"} inputSize="md" placeholder="Enter your password" value={formData.password} onChange={(e) => handleFormChange("password", e.target.value)} - autoComplete="on" + autoComplete="current-password" + required /> {showPassword ? ( <button type="button" className="absolute right-3 top-3.5 flex items-center justify-center text-custom-text-400" onClick={() => setShowPassword(false)} + aria-label="Hide password" > <EyeOff className="h-4 w-4" /> </button> ) : ( <button type="button" className="absolute right-3 top-3.5 flex items-center justify-center text-custom-text-400" onClick={() => setShowPassword(true)} + aria-label="Show password" > <Eye className="h-4 w-4" /> </button> )}Also applies to: 165-180
apps/admin/core/components/instance/setup-form.tsx (1)
165-175: Use proper autocomplete tokens and required on required fieldsImprove autofill and accessibility by using semantic tokens and required on starred fields.
<Input ... - autoComplete="on" + autoComplete="given-name" + required ... <Input ... - autoComplete="on" + autoComplete="family-name" + required ... <Input ... - autoComplete="on" + autoComplete="email" + required ... <Input ... - autoComplete="on" + autoComplete="new-password" + required ... <Input ... - onFocus={() => setIsRetryPasswordInputFocused(true)} - onBlur={() => setIsRetryPasswordInputFocused(false)} + onFocus={() => setIsRetryPasswordInputFocused(true)} + onBlur={() => setIsRetryPasswordInputFocused(false)} + autoComplete="new-password" + requiredAlso applies to: 173-175, 191-191, 209-210, 249-250, 291-293
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apps/admin/public/images/logo-spinner-dark.gifis excluded by!**/*.gifapps/admin/public/images/logo-spinner-light.gifis excluded by!**/*.gifapps/admin/public/plane-logos/black-horizontal-with-blue-logo.pngis excluded by!**/*.pngapps/admin/public/plane-logos/blue-without-text.pngis excluded by!**/*.pngapps/admin/public/plane-logos/white-horizontal-with-blue-logo.pngis excluded by!**/*.png
📒 Files selected for processing (12)
apps/admin/app/(all)/(home)/auth-header.tsx(1 hunks)apps/admin/app/(all)/(home)/layout.tsx(1 hunks)apps/admin/app/(all)/(home)/page.tsx(2 hunks)apps/admin/app/(all)/(home)/sign-in-form.tsx(2 hunks)apps/admin/core/components/common/logo-spinner.tsx(1 hunks)apps/admin/core/components/instance/failure.tsx(2 hunks)apps/admin/core/components/instance/form-header.tsx(1 hunks)apps/admin/core/components/instance/instance-not-ready.tsx(1 hunks)apps/admin/core/components/instance/loading.tsx(1 hunks)apps/admin/core/components/instance/setup-form.tsx(2 hunks)apps/admin/styles/globals.css(0 hunks)apps/space/core/components/common/logo-spinner.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/admin/styles/globals.css
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-04T16:22:44.344Z
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
Applied to files:
apps/admin/core/components/instance/instance-not-ready.tsxapps/admin/app/(all)/(home)/page.tsx
📚 Learning: 2024-11-28T07:02:54.664Z
Learnt from: mathalav55
PR: makeplane/plane#6107
File: web/ce/components/workspace-notifications/sidebar/notification-card/options/archive.tsx:11-14
Timestamp: 2024-11-28T07:02:54.664Z
Learning: When components are still located in `core`, it's appropriate for files to import them using `@/components/...`, and the migration to the new import paths is not necessary in such cases.
Applied to files:
apps/admin/app/(all)/(home)/layout.tsx
📚 Learning: 2025-05-14T13:16:23.323Z
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/admin/app/(all)/(home)/auth-header.tsx
📚 Learning: 2025-06-18T09:46:08.566Z
Learnt from: prateekshourya29
PR: makeplane/plane#7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
Applied to files:
apps/admin/app/(all)/(home)/page.tsx
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshita
PR: makeplane/plane#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)/(home)/sign-in-form.tsxapps/admin/core/components/instance/setup-form.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). (4)
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
apps/admin/core/components/instance/instance-not-ready.tsx (2)
16-16: Token migration LGTMSwitch to text-custom-text-400 aligns with the removal of onboarding-specific tokens. Looks good.
22-26: Avoid nested interactive elements—use router for navigation
To fix the<button>inside<a>a11y violation, drop theLinkwrapper and navigate programmatically:• File
apps/admin/core/components/instance/instance-not-ready.tsx
- At the very top (if not already present), add:
"use client"; import { useRouter } from "next/navigation";- Inside your component, initialize the router:
const router = useRouter();- Replace the Link/Button block (around lines 22–26):
- <Link href="/setup/?auth_enabled=0"> - <Button size="lg" className="w-full"> - Get started - </Button> - </Link> + <Button + size="lg" + className="w-full" + onClick={() => router.push("/setup/?auth_enabled=0")} + > + Get started + </Button>Ensure the
/setup/?auth_enabled=0route or page exists in your Next.js app.apps/admin/core/components/common/logo-spinner.tsx (1)
10-10: Theme inversion looks consistentThe dark theme using the light spinner matches the PR-wide branding change. No functional issues here.
apps/space/core/components/common/logo-spinner.tsx (1)
11-11: Theme inversion acknowledgedChange aligns with admin app for consistent branding.
apps/admin/app/(all)/(home)/page.tsx (1)
20-22: LGTM: simpler, consistent state renderingThe streamlined returns (spinner/failure/setup/sign-in) improve readability and align with the new headers and layouts.
Also applies to: 28-28, 33-33, 37-37
gakshita
approved these changes
Aug 8, 2025
sriramveeraghanta
approved these changes
Aug 11, 2025
sriramveeraghanta
deleted the
feat-god-mode-auth-revamp-and-code-refactor
branch
1 task
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
This PR includes the following updates:
Type of Change
Summary by CodeRabbit
New Features
Refactor
Style