Seanaye/refactor/bundle updater#4976
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe web build now embeds the bundle build number in 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/tauri/src-tauri/src/tauri_protocol.rs (1)
73-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRewrite bare
/appURIs that include a query string.For
tauri://localhost/app?cache=1,reststarts with?, so embedded delegation retains/appwhile OTA resolution servesindex.html. This can make the embedded entrypoint return 404.Proposed fix and regression test
- if !prefix.ends_with('/') && !rest.is_empty() && !rest.starts_with('/') { + if !prefix.ends_with('/') + && !rest.is_empty() + && !rest.starts_with('/') + && !rest.starts_with('?') + { continue; }+ assert_eq!( + rewrite_uri("tauri://localhost/app?cache=1"), + "tauri://localhost/?cache=1" + );🤖 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 `@apps/web/tauri/src-tauri/src/tauri_protocol.rs` around lines 73 - 80, Update the URI matching logic around the prefix handling in the protocol rewrite function so a bare “/app” with a query string, such as “/app?cache=1”, is treated as the app root rather than rejected. Preserve rejection of path-like suffixes such as “/app.css”, and ensure the rewritten URI delegates to the embedded entrypoint while retaining the query string.
🤖 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 `@apps/web/tauri/macro_bundle_updater_plugin/src/domain/service.rs`:
- Around line 833-840: Update clear_bundle_root so the cleared bundle root is
persisted before mutating self.bundle_root in memory: create or persist the
cleared root independently, then assign it to self.bundle_root only after
persist succeeds, and transition bundle_routes afterward. Preserve the existing
state whenever persistence returns an error.
In `@apps/web/tauri/macro_bundle_updater_plugin/src/inbound/plugin.rs`:
- Around line 367-372: Update the ReloadNeeded handling around webview_windows
and the webview.eval call so the reload dispatch state is also unmarked when no
webview is available. Mirror the None-case behavior implemented by
apply_completed_update_from, while preserving the existing eval-error warning
and reset behavior.
In `@apps/web/tauri/src-tauri/src/tauri_protocol.rs`:
- Around line 156-160: Update the asset-resolution flow around the timeout on
resolver.resolve so the read lease remains held until the subsequent
tokio::fs::read operation completes, including when resolution times out. Ensure
finish_transition cannot acquire the write lease and clean up the OTA directory
before the in-flight read finishes.
---
Outside diff comments:
In `@apps/web/tauri/src-tauri/src/tauri_protocol.rs`:
- Around line 73-80: Update the URI matching logic around the prefix handling in
the protocol rewrite function so a bare “/app” with a query string, such as
“/app?cache=1”, is treated as the app root rather than rejected. Preserve
rejection of path-like suffixes such as “/app.css”, and ensure the rewritten URI
delegates to the embedded entrypoint while retaining the query string.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6dfe52a-8d1e-4d0d-a5b2-5aea3404bb0c
📒 Files selected for processing (22)
apps/web/.helix/languages.tomlapps/web/index.htmlapps/web/scripts/write-bundle-manifest.mjsapps/web/src/lib/tauri/TauriProvider.tsxapps/web/tauri/macro_bundle_updater_plugin/src/domain.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service/test.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/bundle_routes.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/bundle_routes/test.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/models.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/ports.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/service.rsapps/web/tauri/macro_bundle_updater_plugin/src/domain/service/reload_test.rsapps/web/tauri/macro_bundle_updater_plugin/src/inbound/plugin.rsapps/web/tauri/macro_bundle_updater_plugin/src/outbound.rsapps/web/tauri/macro_bundle_updater_plugin/src/outbound/fs.rsapps/web/tauri/macro_bundle_updater_plugin/src/outbound/runtime.rsapps/web/tauri/macro_bundle_updater_plugin/src/outbound/runtime/test.rsapps/web/tauri/macro_bundle_updater_plugin/src/outbound/system_info.rsapps/web/tauri/src-tauri/src/lib.rsapps/web/tauri/src-tauri/src/tauri_protocol.rsapps/web/tauri/src-tauri/src/tauri_protocol/test.rs
| async fn clear_bundle_root(&mut self, cache_dir: &Path) -> Result<(), std::io::Error> { | ||
| self.clear_pending_bundle(cache_dir).await?; | ||
| self.bundle_root.clear(); | ||
| self.bundle_root.persist(cache_dir, &self.fs_repo).await | ||
| self.bundle_root.persist(cache_dir, &self.fs_repo).await?; | ||
| self.bundle_routes | ||
| .transition_to(BundleSource::embedded(self.embedded_bundle_build)) | ||
| .await; | ||
| Ok(()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Persist the cleared root before mutating in-memory state.
If persistence fails at Line 836, bundle_root remains cleared while bundle_routes still points to the OTA bundle. Preserve the old state until persistence succeeds.
Proposed fix
async fn clear_bundle_root(&mut self, cache_dir: &Path) -> Result<(), std::io::Error> {
self.clear_pending_bundle(cache_dir).await?;
- self.bundle_root.clear();
- self.bundle_root.persist(cache_dir, &self.fs_repo).await?;
+ let cleared_root = BundleRoot::new();
+ cleared_root.persist(cache_dir, &self.fs_repo).await?;
+ self.bundle_root = cleared_root;
self.bundle_routes
.transition_to(BundleSource::embedded(self.embedded_bundle_build))
.await;
Ok(())
}📝 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.
| async fn clear_bundle_root(&mut self, cache_dir: &Path) -> Result<(), std::io::Error> { | |
| self.clear_pending_bundle(cache_dir).await?; | |
| self.bundle_root.clear(); | |
| self.bundle_root.persist(cache_dir, &self.fs_repo).await | |
| self.bundle_root.persist(cache_dir, &self.fs_repo).await?; | |
| self.bundle_routes | |
| .transition_to(BundleSource::embedded(self.embedded_bundle_build)) | |
| .await; | |
| Ok(()) | |
| async fn clear_bundle_root(&mut self, cache_dir: &Path) -> Result<(), std::io::Error> { | |
| self.clear_pending_bundle(cache_dir).await?; | |
| let cleared_root = BundleRoot::new(); | |
| cleared_root.persist(cache_dir, &self.fs_repo).await?; | |
| self.bundle_root = cleared_root; | |
| self.bundle_routes | |
| .transition_to(BundleSource::embedded(self.embedded_bundle_build)) | |
| .await; | |
| Ok(()) |
🤖 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 `@apps/web/tauri/macro_bundle_updater_plugin/src/domain/service.rs` around
lines 833 - 840, Update clear_bundle_root so the cleared bundle root is
persisted before mutating self.bundle_root in memory: create or persist the
cleared root independently, then assign it to self.bundle_root only after
persist succeeds, and transition bundle_routes afterward. Preserve the existing
state whenever persistence returns an error.
| if apply_result == ApplyUpdateResult::ReloadNeeded | ||
| && let Some(webview) = app_handle.webview_windows().values().next() | ||
| && let Err(error) = webview.eval("window.location.reload();") | ||
| { | ||
| tracing::warn!(error=?error, "[bundle-update] failed to reload after clearing bundle"); | ||
| service.lock().await.unmark_update_reload_dispatched(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset reload dispatch when no webview is available.
The let-chain only unmarks on eval failure. If no webview exists, no reload is sent but retry state remains suppressed. Mirror apply_completed_update_from’s None handling.
Proposed fix
- if apply_result == ApplyUpdateResult::ReloadNeeded
- && let Some(webview) = app_handle.webview_windows().values().next()
- && let Err(error) = webview.eval("window.location.reload();")
- {
- tracing::warn!(error=?error, "[bundle-update] failed to reload after clearing bundle");
- service.lock().await.unmark_update_reload_dispatched();
+ if apply_result == ApplyUpdateResult::ReloadNeeded {
+ if let Some(webview) = app_handle.webview_windows().values().next() {
+ if let Err(error) = webview.eval("window.location.reload();") {
+ tracing::warn!(error=?error, "[bundle-update] failed to reload after clearing bundle");
+ service.lock().await.unmark_update_reload_dispatched();
+ }
+ } else {
+ tracing::warn!("[bundle-update] no webview available after clearing bundle");
+ service.lock().await.unmark_update_reload_dispatched();
+ }
}📝 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.
| if apply_result == ApplyUpdateResult::ReloadNeeded | |
| && let Some(webview) = app_handle.webview_windows().values().next() | |
| && let Err(error) = webview.eval("window.location.reload();") | |
| { | |
| tracing::warn!(error=?error, "[bundle-update] failed to reload after clearing bundle"); | |
| service.lock().await.unmark_update_reload_dispatched(); | |
| if apply_result == ApplyUpdateResult::ReloadNeeded { | |
| if let Some(webview) = app_handle.webview_windows().values().next() { | |
| if let Err(error) = webview.eval("window.location.reload();") { | |
| tracing::warn!(error=?error, "[bundle-update] failed to reload after clearing bundle"); | |
| service.lock().await.unmark_update_reload_dispatched(); | |
| } | |
| } else { | |
| tracing::warn!("[bundle-update] no webview available after clearing bundle"); | |
| service.lock().await.unmark_update_reload_dispatched(); | |
| } | |
| } |
🤖 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 `@apps/web/tauri/macro_bundle_updater_plugin/src/inbound/plugin.rs` around
lines 367 - 372, Update the ReloadNeeded handling around webview_windows and the
webview.eval call so the reload dispatch state is also unmarked when no webview
is available. Mirror the None-case behavior implemented by
apply_completed_update_from, while preserving the existing eval-error warning
and reset behavior.
| let resolution = match tokio::time::timeout( | ||
| ASSET_RESOLUTION_TIMEOUT, | ||
| resolver.resolve(&asset_path), | ||
| ) | ||
| .await |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'tokio::time::timeout|resolver\.resolve' \
apps/web/tauri/src-tauri/src/tauri_protocol.rs
rg -n -C4 'tokio::fs::read|read_owned|finish_transition' \
apps/web/tauri/macro_bundle_updater_plugin/src/{outbound/fs.rs,domain/bundle_routes.rs}
lockfile="$(fd '^Cargo\.lock$' . | head -n1)"
rg -n -A4 -B2 'name = "tokio"' "$lockfile"
curl -fsSL https://docs.rs/tokio/1.47.1/tokio/fs/index.html |
rg -i -m2 'spawn_blocking|blocking'
curl -fsSL https://docs.rs/tokio/1.47.1/tokio/task/fn.spawn_blocking.html |
rg -i -m2 'cannot be aborted|not possible to cancel'Repository: macro-inc/macro
Length of output: 5674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source slices with line numbers.
sed -n '1,220p' apps/web/tauri/src-tauri/src/tauri_protocol.rs
printf '\n--- bundle_routes.rs ---\n'
sed -n '1,220p' apps/web/tauri/macro_bundle_updater_plugin/src/domain/bundle_routes.rs
printf '\n--- outbound/fs.rs ---\n'
sed -n '1,180p' apps/web/tauri/macro_bundle_updater_plugin/src/outbound/fs.rs
# Look for the resolver implementation and any comments about lease lifetime.
rg -n -C3 'resolve\(|read_owned|finish_transition|read lease|write lease|tokio::fs::read' apps/web/tauri/macro_bundle_updater_plugin/srcRepository: macro-inc/macro
Length of output: 28482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Tokio docs/source semantics for timeout cancellation and fs::read.
python3 - <<'PY'
import urllib.request, re
urls = {
"timeout": "https://docs.rs/tokio/1.47.1/tokio/time/fn.timeout.html",
"fs_read": "https://docs.rs/tokio/1.47.1/tokio/fs/fn.read.html",
"spawn_blocking": "https://docs.rs/tokio/1.47.1/tokio/task/fn.spawn_blocking.html",
}
for name, url in urls.items():
print(f"\n=== {name} ===")
html = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
for pat in [
r'(?i)canceled[^<]{0,160}',
r'(?i)drop[^<]{0,160}',
r'(?i)blocking[^<]{0,160}',
r'(?i)cancel[^<]{0,160}',
r'(?i)not possible to cancel[^<]{0,160}',
]:
m = re.search(pat, html)
if m:
s = re.sub(r'<[^>]+>', ' ', m.group(0))
print(s.strip())
break
PYRepository: macro-inc/macro
Length of output: 2765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the tokio dependency version from Cargo.lock and inspect the source docs if available.
lockfile="$(fd '^Cargo\.lock$' . | head -n1)"
python3 - <<'PY' "$lockfile"
import sys, re
path = sys.argv[1]
data = open(path, 'r', encoding='utf-8').read()
m = re.search(r'\[\[package\]\]\nname = "tokio"\nversion = "([^"]+)"', data)
print(m.group(1) if m else "tokio-version-not-found")
PYRepository: macro-inc/macro
Length of output: 160
🌐 Web query:
Tokio 1.47 tokio::fs::read cancellation behavior drop future spawn_blocking docs
💡 Result:
In Tokio 1.47, the function tokio::fs::read is implemented by offloading the file I/O operation to a separate thread pool using spawn_blocking [1][2]. When you await the future returned by tokio::fs::read, you are awaiting a task running on the blocking thread pool [1][2]. Because spawn_blocking tasks perform synchronous, blocking operations, they are not cancellation-aware in the sense that they cannot be interrupted once they have started executing [3][4]. Key details regarding the cancellation behavior include: 1. Future Cancellation: If you drop the future returned by tokio::fs::read before it completes, the future itself is dropped [5]. However, this only cancels the interface to the task; it does not stop the underlying blocking thread from executing the I/O operation [3][4]. The operation will continue to run to completion in the background on the spawn_blocking thread pool [3][4]. 2. Non-Abortable: Unlike tasks spawned via tokio::spawn, which yield to the runtime at.await points and can be aborted, spawn_blocking tasks cannot be aborted once they have started [3][4]. 3. Resource Impact: Because the underlying thread is blocked until the I/O completes, the thread cannot be reused by the runtime until the operation finishes, even if you have dropped the future in your async code [3][4]. In summary, dropping the future returned by tokio::fs::read does not stop the I/O operation from occurring; it merely discards the result of that operation once the blocking thread eventually finishes the work and returns [3][4].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/fs/fn.read.html
- 2: https://github.com/tokio-rs/tokio/blob/master/tokio/src/fs/read.rs
- 3: https://docs.rs/tokio/latest/tokio/task/fn.spawn_blocking.html
- 4: https://docs.rs/tokio/latest/tokio/task/
- 5: https://learnrust.github.io/comprehensive-rust/async/pitfalls/cancellation.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the resolver implementation and its tests.
sed -n '1,220p' apps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service.rs
printf '\n--- asset_service tests ---\n'
sed -n '1,220p' apps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service/test.rs
# Confirm how read leases are acquired and held in the resolver path.
rg -n -C4 'routes\.read|read_owned|read_asset|fallback|tokio::fs::read' \
apps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service.rs \
apps/web/tauri/macro_bundle_updater_plugin/src/domain/asset_service/test.rsRepository: macro-inc/macro
Length of output: 17076
Keep the lease alive until the read finishes. timeout(...) drops resolver.resolve() on expiry, which releases the read guard before the blocking tokio::fs::read has completed. finish_transition() can then acquire the write lease and clean up the OTA directory while that I/O is still in flight.
🤖 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 `@apps/web/tauri/src-tauri/src/tauri_protocol.rs` around lines 156 - 160,
Update the asset-resolution flow around the timeout on resolver.resolve so the
read lease remains held until the subsequent tokio::fs::read operation
completes, including when resolution times out. Ensure finish_transition cannot
acquire the write lease and clean up the OTA directory before the in-flight read
finishes.
fix(web): resolve rust-analyzer project from Tauri root
f39d568 to
2762811
Compare
|
Confirmed this works in the simulator by running the native_app_server |
1 participant
This PR refactors the bundle updater plugin to have cleaner hex boundaries and correctly causes incoming asset requests to wait for the updater plugin to release any pending locks before serving assets.
Previously this allowed for a race condition where if you tried to load an asset while files were being patched, the backend would send an error. This is an issue for the frontend because there is no retry logic for reloading things like .js assets.