Conversation
WalkthroughThe changes refactor the modal component system by replacing Solid.js-based implementations with asynchronous functions that render content into HTMLDialogElements using the uhtml library or direct DOM manipulation. Lyrics and video watching dialogs are now dynamically imported and initialized with dialogs, and a new Selector component is introduced using uhtml. The update prompt and player logic are adapted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionsMenu
participant Dialog
participant Lyrics/WatchVideo Module
participant Store/API
User->>ActionsMenu: Click "Show Lyrics" or "Watch Video"
ActionsMenu->>Dialog: Create <dialog> element and append to body
ActionsMenu->>Lyrics/WatchVideo Module: Dynamically import module
Lyrics/WatchVideo Module->>Dialog: Initialize and render content
Lyrics/WatchVideo Module->>Store/API: Fetch data (lyrics/video streams)
Store/API-->>Lyrics/WatchVideo Module: Return data
Lyrics/WatchVideo Module->>Dialog: Update dialog content
User->>Dialog: Interact (close, select options, etc.)
Dialog->>Lyrics/WatchVideo Module: Trigger cleanup on close
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 6
🧹 Nitpick comments (5)
src/components/Selector.ts (2)
20-20: Missing TranslationKeys type importThe code uses
TranslationKeystype but it's not explicitly imported. This type appears to be globally available, but explicitly importing it would improve code clarity and type safety.import { Hole, html } from "uhtml"; import { i18n } from "../lib/utils"; +import type { TranslationKeys } from "../types"; // Add explicit import
23-29: Optimize onmount callback checkThe
_.onmountcallback is checked twice - once before creating the Promise and again inside the Promise's then handler. This creates unnecessary conditional logic.<select ref=${(s: HTMLSelectElement) => { - if (_.onmount) Promise.resolve().then(() => { if (s.isConnected && _.onmount) _.onmount(s) }); }}src/components/ActionsMenu.tsx (1)
129-135: PrefershowModal()over settingopen = true
dialog.open = trueproduces a non-modal dialog.
BecauseWatchVideorenders interactive media, accidental clicks on the page underneath are undesirable. Callingdialog.showModal()yields proper modality and a backdrop without extra CSS.-const dialog = document.createElement('dialog') as HTMLDialogElement; -dialog.open = true; +const dialog = document.createElement('dialog') as HTMLDialogElement; +dialog.showModal();src/components/UpdatePrompt.ts (1)
20-22: Open external links safely
open(commitsLink);opens the changelog in the current browsing context, which can unexpectedly replace the app.
Usewindow.open(url, '_blank', 'noopener')to create a new tab and preventwindow.opener-based attacks.-open(commitsLink); +window.open(commitsLink, '_blank', 'noopener');src/components/Lyrics.ts (1)
36-55: Guard against missed DOM look-ups
dialog.firstElementChild!.childrenassumes rendering succeeded and the DOM is still intact.
If the template fails to render (e.g.uhtmlthrows)firstElementChildwill benulland the exclamation mark suppresses TypeScript’s helpful warning, leading to a runtime crash on the firstlrcSynccall.Instead:
-const p = dialog.firstElementChild!.children; +const section = dialog.firstElementChild; +if (!section) return; +const p = section.children;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
package.json(1 hunks)src/components/ActionsMenu.tsx(3 hunks)src/components/Lyrics.ts(1 hunks)src/components/Lyrics.tsx(0 hunks)src/components/Selector.ts(1 hunks)src/components/UpdatePrompt.ts(1 hunks)src/components/WatchVideo.ts(1 hunks)src/components/WatchVideo.tsx(0 hunks)src/lib/player.ts(1 hunks)src/main.ts(1 hunks)src/modules/start.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/Lyrics.tsx
- src/components/WatchVideo.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/UpdatePrompt.ts (1)
src/lib/utils.ts (1)
i18n(40-45)
src/components/ActionsMenu.tsx (1)
src/lib/store.ts (1)
store(5-97)
src/components/Lyrics.ts (3)
src/lib/dom.ts (1)
loadingScreen(41-41)src/lib/store.ts (1)
store(5-97)src/lib/utils.ts (1)
notify(69-77)
🔇 Additional comments (7)
package.json (1)
18-18: Upgrade @types/node patch version to ^22.15.18
This is a safe patch-level bump for your Node.js type definitions. Please ensure your TypeScript build and any Netlify edge‐function types still compile cleanly with the updated defs.src/components/Selector.ts (3)
1-3: Appropriate imports for uhtml-based componentThe imports look good, including both the necessary
Holeandhtmlfrom uhtml, and thei18nutility for internationalization.
4-15: Well-typed props interfaceThe component function has a well-structured props interface with appropriate types:
idfor element identificationlabelfor user-facing texthandlerwith precise event typingchildrenusing uhtml'sHoletype- Optional
onmountcallback for post-connection operations
17-34: Well-structured uhtml template implementationThe template structure follows best practices:
- Label properly connected to select via
forattribute- Internationalization applied to label text
- Event handler properly bound with
@change- Children properly rendered inside the select element
- Asynchronous onmount pattern ensures DOM connection before callback execution
src/main.ts (1)
44-45: Refactor to dialog-based component approachThis change aligns with the overall refactoring strategy - replacing direct uhtml rendering with passing a dialog element to the component function. The implementation correctly maintains the dialog event handling while simplifying the rendering approach.
src/modules/start.ts (1)
63-69: Refactored to use dialog-based WatchVideo componentThe implementation properly:
- Creates a dialog element with TypeScript casting
- Sets the dialog's open state and class name
- Appends it to document body
- Dynamically imports the WatchVideo component
- Passes the dialog to the component function
This approach is consistent with the overall refactoring from Solid.js rendering to dialog-based components using uhtml.
src/lib/player.ts (1)
14-20: Consistent implementation of dialog-based WatchVideo componentThis implementation mirrors the approach used in start.ts, creating and configuring a dialog element before passing it to the WatchVideo component. The change aligns with the project's move from Solid.js rendering to dialog-based components using uhtml.
| const dialog = document.createElement('dialog') as HTMLDialogElement; | ||
| dialog.className = 'displayer'; | ||
| dialog.addEventListener('click', () => { | ||
| dialog.close(); | ||
| dialog.remove(); | ||
| store.lrcSync = () => ''; | ||
| }); | ||
| document.body.appendChild(dialog); | ||
| import('./Lyrics.ts') | ||
| .then(async mod => await mod.default(dialog)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive error handling when the dynamic import fails
If the Lyrics.ts module fails to load (network hiccup, 404 after future refactor, etc.) the dialog node you just appended will sit in the DOM forever and the user receives no feedback. Wrap the import in a try/catch and close / remove the dialog in the error branch, optionally notify()-ing the user.
-import('./Lyrics.ts')
- .then(async mod => await mod.default(dialog))
+try {
+ const mod = await import('./Lyrics.ts');
+ await mod.default(dialog);
+} catch (err) {
+ console.error('Failed to load Lyrics module', err);
+ dialog.close();
+ dialog.remove();
+ notify('Unable to load lyrics. Please try again later.');
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ActionsMenu.tsx around lines 106 to 115, the dynamic import of
'Lyrics.ts' lacks error handling, causing the dialog to remain in the DOM
indefinitely if the import fails. Wrap the import statement in a try/catch
block; in the catch block, close and remove the dialog element and optionally
notify the user of the failure to load the module.
| const data = await fetch( | ||
| `https://lrclib.net/api/get?track_name=${store.actionsMenu.title}&artist_name=${store.actionsMenu.author.slice(0, -8)}`, | ||
| { | ||
| headers: { | ||
| 'Lrclib-Client': `ytify ${Build} (https://github.com/n-ce/ytify)` | ||
| } | ||
| }) |
There was a problem hiding this comment.
Encode user-generated strings in the request URL
Track titles or artist names containing spaces, &, ?, etc. will break the LRCLib request.
Wrap both parameters with encodeURIComponent.
-`https://lrclib.net/api/get?track_name=${store.actionsMenu.title}&artist_name=${store.actionsMenu.author.slice(0, -8)}`
+`https://lrclib.net/api/get?track_name=${encodeURIComponent(store.actionsMenu.title)}&artist_name=${encodeURIComponent(store.actionsMenu.author.slice(0, -8))}`While touching this code, double-check that the global Build variable is always defined in production builds; otherwise a reference error will occur in development mode.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = await fetch( | |
| `https://lrclib.net/api/get?track_name=${store.actionsMenu.title}&artist_name=${store.actionsMenu.author.slice(0, -8)}`, | |
| { | |
| headers: { | |
| 'Lrclib-Client': `ytify ${Build} (https://github.com/n-ce/ytify)` | |
| } | |
| }) | |
| const data = await fetch( | |
| `https://lrclib.net/api/get?track_name=${encodeURIComponent(store.actionsMenu.title)}&artist_name=${encodeURIComponent(store.actionsMenu.author.slice(0, -8))}`, | |
| { | |
| headers: { | |
| 'Lrclib-Client': `ytify ${Build} (https://github.com/n-ce/ytify)` | |
| } | |
| } | |
| ) |
🤖 Prompt for AI Agents
In src/components/Lyrics.ts around lines 10 to 16, the track_name and
artist_name parameters in the fetch URL are not URL-encoded, which can cause the
request to break if they contain special characters. Fix this by wrapping
store.actionsMenu.title and the sliced store.actionsMenu.author with
encodeURIComponent before inserting them into the URL string. Also, verify that
the global Build variable is defined in all environments to prevent reference
errors.
| audio.src = proxyHandler(audioArray[0].url); | ||
| audio.currentTime = video.currentTime; | ||
| loadingScreen.close(); |
There was a problem hiding this comment.
Avoid out-of-bounds access when audioArray is empty
audioArray[0] will throw if no compatible audio stream is returned.
Add a guard and surface the error to the user:
-if (audioArray.length) {
- audio.src = proxyHandler(audioArray[0].url);
- audio.currentTime = video.currentTime;
-} else {
- loadingScreen.close();
- notify('No playable audio stream found.');
- return;
-}
+if (!audioArray.length) {
+ loadingScreen.close();
+ notify('No playable audio stream found.');
+ return;
+}
+
+audio.src = proxyHandler(audioArray[0].url);
+audio.currentTime = video.currentTime;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/WatchVideo.ts around lines 192 to 194, the code accesses
audioArray[0] without checking if audioArray is empty, which can cause an
out-of-bounds error. Add a guard condition to check if audioArray has at least
one element before accessing audioArray[0]. If it is empty, handle the error
gracefully by showing an appropriate message to the user instead of proceeding
with the assignment.
| <div> | ||
|
|
||
| <button @click=${close}>Close</button> | ||
|
|
||
| ${media.video.length ? | ||
| Selector({ | ||
| id: 'videoCodecSelector', | ||
| label: '', | ||
| handler: (_) => { | ||
| video.src = proxyHandler(_.target.value); | ||
| video.currentTime = audio.currentTime; | ||
| if (savedQ) | ||
| save('watchMode', _.target.selectedOptions[0].textContent as string); | ||
| }, | ||
| children: html` | ||
| <option>Video</option> | ||
| ${media.video.map(f => html` | ||
| <option | ||
| value=${f[1]} | ||
| selected=${f[0] === savedQ} | ||
| >${f[0]}</option> | ||
| `) | ||
| }`, | ||
| onmount: (_) => { | ||
| if (savedQ) | ||
| video.src = proxyHandler(_.value); | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set an initial video.src to guarantee playback
When no previous quality preference is saved, the selector’s onmount hook might not execute and video.src remains empty, so the player stays black while audio plays.
Assign the first available stream as a safe default:
@@
- onmount: (_) => {
- if (savedQ)
- video.src = proxyHandler(_.value);
- }
+ onmount: (_) => {
+ const chosen = savedQ ? _.value : media.video[0]?.[1];
+ if (chosen) video.src = proxyHandler(chosen);
+ }📝 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.
| <div> | |
| <button @click=${close}>Close</button> | |
| ${media.video.length ? | |
| Selector({ | |
| id: 'videoCodecSelector', | |
| label: '', | |
| handler: (_) => { | |
| video.src = proxyHandler(_.target.value); | |
| video.currentTime = audio.currentTime; | |
| if (savedQ) | |
| save('watchMode', _.target.selectedOptions[0].textContent as string); | |
| }, | |
| children: html` | |
| <option>Video</option> | |
| ${media.video.map(f => html` | |
| <option | |
| value=${f[1]} | |
| selected=${f[0] === savedQ} | |
| >${f[0]}</option> | |
| `) | |
| }`, | |
| onmount: (_) => { | |
| if (savedQ) | |
| video.src = proxyHandler(_.value); | |
| } | |
| }) | |
| <div> | |
| <button @click=${close}>Close</button> | |
| ${media.video.length ? | |
| Selector({ | |
| id: 'videoCodecSelector', | |
| label: '', | |
| handler: (_) => { | |
| video.src = proxyHandler(_.target.value); | |
| video.currentTime = audio.currentTime; | |
| if (savedQ) | |
| save('watchMode', _.target.selectedOptions[0].textContent as string); | |
| }, | |
| children: html` | |
| <option>Video</option> | |
| ${media.video.map(f => html` | |
| <option | |
| value=${f[1]} | |
| selected=${f[0] === savedQ} | |
| >${f[0]}</option> | |
| `) | |
| }`, | |
| onmount: (_) => { | |
| const chosen = savedQ ? _.value : media.video[0]?.[1]; | |
| if (chosen) video.src = proxyHandler(chosen); | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/components/WatchVideo.ts between lines 152 and 179, the video element's
src is not set initially if there is no saved quality preference, causing the
player to remain black while audio plays. To fix this, assign the first
available video stream URL to video.src as a default before rendering the
selector, ensuring the video source is always set for playback even when no
saved preference exists.
| .filter(f => { | ||
| const av1 = hasAv1 && supportsAv1 && f.type.includes('av01'); | ||
| if (av1) return true; | ||
| const vp9 = !hasAv1 && f.type.includes('vp9'); | ||
| if (vp9) return true; | ||
| const avc = !hasVp9 && f.type.includes('avc1'); | ||
| if (avc) return true; | ||
| }) | ||
| .map(f => ([f.resolution, f.url])); |
There was a problem hiding this comment.
Codec-selection filter can drop all streams when AV1 is available but unsupported
If the API returns AV1 streams and the browser doesn’t support AV1, hasAv1 is truthy, supportsAv1 is false – the first clause rejects the item, but vp9 and avc branches are also skipped because of the !hasAv1 / !hasVp9 checks.
Result: media.video becomes empty ⇒ no video rendered, silent failure.
Refactor the filter to test each codec independently and short-circuit only when a match is found:
-media.video = data.videoStreams
- .filter(f => {
- const av1 = hasAv1 && supportsAv1 && f.type.includes('av01');
- if (av1) return true;
- const vp9 = !hasAv1 && f.type.includes('vp9');
- if (vp9) return true;
- const avc = !hasVp9 && f.type.includes('avc1');
- if (avc) return true;
- })
+media.video = data.videoStreams.filter(f => {
+ if (supportsAv1 && f.type.includes('av01')) return true; // prefer AV1
+ if (f.type.includes('vp9')) return true; // else VP9
+ return f.type.includes('avc1'); // fallback AVC
+})📝 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.
| .filter(f => { | |
| const av1 = hasAv1 && supportsAv1 && f.type.includes('av01'); | |
| if (av1) return true; | |
| const vp9 = !hasAv1 && f.type.includes('vp9'); | |
| if (vp9) return true; | |
| const avc = !hasVp9 && f.type.includes('avc1'); | |
| if (avc) return true; | |
| }) | |
| .map(f => ([f.resolution, f.url])); | |
| media.video = data.videoStreams.filter(f => { | |
| if (supportsAv1 && f.type.includes('av01')) return true; // prefer AV1 | |
| if (f.type.includes('vp9')) return true; // else VP9 | |
| return f.type.includes('avc1'); // fallback AVC | |
| }) | |
| .map(f => ([f.resolution, f.url])); |
🤖 Prompt for AI Agents
In src/components/WatchVideo.ts around lines 52 to 60, the filter logic
incorrectly excludes all streams when AV1 streams exist but are unsupported by
the browser, due to conditions that skip VP9 and AVC checks if AV1 is present.
Refactor the filter to independently check each codec without mutually exclusive
conditions, returning true as soon as a matching supported codec is found,
ensuring that unsupported AV1 streams do not cause all streams to be dropped.
n-ce
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/scripts/router.ts (1)
125-127: Consider using optional chaining for null safetyThe dynamic dialog element retrieval is a good improvement that aligns with the modal component refactoring. However, you should use optional chaining to safely check if the element exists before accessing its properties.
- const actionsMenu = document.getElementById('actionsMenu') as HTMLDialogElement | null; - if (actionsMenu && actionsMenu.open) { - actionsMenu.click(); + const actionsMenu = document.getElementById('actionsMenu') as HTMLDialogElement | null; + if (actionsMenu?.open) { + actionsMenu.click();🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/utils.ts (1)
69-77: Prevent multiple snackbars piling upChanging the
clearpredicate toel.isConnectedno longer removes any previous snackbar. Twonotify()calls within 8 s now produce stacked toasts. Consider explicitly removing an existing snackbar before appending a new one:const el = $('p'); +// Ensure only one snackbar is visible at a time +document.querySelector('.snackbar')?.remove(); const clear = () => el.isConnected && el.remove();This preserves the new robustness while restoring the former single-toast behaviour.
src/components/CollectionSelector.ts (1)
24-43: Tighten typing & improve accessibility of the<select>
- The custom cast on
e.targetworks but bypasses the DOM typings. A safer, self-documenting alternative:const select = e.target as HTMLSelectElement; const { value } = select; ... select.selectedIndex = 0;
The control has no accessible label. Adding
aria-label="${i18n('collection_selector_add_to')}"(or linking a<label for="collectionSelector">) will help screen-reader users.Hard-coding
tabindex="2"risks conflicting when some list items are conditionally removed. Consider relying on natural tab order or calculating the index dynamically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
index.html(0 hunks)src/components/ActionsMenu.ts(1 hunks)src/components/ActionsMenu.tsx(0 hunks)src/components/CollectionSelector.ts(1 hunks)src/components/CollectionSelector.tsx(0 hunks)src/components/Lyrics.ts(1 hunks)src/components/Selector.ts(1 hunks)src/components/UpdatePrompt.ts(1 hunks)src/components/WatchVideo.ts(1 hunks)src/lib/dom.ts(0 hunks)src/lib/libraryUtils.ts(1 hunks)src/lib/player.ts(1 hunks)src/lib/store.ts(2 hunks)src/lib/utils.ts(3 hunks)src/main.ts(2 hunks)src/modules/setMetadata.ts(2 hunks)src/modules/start.ts(1 hunks)src/scripts/list.ts(3 hunks)src/scripts/router.ts(2 hunks)
💤 Files with no reviewable changes (4)
- index.html
- src/lib/dom.ts
- src/components/CollectionSelector.tsx
- src/components/ActionsMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main.ts
- src/modules/start.ts
- src/components/UpdatePrompt.ts
- src/lib/player.ts
- src/components/WatchVideo.ts
- src/components/Lyrics.ts
- src/components/Selector.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/lib/libraryUtils.ts (2)
src/lib/store.ts (1)
store(5-100)src/lib/utils.ts (1)
notify(69-77)
src/components/CollectionSelector.ts (3)
src/lib/libraryUtils.ts (4)
getDB(7-7)reservedCollections(5-5)createCollection(74-83)addToCollection(46-57)src/lib/store.ts (1)
store(5-100)src/lib/utils.ts (1)
i18n(40-45)
src/scripts/list.ts (1)
src/lib/store.ts (1)
store(5-100)
🪛 Biome (1.9.4)
src/scripts/router.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/lib/store.ts (1)
36-36: Good refactoring to centralize collection options state!Moving the collection options from DOM elements to a centralized array in the store is a good architectural improvement. This change properly separates state management from DOM manipulation, making the code more maintainable and easier to test.
Also applies to: 98-98
src/scripts/router.ts (1)
1-1: LGTM: Improved import statementRemoving the direct actionsMenu import aligns with the modal refactoring to use dynamic DOM queries instead of static imports.
src/scripts/list.ts (1)
34-34: LGTM: Good refactoring from DOM to state managementThe use of the store array for managing collection options is consistent with the refactoring goals. Using indexOf to find the collection and updating the array at the found index for renaming are good practices.
Also applies to: 79-79
src/lib/libraryUtils.ts (1)
78-79: LGTM: Well-implemented state management refactoringThe changes to use
store.addToCollectionOptionsinstead of DOM queries for both checking existence and adding new collections are well-implemented. This properly centralizes the collection options state and moves away from direct DOM manipulation, making the code more maintainable.Also applies to: 82-82
src/components/ActionsMenu.ts (1)
120-137: Verify dialog opening strategy for the “Watch on …” flowYou pre-set
dialog.open = truebut rely on the lazily-loadedWatchVideomodule to finalise rendering. If that module internally callsshowModal()anInvalidStateErrorwill be thrown because the dialog is already open.Please double-check
WatchVideo.default(); if it callsshowModal(), remove theopen = trueline and let the module decide. Otherwise, preferdialog.showModal()here for consistency with the other dialog invocations.
| more = function() { | ||
| store.actionsMenu = data; | ||
| actionsMenu.showModal(); | ||
| history.pushState({}, '', '#'); | ||
| const dialog = document.createElement('dialog') as HTMLDialogElement; | ||
| document.body.appendChild(dialog); | ||
| import('../components/ActionsMenu.ts') | ||
| .then(mod => mod.default(dialog)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make sure the temporary <dialog> node is always cleaned-up (Esc key, close() call, etc.)
ActionsMenu removes the node via its own close() helper, but if the user dismisses the dialog with the browser default “Esc” shortcut the element stays in the DOM, leaking one detached node per open.
Add a cancel/close listener immediately after creating the element:
const dialog = document.createElement('dialog') as HTMLDialogElement;
+// Remove the element even when it is dismissed with <Esc> or via `dialog.close()`.
+dialog.addEventListener('cancel', () => dialog.remove());
+dialog.addEventListener('close', () => dialog.remove());
document.body.appendChild(dialog);This keeps the DOM tidy and avoids an ever-growing pile of hidden dialogs.
📝 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.
| more = function() { | |
| store.actionsMenu = data; | |
| actionsMenu.showModal(); | |
| history.pushState({}, '', '#'); | |
| const dialog = document.createElement('dialog') as HTMLDialogElement; | |
| document.body.appendChild(dialog); | |
| import('../components/ActionsMenu.ts') | |
| .then(mod => mod.default(dialog)); | |
| } | |
| more = function() { | |
| store.actionsMenu = data; | |
| const dialog = document.createElement('dialog') as HTMLDialogElement; | |
| // Remove the element even when it is dismissed with <Esc> or via `dialog.close()`. | |
| dialog.addEventListener('cancel', () => dialog.remove()); | |
| dialog.addEventListener('close', () => dialog.remove()); | |
| document.body.appendChild(dialog); | |
| import('../components/ActionsMenu.ts') | |
| .then(mod => mod.default(dialog)); | |
| } |
🤖 Prompt for AI Agents
In src/modules/setMetadata.ts around lines 51 to 57, the temporary <dialog>
element created is not always removed from the DOM if the user dismisses it with
the Esc key, causing a memory leak. To fix this, add an event listener for the
'cancel' or 'close' event on the dialog immediately after creating it, and in
the handler, remove the dialog element from the DOM to ensure it is always
cleaned up regardless of how it is closed.
| const dialog = $('dialog') as HTMLDialogElement; | ||
| document.body.appendChild(dialog); | ||
| import('../components/ActionsMenu.ts') | ||
| .then(mod => mod.default(dialog)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Attach a cancel/close handler to the ad-hoc dialog as well
superClick mirrors the logic in setMetadata, so the same leak can occur here. Right after creating the dialog:
const dialog = $('dialog') as HTMLDialogElement;
+dialog.addEventListener('cancel', () => dialog.remove());
+dialog.addEventListener('close', () => dialog.remove());
document.body.appendChild(dialog);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dialog = $('dialog') as HTMLDialogElement; | |
| document.body.appendChild(dialog); | |
| import('../components/ActionsMenu.ts') | |
| .then(mod => mod.default(dialog)); | |
| const dialog = $('dialog') as HTMLDialogElement; | |
| dialog.addEventListener('cancel', () => dialog.remove()); | |
| dialog.addEventListener('close', () => dialog.remove()); | |
| document.body.appendChild(dialog); | |
| import('../components/ActionsMenu.ts') | |
| .then(mod => mod.default(dialog)); |
🤖 Prompt for AI Agents
In src/lib/utils.ts around lines 210 to 214, the dialog created and appended to
the body lacks a cancel or close event handler, which can cause a memory leak
similar to the one in setMetadata. To fix this, add an event listener for the
dialog's cancel or close event immediately after appending it, and ensure the
handler properly cleans up or removes the dialog to prevent leaks.
| function close() { | ||
| dialog.close(); | ||
| history.back(); | ||
| dialog.remove(); | ||
| } | ||
|
|
||
| dialog.id = 'actionsMenu'; | ||
| dialog.onclick = close; | ||
| dialog.showModal(); | ||
| history.pushState({}, '', '#'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle cancel / close events to avoid orphaned dialogs & stale history
Pressing Esc closes the dialog implicitly but never calls close(), leaving the node in the DOM and the extra history.pushState() entry intact. Tie both lifecycle events together:
dialog.id = 'actionsMenu';
dialog.onclick = close;
+dialog.addEventListener('cancel', () => close());
+dialog.addEventListener('close', () => dialog.remove()); // fallback safety
dialog.showModal();
history.pushState({}, '', '#');📝 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 close() { | |
| dialog.close(); | |
| history.back(); | |
| dialog.remove(); | |
| } | |
| dialog.id = 'actionsMenu'; | |
| dialog.onclick = close; | |
| dialog.showModal(); | |
| history.pushState({}, '', '#'); | |
| function close() { | |
| dialog.close(); | |
| history.back(); | |
| dialog.remove(); | |
| } | |
| dialog.id = 'actionsMenu'; | |
| dialog.onclick = close; | |
| dialog.addEventListener('cancel', () => close()); | |
| dialog.addEventListener('close', () => dialog.remove()); // fallback safety | |
| dialog.showModal(); | |
| history.pushState({}, '', '#'); |
🤖 Prompt for AI Agents
In src/components/ActionsMenu.ts around lines 15 to 25, the dialog's implicit
close via Esc key does not trigger the close() function, causing the dialog
element to remain in the DOM and the history state to be stale. Fix this by
adding event listeners for the dialog's 'cancel' and 'close' events that call
the close() function, ensuring proper cleanup of the dialog element and history
state in all close scenarios.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores