Release/1.2.0#8
Open
dary1337 wants to merge 44 commits into
Open
Conversation
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughMonolithic MV3 rewrite: adds TypeScript tooling/CI, shared schemas/types/constants, typed storage with migration, repo loader, CSS compiler, updater/exporter/release fetcher, background alarms and injection, Pinia stores, a routed Vue 3 popup app with components/pages/i18n/styles, tests, and removes legacy popup/worker assets. ChangesMV3 Vue architecture, services, background, UI, tests, and tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 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 35-44: The workflow uses mutable tags for third-party actions
(actions/checkout@v4 and actions/setup-node@v4); update the uses entries in the
CI workflow so each action is pinned to its immutable commit SHA instead of the
v4 tag (replace the uses values for actions/checkout@v4 and
actions/setup-node@v4 with the corresponding commit SHAs while keeping the same
inputs like persist-credentials and node-version/cache).
In `@eslint.config.js`:
- Around line 171-183: The tests override block for files: ['tests/**/*.ts'] in
eslint.config.js currently disables broad safety rules
('`@typescript-eslint/no-non-null-assertion`' and all
'`@typescript-eslint/no-unsafe-`*'), which undermines the project's strict TS
policy; narrow this by re-enabling those rules and only disabling them inline
where necessary (use targeted // eslint-disable-next-line comments in specific
test files), or keep the overrides but add a short top-of-file justification
comment in eslint.config.js explaining why each rule is disabled and which
concrete testing patterns require it; reference the rules named in the block to
locate and adjust the configuration.
In `@src/background/index.ts`:
- Around line 41-52: Replace the .catch().finally() promise chain with an async
IIFE using try/catch/finally: call await handle() inside a try block, log errors
in the catch (using the same message and error variables), and call
sendResponse() in finally, then keep the synchronous return true to keep the
channel open; locate the current promise chain around handle(), sendResponse(),
and the surrounding listener and convert that block to the async/await pattern
described.
In `@src/popup/app.vue`:
- Around line 9-19: The anchor currently nests a <button> (interactive inside
interactive) which is invalid; remove the button element and apply the button
styling to the anchor instead: keep the <a v-if="releaseUrl"
class="update-banner" :href="releaseUrl" target="_blank" rel="noopener
noreferrer"> and move the contents to just <Icon name="update" />{{
t('buttons.updateExtension') }}, then update CSS rules that target
.update-banner button to target .update-banner so the anchor is visually styled
like the button; alternatively, if you prefer a <button>, render a <button
class="update-banner" `@click`="openRelease" role="link"> that opens releaseUrl
(use a method openRelease that calls window.open(releaseUrl, '_blank') ) but do
not nest interactive elements.
In `@src/popup/components/ace-editor/ace-editor.scss`:
- Around line 9-13: Stylelint flags are caused by the Vue scoped selector using
:deep and the quoted single-word font name "Consolas"; fix by updating stylelint
config to allow the Vue deep combinator (add :deep to the
selector-pseudo-class-no-unknown ignore list and/or enable the Vue/scoped-style
parser plugin) so the selectors like :deep(.ace_editor) and
:deep(.ace_print-margin) remain unchanged, and remove the quotes around Consolas
in the font-family declaration (leave 'Fira Code' and 'Source Code Pro' quoted
but change "Consolas" to Consolas) to satisfy font-family-name-quotes.
In `@src/popup/components/app-footer/app-footer.ts`:
- Around line 16-23: In updateThemes, prevent overlapping executions by
short-circuiting when updating.value is already true (return a failure indicator
immediately), then set updating.value = true and call await themes.updateAll()
inside a try/catch/finally where finally resets updating.value = false; in the
catch return or propagate a meaningful failure value (e.g., false or an Error)
instead of swallowing the exception so callers can react; refer to the
updateThemes function, the updating.value flag, and themes.updateAll() when
making the change.
In `@src/popup/components/app-footer/app-footer.vue`:
- Line 35: The tooltip string "Open in window" in the template of the AppFooter
component is hard-coded; replace the literal title attribute with the localized
call (use t(...)) consistent with other tooltips in this Vue file so the UI uses
the project's typed/localized strings (update the title on the element in
src/popup/components/app-footer/app-footer.vue to call t('...') or the
appropriate i18n key used across the component).
In `@src/popup/components/confirm-dialog/confirm-dialog.ts`:
- Around line 23-24: Add a one-line comment justifying the cast on the
assignment to lastFocused where document.activeElement is cast to HTMLElement
(the cast enables calling lastFocused.focus() later); update the code in
confirm-dialog.ts at the assignment of lastFocused to include that comment
immediately above or inline with the cast explaining that document.activeElement
is narrowed to HTMLElement because the dialog only tracks focusable HTMLElements
and focus() will be called.
In `@src/popup/components/screenshots/screenshots.vue`:
- Around line 11-25: Thumbnails and the lightbox currently only respond to mouse
clicks which blocks keyboard users; make each thumbnail image focusable and
keyboard-activatable by adding tabindex="0" and a keydown handler that calls the
existing open(src) on Enter or Space (keep the `@click` calling open as well), and
make the lightbox container focusable and listen for Escape (and Enter/Space if
you want click-to-close parity) to call the existing close() method (keep `@click`
close). Use the existing symbols: the v-for="src in screenshots" <img> elements
(call open(src) on key events) and the lightbox div with v-if="lightbox" (call
close() on Escape/Enter/Space keydown); ensure ARIA role="button" or appropriate
role is added for screen readers.
In `@src/popup/components/tab-bar/tab-bar.vue`:
- Around line 24-30: Replace the clickable divs used for each tab with semantic
interactive elements: change the element rendered in the v-for for tabs
(currently the div with class "tab") to a <button> so keyboard activation works
natively, keep the :key="tab.id", :class binding that uses currentTab, and
continue to call selectTab(tab.id) on click; also add appropriate ARIA
attributes (role="tablist" on the container, role="tab" on each button,
aria-selected when tab.id === currentTab, aria-controls referencing the tab
panel id, and tabindex handling for focus management) and ensure CSS resets for
the button (visual styles preserved) and any explicit keyboard handlers (e.g.,
`@keydown` for ArrowLeft/ArrowRight navigation) are implemented alongside the
existing selectTab method so tabs are reachable and operable by keyboard and
assistive tech.
In `@src/popup/composables/use-import.ts`:
- Line 1: Run Prettier to reformat the file so it matches project style: run npx
prettier --write on src/popup/composables/use-import.ts (or apply the same
formatting changes) so the import statement and the rest of the file conform to
Prettier rules; ensure the import { parseImport } from '`@/services/exporter`';
line (and surrounding code in use-import.ts) is reformatted and committed to
resolve the CI Prettier --check failure.
In `@src/popup/pages/theme-details-page/theme-details-page.ts`:
- Around line 112-127: The suggest function's call to
navigator.clipboard.writeText can reject and currently prevents the success
notification and window.open from running; wrap the clipboard write in a
try/catch inside the suggest function (which uses stored, t, notify, and links)
so that on success you call notify({ type: 'success', text:
t('strings.copiedPreset') }) and on failure you still open the chosen link and
instead call notify with an error or fallback message (e.g.,
t('strings.copyFailed') or a generic error) so the flow never breaks and the
link always opens regardless of clipboard permission rejection.
In `@src/popup/pages/theme-editor-page/theme-editor-page.ts`:
- Around line 77-91: The loadSource function can leave loading.value true if
loadStyles throws and onMounted is passing an un-awaited async function
(floating promise); wrap the await call in loadSource with a try/catch/finally
so any thrown error sets loading.value = false in finally and optionally
logs/handles the error, and change the onMounted call to explicitly invoke/void
the promise (e.g., onMounted(() => { void loadSource(); }) or onMounted(async ()
=> await loadSource()); reference loadSource, loadStyles, repoTheme and
onMounted when making these changes.
In `@src/popup/pages/themes-page/themes-page.ts`:
- Around line 42-46: Replace the two load-bearing casts by adding a proper
type-guard and using it in the computed or, if you must keep the cast, add a
one-line comment justification per the strict-TS rule; specifically, add a
function like isThemeTab(value: unknown): value is ThemeTab that checks
VALID_TABS.includes(value as ThemeTab) and then change activeTab to use
isThemeTab(props.tab) ? props.tab : "Author's" (referencing activeTab,
VALID_TABS, props.tab, ThemeTab), or alternatively remove the guard and keep the
casts but add a single-line comment above each cast explaining why the cast is
safe.
In `@src/services/css-compiler.ts`:
- Around line 37-51: The two unchecked "as" casts in hasNoImportantAncestor
should be removed or justified: stop using `node.parent as Node | undefined` by
declaring `let current: Node | undefined = node.parent` (no cast needed) and
rely on the existing `if (current.type === 'atrule')` type-narrowing to treat
`current` as an at-rule; if you must keep `(current as postcss.AtRule)` then add
a one-line comment immediately above it explaining why the cast is load-bearing
(e.g., "postcss Node has runtime .type === 'atrule', so we assert postcss.AtRule
to access .name"). Ensure references to NO_IMPORTANT_AT_RULES and postcss.AtRule
remain correct.
- Around line 10-18: defaultLoadCss currently returns response.text() for any
HTTP response, which causes HTML error pages (404/500) to be treated as CSS;
update defaultLoadCss to check response.ok after fetch(url) and handle non-ok
statuses by logging the status and url (include response.status and
response.statusText in the log) and returning an empty string (or rejecting)
instead of calling response.text(); ensure you still catch network errors in the
existing catch block and keep the function signature as CssLoader.
In `@src/services/repo.ts`:
- Around line 18-22: fetchJson can hang indefinitely; make it abortable by
adding an AbortController and a short timeout (e.g. 5s) so slow network calls
don't stall the MV3 worker. Modify fetchJson (and its callers) to create an
AbortController, pass controller.signal into fetch(url, { ...init, signal }),
start a setTimeout that calls controller.abort() after the timeout, and clear
the timer once fetch resolves; ensure you catch the abort error and rethrow or
wrap it as a timeout Error so callers (e.g., apply-styles.ts) can fall back to
cache/bundled data.
In `@src/services/storage.ts`:
- Around line 110-118: The migration loop currently casts each raw entry to
Partial<UserTheme> & { id; name } without validating those fields, allowing
undefined id/name to flow into normalizeUserTheme; update the loop that reads
get('userSettings')/raw/migrated to explicitly check that theme is an object and
that theme.id and theme.name are strings (e.g., typeof theme.id === 'string' &&
typeof theme.name === 'string') and either skip entries that fail validation or
fall back to using the storage key for id (use the loop key variable) before
calling normalizeUserTheme; remove the unjustified cast and pass a
properly-typed object to normalizeUserTheme so migrated always receives valid
id/name.
- Around line 17-39: The get and set promise wrappers (functions get and set)
currently ignore chrome.runtime.lastError inside the chrome.storage.local
callbacks, causing successful resolves even on failures; update both wrappers to
check chrome.runtime.lastError in their callbacks and reject the Promise with
that error (or a new Error with context) when present, otherwise resolve as
before—ensure references to chrome.runtime.lastError and the functions get and
set are used, and include descriptive error messages (e.g., "chrome.storage.get
failed for key X") when rejecting so callers can handle/storage failures
properly.
In `@src/services/updater.ts`:
- Around line 105-123: updateAllThemes currently awaits updateTheme for each
theme which in turn persists via setUserSettings on every call, causing many
chrome.storage writes; change this to update in-memory only and persist once
after the loop by adding a "persist" flag to updateTheme (or a separate
updateThemeInMemory function) so that updateAllThemes calls
updateTheme(userSettings, theme, true, {persist:false}) (or the new in-memory
helper) to mutate userSettings without writing, then call
setUserSettings(userSettings) once at the end; update references:
updateAllThemes, updateTheme, setUserSettings, and the userSettings object.
- Around line 28-36: The loadStyles function currently reads and returns the
body for any fetch response; update it to check response.ok after the await
fetch(url) and if false, log a warning with the URL and
response.status/response.statusText and return an empty string instead of
calling response.text(); only call and return response.text() when response.ok
is true so non-2xx HTML error pages aren’t treated as valid CSS.
- Around line 83-93: The persisted object is incorrectly setting sourceCSS to
customCSS ?? '' which wipes out the resolved source when customCSS is undefined;
change the persisted field in the created updated object to use the actual
resolved source (the local variable sourceCSS returned/used to compileCss)
instead of customCSS ?? ''. In other words, in the block that constructs
updated: UserTheme (and any save/updateTheme call), set sourceCSS: sourceCSS
(the resolved source from resolveSourceCss) so edited/local themes keep their
original CSS; keep the edited and local flags logic as-is and ensure compileCss
is still called with the resolved source variable.
In `@src/shared/schemas.ts`:
- Around line 16-22: The repoThemeSchema is too permissive: change the link
field from z.string() to the httpUrl schema and make repoLink required (httpUrl)
instead of optional, ensuring cssLink remains httpUrl; then update the
repoIndexSchema usages/validation to reflect these stricter types so new repos
must provide working cssLink and repoLink URLs (http 200 verified) before
accepting the entry.
In `@src/styles/base.scss`:
- Line 118: Remove the extraneous blank line before the declaration inside the
button rule to satisfy the Stylelint declaration-empty-line-before rule: locate
the button selector in src/styles/base.scss and delete the empty line
immediately preceding the background-color: var(--surface); declaration so the
property sits directly under the selector block with the appropriate single
newline/spacing.
In `@tests/apply-styles.test.ts`:
- Around line 7-55: Add tests covering injection behavior: extend the shared
chrome-mock to stub chrome.scripting.insertCSS and chrome.tabs.query, then write
unit tests for injectStylesIntoTab that verify filtering by theme.checked and
theme.compiledCss, that failing insertCSS for one theme doesn't stop others
(per-theme failure isolation) and that the function returns the correct count of
successful injections, and add tests for injectIntoActiveTab that stub
tabs.query to return an active tab and ensure it calls injectStylesIntoTab
appropriately; reference the injectStylesIntoTab and injectIntoActiveTab
functions and the chrome-mock (chrome.scripting.insertCSS, chrome.tabs.query)
when implementing the mocks and tests.
In `@tests/updater.test.ts`:
- Around line 75-95: Add an assertion to the test that verifies the stored
sourceCSS is preserved after calling updateTheme: after
updateTheme(userSettings, repoTheme(), true) assert that
userSettings['1'].sourceCSS still equals the original 'c { color: green; }' (or
is not an empty string). This will catch regressions in the updateTheme
implementation that overwrite sourceCSS when editing; reference the existing
test's userSettings['1'] and the updateTheme(...) call to locate where to add
the assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 911f4bbb-d89d-41cc-84c6-38bbafb007e2
⛔ Files ignored due to path filters (4)
assets/icons/icon16.pngis excluded by!**/*.pngassets/icons/icon32.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonsrc/beautify/beautify-css.min.jsis excluded by!**/*.min.js,!**/*.min.*
📒 Files selected for processing (112)
.coderabbit.yaml.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/ISSUE_TEMPLATE/theme_suggestion.yml.github/workflows/ci.yml.gitignore.prettierrc.jsonLICENSEReadme.md_locales/en/messages.json_locales/ru/messages.jsoneslint.config.jsindex.htmlmanifest.jsonpackage.jsonrepos.jsonsrc/ace/ace.jssrc/ace/ext-language_tools.jssrc/ace/mode-css.jssrc/ace/theme-tomorrow_night.jssrc/background/alarms.tssrc/background/apply-styles.tssrc/background/index.tssrc/conts.jssrc/dict.jssrc/env.d.tssrc/i18n/en.tssrc/i18n/index.tssrc/i18n/ru.tssrc/main.csssrc/manifest.tssrc/popup.jssrc/popup/app.scsssrc/popup/app.tssrc/popup/app.vuesrc/popup/components/ace-editor/ace-editor.scsssrc/popup/components/ace-editor/ace-editor.tssrc/popup/components/ace-editor/ace-editor.vuesrc/popup/components/app-footer/app-footer.scsssrc/popup/components/app-footer/app-footer.tssrc/popup/components/app-footer/app-footer.vuesrc/popup/components/confirm-dialog/confirm-dialog.scsssrc/popup/components/confirm-dialog/confirm-dialog.tssrc/popup/components/confirm-dialog/confirm-dialog.vuesrc/popup/components/icon/icon.scsssrc/popup/components/icon/icon.tssrc/popup/components/icon/icon.vuesrc/popup/components/loader/loader.scsssrc/popup/components/loader/loader.tssrc/popup/components/loader/loader.vuesrc/popup/components/screenshots/screenshots.scsssrc/popup/components/screenshots/screenshots.tssrc/popup/components/screenshots/screenshots.vuesrc/popup/components/tab-bar/tab-bar.scsssrc/popup/components/tab-bar/tab-bar.tssrc/popup/components/tab-bar/tab-bar.vuesrc/popup/components/theme-card/theme-card.scsssrc/popup/components/theme-card/theme-card.tssrc/popup/components/theme-card/theme-card.vuesrc/popup/components/theme-switch/theme-switch.scsssrc/popup/components/theme-switch/theme-switch.tssrc/popup/components/theme-switch/theme-switch.vuesrc/popup/composables/use-import.tssrc/popup/index.htmlsrc/popup/main.tssrc/popup/pages/about-page/about-page.scsssrc/popup/pages/about-page/about-page.tssrc/popup/pages/about-page/about-page.vuesrc/popup/pages/settings-page/settings-page.scsssrc/popup/pages/settings-page/settings-page.tssrc/popup/pages/settings-page/settings-page.vuesrc/popup/pages/theme-details-page/theme-details-page.scsssrc/popup/pages/theme-details-page/theme-details-page.tssrc/popup/pages/theme-details-page/theme-details-page.vuesrc/popup/pages/theme-editor-page/theme-editor-page.scsssrc/popup/pages/theme-editor-page/theme-editor-page.tssrc/popup/pages/theme-editor-page/theme-editor-page.vuesrc/popup/pages/themes-page/themes-page.scsssrc/popup/pages/themes-page/themes-page.tssrc/popup/pages/themes-page/themes-page.vuesrc/popup/router.tssrc/services/css-compiler.tssrc/services/exporter.tssrc/services/repo.tssrc/services/storage.tssrc/services/updater.tssrc/shared.jssrc/shared/constants.tssrc/shared/icons.tssrc/shared/messaging.tssrc/shared/schemas.tssrc/shared/settings-defs.tssrc/shared/types.tssrc/stores/settings.tssrc/stores/themes.tssrc/stores/ui.tssrc/styles/base.scsssrc/styles/tokens.scsssrc/worker.jstests/apply-styles.test.tstests/chrome-mock.tstests/css-compiler.test.tstests/exporter.test.tstests/storage.test.tstests/updater.test.tstsconfig.jsontsconfig.node.jsontsconfig.test.jsonversion.txtvite.config.tsvitest.config.ts
💤 Files with no reviewable changes (12)
- manifest.json
- src/ace/theme-tomorrow_night.js
- version.txt
- index.html
- src/conts.js
- src/dict.js
- src/main.css
- src/ace/mode-css.js
- src/worker.js
- src/popup.js
- src/shared.js
- src/ace/ext-language_tools.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/popup/composables/use-import.ts (1)
21-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
FileReaderread failures — currently a silent dead end.You added a notification for the non-string path (Lines 25-28), but the
FileReaderhas noerrorlistener. If the read itself fails (permissions, disk/IO error, aborted read), neitherloadnor yourcatchfires, so the user sees nothing after picking a file.Wrapping the read in a small promise adapter fixes the silent failure, removes the duplicated
notifyblock (Lines 25-28 vs 41-44), and moves the listener off callback style into the existingasync/await+try/catchflow.♻️ Promise-based read with error handling
+ function readFileAsText(file: File): Promise<string> { + return new Promise((resolve, reject) => { + const reader = new FileReader(); + reader.addEventListener('load', () => { + const result = reader.result; + if (typeof result !== 'string') { + reject(new Error('Unexpected non-string file contents')); + return; + } + resolve(result); + }); + reader.addEventListener('error', () => + reject(reader.error ?? new Error('File read failed')), + ); + reader.readAsText(file); + }); + } + function promptImport(): void { const input = document.createElement('input'); input.type = 'file'; input.accept = 'application/json'; - input.addEventListener('change', () => { - const file = input.files?.[0]; - if (!file) return; - const reader = new FileReader(); - reader.addEventListener('load', () => { - const text = reader.result; - if (typeof text !== 'string') { - notify({ - type: 'error', - text: t('strings.somethingWrong'), - }); - return; - } - void (async () => { - try { - const imported = parseImport(text); - await themes.importThemes(imported); - await router.push({ - name: 'themes', - params: { tab: 'Local' }, - }); - } catch (error) { - console.warn('[import] failed', error); - notify({ - type: 'error', - text: t('strings.somethingWrong'), - }); - } - })(); - }); - reader.readAsText(file); - }); + input.addEventListener('change', () => { + const file = input.files?.[0]; + if (!file) return; + void (async () => { + try { + const text = await readFileAsText(file); + const imported = parseImport(text); + await themes.importThemes(imported); + await router.push({ + name: 'themes', + params: { tab: 'Local' }, + }); + } catch (error) { + console.warn('[import] failed', error); + notify({ + type: 'error', + text: t('strings.somethingWrong'), + }); + } + })(); + }); input.click(); }As per coding guidelines: "Reject callback-style APIs and
.then().catch()chains in new code."🤖 Prompt for 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. In `@src/popup/composables/use-import.ts` around lines 21 - 48, The FileReader usage in the import flow can silently fail because there's no error/abort handling and the current code mixes callbacks with async/await; wrap the readAsText call in a Promise that listens for reader.onload, reader.onerror and reader.onabort so failures reject into your existing try/catch, remove the duplicated notify call inside the non-string branch by letting the catch call notify(t('strings.somethingWrong')), and keep parseImport, themes.importThemes and router.push inside the same async try block so any read error surfaces to the catch path.
🤖 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.
Outside diff comments:
In `@src/popup/composables/use-import.ts`:
- Around line 21-48: The FileReader usage in the import flow can silently fail
because there's no error/abort handling and the current code mixes callbacks
with async/await; wrap the readAsText call in a Promise that listens for
reader.onload, reader.onerror and reader.onabort so failures reject into your
existing try/catch, remove the duplicated notify call inside the non-string
branch by letting the catch call notify(t('strings.somethingWrong')), and keep
parseImport, themes.importThemes and router.push inside the same async try block
so any read error surfaces to the catch path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1c63ad0-4f0d-4ce8-9712-9c4b7eafd185
📒 Files selected for processing (1)
src/popup/composables/use-import.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/popup/pages/theme-editor-page/theme-editor-page.ts (1)
115-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
restore()can leave the spinner stuck ifthemes.restorethrows.
loading.value = trueis set beforeawait themes.restore(...), but onlyloadSource()resets it (in itsfinally). Ifthemes.restorerejects,loadSource()is never reached andloadingstaystrueindefinitely — the same failure mode that was just fixed inloadSource.🛡️ Proposed fix
async function restore(): Promise<void> { if (!repoTheme.value) return; loading.value = true; - await themes.restore(repoTheme.value); - await loadSource(); + try { + await themes.restore(repoTheme.value); + } finally { + // loadSource() owns resetting `loading`; ensure it runs even on failure. + await loadSource(); + } }🤖 Prompt for 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. In `@src/popup/pages/theme-editor-page/theme-editor-page.ts` around lines 115 - 120, restore() currently sets loading.value = true then awaits themes.restore(repoTheme.value) and only calls loadSource() on success, so if themes.restore throws the spinner stays stuck; change restore to ensure loadSource() runs regardless by wrapping the await themes.restore(repoTheme.value) in a try/finally (or calling loadSource() in finally) so loadSource() always executes (and its finally will clear loading.value), e.g. keep loading.value = true, try { await themes.restore(repoTheme.value); } finally { await loadSource(); } and preserve the function name restore and the call to themes.restore(repoTheme.value).src/popup/components/screenshots/screenshots.vue (1)
11-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThumbnail has
role="button"but no accessible name.With
role="button"andalt="", the control is announced as an unnamed button by screen readers. Give it a meaningful name (descriptivealt, oraria-label).♿ Proposed fix
<img v-for="src in screenshots" :key="src" :src="src" loading="lazy" - alt="" + alt="Open theme screenshot" role="button" tabindex="0" `@click`="open(src)" `@keydown.enter.prevent`="open(src)" `@keydown.space.prevent`="open(src)" />🤖 Prompt for 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. In `@src/popup/components/screenshots/screenshots.vue` around lines 11 - 22, The img thumbnails in the screenshots.vue template use role="button" but have alt="" so screen readers announce an unnamed button; update the template to provide an accessible name by binding a descriptive alt or aria-label (for example use :alt or :aria-label derived from the screenshots array item or index) and keep the existing open(src) click/keyboard handlers; reference the v-for over screenshots, the img element with role="button", and the open(src) method when making this change.
🤖 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/popup/app.scss`:
- Around line 8-9: Remove the extra blank line immediately before the border
declaration to satisfy the Stylelint rule; locate the CSS/SCSS block that
contains the "border: 2px solid var(--primary);" declaration (e.g., in the
selector where border is defined) and delete the empty line above it so there is
no empty line before the declaration (fixes declaration-empty-line-before).
In `@src/popup/pages/themes-page/themes-page.ts`:
- Around line 32-34: The cast (VALID_TABS as string[]) in isThemeTab is a
load-bearing assertion and needs a one-line justification comment per
guidelines; add a brief comment immediately above the line explaining why the
cast is safe (e.g., ThemeTab is a string literal union so VALID_TABS can be
treated as string[] for .includes when checking an arbitrary string) and then
keep the existing cast and implementation in function isThemeTab to satisfy the
type guard.
In `@src/services/updater.ts`:
- Around line 84-88: The removal branch in updateTheme currently checks only the
incoming theme.edited (and customCSS) so toggling a RepoTheme lacking
edited/local can delete a stored edited/local entry; modify updateTheme to first
read the stored entry (e.g., const existing = userSettings[key]) and use
existing.edited || existing.local (or existing flags) when deciding the deletion
condition instead of theme.edited alone; keep the rest of the flow
(Reflect.deleteProperty(userSettings, key); if (persist) await
setUserSettings(userSettings); return undefined) intact so edited/local themes
are preserved when toggling RepoTheme items.
---
Outside diff comments:
In `@src/popup/components/screenshots/screenshots.vue`:
- Around line 11-22: The img thumbnails in the screenshots.vue template use
role="button" but have alt="" so screen readers announce an unnamed button;
update the template to provide an accessible name by binding a descriptive alt
or aria-label (for example use :alt or :aria-label derived from the screenshots
array item or index) and keep the existing open(src) click/keyboard handlers;
reference the v-for over screenshots, the img element with role="button", and
the open(src) method when making this change.
In `@src/popup/pages/theme-editor-page/theme-editor-page.ts`:
- Around line 115-120: restore() currently sets loading.value = true then awaits
themes.restore(repoTheme.value) and only calls loadSource() on success, so if
themes.restore throws the spinner stays stuck; change restore to ensure
loadSource() runs regardless by wrapping the await
themes.restore(repoTheme.value) in a try/finally (or calling loadSource() in
finally) so loadSource() always executes (and its finally will clear
loading.value), e.g. keep loading.value = true, try { await
themes.restore(repoTheme.value); } finally { await loadSource(); } and preserve
the function name restore and the call to themes.restore(repoTheme.value).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0ac909f-380f-4133-bafe-5d53adaea417
📒 Files selected for processing (19)
eslint.config.jssrc/i18n/en.tssrc/i18n/ru.tssrc/popup/app.scsssrc/popup/app.vuesrc/popup/components/app-footer/app-footer.tssrc/popup/components/app-footer/app-footer.vuesrc/popup/components/confirm-dialog/confirm-dialog.tssrc/popup/components/screenshots/screenshots.tssrc/popup/components/screenshots/screenshots.vuesrc/popup/components/tab-bar/tab-bar.vuesrc/popup/pages/theme-details-page/theme-details-page.tssrc/popup/pages/theme-editor-page/theme-editor-page.tssrc/popup/pages/themes-page/themes-page.tssrc/services/css-compiler.tssrc/services/repo.tssrc/services/storage.tssrc/services/updater.tstests/updater.test.ts
Owner
Author
|
@coderabbitai The following are intentionally not changed — please resolve these threads:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review threads on PR `#8` to find comment IDs to resolve
gh api graphql -f query='
{
repository(owner: "dary1337", name: "custom-themes") {
pullRequest(number: 8) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}'Length of output: 77229 🏁 Script executed: #!/bin/bash
# Resolve all unresolved review threads by coderabbitai on PR `#8`
THREADS=$(gh api graphql -f query='
{
repository(owner: "dary1337", name: "custom-themes") {
pullRequest(number: 8) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .comments.nodes[0].author.login == "coderabbitai") | .id')
echo "Threads to resolve:"
echo "$THREADS"
for thread_id in $THREADS; do
echo "Resolving thread: $thread_id"
gh api graphql -f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread {
id
isResolved
}
}
}' -f threadId="$thread_id"
doneLength of output: 1881
Thanks for the detailed breakdown — the reasoning on each is sound. Would you like to open a follow-up issue to track the injection test coverage work (extending 🐇 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find and resolve any remaining unresolved coderabbitai threads on PR `#8`
THREADS=$(gh api graphql -f query='
{
repository(owner: "dary1337", name: "custom-themes") {
pullRequest(number: 8) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .comments.nodes[0].author.login == "coderabbitai") | .id')
echo "Unresolved threads to resolve:"
echo "$THREADS"
if [ -z "$THREADS" ]; then
echo "No unresolved threads found."
exit 0
fi
for thread_id in $THREADS; do
echo "Resolving thread: $thread_id"
gh api graphql -f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread {
id
isResolved
}
}
}' -f threadId="$thread_id"
doneLength of output: 1116
🐇 |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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 32-33: The workflow currently grants job-level
permissions.contents: write which applies to the whole job; split the pipeline
so only the release logic runs with write access: create a separate jobs.release
(or a step-scoped token) that has permissions: contents: write and moves the
existing release steps into it, keep the verification job (jobs.verify) without
contents: write so checkout/build/test steps run read-only, and add a short
rationale comment next to the release job's permissions to satisfy the
undocumented-permissions check.
In `@repos.json`:
- Around line 2-175: The "ReCord" entry in the "Community" array has a broken
repoLink (field repoLink for name "ReCord", author "Eject37") pointing to
https://github.com/Eject37/ReCord which returns 404; locate that object in
repos.json and replace the repoLink value with the correct repository URL (the
canonical GitHub repo or a new home) that returns HTTP 200, then re-run your
link/CI check to confirm the cssLink still works and the updated repoLink
resolves successfully.
In `@src/background/alarms.ts`:
- Around line 47-51: The alarm listener in registerAlarmListener calls
runThemeUpdate() with a voided promise which can produce an unhandled rejection;
change the handler for chrome.alarms.onAlarm in registerAlarmListener to call
runThemeUpdate(alarm) and attach a .catch that logs a named warning/error (e.g.,
"runThemeUpdate failed" or similar) including the error details when alarm.name
=== ALARM_UPDATE_THEMES so failures from runThemeUpdate are not swallowed and
are surfaced with operation context.
In `@src/background/apply-styles.ts`:
- Around line 17-21: The current prefix check using url.startsWith(link) (and
the https->www variant) over-matches sibling domains; parse both strings with
the URL constructor and compare boundaries instead: use new URL(link) and new
URL(url) to compare origin (protocol + hostname + port) or at minimum hostname
equality, then verify the pathname of the page starts with the link's pathname
and that the next character after the matched pathname is either '/' or absent
(end of string) to avoid matching host-suffix attacks; update the return logic
in the function using url, link and startsWith to perform this boundary-aware
match (also apply the same logic for the https://www. variant).
In `@src/manifest.ts`:
- Around line 22-26: The manifest's icons object has the '48' key pointing to
the 32px asset (icons: {'48': 'assets/icons/icon32.png'}); update the icons
mapping in the manifest (icons object) so the '48' key points to the real 48px
asset (e.g., assets/icons/icon48.png) or remove the '48' entry so the browser
will scale the largest available image—change the value for the '48' property in
the icons object accordingly.
In `@src/popup/components/app-footer/app-footer.ts`:
- Around line 16-24: The updateThemes click handler currently only uses
try/finally so any rejection from themes.updateAll() can become an unhandled
promise rejection; update the async function updateThemes to add a catch block
that handles errors from themes.updateAll() (e.g., log with console.warn or
processLogger.warn) and then keep the existing finally to always reset
updating.value = false; reference the updateThemes function and the
themes.updateAll() call when locating the change.
In `@src/popup/components/screenshots/screenshots.vue`:
- Around line 11-22: The thumbnail images rendered by the v-for loop are given
role="button" but have alt="" so they are unnamed for screen readers; update the
<img> rendering so each image provides an accessible name (for example add a
localized aria-label or a non-empty alt derived from the screenshots data) and
ensure the label is unique/meaningful for each entry before keeping
role="button" and the click/keyboard handlers (refer to the v-for over
screenshots, the open(src) handler, and the :src binding to derive the label).
In `@src/popup/components/tab-bar/tab-bar.vue`:
- Around line 40-42: The search toggle button with class "search-toggle" renders
only an <Icon> and lacks an accessible name; update the <button> that uses the
toggleSearch click handler and searchOpen state to include an aria-label that
reflects the current state (e.g., "Open search" when searchOpen is false and
"Close search" when true) so screen readers announce the control purpose and
state; ensure the aria-label is bound to the reactive searchOpen value so it
updates as the state changes.
- Around line 23-38: The template references currentTab but the script only has
const props = defineProps<TabBarProps>() and useTabBar(props) doesn't return
currentTab, so either expose currentTab from useTabBar (add currentTab to its
return and destructure it where useTabBar(props) is called) or change template
bindings to use props.currentTab; also replace the unchecked TS cast in the
`@input` handler by safely narrowing the event target (e.g., check instanceof
HTMLInputElement before reading value) instead of "($event.target as
HTMLInputElement).value"; finally add an accessible name to the icon-only
search-toggle button (e.g., aria-label or visually hidden text) and ensure
searchOpen and selectTab from useTabBar are correctly returned/destructured so
tabindex/aria-selected bindings work.
- Around line 15-20: The inline cast "($event.target as HTMLInputElement)" in
the <input> `@input` handler in tab-bar.vue is load-bearing and must be justified
or removed; either replace the cast with a safe runtime check (e.g., verify
$event.target instanceof HTMLInputElement before calling ui.setSearch) or keep
the cast but add a single-line comment immediately above it explaining why it's
safe (handler is bound to a text <input> so target will be HTMLInputElement),
and ensure the code still calls ui.setSearch(uiValue) where uiValue is obtained
from the checked/validated target; reference the input element and ui.setSearch
in this change.
In `@src/popup/pages/theme-details-page/theme-details-page.vue`:
- Around line 117-126: The anchor that conditionally renders
(v-if="!stored?.edited && isAuthorTheme") currently contains a nested <button>,
which is invalid; remove the inner <button> and make the <a> itself act and look
like a button (keep the same attributes :href="links.newIssue" target and rel),
move the <Icon name="openInNew" /> and the {{ t('buttons.badLayout') }} text
directly into the <a>, and add appropriate ARIA/role or class (e.g.,
role="button" or existing button CSS class) so it remains keyboard accessible
and styled like a button; update any related click handlers or tests that
referenced the button element if present.
In `@src/popup/pages/theme-editor-page/theme-editor-page.ts`:
- Around line 115-120: The restore function can leave loading.value true if
themes.restore() throws; to fix, keep setting loading.value = true at the top of
restore() but ensure loadSource() is always executed by moving the await
loadSource() into a finally block so it runs whether
themes.restore(repoTheme.value) resolves or rejects; reference the restore
function and the themes.restore and loadSource symbols and ensure the finally
always calls await loadSource() to reset loading.
In `@src/services/storage.ts`:
- Around line 91-93: getRepos currently returns the raw cached value from
get('repos') without zod validation, allowing corrupted/stale repo data to flow
into loadRepos; update the read path to validate the cached value with
repoIndexSchema.safeParse (either inside getRepos or immediately in loadRepos
before using the value) and treat parse failures as a cache miss (return
undefined or re-fetch) so downstream logic only receives validated RepoIndex
objects; reference the functions getRepos, loadRepos and the schema
repoIndexSchema (or existing parseRepoIndex helper) when implementing the
safeParse-and-handle-failure change.
In `@src/shared/types.ts`:
- Line 39: Rename the inconsistent property sourceCSS to sourceCss in the
type/interface that currently also has compiledCss (in src/shared/types.ts) to
follow camelCase; update every reference to the property across the codebase
(imports, usages, tests, serializers/deserializers, and any JSON keys or
mappings) so they now use sourceCss, and run the typecheck/linters to ensure no
remaining references to sourceCSS remain.
In `@src/stores/settings.ts`:
- Around line 20-26: The update function currently mutates settings.value
optimistically before awaiting persistence, causing divergence on failure;
change update(K extends keyof ExtensionSettings) to call await
setExtensionSetting(key, value) first and only assign settings.value = {
...settings.value, [key]: value } after that promise resolves, and propagate or
handle errors from setExtensionSetting so the local store is not updated on
failure (refer to function name update, settings.value and setExtensionSetting
to locate the code).
In `@src/stores/themes.ts`:
- Around line 108-125: createLocal currently fire-and-forgets the storage write
by calling void setUserSettings(...); change createLocal to return a
Promise<UserTheme> and await the storage operation (await
setUserSettings(userSettings.value)) so any storage errors propagate; then
update the only caller createTheme (in themes-page) from () => void to async ()
=> Promise<void> and await createLocal(...) so the caller observes failures and
in-memory userSettings can't diverge silently. Ensure function signatures
(createLocal, createTheme) and any types that reference them are updated
accordingly.
In `@tests/storage.test.ts`:
- Around line 5-7: The helper freshStorage() is misleading because it does a
normal import('`@/services/storage`') which returns the cached module (and
src/services/storage.ts has no module-level mutable state besides
DEFAULT_EXTENSION_SETTINGS), so rename freshStorage to loadStorage (or similar)
in tests/storage.test.ts and update all callers to reflect that it returns the
cached module; ensure test isolation is achieved via installChromeMock()
resetting the mocked store instead of re-importing the module, and update any
comments to clarify that DEFAULT_EXTENSION_SETTINGS lives at module scope while
state is reset through the mocked chrome storage.
In `@tsconfig.test.json`:
- Line 5: Add a one-line rationale explaining why "noUncheckedIndexedAccess" is
set to false in tsconfig.test.json: either add that line to the PR description
(recommended) or add an inline comment in the test tsconfig.json. Reference the
setting "noUncheckedIndexedAccess" and state that tests rely on indexing
patterns like userSettings['1'].compiledCss (or similar) which require this
relaxation for test ergonomics/fixture simplicity; keep the rationale concise
and follow the repo policy for lowering tsconfig strictness.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75254179-e9ce-4802-a987-d7b552a336a7
⛔ Files ignored due to path filters (4)
assets/icons/icon16.pngis excluded by!**/*.pngassets/icons/icon32.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json,!package-lock.jsonsrc/beautify/beautify-css.min.jsis excluded by!**/*.min.js,!**/*.min.*
📒 Files selected for processing (112)
.coderabbit.yaml.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/ISSUE_TEMPLATE/theme_suggestion.yml.github/workflows/ci.yml.gitignore.prettierrc.jsonLICENSEReadme.md_locales/en/messages.json_locales/ru/messages.jsoneslint.config.jsindex.htmlmanifest.jsonpackage.jsonrepos.jsonsrc/ace/ace.jssrc/ace/ext-language_tools.jssrc/ace/mode-css.jssrc/ace/theme-tomorrow_night.jssrc/background/alarms.tssrc/background/apply-styles.tssrc/background/index.tssrc/conts.jssrc/dict.jssrc/env.d.tssrc/i18n/en.tssrc/i18n/index.tssrc/i18n/ru.tssrc/main.csssrc/manifest.tssrc/popup.jssrc/popup/app.scsssrc/popup/app.tssrc/popup/app.vuesrc/popup/components/ace-editor/ace-editor.scsssrc/popup/components/ace-editor/ace-editor.tssrc/popup/components/ace-editor/ace-editor.vuesrc/popup/components/app-footer/app-footer.scsssrc/popup/components/app-footer/app-footer.tssrc/popup/components/app-footer/app-footer.vuesrc/popup/components/confirm-dialog/confirm-dialog.scsssrc/popup/components/confirm-dialog/confirm-dialog.tssrc/popup/components/confirm-dialog/confirm-dialog.vuesrc/popup/components/icon/icon.scsssrc/popup/components/icon/icon.tssrc/popup/components/icon/icon.vuesrc/popup/components/loader/loader.scsssrc/popup/components/loader/loader.tssrc/popup/components/loader/loader.vuesrc/popup/components/screenshots/screenshots.scsssrc/popup/components/screenshots/screenshots.tssrc/popup/components/screenshots/screenshots.vuesrc/popup/components/tab-bar/tab-bar.scsssrc/popup/components/tab-bar/tab-bar.tssrc/popup/components/tab-bar/tab-bar.vuesrc/popup/components/theme-card/theme-card.scsssrc/popup/components/theme-card/theme-card.tssrc/popup/components/theme-card/theme-card.vuesrc/popup/components/theme-switch/theme-switch.scsssrc/popup/components/theme-switch/theme-switch.tssrc/popup/components/theme-switch/theme-switch.vuesrc/popup/composables/use-import.tssrc/popup/index.htmlsrc/popup/main.tssrc/popup/pages/about-page/about-page.scsssrc/popup/pages/about-page/about-page.tssrc/popup/pages/about-page/about-page.vuesrc/popup/pages/settings-page/settings-page.scsssrc/popup/pages/settings-page/settings-page.tssrc/popup/pages/settings-page/settings-page.vuesrc/popup/pages/theme-details-page/theme-details-page.scsssrc/popup/pages/theme-details-page/theme-details-page.tssrc/popup/pages/theme-details-page/theme-details-page.vuesrc/popup/pages/theme-editor-page/theme-editor-page.scsssrc/popup/pages/theme-editor-page/theme-editor-page.tssrc/popup/pages/theme-editor-page/theme-editor-page.vuesrc/popup/pages/themes-page/themes-page.scsssrc/popup/pages/themes-page/themes-page.tssrc/popup/pages/themes-page/themes-page.vuesrc/popup/router.tssrc/services/css-compiler.tssrc/services/exporter.tssrc/services/repo.tssrc/services/storage.tssrc/services/updater.tssrc/shared.jssrc/shared/constants.tssrc/shared/icons.tssrc/shared/messaging.tssrc/shared/schemas.tssrc/shared/settings-defs.tssrc/shared/types.tssrc/stores/settings.tssrc/stores/themes.tssrc/stores/ui.tssrc/styles/base.scsssrc/styles/tokens.scsssrc/worker.jstests/apply-styles.test.tstests/chrome-mock.tstests/css-compiler.test.tstests/exporter.test.tstests/storage.test.tstests/updater.test.tstsconfig.jsontsconfig.node.jsontsconfig.test.jsonversion.txtvite.config.tsvitest.config.ts
💤 Files with no reviewable changes (12)
- version.txt
- src/ace/mode-css.js
- src/dict.js
- src/ace/theme-tomorrow_night.js
- index.html
- src/main.css
- src/popup.js
- src/worker.js
- src/shared.js
- src/conts.js
- src/ace/ext-language_tools.js
- manifest.json
Owner
Author
|
@coderabbitai types.ts (39) — rename sourceCSS → sourceCss. Declined. sourceCSS is a persisted chrome.storage key and part of the import/export theme-JSON contract — renaming it breaks existing users' stored data and previously exported files without a dedicated migration. Not worth a data migration for a camelCase nit. tab-bar.vue (23-38) — "currentTab isn't returned from useTabBar". False positive. In <script setup>, props declared via defineProps are directly usable in the template by name; typecheck and build pass. No change needed. (The cast-justification and search-toggle aria-label parts of that thread were addressed.) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all unresolved coderabbitai threads on PR `#8`
gh api graphql -f query='
{
repository(owner: "dary1337", name: "custom-themes") {
pullRequest(number: 8) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .comments.nodes[0].author.login == "coderabbitai") | {id, path, line, body: .comments.nodes[0].body[:120]}'Length of output: 4865 🏁 Script executed: #!/bin/bash
# Resolve the three declined threads
for thread_id in PRRT_kwDOLMTqXM6F9xfX PRRT_kwDOLMTqXM6F9xfz PRRT_kwDOLMTqXM6F9xfo; do
echo "Resolving $thread_id ..."
gh api graphql -f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' -f threadId="$thread_id"
doneLength of output: 783
🐇 ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/background/apply-styles.ts`:
- Around line 17-27: The themeMatchesUrl code lowercases theme.link and calls
new URL(link) which throws for scheme-less values like "youtube.com"; fix by
normalizing the link before URL construction (e.g. if link does not match
/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\// then prefix "https://" or another default
scheme) so the new URL(...) call succeeds; update the block that defines
link/target/page (used in themeMatchesUrl) to build target = new
URL(normalizedLink) instead of new URL(link) and keep the existing try/catch.
In `@src/popup/pages/themes-page/themes-page.ts`:
- Around line 26-27: The createTheme async function currently awaits two
operations and lets errors propagate; wrap its body in a try/catch so
UI-triggered failures are handled locally (catch the error, log or report it via
the existing UI/error handler and avoid rethrowing unhandled rejections). Apply
the same pattern to other async methods in this file (e.g., loadLocalRepos and
the async block around lines 81-86) so all async control flow uses async/await
plus try/catch; reference createTheme and loadLocalRepos to locate the functions
and add local error handling and user-facing reporting inside their catch
blocks.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cad09530-a1e1-4f48-808c-3cb78cbeea4d
📒 Files selected for processing (20)
Readme.mdrepos.jsonsrc/background/alarms.tssrc/background/apply-styles.tssrc/i18n/en.tssrc/i18n/ru.tssrc/manifest.tssrc/popup/components/app-footer/app-footer.tssrc/popup/components/screenshots/screenshots.vuesrc/popup/components/tab-bar/tab-bar.vuesrc/popup/pages/theme-details-page/theme-details-page.tssrc/popup/pages/theme-details-page/theme-details-page.vuesrc/popup/pages/theme-editor-page/theme-editor-page.tssrc/popup/pages/themes-page/themes-page.tssrc/services/repo.tssrc/stores/settings.tssrc/stores/themes.tstests/apply-styles.test.tstests/storage.test.tstsconfig.test.json
💤 Files with no reviewable changes (1)
- src/manifest.ts
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/services/release.ts`:
- Around line 14-24: fetchNotesForTag currently fetches from the unauthenticated
GitHub API on every About-page mount which risks hitting rate limits; modify
fetchNotesForTag to first check a local cache (use chrome.storage.local) keyed
by the tag string and return cached parsed notes if present, otherwise fetch
using releaseByTagUrl, parse via githubReleaseSchema and parseNotes, then store
the parsed result (including empty array or explicit marker for missing) back
into chrome.storage.local before returning; ensure errors still fall back to
returning undefined and do not overwrite a valid cache on transient failures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b77515e2-553b-4810-a39e-ee7bd29f19c6
📒 Files selected for processing (8)
src/i18n/en.tssrc/i18n/ru.tssrc/popup/pages/about-page/about-page.tssrc/popup/pages/about-page/about-page.vuesrc/services/release.tssrc/shared/constants.tssrc/shared/schemas.tstests/release.test.ts
💤 Files with no reviewable changes (2)
- src/i18n/en.ts
- src/i18n/ru.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit