Fix theme toggle issue on "How it works " page and Implement Cache Busting#259
Fix theme toggle issue on "How it works " page and Implement Cache Busting#259Rudra-rps wants to merge 10 commits intoOWASP-BLT:mainfrom
Conversation
🍃 PR Readiness CheckCheck the readiness of this PR on Leaf: Leaf reviews pull requests for operational readiness, security risks, and production-impacting changes before they ship. |
3c46985 to
be92b7d
Compare
|
is it ready? |
|
I have a similar update PR for the layout that uses scroll-able divs see if you like that one and I'll merge it |
be92b7d to
221a952
Compare
Looks good to me.. |
e-esakman
left a comment
There was a problem hiding this comment.
please resolve the conflicts
done.. |
There was a problem hiding this comment.
Pull request overview
Updates frontend theme handling and mitigates stale-asset issues (especially in local dev) for the BLT-Leaf static UI served via the Cloudflare Worker.
Changes:
- Adds shared
public/theme.jsand migrates theme toggle markup todata-attributes. - Updates How It Works page to use the shared theme script and removes page-specific theme logic.
- Adds local-dev cache-bypass behavior in the service worker and introduces explicit cache headers when serving assets from
env.ASSETS.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.py | Adds helper to serve static assets with explicit Cache-Control behavior. |
| public/theme.js | Introduces shared theme initialization + toggle/icon behavior. |
| public/index.html | Switches theme toggles to data- attributes and loads theme.js; adjusts SW handling in local dev. |
| public/how-it-works.html | Removes inline theme init/toggle logic and loads shared theme.js. |
| public/sw.js | Adds local-dev behavior to skip/bypass SW caching for static assets. |
| public/diagnostics.html | Migrates theme toggle selectors to data- attributes. |
| public/test-error.html | Migrates theme toggle selectors to data- attributes. |
Comments suppressed due to low confidence (1)
public/how-it-works.html:14
theme.jsis loaded afterhttps://cdn.tailwindcss.comand after initial rendering, but it’s currently responsible for settingwindow.tailwind.config.darkModeand applying the initial theme. Tailwind CDN readswindow.tailwind.configat load time, so setting it after Tailwind loads can makedark:variants unreliable, and applying the theme at the end of the body can cause a flash of incorrect theme. Consider restoring an early head snippet (before Tailwind loads) to setdarkMode: 'class'and apply the initialdarkclass, and keeptheme.jsfor the toggle/icon wiring.
<script src="error-reporter.js"></script>
<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"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed all review comments: Split cache policy by asset type (HTML & sw.js use no-cache, must-revalidate; fingerprinted JS/CSS use long-lived immutable caching). Fixed theme.js to respect prefers-color-scheme when no saved theme exists. Wrapped DOM-dependent logic in DOMContentLoaded to ensure safe execution when loaded in . Please let me know if any further adjustments are needed. |
|
I’m not sure about this approach. It feels more complex than necessary and could likely be simplified. |
The main issue I was addressing here was related to caching behaviour on the cloudflare worker side, which was preventing new deployments from being picked up correctly. If there is a simpler approach I’d be happy to align with it.. |
|
@Rudra-rps i looked into this. The problem is with multiple cache no coordination but we need this multiple cache. I got a solution as |
Makes sense, I'll look into this. Thanks! |
|
This change introduces a lightweight fingerprinting step that hashes JS assets and rewrites the HTML references to the hashed filenames. Because the filename changes whenever the content changes, browsers and CDNs automatically fetch the new version while safely caching the old one. Also updated the worker and service worker logic so that only fingerprinted assets receive long-lived immutable caching, while HTML and non fingerprinted assets continue to revalidate normally. |
|
Ready for review @DonnieBLT |
|
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:
WalkthroughThis pull request introduces a comprehensive asset fingerprinting and caching system. It adds a build-time fingerprinting script that generates hash-based filenames for static assets, updates the build pipeline and HTML files to reference these fingerprinted versions, refactors theme management into a shared versioned module, enhances the service worker for local development, implements new backend cache control headers, adds error reporting sanitization, and introduces an interactive calculator for the how-it-works page. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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 (2)
public/diagnostics.html (1)
171-181:⚠️ Potential issue | 🟡 MinorAdd null checks before accessing
themeToggleandthemeIcon.Same issue as in
test-error.html: if these elements aren't found,querySelectorreturnsnull, causing aTypeErroronaddEventListener.🛡️ Proposed fix to add defensive null checks
const themeToggle = document.querySelector('[data-theme-toggle]'); const themeIcon = document.querySelector('[data-theme-icon]'); -themeToggle.addEventListener('click', () => { - const isDark = document.documentElement.classList.toggle('dark'); - themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon'; - localStorage.setItem('theme', isDark ? 'dark' : 'light'); -}); -// Set initial icon -if (document.documentElement.classList.contains('dark')) { - themeIcon.className = 'fas fa-sun'; +if (themeToggle && themeIcon) { + themeToggle.addEventListener('click', () => { + const isDark = document.documentElement.classList.toggle('dark'); + themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon'; + localStorage.setItem('theme', isDark ? 'dark' : 'light'); + }); + if (document.documentElement.classList.contains('dark')) { + themeIcon.className = 'fas fa-sun'; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/diagnostics.html` around lines 171 - 181, The code assumes themeToggle and themeIcon are non-null; add defensive null checks around their usage: after const themeToggle = document.querySelector('[data-theme-toggle]') and const themeIcon = document.querySelector('[data-theme-icon]'), verify both are truthy before calling themeToggle.addEventListener, accessing themeIcon.className, or setting the initial icon with document.documentElement.classList.contains('dark'); if either is missing, skip the event wiring and initial icon assignment (or optionally log a warning). Ensure the checks reference the existing symbols themeToggle and themeIcon and guard all subsequent accesses to those variables.public/test-error.html (1)
225-231:⚠️ Potential issue | 🟡 MinorAdd null checks before accessing
themeToggleandthemeIcon.If the elements with
[data-theme-toggle]or[data-theme-icon]are not found (e.g., due to DOM manipulation or future template changes),querySelectorreturnsnull, and callingaddEventListeneronnullwill throw aTypeError.🛡️ Proposed fix to add defensive null checks
const themeToggle = document.querySelector('[data-theme-toggle]'); const themeIcon = document.querySelector('[data-theme-icon]'); -themeToggle.addEventListener('click', () => { - const isDark = document.documentElement.classList.toggle('dark'); - themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon'; - localStorage.setItem('theme', isDark ? 'dark' : 'light'); -}); -if (document.documentElement.classList.contains('dark')) { - themeIcon.className = 'fas fa-sun'; +if (themeToggle && themeIcon) { + themeToggle.addEventListener('click', () => { + const isDark = document.documentElement.classList.toggle('dark'); + themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon'; + localStorage.setItem('theme', isDark ? 'dark' : 'light'); + }); + if (document.documentElement.classList.contains('dark')) { + themeIcon.className = 'fas fa-sun'; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/test-error.html` around lines 225 - 231, The current code assumes document.querySelector found elements and will throw if null; update the block that references themeToggle and themeIcon to first guard their existence (check themeToggle and themeIcon are truthy) before calling themeToggle.addEventListener or setting themeIcon.className and localStorage; alternatively use optional chaining when adding the listener and when updating themeIcon (e.g., only call document.documentElement.classList.toggle and localStorage.setItem inside the guarded branch) so themeToggle and themeIcon are not accessed when null to prevent a TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/error-reporter.1ba0a9b8.js`:
- Around line 73-97: The current hookConsoleError wrapper forwards every
console.error invocation (console.error) by serializing arbitrary args and
calling reportError, which can leak user/request data and duplicate handled
errors; update hookConsoleError so it only calls reportError for real Error
instances found in arguments (inspect arguments for instanceof Error), avoid
JSON.stringify on plain objects (do not serialize objects or strip/replace
sensitive fields like request, headers, user, cookies), redact PII from
error.message/stack and truncate safely, and add a marker/metadata (e.g.,
source: 'console.error:unhandled') so downstream slack_notifier handlers can
skip duplicates; ensure reportError is not invoked for non-Error diagnostics to
prevent noisy Slack alerts and leakage.
In `@public/index.html`:
- Around line 332-335: The theme toggle button using the data-theme-toggle
attribute is an icon-only control with no accessible name; add an accessible
name (e.g., aria-label="Toggle theme") to the button element and mark the inner
<i> with data-theme-icon as decorative (aria-hidden="true") so assistive tech
ignores the icon; apply the same change to the other theme toggle instance (the
second data-theme-toggle / data-theme-icon pair) to ensure both buttons are
accessible.
- Line 3291: The page throws a ReferenceError because the inline app script
calls initTheme() before the theme bundle is loaded; either move the theme
script tag that loads public/theme.196e3e21.js so it appears before the inline
app script, or remove the initTheme() invocation from the inline script (or
replace it with a safe feature check like if (typeof initTheme === 'function')
initTheme()); update the inline script that contains initTheme() call and the
script tag loading public/theme.196e3e21.js to ensure initTheme() is defined
before being invoked.
In `@scripts/fingerprint.js`:
- Around line 56-61: The loop over ASSETS in fingerprint.js currently warns and
continues when a declared asset is missing; change this to fail the build
instead by throwing an error or exiting with non‑zero status when
fs.existsSync(srcPath) is false. Locate the for (const asset of ASSETS) loop and
replace the console.warn/continue branch with a console.error (including asset
and srcPath) followed by throwing a new Error or calling process.exit(1) so
missing entries in ASSETS (and PUBLIC/srcPath) cause the build to fail.
---
Outside diff comments:
In `@public/diagnostics.html`:
- Around line 171-181: The code assumes themeToggle and themeIcon are non-null;
add defensive null checks around their usage: after const themeToggle =
document.querySelector('[data-theme-toggle]') and const themeIcon =
document.querySelector('[data-theme-icon]'), verify both are truthy before
calling themeToggle.addEventListener, accessing themeIcon.className, or setting
the initial icon with document.documentElement.classList.contains('dark'); if
either is missing, skip the event wiring and initial icon assignment (or
optionally log a warning). Ensure the checks reference the existing symbols
themeToggle and themeIcon and guard all subsequent accesses to those variables.
In `@public/test-error.html`:
- Around line 225-231: The current code assumes document.querySelector found
elements and will throw if null; update the block that references themeToggle
and themeIcon to first guard their existence (check themeToggle and themeIcon
are truthy) before calling themeToggle.addEventListener or setting
themeIcon.className and localStorage; alternatively use optional chaining when
adding the listener and when updating themeIcon (e.g., only call
document.documentElement.classList.toggle and localStorage.setItem inside the
guarded branch) so themeToggle and themeIcon are not accessed when null to
prevent a TypeError.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 4d880c58-76ae-4bad-b0cd-0f20264832b8
📒 Files selected for processing (14)
package.jsonpublic/asset-manifest.jsonpublic/diagnostics.htmlpublic/error-reporter.1ba0a9b8.jspublic/how-it-works.3ebd3397.jspublic/how-it-works.htmlpublic/index.htmlpublic/sw.jspublic/test-error.htmlpublic/theme.196e3e21.jspublic/theme.jsscripts/fingerprint.jssrc/index.pywrangler.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
public/diagnostics.html (1)
171-180: Prefer the shared theme bundle here instead of duplicating the toggle logic.
public/theme.jsalready owns the same selectors and icon initialization, andscripts/fingerprint.jsis set up to rewritetheme.jsreferences for this page. Keeping a second page-local implementation here makes future theme fixes easy to miss on diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/diagnostics.html` around lines 171 - 180, This page duplicates the theme toggle logic (the DOM selectors themeToggle and themeIcon, toggling document.documentElement.classList, updating themeIcon.className, and writing localStorage) that already lives in the shared public/theme.js; remove this local block and rely on the shared bundle instead — delete the click handler and initial icon setting here and ensure the page includes/uses the existing public/theme.js (scripts/fingerprint.js will rewrite references) so all theme behavior is sourced from the single implementation.scripts/fingerprint.js (1)
25-31: CSS fingerprinting is only half implemented right now.The file header/comments describe JS/CSS support, but the HTML rewrite only matches
src="...". If a stylesheet is ever added toASSETS, the build will create the hashed file while the HTML keeps pointing at the oldhref. Either extend the matcher to coverhreftoo or narrow the script/docs to JS-only.🔧 One way to cover both `src` and `href`
- const re = new RegExp( - `(src=["'][^"']*)${escapeRegex(stem)}(?:\\.[0-9a-f]{8})?${escapeRegex(ext)}(["'])`, - 'g' - ); + const re = new RegExp( + `((?:src|href)=["'][^"']*)${escapeRegex(stem)}(?:\\.[0-9a-f]{8})?${escapeRegex(ext)}((?:\\?[^"']*)?["'])`, + 'g' + );Also applies to: 116-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/fingerprint.js` around lines 25 - 31, The comment/behavior mismatch comes from ASSETS claiming JS/CSS support while the HTML rewrite only replaces src="..." occurrences; update the HTML rewrite logic that currently targets src attributes so it also matches href attributes (or both src|href) when rewriting filenames from ASSETS, ensuring stylesheet links get their fingerprinted filenames, and keep the ASSETS array as the single source of truth; alternatively, if you prefer JS-only, change the header comment to say "JS assets only" instead of "JS/CSS". Reference: ASSETS and the HTML rewrite that matches src="...".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/error-reporter.js`:
- Around line 97-104: The current redaction/truncation is only applied in the
console.error branch; move normalization into reportError so all reporting paths
(console error, window error, unhandledrejection) are sanitized centrally.
Update reportError to run redactSensitive and safeTruncate on its main string
parameters and on any values inside the extra payload before sending to the
server, ensuring message, stack, url, resource and other string fields are
normalized; then re-run fingerprinting so the hashed bundle stays in sync.
Ensure callers like the console handler and the handlers for
window.error/unhandledrejection simply pass raw data to reportError and do not
duplicate sanitization.
---
Nitpick comments:
In `@public/diagnostics.html`:
- Around line 171-180: This page duplicates the theme toggle logic (the DOM
selectors themeToggle and themeIcon, toggling
document.documentElement.classList, updating themeIcon.className, and writing
localStorage) that already lives in the shared public/theme.js; remove this
local block and rely on the shared bundle instead — delete the click handler and
initial icon setting here and ensure the page includes/uses the existing
public/theme.js (scripts/fingerprint.js will rewrite references) so all theme
behavior is sourced from the single implementation.
In `@scripts/fingerprint.js`:
- Around line 25-31: The comment/behavior mismatch comes from ASSETS claiming
JS/CSS support while the HTML rewrite only replaces src="..." occurrences;
update the HTML rewrite logic that currently targets src attributes so it also
matches href attributes (or both src|href) when rewriting filenames from ASSETS,
ensuring stylesheet links get their fingerprinted filenames, and keep the ASSETS
array as the single source of truth; alternatively, if you prefer JS-only,
change the header comment to say "JS assets only" instead of "JS/CSS".
Reference: ASSETS and the HTML rewrite that matches src="...".
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 26aa1a1f-3c69-4ab5-a3f2-683edf417c36
📒 Files selected for processing (8)
public/asset-manifest.jsonpublic/diagnostics.htmlpublic/error-reporter.71a8b085.jspublic/error-reporter.jspublic/how-it-works.htmlpublic/index.htmlpublic/test-error.htmlscripts/fingerprint.js
✅ Files skipped from review due to trivial changes (2)
- public/asset-manifest.json
- public/test-error.html
🚧 Files skipped from review as they are similar to previous changes (1)
- public/how-it-works.html
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
public/diagnostics.html (1)
58-61:⚠️ Potential issue | 🟡 MinorMatch the accessibility fix on the diagnostics toggle.
This is still an icon-only button with only a
title, so screen readers won't get a reliable name.public/index.htmlalready applies the right pattern to the same control.🔧 Suggested change
- <button data-theme-toggle + <button data-theme-toggle aria-label="Toggle theme" class="rounded-lg p-2 text-slate-500 hover:bg-slate-100 dark:text-slate-400 dark:hover:bg-slate-700 transition-colors" title="Toggle dark/light mode"> - <i class="fas fa-moon" data-theme-icon></i> + <i class="fas fa-moon" data-theme-icon aria-hidden="true"></i>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/diagnostics.html` around lines 58 - 61, The diagnostics theme toggle (element with data-theme-toggle and its icon data-theme-icon) is an icon-only button without an accessible name; update it to match the other toggle pattern by adding an explicit accessible label (for example add a visually-hidden <span> with the text "Toggle dark/light mode" inside the button or add aria-label="Toggle dark/light mode" on the button) so screen readers get a reliable name; ensure the existing title can remain but do not rely on it alone and keep data-theme-icon unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/error-reporter.9455923c.js`:
- Around line 92-93: The reported page URL currently sends full location.href
(which leaks query and hash) to the backend; update the error payload creation
where { url: location.href, resource: resourceUrl } is used (and the similar
occurrences around the other listed locations) to instead send a stripped URL
built from the location object (e.g., origin + pathname) or by constructing a
URL and omitting search and hash so query strings and fragments are removed
before forwarding to Slack; ensure you update every occurrence (the one shown
and the other similar blocks at the referenced ranges) so all reported urls are
sanitized.
- Around line 87-93: The code is reporting every IMG load failure, including
images with inline self-healing fallbacks; update the conditional before calling
reportError in the error reporter so IMG errors with inline handlers or explicit
opt-outs are ignored: specifically, when target.tagName === 'IMG' skip reporting
if target.hasAttribute('onerror') or target.hasAttribute('data-ignore-error')
(or similar attribute you add), otherwise preserve reporting for SCRIPT/LINK and
other IMG cases; update the branch around resourceUrl/target.tagName and
reference the reportError call to ensure only unhandled resource failures are
sent.
In `@public/index.html`:
- Around line 3842-3856: The SW purge runs concurrently with data loads, risking
stale cached responses; wrap the localhost purge (the
navigator.serviceWorker.getRegistrations() unregister sequence and caches.keys()
deletes) in an immediately-invoked async function, await the Promise.all results
for unregistration and cache deletion, and ensure that this awaited purge
completes before invoking the data loaders loadAuthState, loadRateLimit,
loadLatestRelease, loadRepos, loadAuthors, and loadPrs (i.e., move/await the
purge block so the loaders run only after the purge finishes).
- Line 31: Remove the defer attribute from the error-reporter script tag so its
global handlers install before bootstrap; specifically update the <script
src="error-reporter.9455923c.js"> inclusion (the file that registers global
error handlers used to capture startup errors from functions like
loadAuthState() and loadRepos()) to load immediately in the head rather than
deferred, ensuring the error-reporter executes before any inline initialization
code.
---
Duplicate comments:
In `@public/diagnostics.html`:
- Around line 58-61: The diagnostics theme toggle (element with
data-theme-toggle and its icon data-theme-icon) is an icon-only button without
an accessible name; update it to match the other toggle pattern by adding an
explicit accessible label (for example add a visually-hidden <span> with the
text "Toggle dark/light mode" inside the button or add aria-label="Toggle
dark/light mode" on the button) so screen readers get a reliable name; ensure
the existing title can remain but do not rely on it alone and keep
data-theme-icon unchanged.
🪄 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: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 67b9abd5-7f39-4d45-8419-d821370da0fc
📒 Files selected for processing (7)
public/asset-manifest.jsonpublic/diagnostics.htmlpublic/error-reporter.9455923c.jspublic/error-reporter.jspublic/how-it-works.htmlpublic/index.htmlpublic/test-error.html
✅ Files skipped from review due to trivial changes (2)
- public/asset-manifest.json
- public/test-error.html
🚧 Files skipped from review as they are similar to previous changes (2)
- public/error-reporter.js
- public/how-it-works.html
There was a problem hiding this comment.
🧹 Nitpick comments (3)
public/error-reporter.9455923c.js (1)
60-64: Consider using a Blob with explicit Content-Type for sendBeacon.
navigator.sendBeacon(url, body)with a string sends it astext/plain, notapplication/json. While the backend'srequest.json()may still parse the body correctly, using a Blob ensures the correct Content-Type header is sent.♻️ Optional fix for explicit Content-Type
var ok = false; try { - ok = navigator.sendBeacon('/api/client-error', body); + ok = navigator.sendBeacon('/api/client-error', new Blob([body], { type: 'application/json' })); } catch (e) { ok = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/error-reporter.9455923c.js` around lines 60 - 64, The sendBeacon call currently sends a string (body) which defaults to text/plain; update the navigator.sendBeacon usage in the try block to send a Blob with explicit Content-Type 'application/json' (e.g., new Blob([JSON.stringify(body)], { type: 'application/json' })) so the backend receives application/json; keep the existing try/catch and assignment to ok, replacing the second argument to navigator.sendBeacon with the JSON Blob while ensuring body is JSON-serializable before creating the Blob.public/index.html (2)
3940-3940: Verify the theme script path is correct.The script source is
theme.196e3e21.js(no leading/), making it relative to the current page path. If this page is accessed at a non-root path (e.g.,/app/index.html), the browser would try to load/app/theme.196e3e21.jsinstead of/theme.196e3e21.js.The error-reporter at line 31 uses an absolute path (
/error-reporter.9455923c.js). Consider using the same pattern for consistency:♻️ Use absolute path for theme script
- <script src="theme.196e3e21.js"></script> + <script src="/theme.196e3e21.js"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` at line 3940, The theme script tag uses a relative path ("theme.196e3e21.js") which can break on non-root URLs; change the src to an absolute path (e.g., "/theme.196e3e21.js") to match the error-reporter pattern and ensure the browser always loads the correct asset; locate the <script> element referencing "theme.196e3e21.js" in public/index.html and update its src to the absolute path.
3856-3860:window.updateThemeIconsis not exported from theme.js — this call is dead code.Per context snippet 4 from
public/theme.js, onlytoggleThemeis exported towindow:window.toggleTheme = toggleTheme;The
updateThemeIconsfunction is not exported, so the typeof check at line 3857 always fails and the call never executes. This is harmless since theme.js callsupdateThemeIcons()internally via its ownDOMContentLoadedlistener, but the code here is misleading.Either export
updateThemeIconsfrom theme.js or remove this dead code:♻️ Option A: Remove dead code from bootstrapPageData
(async function bootstrapPageData() { - if (typeof window.updateThemeIcons === 'function') { - window.updateThemeIcons(); - } - await setupServiceWorkerForEnvironment();♻️ Option B: Export updateThemeIcons in theme.js
Add to
public/theme.js:// Export for other scripts window.toggleTheme = toggleTheme; +window.updateThemeIcons = updateThemeIcons;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/index.html` around lines 3856 - 3860, The bootstrapPageData IIFE contains a dead check/call for window.updateThemeIcons (window.updateThemeIcons() in bootstrapPageData) but theme.js only exports window.toggleTheme; either remove the dead code from bootstrapPageData (delete the typeof check and call to window.updateThemeIcons) or export updateThemeIcons from theme.js by adding window.updateThemeIcons = updateThemeIcons alongside window.toggleTheme; update the file that defines bootstrapPageData (remove the check/call) or update theme.js (add the export) so the symbols bootstrapPageData and updateThemeIcons are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@public/error-reporter.9455923c.js`:
- Around line 60-64: The sendBeacon call currently sends a string (body) which
defaults to text/plain; update the navigator.sendBeacon usage in the try block
to send a Blob with explicit Content-Type 'application/json' (e.g., new
Blob([JSON.stringify(body)], { type: 'application/json' })) so the backend
receives application/json; keep the existing try/catch and assignment to ok,
replacing the second argument to navigator.sendBeacon with the JSON Blob while
ensuring body is JSON-serializable before creating the Blob.
In `@public/index.html`:
- Line 3940: The theme script tag uses a relative path ("theme.196e3e21.js")
which can break on non-root URLs; change the src to an absolute path (e.g.,
"/theme.196e3e21.js") to match the error-reporter pattern and ensure the browser
always loads the correct asset; locate the <script> element referencing
"theme.196e3e21.js" in public/index.html and update its src to the absolute
path.
- Around line 3856-3860: The bootstrapPageData IIFE contains a dead check/call
for window.updateThemeIcons (window.updateThemeIcons() in bootstrapPageData) but
theme.js only exports window.toggleTheme; either remove the dead code from
bootstrapPageData (delete the typeof check and call to window.updateThemeIcons)
or export updateThemeIcons from theme.js by adding window.updateThemeIcons =
updateThemeIcons alongside window.toggleTheme; update the file that defines
bootstrapPageData (remove the check/call) or update theme.js (add the export) so
the symbols bootstrapPageData and updateThemeIcons are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 40d1e8e6-9a50-4241-8d7e-6d0ee27d3d1d
📒 Files selected for processing (3)
public/error-reporter.9455923c.jspublic/error-reporter.jspublic/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- public/error-reporter.js
|
@DonnieBLT Ready.. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
public/diagnostics.html (1)
171-177: Consider adding null checks for defensive coding.If the data attributes are ever removed or renamed,
themeToggle.addEventListenerwould throw. A null check would make this more resilient:🛡️ Optional defensive check
const themeToggle = document.querySelector('[data-theme-toggle]'); const themeIcon = document.querySelector('[data-theme-icon]'); +if (themeToggle && themeIcon) { themeToggle.addEventListener('click', () => { const isDark = document.documentElement.classList.toggle('dark'); themeIcon.className = isDark ? 'fas fa-sun' : 'fas fa-moon'; localStorage.setItem('theme', isDark ? 'dark' : 'light'); }); // Set initial icon if (document.documentElement.classList.contains('dark')) { themeIcon.className = 'fas fa-sun'; } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/diagnostics.html` around lines 171 - 177, The event handler assumes querySelector returned elements; add null checks before using them: verify themeToggle is non-null before calling themeToggle.addEventListener and verify themeIcon exists (or use optional chaining) before setting themeIcon.className so the script won’t throw if the data attributes are missing or renamed; keep the same behavior (toggle document.documentElement.classList, update localStorage) inside the guarded block.public/error-reporter.1b9dc33f.js (1)
170-175: Non-Error console.error calls may still generate noise.The
elsebranch reports allconsole.errorcalls that don't contain anErrorinstance. This could forward diagnostic/debug logs to Slack (e.g.,console.error("Debug:", someObject)), potentially causing alert fatigue.Consider gating this on a more restrictive condition, or adding a
sourcemarker that the backend can filter differently.💡 Optional: only report non-Error calls if they look like real errors
} else if (msg) { - reportError('ConsoleError', msg, msg, { + // Only report if message looks like an error, not debug output + var looksLikeError = /error|exception|fail|crash/i.test(msg); + if (looksLikeError) { + reportError('ConsoleError', msg, msg, { url: getPageUrl(), - source: 'console.error', - }); + source: 'console.error:filtered', + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/error-reporter.1b9dc33f.js` around lines 170 - 175, The current else branch sends every non-Error console.error to reportError which causes noise; update the handling in the console.error branch that calls reportError('ConsoleError', msg, msg, { url: getPageUrl(), source: 'console.error' }) to either (A) gate non-Error reports with a conservative heuristic (e.g., only report when typeof msg === 'string' && /error|failed|exception/i.test(msg') or when an additional context flag is present) or (B) mark them so the backend can filter (e.g., change the metadata to source: 'console.error:non-error' or add { nonError: true }) and only send full alerts for true Error instances; apply this change where reportError and getPageUrl are invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@public/diagnostics.html`:
- Around line 171-177: The event handler assumes querySelector returned
elements; add null checks before using them: verify themeToggle is non-null
before calling themeToggle.addEventListener and verify themeIcon exists (or use
optional chaining) before setting themeIcon.className so the script won’t throw
if the data attributes are missing or renamed; keep the same behavior (toggle
document.documentElement.classList, update localStorage) inside the guarded
block.
In `@public/error-reporter.1b9dc33f.js`:
- Around line 170-175: The current else branch sends every non-Error
console.error to reportError which causes noise; update the handling in the
console.error branch that calls reportError('ConsoleError', msg, msg, { url:
getPageUrl(), source: 'console.error' }) to either (A) gate non-Error reports
with a conservative heuristic (e.g., only report when typeof msg === 'string' &&
/error|failed|exception/i.test(msg') or when an additional context flag is
present) or (B) mark them so the backend can filter (e.g., change the metadata
to source: 'console.error:non-error' or add { nonError: true }) and only send
full alerts for true Error instances; apply this change where reportError and
getPageUrl are invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 098c4a5f-e166-4eb0-a9c5-f426b23c63ac
📒 Files selected for processing (8)
public/asset-manifest.jsonpublic/diagnostics.htmlpublic/error-reporter.1b9dc33f.jspublic/error-reporter.jspublic/how-it-works.htmlpublic/index.htmlpublic/test-error.htmlsrc/index.py
✅ Files skipped from review due to trivial changes (2)
- public/asset-manifest.json
- public/error-reporter.js
🚧 Files skipped from review as they are similar to previous changes (3)
- public/test-error.html
- public/how-it-works.html
- public/index.html








Fixes layout and theme toggle on "How it works page"
Summary by CodeRabbit
New Features
Bug Fixes
Refactor