fix(browser): resolve GPUI migration bugs, add high-level drawing API…#28
Merged
Conversation
…, and update docs Three input-handling bugs introduced during the egui → GPUI migration are fixed, a new GPUI-inspired drawing module is added to the SDK, and all documentation is updated to reflect the current API surface. ## Bug fixes (ui.rs) - **Modifier keys never synced**: `modifiers_shift`, `modifiers_ctrl`, and `modifiers_alt` in `InputState` were never populated from GPUI keystroke events. The SDK functions `shift_held()`, `ctrl_held()`, `alt_held()` always returned false. Fixed by reading `event.keystroke.modifiers` in both `on_key_down` and `on_key_up` handlers. - **Canvas dimensions hardcoded to 800×600**: `CanvasState.width/height` were set once in `Default::default()` and never updated from the actual GPUI canvas bounds. `canvas_dimensions()` always returned (800, 600). Fixed by writing `bounds.size` into `CanvasState` in the canvas prepare callback each frame. - **Right/middle mouse clicks not tracked**: Only the left mouse button was reflected in `mouse_buttons_clicked`. The `on_mouse_up` handlers for Right and Middle now set the corresponding clicked flag, and the duplicate left-click tracking in `on_click` is removed to avoid double-counting. ## New: oxide-sdk/src/draw.rs Added a high-level drawing module inspired by GPUI conventions: - `Color` — sRGB+alpha with `rgb()`, `rgba()`, `hex()`, named constants - `Point2D` — 2D canvas point - `Rect` — axis-aligned rectangle with `contains()` hit-testing - `Canvas` — zero-cost facade wrapping low-level `canvas_*` functions ## Documentation - Rewrote DOCS.md with complete API reference covering all ~100 host functions: canvas (low-level + high-level), widgets, audio, video, media capture, timers, navigation, hyperlinks, crypto, and troubleshooting. - Updated oxide-sdk module docs with draw module, version references, and guest module contract. - Updated oxide-browser/src/lib.rs with GPUI-to-SDK mapping table showing how each SDK draw call translates to GPUI GPU primitives. - Updated oxide-docs/src/lib.rs with high-level drawing API, video, and media capture sections. Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughReplaced egui/eframe with GPUI across docs and the desktop shell, added a high-level drawing API ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
DOCS.md (1)
55-55: Reword repeated sentence openings for readability.Line 55 starts three consecutive sentences with “No …”; consider combining for smoother flow.
✍️ Suggested wording
-No layout engine. No style cascade. No DOM. The guest decides exactly where every pixel goes. +There is no layout engine, style cascade, or DOM—the guest decides exactly where every pixel goes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DOCS.md` at line 55, The three consecutive sentences that each start with "No" (the sentence beginning "No layout engine.", "No style cascade.", and "No DOM.") should be rephrased/combined for better flow; edit the paragraph containing "No layout engine. No style cascade. No DOM. The guest decides exactly where every pixel goes." to either combine into a single sentence like "There is no layout engine, style cascade, or DOM; the guest decides exactly where every pixel goes." or vary the openings (e.g., "There is no layout engine. There is no style cascade. There is no DOM; the guest...") so the repetition is removed while preserving meaning. Ensure the revised sentence(s) keep the original emphasis that the guest controls pixel placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DOCS.md`:
- Around line 16-43: The fenced ASCII diagram block in the DOCS.md architecture
section is missing a language tag; update the opening fence for that diagram
(the triple backticks that currently start the ASCII art block) to include a
language like text (e.g., change ``` to ```text) so the markdown linter
recognizes the block as a code/fenced block with a language.
In `@oxide-browser/src/main.rs`:
- Line 10: The code currently panics on window creation inside
oxide_browser::ui::run_browser (the call that uses .expect("open window"));
remove the .expect and propagate the error instead so callers (like main's
run_browser call) can handle it. Replace the .expect("open window") on the
window creation Result with a fallible return (use the ? operator, or
map_err/with_context to convert to anyhow::Error and then ?), ensuring
run_browser keeps returning anyhow::Result<()> and that any context is added via
with_context or .context(...) before returning.
In `@oxide-sdk/src/draw.rs`:
- Around line 112-114: Rect::contains currently treats right and bottom edges as
inclusive which causes adjacent rectangles that touch to both report hits;
change the bounds to half-open by testing px >= self.x && px < self.x + self.w
and py >= self.y && py < self.y + self.h so the max edges are exclusive. Update
the contains method in the Rect implementation accordingly so touching edges are
only counted by one rect.
In `@oxide-sdk/src/lib.rs`:
- Around line 103-107: The docs currently suggest on_timer works independently;
update the comment block describing the guest contract to state that timer
callbacks (on_timer) are only delivered for modules that also export on_frame
(i.e., live/interactive modules), so guests exporting only on_timer will not
receive timer callbacks; reference the symbols start_app, on_frame, on_timer,
set_timeout, and set_interval and add a short note explaining this dependency in
the comment.
---
Nitpick comments:
In `@DOCS.md`:
- Line 55: The three consecutive sentences that each start with "No" (the
sentence beginning "No layout engine.", "No style cascade.", and "No DOM.")
should be rephrased/combined for better flow; edit the paragraph containing "No
layout engine. No style cascade. No DOM. The guest decides exactly where every
pixel goes." to either combine into a single sentence like "There is no layout
engine, style cascade, or DOM; the guest decides exactly where every pixel
goes." or vary the openings (e.g., "There is no layout engine. There is no style
cascade. There is no DOM; the guest...") so the repetition is removed while
preserving meaning. Ensure the revised sentence(s) keep the original emphasis
that the guest controls pixel placement.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 005eabaf-5b3a-49cf-a249-4cb73da09ef9
📒 Files selected for processing (16)
CONTRIBUTING.mdDOCS.mdREADME.mdROADMAP.mdoxide-browser/Cargo.tomloxide-browser/src/audio_format.rsoxide-browser/src/capabilities.rsoxide-browser/src/lib.rsoxide-browser/src/main.rsoxide-browser/src/ui.rsoxide-browser/src/video_format.rsoxide-docs/index.htmloxide-docs/src/lib.rsoxide-landing/index.htmloxide-sdk/src/draw.rsoxide-sdk/src/lib.rs
Comment on lines
+16
to
+43
| ┌──────────────────────────────────────────────────────┐ | ||
| │ Oxide Browser │ | ||
| │ ┌──────────┐ ┌────────────┐ ┌──────────────┐ │ | ||
| │ │ URL Bar │ │ Canvas │ │ Console │ │ | ||
| │ └────┬─────┘ └──────┬─────┘ └──────┬───────┘ │ | ||
| │ │ │ │ │ | ||
| │ ┌────▼───────────────▼───────────────▼───────┐ │ | ||
| │ │ Host Runtime │ │ | ||
| │ │ wasmtime engine + sandbox policy │ │ | ||
| │ │ fuel limit: 500M │ memory: 16MB max │ │ | ||
| │ └────────────────────┬───────────────────────┘ │ | ||
| │ │ │ | ||
| │ ┌────────────────────▼───────────────────────┐ │ | ||
| │ │ Capability Provider │ │ | ||
| │ │ "oxide" import module │ │ | ||
| │ │ canvas, console, storage, clipboard, │ │ | ||
| │ │ fetch, audio, video, media capture, │ │ | ||
| │ │ crypto, timers, navigation, widgets, │ │ | ||
| │ │ input, hyperlinks, dynamic loading │ │ | ||
| │ └────────────────────┬───────────────────────┘ │ | ||
| │ │ │ | ||
| │ ┌────────────────────▼───────────────────────┐ │ | ||
| │ │ Guest .wasm Module │ │ | ||
| │ │ exports: start_app(), on_frame(dt_ms) │ │ | ||
| │ │ imports: oxide::* │ │ | ||
| │ └────────────────────────────────────────────┘ │ | ||
| └──────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
The architecture block should declare a fence language (e.g., text) to satisfy markdown linting.
🧩 Suggested change
-```
+```text
┌──────────────────────────────────────────────────────┐
...
└──────────────────────────────────────────────────────┘</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DOCS.md` around lines 16 - 43, The fenced ASCII diagram block in the DOCS.md
architecture section is missing a language tag; update the opening fence for
that diagram (the triple backticks that currently start the ASCII art block) to
include a language like text (e.g., change ``` to ```text) so the markdown
linter recognizes the block as a code/fenced block with a language.
| ) | ||
| .map_err(|e| anyhow::anyhow!("eframe error: {e}"))?; | ||
|
|
||
| oxide_browser::ui::run_browser(host_state, status)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether run_browser still panics on window-open failure.
rg -n 'expect\("open window"\)' oxide-browser/src/ui.rs -C2
ast-grep --pattern $'pub fn run_browser($_, $_) -> anyhow::Result<()> { $$$ expect($_) $$$ }' oxide-browser/src/ui.rsRepository: niklabh/oxide
Length of output: 230
Remove expect("open window") to avoid panics in error path.
The run_browser function returns anyhow::Result<()>, but line 1671 in oxide-browser/src/ui.rs calls .expect("open window") which panics on window creation failure instead of propagating the error. This bypasses graceful error handling at the call site in main.rs:10. Replace the expect() call with proper error handling to return the error to the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@oxide-browser/src/main.rs` at line 10, The code currently panics on window
creation inside oxide_browser::ui::run_browser (the call that uses .expect("open
window")); remove the .expect and propagate the error instead so callers (like
main's run_browser call) can handle it. Replace the .expect("open window") on
the window creation Result with a fallible return (use the ? operator, or
map_err/with_context to convert to anyhow::Error and then ?), ensuring
run_browser keeps returning anyhow::Result<()> and that any context is added via
with_context or .context(...) before returning.
Comment on lines
+103
to
+107
| //! 1. **Export `start_app`** — `extern "C" fn()` entry point, called once on load. | ||
| //! 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for | ||
| //! interactive apps with a render loop (called every frame, fuel replenished). | ||
| //! 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` | ||
| //! to receive timer callbacks from [`set_timeout`] / [`set_interval`]. |
There was a problem hiding this comment.
Clarify on_timer dependency on on_frame in the guest contract.
As written, this implies on_timer works independently. In current runtime flow, timer callbacks are only driven for live modules that export on_frame, so on_timer-only guests won’t receive callbacks.
🛠️ Suggested doc correction
-//! 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for
-//! interactive apps with a render loop (called every frame, fuel replenished).
-//! 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)`
-//! to receive timer callbacks from [`set_timeout`] / [`set_interval`].
+//! 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for
+//! interactive apps with a render loop (called every frame, fuel replenished).
+//! 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)`
+//! to receive timer callbacks from [`set_timeout`] / [`set_interval`].
+//! In the current host runtime, timer callbacks are dispatched during the live
+//! frame loop, so exporting `on_frame` is also required.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| //! 1. **Export `start_app`** — `extern "C" fn()` entry point, called once on load. | |
| //! 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for | |
| //! interactive apps with a render loop (called every frame, fuel replenished). | |
| //! 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` | |
| //! to receive timer callbacks from [`set_timeout`] / [`set_interval`]. | |
| //! 1. **Export `start_app`** — `extern "C" fn()` entry point, called once on load. | |
| //! 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for | |
| //! interactive apps with a render loop (called every frame, fuel replenished). | |
| //! 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` | |
| //! to receive timer callbacks from [`set_timeout`] / [`set_interval`]. | |
| //! In the current host runtime, timer callbacks are dispatched during the live | |
| //! frame loop, so exporting `on_frame` is also required. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@oxide-sdk/src/lib.rs` around lines 103 - 107, The docs currently suggest
on_timer works independently; update the comment block describing the guest
contract to state that timer callbacks (on_timer) are only delivered for modules
that also export on_frame (i.e., live/interactive modules), so guests exporting
only on_timer will not receive timer callbacks; reference the symbols start_app,
on_frame, on_timer, set_timeout, and set_interval and add a short note
explaining this dependency in the comment.
- Add alpha channel support to canvas_text and canvas_line (FFI, SDK, host renderer, and all call sites) so the full Color API is honored - Fix double-lock of status mutex in toolbar rendering (TOCTOU race) - Add SAFETY comment to unsafe impl Send for RunResult - Use can_back/can_fwd to visually disable navigation buttons - Replace window.drop_image() with Arc drop (not a public GPUI API) - Fix Rect::contains to use half-open intervals (standard convention) - Add #[must_use] to Color::with_alpha - Fix "builder-style" → "immediate-mode" in draw.rs docs - Fix unused mut in draw.rs doc example - Update README: "~50 host functions" → "~100", "Open File" → "Open" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DOCS.md`:
- Around line 579-589: The docs state `on_frame` and `on_timer` are optional but
omit that timer callbacks are only dispatched during the frame loop; update the
Guest Module Contract section to explicitly note that `on_timer` callbacks are
only delivered during `LiveModule::advance()` (the frame loop) so a guest must
also export `on_frame` (i.e., participate in the frame loop) to receive timers;
mention the symbols `on_timer`, `on_frame`, and `LiveModule::advance` to make
locating the behavior clear.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01a36304-2398-4077-a572-32217a881c20
📒 Files selected for processing (16)
CONTRIBUTING.mdDOCS.mdREADME.mdexamples/audio-player/src/lib.rsexamples/fullstack-notes/frontend/src/lib.rsexamples/hello-oxide/src/lib.rsexamples/index/src/lib.rsexamples/media-capture/src/lib.rsexamples/timer-demo/src/lib.rsexamples/video-player/src/lib.rsoxide-browser/src/capabilities.rsoxide-browser/src/ui.rsoxide-docs/src/lib.rsoxide-sdk/README.mdoxide-sdk/src/draw.rsoxide-sdk/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- oxide-sdk/src/draw.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- CONTRIBUTING.md
- README.md
- oxide-docs/src/lib.rs
Comment on lines
+579
to
+589
| ## Guest Module Contract | ||
|
|
||
| Every `.wasm` module loaded by Oxide must: | ||
|
|
||
| 1. **Export `start_app`** — `extern "C" fn()` entry point, called once on load. | ||
| 2. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for interactive apps with a render loop. | ||
| 3. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` to receive timer callbacks. | ||
| 4. **Import from `"oxide"` module** — All host capabilities are under this namespace. | ||
| 5. **Compile as `cdylib`** — `crate-type = ["cdylib"]` in `Cargo.toml`. | ||
| 6. **Target `wasm32-unknown-unknown`** — no WASI, pure capability-based I/O. | ||
|
|
There was a problem hiding this comment.
Clarify on_timer dependency on on_frame.
The Guest Module Contract states both on_frame and on_timer are optional, but timer callbacks are only dispatched during the frame loop (within LiveModule::advance()). A guest that exports on_timer without on_frame will not receive timer callbacks.
📝 Suggested clarification
4. **Optionally export `on_frame`** — `extern "C" fn(dt_ms: u32)` for interactive apps with a render loop.
-5. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` to receive timer callbacks.
+5. **Optionally export `on_timer`** — `extern "C" fn(callback_id: u32)` to receive timer callbacks.
+ Timer callbacks are dispatched during the frame loop, so `on_frame` must also be exported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DOCS.md` around lines 579 - 589, The docs state `on_frame` and `on_timer` are
optional but omit that timer callbacks are only dispatched during the frame
loop; update the Guest Module Contract section to explicitly note that
`on_timer` callbacks are only delivered during `LiveModule::advance()` (the
frame loop) so a guest must also export `on_frame` (i.e., participate in the
frame loop) to receive timers; mention the symbols `on_timer`, `on_frame`, and
`LiveModule::advance` to make locating the behavior clear.
This was referenced Apr 12, 2026
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.
…, and update docs
Three input-handling bugs introduced during the egui → GPUI migration are fixed, a new GPUI-inspired drawing module is added to the SDK, and all documentation is updated to reflect the current API surface.
Bug fixes (ui.rs)
Modifier keys never synced:
modifiers_shift,modifiers_ctrl, andmodifiers_altinInputStatewere never populated from GPUI keystroke events. The SDK functionsshift_held(),ctrl_held(),alt_held()always returned false. Fixed by readingevent.keystroke.modifiersin bothon_key_downandon_key_uphandlers.Canvas dimensions hardcoded to 800×600:
CanvasState.width/heightwere set once inDefault::default()and never updated from the actual GPUI canvas bounds.canvas_dimensions()always returned (800, 600). Fixed by writingbounds.sizeintoCanvasStatein the canvas prepare callback each frame.Right/middle mouse clicks not tracked: Only the left mouse button was reflected in
mouse_buttons_clicked. Theon_mouse_uphandlers for Right and Middle now set the corresponding clicked flag, and the duplicate left-click tracking inon_clickis removed to avoid double-counting.New: oxide-sdk/src/draw.rs
Added a high-level drawing module inspired by GPUI conventions:
Color— sRGB+alpha withrgb(),rgba(),hex(), named constantsPoint2D— 2D canvas pointRect— axis-aligned rectangle withcontains()hit-testingCanvas— zero-cost facade wrapping low-levelcanvas_*functionsDocumentation
Made-with: Cursor
Summary by CodeRabbit
New Features
API Changes
Documentation