feat(navigation): Add flagged new sidebar components#3815
Conversation
Had to centralize the fetching of nav components to avoid to duplicate the flag and component choosing logic
doubleface
requested review from
JF-Cozy,
lethemanh,
rezk2ll and
zatteo
as code owners
WalkthroughThis pull request upgrades the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
BundleMonFiles updated (2)
Unchanged files (19)
Total files change +520B +0.01% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/navigation/navComponents.ts (2)
17-24: Nit: declaring the shared type fromNavNewis fine, but worth a note in a comment.Future readers may wonder why
NavComponentsis keyed bytypeof NavNew. A one-line comment ("New variant is the canonical type; old is asserted to match for migration") would make the choice obvious and the casts on lines 33-40 less surprising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/navComponents.ts` around lines 17 - 24, Add a one-line comment above the NavComponents type explaining that NavNew is treated as the canonical type for the new variant and the older components are asserted to match for migration; reference NavNew as the canonical shape and mention that NavDesktopDropdownNew, NavIconNew, NavItemNew, NavLinkNew, and NavTextNew are expected to conform so the subsequent casts (where older exports are asserted to that type) are intentional.
29-42: Optional: memoize the resolved component map.
getNavComponents()is invoked on every render across at least 7 navigation components, allocating a fresh object each time. Since the underlying components are module-stable, you can resolve once per flag value:♻️ Cache by flag value
-export const getNavComponents = (): NavComponents => { - const useNewSidebar = isNewSidebarEnabled() - - return { - Nav: useNewSidebar ? NavNew : (NavOld as typeof NavNew), - NavDesktopDropdown: useNewSidebar - ? NavDesktopDropdownNew - : (NavDesktopDropdownOld as typeof NavDesktopDropdownNew), - NavIcon: useNewSidebar ? NavIconNew : (NavIconOld as typeof NavIconNew), - NavItem: useNewSidebar ? NavItemNew : (NavItemOld as typeof NavItemNew), - NavLink: useNewSidebar ? NavLinkNew : (NavLinkOld as typeof NavLinkNew), - NavText: useNewSidebar ? NavTextNew : (NavTextOld as typeof NavTextNew) - } -} +const NEW_COMPONENTS: NavComponents = { + Nav: NavNew, + NavDesktopDropdown: NavDesktopDropdownNew, + NavIcon: NavIconNew, + NavItem: NavItemNew, + NavLink: NavLinkNew, + NavText: NavTextNew +} + +const OLD_COMPONENTS: NavComponents = { + Nav: NavOld as typeof NavNew, + NavDesktopDropdown: NavDesktopDropdownOld as typeof NavDesktopDropdownNew, + NavIcon: NavIconOld as typeof NavIconNew, + NavItem: NavItemOld as typeof NavItemNew, + NavLink: NavLinkOld as typeof NavLinkNew, + NavText: NavTextOld as typeof NavTextNew +} + +export const getNavComponents = (): NavComponents => + isNewSidebarEnabled() ? NEW_COMPONENTS : OLD_COMPONENTS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/navComponents.ts` around lines 29 - 42, getNavComponents currently allocates a new NavComponents object on every render; cache the resolved map per flag value so repeated calls return the same object. Implement a small memoization: add a module-level cache keyed by the boolean result of isNewSidebarEnabled() (or two module-scoped variables like cachedNewSidebarTrue/cachedNewSidebarFalse) and inside getNavComponents call isNewSidebarEnabled(), return the cached object if present, otherwise build the map using Nav, NavNew, NavOld, NavDesktopDropdownNew, NavDesktopDropdownOld, NavIconNew, NavIconOld, NavItemNew, NavItemOld, NavLinkNew, NavLinkOld, NavTextNew, NavTextOld, store it in the cache and return it; preserve the NavComponents type when storing/returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/navigation/navComponents.ts`:
- Around line 26-42: Summary: NavNext (NavNew) may lack static NavLink.className
and NavLink.activeClassName so toggling the new sidebar can yield undefined
class names; fix by ensuring the new NavLink exposes those static properties or
by copying/falling back to the old values. In
getNavComponents/isNewSidebarEnabled, verify NavNew.NavLink has className and
activeClassName at runtime and if missing assign them from NavOld.NavLink (e.g.,
if (useNewSidebar && !NavNew.NavLink.className) { NavNew.NavLink.className =
NavOld.NavLink.className; NavNew.NavLink.activeClassName =
NavOld.NavLink.activeClassName }), and add a proper TS declaration for
'cozy-ui/transpiled/react/NavNext' to include these static props so callers
(NavLink.jsx, ExternalNavItem.jsx, FavoriteListItem.tsx,
ExternalDriveListItem.tsx) are statically safe.
---
Nitpick comments:
In `@src/modules/navigation/navComponents.ts`:
- Around line 17-24: Add a one-line comment above the NavComponents type
explaining that NavNew is treated as the canonical type for the new variant and
the older components are asserted to match for migration; reference NavNew as
the canonical shape and mention that NavDesktopDropdownNew, NavIconNew,
NavItemNew, NavLinkNew, and NavTextNew are expected to conform so the subsequent
casts (where older exports are asserted to that type) are intentional.
- Around line 29-42: getNavComponents currently allocates a new NavComponents
object on every render; cache the resolved map per flag value so repeated calls
return the same object. Implement a small memoization: add a module-level cache
keyed by the boolean result of isNewSidebarEnabled() (or two module-scoped
variables like cachedNewSidebarTrue/cachedNewSidebarFalse) and inside
getNavComponents call isNewSidebarEnabled(), return the cached object if
present, otherwise build the map using Nav, NavNew, NavOld,
NavDesktopDropdownNew, NavDesktopDropdownOld, NavIconNew, NavIconOld,
NavItemNew, NavItemOld, NavLinkNew, NavLinkOld, NavTextNew, NavTextOld, store it
in the cache and return it; preserve the NavComponents type when
storing/returning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27f95179-a859-4875-8618-3fd1adf536bc
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.jsonsrc/modules/navigation/ExternalNavItem.jsxsrc/modules/navigation/FavoriteList.tsxsrc/modules/navigation/FavoriteListItem.tsxsrc/modules/navigation/Nav.jsxsrc/modules/navigation/NavContent.tsxsrc/modules/navigation/NavItem.jsxsrc/modules/navigation/NavLink.jsxsrc/modules/navigation/components/ExternalDriveListItem.tsxsrc/modules/navigation/navComponents.ts
| const isNewSidebarEnabled = (): boolean => | ||
| Boolean(flag('drive.new-sidebar.enabled')) | ||
|
|
||
| export const getNavComponents = (): NavComponents => { | ||
| const useNewSidebar = isNewSidebarEnabled() | ||
|
|
||
| return { | ||
| Nav: useNewSidebar ? NavNew : (NavOld as typeof NavNew), | ||
| NavDesktopDropdown: useNewSidebar | ||
| ? NavDesktopDropdownNew | ||
| : (NavDesktopDropdownOld as typeof NavDesktopDropdownNew), | ||
| NavIcon: useNewSidebar ? NavIconNew : (NavIconOld as typeof NavIconNew), | ||
| NavItem: useNewSidebar ? NavItemNew : (NavItemOld as typeof NavItemNew), | ||
| NavLink: useNewSidebar ? NavLinkNew : (NavLinkOld as typeof NavLinkNew), | ||
| NavText: useNewSidebar ? NavTextNew : (NavTextOld as typeof NavTextNew) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect cozy-ui Nav and NavNext to confirm both expose static className / activeClassName on NavLink
fd -i 'NavNext' node_modules/cozy-ui 2>/dev/null | head -20
fd -i '^Nav\.(js|ts|jsx|tsx|d\.ts)$' node_modules/cozy-ui 2>/dev/null | head -20
# Search for static className/activeClassName assignments in the package
rg -nP --type=ts --type=tsx --type=js --type=jsx -C2 '\bNavLink\.(className|activeClassName)\s*=' node_modules/cozy-ui 2>/dev/null
rg -nP -C2 '(className|activeClassName)\s*=\s*' node_modules/cozy-ui/transpiled/react/NavNext 2>/dev/null
rg -nP -C2 '(className|activeClassName)\s*=\s*' node_modules/cozy-ui/transpiled/react/Nav 2>/dev/nullRepository: linagora/twake-drive
Length of output: 46
🌐 Web query:
cozy-ui NavNext NavLink className activeClassName static property
💡 Result:
No evidence found of NavNext or NavLink components in cozy-ui. Cozy-ui is a React UI library based on Material-UI with documented components like Button, SearchBar, ListItem, but no navigation components named NavNext or NavLink. There is a NavLimiter component mentioned in the CHANGELOG. The terms className, activeClassName, and static property likely refer to standard React props (className for CSS classes, activeClassName from react-router-dom NavLink for active route styling), but these are not specific static properties or components in cozy-ui. Official docs and GitHub repo confirm no such components exist.
Citations:
- 1: https://github.com/cozy/cozy-ui
- 2: https://docs.cozy.io/en/cozy-ui/
- 3: https://docs.cozy.io/en/cozy-ui
- 4: https://docs.cozy.io/cozy-ui/react/
- 5: https://github.com/cozy/cozy-ui/blob/5ac6a96f819b177eb43ae399f9b7c8f586d1fa54/CHANGELOG.md
🏁 Script executed:
# First, let's examine the actual navComponents.ts file
cat -n src/modules/navigation/navComponents.tsRepository: linagora/twake-drive
Length of output: 1718
🏁 Script executed:
# Search for NavNext and NavOld definitions in the codebase
rg -l "NavNext|NavOld" --type=ts --type=tsx --type=js --type=jsx | head -20Repository: linagora/twake-drive
Length of output: 92
🏁 Script executed:
# Check the actual imports in navComponents.ts to understand the source
rg "import.*Nav" src/modules/navigation/navComponents.ts -A 2 -B 2Repository: linagora/twake-drive
Length of output: 337
🏁 Script executed:
# Search for the consumer files mentioned in the review
rg -l "NavLink\.className|NavLink\.activeClassName" --type=js --type=ts | head -20Repository: linagora/twake-drive
Length of output: 246
🏁 Script executed:
# Check if NavLink.jsx exists and what it does
find . -name "NavLink.jsx" -o -name "NavLink.tsx" -o -name "NavLink.ts" | head -10Repository: linagora/twake-drive
Length of output: 101
🏁 Script executed:
# Search for any reference to NavLink in the codebase to understand usage
rg "from.*NavLink|import.*NavLink" --type=js --type=ts | head -30Repository: linagora/twake-drive
Length of output: 154
🏁 Script executed:
# Look for the specific consumer files mentioned
find . -name "ExternalNavItem.jsx" -o -name "FavoriteListItem.tsx" -o -name "ExternalDriveListItem.tsx" | head -10Repository: linagora/twake-drive
Length of output: 217
🏁 Script executed:
# Read NavLink.jsx to understand its structure
cat -n src/modules/navigation/NavLink.jsxRepository: linagora/twake-drive
Length of output: 1594
🏁 Script executed:
# Check ExternalNavItem.jsx around line 42
cat -n src/modules/navigation/ExternalNavItem.jsx | sed -n '35,50p'Repository: linagora/twake-drive
Length of output: 511
🏁 Script executed:
# Check FavoriteListItem.tsx around line 50
cat -n src/modules/navigation/FavoriteListItem.tsx | sed -n '40,60p'Repository: linagora/twake-drive
Length of output: 739
🏁 Script executed:
# Check ExternalDriveListItem.tsx
cat -n src/modules/navigation/components/ExternalDriveListItem.tsx | head -60Repository: linagora/twake-drive
Length of output: 1615
🏁 Script executed:
# Search for NavLink type definitions or interfaces in the codebase
rg "NavLink\s*[:=].*className|activeClassName" --type=ts --type=js -B 3 -A 3Repository: linagora/twake-drive
Length of output: 844
🏁 Script executed:
# Check if there's a package.json to see the cozy-ui version
cat package.json | grep -A 5 "cozy-ui"Repository: linagora/twake-drive
Length of output: 254
🏁 Script executed:
# Look for any TypeScript configuration or declaration files
find . -name "*.d.ts" -path "*/node_modules/cozy-ui*" | head -10Repository: linagora/twake-drive
Length of output: 46
🏁 Script executed:
# Check if the cozy-ui types are available in the codebase
rg "declare.*NavLink|export.*NavLink" --type=ts | head -20Repository: linagora/twake-drive
Length of output: 157
🏁 Script executed:
# Read the full declarations.d.ts to see all module declarations
cat -n src/declarations.d.tsRepository: linagora/twake-drive
Length of output: 7611
🏁 Script executed:
# Search for any NavNext type declarations
rg "NavNext|navnext" src/declarations.d.ts -iRepository: linagora/twake-drive
Length of output: 46
🏁 Script executed:
# Check if there are any type definition files for NavNext
find . -name "*.d.ts" -type f | xargs grep -l "NavNext" 2>/dev/nullRepository: linagora/twake-drive
Length of output: 48
🏁 Script executed:
# Look for `@types` packages or type definitions in package.json
cat package.json | grep -E "@types|types"Repository: linagora/twake-drive
Length of output: 307
Verify that NavNew (from cozy-ui/transpiled/react/NavNext) exposes the same static className and activeClassName properties on NavLink as NavOld.
Multiple consumers (NavLink.jsx lines 33–34, ExternalNavItem.jsx line 42, FavoriteListItem.tsx line 50, ExternalDriveListItem.tsx line 30) access NavLink.className and NavLink.activeClassName as static properties. The type assertions on lines 33–40 (NavOld as typeof NavNew, etc.) suppress structural type checking. declarations.d.ts declares the Nav variant's NavLink with these properties, but NavNext has no corresponding type declarations. If NavNext.NavLink lacks these properties, toggling the feature flag will silently produce className={undefined}, breaking link styling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/navigation/navComponents.ts` around lines 26 - 42, Summary:
NavNext (NavNew) may lack static NavLink.className and NavLink.activeClassName
so toggling the new sidebar can yield undefined class names; fix by ensuring the
new NavLink exposes those static properties or by copying/falling back to the
old values. In getNavComponents/isNewSidebarEnabled, verify NavNew.NavLink has
className and activeClassName at runtime and if missing assign them from
NavOld.NavLink (e.g., if (useNewSidebar && !NavNew.NavLink.className) {
NavNew.NavLink.className = NavOld.NavLink.className;
NavNew.NavLink.activeClassName = NavOld.NavLink.activeClassName }), and add a
proper TS declaration for 'cozy-ui/transpiled/react/NavNext' to include these
static props so callers (NavLink.jsx, ExternalNavItem.jsx, FavoriteListItem.tsx,
ExternalDriveListItem.tsx) are statically safe.
|
@JF-Cozy this is the approach you don't like when changing an existing component? Because I see only two possible options for this kind of update:
|
2 participants
Had to centralise the fetching of Nav/NavNext components to avoid to duplicate the flag and component choosing logic
Summary by CodeRabbit
Refactor
Chores