Separate playlist management buttons#1076
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR refactors the playlist management buttons feature by splitting a single Changes
Sequence DiagramsequenceDiagram
participant User
participant Settings as Settings UI
participant Storage as Chrome Storage
participant ContentPage as Content Page
participant Embedded as Embedded Page
participant Feature as Playlist Feature
participant DOM as Playlist DOM
User->>Settings: Toggle "Remove video button"
Settings->>Storage: Save enable_playlist_remove_button
Storage->>ContentPage: Storage change event
ContentPage->>Embedded: Send playlistRemoveButtonChange message
Embedded->>Feature: disablePlaylistManagementButtons()
Embedded->>Feature: enablePlaylistManagementButtons()
Feature->>DOM: addButtonToPlaylistItems()<br/>(checks enable_playlist_remove_button<br/>and enable_playlist_reset_button)
DOM-->>Feature: Remove button inserted<br/>(if flag enabled)
Feature-->>User: UI updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
public/locales/ca-ES.json(0 hunks)public/locales/cs-CZ.json(0 hunks)public/locales/de-DE.json(0 hunks)public/locales/en-GB.json(0 hunks)public/locales/en-US.json(1 hunks)public/locales/en-US.json.d.ts(1 hunks)public/locales/es-ES.json(0 hunks)public/locales/fa-IR.json(0 hunks)public/locales/fr-FR.json(0 hunks)public/locales/he-IL.json(0 hunks)public/locales/hi-IN.json(0 hunks)public/locales/it-IT.json(1 hunks)public/locales/ja-JP.json(0 hunks)public/locales/ko-KR.json(0 hunks)public/locales/nl-NL.json(0 hunks)public/locales/pl-PL.json(0 hunks)public/locales/pt-BR.json(0 hunks)public/locales/ru-RU.json(0 hunks)public/locales/sv-SE.json(0 hunks)public/locales/tr-TR.json(0 hunks)public/locales/uk-UA.json(0 hunks)public/locales/vi-VN.json(0 hunks)public/locales/zh-CN.json(0 hunks)public/locales/zh-TW.json(0 hunks)src/components/Settings/Settings.tsx(1 hunks)src/features/playlistManagementButtons/ActionButton/index.css(1 hunks)src/features/playlistManagementButtons/index.ts(5 hunks)src/pages/content/index.ts(1 hunks)src/pages/embedded/index.ts(1 hunks)src/types/index.ts(2 hunks)src/utils/constants.ts(2 hunks)
💤 Files with no reviewable changes (21)
- public/locales/vi-VN.json
- public/locales/pl-PL.json
- public/locales/ko-KR.json
- public/locales/fr-FR.json
- public/locales/cs-CZ.json
- public/locales/es-ES.json
- public/locales/uk-UA.json
- public/locales/ru-RU.json
- public/locales/hi-IN.json
- public/locales/ja-JP.json
- public/locales/en-GB.json
- public/locales/he-IL.json
- public/locales/fa-IR.json
- public/locales/tr-TR.json
- public/locales/pt-BR.json
- public/locales/nl-NL.json
- public/locales/de-DE.json
- public/locales/sv-SE.json
- public/locales/zh-TW.json
- public/locales/zh-CN.json
- public/locales/ca-ES.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/pages/embedded/index.ts (1)
src/features/playlistManagementButtons/index.ts (2)
disablePlaylistManagementButtons(29-39)enablePlaylistManagementButtons(41-142)
src/components/Settings/Settings.tsx (3)
src/components/Settings/components/SettingSection.tsx (1)
SettingSection(9-24)src/components/Settings/components/SettingTitle.tsx (1)
SettingTitle(3-6)src/components/Settings/components/Setting.tsx (1)
Setting(42-74)
src/features/playlistManagementButtons/index.ts (3)
src/utils/utilities.ts (2)
waitForSpecificMessage(833-860)IsDarkMode(538-541)src/features/playlistManagementButtons/ActionButton/index.ts (1)
createActionButton(24-69)src/features/playlistLength/utils.ts (1)
getPlaylistId(178-181)
src/pages/content/index.ts (1)
src/utils/utilities.ts (1)
sendExtensionOnlyMessage(744-758)
🔇 Additional comments (6)
src/types/index.ts (1)
270-271: Types for new playlist flags and messages are consistent across the codebase
configurationnow exposesenable_playlist_remove_buttonandenable_playlist_reset_button, andExtensionSendOnlyMessageMappingsdefinesplaylistRemoveButtonChange/playlistResetButtonChangewith payloads that match their use insrc/pages/content/index.tsandsrc/pages/embedded/index.ts. No issues spotted here.Also applies to: 443-444
src/features/playlistManagementButtons/ActionButton/index.css (1)
14-20: Hover styling for action buttons looks goodThe added hover backgrounds (white in light mode, semi-transparent in dark mode) are consistent with the existing button sizing and icon color logic; nothing problematic here.
src/pages/embedded/index.ts (1)
937-941: Playlist management buttons refresh path is wired correctlyOn
playlistRemoveButtonChange/playlistResetButtonChangeyou fully tear down and re-enable playlist management buttons, lettingenablePlaylistManagementButtonsre-read the latest options and rebuild DOM/observers. This matches how the feature is structured and should keep the buttons in sync with the two toggles.src/utils/constants.ts (1)
86-87: Defaults and import schema for new playlist flags are aligned
defaultConfigurationandconfigurationImportSchemanow both exposeenable_playlist_remove_buttonandenable_playlist_reset_buttonas optional booleans with sane defaults. This keeps runtime config, validation, and types in sync.Also applies to: 195-196
public/locales/en-US.json (1)
312-319: New English strings for playlist management toggles are clear and consistentThe
enableRemoveVideoButtonandenableMarkAsUnwatchedButtonentries undersettings.sections.miscellaneous.featuresaccurately describe the new toggles and align with the playlist management behavior implemented elsewhere. No issues here.src/pages/content/index.ts (1)
424-433: Storage change handling for new playlist flags is correctly wiredWhen
enable_playlist_remove_buttonorenable_playlist_reset_buttonchanges, you emitplaylistRemoveButtonChange/playlistResetButtonChangewith the expected*Enabledbooleans. These match the mapped types inExtensionSendOnlyMessageMappingsand the handlers insrc/pages/embedded/index.ts, so the end‑to‑end flow from config → UI looks sound.
| "label": "Default to original audio track", | ||
| "title": "Always default to the original audio track" |
There was a problem hiding this comment.
Italian translation for defaultToOriginalAudioTrack has been replaced by English
The label and title for settings.sections.miscellaneous.features.defaultToOriginalAudioTrack are now English while the rest of the locale is Italian. This looks unintentional and will create an inconsistent UI in it-IT.
Recommend restoring an Italian translation (or reusing the previous one) instead of the English text.
🤖 Prompt for AI Agents
In public/locales/it-IT.json around lines 309-310, the "label" and "title" for
settings.sections.miscellaneous.features.defaultToOriginalAudioTrack are in
English and must be restored to Italian; replace "Default to original audio
track" and "Always default to the original audio track" with appropriate Italian
translations (e.g., "Usa di default la traccia audio originale" for the label
and "Imposta sempre come predefinita la traccia audio originale" for the title)
so the locale is consistent.
| <SettingSection title="Playlist management settings"> | ||
| <SettingTitle /> | ||
| <Setting | ||
| checked={settings.enable_playlist_remove_button?.toString() === "true"} | ||
| label={t("settings.sections.miscellaneous.features.enableRemoveVideoButton.label")} | ||
| onChange={setCheckboxOption("enable_playlist_remove_button")} | ||
| parentSetting={null} | ||
| title="Adds a button to remove videos from the playlist" | ||
| type="checkbox" | ||
| /> | ||
| <Setting | ||
| checked={settings.enable_playlist_reset_button?.toString() === "true"} | ||
| label={t("settings.sections.miscellaneous.features.enableMarkAsUnwatchedButton.label")} | ||
| onChange={setCheckboxOption("enable_playlist_reset_button")} | ||
| parentSetting={null} | ||
| title="Adds a button to mark videos as unwatched" | ||
| type="checkbox" | ||
| /> |
There was a problem hiding this comment.
New playlist management settings bypass i18n (hard‑coded English strings)
The new section and its tooltips are not localized:
- Section title is a raw string:
"Playlist management settings". - Setting titles are raw English strings even though matching
*.titlekeys already exist inen-US.json.
Every other section uses t(...) for both section title and setting title, so this is inconsistent and will show English text for non‑English locales.
You can align with the existing pattern by wiring titles through i18n, e.g.:
- <SettingSection title="Playlist management settings">
+ <SettingSection title={t("settings.sections.playlistManagementButtons.title")}>
<SettingTitle />
<Setting
checked={settings.enable_playlist_remove_button?.toString() === "true"}
label={t("settings.sections.miscellaneous.features.enableRemoveVideoButton.label")}
- onChange={setCheckboxOption("enable_playlist_remove_button")}
- parentSetting={null}
- title="Adds a button to remove videos from the playlist"
+ onChange={setCheckboxOption("enable_playlist_remove_button")}
+ parentSetting={null}
+ title={t("settings.sections.miscellaneous.features.enableRemoveVideoButton.title")}
type="checkbox"
/>
<Setting
checked={settings.enable_playlist_reset_button?.toString() === "true"}
label={t("settings.sections.miscellaneous.features.enableMarkAsUnwatchedButton.label")}
- onChange={setCheckboxOption("enable_playlist_reset_button")}
- parentSetting={null}
- title="Adds a button to mark videos as unwatched"
+ onChange={setCheckboxOption("enable_playlist_reset_button")}
+ parentSetting={null}
+ title={t("settings.sections.miscellaneous.features.enableMarkAsUnwatchedButton.title")}
type="checkbox"
/>
</SettingSection>You’d also need to add settings.sections.playlistManagementButtons.title (or similar) to the locales alongside en‑US.
🤖 Prompt for AI Agents
In src/components/Settings/Settings.tsx around lines 1143-1160 the section title
and Setting title props are hard-coded English strings; replace them with i18n
keys (e.g. t("settings.sections.playlistManagementButtons.title") for the
Section and
t("settings.sections.playlistManagementButtons.enableRemoveVideoButton.title") /
t("settings.sections.playlistManagementButtons.enableMarkAsUnwatchedButton.title")
for each Setting title) and keep labels as they are using t(...); additionally
add corresponding keys and translations to the locale files (e.g. en-US.json and
other locales) so the section and tooltips are localized.
There was a problem hiding this comment.
@dnicolson strings should be accessed with the i18nInstance.t function. Some strings are hard coded in this recent code.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if ( | ||
| (!enable_playlist_remove_button && !enable_playlist_reset_button) || | ||
| !document.querySelector("ytd-playlist-video-list-renderer #sort-filter-menu:not(:empty)") | ||
| ) { |
There was a problem hiding this comment.
Don't bail out before the playlist renders
This early return now depends on #sort-filter-menu already having content. On many playlist loads that menu stays empty for a moment, so we exit before registering listeners or observers. Result: the remove/reset buttons never appear unless the menu happened to be populated in time, regressing existing behavior.
Drop this synchronous DOM gate (or defer it until after the observer has a chance to run) so we still observe the playlist and inject the buttons once YouTube finishes rendering.
- if (
- (!enable_playlist_remove_button && !enable_playlist_reset_button) ||
- !document.querySelector("ytd-playlist-video-list-renderer #sort-filter-menu:not(:empty)")
- ) {
+ if (!enable_playlist_remove_button && !enable_playlist_reset_button) {
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.
| if ( | |
| (!enable_playlist_remove_button && !enable_playlist_reset_button) || | |
| !document.querySelector("ytd-playlist-video-list-renderer #sort-filter-menu:not(:empty)") | |
| ) { | |
| if (!enable_playlist_remove_button && !enable_playlist_reset_button) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/features/playlistManagementButtons/index.ts around lines 48 to 51, remove
the synchronous check that bails out when "#sort-filter-menu" is empty and
instead always register the playlist observers/listeners; move any querySelector
check for non-empty menu into the MutationObserver/observer callback (or defer
the check with a short timeout) so the code still observes the DOM and injects
the remove/reset buttons once YouTube finishes rendering the menu.
github-project-automation
Bot
moved this from Todo
to Done - Released
in YouTube Enhancer
Co-authored-by: Nathan <45531575+VampireChicken12@users.noreply.github.com>
|
🎉 This PR is included in version 1.32.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
This implements features requested in #951 (comment).
Summary by CodeRabbit
Release Notes
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.