Fix ERD delete crash on a stale foreign-key column (#8318)#10042
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes issue ChangesERD Tool Bug Fix and Release Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js (1)
379-379: 💤 Low valueConsider applying optional chaining to other column lookups.
While the fix on line 529 correctly addresses the reported deletion crash, there are several other locations in this file where
_.find(...).attnumor_.find(...).namepatterns are used without optional chaining. These could potentially crash in similar scenarios:
- Line 379:
_.find(tableData.columns, ...).name- Lines 412-413:
_.find(...).attnuminaddLink- Lines 475-476:
_.find(...).nameinaddOneToManyLink- Line 506:
_.find(...).name- Lines 712-715, 718-720:
_.find(...).attnuminaddLinksBetweenNodesConsider adding optional chaining (
?.) to these lookups as well, with appropriate fallback handling when columns are not found (e.g., early return, validation error, or skipping the operation).Also applies to: 412-413, 475-476, 506-506, 712-720
🤖 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/erd/static/js/erd_tool/ERDCore.js` at line 379, Several lookups like fkColumn.referenced = _.find(tableData.columns, (colm)=>colm.attnum==attnum).name and other similar patterns in addLink, addOneToManyLink, and addLinksBetweenNodes can throw when _.find returns undefined; update each of these to use optional chaining (e.g., _.find(...)? .name or ? .attnum) and add a safe fallback/validation: if the find returns undefined then either return early, log/throw a clear validation error, or skip the operation so the code won’t crash; specifically change the fkColumn.referenced assignment and all _.find(...).attnum/.name usages inside addLink, addOneToManyLink, addLinksBetweenNodes to use ?., check the result, and handle the missing-column case consistently.
🤖 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.
Nitpick comments:
In `@web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js`:
- Line 379: Several lookups like fkColumn.referenced = _.find(tableData.columns,
(colm)=>colm.attnum==attnum).name and other similar patterns in addLink,
addOneToManyLink, and addLinksBetweenNodes can throw when _.find returns
undefined; update each of these to use optional chaining (e.g., _.find(...)?
.name or ? .attnum) and add a safe fallback/validation: if the find returns
undefined then either return early, log/throw a clear validation error, or skip
the operation so the code won’t crash; specifically change the
fkColumn.referenced assignment and all _.find(...).attnum/.name usages inside
addLink, addOneToManyLink, addLinksBetweenNodes to use ?., check the result, and
handle the missing-column case consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 668d2817-8250-49d8-ae30-c3d06fda8f01
📒 Files selected for processing (3)
docs/en_US/release_notes.rstdocs/en_US/release_notes_9_16.rstweb/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js
dpage
force-pushed
the
fix-8318-erd-delete-crash
branch
2 times, most recently
from
0842cfe to
16cd6de
Compare
removeOneToManyLink looked up a column by the FK's stored local_column name and read .attnum unconditionally. After a column rename the stored name no longer matches, so _.find returned undefined and .attnum threw, blocking deletion of the table/link. Use optional chaining so a stale FK simply doesn't match the link being removed and deletion proceeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dpage
force-pushed
the
fix-8318-erd-delete-crash
branch
from
16cd6de to
7c8a44a
Compare
There was a problem hiding this comment.
Pull request overview
Fixes an ERD tool crash that occurred when deleting a table or relationship link after a foreign-key column rename left stale local_column metadata, and documents the fix in the v9.16 release notes.
Changes:
- Prevent
removeOneToManyLinkfrom throwing when the FK’slocal_columnno longer matches any current column (optional chaining on the_.find(...).attnumaccess). - Add a v9.16 release note entry referencing Issue #8318.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js | Avoids a delete-time crash when FK column metadata is stale by safely handling missing column lookups. |
| docs/en_US/release_notes_9_16.rst | Adds release note entry for the ERD delete crash fix (Issue #8318). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asheshv
left a comment
asheshv
left a comment
There was a problem hiding this comment.
Crash fix is correct — happy to approve.
Worth filing a follow-up though: the underlying cause is syncFkRefNames only syncing the referenced side, not local_column when a local column is renamed, so the stale row continues to silently disappear from the model on delete rather than be matched and removed properly. There's also a sibling unguarded .name deref at the same function (line 379) that has the same shape.
A regression test exercising the stale-column delete path on ERDCore.removeOneToManyLink would help — there's no coverage for it today.
Summary
Fixes #8318.
Deleting a table or relationship link in the ERD tool could fail with
i.default.find(...) is undefined(aremoveOneToManyLinkstack trace), blocking the delete.Root cause:
removeOneToManyLink(ERDCore.js:529) did_.find(tableNode.getColumns(), (col)=>col.name==theFk.local_column).attnum. If a column was renamed, the FK's storedlocal_columnno longer matches any current column, so_.findreturnsundefinedand.attnumthrows — and the link removal code below it never runs.Fix: use optional chaining (
?.attnum). A stale FK then simply doesn't match the link'sattnum, and the link is removed as expected.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation