Skip to content

Conversation

@hatton
Copy link
Member

@hatton hatton commented Dec 30, 2025

This change is Reviewable

Copilot AI review requested due to automatic review settings December 30, 2025 23:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements comprehensive file watching for the development workflow by introducing a sophisticated LESS dependency tracking system and consolidating all file watchers into a unified yarn dev command.

Key Changes:

  • New LessWatchManager class that tracks transitive LESS imports and rebuilds dependents automatically
  • Unified dev.mjs script that orchestrates Vite, LESS, Pug, and static file watchers in a single process
  • Enhanced Pug compilation with dependency tracking to rebuild files when included partials change

Reviewed changes

Copilot reviewed 17 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/BloomBrowserUI/scripts/watchLessManager.js Core LESS watch manager with dependency graph tracking and incremental rebuilds
src/BloomBrowserUI/scripts/watchLess.mjs CLI wrapper for watch manager supporting content and browser-ui scopes
src/BloomBrowserUI/scripts/dev.mjs New unified dev script orchestrating Vite and all file watchers
src/BloomBrowserUI/scripts/compilePug.mjs Enhanced with dependency tracking to detect changes in included Pug files
src/BloomBrowserUI/scripts/compileLess.mjs Refactored to use watch manager for consistent LESS compilation
src/content/scripts/copyContentFile.mjs New utility for copying content files with timestamp-based change detection
src/BloomBrowserUI/scripts/copyStaticFile.mjs New utility for copying static files excluding build artifacts
src/BloomBrowserUI/vite.config.mts Simplified to delegate LESS compilation to new watch manager
src/content/package.json Replaced cpx watch commands with onchange-based watchers for better reliability
src/BloomBrowserUI/package.json Updated dev script and added onchange/chokidar dependencies
src/BloomBrowserUI/scripts/__tests__/watchLess.test.{ts,mjs} Comprehensive tests for watch manager functionality
src/BloomBrowserUI/scripts/__tests__/compilePug.test.ts Tests verifying Pug dependency tracking
src/BloomBrowserUI/scripts/build.js New unified build script replacing npm-run-all orchestration
src/BloomBrowserUI/scripts/clean.js Enhanced with quiet mode for cleaner output

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


main().catch((err) => {
console.error("Dev script failed:", err);
cleanup();
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup() function is called without passing the error exit code. When an error occurs in main(), the process should exit with a non-zero status. Pass 1 to indicate failure: cleanup(1).

Suggested change
cleanup();
cleanup(1);

Copilot uses AI. Check for mistakes.
Comment on lines 39 to 41
if (typeof args[0] === "string" && args[0].startsWith("[LESS] ✓")) {
compiled++;
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logger wraps console.log to count compiled files by checking if the log message starts with "[LESS] ✓". This is fragile and tightly couples the counting logic to the log message format. Consider having compileEntry return a status or having the manager expose a compilation count, rather than parsing log messages.

Suggested change
if (typeof args[0] === "string" && args[0].startsWith("[LESS] ✓")) {
compiled++;
}
compiled++;

Copilot uses AI. Check for mistakes.
await manager.initialize();
const entryId = getEntryId(manager, entryPath);
const cssPath = path.join(outputRoot, "main.css");
const baselineMTime = fs.statSync(cssPath).mtimeMs;
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable baselineMTime is declared with const but reassigned on line 124. This should be declared with let instead of const to allow reassignment.

Suggested change
const baselineMTime = fs.statSync(cssPath).mtimeMs;
let baselineMTime = fs.statSync(cssPath).mtimeMs;

Copilot uses AI. Check for mistakes.
Comment on lines 117 to 124
let baselineMTime = fs.statSync(cssPath).mtimeMs;

await new Promise((resolve) => setTimeout(resolve, 30));
writeFile(entryPath, "body { color: black; }\n");
await manager.handleFileChanged(entryPath, "entry updated");
const deps = manager.entryDependencies.get(entryId) ?? [];
expect(deps.length).toBe(1);
baselineMTime = fs.statSync(cssPath).mtimeMs;
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial value of baselineMTime is unused, since it is always overwritten.

Suggested change
let baselineMTime = fs.statSync(cssPath).mtimeMs;
await new Promise((resolve) => setTimeout(resolve, 30));
writeFile(entryPath, "body { color: black; }\n");
await manager.handleFileChanged(entryPath, "entry updated");
const deps = manager.entryDependencies.get(entryId) ?? [];
expect(deps.length).toBe(1);
baselineMTime = fs.statSync(cssPath).mtimeMs;
await new Promise((resolve) => setTimeout(resolve, 30));
writeFile(entryPath, "body { color: black; }\n");
await manager.handleFileChanged(entryPath, "entry updated");
const deps = manager.entryDependencies.get(entryId) ?? [];
expect(deps.length).toBe(1);
const baselineMTime = fs.statSync(cssPath).mtimeMs;

Copilot uses AI. Check for mistakes.
@hatton hatton force-pushed the bl-15641_more-vite-dev branch from af66281 to fefa970 Compare December 30, 2025 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants