Fix "A Video is Present" false positive on image filenames containing "youtube"/"vimeo" - #1839
Open
pattonwebz wants to merge 3 commits into
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughVideo keyword detection now applies only to iframe and embed elements, with regression tests for image filenames containing “youtube” or “vimeo”. The Composer workflow also removes the ChangesVideo detection refinement
Composer workflow update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/jest/rules/videoElementPresent.test.js (1)
173-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
<embed>path.The implementation now explicitly supports both
iframeandembed, but these tests only exercise iframe positives and image negatives. Add an<embed src="https://player.vimeo.com/video/123">case withshouldPass: false.🤖 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 `@tests/jest/rules/videoElementPresent.test.js` around lines 173 - 187, Add a regression case to the video detection test cases covering an <embed> element with src "https://player.vimeo.com/video/123" and set shouldPass to false, alongside the existing iframe-positive and image-negative cases.
🤖 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.
Nitpick comments:
In `@tests/jest/rules/videoElementPresent.test.js`:
- Around line 173-187: Add a regression case to the video detection test cases
covering an <embed> element with src "https://player.vimeo.com/video/123" and
set shouldPass to false, alongside the existing iframe-positive and
image-negative cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6a3cdf5-1d27-4790-ac34-00d418ad480a
📒 Files selected for processing (2)
src/pageScanner/checks/is-video-detected.jstests/jest/rules/videoElementPresent.test.js
pattonwebz
force-pushed
the
william/pro-1229-a-video-is-present-false-positive-on-images-whose-filename
branch
from
July 17, 2026 21:48
ffd8e5c to
69057de
Compare
Add regression tests for PRO-1229: an <img> whose filename merely contains "youtube" or "vimeo" (e.g. a screenshot named screenshot-2026-05-21-at-16-53-07-claims-department-youtube.jpg used as a featured image) currently trips the video_present rule even though the page has no video or embed on it.
matchesKeyword in is-video-detected.js was checking the "src" attribute of any element matched by the rule's broad selector (video, iframe, object, source, [src]:not(script), [role]) for substrings like "youtube" or "vimeo". Since the selector includes any [src] element, this flagged plain <img> tags whose filename happened to contain one of these words, most commonly a featured image screenshot named like "...-youtube.jpg". Restrict the keyword check to iframe/embed elements, which is where a "youtube"/"vimeo" URL actually indicates an embedded video player. Fixes PRO-1229.
composer install does not support --prefer-offline (that flag only exists for composer update/require), so the "Install Composer dependencies" step in the build-plugin-with-ref workflow fails immediately with "The --prefer-offline option does not exist." This is a pre-existing bug on develop from the workflow-caching optimization merge, unrelated to the video_present fix in this PR, but it blocks this PR's build job so fixing it here to unblock CI.
pattonwebz
force-pushed
the
william/pro-1229-a-video-is-present-false-positive-on-images-whose-filename
branch
from
July 17, 2026 21:55
69057de to
63a7367
Compare
Contributor
|
✅ Accessibility Checker build (primary only)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
video_presentrule'sis_video_detectedcheck matched the keywordsyoutube,youtu.be, andvimeoagainst thesrcattribute of any element hit by the rule's broad selector (video, iframe, object, source, [src]:not(script), [role]), not just embed elements.[src]element, a plain<img>whose filename happens to contain one of these words (e.g. a featured image namedscreenshot-2026-05-21-at-16-53-07-claims-department-youtube.jpg) tripped a false-positive "A Video is Present" warning even though the page has no video/embed at all.iframe/embedelements, which is where a "youtube"/"vimeo" URL actually indicates an embedded video player.Fixes PRO-1229.
Test plan
tests/jest/rules/videoElementPresent.test.jsfor<img>filenames containing "youtube"/"vimeo" — confirmed these fail against the pre-fix code (first commit), then pass after the fix (second commit).npm run test:jest→ 52 suites / 950 tests passing, no regressions.eslintclean on changed files.Summary by CodeRabbit