Skip to content

fix(renderer): guard window.maestro accesses in useAppInitialization#1072

Open
ngothanhthien wants to merge 1 commit into
RunMaestro:rcfrom
ngothanhthien:fix/loading-screen-git-undefined
Open

fix(renderer): guard window.maestro accesses in useAppInitialization#1072
ngothanhthien wants to merge 1 commit into
RunMaestro:rcfrom
ngothanhthien:fix/loading-screen-git-undefined

Conversation

@ngothanhthien
Copy link
Copy Markdown

@ngothanhthien ngothanhthien commented Jun 3, 2026

Summary

Fixes the loading-screen error: Error: Cannot read properties of undefined (reading 'git') shown beneath the splash progress bar.

Root cause

useAppInitialization.ts had several useEffect hooks that accessed window.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.maestro by the time React mounted, the renderer would throw, and splash.js would render the error text on the splash screen — preventing the app from ever loading.

Fix

Apply optional chaining (?.) to all window.maestro accesses inside useAppInitialization.ts so 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

  • 730/730 test files pass, 28 521/28 521 tests pass (npm test)
  • npm run format:check:all → clean
  • npm run lint → clean

Files changed

  • src/renderer/hooks/ui/useAppInitialization.ts (13 insertions, 11 deletions)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved app initialization to gracefully handle unavailable or missing system APIs and dependencies throughout the startup process. The application now prevents crashes when certain features are not accessible, ensuring stable and reliable operation across different environments, system configurations, and various setups without affecting core functionality or user experience.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: afc6a039-02de-497f-94c7-2dce7bf7e4c6

📥 Commits

Reviewing files that changed from the base of the PR and between 575efd0 and c7d02a8.

📒 Files selected for processing (1)
  • src/renderer/hooks/ui/useAppInitialization.ts

📝 Walkthrough

Walkthrough

The PR adds defensive optional chaining and early-return guards throughout the useAppInitialization hook to gracefully handle missing or unavailable Electron/maestro APIs. All Electron/settings/updates/leaderboard API calls are now protected to prevent runtime errors during initialization.

Changes

App Initialization Resilience

Layer / File(s) Summary
API availability checks
src/renderer/hooks/ui/useAppInitialization.ts
GitHub CLI availability and Windows power status checks now use optional chaining on window.maestro?.git?.checkGhCli() and window.maestro?.power?.getStatus() to safely no-op when APIs are missing.
Settings file gist URL persistence
src/renderer/hooks/ui/useAppInitialization.ts
File gist URLs are loaded via window.maestro?.settings?.get('fileGistUrls') and saved via window.maestro?.settings?.set?.('fileGistUrls', ...) with optional chaining; beta prerelease sync calls window.maestro?.updates?.setAllowPrerelease?.(...).
Async timer early-return guards
src/renderer/hooks/ui/useAppInitialization.ts
Startup update check and leaderboard sync timers now return early if window.maestro?.updates?.check or window.maestro?.leaderboard?.sync are missing before timer invocation.

🎯 2 (Simple) | ⏱️ ~10 minutes

approved

🐰 A hook once brittle, now stands tall,
With question marks that catch the fall,
When maestro's gone or APIs shy,
The app won't crash—it'll just get by!
Optional chains and early returns bright,
Make initialization bulletproof and right. 🎯

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title precisely describes the main change: guarding window.maestro accesses in useAppInitialization to handle undefined scenarios during startup.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 3, 2026

Greptile Summary

This PR applies optional chaining (?.) to all window.maestro.* calls inside useAppInitialization.ts to guard against a race condition where Electron's contextBridge hasn't finished exposing the API by the time React's useEffect hooks fire.

  • Three promise chains (git.checkGhCli, power.getStatus, settings.get('fileGistUrls')) now use ?.then(...) but still end with a bare .catch(...). Because optional chaining short-circuits and returns undefined when window.maestro is absent, the unguarded .catch() call on undefined throws a new TypeError — meaning the crash still occurs in exactly the scenario the PR is meant to fix.
  • The guard patterns used for updates.check and leaderboard.sync (explicit if (!window.maestro?.…) return before an await call) are correctly written and handle the race condition as intended.
  • window.maestro?.settings?.set?.('fileGistUrls', updated) and window.maestro?.updates?.setAllowPrerelease?.(enableBetaUpdates) are also correctly written.

Confidence Score: 3/5

The 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

Filename Overview
src/renderer/hooks/ui/useAppInitialization.ts Adds optional chaining to window.maestro accesses to tolerate a delayed contextBridge; however, three promise chains use ?.then() followed by a bare .catch(), which will still throw TypeError on undefined when the guard is needed most.

Sequence Diagram

sequenceDiagram
    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
Loading

Comments Outside Diff (1)

  1. src/renderer/hooks/ui/useAppInitialization.ts, line 84-92 (link)

    P1 Incomplete guard — .catch() still throws when window.maestro is undefined

    The optional chain short-circuits at ?.then(...) and returns undefined, but the immediately following .catch(...) is a regular method call on that undefined result. When window.maestro is absent (the exact race condition this PR targets), this produces a new TypeError: Cannot read properties of undefined (reading 'catch') — a different crash at a different line, but a crash all the same, which would still surface on the splash screen.

    The same pattern appears in the power.getStatus() chain (line ~107) and the settings.get('fileGistUrls') chain (line ~121). All three need ?.catch() instead of .catch().

Reviews (1): Last reviewed commit: "fix(renderer): guard window.maestro acce..." | Re-trigger Greptile

@pedramamini
Copy link
Copy Markdown
Collaborator

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 window.maestro.* accesses in useAppInitialization.ts is the right way to tolerate the brief window before the preload contextBridge finishes exposing window.maestro. CI is green (2/2) and there are no merge conflicts.

One note on the automated review, for the record: Greptile flagged (P1) that the three promise chains ending in a bare .catch() — e.g.

window.maestro?.git
	?.checkGhCli()
	?.then(...)
	.catch(...);

would throw a fresh TypeError on the trailing .catch() when window.maestro is undefined. That's a false positive: optional-chaining short-circuiting extends to the end of the entire chain expression, so when window.maestro is nullish the whole expression — including the non-optional .catch() — evaluates to undefined without ever invoking .catch(). Confirmed empirically:

const w = { maestro: undefined };
w.maestro?.git?.checkGhCli()?.then(() => {}).catch(() => {}); // => undefined, no throw

So the guard is complete as written; no change needed there.

Nice, surgical fix — thanks again! 🙏

@jSydorowicz21
Copy link
Copy Markdown
Contributor

Hey @ngothanhthien could you retarget this to RC? Our branching strategy is to let things cook in a release candidate before merging into mainline

@jSydorowicz21 jSydorowicz21 changed the base branch from main to rc June 6, 2026 17:59
@jSydorowicz21 jSydorowicz21 force-pushed the fix/loading-screen-git-undefined branch from c7d02a8 to 09103a6 Compare June 6, 2026 18:53
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.
@jSydorowicz21 jSydorowicz21 force-pushed the fix/loading-screen-git-undefined branch from 09103a6 to 7cc5754 Compare June 6, 2026 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants