Cloud Sync v2 & Unified LocalStorage State Handling#306
Conversation
Currently translated at 98.2% (164 of 167 strings) Translation: ytify/web Translate-URL: https://hosted.weblate.org/projects/ytify/web/fr/
Currently translated at 19.7% (33 of 167 strings) Translation: ytify/web Translate-URL: https://hosted.weblate.org/projects/ytify/web/frc/
|
""" WalkthroughThis update centralizes application settings and feature flags into a single reactive Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant state
participant setState
participant localStorage
User->>UI: Toggle a setting or change preference
UI->>state: Read current value
UI->>setState: setState(key, newValue)
setState->>state: Update value
setState->>localStorage: Persist entire state as JSON
UI->>User: Show notification if reload is needed
sequenceDiagram
participant User
participant UI
participant cloudSync
participant localStorage
participant Cloud
User->>UI: Click sync button
UI->>cloudSync: Initiate sync (with dbhash)
cloudSync->>localStorage: Read library data
cloudSync->>Cloud: POST library data (save to cloud)
Cloud-->>cloudSync: Respond with success/error
cloudSync->>UI: Update sync icon/status
Note over UI,User: User can import from cloud if local library is empty
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
src/scripts/queue.ts (4)
53-63:⚠️ Potential issueFix the toggle logic - state and UI are out of sync.
The toggle logic is inverted and will cause state to be opposite of the UI. Currently:
setState('shuffle', btn.classList.contains('on'))saves the current statebtn.classList.toggle('on')then changes the UIThis means if the button shows "on" and the user clicks to turn it "off", the state will be saved as
truebut the UI will show "off".Apply this fix to synchronize state with UI:
- setState('shuffle', btn.classList.contains('on')); - btn.classList.toggle('on'); + setState('shuffle', btn.classList.contains('on'));
85-95:⚠️ Potential issueFix the toggle logic - state and UI are out of sync.
Same issue as the shuffle toggle - the state is saved before the UI is updated, causing them to be out of sync.
Apply this fix:
- setState('filterLT10', btn.classList.contains('on')); - btn.classList.toggle('on'); + setState('filterLT10', btn.classList.contains('on'));
103-113:⚠️ Potential issueFix the toggle logic - state and UI are out of sync.
Same toggle logic issue affecting the allowDuplicates setting.
Apply this fix:
- setState('allowDuplicates', btn.classList.contains('on')); - btn.classList.toggle('on'); + setState('allowDuplicates', btn.classList.contains('on'));
121-132:⚠️ Potential issueFix the toggle logic - state and UI are out of sync.
Same toggle logic issue affecting the enqueueRelatedStreams setting.
Apply this fix:
- setState('enqueueRelatedStreams', btn.classList.contains('on')); - btn.classList.toggle('on'); + setState('enqueueRelatedStreams', btn.classList.contains('on'));src/components/Settings/library.ts (1)
31-60:⚠️ Potential issueFix race condition in async hash creation.
The
setState('dbsync', hash)call at line 60 executes before the hash is generated asynchronously. This will always set an empty string to the state.Move the
setStatecall inside the hash creation callback:- location.reload(); - }); - } - if (!state.dbsync) { + setState('dbsync', hash); + location.reload(); + }); + } + if (!state.dbsync) { const termsAccepted = confirm('Data will be automatically deleted after one week of inactivity.\nytify is not responsible for data loss.\n\nI Understand'); if (termsAccepted) { const username = prompt('Enter Username :'); if (username) { const password = prompt('Enter Password :'); const confirmpw = prompt('Confirm Password :'); if (password && password === confirmpw) hashCreator(username + password); else alert('Incorrect Information!'); } } e.preventDefault(); + } else { + setState('dbsync', ''); } - setState('dbsync', hash);
🧹 Nitpick comments (4)
src/stylesheets/library.css (1)
4-33: LGTM! Well-structured CSS for cloud sync feature.The CSS additions properly support the new cloud sync functionality with:
- Inline display for header elements
- Comprehensive icon state management
- User-friendly hover tooltips
- Proper loading and error states
Consider accessibility improvements for the tooltips:
&.ri-cloud-fill:hover::after { position: fixed; content: 'Import from cloud'; font-size: small; + z-index: 1000; + background: var(--bg); + border: var(--border); + padding: 0.25rem; + border-radius: var(--roundness); } &.ri-cloud-off-fill:hover::after { position: fixed; content: 'Save to cloud'; font-size: small; + z-index: 1000; + background: var(--bg); + border: var(--border); + padding: 0.25rem; + border-radius: var(--roundness); }src/scripts/library.ts (1)
81-85: Add error handling for the dynamic cloud sync import.The cloud sync implementation looks good, but consider adding error handling for the dynamic import to gracefully handle potential module loading failures.
-if (dbsync) import('../modules/cloudSync').then(mod => mod.default(dbsync, syncBtn)); +if (dbsync) import('../modules/cloudSync') + .then(mod => mod.default(dbsync, syncBtn)) + .catch(err => console.warn('Cloud sync module failed to load:', err)); else syncBtn.remove();src/modules/start.ts (1)
127-127: Add context to the TODO comment.The TODO comment doesn't explain what needs to be implemented or fixed.
Please add a descriptive comment explaining what work is needed here, for example:
- // TODO + // TODO: Add logic to handle specific edge cases when clicking outside detailssrc/lib/store.ts (1)
4-46: Consider using more conventional property naming and add JSDoc documentation.The state object contains properties with spaces in their names (e.g.,
'part Reserved Collections'), which is unconventional and could be error-prone. Consider using camelCase or kebab-case naming conventions instead.- 'part Reserved Collections': true, - 'part Navigation Library': true, + partReservedCollections: true, + partNavigationLibrary: true,Additionally, consider adding JSDoc documentation to describe what each setting does, especially for properties that aren't self-explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/remixicon.woff2is excluded by!**/*.woff2
📒 Files selected for processing (39)
index.html(1 hunks)netlify/edge-functions/landing.ts(0 hunks)package.json(1 hunks)src/components/ActionsMenu.ts(6 hunks)src/components/ListItem.ts(2 hunks)src/components/Settings/app.ts(6 hunks)src/components/Settings/index.ts(2 hunks)src/components/Settings/library.ts(3 hunks)src/components/Settings/parental.ts(4 hunks)src/components/Settings/personalize.ts(5 hunks)src/components/Settings/playback.ts(3 hunks)src/components/Settings/search.ts(2 hunks)src/components/StreamItem.ts(3 hunks)src/components/SuperCollectionList.ts(2 hunks)src/components/WatchVideo.ts(4 hunks)src/lib/player.ts(4 hunks)src/lib/store.ts(3 hunks)src/lib/utils.ts(6 hunks)src/modules/audioErrorHandler.ts(2 hunks)src/modules/cloudSync.ts(1 hunks)src/modules/extractColorFromImage.ts(1 hunks)src/modules/hls.ts(2 hunks)src/modules/miniPlayer.ts(1 hunks)src/modules/partsManager.ts(2 hunks)src/modules/setAudioStreams.ts(2 hunks)src/modules/setMetadata.ts(2 hunks)src/modules/start.ts(4 hunks)src/scripts/audioEvents.ts(4 hunks)src/scripts/i18n.ts(1 hunks)src/scripts/library.ts(4 hunks)src/scripts/queue.ts(7 hunks)src/scripts/router.ts(3 hunks)src/scripts/search.ts(5 hunks)src/scripts/theme.ts(4 hunks)src/stylesheets/global.css(1 hunks)src/stylesheets/header.css(0 hunks)src/stylesheets/library.css(2 hunks)src/stylesheets/list.css(1 hunks)src/stylesheets/remixicon.css(2 hunks)
💤 Files with no reviewable changes (2)
- src/stylesheets/header.css
- netlify/edge-functions/landing.ts
🧰 Additional context used
🧬 Code Graph Analysis (24)
src/modules/setMetadata.ts (1)
src/lib/store.ts (1)
state(4-46)
src/modules/miniPlayer.ts (2)
src/lib/dom.ts (1)
img(1-1)src/lib/store.ts (1)
state(4-46)
src/scripts/i18n.ts (1)
src/lib/store.ts (1)
state(4-46)
src/scripts/search.ts (2)
src/lib/store.ts (1)
state(4-46)src/lib/dom.ts (1)
searchFilters(15-15)
src/modules/audioErrorHandler.ts (1)
src/lib/store.ts (1)
state(4-46)
src/components/ListItem.ts (2)
src/lib/store.ts (1)
state(4-46)src/lib/utils.ts (1)
hostResolver(28-32)
src/scripts/router.ts (2)
src/lib/store.ts (1)
state(4-46)src/lib/utils.ts (1)
goTo(12-12)
src/components/Settings/search.ts (1)
src/lib/store.ts (2)
state(4-46)setState(56-60)
src/modules/setAudioStreams.ts (3)
src/lib/dom.ts (1)
title(3-3)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/store.ts (2)
state(4-46)store(63-158)
src/modules/partsManager.ts (2)
src/lib/store.ts (2)
state(4-46)setState(56-60)src/scripts/i18n.ts (1)
i18n(44-49)
src/components/WatchVideo.ts (2)
src/lib/store.ts (2)
state(4-46)setState(56-60)netlify/edge-functions/fallback.ts (1)
_(3-44)
src/scripts/library.ts (1)
src/lib/store.ts (1)
state(4-46)
src/lib/utils.ts (1)
src/lib/store.ts (1)
state(4-46)
src/modules/cloudSync.ts (2)
src/lib/libraryUtils.ts (2)
saveDB(10-13)getDB(8-8)src/lib/utils.ts (1)
notify(57-65)
src/components/Settings/personalize.ts (2)
src/lib/store.ts (2)
state(4-46)setState(56-60)src/scripts/i18n.ts (1)
i18n(44-49)
src/components/Settings/parental.ts (1)
src/lib/store.ts (2)
state(4-46)setState(56-60)
src/scripts/audioEvents.ts (1)
src/lib/store.ts (2)
state(4-46)setState(56-60)
src/components/SuperCollectionList.ts (1)
src/lib/store.ts (2)
state(4-46)setState(56-60)
src/components/Settings/library.ts (3)
src/lib/store.ts (2)
state(4-46)setState(56-60)src/lib/libraryUtils.ts (2)
getDB(8-8)saveDB(10-13)src/scripts/i18n.ts (1)
i18n(44-49)
src/scripts/queue.ts (1)
src/lib/store.ts (2)
state(4-46)setState(56-60)
src/scripts/theme.ts (1)
src/lib/store.ts (2)
state(4-46)store(63-158)
src/components/Settings/index.ts (1)
src/lib/store.ts (1)
params(2-2)
src/lib/player.ts (1)
src/lib/store.ts (2)
state(4-46)store(63-158)
src/modules/hls.ts (1)
src/lib/store.ts (1)
state(4-46)
🪛 Biome (1.9.4)
src/components/Settings/library.ts
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (104)
package.json (1)
17-17: LGTM! Routine dependency update.This patch version update for Node.js type definitions is appropriate and helps maintain compatibility with the latest type definitions.
src/stylesheets/global.css (1)
226-226: LGTM! Minor formatting improvement.Adding a newline for consistency is good practice and improves code readability.
src/stylesheets/list.css (1)
6-9: LGTM! CSS supports new sync icon layout.The
padding-right: 2remon direct child<p>elements provides appropriate spacing for the new sync icon added to the library header. The selector is specific and won't interfere with other elements.index.html (1)
211-211: LGTM! Sync icon properly positioned in library header.The sync icon placement in the library section header is logical and appropriate for cloud sync functionality. The
syncNowID andri-cloud-fillclass clearly indicate its purpose and provide the necessary hooks for JavaScript interaction.src/modules/hls.ts (2)
1-1: LGTM! Proper import for centralized state management.Adding
stateto the import aligns with the refactoring to centralize state management throughout the application.
12-12: LGTM! Consistent state refactoring.The change from
store.player.hqtostate.hqcorrectly implements the centralized state management pattern without altering the underlying logic. Thestate.hqboolean flag maintains the same functionality for determining HLS quality level selection.src/modules/setMetadata.ts (2)
32-32: LGTM! Consistent state management refactoring.The change from
store.loadImagetostate.loadImagealigns with the unified state handling approach and maintains the same boolean logic for conditional image loading.
3-3:Details
✅ Verification successful
Verify both imports are needed.
The import now includes both
stateandstorefrom the same module. Please ensure both are actually used in this file to avoid unused imports.
🏁 Script executed:
#!/bin/bash # Check if both state and store are used in the file echo "Checking usage of 'state' in setMetadata.ts:" rg "\bstate\." src/modules/setMetadata.ts echo -e "\nChecking usage of 'store' in setMetadata.ts:" rg "\bstore\." src/modules/setMetadata.tsLength of output: 421
No Unused Imports Detected
Both
stateandstoreare actively used insrc/modules/setMetadata.ts(state.loadImage,store.stream,store.actionsMenu), so no imports need to be removed. Approval granted.src/scripts/i18n.ts (2)
1-1: Good refactoring to centralized state management.The import change from
getSavedtostatealigns with the unified state handling approach.
4-4: LGTM! Maintains fallback logic correctly.The change from
getSaved('language')tostate.languagepreserves the existing fallback behavior while using the centralized state object. The logical OR operation ensures proper defaults are maintained.src/components/ListItem.ts (2)
2-2: Consistent with state management refactoring.The import change from
storetostatealigns with the unified state handling approach across the codebase.
27-27: LGTM! Consistent image loading logic.Both changes from
store.loadImagetostate.loadImagemaintain consistent behavior:
- Line 27: Controls the 'ravel' CSS class based on image loading preference
- Line 34: Controls conditional rendering of the image element
The logic remains the same while using the centralized state object.
Also applies to: 34-34
src/modules/miniPlayer.ts (2)
3-3: Consistent import change for unified state management.The import change from
storetostatealigns with the broader refactoring to centralize state management.
7-7: LGTM! Maintains image loading logic correctly.The change from
store.loadImagetostate.loadImagepreserves the functionality where the image element is removed when image loading is disabled. The negation logic (!state.loadImage) remains correct.src/modules/audioErrorHandler.ts (2)
2-2: LGTM: Import updated for centralized state managementThe import change aligns with the broader refactor to use centralized state management.
11-11: LGTM: State access updated correctlyThe change from
getSaved('custom_instance')tostate.customInstancemaintains the same logic while using the new centralized state approach. The property exists in the state object with appropriate default value.src/components/StreamItem.ts (2)
2-2: LGTM: Import updated for centralized stateThe import change correctly switches from
storetostatefor the new centralized state management.
47-47: LGTM: Consistent state property accessAll references to
store.loadImagehave been correctly updated tostate.loadImage. The conditional logic for image loading and CSS class handling remains functionally identical.Also applies to: 54-54, 66-66
src/components/WatchVideo.ts (3)
4-4: LGTM: Import updated for centralized state managementThe import correctly includes
setStateandstatefrom the new centralized store module, replacing the previousgetSavedandsavefunctions.
20-20: LGTM: State property access updated correctlyBoth
state.watchModeandstate.hqcorrectly replace the previousgetSavedcalls, accessing the same properties from the centralized state object.Also applies to: 48-48
163-163: LGTM: State update method correctly implementedThe change from
save('watchMode', ...)tosetState('watchMode', ...)correctly uses the new centralized state update function that both updates the state object and persists to localStorage.src/scripts/router.ts (2)
3-3: LGTM: Import updated for centralized stateThe import correctly switches to the new centralized state management system.
109-109:Details
✅ Verification successful
LGTM: Fallback logic simplified through state defaults
The changes correctly remove the explicit fallback to
'/search'sincestate.startupTabhas'/search'as its default value in the state object definition. This simplifies the code while maintaining the same behavior.Let me verify that the state initialization properly handles edge cases:
Also applies to: 118-118
🏁 Script executed:
#!/bin/bash # Verify that state.startupTab always has a valid default value ast-grep --pattern 'startupTab: $_'Length of output: 37
I couldn’t find any
startupTabdeclarations with the first pattern. Let’s locate every occurrence ofstartupTab(definitions & references) to confirm its default value:
🏁 Script executed:
#!/bin/bash # List all occurrences of "startupTab" to find its initialization rg -n "startupTab" . # Look for possible initialState objects that might include startupTab rg -n "initialState" .Length of output: 510
Fallback logic is safe via default state
Confirmed that in
src/lib/store.ts(around line 19) thestartupTabproperty is initialized to'/search'. This ensures the removed explicit fallback in the router still resolves correctly.• src/lib/store.ts:19 —
startupTab: '/search'src/scripts/search.ts (3)
3-4: LGTM! Clean import refactoring.The import changes correctly replace utility functions with centralized state management from the store module.
83-83: LGTM! Proper DOM API usage.Good replacement of the utility function with native
document.createElement().
156-160: LGTM! Correct state-based initialization.The logic correctly removes the suggestions element when disabled and sets the search filter value from state.
src/modules/extractColorFromImage.ts (2)
2-2: LGTM! Clean export pattern.Converting to default export is consistent with the module's single responsibility.
6-6: LGTM! Proper DOM API usage.Good replacement of the utility function with native
document.createElement(), maintaining the same functionality while removing dependencies.src/lib/player.ts (4)
2-3: LGTM! Clean import refactoring.Properly updated imports to use centralized state management instead of scattered utility functions.
12-21: LGTM! Consistent state access pattern.The watch mode check and dialog creation properly use the centralized state and native DOM APIs.
71-73: LGTM! Proper feature flag handling.Correctly uses the centralized state for the
enqueueRelatedStreamsfeature flag.
92-98: LGTM! Consistent discover feature handling.Properly uses the centralized state for the
discoverfeature flag check.src/modules/setAudioStreams.ts (4)
2-2: LGTM! Proper state management import.Correctly imports the centralized state management objects.
6-7: LGTM! Necessary async conversion.Making the function async is required to properly await the opus support detection in the codec preference logic.
11-11: LGTM! Improved codec selection logic.The codec preference logic correctly uses the centralized state and properly handles the async opus support detection when codec is set to 'any'.
38-38: LGTM! Consistent HQ preference handling.Properly uses the centralized state for the HQ preference check.
src/scripts/audioEvents.ts (4)
3-4: LGTM! Clean import refactoring.The import changes correctly remove unused localStorage utility functions and add the necessary centralized state management imports.
205-205: LGTM! Simplified volume persistence.The refactoring correctly simplifies volume persistence by always using
setStateinstead of conditional save/remove logic. This is more consistent and cleaner.
214-218: LGTM! Consistent state initialization.The volume initialization correctly uses the centralized state object instead of direct localStorage access. The logic remains the same while being more consistent with the new state management approach.
57-57:Details
✅ Verification successful
Verify the boolean logic for history state.
The logic changed from
getSaved('history') === 'off'to!state.history. This suggests the state type changed from a string ('off'/'on') to a boolean. Ensure this is consistent with the state definition and other usages.
🏁 Script executed:
#!/bin/bash # Description: Verify history state usage consistency across the codebase # Expected: All history state checks should use boolean logic # Search for history state usage patterns rg -A 2 -B 2 "state\.history|getSaved.*history|save.*history" --type tsLength of output: 604
History state logic is correctly implemented as boolean
Both usages of
state.historynow operate on a boolean value driven by the settings checkbox insrc/components/Settings/library.ts(checked: state.history) and the guard insrc/scripts/audioEvents.ts(if (!state.history) return;). There are no remaining string-based comparisons such asgetSaved('history') === 'off'. Everything is consistent.src/modules/partsManager.ts (4)
1-1: LGTM! Correct import refactoring.The import change properly replaces localStorage utility functions with centralized state management.
8-27: LGTM! Consistent state access pattern.All state checks correctly use direct boolean access from the centralized state object, maintaining the same functionality while improving consistency.
85-89: LGTM! Type-safe state handler.The rewritten
lsHandlerfunction correctly uses type-safe state key access andsetStatefor persistence. The implementation is clean and maintains the toggle functionality.
96-98: LGTM! Consistent PIN verification.The PIN verification correctly uses the centralized state instead of direct localStorage access, maintaining the same security logic while being more consistent.
src/components/Settings/search.ts (3)
4-4: LGTM! Correct import for centralized state management.The import change properly replaces localStorage utility functions with the centralized state management imports.
17-24: LGTM! Consistent search filter toggle logic.The refactoring correctly:
- Uses state object for checked state determination
- Maintains the toggle behavior between empty string and 'music_songs'
- Preserves navigation to '/search' after state change
- Uses type-safe setState function
30-34: LGTM! Consistent search suggestions toggle.The refactoring properly uses the centralized state for both checking the current value and updating it via setState, while preserving the page reload functionality.
src/lib/utils.ts (4)
4-4: LGTM: Import statement updated correctly.The import has been updated to use the centralized
stateandstoreobjects instead of thegetSavedutility function, aligning with the unified state management approach.
42-44: LGTM: State access pattern migrated correctly.The
getSavedcalls have been properly replaced with direct access tostateproperties. This maintains the same logic while using the centralized state management.
58-58: LGTM: Explicit DOM element creation.The
$utility function has been replaced with explicitdocument.createElementcalls, making the code more explicit and removing dependency on the utility shorthand.Also applies to: 182-182
85-85: LGTM: Consistent state property access.These
getSavedcalls have been correctly migrated to usestateproperties, maintaining consistency with the centralized state management pattern.Also applies to: 203-203
src/components/Settings/personalize.ts (5)
5-5: LGTM: Import statement updated for centralized state management.The import has been correctly updated to use
setStateandstatefrom the centralized store instead of the old utility functions.
19-24: LGTM: Toggle switch logic correctly migrated.The load image toggle now uses
state.loadImagefor the initial checked state andsetState('loadImage', !state.loadImage)for toggling, which correctly inverts the boolean value.
31-36: LGTM: Roundness selector properly migrated.Both the event handler and mount logic correctly use the centralized state pattern for managing the roundness setting.
49-64: LGTM: Custom theme logic correctly updated.The custom theme toggle logic has been properly migrated to use
state.customThemefor checking existence andsetStatefor updates. The prompt logic for color input remains intact.
71-75: LGTM: Theme selector migration is correct.The theme selector correctly uses
state.themefor the initial value andsetStatefor updates when the selection changes.src/scripts/queue.ts (3)
1-9: LGTM: Import statements updated correctly.The imports have been properly updated to use the centralized state management pattern, removing unused utilities and adding the required
setStateandstateimports.
194-194: LGTM: Improved destructuring syntax.The destructuring assignment for extracting the
idfrom the dataset is more concise and readable.
213-213: LGTM: State access correctly migrated.The shuffle check correctly uses
state.shuffleinstead of the oldgetSavedfunction.src/components/Settings/playback.ts (6)
5-6: LGTM: Import statements updated correctly.The imports have been properly updated to use centralized state management and include the
quickSwitchutility for applying changes immediately.
19-24: LGTM: High quality audio toggle correctly implemented.The toggle correctly uses
state.hqfor the initial checked state andsetState('hq', !state.hq)for proper boolean toggling, followed byquickSwitch()to apply the change.
47-62: LGTM: Audio settings toggles correctly implemented.Both the stable volume and enforce proxy toggles correctly use the centralized state pattern with proper boolean inversion for toggling.
68-73: LGTM: HLS toggle with page reload.The HLS toggle correctly updates the state and triggers a page reload, which is appropriate for this type of fundamental playback setting change.
78-85: LGTM: Watch mode toggle logic is correct.The watch mode toggle uses a conditional assignment pattern that sets the state to either an empty string (off) or '144p' (on), which correctly replaces the previous save/remove pattern.
26-42:Details
✅ Verification successful
Verify the codec option value change.
The codec selector has been updated to use centralized state, which looks good. However, the empty string option value was changed to
"any".Please verify that this change from
value=""tovalue="any"is intentional and aligns with how codec preferences are handled elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Search for codec handling in the codebase to verify the "any" value usage rg -A 3 -B 3 "codec.*any|any.*codec" --type tsLength of output: 791
No action needed: “any” codec option is consistent with existing logic
The codebase already defines the default codec as
'any'insrc/lib/store.ts, andsrc/modules/setAudioStreams.tscorrectly treatsstate.codec === 'any'as “choose best available codec.” This update fromvalue=""tovalue="any"aligns with how codec preferences are handled elsewhere.src/scripts/library.ts (3)
3-3: LGTM: Centralized state import aligns with refactoring objectives.The addition of
stateandstoreimports from the centralized store module is consistent with the PR's goal of unifying localStorage state handling.
52-52: LGTM: Direct DOM API usage simplifies dependencies.Replacing utility function calls with direct
document.createElementis consistent with the broader refactoring pattern seen across the codebase.
61-61: LGTM: Direct localStorage access for library cleanup.Using
localStorage.removeItem('library')directly is appropriate for the clean library operation and aligns with removing utility function dependencies.src/scripts/theme.ts (7)
3-3: LGTM: Centralized state import consistent with refactoring.The import change aligns perfectly with the unified state management approach.
89-89: LGTM: Direct state access replaces utility function.Using
state.themeinstead ofgetSaved('theme')is consistent with the centralized state management refactoring.
112-112: LGTM: Custom theme access updated consistently.The change to
state.customThememaintains the same functionality while using the centralized state.
114-114: LGTM: Load image setting accessed from centralized state.The conditional logic correctly uses
state.loadImagefor determining whether to load images.
116-116: LGTM: Default export access for color extraction module.The change from named export to default export access is appropriate for the module structure.
128-129: LGTM: Optimized CSS variable setting.The conditional check prevents unnecessary CSS variable setting when the roundness value matches the default, which is a good performance optimization.
131-131: LGTM: Consistent state access for image loading flag.Using
state.loadImageis consistent with the centralized state management pattern.src/components/SuperCollectionList.ts (3)
7-7: LGTM: Centralized state management imports.The import of
setStateandstatealigns with the unified state handling approach.
10-10: LGTM: Direct state access for default super collection.Using
state.defaultSuperCollectiondirectly replaces the previousgetSavedcall and maintains type safety with the cast toSuperCollection.
30-32: LGTM: Simplified state management in event listener.The refactored logic using
setState('defaultSuperCollection', elm.value)is much cleaner than the previous conditional save/remove pattern and maintains the same functionality.src/stylesheets/remixicon.css (2)
10-10: LGTM: Improved font-face declaration.The explicit
format("woff2")specification and consistent double quotes improve font loading behavior and maintain CSS consistency.
221-243: LGTM: New icon classes support cloud sync functionality.The addition of cloud sync icons (
.ri-cloud-off-fill,.ri-cloud-fill) and reorganization of existing icons properly supports the new cloud sync feature implemented in the library component. The.ri-download-2-fillreplacement also aligns with updated icon usage across components.src/components/Settings/parental.ts (4)
4-4: LGTM: Clean import update for centralized state management.The import change correctly replaces the old localStorage utility functions with the new centralized state management system.
12-16: LGTM: Consistent state-based initialization.The parental controls initialization correctly uses the new
state.partsManagerPINinstead of the old localStorage-based approach. The boolean check properly determines if parental controls are enabled.
29-52: LGTM: Proper PIN validation and state updates.The toggle handler correctly:
- Uses
state.partsManagerPINfor PIN validation- Iterates through all state keys to disable parts starting with 'part '
- Uses
setStateto clear the PIN and update individual parts- Maintains the same UX flow with prompts and page reload
The logic is sound and consistent with the new state management approach.
58-58: LGTM: Correct state key mapping for part toggles.The toggle switch correctly uses the new state key format
'part ' + item.nameinstead of the old localStorage key format. The negation logic is preserved to maintain the same UI behavior.src/modules/cloudSync.ts (5)
4-8: LGTM: Clean module structure and helper functions.The module exports a well-structured function that takes the necessary parameters. The
hashpointURL construction andclshelper function for icon states are appropriately designed.
9-28: LGTM: Robust sync function with proper error handling.The sync function includes:
- Proper loading state management
- Correct HTTP POST with JSON content type
- Appropriate error handling with visual feedback
- Prevention of redundant sync operations
The implementation handles both success and error cases appropriately.
30-34: LGTM: Clean fetch function implementation.The
fetchFromCloudfunction is concise and correctly:
- Fetches data from the cloud endpoint
- Parses JSON response
- Saves to local database with 'cloud' source tag to prevent sync loops
36-45: LGTM: Smart debounced sync with cloud change detection.The event listener implementation is well thought out:
- Ignores changes originating from cloud to prevent sync loops
- Uses debounced timeout to batch rapid changes
- Properly clears previous timeouts
- Updates UI to indicate pending sync
61-70: LGTM: Thoughtful initialization with user choice.The initialization logic appropriately:
- Checks if local database is empty
- Prompts user for import decision
- Handles both import and existing data scenarios
- Provides proper error messaging for missing cloud data
- Sets appropriate final icon state
src/components/ActionsMenu.ts (8)
4-5: LGTM: Clean import update for centralized state management.The import changes correctly replace the old localStorage utilities and DOM selector utility with the new centralized state management system.
48-56: LGTM: Consistent state-based conditional rendering.The conditional check correctly uses
state['part Start Radio']instead of the old!getSaved(...)pattern, maintaining the same functionality with the new state system.
62-62: LGTM: Native DOM API usage.The change from
$('a')todocument.createElement('a')is consistent with removing the DOM utility in favor of native APIs.
70-70: LGTM: Icon style update.The change from
"ri-download-2-line"to"ri-download-2-fill"appears to be part of a design update to use filled icon variants.
74-96: LGTM: Consistent state-based conditional rendering.The conditional check correctly uses
state['part View Author']instead of the old localStorage-based approach, maintaining the same functionality.
104-114: LGTM: Improved dialog element management.The change to
document.createElement('dialog')with proper cleanup (addingdialog.remove()in the event listener) is a good improvement over the old DOM utility approach.
120-138: LGTM: Consistent state usage for watch functionality.The changes correctly:
- Use
state['part Watch On']for conditional rendering- Use
state.linkHostinstead ofgetSaved('linkHost')- Maintain the same logic flow with improved state management
145-154: LGTM: Consistent native DOM API usage.The change to
document.createElement('dialog')with proper event handling maintains the same functionality while using native DOM APIs consistently.src/components/Settings/app.ts (6)
5-5: LGTM: Clean import update for centralized state management.The import change correctly replaces the old localStorage utility functions with the new centralized state management system.
19-36: LGTM: Simplified custom instance handling with proper state management.The custom instance toggle correctly:
- Uses
state.customInstancefor checked state- Sets empty string instead of removing saved data (consistent with state approach)
- Uses
setStatefor persistence- Maintains the same user interaction flow
38-52: LGTM: Consistent language selector implementation.The language selector correctly uses
setStatefor persistence and maintains the same functionality with the new state system.
54-75: LGTM: Proper link host handling with dual updates.The link host selector correctly:
- Updates both
store.linkHost(for immediate use) andstate.linkHost(for persistence)- Uses conditional logic to handle empty vs. selected values
- Maintains the same initialization behavior using
state.linkHost
77-94: LGTM: Consistent download format selector implementation.The download format selector correctly uses
state.dlFormatfor both initialization and updates viasetState.
96-111: LGTM: Consistent share action selector implementation.The share action selector correctly uses
state.shareActionfor both initialization and updates viasetState. The removal of the "always ask" option appears to be an intentional simplification.src/lib/store.ts (1)
138-140: LGTM! Good integration with existing store.The integration of the new state object with existing store properties looks clean and maintains backward compatibility while enabling centralized state management.
Also applies to: 157-157
| async function importSettings(e: Event) { | ||
| e.preventDefault(); | ||
| const newSettings = JSON.parse( | ||
| await ( | ||
| (e.target as HTMLInputElement).files as FileList | ||
| )[0].text() | ||
| ); | ||
|
|
||
| if (confirm('This will merge your current settings with the imported settings, continue?')) { | ||
| for (const key in newSettings) | ||
| save(key, newSettings[key]); | ||
|
|
||
| location.reload(); | ||
| } | ||
| const newSettings = await ( | ||
| (e.target as HTMLInputElement).files as FileList | ||
| )[0].text(); | ||
|
|
||
| if (confirm('This will overwrite your current settings with the imported settings, continue?')) | ||
| localStorage.setItem('store', newSettings); | ||
| } |
There was a problem hiding this comment.
Add JSON validation for imported settings.
The imported settings are directly saved to localStorage without validation. This could break the application if the JSON is malformed or contains unexpected data.
Add validation:
async function importSettings(e: Event) {
e.preventDefault();
const newSettings = await (
(e.target as HTMLInputElement).files as FileList
)[0].text();
- if (confirm('This will overwrite your current settings with the imported settings, continue?'))
- localStorage.setItem('store', newSettings);
+ if (confirm('This will overwrite your current settings with the imported settings, continue?')) {
+ try {
+ const parsed = JSON.parse(newSettings);
+ // Validate that it's an object
+ if (typeof parsed === 'object' && parsed !== null) {
+ localStorage.setItem('store', newSettings);
+ location.reload();
+ } else {
+ alert('Invalid settings file format');
+ }
+ } catch (error) {
+ alert('Failed to parse settings file: ' + error.message);
+ }
+ }
}📝 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.
| async function importSettings(e: Event) { | |
| e.preventDefault(); | |
| const newSettings = JSON.parse( | |
| await ( | |
| (e.target as HTMLInputElement).files as FileList | |
| )[0].text() | |
| ); | |
| if (confirm('This will merge your current settings with the imported settings, continue?')) { | |
| for (const key in newSettings) | |
| save(key, newSettings[key]); | |
| location.reload(); | |
| } | |
| const newSettings = await ( | |
| (e.target as HTMLInputElement).files as FileList | |
| )[0].text(); | |
| if (confirm('This will overwrite your current settings with the imported settings, continue?')) | |
| localStorage.setItem('store', newSettings); | |
| } | |
| async function importSettings(e: Event) { | |
| e.preventDefault(); | |
| const newSettings = await ( | |
| (e.target as HTMLInputElement).files as FileList | |
| )[0].text(); | |
| if (confirm('This will overwrite your current settings with the imported settings, continue?')) { | |
| try { | |
| const parsed = JSON.parse(newSettings); | |
| // Validate that it's an object | |
| if (typeof parsed === 'object' && parsed !== null) { | |
| localStorage.setItem('store', newSettings); | |
| location.reload(); | |
| } else { | |
| alert('Invalid settings file format'); | |
| } | |
| } catch (error) { | |
| alert('Failed to parse settings file: ' + (error instanceof Error ? error.message : error)); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/Settings/index.ts around lines 52 to 60, the imported settings
JSON is saved directly to localStorage without validation, risking application
errors if the JSON is malformed or invalid. Modify the importSettings function
to parse the imported text as JSON inside a try-catch block, validate its
structure if needed, and only save it to localStorage if parsing succeeds. If
parsing fails, show an error message to the user and do not overwrite the
current settings.
| export function setState<K extends keyof AppSettings>(key: K, val: AppSettings[K]) { | ||
| state[key] = val; | ||
| const str = JSON.stringify(state); | ||
| localStorage.setItem('store', str); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider performance optimizations for setState.
The setState function lacks error handling for localStorage operations and could be inefficient for frequent updates.
export function setState<K extends keyof AppSettings>(key: K, val: AppSettings[K]) {
state[key] = val;
- const str = JSON.stringify(state);
- localStorage.setItem('store', str);
+ try {
+ const str = JSON.stringify(state);
+ localStorage.setItem('store', str);
+ } catch (error) {
+ console.error('Failed to save state to localStorage:', error);
+ // Could implement fallback strategy or notify user
+ }
}For performance optimization, consider implementing debounced saves for frequent updates:
let saveTimeout: number;
export function setState<K extends keyof AppSettings>(key: K, val: AppSettings[K]) {
state[key] = val;
// Debounce localStorage writes
clearTimeout(saveTimeout);
saveTimeout = setTimeout(() => {
try {
localStorage.setItem('store', JSON.stringify(state));
} catch (error) {
console.error('Failed to save state to localStorage:', error);
}
}, 100);
}🤖 Prompt for AI Agents
In src/lib/store.ts around lines 56 to 60, the setState function currently
writes to localStorage immediately without error handling and can cause
performance issues with frequent updates. Modify setState to debounce the
localStorage writes by using a timeout to delay saving for 100ms after the last
update. Wrap the localStorage.setItem call in a try-catch block to handle and
log any errors during saving. This will improve performance and add robustness
to the function.
| const savedStore = localStorage.getItem('store'); | ||
| if (savedStore) | ||
| state = JSON.parse(savedStore); |
There was a problem hiding this comment.
Add validation and error handling for localStorage state loading.
The current implementation directly assigns the parsed localStorage value without validation, which could cause runtime errors if the stored state has a different structure or invalid data.
const savedStore = localStorage.getItem('store');
-if (savedStore)
- state = JSON.parse(savedStore);
+if (savedStore) {
+ try {
+ const parsed = JSON.parse(savedStore);
+ // Validate and merge with default state to handle schema changes
+ state = { ...state, ...parsed };
+ } catch (error) {
+ console.warn('Failed to parse stored state, using defaults:', error);
+ // Optionally clear invalid localStorage data
+ localStorage.removeItem('store');
+ }
+}📝 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.
| const savedStore = localStorage.getItem('store'); | |
| if (savedStore) | |
| state = JSON.parse(savedStore); | |
| const savedStore = localStorage.getItem('store'); | |
| if (savedStore) { | |
| try { | |
| const parsed = JSON.parse(savedStore); | |
| // Validate and merge with default state to handle schema changes | |
| state = { ...state, ...parsed }; | |
| } catch (error) { | |
| console.warn('Failed to parse stored state, using defaults:', error); | |
| // Optionally clear invalid localStorage data | |
| localStorage.removeItem('store'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/store.ts around lines 51 to 53, the code directly parses and assigns
the localStorage item to state without validation, risking runtime errors if the
data is invalid or malformed. Modify the code to wrap the JSON.parse call in a
try-catch block to handle parsing errors gracefully. Additionally, validate the
parsed object to ensure it matches the expected state structure before assigning
it to state, and fallback to a default state if validation fails.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Settings/library.ts (1)
31-62:⚠️ Potential issueCritical: Fix async hash creation logic.
There's a critical issue with the dbsync handler. The
setState('dbsync', hash)call on line 61 executes immediately, buthashis only set inside the async Promise callback ofcrypto.subtle.digest(). This means the state will always be set to an empty string, not the computed hash.Move the
setStatecall inside the Promise callback:function hashCreator(text: string) { const msgBuffer = new TextEncoder().encode(text); crypto.subtle.digest('SHA-256', msgBuffer) .then(hashBuffer => { hash = Array .from(new Uint8Array(hashBuffer)) .map(b => b.toString(16).padStart(2, '0')) .join(''); + setState('dbsync', hash); notify(i18n('settings_reload')); }); } if (!state.dbsync) { // ... existing logic } - setState('dbsync', hash);
🧹 Nitpick comments (1)
src/components/Settings/library.ts (1)
75-75: Consider replacing delete operator for better performance.Static analysis correctly flags the use of
deleteoperator which can impact performance. Consider using undefined assignment instead.Apply these changes for better performance:
- delete db.discover; + db.discover = undefined;- delete db.history; + db.history = undefined;Also applies to: 95-95
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
package.json(1 hunks)src/components/Settings/app.ts(6 hunks)src/components/Settings/library.ts(3 hunks)src/components/Settings/parental.ts(4 hunks)src/components/Settings/personalize.ts(5 hunks)src/components/Settings/playback.ts(3 hunks)src/components/Settings/search.ts(2 hunks)src/locales/en.json(1 hunks)src/modules/cloudSync.ts(1 hunks)src/scripts/queue.ts(7 hunks)src/scripts/search.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/scripts/search.ts
- src/modules/cloudSync.ts
- package.json
- src/scripts/queue.ts
- src/components/Settings/parental.ts
- src/components/Settings/app.ts
- src/components/Settings/personalize.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/Settings/search.ts (3)
src/lib/store.ts (2)
state(4-46)setState(56-60)src/lib/utils.ts (1)
notify(57-65)src/scripts/i18n.ts (1)
i18n(44-49)
src/components/Settings/library.ts (4)
src/lib/store.ts (2)
state(4-46)setState(56-60)src/lib/utils.ts (1)
notify(57-65)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/libraryUtils.ts (2)
getDB(8-8)saveDB(10-13)
🪛 Biome (1.9.4)
src/components/Settings/library.ts
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 95-95: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (10)
src/components/Settings/search.ts (3)
4-6: Excellent refactoring to centralized state management.The migration from scattered localStorage calls to the unified
stateandsetStateAPI is well-executed. The imports are clean and the code now follows the centralized state management pattern consistently.
18-25: Toggle logic correctly implemented.The searchFilter toggle correctly reads from
state.searchFilterand updates usingsetState(). The logic properly toggles between an empty string and'music_songs'while navigating to/searchafterward.
31-36: Improved user experience with notification instead of forced reload.The searchSuggestions toggle correctly uses the reactive state and replaces the previous forced page reload with a user-friendly notification. This is a better UX pattern that allows users to control when they want to reload.
src/components/Settings/playback.ts (4)
5-6: Clean import structure for centralized state management.The imports correctly bring in the unified state management (
setState,state) and utility functions (notify,quickSwitch), establishing a clean foundation for the refactored component.
19-24: Audio quality toggle correctly refactored.The HQ audio toggle properly reads from
state.hqand updates viasetState(). ThequickSwitch()call ensures immediate application of the setting change.
40-40: Good alignment with state defaults.Changing the codec option value from an empty string to
"any"aligns with the default value in the centralized state object (line 27 insrc/lib/store.ts), ensuring consistency across the application.
68-73: Improved UX by replacing forced reload with notification.The HLS toggle now uses a notification to inform users about the need to reload instead of forcing an immediate page reload. This gives users control over when to apply the change and is a better user experience pattern.
src/components/Settings/library.ts (3)
4-6: Excellent migration to centralized state management.The imports correctly establish the unified state management pattern, replacing the previous scattered localStorage approach with a clean, centralized system.
19-25: startupTab toggle logic is now consistent.The startupTab comparison logic is correct and consistent - both the
checkedproperty (line 19) and the handler comparison (line 22) properly use/librarywith the leading slash. This appears to address the previous review concern.
68-82: Discover and history toggles correctly implemented.Both toggles properly integrate with the centralized state management and handle the data clearing logic appropriately. The user confirmation flow and state updates are well-structured.
Also applies to: 88-101
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Settings/Settings.css (2)
5-6: Simplify padding declarations for clarity.You can consolidate
padding: 5%andpadding-top: 0into a single shorthand (for example,padding: 0 5% 5% 5%) to avoid overriding values and make the intent clearer.
15-23: Refine sticky header styling for readability and consistency.
- Add spacing around the child combinator (
> h3→> h3) to improve selector readability.- Consider using fixed units (e.g.,
remorpx) instead of%for vertical padding so the header height remains consistent across different screen sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Settings/Settings.css(2 hunks)src/components/Settings/app.ts(5 hunks)src/lib/utils.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/utils.ts
- src/components/Settings/app.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/setAudioStreams.ts (1)
10-12: Consider more robust prefetch detectionThe prefetch detection logic
!receiver.parentNodecould be fragile. Consider making the prefetch parameter explicit rather than inferring it from DOM state.-export default async function(audioStreams: AudioStream[], - isLive = false, - receiver: HTMLAudioElement = audio -) { - const prefetch = !receiver.parentNode; +export default async function(audioStreams: AudioStream[], + isLive = false, + receiver: HTMLAudioElement = audio, + prefetch = false +) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/player.ts(5 hunks)src/lib/utils.ts(6 hunks)src/modules/setAudioStreams.ts(2 hunks)src/scripts/audioEvents.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/player.ts
- src/lib/utils.ts
- src/scripts/audioEvents.ts
🔇 Additional comments (4)
src/modules/setAudioStreams.ts (4)
2-2: LGTM: State centralization importGood addition of the centralized
stateimport, aligning with the broader refactoring objective.
41-41: LGTM: State centralization for HQ preferenceGood change to use the centralized
state.hqinstead ofstore.player.hq.
47-47: LGTM: Receiver parameter usage with prefetch flagProper use of the receiver parameter and passing the prefetch flag to
proxyHandler.
6-9:Details
❓ Verification inconclusive
Verify backward compatibility for async function change
The function signature change from synchronous to asynchronous is a breaking change that could affect existing callers.
Please run this script to identify all callers and ensure they properly await this function:
🏁 Script executed:
#!/bin/bash # Search for all calls to setAudioStreams function ast-grep --pattern 'setAudioStreams($$$)'Length of output: 43
Let's broaden our search for callers using ripgrep to catch all invocation sites:
🏁 Script executed:
#!/bin/bash # Search for all occurrences of setAudioStreams( in .ts/.tsx files rg -n 'setAudioStreams\(' --glob '*.ts' --glob '*.tsx'Length of output: 56
No internal call sites detected for setAudioStreams – please verify external callers expect async behavior
I searched the entire codebase for
setAudioStreams(in TS/TSX files and found no invocations. Since converting this export toasyncis a breaking change for any existing consumers, please:
- Confirm whether this function is imported or called by other modules, scripts, or external repositories not covered by our search.
- If so, ensure all callers now properly
awaitthe returned promise or handle the async nature.
| title.textContent = i18n('player_audiostreams_setup'); | ||
|
|
||
| const preferedCodec = store.player.codec; | ||
| const preferedCodec = state.codec === 'any' ? ((await store.player.supportsOpus) ? 'opus' : 'aac') : state.codec; |
There was a problem hiding this comment.
Add error handling for async codec detection
The codec detection logic lacks error handling and could fail if store.player.supportsOpus is undefined or throws an error.
- const preferedCodec = state.codec === 'any' ? ((await store.player.supportsOpus) ? 'opus' : 'aac') : state.codec;
+ let preferedCodec: string;
+ if (state.codec === 'any') {
+ try {
+ preferedCodec = (await store.player.supportsOpus) ? 'opus' : 'aac';
+ } catch (error) {
+ console.warn('Failed to detect opus support, defaulting to aac:', error);
+ preferedCodec = 'aac';
+ }
+ } else {
+ preferedCodec = state.codec;
+ }📝 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.
| const preferedCodec = state.codec === 'any' ? ((await store.player.supportsOpus) ? 'opus' : 'aac') : state.codec; | |
| let preferedCodec: string; | |
| if (state.codec === 'any') { | |
| try { | |
| preferedCodec = (await store.player.supportsOpus) ? 'opus' : 'aac'; | |
| } catch (error) { | |
| console.warn('Failed to detect opus support, defaulting to aac:', error); | |
| preferedCodec = 'aac'; | |
| } | |
| } else { | |
| preferedCodec = state.codec; | |
| } |
🤖 Prompt for AI Agents
In src/modules/setAudioStreams.ts at line 14, the async codec detection using
store.player.supportsOpus lacks error handling. Wrap the await call in a
try-catch block to handle potential errors or undefined values gracefully. In
the catch block, set a default codec or fallback value to ensure the code
continues to function without crashing.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Settings/library.ts (1)
31-61:⚠️ Potential issueFix logic error and improve security in dbsync handler.
The current implementation has several issues:
Logic error: The
hashvariable is only populated when!state.dbsyncis true, butsetState('dbsync', hash)is called unconditionally, potentially setting an empty string when toggling off.Security concern: Concatenating username+password directly without salt is not a secure hashing practice.
Unclear flow: The conditional hash creation makes the logic hard to follow.
Apply this diff to fix the logic:
handler: async e => { - let hash = ''; - async function hashCreator(text: string) { - const msgBuffer = new TextEncoder().encode(text); - const hashBuffer = await crypto.subtle.digest('SHA-256', msgBuffer); - - hash = Array - .from(new Uint8Array(hashBuffer)) - .map(b => b.toString(16).padStart(2, '0')) - .join(''); - - notify(i18n('settings_reload')); - } - if (!state.dbsync) { + if (!state.dbsync) { + // Enabling sync + let hash = ''; const termsAccepted = confirm('Data will be automatically deleted after one week of inactivity.\nytify is not responsible for data loss.\n\nI Understand'); if (termsAccepted) { const username = prompt('Enter Username :'); if (username) { const password = prompt('Enter Password :'); const confirmpw = prompt('Confirm Password :'); if (password && password === confirmpw) { - await hashCreator(username + password); + const msgBuffer = new TextEncoder().encode(username + password); + const hashBuffer = await crypto.subtle.digest('SHA-256', msgBuffer); + hash = Array + .from(new Uint8Array(hashBuffer)) + .map(b => b.toString(16).padStart(2, '0')) + .join(''); + setState('dbsync', hash); + notify(i18n('settings_reload')); } else alert('Incorrect Information!'); } } e.preventDefault(); + } else { + // Disabling sync + setState('dbsync', ''); + notify(i18n('settings_reload')); } - setState('dbsync', hash); }Security recommendation: Consider adding a salt to the hash for better security:
const salt = crypto.getRandomValues(new Uint8Array(16)); const combined = username + password + Array.from(salt).join('');
🧹 Nitpick comments (2)
src/components/Settings/library.ts (2)
67-82: Replace delete operator for better performance.The static analysis correctly flags the delete operator usage which can impact performance.
Apply this diff to improve performance:
if (confirm(i18n("settings_clear_discoveries", count.toString()))) { - delete db.discover; + db.discover = undefined; saveDB(db);🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
87-101: Replace delete operator for better performance.Similar to the discover toggle, the delete operator should be replaced for better performance.
Apply this diff to improve performance:
if (confirm(i18n("settings_clear_history", count.toString()))) { - delete db.history; + db.history = undefined; saveDB(db);🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Settings/library.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Settings/library.ts
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
src/components/Settings/library.ts (2)
4-6: LGTM! Clean import refactor for centralized state management.The imports correctly bring in the new centralized state management (
setState,state) and notification utilities, aligning with the broader refactor to replace scattered localStorage access.
19-26: LGTM! Consistent state-based toggle implementation.The startupTab toggle now correctly uses the centralized state for both checked status and state updates. The string comparison consistency issue from the previous review has been properly addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Settings/index.ts (1)
55-60:⚠️ Potential issueAdd JSON validation for imported settings.
The imported settings are still directly saved to localStorage without validation, which could break the application if the JSON is malformed or contains unexpected data. This issue was previously identified and remains unaddressed.
The validation should be added as suggested in the previous review to ensure robust error handling during settings import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Settings/index.ts(4 hunks)src/components/Settings/parental.ts(3 hunks)src/lib/libraryUtils.ts(1 hunks)src/modules/partsManager.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Settings/parental.ts
- src/modules/partsManager.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/Settings/index.ts (1)
src/lib/store.ts (1)
params(2-2)
src/lib/libraryUtils.ts (1)
src/lib/store.ts (1)
store(63-158)
🔇 Additional comments (5)
src/components/Settings/index.ts (5)
4-4: LGTM: Centralized state management import.The import of
paramsfrom the store module aligns with the centralized state management approach mentioned in the AI summary.
13-13: LGTM: Async function conversion for parental component.The conversion to an async function and awaiting the
parental()component is correct and necessary for handling the async parental component properly.Also applies to: 15-15, 26-26
47-47: LGTM: Simplified export settings implementation.The direct DOM element creation and simplified localStorage access using the centralized 'store' key is a good improvement that aligns with the unified state management approach.
Also applies to: 49-49
67-67: LGTM: Simplified emergency reset.The change to only remove the 'store' key instead of clearing all localStorage (except 'library') is more precise and aligns with the centralized state management approach.
75-78: LGTM: Simplified restore button handler.The inline event handler that directly removes the 'store' key and reloads the page is cleaner and more straightforward than the previous multi-step approach.
| }); | ||
|
|
||
| store.list.id = collection; | ||
| store.list.id = decodeURI(collection); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good consistency improvement, but verify usage across codebase.
This change improves consistency by storing the decoded collection ID in store.list.id, which aligns with how the function uses decodeURI(collection) for database lookup on line 151. Storing data in its decoded form is generally preferable for internal state management.
Please verify that other parts of the codebase don't expect store.list.id to be URI-encoded:
🏁 Script executed:
#!/bin/bash
# Description: Check if other code expects store.list.id to be URI-encoded
# Expected: No usage that applies URI encoding to store.list.id or treats it as encoded
# Search for usage of store.list.id
rg -A 3 -B 3 "store\.list\.id"
# Search for any URI encoding operations on store.list
rg -A 3 -B 3 "encodeUR.*store\.list|store\.list.*encodeUR"Length of output: 3354
Ensure all store.list.id assignments use decoded values
To maintain consistency with decodeURI(collection) in libraryUtils.ts, wrap other direct assignments of store.list.id in decodeURI(...).
• In src/modules/fetchList.ts
- Replace:
With:
store.list.id = url.slice(type === 'playlist' ? 11 : 9);
store.list.id = decodeURI(url.slice(type === 'playlist' ? 11 : 9));
- Replace:
With:
store.list.id = id.slice(9);
store.list.id = decodeURI(id.slice(9));
• (Optional) In src/modules/supermix.ts, the literal assignment store.list.id = 'supermix' remains safe, but if you ever parameterize it, apply decodeURI as well.
This ensures downstream lookups (e.g., in src/scripts/list.ts, router.ts, and utility calls) consistently operate on decoded IDs.
🤖 Prompt for AI Agents
In src/lib/libraryUtils.ts at line 195, you updated store.list.id to use
decodeURI(collection) for consistency with database lookups. To maintain this
consistency across the codebase, update all other direct assignments to
store.list.id to wrap the assigned value in decodeURI as well. Specifically, in
src/modules/fetchList.ts, replace assignments like store.list.id =
url.slice(...) and store.list.id = id.slice(...) with decodeURI wrapped
versions. Also, optionally consider applying decodeURI to any future
parameterized assignments like in src/modules/supermix.ts. This ensures all
store.list.id values are stored decoded for consistent downstream usage.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style
Chores
Localization