fix: guard scrollTo against a detached list ref - #1067
Conversation
|
@hippee-lee is attempting to deploy a commit to the afc163's projects Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough本次修改使 Changes滚动调用保护与回归测试
Estimated code review effort: 2 (简单) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Tree.spec.tsx (1)
1087-1105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win补充
NodeList内层引用的回归覆盖。当前用例主要验证首次挂载时
Tree.listRef为空的路径。src/Tree.tsx的 Line 1410 会直接跳过调用，因此该用例不保证会执行src/NodeList.tsx的 Line 164。如果
NodeList的内部VirtualList引用在重渲染期间为空，而外层NodeListRef仍存在，NodeList的保护可能回归且测试仍会通过。请增加直接覆盖NodeListRef.scrollTo的测试，或构造该重渲染时序。🤖 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 `@tests/Tree.spec.tsx` around lines 1087 - 1105, 增强测试用例以直接覆盖 NodeListRef.scrollTo 在内部 VirtualList 引用暂时为空时的保护逻辑，而不仅依赖 Tree.listRef 为空导致 Tree.tsx 跳过调用。围绕 NodeListRef.scrollTo 构造重渲染时序或直接调用该方法，验证 NodeList.tsx 中的内部引用缺失不会抛出异常。
🤖 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.
Nitpick comments:
In `@tests/Tree.spec.tsx`:
- Around line 1087-1105: 增强测试用例以直接覆盖 NodeListRef.scrollTo 在内部 VirtualList
引用暂时为空时的保护逻辑，而不仅依赖 Tree.listRef 为空导致 Tree.tsx 跳过调用。围绕 NodeListRef.scrollTo
构造重渲染时序或直接调用该方法，验证 NodeList.tsx 中的内部引用缺失不会抛出异常。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cba34716-f27d-4448-86c1-6e6bfd14032d
📒 Files selected for processing (3)
src/NodeList.tsxsrc/Tree.tsxtests/Tree.spec.tsx
nrps9909
left a comment
nrps9909
left a comment
There was a problem hiding this comment.
Reviewed exact head 4682557bc0ade17847227be55082b1ca77fa09c8.
The two guards match the transient ref-detachment failure mode without changing normal scrolling: the public Tree.scrollTo path safely no-ops before NodeList attaches, while NodeListRef.scrollTo safely no-ops during the narrower window where its inner VirtualList ref is unavailable. Once either ref is attached, the existing call and arguments are unchanged.
I independently reproduced both layers against base b35c7b28e45e37b09bcbeab8b17dfe0ebb0c5680:
- the submitted autofocus scenario failed on base with
TypeError: Cannot read properties of null (reading 'scrollTo')and passes on this head; - a temporary isolated probe that kept
NodeListRefattached while withholding the innerVirtualListref failed on base atNodeList.tsx:164and passes on this head.
Exact-head validation:
npm test -- tests/Tree.spec.tsx --runInBand— 68/68 passed, 8/8 snapshotsnpm test -- --runInBand— 223/223 passed, 42/42 snapshotsnpm run tsc— passednpm run lint -- --no-fix— 0 errors (11 existing warnings)- Prettier check for all changed files — passed
npm run build— passedgit diff --check b35c7b28e45e37b09bcbeab8b17dfe0ebb0c5680...4682557bc0ade17847227be55082b1ca77fa09c8— passed
The repository's GitHub Actions runs are marked action_required with no jobs because the external contribution requires maintainer authorization; this is not a test failure. The Vercel status is likewise an external deployment-authorization gate. I found no blocking issue in the code.
Disclosure: Codex assisted with source tracing, test execution, and drafting this review; I independently verified the reproductions and conclusion against the exact PR head.
|
@li-jia-nan @yoyo837 @zombieJ @afc163 大佬们帮忙看看这个 |
|
@yoyo837 needs to trigger the workflow again |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1067 +/- ##
=======================================
Coverage 99.84% 99.84%
=======================================
Files 15 15
Lines 1314 1314
Branches 387 396 +9
=======================================
Hits 1312 1312
Misses 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
这次改了两处 scrollTo，但新增用例在第一处就被拦住了，第二处修改其实没有测试到。建议再补一个能执行到 NodeList.tsx 的用例。 |
nrps9909
left a comment
nrps9909
left a comment
There was a problem hiding this comment.
Re-reviewing this after #1063 merged, I no longer think the reported #1068 reproduction justifies merging this PR as-is.
On current master (528f6b0da40ac7bbc4bee7c676bce45c0df8586b), I added the original minimal scenario—a title <input autoFocus />—and tests/Tree.spec.tsx passed (70/70). For a causal check, I temporarily removed only #1063's event.target === event.currentTarget guard from Tree.onFocus; that same test then failed at Tree.tsx:1413 with TypeError: Cannot read properties of null (reading 'scrollTo'). Restoring the guard made it pass again.
That demonstrates that merged PR #1063 already resolves the reported descendant-autofocus path before Tree.scrollTo is reached. The separate NodeList.tsx optional-chain change may still be defensively useful, but this PR does not include the distinct inner-VirtualList ref-detachment regression requested in the maintainer comment, so there is not yet a failing current-master case proving that remaining change.
Please either close this as superseded for #1068, or narrow it to a separately reproducible NodeList gap with a regression test that fails on current master and passes with the guard.
Disclosure: Codex assisted with source/history tracing, the temporary causal mutation, test execution, and drafting; I verified the commands and exact revisions locally.
4 participants
NodeList'suseImperativeHandlehas no dependency array, so every re-render detaches the handle during the commit's mutation phase and re-attaches it in the layout phase. Anything that runs between those two — for example anautoFocusinput mounting inside a node title, whose bubbled focus triggers the tree's focus activation andscrollTo— dereferences a nulllistRefand unmounts the whole React root.This adds optional chaining at the two
listRef.current.scrollTocall sites, plus a regression test that reproduces the crash on first mount (fails withTypeError: Cannot read properties of null (reading 'scrollTo')without the fix).Downstream, this crash reaches every antd
Tree/TreeSelect/Cascaderconsumer.All checks pass locally:
lint(0 errors),tsc,test --runInBand(223/223),build.Summary by CodeRabbit
Bug Fixes
Tests
Fixes #1068