fix: middle-click forward-navigation on Windows + sd-archive unused warnings#3068
fix: middle-click forward-navigation on Windows + sd-archive unused warnings#3068alfinaloshi wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a background stderr reader task for the script adapter; applies several small Rust import/variable cleanups; and registers a platform-gated document Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| return null; | ||
| }, [params.locationId, locationsQuery.data, currentPath]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This is a global document handler; it will also affect the web build (and non-Windows desktop) by disabling middle-click default behavior everywhere. Might be worth gating to Tauri+Windows only.
| useEffect(() => { | |
| useEffect(() => { | |
| if (platform.platform !== 'tauri' || !navigator.userAgent.includes('Windows')) return; | |
| // Prevent WebView2 (Windows) from entering autoscroll mode on middle-click, | |
| // which causes forward-navigation after browser history traversal (#3008). | |
| const preventMiddleClickScroll = (e: MouseEvent) => { | |
| if (e.button === 1) e.preventDefault(); | |
| }; | |
| document.addEventListener('mousedown', preventMiddleClickScroll, { passive: false }); | |
| return () => document.removeEventListener('mousedown', preventMiddleClickScroll); | |
| }, [platform.platform]); |
There was a problem hiding this comment.
made suggested changes and updated
|
|
||
| if let Some(ref temp) = temporal { | ||
| if let Some(date_field) = &self.schema.search.date_field { | ||
| if self.schema.search.date_field.is_some() { |
There was a problem hiding this comment.
Minor, but is_some() + unwrap() here is a bit brittle/less idiomatic; if let keeps it simple and avoids accidental unwraps if this block ever gets edited.
| if self.schema.search.date_field.is_some() { | |
| if let Some(ref temp) = temporal { | |
| if self.schema.search.date_field.is_some() { | |
| if let Some(after) = temp.date_after { | |
| q = q.bind(after); | |
| } | |
| if let Some(before) = temp.date_before { | |
| q = q.bind(before); | |
| } | |
| } | |
| } |
| .take() | ||
| .ok_or_else(|| Error::AdapterSync("failed to open stdout".into()))?; | ||
| let stderr = child | ||
| let _stderr = child |
There was a problem hiding this comment.
Even if you don't need stderr for parsing, dropping it without draining can deadlock the child if it writes enough to fill the pipe buffer. Logging/draining it in the background avoids that and makes adapter failures easier to diagnose.
| let _stderr = child | |
| let stderr = child | |
| .stderr | |
| .take() | |
| .ok_or_else(|| Error::AdapterSync("failed to open stderr".into()))?; | |
| // Best-effort: drain stderr so the child can't block on a full pipe. | |
| tokio::spawn(async move { | |
| let mut err_reader = BufReader::new(stderr).lines(); | |
| while let Ok(Some(line)) = err_reader.next_line().await { | |
| tracing::warn!(line = %line, "adapter stderr"); | |
| } | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/archive/src/adapter/script.rs (1)
467-483:⚠️ Potential issue | 🟠 MajorUse
tokio::process::Commandfor async subprocess management or explicitly discard stderr to avoid deadlock.This code uses
std::process::Commandin an async context and pipes stderr without consuming it. The piped stderr handle is taken but dropped immediately (renamed to_stderrto suppress the warning). If the adapter writes to stderr and fills the pipe buffer, the child process will block—while the parent is blocked reading stdout—causing a deadlock.Either:
- Switch to
tokio::process::Command(preferred for async code per guidelines), or- Explicitly discard stderr with
Stdio::null()if stderr is not needed, or- Spawn a background task to drain stderr continuously.
The
_stderrrename only hides the compiler warning without fixing the underlying reliability issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/archive/src/adapter/script.rs` around lines 467 - 483, The current spawn block in script.rs creates a std::process::Command child, pipes stderr and then immediately drops it (see child, stdin, stdout, _stderr and the Error::AdapterSync errors), which can deadlock in async contexts; fix by using tokio::process::Command for async subprocess management (replace std::process::Command usage with tokio::process::Command and await/handle the child via tokio APIs) or, if stderr is not needed, set .stderr(Stdio::null()) when building the command, or alternatively keep stderr piped but immediately detach a tokio task to continuously drain the child's stderr stream (spawn a background tokio::task::spawn that reads from the stderr handle) and only then take stdin/stdout as done now to avoid blocking; ensure Error::AdapterSync messages remain for failure cases.
🧹 Nitpick comments (2)
crates/archive/src/db.rs (2)
589-597: LGTM! Clearer without unused variable.The change from
if let Some(date_field)tois_some()is appropriate since thedate_fieldvariable isn't used in this block.Minor note: Lines 590-595 use the
is_some()followed byunwrap()pattern. While this works, it could be slightly safer to useif let Some(value) = temp.date_afterto avoid the unwrap, but this pre-existed in the codebase and isn't introduced by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/archive/src/db.rs` around lines 589 - 597, The current block checks self.schema.search.date_field.is_some() and then uses is_some() + unwrap() on temp.date_after/temp.date_before before calling q = q.bind(...); replace those is_some() + unwrap() pairs with idiomatic if let Some(value) = temp.date_after and if let Some(value) = temp.date_before so you bind value directly (calling q.bind(value)) and avoid unwrap; keep the outer check for self.schema.search.date_field as-is and preserve the q = q.bind(...) calls and variable names.
575-580: Consider usingis_some()for consistency.The pattern
if let Some(_after)is valid but slightly unusual when the value isn't used—you're destructuring just to check existence. For consistency with the binding block below (line 589) and to make the intent clearer, consider usingtemp.date_after.is_some():♻️ Optional refactor for consistency
if let Some(ref temp) = temporal { if let Some(date_field) = &self.schema.search.date_field { - if let Some(_after) = temp.date_after { + if temp.date_after.is_some() { sql.push_str(&format!(" AND t.\"{}\" >= ?", date_field)); } - if let Some(_before) = temp.date_before { + if temp.date_before.is_some() { sql.push_str(&format!(" AND t.\"{}\" <= ?", date_field)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/archive/src/db.rs` around lines 575 - 580, The code uses `if let Some(_after) = temp.date_after` and `if let Some(_before) = temp.date_before` solely to check existence; change these to `if temp.date_after.is_some()` and `if temp.date_before.is_some()` to match the binding style used later and make intent clearer; keep the same `sql.push_str(&format!(...))` calls and ensure no other behavior changes in the block that constructs the SQL condition strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/archive/src/adapter/script.rs`:
- Around line 467-483: The current spawn block in script.rs creates a
std::process::Command child, pipes stderr and then immediately drops it (see
child, stdin, stdout, _stderr and the Error::AdapterSync errors), which can
deadlock in async contexts; fix by using tokio::process::Command for async
subprocess management (replace std::process::Command usage with
tokio::process::Command and await/handle the child via tokio APIs) or, if stderr
is not needed, set .stderr(Stdio::null()) when building the command, or
alternatively keep stderr piped but immediately detach a tokio task to
continuously drain the child's stderr stream (spawn a background
tokio::task::spawn that reads from the stderr handle) and only then take
stdin/stdout as done now to avoid blocking; ensure Error::AdapterSync messages
remain for failure cases.
---
Nitpick comments:
In `@crates/archive/src/db.rs`:
- Around line 589-597: The current block checks
self.schema.search.date_field.is_some() and then uses is_some() + unwrap() on
temp.date_after/temp.date_before before calling q = q.bind(...); replace those
is_some() + unwrap() pairs with idiomatic if let Some(value) = temp.date_after
and if let Some(value) = temp.date_before so you bind value directly (calling
q.bind(value)) and avoid unwrap; keep the outer check for
self.schema.search.date_field as-is and preserve the q = q.bind(...) calls and
variable names.
- Around line 575-580: The code uses `if let Some(_after) = temp.date_after` and
`if let Some(_before) = temp.date_before` solely to check existence; change
these to `if temp.date_after.is_some()` and `if temp.date_before.is_some()` to
match the binding style used later and make intent clearer; keep the same
`sql.push_str(&format!(...))` calls and ensure no other behavior changes in the
block that constructs the SQL condition strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2287d8-03bd-4aa7-8be3-298d5841f0b4
📒 Files selected for processing (8)
crates/archive/src/adapter/script.rscrates/archive/src/db.rscrates/archive/src/engine.rscrates/archive/src/schema/migration.rscrates/archive/src/search/router.rscrates/archive/src/search/vector.rscrates/archive/src/source.rspackages/interface/src/ShellLayout.tsx
💤 Files with no reviewable changes (1)
- crates/archive/src/search/vector.rs
…pacedriveapp#3008) WebView2 enters autoscroll mode on middle-mousedown. After navigating forward then back, a second middle-click exits scroll mode via the browser forward-navigation gesture instead, causing unintended routing. Gate to Tauri+Windows only. Prevent default on button-1 mousedown with {passive: false} — disables WebView2 autoscroll mode, which is not useful in a file manager. Fixes spacedriveapp#3008
…nings Cleans up 15 compiler warnings in sd-archive: Unused imports removed: - std::pin::Pin (script.rs) - RelationsDef (db.rs) - ConfigField, AdapterUpdateResult (engine.rs — used via full path) - HashMap, Error, Result, FieldType, ModelDef (migration.rs) - FtsHit (search/router.rs) - EMBEDDING_DIM (search/vector.rs) - Path (source.rs — only PathBuf used) Unused variables silenced: - _stderr in script.rs (stderr handle intentionally discarded) - _after/_before in db.rs (presence checked via is_some later) - date_field binding replaced with .is_some() check in db.rs - name: _ in source.rs AddTable match arm
1 participant
Summary
fix(interface): Middle-clicking after browser history traversal on Windows caused unintended forward-navigation. WebView2 enters autoscroll mode on
mousedownbutton-1; navigating back leaves a stale forward-history entry that the next middle-click triggers. Fixed by blockingmousedownbutton-1 default globally inShellLayoutContent. Closes Middle-click causes forward-navigation #3008chore(archive): Removed 15 compiler warnings (
unused_imports,unused_variables) across 7 files in thesd-archivecrate.Test plan
cargo check -p sd-archiveproduces zero unused warningsFiles changed
packages/interface/src/ShellLayout.tsxcrates/archive/src/adapter/script.rscrates/archive/src/db.rscrates/archive/src/engine.rscrates/archive/src/schema/migration.rscrates/archive/src/search/router.rscrates/archive/src/search/vector.rscrates/archive/src/source.rsNote
Two focused changes: (1) Fixes a Windows-specific middle-click bug in the shell layout by preventing WebView2's autoscroll mode, which was interfering with browser navigation history. (2) Cleans up 15 unused compiler warnings across the archive crate—mostly unused imports and variables in database, adapter, and search modules.
Written by Tembo for commit 7f5123c. This will update automatically on new commits.