[feat]: improved side bar#4843
Conversation
…ubmenu integration
…mance and clarity
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds shared sidebar contracts, dynamic entity sources, sidebar shell/menu rendering, scoped main and settings wiring, settings navigation helpers, and a references icon helper export. ChangesSidebar overhaul
References icon helper
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
web/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx (1)
50-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the composed sidebar tree before returning it.
Line 257 returns a fresh
injectDynamicChildren(sidebarConfig, dynamicChildren)result on every render, so downstream memoization inweb/oss/src/components/Sidebar/scopes/mainScope.tsx:35-73is invalidated even when the menu is logically unchanged. Wrap the base config inuseMemo, and hoist the click handlers intouseCallback, so the injected tree can also be memoized.♻️ Suggested shape
+ const handleOpenWidget = useCallback( + (e: React.MouseEvent<HTMLElement>) => { + e.preventDefault() + openWidget() + }, + [openWidget], + ) + + const handleToggleSupport = useCallback( + (e: React.MouseEvent<HTMLElement>) => { + e.preventDefault() + toggle() + }, + [toggle], + ) + - const sidebarConfig: SidebarConfig[] = [ + const sidebarConfig = useMemo<SidebarConfig[]>(() => [ // ... { key: "get-started-guide-link", // ... - onClick: (e) => { - e.preventDefault() - openWidget() - }, + onClick: handleOpenWidget, }, { key: "support-chat-link", // ... - onClick: (e) => { - e.preventDefault() - toggle() - }, + onClick: handleToggleSupport, }, - ] + ], [ + appURL, + baseAppURL, + canInviteMembers, + currentApp, + doesSessionExist, + dynamicChildren, + handleOpenWidget, + handleToggleSupport, + hasAppContext, + hasProjectURL, + isCrispEnabled, + isVisible, + projectURL, + recentlyVisitedAppId, + recentlyVisitedAppURL, + selectedOrg, + ]) - return injectDynamicChildren(sidebarConfig, dynamicChildren) + return useMemo( + () => injectDynamicChildren(sidebarConfig, dynamicChildren), + [sidebarConfig, dynamicChildren], + )As per coding guidelines, "Use
useMemofor expensive computations" and "Do not pass inline arrays of objects with JSX content as props. Memoize such arrays withuseMemoto prevent re-renders."Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9f399e0-ac3c-4b89-9ff2-8fe568af47b5
📒 Files selected for processing (18)
web/oss/src/components/References/ReferenceTag.tsxweb/oss/src/components/References/index.tsweb/oss/src/components/Sidebar/SettingsSidebar.tsxweb/oss/src/components/Sidebar/Sidebar.tsxweb/oss/src/components/Sidebar/components/SidebarMenu.tsxweb/oss/src/components/Sidebar/components/WorkflowEntityCard.tsxweb/oss/src/components/Sidebar/dynamic/registry.tsweb/oss/src/components/Sidebar/dynamic/source.tsweb/oss/src/components/Sidebar/dynamic/types.tsweb/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.tsweb/oss/src/components/Sidebar/engine/SidebarMenu.tsxweb/oss/src/components/Sidebar/engine/SidebarShell.tsxweb/oss/src/components/Sidebar/engine/types.tsweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsxweb/oss/src/components/Sidebar/scopes/mainScope.tsxweb/oss/src/components/Sidebar/scopes/settingsScope.tsxweb/oss/src/components/Sidebar/types.d.tsweb/oss/src/lib/atoms/sidebar.ts
💤 Files with no reviewable changes (3)
- web/oss/src/components/Sidebar/types.d.ts
- web/oss/src/components/Sidebar/SettingsSidebar.tsx
- web/oss/src/components/Sidebar/components/SidebarMenu.tsx
Railway Preview Environment
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx (1)
50-50: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize injected sidebar config to avoid unnecessary rerenders.
injectDynamicChildren(sidebarConfig, dynamicChildren)currently recomputes a full recursive config tree every render. Please wrap both the basesidebarConfigconstruction and injected result inuseMemowith explicit dependencies so child menu trees stay referentially stable.Suggested refactor
+import {useMemo} from "react" ... - const sidebarConfig: SidebarConfig[] = [ + const sidebarConfig: SidebarConfig[] = useMemo(() => [ ... - ] + ], [ + baseAppURL, + projectURL, + appURL, + recentlyVisitedAppURL, + hasProjectURL, + hasAppContext, + currentApp, + recentlyVisitedAppId, + doesSessionExist, + selectedOrg, + canInviteMembers, + isVisible, + isCrispEnabled, + toggle, + openWidget, + ]) - return injectDynamicChildren(sidebarConfig, dynamicChildren) + return useMemo( + () => injectDynamicChildren(sidebarConfig, dynamicChildren), + [sidebarConfig, dynamicChildren], + )As per coding guidelines,
web/**/*.tsxshould useuseMemofor expensive computations and avoid render-time object churn where possible.Also applies to: 257-257
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69dfc58e-d31b-4cfb-9032-db99efb47999
📒 Files selected for processing (3)
web/oss/src/components/Sidebar/dynamic/registry.tsweb/oss/src/components/Sidebar/engine/SidebarMenu.tsxweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx
💤 Files with no reviewable changes (1)
- web/oss/src/components/Sidebar/dynamic/registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/oss/src/components/Sidebar/engine/SidebarMenu.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/oss/src/components/Sidebar/scopes/settingsScope.tsx (1)
171-178: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize the controlled selection callback.
onSelectis recreated on every render; memoizing it keeps the sidebar selection contract stable for downstream menu components.♻️ Proposed refactor
+ const handleSelect = useCallback( + (key: string) => { + setSettingsTab(key) + setTab(key) + }, + [setSettingsTab, setTab], + ) + - return { + return useMemo<SidebarSelection>(() => ({ mode: "controlled", selectedKey: activeTab, - onSelect: (key) => { - setSettingsTab(key) - setTab(key) - }, - } + onSelect: handleSelect, + }), [activeTab, handleSelect])Add
useCallbackto the React import if it is not already present.As per coding guidelines, “Use
useCallbackto create stable callbacks in render instead of defining inline functions.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d62064a-4ad4-4048-907c-75b79a690a61
📒 Files selected for processing (5)
web/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.tsweb/oss/src/components/Sidebar/engine/SidebarMenu.tsxweb/oss/src/components/Sidebar/engine/SidebarShell.tsxweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsxweb/oss/src/components/Sidebar/scopes/settingsScope.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/oss/src/components/Sidebar/dynamic/useSidebarDynamicChildren.ts
- web/oss/src/components/Sidebar/engine/SidebarShell.tsx
- web/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx
- web/oss/src/components/Sidebar/engine/SidebarMenu.tsx
…ve navigation handling - Changed SidebarIslandProps to accept a SidebarView instead of showSettingsView and lastPath. - Refactored Sidebar component to resolve sidebar scope based on SidebarView. - Removed unused imports and cleaned up ListOfProjects component. - Updated dynamic sidebar registry to use scope IDs for better organization. - Enhanced gatedSidebarSource to accept scopeId as a parameter. - Simplified SidebarConfig interface by removing unnecessary properties. - Improved useSidebarConfig hook to return structured sidebar items. - Added constants for main and settings sidebar scope IDs. - Refactored settingsScope to utilize new settings tab management logic. - Introduced new navigation functions for settings tab resolution and visibility. - Added tests for settings tab resolution logic. - Updated projectSwitchHref to handle navigation context more effectively. - Cleaned up settings page to utilize new settings access logic. - Updated org hooks to use the new projectSwitchHref for navigation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/oss/src/components/Sidebar/components/assets/workflowEntitySelection.test.ts (1)
23-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the stale-route-id regression case.
This suite never exercises
currentWorkflowIdbeing set but absent from both lists, which is exactly where the new helper can skip the recent-app / recent-evaluator fallback. Pinning that behavior down here would keep this precedence logic from regressing again.Suggested test
+ it("falls back to recents when the current workflow id is stale", () => { + expect( + resolveWorkflowEntitySelection({ + currentWorkflow: null, + currentWorkflowId: "missing-workflow", + apps: [app], + evaluators: [evaluator], + recentAppId: app.id, + recentEvaluatorId: evaluator.id, + }), + ).toBe(app) + })web/oss/src/pages/w/[workspace_id]/p/[project_id]/settings/index.tsx (1)
76-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
SettingsAccessconstruction into one shared helper.Lines 76-86 duplicate the same
SettingsAccessassembly already present inweb/oss/src/components/Sidebar/scopes/settingsScope.tsxLines 47-65. Both the page and the sidebar now depend on the same navigation helpers, so a future access-rule change can desync the active settings content from the sidebar state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e353ae73-6acd-4e96-ada7-21b736ce8878
📒 Files selected for processing (21)
web/oss/src/components/Layout/Layout.tsxweb/oss/src/components/Layout/SidebarIsland.tsxweb/oss/src/components/Sidebar/Sidebar.tsxweb/oss/src/components/Sidebar/components/ListOfProjects.tsxweb/oss/src/components/Sidebar/components/WorkflowEntityCard.tsxweb/oss/src/components/Sidebar/components/assets/workflowEntitySelection.test.tsweb/oss/src/components/Sidebar/components/assets/workflowEntitySelection.tsweb/oss/src/components/Sidebar/dynamic/registry.tsweb/oss/src/components/Sidebar/dynamic/source.tsweb/oss/src/components/Sidebar/engine/types.tsweb/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsxweb/oss/src/components/Sidebar/scopes/constants.tsweb/oss/src/components/Sidebar/scopes/mainScope.tsxweb/oss/src/components/Sidebar/scopes/settingsScope.tsxweb/oss/src/components/Sidebar/types.tsweb/oss/src/components/pages/settings/assets/navigation.test.tsweb/oss/src/components/pages/settings/assets/navigation.tsweb/oss/src/lib/navigation/projectSwitchHref.test.tsweb/oss/src/lib/navigation/projectSwitchHref.tsweb/oss/src/pages/w/[workspace_id]/p/[project_id]/settings/index.tsxweb/oss/src/state/org/hooks.ts
💤 Files with no reviewable changes (1)
- web/oss/src/components/Sidebar/engine/types.ts
✅ Files skipped from review due to trivial changes (4)
- web/oss/src/lib/navigation/projectSwitchHref.ts
- web/oss/src/components/Sidebar/scopes/constants.ts
- web/oss/src/lib/navigation/projectSwitchHref.test.ts
- web/oss/src/components/Sidebar/components/ListOfProjects.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/oss/src/components/Sidebar/dynamic/source.ts
- web/oss/src/components/Sidebar/Sidebar.tsx
- web/oss/src/components/Sidebar/hooks/useSidebarConfig/index.tsx
- web/oss/src/components/Sidebar/scopes/settingsScope.tsx
1 participant
No description provided.