Skip to content

fix: duplicate unique identifiers in MeasureRow column headers #1378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: duplicate unique identifiers in MeasureRow column headers #1378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e69e7c3
fix: duplicate unique identifiers in MeasureRow column headers
Oct 9, 2025
a8636fb
fix: Nullify ref for filteredProps
Oct 9, 2025
1fdabd9
update: remove Attributes that should be removed
Oct 9, 2025
afc8cff
update: cleanMeasureCell
Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/Body/MeasureCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import ResizeObserver from '@rc-component/resize-observer';
import useLayoutEffect from '@rc-component/util/lib/hooks/useLayoutEffect';
import { cleanMeasureRowAttributes } from '../utils/measureUtil';

export interface MeasureCellProps {
columnKey: React.Key;
Expand All @@ -12,20 +13,27 @@ const MeasureCell: React.FC<MeasureCellProps> = props => {
const { columnKey, onColumnResize, title } = props;

const cellRef = React.useRef<HTMLTableCellElement>(null);
const contentRef = React.useRef<HTMLDivElement>(null);

useLayoutEffect(() => {
if (cellRef.current) {
onColumnResize(columnKey, cellRef.current.offsetWidth);
}
}, []);

if (contentRef.current) {
cleanMeasureRowAttributes(contentRef.current);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果里面有异步逻辑，比如依赖里面的 id 或者其他属性的做 dom 操作，这样移除后会可能会引发报错。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果里面有异步逻辑，比如依赖里面的 id 或者其他属性的做 dom 操作，这样移除后会可能会引发报错。

如果真的依赖id做操作，操作显示的那个dom才是正常的吧；这个dom只是用于测量宽度，实际不可见，反而不去除这些属性就可能有两个id一样的dom

}, [title, columnKey, onColumnResize]);
Comment on lines +22 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

清理逻辑不完整，且存在性能隐患。

存在以下问题：

  1. 清理逻辑不完整：cleanMeasureRowAttributes 的实现（位于 src/utils/measureUtil.ts）仅使用 querySelectorAll('*') 清理后代元素，但不会清理 contentRef 元素本身的属性。如果被克隆的 title 在根元素上包含 id 或 data-testid，这些属性将不会被移除，导致重复 ID 问题依然存在。

  2. 依赖项可能导致性能问题：onColumnResize 函数包含在依赖数组中。如果父组件未对该函数进行记忆化处理（使用 useCallback），每次渲染都会创建新的函数引用，导致 effect 频繁执行，产生不必要的 DOM 遍历开销。

建议修复方案：

  1. 在 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);
    });
  });
}
  1. 建议在 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.


return (
<ResizeObserver data={columnKey}>
<td
ref={cellRef}
style={{ paddingTop: 0, paddingBottom: 0, borderTop: 0, borderBottom: 0, height: 0 }}
>
<div style={{ height: 0, overflow: 'hidden', fontWeight: 'bold' }}>{title || '\xa0'}</div>
<div ref={contentRef} style={{ height: 0, overflow: 'hidden', fontWeight: 'bold' }}>
{title || '\xa0'}
</div>
</td>
</ResizeObserver>
);
Expand Down
1 change: 1 addition & 0 deletions src/Body/MeasureRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const MeasureRow: React.FC<MeasureRowProps> = ({
const titleForMeasure = React.isValidElement<React.RefAttributes<any>>(rawTitle)
? React.cloneElement(rawTitle, { ref: null })
: rawTitle;

return (
<MeasureCell
key={columnKey}
Expand Down
31 changes: 31 additions & 0 deletions src/utils/measureUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* Attributes that should be removed from measure row DOM to avoid conflicts
*/
const FILTERED_ATTRIBUTES = [
// Unique identifiers that shouldn't be duplicated in DOM
'id',
'data-testid',
'data-test-id',
'data-cy', // Cypress
'data-qa',
'data-automation-id',
'data-id',
'data-key',
] as const;

/**
* Remove all ID and test attributes from DOM element and its descendants
* This ensures the measure row complies with HTML spec (no duplicate IDs)
* and works with custom components whose internal DOM we cannot control at React level
* @param element - The DOM element to clean
*/
export function cleanMeasureRowAttributes(element: HTMLElement): void {
if (!element) return;

const allElements = element.querySelectorAll('*');
allElements.forEach(el => {
FILTERED_ATTRIBUTES.forEach(attr => {
el.removeAttribute(attr);
});
});
Comment on lines +25 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

当前使用 element.querySelectorAll('*') 的实现只选择了后代元素，而没有包含 element 本身。如果传递给此函数的根元素自身也包含需要过滤的属性，这些属性将不会被移除。为了使此函数更加健壮，它也应该清理根元素上的属性。

  [element, ...element.querySelectorAll('*')].forEach(el => {
    FILTERED_ATTRIBUTES.forEach(attr => {
      el.removeAttribute(attr);
    });
  });

}
Comment on lines +22 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

函数未清理根元素本身的属性。

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.