feat(watcher): add support for excluding files from watch patterns #22767
Conversation
WalkthroughAdds watch exclusion support via glob patterns. Introduces InitConfig with exclude_patterns to Watcher and updates init signature. Propagates excludes through VM, CLI, bunfig, hot reloader, dev server, and node path watcher. Merges CLI and bunfig excludes. Adds tests validating ignored changes and reload behavior. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bunfig.zig (1)
215-244: watch.excludes not parsed for TestCommand (bun test --watch)This block only runs for Run/Auto, so bunfig.toml excludes won’t populate ctx.debug.watch_excludes for TestCommand. That contradicts the PR goal “Feature works with test --watch”.
Apply this diff to include TestCommand:
- if (comptime cmd == .RunCommand or cmd == .AutoCommand) { + if (comptime cmd == .RunCommand or cmd == .AutoCommand or cmd == .TestCommand) { if (json.get("watch")) |watch| { if (watch.get("excludes")) |expr| {Optional: trim and de-duplicate patterns to reduce matcher work at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/Watcher.zig(6 hunks)src/bake/DevServer.zig(1 hunks)src/bun.js.zig(1 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/hot_reloader.zig(4 hunks)src/bun.js/node/path_watcher.zig(1 hunks)src/bunfig.zig(1 hunks)src/cli.zig(1 hunks)src/cli/Arguments.zig(2 hunks)src/cli/test_command.zig(1 hunks)test/cli/watch/watch-excludes.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/VirtualMachine.zigsrc/bun.js.zigsrc/bun.js/node/path_watcher.zigsrc/cli/test_command.zigsrc/cli.zigsrc/bunfig.zigsrc/Watcher.zigsrc/bun.js/hot_reloader.zigsrc/bake/DevServer.zigsrc/cli/Arguments.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zigsrc/bun.js/node/path_watcher.zigsrc/bun.js/hot_reloader.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/VirtualMachine.zigsrc/bun.js.zigsrc/bun.js/node/path_watcher.zigsrc/cli/test_command.zigsrc/cli.zigsrc/bunfig.zigsrc/Watcher.zigsrc/bun.js/hot_reloader.zigsrc/bake/DevServer.zigsrc/cli/Arguments.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/watch/watch-excludes.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/watch/watch-excludes.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/watch/watch-excludes.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/watch/watch-excludes.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/watch/watch-excludes.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/watch/watch-excludes.test.ts
🧠 Learnings (9)
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/cli/watch/watch-excludes.test.ts
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/Watcher.zig
🧬 Code graph analysis (1)
test/cli/watch/watch-excludes.test.ts (1)
test/harness.ts (2)
tmpdirSync(1211-1213)bunExe(102-105)
🔇 Additional comments (8)
src/cli/test_command.zig (1)
1518-1522: Approve test --watch excludes wiring
Allocator-backedctx.debug.watch_excludesslices outlive the VM/watchers, and pattern normalization is shared with the run path.src/bun.js/node/path_watcher.zig (1)
121-129: Approve default watcher config
Passing.≠{}toWatcher.initusesInitConfig.exclude_patterns = &.{}, so no paths are excluded by default, matchingfs.watchsemantics.src/bun.js.zig (1)
262-268: Verify watch_excludes lifetime and normalize path separators
- Assignment before enabling HMR/watch is correct, but confirm that the backing memory for
ctx.debug.watch_excludesoutlives the VM’s watchers or use avm.setWatchExcludes(...)helper to duplicate into the VM allocator.- In
Watcher.shouldExcludeFile,clean_pathonly strips a leading/—on Windows backslashes won’t match forward-slash patterns. Normalize all\to/before callingglob.match.src/cli.zig (1)
315-316: DebugOptions field addition looks goodPublic surface addition is consistent with downstream wiring.
src/bun.js/hot_reloader.zig (1)
76-76: Propagating exclude_patterns is correct; verify lifetime of the slicePassing ctx.watch_excludes directly is fine if its memory outlives the watcher. If any caller builds this slice on a short‑lived allocator, duplicate the outer slice inside Watcher.init to avoid dangling references.
Would you confirm watch_excludes always originates from process‑lifetime storage (argv or ctx allocator)? If unsure, I can propose a small change in Watcher.init to copy the outer slice (shallow) to watcher.
Also applies to: 203-214, 224-227, 238-249
src/cli/Arguments.zig (1)
80-81: New flag wiring is correctFlag name and help align with feature; included in runtime_params_ for Run/Auto/Test flows.
src/Watcher.zig (2)
52-55: InitConfig and field plumb‑through look correctStructure and initialization are consistent; no functional concerns here.
Also applies to: 60-69, 77-77, 106-107
734-734: Importing glob matcherCorrect dependency; aligns with shouldExcludeFile usage.
| return global.throwError(err, generic_action); | ||
|
|
||
| dev.bun_watcher = Watcher.init(DevServer, dev, fs, bun.default_allocator) catch |err| | ||
| dev.bun_watcher = Watcher.init(DevServer, dev, fs, bun.default_allocator, .{}) catch |err| |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that the empty config is appropriate for DevServer's watcher.
The PR adds watch-excludes support to prevent infinite reload loops, and the objectives state that excludes should "propagate through VM, CLI, bunfig, hot reloader, dev server." However, this line passes an empty config .{} to Watcher.init, meaning no exclude patterns are provided to the DevServer's watcher.
Please confirm:
- Does the
Watcher.initfunction internally retrieve exclude patterns from the VM or global config when an empty config is passed? - Or should the
DevServer.Optionsstruct include awatch_excludesfield to pass patterns here?
If the DevServer's watcher doesn't receive exclude patterns, files that should be excluded may still trigger hot reloads, potentially causing the infinite loops this PR aims to prevent.
Run the following script to check how exclude patterns are propagated to other watcher initialization sites:
🏁 Script executed:
#!/bin/bash
# Check how other Watcher.init calls in the PR pass exclude patterns
rg -A 2 -B 2 'Watcher\.init.*watch.*excludes|excludes.*Watcher\.init' --type zigLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find Watcher.init signature and Options struct definition
rg -n 'Watcher\.init' -C3 --type zig
rg -n 'const Options = struct' -C5 --type zigLength of output: 21885
DevServer must propagate watch_excludes to Watcher.init
In src/bake/DevServer.zig at line 383, you’re passing . {} to Watcher.init, so no exclude patterns reach the watcher. Add a watch_excludes field to DevServer.Options (defaulting to an empty slice) and change the call to
Watcher.init(DevServer, dev, fs, bun.default_allocator, .{ .exclude_patterns = this.watch_excludes })so that exclude patterns flow through exactly like in hot_reloader.zig.
🤖 Prompt for AI Agents
In src/bake/DevServer.zig around line 383, DevServer currently calls
Watcher.init with an empty options struct so exclude patterns are not passed
through; add a watch_excludes field to DevServer.Options (type: slice of []const
u8 or the same type used elsewhere) with a default of an empty slice, then
change the Watcher.init call to pass the watch_excludes (e.g. .{
.exclude_patterns = this.watch_excludes }) so exclude patterns propagate exactly
like in hot_reloader.zig.
| watch_excludes: []const []const u8 = &.{}, | ||
| jsc_vm: *VM = undefined, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Document lifetime or copy patterns into VM allocator
Good addition. Please either:
- Document that watch_excludes points to memory that outlives the VM/watchers, or
- Provide a setter that dupes patterns into vm.allocator to avoid dangling slices.
Example doc tweak:
- hot_reload: bun.cli.Command.HotReload = .none,
- watch_excludes: []const []const u8 = &.{},
+ hot_reload: bun.cli.Command.HotReload = .none,
+ /// Glob patterns ignored by HMR/watch; must outlive watchers or be copied.
+ watch_excludes: []const []const u8 = &.{},To confirm usage/lifetime across the repo:
🏁 Script executed:
#!/bin/bash
rg -n --type=zig -C3 '\bwatch_excludes\b|exclude_patterns|InitConfig|Watcher\.init\('Length of output: 15199
Document or clone exclude patterns
Add a doc comment in VirtualMachine.zig above watch_excludes stating that these glob patterns are borrowed from the CLI/config and must outlive any spawned Watcher, or provide a setter that duplicates them into the VM/Watcher allocator.
pub fn VirtualMachine() type {
return struct {
counters: Counters = .{},
hot_reload: bun.cli.Command.HotReload = .none,
+ /// Glob patterns ignored by HMR/watch; borrowed — must outlive spawned watchers or be cloned into the allocator.
watch_excludes: []const []const u8 = &.{},
jsc_vm: *VM = undefined,📝 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.
| watch_excludes: []const []const u8 = &.{}, | |
| jsc_vm: *VM = undefined, | |
| pub fn VirtualMachine() type { | |
| return struct { | |
| counters: Counters = .{}, | |
| hot_reload: bun.cli.Command.HotReload = .none, | |
| /// Glob patterns ignored by HMR/watch; borrowed — must outlive spawned watchers | |
| /// or be cloned into the allocator. | |
| watch_excludes: []const []const u8 = &.{}, | |
| jsc_vm: *VM = undefined, | |
| // …rest of fields… | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/VirtualMachine.zig around lines 54–55, the watch_excludes field
currently holds borrowed glob pattern slices but lacks documentation or
ownership guarantees; either add a doc comment directly above watch_excludes
stating these patterns are borrowed from the CLI/config and therefore must
outlive any spawned Watcher, or implement a setter on VirtualMachine that
duplicates/clones the pattern array and each string into the VM/Watcher
allocator so the VM/Watcher owns them; if you choose cloning, ensure the setter
allocates the array and each []const u8 with the same allocator used by Watcher,
updates watch_excludes to point to the owned copies, and document or implement
teardown to free them when the VM/Watcher is destroyed.
| // Combine CLI watch excludes with bunfig.toml excludes | ||
| const cli_excludes = args.options("--watch-excludes"); | ||
| if (cli_excludes.len > 0) { | ||
| if (ctx.debug.watch_excludes.len > 0) { | ||
| // Merge CLI and bunfig excludes | ||
| var combined = try allocator.alloc([]const u8, ctx.debug.watch_excludes.len + cli_excludes.len); | ||
| @memcpy(combined[0..ctx.debug.watch_excludes.len], ctx.debug.watch_excludes); | ||
| @memcpy(combined[ctx.debug.watch_excludes.len..], cli_excludes); | ||
| ctx.debug.watch_excludes = combined; | ||
| } else { | ||
| ctx.debug.watch_excludes = cli_excludes; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CLI/bunfig merge works; consider dedup and guard to watch/hot
Current merge always runs; harmless, but you could:
- Only merge when --watch/--hot (minor).
- De‑duplicate combined patterns to avoid redundant matching.
- ctx.debug.watch_excludes = combined;
+ // dedupe (optional)
+ var seen = std.AutoHashMap([]const u8, void).init(allocator);
+ defer seen.deinit();
+ var uniq = try allocator.alloc([]const u8, combined.len);
+ var n: usize = 0;
+ for (combined) |p| if (!seen.contains(p)) {
+ try seen.put(p, {});
+ uniq[n] = p; n += 1;
+ }
+ ctx.debug.watch_excludes = uniq[0..n];📝 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.
| // Combine CLI watch excludes with bunfig.toml excludes | |
| const cli_excludes = args.options("--watch-excludes"); | |
| if (cli_excludes.len > 0) { | |
| if (ctx.debug.watch_excludes.len > 0) { | |
| // Merge CLI and bunfig excludes | |
| var combined = try allocator.alloc([]const u8, ctx.debug.watch_excludes.len + cli_excludes.len); | |
| @memcpy(combined[0..ctx.debug.watch_excludes.len], ctx.debug.watch_excludes); | |
| @memcpy(combined[ctx.debug.watch_excludes.len..], cli_excludes); | |
| ctx.debug.watch_excludes = combined; | |
| } else { | |
| ctx.debug.watch_excludes = cli_excludes; | |
| } | |
| } | |
| // Combine CLI watch excludes with bunfig.toml excludes | |
| const cli_excludes = args.options("--watch-excludes"); | |
| if (cli_excludes.len > 0) { | |
| if (ctx.debug.watch_excludes.len > 0) { | |
| // Merge CLI and bunfig excludes | |
| var combined = try allocator.alloc([]const u8, ctx.debug.watch_excludes.len + cli_excludes.len); | |
| @memcpy(combined[0..ctx.debug.watch_excludes.len], ctx.debug.watch_excludes); | |
| @memcpy(combined[ctx.debug.watch_excludes.len..], cli_excludes); | |
| // dedupe (optional) | |
| var seen = std.AutoHashMap([]const u8, void).init(allocator); | |
| defer seen.deinit(); | |
| var uniq = try allocator.alloc([]const u8, combined.len); | |
| var n: usize = 0; | |
| for (combined) |p| { | |
| if (!seen.contains(p)) { | |
| try seen.put(p, {}); | |
| uniq[n] = p; | |
| n += 1; | |
| } | |
| } | |
| ctx.debug.watch_excludes = uniq[0..n]; | |
| } else { | |
| ctx.debug.watch_excludes = cli_excludes; | |
| } | |
| } |
| fn shouldExcludeFile(this: *Watcher, file_path: string) bool { | ||
| if (this.exclude_patterns.len == 0) { | ||
| return false; | ||
| } | ||
|
|
||
| const relative_path = if (strings.startsWith(file_path, this.cwd)) | ||
| file_path[this.cwd.len..] | ||
| else | ||
| file_path; | ||
|
|
||
| const clean_path = if (relative_path.len > 0 and relative_path[0] == '/') | ||
| relative_path[1..] | ||
| else | ||
| relative_path; | ||
|
|
||
| for (this.exclude_patterns) |pattern| { | ||
| if (glob.match(this.allocator, pattern, clean_path).matches()) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Path normalization in shouldExcludeFile for Windows
You compare against cwd and strip leading '/' only. On Windows, backslashes may prevent matching (e.g., '\foo\bar.json' vs '*.json'). Normalize separators (to '/') before glob matching to ensure cross‑platform behavior.
- const clean_path = if (relative_path.len > 0 and relative_path[0] == '/')
- relative_path[1..]
- else
- relative_path;
+ var clean_path = if (relative_path.len > 0 and (relative_path[0] == '/' or relative_path[0] == '\\'))
+ relative_path[1..]
+ else
+ relative_path;
+ // Normalize separators on Windows
+ if (comptime Environment.isWindows) {
+ for (clean_path) |*c| if (c.* == '\\') c.* = '/';
+ }Please verify glob.match handles Windows paths without normalization; if it already normalizes, this change may be unnecessary.
🏁 Script executed:
#!/bin/bash
# Find glob import in src/Watcher.zig
rg -n "import.*glob" -g "src/Watcher.zig"Length of output: 61
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/glob/match.zigLength of output: 8370
🏁 Script executed:
#!/bin/bash
# Find definition and usage of isSeparator in glob module
rg -n "fn isSeparator" -g "src/glob/*.zig"
rg -n "isSeparator" -g "src/glob/*.zig"Length of output: 896
🏁 Script executed:
#!/bin/bash
sed -n '390,430p' src/glob/match.zigLength of output: 983
🏁 Script executed:
#!/bin/bash
sed -n '220,280p' src/glob/match.zigLength of output: 2681
🏁 Script executed:
#!/bin/bash
rg -n "shouldExcludeFile" -g "src/Watcher.zig"Length of output: 147
Normalize Windows path separators in shouldExcludeFile
Strip both leading '/' or '' and, on Windows, convert all '' to '/' before invoking glob.match:
- const clean_path = if (relative_path.len > 0 and relative_path[0] == '/')
- relative_path[1..]
+ var clean_path = if (relative_path.len > 0 and (relative_path[0] == '/' or relative_path[0] == '\\'))
+ relative_path[1..]
else
relative_path;
+ if (comptime @import("builtin").os.tag == .windows) {
+ for (clean_path) |*c| if (c.* == '\\') c.* = '/';
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Watcher.zig around lines 316 to 339, shouldExcludeFile currently only
strips a leading '/' and passes the raw path to glob.match; on Windows
backslashes need to be normalized and a leading '\' should also be stripped.
Normalize by converting all '\' characters to '/' (e.g., create a cleaned string
where you replace backslashes with slashes) before the leading-separator check,
then strip either leading '/' or '\' (or simply strip a leading '/' after
normalization), and use that normalized clean_path when calling glob.match;
ensure you allocate the normalized string with this.allocator and free if
required by surrounding code.
| if (this.shouldExcludeFile(file_path)) { | ||
| if (comptime DebugLogScope.isVisible()) { | ||
| log("Excluded file from watch: {s}", .{file_path}); | ||
| } | ||
| return .success; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Skipping excluded files is good; consider optional directory-level skip
For patterns like "coverage/**", you might also skip adding watchers for those directories to reduce event churn. Optional, not required for correctness.
🤖 Prompt for AI Agents
In src/Watcher.zig around lines 359 to 365, add an optional directory-level skip
so that when shouldExcludeFile(file_path) matches a directory pattern (e.g.,
"coverage/**") you avoid installing watchers for that directory and its
children; detect whether the matched path is a directory (stat or
Dir.Entry.isDir), and if it is treat it as a directory-exclude and return
.success from the watcher-add flow without recursing or creating watchers under
it; if it's a file pattern continue as before. Ensure the check is applied in
the code paths that create or recurse into watchers so excluded directories are
never added.
| it("should exclude files matching --watch-excludes patterns", async () => { | ||
| const cwd = tmpdirSync(); | ||
| const scriptPath = join(cwd, "script.js"); | ||
| const dataPath = join(cwd, "data.json"); | ||
| const logPath = join(cwd, "output.log"); | ||
|
|
||
| // Create script that continuously modifies excluded files | ||
| await Bun.write(scriptPath, ` | ||
| require("fs").writeFileSync("${logPath}", "executed-" + Date.now()); | ||
| setInterval(() => { | ||
| require("fs").writeFileSync("${dataPath}", JSON.stringify({ timestamp: Date.now() })); | ||
| }, 500); | ||
| process.on('SIGTERM', () => process.exit(0)); | ||
| `); | ||
|
|
||
| await Bun.write(dataPath, "{}"); | ||
|
|
||
| // Start watching with *.json excluded | ||
| watchee = spawn({ | ||
| cwd, | ||
| cmd: [bunExe(), "--watch", "--watch-excludes", "*.json", "script.js"], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "inherit", | ||
| stdin: "ignore", | ||
| }); | ||
|
|
||
| await Bun.sleep(1000); | ||
| const initialLog = await Bun.file(logPath).text(); | ||
|
|
||
| // Wait for script to modify excluded files multiple times | ||
| await Bun.sleep(2000); | ||
|
|
||
| // Excluded file modifications should not trigger reload | ||
| expect(await Bun.file(logPath).text()).toBe(initialLog); | ||
|
|
||
| // Modify watched file - should trigger reload | ||
| await Bun.write(scriptPath, ` | ||
| require("fs").writeFileSync("${logPath}", "reloaded-" + Date.now()); | ||
| process.on('SIGTERM', () => process.exit(0)); | ||
| `); | ||
|
|
||
| await Bun.sleep(1000); | ||
|
|
||
| // Should have reloaded | ||
| const finalLog = await Bun.file(logPath).text(); | ||
| expect(finalLog).toContain("reloaded-"); | ||
| expect(finalLog).not.toBe(initialLog); | ||
|
|
||
| // Cleanup | ||
| rmSync(scriptPath, { force: true }); | ||
| rmSync(dataPath, { force: true }); | ||
| rmSync(logPath, { force: true }); | ||
| }, 6000); | ||
|
|
There was a problem hiding this comment.
Tests are flaky: avoid arbitrary sleeps and explicit timeouts
- Replace fixed sleeps with condition-based waits (e.g., poll until log file appears/changes).
- Don’t set per-test timeouts; rely on Bun’s defaults. As per testing guidelines.
- await Bun.sleep(1000);
- const initialLog = await Bun.file(logPath).text();
+ // wait for initial log to be written
+ const initialLog = await waitFor(() => Bun.file(logPath).exists().then(ex => ex ? Bun.file(logPath).text() : null));
@@
- await Bun.sleep(2000);
+ // wait to ensure excluded writes have happened
+ await Bun.sleep(500);
@@
- await Bun.sleep(1000);
+ const finalLog = await waitFor(async () => {
+ const txt = await Bun.file(logPath).text();
+ return txt.includes("reloaded-") ? txt : null;
+ });
- const finalLog = await Bun.file(logPath).text();
+ // finalLog returned above
@@
-}, 6000);
+});
@@
-}, 6000);
+});
@@
+async function waitFor<T>(fn: () => Promise<T | null>, timeoutMs = 5000, intervalMs = 50): Promise<T> {
+ const start = Date.now();
+ while (Date.now() - start < timeoutMs) {
+ const v = await fn();
+ if (v != null) return v;
+ await Bun.sleep(intervalMs);
+ }
+ throw new Error("timeout waiting for condition");
+}Based on guidelines
Also applies to: 67-132, 133-135
🤖 Prompt for AI Agents
In test/cli/watch/watch-excludes.test.ts around lines 10-66 (and also apply same
changes to blocks covering 67-132 and 133-135), the test uses fixed Bun.sleep
delays and an explicit per-test timeout which makes it flaky; replace the
arbitrary sleeps and the trailing 6000 timeout with condition-based polling and
short retry loops that assert within a bounded timeout: wait for the log file to
appear by polling Bun.file(logPath).exists() (or reading .text() until
non-empty) with small intervals (e.g., 50-100ms) and a sensible overall timeout,
then poll for content changes after the excluded-file modifications instead of
sleeping, and after writing the updated script poll until the log contains
"reloaded-"; finally remove the explicit per-test timeout parameter so the test
relies on Bun’s default timing and fail if the polls time out.
|
hey @nektro @dylan-conway, can you give me some pointers on which coderabbit feedbacks are actually relevant so I can focus on implementation? |
1 participant
What does this PR do?
Adds
--watch-excludesCLI flag to prevent infinite reload loops when scripts modify files they import.bun --watch --watch-excludes "*.json" "*.log" script.js[watch] excludes = ["*.json"]in bunfig.toml--watch,--hot,test --watchrelated #5278
How did you verify your code works?
test/cli/watch/watch-excludes.test.ts