refactor: improve watch mode - #9859
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughThis pull request refactors the file watcher infrastructure in the Biome CLI. It introduces a trait-based watcher abstraction with improved error handling by replacing a simple event struct with an enum distinguishing between Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/biome_cli/src/runner/diagnostics.rs (1)
152-155: Consider tightening visibility onWatcherDiagnostic.Given
runneris crate-private,pub(crate)here would better signal intent and avoid accidental API creep.Small visibility tidy-up
-pub struct WatcherDiagnostic { +pub(crate) struct WatcherDiagnostic { #[source] pub(crate) source: Option<Error>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/src/runner/diagnostics.rs` around lines 152 - 155, WatcherDiagnostic is declared public but the runner module is crate-private; change its visibility to pub(crate) and make its field source also pub(crate) (or keep it private if not accessed outside the crate) to avoid API creep—locate the struct WatcherDiagnostic and update its declaration and the #[source] pub(crate) source: Option<Error> visibility accordingly so the type and its source field are crate-scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_cli/src/runner/impls/watchers/default.rs`:
- Around line 17-21: The constructor new() currently calls
recommended_watcher(tx).expect("watcher created") which panics on failure;
change new() to return a Result<Self, WatcherError> (or the runner's existing
error type) and propagate the error from recommended_watcher instead of
unwrapping, e.g. call recommended_watcher(tx)? and map or convert its error into
the runner/watcher error plumbing; update the type of new, and the caller(s)
that construct DefaultWatcher to handle the Result so the watcher creation error
is surfaced as a diagnostic rather than panicking.
In `@crates/biome_cli/src/runner/watcher.rs`:
- Around line 9-11: The Watcher trait's watch registration can fail but
currently returns ()—change the signature of fn watch(&mut self, paths:
Vec<Utf8PathBuf>) to return a Result (e.g., Result<(), WatchError> or
anyhow::Result<()>) so implementations can surface registration errors; update
all impls of Watcher (and any callers) to propagate or handle that Result, and
ensure the CLI checks the returned Result from watch() and handles failures
before calling or printing the watch banner and before starting poll() (or
calling fn poll(&mut self) if present).
In `@crates/biome_cli/tests/cases/watcher.rs`:
- Around line 21-25: The current test only produces WatcherEvent::Changed and
never exercises the new WatcherEvent::Error branch; add a small companion test
(or extend the existing one) that sets the seed to include
WatcherEvent::Error(...) (using MockWatcher::with_events via the existing
watcher_factory pattern) so the runner's error handling path is hit, then assert
the diagnostic/error message is emitted to the console capture used by the test
(verify the diagnostic string that runner/mod.rs emits). Ensure the test uses
the same Mutex/seed/watcher_factory setup and asserts the expected console
output for the error event.
---
Nitpick comments:
In `@crates/biome_cli/src/runner/diagnostics.rs`:
- Around line 152-155: WatcherDiagnostic is declared public but the runner
module is crate-private; change its visibility to pub(crate) and make its field
source also pub(crate) (or keep it private if not accessed outside the crate) to
avoid API creep—locate the struct WatcherDiagnostic and update its declaration
and the #[source] pub(crate) source: Option<Error> visibility accordingly so the
type and its source field are crate-scoped.
🪄 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: ba534f41-8a90-4d3f-8c56-a3524e9d698b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**
📒 Files selected for processing (16)
crates/biome_cli/Cargo.tomlcrates/biome_cli/src/lib.rscrates/biome_cli/src/runner/diagnostics.rscrates/biome_cli/src/runner/impls/commands/custom_execution.rscrates/biome_cli/src/runner/impls/commands/traversal.rscrates/biome_cli/src/runner/impls/watchers/default.rscrates/biome_cli/src/runner/impls/watchers/mock.rscrates/biome_cli/src/runner/impls/watchers/mod.rscrates/biome_cli/src/runner/mod.rscrates/biome_cli/src/runner/watcher.rscrates/biome_cli/tests/cases/mod.rscrates/biome_cli/tests/cases/watcher.rscrates/biome_cli/tests/main.rscrates/biome_diagnostics/Cargo.tomlcrates/biome_diagnostics/src/adapters.rscrates/biome_diagnostics/src/lib.rs
💤 Files with no reviewable changes (2)
- crates/biome_cli/src/runner/impls/commands/custom_execution.rs
- crates/biome_cli/src/runner/impls/commands/traversal.rs
| pub trait Watcher { | ||
| /// Start watching file changes under the paths recursively. | ||
| fn watch(&mut self, paths: impl IntoIterator<Item = Utf8PathBuf>); | ||
| fn watch(&mut self, paths: Vec<Utf8PathBuf>); |
There was a problem hiding this comment.
watch() needs a synchronous error path.
Registering the watch roots can fail before poll() is ever called. With fn watch(&mut self, ...) returning (), implementations have nowhere to surface that, so the CLI can carry on into watch mode after registration already failed. Please return a Result here and handle it before printing the watch banner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_cli/src/runner/watcher.rs` around lines 9 - 11, The Watcher
trait's watch registration can fail but currently returns ()—change the
signature of fn watch(&mut self, paths: Vec<Utf8PathBuf>) to return a Result
(e.g., Result<(), WatchError> or anyhow::Result<()>) so implementations can
surface registration errors; update all impls of Watcher (and any callers) to
propagate or handle that Result, and ensure the CLI checks the returned Result
from watch() and handles failures before calling or printing the watch banner
and before starting poll() (or calling fn poll(&mut self) if present).
| let seed = Mutex::new(Some(vec![WatcherEvent::Changed(vec![bad_path.clone()])])); | ||
| let watcher_factory = Box::new(move || { | ||
| let events = seed.lock().unwrap().take().unwrap_or_default(); | ||
| Box::new(MockWatcher::with_events(events)) as Box<dyn biome_cli::Watcher> | ||
| }); |
There was a problem hiding this comment.
Please exercise the error branch too.
This only scripts WatcherEvent::Changed(...), so the new WatcherEvent::Error(...) path in crates/biome_cli/src/runner/mod.rs is still unpinned. A tiny companion test that injects one error event and asserts the diagnostic reaches the console would close the gap nicely. As per coding guidelines, "All code changes MUST include appropriate tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_cli/tests/cases/watcher.rs` around lines 21 - 25, The current
test only produces WatcherEvent::Changed and never exercises the new
WatcherEvent::Error branch; add a small companion test (or extend the existing
one) that sets the seed to include WatcherEvent::Error(...) (using
MockWatcher::with_events via the existing watcher_factory pattern) so the
runner's error handling path is hit, then assert the diagnostic/error message is
emitted to the console capture used by the test (verify the diagnostic string
that runner/mod.rs emits). Ensure the test uses the same
Mutex/seed/watcher_factory setup and asserts the expected console output for the
error event.
Summary
This PR improves the recently merged watcher mode by making it testable and predictable.
It also improves error handling by capturing possible errors emitted during the polling
Test Plan
Added a new test
Docs