chore: Improving click and select actions in PZ certification BED-7356#2523
chore: Improving click and select actions in PZ certification BED-7356#2523LucasParraF wants to merge 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughModified row click selection logic in DataTable component to be conditional on Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx`:
- Around line 204-206: The selection-control predicate is inconsistent: one
branch checks selectedRow === undefined while the state-sync effect uses
truthy/falsy checks, which breaks when IDs are falsy (e.g., 0 or ""). Update the
state-sync effect to use the same explicit undefined check (selectedRow ===
undefined) or change both places to use a single canonical predicate (e.g.,
selectedRow === undefined for uncontrolled) so selection clearing is consistent;
locate the logic around selectedRow, the branch using
table.getState().rowSelection[row.id], and the state-sync effect and make their
checks identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 41149027-c339-4cf8-90f2-3797a37053f0
📒 Files selected for processing (1)
packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx
| // When selectedRow is provided the parent controls selection. | ||
| if (selectedRow === undefined) { | ||
| const isAlreadySelected = table.getState().rowSelection[row.id]; |
There was a problem hiding this comment.
Use one consistent controlled/uncontrolled predicate.
This branch uses selectedRow === undefined, but the state-sync effect uses truthy/falsy checks. That mismatch can behave differently for falsy-but-defined IDs and lead to unexpected selection clearing.
Proposed alignment
- useEffect(() => {
- if (!selectedRow && table.getIsSomeRowsSelected()) {
+ useEffect(() => {
+ if (selectedRow === undefined && table.getIsSomeRowsSelected()) {
table.setRowSelection({});
}
- if (selectedRow && !table.getState().rowSelection[selectedRow]) {
+ if (selectedRow !== undefined && !table.getState().rowSelection[selectedRow]) {
table.setRowSelection({ [selectedRow]: true });
}
}, [selectedRow, table]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx` around
lines 204 - 206, The selection-control predicate is inconsistent: one branch
checks selectedRow === undefined while the state-sync effect uses truthy/falsy
checks, which breaks when IDs are falsy (e.g., 0 or ""). Update the state-sync
effect to use the same explicit undefined check (selectedRow === undefined) or
change both places to use a single canonical predicate (e.g., selectedRow ===
undefined for uncontrolled) so selection clearing is consistent; locate the
logic around selectedRow, the branch using
table.getState().rowSelection[row.id], and the state-sync effect and make their
checks identical.
Description
While working on Certifications Table, the Data Table was managing the state of the selected rows. Now Data Table will manage the state of the of selectedRow if it is undefined adding more flexibility to the DataTable.
Motivation and Context
Resolves BED-7356
How Has This Been Tested?
Tested locally
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit