fix(ci): preserve code while normalizing issue media - #1270
Conversation
📝 WalkthroughWalkthroughThe issue-quality normalizer now protects fenced and indented code during media stripping, tracks HTML media nesting, restores protected content by token, and removes media blocks containing only recognized placeholders. ChangesMedia normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts/issue-quality-core.cjs:
- Around line 127-133: Update htmlMediaDepthDelta and its caller to preserve
media-tag scanning state across lines, counting a picture, video, or audio
opening tag once its eventual closing “>” is encountered even when attributes
span lines, while retaining correct closing-tag handling. Add a regression test
covering a multiline opening tag with indented children and verify media-only
normalization still succeeds.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7dbdba0a-744e-4249-92fe-1b6334f5bb1d
📒 Files selected for processing (2)
.github/scripts/issue-quality-core.cjs.github/scripts/issue-quality.test.cjs
| function htmlMediaDepthDelta(line) { | ||
| let delta = 0; | ||
| for (const match of line.matchAll(/<(\/)?(picture|video|audio)\b[^>]*>/gi)) { | ||
| if (match[1]) delta -= 1; | ||
| else if (!/\/\s*>$/.test(match[0])) delta += 1; | ||
| } | ||
| return delta; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Handle HTML media start tags that span multiple lines.
htmlMediaDepthDelta only detects a media tag when its closing > is on the same line. A valid block with a multiline opening tag leaves mediaDepth at zero.
<video
src="clip.mp4">
<source src="clip.mp4">
</video>The indented attribute and child lines are then masked as Markdown code. stripHtmlMedia cannot remove the resulting tokenized block. isMediaOnly returns false, so media-only issue content bypasses normalization.
Track media-tag state across lines. Set media depth when the scanner sees an opening <picture>, <video>, or <audio> tag. Add a regression test for a multiline opening tag with indented children.
🤖 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/scripts/issue-quality-core.cjs around lines 127 - 133, Update
htmlMediaDepthDelta and its caller to preserve media-tag scanning state across
lines, counting a picture, video, or audio opening tag once its eventual closing
“>” is encountered even when attributes span lines, while retaining correct
closing-tag handling. Add a regression test covering a multiline opening tag
with indented children and verify media-only normalization still succeeds.
|
Thanks for this — the token-based restoration is the right shape, and it fixes the line-position fragility properly. One blocker before it can land, and it is the same one that stopped my own attempt at this: the placeholder check is too broad for media fallback text.
That flips a legitimate submission. A feature request whose example is What I would suggest instead: a media-specific predicate limited to the serialization GitHub actually emits for an unanswered field — Worth adding negative tests for The restoration half I have no concerns about. It is only the one predicate. |
|
One more, found while verifying the first: the depth scanner only recognises a media tag once its
Same content, same indentation; only the opening tag is wrapped. GitHub wraps attributes like this when a reporter pastes an embed from an editor that formats HTML, so it is not a contrived shape. Carrying opening-tag state across lines would cover it. Worth a regression alongside the two-space / four-space / tab cases. Neither this nor the predicate issue touches the restoration work, which is the substantial part of the PR and looks right to me. |
2 participants
Summary
Root cause
The issue-quality normalizer protected every indented line before it identified HTML media blocks. Indented
<source>and<img>children were therefore preserved as Markdown code, keeping otherwise media-only blocks substantive. Restoration also depended on original line indexes even though stripping a multiline media block can remove lines. Separately, placeholder fallback text such as<video>No response</video>was treated as a real caption.The fix distinguishes indented children of an active HTML media block from literal Markdown code, protects fenced and indented examples with collision-resistant ordered tokens, and restores those tokens independently of line count. Only exact placeholder fallback values are removed; real captions remain substantive.
Validation
node --test .github/scripts/issue-quality.test.cjs— 112 passedbun run typecheck— passedbun run privacy:scan— passedgit diff --check— passedCloses #1196
Summary by CodeRabbit