Revert "fix: duplicate unique identifiers in MeasureRow column headers" #1377
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @afc163, 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 undoes a previous attempt to fix issues related to duplicate unique identifiers in table measure row column headers. The original solution involved a dedicated utility file for filtering props and preparing titles, which has now been removed. The current change simplifies the logic by directly handling React element cloning 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
|
Walkthrough在 MeasureRow 中用内联逻辑替代 prepareMeasureTitle；删除 src/utils/measureUtil.ts（包含 filterMeasureProps 与 prepareMeasureTitle）。MeasureRow 现在在运行时判断列标题是否为 React 元素并按需 clone（移除 ref），否则直接使用原值。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Table
participant MeasureRow
participant React
Table->>MeasureRow: render(columns)
loop For each column
MeasureRow->>MeasureRow: rawTitle = column.title
alt rawTitle 是 React 元素
MeasureRow->>React: cloneElement(rawTitle, { ref: null })
React-->>MeasureRow: titleForMeasure(已移除 ref)
else 非元素
MeasureRow->>MeasureRow: titleForMeasure = rawTitle
end
end
MeasureRow-->>Table: 渲染测量行（不再使用 filterMeasureProps/prepareMeasureTitle）
note over MeasureRow: 已移除 measureUtil 依赖与递归过滤
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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
This pull request reverts a previous fix that addressed an issue with duplicate unique identifiers in MeasureRow column headers. By reverting this change, the application will once again be susceptible to creating duplicate DOM element IDs when column titles are complex React components. This can lead to invalid HTML and issues with accessibility and testing. I have added a comment highlighting this regression and questioning the rationale behind the revert, as it appears to re-introduce a known bug.
| const titleForMeasure = React.isValidElement<React.RefAttributes<any>>(rawTitle) | ||
| ? React.cloneElement(rawTitle, { ref: null }) | ||
| : rawTitle; |
There was a problem hiding this comment.
This revert re-introduces an issue where unique identifiers in column titles (like id, data-testid, etc.) can be duplicated in the DOM. The MeasureRow is a hidden element used for measuring, and if it renders a column title that is a React component with such identifiers, it will create duplicate IDs. This is invalid HTML and can cause problems with accessibility and testing frameworks.
The previous implementation that is being removed (prepareMeasureTitle from the now-deleted measureUtil.ts) correctly stripped these problematic props. By reverting to only nullifying the ref, the original bug is being brought back.
If there was a specific problem with the previous fix that prompted this revert, it would be beneficial to document it. Otherwise, it would be best to avoid re-introducing this bug.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1377 +/- ##
==========================================
- Coverage 96.10% 96.09% -0.01%
==========================================
Files 58 57 -1
Lines 3492 3434 -58
Branches 640 632 -8
==========================================
- Hits 3356 3300 -56
+ Misses 131 129 -2
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(1 hunks)src/utils/measureUtil.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils/measureUtil.ts
⏰ 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
| const titleForMeasure = React.isValidElement<React.RefAttributes<any>>(rawTitle) | ||
| ? React.cloneElement(rawTitle, { ref: null }) | ||
| : rawTitle; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
请说明回滚原因并验证不会重新引入原 bug。
此 PR 回滚了修复重复唯一标识符的 #1376。建议：
- 在 PR 描述中说明回滚的原因（原修复是否引入了其他问题？）
- 验证回滚后不会重新引入原来的重复 ID 问题
- 确认当前的内联逻辑能够正确处理所有边缘情况
另外，类型注解 React.RefAttributes<any> 使用了 any，建议改为更具体的类型以提高类型安全性。
运行以下脚本搜索 PR #1376 的相关讨论和测试用例：
🏁 Script executed:
#!/bin/bash
# 查找 PR #1376 的相关信息和测试
gh pr view 1376 --json title,body,comments
# 搜索相关的测试文件
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | xargs rg -l "MeasureRow|duplicate.*id" Length of output: 22036
澄清回滚原因并验证不再引入重复唯一标识符问题
建议：
- 在 PR 描述中说明此次回滚的具体原因（例如
measureUtil对DashboardColumnTitle等自定义组件内部的id/data-*属性无法递归清理）并规划后续解决方案。 - 确认目前的内联逻辑已涵盖
prepareMeasureTitle原有的递归过滤能力，并为嵌套组件场景添加单元测试。 - 将
React.RefAttributes<any>替换为更精确的类型（如JSX.Element或自定义泛型），提升类型安全。
1 participant
Reverts #1376
Summary by CodeRabbit