fix(data-table): align defaultSort optionality across internal types and fix sort icon direction#841
Conversation
…and fix sort icon direction
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe data-table sort flow now allows Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/data-table/components/ordering.tsx`:
- Around line 74-80: The sort icon logic in ordering.tsx should treat an
undefined value as ascending instead of falling through to the descending icon.
Update the conditional around value.order in the ordering render so the default
branch maps to SortOrders.ASC, and remove the unnecessary optional chaining on
SortOrders since it is a non-nullable constant. Keep the fix localized to the
icon selection in this component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2459c9a-9cfb-4adb-9e8f-9285043e0cf6
📒 Files selected for processing (4)
packages/raystack/components/data-table/components/ordering.tsxpackages/raystack/components/data-table/data-table.tsxpackages/raystack/components/data-table/data-table.types.tsxpackages/raystack/components/data-table/utils/index.tsx
| {value?.order === SortOrders?.ASC ? ( | ||
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | ||
| ) : ( | ||
| <TextAlignBottomIcon | ||
| className={styles['display-popover-sort-icon']} | ||
| /> | ||
| ) : ( | ||
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm SortOrders is a non-nullable const/enum (no `?.` warranted)
fd -e tsx -e ts | xargs rg -nP '\b(SortOrders)\b\s*=' 2>/dev/null
ast-grep run --pattern 'export const SortOrders = $_' --lang typescript $(fd -e ts -e tsx) 2>/dev/null
rg -nP 'SortOrders\s*:' $(fd -e ts -e tsx) 2>/dev/null | headRepository: raystack/apsara
Length of output: 430
Default state renders the wrong icon and uses unnecessary optional chaining
When value is undefined, the condition value?.order === SortOrders?.ASC evaluates to false, causing the component to render the descending (TextAlignBottomIcon) icon. However, the logical default for a new sort is ascending. Additionally, SortOrders is a non-nullable constant, making the optional chaining (?.) on SortOrders?.ASC redundant.
Update the logic to treat the absent state as ASC and remove the redundant optional chaining:
🔧 Proposed fix
- {value?.order === SortOrders?.ASC ? (
+ {(value?.order ?? SortOrders.ASC) === SortOrders.ASC ? (
<TextAlignTopIcon className={styles['display-popover-sort-icon']} />
) : (
<TextAlignBottomIcon
className={styles['display-popover-sort-icon']}
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {value?.order === SortOrders?.ASC ? ( | |
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | |
| ) : ( | |
| <TextAlignBottomIcon | |
| className={styles['display-popover-sort-icon']} | |
| /> | |
| ) : ( | |
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | |
| )} | |
| {(value?.order ?? SortOrders.ASC) === SortOrders.ASC ? ( | |
| <TextAlignTopIcon className={styles['display-popover-sort-icon']} /> | |
| ) : ( | |
| <TextAlignBottomIcon | |
| className={styles['display-popover-sort-icon']} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/data-table/components/ordering.tsx` around lines
74 - 80, The sort icon logic in ordering.tsx should treat an undefined value as
ascending instead of falling through to the descending icon. Update the
conditional around value.order in the ordering render so the default branch maps
to SortOrders.ASC, and remove the unnecessary optional chaining on SortOrders
since it is a non-nullable constant. Keep the fix localized to the icon
selection in this component.
1 participant
Description
This PR contains two changes to the
data-tablecomponent'sOrderingcontrol and surrounding types.1. Corrected the sort direction icons. In the
Orderingtoggle the ascending/descending icons were swapped — ascending (asc) showed the "align bottom" icon and descending (desc) showed the "align top" icon. They are now correct:ascshowsTextAlignTopIconanddescshowsTextAlignBottomIcon, so the icon matches the actual sort direction.2. Fixed
defaultSorttype-safety issues. ThedefaultSortprop is optional on the publicDataTableProps, but several internal types incorrectly required it (DataTableSortinstead ofDataTableSort | undefined), producing latent type errors and a possible runtime crash in theOrderingcontrol when nodefaultSortis provided.Changes:
Orderingso the direction shown matches the applied sort order.getDefaultTableQueryandhasActiveQueryparameters to optionaldefaultSort?(their bodies already guarded forundefined).TableContextType.defaultSortoptional to match the public prop.sort: defaultSort ? [defaultSort] : []instead of[undefined].Ordering'svalueoptional and guarded its internal accesses (value?.name,value?.order, early-return in the order toggle), preventing a deref of a possibly-undefined value.The public API is unchanged —
defaultSortremains optional and the change is fully backward-compatible.Type of Change
How Has This Been Tested?
raystackpackage; the four changed files (ordering.tsx,data-table.tsx,data-table.types.tsx,utils/index.tsx) produce zero type errors, and no new cross-errors were introduced in any other file.data-table.test.tsx,utils/index.test.tsx,utils/filter-operations.test.tsx).data-view/data-view-betasuites as a sanity check (they keep independent copies of these utils/components) — 59 passed, 1 skipped, unaffected.Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]