Repository navigation
fix(desktop): stop blocking the renderer during topology polling - #19300
Conversation
github-actions
Bot
added
vouch:unvouched
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a focused desktop performance fix that moves recurring topology polling off synchronous renderer-blocking IPC while preserving startup behavior, failure handling, snapshot identity, and older-bridge compatibility. The implementation is localized, additive at the contract boundary, and covered by targeted tests. You can add or adjust custom eligibility rules. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 9 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description clearly explains the problem, implementation, compatibility behavior, verification, limitations, and contributors. However, it does not provide the required explicit maintainer approva… | Add a Scope and approval section that links the triaged issue or discussion and quotes or links the maintainer approval comment. If no prior approval was required, explain why this focused fix qualifies for the exemption. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely identifies the main change: preventing renderer blocking during desktop topology polling.
Full details: Description check
-
Explanation
The description clearly explains the problem, implementation, compatibility behavior, verification, limitations, and contributors. However, it does not provide the required explicit maintainer approval of the direction and scope, including an approval comment or a clear small-fix exemption.
Resolution
Add a Scope and approval section that links the triaged issue or discussion and quotes or links the maintainer approval comment. If no prior approval was required, explain why this focused fix qualifies for the exemption.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Thanks @daniyal-sh — I built on this in #19511. Your commit is cherry-picked unchanged as the first commit, and a second commit makes the desktop push topology changes into your preload cache, so the renderer no longer polls the main process at all (it also fixes the trace flooding in #19505). That made the async |
|
Taking over this one as maintainer for desktop-app-10. I will preserve @daniyal-sh’s commits, integrate main, minimize the behavior tests and review the recurring async topology path across preload, IPC and web. The separate push-topology proposal is outside this batch. Current-head natural-workload native verification will remain explicit; this does not include merging. |
…ology-sol61 # Conflicts: # apps/desktop/src/ipc/methods/window.ts # apps/web/src/connection/desktopLocal.ts
|
Current-head CI passed lint, build/typecheck, all eight server shards, the general test job and release smoke. Web tests passed 8,048/8,049; the single failure is in the unchanged ACP Registry browser test's “submits a typed query at once…” case at line 169. The test uses real time between filling the input and pressing Enter, while the unchanged component submits after a 300 ms debounce. If that debounce wins, Enter legitimately refreshes the already-submitted query and violates the test's no-refresh assumption. Neither the component nor this test differs from pinned main. I am rerunning the failed jobs once to distinguish this timing race from a persistent failure; no topology source change or unrelated test edit is justified by this result. The takeover's five focused files passed 45/45, and independent review also passed 87/87 with adjacent auth/bootstrap suites. Native Windows/WSL natural-workload verification remains a human follow-up as documented in the PR body. |
|
Reverting this in #22222 together with the other desktop fixes the takeover campaign merged yesterday. Nothing was wrong with the review of this change; it was merged on CI and code audits only, and after #20907 from the same batch broke existing installs the maintainer decided that desktop changes do not ship without being run on a real desktop build. To bring it back: open a fresh PR from the original commit, run it on a real build that already has user state on each platform it touches, and put the before/after proof in the body. |
…gdotgg#19300) Co-authored-by: Julius Marminge <julius0216@outlook.com> (cherry picked from commit 6bcbb95)
3 participants
Recurring desktop topology polls currently cross synchronous Electron IPC, so a slow main-process reply can stall typing and rendering. This keeps the synchronous startup bridge for compatibility, caches its initial snapshot and relaunch-only enabled setting, and refreshes topology through asynchronous IPC during polling.
The sync and async handlers share main's current bootstrap builder. Successful async replies replace the cached snapshot, including an empty topology; failed refreshes retain the last successful snapshot. Concurrent refreshes share one request. Older desktop bridges retain the existing synchronous fallback, and unchanged topology keeps its array identity.
Maintainer takeover of #19300, credit to @daniyal-sh for the original fix. Contributor commits are retained, main was merged, and this updates the original PR. Push-based topology is outside this change.
Verification
Current-head browser proof
The actual web app ran against isolated worktree state. A controlled bridge exercised the actual desktop-local reader and preload snapshot helper while its async topology request was held pending. All 23 trusted keystrokes appeared in the real composer before request completion; overlapping refreshes shared one invoke, cached reads used one startup sync read, and resolving an empty result cleared the topology. No message was submitted.
This is a controlled integration proof, not a native Electron or natural-workload latency benchmark. The bridge was private to the probe; it did not replace the web app's global desktop bridge or simulate a live WSL backend. The historical benchmark claims from the original body have been removed.
Recording of typing with the pending refresh · Probe observations
Human verification remaining
Packaged Electron cannot start on this Linux VM because
libcups.so.2is missing. Native Windows typing/FPS under natural workload, Electron contextBridge behavior, live WSL startup/removal and endpoint/token changes still need a desktop pass. macOS, Windows and native Linux Electron were not verified; iOS/Android were not run and have no changed client path. Web-only rendering was exercised as described above. Current-head CI passed lint, build/typecheck, all eight server shards, general tests, web tests, release smoke and the aggregate Check. One unchanged ACP Registry browser timing test failed on the first run; the failed-job retry passed without source changes.Implementation: GPT-6.1 Sol; tests: GPT-6 Astra; adversarial reviews: Claude Opus 5.5 and GPT-6 Sol. Codex and Claude Code harnesses in T3 Code.