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 addresses a critical issue where the Progressive Web App (PWA) would crash in a loop, particularly after worktree deletions, due to attempting to restore stale cached worktree paths. The changes introduce robust crash loop detection and an automatic cache-clearing mechanism within the error boundary. Additionally, the UI cache restoration process has been made more resilient by sanitizing cached worktree selections, ensuring that only valid, non-stale data is restored, thereby preventing future crashes related to invalid worktree states. 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
|
📝 WalkthroughWalkthroughThis change adds crash-loop detection and recovery to the error boundary component, and sanitizes the cached worktree selection in the UI cache store to prevent restoring stale or invalid worktree references. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 PWA crash loop caused by stale worktree paths in the cache. The solution is two-pronged: first, it introduces a robust crash loop detection in the AppErrorBoundary to automatically clear the cache after repeated crashes. Second, it fixes the root cause by sanitizing the ui-cache-store on restore, ensuring only safe worktree selections are loaded. The implementation is solid. I've included a couple of suggestions to enhance code maintainability and readability.
| /** Number of crashes within the time window that constitutes a crash loop */ | ||
| const CRASH_LOOP_THRESHOLD = 3; | ||
| /** Time window in ms for crash loop detection (30 seconds) */ | ||
| const CRASH_LOOP_WINDOW_MS = 30_000; |
There was a problem hiding this comment.
To avoid using a magic string for the UI cache key ('automaker-ui-cache'), it's good practice to define it as a constant here, alongside the other constants. This key is used in componentDidCatch and handleClearCacheAndReload.
After adding this constant, you can replace the hardcoded string in both places.
const CRASH_LOOP_WINDOW_MS = 30_000;
/** Key for the UI cache in localStorage, used for clearing on crash loops. */
const UI_CACHE_KEY = 'automaker-ui-cache';
| const sanitized: Record<string, { path: string | null; branch: string }> = {}; | ||
| for (const [projectPath, worktree] of Object.entries(cache.cachedCurrentWorktreeByProject)) { | ||
| if (worktree.path === null) { | ||
| // Main branch selection — always safe to restore | ||
| sanitized[projectPath] = worktree; | ||
| } | ||
| // Non-null paths are dropped; the app will re-discover actual worktrees | ||
| // from the server and the validation effect in use-worktrees will handle | ||
| // resetting to main if the cached worktree no longer exists. | ||
| } | ||
| if (Object.keys(sanitized).length > 0) { | ||
| stateUpdate.currentWorktreeByProject = sanitized; | ||
| } |
There was a problem hiding this comment.
This logic for sanitizing the worktree cache can be written more concisely using functional programming constructs like filter and Object.fromEntries. This improves readability by making the intent of filtering and creating a new object more explicit.
// Main branch selection — always safe to restore
const sanitizedEntries = Object.entries(cache.cachedCurrentWorktreeByProject).filter(
([, worktree]) => worktree.path === null
);
if (sanitizedEntries.length > 0) {
stateUpdate.currentWorktreeByProject = Object.fromEntries(sanitizedEntries);
}
// Non-null paths are dropped; the app will re-discover actual worktrees
// from the server and the validation effect in use-worktrees will handle
// resetting to main if the cached worktree no longer exists.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ui/src/components/ui/app-error-boundary.tsx (1)
71-71: Consider extracting the'automaker-ui-cache'key to avoid magic string duplication.The literal
'automaker-ui-cache'appears here in two places (lines 71 and 88) and is also defined asSTORE_NAMEinui-cache-store.ts. If the store name ever changes, these references will silently diverge. Consider either:
- Exporting
STORE_NAME(or aclearUICache()helper) from the cache store and importing it here, or- Defining a shared constant.
♻️ Option 1: Export a helper from the cache store
In
apps/ui/src/store/ui-cache-store.ts:+/** Clear all persisted UI cache data. Useful for crash-loop recovery. */ +export function clearUICache(): void { + try { + localStorage.removeItem(STORE_NAME); + } catch { + // localStorage may be unavailable + } +}Then in
app-error-boundary.tsx:+import { clearUICache } from '@/store/ui-cache-store'; ... -localStorage.removeItem('automaker-ui-cache'); +clearUICache();Also applies to: 84-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/ui/app-error-boundary.tsx` at line 71, The hard-coded 'automaker-ui-cache' string in app-error-boundary.tsx should be replaced with a single exported identifier from the cache module to avoid divergence: export STORE_NAME (or a clearUICache() helper) from ui-cache-store.ts and import it into app-error-boundary.tsx, then replace localStorage.removeItem('automaker-ui-cache') (and any other occurrences in that file) with use of the imported STORE_NAME (or call clearUICache()); update the import and references in app-error-boundary.tsx so all cache key usage is driven by the shared symbol.
🤖 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/ui/app-error-boundary.tsx`:
- Line 71: The hard-coded 'automaker-ui-cache' string in app-error-boundary.tsx
should be replaced with a single exported identifier from the cache module to
avoid divergence: export STORE_NAME (or a clearUICache() helper) from
ui-cache-store.ts and import it into app-error-boundary.tsx, then replace
localStorage.removeItem('automaker-ui-cache') (and any other occurrences in that
file) with use of the imported STORE_NAME (or call clearUICache()); update the
import and references in app-error-boundary.tsx so all cache key usage is driven
by the shared symbol.
* Changes from fix/docker-compose-cors-error * Update apps/server/src/index.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Fix: Delete Worktree Crash + PR Comments + Dev Server UX Improvements (#792) * Changes from fix/delete-worktree-hotifx * fix: Improve bot detection and prevent UI overflow issues - Include GitHub app-initiated comments in bot detection - Wrap handleQuickCreateSession with useCallback to fix dependency issues - Truncate long branch names in agent header to prevent layout overflow * feat: Support GitHub App comments in PR review and fix session filtering * feat: Return invalidation result from delete session handler * fix: Improve CORS origin validation to handle wildcard correctly * fix: Correct IPv6 localhost parsing and improve responsive UI layouts * Changes from fix/pwa-cache-fix (#794) * fix: Add type checking to prevent crashes from malformed cache entries --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Fix crash on PWA worktree deletion
Summary by CodeRabbit
New Features
Bug Fixes