[WEB-3998] feat: settings page revamp #6959
Conversation
WalkthroughThis update introduces a comprehensive reorganization and enhancement of workspace, profile, and project settings across backend and frontend. It adds new settings categories, modularizes preferences (theme, start-of-week, language/timezone), restructures navigation and layout for settings pages, updates API and UI for user preferences, and adjusts routing, permissions, and redirects throughout the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Store
User->>Frontend: Navigate to /[workspaceSlug]/settings/account
Frontend->>Store: Fetch user and workspace settings (includes workspace name/logo)
Store->>Backend: Request user settings data
Backend-->>Store: Respond with user settings including workspace metadata
Store-->>Frontend: Provide user and workspace data
Frontend->>Frontend: Render settings layout, header, sidebar, preferences
User->>Frontend: Update a preference (e.g., theme)
Frontend->>Store: Update preference state
Store->>Backend: Persist updated preference
Backend-->>Store: Confirm update
Store-->>Frontend: Update UI with new preference
Suggested labels
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sangeethailango
changed the title
…/makeplane/plane into feat-settings-revamp
…akeplane/plane into feat-settings-revamp
…t-settings-revamp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx (1)
48-55: Consider adding error boundary and loading states.While the current implementation handles authorization well, consider adding loading states for when
currentProjectDetailsis being fetched and error boundaries for potential drag-and-drop failures.return ( <SettingsContentWrapper> <PageHead title={pageTitle} /> + {!currentProjectDetails ? ( + <div className="flex items-center justify-center h-32"> + <div className="text-custom-text-300">Loading...</div> + </div> + ) : ( <div ref={scrollableContainerRef} className="h-full w-full gap-10"> <ProjectSettingsLabelList /> </div> + )} </SettingsContentWrapper> );web/app/[workspaceSlug]/(settings)/settings/projects/page.tsx (1)
20-20: Consider adding alt text localization.For better internationalization support, consider making the alt text translatable.
- <Image src={resolvedPath} alt="No projects yet" width={384} height={250} /> + <Image src={resolvedPath} alt={t("projects.empty_state.alt_text")} width={384} height={250} />Note: This would require adding the
useTranslationhook import and usage.web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx (1)
26-28: Consider removing redundant opacity styling for unauthorized users.The opacity reduction on line 33 may be unnecessary since users without admin permissions already see the
NotAuthorizedViewand never reach the features list rendering.if (workspaceUserInfo && !canPerformProjectAdminActions) { return <NotAuthorizedView section="settings" isProjectView />; }web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx (1)
24-24: Maintain consistency with other settings pages.The features page returns
nullfor missing parameters, but this returns an empty fragment. Consider usingnullfor consistency.- if (!workspaceSlug || !projectId) return <></>; + if (!workspaceSlug || !projectId) return null;web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx (1)
67-67: Improve accessibility by providing context for opacity change.The opacity reduction to indicate non-admin status lacks accessibility context. Screen readers won't understand why the content appears faded.
Consider adding an accessible indicator:
- <div className={`w-full ${isAdmin ? "" : "opacity-60"}`}> + <div className={`w-full ${isAdmin ? "" : "opacity-60"}`} aria-label={!isAdmin ? "Read-only view - Admin permissions required to edit" : undefined}>web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx (1)
53-53: Remove redundant opacity styling since unauthorized users see a different view.The opacity styling is redundant because users without admin permissions see the
NotAuthorizedViewcomponent instead of this content.Simplify the styling since unauthorized users are already redirected:
- <section className={`w-full ${canPerformProjectAdminActions ? "" : "opacity-60"}`}> + <section className="w-full">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/intake/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/page.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/sidebar.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/layout.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/page.tsx(1 hunks)web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx(1 hunks)web/core/components/project/card.tsx(2 hunks)web/core/components/settings/project/sidebar/nav-item-children.tsx(1 hunks)web/core/components/settings/project/sidebar/root.tsx(1 hunks)web/core/components/settings/tabs.tsx(1 hunks)web/core/components/workspace/sidebar/projects-list-item.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/core/components/workspace/sidebar/projects-list-item.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/intake/page.tsx
- web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/page.tsx
- web/core/components/project/card.tsx
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx
- web/core/components/settings/tabs.tsx
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/page.tsx
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/page.tsx
- web/core/components/settings/project/sidebar/nav-item-children.tsx
- web/core/components/settings/project/sidebar/root.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/app/[workspaceSlug]/(settings)/settings/projects/layout.tsx (5)
web/core/store/router.store.ts (2)
projectId(85-87)workspaceSlug(69-71)web/core/store/project/project.store.ts (1)
joinedProjectIds(216-227)web/core/components/settings/mobile/nav.tsx (1)
SettingsMobileNav(13-46)web/core/components/settings/project/sidebar/root.tsx (1)
ProjectSettingsSidebar(15-49)web/core/components/settings/helper.ts (1)
getProjectActivePath(50-56)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx (6)
packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)packages/types/src/project/projects.d.ts (1)
IProject(39-55)web/core/services/project/project.service.ts (1)
updateProject(77-83)web/core/components/settings/content-wrapper.tsx (1)
SettingsContentWrapper(11-59)web/core/components/automation/auto-archive-automation.tsx (1)
AutoArchiveAutomation(25-125)web/core/components/automation/auto-close-automation.tsx (1)
AutoCloseAutomation(24-191)
🔇 Additional comments (12)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx (2)
16-29: Excellent permission handling and component structure!The component correctly implements permission checks and derives the page title dynamically. The permission logic appropriately restricts access to users with ADMIN or MEMBER roles at the project level.
31-42: Auto-scroll implementation looks good with proper cleanup.The drag-and-drop auto-scroll is correctly implemented using the
combineutility, which ensures proper cleanup when the component unmounts. The effect properly checks for element existence before setting up the auto-scroll.web/app/[workspaceSlug]/(settings)/settings/projects/page.tsx (1)
14-17: Theme-aware image handling is well implemented.The conditional image path based on theme is a good UX practice. The resolved theme check properly handles both dark and light modes.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (1)
15-29: Good permission logic and internationalization usage.The component properly implements permission checks and uses internationalization for the "States" label. The permission logic correctly restricts access to ADMIN or MEMBER roles at the project level.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (2)
19-24: Excellent granular permission handling!The permission logic is well-structured, combining both project-level and workspace-level permissions. This provides appropriate access control where workspace admins can manage project members even if they're not project members themselves.
33-36: Clean component composition with proper semantic structure.The use of semantic
<section>element and clean separation ofProjectSettingsMemberDefaultsandProjectMemberListcomponents follows good React composition patterns.web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx (1)
14-42: LGTM: Well-structured component with proper permission handling.The component follows good practices with:
- Proper MobX observer pattern
- Null checks for required parameters
- Permission-based authorization
- Dynamic page title generation
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx (1)
33-33: Good implementation of disabled state.The use of both
pointer-events-noneandopacity-60properly disables interaction while providing visual feedback when users lack admin permissions.web/app/[workspaceSlug]/(settings)/settings/projects/layout.tsx (1)
18-44: Well-designed layout component with proper authorization.The layout effectively handles:
- Auto-redirect for missing project IDs
- Mobile navigation integration
- Proper authorization wrapping
- Conditional sidebar rendering
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/sidebar.tsx (2)
30-43: Excellent loading state implementation.The skeleton loader with proper placeholder items provides good UX while waiting for user permissions to load.
50-68: Good permission-based navigation rendering.The sidebar properly:
- Checks permissions for each link before rendering
- Uses internationalization for labels
- Implements proper active state highlighting
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx (1)
32-32: 🛠️ Refactor suggestionAdd validation before type conversion to prevent runtime errors.
The function converts
workspaceSlugandprojectIdto strings without validating they exist, which could cause runtime errors if the params are undefined.Add proper validation:
- const handleChange = async (formData: Partial<IProject>) => { - if (!workspaceSlug || !projectId || !projectDetails) return; + const handleChange = async (formData: Partial<IProject>) => { + if (!workspaceSlug || !projectId || !projectDetails) { + console.warn("Missing required parameters for project update"); + return; + }Likely an incorrect or invalid review comment.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/layout.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/sidebar.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (1)
13-42: 🛠️ Refactor suggestionAdd loading state handling for better UX
The component doesn't handle loading states when
currentProjectDetailsor user permissions are being fetched, which could result in flickering or incomplete renders.Consider adding loading state handling:
const MembersSettingsPage = observer(() => { // store const { currentProjectDetails } = useProject(); const { workspaceUserInfo, allowPermissions } = useUserPermissions(); + + // Show loading state while data is being fetched + if (!currentProjectDetails || workspaceUserInfo === undefined) { + return <div className="flex justify-center items-center h-64">Loading...</div>; + } // derived values const pageTitle = currentProjectDetails?.name ? `${currentProjectDetails?.name} - Members` : undefined;
♻️ Duplicate comments (5)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx (1)
32-42: Move the DND logic to the ProjectSettingsLabelsList component.The auto-scroll setup for drag-and-drop functionality should be moved to the
ProjectSettingsLabelsListcomponent for better separation of concerns and modularity.web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (1)
42-44: Type safety issue persists from previous review.The URL parameters still require proper type validation as identified in the previous review. The use of
.toString()indicates these parameters might not be strings, which could cause runtime issues.This is the same type safety concern raised in the previous review about validating
workspaceSlugandprojectIdbefore using them.web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (1)
31-31: SettingsContentWrapper sizing is consistent with past decisionsThis component correctly uses
size="lg"for a member list page, which aligns with the established pattern for table/list pages as identified in previous reviews.web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx (2)
29-29: Fix incomplete permission check - missing workspace and project context.The permission check is missing the required workspace and project context parameters, making it less precise than expected for project-level permissions.
Based on standard permission check patterns, the
allowPermissionscall should include workspace and project context:- const canPerformProjectAdminActions = allowPermissions([EUserPermissions.ADMIN], EUserPermissionsLevel.PROJECT); + const canPerformProjectAdminActions = allowPermissions( + [EUserPermissions.ADMIN], + EUserPermissionsLevel.PROJECT, + workspaceSlug?.toString(), + projectId?.toString() + );
34-40: Enhance error handling with proper logging and user feedback.The error handling catches all errors but doesn't log them for debugging purposes, and provides a generic error message that doesn't help users understand what went wrong.
Improve error handling:
- await updateProject(workspaceSlug.toString(), projectId.toString(), formData).catch(() => { + await updateProject(workspaceSlug.toString(), projectId.toString(), formData).catch((error) => { + console.error("Failed to update project automation settings:", error); setToast({ type: TOAST_TYPE.ERROR, title: "Error!", - message: "Something went wrong. Please try again.", + message: error?.message || "Failed to update automation settings. Please try again.", }); });
🧹 Nitpick comments (6)
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (1)
23-25: Remove duplicate comment.There's a duplicate "// derived values" comment on both lines 23 and 25.
const { t } = useTranslation(); // derived values const pageTitle = currentProjectDetails?.name ? `${currentProjectDetails?.name} - States` : undefined; - // derived values const canPerformProjectMemberActions = allowPermissions(web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx (3)
10-11: MoveSettingsContentWrapperimport to the components section.The
SettingsContentWrapperis a component but is imported after the "// hooks" comment, which could cause confusion.// components import { EUserPermissions, EUserPermissionsLevel } from "@plane/constants"; import { NotAuthorizedView } from "@/components/auth-screens"; import { PageHead } from "@/components/core"; import { EstimateRoot } from "@/components/estimates"; +import { SettingsContentWrapper } from "@/components/settings"; // hooks -import { SettingsContentWrapper } from "@/components/settings"; import { useProject, useUserPermissions } from "@/hooks/store";
14-15: Add TypeScript types for better type safety.Consider adding type annotations for the component and params to improve type safety and catch potential runtime errors.
-const EstimatesSettingsPage = observer(() => { - const { workspaceSlug, projectId } = useParams(); +const EstimatesSettingsPage: React.FC = observer(() => { + const { workspaceSlug, projectId } = useParams<{ workspaceSlug: string; projectId: string }>();
24-24: Returnnullinstead of empty fragment.For cleaner code, return
nullinstead of an empty fragment when params are missing.- if (!workspaceSlug || !projectId) return <></>; + if (!workspaceSlug || !projectId) return null;web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx (2)
10-12: Fix import organization -SettingsContentWrapperis not a hook.The
SettingsContentWrapperimport is placed under the "// hooks" comment, but it's a component, not a hook.-// hooks -import { SettingsContentWrapper } from "@/components/settings"; -import { useProject, useUserPermissions } from "@/hooks/store"; +import { SettingsContentWrapper } from "@/components/settings"; +// hooks +import { useProject, useUserPermissions } from "@/hooks/store";
33-33: Remove redundant opacity styling.The conditional opacity styling is unnecessary since non-admin users are already redirected by the permission check above.
- <section className={`w-full ${canPerformProjectAdminActions ? "" : "opacity-60"}`}> + <section className="w-full">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
web/app/[workspaceSlug]/(settings)/settings/(workspace)/billing/page.tsx(2 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx(3 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx(3 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx(2 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx(3 hunks)web/app/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx(3 hunks)web/app/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx(3 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx(1 hunks)web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx(1 hunks)web/core/components/auth-screens/not-authorized-view.tsx(1 hunks)web/core/layouts/default-layout/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- web/app/[workspaceSlug]/(settings)/settings/(workspace)/billing/page.tsx
- web/app/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx
- web/app/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx
- web/app/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx
- web/app/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx
🔇 Additional comments (21)
web/core/components/auth-screens/not-authorized-view.tsx (3)
15-15: LGTM: Well-defined optional styling prop.The addition of the optional
classNameprop follows TypeScript best practices and maintains backwards compatibility.
19-19: LGTM: Clean destructuring implementation.The destructuring is properly updated to include the new
classNameparameter while maintaining a clean function signature.
26-26: LGTM: Proper prop forwarding to layout component.The
classNameprop is correctly forwarded to theDefaultLayoutcomponent, enabling external styling customization while following React patterns for prop delegation.web/app/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx (3)
11-11: LGTM! Proper import added for layout wrapper.The import for
SettingsContentWrapperis correctly added to support the new layout structure.
33-33: Good improvement to unauthorized view styling.Adding explicit
className="h-auto"provides better height control for the unauthorized state, which is consistent with the layout improvements across the settings pages.
37-49: Excellent refactoring to standardize layout structure.The introduction of
SettingsContentWrapperwithsize="lg"creates a consistent layout pattern across settings pages. The removal ofoverflow-y-autofrom the inner div (line 40) is appropriate since the wrapper component likely handles scroll behavior centrally, improving the overall user experience and maintainability.web/core/layouts/default-layout/index.tsx (3)
2-2: LGTM: Clean utility import.The import of the
cnutility from@plane/utilsis correctly added to support class name merging functionality.
7-7: LGTM: Well-typed optional prop.The optional
classNameprop is properly typed asstringand follows React conventions for allowing external styling.
10-14: LGTM: Clean implementation with proper class merging.The component correctly:
- Destructures the new
classNameparameter- Uses the
cnutility to merge base classes with the optionalclassNameprop- Preserves the existing gradient conditional logic
- Maintains proper TypeScript typing
This enhancement successfully adds external styling capability while maintaining backward compatibility.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx (2)
16-30: Well-structured component setup with proper permission handling.The component correctly uses MobX observer pattern, sets up store hooks appropriately, and implements proper permission checking for project-level access control.
48-55: Clean render structure with appropriate component composition.The render logic is well-organized with proper use of the settings wrapper, page head component, and scrollable container setup. The component composition follows React best practices.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (2)
15-33: Well-structured permission handling and authorization logic.The component correctly:
- Uses MobX observer pattern for reactive updates
- Implements proper permission checks using
allowPermissions- Handles unauthorized access with
NotAuthorizedView- Follows the permission-based rendering pattern consistent with other settings pages
35-47: Clean render logic with proper layout structure.The render logic is well-organized with:
- Proper use of
SettingsContentWrapperfor consistent layout- Internationalized text using the translation function
- Clean header structure with border styling
- Conditional rendering based on parameter availability
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (1)
19-24: Permission logic is well-structured and secureThe permission logic correctly implements the business rule where either project members/admins OR workspace admins can manage project members. The separation of concerns between project-level and workspace-level permissions is clear and maintainable.
web/app/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx (3)
7-7: Great improvement! Consistent unauthorized handling implemented.The addition of
NotAuthorizedViewcomponent addresses the previous feedback about having consistent unauthorized components across settings pages. The early return pattern with the dedicated component is much cleaner than the previous inline implementation.Also applies to: 35-35
10-10: Excellent! Layout consistency achieved with SettingsContentWrapper.The implementation properly addresses the previous feedback by wrapping the main content in
SettingsContentWrapper. This ensures consistent styling and layout across all workspace settings pages. Thesize="lg"prop usage is appropriate for this content type.Also applies to: 38-38, 52-52
31-33: Hook usage is actually correct - clarifying previous feedback.Regarding the previous comment about "calling a hook conditionally" - the
useSWRhook is actually being called correctly here. The hook itself is always invoked, but with conditional parameters (key and fetcher). This is a valid SWR pattern where:
- When
isAdminis false, the key becomesnulland SWR won't fetch- When
isAdminis true, SWR will fetch the dataThis pattern is recommended by SWR documentation for conditional fetching and doesn't violate the Rules of Hooks.
web/app/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx (4)
15-15: LGTM: Clean architectural improvement.The import of
SettingsContentWrapperaligns well with the settings page revamp objective, promoting consistent layout patterns across settings pages.
58-58: LGTM: Consistent wrapper implementation.The
SettingsContentWrapperprovides a clean abstraction for settings page layouts, replacing the previous React fragment. This promotes consistency across the settings section.Also applies to: 104-104
90-98: Excellent UX enhancement to the empty state.The improvements to
DetailedEmptyStatesignificantly enhance user experience:
- Custom styling with
className="!px-0 py-5"- Appropriate sizing with
size="sm"- Actionable primary button that directly enables webhook creation
This makes the empty state more engaging and functional.
60-60:✅ Verification successful
Verify scroll behavior after removing overflow-y-auto.
The removal of
overflow-y-autoclass suggests that scroll handling is now managed bySettingsContentWrapper. Ensure the parent wrapper properly handles content overflow to prevent layout issues.
🏁 Script executed:
#!/bin/bash # Description: Check SettingsContentWrapper implementation for overflow handling # Expected: Find the component definition and verify it handles scrolling appropriately # Search for SettingsContentWrapper component definition ast-grep --pattern 'const SettingsContentWrapper = $_' ast-grep --pattern 'function SettingsContentWrapper($_) { $$$ }' # Check for overflow handling in the component rg -A 10 -B 5 'overflow' --type tsx --type ts | rg -A 10 -B 5 'SettingsContentWrapper'Length of output: 4328
Scroll behavior is covered by SettingsContentWrapper – no further action required
SettingsContentWrapper’s root
<div>already includesoverflow-y-scroll(see web/core/components/settings/content-wrapper.tsx:50), so removingoverflow-y-autofrom the child won’t break scrolling.
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx
Outdated
Show resolved
Hide resolved
web/app/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (1)
web/core/components/core/modals/workspace-image-upload-modal.tsx (1)
74-80: Good toast notification addition, but address previous feedback on error logging.The toast notification is a great improvement for user experience. However, the previous review comment about adding more descriptive error logging hasn't been fully addressed.
The console.log still doesn't indicate where the error originated in the frontend. Consider updating it to:
- console.log("error", error); + console.log("Error uploading workspace image:", error);This would help with debugging and address the previous reviewer's concern about identifying the error source in the frontend.
🧹 Nitpick comments (40)
web/app/(all)/[workspaceSlug]/(settings)/settings/account/page.tsx (1)
24-29: Add error handling for better user experience.The component lacks error handling for cases where user data fails to load. Consider adding error boundary or error state handling to improve user experience.
Consider adding error handling:
+ const { data: currentUser, userProfile, error } = useUser(); + + if (error) { + return ( + <div className="grid h-full w-full place-items-center px-4 sm:px-0"> + <div className="text-center"> + <p className="text-red-500">{t("error.loading_profile")}</p> + </div> + </div> + ); + }web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx (3)
17-17: Add TypeScript types for better type safety.The component lacks TypeScript type definitions. Consider adding proper types for improved type safety and developer experience.
Example implementation:
-const ProfileActivityPage = observer(() => { +const ProfileActivityPage: React.FC = observer(() => {Additionally, consider defining interfaces for the state and callback functions.
73-75: Enhance accessibility for the load more button.Add an
aria-labelto improve accessibility for screen reader users.- <Button variant="accent-primary" size="sm" onClick={handleLoadMore}> + <Button variant="accent-primary" size="sm" onClick={handleLoadMore} aria-label={t("load_more_activities")}> {t("load_more")} </Button>
15-15: Consider making PER_PAGE configurable.The hardcoded value of 100 items per page might not be optimal for all use cases. Consider making this configurable through environment variables or user preferences.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/preferences/page.tsx (1)
21-33: Remove unnecessary fragment wrapperThe fragment wrapper
<>...</>is redundant here since it contains only a single child element.Apply this diff to simplify the JSX:
- <> - <div className="flex flex-col gap-4 w-full"> - <div> - <ProfileSettingContentHeader title={t("preferences")} /> - <PreferencesList /> - </div> - <div> - <ProfileSettingContentHeader title={t("language_and_time")} /> - <LanguageTimezone /> - </div> - </div> - </> + <div className="flex flex-col gap-4 w-full"> + <div> + <ProfileSettingContentHeader title={t("preferences")} /> + <PreferencesList /> + </div> + <div> + <ProfileSettingContentHeader title={t("language_and_time")} /> + <LanguageTimezone /> + </div> + </div>web/app/(all)/[workspaceSlug]/(settings)/settings/account/notifications/page.tsx (2)
13-13: Consider moving service instantiation inside the component or using a singleton pattern.The
UserServiceinstance is created outside the component, which could lead to potential issues with module-level side effects or memory leaks in server-side rendering contexts.Consider one of these approaches:
-const userService = new UserService(); - export default function ProfileNotificationPage() { + const userService = new UserService(); const { t } = useTranslation();Or create a singleton service:
-const userService = new UserService(); +const getUserService = () => UserService.getInstance(); export default function ProfileNotificationPage() { const { t } = useTranslation(); // fetching user email notification settings - const { data, isLoading } = useSWR("CURRENT_USER_EMAIL_NOTIFICATION_SETTINGS", () => - userService.currentUserEmailNotificationSettings() + const { data, isLoading } = useSWR("CURRENT_USER_EMAIL_NOTIFICATION_SETTINGS", () => + getUserService().currentUserEmailNotificationSettings() );
22-24: Optimize loading condition logic.The condition
!data || isLoadingmight be redundant since SWR typically handles the data/loading states appropriately. WhenisLoadingis true,datais usually undefined anyway.- if (!data || isLoading) { + if (isLoading) { return <EmailSettingsLoader />; }However, if you want to be explicit about both conditions for clarity, the current approach is also acceptable.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/security/page.tsx (3)
101-101: Use strict equality for enum comparison.- getPasswordStrength(password) != E_PASSWORD_STRENGTH.STRENGTH_VALID || + getPasswordStrength(password) !== E_PASSWORD_STRENGTH.STRENGTH_VALID ||
113-113: Simplify password match error display logic for better UX.The current logic for showing password mismatch errors is complex and might show errors while the user is still typing. Consider showing the error only after the field loses focus or when the user has typed enough characters.
- const renderPasswordMatchError = !isRetryPasswordInputFocused || confirmPassword.length >= password.length; + const renderPasswordMatchError = !isRetryPasswordInputFocused && confirmPassword.length > 0;This ensures the error only shows after the user has finished typing in the confirm password field.
Also applies to: 233-235
109-109: Use strict equality for consistency.- getPasswordStrength(password) != E_PASSWORD_STRENGTH.STRENGTH_VALID && ( + getPasswordStrength(password) !== E_PASSWORD_STRENGTH.STRENGTH_VALID && (web/app/(all)/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx (2)
40-44: Simplify the SWR fetcher function.The condition check is duplicated in both the SWR key and fetcher function. The fetcher should assume it's only called when the key is valid.
const { data: tokens } = useSWR( workspaceSlug && canPerformWorkspaceAdminActions ? API_TOKENS_LIST(workspaceSlug.toString()) : null, - () => - workspaceSlug && canPerformWorkspaceAdminActions ? apiTokenService.getApiTokens(workspaceSlug.toString()) : null + () => apiTokenService.getApiTokens(workspaceSlug.toString()) );
91-95: Consider removing the duplicate "Add Token" button.The empty state includes a primary button that duplicates the functionality of the "Add Token" button in the header. Having two buttons with identical actions on the same view may confuse users.
Consider removing the primaryButton from the DetailedEmptyState since users can use the header button:
<DetailedEmptyState title={t("workspace_settings.empty_state.api_tokens.title")} description={t("workspace_settings.empty_state.api_tokens.description")} assetPath={resolvedPath} className="w-full !px-0 justify-center mx-auto" size="sm" - primaryButton={{ - text: t("workspace_settings.settings.api_tokens.add_token"), - onClick: () => setIsCreateTokenModalOpen(true), - }} />web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/mobile-header-tabs.tsx (1)
20-20: Consider extracting z-index to a constant.The hardcoded
z-10might conflict with other UI elements. Consider defining z-index values in a central constants file for better maintainability.web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/sidebar.tsx (1)
59-62: Consider documenting the special case for /settings route.The isActive function has special handling for the exact
/settingsroute vs nested routes. Consider adding a comment explaining why this distinction is necessary.isActive={(data: { href: string }) => + // Exact match for /settings root, regex match for nested routes data.href === "/settings" ? pathname === `/${workspaceSlug}${data.href}/` : new RegExp(`^/${workspaceSlug}${data.href}/`).test(pathname) }web/app/(all)/[workspaceSlug]/(settings)/settings/account/layout.tsx (1)
15-33: Consider aligning component name with file pathThe component is named
ProfileSettingsLayoutbut is located in theaccountfolder. Consider renaming either the component or the folder to maintain consistency and avoid confusion.If this is meant to be for account settings:
-const ProfileSettingsLayout = observer((props: Props) => { +const AccountSettingsLayout = observer((props: Props) => {And update the export:
-export default ProfileSettingsLayout; +export default AccountSettingsLayout;web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (1)
33-33: Remove unnecessary template literalThe template literal is not needed here since there's no string interpolation.
- <section className={`w-full`}> + <section className="w-full">web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx (2)
18-18: Consider removing unused variable or adding null check earlier.The
workspaceUserInfovariable is only used to check existence on line 32. Consider either:
- Removing it if not needed elsewhere
- Moving the null check earlier in the component for consistency with other permission checks
- const { workspaceUserInfo, allowPermissions } = useUserPermissions(); + const { allowPermissions } = useUserPermissions();Also applies to: 32-33
40-42: Remove redundant opacity styling.The opacity styling is unnecessary since unauthorized users will see the
NotAuthorizedViewcomponent instead of this content.- <div - className={cn("w-full", { - "opacity-60": !canPerformWorkspaceMemberActions, - })} - > + <div className="w-full">web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx (2)
14-14: Remove incomplete comment.The comment
// routerappears to be incomplete or leftover from previous code.- // router
17-17: Add workspaceUserInfo check for consistency.For consistency with the ExportsPage and to prevent potential issues, consider checking if
workspaceUserInfoexists before evaluating permissions.- const { allowPermissions } = useUserPermissions(); + const { workspaceUserInfo, allowPermissions } = useUserPermissions();- if (!isAdmin) return <NotAuthorizedView section="settings" className="h-auto" />; + if (workspaceUserInfo && !isAdmin) return <NotAuthorizedView section="settings" className="h-auto" />;Also applies to: 22-22
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx (2)
24-24: Return null instead of empty fragment.For better React practices and consistency, return
nullinstead of an empty fragment when conditions aren't met.- if (!workspaceSlug || !projectId) return <></>; + if (!workspaceSlug || !projectId) return null;
33-33: Use cn() helper for consistency.For consistency with other settings pages (like ExportsPage), use the
cn()helper function for conditional class names.First, add the import:
import { NotAuthorizedView } from "@/components/auth-screens"; import { PageHead } from "@/components/core"; import { EstimateRoot } from "@/components/estimates"; +// helpers +import { cn } from "@/helpers/common.helper"; // hooks import { SettingsContentWrapper } from "@/components/settings";Then update the className:
- <div className={`w-full ${canPerformProjectAdminActions ? "" : "pointer-events-none opacity-60"}`}> + <div className={cn("w-full", { + "pointer-events-none opacity-60": !canPerformProjectAdminActions, + })}>web/app/(all)/[workspaceSlug]/(settings)/settings/projects/page.tsx (1)
9-36: Consider renaming the component to better reflect its purpose.The component is named
ProjectSettingsPagebut it actually renders an empty state for when there are no projects. A more descriptive name likeProjectsEmptyStateorNoProjectsPagewould better communicate its purpose.web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/[webhookId]/page.tsx (2)
29-32: Address or remove the TODO comment.There's commented-out code with a TODO that appears to handle clearing a secret key based on an
isCreatedparameter. This unfinished functionality should either be implemented or removed to maintain code cleanliness.Would you like me to help implement this functionality or open an issue to track this TODO?
38-43: Add type safety for URL parameters.Consider adding type guards or validation for
workspaceSlugandwebhookIdto ensure they are strings before using them.useSWR( - workspaceSlug && webhookId && isAdmin ? `WEBHOOK_DETAILS_${workspaceSlug}_${webhookId}` : null, + workspaceSlug && webhookId && isAdmin ? `WEBHOOK_DETAILS_${workspaceSlug.toString()}_${webhookId.toString()}` : null, workspaceSlug && webhookId && isAdmin ? () => fetchWebhookById(workspaceSlug.toString(), webhookId.toString()) : null );web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx (1)
31-33: Simplify the SWR fetcher functionThe conditional logic in the fetcher function is redundant since SWR won't call the fetcher when the key is falsy.
- const { data: appIntegrations } = useSWR(workspaceSlug && isAdmin ? APP_INTEGRATIONS : null, () => - workspaceSlug && isAdmin ? integrationService.getAppIntegrationsList() : null - ); + const { data: appIntegrations } = useSWR( + workspaceSlug && isAdmin ? APP_INTEGRATIONS : null, + () => integrationService.getAppIntegrationsList() + );web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx (2)
31-35: Consider simplifying the type annotation forisAuthorized.The variable
isAuthorizedis typed asboolean | string, but the expression always returns a boolean value. The type annotation could be misleading.- const isAuthorized: boolean | string = + const isAuthorized: boolean = pathname && workspaceSlug && userWorkspaceRole && WORKSPACE_SETTINGS_ACCESS[accessKey]?.includes(userWorkspaceRole as EUserWorkspaceRoles);
49-49: Remove redundant CSS classes.The combination of
sm:hiddenandhiddenis redundant sincehiddenalready hides the element on all screen sizes.- <div className="flex-shrink-0 overflow-y-hidden sm:hidden hidden md:block lg:block"> + <div className="flex-shrink-0 overflow-y-hidden hidden md:block">web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx (1)
26-39: Remove dead code for non-admin opacity styling.The opacity styling on line 33 for non-admin users is unreachable code since users without admin permissions are redirected to
NotAuthorizedViewon lines 26-28.return ( <SettingsContentWrapper> <PageHead title={pageTitle} /> - <section className={`w-full ${canPerformProjectAdminActions ? "" : "opacity-60"}`}> + <section className="w-full"> <ProjectFeaturesList workspaceSlug={workspaceSlug.toString()} projectId={projectId.toString()} isAdmin={canPerformProjectAdminActions} /> </section> </SettingsContentWrapper> );web/app/(all)/[workspaceSlug]/(settings)/settings/projects/layout.tsx (1)
26-31: Verify that using the first project in the array is the intended behavior.The redirect logic uses
joinedProjectIds[0]which assumes the array is consistently ordered. Consider if this should use a more deterministic approach like the most recently accessed project or a default project setting.Consider implementing a more user-friendly approach:
- Store the last accessed project in user preferences
- Allow users to set a default project
- Sort projects by last accessed time
This would provide a better user experience than always redirecting to the first project in the array.
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (1)
24-25: Remove duplicate comment.There's a duplicate "// derived values" comment that should be removed for clarity.
// derived values const pageTitle = currentProjectDetails?.name ? `${currentProjectDetails?.name} - States` : undefined; - // derived values const canPerformProjectMemberActions = allowPermissions(web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx (3)
43-44: Remove duplicate comment.There's a duplicate "// derived values" comment that should be removed.
- // derived values const pageTitle = projectDetails?.name ? `${projectDetails?.name} - Automations` : undefined;
34-40: Enhance error handling with specific error messages.The current error handling only shows a generic message. Consider providing more context about what went wrong.
- await updateProject(workspaceSlug.toString(), projectId.toString(), formData).catch(() => { + await updateProject(workspaceSlug.toString(), projectId.toString(), formData).catch((error) => { setToast({ type: TOAST_TYPE.ERROR, title: "Error!", - message: "Something went wrong. Please try again.", + message: error?.message || "Failed to update project automation settings. Please try again.", }); });
53-53: Remove redundant opacity styling.The opacity styling on line 53 appears redundant since users without admin permissions are already redirected to
NotAuthorizedViewon line 47.- <section className={`w-full ${canPerformProjectAdminActions ? "" : "opacity-60"}`}> + <section className="w-full">web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx (2)
31-36: Address TODO comment about API call necessity.The TODO comment suggests this API call might be redundant. If
currentProjectDetailsis already available from the store, consider removing this fetch.Would you like me to help verify if this API call is necessary by checking how
currentProjectDetailsis populated in the store?
87-87: Simplify redundant null fallback.Using nullish coalescing with
nullas the fallback value is redundant.- handleDelete={() => setSelectedProject(currentProjectDetails.id ?? null)} + handleDelete={() => setSelectedProject(currentProjectDetails.id)}web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx (3)
88-88: Fix unnecessary template literal.The template literal is redundant when using the nullish coalescing operator.
- message: `${err.error ?? t("something_went_wrong_please_try_again")}`, + message: err.error ?? t("something_went_wrong_please_try_again"),
111-114: Remove redundant opacity styling.The opacity styling based on
canPerformWorkspaceMemberActionsis unnecessary since unauthorized users are already redirected at line 98-100.- <section - className={cn("w-full h-full", { - "opacity-60": !canPerformWorkspaceMemberActions, - })} - > + <section className="w-full h-full">
128-128: Consider removing autoFocus from search input.The
autoFocusattribute might be disruptive to users who navigate using keyboard or screen readers, as it automatically moves focus away from the expected tab order.- autoFocusweb/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/page.tsx (1)
14-26: Consider adding loading state for better UX.While the current implementation handles undefined workspace gracefully, you might want to add a loading state if workspace data is fetched asynchronously.
Example implementation:
const WorkspaceSettingsPage = observer(() => { // store hooks - const { currentWorkspace } = useWorkspace(); + const { currentWorkspace, isLoading } = useWorkspace(); const { t } = useTranslation(); // derived values const pageTitle = currentWorkspace?.name ? t("workspace_settings.page_label", { workspace: currentWorkspace.name }) : undefined; + if (isLoading) { + return ( + <SettingsContentWrapper> + <PageHead title={t("workspace_settings.loading")} /> + <div>Loading workspace settings...</div> + </SettingsContentWrapper> + ); + } + return ( <SettingsContentWrapper> <PageHead title={pageTitle} /> <WorkspaceDetails /> </SettingsContentWrapper> ); });Note: This assumes the
useWorkspacehook provides anisLoadingstate. Adjust based on your actual store implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
packages/i18n/src/locales/en/translations.json(6 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/intake/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/billing/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/integrations/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/mobile-header-tabs.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/sidebar.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/[webhookId]/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/notifications/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/preferences/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/security/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/sidebar.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/automations/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/estimates/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/page.tsx(1 hunks)web/core/components/core/modals/workspace-image-upload-modal.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/page.tsx
🔇 Additional comments (33)
web/core/components/core/modals/workspace-image-upload-modal.tsx (2)
1-1: Flag inconsistent AI summary.The AI-generated summary describes changes to an
EstimateEmptyScreencomponent, but this file contains aWorkspaceImageUploadModalcomponent. The summary appears to be for a different file entirely.Likely an incorrect or invalid review comment.
11-11: Good addition of toast imports for user notifications.Adding the toast imports enables better user experience by showing error messages directly to users instead of just logging to console.
web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx (1)
71-71:✅ Verification successful
URL restructuring looks good, but verify route consistency.
The path change from
/projects/{projectId}/settings/featuresto/settings/projects/{projectId}/featuresaligns well with the settings page revamp objective and centralizes settings navigation.Please verify that:
- The new route pattern is consistently implemented across all settings-related navigation
- The target route
/settings/projects/{projectId}/featuresactually exists and is properly defined
🏁 Script executed:
#!/bin/bash # Description: Verify the new settings route structure is consistently used across the codebase # Expected: All settings navigation should use the new `/settings/projects/` pattern echo "=== Checking for old route patterns that may need updating ===" rg -n "/${workspaceSlug}/projects/.*?/settings" --type tsx --type ts --type js --type jsx echo -e "\n=== Checking for new route pattern usage ===" rg -n "/${workspaceSlug}/settings/projects" --type tsx --type ts --type js --type jsx echo -e "\n=== Checking if the target route definition exists ===" fd -e tsx -e ts | xargs rg -l "settings/projects/.*?/features"Length of output: 1211
I’d like to double-check that there are no lingering old URLs and that the actual route file is in place. Please run:
🏁 Script executed:
#!/bin/bash echo "=== Checking for any remaining old `/projects/{id}/settings` usages ===" rg -n "/projects/.*/settings" -g "*.tsx" echo -e "\n=== Locating the new Next.js route definition for features ===" find web/app -type f -path "*/settings/projects/*/features/page.tsx"Length of output: 597
Routing update verified – no legacy routes remain
I’ve confirmed that:
- There are no remaining imports or links using the old
/projects/{projectId}/settingspattern.- The new feature route is defined at
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/page.tsxEverything is consistent with the settings-centric URL structure.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/page.tsx (3)
1-11: Clean imports and setup structure.The component imports are well-organized and follow good practices with proper separation of concerns. The use of
"use client"directive is appropriate for this interactive settings component.
12-30: Well-structured component with MobX observer pattern.The component follows React best practices with proper use of MobX observer for reactive state management. The conditional rendering and component composition are clean and maintainable.
26-26:✅ Verification successful
Verify translation keys exist.
Ensure that the translation keys
"profile.label"and"general_settings"are properly defined in the i18n configuration.
🏁 Script executed:
#!/bin/bash # Description: Check if translation keys exist in the i18n files # Expected: Find both translation keys in locale files rg -r '$1' '"(profile\.label|general_settings)"' --type jsonLength of output: 1709
🏁 Script executed:
#!/bin/bash # Search for nested "profile" entries in translation files to verify existence of "profile.label" rg -n '"profile"' --type json packages/i18n/src/locales -A2Length of output: 9165
Translation keys confirmed in i18n files
Both"profile.label"and"general_settings"are defined across all locale JSONs underpackages/i18n/src/locales. No further action is needed.web/app/(all)/[workspaceSlug]/(settings)/settings/account/notifications/page.tsx (3)
1-12: Well-organized imports with good separation of concerns.The imports are logically grouped and the "use client" directive is correctly placed for Next.js app router client components. The import structure follows good practices with clear separation between external libraries, internal components, and services.
15-36: Clean component structure with good separation of concerns.The component follows React best practices with proper hook usage, conditional rendering, and clean JSX structure. The use of internationalization and dedicated UI components shows good architectural planning.
28-32:✅ Verification successful
Verify translation keys exist in the i18n configuration.
The component uses several translation keys that should be verified to exist in the internationalization configuration.
🏁 Script executed:
#!/bin/bash # Description: Check if the translation keys used in this component exist in the i18n files # Expected: Find corresponding entries for the translation keys echo "Searching for translation keys used in the notifications page..." # Search for translation key definitions rg -A 2 -B 2 '"profile\.label"' rg -A 2 -B 2 '"notifications"' rg -A 2 -B 2 '"email_notifications"' rg -A 2 -B 2 '"stay_in_the_loop_on_issues_you_are_subscribed_to_enable_this_to_get_notified"' echo "Checking i18n files structure..." fd -e json -e ts -e js | grep -E "(i18n|locale|translation)" | head -10Length of output: 56669
🏁 Script executed:
#!/bin/bash # Verify that 'profile.label' is defined in the default locale translations (English) echo "Searching for 'profile.label' in packages/i18n/src/locales/en/translations.json..." rg -n -C2 '"profile"' packages/i18n/src/locales/en/translations.jsonLength of output: 497
Translation keys verified in i18n configuration
All translation keys used in
notifications/page.tsxare present in the default English locale:
profile.label(packages/i18n/src/locales/en/translations.json line 1494–1496)notifications(packages/i18n/src/locales/en/translations.json)email_notifications(packages/i18n/src/locales/en/translations.json)stay_in_the_loop_on_issues_you_are_subscribed_to_enable_this_to_get_notified(packages/i18n/src/locales/en/translations.json)No missing keys—no action required.
web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/page.tsx (1)
64-64:✅ Verification successful
Verify all navigation paths have been updated to the new settings structure.
The URL path change from
/${workspaceSlug}/projects/${projectId}/settings/featuresto/${workspaceSlug}/settings/projects/${projectId}/featuresaligns with the settings page revamp objectives. This creates a more logical hierarchy where all settings are grouped under a single/settings/path.Run the following script to ensure all old settings paths have been updated:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old settings URL pattern # Search for old URL pattern in TypeScript/JavaScript files echo "Searching for old settings URL pattern..." rg -t ts -t tsx -t js -t jsx '/projects/[^/]+/settings' --glob '!node_modules' -A 2 -B 2 # Also check for string literals or template literals with the old pattern rg -t ts -t tsx -t js -t jsx 'projects.*settings.*features' --glob '!node_modules' -A 2 -B 2Length of output: 334
🏁 Script executed:
#!/bin/bash # Verifying no old settings paths remain—using file globs instead of unsupported type flags echo "Searching for any references to the old nested settings path..." rg -n "/projects/[^/]+/settings/features" -g "*.{ts,tsx,js,jsx}" --glob "!node_modules" -A2 -B2 || echo "No matches for old features URL." echo echo "Searching for any other references to the old settings path (excluding the new `/settings/projects`)…" rg -n "/projects/[^/]+/settings" -g "*.{ts,tsx,js,jsx}" --glob "!node_modules" -A2 -B2 \ | grep -v "/settings/projects" \ || echo "No leftover old settings paths found."Length of output: 832
No leftover old settings URLs—change approved.
I searched the codebase for both the old
/projects/{workspaceSlug}/settings/featurespattern and any remaining/projects/{workspaceSlug}/settingsoccurrences (excluding the new/settings/projects) and found none. The updated route
/${workspaceSlug}/settings/projects/${projectId}/features
is applied consistently.web/app/(all)/[workspaceSlug]/(settings)/settings/account/sidebar.tsx (1)
58-58: 🛠️ Refactor suggestionAdd null check for avatar_url to prevent potential issues.
The current condition doesn't check for
null, which could cause the avatar to display incorrectly ifavatar_urlis null.- {!currentUser?.avatar_url || currentUser?.avatar_url === "" ? ( + {!currentUser?.avatar_url || currentUser?.avatar_url === "" || currentUser?.avatar_url === null ? (Likely an incorrect or invalid review comment.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/security/page.tsx (1)
71-98: Well-implemented password change handler with proper security measures.The implementation correctly handles CSRF token retrieval, conditional old password requirement, and provides comprehensive error handling with localized messages.
web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/intake/page.tsx (1)
45-45:✅ Verification successful
URL restructuring looks good - verify route consistency.
The URL path update from
/${workspaceSlug}/projects/${projectId}/settings/featuresto/${workspaceSlug}/settings/projects/${projectId}/featuresaligns with the broader settings page restructuring effort described in the PR.Please verify that:
- The new route structure is properly defined in the Next.js app directory
- All other references to the old URL pattern have been consistently updated
🏁 Script executed:
#!/bin/bash # Description: Verify all references to the old URL pattern have been updated and new routes exist # Check for any remaining references to the old URL pattern rg -A 3 -B 3 "projects/.*?/settings/features" --type tsx --type ts # Check for new route definitions matching the updated pattern fd "settings" --type d | head -10 # Search for the new URL pattern to confirm consistent usage rg -A 3 "settings/projects/.*?/features" --type tsx --type tsLength of output: 651
I’ll re-run the checks using proper file globs for
.ts/.tsxand list the route dirs underweb/app:
🏁 Script executed:
#!/bin/bash set -e echo "=== Old URL references (projects/.../settings/features) ===" rg -n "projects/.*/settings/features" -g "*.tsx" -g "*.ts" || echo "→ None found" echo echo "=== New URL references (settings/projects/.../features) ===" rg -n "settings/projects/.*/features" -g "*.tsx" -g "*.ts" || echo "→ None found" echo echo "=== Feature route directories under web/app ===" fd "features" -t d web/app | grep "/settings/projects/" || echo "→ None found"Length of output: 1596
URL restructuring verified across the codebase
All references to the old path have been removed, and the new URL structure is consistently applied:
- No occurrences of
projects/.../settings/featuresremain.- All
router.pushcalls in:
modules/(list)/page.tsxviews/(list)/page.tsxintake/page.tsxcycles/(list)/page.tsxpages/(list)/page.tsx
have been updated to/${workspaceSlug}/settings/projects/${projectId}/features.- The feature route directory exists at:
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/features/Everything looks consistent—ready to merge.
web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/page.tsx (1)
72-72: Consistent URL restructuring for cycles feature navigation.The URL path update matches the pattern seen in other files, changing from
/${workspaceSlug}/projects/${projectId}/settings/featuresto/${workspaceSlug}/settings/projects/${projectId}/features. This maintains consistency across the settings page restructuring effort.The change correctly preserves the existing permission check (
hasAdminLevelPermission) and user experience flow, only updating the navigation destination to align with the new URL hierarchy.packages/i18n/src/locales/en/translations.json (3)
46-47: LGTM! Consistent terminology update.The renaming from "Appearance" to "Preferences" is consistent with the broader scope of settings that now includes language and timezone options.
60-63: Well-structured user guidance.The new translation keys provide clear explanations about the relocated settings and helpful navigation hints for users.
1455-1456: Consistent terminology update to industry standard.The change from "API Tokens" to "Personal Access Tokens" aligns with industry standards and is more descriptive for end users. All related messages have been updated consistently.
Also applies to: 1462-1471, 1477-1478
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/mobile-header-tabs.tsx (1)
21-37: Well-implemented permission-aware navigation tabs.The tab rendering logic properly checks permissions and workspace context before rendering each link. The active state styling provides good visual feedback.
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/sidebar.tsx (2)
32-35: Good defensive programming with null checks.The component properly handles undefined types and missing icons, preventing runtime errors.
53-56: Clean permission-based filtering.The category filtering logic clearly restricts FEATURES and DEVELOPER categories to admin users only, providing appropriate access control.
web/app/(all)/[workspaceSlug]/(settings)/layout.tsx (2)
9-23: Well-structured layout component with proper authentication flow!The component correctly implements authentication and workspace authorization checks before rendering settings content. The responsive layout with flex containers is appropriate for a settings page.
18-18:✅ Verification successful
Verify the custom Tailwind class
md:py-page-yThe class
md:py-page-yappears to be a custom spacing value. Ensure this is defined in your Tailwind configuration.
🏁 Script executed:
#!/bin/bash # Description: Check if md:py-page-y is defined in Tailwind config # Search for page-y definition in Tailwind config files fd -e js -e ts -e json "tailwind.config" --exec grep -l "page-y" # Also check for any CSS files that might define this custom value rg -g "*.css" "page-y" -A 2Length of output: 136
🏁 Script executed:
#!/bin/bash # Inspect the definition of the custom spacing key `page-y` in the Tailwind config rg -C3 "page-y" packages/tailwind-config/tailwind.config.jsLength of output: 202
Confirmed custom spacing
page-yis definedThe Tailwind config at
packages/tailwind-config/tailwind.config.jsincludes:spacing: { // … "page-x": "1.35rem", "page-y": "1.35rem", },No further action required.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/layout.tsx (1)
1-9: Component implementation follows good patternsweb/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx (1)
19-28: Excellent permission checking implementation!The permission logic correctly checks for project-level permissions (admin/member) OR workspace admin permissions, following the principle of least privilege. The conditional rendering of
NotAuthorizedViewprovides good UX for unauthorized users.web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx (1)
45-45:⚠️ Potential issueMissing translation function call for header text.
The header text displays the translation key instead of the translated text. You need to wrap it with the
t()function.- <h3 className="text-xl font-medium">{t("workspace_settings.settings.exports.title")}</h3> + <h3 className="text-xl font-medium">{t("workspace_settings.settings.exports.title")}</h3>Wait, I see the
t()function is already being used. The issue might be that the same translation key is used for both page title and header. Please verify if this is intentional or if a different key should be used for the header.web/app/(all)/[workspaceSlug]/(settings)/settings/projects/page.tsx (1)
1-39: Major inconsistency between AI summary and actual implementation.The AI summary describes this component as a comprehensive project settings management page with features like:
- Permission checks using
useUserPermissions- Project data fetching with
useSWR- Archive/restore and delete modals
ProjectDetailsFormfor editingHowever, the actual implementation is a simple empty state component that only shows when there are no projects. This suggests either:
- The wrong file content was provided
- The AI summary is incorrect
- There's been a significant change in implementation
Please verify that the correct file content is being reviewed.
Likely an incorrect or invalid review comment.
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx (1)
47-49: Good implementation of cleanup logic.The
useEffecthook properly cleans up the webhook secret key when the modal is closed, preventing potential security issues from lingering sensitive data.web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/labels/page.tsx (1)
19-19:❌ Incorrect review comment
Verify workspaceUserInfo availability
The code uses
workspaceUserInfoin the permission check, but it's not destructured from theuseUserPermissionshook on line 19. This could lead to a runtime error.Also applies to: 44-46
🏁 Script executed:
#!/bin/bash # Description: Check the return value of useUserPermissions hook to verify if workspaceUserInfo is available # Test: Search for the useUserPermissions hook implementation ast-grep --pattern 'export $_ function useUserPermissions($$$) { $$$ return { $$$ } }' # Also check for the hook definition with different patterns rg -A 20 'useUserPermissions.*=' --glob '**/hooks/store/**'Length of output: 684
🏁 Script executed:
#!/bin/bash # Locate the IUserPermissionStore definition to see if workspaceUserInfo is part of its shape rg -n "IUserPermissionStore" -A 5 -B 5Length of output: 5597
🏁 Script executed:
#!/bin/bash # Search for allowPermissions in the codebase rg -n "allowPermissions" -A 5 -B 5Length of output: 66169
Ignore workspaceUserInfo availability concern
The
useUserPermissionshook’s return type (IUserPermissionStore) includesworkspaceUserInfo, and you already destructure it on line 19:const { workspaceUserInfo, allowPermissions } = useUserPermissions();There is no missing import or destructuring—no action required.
Likely an incorrect or invalid review comment.
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/billing/page.tsx (1)
14-24: Clean permission handling implementationThe permission checking logic is well-structured and follows a consistent pattern with proper workspace-level admin verification.
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx (1)
15-48: Well-structured permission-aware component!The component follows best practices:
- Proper permission checks before rendering sensitive content
- Clean conditional rendering with early returns
- Proper null checks before converting params to strings
- Good use of internationalization
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx (1)
1-26: LGTM! Well-organized imports.The imports are properly structured and grouped by type (external packages, types, UI components, helpers, hooks).
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/page.tsx (2)
1-29: Well-structured workspace settings page component!The implementation follows React and Next.js best practices with proper use of MobX observer pattern and internationalization support.
17-19:✅ Verification successful
Verify the translation key exists in i18n configuration.
The translation key
workspace_settings.page_labelwith interpolation parameterworkspaceshould be defined in your translation files.
🏁 Script executed:
#!/bin/bash # Description: Verify that the translation key 'workspace_settings.page_label' exists in translation files # Search for the translation key in common translation file locations echo "Searching for translation key 'workspace_settings.page_label'..." fd -e json -e ts -e js | xargs rg -l "workspace_settings" | head -20 # Look for the specific key pattern rg -A 2 -B 2 'workspace_settings.*page_label' --type-add 'trans:*.{json,ts,js}' -t transLength of output: 1366
🏁 Script executed:
#!/bin/bash # Verify that workspace_settings.page_label exists in each locale translations.json echo "Checking 'workspace_settings.page_label' across all locale files..." for file in packages/i18n/src/locales/*/translations.json; do echo echo "File: $file" python3 - "$file" << 'EOF' import json, sys data = json.load(open(sys.argv[1])) value = data.get("workspace_settings", {}).get("page_label") print("→", value if value is not None else "<missing>") EOF doneLength of output: 5402
Translation key confirmed in all locales
Verified that
workspace_settings.page_labelis defined in everytranslations.jsonfile underpackages/i18n/src/locales/*/translations.json, each using the{workspace}placeholder correctly. No further action required.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/account/preferences/page.tsx
Outdated
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/account/preferences/page.tsx
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx
Outdated
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx
Outdated
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/page.tsx
Outdated
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx
Outdated
Show resolved
Hide resolved
web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/members/page.tsx
Outdated
Show resolved
Hide resolved
…lane into feat-settings-revamp
…t-settings-revamp
gakshita
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx (2)
36-47: 🛠️ Refactor suggestionPerformance concern: Multiple components rendered simultaneously.
The current implementation creates and renders all
ProfileActivityListPagecomponents at once, which can cause performance issues as the number of pages grows.Consider implementing virtualization or lazy loading:
- Use a single component that manages pagination internally
- Implement windowing with libraries like
react-window- Load pages only when needed rather than all at once
- const activityPages: JSX.Element[] = []; - for (let i = 0; i < pageCount; i++) - activityPages.push( - <ProfileActivityListPage - key={i} - cursor={`${PER_PAGE}:${i}:0`} - perPage={PER_PAGE} - updateResultsCount={updateResultsCount} - updateTotalPages={updateTotalPages} - updateEmptyState={updateEmptyState} - /> - ); + // Render only the current page or implement virtualization + <ProfileActivityListPage + key={pageCount - 1} + cursor={`${PER_PAGE}:${pageCount - 1}:0`} + perPage={PER_PAGE} + updateResultsCount={updateResultsCount} + updateTotalPages={updateTotalPages} + updateEmptyState={updateEmptyState} + />
39-46: 🛠️ Refactor suggestionMissing error handling for component failures.
The component doesn't handle potential errors when
ProfileActivityListPagecomponents fail to load data.Implement error boundaries or error states to gracefully handle loading failures:
+ const [hasError, setHasError] = useState(false); + + const handleError = () => setHasError(true); + + if (hasError) { + return ( + <div className="flex h-full w-full flex-col"> + <ProfileSettingContentHeader title={t("activity")} /> + <div className="text-center text-custom-text-400 mt-8"> + {t("error_loading_activity")} + </div> + </div> + ); + } <ProfileActivityListPage key={i} cursor={`${PER_PAGE}:${i}:0`} perPage={PER_PAGE} updateResultsCount={updateResultsCount} updateTotalPages={updateTotalPages} updateEmptyState={updateEmptyState} + onError={handleError} />
🧹 Nitpick comments (3)
web/core/components/settings/heading.tsx (2)
3-12: Well-designed Props interface with good flexibility.The Props type definition is comprehensive and provides good flexibility:
titleaccepts both string and ReactNode for versatility- Optional props are properly typed
- Button configuration is well-structured
Consider adding JSDoc comments to document the expected behavior of each prop.
14-27: Consider accessibility improvements for the heading structure.The component implementation is solid, but consider these accessibility enhancements:
- Add proper heading hierarchy support
- Consider ARIA attributes for better screen reader support
- The conditional title rendering could benefit from consistent heading levels
export const SettingsHeading = ({ title, description, button, appendToRight, showButton = true }: Props) => ( - <div className="flex items-center justify-between border-b border-custom-border-100 pb-3.5"> + <div className="flex items-center justify-between border-b border-custom-border-100 pb-3.5" role="banner"> <div className="flex flex-col items-start gap-1"> - {typeof title === "string" ? <h3 className="text-xl font-medium">{title}</h3> : title} + {typeof title === "string" ? <h3 className="text-xl font-medium" id="settings-heading">{title}</h3> : title} - {description && <div className="text-sm text-custom-text-300">{description}</div>} + {description && <div className="text-sm text-custom-text-300" aria-describedby="settings-heading">{description}</div>} </div>web/core/components/settings/layout.tsx (1)
20-24: Consider simplifying the scroll condition logic.The condition
container.scrollHeight > container.clientHeight || scrolledRef.currentmight be clearer with better documentation or simplification.Consider adding a comment to explain the logic:
const handleScroll = () => { const scrollTop = container.scrollTop; + // Only update scroll state if content is scrollable or was previously scrolled if (container.scrollHeight > container.clientHeight || scrolledRef.current) { const _isScrolled = scrollTop > 0; toggleIsScrolled(_isScrolled); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/(with-sidebar)/layout.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/(with-sidebar)/sidebar.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/header.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(projects)/settings/(with-sidebar)/layout.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(projects)/settings/(with-sidebar)/sidebar.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(projects)/settings/header.tsx(0 hunks)web/app/(all)/[workspaceSlug]/(settings)/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx(2 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx(3 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx(1 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx(3 hunks)web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx(2 hunks)web/ce/components/workspace/billing/root.tsx(2 hunks)web/core/components/estimates/empty-screen.tsx(2 hunks)web/core/components/labels/project-setting-label-list.tsx(3 hunks)web/core/components/project/member-list.tsx(2 hunks)web/core/components/project/settings/features-list.tsx(2 hunks)web/core/components/settings/content-wrapper.tsx(1 hunks)web/core/components/settings/header.tsx(1 hunks)web/core/components/settings/heading.tsx(1 hunks)web/core/components/settings/index.ts(1 hunks)web/core/components/settings/layout.tsx(1 hunks)web/core/components/settings/sidebar/root.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- web/app/(all)/[workspaceSlug]/(projects)/settings/(with-sidebar)/layout.tsx
- web/app/(all)/[workspaceSlug]/(projects)/settings/header.tsx
- web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/(with-sidebar)/sidebar.tsx
- web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/(with-sidebar)/layout.tsx
- web/app/(all)/[workspaceSlug]/(projects)/settings/(with-sidebar)/sidebar.tsx
- web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/settings/header.tsx
✅ Files skipped from review due to trivial changes (4)
- web/core/components/project/settings/features-list.tsx
- web/ce/components/workspace/billing/root.tsx
- web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/webhooks/page.tsx
- web/app/(all)/[workspaceSlug]/(settings)/settings/account/api-tokens/page.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- web/core/components/settings/index.ts
- web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/imports/page.tsx
- web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/states/page.tsx
- web/core/components/labels/project-setting-label-list.tsx
- web/core/components/settings/content-wrapper.tsx
- web/core/components/estimates/empty-screen.tsx
- web/core/components/settings/header.tsx
- web/app/(all)/[workspaceSlug]/(settings)/layout.tsx
- web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/exports/page.tsx
- web/app/(all)/[workspaceSlug]/(settings)/settings/(workspace)/layout.tsx
- web/core/components/settings/sidebar/root.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/components/project/member-list.tsx (2)
web/core/components/settings/heading.tsx (1)
SettingsHeading(14-27)packages/i18n/src/store/index.ts (1)
t(211-232)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
web/core/components/project/member-list.tsx (2)
16-16: Good refactoring to use standardized component.The import of
SettingsHeadingaligns with the broader effort to standardize header layouts across settings pages.
52-79: Excellent refactoring that maintains functionality while improving consistency.The replacement of the manual header div with
SettingsHeadingis well-executed:
- The title is properly passed using the translation function
- The search input and add member button are preserved in the
appendToRightprop- All styling, event handlers, and conditional logic remain intact
- Admin permission check for the add button is maintained
This change improves UI consistency across the settings pages while preserving all existing functionality.
web/core/components/settings/heading.tsx (1)
29-29: Export pattern is appropriate.Both named and default exports provide flexibility for different import styles across the codebase.
web/app/(all)/[workspaceSlug]/(settings)/settings/account/activity/page.tsx (2)
17-32: Good state management and callback structure.The component properly manages pagination state and provides clear callback functions for child components to update parent state. The state structure is logical and the naming is clear.
51-64: Well-implemented empty state handling.The empty state implementation provides good user experience with:
- Proper conditional rendering
- Localized messages
- Consistent styling with the rest of the page
- Appropriate use of DetailedEmptyState component
web/core/components/settings/layout.tsx (4)
6-12: Good use of MobX observer and proper hook integration.The component is properly wrapped with
observerfor reactivity and correctly uses theuseUserSettingshook to manage scroll state.
13-36: Excellent scroll event handling with proper optimization.The implementation demonstrates good practices:
- Throttling with 150ms delay to improve performance
- Proper cleanup of event listeners and throttled functions
- Guard clause for container existence
- Scrollability check before updating state
The scroll logic correctly identifies when scrolling occurs and updates the global state accordingly.
38-40: Proper state synchronization between ref and store.The effect correctly synchronizes the ref with the store state, ensuring consistency between local and global scroll state.
41-46: Clean and functional layout structure.The layout provides a simple, scrollable container with proper styling classes. The full height and overflow-y-scroll are appropriate for a settings content area.
Description
New Features
Improvements
Bug Fixes
Style
Chores
Documentation
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
SettingsHeading,DetailedEmptyState, andSettingsSidebarNavItem.Style
Chores