docs: fix markdown links not rendered inside GFM alert HTML blocks#46569
Conversation
… HTML blocks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…Html Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…InHtml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46569 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Adds a remark plugin to render Markdown links and code spans inside opaque HTML blocks.
Changes:
- Adds and registers the HTML-inline Markdown transformer.
- Adds unit tests and a Make target.
- Supports several inline-content HTML tags.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds remark test target. |
docs/astro.config.mjs |
Registers the plugin. |
docs/src/lib/remark/inlineMarkdownInHtml.js |
Implements HTML-content transformation. |
docs/src/lib/remark/inlineMarkdownInHtml.test.js |
Tests transformation behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Medium
| import { unified } from 'unified'; | ||
| import remarkParse from 'remark-parse'; | ||
| import remarkRehype from 'remark-rehype'; | ||
| import rehypeStringify from 'rehype-stringify'; |
| return html.replace(tagRe, (_match, openTag, content, closeTag) => { | ||
| // Skip content that already contains an anchor tag to avoid double-processing. | ||
| if (/<a[\s>]/i.test(content)) return _match; | ||
|
|
||
| const processed = applyMarkdownTransformation(content); | ||
| return openTag + processed + closeTag; | ||
| }); |
| test-docs-remark: | ||
| @echo "Running docs remark plugin unit tests..." | ||
| @node docs/src/lib/remark/inlineMarkdownInHtml.test.js |
| * - inputs are documentation source files committed by repository maintainers | ||
| * and are not derived from end-user input; | ||
| * - no output is injected into an HTTP response or an untrusted context. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: docs fix for markdown links in GFM alert HTML blocks
The approach is sound — using a proper unified pipeline (remark-parse → remark-rehype → rehype-stringify) to transform markdown inside opaque HTML nodes is the right AST-level fix. The implementation is clean, well-documented, and includes a solid test suite covering the key edge cases.
One non-blocking observation left as an inline comment on line 93: the double-processing guard skips the whole tag content when any <a> is found, which could silently leave markdown links unprocessed if an author mixes raw anchors and markdown links in the same <summary>. Worth documenting or tightening the guard.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.4 AIC · ⌖ 7.08 AIC · ⊞ 5K
| * | ||
| * - Only targets known inline-text tags listed in INLINE_TEXT_TAGS. | ||
| * - Text that already contains an `<a` tag is left untouched to avoid | ||
| * double-processing. |
There was a problem hiding this comment.
The double-processing guard (/<a[\s>]/i.test(content)) skips the entire tag content if it contains any existing anchor — including cases where a markdown link appears alongside an already-rendered <a>. In practice this is safe for build-time docs, but if a <summary> ever contains both an authored <a> and a markdown link, the markdown link will silently stay unprocessed. Consider adding a check for remaining [...](...) patterns before short-circuiting, or document this trade-off. @copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 89/100 — Excellent
📊 Metrics (8 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — 3 correctness issues found; no blocking changes but addressing them will make the plugin more robust.
📋 Key Themes & Highlights
Issues
- Tag mismatch in regex (line 88): The tag pattern matches any open tag with any close tag in
INLINE_TEXT_TAGS. A back-reference (\2) would ensure open/close tags are the same element. - Greedy
<p>strip (line 57):applyMarkdownTransformationstrips the outer<p>wrapper with[\s\S]*?which collapses multi-paragraph content — strip leading and trailing<p>independently instead. <aguard documentation (line 97): The skip guard is correct for the current tag set; adding a test for content with<abbr>or similar would document the intended boundary.
Positive Highlights
- ✅ Using a
unifiedpipeline (remark-parse→remark-rehype→rehype-stringify) is the right AST-level fix; previous regex-only approach was fragile - ✅
allowDangerousHtmlrationale is well-documented with the build-time safety justification - ✅ 8 unit tests cover the primary happy paths and the double-processing guard
- ✅
make test-docs-remarktarget makes the tests easy to run in CI
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 28.7 AIC · ⌖ 4.8 AIC · ⊞ 6.7K
Comment /matt to run again
| */ | ||
| const INLINE_TEXT_TAGS = ['summary', 'figcaption', 'caption', 'dt', 'dd', 'th', 'td', 'li']; | ||
|
|
||
| /** |
There was a problem hiding this comment.
[/diagnosing-bugs] The closing-tag regex can match across mismatched open/close tags — e.g. <summary>...</td> would be processed if both tags appear in INLINE_TEXT_TAGS.
💡 Suggested fix
Capture the opening tag name and use a back-reference so only a matching close tag is accepted:
const tagRe = new RegExp(
`(<(${tagPattern})(?:\\s[^>]*)?>)([\\s\\S]*?)(<\\/\\2>)`,
'gi',
);
// and update the replacer: (_match, openTag, _tagName, content, closeTag)Without this, content between an <li> and the next </summary> in a complex HTML block could be incorrectly reprocessed.
@copilot please address this.
| * | ||
| * @param {string} html | ||
| * @returns {string} | ||
| */ |
There was a problem hiding this comment.
[/diagnosing-bugs] The <a skip guard uses /<a[\s>]/i but the regex pattern string uses 'gi' flags, matching case-insensitively — while the guard itself already uses /i. This is fine, but the guard won't catch <A HREF=...> produced by external HTML (capital-A anchor). Both the existing regex and the guard need to be case-insensitive consistently, which they are — just worth a note that /<A[\s>]/i is equivalent and matches both cases.
Actually the real concern: /<a[\s>]/i will also skip content that contains an <abbr> tag (it matches <ab which starts with <a followed by a space... wait, <abbr is <a + b — the pattern <a[\s>] requires <a followed by whitespace or >, so <abbr> won't match). This is actually fine — just confirm the guard is intentional and add a test for <summary> content containing <abbr> to document the behaviour.
@copilot please address this.
| /** | ||
| * @returns {(tree: import('unist').Node) => void} | ||
| */ | ||
| export default function remarkInlineMarkdownInHtml() { |
There was a problem hiding this comment.
[/diagnosing-bugs] applyMarkdownTransformation uses a greedy [\s\S]*? regex to strip the outer <p> wrapper, but processSync can emit multiple paragraphs (e.g. if the summary content has a blank line). In that case the result would begin with <p> and end with </p>\n, so the strip regex matches only the first-to-last </p> — potentially stripping intermediate </p><p> boundaries.
💡 Safer approach
Strip only the leading and trailing <p> tags independently:
function applyMarkdownTransformation(text) {
const result = String(inlineProcessor.processSync(text));
return result
.replace(/^<p>/, '')
.replace(/<\/p>\n?$/, '');
}Alternatively, limit inputs to single lines so multi-paragraph summaries are not silently mangled.
@copilot please address this.
There was a problem hiding this comment.
Two correctness bugs need fixing before merge
The regex-based HTML tag matching in processMarkdownInHtml has two defects that can produce invalid or corrupted HTML output.
Blocking issues summary
- Mismatched tag pairs (line 144): the regex does not enforce that the closing tag matches the opening tag name, so
<td>content</th>is matched and rewritten — silently corrupting the HTML structure. - Multi-paragraph
<p>injection (line 94): when tag content contains multiple paragraphs,applyMarkdownTransformationreturns multiple<p>...</p>blocks; the stripping regex does not match and those block-level<p>tags are injected into inline contexts like<summary>— producing invalid HTML.
Both are fixable with small, targeted changes. See inline comments for concrete suggestions.
🔎 Code quality review by PR Code Quality Reviewer · 33.5 AIC · ⌖ 4.86 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
docs/src/lib/remark/inlineMarkdownInHtml.js:144
Mismatched open/close tags are silently processed: the regex allows <td>foo</th> to match because the closing tag is not constrained to match the opening tag name — producing silently corrupted HTML.
<details>
<summary>💡 Suggested fix</summary>
Capture the tag name in a group and use a backreference for the closing tag:
const tagRe = new RegExp(
`(<()(\\s[^>]*)?>)([\\s\\S]*?)(<\\/\\2>)`,
'gi',
);
return html.replace(tagRe, (_match, openTag, _tagName, _attrs, content, closeT…
</details>
<details><summary>docs/src/lib/remark/inlineMarkdownInHtml.js:94</summary>
**Multi-paragraph content produces invalid HTML**: when `content` spans multiple paragraphs, `processSync` emits multiple `<p>...</p>` blocks; the strip regex anchored at start/end fails to match, so the raw `<p>` tags are injected directly into `<summary>` — invalid HTML.
<details>
<summary>💡 Suggested fix</summary>
Strip the outer `<p>` only when the result is exactly a single paragraph, and otherwise return the inner content as-is, or skip multi-paragraph content:
```js
function applyMar…
</details>
|
@copilot run pr-finisher skill |
Completed a PR-finisher pass. No additional code changes were needed for the new @copilot threads in this round. I ran local validation ( |
📸 Visual Regression ReportScreenshots captured for PR #46569 on the docs site (`(localhost/redacted) Viewport Screenshots
Accessibility SnapshotThe page snapshot shows a well-structured document:
Console Warnings7 warnings observed (0 errors):
The media decode warnings are expected in a headless Firefox environment (no system MP4 codecs). Font preload timing warnings are benign in dev mode. Summary✅ No visual regressions detected. The page renders correctly across all three viewport sizes with no accessibility violations. Warning Firewall blocked 5 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "10.1.0.215"
- "aus5.mozilla.org"
- "awmgmcpg"
- "host.docker.internal"
- "runnervm3jd5f.ggcgsnmn1r…dx.internal.cloudapp.net"See Network Configuration for more information.
|
🖼️ Visual Regression CheckScreenshots captured of the docs preview server for PR #46569. Screenshot Status
Accessibility Findings
SummaryNo visual regressions or layout errors detected. The docs page renders correctly at all three tested viewports. Warning Firewall blocked 7 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "172.30.0.1"
- "accounts.google.com"
- "awmgmcpg"
- "clients2.google.com"
- "host.docker.internal"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
CommonMark treats block-level HTML elements (
<details>,<summary>) as opaque — remark-parse never processes markdown syntax inside them. A GFM alert like:…renders
[Step 6](06-install-gh-aw.md)as raw text instead of an anchor, because the<summary>content is an HTML block from remark's perspective.Changes
docs/src/lib/remark/inlineMarkdownInHtml.js— new remark plugin that visitshtmlMDAST nodes and converts[text](url)link syntax within inline-content tags (<summary>,<figcaption>,<caption>,<dt>,<dd>,<th>,<td>,<li>) to<a href="url">text</a>. Guards: skips nodes already containing<a; preserves backtick code spans; rejects nested/unbalanced brackets in link text.docs/astro.config.mjs— registers the plugin in theunifiedprocessor alongside existing remark plugins.docs/src/lib/remark/inlineMarkdownInHtml.test.js— 8 unit tests (single/multiple links, code span preservation, double-processing guard, full<details>/<summary>roundtrip).Makefile— addsmake test-docs-remarktarget.