[refactor] Give table consumers class hooks that outlive antd - #5960
[refactor] Give table consumers class hooks that outlive antd#5960ardaerzin wants to merge 1 commit into
Conversation
Step 4 of the table port. App code styling a table reached into antd's DOM (`[&_.ant-table-thead_tr:nth-child(2)]:hidden` and friends), so replacing the render leaf would silently stop that styling from applying, with no type error and no failing test to catch it. The package now stamps its own hooks and app code targets those instead: avt-table, avt-container, avt-body, avt-thead, avt-row, avt-cell, avt-head-cell. Structural nodes are stamped once on mount. Rows and cells cannot be, because virtualization recycles them, so they get their class through rowClassName and through the column adapter, which is already the one place columns cross into antd. tableDom.ts is now the only file in the directory that spells an antd table selector. The hooks that read the table's DOM go through its map, so the leaf swap edits one file rather than six. Consumers migrated: the observability traces and sessions tables, the agents and prompts sections, and useCellVisibility. globals.css keeps its antd selector alongside the new one, since raw <Table> call sites still rely on it. Left for step 3, deliberately: the wrapper-scoped blocks in globals.css, evaluations.css and human-evals.css (.comparison-table, .agenta-testsets-table, .org-domains-table and the rest). They are structural, they carry real visual risk, and most of them belong to raw antd tables that keep .ant-table-* anyway. The contract is pinned by unit tests rather than left to a browser pass, since a broken hook shows up as styling that quietly stops applying.
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
refactoring
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe virtual table adds stable AVT DOM hooks, centralizes Ant Design selectors, applies hooks to table structure, rows, cells, and headers, and updates OSS table consumers to use the new selectors. ChangesVirtual table DOM contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The change is mergeable with owner awareness: keep implementation-specific selectors private and ensure table styling hooks are present before the first client frame to avoid a brief visual regression; the remaining performance follow-up is non-blocking. Sequence Diagram(s)sequenceDiagram
participant InfiniteVirtualTableInner
participant stampTableDom
participant toAntdColumns
participant AntDesignTable
participant OSSTableConsumers
InfiniteVirtualTableInner->>stampTableDom: stamp root, container, body, and header hooks
InfiniteVirtualTableInner->>toAntdColumns: transform column definitions
toAntdColumns->>AntDesignTable: provide AVT cell and header classes
InfiniteVirtualTableInner->>AntDesignTable: provide AVT row and root classes
OSSTableConsumers->>AntDesignTable: query and style AVT table elements
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
Railway Preview Environment
|
|
@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
🧹 Nitpick comments (1)
web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsx (1)
755-755: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the adapted column tree.
toAntdColumns(finalColumns)maps every column and creates newonCellandonHeaderCellcallbacks on every render. Ant Design then receives a newcolumnsvalue even whenfinalColumnsis unchanged. Memoize this conversion before passing it toTable.Proposed fix
+ const antdColumns = useMemo(() => toAntdColumns(finalColumns), [finalColumns]) + return ( @@ - columns={toAntdColumns(finalColumns)} + columns={antdColumns}As per coding guidelines, minimize React re-renders with
useMemoand avoid unstable objects in virtualized lists.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 50095df4-0436-4fd2-a2ee-9f0b4dfbaa9d
📒 Files selected for processing (16)
web/oss/src/components/EvalRunDetails/hooks/useCellVisibility.tsweb/oss/src/components/pages/agents/AgentsTableSection.tsxweb/oss/src/components/pages/observability/components/ObservabilityTable/index.tsxweb/oss/src/components/pages/observability/components/SessionsTable/index.tsxweb/oss/src/components/pages/prompts/components/PromptsTableSection.tsxweb/oss/src/styles/globals.cssweb/packages/agenta-entity-ui/tests/unit/tableClassHooks.test.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/antdColumns.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useColumnDomRefs.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useScrollConfig.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useScrollContainer.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableHeaderHeight.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/index.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/tableDom.ts
| // Stable class hooks for app code, so a consumer's selector does not depend on antd's DOM. | ||
| // The structural nodes exist for the table's lifetime; rows and cells get theirs from | ||
| // rowClassName and the column adapter, because virtualization recycles them. | ||
| useEffect(() => { | ||
| stampTableDom(containerRef.current) | ||
| }, [dataSource]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsx"
printf '%s\n' '--- outline ---'
ast-grep outline "$file" --lang tsx | head -200
printf '%s\n' '--- imports and relevant source ---'
sed -n '1,120p' "$file"
sed -n '640,790p' "$file"
printf '%s\n' '--- relevant symbols ---'
rg -n -C 5 'stampTableDom|toAntdColumns|use(Layout)?Effect|containerRef|finalColumns' "$file"Repository: Agenta-AI/agenta
Length of output: 22377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate tableDom files ---'
fd -i 'tableDom' web/packages/agenta-ui
printf '%s\n' '--- tableDom symbols and implementation ---'
rg -n -C 8 'stampTableDom|AVT|ANTD_SELECTOR' web/packages/agenta-ui
printf '%s\n' '--- column adapter ---'
fd -i 'antdColumns' web/packages/agenta-ui
rg -n -C 5 'function toAntdColumns|const toAntdColumns|export .*toAntdColumns' web/packages/agenta-ui
printf '%s\n' '--- package metadata and test references ---'
fd -i 'package.json|test|spec' web/packages/agenta-ui | head -100
rg -n -C 4 'stampTableDom|avt-container|avt-body|avt-thead' web/packages/agenta-ui --glob '*.{ts,tsx,js,jsx,test,spec}'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- isomorphic layout-effect conventions ---'
rg -n -C 4 'useIsomorphicLayoutEffect|typeof window.*useLayoutEffect|useLayoutEffect.*typeof window' web --glob '*.{ts,tsx,js,jsx}'
printf '%s\n' '--- InfiniteVirtualTable usage ---'
rg -n -C 3 '<InfiniteVirtualTable|InfiniteVirtualTable\(' web --glob '*.{ts,tsx,js,jsx}'
printf '%s\n' '--- consumers of structural AVT hooks ---'
rg -n -C 3 'avt-container|avt-body|avt-thead|AVT\.container|AVT\.body|AVT\.header' . --glob '*.{ts,tsx,css,less,scss,js,jsx}'
printf '%s\n' '--- package metadata ---'
cat web/packages/agenta-ui/package.jsonRepository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsx")
text = path.read_text()
effect = re.search(
r"useEffect\(\(\) => \{\s*stampTableDom\(containerRef\.current\)\s*\}, \[dataSource\]\)",
text,
)
layout_effect = re.search(
r"useLayoutEffect\(\(\) => \{\s*stampTableDom\(containerRef\.current\)\s*\}, \[dataSource\]\)",
text,
)
dom = Path("web/packages/agenta-ui/src/InfiniteVirtualTable/tableDom.ts").read_text()
selectors = re.findall(r"\[ANTD_SELECTOR\.(\w+), AVT\.(\w+)\]", dom)
print({
"passive_stamp_present": bool(effect),
"layout_stamp_present": bool(layout_effect),
"structural_mappings": selectors,
"layout_effect_already_imported": "useLayoutEffect" in text.split("from \"react\"", 1)[0],
})
PYRepository: Agenta-AI/agenta
Length of output: 350
🌐 Web query:
React official documentation useEffect after paint useLayoutEffect before browser repaint server rendering warning
💡 Result:
The fundamental difference between useEffect and useLayoutEffect lies in their execution timing relative to the browser's painting process [1][2][3]. useEffect: - Runs asynchronously after the browser has painted the updated screen [1][2]. - It is the default choice for most side effects (e.g., data fetching, subscriptions, setting up event handlers) because it avoids blocking the browser from updating the user interface [1][4][3]. useLayoutEffect: - Runs synchronously after React performs all DOM mutations, but before the browser paints those changes to the screen [1][2][3]. - It effectively blocks the browser from repainting until the effect completes [1][2]. - It is intended for specific use cases, such as measuring DOM elements or performing visual mutations that must occur before the user sees the rendered output to prevent visible "flicker" [1][2][3]. Server Rendering Warning: React issues a warning when useLayoutEffect is used during server-side rendering because this hook cannot execute on the server [5][6]. On the server, React renders components to a string or stream without a DOM environment; therefore, the code inside useLayoutEffect is skipped [5][7]. The warning serves as a safeguard because useLayoutEffect is typically used to fix visual inconsistencies or measure layout [8][6]. If a component relies on this effect for correct rendering, the server-rendered HTML sent to the client will likely look incorrect or "broken" until the client-side JavaScript loads and the effect eventually runs [6][7]. To resolve this warning: 1. Prefer useEffect: If the logic does not require immediate, blocking layout adjustments, switch to useEffect [1][5]. 2. Conditional Rendering: If a component depends on client-only layout logic, render it conditionally so it only appears after the component has mounted on the client [8][5]. This can be achieved by using state (e.g., initialized to false) that is set to true within a useEffect hook [8][5]. This prevents the "broken" UI from being displayed in the initial server-rendered HTML [6][7].
Citations:
- 1: https://react.dev/reference/react/useLayoutEffect
- 2: https://daveceddia.com/useeffect-vs-uselayouteffect/
- 3: https://kentcdodds.com/blog/useeffect-vs-uselayouteffect
- 4: https://legacy.reactjs.org/docs/hooks-reference.html
- 5: https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85
- 6: https://kevinyank.com/posts/useeffect-vs-uselayouteffect-and-ssr/
- 7: https://handlewithcare.dev/blog/no_such_thing_isomorphic_layout_effect/
- 8: useLayoutEffect in ssr react/react#14927
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package-local isomorphic hook and table entry point ---'
sed -n '1,100p' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useContainerResize.ts
sed -n '1,90p' web/packages/agenta-ui/src/InfiniteVirtualTable/InfiniteVirtualTable.tsx
printf '%s\n' '--- package-level server/client markers and Next integration ---'
rg -n -C 2 '"use client"|next/|dynamic\(' web/packages/agenta-ui web/oss/src --glob '*.{ts,tsx,js,jsx}' | head -240
printf '%s\n' '--- focused table class-hook tests ---'
sed -n '1,100p' web/packages/agenta-entity-ui/tests/unit/tableClassHooks.test.tsRepository: Agenta-AI/agenta
Length of output: 26811
Stamp structural hooks before the first paint.
useEffect can let the first client frame render without avt-container, avt-body, or avt-thead, so consumer selectors may not apply. Use a client/server-safe layout effect: useLayoutEffect on the client and useEffect during SSR.
| ColumnSorterConfig, | ||
| } from "./columnDef" | ||
| export {toAntdColumns, fromAntdColumns} from "./antdColumns" | ||
| export {AVT, ANTD_SELECTOR, stampTableDom, type AvtClass} from "./tableDom" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep ANTD_SELECTOR internal.
Line 140 exposes Ant Design DOM selectors as a public API. web/packages/agenta-ui/src/InfiniteVirtualTable/tableDom.ts states that app code must use AVT and that ANTD_SELECTOR is an implementation detail. Remove this export so consumers cannot couple to the DOM that this PR intends to replace.
Proposed fix
-export {AVT, ANTD_SELECTOR, stampTableDom, type AvtClass} from "./tableDom"
+export {AVT, stampTableDom, type AvtClass} from "./tableDom"📝 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.
| export {AVT, ANTD_SELECTOR, stampTableDom, type AvtClass} from "./tableDom" | |
| export {AVT, stampTableDom, type AvtClass} from "./tableDom" |
1 participant
Context
App code that styles a table reaches into antd's DOM. The observability traces table hides a header row with
[&_.ant-table-thead_tr:nth-child(2)]:hidden, the sessions table aligns body cells with[&_.ant-table-tbody_.ant-table-cell]:align-top, and four more places do the same.<Table virtual>is going to be replaced (docs/design/observability-packages/plan.md§8 step 3). When it is, every one of those selectors stops matching. Nothing catches that: no type error, no failing test, just styling that quietly stops applying.Step 4 of that plan exists to remove the trap before the swap, and it is scheduled ahead of the swap for exactly this reason. This PR does it.
Changes
The table stamps its own class hooks, and app code targets those:
Structural nodes are stamped once on mount, since they live as long as the table does. Rows and cells cannot be stamped that way, because virtualization recycles them. Rows get their class through
rowClassName(merged with whatever the caller passed) and cells through the column adapter, which was already the one place columns cross into antd.Before, in
ObservabilityTable:After:
tableDom.tsis now the only file in the directory that spells an antd table selector. The six hooks that read the table's DOM go through its map, so step 3 edits one file instead of hunting selectors across the package.Consumers migrated: the observability traces and sessions tables, the agents and prompts table sections, and
useCellVisibility.globals.csskeeps its antd selector next to the new one, because raw<Table>call sites still rely on it.Tests / notes
@agenta/entity-ui: the structural stamp, the missing-node and null cases, cell hooks on a plain column, merging with a column's ownonCellprops, and reaching columns nested in a group. A broken hook is invisible at runtime, so a test is worth more here than a browser pass.@agenta/entity-uiis now 330 tests, up from 325.@agenta/uiand OSS both typecheck;@agenta/ui,@agenta/entity-uiand the touched OSS files lint clean.AVTandstampTableDomare exported from@agenta/ui/table, so app code can reference the names rather than hardcode them.globals.css,evaluations.cssandhuman-evals.css(.comparison-table,.agenta-testsets-table,.org-domains-table,.no-expand-coland the rest). They are structural, they carry real visual risk, and most belong to raw antd tables that keep.ant-table-*regardless. They are step-3 work.What to QA
No visual change is expected. The risk is a rule that stops applying, so check the four migrated surfaces.