Repository navigation
feat: add more map export resolution options - #469
Conversation
|
Warning Rate limit exceeded
⌛ 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 (1)
📝 WalkthroughWalkthroughReplaced legacy ratio UI with a custom internal resolution set ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Co-authored-by: Copilot <[email protected]> Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Signed-off-by: bdjulbic <[email protected]>
Co-authored-by: Cursor Agent <[email protected]> Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]> Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]> Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: bdjulbic <[email protected]>
Signed-off-by: bdjulbic <[email protected]>
…ort-resolution-options # Conflicts: # apps/sqlrooms-cli-ui/src/components/DataSourcesPanel.tsx # apps/sqlrooms-cli-ui/src/serverApi.ts # packages/sql-editor/src/SqlMonacoEditor.tsx # packages/sql-editor/src/components/QueryEditorPanelEditor.tsx # pnpm-lock.yaml
…ion-options' into feat/add-more-map-export-resolution-options # Conflicts: # packages/kepler/src/components/KeplerImageExport.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/KeplerImageExport.tsx`:
- Around line 76-77: The Select is bound to resolution but some options are
filtered later, so KeplerImageExport can render a resolution value that no
longer exists; add normalization logic inside the KeplerImageExport component to
ensure the current resolution is a member of the computed filtered options (the
array used in the filter around the existing options code) and if not, update
state to a fallback (e.g., first filtered option or a safe default) via the
resolution state setter (e.g., setResolution) or a useEffect that runs when
filteredOptions or resolution change; ensure ExportResolutionOption values are
compared by the same key used in the options array so the Select never receives
an out-of-list value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 956a9acf-73d8-41de-87aa-386553dea999
📒 Files selected for processing (1)
packages/kepler/src/components/KeplerImageExport.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kepler/src/components/KeplerImageExport.tsx (1)
59-64: Fix typo and consider adding missing dependency.Line 60 has a typo: "incomong" → "incoming".
Additionally,
setExportImageSettingis used inside the effect but not listed in the dependency array. If it's a stable function reference (e.g., from Zustand), adding it to deps won't cause re-runs and satisfies exhaustive-deps lint rules.✏️ Proposed fix
useEffect(() => { - // hardcode default resolution, because the incomong resolution is ONE_X and we don't want to show that one + // hardcode default resolution, because the incoming resolution is ONE_X and we don't want to show that one setExportImageSetting({ resolution: Resolution1280x720Option.id, }); - }, []); + }, [setExportImageSetting]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/components/KeplerImageExport.tsx` around lines 59 - 64, Fix the inline comment typo and add the missing dependency: correct "incomong" to "incoming" in the comment inside the useEffect, and include setExportImageSetting in the dependency array of that useEffect (the effect that sets resolution to Resolution1280x720Option.id) so the hook satisfies exhaustive-deps while preserving behavior if setExportImageSetting is a stable reference.
🤖 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/KeplerImageExport.tsx`:
- Around line 59-64: Fix the inline comment typo and add the missing dependency:
correct "incomong" to "incoming" in the comment inside the useEffect, and
include setExportImageSetting in the dependency array of that useEffect (the
effect that sets resolution to Resolution1280x720Option.id) so the hook
satisfies exhaustive-deps while preserving behavior if setExportImageSetting is
a stable reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc12f8c6-0c10-4c33-8ee1-c95058d2dee5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/data-table/src/QueryDataTableActionsMenu.tsxpackages/kepler/package.jsonpackages/kepler/src/components/KeplerImageExport.tsxpackages/notebook/src/cellOperations/AddNewCellDropdown.tsx
💤 Files with no reviewable changes (1)
- packages/data-table/src/QueryDataTableActionsMenu.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/notebook/src/cellOperations/AddNewCellDropdown.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/KeplerImageExport.tsx`:
- Around line 59-64: The useEffect in KeplerImageExport unconditionally sets
resolution to Resolution1280x720Option.id on mount, which can overwrite a valid
incoming/persisted resolution or pick an unavailable option; change it to first
read the current resolution from the component state/props (the value used by
setExportImageSetting / exportImageSetting) and the list of available options
(e.g., exportResolutionOptions or similar), then only call setExportImageSetting
to set a default when the current resolution is missing or not present in the
available options; when setting a default, prefer Resolution1280x720Option.id
only if it exists in the available options, otherwise fall back to the first
available option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b90ba99-f9b7-4c71-bb3c-c088f5505106
📒 Files selected for processing (1)
packages/kepler/src/components/KeplerImageExport.tsx
There was a problem hiding this comment.
Pull request overview
This PR enhances the Kepler map image export UI by expanding/curating resolution options and improving the export/preview UX while updating Kepler-related dependencies to support the new behavior.
Changes:
- Update
KeplerImageExportto use a custom curated set of export resolutions, disable controls during processing, and add a processing indicator. - Bump
@kepler.gl/*dependencies from3.2.4to3.2.6(and refresh lockfile accordingly). - Minor UI/style cleanups (Tailwind class ordering, whitespace).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Updates lockfile for Kepler and related dependency bumps. |
packages/notebook/src/cellOperations/AddNewCellDropdown.tsx |
Minor Tailwind class reordering for consistent formatting. |
packages/kepler/src/components/KeplerImageExport.tsx |
Implements new resolution selection UX + processing/disable behavior for export. |
packages/kepler/package.json |
Bumps @kepler.gl/* dependencies to 3.2.6. |
packages/data-table/src/QueryDataTableActionsMenu.tsx |
Removes trailing whitespace line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // hardcode default resolution, because the incomong resolution is ONE_X and we don't want to show that one | ||
| setExportImageSetting({ | ||
| resolution: Resolution1280x720Option.id, | ||
| }); | ||
| }, []); | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kepler/src/components/KeplerImageExport.tsx (1)
134-134: Consider simplifying the disabled condition.Since
isPreviewReadyis defined as!processing && imageDataUri, the conditionprocessing || !isPreviewReadysimplifies to!isPreviewReady. The explicitprocessingcheck is redundant.♻️ Suggested simplification
<Button variant="default" className="w-full" onClick={handleExportImage} - disabled={processing || !isPreviewReady} + disabled={!isPreviewReady} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kepler/src/components/KeplerImageExport.tsx` at line 134, The disabled prop on the export button is redundant because isPreviewReady is defined as (!processing && imageDataUri), so change the disabled condition in KeplerImageExport from using both processing and isPreviewReady to only use !isPreviewReady; locate the JSX where disabled={processing || !isPreviewReady} and replace it with disabled={!isPreviewReady} to remove the unnecessary processing check.
🤖 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/KeplerImageExport.tsx`:
- Line 134: The disabled prop on the export button is redundant because
isPreviewReady is defined as (!processing && imageDataUri), so change the
disabled condition in KeplerImageExport from using both processing and
isPreviewReady to only use !isPreviewReady; locate the JSX where
disabled={processing || !isPreviewReady} and replace it with
disabled={!isPreviewReady} to remove the unnecessary processing check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc094ebe-dce5-48f0-b67c-47ddea239aa9
📒 Files selected for processing (1)
packages/kepler/src/components/KeplerImageExport.tsx
There was a problem hiding this comment.
Pull request overview
Updates Kepler map image export to provide a simplified, more robust resolution selection UX while bumping Kepler dependencies to a newer patch release.
Changes:
- Reworked map image export UI to a single resolution dropdown and added a visible processing state + disabled controls during processing.
- Added a default/fallback export resolution when the current resolution isn’t among the supported options.
- Updated
@kepler.gl/*dependencies (and lockfile) to3.2.6and applied small UI/formatting cleanups.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile for Kepler patch bump and related transitive dependency updates. |
| packages/kepler/package.json | Bumps all @kepler.gl/* dependencies from 3.2.4 to 3.2.6. |
| packages/kepler/src/components/KeplerImageExport.tsx | Reworks export UI (resolution-only), adds processing UI/disable states, and enforces a default resolution fallback. |
| packages/notebook/src/cellOperations/AddNewCellDropdown.tsx | Tailwind class order tweak (no functional change). |
| packages/data-table/src/QueryDataTableActionsMenu.tsx | Removes trailing whitespace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Signed-off-by: bdjulbic <[email protected]> Signed-off-by: Ilya Boyandin <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Co-authored-by: Ilya Boyandin <[email protected]> Co-authored-by: Cursor Agent <[email protected]>
Summary by CodeRabbit