PRO-1168: fix: don't flag .ogg audio as video content in video_present rule#1817
Conversation
The Gutenberg Audio block's Ogg Vorbis sample file was being detected as video because .ogg is also a valid Ogg Theora video extension. Only treat a .ogg match as video when it isn't attached to an <audio> element or one of its <source> children. Fixes #1816, PRO-1168. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe video detection check now ignores ChangesOgg Audio Detection Fix
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents .ogg audio files (such as those in Gutenberg Audio blocks) from being incorrectly flagged as video content by ensuring that .ogg matches are only counted as video when they are not associated with an element. Comprehensive Jest tests have been added to verify this behavior. The review feedback suggests valuable performance optimizations, specifically to avoid redundant string lowercasing inside the loop and to restrict DOM parent traversal to only when the element tag is 'source'.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const parentTag = node.parentNode ? node.parentNode.nodeName.toLowerCase() : ''; | ||
| const isInAudioContainer = tag === 'audio' || ( tag === 'source' && parentTag === 'audio' ); | ||
|
|
||
| const matchesExtension = videoExtensions.some( ( ext ) => { | ||
| const srcLower = src.toLowerCase(); | ||
| const dataLower = data.toLowerCase(); | ||
| // Check if the extension is at the end of the string or followed by a query parameter | ||
| return ( | ||
| const matches = ( | ||
| ( srcLower.endsWith( ext ) || srcLower.includes( ext + '?' ) ) || | ||
| ( dataLower.endsWith( ext ) || dataLower.includes( ext + '?' ) ) | ||
| ); | ||
|
|
||
| if ( matches && ext === '.ogg' && isInAudioContainer ) { | ||
| return false; | ||
| } | ||
|
|
||
| return matches; | ||
| } ); |
There was a problem hiding this comment.
Performance & Cleanliness Improvement
- Avoid Unnecessary DOM Traversal: Currently,
node.parentNodeis accessed andtoLowerCase()is called on itsnodeNamefor every evaluated node, even when the node is not a<source>element. We can optimize this by only performing the parent check whentag === 'source'. - Avoid Redundant String Operations:
src.toLowerCase()anddata.toLowerCase()are called inside thevideoExtensions.someloop on every iteration (up to 22 times per node). DefiningsrcLoweranddataLoweronce outside the loop significantly improves performance, especially on pages with many media elements.
Note: You can also update the matchesKeyword check (on line 59) to use the newly defined srcLower variable.
const isInAudioContainer = tag === 'audio' || (
tag === 'source' &&
node.parentNode &&
node.parentNode.nodeName.toLowerCase() === 'audio'
);
const srcLower = src.toLowerCase();
const dataLower = data.toLowerCase();
const matchesExtension = videoExtensions.some( ( ext ) => {
// Check if the extension is at the end of the string or followed by a query parameter
const matches = (
( srcLower.endsWith( ext ) || srcLower.includes( ext + '?' ) ) ||
( dataLower.endsWith( ext ) || dataLower.includes( ext + '?' ) )
);
if ( matches && ext === '.ogg' && isInAudioContainer ) {
return false;
}
return matches;
} );References
- When traversing the DOM tree to check ancestors, use parentNode instead of parentElement if Shadow DOM support is required, as parentElement does not cross shadow boundaries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pageScanner/checks/is-video-detected.js (1)
36-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGeneralize the audio-container exclusion beyond
.ogg.videoExtensionsalso includes.webm, so audio-only WebM sources can still be flagged as video. A small ambiguous-extension list would cover the same false-positive for other dual-purpose containers too.🤖 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/pageScanner/checks/is-video-detected.js` around lines 36 - 56, The audio-container exclusion in is-video-detected should not be limited to .ogg, because videoExtensions also contains other dual-purpose container formats like .webm that can be used for audio-only media. Update the matching logic in is-video-detected.js to use a small ambiguous-extension list and skip matches for any of those extensions when the node is inside an audio element or audio source, keeping the existing parentTag/isInAudioContainer checks and the matchesExtension loop intact.
🤖 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 `@src/pageScanner/checks/is-video-detected.js`:
- Around line 36-56: The audio-container exclusion in is-video-detected should
not be limited to .ogg, because videoExtensions also contains other dual-purpose
container formats like .webm that can be used for audio-only media. Update the
matching logic in is-video-detected.js to use a small ambiguous-extension list
and skip matches for any of those extensions when the node is inside an audio
element or audio source, keeping the existing parentTag/isInAudioContainer
checks and the matchesExtension loop intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44908bda-9963-4fc7-b938-e8bc35c1a347
📒 Files selected for processing (2)
src/pageScanner/checks/is-video-detected.jstests/jest/rules/videoElementPresent.test.js
pattonwebz
changed the title
Only compute node.parentNode's tag when the node is a <source>, and lowercase src/data once instead of on every iteration of the extensions loop, per Gemini code review feedback on PR #1817. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@coderabbitai Thanks for the nitpick on We are intentionally scoping this fix to |
|
Tip For best results, initiate chat on the files or code changes.
If ✏️ Learnings added
|
|
✅ Accessibility Checker build (primary only)
|
1 participant
Summary
.ogg) was being detected as video content by thevideo_presentrule, because.oggis also a valid Ogg Theora video extension..oggstays invideoExtensions(Ogg can still carry Theora video), but a.oggmatch is now only counted as video when it isn't attached to an<audio>element or one of its<source>children.Test plan
<audio src="....ogg">, the realwp-block-audiofigure markup, and a<source>inside<audio>) — confirmed these failed before the fix..oggstill triggers the rule outside an audio container (native<video>, standalone<source>).npx jest --config=./tests/jest/jest.config.js— 847/847 passing, no regressions.Fixes #1816.
🤖 Generated with Claude Code
Summary by CodeRabbit
.oggsources that appear within audio contexts (such as audio elements and audio block structures)..oggwhen it’s used in actual video-related scenarios, including standalone video sources and<video>elements..oggcases to confirm both violations and non-violations across relevant HTML structures.