Add simulation gallery entries and computer links #1160
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -806,6 +806,25 @@ if (MFC_DOCUMENTATION) | |||||||||||||||||||||||||||||||||||
| GEN_DOCS(post_process "MFC: Post-Process") | ||||||||||||||||||||||||||||||||||||
| GEN_DOCS(documentation "MFC") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| # Inject per-page last-updated dates into documentation markdown files. | ||||||||||||||||||||||||||||||||||||
| # Runs after auto-generated .md files exist, before Doxygen processes them. | ||||||||||||||||||||||||||||||||||||
| # Uses a stamp file so it only runs once per build. | ||||||||||||||||||||||||||||||||||||
| add_custom_command( | ||||||||||||||||||||||||||||||||||||
| OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/inject-dates.stamp" | ||||||||||||||||||||||||||||||||||||
| DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/docs/documentation/examples.md" | ||||||||||||||||||||||||||||||||||||
| "${CMAKE_CURRENT_SOURCE_DIR}/docs/documentation/case_constraints.md" | ||||||||||||||||||||||||||||||||||||
| "${CMAKE_CURRENT_SOURCE_DIR}/docs/documentation/physics_constraints.md" | ||||||||||||||||||||||||||||||||||||
| "${CMAKE_CURRENT_SOURCE_DIR}/docs/documentation/cli-reference.md" | ||||||||||||||||||||||||||||||||||||
| "${CMAKE_CURRENT_SOURCE_DIR}/docs/documentation/parameters.md" | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+814
to
+818
There was a problem hiding this comment. The custom command dependency list in CMakeLists.txt includes the generated markdown files that the script is supposed to modify. This creates a circular dependency issue: the script modifies these files, which are listed as dependencies, which would trigger the script to run again. The stamp file approach helps, but the dependencies should be the script itself and the files it reads from, not the files it writes to.
Suggested change
Comment on lines
+814
to
+818
Contributor
There was a problem hiding this comment. P2: The inject-dates custom command runs docs/inject-dates.py but doesn’t list that script in DEPENDS, so updates to the script won’t retrigger the stamp and documentation can go stale without a clean build. Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||||||||||||
| COMMAND "${Python3_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/docs/inject-dates.py" | ||||||||||||||||||||||||||||||||||||
| "${CMAKE_CURRENT_SOURCE_DIR}" | ||||||||||||||||||||||||||||||||||||
| COMMAND "${CMAKE_COMMAND}" -E touch "${CMAKE_CURRENT_BINARY_DIR}/inject-dates.stamp" | ||||||||||||||||||||||||||||||||||||
| COMMENT "Injecting page dates into documentation" | ||||||||||||||||||||||||||||||||||||
| VERBATIM | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Suggestion: Add VERBATIM to the new custom command so that the Python invocation and paths are safely quoted across platforms, matching the other custom commands in this file. [custom_rule] Severity Level: Minor
Suggested change
Why it matters? ⭐Adding VERBATIM matches the pattern used elsewhere in this CMakeLists (other add_custom_command invocations include VERBATIM) and ensures CMake does not perform platform-specific tokenization/escaping on the Python command and its arguments. This improves robustness/portability of the custom command and directly addresses a portability/consistency concern in the diff. The change is syntactically valid CMake and fixes a real potential issue rather than being purely cosmetic. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** CMakeLists.txt
**Line:** 823:823
**Comment:**
*Custom Rule: Add VERBATIM to the new custom command so that the Python invocation and paths are safely quoted across platforms, matching the other custom commands in this file.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||
| add_custom_target(inject_page_dates DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/inject-dates.stamp") | ||||||||||||||||||||||||||||||||||||
| add_dependencies(documentation_doxygen inject_page_dates) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+825
to
+826
There was a problem hiding this comment. The inject_page_dates target is added as a dependency of documentation_doxygen, but the correct ordering might need verification. The script should run after the markdown files are generated (which happens as part of documentation_doxygen's dependencies) but before Doxygen processes them. The current setup may not guarantee this ordering. Consider making inject_page_dates depend on the commands that generate the markdown files, and having documentation_doxygen depend on inject_page_dates. |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| # > Copy Resources (main landing page & assets) | ||||||||||||||||||||||||||||||||||||
| install(DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/docs/res" | ||||||||||||||||||||||||||||||||||||
| DESTINATION "docs/mfc") | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,20 +23,54 @@ | |||||||||||||||
| <meta property="og:url" content="https://mflowcode.github.io" /> | ||||||||||||||||
| <meta property="og:type" content="website" /> | ||||||||||||||||
| <meta name="twitter:card" content="summary_large_image" /> | ||||||||||||||||
| <script type="application/ld+json"> | ||||||||||||||||
| { | ||||||||||||||||
| "@context": "https://schema.org", | ||||||||||||||||
| "@type": "SoftwareSourceCode", | ||||||||||||||||
| "name": "MFC", | ||||||||||||||||
| "alternateName": "Multi-Component Flow Code", | ||||||||||||||||
| "description": "Open-source exascale multiphase flow solver. 2025 Gordon Bell Prize Finalist. Scales to 200+ trillion grid points on 43,000+ GPUs.", | ||||||||||||||||
| "url": "https://mflowcode.github.io", | ||||||||||||||||
| "license": "https://opensource.org/licenses/MIT", | ||||||||||||||||
| "programmingLanguage": ["Fortran", "Python"], | ||||||||||||||||
| "codeRepository": "https://github.com/MFlowCode/MFC", | ||||||||||||||||
| "author": { | ||||||||||||||||
| "@type": "Organization", | ||||||||||||||||
| "name": "MFlowCode", | ||||||||||||||||
| "url": "https://github.com/MFlowCode" | ||||||||||||||||
| }, | ||||||||||||||||
| "award": "2025 ACM Gordon Bell Prize Finalist" | ||||||||||||||||
| } | ||||||||||||||||
| </script> | ||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| <script src="https://cdn.tailwindcss.com"></script> | ||||||||||||||||
| <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.0/css/all.min.css"> | ||||||||||||||||
| <link rel="icon" type="image/x-icon" href="res/icon.ico"> | ||||||||||||||||
| <script> | ||||||||||||||||
| const sims = [ | ||||||||||||||||
| // Rockets & jets | ||||||||||||||||
| { name: "Mach 12 Starship Super Heavy", image: "res/simulations/o.png", computer: "Alps", computerUrl: "https://www.cscs.ch/computers/alps/", accelerators: "5K GH200s", walltime: "18h", source: "https://www.youtube.com/watch?v=NSn3OVF8N4I" }, | ||||||||||||||||
| { name: "Mach 10 triple jet booster", image: "res/simulations/n.png", computer: "Frontier", computerUrl: "https://www.olcf.ornl.gov/frontier/", accelerators: "10K GCDs", walltime: "12h", source: "https://www.youtube.com/watch?v=pMUl55xqGgM" }, | ||||||||||||||||
| { name: "Mach 2 flow over a sphere", image: "res/simulations/i.png", computer: "Phoenix", computerUrl: "https://www.pace.gatech.edu/", accelerators: "36 V100s", walltime: "30m", source: "https://www.youtube.com/watch?v=HQGSUvYEGqM" }, | ||||||||||||||||
| { name: "Mach 2 shear layer", image: "res/simulations/j.png", computer: "Phoenix", computerUrl: "https://www.pace.gatech.edu/", accelerators: "32 V100s", walltime: "15m", source: "https://www.youtube.com/watch?v=GtcdCHLmJO8" }, | ||||||||||||||||
| // Aerodynamics | ||||||||||||||||
| { name: "Flow over an airfoil (vorticity)", image: "res/simulations/g.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "128 A100s", walltime: "19h", source: "https://www.youtube.com/watch?v=FvAgnBW59cY" }, | ||||||||||||||||
| { name: "Pitching airfoil (3D)", image: "res/simulations/m.png", computer: "Phoenix", computerUrl: "https://www.pace.gatech.edu/", accelerators: "1 A100", walltime: "5h", source: "https://www.youtube.com/watch?v=2XH-9MumDHU" }, | ||||||||||||||||
| // Shock-droplet | ||||||||||||||||
| { name: "Shedding water droplet", image: "res/simulations/a.png", computer: "Summit", computerUrl: "https://www.olcf.ornl.gov/summit/", accelerators: "960 V100s", walltime: "4h", source: "https://www.youtube.com/watch?v=Gjj-qZkXcrg" }, | ||||||||||||||||
| // Biomedical & acoustics | ||||||||||||||||
| { name: "Burstwave lithotripsy", image: "res/simulations/k.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "128 A100s", walltime: "30m", source: "https://www.youtube.com/watch?v=XWsUTaJXGF8" }, | ||||||||||||||||
| { name: "Cavitation fragments kidney stone", image: "res/simulations/d.png", computer: "Summit", computerUrl: "https://www.olcf.ornl.gov/summit/", accelerators: "576 V100s", walltime: "30m", source: "https://doi.org/10.48550/arXiv.2305.09163" }, | ||||||||||||||||
| { name: "Kidney stone stress waves", image: "res/simulations/l.png", computer: "Bridges2", computerUrl: "https://www.psc.edu/resources/bridges-2/", accelerators: "8 V100s", walltime: "20m", source: "https://www.youtube.com/watch?v=Q2L0J68qnRw" }, | ||||||||||||||||
| { name: "Whale bubble net feeding", image: "res/simulations/p.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "128 A100s", walltime: "30m", source: "https://www.youtube.com/watch?v=6EpP6tdCZSA" }, | ||||||||||||||||
| { name: "Earplug acoustics (kinetic energy)", image: "res/simulations/q.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "8 A100s", walltime: "5h", source: "https://www.youtube.com/watch?v=xSW5wZkdbrc" }, | ||||||||||||||||
| { name: "Circular orifice (1 kHz)", image: "res/simulations/r.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "16 A100s", walltime: "5h", source: "https://www.youtube.com/watch?v=jOhJ_c7eco4" }, | ||||||||||||||||
| // Bubble dynamics | ||||||||||||||||
| { name: "Collapsing bubbles (pressure)", image: "res/simulations/b.png", computer: "Summit", computerUrl: "https://www.olcf.ornl.gov/summit/", accelerators: "216 V100s", walltime: "3h", source: "https://doi.org/10.48550/arXiv.2305.09163" }, | ||||||||||||||||
| { name: "Collapsing bubbles (streamlines)", image: "res/simulations/c.png", computer: "Summit", computerUrl: "https://www.olcf.ornl.gov/summit/", accelerators: "216 V100s", walltime: "3h", source: "https://doi.org/10.48550/arXiv.2305.09163" }, | ||||||||||||||||
| { name: "Euler-Lagrange particle cloud", image: "res/simulations/s.png", computer: "Phoenix", computerUrl: "https://www.pace.gatech.edu/", accelerators: "8 A100s", walltime: "<1h", source: "https://www.youtube.com/watch?v=RoT-yC5Lxmg" }, | ||||||||||||||||
| // Fundamentals | ||||||||||||||||
| { name: "Breakup of vibrated interface", image: "res/simulations/f.png", computer: "Summit", computerUrl: "https://www.olcf.ornl.gov/summit/", accelerators: "128 V100s", walltime: "4h", source: "https://www.youtube.com/watch?v=XQ3g1oSg8mc" }, | ||||||||||||||||
| { name: "Viscous Taylor-Green vortex", image: "res/simulations/h.png", computer: "Delta", computerUrl: "https://www.ncsa.illinois.edu/research/project-highlights/delta/", accelerators: "128 A100s", walltime: "17h", source: "https://www.youtube.com/watch?v=7i2h08dlDQw" }, | ||||||||||||||||
| ]; | ||||||||||||||||
|
|
||||||||||||||||
| const scalings = [ | ||||||||||||||||
|
|
@@ -54,14 +88,14 @@ | |||||||||||||||
| <img class="h-10" src="res/logo.png" alt=""> | ||||||||||||||||
| </div> | ||||||||||||||||
| <div class="flex-1 p-2 font-semibold text-center">${s.name}</div> | ||||||||||||||||
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank"> | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. P2: Add rel="noopener noreferrer" to the external link opened with target="_blank" to prevent window.opener security risks. Prompt for AI agents
Suggested change
Contributor
There was a problem hiding this comment. Suggestion: Using target="_blank" on external links without rel="noopener noreferrer" allows the opened page to access window.opener and potentially redirect the original page (reverse tabnabbing), so the link to simulation sources should include rel attributes to prevent this. [security] Severity Level: Major
|
||||||||||||||||
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank"> | |
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank" rel="noopener noreferrer"> |
Steps of Reproduction ✅
1. Open `docs/index.html` in a browser (homepage entry point).
2. On DOMContentLoaded, script block at `docs/index.html:82-99` populates `#ft-sim` with
cards, rendering anchor at `docs/index.html:93` as `<a ... href="${s.source}"
target="_blank">` without `rel`.
3. Click any simulation source link (e.g., a YouTube or DOI URL) so it opens in a new tab
due to `target="_blank"`.
4. In the DevTools console of the newly opened tab, execute `window.opener.location =
'https://example.com'` and observe the original `docs/index.html` tab is navigated away,
demonstrating the reverse‑tabnabbing risk caused by missing `rel="noopener noreferrer"`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/index.html
**Line:** 93:93
**Comment:**
*Security: Using target="_blank" on external links without rel="noopener noreferrer" allows the opened page to access window.opener and potentially redirect the original page (reverse tabnabbing), so the link to simulation sources should include rel attributes to prevent this.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Suggestion: Add a rel attribute to the external link opened in a new tab to prevent the new page from accessing the opener context. [custom_rule]
Severity Level: Minor
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank"> | |
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank" rel="noopener noreferrer"> |
Why it matters? ⭐
Adding rel="noopener noreferrer" mitigates reverse tabnabbing and prevents the opened page from accessing window.opener. This is a security best-practice for links using target="_blank" and directly improves the code's safety without changing behavior for legitimate destinations.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/index.html
**Line:** 93:93
**Comment:**
*Custom Rule: Add a rel attribute to the external link opened in a new tab to prevent the new page from accessing the opener context.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
There was a problem hiding this comment.
Add rel="noopener noreferrer" to all target="_blank" links
Both the source icon link (line 93) and the computer URL link (line 100) open a new tab without rel="noopener noreferrer". This leaves window.opener accessible in the opened tab and leaks the Referer header. These are straightforward to fix:
🔒 Proposed fix
- <a class="w-10 text-center text-xl" href="${s.source}" target="_blank">
+ <a class="w-10 text-center text-xl" href="${s.source}" target="_blank" rel="noopener noreferrer">- <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank">${s.computer}</a>` : s.computer}</div>
+ <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank" rel="noopener noreferrer">${s.computer}</a>` : s.computer}</div>Also applies to: 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/index.html` around lines 93 - 94, The anchor element rendering the
source icon (the <a class="w-10 text-center text-xl" href="${s.source}"
target="_blank"> element) and any other anchors that open new tabs should
include rel="noopener noreferrer"; update the JSX/HTML that generates this link
(and the computer URL link) to add rel="noopener noreferrer" whenever
target="_blank" is used so window.opener is not exposed and the referrer is not
leaked.
There was a problem hiding this comment.
Suggestion: Guard the use of the includes method on the source URL to avoid runtime errors if the field is ever missing or null. [custom_rule]
Severity Level: Minor
| <i class="${s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}"></i> | |
| <i class="${s.source && s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}"></i> |
Why it matters? ⭐
The proposed change (s.source && s.source.includes(...)) guards against a potential runtime TypeError if a future sims entry omitted source or set it to null/undefined. This is a correctness improvement (runtime-safety) rather than cosmetic, and the improved code is a small, syntactically valid defensive check that directly resolves that issue.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/index.html
**Line:** 94:94
**Comment:**
*Custom Rule: Guard the use of the `includes` method on the source URL to avoid runtime errors if the field is ever missing or null.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Source-link icon has no accessible label — screen readers cannot identify the link.
The <a> contains only a FontAwesome <i> icon with no visible text, aria-label, or title. Screen-reader users will hear an unlabelled link.
♿ Proposed fix
- <a class="w-10 text-center text-xl" href="${s.source}" target="_blank">
- <i class="${s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}"></i>
+ <a class="w-10 text-center text-xl" href="${s.source}" target="_blank" rel="noopener noreferrer"
+ aria-label="${s.source.includes('youtube.com') ? 'Watch on YouTube' : 'View source'}">
+ <i class="${s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}" aria-hidden="true"></i>📝 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.
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank"> | |
| <i class="${s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}"></i> | |
| </a> | |
| <a class="w-10 text-center text-xl" href="${s.source}" target="_blank" rel="noopener noreferrer" | |
| aria-label="${s.source.includes('youtube.com') ? 'Watch on YouTube' : 'View source'}"> | |
| <i class="${s.source.includes('youtube.com') ? 'fa-brands fa-youtube' : 'fa-solid fa-arrow-up-right-from-square'}" aria-hidden="true"></i> | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/index.html` around lines 93 - 95, The anchor that renders the source
icon (the <a> that uses s.source and contains the <i> element) lacks an
accessible label; update its markup to include an accessible name by adding an
aria-label (and optionally a title or visually-hidden text) that describes the
destination (e.g., "Open source on YouTube" when s.source includes
'youtube.com', otherwise "Open source link"); ensure the label text is generated
from s.source or context so screen readers receive a meaningful description and
keep the existing icon for visual users.
There was a problem hiding this comment.
P2: Add rel="noopener noreferrer" to the computer link opened with target="_blank" to avoid window.opener vulnerabilities.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/index.html, line 100:
<comment>Add rel="noopener noreferrer" to the computer link opened with target="_blank" to avoid window.opener vulnerabilities.</comment>
<file context>
@@ -54,14 +90,14 @@
<div class="flex flex-row items-center">
<div class="pr-2"><i class="fa-solid fa-server"></i></div>
- <div class="flex-1 text-center">${s.computer}</div>
+ <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank">${s.computer}</a>` : s.computer}</div>
</div>
<div class="flex flex-row items-center">
</file context>
| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank">${s.computer}</a>` : s.computer}</div> | |
| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank" rel="noopener noreferrer">${s.computer}</a>` : s.computer}</div> |
There was a problem hiding this comment.
Suggestion: The dynamically generated external links to HPC center pages also use target="_blank" without rel="noopener noreferrer", which can be abused by the new tab to manipulate the originating page, so the anchor should include rel attributes. [security]
Severity Level: Major ⚠️
- CRITICAL Docs homepage HPC links expose window.opener reference.
- WARN External HPC pages can navigate original documentation tab.
- WARN Security posture of external-resource links is weakened.| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank">${s.computer}</a>` : s.computer}</div> | |
| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank" rel="noopener noreferrer">${s.computer}</a>` : s.computer}</div> |
Steps of Reproduction ✅
1. Open `docs/index.html` in a browser (homepage).
2. DOMContentLoaded handler at `docs/index.html:82-107` renders the server row, producing
the template at `docs/index.html:100` with `<a href="${s.computerUrl}" ...
target="_blank">` and no `rel`.
3. Click any HPC center link (e.g., CSCS Alps, OLCF Frontier) so it opens in a new tab due
to `target="_blank"`.
4. In the DevTools console of that new HPC tab, run `window.opener.location =
'https://example.com'` and see the original docs tab navigated, confirming the
reverse‑tabnabbing condition from missing `rel="noopener noreferrer"`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/index.html
**Line:** 100:100
**Comment:**
*Security: The dynamically generated external links to HPC center pages also use target="_blank" without rel="noopener noreferrer", which can be abused by the new tab to manipulate the originating page, so the anchor should include rel attributes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Suggestion: Ensure that dynamically generated external computer links opened in a new tab also include a rel attribute for security. [custom_rule]
Severity Level: Minor
| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank">${s.computer}</a>` : s.computer}</div> | |
| <div class="flex-1 text-center">${s.computerUrl ? `<a href="${s.computerUrl}" class="underline hover:text-amber-400" target="_blank" rel="noopener noreferrer">${s.computer}</a>` : s.computer}</div> |
Why it matters? ⭐
Same as above: adding rel="noopener noreferrer" to programmatically-generated external links prevents window.opener access and improves security. The improved code is a minimal, correct fix that addresses a real security concern and is applicable to this PR hunk.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/index.html
**Line:** 100:100
**Comment:**
*Custom Rule: Ensure that dynamically generated external computer links opened in a new tab also include a rel attribute for security.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env python3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Inject last-updated dates into docs before Doxygen runs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Usage: python3 inject-dates.py [source_dir] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| source_dir defaults to current directory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| from datetime import date | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| src_dir = Path(sys.argv[1]) if len(sys.argv) > 1 else Path(".") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| docs_dir = src_dir / "docs" / "documentation" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| for md_file in sorted(docs_dir.glob("*.md")): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "Page last updated:" in md_file.read_text(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. The inject-dates.py script reads each markdown file in its entirety into memory on line 16, then modifies it by appending content. This can cause issues if the script is run multiple times on the same files, as the check "Page last updated:" only verifies presence anywhere in the file, not whether it's already been appended at the end. This could lead to duplicate date footers being appended on subsequent builds. Consider checking if the footer has already been added at the end of the file, or tracking which files have been processed using a more robust mechanism (e.g., a separate state file).
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = subprocess.run( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ["git", "log", "-1", "--format=%as", "--", str(md_file)], | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_output=True, text=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd=str(src_dir), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| page_date = result.stdout.strip() or str(date.today()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+19
to
+24
There was a problem hiding this comment. The subprocess.run call does not check the return code of the git command. If git fails (e.g., file not in git history, git not available, not in a git repository), the script silently falls back to today's date. Consider adding error handling or logging to make debugging easier if the git command fails unexpectedly.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open(md_file, "a") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Missing Without an explicit encoding, the file is opened using the platform default, which is not 🐛 Proposed fix- with open(md_file, "a") as f:
+ with open(md_file, "a", encoding="utf-8") as f:📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. P2: Add Prompt for AI agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"\n\n<div style='text-align:center; font-size:0.75rem; " | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"color:#888; padding:16px 0 0;'>" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Page last updated: {page_date}</div>\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+28
to
+31
There was a problem hiding this comment. The script uses f-strings spanning multiple lines (28-30) which may have readability issues. While this works, consider using parentheses to make the multi-line f-string more explicit, or breaking it into smaller parts for better maintainability.
Suggested change
Comment on lines
+15
to
+31
There was a problem hiding this comment. The script does not specify encoding when reading/writing files. While Python 3 defaults to UTF-8 on most platforms, it's better to be explicit about encoding to avoid issues on Windows or other platforms. Add encoding='utf-8' to both read_text() and the open() calls. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| User-agent: * | ||
| Allow: / | ||
|
|
||
| Sitemap: https://mflowcode.github.io/sitemap.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lychee now runs after publish — broken links won't block deployment on scheduled/dispatch runs.
Moving Lychee after the Publish step (with
fail: true) means that onschedule/workflow_dispatchtriggers, documentation is deployed to GitHub Pages before link checking occurs. A link-check failure will mark the workflow as failed but won't roll back the published content.On PRs this is not an issue (the publish step is skipped), so Lychee correctly gates PR merges. For production runs, this trade-off appears intentional per the PR description ("move publish step before lychee"), but it's worth confirming that alerting-on-failure (e.g., via GitHub notification or branch protection) is sufficient to catch post-deploy regressions.
🤖 Prompt for AI Agents