[codex] refactor grouped data hooks#44
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
Walkthrough该 PR 重构了分组数据结构和相关 API。useGroupSegments 从返回连续段索引改为返回按 key 聚合的 Map，useFlattenRows 相应接收新的 groupData 结构；scrollTo 移除工具函数依赖改用结构判断；公共类型从 interface.ts 迁移到 List.tsx；测试全面更新以验证新的 Map 结构。 ChangesGroup Data Structure and List Type Reorganization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/hooks/useFlattenRows.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected][email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. src/List.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected][email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. src/hooks/useStickyGroupHeader.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs[email protected][email protected]/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44 +/- ##
==========================================
- Coverage 96.37% 95.90% -0.48%
==========================================
Files 5 4 -1
Lines 138 122 -16
Branches 38 34 -4
==========================================
- Hits 133 117 -16
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the grouping and flattening logic by replacing useGroupSegments and useFlattenRows with useGroupData and useFlattenData, while also consolidating type definitions into List.tsx. Feedback highlights a breaking change where support for static group keys was removed, potentially causing runtime errors, and suggests optimizing the data flattening process by combining multiple iterations into a single pass.
| import * as React from 'react'; | ||
|
|
||
| export interface Group<T, K extends React.Key = React.Key> { | ||
| key: (item: T) => K; |
There was a problem hiding this comment.
The Group.key type has been narrowed from ((item: T) => K) | K to strictly (item: T) => K. This is a breaking change that removes support for static group keys, which was previously supported in interface.ts. Unless this was an intentional API simplification, consider restoring the union type to maintain backward compatibility.
| key: (item: T) => K; | |
| key: ((item: T) => K) | K; |
| } | ||
|
|
||
| data.forEach((item, index) => { | ||
| const groupKey = group.key(item); |
There was a problem hiding this comment.
To support both function and static values for group.key (consistent with the previous implementation in useGroupSegments), the logic should check the type of group.key before calling it to avoid runtime errors.
| const groupKey = group.key(item); | |
| const groupKey = typeof group.key === 'function' ? group.key(item) : group.key; |
| groupData.forEach((groupItems, groupKey) => { | ||
| groupKeyToItems.set( | ||
| groupKey, | ||
| groupItems.map(({ item }) => item), | ||
| ); | ||
|
|
||
| headerRows.push({ groupKey, rowIndex: flatRows.length }); | ||
| flatRows.push({ type: 'header', groupKey }); | ||
|
|
||
| groupItems.forEach(({ item, index }) => { | ||
| flatRows.push({ type: 'item', item, index }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The current implementation iterates over groupItems twice: once for mapping items to groupKeyToItems and once for populating flatRows. These operations can be combined into a single pass to improve performance and reduce intermediate array allocations.
groupData.forEach((groupItems, groupKey) => {
const items: T[] = [];
headerRows.push({ groupKey, rowIndex: flatRows.length });
flatRows.push({ type: 'header', groupKey });
groupItems.forEach(({ item, index }) => {
items.push(item);
flatRows.push({ type: 'item', item, index });
});
groupKeyToItems.set(groupKey, items);
});
1 participant
Summary
interface.tsand move public/list-related types next to their owning modules.useGroupDataanduseFlattenData.useGroupSegments,useFlattenRows, and the tiny util file after inlining their remaining responsibilities.Validation
npx tsc --noEmit --pretty falsenpm test -- --runInBandnpm run lintnpm run compileSummary by CodeRabbit
发布说明
Bug Fixes
Refactor