From 7b2b2efe6bc66102fd0d94b31677c1dcf71320df Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Wed, 19 Aug 2026 15:37:12 +0530 Subject: [PATCH] fix(explorer): harden markdown viewer review findings --- explorer/package-lock.json | 31 ++-- .../GraphWorkspace/GraphInspectorPanel.tsx | 20 ++- .../GraphWorkspace/MarkdownContentViewer.tsx | 60 +++++++- explorer/tests/markdownContentViewer.test.ts | 144 ++++++++++++++++++ 4 files changed, 234 insertions(+), 21 deletions(-) diff --git a/explorer/package-lock.json b/explorer/package-lock.json index f8ffecff..c1dc30f4 100644 --- a/explorer/package-lock.json +++ b/explorer/package-lock.json @@ -80,6 +80,7 @@ "integrity": "sha512-QdxmAo/ikZqqRGA8s43ww8lcql6naWRvEz0FFrl6MIlc7Gi6TroXnSdWa5U/kq6fzcpqpHesicQxFZIieZbyIA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@babel/code-frame": "^7.29.0", "@babel/generator": "^7.29.6", @@ -1602,8 +1603,7 @@ "version": "2.0.46", "resolved": "https://registry.npmjs.org/@types/hammerjs/-/hammerjs-2.0.46.tgz", "integrity": "sha512-ynRvcq6wvqexJ9brDMS4BnBLzmr0e14d6ZJTEShTBWKymQiHwlAyGu0ZPEFI2Fh1U53F7tN9ufClWM5KvqkKOw==", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@types/hast": { "version": "3.0.5", @@ -1642,6 +1642,7 @@ "integrity": "sha512-A1sre26ke7HDIuY/M23nd9gfB+nrmhtYyMINbjI1zHJxYteKR6qSMX56FsmjMcDb3SMcjJg5BiRRgOCC/yBD0g==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "undici-types": "~7.16.0" } @@ -1651,6 +1652,7 @@ "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.14.tgz", "integrity": "sha512-ilcTH/UniCkMdtexkoCN0bI7pMcJDvmQFPvuPvmEaYA/NSfFTAgdUSLAoVjaRJm7+6PvcM+q1zYOwS4wTYMF9w==", "license": "MIT", + "peer": true, "dependencies": { "csstype": "^3.2.2" } @@ -1670,8 +1672,7 @@ "resolved": "https://registry.npmjs.org/@types/trusted-types/-/trusted-types-2.0.7.tgz", "integrity": "sha512-ScaPdn1dQczgbl0QFTeTOmVHFULt394XJgOQNoyVhZ6r2vLnMLJfBPd53SB52T/3G36VI1/g2MZaX0cwDuXsfw==", "license": "MIT", - "optional": true, - "peer": true + "optional": true }, "node_modules/@types/unist": { "version": "3.0.3", @@ -1724,6 +1725,7 @@ "integrity": "sha512-/Zb/xaIDfxeJnvishjGdcR4jmr7S+bda8PKNhRGdljDM+elXhlvN0FyPSsMnLmJUrVG9aPO6dof80wjMawsASg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.58.2", "@typescript-eslint/types": "8.58.2", @@ -1993,6 +1995,7 @@ "integrity": "sha512-xRQbDb9BnwDafYNn6Vwl839DYVjqXYb1XVGtWAZ1kcDc6iwAL4hg3B1dZlRiuENFeO2H53gFG3in621AdERVAg==", "dev": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -2112,6 +2115,7 @@ } ], "license": "MIT", + "peer": true, "dependencies": { "baseline-browser-mapping": "^2.10.12", "caniuse-lite": "^1.0.30001782", @@ -2217,8 +2221,7 @@ "version": "2.20.3", "resolved": "https://registry.npmjs.org/commander/-/commander-2.20.3.tgz", "integrity": "sha512-GpVkmM8vF2vQUkj2LvZmD35JxeJOLCwJ9cUkugyk2nuhbv3+mJvpLYYt+0+USMxE+oj+ey/lJEnhZw75x/OMcQ==", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/component-emitter": { "version": "1.3.1", @@ -2256,8 +2259,7 @@ "version": "0.0.10", "resolved": "https://registry.npmjs.org/cssfilter/-/cssfilter-0.0.10.tgz", "integrity": "sha512-FAaLDaplstoRsDR8XGYH51znUN0UY7nMc6Z9/fvE8EXGwvJE9hu7W2vHwx1+bd6gCYnln9nLbzxFTrcO9YQDZw==", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/csstype": { "version": "3.2.3", @@ -2322,6 +2324,7 @@ "resolved": "https://registry.npmjs.org/d3-selection/-/d3-selection-3.0.0.tgz", "integrity": "sha512-fmTRWbNMmsmWq6xJV8D19U/gw/bwrHfNXxrIN+HfZgnzqTHp9jOmKMhsTUjXOJnZOdZY9Q28y4yebKzqDKlxlQ==", "license": "ISC", + "peer": true, "engines": { "node": ">=12" } @@ -2454,7 +2457,6 @@ "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.13.tgz", "integrity": "sha512-2vmYIoqjze2d+kakP8S/nS5shfsl587kzwEjcGlTdiksUVgFHnFCsLYDVj/JNqJVOQZGSYBTmuycv0PodwmnMQ==", "license": "(MPL-2.0 OR Apache-2.0)", - "peer": true, "optionalDependencies": { "@types/trusted-types": "^2.0.7" } @@ -2537,6 +2539,7 @@ "integrity": "sha512-nuKKvN+oIBO0koN7Tm7dlkmnkc21mtt0QJLwAKzjLq14y6lRTdVG36MZHJ8eQHwdJMwZbQNMlPOYedMq/oVJvQ==", "dev": true, "license": "MIT", + "peer": true, "workspaces": [ "packages/*" ], @@ -3336,7 +3339,6 @@ "resolved": "https://registry.npmjs.org/marked/-/marked-14.0.0.tgz", "integrity": "sha512-uIj4+faQ+MgHgwUW1l2PsPglZLOLOT1uErt06dAPtx2kjteLAkbsd/0FiYg/MGS+i7ZKLb7w2WClxHkzOOuryQ==", "license": "MIT", - "peer": true, "bin": { "marked": "bin/marked.js" }, @@ -4412,6 +4414,7 @@ "integrity": "sha512-QP88BAKvMam/3NxH6vj2o21R6MjxZUAd6nlwAS/pnGvN9IVLocLHxGYIzFhg6fUQ+5th6P4dv4eW9jX3DSIj7A==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -4548,6 +4551,7 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.5.tgz", "integrity": "sha512-llUJLzz1zTUBrskt2pwZgLq59AemifIftw4aB7JxOqf1HY2FDaGDxgwpAPVzHU1kdWabH7FauP4i1oEeer2WCA==", "license": "MIT", + "peer": true, "engines": { "node": ">=0.10.0" } @@ -4613,6 +4617,7 @@ "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.2.5.tgz", "integrity": "sha512-J5bAZz+DXMMwW/wV3xzKke59Af6CHY7G4uYLN1OvBcKEsWOs4pQExj86BBKamxl/Ik5bx9whOrvBlSDfWzgSag==", "license": "MIT", + "peer": true, "dependencies": { "scheduler": "^0.27.0" }, @@ -4858,6 +4863,7 @@ "resolved": "https://registry.npmjs.org/sigma/-/sigma-3.0.2.tgz", "integrity": "sha512-/BUbeOwPGruiBOm0YQQ6ZMcLIZ6tf/W+Jcm7dxZyAX0tK3WP9/sq7/NAWBxPIxVahdGjCJoGwej0Gdrv0DxlQQ==", "license": "MIT", + "peer": true, "dependencies": { "events": "^3.3.0", "graphology-utils": "^2.5.2" @@ -4983,6 +4989,7 @@ "integrity": "sha512-X8EX+XV4QR5xCsrgxaED954zTDfY8KqlDtskKEL0cHhyS/P8b4IFOvGDQpsC9Q1XnLq915wEfwwY/zzskCtmhg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.28.0" }, @@ -5015,6 +5022,7 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "dev": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -5238,6 +5246,7 @@ "resolved": "https://registry.npmjs.org/vis-data/-/vis-data-8.0.3.tgz", "integrity": "sha512-jhnb6rJNqkKR1Qmlay0VuDXY9ZlvAnYN1udsrP4U+krgZEq7C0yNSKdZqmnCe13mdnf9AdVcdDGFOzy2mpPoqw==", "license": "(Apache-2.0 OR MIT)", + "peer": true, "funding": { "type": "opencollective", "url": "https://opencollective.com/visjs" @@ -5292,6 +5301,7 @@ "integrity": "sha512-NTKlcQjlAK7MlQoyb6LgaqHc8sso/pVyUJYWMws3jg21uTJw/LddqIFPcPqP6PzpgbIcZyKI85sFE4HBrQDA8A==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.4.4", @@ -5430,6 +5440,7 @@ "integrity": "sha512-rftlrkhHZOcjDwkGlnUtZZkvaPHCsDATp4pGpuOOMDaTdDDXF91wuVDJoWoPsKX/3YPQ5fHuF3STjcYyKr+Qhg==", "dev": true, "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/explorer/src/workspaces/GraphWorkspace/GraphInspectorPanel.tsx b/explorer/src/workspaces/GraphWorkspace/GraphInspectorPanel.tsx index 3cae43f6..8c3a57c8 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphInspectorPanel.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphInspectorPanel.tsx @@ -414,13 +414,19 @@ export function GraphInspectorPanel({ ) : null} - {/* Content Section */} -
- Content -
- -
-
+ {/* Content Section — only rendered when the node carries actual content. + This matches the existing inspector convention: sections that have no + data for the current node are either hidden (temporal bounds) or closed + by default (Source Attribution, Properties). Always showing an open + empty panel would add noise for every relationship/predicate node. */} + {nodeContent && ( +
+ Content +
+ +
+
+ )} {/* Actions */}
diff --git a/explorer/src/workspaces/GraphWorkspace/MarkdownContentViewer.tsx b/explorer/src/workspaces/GraphWorkspace/MarkdownContentViewer.tsx index 6525c124..f00d3dc5 100644 --- a/explorer/src/workspaces/GraphWorkspace/MarkdownContentViewer.tsx +++ b/explorer/src/workspaces/GraphWorkspace/MarkdownContentViewer.tsx @@ -13,6 +13,11 @@ export interface MarkdownContentViewerProps { export function isSafeUrl(url?: string): boolean { if (!url) return false; const trimmed = url.trim(); + // Reject whitespace-only strings — new URL("", base) would resolve to the base + // protocol and produce a false positive. This guards direct callers of the exported + // function; markdown parsers normalise whitespace-only destinations to "" which + // already fails the !url check above. + if (!trimmed) return false; if (trimmed.startsWith("//")) return false; if (trimmed.startsWith("#")) return true; if (trimmed.startsWith("/")) return true; @@ -31,8 +36,24 @@ export function MarkdownContentViewer({ }: MarkdownContentViewerProps) { const [activeMode, setActiveMode] = useState<"preview" | "source">(defaultMode); const [copied, setCopied] = useState(false); + // Track the content value for which the copied indicator is valid. + // When content changes (i.e. the user selects a different node), reset the + // copied indicator inline during render rather than in a useEffect — this + // avoids a cascading-render lint error and is the React-recommended pattern + // for resetting derived visual state on prop changes. + const [copiedForContent, setCopiedForContent] = useState(content); + if (copiedForContent !== content) { + setCopiedForContent(content); + if (copied) { + // Clear the stale indicator synchronously so the new node's copy button + // never shows "Copied" from the previous selection. + setCopied(false); + } + } + const copyTimeoutRef = useRef | null>(null); + // Clean up any outstanding timeout on unmount. useEffect(() => { return () => { if (copyTimeoutRef.current) { @@ -113,12 +134,41 @@ export function MarkdownContentViewer({ { + // C-1: react-markdown passes a HAST `node` prop (the raw AST + // Element) to every custom component override via passNode:true. + // In React 19 any unknown prop spreads onto a native element are + // serialised as HTML attributes, producing node="[object Object]" + // on every rendered link. Fix: destructure `node` by name so it + // is explicitly discarded, then spread `...rest` to preserve all + // other legitimate HAST/remark-gfm attributes — e.g. the `id`, + // `aria-describedby`, `aria-label`, `data-footnote-ref`, + // `data-footnote-backref`, and `class` attrs that GFM footnotes + // require for correct in-page navigation and accessibility. + // + // C-2: fragment links (#anchor, GFM footnote backlinks) must + // navigate within the current document. External links continue + // to use target="_blank" with noopener noreferrer. + // + // eslint-disable-next-line @typescript-eslint/no-unused-vars + a: ({ href, children, title, node: _node, ...rest }) => { if (!isSafeUrl(href)) { return {children}; } + // isSafeUrl returning true guarantees href is a non-empty string. + const safeHref = href ?? ""; + // Fragment links (#section, footnote backlinks like + // #user-content-fnref-1) are in-document anchors. Opening them + // in a new tab would break GFM footnote back-navigation. + const isFragment = safeHref.startsWith("#"); + if (isFragment) { + return ( + + {children} + + ); + } return ( - + {children} @@ -151,10 +201,12 @@ export function MarkdownContentViewer({ th: ({ children }) => {children}, td: ({ children }) => {children}, pre: ({ children }) => 
{children}
, - code: ({ className: codeClass, children, ...props }) => { + // C-1: discard `node` here too — code elements are custom components + // and would otherwise receive node="[object Object]" in the DOM. + code: ({ className: codeClass, children }) => { const isInline = !codeClass && typeof children === "string" && !children.includes("\n"); return ( - + {children} ); diff --git a/explorer/tests/markdownContentViewer.test.ts b/explorer/tests/markdownContentViewer.test.ts index 5596d0ab..aa89f3cb 100644 --- a/explorer/tests/markdownContentViewer.test.ts +++ b/explorer/tests/markdownContentViewer.test.ts @@ -30,6 +30,18 @@ test("isSafeUrl rejects protocol-relative URLs and dangerous schemes", () => { assert.equal(isSafeUrl(undefined), false); }); +// ─── C URL contract: whitespace-only strings ──────────────────────────────── +// The CommonMark parser normalises whitespace-only link destinations to "" so +// these values are unreachable through normal markdown rendering. However, the +// function is exported and its direct-call contract must be correct. +test("isSafeUrl rejects whitespace-only strings (contract correctness)", () => { + assert.equal(isSafeUrl(" "), false, "single space must be rejected"); + assert.equal(isSafeUrl("\t"), false, "tab must be rejected"); + assert.equal(isSafeUrl("\n"), false, "newline must be rejected"); + assert.equal(isSafeUrl(" "), false, "multiple spaces must be rejected"); + assert.equal(isSafeUrl(" \t\n "), false, "mixed whitespace must be rejected"); +}); + test("renders Preview mode with formatted Markdown elements and tabs", () => { const markdown = `# Main Title\n\n**Bold Statement**\n\n* Item A\n* Item B`; const html = renderToString(React.createElement(MarkdownContentViewer, { content: markdown, defaultMode: "preview" })); @@ -69,6 +81,118 @@ test("renders raw HTML safely as escaped text without executing elements", () => assert.equal(html.includes("<script>"), true); }); +// ─── C-1: HAST node prop must not reach the DOM ───────────────────────────── +// react-markdown passes a HAST `node` (Element) object to custom component +// overrides. Before this fix, ...props spread caused React 19 to serialise it +// as node="[object Object]" on every and element. +test("rendered links do not expose the HAST node object as a DOM attribute", () => { + const content = `[Example](https://example.com)\n\nInline \`code\` here.`; + const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" })); + + // The rendered HTML must not contain the serialised HAST object + assert.equal(html.includes("node="), false, "node= attribute must not appear in rendered HTML"); + assert.equal(html.includes("[object Object]"), false, "serialised HAST object must not appear in rendered HTML"); + + // The link must still render correctly with the right href + assert.equal(html.includes('href="https://example.com"'), true, "href must be present"); +}); + +// ─── C-2: Fragment links must not open in a new tab ───────────────────────── +// Links to in-document anchors such as #section or GFM footnote backlinks like +// #user-content-fn-1 must stay in the current document. Only external links +// use target="_blank". +test("fragment links render in the current document without target blank", () => { + const content = `[Jump to section](#introduction)\n\n[External](https://example.com)`; + const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" })); + + // Fragment link must have the href + assert.equal(html.includes('href="#introduction"'), true, "fragment href must be present"); + + // Confirm no target=_blank attribute appears anywhere near the fragment link. + // We check that the output contains a fragment href WITHOUT target="_blank" + // by verifying the two strings are not both present (the external link has + // target blank; the fragment link must not). + const fragmentLinkIdx = html.indexOf('href="#introduction"'); + assert.notEqual(fragmentLinkIdx, -1, "fragment link must be rendered"); + // Inspect the 80 chars around the fragment href — should not contain target + const fragmentContext = html.slice(Math.max(0, fragmentLinkIdx - 10), fragmentLinkIdx + 90); + assert.equal(fragmentContext.includes('target="_blank"'), false, "fragment link must not have target=_blank"); + + // External link must still have target blank + assert.equal(html.includes('href="https://example.com"'), true, "external href must be present"); + assert.equal(html.includes('target="_blank"'), true, "external link must have target=_blank"); + assert.equal(html.includes('rel="noopener noreferrer"'), true, "external link must have rel"); +}); + +test("GFM footnote backlinks render without target blank", () => { + // GFM footnote syntax: footnote ref in text + definition below + const content = `See the note[^1] for more.\n\n[^1]: This is the footnote text.`; + const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" })); + + // The footnote reference link (#user-content-fn-1) and backlink + // (#user-content-fnref-1) are fragment links and must not open in a new tab. + // We verify no fragment href is paired with target=_blank. + // Extract all href="#..." occurrences and confirm none is adjacent to target=_blank. + const anchorMatches = [...html.matchAll(/href="#[^"]*"/g)]; + assert.ok(anchorMatches.length > 0, "GFM footnotes must produce fragment links"); + for (const match of anchorMatches) { + const start = match.index ?? 0; + const context = html.slice(Math.max(0, start - 10), start + 120); + assert.equal( + context.includes('target="_blank"'), + false, + `fragment link ${match[0]} must not have target=_blank`, + ); + } +}); + +// ─── C-1-R: GFM footnote attributes must be preserved (regression test) ───── +// The C-1 fix (removing the HAST `node` prop) must NOT silently drop other +// legitimate HAST attributes. remark-gfm generates the following on footnote +// links that are required for correct in-page navigation and accessibility: +// +// Footnote reference anchor: +// id="user-content-fnref-1" ← backlink target +// data-footnote-ref="true" +// aria-describedby="footnote-label" +// +// Footnote back-link anchor: +// data-footnote-backref="" +// aria-label="Back to reference 1" ← screen-reader label +// class="data-footnote-backref" +// +// If these are absent, clicking the ↩ back-link cannot scroll back to the +// in-text reference, and screen readers cannot announce the backlink purpose. +test("GFM footnote links preserve generated id, aria, and class attributes", () => { + const content = `See the note[^1] for more.\n\n[^1]: This is the footnote text.`; + const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" })); + + // The HAST `node` object must not appear serialised as a DOM attribute. + assert.equal(html.includes("node="), false, "node= attribute must not appear in HTML"); + assert.equal(html.includes("[object Object]"), false, "serialised HAST object must not appear in HTML"); + + // Footnote reference anchor must retain its id so the backlink can navigate to it. + assert.equal( + html.includes('id="user-content-fnref-1"'), + true, + "footnote reference anchor must retain id for back-navigation", + ); + + // Footnote backlink must retain its aria-label for screen-reader accessibility. + assert.equal( + html.includes('aria-label="Back to reference 1"'), + true, + "footnote backlink must retain aria-label for accessibility", + ); + + // Footnote backlink must retain its class attribute. + assert.equal( + html.includes('class="data-footnote-backref"'), + true, + "footnote backlink must retain class attribute", + ); +}); + test("renders safe links as with target blank and unclickable span for unsafe links", () => { const content = `[Safe Link](https://getsemantica.ai)\n\n[Unsafe Scheme](javascript:alert(1))\n\n[Protocol Relative](//evil.com)`; const html = renderToString(React.createElement(MarkdownContentViewer, { content, defaultMode: "preview" })); @@ -118,3 +242,23 @@ test("handles very large Markdown content without failure", () => { const html = renderToString(React.createElement(MarkdownContentViewer, { content: largeContent, defaultMode: "preview" })); assert.equal(html.includes("Large Knowledge Node"), true); }); + +// ─── H-2: Stale copied state lifecycle (SSR-compatible portion) ───────────── +// Full state-transition testing (Node A → copy → Node B) requires an interactive +// framework. The lifecycle correctness is guaranteed by the render-phase +// previous-prop synchronisation pattern: a `copiedForContent` state value tracks +// the content for which the copied indicator was set; when `content` changes, the +// mismatch is detected during render and `copied` is reset to false in the same +// React batch, before the new node's UI is painted. What we CAN verify in SSR +// is that the initial render for any content value shows the Copy button (not the +// Copied indicator), which confirms the initial state is always clean. +test("copy button always starts in un-copied state on initial render", () => { + const html = renderToString(React.createElement(MarkdownContentViewer, { + content: "# Some Node\n\nDescription text.", + defaultMode: "preview", + })); + + // Initial render must show 'Copy', never 'Copied' + assert.equal(html.includes("Copy"), true, "Copy button must be present on initial render"); + assert.equal(html.includes("Copied"), false, "Copied indicator must NOT be present on initial render"); +});