Recode Cookie Quick Manager 0.6.0 - #3
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCookie Quick Manager 0.6.0 adds shared cookie identity and protection logic, dual Chromium/Firefox builds, asynchronous runtime workflows, partition-aware interfaces, expanded QA coverage, and automated release checks. ChangesCookie Quick Manager modernization
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 3-5: Add workflow-level concurrency configuration to the CI
workflow alongside the existing push and pull_request triggers, using a suitable
group key and setting cancel-in-progress to true so superseded runs for the same
change are cancelled.
- Around line 12-13: Update the workflow steps using actions/checkout and
actions/setup-node to reference reviewed immutable commit SHAs instead of `@v4`
tags, and set persist-credentials to false on the checkout step. Preserve the
existing action configuration and add comments identifying the pinned versions
if consistent with the workflow style.
- Around line 1-9: Add a top-level permissions configuration to the CI workflow
granting only contents: read, positioned alongside name and on so all jobs,
including check, inherit the restricted token scope.
In `@package.json`:
- Around line 12-13: Update the package and package:firefox scripts to create
deterministic release ZIPs by normalizing archived file timestamps and ordering,
or by using a reproducible archiver, while preserving each existing build and
output path. Add CI coverage that runs and verifies reproducible packaging for
both Chromium and Firefox targets.
In `@qa/chromium-qa.mjs`:
- Around line 945-953: Update the domain-boundary-grouping assertion in the QA
test to require example.com while asserting that notexample.com is absent from
boundaryDomains. Keep the existing diagnostic message and result reporting
aligned with this corrected expectation.
- Around line 1127-1131: Restructure the cleanup sequence around context.close()
so its failure cannot skip the later cleanup steps: use nested try/finally
blocks to always execute fs.rmSync(userDataDir, ...) and then
stopFixtureServer(fixtureProcess), while preserving the existing conditional
checks.
- Around line 66-73: Update fixtureIsReady() to create an AbortController and
schedule a short timeout that aborts the health-check fetch, passing its signal
to fetch(fixtureHealthUrl). Ensure the timeout is cleared after each attempt,
including failures, so ensureFixtureServer() retries promptly without leaving
timers active.
In `@qa/firefox-baseline-rdp.py`:
- Around line 315-319: Replace the fixed 2-second wait before capturing
after_delete with deadline-based polling, matching the protection-activation
pattern around the existing polling logic. Repeatedly recapture the relevant
cookie state until fixture_js_host= appears or the deadline expires, then retain
the strict assertion in the surrounding validation.
In `@scripts/build.mjs`:
- Around line 9-17: Validate outputArgument and the resolved outputDirectory
before the rm call in the build script. Reject absolute paths, traversal, the
repository root, src, and any location outside repositoryRoot; allow only
approved build directories or safe non-root paths within the repository.
Preserve the existing target validation and directory creation flow after
validation.
In `@src/cookies.html`:
- Around line 406-411: Localize the user-facing partitionKey label in the
partition-key-row markup by adding the appropriate locale key and binding the
label through the existing data-i18n-content flow, matching the pattern used by
neighboring fields.
In `@src/cookies.js`:
- Around line 220-234: Preserve the current cookie selection in the protection
toggle handler before re-triggering the domain click. In the success callback
around set_protect_lock_icon and the active `#domain-list` click, assign
last_selected_cookie_index from the active `#cookie-list` item’s index so
showCookiesList restores the protected cookie instead of defaulting to the first
entry.
In `@src/export.js`:
- Around line 486-492: Update the expired-record handling in the import catch
block and the corresponding core.parseJsonCookieRecord error path to use a
stable dedicated error code or type, such as EXPIRED, instead of matching
error.message text. Have core.parseJsonCookieRecord mark expiration failures
consistently, and make the catch branch increment expiredCount and continue
based on that marker while rethrowing all other errors.
- Around line 516-523: The restore result handling around
cookieRestoredSingleError should pass only failedCount to the localized error
message, rather than hardcoded English text, and include the added and expired
counts so partial imports are represented. Update the failedCount branch while
preserving the existing success path and expiredCount handling.
In `@src/menu.html`:
- Around line 38-41: Update the decorative img elements in the menu links,
including those under simple_cookie_manager, delete_current_cookies, and
delete_context_cookies, to include empty alt attributes. Leave the visible
localized labels and existing image sources unchanged.
In `@src/options.html`:
- Around line 192-193: Update the “Report an issue” anchor identified by
data-i18n-content="oIssues" to link directly to the repository’s /issues path,
while leaving the “Source code” link identified by data-i18n-content="oLicense"
unchanged.
- Around line 127-139: Update the First-Party Isolation block around the
fpi_status, fpi_info, and Mozilla link markup to use locale message keys for the
label, explanatory paragraphs, emphasized text, and link text instead of
hard-coded English. Split mixed-content elements as needed so each translatable
text node uses the existing data-i18n-content mechanism, and add the
corresponding messages to the project’s locale resources while preserving the
current structure, link URL, and Firefox-only notice.
In `@src/options.js`:
- Around line 248-323: Wrap the domain-level selectDomain checkbox and its
domain text in a label within the protected-cookie tree construction, so
clicking the domain name toggles the checkbox while preserving the existing
legend structure and selection behavior.
- Around line 182-191: Update get_options to use core.getDefaultSettings() as
the defaults source instead of duplicating the inline Object.assign settings
object, while preserving the merge with core.sanitizeSettings(items) and the
existing options behavior.
In `@test/background.test.js`:
- Line 100: Replace the fixed 190 ms sleeps in the affected background tests
with deterministic timer control. Update the restore-timer setup used by the
tested background behavior to inject a controllable timer or expose a test
harness method that flushes pending restore callbacks, then invoke it before
assertions while preserving the existing timing and assertion behavior.
In `@test/core.test.js`:
- Around line 164-180: Add an explicitly domain-scoped cookie variant alongside
the existing rootCookie in the “exact protection distinguishes path, store, host
scope, and partition” test, keeping its name, path, and store identical while
setting hostOnly/domain scope appropriately, then assert isCookieProtected
returns false for it against the host-only protection record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2dd22056-451f-48ff-b1ba-d43fe32c7717
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/static/js/bootstrap-treeview-1.2.0.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (35)
.github/workflows/ci.yml.gitignoreREADME.mdpackage.jsonqa/PARITY_REPORT.mdqa/chromium-qa.mjsqa/firefox-baseline-rdp.pyqa/fixture-server.mjsscripts/build.mjssrc/_locales/de/messages.jsonsrc/_locales/en/messages.jsonsrc/_locales/fr/messages.jsonsrc/api.jssrc/background-script.jssrc/browser-shim.jssrc/cookies.htmlsrc/cookies.jssrc/core.jssrc/export.jssrc/manifest.firefox.jsonsrc/manifest.jsonsrc/menu.htmlsrc/menu.jssrc/options.htmlsrc/options.jssrc/platform.jssrc/service-worker.jssrc/static/css/bootstrap-theme.min.csssrc/static/css/jquery-ui.min.csssrc/static/js/bootstrap-treeview.min.csstest/api.test.jstest/background.test.jstest/core.test.jstest/platform.test.jstest/repository.test.js
💤 Files with no reviewable changes (3)
- src/static/js/bootstrap-treeview.min.css
- src/static/css/bootstrap-theme.min.css
- src/static/css/jquery-ui.min.css
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🧰 Additional context used
🪛 ast-grep (0.44.1)
test/platform.test.js
[warning] 17-17: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(__dirname, '../src/platform.js'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
test/api.test.js
[warning] 52-52: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(__dirname, '../src/api.js'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
test/repository.test.js
[warning] 8-8: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(root, relativePath), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 41-41: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(root, 'src', file), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
test/background.test.js
[warning] 76-76: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(__dirname, '../src/background-script.js'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 89-89: Avoid using the initial state variable in setState
Context: setImmediate(resolve)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 99-99: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 190)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 119-119: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 190)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 131-131: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 190)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 138-138: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 190)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 149-149: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 190)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
src/background-script.js
[error] 86-89: React's useState should not be directly called
Context: setTimeout(() => {
restoreTimers.delete(key);
restoreCookieIfStillMissing(cookie).catch(vAPI.onError);
}, 150)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🪛 HTMLHint (1.9.2)
src/menu.html
[warning] 38-38: An alt attribute must be present on elements.
(alt-require)
[warning] 40-40: An alt attribute must be present on elements.
(alt-require)
[warning] 41-41: An alt attribute must be present on elements.
(alt-require)
🪛 OpenGrep (1.23.0)
src/core.js
[ERROR] 377-377: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🪛 Ruff (0.15.20)
qa/firefox-baseline-rdp.py
[warning] 148-148: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 172-172: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 268-268: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 287-287: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 295-295: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 316-319: Avoid specifying long messages outside the exception class
(TRY003)
🪛 zizmor (1.26.1)
.github/workflows/ci.yml
[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 1-20: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 12-12: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 8-8: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 3-5: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (60)
test/api.test.js (1)
1-109: LGTM!test/background.test.js (1)
1-99: LGTM!Also applies to: 101-119, 121-131, 133-138, 140-149, 151-163
test/core.test.js (1)
1-163: LGTM!Also applies to: 181-289
test/platform.test.js (1)
1-32: LGTM!test/repository.test.js (1)
1-46: LGTM!src/background-script.js (3)
84-92: The ast-grepusestate-direct-usagehint here is a false positive — this is asetTimeoutscheduling a restore, not a React hook. No change needed; the per-identity timer bookkeeping andcancelScheduledRestorecleanup are correct.
23-61: LGTM!Also applies to: 94-126, 157-181
127-155: LGTM!src/api.js (3)
30-33: LGTM!Also applies to: 48-64, 82-117
119-149: LGTM!Also applies to: 163-229, 231-250, 252-274, 276-350, 352-419, 453-497, 544-547
151-161: 🩺 Stability & AvailabilityNo issue here.
partitionKey: {}is the wildcard form and returns partitioned cookies as well, so the extragetAll()call is fine.> Likely an incorrect or invalid review comment.src/platform.js (2)
65-68: LGTM!Also applies to: 88-88
18-19: 🩺 Stability & AvailabilityNo change needed for
supportsPartitionedCookiesFirefox 140.0 already includescookies.getAll(..., partitionKey), so the hardcodedtrueis fine..gitignore (1)
3-7: LGTM!README.md (1)
1-11: LGTM!Also applies to: 13-29, 30-35, 36-59, 60-89, 90-107, 108-123, 124-131, 132-137, 138-140
src/cookies.html (2)
6-7: LGTM!Also applies to: 640-640
68-74: 🎯 Functional CorrectnessNo change needed for
.partition-badge.partition-badgealready inherits Bootstrap’s.badgestyling, which includeswhite-space: nowrap, so the ellipsis behavior is already covered.> Likely an incorrect or invalid review comment.src/service-worker.js (1)
2-3: LGTM!src/options.html (1)
6-7: LGTM!Also applies to: 115-116, 125-126, 203-213, 395-396
qa/PARITY_REPORT.md (1)
1-15: LGTM!Also applies to: 16-37, 38-81, 82-95, 96-101, 102-108, 109-111
qa/chromium-qa.mjs (2)
5-5: LGTM!Also applies to: 14-14, 50-64, 75-106, 108-164, 195-212, 224-231, 280-286, 327-327, 360-371, 383-397, 399-944, 954-996, 1027-1028, 1049-1113
216-230: 🩺 Stability & AvailabilityNo change needed The service worker bootstrap doesn’t emit a console error before these listeners are attached, so moving
getExtensionId()below this block isn’t necessary.> Likely an incorrect or invalid review comment.qa/fixture-server.mjs (1)
286-295: LGTM!scripts/build.mjs (1)
1-8: LGTM!Also applies to: 19-35
src/_locales/de/messages.json (1)
383-392: LGTM!Also applies to: 451-452, 626-650
src/_locales/en/messages.json (1)
409-417: LGTM!Also applies to: 477-478, 654-654
src/_locales/fr/messages.json (1)
408-417: LGTM!Also applies to: 476-477, 653-653
src/manifest.firefox.json (1)
1-48: LGTM!src/manifest.json (2)
4-9: LGTM!Also applies to: 11-14, 33-35
10-10: 🎯 Functional CorrectnessNo issue:
delete_current_localstorageusesbrowser.scripting.executeScriptto clearwindow.localStorage, so removingbrowsingDatadoes not break this action.> Likely an incorrect or invalid review comment.src/menu.html (1)
6-7: LGTM!Also applies to: 42-48
src/browser-shim.js (1)
1-5: LGTM!src/core.js (2)
1-146: LGTM!Also applies to: 148-159, 162-517
147-147: 🎯 Functional CorrectnessNo issue: deletion uses
browser.cookies.set()buildCookieDeletionDetails()is only used for an expired set operation, so the explicitpathis honored and non-root cookie paths are preserved.> Likely an incorrect or invalid review comment.src/export.js (9)
137-137: LGTM!Also applies to: 195-195
222-224: LGTM!
272-273: LGTM!Also applies to: 287-288
411-413: 🎯 Functional Correctness | ⚡ Quick winNetscape import: expired rows aren't counted, and the 10k cap counts raw lines, not cookies.
parseNETSCAPEFilesilently drops expired rows (Line 436-437) without incrementing any counter, unlikeparseJSONFilewhich tracksexpiredCount. Inadd_cookies,cookieDetails.expiredCountis therefore always 0 for Netscape imports, so users never see the "expired skipped" message for.txtimports. Separately,lines.length > 10000(Line 412-413) counts every physical line (including blank/comment lines), not actual cookie rows, so the thrown message "limited to 10,000 cookies" is inaccurate for files padded with comments/blank lines.🐛 Track expired rows like the JSON path
+ let expiredCount = 0; for (let index = 0; index < lines.length; index++) { ... - if (expirationDate !== 0 && expirationDate <= ((Date.now() / 1000 | 0) + 1)) - continue; + if (expirationDate !== 0 && expirationDate <= ((Date.now() / 1000 | 0) + 1)) { + expiredCount++; + continue; + } ... } + details.expiredCount = expiredCount; return details; }Also applies to: 436-437
414-417: 🗄️ Data Integrity & Integration
legacyCqmFlagsheuristic can't detect legacy exports made only of host-only cookies.The file-wide legacy detection only fires when a domain starts with
.and its flag column is falsy (Line 414-417). Old (pre-fix) exports containing exclusively host-only cookies never produce that combination, so such legacy files are silently parsed with the new inverted formula against old non-inverted data — flippinghostOnlyfor every row in the file. Worth confirming this gap is acceptable, or consider stamping new exports with an explicit format-version marker so detection doesn't rely on this per-file heuristic at all.Also applies to: 441-442
505-514: 🩺 Stability & Availability | ⚡ Quick winA protection failure after a successful import is reported as a full import error.
vAPI.set_cookie_protection(Line 514) is awaited with no local try/catch. If the runtime message it sends rejects, the exception propagates out ofadd_cookiesintohandleUploadedFile's catch, which showscookieRestoredError— even though the cookies were already successfully inserted. Users see a false failure and$('#actualize_button').click()never runs, hiding the actually-imported cookies.🛡️ Isolate protection failures from the import result
- if (items.import_protected_cookies && addedCookies.length) - await vAPI.set_cookie_protection(addedCookies, true); + if (items.import_protected_cookies && addedCookies.length) { + try { + await vAPI.set_cookie_protection(addedCookies, true); + } catch (error) { + console.error('Failed to protect imported cookies:', error); + } + }
529-538: LGTM!
32-32: 🩺 Stability & AvailabilityNo issue:
core.jsloads beforeexport.js
src/cookies.htmlincludescore.jsbeforeexport.js, sowindow.CQMCoreis available when this module initializes.> Likely an incorrect or invalid review comment.
300-325: 🗄️ Data Integrity & IntegrationNo change needed for
Partition keyimport.core.parseJsonCookieRecordalready maps'Partition key'back topartitionKey, and the set path preserves it.src/cookies.js (10)
32-40: LGTM!Also applies to: 169-172, 303-304
112-124: 🗄️ Data Integrity & Integration | ⚡ Quick winStale
partitionKeycan leak across domains on save.
partitionKeyis copied from whatever cookie is currentlyli.activein the list, not validated against the domain/name actually being saved. If a user selects a partitioned cookie, then edits#domain(or#name) to author a different cookie before clicking save, the new cookie will incorrectly inherit the old cookie'spartitionKey.topLevelSite, mis-scoping it.🛡️ Suggested guard
const selectedCookie = $('`#cookie-list`').find('li.active').data('cookie'); - if (selectedCookie?.partitionKey) + if (selectedCookie?.partitionKey && selectedCookie.domain === params.domain) params.partitionKey = selectedCookie.partitionKey;Please confirm whether the "new cookie" UI flow clears
li.active/its cookie data before allowing edits, since that would determine how reachable this edge case is.
514-514: LGTM!Also applies to: 610-610, 691-692
809-811: LGTM!filter_master_domainscorrectly fixes the substring-boundary bug (e.g.notexample.comno longer falsely groups underexample.com) via theendsWith('.'+candidate)check, and the option rendering now safely usestextContent/DOM append instead of raw HTML.Also applies to: 827-852, 887-890
1012-1012: LGTM!Also applies to: 1033-1033, 1045-1053
1165-1175: LGTM!Also applies to: 1190-1190, 1259-1261
1360-1372: LGTM! Partition badges and protection checks correctly usetextContent/core.isCookieProtected.Also applies to: 1427-1441, 1488-1488
1523-1534: 📐 Maintainability & Code Quality | ⚡ Quick winStale docstring contradicts new
vAPI.remove_cookiesemantics.The function's docstring (Line 1519: "NOTE: Remove inexistant cookie: Removed: null") describes the old
browser.cookies.remove()behavior. Per the newvAPI.remove_cookiecontract (src/api.js),nullis returned when the cookie still exists (removal failed), and the original cookie object is returned when it's gone (removal succeeded or it never existed) — the inverse of what the comment says. The code at Line 1534 correctly follows the new semantics, but the stale comment will mislead future maintainers.📝 Suggested comment update
function delete_current_cookie() { /* Remove a cookie displayed on details zone - * NOTE: Remove inexistant cookie: Removed: null + * NOTE: vAPI.remove_cookie resolves to null if the cookie still exists + * (removal failed), and to the original cookie otherwise (removed or + * never existed). * NOTE: This function does not try to delete protected cookie */
1609-1610: LGTM! Skin allowlisting prevents arbitrary stylesheet hrefs, the overwrite-event skip plus selection-preserving domain click is correct, and null-prototype globals avoid prototype-pollution-style key collisions.Also applies to: 1681-1687, 1703-1710
503-505: 🚀 Performance & ScalabilityNo duplicate reload here.
changes['protected_cookies']handles the aggregated object, whilecore.isProtectionStorageKey(key)matches onlyprotected_cookie:*entries, so the two branches cover different update shapes.> Likely an incorrect or invalid review comment.src/menu.js (2)
44-45: LGTM! Guard clauses prevent crashes when no active tab/store id is available, and theString()coercion matchesi18n.getMessagesubstitution expectations.Also applies to: 66-67, 80-81, 94-94
112-121: LGTM! Thebrowser.scripting.executeScript-based LocalStorage clear is more precise than the previousbrowsingData.removeapproach, and thevAPI.get_cookies/vAPI.delete_cookiesadoption matches the API contract.Also applies to: 148-148, 314-315, 344-344
src/options.js (2)
32-32: LGTM!Also applies to: 67-76, 104-113, 156-168
124-144: 🗄️ Data Integrity & IntegrationLegacy string-only protection entries are matched by name only.
updateProtectionMap()removes string records withrecord === cookie.name, so the syntheticpath,storeId, andhostOnlyvalues here do not affect unprotecting legacy entries.> Likely an incorrect or invalid review comment.qa/firefox-baseline-rdp.py (3)
147-148: LGTM!
257-295: LGTM! The deadline-based polling for protection activation is a solid improvement over a fixed sleep.
161-172: 🎯 Functional CorrectnessTitle is fixed, so this check is locale-safe.
src/cookies.htmlhardcodes<title>Cookie Quick Manager</title>, and there’s nodocument.titleoverride in the page scripts.> Likely an incorrect or invalid review comment.
|
Addressed the actionable findings from the expanded CodeRabbit review in
The docstring-coverage item is a generic CodeRabbit advisory, not a configured repository quality gate or a defect in this procedural JavaScript extension. I did not add hundreds of mechanical JSDoc blocks solely to satisfy that heuristic; the changed behavior is documented at the relevant functions and covered by tests. Validation after the fixes:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/export.js`:
- Around line 443-445: Update the expiration check in the export processing loop
to calculate current epoch seconds without the signed 32-bit coercion from `|
0`, while preserving the existing one-second grace period and expiredCount
increment/continue behavior. Ensure buildCookieSetDetails receives no records
that are already expired due to timestamp overflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3eacb7c6-805d-42a5-b011-4ebac67c90df
📒 Files selected for processing (20)
.github/workflows/ci.ymlpackage.jsonqa/chromium-qa.mjsqa/firefox-baseline-rdp.pyscripts/build.mjsscripts/package.mjsscripts/verify-packages.mjssrc/_locales/de/messages.jsonsrc/_locales/en/messages.jsonsrc/_locales/fr/messages.jsonsrc/cookies.htmlsrc/cookies.jssrc/core.jssrc/export.jssrc/menu.htmlsrc/options.htmlsrc/options.jstest/background.test.jstest/build.test.jstest/core.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🧰 Additional context used
🪛 ast-grep (0.44.1)
test/build.test.js
[warning] 4-4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require('node:child_process')
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
test/background.test.js
[warning] 94-94: Avoid using the initial state variable in setState
Context: setImmediate(resolve)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 85-85: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(__dirname, '../src/background-script.js'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🪛 zizmor (1.26.1)
.github/workflows/ci.yml
[info] 15-15: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🔇 Additional comments (20)
.github/workflows/ci.yml (1)
7-31: LGTM!package.json (1)
12-14: LGTM!src/cookies.html (1)
407-407: LGTM!qa/chromium-qa.mjs (1)
67-75: LGTM!Also applies to: 783-792, 803-808, 819-819, 964-965, 1069-1069, 1140-1150
src/core.js (1)
188-194: LGTM!Also applies to: 401-415
test/core.test.js (1)
164-172: LGTM!Also applies to: 178-178, 190-190, 267-267
scripts/build.mjs (1)
16-22: LGTM!scripts/package.mjs (1)
1-56: LGTM!scripts/verify-packages.mjs (1)
1-28: LGTM!src/_locales/de/messages.json (1)
403-407: LGTM!Also applies to: 608-642
src/_locales/en/messages.json (1)
429-433: LGTM!Also applies to: 622-656
src/_locales/fr/messages.json (1)
428-433: LGTM!Also applies to: 621-655
test/background.test.js (1)
39-46: LGTM!Also applies to: 82-96, 115-115, 135-135, 147-154, 165-165
src/cookies.js (1)
123-124: LGTM!Also applies to: 231-231, 1521-1522
src/export.js (1)
223-225: LGTM!Also applies to: 353-353, 414-421, 437-442, 446-467, 497-497, 523-545
src/menu.html (1)
38-41: LGTM!src/options.html (1)
129-135: LGTM!Also applies to: 189-189
src/options.js (1)
173-175: LGTM!Also applies to: 255-260
qa/firefox-baseline-rdp.py (1)
304-311: LGTM!test/build.test.js (1)
1-19: LGTM!
1 participant
Summary
Verification
npm run checkweb-ext lintpassed with 0 errors and 0 warningsnpm audit: 0 vulnerabilitiesgit diff --checkpassedReview
The full diff was reviewed manually. No CodeRabbit or automated review bot was used.