Skip to content

Conversation

@S-A-Adit
Copy link
Contributor

@S-A-Adit S-A-Adit commented Jan 8, 2026

Refactors the compareString function to safely handle null and undefined inputs and improves overall string comparison logic. This prevents incorrect sort behavior and ensures consistent ordering when comparing mixed or missing values, particularly in table view sorting scenarios.

Summary by CodeRabbit

  • Bug Fixes
    • Improved string comparison used for sorting: empty values are consistently placed last, numeric parts sort numerically before non-numeric parts, and mixed-type and case variations are handled more predictably for stable, consistent ordering across data views.

✏️ Tip: You can customize this high-level summary in your review settings.

Refactor compareString function to handle null/undefined inputs and improve string comparison logic.
@S-A-Adit S-A-Adit requested a review from a team as a code owner January 8, 2026 02:12
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

📝 Walkthrough

Walkthrough

The compareString function in the sort evaluation module was refactored to coerce inputs to strings, treat empty strings as greater than non-empty, normalize parts to strings before case-folding, and clarify numeric vs non-numeric part ordering while preserving overall sorting flow.

Changes

Cohort / File(s) Summary
String comparison refinement
blocksuite/affine/data-view/src/core/sort/eval.ts
Inputs are coerced via String(a ?? '') / String(b ?? ''); empty-string handling changed so empty <-> non-empty places empty strings last; splitting operates on coerced strings; per-part tokens explicitly coerced to strings before toLowerCase; numeric vs non-numeric comparison behavior clarified and enforced.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hopped through parts and made them strings,

Empty tails now sit where silence rings.
Numbers first, then letters small,
Split and lowered—order for all.
A tidy sort from my rabbit hall. 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and specifically describes the main change: enhancing string comparison handling in the eval.ts file, which aligns with the PR's core objective of refactoring compareString to handle null/undefined inputs and improve sort behavior.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @blocksuite/affine/data-view/src/core/sort/eval.ts:
- Around line 51-52: The code incorrectly uses logical OR when normalizing
values to strings, so falsy-but-valid values like 0 and false become empty
strings; change the normalization in eval.ts to use nullish coalescing for both
variables (replace the expressions that set strA and strB from String(a || '') /
String(b || '') with String(a ?? '') and String(b ?? '')) so only null/undefined
map to empty strings and other falsy values are preserved.
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e4dc82e and 9ba064e.

📒 Files selected for processing (1)
  • blocksuite/affine/data-view/src/core/sort/eval.ts
🔇 Additional comments (3)
blocksuite/affine/data-view/src/core/sort/eval.ts (3)

54-62: Empty string handling looks correct (after fixing the ?? issue above).

The logic consistently treats empty strings as "greater than" non-empty strings, ensuring they sort last. This aligns with the PR objective of "consistent ordering when comparing mixed or missing values" and matches the null handling pattern in compareNumber (lines 89-94).


67-68: Good defensive coding practice.

While split() returns a string array, explicitly coercing to strings before calling toLowerCase() adds robustness. This prevents potential runtime errors if the function's input handling changes in the future.


75-86: Numeric vs non-numeric comparison logic is correct.

The implementation correctly prioritizes numeric parts before non-numeric parts, with clear comments. Using localeCompare for string parts provides locale-aware sorting, which is good for internationalization.

@darkskygit darkskygit changed the title Enhance string comparison handling in eval.ts feat(editor): enhance string comparison handling in eval.ts Jan 9, 2026
@darkskygit darkskygit enabled auto-merge January 9, 2026 02:27
@codecov
Copy link

codecov bot commented Jan 9, 2026

Codecov Report

❌ Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.50%. Comparing base (e4dc82e) to head (cd2d9ca).
⚠️ Report is 1 commits behind head on canary.

Files with missing lines Patch % Lines
blocksuite/affine/data-view/src/core/sort/eval.ts 0.00% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           canary   #14233      +/-   ##
==========================================
+ Coverage   56.40%   56.50%   +0.09%     
==========================================
  Files        2762     2775      +13     
  Lines      141590   142448     +858     
  Branches    21745    21703      -42     
==========================================
+ Hits        79868    80491     +623     
- Misses      59487    59623     +136     
- Partials     2235     2334      +99     
Flag Coverage Δ
server-test 76.15% <ø> (-1.18%) ⬇️
unittest 32.04% <0.00%> (+1.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

@darkskygit darkskygit added this pull request to the merge queue Jan 9, 2026
Merged via the queue into toeverything:canary with commit d515d29 Jan 9, 2026
109 of 111 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants