Skip to content

Clear React Hooks lint backlog; SpatialCanvas picking/tooltip perf + hoverTooltipMode - #79

Merged
xinaesthete merged 10 commits into
mainfrom
claude/amazing-swanson-131327
Jul 2, 2026
Merged

Clear React Hooks lint backlog; SpatialCanvas picking/tooltip perf + hoverTooltipMode#79
xinaesthete merged 10 commits into
mainfrom
claude/amazing-swanson-131327

Conversation

@xinaesthete

@xinaesthete xinaesthete commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What & why

Two related workstreams in @spatialdata/vis (+ small @spatialdata/layers/@spatialdata/react bits), both centred on the SpatialCanvas/tooltip code.

1. Rules-of-React lint backlog → 0, CI made required

The scoped ESLint setup (pnpm lint:react, eslint-plugin-react-hooks v7) had 19 pre-existing findings. All fixed by fixing the actual pattern, not suppressing:

  • set-state-in-effect — replaced auto-select-first-item effects and derivable async state with derived state (useSpatialData, Transforms, Table, Shapes, ImageView, and the fullscreen-refit path in SpatialCanvas).
  • 10× refs — moved ref reads into event handlers (tooltip client-coords captured at hover time), and converted elementMap to a useMemo-backed value mirrored into a stable ref written during render.
  • Two remaining ref reads are intentional external-store reads (the imperatively-maintained loadedDataRef cache consulted by isBlocking, and the latest-layers mirror) — kept with documented eslint-disables, matching the precedent already in the file.

pnpm lint:react is now 0 findings and the react-lint CI job is flipped to required (removed continue-on-error).

2. Tooltip / picking performance

  • New hoverTooltipMode prop ('off' | 'simple' | 'aggregate', default 'simple') on SpatialCanvas and the headless SpatialCanvasViewer, with a UI selector in the SpatialCanvas controls bar. Replaces the short-lived boolean aggregateHoverTooltips. HoverTooltipMode is exported from the package root.
    • simple: tooltip from the single top-most pick deck already does for hover/highlight (no extra passes).
    • aggregate: adds pickMultipleObjects passes for stacked-layer tooltips (opt-in, more expensive).
    • off: shapes are built non-pickable and deck's onHover is left unwired → zero picking work (the genuinely cheap mode).
  • Shape picking (autoHighlight + pickable) is gated off during pan/zoom via an interaction gate; the per-missing-layer supplemental aggregation pick is collapsed into a single batched pick.
  • Demo dev server (dev:demo) now auto-selects a free port (prefers 5173, falls back; honours PORT) instead of failing hard on --strictPort.

Fixes

  • "Center on layer" stuck disabled — a regression introduced mid-branch when layersRef was moved to an effect; hasRenderableLayerData reads it during render, so an effect-deferred write left the button wrongly disabled. Fixed by restoring the synchronous mirror (and the same class for elementMap).

Reviewer notes

Explicitly out of scope

