feat(cli): fix page file update racing problems and avoid middle state processing#1691
feat(cli): fix page file update racing problems and avoid middle state processing#1691Mister-Hope wants to merge 4 commits intomainfrom
Conversation
Pull Request Test Coverage Report for Build 23324451747Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Serializes page/dependency watcher handlers in the CLI dev server to prevent race conditions when multiple filesystem events arrive in quick succession.
Changes:
- Introduces an internal Promise queue (
enqueue) to run page operations sequentially. - Refactors chokidar event handlers to enqueue work instead of using
asynchandlers directly. - Removes the file-level ESLint disable for
@typescript-eslint/no-misused-promises.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent onPageUpdated-related races in the dev file-watching flow by serializing (and debouncing) page update handlers via a new debounced async queue utility.
Changes:
- Added
createDebouncedQueue()utility to serialize and debounce async operations, withflush()support. - Updated
watchPageFiles()to enqueue page/deps file events through the debounced queue and to return a{ watchers, flush }API. - Updated
devrestart logic to call the new flush function during restart; added unit tests for the queue.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/async/debouncedQueue.ts | Introduces a debounced, coalescing async queue with enqueue() and flush(). |
| packages/utils/src/async/index.ts | Exports the new async utility entrypoint. |
| packages/utils/src/index.ts | Re-exports async utilities from @vuepress/utils. |
| packages/utils/tests/async/debouncedQueue.spec.ts | Adds unit tests for queue ordering, coalescing, flush behavior, and error handling. |
| packages/cli/src/commands/dev/watchPageFiles.ts | Routes chokidar events through the debounced queue; returns watchers + flush. |
| packages/cli/src/commands/dev/dev.ts | Uses the new watchPageFiles() return shape and flushes pending work on restart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mister-Hope
force-pushed
the
page-update-racing
branch
from
afe5e63 to
c603a5c
Compare
Mister-Hope
changed the title
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a dev-mode race condition around onPageUpdated by serializing/coalescing page file events so page updates for the same path don’t run concurrently during rapid change bursts (including dependency-triggered updates).
Changes:
- Add per-page event queueing + merging (
add/change/unlink) and per-page promise chaining to serialize processing. - Change
watchPageFiles()to return{ watchers, cleanup }, wherecleanup()is intended to flush pending work. - Update
dev.tsto use the newwatchPageFiles()return shape and callcleanup()during restart.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/dev/watchPageFiles.ts | Introduces per-page event coalescing/serialization and adds a cleanup hook; changes exported API shape. |
| packages/cli/src/commands/dev/dev.ts | Adapts dev watcher setup/restart logic to the new watchPageFiles() API and calls its cleanup on restart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate race conditions during dev-mode page file updates by coalescing/serializing page events and avoiding processing intermediate filesystem states.
Changes:
- Added per-page pending-event coalescing (
mergeEvents) and deferred processing pipeline for add/change/unlink. - Introduced per-page serialization via a promise chain, and updated dependency-change handling to enqueue page changes instead of processing immediately.
- Updated
watchPageFiles()to return{ watchers, cleanup }, and invokedcleanup()during dev server restart.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/dev/watchPageFiles.ts | Adds event coalescing + per-page serialized processing and exposes a cleanup hook to coordinate shutdown/restart. |
| packages/cli/src/commands/dev/dev.ts | Adapts to the new watchPageFiles() return shape and runs page watcher cleanup during restart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI dev file-watching pipeline to reduce race conditions during rapid page/dependency changes by coalescing multiple chokidar events per page into a single serialized operation, and by adding a teardown step to flush/clean queued work before restarting the dev server.
Changes:
- Add per-page event coalescing (
mergeEvents) and per-page serialization (pagePromises) to avoid overlapping page add/change/unlink processing. - Route dependency watcher updates through the same page event pipeline (instead of directly calling
handlePageChange). - Change
watchPageFiles()to return{ watchers, cleanup }and callcleanup()during dev restart.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/dev/watchPageFiles.ts | Introduces event coalescing + per-page serialized processing, and returns a cleanup function alongside watchers. |
| packages/cli/src/commands/dev/dev.ts | Adapts dev command to new watchPageFiles() return shape and awaits cleanup during restart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // cancel queued page events, wait for in-flight operations to finish, and reset | ||
| const cleanup = async (): Promise<void> => { | ||
| // clear pending events | ||
| pendingEvents.clear() | ||
| // wait for all pending page operations to finish | ||
| await Promise.all(pagePromises.values()) | ||
| // clear pending promises | ||
| pagePromises.clear() |
There was a problem hiding this comment.
cleanup() clears pendingEvents, awaits current pagePromises, then clears pagePromises, but it does not prevent new events from being queued while cleanup() is awaiting. If cleanup() is called before/without closing the watchers, events can be enqueued during cleanup and then be dropped when pagePromises.clear() runs. Consider guarding pageEventHandler during cleanup (e.g. an isCleaningUp flag) or making cleanup() responsible for stopping the watchers (or at least documenting that watchers must be closed before calling cleanup).
| type PageEventType = 'add' | 'change' | 'unlink' | ||
|
|
||
| /** | ||
| * Merge pending events into final operation. | ||
| */ | ||
| const mergeEvents = (events: PageEventType[]): PageEventType | null => { | ||
| if (events.length === 0) return null | ||
|
|
||
| if (events.length === 1) return events[0] | ||
|
|
||
| const first = events[0] | ||
| const last = events[events.length - 1] | ||
|
|
||
| // add + ... + remove: nothing | ||
| if (first === 'add' && last === 'unlink') return null | ||
|
|
||
| if (first === 'add') return 'add' | ||
| if (last === 'unlink') return 'unlink' | ||
|
|
||
| return 'change' | ||
| } |
There was a problem hiding this comment.
The new event coalescing/serialization logic (mergeEvents, pendingEvents, pagePromises) is non-trivial and affects dev hot-update correctness, but there are no tests covering the merge behavior (e.g. add→change, unlink→add, add→unlink) or serialization guarantees. Since this package already uses vitest for dev helpers, consider adding unit tests for the merge/queue behavior (possibly by extracting the merge logic into a testable helper).
No description provided.