Fix 'Cannot have two HTML5 backends at the same time' in DataGridView. #10485 - #10487
kundansable wants to merge 2 commits into
Conversation
…pgadmin-org#10485 Pass an explicit context to DndProvider so react-dnd does not null its shared manager when one root's provider unmounts while another root's grid still uses the HTML5 backend. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pgadmin-org/pgadmin4/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. WalkthroughDataGridView and PgTreeView now use a cached React DnD manager with an HTML5 backend. Regression tests cover shared manager identity and grid and tree behavior across separate React roots. ChangesShared drag-and-drop manager
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The shared manager addresses overlapping grid and tree lifecycles without changing the tree's effective backend scope. No actionable merge-blocking defect was established; merge after normal checks, including the added regression tests. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change shares drag-and-drop state within one browser page. No newly reachable privileged operation is established, but cleanup during interrupted drags and compatibility across dependency versions remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (1 skipped: 1 unsupported.)
✨ 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 |
…iew. pgadmin-org#10485 Passing context={window} only stopped DataGridView's react-dnd 16 from nulling the global manager. react-arborist bundles react-dnd 14, which stores its manager under the same global key with its own reference count, so unmounting a PgTreeView could still drop the manager while a grid's HTML5 backend was set up on window. Create a single manager with dnd-core 16 and pass it explicitly to both DndProvider (manager) and react-arborist (dndManager), bypassing the global singleton entirely. Also drop the dndRootElement prop, whose ref was never attached and so always resolved to window. Extend the tests to cover grids and trees mounted in separate roots. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#10485 (#10487) Share one explicit react-dnd manager between DataGridView and PgTreeView. react-arborist bundles react-dnd 14, which stores its manager under the same global key as react-dnd 16 but keeps a separate reference count, so unmounting one provider could drop the manager while its HTML5 backend was still attached to window.
|
Thanks! Rebased onto master (the only conflicts were the neighbouring dependency bumps in package.json/yarn.lock), squashed and merged as 467a200. |
2 participants
Fixes #10485
Cause
DataGridViewused<DndProvider backend={HTML5Backend}>. With only abackendprop, react-dnd keeps its manager in a global singleton (window[Symbol.for('__REACT_DND_CONTEXT_INSTANCE__')]) guarded by a module-levelrefCount, and nulls it when the count reaches 0.react-arborist(used byPgTreeView) bundles react-dnd 14. It stores its manager under the same global key but keeps a separaterefCount. Dialogs mount in separate React roots, so when a grid and a tree were mounted together, unmounting whichever one created the manager nulled it while the other was still using its HTML5 backend onwindow. The next grid created a second backend, andsetup()threw "Cannot have two HTML5 backends at the same time" fromDataGridRow(react-dnd/react-dnd#3178).Steps to reproduce (on master)
The trigger is a
DataGridViewwith reorderable rows plus a react-arborist tree (PgTreeView) open at the same time, then closing one of them. It is deterministic, not timing-dependent. react-arborist bundles react-dnd 14, which stores its manager under the same global key as react-dnd 16 but keeps a separate reference count. Whichever one drops to zero nulls the shared manager while the other still has the HTML5 backend set up onwindow.The columns grid only registers drag sources when rows can be reordered (
canReorder, which is true for a new table). In the Preferences dialog, the left-hand category tree is aPgTreeView.Order A: tree first
Error: Cannot have two HTML5 backends at the same time., coming from<DataGridRow>.Order B: grid first
Both orders are reproduced as automated tests in
regression/javascript/SchemaView/DataGridViewDnd.spec.js(see the "DataGridView together with PgTreeView" tests). On master they fail with exactly this error. The UI steps above are derived from that reproduction and the code paths. They have not yet been run manually in a browser.After this change, both orders render the grid normally.
Fix
sources/dnd_manager.jsbuilds a single app-wide manager withcreateDragDropManager(HTML5Backend)fromdnd-core16, which is now a direct dependency. It resolves to the version react-dnd 16 already uses, so the only lockfile change is the new specifier.DataGridViewpasses it as<DndProvider manager={...}>, andPgTreeViewpasses it as react-arborist'sdndManager. An explicit manager bypasses the global singleton and its ref counting, so the manager is never torn down and only one backend is ever set up onwindow.PgTreeView'sdndRootElement. Its ref was never attached to an element, so it always resolved towindowanyway.Tests
regression/javascript/SchemaView/DataGridViewDnd.spec.jsuses a reorderable collection so the HTML5 backend is actually set up. It covers:windowPgTreeViewin separate roots, unmounted in either order, then a new grid mountedOn master, 3 of these tests fail with the exact error from the issue. All pass with this change. The full JS suite (976 tests) and the linter pass.
🤖 Generated with Claude Code
Summary by CodeRabbit