fix: watch cwds of dependencies#10054
Conversation
When I changed watch mode to also watch dependencies' sources, I didn't
take into account the fact that the `sources` will be used _as-is_ (i.e.
not resolved to anything).
This meant the following would happen:
- `build:a` has `["src/*.ts"]`
- `build:b` has `["lib/*.js"]` and depends on `build:a`
- We pass `["src/*.ts", "lib/*.js"]` to watchexec _in the directory of
`build:b`_
This made `lib/*.js` basically no-op, or worse, watch the wrong files.
This change resolves the paths to their owning directory so we end up
with `["wherever-builda-lives/src/*.ts",
"wherever-buildb-lives/lib/*.js"]`.
**Notable Changes:**
- Instead of passing relative globs to watchexec, we now pass resolved
ones (relative to the root)
- We now pass `--project-origin` to watchexec which comes with some perf
gains but also means globs are now relative to it
- We pass `--watch {cwd}` for the cwd of each dependency
- This new `resolve_source` function is basically turning a source glob
into a relative-to-the-root glob while retaining negations
There was a problem hiding this comment.
Code Review
This pull request introduces logic to calculate a "filter anchor" for watchexec by determining the common ancestor of task working directories, ensuring glob filters are correctly interpreted relative to a project origin. It adds utility functions for finding common path ancestors and resolving source patterns, including support for negations and escaped characters. Review feedback focused on optimizing the common_ancestor function by using AsRef to avoid unnecessary cloning and reducing memory allocations during path component comparisons.
Greptile SummaryThis PR fixes a regression introduced when watch mode was extended to track dependency sources: source globs were passed verbatim to watchexec in the wrong working directory, causing out-of-cwd patterns like
Confidence Score: 4/5Safe to merge; the core path-resolution logic is correct and well unit-tested, and the one minor gap does not affect correctness. The anchor computation correctly chains both task cwds and resolved source absolute paths into common_ancestor, so --project-origin always covers out-of-cwd sources. The watch_dirs coverage check has a one-sided heuristic that can produce redundant --watch arguments for source dirs that are parents of a task cwd, but this is harmless for correctness. The watch_dirs coverage check in src/cli/watch.rs around line 259 could be tightened to also skip directories that already cover a watched cwd. Important Files Changed
Reviews (9): Last reviewed commit: "chore: formatting" | Re-trigger Greptile |
|
This mostly looks good, but I think there is one missed case before merge. When a source escapes the task cwd, the filter anchor is widened correctly, but the watched paths are still only the task cwds. For example: [tasks.build]
dir = "packages/foo"
sources = ["../../shared/src/*.ts"]This becomes roughly: The filter now points at Could we either watch the computed anchor/common ancestor, or add watch dirs for source paths that fall outside their task cwd? A regression test for This comment was generated by an AI coding assistant. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes ChangesWatch filter anchoring for escaped sources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
i've added logic to do the latter - basically slice the source dirs upto the first glob character and watch that. so and i added an e2e test similar to the existing ones but with a |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e/tasks/test_task_watch_cwd_escape`:
- Around line 38-44: Replace the fixed "sleep 2" delay with the same bounded
polling loop used earlier in this test: repeatedly check for the "built" marker
in LOG_FILE (using grep -q "built" "$LOG_FILE") with a short interval and a
total timeout, and call fail with the current log contents if the timeout
elapses; remove the literal sleep 2, use the same poll/timeout variables and
loop structure used above in this file to ensure deterministic wait behavior for
the rerun assertion.
🪄 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 Plus
Run ID: 605b48de-4cb7-4a5d-ab74-44941b5b438f
📒 Files selected for processing (2)
e2e/tasks/test_task_watch_cwd_escapesrc/cli/watch.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
@jdx this should be good to go now 🎉 |
When I changed watch mode to also watch dependencies' sources, I didn't
take into account the fact that the
sourceswill be used as-is (i.e.not resolved to anything).
This meant the following would happen:
build:ahas["src/*.ts"]build:bhas["lib/*.js"]and depends onbuild:a["src/*.ts", "lib/*.js"]to watchexec in the directory ofbuild:bThis made
lib/*.jsbasically no-op, or worse, watch the wrong files.This change resolves the paths to their owning directory so we end up
with
["wherever-builda-lives/src/*.ts", "wherever-buildb-lives/lib/*.js"].Notable Changes:
ones (relative to the root)
--project-originto watchexec which comes with some perfgains but also means globs are now relative to it
--watch {cwd}for the cwd of each dependencyresolve_sourcefunction is basically turning a source globinto a relative-to-the-root glob while retaining negations
common_ancestorfunction tries to find the common ancestor of two directoriescc @jdx i don't usually delve into rust so i'd love some help here if you can.
especially if there's a better way to find the common ancestor, or if you think we should deal with that differently.
basically, watchexec doesn't seem to support absolute paths, so we have to set the
project-originto something above all globs. which is why this new function tries to find a common root (if not the configured one)Summary by CodeRabbit
Bug Fixes
Improvements
Tests