Fixed an issue where Geometry Viewer was showing stale data and not a…#9644
Open
anilsahoo20 wants to merge 4 commits intopgadmin-org:masterfrom
Open
Fixed an issue where Geometry Viewer was showing stale data and not a…#9644anilsahoo20 wants to merge 4 commits intopgadmin-org:masterfrom
anilsahoo20 wants to merge 4 commits intopgadmin-org:masterfrom
Conversation
WalkthroughAdds PK-based row-identification and memoized selection logic to GeometryViewer; ResultSet gets helpers to compute filtered rows and open/refresh the Geometry Viewer tab when rows or columns change; also tightens ResizeObserver lifecycle and conditional geometry parsing. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ResultSet
participant TabManager
participant GeometryViewer
participant Map
User->>ResultSet: select cell or change rows/columns
ResultSet->>ResultSet: getFilteredRowsForGeometryViewer(rows, cols, selection)
ResultSet->>TabManager: openGeometryViewerTab(filteredData)
TabManager-->>GeometryViewer: supply tab data / ensure tab visible
GeometryViewer->>GeometryViewer: compute currentColumnKey/currentColumnNames and pkColumn
GeometryViewer->>GeometryViewer: derive displayRows (memoized) and parse geometries if column selected
GeometryViewer->>Map: render map with key = `${selectedSRID}-${mapKey}`
Map->>GeometryViewer: attach ResizeObserver
GeometryViewer->>GeometryViewer: cleanup ResizeObserver on unmount or change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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: 3
🧹 Nitpick comments (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (2)
485-490:Array.includesfor PK lookup is O(n) per row — use aSetfor large datasets.Both lines 487 and 504 use
prevState.selectedRowPKs.includes(row.__temp_PK)inside.filter(), making the overall complexity O(n × m). For result sets approaching the 100k geometry limit, this can be sluggish.♻️ Proposed fix using Set
- if (prevState.selectedRowPKs.length > 0 && prevState.selectedRowPKs.length < rows.length) { - const newSelectedPKs = rows - .filter(row => prevState.selectedRowPKs.includes(row.__temp_PK)) - .map(row => row.__temp_PK); + if (prevState.selectedRowPKs.length > 0 && prevState.selectedRowPKs.length < rows.length) { + const prevPKSet = new Set(prevState.selectedRowPKs); + const newSelectedPKs = rows + .filter(row => prevPKSet.has(row.__temp_PK)) + .map(row => row.__temp_PK);Similarly in
displayRows:const displayRows = React.useMemo(() => { if (!currentColumnKey || rows.length === 0) return []; const selectedPKs = prevStateRef.current.selectedRowPKs; - return selectedPKs.length > 0 && selectedPKs.length < rows.length - ? rows.filter(row => selectedPKs.includes(row.__temp_PK)) + if (selectedPKs.length > 0 && selectedPKs.length < rows.length) { + const pkSet = new Set(selectedPKs); + return rows.filter(row => pkSet.has(row.__temp_PK)); + } + return rows; - : rows; }, [rows, currentColumnKey]);Also applies to: 499-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 485 - 490, The code uses prevState.selectedRowPKs.includes(row.__temp_PK) inside rows.filter(...) (see the block updating prevStateRef.current.selectedRowPKs and the similar logic in displayRows), which is O(n*m); change it to build a Set once from prevState.selectedRowPKs (e.g., const prevPKs = new Set(prevState.selectedRowPKs)) and then use prevPKs.has(row.__temp_PK) in the .filter() calls so lookups become O(1); update both the block that assigns prevStateRef.current.selectedRowPKs and the analogous displayRows section to use the Set-based lookup.
441-497: Reading a mutable ref insideuseMemocreates a render-order dependency.
displayRows(line 502) readsprevStateRef.current.selectedRowPKs, but this ref is updated in theuseEffectbelow, which runs after render. On the first render cycle whererowschanges (without a column change),displayRowsis computed with the previous cycle'sselectedRowPKs, and no re-render is triggered to correct it (unlike the column-change branches which callsetMapKey).In practice the fallback (
selectedPKs.length === 0 → return rows) masks this for initial loads. But for subsequent row-set changes where a subset was previously selected, the displayed data during that render frame is based on stale PKs until an unrelated re-render occurs.Consider either:
- Moving the PK computation into
displayRowsitself (pure derivation, no ref), or- Storing
selectedRowPKsin state (viauseState) so updating it triggers a re-render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 441 - 497, The bug is stale selectedRowPKs being read from prevStateRef during render; instead lift selectedRowPKs into React state so updates trigger renders: add a useState (e.g., [selectedRowPKs, setSelectedRowPKs]) and remove selectedRowPKs from prevStateRef, update the useEffect branches that currently write prevStateRef.current.selectedRowPKs to call setSelectedRowPKs (including the column-change branch that resets to []), and update any consumers (like displayRows) to read selectedRowPKs state rather than prevStateRef.current.selectedRowPKs; keep prevStateRef for columnKey/columnNames only and continue calling setMapKey where needed.web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
1501-1520: Auto-update effect:columns.findpicks only the first geometry/geography column.Line 1505 selects the first column matching
'geometry'or'geography'. If a table has multiple geometry columns and the user previously chose a different one via the manual trigger, the auto-refresh will silently switch to the first one.Consider preserving the previously selected geometry column (e.g., by storing its key in a ref when
TRIGGER_RENDER_GEOMETRIESfires) and falling back tocolumns.find(...)only if that column no longer exists in the new result set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx` around lines 1501 - 1520, The auto-update useEffect currently picks the first geometry/geography column via columns.find, which can override a user-selected geometry column; modify this by persisting the user-selected geometry column key (e.g., in a ref like selectedGeometryKeyRef that's set when TRIGGER_RENDER_GEOMETRIES fires) and in the effect prefer columns.find(col => col.key === selectedGeometryKeyRef.current) before falling back to columns.find(col => col.cell === 'geometry' || col.cell === 'geography'); keep using getFilteredRowsForGeometryViewer and openGeometryViewerTab as before, and clear selectedGeometryKeyRef when that key no longer exists in the new columns so the fallback behavior works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Line 540: Fix the typo in the inline comment inside GeometryViewer.jsx: change
"Dyanmic CRS is not supported." to "Dynamic CRS is not supported." so the
comment above the map key logic (in the GeometryViewer component) reads
correctly; no code logic changes needed, just update the comment text near the
line referencing srid and mapKey.
- Around line 486-490: Replace the hardcoded '__temp_PK' property with the
shared GRID_ROW_SELECT_KEY constant: update the filtering and mapping that
builds newSelectedPKs (and any other uses in this snippet that reference
__temp_PK) to use rows[GRID_ROW_SELECT_KEY] (or the bracket notation using the
GRID_ROW_SELECT_KEY symbol) so the logic that computes
prevStateRef.current.selectedRowPKs and compares to prevState.selectedRowPKs
uses the actual row key constant defined in QueryToolDataGrid/ResultSet; apply
this substitution for all five occurrences referenced in the comment.
In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx`:
- Around line 1501-1520: The effect currently checks
layoutDocker.isTabOpen(PANELS.GEOMETRY) which triggers updates even when the
geometry tab is not visible; change that call to
layoutDocker.isTabVisible(PANELS.GEOMETRY) inside the useEffect so the geometry
viewer update logic (using prevRowsRef, prevColumnsRef,
getFilteredRowsForGeometryViewer and openGeometryViewerTab) only runs when the
tab is actively visible; keep the rest of the effect (rows/columns change
detection and the null/empty fallback) unchanged.
---
Nitpick comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 485-490: The code uses
prevState.selectedRowPKs.includes(row.__temp_PK) inside rows.filter(...) (see
the block updating prevStateRef.current.selectedRowPKs and the similar logic in
displayRows), which is O(n*m); change it to build a Set once from
prevState.selectedRowPKs (e.g., const prevPKs = new
Set(prevState.selectedRowPKs)) and then use prevPKs.has(row.__temp_PK) in the
.filter() calls so lookups become O(1); update both the block that assigns
prevStateRef.current.selectedRowPKs and the analogous displayRows section to use
the Set-based lookup.
- Around line 441-497: The bug is stale selectedRowPKs being read from
prevStateRef during render; instead lift selectedRowPKs into React state so
updates trigger renders: add a useState (e.g., [selectedRowPKs,
setSelectedRowPKs]) and remove selectedRowPKs from prevStateRef, update the
useEffect branches that currently write prevStateRef.current.selectedRowPKs to
call setSelectedRowPKs (including the column-change branch that resets to []),
and update any consumers (like displayRows) to read selectedRowPKs state rather
than prevStateRef.current.selectedRowPKs; keep prevStateRef for
columnKey/columnNames only and continue calling setMapKey where needed.
In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx`:
- Around line 1501-1520: The auto-update useEffect currently picks the first
geometry/geography column via columns.find, which can override a user-selected
geometry column; modify this by persisting the user-selected geometry column key
(e.g., in a ref like selectedGeometryKeyRef that's set when
TRIGGER_RENDER_GEOMETRIES fires) and in the effect prefer columns.find(col =>
col.key === selectedGeometryKeyRef.current) before falling back to
columns.find(col => col.cell === 'geometry' || col.cell === 'geography'); keep
using getFilteredRowsForGeometryViewer and openGeometryViewerTab as before, and
clear selectedGeometryKeyRef when that key no longer exists in the new columns
so the fallback behavior works.
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx
Outdated
Show resolved
Hide resolved
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx
Outdated
Show resolved
Hide resolved
…uto-updating on query reruns or new query runs with new data or different geometry columns in Query tool. pgadmin-org#9392
…ed of first column or rowdata
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (2)
487-538:displayRowsuseMemo reads fromprevStateRefwhich is updated by auseEffectthat runs after render — the matched selection is always one render behind.
useMemo(line 529) runs during render and readsprevStateRef.current.selectedRowData. TheuseEffect(line 487) that computes the matched selection runs after render and writes to that same ref — but triggers no state update to force a re-render. This means when onlyrowschanges (same columns),displayRowsuses the previous render'sselectedRowData, and the freshly matched data computed by the effect is never reflected.In practice, since
openGeometryViewerTabinResultSet.jsxlikely creates a newGeometryViewerinstance each time (resetting all refs), this code path may rarely be exercised. But if the layout docker reuses the component instance, the selection-preservation logic will be off by one render.Consider moving the matching logic into the
useMemodirectly (removing the effect for the row-matching branch) so thatdisplayRowsalways reflects the current computation:♻️ Sketch — merge matching into useMemo
- useEffect(() => { - const prevState = prevStateRef.current; - if (!currentColumnKey) { - setMapKey(prev => prev + 1); - ... - return; - } - if (currentColumnKey !== prevState.columnKey || - currentColumnNames !== prevState.columnNames) { - setMapKey(prev => prev + 1); - ... - return; - } - if (currentColumnKey === prevState.columnKey && - currentColumnNames === prevState.columnNames && - rows.length > 0) { - ... - prevStateRef.current.selectedRowData = newSelectedRowData; - } - }, [currentColumnKey, currentColumnNames, rows, pkColumn, columns]); - - const displayRows = React.useMemo(() => { - ... - const selected = prevState.selectedRowData; - return selected.length > 0 && selected.length < rows.length ? selected : rows; - }, [rows, currentColumnKey, currentColumnNames]); + // Keep the effect only for the branches that call setMapKey + useEffect(() => { + if (!currentColumnKey) { + setMapKey(prev => prev + 1); + prevStateRef.current = { columnKey: null, columnNames: null, selectedRowData: [] }; + return; + } + if (currentColumnKey !== prevStateRef.current.columnKey || + currentColumnNames !== prevStateRef.current.columnNames) { + setMapKey(prev => prev + 1); + prevStateRef.current = { columnKey: currentColumnKey, columnNames: currentColumnNames, selectedRowData: [] }; + } + }, [currentColumnKey, currentColumnNames]); + + // Compute displayRows synchronously during render + const displayRows = React.useMemo(() => { + if (!currentColumnKey || rows.length === 0) return []; + const prevState = prevStateRef.current; + if (currentColumnKey !== prevState.columnKey || currentColumnNames !== prevState.columnNames) { + return rows; + } + let result = rows; + if (prevState.selectedRowData.length > 0 && prevState.selectedRowData.length < rows.length) { + const matched = matchRowSelection(prevState.selectedRowData, rows, pkColumn, columns); + result = matched.length > 0 ? matched : rows; + } + prevStateRef.current.selectedRowData = result; + return result; + }, [rows, columns, currentColumnKey, currentColumnNames, pkColumn]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 487 - 538, The effect-based matching is producing stale results because useEffect runs after render; move the row-matching logic into the displayRows useMemo so it computes matched selection synchronously during render: in other words, remove the branch of the useEffect that computes matched selection and instead, inside the displayRows React.useMemo, call matchRowSelection(prevStateRef.current.selectedRowData, rows, pkColumn, columns) when currentColumnKey/currentColumnNames match and rows changed, then return the matched result (or rows fallback) and also update prevStateRef.current.selectedRowData with that computed selection so subsequent renders use the up-to-date value; keep the effect only for resetting mapKey/prevState when columnKey/columnNames change or become null.
52-52: Hardcoded PK heuristic won't match many real-world primary keys.
PK_COLUMN_NAMES = ['id', 'oid', 'ctid']is a narrow heuristic. Columns likegid(common in PostGIS tables),pk, UUIDs, or composite PKs will miss, falling back tocolumns[0]?.keyorJSON.stringify(row). Since this is only used for best-effort selection preservation (not data integrity), the fallback is safe — but worth a comment noting the limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` at line 52, PK_COLUMN_NAMES is a too-narrow heuristic (only ['id','oid','ctid']) and will miss common PK names like gid, pk, UUID columns and composite keys; update the heuristic in GeometryViewer.jsx by (1) extending PK_COLUMN_NAMES to include 'gid' and 'pk', (2) adding a simple UUID-name/pattern check (e.g., detect column names containing 'uuid' or matching a UUID-like regex) when choosing a primary-key-like column, and (3) leave the existing safe fallbacks (columns[0]?.key and JSON.stringify(row)) intact; also add a brief inline comment near PK_COLUMN_NAMES and the selection-preservation logic explaining that this is a best-effort heuristic and may not catch composite or custom PKs.web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
1501-1520: Auto-update effect: dependency list is broader than necessary, causing redundant runs.The effect's purpose is to react to
rows/columnschanges when the geometry tab is open. However, includinggetFilteredRowsForGeometryViewer(which changes whenselectedRows/selectedColumnschange) andopenGeometryViewerTabin the dependency array causes this effect to re-run on every selection change — even though therowsChanged || columnsChangedguard prevents a tab update. These redundant executions are harmless but wasteful.Consider extracting the current values of the callbacks via refs (similar to
useLatestFuncalready in the codebase) so the dependency array only contains[rows, columns, layoutDocker].Also,
isTabOpenis intentionally used instead ofisTabVisibleto avoid unwanted focus on the Geometry Viewer tab when opening Query Tool. Based on learnings, this is the correct choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx` around lines 1501 - 1520, Extract the callbacks into stable refs using the existing useLatestFunc (or implement refs) so the effect uses the latest getFilteredRowsForGeometryViewer and openGeometryViewerTab via those refs instead of listing them as dependencies; then change the useEffect dependency array to only [rows, columns, layoutDocker] while keeping the same internal logic that compares prevRowsRef/current prevColumnsRef, checks layoutDocker.isTabOpen(PANELS.GEOMETRY), finds currentGeometryColumn, and calls the ref-wrapped getFilteredRowsForGeometryViewer() and openGeometryViewerTab() (or passes null/[] when no geometry column).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 487-538: The effect-based matching is producing stale results
because useEffect runs after render; move the row-matching logic into the
displayRows useMemo so it computes matched selection synchronously during
render: in other words, remove the branch of the useEffect that computes matched
selection and instead, inside the displayRows React.useMemo, call
matchRowSelection(prevStateRef.current.selectedRowData, rows, pkColumn, columns)
when currentColumnKey/currentColumnNames match and rows changed, then return the
matched result (or rows fallback) and also update
prevStateRef.current.selectedRowData with that computed selection so subsequent
renders use the up-to-date value; keep the effect only for resetting
mapKey/prevState when columnKey/columnNames change or become null.
- Line 52: PK_COLUMN_NAMES is a too-narrow heuristic (only ['id','oid','ctid'])
and will miss common PK names like gid, pk, UUID columns and composite keys;
update the heuristic in GeometryViewer.jsx by (1) extending PK_COLUMN_NAMES to
include 'gid' and 'pk', (2) adding a simple UUID-name/pattern check (e.g.,
detect column names containing 'uuid' or matching a UUID-like regex) when
choosing a primary-key-like column, and (3) leave the existing safe fallbacks
(columns[0]?.key and JSON.stringify(row)) intact; also add a brief inline
comment near PK_COLUMN_NAMES and the selection-preservation logic explaining
that this is a best-effort heuristic and may not catch composite or custom PKs.
In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx`:
- Around line 1501-1520: Extract the callbacks into stable refs using the
existing useLatestFunc (or implement refs) so the effect uses the latest
getFilteredRowsForGeometryViewer and openGeometryViewerTab via those refs
instead of listing them as dependencies; then change the useEffect dependency
array to only [rows, columns, layoutDocker] while keeping the same internal
logic that compares prevRowsRef/current prevColumnsRef, checks
layoutDocker.isTabOpen(PANELS.GEOMETRY), finds currentGeometryColumn, and calls
the ref-wrapped getFilteredRowsForGeometryViewer() and openGeometryViewerTab()
(or passes null/[] when no geometry column).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (1)
196-221: Row identifier heuristics may match wrong rows in some scenarios.A few concerns with the row identification approach:
PK_COLUMN_NAMESonly covers'id'and'oid'with exact case match. Common geometry query columns likegid,pk, orID(uppercase) won't be detected.- The first-column fallback in
getRowIdentifier(line 206-207) is not guaranteed unique — it could cause false-positive matches when two different rows share the same first-column value but differ in other columns.JSON.stringify(row)is property-order-dependent and expensive for large rows.Since this is best-effort for auto-update UX, these may be acceptable, but consider at minimum a case-insensitive PK name match:
♻️ Suggested improvement
-const PK_COLUMN_NAMES = ['id', 'oid']; +const PK_COLUMN_NAMES = ['id', 'oid', 'gid', 'pk']; function findPkColumn(columns) { - return columns.find(c => PK_COLUMN_NAMES.includes(c.name)); + return columns.find(c => PK_COLUMN_NAMES.includes(c.name?.toLowerCase())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 196 - 221, Update the PK detection and fallback used by findPkColumn/getRowIdentifier: make PK_COLUMN_NAMES matching case-insensitive and include common alternatives like "gid" and "pk" by normalizing column.name to lowerCase before checking (refer to PK_COLUMN_NAMES and function findPkColumn), and replace the fragile first-column/JSON.stringify fallback in getRowIdentifier with a deterministic composite identifier built from the ordered columns array (e.g., join of column keys and their values) so identifiers are stable and less error-prone; ensure createIdentifierSet and matchRowSelection continue to call getRowIdentifier so the new logic is used everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 486-551: The bug is caused by splitting selection logic between
the useEffect and the displayRows useMemo, causing displayRows to read stale
prevStateRef.current.selectedRowData during render; consolidate the
row-selection computation into the displayRows React.useMemo so it
deterministically returns the correct rows during render (use
prevStateRef.current, rows, pkColumn, columns and call matchRowSelection there),
and keep the useEffect simplified to only detect column/columns changes and bump
setMapKey or to sync prevStateRef.current (assign the computed selection into
prevStateRef.current.selectedRowData) after render; ensure you still increment
setMapKey when currentColumnKey or currentColumnNames change and that
parseData(data) continues to consume displayRows.
---
Nitpick comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 196-221: Update the PK detection and fallback used by
findPkColumn/getRowIdentifier: make PK_COLUMN_NAMES matching case-insensitive
and include common alternatives like "gid" and "pk" by normalizing column.name
to lowerCase before checking (refer to PK_COLUMN_NAMES and function
findPkColumn), and replace the fragile first-column/JSON.stringify fallback in
getRowIdentifier with a deterministic composite identifier built from the
ordered columns array (e.g., join of column keys and their values) so
identifiers are stable and less error-prone; ensure createIdentifierSet and
matchRowSelection continue to call getRowIdentifier so the new logic is used
everywhere.
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx (3)
477-484: Minor:currentColumnKeymemo provides no memoization benefit.
column?.keyis a simple property access that always returns the same value for the samecolumnreference. TheuseMemoadds overhead without caching benefit since it depends on[column]— whenevercolumnchanges, the memo recomputes anyway. A plainconstwould be equivalent and simpler:const currentColumnKey = column?.key;That said, this is cosmetic — no functional impact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 477 - 484, Replace the unnecessary useMemo wrapper around currentColumnKey: instead of computing currentColumnKey via useMemo(() => column?.key, [column]) simply assign the value directly (const currentColumnKey = column?.key) in the GeometryViewer component; this removes the extra hook overhead while keeping the same semantics and still updates when the column variable changes.
52-52: Row identification heuristic is fragile and can silently match wrong rows.
PK_COLUMN_NAMESonly covers'id'and'oid'. Most tables use names likeuser_id,gid,pk, etc., sofindPkColumnwill returnundefinedfor those. The fallback ingetRowIdentifier(first column value) is not guaranteed to be unique — if two rows share the same first-column value,matchRowSelectionwill match both, showing incorrect geometry.Additionally, the
JSON.stringifylast-resort fallback (line 207) is expensive for rows containing large hex geometry strings and produces unstable keys if property enumeration order differs across rows.Consider either:
- Accepting the PK column key from the caller (ResultSet already knows the actual row key via
GRID_ROW_SELECT_KEY/clientPK), or- At minimum, using a composite key from all column values rather than just the first column as the non-PK fallback, to reduce false matches.
#!/bin/bash # Check how ResultSet passes data to GeometryViewer and whether it already has PK info rg -n -B5 -A10 "GeometryViewer" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -80 echo "" echo "=== Check GRID_ROW_SELECT_KEY and clientPK in ResultSet ===" rg -n "GRID_ROW_SELECT_KEY\|clientPK" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -30Also applies to: 196-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` at line 52, The current row identification logic in GeometryViewer (findPkColumn, getRowIdentifier, matchRowSelection) is fragile: update it to prefer a caller-supplied primary-key when available (use GRID_ROW_SELECT_KEY/clientPK coming from ResultSet) and only fall back to computed keys if none provided; if you must compute a fallback, build a stable composite key from all column values in predictable column order (not just the first column) instead of JSON.stringify or single-column values, and avoid expensive serialization of large geometry blobs by excluding geometry columns when composing the key; locate and modify functions findPkColumn, getRowIdentifier, and matchRowSelection in GeometryViewer.jsx and ensure ResultSet passes its GRID_ROW_SELECT_KEY/clientPK through props when rendering GeometryViewer.
486-516:displayRowsused in effect but not listed in its dependency array.Line 514 assigns
prevStateRef.current.selectedRowData = displayRows, butdisplayRowsis absent from the dependency array on line 516. WhiledisplayRowsis fully derived from the listed dependencies today (making this functionally safe), adding it to the dependency array improves clarity and prevents fragility ifdisplayRows's computation changes in the future.Add
displayRowsto the dependency array:Suggested fix
- }, [currentColumnKey, currentColumnNames, rows, pkColumn, columns]); + }, [currentColumnKey, currentColumnNames, rows, pkColumn, columns, displayRows]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx` around lines 486 - 516, The effect in useEffect (which updates prevStateRef.current.selectedRowData = displayRows) uses displayRows but does not list it in the dependency array; update the dependency array for the useEffect that references currentColumnKey/currentColumnNames/rows/pkColumn/columns to also include displayRows so the effect re-runs whenever displayRows changes, ensuring prevStateRef and mapKey logic (setMapKey, prevStateRef.current updates) remain consistent; locate the useEffect and add displayRows to its dependency array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx`:
- Around line 477-484: Replace the unnecessary useMemo wrapper around
currentColumnKey: instead of computing currentColumnKey via useMemo(() =>
column?.key, [column]) simply assign the value directly (const currentColumnKey
= column?.key) in the GeometryViewer component; this removes the extra hook
overhead while keeping the same semantics and still updates when the column
variable changes.
- Line 52: The current row identification logic in GeometryViewer (findPkColumn,
getRowIdentifier, matchRowSelection) is fragile: update it to prefer a
caller-supplied primary-key when available (use GRID_ROW_SELECT_KEY/clientPK
coming from ResultSet) and only fall back to computed keys if none provided; if
you must compute a fallback, build a stable composite key from all column values
in predictable column order (not just the first column) instead of
JSON.stringify or single-column values, and avoid expensive serialization of
large geometry blobs by excluding geometry columns when composing the key;
locate and modify functions findPkColumn, getRowIdentifier, and
matchRowSelection in GeometryViewer.jsx and ensure ResultSet passes its
GRID_ROW_SELECT_KEY/clientPK through props when rendering GeometryViewer.
- Around line 486-516: The effect in useEffect (which updates
prevStateRef.current.selectedRowData = displayRows) uses displayRows but does
not list it in the dependency array; update the dependency array for the
useEffect that references
currentColumnKey/currentColumnNames/rows/pkColumn/columns to also include
displayRows so the effect re-runs whenever displayRows changes, ensuring
prevStateRef and mapKey logic (setMapKey, prevStateRef.current updates) remain
consistent; locate the useEffect and add displayRows to its dependency array.
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.
…uto-updating on query reruns or new query runs with new data or different geometry columns in Query tool. #9392
Summary by CodeRabbit
New Features
Improvements