Markdown viewer: fix entities, add callouts, line numbers, perf - #4185
Open
cameronsjo wants to merge 7 commits into
Open
cameronsjo wants to merge 7 commits into
cameronsjo wants to merge 7 commits into
Conversation
|
@cameronsjo is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughRefactors rendering into doRender(md) with fnv1aHash-based change detection, adds a sanitization regex fast-check, preserves inline-code escaping, renders fenced code with per-line gutters, emits GitHub-style callouts with injected SVG icons, debounces window.__cmuxRenderMarkdown, cleans exported clones of gutters, and fixes Cmd/Ctrl+A selection. ChangesMarkdown Viewer Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant doRender
participant needsSanitization
participant DOMPurify
participant injectCalloutIcons
participant contentEl
Client->>doRender: call with md
doRender->>needsSanitization: test rendered HTML
alt needs sanitization
needsSanitization->>DOMPurify: sanitize HTML
DOMPurify->>doRender: sanitized HTML
else skip sanitization
needsSanitization-->>doRender: original HTML
end
doRender->>injectCalloutIcons: post-process sanitized HTML
injectCalloutIcons->>doRender: final HTML
doRender->>contentEl: set innerHTML (if hash differs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (14 passed)
✨ 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 |
![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
🤖 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 `@Resources/markdown-viewer/shell.html`:
- Around line 454-460: The fast-path regex in sanitizationPattern used by
needsSanitization is too narrow and can miss unsafe HTML (e.g., "<link>" without
intervening space and event attributes with spaces like "onload ="); update
sanitizationPattern to more robustly detect these cases by changing the
"<link(?:\s)" fragment to "<link\\b" (so tag boundary is respected) and replace
the current on-attribute pattern with something that matches "on" followed by
letters and optional whitespace before the "=" (e.g., "on[a-z]+\\s*=") so
attributes like "onload =" are caught; ensure the revised regex still includes
the existing checks (script, iframe, object, embed, meta, base, form controls,
style/srcdoc/autofocus/formaction/xlink:href and javascript/vbscript/data URI
schemes) and keep it used by needsSanitization and the surrounding
sanitizeRenderedHTML flow.
- Around line 1133-1135: The keyboard shortcut listener using
document.addEventListener('keydown', function(ev) { ... }) should check the
physical key via ev.code instead of the character via ev.key; update the
conditional that currently checks ev.key === 'a' to use ev.code === 'KeyA' while
keeping the modifier checks ((ev.metaKey || ev.ctrlKey) && !ev.shiftKey &&
!ev.altKey) and ev.preventDefault(), so Cmd/Ctrl+A is detected reliably across
layouts and shift states.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 641ac84e-65c5-437b-852d-69c9da9ea06b
📒 Files selected for processing (1)
Resources/markdown-viewer/shell.html
Contributor
Greptile SummaryThis PR upgrades the WebKit markdown viewer with GitHub-style callout blocks, per-line code gutters, a fix for double-escaped HTML entities in inline code (confirmed against the bundled marked v13.0.3), Cmd+A document-level selection, and performance improvements via FNV-1a hash diffing and an 80ms debounce. All changes are confined to
Confidence Score: 4/5Safe to merge for documents without remote images; callout icons are silently lost whenever the sanitizer runs alongside callout content. The previously-reported data-cmux-callout-type stripping bug is still present: the sanitizer unconditionally removes all data-cmux-* attributes (lines 1003-1006), so when the full TreeWalker path runs the injectCalloutIcons function finds no matching elements and icons are silently dropped. The four issues flagged in the prior review round are all correctly addressed. Resources/markdown-viewer/shell.html — the interaction between sanitizeRenderedHTML stripping data-cmux-* attributes and injectCalloutIcons relying on data-cmux-callout-type. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[__cmuxRenderMarkdown called] --> B{lastRenderedHash === null?}
B -- Yes, first render --> C[doRender immediately]
B -- No --> D[clearTimeout + setTimeout 80ms]
D --> C
C --> E[extractFrontmatter]
E --> F[marked.parse body]
F --> G{needsSanitization regex match?}
G -- No, fast path --> H[use parsed HTML directly]
G -- Yes --> I[sanitizeRenderedHTML via TreeWalker]
I --> J[strips blocked tags, on* attrs, data-cmux-* attrs]
H --> K[renderFrontmatter + parsed/sanitized]
J --> K
K --> L{fnv1aHash === lastRenderedHash?}
L -- Same hash, skip --> M[rewriteImageSources + postProcessSpecialBlocks]
L -- Different hash --> N[contentEl.innerHTML = html]
N --> O[restoreScrollState]
N --> M
M --> P[injectCalloutIcons via data-cmux-callout-type]
P --> Q{attribute present?}
Q -- Yes, fast path taken --> R[Insert Octicon SVG]
Q -- No, sanitizer stripped it --> S[Icons silently omitted]
Reviews (5): Last reviewed commit: "refactor(markdown-viewer): derive saniti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+613
to
+631
| blockquote(quote) { | ||
| var calloutMatch = (quote || '').match(/^<p>\[!(NOTE|TIP|IMPORTANT|WARNING|CAUTION)\]/i); | ||
| if (!calloutMatch) { | ||
| return '<blockquote>\n' + quote + '</blockquote>\n'; | ||
| } | ||
| var type = calloutMatch[1].toLowerCase(); | ||
| var body = quote.replace(calloutMatch[0], '<p>').replace(/^<p>\s*(?:<br\s*\/?>)?\s*/, '<p>'); | ||
| var icons = { | ||
| note: '<svg viewBox="0 0 16 16" fill="currentColor"><path d="M0 8a8 8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0 0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75 0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8 6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"/></svg>', | ||
| tip: '<svg viewBox="0 0 16 16" fill="currentColor"><path d="M8 1.5c-2.363 0-4 1.69-4 3.75 0 .984.424 1.625.984 2.304l.214.253c.223.264.47.556.673.848.284.411.537.896.621 1.49a.75.75 0 0 1-1.484.211c-.04-.282-.163-.547-.37-.847a8.456 8.456 0 0 0-.542-.68c-.084-.1-.173-.205-.268-.32C3.201 7.75 2.5 6.766 2.5 5.25 2.5 2.31 4.863.5 8 .5s5.5 1.81 5.5 4.75c0 1.516-.701 2.5-1.328 3.259a10.8 10.8 0 0 0-.268.32c-.207.245-.383.453-.542.68-.207.3-.33.565-.37.847a.751.751 0 0 1-1.485-.212c.084-.593.337-1.078.621-1.489.203-.292.45-.584.673-.848.075-.088.147-.173.213-.253.561-.679.985-1.32.985-2.304 0-2.06-1.637-3.75-4-3.75ZM5.75 12h4.5a.75.75 0 0 1 0 1.5h-4.5a.75.75 0 0 1 0-1.5ZM6 15.25a.75.75 0 0 1 .75-.75h2.5a.75.75 0 0 1 0 1.5h-2.5a.75.75 0 0 1-.75-.75Z"/></svg>', | ||
| important: '<svg viewBox="0 0 16 16" fill="currentColor"><path d="M0 1.75C0 .784.784 0 1.75 0h12.5C15.216 0 16 .784 16 1.75v9.5A1.75 1.75 0 0 1 14.25 13H8.06l-2.573 2.573A1.458 1.458 0 0 1 3 14.543V13H1.75A1.75 1.75 0 0 1 0 11.25Zm1.75-.25a.25.25 0 0 0-.25.25v9.5c0 .138.112.25.25.25h2a.75.75 0 0 1 .75.75v2.19l2.72-2.72a.749.749 0 0 1 .53-.22h6.5a.25.25 0 0 0 .25-.25v-9.5a.25.25 0 0 0-.25-.25Zm7 2.25v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 9a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z"/></svg>', | ||
| warning: '<svg viewBox="0 0 16 16" fill="currentColor"><path d="M6.457 1.047c.659-1.234 2.427-1.234 3.086 0l6.082 11.378A1.75 1.75 0 0 1 14.082 15H1.918a1.75 1.75 0 0 1-1.543-2.575Zm1.763.707a.25.25 0 0 0-.44 0L1.698 13.132a.25.25 0 0 0 .22.368h12.164a.25.25 0 0 0 .22-.368Zm.53 3.996v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 11a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z"/></svg>', | ||
| caution: '<svg viewBox="0 0 16 16" fill="currentColor"><path d="M4.47.22A.749.749 0 0 1 5 0h6c.199 0 .389.079.53.22l4.25 4.25c.141.14.22.331.22.53v6a.749.749 0 0 1-.22.53l-4.25 4.25A.749.749 0 0 1 11 16H5a.749.749 0 0 1-.53-.22L.22 11.53A.749.749 0 0 1 0 11V5c0-.199.079-.389.22-.53Zm.84 1.28L1.5 5.31v5.38l3.81 3.81h5.38l3.81-3.81V5.31L10.69 1.5ZM8 4a.75.75 0 0 1 .75.75v3.5a.75.75 0 0 1-1.5 0v-3.5A.75.75 0 0 1 8 4Zm0 8a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"/></svg>' | ||
| }; | ||
| var title = type.charAt(0).toUpperCase() + type.slice(1); | ||
| return '<div class="cmux-callout cmux-callout-' + type + '">' | ||
| + '<div class="cmux-callout-title">' + (icons[type] || '') + ' ' + title + '</div>' | ||
| + '<div class="cmux-callout-body">' + body + '</div>' | ||
| + '</div>\n'; |
Contributor
There was a problem hiding this comment.
The blockquote renderer injects hardcoded <svg viewBox="..."> elements as callout icons. Because needsSanitization includes <svg\b in its trigger pattern, needsSanitization(parsed) returns true for every document that contains a callout. This calls sanitizeRenderedHTML, which has svg: true in blockedTags and unconditionally el.remove()s every <svg> element. The icons are generated and then immediately deleted — they will never be visible in any callout block.
Comment on lines
+454
to
+460
| // Fast pre-check: most markdown files produce HTML with no dangerous | ||
| // elements or attributes. This regex tests for patterns the full | ||
| // TreeWalker sanitizer would act on — if none match, skip the walk. | ||
| var sanitizationPattern = /<(?:script|iframe|object|embed|link(?:\s)|meta|base|form|button|textarea|select|option|svg|math)\b|<input\b(?![^>]*type\s*=\s*["']?checkbox)|(?:\son[a-z]+=|\bstyle\s*=|\bsrcdoc\s*=|\bautofocus\b|\bformaction\s*=|\bxlink:href\s*=)|(?:href|src)\s*=\s*["']?\s*(?:javascript|vbscript|data):/i; | ||
| function needsSanitization(html) { | ||
| return sanitizationPattern.test(html || ''); | ||
| } |
Contributor
There was a problem hiding this comment.
needsSanitization regex does not cover HTML-entity-encoded dangerous protocols
The regex checks for the literal string javascript:, vbscript:, or data: in href/src values. HTML-entity-encoded variants such as javascript: or javascript: are not matched by the pattern but are decoded by the browser's HTML attribute parser before the URL is evaluated. If such content appears in a markdown-generated link with no other blocked patterns present, the fast-path returns false and the HTML is inserted unsanitized. The full TreeWalker path's isSafeURLAttribute uses compactForProtocolCheck which strips whitespace and control characters, but does not decode HTML entities either — however that path is now skipped entirely for these inputs.
Comment on lines
+602
to
+607
| var lineCount = (raw.match(/\n/g) || []).length + 1; | ||
| var gutter = ''; | ||
| for (var ln = 1; ln <= lineCount; ln++) { | ||
| gutter += '<span class="cmux-line-num">' + ln + '</span>'; | ||
| } | ||
| return '<pre class="cmux-code-with-lines"><span class="cmux-line-gutter" aria-hidden="true">' + gutter + '</span><code class="hljs' + langClass + '">' + highlighted + '</code></pre>\n'; |
Contributor
There was a problem hiding this comment.
lineCount is computed as (raw.match(/\n/g) || []).length + 1. If raw ends with a \n, the count will be one more than the number of visible lines, producing an empty dangling line number at the bottom of the gutter that has no corresponding code line.
Comment on lines
+1077
to
1087
| window.__cmuxRenderMarkdown = function(md) { | ||
| if (renderTimer) { clearTimeout(renderTimer); } | ||
| if (lastRenderedHash === 0) { | ||
| doRender(md); | ||
| return; | ||
| } | ||
| renderTimer = setTimeout(function() { | ||
| renderTimer = null; | ||
| doRender(md); | ||
| }, 80); | ||
| }; |
Contributor
There was a problem hiding this comment.
lastRenderedHash === 0 used as "not yet rendered" sentinel collides with a valid hash
lastRenderedHash starts at 0 and is used to decide whether to skip the 80ms debounce. FNV-1a on a 32-bit unsigned result can legitimately produce 0 for some inputs. If the hash of the first rendered document is 0, the next call to __cmuxRenderMarkdown will again bypass the debounce. A named sentinel like -1 (which >>> 0 arithmetic never produces) or a separate boolean hasRendered flag would eliminate the ambiguity.
cameronsjo
added a commit
to cameronsjo/cmux
that referenced
this pull request
May 20, 2026
Addresses the six findings from CodeRabbit and Greptile on PR manaflow-ai#4185: 1. Callout SVGs are no longer stripped by their own sanitizer. The icons are kept out of the rendered HTML pipeline and injected from postProcessSpecialBlocks() via a new injectCalloutIcons() helper. The blockquote renderer emits a data-cmux-callout-type placeholder whose value comes from a whitelisted capture group, so the attribute is safe even after a sanitization pass. (Greptile P1) 2. The fast-path sanitization regex now triggers on any character reference inside an href/src attribute value (e.g. "&manaflow-ai#106;avascript:"), so entity-encoded protocol smuggling falls through to the full sanitizer (which already decodes via attribute parsing). (Greptile P1 security) 3. Sanitization regex tightened: "<link\b" replaces "<link(?:\s)" so a bare "<link>" is caught, and "\son[a-z]+\s*=" allows whitespace before "=", so attributes like "onload =" are detected. (CodeRabbit critical) 4. lastRenderedHash now uses a null sentinel instead of 0. fnv1aHash can legitimately produce 0 (FNV-1a 32-bit, post ">>> 0"), which previously collided with the "not yet rendered" check. (Greptile P2) 5. Code-block line counting strips exactly one trailing newline before counting, removing the phantom dangling line number when raw code ends with "\n" (which is the common case from marked). (Greptile P2) 6. The viewer's Cmd/Ctrl+A handler matches on ev.code === "KeyA" instead of ev.key === "a" so the shortcut works regardless of keyboard layout or shift state. (CodeRabbit minor) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cameronsjo
force-pushed
the
feat/md-viewer-improvements-wt
branch
from
May 20, 2026 00:43
6f428f6 to
32a9c10
Compare
cameronsjo
added a commit
to cameronsjo/cmux
that referenced
this pull request
May 20, 2026
Addresses the six findings from CodeRabbit and Greptile on PR manaflow-ai#4185: 1. Callout SVGs are no longer stripped by their own sanitizer. The icons are kept out of the rendered HTML pipeline and injected from postProcessSpecialBlocks() via a new injectCalloutIcons() helper. The blockquote renderer emits a data-cmux-callout-type placeholder whose value comes from a whitelisted capture group, so the attribute is safe even after a sanitization pass. (Greptile P1) 2. The fast-path sanitization regex now triggers on any character reference inside an href/src attribute value (e.g. "&manaflow-ai#106;avascript:"), so entity-encoded protocol smuggling falls through to the full sanitizer (which already decodes via attribute parsing). (Greptile P1 security) 3. Sanitization regex tightened: "<link\b" replaces "<link(?:\s)" so a bare "<link>" is caught, and "\son[a-z]+\s*=" allows whitespace before "=", so attributes like "onload =" are detected. (CodeRabbit critical) 4. lastRenderedHash now uses a null sentinel instead of 0. fnv1aHash can legitimately produce 0 (FNV-1a 32-bit, post ">>> 0"), which previously collided with the "not yet rendered" check. (Greptile P2) 5. Code-block line counting strips exactly one trailing newline before counting, removing the phantom dangling line number when raw code ends with "\n" (which is the common case from marked). (Greptile P2) 6. The viewer's Cmd/Ctrl+A handler matches on ev.code === "KeyA" instead of ev.key === "a" so the shortcut works regardless of keyboard layout or shift state. (CodeRabbit minor) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Resources/markdown-viewer/shell.html`:
- Around line 1091-1097: The rendered line-number spans (class
"cmux-line-gutter") are being serialized into plain-text exports; update the
HTML generation that builds the gutter (the loop creating '<span
class="cmux-line-num">') to mark the gutter container/spans with a removable
data attribute or explicit hook (e.g., add a data-cmux-line-gutter marker on the
gutter element) so it can be identified for stripping, then modify
cleanRenderedContentClone() to remove elements matching that marker (or
'.cmux-line-gutter') during clone cleanup and stop relying on the
[data-cmux-chrome="1"] selector—ensure window.__cmuxRenderedText() uses the
cleaned clone so numbers are omitted from copied/exported plain text.
- Around line 901-926: The fast-path regex in sanitizationPattern (used by
needsSanitization) is missing tags and entity/unquoted cases that
sanitizeRenderedHTML would catch, allowing <style>, media tags
(video/audio/source), unquoted entity-encoded href/src (e.g.
href=&`#106`;avascript:) and similar to bypass sanitization; update
sanitizationPattern to include style and media tags (style, video, audio,
source, picture, track), add unquoted attribute value matching for entity
references on href/src (not just quoted), and ensure input patterns like
\bstyle\b and \b(?:href|src)\s*=\s*[^>\s]*& are covered so needsSanitization
returns true for cases sanitizeRenderedHTML would strip, keeping behavior
consistent with sanitizeRenderedHTML and rewriteLocalImageSources.
- Around line 333-338: The fnv1aHash function uses floating-point multiplication
(hash * 0x01000193) which loses precision; replace that multiplication with
Math.imul to perform a 32-bit integer multiply and preserve correct FNV-1a
behavior (update the multiplication in fnv1aHash to use Math.imul(hash,
0x01000193) and keep the existing >>> 0 unsigned wrap).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b34298a-d6af-4c51-b433-fb5c8f9d0ff4
📒 Files selected for processing (1)
Resources/markdown-viewer/shell.html
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ers, selection - Fix manaflow-ai#4144: stop double-escaping HTML entities in inline code spans. marked v13 passes codespan content already escaped; remove redundant escapeHtml() call and add unescapeHtml() for path detection. - Add GitHub-style callout/admonition blocks (> [!NOTE], > [!TIP], > [!IMPORTANT], > [!WARNING], > [!CAUTION]) with colored left borders and Octicon SVG icons. - Add line number gutter to fenced code blocks. Numbers are non-selectable so copy grabs only code content. - Fix manaflow-ai#2591: Cmd+A / Ctrl+A now selects the entire document content instead of just the focused paragraph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… debounce Three performance improvements to the markdown viewer render cycle: - Replace O(n) innerHTML string comparison with FNV-1a hash check. On a 200KB document, this cuts the "nothing changed" path from milliseconds of string comparison to microseconds of integer compare. - Add regex pre-check before the sanitizer TreeWalker. Most markdown files produce no dangerous HTML (no <script>, no onclick=, no javascript: URLs), so the full DOM walk is skipped entirely on the happy path. Sanitizer still fires when suspicious patterns are found. - Debounce rapid re-renders with 80ms delay. First render is immediate; subsequent calls within the window are collapsed. Editors that do atomic write-rename (vim, VS Code) fire the file watcher 2-3 times in quick succession — this collapses them into one render. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses the six findings from CodeRabbit and Greptile on PR manaflow-ai#4185: 1. Callout SVGs are no longer stripped by their own sanitizer. The icons are kept out of the rendered HTML pipeline and injected from postProcessSpecialBlocks() via a new injectCalloutIcons() helper. The blockquote renderer emits a data-cmux-callout-type placeholder whose value comes from a whitelisted capture group, so the attribute is safe even after a sanitization pass. (Greptile P1) 2. The fast-path sanitization regex now triggers on any character reference inside an href/src attribute value (e.g. "&manaflow-ai#106;avascript:"), so entity-encoded protocol smuggling falls through to the full sanitizer (which already decodes via attribute parsing). (Greptile P1 security) 3. Sanitization regex tightened: "<link\b" replaces "<link(?:\s)" so a bare "<link>" is caught, and "\son[a-z]+\s*=" allows whitespace before "=", so attributes like "onload =" are detected. (CodeRabbit critical) 4. lastRenderedHash now uses a null sentinel instead of 0. fnv1aHash can legitimately produce 0 (FNV-1a 32-bit, post ">>> 0"), which previously collided with the "not yet rendered" check. (Greptile P2) 5. Code-block line counting strips exactly one trailing newline before counting, removing the phantom dangling line number when raw code ends with "\n" (which is the common case from marked). (Greptile P2) 6. The viewer's Cmd/Ctrl+A handler matches on ev.code === "KeyA" instead of ev.key === "a" so the shortcut works regardless of keyboard layout or shift state. (CodeRabbit minor) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t-path The render perf work added a needsSanitization() fast-path that skips sanitizeRenderedHTML() when no dangerous pattern matches. After rebasing onto main (which landed manaflow-ai#4288's remote-image privacy feature), this created a gap: manaflow-ai#4288 strips a remote <img> src into data-cmux-remote-src *inside* sanitizeRenderedHTML — which runs on an inert <template>, so the image never fetches. A document containing only a plain remote image matched none of the fast-path patterns, so sanitization was skipped, the live src survived into the live DOM, and the browser auto-fetched the remote image — defeating the "don't auto-load remote images" gate. Add an <img ... src=...http(s): alternative to the fast-path regex so any remote image forces the full sanitizer. Local/relative and file images stay on the fast path (rewriteLocalImageSources scans img[src] directly, independent of sanitization); data: images already trigger via the existing data: alternative. Verified against a 22-case regex suite covering the original review vectors plus remote/local/data image and lazy data-src cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three findings from CodeRabbit's re-review of the rebased branch: 1. FNV-1a hash now uses Math.imul(hash, 0x01000193) instead of `hash * 0x01000193`. Plain `*` is an IEEE-754 double multiply; once the product exceeds 2^53 the `>>> 0` truncation can't recover the correct 32-bit value, so the content-change hash could miss real edits. (Major) 2. The needsSanitization() fast-path is now a strict superset of what sanitizeRenderedHTML() strips. Added the tags that manaflow-ai#4288 introduced into blockedTags but were missing from the regex (style, audio, video, picture, source, track), and broadened the entity-reference branch to match unquoted href/src values too (e.g. `href=&manaflow-ai#106;avascript:`), so nothing the slow path would strip can skip it. (Critical) 3. Line-number gutters (.cmux-line-gutter) are stripped in cleanRenderedContentClone(), so exported HTML and copied plain text from __cmuxRenderedHTML()/__cmuxRenderedText() no longer carry "1 2 3 …". (Major) Verified against a 26-case regex suite covering the new tag/entity cases, the original review vectors, and safe-content negatives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d (round 3) cubic flagged that needsSanitization() omitted the `srcset` and `background` attributes that sanitizeRenderedHTML() strips (lines 1001-1002), violating the documented superset invariant. A document containing only those attributes (e.g. <img srcset="https://...">) would skip the slow path, leaving a remote-image privacy bypass. Added \bsrcset\s*= and \bbackground\s*= to the attribute alternation; the fast-path attribute set now matches the sanitizer's blocked-attribute list exactly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three consecutive review rounds flagged the needsSanitization() fast-path drifting from sanitizeRenderedHTML() — first missing style/media tags, then unquoted entity refs, then srcset/background. Root cause: the fast-path regex and the sanitizer's blocked-tag/attr lists were maintained by hand in two places, so every sanitizer change needed a mirror edit that was easy to miss. Make them share one source of truth: - SANITIZER_BLOCKED_TAGS and SANITIZER_BLOCKED_ATTRS are now module-level constants. sanitizeRenderedHTML() consumes them directly (blockedTags alias + SANITIZER_BLOCKED_ATTRS.indexOf for attribute stripping). - buildSanitizationPattern() generates the fast-path regex from the same lists, with SANITIZER_BOOLEAN_ATTRS marking valueless attributes (autofocus) so they match on a word boundary rather than `=`. - Adding a tag or attribute now updates both the sanitizer and the fast-path together; they cannot drift. The generated regex is behaviorally identical to the prior hand-written one. Verified by running the in-file builder against a 30-case suite covering every blocked tag, blocked attribute (incl. bare boolean autofocus), the quoted/unquoted entity and active-protocol URL branches, remote vs local images, and safe-content negatives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cameronsjo
force-pushed
the
feat/md-viewer-improvements-wt
branch
from
July 29, 2026 21:35
3e97b56 to
8209354
Compare
Contributor
Author
|
Rebased onto current Both conflict regions were additive on both sides with nothing in the base, so upstream's Every feature this PR adds is intact post-rebase, and all of it is still absent from Prior automated findings (CodeRabbit, greptile, cubic) are all addressed on the rebased head:
The two greptile threads still showing unresolved were fixed in the first round of fixes and simply never marked resolved on the thread — verified against current source rather than assumed. Both issues this fixes are still open: #4144 (HTML entities inside backticks) and #2591 (Ctrl+A selecting only one paragraph). @coderabbitai review |
|
✅ Action performedReview finished.
|
teamleaderleo
added
S2: major
This branch has not been deployed
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
2 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.
Summary
> [!NOTE],> [!TIP],> [!IMPORTANT],> [!WARNING],> [!CAUTION]with colored borders and Octicon SVG iconsAll changes are in
Resources/markdown-viewer/shell.html— no Swift changes.Test plan
`<div>`and`A & B`— entities should render as literal characters, not<div>> [!NOTE],> [!WARNING], etc. blockquotes — should render with colored left border and icon🤖 Generated with Claude Code
Summary by cubic
Upgraded the Markdown viewer with GitHub-style callouts and code-block line numbers, fixed inline-code entities and select-all, and sped up renders with a hash diff, a sanitizer fast-path generated from shared config, and an 80ms debounce. Remote-image privacy is preserved (including
srcset/backgroundand http(s)img[src]), callout icons are injected post-sanitization, gutters no longer show phantom lines, and line numbers are excluded from exported HTML and copied text.New Features
> [!NOTE],> [!TIP],> [!IMPORTANT],> [!WARNING],> [!CAUTION]with icons and colored borders.Bug Fixes
marked v13already escapes); strip one trailing newline to remove phantom last line; exclude line-number gutters from exported HTML and copied text.srcset/background, remote http(s)img[src], and quoted/unquoted entity-encoded protocols; callout icons added after sanitization; remote-image privacy gate remains.Math.imulfor correct 32‑bit diffing.Written for commit 8209354. Summary will update on new commits.
Summary by CodeRabbit