Fix "bulk delete videos by watch progress" feature

[CJWmort]

Fixes Bug 2 of #4318

What

Fix the bug where setting the deletion threshold down to 5% doesn't do the job. Only if the threshold is set to 0% do the videos get deleted.

Why

Users should be able to properly bulk delete videos that match the selected threshold (E.g, watch progress >= 5% should be deleted)

How

Root cause: YouTube renamed to and removed the #progress id, so getWatchedPercentFromRenderer() always fell through to 0 for any partially-watched video, meaning only threshold=0 ever matched anything.

Note: The package-lock.json changes in this PR are from a local npm install / dependency refresh and aren't functionally related to this fix.

Testing

npx jest tests/unit/playlist-watched-progress.test.js

• 1 test suites passed
• 9 tests passed

npm test

• 25 test suites passed
• 122 tests passed

npm run lint

• passed

[wahajahmed010]

Review: Fix "bulk delete videos by watch progress"

Clean, focused fix for a real DOM-rename regression. A few items:

Selector strategy
The new selector 'ytw-thumbnail-overlay-resume-playback-renderer [class*="ThumbnailOverlayResumePlaybackProgress"], ytd-thumbnail-overlay-resume-playback-renderer #progress' is correct for the bug surface: YouTube's ytw- prefix is the new tag, the ThumbnailOverlayResumePlaybackProgress class is the new progress indicator (no #progress id), and the old #progress lookup is kept as a fallback for users who have not received the rollout. Good defense-in-depth.

One concern: the attribute selector [class*="ThumbnailOverlayResumePlaybackProgress"] is a substring match — it will match ThumbnailOverlayResumePlaybackProgressActive, ThumbnailOverlayResumePlaybackProgressBar, anything that contains the substring. The intent is to find the element with that class, but a stricter selector would be [class~="ThumbnailOverlayResumePlaybackProgress"] (exact class token match) or div.ThumbnailOverlayResumePlaybackProgress. The substring form is more permissive (matches more elements), which in querySelector is fine because only the first match is read, but it would mask future DOM changes that introduce a sibling with the same substring. Worth a comment explaining why substring was preferred over exact match (probably: the class name might get version suffixes like ...ProgressV2 in future rollouts).

return 100 selector
The new 'ytw-thumbnail-overlay-watched-status-renderer, ytd-thumbnail-overlay-watched-status-renderer' keeps the same fallback shape. Solid. Minor: same substring/attribute concern does not apply here because tag names are exact matches.

Whitespace in the diff
Two blank lines added where the diff shows empty diff context lines (+ on a line with only whitespace). The new code has a trailing-space blank line after each new querySelector call. prettier --check passed per the PR description, so this is whatever the repo's prettier config produces — not a bug, just visible noise in the diff. Fine.

Test file: source-string assertions
tests/unit/playlist-watched-progress.test.js does two things:

1. Source-string assertions: expect(sourceContent).toContain(...) and expect(sourceContent).not.toMatch(...) — these lock down the exact selector strings.
2. Behavioral assertions: load the source into a vm sandbox with mocked DOM, call getWatchedPercentFromRenderer, assert returned percentages.

The source-string assertions are the strongest part of this PR. They will fail if anyone "refactors" the selector back to a single-version lookup, even if the behavioral test happens to pass on the current YouTube DOM. Worth keeping in mind: the source-string assertion expect(sourceContent).not.toMatch(/querySelector\('ytw-thumbnail-overlay-resume-playback-renderer #progress'\)/) will break the moment YouTube does restore the #progress id (which is plausible). Consider weakening it to assert only that the selector covers both markup versions, not the specific spelling.

Behavioral test: collectCandidates thresholds
The threshold test (threshold 5% picks up v8, v45, v100 but not v0/v3 (this is the bug being fixed)) is exactly the regression test this PR needs. The fixture setup is clear (v0=0%, v3=3%, v8=8%, v45=45%, v100=fully watched) and the assertion text explicitly references the bug being fixed. Good.

The threshold 0% still matches everything test guards against an over-correction where the fix accidentally narrows the legacy threshold-0 path. Good belt-and-suspenders.

Missing newline at end of file
The new test file is missing a trailing newline (\ No newline at end of file). POSIX text files should end with a newline. Some linters (prettier, eslint) flag this. If the project's prettier config requires it (likely, given npm run lint passed per the PR description), check why this slipped through — could be a .prettierignore covering tests/unit/, could be an endOfLine setting, or could be that the file was written without a final newline. Worth fixing for consistency with the rest of the test suite.

Test sandbox: minimal globals
The vm.createContext approach with ImprovedTube, satus, document.querySelectorAll mocked just enough for playlist-complete-playlist.js to load and expose getWatchedPercentFromRenderer and collectCandidates is good — avoids the jsdom dependency cost while still running the real production code. The __setMockNodes swap-out per beforeEach is a clean way to vary fixtures.

