Fix crash on project selection and worktree switch#795
Fix crash on project selection and worktree switch#795gsxdsm merged 1 commit intoAutoMaker-Org:v0.15.0rcfrom
Conversation
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements several targeted optimizations and fixes to enhance the stability and performance of the UI, particularly around project and worktree switching. By leveraging React's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThe PR optimizes UI performance during project and worktree switches by: batching state updates in ProjectSwitcher via Changes
Sequence DiagramsequenceDiagram
actor User
participant ProjectSwitcher
participant startTransition
participant Navigation
participant BoardView as BoardView<br/>(Worktree Selector)
participant useAutoMode
participant UICache as UICache Store
User->>ProjectSwitcher: Click project
ProjectSwitcher->>startTransition: Batch state & nav
startTransition->>ProjectSwitcher: Low-priority update
ProjectSwitcher->>Navigation: Navigate to /board
Navigation->>BoardView: Render with new project
BoardView->>BoardView: Memoized worktree select<br/>with ref stabilization
BoardView->>useAutoMode: Mount/trigger
useAutoMode->>useAutoMode: Debounce 150ms
useAutoMode->>UICache: refreshStatus()
UICache->>UICache: Sanitize worktree cache<br/>(null-path only)
UICache->>BoardView: Return stabilized ref
BoardView->>User: Render stable worktree UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a crash related to cascading re-renders during project and worktree switches. The use of startTransition in the project switcher, reference stabilization in the board view, and debouncing in the useAutoMode hook are all appropriate and well-implemented solutions for breaking render loops. Additionally, the change to sanitize cached worktree data on write is a great defensive improvement that enhances application stability. I have one minor suggestion to refactor a loop for conciseness, but overall this is an excellent fix.
| const sanitized: Record<string, { path: string | null; branch: string }> = {}; | ||
| for (const [projectPath, worktree] of Object.entries(appState.currentWorktreeByProject)) { | ||
| if ( | ||
| typeof worktree === 'object' && | ||
| worktree !== null && | ||
| 'path' in worktree && | ||
| worktree.path === null | ||
| ) { | ||
| sanitized[projectPath] = worktree; | ||
| } | ||
| } | ||
| update.cachedCurrentWorktreeByProject = sanitized; |
There was a problem hiding this comment.
This sanitization logic is great for ensuring cache integrity. For improved conciseness and a more functional style, you could consider refactoring this loop using Object.fromEntries and Array.prototype.filter.
const sanitized = Object.fromEntries(
Object.entries(appState.currentWorktreeByProject).filter(
([, worktree]) =>
typeof worktree === 'object' &&
worktree !== null &&
'path' in worktree &&
worktree.path === null
)
);
update.cachedCurrentWorktreeByProject = sanitized;![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/ui/src/components/layout/project-switcher/project-switcher.tsx (1)
152-208:handleOpenFoldernavigates outsidestartTransition— inconsistency worth aligning.
handleOpenFoldercallsupsertAndSetCurrentProject(...)(which also setscurrentProjectin the store) followed bynavigate({ to: '/board' })at line 200, without the transition batching added tohandleProjectClick. When a user opens an existing project via the folder picker, the same store-update → navigate cascade that caused the original crash could theoretically recur.♻️ Proposed fix for handleOpenFolder (line 200)
- navigate({ to: '/board' }); + startTransition(() => { + navigate({ to: '/board' }); + });Note:
upsertAndSetCurrentProjectfires before this point (line 177) so it remains outside the transition; only the navigation call needs to be deferred here, mirroring the pattern inhandleProjectClick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/project-switcher/project-switcher.tsx` around lines 152 - 208, The navigation call in handleOpenFolder runs immediately after upsertAndSetCurrentProject, risking the same store-update→navigate race that startTransition avoided in handleProjectClick; wrap the navigate({ to: '/board' }) call in React's startTransition (e.g. import { startTransition } from 'react' and call startTransition(() => navigate({ to: '/board' }))) so navigation is deferred/batched like in handleProjectClick, keeping upsertAndSetCurrentProject unchanged.apps/ui/src/components/views/board-view.tsx (1)
487-487: Document the intentional stale-field invariant in the stabilization check.The stabilization correctly returns
prevwhen the five key fields are unchanged, preventing reference-equality churn foruseAutoMode. However, optional fields spread fromfound(hasChanges,changedFilesCount,pr,hasConflicts,conflictType,conflictFiles) are silently frozen whenever those five fields are stable — even if the backing store data for those fields has updated.This is safe today because none of those optional fields are consumed from
selectedWorktreeanywhere inboard-view.tsx(theWorktreePanelsources its own detail data viarefreshTrigger). But the invariant is non-obvious and creates a hidden trap for future readers.📝 Proposed inline documentation
// Return the previous reference if the key fields haven't changed, // preventing downstream hooks from seeing a "new" worktree on every render. + // + // NOTE: Only the five identity fields (path, branch, isMain, isCurrent, + // hasWorktree) are included in this check. Optional fields spread from the + // store (hasChanges, pr, hasConflicts, etc.) will NOT update while these + // five fields are stable. This is intentional — useAutoMode (the only + // consumer of selectedWorktree) does not read those fields. If future code + // needs hasChanges/pr from selectedWorktree, either add those fields to this + // check or source them directly from the `worktrees` array. const prev = prevSelectedWorktreeRef.current;Also applies to: 521-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/board-view.tsx` at line 487, The stabilization logic around prevSelectedWorktreeRef intentionally returns the previous object when the five core identity fields are unchanged, which freezes optional spread fields (hasChanges, changedFilesCount, pr, hasConflicts, conflictType, conflictFiles) even if their backing store updates; add an explicit comment next to the stabilization block referencing prevSelectedWorktreeRef and selectedWorktree explaining this stale-field invariant, why it exists (to prevent reference-equality churn for useAutoMode), that WorktreePanel consumes up-to-date detail data via refreshTrigger, and warn future maintainers not to rely on optional fields from selectedWorktree (also apply the same explanatory comment to the analogous block around the logic at the other stabilization site).apps/ui/src/store/ui-cache-store.ts (1)
116-134: Consider extracting the duplicate sanitization predicate into a shared helper.The
path === nullfiltering loop added here (lines 123–133) is structurally identical to the one already present inrestoreFromUICache(lines 192–207). If the filtering rule ever changes (e.g., a future migration retains a specific set of non-null paths), both branches must be updated in lockstep.♻️ Proposed extraction
+/** + * Strips any entries whose worktree `path` is non-null (i.e. points to a + * directory that may no longer exist on disk). Only main-branch entries + * (path === null) are safe to persist/restore across app restarts. + */ +function filterMainBranchWorktrees( + input: Record<string, { path: string | null; branch: string }> +): Record<string, { path: string | null; branch: string }> { + const result: Record<string, { path: string | null; branch: string }> = {}; + for (const [projectPath, worktree] of Object.entries(input)) { + if ( + typeof worktree === 'object' && + worktree !== null && + 'path' in worktree && + worktree.path === null + ) { + result[projectPath] = worktree; + } + } + return result; +}Then in
syncUICache:- const sanitized: Record<string, { path: string | null; branch: string }> = {}; - for (const [projectPath, worktree] of Object.entries(appState.currentWorktreeByProject)) { - if ( - typeof worktree === 'object' && - worktree !== null && - 'path' in worktree && - worktree.path === null - ) { - sanitized[projectPath] = worktree; - } - } - update.cachedCurrentWorktreeByProject = sanitized; + update.cachedCurrentWorktreeByProject = filterMainBranchWorktrees(appState.currentWorktreeByProject);And replace the equivalent block in
restoreFromUICache(lines 192–207) with a call to the same helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/store/ui-cache-store.ts` around lines 116 - 134, The duplicate sanitization logic that filters currentWorktreeByProject by worktree.path === null appears in syncUICache and restoreFromUICache; extract it into a single helper (e.g., sanitizeCurrentWorktreeByProject or isPersistableWorktree) and replace the loop in both syncUICache and restoreFromUICache to call that helper so the predicate and map-building live in one place (ensure the helper accepts the original Record<string, any> and returns Record<string, { path: string | null; branch: string }> preserving the exact semantics).apps/ui/src/hooks/use-auto-mode.ts (1)
265-268: Extract the150debounce delay to a named constant.
AUTO_MODE_POLLING_INTERVAL(line 20) sets the pattern for named timing constants. Keeping150inline is a minor inconsistency, and co-locating it with the polling interval makes both easier to adjust.♻️ Proposed change
+const AUTO_MODE_INITIAL_SYNC_DELAY_MS = 150; const AUTO_MODE_POLLING_INTERVAL = 30000;- const timer = setTimeout(() => void refreshStatus(), 150); + const timer = setTimeout(() => void refreshStatus(), AUTO_MODE_INITIAL_SYNC_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-auto-mode.ts` around lines 265 - 268, Extract the literal 150ms debounce into a named constant (e.g. AUTO_MODE_DEBOUNCE_MS) located alongside the existing AUTO_MODE_POLLING_INTERVAL, then replace the inline 150 in the useEffect timeout call with that constant so the useEffect hook (which calls refreshStatus via setTimeout) uses the new named debounce value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ui/src/components/layout/project-switcher/project-switcher.tsx`:
- Around line 152-208: The navigation call in handleOpenFolder runs immediately
after upsertAndSetCurrentProject, risking the same store-update→navigate race
that startTransition avoided in handleProjectClick; wrap the navigate({ to:
'/board' }) call in React's startTransition (e.g. import { startTransition }
from 'react' and call startTransition(() => navigate({ to: '/board' }))) so
navigation is deferred/batched like in handleProjectClick, keeping
upsertAndSetCurrentProject unchanged.
In `@apps/ui/src/components/views/board-view.tsx`:
- Line 487: The stabilization logic around prevSelectedWorktreeRef intentionally
returns the previous object when the five core identity fields are unchanged,
which freezes optional spread fields (hasChanges, changedFilesCount, pr,
hasConflicts, conflictType, conflictFiles) even if their backing store updates;
add an explicit comment next to the stabilization block referencing
prevSelectedWorktreeRef and selectedWorktree explaining this stale-field
invariant, why it exists (to prevent reference-equality churn for useAutoMode),
that WorktreePanel consumes up-to-date detail data via refreshTrigger, and warn
future maintainers not to rely on optional fields from selectedWorktree (also
apply the same explanatory comment to the analogous block around the logic at
the other stabilization site).
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 265-268: Extract the literal 150ms debounce into a named constant
(e.g. AUTO_MODE_DEBOUNCE_MS) located alongside the existing
AUTO_MODE_POLLING_INTERVAL, then replace the inline 150 in the useEffect timeout
call with that constant so the useEffect hook (which calls refreshStatus via
setTimeout) uses the new named debounce value.
In `@apps/ui/src/store/ui-cache-store.ts`:
- Around line 116-134: The duplicate sanitization logic that filters
currentWorktreeByProject by worktree.path === null appears in syncUICache and
restoreFromUICache; extract it into a single helper (e.g.,
sanitizeCurrentWorktreeByProject or isPersistableWorktree) and replace the loop
in both syncUICache and restoreFromUICache to call that helper so the predicate
and map-building live in one place (ensure the helper accepts the original
Record<string, any> and returns Record<string, { path: string | null; branch:
string }> preserving the exact semantics).
1 participant
Fix crash on project load and worktree switch
Summary by CodeRabbit
Release Notes