Repository navigation
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReorders Kepler init/config flow to call dataset syncing before map config updates; limits dataset syncing to only dataIds referenced by map visState; replaces the injected Kepler AddLayerButton with a local AddTableLayerButton that reads DuckDB tables and either syncs a dataset or adds a layer. Changes
Sequence DiagramsequenceDiagram
participant User
participant CustomLayerManager
participant ReduxStore
participant addTableToMap
participant addLayer
User->>CustomLayerManager: Click "Add layer"
CustomLayerManager->>ReduxStore: read db.tables, keplerState.visState.datasets
ReduxStore-->>CustomLayerManager: available tables + dataset status
alt single table available
CustomLayerManager->>ReduxStore: is dataset present?
ReduxStore-->>CustomLayerManager: present / missing
alt missing
CustomLayerManager->>addTableToMap: addTableToMap(mapId, tableName, {autoCreateLayers: true, centerMap: true})
else present
CustomLayerManager->>addLayer: addLayer(undefined, tableName)
end
else multiple tables available
CustomLayerManager->>User: show Typeahead dropdown
User->>CustomLayerManager: select tableName
CustomLayerManager->>ReduxStore: is dataset present?
ReduxStore-->>CustomLayerManager: present / missing
alt missing
CustomLayerManager->>addTableToMap: addTableToMap(mapId, tableName, {autoCreateLayers: true, centerMap: true})
else present
CustomLayerManager->>addLayer: addLayer(undefined, tableName)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/kepler/src/components/CustomLayerManager.tsx (2)
266-279: Consider adding error handling for the async operation.The
addTableToMapcall is awaited but errors are not caught. If the DuckDB query or data processing fails, the error will propagate unhandled, potentially leaving the UI in an inconsistent state (e.g., user expects a layer but nothing appears).🛡️ Proposed error handling
const onAddTableLayer = useCallback( async (tableName: string) => { const keplerDatasets = keplerState?.visState.datasets; if (!keplerDatasets?.[tableName]) { - await addTableToMap(mapId, tableName, { - autoCreateLayers: true, - centerMap: true, - }); + try { + await addTableToMap(mapId, tableName, { + autoCreateLayers: true, + centerMap: true, + }); + } catch (error) { + console.error(`Failed to add table "${tableName}" to map:`, error); + // Optionally: show user-facing error notification + } } else { addLayer(undefined, tableName); } }, [addLayer, addTableToMap, keplerState?.visState.datasets, mapId], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/components/CustomLayerManager.tsx` around lines 266 - 279, The onAddTableLayer callback awaits addTableToMap but lacks error handling; wrap the await call in a try/catch inside onAddTableLayer (referencing onAddTableLayer and the addTableToMap call) and on failure log the error (use console.error or processLogger) and surface a user-friendly notification or fallback (e.g., call a passed-in onError handler or show toast) so the UI remains consistent; ensure the else branch still calls addLayer(tableName) only when addTableToMap succeeds.
38-39: Hardcoded color may not adapt to theme.The color
#2563EBis hardcoded instead of using a theme variable. If the application supports multiple themes, this blue may not provide sufficient contrast in all modes.Consider using a theme variable
.add-layer-button { background-color: ${(props) => props.theme.sidePanelBg || props.theme.panelBackground}; - color: `#2563EB`; + color: ${(props) => props.theme.activeColor || props.theme.linkBtnColor || '#2563EB'};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/components/CustomLayerManager.tsx` around lines 38 - 39, Replace the hardcoded color "#2563EB" in the styled rules inside CustomLayerManager with a theme-driven value—use something like ${(props) => props.theme.primary || props.theme.linkColor || '#2563EB'} instead of the literal to ensure the component uses the active theme; update the CSS rule that currently reads "color: `#2563EB`" to reference props.theme (matching the existing background-color usage) and keep a sensible fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/kepler/src/components/CustomLayerManager.tsx`:
- Around line 266-279: The onAddTableLayer callback awaits addTableToMap but
lacks error handling; wrap the await call in a try/catch inside onAddTableLayer
(referencing onAddTableLayer and the addTableToMap call) and on failure log the
error (use console.error or processLogger) and surface a user-friendly
notification or fallback (e.g., call a passed-in onError handler or show toast)
so the UI remains consistent; ensure the else branch still calls
addLayer(tableName) only when addTableToMap succeeds.
- Around line 38-39: Replace the hardcoded color "#2563EB" in the styled rules
inside CustomLayerManager with a theme-driven value—use something like ${(props)
=> props.theme.primary || props.theme.linkColor || '#2563EB'} instead of the
literal to ensure the component uses the active theme; update the CSS rule that
currently reads "color: `#2563EB`" to reference props.theme (matching the existing
background-color usage) and keep a sensible fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aace18b-c506-4b0f-ad7e-f631107c658c
📒 Files selected for processing (2)
packages/kepler/src/KeplerSlice.tspackages/kepler/src/components/CustomLayerManager.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kepler/src/components/CustomLayerManager.tsx`:
- Around line 146-152: The options array currently built in CustomLayerManager
(useMemo producing options from dbTables.filter(...).map(...)) only includes DB
tables; change it to compute the union of state.db.tables (filtered by schema
=== 'main') and existing dataset IDs from keplerState.visState.datasets so the
dropdown exposes both DB tables and already-loaded datasets (the handler for
adding layers already supports "dataset is already loaded" via addTileSetToMap).
Locate the useMemo that defines options and update its source to merge unique
names/IDs from dbTables and keplerState.visState.datasets, producing TableOption
{label, value} entries; also apply the same union logic to the other similar
options construction referenced near the second handler to ensure both menus
include datasets not present in db.tables.
- Around line 155-157: The quick fix is to await and handle promise rejections
from layer-add calls so the UI only closes on success: make the handler
(handleSelect) async, await the result of onAdd(options[0].value) (and any other
call site that invokes onAddTableLayer/addTableToMap), wrap the await in
try/catch, log or surface the error in the catch, and only proceed to close the
menu/return on successful completion; apply the same await+try/catch change to
the other branch that calls onAddTableLayer/addTableToMap.
- Around line 195-200: The add-layer button is currently removed from keyboard
navigation because tabIndex={-1}; make it keyboard-focusable by removing
tabIndex or setting it to 0 on the Button referenced by buttonRef, and update
the open flow in handleClick to programmatically move focus into the typeahead
input after opening (use buttonRef to detect open and focus the typeahead
component). Ensure the disabled logic (disabled || !options.length) remains
unchanged and only adjust focus behavior so keyboard users can tab to the
Button, activate it, and have the typeahead receive focus.
In `@packages/kepler/src/KeplerSlice.ts`:
- Around line 435-436: The code calls get().kepler.syncKeplerDatasets() before
updateMapConfigs(), causing syncKeplerDatasets() to compute referencedDataIds
from empty visState; swap the order so updateMapConfigs() runs first to hydrate
visState/layers/filters and then call get().kepler.syncKeplerDatasets(); apply
the same swap for the other occurrence around the second call site (the block at
the other occurrence referenced in the comment). Ensure the calls remain
adjacent and preserve any surrounding logic in initialize()/setConfig() flows.
🪄 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: 34dac182-d074-47ec-b6f5-b6d8cf6c8962
📒 Files selected for processing (3)
packages/kepler/src/KeplerSlice.tspackages/kepler/src/components/CustomLayerManager.tsxpackages/mosaic/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/mosaic/package.json
bdjulbic
approved these changes
Apr 14, 2026
bdjulbic
deleted the
532-feat-synckeplerdatasets-should-only-add-tables-to-kepler-which-are-used-in-some-visible-layer
branch
April 14, 2026 10:58
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Performance