feat: add x-markdown component for rendering markdown content with st…#2190
feat: add x-markdown component for rendering markdown content with st…#2190PupilTong wants to merge 6 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d9fb103 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new XMarkdown web component (parser, incremental renderer, markdown-style CSS injection, link/image events), exposes it via package exports, updates templates and CSS imports, and adds Playwright tests and HTML fixtures plus test-run guidance. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-elements/src/elements/htmlTemplates.ts (1)
89-98:⚠️ Potential issue | 🟠 MajorRegex with global flag causes inconsistent behavior in
.test()calls.The
XSSDetectorregex uses the global flag (/g), which maintainslastIndexstate between.test()calls. This causes alternating true/false results for consecutive matching inputs on the same regex instance.🐛 Proposed fix: remove the global flag
-const XSSDetector = /*#__PURE__*/ /<\s*script/g; +const XSSDetector = /*#__PURE__*/ /<\s*script/i;The
iflag (case-insensitive) is more useful here for catching<SCRIPTvariants, while removinggeliminates the stateful behavior issue.
🤖 Fix all issues with AI agents
In `@packages/web-platform/web-elements/package.json`:
- Around line 61-65: Add the missing public API import for the XMarkdown element
by editing src/elements/all.ts and following the existing pattern: add an import
statement for './XMarkdown/index.js' alongside the other element imports so
XMarkdown is bundled and exported; ensure the import path matches the
package.json export entry and that it is placed with the other element imports
in src/elements/all.ts.
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`:
- Around line 192-208: The markdown HTML returned by
markdownParser.render(this.#content) is injected directly into the DOM in
`#render`(), risking XSS if `#content` is user-controlled; update `#render`() to
sanitize the rendered HTML before assigning to root.innerHTML (e.g., pass the
output through a sanitizer like DOMPurify or use the markdown parser's
safe/render-to-fragment APIs), or render into a DocumentFragment and sanitize
links/attributes, and ensure all uses of markdownParser.render and assignments
to root.innerHTML are replaced with the sanitized/safer output path.
In `@packages/web-platform/web-elements/src/types/markdown-it.d.ts`:
- Line 1: Remove the empty ambient module declaration "declare module
'markdown-it';" which overrides the installed `@types/markdown-it` and causes all
imports to be typed as any; either delete this declaration file entirely or
replace it with a proper re-export of the upstream types (e.g., export * from
'markdown-it' and export {default} from 'markdown-it') so imports pick up the
correct typings from `@types/markdown-it`.
🧹 Nitpick comments (2)
packages/web-platform/web-elements/src/elements/htmlTemplates.ts (1)
409-411: Inconsistent template pattern:templateXSvgis now a function returning a string.Other templates are either string constants or functions with parameters (like
templateXImage). This parameterless function wrapper seems unnecessary and inconsistent with the pattern used elsewhere.♻️ Suggested simplification
-export const templateXSvg = /*#__PURE__*/ () => { - return `<img part="img" alt="" loading="lazy" id="img" /> `; -}; +export const templateXSvg = /*#__PURE__*/ `<img part="img" alt="" loading="lazy" id="img" /> `;packages/web-platform/web-elements/src/elements/XMarkdown/x-markdown.css (1)
6-11: Consider wiring Lynx linear-layout CSS variables.If
x-markdownshould participate in Lynx linear layout, expose the custom properties so the engine can override layout without hard-coded flex.♻️ Suggested tweak
x-markdown { + --lynx-display: linear; + --lynx-linear-orientation: column; + --lynx-linear-weight: 0; + --lynx-linear-weight-sum: 0; + --lynx-linear-weight-basis: auto; display: flex; flex-direction: column; align-items: stretch; color: inherit; }As per coding guidelines, Use custom CSS properties to control linear layout behavior:
--lynx-display: linear,--lynx-linear-orientation,--lynx-linear-weight,--lynx-linear-weight-sum, and--lynx-linear-weight-basis.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`:
- Around line 160-165: The dispose() method currently removes click listeners
but doesn't cancel any pending append flush timers, which can still run and
modify the shadow DOM after disconnect; update dispose() to clear any scheduled
append timers (e.g., clearTimeout/clearInterval on the append flush handle(s)
used elsewhere in this class — look for symbols like appendFlushTimer,
`#appendFlushTimer`, scheduleAppendFlush, flushAppends or similar), null out or
reset those timer fields, and ensure no further flush is attempted (set any
pending flags to false) before setting `#eventsAttached` = false and returning.
| dispose() { | ||
| if (!this.#eventsAttached) return; | ||
| const root = this.#root(); | ||
| root.removeEventListener('click', this.#handleClick); | ||
| this.#eventsAttached = false; | ||
| } |
There was a problem hiding this comment.
Clear pending append timers on dispose to avoid post-disconnect DOM writes.
A scheduled append flush can still fire after dispose() and touch the shadow DOM.
🧹 Proposed fix
dispose() {
if (!this.#eventsAttached) return;
+ this.#clearAppendFlushTimer();
+ this.#appendRemainder = '';
+ this.#pendingRender = false;
const root = this.#root();
root.removeEventListener('click', this.#handleClick);
this.#eventsAttached = false;
}🤖 Prompt for AI Agents
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`
around lines 160 - 165, The dispose() method currently removes click listeners
but doesn't cancel any pending append flush timers, which can still run and
modify the shadow DOM after disconnect; update dispose() to clear any scheduled
append timers (e.g., clearTimeout/clearInterval on the append flush handle(s)
used elsewhere in this class — look for symbols like appendFlushTimer,
`#appendFlushTimer`, scheduleAppendFlush, flushAppends or similar), null out or
reset those timer fields, and ensure no further flush is attempted (set any
pending flags to false) before setting `#eventsAttached` = false and returning.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`:
- Line 6: CI fails because dompurify lacks bundled TypeScript declarations; add
the dev type package and update lockfile by installing `@types/dompurify` for the
`@aspect/web-elements` package (this will satisfy the import createDOMPurify in
XMarkdownAttributes.ts). Run: pnpm add -D `@types/dompurify` --filter
`@aspect/web-elements` and commit the updated package.json and pnpm-lock.yaml.
- Around line 48-52: The sanitizeHtml helper currently returns the raw value
when getHtmlSanitizer() is null (fail-open); change it to fail-closed by
returning a safe fallback (e.g., an empty string or an escaped/plain-text
representation) when getHtmlSanitizer() returns null. Update the sanitizeHtml
function (referenced by name) to check the sanitizer result and, instead of
returning the unsanitized value, return the chosen safe fallback so no raw HTML
is ever passed through if sanitizer initialization fails.
| // Licensed under the Apache License Version 2.0 that can be found in the | ||
| // LICENSE file in the root directory of this source tree. | ||
| */ | ||
| import createDOMPurify from 'dompurify'; |
There was a problem hiding this comment.
Install type declarations for dompurify to fix CI failure.
The CI check is failing because dompurify doesn't bundle TypeScript declarations. Install @types/dompurify as a dev dependency.
pnpm add -D `@types/dompurify` --filter `@aspect/web-elements`🧰 Tools
🪛 GitHub Check: code-style-check
[failure] 6-6:
Could not find a declaration file for module 'dompurify'. '/home/runner/_work/lynx-stack/lynx-stack/node_modules/.pnpm/dompurify@3.0.8/node_modules/dompurify/dist/purify.cjs.js' implicitly has an 'any' type.
🤖 Prompt for AI Agents
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`
at line 6, CI fails because dompurify lacks bundled TypeScript declarations; add
the dev type package and update lockfile by installing `@types/dompurify` for the
`@aspect/web-elements` package (this will satisfy the import createDOMPurify in
XMarkdownAttributes.ts). Run: pnpm add -D `@types/dompurify` --filter
`@aspect/web-elements` and commit the updated package.json and pnpm-lock.yaml.
| const sanitizeHtml = (value: string) => { | ||
| const sanitizer = getHtmlSanitizer(); | ||
| if (!sanitizer) return value; | ||
| return sanitizer.sanitize(value, { USE_PROFILES: { html: true } }) as string; | ||
| }; |
There was a problem hiding this comment.
Fail-closed on sanitizer initialization failure to prevent XSS.
If getHtmlSanitizer() returns null (due to initialization error), the function currently returns the raw unsanitized value. This fail-open behavior is a security risk—if DOMPurify fails to load, user content would be rendered without sanitization.
🛡️ Proposed fix: fail-closed behavior
const sanitizeHtml = (value: string) => {
const sanitizer = getHtmlSanitizer();
- if (!sanitizer) return value;
+ if (!sanitizer) return ''; // Fail-closed: render nothing if sanitizer unavailable
return sanitizer.sanitize(value, { USE_PROFILES: { html: true } }) as string;
};Alternatively, you could escape the content as plain text, but returning an empty string is safer and more predictable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sanitizeHtml = (value: string) => { | |
| const sanitizer = getHtmlSanitizer(); | |
| if (!sanitizer) return value; | |
| return sanitizer.sanitize(value, { USE_PROFILES: { html: true } }) as string; | |
| }; | |
| const sanitizeHtml = (value: string) => { | |
| const sanitizer = getHtmlSanitizer(); | |
| if (!sanitizer) return ''; // Fail-closed: render nothing if sanitizer unavailable | |
| return sanitizer.sanitize(value, { USE_PROFILES: { html: true } }) as string; | |
| }; |
🤖 Prompt for AI Agents
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`
around lines 48 - 52, The sanitizeHtml helper currently returns the raw value
when getHtmlSanitizer() is null (fail-open); change it to fail-closed by
returning a safe fallback (e.g., an empty string or an escaped/plain-text
representation) when getHtmlSanitizer() returns null. Update the sanitizeHtml
function (referenced by name) to check the sanitizer result and, instead of
returning the unsanitized value, return the chosen safe fallback so no raw HTML
is ever passed through if sanitizer initialization fails.
PupilTong
force-pushed
the
p/hw/support-x-markdown
branch
from
ada5d42 to
543fff4
Compare
Merging this PR will degrade performance by 69.33%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#7919 Bundle Size — 383.64KiB (+0.03%).d9fb103(current) vs d32c4c6 main#7915(baseline) Bundle metrics
|
| Current #7919 |
Baseline #7915 |
|
|---|---|---|
 Initial JS |
154.63KiB |
154.63KiB |
 Initial CSS |
35.21KiB(+0.32%) |
35.1KiB |
 Cache Invalidation |
9.15% |
49.49% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
239 |
239 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.98%(-0.33%) |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7919 |
Baseline #7915 |
|
|---|---|---|
| JS |
252.58KiB |
252.58KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.21KiB (+0.32%) |
35.1KiB |
Bundle analysis report Branch PupilTong:p/hw/support-x-markdow... Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/support-x-markdown
branch
from
7a95b9a to
fab3080
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/nasty-lizards-refuse.md:
- Line 7: Fix the typo in the changeset text by replacing the string
"x-markdonw" with "x-markdown" in the .changeset/nasty-lizards-refuse.md file
(search for the exact token "x-markdonw" and update it to "x-markdown" so the
description reads correctly).
|
|
||
| feat: add x-markdown support | ||
|
|
||
| The x-markdonw is under opt-in importing pattern. |
There was a problem hiding this comment.
Fix typo: “x-markdonw” → “x-markdown”.
Minor spelling issue in the changeset description.
✍️ Proposed fix
-The x-markdonw is under opt-in importing pattern.
+The x-markdown is under opt-in importing pattern.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The x-markdonw is under opt-in importing pattern. | |
| The x-markdown is under opt-in importing pattern. |
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Ensure spelling is correct
Context: ...-- feat: add x-markdown support The x-markdonw is under opt-in importing pattern. ```...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In @.changeset/nasty-lizards-refuse.md at line 7, Fix the typo in the changeset
text by replacing the string "x-markdonw" with "x-markdown" in the
.changeset/nasty-lizards-refuse.md file (search for the exact token "x-markdonw"
and update it to "x-markdown" so the description reads correctly).
…ing error handling
PupilTong
force-pushed
the
p/hw/support-x-markdown
branch
from
62d3190 to
d9fb103
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.changeset/nasty-lizards-refuse.md (1)
7-7:⚠️ Potential issue | 🟡 MinorFix typo in changeset description.
x-markdonwshould bex-markdown.✍️ Proposed fix
-The x-markdonw is under opt-in importing pattern. +The x-markdown is under opt-in importing pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/nasty-lizards-refuse.md at line 7, Typo in the changeset description: replace the misspelled token "x-markdonw" with the correct "x-markdown" in the .changeset/nasty-lizards-refuse.md file so the opt-in import pattern description reads correctly; locate the phrase "x-markdonw" in the file and update it to "x-markdown".packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts (2)
177-182:⚠️ Potential issue | 🟡 MinorClear pending append work during dispose.
dispose()should also cancel pending append timers/state to avoid post-disconnect DOM writes.🧹 Proposed fix
dispose() { if (!this.#eventsAttached) return; + this.#clearAppendFlushTimer(); + this.#appendRemainder = ''; + this.#pendingRender = false; const root = this.#root(); root.removeEventListener('click', this.#handleClick); this.#eventsAttached = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts` around lines 177 - 182, The dispose() method currently only removes click listeners and flips `#eventsAttached`; extend it to also cancel any pending append work by clearing any append timer and queued state: if you use an internal timer field (e.g. this.#appendTimer) call clearTimeout(this.#appendTimer) and set it to undefined, clear any append queue (e.g. this.#appendQueue = []) and reset any scheduled flag (e.g. this.#appendScheduled = false), and if you have a helper like `#cancelPendingAppend`(), invoke it from dispose(); ensure you still call `#root`().removeEventListener('click', this.#handleClick) and set `#eventsAttached` = false after cleaning up.
43-46:⚠️ Potential issue | 🟠 MajorDo not fail open when sanitizer is unavailable.
If sanitizer resolution fails, returning raw HTML reintroduces XSS risk. Use a safe fallback instead of raw passthrough.
🛡️ Proposed fix
const sanitizeHtml = (value: string) => { const sanitizer = getHtmlSanitizer(); - if (!sanitizer) return value; + if (!sanitizer) return ''; return sanitizer.sanitize(value, { USE_PROFILES: { html: true } }) as string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts` around lines 43 - 46, The current sanitizeHtml function returns raw HTML when getHtmlSanitizer() yields null, reintroducing XSS risk; change sanitizeHtml so it never returns raw input: if sanitizer is available call sanitizer.sanitize(value, { USE_PROFILES: { html: true } }), otherwise apply a safe fallback such as an HTML-escaping helper (e.g. escapeHtml(value)) or a sanitizer-less safe-strip (remove all tags / return an empty string) and optionally log a warning; update or add the escapeHtml helper and ensure sanitizeHtml and any callers use it instead of returning value when sanitizer === null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web-platform/web-elements/index.css`:
- Line 16: The CSS import uses url(...) notation which violates the stylelint
rule; replace the `@import` url("./src/elements/XMarkdown/x-markdown.css");
statement with the direct string form `@import`
"./src/elements/XMarkdown/x-markdown.css"; so the import uses a plain string
instead of url(...) (look for the `@import` line referencing x-markdown.css to
update).
---
Duplicate comments:
In @.changeset/nasty-lizards-refuse.md:
- Line 7: Typo in the changeset description: replace the misspelled token
"x-markdonw" with the correct "x-markdown" in the
.changeset/nasty-lizards-refuse.md file so the opt-in import pattern description
reads correctly; locate the phrase "x-markdonw" in the file and update it to
"x-markdown".
In
`@packages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.ts`:
- Around line 177-182: The dispose() method currently only removes click
listeners and flips `#eventsAttached`; extend it to also cancel any pending append
work by clearing any append timer and queued state: if you use an internal timer
field (e.g. this.#appendTimer) call clearTimeout(this.#appendTimer) and set it
to undefined, clear any append queue (e.g. this.#appendQueue = []) and reset any
scheduled flag (e.g. this.#appendScheduled = false), and if you have a helper
like `#cancelPendingAppend`(), invoke it from dispose(); ensure you still call
`#root`().removeEventListener('click', this.#handleClick) and set `#eventsAttached`
= false after cleaning up.
- Around line 43-46: The current sanitizeHtml function returns raw HTML when
getHtmlSanitizer() yields null, reintroducing XSS risk; change sanitizeHtml so
it never returns raw input: if sanitizer is available call
sanitizer.sanitize(value, { USE_PROFILES: { html: true } }), otherwise apply a
safe fallback such as an HTML-escaping helper (e.g. escapeHtml(value)) or a
sanitizer-less safe-strip (remove all tags / return an empty string) and
optionally log a warning; update or add the escapeHtml helper and ensure
sanitizeHtml and any callers use it instead of returning value when sanitizer
=== null.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/nasty-lizards-refuse.md.github/lynx-stack.instructions.mdpackages/web-platform/web-elements/index.csspackages/web-platform/web-elements/package.jsonpackages/web-platform/web-elements/src/elements/XMarkdown/XMarkdown.tspackages/web-platform/web-elements/src/elements/XMarkdown/XMarkdownAttributes.tspackages/web-platform/web-elements/src/elements/XMarkdown/index.tspackages/web-platform/web-elements/src/elements/XMarkdown/x-markdown.csspackages/web-platform/web-elements/src/elements/htmlTemplates.tspackages/web-platform/web-elements/tests/fixtures/shell-project.tspackages/web-platform/web-elements/tests/fixtures/x-markdown/basic.htmlpackages/web-platform/web-elements/tests/fixtures/x-markdown/events.htmlpackages/web-platform/web-elements/tests/fixtures/x-markdown/image.htmlpackages/web-platform/web-elements/tests/fixtures/x-markdown/incremental.htmlpackages/web-platform/web-elements/tests/fixtures/x-markdown/style.html
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/web-platform/web-elements/package.json
- packages/web-platform/web-elements/src/elements/XMarkdown/index.ts
- packages/web-platform/web-elements/tests/fixtures/x-markdown/image.html
- packages/web-platform/web-elements/tests/fixtures/x-markdown/style.html
- packages/web-platform/web-elements/tests/fixtures/x-markdown/basic.html
- packages/web-platform/web-elements/src/elements/XMarkdown/x-markdown.css
- .github/lynx-stack.instructions.md
| @import url("./src/elements/XSvg/x-svg.css"); | ||
| @import url("./src/elements/XImage/x-image.css"); | ||
| @import url("./src/elements/XInput/x-input.css"); | ||
| @import url("./src/elements/XMarkdown/x-markdown.css"); |
There was a problem hiding this comment.
Fix CSS import notation to satisfy stylelint.
Line 16 uses url(...) import notation, but the configured rule expects direct string notation.
🔧 Proposed fix
-@import url("./src/elements/XMarkdown/x-markdown.css");
+@import "./src/elements/XMarkdown/x-markdown.css";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @import url("./src/elements/XMarkdown/x-markdown.css"); | |
| `@import` "./src/elements/XMarkdown/x-markdown.css"; |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 16-16: Expected "url("./src/elements/XMarkdown/x-markdown.css")" to be ""./src/elements/XMarkdown/x-markdown.css"" (import-notation)
(import-notation)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-platform/web-elements/index.css` at line 16, The CSS import uses
url(...) notation which violates the stylelint rule; replace the `@import`
url("./src/elements/XMarkdown/x-markdown.css"); statement with the direct string
form `@import` "./src/elements/XMarkdown/x-markdown.css"; so the import uses a
plain string instead of url(...) (look for the `@import` line referencing
x-markdown.css to update).
1 participant
…yling support
Summary by CodeRabbit
New Features
Tests
Documentation
Packaging
Checklist