[codex] fix sidebar removal and rerender churn#3741
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a race condition where a still-mounted v2 workspace component could re-insert itself into the sidebar immediately after being removed — most visibly for remote workspaces. Two targeted patches address the problem: Confidence Score: 5/5Safe to merge; both re-add paths are closed and all remaining feedback is P2 style notes. The two targeted fixes (provider memoisation + removal of ensureWorkspaceInSidebar from the store subscription) are minimal, well-reasoned, and manually verified. The Part 2 hardening is additive and low-risk. Only P2 findings remain: the JSON.stringify fingerprinting edge cases are unlikely to cause visible bugs with the current plain-DTO types and are noted for future improvement. useDashboardSidebarData.ts — review JSON.stringify fingerprinting edge cases if DashboardSidebarProject or pullRequest types gain undefined-valued or non-serializable fields.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx | Memoizes getCollections() by activeOrganizationId and wraps contextValue in useMemo — stabilises the context object and fixes the primary re-add trigger for the sidebar removal race. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts | Removes ensureWorkspaceInSidebar from the store-subscription callback and trims projectId/ensureWorkspaceInSidebar from the effect deps — eliminates the second re-add path after sidebar removal. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts | Adds three stable-identity hooks (string array, PR map, project array) using useRef + JSON.stringify fingerprinting; JSON.stringify cost is amortised over 10 s poll intervals but could silently drop undefined-valued PR fields from equality checks. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts | Adds useStableWorkspaceShortcutLabels to preserve Map identity when the first nine workspace IDs are unchanged, reducing downstream memo invalidation. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx | Wraps SortableProjectWrapper in React.memo; props interface extracted for clarity. Low-risk render optimisation. |
| apps/desktop/plans/done/20260425-1430-sidebar-remove-and-rerender.md | Implementation plan/notes — documentation only, no executable changes. |
Sequence Diagram
sequenceDiagram
participant User as User (right-click)
participant SidebarState as useDashboardSidebarState
participant Collections as CollectionsProvider (memoized)
participant PaneLayout as useV2WorkspacePaneLayout
participant PaneStore as Pane Store
User->>SidebarState: removeWorkspaceFromSidebar(id)
SidebarState->>Collections: v2WorkspaceLocalState.delete(id)
Note over Collections: Row removed ✓
Note over PaneStore: Store emits (workspace still mounted)
PaneStore->>PaneLayout: subscription callback fires
PaneLayout->>Collections: get(workspaceId) → undefined (FIXED: no ensureWorkspaceInSidebar call)
PaneLayout-->>PaneLayout: return early — row not re-inserted ✓
Note over Collections: Parent re-renders (unrelated cause)
Collections-->>Collections: contextValue stable (useMemo on activeOrganizationId) ✓
Note over PaneLayout: ensureWorkspaceInSidebar dep unchanged → mount effect does NOT re-run ✓
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 56-60
Comment:
**`JSON.stringify` fingerprinting silently drops `undefined` fields**
`JSON.stringify` omits object keys whose values are `undefined`, so a `pullRequest` or project field transitioning between `undefined` and a missing key would produce identical fingerprints, masking a real change. The same applies to `getDashboardSidebarProjectFingerprint`. For pure data DTOs this is unlikely to matter in practice, but a shallow recursive equality helper (or a library like `fast-deep-equal`) would be more explicit and wouldn't silently coerce `undefined` → absent.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 56-66
Comment:
**`getPullRequestRowsFingerprint` sorts before stringifying, but `getDashboardSidebarProjectFingerprint` does not**
`getPullRequestRowsFingerprint` sorts rows by `workspaceId` to get a stable fingerprint regardless of API response order. `getDashboardSidebarProjectFingerprint` calls `JSON.stringify(project)` directly, which depends on property-insertion order. If any nested array inside a project is reordered by the live query (e.g. workspaces within a section), two semantically equal projections can produce different fingerprints, causing unnecessary reference churn instead of the intended stability.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix sidebar removal and rerender churn" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| function getDashboardSidebarProjectFingerprint( | ||
| project: DashboardSidebarProject, | ||
| ): string { |
There was a problem hiding this comment.
JSON.stringify fingerprinting silently drops undefined fields
JSON.stringify omits object keys whose values are undefined, so a pullRequest or project field transitioning between undefined and a missing key would produce identical fingerprints, masking a real change. The same applies to getDashboardSidebarProjectFingerprint. For pure data DTOs this is unlikely to matter in practice, but a shallow recursive equality helper (or a library like fast-deep-equal) would be more explicit and wouldn't silently coerce undefined → absent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 56-60
Comment:
**`JSON.stringify` fingerprinting silently drops `undefined` fields**
`JSON.stringify` omits object keys whose values are `undefined`, so a `pullRequest` or project field transitioning between `undefined` and a missing key would produce identical fingerprints, masking a real change. The same applies to `getDashboardSidebarProjectFingerprint`. For pure data DTOs this is unlikely to matter in practice, but a shallow recursive equality helper (or a library like `fast-deep-equal`) would be more explicit and wouldn't silently coerce `undefined` → absent.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| function getDashboardSidebarProjectFingerprint( | ||
| project: DashboardSidebarProject, | ||
| ): string { | ||
| return JSON.stringify(project); | ||
| } | ||
|
|
||
| function useStableStringArray(values: string[]): string[] { | ||
| const previousRef = useRef<string[] | null>(null); | ||
|
|
There was a problem hiding this comment.
getPullRequestRowsFingerprint sorts before stringifying, but getDashboardSidebarProjectFingerprint does not
getPullRequestRowsFingerprint sorts rows by workspaceId to get a stable fingerprint regardless of API response order. getDashboardSidebarProjectFingerprint calls JSON.stringify(project) directly, which depends on property-insertion order. If any nested array inside a project is reordered by the live query (e.g. workspaces within a section), two semantically equal projections can produce different fingerprints, causing unnecessary reference churn instead of the intended stability.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 56-66
Comment:
**`getPullRequestRowsFingerprint` sorts before stringifying, but `getDashboardSidebarProjectFingerprint` does not**
`getPullRequestRowsFingerprint` sorts rows by `workspaceId` to get a stable fingerprint regardless of API response order. `getDashboardSidebarProjectFingerprint` calls `JSON.stringify(project)` directly, which depends on property-insertion order. If any nested array inside a project is reordered by the live query (e.g. workspaces within a section), two semantically equal projections can produce different fingerprints, causing unnecessary reference churn instead of the intended stability.
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Fixes the sidebar removal race where a still-mounted v2 workspace could reinsert itself after Remove from Sidebar, especially for remote workspaces. The provider now keeps the collections context stable across unrelated renders, and pane-layout persistence no longer calls
ensureWorkspaceInSidebarbefore checking that the local sidebar row still exists.Also adds low-risk sidebar re-render hardening without splitting
v2WorkspaceLocalState: stable local workspace id arrays, stable pull-request maps for structurally equal refetches, stable project references for unchanged project trees, stable shortcut-label maps, and a memo boundary for sortable project rows.The implementation notes were moved into
apps/desktop/plans/done/20260425-1430-sidebar-remove-and-rerender.md.Validation
bunx @biomejs/biome@2.4.2 check <changed files>bun run --cwd apps/desktop typecheckSummary by cubic
Fixes the “Remove from Sidebar” race that caused remote workspaces to reappear and reduces sidebar re-render churn for smoother performance.
Bug Fixes
CollectionsProviderby memoizinggetCollections(activeOrganizationId)and the context value, preventing stale workspace effects from re-running.useV2WorkspacePaneLayoutto stop auto-ensuring sidebar rows during pane-store updates; now persists layout only if the local row still exists.Refactors
Mapwhen refetched data is structurally equal.memoboundary for sortable project rows.Mapwhen the first nine workspace IDs are unchanged.Written for commit 3e40656. Summary will update on new commits.