fix: preserve text following colons in markdown rendering#1115
Open
fbricon wants to merge 1 commit intokortex-hub:mainfrom
Open
fix: preserve text following colons in markdown rendering#1115fbricon wants to merge 1 commit intokortex-hub:mainfrom
fbricon wants to merge 1 commit intokortex-hub:mainfrom
Conversation
fbricon
requested review from
MarsKubeX and
benoitf
and removed request for
a team
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a micromark fallback directive handler that preserves unrecognized directive-like text during markdown rendering, wires it into the Markdown renderer, and adds tests to assert the original raw text remains present. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
benoitf
approved these changes
Mar 18, 2026
Contributor
|
for this one there is already some custom addition only on Kortex, I'm fine for now if it's not backported |
benoitf
approved these changes
Mar 19, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/renderer/src/lib/markdown/Markdown.spec.ts (1)
266-280: Consider adding a test for directives with attributes.The tests cover the basic cases well. Consider adding a test for the attributes reconstruction path to ensure full coverage of the fallback handler:
test('Expect unrecognized directive with label and attributes to be preserved', async () => { await waitRender({ markdown: ':unknown[some label]{foo="bar"}' }); const markdownContent = screen.getByRole('region', { name: 'markdown-content' }); expect(markdownContent).toBeInTheDocument(); expect(markdownContent.textContent).toContain(':unknown[some label]{foo="bar"}'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/markdown/Markdown.spec.ts` around lines 266 - 280, Add a new test to the "Unrecognized directives" suite that exercises the attributes reconstruction fallback: create a test titled like "Expect unrecognized directive with label and attributes to be preserved" that calls waitRender({ markdown: ':unknown[some label]{foo=\"bar\"}' }), then select the rendered region via screen.getByRole('region', { name: 'markdown-content' }) and assert the element exists and its textContent contains the exact string ':unknown[some label]{foo="bar"}' to cover the attributes path in the fallback handler.packages/renderer/src/lib/markdown/micromark-fallback-directive.ts (1)
19-20: Avoid blanket@ts-nocheck; prefer targeted fixes.Using
@ts-nocheckdisables all type checking for the file. Thethiscontext typing issue can be resolved with a more targeted approach, similar to how other directive handlers in this codebase might handle it.♻️ Suggested approach
-/* eslint-disable `@typescript-eslint/ban-ts-comment` */ -// `@ts-nocheck` import type { Directive } from 'micromark-extension-directive'; +import type { CompileContext } from 'micromark-util-types'; + +// Type the function with explicit `this` context +export function fallback(this: CompileContext, d: Directive): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts` around lines 19 - 20, Remove the blanket file-level "@ts-nocheck" and instead fix the specific typing issues: remove the "// `@ts-nocheck`" and add explicit "this" typings on the micromark directive handler functions (e.g., declare handlers as "function <handlerName>(this: unknown | HTMLElement | ParserState, ...)" or the precise type used elsewhere in the repo) or add a single-line "// `@ts-expect-error`" only where an unavoidable mismatch occurs; target symbols to update are the directive handler functions in micromark-fallback-directive.ts (the exported directive/handler definitions), replacing the global suppression with localized typing/ts-comment fixes.
🤖 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/renderer/src/lib/markdown/micromark-fallback-directive.ts`:
- Around line 38-43: In the block inside micromark-fallback-directive.ts that
builds attrs (the code that iterates d.attributes, maps to `${key}="${value}"`
and calls this.raw), filter out attribute entries whose value is undefined,
null, or an empty string before mapping so you don't emit key="undefined" or
empty attributes; update the logic that constructs attrs (the
Object.entries(d.attributes).map(...) chain) to first .filter(([key, value]) =>
value != null && value !== '') and then join the remaining pairs into the
`{...}` string passed to this.raw.
- Around line 34-42: The directive renderer currently emits raw label and
attribute content using this.raw(...) which can allow unencoded user input;
update the handler that writes d.label and attribute values to HTML-encode them
(use this.encode(...) for label and for each attribute value) before calling
this.raw so labels and values are safely escaped, while preserving the
surrounding literal characters (':' prefix, '['/']' and '{'/' }'). Target the
code handling d.label and the mapping over d.attributes (and keep the existing
attribute key names as-is).
---
Nitpick comments:
In `@packages/renderer/src/lib/markdown/Markdown.spec.ts`:
- Around line 266-280: Add a new test to the "Unrecognized directives" suite
that exercises the attributes reconstruction fallback: create a test titled like
"Expect unrecognized directive with label and attributes to be preserved" that
calls waitRender({ markdown: ':unknown[some label]{foo=\"bar\"}' }), then select
the rendered region via screen.getByRole('region', { name: 'markdown-content' })
and assert the element exists and its textContent contains the exact string
':unknown[some label]{foo="bar"}' to cover the attributes path in the fallback
handler.
In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts`:
- Around line 19-20: Remove the blanket file-level "@ts-nocheck" and instead fix
the specific typing issues: remove the "// `@ts-nocheck`" and add explicit "this"
typings on the micromark directive handler functions (e.g., declare handlers as
"function <handlerName>(this: unknown | HTMLElement | ParserState, ...)" or the
precise type used elsewhere in the repo) or add a single-line "//
`@ts-expect-error`" only where an unavoidable mismatch occurs; target symbols to
update are the directive handler functions in micromark-fallback-directive.ts
(the exported directive/handler definitions), replacing the global suppression
with localized typing/ts-comment fixes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dc4bd11-f430-4812-acd2-b9ade8e7c371
📒 Files selected for processing (3)
packages/renderer/src/lib/markdown/Markdown.spec.tspackages/renderer/src/lib/markdown/Markdown.sveltepackages/renderer/src/lib/markdown/micromark-fallback-directive.ts
Comment on lines
+34
to
+42
| this.raw(':' + (d.name ?? '')); | ||
| if (d.label) { | ||
| this.raw('[' + d.label + ']'); | ||
| } | ||
| if (d.attributes && Object.keys(d.attributes).length > 0) { | ||
| const attrs = Object.entries(d.attributes) | ||
| .map(([key, value]) => `${key}="${value}"`) | ||
| .join(' '); | ||
| this.raw('{' + attrs + '}'); |
There was a problem hiding this comment.
Consider HTML-encoding user content to prevent XSS.
Using this.raw() emits content without HTML encoding. While the DOMParser in Markdown.svelte provides some sanitization, it's safer to encode user-supplied content (label and attribute values) to prevent potential injection issues. Other directive handlers in this codebase use this.encode() for attribute values.
🛡️ Proposed defensive fix
export function fallback(d: Directive): void {
if (d.type === 'textDirective') {
// Reconstruct the original text: :name[label]{attributes}
this.raw(':' + (d.name ?? ''));
if (d.label) {
- this.raw('[' + d.label + ']');
+ this.raw('[' + this.encode(d.label) + ']');
}
if (d.attributes && Object.keys(d.attributes).length > 0) {
const attrs = Object.entries(d.attributes)
- .map(([key, value]) => `${key}="${value}"`)
+ .filter(([, value]) => value !== undefined && value !== '')
+ .map(([key, value]) => `${key}="${this.encode(value)}"`)
.join(' ');
- this.raw('{' + attrs + '}');
+ if (attrs) {
+ this.raw('{' + attrs + '}');
+ }
}
}
}📝 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.
Suggested change
| this.raw(':' + (d.name ?? '')); | |
| if (d.label) { | |
| this.raw('[' + d.label + ']'); | |
| } | |
| if (d.attributes && Object.keys(d.attributes).length > 0) { | |
| const attrs = Object.entries(d.attributes) | |
| .map(([key, value]) => `${key}="${value}"`) | |
| .join(' '); | |
| this.raw('{' + attrs + '}'); | |
| this.raw(':' + (d.name ?? '')); | |
| if (d.label) { | |
| this.raw('[' + this.encode(d.label) + ']'); | |
| } | |
| if (d.attributes && Object.keys(d.attributes).length > 0) { | |
| const attrs = Object.entries(d.attributes) | |
| .filter(([, value]) => value !== undefined && value !== '') | |
| .map(([key, value]) => `${key}="${this.encode(value)}"`) | |
| .join(' '); | |
| if (attrs) { | |
| this.raw('{' + attrs + '}'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts` around
lines 34 - 42, The directive renderer currently emits raw label and attribute
content using this.raw(...) which can allow unencoded user input; update the
handler that writes d.label and attribute values to HTML-encode them (use
this.encode(...) for label and for each attribute value) before calling this.raw
so labels and values are safely escaped, while preserving the surrounding
literal characters (':' prefix, '['/']' and '{'/' }'). Target the code handling
d.label and the mapping over d.attributes (and keep the existing attribute key
names as-is).
Comment on lines
+38
to
+43
| if (d.attributes && Object.keys(d.attributes).length > 0) { | ||
| const attrs = Object.entries(d.attributes) | ||
| .map(([key, value]) => `${key}="${value}"`) | ||
| .join(' '); | ||
| this.raw('{' + attrs + '}'); | ||
| } |
There was a problem hiding this comment.
Handle empty or undefined attribute values.
When value is undefined or an empty string, the current code would produce malformed output like key="undefined" or emit empty attributes. Consider filtering these out.
🔧 Suggested fix
if (d.attributes && Object.keys(d.attributes).length > 0) {
const attrs = Object.entries(d.attributes)
+ .filter(([, value]) => value != null && value !== '')
.map(([key, value]) => `${key}="${value}"`)
.join(' ');
- this.raw('{' + attrs + '}');
+ if (attrs) {
+ this.raw('{' + attrs + '}');
+ }
}📝 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.
Suggested change
| if (d.attributes && Object.keys(d.attributes).length > 0) { | |
| const attrs = Object.entries(d.attributes) | |
| .map(([key, value]) => `${key}="${value}"`) | |
| .join(' '); | |
| this.raw('{' + attrs + '}'); | |
| } | |
| if (d.attributes && Object.keys(d.attributes).length > 0) { | |
| const attrs = Object.entries(d.attributes) | |
| .filter(([, value]) => value != null && value !== '') | |
| .map(([key, value]) => `${key}="${value}"`) | |
| .join(' '); | |
| if (attrs) { | |
| this.raw('{' + attrs + '}'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts` around
lines 38 - 43, In the block inside micromark-fallback-directive.ts that builds
attrs (the code that iterates d.attributes, maps to `${key}="${value}"` and
calls this.raw), filter out attribute entries whose value is undefined, null, or
an empty string before mapping so you don't emit key="undefined" or empty
attributes; update the logic that constructs attrs (the
Object.entries(d.attributes).map(...) chain) to first .filter(([key, value]) =>
value != null && value !== '') and then join the remaining pairs into the
`{...}` string passed to this.raw.
…#1107) Unrecognized micromark directives (e.g. :poof in "look at that:poof!") were silently stripped. Add a wildcard fallback handler that outputs the raw text for any directive not handled by button, image, link, or warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
auto-merge was automatically disabled
March 19, 2026 18:27
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/renderer/src/lib/markdown/micromark-fallback-directive.ts (1)
19-21: Consider removing@ts-nocheckin favor of explicitthistyping.Blanket disabling TypeScript checking loses type safety for the entire file. TypeScript supports explicit
thisparameter typing which would preserve type checking while addressing the compile context typing:import type { CompileContext } from 'micromark-util-types'; export function fallback(this: CompileContext, d: Directive): void { // ... }This keeps the rest of the file type-checked and documents the expected context explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts` around lines 19 - 21, Remove the file-wide "@ts-nocheck" and instead add an explicit "this" parameter type to the fallback function to preserve typechecking; import the CompileContext type from 'micromark-util-types' and declare the function signature as fallback(this: CompileContext, d: Directive): void (or the appropriate return type) so TypeScript can type the compile context while keeping the Directive import in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/renderer/src/lib/markdown/micromark-fallback-directive.ts`:
- Around line 19-21: Remove the file-wide "@ts-nocheck" and instead add an
explicit "this" parameter type to the fallback function to preserve
typechecking; import the CompileContext type from 'micromark-util-types' and
declare the function signature as fallback(this: CompileContext, d: Directive):
void (or the appropriate return type) so TypeScript can type the compile context
while keeping the Directive import in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c80b05ce-428c-4080-8895-68f9234bce3f
📒 Files selected for processing (4)
Untitledpackages/renderer/src/lib/markdown/Markdown.spec.tspackages/renderer/src/lib/markdown/Markdown.sveltepackages/renderer/src/lib/markdown/micromark-fallback-directive.ts
✅ Files skipped from review due to trivial changes (2)
- Untitled
- packages/renderer/src/lib/markdown/Markdown.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/renderer/src/lib/markdown/Markdown.spec.ts
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
4 participants
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.
Unrecognized micromark directives (e.g. :2b in "granite3.2:2b")
were silently stripped. Add a wildcard fallback handler that outputs the
raw text for any directive not handled by button, image, link, or warnings.
Before the change:
After, the same message is rendered properly:
Fixes #1107