Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for htmlplayer-v2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedToo many files! This PR contains 299 files, which is 149 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (299)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NellowTCS
force-pushed
the
new-architecture
branch
2 times, most recently
from
8bddc8b to
6c8e629
Compare
NellowTCS
force-pushed
the
new-architecture
branch
from
6c8e629 to
88690dd
Compare
added 13 commits
March 16, 2026 23:45
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review src/engine src/core src/hooks |
|
Please focus the review on the following paths: ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Introduces a new WIP architecture by adding a modular core engine + platform layer (audio backends, library management, integrations) and refactoring the app entry/UI to consume the new abstractions (including new privacy/terms pages and updated theming setup).
Changes:
- Added a new
core/enginefoundation (types, events, state machine, queue, scheduler). - Added
platform/*modules for library, audio (backends/decoders/effects), and integrations (MediaSession/Discord/share target). - Refactored the main page entry to use the new
useKomorebi+AppShellcomposition and moved legal pages into React routes/entries.
Reviewed changes
Copilot reviewed 91 out of 464 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| Build/src/platform/lyrics/index.ts | Re-exports lyrics metadata filter utilities. |
| Build/src/platform/library/types.ts | Defines library state/actions/events types for new library layer. |
| Build/src/platform/library/smartPlaylist.ts | Adds a rule-based smart playlist generator built on scoring state. |
| Build/src/platform/library/scoring.ts | Introduces per-track scoring engine used for shuffle/playlists. |
| Build/src/platform/library/persistence.ts | Adds persistence wrapper around storage modules for library data. |
| Build/src/platform/library/library.ts | Implements LibraryManager and playlist/folder helpers. |
| Build/src/platform/library/index.ts | Barrels exports for the new library module. |
| Build/src/platform/library/duplicateDetector.ts | Adds duplicate detection via hashing and metadata signatures. |
| Build/src/platform/integrations/shareTarget.ts | Implements share-target parsing and cache retrieval for shared files/text. |
| Build/src/platform/integrations/mediaSession.ts | Adds Media Session integration with action handlers + metadata updates. |
| Build/src/platform/integrations/index.ts | Barrels exports for integrations. |
| Build/src/platform/integrations/discordService.ts | Reworks Discord service logging to use the new logger. |
| Build/src/platform/integrations/discord.ts | Adds Discord integration wrapper around DiscordService. |
| Build/src/platform/integrations/base.ts | Adds common integration base class + interface. |
| Build/src/platform/audio/replayGain.ts | Adds ReplayGain analyzer + tag parsing helpers. |
| Build/src/platform/audio/preloader.ts | Adds a track preloader with cache/TTL/eviction. |
| Build/src/platform/audio/index.ts | Defines audio backend interfaces and factory types. |
| Build/src/platform/audio/floWavDecoder.ts | Adds flo→wav decoder wrapper (reflo) with init guard. |
| Build/src/platform/audio/floDecoder.ts | Adds flo decoder wrapper (libflo) to AudioBuffer + helpers. |
| Build/src/platform/audio/equalizer.ts | Adds an equalizer implementation with presets and filter chain. |
| Build/src/platform/audio/backends/index.ts | Adds backend manager selection and exports for audio backends. |
| Build/src/platform/audio/backends/WebAudioBackend.ts | Adds WebAudio backend implementation with time updates + analyser. |
| Build/src/platform/audio/backends/PitchBackend.ts | Adds pitch shifting backend wrapper using Tone.js PitchShift. |
| Build/src/platform/audio/backends/HybridBackend.ts | Adds hybrid backend delegating to HTML/WebAudio backends. |
| Build/src/platform/audio/backends/HTMLBackend.ts | Adds HTMLAudioElement backend implementation. |
| Build/src/platform/audio/backends/FloBackend.ts | Adds flo-specific backend using libflo decoding + WebAudio playback. |
| Build/src/platform/audio/backends/BaseBackend.ts | Adds shared callback management for backends (time/ended/error). |
| Build/src/pages/termsEntry.tsx | Adds React entrypoint for Terms page with i18n/theme providers. |
| Build/src/pages/terms.tsx | Adds Terms page wrapper around LegalPage. |
| Build/src/pages/privacyEntry.tsx | Adds React entrypoint for Privacy page with i18n/theme providers. |
| Build/src/pages/privacy.tsx | Adds Privacy page wrapper around LegalPage. |
| Build/src/pages/main.tsx | Refactors app root render to new ThemeProvider + App component composition. |
| Build/src/pages/_index.tsx | Replaces legacy page implementation with useKomorebi + AppShell. |
| Build/src/locales/fr/translation.json | Adds common.confirm i18n string (FR). |
| Build/src/locales/en/translation.json | Adds common.confirm i18n string (EN). |
| Build/src/hooks/useVisualizerCanvas.ts | Extracts visualizer rendering into a dedicated hook. |
| Build/src/hooks/useShareTarget.ts | Adds hook wrapper around share target handlers. |
| Build/src/hooks/useKeyboardShortcuts.ts | Refactors keyboard shortcut hook to use new storage + Komorebi API. |
| Build/src/hooks/useFilePicker.tsx | Adds Uppy-based audio file picker dialog. |
| Build/src/hooks/useFileHandler.ts | Adds launchQueue-based file handler hook (PWA file opening). |
| Build/src/hooks/useDialogVisibility.ts | Removes legacy dialog visibility hook (IndexedDB-based). |
| Build/src/hooks/useAudioSync.ts | Removes legacy cross-window audio sync hook. |
| Build/src/hooks/useAlbumArt.tsx | Switches album art caching/loading to new albumArtStorage. |
| Build/src/helpers/wallpaperLoader.tsx | Removes legacy wallpaper loader/provider. |
| Build/src/helpers/themeMode.ts | Removes legacy theme mode utilities + PiP theme broadcasting. |
| Build/src/helpers/themeMetadata.ts | Removes legacy theme metadata loader. |
| Build/src/helpers/themeLoader.tsx | Removes legacy theme loader/provider. |
| Build/src/helpers/shuffleManager.ts | Removes legacy shuffle manager and next-song caching utilities. |
| Build/src/helpers/safariHelper.ts | Removes legacy Safari background audio manager. |
| Build/src/helpers/refloWavHelper.ts | Removes legacy flo→wav helper (moved to platform/audio). |
| Build/src/helpers/playlistImageHelper.ts | Removes legacy playlist image generator helper. |
| Build/src/helpers/platform.ts | Removes legacy platform abstraction helpers. |
| Build/src/helpers/pitchShiftHelper.ts | Removes legacy pitch helpers. |
| Build/src/helpers/musicPlayerUtils.ts | Removes legacy tempo/pitch playback rate helpers. |
| Build/src/helpers/logger.ts | Introduces new Satori-based logging wrapper and throwError. |
| Build/src/helpers/importAudioFiles.tsx | Refactors import pipeline to new metadata/audio/storage modules. |
| Build/src/helpers/gaplessHelper.ts | Removes legacy gapless offset calculator. |
| Build/src/helpers/floProcessor.ts | Removes legacy flo processing helpers (moved to platform/audio). |
| Build/src/helpers/discordService_backup.ts | Removes backup Discord service implementation. |
| Build/src/helpers/audioProcessor.ts | Removes legacy audio processor (IndexedDB + flo init). |
| Build/src/global.css | Adds base theme import + new layout/util CSS patterns. |
| Build/src/data/legal.ts | Adds legal document data as typed TS objects. |
| Build/src/core/engine/types.ts | Adds engine domain types (track/playlist/state/events/config). |
| Build/src/core/engine/state/index.ts | Adds a state machine with validated transitions + history. |
| Build/src/core/engine/scheduler/index.ts | Adds crossfade/gapless schedulers + scheduler facade. |
| Build/src/core/engine/queue/index.ts | Adds queue manager with shuffle and smart shuffle support. |
| Build/src/core/engine/index.ts | Barrels exports for engine module. |
| Build/src/core/engine/events.ts | Adds typed event emitter for engine events. |
| Build/src/contexts/audioStore.ts | Removes legacy zustand audio store + BroadcastChannel syncing. |
| Build/src/constants/pip.ts | Adds constants for PiP window sizing and theme variable list. |
| Build/src/components/Visualizer.tsx | Removes legacy Visualizer component (replaced by hook/UI). |
| Build/src/components/Miniplayer.tsx | Removes legacy PiP/Miniplayer implementation. |
| Build/public/tos.html | Removes static public TOS page (moved into React legal pages). |
| Build/public/privacy.html | Removes static public privacy page (moved into React legal pages). |
| Build/privacy.html | Adds standalone privacy HTML entry wiring to React privacy entry. |
| Build/package.json | Switches testing to Jest + adds Tone.js and Satori dependency changes. |
| Build/jest.config.cjs | Adds Jest configuration (ts-jest, coverage, jsdom). |
| Build/index.html | Updates default theme link path to new theme resource location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+313
to
+325
| for (const item of items) { | ||
| if (item.id === id) return null; | ||
| if ("children" in item) { | ||
| for (const child of item.children) { | ||
| if (child.id === id) return item.id; | ||
| if ("children" in child) { | ||
| const found = findParentFolderId(item.children, id); | ||
| if (found) return found; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The recursive call uses findParentFolderId(item.children, id) instead of recursing into the child subtree. This re-traverses the same item.children array and can lead to incorrect results or runaway recursion when nested folders exist. Fix by recursing into the child's children (or restructure the traversal to recurse once per node).
Suggested change
| for (const item of items) { | |
| if (item.id === id) return null; | |
| if ("children" in item) { | |
| for (const child of item.children) { | |
| if (child.id === id) return item.id; | |
| if ("children" in child) { | |
| const found = findParentFolderId(item.children, id); | |
| if (found) return found; | |
| } | |
| } | |
| } | |
| } | |
| return null; | |
| function search( | |
| nodes: (Playlist | PlaylistFolder)[], | |
| parentId: string | null, | |
| ): string | null { | |
| for (const item of nodes) { | |
| if (item.id === id) { | |
| return parentId; | |
| } | |
| if ("children" in item && item.children?.length) { | |
| const found = search(item.children, item.id); | |
| if (found) { | |
| return found; | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| return search(items, null); |
Comment on lines
+65
to
+69
| if (this.loading.has(track.id)) { | ||
| return new Promise((resolve) => { | ||
| this.preloadCallbacks.set(track.id, resolve); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Concurrent callers to preload(track) while a load is in-flight will overwrite the previous resolver in preloadCallbacks, causing earlier Promises to never resolve. Store an array/set of resolvers per track ID, or replace the callback map with an inflight: Map<string, Promise<string>> and return the same Promise to all callers.
Comment on lines
+19
to
+26
| export function setProcessingState(processing: boolean) { | ||
| if (processing) { | ||
| window.addEventListener("beforeunload", (e) => { | ||
| e.preventDefault(); | ||
| e.returnValue = ""; | ||
| }); | ||
| } else { | ||
| window.removeEventListener("beforeunload", () => {}); |
There was a problem hiding this comment.
The beforeunload listener added is an anonymous function, but the removal uses a different anonymous function, so the listener is never removed (leaks and can wrongly block navigation later). Use a stable handler reference (module-level function, ref, or closure variable) for both add/remove.
Suggested change
| export function setProcessingState(processing: boolean) { | |
| if (processing) { | |
| window.addEventListener("beforeunload", (e) => { | |
| e.preventDefault(); | |
| e.returnValue = ""; | |
| }); | |
| } else { | |
| window.removeEventListener("beforeunload", () => {}); | |
| const beforeUnloadHandler = (e: BeforeUnloadEvent) => { | |
| e.preventDefault(); | |
| e.returnValue = ""; | |
| }; | |
| export function setProcessingState(processing: boolean) { | |
| if (processing) { | |
| window.addEventListener("beforeunload", beforeUnloadHandler); | |
| } else { | |
| window.removeEventListener("beforeunload", beforeUnloadHandler); |
Comment on lines
+86
to
+91
| setEnabled(enabled: boolean): void { | ||
| this.enabled = enabled; | ||
| if (this.gainNode) { | ||
| this.gainNode.gain.value = enabled ? 1 : 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
setEnabled(false) has no effect because it sets the gain to 1 in both branches. If the intent is to bypass the EQ, either (a) disconnect/reconnect the processing chain to bypass filters, or (b) set all filter gains to 0 when disabled and restore when enabled.
Comment on lines
+83
to
+89
| const source = ctx.createBufferSource(); | ||
| source.buffer = this.chain.audioBuffer; | ||
| source.connect(this.chain.gainNode); | ||
|
|
||
| const analyser = this.ensureAnalyser(); | ||
| this.chain.gainNode.connect(analyser); | ||
| analyser.connect(ctx.destination); |
There was a problem hiding this comment.
Each play() call reconnects gainNode -> analyser -> destination without disconnecting them, which can create multiple parallel connections that sum audio (volume increases) and leak nodes over time. Ensure the graph is connected exactly once (e.g., connect in ensureContext() and only reconnect the source, or disconnect/rebuild the chain on stop).
| "start": "cd dist && python3 -m http.server 8000", | ||
| "preview": "vite preview", | ||
| "test": "react-scripts test", | ||
| "test": "jest", |
There was a problem hiding this comment.
With test now mapped to plain jest, CI will fail if there are currently no tests. If the repo is mid-migration, consider aligning test to jest --passWithNoTests (and keep a stricter target for CI later), or add at least a minimal smoke test to keep the default test script meaningful.
Suggested change
| "test": "jest", | |
| "test": "jest --passWithNoTests", |
| "eslint": "^9.39.4", | ||
| "eslint-plugin-react": "^7.37.5", | ||
| "jest": "^29.7.0", | ||
| "jest-environment-jsdom": "^30.3.0", |
There was a problem hiding this comment.
jest-environment-jsdom@^30.x targets Jest 30, but jest is pinned to ^29.7.0. This version mismatch commonly breaks Jest runtime resolution. Either bump Jest to 30.x (and ensure ts-jest compatibility) or downgrade jest-environment-jsdom to a 29.x version.
Suggested change
| "jest-environment-jsdom": "^30.3.0", | |
| "jest-environment-jsdom": "^29.7.0", |
Comment on lines
+132
to
+136
| return track.gapless?.encoderDelay ?? this.config.startOffset; | ||
| } | ||
|
|
||
| getEndOffset(track: Track): number { | ||
| return track.gapless?.encoderPadding ?? this.config.endOffset; |
There was a problem hiding this comment.
Gapless encoder delay/padding values are typically expressed in samples, not seconds. Subtracting raw encoderPadding from track.duration (seconds) can cause incorrect transition timing (potentially huge negative/positive offsets). Convert samples to seconds using track.encoding.sampleRate (or clarify/rename the units in GaplessInfo and ensure producers store seconds).
Suggested change
| return track.gapless?.encoderDelay ?? this.config.startOffset; | |
| } | |
| getEndOffset(track: Track): number { | |
| return track.gapless?.encoderPadding ?? this.config.endOffset; | |
| const encoderDelay = track.gapless?.encoderDelay; | |
| const sampleRate = (track as any).encoding?.sampleRate as number | undefined; | |
| if ( | |
| typeof encoderDelay === "number" && | |
| typeof sampleRate === "number" && | |
| sampleRate > 0 | |
| ) { | |
| // encoderDelay is typically expressed in samples; convert to seconds | |
| return encoderDelay / sampleRate; | |
| } | |
| return this.config.startOffset; | |
| } | |
| getEndOffset(track: Track): number { | |
| const encoderPadding = track.gapless?.encoderPadding; | |
| const sampleRate = (track as any).encoding?.sampleRate as number | undefined; | |
| if ( | |
| typeof encoderPadding === "number" && | |
| typeof sampleRate === "number" && | |
| sampleRate > 0 | |
| ) { | |
| // encoderPadding is typically expressed in samples; convert to seconds | |
| return encoderPadding / sampleRate; | |
| } | |
| return this.config.endOffset; |
Comment on lines
+144
to
+147
| calculatePlayEnd(track: Track): number { | ||
| const offset = this.getEndOffset(track); | ||
| return track.duration - offset; | ||
| } |
There was a problem hiding this comment.
Gapless encoder delay/padding values are typically expressed in samples, not seconds. Subtracting raw encoderPadding from track.duration (seconds) can cause incorrect transition timing (potentially huge negative/positive offsets). Convert samples to seconds using track.encoding.sampleRate (or clarify/rename the units in GaplessInfo and ensure producers store seconds).
Comment on lines
+176
to
+184
| private updateMode(): void { | ||
| if (this.gapless.isEnabled() && this.gapless.getConfig().enabled) { | ||
| this.currentMode = "gapless"; | ||
| } else if (this.crossfade.isEnabled()) { | ||
| this.currentMode = "crossfade"; | ||
| } else { | ||
| this.currentMode = "none"; | ||
| } | ||
| } |
There was a problem hiding this comment.
With this precedence, if gapless is enabled, crossfade mode is never selected even when crossfade is enabled. If crossfade is intended to override gapless when enabled (common behavior), swap the precedence or make the choice explicit via a single 'transition mode' setting.
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
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.
No description provided.