Repository navigation
feat: Kepler: Removing currentMapId to align with artifacts - #595
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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 (2)
📝 WalkthroughWalkthroughThis PR refactors Kepler's map selection from an implicit ChangesMap Selection Refactor: currentMapId → Artifact-Driven
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/kepler/src/components/CustomFilterPanelHeader.tsx (1)
88-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing backtick:
${filter.id}is not interpolated in the error message.Line 89 uses single quotes — the thrown error will literally read
"Filter index not found by id ${filter.id}"rather than including the actual filter ID value.🐛 Proposed fix
- throw new Error('Filter index not found by id ${filter.id}'); + throw new Error(`Filter index not found by id ${filter.id}`);🤖 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/kepler/src/components/CustomFilterPanelHeader.tsx` around lines 88 - 90, The error message thrown in CustomFilterPanelHeader when mapId is falsy or filterIdx is undefined uses single quotes so `${filter.id}` is not interpolated; change the throw in the block that checks mapId and filterIdx to use a template literal (or string concatenation) so the actual filter.id value is included in the thrown Error (refer to variables filter.id, mapId, filterIdx and the throw site in CustomFilterPanelHeader.tsx).
🧹 Nitpick comments (1)
packages/kepler/src/KeplerSlice.ts (1)
869-871: 💤 Low valueDead
&& mapIdguard — unreachable after the unconditional throw above it.Line 871:
if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type) && mapId)—mapIdis always truthy here because line 869 already throws when it'sundefined. The&& mapIdcondition can be removed.🔧 Proposed fix
- if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type) && mapId) { + if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type)) {🤖 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/kepler/src/KeplerSlice.ts` around lines 869 - 871, In KeplerSlice (around the middleware handling where mapId is retrieved and next(action) is called), remove the redundant "&& mapId" from the if condition since mapId is already guaranteed by the preceding "if (!mapId) throw" check; change "if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type) && mapId)" to simply "if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type))" (leave the surrounding logic that throws when mapId is missing and the call to next(action) intact).
🤖 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.
Outside diff comments:
In `@packages/kepler/src/components/CustomFilterPanelHeader.tsx`:
- Around line 88-90: The error message thrown in CustomFilterPanelHeader when
mapId is falsy or filterIdx is undefined uses single quotes so `${filter.id}` is
not interpolated; change the throw in the block that checks mapId and filterIdx
to use a template literal (or string concatenation) so the actual filter.id
value is included in the thrown Error (refer to variables filter.id, mapId,
filterIdx and the throw site in CustomFilterPanelHeader.tsx).
---
Nitpick comments:
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 869-871: In KeplerSlice (around the middleware handling where
mapId is retrieved and next(action) is called), remove the redundant "&& mapId"
from the if condition since mapId is already guaranteed by the preceding "if
(!mapId) throw" check; change "if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type)
&& mapId)" to simply "if (!SKIP_AUTO_SAVE_ACTIONS.includes(action.type))" (leave
the surrounding logic that throws when mapId is missing and the call to
next(action) intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3d92440-fc6b-4e10-afc8-a6fa0c5f07f5
📒 Files selected for processing (10)
examples/kepler/src/components/KeplerMapsContainer.tsxexamples/kepler/src/components/useCurrentMap.tsexamples/kepler/src/store.tspackages/kepler-config/src/index.tspackages/kepler/README.mdpackages/kepler/src/KeplerSlice.tspackages/kepler/src/components/CustomDndContext.tsxpackages/kepler/src/components/CustomFilterPanelHeader.tsxpackages/kepler/src/hooks/useDndEffects.tspackages/kepler/src/hooks/useDndLayers.ts
💤 Files with no reviewable changes (2)
- packages/kepler-config/src/index.ts
- examples/kepler/src/components/KeplerMapsContainer.tsx
There was a problem hiding this comment.
Pull request overview
This PR refactors @sqlrooms/kepler to remove the slice-owned “current map” concept (currentMapId) so host applications (especially those using @sqlrooms/artifacts) own map/tab selection, enabling more robust multi-map scenarios.
Changes:
- Removed
KeplerSliceConfig.currentMapIdand the corresponding runtime APIs (getCurrentMap,setCurrentMapId), shifting selection responsibility to the host/app layer. - Updated Kepler initialization and drag-and-drop plumbing to work with an explicit/derived
mapId(including support formapIdbeingundefinedin DnD hooks). - Updated docs and the Kepler example app to derive the active map from artifact selection and documented the new selection model + migration guidance.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/kepler/src/KeplerSlice.ts | Removes currentMapId-based behaviors and initializes/registers Kepler redux state per configured map. |
| packages/kepler/src/hooks/useDndLayers.ts | Accepts mapId?: string and safely no-ops DnD handlers when no map is resolved. |
| packages/kepler/src/hooks/useDndEffects.ts | Accepts mapId?: string and safely no-ops DnD handlers when no map is resolved. |
| packages/kepler/src/components/CustomFilterPanelHeader.tsx | Derives mapId by locating the map that owns the filter instead of relying on currentMapId. |
| packages/kepler/src/components/CustomDndContext.tsx | Derives mapId from the provided visState by matching it to a map entry in store state. |
| packages/kepler/README.md | Documents the new selection model and updates example usage away from currentMapId. |
| packages/kepler-config/src/index.ts | Updates persisted schema to remove currentMapId while keeping legacy migration support. |
| examples/kepler/src/store.ts | Moves “current map” determination to artifacts selection and uses that for actions like adding tables after file load. |
| examples/kepler/src/components/useCurrentMap.ts | Updates hook to resolve current map from artifact selection rather than Kepler slice state. |
| examples/kepler/src/components/KeplerMapsContainer.tsx | Removes syncing artifact tab selection into Kepler’s (now-removed) currentMapId. |
| docs/upgrade-guide.md | Adds a breaking-change note and migration guidance for apps moving selection to @sqlrooms/artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mapId = useStoreWithKepler( | ||
| // TODO: pass the mapId via the props, currentMapId is not necessarily the map where filter is originated from | ||
| (state) => state.kepler.config.currentMapId, | ||
| (state) => | ||
| Object.entries(state.kepler.map).find(([, mapState]) => | ||
| mapState?.visState.filters.some((f: Filter) => f.id === filter.id), | ||
| )?.[0], |
Summary by CodeRabbit
New Features
addFilemethod to load external files into Kepler maps.Documentation
Refactor