fix(desktop): harden renderer boot to avoid white screens#1190
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds macOS Chromium switch, GPU crash handling and forced repaint in the main process; introduces renderer boot-error handling (report/render/init/cleanup), a BootErrorBoundary React component, guarded renderer bootstrap with router context/preload changes, and resilient PostHog initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Electron App (main)
participant GPU as GPU Process
participant Main as Main Window
participant Renderer as Renderer Process
GPU-->>App: child-process-gone (GPU)
App->>Main: notify child-process-gone (details)
Main->>Main: if details.type == "GPU" -> webContents.invalidate()
Main->>Main: forceRepaint(win): invalidate -> resize (w+1,h) -> wait -> restore size
Main->>Renderer: compositor repaint / renderer refresh
Note over Main,Renderer: On macOS -> disable backgrounding & invalidate on restore/show
sequenceDiagram
participant Renderer as Renderer Process
participant Bootstrap as index.tsx (bootstrap)
participant BootLib as boot-errors lib
participant Boundary as BootErrorBoundary
participant Router as RouterProvider
participant UI as Application UI
Renderer->>Bootstrap: start initialization
Bootstrap->>BootLib: initBootErrorHandling(root)
BootLib-->>Bootstrap: listeners attached
Bootstrap->>Bootstrap: check root element
alt root missing or early boot error
Bootstrap->>BootLib: reportBootError(msg, err)
BootLib->>Bootstrap: renderBootError overlay into root
else root present
Bootstrap->>Boundary: mount BootErrorBoundary
Boundary->>Router: createRouter(defaultPreload="intent", context:{ queryClient })
Router->>UI: render application
UI-->>Boundary: runtime error -> componentDidCatch -> optional onError
Boundary->>UI: render fallback UI
Bootstrap->>BootLib: markBootMounted()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
Kitenite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/index.tsx`:
- Around line 141-147: The hasMounted flag is being set immediately after
calling ReactDom.createRoot(...).render, which is premature because rendering is
asynchronous and makes reportBootError's early return (if (hasMounted) return;)
bypass legitimate boot errors caught by BootErrorBoundary; fix by removing the
immediate hasMounted = true and instead set hasMounted to true from inside the
app once the root component has actually mounted (e.g. add an onMount prop or
use a useEffect inside the top-level component/BootErrorBoundary to call a
provided callback that sets hasMounted), update BootErrorBoundary usage to
accept that mount callback, and ensure reportBootError still receives errors
from BootErrorBoundary so the flag gating behaves correctly.
🧹 Nitpick comments (4)
apps/desktop/src/main/windows/main.ts (1)
92-92: Extract the background color to a named constant.
This avoids magic values per the TS guideline and keeps styling centralized.♻️ Suggested refactor
@@ -let currentWindow: BrowserWindow | null = null; +let currentWindow: BrowserWindow | null = null; +const MAIN_WINDOW_BACKGROUND_COLOR = "#1a1a1a"; @@ const window = createWindow({ @@ - backgroundColor: "#1a1a1a", + backgroundColor: MAIN_WINDOW_BACKGROUND_COLOR,apps/desktop/src/renderer/providers/PostHogProvider/PostHogProvider.tsx (1)
14-29: Consider rendering children without PHProvider on initialization failure.When
initPostHog()throws, the code logs the error but still wraps children in<PHProvider client={posthog}>whereposthogcould benull(based onapps/desktop/src/main/lib/analytics/index.ts). This may cause downstream issues when components try to use PostHog hooks.Consider tracking initialization success separately and conditionally wrapping with
PHProvider:Proposed fix
export function PostHogProvider({ children }: PostHogProviderProps) { const [isInitialized, setIsInitialized] = useState(false); + const [initSuccess, setInitSuccess] = useState(false); useEffect(() => { try { initPostHog(); posthog.capture("desktop_opened"); + setInitSuccess(true); } catch (error) { console.error("[posthog] Failed to initialize:", error); } finally { setIsInitialized(true); } }, []); if (!isInitialized) { return null; } - return <PHProvider client={posthog}>{children}</PHProvider>; + if (!initSuccess || !posthog) { + return <>{children}</>; + } + + return <PHProvider client={posthog}>{children}</PHProvider>; }apps/desktop/src/renderer/components/BootErrorBoundary/BootErrorBoundary.tsx (1)
24-27: Consider capturingErrorInfofor component stack traces.React's
componentDidCatchreceives a second parameterErrorInfocontaining the component stack, which is valuable for debugging. Consider including it in logging and the callback.Proposed enhancement
- componentDidCatch(error: Error): void { - console.error("[renderer] Boot error boundary caught:", error); - this.props.onError?.(error); + componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void { + console.error("[renderer] Boot error boundary caught:", error, errorInfo.componentStack); + this.props.onError?.(error); }apps/desktop/src/renderer/index.tsx (1)
19-75: Consider extracting shared error UI styles to reduce duplication.The inline styles in
renderBootErrorduplicate those inBootErrorBoundary.tsx. While acceptable for boot-time code that must work before React mounts, consider extracting shared constants if this pattern grows.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/index.tsx`:
- Around line 44-50: Current code directly uses window.ipcRenderer and
handleDeepLink in the renderer; instead create a deep-link-events.ts emitter
under src/main/lib (mirroring menu.ts emitter pattern), add a tRPC subscription
in the app router that forwards emitted deep-link events, then remove the
window.ipcRenderer usage and subscribe to that tRPC subscription from the
renderer via the trpc client and call handleDeepLink from the subscription
handler; reference the existing symbols handleDeepLink, window.ipcRenderer,
menu.ts pattern, and the new deep-link-events.ts and the app router subscription
when making these changes.
🧹 Nitpick comments (3)
apps/desktop/src/main/windows/main.ts (1)
64-75: Extract resize delta and delay into named constants.This keeps the repaint tuning explicit and consistent with the project's no–magic-number guideline.
As per coding guidelines "Avoid magic numbers by extracting them to named constants at module top".♻️ Proposed refactor
+const REPAINT_RESIZE_DELTA_PX = 1; +const REPAINT_RESTORE_DELAY_MS = 32; + const forceRepaint = (win: BrowserWindow) => { if (win.isDestroyed()) return; win.webContents.invalidate(); if (win.isMaximized() || win.isFullScreen()) return; const [width, height] = win.getSize(); - win.setSize(width + 1, height); + win.setSize(width + REPAINT_RESIZE_DELTA_PX, height); setTimeout(() => { if (!win.isDestroyed()) win.setSize(width, height); - }, 32); + }, REPAINT_RESTORE_DELAY_MS); };apps/desktop/src/renderer/lib/boot-errors.ts (2)
6-62: Consider extracting UI size values into named constants.
renderBootErrorhardcodes several sizing values (e.g., 100vh, 24px, 520px). Hoisting them to top-level constants keeps styling intent clear and reduces magic numbers.As per coding guidelines, avoid magic numbers by extracting them to named constants at module top.
64-78: Use params object and standardized log prefix for boot errors.
This keeps the API aligned with the params-object guideline and enforces the[domain/operation]log prefix.♻️ Proposed refactor
-const renderBootError = (message: string, error?: unknown) => { +const renderBootError = ({ + message, + error, +}: { + message: string; + error?: unknown; +}) => { if (bootErrorReported) return; bootErrorReported = true; @@ -export const reportBootError = (message: string, error?: unknown) => { - console.error("[renderer] Boot error:", message, error); +export const reportBootError = ({ + message, + error, +}: { + message: string; + error?: unknown; +}) => { + console.error("[renderer/boot] Boot error:", message, error); if (hasMounted) return; - renderBootError(message, error); + renderBootError({ message, error }); }; @@ - reportBootError(event.message || "Unhandled error", event.error); + reportBootError({ message: event.message || "Unhandled error", error: event.error }); @@ - reportBootError("Unhandled promise rejection", event.reason); + reportBootError({ message: "Unhandled promise rejection", error: event.reason });// apps/desktop/src/renderer/index.tsx (call sites) -reportBootError("Missing <app> root element"); +reportBootError({ message: "Missing <app> root element" }); -reportBootError("Renderer preload not available (window.ipcRenderer missing)"); +reportBootError({ message: "Renderer preload not available (window.ipcRenderer missing)" }); -onError={(error) => reportBootError("Render failed", error)} +onError={(error) => reportBootError({ message: "Render failed", error })}As per coding guidelines, functions with 2+ parameters should accept a single params object with named properties instead of positional arguments, and use prefixed console logging with pattern
[domain/operation] messagefor all logging.
| if (window.ipcRenderer?.on && window.ipcRenderer?.off) { | ||
| window.ipcRenderer.on("deep-link-navigate", handleDeepLink); | ||
| } else { | ||
| reportBootError( | ||
| "Renderer preload not available (window.ipcRenderer missing)", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file and its context
cat -n apps/desktop/src/renderer/index.tsx | head -60Repository: superset-sh/superset
Length of output: 2077
🏁 Script executed:
# Check for tRPC setup in the codebase
fd -t f "trpc" apps/desktop/src --type f -x basename {} \;Repository: superset-sh/superset
Length of output: 155
🏁 Script executed:
# Search for window.ipcRenderer usage in renderer
rg "window\.ipcRenderer" apps/desktop/src/renderer --type ts --type tsx -nRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Look for deep-link handling elsewhere
rg "deep-link" apps/desktop/src -nRepository: superset-sh/superset
Length of output: 452
🏁 Script executed:
# Check the shared IPC channels definition
cat -n apps/desktop/src/shared/ipc-channels.ts 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the electron-trpc.ts to see how tRPC is configured
cat -n apps/desktop/src/lib/trpc/electron-trpc.tsRepository: superset-sh/superset
Length of output: 138
🏁 Script executed:
# Check ElectronTRPCProvider
cat -n apps/desktop/src/renderer/providers/ElectronTRPCProvider.tsxRepository: superset-sh/superset
Length of output: 156
🏁 Script executed:
# Search for tRPC router definition
fd -t f "router\|.*procedure\|.*subscription" apps/desktop/src/lib/trpc -e ts -e tsxRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check main/lib to see IPC handlers
fd -t f ".*ipc" apps/desktop/src/main/lib -e tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there are other subscription patterns in the codebase
rg "\.subscription\(" apps/desktop/src/lib/trpc -A 3Repository: superset-sh/superset
Length of output: 3569
🏁 Script executed:
# List the trpc directory structure
find apps/desktop/src/lib/trpc -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: superset-sh/superset
Length of output: 1204
🏁 Script executed:
# Find ElectronTRPCProvider
find apps/desktop/src -name "*ElectronTRPC*" -o -name "*TRPC*Provider*"Repository: superset-sh/superset
Length of output: 202
🏁 Script executed:
# Check the main trpc index or router
cat -n apps/desktop/src/lib/trpc/index.ts 2>/dev/null || cat -n apps/desktop/src/lib/trpc/router.ts 2>/dev/null || ls -la apps/desktop/src/lib/trpc/Repository: superset-sh/superset
Length of output: 1642
🏁 Script executed:
# Look for where deep-link might be handled in main process
cat -n apps/desktop/src/main/index.ts | grep -A 5 -B 5 "deep-link"Repository: superset-sh/superset
Length of output: 446
🏁 Script executed:
# Check if there's a router definition for navigation or deep-link
rg "navigation\|deepLink\|deep.link" apps/desktop/src/lib/trpc -t ts -iRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the ElectronTRPCProvider to understand how tRPC subscriptions work
cat -n apps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxRepository: superset-sh/superset
Length of output: 1272
🏁 Script executed:
# Check the menu router to understand the subscription pattern for events
cat -n apps/desktop/src/lib/trpc/routers/menu.ts | head -80Repository: superset-sh/superset
Length of output: 1352
🏁 Script executed:
# Look for the routers index to see what's exported
cat -n apps/desktop/src/lib/trpc/routers/index.tsRepository: superset-sh/superset
Length of output: 2277
🏁 Script executed:
# Check the app router to see all available procedures
cat -n apps/desktop/src/lib/trpc/routers/app.ts 2>/dev/null || cat -n apps/desktop/src/lib/trpc/routers.tsRepository: superset-sh/superset
Length of output: 132
🏁 Script executed:
# Check if there's any deep-link related event emitter
find apps/desktop/src/main -name "*deep*" -o -name "*navigation*" -o -name "*events*"Repository: superset-sh/superset
Length of output: 148
🏁 Script executed:
# Check the main/lib directory structure
ls -la apps/desktop/src/main/lib/Repository: superset-sh/superset
Length of output: 2057
🏁 Script executed:
# Search for EventEmitter patterns in main/lib
rg "EventEmitter\|emitter\|\.on\(" apps/desktop/src/main/lib -t ts --max-count=5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the menu-events pattern to understand how events are exposed
cat -n apps/desktop/src/main/lib/menu-events.tsRepository: superset-sh/superset
Length of output: 541
Route deep-link navigation through a tRPC subscription instead of window.ipcRenderer.
Direct ipcRenderer access bypasses the tRPC IPC layer established throughout the codebase. Create a deep-link-events.ts emitter in src/main/lib/ and expose deep-link navigation as a tRPC subscription in the app router (following the pattern used in menu.ts), then consume it through the tRPC client in the renderer instead.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/index.tsx` around lines 44 - 50, Current code
directly uses window.ipcRenderer and handleDeepLink in the renderer; instead
create a deep-link-events.ts emitter under src/main/lib (mirroring menu.ts
emitter pattern), add a tRPC subscription in the app router that forwards
emitted deep-link events, then remove the window.ipcRenderer usage and subscribe
to that tRPC subscription from the renderer via the trpc client and call
handleDeepLink from the subscription handler; reference the existing symbols
handleDeepLink, window.ipcRenderer, menu.ts pattern, and the new
deep-link-events.ts and the app router subscription when making these changes.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Description
Related Issues
Type of Change
Testing
Not run (defensive-only changes).
Screenshots (if applicable)
N/A
Additional Notes
These changes only affect failure paths and should not alter healthy startup behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Stability
Style