-
Notifications
You must be signed in to change notification settings - Fork 853
.pr_agent_accepted_suggestions
| PR 886 (2025-08-30) |
[general] Prevent unnecessary cache busting
✅ Prevent unnecessary cache busting
Avoid putting unstable values (like worker count) into the cache version, or it will constantly invalidate caches across machines/CI runners. Limit the cache version to configuration that truly affects module outputs.
const compiler = webpack({
...
devtool: isProduction ? false : 'cheap-module-source-map',
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'),
...['package.json', 'package-lock.json']
.map((p) => path.resolve(p))
.filter((p) => fs.existsSync(p)),
],
},
},
...
})Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that including the threadWorkers count, which depends on the machine's CPU cores, in the cache version will reduce cache hits across different environments like local development and CI.
[possible issue] Fix env flag parsing
✅ Fix env flag parsing
Parse boolean-like env flags explicitly; the current truthiness check treats any non-empty string (including "0" or "false") as true. This can unintentionally enable watch-once mode. Normalize values like "0", "false", and "" to false.
const isAnalyzing = process.argv[2] === '--analyze'
const parallelBuild = process.env.BUILD_PARALLEL !== '0'
-const isWatchOnce = !!process.env.BUILD_WATCH_ONCE
+const isWatchOnce = (() => {
+ const v = process.env.BUILD_WATCH_ONCE
+ if (v == null) return false
+ const s = String(v).trim().toLowerCase()
+ return !(s === '' || s === '0' || s === 'false' || s === 'no' || s === 'off')
+})()Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that !!process.env.BUILD_WATCH_ONCE is a fragile way to parse a boolean environment variable, leading to incorrect behavior if a user sets it to "0" or "false".
[possible issue] Resolve zip on finish or close
✅ Resolve zip on finish or close
If archiver emits 'close' without 'finish' in some Node streams, the promise may never resolve. Listen to both 'finish' and 'close' on the output stream to guarantee completion across environments, avoiding stuck builds in CI.
const zipPath = `${dir}.zip`
await fs.ensureDir(path.dirname(zipPath))
await new Promise((resolve, reject) => {
const output = fs.createWriteStream(zipPath)
const archive = archiver('zip', { zlib: { level: 9 } })
let settled = false
+ const settleOk = () => {
+ if (!settled) {
+ settled = true
+ resolve()
+ }
+ }
const fail = (err) => {
if (!settled) {
settled = true
reject(err)
}
}
output.once('error', fail)
+ output.once('finish', settleOk)
+ output.once('close', settleOk)
archive.once('error', fail)
archive.on('warning', (err) => {
- // Log non-fatal archive warnings for diagnostics
console.warn('[build][zip] warning:', err)
- })
- output.once('finish', () => {
- if (!settled) {
- settled = true
- resolve()
- }
})
archive.pipe(output)
archive.directory(dir, false)
archive.finalize()
})Suggestion importance[1-10]: 7
__
Why: This is a good suggestion for improving robustness. While the finish event on a write stream is generally sufficient, adding a handler for close provides a fallback to prevent hung builds in edge cases, which is valuable for CI stability.
[general] Wait for stream close for ZIP
✅ Wait for stream close for ZIP
Listen for the close event on the output stream to ensure the file descriptor is flushed and closed before proceeding. This avoids intermittent truncated ZIPs on fast CI runners.
async function zipFolder(dir) {
const zipPath = `${dir}.zip`
await fs.ensureDir(path.dirname(zipPath))
await new Promise((resolve, reject) => {
const output = fs.createWriteStream(zipPath)
const archive = archiver('zip', { zlib: { level: 9 } })
let settled = false
- const fail = (err) => {
+ const settle = (err) => {
if (!settled) {
settled = true
- reject(err)
+ err ? reject(err) : resolve()
}
}
+ const fail = (err) => settle(err)
output.once('error', fail)
archive.once('error', fail)
archive.on('warning', (err) => {
- // Log non-fatal archive warnings for diagnostics
console.warn('[build][zip] warning:', err)
})
- output.once('finish', () => {
- if (!settled) {
- settled = true
- resolve()
- }
- })
+ // finish fires when all data is flushed to the OS, but close ensures fd closed
+ output.once('close', () => settle())
archive.pipe(output)
archive.directory(dir, false)
archive.finalize()
})
}Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a potential race condition where the promise could resolve before the file stream is fully closed, and proposes listening to the close event, which is a critical fix for ensuring ZIP file integrity.
[incremental [*]] Avoid copy failures for missing assets
✅ Avoid copy failures for missing assets
Guard optional assets before copying by filtering commonFiles to entries that exist to avoid hard failures when a chunk or map is not emitted (e.g., tree-shaken CSS or watch-first run), and log a warning instead of throwing for missing dev maps
async function finishOutput(outputDirSuffix, sourceBuildDir = outdir) {
- const commonFiles = [
+ const maybeFiles = [
{ 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.css', dst: 'content-script.css' },
{ 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' },
...(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' },
]),
]
+ const commonFiles = []
+ for (const f of maybeFiles) {
+ if (await fs.pathExists(f.src)) {
+ commonFiles.push(f)
+ } else if (!isProduction) {
+ console.warn('[build] optional asset missing (dev):', f.src)
+ }
+ }
...
await copyFiles(
[...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
chromiumOutputDir,
)
...
await copyFiles(
[...commonFiles, { src: 'src/manifest.v2.json', dst: 'manifest.json' }],
firefoxOutputDir,
)
+}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly points out that the PR's implementation in copyFiles would cause the build to fail if an optional development file like a source map is missing, and proposes a more robust solution by checking for file existence first.
[possible issue] Fail clearly when Sass missing
✅ Fail clearly when Sass missing
Guard against absence of both Sass implementations to prevent runtime crashes. Throw a clear error message if neither module can be resolved so failures are actionable.
-const sassImpl
+let sassImpl
try {
const mod = await import('sass-embedded')
sassImpl = mod.default || mod
-} catch (e) {
- const mod = await import('sass')
- sassImpl = mod.default || mod
+} catch (_) {
+ try {
+ const mod = await import('sass')
+ sassImpl = mod.default || mod
+ } catch (err) {
+ throw new Error(
+ `No Sass implementation available. Install 'sass-embedded' or 'sass'. Original error: ${err && err.message}`,
+ )
+ }
}Suggestion importance[1-10]: 7
__
Why: This suggestion improves the build script's robustness by adding explicit error handling for when neither sass-embedded nor sass is available, providing a clear and actionable error message.
[incremental [*]] Provide CSS sourcemap placeholders
✅ Provide CSS sourcemap placeholders
*Since devtool produces external source maps, also copy or generate placeholder .map files for CSS assets in development to avoid 404s when the browser requests .css.map
devtool: isProduction ? false : 'cheap-module-source-map',
...
await copyFiles(
[...commonFiles, { src: 'src/manifest.json', dst: 'manifest.json' }],
chromiumOutputDir,
)
-// In development, ensure placeholder CSS files exist to avoid 404 noise
+// In development, ensure placeholder CSS and CSS sourcemap files exist to avoid 404 noise
if (!isProduction) {
const chromiumCssPlaceholders = [
path.join(chromiumOutputDir, 'popup.css'),
path.join(chromiumOutputDir, 'content-script.css'),
]
for (const p of chromiumCssPlaceholders) {
if (!(await fs.pathExists(p))) {
await fs.outputFile(p, '/* dev placeholder */\n')
}
+ const mapPath = `${p}.map`
+ if (!(await fs.pathExists(mapPath))) {
+ await fs.outputFile(mapPath, '{"version":3,"sources":[],"mappings":"","names":[]}')
+ }
}
}Suggestion importance[1-10]: 5
__
Why: The suggestion correctly points out that missing CSS sourcemaps will cause 404 errors in development, and proposes a valid fix that complements the existing placeholder logic for CSS files.
| PR 866 (2025-06-07) |
[incremental [*]] Fix inconsistent prompt formatting
✅ Fix inconsistent prompt formatting
The bidirectional translation instruction is appended with a space at the beginning, which creates inconsistent spacing when combined with the previous message. This could lead to formatting issues in the prompt.
src/content-script/selection-tools/index.mjs [33]
-fullMessage += ` If it is already in ${preferredLanguage}, translate it into English and only show me the translated content.`
+fullMessage += `. If it is already in ${preferredLanguage}, translate it into English and only show me the translated content.`Suggestion importance[1-10]: 7
__
Why: Valid formatting concern - using a period to connect sentences is grammatically correct, while the space creates awkward sentence flow. The change from period to space may have been unintentional.
[general] Maintain consistent prompt style
✅ Maintain consistent prompt style
The bidirectional translation prompt is inconsistent with the new professional translator prompt style. Update it to match the tone and format of the main translation prompt for consistency.
src/content-script/selection-tools/index.mjs [32-34]
if (enableBidirectional) {
- fullMessage += ` If it is already in ${preferredLanguage}, translate it into English and only show me the translated content.`
+ fullMessage += ` If the text is already in ${preferredLanguage}, translate it into English instead, preserving meaning, tone, and formatting. Only provide the translated result.`
}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies an inconsistency between the new professional translator prompt style and the bidirectional translation text. The improvement maintains consistent tone and terminology across related prompts, enhancing overall code quality.
[general] Fix inconsistent punctuation
✅ Fix inconsistent punctuation
The bidirectional translation logic has inconsistent formatting. The first translation prompt ends without a period, but the bidirectional addition includes one. This creates an awkward sentence structure when combined.
src/content-script/selection-tools/index.mjs [29-34]
fullMessage = isTranslation
- ? `You are a professional translator. Translate the following text into ${preferredLanguage}, preserving meaning, tone, and formatting. Only provide the translated result`
+ ? `You are a professional translator. Translate the following text into ${preferredLanguage}, preserving meaning, tone, and formatting. Only provide the translated result.`
: message
if (enableBidirectional) {
- fullMessage += `. If it is already in ${preferredLanguage}, translate it into English and only show me the translated content.`
+ fullMessage += ` If it is already in ${preferredLanguage}, translate it into English and only show me the translated content.`
}Suggestion importance[1-10]: 4
__
Why: The suggestion correctly identifies a minor punctuation inconsistency that creates awkward sentence flow when enableBidirectional is true. While technically correct, this is a minor stylistic improvement with limited impact.