Auto-select the sole coordinate system - #75
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughConsolidates two separate effects in ChangesCoordinate System Selection Fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vis/src/SpatialCanvas/index.tsx (2)
460-471: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEffect re-triggers itself on auto-select, causing a redundant
actions.reset().
coordinateSystemis both read and written by this effect. When it starts falsy andcoordinateSystems.length === 1,actions.setCoordinateSystem(coordinateSystems[0])changes the store'scoordinateSystemto a genuinely new value, which re-runs this same effect (sincecoordinateSystemis a dependency) and callsactions.reset()a second time. Previously the "preserve current selection" branch calledsetCoordinateSystemwith the same value it already held, so it never changed the subscribed slice and never re-triggered itself — the new defaulting branch is the first path that can do so.React 18's automatic batching means both
set()calls inside one effect run get batched into a single commit, so there's no visible flicker, but the effect body (includingreset(), which also clearslayers/layerOrder/selectedLayerId/viewState) still executes twice for this transition. This is currently harmless at mount, but is a fragile coupling if more logic is ever added toreset().Consider decoupling "is the current selection still valid" (read via a ref, not a dependency) from "did the available
coordinateSystemschange" (the real trigger for reset), so the effect doesn't re-run because of its own write.♻️ Illustrative refactor (verify against desired "reset on manual switch" semantics)
+ const coordinateSystemRef = useRef(coordinateSystem); + coordinateSystemRef.current = coordinateSystem; + useEffect(() => { actions.reset(); - const nextCoordinateSystem = - coordinateSystem && coordinateSystems.includes(coordinateSystem) - ? coordinateSystem - : coordinateSystems.length === 1 - ? coordinateSystems[0] - : null; + const current = coordinateSystemRef.current; + const nextCoordinateSystem = + current && coordinateSystems.includes(current) + ? current + : coordinateSystems.length === 1 + ? coordinateSystems[0] + : null; if (nextCoordinateSystem) { actions.setCoordinateSystem(nextCoordinateSystem); } - }, [coordinateSystem, coordinateSystems, actions]); + }, [coordinateSystems, actions]);Note: this changes when
reset()fires — confirm whether manual coordinate-system switches (viahandleCSChange) are still meant to clear layers/selection before adopting this.🤖 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 460 - 471, The useEffect in SpatialCanvas is re-triggering itself because it both reads and writes coordinateSystem, causing a redundant actions.reset() when auto-selecting the single available coordinate system. Decouple the reset trigger from the selected value by basing the effect on coordinateSystems changes only, and use a ref or equivalent to check whether the current coordinateSystem is still valid before calling actions.setCoordinateSystem. Keep the reset/set logic in SpatialCanvas and ensure handleCSChange semantics still behave as intended.
460-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new selection matrix.
This effect now encodes several distinct, user-facing branches (0 systems, exactly 1, N with valid current selection, N with stale/invalid current selection). Given the PR explicitly fixes a prior UX bug here, a small unit/integration test around
SpatialCanvasInner's coordinate-system selection would guard against regressions.🤖 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 460 - 471, Add test coverage around SpatialCanvasInner’s coordinate-system selection effect, since it now has distinct branches for no available systems, exactly one system, a valid current selection, and a stale/invalid selection. Create focused unit/integration tests that exercise the useEffect logic in SpatialCanvasInner and verify actions.reset and actions.setCoordinateSystem are called appropriately for each case, so the new selection matrix is protected from regressions.
🤖 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.
Nitpick comments:
In `@packages/vis/src/SpatialCanvas/index.tsx`:
- Around line 460-471: The useEffect in SpatialCanvas is re-triggering itself
because it both reads and writes coordinateSystem, causing a redundant
actions.reset() when auto-selecting the single available coordinate system.
Decouple the reset trigger from the selected value by basing the effect on
coordinateSystems changes only, and use a ref or equivalent to check whether the
current coordinateSystem is still valid before calling
actions.setCoordinateSystem. Keep the reset/set logic in SpatialCanvas and
ensure handleCSChange semantics still behave as intended.
- Around line 460-471: Add test coverage around SpatialCanvasInner’s
coordinate-system selection effect, since it now has distinct branches for no
available systems, exactly one system, a valid current selection, and a
stale/invalid selection. Create focused unit/integration tests that exercise the
useEffect logic in SpatialCanvasInner and verify actions.reset and
actions.setCoordinateSystem are called appropriately for each case, so the new
selection matrix is protected from regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2a4fffc-bf4e-4e8f-9471-964816b1ca10
📒 Files selected for processing (2)
.changeset/default-coordinate-system-selection.mdpackages/vis/src/SpatialCanvas/index.tsx
Harvested from the larger points-loading branch as an isolated, low-risk change.
When a SpatialData object exposes exactly one coordinate system, select it by default instead of leaving the picker on "Select a coordinate system". Multi-system datasets still require an explicit choice (the previous behaviour eagerly picked the first of several, which this also removes).
packages/vis/src/SpatialCanvas/index.tsx@spatialdata/visincluded.🤖 Generated with Claude Code
Summary by CodeRabbit