feat: add menu button and download manager for non-WASM URLs with parallel downloads and progress UI#32
Merged
Merged
Conversation
…and progress UI When the browser navigates to a URL whose path has a file extension other than .wasm (e.g. .txt, .zip, .pdf), the file is now downloaded to the system Downloads folder instead of being loaded as a WebAssembly module. Download manager (oxide-browser/src/download.rs): - Parallel background downloads via per-download worker threads - Streaming progress tracking (bytes received, total size, speed) - Cancel and dismiss support with automatic partial-file cleanup - Duplicate filename deduplication (appends " (1)", " (2)", etc.) - Placeholder file created eagerly to prevent parallel name collisions Browser UI changes (oxide-browser/src/ui.rs): - Downloads panel with per-file progress bar, speed, and percentage - Cancel button for active downloads, dismiss for finished ones - Toolbar download button (⬇) that highlights when downloads are active - Downloads toggle added to the browser menu - navigate() and navigate_to() detect downloadable URLs via extension Example and landing page: - hello-oxide example now includes a clickable download link for https://github.com/robots.txt demonstrating the feature end-to-end - All oxide-landing .wasm files rebuilt from latest source Also fixes pre-existing clippy warnings across the workspace: - oxide-sdk: allow too_many_arguments, doc_overindented_list_items, should_implement_trait - gpu-graphics-demo: replace needless_range_loop with enumerate - fullstack-notes backend: convert doc comment to module-level comment Made-with: Cursor
📝 WalkthroughWalkthroughChanges add download manager and browsing history modules to oxide-browser, update dependencies (futures-util, reqwest stream feature), extend HostState with history_store field, add a new download link demo to hello-oxide example, refactor arc rendering to use iterators, and add clippy suppressions. Changes
Sequence DiagramssequenceDiagram
participant UI as Browser UI
participant DM as DownloadManager
participant BT as Background Thread
participant RW as Reqwest/Writer
participant FS as File System
UI->>DM: start_download(url)
DM->>DM: generate DownloadId
DM->>DM: create DownloadProgress entry
DM->>BT: spawn background thread
BT->>RW: reqwest GET stream
activate RW
loop streaming bytes
RW->>RW: receive chunk
RW->>DM: update bytes_downloaded
RW->>RW: calculate speed
RW->>FS: write chunk
end
RW->>DM: set total_bytes (Content-Length)
RW->>DM: transition state to Completed
deactivate RW
UI->>DM: downloads()
DM->>UI: return SharedDownloads with progress
sequenceDiagram
participant Tab as Tab/Navigation
participant HS as HistoryStore
participant DB as Sled Database
Tab->>HS: record(url, title)
HS->>HS: generate composite key (timestamp + sequence)
HS->>HS: serialize HistoryItem
HS->>DB: insert(key, serialized_item)
DB->>DB: persist to disk
Tab->>HS: list_all()
HS->>DB: iterate entries
DB->>HS: return all key-value pairs
HS->>HS: deserialize items
HS->>HS: reverse to newest-first order
HS->>Tab: return Vec<(key, HistoryItem)>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (9)
examples/hello-oxide/src/lib.rs (1)
141-141: Avoid hard-coded bottom placement for mouse info.Line 141 uses a fixed Y coordinate (
525.0), which can clip on smaller canvases. Consider deriving this fromlink_yand clamping with canvas height.♻️ Suggested layout-safe adjustment
- let (width, _height) = canvas_dimensions(); + let (width, height) = canvas_dimensions(); @@ - canvas_text( + let mouse_info_y = (link_y + 45.0).min(height as f32 - 16.0); + canvas_text( 20.0, - 525.0, + mouse_info_y, 12.0, 100, 100,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/hello-oxide/src/lib.rs` at line 141, Replace the hard-coded mouse-info Y coordinate (525.0) with a layout-safe value derived from link_y and the canvas height: compute a preferred Y (e.g., link_y + offset) and clamp it between a top and bottom margin using the canvas height (e.g., max(margin, min(preferred_y, canvas_height - margin))) so the mouse info never gets clipped; update the place where the mouse info Y is set in lib.rs to use this computed value instead of 525.0.oxide-sdk/src/lib.rs (1)
1-2: Consider more targeted clippy suppressions.The crate-level
#![allow(...)]attributes apply broadly to the entire file, which may hide legitimate warnings in non-FFI code or future additions. A more targeted approach would suppress lints only where needed.♻️ Suggested refinement for more targeted suppressions
Instead of crate-level suppressions, apply them to specific scopes:
-#![allow(clippy::too_many_arguments)] -#![allow(clippy::doc_overindented_list_items)] - //! # Oxide SDKThen add to the FFI block:
// ─── Raw FFI imports from the host ────────────────────────────────────────── +#[allow(clippy::too_many_arguments)] #[link(wasm_import_module = "oxide")] extern "C" {And for the doc comment with the table:
//! ## API Categories //! +#![allow(clippy::doc_overindented_list_items)] //! | Category | Key types / functions | //! |----------|-----------|🤖 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 1 - 2, The crate currently uses broad crate-level suppressions (#![allow(clippy::too_many_arguments)] and #![allow(clippy::doc_overindented_list_items)]); narrow them by removing these top-level attributes and instead apply #[allow(...)] directly on the specific FFI module or functions that need them (e.g., the extern "C" block or the functions that expose many parameters) and apply #[allow(clippy::doc_overindented_list_items)] to the specific doc-commented item that contains the table; this keeps the rest of the crate subject to clippy checks while preserving the suppressions only where necessary.oxide-sdk/src/proto.rs (1)
309-310: Consider implementing theIteratortrait instead of suppressing the lint.The
nextmethod signature matchesIterator::next, and implementing the trait would enable use of iterator combinators (.map(),.filter(),.collect(), etc.) and make the API more idiomatic.♻️ Proposed refactor to implement Iterator
- #[allow(clippy::should_implement_trait)] - pub fn next(&mut self) -> Option<ProtoField<'a>> { +} + +impl<'a> Iterator for ProtoDecoder<'a> { + type Item = ProtoField<'a>; + + fn next(&mut self) -> Option<Self::Item> { if self.pos >= self.buf.len() { return None; }With this change, the
collect_fieldsmethod could be simplified to:pub fn collect_fields(&mut self) -> Vec<ProtoField<'a>> { - let mut fields = Vec::new(); - while let Some(f) = self.next() { - fields.push(f); - } - fields + self.collect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-sdk/src/proto.rs` around lines 309 - 310, The current standalone pub fn next(&mut self) -> Option<ProtoField<'a>> should be converted into a proper Iterator implementation: implement impl<'a> Iterator for the concrete iterator type that defines this next method (replace the standalone fn with an impl block), set type Item = ProtoField<'a>, and move the existing next body into fn next(&mut self) -> Option<Self::Item>; then remove the clippy allow(should_implement_trait) and update call sites (e.g., collect_fields) to use iterator combinators. Ensure the impl uses the same lifetime parameter and the existing ProtoField<'a> return type.oxide-browser/src/history.rs (2)
97-109: Consider usingiter().rev()instead of collecting and reversing.
list_all()collects all entries then reverses in-place. Since sled'sTree::iter()supports.rev(), you can iterate in reverse order directly, avoiding the O(n) reversal overhead.Suggested optimization
pub fn list_all(&self) -> Vec<(Vec<u8>, HistoryItem)> { let mut items = Vec::new(); - for entry in self.tree.iter().flatten() { + for entry in self.tree.iter().rev().flatten() { let (key, val) = entry; if let Some(item) = HistoryItem::from_bytes(&val) { items.push((key.to_vec(), item)); } } - items.reverse(); items }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/history.rs` around lines 97 - 109, The current list_all method collects entries then calls items.reverse(), which adds an unnecessary O(n) reversal; instead iterate the sled tree in reverse by using self.tree.iter().flatten().rev() inside list_all (keep the same key/value handling and HistoryItem::from_bytes check) and push entries in iteration order so you can remove the items.reverse() call and return the Vec directly; update the loop that currently destructures (key, val) to work with the reversed iterator and preserve pushing (key.to_vec(), item).
100-106: Sled iteration errors are silently ignored.Using
.flatten()discards anyErrvariants from the iterator. Consider logging failures to aid debugging persistent storage issues.Optional: Log errors instead of silently ignoring
pub fn list_all(&self) -> Vec<(Vec<u8>, HistoryItem)> { let mut items = Vec::new(); - for entry in self.tree.iter().flatten() { + for entry in self.tree.iter().rev() { + let (key, val) = match entry { + Ok(kv) => kv, + Err(e) => { + tracing::warn!("history iteration error: {e}"); + continue; + } + }; - let (key, val) = entry; if let Some(item) = HistoryItem::from_bytes(&val) { items.push((key.to_vec(), item)); } } - items.reverse(); items }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/history.rs` around lines 100 - 106, The current iteration over self.tree uses .flatten(), which silently drops Errs from the sled iterator; replace that with explicit matching on each entry from self.tree.iter() (e.g., for entry in self.tree.iter() { match entry { Ok((key, val)) => { if let Some(item) = HistoryItem::from_bytes(&val) { items.push((key.to_vec(), item)); } }, Err(e) => { /* log error */ } } }) so that any iteration errors are logged (use the project's logger, e.g., log::error or the existing logging util) and not ignored; ensure you reference self.tree.iter() and HistoryItem::from_bytes when making the change.oxide-browser/src/download.rs (3)
53-63: MoveDefaultimpl after struct definition.The
impl Default for DownloadManagerblock appears before the struct definition on line 59. Conventional Rust style places the struct definition first.Reorder for readability
-/// Manages parallel file downloads. Owns a shared list of [`DownloadProgress`] -/// entries that the UI polls each frame. -impl Default for DownloadManager { - fn default() -> Self { - Self::new() - } -} - #[derive(Clone)] pub struct DownloadManager { downloads: SharedDownloads, next_id: Arc<Mutex<u64>>, } + +impl Default for DownloadManager { + fn default() -> Self { + Self::new() + } +}Also move the doc comment to the struct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/download.rs` around lines 53 - 63, The impl Default for DownloadManager is placed before the DownloadManager struct; move the entire impl Default block so it appears after the DownloadManager struct definition and attach any existing doc comment to the struct itself; ensure the impl still returns Self::new() and references DownloadManager, next_id, and downloads unchanged.
118-121: Each download spawns a new thread and Tokio runtime.Creating a new
tokio::runtime::Runtimeper download has overhead. For a small number of concurrent downloads this is acceptable, but consider using a shared runtime if download volume increases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/download.rs` around lines 118 - 121, The code currently spawns a new OS thread and creates a fresh tokio::runtime::Runtime for every download (std::thread::spawn { let rt = tokio::runtime::Runtime::new().unwrap(); rt.block_on(run_download(...)) }), which is costly; instead create and reuse a single shared runtime (e.g., construct one Runtime once via once_cell::sync::Lazy or pass a tokio::runtime::Handle into the downloader) and replace the per-download Runtime::new()/block_on with runtime.handle().spawn(async move { run_download(id, url, dest, downloads).await }) or runtime.spawn(...) so run_download runs on the shared runtime; update callers to accept or access the shared Handle/Runtime and remove the per-download Runtime::new() usage.
282-289: Theoretically unbounded loop inunique_path.The
for i in 1u32..loop will iterate ~4 billion times before wrapping. In practice this is unreachable, but the fallback on line 289 is dead code.Optional: Add a reasonable upper bound
- for i in 1u32.. { + for i in 1..=10000u32 { let try_name = format!("{stem} ({i}){ext}"); let p = dir.join(&try_name); if !p.exists() { return p; } } dir.join(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/download.rs` around lines 282 - 289, The loop in unique_path currently uses for i in 1u32.. which is effectively unbounded and makes the final fallback dead code; change it to a bounded loop (e.g., const MAX_TRIES: u32 = 10_000 or another reasonable limit) and iterate for i in 1..=MAX_TRIES, constructing try_name and checking dir.join(&try_name).exists() exactly as before, and if no free name is found after MAX_TRIES return the original dir.join(name) (or return a Result::Err if you prefer to surface the failure); update the function unique_path and its use of stem, ext, dir, and name accordingly so the fallback is reachable.oxide-browser/src/runtime.rs (1)
17-17: History store integration looks correct; minor: unnecessaryArc::clone.The
HistoryStoreis correctly initialized from the shared KV database. However, sinceBookmarkStore::open(&kv_db)andHistoryStore::open(&kv_db)only borrow theArc<sled::Db>, the.clone()on line 127 is unnecessary—kv_dbcan be moved directly intoSome(...).Suggested simplification
let host_state = HostState { module_loader: Some(loader), - kv_db: Some(kv_db.clone()), + kv_db: Some(kv_db), bookmark_store: Arc::new(Mutex::new(Some(bookmark_store))), history_store: Arc::new(Mutex::new(Some(history_store))), ..Default::default() };Also applies to: 122-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oxide-browser/src/runtime.rs` at line 17, The code unnecessarily calls Arc::clone when constructing the stores; since BookmarkStore::open(&kv_db) and HistoryStore::open(&kv_db) only borrow the Arc<sled::Db>, remove the redundant Arc::clone and pass kv_db (or &kv_db) directly into Some(...) instead of cloning; update the places around the initialization of BookmarkStore and HistoryStore (references to BookmarkStore::open, HistoryStore::open and the kv_db variable) to eliminate Arc::clone calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@oxide-browser/src/download.rs`:
- Around line 37-41: The percent() method on DownloadProgress can divide by zero
when total_bytes is Some(0); update DownloadProgress::percent to check the
mapped total is non‑zero before performing the division (e.g., map only when
total > 0, otherwise return None or a sensible fallback) so that
bytes_downloaded as f64 / total as f64 is never computed with total == 0.
---
Nitpick comments:
In `@examples/hello-oxide/src/lib.rs`:
- Line 141: Replace the hard-coded mouse-info Y coordinate (525.0) with a
layout-safe value derived from link_y and the canvas height: compute a preferred
Y (e.g., link_y + offset) and clamp it between a top and bottom margin using the
canvas height (e.g., max(margin, min(preferred_y, canvas_height - margin))) so
the mouse info never gets clipped; update the place where the mouse info Y is
set in lib.rs to use this computed value instead of 525.0.
In `@oxide-browser/src/download.rs`:
- Around line 53-63: The impl Default for DownloadManager is placed before the
DownloadManager struct; move the entire impl Default block so it appears after
the DownloadManager struct definition and attach any existing doc comment to the
struct itself; ensure the impl still returns Self::new() and references
DownloadManager, next_id, and downloads unchanged.
- Around line 118-121: The code currently spawns a new OS thread and creates a
fresh tokio::runtime::Runtime for every download (std::thread::spawn { let rt =
tokio::runtime::Runtime::new().unwrap(); rt.block_on(run_download(...)) }),
which is costly; instead create and reuse a single shared runtime (e.g.,
construct one Runtime once via once_cell::sync::Lazy or pass a
tokio::runtime::Handle into the downloader) and replace the per-download
Runtime::new()/block_on with runtime.handle().spawn(async move {
run_download(id, url, dest, downloads).await }) or runtime.spawn(...) so
run_download runs on the shared runtime; update callers to accept or access the
shared Handle/Runtime and remove the per-download Runtime::new() usage.
- Around line 282-289: The loop in unique_path currently uses for i in 1u32..
which is effectively unbounded and makes the final fallback dead code; change it
to a bounded loop (e.g., const MAX_TRIES: u32 = 10_000 or another reasonable
limit) and iterate for i in 1..=MAX_TRIES, constructing try_name and checking
dir.join(&try_name).exists() exactly as before, and if no free name is found
after MAX_TRIES return the original dir.join(name) (or return a Result::Err if
you prefer to surface the failure); update the function unique_path and its use
of stem, ext, dir, and name accordingly so the fallback is reachable.
In `@oxide-browser/src/history.rs`:
- Around line 97-109: The current list_all method collects entries then calls
items.reverse(), which adds an unnecessary O(n) reversal; instead iterate the
sled tree in reverse by using self.tree.iter().flatten().rev() inside list_all
(keep the same key/value handling and HistoryItem::from_bytes check) and push
entries in iteration order so you can remove the items.reverse() call and return
the Vec directly; update the loop that currently destructures (key, val) to work
with the reversed iterator and preserve pushing (key.to_vec(), item).
- Around line 100-106: The current iteration over self.tree uses .flatten(),
which silently drops Errs from the sled iterator; replace that with explicit
matching on each entry from self.tree.iter() (e.g., for entry in
self.tree.iter() { match entry { Ok((key, val)) => { if let Some(item) =
HistoryItem::from_bytes(&val) { items.push((key.to_vec(), item)); } }, Err(e) =>
{ /* log error */ } } }) so that any iteration errors are logged (use the
project's logger, e.g., log::error or the existing logging util) and not
ignored; ensure you reference self.tree.iter() and HistoryItem::from_bytes when
making the change.
In `@oxide-browser/src/runtime.rs`:
- Line 17: The code unnecessarily calls Arc::clone when constructing the stores;
since BookmarkStore::open(&kv_db) and HistoryStore::open(&kv_db) only borrow the
Arc<sled::Db>, remove the redundant Arc::clone and pass kv_db (or &kv_db)
directly into Some(...) instead of cloning; update the places around the
initialization of BookmarkStore and HistoryStore (references to
BookmarkStore::open, HistoryStore::open and the kv_db variable) to eliminate
Arc::clone calls.
In `@oxide-sdk/src/lib.rs`:
- Around line 1-2: The crate currently uses broad crate-level suppressions
(#![allow(clippy::too_many_arguments)] and
#![allow(clippy::doc_overindented_list_items)]); narrow them by removing these
top-level attributes and instead apply #[allow(...)] directly on the specific
FFI module or functions that need them (e.g., the extern "C" block or the
functions that expose many parameters) and apply
#[allow(clippy::doc_overindented_list_items)] to the specific doc-commented item
that contains the table; this keeps the rest of the crate subject to clippy
checks while preserving the suppressions only where necessary.
In `@oxide-sdk/src/proto.rs`:
- Around line 309-310: The current standalone pub fn next(&mut self) ->
Option<ProtoField<'a>> should be converted into a proper Iterator
implementation: implement impl<'a> Iterator for the concrete iterator type that
defines this next method (replace the standalone fn with an impl block), set
type Item = ProtoField<'a>, and move the existing next body into fn next(&mut
self) -> Option<Self::Item>; then remove the clippy
allow(should_implement_trait) and update call sites (e.g., collect_fields) to
use iterator combinators. Ensure the impl uses the same lifetime parameter and
the existing ProtoField<'a> return type.
🪄 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: 9ee7e559-81b1-42bb-abb5-cb0fd06ceabd
⛔ Files ignored due to path filters (4)
oxide-landing/audio_player.wasmis excluded by!**/*.wasmoxide-landing/hello_oxide.wasmis excluded by!**/*.wasmoxide-landing/index.wasmis excluded by!**/*.wasmoxide-landing/timer_demo.wasmis excluded by!**/*.wasm
📒 Files selected for processing (12)
examples/fullstack-notes/backend/src/proto.rsexamples/gpu-graphics-demo/src/lib.rsexamples/hello-oxide/src/lib.rsoxide-browser/Cargo.tomloxide-browser/src/capabilities.rsoxide-browser/src/download.rsoxide-browser/src/history.rsoxide-browser/src/lib.rsoxide-browser/src/runtime.rsoxide-browser/src/ui.rsoxide-sdk/src/lib.rsoxide-sdk/src/proto.rs
Comment on lines
+37
to
+41
| impl DownloadProgress { | ||
| pub fn percent(&self) -> Option<f64> { | ||
| self.total_bytes | ||
| .map(|total| (self.bytes_downloaded as f64 / total as f64) * 100.0) | ||
| } |
There was a problem hiding this comment.
Guard against division by zero in percent().
If total_bytes is Some(0) (e.g., empty file or malformed Content-Length), dividing by zero produces NaN.
Suggested fix
pub fn percent(&self) -> Option<f64> {
self.total_bytes
- .map(|total| (self.bytes_downloaded as f64 / total as f64) * 100.0)
+ .filter(|&total| total > 0)
+ .map(|total| (self.bytes_downloaded as f64 / total as f64) * 100.0)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@oxide-browser/src/download.rs` around lines 37 - 41, The percent() method on
DownloadProgress can divide by zero when total_bytes is Some(0); update
DownloadProgress::percent to check the mapped total is non‑zero before
performing the division (e.g., map only when total > 0, otherwise return None or
a sensible fallback) so that bytes_downloaded as f64 / total as f64 is never
computed with total == 0.
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.
When the browser navigates to a URL whose path has a file extension other than .wasm (e.g. .txt, .zip, .pdf), the file is now downloaded to the system Downloads folder instead of being loaded as a WebAssembly module.
Download manager (oxide-browser/src/download.rs):
Browser UI changes (oxide-browser/src/ui.rs):
Example and landing page:
Also fixes pre-existing clippy warnings across the workspace:
Made-with: Cursor
Summary by CodeRabbit
Release Notes