Skip to content

Conversation

@akashsonune
Copy link
Member

@akashsonune akashsonune commented Dec 10, 2025

Describe in detail what your merge request does and why. Add relevant
screenshots and reference related issues via Closes #XY or Related to #XY.


Removes a test case titled "should emit criterion with value = label from config" from the filtered-search component spec file. The removed test verified that when selecting a country option with a value-label pair (e.g., { label: 'Switzerland' } without an explicit value property), the emitted criterion would use the label as the value. The test was marked as non-sense in TODO comments, noting that the interface does not allow empty values for options and was scheduled for removal in version 47.

@akashsonune akashsonune requested a review from a team as a code owner December 10, 2025 16:23
@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

A test case was removed from the si-filtered-search.component.spec.ts file. The deleted test, "should emit criterion with value = label from config," previously verified that when selecting a country from options containing value-label pairs, the emitted criterion would use the label (e.g., Switzerland) instead of the underlying value (e.g., CH). No production code was modified, and all other tests and test structure remain unchanged.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: removal of a test case from the filtered-search component spec file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test/remove-irrelevant-test

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e9182a and 1303528.

📒 Files selected for processing (1)
  • projects/element-ng/filtered-search/si-filtered-search.component.spec.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • projects/element-ng/filtered-search/si-filtered-search.component.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

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.

❤️ Share

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

Copy link
Member

@chintankavathia chintankavathia left a comment

Choose a reason for hiding this comment

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

👍

@spike-rabbit
Copy link
Member

@akashsonune somewhere in the actual code, there must also be a check for empty options. I guess you can remove this now. But I don't know where exactly.

@akashsonune akashsonune force-pushed the test/remove-irrelevant-test branch from 7e81554 to d0e39c7 Compare January 5, 2026 16:09
@akashsonune
Copy link
Member Author

akashsonune commented Jan 5, 2026

@akashsonune somewhere in the actual code, there must also be a check for empty options. I guess you can remove this now. But I don't know where exactly.

@spike-rabbit I guess you mean these changes that I just pushed?

@spike-rabbit
Copy link
Member

I think just the ?? optionValue.label part. I thing the rest must be kept.

@akashsonune
Copy link
Member Author

I think just the ?? optionValue.label part. I thing the rest must be kept.

In that case , we dont see the value field populated in the output event when the value is empty, as the above code ultimately assigns undefined to this.criterionValue().value (user can still set empty value through template)

@akashsonune akashsonune force-pushed the test/remove-irrelevant-test branch 3 times, most recently from cdac930 to 5fe0674 Compare January 6, 2026 12:11
@spike-rabbit
Copy link
Member

@akashsonune please squas into the recatoring commit, then everything is g2g

@akashsonune
Copy link
Member Author

akashsonune commented Jan 6, 2026

@akashsonune please squas into the recatoring commit, then everything is g2g

@spike-rabbit What I meant, like in this example, the output
("searchCriteriaChange", {"value":"Ma","criteria":[{"name":"country"}]})
will not emit the value field since the value is empty in this case, not sure if there should be a value field and with empty value in this case.

Copy link
Member

@spike-rabbit spike-rabbit left a comment

Choose a reason for hiding this comment

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

👍

We already have an issue for those kind of empty values. The plan is support them in the future.

@spike-rabbit spike-rabbit force-pushed the test/remove-irrelevant-test branch from 5fe0674 to af09192 Compare February 6, 2026 08:15
BREAKING CHANGE: Empty `value` fields of criterion options
are no longer replaced with the value from the `label`.

Always provide a correct `value`.
@spike-rabbit spike-rabbit force-pushed the test/remove-irrelevant-test branch from af09192 to 58f9ba4 Compare February 6, 2026 08:18
@spike-rabbit spike-rabbit enabled auto-merge (rebase) February 6, 2026 08:18
@spike-rabbit spike-rabbit added this to the 49.0.0 milestone Feb 6, 2026
@github-actions
Copy link

github-actions bot commented Feb 6, 2026

Code Coverage

@spike-rabbit spike-rabbit merged commit 81dda1f into main Feb 6, 2026
10 checks passed
@spike-rabbit spike-rabbit deleted the test/remove-irrelevant-test branch February 6, 2026 08:32
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.

3 participants