Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and safety concerns around env syncing (flush coverage in the test renderer and potential cross-thread environment mutation in native) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves core runtime ergonomics across GPUIX’s TypeScript/Rust boundary: fixing selection wash coverage on soft-wrapped text, keeping event routing stable across Bun hot reloads, enabling live scroll-wheel automation, and ensuring Bun process.env overrides can be observed from Rust.
Changes:
- Fix selection wash geometry so wrapped-line continuation rows include the first glyph.
- Make Bun hot remounts preserve event delivery by sharing renderer→container mappings via
globalThisand by makingonEventswappable. - Add live automation scroll-wheel dispatch plus a new native
syncEnvVarhook (and debug-profile PNG encoder optimization).
File summaries
| File | Description |
|---|---|
| packages/react/src/testing.ts | Adds env override syncing and exposes scrollIntoView on the JS test renderer. |
| packages/react/src/reconciler/renderer.ts | Routes native events through a hot-reload-stable onEvent slot. |
| packages/react/src/reconciler/event-registry.ts | Persists renderer→container mapping across Bun --hot module reloads. |
| packages/react/src/automation/client.ts | Enables live scrollWheel by calling into native scroll-wheel simulation. |
| packages/react/src/tests/automation-stdio.test.ts | Adjusts rejection assertion to avoid Bun runner stalling behavior. |
| packages/native/src/text/paint.rs | Fixes selection wash rects across wrapped visual rows. |
| packages/native/src/style/vars.rs | Formatting-only test/utility cleanup. |
| packages/native/src/style/resolve.rs | Formatting-only test cleanup. |
| packages/native/src/style.rs | Formatting-only test cleanup. |
| packages/native/src/renderer/frame.rs | Formatting-only refactor of element build dispatch. |
| packages/native/src/renderer.rs | Adds sync_env_var export and production simulate_scroll_wheel dispatch path. |
| packages/native/src/motion.rs | Formatting-only test cleanup. |
| packages/native/src/color.rs | Adds gradient stop easing mapping plus formatting changes. |
| packages/native/src/automation.rs | Implements scroll-wheel event dispatch through the window. |
| packages/native/index.js | Exports syncEnvVar from the native binding. |
| packages/native/index.d.ts | Declares simulateScrollWheel and syncEnvVar in the native TS surface. |
| packages/native/Cargo.toml | Speeds up debug visual tests by compiling PNG/image crates with opt-level 3. |
| AGENTS.md | Documentation tidy-ups and a corrected Zed link. |
| .changeset/sync-env-overrides-under-bun.md | Changeset for Bun env override syncing. |
| .changeset/survive-a-hot-remount.md | Changeset for hot remount event survivability. |
| .changeset/select-across-wrapped-rows.md | Changeset for wrapped-row selection wash fix. |
| .changeset/live-scroll-wheel.md | Changeset for live scroll-wheel automation. |
Review details
Suppressed comments (1)
packages/react/src/testing.ts:244
- syncEnvOverrides() is only invoked when calling TestRenderer.flush(), but most methods in this class call this.native.flush() directly. That means env overrides may not be synced for many render passes (e.g. scrollTo/scrollIntoView/simulation helpers) even though the comment says it runs before every frame flush. One way to ensure coverage without touching all call sites is to wrap this.native.flush once and then call it normally.
flush(): void {
syncEnvOverrides()
this.native.flush()
}
- Files reviewed: 21/22 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
# Conflicts: # packages/native/src/renderer/frame.rs
# Conflicts: # packages/native/src/renderer/frame.rs # packages/react/src/reconciler/event-registry.ts
|
Moved to remorses#54. |
Contains #4.
What this adds
position_for_index, which reports a soft-wrap boundary on the earlier row, so each continuation row started one glyph late.bun --hotremount. The renderer-to-container map now lives onglobalThis, so the module instance the native callback holds and the reloaded instance share it. The slot only takes over a value that really is a WeakMap.process.envoverrides reach Rust under Bun through asyncEnvVarexport. Bun only updates its JS snapshot, so the Rust side never saw a late assignment. The values land in an override map, not insetenv, becausesetenvracesgetenvon the dedicated UI thread of Windows and Linux.capture_screenshotencodes a png per visual test, and the encoder at opt-level 0 was slow enough that vitest killed the worker.The live
scrollWheelautomation method this PR first added now comes from upstream main with modifier support, so this branch dropped its own copy.What I ran and saw
cargo testat this branch: 301 passed, 0 failed.What I did not verify