Profiling (with the user) showed the residual pan/zoom slowness with image + shapes together is vertex-stage cost in the current shape geometry storage/fetch/render — not picking, not React, not decode (HTJ2K-optimised vs unoptimised data perform identically), and not fragment/fill/resolution. That needs the larger shapes-rendering rework and is left for a separate PR. Also deferred: a manual-picking hover path (to remove deck's per-move pickObject readPixels in simple/aggregate during drag — a secondary contributor) and async pick readback (upstream deck.gl).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added hoverTooltipMode for SpatialCanvas tooltips: off, simple, or aggregate, plus a live Tooltips selector.
    • Enabled controlling whether shape layers are pickable (and highlighted) during interactions.
    • Demo/dev server now selects an available port more flexibly.
  • Bug Fixes

    • Improved tooltip behavior during drag/pan/zoom and reduced stale/incorrect hover results.
    • Better recovery of occluded/overlapping features, and more reliable selection + async rendering for images, shapes, tables, and transforms.
  • Chores

    • Strengthened React Hooks linting to fail on new Issues.

xinaesthete and others added 7 commits July 2, 2026 07:42
Tooltip/picking performance:
- Default `aggregateHoverTooltips` to false. Aggregation ran extra
  `pickMultipleObjects` GPU passes per pointer move on top of deck's own
  hover/highlight pick — very costly over large pickable geometry. Single-pick
  hover reuses the existing pick; aggregation is now opt-in.
- Disable shape picking + autoHighlight during pan/zoom via a debounced
  interaction gate (`useViewInteractionGate` -> `pickingEnabled` on the shapes
  layer), so deck doesn't re-render shape geometry into the picking buffer
  mid-gesture.
- Throttle hover tooltip resolution to one run per animation frame, skip
  redundant same-pixel picks, suppress picking while a pointer button is held,
  and batch the per-missing-layer supplemental pick into a single pass.

Rules-of-React (eslint-plugin-react-hooks): clear the 19-finding backlog and
make the `react-lint` CI job required. Replace ref-during-render and
setState-in-effect patterns with derived state across `@spatialdata/react`
useSpatialData and the vis Transforms/Table/Shapes/ImageView/SpatialCanvas
components; document the one intentional external-store ref read in
useLayerData's isBlocking memo.

Adds tests for the batched supplemental pick and the drag-suppression helper.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Rules-of-React cleanup moved `layersRef.current = layers` from render into
a commit-phase effect. But that ref is read synchronously during render by
`hasRenderableLayerData` (and the loaded-data getters consumed via context),
which drives the "Center on layer" button's enablement and the image/labels
panels. Deferring the write let those readers observe a stale layer config on
the render where a layer first appears, so the button could stay disabled
indefinitely with no follow-up render to correct it.

Restore the synchronous mirror (it must be written during render because it is
consumed during render) with a documented eslint-disable. `pnpm lint:react`
stays clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`dev:demo` was pinned to 127.0.0.1:5173 with `--strictPort`, so it failed hard
when 5173 was already taken (e.g. another project's dev server). Prefer 5173 but
fall back to the next free port, and honour the PORT env var when a launcher
pins one.

- vite.config.demo.ts: port = Number(process.env.PORT) || 5173, strictPort: false.
- package.json: drop the hardcoded `--host 127.0.0.1 --port 5173 --strictPort`
  from `dev:demo` (host/port now come from the config).
- .claude/launch.json: run `dev:demo` with autoPort: true so the preview server
  assigns a free port via PORT.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ass)

`elementMap` (layerId/element lookup) was a useRef populated in a
useEffect([availableElements]), yet it's read *during render* by
`hasRenderableLayerData` (which gates the "Center on layer" button) and by
`loadData`. An effect-deferred write means a render/effect that runs before the
write observes a commit-stale map -> element resolution returns undefined ->
button stuck disabled, with no follow-up render to correct it. This is the same
hazard class as the earlier `layersRef` regression.

Fix by computing the map with useMemo (rebuilds only when availableElements
changes) and mirroring it into a stable ref written synchronously during render.
That keeps render-time reads current, preserves stable identity (no dependency-
array churn across the many consuming callbacks / the isBlocking memo / loadData),
and needs a single documented eslint-disable for the intentional latest-value
mirror. `pnpm lint:react` stays clean (no new warnings).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the boolean `aggregateHoverTooltips` with a tri-state `hoverTooltipMode`
on `SpatialCanvas` and the headless `SpatialCanvasViewer`:

- 'simple' (default): tooltip from the single top-most pick deck already does
  for hover/highlight — no extra GPU passes.
- 'aggregate': adds `pickMultipleObjects` passes to include every layer under
  the cursor (more expensive; for stacked-layer tooltips).
- 'off': shape layers are built non-pickable (autoHighlight off, no picking-
  buffer render) and no tooltip is resolved — the cheapest mode.

`SpatialCanvas` exposes a "Tooltips" selector in the controls bar (initial value
from the prop). The renderer hook now takes `pickingEnabled` (mode !== 'off' &&
!interacting) instead of `interacting`. Export `HoverTooltipMode` from the
package root.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Profiling showed the residual readPixels stall in 'off' mode came from deck's own
`DeckPicker.pickObject` (stack: readPixels <- _drawAndSample <- _pickClosestObject
<- pickObject), triggered purely because an `onHover` handler was still passed to
deck. deck runs a pick (a readPixels/glFinish that flushes the in-flight
image+shape render) on pointer events before invoking onHover, even when nothing
is pickable — so 'off' still stalled during pan.

In 'off' mode the shapes are already non-pickable; also leave deck's `onHover`
unset so deck has no reason to pick at all → no pickObject, no readPixels. Makes
'off' the genuinely cheap mode.

(Simple/aggregate still pay deck's per-move pickObject during drag; suppressing
that for the live-hover modes needs the manual-picking follow-up.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rAF throttle + same-pixel-skip in useThrottledHoverTooltip didn't earn its
keep: deck already coalesces hover picking to one pass per animation frame, and
in the default 'simple' mode tooltip resolution just reuses deck's existing pick
(no extra work to throttle). The hook only added a frame of latency and
indirection.

Drop the hook and call `resolveTooltip` straight from the hover handlers. Keep
the one durable piece — the `isHoverDuringDrag` pointer-button drag-gate — moved
into `featureTooltipHover` (its natural home) alongside the other hover helpers,
with its test folded into featureTooltipHover.spec.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@xinaesthete, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a00fdba3-9eb0-4888-b782-36733e595af7

📥 Commits

Reviewing files that changed from the base of the PR and between 13b3d30 and 3c1ce50.

📒 Files selected for processing (1)
  • .claude/launch.json
📝 Walkthrough

Walkthrough

Replaces the boolean hover aggregation flag with a tri-state hover mode in SpatialCanvas, adds interaction-gated shape picking, batches supplemental hover picks, and refactors several vis/react components to derive state during render. It also tightens CI lint gating and updates demo-server port handling.

Changes

Hover Tooltip Mode and Picking Gating

Layer / File(s) Summary
Shapes layer pickingEnabled contract
packages/layers/src/shapesLayer.ts, packages/vis/src/SpatialCanvas/renderers/shapesRenderer.ts
Adds pickingEnabled to shapes-layer options and render config, and uses it to control pickable and autoHighlight for polygon and circle/point layers.
View interaction gate hook
packages/vis/src/SpatialCanvas/useViewInteractionGate.ts
Adds a hook and interaction-state interfaces that track when a view is interacting and debounce the return to idle.
Layer data and renderer picking wiring
packages/vis/src/SpatialCanvas/useLayerData.ts
getLayers accepts pickingEnabled, forwards it to shape rendering, and moves element-map construction to render-time memoization.
Hover drag detection and batched supplemental picking
packages/vis/src/SpatialCanvas/featureTooltipHover.ts, packages/vis/tests/featureTooltipHover.spec.ts
Adds drag detection for hover events and changes supplemental hover recovery to a single batched pickMultipleObjects call, with tests for both behaviors.
SpatialCanvasViewer and SpatialCanvas hover mode integration
packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx, packages/vis/src/SpatialCanvas/index.tsx
Introduces hoverTooltipMode, computes picking state from mode and interaction state, rewrites tooltip handling/positioning, and wires the mode through viewer and canvas props.
Public API export of HoverTooltipMode
packages/vis/src/SpatialCanvas/public.ts, packages/vis/src/index.ts
Re-exports HoverTooltipMode through the SpatialCanvas barrel and package index.

Rules-of-React Cleanup in React and Vis Components

Layer / File(s) Summary
Changeset documentation
.changeset/spatialcanvas-picking-perf-and-rules-of-react.md
Updates the changeset entry to describe the hover, picking, and Rules-of-React changes.
useSpatialData derived resolution state
packages/react/src/hooks/useSpatialData.ts
Replaces separate loading, spatialData, and error state with a resolved promise record and derives return values from the current promise.
Derived selection in vis components
packages/vis/src/ImageView/index.tsx, packages/vis/src/Shapes/index.tsx, packages/vis/src/Table/index.tsx, packages/vis/src/Transforms/index.tsx
Derives effective selections and loaded data during render instead of synchronizing them through effects.

CI Lint Gating and Local Dev Server Port Config

Layer / File(s) Summary
React Hooks lint gate
.github/workflows/test.yml
Removes continue-on-error from the React lint step and updates the surrounding comments.
Vis demo dev server port configuration
.claude/launch.json, packages/vis/package.json, packages/vis/vite.config.demo.ts, packages/vis/scripts/dev.mjs
Updates demo launch and startup wiring to use configurable port selection with fallback behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Deck as deck.gl
  participant Gate as useViewInteractionGate
  participant Canvas as SpatialCanvasViewer
  participant Hover as resolveHoverFeatureTooltip
  Deck->>Gate: onInteractionStateChange(state)
  Gate-->>Canvas: interacting
  Deck->>Canvas: onHover(pickInfo, event)
  Canvas->>Canvas: isHoverDuringDrag(event)
  alt tooltip mode enabled and not dragging
    Canvas->>Hover: resolveHoverFeatureTooltip(aggregate/simple)
    Hover->>Deck: pickMultipleObjects(initial)
    Hover->>Deck: pickMultipleObjects(batched supplemental)
    Hover-->>Canvas: tooltip sections
  else off or dragging
    Canvas-->>Canvas: skip tooltip update
  end
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the two main themes: React Hooks lint cleanup and SpatialCanvas hover/picking changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/amazing-swanson-131327

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/vis/src/SpatialCanvas/index.tsx (1)

627-636: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clear the tooltip when switching tooltips off and avoid the select assertion

  • hoverTooltip can stay rendered after tooltipMode becomes 'off' because nothing clears that state on mode changes.
  • Replace setTooltipMode(e.target.value as HoverTooltipMode) with a narrow onChange handler that accepts only the three allowed values and clears hoverTooltip when turning tooltips off.
🤖 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/src/SpatialCanvas/index.tsx` around lines 627 - 636, Tooltip
state is not cleared when switching `tooltipMode` to off, so `hoverTooltip` can
continue rendering; update the `SpatialCanvas` mode change handler to accept
only the three allowed values instead of using the `setTooltipMode` assertion,
and explicitly clear `hoverTooltip` when the selected mode is off. Make the fix
in the `onChange` logic associated with `tooltipMode` and ensure
`tooltipPayload`/`tooltipClientPosition` stop producing tooltip content once
tooltips are disabled.

Source: Coding guidelines

🧹 Nitpick comments (2)
packages/vis/src/Table/index.tsx (1)

13-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Avoid any for tableData.data.

Use the actual return type of table.getAnnDataJS() (or unknown if it's not exported) instead of any, so currentData retains type safety through to JsonView.

♻️ Suggested typing
-  const [tableData, setTableData] = useState<{ table: unknown; data: any } | undefined>(undefined);
+  const [tableData, setTableData] = useState<
+    { table: unknown; data: Awaited<ReturnType<typeof table.getAnnDataJS>> } | undefined
+  >(undefined);

As per coding guidelines, "Prefer types that match runtime behavior (e.g. union or unknown + narrowers when data shape varies)."

🤖 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/src/Table/index.tsx` around lines 13 - 26, The Table component
stores resolved data in tableData.data using any, which loses type safety for
currentData and JsonView. Update the useState type in Table/index.tsx to use the
real return type from table.getAnnDataJS() (or unknown if that type cannot be
imported/exported), and keep the table/tableData/currentData flow typed
accordingly so the render path preserves safe inference.

Source: Coding guidelines

packages/vis/src/SpatialCanvas/index.tsx (1)

227-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the hover callback type aligned with the forwarded runtime event.

handleHover uses the second event argument for drag suppression, so ViewerSectionProps.onHover should preserve that contract instead of narrowing it to (info) => void. As per coding guidelines, prefer types that match runtime behavior.

Proposed fix
-  onHover?: (info: PickingInfo) => void;
+  onHover?: (info: PickingInfo, event?: HoverPointerEvent) => void;
🤖 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/src/SpatialCanvas/index.tsx` around lines 227 - 228, The hover
callback type is too narrow and no longer matches the runtime contract used by
handleHover. Update ViewerSectionProps.onHover in SpatialCanvas/index.tsx to
preserve the forwarded event argument shape expected by the hover handler, so
consumers can receive the same second event parameter used for drag suppression.
Keep the type aligned with the handler implementation and the PickingInfo-based
hover flow rather than reducing it to a single-argument callback.

Source: Coding guidelines

🤖 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 `@packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx`:
- Around line 493-498: Clear stale hover tooltip state when `hoverTooltipMode`
is switched to `'off'`, because `resolveTooltip`/`onHover` may no longer run and
the portal can keep showing the previous tooltip. Update the
`SpatialCanvasViewer` logic around `resolveTooltip`, the `onHover` handler, and
the hover mode effect so that `setHoverTooltip(null)` is called as soon as hover
tooltips are disabled. Make sure the state reset happens even when the hover
callback is removed, so the portal stops rendering the old tooltip immediately.
- Around line 486-488: The mergedDeckProps composition in SpatialCanvasViewer is
replacing the caller’s onInteractionStateChange handler instead of preserving
it. Update the useMemo that builds mergedDeckProps so it composes
deckProps.onInteractionStateChange with the local onInteractionStateChange
callback, and keep both handlers invoked when DeckGL interaction state changes.

In `@packages/vis/vite.config.demo.ts`:
- Around line 32-36: The dev demo port fallback is still being overridden by the
main launcher, so `pnpm dev` will keep forcing port 5173 and strict port
behavior. Update `packages/vis/scripts/dev.mjs` so it no longer passes hardcoded
`--port 5173 --strictPort`, and instead lets `vite.config.demo.ts` handle the
fallback or respects `process.env.PORT` consistently. Use the existing
`dev:demo` flow and the Vite demo config symbols to keep the port selection
centralized.

---

Outside diff comments:
In `@packages/vis/src/SpatialCanvas/index.tsx`:
- Around line 627-636: Tooltip state is not cleared when switching `tooltipMode`
to off, so `hoverTooltip` can continue rendering; update the `SpatialCanvas`
mode change handler to accept only the three allowed values instead of using the
`setTooltipMode` assertion, and explicitly clear `hoverTooltip` when the
selected mode is off. Make the fix in the `onChange` logic associated with
`tooltipMode` and ensure `tooltipPayload`/`tooltipClientPosition` stop producing
tooltip content once tooltips are disabled.

---

Nitpick comments:
In `@packages/vis/src/SpatialCanvas/index.tsx`:
- Around line 227-228: The hover callback type is too narrow and no longer
matches the runtime contract used by handleHover. Update
ViewerSectionProps.onHover in SpatialCanvas/index.tsx to preserve the forwarded
event argument shape expected by the hover handler, so consumers can receive the
same second event parameter used for drag suppression. Keep the type aligned
with the handler implementation and the PickingInfo-based hover flow rather than
reducing it to a single-argument callback.

In `@packages/vis/src/Table/index.tsx`:
- Around line 13-26: The Table component stores resolved data in tableData.data
using any, which loses type safety for currentData and JsonView. Update the
useState type in Table/index.tsx to use the real return type from
table.getAnnDataJS() (or unknown if that type cannot be imported/exported), and
keep the table/tableData/currentData flow typed accordingly so the render path
preserves safe inference.
🪄 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: f5ccf9bf-8471-4db4-8789-8ae76024cf81

📥 Commits

Reviewing files that changed from the base of the PR and between 67987ee and 1ff4304.

📒 Files selected for processing (20)
  • .changeset/spatialcanvas-picking-perf-and-rules-of-react.md
  • .claude/launch.json
  • .github/workflows/test.yml
  • packages/layers/src/shapesLayer.ts
  • packages/react/src/hooks/useSpatialData.ts
  • packages/vis/package.json
  • packages/vis/src/ImageView/index.tsx
  • packages/vis/src/Shapes/index.tsx
  • packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx
  • packages/vis/src/SpatialCanvas/featureTooltipHover.ts
  • packages/vis/src/SpatialCanvas/index.tsx
  • packages/vis/src/SpatialCanvas/public.ts
  • packages/vis/src/SpatialCanvas/renderers/shapesRenderer.ts
  • packages/vis/src/SpatialCanvas/useLayerData.ts
  • packages/vis/src/SpatialCanvas/useViewInteractionGate.ts
  • packages/vis/src/Table/index.tsx
  • packages/vis/src/Transforms/index.tsx
  • packages/vis/src/index.ts
  • packages/vis/tests/featureTooltipHover.spec.ts
  • packages/vis/vite.config.demo.ts

Comment thread packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx Outdated
Comment on lines +493 to 498
const resolveTooltip = useCallback(
(info: PickingInfo) => {
onHover?.(info);
if (!info.picked || typeof info.x !== 'number' || typeof info.y !== 'number') {
if (hoverTooltipMode === 'off' || !shouldRenderInternalTooltip(renderTooltip)) {
setHoverTooltip(null);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Clear stale tooltip state when hover mode is disabled.

If a tooltip is visible and hoverTooltipMode changes to 'off', onHover is removed, so the off-branch in resolveTooltip may never run and the portal can keep rendering the old tooltip.

Proposed fix
+  useEffect(() => {
+    if (hoverTooltipMode === 'off' || !shouldRenderInternalTooltip(renderTooltip)) {
+      setHoverTooltip(null);
+    }
+  }, [hoverTooltipMode, renderTooltip]);
+
   const tooltipClientPosition = hoverTooltip
     ? { x: hoverTooltip.clientX, y: hoverTooltip.clientY }
     : null;
 
-  const tooltipPayload: SpatialFeatureTooltipData | null = hoverTooltip;
+  const tooltipPayload: SpatialFeatureTooltipData | null =
+    hoverTooltipMode !== 'off' && shouldRenderInternalTooltip(renderTooltip) ? hoverTooltip : null;

Also applies to: 607-611, 676-678

🤖 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/src/SpatialCanvas/SpatialCanvasViewer.tsx` around lines 493 -
498, Clear stale hover tooltip state when `hoverTooltipMode` is switched to
`'off'`, because `resolveTooltip`/`onHover` may no longer run and the portal can
keep showing the previous tooltip. Update the `SpatialCanvasViewer` logic around
`resolveTooltip`, the `onHover` handler, and the hover mode effect so that
`setHoverTooltip(null)` is called as soon as hover tooltips are disabled. Make
sure the state reset happens even when the hover callback is removed, so the
portal stops rendering the old tooltip immediately.

Comment thread packages/vis/vite.config.demo.ts
…ast-free mode guard

- SpatialCanvas/SpatialCanvasViewer: gate the tooltip portal/payload on
  hoverTooltipMode !== 'off' so switching to 'off' (which unwires onHover)
  clears the last tooltip immediately, via derived render rather than an effect.
- SpatialCanvasViewer.mergedDeckProps: compose the caller's
  onInteractionStateChange with the local interaction gate instead of
  overwriting it.
- scripts/dev.mjs: stop passing hardcoded --port 5173 --strictPort and let
  vite.config.demo.ts own port selection (free-port fallback / PORT env); a busy
  demo port is now a heads-up rather than a fatal preflight exit.
- Add cast-free isHoverTooltipMode type guard; use it in the tooltip-mode
  <select> onChange instead of `as HoverTooltipMode`.
- Widen ViewerSection.onHover type to (info, event?) to match handleHover.
- Type Table's resolved data via Awaited<ReturnType<...getAnnDataJS>> not `any`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/vis/scripts/dev.mjs (1)

10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

DEMO_PORT constant ignores PORT env var used by vite.config.demo.ts.

vite.config.demo.ts derives its actual port from process.env.PORT (falling back to 5173), but preflight() here always checks the hardcoded DEMO_PORT (5173). If a caller pins a different port via PORT, the preflight heads-up will check/report the wrong port entirely, making the message misleading. Consider reading process.env.PORT (with the same 5173 fallback) when computing the port to check here, to keep this script's diagnostics in sync with the actual server startup config.

♻️ Proposed fix
-const DEMO_PORT = 5173;
+const DEMO_PORT = Number(process.env.PORT) || 5173;

Also applies to: 36-48

🤖 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/scripts/dev.mjs` at line 10, The preflight port check in dev.mjs
is using a hardcoded DEMO_PORT, which can drift from the actual demo server
port. Update the port calculation in preflight() to read process.env.PORT with
the same 5173 fallback used by vite.config.demo.ts, and use that resolved value
for the availability check and warning output so the diagnostics stay aligned
with the real startup port.
🤖 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 @.claude/launch.json:
- Around line 8-9: Fix the invalid JSON in the launch configuration by adding
the missing trailing comma after the autoPort property so the port field is
parsed correctly; update the object in launch.json near the autoPort and port
entries and ensure the JSON remains valid.

---

Nitpick comments:
In `@packages/vis/scripts/dev.mjs`:
- Line 10: The preflight port check in dev.mjs is using a hardcoded DEMO_PORT,
which can drift from the actual demo server port. Update the port calculation in
preflight() to read process.env.PORT with the same 5173 fallback used by
vite.config.demo.ts, and use that resolved value for the availability check and
warning output so the diagnostics stay aligned with the real startup port.
🪄 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: 2ae3c219-1331-41e4-99f7-37f40283c91d

📥 Commits

Reviewing files that changed from the base of the PR and between 1ff4304 and 13b3d30.

📒 Files selected for processing (5)
  • .claude/launch.json
  • packages/vis/scripts/dev.mjs
  • packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx
  • packages/vis/src/SpatialCanvas/index.tsx
  • packages/vis/src/Table/index.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/vis/src/Table/index.tsx
  • packages/vis/src/SpatialCanvas/SpatialCanvasViewer.tsx
  • packages/vis/src/SpatialCanvas/index.tsx

Comment thread .claude/launch.json Outdated
@xinaesthete
xinaesthete merged commit 8607083 into main Jul 2, 2026
3 checks passed
@xinaesthete
xinaesthete deleted the claude/amazing-swanson-131327 branch July 2, 2026 10:38
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
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