feat(shell): C3 — device facts (dppx + prefers-color-scheme) shell→content delivery#415
Merged
Conversation
…s.rs (touch-time split) `app/mod.rs` is at the 1000-line guideline; the C3 device-facts work would push it over. `drain_content_messages` (~315 lines) is a self-contained ContentToBrowser dispatcher — the browser-thread counterpart to the OS-event dispatcher already extracted to `threaded.rs`, and a real cohesion seam beyond the viewport cluster (#407). Extract it verbatim (no behavior change) into its own module so the seam is reviewable on its own, ahead of the C3 feature PR (touch-time discipline — split is a standalone PR, not bundled into the feature diff). mod.rs: 999 → 676. The per-tab drain cap (MAX_DRAIN_PER_TAB) moves with its sole user. No public surface change (drain_content_messages stays pub(super), called from handle_redraw_threaded). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… plan-reviewed PR-C sub-slice C3. Delivers window.devicePixelRatio + @media(resolution) (dppx) and @media(prefers-color-scheme) as live content facts through the one device-fact chokepoint, and CLOSES the carved F3 slot #11-shell-viewport-scalefactorchanged-x11- coverage (the dppx producer = the same "detect every DPI change and propagate" mechanism). Key decisions (D1-D6): F3 subsumption / redraw-top single steady-state chokepoint with atomic placement+seq (D2) / ViewportCell → device-state cell, seq stays size-only (D3) / route boa @media through canonical elidex-css evaluator, retire the prefers-color-scheme=>false stub (D4, route-canonical-now not defer-to-S5) / unified SetDeviceFacts{color_scheme,dppx} (D5). /elidex-plan-review: 5-agent (0 CRIT / 6 IMP / 4 MIN) → all resolved; D1+D4 (the central bets) adversarially verified CLEAN. Focused F1 re-check caught the redraw-top reclip relocation edges (zero-size deferral + reclip-gate origin axis) → resolved (full placement!=cached gate; benign 0x0 deferral). Not yet implemented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntent delivery Deliver the two DPI/theme content facts — window.devicePixelRatio / @media (resolution) and @media (prefers-color-scheme) — from the shell to every content thread, by construction + live, through one device-state chokepoint. Producer (shell): - ViewportCell { size, seq } → { size, seq, facts: DeviceFacts }; publish_if_changed → publish_device_state(size, facts) -> DeviceDelta { size_changed, facts_changed }. Facts are seq-orthogonal (D3): a pure-scale change the OS absorbs ships SetDeviceFacts without bumping seq (no phantom input-drop). - Redraw-top is the single steady-state chokepoint (D2): recompute placement + device facts, publish, broadcast (size→SetViewport+seq, facts→SetDeviceFacts), reclip on full placement!=prev — all atomic, so placement↔seq stay coherent. - Resized arm reduced to gpu.resize + request_redraw; NEW ScaleFactorChanged arm (closes the X11-only DPI gap #11-shell-viewport-scalefactorchanged-x11-coverage, the F3 slot) + ThemeChanged arm; resumed seeds facts before spawn. Construction-input (C1 parity): DeviceFacts thread through the pipeline builders → run_scripts_and_finalize seeds the bridge dppx/color-scheme before initial scripts, so a tab on a HiDPI/dark display is born with the right devicePixelRatio/matchMedia. Consumer (boa, deleted-with-boa at S5): - evaluate_media_query_raw routed through canonical elidex_css::media::evaluate (D4) over a MediaEnvironment built by the single HostBridge::media_environment; retires the boa-local table + the prefers-color-scheme => false stub. - SetDeviceFacts arm: value-guarded set_device_pixel_ratio + set_color_scheme → re_evaluate_media_queries → dispatch change → re_render. IPC: BrowserToContent::SetDeviceFacts { color_scheme, dppx } (unified, no seq, D5). Scope: CSS-cascade <style> @media (resolution|prefers-color-scheme) stays carved (#11-media-prefers-features / #11-media-css-values-fidelity, engine-independent elidex-style); C3 lights the JS-observable surface (devicePixelRatio + matchMedia/MQL). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db194df6ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d4ec496d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…deferred) Codex round-1 on PR #415 (C3 device facts). 3 IMPORTANT fixed, 1 deferred to a genuine cross-PR layer boundary. F4 (ipc.rs:306) — stale SetDeviceFacts replay. SetDeviceFacts carried no generation, so a navigation racing rapid DPI/theme changes — whose build seeds the bridge from the cell's *latest* facts — would replay an older queued SetDeviceFacts backward (and fire a spurious MQL `change`), the facts analog of the SetViewport stale-replay the `seq` guard prevents. Add a `facts_seq` generation to ViewportCell/ViewportSnapshot/SetDeviceFacts (orthogonal to the size `seq`, D3) + an `applied_facts_seq` high-water mark, mirroring SetViewport's staleness guard exactly. Regression test: a stale (older facts_seq) delivery with *different* facts is dropped (value-guard alone cannot catch it), a newer one applies. F2 (app/mod.rs:678) — DPI/theme events not forwarded to egui. C3's new ScaleFactorChanged/ThemeChanged early-return arms exit before the per-mode dispatch where egui's on_window_event runs, so egui-winit's cached native_pixels_per_point/system_theme went stale → the browser chrome kept rendering at the old DPI scale / theme. Add a single `forward_to_egui` helper, called by all three window-state-and-return arms (Resized too, for consistency); non-input events, so it forwards on render_state without disturbing input-routing gating. F3 (iframe/load.rs:184) — iframe device facts defaulted. Device facts are window/display facts (the parent bridge holds the real dppx/color-scheme at iframe build), not box facts (genuinely unknown → #11-iframe-build-viewport). Bundling them with the box-viewport deferral was a category error: a sub-frame on a HiDPI/dark display reported 1×/Light. Thread the parent's facts through IframeLoadContext to every build site (srcdoc/same-origin/OOP); the *size* stays DEFAULT (still deferred). Regression test: an in-process iframe inherits the parent's dppx/color-scheme. Deferred — F1 (event_loop.rs:394) CSS-cascade `<style>@media` rendering. C3 wires the JS-observable surface (matchMedia/MQL via boa→canonical, devicePixelRatio getter); the cascade path (resolve_styles_with_compat) feeds the non-viewport device facts at MediaEnvironment::default() (elidex-style/lib.rs:196-198). Wiring them is a cross-cutting signature change in the engine-independent elidex-style layer — carved to #11-media-css-values-fidelity / #11-media-prefers-features (user Option-B decision). Plan-memo §7 now states this carve explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Codex R1 — disposition (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7555874b7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…-drain (+2 carved) Codex round-2 on PR #415. 2 IMPORTANT fixed in C3, 1 plan-reviewed-tradeoff doc-corrected, 2 deferred to genuine cross-PR boundaries (design lens → "fix C3's own delivery bugs, carve the genuinely-new edge-dense mechanisms"). F3 (size+facts non-atomic) — when one redraw changed BOTH the logical size and the device facts (a DPI move altering the logical size), the producer broadcast SetViewport then SetDeviceFacts as two messages, so the content re-evaluated an inconsistent new-size + old-facts intermediate, firing a spurious/wrong MQL `change` for a query like `(min-width: 800px) and (prefers-color-scheme: dark)`. Fix: SetViewport now carries the settled facts (the cell snapshot is atomic), so a both-changed frame delivers in ONE message → ONE content re-eval reflecting both, never an intermediate. SetDeviceFacts is sent only for a facts-only frame (size unchanged) — mutually exclusive, facts never double-send. New shared `apply_device_facts` helper (the facts_seq staleness + value guards) used by both the SetViewport and SetDeviceFacts arms. Regression test asserts a both-changed delivery whose final match equals its initial fires ZERO MQL `change` events (the buggy two-message path fired twice through a spurious intermediate); phase 2 confirms the query is live. F4a (drain-before-publish) — C3's D2 moved the redraw-top publish to *after* `drain_content_messages`, so a `window.open()` spawn drained in a frame following a resize/DPI change read the stale pre-resize cell as its construction input. Fix: publish the settled placement + facts BEFORE the drain (the drain's spawns clone the freshly-published cell). C3-introduced regression (C2 published synchronously in the Resized arm). F4b (gap-input) — Codex correctly flagged that gap input (mouse between a resize event and its redraw) is *applied* against the old layout, not dropped/superseded as D2/E5 prose claimed (the FIFO orders it before the new seq; placement_seq == applied, not <). The ≤1-frame tradeoff stands (plan-reviewed; reverting to C2's synchronous recompute reintroduces the F1 partial-move hazard) — only the inaccurate "superseded" prose is corrected in the plan-memo (D2 (1), §2 matrix, E5). Deferred (genuine cross-PR boundaries, plan-memo §8 + open-defer-slots): - F1 CSS-cascade `<style>@media` rendering → re-deferred `#11-media-css-values-fidelity` (cost-confirmed: ~9 callers in the engine-independent elidex-style crate). - F2 iframe LIVE device-fact propagation → NEW `#11-iframe-device-fact-live-propagation` (edge-dense new OOP IPC + in-process iteration; build-seed R1 covers the common case; distinct from #11-iframe-build-viewport — device facts ≠ box facts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cumulative /elidex-review (5-agent) on the R1+R2 delta returned 0 CRIT / 0 IMP (fixes structurally sound) + 3 MIN, all resolved here (comment/doc only): - app/mod.rs Resized-arm comment: "gap-input ... superseded together" → "applied against the still-old layout (placement_seq == applied_viewport_seq, not dropped/ superseded)". The R2 correction reached the plan-memo (D2/E5/§2-matrix) but had been left in this code comment. - viewport_tests.rs docstring: MQL `change` event cite §4.2 → §4.2.1 (the precise CSSOM-View section the plan-memo §3/§10 already use; §4.2 is only the containing interface section). - viewport_tests.rs header: the file crossed 1000 lines (681→1071) via the C3 device-fact tests; the stale "under the 1000-line guideline" claim is replaced with the cohesion-exemption rationale (one ViewportCell→content delivery unit; the atomic-delivery test spans the size/facts seam, so no clean split; a future split would be a standalone prereq commit, not bundled here). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Codex R2 — disposition (commits
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbacbb31e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…awn root fix Codex round-3 on PR #415. 2 IMPORTANT, both fixed (one a ≥2-round root fix). A (dppx f32 precision) — `window.devicePixelRatio` and `@media (resolution)` were fed an f32 dppx, rounding a fractional scale like 1.2 to 1.2000000476837158, so JS observed a wrong DPR (CSSOM View says devicePixelRatio is a double). Carry the scale as lossless f64 end-to-end: ContentAreaPlacement.scale_factor (the D6 single source) → DeviceFacts.dppx → SetViewport/SetDeviceFacts → the bridge → the devicePixelRatio getter. The compositor transform / input ÷scale tolerate f32, so the narrowing is centralized in one `ContentAreaPlacement::scale_f32()` helper (origin_phys/size_phys/ContentPlacement/scroll); the input mapper (cursor_to_content) now divides by the f64 directly (a more precise inverse). Re-examined the @media half of the finding: with cssparser tokenizing a `<dimension>` value as f32 (parse.rs → `Dppx(f64::from(f32))`) and `RangeValue::Dppx` comparing EXACTLY, an f64 device 1.2 would never equal the f32-sourced query 1.2 — the f32 device coincidentally matched, and a naive f64 device would have *regressed* `(resolution: 1.2dppx)`. So the media_environment narrows the device dppx through f32 to share the cssparser precision (Codex's "normalize before feeding MediaEnvironment" alternative) — devicePixelRatio stays exact f64, the media comparison stays consistent. Regression test: a delivered 1.2 dppx → devicePixelRatio === 1.2 AND (resolution: 1.2dppx) matches. B (publish before browser-side tab spawns) — ≥2-round root fix. R2's #4a publish- before-drain covered window.open()-via-drain (drain runs after the redraw-top publish); Codex R3 found the sibling entry: a browser-side command (Ctrl+T via the keyboard handler, in the event→redraw gap) calls open_new_tab, which clones the viewport_cell for the spawned content thread while the resize/DPI arm has deferred the publish to redraw — so the new tab is born at the stale pre-resize innerWidth/devicePixelRatio. Root: a construction-input spawn can read a stale cell because the publish is deferred (D2). Fix: extract the redraw-top recompute+publish into `refresh_viewport_cell` (idempotent, placement+seq atomic) and call it at BOTH chokepoints — redraw-top (before the drain) and open_new_tab (before cloning the cell) — so the cell holds the settled state before any spawn reads it. Subsumes both entries by construction. (Browser-thread spawn isn't headlessly testable — no winit window in the content-test harness; structural-by-construction + manual smoke, per plan-memo §6's stance on window-level behavior.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o elidex_css evaluator
The cumulative /elidex-review (5-agent) on the R-loop delta flagged 1 IMP (Axis 1
Layering + Axis 3): R3's media-comparison precision-alignment was placed in the boa
producer (`bridge/media.rs` fed `resolution_dppx: f64::from(device_pixel_ratio() as
f32)`), but it encodes engine-independent CSS-comparison knowledge (the device dppx
must share the cssparser-f32 query precision for the exact `RangeValue::Dppx`
compare). Placing it at one producer (a) diverged from the VM producer
(`vm/host/media_query.rs` feeds f64, no narrowing) — so the VM, the S5-FLIP survivor,
would re-introduce the `(resolution: 1.2dppx)` mismatch — and (b) left the
`MediaEnvironment { resolution_dppx }` contract with an undocumented "must be
f32-quantized" precondition.
Fix: own the alignment in the engine-independent evaluator —
`elidex_css::media::eval::range_feature_value` narrows the device `resolution_dppx`
to f32 for the Resolution feature, so every producer (boa bridge + VM host) feeds the
lossless f64 and the evaluator aligns it to the query precision once. boa
`media_environment` reverts to feeding the f64 directly. `Converted` (dpi/dpcm) is
unaffected (its `VALUE_REL_EPS` tolerance absorbs the ≤6e-8 f32 narrowing); integer
dppx (exact in f32) is unchanged. elidex-css 462 tests + the fractional_dppx
regression test pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Codex R3 — disposition (commits
|
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77273d396d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Owner
Author
|
Codex R4 — both findings are re-raises of already-carved scope boundaries (no new findings; re-examined, carves hold).
All in-scope delivery bugs found across R1–R3 (facts_seq staleness, egui DPI/theme, iframe build-seed, atomic size+facts delivery, drain ordering, dppx f64 precision, spawn-cell freshness) are fixed. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR-C sub-slice C3 (
#11-shell-viewport-delivery, after C1 #396 / C2 #411). Stacked on #414 (thedrain_content_messagestouch-time split) — review/merge #414 first; this PR's base retargets tomainon #414 merge.Delivers the two DPI/theme content facts —
window.devicePixelRatio/@media (resolution)(dppx) and@media (prefers-color-scheme)— from the shell to every content thread, by construction + live, through one device-state chokepoint.Producer (shell)
ViewportCell { size, seq }→{ size, seq, facts: DeviceFacts };publish_if_changed→publish_device_state(size, facts) -> DeviceDelta { size_changed, facts_changed }. Facts are seq-orthogonal (D3): a pure-scale change the OS absorbs shipsSetDeviceFactswithout bumpingseq(no phantom input-drop).SetViewport+seq, facts→SetDeviceFacts), reclip on fullplacement != prev— all atomic, so placement↔seq stay coherent.Resizedarm reduced togpu.resize+request_redraw; NEWScaleFactorChangedarm closes the X11-only DPI gap (#11-shell-viewport-scalefactorchanged-x11-coverage, the F3 slot) + NEWThemeChangedarm;resumedseeds facts before spawn.Construction-input (C1 parity)
DeviceFactsthread through the pipeline builders →run_scripts_and_finalizeseeds the bridge dppx/color-scheme before initial scripts, so a tab on a HiDPI / dark display is born with the rightdevicePixelRatio/matchMedia, not 1×/Light raced-in after.Consumer (boa; deleted-with-boa at S5)
evaluate_media_query_rawrouted through canonicalelidex_css::media::evaluate(D4) over aMediaEnvironmentbuilt by the singleHostBridge::media_environment; retires the boa-local feature table + theprefers-color-scheme => falsestub.SetDeviceFactsarm: value-guardedset_device_pixel_ratio+set_color_scheme→re_evaluate_media_queries→ dispatch MQLchange→re_render.IPC
BrowserToContent::SetDeviceFacts { color_scheme, dppx }— unified, noseq(D5).Scope boundary (design-lens call)
CSS-cascade
<style>@media (resolution | prefers-color-scheme)stays carved (#11-media-prefers-features/#11-media-css-values-fidelity, engine-independentelidex-style— a different layer). C3 lights the JS-observable surface (devicePixelRatio+matchMedia/MQL).#11-media-prefers-features's discharge is scoped accordingly.Tests
Cell delta (size/facts seq-orthogonality), D4 routing (matchMedia reads
prefers-color-scheme/resolutionvia canonical eval),SetDeviceFactsarm activation + MQLchange.Gates: full
mise run ci✅,/elidex-review5-axis 0 CRIT / 0 IMP (6 MIN dispositioned — §-number doc adds + test relocation applied;lib.rs@1000 +#11-window-level-tab-bar-positionledger registration noted for landing).🤖 Generated with Claude Code