First pass on 'headless' version of SpatialCanvas component with some notes on future design - #32
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces the "bridge path" implementation for MDV integration: a new headless ChangesMDV Bridge Path Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Clarify MDV-controlled layer state, shapes v1 scope, custom deckLayers, and future editable/write paths. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vis/tests/index.spec.tsx (1)
30-31: ⚡ Quick winCover all new runtime exports in the barrel export test.
Please also assert
shouldRenderInternalTooltipanduseSpatialCanvasRendererhere so root API regressions are caught in one place.As per coding guidelines, "Prefer types that match runtime behavior ... at API boundaries."Suggested test additions
expect(VisExports.composeSpatialDeckLayers).toBeDefined(); expect(VisExports.shouldAutoFitSpatialView).toBeDefined(); + expect(VisExports.shouldRenderInternalTooltip).toBeDefined(); + expect(VisExports.useSpatialCanvasRenderer).toBeDefined();🤖 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 `@packages/vis/tests/index.spec.tsx` around lines 30 - 31, Add runtime assertions for the two missing exports in the barrel export test by adding expectations that shouldRenderInternalTooltip and useSpatialCanvasRenderer are defined (similar to the existing expect(VisExports.composeSpatialDeckLayers).toBeDefined() and expect(VisExports.shouldAutoFitSpatialView).toBeDefined()). Locate the test in the same spec where VisExports is used and add expect(VisExports.shouldRenderInternalTooltip).toBeDefined() and expect(VisExports.useSpatialCanvasRenderer).toBeDefined() so runtime API regressions are caught.
🤖 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 `@docs/docs/vis/mdv-release-checklist.mdx`:
- Line 140: The two MDV docs are inconsistent about Viv PR `#924` (one says
"merged" and mentions deck `9.2.9`, the integration roadmap still lists it as
open); update the wording so both pages match — either mark PR `#924` as merged on
the integration roadmap or revert the release-checklist wording to "open" or use
a date-scoped phrase like "as of <date>, PR `#924` is merged" — ensure the same
phrasing appears for Viv PR `#924` and deck `9.2.9` across the release-checklist
and integration roadmap documents to eliminate ambiguity.
---
Nitpick comments:
In `@packages/vis/tests/index.spec.tsx`:
- Around line 30-31: Add runtime assertions for the two missing exports in the
barrel export test by adding expectations that shouldRenderInternalTooltip and
useSpatialCanvasRenderer are defined (similar to the existing
expect(VisExports.composeSpatialDeckLayers).toBeDefined() and
expect(VisExports.shouldAutoFitSpatialView).toBeDefined()). Locate the test in
the same spec where VisExports is used and add
expect(VisExports.shouldRenderInternalTooltip).toBeDefined() and
expect(VisExports.useSpatialCanvasRenderer).toBeDefined() so runtime API
regressions are caught.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c42ba39-a112-4175-9e73-dd632e3ad00f
📒 Files selected for processing (8)
docs/docs/vis/mdv-integration.mdxdocs/docs/vis/mdv-release-checklist.mdxpackages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsxpackages/vis/src/SpatialCanvas/SpatialViewer.tsxpackages/vis/src/SpatialCanvas/index.tsxpackages/vis/src/index.tspackages/vis/tests/index.spec.tsxpackages/vis/tests/spatialCanvasViewer.spec.ts
Parse and sync the SpatialData store URL from the address bar so MDV can deep-link into the demo. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary by CodeRabbit
New Features
SpatialCanvasViewer: New component for rendering spatial data with support for custom deck.gl layers, automatic view framing, and hover tooltips.SpatialViewernow accepts custom deck.gl configuration options viadeckProps.Documentation