Fixed an issue where Geometry Viewer was showing stale data and not a…#9644
Open
anilsahoo20 wants to merge 2 commits intopgadmin-org:masterfrom
Open
Fixed an issue where Geometry Viewer was showing stale data and not a…#9644anilsahoo20 wants to merge 2 commits intopgadmin-org:masterfrom
anilsahoo20 wants to merge 2 commits intopgadmin-org:masterfrom
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip You can get early access to new features in CodeRabbit.Enable the WalkthroughThe changes enhance the Geometry Viewer and Result Set components with stateful change detection, memoization, and auto-refresh logic. Introduces previous-state tracking to detect row and column changes, adds helper functions for filtering geometry data, and improves lifecycle management with ResizeObserver handling and effect-based dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Comment on lines
486
to
490
| const newSelectedPKs = rows | ||
| .filter(row => prevState.selectedRowPKs.includes(row.__temp_PK)) | ||
| .map(row => row.__temp_PK); | ||
|
|
||
| prevStateRef.current.selectedRowPKs = newSelectedPKs.length > 0 ? newSelectedPKs : rows.map(r => r.__temp_PK); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== GRID_ROW_SELECT_KEY definition ==="
rg -n 'GRID_ROW_SELECT_KEY' --type=js --type=jsx --type=ts --type=tsx -g '!node_modules/**' | head -50
echo ""
echo "=== Hardcoded __temp_PK usage ==="
rg -n '__temp_PK' --type=js --type=jsx --type=ts --type=tsx -g '!node_modules/**' | head -100Repository: pgadmin-org/pgadmin4
Length of output: 210
🏁 Script executed:
#!/bin/bash
echo "=== Searching for GRID_ROW_SELECT_KEY ==="
rg -n 'GRID_ROW_SELECT_KEY' --type js -g '!node_modules/**'
echo ""
echo "=== Searching for __temp_PK ==="
rg -n '__temp_PK' --type js -g '!node_modules/**'
echo ""
echo "=== Check GeometryViewer.jsx directly ==="
cat web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx | wc -l
echo ""
echo "=== Check lines 486-490 in GeometryViewer.jsx ==="
sed -n '480,510p' web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsxRepository: pgadmin-org/pgadmin4
Length of output: 2686
🏁 Script executed:
#!/bin/bash
echo "=== Search for where __temp_PK might be set or defined ==="
rg -n 'temp_PK|__temp_PK' --type js -g '!node_modules/**' | head -50
echo ""
echo "=== Check imports in GeometryViewer.jsx ==="
head -30 web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx
echo ""
echo "=== Search for row transformation/creation logic ==="
rg -n 'rows\s*=' web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx | head -20Repository: pgadmin-org/pgadmin4
Length of output: 2120
🏁 Script executed:
#!/bin/bash
echo "=== Check GeometryViewer component signature and where rows come from ==="
grep -n "function GeometryViewer\|const GeometryViewer\|export.*GeometryViewer" web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx | head -5
echo ""
echo "=== Look for rows prop/parameter in GeometryViewer ==="
sed -n '1,100p' web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx | grep -n "rows\|props"
echo ""
echo "=== Check QueryToolDataGrid to see what key it uses ==="
head -100 web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/index.jsx | grep -n "temp_PK\|GRID_ROW"
echo ""
echo "=== Look for where GRID_ROW_SELECT_KEY is actually used ==="
rg -n -B2 -A2 "GRID_ROW_SELECT_KEY" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -50Repository: pgadmin-org/pgadmin4
Length of output: 983
🏁 Script executed:
#!/bin/bash
echo "=== Find where GeometryViewer is imported/used ==="
rg -n "GeometryViewer" --type js -g '!node_modules/**' | head -20
echo ""
echo "=== Check ResultSet component to see how it passes rows ==="
sed -n '1,50p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | grep -n "rows\|GeometryViewer"
echo ""
echo "=== Search for GeometryViewer usage in ResultSet ==="
rg -n -B5 -A5 "GeometryViewer" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -60Repository: pgadmin-org/pgadmin4
Length of output: 5969
🏁 Script executed:
#!/bin/bash
echo "=== Search for rowKeyGetter definition ==="
rg -n "rowKeyGetter" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -20
echo ""
echo "=== Get context around rowKeyGetter definition ==="
rg -n -B3 -A3 "rowKeyGetter\s*=" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 1405
🏁 Script executed:
#!/bin/bash
echo "=== Find rsu initialization and clientPK setup ==="
rg -n "rsu\s*=" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -10
echo ""
echo "=== Search for where clientPK is set in rsu ==="
rg -n "setClientPK\|clientPK" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -30
echo ""
echo "=== Check QueryToolDataGrid to understand how rows are structured ==="
sed -n '1,100p' web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/index.jsx | grep -n "clientPK\|GRID_ROW_SELECT_KEY"Repository: pgadmin-org/pgadmin4
Length of output: 441
🏁 Script executed:
#!/bin/bash
echo "=== Find ResultSetUtils class definition ==="
rg -n "class ResultSetUtils" --type js -g '!node_modules/**'
echo ""
echo "=== Check if ResultSetUtils is in same file or imported ==="
rg -n "ResultSetUtils" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -5
echo ""
echo "=== Find where ResultSetUtils is defined ==="
find web/pgadmin/tools/sqleditor -name "*.js" -o -name "*.jsx" | xargs grep -l "class ResultSetUtils\|export.*ResultSetUtils" 2>/dev/nullRepository: pgadmin-org/pgadmin4
Length of output: 768
🏁 Script executed:
#!/bin/bash
echo "=== Check ResultSetUtils class and its clientPK initialization ==="
sed -n '43,200p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -100
echo ""
echo "=== Look for setClientPK method in ResultSetUtils ==="
rg -n "setClientPK" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -A 3Repository: pgadmin-org/pgadmin4
Length of output: 3503
🏁 Script executed:
#!/bin/bash
echo "=== Check what rows are actually passed to GeometryViewer ==="
sed -n '1466,1480p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
echo ""
echo "=== Search for any row transformation or __temp_PK assignment in ResultSet ==="
rg -n "__temp_PK" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
echo ""
echo "=== Check QueryToolDataGrid for __temp_PK ==="
rg -n "__temp_PK" web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/ --type jsRepository: pgadmin-org/pgadmin4
Length of output: 988
Replace hardcoded __temp_PK references with the actual row key constant.
GRID_ROW_SELECT_KEY (defined in QueryToolDataGrid and used throughout ResultSet) is the actual property on row objects. The hardcoded __temp_PK references will fail at runtime because this property is never set.
♻️ Proposed fix
+import { GRID_ROW_SELECT_KEY } from '../QueryToolDataGrid';
...
- .filter(row => prevState.selectedRowPKs.includes(row.__temp_PK))
- .map(row => row.__temp_PK);
+ .filter(row => prevState.selectedRowPKs.includes(row[GRID_ROW_SELECT_KEY]))
+ .map(row => row[GRID_ROW_SELECT_KEY]);Apply this substitution at all 5 __temp_PK references (lines 487, 488, 490, 493, 504).
🤖 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 - 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.
web/pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx
Outdated
Show resolved
Hide resolved
Comment on lines
+1501
to
+1520
| // Auto-update Geometry Viewer when rows/columns change | ||
| useEffect(()=>{ | ||
| const rowsChanged = prevRowsRef.current !== rows; | ||
| const columnsChanged = prevColumnsRef.current !== columns; | ||
| const currentGeometryColumn = columns.find(col => col.cell === 'geometry' || col.cell === 'geography'); | ||
|
|
||
| if((rowsChanged || columnsChanged) && layoutDocker.isTabOpen(PANELS.GEOMETRY)) { | ||
|
|
||
| if(currentGeometryColumn) { | ||
| const selRowsData = getFilteredRowsForGeometryViewer(); | ||
| openGeometryViewerTab(currentGeometryColumn, selRowsData); | ||
| } else { | ||
| // No geometry column | ||
| openGeometryViewerTab(null, []); | ||
| } | ||
| } | ||
|
|
||
| prevRowsRef.current = rows; | ||
| prevColumnsRef.current = columns; | ||
| }, [rows, columns, getFilteredRowsForGeometryViewer, openGeometryViewerTab, layoutDocker]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check LayoutDocker API for isTabOpen vs isTabVisible
echo "=== isTabOpen definition ==="
rg -n 'isTabOpen' --type=js --type=ts -g '!node_modules/**' -C 3
echo ""
echo "=== isTabVisible definition ==="
rg -n 'isTabVisible' --type=js --type=ts -g '!node_modules/**' -C 3Repository: pgadmin-org/pgadmin4
Length of output: 8893
Use isTabVisible instead of isTabOpen to avoid unnecessary updates when the tab is hidden.
The current code at line 1507 uses layoutDocker.isTabOpen(PANELS.GEOMETRY), which checks if the tab exists but may be behind another tab in the same panel group. The correct method is isTabVisible (line 203-206 in Layout/index.jsx), which checks if the tab is actively visible. This matches the pattern used in GeometryViewer.jsx (line 525), GraphVisualiser.jsx (line 290), and QueryToolComponent.jsx (lines 467, 469), which all use isTabVisible to avoid re-rendering and re-calculating when tabs are open but not actively displayed to the user.
🤖 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 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.
…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
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