Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a generic Reactive class and a FileWatcher utility with per-subscription debounced event batching, depth-aware path filtering, reference-counted path management, and optional module decache. Introduces debounced-batch utilities and path-set comparison helpers. Adds unit tests for FileWatcher and Reactive. Re-exports Reactive and FileWatcher from dev-utils main. Replaces previous watchDebounced usage with FileWatcher across FunctionsRegistry and NetlifyDev, wiring reactive configuration and FileWatcher into initialization and tests; updates call sites and types to use Reactive and FileWatchSubscriptionHandle. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
paulo
previously approved these changes
Mar 4, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/dev-utils/src/lib/file-watcher/index.test.ts (1)
188-210: Add regression tests for post-unsubscribe handle safety and pending debounce behavior.Current suite is strong, but it misses two lifecycle edge cases: calling
handle.add()after unsubscribe/id-swap, and ensuring no delayed callback fires after unsubscribe.Also applies to: 258-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dev-utils/src/lib/file-watcher/index.test.ts` around lines 188 - 210, Add regression tests around the watcher subscription lifecycle: after obtaining a handle from watcher.subscribe({ paths, onChange }), verify that calling handle.add(...) after handle.unsubscribe() is safe (does not throw and has no effect), and verify that any pending debounced callbacks scheduled before unsubscribe do not fire after handle.unsubscribe() (e.g., write a file, call handle.unsubscribe() before debounce elapses, waitForEvents(), and assert onChange was not called). Use the existing test's symbols (watcher.subscribe, handle.unsubscribe, handle.add, onChange, waitForEvents) to locate where to add these assertions and ensure both the "post-unsubscribe add" and "no delayed callback" cases are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dev-utils/src/lib/file-watcher/index.ts`:
- Around line 201-206: Queued debounced flushes are still firing after
subscription removal or close; update the unsubscribe method (the one removing
entries from this.#subscriptions) and the async close() method to cancel any
pending debounced flushes for those subscriptions before clearing state.
Specifically, track debounced timers/handlers for each subscription (or use a
debounced function that exposes .cancel) and call clearTimeout or
debounced.cancel() for that subscription in unsubscribe, and iterate/cancel all
remaining pending debounces before calling this.#subscriptions.clear(),
this.#handles.clear(), this.#pathRefCounts.clear(), and await
this.#watcher.close() in close().
- Around line 170-193: The handle returned from FileWatchSubscriptionHandle
(captured in the add, unwatch closures) remains usable after calling
unsubscribe, allowing stale add()/unwatch() calls to change
subscription.watchedPaths and call `#addPathRef/`#removePathRef with no active
subscriber; fix by adding an "active" flag captured in the handle closure (or
attach a closed property to the handle) that is true initially and set to false
inside unsubscribe (before/after calling this.#unsubscribe(id)), and then
early-return from add() and unwatch() when active is false so they no-op for
stale handles, preventing orphaned path ref increments/decrements.
In `@packages/dev-utils/src/lib/file-watcher/util.ts`:
- Around line 39-50: The function pathSetsEqual currently treats duplicate
entries in the incoming array as distinct; normalize incoming by building a Set
from the incoming array (e.g., incomingSet = new Set(incoming)) and then compare
sizes (existing.size === incomingSet.size) and membership (for each p of
incomingSet ensure existing.has(p)). Update the function pathSetsEqual to use
the incomingSet for size and element checks so duplicates no longer cause false
negatives when sets are otherwise equal.
In `@packages/dev-utils/src/lib/reactive.test.ts`:
- Around line 27-28: The test currently uses two assertions on the mock callback
(expect(callback).toHaveBeenCalledOnce() and
expect(callback).toHaveBeenCalledWith({ port: 8080 })); replace both with a
single assertion using vitest's matcher:
expect(callback).toHaveBeenCalledExactlyOnceWith({ port: 8080 }) to satisfy the
vitest/prefer-called-exactly-once-with rule—update the assertion for the mock
named callback in reactive.test.ts accordingly.
In `@packages/dev/src/main.ts`:
- Around line 476-490: The subscription to fileWatcher.subscribe only handles
onChange; extract the reload block (the async function that calls
this.getConfig(), assigns this.#config, calls reactiveConfig.set(newConfig), and
catches errors to call this.#logger.warn) into a single helper method or local
async function (e.g., reloadConfig) and pass that same handler to onChange,
onAdd and onUnlink in the subscription for id 'netlify-config' and paths
config.configPath so config reloads when files are replaced as unlink+add.
In `@packages/functions/dev/src/registry.ts`:
- Around line 333-335: The current conditional prevents the initial build when
this.fileWatcher is falsy; remove the dependency on this.fileWatcher so
buildFunctionAndWatchFiles(func, !isReload) is always invoked for initial
builds. Replace the else-if that checks this.fileWatcher with an else (or
unguarded call) so buildFunctionAndWatchFiles runs whether or not a watcher
exists, keeping any existing internal handling for no-watcher mode intact.
- Around line 215-217: The watcher callbacks call async functions
(buildFunctionAndWatchFiles and scan) without handling rejections, risking
unhandled promise rejections; update the callbacks (the onChange handler and
other watcher callbacks that call buildFunctionAndWatchFiles and scan) to call
the async functions and handle errors, e.g. invoke them and attach .catch(...)
or make the callback async and await inside a try/catch, and log or propagate
the error via the existing logger so all rejections are handled.
---
Nitpick comments:
In `@packages/dev-utils/src/lib/file-watcher/index.test.ts`:
- Around line 188-210: Add regression tests around the watcher subscription
lifecycle: after obtaining a handle from watcher.subscribe({ paths, onChange }),
verify that calling handle.add(...) after handle.unsubscribe() is safe (does not
throw and has no effect), and verify that any pending debounced callbacks
scheduled before unsubscribe do not fire after handle.unsubscribe() (e.g., write
a file, call handle.unsubscribe() before debounce elapses, waitForEvents(), and
assert onChange was not called). Use the existing test's symbols
(watcher.subscribe, handle.unsubscribe, handle.add, onChange, waitForEvents) to
locate where to add these assertions and ensure both the "post-unsubscribe add"
and "no delayed callback" cases are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50ef01d9-667c-436a-886e-76cb50c8967f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
packages/dev-utils/src/lib/file-watcher/index.test.tspackages/dev-utils/src/lib/file-watcher/index.tspackages/dev-utils/src/lib/file-watcher/util.tspackages/dev-utils/src/lib/reactive.test.tspackages/dev-utils/src/lib/reactive.tspackages/dev-utils/src/main.tspackages/dev/src/main.tspackages/functions/dev/src/main.test.tspackages/functions/dev/src/registry.test.tspackages/functions/dev/src/registry.ts
Comment on lines
+170
to
+193
| const handle: FileWatchSubscriptionHandle = { | ||
| add: (newPaths) => { | ||
| const normalized = ensureArray(newPaths) | ||
|
|
||
| for (const p of normalized) { | ||
| if (!subscription.watchedPaths.has(p)) { | ||
| subscription.watchedPaths.add(p) | ||
| this.#addPathRef(p) | ||
| } | ||
| } | ||
| }, | ||
| unwatch: (removePaths) => { | ||
| const normalized = ensureArray(removePaths) | ||
|
|
||
| for (const p of normalized) { | ||
| if (subscription.watchedPaths.has(p)) { | ||
| subscription.watchedPaths.delete(p) | ||
| this.#removePathRef(p) | ||
| } | ||
| } | ||
| }, | ||
| unsubscribe: () => { | ||
| this.#unsubscribe(id) | ||
| }, |
There was a problem hiding this comment.
Guard stale handles after unsubscription to prevent orphaned watches.
After #unsubscribe removes the subscription (Line 288 and Line 289), an old handle can still call add() and increment refs for paths that no subscription will receive events for.
✅ Suggested change
const handle: FileWatchSubscriptionHandle = {
add: (newPaths) => {
+ if (!this.#subscriptions.has(id)) {
+ return
+ }
+
const normalized = ensureArray(newPaths)
for (const p of normalized) {
if (!subscription.watchedPaths.has(p)) {
subscription.watchedPaths.add(p)
this.#addPathRef(p)
}
}
},
unwatch: (removePaths) => {
+ if (!this.#subscriptions.has(id)) {
+ return
+ }
+
const normalized = ensureArray(removePaths)
for (const p of normalized) {
if (subscription.watchedPaths.has(p)) {
subscription.watchedPaths.delete(p)
this.#removePathRef(p)
}
}
},
unsubscribe: () => {
+ if (!this.#subscriptions.has(id)) {
+ return
+ }
this.#unsubscribe(id)
},
}Also applies to: 277-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dev-utils/src/lib/file-watcher/index.ts` around lines 170 - 193, The
handle returned from FileWatchSubscriptionHandle (captured in the add, unwatch
closures) remains usable after calling unsubscribe, allowing stale
add()/unwatch() calls to change subscription.watchedPaths and call
`#addPathRef/`#removePathRef with no active subscriber; fix by adding an "active"
flag captured in the handle closure (or attach a closed property to the handle)
that is true initially and set to false inside unsubscribe (before/after calling
this.#unsubscribe(id)), and then early-return from add() and unwatch() when
active is false so they no-op for stale handles, preventing orphaned path ref
increments/decrements.
Comment on lines
+201
to
+206
| async close(): Promise<void> { | ||
| this.#subscriptions.clear() | ||
| this.#handles.clear() | ||
| this.#pathRefCounts.clear() | ||
| await this.#watcher.close() | ||
| } |
There was a problem hiding this comment.
Cancel pending debounced batches on unsubscribe/close.
Queued debounced flushes can still invoke callbacks after a subscription is removed (or after close()), which breaks lifecycle expectations.
✅ Suggested change (requires util + index updates)
// packages/dev-utils/src/lib/file-watcher/util.ts
export interface DebouncedBatch {
push(path: string): void
+ cancel(): void
}
export function createDebouncedBatch(callback: FileWatchCallback | undefined, debounceMs: number): DebouncedBatch {
const queue: string[] = []
@@
return {
push(filePath: string) {
queue.push(filePath)
flush()
},
+ cancel() {
+ queue.length = 0
+ flush.cancel?.()
+ },
}
}// packages/dev-utils/src/lib/file-watcher/index.ts
async close(): Promise<void> {
+ for (const subscription of this.#subscriptions.values()) {
+ for (const batcher of Object.values(subscription.batchers)) {
+ batcher.cancel()
+ }
+ }
this.#subscriptions.clear()
this.#handles.clear()
this.#pathRefCounts.clear()
await this.#watcher.close()
}
@@
`#unsubscribe`(id: string) {
const subscription = this.#subscriptions.get(id)
@@
+ for (const batcher of Object.values(subscription.batchers)) {
+ batcher.cancel()
+ }
+
for (const p of subscription.watchedPaths) {
this.#removePathRef(p)
}Also applies to: 277-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dev-utils/src/lib/file-watcher/index.ts` around lines 201 - 206,
Queued debounced flushes are still firing after subscription removal or close;
update the unsubscribe method (the one removing entries from
this.#subscriptions) and the async close() method to cancel any pending
debounced flushes for those subscriptions before clearing state. Specifically,
track debounced timers/handlers for each subscription (or use a debounced
function that exposes .cancel) and call clearTimeout or debounced.cancel() for
that subscription in unsubscribe, and iterate/cancel all remaining pending
debounces before calling this.#subscriptions.clear(), this.#handles.clear(),
this.#pathRefCounts.clear(), and await this.#watcher.close() in close().
Comment on lines
+39
to
+50
| export function pathSetsEqual(existing: Set<string>, incoming: string[]): boolean { | ||
| if (existing.size !== incoming.length) { | ||
| return false | ||
| } | ||
|
|
||
| for (const p of incoming) { | ||
| if (!existing.has(p)) { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
Normalize duplicate incoming paths before set comparison.
If incoming contains duplicates, equal path sets can be treated as different, causing unnecessary teardown/recreation for same-id subscriptions.
✅ Suggested change
export function pathSetsEqual(existing: Set<string>, incoming: string[]): boolean {
- if (existing.size !== incoming.length) {
+ const normalized = new Set(incoming)
+
+ if (existing.size !== normalized.size) {
return false
}
- for (const p of incoming) {
+ for (const p of normalized) {
if (!existing.has(p)) {
return false
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dev-utils/src/lib/file-watcher/util.ts` around lines 39 - 50, The
function pathSetsEqual currently treats duplicate entries in the incoming array
as distinct; normalize incoming by building a Set from the incoming array (e.g.,
incomingSet = new Set(incoming)) and then compare sizes (existing.size ===
incomingSet.size) and membership (for each p of incomingSet ensure
existing.has(p)). Update the function pathSetsEqual to use the incomingSet for
size and element checks so duplicates no longer cause false negatives when sets
are otherwise equal.
Comment on lines
215
to
217
| onChange: () => { | ||
| this.buildFunctionAndWatchFiles(func, false) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l packages/functions/dev/src/registry.tsRepository: netlify/primitives
Length of output: 104
🏁 Script executed:
sed -n '205,225p' packages/functions/dev/src/registry.tsRepository: netlify/primitives
Length of output: 666
🏁 Script executed:
sed -n '480,495p' packages/functions/dev/src/registry.tsRepository: netlify/primitives
Length of output: 357
🏁 Script executed:
# Find the method definitions for buildFunctionAndWatchFiles and scan
rg -n "^\s*(async\s+)?buildFunctionAndWatchFiles|^\s*(async\s+)?scan\s*\(" packages/functions/dev/src/registry.ts -A 3Repository: netlify/primitives
Length of output: 521
🏁 Script executed:
sed -n '480,495p' packages/functions/dev/src/registry.ts | cat -nRepository: netlify/primitives
Length of output: 469
Handle unhandled promise rejections in watcher callbacks.
The methods buildFunctionAndWatchFiles (line 161) and scan (line 354) are both async, but the callbacks at lines 216, 486, and 489 invoke them without awaiting or handling rejections. This creates unhandled promise rejection risk.
Proposed fixes
onChange: () => {
- this.buildFunctionAndWatchFiles(func, false)
+ void this.buildFunctionAndWatchFiles(func, false).catch(() => {})
}, onAdd: () => {
- this.scan([directory])
+ void this.scan([directory]).catch(() => {})
},
onUnlink: () => {
- this.scan([directory])
+ void this.scan([directory]).catch(() => {})
},📝 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.
Suggested change
| onChange: () => { | |
| this.buildFunctionAndWatchFiles(func, false) | |
| }, | |
| onChange: () => { | |
| void this.buildFunctionAndWatchFiles(func, false).catch(() => {}) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/functions/dev/src/registry.ts` around lines 215 - 217, The watcher
callbacks call async functions (buildFunctionAndWatchFiles and scan) without
handling rejections, risking unhandled promise rejections; update the callbacks
(the onChange handler and other watcher callbacks that call
buildFunctionAndWatchFiles and scan) to call the async functions and handle
errors, e.g. invoke them and attach .catch(...) or make the callback async and
await inside a try/catch, and log or propagate the error via the existing logger
so all rejections are handled.
Comment on lines
+333
to
335
| } else if (this.fileWatcher) { | ||
| this.buildFunctionAndWatchFiles(func, !isReload) | ||
| } |
There was a problem hiding this comment.
Don’t skip function build when fileWatcher is undefined.
On Line 333, the initial build is now conditional on this.fileWatcher. That changes behavior: functions won’t be built in no-watcher setups, even though buildFunctionAndWatchFiles already supports that mode.
Proposed fix
- } else if (this.fileWatcher) {
+ } else {
this.buildFunctionAndWatchFiles(func, !isReload)
}📝 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.
Suggested change
| } else if (this.fileWatcher) { | |
| this.buildFunctionAndWatchFiles(func, !isReload) | |
| } | |
| } else { | |
| this.buildFunctionAndWatchFiles(func, !isReload) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/functions/dev/src/registry.ts` around lines 333 - 335, The current
conditional prevents the initial build when this.fileWatcher is falsy; remove
the dependency on this.fileWatcher so buildFunctionAndWatchFiles(func,
!isReload) is always invoked for initial builds. Replace the else-if that checks
this.fileWatcher with an else (or unguarded call) so buildFunctionAndWatchFiles
runs whether or not a watcher exists, keeping any existing internal handling for
no-watcher mode intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dev/src/main.ts`:
- Around line 479-482: reloadConfig currently updates this.#config and
reactiveConfig.set(newConfig) but only FunctionsHandler listens to
reactiveConfig, so other handlers created from startup snapshots
(HeadersHandler, RedirectsHandler, StaticHandler, ImageHandler,
EdgeFunctionsHandler) are not updated on hot-reload; fix by wiring those
handlers to the reactive config or calling their refresh/update methods from
reloadConfig: locate where handlers are instantiated (references to
HeadersHandler, RedirectsHandler, StaticHandler, ImageHandler,
EdgeFunctionsHandler and the reactiveConfig usage), and either subscribe each
handler to reactiveConfig changes or add a unified update step in reloadConfig
that calls a refresh/reinitialize method on each handler instance so they pick
up the new this.#config/reactiveConfig state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f0121cb-3ea0-46e1-a9d2-062cea6753df
📒 Files selected for processing (1)
packages/dev/src/main.ts
Comment on lines
+479
to
+482
| const newConfig = await this.getConfig() | ||
| this.#config = newConfig | ||
| reactiveConfig.set(newConfig) | ||
| } catch (error) { |
There was a problem hiding this comment.
Config hot-reload currently propagates only to FunctionsHandler.
reloadConfig updates this.#config and reactiveConfig, but only FunctionsHandler is wired to reactiveConfig (Line 647). Handlers initialized from startup snapshots (HeadersHandler, RedirectsHandler, StaticHandler, ImageHandler, EdgeFunctionsHandler) are not refreshed, so relevant netlify.toml edits still require restart.
Also applies to: 647-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dev/src/main.ts` around lines 479 - 482, reloadConfig currently
updates this.#config and reactiveConfig.set(newConfig) but only FunctionsHandler
listens to reactiveConfig, so other handlers created from startup snapshots
(HeadersHandler, RedirectsHandler, StaticHandler, ImageHandler,
EdgeFunctionsHandler) are not updated on hot-reload; fix by wiring those
handlers to the reactive config or calling their refresh/update methods from
reloadConfig: locate where handlers are instantiated (references to
HeadersHandler, RedirectsHandler, StaticHandler, ImageHandler,
EdgeFunctionsHandler and the reactiveConfig usage), and either subscribe each
handler to reactiveConfig changes or add a unified update step in reloadConfig
that calls a refresh/reinitialize method on each handler instance so they pick
up the new this.#config/reactiveConfig state.
paulo
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new
FileWatcherclass to handle file watching. We previously had thefunctionspackage doing file watching, but each function had its ownwatchDebounced, which in its turn had its own chokidar instance.FileWatcheris a shared file-watching primitive backed by a single chokidar instance. Supports multiple subscriptions with ref-counted paths, debounced event batching, optional depth constraints, and named subscriptions (viaid) for idempotent directory watching (so that we can call it repeatedly with adb-migrations-pathID whenever the Netlify config changes, and nothing happens unless the migrations path actually changes).Also,
NetlifyDevnow wraps the resolved config in aReactivevalue, watchesnetlify.tomlfor changes viaFileWatcher, and propagates updates to handlers automatically.In a future PR we can refactor the CLI to use
FileWatcherand removewatchDebouncedentirely.