feat: profile page revamp#1484
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughUpgrades React and React-DOM from v18 to v19 in client-demo, introduces new SDK React components (ImageUpload, ViewContainer, ViewHeader) and views (GeneralView, PreferencesView, DeleteOrganizationDialog), adds corresponding CSS modules and design tokens, integrates these into client-demo with new organization settings routes and UI, and updates package dependencies and SDK exports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
web/apps/client-demo/package.json (1)
22-22: Consider removing@types/react-router-dom.
react-router-domv7+ includes built-in TypeScript definitions. The@types/react-router-dompackage (v5.3.3) is for older versions and is unnecessary here—it may even cause type conflicts.♻️ Suggested removal
"devDependencies": { "@types/node": "^22.10.0", "@types/react": "^19.2.2", "@types/react-dom": "^19.2.2", - "@types/react-router-dom": "^5.3.3", "@types/uuid": "^10.0.0",web/sdk/tsup.config.ts (1)
41-43: Consider externalizingreact-domfor Hooks APIs for consistency, but it's not currently required.The React APIs build (line 25) externalizes
react-domalongsidereact, but the Hooks APIs build (line 42) only externalizesreact. However, analysis shows the hooks code currently has noreact-domimports, so there's no actual risk of bundling duplicate instances. Adding'react-dom'to the Hooks APIsexternallist (line 42) would be a consistency measure rather than addressing an existing issue.web/apps/client-demo/src/pages/Home.tsx (1)
63-68: Preferorg.idfor the settings URL and test-id key.Using
org.namemakes this path/selector more fragile (renames/encoding).org.idis the safer canonical identifier.♻️ Suggested change
- <Link - to={`/${org.name}/settings`} - data-test-id={`[organization-new-ui-link-${org.name}]`} - > + <Link + to={`/${org.id}/settings`} + data-test-id={`[organization-new-ui-link-${org.id}]`} + > {org.title} (NEW UI) </Link>web/sdk/react/views-new/general/general-view.tsx (1)
286-293: Make the delete opener an explicit non-submit button.If
Buttonkeeps native button semantics, omittingtypehere turns the delete opener into a submit control because it sits inside the update form.type="button"avoids accidentally submitting the form when the dialog is opened.Minimal fix
<Button + type="button" variant="solid" color="danger" onClick={() => setShowDeleteDialog(true)} disabled={!canDeleteWorkspace}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5c09c1c-9df2-4153-ac28-26648adadc75
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
web/apps/client-demo/package.jsonweb/apps/client-demo/src/App.tsxweb/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Home.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/General.tsxweb/apps/client-demo/src/pages/settings/Preferences.tsxweb/apps/client-demo/src/styles.cssweb/sdk/admin/components/SheetFooter.tsxweb/sdk/package.jsonweb/sdk/react/components/image-upload/image-upload.module.cssweb/sdk/react/components/image-upload/image-upload.tsxweb/sdk/react/components/image-upload/index.tsweb/sdk/react/components/view-container/index.tsweb/sdk/react/components/view-container/view-container.module.cssweb/sdk/react/components/view-container/view-container.tsxweb/sdk/react/components/view-header/index.tsweb/sdk/react/components/view-header/view-header.tsxweb/sdk/react/index.tsweb/sdk/react/views-new/general/components/delete-organization-dialog.module.cssweb/sdk/react/views-new/general/components/delete-organization-dialog.tsxweb/sdk/react/views-new/general/general-view.module.cssweb/sdk/react/views-new/general/general-view.tsxweb/sdk/react/views-new/general/index.tsweb/sdk/react/views-new/preferences/components/preference-row.tsxweb/sdk/react/views-new/preferences/components/preferences-row.module.cssweb/sdk/react/views-new/preferences/index.tsweb/sdk/react/views-new/preferences/preferences-view.module.cssweb/sdk/react/views-new/preferences/preferences-view.tsxweb/sdk/tsup.config.ts
| useEffect(() => { | ||
| if (!orgId || organizations.length === 0) return; | ||
| const org = organizations.find(_org => _org.id === orgId || _org.name === orgId); | ||
| if (org && activeOrganization?.id !== org.id) { | ||
| setActiveOrganization(org); | ||
| } | ||
| }, [orgId, organizations, activeOrganization?.id, setActiveOrganization]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n web/apps/client-demo/src/pages/Settings.tsx | head -100Repository: raystack/frontier
Length of output: 2649
Synchronize the organization before rendering nested settings.
activeOrganization is only corrected after this effect runs, and when orgId has no match in the organizations list it is never corrected at all. This lets nested settings routes render the previous organization's data for one paint—or indefinitely if the URL contains an invalid orgId. Check for a valid organization match first, and only render <Outlet /> once the context is in sync or after redirecting invalid orgId values.
| <Route path="/:orgId/settings" element={<Settings />}> | ||
| <Route path="general" element={<General />} /> | ||
| <Route path="preferences" element={<Preferences />} /> | ||
| </Route> |
There was a problem hiding this comment.
Protect the new settings route with the same auth guard pattern used elsewhere.
This route currently depends on Settings for enforcement, and the provided web/apps/client-demo/src/pages/Settings.tsx snippet has no auth redirect. Please gate /:orgId/settings to prevent unauthenticated access to the settings shell.
| @@ -0,0 +1,231 @@ | |||
| 'use client'; | |||
|
|
|||
| import { useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "createObjectURL|revokeObjectURL" web/sdk/react/components/image-upload/image-upload.tsxRepository: raystack/frontier
Length of output: 114
🏁 Script executed:
cat -n web/sdk/react/components/image-upload/image-upload.tsxRepository: raystack/frontier
Length of output: 7562
Add cleanup for blob URLs to prevent memory leaks across repeated uploads.
Line 180 creates a blob URL with URL.createObjectURL(), but there is no corresponding cleanup. When a new file is selected, the dialog closes, or the component unmounts, the previous blob URLs accumulate in memory and are never released.
The cleanup must revoke blob URLs:
- Before creating a new one (when a new file is selected)
- When the component unmounts
Add useEffect to the imports and implement cleanup:
Proposed fix
-import { useRef, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
@@
const inputRef = useRef<HTMLInputElement>(null);
const [imgSrc, setImgSrc] = useState('');
const [showCropDialog, setShowCropDialog] = useState(false);
+
+ useEffect(() => {
+ return () => {
+ if (imgSrc.startsWith('blob:')) URL.revokeObjectURL(imgSrc);
+ };
+ }, [imgSrc]);
@@
function onFileChange(e: ChangeEvent<HTMLInputElement>) {
const files = e.target.files || [];
if (files.length > 0) {
const file = files[0];
const imageUrl = URL.createObjectURL(file);
- setImgSrc(imageUrl);
+ setImgSrc(prev => {
+ if (prev.startsWith('blob:')) URL.revokeObjectURL(prev);
+ return imageUrl;
+ });
setShowCropDialog(true);
e.target.files = null;
}
}Also applies to: 169-170, 176-184, 223-227
| const height = ((crop?.height || 0) * image.height) / 100; | ||
| const width = ((crop?.width || 0) * image.width) / 100; | ||
| const x = ((crop?.x || 0) * image.width) / 100; | ||
| const y = ((crop?.y || 0) * image.width) / 100; | ||
|
|
There was a problem hiding this comment.
Fix Y-axis crop math (wrong dimension used).
Line 62 computes y using image.width; it must use image.height. Current code can shift crop selection vertically and produce incorrect output.
🐛 Proposed fix
- const y = ((crop?.y || 0) * image.width) / 100;
+ const y = ((crop?.y || 0) * image.height) / 100;📝 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 height = ((crop?.height || 0) * image.height) / 100; | |
| const width = ((crop?.width || 0) * image.width) / 100; | |
| const x = ((crop?.x || 0) * image.width) / 100; | |
| const y = ((crop?.y || 0) * image.width) / 100; | |
| const height = ((crop?.height || 0) * image.height) / 100; | |
| const width = ((crop?.width || 0) * image.width) / 100; | |
| const x = ((crop?.x || 0) * image.width) / 100; | |
| const y = ((crop?.y || 0) * image.height) / 100; |
| @@ -0,0 +1,149 @@ | |||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd delete-organization-dialog.tsxRepository: raystack/frontier
Length of output: 193
🏁 Script executed:
cat -n web/sdk/react/views-new/general/components/delete-organization-dialog.tsxRepository: raystack/frontier
Length of output: 5602
Reset the confirmation state between dialog sessions.
This component stays mounted while open toggles, so the typed organization name and acknowledgment checkbox survive close/reopen. This weakens the confirmation flow for a destructive action. Additionally, after a successful deletion, the dialog remains open if onDeleteSuccess is not supplied, since onOpenChange(false) is never called.
Fix: Import useEffect, extract the reset function from useForm, and add an effect that resets both the form and isAcknowledged when the dialog closes. Also call onOpenChange(false) after successful deletion:
Minimal fix
-import { useState } from 'react';
+import { useEffect, useState } from 'react';
@@
const {
register,
handleSubmit,
+ reset,
watch,
setError,
formState: { errors, isSubmitting }
} = useForm({
resolver: yupResolver(deleteOrgSchema)
});
+
+ useEffect(() => {
+ if (!open) {
+ reset({ title: '' });
+ setIsAcknowledged(false);
+ }
+ }, [open, reset]);
@@
await deleteOrganization(req);
+ onOpenChange(false);
toastManager.add({
title: `${orgLabel} deleted`,
type: 'success'
});| async function onDeleteSubmit(data: { title?: string }) { | ||
| if (!organization?.id) return; | ||
| if (data.title !== organization.title) { | ||
| setError('title', { | ||
| message: `The ${orgLabelLower} name does not match` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const req = create(DeleteOrganizationRequestSchema, { | ||
| id: organization.id | ||
| }); | ||
| await deleteOrganization(req); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "delete-organization-dialog.tsx" | head -5Repository: raystack/frontier
Length of output: 197
🏁 Script executed:
cat -n "web/sdk/react/views-new/general/components/delete-organization-dialog.tsx" | head -150Repository: raystack/frontier
Length of output: 5602
🏁 Script executed:
wc -l "web/sdk/react/views-new/general/components/delete-organization-dialog.tsx"Repository: raystack/frontier
Length of output: 138
🏁 Script executed:
cat -n "web/sdk/react/views-new/general/components/delete-organization-dialog.tsx"Repository: raystack/frontier
Length of output: 5602
Add isAcknowledged check to the submit handler.
The acknowledgment guard only exists on the button's disabled state. Users can bypass this by submitting the form via Enter key in the text input or other non-click submission methods. The form reaches deleteOrganization once the title matches, regardless of the checkbox state. Add a check inside onDeleteSubmit before attempting the delete:
if (!isAcknowledged) return;Alternatively, move the acknowledgment into the form schema so validation is enforced for all submission paths.
| const { mutateAsync: updateOrganization } = useMutation( | ||
| FrontierServiceQueries.updateOrganization, | ||
| { | ||
| onSuccess: data => { | ||
| if (data.organization) { | ||
| setActiveOrganization(data.organization); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: createConnectQueryKey({ | ||
| schema: FrontierServiceQueries.getOrganization, | ||
| transport, | ||
| input: { id: organization?.id || '' }, | ||
| cardinality: 'finite' | ||
| }) | ||
| }); | ||
| } | ||
| toastManager.add({ | ||
| title: `Updated ${t.organization({ case: 'lower' })}`, | ||
| type: 'success' | ||
| }); | ||
| }, | ||
| onError: (error: Error) => { | ||
| toastManager.add({ | ||
| title: 'Something went wrong', | ||
| description: error?.message || 'Failed to update', | ||
| type: 'error' | ||
| }); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its total line count
wc -l web/sdk/react/views-new/general/general-view.tsxRepository: raystack/frontier
Length of output: 113
🏁 Script executed:
# Read the specific lines mentioned in the review comment (99-127)
sed -n '99,127p' web/sdk/react/views-new/general/general-view.tsxRepository: raystack/frontier
Length of output: 945
🏁 Script executed:
# Also check the secondary location mentioned (146-169)
sed -n '146,169p' web/sdk/react/views-new/general/general-view.tsxRepository: raystack/frontier
Length of output: 694
🏁 Script executed:
# Search for the onUpdateSubmit function and its surrounding context
sed -n '70,180p' web/sdk/react/views-new/general/general-view.tsxRepository: raystack/frontier
Length of output: 3013
🏁 Script executed:
# Search for other useMutation patterns in the file to identify the secondary location
rg "useMutation|mutateAsync" web/sdk/react/views-new/general/general-view.tsx -nRepository: raystack/frontier
Length of output: 140
🏁 Script executed:
# Get more context around lines 146-200 to see if there's another mutation
sed -n '130,200p' web/sdk/react/views-new/general/general-view.tsxRepository: raystack/frontier
Length of output: 2153
Remove the catch block to prevent duplicate error toasts.
The mutation's onError handler already displays an error toast when the request fails. The surrounding try-catch block catches the same rejection and displays another toast with an identical message, resulting in two notifications for a single failure. Rely on the mutation's error handler alone.
async function onUpdateSubmit(data: FormData) {
if (!organization?.id) return;
-
- try {
- const req = create(UpdateOrganizationRequestSchema, {
- id: organization.id,
- body: {
- title: data.title,
- name: data.name,
- avatar: data.avatar
- }
- });
- await updateOrganization(req);
- } catch (error: unknown) {
- toastManager.add({
- title: 'Something went wrong',
- description:
- error instanceof Error
- ? error.message
- : `Failed to update ${t.organization({ case: 'lower' })}`,
- type: 'error'
- });
- }
+ const req = create(UpdateOrganizationRequestSchema, {
+ id: organization.id,
+ body: {
+ title: data.title,
+ name: data.name,
+ avatar: data.avatar
+ }
+ });
+ await updateOrganization(req);
}| const { theme, setTheme } = useTheme(); | ||
| const { preferences, isLoading, isFetching, updatePreferences } = | ||
| usePreferences({}); | ||
| console.log(theme); |
There was a problem hiding this comment.
Remove debug logging from render path.
console.log(theme) should not ship in this view.
| <ViewContainer> | ||
| <ViewHeader | ||
| title="Preferences" | ||
| description="Manage members for this domain." |
There was a problem hiding this comment.
Header description copy appears mismatched for this page.
“Manage members for this domain.” reads like a members page, not Preferences.
| <Select defaultValue={theme} onValueChange={setTheme}> | ||
| <Select.Trigger className={styles.selectTrigger}> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @raystack/apsara-v1Select API, is controlled usage withvalue+onValueChangesupported and recommended overdefaultValue for async state?
💡 Result:
Yes, controlled usage with value + onValueChange is supported and recommended over defaultValue for async state management in the @raystack/apsara-v1 Select API.
Citations:
- 1: https://github.com/raystack/apsara
- 2: https://apsara.raystack.org/docs
- 3: https://stackoverflow.com/questions/78321710/shadcn-ui-select-change-state-when-user-selects-item/78321774
- 4: https://stackoverflow.com/questions/44786669/warning-use-the-defaultvalue-or-value-props-on-select-instead-of-setting
- 5: Strange behaviour in Select defaultValue JedWatson/react-select#3483
- 6: https://registry.npmjs.org/%40raystack%2Fapsara
- 7: https://mintlify.com/raystack/apsara/guides/accessibility
- 8: https://www.luisferreiradasilvaarchitects.com
🏁 Script executed:
# Find the file mentioned in the review
find . -name "preferences-view.tsx" -type fRepository: raystack/frontier
Length of output: 119
🏁 Script executed:
# Check the file contents around the specified lines
if [ -f "web/sdk/react/views-new/preferences/preferences-view.tsx" ]; then
wc -l "web/sdk/react/views-new/preferences/preferences-view.tsx"
fiRepository: raystack/frontier
Length of output: 120
🏁 Script executed:
cat -n "web/sdk/react/views-new/preferences/preferences-view.tsx"Repository: raystack/frontier
Length of output: 3501
Replace defaultValue with controlled value prop in Select components to keep UI in sync with async state updates.
Both Select instances use defaultValue, which is initialization-only. The theme value from useTheme() can update externally, and newsletterValue is derived from async preference fetches. This causes stale selections when state changes after mount. Use controlled value prop instead:
Suggested fix
- <Select defaultValue={theme} onValueChange={setTheme}>
+ <Select value={theme} onValueChange={setTheme}>
<Select.Trigger className={styles.selectTrigger}>
<Select.Value placeholder="Theme" />
</Select.Trigger>- <Select
- defaultValue={newsletterValue}
+ <Select
+ value={newsletterValue}
onValueChange={value => {
updatePreferences([
{ name: PREFERENCE_OPTIONS.NEWSLETTER, value }
]);
}}
1 participant
Summary
ProfileViewcomponent inviews-new/profile/using@raystack/apsara-v1primitivesImageUpload, form fields for full name and (read-only) email, and an Update button with dirty-state guarduseFrontier,useMutation,FrontierServiceQueries) — no business logic rewrittenViewContainer/ViewHeaderlayout pattern established by the general and preferences revamps/:orgId/settings/profileand a sidebar nav item