test (desktop): added logs for release testing on white screening by AviPeltz · Pull Request #763 · superset-sh/superset

AviPeltz · 2026-01-15T01:15:24Z

Description

Related Issues

Type of Change

Testing

Screenshots (if applicable)

Additional Notes

Summary by CodeRabbit

Bug Fixes
- Enhanced error handling and surfaced load failures in both development and production launch paths.
- Broadened filename handling for cross-platform URL resolution to reduce load errors.
- Added detailed logging around protocol handling and renderer lifecycle (load success/failure, crashes, preload issues) and ensured windows are shown when renderer errors occur.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-15T01:15:36Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Added diagnostic logging and explicit error handling for window loading, custom protocol requests, and renderer lifecycle events; made production filename extraction cross-platform.

Changes

Cohort / File(s)	Summary
Window Loader `apps/desktop/src/lib/window-loader.ts`	Wrapped `loadURL()` calls with `catch` handlers; added console logs for dev and prod paths; replaced brittle filename split with cross-platform extraction; logged produced URLs and attempted HTML file paths.
Protocol Registration & Request Handling `apps/desktop/src/main/index.ts`	Added diagnostic logs around custom protocol registration and per-request handling in non-dev mode (protocol scheme, __dirname, renderer path, request URL, pathname, resolved file path). Core handler logic unchanged.
Renderer Lifecycle Monitoring `apps/desktop/src/main/windows/main.ts`	Added listeners and logs for `did-finish-load`, `did-fail-load`, `render-process-gone`, and `preload-error`; ensure window visibility on load failure and surface error details (codes, descriptions, URLs, preload path).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  Participant AppMain as Main Process
  Participant Protocol as Custom Protocol Handler
  Participant FS as File System
  Participant BrowserWin as BrowserWindow (Renderer)

  AppMain->>Protocol: register protocol (non-dev)
  Note right of Protocol: logs protocol scheme, paths
  BrowserWin->>AppMain: request URL (via protocol)
  Protocol->>FS: resolve pathname -> file path
  Protocol-->>AppMain: return file path (served)
  AppMain->>BrowserWin: loadURL(resolved URL)
  BrowserWin-->>AppMain: did-finish-load / did-fail-load / render-process-gone / preload-error
  Note left of AppMain: logs details and ensures window shown on failures

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped through code, found paths astray,
I logged each step to light the way,
When loads may fail or renderers sigh,
I cheerfully note the where and why,
Debug carrots left—no bug shall stay.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is largely empty with only the template structure and 'Bug fix' checkbox marked. Required sections like Description, Related Issues, and Testing lack substantive content.	Add a detailed description of the changes, link any related issues, and describe testing performed to verify the white screen logging improvements.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix (desktop): added logs for release testing on white screening' directly describes the main change—adding logging for debugging white screen issues in the desktop app during release testing.

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

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e41bf4 and 2632556.

📒 Files selected for processing (1)

apps/desktop/src/lib/window-loader.ts

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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.}

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@apps/desktop/src/lib/window-loader.ts`:
- Line 1: Remove the Node.js path import and replace the use of path.basename in
window-loader.ts: instead of importing "node:path" and calling
path.basename(props.htmlFile), compute the file name from props.htmlFile using a
split on both slashes (e.g., split on /[/\\]/ and take the last element) and
fallback to "index.html" if empty; update any references to the old variable
(e.g., fileName) accordingly and delete the import line.

🧹 Nitpick comments (1)

apps/desktop/src/main/index.ts (1)
229-231: Consider removing or reducing verbosity of per-request logging.

Logging every protocol request may generate excessive noise in production. These logs are helpful for debugging but could clutter logs during normal operation, especially if resources like images or fonts are loaded repeatedly.

Consider either:

Removing these per-request logs before release (since they're for "release testing" per PR title)

Gating them behind a debug flag or environment variable
💡 Optional: Gate verbose logging
+const DEBUG_PROTOCOL = process.env.DEBUG_PROTOCOL === "true";
+
 protocol.handle(PROTOCOL_SCHEME, (request) => {
   const parsedUrl = new URL(request.url);
   const pathname = parsedUrl.pathname;
   const filePath = path.normalize(
     path.join(__dirname, "../renderer", pathname),
   );
-  console.log(`[protocol] Request: ${request.url}`);
-  console.log(`[protocol] Pathname: ${pathname}`);
-  console.log(`[protocol] Resolved file path: ${filePath}`);
+  if (DEBUG_PROTOCOL) {
+    console.log(`[protocol] Request: ${request.url}`);
+    console.log(`[protocol] Pathname: ${pathname}`);
+    console.log(`[protocol] Resolved file path: ${filePath}`);
+  }
   return net.fetch(`file://${filePath}`);
 });

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cfd3b06 and 4e41bf4.

📒 Files selected for processing (3)

apps/desktop/src/lib/window-loader.ts
apps/desktop/src/main/index.ts
apps/desktop/src/main/windows/main.ts

🧰 Additional context used

📓 Path-based instructions (6)

apps/desktop/**/*.{ts,tsx}

📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)

apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc
Use alias as defined in tsconfig.json when possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from @trpc/server/observable instead of async generators, as the library explicitly checks isObservable(result) and throws an error otherwise

Files:

apps/desktop/src/main/windows/main.ts
apps/desktop/src/lib/window-loader.ts
apps/desktop/src/main/index.ts

**/*.{ts,tsx}