[WEB-5170] feat: navigation revamp #8162
Conversation
… to handle updation of multiple keys
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReworks workspace navigation: removes AppRail context/hook, adds AppRailRoot and withDockItems HOC, introduces top nav and tabbed project navigation (per-member prefs), adds navigation customization UI and hooks/stores/services, replaces CommonProjectBreadcrumbs with BreadcrumbLink usage, updates routing and many sidebar/header components. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant WorkspaceLayout as Workspace Layout
participant TopNav as TopNavigationRoot
participant HOC as withDockItems
participant AppRail as AppRailRoot
participant Prefs as WorkspacePreferencesStore
User->>WorkspaceLayout: open workspace page
WorkspaceLayout->>TopNav: mount top navigation
TopNav->>HOC: render wrapped component
HOC-->>TopNav: inject dockItems (projects)
TopNav->>AppRail: render rail region
AppRail->>Prefs: read displayMode
Prefs-->>AppRail: displayMode
AppRail-->>User: rendered rail + dock items
sequenceDiagram
autonumber
participant User
participant ProjectPage as Project Page
participant TabNav as TabNavigationRoot
participant Prefs as Member Tab Preferences
participant Store as ProjectMemberStore
participant API as ProjectMemberPreference API
User->>ProjectPage: navigate to project root
ProjectPage->>TabNav: mount with projectId
TabNav->>Prefs: getTabPreferences(projectId)
Prefs-->>TabNav: defaultTab, hiddenTabs
TabNav->>TabNav: compute visible vs overflow (useResponsiveTabLayout)
TabNav-->>User: render visible tabs + overflow menu
User->>TabNav: set default tab / hide tab
TabNav->>Store: updateProjectMemberPreferences(...)
Store->>API: persist changes
API-->>Store: success
Store-->>Prefs: update cached prefs
Prefs-->>TabNav: updated prefs reflected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Focus areas:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/propel/src/context-menu/context-menu.tsx (1)
48-74: Fix indentation in ContextMenuContent component.The refactoring correctly separates styling concerns with
positionerClassNamefor the Positioner element andclassNamefor the Popup. Verification found existing usage already follows this pattern, confirming the behavior change is intentional and not breaking.Lines 54-58 have inconsistent indentation. Apply this diff:
const ContextMenuContent = React.forwardRef(function ContextMenuContent( { positionerClassName, className, children, side = "bottom", sideOffset = 4, ...props }: ContextMenuContentProps, ref: React.ForwardedRef<React.ElementRef<typeof ContextMenuPrimitive.Positioner>> ) { return ( <ContextMenuPrimitive.Positioner - ref={ref} - side={side} - sideOffset={sideOffset} - {...props} - className={positionerClassName} - > + ref={ref} + side={side} + sideOffset={sideOffset} + className={positionerClassName} + {...props} + > <ContextMenuPrimitive.Popup
🧹 Nitpick comments (34)
packages/ui/src/sortable/sortable.tsx (1)
39-40: Consider clarifying the comment tense.The comment says "we're removing the source item first" but the actual removal occurs later at line 44. For clarity, consider:
- // Adjust for the fact that we're removing the source item first + // Adjust for the fact that we will remove the source item nextapps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(detail)/header.tsx (1)
135-138: Consider removing duplicateisLastprop.The
isLastprop appears on both theBreadcrumbLink(line 135) and theBreadcrumbs.Item(line 138). Verify if both are necessary or if one is redundant.apps/web/core/components/navigation/project-actions-menu.tsx (2)
4-4: Remove unused imports.
useStateanduseRefare imported but the state and ref they create are not meaningfully used in this component.Apply this diff to clean up unused imports:
-import React, { useState, useRef } from "react"; +import type { FC } from "react";
34-35: Remove unused local state and ref.The
actionSectionRefandisMenuActivestate are declared but not used for any meaningful logic. TheCustomMenucomponent likely manages its own open/close state internally, making these redundant.Apply this diff to remove the unused declarations:
- const actionSectionRef = useRef<HTMLDivElement | null>(null); - const [isMenuActive, setIsMenuActive] = useState(false);Also remove the related attributes from the CustomMenu:
<CustomMenu customButton={ <span - ref={actionSectionRef} className="grid place-items-center p-0.5 text-custom-sidebar-text-400 hover:bg-custom-sidebar-background-80 rounded" - onClick={() => setIsMenuActive(!isMenuActive)} > <MoreHorizontal className="size-4" /> </span> } className="flex-shrink-0" customButtonClassName="grid place-items-center" placement="bottom-start" ariaLabel={t("aria_labels.projects_sidebar.toggle_quick_actions_menu")} useCaptureForOutsideClick closeOnSelect - onMenuClose={() => setIsMenuActive(false)} >apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/layout.tsx (1)
1-14: Directly renderingProjectWorkItemDetailsHeadersimplifies this layoutWiring the layout to render
ProjectWorkItemDetailsHeaderdirectly (instead of going throughAppHeaderindirection) aligns with the new navigation/header composition and keeps this route consistent with the shared header implementation inheader.tsx.Minor nit: the component name
ProjectIssueDetailsLayoutno longer matches the/browse/[workItem]route semantics; if you touch this file again, consider renaming it to something work‑item–specific for clarity.apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/header.tsx (1)
42-52: Remove redundantisLastprop.Both the
BreadcrumbLink(line 48) andBreadcrumbs.Item(line 51) have theisLastprop. This is redundant and could cause confusion about which component is actually using it.Apply this diff to remove the redundant prop:
<BreadcrumbLink label="Modules" href={`/${workspaceSlug}/projects/${projectId}/modules/`} icon={<ModuleIcon className="h-4 w-4 text-custom-text-300" />} - isLast /> } isLast />apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx (1)
39-49: Remove redundantisLastprop.Both the
BreadcrumbLink(line 45) andBreadcrumbs.Item(line 48) have theisLastprop, which is redundant.Apply this diff:
<BreadcrumbLink label="Cycles" href={`/${workspaceSlug}/projects/${currentProjectDetails?.id}/cycles/`} icon={<CycleIcon className="h-4 w-4 text-custom-text-300" />} - isLast /> } isLast />apps/web/ce/components/navigations/top-navigation-root.tsx (1)
17-20: Remove redundant padding class.The base className includes
px-3.5, and the conditional object also appliespx-3.5whenshowLabelis true, creating unnecessary duplication. Whilecn()handles this, it's cleaner to avoid the redundancy.Apply this diff to remove the redundancy:
<div - className={cn("flex items-center justify-evenly min-h-11 w-full px-3.5 z-[27] transition-all duration-300", { + className={cn("flex items-center justify-evenly min-h-11 w-full z-[27] transition-all duration-300", { "px-3.5": showLabel, "px-2": !showLabel, })}apps/web/app/(all)/[workspaceSlug]/(projects)/_sidebar.tsx (1)
35-40: Improve pathname matching to avoid false positives.The current
pathname.includes()check could match unintended routes. For example, a path like/${workspaceSlug}/projects/notifications-featurewould incorrectly trigger the early return.Consider using a more precise check:
- const isNotificationsPath = pathname.includes(`/${workspaceSlug}/notifications`); + const isNotificationsPath = pathname === `/${workspaceSlug}/notifications` || pathname.startsWith(`/${workspaceSlug}/notifications/`);Alternatively, if notifications is always a top-level route under workspace:
- const isNotificationsPath = pathname.includes(`/${workspaceSlug}/notifications`); + const isNotificationsPath = pathname.split('/')[2] === 'notifications';packages/propel/src/tab-navigation/tab-navigation-item.tsx (1)
6-30: LGTM! Clean animation implementation.The component uses
framer-motioneffectively for smooth tab transitions. The z-index layering (z-10for the container,-z-10for the background) ensures proper visual stacking.If you encounter z-index conflicts with other UI elements in the future, consider using CSS variables or a z-index scale constant to manage layering more systematically across the component library.
apps/web/core/components/workspace/sidebar/projects-list.tsx (1)
247-264: Consider UX clarity for the toggle text.The toggle button shows "Hide" when
isExtendedProjectSidebarOpenedis true, but this button is located in the main sidebar, not the extended sidebar. Users might find it unclear what "Hide" refers to since the button itself remains visible.Consider more descriptive labels:
- <span>{isExtendedProjectSidebarOpened ? "Hide" : "More"}</span> + <span>{isExtendedProjectSidebarOpened ? "Hide More" : "More"}</span>Or use a chevron icon to indicate expansion state rather than text:
<Ellipsis className="flex-shrink-0 size-4" /> - <span>{isExtendedProjectSidebarOpened ? "Hide" : "More"}</span> + <span>More</span> + <ChevronRightIcon className={cn("size-3 transition-transform", { "rotate-90": isExtendedProjectSidebarOpened })} />apps/web/core/components/workspace/sidebar/sidebar-item.tsx (1)
44-52: Consider extracting staticItems to a constant file.The
staticItemsarray is hardcoded inline and could be reused in other parts of the navigation system. Extracting it to a shared constant would improve maintainability.Consider creating a constant:
// In a constants file like @/constants/navigation.ts export const STATIC_WORKSPACE_NAVIGATION_ITEMS = [ "home", "pi_chat", "projects", "your_work", "stickies", "drafts", ] as const;Then import and use:
+import { STATIC_WORKSPACE_NAVIGATION_ITEMS } from "@/constants/navigation"; + const staticItems = [ - "home", - "pi_chat", - "projects", - "your_work", - "stickies", - "drafts", + ...STATIC_WORKSPACE_NAVIGATION_ITEMS, ...(additionalStaticItems || []), ];apps/web/ce/components/issues/header.tsx (1)
64-74: Consider i18n for the hardcoded label.The "Work Items" label is hardcoded, while other parts of this component use the
t()translation function. For consistency and internationalization support, consider using a translation key.<BreadcrumbLink - label="Work Items" + label={t("work_items")} href={`/${workspaceSlug}/projects/${projectId}/issues/`} icon={<WorkItemsIcon className="h-4 w-4 text-custom-text-300" />} isLast />Additionally, similar to other breadcrumb components in this PR, consider guarding against empty
projectIdduring loading states to prevent invalid URLs.packages/propel/src/tab-navigation/tab-navigation.stories.tsx (1)
1-99: Well-structured story; consider hoistingnavItemsfor reuse/readabilityThe story is clear, uses realistic navigation data, and nicely shows both a live example and code snippet. One minor optional improvement: you could define
navItemsat module scope (or as aconstoutside the render function) so it’s not reallocated on every render and can be reused across stories if needed.If you expect to add more stories that share the same navigation model, it may be worth confirming this extraction now to avoid duplication later.
apps/web/core/components/workspace/sidebar/sidebar-menu-items.tsx (1)
20-23: Clarify preferences assumptions and behavior offilteredStaticNavigationItemsThis block looks good conceptually (merging static items with preference-driven personal items), but there are a few points worth tightening up:
- The code assumes that
personalPreferences(andpersonalPreferences.items) andworkspacePreferences.itemsare always defined. If either hook can return a transientundefined/partial shape (e.g., during initial load), property access likepersonalPreferences.items.stickiesorworkspacePreferences.items[item.key]would throw. It may be safer to either:
- Ensure the hooks always return a fully defaulted
preferencesobject, or- Add defensive guards like
personalPreferences?.items?.stickiesand defaultsort_ordervalues.- The comment “Filter static navigation items based on personal preferences” doesn’t quite match the implementation: you’re always including
WORKSPACE_SIDEBAR_STATIC_NAVIGATION_ITEMS_LINKS, then appending extra personal items sorted bysort_order. Renaming the variable/comment to something likeaugmentedStaticNavigationItemswould better describe the behavior.sort_ordercurrently only influences the ordering within the personal items block; base static items will always precede them. If the intent is to allow personal items to interleave with or reorder base static entries, you may eventually want a single merged list with a unified sorting pass.Please confirm the hook contracts guarantee fully-populated
preferences.items, or consider adding guards/defaults here to avoid runtime errors if that assumption doesn’t always hold.Also applies to: 46-77, 79-89, 94-97
packages/propel/src/tab-navigation/tab-navigation-list.tsx (1)
1-12: Simple, focused implementation; consider making the layout group id configurableThe component is concise and does what it promises: a flex container for tab items wrapped in a layout group, with a sensible default class set and displayName for debugging.
If you anticipate multiple independent tab navigations coexisting on the same view, you might later want to expose a prop (e.g.,
layoutIdwith default"tab-navigation") so callers can avoid unintended layout-group coupling between different nav instances. Not urgent, just something to keep in mind.apps/web/ce/components/app-rail/app-rail-hoc.tsx (1)
11-34: HOC typing and injected dock item wiring look solidThe HOC correctly narrows props with
P extends WithDockItemsPropsand reintroducesdockItemsinternally, and the active-state logic viauseWorkspacePathskeeps the wrapper lean. If other components need to reference the injected prop shape directly, you might optionally exportWithDockItemsPropsfrom this module for reuse, but the current setup is perfectly fine as-is.apps/web/core/components/navigation/items-root.tsx (1)
14-20: Component behavior is sound; consider a more stable React keyThe dock item rendering and
showLabeloverride are straightforward and correct. IfAppSidebarItemDataexposes a unique field (e.g.,keyorhref), consider using that instead ofitem.labelfor the Reactkeyto avoid potential collisions if labels ever repeat.apps/web/core/components/navigation/use-responsive-tab-layout.ts (2)
64-95: RecomputevisibleCountwhen the visible navigation items change, not just when their count or width changesThe width calculation loop and smart overflow reservation look good, but the effect currently only re-runs when
containerWidthorvisibleNavigationItems.lengthchange. If the set/order/content ofvisibleNavigationItemschanges while the length and container width remain the same (e.g., items swapped, labels or icons change width),visibleCountcan become stale and not reflect the new layout.Consider tracking the items themselves in the dependency array so any material change to the visible items triggers a recomputation:
- useEffect(() => { + useEffect(() => { if (!containerWidth || itemRefs.current.length === 0) return; @@ - setVisibleCount(count); - }, [containerWidth, visibleNavigationItems.length, gap, overflowButtonWidth]); + setVisibleCount(count); + }, [containerWidth, visibleNavigationItems, gap, overflowButtonWidth]);If
visibleNavigationItemsis not referentially stable, this will still be cheap given the small tab count and avoids subtle layout desyncs.
45-61: Optional: guardResizeObserverusage for environments without itThe
ResizeObserversetup is correctly scoped touseEffect, so it won’t run during SSR. In test or older browser environments whereResizeObservermay be undefined, this hook will currently throw atnew ResizeObserver(...).If those environments are in scope, you could add a small guard:
- const resizeObserver = new ResizeObserver((entries) => { + if (typeof ResizeObserver === "undefined") return; + + const resizeObserver = new ResizeObserver((entries) => { for (const entry of entries) { setContainerWidth(entry.contentRect.width); } });Otherwise, the existing implementation is fine for modern-browser-only support.
apps/web/core/components/sidebar/resizable-sidebar.tsx (1)
47-52: Remove now-unused hover trigger state and related effectsWith the dedicated peek trigger area removed,
isHoveringTriggeris no longer set totrueanywhere (it’s only reset tofalse), and the effects that depend on it:
- The
isHoveringTriggerstate itself (Line 48).- The logic in
toggleCollapsedthat clears it.- The two
useEffectblocks that checkisCollapsed && isHoveringTriggerto callhandlePeekLeave.are effectively dead code and add complexity without behavioral impact.
You can simplify the component by removing
isHoveringTrigger, its setter usage intoggleCollapsedand the “Reset peek when dropdown/extended sidebar closes” effects that hinge on it. This will make the remaining peek logic easier to reason about.Also applies to: 87-95, 139-160
apps/web/core/components/navigation/tab-navigation-visible-item.tsx (1)
49-64: Consider internationalizing context menu text.The context menu actions use hardcoded English strings ("Clear default", "Set as default", "Hide in more menu") while the tab label uses
t(item.i18n_key)for translation. For consistency and to support non-English locales, consider adding i18n keys for these menu actions.For example:
- <span className="text-xs">{isDefault ? "Clear default" : "Set as default"}</span> + <span className="text-xs">{t(isDefault ? "navigation.clear_default" : "navigation.set_as_default")}</span>- <span className="text-xs">Hide in more menu</span> + <span className="text-xs">{t("navigation.hide_in_more_menu")}</span>apps/web/ce/components/navigations/use-navigation-items.ts (1)
46-95: Confirm guest access expectations for cycles/modules whenguest_view_all_featuresis enabled.Right now, cycles and modules are restricted to
[ADMIN, MEMBER]while other sections (work items, views, pages, intake) allowGUEST. GivenIPartialProjectexposesguest_view_all_features?: boolean, you may want to extend guest access to these items when that flag is true to keep navigation consistent with feature visibility assumptions.If guests are not supposed to see cycles/modules even with
guest_view_all_features, this is fine as-is; otherwise, consider conditionally includingEUserPermissions.GUESTin theaccessarrays based on that flag.apps/web/core/components/sidebar/sidebar-wrapper.tsx (1)
43-85: Sidebar wrapper changes look good; you can optionally lazy‑mount the dialog.The new preferences button and
CustomizeNavigationDialogwiring are straightforward and keep the sidebar API unchanged. If the dialog is heavy, you might optionally mount it only when opened:- <CustomizeNavigationDialog isOpen={isCustomizeNavDialogOpen} onClose={() => setIsCustomizeNavDialogOpen(false)} /> + {isCustomizeNavDialogOpen && ( + <CustomizeNavigationDialog + isOpen={isCustomizeNavDialogOpen} + onClose={() => setIsCustomizeNavDialogOpen(false)} + /> + )}This avoids loading dialog internals until the user actually opens it, without changing behavior.
apps/web/core/constants/fetch-keys.ts (1)
65-67: New fetch-key constants look consistent with existing patternsThe added keys for workspace workflow states, active cycles, marketplace applications, and project member preferences all follow the existing conventions (uppercased segments, function-style factories). I don’t see correctness issues here.
Note:
PROJECT_MEMBER_PREFERENCEStakesworkspaceSlugbut doesn’t use it in the key, which is consistent with otherPROJECT_*keys above. If you ever do a broader cleanup, you could consider aligning the signatures with the actual key usage, but that’s purely optional and non-blocking.Also applies to: 86-88, 144-149, 178-179
apps/web/core/components/navigation/use-active-tab.ts (1)
14-31: Hook logic is sound; consider simplifyingworkItemIdusageThe combined conditions for:
"work_items"vs"epics"based onworkItem?.is_epicandproject_id, and- path-based activation via
pathname === item.href || pathname.startsWith(item.href + "/")look reasonable and should give the right active state in both work-item and route-based contexts.
Currently,
workItemIdis only used as a truthy flag inworkItemCondition/epicConditionand isn’t compared directly. If you don’t need the id itself, you could:
- Drop
workItemIdfromUseActiveTabPropsand the conditions, or- Incorporate it explicitly (e.g., tie activation to an issue route that includes the id) to make the intent clearer.
This is non-blocking, but simplifying the signature or usage would make the hook’s behavior more self-explanatory.
Also applies to: 35-37
apps/web/app/(all)/[workspaceSlug]/(projects)/extended-sidebar.tsx (1)
22-27: Workspace navigation preferences wiring and sorting look good; one minor cleanupThe integration with
useWorkspaceNavigationPreferences, permission checks viaallowPermissions, and the pinned‑first/sort_order‑second sorting logic all look solid and match the intended behavior for a draggable extended sidebar.One small cleanup opportunity:
orderNavigationItemacceptssourceIndexbut doesn’t use it in its calculations; the logic relies only ondestinationIndexand neighboring items’sort_order. You could dropsourceIndexfrom the function signature and call sites to reduce noise and keep the API minimal.Also applies to: 28-53, 57-81, 92-108
apps/web/core/components/navigation/tab-navigation-overflow-menu.tsx (1)
68-93: Add aria-label and localize button action labelsThe codebase already uses
useTranslationand thet()function in this component. The suggestion to addaria-labelwith translated strings is valid and aligns with codebase patterns—other components (e.g.,customize-navigation-dialog.tsx,projects-list.tsx) followaria-label={t(...)}for icon buttons.At lines 78 and 90, add
aria-labelprops alongside the existingtitleattributes:
- Line 78 (Show button): Add
aria-label={t("navigation.show_tab")}- Line 90 (default button): Add
aria-label={t(isDefault ? "navigation.clear_default_tab" : "navigation.set_default_tab")}Also replace hardcoded
titlestrings with translation keys for consistency with howitem.i18n_keyis already used on line 60.apps/web/core/services/project/project-member.service.ts (1)
67-72: AligndeleteProjectMemberimplementation withPromise<void>return typeRight now the method is typed as
Promise<void>but still returnsresponse?.datafrom thedeletecall. It works (the value is effectively ignored), but the implementation is slightly misleading.You can make the intent explicit and avoid leaking the response by awaiting the delete and not returning the data:
- async deleteProjectMember(workspaceSlug: string, projectId: string, memberId: string): Promise<void> { - return this.delete(`/api/workspaces/${workspaceSlug}/projects/${projectId}/members/${memberId}/`) - .then((response) => response?.data) - .catch((error) => { - throw error?.response?.data; - }); - } + async deleteProjectMember(workspaceSlug: string, projectId: string, memberId: string): Promise<void> { + return this.delete(`/api/workspaces/${workspaceSlug}/projects/${projectId}/members/${memberId}/`) + .then(() => undefined) + .catch((error) => { + throw error?.response?.data; + }); + }apps/web/core/hooks/use-navigation-preferences.ts (1)
107-155: Confirm scope of project navigation preferences (global vs workspace-specific)
useProjectNavigationPreferencesstores preferences under a single key ("navigation_preferences_projects"), so navigation mode / visibility settings are effectively global for the user, not partitioned per workspace or project.If that’s intentional (one project navigation mode across the app), this is fine. If you expected workspace- or project-specific behavior, consider including
workspaceSlugand/orprojectIdin the storage key to avoid cross-contamination of preferences.apps/web/core/components/navigation/tab-navigation-utils.ts (1)
16-51: Tab preference helpers are robust; consider centralizing tab key/URL mappingThe helpers handle missing/malformed
localStoragestate gracefully and always fall back to a sane default (DEFAULT_TAB_KEY→ work_items,[]for hiddenTabs). Validation of the default tab againstavailableTabKeysis also sound.Two optional tweaks you might consider:
- Hoist
tabUrlMapto a top-level constant (or reuse the URLs fromuseNavigationItems) so you don’t have two separate sources of truth for tab keys → paths.- If you introduce a shared union type for tab keys, you can type
TTabPreferences.defaultTabandtabUrlMapmore strictly to catch mismatches at compile time.These are purely maintainability improvements; current behavior is fine.
Also applies to: 60-73, 82-115
apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
107-111: KeepDisclosure’s internal open state in sync withisProjectListOpenYou’re now treating
isProjectListOpenfrom the command-palette store as the source of truth (used for arrow rotation,Transition show, and programmatic opening based onURLProjectId), whileDisclosureis still uncontrolled viadefaultOpen={isProjectListOpen}and its own internal state managed byDisclosure.Button.This can lead to subtle desyncs:
- The
useEffectthat callssetIsProjectListOpen(true)whenURLProjectId === project.idwon’t updateDisclosure’s internalopenstate (sincedefaultOpenis only used on mount), so ARIA state managed by Headless UI may disagree with the visual state driven byisProjectListOpen.To avoid that, consider one of:
- Making
Disclosurecontrolled by passing an explicitopen={isProjectListOpen}andonChange={setIsProjectListOpen}, or- Dropping
Disclosureentirely if you’re already managing visibility viaTransitionand your own state, replacing the remainingDisclosure.Buttonusages with plain buttons.This would keep accessibility attributes and visual state aligned.
Also applies to: 227-229, 241-242, 408-431, 434-451
apps/web/core/store/member/project/base-project-member.store.ts (2)
39-41: Clarify whetherprojectMemberPreferencesMapis per‑project or per‑member
projectMemberPreferencesMapis keyed only byprojectId, but both fetch/update APIs take amemberId. This works well if the store is intended to hold navigation preferences only for “the current member” per project, but it will overwrite data if you ever need preferences for multiple members of the same project at once (e.g., an admin editing others’ defaults).If the scope is “current user only”, consider documenting that in the type or via a comment. Otherwise, consider changing the shape to a nested map like
{ [projectId]: { [memberId]: IProjectMemberNavigationPreferences } }.Also applies to: 94-97, 439-446
88-90: Consider makingprojectMemberFetchStatusMapobservable for full reactivityUnrelated to this PR but visible here:
projectMemberFetchStatusMapis used bygetProjectMemberFetchStatus, yet it isn’t registered inmakeObservable. That means observers callinggetProjectMemberFetchStatus(projectId)won’t react to changes unless something else causes a re‑render.If you rely on that getter in reactive views, you may want to include:
makeObservable(this, { - projectMemberMap: observable, + projectMemberMap: observable, + projectMemberFetchStatusMap: observable,to ensure fetch status updates propagate correctly.
Also applies to: 109-123, 163-163
.../app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx
Show resolved
Hide resolved
...b/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx
Show resolved
Hide resolved
...b/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/header.tsx
Show resolved
Hide resolved
| disabled={disabled} | ||
| className={cn( | ||
| "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none focus:bg-custom-background-80", | ||
| "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify keyboard accessibility after removing focus background.
Removing focus:bg-custom-background-80 combined with outline-none eliminates visual focus indicators for keyboard navigation. This creates an accessibility barrier for users navigating menu items with the Tab key, potentially violating WCAG 2.4.7 (Focus Visible).
Please confirm:
- Is this removal intentional as part of the navigation styling overhaul?
- Are focus states being handled by a parent component or global styles?
- Has keyboard navigation been tested to ensure focus remains visible?
If no alternative focus indicator exists, consider restoring a focus state or adding a different visual cue:
- "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none",
+ "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 focus:bg-custom-background-80 cursor-pointer outline-none",Or use a focus ring instead:
- "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none",
+ "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 focus:bg-custom-background-80 cursor-pointer focus:outline-2 focus:outline-custom-primary",🏁 Script executed:
#!/bin/bash
# Examine the full MenuItem component and surrounding context
cat packages/propel/src/menu/menu.tsx | head -80Length of output: 2415
🏁 Script executed:
#!/bin/bash
# Search for focus-related styling across the codebase
rg -n "focus:" --type=tsx --type=ts -C 3 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for BaseMenu.Item to understand if parent provides focus handling
rg -n "BaseMenu\.Item" --type=tsx --type=ts -C 5Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for other menu-related components and their focus handling
fd -e tsx -e ts | xargs rg -l "menu|Menu" --type=tsx --type=ts | head -20Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check for outline-none usage patterns in the codebase
rg -n "outline-none" --type=tsx --type=ts -C 2 | head -80Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for focus styling using glob pattern instead of --type flag
rg -n "focus:" -g "*.tsx" -g "*.ts" -C 3 packages/propel/src/menu/ | head -60
Length of output: 1513
🏁 Script executed:
#!/bin/bash
# Check git to see what was removed from this file
git diff HEAD packages/propel/src/menu/menu.tsx | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any focus-related patterns in the entire menu directory
rg -n "focus" -g "*.tsx" -g "*.ts" packages/propel/src/menu/ -C 2Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Search for outline-none to understand removal context
rg -n "outline-none" -g "*.tsx" -g "*.ts" packages/propel/src/ -C 2 | head -80Length of output: 7051
🏁 Script executed:
#!/bin/bash
# Get full menu.tsx file to see complete context
cat packages/propel/src/menu/menu.tsxLength of output: 6791
🏁 Script executed:
#!/bin/bash
# Check if there's a tailwind config with focus defaults
find . -name "tailwind.config.*" -o -name "tailwind.config.ts" -o -name "tailwind.config.js" | head -5Length of output: 170
🌐 Web query:
@base-ui-components BaseMenu.Item focus handling accessibility
💡 Result:
Short summary — how BaseMenu.Item handles focus & accessibility (@base-ui-components):
-
Keyboard & ARIA behavior: Menu and its items implement WAI‑ARIA keyboard patterns (arrow keys, Home/End, typeahead, Enter/Esc). Items are navigable/highlighted by keyboard; use the Item.label prop to control typeahead matching. [1][2]
-
Rendering & roles: Menu.Item renders as a
by default (but you can replace it via the render prop). Visual/semantic behavior (highlighting, keyboard activation) is provided by the component; when you render a native or , set nativeButton={true} so the component treats it as a native control. [2] -
Focus management on open/close: The Menu.Popup has finalFocus (true | false | ref | function) to control where focus goes when the menu closes (default returns focus to trigger or previously focused element). Components also manage focus internally after interactions. [1][2]
-
Visual focus indicator: Base UI handles focus movement but not styling — you must provide visible focus styles (use :focus/:focus-visible) to meet WCAG guidelines. [1][3]
-
Data/state hooks for styling & a11y: Items expose state attributes (e.g., data-highlighted, data-disabled) so you can style highlighted/focused items and convey state to assistive tech. Use render prop to pass/forward aria attributes if replacing the element. [2]
If you want, I can show a minimal example (native button vs. div) demonstrating keyboard focus, typeahead label, and finalFocus usage.
🏁 Script executed:
#!/bin/bash
# Check tailwind config for default focus utilities
cat packages/propel/tailwind.config.tsLength of output: 202
🏁 Script executed:
#!/bin/bash
# Search for data-highlighted styling in the codebase (BaseMenu.Item state)
rg -n "data-highlighted" -g "*.tsx" -g "*.ts" -g "*.css" packages/propel/src/ -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any global CSS that might style MenuItem or BaseMenu.Item
rg -n "BaseMenu" -g "*.css" -g "*.scss" packages/propel/ -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for MenuItem styling in any CSS files
fd -e css -e scss packages/propel/src/ | xargs catLength of output: 385
🏁 Script executed:
#!/bin/bash
# Look for focus styling patterns in SubMenuTrigger to understand parent context
rg -n "SubmenuTrigger\|SubMenu" -g "*.tsx" -g "*.ts" packages/propel/src/menu/ -A 5 -B 2 | head -80Length of output: 0
Restore focus indicator for MenuItem keyboard navigation.
Removing focus:bg-custom-background-80 eliminates the only visual focus indicator for MenuItem, creating an accessibility barrier. The MenuItem has outline-none (removes default browser focus ring) and only hover: styling for mouse users, leaving no keyboard focus indicator. This violates WCAG 2.4.7 (Focus Visible).
@base-ui-components does not provide default focus styling — the developer must supply it via Tailwind utilities or data-state selectors. Restore the focus state:
- "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none",
+ "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 focus:bg-custom-background-80 cursor-pointer outline-none",📝 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.
| "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 cursor-pointer outline-none", | |
| "w-full select-none truncate rounded px-1 py-1.5 text-left text-custom-text-200 hover:bg-custom-background-80 focus:bg-custom-background-80 cursor-pointer outline-none", |
🤖 Prompt for AI Agents
In packages/propel/src/menu/menu.tsx around line 47, the MenuItem class removed
the only visual keyboard focus indicator by omitting a focus state while keeping
outline-none, causing an accessibility issue; restore a visible focus style by
adding a Tailwind focus utility such as focus:bg-custom-background-80 or better
focus-visible:bg-custom-background-80 (or a data-state selector if using
radix/compound components) so keyboard users see a clear focus ring/background,
keeping outline-none if you prefer the custom style. Ensure the chosen utility
is additive to the existing class list and matches hover contrast requirements.
| sm: "before:absolute before:left-1/2 before:top-1/2 before:size-full before:min-h-11 before:-translate-x-1/2 before:-translate-y-1/2", | ||
| md: "before:absolute before:left-1/2 before:top-1/2 before:size-full before:min-h-14 before:-translate-x-1/2 before:-translate-y-1/2", | ||
| lg: "before:absolute before:left-1/2 before:top-1/2 before:size-full before:min-h-17 before:-translate-x-1/2 before:-translate-y-1/2", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify horizontal scrollbar usability without minimum width constraints.
The removal of min-w-XX classes while retaining min-h-XX creates an asymmetry: vertical scrollbar thumbs maintain a minimum height for draggability, but horizontal scrollbar thumbs no longer have a minimum width constraint. This could result in very narrow horizontal thumbs when scrollable content is short, making them harder to click or drag, particularly on touch devices or for users with motor difficulties.
Consider adding corresponding before:min-w-XX classes to maintain consistent usability across both orientations, or verify that horizontal scrollbar interaction has been tested across various content sizes and devices.
🏁 Script executed:
#!/bin/bash
# Search for horizontal ScrollArea usage to assess impact
rg -n --type=tsx --type=ts -C3 'orientation.*horizontal|scrollType' Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check the actual file content at lines 57-59
echo "=== Examining scrollarea.tsx lines 57-59 ==="
sed -n '50,65p' packages/propel/src/scrollarea/scrollarea.tsx
echo -e "\n=== Search for horizontal scrollbar references ==="
rg -n -C3 'horizontal' -g '*.ts' -g '*.tsx'
echo -e "\n=== Search for ScrollArea usage in tests ==="
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | head -20Length of output: 42989
🏁 Script executed:
#!/bin/bash
# Check where thumbSizeStyles is applied and how it's used with orientation
echo "=== Where thumbSizeStyles is applied ==="
rg -n "thumbSizeStyles" -A2 -B2 -g '*.ts' -g '*.tsx'
echo -e "\n=== Check the scroll-area component in packages/ui to compare ==="
if [ -f packages/ui/src/scroll-area.tsx ]; then
echo "=== packages/ui scroll-area.tsx thumbSizeStyles ==="
sed -n '20,30p' packages/ui/src/scroll-area.tsx
fiLength of output: 3273
Add minimum width constraints to maintain consistent scrollbar usability.
The thumbSizeStyles in packages/propel/src/scrollarea/scrollarea.tsx (lines 57-59) lacks before:min-w-XX classes, creating an asymmetry compared to the packages/ui/src/scroll-area.tsx implementation, which includes both before:min-h-XX and before:min-w-XX for all sizes.
This means horizontal scrollbar thumbs will have no minimum width constraint, risking overly narrow thumbs that are harder to interact with, especially on touch devices. Vertical thumbs retain the minimum height constraint, but the missing width constraint for horizontal scrollbars is inconsistent with the established pattern.
Add the corresponding before:min-w-11, before:min-w-14, and before:min-w-17 classes to the sm, md, and lg variants respectively to restore parity and ensure consistent usability across both orientations.
🤖 Prompt for AI Agents
In packages/propel/src/scrollarea/scrollarea.tsx around lines 57 to 59, the
thumbSizeStyles for sm/md/lg include before:min-h-XX but are missing matching
before:min-w-XX classes; add before:min-w-11 to the sm variant, before:min-w-14
to the md variant, and before:min-w-17 to the lg variant so horizontal scrollbar
thumbs have the same minimum width constraints as the vertical ones, restoring
parity with packages/ui/src/scroll-area.tsx.
anmolsinghbhatia
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/core/components/navigation/customize-navigation-dialog.tsx (1)
361-380: Fix inconsistent validation styling for empty project-count inputWhen the input is empty,
parseInt(projectCountInput)yieldsNaN, so the border goes red (line 372) but no error message is shown (line 378 guards onprojectCountInput !== ""). This produces an “invalid but unexplained” state.Treat an empty value as neutral (non-error) in the border styling:
- "text-custom-text-200", - parseInt(projectCountInput) >= 1 - ? "border-custom-border-300 focus:border-custom-primary-100 focus:ring-1 focus:ring-custom-primary-100" - : "border-red-500 focus:border-red-500 focus:ring-1 focus:ring-red-500" + "text-custom-text-200", + projectCountInput === "" || parseInt(projectCountInput, 10) >= 1 + ? "border-custom-border-300 focus:border-custom-primary-100 focus:ring-1 focus:ring-custom-primary-100" + : "border-red-500 focus:border-red-500 focus:ring-1 focus:ring-red-500"This keeps the field in a normal state while the user is clearing/typing, and only shows the red border once there’s an actual invalid numeric value.
🧹 Nitpick comments (4)
apps/web/core/components/navigation/use-tab-preferences.ts (2)
39-52: Loading state may not distinguish “no prefs” vs “not yet loaded”
isLoadingis computed as!storePreferences && memberId !== null. If the store represents “no saved preferences for this member” with a falsy value (e.g.undefinedornull), this hook will keep reportingisLoading = trueindefinitely for such users, even though there’s nothing left to load.Consider having the store expose an explicit
isLoadingPreferences/hasLoadedPreferencesflag, or using a sentinel value, so the UI can differentiate “initial fetch in progress” from “member has no saved prefs”.
88-105: Prevent duplicate hidden entries and hiding the current default tab
handleHideTabalways appendstabKeytohiddenTabswithout checking for existing entries, and it lets callers hide the currentdefaultTab. That can lead to:
- Duplicate keys in
hiddenTabsif the same tab is hidden repeatedly.- A confusing state where the default tab is also hidden from the main strip.
A small adjustment can avoid both issues:
- const handleHideTab = (tabKey: string) => { - const newPreferences = { - ...tabPreferences, - hiddenTabs: [...tabPreferences.hiddenTabs, tabKey], - }; - updatePreferences(newPreferences); - }; + const handleHideTab = (tabKey: string) => { + // No-op if already hidden + if (tabPreferences.hiddenTabs.includes(tabKey)) return; + + const newPreferences: TTabPreferences = { + ...tabPreferences, + hiddenTabs: [...tabPreferences.hiddenTabs, tabKey], + }; + + // If we hide the current default tab, reset default to the global default + if (tabKey === newPreferences.defaultTab) { + newPreferences.defaultTab = DEFAULT_TAB_KEY; + } + + updatePreferences(newPreferences); + };This keeps
hiddenTabsunique and ensures the active default tab remains visible (or at least reverts to a sensible default when hidden).apps/web/core/components/navigation/customize-navigation-dialog.tsx (2)
157-162: Prefer importingKeyboardEventtype instead of relying on globalReactnamespaceUsing
React.KeyboardEventwithout an explicitReactimport depends on ambient global typings. It’s more robust and explicit to import the event type:-import type { FC } from "react"; +import type { FC, KeyboardEvent } from "react"; @@ - const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { + const handleKeyDown = (e: KeyboardEvent<HTMLInputElement>) => {This keeps the file free of implicit globals and aligns with the modern JSX transform.
187-190: Localize hard-coded helper/description copy for full i18n coverageSeveral user-facing strings are hard-coded instead of going through
t():
- Dialog description paragraph (lines 187-190).
- Accordion mode helper text (lines 321-324).
- Horizontal mode helper text (lines 338-341).
For consistency with the rest of the file and to support localization, consider moving these into translation keys, e.g.:
<p className="mt-1 text-sm text-custom-text-300"> {t("customize_navigation.description")} </p> ... <div className="text-xs text-custom-text-300"> {t("accordion_navigation_control_help")} </div> ... <div className="text-xs text-custom-text-300"> {t("horizontal_navigation_bar_help")} </div>(With corresponding entries added to your i18n resources.)
Also applies to: 321-324, 338-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx(3 hunks)apps/web/core/components/navigation/customize-navigation-dialog.tsx(1 hunks)apps/web/core/components/navigation/use-tab-preferences.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-18T09:46:08.566Z
Learnt from: prateekshourya29
Repo: makeplane/plane PR: 7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
Applied to files:
apps/web/core/components/navigation/customize-navigation-dialog.tsxapps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.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/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
apps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx
🧬 Code graph analysis (2)
apps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx (3)
packages/propel/src/empty-state/compact-empty-state.tsx (1)
EmptyStateCompact(8-62)packages/i18n/src/store/index.ts (1)
t(223-244)apps/web/core/components/workspace/sidebar/projects-list-item.tsx (1)
SidebarProjectsListItem(54-457)
apps/web/core/components/navigation/use-tab-preferences.ts (4)
apps/web/core/components/navigation/tab-navigation-utils.ts (2)
TTabPreferences(2-5)DEFAULT_TAB_KEY(9-9)apps/space/core/hooks/store/use-member.ts (1)
useMember(7-11)apps/web/core/services/project/project-member.service.ts (2)
getProjectMemberPreferences(75-85)updateProjectMemberPreferences(87-98)packages/propel/src/toast/toast.tsx (1)
setToast(202-222)
🔇 Additional comments (5)
apps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx (2)
155-155: LGTM: isLastChild bug fix applied correctly.The calculation now correctly uses
filteredProjects.length - 1instead ofjoinedProjects.length - 1, ensuring the last visible item is properly identified during drag-and-drop operations when the list is filtered.
135-144: LGTM: Empty state implementation is clean and properly configured.The search empty state correctly uses translation keys and the
EmptyStateCompactcomponent with appropriate props.apps/web/core/components/navigation/use-tab-preferences.ts (2)
57-73: Overall hook structure and error handling look goodThe core flow—deriving
tabPreferencesfrom the store, updating viaupdateProjectMemberPreferences, and surfacing simple handlers for toggle/hide/show—is clear and matches the rest of the navigation/preferences architecture. Error handling via toast is appropriate for this layer.Also applies to: 79-83, 107-113
24-37: UsecurrentUserData.idconsistently with existing patternsThe current implementation in
use-tab-preferences.tscorrectly usesmemberId = data?.id(user ID) and matches the pattern inproject-wrapper.tsxline 93, which also callsfetchProjectMemberPreferences(workspaceSlug, projectId, currentUserData.id). The API endpoint/api/workspaces/{workspaceSlug}/projects/{projectId}/preferences/member/{memberId}/expects a user ID, not a project membership record. The commented-out intent to read fromprojectUserInfowas superseded by the simpler and consistent user ID approach now in place across the codebase.apps/web/core/components/navigation/customize-navigation-dialog.tsx (1)
41-392: Overall dialog structure and state wiring look solidThe dialog’s composition, preference hooks integration, permission-filtered workspace items, and sortable personal/workspace lists are all coherent and match the described navigation revamp. The separation of concerns between local UI state and persisted preferences is clear; only small UX/type/i18n tweaks are needed as noted in other comments.
| excludedElementId="extended-project-sidebar-toggle" | ||
| > | ||
| <div className="flex flex-col gap-1 w-full sticky top-4 pt-0 px-4"> | ||
| <div className="flex flex-col gap-1 w-full sticky top-4 pt-0"> |
There was a problem hiding this comment.
Padding inconsistency between empty state and list.
The horizontal padding differs across states:
- Header (Line 106): no horizontal padding (removed
px-4) - Empty state (Line 136):
px-6 - List (Line 146):
pl-4(no right padding)
When users switch between empty and populated states (via search), the content will shift horizontally due to mismatched left padding (6 units vs 4 units). Additionally, the list's missing right padding may cause the scrollbar to overlap content.
Consider applying consistent horizontal padding:
-<div className="flex flex-col gap-1 w-full sticky top-4 pt-0">
+<div className="flex flex-col gap-1 w-full sticky top-4 pt-0 px-4">-<div className="flex flex-col items-center mt-4 px-6 pt-10">
+<div className="flex flex-col items-center mt-4 px-4 pt-10">-<div className="flex flex-col gap-0.5 overflow-x-hidden overflow-y-auto vertical-scrollbar scrollbar-sm flex-grow mt-4 pl-4">
+<div className="flex flex-col gap-0.5 overflow-x-hidden overflow-y-auto vertical-scrollbar scrollbar-sm flex-grow mt-4 px-4">Also applies to: 136-136, 146-146
🤖 Prompt for AI Agents
In apps/web/app/(all)/[workspaceSlug]/(projects)/extended-project-sidebar.tsx
around lines 106, 136 and 146, the header, empty state and list use inconsistent
horizontal padding causing content shift and potential scrollbar overlap;
standardize horizontal spacing (pick one padding value such as px-4 or px-6) on
the main container div (line 106) and remove or adjust conflicting px/pl classes
on the empty state (line 136) and list (line 146) so all three states share the
same left and right padding; also ensure the list retains right padding to
prevent scrollbar overlap (apply symmetric px-X on the wrapper or add pr-X to
the list if keeping pl-X).
| }} | ||
| /> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Bug: Incorrect ref assignment for responsive tab layout
The itemRefs array is being populated with refs based on originalIndex from allNavigationItems, but the responsive layout hook expects refs indexed by position in visibleNavigationItems. When items are filtered by hiddenTabs, the indices don't align, causing the width calculation logic in useResponsiveTabLayout to access wrong or undefined refs. This breaks the responsive overflow behavior since itemRefs.current[i] in the hook iterates over visibleNavigationItems.length but refs are stored at allNavigationItems.indexOf(item) positions.
Additional Locations (1)
| <TabNavigationRoot | ||
| workspaceSlug={workspaceSlug} | ||
| projectId={issueDetails?.project_id?.toString() ?? ""} | ||
| /> |
There was a problem hiding this comment.
Bug: Empty projectId passed to TabNavigationRoot component
When issueDetails?.project_id is undefined, an empty string is passed to TabNavigationRoot as the projectId prop. This causes the component to execute hooks like useTabPreferences, useNavigationItems, and useProjectActions with an invalid empty projectId before the null check at line 143 returns early. These hooks may attempt API calls or state operations with the empty string, potentially causing errors or unnecessary network requests. The component should return early or handle the undefined case before rendering TabNavigationRoot.
There was a problem hiding this comment.
Bug: Missing dependencies in useEffect hooks
Two useEffect hooks call handlePeekLeave() but don't include it in their dependency arrays. They also reference isCollapsed and isHoveringTrigger without including them. This violates the exhaustive-deps rule and can cause stale closures where the effects use outdated values of these variables, leading to incorrect peek behavior when sidebar state changes.
apps/web/core/components/sidebar/resizable-sidebar.tsx#L136-L145
plane/apps/web/core/components/sidebar/resizable-sidebar.tsx
Lines 136 to 145 in 92867ba
|
|
||
| if preference: | ||
| serializer = WorkspaceUserPreferenceSerializer(preference, data=request.data, partial=True) | ||
| preference = WorkspaceUserPreference.objects.filter(key=key, workspace__slug=slug).first() |
There was a problem hiding this comment.
Bug: Missing user filter in workspace preference query
The patch method queries WorkspaceUserPreference using only key and workspace__slug, but doesn't filter by user=request.user. This allows any authenticated user to modify another user's workspace preferences, causing a security vulnerability and data corruption. The query at line 74 should include user=request.user to ensure users can only update their own preferences.
| project_member = self.get_project_member(slug, project_id, member_id) | ||
|
|
||
| current_preferences = project_member.preferences or {} | ||
| current_preferences["navigation"] = request.data["navigation"] |
There was a problem hiding this comment.
Bug: Missing validation for navigation field in request
The patch method directly accesses request.data["navigation"] without checking if the key exists, which will raise a KeyError if the client doesn't send the navigation field. This causes the API to return a 500 error instead of a proper validation error. The code should validate that navigation exists in request.data before accessing it.
| href: `/${workspaceSlug}/projects/${projectId}/intake`, | ||
| icon: IntakeIcon, | ||
| access: [EUserPermissions.ADMIN, EUserPermissions.MEMBER, EUserPermissions.GUEST], | ||
| shouldRender: !!project?.inbox_view, |
There was a problem hiding this comment.
Bug: Incorrect field name for intake feature check
The code checks project?.inbox_view but the database model field is named intake_view. This field name mismatch causes the intake navigation item to never render even when the feature is enabled in the project settings, breaking the intake feature visibility in the navigation.
…t-navigation-revamp
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/api/plane/app/urls/project.py(2 hunks)apps/api/plane/app/urls/workspace.py(0 hunks)apps/api/plane/app/views/__init__.py(1 hunks)apps/api/plane/app/views/project/member.py(1 hunks)apps/api/plane/app/views/workspace/user_preference.py(1 hunks)apps/api/plane/db/models/project.py(1 hunks)apps/api/plane/db/models/workspace.py(1 hunks)packages/i18n/src/locales/en/translations.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/plane/app/urls/workspace.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/i18n/src/locales/en/translations.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/plane/app/urls/project.py (1)
apps/api/plane/app/views/project/member.py (1)
ProjectMemberPreferenceEndpoint(305-336)
apps/api/plane/app/views/project/member.py (1)
apps/api/plane/db/models/project.py (5)
ProjectMember(192-236)ROLE(21-24)save(157-159)save(169-171)save(208-218)
apps/api/plane/app/views/__init__.py (1)
apps/api/plane/app/views/project/member.py (1)
ProjectMemberPreferenceEndpoint(305-336)
apps/api/plane/app/views/workspace/user_preference.py (4)
apps/api/plane/app/views/project/member.py (1)
patch(314-323)apps/api/plane/app/views/workspace/home.py (1)
patch(64-75)apps/api/plane/api/views/member.py (1)
patch(200-205)apps/api/plane/db/models/workspace.py (1)
WorkspaceUserPreference(410-448)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/api/plane/db/models/workspace.py (1)
413-421: Stickies user preference key looks consistentAdding
STICKIES = "stickies", "Stickies"toUserPreferenceKeysaligns with existing keys and home preferences; no issues from the model side.apps/api/plane/app/views/__init__.py (1)
17-22: ExportingProjectMemberPreferenceEndpointis appropriateIncluding
ProjectMemberPreferenceEndpointin the views package exports matches its use inurls/project.pyand keeps the public API consistent.apps/api/plane/db/models/project.py (1)
61-63: Navigation defaults inget_default_preferenceslook correctReturning both
"pages"and"navigation"(withdefault_tab: "work_items"and emptyhide_in_more_menu) is a reasonable default shape forProjectMember.preferencesand will give new records the structure expected by the navigation code.apps/api/plane/app/urls/project.py (1)
3-18: Member preference endpoint routing is wired correctlyThe import and new route:
- Use the same
(slug, project_id, member_id)parameter set as the view.- Fit the existing URL pattern style under
workspaces/<slug>/projects/<uuid:project_id>/....No routing conflicts are apparent.
Also applies to: 129-133
| class ProjectMemberPreferenceEndpoint(BaseAPIView): | ||
| def get_project_member(self, slug, project_id, member_id): | ||
| return ProjectMember.objects.get( | ||
| project_id=project_id, | ||
| member_id=member_id, | ||
| workspace__slug=slug, | ||
| ) | ||
|
|
||
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | ||
| def patch(self, request, slug, project_id, member_id): | ||
| project_member = self.get_project_member(slug, project_id, member_id) | ||
|
|
||
| current_preferences = project_member.preferences or {} | ||
| current_preferences["navigation"] = request.data["navigation"] | ||
|
|
||
| project_member.preferences = current_preferences | ||
| project_member.save(update_fields=["preferences"]) | ||
|
|
||
| return Response({"preferences": project_member.preferences}, status=status.HTTP_200_OK) | ||
|
|
||
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | ||
| def get(self, request, slug, project_id, member_id): | ||
| project_member = self.get_project_member(slug, project_id, member_id) | ||
|
|
||
| response = { | ||
| "preferences": project_member.preferences, | ||
| "project_id": project_member.project_id, | ||
| "member_id": project_member.member_id, | ||
| "workspace_id": project_member.workspace_id, | ||
| } | ||
|
|
||
| return Response(response, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
ProjectMemberPreferenceEndpoint lacks per‑member authorization and input validation
As written:
- Any workspace user with
ADMIN,MEMBER, orGUESTpermissions canPATCHorGETpreferences for anymember_idin the project, with no check that they’re updating their own record or are an admin. patchassumesrequest.data["navigation"]is present; missing/invalid payload will raise aKeyErrorand 500 instead of a 4xx.
Given this endpoint is meant for per‑member navigation preferences, you should:
- Restrict modification (and probably read) to either:
- The member themselves, or
- A workspace admin.
- Validate that a
navigationpayload is provided before updating.
One way to address this:
class ProjectMemberPreferenceEndpoint(BaseAPIView):
- def get_project_member(self, slug, project_id, member_id):
- return ProjectMember.objects.get(
- project_id=project_id,
- member_id=member_id,
- workspace__slug=slug,
- )
+ def get_project_member(self, slug, project_id, member_id):
+ return ProjectMember.objects.get(
+ project_id=project_id,
+ member_id=member_id,
+ workspace__slug=slug,
+ is_active=True,
+ )
+
+ def _is_workspace_admin(self, request, slug):
+ return WorkspaceMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ is_active=True,
+ role=ROLE.ADMIN.value,
+ ).exists()
@@
@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
def patch(self, request, slug, project_id, member_id):
project_member = self.get_project_member(slug, project_id, member_id)
- current_preferences = project_member.preferences or {}
- current_preferences["navigation"] = request.data["navigation"]
+ # Only the member themselves or a workspace admin can update these prefs
+ if request.user.id != project_member.member_id and not self._is_workspace_admin(request, slug):
+ return Response(
+ {"error": "You are not allowed to update this member's preferences."},
+ status=status.HTTP_403_FORBIDDEN,
+ )
+
+ navigation = request.data.get("navigation")
+ if navigation is None:
+ return Response(
+ {"error": "Missing 'navigation' payload."},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ current_preferences = project_member.preferences or {}
+ current_preferences["navigation"] = navigation
@@
@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
def get(self, request, slug, project_id, member_id):
project_member = self.get_project_member(slug, project_id, member_id)
+
+ if request.user.id != project_member.member_id and not self._is_workspace_admin(request, slug):
+ return Response(
+ {"error": "You are not allowed to view this member's preferences."},
+ status=status.HTTP_403_FORBIDDEN,
+ )This keeps the endpoint behavior but closes the authorization hole and handles bad payloads gracefully. Adjust the exact error messages/roles if you intend admins to be able to manage other members’ navigation settings differently.
📝 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.
| class ProjectMemberPreferenceEndpoint(BaseAPIView): | |
| def get_project_member(self, slug, project_id, member_id): | |
| return ProjectMember.objects.get( | |
| project_id=project_id, | |
| member_id=member_id, | |
| workspace__slug=slug, | |
| ) | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def patch(self, request, slug, project_id, member_id): | |
| project_member = self.get_project_member(slug, project_id, member_id) | |
| current_preferences = project_member.preferences or {} | |
| current_preferences["navigation"] = request.data["navigation"] | |
| project_member.preferences = current_preferences | |
| project_member.save(update_fields=["preferences"]) | |
| return Response({"preferences": project_member.preferences}, status=status.HTTP_200_OK) | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def get(self, request, slug, project_id, member_id): | |
| project_member = self.get_project_member(slug, project_id, member_id) | |
| response = { | |
| "preferences": project_member.preferences, | |
| "project_id": project_member.project_id, | |
| "member_id": project_member.member_id, | |
| "workspace_id": project_member.workspace_id, | |
| } | |
| return Response(response, status=status.HTTP_200_OK) | |
| class ProjectMemberPreferenceEndpoint(BaseAPIView): | |
| def get_project_member(self, slug, project_id, member_id): | |
| return ProjectMember.objects.get( | |
| project_id=project_id, | |
| member_id=member_id, | |
| workspace__slug=slug, | |
| is_active=True, | |
| ) | |
| def _is_workspace_admin(self, request, slug): | |
| return WorkspaceMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| is_active=True, | |
| role=ROLE.ADMIN.value, | |
| ).exists() | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def patch(self, request, slug, project_id, member_id): | |
| project_member = self.get_project_member(slug, project_id, member_id) | |
| # Only the member themselves or a workspace admin can update these prefs | |
| if request.user.id != project_member.member_id and not self._is_workspace_admin(request, slug): | |
| return Response( | |
| {"error": "You are not allowed to update this member's preferences."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| navigation = request.data.get("navigation") | |
| if navigation is None: | |
| return Response( | |
| {"error": "Missing 'navigation' payload."}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| current_preferences = project_member.preferences or {} | |
| current_preferences["navigation"] = navigation | |
| project_member.preferences = current_preferences | |
| project_member.save(update_fields=["preferences"]) | |
| return Response({"preferences": project_member.preferences}, status=status.HTTP_200_OK) | |
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST]) | |
| def get(self, request, slug, project_id, member_id): | |
| project_member = self.get_project_member(slug, project_id, member_id) | |
| if request.user.id != project_member.member_id and not self._is_workspace_admin(request, slug): | |
| return Response( | |
| {"error": "You are not allowed to view this member's preferences."}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| response = { | |
| "preferences": project_member.preferences, | |
| "project_id": project_member.project_id, | |
| "member_id": project_member.member_id, | |
| "workspace_id": project_member.workspace_id, | |
| } | |
| return Response(response, status=status.HTTP_200_OK) |
🤖 Prompt for AI Agents
In apps/api/plane/app/views/project/member.py around lines 305 to 336, the
endpoint currently allows any workspace user with ROLE.ADMIN/MEMBER/GUEST to
read or modify any member's preferences and assumes request.data["navigation"]
exists; restrict access so only the member themself or a workspace admin can
GET/PATCH another member's preferences: check request.user.id (or
request.user.member_id) against member_id and allow if equal or if the requester
has workspace admin role (return 403 otherwise), and validate the payload before
updating by ensuring "navigation" is present and has the expected type/shape
(return a 400 with a clear message if missing/invalid); finally, when updating,
merge into project_member.preferences safely (defaulting to {}) and save only
after validation.
| @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE") | ||
| def patch(self, request, slug, key): | ||
| preference = WorkspaceUserPreference.objects.filter(key=key, workspace__slug=slug, user=request.user).first() | ||
| def patch(self, request, slug): | ||
| for data in request.data: | ||
| key = data.pop("key", None) | ||
| if not key: | ||
| continue | ||
|
|
||
| if preference: | ||
| serializer = WorkspaceUserPreferenceSerializer(preference, data=request.data, partial=True) | ||
| preference = WorkspaceUserPreference.objects.filter(key=key, workspace__slug=slug).first() | ||
|
|
||
| if serializer.is_valid(): | ||
| serializer.save() | ||
| return Response(serializer.data, status=status.HTTP_200_OK) | ||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | ||
| if not preference: | ||
| continue | ||
|
|
||
| return Response({"detail": "Preference not found"}, status=status.HTTP_404_NOT_FOUND) | ||
| if "is_pinned" in data: | ||
| preference.is_pinned = data["is_pinned"] | ||
|
|
||
| if "sort_order" in data: | ||
| preference.sort_order = data["sort_order"] | ||
|
|
||
| preference.save(update_fields=["is_pinned", "sort_order"]) | ||
|
|
||
| return Response({"message": "Successfully updated"}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
PATCH updates the wrong user’s preferences (missing user filter)
Current logic:
- Looks up
WorkspaceUserPreferencebykeyandworkspace__slugonly, then.first(). - Ignores
user, even though the model is per‑user andget()above always filters byuser=request.user.
This means:
- You’re not guaranteed to update the calling user’s record.
- You may end up mutating another user’s preference for that key, which is both a correctness and authorization problem.
You can fix this and simplify the loop by scoping to request.user and avoiding mutation of request.data:
@@
- @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE")
- def patch(self, request, slug):
- for data in request.data:
- key = data.pop("key", None)
- if not key:
- continue
-
- preference = WorkspaceUserPreference.objects.filter(key=key, workspace__slug=slug).first()
-
- if not preference:
- continue
-
- if "is_pinned" in data:
- preference.is_pinned = data["is_pinned"]
-
- if "sort_order" in data:
- preference.sort_order = data["sort_order"]
-
- preference.save(update_fields=["is_pinned", "sort_order"])
-
- return Response({"message": "Successfully updated"}, status=status.HTTP_200_OK)
+ @allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE")
+ def patch(self, request, slug):
+ for item in request.data:
+ key = item.get("key")
+ if not key:
+ continue
+
+ preference = WorkspaceUserPreference.objects.filter(
+ key=key,
+ workspace__slug=slug,
+ user=request.user,
+ ).first()
+
+ if not preference:
+ continue
+
+ if "is_pinned" in item:
+ preference.is_pinned = item["is_pinned"]
+
+ if "sort_order" in item:
+ preference.sort_order = item["sort_order"]
+
+ preference.save(update_fields=["is_pinned", "sort_order"])
+
+ return Response({"message": "Successfully updated"}, status=status.HTTP_200_OK)This keeps the bulk‑update behavior but guarantees you only touch the current user’s preferences.
🤖 Prompt for AI Agents
In apps/api/plane/app/views/workspace/user_preference.py around lines 67–87, the
loop looks up WorkspaceUserPreference by key and workspace slug only, so it may
update another user's preference; change the query to scope to the current user
(filter by user=request.user, workspace__slug=slug, key=key) and use
data.get("key") instead of popping to avoid mutating request.data; only set and
save fields that are present in the incoming object and build a dynamic
update_fields list (e.g., add "is_pinned" and/or "sort_order" only if provided)
and skip if no matching preference is found.
| if (!isNaN(numValue)) { | ||
| const validValue = Math.max(1, numValue); | ||
| updateLimitedProjectsCount(validValue); | ||
| } |
There was a problem hiding this comment.
Bug: Empty input causes state desync in project count
When the user clears the project count input completely, parseInt(cleanedValue, 10) returns NaN, causing the condition if (!isNaN(numValue)) to be false. This prevents updateLimitedProjectsCount from being called, leaving the stored value unchanged while the UI displays an empty input. This creates a desync between the displayed value and the actual stored preference, potentially confusing users about the current setting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/ce/components/navigations/use-navigation-items.ts (2)
80-87: Confirm correct feature flag field for Intake visibility.
shouldRenderfor the Intake item uses!!project?.inbox_view. A previous static-analysis comment noted a possible mismatch with a backend field namedintake_view. Please double-check that your project type and API response both use the same field name so this flag actually reflects the project’s intake setting; otherwise Intake could remain hidden even when enabled.
98-105: Fix project-level permission check fallback to useprojectIdinstead of"".If
projectis undefined, the current call:const hasAccess = allowPermissions(item.access, EUserPermissionsLevel.PROJECT, workspaceSlug, project?.id ?? "");passes an empty string as the project id, which can break permission resolution and incorrectly hide items while data is loading or when
projectisn’t supplied, even though you already have a requiredprojectIdprop. Recommend falling back toprojectId:- const hasAccess = allowPermissions(item.access, EUserPermissionsLevel.PROJECT, workspaceSlug, project?.id ?? ""); + const hasAccess = allowPermissions( + item.access, + EUserPermissionsLevel.PROJECT, + workspaceSlug, + project?.id ?? projectId + );This keeps the filter stable whenever a valid
projectIdis known. Based on learnings, avoiding empty-string project ids here prevents subtle permission/visibility bugs.
🧹 Nitpick comments (2)
apps/web/core/components/navigation/tab-navigation-visible-item.tsx (1)
49-64: Localize context menu labels instead of hardcoded strings.The menu item labels (
"Clear default","Set as default","Hide in more menu") are currently hardcoded while the tab label itself usest(item.i18n_key). For consistency and localization support, consider moving these strings behind i18n keys as well.apps/web/core/components/navigation/use-project-actions.ts (1)
24-35: Move toast titles/messages into i18n.
"Link copied!","Project link copied to clipboard.","Copy failed", and"We couldn't copy the link. Please try again."are user-facing strings; consider wiring them through your i18n layer for consistency with the rest of the navigation UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/ce/components/navigations/use-navigation-items.ts(1 hunks)apps/web/core/components/analytics/overview/active-project-item.tsx(1 hunks)apps/web/core/components/navigation/tab-navigation-utils.ts(1 hunks)apps/web/core/components/navigation/tab-navigation-visible-item.tsx(1 hunks)apps/web/core/components/navigation/use-active-tab.ts(1 hunks)apps/web/core/components/navigation/use-project-actions.ts(1 hunks)apps/web/core/components/navigation/use-responsive-tab-layout.ts(1 hunks)apps/web/ee/components/navigations/index.ts(1 hunks)packages/i18n/src/locales/en/translations.ts(1 hunks)packages/propel/src/context-menu/context-menu.tsx(2 hunks)packages/propel/src/icons/actions/preferences-icon.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/ee/components/navigations/index.ts
- apps/web/core/components/navigation/tab-navigation-utils.ts
- packages/propel/src/context-menu/context-menu.tsx
- packages/propel/src/icons/actions/preferences-icon.tsx
- packages/i18n/src/locales/en/translations.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/navigation/use-active-tab.tsapps/web/ce/components/navigations/use-navigation-items.tsapps/web/core/components/analytics/overview/active-project-item.tsxapps/web/core/components/navigation/tab-navigation-visible-item.tsxapps/web/core/components/navigation/use-project-actions.tsapps/web/core/components/navigation/use-responsive-tab-layout.ts
🧠 Learnings (6)
📚 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/navigation/use-active-tab.ts
📚 Learning: 2025-06-18T09:46:08.566Z
Learnt from: prateekshourya29
Repo: makeplane/plane PR: 7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.
Applied to files:
apps/web/ce/components/navigations/use-navigation-items.ts
📚 Learning: 2025-07-08T13:41:01.659Z
Learnt from: prateekshourya29
Repo: makeplane/plane PR: 7363
File: apps/web/core/components/issues/select/dropdown.tsx:9-11
Timestamp: 2025-07-08T13:41:01.659Z
Learning: The `getProjectLabelIds` function in the label store handles undefined projectId internally, so it's safe to pass undefined values to it without explicit checks in the calling component.
Applied to files:
apps/web/ce/components/navigations/use-navigation-items.ts
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
Repo: makeplane/plane PR: 7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/web/ce/components/navigations/use-navigation-items.ts
📚 Learning: 2025-06-16T07:23:39.497Z
Learnt from: vamsikrishnamathala
Repo: makeplane/plane PR: 7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.
Applied to files:
apps/web/ce/components/navigations/use-navigation-items.ts
📚 Learning: 2025-09-02T08:14:49.260Z
Learnt from: sriramveeraghanta
Repo: makeplane/plane PR: 7697
File: apps/web/app/(all)/[workspaceSlug]/(projects)/header.tsx:12-13
Timestamp: 2025-09-02T08:14:49.260Z
Learning: The star-us-link.tsx file in apps/web/app/(all)/[workspaceSlug]/(projects)/ already has "use client" directive at the top, making it a proper Client Component for hook usage.
Applied to files:
apps/web/core/components/navigation/use-project-actions.ts
🧬 Code graph analysis (2)
apps/web/ce/components/navigations/use-navigation-items.ts (2)
packages/types/src/project/projects.ts (1)
IPartialProject(13-36)apps/web/core/components/navigation/tab-navigation-root.tsx (1)
TNavigationItem(31-40)
apps/web/core/components/navigation/use-project-actions.ts (4)
apps/web/core/components/navigation/tab-navigation-root.tsx (1)
TNavigationItem(31-40)apps/space/core/store/publish/publish.store.ts (1)
workspaceSlug(93-95)packages/utils/src/string.ts (1)
copyUrlToClipboard(66-72)packages/propel/src/toast/toast.tsx (1)
setToast(202-222)
⏰ 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). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
apps/web/core/components/analytics/overview/active-project-item.tsx (1)
59-59: LGTM — stylistic cleanup.The semicolon removal after the function declaration aligns with modern JavaScript/TypeScript conventions and has no impact on behavior.
apps/web/core/components/navigation/tab-navigation-visible-item.tsx (2)
2-2: Verify routerLinkimport and usage (react-routervs app router).This component imports
Linkfrom"react-router"and uses thetoprop. Given the rest of the app appears to be Next-based, please confirm this is rendered within a React Router context and that"react-router"is the intended dependency here; otherwise, this should likely use the app’s standard link component (e.g., Next’snext/linkor a shared<Link>wrapper) to avoid runtime router-context errors.Also applies to: 41-45
22-47: Per-item visible tab composition and default handling look solid.The way
isDefaultis derived and how theTabNavigationItemis wrapped in theContextMenu.Triggerwith a stableitemReffor measurement is clean and easy to integrate with the surrounding tab system.apps/web/core/components/navigation/use-project-actions.ts (1)
12-54: Hook structure and dependencies are consistent.State for publish/leave modals and the callbacks (including
handleCopyTextwith correct dependencies) are well-scoped and should integrate cleanly with the navigation header actions.apps/web/ce/components/navigations/use-navigation-items.ts (1)
26-90: Base navigation shape and permission-aware filtering look coherent.The
baseNavigationdefinition, feature-flag-basedshouldRenderchecks, and final sort bysortOrdergive you a predictable, permission-scoped navigation list that fits well with the new tab navigation system.Also applies to: 94-106
apps/web/core/components/navigation/use-responsive-tab-layout.ts (1)
45-95: Responsive tab layout and active-tab pinning logic look solid.The ResizeObserver-based width tracking,
visibleCountcalculation (including reserved overflow-button space and one-item minimum), and the swap logic that keeps the active tab in the visible section all read correctly and should produce intuitive behavior as tabs overflow. Just ensure the caller maintainsitemRefs.currentin sync withvisibleNavigationItemsindices so measurements stay accurate.Also applies to: 103-133
apps/web/core/components/navigation/use-active-tab.ts (1)
13-35: Active-tab detection is concise and covers work items, epics, and path-based routes.The
isActivecallback cleanly combines work-item/epic checks scoped byprojectIdwith a pathname equality/prefix check, and memoizingactiveItemover this keeps navigation rendering efficient.
Description
This PR includes major refactoring of the navigation introducing flexible navigation modes, improved sidebar functionality, and better user preference management.
Type of Change
References
Enterprise PR
Note
Overhauls navigation with customizable workspace/project navigation (app rail, horizontal tabs), adds APIs for member and sidebar preferences, refactors headers/breadcrumbs, and introduces new UI components/icons.
ProjectMemberPreferenceEndpoint(GET/PATCH) atworkspaces/<slug>/projects/<project_id>/preferences/member/<member_id>/to persist per-member project navigation (default tab, hidden items).WorkspaceUserPreferenceViewSetto support bulkPATCH /workspaces/<slug>/sidebar-preferences/and include newstickieskey.preferences.navigationincludesdefault_tabandhide_in_more_menu.TabNavigationRoot.[projectId]/layout.tsx; ensure tab header renders for horizontal mode.framer-motiondependency.Written by Cursor Bugbot for commit d775be7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.