One thing missing: the test does not assert the rendered thumbnail's progress matches what getWatchedPercentFromRenderer returns. If a future refactor decouples these (e.g., DOM update on a different code path), the regression could pass getWatchedPercentFromRenderer but break the user-visible feature. Not a blocker — out of scope for this fix.

package-lock.json churn
The diff shows several entries removed (@ampproject/remapping) and a few + "peer": true annotations added across @babel/code-frame, acorn, caniuse-lite, eslint. This is lockfile drift from a recent npm install, not a change this PR introduced. Two options:

1. Split the lockfile churn into a separate commit (or omit it with npm install --no-save and revert).
2. Leave it in but call it out in the PR description so reviewers know it's noise.

The PR description doesn't mention it. A one-line "lockfile churn from local npm install, no functional impact" note in the description would help future archaeological reviews. Minor — not a blocker.

ImprovedTube and satus references in test sandbox
The mock sandbox references ImprovedTube.storage and ImprovedTube.elements and satus.storage.data / satus.events.trigger. These are real globals from the extension code — the sandbox is shaped exactly like the runtime. Good. Just be aware that any future refactor renaming these globals (ImprovedTube → window.ImprovedTube, etc.) will require test fixture updates too. Worth a // keep in sync with production globals comment.

Otherwise: the fix is correct, the regression coverage is strong, the test design (source-string + behavioral) is exactly what is needed for a DOM-rename regression, and the PR description explains the root cause clearly. Approve once the trailing newline and the lockfile note are addressed.

[wahajahmed010]

Targeted fix with good regression coverage, but a few things worth tightening.

Logic

• The new resume-playback selector 'ytw-thumbnail-overlay-resume-playback-renderer [class*="ThumbnailOverlayResumePlaybackProgress"], ytd-thumbnail-overlay-resume-playback-renderer #progress' works, but the asymmetry is a smell: the ytw branch uses [class*="ThumbnailOverlayResumePlaybackProgress"] while the ytd branch still relies on the id="progress" element. When YouTube eventually drops the id, the same bug recurs. If you have a stable DOM hook in the new markup (a specific class or attribute), prefer an exact selector over [class*=]. If the class is genuinely the only handle, the comment in the source should call out that we are matching on a substring, since attribute-substring selectors are easy to over-match on shared utility classes (ThumbnailOverlayResumePlaybackProgressBarBg, ThumbnailOverlayResumePlaybackProgressLabel, etc., if YouTube adds them) and silently pick up the wrong element.
• The watched-status branch ytw-thumbnail-overlay-watched-status-renderer, ytd-thumbnail-overlay-watched-status-renderer is fine and the comma-grouping is concise.

Robustness

• parsePercentFromStyle(progress.style.width) returns 0 (via Number.isNaN → falls through) when the style.width is "0px", "auto", or empty. That is correct behaviour for the "never played" case, but you may want to log once at debug level when a renderer has a progress element with no usable width — that often means YouTube shipped a new wrapper class and the bulk-delete feature silently regresses for those users. A console.debug would be cheap insurance.
• getWatchedPercentFromRenderer is invoked many times per playlist render. The two querySelector calls are unavoidable, but make sure the surrounding code does not call this in a tight loop without caching. (Looking at the file, collectCandidates is the only consumer and iterates once per render, so it is fine.)

Tests

• The new playlist-watched-progress.test.js covers the bug scenario well: thresholds at 0/5/10% with a mixed playlist, both markup variants, and the regression check that the old broken selector is gone.
• One assertion to flag: expect(sourceContent).not.toMatch(/querySelector\('ytw-thumbnail-overlay-resume-playback-renderer #progress'\)/); only catches the literal broken form. If a future contributor re-introduces the id-only lookup under a slightly different selector string, this test stays green. Consider a more semantic check, e.g. assert that for a renderer using the ytw engine with useYtw=true, getWatchedPercentFromRenderer returns the configured width without falling through to 0. You already have that test ("reads percentage from current (ytw) markup"), so the regex test is the weaker duplicate.
• The sandbox-built renderer fakes querySelector with a string-includes match, which is permissive. Tightening to exact selector equality would catch regressions where a partial-match bug slips in.

Nits

• Trailing whitespace after the new querySelector( call (+\n \n) and after the watched-status line. git diff shows it; the linter should flag it.
• package-lock.json noise: you flag it in the PR description. Consider splitting unrelated lock churn into a separate PR — easier to revert, easier to review, and CI failure modes are clearer. Not blocking.
• The PR title is fine but the body could link the upstream YouTube change or paste a snippet of the new DOM for context. Future archaeologists will thank you.

Overall the fix is correct and the test suite nails the regression. The selector robustness and the redundant regex test are the only items I would push on.

[jonasschulze-dev]

Tested locally:

• All automated tests pass.
• Manually tested in Firefox and Vivaldi using web-ext and verified bulk deletion at thresholds of 5%, 50%, and 100%. Confirmed videos are correctly deleted according to the configured watch-progress threshold.
• No issues observed.