[blog] Publish feedback from Stellar about extending OpenWISP#205
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Mermaid to the npm build and site runtime (copies mermaid.min.js into theme/static/js and initializes it on article pages when article.mermaid is set), publishes a 571-line Stellar Telecommunications blog post about fork→extension migration and detailed database migration strategies, and updates Black, flake8, QA script syntax, and a requirements URL. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 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 `@content/blog/stellar_openwisp_feedback.rst`:
- Around line 44-53: Add a new documentation section describing the newly
introduced Mermaid diagram support: explain how to embed diagrams using the RST
.. raw:: html block and the required <pre class="mermaid"> wrapper, list
supported Mermaid diagram types (flowchart, sequence, class, etc.), note
limitations/best practices (escaping, sanitization, rendering caveats), and
include a short example snippet showing a minimal Mermaid block and expected
output; reference the sample in the blog post (the graph TD block) and add this
content to the site contributing/content-authoring docs so authors know how to
use Mermaid diagrams.
In `@theme/templates/base.html`:
- Around line 93-99: Change the external Mermaid script tag to load with the
defer attribute so it doesn't block HTML parsing: add defer to the <script
src="{{ SITEURL }}/theme/js/mermaid.min.js"></script> tag while keeping the
existing inline initialization (mermaid.initialize({ startOnLoad: false, theme:
'dark' })) and the DOMContentLoaded handler that calls mermaid.run(); this
ensures the library is fetched/deferred but still initialized and run as before.
- Line 95: mermaid is initialized with a hardcoded 'dark' theme; update the
initialization to derive the theme from the document's data-theme attribute
(e.g., read document.documentElement.getAttribute('data-theme') or check the
theme toggle state) and pass 'dark' or 'default'/'forest' as appropriate to
mermaid.initialize, and add an event listener tied to the theme toggle that
calls mermaid.initialize with the new theme and then mermaid.run() so diagrams
re-render when the page theme changes; look for mermaid.initialize and
mermaid.run usage in this template to implement the dynamic theme logic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c40dcca6-47f1-43df-ac22-c5c06450b23e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
content/blog/stellar_openwisp_feedback.rstcontent/images/blog/steer-management.webppackage.jsontheme/templates/base.html
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: If a change affects documented behavior, update the documentation to reflect the new behavior
Documentation must mention new features; heavily UI-related features should have a new section or page in documentation
Files:
content/blog/stellar_openwisp_feedback.rst
🔇 Additional comments (2)
content/blog/stellar_openwisp_feedback.rst (1)
1-327: LGTM!package.json (1)
35-35: Validate Mermaid^11.15.0and check for known vulnerabilities
Themermaiddependency pinned to^11.15.0matches the latest npm release (11.15.0), and security advisory sources (including Snyk) report no known security vulnerabilities inmermaid11.15.0; earlier issues were addressed in this release.
| .. raw:: html | ||
|
|
||
| <pre class="mermaid"> | ||
| graph TD | ||
| OpenWISP_Users | ||
| OpenWISP_Notifications | ||
| OpenWISP_Controller | ||
| OpenWISP_Monitoring | ||
| OpenWISP_FirmwareUpgrader | ||
| </pre> |
There was a problem hiding this comment.
Missing documentation for the new Mermaid diagram feature.
This PR introduces Mermaid diagram support to the website and demonstrates its usage in this blog post. However, there is no documentation explaining how to use Mermaid diagrams in blog posts or other content. As per coding guidelines, documentation must mention new features, and heavily UI-related features should have a new section or page in documentation.
Consider adding documentation that covers:
- How to embed Mermaid diagrams in RST files (using
.. raw:: htmlblocks) - Supported Mermaid diagram types
- The
<pre class="mermaid">syntax requirement - Any limitations or best practices
- Example code snippets
This documentation could be added to a contributing guide, README, or a separate documentation page about content authoring.
As per coding guidelines, documentation must mention new features; heavily UI-related features should have a new section or page in documentation.
🤖 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 `@content/blog/stellar_openwisp_feedback.rst` around lines 44 - 53, Add a new
documentation section describing the newly introduced Mermaid diagram support:
explain how to embed diagrams using the RST .. raw:: html block and the required
<pre class="mermaid"> wrapper, list supported Mermaid diagram types (flowchart,
sequence, class, etc.), note limitations/best practices (escaping, sanitization,
rendering caveats), and include a short example snippet showing a minimal
Mermaid block and expected output; reference the sample in the blog post (the
graph TD block) and add this content to the site contributing/content-authoring
docs so authors know how to use Mermaid diagrams.
There was a problem hiding this comment.
Thanks, but I'm going to skip this one for now. The Mermaid usage here is a very thin convention (a .. raw:: html block wrapping a <pre class="mermaid"> element), and I'd rather not introduce a dedicated documentation section for what is currently a single blog post. If we start using Mermaid more widely across the site, I'll document the pattern in the contributing guide at that point.
There was a problem hiding this comment.
@nemesifier, understood — that's a sensible approach. No need to create overhead for a single-use pattern; documenting it in the contributing guide when it sees broader adoption makes more sense.
(ﾐↀ ﻌ ↀﾐ)
| <script src="{{ SITEURL }}/theme/js/mermaid.min.js"></script> | ||
| <script> | ||
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | ||
| document.addEventListener('DOMContentLoaded', () => { | ||
| mermaid.run(); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider deferring Mermaid script loading to improve page load performance.
The Mermaid library is loaded synchronously in the <head>, which blocks HTML parsing. Since diagram rendering is deferred until DOMContentLoaded anyway, loading the script with the defer attribute would improve initial page load performance without affecting functionality.
⚡ Proposed fix to defer Mermaid loading
- <script src="{{ SITEURL }}/theme/js/mermaid.min.js"></script>
+ <script src="{{ SITEURL }}/theme/js/mermaid.min.js" defer></script>
<script>
- mermaid.initialize({ startOnLoad: false, theme: 'dark' });
- document.addEventListener('DOMContentLoaded', () => {
+ document.addEventListener('DOMContentLoaded', () => {
+ mermaid.initialize({ startOnLoad: false, theme: 'dark' });
mermaid.run();
});
</script>📝 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.
| <script src="{{ SITEURL }}/theme/js/mermaid.min.js"></script> | |
| <script> | |
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | |
| document.addEventListener('DOMContentLoaded', () => { | |
| mermaid.run(); | |
| }); | |
| </script> | |
| <script src="{{ SITEURL }}/theme/js/mermaid.min.js" defer></script> | |
| <script> | |
| document.addEventListener('DOMContentLoaded', () => { | |
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | |
| mermaid.run(); | |
| }); | |
| </script> |
🤖 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 `@theme/templates/base.html` around lines 93 - 99, Change the external Mermaid
script tag to load with the defer attribute so it doesn't block HTML parsing:
add defer to the <script src="{{ SITEURL }}/theme/js/mermaid.min.js"></script>
tag while keeping the existing inline initialization (mermaid.initialize({
startOnLoad: false, theme: 'dark' })) and the DOMContentLoaded handler that
calls mermaid.run(); this ensures the library is fetched/deferred but still
initialized and run as before.
| {% endif %} | ||
| <script src="{{ SITEURL }}/theme/js/mermaid.min.js"></script> | ||
| <script> | ||
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Mermaid theme is hardcoded and doesn't respect the page theme toggle.
The page supports light/dark theme switching (see data-theme attribute on line 4 and toggle components), but Mermaid is hardcoded to use the 'dark' theme. This creates an inconsistent user experience when users select the light theme.
🎨 Proposed fix to respect theme toggle
<script>
- mermaid.initialize({ startOnLoad: false, theme: 'dark' });
document.addEventListener('DOMContentLoaded', () => {
+ const theme = document.documentElement.getAttribute('data-theme') === 'dark' ? 'dark' : 'default';
+ mermaid.initialize({ startOnLoad: false, theme: theme });
mermaid.run();
});
</script>You may also need to re-run mermaid.run() when the theme toggle is activated to re-render diagrams with the new theme.
📝 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.
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | |
| <script> | |
| document.addEventListener('DOMContentLoaded', () => { | |
| const theme = document.documentElement.getAttribute('data-theme') === 'dark' ? 'dark' : 'default'; | |
| mermaid.initialize({ startOnLoad: false, theme: theme }); | |
| mermaid.run(); | |
| }); | |
| </script> |
🤖 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 `@theme/templates/base.html` at line 95, mermaid is initialized with a
hardcoded 'dark' theme; update the initialization to derive the theme from the
document's data-theme attribute (e.g., read
document.documentElement.getAttribute('data-theme') or check the theme toggle
state) and pass 'dark' or 'default'/'forest' as appropriate to
mermaid.initialize, and add an event listener tied to the theme toggle that
calls mermaid.initialize with the new theme and then mermaid.run() so diagrams
re-render when the page theme changes; look for mermaid.initialize and
mermaid.run usage in this template to implement the dynamic theme logic.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@asmodehn thanks for this! It seems you have to rebase/merge this with the latest master as there's conflicts.
asmodehn
force-pushed
the
stellar_feedback
branch
from
fc14a6f to
410d187
Compare
Multiple QA Failures DetectedHello @asmodehn,
To fix these, please run
|
Sass Deprecation Warnings and ReST FormattingHello @asmodehn,
|
nemesifier
changed the title
nemesifier
force-pushed
the
stellar_feedback
branch
from
7979b20 to
0cd583d
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
nemesifier
force-pushed
the
stellar_feedback
branch
2 times, most recently
from
6f99cac to
9ee3f32
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
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 `@run-qa-checks`:
- Around line 17-20: The CI/job removed the RST validation by disabling checkrst
and only running docstrfmt; restore a separate RST validation step using the
checkrst command (or the same RST linter used previously) and ensure it excludes
node_modules like docstrfmt does. Update the job so both checkrst and docstrfmt
run: keep the existing docstrfmt invocation and add a new checkrst invocation
(or equivalent RST validator) that uses --extend-exclude node_modules (or the
same exclusion mechanism) so invalid directives/references are still caught;
locate the commands named checkrst and docstrfmt in the script to add the
validation step.
In `@setup.cfg`:
- Line 3: The flake8 exclude setting currently lists "venv, node_modules" and
should also exclude ".venv" to match Black and avoid linting virtualenv files;
update the "exclude" value in setup.cfg (the exclude setting) to include ".venv"
alongside "venv" and "node_modules" so virtualenv directories are ignored by
flake8.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1d4fceb-3298-4b4d-b27f-7e3512c02176
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
content/blog/stellar_openwisp_feedback.rstcontent/images/blog/steer-management.webppackage.jsonpyproject.tomlrun-qa-checkssetup.cfgtheme/templates/base.html
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: If a change affects documented behavior, update the documentation to reflect the new behavior
Documentation must mention new features; heavily UI-related features should have a new section or page in documentation
Files:
content/blog/stellar_openwisp_feedback.rst
🧠 Learnings (2)
📚 Learning: 2026-05-13T20:37:38.231Z
Learnt from: CR
Repo: openwisp/OpenWISP-Website PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-05-13T20:37:38.231Z
Learning: Applies to **/*.{md,rst,txt} : Documentation must mention new features; heavily UI-related features should have a new section or page in documentation
Applied to files:
content/blog/stellar_openwisp_feedback.rst
📚 Learning: 2026-05-13T20:37:38.231Z
Learnt from: CR
Repo: openwisp/OpenWISP-Website PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-05-13T20:37:38.231Z
Learning: Applies to **/*.{md,rst,txt} : If a change affects documented behavior, update the documentation to reflect the new behavior
Applied to files:
content/blog/stellar_openwisp_feedback.rst
🔇 Additional comments (4)
theme/templates/base.html (1)
93-98: Previously flagged Mermaid loading/theme concerns are still present.Line 93 still loads Mermaid synchronously in
<head>, and Line 95 still hardcodes'dark'despitedata-theme/theme toggle support.content/blog/stellar_openwisp_feedback.rst (1)
44-53: Dedicated Mermaid authoring docs still appear missing.This post demonstrates the new Mermaid feature, but there is still no contributor/content-authoring documentation update in the provided changes explaining how to embed Mermaid blocks safely and consistently.
As per coding guidelines, "If a change affects documented behavior, update the documentation to reflect the new behavior" and "Documentation must mention new features; heavily UI-related features should have a new section or page in documentation."
package.json (1)
18-19: LGTM!Also applies to: 28-35
pyproject.toml (1)
1-4: LGTM!
nemesifier
force-pushed
the
stellar_feedback
branch
6 times, most recently
from
73fddd6 to
39dd2b3
Compare
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
Co-authored-by: Alexandre Vincent <alexandre.vincent@stellar.tc>
nemesifier
force-pushed
the
stellar_feedback
branch
from
39dd2b3 to
85bccf0
Compare
- Added pyproject.toml with Black config to exclude node_modules - Added node_modules to flake8 exclude list in setup.cfg - Skip checkrst in openwisp-qa-check (incompatible with CI docstrfmt version) - Fix RST heading style in stellar_openwisp_feedback.rst
nemesifier
force-pushed
the
stellar_feedback
branch
from
85bccf0 to
e1a5b5d
Compare
This caused the header image to not show up in the blog index. The summary is auto-generated anyway
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
setup.cfg (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign flake8 excludes with Black by adding
.venv.The flake8 exclude list only includes
venvbut not.venv, while the Black configuration inpyproject.toml(lines 1-4) excludes both. Projects commonly use.venvas the virtualenv directory, and this mismatch can cause third-party code to be linted accidentally.🔧 Suggested fix to align with Black config
-exclude = venv, node_modules +exclude = .venv, venv, node_modules🤖 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 `@setup.cfg` at line 3, The flake8 exclude setting currently lists "venv" but omits ".venv", causing a mismatch with Black's exclusions; update the "exclude" value for flake8 (the exclude key in setup.cfg) to include ".venv" alongside "venv" and "node_modules" so both tools ignore the same virtualenv folder (align with pyproject.toml's Black excludes).run-qa-checks (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRST validation was disabled without replacement.
Adding
--skip-checkrstremoves the RST correctness gate for invalid directives and references. Since this PR includes a new blog post (content/blog/stellar-adoption-journey-from-fork-to-extension.rst), RST validation is especially important.Consider running
checkrstseparately withnode_modulesexcluded, similar to how other checks exclude generated/vendored directories.📝 Suggested fix to restore RST validation
openwisp-qa-check \ --skip-checkmigrations \ --skip-checkmakemigrations \ --skip-checkendline \ --skip-checkrst +# Run RST validation separately with node_modules excluded +checkrst $(find . -path './node_modules' -prune -o -name '*.rst' -print) +🤖 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 `@run-qa-checks` at line 17, The CI script disables RST validation by passing the --skip-checkrst flag; remove that flag from run-qa-checks (or stop passing --skip-checkrst) and instead invoke checkrst explicitly (e.g., run the checkrst command) while excluding vendored/generated dirs like node_modules and build outputs so the new RST post content/blog/stellar-adoption-journey-from-fork-to-extension.rst is validated; update the script to either call checkrst with an exclude pattern or re-enable the default RST step rather than using --skip-checkrst.
🤖 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 `@theme/templates/article.html`:
- Around line 96-101: Wrap the Mermaid initialization inside a defensive check
and try/catch: in the DOMContentLoaded listener, first verify the global mermaid
object exists (check window.mermaid or typeof mermaid !== 'undefined'), then
call mermaid.initialize(...) and mermaid.run() inside a try/catch block to log
or silently handle initialization errors; update the listener that currently
calls mermaid.initialize and mermaid.run so failures do not throw uncaught
exceptions.
---
Duplicate comments:
In `@run-qa-checks`:
- Line 17: The CI script disables RST validation by passing the --skip-checkrst
flag; remove that flag from run-qa-checks (or stop passing --skip-checkrst) and
instead invoke checkrst explicitly (e.g., run the checkrst command) while
excluding vendored/generated dirs like node_modules and build outputs so the new
RST post content/blog/stellar-adoption-journey-from-fork-to-extension.rst is
validated; update the script to either call checkrst with an exclude pattern or
re-enable the default RST step rather than using --skip-checkrst.
In `@setup.cfg`:
- Line 3: The flake8 exclude setting currently lists "venv" but omits ".venv",
causing a mismatch with Black's exclusions; update the "exclude" value for
flake8 (the exclude key in setup.cfg) to include ".venv" alongside "venv" and
"node_modules" so both tools ignore the same virtualenv folder (align with
pyproject.toml's Black excludes).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07a75925-b964-48ea-bd76-29116d6b83c8
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
content/blog/stellar-adoption-journey-from-fork-to-extension.rstcontent/images/blog/steer-management.webppackage.jsonpyproject.tomlrun-qa-checkssetup.cfgtheme/templates/article.html
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: If a change affects documented behavior, update the documentation to reflect the new behavior
Documentation must mention new features; heavily UI-related features should have a new section or page in documentation
Files:
content/blog/stellar-adoption-journey-from-fork-to-extension.rst
🔇 Additional comments (11)
pyproject.toml (1)
1-4: LGTM!content/blog/stellar-adoption-journey-from-fork-to-extension.rst (8)
43-52: Missing documentation for the Mermaid diagram feature.This issue was previously raised and remains unresolved. The PR introduces Mermaid diagram support but lacks documentation explaining how content authors can use this feature. Per coding guidelines, documentation must mention new features, and heavily UI-related features should have a new section or page.
The documentation should cover:
- How to enable Mermaid in blog posts (
:mermaid: truemetadata)- How to embed diagrams using
.. raw:: htmland<pre class="mermaid">syntax- Supported diagram types (flowchart, sequence, class, state, etc.)
- Any limitations or best practices
This documentation should be added to a contributing guide, README, or content authoring documentation.
As per coding guidelines, documentation must mention new features; heavily UI-related features should have a new section or page in documentation.
1-17: LGTM!
43-52: LGTM!
78-87: LGTM!
129-139: LGTM!
162-173: LGTM!
208-227: LGTM!Also applies to: 250-269
286-325: LGTM!package.json (2)
18-18: LGTM!
35-35: Mermaid^11.15.0is valid and covers the known security advisories.
- npm registry latest version is
11.15.0(matches the dependency)- Security advisories show affected ranges up to
11.14.0, with fixes/patched version11.15.0;npm auditreports no mermaid vulnerabilities
| <script> | ||
| document.addEventListener('DOMContentLoaded', () => { | ||
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | ||
| mermaid.run(); | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding error handling for Mermaid initialization.
If the Mermaid library fails to load (network issue, missing file, etc.), the current code will throw an uncaught error. Consider wrapping the initialization in a try-catch or checking for the existence of the mermaid object.
🛡️ Proposed defensive initialization
<script>
document.addEventListener('DOMContentLoaded', () => {
+ if (typeof mermaid === 'undefined') {
+ console.error('Mermaid library failed to load');
+ return;
+ }
mermaid.initialize({ startOnLoad: false, theme: 'dark' });
mermaid.run();
});
</script>📝 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.
| <script> | |
| document.addEventListener('DOMContentLoaded', () => { | |
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | |
| mermaid.run(); | |
| }); | |
| </script> | |
| <script> | |
| document.addEventListener('DOMContentLoaded', () => { | |
| if (typeof mermaid === 'undefined') { | |
| console.error('Mermaid library failed to load'); | |
| return; | |
| } | |
| mermaid.initialize({ startOnLoad: false, theme: 'dark' }); | |
| mermaid.run(); | |
| }); | |
| </script> |
🤖 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 `@theme/templates/article.html` around lines 96 - 101, Wrap the Mermaid
initialization inside a defensive check and try/catch: in the DOMContentLoaded
listener, first verify the global mermaid object exists (check window.mermaid or
typeof mermaid !== 'undefined'), then call mermaid.initialize(...) and
mermaid.run() inside a try/catch block to log or silently handle initialization
errors; update the listener that currently calls mermaid.initialize and
mermaid.run so failures do not throw uncaught exceptions.
There was a problem hiding this comment.
Thanks, but I'll skip this one. If the library fails to load, the diagrams simply won't render, which is an acceptable degradation for a blog post and doesn't affect the rest of the page. I'd prefer to keep the snippet minimal rather than add a guard for that case.
There was a problem hiding this comment.
@nemesifier, that's a fair call — graceful degradation is perfectly reasonable here, and the defer attribute already ensures a load failure won't block the rest of the page. Happy to leave it as-is.
(ᵕ˃ᴗ˂ᵕ)
|
Thanks a lot for this write-up. It's a great real-world account of extending OpenWISP, and I think many people in the community will find it genuinely useful. I have just a couple of suggestions that I believe would make it even more impactful before we publish. 1. A bit of code would go a long way. In my opinion the most valuable parts of the post are the migration strategies you describe, in particular the management command that fake-applies migrations when an equivalent OpenWISP migration already exists, and the command that recovers forward migration from inconsistent states. The same applies to the swapper defaults you load in 2. Some sense of scale. The post mentions a "growing fleet of GLOBBLE routers" and managing connections "at scale", but it never gives the reader a feel for the actual numbers. Even a rough order of magnitude (hundreds? thousands?) would make both the motivation and the achievement much more concrete and credible. Everything else looks good to me. Thanks again for taking the time to contribute this, and especially for upstreaming your improvements. This is exactly the kind of collaboration we hope to see. |
The openwisp-utils dev version's RST node_modules exclude fix is broken (see openwisp/openwisp-utils#692). Temporarily skip checkrst and revert to 1.2 until the fix is merged.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
content/blog/stellar-adoption-journey-from-fork-to-extension.rst (1)
26-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd approximate scale information for the router fleet.
The phrase "growing fleet of GLOBBLE routers" is vague. As per the PR feedback from nemesifier, adding a rough order of magnitude (e.g., "hundreds" or "thousands") would make the motivation and impact more concrete for readers.
📝 Suggested revision
-to manage our growing fleet of GLOBBLE routers. For the past two years, +to manage our growing fleet of hundreds of GLOBBLE routers. For the past two years,Adjust the scale term based on your actual deployment size.
🤖 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 `@content/blog/stellar-adoption-journey-from-fork-to-extension.rst` at line 26, Replace the vague phrase "growing fleet of GLOBBLE routers" with a concrete approximate scale (e.g., "growing fleet of GLOBBLE routers (approximately hundreds)" or "…(over a thousand)") in the sentence that currently contains that exact string; choose the correct order-of-magnitude wording to reflect actual deployment and keep the parenthetical or adjective concise so the sentence reads naturally. Ensure you update the same occurrence of the phrase in the document (the line containing "growing fleet of GLOBBLE routers") rather than adding a separate footnote.
🤖 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 `@content/blog/stellar-adoption-journey-from-fork-to-extension.rst`:
- Around line 266-268: Replace the phrase "is straightforward" in the sentence
that begins "The command to fake-apply migrations when an equivalent OpenWISP
migration has already been applied to the database is straightforward" with a
more accurate description; e.g., change it to "is conceptually simple but the
implementation handles complex state management, error handling, and edge cases"
or "is simple in concept but requires detailed implementation to handle state,
errors, and edge cases" so the text reflects the command's complexity.
- Line 262: Fix the typo in the blog content line containing "run code form the
internet (or any AI). Use your own judgement" by changing "form" to "from" so
the sentence reads "run code from the internet (or any AI). Use your own
judgement"; update the text in the file
content/blog/stellar-adoption-journey-from-fork-to-extension.rst at the
occurrence of that exact phrase.
- Around line 182-220: The example uses the Django settings object but does not
show its import; add an import for django.conf.settings before the
OpenwispExtensionUsersConfig class so references to settings (used in the
__init__ method and the settings.OPENWISP_* assignments) are valid and the
snippet is copy-paste ready.
In `@requirements.txt`:
- Line 7: Replace the openwisp-utils[qa] dependency that currently points to
tarball/master with a pinned reference to a stable identifier: update the
existing line for openwisp-utils[qa] to use either a release tag (e.g.,
tarball/<tag>) or a specific commit hash (tarball/<commit-hash>) so builds are
reproducible; ensure you pick a commit/tag that includes the desired RST fix and
update requirements.txt accordingly.
---
Outside diff comments:
In `@content/blog/stellar-adoption-journey-from-fork-to-extension.rst`:
- Line 26: Replace the vague phrase "growing fleet of GLOBBLE routers" with a
concrete approximate scale (e.g., "growing fleet of GLOBBLE routers
(approximately hundreds)" or "…(over a thousand)") in the sentence that
currently contains that exact string; choose the correct order-of-magnitude
wording to reflect actual deployment and keep the parenthetical or adjective
concise so the sentence reads naturally. Ensure you update the same occurrence
of the phrase in the document (the line containing "growing fleet of GLOBBLE
routers") rather than adding a separate footnote.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 666df821-6853-4014-b955-272fa505bba0
📒 Files selected for processing (4)
content/blog/stellar-adoption-journey-from-fork-to-extension.rstrequirements.txtrun-qa-checkssetup.cfg
📜 Review details
⏰ 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). (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: If a change affects documented behavior, update the documentation to reflect the new behavior
Documentation must mention new features; heavily UI-related features should have a new section or page in documentation
Files:
requirements.txtcontent/blog/stellar-adoption-journey-from-fork-to-extension.rst
🔇 Additional comments (6)
content/blog/stellar-adoption-journey-from-fork-to-extension.rst (4)
156-220: LGTM!The code examples effectively address the reviewer feedback by providing concrete, copy-paste-ready implementations of swapper defaults and AppConfig initialization patterns.
259-265: LGTM!The disclaimer appropriately warns readers about the risks of running code from blog posts and emphasizes personal responsibility.
269-400: LGTM!The management command implementation is thorough with proper error handling, safety checks, informative output, and clear failure modes. The code quality is production-ready.
441-446: LGTM!The collaborative tone and acknowledgment of ongoing work with OpenWISP maintainers is constructive and aligns well with open-source best practices.
setup.cfg (1)
3-3: LGTM!run-qa-checks (1)
16-16: LGTM!
I added some code, that helps make this post feel more technical. nothing very complex or difficult in my opinion though. However the syntax coloring doesn't seem to work (maybe the mermaid css mess it up ?) I am also not sure the strategy we went with the DB is the best/most practical one, so I don't want to advertise it too much. It is practical to use when DB changes are minimal, but likely cumbersome when they become significant, so I added a small disclaimer in there.
Added some mention of it. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Great work @asmodehn and Stellar, thanks 👍
2 participants
A blog post detailing how stellar extended openwisp and what were the main challenges