feat: add custom tile provider settings for Geometry Viewer#10142
Open
NivGreenstein wants to merge 12 commits into
Open
feat: add custom tile provider settings for Geometry Viewer#10142NivGreenstein wants to merge 12 commits into
NivGreenstein wants to merge 12 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds custom XYZ tile provider support for the Geometry Viewer, wires preference-driven CRS and base-layer selection into rendering, and updates preference form handling for select-file and keyboard-shortcut fields. ChangesCustom Tile Provider Support
Preferences helper normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GeometryViewer
participant usePreferences
participant GeometryViewerUtils
participant MapContainer
participant TheMap
GeometryViewer->>usePreferences: read module preferences
GeometryViewer->>GeometryViewerUtils: getCustomTileProvider(prefs)
GeometryViewerUtils-->>GeometryViewer: provider config or invalid flag
GeometryViewer->>GeometryViewerUtils: getMapCrs(selectedSRID, customTileProvider)
GeometryViewerUtils-->>GeometryViewer: resolved CRS
GeometryViewer->>MapContainer: render with crs and providerKey
GeometryViewer->>TheMap: render with customTileProvider
TheMap->>GeometryViewerUtils: getBaseLayers(customTileProvider)
GeometryViewerUtils-->>TheMap: base layer list
TheMap->>TheMap: show warning when provider is invalid
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (2)
400-407: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnscoped
usePreferences()subscription causes broad re-renders.Calling
usePreferences()without a selector subscribes to the whole preferences store, soGeometryViewerre-renders on any preference change app-wide, not just sqleditor/geometry-viewer ones. Since the only value used isprefStore.version(as a memo dependency) plus a one-offgetPreferencesForModulecall, select justversion.♻️ Suggested fix
- const prefStore = usePreferences(); + const prefStoreVersion = usePreferences((state) => state.version); const customTileProvider = useMemo(() => { - return getCustomTileProvider(prefStore.getPreferencesForModule('sqleditor')); - }, [prefStore.version]); + return getCustomTileProvider(usePreferences.getState().getPreferencesForModule('sqleditor')); + }, [prefStoreVersion]);🤖 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 `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 400 - 407, The `GeometryViewer` component is subscribing to the entire preferences store via `usePreferences()`, which causes unnecessary re-renders on unrelated preference updates. Update the `GeometryViewer` hook usage to select only the `version` field from `usePreferences`, and keep the `getCustomTileProvider` lookup driven by that selected value so the memo still refreshes when preferences change.
369-378: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPotential duplicate React key if custom tile name matches a built-in layer name.
key={layer.name}can collide when a user sets Custom tile provider name to "Street", "Topography", "Gray Style", "Light Color", or "Dark Matter" (the field is free text, default is only "Custom"). Duplicate keys can cause React reconciliation to misbehave for the LayersControl entries. Consider a positionally-unique key, e.g.`${idx}-${layer.name}`.🤖 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 `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 369 - 378, The LayersControl base-layer list in GeometryViewer.jsx uses key={layer.name}, which can collide when a custom tile provider name matches one of the built-in layer names. Update the map over getBaseLayers(customTileProvider) to use a positionally unique key (for example, include the index with the layer name) so each LayersControl.BaseLayer remains uniquely identified even when names duplicate.
🤖 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
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 423-428: The custom tile provider warning is being added to
parsed.infoList in GeometryViewer, which makes it render as a full-panel
EmptyPanelMessage even when geometry exists. Update the GeometryViewer
parsing/render flow so invalid customTileProvider warnings are only shown in
empty-state/fallback cases or inline near the tile controls, and keep
parsed.infoList reserved for actual empty-panel messages.
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewerUtils.js`:
- Around line 22-40: The custom tile provider name in getCustomTileProvider is
returned unsanitized and later rendered by L.Control.Layers via innerHTML, which
can allow injected markup. Update the name handling in
GeometryViewerUtils.getCustomTileProvider to sanitize or escape
prefs.custom_tile_name the same way attribution is handled before returning the
provider object, while keeping the fallback gettext('Custom') intact.
---
Nitpick comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 400-407: The `GeometryViewer` component is subscribing to the
entire preferences store via `usePreferences()`, which causes unnecessary
re-renders on unrelated preference updates. Update the `GeometryViewer` hook
usage to select only the `version` field from `usePreferences`, and keep the
`getCustomTileProvider` lookup driven by that selected value so the memo still
refreshes when preferences change.
- Around line 369-378: The LayersControl base-layer list in GeometryViewer.jsx
uses key={layer.name}, which can collide when a custom tile provider name
matches one of the built-in layer names. Update the map over
getBaseLayers(customTileProvider) to use a positionally unique key (for example,
include the index with the layer name) so each LayersControl.BaseLayer remains
uniquely identified even when names duplicate.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 346ffa83-31fa-4fe6-93ec-bad82d724884
📒 Files selected for processing (7)
docs/en_US/editgrid.rstdocs/en_US/preferences.rstweb/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsxweb/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewerUtils.jsweb/pgadmin/tools/sqleditor/utils/query_tool_preferences.pyweb/pgadmin/utils/constants.pyweb/regression/javascript/sqleditor/GeometryViewerUtils.spec.js
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)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (1)
374-375: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid provider-controlled React keys.
layer.namecan collide with built-in layer names likeEmptyorStreet, which creates duplicate keys and unstable reconciliation. Use a stable key that includes a non-display field.🤖 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 `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 374 - 375, The BaseLayer list in GeometryViewer.jsx is using provider-controlled layer.name as the React key, which can collide with built-in names and cause unstable reconciliation. Update the map over getBaseLayers(customTileProvider) so the key is derived from a stable identifier that includes a non-display field from the layer object, and keep the visible name prop unchanged on LayersControl.BaseLayer.
🤖 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
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 374-375: The BaseLayer list in GeometryViewer.jsx is using
provider-controlled layer.name as the React key, which can collide with built-in
names and cause unstable reconciliation. Update the map over
getBaseLayers(customTileProvider) so the key is derived from a stable identifier
that includes a non-display field from the layer object, and keep the visible
name prop unchanged on LayersControl.BaseLayer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60345c4a-d32b-400c-8fd1-31dfac5c6342
📒 Files selected for processing (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsxweb/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewerUtils.jsweb/regression/javascript/sqleditor/GeometryViewerUtils.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewerUtils.js
- web/regression/javascript/sqleditor/GeometryViewerUtils.spec.js
NivGreenstein
force-pushed
the
feat/geometry-viewer-settings
branch
from
4e8b1e9 to
5cc2441
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (1)
452-458: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep the observed container stable when remounting the map. The
StyledBoxkey remounts the element thatResizeObserverwatches, but the observer is only created whenqueryToolCtxchanges. After an SRID/provider/column switch, the new container is never observed, so later resizes can stop invalidating the Leaflet map. Move the key toMapContaineror reattach the observer when the ref target changes.🤖 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 `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 452 - 458, The StyledBox key in GeometryViewer.jsx is remounting the element that contentRef and ResizeObserver depend on, so the new container is not re-observed after SRID/provider/column changes. Update the GeometryViewer render so the stable observed container keeps the same ref target, and apply the remount key to MapContainer instead, or add logic in the effect that sets up ResizeObserver to reattach whenever the ref element changes. Ensure the observer lifecycle is tied to the actual container referenced by contentRef and not only to queryToolCtx.
🧹 Nitpick comments (1)
web/pgadmin/preferences/static/js/components/PreferencesHelper.jsx (1)
184-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated dynamic-options loader.
Both branches implement the same cache/API/fallback flow. A small helper would keep refresh and non-refresh selects from drifting.
Refactor sketch
+const getDynamicOptionsLoader = (optionsEndpoint, staticOptions = []) => () => { + if (optionsCache[optionsEndpoint]) { + return Promise.resolve([ + ...optionsCache[optionsEndpoint], + ...staticOptions, + ]); + } + + const api = getApiInstance(); + const optionsUrl = url_for(optionsEndpoint); + return api.get(optionsUrl) + .then((res) => { + const dynamicOptions = res.data?.data?.models; + if (!dynamicOptions) { + return staticOptions; + } + optionsCache[optionsEndpoint] = dynamicOptions; + return [...dynamicOptions, ...staticOptions]; + }) + .catch(() => staticOptions); +};🤖 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 `@web/pgadmin/preferences/static/js/components/PreferencesHelper.jsx` around lines 184 - 260, The dynamic options loading logic is duplicated in both branches that assign element.options in PreferencesHelper.jsx, and both implement the same cache, API fetch, and static fallback flow. Extract that shared behavior into a small helper used by both the refresh and non-refresh paths, preserving the existing optionsCache, getApiInstance, and url_for handling so the two select modes cannot drift apart.
🤖 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 `@web/pgadmin/preferences/static/js/components/PreferencesHelper.jsx`:
- Line 99: Preserve any previously injected control handlers when normalizing
control props in PreferencesHelper’s control prop handling. The assignment to
element.controlProps should merge with existing handlers instead of replacing
them, so the onFocus/onBlur wiring added in the select-refresh dependency path
still survives and depchange can fire. Update the normalization logic in
PreferencesHelper to keep both the existing controlProps and the normalized
control_props data, especially for the dependency-related flow that attaches
handlers later.
- Around line 269-273: The shortcut value fallback in PreferencesHelper should
preserve intentionally empty strings instead of restoring the default. Update
the field assignment in PreferencesHelper.jsx around the getPreferences lookup
so it uses a nullish fallback rather than a truthy one, ensuring a saved empty
shortcut remains cleared while still falling back only when the stored
preference is missing or undefined.
- Around line 391-401: The follow-up actions in the PreferencesHelper flow are
not awaited, so rejections from reloadPgAdmin() or
pgAdmin.Browser.tree.destroy() bypass the existing error handling. Update the
branch that handles reloadNow and the tree.destroy() path to await those async
calls (and keep the pgAdmin.Browser.Events.trigger and modalClose sequencing
after destruction), so any failure is caught by the surrounding catch/notifier
logic.
- Line 112: The JSON.parse call in PreferencesHelper.jsx can throw when a
persisted binary-path preference contains malformed JSON, which blocks the
Preferences UI during schema preparation. Update the PreferencesHelper flow that
builds fieldValues from element.value to safely parse this specific persisted
value, catching parse failures and falling back to a safe default so rendering
can continue. Use the existing fieldValues assignment logic in PreferencesHelper
and keep the guard localized to the preference-parsing path.
---
Outside diff comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 452-458: The StyledBox key in GeometryViewer.jsx is remounting the
element that contentRef and ResizeObserver depend on, so the new container is
not re-observed after SRID/provider/column changes. Update the GeometryViewer
render so the stable observed container keeps the same ref target, and apply the
remount key to MapContainer instead, or add logic in the effect that sets up
ResizeObserver to reattach whenever the ref element changes. Ensure the observer
lifecycle is tied to the actual container referenced by contentRef and not only
to queryToolCtx.
---
Nitpick comments:
In `@web/pgadmin/preferences/static/js/components/PreferencesHelper.jsx`:
- Around line 184-260: The dynamic options loading logic is duplicated in both
branches that assign element.options in PreferencesHelper.jsx, and both
implement the same cache, API fetch, and static fallback flow. Extract that
shared behavior into a small helper used by both the refresh and non-refresh
paths, preserving the existing optionsCache, getApiInstance, and url_for
handling so the two select modes cannot drift apart.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e71c64bd-99de-42af-b4a0-563e2887c76e
📒 Files selected for processing (3)
web/pgadmin/preferences/static/js/components/PreferencesHelper.jsxweb/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsxweb/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
Author
|
Hey @adityatoshniwal! |
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
1 participant
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
Testing
Summary by CodeRabbit