Conversation
- Preserve git top-level root paths in repository identity - Distinguish nested workspaces within the same repo when deriving project grouping keys - Add coverage for nested-root resolution and cross-environment grouping
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
bot
added
size:M
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces a significant new feature for configurable sidebar project grouping, including new settings, new UI dialogs, nested context menu support, and substantial new logic in sidebarProjectGrouping.ts. New user-facing features of this scope warrant human review regardless of implementation quality. Additionally, the author appears to be a less frequent contributor to these core files, and there are unresolved medium-severity review comments about memoization and positioning bugs. You can customize Macroscope's approvability policy. Learn more. |
- Group projects by repository path with per-project overrides - Add nested context menu actions for grouped project members - Update rename/delete flows for multi-project sidebar rows
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41e5a90. Configure here.
| return ( | ||
| project.repositoryIdentity?.canonicalKey ?? | ||
| deriveRepositoryScopedKey(project, groupingMode) ?? | ||
| derivePhysicalProjectKey(project) ?? |
There was a problem hiding this comment.
Unreachable fallback due to always-truthy return value
Low Severity
derivePhysicalProjectKey always returns a non-empty string (${environmentId}:${normalizedCwd}), so the ?? fallback to scopedProjectKey(scopeProjectRef(...)) on the next line is unreachable dead code. This makes the fallback chain misleading — a reader might think the scopedProjectKey path is reachable, but it never executes.
Reviewed by Cursor Bugbot for commit 41e5a90. Configure here.
| const projectGroupingSettings = useSettings((settings) => ({ | ||
| sidebarProjectGroupingMode: settings.sidebarProjectGroupingMode, | ||
| sidebarProjectGroupingOverrides: settings.sidebarProjectGroupingOverrides, | ||
| })); |
There was a problem hiding this comment.
New object reference every render defeats downstream memoization
Medium Severity
useSettings with an inline selector that returns a new object produces a fresh reference on every render. Internally, useSettings uses useMemo with [merged, selector] as deps — since the inline arrow function is a new reference each render, the memo always recomputes, returning a new object. This projectGroupingSettings object is then used as a dependency in multiple useMemo/useCallback hooks (e.g. physicalToLogicalKey, sidebarProjects, threadsByProjectKey), causing them all to needlessly recompute on every render.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 41e5a90. Configure here.
| ): string { | ||
| return project?.repositoryIdentity?.canonicalKey ?? scopedProjectKey(projectRef); | ||
| return project ? deriveLogicalProjectKey(project, options) : scopedProjectKey(projectRef); | ||
| } |
There was a problem hiding this comment.
Exported function deriveLogicalProjectKeyFromRef is now unused
Low Severity
deriveLogicalProjectKeyFromRef is exported but no longer imported or called anywhere in the codebase after this PR refactored all callers to use deriveLogicalProjectKeyFromSettings instead. This is dead exported code.
Reviewed by Cursor Bugbot for commit 41e5a90. Configure here.
- lets users change project grouping from the project sort menu - removes the duplicate setting from General settings Co-authored-by: codex <codex@users.noreply.github.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| for (const sourceItem of source) { | ||
| if (typeof sourceItem.id !== "string" || typeof sourceItem.label !== "string") { |
There was a problem hiding this comment.
🟢 Low src/main.ts:165
When source contains null or undefined elements (possible via untrusted IPC input), accessing sourceItem.id on line 166 throws a TypeError instead of being filtered out. Consider adding a guard to skip non-object elements before property access.
for (const sourceItem of source) {
+ if (!sourceItem || typeof sourceItem !== 'object') {
+ continue;
+ }
if (typeof sourceItem.id !== "string" || typeof sourceItem.label !== "string") {🤖 Copy this AI Prompt to have your agent fix this:
In file apps/desktop/src/main.ts around lines 165-166:
When `source` contains `null` or `undefined` elements (possible via untrusted IPC input), accessing `sourceItem.id` on line 166 throws a `TypeError` instead of being filtered out. Consider adding a guard to skip non-object elements before property access.
Evidence trail:
apps/desktop/src/main.ts lines 162-167 (normalizeContextMenuItems function with typeof sourceItem.id check), apps/desktop/src/main.ts lines 1734-1737 (ipcMain.handle calling normalizeContextMenuItems with untrusted IPC items)
|
|
||
| if (!isDisabled) { | ||
| if (hasChildren) { | ||
| button.addEventListener("mouseenter", () => { |
There was a problem hiding this comment.
🟡 Medium src/contextMenuFallback.ts:96
When a submenu overflows the right viewport edge, the mouseenter handler (lines 96-110) calls clampMenuPosition to flip it to the left side, but openMenu's requestAnimationFrame callback (lines 132-134) immediately re-clamps it using the original preferredLeft (which caused the overflow). The menu ends up pinned to the right edge instead of the intended left-side position, making the flip logic non-functional.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/contextMenuFallback.ts around line 96:
When a submenu overflows the right viewport edge, the `mouseenter` handler (lines 96-110) calls `clampMenuPosition` to flip it to the left side, but `openMenu`'s `requestAnimationFrame` callback (lines 132-134) immediately re-clamps it using the original `preferredLeft` (which caused the overflow). The menu ends up pinned to the right edge instead of the intended left-side position, making the flip logic non-functional.
Evidence trail:
apps/web/src/contextMenuFallback.ts lines 96-110 (mouseenter handler with flip logic), lines 131-133 (requestAnimationFrame callback in openMenu), lines 3-15 (clampMenuPosition function). The rAF callback scheduled in openMenu uses the original preferredLeft parameter, which overwrites any position changes made after openMenu returns but before the rAF callback executes.
There was a problem hiding this comment.
🟡 Medium components/Sidebar.tsx:2689
The inline selector at lines 2689-2692 creates a new object reference on every render. Since useSettings includes the selector function in its useMemo dependency array, projectGroupingSettings becomes a new reference each render. This causes the useMemo hooks at lines 2729-2734 and 2746-2763 to recompute unnecessarily, defeating memoization. Consider extracting the selector to a stable reference with useCallback or defining it outside the component.
const projectGroupingSettings = useSettings(useCallback((settings) => ({
sidebarProjectGroupingMode: settings.sidebarProjectGroupingMode,
sidebarProjectGroupingOverrides: settings.sidebarProjectGroupingOverrides,
}), []));
Also found in 1 other location(s)
apps/web/src/hooks/useHandleNewThread.ts:22
The inline selector arrow function passed to
useSettingscreates a new function reference on every render. SinceuseSettingsusesuseMemowith theselectorin its dependency array,projectGroupingSettingswill be a new object reference on every render even when the values haven't changed. This object is included in theuseCallbackdependency array at line 137, causinghandleNewThreadto be recreated on every render and defeating the purpose ofuseCallback. Consumers that depend onhandleNewThreadbeing stable (e.g., in effect dependencies or child component memoization) will see unnecessary re-renders or potentially infinite update loops.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/Sidebar.tsx around lines 2689-2692:
The inline selector at lines 2689-2692 creates a new object reference on every render. Since `useSettings` includes the `selector` function in its `useMemo` dependency array, `projectGroupingSettings` becomes a new reference each render. This causes the `useMemo` hooks at lines 2729-2734 and 2746-2763 to recompute unnecessarily, defeating memoization. Consider extracting the selector to a stable reference with `useCallback` or defining it outside the component.
Evidence trail:
apps/web/src/components/Sidebar.tsx lines 2689-2692 (inline selector), lines ~2729-2734 (physicalToLogicalKey useMemo with projectGroupingSettings dependency), lines ~2746-2763 (sidebarProjects useMemo with projectGroupingSettings dependency) at REVIEWED_COMMIT; apps/web/src/hooks/useSettings.ts line 141 shows `return useMemo(() => (selector ? selector(merged) : (merged as T)), [merged, selector]);` - the selector is in the dependency array at REVIEWED_COMMIT.
Also found in 1 other location(s):
- apps/web/src/hooks/useHandleNewThread.ts:22 -- The inline selector arrow function passed to `useSettings` creates a new function reference on every render. Since `useSettings` uses `useMemo` with the `selector` in its dependency array, `projectGroupingSettings` will be a new object reference on every render even when the values haven't changed. This object is included in the `useCallback` dependency array at line 137, causing `handleNewThread` to be recreated on every render and defeating the purpose of `useCallback`. Consumers that depend on `handleNewThread` being stable (e.g., in effect dependencies or child component memoization) will see unnecessary re-renders or potentially infinite update loops.
juliusmarminge
changed the title
|
Extended this a bit to solve #1912 as well: CleanShot.2026-04-16.at.00.01.22.mp4Can you verify that it still solves your problem? |
|
Can confirm this would fix the issue |
2 participants
What changed
This changes sidebar project grouping so projects are no longer grouped only by repository identity and fixes #2054 & fixes #1912
Previously, any project inside the same git repository shared the same logical sidebar key. That caused a parent directory project and a nested child project to collapse into one sidebar entry.
Now the logical project key uses:
This keeps cross-environment grouping for the same project path, but stops parent and nested directories from overlapping.
Why this should exist
If a user adds both:
both should appear as separate entries in the Projects sidebar immediately.
The old behavior hid one project behind the other until the first project was removed, which made the sidebar state incorrect and confusing.
Scope
This PR only changes nested project grouping behavior.
Tests
Note
Medium Risk
Changes how projects are keyed/grouped and how draft threads are reused, which can alter sidebar ordering/group membership and persisted UI state for existing users. Also expands context menu handling to nested menus, which could introduce edge-case UI regressions across Electron vs web fallback.
Overview
Fixes sidebar project grouping collisions by changing logical project keys to incorporate the repository root path (and optionally repo-relative subpaths), so parent and nested projects no longer collapse into a single sidebar row.
Introduces
SidebarProjectGroupingModeand per-project grouping overrides in client settings, updates key derivation/usage acrossSidebar,ChatView, and new-thread/draft reuse flows, and adds sidebar UI to change the global grouping mode plus a per-project grouping/rename workflow.Extends
RepositoryIdentityto includerootPath(resolved viagit rev-parse --show-toplevel) and upgrades context menus to support nested submenu items in both Electron IPC (main.ts) and the browser fallback (contextMenuFallback.ts), with accompanying test coverage updates.Reviewed by Cursor Bugbot for commit ce291f5. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Scope sidebar project grouping by repository root with per-project overrides
SidebarProjectGroupingModesetting (repository,repository_path,separate) that controls how projects are grouped in the sidebar, defaulting torepository.RepositoryIdentitywith arootPathfield (resolved inRepositoryIdentityResolver.ts) so logical project keys can be scoped to the git top-level directory.logicalProject.tsto derive logical project keys from grouping settings and repository root, with physical project keys as fallback.Sidebar.tsx) to expose grouping mode controls, per-project overrides, and submenus for multi-member project groups (rename, copy path, delete with emptiness check).contextMenuFallback.ts).Macroscope summarized ce291f5.