fix: recover wedged webview boot instead of dying on injected script failure - #3436
fix: recover wedged webview boot instead of dying on injected script failure#3436jeanfbrito merged 4 commits into
Conversation
Sorry, something went wrong.
…failure
When the Meteor module registry is incomplete ('Cannot find module
/app/utils/rocketchat.info'), the injected script used to throw an uncaught
rejection and give up, leaving the workspace stuck on the loading throbber
until a full app restart — a plain reload never recovers this state.
- On require failure after retries, trigger reloadServer() (force reload
with service worker + cache storage clearing)
- Cap automatic recoveries at 2 per session via a sessionStorage counter
(reset on successful boot); the pre-existing window.require recovery
path previously reloaded without any limit and now shares the same guard
- Catch rejections from start() and its setTimeout retries so failures are
logged instead of surfacing as unhandled rejections in Bugsnag
|
Warning Review limit reached@jeanfbrito, you've reached your PR review limit, so we couldn't start this review. Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 357ab1fe-5000-419c-9a21-9c62c024c9ef 📥 CommitsReviewing files that changed from the base of the PR and between 090de61 and 21699c8. 📒 Files selected for processing (4)
WalkthroughChangesBoot recovery and watchdog diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: type: bug Sequence Diagram(s)sequenceDiagram
participant MainStartup
participant ServerWebview
participant Startup
participant ServerInfo
participant BootWatchdog
participant DiagnosticReport
MainStartup->>BootWatchdog: setupBootWatchdog()
ServerWebview->>BootWatchdog: attachBootWatchdog()
Startup->>ServerInfo: load server-info module
alt module loading fails
Startup->>BootWatchdog: record recovery event
Startup->>Startup: attemptBootRecovery()
else server info loads
ServerInfo->>BootWatchdog: setVersion(serverVersion)
BootWatchdog->>ServerWebview: clear boot deadline
end
BootWatchdog->>DiagnosticReport: append failure diagnostics when required
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@src/injected.ts`: - Around line 59-66: Update the recovery flow around the sessionStorage write so automatic recovery is attempted only when incrementing and storing the counter succeeds; when setItem throws, skip reloadServer() rather than proceeding with an untracked attempt. Preserve the existing bounded-attempt behavior when storage is available. - Around line 145-150: The server-info handling in injected.ts returns early when serverInfo.version is missing, which leaves the webview stuck after tryRequire() resolves an incomplete export. Update this branch to call attemptBootRecovery() before returning so recovery is retried, and keep resetBootRecovery() only on the path where serverInfo.version is present and valid.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29f062a7-f6af-450f-a300-15dbfc00f55e
📥 CommitsReviewing files that changed from the base of the PR and between 866a9b4 and 02997be.
📒 Files selected for processing (1)- src/injected.ts
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: Analyze (javascript)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from @rocket.chat/fuselage for UI work unless the design requires something Fuselage does not provide.
Check Theme.d.ts for valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local .d.ts files instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from @rocket.chat/fuselage.
Use only valid color tokens documented by Theme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such as process.getuid(), getgid(), geteuid(), and getegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and .d.ts files instead of assuming they are valid.
Files:
- src/injected.ts
src/injected.ts (1)96-123: LGTM!
Also applies to: 126-143, 764-766
Sorry, something went wrong.
Linux installer download |
Sorry, something went wrong.
…views Every wedge occurrence now produces a self-contained forensic report in <logs>/boot-watchdog.jsonl instead of requiring a live autopsy. Enabled in development, opt-in elsewhere via ROCKETCHAT_BOOT_WATCHDOG=true. - Track each server webview boot cycle: a committed main-frame navigation (did-navigate) arms a 90s deadline cleared by the WEBVIEW_SERVER_VERSION_UPDATED signal; did-start-loading is recorded but does not reset the cycle (it fires for subframe/resource loads too) - On deadline exceeded, render-process-gone, unresponsive, or injected recovery exhaustion, append a JSONL report with an in-page probe (require/module/sessionStorage/service-worker state), running service workers, process metrics, event timeline, and the last 150 console messages - Wire the orphaned setVersion preload call into setServerInfo so WEBVIEW_SERVER_VERSION_UPDATED actually fires on webapp boot, as the reducer comment already assumed
Update: dev boot watchdog added (1e098ec)Every wedge occurrence now produces a self-contained forensic report — no more depending on catching it live with DevTools open. How it works
Validated by dogfooding: the watchdog's first-ever report diagnosed a false positive in itself (did-start-loading fires for subframe loads and was resetting the boot cycle after the version signal) — fixed by keying the cycle on did-navigate. After the fix: 150s uptime with multiple servers, zero false reports, healthy boots confirmed via CDP. Next: when the wedge reproduces naturally, the JSONL report should pin whether the broken state lives in the service worker, the module registry, or the renderer process — which decides if the escalation needs to be webview recreation. |
Sorry, something went wrong.
…tion Agent/dev workflow: read boot-watchdog.jsonl reports first, run live CDP autopsies on wedged webviews (cdp-eval.mjs helper included), walk the manual cure ladder to localize the broken layer, and map findings to the next fix on this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@src/main.ts`: - Line 163: Move the setupBootWatchdog() call to immediately after createMainReduxStore() so the version listener is registered before attachGuestWebContentsEvents(), the root window, and awaited setupSpellChecking() can allow webview boot events. Remove the later invocation to ensure the watchdog is initialized only once.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dded84fe-e543-4779-b5db-a27fef4558ed
📥 CommitsReviewing files that changed from the base of the PR and between 02997be and 1e098ec.
📒 Files selected for processing (4)- src/main.ts
- src/servers/bootWatchdog.ts
- src/servers/preload/api.ts
- src/ui/main/serverView/index.ts
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from @rocket.chat/fuselage for UI work unless the design requires something Fuselage does not provide.
Check Theme.d.ts for valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local .d.ts files instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; create custom components only when Fuselage lacks the required functionality.
Import Fuselage components from @rocket.chat/fuselage.
Use only valid color tokens documented by Theme.d.ts.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs such as process.getuid(), getgid(), geteuid(), and getegid().
Use TypeScript strict mode.
Redux actions must follow the Flux Standard Action pattern.
Use camelCase for file names and PascalCase for component names.
Avoid unnecessary comments; prefer self-documenting code through clear naming.
Do not commit or push without explicit user permission.
Verify library APIs, props, tokens, and types against official documentation and .d.ts files instead of assuming they are valid.
Files:
- src/ui/main/serverView/index.ts
- src/servers/preload/api.ts
- src/main.ts
- src/servers/bootWatchdog.ts
src/servers/bootWatchdog.ts (1)src/ui/main/serverView/index.ts (1)1-327: LGTM!
src/servers/preload/api.ts (1)24-24: LGTM!
Also applies to: 268-268
45-45: LGTM!
Also applies to: 87-87
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In @.claude/skills/boot-wedge-debug/cdp-eval.mjs: - Around line 17-19: Update the target selection logic around targets.find to reject ambiguous webview matches: collect all webviews whose URLs match urlPart and continue only when exactly one matches, otherwise fail with a clear error; alternatively, normalize and require an exact origin match before selecting the target. - Around line 14-16: Update target discovery in cdp-eval.mjs to enforce an abort timeout around the /json fetch, validate response.ok before parsing, and handle fetch or JSON failures as controlled non-zero exits. Ensure discovery cannot block before the existing CDP evaluation timeout begins, while preserving normal target selection for valid responses. - Around line 28-52: Update the CDP evaluation promise around the WebSocket lifecycle so `ws.close()` runs in a `finally` block for success, timeout, and error paths. Add an `ws.onclose` handler that rejects if the promise is still pending, and catch synchronous `ws.send` and `JSON.parse` failures to reject promptly while clearing the timer and preventing duplicate settlement. In @.claude/skills/boot-wedge-debug/SKILL.md: - Around line 27-33: Update the dev-only boot watchdog documentation in SKILL.md to identify the boot-watchdog.jsonl location for every supported platform, including Windows and Linux, or document the runtime <logs> directory as the portable location. Preserve the existing macOS path and ensure developers can locate the file from this workflow.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0035bcd0-ab51-4ffa-af04-1076237b4e95
📥 CommitsReviewing files that changed from the base of the PR and between 1e098ec and 090de61.
📒 Files selected for processing (2)- .claude/skills/boot-wedge-debug/SKILL.md
- .claude/skills/boot-wedge-debug/cdp-eval.mjs
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (macos-latest, mac)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.5.1) .claude/skills/boot-wedge-debug/SKILL.md[warning] 65: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 103: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[error] 81: [SC2] External Script Fetching: Remote code is downloaded and executed. This bypasses code review and could introduce malicious code.
Remediation: Avoid downloading and executing remote scripts. Use trusted packages from PyPI/npm. If remote fetch is required, verify checksums and use HTTPS.
(Supply Chain (SC2))
🔇 Additional comments (4).claude/skills/boot-wedge-debug/SKILL.md (2).claude/skills/boot-wedge-debug/cdp-eval.mjs (2)1-4: LGTM!
Also applies to: 6-23, 35-60, 68-87, 89-102, 104-120
65-65: 🔒 Security & Privacy
Other (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: Internal
Prevent npx from resolving unpinned packages.
When the local electron or typescript binary is missing, these commands can download and execute a package selected by the registry. A compromised or newly published package would run with developer permissions. Use npx --no-install, the repository package-manager executable, or an exact version. Verify that the install contract always provides the local binaries.
Also applies to: 103-103
14-16: 🎯 Functional Correctness
Verify the Node runtime for the built-in APIs.
This utility runs with node and uses global fetch and global WebSocket. If the repository's declared Node target does not provide either API, the documented command fails before it can inspect a webview. Confirm the declared toolchain and run this file with that pinned runtime, or add a supported client implementation.
Also applies to: 28-28
1-12: LGTM!
Also applies to: 20-27, 53-53
Sorry, something went wrong.
- injected.ts: abort automatic recovery when the sessionStorage counter cannot be persisted (an unpersisted counter would allow an unlimited reload loop), and trigger recovery when the server-info module resolves without a version instead of silently leaving the view wedged - main.ts: register the boot watchdog right after the store is created so WEBVIEW_SERVER_VERSION_UPDATED cannot be dispatched before the listener exists (would cause false boot-deadline-exceeded reports) - cdp-eval.mjs: reject ambiguous webview matches, add fetch timeout and response.ok check, settle the WebSocket promise exactly once and close the socket in a finally block - SKILL.md: document the watchdog log path for macOS, Windows and Linux
Windows installer download |
Sorry, something went wrong.
|
Applied all CodeRabbit findings in 21699c8 — all three Majors were valid:
Full suite green (155 suites, 1662 tests) and build clean. |
Sorry, something went wrong.
macOS installer download |
Sorry, something went wrong.
Symptom
Intermittently, a workspace tab gets stuck on the loading throbber forever. Its webview console shows:
A plain reload (⌘R = webContents.reload()) never recovers it; only fully restarting the app does. The wedge is intermittent and so far not deterministically reproducible (kill-during-boot and instance-race attempts all booted healthy).
What was happening
injected.ts requires /app/utils/rocketchat.info with 5 retries (~31s). When the Meteor module registry is incomplete (wedged webview state), the retries exhaust and the error escapes as an uncaught rejection — the entire injected script dies with no recovery path, and everything after that point (setServerInfo, Notification override, unread badge listeners, presence, Outlook) never initializes.
Meanwhile the app already ships the right remedy — WEBVIEW_FORCE_RELOAD_WITH_CACHE_CLEAR (clears service workers + cache storage + cache, then reloadIgnoringCache) — but it was only wired to one failure mode (window.require never appearing).
Changes (src/injected.ts only)
Live validation
Observed once in dev with the fix applied: the wedge reproduced on a real workspace and the new path fired Triggering force reload with cache clear to recover (attempt 1 of 2) instead of dying silently. In that single observation the cache-clearing reload did not cure the wedge — see below.
Known limitation / next steps (PR will be updated as the investigation progresses)
Validation
Summary by CodeRabbit