fix: duplicate unique identifiers in MeasureRow column headers #1378
Conversation
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
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新增并导出清理工具 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Render as Renderer
participant MC as MeasureCell
participant DOM as Measure Row DOM
participant MU as measureUtil.cleanMeasureRowAttributes
Render->>MC: 渲染或更新（props: title, columnKey, onColumnResize）
MC->>MC: useLayoutEffect 触发（布局阶段）
MC->>DOM: 获取 contentRef.current
alt contentRef 存在
MC->>MU: cleanMeasureRowAttributes(contentRef.current)
MU->>DOM: 遍历子孙节点，移除 `id` 与指定 `data-*` 属性
MU-->>MC: 返回
else contentRef 为空
MC-->>MC: 跳过清理
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
Summary of ChangesHello @cactuser-Lu, 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! 此拉取请求旨在解决 Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| const allElements = element.querySelectorAll('*'); | ||
| allElements.forEach(el => { | ||
| FILTERED_ATTRIBUTES.forEach(attr => { | ||
| el.removeAttribute(attr); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
当前使用 element.querySelectorAll('*') 的实现只选择了后代元素，而没有包含 element 本身。如果传递给此函数的根元素自身也包含需要过滤的属性，这些属性将不会被移除。为了使此函数更加健壮，它也应该清理根元素上的属性。
[element, ...element.querySelectorAll('*')].forEach(el => {
FILTERED_ATTRIBUTES.forEach(attr => {
el.removeAttribute(attr);
});
});
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1378 +/- ##
==========================================
+ Coverage 96.09% 96.12% +0.02%
==========================================
Files 57 58 +1
Lines 3434 3460 +26
Branches 632 636 +4
==========================================
+ Hits 3300 3326 +26
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Body/MeasureRow.tsx(3 hunks)src/utils/measureUtil.ts(1 hunks)
⏰ 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). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (4)
src/utils/measureUtil.ts (1)
4-14: 属性列表很全面。过滤的属性列表覆盖了常见的唯一标识符和测试框架使用的属性，能够有效防止 ID 重复问题。
src/Body/MeasureRow.tsx (3)
8-8: 导入语句正确。从新的工具文件中正确导入了
cleanMeasureRowAttributes函数。
50-50: 格式调整合理。添加空行提高了代码可读性，将
return语句与前面的逻辑分隔开。
27-31: 无需移除columns依赖项. MeasureRow 的columns来源于useColumns与useWidthColumns的useMemo，引用仅在实际列配置或宽度变更时更新，不会导致不必要的清理操作。Likely an incorrect or invalid review comment.
| export function cleanMeasureRowAttributes(element: HTMLElement): void { | ||
| if (!element) return; | ||
|
|
||
| const allElements = element.querySelectorAll('*'); | ||
| allElements.forEach(el => { | ||
| FILTERED_ATTRIBUTES.forEach(attr => { | ||
| el.removeAttribute(attr); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
函数未清理根元素本身的属性。
querySelectorAll('*') 只选择后代元素，不包括传入的根元素本身。如果 element 自身包含需要过滤的属性（如 id），这些属性不会被移除。
应用此修复：
export function cleanMeasureRowAttributes(element: HTMLElement): void {
if (!element) return;
+ // Remove attributes from the element itself
+ FILTERED_ATTRIBUTES.forEach(attr => {
+ element.removeAttribute(attr);
+ });
+
+ // Remove attributes from all descendants
const allElements = element.querySelectorAll('*');
allElements.forEach(el => {
FILTERED_ATTRIBUTES.forEach(attr => {
el.removeAttribute(attr);
});
});
}🤖 Prompt for AI Agents
In src/utils/measureUtil.ts around lines 22 to 31, the cleaning loop only
iterates over element.querySelectorAll('*') and thus skips the root element
itself; update the function to also remove FILTERED_ATTRIBUTES from the root
element (either by processing element before/after the querySelectorAll loop or
by creating a NodeList/array that includes element plus its descendants) so that
any filtered attributes on the provided root are removed as well.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Body/MeasureCell.tsx(2 hunks)src/Body/MeasureRow.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Body/MeasureCell.tsx (1)
src/utils/measureUtil.ts (1)
cleanMeasureRowAttributes(22-31)
⏰ 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). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (3)
src/Body/MeasureRow.tsx (1)
43-43: 格式改进。添加空行提升了代码可读性。
src/Body/MeasureCell.tsx (2)
16-16: LGTM！正确地为内容容器创建了 ref 引用。
34-36: LGTM！正确地将 ref 应用于内容容器，以便在 layout effect 中进行属性清理。
|
|
||
| if (contentRef.current) { | ||
| cleanMeasureRowAttributes(contentRef.current); | ||
| } | ||
| }, [title, columnKey, onColumnResize]); |
There was a problem hiding this comment.
清理逻辑不完整，且存在性能隐患。
存在以下问题：
-
清理逻辑不完整：
cleanMeasureRowAttributes的实现（位于src/utils/measureUtil.ts）仅使用querySelectorAll('*')清理后代元素，但不会清理contentRef元素本身的属性。如果被克隆的title在根元素上包含id或data-testid，这些属性将不会被移除，导致重复 ID 问题依然存在。 -
依赖项可能导致性能问题：
onColumnResize函数包含在依赖数组中。如果父组件未对该函数进行记忆化处理（使用useCallback），每次渲染都会创建新的函数引用，导致 effect 频繁执行，产生不必要的 DOM 遍历开销。
建议修复方案：
- 在
src/utils/measureUtil.ts中修改cleanMeasureRowAttributes以清理元素本身：
export function cleanMeasureRowAttributes(element: HTMLElement): void {
if (!element) return;
// 清理元素本身的属性
FILTERED_ATTRIBUTES.forEach(attr => {
element.removeAttribute(attr);
});
// 清理所有后代元素
const allElements = element.querySelectorAll('*');
allElements.forEach(el => {
FILTERED_ATTRIBUTES.forEach(attr => {
el.removeAttribute(attr);
});
});
}- 建议在
MeasureRow.tsx中使用useCallback包装传递给MeasureCell的onColumnResize，或者从依赖数组中移除该函数（如果它的稳定性可以保证）。
🤖 Prompt for AI Agents
In src/Body/MeasureCell.tsx around lines 22 to 26, the cleanup logic and effect
dependencies are problematic: cleanMeasureRowAttributes currently only removes
attributes from descendants (via querySelectorAll) and misses attributes on the
contentRef element itself, which can leave duplicate id/data-testid, and
including onColumnResize in the effect deps can cause unnecessary re-runs if
that callback isn't memoized. Update src/utils/measureUtil.ts so
cleanMeasureRowAttributes also removes FILTERED_ATTRIBUTES from the passed
element itself before iterating descendants; and ensure onColumnResize is stable
by wrapping it with useCallback in the parent (MeasureRow) before passing it
down or, if its stability is guaranteed, remove it from the effect dependency
array to avoid repeated DOM traversals.
|
Thank you so much for the quick patch 🙌 |
|
|
||
| if (contentRef.current) { | ||
| cleanMeasureRowAttributes(contentRef.current); | ||
| } |
There was a problem hiding this comment.
如果里面有异步逻辑，比如依赖里面的 id 或者其他属性的做 dom 操作，这样移除后会可能会引发报错。
There was a problem hiding this comment.
如果里面有异步逻辑，比如依赖里面的 id 或者其他属性的做 dom 操作，这样移除后会可能会引发报错。
如果真的依赖id做操作，操作显示的那个dom才是正常的吧；这个dom只是用于测量宽度，实际不可见，反而不去除这些属性就可能有两个id一样的dom
|
复制过去后全部重命名可以吗？ |
可以试试。 |
5 participants
在使用 rc-table 时，当列标题包含 id、data-testid 等唯一标识符时，由于 MeasureRow 会复制列标题内容，会导致 DOM 中出现重复的标识符，这违反了 HTML 规范并可能影响测试和可访问性。
fix: ant-design/ant-design#55244
Summary by CodeRabbit