-
Notifications
You must be signed in to change notification settings - Fork 858
.pr_agent_accepted_suggestions
| PR 965 (2026-04-25) |
[correctness] Missing session-cookie fallback
Missing session-cookie fallback
When Browser.cookies is unavailable, getChatGptAccessToken still sends an always-present (often empty) Cookie header and does not set credentials: 'include', so the /api/auth/session request may not carry any session cookies and will keep returning no accessToken (throwing UNAUTHORIZED). This makes the new guard avoid the crash but still breaks token retrieval in the target environments.getChatGptAccessToken() now avoids a crash when Browser.cookies is missing, but the fallback still makes a fetch('https://chatgpt.com/api/auth/session') request that may not include any session cookies (it always sets headers.Cookie, often to '', and does not set credentials: 'include'). This can cause the endpoint to return no accessToken, resulting in persistent UNAUTHORIZED errors in the very environments this PR targets.
A more robust pattern already exists in chatgpt-web.mjs: always use credentials: 'include', and only set a manual Cookie header when you actually have a non-empty cookie string.
- src/services/wrappers.mjs[18-30]
- Build
headerssoCookieis only included whencookieis non-empty (e.g....(cookie && { Cookie: cookie })). - Add
credentials: 'include'to thefetchoptions so environments withoutBrowser.cookiesstill have a chance to send existing session cookies automatically. - (Optional but recommended) Add/extend a unit test to cover the
Browser.cookiesundefined case and assert that the request either omitsCookiewhen empty and/or setscredentials: 'include'.
| PR 963 (2026-04-20) |
[reliability] Side panel open can throw
Side panel open can throw
`onClickMenu` now calls `menuConfig.openSidePanel.action(true, tab)` synchronously and returns, but the action unconditionally dereferences `tab.windowId/tab.id` and calls `chrome.sidePanel.open()` without feature detection or error handling, so a missing API (or unexpected tab shape) will throw directly in the click handler.The synchronous openSidePanel path can throw (e.g., SidePanel API not available, or tab missing expected fields), which will surface as an unhandled exception during the context-menu click handler.
onClickMenu now returns early after calling menuConfig.openSidePanel.action(true, tab). The underlying action uses chrome.sidePanel.open({ windowId: tab.windowId, tabId: tab.id }) without feature detection/try-catch.
- src/background/menus.mjs[14-17]
- src/content-script/menu-tools/index.mjs[60-70]
- Keep the
sidePanel.open()call synchronous (noawaitbefore it), but add: -
tabshape guard (tab?.id/tab?.windowId) - API availability check (
Browser.sidePanel?.openorglobalThis.chrome?.sidePanel?.open) -
try/catcharound theopen()call with a clearconsole.warnso failures don’t crash the handler.
| PR 960 (2026-04-11) |
[correctness] Overbroad key validation
Overbroad key validation
`Claude` now accepts any `sessionKey` starting with `sk-ant-sid`, including malformed values missing a numeric version and `-` separator, which pushes failures to later API calls instead of failing fast in the constructor. This key is then used verbatim in the Cookie header for all Claude requests, so malformed keys will cause confusing auth/request errors downstream.Claude session key validation was broadened to accept sid02, but the current check only enforces a prefix (startsWith('sk-ant-sid')). This allows malformed keys (missing version digits and/or - separator) to pass validation and fail later during HTTP requests.
The sessionKey is used verbatim in the Cookie header (cookie: sessionKey=${this.sessionKey}`) across Claude requests, so validating the full expected shape up front improves correctness and debuggability.
- src/services/clients/claude/index.mjs[74-84]
- src/services/clients/claude/index.mjs[218-224]
Replace the prefix check with a forward-compatible format check, e.g.:
const key = String(sessionKey).trim()-
if (!/^sk-ant-sid\d+-/.test(key)) throw new Error('... sk-ant-sid<version>-*****')This keeps compatibility withsid01,sid02, etc., while rejecting clearly malformed inputs early.
[maintainability] Sid01-only examples remain
Sid01-only examples remain
The file still includes sid01-specific example text (JSDoc and a `request()` help message), which is now inconsistent with the updated version-agnostic validation. This can mislead users into thinking only `sid01` is supported even though the constructor now accepts `sid02`+.After updating validation to accept sk-ant-sid*, the file still contains sid01-only examples/help text that contradict the new behavior.
Users often copy/paste these strings when configuring the client; keeping them aligned reduces confusion.
- src/services/clients/claude/index.mjs[40-53]
- src/services/clients/claude/index.mjs[163-169]
Replace sk-ant-sid01-***** example values with a version-agnostic form (e.g., sk-ant-sid02-***** or sk-ant-sid<version>-*****), consistent with the constructor validation and error message.
| PR 928 (2026-02-22) |
[general] Update Claude log label
✅ Update Claude log label
In src/background/index.mjs, update the debug log message from "Using Claude API Model" to "Using Anthropic API Model" for consistency.
src/background/index.mjs [588]
-console.debug('[background] Using Claude API Model')
+console.debug('[background] Using Anthropic API Model')Suggestion importance[1-10]: 4
__
Why: The suggestion improves consistency by aligning a debug log message with the Claude to Anthropic renaming, which is a good practice for maintainability, though it has a low impact on functionality.
| 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.