fix: duplicate unique identifiers in MeasureRow column headers #1376
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. |
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! This pull request resolves a critical DOM validation issue within the 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
|
|
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将 MeasureRow 中内联的标题清洗与克隆逻辑替换为对外工具方法 prepareMeasureTitle；新增 src/utils/measureUtil.ts 提供属性过滤与标题预处理，MeasureRow 将处理后的标题通过 titleForMeasure 传给 MeasureCell，未修改公共 API。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor React
participant MeasureRow
participant measureUtil as prepareMeasureTitle
participant MeasureCell
React->>MeasureRow: render(columns)
MeasureRow->>measureUtil: prepareMeasureTitle(rawTitle)
measureUtil-->>MeasureRow: sanitizedTitle
MeasureRow->>MeasureCell: render(titleForMeasure = sanitizedTitle)
MeasureCell-->>React: measured output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
![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
这个 PR 旨在解决 MeasureRow 复制列标题时，由于 id 等唯一标识符重复导致的问题。通过创建一个新的工具函数 prepareMeasureTitle 来过滤掉这些可能引起冲突的属性，这个方案是合理的。代码实现很清晰，递归地处理了子元素，并且过滤的属性列表也比较全面。
我发现一个新的潜在问题，新的实现似乎遗漏了清除 ref 属性的逻辑，这在之前的代码中是存在的。如果原始的标题元素带有 ref，复制后可能会导致 React 警告或非预期的行为。我在 src/utils/measureUtil.ts 文件中提出了具体的修改建议以修复此问题。
| FILTERED_PROPS.forEach(prop => { | ||
| filteredProps[prop] = undefined; | ||
| }); | ||
|
|
||
| // Recursively filter children if deep filtering is enabled | ||
| if (deep && filteredProps.children) { | ||
| filteredProps.children = filterMeasureChildren(filteredProps.children); | ||
| } |
There was a problem hiding this comment.
此前的 MeasureRow 实现中，通过 React.cloneElement(rawTitle, { ref: null }) 显式地将 ref 置空，以避免在测量用的副本元素上附加 ref。当前的 filterMeasureProps 实现虽然过滤了许多其他属性，但遗漏了对 ref 的处理。如果 title 组件上附加了 ref，这可能会导致非预期的副作用或 React 警告，算是一个功能上的回归。
建议在此处将 ref 也置为 null，以保持原有逻辑并增强健壮性。
FILTERED_PROPS.forEach(prop => {
filteredProps[prop] = undefined;
});
// Nullify ref to avoid warnings and conflicts
filteredProps.ref = null;
// Recursively filter children if deep filtering is enabled
if (deep && filteredProps.children) {
filteredProps.children = filterMeasureChildren(filteredProps.children);
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1376 +/- ##
=======================================
Coverage 96.09% 96.10%
=======================================
Files 57 58 +1
Lines 3434 3492 +58
Branches 632 640 +8
=======================================
+ Hits 3300 3356 +56
- Misses 129 131 +2
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
递归解决不了 DashboardColumnTitle 里面的节点有 id 和 data-* 属性的这种写法。我先回滚。 |
|
可能要用 React.createProtal + dom 操作才有可能彻底解决。 |
|
使用useLayoutEffect在渲染前清理可行吗 |
我又新提了一个，应该可以了 |
2 participants
问题描述
在使用 rc-table 时，当列标题包含
id、data-testid等唯一标识符时，由于MeasureRow会复制列标题内容，会导致 DOM 中出现重复的标识符，这违反了 HTML 规范并可能影响测试和可访问性。fix ant-design/ant-design#55244
close #1375
问题示例