Skip to content

Fix 3D browse-mode follow-ups from post-merge review of #253 - #255

Open
koriym wants to merge 1 commit into
2.xfrom
fix-254-3d
Open

Fix 3D browse-mode follow-ups from post-merge review of #253#255
koriym wants to merge 1 commit into
2.xfrom
fix-254-3d

Conversation

@koriym

@koriym koriym commented Aug 18, 2026

Copy link
Copy Markdown
Member

Addresses the should-fix items and nits from the post-merge review of #253.

Should-fix

  • SRI on CDN loads: loadScript now sets integrity + crossorigin="anonymous" for the 3d-force-graph UMD. Three ≥0.179 ships no UMD build, so its ESM build is loaded as a module script with SRI (populating the SRI-verified module cache) and the existing import() resolves from that cache.
  • unpinNode discards semantic depth: assignDepthLevels stores __asd3dDepthFz; unpinNode clears only fx/fy and restores fz to the depth-derived layer.

Nits

  • Removed unreachable if (lastTagKey === null) refreshGraphData(true) in open3D.
  • Removed unused GRAIN_HALF.
  • Added a warning comment near the version constants about defogLinks/checkParticleArrivals relying on 3d-force-graph private internals.
  • Nodes unreachable from the entry state now get a distinct fallback layer (maxDepth + 1) instead of depth 0.
  • asd3dOverlay escapes the title at the interpolation point (defense in depth).

Tests

Added html-generator.test.ts covering the enable3d/theme branching and title escaping. Full suite: 128 tests pass; tsc --noEmit clean.

Summary by CodeRabbit

  • Security

    • Added integrity verification when loading 3D visualization resources.
    • Profile titles are now safely escaped before appearing in generated HTML.
  • Bug Fixes

    • Improved 3D node depth handling, including disconnected nodes.
    • Fixed initial graph refresh behavior and grain movement in the 3D view.
  • Tests

    • Added coverage for 3D settings, theme handling, script loading, and title escaping.

- Add SRI + crossorigin to the three and 3d-force-graph CDN loads
- Restore the semantic depth layer (fz) when unpinning a focused node
- Give nodes unreachable from the entry state a distinct fallback layer
- Escape the profile title at the asd3dOverlay interpolation point
- Remove dead code in open3D and the unused GRAIN_HALF constant
- Warn that defogLinks/checkParticleArrivals rely on private internals
- Add tests for the enable3d/theme branching and title escaping
@koriym

koriym commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 97e43586-2e2d-4864-b0e0-fbf910a74e0b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d39b9c37-e42f-46a7-86cf-e34f5c92e1a9

📥 Commits

Reviewing files that changed from the base of the PR and between d986560 and 7b4cb8e.

📒 Files selected for processing (3)
  • packages/app-state-diagram/src/generator/html-generator-3d.ts
  • packages/app-state-diagram/src/generator/html-generator.test.ts
  • packages/app-state-diagram/src/generator/html-generator.ts

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 3D overlay now escapes profile titles, uses SRI-validated CDN loading, preserves semantic node depth during unpinning, assigns fallback depth layers, and refreshes the graph through the direct path. Tests cover 3D output, themes, and title escaping.

Changes

3D overlay security and generation

Layer / File(s) Summary
Overlay title and verified asset loading
packages/app-state-diagram/src/generator/html-generator-3d.ts, packages/app-state-diagram/src/generator/html-generator.ts, packages/app-state-diagram/src/generator/html-generator.test.ts
The overlay escapes raw profile titles. Three.js and 3d-force-graph load with SRI validation. Tests cover 3D enablement, theme handling, and escaped titles.

3D graph depth and refresh

Layer / File(s) Summary
Node depth and graph refresh behavior
packages/app-state-diagram/src/generator/html-generator-3d.ts
Unpinning restores saved semantic Z-depth. Unreachable nodes receive a fallback depth layer. Grain bounds use the box size, and 3D opening uses the direct graph refresh path.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 7b4cb

The PR hardens 3D asset loading and preserves node depth when unpinning, with focused tests and clean type checking; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant HtmlGenerator
  participant ThreeModuleLoader
  participant ThreeJsCDN
  participant ForceGraphCDN
  HtmlGenerator->>ThreeModuleLoader: Load verified Three.js module
  ThreeModuleLoader->>ThreeJsCDN: Request module with SRI
  ThreeJsCDN-->>ThreeModuleLoader: Return validated module
  HtmlGenerator->>ForceGraphCDN: Load 3d-force-graph with SRI
  ForceGraphCDN-->>HtmlGenerator: Return validated script
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the 3D browse-mode fixes and links them to the post-merge review that motivated the changes.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-254-3d

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.62%. Comparing base (d986560) to head (7b4cb8e).

Additional details and impacted files
@@             Coverage Diff             @@
##               2.x     #255      +/-   ##
===========================================
- Coverage   100.00%   91.62%   -8.38%     
===========================================
  Files           11       14       +3     
  Lines          850     1027     +177     
  Branches       247      304      +57     
===========================================
+ Hits           850      941      +91     
- Misses           0       62      +62     
- Partials         0       24      +24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant