feat: markdown editor and markdown renderer #2140
Conversation
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
WalkthroughReplaces inline ReactMarkdown/Textarea UIs with a read-only Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Viewer as App Component
participant MR as MarkdownRenderer
participant ELW as ExternalLinkWarning
User->>Viewer: Open/view content
Viewer->>MR: Render(content)
Note over MR: sanitize (rehype-sanitize) + GFM
User->>MR: Click <a> link
MR->>MR: isExternalLink(url)?
alt external
MR-->>ELW: request open(url)
User-->>ELW: Confirm
ELW->>Browser: window.open(url, "_blank")
else internal/cancel
MR->>Viewer: follow internal link / do nothing
end
sequenceDiagram
autonumber
actor User
participant Page as Parent
participant ME as MarkdownEditor
participant MDX as @mdxeditor/editor
User->>ME: Click to focus / Type / Paste
ME->>ME: sanitize input (strip escapes, normalize ZWSP)
ME->>MDX: update markdown prop
MDX-->>ME: onChange(markdown)
ME-->>Page: onChange(sanitizedValue)
Note over ME: placeholder/isEmpty toggle UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Comment |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (16)
web/src/components/MarkdownEditor.tsx (6)
1-1: Remove unused ref and type import
editorRefisn’t used; drop it and theMDXEditorMethodstype to reduce noise.Apply:
-import React, { useRef } from "react"; +import React from "react"; @@ - type MDXEditorMethods, type MDXEditorProps, @@ - const editorRef = useRef<MDXEditorMethods>(null); @@ - <MDXEditor ref={editorRef} {...editorProps} /> + <MDXEditor {...editorProps} />Also applies to: 165-165, 208-208, 6-7
207-207: Add dir="auto" for bidi textPreserves previous behavior from textareas and improves RTL handling.
Apply:
- <Container isEmpty={isEmpty}> + <Container isEmpty={isEmpty} dir="auto">
72-75: Let the editor’s placeholder handle emptinessThe custom
p:empty::beforeis brittle with editor updates. Rely onplaceholderinstead.Apply:
- p:empty::before { - color: ${({ theme }) => theme.secondaryText} !important; - opacity: 0.6 !important; - }Also applies to: 176-176
34-57: Target stable class names instead of [class="…"]*Attribute‑contains selectors are fragile. Prefer a wrapper class or the editor’s stable root/toolbar classnames.
If MDXEditor supports
className, set one and scope styles under it to reduce breakage on upstream CSS changes.Also applies to: 59-119
210-217: Externalize copy and link to policyMove this message to i18n and link the “justification policy” to a canonical URL.
173-204: Add aria-label for a11yAssistive tech can better announce the editable region.
Apply:
const editorProps: MDXEditorProps = { markdown: value, onChange: handleChange, placeholder, + contentEditableProps: { "aria-label": placeholder ?? "Markdown editor" },web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx (1)
89-90: Clamp justification length and consider hiding helper message hereSame calldata concern as Reveal. Also, this screen is dense; you may want
showMessage={false}to reduce clutter.Apply:
- <MarkdownEditor value={justification} onChange={setJustification} /> + <MarkdownEditor + value={justification} + onChange={(v) => setJustification(v.slice(0, 4096))} + showMessage={false} + />web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx (3)
40-46: Redundant min-height override
MarkdownEditoralready sets a 200px min-height; this override duplicates styling.Apply:
-const EditorContainer = styled.div` - width: 100%; - - [class*="contentEditable"] { - min-height: 200px; - } -`; +const EditorContainer = styled.div` + width: 100%; +`;
100-107: Restore dir='auto' and keep editor lightweight in a modal
- Add
dir="auto"for bidi text.- Consider lazy‑loading MarkdownEditor in this modal to avoid pulling the heavy editor on initial page load.
Apply:
- <EditorContainer> + <EditorContainer dir="auto"> <MarkdownEditor value={message} onChange={setMessage} placeholder="Describe your evidence in detail..." showMessage={false} /> </EditorContainer>Optional: lazy-load in the parent route or here with React.lazy/Suspense.
72-73: Strip markdown/formatting before isEmpty — treat formatting-only input as emptyweb/src/utils/index.ts:isEmpty (line 11) and the local isEmpty in web/src/components/MarkdownEditor.tsx (≈171) only trim whitespace; they won’t treat inputs like
**,_,`code`,[](), or other formatting-only Markdown as empty. Update isEmpty (or add isEmptyMarkdown) to strip Markdown/formatting (or compute rendered/plaintext length) and use that in SubmitEvidenceModal (web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx:72-73) or normalize message onChange so formatting-only content disables Submit.web/src/components/EvidenceCard.tsx (1)
219-226: Rename leftover ReactMarkdownWrapper for clarity.The wrapper is still named ReactMarkdownWrapper though ReactMarkdown is no longer used. Rename to MarkdownWrapper to avoid confusion.
Apply this diff here (usage), and also rename the definition at Line 69 accordingly:
- <ReactMarkdownWrapper dir="auto"> + <MarkdownWrapper dir="auto"> <MarkdownRenderer content={description} /> - </ReactMarkdownWrapper> + </MarkdownWrapper>- <ReactMarkdownWrapper dir="auto"> + <MarkdownWrapper dir="auto"> <MarkdownRenderer content={evidence} /> - </ReactMarkdownWrapper> + </MarkdownWrapper>Additional change outside this hunk:
-const ReactMarkdownWrapper = styled.div``; +const MarkdownWrapper = styled.div``;web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (1)
88-96: Prevent overflow on long tokens in justifications.Add wrapping to avoid layout breakage with hashes/addresses/URLs.
Apply this diff:
const JustificationContainer = styled.div` line-height: 1.25; + overflow-wrap: anywhere; + word-break: break-word; &::before { content: "Justification: "; color: ${({ theme }) => theme.primaryText}; font-size: 16px; } `;web/src/components/MarkdownRenderer.tsx (4)
158-161: Clickable links show text cursor due to global rule.The universal cursor override forces anchors to use the default cursor. Restore pointer for links.
Apply this diff:
* { cursor: default !important; user-select: text !important; } + a { + cursor: pointer !important; + }
34-36: Wrap long words to avoid horizontal overflow.Markdown often includes long hashes/URLs; add wrapping at the editor root.
Apply this diff:
font-size: 16px; line-height: 1.5; + overflow-wrap: anywhere; + word-break: break-word;
4-14: Lazy‑load MDXEditor to reduce bundle cost.Rendering lists of EvidenceCards/Votes can be heavy. Lazy-load the editor while keeping plugins statically imported.
Apply this diff:
-import { - MDXEditor, - type MDXEditorProps, +import { + type MDXEditorProps, headingsPlugin, listsPlugin, quotePlugin, thematicBreakPlugin, markdownShortcutPlugin, linkPlugin, tablePlugin, } from "@mdxeditor/editor";+const LazyMDXEditor = React.lazy(() => + import("@mdxeditor/editor").then((m) => ({ default: m.MDXEditor })) +);return ( <Container className={className}> - <MDXEditor {...editorProps} /> + <React.Suspense fallback={null}> + <LazyMDXEditor {...editorProps} /> + </React.Suspense> </Container> );Also applies to: 189-193
16-16: Consider relocating the global CSS import if tree-shaking is a concern.Importing "@mdxeditor/editor/style.css" here is fine, but moving it to a single top-level app entry avoids duplicate inclusion across multiple bundles if MarkdownRenderer is code-split.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
web/package.json(1 hunks)web/src/components/EvidenceCard.tsx(2 hunks)web/src/components/MarkdownEditor.tsx(1 hunks)web/src/components/MarkdownRenderer.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx(4 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/JustificationArea.tsx(0 hunks)web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- web/src/pages/Cases/CaseDetails/Voting/JustificationArea.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-09-12T14:45:48.019Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1687
File: web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx:67-84
Timestamp: 2024-09-12T14:45:48.019Z
Learning: In the `submitEvidence` function of the SubmitEvidenceModal component, the try-catch block is specifically designed to handle errors from the `constructEvidence` function, which may throw an Error if file upload fails. This ensures that the file is successfully uploaded before proceeding with evidence submission and transaction signing.
Applied to files:
web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx
📚 Learning: 2024-10-23T08:02:43.167Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1724
File: web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx:86-89
Timestamp: 2024-10-23T08:02:43.167Z
Learning: In the `submitEvidence` function within `SubmitEvidenceModal.tsx`, most errors are caught by `wrapWithToast`, and the `catch` block is mainly for edge cases.
Applied to files:
web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx
📚 Learning: 2024-10-29T06:46:13.522Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/context/AtlasProvider.tsx:159-171
Timestamp: 2024-10-29T06:46:13.522Z
Learning: Both `SubmitEvidenceModal.tsx` and `Policy/index.tsx` use the `uploadFile` function from `AtlasProvider`, which includes the `fetchWithAuthErrorHandling` error handling utility.
Applied to files:
web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx
🧬 Code graph analysis (3)
web/src/components/MarkdownEditor.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
web/src/components/MarkdownRenderer.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: Mend Security Check
🔇 Additional comments (8)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (2)
23-23: Good swap to MarkdownEditorConsistent editor usage across voting views.
116-116: ```shell
#!/bin/bash
set -euo pipefail
echo "1) Searching repo for simulate/contract helpers, MarkdownEditor usage, and 'justification' occurrences..."
rg -n --hidden -S -C3 'simulateDisputeKitClassicCastVote|simulateDisputeKitGatedCastVote|simulateDisputeKitClassic|simulateDisputeKitGated|MarkdownEditor|justification\b|castVote(|submitVote(|revealVote(' || trueecho
echo "2) Show contents of hooks/contracts/generated (if present)"
if [ -d "hooks/contracts/generated" ]; then
ls -la hooks/contracts/generated
echo
rg -n --hidden -S 'simulateDisputeKitClassicCastVote|simulateDisputeKitGatedCastVote' hooks/contracts/generated || true
for f in $(rg -n --hidden --files hooks/contracts/generated || true); do
echo "---- FILE: $f ----"
sed -n '1,240p' "$f" || true
echo
done
else
echo "hooks/contracts/generated not found"
fiecho
echo "3) Search for Solidity contracts and relevant function signatures in repo (.sol)..."
rg -n --hidden -S -g '**/.sol' 'justification\b|function\s+\w*(castVote|submitVote|revealVote)\s*(' || trueecho
echo "4) Search for explicit length checks on justification-like strings in any file (require(bytes(...).length)"
rg -n --hidden -S 'require(.{0,120}bytes(.{0,60}).length|bytes(.{0,60}).length\s*<=|MAX_JUST|MAX_JUSTIFICATION|MAX_REASON|MAX_REASON_LENGTH' || trueecho
echo "5) If no solidity found, list top-level directories to help diagnose missing contracts."
ls -la || true</blockquote></details> <details> <summary>web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx (1)</summary><blockquote> `15-15`: **Good migration to MarkdownEditor** Keeps editing UX consistent with Reveal and Evidence flows. </blockquote></details> <details> <summary>web/src/components/MarkdownEditor.tsx (1)</summary><blockquote> `35-37`: **Theme tokens present — no action required** whiteBackground, stroke, lightGrey, primaryText, secondaryText, mediumBlue and primaryBlue are defined in web/src/styles/themes.ts (light + dark variants), so the styles in web/src/components/MarkdownEditor.tsx (and the other ranges you flagged) are safe. </blockquote></details> <details> <summary>web/package.json (1)</summary><blockquote> `94-94`: **New heavy dependency — lazy-load the editor, audit deps & security** @mdxeditor/editor@3.44.2 — bundlephobia: size 2,965,325 bytes, gzip 868,018 bytes, dependencyCount 56. Imported/used in: - web/src/components/MarkdownEditor.tsx (export) - web/src/components/MarkdownRenderer.tsx (also imports "@mdxeditor/editor/style.css") - web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx - web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx - web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx Action items: - Lazy-load the editor component and its CSS where possible (modals, voting UI); code-split so pages that don’t need the editor don’t download it. - Only include the plugins you actually use (the package pulls in lexical, codemirror, radix, etc.). - Run a security audit and confirm peer deps (e.g., `npm audit` / `npm audit --production` / GitHub securityVulnerabilities GraphQL) before merging. </blockquote></details> <details> <summary>web/src/components/EvidenceCard.tsx (1)</summary><blockquote> `220-226`: **Switch to MarkdownRenderer looks good.** Rendering user-supplied description/evidence through the shared renderer keeps behavior consistent across the app. The dir="auto" wrapper is a nice touch. </blockquote></details> <details> <summary>web/src/pages/Cases/CaseDetails/Voting/VotesDetails/index.tsx (2)</summary><blockquote> `17-17`: **Good reuse of MarkdownRenderer.** Aligns justification rendering with the rest of the app. --- `124-126`: **LGTM on structure and directionality.** dir="auto" plus MarkdownRenderer is appropriate here. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/src/components/MarkdownEditor.tsx (1)
199-230: Harden link creation + avoid plugin re-instantiation on every render
- Add URL validation to linkPlugin/linkDialogPlugin to block javascript: and non-image data: URIs.
- Memoize the plugins array to prevent plugin re-mounts on each keystroke.
Apply this diff:
- import React, { useRef } from "react"; + import React, { useMemo, useRef } from "react"; @@ interface IMarkdownEditor { value: string; onChange: (value: string) => void; placeholder?: string; showMessage?: boolean; } + +// Allow only http(s), mailto, tel. Permit data:image/* if you explicitly want inline images. +const isSafeUrl = (url: string) => { + try { + const u = new URL(url, window.location.origin); + const protocol = u.protocol.replace(":", ""); + if (protocol === "http" || protocol === "https" || protocol === "mailto" || protocol === "tel") return true; + if (protocol === "data") return /^data:image\/(png|jpe?g|gif|webp|svg\+xml);/i.test(url); + return false; + } catch { + return false; + } +}; @@ - const editorProps: MDXEditorProps = { - markdown: value, - onChange: handleChange, - placeholder, - plugins: [ - headingsPlugin(), - listsPlugin(), - quotePlugin(), - thematicBreakPlugin(), - markdownShortcutPlugin(), - linkPlugin(), - linkDialogPlugin(), - tablePlugin(), - toolbarPlugin({ - toolbarContents: () => ( - <> - <UndoRedo /> - <Separator /> - <BoldItalicUnderlineToggles /> - <CodeToggle /> - <Separator /> - <BlockTypeSelect /> - <Separator /> - <ListsToggle /> - <Separator /> - <CreateLink /> - <InsertTable /> - </> - ), - }), - ], - }; + const plugins = useMemo( + () => [ + headingsPlugin(), + listsPlugin(), + quotePlugin(), + thematicBreakPlugin(), + markdownShortcutPlugin(), + linkPlugin({ validateUrl: isSafeUrl }), + linkDialogPlugin({ validateUrl: isSafeUrl }), + tablePlugin(), + toolbarPlugin({ + toolbarContents: () => ( + <> + <UndoRedo /> + <Separator /> + <BoldItalicUnderlineToggles /> + <CodeToggle /> + <Separator /> + <BlockTypeSelect /> + <Separator /> + <ListsToggle /> + <Separator /> + <CreateLink /> + <InsertTable /> + </> + ), + }), + ], + [] + ); + + const editorProps: MDXEditorProps = { + markdown: value, + onChange: handleChange, + placeholder, + plugins, + };Does @mdxeditor/editor's linkPlugin and linkDialogPlugin support a validateUrl option? If so, what's the exact prop name and signature? Also, does MDXEditor expose a suppressHtmlProcessing (or similar) prop to ignore raw HTML/JSX while editing?
🧹 Nitpick comments (6)
web/src/components/MarkdownEditor.tsx (6)
34-49: Avoid brittle wildcard class selectorsThe selectors [class*="mdxeditor-toolbar"] and [class*="contentEditable"] are fragile. Target the concrete MDXEditor class names to reduce accidental matches.
Apply this diff:
- [class*="mdxeditor-toolbar"] { + .mdxeditor-toolbar { @@ - [class*="contentEditable"] { + .mdxeditor-contentEditable {Optional: pass className to MDXEditor and scope styles via that root class to further reduce bleed.
Also applies to: 71-132
110-118: Monospace font and overflow for code blocksImprove readability and prevent layout breakage on long lines.
Apply this diff:
code { background-color: ${({ theme }) => theme.lightGrey} !important; color: ${({ theme }) => theme.primaryText} !important; + font-family: var(--font-mono, ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace); } @@ pre { background-color: ${({ theme }) => theme.lightGrey} !important; color: ${({ theme }) => theme.primaryText} !important; + font-family: var(--font-mono, ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace); + overflow: auto; + padding: 12px; + border-radius: 4px; }
120-122: Preserve link affordanceEnsure links remain visibly distinct for a11y (WCAG). Consider keeping underlines.
Apply this diff:
a { color: ${({ theme }) => theme.primaryBlue} !important; + text-decoration: underline; + text-underline-offset: 2px; }
76-79: Constrain editor height on small screensPrevent the editor from overtaking the viewport.
Apply this diff:
min-height: 200px; + max-height: 50vh; + overflow: auto;
186-195: Focus behavior: reduce reliance on ZWSP seedingTry focusing first; seed ZWSP only if needed. This avoids triggering onChange with phantom content.
Apply this diff:
- const handleContainerClick = () => { - if (isEmpty && editorRef.current) { - editorRef.current.setMarkdown("\u200B"); - setTimeout(() => { - if (editorRef.current) { - editorRef.current.focus(); - } - }, 0); - } - }; + const handleContainerClick = () => { + if (!editorRef.current) return; + editorRef.current.focus(); + if (isEmpty) { + // Seed only if caret placement fails on empty content. + requestAnimationFrame(() => editorRef.current?.setMarkdown("\u200B")); + } + };Please verify this still places the caret reliably across browsers.
166-171: Externalize user-facing strings (i18n-ready)Make the message text configurable like placeholder.
Apply this diff:
interface IMarkdownEditor { value: string; onChange: (value: string) => void; placeholder?: string; showMessage?: boolean; + messageText?: string; } @@ const MarkdownEditor: React.FC<IMarkdownEditor> = ({ value, onChange, placeholder = "Justify your vote...", showMessage = true, + messageText = "Please provide a comprehensive justification for your decision. Quality explanations are essential for the parties involved and may be eligible for additional compensation in accordance with our justification policy.", }) => { @@ - <MessageText> - Please provide a comprehensive justification for your decision. Quality explanations are essential for the - parties involved and may be eligible for additional compensation in accordance with our justification - policy. - </MessageText> + <MessageText>{messageText}</MessageText>Also applies to: 173-178, 239-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
web/package.json(1 hunks)web/src/components/MarkdownEditor.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
web/src/components/MarkdownEditor.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
🔇 Additional comments (1)
web/src/components/MarkdownEditor.tsx (1)
212-229: Toolbar composition looks goodNice, concise set of essentials (undo/redo, inline styles, block types, lists, links, tables).
|
let's add a max-height to the editor container and allow scrolling beyond that |
|
Some things are missing dark-mode 🤔 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
web/src/components/MarkdownRenderer.tsx (2)
3-13: Code blocks and images support (optional).You style
pre/inline code in the theme; if you want parity, addcodeBlockPlugin(and optionallyimagePlugin) or drop the styles to avoid confusion. This was mentioned earlier.
33-45: Do not rely on MDXEditor(readOnly) as a safe renderer; disable HTML/JSX parsing.You sanitize via regex and then pass to
MDXEditorwhich still parses raw HTML/JSX by default. SetsuppressHtmlProcessingto stop HTML parsing. For robust safety, prefer rendering with a sanitizer (e.g., react-markdown + rehype-sanitize) instead of MDXEditor read-only.const editorProps: MDXEditorProps = { markdown: sanitizedContent, readOnly: true, + suppressHtmlProcessing: true, plugins: [web/src/components/MarkdownEditor.tsx (1)
102-113: Harden the editor: disable HTML/JSX processing and validate URLs in the link plugin.Set
suppressHtmlProcessingand wirevalidateUrlto preventjavascript:/data:links from being inserted.const editorProps: MDXEditorProps = { markdown: value, onChange: handleChange, placeholder, + suppressHtmlProcessing: true, plugins: [ @@ - linkPlugin(), + linkPlugin({ validateUrl: isValidUrl }), linkDialogPlugin(),
🧹 Nitpick comments (6)
web/src/utils/urlValidation.ts (2)
10-14: Parse the trimmed input and avoid false negatives due to leading/trailing spaces.
new URL(url)will throw for" https://example.com "; you already compute a trimmed value—use it for parsing and for the danger check.Apply:
- const trimmedUrl = url.trim().toLowerCase(); + const trimmed = url.trim(); + const lowered = trimmed.toLowerCase(); @@ - for (const protocol of DANGEROUS_PROTOCOLS) { - if (trimmedUrl.startsWith(protocol)) { + for (const protocol of DANGEROUS_PROTOCOLS) { + if (lowered.startsWith(protocol)) { return false; } } @@ - const urlObj = new URL(url); + const urlObj = new URL(trimmed); return ALLOWED_PROTOCOLS.includes(urlObj.protocol);Also applies to: 22-25
1-3: Revisit allowed/blocked schemes; consider blocking blob: and supporting relative links (behind an option).
- If users can paste blob: URLs, they can reference attacker-controlled HTML. Block
blob:(and optionallyfilesystem:) unless there’s a concrete need.- If you want to support in‑app relative links (e.g.,
/cases/123), consider an opt-in to allow relative URLs with a base.Proposed adjustments:
-const DANGEROUS_PROTOCOLS = ["javascript:", "data:", "vbscript:", "file:", "about:"]; +const DANGEROUS_PROTOCOLS = ["javascript:", "data:", "vbscript:", "file:", "about:", "blob:", "filesystem:"]; -export const isValidUrl = (url: string): boolean => { +export const isValidUrl = (url: string, opts: { allowRelative?: boolean; base?: string } = {}): boolean => { @@ - try { - const urlObj = new URL(trimmed); + try { + const urlObj = + opts.allowRelative && !/^[a-z][a-z0-9+.-]*:/i.test(trimmed) + ? new URL(trimmed, opts.base ?? (typeof window !== "undefined" ? window.location.origin : "https://example.org")) + : new URL(trimmed); return ALLOWED_PROTOCOLS.includes(urlObj.protocol) || (opts.allowRelative && !urlObj.protocol);And gate relative support where needed (e.g., editor vs. renderer).
Also applies to: 16-20
web/src/styles/mdxEditorTheme.ts (2)
94-105: Use min/max-height instead of fixed height to match the “scroll after a limit” UX.A fixed
height: 220pxalways shows a scrollbar, even for short content. Use a min/max to grow naturally and only scroll when exceeding the cap.- height: 220px; - overflow-y: auto; + min-height: 120px; + max-height: 320px; + height: auto; + overflow-y: auto;
3-53: Brittle class name matching and heavy use of!important.Relying on
[class*="mdx"]and!importanteverywhere is fragile for upstream updates and complicates overrides (esp. dark mode). Where possible, prefer the theming hooks MDXEditor provides and reduce!importants in favor of specificity.Do you want me to extract a theme object and centralize tokens to simplify dark‑mode tweaks? I can send a follow‑up patch.
Also applies to: 221-230
web/src/components/MarkdownEditor.tsx (1)
135-151: Nice accessibility touches (role/aria); consider labeling the info text for screen readers.Associate the helper text via
aria-describedbyso SRs announce it when focusing the editor.- <Container isEmpty={isEmpty} onClick={handleContainerClick} role="region" aria-label="Markdown editor"> - <MDXEditor ref={editorRef} {...editorProps} aria-label="Rich text editor for markdown content" /> + <Container + isEmpty={isEmpty} + onClick={handleContainerClick} + role="region" + aria-label="Markdown editor" + aria-describedby={showMessage ? "markdown-editor-hint" : undefined} + > + <MDXEditor ref={editorRef} {...editorProps} aria-label="Rich text editor for markdown content" /> {showMessage && ( <MessageContainer> <StyledInfoIcon /> - <MessageText> + <MessageText id="markdown-editor-hint">web/src/components/MarkdownRenderer.tsx (1)
37-44: Remove editing-only plugins in read-only mode.
markdownShortcutPluginis for editing; drop it to reduce surface.- markdownShortcutPlugin(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/src/components/MarkdownEditor.tsx(1 hunks)web/src/components/MarkdownRenderer.tsx(1 hunks)web/src/styles/mdxEditorTheme.ts(1 hunks)web/src/utils/markdownSanitization.ts(1 hunks)web/src/utils/urlValidation.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
web/src/styles/mdxEditorTheme.ts (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
web/src/utils/markdownSanitization.ts (1)
web/src/utils/urlValidation.ts (1)
sanitizeUrl(30-35)
web/src/components/MarkdownEditor.tsx (2)
web/src/styles/mdxEditorTheme.ts (2)
MDXEditorContainer(55-130)MDXEditorGlobalStyles(132-204)web/src/utils/markdownSanitization.ts (1)
sanitizeMarkdown(3-25)
web/src/components/MarkdownRenderer.tsx (2)
web/src/utils/markdownSanitization.ts (1)
sanitizeMarkdown(3-25)web/src/styles/mdxEditorTheme.ts (1)
MDXRendererContainer(206-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (2)
web/src/utils/urlValidation.ts (1)
30-35: Decide on invalid-URL fallback semantics.Returning
"#"makes the text look like a (dead) link. Consider returning an empty string to drop the href entirely, or preserving the text without a link in the renderer.If you want me to wire this through
MarkdownRendererso invalid links render as plain text, say the word and I’ll provide the diff.web/src/components/MarkdownRenderer.tsx (1)
1-55: If you keep MDXEditor for rendering, ensure anchors can’t cause tabnabbing.MDXEditor’s default anchor rendering may omit
rel="noopener noreferrer"for external links opened in a new tab. If you later enabletarget="_blank", enforcerel(either via sanitizer or a post-render pass).I can provide a scoped mutation effect that adds
rel="noopener noreferrer"to anchors withinMDXRendererContainerpost-render.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/FileViewer/Viewers/MarkdownViewer.tsx (1)
16-18: Fix base64 decoding for UTF‑8 content.
atobreturns a binary string; non‑ASCII characters will garble. Use TextDecoder.Apply this diff:
- // Decode the base64 string - const decodedData = atob(base64String); + // Decode the base64 string as UTF-8 + let decodedData = ""; + try { + const bin = atob(base64String); + const bytes = Uint8Array.from(bin, (c) => c.charCodeAt(0)); + decodedData = new TextDecoder("utf-8").decode(bytes); + } catch { + return null; + }
♻️ Duplicate comments (2)
web/src/utils/markdownSanitization.ts (2)
13-57: Regex HTML sanitization is unsafe and corrupts content; restrict to URL normalization only.The current approach misses many vectors (e.g., encoded schemes) and removes legitimate text (numeric char refs, code blocks). We already render with MDXEditor using
suppressHtmlProcessing: trueand validate links withisValidUrl, so HTML-stripping here is unnecessary.Apply this minimal, safe rewrite that only normalizes link URLs (and converts simple to Markdown); no global tag/attribute stripping:
import { sanitizeUrl } from "./urlValidation"; -export const sanitizeMarkdown = (markdown: string): string => { - if (!markdown || typeof markdown !== "string") { - return ""; - } - - let sanitized = markdown; - - sanitized = sanitized.replace(/\[([^\]]*)\]\(([^)]+)\)/g, (match, text, url) => { - const sanitizedUrl = sanitizeUrl(url); - return `[${text}](${sanitizedUrl})`; - }); - - sanitized = sanitized.replace(/<a\s+href="([^"]*)"[^>]*>([^<]*)<\/a>/gi, (match, url, text) => { - const sanitizedUrl = sanitizeUrl(url); - return `[${text}](${sanitizedUrl})`; - }); - - sanitized = sanitizeRepeated(sanitized, /<script[^>]*>.*?<\/script>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<iframe[^>]*>.*?<\/iframe>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<object[^>]*>.*?<\/object>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<embed[^>]*\/?>/gi, ""); - sanitized = sanitizeRepeated(sanitized, /<form[^>]*>.*?<\/form>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<input[^>]*\/?>/gi, ""); - sanitized = sanitizeRepeated(sanitized, /<button[^>]*>.*?<\/button>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<applet[^>]*>.*?<\/applet>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<audio[^>]*>.*?<\/audio>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<video[^>]*>.*?<\/video>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<svg[^>]*>.*?<\/svg>/gis, ""); - sanitized = sanitizeRepeated(sanitized, /<canvas[^>]*>.*?<\/canvas>/gis, ""); - - sanitized = sanitizeRepeated(sanitized, /javascript:/gi, ""); - sanitized = sanitizeRepeated(sanitized, /vbscript:/gi, ""); - - sanitized = sanitizeRepeated(sanitized, /on\w+\s*=/gi, ""); - sanitized = sanitizeRepeated(sanitized, /style\s*=/gi, ""); - - sanitized = sanitizeRepeated(sanitized, /<meta[^>]*\/?>/gi, ""); - sanitized = sanitizeRepeated(sanitized, /<link[^>]*\/?>/gi, ""); - sanitized = sanitizeRepeated(sanitized, /<base[^>]*\/?>/gi, ""); - - sanitized = sanitized.replace(/&#x[0-9a-f]+;/gi, ""); - sanitized = sanitized.replace(/&#[0-9]+;/gi, ""); - - return sanitized; -}; +export const sanitizeMarkdown = (markdown: string): string => { + if (!markdown || typeof markdown !== "string") return ""; + let out = markdown; + // Normalize Markdown links + out = out.replace(/\[([^\]]*)\]\(([^)]+)\)/g, (_m, text, url) => { + return `[${text}](${sanitizeUrl(String(url))})`; + }); + // Convert simple HTML anchors to Markdown links + out = out.replace(/<a\s+href="([^"]*)"[^>]*>([^<]*)<\/a>/gi, (_m, url, text) => { + return `[${text}](${sanitizeUrl(String(url))})`; + }); + return out; +};If you prefer full HTML allowance, switch to a DOM/AST sanitizer (e.g., remark/rehype with rehype-sanitize) instead of regex.
3-11: Drop iterative regex sanitizer helper.Regex-based HTML scrubbing is brittle and slow; we shouldn’t apply global, repeated replacements to user content.
Apply this diff to remove the helper (no longer needed by the simplified sanitizer below):
-const sanitizeRepeated = (text: string, pattern: RegExp, replacement: string): string => { - let result = text; - let previousResult; - do { - previousResult = result; - result = result.replace(pattern, replacement); - } while (result !== previousResult); - return result; -};
🧹 Nitpick comments (5)
web/src/utils/urlValidation.ts (1)
1-4: Danger-list is redundant with allow-list.
DANGEROUS_PROTOCOLScheck is now mostly redundant given the strict allow-list and earlydata:handling. Keeping it is fine, but consider removing it to reduce surface for false positives.web/src/components/FileViewer/Viewers/MarkdownViewer.tsx (1)
14-17: Guard against malformed data URIs.If
fileDatalacks a comma,split(",")[1]isundefinedandatobthrows.Apply this diff:
- const base64String = (currentDocument.fileData as string).split(",")[1]; + const fileData = currentDocument.fileData as string; + const commaIdx = fileData.indexOf(","); + if (commaIdx === -1) return null; + const base64String = fileData.slice(commaIdx + 1);web/src/components/MarkdownRenderer.tsx (3)
33-37: Avoid double “sanitization”; rely on validateUrl + suppressHtmlProcessing.Given
suppressHtmlProcessing: trueandlinkPlugin({ validateUrl }), the extrasanitizeMarkdown()adds cost and can mutate content. Prefer passingcontentdirectly (or keep only the anchor→Markdown normalization in the helper).Apply this diff:
- const sanitizedContent = sanitizeMarkdown(content); + const sanitizedContent = sanitizeMarkdown(content); // keep only if it normalizes links; otherwise pass `content`If you adopt the simplified sanitizer from utils, keep as-is; otherwise remove the helper call.
21-21: Move editor CSS import to a top-level entry.Importing
@mdxeditor/editor/style.cssin a leaf component risks duplicate injections when used many times. Prefer a single import in the app root or a styles aggregator.
35-51: Consider a lighter, dedicated renderer for consumption.MDXEditor is heavy and its docs discourage using readOnly as a general-purpose renderer. For display paths (votes/evidence/file viewer), consider
react-markdown + rehype-sanitizeor a unified pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/src/components/FileViewer/Viewers/MarkdownViewer.tsx(2 hunks)web/src/components/MarkdownEditor.tsx(1 hunks)web/src/components/MarkdownRenderer.tsx(1 hunks)web/src/utils/markdownSanitization.ts(1 hunks)web/src/utils/urlValidation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/MarkdownEditor.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/components/MarkdownRenderer.tsx (3)
web/src/utils/markdownSanitization.ts (1)
sanitizeMarkdown(13-57)web/src/utils/urlValidation.ts (1)
isValidUrl(19-46)web/src/styles/mdxEditorTheme.ts (1)
MDXRendererContainer(206-230)
web/src/utils/markdownSanitization.ts (1)
web/src/utils/urlValidation.ts (1)
sanitizeUrl(48-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Pages changed - kleros-v2-university
🔇 Additional comments (1)
web/src/components/MarkdownRenderer.tsx (1)
45-50: Support in‑document anchors and relative links.Once
isValidUrlallows#and app‑relative paths, this validator will keep them working inside rendered Markdown.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/MarkdownRenderer.tsx (1)
104-120: Disable HTML/JSX processing; don’t rely on readOnly for safetyYou sanitize upstream, but leaving suppressHtmlProcessing=false keeps HTML/JSX parsing on. Prior review asked to set this true; it’s still false.
Apply this diff:
- suppressHtmlProcessing: false, + suppressHtmlProcessing: true,
🧹 Nitpick comments (7)
web/src/utils/linkUtils.ts (1)
20-27: SSR-safe origin check and avoid classifying parse failures as externalAccessing window during SSR will throw, and treating URL parse failures as external causes false positives. Guard for SSR and return false on parse failure; upstream validators can decide.
Apply this diff:
try { - const currentOrigin = window.location.origin; - const linkUrl = new URL(trimmedUrl, currentOrigin); - - return linkUrl.origin !== currentOrigin; - } catch { - return true; + const baseOrigin = + typeof window !== "undefined" && window.location ? window.location.origin : "http://localhost"; + const linkUrl = new URL(trimmedUrl, baseOrigin); + return linkUrl.origin !== baseOrigin; + } catch { + // Let URL validation handle bad inputs instead of warning as "external". + return false; }web/src/components/ExternalLinkWarning.tsx (4)
30-38: Reuse the shared Overlay component for consistent theming and z-indexingYou already have components/Overlay.tsx. Reuse it here to avoid duplicate styling and ensure dark‑mode consistency.
Apply this diff:
-const Overlay = styled.div` - position: fixed; - top: 0; - left: 0; - right: 0; - bottom: 0; - background-color: rgba(0, 0, 0, 0.5); - z-index: 10001; -`; +import { Overlay as AppOverlay } from "components/Overlay";And in StyledModal usage:
- overlayElement={(props, contentElement) => <Overlay {...props}>{contentElement}</Overlay>} + overlayElement={(props, contentElement) => <AppOverlay {...props}>{contentElement}</AppOverlay>}
126-136: Enable proper aria‑hiding for background contentreact-modal recommends setting an app element and leaving ariaHideApp enabled for a11y. Current ariaHideApp={false} can confuse SRs.
Either set the app element globally or here:
- ariaHideApp={false} + ariaHideApp + appElement={typeof document !== "undefined" ? (document.getElementById("root") as HTMLElement) : undefined}If you already call Modal.setAppElement elsewhere, just drop ariaHideApp.
114-116: Avoid appending hex alpha to theme color
${theme.warning}BBassumes a hex value. If theme switches to rgb/hsl, hover color breaks. Prefer opacity or brightness.Apply this diff:
- &:hover { - background-color: ${({ theme }) => theme.warning}BB; - } + &:hover { + opacity: 0.9; + }
160-163: Button copy: not always a “site” (mailto:, tel:)Label is misleading for non-http(s) links. Use neutral copy.
Apply this diff:
- <ConfirmButton text="Continue to External Site" onClick={onConfirm} /> + <ConfirmButton text="Continue" onClick={onConfirm} />web/src/components/MarkdownRenderer.tsx (2)
104-108: Memoize sanitation to avoid redundant work on re-rendersSmall perf tidy-up.
Apply this diff:
- const sanitizedContent = sanitizeMarkdown(content); + const sanitizedContent = useMemo(() => sanitizeMarkdown(content), [content]);
61-68: Guard window.open usage in SSR/hydration edge casesNot critical, but safe-guarding avoids reference errors in non-browser tests.
Apply this diff:
- if (pendingUrl) { - window.open(pendingUrl, "_blank", "noopener,noreferrer"); - } + if (pendingUrl && typeof window !== "undefined") { + window.open(pendingUrl, "_blank", "noopener,noreferrer"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/components/ExternalLinkWarning.tsx(1 hunks)web/src/components/MarkdownRenderer.tsx(1 hunks)web/src/utils/linkUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/components/ExternalLinkWarning.tsx (2)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)web/src/components/Overlay.tsx (1)
Overlay(3-11)
web/src/components/MarkdownRenderer.tsx (4)
web/src/styles/mdxEditorTheme.ts (1)
MDXRendererContainer(206-230)web/src/utils/urlValidation.ts (1)
isValidUrl(19-46)web/src/utils/linkUtils.ts (1)
isExternalLink(1-28)web/src/utils/markdownSanitization.ts (1)
sanitizeMarkdown(13-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/utils/markdownSanitization.ts (1)
47-51: Remove global string stripping; it corrupts content and is redundant.These replacements can mangle code/text (e.g., “javascript:” in code blocks; numeric entities like
©). DOMPurify already handles dangerous protocols.Apply:
- // Optional: Remove encoded protocols/entities as before - sanitized = sanitized.replace(/javascript:/gi, ""); - sanitized = sanitized.replace(/vbscript:/gi, ""); - sanitized = sanitized.replace(/&#x[0-9a-f]+;/gi, ""); - sanitized = sanitized.replace(/&#[0-9]+;/gi, "");
🧹 Nitpick comments (5)
web/src/utils/markdownSanitization.ts (5)
4-12: Remove unused helper.
sanitizeRepeatedis dead code.Apply:
-const sanitizeRepeated = (text: string, pattern: RegExp, replacement: string): string => { - let result = text; - let previousResult; - do { - previousResult = result; - result = result.replace(pattern, replacement); - } while (result !== previousResult); - return result; -};
21-29: Regex link normalization is brittle; prefer AST-based handling.These regexes miss nested parentheses, titles, single-quoted hrefs, and nested content in anchors. Let the markdown/MDX pipeline parse links and sanitize at the HTML/rehype stage, or walk the AST to normalize
href.If you want a minimal change, drop both replacements and rely on DOMPurify + a URI allowlist (see next comment). Otherwise, I can wire
remark+rehype-sanitize.
31-45: Tighten URL protocols; add explicit allowlist.Add an allowlist for
href/srcto blockdata:and other schemes; keep relative paths and#anchors.Apply:
sanitized = DOMPurify.sanitize(sanitized, { ALLOWED_TAGS: [ @@ ALLOWED_ATTR: ["href", "src", "alt", "title"], // No style, no event handlers + ALLOWED_URI_REGEXP: /^(?:(?:https?|mailto|tel):|\/(?!\/)|#)/i, FORBID_TAGS: [And add this hook near the imports (module scope) to differentiate
hrefvssrc:// Enforce per-attribute URI rules const ALLOWED_HREF = /^(https?:|mailto:|tel:|\/(?!\/)|#)/i; DOMPurify.addHook("uponSanitizeAttribute", (_node, data) => { if (data.attrName === "href") { const v = String(data.attrValue || ""); if (!ALLOWED_HREF.test(v)) data.attrValue = "#"; } if (data.attrName === "src") { const v = String(data.attrValue || ""); if (!/^https?:/i.test(v)) data.keepAttr = false; // drop non-http(s) images } });
33-37: Double-check allowing: privacy and UX.
Allowing remote images may leak user IPs and slow render; if this is undesired, either proxy images or drop
imgfromALLOWED_TAGS. If keeping images, consider allowingloading/decoding/referrerpolicyattributes for UX/privacy.
14-54: Sanitize after render (recommended).Best practice: parse Markdown → render to HTML → sanitize HTML (remark/rehype + rehype-sanitize or DOMPurify on the HTML). Pre-sanitizing raw markdown can strip valid code samples containing
<script>text.I can provide a concise
react-markdown + rehype-sanitizeorunified(remark→rehype→rehype-sanitize)setup and simplify this util accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/package.json(2 hunks)web/src/utils/markdownSanitization.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
web/src/utils/markdownSanitization.ts (1)
web/src/utils/urlValidation.ts (1)
sanitizeUrl(48-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Analyze (javascript)
- GitHub Check: dependency-review
- GitHub Check: hardhat-tests
- GitHub Check: SonarCloud
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Mend Security Check
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (1)
web/src/utils/markdownSanitization.ts (1)
2-2: Avoid SSR breakage — use isomorphic-dompurify if this code may run on the serverweb/src/utils/markdownSanitization.ts imports "dompurify" and web/package.json contains dompurify; the repo appears client-only (react-scripts + vite) so no change is required unless you perform SSR. If you do render server-side, replace the import:
-import DOMPurify from "dompurify"; +import DOMPurify from "isomorphic-dompurify";Add isomorphic-dompurify to web/package.json and remove dompurify when switching.
…heck Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/DisputePreview/DisputeContext.tsx (1)
121-126: Fix ruled indicator condition (always true‑ish due to Boolean() wrapping).
!isUndefined(Boolean(...))is always true. Use the actual value ofruledor check for definedness.- {!isUndefined(Boolean(dispute?.dispute?.ruled)) || jurorRewardsDispersed ? ( + {Boolean(dispute?.dispute?.ruled) || jurorRewardsDispersed ? ( <RulingAndRewardsIndicators - ruled={Boolean(dispute?.dispute?.ruled)} + ruled={Boolean(dispute?.dispute?.ruled)} jurorRewardsDispersed={jurorRewardsDispersed} /> ) : null}
🧹 Nitpick comments (7)
web/src/components/DisputePreview/DisputeContext.tsx (2)
128-129: Guard against negative round index when no rounds.
rounds?.length - 1can be −1. Clamp to 0.- <CardLabel {...{ disputeId }} round={rounds?.length - 1} isList={false} isOverview={true} /> + <CardLabel {...{ disputeId }} round={Math.max(0, (rounds?.length ?? 1) - 1)} isList={false} isOverview={true} />
108-108: Remove stray console.log.Avoid leaking internal data to the console.
- console.log({ jurorRewardsDispersed }, disputeDetails); + // no-opweb/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx (1)
50-51: Remove empty wrapper.
MarkdownWrapperhas no styles; inlinedir="auto"on the renderer or the parent.-const MarkdownWrapper = styled.div``; +// Inline dir="auto" where needed; wrapper not required.web/src/pages/Resolver/Briefing/Description.tsx (2)
20-27: Expose a max-height via theme instead of fixed editor height.Current editor height is fixed in theme; implement min/max height so it scrolls when content grows (per feedback).
-const StyledMarkdownEditor = styled(MarkdownEditor)` - width: 84vw; +const StyledMarkdownEditor = styled(MarkdownEditor)` + width: 84vw; ${landscapeStyle( () => css` width: ${responsiveSize(442, 700, 900)}; ` )} `;Then, in
mdxEditorTheme.ts, change the contenteditable height to min/max (see separate comment/diff).
37-45: Focus: avoid DOM queries; expose focus() from MarkdownEditor.Querying
[contenteditable="true"]is brittle across library updates. Forward a ref fromMarkdownEditorand callfocus().I can wire a
forwardRefexposingfocus()onMarkdownEditorif you want.web/src/styles/mdxEditorTheme.ts (1)
71-91: Dark-mode inconsistencies: replace hard “whiteBackground” with theme surfaces.Some areas use
whiteBackgroundwhere a surface token (e.g.,lightBackground) better adapts to dark mode.Please verify in dark mode that toolbar/panels/popovers contrast is AA. If not, swap
whiteBackgroundfor the appropriate surface token.Also applies to: 107-118, 145-234
web/src/components/MarkdownEditor.tsx (1)
150-167: Consider exposing focus via ref for callers.Forward a ref from
MarkdownEditorto callfocus()without DOM queries (see Resolver Briefing).I can add
forwardRef<{ focus: () => void }>and implement it witheditorRef.current?.focus().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
web/package.json(2 hunks)web/src/components/DisputePreview/DisputeContext.tsx(2 hunks)web/src/components/MarkdownEditor.tsx(1 hunks)web/src/components/MarkdownRenderer.tsx(1 hunks)web/src/components/ReactMarkdown.tsx(0 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(3 hunks)web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx(3 hunks)web/src/pages/Courts/CourtDetails/Description.tsx(3 hunks)web/src/pages/Resolver/Briefing/Description.tsx(3 hunks)web/src/styles/mdxEditorTheme.ts(1 hunks)web/src/utils/urlValidation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/components/ReactMarkdown.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx
- web/src/utils/urlValidation.ts
- web/package.json
- web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/components/DisputePreview/DisputeContext.tsx
📚 Learning: 2025-09-09T13:33:46.896Z
Learnt from: tractorss
PR: kleros/kleros-v2#2117
File: web/src/components/DisputeFeatures/Features/GatedErc1155.tsx:51-66
Timestamp: 2025-09-09T13:33:46.896Z
Learning: The `setDisputeData` function in `NewDisputeContext` at `web/src/context/NewDisputeContext.tsx` has signature `(disputeData: IDisputeData) => void` and only accepts direct state values, not functional updates like standard React state setters. It cannot be used with the pattern `setDisputeData((prev) => ...)`.
Applied to files:
web/src/components/DisputePreview/DisputeContext.tsxweb/src/pages/Resolver/Briefing/Description.tsx
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/components/DisputePreview/DisputeContext.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
PR: kleros/kleros-v2#1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/components/DisputePreview/DisputeContext.tsx
🧬 Code graph analysis (5)
web/src/pages/Courts/CourtDetails/Description.tsx (1)
web/src/components/StyledSkeleton.tsx (1)
StyledSkeleton(8-10)
web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx (1)
web/src/consts/index.ts (2)
RPC_ERROR(42-42)INVALID_DISPUTE_DATA_ERROR(41-41)
web/src/components/MarkdownRenderer.tsx (2)
web/src/utils/urlValidation.ts (1)
isValidUrl(19-46)web/src/utils/linkUtils.ts (1)
isExternalLink(1-28)
web/src/components/MarkdownEditor.tsx (2)
web/src/styles/mdxEditorTheme.ts (2)
MDXEditorContainer(68-143)MDXEditorGlobalStyles(145-250)web/src/utils/urlValidation.ts (1)
isValidUrl(19-46)
web/src/styles/mdxEditorTheme.ts (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: SonarCloud
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (8)
web/src/pages/Cases/CaseDetails/Voting/VotingHistory.tsx (1)
50-57: LGTM on renderer swap and wrapper.Switch to MarkdownRenderer is consistent and styled properly.
Also applies to: 95-103
web/src/pages/Courts/CourtDetails/Description.tsx (1)
22-55: Renderer replacement looks good; styles align with page.Also applies to: 130-132
web/src/pages/Resolver/Briefing/Description.tsx (1)
52-57: Prop wiring is correct.
onChangesignature matches context setter and placeholder/message usage is clear.web/src/components/MarkdownEditor.tsx (2)
108-148: Good: HTML suppressed and URL validation wired into link plugin.Confirm
isValidUrlallows only http/https/mailto/tel as intended; if you need ipfs:// or ar://, extend the allowlist accordingly.
85-93: Do not strip backslashes/escapes on every change; this corrupts user input.Removing escapes breaks code blocks and literal Markdown. Only strip the zero‑width space.
- const handleChange = (markdown: string) => { - let cleanedMarkdown = markdown === "\u200B" ? "" : markdown.replace(/^\u200B/, ""); - // Remove ALL escape characters - no exceptions - cleanedMarkdown = cleanedMarkdown.replace(/\\([`[]*_#|>-+=~^{}()!&<$%\\])/g, "$1"); - // Also handle multiple consecutive backslashes that might accumulate - cleanedMarkdown = cleanedMarkdown.replace(/\\+/g, ""); - - onChange(cleanedMarkdown); - }; + const handleChange = (markdown: string) => { + const cleaned = markdown === "\u200B" ? "" : markdown.replace(/^\u200B/, ""); + onChange(cleaned); + };web/src/components/MarkdownRenderer.tsx (2)
23-38: Anchor overlay breaks internal links and a11y; remove it.The pointer-events overlay prevents normal navigation and context menus.
- a { - pointer-events: none; - cursor: pointer; - position: relative; - - &::after { - content: ""; - position: absolute; - top: 0; - left: 0; - width: 100%; - height: 100%; - pointer-events: auto; - cursor: pointer; - } - } + /* anchors use default pointer events; clicks are intercepted via capture handler */
247-258: Delegate link interception via capture handlers; drop manual DOM listeners.Use React capture handlers to avoid double-capture and keep internal links working.
+ const onClickCapture = useCallback( + (e: React.MouseEvent<HTMLDivElement>) => { + const link = (e.target as HTMLElement).closest("a") as HTMLAnchorElement | null; + if (!link) return; + const href = link.getAttribute("href") || link.href; + if (href && isValidUrl(href) && isExternalLink(href)) { + e.preventDefault(); + e.stopPropagation(); + handleExternalLink(href); + } + }, + [handleExternalLink] + ); + + const onMouseDownCapture = useCallback( + (e: React.MouseEvent<HTMLDivElement>) => { + // Intercept middle-click or meta/ctrl-click new-tab behavior + if (e.button !== 1 && !(e.metaKey || e.ctrlKey)) return; + const link = (e.target as HTMLElement).closest("a") as HTMLAnchorElement | null; + if (!link) return; + const href = link.getAttribute("href") || link.href; + if (href && isValidUrl(href) && isExternalLink(href)) { + e.preventDefault(); + e.stopPropagation(); + handleExternalLink(href); + } + }, + [handleExternalLink] + ); - - useEffect(() => { - const container = containerRef.current; - if (!container) return; - - const handleClick = (event: Event) => { - const target = event.target as HTMLElement; - - if (!container.contains(target)) { - return; - } - - const linkElement = target.closest("a") as HTMLAnchorElement | null; - - if (linkElement) { - const href = linkElement.getAttribute("href") || linkElement.href; - if (href && isValidUrl(href) && isExternalLink(href)) { - event.preventDefault(); - event.stopImmediatePropagation(); - handleExternalLink(href); - } - } - }; - - container.addEventListener("click", handleClick, true); - - return () => { - container.removeEventListener("click", handleClick, true); - }; - }, [handleExternalLink]); @@ - <MarkdownContainer ref={containerRef} className={className} role="region" aria-label="Markdown content"> + <MarkdownContainer + ref={containerRef} + className={className} + role="region" + aria-label="Markdown content" + onClickCapture={onClickCapture} + onMouseDownCapture={onMouseDownCapture} + >Also applies to: 265-293, 299-301
web/src/styles/mdxEditorTheme.ts (1)
236-250: Scope global[role="button"]styles to MDX editor only.This leaks hover/cursor styles app‑wide.
- [class*="mdx"] button, - [class*="select"] button, - [class*="trigger"], - [role="button"], - [data-radix-portal] [role="button"], - [data-radix-popper-content-wrapper] [role="button"], - [data-radix-portal] [data-state], - [data-radix-popper-content-wrapper] [data-state] { + [class*="mdx"] button, + [class*="mdx"] [class*="select"] button, + [class*="mdx"] [class*="trigger"], + [data-radix-portal] [class*="mdx"] [role="button"], + [data-radix-popper-content-wrapper] [class*="mdx"] [role="button"], + [data-radix-portal] [class*="mdx"] [data-state], + [data-radix-popper-content-wrapper] [class*="mdx"] [data-state] { cursor: pointer !important; transition: all 0.1s ease !important; &:hover { background-color: ${({ theme }) => theme.lightGrey} !important; } }
|
PR-Codex overview
This PR introduces a new
MarkdownRenderercomponent, replacing the previousReactMarkdownusage across various files. It enhances link validation and adds aMarkdownEditor, improving markdown handling and user experience.Detailed summary
ReactMarkdowncomponent.MarkdownRendererfor rendering markdown content.MarkdownEditorfor editing markdown input.linkUtilsandurlValidation.MarkdownRendererandMarkdownEditor.Summary by CodeRabbit
New Features
Refactor
Style
Chores