You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Migrates column bulk operation component to Untitled UI with several issues unresolved: fragile button selector for expand, services search using keyword wildcard instead of full-text query, and getAllDescendantIds lacking cycle guard causing potential infinite loops and O(n²) rescanning. Five findings were addressed including keyboard event propagation and dead props removal.
The search parameter was removed from getServiceDetails calls and is no longer passed to searchService. This means the Elasticsearch q parameter defaults to * (wildcard match-all), and the search term is applied exclusively via queryFilter as keyword wildcard clauses (*term* on name.keyword and displayName.keyword).
This is a behavioral regression: previously, the search term was passed as the q parameter for analyzed full-text matching (which handles tokenization, partial words, etc.). Now only exact substring matches on keyword fields will work. For example, searching for 'mysql' won't match a service named 'My MySQL Production Service' stored in a non-keyword field.
Additionally, keyword fields are case-sensitive by default in Elasticsearch, so searching 'mysql' won't find 'MySQL' unless the field has a normalizer configured.
⚠️Bug:getAllDescendantIds has no cycle guard — can infinite loop
The BFS in getAllDescendantIds (line 1445) has no visited set. If structParentId references ever form a cycle (e.g., due to a data bug), the queue will grow unboundedly, hanging the browser tab. This is a defensive programming issue — even if cycles shouldn't exist in the data model, a single corrupt row would freeze the UI.
Suggested fix
Add a `visited` Set to break cycles:
```tsx
const getAllDescendantIds = useCallback(
(parentId: string): string[] => {
const allRows = columnGridListing.allRows;
const result: string[] = [parentId];
const visited = new Set<string>([parentId]);
const queue = [parentId];
while (queue.length > 0) {
const current = queue.shift() as string;
const children = allRows.filter((r) => r.structParentId === current);
for (const c of children) {
if (!visited.has(c.id)) {
visited.add(c.id);
result.push(c.id);
queue.push(c.id);
}
}
}
return result;
},
[columnGridListing.allRows]
);
```
💡 Quality: Fragile getByRole('button').first() selector for expand
The expand-button locator was changed from button.expand-button (a specific class) to structRow.getByRole('button').first(). This silently picks whatever button happens to be first in the row. If the row layout changes (e.g., a selection or action button is added before the expand toggle), the test will click the wrong element and fail with a confusing error. Consider adding a data-testid to the expand button in the component code and using getByTestId here instead.
💡 Performance:getAllDescendantIds rescans allRows per BFS node — O(n²)
getAllDescendantIds calls allRows.filter((r) => r.structParentId === current) for every node in the BFS queue. For a table with N columns, each BFS step is O(N), making the total O(N²) per call. This function is then called inside forEach loops over selected entities (lines 1638, 1664), compounding the cost. For tables with hundreds of columns this could cause noticeable UI lag during selection changes.
Suggested fix
Build a parentId→children lookup map once:
```tsx
const childrenMap = useMemo(() => {
const map = new Map<string, string[]>();
for (const r of columnGridListing.allRows) {
if (r.structParentId) {
const arr = map.get(r.structParentId) ?? [];
arr.push(r.id);
map.set(r.structParentId, arr);
}
}
return map;
}, [columnGridListing.allRows]);
```
Then use `childrenMap.get(current) ?? []` inside the BFS instead of `.filter()`.
✅ 5 resolved✅ Bug: onClick on column-name Buttons may not stop propagation for keyboard
📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:932📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:1067
Two column-name <Button> components use the native onClick prop (lines 932 and 1067) instead of react-aria's onPress. While mouse clicks are intercepted by react-aria's internal usePress and won't bubble to the parent Table.Row, keyboard activations (Enter/Space) fire a synthetic click event that may propagate differently, potentially toggling row selection unintentionally when the user keyboard-activates the column name link.
The expand buttons in the same file correctly use onPress. These two call sites should be consistent.
✅ Quality: Dead props passed to ColumnGridTableRow should be removed
📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/components/ColumnGridTableRow.tsx:54📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/components/ColumnGridTableRow.tsx:59📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/components/ColumnGridTableRow.tsx:60 isIndeterminate, onSelect, and onGroupSelect are still declared in the ColumnGridTableRowProps interface and passed from the parent ColumnGrid.component.tsx, but they are immediately destructured into underscore-prefixed unused variables (_isIndeterminate, _onSelect, _onGroupSelect). Now that the Table component handles selection natively, these props are dead code and should be cleaned up from both the interface and the call site to avoid confusion.
✅ Quality: ROW_BG_TRANSITION string used as className instead of style
📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/components/ColumnGridTableRow.tsx:43📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/components/ColumnGridTableRow.tsx:83
In ColumnGridTableRow.tsx, the constant ROW_BG_TRANSITION = 'background-color 0.4s ease-in-out' (a CSS transition value) is passed into classNames() as if it were a CSS class name (e.g., line 83). This produces an invalid class name string like tw:bg-utility-warning-50 background-color 0.4s ease-in-out which has no effect. The transition styling appears to be covered by the tw:transition-colors utility on the row class, making this usage a no-op at best. Either remove the ROW_BG_TRANSITION usages from classNames or convert it to a Tailwind class.
✅ Quality: "Discard changes" test no longer enters data before discarding
📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ColumnBulkOperations.spec.ts:725📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ColumnBulkOperations.spec.ts:760
The test should discard changes when closing drawer without saving previously had a step that opened the drawer and filled in a display name ('Temporary Display Name') before closing it. That step was removed in this commit, so the test now:
Selects a column
Presses Escape (no drawer is open at this point — only a checkbox was clicked)
Reopens the drawer and asserts displayNameValue is not 'Temporary Display Name'
This assertion will always pass trivially because no data was ever entered, making the test a no-op for its stated purpose. The test title says "discard changes" but it never creates changes to discard.
✅ Bug: Nesting indentation removed for struct child rows
📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:962📄 openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:329
The old code indented struct child rows based on their nesting level:
This was removed in the refactor and not replaced with any alternative. nestingLevel is still computed and stored on each row (lines 329, 341) but never used for visual rendering. This means deeply nested STRUCT columns (e.g., address.street.number) will all render flush-left with no visual hierarchy, making it impossible for users to understand parent-child relationships.
🤖 Prompt for agents
Code Review: Migrates column bulk operation component to Untitled UI with several issues unresolved: fragile button selector for expand, services search using keyword wildcard instead of full-text query, and `getAllDescendantIds` lacking cycle guard causing potential infinite loops and O(n²) rescanning. Five findings were addressed including keyboard event propagation and dead props removal.
1. 💡 Quality: Fragile `getByRole('button').first()` selector for expand
Files: openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ColumnBulkOperations.spec.ts:975, openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/ColumnBulkOperations.spec.ts:997
The expand-button locator was changed from `button.expand-button` (a specific class) to `structRow.getByRole('button').first()`. This silently picks whatever button happens to be first in the row. If the row layout changes (e.g., a selection or action button is added before the expand toggle), the test will click the wrong element and fail with a confusing error. Consider adding a `data-testid` to the expand button in the component code and using `getByTestId` here instead.
2. ⚠️ Edge Case: Services search uses keyword wildcard instead of full-text query
Files: openmetadata-ui/src/main/resources/ui/src/components/Settings/Services/Services.tsx:112, openmetadata-ui/src/main/resources/ui/src/components/Settings/Services/Services.tsx:117
The `search` parameter was removed from `getServiceDetails` calls and is no longer passed to `searchService`. This means the Elasticsearch `q` parameter defaults to `*` (wildcard match-all), and the search term is applied exclusively via `queryFilter` as keyword wildcard clauses (`*term*` on `name.keyword` and `displayName.keyword`).
This is a behavioral regression: previously, the search term was passed as the `q` parameter for analyzed full-text matching (which handles tokenization, partial words, etc.). Now only exact substring matches on keyword fields will work. For example, searching for 'mysql' won't match a service named 'My MySQL Production Service' stored in a non-keyword field.
Additionally, `keyword` fields are case-sensitive by default in Elasticsearch, so searching 'mysql' won't find 'MySQL' unless the field has a normalizer configured.
3. ⚠️ Bug: `getAllDescendantIds` has no cycle guard — can infinite loop
Files: openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:1445
The BFS in `getAllDescendantIds` (line 1445) has no `visited` set. If `structParentId` references ever form a cycle (e.g., due to a data bug), the `queue` will grow unboundedly, hanging the browser tab. This is a defensive programming issue — even if cycles shouldn't exist in the data model, a single corrupt row would freeze the UI.
Suggested fix:
Add a `visited` Set to break cycles:
```tsx
const getAllDescendantIds = useCallback(
(parentId: string): string[] => {
const allRows = columnGridListing.allRows;
const result: string[] = [parentId];
const visited = new Set<string>([parentId]);
const queue = [parentId];
while (queue.length > 0) {
const current = queue.shift() as string;
const children = allRows.filter((r) => r.structParentId === current);
for (const c of children) {
if (!visited.has(c.id)) {
visited.add(c.id);
result.push(c.id);
queue.push(c.id);
}
}
}
return result;
},
[columnGridListing.allRows]
);
```
4. 💡 Performance: `getAllDescendantIds` rescans allRows per BFS node — O(n²)
Files: openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:1452, openmetadata-ui/src/main/resources/ui/src/pages/ColumnBulkOperations/ColumnGrid/ColumnGrid.component.tsx:1638
`getAllDescendantIds` calls `allRows.filter((r) => r.structParentId === current)` for every node in the BFS queue. For a table with N columns, each BFS step is O(N), making the total O(N²) per call. This function is then called inside `forEach` loops over selected entities (lines 1638, 1664), compounding the cost. For tables with hundreds of columns this could cause noticeable UI lag during selection changes.
Suggested fix:
Build a parentId→children lookup map once:
```tsx
const childrenMap = useMemo(() => {
const map = new Map<string, string[]>();
for (const r of columnGridListing.allRows) {
if (r.structParentId) {
const arr = map.get(r.structParentId) ?? [];
arr.push(r.id);
map.set(r.structParentId, arr);
}
}
return map;
}, [columnGridListing.allRows]);
```
Then use `childrenMap.get(current) ?? []` inside the BFS instead of `.filter()`.
Options
Auto-apply is off → Gitar will not commit updates to this branch. Display: compact → Showing less information.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
safe to testAdd this label to run secure Github workflows on PRsUIUI specific issues
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
anuj-kumary
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Describe your changes:
Migrated the Column Bulk Operation component to Untitled UI and removed all usages of Material UI components from this module.
Screen.Recording.2026-03-13.at.1.12.17.PM.mov
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>