avoiding closure problems #1352
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough本次变更调整 FixedHolder 中 ColGroup 的渲染控制：以 mayBeEmpty 决定是否渲染 ColGroup，仅在存在数据且 mergedColumnWidth 有效时渲染；同时精简了 useMemo 依赖并移除之前的 colGroupNode 分支。示例代码 measureRowRender 的渲染包装逻辑也被条件化处理。 Changes
Sequence Diagram(s)sequenceDiagram
participant Data as 输入 (noData, mergedColumnWidth)
participant FixedHolder as FixedHolder
participant ColGroup as ColGroup
Note over Data,FixedHolder: FixedHolder 接收 noData 与 mergedColumnWidth
Data-->>FixedHolder: 提供 noData, mergedColumnWidth
Note over FixedHolder: 计算 mayBeEmpty = noData || !mergedColumnWidth
alt mayBeEmpty = true
FixedHolder--x ColGroup: 不渲染（返回 null）
else mayBeEmpty = false
FixedHolder->>ColGroup: 渲染 ColGroup\nprops: colWidths=[...mergedColumnWidth, combinationScrollBarSize]\ncolumCount+1, columns=flattenColumnsWithScrollbar
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @li-jia-nan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the rendering mechanism for the ColGroup component within FixedHolder to address potential closure-related issues and enhance rendering efficiency. It extracts the complex conditional logic for ColGroup's presence into a dedicated memoized boolean, mayBeEmpty, which then drives the direct conditional rendering of the component, leading to a more predictable and optimized update cycle.
Highlights
- Refactored ColGroup Rendering Logic: The logic for conditionally rendering the
ColGroupcomponent has been refactored to improve performance and avoid potential closure issues. - Introduced
mayBeEmptyMemoized State: A newuseMemohook,mayBeEmpty, was introduced to encapsulate the boolean logic determining if theColGroupshould be rendered, based on data availability and column width validity. - Optimized
useMemoDependencies: By separating the conditional logic, the dependencies for the memoized boolean are now more focused, which helps prevent unnecessary re-renders of theColGroupcomponent.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1352 +/- ##
==========================================
- Coverage 96.11% 96.10% -0.02%
==========================================
Files 57 57
Lines 3449 3437 -12
Branches 632 629 -3
==========================================
- Hits 3315 3303 -12
Misses 129 129
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FixedHolder/index.tsx (1)
90-121: 修复 FixedHolder 中 wheel 监听器的闭包陈旧（direction / onScroll）当前 useEffect([]) 捕获了初始的 direction 与 onScroll，组件更新后事件仍会使用旧值。应使用 refs 保存最新回调与方向，保持监听器绑定稳定。
位置：src/FixedHolder/index.tsx（useEffect 注册 wheel 监听器，约 90–121 行）
const scrollRef = React.useRef<HTMLDivElement>(null); const setScrollRef = React.useCallback((element: HTMLElement) => { fillRef(ref, element); fillRef(scrollRef, element); }, []); + // 保存最新 onScroll 与 direction，避免闭包陈旧 + const onScrollRef = React.useRef(onScroll); + const directionRef = React.useRef(direction); + React.useEffect(() => { + onScrollRef.current = onScroll; + }, [onScroll]); + React.useEffect(() => { + directionRef.current = direction; + }, [direction]); + React.useEffect(() => { function onWheel(e: WheelEvent) { const { currentTarget, deltaX } = e as unknown as React.WheelEvent<HTMLDivElement>; if (deltaX) { const { scrollLeft, scrollWidth, clientWidth } = currentTarget; const maxScrollWidth = scrollWidth - clientWidth; let nextScroll = scrollLeft + deltaX; - if (direction === 'rtl') { + if (directionRef.current === 'rtl') { nextScroll = Math.max(-maxScrollWidth, nextScroll); nextScroll = Math.min(0, nextScroll); } else { nextScroll = Math.min(maxScrollWidth, nextScroll); nextScroll = Math.max(0, nextScroll); } - onScroll({ + onScrollRef.current({ currentTarget, scrollLeft: nextScroll, }); e.preventDefault(); } } const scrollEle = scrollRef.current; scrollEle?.addEventListener('wheel', onWheel, { passive: false }); return () => { scrollEle?.removeEventListener('wheel', onWheel); }; }, []);在切换 direction 或替换 onScroll 回调时，验证滚动行为是否与预期一致，确认未出现双绑或遗漏解绑。
🧹 Nitpick comments (1)
src/FixedHolder/index.tsx (1)
160-162: 注释与实现不一致，容易误导维护者注释写的是“无数据或无列宽时使用 original ColGroup”，但实现是直接不渲染。若保留当前实现，请同步更新注释，或恢复对应的回退逻辑，避免后续阅读者产生误解。
建议更新为：
- // use original ColGroup if no data or no calculated column width, otherwise use calculated column width - // Return original colGroup if no data, or mergedColumnWidth is empty, or all widths are falsy + // 仅在「无数据 ∧ 未计算到列宽 ∧ 不存在滚动条列」时跳过渲染 ColGroup； + // 其余情况渲染，以保证滚动条列与 sticky/fixed 布局的对齐稳定性。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/FixedHolder/index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bbb169
PR: react-component/table#1202
File: src/Table.tsx:903-904
Timestamp: 2024-11-08T12:53:09.293Z
Learning: 在 `src/Table.tsx` 文件的 React 组件 `Table` 中，即使 `bodyScrollLeft` 频繁更新，也需要在 `TableContextValue` 的 `useMemo` 依赖数组中包含 `bodyScrollLeft` 和 `headerCellRefs`，因为每次滚动时重新计算 `TableContextValue` 是解决该问题所必须的。
🔇 Additional comments (1)
src/FixedHolder/index.tsx (1)
190-196: 在空数据但存在固定列/滚动条时仍渲染 ，并校验 colWidths 长度一致文件：src/FixedHolder/index.tsx 行 ~190-196
- 当前仅在 !mayBeEmpty 时渲染 ，会在“空数据 + sticky/fixed/滚动条列”场景缺失 ，导致列对齐/滚动条列宽异常。修改 mayBeEmpty 判定或在存在固定列/滚动条时强制渲染 。
- 确认 columCount + 1 与 colWidths = [...mergedColumnWidth, combinationScrollBarSize] 在所有分支长度一致；若不一致，修正列计数或 colWidths 构造逻辑以保证一一对应。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/interface.ts (1)
156-159: 微优化：React.FC<P>与React.ComponentType<P>存在重叠
React.FC<P>本质可赋值给React.ComponentType<P>，保留两者略显冗余。可按需精简（不影响行为）。可选精简：
type Component<P> = | React.ComponentType<P> | React.ForwardRefExoticComponent<P> - | React.FC<P> - | keyof JSX.IntrinsicElements; + | keyof JSX.IntrinsicElements;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/demo/measureRowRender.md(1 hunks)src/interface.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/demo/measureRowRender.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test / react component workflow
- GitHub Check: test / react component workflow
🔇 Additional comments (2)
src/interface.ts (2)
156-161: AI 摘要与代码不一致，请确认是否有意移除 React.ReactHTML摘要称“新增了
keyof React.JSX.IntrinsicElements”，但当前代码未见keyof React.ReactHTML。如果旧版曾允许 HTML 标签键且你不想扩大到 SVG，可考虑继续保留或改为仅 HTML。请确认意图。
156-161: 建议：将 React.JSX.IntrinsicElements 替换为全局 JSX.IntrinsicElements；需验证 React/@types/react 版本与 tsconfig.jsx 配置原因：React.JSX.IntrinsicElements 依赖较新 React 类型定义与 TS 配置，使用全局 JSX.IntrinsicElements 在更多 TS/React 组合下更兼容。上次校验脚本运行时出现 jq/路径解析噪音（node_modules 的 tsconfig），无法自动确认。运行下列修正脚本并粘贴输出以完成验证。
可选修正（仅类型层变动，不影响运行时）：
type Component<P> = | React.ComponentType<P> | React.ForwardRefExoticComponent<P> - | React.FC<P> - | keyof React.JSX.IntrinsicElements; + | React.FC<P> + | keyof JSX.IntrinsicElements;修正校验脚本（在仓库根目录运行并粘贴输出）：
#!/bin/bash set -euo pipefail if [ ! -f package.json ]; then echo "package.json not found in current directory" exit 0 fi echo "React versions (dependencies / peerDependencies / devDependencies):" jq -r ' [ (.dependencies.react // empty), (.peerDependencies.react // empty), (.devDependencies.react // empty) ] | unique[]' package.json || true echo echo "@types/react versions (dependencies / peerDependencies / devDependencies):" jq -r ' [ (.dependencies["@types/react"] // empty), (.peerDependencies["@types/react"] // empty), (.devDependencies["@types/react"] // empty) ] | unique[]' package.json || true echo echo "tsconfig files (excluding node_modules) and jsx settings:" find . -path './node_modules' -prune -o -type f -name 'tsconfig*.json' -print | while read -r f; do echo "==> $f" jq -r '.compilerOptions.jsx // "N/A", .compilerOptions.jsxImportSource // "N/A"' "$f" || true done echo echo "Search for legacy React.ReactHTML usage (excluding node_modules/dist):" rg -n --hidden 'React\.ReactHTML' --glob '!**/node_modules/**' --glob '!**/dist/**' || true若不希望放宽到 SVG（例如在 table 语境下不期望 ），可改为只接受 HTML 标签键（参见下一条评论）。
| !mergedColumnWidth.length || | ||
| mergedColumnWidth.every(width => !width) | ||
| ) { | ||
| return ColGroup; |
There was a problem hiding this comment.
ColGroup 是一个函数，直接渲染到 UI 会报错，这里应该返回 null
docs/examples/measureRowRender.tsx
Outdated
| // 按照 html 规范，tr 的父元素必须是 table、thead、tbody、tfoot 标签，子元素必须是 th、td 标签 | ||
| // 因此这里我们用一个 div 包裹是不对的，在控制台中会报错 | ||
| const measureRowRender: TableProps['measureRowRender'] = measureRow => ( | ||
| <div style={{ display: 'none' }}>{measureRow}</div> |
There was a problem hiding this comment.
不行，加什么都会报错，原本的 dom 结构就是刚刚好，我直接用 cloneElement 把 style 传进去了
2 participants
Summary by CodeRabbit