fix: adjust return order on normalizeSelectAllField function#948
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughnormalizeSelectAllField now computes the "all selected" flag (items?.includes("all")) before checking for empty/missing items and returns early for empty/undefined items; tests were added to cover items as [], null, and undefined with explicit allSelected true/false. ChangesSelect-All Field Normalization Logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| allSelected | ||
| ) => { | ||
| if (!items?.length) | ||
| const isAllSelected = allSelected ?? items?.includes("all"); |
There was a problem hiding this comment.
@tomrndom
No test coverage for the fixed edge case
The exact bug scenario (allSelected=true with empty/null/undefined items) has no test — the three changed paths can regress silently on any future refactor of this utility used across 7+ call sites .
Add tests for normalizeSelectAllField([], "apply_to_all", "items", true), same for null and undefined items — each should assert { apply_to_all: true, items: [] }
| allSelected | ||
| ) => { | ||
| if (!items?.length) | ||
| const isAllSelected = allSelected ?? items?.includes("all"); |
There was a problem hiding this comment.
@tomrndom
Add a test for normalizeSelectAllField([], "flag", "list", false) asserting { flag: false, list: [] } to pin this behavior
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
ref: https://app.clickup.com/t/86b9ydmgt
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests