[refactor] The table's peripheral chrome comes off antd - #5961
Open
ardaerzin wants to merge 1 commit into
Open
Conversation
Step 2 of the table port. Everything in the package except the render leaf was still antd: Typography, Skeleton, Tooltip, Popover, Dropdown, Checkbox and Grid.useBreakpoint across nine files. The swaps are onto the primitives WP3 established. Grid.useBreakpoint becomes useIsNarrowScreen, a matchMedia hook on antd's own lg breakpoint, which is all the two call sites ever asked it for. antd's MenuProps becomes TableMenuItem, a plain descriptor the menu-building hooks return and one renderer turns into dropdown-menu parts. That also drops the antd-shaped onClick(e.domEvent) from the package's public ActionItem, which no consumer was using. This fixes a live bug on the way. The column-visibility gear did nothing when clicked: its trigger called preventDefault, and a default-prevented event makes both antd and Radix skip the open handler composed onto the same element. Stopping propagation is what that handler actually wanted, so the header cell does not sort out from under the click. Two antd components stay, and neither is a swap that WP3 established: - Table, which is step 3 and the whole point of the sequence. - Pagination, which has no @agenta/ui equivalent. Building one means page numbers, ellipsis and a total, which is new UI and wants a design pass, not a 4am hand-roll. One page renders paginationMode="paginated". Also deferred: the checkable Tree inside ColumnVisibilityPopoverContent. It needs a checkable-tree primitive that does not exist, so the same argument applies.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:L
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds shared table menu and media-query primitives, then migrates InfiniteVirtualTable and related OSS components from Ant Design menu, tooltip, popover, checkbox, typography, skeleton, breakpoint, and tabs APIs. ChangesInfinite table UI migration
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The refactor is broadly mergeable, but it leaves two bounded UI issues: table descriptions may have extra vertical spacing, and some composed controls may not receive refs correctly under React 18, which can affect trigger behavior or accessibility. These should have explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant TableUser
participant InfiniteVirtualTableFeatureShell
participant renderTableMenuItems
participant TableAction
TableUser->>InfiniteVirtualTableFeatureShell: open a table menu
InfiniteVirtualTableFeatureShell->>renderTableMenuItems: pass TableMenuItem[]
renderTableMenuItems->>TableAction: invoke the selected callback
TableAction->>InfiniteVirtualTableFeatureShell: update action state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
Railway Preview Environment
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 544bbb51-8943-4f05-a0ba-47f4465c4faf
📒 Files selected for processing (16)
web/oss/src/components/EvalRunDetails/hooks/useRowHeightMenuItems.tsxweb/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/columns/createStandardColumns.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/ColumnVisibilityTrigger.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/TableDescription.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/columnVisibility/TableSettingsDropdown.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/common/ResizableTitle.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/features/InfiniteVirtualTableFeatureShell.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useRowHeight.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useRowHeightFeature.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTypeChipFeature.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/index.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/tableMenu.tsxweb/packages/agenta-ui/src/hooks/index.tsweb/packages/agenta-ui/src/hooks/useMediaQuery.ts
Comment on lines
+116
to
+120
| {/* Tooltip outside, popover trigger inside: each `asChild` clones down to the same | ||
| button, so both sets of handlers compose onto it. */} | ||
| <SimpleTooltip title={variant === "icon" ? label : undefined}> | ||
| <PopoverTrigger asChild>{triggerNode}</PopoverTrigger> | ||
| </SimpleTooltip> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'function (DropdownMenuTrigger|PopoverTrigger)' \
web/packages/agenta-ui/src/components/ui/{dropdown-menu,popover}.tsx
rg -n -C 4 'SimpleTooltip|DropdownMenuTrigger asChild|PopoverTrigger asChild' \
web/packages/agenta-ui/src/InfiniteVirtualTable/{columns/createStandardColumns.tsx,components/ColumnVisibilityTrigger.tsx,components/columnVisibility/TableSettingsDropdown.tsx}
fd -a '^package.json$' web -x rg -n '"react"\s*:' {}Repository: Agenta-AI/agenta
Length of output: 14167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package/runtime declarations ---'
fd -a '^package.json$' web -x sh -c 'printf "\n%s\n" "$1"; rg -n -C 2 "\"(react|react-dom|`@radix-ui/react-`(tooltip|popover|dropdown-menu))\"" "$1"' sh {}
printf '%s\n' '--- tooltip implementation and primitive imports ---'
fd -a 'tooltip-composed.tsx|tooltip.tsx|button.tsx|popover.tsx|dropdown-menu.tsx' web/packages/agenta-ui/src -x sh -c 'printf "\n### %s\n" "$1"; sed -n "1,90p" "$1"' sh {}
printf '%s\n' '--- trigger declarations and call sites ---'
rg -n -C 5 'function (DropdownMenuTrigger|PopoverTrigger)|TooltipTrigger|SimpleTooltip' \
web/packages/agenta-ui/src/components/ui \
web/packages/agenta-ui/src/InfiniteVirtualTable/{columns/createStandardColumns.tsx,components/ColumnVisibilityTrigger.tsx,components/columnVisibility/TableSettingsDropdown.tsx}Repository: Agenta-AI/agenta
Length of output: 8043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
web/packages/agenta-ui/src/components/ui/tooltip-composed.tsx \
web/packages/agenta-ui/src/components/ui/tooltip.tsx \
web/packages/agenta-ui/src/components/ui/popover.tsx \
web/packages/agenta-ui/src/components/ui/dropdown-menu.tsx \
web/packages/agenta-ui/src/components/ui/button.tsx
do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
cat -n "$f" | sed -n '1,180p'
fi
done
printf '\n--- resolved React versions in lockfiles ---\n'
rg -n -C 2 '(^|/)react(@|:)|react-dom(@|:)|`@radix-ui/react-`(tooltip|popover|dropdown-menu)' \
web/pnpm-lock.yaml web/**/package.json 2>/dev/null | head -240 || true
printf '\n--- all local trigger wrapper definitions ---\n'
rg -n -C 3 'function (DropdownMenuTrigger|PopoverTrigger)|const (DropdownMenuTrigger|PopoverTrigger)' \
web/packages/agenta-ui/src web/packages 2>/dev/null | head -160 || trueRepository: Agenta-AI/agenta
Length of output: 40157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("web/packages/agenta-ui")
package = (root / "package.json").read_text()
popover = (root / "src/components/ui/popover.tsx").read_text()
dropdown = (root / "src/components/ui/dropdown-menu.tsx").read_text()
sites = [
root / "src/InfiniteVirtualTable/components/ColumnVisibilityTrigger.tsx",
root / "src/InfiniteVirtualTable/columns/createStandardColumns.tsx",
root / "src/InfiniteVirtualTable/components/columnVisibility/TableSettingsDropdown.tsx",
]
peer_supports_react18 = bool(re.search(r'"react"\s*:\s*">=18\.0\.0"', package))
wrappers_forward_refs = {
"PopoverTrigger": "forwardRef" in popover and "function PopoverTrigger" not in popover,
"DropdownMenuTrigger": "forwardRef" in dropdown and "function DropdownMenuTrigger" not in dropdown,
}
print(f"agenta-ui peer supports React 18: {peer_supports_react18}")
for name, value in wrappers_forward_refs.items():
print(f"{name} forwards a ref: {value}")
for path in sites:
text = path.read_text()
uses = ("<SimpleTooltip" in text and
("<PopoverTrigger asChild>" in text or "<DropdownMenuTrigger asChild>" in text))
print(f"{path}: composes SimpleTooltip with a local trigger wrapper: {uses}")
PYRepository: Agenta-AI/agenta
Length of output: 720
Forward refs through composed tooltip triggers.
agenta-ui supports React 18, but PopoverTrigger and DropdownMenuTrigger do not forward refs. In React 18, SimpleTooltip cannot pass its trigger ref to the underlying button. Update both shared wrappers to use React.forwardRef; this fixes all three affected call sites.
📍 Affects 3 files
web/packages/agenta-ui/src/InfiniteVirtualTable/components/ColumnVisibilityTrigger.tsx#L116-L120(this comment)web/packages/agenta-ui/src/InfiniteVirtualTable/columns/createStandardColumns.tsx#L587-L605web/packages/agenta-ui/src/InfiniteVirtualTable/components/columnVisibility/TableSettingsDropdown.tsx#L133-L145
Comment on lines
+38
to
+44
| <p | ||
| className={cn( | ||
| "mb-0 text-colorTextSecondary", | ||
| maxWidthClass, | ||
| "line-clamp-2 h-10", | ||
| className, | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the native paragraph top margin.
Line 40 resets only the bottom margin. The native p top margin returns because this app disables Tailwind preflight. This adds vertical space before every table description.
Proposed fix
- "mb-0 text-colorTextSecondary",
+ "m-0 text-colorTextSecondary",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p | |
| className={cn( | |
| "mb-0 text-colorTextSecondary", | |
| maxWidthClass, | |
| "line-clamp-2 h-10", | |
| className, | |
| )} | |
| <p | |
| className={cn( | |
| "m-0 text-colorTextSecondary", | |
| maxWidthClass, | |
| "line-clamp-2 h-10", | |
| className, | |
| )} |
ardaerzin
added a commit
that referenced
this pull request
Aug 13, 2026
Diagnosed but not fixed. The console stack shows composeRefs recursing through setRef into dispatchSetState, which is a composed ref callback rebuilt every render, not a data problem. Prime suspect is the nested asChild pair in ColumnVisibilityTrigger — SimpleTooltip around PopoverTrigger, both collapsing onto one button — which CodeRabbit independently flagged on #5961. The plan leads with confirming the diagnosis in a unit test before touching anything, since a stack trace is a hypothesis, and records the trap that the component throws a TypeError without a controls prop, which reads like a pass.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Context
@agenta/ui's table package was antd well beyond the<Table>it renders. Nine files pulled inTypography,Skeleton,Tooltip,Popover,Dropdown,CheckboxandGrid.useBreakpoint, so "the table depends on antd" was much bigger than the one component that actually needs replacing.Step 2 of
docs/design/observability-packages/plan.md§8 shrinks that to the render leaf, which is what makes step 3 a single-file project.Changes
The swaps go onto the primitives WP3 established. Two are worth calling out.
Grid.useBreakpoint()→useIsNarrowScreen(). Both call sites only ever read!screens.lg, so the replacement is a smallmatchMediahook on antd's ownlgvalue (992px), living in@agenta/ui/hookswhere mobile can use it too.MenuProps["items"]→TableMenuItem[]. The hooks that build menus now return a plain descriptor and one renderer turns it intodropdown-menuparts:That also drops the antd-shaped
onClick(e.domEvent)from the package's publicActionItem. No consumer used the event argument. Stopping propagation is the renderer's job now, so an item'sonClicktakes nothing.A live bug goes with it. The column-visibility gear did nothing when clicked. Its trigger called
preventDefault(), and a default-prevented event makes antd and Radix both skip the open handler composed onto the same element. What that handler wanted wasstopPropagation(), so the header cell does not sort out from under the click. It now does only that, and the gear opens.Tests / notes
@agenta/uitypecheck.@agenta/uilints clean;@agenta/entity-uiis 330 tests green.settingsDropdownMenuItems(EvalRunDetailsandTestcasesTableNew); both now buildTableMenuItem[].Tableis step 3 and the point of the sequence.Paginationhas no@agenta/uiequivalent, and building one means page numbers, ellipsis and a total count, which is new UI that wants a design pass rather than an invented primitive. One page (ApplicationManagementSection) renderspaginationMode="paginated".TreeinsideColumnVisibilityPopoverContent, for the same reason — there is no checkable-tree primitive to swap onto. Its siblingColumnVisibilityTriggeruses a recursive checkbox list and is ported here, so the pattern for a later port already exists.What to QA
Every change is a visual swap, so the risk is a control that looks or behaves subtly differently.
mainthe icon does nothing on several tables.⋯actions menu: items fire, destructive items are red, dividers sit between groups and never at an edge.