Conversation
Commit 0569a00 changed the SDK ui_button signature from returning a bool to taking an on_click closure, but hello-oxide still used the old API, breaking `cargo clippy --workspace --all-targets`. Co-authored-by: Cursor <cursoragent@cursor.com>
Sensitive APIs (camera, microphone, geolocation, screen capture) are no longer granted automatically. The first request per origin queues a Chrome-style prompt rendered by the GPUI shell at the top-left under the toolbar, with Allow/Block buttons; decisions are remembered per (origin, kind) for the lifetime of the tab. Because guest frame callbacks run on the UI thread, a blocking modal inside a host function would deadlock the renderer. The flow is instead non-blocking: gated APIs return PERMISSION_PENDING (-5) while the prompt is up and the guest retries on a later frame, mirroring the async nature of the web's getUserMedia/geolocation APIs. - new oxide-browser/src/permissions.rs: PermissionKind/Status, per-origin decision store, single pending prompt slot, unit tests - OxideUrl::app_origin / app_origin_of: stable app identity (scheme+host+ port for http(s); containing directory for file:// so local apps don't share grants) - media_capture: replace blocking rfd dialogs with the permission gate - api_get_location: gated; returns 0 bytes while pending or denied - ui: render the pending prompt with Allow/Block wired to resolve_pending - sdk: PERMISSION_PENDING const + doc updates for the gated wrappers - media-capture example: "wanted" flags retry opens each frame while the prompt is pending, so capture starts right after the user clicks Allow Co-authored-by: Cursor <cursoragent@cursor.com>
Apps can ship metadata next to their .wasm module: the browser resolves the manifest at the same URL with .wasm replaced by .toml (https://host/app.wasm -> https://host/app.toml), for HTTP(S), file:// loads, and the toolbar Open picker. Format: name = "Media Capture" # required, shown as the tab title description = "..." # optional version = "0.1.0" # optional permissions = ["camera", "microphone", "geolocation", "screen-capture"] A present manifest acts as a capability declaration that integrates with the permission model: sensitive APIs not listed in `permissions` are denied without prompting, so users are only ever asked for capabilities an app declares up front. Apps without a manifest keep the legacy prompt-on-first-use behavior. Malformed manifests log a console warning and the app loads without one. - new oxide-browser/src/manifest.rs: AppManifest parse/fetch (64 KiB cap, 10s timeout, 404 treated as absent), sibling-path lookup for local files, manifest_allows() gate, unit tests - HostState.manifest set on navigation (fetch_and_run), file picker (poll_file_pick reads the sibling .toml), and LoadLocal requests - permission gates (camera/mic/screen/geolocation) consult the manifest before the per-origin prompt flow - tab title prefers the manifest name over the URL-derived title - ship a manifest with every example app, named after its build artifact (e.g. examples/media-capture/media_capture.toml); media-capture declares camera/microphone/screen-capture - document the format and permission integration in DOCS.md Co-authored-by: Cursor <cursoragent@cursor.com>
Persistent KV keys were prefixed with the full current_url string, which broke storage in two ways: - current_url mutates when the guest calls push_state/replace_state, so an app that pushed a route lost access to KV data it wrote earlier - every path got its own store (https://host/a.wasm vs https://host/b.wasm), while scheme variants of the same host were also treated as unrelated Introduce HostState::module_origin, captured once per module load (in run_module, before start_app) via set_module_origin() and immune to guest history calls. Origins use OxideUrl::app_origin: scheme+host+port for http(s), the containing directory for file:// so separate local apps don't share state. KV storage and the permission gates from the permission model now key off this stable origin. Session storage (api_storage_*) previously survived navigation across apps in the same tab, leaking one app's data to the next. It is now cleared whenever a load crosses origins, while still surviving same-origin reloads (sessionStorage-like semantics). Existing KV data stored under old full-URL prefixes is not migrated. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 15 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR migrates the Oxide UI button API from boolean-return polling to callback closures, and introduces a complete permission gating system with optional app manifests. The host implements per-origin permission tracking, manifest loading, and UI overlays; the SDK changes the button signature; and all examples and documentation are updated to use the new patterns. ChangesUI Button API Migration and Permission System Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Bring the documentation and the landing page in line with the recently shipped permission model, manifest system, and origin model fix: - README: permission prompts + manifests in the intro and security model, "Optional app manifest" section with the TOML format, manifest step in the loading pipeline, origin-scoped storage in the capability table - DOCS.md: storage section explains origin scoping and sessionStorage-like clearing; media capture section documents the non-blocking PERMISSION_PENDING retry flow with an updated example; get_location notes permission gating - SECURITY.md: permission bypasses and cross-origin storage leaks added to scope; security design lists prompts, manifest declarations, and origin-scoped storage as layers - ROADMAP.md: mark permission prompts, app manifests, and origin-scoped storage as shipped in Phase 0 - oxide-docs: storage and media-capture overviews updated to match - oxide-landing: hero stat 150+ -> 200+ host APIs (203 registered), Sandboxed card mentions permission prompts and manifests, permissions capability tag in the architecture diagram, Phase 0 roadmap card lists the new security features Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DOCS.md (1)
331-348:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Interactive Widgets section to the callback API.
Line 335 and Line 341 still show the old
ui_button(...)->boolusage, which no longer matches the current SDK and will fail when copied.Suggested doc fix
-Widgets are **immediate-mode**: call them every frame from `on_frame()`. They return the current value. The host renders them as native GPUI elements overlaid on the canvas. +Widgets are **immediate-mode**: call them every frame from `on_frame()`. Most return current value; buttons run a callback on click. The host renders them as native GPUI elements overlaid on the canvas. -| `ui_button` | `fn(id, x, y, w, h: f32, label: &str) -> bool` | Button; returns `true` when clicked | +| `ui_button` | `fn(id, x, y, w, h: f32, label: &str, on_click: impl FnOnce())` | Button; runs callback when clicked | -if ui_button(1, 20.0, 100.0, 120.0, 28.0, "Click Me!") { - log("Button was clicked!"); -} +ui_button(1, 20.0, 100.0, 120.0, 28.0, "Click Me!", || { + log("Button was clicked!"); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DOCS.md` around lines 331 - 348, The docs still show the old immediate-mode signatures for ui_button, ui_checkbox, ui_slider, and ui_text_input that return values; update the "Interactive Widgets" section to the new callback-style API by replacing those signatures and examples with the callback-based usage (ui_button, ui_checkbox, ui_slider, ui_text_input) where you pass a closure/handler invoked with events or the current value instead of expecting a return value—ensure the table entries and the example block reflect the new signatures and demonstrate handling the click/event or receiving the current state via the provided callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/worker-demo/src/lib.rs`:
- Around line 90-118: The click handler currently sets s.phase = 1 and
overwrites s.handle by calling spawn_worker(...) unconditionally, which can
orphan a running worker if the button is clicked again; update the closure to
first check s.handle (the current worker handle) and either ignore the click
while a worker is active, or call worker_terminate(s.handle) and wait/confirm
termination before spawning a new worker, then only assign s.handle = handle
when spawn_worker returns a positive value and call
worker_post_message(s.handle, ...); also ensure s.handle is reset to 0 if
termination fails or after successful termination to avoid accidental reuse.
In `@oxide-browser/src/capabilities.rs`:
- Around line 750-756: When the module origin changes in set_module_origin, also
clear the live media-capture state so a new origin cannot reuse prior
MediaCaptureState; locate set_module_origin and, alongside the existing
state.storage.lock().unwrap().clear(), acquire and reset/clear the media capture
structure (e.g. state.media_capture.lock().unwrap().clear() or otherwise reset
the MediaCaptureState held on HostState) so any existing camera/microphone
handles cannot be used by the new origin (affecting api_camera_capture_frame and
api_microphone_read_samples).
- Around line 1365-1382: The current geolocation host-call closure collapses
manifest-denied, user-denied, and prompt-pending into the same 0 return; update
the closure so each PermissionStatus is mapped to a distinct u32 so the guest
can distinguish states: use crate::manifest::manifest_allows to return a
specific "manifest not declared" code, handle
crate::permissions::check_or_request and map PermissionStatus::Granted to the
existing success path, PermissionStatus::Denied to a distinct "denied" code, and
the prompt/pending variant to a distinct "pending" code; ensure callers of this
closure (ABI consumers) are updated to expect these distinct codes if needed.
In `@oxide-browser/src/manifest.rs`:
- Around line 142-151: The code currently calls response.bytes().await then
checks bytes.len(), which buffers the entire body; instead, first check
response.content_length() (or the Content-Length header) and return Err if it
exceeds MAX_MANIFEST_SIZE, and if unknown stream the body with
response.bytes_stream() or response.chunk() inside the function that currently
calls response.bytes().await: iterate chunks, accumulate into a Vec<u8> (or a
BytesMut) while tracking total_read and immediately return an Err when
total_read > MAX_MANIFEST_SIZE; replace the direct use of response.bytes().await
and the bytes variable with this streaming read and the same error messages,
referencing response, MAX_MANIFEST_SIZE, and the current bytes handling logic.
- Around line 76-82: manifest_url_for currently strips query and fragment by
using without_query, which loses signed/versioned query params; instead, keep
the original suffix (query/fragment) when converting ".wasm" to ".toml". In
manifest_url_for, compute the split index of the first '?' or '#' (if any), let
base = &url[..idx] and suffix = &url[idx..] (or suffix = "" if none); check
base.strip_suffix(".wasm") and, if present, return
format!("{base_without_wasm}.toml{suffix}"). Update references in the function
(wasm_url.as_str(), without_query) accordingly to preserve query/fragment in the
returned manifest URL.
In `@oxide-browser/src/runtime.rs`:
- Around line 254-267: run_bytes() is inheriting the previous BrowserHost
origin/manifest (host_state.current_url and host_state.manifest) and must not
reuse them; either extend run_bytes() to accept an explicit source URL/origin
and optional Manifest (mirroring fetch_and_run()) and set those into
self.host_state before invoking run_module(), or explicitly reset
self.host_state.current_url and self.host_state.manifest to a safe local
default/None under the same lock() guard before calling run_module(); update any
callers accordingly so in-memory/local modules never reuse the previous app's
origin/manifest.
In `@oxide-browser/src/ui.rs`:
- Around line 282-290: The tab title logic in the PageStatus::Running arm
currently always prefers host_state.manifest.name, causing internal pages to
show an app's manifest title; change the branch to first detect internal pages
(e.g., URL scheme starts_with "oxide://" or the specific paths "oxide://home",
"oxide://history", "oxide://about", "oxide://forge") and return
url_to_title(url) for those; only fall back to using
self.host_state.manifest.lock().unwrap().as_ref().name when the URL is not an
internal oxide:// page and the manifest name is non-empty. Ensure you update the
PageStatus::Running match arm in ui.rs accordingly.
---
Outside diff comments:
In `@DOCS.md`:
- Around line 331-348: The docs still show the old immediate-mode signatures for
ui_button, ui_checkbox, ui_slider, and ui_text_input that return values; update
the "Interactive Widgets" section to the new callback-style API by replacing
those signatures and examples with the callback-based usage (ui_button,
ui_checkbox, ui_slider, ui_text_input) where you pass a closure/handler invoked
with events or the current value instead of expecting a return value—ensure the
table entries and the example block reflect the new signatures and demonstrate
handling the click/event or receiving the current state via the provided
callback.
🪄 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: 375001cf-bdb5-4db8-aadf-85733774b1bb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
DOCS.mdREADME.mdexamples/audio-player/audio_player.tomlexamples/audio-player/src/lib.rsexamples/events-demo/events_demo.tomlexamples/events-demo/src/lib.rsexamples/file-picker-demo/file_picker_demo.tomlexamples/file-picker-demo/src/lib.rsexamples/fullstack-notes/frontend/fullstack_notes_frontend.tomlexamples/gpu-graphics-demo/gpu_graphics_demo.tomlexamples/gradient-demo/gradient_demo.tomlexamples/hello-oxide/hello_oxide.tomlexamples/hello-oxide/src/lib.rsexamples/index/index.tomlexamples/media-capture/media_capture.tomlexamples/media-capture/src/lib.rsexamples/midi-demo/midi_demo.tomlexamples/midi-demo/src/lib.rsexamples/raf-demo/raf_demo.tomlexamples/raf-demo/src/lib.rsexamples/rtc-chat/rtc_chat.tomlexamples/rtc-chat/src/lib.rsexamples/shadcn-demo/shadcn_demo.tomlexamples/shadcn-demo/src/lib.rsexamples/stream-fetch-demo/src/lib.rsexamples/stream-fetch-demo/stream_fetch_demo.tomlexamples/timer-demo/src/lib.rsexamples/timer-demo/timer_demo.tomlexamples/typography-demo/typography_demo.tomlexamples/video-player/src/lib.rsexamples/video-player/video_player.tomlexamples/worker-demo/src/lib.rsexamples/worker-demo/worker_demo.tomlexamples/ws-chat/src/lib.rsexamples/ws-chat/ws_chat.tomlforge/skills/oxide-wasm-app/SKILL.mdforge/skills/oxide-wasm-app/references/CAPABILITIES.mdforge/skills/oxide-wasm-app/references/PATTERNS.mdforge/skills/oxide-wasm-app/references/RECIPES.mdoxide-browser/Cargo.tomloxide-browser/src/capabilities.rsoxide-browser/src/lib.rsoxide-browser/src/manifest.rsoxide-browser/src/media_capture.rsoxide-browser/src/permissions.rsoxide-browser/src/runtime.rsoxide-browser/src/ui.rsoxide-browser/src/url.rsoxide-docs/src/lib.rsoxide-sdk/src/lib.rs
Resolve all 8 actionable comments plus the outside-diff docs issue from the CodeRabbit review of #50: - capabilities: set_module_origin also resets MediaCaptureState on an origin change, so a new origin can't read camera frames or mic samples from devices the previous origin opened (critical) - capabilities/sdk: api_get_location returns i32 — bytes written, PERMISSION_PENDING (-5) while the prompt shows, -1 once blocked (by the user or an undeclared manifest permission); the SDK wrapper is now get_location() -> Result<String, i32> so guests can retry only while pending (same wire type, no ABI break) - manifest: manifest_url_for preserves the query string so signed or versioned module URLs (app.wasm?token=…) resolve their sibling .toml; fragment is still dropped - manifest: the 64 KiB cap is enforced while downloading — Content-Length is rejected up front and the body is streamed with a running byte budget instead of buffering before the check - runtime/ui: run_bytes() takes an explicit source URL + manifest and sets both on HostState before run_module, so a reused BrowserHost never scopes storage/permissions to the previous app; RunRequest:: LoadLocal threads the URL from the file picker and forge runs - ui: tab titles only use the manifest name when no internal page is shown, so oxide:// pages stop displaying a stale app name - ui: keyboard path for the permission prompt — Enter allows, Escape blocks (unmodified keys, swallowed before the guest sees them), with a hint line in the prompt - worker-demo: ignore clicks while a worker is computing so a running worker is never orphaned by overwriting its handle - DOCS/forge skill: Interactive Widgets section updated to the callback ui_button API; get_location docs reflect the Result return Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Summary by CodeRabbit
New Features
Documentation
Improvements