Skip to content

fix: restore memory graph canvas height blocked by CSP style-src-elem nonce#415

Closed
Brixyy wants to merge 1 commit intobuilderz-labs:mainfrom
Brixyy:fix/memory-graph-canvas-height
Closed

fix: restore memory graph canvas height blocked by CSP style-src-elem nonce#415
Brixyy wants to merge 1 commit intobuilderz-labs:mainfrom
Brixyy:fix/memory-graph-canvas-height

Conversation

@Brixyy
Copy link
Contributor

@Brixyy Brixyy commented Mar 16, 2026

Fixes #414

Summary

  • Root cause: style-src-elem directive included a per-request nonce. CSP Level 3 ignores 'unsafe-inline' when a nonce is present, so reagraph's dynamic <style> injection (which has no nonce) was blocked by the browser. The reagraph canvas container uses injected CSS (position: absolute; inset: 0) to fill its parent — without it the element has no height, causing the <canvas> to fall back to the browser default of 150 px. All nodes then render in a 150 px sliver at the top of the panel.
  • Fix in csp.ts: remove nonce from style-src-elem, rely on 'unsafe-inline' (matching style-src and style-src-attr which already used it).
  • Fix in memory-graph.tsx: extend fitNodesInView retry window from 2 s to 8 s — d3-force needs ~300 ticks (≈ 5 s at 60 fps) to fully converge before the camera fit produces a stable result.

Test plan

  • Navigate to Memory → Graph — nodes fill the full canvas area
  • Verify canvas.clientHeight equals panel height (not 150)
  • Switch between agents — nodes re-center after layout settles
  • Run pnpm test — CSP unit test passes with updated assertion

🤖 Generated with Claude Code

@Brixyy Brixyy requested a review from 0xNyk as a code owner March 16, 2026 15:40
@Brixyy Brixyy force-pushed the fix/memory-graph-canvas-height branch from 2b7494b to c218761 Compare March 16, 2026 16:18
Copy link
Member

@0xNyk 0xNyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: REQUEST CHANGES

Summary

This PR makes justified CSP changes for reagraph compatibility and extends memory-graph fit timing. However, it has overlap with PR #413 and needs cleanup before merging.

Issues Found

  1. Duplicate getSkillRoots() — The workspace discovery logic in this PR is identical to what's in #413. Please extract to a shared module (e.g., src/lib/skill-roots.ts) to avoid divergence.

  2. Memory-graph timing is heuristic — The 4 hardcoded timeouts (800, 2500, 5000, 8000ms) work but are fragile. Consider using a MutationObserver or reagraph's ready callback if available, rather than hoping 8 seconds is enough.

  3. CSP approach is sound — Using unsafe-inline for style-src is an acceptable tradeoff since reagraph injects <style> elements without nonce support. script-src still uses nonce + strict-dynamic, which is good. This is better than PR #397's broader approach.

  4. Tests needed — CSP test was updated (good), but workspace discovery and memory-graph timing have no test coverage.

  5. Needs rebase — Branch has merge conflicts with main.

Requested Changes

  • Rebase on main
  • Extract shared getSkillRoots() to src/lib/skill-roots.ts
  • Add unit tests for workspace discovery
  • Run pnpm typecheck && pnpm test and confirm passing

… nonce

reagraph dynamically injects <style> elements at runtime without a nonce.
CSP Level 3 ignores unsafe-inline when a nonce is present in style-src-elem,
causing the injected styles to be blocked. The reagraph canvas container
(_canvas class) relies on position:absolute/inset:0 from these injected
styles to fill its parent — without them it falls back to the browser
default canvas height of 150px, so all graph nodes render in a tiny sliver
at the top of the panel.

Fix: remove nonce from style-src-elem so unsafe-inline takes effect,
matching style-src and style-src-attr which already use unsafe-inline.
Also extend fitNodesInView retry window to 8 s to cover the full d3-force
convergence time (~300 ticks at 60 fps).
@Brixyy Brixyy force-pushed the fix/memory-graph-canvas-height branch from c218761 to 514c681 Compare March 16, 2026 16:45
@Brixyy
Copy link
Contributor Author

Brixyy commented Mar 16, 2026

Addressed review points:

  1. Rebased on main — fast-forward, no conflicts
  2. Duplicate getSkillRoots() — PR fix: restore memory graph canvas height blocked by CSP style-src-elem nonce #415 never contained getSkillRoots(). The shared extraction was done in PR feat: show per-agent workspace skill roots in Skills Hub #413 (see src/lib/skill-roots.ts). These two PRs are independent and can be merged in any order.
  3. Memory-graph timing — reagraph v4.30.8 has no onLayoutComplete callback. A ResizeObserver would detect container resize but not d3-force convergence. The 4-timeout approach (800/2500/5000/8000ms) is pragmatic: early attempts catch fast layouts; the 8s fallback covers large graphs. Added inline comment explaining the rationale.

@Brixyy
Copy link
Contributor Author

Brixyy commented Mar 16, 2026

CI failure is pre-existing, not caused by this PR.

gnap-sync.test.ts fails with git commit failed: Author identity unknown — the GitHub Actions runner has no git identity configured. This affects all branches including main (runs ccf3f3f, 6f12377 fail identically).

This PR only touches src/lib/csp.ts, src/lib/__tests__/csp.test.ts, and src/components/panels/memory-graph.tsx — none of which are related to the gnap-sync failure.

@0xNyk
Copy link
Member

0xNyk commented Mar 17, 2026

Hey @Brixyy — friendly ping on the requested changes from yesterday's review. Quick summary of what's needed:

  1. CSP nonce approach: The inline nonce attribute on <canvas> won't work because style-src CSP doesn't apply to canvas element dimensions. The fix should set canvas.style.height via JS in a useEffect or use a CSS class instead.
  2. Height value: The 150px default is a browser default for <canvas> elements without explicit dimensions. The fix should derive the height from the container or use 100% rather than hardcoding a pixel value.

Let me know if you'd like to discuss the approach — happy to help.

0xNyk added a commit that referenced this pull request Mar 17, 2026
Replace style-src nonce directive with unsafe-inline to support
reagraph's runtime <style> injection. Add style-src-elem and
style-src-attr directives for CSP Level 3 compliance. Extend
fitNodesInView retries from 2 to 4 for more reliable canvas sizing.

Closes #414
Supersedes #415
@0xNyk
Copy link
Member

0xNyk commented Mar 17, 2026

Thanks for identifying the CSP nonce issue, @Brixyy! We've implemented the fix in #425 which supersedes this PR. The approach uses unsafe-inline for style-src directives (nonce must be dropped since reagraph injects <style> without one) and extends the fitNodesInView retries.

@0xNyk
Copy link
Member

0xNyk commented Mar 17, 2026

Superseded by #425

@0xNyk 0xNyk closed this Mar 17, 2026
0xNyk added a commit that referenced this pull request Mar 17, 2026
Replace style-src nonce directive with unsafe-inline to support
reagraph's runtime <style> injection. Add style-src-elem and
style-src-attr directives for CSP Level 3 compliance. Extend
fitNodesInView retries from 2 to 4 for more reliable canvas sizing.

Closes #414
Supersedes #415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Memory Graph canvas renders at 150px due to CSP blocking reagraph style injection

2 participants