refactor: migration to FC#1021
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough该 PR 将 Tree 组件从类组件重构为函数式组件，使用 React Hooks（useRef、useImperativeHandle）管理状态和生命周期，并暴露新的 TreeRef 接口。同时优化了默认 props 的处理方式，并更新了相关测试以适配新的类型系统。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1021 +/- ##
==========================================
+ Coverage 99.84% 99.92% +0.08%
==========================================
Files 14 15 +1
Lines 1302 1417 +115
Branches 396 424 +28
==========================================
+ Hits 1300 1416 +116
+ Misses 2 1 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Tree component from a class-based implementation to a functional component using hooks and React.forwardRef. It also adds onContextMenu and onScroll support to NodeList and includes a ResizeObserver polyfill for the test environment. Feedback focuses on simplifying state updates within the onNodeLoad callback by utilizing the setMergedState helper instead of direct setInnerState calls to ensure consistency and prevent potential state staleness.
There was a problem hiding this comment.
Pull request overview
This PR refactors the core Tree component from a class component to a function component (FC) using hooks/forwardRef, and updates surrounding types/test setup to support the new implementation.
Changes:
- Migrate
src/Tree.tsxfrom a class-based component to a hook-basedforwardRefcomponent while preserving existing behaviors (drag/drop, loadData, keyboard, etc.). - Extend
NodeListprops typing to includeonContextMenuandonScroll. - Add a Jest
ResizeObserverpolyfill in test setup to prevent missing-browser-API failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/setupFilesAfterEnv.js | Adds a ResizeObserver stub for the Jest environment. |
| src/index.ts | Updates exported types from the package entrypoint (per diff). |
| src/Tree.tsx | Large refactor: class component rewritten as a function component with hooks and forwardRef. |
| src/NodeList.tsx | Adds typings for onContextMenu and onScroll passthrough. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Tree.tsx`:
- Around line 339-347: When fieldNames changes (detected by
needSync('fieldNames')), you must rebuild the full derived chain instead of only
updating fieldNames: after setting fieldNames =
fillFieldNames(mergedProps.fieldNames) and newState.fieldNames, also recompute
keyEntities and flattenNodes the same way you do in the treeData/children sync
branch so that derived structures used by expand/select/locate reflect the new
mapping; update the logic around needSync('fieldNames') to call the same code
paths that compute keyEntities and flattenNodes from
mergedProps.treeData/children (and assign them into newState) so shared treeData
references work correctly when only fieldNames changes.
- Around line 796-817: The current call to currentLoadData(treeNode) can
synchronously throw, leaving loadingKeysSetRef and loadingKeys state stuck; wrap
the call so synchronous throws become a rejected Promise (e.g. assign promise =
Promise.resolve().then(() => currentLoadData(treeNode)) or try/catch and set
promise = Promise.reject(err)) and ensure the existing .then/.catch/.finally
handlers still run to remove the key from loadingKeysSetRef and update
loadingKeys via setInnerState (the same pattern should be applied to the other
loadData call site that uses currentLoadData and loadingKeys handling).
- Around line 719-735: The onActiveChange callback currently calls
setMergedState({ activeKey: newActiveKey }) which overrides a controlled
activeKey prop; instead mirror the pattern used for expandedKeys/selectedKeys by
only updating internal state when activeKey is uncontrolled. Update
onActiveChange to check whether props.activeKey (via
mergedPropsRef.current.activeKey or stateRef.current.controlledKeys flag) is
undefined before calling setMergedState, keep scrollTo and
mergedPropsRef.current.onActiveChange?.(newActiveKey) as-is, and ensure the
initial sync logic that reads props.activeKey still sets internal state when
uncontrolled so controlled props are never clobbered by onActiveChange or
initial mount.
🪄 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: 8b10e225-a409-412f-88c6-57ea7991813a
📒 Files selected for processing (4)
src/NodeList.tsxsrc/Tree.tsxsrc/index.tstests/setupFilesAfterEnv.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Tree.spec.tsx (2)
1488-1490: 类型不完整可能导致 TypeScript 错误。
dragNodeProps被赋值为{ eventKey: '0' }，但根据TreeRef接口定义，该字段类型为TreeNodeProps<TreeDataType> | null。这个部分对象可能无法满足完整的TreeNodeProps类型要求。建议：使用类型断言或更完整的模拟对象
- const dragNodeProps = { eventKey: '0' }; + const dragNodeProps = { eventKey: '0' } as any;或者提供符合
TreeNodeProps的完整对象。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Tree.spec.tsx` around lines 1488 - 1490, The test assigns a partial object to treeRef.current.dragNodeProps which is typed as TreeNodeProps<TreeDataType> | null; fix by making the mock satisfy the full TreeNodeProps<TreeDataType> shape or use a type assertion. Update the test to set dragNodeProps to either a complete mock object including required fields from TreeNodeProps (e.g., key, title, children, pos as needed) or cast the partial to the correct type like ({} as unknown as TreeNodeProps<TreeDataType>) before assigning, and keep the expect assertion on treeRef.current.dragNodeProps.
1503-1503: 考虑使用TreeRef替代any以保持一致性。此测试中使用
React.createRef<any>()，而文件中其他类似测试已改用TreeRef。根据上下文代码片段，onNodeLoad、triggerExpandActionExpand、onNodeDrop等方法均已在useImperativeHandle中暴露。如果这是为了测试非公开 API 而有意使用
any，建议添加注释说明原因。建议：统一类型或添加注释
- const treeRef = React.createRef<any>(); + // Use `any` to access internal imperative methods not part of public TreeRef + const treeRef = React.createRef<any>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Tree.spec.tsx` at line 1503, Replace the loose any ref with the specific TreeRef type to match other tests: change the creation from React.createRef<any>() to React.createRef<TreeRef>() (or import/define TreeRef if missing), since methods like onNodeLoad, triggerExpandActionExpand and onNodeDrop are exposed via useImperativeHandle; if you intentionally need any to access non-public internals, add a brief comment above the treeRef declaration explaining why any is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Tree.spec.tsx`:
- Around line 1488-1490: The test assigns a partial object to
treeRef.current.dragNodeProps which is typed as TreeNodeProps<TreeDataType> |
null; fix by making the mock satisfy the full TreeNodeProps<TreeDataType> shape
or use a type assertion. Update the test to set dragNodeProps to either a
complete mock object including required fields from TreeNodeProps (e.g., key,
title, children, pos as needed) or cast the partial to the correct type like ({}
as unknown as TreeNodeProps<TreeDataType>) before assigning, and keep the expect
assertion on treeRef.current.dragNodeProps.
- Line 1503: Replace the loose any ref with the specific TreeRef type to match
other tests: change the creation from React.createRef<any>() to
React.createRef<TreeRef>() (or import/define TreeRef if missing), since methods
like onNodeLoad, triggerExpandActionExpand and onNodeDrop are exposed via
useImperativeHandle; if you intentionally need any to access non-public
internals, add a brief comment above the treeRef declaration explaining why any
is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72f390ad-05e9-4cc7-84f1-c18a5b434f21
📒 Files selected for processing (1)
tests/Tree.spec.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Tree.tsx (1)
1750-1862:useImperativeHandle暴露了过多内部实现细节。
TreeRef接口暴露了大量内部方法（如onNodeClick、onNodeExpand、cleanDragState等），这些本应是私有的。这会增加 API 维护负担，且外部调用这些方法可能导致状态不一致。建议仅暴露真正需要的公共 API（如
scrollTo、state读取），将内部方法保留为私有实现。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Tree.tsx` around lines 1750 - 1862, The useImperativeHandle currently exposes many internal handlers which should remain private; update the exposed API to only public members (e.g., scrollTo, listRef, getters for state/destroyed, and any truly public helpers) and remove internal methods like onNodeClick, onNodeExpand, onNodeDragStart/Enter/Over/Leave/End/Drop, cleanDragState, resetDragState, onListChangeStart/End, onMouseDown, onGlobalMouseUp, onFocus, onBlur, onKeyDown, getTreeNodeRequiredProps, setUncontrolledState, triggerExpandActionExpand, getActiveItem, offsetActiveKey, setExpandedKeys, onNodeLoad, onNodeSelect, onNodeCheck, onNodeMouseEnter/Leave from the object returned by useImperativeHandle and from its dependency array; also update the TreeRef interface to match the reduced public surface and adjust any external callers/tests to use only the new public methods (leave internal functions as module/private refs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Tree.tsx`:
- Around line 589-591: 当前组件在渲染体内直接调用 setInnerState(mergedState)（比较由
shallowEqual(innerState, mergedState) 判断）属于 React 反模式；请把派生状态更新移出渲染流程：要么用 useMemo
计算 mergedState 并直接在渲染中使用该 memoized 值而不维护 innerState（移除 setInnerState 调用），要么保留
innerState 但将条件更新放入 useEffect(() => { if (!shallowEqual(innerState,
mergedState)) setInnerState(mergedState); }, [mergedState])，并确保引用
shallowEqual、innerState、mergedState、setInnerState 的地方一致以避免不必要的重渲染或循环。
In `@src/TreeNode.tsx`:
- Around line 45-46: TreeNode directly destructures the result of
React.useContext(TreeContext) which can be null and causes runtime crashes when
TreeNode is used outside a provider; change the code to guard against a null
context by providing safe defaults or checking before destructuring (e.g.,
ensure the value from React.useContext(TreeContext) is non-null or replace it
with a fallback object) so that symbols like classNames/treeClassNames, styles
and context.disabled are read safely (use the guarded context or optional
chaining) to prevent TypeError when TreeNode is imported/used without a
surrounding TreeContext.Provider.
---
Nitpick comments:
In `@src/Tree.tsx`:
- Around line 1750-1862: The useImperativeHandle currently exposes many internal
handlers which should remain private; update the exposed API to only public
members (e.g., scrollTo, listRef, getters for state/destroyed, and any truly
public helpers) and remove internal methods like onNodeClick, onNodeExpand,
onNodeDragStart/Enter/Over/Leave/End/Drop, cleanDragState, resetDragState,
onListChangeStart/End, onMouseDown, onGlobalMouseUp, onFocus, onBlur, onKeyDown,
getTreeNodeRequiredProps, setUncontrolledState, triggerExpandActionExpand,
getActiveItem, offsetActiveKey, setExpandedKeys, onNodeLoad, onNodeSelect,
onNodeCheck, onNodeMouseEnter/Leave from the object returned by
useImperativeHandle and from its dependency array; also update the TreeRef
interface to match the reduced public surface and adjust any external
callers/tests to use only the new public methods (leave internal functions as
module/private refs).
🪄 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: 0d0b1e61-0f3b-4226-aa50-00ce1983f574
📒 Files selected for processing (5)
src/Tree.tsxsrc/TreeNode.tsxsrc/index.tstests/util.spec.jstests/util.ts
✅ Files skipped from review due to trivial changes (1)
- tests/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
| if (!shallowEqual(innerState, mergedState)) { | ||
| setInnerState(mergedState); | ||
| } |
There was a problem hiding this comment.
在渲染期间调用 setInnerState 是 React 反模式。
当 innerState 与 mergedState 不相等时，直接在渲染函数体内调用 setInnerState(mergedState) 会触发立即重新渲染。虽然 React 18 对此有一定的批处理优化，但这仍然是不推荐的模式，可能导致：
- 性能问题：每次 props 变化都会触发额外的重渲染
- 可能的无限循环风险（如果 shallowEqual 判断不准确）
🔧 建议使用 useEffect 或调整派生状态逻辑
考虑将派生状态计算移入 useMemo 并直接使用 mergedState，而非同步更新 innerState：
const mergedState = React.useMemo(
() =>
mergeState(
innerState,
getDerivedStateFromProps(props, mergedProps, innerState, prevPropsRef.current),
),
[innerState, mergedProps, props],
);
- if (!shallowEqual(innerState, mergedState)) {
- setInnerState(mergedState);
- }
+ // 直接使用 mergedState 作为当前状态，无需同步回 innerState
+ // innerState 仅用于存储用户交互产生的状态变更或者如果确实需要同步，使用 useEffect：
- if (!shallowEqual(innerState, mergedState)) {
- setInnerState(mergedState);
- }
+ React.useEffect(() => {
+ if (!shallowEqual(innerState, mergedState)) {
+ setInnerState(mergedState);
+ }
+ }, [innerState, mergedState]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Tree.tsx` around lines 589 - 591, 当前组件在渲染体内直接调用
setInnerState(mergedState)（比较由 shallowEqual(innerState, mergedState) 判断）属于 React
反模式；请把派生状态更新移出渲染流程：要么用 useMemo 计算 mergedState 并直接在渲染中使用该 memoized 值而不维护
innerState（移除 setInnerState 调用），要么保留 innerState 但将条件更新放入 useEffect(() => { if
(!shallowEqual(innerState, mergedState)) setInnerState(mergedState); },
[mergedState])，并确保引用 shallowEqual、innerState、mergedState、setInnerState
的地方一致以避免不必要的重渲染或循环。
| const context = React.useContext(TreeContext); | ||
| const { classNames: treeClassNames, styles } = context || {}; | ||
| const { classNames: treeClassNames, styles } = context; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 TreeNode 是否被导出并可能被独立使用
rg -n "export.*TreeNode" --type=ts --type=tsx
rg -n "import.*TreeNode.*from" --type=ts --type=tsx -g '!src/Tree.tsx' -g '!src/index.ts'Repository: react-component/tree
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Check TreeNode exports and imports with corrected rg syntax
echo "=== TreeNode exports ==="
rg -n "export.*TreeNode" --type=jsx
echo "=== TreeNode imports outside Tree.tsx ==="
rg -n "import.*TreeNode" --type=jsx | grep -v "src/Tree.tsx"
echo "=== Check index.ts exports ==="
cat -n src/index.ts 2>/dev/null || cat -n src/index.tsx 2>/dev/null
echo "=== Check TreeContext definition ==="
rg -n "createContext" --type=jsx -A 2Repository: react-component/tree
Length of output: 791
🏁 Script executed:
#!/bin/bash
# Try without type filter
echo "=== TreeNode imports (all files) ==="
rg "import.*TreeNode" -n
echo "=== TreeContext definition ==="
cat -n src/contextTypes.ts | head -100Repository: react-component/tree
Length of output: 6453
🏁 Script executed:
#!/bin/bash
# Check actual usage of TreeNode in tests - is it standalone or within Tree?
echo "=== TreeNode usage in tests ==="
grep -n "TreeNode\|Tree" tests/Tree.spec.tsx | head -40
echo "=== Check src/TreeNode.tsx lines around 45-46 ==="
sed -n '40,55p' src/TreeNode.tsx
echo "=== Check src/TreeNode.tsx destructuring pattern ==="
rg -n "context\?" src/TreeNode.tsx | head -20Repository: react-component/tree
Length of output: 2462
🏁 Script executed:
#!/bin/bash
# Check if there's git diff or if we can see the original code
echo "=== Git log for TreeNode.tsx ==="
git log --oneline -10 src/TreeNode.tsx 2>/dev/null || echo "Git history unavailable"
echo "=== Check full usage of context in TreeNode.tsx ==="
sed -n '45,65p' src/TreeNode.tsx
echo "=== Check if context.disabled is safe with null ==="
grep -n "context\." src/TreeNode.tsx | head -15Repository: react-component/tree
Length of output: 1984
删除 context 的空值保护存在运行时崩溃风险。
TreeContext 的默认值为 null，而 TreeNode.tsx 第 46 行直接解构 context 属性而无空值检查：const { classNames: treeClassNames, styles } = context;。如果 TreeNode 在 TreeContext.Provider 外部渲染（例如独立导入使用），会抛出 TypeError: Cannot destructure property 'classNames' of 'null'。此外，后续代码如第 55 行 context.disabled 也会引发错误。
虽然当前所有测试和示例都在 Tree 组件内渲染 TreeNode，但由于 TreeNode 在 src/index.ts 中被公开导出，消费者可以独立导入和使用它，这构成了一个潜在的破坏性变更。
建议保留防御性检查：
const context = React.useContext(TreeContext);
- const { classNames: treeClassNames, styles } = context;
+ const { classNames: treeClassNames, styles } = context || {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TreeNode.tsx` around lines 45 - 46, TreeNode directly destructures the
result of React.useContext(TreeContext) which can be null and causes runtime
crashes when TreeNode is used outside a provider; change the code to guard
against a null context by providing safe defaults or checking before
destructuring (e.g., ensure the value from React.useContext(TreeContext) is
non-null or replace it with a fallback object) so that symbols like
classNames/treeClassNames, styles and context.disabled are read safely (use the
guarded context or optional chaining) to prevent TypeError when TreeNode is
imported/used without a surrounding TreeContext.Provider.
|
看下 AI 的 review |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -43,7 +43,7 @@ const TreeNode: React.FC<Readonly<TreeNodeProps>> = props => { | |||
| const nodeId = getId(treeId, eventKey); | |||
|
|
|||
| const context = React.useContext(TreeContext); | |||
There was a problem hiding this comment.
TreeContext is created with a default value of null (see src/contextTypes.ts), so React.useContext(TreeContext) can be null when TreeNode is rendered outside of a provider (or during import-loop edge cases called out in contextTypes.ts). Destructuring context directly will throw at runtime. Consider restoring a safe fallback (e.g. const { ... } = context || {}) or ensuring TreeContext is created with a non-null default value.
| const context = React.useContext(TreeContext); | |
| const context = | |
| React.useContext(TreeContext) || | |
| ({} as NonNullable<React.ContextType<typeof TreeContext>>); |
| const mergedState = React.useMemo( | ||
| () => | ||
| mergeState( | ||
| innerState, | ||
| getDerivedStateFromProps(props, mergedProps, innerState, prevPropsRef.current), | ||
| ), | ||
| [innerState, mergedProps, props], | ||
| ); | ||
|
|
||
| // ================= loadedKeys ================== | ||
| if (needSync('loadedKeys')) { | ||
| newState.loadedKeys = props.loadedKeys; | ||
| if (!shallowEqual(innerState, mergedState)) { | ||
| setInnerState(mergedState); | ||
| } | ||
|
|
There was a problem hiding this comment.
Calling setInnerState during render (if (!shallowEqual(...)) { setInnerState(...) }) is an unsafe pattern and can cause render loops / React warnings (state updates during render). This derived-state sync should be moved into an effect (useEffect/useLayoutEffect) or refactored so state is computed without a render-time state update.
| const shallowEqual = (objA: object, objB: object) => { | ||
| if (objA === objB) { | ||
| return true; | ||
| } | ||
| const keysA = Object.keys(objA); | ||
| return ( | ||
| keysA.length === Object.keys(objB).length && keysA.every(key => Object.is(objA[key], objB[key])) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
shallowEqual indexes into parameters typed as object (objA[key] / objB[key]), which is not type-safe and can produce TS errors depending on compiler options. Consider typing these as Record<string, unknown> (or a generic constrained to record types) to avoid unsafe indexing.
| mergedState.loadedKeys.forEach(key => { | ||
| pendingLoadedKeysRef.current.delete(key); | ||
| loadingKeysSetRef.current.delete(key); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This forEach mutates refs (pendingLoadedKeysRef, loadingKeysSetRef) during render, which is a side effect and can behave incorrectly under StrictMode/concurrent rendering (render may run more than once). Consider moving this cleanup into an effect that runs when mergedState.loadedKeys changes.
| mergedState.loadedKeys.forEach(key => { | |
| pendingLoadedKeysRef.current.delete(key); | |
| loadingKeysSetRef.current.delete(key); | |
| }); | |
| React.useEffect(() => { | |
| mergedState.loadedKeys.forEach(key => { | |
| pendingLoadedKeysRef.current.delete(key); | |
| loadingKeysSetRef.current.delete(key); | |
| }); | |
| }, [mergedState.loadedKeys]); |
| it('convertTreeToEntities keeps wrapper when initWrapper returns nothing', () => { | ||
| const wrapper = convertDataToEntities([{ key: 'test' }], { | ||
| initWrapper: jest.fn(() => undefined), | ||
| }); | ||
|
|
||
| expect(wrapper.keyEntities.test.key).toBe('test'); | ||
| }); |
There was a problem hiding this comment.
Test name says convertTreeToEntities, but the test is exercising convertDataToEntities. Renaming the test case will make failures easier to interpret.
Summary by CodeRabbit
发布说明
新功能
改进
重构