fix(media): resolve React VDOM crashes, lightbox leaks, Mermaid SVG ID collisions, and i18n caption sync - #692
Conversation
…D collisions, and i18n caption sync Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhuvan-somisetty The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @bhuvan-somisetty! It looks like this is your first PR to Project-HAMi/website 🎉 |
📝 WalkthroughWalkthroughImage figure numbering moves from route-time DOM mutation to MDX image rendering and CSS counters. The lightbox moves from an imperative hook to a React portal with SVG ID sanitization, focus management, and lifecycle cleanup. ChangesImage media rendering and lightbox
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ImageLightbox
participant Document
participant PortalDialog
User->>Document: click eligible image or Mermaid SVG
Document->>ImageLightbox: dispatch global click event
ImageLightbox->>PortalDialog: set modal state and render portal
PortalDialog->>Document: toggle body class and manage focus
User->>PortalDialog: close by control, backdrop, Escape, or content
PortalDialog->>Document: restore focus and remove listeners
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/theme/utils/useImageLightbox.js (1)
236-251: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse one owner for the global lightbox lifecycle.
useImageLightboxinstalls and tears down the global click listener and lightbox DOM together.src/theme/Layout/index.js,src/theme/DocItem/Layout/index.js, andsrc/theme/BlogPostItem/Container/index.jsall mount it, but only the first instance may initialize it. If a later instance unmounts while another remains mounted, the cleanup returns the listener to an uninitialized state. Store the lifecycle in the persistent layout owner, or use reference counting so cleanup only runs when the last owner unmounts.🤖 Prompt for 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. In `@src/theme/utils/useImageLightbox.js` around lines 236 - 251, Update useImageLightbox so the global click listener and lightbox DOM have a single persistent lifecycle owner, or add reference counting for all mounted consumers. Ensure cleanup in useImageLightbox only removes the listener, resets window.__hamiLightboxInitialized, and invokes __hamiLightboxCleanup when the final owner unmounts, preventing a later instance from disabling an active lightbox.
🤖 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 `@src/theme/MDXComponents/Img/index.js`:
- Around line 4-10: Update the isLogo heuristic in the MDX image component to
use word-boundary matching: match logo and avatar as complete className tokens,
and logo as a complete word in trimmedAlt, rather than arbitrary substrings.
Preserve the existing trimming, case-insensitive behavior, and figure/caption
handling.
In `@src/theme/utils/useImageLightbox.js`:
- Around line 138-142: Update __hamiLightboxCleanup to remove the
hami-lightbox-open class from document.body during cleanup, alongside removing
the keydown listener and lightbox root, so unmounting an open lightbox clears
all body state.
- Around line 32-64: The sanitizeSvgIds function currently scans only
descendants, leaving root SVG IDREF attributes stale. Update its
attribute-rewrite pass to inspect the clone root as well as
clone.querySelectorAll("*"), and rewrite tokenized
aria-labelledby/aria-describedby ID references without “#” using idMap while
preserving existing descendant reference handling.
---
Outside diff comments:
In `@src/theme/utils/useImageLightbox.js`:
- Around line 236-251: Update useImageLightbox so the global click listener and
lightbox DOM have a single persistent lifecycle owner, or add reference counting
for all mounted consumers. Ensure cleanup in useImageLightbox only removes the
listener, resets window.__hamiLightboxInitialized, and invokes
__hamiLightboxCleanup when the final owner unmounts, preventing a later instance
from disabling an active lightbox.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 93d35f27-70d0-42a1-902b-4819e7f7a993
📒 Files selected for processing (5)
src/client/imageFigureNumber.jssrc/css/custom.csssrc/theme/MDXComponents/Img/index.jssrc/theme/MDXComponents/index.jssrc/theme/utils/useImageLightbox.js
…ycle, and SVG ID sanitization Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
Pushed a follow-up commit addressing the feedback:
|
|
Hi @mesutoezdil @rootsongjc - all review feedback and automated checks have been addressed in the latest commits. Whenever you get a moment, could you please take a look for review? Thank you! |
|
there are several bugs in this code, the acceptance criteria in #691 are not met, and the contribution rules of this website repo are not followed, pls rework. |
…s management Replaces the imperative document.body DOM node and cross-component reference-counting hack with a single ImageLightbox component rendered via ReactDOM.createPortal from the root Layout. DocItem/Layout and BlogPostItem/Container no longer need to mount the hook themselves, since Layout already wraps every page - removing the redundant multi-owner lifecycle that previously required activeConsumers bookkeeping. Also adds focus management: the previously focused element is restored on close, and Tab is trapped on the dialog's only focusable control while open. Tightens the logo/avatar heuristic to match whole class tokens instead of relying on \b boundaries against hyphenated names. Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
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 `@src/theme/Lightbox/index.js`:
- Around line 137-163: Update the parentLink interception in the lightbox click
handler to call event.preventDefault() and open the lightbox only for an
unmodified primary-button click. Allow Ctrl-click, Cmd-click, Shift-click,
Alt-click, and non-primary clicks—including middle-click—to follow normal
browser link behavior.
- Around line 33-76: Update sanitizeSvgIds so each oldId is regex-escaped before
interpolation into the RegExp used to replace URL references. Keep the existing
ID mapping and aria attribute handling unchanged, while ensuring IDs containing
regex metacharacters are matched literally.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 6fab492c-6555-4078-bdbc-464a44d3724c
📒 Files selected for processing (6)
src/theme/BlogPostItem/Container/index.jssrc/theme/DocItem/Layout/index.jssrc/theme/Layout/index.jssrc/theme/Lightbox/index.jssrc/theme/MDXComponents/Img/index.jssrc/theme/utils/useImageLightbox.js
💤 Files with no reviewable changes (3)
- src/theme/BlogPostItem/Container/index.js
- src/theme/DocItem/Layout/index.js
- src/theme/utils/useImageLightbox.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/theme/MDXComponents/Img/index.js
| function sanitizeSvgIds(clone) { | ||
| const uniqueSuffix = `-lb-${Math.random().toString(36).substring(2, 8)}`; | ||
| const allElements = [clone, ...Array.from(clone.querySelectorAll("*"))]; | ||
| const idMap = new Map(); | ||
|
|
||
| allElements.forEach((el) => { | ||
| const oldId = el.getAttribute("id"); | ||
| if (oldId) { | ||
| const newId = `${oldId}${uniqueSuffix}`; | ||
| idMap.set(oldId, newId); | ||
| el.setAttribute("id", newId); | ||
| } | ||
| }); | ||
|
|
||
| if (idMap.size > 0) { | ||
| allElements.forEach((el) => { | ||
| Array.from(el.attributes).forEach((attr) => { | ||
| let val = attr.value; | ||
| let changed = false; | ||
|
|
||
| idMap.forEach((newId, oldId) => { | ||
| if (val.includes(`#${oldId}`)) { | ||
| val = val.replace(new RegExp(`#${oldId}\\b`, "g"), `#${newId}`); | ||
| changed = true; | ||
| } | ||
|
|
||
| if (attr.name === "aria-labelledby" || attr.name === "aria-describedby") { | ||
| const tokens = val.split(/\s+/); | ||
| if (tokens.includes(oldId)) { | ||
| val = tokens.map((t) => (t === oldId ? newId : t)).join(" "); | ||
| changed = true; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (changed) { | ||
| el.setAttribute(attr.name, val); | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return clone; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Mermaid.js guarantee generated SVG element IDs contain only alphanumeric, dash, and underscore characters?
💡 Result:
No, Mermaid.js does not provide a strict guarantee that generated SVG element IDs contain only alphanumeric, dash, and underscore characters [1][2][3]. While recent versions of Mermaid.js have implemented extensive scoping and prefixing strategies to prevent ID collisions (such as prefixing internal element IDs with a unique diagram ID) [1][3][4], these IDs are often derived from user-provided input, such as node labels, subgraph titles, or custom identifiers [5]. Key points regarding Mermaid.js ID generation include: - User-Influenced IDs: For components like flowchart subgraphs, Mermaid may use the provided title directly as part of an element's ID [5]. If a user provides a title containing special characters, those characters may be propagated into the SVG element's ID [5]. - Collision Prevention: To solve issues with non-unique IDs when multiple diagrams appear on the same page, Mermaid now routinely prefixes internal IDs (e.g., ${diagramId}-${originalId}) [1][3]. While this improves uniqueness, it does not sanitize the originalId portion to restricted character sets [1][3]. - No Explicit Sanitization: There is no documented enforcement or standard internal utility within the Mermaid codebase that forces all generated IDs to adhere strictly to alphanumeric, dash, and underscore characters. Developers integrating Mermaid who require strict ID validation should sanitize inputs or perform post-processing on the generated SVG output [6]. In summary, because Mermaid relies on user-supplied definition text to name certain elements, it does not guarantee that the resulting SVG element IDs will be limited to a specific subset of safe characters [5].
Citations:
- 1: Bug/5741 fix duplicate svg element ids mermaid-js/mermaid#7410
- 2: mermaid-js/mermaid@7fb25b9
- 3: mermaid-js/mermaid@0fe25bf
- 4: feat(architecture): Add ids in generated SVG mermaid-js/mermaid#6921
- 5: ID of flowchart subgraph SVG element breaks other scripts mermaid-js/mermaid#4948
- 6: https://www.mintlify.com/mermaid-js/mermaid/advanced/rendering-options
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate files =="
git ls-files | rg '(^|/)package\.json$|Lightbox/index\.js$|Lightbox/package\.json$' || true
echo
echo "== Lightbox location =="
fd -a 'index\.js$' . | rg 'Lightbox/index\.js' || true
echo
echo "== Lightbox index outline if available =="
if [ -f src/theme/Lightbox/index.js ]; then
wc -l src/theme/Lightbox/index.js
ast-grep outline src/theme/Lightbox/index.js || true
sed -n '1,120p' src/theme/Lightbox/index.js
fi
echo
echo "== Search Mermaid version/user-input context =="
rg -n '"mermaid|mermaidjs|\\.mermaid|sanitizeSvgIds|import Mermaid|render\\(' -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: Project-HAMi/website
Length of output: 4368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json mermaid dependency/context =="
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
const names = [...(pkg.dependencies||{}), ...(pkg.devDependencies||{}), ...(pkg.peerDependencies||{})].filter(String).sort();
for (const [name, version] of Object.entries(pkg.dependencies||{})) {
if (name.includes('mermaid') || name.includes('docusaurus')) console.log('dep', name, version);
}
for (const [name, version] of Object.entries(pkg.devDependencies||{})) {
if (name.includes('mermaid') || name.includes('docusaurus')) console.log('dev', name, version);
}
JS
echo
echo "== Lightbox rest =="
sed -n '120,281p' src/theme/Lightbox/index.js
echo
echo "== repository mermaid references =="
rg -n 'mermaid|sanitizeSvgIds|docusaurus-mermaid-container' -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -300Repository: Project-HAMi/website
Length of output: 936
🏁 Script executed:
#!/bin/bash
set - euo pipefail
echo "== package.json =="
cat package.json
echo
echo "== Lightbox click handler =="
sed -n '120,281p' src/theme/Lightbox/index.js
echo
echo "== repository mermaid references =="
rg -n 'mermaid|sanitizeSvgIds|docusaurus-mermaid-container' -S . --glob '!node_modules' --glob '!dist' --glob '!build' | head -300Repository: Project-HAMi/website
Length of output: 17134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== regex metachar case probe =="
node - <<'JS'
function sanitizeSvgIdsImpl(cloneText) {
const clone = document.implementation.createHTMLDocument().createElement('svg');
clone.innerHTML = cloneText;
const uniqueSuffix = `-lb-${Math.random().toString(36).substring(2, 8)}`;
const allElements = [clone, ...Array.from(clone.querySelectorAll("*"))];
const idMap = new Map();
allElements.forEach((el) => {
const oldId = el.getAttribute("id");
if (oldId) idMap.set(oldId, `${oldId}${uniqueSuffix}`);
});
try {
allElements.forEach((el) => {
Array.from(el.attributes).forEach((attr) => {
let val = attr.value;
let changed = false;
idMap.forEach((newId, oldId) => {
if (val.includes(`#${oldId}`)) {
val = val.replace(new RegExp(`#${oldId}\\b`, "g"), `#${newId}`);
changed = true;
}
if (attr.name === "aria-labelledby" || attr.name === "aria-describedby") {
const tokens = val.split(/\s+/);
if (tokens.includes(oldId)) {
val = tokens.map((t) => (t === oldId ? newId : t)).join(" ");
changed = true;
}
}
});
if (changed) el.setAttribute(attr.name, val);
});
});
return { ok: true, outerId: clone.getAttribute("id"), innerLabel: Array.from(clone.querySelectorAll("*")).slice(0, 1).map(el => el.id ?? null) };
} catch (error) {
return { ok: false, error: String(error && error.message && error.message.slice(0, 200) || error) };
}
}
for (const label of ["A", "A.B", "A+B", "A(B)"]) {
const out = sanitizeSvgIdsImpl(String.raw`<svg id="root-${label}"><a id="node-${label}" href="x"><label>${label}</label></a></svg>`);
console.log(label, JSON.stringify(out));
}
JSRepository: Project-HAMi/website
Length of output: 850
🌐 Web query:
Mermaid 11 node ID characters generated SVG ids subgraph ids title allowed characters
💡 Result:
In Mermaid, node and subgraph identifiers (IDs) are generally recommended to follow alphanumeric conventions (A-Za-z0-9_) to ensure compatibility and prevent syntax errors [1][2]. While early versions of Mermaid were more permissive, using IDs that begin with numbers or contain special characters can lead to parsing issues in certain diagram types [3][4][1]. Key guidelines for IDs and titles include: Node IDs: The most robust approach is to use alphanumeric identifiers, such as CamelCase or snake_case, avoiding spaces and special characters [2]. If a node label needs to contain spaces or special characters, define the node with a unique alphanumeric ID and use brackets to specify the display label (e.g., nodeID[Display Label with Spaces]) [3][5][2]. Reserved keywords (like default, style, end, or flowchart) should be avoided as node IDs, as they can conflict with the parser [3]. Subgraph IDs: Similar to node IDs, subgraph IDs should ideally start with a letter and contain only alphanumeric characters or underscores [1][2]. If a subgraph requires a complex title with spaces, it should be defined with an explicit alphanumeric ID and a separate title in brackets (e.g., subgraph subID[Subgraph Title with Spaces]) [6][2]. SVG ID Generation: To prevent DOM ID collisions when multiple Mermaid diagrams are embedded on the same page, Mermaid now namespaces internal SVG element IDs (such as nodes, edges, and markers) by prefixing them with the unique ID of the SVG container [7][8][9]. The resulting format generally follows the pattern: {diagramID}-{originalElementID} [7][8][10]. Title Characters: Mermaid titles (for nodes, subgraphs, or graphs) are more flexible than IDs, especially when enclosed in quotes or brackets, allowing for a wider range of characters, including spaces and symbols [11][6][2]. Always quote or bracket titles if they contain spaces or special characters to ensure the parser correctly distinguishes them from structural keywords [3][6][2].
Citations:
- 1: subgraph fails to render when subgraph ID starts with a number mermaid-js/mermaid#900
- 2: https://gist.github.com/tbowman01/33bd5c73592df84c8ff6698a223b532c
- 3: Node names with special characters that break syntax mermaid-js/mermaid#2388
- 4: fix(er-diagram): handle syntax errors for special characters in node names mermaid-js/mermaid#6984
- 5: How to manage node id names in flowchart? mermaid-js/mermaid#2650
- 6: Support styling of subgraphs mermaid-js/mermaid#845
- 7: Bug/5741 fix duplicate svg element ids mermaid-js/mermaid#7410
- 8: mermaid-js/mermaid@0fe25bf
- 9: Fix/5741 unique marker ids mermaid-js/mermaid#5756
- 10: feat(architecture): Add ids in generated SVG mermaid-js/mermaid#6921
- 11: Bug/4645 graph node containing keyword mermaid-js/mermaid#4657
🌐 Web query:
Mermaid 11 generated SVG element ids subgraph titles regex ID regex escape
💡 Result:
In Mermaid 11 and later, SVG element IDs—including those for subgraphs—are dynamically scoped to ensure uniqueness when multiple diagrams appear on the same page [1][2]. Key behaviors regarding element IDs and subgraph titles: 1. Dynamic Namespacing: To prevent DOM ID collisions (which historically caused issues where subgraph IDs acted as global variables or conflicted with other diagrams), Mermaid now prefixes internal IDs with a unique diagram identifier [3][2]. The internal logic generally follows the pattern: <diagramId>-<originalId> [4][2]. 2. Subgraph ID Assignment: You can define an explicit ID for a subgraph using the subgraph id[Title] syntax [5]. If an explicit ID is not provided, Mermaid generates one, often utilizing an internal counter (e.g., based on subCount) to maintain uniqueness within that specific diagram context [4]. 3. Title Handling and Escaping: - Titles with spaces or special characters should be wrapped in double quotes (e.g., subgraph "My Subgraph Title") to ensure they are correctly parsed as labels rather than syntax tokens [6]. - Mermaid includes utility functions to sanitize and escape text used in labels and IDs [4][7]. Characters like brackets [ and ] often require special handling or escaping if they are part of a title, as they are used in Mermaid's syntax to define IDs [5][6][7]. - If you need to manually handle or match these generated IDs in your own scripts (e.g., via CSS selectors), it is recommended to use attribute selectors like [id^="prefix-"] or [id$="-suffix"] rather than strict #id matching, as the exact ID string is dynamically generated and may include the unique diagram container ID [8]. In summary, do not rely on static, predictable IDs for subgraphs across different renders or multiple diagrams. Rely instead on the CSS classes provided by Mermaid or flexible attribute selectors for any custom DOM manipulation [2][8].
Citations:
- 1: Bug/5741 fix duplicate svg element ids mermaid-js/mermaid#7410
- 2: mermaid-js/mermaid@0fe25bf
- 3: ID of flowchart subgraph SVG element breaks other scripts mermaid-js/mermaid#4948
- 4: https://github.com/mermaid-js/mermaid/blob/8b52e537/packages/mermaid/src/diagrams/flowchart/flowDb.ts
- 5: Support styling of subgraphs mermaid-js/mermaid#845
- 6: Spaces in subgraph titles no longer render in Mermaid 8.2.3 mermaid-js/mermaid#895
- 7: https://github.com/flowr-analysis/flowr/blob/67c37bb18e978f432e341b0c2694242cf53410d7/src/util/mermaid/mermaid.ts
- 8: mermaid-js/mermaid@107edad
Escape oldId before building RegExp in sanitizeSvgIds.
Mermaid SVG IDs are reused from diagram identifiers and are not limited to alphanumeric characters. Any regex metacharacter in oldId can make this pattern invalid or match unexpected text inside attributes from the SVG copy. Escape the value before using it in new RegExp.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 54-54: Detects non-literal values in regular expressions
Context: new RegExp(#${oldId}\\b, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🤖 Prompt for 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.
In `@src/theme/Lightbox/index.js` around lines 33 - 76, Update sanitizeSvgIds so
each oldId is regex-escaped before interpolation into the RegExp used to replace
URL references. Keep the existing ID mapping and aria attribute handling
unchanged, while ensuring IDs containing regex metacharacters are matched
literally.
| const image = target instanceof HTMLImageElement ? target : target.closest("img"); | ||
| if (!shouldOpenLightbox(image)) { | ||
| return; | ||
| } | ||
|
|
||
| const parentLink = image.closest("a"); | ||
| if (parentLink) { | ||
| const href = parentLink.getAttribute("href") || ""; | ||
| const sameAsImage = | ||
| href === image.currentSrc || href === image.src || href === image.getAttribute("src"); | ||
| if (!sameAsImage && !isImageHref(href)) { | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
| } | ||
|
|
||
| const caption = getImageCaption(image); | ||
| lastFocusedRef.current = document.activeElement; | ||
| setState({ | ||
| open: true, | ||
| mode: "image", | ||
| src: image.currentSrc || image.src, | ||
| alt: image.alt || caption, | ||
| caption, | ||
| svg: null, | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect modifier-key and middle-click behavior for image links.
At line 150, event.preventDefault() runs unconditionally whenever parentLink should be intercepted. This blocks Ctrl-click, Cmd-click, Shift-click, and middle-click, which browsers normally use to open the link in a new tab or window. Add a guard so the lightbox only intercepts a plain primary click.
🖱️ Proposed fix to respect modifier keys and non-primary clicks
const parentLink = image.closest("a");
if (parentLink) {
+ if (event.button !== 0 || event.ctrlKey || event.metaKey || event.shiftKey) {
+ return;
+ }
const href = parentLink.getAttribute("href") || "";
const sameAsImage =
href === image.currentSrc || href === image.src || href === image.getAttribute("src");
if (!sameAsImage && !isImageHref(href)) {
return;
}
event.preventDefault();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const image = target instanceof HTMLImageElement ? target : target.closest("img"); | |
| if (!shouldOpenLightbox(image)) { | |
| return; | |
| } | |
| const parentLink = image.closest("a"); | |
| if (parentLink) { | |
| const href = parentLink.getAttribute("href") || ""; | |
| const sameAsImage = | |
| href === image.currentSrc || href === image.src || href === image.getAttribute("src"); | |
| if (!sameAsImage && !isImageHref(href)) { | |
| return; | |
| } | |
| event.preventDefault(); | |
| } | |
| const caption = getImageCaption(image); | |
| lastFocusedRef.current = document.activeElement; | |
| setState({ | |
| open: true, | |
| mode: "image", | |
| src: image.currentSrc || image.src, | |
| alt: image.alt || caption, | |
| caption, | |
| svg: null, | |
| }); | |
| }, []); | |
| const image = target instanceof HTMLImageElement ? target : target.closest("img"); | |
| if (!shouldOpenLightbox(image)) { | |
| return; | |
| } | |
| const parentLink = image.closest("a"); | |
| if (parentLink) { | |
| if (event.button !== 0 || event.ctrlKey || event.metaKey || event.shiftKey) { | |
| return; | |
| } | |
| const href = parentLink.getAttribute("href") || ""; | |
| const sameAsImage = | |
| href === image.currentSrc || href === image.src || href === image.getAttribute("src"); | |
| if (!sameAsImage && !isImageHref(href)) { | |
| return; | |
| } | |
| event.preventDefault(); | |
| } | |
| const caption = getImageCaption(image); | |
| lastFocusedRef.current = document.activeElement; | |
| setState({ | |
| open: true, | |
| mode: "image", | |
| src: image.currentSrc || image.src, | |
| alt: image.alt || caption, | |
| caption, | |
| svg: null, | |
| }); | |
| }, []); |
🤖 Prompt for 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.
In `@src/theme/Lightbox/index.js` around lines 137 - 163, Update the parentLink
interception in the lightbox click handler to call event.preventDefault() and
open the lightbox only for an unmodified primary-button click. Allow Ctrl-click,
Cmd-click, Shift-click, Alt-click, and non-primary clicks—including
middle-click—to follow normal browser link behavior.
|
Hey @mesutoezdil, thanks for the honest push-back — fair call. Looking back at #691's acceptance criteria, the lightbox was still doing imperative DOM manipulation (creating and appending the modal node by hand, with a reference-counting hack across three separate mount points) instead of the declarative React Portal you asked for. That patchwork was also the root of the fragile multi-owner lifecycle CodeRabbit flagged earlier. Reworked it properly in the latest commit:
Verified with a full |
Reminder: Answers must be written by human being. You can view the relevant rule here. |
|
Fixed both points, thanks for the sharp eyes:
Pushed, build's clean, CI's green. |
2 participants
Summary of Changes
Fixes #691.
This PR addresses and resolves the client-side media rendering and interactivity defects reported in #691:
React-Native MDX Component Wrapper (
src/theme/MDXComponents/Img/index.js+src/theme/MDXComponents/index.js):insertBefore,appendChild) insrc/client/imageFigureNumber.jsthat previously modified nodes outside React's Virtual DOM.MDXComponents/Imgcomponent wrapper that renders Markdown images inside<figure>and<figcaption>elements during React rendering.DOMException: Failed to execute 'removeChild' on 'Node'crashes when state updates (tab toggling, color theme switching, version dropdown changes) occur on pages with images.CSS Counter System & Reactive i18n Captions (
src/css/custom.css):counter-reset: hami-figure-counterandcounter-increment: hami-figure-counter).html[lang^="zh"]selectors for instantaneous, reactive caption prefix rendering (Figure X:in English vs图 X:in Chinese) without JS timers or stale text regex matching.Lightbox Portal Cleanup (
src/theme/utils/useImageLightbox.js):root.__hamiLightboxCleanup) so.hami-lightboxDOM overlay elements and keydown listeners are properly removed fromdocument.bodyupon component unmount, preventing memory leaks and stale closure bindings across SPA navigations.Mermaid SVG ID Namespacing (
src/theme/utils/useImageLightbox.js):idattributes (<defs>,<clipPath>,<linearGradient>, marker arrows) andurl(#id)references when cloning Mermaid SVGs into the lightbox overlay.idcollisions with background diagrams, rendering marker arrows and gradients cleanly inside the lightbox modal.Verification
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>).Summary by CodeRabbit
New Features
Bug Fixes