Repository navigation
Conversation
|
@codex review Please check the isolation gate, Trunk's custom preload/initializer semantics, preservation of hashed and relative asset URLs, already-isolated/browser compatibility, and whether the transfer benchmark supports only the claims stated in the description. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Please review 604d7bb, especially fallback compatibility. Browser probing confirmed WebAssembly shared memory can exist without a global SharedArrayBuffer, and GPUI has a main-thread fallback. The loader now awaits bounded isolation setup rather than requiring crossOriginIsolated unconditionally. Existing failures/absent APIs/scripts continue startup, and stalled installation releases it after 10 seconds. Fifteen Node tests pass; repeated eight-pair transfer results remain 2 requests/1 cancellation for base versus 1/0 for head. No CPU/RAM/latency improvement claimed. |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Final follow-up 0faedd9 handles a browser-proven edge case: Chromium can restore the initial, still-waiting document from bfcache. A persisted pageshow now releases that document's loader to the existing main-thread fallback and clears its deadline. Sixteen Node tests pass. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Withdrawing this proposal after failure-mode qualification. With service-worker registration forced to remain pending, the same Chromium runner and synthetic module reached the initialization event in 1,936 ms for base versus 11,938 ms for head. The added 10-second startup wait is a material regression; the normal-case transfer saving does not justify it. Also tested a no-wait No CPU/RAM/latency improvement is being delivered. Closing rather than retaining a startup regression. The positive Codex review and passing loader tests do not supersede this measured failure-mode result; CI for the rejected heads remains queued. |
|
The withdrawal is now committed as 263ef87; the branch tree matches origin/main with no remaining application changes. Additional rejected web-memory hypothesis: moving the downloaded emoji font Vec into the font database would not remove an extra font-data copy. In the pinned gpui-pre-wgpu 0.3.4, Other excluded ideas: trimming mandatory font faces risks style/language coverage; deferring emoji registration beyond the first frame conflicts with the documented persistent line-layout cache; redoing preload service-worker bypass and theme-revision memoization would duplicate optimizations already on main. No independent large CPU/RAM optimization was established in this web-only investigation. |
Withdrawn after qualification: stalled service-worker registration delays the synthetic initialization event from 1,936 ms on base to 11,938 ms on head. This startup regression outweighs the normal-case transfer saving. A no-wait keepalive alternative did not establish a useful reduction. See the final investigation comment for before/after evidence. No merge or deployment.
A first visit starts preloading WASM while the isolation service worker installs, then abandons that request during the required reload. Keep Trunk's preloads in an inert template and defer the dynamic import while that reload is pending. Already isolated documents activate both preloads during parsing. Preserve Trunk's initializer API and application-started event.
Preserve GPUI's main-thread fallback when isolation is unavailable: the startup promise settles immediately for missing/failed service workers, insecure contexts, or an existing non-isolated controller. A missing worker script also retains the original startup path. A stalled installation releases startup after 10 seconds; a late worker takeover can still cause a reload. That ceiling is an explicit trade-off for avoiding a download during setup.
Measured with Chromium 151 on one Linux runner, eight alternating base/head pairs and fresh scopes, the real service worker plus a synthetic 22 MiB WASM custom section served in 64 KiB chunks every 5 ms: base made 2 WASM requests and cancelled 1 in all pairs; head made 1 and cancelled 0. Final-head median avoided server-written payload was 1,474,560 bytes. Both completed in every run. With isolation headers supplied by the host, both variants made 1 request and wrote identical payload in all eight pairs. No latency improvement demonstrated; these fixtures measure avoided transfer, not application CPU, heap/RSS, or production startup latency. The amount saved depends on worker installation and network speed.
Reproduce with
node web/startup-transfer.mjsandISOLATED=1 node web/startup-transfer.mjsusing Chromium on PATH.CHECK_FLOWS=1 node web/startup-transfer.mjschecks first load, reload/bfcache and an already-loaded document with networking disabled; all pass for base/head. It does not exercise full client offline storage/session behavior or claim offline installation. No application dependencies added.Validation: 16 Node service-worker/startup tests pass, covering absent APIs/scripts, registration failure, stalled installation, existing controllers, bfcache restoration and the one-reload bound; all 50 xtask tests pass; TOML parses and diff check passes. Full web bundle and platform checks run in CI. Draft keeps preview publication off while validating. Independent of #265 and the daemon/UI/CLI/media/SQLite fronts.