Update and merge fork with latest upstream changes (webosbrew/main) - #146
Update and merge fork with latest upstream changes (webosbrew/main)#146anikettuli wants to merge 121 commits into
Conversation
Fix sponsorblock segment overlay
* Add option to display time in UI
Add an option to force max video quality playback.
…from preview -> watch
Fix video quality not upgraded in transition from preview -> watch
Fix `async` keyword leaking into bundle
Co-authored-by: reisxd <29177546+reisxd@users.noreply.github.com>
Fix language switching
Redo issue templates
Co-authored-by: fire332 <96039230+fire332@users.noreply.github.com>
…readme Overhaul `README.md` to align with the current tooling and to clarify setup instructions. Based on webosbrew#322: Overhaul `README.md`
# Conflicts: # .husky/pre-commit # CHANGELOG.md # README.md # assets/appinfo.json # package-lock.json # package.json # src/adblock.js # src/app_api/index.ts # src/block-webos-cast.ts # src/config.js # src/custom-event-target.ts # src/font-fix.css # src/hooks/fetch.ts # src/hooks/index.ts # src/lang-settings-fix.ts # src/screensaver-fix.ts # src/sponsorblock.js # src/thumbnail-quality.ts # src/ui.css # src/ui.js # src/userScript.ts # src/watch.css # src/watch.js # src/yt-fixes.css # webpack.config.js
📝 WalkthroughWalkthroughThis PR updates pnpm-based tooling and CI, replaces issue intake and documentation, adds typed player and runtime integrations, updates ad/content and thumbnail processing, and reorganizes UI and supporting runtime modules. ChangesProject automation and build
Documentation and issue intake
Typed runtime and player integrations
Content filtering and media processing
UI controls and supporting fixes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UserScript
participant ResolveCommandRegistry
participant PlayerManager
participant YTPlayer
participant VideoQuality
UserScript->>ResolveCommandRegistry: register runtime command hooks
UserScript->>PlayerManager: initialize player integration
PlayerManager->>YTPlayer: observe video and playback state
YTPlayer-->>PlayerManager: emit newVideo and playbackStart
PlayerManager->>VideoQuality: dispatch typed player events
VideoQuality->>YTPlayer: set playback quality to highres
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR syncs the fork with upstream and updates the build/tooling pipeline (pnpm, updated CI, TypeScript-oriented lint/type-check), while also introducing several new runtime hooks/features (player manager API, thumbnail/quality upgrades, JSON hooks, endscreen/account-selector changes) and refreshing docs/metadata.
Changes:
- Migrates and standardizes tooling: pnpm-based workflows, updated ESLint/TS configs, and webpack build/version injection changes.
- Adds new runtime modules and hooks: player API manager, forced playback quality notifications, thumbnail URL upgrading, JSON.parse / JSON.stringify hooks, and account selector / endscreen behaviors.
- Updates packaging/docs/repo hygiene: appinfo updates, README overhaul, GitHub issue templates/dependabot, and CI setup action.
Reviewed changes
Copilot reviewed 47 out of 65 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Build config updates; appinfo/version injection; define plugin. |
| tsconfig.tooling.json | Tooling TS config tweaks (node types, libcheck). |
| tsconfig.base.json | Base TS strictness/emit settings adjustments. |
| tools/sync-version.cjs | Removes legacy appinfo version sync script. |
| tools/gen-manifest.cjs | Manifest generation now uses package.json version. |
| tools/deploy.js | Deploy uses package.json version for IPK name. |
| src/yt-fixes.js | Formatting/structure retained for YT fixes logic. |
| src/yt-fixes.css | CSS formatting/cleanup for fix styles. |
| src/webos-utils.js | Formatting-only changes to webOS version detection. |
| src/watch.js | Formatting-only changes to clock overlay logic. |
| src/watch.css | Formatting-only changes to watch overlay styling. |
| src/video-quality.ts | New: force high-res playback quality + notification flow. |
| src/userScript.ts | Entry script updated; adds new modules/hooks imports. |
| src/ui.css | UI styling file updated (currently contains conflict marker). |
| src/thumbnail-quality.ts | New: thumbnail URL upgrading with WebP detection. |
| src/remove-endscreen.ts | New: JSON.parse hook to strip endscreen data. |
| src/player_api/yt-api.ts | New: typed YT player surface definitions. |
| src/player_api/manager.ts | New: PlayerManager event wrapper over YT player. |
| src/player_api/index.ts | New: re-exports for player_api module. |
| src/player_api/helpers.ts | New: async element acquisition helpers for player. |
| src/lang-settings-fix.ts | Formatting-only changes to language cookie hook. |
| src/hooks/json-stringify.ts | New: JSON.stringify hook to mutate playback context. |
| src/hooks/index.ts | Formatting-only change to hooks export. |
| src/hooks/fetch.ts | Fetch hook updated; registry now auto-initializes. |
| src/globals.ts | New: global typings for launch params/debug flags. |
| src/custom-event-target.ts | Extends typed event helpers and event target typing. |
| src/config.js | Adds audio_only shortcut label; config list unchanged otherwise. |
| src/block-webos-cast.ts | Minor change in fetch hook handler destructuring. |
| src/auto-account-select.ts | New: resolveCommand hook to bypass account selector. |
| src/app_api/index.ts | Formatting-only changes to resolve command registry. |
| src/adblock.js | Minor formatting changes; behavior unchanged in excerpt. |
| README.md | Major documentation refresh (features, setup, commands). |
| pnpm-workspace.yaml | New pnpm workspace/build-allowlist config (currently placeholder). |
| package.json | Switches scripts/tooling to pnpm; deps/devDeps refreshed. |
| lint-staged.config.js | Adds eslint concurrency flag for staged linting. |
| eslint.config.ts | Flat-config updates; adds new lint rules and config refactors. |
| dist/appinfo.json | App metadata updated (icons/images/etc). |
| babel.config.js | Babel config refactor; adds preset-typescript and polyfill excludes. |
| assets/appinfo.json | App metadata updated (icons/images/etc). |
| .prettierignore | Updates ignore list (adds pnpm lock). |
| .husky/pre-commit | Adds lint-staged pre-commit hook command. |
| .github/workflows/release.yml | Uses composite setup action; switches to pnpm commands. |
| .github/workflows/main.yml | Uses composite setup; runs build/lint/test/package (continue-on-error added). |
| .github/release.yml | Adds release note categorization config. |
| .github/ISSUE_TEMPLATE/config.yml | New issue template config / contact links. |
| .github/ISSUE_TEMPLATE/bug.md | Removes legacy markdown bug template. |
| .github/ISSUE_TEMPLATE/3-other.yml | New: “Other Issue” form template. |
| .github/ISSUE_TEMPLATE/2-feature-req.yml | New: feature request form template. |
| .github/ISSUE_TEMPLATE/1-bug.yml | New: structured bug report form template. |
| .github/dependabot.yml | Adds dependabot config for actions/npm. |
| .github/actions/setup-env/action.yaml | New composite action to install pnpm/node deps. |
| .escheckrc | New: es-check config for dist JS compatibility checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transform: translateX(1.85vh); | ||
| } | ||
|
|
||
| <<<<<<< HEAD |
| allowBuilds: | ||
| '@webos-tools/cli@https://codeload.github.com/fire332/webos-cli/tar.gz/6cfbcf231a0fa1e50519d370ee6e5edc71c4998a': set this to true or false | ||
| core-js-pure: set this to true or false | ||
| cpu-features: set this to true or false | ||
| ssh2: set this to true or false | ||
| onlyBuiltDependencies: | ||
| - '@webos-tools/cli' | ||
| - core-js-pure | ||
| - cpu-features | ||
| - ssh2 |
| if (!configRead('removeEndscreen')) { | ||
| return res; | ||
| } |
| if (!configRead('autoAccountSelect')) { | ||
| resolveCommand(payload, extra); | ||
| return; | ||
| } |
| FetchRegistry.getInstance().addEventListener('request', (evt) => { | ||
| const { url, resource, init } = evt.detail; | ||
| if (url.pathname === '/wake_cast_core') evt.preventDefault(); | ||
| }); |
| #handlePlayerStateChange = () => { | ||
| const current = this.#player.getPlayerStateObject(); | ||
| const diff = diffPlayerState( | ||
| this.#lastPlayerState, | ||
| this.#player.getPlayerStateObject() | ||
| ); | ||
| this.#lastPlayerState = current; |
| instance.addEventListener('playbackStart', function (event) { | ||
| event.type; | ||
| event.currentTarget?.currentVideoID; | ||
| event.detail; | ||
| }); |
| build: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| continue-on-error: true |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (7)
src/app_api/index.ts (1)
112-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the initialization promise to prevent concurrent polling loops.
If
getInstance()is called multiple times before the hook target is found, it will spawn multiple concurrent polling loops becausegetHookTarget()returns a new promise each time.Cache the promise so that concurrent callers await the same initialization process.
⚡ Proposed refactor
+ static `#instancePromise`: Promise<ResolveCommandRegistry> | null = null; + static async getInstance() { if (registry) return registry; - const key = await this.getHookTarget(); - - registry = registry ?? new ResolveCommandRegistry(key); - return registry; + if (!this.#instancePromise) { + this.#instancePromise = this.getHookTarget().then((key) => { + registry = registry ?? new ResolveCommandRegistry(key); + return registry; + }); + } + return this.#instancePromise; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app_api/index.ts` around lines 112 - 119, Update the static getInstance() initialization flow to cache the in-progress promise, ensuring concurrent callers await the same getHookTarget() and ResolveCommandRegistry initialization rather than starting separate polling loops. Reuse the cached initialization promise until registry is assigned, then return the shared registry as before.src/video-quality.ts (2)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary event listener cleanup.
notifyPlaybackQualityis only ever invoked directly (via.call(this)) and is never registered as an event listener forplaybackStart. This line is dead code and can be removed to avoid confusion.♻️ Proposed fix
- this.removeEventListener('playbackStart', notifyPlaybackQuality);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/video-quality.ts` at line 28, Remove the unnecessary removeEventListener cleanup targeting playbackStart and notifyPlaybackQuality; retain the direct notifyPlaybackQuality invocation and surrounding playback-quality logic unchanged.
51-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClear timers before invoking the callback.
It is a defensive best practice to call
clearIntervalandclearTimeoutbefore executing external logic. IfnotifyPlaybackQualitywere to unexpectedly throw an error, executing it first could interrupt the block and leave the interval running, leading to an infinite polling leak.♻️ Proposed fix
- notifyPlaybackQuality.call(this); clearInterval(intervalToken); clearTimeout(timeoutToken); + notifyPlaybackQuality.call(this); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/video-quality.ts` around lines 51 - 54, In the completion block of the playback-quality polling flow, move clearInterval(intervalToken) and clearTimeout(timeoutToken) before notifyPlaybackQuality.call(this). Keep the callback invocation and timer cleanup behavior otherwise unchanged so timers are always cleared before external logic runs.src/player_api/helpers.ts (1)
13-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude descriptive error messages for type mismatches.
Throwing an empty
Errormakes it difficult to diagnose runtime issues if the matched element fails the constructor assertion. Consider including the CSS selector and expected type name in the error message.💡 Proposed refactor
const alreadyPresent = document.querySelector(cssSelectors); if (alreadyPresent) { - if (!(alreadyPresent instanceof expected)) throw new Error(); + if (!(alreadyPresent instanceof expected)) throw new Error(`Element matching '${cssSelectors}' failed constructor check.`); // Cast required due to narrowing limitations. // https://github.com/microsoft/TypeScript/issues/55241 return alreadyPresent as InstanceType<E>; } const result = await waitForChildAdd( document.body, (node: Node): node is Element => node instanceof Element && node.matches(cssSelectors), true ); - if (!(result instanceof expected)) throw new Error(); + if (!(result instanceof expected)) throw new Error(`Element matching '${cssSelectors}' failed constructor check.`); return result as InstanceType<E>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/player_api/helpers.ts` around lines 13 - 29, Update both constructor-mismatch checks in the helper to throw descriptive errors that include the CSS selector and the expected constructor/type name; preserve the existing successful return and casting behavior.src/custom-event-target.ts (1)
60-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
dispatchEvent's parameter type doesn't enforce thetypediscriminant likeaddEventListenerdoes.
EventListenerintersects its callback event withTypedEvent<Self, EventName>, ensuringevt.typematches the registered key.EventMapValue<T, K>(used bydispatchEvent) has no equivalent constraint, sodispatchEventdoesn't type-check that the passed event'stypeactually matchesK. This weakens the type-safety the rest of the file is designed to provide.♻️ Suggested fix
-type EventMapValue< - T extends EmptyEventMap, - K extends keyof T & string -> = T[K] extends Event ? T[K] : never; +type EventMapValue< + Self extends EventTarget, + T extends EmptyEventMap, + K extends keyof T & string +> = T[K] extends Event ? T[K] & TypedEvent<Self, K> : never;(Adjust the
CustomEventTarget.dispatchEventsignature to passthisthrough asSelf.)Also applies to: 103-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom-event-target.ts` around lines 60 - 63, Update EventMapValue and the CustomEventTarget.dispatchEvent signature so the event type is constrained with TypedEvent<Self, K>, passing the target instance type through as Self. Preserve the existing EventMapValue key lookup and ensure dispatchEvent enforces that the event.type matches its K parameter, consistent with addEventListener.src/hooks/json-stringify.ts (1)
23-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap object mutation in a
try...catchblock.Because TypeScript modules run in strict mode, attempting to mutate an object that happens to be frozen or non-extensible will throw a
TypeError. Since this function intercepts everyJSON.stringifycall in the application, an unexpected exception here could crash unrelated parts of YouTube.Wrapping the mutation in a
try...catchblock ensures that the application remains stable even if YouTube's state objects become immutable in the future.🛡️ Proposed fix to prevent potential crashes
if (!isPrimitive(ctx)) { - (ctx as Record<string, unknown>).isInlinePlaybackNoAd = true; - console.info(`[JSON.stringify] Set isInlinePlaybackNoAd`); + try { + (ctx as Record<string, unknown>).isInlinePlaybackNoAd = true; + console.info(`[JSON.stringify] Set isInlinePlaybackNoAd`); + } catch (err) { + console.warn(`[JSON.stringify] Failed to set isInlinePlaybackNoAd`, err); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/json-stringify.ts` around lines 23 - 26, Wrap the isInlinePlaybackNoAd assignment in the non-primitive branch of the JSON.stringify hook with try...catch, so mutation failures such as frozen or non-extensible objects are contained and do not propagate from the stringify interception. Preserve the existing assignment and logging behavior when mutation succeeds.src/block-webos-cast.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused variables from destructuring.
The variables
resourceandinitare destructured but never used. Consider removing them to reduce clutter and prevent potential linting or TypeScript warnings.🧹 Proposed fix
- const { url, resource, init } = evt.detail; + const { url } = evt.detail; if (url.pathname === '/wake_cast_core') evt.preventDefault();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/block-webos-cast.ts` around lines 11 - 13, Update the event detail destructuring in the wake-cast handler to extract only url, removing the unused resource and init variables while preserving the existing preventDefault behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/main.yml:
- Around line 13-14: Remove the job-level continue-on-error setting from the CI
build job in the workflow, leaving the build, linting, and test steps to fail
the workflow normally when they fail.
- Line 16: Update the actions/checkout@v4 step to disable credential persistence
by configuring its persist-credentials option to false.
In `@CHANGELOG.md`:
- Around line 1606-1613: Update the changelog footer’s Unreleased reference to
use v0.8.0 as its comparison base and ensure an Unreleased heading is present;
alternatively, remove the unused Unreleased definition. Keep the existing
release comparison entries unchanged.
In `@pnpm-workspace.yaml`:
- Around line 2-5: Replace the placeholder values in the allowBuilds
configuration for `@webos-tools/cli`, core-js-pure, cpu-features, and ssh2 with
explicit boolean true or false values according to the intended build approval
policy.
In `@README.md`:
- Around line 90-94: Remove the duplicate installation block near the webOS CLI
instructions, retain the existing installation section, and update its CLI
guidance link from the obsolete `#development-tv-setup` anchor to
`#development-setup`.
In `@src/app_api/index.ts`:
- Around line 99-110: Increase the retry delay in the findHookTarget polling
loop from zero to a reasonable interval, such as 50–100 milliseconds, before
invoking poll again. Preserve the existing immediate resolution when a hook is
found.
In `@src/auto-account-select.ts`:
- Around line 7-27: Update the hook function to return the result of
resolveCommand in the autoAccountSelect-disabled branch and return the result of
registry.dispatchCommand in the dispatch path, preserving the upstream command
contract while leaving the existing command payload unchanged.
In `@src/config.js`:
- Around line 55-90: Add removeEndscreen and autoAccountSelect entries to the
configOptions map with their intended runtime defaults and descriptive labels,
so configRead() recognizes the keys consumed by the removeEndscreen and
autoAccountSelect hooks. Keep the existing configuration entries unchanged.
In `@src/player_api/manager.ts`:
- Around line 64-69: Update the video-change handling around currentVideoID and
`#lastVideoID` to treat a null or empty ID as a player unload: update the cached
`#lastVideoID` state without calling `#handleNewVideo`, and remove the error throw.
Continue dispatching `#handleNewVideo` only when currentVideoID contains a valid
video ID.
- Around line 110-114: Remove the no-op playbackStart event listener from
getPlayerManager, including its unused event-property reads. Do not replace it
with another listener; preserve the remaining player manager initialization
behavior.
In `@src/sponsorblock.js`:
- Around line 1244-1250: Update the module re-entry cleanup around
window.sponsorblock to call its destroy() method before assigning
window.sponsorblock = null. Preserve the existing hashchange listener removal,
and only clear the global after the previous sponsorblock instance has been
destroyed.
In `@src/thumbnail-quality.ts`:
- Around line 129-137: Update the mutation-processing chain around upgradeBgImg
to include each added HTMLElement itself alongside the elements returned by
querySelectorAll, so directly inserted styled YT_THUMBNAIL_ELEMENT_TAG nodes are
upgraded while descendant scanning remains unchanged.
In `@src/ui.css`:
- Around line 297-298: Remove the unresolved `<<<<<<< HEAD` merge-conflict
marker immediately before the `@media (max-width: 768px)` rule, preserving the
valid responsive CSS declaration.
In `@src/ui.js`:
- Around line 1991-2020: Update applyUIFixes and its bodyClassCallback so they
no longer remove app-quality-root from document.body. Preserve the class
required by the injected UI rules, while retaining the existing observer setup
only if it has another necessary purpose.
In `@src/watch.js`:
- Around line 8-10: Update the watch destruction/disable path, using the
existing _timer and destroy-related methods, to cancel any pending
minute-alignment timeout before cleanup completes. Clear the timeout and reset
_timer so its callback cannot create an interval after destroy().
In `@src/yt-fixes.js`:
- Around line 80-108: Update the delayed callback in attemptSearchHistoryFix to
clear suggestionsBox.dataset.historyCheckPending when the box disconnects or
population does not succeed, including thrown errors or a false result while the
box remains empty. Preserve the successful historyCache/historyFixed behavior,
allowing subsequent calls to retry when the initial attempt fails.
In `@webpack.config.js`:
- Around line 188-194: Update the runtime.version value in
TransformAsyncModulesPlugin to use the resolved installed `@babel/runtime-corejs3`
version rather than pkgJson.dependencies, while leaving the existing
absoluteRuntime setting unchanged.
---
Nitpick comments:
In `@src/app_api/index.ts`:
- Around line 112-119: Update the static getInstance() initialization flow to
cache the in-progress promise, ensuring concurrent callers await the same
getHookTarget() and ResolveCommandRegistry initialization rather than starting
separate polling loops. Reuse the cached initialization promise until registry
is assigned, then return the shared registry as before.
In `@src/block-webos-cast.ts`:
- Around line 11-13: Update the event detail destructuring in the wake-cast
handler to extract only url, removing the unused resource and init variables
while preserving the existing preventDefault behavior.
In `@src/custom-event-target.ts`:
- Around line 60-63: Update EventMapValue and the
CustomEventTarget.dispatchEvent signature so the event type is constrained with
TypedEvent<Self, K>, passing the target instance type through as Self. Preserve
the existing EventMapValue key lookup and ensure dispatchEvent enforces that the
event.type matches its K parameter, consistent with addEventListener.
In `@src/hooks/json-stringify.ts`:
- Around line 23-26: Wrap the isInlinePlaybackNoAd assignment in the
non-primitive branch of the JSON.stringify hook with try...catch, so mutation
failures such as frozen or non-extensible objects are contained and do not
propagate from the stringify interception. Preserve the existing assignment and
logging behavior when mutation succeeds.
In `@src/player_api/helpers.ts`:
- Around line 13-29: Update both constructor-mismatch checks in the helper to
throw descriptive errors that include the CSS selector and the expected
constructor/type name; preserve the existing successful return and casting
behavior.
In `@src/video-quality.ts`:
- Line 28: Remove the unnecessary removeEventListener cleanup targeting
playbackStart and notifyPlaybackQuality; retain the direct notifyPlaybackQuality
invocation and surrounding playback-quality logic unchanged.
- Around line 51-54: In the completion block of the playback-quality polling
flow, move clearInterval(intervalToken) and clearTimeout(timeoutToken) before
notifyPlaybackQuality.call(this). Keep the callback invocation and timer cleanup
behavior otherwise unchanged so timers are always cleared before external logic
runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f55fab9d-f543-4910-9ad4-e52f389c37b3
⛔ Files ignored due to path filters (11)
assets/bgImage.pngis excluded by!**/*.pngassets/extraLargeIcon.pngis excluded by!**/*.pngassets/imageForRecents.pngis excluded by!**/*.pngassets/mediumLargeIcon.pngis excluded by!**/*.pngassets/playIcon.pngis excluded by!**/*.pngassets/splashBackground-v1.pngis excluded by!**/*.pngdist/appinfo.jsonis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/webOSUserScripts/userScript.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
.escheckrc.github/ISSUE_TEMPLATE/1-bug.yml.github/ISSUE_TEMPLATE/2-feature-req.yml.github/ISSUE_TEMPLATE/3-other.yml.github/ISSUE_TEMPLATE/bug.md.github/ISSUE_TEMPLATE/config.yml.github/actions/setup-env/action.yaml.github/dependabot.yml.github/release.yml.github/workflows/main.yml.github/workflows/release.yml.husky/pre-commit.prettierignoreCHANGELOG.mdREADME.mdassets/appinfo.jsonbabel.config.jseslint.config.tslint-staged.config.jspackage.jsonpnpm-workspace.yamlsrc/adblock.jssrc/app_api/index.tssrc/auto-account-select.tssrc/block-webos-cast.tssrc/config.jssrc/custom-event-target.tssrc/globals.tssrc/hooks/fetch.tssrc/hooks/index.tssrc/hooks/json-stringify.tssrc/lang-settings-fix.tssrc/player_api/helpers.tssrc/player_api/index.tssrc/player_api/manager.tssrc/player_api/yt-api.tssrc/remove-endscreen.tssrc/sponsorblock.jssrc/thumbnail-quality.tssrc/ui.csssrc/ui.jssrc/userScript.tssrc/video-quality.tssrc/watch.csssrc/watch.jssrc/webos-utils.jssrc/yt-fixes.csssrc/yt-fixes.jstools/deploy.jstools/gen-manifest.cjstools/sync-version.cjstsconfig.base.jsontsconfig.tooling.jsonwebpack.config.js
💤 Files with no reviewable changes (2)
- .github/ISSUE_TEMPLATE/bug.md
- tools/sync-version.cjs
| '@webos-tools/cli@https://codeload.github.com/fire332/webos-cli/tar.gz/6cfbcf231a0fa1e50519d370ee6e5edc71c4998a': set this to true or false | ||
| core-js-pure: set this to true or false | ||
| cpu-features: set this to true or false | ||
| ssh2: set this to true or false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
pnpm allowBuilds placeholder "set this to true or false" behavior if left unset
💡 Result:
In pnpm, the allowBuilds setting is a configuration map introduced in version 10.26.0 that allows you to explicitly define which packages are permitted to run build scripts [1][2][3]. If a package is not listed in your allowBuilds configuration (i.e., the setting is left unset for that specific package), the default behavior is that the package is disallowed from executing build scripts [4][2]. When this occurs, pnpm will typically print a warning to the console during installation [4][2]. If the strictDepBuilds configuration is enabled (set to true), pnpm will treat this as an error instead of a warning, preventing the installation from proceeding [2]. In summary: - true: The package is explicitly permitted to execute its build/postinstall scripts [4][5][2]. - false: The package is explicitly denied from executing its build/postinstall scripts [4][5][2]. - Unset (not in the map): The package is disallowed by default, resulting in a warning (or an error if strictDepBuilds is true) [4][6][2]. This setting replaces older, deprecated configurations such as onlyBuiltDependencies and ignoredBuiltDependencies, serving as the modern, unified source of truth for managing build script execution [4][2].
Citations:
- 1: https://pnpm.io/settings
- 2: https://pnpm.io/10.x/settings
- 3: https://pnpm.io/blog/releases/10.26
- 4: pnpm/pnpm@cb367b9
- 5: https://github.com/pnpm/pnpm/blob/e1e29c15/building/policy/src/index.ts
- 6: https://github.com/pnpm/pnpm/blob/2b14c742/exec/build-modules/src/index.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- pnpm-workspace.yaml ---\n'
cat -n pnpm-workspace.yaml
printf '\n--- search for allowBuilds references ---\n'
rg -n "allowBuilds|approve-builds|strictDepBuilds|onlyBuiltDependencies|ignoredBuiltDependencies" -S . || trueRepository: NicholasBly/youtube-webos
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('pnpm-workspace.yaml')
text = p.read_text()
print('contains placeholder:', 'set this to true or false' in text)
print('allowBuilds entries:')
for i, line in enumerate(text.splitlines(), 1):
if 'allowBuilds' in line or 'set this to true or false' in line:
print(f'{i}: {line}')
PYRepository: NicholasBly/youtube-webos
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sys
from pathlib import Path
p = Path('pnpm-workspace.yaml')
if p.exists():
print(p.read_text())
else:
print("missing pnpm-workspace.yaml", file=sys.stderr)
PYRepository: NicholasBly/youtube-webos
Length of output: 526
Replace the placeholder allowBuilds entries in pnpm-workspace.yaml:2-5. set this to true or false is pnpm’s placeholder for pending build approvals, so these values still need to be set explicitly to true or false for each package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pnpm-workspace.yaml` around lines 2 - 5, Replace the placeholder values in
the allowBuilds configuration for `@webos-tools/cli`, core-js-pure, cpu-features,
and ssh2 with explicit boolean true or false values according to the intended
build approval policy.
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (11)
src/sponsorblock.js (1)
1252-1300: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
initSBtears down and recreates the handler on everyhashchange, even for the same video.There's no check against the currently active
window.sponsorblock.videoIDbefore destroying and recreating — anyhashchangeevent while still on the same video (e.g. other hash-carried params changing) forces a full teardown, a fresh network fetch to the SponsorBlock API, and overlay re-render.Suggested guard to skip redundant recreation
if (window.sponsorblock) { + if (window.sponsorblock.videoID === videoID && !window.sponsorblock.isDestroyed) { + return; + } window.sponsorblock.destroy(); window.sponsorblock = null; }Please confirm whether the webOS hash-router can fire
hashchangefor reasons other than a video change (e.g. time/param updates) — if so this guard avoids needless refetches against the public SponsorBlock APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sponsorblock.js` around lines 1252 - 1300, Update initSB to compare the parsed videoID with window.sponsorblock.videoID before destroying the existing handler; when the current handler already represents the same video, return without teardown or recreation. Preserve the existing cleanup behavior for navigation away from /watch or when the video ID is missing, and retain full reinitialization when the video changes.src/config.js (1)
125-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated localStorage-write logic between
flushPendingWriteandscheduleWrite.Both functions contain an identical try/catch block that stringifies and persists
localConfig. Extract a sharedpersist()helper to avoid the two copies silently diverging in the future (e.g., if error handling or serialization needs to change).♻️ Proposed refactor
+function persistConfig() { + try { window.localStorage.setItem(CONFIG_KEY, JSON.stringify(localConfig)); } + catch (e) { /* quota / SecurityError on private mode */ } +} + function flushPendingWrite() { if (pendingWriteTimer === null) return; clearTimeout(pendingWriteTimer); pendingWriteTimer = null; - try { window.localStorage[CONFIG_KEY] = JSON.stringify(localConfig); } - catch (e) { /* quota / SecurityError on private mode */ } + persistConfig(); } function scheduleWrite() { if (pendingWriteTimer !== null) return; pendingWriteTimer = setTimeout(() => { pendingWriteTimer = null; - try { window.localStorage[CONFIG_KEY] = JSON.stringify(localConfig); } - catch (e) { /* ignore */ } + persistConfig(); }, 200); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.js` around lines 125 - 140, Extract the duplicated localStorage serialization and write logic from flushPendingWrite and scheduleWrite into a shared persist helper, including the existing error handling. Replace both inline try/catch blocks with calls to persist while preserving the current timer behavior and ignored storage errors.src/ui.js (1)
1979-1989: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winObserver keeps running indefinitely even after audio-only mode is disabled.
Every call to
initAudioOnlyToggle()— including the one that turns the feature off — disconnects the old observer and immediately creates/attaches a new one oncontrolsContainer. Once the feature is toggled off, there's no need to keep observing every child mutation in the controls area for the rest of the session (the callback is effectively a no-op whenaudioOnlyEnabledisfalse).♻️ Proposed fix
if (overlayObserver) overlayObserver.disconnect(); - overlayObserver = new MutationObserver(() => { - updateOverlay(); - applyAudioOverlayFilter(); - }); - - overlayObserver.observe(controlsContainer, { - childList: true, - subtree: true - }); + if (audioOnlyEnabled) { + overlayObserver = new MutationObserver(() => { + updateOverlay(); + applyAudioOverlayFilter(); + }); + overlayObserver.observe(controlsContainer, { + childList: true, + subtree: true + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui.js` around lines 1979 - 1989, Update initAudioOnlyToggle so it disconnects the existing overlayObserver but only creates and attaches a new MutationObserver when audioOnlyEnabled is true; leave overlayObserver inactive when audio-only mode is disabled.src/player_api/helpers.ts (1)
32-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the Promise to prevent concurrent
MutationObserverallocations.If
getPlayer()is called multiple times before the player element is added to the DOM, it will trigger multiple concurrent calls torequireElement. This results in multipleMutationObserverinstances watchingdocument.bodyfor subtree changes simultaneously, which can cause significant overhead on resource-constrained WebOS devices during startup.Consider caching the Promise instead of the resolved value to ensure only one observer is created.
⚡ Proposed fix
-let player: YTPlayer | null = null; +let playerPromise: Promise<YTPlayer> | null = null; -export async function getPlayer(): Promise<YTPlayer> { - if (player) return player; - - player = (await requireElement( - '.html5-video-player', - HTMLElement - )) as YTPlayer; - - return player; +export function getPlayer(): Promise<YTPlayer> { + if (!playerPromise) { + playerPromise = requireElement( + '.html5-video-player', + HTMLElement + ) as Promise<YTPlayer>; + } + return playerPromise; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/player_api/helpers.ts` around lines 32 - 43, Update the player cache in getPlayer so it stores and reuses the in-flight Promise from requireElement, preventing concurrent calls from creating multiple MutationObserver instances. Return the cached Promise for subsequent calls, and preserve the resolved YTPlayer value once the lookup completes.src/globals.ts (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the standard DOM API signature for
addEventListeneroptions.The standard
Document.addEventListenersignature acceptsboolean | AddEventListenerOptionsfor its third parameter. Using onlybooleanrestricts the ability to pass options like{ once: true }or{ passive: true }if they are ever needed.♻️ Proposed refactor
interface Document { addEventListener( eventName: 'webOSRelaunch', listener: (evt: CustomEvent<webOSLaunchParams>) => void, - useCapture?: boolean + options?: boolean | AddEventListenerOptions ): void; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/globals.ts` around lines 9 - 15, Update the webOSRelaunch overload in the Document interface so its addEventListener third parameter accepts boolean or AddEventListenerOptions, matching the standard DOM API while preserving the existing event and listener types.src/video-quality.ts (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary
removeEventListener.
notifyPlaybackQualityis called directly as a function (notifyPlaybackQuality.call(this)) and is never attached as an event listener toplaybackStart. This line has no effect and can be removed.♻️ Proposed refactor
- this.removeEventListener('playbackStart', notifyPlaybackQuality);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/video-quality.ts` at line 28, Remove the unnecessary removeEventListener call for playbackStart from the surrounding video-quality logic, leaving the direct notifyPlaybackQuality.call(this) invocation unchanged.src/player_api/manager.ts (1)
51-57: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid redundant calls to
getPlayerStateObject().You can use the
currentvariable instead of callinggetPlayerStateObject()a second time.♻️ Proposed refactor
`#handlePlayerStateChange` = () => { const current = this.#player.getPlayerStateObject(); const diff = diffPlayerState( this.#lastPlayerState, - this.#player.getPlayerStateObject() + current ); this.#lastPlayerState = current;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/player_api/manager.ts` around lines 51 - 57, Update `#handlePlayerStateChange` to pass the already captured current value into diffPlayerState instead of calling `#player.getPlayerStateObject`() a second time, while preserving the existing state update behavior.pnpm-workspace.yaml (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant build-approval config: same packages listed under both
allowBuildsandonlyBuiltDependencies.
allowBuilds(the mechanismpnpm approve-buildswrites to, and the sole replacement foronlyBuiltDependenciesin pnpm v11+) and the legacyonlyBuiltDependencieslist both enumerate the identical four packages. On pnpm 10.33.0 (pinned viapackageManager) both are read, so this works, but the duplication risks drifting out of sync if either list is updated independently (e.g. viapnpm approve-builds) without updating the other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpm-workspace.yaml` around lines 1 - 10, Remove the redundant onlyBuiltDependencies block from pnpm-workspace.yaml and retain the four package approvals exclusively under allowBuilds. Ensure all currently approved packages remain listed in allowBuilds.src/lang-settings-fix.ts (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the trailing quote in the log message.
The string ends with an unnecessary
"character inside the backticks.💡 Proposed refactor
const passthrough = () => { console.warn( - `[lang-settings-fix] Passing through due to payload mismatch."` + `[lang-settings-fix] Passing through due to payload mismatch.` ); return resolveCommand(payload, extra); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lang-settings-fix.ts` around lines 20 - 22, Remove the extraneous trailing double-quote character from the template literal passed to console.warn in the lang-settings-fix warning log, leaving the rest of the message unchanged.src/app_api/index.ts (2)
99-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIncrease polling interval to reduce CPU load.
Using
setTimeout(poll, 0)causes a tight loop that aggressively consumes CPU resources while waiting for the player hook to initialize, potentially degrading app startup performance on low-end TVs. A modest delay (e.g., 50ms) yields to the event loop efficiently without noticeably delaying hooking.♻️ Proposed refactor
return new Promise((resolve) => { const poll = () => { hook = this.findHookTarget(); if (hook) { resolve(hook); } else { - setTimeout(poll, 0); + setTimeout(poll, 50); } }; poll(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app_api/index.ts` around lines 99 - 110, Update the polling loop in the hook-waiting Promise to replace the zero-delay setTimeout call with a modest interval such as 50ms. Keep the existing findHookTarget check and resolve behavior unchanged.
112-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the initialization Promise to prevent multiple parallel polling loops.
Because multiple modules (
lang-settings-fix.ts,auto-account-select.ts) callgetInstance()concurrently at startup,registryremainsnullwhilegetHookTargetawaits. This causesgetHookTarget()to be invoked concurrently, triggering redundant, parallel polling loops. Cache the Promise to properly orchestrate the singleton initialization.♻️ Proposed refactor to synchronize singleton instantiation
+ static `#instancePromise`: Promise<ResolveCommandRegistry> | null = null; + static async getInstance() { if (registry) return registry; + if (this.#instancePromise) return this.#instancePromise; - const key = await this.getHookTarget(); - - registry = registry ?? new ResolveCommandRegistry(key); - return registry; + this.#instancePromise = (async () => { + const key = await this.getHookTarget(); + registry = registry ?? new ResolveCommandRegistry(key); + return registry; + })(); + return this.#instancePromise; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app_api/index.ts` around lines 112 - 119, Update ResolveCommandRegistry.getInstance to cache the in-flight initialization Promise before awaiting getHookTarget, so concurrent callers share one initialization and polling loop. Return the existing registry when initialized, reuse the cached Promise while initialization is pending, and assign the resolved ResolveCommandRegistry instance to registry once complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/main.yml:
- Around line 13-14: Remove the job-level continue-on-error setting from the
main workflow so build, lint, and test failures propagate and cause CI to fail
normally.
In `@package.json`:
- Around line 23-26: Align the package metadata URLs in package.json: update the
repository field to use the same NicholasBly/youtube-webos fork referenced by
bugs and homepage, unless the upstream repository is intentionally canonical.
Keep the existing license and other metadata unchanged.
In `@src/app_api/index.ts`:
- Around line 72-80: Update the target validation assertion in the visible
type-checking function to safely handle null instances by using optional
chaining when accessing resolveCommand. Preserve the existing checks for target,
function type, instance presence, and resolveCommand being callable while
preventing property access on a null target.instance.
- Around line 29-47: Update resolveCommand to return this.#originalFn(command,
extra) immediately when command is null or not an object, before logging or
calling Object.keys; retain the existing command-key resolution for object
payloads.
In `@src/auto-account-select.ts`:
- Around line 7-27: Return the result of resolveCommand(payload, extra) in the
bypass branch of hook, and return the result of registry.dispatchCommand in the
active auto-account-selection branch. Preserve the existing arguments and
command payload while propagating both command outputs to callers.
In `@src/hooks/fetch.ts`:
- Around line 45-49: Update the Promise created around the FileReader in the
fetch flow to listen for both error and abort events in addition to load, and
settle the Promise when either failure event occurs so it cannot remain pending.
Preserve the existing load behavior and return type while ensuring debug-mode
fetches do not hang on FileReader failures.
In `@src/lang-settings-fix.ts`:
- Around line 40-42: Update the findIndex predicate in the settingDatas lookup
to optional-chain the setting element itself before accessing clientSettingEnum,
preserving the existing I18N_LANGUAGE match behavior while safely handling null
entries.
In `@src/player_api/manager.ts`:
- Around line 110-114: Remove the no-op playbackStart event listener
registration from getPlayerManager(), including the unused event property
accesses. Leave the actual player manager initialization and event handling
unchanged.
In `@src/remove-endscreen.ts`:
- Around line 1-8: Replace the external kindOf call in isObject with a native
object check that identifies JSON-parsed plain objects while excluding null and
non-object values, and remove the now-unused which-builtin-type import.
In `@src/thumbnail-quality.ts`:
- Around line 128-137: Update the childList mutation handling around
upgradeBgImg so each added HTMLElement is checked alongside its matching
descendants. Include the added node itself when it matches
YT_THUMBNAIL_ELEMENT_TAG, while preserving the existing backgroundImage filter
and upgrade behavior for nested thumbnail elements.
In `@src/ui.css`:
- Around line 297-298: Remove the stray `<<<<<<< HEAD` merge marker immediately
before the `@media (max-width: 768px)` rule in the stylesheet, leaving the media
query and its mobile styles intact.
In `@src/ui.js`:
- Around line 1936-1977: Add a CSS rule in src/ui.css for the
ytaf-ui-watchControl-overlayMessage class created by updateOverlay, defining
appropriate positioning, background, and typography so the audio-only message is
styled as an overlay over the controls.
In `@src/video-quality.ts`:
- Around line 45-62: Move the polling and timeout token variables to shared
scope for setPlaybackQuality, then clear any existing interval and timeout at
the start of that function before creating new timers. Reuse the shared tokens
in the quality-change and timeout callbacks, preserving the existing
notification and cleanup behavior while preventing overlapping polling runs.
---
Nitpick comments:
In `@pnpm-workspace.yaml`:
- Around line 1-10: Remove the redundant onlyBuiltDependencies block from
pnpm-workspace.yaml and retain the four package approvals exclusively under
allowBuilds. Ensure all currently approved packages remain listed in
allowBuilds.
In `@src/app_api/index.ts`:
- Around line 99-110: Update the polling loop in the hook-waiting Promise to
replace the zero-delay setTimeout call with a modest interval such as 50ms. Keep
the existing findHookTarget check and resolve behavior unchanged.
- Around line 112-119: Update ResolveCommandRegistry.getInstance to cache the
in-flight initialization Promise before awaiting getHookTarget, so concurrent
callers share one initialization and polling loop. Return the existing registry
when initialized, reuse the cached Promise while initialization is pending, and
assign the resolved ResolveCommandRegistry instance to registry once complete.
In `@src/config.js`:
- Around line 125-140: Extract the duplicated localStorage serialization and
write logic from flushPendingWrite and scheduleWrite into a shared persist
helper, including the existing error handling. Replace both inline try/catch
blocks with calls to persist while preserving the current timer behavior and
ignored storage errors.
In `@src/globals.ts`:
- Around line 9-15: Update the webOSRelaunch overload in the Document interface
so its addEventListener third parameter accepts boolean or
AddEventListenerOptions, matching the standard DOM API while preserving the
existing event and listener types.
In `@src/lang-settings-fix.ts`:
- Around line 20-22: Remove the extraneous trailing double-quote character from
the template literal passed to console.warn in the lang-settings-fix warning
log, leaving the rest of the message unchanged.
In `@src/player_api/helpers.ts`:
- Around line 32-43: Update the player cache in getPlayer so it stores and
reuses the in-flight Promise from requireElement, preventing concurrent calls
from creating multiple MutationObserver instances. Return the cached Promise for
subsequent calls, and preserve the resolved YTPlayer value once the lookup
completes.
In `@src/player_api/manager.ts`:
- Around line 51-57: Update `#handlePlayerStateChange` to pass the already
captured current value into diffPlayerState instead of calling
`#player.getPlayerStateObject`() a second time, while preserving the existing
state update behavior.
In `@src/sponsorblock.js`:
- Around line 1252-1300: Update initSB to compare the parsed videoID with
window.sponsorblock.videoID before destroying the existing handler; when the
current handler already represents the same video, return without teardown or
recreation. Preserve the existing cleanup behavior for navigation away from
/watch or when the video ID is missing, and retain full reinitialization when
the video changes.
In `@src/ui.js`:
- Around line 1979-1989: Update initAudioOnlyToggle so it disconnects the
existing overlayObserver but only creates and attaches a new MutationObserver
when audioOnlyEnabled is true; leave overlayObserver inactive when audio-only
mode is disabled.
In `@src/video-quality.ts`:
- Line 28: Remove the unnecessary removeEventListener call for playbackStart
from the surrounding video-quality logic, leaving the direct
notifyPlaybackQuality.call(this) invocation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a7ef6d6-9108-4037-b644-f3b134ae80c3
⛔ Files ignored due to path filters (11)
assets/bgImage.pngis excluded by!**/*.pngassets/extraLargeIcon.pngis excluded by!**/*.pngassets/imageForRecents.pngis excluded by!**/*.pngassets/mediumLargeIcon.pngis excluded by!**/*.pngassets/playIcon.pngis excluded by!**/*.pngassets/splashBackground-v1.pngis excluded by!**/*.pngdist/appinfo.jsonis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/webOSUserScripts/userScript.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
.escheckrc.github/ISSUE_TEMPLATE/1-bug.yml.github/ISSUE_TEMPLATE/2-feature-req.yml.github/ISSUE_TEMPLATE/3-other.yml.github/ISSUE_TEMPLATE/bug.md.github/ISSUE_TEMPLATE/config.yml.github/actions/setup-env/action.yaml.github/dependabot.yml.github/release.yml.github/workflows/main.yml.github/workflows/release.yml.husky/pre-commit.prettierignoreCHANGELOG.mdREADME.mdassets/appinfo.jsonbabel.config.jseslint.config.tslint-staged.config.jspackage.jsonpnpm-workspace.yamlsrc/adblock.jssrc/app_api/index.tssrc/auto-account-select.tssrc/block-webos-cast.tssrc/config.jssrc/custom-event-target.tssrc/globals.tssrc/hooks/fetch.tssrc/hooks/index.tssrc/hooks/json-stringify.tssrc/lang-settings-fix.tssrc/player_api/helpers.tssrc/player_api/index.tssrc/player_api/manager.tssrc/player_api/yt-api.tssrc/remove-endscreen.tssrc/sponsorblock.jssrc/thumbnail-quality.tssrc/ui.csssrc/ui.jssrc/userScript.tssrc/video-quality.tssrc/watch.csssrc/watch.jssrc/webos-utils.jssrc/yt-fixes.csssrc/yt-fixes.jstools/deploy.jstools/gen-manifest.cjstools/sync-version.cjstsconfig.base.jsontsconfig.tooling.jsonwebpack.config.js
💤 Files with no reviewable changes (2)
- .github/ISSUE_TEMPLATE/bug.md
- tools/sync-version.cjs
| import kindOf from 'which-builtin-type'; | ||
|
|
||
| import { configRead } from './config'; | ||
|
|
||
| function isObject(value: unknown): value is object { | ||
| // @ts-expect-error - bad types | ||
| return kindOf(value) === 'Object'; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Use native checks instead of an external library on this hot path.
JSON.parse is a heavily used hot path. Calling an external library like which-builtin-type on every parsed object introduces unnecessary function call and type-checking overhead. Using native JavaScript checks is significantly faster.
⚡ Proposed fix to use native checks
-import kindOf from 'which-builtin-type';
-
import { configRead } from './config';
function isObject(value: unknown): value is object {
- // `@ts-expect-error` - bad types
- return kindOf(value) === 'Object';
+ return typeof value === 'object' && value !== null && !Array.isArray(value);
}📝 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.
| import kindOf from 'which-builtin-type'; | |
| import { configRead } from './config'; | |
| function isObject(value: unknown): value is object { | |
| // @ts-expect-error - bad types | |
| return kindOf(value) === 'Object'; | |
| } | |
| import { configRead } from './config'; | |
| function isObject(value: unknown): value is object { | |
| return typeof value === 'object' && value !== null && !Array.isArray(value); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/remove-endscreen.ts` around lines 1 - 8, Replace the external kindOf call
in isObject with a native object check that identifies JSON-parsed plain objects
while excluding null and non-object values, and remove the now-unused
which-builtin-type import.
| const YTAF_OVERLAY_CLASS = 'ytaf-ui-watchControl-overlayMessage'; | ||
|
|
||
| const applyAudioOverlayFilter = () => { | ||
| const node = document.querySelector(AUDIO_OVERLAY_SELECTOR); | ||
| if (!node) return; | ||
| if (audioOnlyEnabled) { | ||
| node.style.setProperty('filter', 'brightness(0)', 'important'); | ||
| } else { | ||
| node.style.removeProperty('filter'); | ||
| } | ||
| }; | ||
| applyAudioOverlayFilter(); | ||
|
|
||
| showNotification( | ||
| `Audio-Only mode: ${audioOnlyEnabled ? 'Enabled' : 'Disabled'}`, | ||
| 2000, | ||
| 'blue' | ||
| ); | ||
|
|
||
| const controlsContainer = await requireElement( | ||
| '[idomkey="controls"]', | ||
| HTMLElement | ||
| ); | ||
|
|
||
| const updateOverlay = (root = controlsContainer) => { | ||
| let overlay = root.querySelector(`.${YTAF_OVERLAY_CLASS}`); | ||
|
|
||
| if (!audioOnlyEnabled) { | ||
| overlay?.remove(); | ||
| return; | ||
| } | ||
|
|
||
| if (overlay) return; | ||
|
|
||
| overlay = Object.assign(document.createElement('div'), { | ||
| textContent: 'Audio-Only Mode Enabled', | ||
| className: YTAF_OVERLAY_CLASS | ||
| }); | ||
| root.prepend(overlay); | ||
| }; | ||
| updateOverlay(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'ytaf-ui-watchControl-overlayMessage' --type=css --type=js --type=tsRepository: NicholasBly/youtube-webos
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ui.js around overlay code ---'
sed -n '1930,1995p' src/ui.js
echo
echo '--- all references to the overlay class ---'
rg -n 'ytaf-ui-watchControl-overlayMessage|AUDIO_OVERLAY_SELECTOR|applyAudioOverlayFilter|updateOverlay' src
echo
echo '--- ui.css around nearby watchControl styles ---'
rg -n 'watchControl|overlay|audio|controls' src/ui.cssRepository: NicholasBly/youtube-webos
Length of output: 2664
Style the audio-only overlay message.
ytaf-ui-watchControl-overlayMessage needs a CSS rule here; otherwise the injected overlay renders as plain text over the controls. Add positioning/background/typography styles in src/ui.css.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui.js` around lines 1936 - 1977, Add a CSS rule in src/ui.css for the
ytaf-ui-watchControl-overlayMessage class created by updateOverlay, defining
appropriate positioning, background, and typography so the audio-only message is
styled as an overlay over the controls.
| let timeoutToken: number | undefined; | ||
|
|
||
| // No reliable event for quality change, so poll for it | ||
| const intervalToken = window.setInterval(() => { | ||
| const currQuality = this.player.getPlaybackQualityLabel(); | ||
| if (currQuality !== prevQuality) { | ||
| notifyPlaybackQuality.call(this); | ||
| clearInterval(intervalToken); | ||
| clearTimeout(timeoutToken); | ||
| } | ||
| }, 100); | ||
|
|
||
| timeoutToken = window.setTimeout(() => { | ||
| console.warn('[video-quality] timed out waiting for quality change'); | ||
| clearInterval(intervalToken); | ||
| notifyPlaybackQuality.call(this); | ||
| }, 3000); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear previous polling intervals to prevent overlapping notifications.
If a user skips to a new video or triggers playbackStart rapidly before the 3-second timeout completes, multiple intervals and timeouts will run concurrently. This can lead to duplicate or delayed notifications from the previous video. Consider storing the tokens outside the function and clearing them at the beginning of setPlaybackQuality.
🐛 Proposed fix
Define the token variables outside the function:
+let qualityPollInterval: number | undefined;
+let qualityPollTimeout: number | undefined;
function setPlaybackQuality(this: PlayerManager, _: unknown) {Then update the function to clear and reuse them:
if (prevQuality === getMaxQualityLabel(this.player)) {
notifyPlaybackQuality.call(this);
return;
}
- let timeoutToken: number | undefined;
+ clearInterval(qualityPollInterval);
+ clearTimeout(qualityPollTimeout);
// No reliable event for quality change, so poll for it
- const intervalToken = window.setInterval(() => {
+ qualityPollInterval = window.setInterval(() => {
const currQuality = this.player.getPlaybackQualityLabel();
if (currQuality !== prevQuality) {
notifyPlaybackQuality.call(this);
- clearInterval(intervalToken);
- clearTimeout(timeoutToken);
+ clearInterval(qualityPollInterval);
+ clearTimeout(qualityPollTimeout);
}
}, 100);
- timeoutToken = window.setTimeout(() => {
+ qualityPollTimeout = window.setTimeout(() => {
console.warn('[video-quality] timed out waiting for quality change');
- clearInterval(intervalToken);
+ clearInterval(qualityPollInterval);
notifyPlaybackQuality.call(this);
}, 3000);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/video-quality.ts` around lines 45 - 62, Move the polling and timeout
token variables to shared scope for setPlaybackQuality, then clear any existing
interval and timeout at the start of that function before creating new timers.
Reuse the shared tokens in the quality-change and timeout callbacks, preserving
the existing notification and cleanup behavior while preventing overlapping
polling runs.
fix: clone in `stringify` hook to work around frozen objects
… lint/type errors
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/json-stringify.ts (1)
17-30: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard the legacy bundle against modern-only built-ins. The legacy build uses
babel.config.jswithchrome 38, so this path will hitstructuredCloneinsrc/hooks/json-stringify.tsandObject.hasOwninsrc/ui.js; both missing built-ins throwTypeErroron hot code paths. Use a shallow copy or legacy-safeJSON.stringifyhandling in the hook, and useObject.prototype.hasOwnProperty.call(...)or gateObject.hasOwnbehind runtime support insrc/ui.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/json-stringify.ts` around lines 17 - 30, The legacy bundle calls modern-only built-ins on hot paths. In src/hooks/json-stringify.ts lines 17-30, update the non-primitive cloning in the JSON.stringify hook to use a shallow copy or legacy-safe JSON.stringify handling instead of structuredClone; in src/ui.js lines 197-208, replace Object.hasOwn with Object.prototype.hasOwnProperty.call(...) or guard it behind runtime support, preserving the existing ownership check.src/app_api/index.ts (1)
105-115: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
setTimeout(poll, 0)still busy-polls. The prior round marked this as addressed, but the zero-delay retry remains — on webOS hardware this spins the macrotask queue until_yttvappears, competing with app startup. A 50–100ms interval (plus an eventual give-up) would be materially cheaper.⚡ Proposed fix
if (hook) { resolve(hook); } else { - setTimeout(poll, 0); + setTimeout(poll, 50); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app_api/index.ts` around lines 105 - 115, Update the polling loop in the surrounding hook-waiting method to avoid zero-delay busy polling: retry on a 50–100ms interval and add an eventual give-up path that settles the Promise when the hook never appears. Preserve immediate resolution when findHookTarget() returns a hook and ensure any timer is stopped once polling completes.src/hooks/fetch.ts (1)
65-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
FileReadererror/abort still unhandled — debug fetches can hang forever. Onlyloadsettles the promise, so a read failure leaves#customFetchawaiting indefinitely when__ytaf_debug__is on.🐛 Proposed fix
const res = new Promise<string | ArrayBuffer | null>((resolve) => { fr.addEventListener('load', () => { resolve(fr.result); }); + fr.addEventListener('error', () => resolve(null)); + fr.addEventListener('abort', () => resolve(null)); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/fetch.ts` around lines 65 - 75, Update the FileReader promise in the custom fetch flow to settle on read failures and aborts as well as successful load events. Attach error and abort handlers alongside the existing load handler, rejecting the promise with the corresponding FileReader error or abort failure so `#customFetch` cannot remain pending when __ytaf_debug__ is enabled.
🧹 Nitpick comments (10)
eslint.config.ts (1)
60-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope
preserve-caught-erroraway from the legacy webOS target.The shared rules apply to source files that still ship a webOS 3 build. ESLint specifically notes that this rule may be unsuitable when
Error’scauseoption is unsupported; fixing violations by adding{ cause: error }will not preserve error context on those TVs. Restrict it to tooling/modern targets or verify a legacy-compatible wrapper/polyfill. (eslint.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.ts` at line 60, Restrict the preserve-caught-error rule in the ESLint configuration to tooling or modern-target source files, excluding the legacy webOS 3 build. If shared coverage is required, use a legacy-compatible error wrapper or polyfill instead of requiring unsupported Error cause options.Source: MCP tools
.github/workflows/main.yml (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the CI Node.js version.
.github/actions/setup-env/action.yamldefaultsnode-versiontolts/*, but this workflow supplies no explicit version. The CI toolchain will silently change as the LTS line advances; pass the repository-declared Node.js target explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/main.yml at line 17, Update the setup-env action invocation in the workflow to pass the repository-declared Node.js target explicitly via its node-version input, instead of relying on the action's lts/* default. Preserve the existing setup-env usage and use the version already declared by the repository.src/adblock.js (1)
493-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable
PLAYERcheck in the fallback branch.
SCHEMA_REGISTRY.paths.PLAYERis always defined ({ overlayPath: [...] }), so the precedingelse if (responseType && SCHEMA_REGISTRY.paths[responseType])branch always matches PLAYER first. TheresponseType === 'PLAYER'check here is dead code — harmless since both branches call the identicalapplySchemaFilters(...), but confusing for future readers.♻️ Proposed simplification
- } else if (responseType === 'ACTION' || responseType === 'PLAYER') { + } else if (responseType === 'ACTION') { if (DEBUG) debugLog(`Schema Match: [${responseType}]`); applySchemaFilters(data, responseType, cfgFlags, needsContentFiltering);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/adblock.js` around lines 493 - 506, Remove the redundant responseType === 'ACTION' || responseType === 'PLAYER' fallback branch following the SCHEMA_REGISTRY.paths lookup. Keep the existing schema handling through applySchemaFilters for registered response types, including PLAYER, and leave the remaining fallback behavior unchanged.src/hooks/fetch.ts (2)
127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the typed cast used on the fast path. Line 90 already uses
resource as Parameters<typeof fetch>[0]; usingas anyhere loses the check for no benefit.♻️ Proposed change
- const res = await this.#originalFetch(resource as any, init); + const res = await this.#originalFetch( + resource as Parameters<typeof fetch>[0], + init + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/fetch.ts` at line 127, In the fetch path around the `#originalFetch` call, replace the broad resource as any cast with the typed cast already used on the fast path: resource as Parameters<typeof fetch>[0]. Preserve the existing fetch invocation and init handling.
33-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winListener counts can drift out of sync with the real listener set.
addEventListenerincrements even when the native target de-dupes an identical(type, callback, options)registration, andremoveEventListenerdecrements for callbacks that were never registered. Drift in the low direction silently disablesrequest/responsedispatch (adblock telemetry filtering would stop) while a real listener is still attached. Tracking callbacks in aSet<EventListenerOrEventListenerObject>per type and deriving the count fromset.sizeavoids both directions of drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/fetch.ts` around lines 33 - 46, Update the listener tracking in the event-target class around addEventListener and removeEventListener to maintain a separate Set<EventListenerOrEventListenerObject> for each request/response type, adding and removing callbacks only through native registration semantics and deriving `#listenerCounts` from each set’s size. Ensure duplicate registrations and removals of unregistered callbacks do not change the tracked count, while preserving existing event-target behavior.src/return-dislike.js (2)
136-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTimeout timer isn't cleared when the fetch wins the race. Each
fetchVideoDataleaves an 8s pending timer whose rejection is discarded; harmless but avoidable by capturing the id and clearing it infinally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/return-dislike.js` around lines 136 - 144, Update fetchVideoData’s Promise.race timeout handling to capture the setTimeout identifier and clear it in a finally block after the race settles. Preserve the existing 8-second timeout rejection and fetch result behavior while ensuring the timer is removed whether fetch resolves, rejects, or times out.
11-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
mainContainerselector.zylon-provider-6is only stored inSELECTORSand never referenced after thezylon-providerupdate, so it is left as dead, hashed selector documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/return-dislike.js` around lines 11 - 29, Remove the unused mainContainer entry from the SELECTORS object in return-dislike.js, leaving all other selector definitions unchanged.src/auto-login.js (1)
110-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the commented-out launch-param logic.
Lines 110-111 and 114 are dead code for an
extractLaunchParamsimport that this PR removed. Deleting them keeps the bypass condition readable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/auto-login.js` around lines 110 - 114, Remove the commented-out extractLaunchParams and hasParams declarations, along with the trailing commented conditional, from the logic surrounding the isSelector/force guard. Keep the active bypass condition unchanged.src/config.js (1)
228-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate listener failures so one throwing subscriber doesn't starve the rest.
sponsorblock.js,ui.js,watch.jsandvideo-quality.jsall subscribe to config keys. An exception in any callback aborts the loop, so later subscribers never observe the change and drift out of sync withlocalConfig.♻️ Proposed refactor
const listeners = changeListeners.get(key); if (listeners) { const syntheticEvent = { detail: { key, newValue: value, oldValue } }; - for (const callback of listeners) { - callback(syntheticEvent); - } + for (const callback of [...listeners]) { + try { + callback(syntheticEvent); + } catch (e) { + console.error('config listener failed for key', key, e); + } + } }Copying the Set also avoids surprises when a callback adds or removes listeners mid-dispatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.js` around lines 228 - 234, Update the listener dispatch around changeListeners.get(key) to iterate over a copy of the listener set, and isolate each callback invocation in its own error boundary so one failing subscriber cannot prevent later subscribers from receiving the syntheticEvent. Preserve the existing event payload and localConfig update behavior.src/perf_mon.js (1)
56-57: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnstoppable 1 Hz timer with no teardown handle.
The interval id is discarded, so
updateUI(which runsdocument.querySelectorAll('*')on YouTube's multi-thousand-node tree, line 169) executes every second for the lifetime of the app with no way to stop it. Store the id and expose adestroy()/stop().♻️ Proposed change
document.body.appendChild(this.uiElement); - setInterval(() => this.updateUI(), 1000); // Update OSD every second + this.updateTimer = setInterval(() => this.updateUI(), 1000); // Update OSD every second + } + + destroy() { + clearInterval(this.updateTimer); + this.updateTimer = null; + if (this.uiElement) { + this.uiElement.remove(); + this.uiElement = null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/perf_mon.js` around lines 56 - 57, Update the initialization flow around the interval that calls updateUI to store its timer handle on the instance, and add a public destroy() or stop() method that clears the interval and removes the associated UI element. Ensure teardown is safe to call and prevents further updateUI executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 9: Update the changelog reference definitions for [Unreleased] and
[0.8.1]: add the missing 0.8.1 comparison reference, use the actual 0.8.1 tag
spelling without the “v” prefix, and ensure the Unreleased comparison points
from 0.8.1 to HEAD.
- Line 61: Update CHANGELOG.md to eliminate duplicate sibling headings for
Optimizations, Changes, Fixes, and Added by nesting release-specific sections
under their corresponding release headings; alternatively, configure the
changelog lint rule with MD024.headers.siblings_only or a targeted
changelog-specific exception.
In `@src/perf_mon.js`:
- Around line 261-264: Gate singleton construction at src/perf_mon.js lines
261-264 behind a non-production or explicit opt-in check so shipped builds have
no monitor side effects. In the MutationObserver instrumentation at
src/perf_mon.js lines 131-156, preserve the prototype, invoke callbacks with the
observer context, use try/finally for timing, and bound mutationStats. In the
interval setup at src/perf_mon.js lines 56-57, retain the interval ID and add
PerfMonitor.destroy() to clear it and remove uiElement.
In `@src/spatial-navigation.modern.js`:
- Around line 441-449: Update the !isFocusable(bestTarget) branch in the
spatial-navigation search flow to call candidates.indexOf(bestTarget) before
splicing, and only remove an element when the returned index is nonnegative. If
bestTarget is not present, avoid mutating candidates and preserve the existing
search behavior without recursively retrying the unchanged input.
- Around line 1434-1438: Update the initialization flow around
spatialNavigationHandler so it runs immediately when the document has already
finished loading, while retaining the window load listener for earlier
injection. Ensure the handler is invoked exactly once so its keydown, mouseup,
and focusin listeners are not duplicated.
- Around line 116-129: Ensure the per-navigation geometry caches are always
cleared when navigate throws by wrapping the navigate(dir) flow in cleanup logic
that resets mapOfBoundRect, mapOfComputedStyle, and startingPoint on both
success and failure. Also update the eventTarget.nodeName access in the
surrounding key-navigation handler to safely support a null
document.activeElement via optional access.
In `@src/sponsorblock.js`:
- Around line 889-902: In the overlay insertion logic using the cached anchor
from _getProgressBarAnchor, validate that container.parentNode still exists
before calling insertBefore or appendChild when asSibling is true. If the cached
container is detached, exit or safely skip insertion and allow the existing
anchor reacquisition flow to handle it, while preserving normal sibling and
append behavior for attached containers.
- Around line 1154-1177: Update the automatic skip flow around the `isSkipping`
assignment and `this.video.currentTime` update to avoid performing a no-op seek
when playback has already passed `jumpTarget`, while still clearing the skipping
state. Add a 500ms `_skipWatchdog` reset matching `handleBlueButton`, and clear
that timer in `destroy()` alongside the other timers.
In `@src/thumbnail-quality.js`:
- Around line 486-504: Set isObserving immediately at the start of
enableObserver, before any await or DOM lookup, so concurrent calls return
without registering duplicate listeners or observers. Remove the later
isObserving assignment while preserving the existing initialization flow.
In `@src/utils.js`:
- Around line 298-311: Harden the object branch in handleLaunch before
destructuring or calling match: require contentTarget to be non-null and ensure
intent is a string, while preserving the existing URL parameter updates for
valid targets. Skip this branch safely when contentTarget is null or intent is
absent/invalid so navigation does not throw.
In `@src/yt-fixes.js`:
- Around line 113-118: Update the pending-check branch in
attemptSearchHistoryFix to return false while the 500ms deferred attempt is
still pending, so initSearchHistoryFix keeps the observer connected. Preserve
the historyCache success short-circuit, allowing the observer to stop only after
cache population actually succeeds.
---
Outside diff comments:
In `@src/app_api/index.ts`:
- Around line 105-115: Update the polling loop in the surrounding hook-waiting
method to avoid zero-delay busy polling: retry on a 50–100ms interval and add an
eventual give-up path that settles the Promise when the hook never appears.
Preserve immediate resolution when findHookTarget() returns a hook and ensure
any timer is stopped once polling completes.
In `@src/hooks/fetch.ts`:
- Around line 65-75: Update the FileReader promise in the custom fetch flow to
settle on read failures and aborts as well as successful load events. Attach
error and abort handlers alongside the existing load handler, rejecting the
promise with the corresponding FileReader error or abort failure so `#customFetch`
cannot remain pending when __ytaf_debug__ is enabled.
In `@src/hooks/json-stringify.ts`:
- Around line 17-30: The legacy bundle calls modern-only built-ins on hot paths.
In src/hooks/json-stringify.ts lines 17-30, update the non-primitive cloning in
the JSON.stringify hook to use a shallow copy or legacy-safe JSON.stringify
handling instead of structuredClone; in src/ui.js lines 197-208, replace
Object.hasOwn with Object.prototype.hasOwnProperty.call(...) or guard it behind
runtime support, preserving the existing ownership check.
---
Nitpick comments:
In @.github/workflows/main.yml:
- Line 17: Update the setup-env action invocation in the workflow to pass the
repository-declared Node.js target explicitly via its node-version input,
instead of relying on the action's lts/* default. Preserve the existing
setup-env usage and use the version already declared by the repository.
In `@eslint.config.ts`:
- Line 60: Restrict the preserve-caught-error rule in the ESLint configuration
to tooling or modern-target source files, excluding the legacy webOS 3 build. If
shared coverage is required, use a legacy-compatible error wrapper or polyfill
instead of requiring unsupported Error cause options.
In `@src/adblock.js`:
- Around line 493-506: Remove the redundant responseType === 'ACTION' ||
responseType === 'PLAYER' fallback branch following the SCHEMA_REGISTRY.paths
lookup. Keep the existing schema handling through applySchemaFilters for
registered response types, including PLAYER, and leave the remaining fallback
behavior unchanged.
In `@src/auto-login.js`:
- Around line 110-114: Remove the commented-out extractLaunchParams and
hasParams declarations, along with the trailing commented conditional, from the
logic surrounding the isSelector/force guard. Keep the active bypass condition
unchanged.
In `@src/config.js`:
- Around line 228-234: Update the listener dispatch around
changeListeners.get(key) to iterate over a copy of the listener set, and isolate
each callback invocation in its own error boundary so one failing subscriber
cannot prevent later subscribers from receiving the syntheticEvent. Preserve the
existing event payload and localConfig update behavior.
In `@src/hooks/fetch.ts`:
- Line 127: In the fetch path around the `#originalFetch` call, replace the broad
resource as any cast with the typed cast already used on the fast path: resource
as Parameters<typeof fetch>[0]. Preserve the existing fetch invocation and init
handling.
- Around line 33-46: Update the listener tracking in the event-target class
around addEventListener and removeEventListener to maintain a separate
Set<EventListenerOrEventListenerObject> for each request/response type, adding
and removing callbacks only through native registration semantics and deriving
`#listenerCounts` from each set’s size. Ensure duplicate registrations and
removals of unregistered callbacks do not change the tracked count, while
preserving existing event-target behavior.
In `@src/perf_mon.js`:
- Around line 56-57: Update the initialization flow around the interval that
calls updateUI to store its timer handle on the instance, and add a public
destroy() or stop() method that clears the interval and removes the associated
UI element. Ensure teardown is safe to call and prevents further updateUI
executions.
In `@src/return-dislike.js`:
- Around line 136-144: Update fetchVideoData’s Promise.race timeout handling to
capture the setTimeout identifier and clear it in a finally block after the race
settles. Preserve the existing 8-second timeout rejection and fetch result
behavior while ensuring the timer is removed whether fetch resolves, rejects, or
times out.
- Around line 11-29: Remove the unused mainContainer entry from the SELECTORS
object in return-dislike.js, leaving all other selector definitions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d88c135-d936-4e21-813d-69e632c54982
⛔ Files ignored due to path filters (3)
dist/appinfo.jsonis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/webOSUserScripts/userScript.jsis excluded by!**/dist/**
📒 Files selected for processing (33)
.github/workflows/main.ymlCHANGELOG.mdREADME.mdassets/appinfo.jsoneslint.config.tspackage.jsonrepo.jsonsrc/adblock.jssrc/app_api/index.tssrc/auto-account-select.tssrc/auto-login.jssrc/block-webos-cast.tssrc/config.jssrc/hooks/fetch.tssrc/hooks/json-stringify.tssrc/perf_mon.jssrc/player_api/manager.tssrc/remove-endscreen.tssrc/return-dislike.jssrc/screensaver-fix.jssrc/spatial-navigation.modern.jssrc/sponsorblock.jssrc/thumbnail-quality.jssrc/thumbnail-quality.tssrc/ui.csssrc/ui.jssrc/userScript.tssrc/utils.jssrc/video-quality.jssrc/watch.jssrc/yt-fixes.csssrc/yt-fixes.jswebpack.config.js
🚧 Files skipped from review as they are similar to previous changes (10)
- src/block-webos-cast.ts
- src/remove-endscreen.ts
- src/auto-account-select.ts
- src/yt-fixes.css
- package.json
- src/player_api/manager.ts
- src/userScript.ts
- webpack.config.js
- src/watch.js
- src/thumbnail-quality.ts
| ## [Unreleased] | ||
|
|
||
| ## [0.8.1] - 2026/07/20 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the new changelog reference links.
[0.8.1] has no reference definition, and [Unreleased] points to compare/v0.8.1...HEAD. The fork uses 0.8.1 without the v prefix, so the current comparison URL returns 404. Add the 0.8.1 comparison reference and use the actual tag spelling. ()
Proposed footer fix
-[Unreleased]: https://github.com/NicholasBly/youtube-webos/compare/v0.8.1...HEAD
+[Unreleased]: https://github.com/NicholasBly/youtube-webos/compare/0.8.1...HEAD
+[0.8.1]: https://github.com/NicholasBly/youtube-webos/compare/0.8.0...0.8.1Also applies to: 1731-1732
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 9, Update the changelog reference definitions for
[Unreleased] and [0.8.1]: add the missing 0.8.1 comparison reference, use the
actual 0.8.1 tag spelling without the “v” prefix, and ensure the Unreleased
comparison points from 0.8.1 to HEAD.
Source: MCP tools
| Codebase-wide audit pass focused on **webOS 3 performance**, correctness bugs, and code-quality cleanup. Settings panel reliability, SponsorBlock UI labels, and overall CPU usage on older TVs all improved. | ||
|
|
||
| ## Optimizations |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)CHANGELOG\.md$|^\.markdownlint|markdownlint' || true
echo
echo "CHANGELOG excerpt around heading lines:"
if [ -f CHANGELOG.md ]; then
nl -ba CHANGELOG.md | sed -n '1,130p'
fi
echo
echo "Markdownlint config files:"
for f in $(git ls-files | rg '(^|/)\.markdownlint.*|(^|/)markdownlint.*|(^|/)package\.json$|(^|/)pyproject\.toml$|(^|/)setup\.cfg$|(^|/)tox\.ini$|(^|/)Makefile$' || true); do
echo "--- $f"
rg -n 'markdownlint|MD024|no-duplicate|single|headers|heading|CHANGELOG|ignore' "$f" || true
done
echo
echo "All ## headings in CHANGELOG.md:"
python3 - <<'PY'
from pathlib import Path
p=Path('CHANGELOG.md')
if not p.exists():
exit(0)
lines=p.read_text().splitlines()
from collections import defaultdict
d=defaultdict(list)
for i,l in enumerate(lines,1):
s=l.strip()
if s.startswith('## '):
d[s].append(i)
for h,ls in sorted(d.items(), key=lambda x:x[0]):
if len(ls)>1:
print(h, ls)
PYRepository: NicholasBly/youtube-webos
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CHANGELOG.md line count:"
wc -l CHANGELOG.md
echo
echo "All headings in CHANGELOG.md:"
python3 - <<'PY'
from collections import defaultdict
from pathlib import Path
lines = Path("CHANGELOG.md").read_text().splitlines()
by_heading = defaultdict(list)
for i, line in enumerate(lines, 1):
stripped = line.strip()
if stripped.startswith("#") and not stripped.startswith("###"):
by_heading[stripped].append(i)
for heading, lines_ in sorted(by_heading.items()):
print(f"[{heading}] #{', '.join(map(str, lines_))}")
PY
echo
echo "Occurrences of common lint/Markdown tool configuration keys in tracked config files:"
python3 - <<'PY'
from pathlib import Path
import re
keys = ["markdownlint", "MD024", "no-duplicate", "single", "headers", "heading", "CHANGELOG", "ignore", "rules"]
for p in [Path(".markdownlintrc"), Path(".markdownlint.yaml"), Path(".markdownlint.json"), Path("package.json"), Path("pyproject.toml"), Path("setup.cfg"), Path("tox.ini"), Path("Makefile")]:
if p.exists():
print(f"--- {p}")
text = p.read_text()
found = []
for n, line in enumerate(text.splitlines(), 1):
if any(k.lower() in line.lower() for k in keys):
found.append((n, line))
for n, line in found:
print(f"{n}: {line}")
PYRepository: NicholasBly/youtube-webos
Length of output: 3223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CHANGELOG.md around headings 50-110:"
python3 - <<'PY'
from pathlib import Path
lines = Path("CHANGELOG.md").read_text().splitlines()
for i in range(max(1, 50), min(len(lines), 110) + 1):
print(f"{i}: {lines[i-1]}")
PYRepository: NicholasBly/youtube-webos
Length of output: 3965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CHANGELOG.md line count:"
wc -l CHANGELOG.md
echo
echo "All headings in CHANGELOG.md:"
python3 - <<'PY'
from collections。defaultdict
from pathlib import Path
lines = Path("CHANGELOG.md").read_text().splitlines()
by_heading = defaultdict(list)
for i, line in enumerate(lines, 1):
stripped = line.strip()
if stripped.startswith("#") and not stripped.startswith("###"):
by_heading[stripped].append(i)
for heading, lines_ in sorted(by_heading.items()):
print(f"## {', '.join(map(str, lines_))}: {heading}")
PY
echo
echo "Occurrences of Markdown lint/config keys:"
python3 - <<'PY'
from pathlib import Path
keys = ["markdownlint", "MD024", "duplicate", "heading", "header", "changelog", "ignore"]
for name in [".markdownlintrc",".markdownlint.yaml",".markdownlint.json","package.json","pyproject.toml","setup.cfg","tox.ini","Makefile"]:
p=Path(name)
if not p.exists():
continue
print(f"--- {name}")
for n, line in enumerate(p.read_text().splitlines(), 1):
if any(k.lower() in line.lower() for k in keys):
print(f"{n}: {line}")
PYRepository: NicholasBly/youtube-webos
Length of output: 361
Resolve the duplicate-heading lint warnings in the changelog.
CHANGELOG.md contains repeated ## Optimizations, ## Changes, ## Fixes, and ## Added sibling headings, which trigger MD024. Nest release-specific subsections under each release heading and set MD024.headers: { siblings_only: true }, or add a changelog-specific MD024 exception.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 61-61: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 61, Update CHANGELOG.md to eliminate duplicate sibling
headings for Optimizations, Changes, Fixes, and Added by nesting
release-specific sections under their corresponding release headings;
alternatively, configure the changelog lint rule with
MD024.headers.siblings_only or a targeted changelog-specific exception.
Source: Linters/SAST tools
| // Inject singleton | ||
| if (!window.__webosPerfMon) { | ||
| window.__webosPerfMon = new PerfMonitor(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Ungated debug instrumentation with global side effects ships to end users. The root cause is that importing src/perf_mon.js unconditionally constructs the monitor: there is no DEBUG/NODE_ENV gate, unlike every other diagnostic in this repo (src/config.js line 216, src/video-quality.js, src/utils.js). Because construction is unconditional, its two global side effects — a permanently patched DOM constructor and an unstoppable 1 Hz DOM walk — are also unconditional.
src/perf_mon.js#L261-L264: gate the singleton behindprocess.env.NODE_ENV !== 'production'(or an explicit opt-in flag) so nothing below runs in shipped builds.src/perf_mon.js#L131-L156: preserve theMutationObservercontract — restoreprototype, invoke the callback withcallback.call(this, ...), wrap the timing intry/finally, and cap the unboundedmutationStatsmap.src/perf_mon.js#L56-L57: store thesetIntervalid and add adestroy()that clears it and removesthis.uiElement, so the monitor can be turned off once enabled.
📍 Affects 1 file
src/perf_mon.js#L261-L264(this comment)src/perf_mon.js#L131-L156src/perf_mon.js#L56-L57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/perf_mon.js` around lines 261 - 264, Gate singleton construction at
src/perf_mon.js lines 261-264 behind a non-production or explicit opt-in check
so shipped builds have no monitor side effects. In the MutationObserver
instrumentation at src/perf_mon.js lines 131-156, preserve the prototype, invoke
callbacks with the observer context, use try/finally for timing, and bound
mutationStats. In the interval setup at src/perf_mon.js lines 56-57, retain the
interval ID and add PerfMonitor.destroy() to clear it and remove uiElement.
| if (focusNavigableArrowKey[dir]) { | ||
| e.preventDefault(); | ||
|
|
||
| // Use standard Map for fastest possible single-frame read/write speeds | ||
| mapOfBoundRect = new Map(); | ||
| mapOfComputedStyle = new Map(); | ||
|
|
||
| navigate(dir); | ||
|
|
||
| // Free memory instantly | ||
| mapOfBoundRect = null; | ||
| mapOfComputedStyle = null; | ||
| startingPoint = null; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A throw inside navigate() permanently poisons the per-tick geometry caches.
mapOfBoundRect / mapOfComputedStyle are created at lines 120-121 and cleared at 126-127 only on the success path. If navigate(dir) throws — easy to reach, e.g. getSpatialNavigationContainer walking a detached node, or the cross-frame window.parent.document accesses at lines 224-226 hitting a SecurityError — the maps stay populated for the rest of the session. Every later navigation then reads stale DOMRect values and focus lands on the wrong elements, with no recovery short of a reload.
🛡️ Proposed fix
if (focusNavigableArrowKey[dir]) {
e.preventDefault();
// Use standard Map for fastest possible single-frame read/write speeds
mapOfBoundRect = new Map();
mapOfComputedStyle = new Map();
- navigate(dir);
-
- // Free memory instantly
- mapOfBoundRect = null;
- mapOfComputedStyle = null;
- startingPoint = null;
+ try {
+ navigate(dir);
+ } finally {
+ // Free memory instantly — must happen even if navigate() throws,
+ // otherwise stale rects leak into every subsequent tick.
+ mapOfBoundRect = null;
+ mapOfComputedStyle = null;
+ startingPoint = null;
+ }
}Line 110 also dereferences eventTarget.nodeName while document.activeElement can be null; worth an eventTarget?.nodeName there.
📝 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 (focusNavigableArrowKey[dir]) { | |
| e.preventDefault(); | |
| // Use standard Map for fastest possible single-frame read/write speeds | |
| mapOfBoundRect = new Map(); | |
| mapOfComputedStyle = new Map(); | |
| navigate(dir); | |
| // Free memory instantly | |
| mapOfBoundRect = null; | |
| mapOfComputedStyle = null; | |
| startingPoint = null; | |
| } | |
| if (focusNavigableArrowKey[dir]) { | |
| e.preventDefault(); | |
| // Use standard Map for fastest possible single-frame read/write speeds | |
| mapOfBoundRect = new Map(); | |
| mapOfComputedStyle = new Map(); | |
| try { | |
| navigate(dir); | |
| } finally { | |
| // Free memory instantly — must happen even if navigate() throws, | |
| // otherwise stale rects leak into every subsequent tick. | |
| mapOfBoundRect = null; | |
| mapOfComputedStyle = null; | |
| startingPoint = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spatial-navigation.modern.js` around lines 116 - 129, Ensure the
per-navigation geometry caches are always cleared when navigate throws by
wrapping the navigate(dir) flow in cleanup logic that resets mapOfBoundRect,
mapOfComputedStyle, and startingPoint on both success and failure. Also update
the eventTarget.nodeName access in the surrounding key-navigation handler to
safely support a null document.activeElement via optional access.
| else if (!isFocusable(bestTarget)) { | ||
| candidates.splice(candidates.indexOf(bestTarget), 1); | ||
| bestTarget = candidates.length | ||
| ? targetElement.spatialNavigationSearch(dir, { | ||
| candidates, | ||
| container | ||
| }) | ||
| : null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard indexOf before splicing.
bestTarget can originate from externalCandidates (a filtered copy, line 397) or from focusableAreas() (line 388), neither of which is guaranteed to be a member of candidates. When indexOf returns -1, splice(-1, 1) silently removes the last candidate instead and then recurses — dropping a valid target and risking repeated recursion on the same input.
🛡️ Proposed guard
else if (!isFocusable(bestTarget)) {
- candidates.splice(candidates.indexOf(bestTarget), 1);
- bestTarget = candidates.length
+ const idx = candidates.indexOf(bestTarget);
+ if (idx === -1) return null;
+ candidates.splice(idx, 1);
+ bestTarget = candidates.length
? targetElement.spatialNavigationSearch(dir, {
candidates,
container
})
: null;
}📝 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.
| else if (!isFocusable(bestTarget)) { | |
| candidates.splice(candidates.indexOf(bestTarget), 1); | |
| bestTarget = candidates.length | |
| ? targetElement.spatialNavigationSearch(dir, { | |
| candidates, | |
| container | |
| }) | |
| : null; | |
| } | |
| else if (!isFocusable(bestTarget)) { | |
| const idx = candidates.indexOf(bestTarget); | |
| if (idx === -1) return null; | |
| candidates.splice(idx, 1); | |
| bestTarget = candidates.length | |
| ? targetElement.spatialNavigationSearch(dir, { | |
| candidates, | |
| container | |
| }) | |
| : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/spatial-navigation.modern.js` around lines 441 - 449, Update the
!isFocusable(bestTarget) branch in the spatial-navigation search flow to call
candidates.indexOf(bestTarget) before splicing, and only remove an element when
the returned index is nonnegative. If bestTarget is not present, avoid mutating
candidates and preserve the existing search behavior without recursively
retrying the unchanged input.
| const { container, asSibling } = this._getProgressBarAnchor(); | ||
| if (asSibling) { | ||
| // insertAdjacentElement('afterend') requires Chrome 41+, not available on | ||
| // WebOS 3 (Chrome 38). Use insertBefore with nextSibling instead. | ||
| const nextSib = container.nextSibling; | ||
| if (nextSib) { | ||
| container.parentNode.insertBefore(this.overlay, nextSib); | ||
| } else { | ||
| container.parentNode.appendChild(this.overlay); | ||
| } | ||
| this._syncOverlayPosition(container); | ||
| } else { | ||
| container.appendChild(this.overlay); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the cached anchor's parentNode before inserting.
container comes from the _anchorCache, which is only invalidated when checkForProgressBar re-acquires the bar. If the framework detaches ytlr-progress-bar between the cache write and this draw, container.parentNode is null and lines 895/897 throw inside a MutationObserver/RAF callback.
🛡️ Proposed guard
const { container, asSibling } = this._getProgressBarAnchor();
+ if (!container) return;
if (asSibling) {
+ const parent = container.parentNode;
+ if (!parent) {
+ // Bar was detached since the anchor was cached — re-acquire next tick.
+ this._anchorCache = null;
+ this._anchorCacheBar = null;
+ return;
+ }
// insertAdjacentElement('afterend') requires Chrome 41+, not available on
// WebOS 3 (Chrome 38). Use insertBefore with nextSibling instead.
const nextSib = container.nextSibling;
if (nextSib) {
- container.parentNode.insertBefore(this.overlay, nextSib);
+ parent.insertBefore(this.overlay, nextSib);
} else {
- container.parentNode.appendChild(this.overlay);
+ parent.appendChild(this.overlay);
}
this._syncOverlayPosition(container);
} else {
container.appendChild(this.overlay);
}📝 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 { container, asSibling } = this._getProgressBarAnchor(); | |
| if (asSibling) { | |
| // insertAdjacentElement('afterend') requires Chrome 41+, not available on | |
| // WebOS 3 (Chrome 38). Use insertBefore with nextSibling instead. | |
| const nextSib = container.nextSibling; | |
| if (nextSib) { | |
| container.parentNode.insertBefore(this.overlay, nextSib); | |
| } else { | |
| container.parentNode.appendChild(this.overlay); | |
| } | |
| this._syncOverlayPosition(container); | |
| } else { | |
| container.appendChild(this.overlay); | |
| } | |
| const { container, asSibling } = this._getProgressBarAnchor(); | |
| if (!container) return; | |
| if (asSibling) { | |
| const parent = container.parentNode; | |
| if (!parent) { | |
| // Bar was detached since the anchor was cached — re-acquire next tick. | |
| this._anchorCache = null; | |
| this._anchorCacheBar = null; | |
| return; | |
| } | |
| // insertAdjacentElement('afterend') requires Chrome 41+, not available on | |
| // WebOS 3 (Chrome 38). Use insertBefore with nextSibling instead. | |
| const nextSib = container.nextSibling; | |
| if (nextSib) { | |
| parent.insertBefore(this.overlay, nextSib); | |
| } else { | |
| parent.appendChild(this.overlay); | |
| } | |
| this._syncOverlayPosition(container); | |
| } else { | |
| container.appendChild(this.overlay); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sponsorblock.js` around lines 889 - 902, In the overlay insertion logic
using the cached anchor from _getProgressBarAnchor, validate that
container.parentNode still exists before calling insertBefore or appendChild
when asSibling is true. If the cached container is detached, exit or safely skip
insertion and allow the existing anchor reacquisition flow to handle it, while
preserving normal sibling and append behavior for attached containers.
| this.isSkipping = true; | ||
| this.lastSkipTime = currentTime; | ||
| this.lastSkippedSegmentIndex = segmentIdx; | ||
|
|
||
| segmentsToMark.forEach((idx) => { | ||
| this.skippedSegmentIndices.add(idx); | ||
| }); | ||
|
|
||
| if (this.isLegacyWebOSVer) { | ||
| const duration = this.video.duration; | ||
| if (jumpTarget >= duration - 0.5) { | ||
| jumpTarget = Math.max(0, duration - 0.25); | ||
| } | ||
| } | ||
|
|
||
| // Prevents a micro-rewind if a frame drop caused us to overshoot the jump target | ||
| this.video.currentTime = Math.max(jumpTarget, currentTime); | ||
|
|
||
| if (!this.isLegacyWebOSVer) { | ||
| const timeRemaining = this.video.duration - this.video.currentTime; | ||
| if (timeRemaining > 0.5 && this.video.paused) { | ||
| this.video.play(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
isSkipping can latch on and disable all further auto-skips.
isSkipping = true (line 1154) is cleared only by the seeked handler (line 670). Line 1170 clamps with Math.max(jumpTarget, currentTime), so when a frame drop has already carried playback past jumpTarget the assignment writes the current time back — which need not produce a seeked event. isSkipping then stays true and the guard at line 963 short-circuits handleTimeUpdate for the remainder of the video, silently killing every subsequent skip.
handleBlueButton already hedges against exactly this with a 500ms reset (lines 1261-1263); the auto path needs the same watchdog.
🐛 Proposed fix: skip the no-op seek and add a watchdog
this.isSkipping = true;
this.lastSkipTime = currentTime;
this.lastSkippedSegmentIndex = segmentIdx;
@@
// Prevents a micro-rewind if a frame drop caused us to overshoot the jump target
- this.video.currentTime = Math.max(jumpTarget, currentTime);
+ const seekTarget = Math.max(jumpTarget, currentTime);
+ if (seekTarget > currentTime) {
+ this.video.currentTime = seekTarget;
+ // Fallback in case 'seeked' never fires (buffering / legacy webOS).
+ clearTimeout(this._skipWatchdog);
+ this._skipWatchdog = setTimeout(() => {
+ this.isSkipping = false;
+ }, 1000);
+ } else {
+ // Nothing to seek — don't leave the state machine latched.
+ this.isSkipping = false;
+ }Clear this._skipWatchdog in destroy() alongside the other timers.
📝 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.
| this.isSkipping = true; | |
| this.lastSkipTime = currentTime; | |
| this.lastSkippedSegmentIndex = segmentIdx; | |
| segmentsToMark.forEach((idx) => { | |
| this.skippedSegmentIndices.add(idx); | |
| }); | |
| if (this.isLegacyWebOSVer) { | |
| const duration = this.video.duration; | |
| if (jumpTarget >= duration - 0.5) { | |
| jumpTarget = Math.max(0, duration - 0.25); | |
| } | |
| } | |
| // Prevents a micro-rewind if a frame drop caused us to overshoot the jump target | |
| this.video.currentTime = Math.max(jumpTarget, currentTime); | |
| if (!this.isLegacyWebOSVer) { | |
| const timeRemaining = this.video.duration - this.video.currentTime; | |
| if (timeRemaining > 0.5 && this.video.paused) { | |
| this.video.play(); | |
| } | |
| } | |
| this.isSkipping = true; | |
| this.lastSkipTime = currentTime; | |
| this.lastSkippedSegmentIndex = segmentIdx; | |
| segmentsToMark.forEach((idx) => { | |
| this.skippedSegmentIndices.add(idx); | |
| }); | |
| if (this.isLegacyWebOSVer) { | |
| const duration = this.video.duration; | |
| if (jumpTarget >= duration - 0.5) { | |
| jumpTarget = Math.max(0, duration - 0.25); | |
| } | |
| } | |
| // Prevents a micro-rewind if a frame drop caused us to overshoot the jump target | |
| const seekTarget = Math.max(jumpTarget, currentTime); | |
| if (seekTarget > currentTime) { | |
| this.video.currentTime = seekTarget; | |
| // Fallback in case 'seeked' never fires (buffering / legacy webOS). | |
| clearTimeout(this._skipWatchdog); | |
| this._skipWatchdog = setTimeout(() => { | |
| this.isSkipping = false; | |
| }, 1000); | |
| } else { | |
| // Nothing to seek — don't leave the state machine latched. | |
| this.isSkipping = false; | |
| } | |
| if (!this.isLegacyWebOSVer) { | |
| const timeRemaining = this.video.duration - this.video.currentTime; | |
| if (timeRemaining > 0.5 && this.video.paused) { | |
| this.video.play(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sponsorblock.js` around lines 1154 - 1177, Update the automatic skip flow
around the `isSkipping` assignment and `this.video.currentTime` update to avoid
performing a no-op seek when playback has already passed `jumpTarget`, while
still clearing the skipping state. Add a 500ms `_skipWatchdog` reset matching
`handleBlueButton`, and clear that timer in `destroy()` alongside the other
timers.
| async function enableObserver() { | ||
| if (isObserving) return; | ||
|
|
||
| let appContainer = document.querySelector('ytlr-app'); | ||
|
|
||
| if (!appContainer) { | ||
| try { | ||
| appContainer = await waitForChildAdd( | ||
| document.body, | ||
| (n) => n.nodeName === 'YTLR-APP', | ||
| false, | ||
| null, | ||
| 2000 | ||
| ); | ||
| } catch { | ||
| appContainer = document.body; | ||
| console.warn('[ThumbnailFix] Container not found, using body'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
isObserving is set after an await, so concurrent calls can double-register. If enableObserver() is entered again (e.g. the upgradeThumbnails toggle fires while the initial call is still awaiting waitForChildAdd), the guard at Line 487 hasn't been set yet, and listeners/domObserver.observe run twice.
🛡️ Proposed fix
async function enableObserver() {
if (isObserving) return;
+ isObserving = true;
let appContainer = document.querySelector('ytlr-app');(and drop the later isObserving = true; assignment)
📝 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 enableObserver() { | |
| if (isObserving) return; | |
| let appContainer = document.querySelector('ytlr-app'); | |
| if (!appContainer) { | |
| try { | |
| appContainer = await waitForChildAdd( | |
| document.body, | |
| (n) => n.nodeName === 'YTLR-APP', | |
| false, | |
| null, | |
| 2000 | |
| ); | |
| } catch { | |
| appContainer = document.body; | |
| console.warn('[ThumbnailFix] Container not found, using body'); | |
| } | |
| } | |
| async function enableObserver() { | |
| if (isObserving) return; | |
| isObserving = true; | |
| let appContainer = document.querySelector('ytlr-app'); | |
| if (!appContainer) { | |
| try { | |
| appContainer = await waitForChildAdd( | |
| document.body, | |
| (n) => n.nodeName === 'YTLR-APP', | |
| false, | |
| null, | |
| 2000 | |
| ); | |
| } catch { | |
| appContainer = document.body; | |
| console.warn('[ThumbnailFix] Container not found, using body'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/thumbnail-quality.js` around lines 486 - 504, Set isObserving immediately
at the start of enableObserver, before any await or DOM lookup, so concurrent
calls return without registering duplicate listeners or observers. Remove the
later isObserving assignment while preserving the existing initialization flow.
| } else if (typeof contentTarget === 'object') { | ||
| const { intent, intentParam } = contentTarget; | ||
| const search = ytURL.searchParams; | ||
| const voiceContentIntent = intent | ||
| .match(CONTENT_INTENT_REGEX)?.[0] | ||
| ?.toLowerCase(); | ||
|
|
||
| search.set('inApp', true); | ||
| search.set('vs', 9); | ||
| if (voiceContentIntent) search.set('va', voiceContentIntent); | ||
| search.append('launch', 'voice'); | ||
| if (voiceContentIntent === 'search') search.append('launch', 'search'); | ||
| search.set('vq', intentParam); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
contentTarget object branch can throw. typeof null === 'object', and intent may be absent — both make the destructure/intent.match(...) blow up inside handleLaunch, aborting navigation entirely.
🛡️ Proposed fix
- } else if (typeof contentTarget === 'object') {
- const { intent, intentParam } = contentTarget;
+ } else if (contentTarget && typeof contentTarget === 'object') {
+ const { intent, intentParam } = contentTarget;
const search = ytURL.searchParams;
- const voiceContentIntent = intent
- .match(CONTENT_INTENT_REGEX)?.[0]
+ const voiceContentIntent = intent
+ ?.match(CONTENT_INTENT_REGEX)?.[0]
?.toLowerCase();📝 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.
| } else if (typeof contentTarget === 'object') { | |
| const { intent, intentParam } = contentTarget; | |
| const search = ytURL.searchParams; | |
| const voiceContentIntent = intent | |
| .match(CONTENT_INTENT_REGEX)?.[0] | |
| ?.toLowerCase(); | |
| search.set('inApp', true); | |
| search.set('vs', 9); | |
| if (voiceContentIntent) search.set('va', voiceContentIntent); | |
| search.append('launch', 'voice'); | |
| if (voiceContentIntent === 'search') search.append('launch', 'search'); | |
| search.set('vq', intentParam); | |
| } | |
| } else if (contentTarget && typeof contentTarget === 'object') { | |
| const { intent, intentParam } = contentTarget; | |
| const search = ytURL.searchParams; | |
| const voiceContentIntent = intent | |
| ?.match(CONTENT_INTENT_REGEX)?.[0] | |
| ?.toLowerCase(); | |
| search.set('inApp', true); | |
| search.set('vs', 9); | |
| if (voiceContentIntent) search.set('va', voiceContentIntent); | |
| search.append('launch', 'voice'); | |
| if (voiceContentIntent === 'search') search.append('launch', 'search'); | |
| search.set('vq', intentParam); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils.js` around lines 298 - 311, Harden the object branch in
handleLaunch before destructuring or calling match: require contentTarget to be
non-null and ensure intent is a string, while preserving the existing URL
parameter updates for valid targets. Skip this branch safely when contentTarget
is null or intent is absent/invalid so navigation does not throw.
|
|
||
| return true; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Returning true while the 500ms check is still pending disconnects the observer. initSearchHistoryFix treats true as "fixed" and disconnects, so when the deferred attempt later clears historyCheckPending on failure, nothing ever re-invokes attemptSearchHistoryFix. The pending-flag cleanup only helps if some other caller retries. Consider returning false while pending (and letting the historyCache short-circuit stop the observer once population actually succeeds).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/yt-fixes.js` around lines 113 - 118, Update the pending-check branch in
attemptSearchHistoryFix to return false while the 500ms deferred attempt is
still pending, so initSearchHistoryFix keeps the observer connected. Preserve
the historyCache success short-circuit, allowing the observer to stop only after
cache population actually succeeds.
The merge in f92e177 truncated the last ~93 lines of src/ui.js, taking the base-branch tail wholesale. Two features were lost: * initAudioOnlyToggle() and its `case 'audio_only'` shortcut branch. config.js still advertises `audio_only: 'Toggle Audio-Only Mode'` in the shortcut picker, so the option was selectable but fell through to the switch's default and only logged "Unknown action". * applyUIFixes(), the <body> class observer that strips `app-quality-root`. It was never called after the merge. Restores both, along with the `requireElement` import from player_api/helpers that the audio-only code depends on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The merge added listener-count tracking to FetchRegistry by overriding addEventListener/removeEventListener with `(type: any, callback: any, options?: any)`. Because the override is what consumers resolve against, it erased the typed EventMap that CustomEventTarget<EventMap> provides — which is why block-webos-cast.ts had to annotate its handler `(evt: any)` to compile. Types the overrides generically over `keyof EventMap` instead, which restores inference at every call site and lets block-webos-cast.ts drop its `any`. The now-redundant `type === 'request' || type === 'response'` runtime guards go away too: the generic makes those the only possible values. Exports EventListenerArg from custom-event-target.ts so the override can name the base signature's callback type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `version` npm script still invoked `node tools/sync-version.cjs`, but that file no longer exists — webosbrew removed it in 863ec56 ("build: generate appinfo.json version from package.json") once webpack's transformAppInfo started injecting the version into dist/appinfo.json at copy time. The merge kept the base branch's script while taking the upstream file deletion, so any `npm version` / `pnpm version` bump failed with MODULE_NOT_FOUND. Removes the script rather than resurrecting the file, matching upstream's build. Also restores the commented-out `//import './perf_mon.js'` line. The merge added src/perf_mon.js (264 lines) but dropped the comment that documents it as an opt-in debug tool, leaving the file with no reference anywhere in the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`pnpm lint:prettier` (and therefore `lint:all`) has been red on this branch because these nine files were never run through prettier. Formatting only — no behavioural change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The committed bundle was out of sync with src/. Evidence: HEAD's dist/webOSUserScripts/userScript.js contained the `app-quality-root` string from applyUIFixes even though HEAD's src/ui.js no longer defined that function — so the checked-in artifact had been built from pre-merge source. dist/ is listed in .gitignore yet its files are tracked, so routine `git add` skips them and rebuilds are easy to forget. Worth resolving separately: either untrack dist/ and publish artifacts from CI, or drop the .gitignore entry so builds are staged normally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/emoji-font.css`:
- Around line 23-38: Move the Google Fonts `@import` to the beginning of the
stylesheet before all style rules, then update the font-family declarations to
use the project’s required quoting convention for single-word font names and
correct the text-rendering value casing to satisfy Stylelint. Preserve the
existing selectors and fallback order.
In `@src/ui.js`:
- Around line 1976-2005: Remove the MutationObserver setup from applyUIFixes,
including the bodyClassCallback logic that removes app-quality-root, and remove
the applyUIFixes() startup call. Ensure the app-quality-root class remains on
the body for injected UI rules.
- Around line 1935-1939: Update the showNotification call in the Audio-Only mode
toggle to match the supported notification API: either extend showNotification
in notifications.js to accept and apply an explicit blue variant, or remove the
unsupported third 'blue' argument while preserving the existing text and
duration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b254e7b-259d-461e-aa1b-b3b2be12fbac
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/webOSUserScripts/userScript.jsis excluded by!**/dist/**
📒 Files selected for processing (15)
.github/workflows/build_repo.ymlbabel.config.jspackage.jsonsrc/Sponsorblock-UI.jssrc/block-webos-cast.tssrc/custom-event-target.tssrc/emoji-font.csssrc/emoji-font.jssrc/hooks/fetch.tssrc/index.jssrc/notifications.jssrc/sponsorblock-ui.csssrc/ui.jssrc/userScript.tssrc/webos-utils.js
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/webos-utils.js
- src/hooks/fetch.ts
- src/userScript.ts
- src/custom-event-target.ts
- babel.config.js
| @import url('https://fonts.googleapis.com/css2?family=Noto+Sans+Arabic&family=Noto+Sans+Math&display=swap'); | ||
|
|
||
| html.ytaf-legacy-emoji yt-formatted-string, | ||
| html.ytaf-legacy-emoji yt-core-attributed-string, | ||
| html.ytaf-legacy-emoji .yt-tv-text, | ||
| html.ytaf-legacy-emoji .video-title, | ||
| html.ytaf-legacy-emoji .title, | ||
| html.ytaf-legacy-emoji #title, | ||
| html.ytaf-legacy-emoji .description, | ||
| html.ytaf-legacy-emoji #description, | ||
| html.ytaf-legacy-emoji .video-title-text, | ||
| html.ytaf-legacy-emoji .badge-text { | ||
| font-family: | ||
| 'Roboto', 'YouTube Noto', 'YouTube Sans', 'Noto Sans Arabic', 'Arial', | ||
| 'Noto Sans Math', sans-serif !important; | ||
| text-rendering: optimizeLegibility !important; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the font import before all style rules and fix the remaining Stylelint errors.
Line 23’s late @import is invalid and may be ignored, so the legacy font fallback is not loaded. The quoted single-word font names and optimizeLegibility casing also leave this file failing the reported lint rules.
Proposed fix
+@import 'https://fonts.googleapis.com/css2?family=Noto+Sans+Arabic&family=Noto+Sans+Math&display=swap';
+
img.emoji {
height: 1em;
width: 1em;
}
-@import url('https://fonts.googleapis.com/css2?family=Noto+Sans+Arabic&family=Noto+Sans+Math&display=swap');
-
html.ytaf-legacy-emoji yt-formatted-string,
...
font-family:
- 'Roboto', 'YouTube Noto', 'YouTube Sans', 'Noto Sans Arabic', 'Arial',
+ Roboto, 'YouTube Noto', 'YouTube Sans', 'Noto Sans Arabic', Arial,
'Noto Sans Math', sans-serif !important;
- text-rendering: optimizeLegibility !important;
+ text-rendering: optimizelegibility !important;
}🧰 Tools
🪛 Stylelint (17.14.0)
[error] 36-36: Expected no quotes around "Roboto" (font-family-name-quotes)
(font-family-name-quotes)
[error] 36-36: Expected no quotes around "Arial" (font-family-name-quotes)
(font-family-name-quotes)
[error] 23-23: Expected "url('https://fonts.googleapis.com/css2?family=Noto+Sans+Arabic&family=Noto+Sans+Math&display=swap')" to be "'https://fonts.googleapis.com/css2?family=Noto+Sans+Arabic&family=Noto+Sans+Math&display=swap'" (import-notation)
(import-notation)
[error] 23-23: Invalid position for @import rule (no-invalid-position-at-import-rule)
(no-invalid-position-at-import-rule)
[error] 38-38: Expected "optimizeLegibility" to be "optimizelegibility" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/emoji-font.css` around lines 23 - 38, Move the Google Fonts `@import` to
the beginning of the stylesheet before all style rules, then update the
font-family declarations to use the project’s required quoting convention for
single-word font names and correct the text-rendering value casing to satisfy
Stylelint. Preserve the existing selectors and fallback order.
Source: Linters/SAST tools
| `Audio-Only mode: ${audioOnlyEnabled ? 'Enabled' : 'Disabled'}`, | ||
| 2000, | ||
| 'blue' | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a supported notification API for the blue variant.
showNotification only accepts (text, time), so Line 1938’s 'blue' argument is ignored. Add an explicit variant parameter to src/notifications.js or remove the argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui.js` around lines 1935 - 1939, Update the showNotification call in the
Audio-Only mode toggle to match the supported notification API: either extend
showNotification in notifications.js to accept and apply an explicit blue
variant, or remove the unsupported third 'blue' argument while preserving the
existing text and duration behavior.
| try { | ||
| const bodyClasses = document.body.classList; | ||
|
|
||
| const observer = new MutationObserver(function bodyClassCallback( | ||
| _records, | ||
| _observer | ||
| ) { | ||
| try { | ||
| if (bodyClasses.contains('app-quality-root')) { | ||
| bodyClasses.remove('app-quality-root'); | ||
| } | ||
| } catch (e) { | ||
| console.error('error in <body> class observer callback:', e); | ||
| } | ||
| }); | ||
|
|
||
| observer.observe(document.body, { | ||
| subtree: false, | ||
| childList: false, | ||
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| characterData: false | ||
| }); | ||
| } catch (e) { | ||
| console.error('error setting up <body> class observer:', e); | ||
| } | ||
| } | ||
|
|
||
| applyUIFixes(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not remove app-quality-root from <body>.
Lines 1985-1987 reinstate the behavior that breaks injected UI rules targeting .app-quality-root. Remove this observer and its startup call so the required class persists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui.js` around lines 1976 - 2005, Remove the MutationObserver setup from
applyUIFixes, including the bodyClassCallback logic that removes
app-quality-root, and remove the applyUIFixes() startup call. Ensure the
app-quality-root class remains on the body for injected UI rules.
This PR merges the latest changes from the original repository (
webosbrew/main) into this fork, resolving all conflicts while carefully preserving all custom fork features and addressing automated code review feedback.Merged and Preserved Changes
audio_only) in config mapping, while integrating theinitAudioOnlyToggle()launcher inui.js.pnpm run buildandpnpm run build:modernfor modern/legacy targets).Refinements from PR Reviews (Copilot & CodeRabbit)
pnpm, removedcontinue-on-error: truefrom the CI build job, and disabled checkout credential persistence (persist-credentials: false).nullvideo IDs during unloads.setTimeouton watch destruction to prevent background timer leaks.0msto50mson TV hook acquisition to avoid thrashing low-power TV CPUs.no-unused-varsand arrow callback return warnings to pass ESLint verification.Post-merge audit of the 0.8.1 merge (
f92e177)Merging release 0.8.1 from
NicholasBly/mainback into this branch resolved 15 conflicting files by hand. The resolution was audited by diffing the committed merge against the treegit merge-treeproduces automatically, isolating every place the hand resolution deviated from the mechanical merge. Five follow-up commits fix what that surfaced.Verified correct
adblock.jslosingstripTrackingParamsis intentional, not a regression — 0.8.1 folded it into a single unified response walker.spatial-navigation.modern.js,utils.js,video-quality.js,perf_mon.js,auto-login.jsandscreensaver-fix.jsare behaviour-preserving. They are Prettier reformatting plus lint fixes (prefer-const,no-lonely-if,no-useless-assignment,catch (e)→catch). The deadcontainerstores removed fromnavigate()were overwritten unconditionally two lines later, and thereturndropped from thecontainerActionchain was the last statement in the function.Fixed in this update
9cadf62src/ui.js, droppinginitAudioOnlyToggle(), itscase 'audio_only'branch, and therequireElementimport.config.jsstill listedaudio_only: 'Toggle Audio-Only Mode'in the shortcut picker, so the option remained selectable but fell through to the switch default and only loggedUnknown action. The same truncation droppedapplyUIFixes(), the<body>observer that stripsapp-quality-root.83ca59aFetchRegistryhad lost its typed event map. The listener-count tracking added by the merge overrodeaddEventListener/removeEventListeneras(type: any, callback: any, options?: any), erasing inference for every consumer — which is whyblock-webos-cast.tsneeded(evt: any)to compile. Both overrides are now generic overkeyof EventMap, and theanyis gone.695b8b5pnpm versionwas broken. Theversionscript still callednode tools/sync-version.cjs, but the merge took upstream's deletion of that file (removed in863ec56, once webpack began injecting the version at copy time). Removed the obsolete script rather than resurrecting the file. Also restored the//import './perf_mon.js'comment — the merge addedperf_mon.js(264 lines) while dropping the only thing that referenced it.528094apnpm lint:allwas red. Nine files had never been run through Prettier. Formatting only.4e9fdb7dist/was stale. The checked-in bundle contained theapp-quality-rootstring fromapplyUIFixeseven thoughsrc/ui.jsno longer defined it — the artifact had been built from pre-merge source. Rebuilt.Verification
tsc -b,eslint,prettier --check .andes-checkall pass, andwebpack --mode=productionbuilds clean. The restored audio-only andapplyUIFixescode is confirmed present in the rebuilt bundle.Known issues left open
src/video-quality.ts,src/thumbnail-quality.ts,src/player_api/manager.tsandsrc/player_api/index.tsare webosbrew's TypeScript rewrites, but nothing imports them — the extension-explicit imports (./video-quality.js,./thumbnail-quality.js) resolve to this fork's.jsversions, which carry fork-specific features the TS ones lack. They are inert, not broken; left in place deliberately so the decision on whether to migrate to the TS player API stays with the maintainer. (player_api/helpers.tsis live again as of9cadf62.)dist/is in.gitignoreyet tracked. Routinegit addskips it andlint-stagedfails when re-adding it, which is how the bundle went stale. Worth either untrackingdist/and publishing from CI, or dropping the.gitignoreentry..gitattributesdeclares* text=auto, but nine text files are committed with CRLF (src/index.js,src/index.html,src/app_api/index.ts,src/domrect-polyfill.js,src/emoji-font.*,src/Sponsorblock-UI.js,src/spatial-navigation-polyfill.js,src/icons/*.svg). This is what made commita584611look like a 1,163-line change whengit show --ignore-all-spaceshows it changed nothing at all. A standalonegit add --renormalize .commit would settle it; it was kept out of this PR to avoid ~4,000 lines of unreviewable churn.🤖 Generated with Claude Code
Summary by CodeRabbit