Skip to content

fix: adjust return order on normalizeSelectAllField function#948

Merged
smarcet merged 2 commits into
masterfrom
fix/normalize-select-all-order
May 20, 2026
Merged

fix: adjust return order on normalizeSelectAllField function#948
smarcet merged 2 commits into
masterfrom
fix/normalize-select-all-order

Conversation

@tomrndom
Copy link
Copy Markdown

@tomrndom tomrndom commented May 18, 2026

ref: https://app.clickup.com/t/86b9ydmgt

Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com

Summary by CodeRabbit

  • Bug Fixes

    • Select-all behavior now respects an explicit "all selected" flag even when items are empty, null, or undefined, avoiding incorrect deactivation and ensuring consistent selection state.
  • Tests

    • Added parameterized tests covering empty/null/undefined item lists to validate the normalized select-all behavior.

Review Change Stack

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce27d824-a055-4c68-9496-70d5255a60e1

📥 Commits

Reviewing files that changed from the base of the PR and between c1a17e4 and 06cf653.

📒 Files selected for processing (1)
  • src/utils/__tests__/methods.test.js

📝 Walkthrough

Walkthrough

normalizeSelectAllField 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.

Changes

Select-All Field Normalization Logic

Layer / File(s) Summary
normalizeSelectAllField implementation and tests
src/utils/methods.js, src/utils/__tests__/methods.test.js
normalizeSelectAllField computes isAllSelected using items?.includes("all") before the empty-items guard and returns early for empty/undefined items. Tests added to assert apply_to_all when items is [], null, or undefined with allSelected true/false.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • smarcet

Poem

🐰 I checked for "all" before the gate,
Even empty baskets couldn't wait.
Tests hop in, confident and small,
Now select-all answers every call. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: adjust return order on normalizeSelectAllField function' directly matches the main change described in the raw summary, which focuses on adjusting the return order of the normalizeSelectAllField function to compute isAllSelected upfront and reorder the early return logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/normalize-select-all-order

Comment @coderabbitai help to get the list of available commands and usage tips.

@tomrndom tomrndom requested a review from smarcet May 19, 2026 03:41
Comment thread src/utils/methods.js
allSelected
) => {
if (!items?.length)
const isAllSelected = allSelected ?? items?.includes("all");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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: [] }

Comment thread src/utils/methods.js
allSelected
) => {
if (!items?.length)
const isAllSelected = allSelected ?? items?.includes("all");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom
Add a test for normalizeSelectAllField([], "flag", "list", false) asserting { flag: false, list: [] } to pin this behavior

Copy link
Copy Markdown

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom please review

Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Copy link
Copy Markdown

@smarcet smarcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smarcet smarcet merged commit 4d7ab5e into master May 20, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants