[WEB-5860] style: improved profile settings interface #8518
Conversation
📝 WalkthroughWalkthroughThis PR migrates workspace-scoped account settings into a unified, tab-based profile settings under /settings/profile, removes legacy workspace/account/profile pages and sidebars, adds modal-driven profile settings UI (sidebar + lazy tab content), updates routing/redirects, and introduces related components, types, and store state. Changes
Sequence DiagramsequenceDiagram
participant User
participant TopNav
participant UserMenu
participant Store as CommandPaletteStore
participant GlobalModals
participant ProfileModal
participant Sidebar
participant Content
participant TabComponent
User->>TopNav: open user menu
TopNav->>UserMenu: render menu
User->>UserMenu: click "Settings" / "Preferences"
UserMenu->>Store: toggleProfileSettingsModal({ isOpen: true, activeTab: "general" })
Store->>GlobalModals: state: profileSettingsModal.isOpen = true
GlobalModals->>ProfileModal: render modal
ProfileModal->>Sidebar: render with activeTab
ProfileModal->>Content: render with activeTab
Content->>TabComponent: lazy load component for activeTab
TabComponent->>Content: fetch/display data (user/profile)
User->>Sidebar: click another tab
Sidebar->>Store: toggleProfileSettingsModal({ activeTab: "preferences" })
Store->>Content: update activeTab -> Content re-resolves TabComponent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull request overview
This PR improves the profile settings interface by replacing the separate route-based profile settings with a modal-based approach. This change provides a more modern and accessible user experience by allowing users to access their profile settings from anywhere in the application.
Key Changes:
- Introduced a new profile settings modal accessible via command palette store
- Migrated all profile settings pages (general, preferences, activity, notifications, security, api-tokens) to modal-compatible components
- Added redirects from old routes (
profile/*and:workspaceSlug/settings/account/*) to new routes (settings/profile/*)
Reviewed changes
Copilot reviewed 70 out of 70 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/settings.ts | Added new TProfileSettingsTabs type definition for profile settings tabs |
| packages/constants/src/settings.ts | Updated profile settings constants with proper typing and removed href/highlight properties |
| packages/constants/src/profile.ts | Refactored profile settings structure, removed navigation-specific properties |
| apps/web/core/store/base-command-palette.store.ts | Added profile settings modal state management |
| apps/web/core/components/workspace/sidebar/user-menu-root.tsx | Updated user menu to trigger profile settings modal instead of navigation |
| apps/web/core/components/settings/profile/* | New modal-based profile settings components (modal, sidebar, content pages) |
| apps/web/core/layouts/auth-layout/workspace-wrapper.tsx | Updated profile link to new settings route |
| apps/web/app/routes/redirects/core/* | Added redirect routes for backward compatibility |
| apps/web/app/(all)/settings/profile/* | New route-based profile settings pages |
| apps/web/app/(all)/[workspaceSlug]/layout.tsx | Added GlobalModals component |
| Multiple deleted files | Removed old profile settings components and routes |
Comments suppressed due to low confidence (1)
apps/web/core/components/settings/profile/content/pages/security.tsx:99
- The error handling logic has been improved to properly check if error is an instance of Error before accessing error_code. However, the errorInfo variable is initialized inside a conditional block, which could lead to accessing undefined errorInfo if the error is not an Error instance. Consider initializing errorInfo with a default value or ensuring the toast always has a fallback message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { profileTabId } = useParams(); | ||
| // translation | ||
| const { t } = useTranslation(); | ||
|
|
||
| return ( | ||
| <div className="mt-4 flex flex-col gap-y-4"> | ||
| {PROFILE_SETTINGS_CATEGORIES.map((category) => { | ||
| const categoryItems = GROUPED_PROFILE_SETTINGS[category]; | ||
|
|
||
| if (categoryItems.length === 0) return null; | ||
|
|
||
| return ( | ||
| <div key={category} className="shrink-0"> | ||
| <div className="p-2 text-caption-md-medium text-tertiary capitalize">{t(category)}</div> | ||
| <div className="flex flex-col"> | ||
| {categoryItems.map((item) => ( | ||
| <SettingsSidebarItem | ||
| key={item.key} | ||
| as="button" | ||
| onClick={() => updateActiveTab(item.key)} | ||
| isActive={activeTab === item.key} | ||
| icon={ICONS[item.key]} | ||
| label={t(item.i18n_label)} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| })} | ||
| {profileTabId && <ProfileSettingsSidebarWorkspaceOptions />} |
There was a problem hiding this comment.
The profileTabId parameter is used in the conditional render at line 63 to show/hide ProfileSettingsSidebarWorkspaceOptions, but this parameter is coming from useParams() which pulls from the route. Since this is in a modal context (ProfileSettingsSidebarItemCategories), the profileTabId from route params may not be reliable or expected. Consider passing this as a prop instead or clarifying the intended behavior.
| updateEmptyState: (state: boolean) => void; | ||
| }; | ||
|
|
||
| export const ActivityProfileSettingsList = observer(function ProfileActivityListPage(props: Props) { |
There was a problem hiding this comment.
The function name ActivityProfileSettingsList in the observer wrapper doesn't match the parameter name in the function definition (ProfileActivityListPage). While both refer to the same component, this inconsistency could cause confusion. Consider using consistent naming.
| if (!currentUser || !isAValidTab) | ||
| return ( | ||
| <div className="size-full grid place-items-center px-4"> | ||
| <LogoSpinner /> | ||
| </div> | ||
| ); | ||
|
|
There was a problem hiding this comment.
The component returns null when currentUser is not available or when isAValidTab is false, but there's no loading state differentiation. When the tab is invalid, the user sees a loading spinner which is misleading. Consider showing different UI for invalid tabs vs loading states, or redirecting to a valid tab.
| if (!currentUser || !isAValidTab) | |
| return ( | |
| <div className="size-full grid place-items-center px-4"> | |
| <LogoSpinner /> | |
| </div> | |
| ); | |
| if (!currentUser) | |
| return ( | |
| <div className="size-full grid place-items-center px-4"> | |
| <LogoSpinner /> | |
| </div> | |
| ); | |
| if (!isAValidTab) | |
| return ( | |
| <div className="size-full grid place-items-center px-4"> | |
| <p className="text-muted-foreground">Invalid profile settings tab.</p> | |
| </div> | |
| ); |
| console.error("Error updating theme:", error); | ||
| } | ||
| }, | ||
| [updateUserTheme] |
There was a problem hiding this comment.
The useCallback dependency array includes only updateUserTheme but the callback also uses setTheme. While setTheme from next-themes is stable, for completeness and consistency with React best practices, it should be included in the dependency array.
| [updateUserTheme] | |
| [updateUserTheme, setTheme] |
| export const clientLoader = ({ params, request }: Route.ClientLoaderArgs) => { | ||
| const searchParams = new URL(request.url).searchParams; | ||
| const splat = params["*"] || ""; | ||
| throw redirect(`/settings/profile/${splat || "general"}?${searchParams.toString()}`); |
There was a problem hiding this comment.
The redirect preserves query parameters using searchParams.toString(), but if there are no search parameters, this will append an empty ? to the URL (e.g., /settings/profile/general?). Consider only appending the search params if they exist: const search = searchParams.toString(); throw redirect(\/settings/profile/${splat || "general"}${search ? `?${search}` : ""}`);`
| throw redirect(`/settings/profile/${splat || "general"}?${searchParams.toString()}`); | |
| const search = searchParams.toString(); | |
| throw redirect( | |
| `/settings/profile/${splat || "general"}${search ? `?${search}` : ""}`, | |
| ); |
| export const clientLoader = ({ params, request }: Route.ClientLoaderArgs) => { | ||
| const searchParams = new URL(request.url).searchParams; | ||
| const splat = params["*"] || ""; | ||
| throw redirect(`/settings/profile/${splat || "general"}?${searchParams.toString()}`); |
There was a problem hiding this comment.
Same issue as workspace-account-settings.tsx - the redirect may append an empty ? when there are no search parameters. Consider conditionally appending search params.
| throw redirect(`/settings/profile/${splat || "general"}?${searchParams.toString()}`); | |
| const query = searchParams.toString(); | |
| const target = query | |
| ? `/settings/profile/${splat || "general"}?${query}` | |
| : `/settings/profile/${splat || "general"}`; | |
| throw redirect(target); |
| const isAValidTab = PROFILE_SETTINGS_TABS.includes(profileTabId as TProfileSettingsTabs); | ||
|
|
||
| if (!currentUser || !isAValidTab) |
There was a problem hiding this comment.
The variable isAValidTab uses "isA" prefix which is unconventional. Typically, boolean variables use "is" prefix followed by an adjective/noun. Consider renaming to isValidTab for better readability and consistency with naming conventions.
| const isAValidTab = PROFILE_SETTINGS_TABS.includes(profileTabId as TProfileSettingsTabs); | |
| if (!currentUser || !isAValidTab) | |
| const isValidTab = PROFILE_SETTINGS_TABS.includes(profileTabId as TProfileSettingsTabs); | |
| if (!currentUser || !isValidTab) |
| setTimeout(() => { | ||
| toggleProfileSettingsModal({ | ||
| activeTab: null, | ||
| }); | ||
| }, 300); |
There was a problem hiding this comment.
The setTimeout delay of 300ms is used to reset the activeTab after closing the modal. However, this magic number should be documented or made into a constant explaining why this specific delay is needed (likely to match animation duration). Consider extracting this as a named constant like MODAL_CLOSE_ANIMATION_DURATION = 300.
| updateUserAndProfile | ||
| .then(() => { | ||
| return; | ||
| }) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
The promise is being created but then immediately followed with .then(() => { return; }) which doesn't add any value. The empty return statement inside the then block is redundant. This can be simplified to just updateUserAndProfile.catch(() => {}); or removed entirely since setPromiseToast already handles the promise.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/app/(all)/settings/profile/layout.tsx (1)
7-21: Potential non-scrollable settings content due tooverflow-hiddeneverywhere.With the current nesting, longer profile pages can end up clipped unless every child handles its own scrolling. If this layout is meant to own scrolling, consider making the Outlet wrapper scrollable and adding
min-h-0for correct flex overflow.Suggested adjustment
- <div className="relative flex size-full overflow-hidden bg-canvas p-2"> - <main className="relative flex flex-col size-full overflow-hidden bg-surface-1 rounded-lg border border-subtle"> - <div className="size-full overflow-hidden"> + <div className="relative flex size-full overflow-hidden bg-canvas p-2"> + <main className="relative flex flex-col size-full min-h-0 overflow-hidden bg-surface-1 rounded-lg border border-subtle"> + <div className="size-full min-h-0 overflow-auto"> <Outlet /> </div> </main> </div>apps/web/core/components/profile/sidebar.tsx (1)
66-79: Missing dependency in useEffect may cause stale closure.The effect references
profileSidebarCollapsedandtoggleProfileSidebarbut has an empty dependency array. This can cause thehandleToggleProfileSidebarfunction to capture stale values ofprofileSidebarCollapsed.Proposed fix
useEffect(() => { const handleToggleProfileSidebar = () => { if (window && window.innerWidth < 768) { toggleProfileSidebar(true); } if (window && profileSidebarCollapsed && window.innerWidth >= 768) { toggleProfileSidebar(false); } }; window.addEventListener("resize", handleToggleProfileSidebar); handleToggleProfileSidebar(); return () => window.removeEventListener("resize", handleToggleProfileSidebar); - }, []); + }, [profileSidebarCollapsed, toggleProfileSidebar]);apps/web/core/store/base-command-palette.store.ts (1)
103-116: AddprofileSettingsModal.isOpentogetCoreModalsState().The
profileSettingsModalis part of the command palette store with dedicated toggle functionality, but it's not tracked ingetCoreModalsState(). This method is used to computeisAnyModalOpenwhich determines focus management and modal closing behavior in peek-overview components (e.g., preventing unwanted closure while any modal is open). Without includingprofileSettingsModal.isOpen, the focus management logic may behave incorrectly when the profile settings modal is active.
🤖 Fix all issues with AI agents
In @apps/web/app/routes/redirects/core/api-tokens.tsx:
- Around line 3-4: clientLoader currently throws a hardcoded redirect and drops
query params; change its signature to accept ({ request }:
Route.ClientLoaderArgs) and preserve search parameters by building the target
with the incoming URL's search (e.g., const url = new URL(request.url); throw
redirect(`/settings/profile/api-tokens${url.search}`)); also remove the trailing
slash in the target to match other redirects.
In @apps/web/core/components/profile/sidebar.tsx:
- Line 36: The destructured userId from useParams() is string | string[]; before
the comparison currentUser?.id === userId in this file/component, explicitly
narrow or convert userId to a string (e.g., call .toString() or handle array
case like other profile components) or assert it as string so the types match;
update the comparison to use the normalized string variable (mirroring the
approach in profile-issues-filter.tsx).
In
@apps/web/core/components/settings/profile/content/pages/activity/activity-list.tsx:
- Around line 33-42: The SWR cache key for useSWR only includes cursor but the
fetcher userService.getUserActivity also depends on perPage, causing stale
results when perPage changes; update the USER_ACTIVITY key generator to accept
per_page and call useSWR with USER_ACTIVITY({ cursor, per_page: perPage }) (and
ensure the fetcher call uses the same per_page param), so the key uniquely
reflects both cursor and perPage and invalidates cache correctly for
userProfileActivity.
In @apps/web/core/components/settings/profile/content/pages/general/form.tsx:
- Around line 202-218: The img inside the clickable avatar button is given its
own onClick and role="button", creating nested button semantics; remove the
onClick and role attributes from the img (the parent button already calls
setIsImageUploadModalOpen(true)), leaving the img as a plain image using
src={getFileURL(userAvatar)} and alt={currentUser?.display_name} so clicks are
handled by the outer button and HTML remains valid.
- Around line 146-147: Both updateCurrentUserDetail and updateCurrentUserProfile
attach .finally(() => setIsLoading(false)) causing duplicate state updates when
awaited together; remove the per-promise finally handlers and instead call
setIsLoading(false) once in the finally of the combined Promise.all (or after
awaiting Promise.all of updateCurrentUser(userPayload) and
updateUserProfile(profilePayload)) so setIsLoading is reset only once; update
references: updateCurrentUserDetail, updateCurrentUserProfile,
updateCurrentUser, updateUserProfile, setIsLoading, and Promise.all.
In
@apps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsx:
- Around line 29-46: The toggle is being updated optimistically and not reverted
on API failure; modify handleSettingChange (and each caller that does
onChange(newValue)) to capture the previous value before calling onChange, call
userService.updateCurrentUserEmailNotificationSettings as now, and in the catch
block call onChange(previousValue) (or otherwise restore the original boolean)
so the UI is reverted when updateCurrentUserEmailNotificationSettings fails;
ensure you reference the same key (key: keyof IUserEmailNotificationSettings)
when reverting so the correct toggle is restored.
In @apps/web/core/components/settings/profile/content/pages/security.tsx:
- Around line 12-13: The import for SettingsHeading is using default-import
syntax but SettingsHeading is a named export; change the import statement to use
a named import (e.g., import { SettingsHeading } from
"@/components/settings/heading") so the component resolves correctly; update the
import line at the top of the file where SettingsHeading is currently imported
and verify usages of SettingsHeading remain unchanged.
In @apps/web/core/components/settings/profile/content/root.tsx:
- Around line 26-28: The Suspense wrapper around PageComponent is missing a
fallback, causing a blank area during lazy loading; update the Suspense usage to
provide a fallback UI (e.g., a spinner or Skeleton component) by supplying the
fallback prop to Suspense so that a loading indicator is rendered while
PageComponent loads, and reuse or create a lightweight Loading/Skeleton
component if needed to keep UX consistent.
In @apps/web/core/components/settings/profile/sidebar/header.tsx:
- Around line 8-30: ProfileSettingsSidebarHeader currently calls
getFileURL(currentUser?.avatar_url ?? "") and renders names via
{currentUser?.first_name} {currentUser?.last_name}, which can call
getFileURL("") and show "undefined undefined"; change to pass undefined to
getFileURL when no avatar (e.g., getFileURL(currentUser?.avatar_url)) and render
a safe name fallback using display_name or a single-field fallback (e.g., use
currentUser?.display_name ?? currentUser?.first_name ?? currentUser?.last_name
or join available parts) so Avatar receives a proper src/name and the UI never
shows "undefined undefined".
In @apps/web/core/components/settings/profile/sidebar/workspace-options.tsx:
- Around line 21-29: The workspace items always pass isActive={false} so the
current workspace is never highlighted; update the mapping in
workspace-options.tsx to determine isActive by comparing each workspace.slug to
the active workspace slug (e.g., accept a workspaceSlug prop on the component or
read it from the store/hook used by the app) and pass isActive={workspace.slug
=== workspaceSlug} into SettingsSidebarItem (keep the same keys and props like
iconNode, href, label). Ensure workspaceSlug is provided to this component or
obtained via the appropriate hook before the map.
In @apps/web/core/components/workspace/sidebar/user-menu-root.tsx:
- Around line 117-128: The "Preferences" menu item is hardcoded; update the
CustomMenu.MenuItem that calls toggleProfileSettingsModal (activeTab:
"preferences") to use the translation function t() instead of the literal
"Preferences" so it matches other items (e.g., t("settings"), t("sign_out")) —
replace the string "Preferences" with t("preferences") (or the correct key used
in your i18n files) inside the MenuItem rendering.
- Around line 29-30: Replace the hardcoded const isUserInstanceAdmin = false
with a real call to the existing API/service: call
currentUserInstanceAdminStatus() (or wire it into the user store) and use its
boolean result for isUserInstanceAdmin; fetch the status asynchronously (e.g.,
useEffect or the store's initialization) and handle loading/errors so the God
Mode menu item visibility in user-menu-root.tsx is driven by the returned
boolean rather than a constant.
🧹 Nitpick comments (19)
apps/web/core/components/home/widgets/empty-states/no-projects.tsx (1)
87-113: CTA link update looks right; verify icon link flow and consider removing unnecessary template literal
link: \/settings/profile/general`matches the move to global profile settings, but the avatar/namestill targets/${workspaceSlug}/profile/${currentUser?.id}`—please confirm that’s still intended (or that the redirect lands users in the same new modal/settings experience). Also, since there’s no interpolation, prefer a plain string.Proposed tweak
- link: `/settings/profile/general`, + link: "/settings/profile/general",packages/types/src/settings.ts (1)
1-1: Consider deriving the union from an exported const list to avoid drift (type vs runtime).If these tab ids are also used to render UI or validate params, having a single runtime source-of-truth helps.
Proposed refactor
-export type TProfileSettingsTabs = "general" | "preferences" | "activity" | "notifications" | "security" | "api-tokens"; +export const PROFILE_SETTINGS_TABS = [ + "general", + "preferences", + "activity", + "notifications", + "security", + "api-tokens", +] as const; + +export type TProfileSettingsTabs = (typeof PROFILE_SETTINGS_TABS)[number];apps/web/core/components/global/timezone-select.tsx (1)
34-49: Consider deterministic border class selection for clarity.The
placement="bottom-end"prop is valid and fully supported byCustomSearchSelect. For better readability, use a ternary operator to explicitly select between border colors rather than conditionally adding classes:Suggested refactoring
- buttonClassName={cn(buttonClassName, "border border-subtle-1", { - "border-danger-strong": error, - })} + buttonClassName={cn( + "border", + error ? "border-danger-strong" : "border-subtle-1", + buttonClassName + )}apps/web/core/components/settings/sidebar/item.tsx (2)
1-6: Import organization doesn't match the comment grouping.The
// plane importscomment on line 5 suggests grouping, butcnfrom@plane/utilsappears before the type imports fromlucide-reactand@plane/propel/icons. Consider reordering for consistency.Suggested import order
import React from "react"; import Link from "next/link"; +import type { LucideIcon } from "lucide-react"; // plane imports +import type { ISvgIcons } from "@plane/propel/icons"; import { cn } from "@plane/utils"; -import type { LucideIcon } from "lucide-react"; -import type { ISvgIcons } from "@plane/propel/icons";
30-39: Inconsistent icon wrapper betweeniconandiconNodebranches.When using the
iconprop, the icon is wrapped in asize-4container with centering. However, when usingiconNode, it's rendered directly without any wrapper, which may cause visual inconsistency.Proposed fix for consistent icon rendering
const content = ( <> {"icon" in props ? ( <span className="shrink-0 size-4 grid place-items-center">{<props.icon className="size-3.5" />}</span> ) : ( - props.iconNode + <span className="shrink-0 size-4 grid place-items-center">{props.iconNode}</span> )} <span className="truncate">{label}</span> </> );apps/web/core/components/settings/profile/content/pages/general/form.tsx (3)
27-39: Avoid usinganytype forcover_image_asset.The
cover_image_assetfield is typed asany, which bypasses TypeScript's type checking. Consider using a more specific type orunknownif the shape is truly dynamic.type TUserProfileForm = { avatar_url: string; cover_image: string; - cover_image_asset: any; + cover_image_asset: unknown; cover_image_url: string; // ... };
163-167: Remove empty.then()and.catch()handlers.These handlers don't perform any meaningful operations and add unnecessary code. If
setPromiseToastalready handles success/error feedback, these can be removed.
86-110: Mixed async/await with.then()/.catch()pattern.The function uses
awaitbut then chains.then()and.catch(). Consider using consistent try/catch for better readability.Refactored with consistent async/await
const handleProfilePictureDelete = async (url: string | null | undefined) => { if (!url) return; - await updateCurrentUser({ - avatar_url: "", - }) - .then(() => { - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Success!", - message: "Profile picture deleted successfully.", - }); - setValue("avatar_url", ""); - return; - }) - .catch(() => { - setToast({ - type: TOAST_TYPE.ERROR, - title: "Error!", - message: "There was some error in deleting your profile picture. Please try again.", - }); - }) - .finally(() => { - setIsImageUploadModalOpen(false); - }); + try { + await updateCurrentUser({ avatar_url: "" }); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Success!", + message: "Profile picture deleted successfully.", + }); + setValue("avatar_url", ""); + } catch { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "There was some error in deleting your profile picture. Please try again.", + }); + } finally { + setIsImageUploadModalOpen(false); + } };apps/web/core/components/settings/profile/content/pages/activity/activity-list.tsx (3)
59-59: Avoidanytype for activity items.The
activityItemparameter is typed asany, which disables type checking. Consider defining or importing a proper type for user activity items.- {userProfileActivity.results.map((activityItem: any) => { + {userProfileActivity.results.map((activityItem) => {If the
userService.getUserActivityresponse is properly typed, the inference should work. Otherwise, define anIUserActivitytype.
54-54: Acknowledge the TODO for future refactoring.The TODO comment indicates awareness that this component needs refactoring. Consider creating a tracking issue if one doesn't exist.
Would you like me to open a new issue to track the refactoring of this component?
81-83: Unusual!text-20class pattern.The
!text-20class uses the!importantmodifier prefix. This typically indicates a specificity issue. Consider reviewing whether the base styles can be adjusted instead.apps/web/ce/components/common/modal/global.tsx (1)
10-12: Clarify or remove unusedworkspaceSlugprop.The
TGlobalModalsPropstype definesworkspaceSlug, but it's never used in the component (indicated by the_propsparameter name). If this prop is for future extensibility when more workspace-scoped modals are added, consider adding a JSDoc comment explaining this. Otherwise, remove the unused prop from the type definition to keep the interface clean.♻️ Suggested cleanup if not needed for future use
-type TGlobalModalsProps = { - workspaceSlug: string; -}; - /** * GlobalModals component manages all workspace-level modals across Plane applications. * * This includes: * - Profile settings modal */ -export const GlobalModals = observer(function GlobalModals(_props: TGlobalModalsProps) { +export const GlobalModals = observer(function GlobalModals() { return ( <Suspense fallback={null}> <ProfileSettingsModal /> </Suspense> ); });apps/web/app/(all)/[workspaceSlug]/layout.tsx (1)
17-17: Unused prop: workspaceSlug passed to GlobalModals.Looking at the GlobalModals implementation in
apps/web/ce/components/common/modal/global.tsx, the component signature uses_props(underscore prefix), indicating the prop is intentionally unused. Consider removing this prop to avoid confusion and keep the interface clean.♻️ Proposed refactor to remove unused prop
- <GlobalModals workspaceSlug={workspaceSlug} /> + <GlobalModals />You'll also need to update the GlobalModals component interface accordingly.
apps/web/core/components/settings/profile/modal.tsx (2)
19-28: Fragile animation timing with magic number.The 300ms delay in
setTimeoutis hardcoded and tightly coupled to the modal's close animation duration. If the animation timing changes inModalCore, this will break or cause visual glitches.Consider:
- Extracting the duration as a named constant (e.g.,
MODAL_CLOSE_ANIMATION_DURATION)- Using the same constant in both ModalCore's transition and this timeout
- Or handle the state reset via an
onAnimationCompletecallback if available♻️ Example refactor with named constant
+const MODAL_CLOSE_ANIMATION_DURATION = 300; + const handleClose = useCallback(() => { toggleProfileSettingsModal({ isOpen: false, }); setTimeout(() => { toggleProfileSettingsModal({ activeTab: null, }); - }, 300); + }, MODAL_CLOSE_ANIMATION_DURATION); }, [toggleProfileSettingsModal]);
36-36: Fixed height may not be responsive.The modal uses a fixed height of
h-175(700px). This could cause issues on smaller screens or different viewport sizes.Consider using
max-h-175instead to allow the modal to shrink on smaller screens while maintaining a maximum height on larger displays.- className="h-175" + className="max-h-[90vh]"apps/web/core/components/settings/profile/content/pages/preferences/language-and-timezone-list.tsx (2)
25-57: Use translation keys for toast messages.The toast messages in
handleTimezoneChangeandhandleLanguageChangeare hardcoded in English (e.g.,"Success!","Timezone updated successfully"), while the rest of the component usest()for translations. This creates i18n inconsistency.Proposed fix using translations
const handleTimezoneChange = async (value: string) => { try { await updateCurrentUser({ user_timezone: value }); setToast({ - title: "Success!", - message: "Timezone updated successfully", + title: t("success"), + message: t("timezone_updated_successfully"), type: TOAST_TYPE.SUCCESS, }); } catch (_error) { setToast({ - title: "Error!", - message: "Failed to update timezone", + title: t("error"), + message: t("failed_to_update_timezone"), type: TOAST_TYPE.ERROR, }); } }; const handleLanguageChange = async (value: string) => { try { await updateUserProfile({ language: value }); setToast({ - title: "Success!", - message: "Language updated successfully", + title: t("success"), + message: t("language_updated_successfully"), type: TOAST_TYPE.SUCCESS, }); } catch (_error) { setToast({ - title: "Error!", - message: "Failed to update language", + title: t("error"), + message: t("failed_to_update_language"), type: TOAST_TYPE.ERROR, }); } };
93-98: Use translation keys for StartOfWeekPreference strings.The
titleanddescriptionprops are hardcoded English strings, inconsistent with the translatedt("timezone")andt("language")used elsewhere in this component.Proposed fix
<StartOfWeekPreference option={{ - title: "First day of the week", - description: "This will change how all calendars in your app look.", + title: t("first_day_of_the_week"), + description: t("first_day_of_the_week_description"), }} />apps/web/core/components/appearance/theme-switcher.tsx (1)
35-56: Use translation keys for toast messages and addsetThemeto dependencies.
- Toast messages are hardcoded in English while
t()is available for translations.setThemeis used inside the callback but not listed in dependencies.Proposed fix
const handleThemeChange = useCallback( (themeOption: I_THEME_OPTION) => { try { setTheme(themeOption.value); const updatePromise = updateUserTheme({ theme: themeOption.value }); setPromiseToast(updatePromise, { - loading: "Updating theme...", + loading: t("updating_theme"), success: { - title: "Success!", - message: () => "Theme updated successfully!", + title: t("success"), + message: () => t("theme_updated_successfully"), }, error: { - title: "Error!", - message: () => "Failed to update the theme", + title: t("error"), + message: () => t("failed_to_update_theme"), }, }); } catch (error) { console.error("Error updating theme:", error); } }, - [updateUserTheme] + [setTheme, updateUserTheme, t] );apps/web/app/(all)/settings/profile/[profileTabId]/page.tsx (1)
28-33: Consider separating loading state from invalid tab handling.Currently, both
!currentUser(loading) and!isAValidTab(invalid route) render the sameLogoSpinner. This makes it impossible to distinguish between a user still loading and an invalid tab URL. For invalid tabs, consider redirecting to a default tab or showing an error state instead.Proposed approach
+ // Handle loading state + if (!currentUser) + return ( + <div className="size-full grid place-items-center px-4"> + <LogoSpinner /> + </div> + ); + + // Redirect invalid tabs to default + if (!isAValidTab) { + router.replace("/settings/profile/general"); + return ( + <div className="size-full grid place-items-center px-4"> + <LogoSpinner /> + </div> + ); + } - if (!currentUser || !isAValidTab) - return ( - <div className="size-full grid place-items-center px-4"> - <LogoSpinner /> - </div> - );
| export const clientLoader = () => { | ||
| throw redirect(`/settings/profile/api-tokens/`); |
There was a problem hiding this comment.
Inconsistent redirect pattern may lose search parameters.
Other redirect handlers in this PR (profile-settings.tsx, workspace-account-settings.tsx) preserve and forward search parameters using Route.ClientLoaderArgs with request.url. This simplified version doesn't forward any query parameters.
🔧 Proposed fix to preserve search parameters
-export const clientLoader = () => {
- throw redirect(`/settings/profile/api-tokens/`);
+export const clientLoader = ({ request }: Route.ClientLoaderArgs) => {
+ const searchParams = new URL(request.url).searchParams;
+ throw redirect(`/settings/profile/api-tokens?${searchParams.toString()}`);
};Also note: The trailing slash in the redirect path may be unnecessary and inconsistent with other redirects in this PR.
📝 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.
| export const clientLoader = () => { | |
| throw redirect(`/settings/profile/api-tokens/`); | |
| export const clientLoader = ({ request }: Route.ClientLoaderArgs) => { | |
| const searchParams = new URL(request.url).searchParams; | |
| throw redirect(`/settings/profile/api-tokens?${searchParams.toString()}`); | |
| }; |
🤖 Prompt for AI Agents
In @apps/web/app/routes/redirects/core/api-tokens.tsx around lines 3 - 4,
clientLoader currently throws a hardcoded redirect and drops query params;
change its signature to accept ({ request }: Route.ClientLoaderArgs) and
preserve search parameters by building the target with the incoming URL's search
(e.g., const url = new URL(request.url); throw
redirect(`/settings/profile/api-tokens${url.search}`)); also remove the trailing
slash in the target to match other redirects.
| const ref = useRef<HTMLDivElement>(null); | ||
| // router | ||
| const { userId, workspaceSlug } = useParams(); | ||
| const { userId } = useParams(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/core/components/profile/sidebar.tsx | head -100Repository: makeplane/plane
Length of output: 4397
🏁 Script executed:
wc -l apps/web/core/components/profile/sidebar.tsxRepository: makeplane/plane
Length of output: 107
🏁 Script executed:
rg 'const\s*\{\s*\w+\s*\}\s*=\s*useParams\(' apps/web/core/components/ -A5 | head -80Repository: makeplane/plane
Length of output: 6678
🏁 Script executed:
rg 'userId.*useParams' apps/web/ -B2 -A5 | head -100Repository: makeplane/plane
Length of output: 8571
🏁 Script executed:
sed -n '88,100p' apps/web/core/components/profile/sidebar.tsxRepository: makeplane/plane
Length of output: 516
🏁 Script executed:
rg 'currentUser\?.id.*userId' apps/web/core/components/profile/sidebar.tsx -B3 -A3Repository: makeplane/plane
Length of output: 314
🏁 Script executed:
cat apps/web/tsconfig.json | grep -A 10 '"strict'Repository: makeplane/plane
Length of output: 439
🏁 Script executed:
rg 'strict.*true|compilerOptions' apps/web/tsconfig.json -A 15 | head -30Repository: makeplane/plane
Length of output: 793
🏁 Script executed:
sed -n '30,50p' apps/web/core/components/profile/sidebar.tsxRepository: makeplane/plane
Length of output: 838
🏁 Script executed:
rg '@ts-ignore|@ts-expect-error|as string|as const' apps/web/core/components/profile/sidebar.tsxRepository: makeplane/plane
Length of output: 41
🏁 Script executed:
rg 'userId.*string' apps/web/core/components/profile/ -B2 -A2Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
rg 'useParams|return.*Record' node_modules/next/navigation.d.ts 2>/dev/null | head -20Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
grep -r "useParams" apps/web/core/components/profile/ -l | while read f; do echo "=== $f ==="; grep -A 20 "useParams" "$f" | head -25; doneRepository: makeplane/plane
Length of output: 7791
🏁 Script executed:
find apps/web/app -name "*profile*" -type d | head -5Repository: makeplane/plane
Length of output: 318
🏁 Script executed:
ls -la apps/web/app/\(all\)/\[workspaceSlug\]/\(projects\)/profile/Repository: makeplane/plane
Length of output: 227
🏁 Script executed:
grep -r "userId\?.toString\|userId as string" apps/web/core/components/profile/ -B2 -A2Repository: makeplane/plane
Length of output: 2358
🏁 Script executed:
sed -n '1,30p' apps/web/core/components/profile/profile-issues-filter.tsxRepository: makeplane/plane
Length of output: 1292
Add explicit type narrowing or assertion for userId before comparison on line 92.
The useParams() hook returns Record<string, string | string[]>, so destructured userId has type string | string[]. Before the comparison currentUser?.id === userId on line 92, either convert it with .toString() (as done in profile-issues-filter.tsx and other profile components) or narrow the type explicitly. This ensures type consistency across the profile components.
🤖 Prompt for AI Agents
In @apps/web/core/components/profile/sidebar.tsx at line 36, The destructured
userId from useParams() is string | string[]; before the comparison
currentUser?.id === userId in this file/component, explicitly narrow or convert
userId to a string (e.g., call .toString() or handle array case like other
profile components) or assert it as string so the types match; update the
comparison to use the normalized string variable (mirroring the approach in
profile-issues-filter.tsx).
| const { data: userProfileActivity } = useSWR( | ||
| USER_ACTIVITY({ | ||
| cursor, | ||
| }), | ||
| () => | ||
| userService.getUserActivity({ | ||
| cursor, | ||
| per_page: perPage, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find USER_ACTIVITY constant definition
rg -n "USER_ACTIVITY" --type=ts -B2 -A10 apps/web/core/Repository: makeplane/plane
Length of output: 6465
🏁 Script executed:
#!/bin/bash
# Find all usages of USER_ACTIVITY to see scope of changes
rg -n "USER_ACTIVITY\(" --type=ts -B1 -A3Repository: makeplane/plane
Length of output: 1046
SWR cache key doesn't include per_page, which may cause stale data.
The SWR key only includes cursor, but the fetch function uses per_page. If per_page changes while cursor stays the same, SWR will return cached data with the old page size.
Proposed fix
const { data: userProfileActivity } = useSWR(
- USER_ACTIVITY({
- cursor,
- }),
+ USER_ACTIVITY({ cursor, per_page: perPage }),
() =>
userService.getUserActivity({
cursor,
per_page: perPage,
})
);This requires updating the USER_ACTIVITY constant to accept per_page as a parameter.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@apps/web/core/components/settings/profile/content/pages/activity/activity-list.tsx
around lines 33 - 42, The SWR cache key for useSWR only includes cursor but the
fetcher userService.getUserActivity also depends on perPage, causing stale
results when perPage changes; update the USER_ACTIVITY key generator to accept
per_page and call useSWR with USER_ACTIVITY({ cursor, per_page: perPage }) (and
ensure the fetcher call uses the same per_page param), so the key uniquely
reflects both cursor and perPage and invalidates cache correctly for
userProfileActivity.
| const updateCurrentUserDetail = updateCurrentUser(userPayload).finally(() => setIsLoading(false)); | ||
| const updateCurrentUserProfile = updateUserProfile(profilePayload).finally(() => setIsLoading(false)); |
There was a problem hiding this comment.
Double setIsLoading(false) calls cause redundant state updates.
Both updateCurrentUserDetail and updateCurrentUserProfile have .finally(() => setIsLoading(false)) attached. When Promise.all resolves, setIsLoading(false) is called twice. Consider moving the loading state reset to a single location.
Proposed fix
- const updateCurrentUserDetail = updateCurrentUser(userPayload).finally(() => setIsLoading(false));
- const updateCurrentUserProfile = updateUserProfile(profilePayload).finally(() => setIsLoading(false));
+ const updateCurrentUserDetail = updateCurrentUser(userPayload);
+ const updateCurrentUserProfile = updateUserProfile(profilePayload);
const promises = [updateCurrentUserDetail, updateCurrentUserProfile];
const updateUserAndProfile = Promise.all(promises);
setPromiseToast(updateUserAndProfile, {
loading: "Updating...",
success: {
title: "Success!",
message: () => `Profile updated successfully.`,
},
error: {
title: "Error!",
message: () => `There was some error in updating your profile. Please try again.`,
},
});
- updateUserAndProfile
- .then(() => {
- return;
- })
- .catch(() => {});
+ updateUserAndProfile.finally(() => setIsLoading(false));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateCurrentUserDetail = updateCurrentUser(userPayload).finally(() => setIsLoading(false)); | |
| const updateCurrentUserProfile = updateUserProfile(profilePayload).finally(() => setIsLoading(false)); | |
| const updateCurrentUserDetail = updateCurrentUser(userPayload); | |
| const updateCurrentUserProfile = updateUserProfile(profilePayload); | |
| const promises = [updateCurrentUserDetail, updateCurrentUserProfile]; | |
| const updateUserAndProfile = Promise.all(promises); | |
| setPromiseToast(updateUserAndProfile, { | |
| loading: "Updating...", | |
| success: { | |
| title: "Success!", | |
| message: () => `Profile updated successfully.`, | |
| }, | |
| error: { | |
| title: "Error!", | |
| message: () => `There was some error in updating your profile. Please try again.`, | |
| }, | |
| }); | |
| updateUserAndProfile.finally(() => setIsLoading(false)); |
🤖 Prompt for AI Agents
In @apps/web/core/components/settings/profile/content/pages/general/form.tsx
around lines 146 - 147, Both updateCurrentUserDetail and
updateCurrentUserProfile attach .finally(() => setIsLoading(false)) causing
duplicate state updates when awaited together; remove the per-promise finally
handlers and instead call setIsLoading(false) once in the finally of the
combined Promise.all (or after awaiting Promise.all of
updateCurrentUser(userPayload) and updateUserProfile(profilePayload)) so
setIsLoading is reset only once; update references: updateCurrentUserDetail,
updateCurrentUserProfile, updateCurrentUser, updateUserProfile, setIsLoading,
and Promise.all.
| <button type="button" onClick={() => setIsImageUploadModalOpen(true)}> | ||
| {!userAvatar || userAvatar === "" ? ( | ||
| <div className="h-16 w-16 rounded-md bg-layer-1 p-2"> | ||
| <CircleUserRound className="h-full w-full text-secondary" /> | ||
| </div> | ||
| ) : ( | ||
| <div className="relative h-16 w-16 overflow-hidden"> | ||
| <img | ||
| src={getFileURL(userAvatar)} | ||
| className="absolute left-0 top-0 h-full w-full rounded-lg object-cover" | ||
| onClick={() => setIsImageUploadModalOpen(true)} | ||
| alt={currentUser?.display_name} | ||
| role="button" | ||
| /> | ||
| </div> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
Nested button inside button creates invalid HTML.
The outer <button> (line 202) contains an <img> with role="button" and onClick (lines 212-214). This creates confusing semantics. The onClick on the image is redundant since clicking it will already trigger the parent button's handler.
Proposed fix - remove redundant onClick from img
<div className="relative h-16 w-16 overflow-hidden">
<img
src={getFileURL(userAvatar)}
className="absolute left-0 top-0 h-full w-full rounded-lg object-cover"
- onClick={() => setIsImageUploadModalOpen(true)}
alt={currentUser?.display_name}
- role="button"
/>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button type="button" onClick={() => setIsImageUploadModalOpen(true)}> | |
| {!userAvatar || userAvatar === "" ? ( | |
| <div className="h-16 w-16 rounded-md bg-layer-1 p-2"> | |
| <CircleUserRound className="h-full w-full text-secondary" /> | |
| </div> | |
| ) : ( | |
| <div className="relative h-16 w-16 overflow-hidden"> | |
| <img | |
| src={getFileURL(userAvatar)} | |
| className="absolute left-0 top-0 h-full w-full rounded-lg object-cover" | |
| onClick={() => setIsImageUploadModalOpen(true)} | |
| alt={currentUser?.display_name} | |
| role="button" | |
| /> | |
| </div> | |
| )} | |
| </button> | |
| <button type="button" onClick={() => setIsImageUploadModalOpen(true)}> | |
| {!userAvatar || userAvatar === "" ? ( | |
| <div className="h-16 w-16 rounded-md bg-layer-1 p-2"> | |
| <CircleUserRound className="h-full w-full text-secondary" /> | |
| </div> | |
| ) : ( | |
| <div className="relative h-16 w-16 overflow-hidden"> | |
| <img | |
| src={getFileURL(userAvatar)} | |
| className="absolute left-0 top-0 h-full w-full rounded-lg object-cover" | |
| alt={currentUser?.display_name} | |
| /> | |
| </div> | |
| )} | |
| </button> |
🤖 Prompt for AI Agents
In @apps/web/core/components/settings/profile/content/pages/general/form.tsx
around lines 202 - 218, The img inside the clickable avatar button is given its
own onClick and role="button", creating nested button semantics; remove the
onClick and role attributes from the img (the parent button already calls
setIsImageUploadModalOpen(true)), leaving the img as a plain image using
src={getFileURL(userAvatar)} and alt={currentUser?.display_name} so clicks are
handled by the outer button and HTML remains valid.
apps/web/core/components/settings/profile/content/pages/security.tsx
Outdated
Show resolved
Hide resolved
| <Suspense> | ||
| <PageComponent /> | ||
| </Suspense> |
There was a problem hiding this comment.
Missing Suspense fallback degrades loading experience.
The Suspense component has no fallback prop, which means nothing will render while the lazy-loaded component is loading. This creates a jarring user experience with an empty content area.
🎨 Proposed fix with loading state
- <Suspense>
+ <Suspense fallback={<div className="p-8"><Spinner /></div>}>
<PageComponent />
</Suspense>Or use a skeleton loader for a more polished experience.
📝 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.
| <Suspense> | |
| <PageComponent /> | |
| </Suspense> | |
| <Suspense fallback={<div className="p-8"><Spinner /></div>}> | |
| <PageComponent /> | |
| </Suspense> |
🤖 Prompt for AI Agents
In @apps/web/core/components/settings/profile/content/root.tsx around lines 26 -
28, The Suspense wrapper around PageComponent is missing a fallback, causing a
blank area during lazy loading; update the Suspense usage to provide a fallback
UI (e.g., a spinner or Skeleton component) by supplying the fallback prop to
Suspense so that a loading indicator is rendered while PageComponent loads, and
reuse or create a lightweight Loading/Skeleton component if needed to keep UX
consistent.
| export const ProfileSettingsSidebarHeader = observer(function ProfileSettingsSidebarHeader() { | ||
| // store hooks | ||
| const { data: currentUser } = useUser(); | ||
|
|
||
| return ( | ||
| <div className="shrink-0 flex items-center gap-2"> | ||
| <div className="shrink-0"> | ||
| <Avatar | ||
| src={getFileURL(currentUser?.avatar_url ?? "")} | ||
| name={currentUser?.display_name} | ||
| size={32} | ||
| shape="circle" | ||
| className="text-16" | ||
| /> | ||
| </div> | ||
| <div className="truncate"> | ||
| <p className="text-body-sm-medium truncate"> | ||
| {currentUser?.first_name} {currentUser?.last_name} | ||
| </p> | ||
| <p className="text-caption-md-regular truncate">{currentUser?.email}</p> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Avoid getFileURL("") + improve name fallback to prevent “undefined undefined”.
Suggested tweak
export const ProfileSettingsSidebarHeader = observer(function ProfileSettingsSidebarHeader() {
// store hooks
const { data: currentUser } = useUser();
+ const fullName = [currentUser?.first_name, currentUser?.last_name].filter(Boolean).join(" ");
+ const primaryLabel = fullName || currentUser?.display_name || currentUser?.email || "User";
+
return (
<div className="shrink-0 flex items-center gap-2">
<div className="shrink-0">
<Avatar
- src={getFileURL(currentUser?.avatar_url ?? "")}
- name={currentUser?.display_name}
+ src={currentUser?.avatar_url ? getFileURL(currentUser.avatar_url) : undefined}
+ name={primaryLabel}
size={32}
shape="circle"
className="text-16"
/>
</div>
<div className="truncate">
<p className="text-body-sm-medium truncate">
- {currentUser?.first_name} {currentUser?.last_name}
+ {primaryLabel}
</p>
<p className="text-caption-md-regular truncate">{currentUser?.email}</p>
</div>
</div>
);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const ProfileSettingsSidebarHeader = observer(function ProfileSettingsSidebarHeader() { | |
| // store hooks | |
| const { data: currentUser } = useUser(); | |
| return ( | |
| <div className="shrink-0 flex items-center gap-2"> | |
| <div className="shrink-0"> | |
| <Avatar | |
| src={getFileURL(currentUser?.avatar_url ?? "")} | |
| name={currentUser?.display_name} | |
| size={32} | |
| shape="circle" | |
| className="text-16" | |
| /> | |
| </div> | |
| <div className="truncate"> | |
| <p className="text-body-sm-medium truncate"> | |
| {currentUser?.first_name} {currentUser?.last_name} | |
| </p> | |
| <p className="text-caption-md-regular truncate">{currentUser?.email}</p> | |
| </div> | |
| </div> | |
| ); | |
| export const ProfileSettingsSidebarHeader = observer(function ProfileSettingsSidebarHeader() { | |
| // store hooks | |
| const { data: currentUser } = useUser(); | |
| const fullName = [currentUser?.first_name, currentUser?.last_name].filter(Boolean).join(" "); | |
| const primaryLabel = fullName || currentUser?.display_name || currentUser?.email || "User"; | |
| return ( | |
| <div className="shrink-0 flex items-center gap-2"> | |
| <div className="shrink-0"> | |
| <Avatar | |
| src={currentUser?.avatar_url ? getFileURL(currentUser.avatar_url) : undefined} | |
| name={primaryLabel} | |
| size={32} | |
| shape="circle" | |
| className="text-16" | |
| /> | |
| </div> | |
| <div className="truncate"> | |
| <p className="text-body-sm-medium truncate"> | |
| {primaryLabel} | |
| </p> | |
| <p className="text-caption-md-regular truncate">{currentUser?.email}</p> | |
| </div> | |
| </div> | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In @apps/web/core/components/settings/profile/sidebar/header.tsx around lines 8
- 30, ProfileSettingsSidebarHeader currently calls
getFileURL(currentUser?.avatar_url ?? "") and renders names via
{currentUser?.first_name} {currentUser?.last_name}, which can call
getFileURL("") and show "undefined undefined"; change to pass undefined to
getFileURL when no avatar (e.g., getFileURL(currentUser?.avatar_url)) and render
a safe name fallback using display_name or a single-field fallback (e.g., use
currentUser?.display_name ?? currentUser?.first_name ?? currentUser?.last_name
or join available parts) so Avatar receives a proper src/name and the UI never
shows "undefined undefined".
| {Object.values(workspaces).map((workspace) => ( | ||
| <SettingsSidebarItem | ||
| key={workspace.id} | ||
| as="link" | ||
| href={`/${workspace.slug}/`} | ||
| iconNode={<WorkspaceLogo logo={workspace.logo_url} name={workspace.name} classNames="shrink-0" />} | ||
| label={workspace.name} | ||
| isActive={false} | ||
| /> |
There was a problem hiding this comment.
Missing active workspace indication.
All workspace items have isActive={false} hardcoded (line 28), so the current workspace is never highlighted. This degrades the user experience as users cannot easily identify which workspace they're currently in.
🔧 Proposed fix to highlight active workspace
The component should receive the current workspaceSlug (perhaps from the parent or via a store hook) and compare it with each workspace's slug:
-export const ProfileSettingsSidebarWorkspaceOptions = observer(function ProfileSettingsSidebarWorkspaceOptions() {
+export const ProfileSettingsSidebarWorkspaceOptions = observer(function ProfileSettingsSidebarWorkspaceOptions(props: { currentWorkspaceSlug?: string }) {
+ const { currentWorkspaceSlug } = props;
// store hooks
const { workspaces } = useWorkspace();
// translation
const { t } = useTranslation();
return (
<div className="shrink-0">
<div className="p-2 text-caption-md-medium text-tertiary capitalize">{t("workspace")}</div>
<div className="flex flex-col">
{Object.values(workspaces).map((workspace) => (
<SettingsSidebarItem
key={workspace.id}
as="link"
href={`/${workspace.slug}/`}
iconNode={<WorkspaceLogo logo={workspace.logo_url} name={workspace.name} classNames="shrink-0" />}
label={workspace.name}
- isActive={false}
+ isActive={workspace.slug === currentWorkspaceSlug}
/>
))}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/core/components/settings/profile/sidebar/workspace-options.tsx
around lines 21 - 29, The workspace items always pass isActive={false} so the
current workspace is never highlighted; update the mapping in
workspace-options.tsx to determine isActive by comparing each workspace.slug to
the active workspace slug (e.g., accept a workspaceSlug prop on the component or
read it from the store/hook used by the app) and pass isActive={workspace.slug
=== workspaceSlug} into SettingsSidebarItem (keep the same keys and props like
iconNode, href, label). Ensure workspaceSlug is provided to this component or
obtained via the appropriate hook before the map.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/web/core/components/workspace/sidebar/user-menu-root.tsx (1)
26-27: Consolidate duplicateuseUsercalls into a single destructuring.The hook is called twice on consecutive lines. Destructure both
dataandsignOutin one call to reduce overhead.Suggested fix
- const { data: currentUser } = useUser(); - const { signOut } = useUser(); + const { data: currentUser, signOut } = useUser();apps/web/core/components/settings/profile/content/pages/security.tsx (1)
89-94: Improve type safety for error handling.The error handling accesses
error.error_codeafter checkingerror instanceof Error, but the standardErrortype doesn't include anerror_codeproperty. While theinoperator check and optional chaining make this safe at runtime, TypeScript's type system won't properly infer this without explicit type assertions.As per coding guidelines, all files should be fully typed with strict mode enabled.
♻️ Proposed fix with explicit type handling
- } catch (error: unknown) { - let errorInfo = undefined; - if (error instanceof Error) { - const code = "error_code" in error ? error.error_code?.toString() : undefined; - errorInfo = code ? authErrorHandler(code as EAuthenticationErrorCodes) : undefined; - } + } catch (error: unknown) { + let errorInfo = undefined; + if (error instanceof Error) { + const errorWithCode = error as Error & { error_code?: string | number }; + const code = errorWithCode.error_code?.toString(); + errorInfo = code ? authErrorHandler(code as EAuthenticationErrorCodes) : undefined; + }Alternatively, consider defining a custom error type interface for authentication errors that includes the
error_codeproperty.apps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsx (2)
42-48: Log errors for debugging.The error is caught but not logged, making debugging difficult in production. Consider logging the error and, if the API provides specific error messages, displaying them to the user for better feedback.
🔍 Proposed improvement
- } catch (_error) { + } catch (error) { + console.error("Failed to update email notification setting:", error); setToast({ title: t("error"), type: TOAST_TYPE.ERROR, - message: t("failed_to_update_email_notification_setting"), + message: error instanceof Error ? error.message : t("failed_to_update_email_notification_setting"), }); }
57-158: Consider reducing code duplication.The same pattern is repeated five times with only field names and translations differing. Refactoring to use a configuration array would reduce duplication and improve maintainability.
♻️ Suggested refactor using config array
+ const notificationSettings: Array<{ + name: keyof IUserEmailNotificationSettings; + title: string; + description: string; + nested?: boolean; + }> = [ + { name: "property_change", title: t("property_changes"), description: t("property_changes_description") }, + { name: "state_change", title: t("state_change"), description: t("state_change_description") }, + { name: "issue_completed", title: t("issue_completed"), description: t("issue_completed_description"), nested: true }, + { name: "comment", title: t("comments"), description: t("comments_description") }, + { name: "mention", title: t("mentions"), description: t("mentions_description") }, + ]; + return ( <div className="flex flex-col gap-y-1"> - <SettingsControlItem - title={t("property_changes")} - description={t("property_changes_description")} - control={ - <Controller - control={control} - name="property_change" - render={({ field: { value, onChange } }) => ( - <ToggleSwitch - value={value} - onChange={(newValue) => { - onChange(newValue); - handleSettingChange("property_change", newValue); - }} - size="sm" - /> - )} - /> - } - /> - {/* ... other repeated items ... */} + {notificationSettings.map((setting) => { + const item = ( + <SettingsControlItem + key={setting.name} + title={setting.title} + description={setting.description} + control={ + <Controller + control={control} + name={setting.name} + render={({ field: { value, onChange } }) => ( + <ToggleSwitch + value={value} + onChange={(newValue) => { + onChange(newValue); + handleSettingChange(setting.name, newValue); + }} + size="sm" + /> + )} + /> + } + /> + ); + return setting.nested ? ( + <div key={setting.name} className="border-l-3 border-subtle-1 pl-3"> + {item} + </div> + ) : item; + })} </div> );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsxapps/web/core/components/settings/profile/content/pages/security.tsxapps/web/core/components/workspace/sidebar/user-menu-root.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
apps/web/core/components/settings/profile/content/pages/security.tsxapps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsxapps/web/core/components/workspace/sidebar/user-menu-root.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Enable TypeScript strict mode and ensure all files are fully typed
Files:
apps/web/core/components/settings/profile/content/pages/security.tsxapps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsxapps/web/core/components/workspace/sidebar/user-menu-root.tsx
**/*.{js,jsx,ts,tsx,json,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier with Tailwind plugin for code formatting, run
pnpm fix:format
Files:
apps/web/core/components/settings/profile/content/pages/security.tsxapps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsxapps/web/core/components/workspace/sidebar/user-menu-root.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately
Files:
apps/web/core/components/settings/profile/content/pages/security.tsxapps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsxapps/web/core/components/workspace/sidebar/user-menu-root.tsx
🧠 Learnings (6)
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Avoid `import ... assert` syntax; use `with` instead for import attributes
Applied to files:
apps/web/core/components/settings/profile/content/pages/security.tsx
📚 Learning: 2025-10-01T15:30:17.605Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7888
File: packages/propel/src/avatar/avatar.stories.tsx:2-3
Timestamp: 2025-10-01T15:30:17.605Z
Learning: In the makeplane/plane repository, avoid suggesting inline type imports (e.g., `import { Avatar, type TAvatarSize }`) due to bundler compatibility issues. Keep type imports and value imports as separate statements.
Applied to files:
apps/web/core/components/settings/profile/content/pages/security.tsx
📚 Learning: 2025-07-14T11:22:43.964Z
Learnt from: gakshita
Repo: makeplane/plane PR: 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/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsx
📚 Learning: 2025-10-10T13:25:14.810Z
Learnt from: gakshita
Repo: makeplane/plane PR: 7949
File: apps/web/core/components/issues/issue-modal/form.tsx:183-189
Timestamp: 2025-10-10T13:25:14.810Z
Learning: In `apps/web/core/components/issues/issue-modal/form.tsx`, the form reset effect uses a `dataResetProperties` dependency array prop (default: []) to give parent components explicit control over when the form resets. Do not suggest adding the `data` prop itself to the dependency array, as this would cause unwanted resets on every render when the data object reference changes, disrupting user input. The current pattern is intentional and allows the parent to trigger resets only when specific conditions are met.
Applied to files:
apps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsx
📚 Learning: 2025-12-17T10:58:59.591Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: packages/tailwind-config/AGENTS.md:0-0
Timestamp: 2025-12-17T10:58:59.591Z
Learning: Applies to packages/tailwind-config/**/*.{ts,tsx} : Sidebar menu items should use transparent backgrounds with hover states like `hover:bg-layer-1-hover` rather than base layer backgrounds, maintaining visual lightness while providing hover feedback
Applied to files:
apps/web/core/components/workspace/sidebar/user-menu-root.tsx
📚 Learning: 2025-10-21T17:22:05.204Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.
Applied to files:
apps/web/core/components/workspace/sidebar/user-menu-root.tsx
🧬 Code graph analysis (1)
apps/web/core/components/workspace/sidebar/user-menu-root.tsx (4)
packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)apps/web/core/components/common/cover-image.tsx (1)
CoverImage(26-44)packages/i18n/src/store/index.ts (1)
t(222-243)packages/constants/src/endpoints.ts (1)
GOD_MODE_URL(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check:types
- GitHub Check: check:lint
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
apps/web/core/components/workspace/sidebar/user-menu-root.tsx (4)
34-42: LGTM!The fire-and-forget pattern with
.catch()for error toast is appropriate for sign-out flows where success typically triggers a redirect.
44-48: LGTM!The effect correctly synchronizes menu state with the sidebar dropdown toggle, and dependencies are properly listed.
76-103: LGTM!The cover image header implementation correctly uses
CoverImagewithshowDefaultWhenEmpty, and the centered avatar with user details provides a clean visual hierarchy.
104-133: LGTM!The profile settings modal triggers are well-structured with distinct
activeTabvalues, and the sign-out item is cleanly separated. Menu item styling is consistent.apps/web/core/components/settings/profile/content/pages/security.tsx (3)
12-13: LGTM - Import aligns with new settings structure.The import of
SettingsHeadingcorrectly supports the migration from legacy layout components to the new unified profile settings UI.
42-42: LGTM - Component export follows best practices.The named export with observer wrapping is appropriate for the refactored settings structure and enables proper reactivity for MobX store integration.
121-256: LGTM - Well-structured responsive layout.The refactored layout successfully migrates from legacy wrappers to the new
SettingsHeadingcomponent while maintaining all functionality:
- Responsive grid (
sm:grid-cols-2) appropriately displays password fields side-by-side on larger screens- Conditional old password field takes full width when present
- Password visibility toggles, validation, and error states are properly preserved
- Form semantics and accessibility structure remain intact
The changes align well with the PR's objective of improving profile settings UI consistency.
apps/web/core/components/settings/profile/content/pages/notifications/email-notification-form.tsx (2)
51-53: Verify form reset behavior with MobX observer.Including
datadirectly in the dependency array may cause unnecessary form resets if the object reference changes on every render. Whileobservershould provide stable references for unchanged data, consider adding a control mechanism (similar to thedataResetPropertiespattern used elsewhere in the codebase) to give explicit control over when the form resets.Based on learnings, this codebase has patterns for controlling form resets with explicit dependency arrays to prevent unwanted resets during user input.
18-19: > Likely an incorrect or invalid review comment.
| const { signOut } = useUser(); | ||
| const { toggleProfileSettingsModal } = useCommandPalette(); | ||
| // derived values | ||
| const isUserInstanceAdmin = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how isUserInstanceAdmin or similar admin checks are implemented elsewhere
rg -n --type=ts --type=tsx -C3 'isUserInstanceAdmin|isInstanceAdmin|instance.*admin' -g '!node_modules'Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Read the specific file to see the context around line 30 and lines 134-141
wc -l "apps/web/core/components/workspace/sidebar/user-menu-root.tsx"Repository: makeplane/plane
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Read the complete file to examine isUserInstanceAdmin usage
cat -n "apps/web/core/components/workspace/sidebar/user-menu-root.tsx"Repository: makeplane/plane
Length of output: 5875
🏁 Script executed:
#!/bin/bash
# Search for admin patterns with corrected syntax (ts files only or use glob)
rg -n 'isUserInstanceAdmin|isInstanceAdmin|instance.*admin' --type ts -C2Repository: makeplane/plane
Length of output: 10082
Hardcoded isUserInstanceAdmin = false makes God Mode section unreachable.
Lines 134-141 form dead code because false && ... is always false. Either implement proper admin status detection using the existing currentUserInstanceAdminStatus() service method, or remove the unreachable code path and the unused router import.
Description
This PR improves the profile settings interface-
profile/*->settings/profile/*:workspaceSlug/settings/account/*->settings/profile/*Type of Change
Media
Screen.Recording.2026-01-08.at.18.07.11.mov
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.