Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a watch command to automatically rebuild Markdown files when changes are detected. The implementation uses chokidar for file watching with debouncing to batch rapid changes and includes an --once option useful for testing.
Key changes:
- Adds watch command with configurable debounce delay (default 100ms)
- Refactors existing CLI build logic into a reusable
buildOncefunction - Implements intelligent file filtering (only
.mdfiles in input, only.template.htmlfiles directly in template dir) - Adds comprehensive unit tests with mocked chokidar and e2e tests
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/watch.ts | New file implementing watch functionality with chokidar, including file filtering, debouncing, and build coordination |
| src/cli.ts | Refactors build logic into buildOnce helper, adds handleWatchCommand for watch command support, and adds input path existence check |
| tests/unit/watch.test.ts | New comprehensive unit tests for watch functionality using mocked chokidar |
| tests/e2e/cli.e2e.test.ts | Adds e2e tests for watch command with --once and --verbose flags, plus help text validation |
| vitest.config.ts | Explicitly includes src/**/*.ts in coverage to ensure watch.ts is covered |
| package.json | Adds chokidar ^5.0.0 as production dependency, reorders devDependencies alphabetically |
| package-lock.json | Updates lock file with chokidar 5.0.0 and its dependencies, creates nested chokidar 4.0.3 for tsup |
| if (once) { | ||
| watcher.close(); | ||
| } |
There was a problem hiding this comment.
This code is unreachable because the CLI implementation in handleWatchCommand (lines 250-255 in src/cli.ts) bypasses startWatch entirely when once is true. The CLI directly calls buildOnce and returns, so the watcher never starts with once=true in practice. This dead code should be removed to avoid confusion.
| // For --once mode, run initial build immediately | ||
| if (once) { | ||
| building = true; | ||
| try { | ||
| await onBuild(); | ||
| } catch (e) { | ||
| console.error("[watch] initial build error:"); | ||
| console.error(e); | ||
| } finally { | ||
| building = false; | ||
| watcher.close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ready event handler for once mode is dead code. In src/cli.ts lines 250-255, when once is true, the CLI bypasses startWatch entirely and just calls buildOnce directly. This means startWatch never receives once=true in production. The test on line 280-299 validates behavior that never executes in the actual CLI. Consider either removing this code or adjusting the CLI to actually use it.
| it("runs initial build and closes when once=true on ready", async () => { | ||
| const { startWatch } = await import("../../src/watch.js"); | ||
|
|
||
| const onBuild = vi.fn(async () => {}); | ||
|
|
||
| startWatch({ | ||
| inputAbs: "/repo/input", | ||
| templateAbs: "/repo/template", | ||
| outAbs: "/repo/out", | ||
| debounceMs: 0, | ||
| verbose: false, | ||
| once: true, | ||
| onBuild, | ||
| }); | ||
|
|
||
| await state.emitAsync("ready"); | ||
|
|
||
| expect(onBuild).toHaveBeenCalledTimes(1); | ||
| expect(state.watcher.close).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
This test validates dead code. The CLI implementation (handleWatchCommand in src/cli.ts lines 250-255) bypasses startWatch entirely when once is true, so this behavior never executes in production. Either the test should be removed, or the CLI should be changed to actually use startWatch with once=true.
| const isInInput = | ||
| (eventPath.startsWith(inputAbs + path.sep) || eventPath === inputAbs) && | ||
| isMd; |
There was a problem hiding this comment.
The condition eventPath === inputAbs will always be false here. When inputAbs is a directory, eventPath will be file paths within it, not the directory itself. When inputAbs is a file, chokidar won't emit events for it as inputAbs itself, but for the actual file path. This appears to be defensive code that never matches. Consider using the isUnderDir helper function instead for consistency with the output directory check.
| const isInInput = | |
| (eventPath.startsWith(inputAbs + path.sep) || eventPath === inputAbs) && | |
| isMd; | |
| const isInInput = isUnderDir(eventPath, inputAbs) && isMd; |
No description provided.