[feat] Mobile observability surface - #5963
Open
ardaerzin wants to merge 53 commits into
Open
Conversation
WP6. Traces and sessions on /m, project-wide, built entirely from the packages WP0-WP5 extracted. There is no mobile-only rendering of a trace: TracesList is the packaged list shell over the packaged trace row, so a change to how a span reads lands on both surfaces at once. The range control is the same ObservabilityRangePicker desktop renders, not a mobile sort sheet. The original plan called for one; the chrome conversion landed the shared control first precisely so this screen would not need it. That is the whole ordering argument, and this is where it pays. Sessions take the other path, deliberately. An observability session has no non-table rendering anywhere to extract, so mobile stacks the WP3 cells in a layout it owns rather than inventing a shared row for one caller. The cells stay the single source of formatting. This is option (a) of the plan's open design question; (b) remains a design ask and nothing here forecloses it. No scope binding: the seam's defaults are already project-wide with no workflow context, so binding would only re-state them. Deliberately out of scope on v1, each an explicit non-regression rather than a silent drop, and desktop keeps all of it: CSV export, bulk delete, add to testset, add to queue, column visibility and resize, custom date range.
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds mobile observability screens with trace and session tabs. Centralizes observability tables, export, and deletion workflows. Replaces the Ant Design virtual table path with a TanStack-based implementation. Adds tests, Storybook coverage, shared date-time imports, and navigation wiring. ChangesObservability platform
Virtual table engine
Shared package alignment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MobileUser
participant ObservabilityScreen
participant ObservabilityTracesTable
participant useObservability
participant DeleteTraceModal
MobileUser->>ObservabilityScreen: open observability route
ObservabilityScreen->>ObservabilityTracesTable: render traces tab
ObservabilityTracesTable->>useObservability: load traces and pagination
MobileUser->>ObservabilityScreen: select traces and request deletion
ObservabilityScreen->>DeleteTraceModal: open with selected trace IDs
DeleteTraceModal->>useObservability: delete traces and refresh data
DeleteTraceModal-->>ObservabilityScreen: close modal and clear selection
✨ 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
|
The shipping table always renders `virtual`, and in that mode antd emits .ant-table-tbody-virtual-holder INSTEAD of .ant-table-body. The stamp only looked for the latter, so avt-body was absent from every table in the app while the unit tests passed against a fake keyed to the same wrong selector. Found in the browser: six of the seven hooks were on the DOM and that one was not. The stamp gets its own selector rather than widening ANTD_SELECTOR.body, because useScrollContainer reads that key and has always fallen through to the container. Widening it there would change scroll detection, which is not what this fixes. The test now models the virtual table, and keeps a case for the non-virtual one so both paths stay covered.
/m crashed on load: "dayjs(...).utc is not a function", thrown from controls.ts while evaluating DEFAULT_SORT. state/index.ts did call dayjs.extend(utc), but controls.ts computes DEFAULT_SORT at module-evaluation time, so whether .utc() exists came down to which module the bundler evaluated first. Desktop happened to win that race; mobile did not, and the screen died before it rendered. Every file now imports dayjs from @agenta/shared/utils/dateTime, which extends utc and customParseFormat at its own module scope. The plugin is therefore guaranteed present by the time any consumer runs, and the ordering-dependent extend in state/index.ts is gone. Found by loading the mobile observability screen, which is the first consumer of this package outside oss.
I built and QA'd this screen phone-only. That is wrong: /m replaces web/oss and web/ee, so every screen has to hold up at desktop widths without drifting from the app it replaces. At 1600px it rendered as two stacked rows against 1450px of dead space, with no nav rail and no page title, because it never wrapped itself in AppShell the way every other screen here does. It now follows the same shape as SessionListScreen: PageTitle, AppShell for the persistent rail at lg+, ScreenScaffold, and the NavDrawer hamburger only below lg. The body runs the full content width rather than a centred column, because the desktop table runs edge to edge and a centred measure reads as a different page. The bigger drift was hand-rolled chrome. I had written a tabs+range header while ObservabilityToolbar already existed in the package, so mobile silently lost search, Root/LLM/All, realtime and auto-refresh. It renders the shared toolbar now. Export and delete stay hidden by omitting their handlers, which is how that component already expresses a capability the host does not offer — the v1 scope is a prop, not a fork. That exposed a real bug in the toolbar: its first row never wrapped, so on a 390px viewport the range picker and auto-refresh sat off the right edge at x=390. Wrapping is correct at any width and changes nothing where there is room, so it belongs in the component rather than in a mobile override. Still drifting, and out of reach here: the desktop renders a 10-column table where this renders stacked rows. Closing that needs the table itself off antd (§8 step 3), which is why the plan scheduled it after this.
The render leaf. `<Table virtual>` was the last antd component in the package and the reason /m could not show the same table the desktop shows, so /m got a stacked list that drifted from the app it is meant to replace. VirtualTable is plain table DOM plus row windowing: - a fixed row height and a scroll offset pick the visible slice, so only that slice plus an overscan mounts. Uniform rows make this arithmetic, which is why no windowing library is pulled in. - the header is its own table above the scroller, sharing a colgroup with the body so columns cannot drift, and scrolled in step horizontally. - pinned columns are position:sticky at a computed per-column offset. - it emits the same class hooks and data-column-key attributes the package's own hooks query, which is what the step-4 work made possible. getObservabilityColumns moves to @agenta/observability-ui. It turned out to be portable already: one type import and one relative import, both of which the package now owns. That is the ColumnDef seam paying off. Both OSS call sites point at the package. /m renders those columns on VirtualTable at lg+, and keeps the stacked rows below lg where a ten-column grid does not fit. Same columns, same cells, no antd in the bundle. 13 unit tests cover the windowing and the sticky offsets: the slice at rest, mid-scroll, both clamps, the no-height case, and pinned columns stacking from each edge. Those are the parts that fail silently — a wrong slice is blank rows mid-scroll, a wrong offset is overlapping columns.
dosubot
Bot
added
size:XL
The app this was built for has three rows of data, so the behaviours that only
appear at scale or in a corner were never exercised. These stories cover them
without needing a seeded project:
- Windowing: 10,000 rows, to check the mounted row count tracks the viewport
rather than the dataset. This is the part I could not verify in the app.
- StickyColumns: two left-pinned, one right-pinned, with a wide filler forcing
horizontal scroll.
- RowSelection: the leading checkbox column pinning left, select-all, per-row.
- MergedCells: the {props, children} render shape with colSpan 3 and colSpan 0,
which was coded and typed but had never rendered.
- RowInteraction: row click against a cell button that stops propagation.
- Empty, Basic.
- AntdComparison: the same columns and rows through <Table virtual> directly
above ours, so geometry and behaviour read side by side against the thing
being replaced.
Stories that hold state are components rather than render arrows; hooks in a
story arrow break rules-of-hooks.
Also fixes domain/InfiniteVirtualTable.stories.tsx, which still typed its
columns as antd ColumnsType. That was fallout from the ColumnDef seam that
nothing caught, because the storybook workspace is not in the apps' typecheck
path.
dosubot
Bot
added
size:XXL
…and-rolled code
You asked about TanStack Table, and measuring made the case better than my
reasoning had. I had hand-rolled the windowing arithmetic while
@tanstack/react-virtual was ALREADY a direct dependency of this package, and I
had written a ColumnDef type that duplicates TanStack's own.
The bigger point is where the weight sits. The rendering I wrote is ~300 lines.
The model logic already in this package is ~1,500:
useSmartResizableColumns 500 useColumnVisibility 286
useExpandableRows 284 useResizableColumns 221
useColumnVisibilityControls 98 useColumnDomRefs 79
useTableRowSelection 56 useScopedColumnVisibility 27
rc-table would have replaced the 300 and left the 1,500. TanStack Table is
headless — it ships no markup — so it replaces the 1,500 and leaves the markup,
which is the half that has to emit OUR avt-* contract and which no library can
do for us.
So: TanStack Table owns columns, visibility, sizing and selection; TanStack
Virtual owns windowing; this package owns the DOM.
tanstackColumns.ts is the whole migration cost. ColumnDef stays the shape all
82 call sites write — dataIndex / title / render — and the crossing happens in
one file, exactly as toAntdColumns did for antd. Swapping the engine must not
reach the callers, and it does not.
Virtual measures rows rather than assuming a fixed height, which removes the
uniform-row constraint that forced rowHeight={128} on /m.
Pinned to react-table v8: pnpm resolved ^9 by default, which is a rewritten API
(createCoreRowModel/ReactTable), and v8 is the mature one.
The tests move with the code. The windowing arithmetic they covered is
TanStack's problem now; what needs pinning is the adapter, so they cover
identity, plain and path accessors, a render-only column, width mapping, the
meta round-trip and group recursion.
…y found Storybook has been dying for the whole session inside webpack's FileSystemInfo._resolveContextTimestamp, hashing a context entry with no timestampHash. Bisecting the stories glob settled what it was not: with ZERO stories, and again with no addons, no docgen and no @/oss alias, a bare config still crashed. No story was ever involved. It was webpack 5.106.2. Four attempts to move it failed because pnpm 11 reads overrides from pnpm-workspace.yaml, not package.json — which is why every install answered "Already up to date". Retargeting the override there to 5.109.2 fixes it; the preview built first try. Storybook then paid for itself within a minute. The 10,000-row story mounted all 10,000: the scroller had style.height 420px but clientHeight 230000, because flex-1 beats an inline height. "flex: 1 1 0%" hands main-size calculation to the flex algorithm, which ignores "height", so the body grew to content and TanStack Virtual saw an unbounded viewport. flex-1 now applies only when no explicit height is given. 10,000 rows mount 27, and 35 at scrollTop 200000 starting from index 4175. That bug could not have been caught anywhere else: /m has three traces, so it never exceeds a viewport, and no typecheck or unit test can see a layout interaction between a Tailwind class and an inline style. All seven stories verified: windowing, sticky offsets (ID at 0, Name stacked at 200, Cost pinned right), selection wiring, merged cells (40 cells minus the 2 dropped by colSpan 0 = 38), row-click vs cell-button propagation, empty, and the antd comparison.
The new table had no `loadMore`. The observability traces list on /m was open-coding bottom-detection in its own `onScroll`, which meant every future consumer would have had to do the same, and the RAF-throttled hook already sitting next door went unused. VirtualTable now takes `loadMore` + `scrollThreshold` (default 300px) and routes them through the existing `useInfiniteScroll`, matching InfiniteVirtualTable's prop names so the eventual swap is a rename-free move. The handler is only invoked when `loadMore` is passed, so tables without it keep the exact scroll path they had. Verified in Storybook against a paging story: one scroll to the bottom loads page 2, eight scrolls reach the 200-row cap in 8 pages with no re-entrant fetches, and windowing holds throughout (27 rows mounted of 200). The Windowing story is unchanged: 35 mounted at scrollTop 200000, first row span-04175, same as before. Also uses the ROW_HEIGHT constant TracesTable declared but ignored, and reformats one pre-existing prettier failure in ChatScreen that blocked lint.
…eaks Second step of making VirtualTable a drop-in for InfiniteVirtualTable. antd owns selection internally and hands you callbacks; TanStack keeps it in a `Record<rowId, boolean>` the host controls. Same information, different shape, so `useVirtualTableRowSelection` converts between them rather than asking the existing call sites to change how they pass selection. The mapping is exact because VirtualTable's `getRowId` already stringifies `rowKey`, so a RowSelectionState key IS `String(rowKey(record))`. Covered: `selectedRowKeys`/`onChange` (which gets both keys and the matching records), `getCheckboxProps` disabling rows, `columnWidth`, `columnTitle`, `renderCell` (receiving the default control as `originNode`), `selectOnRowClick`, and `type: "radio"`. Disabled rows are filtered on every path, not just the one that renders them, so select-all and row-click can't sneak one in. Radio needs a RadioGroup ancestor and the group can't span rows here, so each row owns a one-item group and exclusivity comes from our state instead. Also fixes a missing `key` on the three conditional leading-column elements. React was warning on every table with a selection column; it predates this change and showed up because the new stories exercise that path. QA'd in Storybook: select-all picks 24 of 30 with the 6 disabled rows excluded and hands onChange 24 records; unchecking one flips the header to indeterminate; row-click toggles, and does nothing on a disabled row; radio replaces its selection instead of adding (row-1 then row-4, one checked throughout) and has no select-all header. Zero key warnings afterwards, verified per-story on a fresh mount.
theme.generated.css was already out of sync with the committed palette, which failed mobile's tokens:check and so blocked pnpm lint-fix. Regenerated with pnpm --filter @agenta/mobile generate:tokens; no palette change.
v8 was a reflex, not a decision: pnpm resolved v9, the API didn't match what I was writing, and I pinned back to ^8.21.3 instead of looking. v9.1.2 is the `latest` tag, not a prerelease. Two properties make it the right engine for this component specifically. v9 registers features as opt-in modules rather than shipping the set, so the bundle carries only what we use: this table exists so /m can replace web/oss and web/ee without antd, and unregistered features are weight it never carries. And v9 is built on TanStack Store with a `Subscribe` component for subscribing to slices of table state, where v8 re-renders the whole table on any state change. For a virtualized table holding selection and per-column sizing, that is the difference that matters. Nothing constrained the choice: @agenta/ui is the only package importing it, across three files, all written this week. Doing it now also means the column sizing work lands once, against v9, rather than being written on v8 and ported. What changed: useReactTable → useTable, with an explicit `features` object getCoreRowModel() → dropped; the core row model comes from core features VisibilityState → ColumnVisibilityState ColumnDef<T, V> → ColumnDef<TFeatures, T, V> The registered set lives in tableFeatures.ts so the column adapter and the table share one definition rather than importing each other. Re-QA'd every story against the v8 numbers; all identical. Basic 12 rows / 6 headers / 72 cells. Windowing 27 mounted of 10,000, and 35 at scrollTop 200000 starting at span-04175. Sticky offsets left 0px / 200px and right 0px. Merged cells 38 of 40, two dropped by colSpan 0. Selection: select-all takes 24 of 30 with the 6 disabled rows excluded and hands onChange 24 records, unchecking one flips the header to indeterminate. Radio replaces rather than adds, one checked throughout. Infinite loading reaches 200 rows over 8 pages with 27 mounted. Console clean.
Groundwork for moving column sizing onto TanStack. I had claimed the sizing migration would delete ~720 lines across the two resize hooks; checking that number showed useResizableColumns (221 lines) was simply dead — no importer, no barrel export, no test — and had nothing to do with TanStack at all. Sweeping the rest of the folder the same way found four more with no reference anywhere in packages, oss, ee, mobile or storybook, and no barrel export, so they are unreachable from outside the package too: useResizableColumns 221 useColumnDomRefs 79 useContainerSize 58 useTableHeaderHeight 55 useScopedColumnVisibility 27 ResizableTitle stays: useSmartResizableColumns still uses it and it is public via the barrel. A second pass found nothing newly orphaned by these removals. The real sizing work is smaller than I said and is still ahead: TanStack replaces the drag mechanics and clamping, while useSmartResizableColumns is a space-distribution algorithm (classify fixed/maxWidth/flexible, share the remainder, hold total >= containerWidth) that v9 has no equivalent for and that gets ported rather than deleted. @agenta/ui and @agenta/oss typecheck; lint green across all 25 tasks.
Step 2 of the sizing migration. The drag handle is a plain span bound to `header.getResizeHandler()` — no react-resizable, no antd. Widths live in `columnSizing`, so the host owns them and can persist them, and `minSize` (from the column's `minWidth`) does the clamping TanStack already knows how to do. `enableColumnResizing` is opt-in and `columnResizeMode` defaults to "onChange"; tables that don't ask for resizing render no handles and keep the DOM they had. Handles carry `avt-resize-handle` and `data-resize-handle="<columnId>"` so they are addressable from the same class-hook contract as the rest of the table. Verified by driving real mouse events in Storybook, not by writing widths: dragging the first handle +120 takes ID from 200 to 320, and both the header cell and the body cell report 320, which is the invariant that matters since header and body are separate tables. Dragging a middle column +60 takes Span type from 140 to 200. Dragging -800 clamps the render to 40. Handles are absent on every story that doesn't opt in. One behaviour worth knowing before the distribution algorithm consumes this: on an over-drag the persisted `columnSizing` holds the raw value (0 in the clamp test) while `getSize()` returns the clamped 40. Anything reading widths must go through `getSize()` rather than the state, and anything persisting the state can store a sub-minimum number. Known issue, not fixed here: since the v9 move, VirtualTable emits one React "unique key prop" warning per mount, including the Empty story with no rows, so it is in the header/colgroup scaffolding. It is dev-only and nothing renders wrong. I could not reproduce it in isolation — a faithful standalone repro of the same colgroup and thead, built from real v9 header groups with the exact column shape, stays silent under both SSR and client StrictMode renders. Other stories in the same Storybook do not warn, so it is specific to this component.
Step 3, and the part TanStack could not do for us. Its column sizing is per-column sizes plus a resize handler, with no notion of filling available space, so the space-sharing rules from useSmartResizableColumns were ported rather than deleted. distributeColumnWidths is now a pure function producing a ColumnSizingState, and VirtualTable applies it behind an opt-in `autoLayout` that measures its own container. The rules are the old ones on purpose, because changing them moves every table: pinned columns and capped columns are reserved first, whatever is left is shared among the rest in proportion to declared width, a drag always wins and opts a capped column out of its cap, and widths stay integers so the header colgroup and the body cannot round apart and drift. The invariant is that the total is never LESS than the container: when space runs short, columns keep their declared width and the table scrolls sideways instead of being squeezed. The width/minWidth defaults (200, and min(150, width)) are carried over verbatim, including the rule that a column narrower than the floor keeps its own smaller floor so it stays draggable. maxWidth is still read off the column rather than from ColumnDef, which is where it has always lived. Being pure, it is unit-tested rather than only clicked: 16 tests over proportional sharing, exact fill, integer output, the capped and pinned and selection-column reservations, all four drag interactions, and the edges (no columns, zero container, a leading column wider than the container). This adds vitest to the package, mirroring agenta-chat's setup. Confirmed in the browser that the live layout matches the tested rules, at four container widths: 1200 gives 120/705/235/140, 900 gives 120/480/160/140, and 700 gives 120/330/110/140, each landing exactly on the container. At 500 the columns stop shrinking, total 660, and the body scrolls.
Comparing /m against ee showed the same rows drawn differently: no cell borders on /m. Diffing what each surface passes explained it — web/oss sent `bordered: true` and `sticky: true` through tableProps and /m sent nothing, so "shared component" still meant "shared component plus whatever each app remembered to configure". Borders and the sticky header are properties of this table, not of either app, so the component owns them now and web/oss drops its copies. Callers can still override, since the defaults sit before the spread. Same for the Jotai store. The per-row cells read page-level atoms, so a table mounted in an isolated store renders zeros and dashes. web/oss passed its page store; /m did not. The component now falls back to the ambient store, which is what both surfaces wanted. Still different, and not a quick fix: /m has no selection checkboxes or Export/Delete/Add, because those live in web/oss's ObservabilityHeader — 459 lines with ten app-layer imports (testset drawer, delete modal, permissions, filter dialogs). Selection is only meaningful alongside the actions it feeds, so extracting them together is the next piece rather than adding checkboxes that do nothing. 23 tests, all three packages typecheck, lint green across 25 tasks.
The tabs sat under the title, left-aligned with a full-width divider, and the toolbar had no filter control — because /m hand-built a header instead of rendering the one web/oss uses. That header is PageLayout, which already lives in @agenta/ui, so /m renders it now: title left, tabs right on the same row, with the same icons. That also fixes the table's alignment. PageLayout carries the page gutter (px-16), so the table sits inside the same inset as everything above it. Before, the toolbar was inside a padded div and the table was not, which is why the table ran ~26px further left than the controls above it and bled to both edges. The filter control is back too. Both the dialog and the column builder are already shared — web/oss only wraps them to bind app-layer icons and the annotation row, neither of which /m needs to filter the same traces. Still missing on /m, and not shareable as they stand: Export, Delete and Add. Export drives CSV through app state, Delete opens web/oss's trace-delete modal, and Add is its testset/queue dropdown. The toolbar already exposes slots for all three, so this is a matter of extracting those actions rather than any further plumbing. Selection checkboxes belong with them, since they exist to feed them. @agenta/mobile typechecks and lints; 23 tests; lint green across 25 tasks.
Export was the largest of the missing actions and turned out to be almost entirely shared already: the query params, the adaptive page fetcher with its rate-limit backoff, the export writer and the row mapper all live in @agenta/observability. Only the app id and filename came from web/oss app state, so those are injected and the rest moved into a `useTracesExport` hook both surfaces call. web/oss keeps its current-app filename; /m names the file plainly, having no app scope. That deletes ~145 lines from web/oss's header along with the imports that fed them — Papa, the writer, the fetcher, the abort ref and its unmount effect. /m also gains selection checkboxes, which is what Delete and Add will read when they follow. Still missing on /m: Delete and Add. Both are whole app-layer components rather than logic — Delete opens web/oss's trace-delete modal, Add is its testset and queue dropdown, which pulls in the testset drawer and project permissions. Neither is a plumbing problem: the toolbar's slots are already wired and unused. @agenta/observability-ui, @agenta/oss and @agenta/mobile typecheck; 23 tests; lint green across 25 tasks. Adds papaparse to the package that now owns the CSV.
…ration The last two actions, and both were smaller than I had been claiming. I had described them as app-layer components that would drag the testset drawer and permissions along; reading them showed AddActionsDropdown has no app-layer imports at all, and DeleteTraceModal had three, all with package equivalents. Delete moves as-is apart from two inversions. Its project id now comes from projectIdAtom, and the two genuinely app-specific things — which trace the host currently has open, and closing the drawer behind a delete — became props, so a host without a trace drawer simply omits them. antd's Modal becomes AlertDialog and the icon comes from phosphor, so it carries no antd. Add moves with its antd Dropdown and Button swapped for the @agenta/ui primitives and the menu rendered through renderTableMenuItems, which already existed for exactly this. Its ButtonProps types were the only other antd tie and they were two string unions. /m now has the full toolbar: filters, Export, Delete and Add, with the selection those last two read. web/oss renders the same components from the package rather than its own copies. @agenta/observability-ui has no runtime antd import. All three packages typecheck, 23 tests, lint green across 25 tasks. Unverified in the browser: Delete's dialog is a different component than before (AlertDialog, not antd Modal) and Add's menu is rendered through a different primitive, so both deserve a real click on web/oss as well as /m.
Contributor
Author
|
@coderabbitai review |
❌ Action failedReview failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/oss/src/components/SharedDrawers/TraceDrawer/components/TraceContent/components/TraceTypeHeader/index.tsx (1)
289-289: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the active trace and drawer-close callback to the shared modal.
DeleteTraceModalreceivesopenTraceId = nulland noonDrawerClosecallback here. After a user deletes the open trace, it cannot navigate to an adjacent trace or close this drawer.Pass
getTraceIdFromNode(displayTrace) ?? nullasopenTraceId. Pass the existingcloseTraceDrawercallback asonDrawerClose.
🧹 Nitpick comments (7)
web/packages/agenta-observability-ui/src/index.ts (1)
133-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a
./tablesubpath instead of adding the tables to the root barrel.
./table/useTracesExportpulls inpapaparseand the export ETL. Adding it to the root barrel places that dependency in the module graph of every consumer that imports any root export. This package already uses subpath entries such as./toolbar. A./tablesubpath keeps the export path out of consumers that only need cells or filters.Line 134 also uses
export *, while the rest of this barrel lists names explicitly. Explicit names prevent accidental future collisions with the existingTraceRowvalue export on line 132.This follows the coding guideline for workspace packages: "use exported subpath imports for tree-shaking".
Source: Coding guidelines
web/packages/agenta-observability-ui/src/table/ObservabilityTracesTable.tsx (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOmit
dataSourcehere, as the sessions table does.
ObservabilitySessionsTablePropsomitsdataSource, but this interface does not. A caller can therefore forwarddataSourcethroughfeaturePropswhilepagination.rowsalso supplies rows. That creates two competing row sources on one public API.♻️ Proposed fix
export interface ObservabilityTracesTableProps extends Omit< InfiniteVirtualTableFeatureProps<TraceRow>, - "columns" | "rowKey" | "tableScope" | "pagination" + "columns" | "rowKey" | "tableScope" | "pagination" | "dataSource" > {web/mobile/src/features/observability/SessionsList.tsx (1)
66-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStabilize the list callbacks.
keyOfandrenderItemreceive new function identities on everySessionsListrender. MovekeyOfto module scope and memoizerenderItemwithuseCallback.Proposed refactor
+const keyOfSession = (sessionId: string) => sessionId + export const SessionsList = () => { + const renderSessionRow = useCallback( + (sessionId: string) => <SessionRow sessionId={sessionId} />, + [], + ) + return ( <ObservabilityList items={sessionIds} - keyOf={(sessionId) => sessionId} - renderItem={(sessionId) => <SessionRow sessionId={sessionId} />} + keyOf={keyOfSession} + renderItem={renderSessionRow}As per coding guidelines, “avoid unstable inline functions and objects, especially in lists.”
Source: Coding guidelines
web/packages/agenta-ui/src/InfiniteVirtualTable/components/VirtualTable.tsx (2)
305-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist
VIRTUAL_ALIGNto module scope.
VIRTUAL_ALIGNis a constant, but it is declared in the component body and read insideuseImperativeHandle.react-hooks/exhaustive-depsreports it as a missing dependency, and the package lint script runs with--max-warnings 0. Moving it besideCELL_PADDINGremoves the warning and the per-render allocation.♻️ Proposed fix
- // antd's align vocabulary differs from the virtualizer's. - const VIRTUAL_ALIGN = {top: "start", bottom: "end", auto: "auto"} as const - useImperativeHandle(Add near
CELL_PADDING:+// antd's align vocabulary differs from the virtualizer's. +const VIRTUAL_ALIGN = {top: "start", bottom: "end", auto: "auto"} as const
546-558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowed
expandedbinding.Line 550 declares
expanded, which shadows theexpandedprop for the whole row closure. The prop is the controlled expansion state, so the shadow makes the row body harder to reason about and blocks any later use of the prop here. Rename the local toisExpanded.web/packages/agenta-entity-ui/tests/unit/virtualTable.test.ts (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the key-less
dataIndexid cases.The current fixtures all set
key, so the id fallbacks inidOfare untested. Those fallbacks are the exact cases that diverge from the key derivation intoDistributable, which is raised onweb/packages/agenta-ui/src/InfiniteVirtualTable/components/VirtualTable.tsxlines 177-189. Add a column with an arraydataIndexand nokey, plus a column with neither, so the id contract is locked.💚 Proposed test addition
+ it("falls back to a dotted dataIndex path, then to the index, when no key is set", () => { + const ids = toTanstackColumns<Row>([ + {title: "Deep", dataIndex: ["nested", "deep"]}, + {title: "Render only", render: () => null}, + ]).map((c) => c.id) + expect(ids).toEqual(["nested.deep", "1"]) + })web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsx (1)
757-806: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRow-click selection now has two implementations, and one of them is dead.
virtualSelectionincludesonRowClickSelectwhenselectOnRowClickis set.VirtualTabledoes not acceptonRowClickSelect, so the spread at Line 799 passes an ignored prop. The active path staysmergedOnRow→handleSelectionRowClick, which re-derives keys and rows by hand (Lines 566-615) and duplicates the adapter logic.Compose
onRowClickSelectintomergedOnRowand deletehandleSelectionRowClick, or droponRowClickSelectfrom the adapter result. Keep one path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b878007-6664-486c-b685-3a4d2332289d
⛔ Files ignored due to path filters (2)
web/mobile/src/styles/theme.generated.cssis excluded by!**/*.generated.*web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
web/mobile/src/features/chat/ChatScreen.tsxweb/mobile/src/features/nav/useMobileNavItems.tsxweb/mobile/src/features/observability/ObservabilityScreen.tsxweb/mobile/src/features/observability/SessionsList.tsxweb/mobile/src/features/observability/SessionsTable.tsxweb/mobile/src/features/observability/TracesFilters.tsxweb/mobile/src/features/observability/TracesList.tsxweb/mobile/src/features/observability/TracesTable.tsxweb/mobile/src/features/observability/states/ObservabilityStates.tsxweb/mobile/src/features/observability/useTracesExportBinding.tsweb/mobile/src/pages/w/[workspace_id]/p/[project_id]/observability/index.tsxweb/oss/src/components/SharedDrawers/TraceDrawer/components/DeleteTraceModal/index.tsxweb/oss/src/components/SharedDrawers/TraceDrawer/components/TraceContent/components/LinkedSpansTabItem/index.tsxweb/oss/src/components/SharedDrawers/TraceDrawer/components/TraceContent/components/TraceTypeHeader/index.tsxweb/oss/src/components/pages/observability/components/ObservabilityHeader/index.tsxweb/oss/src/components/pages/observability/components/ObservabilityTable/index.tsxweb/oss/src/components/pages/observability/components/SessionsTable/index.tsxweb/packages/agenta-entity-ui/tests/unit/tableClassHooks.test.tsweb/packages/agenta-entity-ui/tests/unit/virtualTable.test.tsweb/packages/agenta-observability-ui/package.jsonweb/packages/agenta-observability-ui/src/actions/index.tsxweb/packages/agenta-observability-ui/src/actions/types.tsweb/packages/agenta-observability-ui/src/columns/getObservabilityColumns.tsxweb/packages/agenta-observability-ui/src/columns/getSessionColumns.tsxweb/packages/agenta-observability-ui/src/delete/index.tsxweb/packages/agenta-observability-ui/src/delete/store/atom.tsweb/packages/agenta-observability-ui/src/index.tsweb/packages/agenta-observability-ui/src/table/ObservabilitySessionsTable.tsxweb/packages/agenta-observability-ui/src/table/ObservabilityTracesTable.tsxweb/packages/agenta-observability-ui/src/table/useTracesExport.tsweb/packages/agenta-observability-ui/src/toolbar/ObservabilityToolbar.tsxweb/packages/agenta-observability/src/api/dashboard.tsweb/packages/agenta-observability/src/core/analytics.tsweb/packages/agenta-observability/src/core/presets.tsweb/packages/agenta-observability/src/state/controls.tsweb/packages/agenta-observability/src/state/index.tsweb/packages/agenta-observability/src/state/selectors.tsweb/packages/agenta-ui/package.jsonweb/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/VirtualTable.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/components/columnVisibility/ColumnVisibilityPopoverContent.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/distributeColumnWidths.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/features/InfiniteVirtualTableFeatureShell.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useColumnDomRefs.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useContainerSize.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useResizableColumns.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useScopedColumnVisibility.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableHeaderHeight.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useVirtualTableRowSelection.tsxweb/packages/agenta-ui/src/InfiniteVirtualTable/index.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/tableDom.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/tableFeatures.tsweb/packages/agenta-ui/src/InfiniteVirtualTable/tanstackColumns.tsweb/packages/agenta-ui/src/components/ui/checkbox-tree.tsxweb/packages/agenta-ui/src/components/ui/pagination.tsxweb/packages/agenta-ui/tests/unit/FeatureShell.render.test.tsxweb/packages/agenta-ui/tests/unit/VirtualTable.render.test.tsxweb/packages/agenta-ui/tests/unit/distributeColumnWidths.test.tsweb/packages/agenta-ui/vitest.config.tsweb/pnpm-workspace.yamlweb/storybook/stories/VirtualTable.stories.tsxweb/storybook/stories/domain/InfiniteVirtualTable.stories.tsx
💤 Files with no reviewable changes (6)
- web/oss/src/components/SharedDrawers/TraceDrawer/components/DeleteTraceModal/index.tsx
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useContainerSize.ts
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useResizableColumns.ts
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useColumnDomRefs.ts
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableHeaderHeight.ts
- web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useScopedColumnVisibility.tsx
Comment on lines
+124
to
+140
| onExport={tab === "traces" ? onExport : undefined} | ||
| isExporting={isExporting} | ||
| onDelete={tab === "traces" ? onDelete : undefined} | ||
| actionsSlot={ | ||
| tab === "traces" ? ( | ||
| <AddActionsDropdown | ||
| queueAction={{ | ||
| itemType: "traces", | ||
| itemIds: selectedRowKeys.map(String), | ||
| label: | ||
| selectedRowKeys.length > 0 | ||
| ? `Add ${selectedRowKeys.length} selected to queue` | ||
| : "Add selected to queue", | ||
| disabled: selectedRowKeys.length === 0, | ||
| }} | ||
| /> | ||
| ) : undefined |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the excluded mobile actions.
The stated v1 scope excludes mobile CSV export, bulk delete, and queue actions. These props enable all three actions in the mobile toolbar.
Remove these handlers and the related modal/export binding until the mobile workflows are supported. Keep the selection state for the desktop-width TracesTable.
Comment on lines
+11
to
+17
| /** | ||
| * The traces tab: the packaged list shell over the packaged trace row. | ||
| * | ||
| * There is no mobile-only rendering here on purpose. Both come from | ||
| * `@agenta/observability-ui`, so a change to how a span reads lands on desktop and here at | ||
| * the same time. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reduce the new explanatory comments to one short line.
web/mobile/src/features/observability/TracesList.tsx#L11-L17: reduce or remove the component rationale comment.web/packages/agenta-observability-ui/src/toolbar/ObservabilityToolbar.tsx#L69-L70: reduce the wrapping rationale to one short line.web/mobile/src/features/observability/TracesTable.tsx#L7-L12: reduce or remove the sizing rationale comment.web/mobile/src/features/observability/states/ObservabilityStates.tsx#L6-L11: reduce or remove the skeleton rationale comment.
As per coding guidelines, keep in-code comments to at most one short line; reserve longer comments for genuinely surprising constraints such as bugs, races, or ordering requirements.
📍 Affects 4 files
web/mobile/src/features/observability/TracesList.tsx#L11-L17(this comment)web/packages/agenta-observability-ui/src/toolbar/ObservabilityToolbar.tsx#L69-L70web/mobile/src/features/observability/TracesTable.tsx#L7-L12web/mobile/src/features/observability/states/ObservabilityStates.tsx#L6-L11
Source: Coding guidelines
Comment on lines
+35
to
+46
| <ObservabilityList | ||
| items={traces} | ||
| keyOf={(span, index) => span.span_id ?? String(index)} | ||
| renderItem={(span) => ( | ||
| <div className="border-0 border-b border-solid border-border px-4 py-3"> | ||
| <TraceRow span={span} /> | ||
| </div> | ||
| )} | ||
| isLoading={isLoading} | ||
| isLoadingMore={isFetchingMore} | ||
| hasMore={hasMoreTraces} | ||
| loadMore={fetchMoreTraces} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not expose bulk delete for the narrow trace list.
This list has no selection control. ObservabilityScreen derives traceIds only from selection state set by TracesTable. On narrow screens, Delete opens the shared modal with [], so confirmation performs no deletion.
Hide the delete action for the narrow layout. This also matches the v1 scope that excludes mobile bulk delete.
Comment on lines
+50
to
+54
| const handleDelete = async () => { | ||
| try { | ||
| setIsLoading(true) | ||
| const projectId = projectIdValue | ||
| await Promise.all(traceIds.map((id) => deletePreviewTrace(id, projectId ?? ""))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report delete failures to the user, and skip the request when projectId is missing.
Two gaps in handleDelete:
- The catch block only calls
console.error. The dialog stays open with no feedback, so the user cannot tell that the delete failed. This package already usesmessagefrom@agenta/ui/app-messageinsrc/table/useTracesExport.ts. - Line 54 passes
projectId ?? "". An empty project id sends a request that cannot succeed. Return early instead.
🛡️ Proposed fix
+import {message} from "`@agenta/ui/app-message`" const handleDelete = async () => {
+ const projectId = projectIdValue
+ if (!projectId) return
try {
setIsLoading(true)
- const projectId = projectIdValue
- await Promise.all(traceIds.map((id) => deletePreviewTrace(id, projectId ?? "")))
+ await Promise.all(traceIds.map((id) => deletePreviewTrace(id, projectId))) } catch (error) {
console.error(error)
+ message.error({content: "Failed to delete traces"})
} finally {Also applies to: 83-88
Comment on lines
+59
to
+70
| if (isCurrentTraceDeleted && traceIds.length === 1) { | ||
| const deletedIndex = traces.findIndex((t) => t.trace_id === traceIds[0]) | ||
| const nextTrace = traces[deletedIndex + 1] || traces[deletedIndex - 1] | ||
|
|
||
| if (nextTrace) { | ||
| const url = new URL(window.location.href) | ||
| url.searchParams.set("trace", nextTrace.trace_id) | ||
| url.searchParams.delete("span") | ||
| await Router.push(url.toString(), undefined, {shallow: true}) | ||
| } else { | ||
| closeDrawer() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard the findIndex miss before selecting the next trace.
If traces does not contain traceIds[0], findIndex returns -1. Then traces[deletedIndex + 1] resolves to traces[0], and the code navigates to an unrelated trace instead of closing the drawer. The open trace can be absent from traces when the list has been re-filtered or re-paged since the drawer opened.
🐛 Proposed fix
- if (isCurrentTraceDeleted && traceIds.length === 1) {
- const deletedIndex = traces.findIndex((t) => t.trace_id === traceIds[0])
- const nextTrace = traces[deletedIndex + 1] || traces[deletedIndex - 1]
+ if (isCurrentTraceDeleted && traceIds.length === 1) {
+ const deletedIndex = traces.findIndex((t) => t.trace_id === traceIds[0])
+ const nextTrace =
+ deletedIndex === -1
+ ? undefined
+ : (traces[deletedIndex + 1] ?? traces[deletedIndex - 1])📝 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
| if (isCurrentTraceDeleted && traceIds.length === 1) { | |
| const deletedIndex = traces.findIndex((t) => t.trace_id === traceIds[0]) | |
| const nextTrace = traces[deletedIndex + 1] || traces[deletedIndex - 1] | |
| if (nextTrace) { | |
| const url = new URL(window.location.href) | |
| url.searchParams.set("trace", nextTrace.trace_id) | |
| url.searchParams.delete("span") | |
| await Router.push(url.toString(), undefined, {shallow: true}) | |
| } else { | |
| closeDrawer() | |
| } | |
| if (isCurrentTraceDeleted && traceIds.length === 1) { | |
| const deletedIndex = traces.findIndex((t) => t.trace_id === traceIds[0]) | |
| const nextTrace = | |
| deletedIndex === -1 | |
| ? undefined | |
| : (traces[deletedIndex + 1] ?? traces[deletedIndex - 1]) | |
| if (nextTrace) { | |
| const url = new URL(window.location.href) | |
| url.searchParams.set("trace", nextTrace.trace_id) | |
| url.searchParams.delete("span") | |
| await Router.push(url.toString(), undefined, {shallow: true}) | |
| } else { | |
| closeDrawer() | |
| } |
Comment on lines
+177
to
+189
| return { | ||
| key: String(column.key ?? column.dataIndex ?? ""), | ||
| width, | ||
| // A column narrower than the default floor keeps its own smaller floor, else the | ||
| // floor would exceed the width it asked for and it could never be dragged down. | ||
| minWidth: | ||
| typeof column.minWidth === "number" | ||
| ? column.minWidth | ||
| : Math.min(AUTO_LAYOUT_DEFAULT_MIN_WIDTH, width), | ||
| maxWidth: typeof declaredMax === "number" && declaredMax > 0 ? declaredMax : undefined, | ||
| isFixed: Boolean(column.fixed), | ||
| } | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The distributable key does not match the TanStack column id.
effectiveSizing is fed into state.columnSizing, which TanStack keys by column id. tanstackColumns.ts derives that id with idOf: an array dataIndex joins with ".", and a column with neither key nor dataIndex falls back to its index. This function derives the key differently:
- An array
dataIndexstringifies to"nested,deep"instead of"nested.deep". - A column with neither
keynordataIndexbecomes"", which also collides across all such columns.
For those columns the computed width never applies, and auto-layout falls back to the declared size. Export the id derivation from tanstackColumns.ts and use it in both places so the two key spaces cannot drift.
🐛 Proposed fix: share one id derivation
In web/packages/agenta-ui/src/InfiniteVirtualTable/tanstackColumns.ts, export the helper:
-const idOf = <RecordType>(column: ColumnDef<RecordType>, index: number): string =>
+export const idOf = <RecordType>(column: ColumnDef<RecordType>, index: number): string =>
String(
column.key ??
(Array.isArray(column.dataIndex) ? column.dataIndex.join(".") : column.dataIndex) ??
index,
)Then in this file:
-import {sourceOf, toTanstackColumns} from "../tanstackColumns"
+import {idOf, sourceOf, toTanstackColumns} from "../tanstackColumns"- return leaves.map((column) => {
+ return leaves.map((column, index) => {- key: String(column.key ?? column.dataIndex ?? ""),
+ key: idOf(column, index),Note that leaf flattening must then use the same index basis as toTanstackColumns for the fallback case.
Comment on lines
+72
to
+82
| const emit = useCallback( | ||
| (next: RowSelectionState) => { | ||
| const keys = Object.keys(next).filter((id) => next[id]) | ||
| // Keys can outlive their rows across pages; only map back the ones we hold. | ||
| const rows = keys | ||
| .map((id) => recordById.get(id)) | ||
| .filter((record): record is RecordType => record !== undefined) | ||
| onChange?.(keys, rows) | ||
| }, | ||
| [onChange, recordById], | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find rowSelection call sites and the rowKey they pair with.
rg -nP -C6 'rowSelection=\{\{' --type=tsx --type=ts web | head -300
# Look for numeric selected-key state declarations.
rg -nP -C3 'useState<(number|Key)\[\]>\(\s*\[\s*\]\s*\)' --type=ts --type=tsx webRepository: Agenta-AI/agenta
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- hook ---'
sed -n '1,180p' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useVirtualTableRowSelection.tsx
printf '%s\n' '--- types ---'
sed -n '150,230p' web/packages/agenta-ui/src/InfiniteVirtualTable/types.ts
printf '%s\n' '--- row-selection call sites ---'
rg -n -P -C8 'rowSelection\s*=\s*\{' web -g '*.ts' -g '*.tsx' | head -500
printf '%s\n' '--- selected-key declarations and comparisons ---'
rg -n -P -C4 'selectedRowKeys|selectedKeys|rowKey\s*[:=]' web -g '*.ts' -g '*.tsx' | head -800Repository: Agenta-AI/agenta
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining hook and table identity code ---'
sed -n '150,340p' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useVirtualTableRowSelection.tsx
rg -n -P -C5 'getRowId|rowKey\s*[:=]|rowKey\??\s*=' web/packages/agenta-ui/src/InfiniteVirtualTable -g '*.ts' -g '*.tsx' | head -300
printf '%s\n' '--- selectedRowKeys and selected-key state usage ---'
rg -n -P -C5 'selectedRowKeys|selectedKeys|setSelectedRowKeys|setSelectedKeys' web -g '*.ts' -g '*.tsx' \
| rg -v 'node_modules|dist|build' | head -1000
printf '%s\n' '--- rowKey callbacks that return likely numeric fields ---'
rg -n -P -C3 'rowKey=\{[^}]*=>[^}]*\b(id|index|order|key)\b|rowKey:\s*\([^)]*\)\s*=>\s*[^,\n;}]+' web -g '*.ts' -g '*.tsx' \
| head -500Repository: Agenta-AI/agenta
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- numeric selection declarations ---'
rg -n -P '(useState|useMemo|useAtom|const)\s*<[^>\n]*(number|Key)\[\]|selectedRowKeys\s*:\s*(number|Key)\[\]|selectedKeys\s*:\s*(number|Key)\[\]' \
web -g '*.ts' -g '*.tsx' | head -300
printf '%s\n' '--- story selection implementations ---'
rg -n -P -C12 'selectedKeys|setSelectedKeys|selectedRowKeys|setSelectedRowKeys|interface EvalRow|type EvalRow|interface Row\b|type Row\b' \
web/storybook/stories/domain/InfiniteVirtualTable.stories.tsx \
web/storybook/stories/VirtualTable.stories.tsx | head -500
printf '%s\n' '--- rowKey definitions in selection-bearing areas ---'
rg -n -P -C3 'rowKey\s*=' web/storybook web/oss web/mobile web/packages -g '*.ts' -g '*.tsx' \
| rg -B3 -A3 'rowSelection|selectedRowKeys|selectedKeys|setSelected' | head -500
printf '%s\n' '--- runtime key coercion probe ---'
node - <<'JS'
const next = {12: true, 13: true}
const emitted = Object.keys(next).filter((id) => next[id])
console.log(JSON.stringify({emitted, types: emitted.map((value) => typeof value)}))
JSRepository: Agenta-AI/agenta
Length of output: 31121
Preserve numeric row keys in onChange
Object.keys(next) always returns strings, but Key includes numbers. Map each ID to its original key before calling onChange. Include selectedRowKeys in this mapping so numeric selections that outlive dataSource remain numeric; a current-row-only map still returns strings for those selections.
Comment on lines
+193
to
+212
| return useMemo(() => { | ||
| if (!rowSelection) return undefined | ||
| return { | ||
| rowSelection: selectionState, | ||
| onRowSelectionChange, | ||
| leadingColumnWidth: columnWidth, | ||
| renderLeadingCell, | ||
| renderLeadingHeader, | ||
| ...(selectOnRowClick ? {onRowClickSelect} : {}), | ||
| } | ||
| }, [ | ||
| rowSelection, | ||
| selectionState, | ||
| onRowSelectionChange, | ||
| columnWidth, | ||
| renderLeadingCell, | ||
| renderLeadingHeader, | ||
| selectOnRowClick, | ||
| onRowClickSelect, | ||
| ]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find rowSelection usages that set fixed or onCell.
rg -nP -B4 -A8 'rowSelection\s*[=:]\s*\{' web --type=ts --type=tsx | rg -n -C6 '\bfixed\b|\bonCell\b'Repository: Agenta-AI/agenta
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target declarations and adapter ---'
rg -n -C 8 'InfiniteVirtualTableRowSelection|VirtualTableRowSelectionProps|useVirtualTableRowSelection|fixed|onCell' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useVirtualTableRowSelection.tsx web/packages/agenta-ui/src/InfiniteVirtualTable
printf '%s\n' '--- rowSelection call sites containing fixed/onCell ---'
rg -n -C 8 'rowSelection|fixed|onCell' web -g '*.ts' -g '*.tsx' | rg -C 4 '\b(rowSelection|fixed|onCell)\b'
printf '%s\n' '--- exact hook consumers ---'
rg -n -C 5 'useVirtualTableRowSelection|leadingColumnWidth|renderLeadingHeader' web -g '*.ts' -g '*.tsx'Repository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- hook ---'
sed -n '1,240p' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useVirtualTableRowSelection.tsx
printf '%s\n' '--- row-selection types ---'
sed -n '160,220p' web/packages/agenta-ui/src/InfiniteVirtualTable/types.ts
printf '%s\n' '--- hook consumers and rowSelection assignments ---'
rg -n -C 6 'useVirtualTableRowSelection|rowSelection\s*=' web/packages/agenta-ui web/oss web/packages/agenta-entity-ui -g '*.ts' -g '*.tsx' | head -n 1200Repository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- virtual selection integration ---'
rg -n -C 12 'virtualSelection|leadingColumnWidth|renderLeadingCell|renderLeadingHeader|onRowClickSelect' web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsx web/packages/agenta-ui/src/InfiniteVirtualTable -g '*.ts' -g '*.tsx'
printf '%s\n' '--- selection-option literals in likely row-selection declarations ---'
rg -n -C 3 'fixed\s*:|onCell\s*:' web/oss web/packages/agenta-ui -g '*.ts' -g '*.tsx' | rg -C 5 'rowSelection|fixed\s*:|onCell\s*:'
printf '%s\n' '--- direct rowSelection option references ---'
rg -n 'rowSelection\.(fixed|onCell)|\b(fixed|onCell)\b' web/packages/agenta-ui/src/InfiniteVirtualTable -g '*.ts' -g '*.tsx'Repository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
for p in Path("web").rglob("*"):
if p.suffix not in {".ts", ".tsx"} or not p.is_file():
continue
text = p.read_text(errors="ignore")
if "rowSelection" not in text:
continue
hits = []
lines = text.splitlines()
for i, line in enumerate(lines):
if "fixed:" in line or "onCell:" in line:
hits.append((i + 1, line.strip()))
if hits:
print(p)
for n, line in hits:
print(f" {n}: {line}")
PY
printf '%s\n' '--- VirtualTable leading-column rendering ---'
sed -n '360,520p' web/packages/agenta-ui/src/InfiniteVirtualTable/components/VirtualTable.tsx
printf '%s\n' '--- VirtualTable rendered-cell types ---'
sed -n '1,150p' web/packages/agenta-ui/src/InfiniteVirtualTable/components/VirtualTable.tsxRepository: Agenta-AI/agenta
Length of output: 17315
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- current row-selection option call sites ---'
sed -n '380,410p' web/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsx
sed -n '670,705p' web/oss/src/components/TestsetsTable/TestsetsTable.tsx
sed -n '300,330p' web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
printf '%s\n' '--- legacy selection adapter ---'
sed -n '1,90p' web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableRowSelection.ts
printf '%s\n' '--- engine selection switch ---'
rg -n -C 8 'useVirtualTable|VirtualTable|useTableRowSelection|rowSelection' web/packages/agenta-ui/src/InfiniteVirtualTable/components/InfiniteVirtualTableInner.tsxRepository: Agenta-AI/agenta
Length of output: 19969
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Testcases selection object and later onCell ---'
sed -n '308,340p' web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
sed -n '680,705p' web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx
printf '%s\n' '--- all selection-specific onCell mentions ---'
rg -n -C 5 'rowSelection|onCell' web/oss/src/components/TestcasesTableNew/components/TestcasesTableShell.tsx web/oss/src/components/EvaluationRunsTablePOC/components/EvaluationRunsTable/index.tsx web/oss/src/components/TestsetsTable/TestsetsTable.tsx web/packages/agenta-ui/src/InfiniteVirtualTable/hooks/useTableManager.tsxRepository: Agenta-AI/agenta
Length of output: 26784
Preserve onCell for virtual selection cells.
VirtualTableRowSelectionProps omits the public onCell option, while the legacy adapter applies it to each selection cell. No current caller sets selection-level onCell, and current fixed values (true or "left") already match the virtual table's sticky leading column. Forward onCell and apply its props to the leading cells.
Comment on lines
+36
to
+57
| const {container} = render( | ||
| <QueryClientProvider client={client}> | ||
| <InfiniteVirtualTableFeatureShell<Row> | ||
| tableScope={{scopeId: "test", pageSize: 50}} | ||
| columns={columns} | ||
| rowKey="session_id" | ||
| pagination={{ | ||
| rows: [], | ||
| loadNextPage: () => undefined, | ||
| resetPages: () => undefined, | ||
| paginationInfo: { | ||
| hasMore: false, | ||
| nextCursor: null, | ||
| nextOffset: null, | ||
| isFetching: false, | ||
| totalCount: 0, | ||
| }, | ||
| }} | ||
| tableProps={{locale: {emptyText: "No sessions yet"}}} | ||
| /> | ||
| </QueryClientProvider>, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prettier fails on both new test files. The shared root cause is that the formatter was not run before committing these files.
web/packages/agenta-ui/tests/unit/FeatureShell.render.test.tsx#L36-L57: indent theInfiniteVirtualTableFeatureShellelement one level insideQueryClientProvider.web/packages/agenta-ui/tests/unit/VirtualTable.render.test.tsx#L52-L56: reformat thevi.spyOn(...).mockImplementation(...)chain and therender(...)call at Line 96.
Run pnpm lint-fix from the web directory. As per coding guidelines: "Before committing frontend changes, run pnpm lint-fix from the web directory."
📍 Affects 2 files
web/packages/agenta-ui/tests/unit/FeatureShell.render.test.tsx#L36-L57(this comment)web/packages/agenta-ui/tests/unit/VirtualTable.render.test.tsx#L52-L56
Sources: Coding guidelines, Pipeline failures
Comment on lines
+375
to
+387
| useEffect(() => { | ||
| Object.keys(expanded) | ||
| .filter((key) => expanded[key] && !children[key]) | ||
| .forEach((key) => { | ||
| const timer = setTimeout(() => { | ||
| setChildren((prev) => ({ | ||
| ...prev, | ||
| [key]: [`${key} child A`, `${key} child B`, `${key} child C`], | ||
| })) | ||
| }, 400) | ||
| return () => clearTimeout(timer) | ||
| }) | ||
| }, [expanded, children]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The timer cleanup is discarded, so the timers are never cleared.
The return () => clearTimeout(timer) sits inside the forEach callback. forEach ignores the return value, and the effect itself returns undefined. Every effect run schedules new timers that survive unmount, and each setChildren re-runs the effect, so timers accumulate.
🐛 Proposed fix: collect the timers and clear them in the effect cleanup
useEffect(() => {
- Object.keys(expanded)
+ const timers = Object.keys(expanded)
.filter((key) => expanded[key] && !children[key])
- .forEach((key) => {
- const timer = setTimeout(() => {
+ .map((key) =>
+ setTimeout(() => {
setChildren((prev) => ({
...prev,
[key]: [`${key} child A`, `${key} child B`, `${key} child C`],
}))
- }, 400)
- return () => clearTimeout(timer)
- })
+ }, 400),
+ )
+ return () => timers.forEach(clearTimeout)
}, [expanded, children])📝 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
| useEffect(() => { | |
| Object.keys(expanded) | |
| .filter((key) => expanded[key] && !children[key]) | |
| .forEach((key) => { | |
| const timer = setTimeout(() => { | |
| setChildren((prev) => ({ | |
| ...prev, | |
| [key]: [`${key} child A`, `${key} child B`, `${key} child C`], | |
| })) | |
| }, 400) | |
| return () => clearTimeout(timer) | |
| }) | |
| }, [expanded, children]) | |
| useEffect(() => { | |
| const timers = Object.keys(expanded) | |
| .filter((key) => expanded[key] && !children[key]) | |
| .map((key) => | |
| setTimeout(() => { | |
| setChildren((prev) => ({ | |
| ...prev, | |
| [key]: [`${key} child A`, `${key} child B`, `${key} child C`], | |
| })) | |
| }, 400), | |
| ) | |
| return () => timers.forEach(clearTimeout) | |
| }, [expanded, children]) |
…tion Delete had three real defects, all pre-existing and carried over verbatim by the move — checking against the pre-move file confirmed none were introduced here, but the code lives in a package now, so it gets fixed rather than preserved. A missing project id used to fall back to `?? ""` and delete against nothing; it now refuses and says so. A failed delete only reached console.error, leaving the dialog open with no explanation; it now surfaces. And findIndex returning -1 for a trace outside the current page became traces[0], sending the user to an unrelated trace; that is guarded. /m offered Delete on phone widths, where the card list has no selection control at all, so it could only ever act on an empty set. Rather than repeat the width check at each action, `showsTable` is the single condition deciding both what renders and which selection-dependent actions exist. My first read of this one was that the reviewer was wrong; it was right. ANTD_SELECTOR is no longer exported. It is used in five files inside the package and by no consumer, and with the antd table deleted those selectors match nothing in our own DOM — worth noting as a follow-up, since that makes the internal fallbacks dead rather than merely private. Not taken: the suggestion to remove /m's export and bulk actions as out-of-scope. That reads the PR description, which predates the decision to give /m the same toolbar the desktop has; the description is stale, the code is deliberate. 23 tests, all packages typecheck, lint green across 25 tasks.
…item
The two that mattered were data-integrity bugs in code from this stack.
Column ids were derived twice and did not agree. toTanstackColumns joins an
array dataIndex with "." and falls back to the column index; toDistributable
stringified the array ("a,b") and fell back to "". TanStack keys columnSizing by
the former, so auto-layout's computed widths addressed keys that did not exist
and were silently dropped — columns quietly kept their declared widths. Rather
than align the second copy, the id is now one exported function both call, since
visibility and selection key off it too and a third derivation would fail the
same way.
Row selection lost numeric keys. Object.keys is always strings, so a host
passing numeric rowKeys got strings back from onChange. The ids now map back to
their original keys on the way out.
Also: stampTableDom moves to useLayoutEffect, so the first painted frame is not
missing avt-container/avt-body/avt-thead when a consumer queries them on mount.
createExportWriter is wrapped, since it sat outside the try and the caller fires
this with `void onExport()`, so a rejection escaped unhandled and unreported.
TableDescription resets both margins, the native top one returning because
preflight is off here. CheckboxTree gets treeitem/group roles to match its
role="tree". @testing-library/dom is declared, being a required peer of
@testing-library/react that I added without it.
Rejected after checking: preserving fixed-column behaviour across the antd peer
range, which is moot now that the antd table is deleted. Deferred: widening
groupColumnsRecursive's return to the group union — correct, but it cascades
into inference loss at the sort callbacks and deserves its own change.
23 tests, all packages typecheck, lint green across 25 tasks.
rowSelection.onCell was accepted and then dropped. The adapter destructured everything else from the antd shape and never read onCell, so a caller styling its selection cell got nothing. VirtualTable now takes leadingCellProps and the adapter forwards onCell into it, which makes the claim that the antd shape is supported true rather than nearly true. The Expandable story leaked timers: the cleanup was returned from a forEach callback, which discards it, so nothing was ever cleared. Collected and cleared on unmount. Left open deliberately: building CSV headers from exportable leaf columns. The finding is real — the grouped "Evaluators" title has no matching key in createTraceObject, so Papa writes an empty column, and evaluator metrics live in traceAnnotationInfoAtomFamily rather than on the row. That is a pre-existing export defect this stack only relocated, and fixing it means an export-time evaluator mapper, which deserves its own change with a real CSV to check against rather than being rushed in at the end of a review pass. 23 tests, all packages typecheck, lint green across 25 tasks.
The deferred review item, done properly rather than patched.
groupColumnsRecursive builds `{key, title, children}` group columns and both it
and the public groupColumns declared `ColumnDef<T>[]`, which is leaf-only. That
compiled because a group is structurally assignable to a leaf — every extra
field is optional — so the wrong type was never going to be caught by tsc. Both
signatures now say ColumnDefs<T>, the union the function has always returned.
My first attempt cascaded into "implicitly any" errors at the sort callbacks and
I backed it out. That was self-inflicted: I changed the annotation without
importing ColumnDefs, so `result` became an error type and everything reading it
degraded. ColumnDefs is an array of a union, not a union of arrays, so inference
is fine once the name resolves.
countLeafColumns went with it. It took ColumnDef<T>[] and cast each entry to
reveal a `children` field the type denied having; it now takes the union and
asks isColumnGroupDef, which is the same question without the cast.
@agenta/ui, @agenta/oss and @agenta/entities typecheck — no consumer was
relying on the narrower claim. 23 tests, lint green across 25 tasks.
The last open review item. Papa is handed {fields: headers, data: rows} and
reads row[field] for every header, so the header list and createTraceObject are
one contract. They had drifted: headers came from the visible column titles,
which include the "Evaluators" group and its children, and createTraceObject
emits none of those — evaluator metrics come from traceAnnotationInfoAtomFamily,
not the trace row. Every such header wrote an empty column, and conversely any
key the visible columns omitted was dropped without a word.
Deriving headers from columns could never have changed the data, because the row
mapper does not consult columns at all — it could only misalign it. So headers
now come from what the mapper actually emits, narrowed to the columns still
visible and kept in the mapper's order. Hiding a column still drops it from the
CSV; showing one the mapper cannot fill no longer invents a blank one.
Pinned with a test in the package that owns the mapper: the emitted keys must
equal DEFAULT_TRACE_EXPORT_HEADERS exactly, and every promised header must be
present in the row. That is the contract the writer depends on, and it is what
would have caught this.
65 tests in @agenta/observability, 23 in @agenta/ui, @agenta/oss typechecks,
lint green across 25 tasks.
Still unverified against a real download: this needs an actual export with
evaluator columns visible, which needs the app running.
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.
Opening /evaluations died with "Maximum update depth exceeded" on reload. The cause was two immer copies: oss and ee depend on immer 10, which is where `enableMapSet()` runs, but @agenta/ui declared jotai-immer without declaring immer, so pnpm resolved that peer to immer 11 — a second instance where the plugin was never enabled. `columnVisibilityStateAtom` holds a Map of Maps, so every viewport-visibility write went through immer 11 and threw. The throw tore down the table chrome, and React's repeated attempts to re-render that subtree produced the update loop. It only showed on reload because a first paint writes no visibility state yet. Three changes, smallest first: @agenta/ui now declares immer, a workspace override pins a single version (following the @tanstack/query-core precedent already in that file), and `enableMapSet()` moved into the atom module that owns the Map. That last one matters beyond this bug — only web/oss called it, so /m, Storybook and tests each hit the same crash on their first write.
Three call sites handed SimpleTooltip a child that was itself an asChild Radix trigger, so both collapsed onto one button and Radix composed their refs into a single nested chain. A span now separates them; focus still reaches the tooltip because React's onFocus bubbles from the child. This is hygiene, not a bug fix. It was the prime suspect for the /evaluations loop and it was wrong: the repro tests here pass with and without the change, and un-nesting all three did not clear the page. The real cause was a second immer instance. The tests stay because they pin the structural property — the live DOM had `data-slot="tooltip-trigger"` sitting on the trigger button, and the guard fails if that returns.
Turning on evaluator columns and hitting Export produced a CSV without them and no indication anything was dropped. Two reasons: the header list is fixed and carries no evaluator entries, and the header selection is title-based while evaluator leaf columns have `title: null` (the header cell is hidden and the cell draws its own label), so it could not see them even in principle. The metrics are correlated data, not row data — they live on annotations linked to a span. `exportMatchingTraces` gained an optional `enrich` transform that runs after dedup and the cap, so nothing is fetched for a row that will not be written, and the join happens there: one annotations request per chunk for every invocation link at once, never one per row. That per-chunk property is what made this feasible at all and is the first thing the tests pin. Enrichment belongs in a transform rather than in `flushBatch` because the sink is the format boundary: anything done there cannot be reused by a different sink and sits outside the loop's cancellation path.
The derivation lived in web/oss, so /m hardcoded an empty list and silently showed no evaluator columns — and, once the CSV learned to export them, no evaluator headers either. That is the drift the shared table was meant to end. `useEvaluatorSlugs` now owns collecting slugs off the loaded rows (walking descendants, since a root trace's metrics can sit on a child) and ordering them: annotation order first, row-only slugs appended sorted so the column order is stable across renders. The traces table derives them when a host omits the prop, so a surface gets the columns without knowing they exist. web/oss loses its local copy rather than keeping a second one in step.
…servability The drawer's store, hooks and helpers lived in web/oss, which is why /m had no way to open a trace at all. They move to @agenta/observability/traceDrawer, reachable by any host. Every app dependency resolved to something already packaged rather than a new seam: `getOrgValues().selectedOrg.default_workspace.members` became `workspaceMembersAtom`, the oss `queryAllAnnotations` wrapper became the entities one plus `projectIdAtom`, and `@/oss/services/tracing/types` was only a type re-export of this package. The observability DTOs and the transformer came along, since the store is their only real consumer. Old paths are thin re-exports, so all seventeen callers are untouched — the same technique `oss/src/services/tracing/types` already used. Tightening the DTOs from `any` to `unknown` surfaced a latent bug: `traceDrawerAnnotationLinksAtom` was declared as requiring both ids but built entries from optional fields, so a span missing either entered the annotations query as `undefined`. It now filters those rows out. `atoms.ts` and `openInPlayground.ts` stay in the app: they import @agenta/playground, and a headless observability package importing playground inverts the layering.
…ty-ui Twenty components move out of web/oss and off antd: Collapse becomes a Radix Accordion, Splitter a flex pair, Tabs the kit Tabs, Radio.Group a Segmented, Dropdown a DropdownMenu, and CustomTreeComponent drops react-jss for Tailwind with its connector geometry unchanged (6px rule, 12px elbow at 50%, 200px label cap). AccordionTreePanel's JSS targeted `.ant-collapse-*` internals, so those rules became plain classes on markup this package now owns. Three groups stayed app-coupled and are handed in as slots rather than imported: the References lookup UI (20 files), the DrillIn viewers (large and currently forked), and the playground plus the annotate/testset drawers, which sit above this package in the dependency order. Their fallbacks render a plain label or nothing, so a host that registers none degrades instead of crashing. `registerReferenceSlots` in web/oss supplies the real ones. Navigation is a seam for the same reason routing lives in the app: project URL, base app URL, a navigate, a query-param write and a clear. web/oss binds all five from its provider. ResultTag and LabelValuePill were two components drawing the same split pill with different measurements, so a fix to one missed the other. They share a `Pill` now; each preset supplies its complete class set rather than layering onto a shared base, because these are plain clsx joins and a shared base would leave both `rounded-sm` and `rounded-control` present. The annotations panel dropped antd Table for a 115-line SimpleTable that takes the same column descriptors, so the column factory barely changed. Not runtime-verified: docker was down for this batch.
Mobile had no route to a trace at all: no drawer, no detail page, and neither the table nor the phone card list carried a tap handler. Sessions were openable; traces were not. Now that the drawer is packaged, /m renders the same one web/oss does. Row taps open it on the table, and the card list gets a handler for the first time (with keyboard support, since it is a div). `bindTraceDrawerSeams` points the drawer's navigate, query-param and clear-param seams at /m's router — the second host binding them, which is the reason they are seams. The reference, drill-in and playground slots are deliberately left unregistered here: those components are desktop-only, and their fallbacks render a plain label or nothing, so /m degrades rather than crashes.
Deleting the antd Table branch left the scroll and header lookups pointing at `.ant-table-*`, which the rendered DOM no longer contains. They resolved to null and fell through silently — the scroll container quietly became the wrong element rather than erroring. `DOM_SELECTOR` pairs each hook as `avt-*` first with the antd selector behind it, so a host still mounting an antd table through the legacy column adapter keeps working.
Six issues raised on #5954-#5958, each verified against the code first. `getNodeById` walked `Object.values(node)`, which visits every property rather than just `children`. Span metadata carries `span_id` too — an annotation span's `invocationIds` points at a different span — so a single-node lookup could return that bag instead of the span. The regression test covers exactly that shape; it passes on an array input either way, which is why the first version of it caught nothing. Both annotation queries scoped their request by `projectId` but not their cache key, so a project switch with the same links reused the previous project's annotations. The drawer store had inherited the same mistake when it stopped calling the oss wrapper that resolved the project internally. Session token and cost totals used `||` down their fallback chains, so a real incremental total of 0 fell through to the cumulative one and double-counted a span that reported no new tokens. `getOperator` can return undefined for an operator in the union but missing from OPERATORS; dereferencing `hidesValue` threw during validation and took the dialog's render with it. Plus two small ones: the docs link opened without `noopener`, and a secondary-only empty state rendered an orphaned "Or" separator.
… theme-safe Two findings from the #5958 and #5957 reviews. The date picker's calendar was reachable but not usable without a mouse: every day carried `tabIndex={-1}` and no key handler existed, so a keyboard user could open the popover and go no further. It also declared `role="grid"` while rendering one flat list of cells with no rows. It now uses a roving tabindex — one day in the tab order at a time — with arrows moving by day and by week, Home/End along the week, PageUp/PageDown by month, and Enter or Space to select. Stepping past either edge pages the view so arrows never dead-end at a boundary. The flat cell list is chunked into `role="row"` weeks, which is what makes the row-wise moves mean anything. `spanTypeStyles` mapped its colours to `--ant-*`, which antd's ConfigProvider emits at runtime. On an antd-free host those resolve to nothing, so the span chips lost their background and text colour entirely on /m. Every value is now a generated `--ag-*` token: the `preset-*` hue pairs already exist in palette.ts with their own dark values, so no new tokens were needed. The keyboard tests fail against the previous implementation — all four — which is the only reason to trust them.
…each other The review found the plan and kickoff pairs contradicting themselves in ways that would mislead whoever ran them next. D1 was presented as an open blocking choice in both documents while the code had already resolved it: the evaluator label is an injected prop (`EvaluatorMetricsCell` takes `displayName`) and `useEvaluatorReference` stays in the app. Recorded as resolved; D2 stays open, which is accurate. The shim policy was stated both ways — "leave thin re-export shims at every old path" against the correction that lint bans value re-exports from `@agenta/*` in oss and ee. The ban is the real constraint, so both documents now say rewrite the call sites and delete the old module. The rest are smaller but the same kind: `spanTypeStyles` was assigned to WP2 in one table and WP3 in another; a validation block `cd`-ed without returning, so every later command in it ran from the wrong directory; the antd gate and test commands carried a `web/` prefix inside a block that had already entered `web`; the DateRangePicker brief told the agent to add a direct dayjs dependency while the shipped component takes it through `@agenta/shared/utils/dateTime`; and `FilterTagsInput` appeared in the export handoff with no track owning it. Both chrome documents also embedded an absolute worktree path and a hard-coded branch. They now use repository-relative paths and tell the session to confirm its own worktree and branch.
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
Observability had no mobile surface. This adds one: traces and sessions, project-wide, at
/m/w/:workspace/p/:project/observability, with a nav entry after Agents.It is deliberately small, because WP0 through WP5 already moved everything it needs into packages. The screen is composition.
Changes
Traces reuse the packaged pieces end to end.
TracesListisObservabilityList(WP5's shell) renderingTraceRow(WP5's row). There is no mobile-only rendering of a span, so a change to how a trace reads lands on desktop and here at the same time.The range control is the same component desktop renders. The original plan scheduled an
ObservabilitySortSheetwith the ten presets. It is not here, because the chrome conversion landedObservabilityRangePickerin@agenta/observability-uifirst, which is exactly the ordering argument that motivated doing the chrome work before this WP. Same for filtering: the engine and dialog already exist, so no parallel sheet gets written.Sessions take the other path, on purpose. An observability session has no non-table rendering anywhere to extract, so mobile stacks the WP3 session cells in a layout it owns rather than inventing a shared row for one caller. The cells stay the single source of formatting. This is option (a) of the open design question in the plan; option (b), a designed session row shared by both surfaces, is a design ask and nothing here forecloses it.
Worth restating: this is spans grouped by session id, not the agent-session entity from
@agenta/sessions.SessionCardListrenders aSessionRowVmthat this data cannot fill.No scope binding.
observabilityScopeAtomandobservabilityWorkflowContextAtomalready default to project-wide with no workflow, which is precisely this screen, so binding them would only re-state the defaults.Every data-bearing component has its designed states in
states/: a skeleton that mirrors a real row's geometry so the list does not shift when data lands, an empty state, a filtered-to-nothing state with a clear action, and an error state with retry.Tests / notes
features/chat/ChatScreen.tsx, which this branch does not touch (a pre-existing prettier failure).@agenta/observabilityand@agenta/observability-uiboth grep clean for antd, which is what makes them safe to pull into/mat all.What to QA
On a phone-width viewport, at
/m/w/<workspace>/p/<project>/observability.