Refactor build pipeline for performance and dev output reliability #886
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds build-time environment knobs and CI cache-key changes, replaces Terser with esbuild for JS/CSS minification, integrates thread-loader and sass-embedded fallback, implements per-variant parallel build orchestration with temporary outputs and safer finalization, and updates dependencies and build function signatures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as CI / Dev
participant Orch as build.mjs
participant WP as Webpack
participant Cache as FS_Cache
participant Sass as sass-embedded/sass
participant Zip as Archiver
participant Out as build/*
User->>Orch: npm run build (env: BUILD_*, NODE_MAJOR)
Orch->>Orch: parse BUILD_PARALLEL, BUILD_THREAD*, workers, variantId
Orch->>Sass: import sass-embedded (fallback to sass)
Orch->>Cache: compute cache key (OS + lockfile + NODE_MAJOR + variantId)
alt BUILD_PARALLEL=true
Orch->>WP: start variant A (esbuild minify, thread-loader if enabled)
Orch->>WP: start variant B (esbuild minify, thread-loader if enabled)
else
Orch->>WP: build variant A -> .tmp-*
Orch->>WP: build variant B -> .tmp-*
end
WP-->>Cache: read/write per-variant cache
WP-->>Orch: emit artifacts to sourceBuildDir
Orch->>Zip: finalize & zip per-target artifacts
Zip-->>Out: produce build/*.zip
Orch-->>User: exit (0/non-zero)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @PeterDaveHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request comprehensively refactors the build pipeline to enhance performance, configurability, and developer experience. By leveraging modern tools and optimizing existing processes, it aims to drastically reduce build times for both local development and production environments, leading to a more efficient and responsive workflow.
Highlights
- Build Tooling Upgrade: Replaced JavaScript and CSS minifiers with esbuild for significantly faster build times, improving both development and production workflows.
- Parallel Processing & Threading: Enabled thread-loader by default for Babel with dynamic worker allocation based on CPU cores, and introduced parallel production variant builds, all configurable via new environment variables.
- Optimized Caching: Implemented configurable Webpack filesystem cache compression, defaulting to no compression for faster warm builds on CPU-bound machines, enhancing build efficiency.
- SASS/CSS Enhancements: Adopted sass-embedded for SASS processing and utilized style-loader for faster CSS/SCSS compilation in development, while ensuring production outputs remain unchanged.
- Development Experience Improvements: Ensured 'watch-once' development builds exit cleanly and added placeholder CSS files to prevent 404 errors and reduce noise during development.
- CI/CD Efficiency: Integrated dependency caching into GitHub Actions workflows to accelerate continuous integration and deployment runs.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the build pipeline for performance, introducing esbuild for minification, thread-loader for parallelism, and sass-embedded for SASS compilation. The changes are well-structured and bring substantial performance improvements. My review focuses on improving maintainability by reducing code duplication in the build script. I've identified two areas where helper functions could be introduced to make the code cleaner and more reusable: one for creating placeholder CSS files and another for handling the webpack build process.
PR Code Suggestions ✨Latest suggestions up to 5663e3c
Previous suggestions✅ Suggestions up to commit 6c89d29
✅ Suggestions up to commit abfc14d
✅ Suggestions up to commit b5f27c3
✅ Suggestions up to commit 0fdeae8
✅ Suggestions up to commit d9cc0eb
✅ Suggestions up to commit 7bad746
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
64-99: Add@babel/runtimeto package dependencies
Babel’s@babel/plugin-transform-runtimeis enabled in build.mjs (line 180) and listed under devDependencies, but@babel/runtimeis missing—this will cause runtime “module not found” errors.--- package.json +++ package.json @@ "dependencies": { + "@babel/runtime": "^7.24.7", }build.mjs (1)
173-193: @babel/plugin-transform-runtime is placed inside presets; it won’t runThe plugin configured under presets is ignored. Move it to plugins (and, if you keep it, ensure @babel/runtime is added to package.json dependencies).
options: { cacheDirectory: true, cacheCompression: false, - presets: [ - '@babel/preset-env', - { - plugins: ['@babel/plugin-transform-runtime'], - }, - ], - plugins: [ + presets: ['@babel/preset-env'], + plugins: [ + ['@babel/plugin-transform-runtime'], [ '@babel/plugin-transform-react-jsx', { runtime: 'automatic', importSource: 'preact', }, ], ], },Follow-up (package.json):
- Add "@babel/runtime" to dependencies if you keep the plugin, or remove the plugin here to avoid introducing a runtime dep.
🧹 Nitpick comments (3)
package.json (1)
91-97: sass + sass-embedded together: consider dropping onebuild.mjs uses sass-embedded as the loader implementation. Keeping both sass and sass-embedded increases install size; unless another tool requires sass, consider removing sass to slim CI installs. If you do need both, no action needed.
.github/workflows/pre-release-build.yml (1)
35-43: Broaden the webpack cache key to include build.mjsIncluding build.mjs in the cache key avoids restoring a cache that webpack will immediately invalidate due to buildDependencies. This saves time and bandwidth.
Apply:
- key: ${{ runner.os }}-webpack-${{ hashFiles('**/package-lock.json') }} + key: ${{ runner.os }}-webpack-${{ hashFiles('**/package-lock.json', 'build.mjs') }} - restore-keys: | - ${{ runner.os }}-webpack- + restore-keys: | + ${{ runner.os }}-webpack-.github/copilot-instructions.md (1)
33-35: Tiny grammar/polishMake the performance defaults sentence crisper.
-Performance defaults: esbuild is used for JS/CSS minification; dev injects CSS via style-loader, -prod extracts CSS via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod. +Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader, +in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/pre-release-build.yml(1 hunks)build.mjs(12 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~33-~33: There might be a mistake here.
Context: ...ation; dev injects CSS via style-loader, prod extracts CSS via MiniCssExtractPlug...
(QB_NEW_EN)
🔇 Additional comments (8)
package.json (1)
77-79: Verify esbuild-loader 4.3.0 compatibility with webpack ^5.92.1 and Node 20
package.json lists:
• webpack ^5.92.1
• esbuild ^0.25.9
• esbuild-loader ^4.3.0
Ensure your CI (running Node v20.x) installs these exact versions and completes a full webpack build without errors (for example,npm ci && npm run build), or runnpm ls webpack esbuild-loaderto confirm no version conflicts..github/workflows/pre-release-build.yml (1)
27-34: NPM cache step: LGTMPositioning before npm ci and scoping to ~/.npm is correct for faster restores without locking node_modules.
.github/copilot-instructions.md (1)
20-32: Docs match implementationThe documented BUILD_PARALLEL, BUILD_THREAD/BUILD_THREAD_WORKERS, BUILD_CACHE_COMPRESSION, and BUILD_WATCH_ONCE flags align with build.mjs behavior. Nice.
build.mjs (5)
92-101: Switch to esbuild for JS/CSS minification: LGTMConfiguration and targets look sane; legalComments: 'none' keeps bundles lean, and CSS uses esbuildMinify via css-minimizer.
160-171: Threading defaults and poolTimeout handling are appropriateEnabling thread-loader by default with CPU-derived workers and using poolTimeout = 0 for BUILD_WATCH_ONCE is a good balance for dev/prod.
364-375: Dev CSS placeholders are a nice touchAvoiding 404 noise when using style-loader in development improves DX without affecting production.
Also applies to: 385-395
237-246: Font asset handling: confirm fallback behaviorwoff2 is inlined, while woff/ttf are marked emit: false. That’s fine for modern Chromium/Firefox, but verify there’s no runtime path that requests woff/ttf (e.g., older Firefox ESR or custom user agents). If needed, either remove emit:false or conditionally emit only for production.
Suggested tweak if you need to emit in prod:
- { - test: /\.(woff|ttf)$/, - type: 'asset/resource', - generator: { - emit: false, - }, - }, + { + test: /\.(woff|ttf)$/, + type: 'asset/resource', + generator: { emit: !isProduction ? false : true }, + },
82-90: Filesystem cache compression control: LGTMEnv-driven compression with a safe default (disabled) aligns with the perf objective and warm-build wins.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@codex review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| output: { | ||
| filename: '[name].js', | ||
| path: path.resolve(__dirname, outdir), | ||
| path: path.resolve(__dirname, sourceBuildDir || outdir), | ||
| }, | ||
| mode: isProduction ? 'production' : 'development', | ||
| devtool: isProduction ? false : 'inline-source-map', | ||
| devtool: isProduction ? false : 'cheap-module-source-map', | ||
| cache: { |
There was a problem hiding this comment.
[P2] Copy emitted source maps into dev build output
Development builds switched devtool to cheap-module-source-map, which emits external *.js.map files. However the packaging step only copies the JS and CSS assets into build/chromium*/build/firefox* and never copies the generated source map files. When loading the unpacked extension in a browser the bundles will reference content-script.js.map, popup.js.map, etc., but those files are missing, so DevTools reports 404s and source maps are unusable. Either copy the .map files alongside the bundles or keep inline source maps to avoid the broken references.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
build.mjs (4)
35-56: Thread worker calculation has proper bounds checking.The logic for determining thread workers is robust with proper validation and capping to CPU count. Good defensive programming.
57-58: Consider making the production pool timeout configurable.The hardcoded 2000ms timeout could be made configurable for different environments.
-const PRODUCTION_POOL_TIMEOUT_MS = 2000 +const PRODUCTION_POOL_TIMEOUT_MS = process.env.BUILD_POOL_TIMEOUT ? + parseInt(process.env.BUILD_POOL_TIMEOUT, 10) : 2000
372-385: Add logging for skipped files in development.When files are skipped (especially CSS placeholders), it would be helpful to log this for debugging purposes.
if (!isProduction && isCss) return + console.log(`[build] Skipping missing CSS file: ${entryPoint.src} (placeholder will be created)`)
456-508: Parallel build implementation with temporary directories is well-structured.The parallel and sequential build paths are properly implemented with appropriate cleanup of temporary directories. The error handling ensures failed builds are properly reported.
🧹 Nitpick comments (4)
package.json (1)
92-94: Consider pinning sass-embedded to match sass version.While both
sassandsass-embeddedare at^1.91.0, having separate version specs could lead to divergence. Consider using a single shared version constraint or removing the redundantsasspackage ifsass-embeddedis the primary implementation.If
sass-embeddedis the primary Sass implementation being used inbuild.mjs, you could remove the regularsasspackage:- "sass": "^1.91.0", "sass-embedded": "^1.91.0",.github/workflows/pre-release-build.yml (1)
29-35: Consider adding npm cache version to the cache key.The npm cache key only includes the OS and package-lock hash. If npm itself is upgraded, the cache structure might change. Consider including npm version in the key.
- key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} + key: ${{ runner.os }}-npm-v${{ steps.npm-version.outputs.version }}-${{ hashFiles('**/package-lock.json') }}You would need to add a step to capture npm version:
- name: Get npm version id: npm-version run: echo "version=$(npm -v)" >> $GITHUB_OUTPUT.github/copilot-instructions.md (1)
33-34: Fix minor grammar issue in the documentation.There's a minor grammar issue flagged by static analysis.
-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader, -in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod. +Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader; +in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.build.mjs (1)
20-34: Consider extracting cache compression validation to a helper function.The cache compression validation logic is quite verbose and could be simplified.
-const cacheCompressionEnv = process.env.BUILD_CACHE_COMPRESSION -let cacheCompressionOption -if (cacheCompressionEnv != null) { - const v = String(cacheCompressionEnv).trim().toLowerCase() - if (v === '' || v === '0' || v === 'false' || v === 'none') { - cacheCompressionOption = false - } else if (v === 'gzip' || v === 'brotli') { - cacheCompressionOption = v - } else { - console.warn( - `[build] Unknown BUILD_CACHE_COMPRESSION="${cacheCompressionEnv}", defaulting to no compression`, - ) - cacheCompressionOption = false - } -} +function parseCacheCompression(env) { + if (env == null) return undefined + const v = String(env).trim().toLowerCase() + if (['', '0', 'false', 'none'].includes(v)) return false + if (['gzip', 'brotli'].includes(v)) return v + console.warn(`[build] Unknown BUILD_CACHE_COMPRESSION="${env}", defaulting to no compression`) + return false +} +const cacheCompressionOption = parseCacheCompression(process.env.BUILD_CACHE_COMPRESSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/pre-release-build.yml(1 hunks)build.mjs(12 hunks)package.json(3 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~33-~33: There might be a mistake here.
Context: ...opment CSS is injected via style-loader, in production CSS is extracted via MiniC...
(QB_NEW_EN)
🔇 Additional comments (12)
package.json (3)
78-79: Ensure esbuild is actually installed before the build runs.Adding esbuild and esbuild-loader to devDependencies is correct for the new minification pipeline.
96-97: LGTM! Build performance tooling additions.The addition of
style-loaderfor development builds andthread-loaderfor parallel processing aligns well with the performance optimization goals.
22-22: Keep @babel/runtime in dependencies
@babel/plugin-transform-runtimerewrites your code to import helpers (and regenerator runtime by default) from@babel/runtime, so it must remain independencies, notdevDependencies.Likely an incorrect or invalid review comment.
.github/workflows/pre-release-build.yml (2)
27-28: Node major version detection looks good.The approach for detecting Node major version is correct and will properly differentiate caches across Node versions.
37-45: Webpack cache configuration is well-structured.The cache paths and key structure appropriately include Node major version, which is important for native dependencies that might be compiled differently across Node versions.
.github/copilot-instructions.md (1)
22-31: Documentation of build performance options is comprehensive.The new build performance options are well-documented with clear defaults and use cases. The BUILD_PARALLEL and BUILD_THREAD options provide good control over resource usage.
build.mjs (6)
17-18: LGTM! Parallel build control implementation.The parallel build toggle with sensible default (on) provides good flexibility for different build environments.
82-82: Good fix for sass-embedded import issue.The destructuring of the default export properly addresses the previous review comment about sass-loader expecting the implementation object.
129-136: Excellent choice using esbuild for minification.Using esbuild for both JS and CSS minification should provide significant build performance improvements compared to Terser.
196-211: Thread-loader configuration is well-implemented.The conditional inclusion of thread-loader with appropriate pool timeout settings for different scenarios (production, watch-once, development) is well thought out.
345-354: Watch-once implementation is clean and correct.The implementation properly closes the watcher after the first build when BUILD_WATCH_ONCE is set.
417-427: Good solution for CSS 404 noise in development.Creating placeholder CSS files in development mode is a thoughtful solution to avoid console noise from 404 errors.
Also applies to: 437-446
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
build.mjs (6)
17-17: Simplify boolean logic forparallelBuildThe ternary operator with explicit boolean values is unnecessarily verbose.
-const parallelBuild = process.env.BUILD_PARALLEL === '0' ? false : true +const parallelBuild = process.env.BUILD_PARALLEL !== '0'
35-42: Simplify CPU count fallback logicThe CPU count determination logic is overly complex and can be simplified using Math.max.
-let cpuCount = 1 -try { - const cpuInfo = os.cpus && os.cpus() - const len = Array.isArray(cpuInfo) ? cpuInfo.length : 0 - cpuCount = Number.isInteger(len) && len > 0 ? len : 1 -} catch { - cpuCount = 1 -} +const cpuCount = Math.max(1, os.cpus()?.length || 1)
130-130: Verify source map configuration change impactThe change from 'inline-source-map' to 'cheap-module-source-map' improves build performance but creates external .map files that need to be copied in development builds.
#!/bin/bash # Check if .map files are generated and if there's any handling for them echo "Checking for source map handling in development builds..." rg -n "\.map" build.mjs rg -n "source-map" build.mjs # Check if copyFiles function handles .map files ast-grep --pattern 'copyFiles($$$)'
506-558: Consider extracting duplicated webpack build logicThe production build contains significant code duplication between parallel and sequential builds. This pattern of wrapping runWebpack in promises is repeated multiple times.
Consider refactoring to reduce duplication:
+function createWebpackBuildPromise(isWithoutKatex, isWithoutTiktoken, minimal, tmpDir, suffix) { + return new Promise((resolve, reject) => + runWebpack(isWithoutKatex, isWithoutTiktoken, minimal, tmpDir, async (err, stats) => { + if (err || stats.hasErrors()) { + console.error(err || stats.toString()) + reject(err || new Error('webpack error')) + return + } + await finishOutput(suffix, tmpDir) + resolve() + }) + ) +} async function build() { await deleteOldDir() if (isProduction && !isAnalyzing) { const tmpFull = `${outdir}/.tmp-full` const tmpMin = `${outdir}/.tmp-min` + + const buildMin = () => createWebpackBuildPromise(true, true, true, tmpMin, '-without-katex-and-tiktoken') + const buildFull = () => createWebpackBuildPromise(false, false, false, tmpFull, '') + if (parallelBuild) { - await Promise.all([ - new Promise((resolve, reject) => - runWebpack(true, true, true, tmpMin, async (err, stats) => { - // ... duplicated code - }), - ), - new Promise((resolve, reject) => - runWebpack(false, false, false, tmpFull, async (err, stats) => { - // ... duplicated code - }), - ), - ]) + await Promise.all([buildMin(), buildFull()]) } else { - await new Promise((resolve, reject) => - runWebpack(true, true, true, tmpMin, async (err, stats) => { - // ... duplicated code - }), - ) - await new Promise((resolve, reject) => - runWebpack(false, false, false, tmpFull, async (err, stats) => { - // ... duplicated code - }), - ) + await buildMin() + await buildFull() } await fs.rm(tmpFull, { recursive: true, force: true }) await fs.rm(tmpMin, { recursive: true, force: true }) return } // ... rest of the function }
98-104: Consider using a more stable cache keyIncluding the full sourceBuildDir path in the cache key may cause unnecessary cache invalidation when build directories change between runs.
const variantId = [ isWithoutKatex ? 'no-katex' : 'with-katex', isWithoutTiktoken ? 'no-tiktoken' : 'with-tiktoken', minimal ? 'minimal' : 'full', - sourceBuildDir || outdir, + path.basename(sourceBuildDir || outdir), isProduction ? 'prod' : 'dev', ].join('|')
420-436: Critical: Build may silently skip missing artifactsThe copyFiles function now silently skips missing files when
fs.pathExistsreturns false, which could lead to incomplete builds being published without any failure signal. While CSS placeholders in dev mode are acceptable, critical JS files should never be skipped silently.Consider failing the build for missing critical files:
async function copyFiles(entryPoints, targetDir) { if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true }) await Promise.all( entryPoints.map(async (entryPoint) => { try { if (await fs.pathExists(entryPoint.src)) { await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`) } else { - // Skip missing CSS in development (placeholders will be created later) const isCss = String(entryPoint.dst).endsWith('.css') - if (!isProduction || isCss) { - if (!isProduction && isCss) return + // Only skip CSS files in development (placeholders will be created) + if (!isProduction && isCss) { + console.log(`Skipping missing CSS in dev: ${entryPoint.src}`) + return } + // For all other files or production builds, this is a critical error throw new Error(`Missing build artifact: ${entryPoint.src}`) } } catch (e) { console.error('Copy failed:', entryPoint, e) throw e } }), ) }
🧹 Nitpick comments (1)
build.mjs (1)
401-415: Add error handling for directory creation in zipFolderThe zipFolder function should ensure the parent directory exists before creating the zip file.
The implementation correctly uses
fs.ensureDirto create parent directories, and the Promise-based error handling is comprehensive. Good implementation!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
build.mjs(11 hunks)
🔇 Additional comments (6)
build.mjs (6)
89-96: LGTM! Sass implementation import fixed correctlyThe sass-embedded import now correctly uses the default export, which resolves the previous runtime error issue. The fallback to sass is also properly implemented.
364-399: LGTM! Proper compiler lifecycle managementThe compiler is now properly closed after production runs, preventing resource leaks. The watch mode also correctly handles the BUILD_WATCH_ONCE scenario with appropriate exit codes.
467-477: LGTM! Good solution for CSS 404 noise in developmentThe placeholder CSS files effectively prevent 404 errors in development mode without affecting production builds.
Also applies to: 487-497
149-156: LGTM! Effective esbuild integration for minificationThe migration from Terser to esbuild for both JS and CSS minification is well-implemented and should provide significant build performance improvements as reported in the PR objectives.
216-230: LGTM! Well-configured thread-loader integrationThe thread-loader configuration with dynamic worker allocation based on CPU cores and appropriate pool timeouts for different build scenarios is excellent.
255-255: LGTM! Smart CSS handling for dev vs productionUsing style-loader in development and MiniCssExtractPlugin in production is a best practice that speeds up development builds while maintaining proper CSS extraction for production.
Also applies to: 271-271, 286-286
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@codex review |
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
PeterDaveHello
force-pushed
the
BuildOptimization
branch
from
ee5c711 to
4e0e3a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
build.mjs (3)
91-98: Stabilize cache key: avoid absolute paths in variantId to improve cache hits across environmentsUsing sourceBuildDir or outdir directly can include absolute paths, fragmenting cache keys between CI/local. Normalize to a relative path.
const variantId = [ isWithoutKatex ? 'no-katex' : 'with-katex', isWithoutTiktoken ? 'no-tiktoken' : 'with-tiktoken', minimal ? 'minimal' : 'full', - sourceBuildDir || outdir, + path.relative(process.cwd(), sourceBuildDir || outdir), isProduction ? 'prod' : 'dev', ].join('|')
376-389: Fix watch-once: await async callback before closing watcher/exitingWhen BUILD_WATCH_ONCE is set, the code exits before async work in the callback (e.g., finishOutput) completes, yielding partial outputs. Await the returned promise before closing the watcher and exiting.
- const watching = compiler.watch({}, (err, stats) => { + const watching = compiler.watch({}, (err, stats) => { const hasErrors = !!( err || (stats && typeof stats.hasErrors === 'function' && stats.hasErrors()) ) - callback(err, stats) - if (process.env.BUILD_WATCH_ONCE) { - watching.close((closeErr) => { - if (closeErr) console.error('Error closing watcher:', closeErr) - // Exit explicitly to prevent hanging processes in CI - // Use non-zero exit code when errors occurred - process.exit(hasErrors || closeErr ? 1 : 0) - }) - } + const ret = callback(err, stats) + const finalize = () => + watching.close((closeErr) => { + if (closeErr) console.error('Error closing watcher:', closeErr) + process.exit(hasErrors || closeErr ? 1 : 0) + }) + if (process.env.BUILD_WATCH_ONCE) { + if (ret && typeof ret.then === 'function') ret.then(finalize, finalize) + else finalize() + } })
123-124: Dev source maps: either copy .map files or revert to inline to avoid 404s in unpacked extensionWith devtool set to cheap-module-source-map, bundles reference external *.js.map files, but finishOutput doesn’t copy them, causing broken source maps in the browser. Choose one:
Option A — copy maps:
@@ - devtool: isProduction ? false : 'cheap-module-source-map', + devtool: isProduction ? false : 'cheap-module-source-map', @@ -async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) { +async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) { + const mapFiles = isProduction + ? [] + : [ + { src: `${sourceBuildDir}/shared.js.map`, dst: 'shared.js.map' }, + { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' }, + { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' }, + { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' }, + { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' }, + ] @@ - await copyFiles( - [...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }], + await copyFiles( + [...commonFiles, ...mapFiles, { src: 'src/manifest.json', dst: 'manifest.json' }], chromiumOutputDir, ) @@ - await copyFiles( - [...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }], + await copyFiles( + [...commonFiles, ...mapFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }], firefoxOutputDir, )Option B — revert to inline source maps in dev:
- devtool: isProduction ? false : 'cheap-module-source-map', + devtool: isProduction ? false : 'inline-source-map',Run to confirm maps are included after applying Option A:
#!/bin/bash rg -n "js\.map" -C2 build.mjs && echo "---" && rg -n "shared\.js\.map|content-script\.js\.map|background\.js\.map|popup\.js\.map|IndependentPanel\.js\.map" -C1Also applies to: 438-456, 460-463, 480-483
🧹 Nitpick comments (6)
build.mjs (3)
411-411: Prefer fs.ensureDir for idempotent directory creationSlightly simpler and race-safe.
- if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true }) + await fs.ensureDir(targetDir)
187-187: Document resolve.symlinks: false and its implicationsThis can affect npm/pnpm link and workspace symlinks. Add a note in docs (Build Performance or Bootstrap) about potential impact and workarounds.
499-571: Deduplicate build variant orchestration to reduce boilerplateThe Promise wrapping for the two production variants is repeated. Extract a small helper to run webpack and finishOutput to cut duplication and lower error surface.
Example:
function webpackRunPromise(args, suffix, dir) { return new Promise((resolve, reject) => runWebpack(...args, async (err, stats) => { if (err || stats.hasErrors()) { console.error(err || stats.toString()) reject(err || new Error('webpack error')) return } await finishOutput(suffix, dir) resolve() }), ) } // Usage: const buildMin = () => webpackRunPromise([true, true, true, tmpMin], '-without-katex-and-tiktoken', tmpMin) const buildFull = () => webpackRunPromise([false, false, false, tmpFull], '', tmpFull).github/copilot-instructions.md (3)
33-35: Tighten grammar/punctuation for clarityAdd commas around parenthetical phrases.
-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader; -in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod. +Performance defaults: esbuild handles JS/CSS minification; in development, CSS is injected via style-loader; +in production, CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod.
20-32: Document symlink behavior (resolve.symlinks: false) and local-link caveatsAdd a short note so contributors using npm/pnpm link aren’t surprised.
Suggested addition:
- Note: Webpack is configured with resolve.symlinks: false to speed up/normalize module resolution. If you use npm link or pnpm workspaces with symlinked deps, local changes may not be picked up as expected. Workarounds: avoid symlinked deps for this repo, or temporarily enable symlink resolution in build.mjs while developing the linked package.
36-47: Mention dev source maps location/expectationsSince dev builds emit external source maps, note that .map files are included in build/* outputs to keep DevTools happy.
Suggested addition:
- Development: Bundles reference external source maps (.js.map). These files are copied into build/chromium/ and build/firefox*/ so DevTools can load them without 404s. If you don’t need source maps, set BUILD_SOURCEMAPS=0 (future option) or switch to inline maps locally.
Also applies to: 104-117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/pre-release-build.yml(1 hunks)build.mjs(11 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .github/workflows/pre-release-build.yml
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~33-~33: There might be a mistake here.
Context: ...opment CSS is injected via style-loader; in production CSS is extracted via MiniC...
(QB_NEW_EN)
🔇 Additional comments (4)
build.mjs (4)
80-89: Correct sass-embedded default export usage — LGTMUsing the default export ensures sass-loader receives the implementation object; fallback to sass is correct.
35-55: Thread worker parsing/capping — LGTMPositive integer validation, CPU cap, and warnings look solid. Good defaulting to cpuCount.
56-59: Thread-loader timeout constant — LGTMNamed constant resolves the magic number. Pool timeout choices make sense (2s prod, 0 for watch-once, Infinity in dev).
Also applies to: 209-224
404-407: Remove .catch on archiver.finalize() (returns void) to avoid sync TypeError and broken zipsarchiver.finalize() does not return a promise; calling .catch throws synchronously and can break the zip step.
- archive.directory(dir, false) - archive.finalize().catch(onError) + archive.directory(dir, false) + try { + archive.finalize() + } catch (err) { + onError(err) + }Likely an incorrect or invalid review comment.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
build.mjs (3)
423-449: Avoid TOCTOU on copy; let fs.copy fail and handle ENOENT.Removes the race between pathExists and copy while keeping dev CSS skip behavior.
async function copyFiles(entryPoints, targetDir) { if (!fs.existsSync(targetDir)) await fs.mkdir(targetDir, { recursive: true }) await Promise.all( entryPoints.map(async (entryPoint) => { - try { - if (await fs.pathExists(entryPoint.src)) { - await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`) - } else { - // Skip missing CSS in development (placeholders will be created later) - const isCss = String(entryPoint.dst).endsWith('.css') - if (!isProduction || isCss) { - if (!isProduction && isCss) { - console.log( - `[build] Skipping missing CSS file: ${entryPoint.src} -> ${entryPoint.dst} (placeholder will be created)`, - ) - return - } - } - throw new Error(`Missing build artifact: ${entryPoint.src}`) - } - } catch (e) { - console.error('Copy failed:', entryPoint, e) - throw e - } + try { + await fs.copy(entryPoint.src, `${targetDir}/${entryPoint.dst}`) + } catch (e) { + const isCss = String(entryPoint.dst).endsWith('.css') + if (e && e.code === 'ENOENT') { + if (!isProduction && isCss) { + console.log( + `[build] Skipping missing CSS file: ${entryPoint.src} -> ${entryPoint.dst} (placeholder will be created)`, + ) + return + } + console.error('Missing build artifact:', entryPoint.src) + } else { + console.error('Copy failed:', entryPoint, e) + } + throw e + } }), ) }
451-469: Dev source maps aren’t copied; DevTools shows 404s. Add .map copies in dev.Since devtool is
cheap-module-source-map, include emitted maps alongside bundles.async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) { - const commonFiles = [ + const commonFiles = [ { src: 'src/logo.png', dst: 'logo.png' }, { src: 'src/rules.json', dst: 'rules.json' }, { src: `${sourceBuildDir}/shared.js`, dst: 'shared.js' }, { src: `${sourceBuildDir}/content-script.css`, dst: 'content-script.css' }, // shared { src: `${sourceBuildDir}/content-script.js`, dst: 'content-script.js' }, { src: `${sourceBuildDir}/background.js`, dst: 'background.js' }, { src: `${sourceBuildDir}/popup.js`, dst: 'popup.js' }, { src: `${sourceBuildDir}/popup.css`, dst: 'popup.css' }, { src: 'src/popup/index.html', dst: 'popup.html' }, { src: `${sourceBuildDir}/IndependentPanel.js`, dst: 'IndependentPanel.js' }, { src: 'src/pages/IndependentPanel/index.html', dst: 'IndependentPanel.html' }, - ] + // Dev-only: copy external source maps for CSP-safe debugging + ...(isProduction + ? [] + : [ + { src: `${sourceBuildDir}/content-script.js.map`, dst: 'content-script.js.map' }, + { src: `${sourceBuildDir}/background.js.map`, dst: 'background.js.map' }, + { src: `${sourceBuildDir}/popup.js.map`, dst: 'popup.js.map' }, + { src: `${sourceBuildDir}/IndependentPanel.js.map`, dst: 'IndependentPanel.js.map' }, + ]), + ]
370-404: Watch-once may exit before async finishOutput completes. Await the callback.Process can terminate early, leaving partial outputs. Chain the promise before closing watcher/exiting.
} else { - const watching = compiler.watch({}, (err, stats) => { + const watching = compiler.watch({}, (err, stats) => { const hasErrors = !!( err || (stats && typeof stats.hasErrors === 'function' && stats.hasErrors()) ) - callback(err, stats) + const ret = callback(err, stats) if (process.env.BUILD_WATCH_ONCE) { - watching.close((closeErr) => { - if (closeErr) console.error('Error closing watcher:', closeErr) - // Exit explicitly to prevent hanging processes in CI - // Use non-zero exit code when errors occurred - process.exit(hasErrors || closeErr ? 1 : 0) - }) + const finalize = () => + watching.close((closeErr) => { + if (closeErr) console.error('Error closing watcher:', closeErr) + process.exit(hasErrors || closeErr ? 1 : 0) + }) + if (ret && typeof ret.then === 'function') { + ret.then(finalize, finalize) + } else { + finalize() + } } }) }
🧹 Nitpick comments (5)
.github/copilot-instructions.md (3)
32-33: Fix list indentation to satisfy markdownlint (MD005/MD007).Align top-level and sub-bullets consistently.
- - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing) - - BUILD_POOL_TIMEOUT: Override thread-loader production pool timeout (ms) - - Default: `2000`. Increase if workers recycle too aggressively on slow machines/CI + - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing) +- BUILD_POOL_TIMEOUT: Override thread-loader production pool timeout (ms) + - Default: `2000`. Increase if workers recycle too aggressively on slow machines/CI
35-37: Tighten grammar and readability in “Performance defaults.”Capitalize clauses and use commas/periods for clarity.
-Performance defaults: esbuild handles JS/CSS minification; in development CSS is injected via style-loader; -in production CSS is extracted via MiniCssExtractPlugin; thread-loader is enabled by default in both dev and prod. +Performance defaults: esbuild handles JS/CSS minification. In development, CSS is injected via style-loader; +in production, CSS is extracted via MiniCssExtractPlugin. Thread-loader is enabled by default in both dev and prod.
20-37: Document source map behavior (external .map files in dev).Dev builds use external source maps; call out that .map files are written next to bundles to avoid DevTools 404s and that production builds omit them.
- BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing) +- Source maps (dev): External `.map` files are emitted next to JS bundles for CSP‑safe debugging; production builds disable source maps.build.mjs (2)
136-151: Cache invalidation: include lockfiles/package.json in filesystem cache deps.Without these, cache may go stale when deps change.
cache: { type: 'filesystem', name: `webpack-${variantId}`, version: JSON.stringify({ THREAD: enableThread, WORKERS: threadWorkers, CACHE_COMP: cacheCompressionOption ?? false, PROD: isProduction, }), // default none; override via BUILD_CACHE_COMPRESSION=gzip|brotli compression: cacheCompressionOption ?? false, buildDependencies: { - config: [path.resolve('build.mjs')], + config: [ + path.resolve('build.mjs'), + path.resolve('package.json'), + path.resolve('package-lock.json'), + path.resolve('pnpm-lock.yaml'), + path.resolve('yarn.lock'), + ], }, },
199-205: Resolve.symlinks=false can surprise linked dev setups; document it.Add a short comment here and a note in docs about pnpm/npm link implications and why it’s set.
Would you like me to add a brief blurb under “Build Performance Options” explaining this choice?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/copilot-instructions.md(1 hunks)build.mjs(11 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~35-~35: There might be a mistake here.
Context: ...opment CSS is injected via style-loader; in production CSS is extracted via MiniC...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.github/copilot-instructions.md
32-32: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
32-32: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (3)
build.mjs (3)
214-237: Thread-loader config looks solid.Good defaults, CPU-aware worker cap, and poolTimeout tuning for dev/watch-once.
56-67: Good: pool timeout is named and env-overridable.Clear default with validation; matches docs.
350-366: String-replace edits safe—no dangling syntax
Replacements only target standalone import/call lines insrc/background/index.mjs, producing valid comment lines without stray commas or bindings, and the fonts import insrc/content-script/styles.scssis present.
|
@codex review |
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
|
/gemini review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
build.mjs (7)
17-19: Nit: clarify boolean naming.Rename to improve readability.
-const parallelBuild = process.env.BUILD_PARALLEL !== '0' +const isParallelBuild = process.env.BUILD_PARALLEL !== '0' // ...later: - if (parallelBuild) { + if (isParallelBuild) {
20-29: Simplify cache compression parsing and remove nullish fallback.Return false for unspecified values; drop "?? false" at use site.
-function parseCacheCompressionOption(envVal) { - if (envVal == null) return undefined +function parseCacheCompressionOption(envVal) { + if (envVal == null) return false const v = String(envVal).trim().toLowerCase() if (v === '' || v === '0' || v === 'false' || v === 'none') return false if (v === 'gzip' || v === 'brotli') return v console.warn(`[build] Unknown BUILD_CACHE_COMPRESSION="${envVal}", defaulting to no compression`) return false } const cacheCompressionOption = parseCacheCompressionOption(process.env.BUILD_CACHE_COMPRESSION) @@ - compression: cacheCompressionOption ?? false, + compression: cacheCompressionOption,Also applies to: 145-145
31-36: Tighten CPU detection.
os.cpus()always returns an array; simplify.-let cpuCount = 1 -try { - const cpuInfo = os.cpus && os.cpus() - const len = Array.isArray(cpuInfo) ? cpuInfo.length : 0 - cpuCount = Number.isInteger(len) && len > 0 ? len : 1 -} catch { - cpuCount = 1 -} +let cpuCount = 1 +try { + cpuCount = Math.max(1, os.cpus().length) +} catch {}
90-101: Log which Sass implementation is used (helps debugging).Emit a one-line info or warning on fallback.
let sassImpl try { const mod = await import('sass-embedded') sassImpl = mod.default || mod + console.log('[build] Using sass-embedded') } catch (e1) { try { const mod = await import('sass') sassImpl = mod.default || mod + console.warn('[build] sass-embedded unavailable, falling back to sass:', e1?.message || e1) } catch (e2) { throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") } }
203-206: Consider making symlink resolution configurable.Symlinks: false can break npm link/pnpm workspaces. Gate with an env var defaulting to false.
- symlinks: false, + symlinks: process.env.WEBPACK_SYMLINKS === '1' ? true : false,
423-461: Simplify zip promise; avoid manual settled flag.Use finish/close handlers directly; less state, same safety.
- await new Promise((resolve, reject) => { + await new Promise((resolve, reject) => { const output = fs.createWriteStream(zipPath) const archive = archiver('zip', { zlib: { level: 9 } }) - let settled = false - const fail = (err) => { - if (!settled) { - settled = true - reject(err) - } - } - const done = () => { - if (!settled) { - settled = true - resolve() - } - } - output.once('error', fail) - archive.once('error', fail) + output.once('error', reject) + archive.once('error', reject) archive.on('warning', (err) => { // Log non-fatal archive warnings for diagnostics console.warn('[build][zip] warning:', err) }) - // Resolve on close to ensure FD is flushed and closed - output.once('close', done) - // Ensure close is emitted after finish on some fast runners - output.once('finish', () => { - try { - if (typeof output.close === 'function') output.close() - } catch (_) { - // ignore - } - }) + output.once('close', resolve) // resolve after FD is closed archive.pipe(output) archive.directory(dir, false) archive.finalize() })
468-483: Prefer path.extname over String().endsWith for CSS detection.More robust and intention-revealing.
- const isCss = String(entryPoint.dst).endsWith('.css') + const isCss = path.extname(entryPoint.dst) === '.css'
🧹 Nitpick comments (3)
package.json (2)
1-21: Add an engines field to prevent CI/runner mismatches with esbuild.esbuild ^0.25 ships native binaries and requires a modern Node. Declare the minimum Node in package.json (and ensure CI matches).
{ "name": "chatgptbox", + "engines": { + "node": ">=18" + }, "scripts": {#!/bin/bash # Verify CI uses a compatible Node version fd -a ".github/workflows/*.yml" | xargs -r rg -n "node-version:"
92-95: Confirm if both 'sass-embedded' and 'sass' are needed.build.mjs prefers sass-embedded with a fallback to sass. If you intend to always use sass-embedded, consider dropping 'sass' to reduce install surface; otherwise keep both and document the fallback.
#!/bin/bash # Check for any direct 'sass' imports outside build.mjs rg -nP "from\s+['\"]sass['\"]|require\(['\"]sass['\"]\)"Also applies to: 96-97, 97-98
build.mjs (1)
599-614: Dev watch exits on first compile error—confirm intent.Current flow rejects and process exits on initial error; previous behavior typically stayed in watch to recover. If unintended, change to log error and continue unless BUILD_WATCH_ONCE=1.
- if (err || (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())) { - console.error(err || stats.toString()) - reject(err || new Error('webpack error')) - return - } + if (err || (stats && typeof stats.hasErrors === 'function' && stats.hasErrors())) { + if (process.env.BUILD_WATCH_ONCE === '1') { + console.error(err || stats.toString()) + reject(err || new Error('webpack error')) + } else { + console.error('[watch] initial build error:', err || stats.toString()) + // continue watching + return + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/pre-release-build.yml(1 hunks)build.mjs(11 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/copilot-instructions.md
- .github/workflows/pre-release-build.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T17:46:04.281Z
Learnt from: PeterDaveHello
PR: ChatGPTBox-dev/chatGPTBox#886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.281Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.
Applied to files:
build.mjs
🔇 Additional comments (6)
package.json (2)
22-22: @babel/runtime is correctly added (keep in dependencies).Matches use of @babel/plugin-transform-runtime in build.mjs; leaving it in dependencies is correct since helpers are imported at runtime.
78-80: Tooling alignment looks good.esbuild and esbuild-loader additions match the new minimizer setup in build.mjs.
build.mjs (4)
37-55: Good extraction for thread worker validation.Clear, bounded, and logs caps; aligns with env var semantics.
57-67: Pool timeout control is reasonable.Env override + sane default; comment explains usage.
103-112: Cache key/variantId improvements LGTM.Safe separators and reduced path sensitivity; Windows-friendly.
Also applies to: 139-146
137-146: Dev external source maps are now copied—nice. Verify no 404s in extension dev.Quick sanity check: build dev and confirm maps exist alongside bundles for both Chromium and Firefox outputs.
#!/bin/bash # After: npm run dev (let first compile finish) set -e for dir in build/chromium build/firefox; do echo "Checking $dir" fd -HI --extension map . "$dir" rg -n "sourceMappingURL" "$dir"/*.js doneAlso applies to: 521-530, 539-549
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
build.mjs (7)
17-23: Nit: naming reads clearer as a boolean flagparallelBuild is a boolean; consider isParallelBuild for clarity. This was raised before.
25-34: Cache compression parser: keep return type uniformReturning false for null/invalid values simplifies call sites. Prior feedback already suggested this.
36-41: Simplify CPU detectionos.cpus() always returns an array; current checks are overly defensive. Already suggested previously.
- const cpuInfo = os.cpus && os.cpus() - const len = Array.isArray(cpuInfo) ? cpuInfo.length : 0 - cpuCount = Number.isInteger(len) && len > 0 ? len : 1 + cpuCount = Math.max(1, os.cpus().length || 1)
62-72: Explain the 2000ms defaultAdd a short rationale comment (e.g., balances warm dev rebuild vs worker spin-up). Previously flagged.
207-211: symlinks: false can affect linked workspaces; consider making it opt-in/outPreviously suggested via env toggle; reiterating.
427-466: Zip promise can be simplified; manual settled flag is unnecessaryEarlier feedback covered this; optional clean-up.
475-476: Nit: use path.extname for robustnessEndsWith('.css') can be replaced with path.extname(... ) === '.css'. Previously noted.
🧹 Nitpick comments (2)
build.mjs (2)
95-106: Hoist Sass implementation detection to a reusable helperAvoid repeating dynamic import logic per build; cache once.
- let sassImpl - try { - const mod = await import('sass-embedded') - sassImpl = mod.default || mod - } catch (e1) { - try { - const mod = await import('sass') - sassImpl = mod.default || mod - } catch (e2) { - throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") - } - } + const sassImpl = await getSassImpl()Add once, outside runWebpack:
let cachedSassImpl async function getSassImpl() { if (cachedSassImpl) return cachedSassImpl try { const mod = await import('sass-embedded') cachedSassImpl = mod.default || mod } catch (e1) { try { const mod = await import('sass') cachedSassImpl = mod.default || mod } catch (e2) { throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") } } return cachedSassImpl }
143-151: Cache versioning: include Node major to reduce cross-env collisionsSmall improvement to avoid stale hits across Node majors.
- version: JSON.stringify({ PROD: isProduction }), + version: JSON.stringify({ + PROD: isProduction, + nodeMajor: String(process.versions.node || '').split('.')[0] || 'unknown', + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.mjs(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T17:46:04.281Z
Learnt from: PeterDaveHello
PR: ChatGPTBox-dev/chatGPTBox#886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.281Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.
Applied to files:
build.mjs
🔇 Additional comments (10)
build.mjs (10)
108-117: Good fix: cache key now uses a safe separatorUsing "__" avoids Windows path issues from earlier versions.
162-170: Esbuild/CSS minification swap looks goodConfiguration aligns with the new toolchain; target es2017 is reasonable.
232-247: Thread-loader config is solidWorkers capped; poolTimeout tuned per mode/watch-once.
251-256: Babel cache settings LGTMcacheDirectory on, compression off for speed.
271-281: Sass pipeline is correctstyle-loader in dev, MiniCssExtract in prod; passing implementation explicitly.
380-421: Watch-once exit now awaits post-build tasks and propagates failureNice fix; avoids partial outputs and wrong exit codes.
468-491: Copy semantics are stricter (good)Dev-only CSS is skippable; all other misses fail fast.
508-555: Dev source maps are now copied with assetsThis addresses prior 404/DevTools issues.
557-583: Early setup failures now reject correctlyCatching runWebpack’s early rejection prevents hangs in parallel builds.
604-628: Dev bootstrap keeps watcher alive on initial errorsGood DX trade-off; watch-once still exits non-zero.
|
Codex Review: Didn't find any major issues. Delightful! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
Replace JS/CSS minifiers with esbuild to significantly reduce build times while preserving outputs. Enable thread‑loader by default in dev and prod with dynamically chosen workers. Keep filesystem cache enabled and make compression configurable (favoring uncompressed for faster warm builds on CPU‑bound machines). Scope the Webpack cache version to output‑affecting dimensions for cross‑machine/CI reuse. Add BUILD_PARALLEL to switch between parallel and sequential production builds. Also enable dependency caching in GitHub Actions to speed up CI runs. Improve development behavior and outputs: keep watch mode alive on initial compilation errors with deterministic exits via BUILD_WATCH_ONCE, use style‑loader for faster CSS in dev, emit CSP‑safe external source maps and copy them in dev to avoid 404s, and reduce noisy CSS 404s by skipping only missing assets. Harden related env parsing and consolidate logic to reduce duplication and improve readability. Packaging reliability: wait for the ZIP write stream to close before proceeding to ensure archives are fully flushed on fast CI runners. No breaking changes. Results from a DO VPS (2 cores, ~4 GiB RAM): - Production first run (cold): ~44s (baseline ~105s) - Production second run (warm): ~19s (baseline ~39s) - Development first run: ~31s; second run: ~29s Times vary by environment.
PeterDaveHello
force-pushed
the
BuildOptimization
branch
from
423a43b to
3a3ff77
Compare
|
@coderabbitai review |
|
@PeterDaveHello I'll conduct a comprehensive review of your build pipeline refactor in PR #886. This significant update introduces esbuild-based optimization, thread-loader parallelization, and numerous performance improvements across the build system. The review will cover:
✅ Actions performedReview triggered.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
|
Codex Review: Didn't find any major issues. Keep them coming! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the build system for performance and developer experience improvements, primarily by migrating to esbuild for minification, enabling thread-loader for parallelism, and enhancing caching strategies. The changes are well-structured and robust. I've identified two high-severity issues related to how boolean environment variables are parsed, which could lead to unexpected build behavior. My review includes suggestions to make the flag handling more reliable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
build.mjs (1)
162-170: License banner stripping is intentionally out of scope here.
Past discussion concluded this is not applicable for this project’s distribution model.
🧹 Nitpick comments (9)
build.mjs (8)
34-41: Simplify CPU core detection (clearer and equivalent).
os.cpus()reliably returns an array on Node 20+. The current defensive checks can be reduced without losing safety.-let cpuCount = 1 -try { - const cpuInfo = os.cpus && os.cpus() - const len = Array.isArray(cpuInfo) ? cpuInfo.length : 0 - cpuCount = Number.isInteger(len) && len > 0 ? len : 1 -} catch { - cpuCount = 1 -} +let cpuCount +try { + cpuCount = Math.max(1, os.cpus().length) +} catch { + cpuCount = 1 +}
25-33: Make cache compression parsing return type consistent (drop?? falseat call site).
Returningfalsewhen env is unset simplifies reasoning and removes the need for nullish coalescing where used.function parseCacheCompressionOption(envVal) { - if (envVal == null) return undefined + if (envVal == null) return false const v = String(envVal).trim().toLowerCase() if (v === '' || v === '0' || v === 'false' || v === 'none') return false if (v === 'gzip' || v === 'brotli') return v console.warn(`[build] Unknown BUILD_CACHE_COMPRESSION="${envVal}", defaulting to no compression`) return false } -const cacheCompressionOption = parseCacheCompressionOption(process.env.BUILD_CACHE_COMPRESSION) +const cacheCompressionOption = parseCacheCompressionOption(process.env.BUILD_CACHE_COMPRESSION) … - compression: cacheCompressionOption ?? false, + compression: cacheCompressionOption,Also applies to: 150-151
62-72: Document the default pool timeout constant in-code.
A short comment explaining “why 2000ms” helps future maintainers. If it’s empirical, say so.
95-106: Load Sass implementation once per process, not per build variant.
This dynamic import runs for everyrunWebpackcall. Hoist to a top-level lazy loader to avoid repeated imports and reduce startup jitter.- let sassImpl - try { - const mod = await import('sass-embedded') - sassImpl = mod.default || mod - } catch (e1) { - try { - const mod = await import('sass') - sassImpl = mod.default || mod - } catch (e2) { - throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") - } - } + const sassImpl = await (async () => { + for (const name of ['sass-embedded', 'sass']) { + try { + const mod = await import(name) + return mod.default || mod + } catch (_) {} // try next + } + throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") + })()
207-211: Consider makingresolve.symlinksconfigurable.
Hard-disabling symlinks can hindernpm link/pnpm workflows. An env toggle would ease local package development.- // Disable symlink resolution for consistent behavior/perf; note this can - // affect `npm link`/pnpm workspaces during local development - symlinks: false, + // Disable symlink resolution for consistent behavior/perf. + // Enable via WEBPACK_SYMLINKS=1 when working with linked/workspace deps. + symlinks: process.env.WEBPACK_SYMLINKS === '1' ? true : false,
380-401: Normalize production callback to Promise for symmetry and safety.
Dev/watch path already wraps the callback to catch sync throws. Do the same here to avoid rare hangs if the callback throws before the first await.if (isProduction) { // Ensure compiler is properly closed after production runs compiler.run((err, stats) => { const finishClose = () => compiler.close((closeErr) => { if (closeErr) { console.error('Error closing compiler:', closeErr) process.exitCode = 1 } }) - try { - const ret = callback(err, stats) - if (ret && typeof ret.then === 'function') { - ret.then(finishClose, finishClose) - } else { - finishClose() - } - } catch (_) { - finishClose() - } + Promise.resolve() + .then(() => callback(err, stats)) + .then(finishClose, finishClose) }) } else {
430-465: Stream/archiver promise can be simplified (no manual state).
Rely on the Promise contract andoncehandlers to avoid the customsettledflag.- await new Promise((resolve, reject) => { - const output = fs.createWriteStream(zipPath) - const archive = archiver('zip', { zlib: { level: 9 } }) - let settled = false - const fail = (err) => { - if (!settled) { - settled = true - reject(err) - } - } - const done = () => { - if (!settled) { - settled = true - resolve() - } - } - output.once('error', fail) - archive.once('error', fail) + await new Promise((resolve, reject) => { + const output = fs.createWriteStream(zipPath) + const archive = archiver('zip', { zlib: { level: 9 } }) + output.once('error', reject) + archive.once('error', reject) archive.on('warning', (err) => { // Log non-fatal archive warnings for diagnostics console.warn('[build][zip] warning:', err) }) - // Resolve on close to ensure FD is flushed and closed - output.once('close', done) - // Ensure close is emitted after finish on some fast runners - output.once('finish', () => { - try { - if (typeof output.close === 'function') output.close() - } catch (_) { - // ignore - } - }) + output.once('close', resolve) // resolves after fd flush/close archive.pipe(output) archive.directory(dir, false) archive.finalize() })
493-506: Derive placeholder CSS targets from the copy manifest to prevent drift.
Hardcodingpopup.cssandcontent-script.csscan desync if entries change. Pass expected CSS files to the helper.-async function ensureDevCssPlaceholders(targetDir) { +async function ensureDevCssPlaceholders(targetDir, cssFiles) { if (isProduction) return - const cssFiles = [path.join(targetDir, 'popup.css'), path.join(targetDir, 'content-script.css')] - for (const cssPath of cssFiles) { + for (const cssPath of cssFiles) { if (!(await fs.pathExists(cssPath))) { await fs.outputFile(cssPath, '/* dev placeholder */\n') } const mapPath = `${cssPath}.map` if (!(await fs.pathExists(mapPath))) { await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}') } } } … - await ensureDevCssPlaceholders(chromiumOutputDir) + await ensureDevCssPlaceholders( + chromiumOutputDir, + commonFiles.filter(f => f.dst.endsWith('.css')).map(f => path.join(chromiumOutputDir, f.dst)), + ) … - await ensureDevCssPlaceholders(firefoxOutputDir) + await ensureDevCssPlaceholders( + firefoxOutputDir, + commonFiles.filter(f => f.dst.endsWith('.css')).map(f => path.join(firefoxOutputDir, f.dst)), + )Also applies to: 544-545, 553-554
.github/copilot-instructions.md (1)
20-38: Doc tweaks: add explicit Node.js requirement near the top.
You note Node 20+ under “Build Issues”; repeating it here reduces setup friction.### Bootstrap and Build @@ -- Install dependencies: `npm ci` -- npm audit warnings may appear; for development-only dependencies they generally do not affect the shipped extension. Review and address runtime-impacting advisories separately. +- Prerequisite: Node.js 20+ (LTS). Older versions are unsupported. +- Install dependencies: `npm ci` — npm audit warnings may appear; for development-only dependencies they generally do not affect the shipped extension. Review and address runtime-impacting advisories separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/copilot-instructions.md(1 hunks).github/workflows/pre-release-build.yml(1 hunks)build.mjs(11 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- .github/workflows/pre-release-build.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T17:46:04.281Z
Learnt from: PeterDaveHello
PR: ChatGPTBox-dev/chatGPTBox#886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.281Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.
Applied to files:
build.mjs
🔇 Additional comments (7)
build.mjs (5)
142-142: Nice: dev now emits and copies external .map files.
This fixes the earlier 404s in extension DevTools. Looks good.Also applies to: 526-535
232-247: Thread pool lifetime is clear; good defaults.
Workers capped to cores,0for watch-once to exit quickly,Infinityfor dev. Solid.
408-421: Watch-once now awaits packaging before exit.
Exit code correctly reflects callback failures. This closes a common CI flake. LGTM.
526-535: Approve: dev maps now explicitly copied.
This aligns with the switch to external maps and prevents DevTools 404s.
580-582: Approve: early setup failures now propagate.
CatchingrunWebpackinitial rejections avoids hanging builds when dynamic imports fail. Good fix.Also applies to: 626-628
.github/copilot-instructions.md (2)
34-36: Clarify how to re-enable symlink resolution.
Since the build hard-setsresolve.symlinks: false, add the env knob name or steps to toggle it during local linked-package development.Would you like me to add a
WEBPACK_SYMLINKS=1toggle inbuild.mjsand mirror that here?
35-35: Good: CSP-safe note about external source maps in dev.
Matches the build behavior and avoids inline map CSP pitfalls.
Treat 'false/no/off/0' as disabled for BUILD_PARALLEL and BUILD_THREAD while keeping default enabled when unset. Replace defensive cpu count logic with os.cpus().length guarded by try/catch. This aligns behavior with documented defaults and improves robustness without changing outputs.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the build pipeline to significantly improve performance while preserving output reliability. The main focus is switching from Terser to esbuild for minification, adding configurable parallelization, and implementing advanced caching strategies.
- Replace JS/CSS minifiers with esbuild for faster builds (~58% reduction in build times)
- Add configurable thread-loader parallelism and production variant parallel builds
- Implement advanced Webpack caching with compression control and better cache invalidation
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| build.mjs | Major build system overhaul with esbuild integration, threading controls, and improved build orchestration |
| package.json | Updated dependencies to support new build tools (esbuild, sass-embedded, thread-loader) |
| pre-release-build.yml | Added npm and Webpack filesystem caching to accelerate CI builds |
| copilot-instructions.md | Documented new build performance environment variables and configuration options |
| function parseBooleanEnv(val) { | ||
| if (val == null) return false | ||
| const s = String(val).trim().toLowerCase() | ||
| return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off') | ||
| } | ||
| function isDisabledEnv(val) { | ||
| if (val == null) return false | ||
| const s = String(val).trim().toLowerCase() | ||
| return s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off' | ||
| } | ||
| // Default: parallel build ON unless explicitly disabled | ||
| const parallelBuild = !isDisabledEnv(process.env.BUILD_PARALLEL) |
There was a problem hiding this comment.
These two functions have duplicated logic for parsing string values to boolean. Consider consolidating them into a single function that takes a parameter for the default value or returns a tri-state (true/false/default).
| function parseBooleanEnv(val) { | |
| if (val == null) return false | |
| const s = String(val).trim().toLowerCase() | |
| return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off') | |
| } | |
| function isDisabledEnv(val) { | |
| if (val == null) return false | |
| const s = String(val).trim().toLowerCase() | |
| return s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off' | |
| } | |
| // Default: parallel build ON unless explicitly disabled | |
| const parallelBuild = !isDisabledEnv(process.env.BUILD_PARALLEL) | |
| function parseBooleanEnv(val, defaultValue = false) { | |
| if (val == null) return defaultValue | |
| const s = String(val).trim().toLowerCase() | |
| if (s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off') return false | |
| return true | |
| } | |
| // Default: parallel build ON unless explicitly disabled | |
| const parallelBuild = parseBooleanEnv(process.env.BUILD_PARALLEL, true) |
Copilot uses AI. Check for mistakes.
| function parseThreadWorkerCount(envValue, cpuCount) { | ||
| const maxWorkers = Math.max(1, cpuCount) | ||
| if (envValue !== undefined && envValue !== null) { | ||
| const raw = parseInt(envValue, 10) | ||
| if (Number.isInteger(raw) && raw > 0) { | ||
| if (raw > maxWorkers) { | ||
| console.warn( | ||
| `[build] BUILD_THREAD_WORKERS=${raw} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`, | ||
| ) | ||
| } | ||
| return Math.min(raw, maxWorkers) | ||
| } | ||
| console.warn( | ||
| `[build] Invalid BUILD_THREAD_WORKERS="${envValue}", defaulting to CPU count (${cpuCount})`, | ||
| ) | ||
| } | ||
| return maxWorkers | ||
| } | ||
| const threadWorkers = parseThreadWorkerCount(process.env.BUILD_THREAD_WORKERS, cpuCount) |
There was a problem hiding this comment.
The function name suggests it only parses the worker count, but it also validates against CPU count and applies capping logic. Consider renaming to validateAndCapWorkerCount or splitting into separate parsing and validation functions for better separation of concerns.
| function parseThreadWorkerCount(envValue, cpuCount) { | |
| const maxWorkers = Math.max(1, cpuCount) | |
| if (envValue !== undefined && envValue !== null) { | |
| const raw = parseInt(envValue, 10) | |
| if (Number.isInteger(raw) && raw > 0) { | |
| if (raw > maxWorkers) { | |
| console.warn( | |
| `[build] BUILD_THREAD_WORKERS=${raw} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`, | |
| ) | |
| } | |
| return Math.min(raw, maxWorkers) | |
| } | |
| console.warn( | |
| `[build] Invalid BUILD_THREAD_WORKERS="${envValue}", defaulting to CPU count (${cpuCount})`, | |
| ) | |
| } | |
| return maxWorkers | |
| } | |
| const threadWorkers = parseThreadWorkerCount(process.env.BUILD_THREAD_WORKERS, cpuCount) | |
| function parseWorkerCount(envValue) { | |
| if (envValue !== undefined && envValue !== null) { | |
| const raw = parseInt(envValue, 10) | |
| if (Number.isInteger(raw) && raw > 0) { | |
| return raw | |
| } | |
| } | |
| return undefined | |
| } | |
| function validateAndCapWorkerCount(workerCount, cpuCount) { | |
| const maxWorkers = Math.max(1, cpuCount) | |
| if (workerCount !== undefined) { | |
| if (workerCount > maxWorkers) { | |
| console.warn( | |
| `[build] BUILD_THREAD_WORKERS=${workerCount} exceeds CPU count (${cpuCount}); capping to ${maxWorkers}`, | |
| ) | |
| return maxWorkers | |
| } | |
| return workerCount | |
| } | |
| return maxWorkers | |
| } | |
| const rawWorkerCount = parseWorkerCount(process.env.BUILD_THREAD_WORKERS) | |
| const threadWorkers = validateAndCapWorkerCount(rawWorkerCount, cpuCount) |
Copilot uses AI. Check for mistakes.
| let sassImpl | ||
| try { | ||
| const mod = await import('sass-embedded') | ||
| sassImpl = mod.default || mod | ||
| } catch (e1) { | ||
| try { | ||
| const mod = await import('sass') | ||
| sassImpl = mod.default || mod | ||
| } catch (e2) { | ||
| throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") | ||
| } | ||
| } |
There was a problem hiding this comment.
The Sass implementation is being resolved inside the runWebpack function, which gets called for each build variant. Consider moving this resolution to module level or caching the result to avoid repeated dynamic imports.
Copilot uses AI. Check for mistakes.
| // Disable symlink resolution for consistent behavior/perf; note this can | ||
| // affect `npm link`/pnpm workspaces during local development |
There was a problem hiding this comment.
While the comment mentions that disabling symlinks affects npm link/pnpm workspaces, it doesn't provide guidance on how developers should handle this during local development with linked packages. Consider adding a reference to the documentation or suggesting a workaround.
| // Disable symlink resolution for consistent behavior/perf; note this can | |
| // affect `npm link`/pnpm workspaces during local development | |
| // Disable symlink resolution for consistent behavior/performance. | |
| // ⚠️ This can break local development with linked packages (e.g. `npm link`, pnpm workspaces). | |
| // If you need to work with linked packages locally, set `symlinks: true` temporarily. | |
| // See: https://webpack.js.org/configuration/resolve/#resolve-symlinks |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Code Review
This pull request significantly improves the build pipeline's performance and reliability. The switch to esbuild for minification and the introduction of thread-loader are excellent for speeding up builds. The build script refactoring is very robust, with better error handling, resource management (especially closing webpack compilers), and support for parallel builds. The developer experience is also improved with features like watch-once mode and CSS placeholder generation. My review includes a couple of suggestions to further improve the maintainability and debuggability of the new build script.
.github/copilot-instructions.md
Outdated
| - Default: threads enabled in dev/prod; workers = CPU cores | ||
| - Set `BUILD_THREAD=0` to disable; set `BUILD_THREAD_WORKERS=<n>` to override worker count | ||
| - BUILD_CACHE_COMPRESSION: Webpack filesystem cache compression | ||
| - Default: `0` (no compression) for faster warm builds on CPU‑bound SSD machines |
There was a problem hiding this comment.
The hyphen used in CPU‑bound is a non-standard Unicode character (U+2011), which can cause issues with text processing and search. It's better to use the standard ASCII hyphen-minus (U+002D).
| - Default: `0` (no compression) for faster warm builds on CPU‑bound SSD machines | |
| - Default: `0` (no compression) for faster warm builds on CPU-bound SSD machines |
.github/copilot-instructions.md
Outdated
| - BUILD_WATCH_ONCE (dev): When set, `npm run dev` runs a single build and exits (useful for timing) | ||
| - BUILD_POOL_TIMEOUT: Override thread-loader production pool timeout (ms) | ||
| - Default: `2000`. Increase if workers recycle too aggressively on slow machines/CI | ||
| - Source maps (dev): Dev builds emit external `.map` files next to JS bundles for CSP‑safe debugging; production builds disable source maps |
There was a problem hiding this comment.
The hyphen used in CSP‑safe is a non-standard Unicode character (U+2011), which can cause issues with text processing and search. It's better to use the standard ASCII hyphen-minus (U+002D).
| - Source maps (dev): Dev builds emit external `.map` files next to JS bundles for CSP‑safe debugging; production builds disable source maps | |
| - Source maps (dev): Dev builds emit external `.map` files next to JS bundles for CSP-safe debugging; production builds disable source maps |
| function parseBooleanEnv(val) { | ||
| if (val == null) return false | ||
| const s = String(val).trim().toLowerCase() | ||
| return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off') | ||
| } | ||
| function isDisabledEnv(val) { | ||
| if (val == null) return false | ||
| const s = String(val).trim().toLowerCase() | ||
| return s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off' | ||
| } |
There was a problem hiding this comment.
The two functions for parsing boolean environment variables, parseBooleanEnv and isDisabledEnv, are very similar and handle null/undefined differently, which can be confusing. parseBooleanEnv defaults to false when the variable is not set, while !isDisabledEnv (which is how isDisabledEnv is always used) defaults to true. This can be unified into a single, more intention-revealing function to improve readability and maintainability.
You could replace both with a single function like this:
function getBooleanEnv(val, defaultValue) {
if (val == null) {
return defaultValue;
}
const s = String(val).trim().toLowerCase();
return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off');
}And then update the call sites accordingly:
const parallelBuild = getBooleanEnv(process.env.BUILD_PARALLEL, true)const isWatchOnce = getBooleanEnv(process.env.BUILD_WATCH_ONCE, false)const enableThread = getBooleanEnv(process.env.BUILD_THREAD, true)
| const mod = await import('sass') | ||
| sassImpl = mod.default || mod | ||
| } catch (e2) { | ||
| throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") |
There was a problem hiding this comment.
The error message here is helpful, but it could be even more informative by logging the original errors from the import() calls. This would help diagnose issues if the import fails for reasons other than the package simply not being installed (e.g., permission issues, corrupted files).
console.error('[build] Failed to load sass-embedded:', e1.message)
console.error('[build] Failed to load sass:', e2.message)
throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.")
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
build.mjs (5)
32-39: Returnfalseconsistently for better API uniformity.The function returns
undefinedfor null values butfalsefor invalid ones, creating inconsistency. This requires callers to use?? false.function parseCacheCompressionOption(envVal) { - if (envVal == null) return undefined + if (envVal == null) return false const v = String(envVal).trim().toLowerCase() if (v === '' || v === '0' || v === 'false' || v === 'none') return false if (v === 'gzip' || v === 'brotli') return v console.warn(`[build] Unknown BUILD_CACHE_COMPRESSION="${envVal}", defaulting to no compression`) return false }
101-112: Sass implementation fallback works, but could be cleaner.The nested try-catch blocks handle fallback from sass-embedded to sass. Consider extracting to a helper or using a loop for cleaner code.
- let sassImpl - try { - const mod = await import('sass-embedded') - sassImpl = mod.default || mod - } catch (e1) { - try { - const mod = await import('sass') - sassImpl = mod.default || mod - } catch (e2) { - throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") - } - } + async function loadSassImplementation() { + for (const moduleName of ['sass-embedded', 'sass']) { + try { + const mod = await import(moduleName) + return mod.default || mod + } catch (_) { + // continue to next module + } + } + throw new Error("No Sass implementation available. Install 'sass-embedded' or 'sass'.") + } + const sassImpl = await loadSassImplementation()
214-216: Document symlink resolution impact on development workflows.Disabling symlinks can break
npm linkand pnpm workspaces. The comment mentions this but doesn't provide workarounds.resolve: { extensions: ['.jsx', '.mjs', '.js'], // Disable symlink resolution for consistent behavior/perf; note this can - // affect `npm link`/pnpm workspaces during local development + // affect `npm link`/pnpm workspaces during local development. + // To work with linked packages, temporarily set symlinks: true or + // use BUILD_SYMLINKS=1 environment variable (if implemented). symlinks: false,
594-602: Consider using an options object for better readability.The boolean parameters
(true, true, true, tmpMin, ...)are hard to understand without context. Consider using named options.- createWebpackBuildPromise(true, true, true, tmpMin, '-without-katex-and-tiktoken'), - createWebpackBuildPromise(false, false, false, tmpFull, ''), + createWebpackBuildPromise( + { withoutKatex: true, withoutTiktoken: true, minimal: true }, + tmpMin, + '-without-katex-and-tiktoken' + ), + createWebpackBuildPromise( + { withoutKatex: false, withoutTiktoken: false, minimal: false }, + tmpFull, + '' + ),Note: This would require updating the function signature to accept an options object.
148-148: Use inline source maps in development (build.mjs:148). Chrome extensions only support inline.mapby default; external source-map files won’t load unless you explicitly expose them viaweb_accessible_resourcesinmanifest.json(stackoverflow.com). Change this line:- devtool: isProduction ? false : 'cheap-module-source-map', + devtool: isProduction ? false : 'inline-cheap-module-source-map',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.mjs(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T17:46:04.281Z
Learnt from: PeterDaveHello
PR: ChatGPTBox-dev/chatGPTBox#886
File: build.mjs:167-175
Timestamp: 2025-09-06T17:46:04.281Z
Learning: For the ChatGPTBox browser extension project, license preservation concerns about third-party dependencies may not be applicable according to the maintainer PeterDaveHello, who indicated this is the "wrong scope" for the project.
Applied to files:
build.mjs
🔇 Additional comments (7)
build.mjs (7)
18-27: LGTM! Good extraction of boolean parsing utilities.The
parseBooleanEnvandisDisabledEnvfunctions properly handle common boolean representations including null, empty strings, and various "falsy" keywords. Nice defensive programming.
386-430: Production and watch mode handling looks solid.Good error handling with proper compiler cleanup in production mode and graceful watch-once exit handling. The Promise normalization for callback (line 414) prevents sync throws from bypassing cleanup.
433-471: LGTM! Robust zip creation with proper error handling.The
zipFolderfunction has good error handling with the settled flag preventing double resolution. The warning handler for non-fatal issues is a nice touch.
474-497: Good defensive handling of missing CSS in development.The function properly handles missing CSS files in development by logging and continuing, while failing hard in production for missing artifacts.
500-512: Smart placeholder generation for development CSS.Creating placeholder CSS and source map files prevents 404 noise in dev tools. This is a thoughtful developer experience improvement.
565-589: Well-structured build promise wrapper.The
createWebpackBuildPromisehelper properly handles both webpack errors and post-build failures, with good async error catching for early setup failures.
637-640: Proper top-level error handling ensures build failures are visible.Good practice to catch and log errors at the top level with proper exit code.
| // Use esbuild for JS minification (faster than Terser) | ||
| new EsbuildPlugin({ | ||
| target: 'es2017', | ||
| legalComments: 'none', | ||
| }), | ||
| // Use esbuild-based CSS minify via css-minimizer plugin | ||
| new CssMinimizerPlugin({ | ||
| minify: CssMinimizerPlugin.esbuildMinify, | ||
| }), |
There was a problem hiding this comment.
License stripping may violate third-party obligations.
Setting legalComments: 'none' removes all license banners from dependencies. Many open source licenses require preservation of copyright notices.
Per the previous discussion where this was deemed "wrong scope", I understand license preservation may not apply to this project's distribution model. However, it's worth documenting why license stripping is acceptable here.
🤖 Prompt for AI Agents
In build.mjs around lines 168 to 176, the EsbuildPlugin option legalComments:
'none' strips all third‑party license banners; update the codebase and docs to
justify or change this: either set legalComments to a preserving mode (e.g.,
'linked' or 'inline') if you must preserve notices, or keep 'none' but add a
short documented rationale in the repository (e.g., in README or a new
LICENSES.md) that explains the project's distribution model, the dependency
audit performed, confirmation that no bundled licenses require preservation, and
where to find the list of third‑party licenses; also add a CI check or note to
re-run the dependency/license audit if packaging/distribution changes.
|
Codex Review: Didn't find any major issues. 🎉 About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
Replace U+2011 hyphens with ASCII '-' in 'CPU-bound' and 'CSP-safe' to avoid text-processing issues and keep terminology searchable across tools. No code changes; only documentation cleanup.
User description
Replace JS/CSS minifiers with esbuild to reduce build times while preserving outputs.
Enable
thread-loaderby default in dev and production. Choose workers dynamically (based on CPU cores) and allow overrides via environment variables. Keep filesystem cache enabled and make cache compression configurable, defaulting to uncompressed for faster warm builds on CPU-bound machines. Add BUILD_PARALLEL toggle (default on) to switch between parallel and sequential production variant builds. Ensure watch-once dev builds exit cleanly.Adopt
sass-embeddedfor SASS processing. In development, usestyle-loaderto speed up CSS/SCSS compilation while keeping production outputs unchanged. Maintain CSP-safe source maps for extension contexts and suppress CSS 404 noise in development outputs.Additionally, dependency caching has been added to the GitHub Actions workflow to accelerate CI/CD runs.
Results on this DO VPS (2 cores, ~4 GiB RAM):
Times vary by environment; numbers above are for this machine.
Pull request summary from GitHub Copilot:
PR Type
Enhancement
Description
Replace Terser/CSS minifiers with esbuild for faster builds
Enable thread-loader by default with configurable parallelism
Add filesystem cache compression control and parallel build options
Switch to sass-embedded and style-loader for dev builds
Add GitHub Actions dependency caching for CI acceleration
Diagram Walkthrough
File Walkthrough
build.mjs
Major build system optimization with esbuild and threadingbuild.mjs
pre-release-build.yml
Add dependency and build caching to CI workflow.github/workflows/pre-release-build.yml
package.json
Update dependencies for build optimizationpackage.json
copilot-instructions.md
Document new build performance configuration options.github/copilot-instructions.md
Summary by CodeRabbit
Documentation
Refactor
Chores