Test/merge upstream 2026 03 11 - #3
Conversation
This workflow automates the process of updating the lock file by checking out the code, configuring Git, setting up Bun, caching dependencies, installing them, and committing any changes back to the repository.
…e-workflow Add workflow to update lock file automatically
…-pixels-visualizer-and-visualizer-switching fix(visualizer): correct LED layout and visualizer switching
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughMajor version upgrade to 2.5.0 introducing video streaming support (HLS/DASH), switching authentication from Firebase to Appwrite, implementing command palette and infinite radio functionality, adding comprehensive metadata handling via TagLib, expanding mobile capabilities with new Capacitor plugins, and integrating R2 storage for uploads. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
js/mobile/capacitor.js (1)
1-27:⚠️ Potential issue | 🟡 MinorInconsistent log prefixes and stale file comment.
The log messages at lines 6 and 13 were updated to
[Capacitor], but line 25 still uses[Desktop]. Also, the file header comment on line 1 referencesjs/desktop/desktop.jswhich appears to be stale.Proposed fix for consistency
-// js/desktop/desktop.js +// js/mobile/capacitor.js import CapacitorBridge from './capacitor-bridge.js';} catch (error) { - console.error('[Desktop] Failed to initialize desktop environment:', error); + console.error('[Capacitor] Failed to initialize mobile environment:', error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/mobile/capacitor.js` around lines 1 - 27, The file header comment is stale and the console log prefixes in initDesktop are inconsistent; update the top-of-file comment (remove or correct the "js/desktop/desktop.js" reference) and change the remaining console.error prefix in the initDesktop function from "[Desktop]" to "[Capacitor]" so all logs use the same "[Capacitor]" prefix; locate the initDesktop function and the header comment to make these two small text edits.js/db.js (1)
590-597:⚠️ Potential issue | 🟠 MajorPlaylist dedupe still needs media type in the key.
Now that playlists can hold both tracks and videos,
some((t) => t.id === track.id)incorrectly treats atrackand avideowith the same id as duplicates.removeTrackFromPlaylist()already neededtrackType; additions need the same discriminator.Suggested fix
- if (playlist.tracks.some((t) => t.id === track.id)) return; + const itemType = track.type || 'track'; + if (playlist.tracks.some((t) => t.id === track.id && (t.type || 'track') === itemType)) return; @@ - if (!playlist.tracks.some((t) => t.id === track.id)) { + const itemType = track.type || 'track'; + if (!playlist.tracks.some((t) => t.id === track.id && (t.type || 'track') === itemType)) { const trackWithDate = { ...track, addedAt: Date.now() }; playlist.tracks.push(this._minifyItem(track.type || 'track', trackWithDate)); addedCount++; }Also applies to: 613-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/db.js` around lines 590 - 597, The dedupe check in addTrackToPlaylist treats items with the same id but different media types as duplicates; update addTrackToPlaylist to consider the media type discriminator when checking for existing items by comparing both id and type (use track.type || 'track' to match how items are minified); likewise update removeTrackFromPlaylist (the corresponding logic around lines 613-617) to require both id and type when finding/removing an item so tracks and videos with the same id are handled separately.functions/upload.js (1)
69-91:⚠️ Potential issue | 🔴 CriticalDo not let
/uploadfetch arbitrary remote URLs.
fileUrlgives any caller a server-side fetch primitive, and the entire response is buffered before upload. That bypasses the 10 MB guard entirely for JSON-origin uploads and makes this endpoint usable as an open proxy/downloader unless the URL is tightly allowlisted and size-limited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/upload.js` around lines 69 - 91, The current JSON branch allows fetching arbitrary URLs and buffers the entire response (res.arrayBuffer()) bypassing the 10MB guard; instead validate and limit remote fetches: in the block that reads body.fileUrl (the JSON branch using request.json(), fileUrl, fetch, res.arrayBuffer()), first ensure the URL uses http/https and matches an allowlist or trusted host check, then perform a HEAD request (or a small ranged GET) to read Content-Length and reject if missing or > 10 * 1024 * 1024; if Content-Length is acceptable, perform the GET but stream the response and enforce a byte-counter while reading (abort and error if exceeded) rather than calling res.arrayBuffer(); keep the existing multipart/form-data path (uploaded.size check) as-is.index.html (2)
1832-1847:⚠️ Potential issue | 🟠 MajorDon’t hardcode the logo to the public hosted instance.
Pointing the sidebar logo at
https://monochrome.tf/ejects self-hosted and custom-instance users out of their current deployment. This should stay origin-relative (/) or be derived from the active instance instead of forcing navigation to the public site.Suggested fix
- <a href="https://monochrome.tf/" class="sidebar-logo-link"> + <a href="/" class="sidebar-logo-link">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 1832 - 1847, The sidebar anchor with class "sidebar-logo-link" currently hardcodes href="https://monochrome.tf/" which forces users off their deployment; change it to an origin-relative or instance-derived link (e.g., set href="/" or compute from window.location.origin) so the logo navigates within the current instance; update the anchor in index.html (selector: sidebar-logo-link / app-logo) to use the relative root or assign the href at runtime from the active instance base URL.
5500-5516:⚠️ Potential issue | 🟠 MajorThis privacy copy is now misleading.
The page shows the signed-in email address on Line 5507, but the copy below says “We do not store anything like emails.” That statement is no longer accurate for an email-auth flow and is risky from a compliance/trust perspective. Please update the notice to clearly distinguish what is stored locally vs by the auth backend.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 5500 - 5516, The privacy text is now misleading because the page displays the signed-in email via window.authManager.user.email (element id "auth-status"), so update the copy to accurately state that the app displays/stores the signed-in email for authentication while clarifying what is stored locally vs by the auth backend: modify the paragraph following the auth-status element to explicitly say the app stores a randomized ID and music data locally, that the authentication provider (or backend) retains the email used for sign-in (and is not kept in the anonymous music dataset), and remove or reword the absolute “we do not store emails” claim to reflect this separation.styles.css (1)
7593-7601:⚠️ Potential issue | 🟠 MajorKeep the EQ response canvas anchored to the equalizer wrapper.
position: fixedpins the curve to the viewport instead of.equalizer-bands-wrapper, so it will drift away from the sliders as the settings panel scrolls. This needs to stay positioned relative to the wrapper.💡 Proposed fix
.eq-response-canvas { - position: fixed; + position: absolute; top: var(--spacing-md); left: 4px; width: calc(100% - 8px);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles.css` around lines 7593 - 7601, The .eq-response-canvas is fixed to the viewport; change its positioning to be relative to the equalizer container by switching .eq-response-canvas from position: fixed to position: absolute and adjusting its offsets (top/left/width/height as needed) so it aligns inside the wrapper, and ensure the parent selector .equalizer-bands-wrapper has position: relative (and overflow visible if needed) so the canvas remains anchored to the wrapper rather than the viewport.js/metadata.js (1)
153-180:⚠️ Potential issue | 🟠 MajorPreserve the
{ name }artist contract for local metadata.
metadata.artiststarts as an object, but this path replaces it with a string.js/app.jsstill readstrack.artist.namewhen sorting local files on Lines 2371-2372, so once TagLib metadata is read those artists collapse to empty strings.Possible fix
- metadata.artist = data.artist || metadata.artist; + metadata.artist = { name: data.artist || metadata.artist.name }; @@ - metadata.artist = metadata.artists[0]; + metadata.artist = { name: metadata.artists[0] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/metadata.js` around lines 153 - 180, The code replaces the initial metadata.artist object with plain strings which breaks callers expecting metadata.artist.name; update the assignments so metadata.artist remains an object with a name property: when consuming data.artist assign metadata.artist = { name: data.artist } (only if data.artist present), and when you set metadata.artist from the artists array at the end change metadata.artist = { name: metadata.artists[0] } (guarding for empty array), ensuring metadata.artists remains an array of strings while metadata.artist stays an object as consumers expect.js/player.js (1)
1598-1622:⚠️ Potential issue | 🟠 MajorClear the load timeout once the media settles.
The timer keeps running after
canplay/errorresolves the promise. Ten seconds later it still mutatesautoplayBlockedor tries to reject again, which can leave the player in the wrong state after a successful load.Suggested fix
return await new Promise((resolve, reject) => { + const timeoutId = setTimeout(() => { + cleanup(); + if (document.visibilityState === 'hidden' || (this.isIOS && this.isPwa)) { + this.autoplayBlocked = true; + resolve(false); + return; + } + reject(new Error('Timeout waiting for audio to load')); + }, timeoutMs); + + const cleanup = () => { + clearTimeout(timeoutId); + element.removeEventListener('canplay', onCanPlay); + element.removeEventListener('error', onError); + }; + const onCanPlay = () => { - element.removeEventListener('canplay', onCanPlay); - element.removeEventListener('error', onError); + cleanup(); resolve(true); }; const onError = (e) => { - element.removeEventListener('canplay', onCanPlay); - element.removeEventListener('error', onError); + cleanup(); reject(e); }; element.addEventListener('canplay', onCanPlay); element.addEventListener('error', onError); - - // Timeout after 10 seconds. Treat as autoplay blocked when backgrounded (esp. iOS PWA). - setTimeout(() => { - element.removeEventListener('canplay', onCanPlay); - element.removeEventListener('error', onError); - if (document.visibilityState === 'hidden' || (this.isIOS && this.isPwa)) { - this.autoplayBlocked = true; - resolve(false); - return; - } - reject(new Error('Timeout waiting for audio to load')); - }, timeoutMs); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/player.js` around lines 1598 - 1622, The load timeout created in the promise for waiting on media (the setTimeout call using timeoutMs) isn't cleared when the promise settles; change the promise in js/player.js so you capture the timer id (e.g., const timerId = setTimeout(...)) and call clearTimeout(timerId) in both onCanPlay and onError before removing listeners and resolving/rejecting, and also ensure the timeout handler clears listeners and only mutates this.autoplayBlocked/ rejects if it hasn't already settled.
🟡 Minor comments (11)
CONTRIBUTING.md-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorInconsistent capitalization in step heading.
Line 31 uses lowercase "clone the repository:" while line 29 uses title case "Fork the Repository" and all other steps (lines 38, 46, 54) also use title case. This creates an inconsistency in the documentation style.
📝 Proposed fix for consistent capitalization
-2. clone the repository: +2. Clone the Repository:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 31, Update the step heading "clone the repository:" to use Title Case to match the rest of the document — replace it with "Clone the Repository" (including capitalization and trailing colon) so it is consistent with headings like "Fork the Repository" elsewhere in CONTRIBUTING.md.DOCKER.md-117-120 (1)
117-120:⚠️ Potential issue | 🟡 MinorRemove unprofessional language from documentation.
Line 119 contains casual/unprofessional language that's inappropriate for public documentation. Consider rephrasing to maintain a professional tone.
Proposed fix
#### PocketBase Schema Note -If you are setting up a new PocketBase collection for user data, ensure it has a field named `firebase_id` (this is a legacy name we use when we first started the accounts system, we used firebase. and im too lazy to change it so yea fuck you). +If you are setting up a new PocketBase collection for user data, ensure it has a field named `firebase_id`. This is a legacy field name from when the project originally used Firebase for authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DOCKER.md` around lines 117 - 120, The DOCKER.md PocketBase Schema Note contains unprofessional language; update the paragraph that mentions the `firebase_id` field to remove profanity and casual phrasing and replace it with a concise, professional sentence such as explaining that `firebase_id` is a legacy field name retained from an earlier Firebase-based accounts system and should remain present for compatibility; ensure you edit the sentence referencing `firebase_id` only and preserve the technical guidance about adding that field.README.md-49-50 (1)
49-50:⚠️ Potential issue | 🟡 MinorRemove the duplicate
alt/widthattributes on these screenshot tags.Both new
<img>elements declarealtandwidthtwice. That makes the README HTML invalid, and one of the values will be ignored by the renderer, so the intended accessible label/size is ambiguous.Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 49 - 50, The README contains <img> elements with duplicate alt and width attributes (e.g., the tag shown in the diff); remove the repeated attributes so each <img> has a single alt and a single width attribute with the intended values (retain the correct accessible text and desired width), and apply the same fix to the other affected <img> occurrences noted in the comment (lines 55-56).js/metadata.flac.js-302-308 (1)
302-308:⚠️ Potential issue | 🟡 MinorUse UTC year extraction or parse the year string directly to avoid timezone issues.
new Date('2024-01-01').getFullYear()returns the year in local time, not UTC. Since ISO date-only strings are parsed as UTC midnight, this can return the wrong year in negative UTC offsets (e.g.,2023for'2024-01-01'in UTC−05:00), causing incorrectDATEtags for year-boundary releases. UsegetUTCFullYear()or parse the year directly from the string.Suggested change
- const year = new Date(releaseDateStr).getFullYear(); - if (!isNaN(year)) { - comments.push(['DATE', String(year)]); - } + const year = /^(\d{4})/.exec(releaseDateStr)?.[1]; + if (year) { + comments.push(['DATE', year]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/metadata.flac.js` around lines 302 - 308, The DATE tag extraction uses new Date(releaseDateStr).getFullYear(), which can yield wrong years across timezones; update the logic around releaseDateStr to use UTC-based year extraction (e.g., new Date(releaseDateStr).getUTCFullYear()) or parse the year directly from the string (e.g., take the first 4 digits of releaseDateStr) before pushing to comments (the block that computes year and calls comments.push(['DATE', String(year)])). Ensure you preserve the existing guard (!isNaN(year)) when using the new method.styles.css-3139-3156 (1)
3139-3156:⚠️ Potential issue | 🟡 Minor
--background-secondaryis undefined here.That token is not declared in the theme variables shown in this stylesheet, so this
backgrounddeclaration is dropped and the loading pill falls back to transparent over artwork/video.💡 Proposed fix
- background: var(--background-secondary); + background: var(--card);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles.css` around lines 3139 - 3156, The CSS uses an undefined token --background-secondary in the `#radio-loading-indicator` rule so the background is dropped; fix by either adding a --background-secondary entry to the theme variables (where other --* tokens are declared) with the intended pill background color or update the `#radio-loading-indicator` background to use an existing declared token or explicit non-transparent color so the loading pill renders over artwork/video.js/storage.js-832-844 (1)
832-844:⚠️ Potential issue | 🟡 MinorReturn a real default when the dim value is invalid.
parseFloat()can returnNaN, andgetDimAmount()currently hands that through instead of falling back to1.0. A stale or corrupted localStorage value will leakNaNinto the visualizer dimming path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/storage.js` around lines 832 - 844, getDimAmount() currently returns parseFloat(...) which can be NaN for corrupted/stale localStorage values; change getDimAmount to validate the parsed value (e.g., const n = parseFloat(val); if (!Number.isFinite(n)) return 1.0;) so it falls back to 1.0 instead of leaking NaN into the visualizer; also ensure setDimAmount(value) stores a string (e.g., String(value)) to keep localStorage consistent.js/api.js-1586-1596 (1)
1586-1596:⚠️ Potential issue | 🟡 MinorPreserve already-resolved artist image URLs.
getCoverUrl()andgetVideoCoverUrl()pass throughhttpURLs, butgetArtistPictureUrl()no longer does. Ifartist.pictureis already a CDN URL, this rewrites it into a brokenresources.tidal.com/images/...path.Suggested fix
- if (typeof id === 'string' && (id.startsWith('blob:') || id.startsWith('assets/'))) { + if ( + typeof id === 'string' && + (id.startsWith('http') || id.startsWith('blob:') || id.startsWith('assets/')) + ) { return id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/api.js` around lines 1586 - 1596, getArtistPictureUrl currently rewrites any non-blob/non-assets id into a resources.tidal.com URL which breaks when id is already a full CDN/http(s) URL; update getArtistPictureUrl to early-return the id unchanged when it's already a resolved URL (e.g., starts with "http://" or "https://" or "data:") in addition to the existing blob:/assets checks, otherwise continue formatting with replace and building the resources.tidal.com path.js/metadata.mp4.js-600-605 (1)
600-605:⚠️ Potential issue | 🟡 Minor
toBigEndianBytes()advertisesBigInt, but rejects everyBigIntinput.The JSDoc states the parameter accepts
number|BigInt, and the example demonstrates a BigInt literal (0xDEADBEEFn). However, the function usesNumber.isSafeInteger(value)which always returnsfalseforbiginttypes, causing it to throw on documented input. Either implement abigintbranch or narrow the contract tonumber.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/metadata.mp4.js` around lines 600 - 605, toBigEndianBytes currently calls Number.isSafeInteger which rejects BigInt inputs despite the JSDoc; update the function to handle both number and BigInt (or narrow the documented type). Specifically, in toBigEndianBytes(value, byteLength) branch on typeof value: if number, validate non-negative and Number.isSafeInteger; if bigint, validate value >= 0n and convert using BigInt-aware shifts/masks (or by repeatedly taking value & 0xFFn) to produce a big-endian Uint8Array that respects byteLength (throw if value cannot fit). Replace the single Number.isSafeInteger check with these two branches and keep error messages and byteLength validation consistent.js/ui.js-4233-4236 (1)
4233-4236:⚠️ Potential issue | 🟡 MinorInconsistent type for video like state.
Line 4235 calls
updateLikeState(el, 'track', video.id)for video items, but videos should use'video'type for consistency with other parts of the code (e.g., line 1708 inrenderLibraryPage).if (el) { trackDataStore.set(el, video); - this.updateLikeState(el, 'track', video.id); + this.updateLikeState(el, 'video', video.id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 4233 - 4236, The call to updateLikeState is using the wrong type for video items; in the block that sets trackDataStore.set(el, video) you should call updateLikeState(el, 'video', video.id) instead of 'track' so video like state is consistent with other usages (e.g., renderLibraryPage). Locate the occurrence of updateLikeState in this snippet and replace the second argument from 'track' to 'video' for video objects; keep trackDataStore.set(el, video) as-is.js/ui.js-1586-1588 (1)
1586-1588:⚠️ Potential issue | 🟡 MinorMemory leak: Event listener on
activeElementis never removed.The
volumechangeevent listener is added tothis.player.activeElementbut is never cleaned up when fullscreen is closed or when the active element changes (e.g., switching between audio and video playback). This can cause:
- Listener accumulation on repeated fullscreen opens
- Stale listeners on previous active elements
Suggested fix
+ // Store reference for cleanup + this._fsVolumeChangeHandler = updateFsVolumeUI; + this._fsVolumeElement = this.player.activeElement; - this.player.activeElement.addEventListener('volumechange', updateFsVolumeUI); + this._fsVolumeElement.addEventListener('volumechange', this._fsVolumeChangeHandler); updateFsVolumeUI(); }Then in
closeFullscreenCover:if (this.visualizer) { this.visualizer.stop(); } + + // Cleanup volume change listener + if (this._fsVolumeElement && this._fsVolumeChangeHandler) { + this._fsVolumeElement.removeEventListener('volumechange', this._fsVolumeChangeHandler); + this._fsVolumeElement = null; + this._fsVolumeChangeHandler = null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 1586 - 1588, The volumechange listener added to this.player.activeElement using updateFsVolumeUI must be removed to avoid leaks: store the handler reference (e.g., a bound function or a stored variable like this._fsVolumeHandler) when attaching in the fullscreen/open code path (where updateFsVolumeUI is added) and call removeEventListener(this.player.activeElement, this._fsVolumeHandler) inside closeFullscreenCover and any code paths that swap active elements (e.g., when switching playback) before re-attaching; ensure you clear the stored handler reference after removal so subsequent opens attach a fresh listener.js/ui.js-2650-2698 (1)
2650-2698:⚠️ Potential issue | 🟡 MinorPotential memory leak and infinite loop risk in HLS video setup.
Two concerns with this implementation:
Memory leak: The HLS instance stored at
video._hls(line 2658) is never explicitly destroyed when the video element is removed from the DOM. When videos are replaced (e.g., navigating between pages), HLS instances accumulate.Infinite loop risk: Line 2691-2697 recursively calls
setupHlsVideowhenresult.hlsUrlexists, butresultcan be a string (line 2652-2653). If the initial URL fails andresultis a string, checkingresult.hlsUrlwill beundefined, which is safe. However, if bothvideoUrlandhlsUrlfail sequentially, there's no guard against repeated failures.Proposed fix for HLS cleanup
setupHlsVideo(video, result, fallbackImg) { if (!result) return; + // Cleanup any existing HLS instance + if (video._hls) { + video._hls.destroy(); + video._hls = null; + } const url = typeof result === 'string' ? result : result.videoUrl || result.hlsUrl; if (!url) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 2650 - 2698, In setupHlsVideo, ensure Hls instances are explicitly destroyed to avoid leaks and prevent recursive retry loops: when attaching an Hls instance (hls assigned to video._hls) register cleanup to call hls.destroy() and delete video._hls when the video is removed from DOM or when replacing with fallbackImg (also remove Hls event listeners), and in the video.onerror / Hls.Events.ERROR fallback logic avoid unbounded recursion by tracking retries (e.g., pass a second parameter like triedHls boolean or compare the current url to the fallback so you only retry once) and handle the case when result is a string by resolving hlsUrl safely before attempting retry; update references to setupHlsVideo, video._hls, Hls.Events.ERROR, and video.onerror accordingly.
🧹 Nitpick comments (11)
js/tracker.js (1)
108-135: Consider adding a timeout to prevent indefinite hangs on unresponsive endpoints.The multi-endpoint fallback pattern is well-implemented. However,
fetchrequests without a timeout can hang indefinitely if an endpoint is slow or unresponsive, delaying the fallback to subsequent endpoints and degrading user experience.♻️ Proposed fix using AbortController with timeout
async function fetchTrackerData(sheetId) { const endpoints = [ 'https://tracker.israeli.ovh/get/', 'https://tracker.thug.surf/get/', 'https://trackerapi-2.artistgrid.cx/get/', ]; + const TIMEOUT_MS = 5000; let lastError = null; for (const baseUrl of endpoints) { try { - const response = await fetch(`${baseUrl}${sheetId}`); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS); + const response = await fetch(`${baseUrl}${sheetId}`, { signal: controller.signal }); + clearTimeout(timeoutId); if (!response.ok) { lastError = new Error(`HTTP ${response.status}`); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/tracker.js` around lines 108 - 135, fetchTrackerData can hang on slow/unresponsive endpoints because fetch has no timeout; wrap each fetch call with an AbortController-based timeout (e.g., create an AbortController per endpoint, start a setTimeout that calls controller.abort() after X ms, pass controller.signal to fetch, and clear the timeout on success) so the loop moves to the next baseUrl promptly on timeout; ensure you treat an abort as an error (set lastError and continue) and keep existing behavior of parsing JSON, calling transformErasImages(data.eras) and returning data on success.INSTANCES.md (1)
51-65: Use one canonical provider name per operator across the document.This table introduces naming drift with the other sections, e.g.
Squid.WTFvssquid.wtfand Binimum/bini branding. Normalizing those labels will make cross-referencing instances and APIs much less ambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@INSTANCES.md` around lines 51 - 65, Standardize provider naming across the document by choosing one canonical label for each operator (e.g., “Monochrome”, “Squid.WTF” instead of “squid.wtf”, “Lucida (QQDL)”, “Spotisaver”, “Kinoplus”, and “Binimum”/“bini”) and apply it everywhere the provider appears; update the table rows (the Provider column entries for Monochrome, squid.wtf, Lucida (QQDL), Spotisaver, Kinoplus, Binimum) to use that chosen canonical capitalization/branding and then sweep the rest of the document to replace any alternate variants so all references (including headings, cross-references, and instance lists) consistently use the same names while leaving the URLs unchanged.tsconfig.json (1)
9-11: Consider documenting the!/*path alias to prevent confusion.The
!/*path alias is intentional and used for importing WASM modules fromnode_modules(e.g., injs/taglib.ts). However, this unconventional pattern lacks explanation. Add a comment intsconfig.jsonclarifying its purpose:"paths": { "!/*": ["node_modules/*"] // Used to explicitly import assets from node_modules (e.g., WASM files) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tsconfig.json` around lines 9 - 11, Add a clarifying comment next to the custom path alias "!/*" in tsconfig.json explaining its purpose: that the "!/*" mapping points to node_modules/* and is intentionally used to import assets (e.g., WASM files) from node_modules (see imports like js/taglib.ts); update the "paths" entry for "!/*" to include that brief inline comment so future readers aren’t confused by this unconventional alias.public/reset-password.html (1)
153-164: Add explicit labels and password-manager hints to these fields.These inputs currently rely on placeholders only. Real
<label>elements plusautocomplete="new-password"make the reset flow much more reliable for screen readers and password managers.♿ Suggested markup
- <div class="form-group"> - <input type="password" id="password" placeholder="New Password" required minlength="8" /> - </div> - <div class="form-group"> + <div class="form-group"> + <label for="password">New password</label> + <input + type="password" + id="password" + placeholder="New Password" + required + minlength="8" + autocomplete="new-password" + /> + </div> + <div class="form-group"> + <label for="password-confirm">Confirm new password</label> <input type="password" id="password-confirm" placeholder="Confirm New Password" required minlength="8" + autocomplete="new-password" /> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/reset-password.html` around lines 153 - 164, The password fields inside the form with id "reset-form" use placeholders only; add explicit <label> elements associated with the inputs (use for="password" and for="password-confirm") and set autocomplete="new-password" on both inputs to help screen readers and password managers; ensure the input ids remain "password" and "password-confirm" so the labels target them correctly and keep required and minlength attributes as-is.js/visualizers/lcd.js (1)
271-274: Reasonable fix for visual bugs.Removing the forced
position: relativeon the parent element is appropriate if the container is already positioned via CSS (e.g.,#visualizer-container). The WebGL overlay'sposition: absolute(line 32) will still work correctly with any positioned ancestor.Consider deleting the commented code entirely if the fix is confirmed stable, or add a brief note about which CSS rule provides the necessary positioning for future reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/visualizers/lcd.js` around lines 271 - 274, The commented-out line that set canvas.parentElement.style.position = 'relative' in the block that appends this.glCanvas (inside js/visualizers/lcd.js where this.glCanvas and canvas.parentElement are used) can be removed to clean up the code now that the parent container is positioned via CSS; delete the commented line or replace it with a one-line comment noting which external CSS rule (e.g., `#visualizer-container` { position: relative; }) provides the required positioning so the WebGL overlay (position: absolute configured earlier) continues to work reliably.js/qobuz-api.js (1)
4-5: Please remove the profanity from the file header comment.This is likely to end up in shipped source/sourcemaps and makes the module harder to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/qobuz-api.js` around lines 4 - 5, The file header contains a profane comment ("LMFAOOOO this shit is useless now qobuz killing accounts every time a dude takes their breath"); remove that profanity and replace it with a neutral, descriptive header (e.g., "Qobuz API client" or brief note about why the file exists). Edit the top-of-file comment in js/qobuz-api.js to be professional and maintainable (no profanity or slang), preserving any useful context or author/date information if needed.js/taglib.types.ts (1)
9-13: MakeTagLibWorkerResponsea discriminated union by extracting the message type into a generic parameter.Currently,
TagLibFileResponse | TagLibMetadataResponseboth declaretype: 'Add' | 'Get'on each side, so TypeScript cannot narrow the union when checkingresponse.type. This defeats the type safety goal. With distinct type discriminants per variant (e.g.,TagLibFileResponsehastype: 'Add'only whileTagLibMetadataResponsehastype: 'Get'only), checkingresponse.type === 'Add'will narrow the union to the correct variant and allowdatato be narrowed to the correct payload type.Suggested change
-export interface TagLibWorkerResponse<T> { - type: TagLibWorkerMessageType; - data?: T; +export interface TagLibWorkerResponse<TType extends TagLibWorkerMessageType, TData> { + type: TType; + data?: TData; error?: string; } @@ -export type TagLibFileResponse = TagLibWorkerResponse<Uint8Array>; -export type TagLibMetadataResponse = TagLibWorkerResponse<TagLibReadMetadata>; +export type TagLibFileResponse = TagLibWorkerResponse<'Add', Uint8Array>; +export type TagLibMetadataResponse = TagLibWorkerResponse<'Get', TagLibReadMetadata>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/taglib.types.ts` around lines 9 - 13, TagLibWorkerResponse currently uses a broad TagLibWorkerMessageType so TypeScript cannot narrow when checking response.type; change TagLibWorkerResponse into a discriminated generic (e.g., TagLibWorkerResponse<T, Type extends TagLibWorkerMessageType>) or redefine the concrete variants TagLibFileResponse and TagLibMetadataResponse so each has a literal type field ('Add' for TagLibFileResponse, 'Get' for TagLibMetadataResponse) and corresponding data shape, update usages to use the specific variant types so checking response.type === 'Add' or 'Get' narrows the union and allows data to be correctly inferred.js/music-api.js (1)
169-189: Add a timeout around the third-party artwork fetch.This new
artwork.boidu.devcall has no abort path, so a slow or half-open connection can stall the video artwork path for a long time. Bound it with a timeout and fail fast.💡 Proposed fix
async getVideoArtwork(title, artist) { const cacheKey = `${title}-${artist}`.toLowerCase(); if (this.videoArtworkCache.has(cacheKey)) { return this.videoArtworkCache.get(cacheKey); } + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); try { const url = `https://artwork.boidu.dev/?s=${encodeURIComponent(title)}&a=${encodeURIComponent(artist)}`; - const response = await fetch(url); + const response = await fetch(url, { signal: controller.signal }); if (!response.ok) return null; const data = await response.json(); const result = { videoUrl: data.videoUrl || null, hlsUrl: data.animated || null, }; this.videoArtworkCache.set(cacheKey, result); return result; } catch (error) { console.warn('Failed to fetch video artwork:', error); return null; + } finally { + clearTimeout(timeoutId); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/music-api.js` around lines 169 - 189, The fetch to artwork.boidu.dev in getVideoArtwork has no timeout—wrap the fetch in an AbortController and set a timer to call controller.abort() after a reasonable timeout (e.g., 2–5s), pass controller.signal to fetch, clear the timer on success, and handle the abort/error by returning null (keep the existing try/catch and logging). Use the existing cacheKey and this.videoArtworkCache logic unchanged; ensure the timer is cleaned up in both success and error paths to avoid leaks.js/BaseCodec.ts (1)
117-119: VerifySet.prototype.isSubsetOfagainst your Capacitor target matrix.
decodeNumber()uses a very new Set API. TypeScript will type-check this fine (tsconfig includes "ESNext" lib), but runtime support is not guaranteed. Capacitor apps running on older Android WebView versions (pre-Chrome 122) or older Safari instances will fail at runtime. A plainevery()check avoids this compatibility edge.💡 Compatibility-safe alternative
- if (new Set(str).isSubsetOf(this.dictionarySet) === false) { + if (![...str].every((char) => this.dictionarySet.has(char))) { throw new Error('Input contains invalid characters.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/BaseCodec.ts` around lines 117 - 119, The runtime error stems from using the non-standard Set.prototype.isSubsetOf; in the decodeNumber (or the surrounding method in BaseCodec) replace new Set(str).isSubsetOf(this.dictionarySet) with a compatibility-safe subset check such as iterating the set or using Array.from(new Set(str)).every(ch => this.dictionarySet.has(ch)) so the logic and thrown Error('Input contains invalid characters.') remain identical; update the check in the method that references this.dictionarySet (e.g., decodeNumber) to use this approach.js/ui.js (2)
1636-1656: Code quality: Redundant condition and loose equality.The
downloadsdisabledconstant is alwaystrue, making theif (downloadsdisabled == true)check redundant. Additionally, loose equality (==) is used instead of strict equality (===).If this is a feature flag intended for future toggling, consider:
Suggested improvement
- const downloadsdisabled = true; - if (downloadsdisabled == true) { + const isDownloadsDisabled = true; // Feature flag + if (isDownloadsDisabled) { if (pageId === 'download') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 1636 - 1656, The downloadsdisabled constant is hardcoded true and checked with loose equality; remove the redundant if (downloadsdisabled == true) block or make downloadsdisabled a configurable flag and use strict equality (===) when checking it; specifically, update the code around the downloadsdisabled declaration and the conditional that wraps the pageId === 'download' logic (referencing downloadsdisabled, pageId, maintenance-modal, maintenance-home-btn, and navigate) so the check is either eliminated or becomes if (downloadsdisabled === true) with downloadsdisabled driven by configuration/state.
1699-1720: Missing navigation for video cards in library.Video card click handlers only handle the play button click (lines 1709-1714). Unlike other card types (albums, playlists) that navigate to a detail page when clicking outside the play button, video cards have no navigation behavior.
Consider adding navigation to a video detail page or consistent behavior:
el.addEventListener('click', (e) => { if (e.target.closest('.card-play-btn') || e.target.closest('.card-image-container')) { e.stopPropagation(); this.player.playVideo(video); + } else if (!e.target.closest('.like-btn')) { + // Navigate to video page or play video + this.player.playVideo(video); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/ui.js` around lines 1699 - 1720, Video cards currently only handle play actions (player.playVideo) and never navigate to a detail view; modify the click handling for video card elements (located where createVideoCardHTML results are inserted and trackDataStore.set is called) so that when a click occurs outside the play button/image (same selection logic using e.target.closest('.card-play-btn') || e.target.closest('.card-image-container')), you perform navigation to the video detail (call an existing router/navigation method or add a new method like this.navigateToVideoDetail(video.id) or this.openVideoDetail(video) ), while keeping the existing stopPropagation/playVideo behavior intact; ensure you attach this navigation logic in the same loop that sets up the click listener for each video element and do not remove the current updateLikeState or trackDataStore.set calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b5796de-6291-46f2-871e-82884e1d15db
⛔ Files ignored due to path filters (18)
android/app/src/main/ic_launcher-playstore.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonpublic/lib/ffmpeg-core.wasmis excluded by!**/*.wasm
📒 Files selected for processing (74)
.devcontainer/Dockerfile.devcontainer/devcontainer.json.env.example.github/workflows/update-lockfile.yml.vscode/tasks.jsonAUTH_GATE.mdCONTRIBUTING.mdDESIGN.mdDOCKER.mdINSTANCES.mdREADME.mdandroid/app/capacitor.build.gradleandroid/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlandroid/app/src/main/res/values/ic_launcher_background.xmlandroid/capacitor.settings.gradlecapacitor.config.jsondocker-compose.ymlfunctions/album/[id].jsfunctions/playlist/[id].jsfunctions/track/[id].jsfunctions/upload.jsindex.htmljs/BaseCodec.tsjs/accounts/auth.jsjs/accounts/config.jsjs/accounts/pocketbase.jsjs/api.jsjs/app.jsjs/audio-context.jsjs/commandPalette.jsjs/db.jsjs/doTimed.tsjs/downloads.jsjs/events.jsjs/ffmpeg.jsjs/ffmpeg.worker.jsjs/global.d.tsjs/hls-downloader.jsjs/id3-writer.jsjs/lyrics.jsjs/metadata.flac.jsjs/metadata.jsjs/metadata.mp3.jsjs/metadata.mp4.jsjs/mobile/capacitor.jsjs/mobile/discord-rpc.jsjs/music-api.jsjs/player.jsjs/profile.jsjs/qobuz-api.jsjs/settings.jsjs/side-panel.jsjs/storage.jsjs/taglib.tsjs/taglib.types.tsjs/taglib.worker.tsjs/tracker.jsjs/ui-interactions.jsjs/ui.jsjs/utils.jsjs/visualizer.jsjs/visualizers/lcd.jslegacy.htmlpackage.jsonpublic/editors-picks.jsonpublic/lib/ffmpeg-core.jspublic/login.htmlpublic/reset-password.htmlself-hosted-database.mdstyles.csstsconfig.jsonvite-plugin-auth-gate.jsvite-plugin-upload.jsvite.config.js
💤 Files with no reviewable changes (5)
- AUTH_GATE.md
- public/login.html
- self-hosted-database.md
- DESIGN.md
- js/id3-writer.js
| - name: Commit changes | ||
| run: | | ||
| git add -A . || true | ||
| git commit "update lockfile" || true |
There was a problem hiding this comment.
Critical: git commit syntax error.
The git commit command is missing the -m flag, causing it to fail or open an interactive editor (which will hang in CI).
Proposed fix
- git commit "update lockfile" || true
+ git commit -m "update lockfile" || true📝 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.
| git commit "update lockfile" || true | |
| git commit -m "update lockfile" || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/update-lockfile.yml at line 42, The git commit invocation
currently written as git commit "update lockfile" is missing the -m flag;
replace it with git commit -m "update lockfile" (optionally add --allow-empty if
you want to avoid failing when there are no changes) and keep the existing ||
true if you want the step to never fail; update the command that contains the
string git commit "update lockfile" accordingly.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "appId": "com.monochrome.app", | |||
| "appId": "tf.monochrome.android", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify appId consistency across Android configuration files
echo "=== Checking Android build.gradle for applicationId ==="
fd -e gradle -x grep -l "applicationId" {} \; | xargs -I{} sh -c 'echo "File: {}"; grep -n "applicationId" {}'
echo ""
echo "=== Checking AndroidManifest.xml for package name ==="
fd "AndroidManifest.xml" -x sh -c 'echo "File: {}"; grep -n "package=" {}'Repository: subby2006/monochrome-android
Length of output: 310
🏁 Script executed:
cat ./android/app/src/main/AndroidManifest.xml | head -5Repository: subby2006/monochrome-android
Length of output: 233
🏁 Script executed:
rg "com\.monochrome\.app|tf\.monochrome\.android" --type xml --type gradle --type jsonRepository: subby2006/monochrome-android
Length of output: 552
Update Android configuration files to match the appId change.
The appId in capacitor.config.json has been changed to "tf.monochrome.android", but Android configuration files still reference the old value "com.monochrome.app". This mismatch will cause build failures and runtime issues.
Update the following files to use the new appId:
android/app/build.gradle(bothnamespaceandapplicationId)android/app/src/main/res/values/strings.xml(package_nameandcustom_url_scheme)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@capacitor.config.json` at line 2, The Android configs still use the old
package id; update all Android references to the new appId
"tf.monochrome.android": in android/app/build.gradle update the namespace and
applicationId entries to "tf.monochrome.android", and in
android/app/src/main/res/values/strings.xml change the string values for
package_name and custom_url_scheme to match "tf.monochrome.android" (or the
appropriate URL scheme based on that id) so all identifiers match
capacitor.config.json.
| async function hmac(key, data) { | ||
| const cryptoKey = await crypto.subtle.importKey('raw', key, { name: 'HMAC', hash: 'SHA-1' }, false, ['sign']); | ||
| return new Uint8Array(await crypto.subtle.sign('HMAC', cryptoKey, data)); | ||
| } | ||
|
|
||
| async function sha256(data) { | ||
| return new Uint8Array(await crypto.subtle.digest('SHA-256', data)); | ||
| } | ||
|
|
||
| function buf2hex(buffer) { | ||
| return Array.from(buffer) | ||
| .map((b) => b.toString(16).padStart(2, '0')) | ||
| .join(''); | ||
| } | ||
|
|
||
| async function signature(secretKey, dateStamp, region, service, stringToSign) { | ||
| const kDate = await hmac(new TextEncoder().encode('AWS4' + secretKey), new TextEncoder().encode(dateStamp)); | ||
| const kRegion = await hmac(kDate, new TextEncoder().encode(region)); | ||
| const kService = await hmac(kRegion, new TextEncoder().encode(service)); | ||
| const kSigning = await hmac(kService, new TextEncoder().encode('aws4_request')); | ||
| const sig = await hmac(kSigning, new TextEncoder().encode(stringToSign)); | ||
| return buf2hex(sig); | ||
| } | ||
|
|
||
| async function createSignature(method, path, headers, payloadHash, accessKeyId, secretAccessKey, amzDate, dateStamp) { | ||
| const region = 'auto'; | ||
| const service = 's3'; | ||
|
|
||
| const signedHeaders = Object.keys(headers).sort().join(';'); | ||
| const canonicalHeaders = | ||
| Object.entries(headers) | ||
| .sort((a, b) => a[0].localeCompare(b[0])) | ||
| .map(([k, v]) => `${k.toLowerCase()}:${v}`) | ||
| .join('\n') + '\n'; | ||
|
|
||
| const canonicalRequest = `${method}\n${path}\n\n${canonicalHeaders}\n${signedHeaders}\n${payloadHash}`; | ||
| const credentialScope = `${dateStamp}/${region}/${service}/aws4_request`; | ||
| const stringToSign = `AWS4-HMAC-SHA256\n${amzDate}\n${credentialScope}\n${buf2hex(await sha256(new TextEncoder().encode(canonicalRequest)))}`; | ||
|
|
||
| const sig = await signature(secretAccessKey, dateStamp, region, service, stringToSign); | ||
| return `AWS4-HMAC-SHA256 Credential=${accessKeyId}/${credentialScope}, SignedHeaders=${signedHeaders}, Signature=${sig}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For AWS Signature Version 4 on S3/Cloudflare R2, which hash algorithm is used for HMAC key derivation, and how must object keys with spaces or reserved URL characters be encoded in the canonical URI?
💡 Result:
- HMAC key derivation hash algorithm (SigV4 / S3 / R2): HMAC-SHA256 (algorithm ID
AWS4-HMAC-SHA256). (docs.aws.amazon.com) - Canonical URI encoding for object keys (spaces / reserved characters): Percent-encode the object key in the canonical URI by URI-encoding every byte except
A–Z a–z 0–9 - . _ ~; encode spaces as%20(not+); use uppercase hex in escapes (e.g.%1A). Keep/unencoded only as the path delimiter within the object key (don’t encode the slashes that separate “folders” in the key). (docs.aws.amazon.com)
Citations:
- 1: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-signing-elements.html
- 2: https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
🏁 Script executed:
cat -n functions/upload.js | head -150Repository: subby2006/monochrome-android
Length of output: 6968
Fix the HMAC hash algorithm and percent-encode object keys in the SigV4 implementation.
Line 8: hmac() uses hash: 'SHA-1' but AWS Signature Version 4 requires SHA-256. This will cause all R2 signatures to be rejected.
Additionally, filenames with spaces or reserved URL characters (e.g., spaces, !, ?) must be percent-encoded in the canonical URI per AWS SigV4 rules. Line 97 builds the object key from the raw fileName, and line 121 passes it unencoded to createSignature(). Encoding must follow the S3 rules: percent-encode all bytes except A–Z a–z 0–9 - . _ ~, encode spaces as %20 (not +), and use uppercase hex.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/upload.js` around lines 7 - 47, The HMAC implementation and URI
handling are incorrect: update hmac() to use SHA-256 (change importKey/hash to
'SHA-256') so all derived keys (kDate/kRegion/kService/kSigning) and final HMAC
use SHA-256; then percent-encode the object key before building the
canonicalRequest/path and before calling createSignature() (follow S3 SigV4
rules: percent-encode all bytes except A–Z a–z 0–9 - . _ ~, encode spaces as %20
not +, and use uppercase hex) so canonicalHeaders/canonicalRequest in
createSignature(), the path argument, and the actual request URL use the encoded
key consistently. Ensure buf2hex produces uppercase hex if used for
percent-encoding output where required.
| _connectGraph() { | ||
| if (!this.source || !this.audioContext) return; | ||
| if (!this.isInitialized || !this.source || !this.audioContext) return; | ||
|
|
There was a problem hiding this comment.
Initialize the manager before calling _connectGraph().
init() still invokes _connectGraph() before this.isInitialized = true, so this new guard bails out on the first setup path and leaves the initial media graph disconnected until some later resume/toggle path.
Suggested fix
- this._connectGraph();
-
- this.isInitialized = true;
+ this.isInitialized = true;
+ this._connectGraph();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/audio-context.js` around lines 383 - 385, The guard in _connectGraph that
checks this.isInitialized prevents initial setup because init() calls
_connectGraph before setting this.isInitialized = true; update init() so that it
sets this.isInitialized = true (or otherwise marks the manager initialized)
before calling _connectGraph, ensuring the initial media graph is connected on
first setup; alternatively remove or adjust the isInitialized check in
_connectGraph if intended to allow initial invocation—refer to the init() method
and _connectGraph() function to make the change.
| // Count leading zeros | ||
| let zeroCount = 0; | ||
| while (zeroCount < bytes.length && bytes[zeroCount] === 0) zeroCount++; | ||
|
|
||
| const digits: string[] = []; | ||
| let inputArray = Array.from(bytes); | ||
|
|
||
| while (inputArray.length > 0 && !(inputArray.length === 1 && inputArray[0] === 0)) { | ||
| const newInput: number[] = []; | ||
| let remainder = 0; | ||
|
|
||
| for (const byte of inputArray) { | ||
| const acc = (remainder << 8) + byte; | ||
| const digit = Math.floor(acc / this.base); | ||
| remainder = acc % this.base; | ||
| if (newInput.length > 0 || digit !== 0) newInput.push(digit); | ||
| } | ||
|
|
||
| digits.push(this.dictionary[remainder]); | ||
| inputArray = newInput; | ||
| } | ||
|
|
||
| for (let i = 0; i < zeroCount; i++) digits.push(this.dictionary[0]); | ||
|
|
||
| return digits.reverse().join(''); |
There was a problem hiding this comment.
Leading-zero payloads do not round-trip correctly.
After zeroCount is computed, both methods still feed the full zero-prefixed input through the division loop. For example, an all-zero byte array gets an extra zero symbol on encode, and an all-zero encoded string decodes to too many zero bytes. Slice off the counted prefix before the loop in both directions.
💡 Proposed fix
- const digits: string[] = [];
- let inputArray = Array.from(bytes);
+ const digits: string[] = [];
+ let inputArray = Array.from(bytes.subarray(zeroCount));
@@
- const bytes: number[] = [];
- let inputArray = Array.from(input, (c) => {
+ const bytes: number[] = [];
+ let inputArray = Array.from(input.slice(zeroCount), (c) => {
const v = charToValue[c];
if (v === undefined) throw new Error(`Invalid character: ${c}`);
return v;
});Also applies to: 132-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/BaseCodec.ts` around lines 77 - 101, In the BaseCodec encode/decode
routines (the methods that compute zeroCount and use inputArray/digits), you're
still processing the leading-zero prefix through the division loop which causes
extra zeros; after computing zeroCount slice off the counted-zero prefix (e.g.,
set inputArray = Array.from(bytes.slice(zeroCount)) before the division loop in
the encode method) and do the analogous slice on the encoded string input before
its division loop in decode so only the non-zero remainder is processed and then
prepend the counted zero symbols using this.dictionary[0]; update the blocks
around zeroCount, inputArray, and digits in both methods accordingly.
| // Create or update Vorbis comment block | ||
| const vorbisCommentBlock = createVorbisCommentBlock(track); |
There was a problem hiding this comment.
Pass encoded comments, not the raw track object.
createVorbisCommentBlock() expects an array of [key, value] tuples, but track is passed directly here. That throws on comments.map(...), so FLAC tagging falls back to the original blob every time.
Suggested change
- const vorbisCommentBlock = createVorbisCommentBlock(track);
+ const vorbisCommentBlock = createVorbisCommentBlock(createVorbisComments(track));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/metadata.flac.js` around lines 151 - 152, createVorbisCommentBlock is
being passed the raw track object but expects an array of [key, value] tuples,
causing the comments.map(...) exception; fix by passing the encoded comments
array instead (e.g. call your encoder to produce an array of [key, value] tuples
such as encodeVorbisComments(track) or map track.comments to [key, value] pairs)
so replace createVorbisCommentBlock(track) with
createVorbisCommentBlock(<encodedCommentsArray>) where <encodedCommentsArray> is
the result of the function that converts track into the required tuples.
| for (let i = 0; i < filteredBlocks.length; i++) { | ||
| const block = filteredBlocks[i]; | ||
| const isLast = false; // We'll add more blocks | ||
|
|
||
| // Write block header | ||
| const header = (isLast ? 0x80 : 0x00) | block.type; | ||
| newFile[offset++] = header; | ||
| newFile[offset++] = (block.size >> 16) & 0xff; | ||
| newFile[offset++] = (block.size >> 8) & 0xff; | ||
| newFile[offset++] = block.size & 0xff; | ||
|
|
||
| // Write block data | ||
| newFile.set(originalArray.subarray(block.offset, block.offset + block.size), offset); | ||
| offset += block.size; | ||
| } | ||
|
|
||
| let lastBlockHeaderOffset = offset; | ||
|
|
||
| if (vorbisCommentBlock) { | ||
| // Write new Vorbis comment block | ||
| const vorbisHeaderOffset = offset; | ||
| const vorbisHeader = FLAC_BLOCK_TYPES.VorbisComment; // Vorbis comment type | ||
| newFile[offset++] = vorbisHeader; | ||
| newFile[offset++] = (vorbisCommentBlock.length >> 16) & 0xff; | ||
| newFile[offset++] = (vorbisCommentBlock.length >> 8) & 0xff; | ||
| newFile[offset++] = vorbisCommentBlock.length & 0xff; | ||
| newFile.set(vorbisCommentBlock, offset); | ||
| offset += vorbisCommentBlock.length; | ||
| lastBlockHeaderOffset = vorbisHeaderOffset; | ||
| } | ||
|
|
||
| // Write picture block if available | ||
| if (pictureBlock) { | ||
| const pictureHeaderOffset = offset; | ||
| const pictureHeader = FLAC_BLOCK_TYPES.Picture; // Picture type | ||
| newFile[offset++] = pictureHeader; | ||
| newFile[offset++] = (pictureBlock.length >> 16) & 0xff; | ||
| newFile[offset++] = (pictureBlock.length >> 8) & 0xff; | ||
| newFile[offset++] = pictureBlock.length & 0xff; | ||
| newFile.set(pictureBlock, offset); | ||
| offset += pictureBlock.length; | ||
| lastBlockHeaderOffset = pictureHeaderOffset; | ||
| } | ||
|
|
||
| // Mark the last metadata block with the 0x80 flag | ||
| newFile[lastBlockHeaderOffset] |= 0x80; |
There was a problem hiding this comment.
Track the last metadata header, not the write cursor.
When both vorbisCommentBlock and pictureBlock are omitted, lastBlockHeaderOffset is left pointing at the first audio byte. Setting 0x80 there corrupts the stream and makes a correct metadata-stripping path impossible.
Suggested change
- let lastBlockHeaderOffset = offset;
+ let lastBlockHeaderOffset = null;
@@
for (let i = 0; i < filteredBlocks.length; i++) {
const block = filteredBlocks[i];
const isLast = false; // We'll add more blocks
// Write block header
+ const blockHeaderOffset = offset;
const header = (isLast ? 0x80 : 0x00) | block.type;
newFile[offset++] = header;
newFile[offset++] = (block.size >> 16) & 0xff;
newFile[offset++] = (block.size >> 8) & 0xff;
newFile[offset++] = block.size & 0xff;
@@
// Write block data
newFile.set(originalArray.subarray(block.offset, block.offset + block.size), offset);
offset += block.size;
+ lastBlockHeaderOffset = blockHeaderOffset;
}
@@
- newFile[lastBlockHeaderOffset] |= 0x80;
+ if (lastBlockHeaderOffset == null) {
+ throw new Error('Invalid FLAC file structure: no metadata block written');
+ }
+ newFile[lastBlockHeaderOffset] |= 0x80;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/metadata.flac.js` around lines 517 - 562, The code currently sets
lastBlockHeaderOffset only after writing filteredBlocks (initialized to offset)
and when adding vorbisCommentBlock/pictureBlock, so if both optional blocks are
absent lastBlockHeaderOffset points to the first audio byte and
newFile[lastBlockHeaderOffset] |= 0x80 corrupts the stream; fix by tracking the
header offset for each metadata block written in the filteredBlocks loop (inside
the for loop that writes header/data for filteredBlocks) — capture the header
start (e.g., const headerOffset = offset) before writing header bytes, set
lastBlockHeaderOffset = headerOffset after writing each filtered block header,
and keep the existing updates when adding vorbisCommentBlock or pictureBlock so
the final 0x80 flag is applied to the actual last metadata header.
| TagLibMetadata, | ||
| TagLibReadMetadata, | ||
| } from './taglib.types'; | ||
| import TagLibWorker from './taglib.worker?worker'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vite, what does import WorkerCtor from './taglib.worker?worker'return, and isnew Worker(new URL(WorkerCtor, import.meta.url), { type: 'module' })valid, or should it be instantiated asnew WorkerCtor()?
💡 Result:
import WorkerCtor from './taglib.worker?worker' returns a custom worker constructor (i.e., something you instantiate with new to get a Worker instance). (main.vitejs.dev)
So this is the valid usage:
import WorkerCtor from './taglib.worker?worker'
const worker = new WorkerCtor()new Worker(new URL(WorkerCtor, import.meta.url), { type: 'module' }) is not the right pattern here, because WorkerCtor is not a URL/string—it’s already the constructor. (main.vitejs.dev)
If you specifically want a URL to pass into new Worker(...), import with ?worker&url instead:
import workerUrl from './taglib.worker?worker&url'
const worker = new Worker(workerUrl, { type: 'module' })Citations:
- 1: https://main.vitejs.dev/guide/features
- 2: https://main.vitejs.dev/guide/features
- 3: https://main.vitejs.dev/guide/features
Use new TagLibWorker() directly instead of new Worker(new URL(...)).
The ?worker import returns a worker constructor, not a URL string. TagLibWorker from ./taglib.worker?worker should be instantiated as new TagLibWorker(), matching the pattern in addMetadataWithTagLib(). Using new Worker(new URL(TagLibWorker, import.meta.url), { type: 'module' }) is invalid because TagLibWorker is already the constructor, not a URL.
Suggested fix
- const worker = new Worker(new URL(TagLibWorker, import.meta.url), { type: 'module' });
+ const worker = new TagLibWorker();Also applies to: 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/taglib.ts` at line 13, The import TagLibWorker from
'./taglib.worker?worker' yields a worker constructor, so replace any uses of new
Worker(new URL(TagLibWorker, import.meta.url), { type: 'module' }) with a direct
instantiation new TagLibWorker(); update the instantiation sites (e.g., where
TagLibWorker is created and inside addMetadataWithTagLib) to call new
TagLibWorker() and remove the new URL(...) wrapper and explicit { type: 'module'
} option so the code uses the provided worker constructor correctly.
| <script type="module"> | ||
| import { Client, Account } from 'appwrite'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Appwrite imports / import maps =="
rg -n -C2 'from ["'"'"']appwrite["'"'"']|type="importmap"|appwrite.*(jsdelivr|unpkg|esm)' .
echo
echo "== Build config hints for public asset handling =="
rg -n -C2 'publicDir|copyPublicDir|public/' .
echo
echo "== References to reset-password.html =="
rg -n -C2 'reset-password\.html|/reset-password' .Repository: subby2006/monochrome-android
Length of output: 2532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full Vite config =="
cat vite.config.js
echo
echo "== Check package.json for appwrite =="
cat package.json | grep -A2 -B2 appwrite || echo "No appwrite found in direct grep"
echo
echo "== Look for import maps in all HTML files =="
rg -n 'importmap|preload.*appwrite' . -A5 -B5
echo
echo "== Check if reset-password.html exists and is part of build output =="
ls -la public/reset-password.html 2>/dev/null || echo "File exists"
echo
echo "== Look for any HTML template or processing of reset-password =="
rg -n 'reset-password' . --type html -B3 -A3Repository: subby2006/monochrome-android
Length of output: 3499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if reset-password.html is an entry point in vite config =="
rg -n 'reset-password|entry|input' vite.config.js
echo
echo "== Check if there's a separate build step or script =="
cat package.json | grep -A10 '"scripts"'
echo
echo "== Check index.html entry point =="
head -30 index.html
echo
echo "== Look for how js/accounts/config.js is used (to confirm bundling) =="
rg -n 'accounts/config' . --type js -B2 -A2
echo
echo "== Check if appwrite is directly available in window or globally =="
rg -n 'window\.appwrite|globalThis\.appwrite|<script.*appwrite' .Repository: subby2006/monochrome-android
Length of output: 139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Package.json scripts section =="
sed -n '/"scripts"/,/^[[:space:]]*}/p' package.json | head -20
echo
echo "== Check index.html (main entry) =="
head -50 index.html
echo
echo "== How is js/accounts/config.js imported =="
rg 'accounts/config' . --type js | head -5
echo
echo "== Check if appwrite is in window or imported globally =="
find . -name '*.html' -type f -exec grep -l 'appwrite' {} \;Repository: subby2006/monochrome-android
Length of output: 3734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full reset-password.html file =="
cat public/reset-password.html
echo
echo "== Check index.html for import maps or appwrite setup =="
grep -n 'importmap\|appwrite\|script.*src\|script.*module' index.html | head -20Repository: subby2006/monochrome-android
Length of output: 8387
The bare appwrite import will fail in the browser—this file is served as a static asset.
Line 174 uses import { Client, Account } from 'appwrite', but this file is in public/ and is not processed by Vite's module resolution. Browsers cannot resolve bare module specifiers without an import map or bundling. This page will fail before any reset logic runs.
Either:
- Bundle this file as part of the main build (make it an entry point), or
- Add an import map to inject the appwrite endpoint, or
- Use a CDN URL like
https://cdn.jsdelivr.net/npm/appwrite@latest/dist/appwrite.browser.jsand access the library fromwindow.Appwrite
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/reset-password.html` around lines 173 - 175, The browser-served static
page uses a bare import "import { Client, Account } from 'appwrite'", which will
fail; either (A) move this reset-password script into the app's build (make it a
bundled entry so Vite resolves the "appwrite" module) and update references to
Client/Account where the bundled module exports them, or (B) change the static
page to load Appwrite from a CDN or import map and then reference
window.Appwrite.Client and window.Appwrite.Account in the script; specifically
update the module import in public/reset-password.html (the script that
currently imports Client and Account) to use one of these approaches and adjust
subsequent code that references Client and Account to use the correct global or
bundled symbols.
| if (url === '/api/auth/login' && req.method === 'POST') { | ||
| try { | ||
| const body = await parseBody(req); | ||
| if (!body.token) { | ||
| if (!body.userId) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify({ error: 'Missing token' })); | ||
| res.end(JSON.stringify({ error: 'Missing userId' })); | ||
| return; | ||
| } | ||
| const { payload } = await jwtVerify(body.token, JWKS, { | ||
| issuer: `https://securetoken.google.com/${PROJECT_ID}`, | ||
| audience: PROJECT_ID, | ||
| }); | ||
| req.session.uid = payload.sub; | ||
| req.session.email = payload.email; | ||
| req.session.uid = body.userId; | ||
| req.session.email = body.email; | ||
| req.session.iat = Date.now(); | ||
| res.setHeader('Content-Type', 'application/json'); |
There was a problem hiding this comment.
This login route is an auth bypass right now.
Any caller can POST a made-up userId / email pair to /api/auth/login and immediately receive a valid session cookie. The server needs to verify an Appwrite-issued proof server-side before persisting identity into req.session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite-plugin-auth-gate.js` around lines 133 - 145, The /api/auth/login POST
handler currently accepts any userId/email and writes them to req.session (see
parseBody and the req.session.uid/email/iat assignments); update this flow to
require and validate an Appwrite-issued proof token (e.g., body.proof or
body.jwt) server-side before mutating req.session. Implement a verification step
using the Appwrite SDK or your Appwrite verification endpoint (create a helper
like verifyAppwriteProof(proof) or call account.get() / teams.getSession() as
appropriate) and only set req.session.uid/email/iat after verifyAppwriteProof
returns a valid, matching user; if verification fails return 401. Ensure missing
proof returns 400 and failed verification returns 401 so arbitrary POSTs cannot
bypass auth.
…o movies directory for videos
…is still turbo fucked rn)
… codex for making it worse
7 participants
massive change that's mostly outside my domain, thanks samidy
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation