fix(viewer): drop Google Fonts link + surface dashboard load errors (closes #323)#335
Merged
Merged
Conversation
…loses #323) @hem57 reported the viewer dashboard tab stuck on "Loading dashboard..." on Windows under v0.9.11 with CSP violations visible in the browser console. Two narrow fixes that together close the bug premise: 1. Drop the <link rel="stylesheet" href="https://fonts.googleapis.com/ css2?..."> tag. The viewer ships a strict CSP (src/auth.ts:16-30): default-src 'none' style-src 'unsafe-inline' font-src 'self' Note that style-src does NOT include the Google Fonts origin and font-src does NOT include fonts.gstatic.com. Per CSP semantics style-src does not fall back to default-src, so the external stylesheet was blocked outright. On every viewer load the browser logged a CSP violation for the <link>, plus more for each blocked font file. On Windows Edge this produced the screenshot @hem57 shared. The viewer's font stack already declares system fallbacks on every --font-* CSS variable (src/viewer/index.html:39-42): --font-display: 'Playfair Display', Georgia, ... --font-body: 'Lora', Georgia, serif; --font-ui: 'Inter', -apple-system, sans-serif; --font-mono: 'JetBrains Mono', 'SF Mono', ... Dropping the <link> falls back to the system stack with no visual loss on platforms that ship Georgia / SF Mono / system serifs (most macOS, Windows, Linux distros). 2. Wrap loadDashboard() body in try/catch that surfaces failures in the dashboard panel. Before this fix, any uncaught error in the Promise.all of apiGet calls (or in renderDashboard) left the dashboard stuck on the "Loading dashboard..." text forever, with no indication of what went wrong. apiGet() already swallows network/HTTP errors and returns null, so most errors are caught upstream — but renderDashboard can still throw on shape surprises (CSP-blocked event handler binding, undefined object access, etc.). The new catch renders an inline error block with the error message and a pointer to the browser console for full detail. Future reports of "stuck Loading" will at least come with a concrete error message instead of just a screenshot of the placeholder. Out-of-scope (deferred to follow-up): - CSP script-src-attr 'none' blocks inline event handlers in innerHTML-injected dynamic markup (oninput= / onchange= at lines 2706, 2765, 2766, 2852). Browser logs a CSP violation per element but the handlers simply don't bind — the dashboard still loads. Eliminating these requires moving from inline strings to delegated event listeners, ~50 lines of refactor across four search/filter controls. Separate PR. - Self-hosting the fonts (instead of dropping them) would preserve the design intent but adds 200-400KB of font files to the dist. Acceptable tradeoff for now: fonts go, system stack is fine. Test plan: - npm test — 877/877 pass. - npm run build — tsdown clean, dist/viewer/index.html regenerated. - Visual sanity check pending: open localhost:3113 in Chrome dev tools, confirm no CSP violations from font-related requests; force a 502 from one apiGet endpoint and confirm dashboard shows the error message instead of sticking.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR addresses a blocked dashboard by removing an external Google Fonts dependency that violated Content Security Policy and adding error handling to the dashboard load function. The dashboard now displays a helpful error message instead of remaining stuck on "Loading dashboard..." when fetch or rendering failures occur. ChangesViewer dashboard resilience and CSP compliance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/index.html`:
- Around line 1117-1160: The error message string `msg` is injected into HTML
unescaped in the catch block (used when setting `el.innerHTML`), which can lead
to XSS; fix it by passing the message through the existing `esc()` helper (e.g.
use `esc(msg)` or assign `var safeMsg = esc(msg)`) before concatenating into the
HTML, and update the `el.innerHTML` construction to use that escaped value;
check the catch block around `loadDashboard`/`renderDashboard` where `msg` and
`el.innerHTML` are defined to apply this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1117
to
+1160
| try { | ||
| var results = await Promise.all([ | ||
| apiGet('health'), | ||
| apiGet('sessions'), | ||
| apiGet('memories?latest=true'), | ||
| apiGet('graph/stats'), | ||
| apiGet('audit?limit=5'), | ||
| apiGet('semantic'), | ||
| apiGet('procedural'), | ||
| apiGet('relations'), | ||
| apiGet('lessons'), | ||
| apiGet('crystals') | ||
| ]); | ||
| state.dashboard.health = results[0]; | ||
| state.dashboard.sessions = (results[1] && results[1].sessions) || []; | ||
| state.dashboard.memories = (results[2] && results[2].memories) || []; | ||
| state.dashboard.graphStats = results[3]; | ||
| state.dashboard.recentAudit = (results[4] && results[4].entries) || []; | ||
| state.dashboard.semantic = (results[5] && results[5].facts) || (results[5] && results[5].semantic) || []; | ||
| state.dashboard.procedural = (results[6] && results[6].procedures) || (results[6] && results[6].procedural) || []; | ||
| state.dashboard.lessons = (results[8] && results[8].lessons) || []; | ||
| state.dashboard.crystals = (results[9] && results[9].crystals) || []; | ||
| state.dashboard.relations = (results[7] && results[7].relations) || []; | ||
| state.dashboard.loaded = true; | ||
| renderDashboard(); | ||
| } catch (err) { | ||
| // Without this catch, any uncaught error in the await Promise.all | ||
| // or the renderDashboard call leaves the dashboard stuck on | ||
| // "Loading dashboard..." forever with no indication to the user | ||
| // (#323). apiGet() already swallows network/HTTP errors and | ||
| // returns null, but renderDashboard can still throw on shape | ||
| // surprises (CSP-blocked event handler binding, undefined fields). | ||
| // Surface the error in the panel + log full detail to console. | ||
| var msg = (err && err.message) ? err.message : String(err); | ||
| console.error('[viewer] loadDashboard failed:', err); | ||
| el.innerHTML = | ||
| '<div class="loading" style="color:var(--accent);">' + | ||
| 'Dashboard failed to load: ' + msg + | ||
| '<br><br><span style="font-size:12px;color:var(--ink-muted);">' + | ||
| 'Check the browser console for the full error. If you see CSP ' + | ||
| 'violations, please open an issue with the agentmemory version ' + | ||
| '(top-right of the viewer) and the violation text.' + | ||
| '</span></div>'; | ||
| } |
There was a problem hiding this comment.
Escape the error message to prevent potential XSS.
The error message variable msg on line 1150 is concatenated directly into HTML on line 1154 without escaping. Although JavaScript runtime errors are typically safe, error messages can contain property names or values from malformed data. Use the existing esc() helper to eliminate the XSS risk.
🔒 Proposed fix
el.innerHTML =
'<div class="loading" style="color:var(--accent);">' +
- 'Dashboard failed to load: ' + msg +
+ 'Dashboard failed to load: ' + esc(msg) +
'<br><br><span style="font-size:12px;color:var(--ink-muted);">' +Note: The try/catch approach is excellent and correctly addresses #323 by surfacing errors instead of leaving the UI stuck. The thorough comment explaining the rationale (lines 1143–1149) is also helpful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/viewer/index.html` around lines 1117 - 1160, The error message string
`msg` is injected into HTML unescaped in the catch block (used when setting
`el.innerHTML`), which can lead to XSS; fix it by passing the message through
the existing `esc()` helper (e.g. use `esc(msg)` or assign `var safeMsg =
esc(msg)`) before concatenating into the HTML, and update the `el.innerHTML`
construction to use that escaped value; check the catch block around
`loadDashboard`/`renderDashboard` where `msg` and `el.innerHTML` are defined to
apply this change.
5 tasks
rohitg00
added a commit
that referenced
this pull request
May 13, 2026
…ardening Three landed PRs since v0.9.11: - #327 (#295) — BM25 tokenizer now accepts non-ASCII (Greek, accented Latin, Hebrew, Arabic, Cyrillic), VectorIndex.add now actually called at runtime via vectorIndexAddGuarded helper with dim guard + input clip, migrateVectorIndex for dim migrations. - #326 (#277) — RetentionScore type no longer declares source twice; JSDoc back-compat note no longer shadowed. - #335 (#323) — viewer drops Google Fonts <link> (CSP-blocked), loadDashboard now surfaces load errors inline instead of sticking on "Loading dashboard…". Bumping 0.9.11 -> 0.9.12 across the 9 standard files: - package.json - packages/mcp/package.json - plugin/.claude-plugin/plugin.json - plugin/.codex-plugin/plugin.json - src/version.ts - src/types.ts (ExportData.version literal) - src/functions/export-import.ts (supportedVersions) - test/export-import.test.ts (round-trip expectation) - CHANGELOG.md (new 0.9.12 entry) 886 / 886 tests pass. Build clean.
rohitg00
added a commit
that referenced
this pull request
May 13, 2026
…ardening (#337) Three landed PRs since v0.9.11: - #327 (#295) — BM25 tokenizer now accepts non-ASCII (Greek, accented Latin, Hebrew, Arabic, Cyrillic), VectorIndex.add now actually called at runtime via vectorIndexAddGuarded helper with dim guard + input clip, migrateVectorIndex for dim migrations. - #326 (#277) — RetentionScore type no longer declares source twice; JSDoc back-compat note no longer shadowed. - #335 (#323) — viewer drops Google Fonts <link> (CSP-blocked), loadDashboard now surfaces load errors inline instead of sticking on "Loading dashboard…". Bumping 0.9.11 -> 0.9.12 across the 9 standard files: - package.json - packages/mcp/package.json - plugin/.claude-plugin/plugin.json - plugin/.codex-plugin/plugin.json - src/version.ts - src/types.ts (ExportData.version literal) - src/functions/export-import.ts (supportedVersions) - test/export-import.test.ts (round-trip expectation) - CHANGELOG.md (new 0.9.12 entry) 886 / 886 tests pass. Build clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reproduction
@hem57 on #323: viewer dashboard tab stuck on "Loading dashboard..." under v0.9.11 on Windows with CSP violations visible in browser dev tools.
Root cause (verified)
Viewer CSP at
src/auth.ts:16-30:src/viewer/index.html:7shipped a<link rel="stylesheet">pointing atfonts.googleapis.com. CSPstyle-srcdoes NOT fall back todefault-src, so:fonts.gstatic.comare blocked byfont-src 'self'→ more violations.That's the violation @hem57 saw. On Windows Edge in particular, the pile of CSP errors in the console matched the issue title verbatim.
Compounding: when
loadDashboard()(orrenderDashboard()) hits an exception mid-flight, the placeholder text "Loading dashboard..." sits there forever — no surfaced error, no console hint of which call failed.apiGet()already swallows network/HTTP errors and returns null, so the most-common failure mode is silent — but renderDashboard itself can still throw on edge shapes.Fix
1. Drop the Google Fonts link
The viewer's font stack already has system fallbacks on every
--font-*CSS variable (lines 39-42):'Playfair Display', Georgia, ...'Lora', Georgia, serif'Inter', -apple-system, sans-serif'JetBrains Mono', 'SF Mono', 'Fira Code', monospaceDropping the
<link>falls to system fonts. Visually almost identical on macOS / Windows / most Linux. Zero CSP violations.2. Surface load errors in the dashboard panel
loadDashboard()body wrapped in try/catch that renders the error inline:Future "stuck Loading" reports come with concrete error messages instead of a screenshot of the placeholder.
Out-of-scope (deferred follow-up)
Inline event handlers (
oninput=/onchange=at lines 2706, 2765, 2766, 2852) are blocked byscript-src-attr 'none'— browser logs CSP violations per element, handlers don't bind, but the dashboard still loads. Eliminating these requires moving to delegated listeners (~50 lines across four search/filter controls). Separate PR.Self-host the fonts would preserve the design intent but adds 200-400KB to dist. Acceptable tradeoff: fonts go, system stack is fine.
Test plan
npm test— 877 / 877 pass.npm run build— tsdown clean.apiGetendpoint (e.g. block the REST proxy mid-flight), confirm dashboard shows the error message instead of sticking on "Loading...".Closes #323. Thanks @hem57 for the Windows repro screenshot.
Summary by CodeRabbit