Fix documentation highlighting - #207
Conversation
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…ax highlighting and update dependencies accordingly. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…oss files. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
… HTML comments. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
WalkthroughRefactors Docker asset fetching and paths, adds marked-highlight and a dark CSS theme, and overhauls docker_root/index.html to a deferred, resilient asset-loading and markdown-rendering flow with improved error handling and dark-mode support. Adds .dockerignore and SPDX license files; punctuation and copy edits across governance, DCO, README, and minor comments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as User
participant Doc as index.html
participant CSS as loadCSS
participant Loader as Asset Loader
participant HLJS as highlight.js
participant Marked as marked
participant MH as marked-highlight
participant Fetch as fetch(README.md)
participant Render as renderMarkdownFromFile
Note over Doc: DOMContentLoaded triggers asset loading
User->>Doc: Open docs page
Doc->>CSS: Load github-markdown.css (light)
CSS-->>Doc: onload
Doc->>CSS: Load github-dark-dimmed.css (media=dark)
CSS-->>Doc: onload
par Load scripts (local → CDN fallback)
Doc->>Loader: Load highlight.js
Loader-->>HLJS: available or fallback
Doc->>Loader: Load marked.umd.min.js
Loader-->>Marked: available or fallback
Doc->>Loader: Load marked-highlight.umd.min.js
Loader-->>MH: available or fallback
end
Doc->>Render: renderMarkdownFromFile("#content", "README.md", cb)
Render->>Render: validate target element
alt Marked + MH + HLJS available
Render->>Marked: configure parser with highlight adapter
Render->>Fetch: GET README.md
alt 200 OK
Fetch-->>Render: markdown text
Render-->>Doc: inject HTML with syntax highlighting
else Fetch error
Render-->>Doc: log error, invoke callback
end
else Missing libs
Render-->>Doc: fallback plaintext `<pre>` render + warning
end
Note over Doc: aria-busy=false, noscript message present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageToolREADME.md[grammar] ~298-~298: There might be a mistake here. (QB_NEW_EN) [grammar] ~306-~306: There might be a mistake here. (QB_NEW_EN) ⏰ 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
|
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (3)
216-220: Comma after introductory clauseAdd a comma for readability.
- To add a huge number of headers the `-headerfile` parameter can be used: + To add a huge number of headers, the `-headerfile` parameter can be used:
286-290: Smoother phrasing (“For example” instead of “So … e.g.”)Small readability tweak.
- to *SonicWeb*. So running e.g., + to *SonicWeb*. For example, running
306-307: Hyphenation nit: “non-root user”Use the standard form.
- readable by the non-root-user that *SonicWeb* uses (UID 65532). + readable by the non-root user that *SonicWeb* uses (UID 65532).docker_root/index.html (2)
135-160: Harden global resolution and guard against missing hljsAvoid destructuring assumptions about UMD globals and guard when hljs is unavailable to prevent runtime errors if only some scripts load.
- if (typeof marked !== 'undefined' && typeof markedHighlight !== 'undefined') { - // We control the Markdown content; DOMPurify intentionally omitted - const { Marked } = globalThis.marked; - const { markedHighlight } = globalThis.markedHighlight; - - const marked = new Marked( - markedHighlight({ - emptyLangClass: 'hljs', - langPrefix: 'hljs language-', - highlight: (code, lang, info) => { - const language = hljs.getLanguage(lang) ? lang : 'plaintext'; - return hljs.highlight(code, { language }).value; - } - }) - ); - - target.innerHTML = marked.parse(md); + if (globalThis.marked && (globalThis.markedHighlight || globalThis.markedHighlight?.markedHighlight)) { + // We control the Markdown content; DOMPurify intentionally omitted + const { Marked } = globalThis.marked; + // Support both global shapes: function or { markedHighlight } + const mh = globalThis.markedHighlight?.markedHighlight || globalThis.markedHighlight; + const hasHljs = typeof hljs !== 'undefined' && typeof hljs.highlight === 'function'; + + const parser = new Marked( + mh({ + emptyLangClass: 'hljs', + langPrefix: 'hljs language-', + highlight: (code, lang) => { + if (!hasHljs) return code; + const language = hljs.getLanguage(lang) ? lang : 'plaintext'; + return hljs.highlight(code, { language }).value; + } + }) + ); + + target.innerHTML = parser.parse(md);Also applies to: 140-151
13-16: Verify favicon.svg presence (optional)Head references /favicon.svg but only .ico and apple-touch-icon are added in this PR. If svg is missing at runtime, drop the link or include the asset.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docker_root/apple-touch-icon.pngis excluded by!**/*.pngdocker_root/favicon.icois excluded by!**/*.icodocker_root/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
.dockerignore(1 hunks).gitignore(1 hunks)CODE_OF_CONDUCT.md(1 hunks)DCO.txt(2 hunks)Dockerfile(2 hunks)GOVERNANCE.md(2 hunks)LICENSES/LicenseRef-DCO.txt(2 hunks)README.md(8 hunks)docker_root/apple-touch-icon.png.license(1 hunks)docker_root/favicon.ico.license(1 hunks)docker_root/index.html(5 hunks)middlewares.go(1 hunks)testroot/spa.html(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T14:10:51.227Z
Learnt from: AlphaOne1
PR: AlphaOne1/sonicweb#193
File: governance.md:1-4
Timestamp: 2025-09-21T14:10:51.227Z
Learning: In the SonicWeb repository, AlphaOne1 prefers to maintain original author attribution in SPDX headers even when it differs from the standardized format used elsewhere in the repo, particularly for governance and foundational documents.
Applied to files:
docker_root/favicon.ico.licensedocker_root/apple-touch-icon.png.license
🪛 LanguageTool
README.md
[grammar] ~284-~284: There might be a mistake here.
Context: ...tarts SonicWeb without any parameters. So /www is the default web root direct...
(QB_NEW_EN)
[grammar] ~298-~298: There might be a mistake here.
Context: ...cing the default content entirely. A new web root directory, e.g., myapp/ could...
(QB_NEW_EN)
[grammar] ~306-~306: There might be a mistake here.
Context: ...e care that the content of the mounts is readable by the non-root-user that *Soni...
(QB_NEW_EN)
CODE_OF_CONDUCT.md
[style] ~50-~50: Consider using a different adverb to strengthen your wording.
Context: ...sagreements, both social and technical, happen all the time, and SonicWeb is no exception. It is ...
(ALL_THE_TIME_CONSTANTLY)
🔇 Additional comments (7)
.gitignore (1)
5-5: LGTM: add.DS_Storeto ignoresGood housekeeping; no further changes needed.
docker_root/apple-touch-icon.png.license (1)
1-3: SPDX license stub looks correctMatches project conventions and licensing.
docker_root/favicon.ico.license (1)
1-3: SPDX license stub looks correctConsistent with other docker_root assets.
.dockerignore (1)
1-4: LGTM: ignore license stubs in build contextKeeps images lean; aligns with added asset licenses.
middlewares.go (1)
46-47: Nit: comment punctuation update is fineNo functional impact; reads better.
Dockerfile (1)
23-29: LGTM: add marked-highlight asset with checksumVersioned args plus SHA256 verification and destination path align with index.html loader.
Consider verifying that the index.html fallback SRI hash corresponds to the exact CDN URL/version used here to prevent integrity failures if local load ever fails.
Also applies to: 61-65
docker_root/index.html (1)
205-207: Fix invalid SRI: trailing “%” breaks CDN fallback integrity – unable to auto-fetch the file due to SSL errors, please manually recompute and update the sha384 hash for marked-highlight@2.2.2 (remove the trailing “%”).
…. Update Dockerfile and documentation for consistency. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker_root/index.html (1)
60-83: Preserve media attribute on CSS fallback to avoid always-on dark theme.When local dark CSS fails, the CDN fallback lacks media, applying unconditionally. Mirror media on fallback.
function loadCSS(localHref, fallbackHref, fallbackInt) { const link = document.createElement('link'); link.rel = 'stylesheet'; link.href = localHref; if (localHref.includes('dark')) { link.media = '(prefers-color-scheme: dark)'; } link.onerror = () => { console.warn(`CSS failed: ${localHref}, loading fallback...`); const fallbackLink = document.createElement('link'); fallbackLink.rel = 'stylesheet'; fallbackLink.href = fallbackHref; + // Preserve media attribute for dark-mode styles + if (link.media) { + fallbackLink.media = link.media; + } if (fallbackInt) { fallbackLink.integrity = fallbackInt; fallbackLink.crossOrigin = 'anonymous'; } document.head.appendChild(fallbackLink); }; document.head.appendChild(link); }
🧹 Nitpick comments (3)
README.md (3)
284-287: Minor wording/casing nits.
- Prefer “Dockerfile” (proper noun).
- “So /www is …” → “Therefore, /www is …” (more formal).
- and it will show this documentation. The entrypoint of the dockerfile just starts *SonicWeb* without any parameters. - So `/www` is the default web root directory. Every parameter passed after the image name is appended as a parameter + and it will show this documentation. The entrypoint of the Dockerfile just starts *SonicWeb* without any parameters. + Therefore, `/www` is the default web root directory. Every parameter passed after the image name is appended as a parameter
298-303: Tighten phrasing.“could be mounted” → “can be mounted”.
-A new web root directory, e.g., `myapp/` could be mounted like this: +A new web root directory, e.g., `myapp/`, can be mounted like this:
305-308: Clarity improvement on user wording.Consider: “the non-root user SonicWeb runs as”.
-Also take care that the content of the mounts is readable by the non-root user that *SonicWeb* uses (UID 65532). +Also ensure the mounted content is readable by the non-root user *SonicWeb* runs as (UID 65532).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(3 hunks)README.md(8 hunks)docker_root/index.html(5 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~284-~284: There might be a mistake here.
Context: ...tarts SonicWeb without any parameters. So /www is the default web root direct...
(QB_NEW_EN)
[grammar] ~298-~298: There might be a mistake here.
Context: ...cing the default content entirely. A new web root directory, e.g., myapp/ could...
(QB_NEW_EN)
[grammar] ~306-~306: There might be a mistake here.
Context: ...e care that the content of the mounts is readable by the non-root user that *Soni...
(QB_NEW_EN)
🔇 Additional comments (5)
README.md (1)
165-167: Code block language hint change to sh looks good.Consistent with POSIX shell; no behavioral changes. LGTM.
Also applies to: 176-178, 192-194, 199-205, 212-214, 218-220, 251-253, 267-273, 281-283, 289-296, 301-303, 312-313, 321-323, 327-330
Dockerfile (1)
20-32: Asset pinning, layout, and checksums: LGTM.
- Versions and SHA256s centralized.
- Styles moved to /styles aligned with index.html.
- Dark theme asset added and pinned.
No issues spotted.
Also applies to: 40-46, 62-66, 68-76, 77-81
docker_root/index.html (3)
126-186: Markdown rendering flow: sensible with safe plaintext fallback.
- Validates target; handles non-ok fetch; guarded parsing; hljs adapter fallback.
- DOMPurify omission is acceptable given controlled README in image.
If README rendering ever targets untrusted Markdown, consider DOMPurify.
204-208: Remove trailing comma in function call for broader browser compatibility.Trailing commas in argument lists can break older engines.
loadCSS( 'styles/github-dark-dimmed.min.css', 'https://cdn.jsdelivr.net/npm/highlight.js@11.11.1/styles/github-dark-dimmed.min.css', - 'sha384-PiLidnnRuzFgp4qiN8oGNmktrV8ETL+6a8heAxljUX4A+3XWlocwaMn9duBUepfK', + 'sha384-PiLidnnRuzFgp4qiN8oGNmktrV8ETL+6a8heAxljUX4A+3XWlocwaMn9duBUepfK' )Likely an incorrect or invalid review comment.
188-209: SRI hashes verified—all three CSS fallbacks match current CDN content.
- github-markdown-css/5.8.1: ✓ Correct
- highlight.js/11.11.1 (github): ✓ Correct
- highlight.js/11.11.1 (github-dark-dimmed): ✓ Correct
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores