fix(renderer): guard window.maestro accesses in useAppInitialization#1072
fix(renderer): guard window.maestro accesses in useAppInitialization#1072ngothanhthien wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds defensive optional chaining and early-return guards throughout the ChangesApp Initialization Resilience
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR applies optional chaining (
Confidence Score: 3/5The fix partially guards window.maestro accesses but three promise chains still have a bare .catch() that will throw when the preload bridge is absent, leaving the splash-screen crash intact for those code paths. Three of the guarded promise chains end with an unguarded .catch() call on a value that is undefined when the race condition fires. This means the crash the PR is intended to eliminate can still occur, just at a different callsite. The correctly-guarded paths are fine, but the three broken chains include the very first effect that runs on mount (git.checkGhCli), making the failure likely to reproduce. src/renderer/hooks/ui/useAppInitialization.ts — lines 84–92, 105–115, and 119–129 each end a guarded promise chain with a bare .catch() that needs to become ?.catch(). Important Files Changed
Sequence DiagramsequenceDiagram
participant Electron as Electron Main
participant Preload as Preload / contextBridge
participant React as React (useAppInitialization)
participant Splash as Splash Screen
Electron->>Preload: launch renderer
React->>React: mount (useEffect hooks fire)
Note over Preload,React: Race condition window
alt window.maestro is undefined
React->>React: window.maestro?.git → undefined
React->>React: ?.checkGhCli() → undefined
React->>React: ?.then(...) → undefined
React->>React: .catch(...) on undefined → TypeError ❌
React->>Splash: uncaught error displayed
else window.maestro defined
React->>Preload: window.maestro.git.checkGhCli()
Preload-->>React: "Promise<status>"
React->>React: .then() / .catch() work normally ✅
end
Preload->>React: contextBridge wires up window.maestro
|
|
Thanks for the contribution, @ngothanhthien! 🎉 I reviewed this alongside the automated feedback from CodeRabbit and Greptile, and I'm happy with it — approved. ✅ The fix is correctly scoped: applying optional chaining to the One note on the automated review, for the record: Greptile flagged (P1) that the three promise chains ending in a bare window.maestro?.git
?.checkGhCli()
?.then(...)
.catch(...);would throw a fresh const w = { maestro: undefined };
w.maestro?.git?.checkGhCli()?.then(() => {}).catch(() => {}); // => undefined, no throwSo the guard is complete as written; no change needed there. Nice, surgical fix — thanks again! 🙏 |
|
Hey @ngothanhthien could you retarget this to RC? Our branching strategy is to let things cook in a release candidate before merging into mainline |
c7d02a8 to
09103a6
Compare
Multiple useEffect hooks in useAppInitialization.ts accessed window.maestro.* directly without optional chaining. If the preload bridge hadn't finished wiring up window.maestro by the time React mounted, the renderer would throw "Cannot read properties of undefined (reading 'git')" and the splash.js error handler would display it on the loading screen, preventing the app from rendering. Apply optional chaining to all window.maestro accesses in this hook so the app tolerates a brief window where the preload contextBridge has not yet exposed the API.
09103a6 to
7cc5754
Compare
Summary
Fixes the loading-screen error:
Error: Cannot read properties of undefined (reading 'git')shown beneath the splash progress bar.Root cause
useAppInitialization.tshad severaluseEffecthooks that accessedwindow.maestro.*directly (e.g.window.maestro.git.checkGhCli(),window.maestro.power.getStatus(),window.maestro.settings.get(...),window.maestro.updates.setAllowPrerelease(...),window.maestro.updates.check(...),window.maestro.leaderboard.sync(...)).If the preload contextBridge had not finished wiring up
window.maestroby the time React mounted, the renderer would throw, andsplash.jswould render the error text on the splash screen — preventing the app from ever loading.Fix
Apply optional chaining (
?.) to allwindow.maestroaccesses insideuseAppInitialization.tsso the app tolerates a brief window where the preload contextBridge has not yet exposed the API. The previous design assumed a strict ordering that is not always honored at startup.Verification
npm test)npm run format:check:all→ cleannpm run lint→ cleanFiles changed
src/renderer/hooks/ui/useAppInitialization.ts(13 insertions, 11 deletions)🤖 Generated with Claude Code
Summary by CodeRabbit