-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[OPIK-7126] [FE] feat: unify workspace menu with projects menu #7484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
edfb97f
de2d552
1f00c83
8e84029
c05ff5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Match the Figma instruction note and the projects menu section header
("Recently updated"). Only the user-facing label changes; the underlying
recency mechanism (client-side visit tracking) and its hook/variable names
are unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,7 +87,7 @@ const WorkspaceMenuContent: React.FC<WorkspaceMenuContentProps> = ({ | |
| const isSearching = search.trim().length > 0; | ||
|
|
||
| // When searching, show all matches flat (virtualized above the threshold). | ||
| // Otherwise cluster into Pinned + Recently visited (capped) sections. | ||
| // Otherwise cluster into Pinned + Recently updated (capped) sections. | ||
| const workspacesById = new Map( | ||
| sortedWorkspaces.map((workspace) => [workspace.workspaceId, workspace]), | ||
| ); | ||
|
|
@@ -310,7 +310,7 @@ const WorkspaceMenuContent: React.FC<WorkspaceMenuContentProps> = ({ | |
| {pinnedWorkspaces.length > 0 && ( | ||
| <DropdownMenuSeparator className="my-1" /> | ||
| )} | ||
|
Comment on lines
+341
to
+352
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate section layoutThe pinned/recent section markup in the new workspace menu duplicates Want Baz to fix this for you? Activate Fixer |
||
| <MenuSectionLabel>Recently visited</MenuSectionLabel> | ||
| <MenuSectionLabel>Recently updated</MenuSectionLabel> | ||
| {recentlyVisitedWorkspaces.map(renderWorkspaceItem)} | ||
| </> | ||
| )} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 suggestion | Performance
Derived workspace collections are rebuilt on every render, including during search when they're discarded unused.
workspacesById(a new Map scanning all workspaces),pinnedWorkspaces(map+filter), andrecentlyVisitedWorkspaces(filter+slice) are computed unconditionally in the component body. The menu re-renders on every search keystroke, so these O(n) derivations re-run each time — and they're only consumed in the non-search branch, so during search they're computed and thrown away.The sibling
ProjectMenuContent.tsxmemoizes the analogousprojectsByIdwithuseMemo. Consider wrapping these inuseMemo.🤖 Review posted via /review-github-pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c05ff5d — wrapped
workspacesById,pinnedWorkspaces, andrecentlyVisitedWorkspacesinuseMemo, matchingProjectMenuContent's memoizedprojectsById. Deps are referentially stable:sortedWorkspacesis already memoized inuseWorkspaceSelectorData, andisPinned/pinnedConfigcome fromuseCallback/useLocalStorageState. Lint, typecheck, and the 11 hook tests (Node 20) are green.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit c05ff5d addressed this comment by memoizing
workspacesById,pinnedWorkspaces, andrecentlyVisitedWorkspaceswithuseMemo. This prevents those O(n) derivations from being rebuilt on every render, including search re-renders where they would otherwise be discarded.