Linux Mint add .deb and resolve mac#4407
Open
Rocket-Space wants to merge 57 commits intopear-devs:masterfrom
Open
Linux Mint add .deb and resolve mac#4407Rocket-Space wants to merge 57 commits intopear-devs:masterfrom
Rocket-Space wants to merge 57 commits intopear-devs:masterfrom
Conversation
…tion and automated download support
…artifact generation workflow
…electron-builder dependencies
…ktop across platforms
…S para estabilizar Linux/Windows
…me to summarize." This reverts commit d954004.
…me to summarize." This reverts commit c311928.
…me to summarize." This reverts commit 327b6c4.
descomprime y sube los instaladores de cada artefacto
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI workflow restructuring and release automation; introduces an ad‑blocker plugin (multiple strategies, preload injector, session binding) and a Download Manager with queueing, concurrency, multi‑provider retries, IPC and UI; updates packaging, deps, icons, styles, and i18n strings. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (UI)
participant Main as Main (Process)
participant DM as DownloadManager (Engine)
participant Provider as Provider (Downloader)
participant FS as Filesystem
Renderer->>Main: invoke('download-manager-add', item)
Main->>DM: addToQueue(item)
DM-->>Main: emit 'download-manager-state' (queued)
DM->>DM: promote queued -> downloading (respect maxConcurrent)
DM->>Provider: startAttempt(itemId, provider, attemptNo)
Provider-->>DM: progress(itemId, percent, provider, attempt)
DM-->>Main: emit 'download-manager-item-update' (progress)
Provider-->>DM: success(stream)
DM->>FS: write file(stream, dest)
FS-->>DM: write complete
DM-->>Main: emit 'download-manager-item-update' (completed)
DM->>Renderer: state update (completed)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
.github/workflows/build.yml-46-46 (1)
46-46:⚠️ Potential issue | 🟡 MinorInconsistent lockfile handling between build and release jobs.
The build job uses
--no-frozen-lockfile(line 46) while the release job uses--frozen-lockfile(line 111). This inconsistency could lead to different dependency versions between build and release, potentially causing unexpected behavior.Also applies to: 110-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml at line 46, The workflow has inconsistent pnpm install flags between the build and release jobs: change one so both use the same lockfile behaviour; either make the build job use --frozen-lockfile like the release job or make the release job use --no-frozen-lockfile so dependency resolution is consistent. Update the run commands for the build job (currently "pnpm install --no-frozen-lockfile") and the release job (currently "pnpm install --frozen-lockfile") to use the same flag across both jobs..github/workflows/publish-installers.yml-265-265 (1)
265-265:⚠️ Potential issue | 🟡 MinorDeprecated
::set-outputsyntax.The
::set-outputcommand is deprecated and will be removed. Use$GITHUB_OUTPUTenvironment file instead.🔧 Proposed fix
-echo "::set-output name=release_tag::${RELEASE_TAG}" +echo "release_tag=${RELEASE_TAG}" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-installers.yml at line 265, Replace the deprecated ::set-output usage with writing the output key/value to the GitHub Actions environment file: instead of the line that uses ::set-output for release_tag, append a line with the literal key "release_tag" and the value from ${RELEASE_TAG} to the $GITHUB_OUTPUT file (ensure proper quoting/expansion and use append mode so other outputs are preserved); update the step that emitted release_tag to use this method and remove the old ::set-output syntax..github/workflows/build-macos.yml-25-31 (1)
25-31:⚠️ Potential issue | 🟡 MinorXcode version pattern
Xcode_26*.appappears incorrect.The pattern
/Applications/Xcode_26*.appsearches for Xcode version 26, but the current latest Xcode is around version 15-16. This pattern will never match, and the workflow will always fall back to the genericXcode*.apppattern.If this is intentional future-proofing, consider using a more sensible pattern. If it was meant to target a specific version, the version number appears to be a typo.
🔧 Suggested fix if targeting latest Xcode
- name: Setup Xcode version run: | - XCODE_APP=$(ls -d /Applications/Xcode_26*.app 2>/dev/null | sort -r | head -n 1) + XCODE_APP=$(ls -d /Applications/Xcode_*.app 2>/dev/null | sort -V -r | head -n 1) if [ -z "$XCODE_APP" ]; then XCODE_APP=$(ls -d /Applications/Xcode*.app | sort -r | head -n 1); fiUsing
sort -Vprovides proper version sorting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-macos.yml around lines 25 - 31, The Xcode glob pattern /Applications/Xcode_26*.app is incorrect (likely a typo) and will never match current installs; update the first search to a sensible pattern like /Applications/Xcode_*.app (remove the "26") and use version-aware sorting (e.g., sort -V) when choosing XCODE_APP so the newest installed Xcode is selected; ensure the fallback that checks /Applications/Xcode*.app remains in place and that XCODE_APP is exported/used when calling xcode-select and xcodebuild.package.json-159-160 (1)
159-160:⚠️ Potential issue | 🟡 MinorConfirm the rationale for downgrading electron-builder from v26.7.0 to v24.13.3.
While v24.13.3 is safe (patched for CVE-2024-27303 and has no known vulnerabilities), this is still a two major-version downgrade. Staying on an older version means missing ongoing security updates and improvements from v25 and v26. Please clarify the specific build or compatibility issue that required this downgrade, and consider whether v26 can be restored if the issue is addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 159 - 160, Update the PR with a concise justification for downgrading "electron-builder" and "electron-builder-squirrel-windows" from v26.7.0 to v24.13.3: describe the exact build/compatibility failure observed (e.g., failing build step, broken plugin, CI error), name the affected command or script (npm/yarn script that invokes electron-builder), and note any attempts to resolve it (error logs, repro steps, or patches tried). Also state whether restoring v26 is feasible (blocking bug, missing plugin support, or workaround) and, if possible, include a follow-up action (e.g., open issue/upgrade plan) so reviewers know if this is temporary or permanent.src/i18n/resources/es.json-588-591 (1)
588-591:⚠️ Potential issue | 🟡 MinorSame hardcoded retry count issue as en.json.
The downloading status string also hardcodes
/3. Keep this in sync with the English version when updating to use an interpolation variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/resources/es.json` around lines 588 - 591, The Spanish "status.downloading" string hardcodes "/3"; update it to use the same interpolation variable used in the English version (e.g., change "Descargando… {{progress}}% — {{provider}} ({{attempt}}/3)" to use a max-attempt variable like "Descargando… {{progress}}% — {{provider}} ({{attempt}}/{{maxAttempts}})") and ensure the interpolation key name exactly matches the English resource so runtime interpolation stays in sync.src/i18n/resources/en.json-588-591 (1)
588-591:⚠️ Potential issue | 🟡 MinorHardcoded retry count in i18n string.
The status string
"downloading": "Downloading… {{progress}}% — {{provider}} ({{attempt}}/3)"hardcodes the maximum retry count as/3. IfMAX_RETRIES_PER_PROVIDERchanges in the backend, this string becomes inconsistent. Consider using an interpolation variable like{{maxAttempts}}for consistency.🛠️ Proposed fix
- "downloading": "Downloading… {{progress}}% — {{provider}} ({{attempt}}/3)", + "downloading": "Downloading… {{progress}}% — {{provider}} ({{attempt}}/{{maxAttempts}})",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/resources/en.json` around lines 588 - 591, The i18n string at key "status.downloading" currently hardcodes "/3" which will diverge from the backend constant MAX_RETRIES_PER_PROVIDER; update the English resource value to use an interpolation like "{{maxAttempts}}" instead of "3" and then ensure wherever the translation for "status.downloading" is invoked (the code that supplies progress, provider and attempt) also passes maxAttempts: MAX_RETRIES_PER_PROVIDER so the rendered string stays consistent with the backend constant.src/plugins/downloader/menu.ts-217-228 (1)
217-228:⚠️ Potential issue | 🟡 MinorRemove hardcoded Spanish fallback string.
The fallback
'Descargas simultáneas'is in Spanish, but the primary language for this codebase is English. If the i18n key is missing, the fallback should be in English (e.g.,'Concurrent downloads'). However, since the key is defined inen.json, the fallback shouldn't be necessary at all.🛠️ Proposed fix
{ - label: t('plugins.downloader.menu.concurrent-downloads') || 'Descargas simultáneas', + label: t('plugins.downloader.menu.concurrent-downloads'), type: 'submenu', submenu: [1, 2, 3, 4, 5].map((n) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/menu.ts` around lines 217 - 228, Remove the hardcoded Spanish fallback for the menu label by eliminating the "|| 'Descargas simultáneas'" or replacing it with an English fallback ("Concurrent downloads"); update the label expression that uses t('plugins.downloader.menu.concurrent-downloads') in the submenu block so it relies on the i18n key (or an English fallback) instead of the Spanish string—this change affects the menu item where config.maxConcurrentDownloads is used and the click handler calls setConfig({ maxConcurrentDownloads: n }).src/plugins/downloader/main/index.ts-605-607 (1)
605-607:⚠️ Potential issue | 🟡 MinorHardcoded Spanish error message - use i18n.
The error message on line 606 is hardcoded in Spanish:
"Todos los proveedores fallaron...". This should use an i18n key for consistency with the rest of the codebase.🌐 Proposed fix
if (!success) { - throw new Error(`Todos los proveedores fallaron. Por favor verifica tu conexión o actualiza. Último error: ${lastError instanceof Error ? lastError.message : String(lastError)}`); + throw new Error( + t('plugins.downloader.backend.feedback.all-providers-failed', { + error: lastError instanceof Error ? lastError.message : String(lastError), + }), + ); }Note: You'll need to add the corresponding i18n key to the translation files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/main/index.ts` around lines 605 - 607, Replace the hardcoded Spanish error string thrown when all providers fail with an i18n lookup: instead of `throw new Error('Todos los proveedores fallaron...')` use the project's i18n helper (e.g., i18n.t or translate) to build the message and interpolate lastError (use `lastError instanceof Error ? lastError.message : String(lastError)`), updating the throw in the block that checks `if (!success)` in the downloader module (index.ts) and add the corresponding translation key/value to the locale files so the message is available in supported languages.src/plugins/downloader/main/download-manager-engine.ts-237-240 (1)
237-240:⚠️ Potential issue | 🟡 Minor
clearCompleted()leaves stale totals behind.This clears
completedItems, buttotalCompletedandtotalSkippedkeep their old values. The next state payload says the list is empty while the counters still show completed/skipped items.Suggested fix
clearCompleted(): void { this.completedItems = []; + this.totalCompleted = 0; + this.totalSkipped = 0; this.broadcastState(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/main/download-manager-engine.ts` around lines 237 - 240, The clearCompleted() method currently empties completedItems but leaves totalCompleted and totalSkipped stale; update clearCompleted() to also reset those counters (set totalCompleted and totalSkipped to 0 or recompute them consistently) before calling broadcastState() so the broadcasted state reflects the cleared list; reference clearCompleted(), completedItems, totalCompleted, totalSkipped, and broadcastState() when making the change.src/plugins/downloader/main/download-manager-engine.ts-82-87 (1)
82-87:⚠️ Potential issue | 🟡 MinorNormalize
maxConcurrentto a finite integer.
Math.max(1, Math.min(5, max))passes throughNaNand fractions. TheprocessQueue()loop at line 293 comparesthis.activeDownloads < this.maxConcurrent, which means:
NaNprevents the queue from ever processing (0 < NaN is false)2.5allows 3 concurrent downloads instead of 2 (loop runs while 0, 1, 2 < 2.5)Suggested fix
setMaxConcurrent(max: number): void { - this.maxConcurrent = Math.max(1, Math.min(5, max)); + const normalized = Number.isFinite(max) ? Math.trunc(max) : 1; + this.maxConcurrent = Math.max(1, Math.min(5, normalized)); this.broadcastState(); // Try to start more downloads if we increased concurrency this.processQueue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/main/download-manager-engine.ts` around lines 82 - 87, The setMaxConcurrent setter currently assigns this.maxConcurrent = Math.max(1, Math.min(5, max)) which allows NaN and non-integers; change setMaxConcurrent to coerce/validate the input into a finite integer within [1,5] (e.g., check Number.isFinite(max), use Math.trunc or Math.floor to drop fractions, clamp to 1..5) before assigning and calling this.broadcastState() and this.processQueue(); this prevents NaN and fractional values from breaking the processQueue() comparison with this.activeDownloads.
🧹 Nitpick comments (8)
.github/workflows/build-after-macos-artifacts.yml (1)
34-35: Using--no-frozen-lockfilereduces build reproducibility.This flag allows pnpm to update the lockfile during install, which can lead to inconsistent builds between CI runs. Consider using
--frozen-lockfileto ensure reproducible builds, or document why this flag is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-after-macos-artifacts.yml around lines 34 - 35, The workflow step named "Install dependencies" currently runs "pnpm install --no-frozen-lockfile", which permits lockfile updates and harms reproducibility; change that command to use "pnpm install --frozen-lockfile" (or remove the flag so the default frozen behavior is enforced) in the Install dependencies step, or if lockfile updates are required, add a clear comment in the workflow explaining why "--no-frozen-lockfile" is necessary and document the risk to reproducible CI builds.package.json (1)
138-138: Consider pinningyoutubei.jsto an exact version instead of using a caret range.Using
^17.0.1allows automatic updates to minor/patch versions, which could introduce breaking changes in an unofficial YouTube API library. The previous version16.0.1was pinned exactly. Since youtubei.js interfaces with YouTube's internal API, even minor updates can break functionality.♻️ Proposed fix to pin the version
- "youtubei.js": "^17.0.1", + "youtubei.js": "17.0.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 138, The dependency entry "youtubei.js": "^17.0.1" in package.json is using a caret range which allows minor/patch upgrades; change it to an exact version string "17.0.1" (remove the caret) in package.json to pin the dependency, then update the lockfile by running your package manager (npm install or yarn install) so the lockfile (package-lock.json / yarn.lock) reflects the pinned version; locate the dependency string in package.json to make this change..github/workflows/publish-installers.yml (1)
37-38: Inconsistent action version pinning.This workflow uses
actions/checkout@v4while other workflows in this PR use pinned SHAs (e.g.,actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd). For security consistency, use the same pinning strategy across all workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-installers.yml around lines 37 - 38, The checkout action in the publish-installers workflow is using a floating tag ("uses: actions/checkout@v4") whereas other workflows in this PR use a pinned commit SHA; update the "uses: actions/checkout@v4" entry to the same pinned SHA used elsewhere (for example replace with "uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd" or the repository-wide approved SHA) so the workflow matches the pinning strategy and improves supply-chain security..github/workflows/build-after-pear-desktop-artifacts.yml (1)
100-104: Thechmod +xworkaround on app-builder suggests a packaging issue.This workaround indicates the
app-builderbinary isn't being installed with executable permissions. This could be a symptom of the electron-builder v24 downgrade or a pnpm/npm extraction issue. Consider investigating the root cause rather than working around it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-after-pear-desktop-artifacts.yml around lines 100 - 104, The CI step uses a chmod +x workaround for the app-builder binary which hides the real problem; remove the find node_modules -name "app-builder" -exec chmod +x {} + || true hack and instead investigate and fix why app-builder is not installed as executable: check the electron-builder version rollback (electron-builder v24) and restore the supported version or test with the expected version, verify pnpm extraction/preserve-permissions settings and .npmrc (including tarballMode/unsafe-perm), and if needed add a proper postinstall script in package.json (or fix the packaging/publish tarball) so the app-builder file is published with the executable bit preserved before running pnpm build and pnpm electron-builder --linux deb:x64 -p never.src/plugins/adblocker/types/index.ts (1)
1-5: Separate persisted blocker IDs from display labels.These values are also the stored
AdblockerConfig.blockervalues insrc/plugins/adblocker/index.ts, so renaming or localizing a label becomes a breaking config change. Keep stable internal ids here and map them to user-facing labels in the menu layer instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/adblocker/types/index.ts` around lines 1 - 5, The exported constant blockers currently mixes persisted IDs with display labels; change blockers to export stable machine IDs (e.g., WithBlocklists: 'with_blocklists', InPlayer: 'in_player', AdSpeedup: 'ad_speedup') and leave display strings out of this constant, then update the menu/UI layer to map those IDs to user-facing labels; update any code that reads AdblockerConfig.blocker (reference symbol: AdblockerConfig.blocker) to compare against the new stable IDs and implement a one-time migration or compatibility check if necessary so existing stored configs still resolve to the new IDs.src/plugins/downloader/main/index.ts (2)
254-255: Useconstinstead ofletfor non-reassigned variables.ESLint correctly flags that
idis never reassigned in both locations. Useconstfor clarity.🧹 Proposed fix
Line 254-255:
- let id: string | null; - id = getVideoId(url); + const id = getVideoId(url);Line 952:
- let id: string | null = getVideoId(item.url); + const id = getVideoId(item.url);Also applies to: 952-952
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/main/index.ts` around lines 254 - 255, The variable declaration "let id" that is assigned from getVideoId(url) is never reassigned; change it to "const id = getVideoId(url)" (and the analogous occurrence around the other usage of id assigned from getVideoId) to satisfy ESLint and improve clarity—search for the identifier "id" being set via getVideoId(...) in this file and replace the let + separate assignment with a single const declaration.
248-313: Significant code duplication betweendownloadSongViaManageranddownloadSongViaManagerFromId.These two functions share ~90% of their logic (metadata extraction, preset resolution, file extension determination, queue addition). The only difference is the URL construction. Consider extracting the common logic into a shared helper.
♻️ Refactor suggestion
async function queueSongToManager( info: TrackInfo, url: string, playlistFolder?: string, trackId?: string, ): Promise<void> { const metadata = getMetadata(info); if (metadata.album === 'N/A') metadata.album = ''; metadata.trackId = trackId; // ... shared preset/extension logic ... downloadManager.addToQueue({ url, title: metadata.title ?? 'Unknown', artist: metadata.artist ?? '', playlistFolder, trackId, isPlaylist: !!playlistFolder, downloadFolder, fileExtension: targetFileExtension, }); } // Then simplify: async function downloadSongViaManager(url: string, ...): Promise<void> { const id = getVideoId(url); const info = await yt.music.getInfo(id); await queueSongToManager(info, url, playlistFolder, trackId); } async function downloadSongViaManagerFromId(id: string, ...): Promise<void> { const info = await yt.music.getInfo(id); const url = `https://music.youtube.com/watch?v=${id}`; await queueSongToManager(info, url, playlistFolder, trackId); }Also applies to: 876-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/main/index.ts` around lines 248 - 313, Extract the duplicated logic in downloadSongViaManager and downloadSongViaManagerFromId into a single helper (e.g., queueSongToManager(info: TrackInfo, url: string, playlistFolder?: string, trackId?: string)): move metadata normalization (getMetadata, album === 'N/A', setting metadata.trackId), preset resolution (config.selectedPreset, config.customPresetSetting, DefaultPresetList), defaultDownloadOptions/isPremium logic, file extension selection (VideoFormatList lookup or preset.extension) and downloadManager.addToQueue into that helper; then change downloadSongViaManager to only resolve id, fetch info with yt.music.getInfo(id) and call queueSongToManager(info, url, ...), and change downloadSongViaManagerFromId to fetch info and call queueSongToManager(info, `https://music.youtube.com/watch?v=${id}`, ...). Ensure you reuse the same symbols (getMetadata, DefaultPresetList, VideoFormatList, isPremium, downloadManager.addToQueue) and preserve all existing validations and error handling.src/plugins/downloader/templates/download-manager.css (1)
73-73: Minor: Remove quotes around font family name per stylelint.Stylelint flags the quotes around
"Roboto". While technically valid, removing the quotes aligns with the linter configuration.🧹 Proposed fix
- font-family: 'Google Sans', 'Roboto', sans-serif; + font-family: 'Google Sans', Roboto, sans-serif;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/templates/download-manager.css` at line 73, In the download-manager.css font-family declaration (the line containing font-family: 'Google Sans', 'Roboto', sans-serif;), remove the unnecessary quotes around the Roboto family name to satisfy stylelint (and optionally remove quotes around Google Sans as well); update the font-family value to use unquoted identifiers so it becomes font-family: Google Sans, Roboto, sans-serif; ensuring spacing and commas remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-after-macos-artifacts.yml:
- Around line 37-40: The workflow runs pnpm dist:mac then pnpm dist:mac:arm64
but both scripts invoke pnpm clean which removes the pack/ directory so the
first (x64) artifacts get deleted; instead invoke electron-builder once to
produce both architectures in one run (build both x64 and arm64 together) rather
than calling pnpm dist:mac and pnpm dist:mac:arm64 sequentially, or adjust the
scripts to skip the clean step so pack/ is preserved; update the job to call
electron-builder with both mac architectures (replacing the two pnpm lines) to
ensure both artifacts are produced and retained.
In @.github/workflows/build-after-pear-desktop-artifacts.yml:
- Around line 57-62: The workflow runs pnpm dist:linux, pnpm
dist:linux:deb-arm64, and pnpm dist:linux:rpm-arm64 sequentially but each script
runs pnpm clean and removes pack/, so earlier artifacts get deleted; change the
job to run the clean+build once (e.g., run pnpm dist:linux which does the
initial clean/build) and then invoke electron-builder multiple times (or run the
npm scripts with a flag to skip cleaning) to produce .deb and .rpm artifacts
without intermediate cleans, ensuring pack/ is preserved between invocations;
update the Build on Linux step to call the electron-builder targets (or the dist
scripts with a --no-clean option) for deb-arm64 and rpm-arm64 after the initial
build.
In @.github/workflows/build.yml:
- Around line 71-75: The "Build specifically for Linux Mint (deb only)" step
contains a redundant pnpm build call since the Vite Build step already runs pnpm
build for pull requests. Remove the pnpm build line from this step and keep only
the pnpm electron-builder command to eliminate the duplicate build and reduce
build time for Linux Mint PRs on ubuntu-latest.
In @.github/workflows/publish-installers.yml:
- Around line 51-281: The workflow file is truncated inside the heredoc created
by cat > .github/scripts/download_and_publish_verbose.sh <<'EOF' (the script
never gets its closing EOF) and the following "Run download and publish
(verbose)" step ends mid-env at "${{ env.INSTALL"; fix by restoring the missing
tail: close the heredoc with a standalone EOF, complete the env block for the
Run download and publish (verbose) step (ensure INSTALLER_EXT_REGEX and other
env entries are fully specified), and re-add any remaining steps/outputs after
the run step so the YAML parses; locate the heredoc start and the "Run download
and publish (verbose)" step to insert the missing EOF and finish the truncated
env/step content.
- Around line 22-24: The workflow currently hardcodes env variables OWNER and
REPO which breaks for forks; update the env entries that define OWNER and REPO
to use GitHub context expressions instead (replace the hardcoded "Rocket-Space"
and "pear-desktop" values referenced by the OWNER and REPO env keys with
expressions like github.repository_owner for OWNER and
github.event.repository.name for REPO) so the workflow works for forks and moved
repositories; keep the same env keys (OWNER, REPO) so downstream steps
referencing them remain unchanged.
In `@src/plugins/adblocker/adSpeedup.ts`:
- Around line 10-19: The speedUpAndMute function currently forces playbackRate
to 16/muted true during ads and resets to 1/unmuted after, discarding the user's
original settings; change it to save the video's current playbackRate and muted
state before altering (e.g., using a WeakMap keyed by the video element or
dataset on the player element) and on ad end restore those saved values instead
of hard-coding 1 and false; update speedUpAndMute to check for an existing saved
state when entering an ad (store if not present) and to restore and remove that
saved state when leaving the ad so the user's original speed/mute are preserved.
In `@src/plugins/adblocker/blocker.ts`:
- Around line 24-31: The module currently stores a single singleton "blocker"
which is overwritten by loadAdBlockerEngine(), causing orphaned blockers per
session; change this to track blockers per session using a Map keyed by the
session (or session id/partition) instead of the single "blocker" variable.
Update loadAdBlockerEngine(session, ...) to look up/create the ElectronBlocker
in the Map (e.g., blockersBySession.set(sessionKey, blocker)), update
unloadAdBlockerEngine(session) and isBlockerEnabled(session) to operate on the
Map entry for that session, and ensure you remove and properly dispose the
blocker from the Map when unloading so no orphaned instances remain. Use the
existing function names loadAdBlockerEngine, unloadAdBlockerEngine,
isBlockerEnabled and the "blocker" identifier as references to replace with
session-scoped storage.
In `@src/plugins/adblocker/index.ts`:
- Around line 103-117: onConfigChange currently only loads the blocklist engine
when switching to WithBlocklists but never unloads or refreshes it; update
onConfigChange to (1) detect transitions away from blockers.WithBlocklists and
call an unload function (e.g., unloadAdBlockerEngine or similar) with
this.mainWindow.webContents.session before returning so rules are removed when
switching to InPlayer/AdSpeedup, and (2) when newConfig.blocker ===
blockers.WithBlocklists and the session is already enabled, compare
newConfig.cache/newConfig.additionalBlockLists/newConfig.disableDefaultLists to
the current applied values and call loadAdBlockerEngine(...) again to
rebuild/refresh rules with the new params; ensure you await unload/load calls
and guard on this.mainWindow and its webContents.session, using the existing
isBlockerEnabled, loadAdBlockerEngine, and the new unload function names to
locate the code.
In `@src/plugins/adblocker/injectors/inject.js`:
- Around line 22-39: The pruner function can throw on non-object JSON results;
update pruner (and any _pruner alias) to first return the input unchanged when
it's null or not an object (e.g., if o == null || typeof o !== 'object' or
Array.isArray(o) depending on desired behavior) and only attempt delete
operations when o and nested fields (o.playerResponse,
o.ytInitialPlayerResponse) are plain objects; in short, guard the top-level o
and each nested object with an object-type check before deleting properties so
arrays/primitives/booleans/numbers/strings are returned intact.
- Around line 177-190: The current accessor wrappers call previousGetter() /
previousSetter(a) without a receiver which breaks original this-binding; update
the wrapper in the Object.defineProperty call so both the original accessors and
the new handler accessors are invoked with the original receiver (use
.call(this, ...) or equivalent) — i.e., call previousGetter with this and
previousSetter with this and the argument, and also invoke handler.getter and
handler.setter with this to preserve correct semantics for owner/prop accessors.
In `@src/plugins/downloader/main/download-manager-engine.ts`:
- Around line 96-123: Persist the resolved target path on the DownloadItem and
deduplicate against queued items instead of only checking the filesystem: in
addToQueue(...) compute filePath as you already do, then set that value on the
new DownloadItem (e.g., DownloadItem.destination or similar) when constructing
the item, and before pushing to the queue check the in-memory queue for an
existing item with the same destination (and/or trackId) to avoid duplicate
enqueues; also apply the same change to the alternate enqueue path used around
the 150-166 region so both code paths record destination and perform the same
in-queue dedupe instead of relying solely on existsSync().
In `@src/plugins/downloader/main/index.ts`:
- Around line 225-229: The handler for 'download-manager-set-concurrent'
currently mutates the in-memory config (config.maxConcurrentDownloads) but
doesn't persist it; update the onMainLoad function signature to destructure
setConfig from the BackendContext, then after calling
downloadManager.setMaxConcurrent(max) call setConfig({ ...config,
maxConcurrentDownloads: max }) (or setConfig with the specific key) instead of
only assigning config.maxConcurrentDownloads so the value persists across
restarts; reference the ipc.handle('download-manager-set-concurrent') handler,
downloadManager.setMaxConcurrent, config.maxConcurrentDownloads, onMainLoad,
BackendContext, and setConfig when applying the change.
In `@src/plugins/downloader/templates/download-manager.tsx`:
- Around line 219-377: Replace all hardcoded Spanish UI strings in the Download
Manager JSX with i18n lookups using the translation keys; specifically update
the title text in the toggle button and header (currently "Gestor de Descargas")
to use download-manager.title, the concurrent label "Simultáneas:" to
download-manager.concurrent-downloads, the pause/resume labels to
download-manager.pause and download-manager.resume, the retry/clear button
labels to download-manager.retry-failed and download-manager.clear-completed,
the three tab labels to download-manager.tabs.* (queue/failed/completed), the
empty state "No hay descargas" to download-manager.no-items, and the "activas"
stat label to download-manager.active; locate these strings inside the Download
Manager component return JSX (elements around the toggle button, dm-header,
dm-controls, dm-tabs, dm-empty and dm-stats) and replace them with calls to the
app’s i18n function (e.g., t(...)) so the UI uses the translation keys instead
of hardcoded text.
- Around line 200-215: The getSubtitle function currently returns hardcoded
Spanish strings; import and use the i18n translator (t) and replace each case in
getSubtitle (function getSubtitle, switch on item.status) to return
t('plugins.downloader.download-manager.status.queued'),
t('plugins.downloader.download-manager.status.downloading', { progress:
item.progress, provider: item.currentProvider, attempt: item.currentAttempt }),
t('plugins.downloader.download-manager.status.completed'),
t('plugins.downloader.download-manager.status.failed', { error: item.error ??
t('plugins.downloader.download-manager.status.unknownError') }), and
t('plugins.downloader.download-manager.status.skipped') respectively so messages
and interpolations come from en.json/es.json instead of hardcoded Spanish.
---
Minor comments:
In @.github/workflows/build-macos.yml:
- Around line 25-31: The Xcode glob pattern /Applications/Xcode_26*.app is
incorrect (likely a typo) and will never match current installs; update the
first search to a sensible pattern like /Applications/Xcode_*.app (remove the
"26") and use version-aware sorting (e.g., sort -V) when choosing XCODE_APP so
the newest installed Xcode is selected; ensure the fallback that checks
/Applications/Xcode*.app remains in place and that XCODE_APP is exported/used
when calling xcode-select and xcodebuild.
In @.github/workflows/build.yml:
- Line 46: The workflow has inconsistent pnpm install flags between the build
and release jobs: change one so both use the same lockfile behaviour; either
make the build job use --frozen-lockfile like the release job or make the
release job use --no-frozen-lockfile so dependency resolution is consistent.
Update the run commands for the build job (currently "pnpm install
--no-frozen-lockfile") and the release job (currently "pnpm install
--frozen-lockfile") to use the same flag across both jobs.
In @.github/workflows/publish-installers.yml:
- Line 265: Replace the deprecated ::set-output usage with writing the output
key/value to the GitHub Actions environment file: instead of the line that uses
::set-output for release_tag, append a line with the literal key "release_tag"
and the value from ${RELEASE_TAG} to the $GITHUB_OUTPUT file (ensure proper
quoting/expansion and use append mode so other outputs are preserved); update
the step that emitted release_tag to use this method and remove the old
::set-output syntax.
In `@package.json`:
- Around line 159-160: Update the PR with a concise justification for
downgrading "electron-builder" and "electron-builder-squirrel-windows" from
v26.7.0 to v24.13.3: describe the exact build/compatibility failure observed
(e.g., failing build step, broken plugin, CI error), name the affected command
or script (npm/yarn script that invokes electron-builder), and note any attempts
to resolve it (error logs, repro steps, or patches tried). Also state whether
restoring v26 is feasible (blocking bug, missing plugin support, or workaround)
and, if possible, include a follow-up action (e.g., open issue/upgrade plan) so
reviewers know if this is temporary or permanent.
In `@src/i18n/resources/en.json`:
- Around line 588-591: The i18n string at key "status.downloading" currently
hardcodes "/3" which will diverge from the backend constant
MAX_RETRIES_PER_PROVIDER; update the English resource value to use an
interpolation like "{{maxAttempts}}" instead of "3" and then ensure wherever the
translation for "status.downloading" is invoked (the code that supplies
progress, provider and attempt) also passes maxAttempts:
MAX_RETRIES_PER_PROVIDER so the rendered string stays consistent with the
backend constant.
In `@src/i18n/resources/es.json`:
- Around line 588-591: The Spanish "status.downloading" string hardcodes "/3";
update it to use the same interpolation variable used in the English version
(e.g., change "Descargando… {{progress}}% — {{provider}} ({{attempt}}/3)" to use
a max-attempt variable like "Descargando… {{progress}}% — {{provider}}
({{attempt}}/{{maxAttempts}})") and ensure the interpolation key name exactly
matches the English resource so runtime interpolation stays in sync.
In `@src/plugins/downloader/main/download-manager-engine.ts`:
- Around line 237-240: The clearCompleted() method currently empties
completedItems but leaves totalCompleted and totalSkipped stale; update
clearCompleted() to also reset those counters (set totalCompleted and
totalSkipped to 0 or recompute them consistently) before calling
broadcastState() so the broadcasted state reflects the cleared list; reference
clearCompleted(), completedItems, totalCompleted, totalSkipped, and
broadcastState() when making the change.
- Around line 82-87: The setMaxConcurrent setter currently assigns
this.maxConcurrent = Math.max(1, Math.min(5, max)) which allows NaN and
non-integers; change setMaxConcurrent to coerce/validate the input into a finite
integer within [1,5] (e.g., check Number.isFinite(max), use Math.trunc or
Math.floor to drop fractions, clamp to 1..5) before assigning and calling
this.broadcastState() and this.processQueue(); this prevents NaN and fractional
values from breaking the processQueue() comparison with this.activeDownloads.
In `@src/plugins/downloader/main/index.ts`:
- Around line 605-607: Replace the hardcoded Spanish error string thrown when
all providers fail with an i18n lookup: instead of `throw new Error('Todos los
proveedores fallaron...')` use the project's i18n helper (e.g., i18n.t or
translate) to build the message and interpolate lastError (use `lastError
instanceof Error ? lastError.message : String(lastError)`), updating the throw
in the block that checks `if (!success)` in the downloader module (index.ts) and
add the corresponding translation key/value to the locale files so the message
is available in supported languages.
In `@src/plugins/downloader/menu.ts`:
- Around line 217-228: Remove the hardcoded Spanish fallback for the menu label
by eliminating the "|| 'Descargas simultáneas'" or replacing it with an English
fallback ("Concurrent downloads"); update the label expression that uses
t('plugins.downloader.menu.concurrent-downloads') in the submenu block so it
relies on the i18n key (or an English fallback) instead of the Spanish
string—this change affects the menu item where config.maxConcurrentDownloads is
used and the click handler calls setConfig({ maxConcurrentDownloads: n }).
---
Nitpick comments:
In @.github/workflows/build-after-macos-artifacts.yml:
- Around line 34-35: The workflow step named "Install dependencies" currently
runs "pnpm install --no-frozen-lockfile", which permits lockfile updates and
harms reproducibility; change that command to use "pnpm install
--frozen-lockfile" (or remove the flag so the default frozen behavior is
enforced) in the Install dependencies step, or if lockfile updates are required,
add a clear comment in the workflow explaining why "--no-frozen-lockfile" is
necessary and document the risk to reproducible CI builds.
In @.github/workflows/build-after-pear-desktop-artifacts.yml:
- Around line 100-104: The CI step uses a chmod +x workaround for the
app-builder binary which hides the real problem; remove the find node_modules
-name "app-builder" -exec chmod +x {} + || true hack and instead investigate and
fix why app-builder is not installed as executable: check the electron-builder
version rollback (electron-builder v24) and restore the supported version or
test with the expected version, verify pnpm extraction/preserve-permissions
settings and .npmrc (including tarballMode/unsafe-perm), and if needed add a
proper postinstall script in package.json (or fix the packaging/publish tarball)
so the app-builder file is published with the executable bit preserved before
running pnpm build and pnpm electron-builder --linux deb:x64 -p never.
In @.github/workflows/publish-installers.yml:
- Around line 37-38: The checkout action in the publish-installers workflow is
using a floating tag ("uses: actions/checkout@v4") whereas other workflows in
this PR use a pinned commit SHA; update the "uses: actions/checkout@v4" entry to
the same pinned SHA used elsewhere (for example replace with "uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd" or the
repository-wide approved SHA) so the workflow matches the pinning strategy and
improves supply-chain security.
In `@package.json`:
- Line 138: The dependency entry "youtubei.js": "^17.0.1" in package.json is
using a caret range which allows minor/patch upgrades; change it to an exact
version string "17.0.1" (remove the caret) in package.json to pin the
dependency, then update the lockfile by running your package manager (npm
install or yarn install) so the lockfile (package-lock.json / yarn.lock)
reflects the pinned version; locate the dependency string in package.json to
make this change.
In `@src/plugins/adblocker/types/index.ts`:
- Around line 1-5: The exported constant blockers currently mixes persisted IDs
with display labels; change blockers to export stable machine IDs (e.g.,
WithBlocklists: 'with_blocklists', InPlayer: 'in_player', AdSpeedup:
'ad_speedup') and leave display strings out of this constant, then update the
menu/UI layer to map those IDs to user-facing labels; update any code that reads
AdblockerConfig.blocker (reference symbol: AdblockerConfig.blocker) to compare
against the new stable IDs and implement a one-time migration or compatibility
check if necessary so existing stored configs still resolve to the new IDs.
In `@src/plugins/downloader/main/index.ts`:
- Around line 254-255: The variable declaration "let id" that is assigned from
getVideoId(url) is never reassigned; change it to "const id = getVideoId(url)"
(and the analogous occurrence around the other usage of id assigned from
getVideoId) to satisfy ESLint and improve clarity—search for the identifier "id"
being set via getVideoId(...) in this file and replace the let + separate
assignment with a single const declaration.
- Around line 248-313: Extract the duplicated logic in downloadSongViaManager
and downloadSongViaManagerFromId into a single helper (e.g.,
queueSongToManager(info: TrackInfo, url: string, playlistFolder?: string,
trackId?: string)): move metadata normalization (getMetadata, album === 'N/A',
setting metadata.trackId), preset resolution (config.selectedPreset,
config.customPresetSetting, DefaultPresetList), defaultDownloadOptions/isPremium
logic, file extension selection (VideoFormatList lookup or preset.extension) and
downloadManager.addToQueue into that helper; then change downloadSongViaManager
to only resolve id, fetch info with yt.music.getInfo(id) and call
queueSongToManager(info, url, ...), and change downloadSongViaManagerFromId to
fetch info and call queueSongToManager(info,
`https://music.youtube.com/watch?v=${id}`, ...). Ensure you reuse the same
symbols (getMetadata, DefaultPresetList, VideoFormatList, isPremium,
downloadManager.addToQueue) and preserve all existing validations and error
handling.
In `@src/plugins/downloader/templates/download-manager.css`:
- Line 73: In the download-manager.css font-family declaration (the line
containing font-family: 'Google Sans', 'Roboto', sans-serif;), remove the
unnecessary quotes around the Roboto family name to satisfy stylelint (and
optionally remove quotes around Google Sans as well); update the font-family
value to use unquoted identifiers so it becomes font-family: Google Sans,
Roboto, sans-serif; ensuring spacing and commas remain correct.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd731bc5-6181-4713-8ef4-fa55a9843681
⛔ Files ignored due to path filters (3)
assets/icon_linux.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
.github/workflows/build-after-macos-artifacts.yml.github/workflows/build-after-pear-desktop-artifacts.yml.github/workflows/build-macos.yml.github/workflows/build.yml.github/workflows/pr-build-artifacts.yml.github/workflows/publish-installers.ymlelectron-builder.ymlpackage.jsonsrc/i18n/resources/en.jsonsrc/i18n/resources/es.jsonsrc/plugins/adblocker/.gitignoresrc/plugins/adblocker/adSpeedup.tssrc/plugins/adblocker/blocker.tssrc/plugins/adblocker/index.tssrc/plugins/adblocker/injectors/inject-cliqz-preload.tssrc/plugins/adblocker/injectors/inject.d.tssrc/plugins/adblocker/injectors/inject.jssrc/plugins/adblocker/types/index.tssrc/plugins/downloader/index.tssrc/plugins/downloader/main/download-manager-engine.tssrc/plugins/downloader/main/index.tssrc/plugins/downloader/menu.tssrc/plugins/downloader/renderer.tsxsrc/plugins/downloader/templates/download-manager.csssrc/plugins/downloader/templates/download-manager.tsx
💤 Files with no reviewable changes (1)
- .github/workflows/pr-build-artifacts.yml
Comment on lines
+37
to
+40
| - name: Build on macOS | ||
| run: | | ||
| pnpm dist:mac | ||
| pnpm dist:mac:arm64 |
There was a problem hiding this comment.
Critical: Sequential dist commands will overwrite each other.
Both pnpm dist:mac and pnpm dist:mac:arm64 include pnpm clean which deletes the pack/ directory. Running them sequentially means the x64 build artifacts will be deleted before the arm64 build starts, resulting in only arm64 artifacts being uploaded.
🐛 Proposed fix: Run electron-builder directly without clean
- name: Build on macOS
run: |
- pnpm dist:mac
- pnpm dist:mac:arm64
+ pnpm clean
+ pnpm build
+ pnpm electron-builder --mac dmg:x64 dmg:arm64 -p neverAlternatively, build both architectures in a single electron-builder invocation to avoid the clean/rebuild cycle.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Build on macOS | |
| run: | | |
| pnpm dist:mac | |
| pnpm dist:mac:arm64 | |
| - name: Build on macOS | |
| run: | | |
| pnpm clean | |
| pnpm build | |
| pnpm electron-builder --mac dmg:x64 dmg:arm64 -p never |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-after-macos-artifacts.yml around lines 37 - 40, The
workflow runs pnpm dist:mac then pnpm dist:mac:arm64 but both scripts invoke
pnpm clean which removes the pack/ directory so the first (x64) artifacts get
deleted; instead invoke electron-builder once to produce both architectures in
one run (build both x64 and arm64 together) rather than calling pnpm dist:mac
and pnpm dist:mac:arm64 sequentially, or adjust the scripts to skip the clean
step so pack/ is preserved; update the job to call electron-builder with both
mac architectures (replacing the two pnpm lines) to ensure both artifacts are
produced and retained.
Comment on lines
+57
to
+62
| - name: Build on Linux | ||
| if: matrix.os == 'ubuntu-latest' | ||
| run: | | ||
| pnpm dist:linux | ||
| pnpm dist:linux:deb-arm64 | ||
| pnpm dist:linux:rpm-arm64 |
There was a problem hiding this comment.
Critical: Sequential dist commands will overwrite each other.
Similar to the macOS workflow, running pnpm dist:linux, pnpm dist:linux:deb-arm64, and pnpm dist:linux:rpm-arm64 sequentially is problematic because each script starts with pnpm clean which deletes the pack/ directory. Only the last build's artifacts will survive.
🐛 Proposed fix: Run builds without intermediate clean
- name: Build on Linux
if: matrix.os == 'ubuntu-latest'
run: |
- pnpm dist:linux
- pnpm dist:linux:deb-arm64
- pnpm dist:linux:rpm-arm64
+ pnpm clean
+ pnpm build
+ pnpm electron-builder --linux -p never
+ pnpm electron-builder --linux deb:arm64 -p never
+ pnpm electron-builder --linux rpm:arm64 -p neverThis runs clean and build once, then runs electron-builder multiple times to produce all artifacts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Build on Linux | |
| if: matrix.os == 'ubuntu-latest' | |
| run: | | |
| pnpm dist:linux | |
| pnpm dist:linux:deb-arm64 | |
| pnpm dist:linux:rpm-arm64 | |
| - name: Build on Linux | |
| if: matrix.os == 'ubuntu-latest' | |
| run: | | |
| pnpm clean | |
| pnpm build | |
| pnpm electron-builder --linux -p never | |
| pnpm electron-builder --linux deb:arm64 -p never | |
| pnpm electron-builder --linux rpm:arm64 -p never |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-after-pear-desktop-artifacts.yml around lines 57 -
62, The workflow runs pnpm dist:linux, pnpm dist:linux:deb-arm64, and pnpm
dist:linux:rpm-arm64 sequentially but each script runs pnpm clean and removes
pack/, so earlier artifacts get deleted; change the job to run the clean+build
once (e.g., run pnpm dist:linux which does the initial clean/build) and then
invoke electron-builder multiple times (or run the npm scripts with a flag to
skip cleaning) to produce .deb and .rpm artifacts without intermediate cleans,
ensuring pack/ is preserved between invocations; update the Build on Linux step
to call the electron-builder targets (or the dist scripts with a --no-clean
option) for deb-arm64 and rpm-arm64 after the initial build.
Comment on lines
+71
to
+75
| - name: Build specifically for Linux Mint (deb only) | ||
| if: matrix.os == 'ubuntu-latest' && github.event_name == 'pull_request' | ||
| run: | | ||
| pnpm build | ||
| pnpm electron-builder --linux deb:x64 -p never |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant pnpm build call in Linux Mint step.
The "Vite Build" step at line 52 already runs pnpm build for PRs. Running it again here is wasteful and doubles the build time for Linux Mint PRs on ubuntu-latest.
♻️ Proposed fix to remove redundant build
- name: Build specifically for Linux Mint (deb only)
if: matrix.os == 'ubuntu-latest' && github.event_name == 'pull_request'
run: |
- pnpm build
pnpm electron-builder --linux deb:x64 -p never🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yml around lines 71 - 75, The "Build specifically
for Linux Mint (deb only)" step contains a redundant pnpm build call since the
Vite Build step already runs pnpm build for pull requests. Remove the pnpm build
line from this step and keep only the pnpm electron-builder command to eliminate
the duplicate build and reduce build time for Linux Mint PRs on ubuntu-latest.
Comment on lines
+22
to
+24
| env: | ||
| OWNER: Rocket-Space | ||
| REPO: pear-desktop |
There was a problem hiding this comment.
Hardcoded OWNER and REPO will break for forks.
Using hardcoded values Rocket-Space and pear-desktop means this workflow won't work correctly when run from forks or if the repository is transferred. Use GitHub context variables instead.
🐛 Proposed fix using GitHub context
env:
- OWNER: Rocket-Space
- REPO: pear-desktop
+ OWNER: ${{ github.repository_owner }}
+ REPO: ${{ github.event.repository.name }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish-installers.yml around lines 22 - 24, The workflow
currently hardcodes env variables OWNER and REPO which breaks for forks; update
the env entries that define OWNER and REPO to use GitHub context expressions
instead (replace the hardcoded "Rocket-Space" and "pear-desktop" values
referenced by the OWNER and REPO env keys with expressions like
github.repository_owner for OWNER and github.event.repository.name for REPO) so
the workflow works for forks and moved repositories; keep the same env keys
(OWNER, REPO) so downstream steps referencing them remain unchanged.
Comment on lines
+51
to
+281
| - name: Save download and publish script (fixed heredoc) | ||
| run: | | ||
| mkdir -p .github/scripts | ||
| cat > .github/scripts/download_and_publish_verbose.sh <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| set -x | ||
| IFS=$'\n\t' | ||
|
|
||
| OWNER="${OWNER}" | ||
| REPO="${REPO}" | ||
| GHTOKEN="${GHTOKEN}" | ||
| WORKFLOW_A="${WORKFLOW_A}" | ||
| WORKFLOW_B="${WORKFLOW_B}" | ||
| INSTALLER_EXT_REGEX="${INSTALLER_EXT_REGEX}" | ||
| WAIT_TIMEOUT="${WAIT_TIMEOUT}" | ||
| POLL_INTERVAL="${POLL_INTERVAL}" | ||
| CURL_RETRIES="${CURL_RETRIES}" | ||
| GH_UPLOAD_RETRIES="${GH_UPLOAD_RETRIES}" | ||
| TAG_PREFIX="${TAG_PREFIX}" | ||
| COMPATIBILITY="${COMPATIBILITY}" | ||
| DIAG_MODE="${DIAG_MODE}" | ||
|
|
||
| API_BASE="https://api.github.com" | ||
|
|
||
| log() { echo ">> $*"; } | ||
| api() { | ||
| curl -sS -H "Authorization: token ${GHTOKEN}" -H "Accept: application/vnd.github+json" "${API_BASE}$1" | ||
| } | ||
|
|
||
| resolve_workflow_id() { | ||
| local wf_name="$1" | ||
| log "Resolving workflow id for: ${wf_name}" | ||
| local list | ||
| list=$(api "/repos/${OWNER}/${REPO}/actions/workflows?per_page=100") | ||
| if [ "${DIAG_MODE}" = "true" ]; then | ||
| echo "WORKFLOWS_JSON_COUNT: $(echo "$list" | jq -r '.workflows | length')" | ||
| fi | ||
| id=$(echo "$list" | jq -r --arg name "$wf_name" '.workflows[] | select(.name == $name) | .id' | head -n1 || true) | ||
| if [ -z "$id" ]; then | ||
| id=$(echo "$list" | jq -r --arg name "$wf_name" '.workflows[] | select(.path | contains($name)) | .id' | head -n1 || true) | ||
| fi | ||
| echo "${id:-}" | ||
| } | ||
|
|
||
| get_latest_successful_run_by_workflow_id() { | ||
| local wf_id="$1" | ||
| api "/repos/${OWNER}/${REPO}/actions/workflows/${wf_id}/runs?per_page=50" | jq -c '.workflow_runs[] | select(.conclusion=="success")' | head -n1 || true | ||
| } | ||
|
|
||
| download_artifacts_for_run() { | ||
| local run_id="$1" | ||
| local outdir="$2" | ||
| mkdir -p "${outdir}" | ||
| local artifacts_json | ||
| artifacts_json=$(api "/repos/${OWNER}/${REPO}/actions/runs/${run_id}/artifacts") | ||
| echo "${artifacts_json}" > "${outdir}/artifacts.json" | ||
| log "Artifacts JSON saved to ${outdir}/artifacts.json" | ||
| echo "${artifacts_json}" | jq -r '.artifacts[] | @base64' | while read -r a; do | ||
| _jq() { echo "${a}" | base64 --decode | jq -r "${1}"; } | ||
| name=$(_jq '.name') | ||
| url=$(_jq '.archive_download_url') | ||
| out="${outdir}/${name}.zip" | ||
| log "Downloading artifact ${name} from run ${run_id} -> ${out}" | ||
| n=0 | ||
| until [ $n -ge "${CURL_RETRIES}" ]; do | ||
| if curl -sSL -H "Authorization: token ${GHTOKEN}" -H "Accept: application/vnd.github+json" -L "${url}" -o "${out}"; then | ||
| log "Downloaded ${name}" | ||
| break | ||
| else | ||
| n=$((n+1)) | ||
| log "Retry ${n}/${CURL_RETRIES} for ${name}" | ||
| sleep 2 | ||
| fi | ||
| done | ||
| if [ ! -f "${out}" ]; then | ||
| log "ERROR: Failed to download artifact ${name}" | ||
| return 1 | ||
| fi | ||
| done | ||
| } | ||
|
|
||
| extract_installers() { | ||
| local srcdir="$1" | ||
| local destdir="$2" | ||
| mkdir -p "${destdir}" | ||
| shopt -s nullglob | ||
| for z in "${srcdir}"/*.zip; do | ||
| [ -f "$z" ] || continue | ||
| tmpdir=$(mktemp -d) | ||
| unzip -o "$z" -d "$tmpdir" >/dev/null | ||
| find "$tmpdir" -type f | while read -r f; do | ||
| if echo "$f" | grep -E "${INSTALLER_EXT_REGEX}" >/dev/null; then | ||
| base=$(basename "$f") | ||
| cp -f "$f" "${destdir}/${base}" | ||
| log "Collected installer: ${base}" | ||
| fi | ||
| done | ||
| rm -rf "$tmpdir" | ||
| done | ||
| shopt -u nullglob | ||
| } | ||
|
|
||
| start_ts=$(date +%s) | ||
| deadline=$((start_ts + WAIT_TIMEOUT)) | ||
| matched_run_a="" | ||
| matched_run_b="" | ||
| matched_sha="" | ||
|
|
||
| wf_a_id=$(resolve_workflow_id "${WORKFLOW_A}") | ||
| wf_b_id=$(resolve_workflow_id "${WORKFLOW_B}") | ||
| log "Resolved workflow ids: A=${wf_a_id:-none} B=${wf_b_id:-none}" | ||
| if [ -z "${wf_a_id}" ] || [ -z "${wf_b_id}" ]; then | ||
| log "ERROR: Could not resolve one or both workflow ids. Check WORKFLOW_A/WORKFLOW_B names." | ||
| exit 10 | ||
| fi | ||
|
|
||
| while [ "$(date +%s)" -le "${deadline}" ]; do | ||
| run_a_json=$(get_latest_successful_run_by_workflow_id "${wf_a_id}" || true) | ||
| run_b_json=$(get_latest_successful_run_by_workflow_id "${wf_b_id}" || true) | ||
|
|
||
| if [ -n "${run_a_json}" ] && [ -n "${run_b_json}" ]; then | ||
| sha_a=$(echo "${run_a_json}" | jq -r '.head_sha') | ||
| sha_b=$(echo "${run_b_json}" | jq -r '.head_sha') | ||
| id_a=$(echo "${run_a_json}" | jq -r '.id') | ||
| id_b=$(echo "${run_b_json}" | jq -r '.id') | ||
| log "Found runs: A(id=${id_a}, sha=${sha_a}) B(id=${id_b}, sha=${sha_b})" | ||
| if [ "${sha_a}" = "${sha_b}" ]; then | ||
| matched_run_a="${id_a}" | ||
| matched_run_b="${id_b}" | ||
| matched_sha="${sha_a}" | ||
| log "Matched runs with same commit: ${matched_sha}" | ||
| break | ||
| else | ||
| log "Runs do not match by commit. Waiting..." | ||
| fi | ||
| else | ||
| log "One or both runs not found yet. Polling..." | ||
| fi | ||
| sleep "${POLL_INTERVAL}" | ||
| done | ||
|
|
||
| if [ -z "${matched_run_a}" ] || [ -z "${matched_run_b}" ]; then | ||
| log "ERROR: Timeout reached (${WAIT_TIMEOUT}s) without finding matching successful runs for both workflows." | ||
| exit 2 | ||
| fi | ||
|
|
||
| WORKDIR=$(mktemp -d) | ||
| ART_DIR="${WORKDIR}/artifacts" | ||
| mkdir -p "${ART_DIR}/A" "${ART_DIR}/B" | ||
| INSTALLERS_DIR="${WORKDIR}/installers" | ||
| mkdir -p "${INSTALLERS_DIR}" | ||
|
|
||
| log "Downloading artifacts for run A=${matched_run_a}" | ||
| download_artifacts_for_run "${matched_run_a}" "${ART_DIR}/A" | ||
| log "Downloading artifacts for run B=${matched_run_b}" | ||
| download_artifacts_for_run "${matched_run_b}" "${ART_DIR}/B" | ||
|
|
||
| log "Listing downloaded artifact zips:" | ||
| ls -la "${ART_DIR}" || true | ||
| ls -la "${ART_DIR}/A" || true | ||
| ls -la "${ART_DIR}/B" || true | ||
|
|
||
| log "Extracting installers" | ||
| extract_installers "${ART_DIR}/A" "${INSTALLERS_DIR}" | ||
| extract_installers "${ART_DIR}/B" "${INSTALLERS_DIR}" | ||
|
|
||
| log "Installers collected:" | ||
| ls -la "${INSTALLERS_DIR}" || true | ||
|
|
||
| if [ -z "$(ls -A ${INSTALLERS_DIR} 2>/dev/null || true)" ]; then | ||
| log "ERROR: No installer files found in artifacts." | ||
| exit 3 | ||
| fi | ||
|
|
||
| TIMESTAMP=$(date +%Y%m%d%H%M%S) | ||
| RELEASE_TAG="${TAG_PREFIX}${TIMESTAMP}" | ||
| RELEASE_NAME="Automated release ${RELEASE_TAG}" | ||
| RELEASE_BODY="Automated release combining artifacts from workflows: | ||
| - ${WORKFLOW_A} (run ${matched_run_a}) | ||
| - ${WORKFLOW_B} (run ${matched_run_b}) | ||
|
|
||
| Commit: ${matched_sha} | ||
|
|
||
| Compatible systems: ${COMPATIBILITY} | ||
| " | ||
|
|
||
| log "Authenticating gh CLI" | ||
| echo "${GHTOKEN}" | gh auth login --with-token >/dev/null | ||
|
|
||
| log "Creating release ${RELEASE_TAG}" | ||
| gh release create "${RELEASE_TAG}" -t "${RELEASE_NAME}" -n "${RELEASE_BODY}" || true | ||
|
|
||
| log "Uploading installers" | ||
| for f in "${INSTALLERS_DIR}"/*; do | ||
| [ -f "$f" ] || continue | ||
| n=0 | ||
| until [ $n -ge "${GH_UPLOAD_RETRIES}" ]; do | ||
| if gh release upload "${RELEASE_TAG}" "$f" --clobber; then | ||
| log "Uploaded $(basename "$f")" | ||
| break | ||
| else | ||
| n=$((n+1)) | ||
| log "Retry ${n}/${GH_UPLOAD_RETRIES} uploading $(basename "$f")" | ||
| sleep 2 | ||
| fi | ||
| done | ||
| if [ $n -ge "${GH_UPLOAD_RETRIES}" ]; then | ||
| log "ERROR: Failed to upload $(basename "$f") after ${GH_UPLOAD_RETRIES} attempts" | ||
| exit 4 | ||
| fi | ||
| done | ||
|
|
||
| log "Release published: ${RELEASE_TAG}" | ||
| echo "::set-output name=release_tag::${RELEASE_TAG}" | ||
| EOF | ||
|
|
||
| - name: Verify script and show head | ||
| run: | | ||
| ls -la .github/scripts | ||
| echo "---- script head (first 120 lines) ----" | ||
| sed -n '1,120p' .github/scripts/download_and_publish_verbose.sh || true | ||
|
|
||
| - name: Run download and publish (verbose) | ||
| env: | ||
| GHTOKEN: ${{ secrets.RELEASE_PAT || secrets.GITHUB_TOKEN }} | ||
| OWNER: ${{ env.OWNER }} | ||
| REPO: ${{ env.REPO }} | ||
| WORKFLOW_A: ${{ env.WORKFLOW_A }} | ||
| WORKFLOW_B: ${{ env.WORKFLOW_B }} | ||
| INSTALLER_EXT_REGEX: ${{ env.INSTALL |
There was a problem hiding this comment.
Critical: File appears truncated, causing YAML syntax error.
The static analysis tools report a syntax error because the file ends mid-line at ${{ env.INSTALL. The heredoc starting at line 54 is never properly closed, and the workflow steps after the script are missing. This workflow will fail to parse.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 56-56: could not parse as YAML: could not find expected ':'
(syntax-check)
🪛 YAMLlint (1.38.0)
[error] 57-57: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish-installers.yml around lines 51 - 281, The workflow
file is truncated inside the heredoc created by cat >
.github/scripts/download_and_publish_verbose.sh <<'EOF' (the script never gets
its closing EOF) and the following "Run download and publish (verbose)" step
ends mid-env at "${{ env.INSTALL"; fix by restoring the missing tail: close the
heredoc with a standalone EOF, complete the env block for the Run download and
publish (verbose) step (ensure INSTALLER_EXT_REGEX and other env entries are
fully specified), and re-add any remaining steps/outputs after the run step so
the YAML parses; locate the heredoc start and the "Run download and publish
(verbose)" step to insert the missing EOF and finish the truncated env/step
content.
Comment on lines
+177
to
+190
| Object.defineProperty(owner, prop, { | ||
| configurable, | ||
| get() { | ||
| if (previousGetter !== undefined) { | ||
| previousGetter(); | ||
| } | ||
|
|
||
| // | ||
| return handler.getter(); | ||
| }, | ||
| set(a) { | ||
| if (previousSetter !== undefined) { | ||
| previousSetter(a); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "inject.js" -path "*adblocker*" | head -20Repository: pear-devs/pear-desktop
Length of output: 109
🏁 Script executed:
cat -n ./src/plugins/adblocker/injectors/inject.js | sed -n '170,200p'Repository: pear-devs/pear-desktop
Length of output: 911
🏁 Script executed:
cat -n ./src/plugins/adblocker/injectors/inject.js | sed -n '140,200p'Repository: pear-devs/pear-desktop
Length of output: 1897
🏁 Script executed:
cat -n ./src/plugins/adblocker/injectors/inject.js | sed -n '1,50p'Repository: pear-devs/pear-desktop
Length of output: 1719
🏁 Script executed:
cat -n ./src/plugins/adblocker/injectors/inject.js | sed -n '100,160p'Repository: pear-devs/pear-desktop
Length of output: 1694
Preserve the receiver when forwarding existing accessors.
Calling previousGetter() without a receiver loses the original this binding. If an intercepted accessor depends on its receiver, the trap will change behavior or throw.
The wrappers defined on lines 179 and 187 are themselves accessors—when invoked by property access, this refers to owner. The original accessors must be called with that same receiver to maintain correct semantics.
Proposed fix
get() {
if (previousGetter !== undefined) {
- previousGetter();
+ previousGetter.call(this);
}
//
return handler.getter();
},
set(a) {
if (previousSetter !== undefined) {
- previousSetter(a);
+ previousSetter.call(this, a);
}
//
handler.setter(a);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Object.defineProperty(owner, prop, { | |
| configurable, | |
| get() { | |
| if (previousGetter !== undefined) { | |
| previousGetter(); | |
| } | |
| // | |
| return handler.getter(); | |
| }, | |
| set(a) { | |
| if (previousSetter !== undefined) { | |
| previousSetter(a); | |
| } | |
| Object.defineProperty(owner, prop, { | |
| configurable, | |
| get() { | |
| if (previousGetter !== undefined) { | |
| previousGetter.call(this); | |
| } | |
| // | |
| return handler.getter(); | |
| }, | |
| set(a) { | |
| if (previousSetter !== undefined) { | |
| previousSetter.call(this, a); | |
| } | |
| // | |
| handler.setter(a); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/adblocker/injectors/inject.js` around lines 177 - 190, The
current accessor wrappers call previousGetter() / previousSetter(a) without a
receiver which breaks original this-binding; update the wrapper in the
Object.defineProperty call so both the original accessors and the new handler
accessors are invoked with the original receiver (use .call(this, ...) or
equivalent) — i.e., call previousGetter with this and previousSetter with this
and the argument, and also invoke handler.getter and handler.setter with this to
preserve correct semantics for owner/prop accessors.
Comment on lines
+225
to
+229
| ipc.handle('download-manager-set-concurrent', (max: number) => { | ||
| downloadManager.setMaxConcurrent(max); | ||
| // Persist config | ||
| config.maxConcurrentDownloads = max; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other IPC handlers persist config changes
rg -n "setConfig|ipc.handle.*config" --type=ts -C3Repository: pear-devs/pear-desktop
Length of output: 50378
🏁 Script executed:
# Get the full context of the downloader main file around the problematic lines
head -250 src/plugins/downloader/main/index.ts | tail -50Repository: pear-devs/pear-desktop
Length of output: 1834
🏁 Script executed:
# Also check the function signature and context of where this IPC handler is registered
head -100 src/plugins/downloader/main/index.tsRepository: pear-devs/pear-desktop
Length of output: 3396
🏁 Script executed:
# Search for other IPC handlers in the downloader plugin that modify config
rg -n "ipc.handle.*config" src/plugins/downloader/ -A 3Repository: pear-devs/pear-desktop
Length of output: 48
🏁 Script executed:
# Get the function signature for the main plugin entry point
grep -n "export const onMainLoad\|async function.*BackendContext" src/plugins/downloader/main/index.ts | head -5Repository: pear-devs/pear-desktop
Length of output: 104
🏁 Script executed:
# Find where setConfig should be available - look for the main function signature
rg -n "onMainLoad|export.*main" src/plugins/downloader/ -A 5 -B 2Repository: pear-devs/pear-desktop
Length of output: 1522
🏁 Script executed:
# Check the BackendContext type definition to confirm setConfig availability
cat src/types/contexts.ts | grep -A 20 "BackendContext"Repository: pear-devs/pear-desktop
Length of output: 722
Use setConfig() from context to persist the concurrent downloads setting.
The setConfig parameter is available in the BackendContext but is not destructured in the onMainLoad function signature. Simply assigning to config.maxConcurrentDownloads updates the in-memory variable only. The change won't persist across app restarts. Destructure setConfig from the context and call it after updating the value to ensure the setting is properly persisted, matching the pattern used in menu contexts and other plugins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/main/index.ts` around lines 225 - 229, The handler for
'download-manager-set-concurrent' currently mutates the in-memory config
(config.maxConcurrentDownloads) but doesn't persist it; update the onMainLoad
function signature to destructure setConfig from the BackendContext, then after
calling downloadManager.setMaxConcurrent(max) call setConfig({ ...config,
maxConcurrentDownloads: max }) (or setConfig with the specific key) instead of
only assigning config.maxConcurrentDownloads so the value persists across
restarts; reference the ipc.handle('download-manager-set-concurrent') handler,
downloadManager.setMaxConcurrent, config.maxConcurrentDownloads, onMainLoad,
BackendContext, and setConfig when applying the change.
Comment on lines
+200
to
+215
| const getSubtitle = (item: DownloadItem): string => { | ||
| switch (item.status) { | ||
| case 'queued': | ||
| return 'En cola…'; | ||
| case 'downloading': | ||
| return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`; | ||
| case 'completed': | ||
| return 'Completada'; | ||
| case 'failed': | ||
| return item.error ?? 'Error desconocido'; | ||
| case 'skipped': | ||
| return 'Omitida (ya existe)'; | ||
| default: | ||
| return ''; | ||
| } | ||
| }; |
There was a problem hiding this comment.
UI strings are hardcoded in Spanish instead of using i18n.
The getSubtitle function returns hardcoded Spanish strings like 'En cola…', 'Descargando…', 'Completada', etc. The i18n keys are defined in en.json and es.json under plugins.downloader.download-manager.status, but they're not being used here. This breaks localization for non-Spanish users.
🌐 Proposed fix - use i18n
The component needs to import and use the t function from @/i18n. Example for getSubtitle:
+import { t } from '@/i18n';
+
const getSubtitle = (item: DownloadItem): string => {
switch (item.status) {
case 'queued':
- return 'En cola…';
+ return t('plugins.downloader.download-manager.status.queued');
case 'downloading':
- return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`;
+ return t('plugins.downloader.download-manager.status.downloading', {
+ progress: item.progress,
+ provider: item.currentProvider,
+ attempt: item.currentAttempt,
+ });
case 'completed':
- return 'Completada';
+ return t('plugins.downloader.download-manager.status.completed');
case 'failed':
- return item.error ?? 'Error desconocido';
+ return item.error ?? t('plugins.downloader.download-manager.status.failed');
case 'skipped':
- return 'Omitida (ya existe)';
+ return t('plugins.downloader.download-manager.status.skipped');
default:
return '';
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getSubtitle = (item: DownloadItem): string => { | |
| switch (item.status) { | |
| case 'queued': | |
| return 'En cola…'; | |
| case 'downloading': | |
| return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`; | |
| case 'completed': | |
| return 'Completada'; | |
| case 'failed': | |
| return item.error ?? 'Error desconocido'; | |
| case 'skipped': | |
| return 'Omitida (ya existe)'; | |
| default: | |
| return ''; | |
| } | |
| }; | |
| import { t } from '@/i18n'; | |
| const getSubtitle = (item: DownloadItem): string => { | |
| switch (item.status) { | |
| case 'queued': | |
| return t('plugins.downloader.download-manager.status.queued'); | |
| case 'downloading': | |
| return t('plugins.downloader.download-manager.status.downloading', { | |
| progress: item.progress, | |
| provider: item.currentProvider, | |
| attempt: item.currentAttempt, | |
| }); | |
| case 'completed': | |
| return t('plugins.downloader.download-manager.status.completed'); | |
| case 'failed': | |
| return item.error ?? t('plugins.downloader.download-manager.status.failed'); | |
| case 'skipped': | |
| return t('plugins.downloader.download-manager.status.skipped'); | |
| default: | |
| return ''; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/templates/download-manager.tsx` around lines 200 -
215, The getSubtitle function currently returns hardcoded Spanish strings;
import and use the i18n translator (t) and replace each case in getSubtitle
(function getSubtitle, switch on item.status) to return
t('plugins.downloader.download-manager.status.queued'),
t('plugins.downloader.download-manager.status.downloading', { progress:
item.progress, provider: item.currentProvider, attempt: item.currentAttempt }),
t('plugins.downloader.download-manager.status.completed'),
t('plugins.downloader.download-manager.status.failed', { error: item.error ??
t('plugins.downloader.download-manager.status.unknownError') }), and
t('plugins.downloader.download-manager.status.skipped') respectively so messages
and interpolations come from en.json/es.json instead of hardcoded Spanish.
Comment on lines
+219
to
+377
| return ( | ||
| <> | ||
| {/* Toggle Button */} | ||
| <button | ||
| class={`dm-toggle-btn ${isOpen() ? 'dm-active' : ''}`} | ||
| onClick={() => setIsOpen(!isOpen())} | ||
| title="Gestor de Descargas" | ||
| id="ytmd-download-manager-toggle" | ||
| > | ||
| <DownloadIcon /> | ||
| <Show when={badgeCount() > 0}> | ||
| <span class="dm-badge">{badgeCount()}</span> | ||
| </Show> | ||
| </button> | ||
|
|
||
| {/* Panel */} | ||
| <div class={`dm-panel ${isOpen() ? 'dm-open' : ''}`} id="ytmd-download-manager-panel"> | ||
| {/* Header */} | ||
| <div class="dm-header"> | ||
| <div class="dm-header-title"> | ||
| <DownloadIcon /> | ||
| <span>Gestor de Descargas</span> | ||
| </div> | ||
| <button class="dm-close-btn" onClick={() => setIsOpen(false)}> | ||
| <CloseIcon /> | ||
| </button> | ||
| </div> | ||
|
|
||
| {/* Controls */} | ||
| <div class="dm-controls"> | ||
| <div class="dm-concurrent-selector"> | ||
| <span>Simultáneas:</span> | ||
| <select value={maxConcurrent()} onChange={setConcurrent}> | ||
| <For each={[1, 2, 3, 4, 5]}> | ||
| {(n) => <option value={n}>{n}</option>} | ||
| </For> | ||
| </select> | ||
| </div> | ||
| <button | ||
| class={`dm-ctrl-btn dm-pause`} | ||
| onClick={togglePause} | ||
| > | ||
| {isPaused() ? '▶ Reanudar' : '⏸ Pausar'} | ||
| </button> | ||
| <Show when={failedItems().length > 0}> | ||
| <button class="dm-ctrl-btn dm-retry" onClick={retryFailed}> | ||
| 🔄 Reintentar ({failedItems().length}) | ||
| </button> | ||
| </Show> | ||
| <Show when={completedItems().length > 0}> | ||
| <button class="dm-ctrl-btn dm-clear" onClick={clearCompleted}> | ||
| 🗑️ Limpiar | ||
| </button> | ||
| </Show> | ||
| </div> | ||
|
|
||
| {/* Tabs */} | ||
| <div class="dm-tabs"> | ||
| <button | ||
| class={`dm-tab ${activeTab() === 'queue' ? 'dm-tab-active' : ''}`} | ||
| onClick={() => setActiveTab('queue')} | ||
| > | ||
| 📥 Cola | ||
| <Show when={queueItems().length > 0}> | ||
| <span class="dm-tab-badge">{queueItems().length}</span> | ||
| </Show> | ||
| </button> | ||
| <button | ||
| class={`dm-tab ${activeTab() === 'failed' ? 'dm-tab-active' : ''}`} | ||
| onClick={() => setActiveTab('failed')} | ||
| > | ||
| ❌ Fallidas | ||
| <Show when={failedItems().length > 0}> | ||
| <span class="dm-tab-badge">{failedItems().length}</span> | ||
| </Show> | ||
| </button> | ||
| <button | ||
| class={`dm-tab ${activeTab() === 'completed' ? 'dm-tab-active' : ''}`} | ||
| onClick={() => setActiveTab('completed')} | ||
| > | ||
| ✅ Listas | ||
| <Show when={completedItems().length > 0}> | ||
| <span class="dm-tab-badge">{completedItems().length}</span> | ||
| </Show> | ||
| </button> | ||
| </div> | ||
|
|
||
| {/* List */} | ||
| <div class="dm-list"> | ||
| <Show | ||
| when={currentTabItems().length > 0} | ||
| fallback={ | ||
| <div class="dm-empty"> | ||
| <EmptyIcon /> | ||
| <span>No hay descargas</span> | ||
| </div> | ||
| } | ||
| > | ||
| <For each={currentTabItems()}> | ||
| {(item) => ( | ||
| <div class="dm-item"> | ||
| <div class={`dm-item-icon dm-${item.status}`}> | ||
| <StatusIcon status={item.status} /> | ||
| </div> | ||
| <div class="dm-item-info"> | ||
| <div class="dm-item-title"> | ||
| {item.artist ? `${item.artist} - ${item.title}` : item.title} | ||
| </div> | ||
| <div class="dm-item-subtitle">{getSubtitle(item)}</div> | ||
| <Show when={item.status === 'downloading'}> | ||
| <div class="dm-item-progress"> | ||
| <div | ||
| class="dm-item-progress-bar" | ||
| style={{ width: `${item.progress}%` }} | ||
| /> | ||
| </div> | ||
| </Show> | ||
| </div> | ||
| <Show when={item.status === 'failed'}> | ||
| <div class="dm-item-actions"> | ||
| <button | ||
| class="dm-item-action-btn dm-retry-btn" | ||
| onClick={() => retrySingle(item.id)} | ||
| title="Reintentar" | ||
| > | ||
| 🔄 | ||
| </button> | ||
| <button | ||
| class="dm-item-action-btn dm-remove-btn" | ||
| onClick={() => removeFailed(item.id)} | ||
| title="Eliminar" | ||
| > | ||
| ✕ | ||
| </button> | ||
| </div> | ||
| </Show> | ||
| </div> | ||
| )} | ||
| </For> | ||
| </Show> | ||
| </div> | ||
|
|
||
| {/* Stats */} | ||
| <div class="dm-stats"> | ||
| <div class="dm-stat"> | ||
| ⬇️ <span class="dm-stat-value">{activeCount()}</span> activas | ||
| </div> | ||
| <div class="dm-stat dm-stat-completed"> | ||
| ✅ <span class="dm-stat-value">{totalCompleted()}</span> | ||
| </div> | ||
| <div class="dm-stat dm-stat-failed"> | ||
| ❌ <span class="dm-stat-value">{totalFailed()}</span> | ||
| </div> | ||
| <div class="dm-stat dm-stat-skipped"> | ||
| ⏭️ <span class="dm-stat-value">{totalSkipped()}</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </> |
There was a problem hiding this comment.
Multiple hardcoded Spanish strings in JSX need i18n.
Several UI elements use hardcoded Spanish text instead of the i18n keys defined in the translation files:
| Line | Hardcoded Text | i18n Key |
|---|---|---|
| 225 | "Gestor de Descargas" |
download-manager.title |
| 240 | "Gestor de Descargas" |
download-manager.title |
| 250 | "Simultáneas:" |
download-manager.concurrent-downloads |
| 261 | "▶ Reanudar" / "⏸ Pausar" |
download-manager.resume / download-manager.pause |
| 265 | "🔄 Reintentar" |
download-manager.retry-failed |
| 270 | "🗑️ Limpiar" |
download-manager.clear-completed |
| 281, 290, 299 | Tab labels | download-manager.tabs.* |
| 313 | "No hay descargas" |
download-manager.no-items |
| 364 | "activas" |
download-manager.active |
🧰 Tools
🪛 ESLint
[error] 226-226: Props should be sorted alphabetically
(stylistic/jsx-sort-props)
[error] 235-235: Replace ·class={dm-panel·${isOpen()·?·'dm-open'·:·''}}·id="ytmd-download-manager-panel" with ⏎········class={dm-panel·${isOpen()·?·'dm-open'·:·''}}⏎········id="ytmd-download-manager-panel"⏎······
(prettier/prettier)
[error] 251-251: Props should be sorted alphabetically
(stylistic/jsx-sort-props)
[error] 257-260: Replace ⏎············class={dm-ctrl-btn·dm-pause}⏎············onClick={togglePause}⏎·········· with ·class={dm-ctrl-btn·dm-pause}·onClick={togglePause}
(prettier/prettier)
[error] 310-310: Props should be sorted alphabetically
(stylistic/jsx-sort-props)
[error] 325-325: Replace ·?·${item.artist}·-·${item.title}`` with ⏎························?·${item.artist}·-·${item.title}`⏎·······················`
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/templates/download-manager.tsx` around lines 219 -
377, Replace all hardcoded Spanish UI strings in the Download Manager JSX with
i18n lookups using the translation keys; specifically update the title text in
the toggle button and header (currently "Gestor de Descargas") to use
download-manager.title, the concurrent label "Simultáneas:" to
download-manager.concurrent-downloads, the pause/resume labels to
download-manager.pause and download-manager.resume, the retry/clear button
labels to download-manager.retry-failed and download-manager.clear-completed,
the three tab labels to download-manager.tabs.* (queue/failed/completed), the
empty state "No hay descargas" to download-manager.no-items, and the "activas"
stat label to download-manager.active; locate these strings inside the Download
Manager component return JSX (elements around the toggle button, dm-header,
dm-controls, dm-tabs, dm-empty and dm-stats) and replace them with calls to the
app’s i18n function (e.g., t(...)) so the UI uses the translation keys instead
of hardcoded text.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/downloader/templates/download-manager.css`:
- Line 173: The font-family declarations use an unnecessary quoted font name
causing Stylelint rule font-family-name-quotes to fail; update the font-family
lines (the declarations containing "font-family: 'Google Sans', 'Roboto',
sans-serif;" and the other occurrence) to remove the quotes around Roboto so
they read font-family: 'Google Sans', Roboto, sans-serif; (keep Google Sans
quoted).
- Around line 295-309: The rule for .dm-concurrent-selector select removes the
browser focus ring (outline: none) but doesn't provide a visible replacement, so
add a keyboard-accessible focus style by defining a :focus-visible (and
optionally :focus) rule for .dm-concurrent-selector select that restores a clear
focus indicator (e.g., adjust border-color and/or add a box-shadow ring) while
keeping the existing visual design; ensure the selector names
.dm-concurrent-selector select and the new .dm-concurrent-selector
select:focus-visible are used so keyboard users see a visible focus state.
In `@src/plugins/downloader/templates/download-manager.tsx`:
- Around line 105-108: The initial panel x/y and the drag bounds can put the
panel off-screen because window.innerWidth - 420 is used and the drag logic uses
a different hardcoded width; create a single constant for the panel width/height
(e.g., PANEL_WIDTH, PANEL_HEIGHT) and use it to initialize the position signal
and in the drag clamp logic (locations referencing position/setPosition and the
drag handlers around lines ~202-206), clamp the initial x to [0,
window.innerWidth - PANEL_WIDTH] and y to [0, window.innerHeight - PANEL_HEIGHT]
so the panel is always fully on screen, and replace any other hardcoded
380/400/420 values with the new constants.
- Around line 316-318: The icon-only buttons (e.g., the toast close button using
class "dm-toast-close" and CloseIcon, plus the panel close and retry/remove
buttons referenced around lines 341-343 and 438-449) lack accessible names; add
explicit accessible names by adding aria-label attributes to each icon-only
<button> (for example aria-label="Close toast", aria-label="Close panel",
aria-label="Retry download", aria-label="Remove download") or use
aria-labelledby pointing to a visible label; update the buttons that call
removeToast, retryDownload, removeDownload, or the panel close handler so the
assistive name clearly describes the action. Ensure labels are unique and
descriptive for screen-reader users.
- Around line 289-295: The subtitle currently hardcodes the max retries as "/3"
in getSubtitle causing incorrect UI when retry policy changes; update the
template to use the DownloadItem.totalProviderAttempts property instead of 3 so
the string shows `${item.currentAttempt}/${item.totalProviderAttempts}` (keep
using item.currentProvider and item.progress as-is) to reflect the actual
configured retry count.
- Around line 157-171: The effect in createEffect is firing a "download started"
toast on initial hydration because previousQueueLength() initializes to 0; add
an initialization guard so the first run only sets previousQueueLength to
currentQueueLength without showing any toasts. Concretely, in the createEffect
that reads items(), currentQueueLength, calls addToast, and then calls
setPreviousQueueLength, introduce a check (e.g., an "initialized" flag or treat
previousQueueLength() === null/undefined as first-run) to skip
computing/dispatching the toast on the first invocation, then set
previousQueueLength(currentQueueLength) and mark initialized true so subsequent
changes still trigger addToast as before.
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx`:
- Around line 55-60: The icon-only button in TitleBar.tsx currently hardcodes
Spanish in the title attribute; update the button (the element with class
"ytmd-dm-title-btn" that calls handleClick and uses isOpen()) to use the
existing download-manager title translation instead of a hardcoded string and
set that translated string as both aria-label and title to provide a localized,
accessible name for the control; reuse the project's translation helper (the
same key used for the download manager title) so the button's accessible name is
consistent with other UI.
- Around line 35-36: The toggle handler flips local state which can go out of
sync with the panel (handleToggle using setIsOpen(prev => !prev)); change the
event contract to carry an explicit open boolean (event.detail.open) and update
the listener in TitleBar (and the other occurrences at the same block) to call
setIsOpen(open) instead of toggling; also ensure the download-manager emitter
(e.g., the close logic in download-manager.tsx) dispatches the same
'ytmd-download-manager-toggle' event with { open: false } (and true when
opening) so both sides use the single source-of-truth payload.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 376132d8-a474-4878-980a-ee1acceb7c4f
📒 Files selected for processing (4)
src/plugins/downloader/templates/download-manager.csssrc/plugins/downloader/templates/download-manager.tsxsrc/plugins/in-app-menu/renderer/TitleBar.tsxsrc/plugins/in-app-menu/titlebar.css
Comment on lines
+295
to
+309
| .dm-concurrent-selector select { | ||
| background: rgba(255, 255, 255, 0.08); | ||
| color: #e0e0e0; | ||
| border: 1px solid rgba(255, 255, 255, 0.1); | ||
| border-radius: 6px; | ||
| padding: 4px 8px; | ||
| font-size: 12px; | ||
| cursor: pointer; | ||
| outline: none; | ||
| transition: border-color 0.2s ease; | ||
| } | ||
|
|
||
| .dm-concurrent-selector select:hover { | ||
| border-color: rgba(255, 255, 255, 0.2); | ||
| } |
There was a problem hiding this comment.
Restore a visible focus state for the concurrency selector.
Line 303 removes the browser focus ring, but this rule set never adds a :focus-visible replacement. Keyboard users will have no visible indication that the select is focused.
Diff
.dm-concurrent-selector select {
background: rgba(255, 255, 255, 0.08);
color: `#e0e0e0`;
border: 1px solid rgba(255, 255, 255, 0.1);
border-radius: 6px;
padding: 4px 8px;
font-size: 12px;
cursor: pointer;
- outline: none;
transition: border-color 0.2s ease;
}
.dm-concurrent-selector select:hover {
border-color: rgba(255, 255, 255, 0.2);
}
+
+.dm-concurrent-selector select:focus-visible {
+ outline: 2px solid `#a78bfa`;
+ outline-offset: 2px;
+ border-color: `#a78bfa`;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/templates/download-manager.css` around lines 295 -
309, The rule for .dm-concurrent-selector select removes the browser focus ring
(outline: none) but doesn't provide a visible replacement, so add a
keyboard-accessible focus style by defining a :focus-visible (and optionally
:focus) rule for .dm-concurrent-selector select that restores a clear focus
indicator (e.g., adjust border-color and/or add a box-shadow ring) while keeping
the existing visual design; ensure the selector names .dm-concurrent-selector
select and the new .dm-concurrent-selector select:focus-visible are used so
keyboard users see a visible focus state.
Comment on lines
+289
to
+295
| const getSubtitle = (item: DownloadItem): string => { | ||
| switch (item.status) { | ||
| case 'queued': | ||
| return 'En cola…'; | ||
| case 'downloading': | ||
| return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`; | ||
| case 'completed': |
There was a problem hiding this comment.
Use totalProviderAttempts instead of hardcoding /3.
DownloadItem already carries totalProviderAttempts, so the subtitle becomes wrong as soon as the retry policy changes.
Diff
case 'downloading':
- return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`;
+ return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/${item.totalProviderAttempts})`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getSubtitle = (item: DownloadItem): string => { | |
| switch (item.status) { | |
| case 'queued': | |
| return 'En cola…'; | |
| case 'downloading': | |
| return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/3)`; | |
| case 'completed': | |
| const getSubtitle = (item: DownloadItem): string => { | |
| switch (item.status) { | |
| case 'queued': | |
| return 'En cola…'; | |
| case 'downloading': | |
| return `Descargando… ${item.progress}% — ${item.currentProvider} (${item.currentAttempt}/${item.totalProviderAttempts})`; | |
| case 'completed': |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/templates/download-manager.tsx` around lines 289 -
295, The subtitle currently hardcodes the max retries as "/3" in getSubtitle
causing incorrect UI when retry policy changes; update the template to use the
DownloadItem.totalProviderAttempts property instead of 3 so the string shows
`${item.currentAttempt}/${item.totalProviderAttempts}` (keep using
item.currentProvider and item.progress as-is) to reflect the actual configured
retry count.
Comment on lines
+316
to
+318
| <button class="dm-toast-close" onClick={() => removeToast(toast.id)}> | ||
| <CloseIcon /> | ||
| </button> |
There was a problem hiding this comment.
Add accessible names to the icon-only action buttons.
The toast close button, panel close button, and retry/remove buttons are exposed as SVG/emoji-only controls. Without aria-labels, assistive tech gets no stable action name for these actions.
Also applies to: 341-343, 438-449
🧰 Tools
🪛 ESLint
[error] 316-316: Replace ·class="dm-toast-close"·onClick={()·=>·removeToast(toast.id)} with ⏎················class="dm-toast-close"⏎················onClick={()·=>·removeToast(toast.id)}⏎··············
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/downloader/templates/download-manager.tsx` around lines 316 -
318, The icon-only buttons (e.g., the toast close button using class
"dm-toast-close" and CloseIcon, plus the panel close and retry/remove buttons
referenced around lines 341-343 and 438-449) lack accessible names; add explicit
accessible names by adding aria-label attributes to each icon-only <button> (for
example aria-label="Close toast", aria-label="Close panel", aria-label="Retry
download", aria-label="Remove download") or use aria-labelledby pointing to a
visible label; update the buttons that call removeToast, retryDownload,
removeDownload, or the panel close handler so the assistive name clearly
describes the action. Ensure labels are unique and descriptive for screen-reader
users.
Comment on lines
+55
to
+60
| <button | ||
| class="ytmd-dm-title-btn" | ||
| onClick={handleClick} | ||
| title="Gestor de Descargas" | ||
| data-active={isOpen()} | ||
| > |
There was a problem hiding this comment.
Localize and label this icon-only button.
Line 58 hardcodes Spanish in the title bar, and title alone is not a reliable accessible name for an icon-only control. Reuse the download-manager title translation here and expose it through aria-label as well.
🧰 Tools
🪛 ESLint
[error] 59-59: Props should be sorted alphabetically
(stylistic/jsx-sort-props)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx` around lines 55 - 60, The
icon-only button in TitleBar.tsx currently hardcodes Spanish in the title
attribute; update the button (the element with class "ytmd-dm-title-btn" that
calls handleClick and uses isOpen()) to use the existing download-manager title
translation instead of a hardcoded string and set that translated string as both
aria-label and title to provide a localized, accessible name for the control;
reuse the project's translation helper (the same key used for the download
manager title) so the button's accessible name is consistent with other UI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/plugins/in-app-menu/renderer/TitleBar.tsx (1)
22-75: Move component definition below all imports.The
DownloadManagerTitleButtoncomponent is defined between import statements (lines 22–73 appear before thecacheNoArgsimport on line 75). This breaks standard module organization where all imports should precede component/function definitions.Suggested fix
Move the entire
DownloadManagerTitleButtoncomponent definition (lines 22–73) to after all import statements (after line 78).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/in-app-menu/renderer/TitleBar.tsx` around lines 22 - 75, The DownloadManagerTitleButton component is declared before all imports (it appears above the import of cacheNoArgs); move the entire DownloadManagerTitleButton function/component definition so that all import statements (including import { cacheNoArgs } from '@/providers/decorators') come first, then place DownloadManagerTitleButton below them to restore proper module ordering and avoid interleaving definitions and imports.src/plugins/downloader/templates/download-manager.tsx (2)
292-319: Consider adding error handling to IPC invocations.The action functions invoke IPC methods without handling potential errors. While failures may be rare, unhandled promise rejections could cause silent failures. Consider adding
.catch()handlers or wrapping in try/catch for user feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/templates/download-manager.tsx` around lines 292 - 319, The IPC invocation handlers (setConcurrent, togglePause, retryFailed, retrySingle, removeFailed, clearCompleted) call props.ipc.invoke(...) without any error handling; wrap each invoke call in a try/catch or append .catch(...) to handle rejected promises and surface errors (e.g., log via console.error or notify the user) so failures aren’t silently ignored. Update each function (setConcurrent, togglePause, retryFailed, retrySingle, removeFailed, clearCompleted) to await the invoke or attach a .catch that handles and logs the error and optionally shows user feedback.
137-148: Consider moving IPC setup intoonMount.The IPC listeners (lines 137–138) and initial state fetch (lines 146–148) are registered synchronously during component creation rather than inside
onMount. While this works in SolidJS, wrapping IPC setup inonMountis more conventional and ensures the component is fully initialized before receiving events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/downloader/templates/download-manager.tsx` around lines 137 - 148, Move the IPC listener registration and initial state fetch into Solid's lifecycle by importing and using onMount: register props.ipc.on('download-manager-state', onStateUpdate) and props.ipc.on('download-manager-item-update', onItemUpdate) inside onMount, and also call props.ipc.invoke('download-manager-get-state') there (then call onStateUpdate with the returned DownloadManagerState); keep the props.ipc.removeAllListeners calls in the existing onCleanup to tear down listeners. This ensures props.ipc, onStateUpdate and onItemUpdate are set up when the component is mounted while download-manager-get-state remains invoked after mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/downloader/templates/download-manager.tsx`:
- Around line 209-218: The setTimeout created inside addToast can fire after the
component unmounts and cause state updates on an unmounted component; modify
addToast to store each timeout ID (e.g., in a Map or ref keyed by the toast id)
when you call setTimeout, and ensure removeToast clears its associated timeout
via clearTimeout(timeoutId) before removing from setToasts; additionally
register an onCleanup (or component unmount) handler that iterates stored
timeout IDs and calls clearTimeout for each to cancel pending timers. Ensure
references to addToast, removeToast, setToasts and Toast id are used to
correlate and clear timers.
---
Nitpick comments:
In `@src/plugins/downloader/templates/download-manager.tsx`:
- Around line 292-319: The IPC invocation handlers (setConcurrent, togglePause,
retryFailed, retrySingle, removeFailed, clearCompleted) call
props.ipc.invoke(...) without any error handling; wrap each invoke call in a
try/catch or append .catch(...) to handle rejected promises and surface errors
(e.g., log via console.error or notify the user) so failures aren’t silently
ignored. Update each function (setConcurrent, togglePause, retryFailed,
retrySingle, removeFailed, clearCompleted) to await the invoke or attach a
.catch that handles and logs the error and optionally shows user feedback.
- Around line 137-148: Move the IPC listener registration and initial state
fetch into Solid's lifecycle by importing and using onMount: register
props.ipc.on('download-manager-state', onStateUpdate) and
props.ipc.on('download-manager-item-update', onItemUpdate) inside onMount, and
also call props.ipc.invoke('download-manager-get-state') there (then call
onStateUpdate with the returned DownloadManagerState); keep the
props.ipc.removeAllListeners calls in the existing onCleanup to tear down
listeners. This ensures props.ipc, onStateUpdate and onItemUpdate are set up
when the component is mounted while download-manager-get-state remains invoked
after mount.
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx`:
- Around line 22-75: The DownloadManagerTitleButton component is declared before
all imports (it appears above the import of cacheNoArgs); move the entire
DownloadManagerTitleButton function/component definition so that all import
statements (including import { cacheNoArgs } from '@/providers/decorators') come
first, then place DownloadManagerTitleButton below them to restore proper module
ordering and avoid interleaving definitions and imports.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3543ce55-e5b2-488c-903f-695b94b81e6d
📒 Files selected for processing (3)
src/plugins/downloader/templates/download-manager.tsxsrc/plugins/in-app-menu/renderer/TitleBar.tsxsrc/plugins/in-app-menu/titlebar.css
✅ Files skipped from review due to trivial changes (1)
- src/plugins/in-app-menu/titlebar.css
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx`:
- Around line 32-33: The JSX and event-listener lines in TitleBar.tsx (notably
the window.addEventListener('ytmd-download-badge-update', handleBadgeUpdate as
EventListener) and the JSX prop blocks around the other noted ranges) are
failing Prettier and jsx-sort-props checks; fix by running Prettier-style
formatting and reorder JSX props to match the project's jsx-sort-props rule
(typically alphabetical) for the components in this file, and normalize the
event listener call by removing or moving the type cast into a separate const
(e.g., const badgeListener: EventListener = handleBadgeUpdate;
window.addEventListener('ytmd-download-badge-update', badgeListener);) so the
expression is formatted cleanly; apply the same formatting/prop-sorting to the
blocks at the other mentioned ranges (45-46, 59-64).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04ab9f58-a0a8-4684-9d10-3bf31c4e9cd8
📒 Files selected for processing (1)
src/plugins/in-app-menu/renderer/TitleBar.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/plugins/in-app-menu/renderer/TitleBar.tsx (2)
33-49:⚠️ Potential issue | 🟡 MinorThis JSX block still fails current lint/format rules.
The prop-order and Prettier issues in this block are still present (including the style object trailing comma / multiline SVG prop formatting). Please apply formatter + lint autofix for this section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/in-app-menu/renderer/TitleBar.tsx` around lines 33 - 49, In TitleBar.tsx fix the failing lint/format issues in the JSX block: reorder props to satisfy prop-order (keep style last or as your project's convention), remove the trailing comma in the inline style object, and convert SVG attributes to JSX camelCase (e.g., strokeWidth, strokeLinecap, strokeLinejoin) and format the SVG props into a single consistent multiline style; then run Prettier/ESLint autofix (or your project's formatter) on this file to apply the remaining formatting fixes.
30-33:⚠️ Potential issue | 🟠 MajorLocalize and properly label this icon-only button.
Line 32 is still hardcoded (
"Gestor de Descargas"), and the control still lacks an explicit accessible name viaaria-label. Please reuse the existing download-manager translation string and apply it to bothtitleandaria-label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/in-app-menu/renderer/TitleBar.tsx` around lines 30 - 33, The button in TitleBar.tsx that calls handleClick currently hardcodes title="Gestor de Descargas" and is missing an aria-label; replace the hardcoded text by reusing the existing download-manager translation string (the same translated value) for both the button's title and aria-label attributes so the icon-only control has an explicit accessible name and uses the existing i18n key.
🧹 Nitpick comments (1)
src/plugins/in-app-menu/renderer/TitleBar.tsx (1)
24-26: Remove debug logging from the click handler.Line 25 looks like leftover debug output in renderer code.
Suggested cleanup
const handleClick = () => { - console.log('Download Manager button clicked'); window.dispatchEvent(new CustomEvent('ytmd-download-manager-toggle')); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/in-app-menu/renderer/TitleBar.tsx` around lines 24 - 26, Remove the leftover debug console.log from the click handler: in the handleClick function that dispatches the 'ytmd-download-manager-toggle' CustomEvent (used in TitleBar.tsx), delete the console.log('Download Manager button clicked') line so the handler only dispatches the event; ensure no other debug logs remain in that function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx`:
- Around line 33-49: In TitleBar.tsx fix the failing lint/format issues in the
JSX block: reorder props to satisfy prop-order (keep style last or as your
project's convention), remove the trailing comma in the inline style object, and
convert SVG attributes to JSX camelCase (e.g., strokeWidth, strokeLinecap,
strokeLinejoin) and format the SVG props into a single consistent multiline
style; then run Prettier/ESLint autofix (or your project's formatter) on this
file to apply the remaining formatting fixes.
- Around line 30-33: The button in TitleBar.tsx that calls handleClick currently
hardcodes title="Gestor de Descargas" and is missing an aria-label; replace the
hardcoded text by reusing the existing download-manager translation string (the
same translated value) for both the button's title and aria-label attributes so
the icon-only control has an explicit accessible name and uses the existing i18n
key.
---
Nitpick comments:
In `@src/plugins/in-app-menu/renderer/TitleBar.tsx`:
- Around line 24-26: Remove the leftover debug console.log from the click
handler: in the handleClick function that dispatches the
'ytmd-download-manager-toggle' CustomEvent (used in TitleBar.tsx), delete the
console.log('Download Manager button clicked') line so the handler only
dispatches the event; ensure no other debug logs remain in that function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2fcb160-f3a0-4f32-ae0d-fea48d4740c2
📒 Files selected for processing (1)
src/plugins/in-app-menu/renderer/TitleBar.tsx
This reverts commit 8cc256a.
This reverts commit 0e65dba.
This reverts commit 07638c3.
This reverts commit 99eb27d.
…n generate the commit message for you.
… so I can generate the commit message for you.
…to reduce IPC and network congestion
… distros missing Roboto
… when ad finishes in adSpeedup plugin
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
2 participants
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.
The problem with downloads on Windows and others has been resolved.
Summary by CodeRabbit
New Features
Internationalization
Chores