Fix tree view checkbox selection for backup dialog. #9649#9671
Fix tree view checkbox selection for backup dialog. #9649#9671RohitBhati8269 wants to merge 1 commit intopgadmin-org:masterfrom
Conversation
WalkthroughModified selection logic in PgTreeView component to compute and track complete tree-wide selection state. Updates clicked node and descendants, marks indeterminate states, and passes all checked/indeterminate nodes to callback instead of only directly toggled nodes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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: 2
🤖 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/static/js/PgTreeView/index.jsx`:
- Line 91: Change the concise-arrow forEach callbacks to block-bodied arrows
that explicitly perform their side effects and return nothing; e.g., replace the
single-expression callback in the n.children?.forEach(child =>
updateDescendants(child, val)); call with a block-bodied callback that calls
updateDescendants(child, val) inside braces, do the same for the forEach inside
collectAllCheckedNodes (which currently has a bare return;) and the third
flagged forEach at line 146 so none of the forEach callbacks have implicit
returns; this will satisfy the lint rule and keep intent explicit.
- Around line 126-131: The code mutates the parent-owned node data by setting
n.data.isIndeterminate, causing stale flags; instead stop mutating n.data and
create a wrapper object for each selected node (e.g., push { node: n,
isIndeterminate: state === 'indeterminate' } into allCheckedNodes) so
isIndeterminate is derived from checkedState/newState rather than stored on
n.data; update the selectionChange call-site and the backup-dialog consumer to
read isIndeterminate from the wrapper object (or the wrapper's property) instead
of node.data.isIndeterminate so the flag is always correct and never mutates the
original data.
| if (val) { | ||
| selectedChNodes.push(n); | ||
| } | ||
| n.children?.forEach(child => updateDescendants(child, val)); |
There was a problem hiding this comment.
Fix forEach callbacks to use block bodies — Biome lint errors on lines 91, 133, and 146.
All three arrow functions use concise bodies, causing Biome to flag them as forEach callbacks that return a value (lint/suspicious/useIterableCallbackReturn). collectAllCheckedNodes even has an explicit bare return;, making its void intent clear.
🔧 Proposed fix
- n.children?.forEach(child => updateDescendants(child, val));
+ n.children?.forEach(child => { updateDescendants(child, val); });- n.children?.forEach(child => collectAllCheckedNodes(child));
+ n.children?.forEach(child => { collectAllCheckedNodes(child); });- rootParent.children.forEach(root => collectAllCheckedNodes(root));
+ rootParent.children.forEach(root => { collectAllCheckedNodes(root); });Also applies to: 133-133, 146-146
🧰 Tools
🪛 Biome (2.4.4)
[error] 91-91: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/static/js/PgTreeView/index.jsx` at line 91, Change the
concise-arrow forEach callbacks to block-bodied arrows that explicitly perform
their side effects and return nothing; e.g., replace the single-expression
callback in the n.children?.forEach(child => updateDescendants(child, val));
call with a block-bodied callback that calls updateDescendants(child, val)
inside braces, do the same for the forEach inside collectAllCheckedNodes (which
currently has a bare return;) and the third flagged forEach at line 146 so none
of the forEach callbacks have implicit returns; this will satisfy the lint rule
and keep intent explicit.
| if (state === true || state === 'indeterminate') { | ||
| // Set isIndeterminate flag to differentiate full schema selection | ||
| // from partial selection (only specific tables) in backup dialog | ||
| n.data.isIndeterminate = (state === 'indeterminate'); | ||
| allCheckedNodes.push(n); | ||
| } |
There was a problem hiding this comment.
Mutating n.data directly violates React's immutability principle and leaves stale flags.
n.data is the original object from the data prop array owned by the parent component. Assigning n.data.isIndeterminate mutates the caller's data silently. There is also a concrete stale-state bug: when a node transitions from indeterminate → unchecked, it is skipped by the state === true || state === 'indeterminate' guard, so data.isIndeterminate is never reset to false and permanently remains true on the data object.
The isIndeterminate flag is purely derived state that already exists in checkedState/newState — it doesn't need to be stored on node.data at all. Pass it alongside the node without mutating the data:
♻️ Proposed fix — avoid data mutation
- if (state === true || state === 'indeterminate') {
- // Set isIndeterminate flag to differentiate full schema selection
- // from partial selection (only specific tables) in backup dialog
- n.data.isIndeterminate = (state === 'indeterminate');
- allCheckedNodes.push(n);
- }
+ if (state === true || state === 'indeterminate') {
+ allCheckedNodes.push({ node: n, isIndeterminate: state === 'indeterminate' });
+ }Update the selectionChange call-site (and the backup dialog consumer) to read isIndeterminate from the wrapper object instead of node.data.isIndeterminate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/static/js/PgTreeView/index.jsx` around lines 126 - 131, The code
mutates the parent-owned node data by setting n.data.isIndeterminate, causing
stale flags; instead stop mutating n.data and create a wrapper object for each
selected node (e.g., push { node: n, isIndeterminate: state === 'indeterminate'
} into allCheckedNodes) so isIndeterminate is derived from checkedState/newState
rather than stored on n.data; update the selectionChange call-site and the
backup-dialog consumer to read isIndeterminate from the wrapper object (or the
wrapper's property) instead of node.data.isIndeterminate so the flag is always
correct and never mutates the original data.
1 participant
entire tree, not just the clicked node's subtree
checked (state === true) and indeterminate states
between full schema selection and partial table selection
to collect a complete selection state
Summary by CodeRabbit
Release Notes