Complete uhtml Porting#300
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes refactor queue and library UI management to use declarative rendering with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (index.html)
participant Queue (store.queue)
participant LibraryActions
participant SettingsDialog
User->>UI (index.html): Clicks "Settings" (now opens dialog)
UI (index.html)->>SettingsDialog: Show dialog with settings form
User->>LibraryActions: Clicks import/export/clean
LibraryActions->>LibraryActions: Handles action via uhtml-rendered controls
User->>Queue: Adds/removes/filters items
Queue->>Queue: Updates queue.list, re-renders controls with uhtml
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ 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: 4
🧹 Nitpick comments (4)
src/scripts/i18n.ts (1)
44-49: LGTM! Well-designed centralized i18n function.The function properly centralizes translation logic with good TypeScript typing and safe optional chaining. The placeholder substitution using
replace('$', value)is simple and effective.Consider documenting that only the first occurrence of
$will be replaced if multiple placeholders are needed in the future:+// Note: Only replaces the first occurrence of '$' with the provided value export const i18n = ( key: TranslationKeys, value: string = '' ) => value ? (json?.[key] || key).replace('$', value) : json?.[key] || key;src/lib/utils.ts (1)
9-9: LGTM! Proper centralization of i18n functionality.The import change successfully moves from the local i18n implementation to the centralized function. This aligns with the broader refactoring effort mentioned in the AI summary.
Consider removing the
.tsextension from the import path for consistency with other imports in the file:-import { i18n } from "../scripts/i18n.ts"; +import { i18n } from "../scripts/i18n";src/scripts/list.ts (2)
29-37: Consider adding validation for dataset properties.The function logic is correct, but it could benefit from validating that
item.datasetcontains the expected properties before appending.function listToQ(container: HTMLDivElement) { const items = container.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>; items.forEach(item => { - store.queue.append(item.dataset); + if (item.dataset && item.dataset.id) { + store.queue.append(item.dataset); + } }); goTo('/upcoming'); }
49-53: Consider using a consistent queue clearing approach.The queue clearing uses both
store.queue.list.length = 0andqueuelist.innerHTML = ''. This dual approach could be encapsulated into a single method for consistency across the codebase.Would you like me to suggest implementing a
clearQueue()method in the store to handle both operations atomically?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
index.html(3 hunks)package.json(1 hunks)src/components/ActionsMenu.ts(2 hunks)src/components/CollectionSelector.ts(1 hunks)src/components/ItemsLoader.ts(1 hunks)src/components/Selector.ts(1 hunks)src/components/Settings.tsx(1 hunks)src/components/UpdatePrompt.ts(1 hunks)src/components/WatchVideo.ts(1 hunks)src/index.d.ts(1 hunks)src/lib/store.ts(2 hunks)src/lib/utils.ts(1 hunks)src/main.ts(0 hunks)src/modules/enqueueRelatedStreams.ts(2 hunks)src/modules/fetchList.ts(1 hunks)src/modules/importPipedPlaylists.ts(1 hunks)src/modules/listUtils.ts(1 hunks)src/modules/partsManager.ts(1 hunks)src/modules/setAudioStreams.ts(1 hunks)src/modules/start.ts(1 hunks)src/scripts/audioEvents.ts(3 hunks)src/scripts/i18n.ts(1 hunks)src/scripts/library.ts(1 hunks)src/scripts/list.ts(3 hunks)src/scripts/queue.ts(3 hunks)src/scripts/router.ts(1 hunks)src/scripts/search.ts(1 hunks)src/stylesheets/global.css(1 hunks)src/stylesheets/header.css(1 hunks)src/stylesheets/layout.css(2 hunks)src/stylesheets/library.css(1 hunks)src/stylesheets/nav.css(1 hunks)src/stylesheets/remixicon.css(1 hunks)src/stylesheets/search.css(3 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/main.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/modules/enqueueRelatedStreams.ts (1)
src/lib/store.ts (1)
store(5-108)
src/components/WatchVideo.ts (1)
src/lib/store.ts (1)
store(5-108)
src/components/ActionsMenu.ts (2)
src/lib/store.ts (1)
store(5-108)src/scripts/i18n.ts (1)
i18n(44-49)
src/scripts/audioEvents.ts (1)
src/lib/store.ts (1)
store(5-108)
src/scripts/queue.ts (4)
src/lib/dom.ts (1)
queuelist(23-23)src/lib/store.ts (2)
store(5-108)getSaved(3-3)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/utils.ts (3)
removeSaved(15-15)save(13-13)notify(62-70)
src/scripts/library.ts (3)
src/scripts/i18n.ts (1)
i18n(44-49)src/lib/libraryUtils.ts (3)
getDB(8-8)toCollection(28-45)saveDB(10-13)src/lib/utils.ts (2)
$(11-11)removeSaved(15-15)
🔇 Additional comments (44)
package.json (2)
16-16: Approve version bump for@netlify/blobs.Updating
@netlify/blobsto^9.1.2keeps us on the latest stable release and aligns with other Netlify packages.
18-18: Confirm compatibility of updated@types/node.The bump to
^22.15.21ensures up-to-date type definitions, but please verify that your Node.js runtime and CI environments support this version.vite.config.ts (2)
94-96: Simplify PostCSS plugins configuration.The configuration correctly removes unused
postcss-jit-props(OpenProps)plugin, leaving onlyautoprefixer()as intended.
94-96:Details
✅ Verification successful
Verify no residual references to removed CSS plugins.
Ensure there are no remaining
open-propsorpostcss-jit-propsusages in the codebase to prevent build failures or style regressions.
🏁 Script executed:
#!/bin/bash # Check for lingering references to removed plugins rg -n "open-props" --glob "*.{js,ts,css}" rg -n "postcss-jit-props" --glob "*.{js,ts,css}"Length of output: 90
🏁 Script executed:
#!/bin/bash # Broader search for any lingering plugin references across the repo rg -n "open-props" rg -n "postcss-jit-props" # Check package.json for leftover dependencies rg -n '"open-props"' package.json rg -n '"postcss-jit-props"' package.jsonLength of output: 123
No residual references to removed CSS plugins
A full search of the codebase and package.json found zero occurrences of
open-propsorpostcss-jit-props. All clear.src/stylesheets/nav.css (1)
35-36: LGTM! Good responsive navigation improvement.The addition of
flex: 1andtext-align: centereffectively redistributes space among navigation items and centers their content, which aligns well with the removal of the Settings navigation item mentioned in the AI summary.src/stylesheets/remixicon.css (1)
18-18: LGTM! Font rendering simplification.Replacing vendor-specific font smoothing properties with explicit
font-weight: 500simplifies the styling approach while maintaining consistent icon appearance. This aligns with the broader stylesheet cleanup mentioned in the AI summary.src/lib/utils.ts (1)
42-42: LGTM! Correct usage of centralized i18n function.The function call properly uses the new centralized i18n function with the appropriate translation key.
src/lib/store.ts (1)
23-27:Details
✅ Verification successful
Review the queue object design and stub implementations.
The refactoring to encapsulate queue operations in an object is good, but there are some concerns:
The
firstChildmethod returningHTMLElement | undefinedseems conceptually mismatched for a queue data structure - typically you'd expect to return queue items rather than DOM elements.The stub implementations don't match the type signatures and may cause runtime issues.
Consider whether
firstChildshould return queue data instead of DOM elements, or if this method belongs elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Find where the actual queue methods are implemented # Expected: Find the real implementations of queue.append and queue.firstChild methods rg -A 10 "queue\.append\s*=" --type ts rg -A 10 "queue\.firstChild\s*=" --type tsLength of output: 1260
Queue interface and implementations verified
The
queueobject insrc/lib/store.tsmerely declares the API surface forlist,appendandfirstChild. Their real implementations are assigned insrc/scripts/queue.ts, where:
store.queue.appendcorrectly handles DOM-based queueing logic.store.queue.firstChildreturns the first queue item’s HTMLElement (viaqueuelist.firstElementChild), matching its intended use.No changes are needed in
store.ts.src/components/CollectionSelector.ts (1)
4-4: LGTM! Consistent i18n import refactoring.The import path change aligns with the broader refactoring to centralize internationalization utilities in a dedicated script module.
src/components/Selector.ts (1)
2-2: LGTM! Consistent with i18n centralization.This import path change is part of the consistent refactoring to move i18n functionality to a dedicated script module.
src/components/UpdatePrompt.ts (1)
1-1: LGTM! Final consistent i18n import update.This completes the consistent refactoring of i18n imports across components to use the centralized script module.
src/components/Settings.tsx (1)
7-7: Consistent with the i18n centralization effort.The import path change aligns with the broader refactoring to centralize internationalization functionality. Given the extensive i18n usage in this settings component, ensure all translation calls continue working correctly.
src/modules/setAudioStreams.ts (1)
4-4: Refactoring aligns with i18n centralization.The import path change is consistent with the broader effort to extract internationalization functionality into a dedicated module.
src/modules/partsManager.ts (1)
3-3: Completes the i18n centralization refactoring.This final import path change completes the consistent migration of i18n functionality to a dedicated module across the codebase.
src/scripts/search.ts (1)
25-27: LGTM! DOM selector updated to match new HTML structure.The change from
#search>h1to#search>headercorrectly adapts to the restructured DOM layout mentioned in the PR objectives.src/modules/fetchList.ts (1)
3-3: Good refactoring! Centralized i18n import improves code organization.Moving the
i18nimport from utils to a dedicated scripts/i18n module enhances separation of concerns and maintainability.Also applies to: 7-7
src/stylesheets/layout.css (1)
52-52: Good CSS improvement for landscape layout consistency.Adding explicit
height: 100%ensures proper section sizing in landscape orientation, supporting the UI restructuring effort.src/modules/importPipedPlaylists.ts (1)
2-3: Consistent i18n import refactoring applied.Following the same pattern as other modules, this centralizes internationalization functionality for better code organization and maintainability.
src/scripts/router.ts (1)
11-11: LGTM! Settings route removal aligns with dialog-based architecture.The removal of '/settings' from the routes array is consistent with the architectural change to implement settings as a dialog instead of a dedicated route. This simplifies the routing logic while maintaining functionality.
src/modules/enqueueRelatedStreams.ts (2)
14-16: Clean formatting improvement.The arrow function formatting is more readable with the improved indentation.
30-35: Excellent refactoring to use centralized queue management.The replacement of
appendToQueuelistwithstore.queue.appendaligns with the queue management refactor. The object structure matches the expectedCollectionItemtype, ensuring type safety and consistency.src/modules/listUtils.ts (1)
5-6: Good separation of concerns with i18n import refactor.Moving the
i18nimport to its dedicated module in"../scripts/i18n"improves code organization and aligns with the broader internationalization refactor across the codebase.src/index.d.ts (1)
148-150: Excellent type safety improvement with specific event interface.The
elEvinterface provides better type safety by ensuring event targets are typed asHTMLElementrather than the genericEventTarget. This enables better IntelliSense and compile-time checking for event handlers that interact with DOM elements.src/stylesheets/search.css (2)
7-7: LGTM: Positioning adjustment for new layout structure.The updated
topcalculation properly accommodates the new header structure mentioned in the AI summary.
53-53: LGTM: Margin adjustment aligns with layout changes.The
margin-topcalculation has been adjusted to work with the restructured search interface components.src/components/WatchVideo.ts (1)
89-89: LGTM: Correct implementation of new queue interface.The change from
firstItemInQueue().click()tostore.queue.firstChild()?.click()properly uses the new queue object method with appropriate optional chaining, sincefirstChild()returnsHTMLElement | undefined.src/modules/start.ts (2)
7-7: LGTM: Centralized i18n import.Moving the
i18nimport to a dedicated module improves code organization and aligns with the refactoring effort.
8-9:Details
✅ Verification successful
Verify that side-effect imports are necessary.
The side-effect imports for
../scripts/libraryand../scripts/queuesuggest these modules contain initialization code that needs to run. Ensure these imports are required for proper functionality and that the modules don't have unintended side effects.
🏁 Script executed:
#!/bin/bash # Check what initialization code exists in the library and queue scripts echo "=== Checking library script for side effects ===" cat src/scripts/library.ts | head -20 echo -e "\n=== Checking queue script for side effects ===" cat src/scripts/queue.ts | head -20 echo -e "\n=== Searching for immediate function calls or event listeners ===" rg -A 3 -B 3 "addEventListener|document\.|window\." src/scripts/library.ts src/scripts/queue.tsLength of output: 3877
Side-effect imports are required and correctly used
I’ve checked both
src/scripts/library.tsandsrc/scripts/queue.tsand confirmed they register DOM event listeners and initialize UI elements (e.g.addEventListenercalls on buttons and queue list). The side-effect imports insrc/modules/start.tsare therefore necessary to kick off this setup. No changes needed.src/stylesheets/library.css (1)
23-26: LGTM: CSS simplification aligns with dynamic rendering.The simplification of
#libraryActionsstyles is appropriate for the shift to declarative UI rendering with uhtml. The remaining rule to hide input elements supports the new dynamic component structure.src/stylesheets/global.css (1)
11-11: LGTM! Clean CSS import addition.The new header.css import is properly positioned among other component imports and follows the established pattern.
src/components/ActionsMenu.ts (3)
5-5: LGTM! Import cleanup aligns with queue refactoring.Removing the
appendToQueuelistimport is correct since this functionality is now encapsulated instore.queue.append.
8-8: Good consolidation of i18n imports.Moving the i18n import to
../scripts/i18ncentralizes internationalization functionality, which improves maintainability.
31-31: Correct implementation of new queue interface.The transition from
appendToQueuelist(store.actionsMenu, true/false)tostore.queue.append(store.actionsMenu, true/false)properly uses the new queue object's append method. The method signature matches the interface defined instore.ts.Also applies to: 38-38
src/scripts/audioEvents.ts (3)
158-158: Correct adaptation to new queue structure.Changing from
store.queue[0]tostore.queue.list[0]properly accesses the first item in the queue's list array, matching the new queue object structure.
174-174: Proper use of new queue append method.The transition from
appendToQueuelist(store.stream, true)tostore.queue.append(store.stream, true)correctly uses the new encapsulated queue method for prepending items.
186-186: Good use of optional chaining with new queue method.Replacing
firstItemInQueue().click()withstore.queue.firstChild()?.click()properly uses the new queue method and includes safe optional chaining to prevent errors if no first child exists.src/stylesheets/header.css (1)
1-77: Excellent CSS structure for collapsible headers.This new stylesheet demonstrates several best practices:
- Semantic structure: Uses proper CSS nesting with
>selectors for clear parent-child relationships- Custom properties: Leverages CSS variables like
--onBg,--text,--borderfor consistent theming- Modern layout: Good use of flexbox for alignment and aspect-ratio for consistent sizing
- Smooth interactions: Proper transitions on transform and hover states
- Accessibility: Includes hover effects and proper focus management
- Responsive design: Uses viewport units (
dvmin) and relative units (rem)The collapsible details implementation with rotation animation is particularly well-executed.
src/scripts/list.ts (1)
1-2: LGTM! Import refactoring aligns with the new queue management approach.The import changes correctly reflect the shift to the new queue architecture and centralized i18n module.
Also applies to: 8-8
src/scripts/library.ts (2)
10-31: Excellent refactoring to declarative UI!The template-based approach with embedded event handlers is much cleaner and more maintainable than the previous imperative DOM manipulation.
46-52: LGTM! Clean implementation of export functionality.The export function correctly creates a downloadable JSON file with proper encoding.
index.html (2)
151-157: Good UI structure with collapsible details.The header structure with details element provides a clean, expandable interface for section actions.
299-331: Well-structured settings dialog.Moving settings to a dialog element improves separation of concerns and provides better modal behavior. All controls are properly preserved from the previous implementation.
src/scripts/queue.ts (2)
16-88: Excellent declarative UI implementation!The template-based approach with inline event handlers is much cleaner than the previous imperative code. The use of refs to capture button elements for later use is a good pattern.
94-135: Well-implemented queue management with proper validations.The append function correctly handles duplicates, filtering, and UI updates. Good use of document fragments for performance.
| import { generateImageUrl, getThumbIdFromLink } from "../lib/imageUtils"; | ||
| import { convertSStoHHMMSS, hostResolver, i18n } from "../lib/utils"; | ||
| import { convertSStoHHMMSS, hostResolver } from "../lib/utils"; | ||
| import { i18n } from "../scripts/i18n"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Clean separation of i18n functionality.
The import path change centralizes internationalization in a dedicated module, improving code organization while maintaining functionality.
Please verify the new i18n module exports the function correctly:
🏁 Script executed:
#!/bin/bash
# Description: Verify the new i18n module exists and exports the i18n function correctly
# Check if the new i18n module exists
if [ -f "src/scripts/i18n.ts" ] || [ -f "src/scripts/i18n.js" ]; then
echo "✓ i18n module exists"
cat src/scripts/i18n.{ts,js} 2>/dev/null | head -20
else
echo "✗ i18n module not found"
fi
# Verify i18n function calls still work in this file
echo "Checking i18n usage in ItemsLoader.ts:"
rg "i18n\(" src/components/ItemsLoader.ts -A 1 -B 1Length of output: 1030
i18n module missing named export
The new src/scripts/i18n.ts file exists but does not export an i18n function, so the import in src/components/ItemsLoader.ts will fail.
Please update the module to define and export i18n, for example:
• File: src/scripts/i18n.ts
import { getSaved } from '../lib/store';
// …existing code…
let json: Record<TranslationKeys, string> | undefined;
// after loading json:
export function i18n(key: TranslationKeys): string {
return json?.[key] ?? key;
}• Verify that TranslationKeys, Locales, and any helpers (attributeHandler, etc.) are correctly imported and referenced.
• Run a build or type-check to confirm the import in ItemsLoader.ts resolves without errors.
🤖 Prompt for AI Agents
In src/scripts/i18n.ts around line 1, the file currently does not export the
i18n function, causing the import in src/components/ItemsLoader.ts line 3 to
fail. Define the i18n function in src/scripts/i18n.ts that takes a
TranslationKeys argument and returns the corresponding translation string from
the loaded JSON or the key itself if missing. Export this function as a named
export. Also ensure that TranslationKeys, Locales, and any helper functions used
are properly imported and referenced in the module. After making these changes,
run a build or type-check to verify the import in ItemsLoader.ts resolves
without errors.
| async function importLibrary(e: Event) { | ||
| const importBtn = e.target as HTMLInputElement; | ||
| const newDB = JSON.parse(await (<FileList>importBtn.files)[0].text()); | ||
| const oldDB = getDB(); | ||
| if (!confirm(i18n('library_import_prompt'))) return; | ||
| for (const collection in newDB) for (const item in newDB[collection]) | ||
| toCollection(collection, newDB[collection][item], oldDB) | ||
| saveDB(oldDB); | ||
| location.reload(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add error handling for file operations.
The import function should handle potential failures in file reading and JSON parsing to provide better user feedback.
async function importLibrary(e: Event) {
const importBtn = e.target as HTMLInputElement;
- const newDB = JSON.parse(await (<FileList>importBtn.files)[0].text());
+ try {
+ const file = importBtn.files?.[0];
+ if (!file) return;
+ const text = await file.text();
+ const newDB = JSON.parse(text);
const oldDB = getDB();
if (!confirm(i18n('library_import_prompt'))) return;
for (const collection in newDB) for (const item in newDB[collection])
toCollection(collection, newDB[collection][item], oldDB)
saveDB(oldDB);
location.reload();
+ } catch (error) {
+ notify(i18n('library_import_error'));
+ console.error('Import failed:', error);
+ }
};🤖 Prompt for AI Agents
In src/scripts/library.ts around lines 35 to 44, the importLibrary function
lacks error handling for file reading and JSON parsing, which can cause
unhandled exceptions. Wrap the file reading and JSON.parse operations in a
try-catch block to catch any errors, and provide user feedback such as an alert
or console message if an error occurs. This will improve robustness and user
experience during import failures.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
src/components/Settings/playback.ts (2)
46-53: Simplify the codec selector initialization logic.The current logic for setting the initial codec selection is complex and could be simplified. The async/await pattern in
onmountis good, but the ternary logic could be clearer.Consider this cleaner approach:
onmount: async (target) => { const codecSaved = getSaved('codec'); - target.selectedIndex = codecSaved ? - parseInt(codecSaved) : - ((await store.player.supportsOpus) ? 0 : 1); + if (codecSaved) { + target.selectedIndex = parseInt(codecSaved); + } else { + // Default to Opus if supported, otherwise AAC + target.selectedIndex = (await store.player.supportsOpus) ? 0 : 1; + } store.player.codec = target.value as 'any'; },
66-72: Use consistent variable naming pattern.The use of
_as a variable name for the localStorage key is inconsistent with other parts of the codebase and could be confusing.Use a more descriptive variable name:
handler: () => { - const _ = 'stableVolume'; - if (getSaved(_)) - removeSaved(_); + const key = 'stableVolume'; + if (getSaved(key)) + removeSaved(key); else - save(_, 'true'); + save(key, 'true'); quickSwitch(); }src/components/Settings/parental.ts (2)
60-60: Type casting may not be safe.The type assertion
item.name as TranslationKeysassumes thatitem.nameis always a valid translation key, which may not be guaranteed at runtime.Consider adding runtime validation or using a type guard:
- name: item.name as TranslationKeys, + name: item.name as TranslationKeys, // TODO: Add runtime validation
32-46: PIN validation logic could be improved.The current implementation uses
prompt()which is blocking and may not provide the best UX. Also, there's no validation on PIN format or length.Consider implementing a custom modal for PIN entry with proper validation and better UX.
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/Settings/app.ts (1)
86-97: Type assertion may be unsafe.The type assertion
e.target.value as 'opus'and the assignment pattern assumes the value will always be a valid download format, which may not be guaranteed.Add runtime validation for download format:
handler: (e) => { - store.downloadFormat = e.target.value as 'opus'; + const format = e.target.value; + if (['opus', 'mp3', 'wav', 'ogg'].includes(format)) { + store.downloadFormat = format as 'opus'; + } else { + console.warn('Invalid download format:', format); + return; + } if (store.downloadFormat === 'opus') removeSaved('dlFormat'); else save('dlFormat', store.downloadFormat); },src/components/Settings/library.ts (2)
77-77: Avoid using the delete operator for performance.The static analysis tool correctly identifies that the
deleteoperator can impact performance, especially in frequently called code.Replace with undefined assignment:
- delete db.discover; + db.discover = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
97-97: Avoid using the delete operator for performance.Same performance concern as with the discover section.
Replace with undefined assignment:
- delete db.history; + db.history = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 97-97: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/components/Settings/personalize.ts (2)
17-29: Consider improving UX by avoiding page reload.The image loading toggle forces a page reload, which can be jarring to users. Consider implementing a more seamless way to apply the image loading preference without requiring a full page refresh.
- location.reload(); + // Consider implementing dynamic image loading toggle + // that updates the UI state without page reload
80-81: Ensure themer() is called after saving preference.The
themer()function is called before the preference is saved, which might cause timing issues. Consider reordering to save first, then apply the theme.- themer(); if (e.target.value === 'auto') removeSaved('theme'); else save('theme', e.target.value); + themer();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
index.html(4 hunks)package.json(1 hunks)src/components/Selector.ts(1 hunks)src/components/Settings.tsx(0 hunks)src/components/Settings/Settings.css(2 hunks)src/components/Settings/ToggleSwitch.ts(1 hunks)src/components/Settings/app.ts(1 hunks)src/components/Settings/index.ts(1 hunks)src/components/Settings/library.ts(1 hunks)src/components/Settings/parental.ts(1 hunks)src/components/Settings/personalize.ts(1 hunks)src/components/Settings/playback.ts(1 hunks)src/components/Settings/search.ts(1 hunks)src/lib/dom.ts(1 hunks)src/lib/store.ts(2 hunks)src/main.ts(1 hunks)src/modules/miniPlayer.ts(1 hunks)src/modules/start.ts(1 hunks)src/scripts/router.ts(3 hunks)src/stylesheets/header.css(1 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Settings.tsx
✅ Files skipped from review due to trivial changes (2)
- src/modules/miniPlayer.ts
- src/lib/dom.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main.ts
- src/components/Selector.ts
- package.json
- vite.config.ts
- src/modules/start.ts
- src/scripts/router.ts
- src/stylesheets/header.css
- src/lib/store.ts
- index.html
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/Settings/playback.ts (3)
src/scripts/i18n.ts (1)
i18n(44-49)src/lib/store.ts (2)
getSaved(3-3)store(5-108)src/lib/utils.ts (3)
removeSaved(15-15)save(13-13)quickSwitch(52-60)
src/components/Settings/parental.ts (3)
src/lib/store.ts (1)
getSaved(3-3)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/utils.ts (2)
removeSaved(15-15)save(13-13)
src/components/Settings/index.ts (4)
src/scripts/i18n.ts (1)
i18n(44-49)src/lib/dom.ts (1)
settingsContainer(46-46)src/lib/store.ts (2)
getSaved(3-3)params(1-1)src/lib/utils.ts (2)
save(13-13)$(11-11)
src/components/Settings/ToggleSwitch.ts (1)
src/scripts/i18n.ts (1)
i18n(44-49)
🪛 Biome (1.9.4)
src/components/Settings/parental.ts
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/Settings/library.ts
[error] 77-77: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 97-97: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (10)
src/components/Settings/Settings.css (2)
1-14: LGTM: Well-structured dialog styling with responsive design.The CSS changes appropriately transform the settings from a page section to a modal dialog. The responsive design with landscape media query and scrollable overflow are good practices for dialog UIs.
15-18: Good semantic change from h1 to h3.Changing the heading selector from
h1toh3is semantically correct for a dialog context where this won't be the main page heading.src/components/Settings/playback.ts (1)
99-99:Details
✅ Verification successful
Consider user experience impact of page reload.
The HLS setting triggers a full page reload, which disrupts the user experience. While this may be necessary for HLS changes to take effect, consider if there's a way to apply the change more gracefully.
Verify if the page reload is absolutely necessary for HLS changes, or if there's a way to reinitialize the player without a full reload:
🏁 Script executed:
#!/bin/bash # Description: Check if HLS initialization can be done without page reload # Search for HLS initialization and player setup code rg -A 10 -B 5 "hls.*init|player.*hls|HLS.*setup" --type tsLength of output: 8832
#!/bin/bash # Locate the usage of location.reload in playback settings rg -n "location.reload" src/components/Settings/playback.ts || echo "reload call not found"
#!/bin/bash # Search for page reload calls in playback settings rg -n "location\\.reload" -C2 src/components/Settings/playback.ts || echo "no reload call found"
🏁 Script executed:
#!/bin/bash # Show context around the page reload call in playback settings rg -n -C5 "location\\.reload" src/components/Settings/playback.tsLength of output: 308
Page reload is required for HLS toggle in current architecture
After inspecting the startup flow and HLS module:
- The HLS flag (
store.player.hls.on) is only read during initial module loading insrc/modules/start.ts, where it determines whether to dynamically import and initialize the HLS player or fall back to legacy audio streams.- There is no existing mechanism to tear down an active HLS instance (e.g. call
h.destroy()/detachMedia) and reinitialize it on the fly.- Simply toggling the saved “HLS” setting and calling
location.reload()is the simplest way to re-run the startup logic, re-import the HLS module when needed, or revert to non-HLS playback.If you want to avoid a full page reload in the future, you’ll need to:
- Store the Hls instance and expose teardown methods (destroy/detach).
- On toggle, call teardown and then re-import/initialize the HLS module or re-attach legacy streams.
For now, keeping the
location.reload()call is necessary to ensure the correct playback mode is applied.src/components/Settings/ToggleSwitch.ts (1)
4-9: Excellent type safety with proper event typing.The type definition for the handler parameter is well-structured, ensuring type safety for the event target. The use of
TranslationKeysfor the name parameter is also good for type safety.src/components/Settings/search.ts (3)
32-41: Good pattern for suggestions toggle with clear logic.The suggestions toggle logic is clear and correct: checked means suggestions are NOT 'off', and toggling either removes the setting (enabling suggestions) or sets it to 'off' (disabling suggestions).
1-6: Consistent import pattern with other settings components.The imports follow the established pattern used in other settings components, with proper separation of uhtml, local components, utilities, and i18n.
18-27:Details
✅ Verification successful
Verify the search filter toggle logic.
The logic for the search filter toggle looks correct, but let me verify: when checked, it means the filter is set to 'music_songs', and toggling will either remove it (if currently set) or set it to 'music_songs' (if not set). This seems backwards - typically you'd expect toggling OFF to remove the setting.
The toggle logic might be inverted. Please verify the intended behavior:
🏁 Script executed:
#!/bin/bash # Description: Check how searchFilter is used in the search functionality # Search for searchFilter usage to understand expected behavior rg -A 5 -B 5 "searchFilter.*music_songs|music_songs.*searchFilter" --type tsLength of output: 735
🏁 Script executed:
#!/bin/bash # List all ToggleSwitch components in search.ts with surrounding context rg -n -A 3 -B 2 "ToggleSwitch" --type ts src/components/Settings/search.ts # Print the first 100 lines of the file to understand both toggles sed -n '1,100p' src/components/Settings/search.tsLength of output: 1781
Search filter toggle logic is correct
The toggle’s checked state (
getSaved('searchFilter') === 'music_songs') and handler (removing the key when set, saving'music_songs'when unset, then navigating) align with the intended on/off behavior. No changes required.src/components/Settings/app.ts (1)
17-17:Details
✅ Verification successful
Undefined variable
Build.The
Buildvariable is used but not imported or defined in this file, which will cause a runtime error.Add the missing import or definition:
+// Add at the top with other imports +declare const Build: string;Let me check if
Buildis defined elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for Build variable definition in the codebase rg -A 3 "Build\s*=" --type ts --type js rg -A 3 "const Build" --type ts --type js rg -A 3 "let Build" --type ts --type jsLength of output: 225
Ambient declarations cover
BuildandLocalesThe
Build(andLocales) constants are already declared insrc/vite-env.d.tsas ambient globals:// src/vite-env.d.ts declare const Build: string; declare const Locales: string[];No additional import or declaration is needed in
src/components/Settings/app.ts.src/components/Settings/personalize.ts (2)
1-7: Imports look well-organized.The import statements are properly structured and include all necessary dependencies for the functionality.
31-46: 🛠️ Refactor suggestionAdd null check for getSaved() result.
The code assumes
getSaved('roundness')returns a valid value, but should handle the case where it might return null or undefined more safely.- target.value = getSaved('roundness') || '0.4rem'; + const savedRoundness = getSaved('roundness'); + target.value = savedRoundness || '0.4rem';Likely an incorrect or invalid review comment.
| checked: boolean, | ||
| handler: (e: Event & { target: HTMLInputElement }) => void | ||
| }) { | ||
| let inputEl!: HTMLInputElement; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace unsafe non-null assertion with safer initialization.
The non-null assertion (!) on inputEl is potentially unsafe if the ref callback isn't called as expected.
Use a safer initialization pattern:
-let inputEl!: HTMLInputElement;
+let inputEl: HTMLInputElement | null = null;And update the click handler:
<span @click=${() => {
- inputEl.click();
+ inputEl?.click();
}}></span>📝 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.
| let inputEl!: HTMLInputElement; | |
| // src/components/Settings/ToggleSwitch.ts | |
| import { html, ref } from 'uhtml'; | |
| type Props = { | |
| id: string; | |
| name: string; | |
| checked: boolean; | |
| onChange: (e: Event) => void; | |
| }; | |
| export function ToggleSwitch({ id, name, checked, onChange }: Props) { | |
| - let inputEl!: HTMLInputElement; | |
| + let inputEl: HTMLInputElement | null = null; | |
| return html` | |
| <div class="toggle-switch"> | |
| - <span @click=${() => { | |
| - inputEl.click(); | |
| - }}></span> | |
| + <span @click=${() => { | |
| + inputEl?.click(); | |
| + }}></span> | |
| <input | |
| id=${id} | |
| name=${name} | |
| type="checkbox" | |
| .checked=${checked} | |
| @change=${onChange} | |
| ${ref(el => (inputEl = el))} | |
| /> | |
| </div> | |
| `; | |
| } |
🤖 Prompt for AI Agents
In src/components/Settings/ToggleSwitch.ts at line 10, replace the non-null
assertion on the inputEl variable with a safer initialization by declaring
inputEl as possibly undefined or null initially. Then, update the click handler
to safely check if inputEl is defined before accessing its properties or methods
to avoid runtime errors if the ref callback hasn't been called yet.
| for (let i = 0; i <= len; i++) { | ||
| const key = localStorage.key(i); | ||
| if (key && key.startsWith('kidsMode')) | ||
| removeSaved(key); | ||
| } |
There was a problem hiding this comment.
Fix potential off-by-one error in localStorage iteration.
The loop condition i <= len could cause an index out of bounds error since localStorage.key() is 0-indexed and len represents the total count.
- for (let i = 0; i <= len; i++) {
+ for (let i = 0; i < len; i++) {📝 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.
| for (let i = 0; i <= len; i++) { | |
| const key = localStorage.key(i); | |
| if (key && key.startsWith('kidsMode')) | |
| removeSaved(key); | |
| } | |
| for (let i = 0; i < len; i++) { | |
| const key = localStorage.key(i); | |
| if (key && key.startsWith('kidsMode')) | |
| removeSaved(key); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In src/components/Settings/parental.ts around lines 36 to 40, the for loop
iterates with the condition i <= len, which can cause an off-by-one error
because localStorage.key() is zero-indexed and len is the total count. Change
the loop condition to i < len to ensure the index stays within bounds and
prevent potential errors.
| settingsContainer.prepend(settingsFrag); | ||
| settingsContainer.showModal(); | ||
| settingsContainer.addEventListener('close', () => { | ||
| settingsContainer.innerHTML = document.getElementById('actionsContainer')!.outerHTML; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential null reference error.
Using ! assertion operator assumes the element exists, but this could throw a runtime error if the element is missing.
Add proper null checking:
- settingsContainer.innerHTML = document.getElementById('actionsContainer')!.outerHTML;
+ const actionsContainer = document.getElementById('actionsContainer');
+ if (actionsContainer) {
+ settingsContainer.innerHTML = actionsContainer.outerHTML;
+ }📝 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.
| settingsContainer.innerHTML = document.getElementById('actionsContainer')!.outerHTML; | |
| const actionsContainer = document.getElementById('actionsContainer'); | |
| if (actionsContainer) { | |
| settingsContainer.innerHTML = actionsContainer.outerHTML; | |
| } |
🤖 Prompt for AI Agents
In src/components/Settings/index.ts at line 29, the code uses the non-null
assertion operator (!) on getElementById('actionsContainer'), which can cause a
runtime error if the element is not found. To fix this, add a null check to
verify that the element exists before accessing its outerHTML. If the element is
null, handle the case appropriately, such as by skipping the assignment or
logging an error.
| async function clearCache(_: Event | undefined = undefined) { | ||
| await self.caches.keys().then(s => { s.forEach(k => { self.caches.delete(k) }) }); | ||
| await navigator.serviceWorker.getRegistrations().then(s => { s.forEach(r => { r.unregister() }) }); | ||
|
|
||
| if (_?.type === 'click') location.reload(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling in cache clearing.
The async cache operations don't handle potential errors, which could leave the cache in an inconsistent state.
Add error handling for cache operations:
async function clearCache(_: Event | undefined = undefined) {
- await self.caches.keys().then(s => { s.forEach(k => { self.caches.delete(k) }) });
- await navigator.serviceWorker.getRegistrations().then(s => { s.forEach(r => { r.unregister() }) });
+ try {
+ await self.caches.keys().then(s => { s.forEach(k => { self.caches.delete(k) }) });
+ await navigator.serviceWorker.getRegistrations().then(s => { s.forEach(r => { r.unregister() }) });
+ } catch (error) {
+ console.warn('Error clearing cache:', error);
+ }
if (_?.type === 'click') location.reload();
}📝 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 clearCache(_: Event | undefined = undefined) { | |
| await self.caches.keys().then(s => { s.forEach(k => { self.caches.delete(k) }) }); | |
| await navigator.serviceWorker.getRegistrations().then(s => { s.forEach(r => { r.unregister() }) }); | |
| if (_?.type === 'click') location.reload(); | |
| } | |
| async function clearCache(_: Event | undefined = undefined) { | |
| try { | |
| await self.caches.keys().then(s => { s.forEach(k => { self.caches.delete(k) }) }); | |
| await navigator.serviceWorker.getRegistrations().then(s => { s.forEach(r => { r.unregister() }) }); | |
| } catch (error) { | |
| console.warn('Error clearing cache:', error); | |
| } | |
| if (_?.type === 'click') location.reload(); | |
| } |
🤖 Prompt for AI Agents
In src/components/Settings/index.ts around lines 37 to 42, the async cache
clearing operations lack error handling, risking inconsistent cache state if
failures occur. Wrap the cache keys retrieval and deletion, as well as service
worker unregistration, in try-catch blocks or use promise catch handlers to
properly handle and log any errors during these operations. This ensures that
errors are caught and managed without disrupting the flow.
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling in file import.
The importSettings function doesn't handle potential errors from file reading or JSON parsing, which could crash the application.
Add proper error handling:
async function importSettings(e: Event) {
e.preventDefault();
- const newSettings = JSON.parse(
- await (
- (e.target as HTMLInputElement).files as FileList
- )[0].text()
- );
+ try {
+ const files = (e.target as HTMLInputElement).files;
+ if (!files || files.length === 0) return;
+
+ const fileContent = await files[0].text();
+ const newSettings = JSON.parse(fileContent);
- if (confirm('This will merge your current settings with the imported settings, continue?')) {
- for (const key in newSettings)
- save(key, newSettings[key]);
+ if (confirm('This will merge your current settings with the imported settings, continue?')) {
+ for (const key in newSettings)
+ save(key, newSettings[key]);
- location.reload();
- }
+ location.reload();
+ }
+ } catch (error) {
+ alert('Error importing settings: Invalid file format');
+ }
}📝 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(); | |
| } | |
| } | |
| async function importSettings(e: Event) { | |
| e.preventDefault(); | |
| try { | |
| const files = (e.target as HTMLInputElement).files; | |
| if (!files || files.length === 0) return; | |
| const fileContent = await files[0].text(); | |
| const newSettings = JSON.parse(fileContent); | |
| if (confirm('This will merge your current settings with the imported settings, continue?')) { | |
| for (const key in newSettings) | |
| save(key, newSettings[key]); | |
| location.reload(); | |
| } | |
| } catch (error) { | |
| alert('Error importing settings: Invalid file format'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/Settings/index.ts around lines 71 to 85, the importSettings
function lacks error handling for file reading and JSON parsing, risking
application crashes. Wrap the file reading and JSON.parse logic in a try-catch
block to catch any errors, and handle them gracefully by notifying the user or
logging the error without breaking the app flow.
| children: html`${Locales.map(item => html` | ||
| <option value=${item}>${new Intl.DisplayNames(document.documentElement.lang, { type: 'language' }).of(item)}</option> | ||
| `)}` |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Undefined variable Locales.
The Locales variable is used but not imported or defined in this file, which will cause a runtime error.
Add the missing import:
+import { Locales } from '../../lib/constants'; // or wherever Locales is definedLet me check where Locales is defined:
🏁 Script executed:
#!/bin/bash
# Description: Search for Locales variable definition in the codebase
rg -A 3 "Locales\s*=" --type ts --type js
rg -A 3 "export.*Locales" --type ts --type jsLength of output: 91
Add missing import for Locales
The Locales identifier used in src/components/Settings/app.ts (lines 53–55) isn’t defined or imported, which will cause a runtime error. You’ll need to import it from wherever it’s declared (for example, src/lib/constants).
• File: src/components/Settings/app.ts
• Lines: 53–55
Suggested fix:
+ import { Locales } from '../../lib/constants';
/* … */
children: html`${Locales.map(item => html`
<option value=${item}>${new Intl.DisplayNames(
document.documentElement.lang,
{ type: 'language' }
).of(item)}</option>
`)}`📝 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.
| children: html`${Locales.map(item => html` | |
| <option value=${item}>${new Intl.DisplayNames(document.documentElement.lang, { type: 'language' }).of(item)}</option> | |
| `)}` | |
| // At the top of src/components/Settings/app.ts | |
| import { Locales } from '../../lib/constants'; | |
| /* … */ | |
| children: html`${Locales.map(item => html` | |
| <option value=${item}>${new Intl.DisplayNames( | |
| document.documentElement.lang, | |
| { type: 'language' } | |
| ).of(item)}</option> | |
| `)}` |
🤖 Prompt for AI Agents
In src/components/Settings/app.ts around lines 53 to 55, the variable Locales is
used but not imported or defined, causing a runtime error. To fix this, locate
the module where Locales is declared (such as src/lib/constants) and add an
import statement for Locales at the top of this file. This will ensure Locales
is properly referenced and prevent the undefined variable error.
| const db = getDB(); | ||
| const count = Object.keys(db.discover || {}).length || 0; | ||
| if (confirm(i18n("settings_clear_discoveries", count.toString()))) { | ||
| delete db.discover; | ||
| saveDB(db); | ||
| save('discover', 'off'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Database operations lack error handling.
The getDB() and saveDB() operations could potentially fail, but there's no error handling which could lead to data corruption or inconsistent state.
Add error handling for database operations:
else {
- const db = getDB();
- const count = Object.keys(db.discover || {}).length || 0;
- if (confirm(i18n("settings_clear_discoveries", count.toString()))) {
- db.discover = undefined;
- saveDB(db);
- save('discover', 'off');
- }
+ try {
+ const db = getDB();
+ const count = Object.keys(db.discover || {}).length || 0;
+ if (confirm(i18n("settings_clear_discoveries", count.toString()))) {
+ db.discover = undefined;
+ saveDB(db);
+ save('discover', 'off');
+ }
+ } catch (error) {
+ console.error('Error clearing discoveries:', error);
+ alert('Error clearing discoveries. Please try again.');
+ e.preventDefault();
+ }Apply similar pattern to the history section.
Also applies to: 94-99
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In src/components/Settings/library.ts around lines 74 to 79 and 94 to 99, the
database operations using getDB() and saveDB() lack error handling, risking data
corruption or inconsistent state. Wrap these calls in try-catch blocks to catch
any exceptions, log or handle the errors appropriately, and prevent the
application from crashing. Apply the same error handling pattern to the history
section to ensure consistent and safe database operations.
| crypto.subtle.digest('SHA-256', msgBuffer) | ||
| .then(hashBuffer => { | ||
| const hash = Array | ||
| .from(new Uint8Array(hashBuffer)) | ||
| .map(b => b.toString(16).padStart(2, '0')) | ||
| .join(''); | ||
| save(_, hash); | ||
| location.reload(); | ||
| }); |
There was a problem hiding this comment.
Missing error handling in crypto operations.
The crypto.subtle.digest operation can fail, but there's no error handling, which could leave the UI in an inconsistent state.
Add error handling for crypto operations:
function hashCreator(text: string) {
const msgBuffer = new TextEncoder().encode(text);
- crypto.subtle.digest('SHA-256', msgBuffer)
+ crypto.subtle.digest('SHA-256', msgBuffer)
.then(hashBuffer => {
const hash = Array
.from(new Uint8Array(hashBuffer))
.map(b => b.toString(16).padStart(2, '0'))
.join('');
save(_, hash);
location.reload();
- });
+ })
+ .catch(error => {
+ console.error('Error creating hash:', error);
+ alert('Error setting up sync. Please try again.');
+ });
}🤖 Prompt for AI Agents
In src/components/Settings/library.ts around lines 39 to 47, the
crypto.subtle.digest call lacks error handling, which can cause issues if the
digest operation fails. Add a .catch block after the .then to handle any errors
from the digest operation. In the catch block, log the error or handle it
appropriately to prevent the UI from entering an inconsistent state.
| export default function() { | ||
| return html` | ||
| <div> | ||
| <b> | ||
| <i class="ri-t-shirt-2-line"></i> | ||
| <p>${i18n('settings_interface')}</p> | ||
| </b> | ||
|
|
||
| ${ToggleSwitch({ | ||
| id: 'imgLoadSwitch', | ||
| name: 'settings_load_images', | ||
| checked: store.loadImage, | ||
| handler: () => { | ||
| const _ = 'imgLoad'; | ||
| if (getSaved(_)) | ||
| removeSaved(_); | ||
| else | ||
| save(_, 'off'); | ||
| location.reload(); | ||
| } | ||
| })} | ||
|
|
||
| ${Selector({ | ||
| label: 'settings_roundness', | ||
| id: 'roundnessChanger', | ||
| handler: (e) => { | ||
| cssVar('--roundness', e.target.value); | ||
| if (e.target.value === '0.4rem') | ||
| removeSaved('roundness'); | ||
| else | ||
| save('roundness', e.target.value); | ||
| }, | ||
| onmount: (target) => { | ||
| if (getSaved('roundness')) { | ||
| target.value = getSaved('roundness') || '0.4rem'; | ||
| cssVar('--roundness', target.value); | ||
| } | ||
| }, | ||
| children: html` | ||
| <option value="none">${i18n('settings_roundness_none')}</option> | ||
| <option value="0.2rem">${i18n('settings_roundness_lighter')}</option> | ||
| <option value="0.4rem" selected>${i18n('settings_roundness_light')}</option> | ||
| <option value="0.6rem">${i18n('settings_roundness_heavy')}</option> | ||
| <option value="0.9rem">${i18n('settings_roundness_heavier')}</option> | ||
| ` | ||
| })} | ||
|
|
||
| ${ToggleSwitch({ | ||
| id: "custom_theme", | ||
| name: 'settings_use_custom_color', | ||
| checked: getSaved('custom_theme') !== null, | ||
| handler: e => { | ||
| const _ = 'custom_theme'; | ||
| const colorString = getSaved(_); | ||
|
|
||
| if (colorString) removeSaved(_); | ||
| else { | ||
| const rgbText = i18n('settings_custom_color_prompt'); | ||
| const str = prompt(rgbText, '174,174,174'); | ||
| if (str) | ||
| save(_, str) | ||
| else | ||
| e.preventDefault(); | ||
| } | ||
| themer(); | ||
| } | ||
| })} | ||
|
|
||
| ${Selector({ | ||
| label: 'settings_theming_scheme', | ||
| id: 'themeSelector', | ||
| handler: (e) => { | ||
| themer(); | ||
| if (e.target.value === 'auto') | ||
| removeSaved('theme'); | ||
| else | ||
| save('theme', e.target.value); | ||
| }, | ||
| onmount: (target) => { | ||
| target.value = (getSaved('theme')) || 'auto'; | ||
| }, | ||
| children: html` | ||
| <optgroup label=${i18n('settings_theming_scheme_dynamic')}> | ||
| <option value="auto" selected>${i18n('settings_theming_scheme_system')}</option> | ||
| <option value="light">${i18n('settings_theming_scheme_light')}</option> | ||
| <option value="dark">${i18n('settings_theming_scheme_dark')}</option> | ||
| </optgroup> | ||
| <optgroup label=${i18n('settings_theming_scheme_hc')}> | ||
| <option value="auto-hc">${i18n('settings_theming_scheme_hc_system')}</option> | ||
| <option value="white">${i18n('settings_theming_scheme_white')}</option> | ||
| <option value="black">${i18n('settings_theming_scheme_black')}</option> | ||
| </optgroup> | ||
| ` | ||
| })} | ||
|
|
||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for storage operations.
The code lacks error handling for storage operations (getSaved, save, removeSaved) which could fail. Consider wrapping critical operations in try-catch blocks to provide graceful fallbacks.
+// Add error handling wrapper functions:
+const safeSave = (key, value) => {
+ try {
+ save(key, value);
+ } catch (error) {
+ console.warn(`Failed to save preference ${key}:`, error);
+ }
+};
+
+const safeRemove = (key) => {
+ try {
+ removeSaved(key);
+ } catch (error) {
+ console.warn(`Failed to remove preference ${key}:`, error);
+ }
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Settings/personalize.ts around lines 9 to 106, the calls to
storage functions getSaved, save, and removeSaved lack error handling, which
could cause unhandled exceptions if storage operations fail. Wrap these critical
storage calls in try-catch blocks to catch any errors, log or handle them
gracefully, and ensure the UI remains stable without crashing.
| ${ToggleSwitch({ | ||
| id: "custom_theme", | ||
| name: 'settings_use_custom_color', | ||
| checked: getSaved('custom_theme') !== null, | ||
| handler: e => { | ||
| const _ = 'custom_theme'; | ||
| const colorString = getSaved(_); | ||
|
|
||
| if (colorString) removeSaved(_); | ||
| else { | ||
| const rgbText = i18n('settings_custom_color_prompt'); | ||
| const str = prompt(rgbText, '174,174,174'); | ||
| if (str) | ||
| save(_, str) | ||
| else | ||
| e.preventDefault(); | ||
| } | ||
| themer(); | ||
| } | ||
| })} |
There was a problem hiding this comment.
Replace prompt() with a better UX pattern and fix event handling.
The current implementation has two issues:
- Using
prompt()provides poor UX and doesn't fit modern web standards - The
e.preventDefault()call won't work as expected in a toggle switch handler
Consider replacing the prompt with a proper color picker or input dialog:
- const rgbText = i18n('settings_custom_color_prompt');
- const str = prompt(rgbText, '174,174,174');
- if (str)
- save(_, str)
- else
- e.preventDefault();
+ // Consider implementing a proper color picker dialog
+ // or input field instead of using prompt()
+ const colorInput = document.createElement('input');
+ colorInput.type = 'color';
+ colorInput.click();
+ colorInput.addEventListener('change', () => {
+ if (colorInput.value) {
+ save(_, colorInput.value);
+ themer();
+ }
+ });
+ return; // Exit early if no color selected📝 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.
| ${ToggleSwitch({ | |
| id: "custom_theme", | |
| name: 'settings_use_custom_color', | |
| checked: getSaved('custom_theme') !== null, | |
| handler: e => { | |
| const _ = 'custom_theme'; | |
| const colorString = getSaved(_); | |
| if (colorString) removeSaved(_); | |
| else { | |
| const rgbText = i18n('settings_custom_color_prompt'); | |
| const str = prompt(rgbText, '174,174,174'); | |
| if (str) | |
| save(_, str) | |
| else | |
| e.preventDefault(); | |
| } | |
| themer(); | |
| } | |
| })} | |
| ${ToggleSwitch({ | |
| id: "custom_theme", | |
| name: 'settings_use_custom_color', | |
| checked: getSaved('custom_theme') !== null, | |
| handler: e => { | |
| const _ = 'custom_theme'; | |
| const colorString = getSaved(_); | |
| if (colorString) removeSaved(_); | |
| else { | |
| // Consider implementing a proper color picker dialog | |
| // or input field instead of using prompt() | |
| const colorInput = document.createElement('input'); | |
| colorInput.type = 'color'; | |
| colorInput.click(); | |
| colorInput.addEventListener('change', () => { | |
| if (colorInput.value) { | |
| save(_, colorInput.value); | |
| themer(); | |
| } | |
| }); | |
| return; // Exit early if no color selected | |
| } | |
| themer(); | |
| } | |
| })} |
🤖 Prompt for AI Agents
In src/components/Settings/personalize.ts between lines 56 and 75, replace the
use of prompt() for color input with a modern color picker or input dialog
component to improve UX. Also, remove the e.preventDefault() call from the
toggle switch handler since it does not function correctly in this context.
Instead, handle the toggle state and color saving logic appropriately within the
new input flow, ensuring themer() is called after changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/lib/libraryUtils.ts (1)
153-153: Consider adding item count for shared collections to maintain consistency.The item count display is helpful for users, but it's only shown for local collections. This creates an inconsistent user experience between local and shared collections.
Consider adding similar functionality to
getSharedCollection:async function getSharedCollection( id: string ) { loadingScreen.showModal(); const data = await fetch(`${location.origin}/blob/${id}`) .then(res => res.json()) .catch(() => ''); if (data) { + listTitle.textContent += ` | ${data.length} streams`; renderCollection(data); } else render(listContainer, html`Collection does not exist`); loadingScreen.close(); }src/scripts/queue.ts (1)
24-36: Consider refactoring to avoid programmatic clicking.While functional, programmatically clicking DOM elements can have unintended side effects and makes the code harder to reason about. Consider extracting the remove toggle logic into a shared function.
function filterLT10() { - if (removeQBtn.classList.contains('delete')) - removeQBtn.click(); + if (removeQBtn.classList.contains('delete')) { + removeQBtn.classList.toggle('on'); + queuelist.querySelectorAll('.streamItem') + .forEach(el => el.classList.toggle('delete')); + removeQBtn.classList.toggle('delete'); + } const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>; items.forEach(el => { const duration = el.dataset.duration as string; if (!isLongerThan10Min(duration)) return; el.classList.add('delete'); - el.click(); + // Trigger the same logic as clicking without actual click event + const id = el.dataset.id || ''; + const current = sessionStorage.getItem('trashHistory') || ''; + if (!current?.includes(id)) + sessionStorage.setItem('trashHistory', current + id); + + const { list } = store.queue; + const index = list.indexOf(id); + list.splice(index, 1); + queuelist.children[index].remove(); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
index.html(5 hunks)src/lib/dom.ts(2 hunks)src/lib/libraryUtils.ts(3 hunks)src/lib/utils.ts(2 hunks)src/main.ts(1 hunks)src/modules/fetchList.ts(2 hunks)src/scripts/list.ts(2 hunks)src/scripts/queue.ts(3 hunks)src/stylesheets/global.css(1 hunks)src/stylesheets/home.css(1 hunks)src/stylesheets/list.css(5 hunks)src/stylesheets/upcoming.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/stylesheets/upcoming.css
🚧 Files skipped from review as they are similar to previous changes (7)
- src/stylesheets/global.css
- src/lib/dom.ts
- index.html
- src/scripts/list.ts
- src/modules/fetchList.ts
- src/main.ts
- src/lib/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/lib/libraryUtils.ts (1)
src/lib/dom.ts (1)
listTitle(33-33)
src/scripts/queue.ts (4)
src/lib/dom.ts (1)
queuelist(21-21)src/lib/store.ts (2)
store(5-108)getSaved(3-3)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/utils.ts (3)
removeSaved(15-15)save(13-13)notify(62-70)
🔇 Additional comments (12)
src/lib/libraryUtils.ts (2)
2-2: LGTM: Import addition is necessary and correctly placed.The addition of
listTitleto the imports is required for the new dynamic title updating functionality.
96-96: LGTM: Title setting provides immediate user feedback.Setting the title at the start of
fetchCollectionprovides immediate visual feedback about which collection is being loaded, improving the user experience.src/stylesheets/home.css (1)
151-151: LGTM! Improved font inheritance for better consistency.Changing from a fixed font stack to
inheritallows for more flexible and consistent typography across the interface, which aligns well with the broader UI refactoring in this PR.src/stylesheets/list.css (3)
3-11: LGTM! Selector updates align with UI restructuring.The change from
buttontoliselectors and addition of::afterpseudo-element for aria-label display aligns well with the structural shift to list-based action controls mentioned in the AI summary.
28-28: LGTM! Consistent selector updates for different list tool states.The updates to use
liselectors for channel, reserved, supermix, and shared states are consistent with the broader UI restructuring.Also applies to: 39-39, 62-62
87-87: LGTM! Font inheritance consistency improvement.This change aligns with the typography consistency improvements seen across the CSS files in this PR.
src/scripts/queue.ts (6)
1-8: LGTM! Clean import organization for uhtml refactoring.The new imports support the transition to declarative rendering with uhtml and proper component-based architecture.
16-22: LGTM! Clear duration validation logic.The function correctly identifies durations longer than 10 minutes by checking if it's not a 2-part time format (MM:SS) with minutes < 10.
38-49: LGTM! Clean shuffle implementation with proper state sync.The shuffle logic correctly reorders DOM elements and synchronizes the
store.queue.listarray to match.
150-188: LGTM! Well-structured queue append method with proper filtering.The method correctly handles duplicates, duration filtering, and prepend/append logic while maintaining consistency between DOM and state.
190-213: LGTM! Proper queue item removal with state synchronization.The click handler correctly removes items from both the
store.queue.listarray and the DOM, maintaining consistency.
215-222: LGTM! Sortable integration updated for new queue structure.The sortable logic correctly updates
store.queue.listwhen items are reordered via drag and drop.
| @click=${(e: MouseEvent) => { | ||
| const btn = e.currentTarget as HTMLElement; | ||
| const ls = 'enqueueRelatedStreams'; | ||
|
|
||
| btn.classList.toggle('on'); | ||
|
|
||
| if (btn.classList.contains('on')) | ||
| removeSaved(ls); | ||
| else | ||
| save(ls, 'on'); | ||
|
|
||
| notify(i18n('upcoming_change')); | ||
| }} |
There was a problem hiding this comment.
Fix inverted logic for enqueueRelatedStreams state management.
Same localStorage logic issue as the previous two handlers.
btn.classList.toggle('on');
- if (btn.classList.contains('on'))
+ if (btn.classList.contains('on'))
+ save(ls, 'on');
+ else
removeSaved(ls);
- else
- save(ls, 'on');📝 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.
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'enqueueRelatedStreams'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| removeSaved(ls); | |
| else | |
| save(ls, 'on'); | |
| notify(i18n('upcoming_change')); | |
| }} | |
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'enqueueRelatedStreams'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| save(ls, 'on'); | |
| else | |
| removeSaved(ls); | |
| notify(i18n('upcoming_change')); | |
| }} |
🤖 Prompt for AI Agents
In src/scripts/queue.ts between lines 121 and 133, the logic for toggling the
'enqueueRelatedStreams' state is inverted: when the button has the 'on' class,
it should save the state as 'on' in localStorage, and when it does not, it
should remove the saved state. Fix this by swapping the calls to removeSaved(ls)
and save(ls, 'on') so that the state is saved when the button is 'on' and
removed when it is not.
| @click=${(e: MouseEvent) => { | ||
| const btn = e.currentTarget as HTMLElement; | ||
| const ls = 'filterLT10'; | ||
|
|
||
| btn.classList.toggle('on'); | ||
|
|
||
| if (btn.classList.contains('on')) | ||
| removeSaved(ls); | ||
| else | ||
| save(ls, 'on'); | ||
|
|
||
| filterLT10(); | ||
| }} |
There was a problem hiding this comment.
Fix inverted logic for localStorage state management.
The current logic is backwards - when the button is 'on', it removes the saved setting, but when it's off, it saves 'on'. This contradicts the initialization logic.
btn.classList.toggle('on');
- if (btn.classList.contains('on'))
+ if (btn.classList.contains('on'))
+ save(ls, 'on');
+ else
removeSaved(ls);
- else
- save(ls, 'on');📝 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.
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'filterLT10'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| removeSaved(ls); | |
| else | |
| save(ls, 'on'); | |
| filterLT10(); | |
| }} | |
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'filterLT10'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| save(ls, 'on'); | |
| else | |
| removeSaved(ls); | |
| filterLT10(); | |
| }} |
🤖 Prompt for AI Agents
In src/scripts/queue.ts around lines 73 to 85, the logic for managing
localStorage state is inverted: when the button has the 'on' class, it should
save the 'on' state, and when it does not, it should remove the saved setting.
Swap the calls so that if btn.classList.contains('on') is true, call save(ls,
'on'), otherwise call removeSaved(ls).
| @click=${(e: MouseEvent) => { | ||
| const btn = e.currentTarget as HTMLElement; | ||
| const ls = 'allowDuplicates'; | ||
|
|
||
| btn.classList.toggle('on'); | ||
|
|
||
| if (btn.classList.contains('on')) | ||
| removeSaved(ls); | ||
| else | ||
| save(ls, 'on'); | ||
|
|
||
| notify(i18n('upcoming_change')); | ||
| }} |
There was a problem hiding this comment.
Fix inverted logic for allowDuplicates state management.
Same issue as the filterLT10 logic - the localStorage operations are backwards.
btn.classList.toggle('on');
- if (btn.classList.contains('on'))
+ if (btn.classList.contains('on'))
+ save(ls, 'on');
+ else
removeSaved(ls);
- else
- save(ls, 'on');📝 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.
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'allowDuplicates'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| removeSaved(ls); | |
| else | |
| save(ls, 'on'); | |
| notify(i18n('upcoming_change')); | |
| }} | |
| @click=${(e: MouseEvent) => { | |
| const btn = e.currentTarget as HTMLElement; | |
| const ls = 'allowDuplicates'; | |
| btn.classList.toggle('on'); | |
| if (btn.classList.contains('on')) | |
| save(ls, 'on'); | |
| else | |
| removeSaved(ls); | |
| notify(i18n('upcoming_change')); | |
| }} |
🤖 Prompt for AI Agents
In src/scripts/queue.ts around lines 97 to 109, the logic for managing the
'allowDuplicates' state is inverted; currently, when the button has the 'on'
class, it removes the saved state, and when it does not, it saves it. To fix
this, reverse the conditions so that when the button has the 'on' class, it
saves the 'allowDuplicates' state as 'on', and when it does not, it removes the
saved state. This aligns the localStorage operations correctly with the button's
visual state.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/scripts/queue.ts (3)
76-82: Fix inverted logic for filterLT10 state management.The localStorage logic is backwards - when the button is 'on', it should save the state as 'on', not remove it. This contradicts the initialization logic and matches the issue flagged in previous reviews.
Apply this diff to fix the logic:
- if (btn.classList.contains('on')) - removeSaved(ls); - else - save(ls, 'on'); + if (btn.classList.contains('on')) + save(ls, 'on'); + else + removeSaved(ls);
99-105: Fix inverted logic for allowDuplicates state management.Same localStorage logic issue as noted in previous reviews - the state management is backwards.
Apply this diff to fix the logic:
- if (btn.classList.contains('on')) - removeSaved(ls); - else - save(ls, 'on'); + if (btn.classList.contains('on')) + save(ls, 'on'); + else + removeSaved(ls);
122-128: Fix inverted logic for enqueueRelatedStreams state management.Same localStorage logic issue as the previous two handlers - the state operations are backwards.
Apply this diff to fix the logic:
- if (btn.classList.contains('on')) - removeSaved(ls); - else - save(ls, 'on'); + if (btn.classList.contains('on')) + save(ls, 'on'); + else + removeSaved(ls);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/modules/fetchList.ts(2 hunks)src/scripts/queue.ts(3 hunks)src/stylesheets/header.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/modules/fetchList.ts
- src/stylesheets/header.css
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/scripts/queue.ts (4)
src/lib/dom.ts (1)
queuelist(21-21)src/lib/store.ts (2)
store(5-108)getSaved(3-3)src/scripts/i18n.ts (1)
i18n(44-49)src/lib/utils.ts (3)
removeSaved(15-15)save(13-13)notify(62-70)
🔇 Additional comments (3)
src/scripts/queue.ts (3)
38-142: Excellent refactoring to declarative UI rendering.The transition from manual DOM manipulation to declarative rendering with
uhtmlsignificantly improves code maintainability and readability. The template-based approach with embedded event handlers is a clean architectural improvement.
146-184: Good integration with new queue structure.The
store.queue.appendfunction properly integrates with the newstore.queue.liststructure and correctly references the dynamically rendered button states. The logic flow is clear and handles all the queue management scenarios appropriately.
203-208: Correct update to use store.queue.list.The event handler properly uses the new
store.queue.liststructure instead of the old array reference, maintaining consistency with the refactored queue management approach.
| function filterLT10() { | ||
| if (removeQBtn.classList.contains('delete')) | ||
| removeQBtn.click(); | ||
|
|
||
| const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>; | ||
| items.forEach(el => { | ||
| const duration = el.dataset.duration as string; | ||
| if (!isLongerThan10Min(duration)) | ||
| return; | ||
| el.classList.add('delete'); | ||
| el.click(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix inverted filtering logic in filterLT10 function.
The filtering logic is backwards. The function should remove items that are less than 10 minutes (shorter videos), but it currently removes items that are longer than 10 minutes.
Apply this diff to fix the logic:
- const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>;
- items.forEach(el => {
- const duration = el.dataset.duration as string;
- if (!isLongerThan10Min(duration))
- return;
- el.classList.add('delete');
- el.click();
- });
+ const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>;
+ items.forEach(el => {
+ const duration = el.dataset.duration as string;
+ if (isLongerThan10Min(duration))
+ return;
+ el.classList.add('delete');
+ el.click();
+ });📝 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.
| function filterLT10() { | |
| if (removeQBtn.classList.contains('delete')) | |
| removeQBtn.click(); | |
| const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>; | |
| items.forEach(el => { | |
| const duration = el.dataset.duration as string; | |
| if (!isLongerThan10Min(duration)) | |
| return; | |
| el.classList.add('delete'); | |
| el.click(); | |
| }); | |
| } | |
| function filterLT10() { | |
| if (removeQBtn.classList.contains('delete')) | |
| removeQBtn.click(); | |
| const items = queuelist.querySelectorAll('.streamItem') as NodeListOf<HTMLElement>; | |
| items.forEach(el => { | |
| const duration = el.dataset.duration as string; | |
| // Only delete items shorter than 10 minutes | |
| if (isLongerThan10Min(duration)) | |
| return; | |
| el.classList.add('delete'); | |
| el.click(); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/scripts/queue.ts around lines 24 to 36, the filterLT10 function
incorrectly removes items longer than 10 minutes instead of those shorter than
10 minutes. To fix this, invert the condition inside the forEach loop to check
if the duration is less than 10 minutes and only then add the 'delete' class and
trigger the click event. This will ensure only items shorter than 10 minutes are
removed as intended.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/components/Settings/index.ts (3)
31-31: Potential null reference error.This issue has already been identified in a previous review. Using the
!assertion operator assumes the element exists, but this could throw a runtime error if the element is missing.
38-43: Missing error handling in cache clearing.This issue has already been identified in a previous review. The async cache operations don't handle potential errors, which could leave the cache in an inconsistent state.
72-86: Missing error handling in file import.This issue has already been identified in a previous review. The
importSettingsfunction doesn't handle potential errors from file reading or JSON parsing, which could crash the application.src/scripts/library.ts (1)
41-50:⚠️ Potential issueAdd error handling for file operations.
The import function should handle potential failures in file reading and JSON parsing to provide better user feedback.
async function importLibrary(e: FileEv) { - const importBtn = e.target; - const newDB = JSON.parse(await importBtn.files[0].text()); + try { + const importBtn = e.target; + const file = importBtn.files?.[0]; + if (!file) return; + const text = await file.text(); + const newDB = JSON.parse(text); const oldDB = getDB(); if (!confirm(i18n('library_import_prompt'))) return; for (const collection in newDB) for (const item in newDB[collection]) toCollection(collection, newDB[collection][item], oldDB) saveDB(oldDB); location.reload(); + } catch (error) { + notify(i18n('library_import_error')); + console.error('Import failed:', error); + } };
🧹 Nitpick comments (7)
src/components/Settings/Settings.css (3)
8-8: Consider usingoverflow: autoinstead ofoverflow: scroll.Using
overflow: scrollwill always show scrollbars even when content fits. Consider usingoverflow: autoto show scrollbars only when needed.- overflow: scroll; + overflow: auto;
11-13: Add a default width or max-width constraint.The landscape media query sets width to 60%, but there's no default width specified. Consider adding a max-width to prevent the dialog from becoming too wide on large screens.
margin: auto; padding: 5%; user-select: none; overflow: scroll; border-radius: var(--roundness); + width: 90%; + max-width: 800px; @media(orientation:landscape) { width: 60%; }
6-6: Consider using consistent spacing units instead of percentages.Using percentage-based padding and margins can lead to inconsistent spacing across different screen sizes. Consider using
remunits or CSS custom properties for more predictable spacing.- padding: 5%; + padding: 2rem;- margin: 5% 0; + margin: 1.5rem 0;Also applies to: 25-25
src/modules/fetchSearchResults.ts (1)
21-21: Consider usingundefinedinstead ofvoidin union type.The static analysis tool correctly identifies that
voidin a union type can be confusing. Consider changing the callback return type for better clarity.-function setObserver(callback: () => Promise<string | void>) { +function setObserver(callback: () => Promise<string | undefined>) {🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
index.html (1)
139-140: Move inline styles to CSS.Inline styles should be avoided for better maintainability and separation of concerns.
- <svg xmlns="http://www.w3.org/2000/svg" version="1.0" viewBox="0 0 4172 4172" width="1.5rem" height="1.5rem" - style="transform: scale(1.5);overflow:hidden;"> + <svg id="ytifyIcon" xmlns="http://www.w3.org/2000/svg" version="1.0" viewBox="0 0 4172 4172" width="1.5rem" height="1.5rem">Add to your CSS file:
#ytifyIcon { transform: scale(1.5); overflow: hidden; }src/scripts/list.ts (1)
1-1: Split the long import statement for better readability.This import line is too long and difficult to read. Consider splitting it across multiple lines.
-import { clearListBtn, deleteCollectionBtn, enqueueBtn, importListBtn, listBtnsContainer, listContainer, openInYtBtn, playAllBtn, shareCollectionBtn, removeFromListBtn, renameCollectionBtn, subscribeListBtn, radioCollectionBtn, sortCollectionBtn, queuelist, sortByTitleBtn, sortByArtistBtn } from '../lib/dom'; +import { + clearListBtn, deleteCollectionBtn, enqueueBtn, importListBtn, + listBtnsContainer, listContainer, openInYtBtn, playAllBtn, + shareCollectionBtn, removeFromListBtn, renameCollectionBtn, + subscribeListBtn, radioCollectionBtn, sortCollectionBtn, + queuelist, sortByTitleBtn, sortByArtistBtn +} from '../lib/dom';src/scripts/library.ts (1)
10-36: Improve internationalization consistency and add file type restrictions.
- The "Import From Songshift" text on line 32 should use i18n for consistency
- Add
acceptattributes to file inputs to improve UX- <i class="ri-import-line"></i> Import From Songshift + <i class="ri-import-line"></i> ${i18n('settings_import_from_songshift')}- <input type="file" id="upload_ytify" @change=${importLibrary}/> + <input type="file" id="upload_ytify" accept=".json,application/json" @change=${importLibrary}/>- <input type="file" id="upload_songshift" @change=${async (e: FileEv) => (await import('../modules/importSongshiftStreams')).default(e.target.files[0])}/> + <input type="file" id="upload_songshift" accept=".json,application/json" @change=${async (e: FileEv) => (await import('../modules/importSongshiftStreams')).default(e.target.files[0])}/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
index.html(5 hunks)src/components/CollectionSelector.ts(2 hunks)src/components/Settings/Settings.css(2 hunks)src/components/Settings/index.ts(1 hunks)src/index.d.ts(1 hunks)src/lib/dom.ts(2 hunks)src/locales/en.json(1 hunks)src/modules/fetchSearchResults.ts(4 hunks)src/modules/importSongshiftStreams.ts(1 hunks)src/scripts/library.ts(1 hunks)src/scripts/list.ts(3 hunks)src/stylesheets/header.css(1 hunks)src/stylesheets/upcoming.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/CollectionSelector.ts
- src/stylesheets/upcoming.css
🧰 Additional context used
🪛 Biome (1.9.4)
src/modules/fetchSearchResults.ts
[error] 21-21: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (9)
src/components/Settings/index.ts (1)
14-34: Good refactoring from Solid.js to uhtml!The migration to uhtml for declarative rendering is well-implemented. The use of document fragments for efficiency and proper modal dialog handling shows good practices.
src/stylesheets/header.css (1)
1-75: Well-structured CSS with modern features!The header styling implementation using details/summary pattern for collapsible controls is excellent. The use of CSS variables for theming and smooth transitions enhances user experience.
Note: The
dvminunits used on line 4 are relatively new and may require fallbacks for older browser support. Consider adding a fallback value if supporting legacy browsers is a requirement.src/modules/fetchSearchResults.ts (3)
19-34: Excellent observer lifecycle management!The refactoring to store and return the
IntersectionObserverinstance enables proper cleanup and prevents multiple observers from accumulating. This is a significant improvement for memory management and preventing unexpected behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
55-61: Good duplicate filtering implementation!The addition of duplicate checking using
find()before pushing items to the results array prevents duplicate entries effectively. The filtering logic for both Invidious and Piped APIs is consistent and well-implemented.Also applies to: 102-107
114-120: Proper state reset on filter change!The event listener correctly handles filter changes by disconnecting the current observer, resetting the page counter, and clearing results. This ensures clean state transitions when users change search filters.
src/scripts/list.ts (1)
31-37:Details
✅ Verification successful
Verify that the queue object has the append method.
The code uses
store.queue.append()which suggests the queue has been refactored to an object with methods. Please ensure this method exists and handles the data correctly.
🏁 Script executed:
#!/bin/bash # Description: Verify the queue object structure and its methods in the store # Search for queue definition and methods ast-grep --pattern 'queue = { $$$ }' # Search for queue.append implementation rg -A 5 'queue.*append' # Check store.ts for queue structure fd -e ts -e js store | xargs rg -A 10 'queue'Length of output: 3169
Confirmed:
store.queue.appendis implementedThe
queueobject in the store defines anappend(data, prepend?)method insrc/store.tsand its concrete logic is provided insrc/scripts/queue.ts. No further changes needed.src/lib/dom.ts (1)
33-48: DOM reference updates look good!The new exports and type changes align well with the UI refactoring to use declarative rendering with
uhtml.src/index.d.ts (1)
145-149: Good addition for type safety!The new
FileEvinterface provides proper typing for file input events, making it easier to access thefilesproperty with full type safety and IDE support.src/locales/en.json (1)
63-74: Excellent UX improvements!These label changes significantly improve clarity by:
- Making bulk actions explicit ("Play All", "Enqueue All")
- Clarifying the scope of actions ("Delete Collection", "Rename Collection")
- Better describing behavior ("Toggle Remove", "Start Radio")
These changes will help users understand exactly what each action does, reducing confusion and improving the overall user experience.
| d.forEach((v) => { | ||
| if (v) { | ||
| if (!db[v.src]) db[v.src] = {}; | ||
| db[v.src][v.id] = v as CollectionItem; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix type inconsistency when storing items in database.
The object v includes a src property, but when storing it in the database, you're casting it to CollectionItem which likely doesn't include this property. This creates a type mismatch.
d.forEach((v) => {
if (v) {
if (!db[v.src]) db[v.src] = {};
- db[v.src][v.id] = v as CollectionItem;
+ const { src, ...collectionItem } = v;
+ db[v.src][v.id] = collectionItem;
}
});📝 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.
| d.forEach((v) => { | |
| if (v) { | |
| if (!db[v.src]) db[v.src] = {}; | |
| db[v.src][v.id] = v as CollectionItem; | |
| } | |
| }); | |
| d.forEach((v) => { | |
| if (v) { | |
| if (!db[v.src]) db[v.src] = {}; | |
| const { src, ...collectionItem } = v; | |
| db[v.src][v.id] = collectionItem; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/modules/importSongshiftStreams.ts around lines 56 to 61, the code casts
the variable v to CollectionItem when assigning it to the database, but v also
has a src property that may not be part of CollectionItem, leading to a type
inconsistency. Remove the cast to CollectionItem or define a proper type that
includes src and id properties, then assign v accordingly to ensure type safety
and correctness.
| export default async function(e: File) { | ||
| const songshiftData = JSON.parse(await e.text()) as songshift; | ||
| if (!Array.isArray(songshiftData)) { | ||
| notify('Incompatible Database!'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing.
The JSON.parse call could throw an error if the file contains invalid JSON. Wrap it in a try-catch block to handle parsing errors gracefully.
export default async function(e: File) {
- const songshiftData = JSON.parse(await e.text()) as songshift;
- if (!Array.isArray(songshiftData)) {
- notify('Incompatible Database!');
- return;
- }
+ let songshiftData: songshift;
+ try {
+ songshiftData = JSON.parse(await e.text()) as songshift;
+ if (!Array.isArray(songshiftData)) {
+ notify('Incompatible Database!');
+ return;
+ }
+ } catch (error) {
+ notify('Invalid JSON file!');
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default async function(e: File) { | |
| const songshiftData = JSON.parse(await e.text()) as songshift; | |
| if (!Array.isArray(songshiftData)) { | |
| notify('Incompatible Database!'); | |
| return; | |
| } | |
| export default async function(e: File) { | |
| let songshiftData: songshift; | |
| try { | |
| songshiftData = JSON.parse(await e.text()) as songshift; | |
| if (!Array.isArray(songshiftData)) { | |
| notify('Incompatible Database!'); | |
| return; | |
| } | |
| } catch (error) { | |
| notify('Invalid JSON file!'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/modules/importSongshiftStreams.ts between lines 9 and 14, the JSON.parse
call is not wrapped in a try-catch block, which can cause unhandled exceptions
if the input text is invalid JSON. Wrap the JSON.parse statement in a try-catch
block to catch parsing errors, and notify the user or handle the error
appropriately if parsing fails. This will improve robustness and prevent runtime
crashes due to malformed input.
| .then((metadata: StreamItem) => ({ | ||
| id: metadata.url.substring(9), | ||
| title: metadata.title, | ||
| author: metadata.uploaderName + ' - Topic', | ||
| duration: convertSStoHHMMSS(metadata.duration), | ||
| channelUrl: metadata.uploaderUrl, | ||
| src: src | ||
| })) |
There was a problem hiding this comment.
Improve URL parsing and avoid hardcoded assumptions.
The current implementation has two issues:
- Using
substring(9)to extract the video ID is fragile and assumes the URL always starts with/watch?v= - Hardcoding
- Topicsuffix may not be appropriate for all content
.then((metadata: StreamItem) => ({
- id: metadata.url.substring(9),
+ id: metadata.url.match(/[?&]v=([^&]+)/)?.[1] || metadata.url.substring(9),
title: metadata.title,
- author: metadata.uploaderName + ' - Topic',
+ author: metadata.uploaderName,
duration: convertSStoHHMMSS(metadata.duration),
channelUrl: metadata.uploaderUrl,
src: src
}))📝 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.
| .then((metadata: StreamItem) => ({ | |
| id: metadata.url.substring(9), | |
| title: metadata.title, | |
| author: metadata.uploaderName + ' - Topic', | |
| duration: convertSStoHHMMSS(metadata.duration), | |
| channelUrl: metadata.uploaderUrl, | |
| src: src | |
| })) | |
| .then((metadata: StreamItem) => ({ | |
| id: metadata.url.match(/[?&]v=([^&]+)/)?.[1] || metadata.url.substring(9), | |
| title: metadata.title, | |
| author: metadata.uploaderName, | |
| duration: convertSStoHHMMSS(metadata.duration), | |
| channelUrl: metadata.uploaderUrl, | |
| src: src | |
| })) |
🤖 Prompt for AI Agents
In src/modules/importSongshiftStreams.ts between lines 28 and 35, the code
extracts the video ID using substring(9), which is fragile and assumes the URL
always starts with a specific pattern. Replace this with a more robust method,
such as using URL parsing or regex to extract the video ID. Also, review the
addition of ' - Topic' to the author field to ensure it is appropriate for all
cases or make it configurable. Implement these improvements to make URL parsing
more reliable and avoid hardcoded assumptions.
| <i class="ri-draggable"></i> | ||
| </li> | ||
| <li id="sortCollectionBtn"> | ||
| <i class="ri-draggable"></i> Sort Title A↔Z | ||
| </li> | ||
| <li id="sortCollectionBtn"> | ||
| <i class="ri-draggable"></i> Sort Artist A↔Z | ||
| </li> |
There was a problem hiding this comment.
Remove duplicate IDs - this is invalid HTML.
There are three elements with the same ID sortCollectionBtn. IDs must be unique in HTML. This will cause issues with JavaScript selectors and event handlers.
- <li id="sortCollectionBtn" data-translation-aria-label="list_sort">
+ <li id="sortListBtn" data-translation-aria-label="list_sort">
<i class="ri-draggable"></i>
</li>
- <li id="sortCollectionBtn">
+ <li id="sortByTitleBtn">
<i class="ri-draggable"></i> Sort Title A↔Z
</li>
- <li id="sortCollectionBtn">
+ <li id="sortByArtistBtn">
<i class="ri-draggable"></i> Sort Artist A↔Z
</li>📝 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.
| <i class="ri-draggable"></i> | |
| </li> | |
| <li id="sortCollectionBtn"> | |
| <i class="ri-draggable"></i> Sort Title A↔Z | |
| </li> | |
| <li id="sortCollectionBtn"> | |
| <i class="ri-draggable"></i> Sort Artist A↔Z | |
| </li> | |
| <i class="ri-draggable"></i> | |
| </li> | |
| <li id="sortListBtn" data-translation-aria-label="list_sort"> | |
| <i class="ri-draggable"></i> | |
| </li> | |
| <li id="sortByTitleBtn"> | |
| <i class="ri-draggable"></i> Sort Title A↔Z | |
| </li> | |
| <li id="sortByArtistBtn"> | |
| <i class="ri-draggable"></i> Sort Artist A↔Z | |
| </li> |
🤖 Prompt for AI Agents
In index.html around lines 287 to 294, there are multiple elements with the same
ID `sortCollectionBtn`, which is invalid HTML as IDs must be unique. To fix
this, assign unique IDs to each element, such as `sortTitleBtn` and
`sortArtistBtn`, or remove the IDs if they are not needed, and update any
related JavaScript selectors accordingly.
| <input type="file" id="upload_songshift" @change=${async (e: FileEv) => (await import('../modules/importSongshiftStreams')).default(e.target.files[0])}/> | ||
| </li> | ||
| `; | ||
| render(libraryActions, actionTemplate); | ||
|
|
||
|
|
||
|
|
||
| async function importLibrary(e: FileEv) { |
There was a problem hiding this comment.
Define or import the FileEv type.
The FileEv type is used but not defined or imported in this file, which will cause TypeScript compilation errors.
Add the type definition at the top of the file:
type FileEv = Event & { target: HTMLInputElement };Or import it if it's defined elsewhere:
import { FileEv } from "../lib/types";🤖 Prompt for AI Agents
In src/scripts/library.ts around lines 34 to 41, the type `FileEv` is used in
the event handler but is not defined or imported, which will cause TypeScript
errors. Define the `FileEv` type as `Event & { target: HTMLInputElement }` at
the top of the file or import it from the appropriate module to ensure proper
type checking.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/stylesheets/list.css (2)
41-43: Duplicate: groupli { display: none }rules
This rule for.reservedis the same pattern as in.channel; see my previous suggestion to DRY these selectors by combining them.
64-66: Duplicate: groupli { display: none }rules
This rule for.supermix&.sharedfits the same DRY refactor; consider merging with other state selectors as suggested above.
🧹 Nitpick comments (5)
src/stylesheets/list.css (3)
3-10: Refine the pseudo-element styling for list items
The addition ofli::after { content: attr(aria-label); }enhances accessibility, but consider explicitly settingdisplay(e.g.,inline-block) andpointer-events: noneon the pseudo-element to prevent it from intercepting clicks or breaking layout.
30-32: Group repetitiveli { display: none }rules into a single selector
Multiple states (.channel,.reserved, etc.) independently hide all list items. You can DRY this by combining selectors, e.g.:&.channel, &.reserved, &.supermix, &.shared { li { display: none; } }
88-92: Ensureattr(data-state)content styling is consistent
Switchingfont-familytoinheritis good. Consider adding a fallback or explicitdisplay(e.g.,inline-block) to the pseudo-element to maintain layout consistency and prevent clipping. Also verify that thedata-stateattribute is set on the<i>element.index.html (2)
139-140: Add ARIA attributes to decorative SVG for accessibility.The inline SVG used here is purely decorative. Without ARIA attributes, screen readers may announce it unexpectedly. Please mark it as hidden:
- <svg xmlns="http://www.w3.org/2000/svg" version="1.0" viewBox="0 0 4172 4172" width="1.5rem" height="1.5rem" - style="transform: scale(1.5);overflow:hidden;"> + <svg xmlns="http://www.w3.org/2000/svg" version="1.0" viewBox="0 0 4172 4172" + width="1.5rem" height="1.5rem" + style="transform: scale(1.5);overflow:hidden;" + aria-hidden="true" focusable="false">
209-215: Ensure semantic structure for Library section.For consistency and accessibility:
- Change
<p data-translation="nav_library">Library</p>to<h2 data-translation="nav_library">Library</h2>.- Provide an
aria-labelon the<summary>element (e.g.,aria-label="Library actions").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
index.html(5 hunks)src/stylesheets/list.css(5 hunks)
🔇 Additional comments (2)
src/stylesheets/list.css (1)
18-19: Verify that new sort button IDs exist in the markup
You’ve hidden#sortByTitleBtnand#sortByArtistBtnin the.playliststate. Ensure corresponding<li id="sortByTitleBtn">and<li id="sortByArtistBtn">elements are present and have properaria-labelattributes; otherwise, these rules won’t apply.index.html (1)
159-159: Empty state message is clear.The
<p data-translation="upcoming_info">The Queue is Empty.</p>effectively communicates the empty state.
| <header> | ||
| <p data-translation="nav_upcoming">Upcoming</p> | ||
| <details> | ||
| <summary><i class="ri-more-2-fill"></i></summary> | ||
| <ul id="queuetools"></ul> | ||
| </details> | ||
| </header> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use semantic headings and label details summary for accessibility.
In the Upcoming section header:
- Change the title from a
<p>to a heading (e.g.,<h2 data-translation="nav_upcoming">Upcoming</h2>) to preserve document outline. - Add an
aria-label(or visible text) to the<summary>element, for examplearia-label="Upcoming actions", so assistive technologies announce its purpose.
🤖 Prompt for AI Agents
In index.html between lines 151 and 157, the Upcoming section header uses a
paragraph tag instead of a semantic heading, and the details summary lacks an
accessible label. Replace the <p> tag with an appropriate heading tag such as
<h2> to improve document structure. Add an aria-label attribute to the <summary>
element, such as aria-label="Upcoming actions", to clarify its purpose for
assistive technologies. This enhances accessibility and maintains semantic
correctness.
| <header> | ||
| <p data-translation="nav_search">Search</p> | ||
| <details> | ||
| <summary><i class="ri-more-2-fill"></i></summary> | ||
| <ul> | ||
| <li id="settingsHandler" data-translation="nav_settings">Settings</li> | ||
| <li><a data-translation="settings_changelog" href="https://github.com/n-ce/ytify/wiki/changelog" | ||
| target="_blank">Changelog</a></li> | ||
| <li><a href="https://github.com/n-ce/ytify" target="_blank">Github</a></li> | ||
| <li><a href="https://t.me/ytifytg" target="_blank">Telegram</a></li> | ||
| <li><a href="https://matrix.to/#/#ytify:matrix.org" target="_blank">Matrix</a></li> | ||
| <li id="fullscreenBtn" data-translation="settings_fullscreen">Fullscreen</li> | ||
| </ul> | ||
| </details> | ||
| </header> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve semantics and accessibility of controls menu.
In the Search section header:
- Replace
<p data-translation="nav_search">Search</p>with a heading tag (<h2>or<h3>as appropriate). - Wrap each interactive item (
Settings,Fullscreen) in a native control, e.g.:<li><button id="settingsHandler" data-translation="nav_settings">Settings</button></li>
- Add an
aria-labelto the<summary>(aria-label="Search options") so its purpose is clear to screen readers.
🤖 Prompt for AI Agents
In index.html around lines 165 to 179, the Search section header uses a
paragraph tag which is not semantically appropriate for a heading. Replace the
<p> tag with a suitable heading tag like <h2> or <h3>. Additionally, the list
items contain non-interactive elements; replace the <li> elements containing
text with <li> elements containing <button> elements for better accessibility.
Finally, add an aria-label attribute to the <summary> element to clarify its
purpose for screen readers. These changes improve semantic structure and
accessibility of the controls menu.
| <dialog id="settings"> | ||
|
|
||
|
|
||
| <div id="actionsContainer"> | ||
| <form netlify name="feedback" method="POST"> | ||
| <textarea data-translation-placeholder="settings_feedback_placeholder" name="->" id="netlifyForm" | ||
| required></textarea> | ||
|
|
||
| <button data-translation="settings_feedback_submit" type="submit">Submit Feedback</button> | ||
|
|
||
| <button data-translation="settings_clear_cache" id="clearCacheBtn" type="button">Clear Caches</button> | ||
|
|
||
| <button data-translation="settings_restore" id="restoreSettingsBtn" type="button">Restore Settings</button> | ||
|
|
||
| <button data-translation="settings_export" id="exportSettingsBtn" type="button">Export Settings</button> | ||
| <button type="button"> | ||
| <label data-translation="settings_import" for="importSettingsBtn">Import Settings</label> | ||
| <input type="file" id="importSettingsBtn" class="hide"> | ||
| </button> | ||
| </form> | ||
|
|
||
| </div> | ||
| </dialog> |
There was a problem hiding this comment.
Fix feedback form attributes and dialog usability.
The new <dialog id="settings"> has several critical issues:
- The
<textarea>field’sname="->"is invalid for form submission. - Netlify requires a hidden field to identify the form (
<input type="hidden" name="form-name" value="feedback">). - There’s no close control or accessible labeling on the dialog.
Apply this diff:
- <dialog id="settings">
+ <dialog id="settings" role="dialog" aria-modal="true" aria-labelledby="settingsTitle">
+ <h2 id="settingsTitle" data-translation="nav_settings">Settings</h2>
+ <button type="button" id="closeSettingsBtn" aria-label="Close settings dialog">×</button>
<div id="actionsContainer">
- <form netlify name="feedback" method="POST">
+ <form netlify name="feedback" method="POST">
+ <input type="hidden" name="form-name" value="feedback">
- <textarea data-translation-placeholder="settings_feedback_placeholder" name="->" id="netlifyForm"
+ <textarea data-translation-placeholder="settings_feedback_placeholder"
+ name="feedback" id="feedbackTextarea"
required></textarea>These changes will ensure the feedback form submits correctly and the dialog is operable and accessible.
📝 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.
| <dialog id="settings"> | |
| <div id="actionsContainer"> | |
| <form netlify name="feedback" method="POST"> | |
| <textarea data-translation-placeholder="settings_feedback_placeholder" name="->" id="netlifyForm" | |
| required></textarea> | |
| <button data-translation="settings_feedback_submit" type="submit">Submit Feedback</button> | |
| <button data-translation="settings_clear_cache" id="clearCacheBtn" type="button">Clear Caches</button> | |
| <button data-translation="settings_restore" id="restoreSettingsBtn" type="button">Restore Settings</button> | |
| <button data-translation="settings_export" id="exportSettingsBtn" type="button">Export Settings</button> | |
| <button type="button"> | |
| <label data-translation="settings_import" for="importSettingsBtn">Import Settings</label> | |
| <input type="file" id="importSettingsBtn" class="hide"> | |
| </button> | |
| </form> | |
| </div> | |
| </dialog> | |
| <dialog id="settings" role="dialog" aria-modal="true" aria-labelledby="settingsTitle"> | |
| <h2 id="settingsTitle" data-translation="nav_settings">Settings</h2> | |
| <button type="button" id="closeSettingsBtn" aria-label="Close settings dialog">×</button> | |
| <div id="actionsContainer"> | |
| <form netlify name="feedback" method="POST"> | |
| <input type="hidden" name="form-name" value="feedback"> | |
| <textarea data-translation-placeholder="settings_feedback_placeholder" | |
| name="feedback" id="feedbackTextarea" | |
| required></textarea> | |
| <button data-translation="settings_feedback_submit" type="submit">Submit Feedback</button> | |
| <button data-translation="settings_clear_cache" id="clearCacheBtn" type="button">Clear Caches</button> | |
| <button data-translation="settings_restore" id="restoreSettingsBtn" type="button">Restore Settings</button> | |
| <button data-translation="settings_export" id="exportSettingsBtn" type="button">Export Settings</button> | |
| <button type="button"> | |
| <label data-translation="settings_import" for="importSettingsBtn">Import Settings</label> | |
| <input type="file" id="importSettingsBtn" class="hide"> | |
| </button> | |
| </form> | |
| </div> | |
| </dialog> |
🤖 Prompt for AI Agents
In index.html between lines 310 and 332, the feedback form has an invalid name
attribute on the textarea, lacks a hidden input for Netlify form identification,
and the dialog has no close button or accessible label. Fix these issues by
replacing the textarea's name with a valid name, adding a hidden input with name
"form-name" and value "feedback" inside the form, and include a close button
within the dialog for better usability and accessibility.
| <header> | ||
| <p id="listTitle">List</p> | ||
| <details> | ||
| <summary><i class="ri-more-2-fill"></i></summary> | ||
| <ul id="listTools"> | ||
| <li id="playAllBtn" data-translation-aria-label="list_play"> | ||
| <i class="ri-play-large-line"></i> | ||
| </li> | ||
| <li id="enqueueAllBtn" data-translation-aria-label="list_enqueue"> | ||
| <i class="ri-list-check-2"></i> | ||
| </li> | ||
| <li id="importListBtn" data-translation-aria-label="list_import"> | ||
| <i class="ri-import-line"></i> | ||
| </li> | ||
| <li id="subscribeListBtn"> | ||
| <i class="ri-stack-line" data-state="Subscribe"></i> | ||
| </li> | ||
| <li id="viewOnYTBtn"> | ||
| <i class="ri-external-link-line" data-state=" View on YouTube"></i> | ||
| </li> | ||
| <li id="clearListBtn" data-translation-aria-label="list_clear_all"> | ||
| <i class="ri-close-line"></i> | ||
| </li> | ||
| <li id="removeFromListBtn" data-translation-aria-label="list_remove"> | ||
| <i class="ri-subtract-line"></i> | ||
| </li> | ||
| <li id="deleteCollectionBtn" data-translation-aria-label="list_delete"> | ||
| <i class="ri-delete-bin-2-line"></i> | ||
| </li> | ||
| <li id="renameCollectionBtn" data-translation-aria-label="list_rename"> | ||
| <i class="ri-edit-line"></i> | ||
| </li> | ||
| <li id="shareCollectionBtn" data-translation-aria-label="list_share"> | ||
| <i class="ri-link"></i> | ||
| </li> | ||
| <li id="radioCollectionBtn" data-translation-aria-label="list_radio"> | ||
| <i class="ri-radio-line"></i> | ||
| </li> | ||
| <li id="sortCollectionBtn" data-translation-aria-label="list_sort"> | ||
| <i class="ri-draggable"></i> | ||
| </li> | ||
| <li id="sortByTitleBtn"> | ||
| <i class="ri-draggable"></i> Sort Title A↔Z | ||
| </li> | ||
| <li id="sortByArtistBtn"> | ||
| <i class="ri-draggable"></i> Sort Artist A↔Z | ||
| </li> | ||
| </ul> | ||
| </details> | ||
| </header> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert list tool items into accessible controls.
The <li> elements under #listTools act as buttons but aren’t semantically interactive. To improve keyboard navigation and screen-reader support:
- Wrap the icon in a
<button>and use the existingdata-translation-aria-labelasaria-label. - Ensure IDs are descriptive and unique (for example, rename
sortCollectionBtntoopenSortMenuBtn). - Example refactor for one item:
- <li id="playAllBtn" data-translation-aria-label="list_play"> - <i class="ri-play-large-line"></i> - </li> + <li> + <button id="playAllBtn" aria-label="Play all"> + <i class="ri-play-large-line"></i> + </button> + </li>
🤖 Prompt for AI Agents
In index.html between lines 248 and 297, the list items under #listTools are
used as buttons but are marked up as <li> elements, which are not semantically
interactive. Convert each <li> into a <button> element, set the aria-label
attribute using the existing data-translation-aria-label, and remove the <li>
tags. Update IDs to be more descriptive and unique, such as renaming
sortCollectionBtn to openSortMenuBtn. This improves accessibility and keyboard
navigation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/scripts/list.ts (2)
39-41: Improve type safety for event target.The type assertion to
HTMLLIElementfollowed by a runtime check suggests uncertainty about the actual type. Consider using a type guard or more specific event delegation.- const btn = e.target as HTMLLIElement; - if (!btn.matches('li')) + const btn = e.target as HTMLElement; + if (!btn.matches('li') || !(btn instanceof HTMLLIElement))
48-51: Consider using queue management methods consistently.Direct manipulation of
store.queue.list.lengthandqueuelist.innerHTMLbypasses potential queue management logic. Consider using queue methods if available.- store.queue.list.length = 0; - queuelist.innerHTML = ''; + store.queue.clear(); // if such method exists
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scripts/list.ts(3 hunks)
🔇 Additional comments (3)
src/scripts/list.ts (3)
30-36: LGTM! Clean queue utility function.The
listToQfunction provides a clear abstraction for adding list items to the queue and navigating to the upcoming view.
102-108: Acknowledge improved sorting implementation.Good refactoring! The new
sortfunction with the field parameter successfully addresses the previous DRY concern by eliminating duplicate sorting logic for title and author fields.
110-124:Details
✅ Verification successful
Verify CollectionItem type availability.
The function signature uses
keyof CollectionItembut this type isn't imported or defined in this file. Ensure the type is available in scope.
🏁 Script executed:
#!/bin/bash # Search for CollectionItem type definition rg -A 5 "interface CollectionItem|type CollectionItem"Length of output: 271
No import needed for CollectionItem
The
CollectionItemtype is declared insrc/index.d.tsand is picked up globally by TypeScript, so no additional import is required.
| function sort(field: keyof CollectionItem | '' = '') { | ||
|
|
||
| let clxnArr = Object.values(db[id]); | ||
| if (field) { | ||
| clxnArr = clxnArr.sort((a, b) => { | ||
| if (a[field]! > b[field]!) return 1; | ||
| if (a[field]! < b[field]!) return -1; | ||
| return 0; | ||
| }); | ||
| db[id] = Object.fromEntries(clxnArr.map((v) => [v.id, v])); | ||
| saveDB(db); | ||
| } | ||
| render(listContainer, html``); | ||
| renderCollection(clxnArr, sortCollectionBtn.classList.contains('checked')); | ||
| } |
There was a problem hiding this comment.
Fix logic inconsistency in sort function.
The sort function has inconsistent behavior: when a field is provided, it sorts and saves to DB, but when no field is provided (empty string), it renders without sorting. This could lead to unexpected behavior.
function sort(field: keyof CollectionItem | '' = '') {
let clxnArr = Object.values(db[id]);
if (field) {
clxnArr = clxnArr.sort((a, b) => {
if (a[field]! > b[field]!) return 1;
if (a[field]! < b[field]!) return -1;
return 0;
});
db[id] = Object.fromEntries(clxnArr.map((v) => [v.id, v]));
saveDB(db);
}
+ // When no field is provided, we still need consistent behavior
+ // Either always sort by default field or don't modify DB state
render(listContainer, html``);
renderCollection(clxnArr, sortCollectionBtn.classList.contains('checked'));
}📝 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.
| function sort(field: keyof CollectionItem | '' = '') { | |
| let clxnArr = Object.values(db[id]); | |
| if (field) { | |
| clxnArr = clxnArr.sort((a, b) => { | |
| if (a[field]! > b[field]!) return 1; | |
| if (a[field]! < b[field]!) return -1; | |
| return 0; | |
| }); | |
| db[id] = Object.fromEntries(clxnArr.map((v) => [v.id, v])); | |
| saveDB(db); | |
| } | |
| render(listContainer, html``); | |
| renderCollection(clxnArr, sortCollectionBtn.classList.contains('checked')); | |
| } | |
| function sort(field: keyof CollectionItem | '' = '') { | |
| let clxnArr = Object.values(db[id]); | |
| if (field) { | |
| clxnArr = clxnArr.sort((a, b) => { | |
| if (a[field]! > b[field]!) return 1; | |
| if (a[field]! < b[field]!) return -1; | |
| return 0; | |
| }); | |
| db[id] = Object.fromEntries(clxnArr.map((v) => [v.id, v])); | |
| saveDB(db); | |
| } | |
| // When no field is provided, we still need consistent behavior | |
| // Either always sort by default field or don't modify DB state | |
| render(listContainer, html``); | |
| renderCollection(clxnArr, sortCollectionBtn.classList.contains('checked')); | |
| } |
🤖 Prompt for AI Agents
In src/scripts/list.ts around lines 110 to 124, the sort function sorts and
updates the database only when a field is specified, but it still calls render
and renderCollection regardless of whether sorting occurred. To fix this, move
the render calls inside the if (field) block so they only execute after sorting
and updating the database, ensuring consistent behavior and avoiding unnecessary
rendering when no sorting is performed.
1 participant
Summary by CodeRabbit
i18nfunction.