ERD Tool: Insert table with relations via drag-and-drop#9581
Merged
akshay-joshi merged 3 commits intopgadmin-org:masterfrom Feb 24, 2026
Merged
ERD Tool: Insert table with relations via drag-and-drop#9581akshay-joshi merged 3 commits intopgadmin-org:masterfrom
akshay-joshi merged 3 commits intopgadmin-org:masterfrom
Conversation
Co-authored-by: Christian P. <pirnichristian@gmail.com>
WalkthroughAdds optional insertion of related tables when dragging a table into the ERD, extends table ERD data with Changes
Sequence DiagramsequenceDiagram
participant User
participant ERDTool as ERDTool.jsx
participant API as Backend API
participant ERDCore
participant Canvas
User->>ERDTool: Drag & drop table
ERDTool->>API: Fetch table ERD data
API-->>ERDTool: Return table data (includes oid + original_foreign_keys)
alt insert_table_with_relations enabled
ERDTool->>ERDCore: addNodeWithLinks(data, position, metadata)
ERDCore->>ERDCore: Create node(s) and collect oid→uid map
ERDCore->>ERDCore: Prune missing refs & resolve FKs
ERDCore->>ERDCore: addLinksBetweenNodes(oidUidMap, newNodesUids)
ERDCore->>Canvas: Render nodes and FK links
else
ERDTool->>ERDCore: addNode(cloneTableData(data), position, metadata)
ERDCore->>Canvas: Render node only
end
ERDTool->>Canvas: Select created node
Canvas-->>User: Display diagram
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx`:
- Around line 599-618: The catch block for the API call in the apiObj.get(...)
chain currently only logs and rethrows, causing silent failures; replace the
console.error/throw in that catch with a call to the component's existing error
handler (e.g. this.handleAxiosCatch(err) or the notifier used elsewhere) to
surface the failure to users and remove the rethrow so the promise is handled;
update the catch for apiObj.get(...) where TableSchema.getErdSupportedData and
addNode/addNodeWithLinks are invoked to call handleAxiosCatch(err) instead of
console.error/throw.
In `@web/pgadmin/tools/erd/static/js/erd_tool/nodes/TableNode.jsx`:
- Around line 161-165: serializeData() is mutating the node's live data by
filtering data.foreign_key in-place which can permanently remove FKs; fix by
cloning the data (or at least the foreign_key array) returned from getData()
before applying the filter so the node's persisted state remains unchanged —
e.g., obtain const data = this.getData(); create a shallow copy (or copy of
data.foreign_key) and run the .filter on that copy, then return the
cloned/modified copy while leaving the original getData() object intact.
🧹 Nitpick comments (1)
web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js (1)
686-724: Guard against missing FK column metadata during link creation.
addLinksBetweenNodesassumesfk.columns[0]and column lookups always resolve. A malformed FK or a column removed in the ERD could throw here and break drag‑and‑drop. A small guard keeps this path resilient.🛡️ Suggested defensive checks
- const theFkColumn = theFk.columns[0]; + const theFkColumn = theFk.columns?.[0]; + if (!theFkColumn) return; let referencesUid = oidUidMap[theFkColumn.references_oid]; @@ - const newData = { - local_table_uid: uid, - local_column_attnum: _.find( - tableNodesDict[uid].getColumns(), - (col) => col.name == theFkColumn.local_column - ).attnum, - referenced_table_uid: referencesUid, - referenced_column_attnum: _.find( - tableNodesDict[referencesUid].getColumns(), - (col) => col.name == theFkColumn.referenced - ).attnum, - }; + const localCol = _.find( + tableNodesDict[uid].getColumns(), + (col) => col.name == theFkColumn.local_column + ); + const refCol = _.find( + tableNodesDict[referencesUid].getColumns(), + (col) => col.name == theFkColumn.referenced + ); + if (!localCol || !refCol) return; + const newData = { + local_table_uid: uid, + local_column_attnum: localCol.attnum, + referenced_table_uid: referencesUid, + referenced_column_attnum: refCol.attnum, + };
Contributor
Author
|
We were unsure how to update the translations, thus skipped that for now |
Contributor
|
Hi @Lancear, I tested the PR and identified the following issues:
|
Contributor
Author
|
1 was intentional so the ERD diagram matches the generated SQL and has no additional foreign keys. I think we can change 1 by separating the original foreign keys from the schema, from the ones drawn in the ER diagram, will check it out. Will do both of these on Thursday when I have more time in the evening. |
Lancear
force-pushed
the
erd-insert-table-with-relations
branch
2 times, most recently
from
ccd894a to
2cace1c
Compare
Contributor
Author
|
Addressed both issues |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js`:
- Around line 702-708: The duplicate-link guard in addNodeWithLinks only checks
newNodesUids and misses existing visual links, so update the check (around the
block using newNodesUids, uid and referencesUid) to also detect and skip if a
link between uid and referencesUid already exists before calling addLink;
specifically, query the current links collection (e.g., this.links /
graph.getLinks() or the structure that stores links) and if any link has source
=== uid and target === referencesUid (or vice‑versa if links are undirected),
return/skip adding; ensure the check uses the same identifier fields that
addLink expects so duplicates are never created.
- Around line 692-726: The loop that processes nodeData.original_foreign_keys
must guard against missing pieces: ensure theFk.columns && theFk.columns[0]
exists, initialize nodeData.foreign_key if falsy, and check the _.find(...)
results before accessing .attnum; if either the local column lookup or the
referenced column lookup returns undefined (or referencesUid is missing) then
skip this FK (do not push to nodeData.foreign_key or call this.addLink).
Concretely, in the block handling theFk use a guard like: if (!theFk.columns ||
!theFk.columns[0]) return; set nodeData.foreign_key = nodeData.foreign_key ||
[]; resolve localCol = _.find(tableNodesDict[uid].getColumns(), ...) and refCol
= _.find(tableNodesDict[referencesUid].getColumns(), ...); if (!localCol ||
!refCol) return; then build newData using localCol.attnum and refCol.attnum,
clone theFk, set newForeignKey.columns[0].references = referencesUid, push to
nodeData.foreign_key and call this.addLink.
- Around line 660-666: The function addNodeWithLinks currently calls delete
nodeData.oid which mutates the caller's object; instead make a shallow clone or
destructure at the start (e.g., const node = { ...nodeData } or const { oid,
...node } = nodeData) and operate on that local copy (delete node.oid or use the
destructured node) so you never modify the original nodeData; update all
subsequent references in addNodeWithLinks to use the local cloned variable
(node) and leave nodeData untouched.
Contributor
|
@Lancear, Please fix the javascript test case failure. |
Lancear
force-pushed
the
erd-insert-table-with-relations
branch
from
9a1426e to
2cace1c
Compare
Co-authored-by: Christian P. <pirnichristian@gmail.com>
Co-authored-by: Christian P. <pirnichristian@gmail.com>
Lancear
force-pushed
the
erd-insert-table-with-relations
branch
from
2cace1c to
08699f1
Compare
Contributor
Author
|
Fixed |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/regression/javascript/erd/erd_core_spec.js (1)
234-273: Consider adding dedicated tests for the newaddNodeWithLinksandaddLinksBetweenNodespublic APIs.The
deserializeDatatest now exercisesaddLinksBetweenNodesindirectly, butaddNodeWithLinkshas no coverage at all. Given these are the key entry points for the new "insert with relations" feature, direct tests would guard against regressions and clarify the expected FK-deduplication, oid-existence, and cross-schema-pruning behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/regression/javascript/erd/erd_core_spec.js` around lines 234 - 273, Add explicit unit tests for the new public APIs addNodeWithLinks and addLinksBetweenNodes: create focused Jest specs that call erdCoreObj.addNodeWithLinks and erdCoreObj.addLinksBetweenNodes directly (instead of only via deserializeData), mock dependencies the same way as the existing test (erdCoreObj.getNewPort, getNewLink, addNode, addLink, erdEngine.getModel().getNodesDict, etc.), and assert the expected behaviors — that addNodeWithLinks deduplicates foreign keys, respects existing OIDs (doesn't recreate nodes for existing ids), and prunes cross-schema relationships, and that addLinksBetweenNodes creates the correct number of links and sets source/target ports appropriately; use spies to verify call counts and that ports/links are created with the intended names/ids to guard these specific corner cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js`:
- Around line 692-725: In addLinksBetweenNodes, guard against three unguarded
dereferences: ensure theFk.columns is an array and has an element before
accessing theFk.columns[0]; when computing local_column_attnum and
referenced_column_attnum, check the result of _.find(...) is not undefined
before accessing .attnum and skip this FK if either column is missing; and
ensure nodeData.foreign_key is initialized to an array (e.g., set
nodeData.foreign_key = nodeData.foreign_key || []) before pushing newForeignKey;
in each early-skip case return or continue the loop so invalid/missing data does
not throw and prevents other links from being created.
---
Nitpick comments:
In `@web/regression/javascript/erd/erd_core_spec.js`:
- Around line 234-273: Add explicit unit tests for the new public APIs
addNodeWithLinks and addLinksBetweenNodes: create focused Jest specs that call
erdCoreObj.addNodeWithLinks and erdCoreObj.addLinksBetweenNodes directly
(instead of only via deserializeData), mock dependencies the same way as the
existing test (erdCoreObj.getNewPort, getNewLink, addNode, addLink,
erdEngine.getModel().getNodesDict, etc.), and assert the expected behaviors —
that addNodeWithLinks deduplicates foreign keys, respects existing OIDs (doesn't
recreate nodes for existing ids), and prunes cross-schema relationships, and
that addLinksBetweenNodes creates the correct number of links and sets
source/target ports appropriately; use spies to verify call counts and that
ports/links are created with the intended names/ids to guard these specific
corner cases.
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
3 participants
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.
Feature: Drag and Drop Tables with Relations in ERD
This feature allows tables to be dragged into the ERD tool along with their foreign key relationships automatically.
Closes #5578
Closes #8198
Key Decisions
Keep original
oidand a copy offoreign_keyin nodesPreserves table identity and allows checking future references. Only the first node with each
oidretains theoid.First instance priority
Only the first insert of a table is checked for incoming references from other nodes, preventing duplicate foreign key links.
File save preserves all original foreign keys
Missing table references are kept when saving, allowing users to add the referenced tables later.
User preference added
New preference "Drag and Drop table with relations" to control this behavior, default acts like before.
Co-authored-by: Christian P. pirnichristian@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests