Skip to content

refactor: use shared renderability and value guards - #1252

Merged
yoyo837 merged 3 commits into
react-component:masterfrom
QDyanbing:refactor-renderable-guards
Sep 3, 2026
Merged

refactor: use shared renderability and value guards#1252
yoyo837 merged 3 commits into
react-component:masterfrom
QDyanbing:refactor-renderable-guards

Conversation

@QDyanbing

@QDyanbing QDyanbing commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

说明

  • 使用 isNonNullable 替换 combobox value 与 suffix icon 的本地非空判断
  • 使用 isReactRenderable 统一 clear icon 的 fallback 与渲染逻辑,支持数字 0
  • 移除重复的本地 hasValue helper,并将 @rc-component/util 最低版本提升到 ^1.13.0

验证

  • npm run tsc
  • npm run lint
  • npm test -- tests/Select.test.tsx --runInBand

Summary by CodeRabbit

  • 改进
    • 优化清除图标的显示判断,支持数字 0 等可渲染内容作为清除图标。
    • 当配置或传入的清除图标不可渲染时,自动使用备用图标。
    • 优化组合框模式下搜索值与选中值的同步处理。
    • 优化前缀、后缀及相关内容的渲染判断,确保数字等可渲染内容正常显示,并避免显示不可渲染内容。

@vercel

vercel Bot commented Sep 3, 2026

Copy link
Copy Markdown

@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: ee794407-bacd-4c29-abee-4831fbc2bbdc

📥 Commits

Reviewing files that changed from the base of the PR and between 7296b1b and 677efee.

📒 Files selected for processing (2)
  • src/SelectInput/Affix.tsx
  • tests/Multiple.test.tsx

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.


Walkthrough

本次变更升级 @rc-component/util。选择器使用统一的非空值判断和 React 可渲染值判断。hasValue 工具函数被移除。测试覆盖数字 0 的清除图标、前缀和后缀。

Changes

选择器值与图标处理

Layer / File(s) Summary
统一非空值判断
package.json, src/BaseSelect/index.tsx, src/Select.tsx, src/utils/commonUtil.ts
@rc-component/util 更新至 ^1.13.0。BaseSelect 和 Select 使用 isNonNullableSelect 移除 hasValuecommonUtil 移除该函数。
清除图标与前后缀渲染
src/SelectInput/index.tsx, src/hooks/useAllowClear.tsx, src/SelectInput/Affix.tsx, tests/Select.test.tsx, tests/Multiple.test.tsx
使用 isReactRenderable 处理清除图标、前缀和后缀。不可渲染的清除图标回退为 '×'。测试验证数字 0 可渲染,以及 false、空字符串、nullundefined 不渲染。

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 677ef

Select now consistently renders numeric zero for clear icons and prefix/suffix affixes while omitting non-renderable values. The covered behavior and passing checks show no current merge-blocking risk.

Poem

小兔更新工具箱,
非空判断更统一。
数字零也能成图标,
前缀后缀清晰现。
选择器通过新测试。

🚥 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 标题准确概括了此次变更的主要内容:统一使用共享的可渲染性和数值检查工具。标题简洁、明确,并与代码变更和 PR 目标一致。
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 7 files.
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.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.46%. Comparing base (0128e29) to head (677efee).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1252   +/-   ##
=======================================
  Coverage   99.46%   99.46%           
=======================================
  Files          31       31           
  Lines        1304     1307    +3     
  Branches      473      475    +2     
=======================================
+ Hits         1297     1300    +3     
  Misses          7        7           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yoyo837

yoyo837 commented Sep 3, 2026

Copy link
Copy Markdown
Member

CI failed.

@QDyanbing

Copy link
Copy Markdown
Contributor Author

已补充 clearIcon={0} 的组件测试并推送 7296b1b。当前 Codecov patch、Codecov project 和 test workflow 均已通过。剩余的 Vercel 失败为 Authorization required to deploy,需要 React Component 团队成员授权,不是代码或测试失败。

@yoyo837 yoyo837 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The migration is incomplete for numeric-zero affixes.

Both prefix and suffix eventually pass through src/SelectInput/Affix.tsx, which still returns null for !children. Therefore prefix={0} and suffix={0} are still dropped. For a zero suffix, BaseSelect can also add the -show-arrow class while no affix is rendered, leaving CSS state and DOM output inconsistent.

Please update Affix to use isReactRenderable(children) and add coverage for numeric zero plus the false/empty/nullish fallback cases. The current clear-icon changes and the ^1.13.0 dependency update otherwise look reasonable.

@QDyanbing

Copy link
Copy Markdown
Contributor Author

Addressed in 677efee. Affix now uses isReactRenderable(children), with integration coverage for numeric-zero prefix/suffix values and the false, empty-string, and nullish boundaries. The full test suite (443 tests), TypeScript, and coverage checks pass locally.

@QDyanbing
QDyanbing requested a review from yoyo837 September 3, 2026 03:44

@yoyo837 yoyo837 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Affix now renders numeric-zero children, with integration coverage for the affix and clear-icon boundaries, and the coverage gates are green again. LGTM.

@yoyo837
yoyo837 merged commit 59dd34a into react-component:master Sep 3, 2026
11 of 12 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