feat(api): page, sort and search the substrate reads server-side - #2771
Open
toreysoloio wants to merge 18 commits into
Open
feat(api): page, sort and search the substrate reads server-side#2771toreysoloio wants to merge 18 commits into
toreysoloio wants to merge 18 commits into
Conversation
The substrate page's actor and worker lists were described, in the code and on screen, as server-paged and server-ordered. Neither was true: all three of its reads called `GetSubstrateStatus`, which answers with every actor and worker in one message, and the browser paged and sorted the result. At 410,110 actors that message is roughly 43MB against gRPC's 16MB ceiling, so the page could not load at all, and the split into three reads was cosmetic. `SystemService` gains `ListSubstrateActors`, `ListSubstrateWorkers` and `GetSubstrateSummary`. The two list calls pass a page token straight through to ate-api's own pagination; the summary walks every ate-api page but keeps only tallies, so its answer is a handful of integers and has no size ceiling. Counts live there and nowhere else — a page's length is not a total. ate-api offers paging and nothing else: no order, no filter, no total. So the list RPCs offer nothing else either, because a controller-side sort or filter would have to read the whole inventory to apply it, which is the read being removed. The tables sort and search the page in hand and say so in three places: the note beneath each table, the heading that separates "1 of 4 on this page" from "100 of 4,312", and the empty state that names what it searched. Reordering is now a re-render rather than a re-read of every actor in the cluster. Two further defects surfaced while tracing this: - `ListWorkers` read one ate-api page and dropped the token, so any fleet past ate-api's page ceiling was silently truncated and reported as complete. - `busyWorkerCount` was always zero. `workerFromProto` never set the actor fields, and could not: ate-api's `Worker` carries capacity and allocation and no actor reference, since the binding lives on the actor. It is now counted from the summary's actor walk, and the workers table drops its permanently blank Actor column — which looked populated only against a fixture that had invented the field. `GetSubstrateStatus` stays, deprecated, and the UI no longer calls it. Refs kagent-dev#2704 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
… page Review of the paged substrate reads turned up five defects, four of them ways a partial failure was reported as a complete answer. `ListSubstrateActors` and `ListSubstrateWorkers` collected rows across several ate-api pages and then, if one failed, returned what they had with an empty `next_page_token`. Twelve rows and no Next button, beside a tile counting four hundred thousand. The token handed back is now the failed page's, so a retry resumes there; it stays empty only when nothing was collected, where "next" would point at the page just asked for. Those two also asked ate-api for the full page size on every read while keeping what earlier reads had collected, so a page could come back larger than the one requested — which the proto caps at 100. Each read now asks only for what is still missing. Both loops are one shared collector rather than two copies. `GetSubstrateSummary` read the harnesses from PostgreSQL inside the ActorTemplate listing, so a database outage reached the reader as "ate-api answered with an error" while ate-api was healthy. Worse, every count was gated on that same field, so one failed read reported a cluster of 410,110 actors as running none. The database read is separate now and a failure there is an internal error; the three ate-api reads no longer gate each other, and each contributes what it reached. The counting walks had no page bound. Draining used to be limited by a single call deadline over the whole loop; a per-page timeout is right, and it left nothing to stop a cyclic `next_page_token` spinning against ate-api until the inbound request was cancelled. Both walks are one bounded helper. On the page, a heading with no total to show rendered the page's own row count bare — indistinguishable from a total, and exactly what `PagedSectionTitle` exists to prevent. It keeps "on this page". The workers tile is locale-formatted like the actors tile above it. The mock summary returns an ate-api error beside complete counts, which the controller could not produce until these reads were made independent. It can now, and the fixture says which state it is modelling. Refs kagent-dev#2704 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
Answering "are helper functions being used when logical?" turned up one place where they were not, and it was hiding a defect. `substrate/list.go` carried three copies of the same drain loop — actors, workers, templates — none of them bounded, and none checking whether the token was advancing. The UI has drained `ListAgentInstances` with both a page cap and a non-advancing guard since it was written, and the comment there says why the guard belongs on the second read rather than at the cap: that is where the reason is still obvious. The Go side had neither. All three now go through one `drainPages`, and `AdvancePageToken` is exported so the service's two loops — which visit pages rather than collecting them, and so cannot share the drain — make the same check. `ListActorTemplates` gains the paged read the other two already had, which also gives each of its pages its own deadline instead of one across the whole drain. The rest were duplication without a defect behind them, collapsed while the reason to was in view: - `SubstratePage` had four memos doing two things twice. One `usePagedRows` hook now returns the page and what the search box leaves of it, so the two tables cannot drift into filtering or ordering by different rules. - The two paged transport operations built the same request and unpacked the same envelope inline. A `pageSize` defaulted one way in one and another way in the other is exactly the difference nothing would notice. - The two mock list handlers were the same handler around a different row type. No behaviour changes beyond the bound and the guard. Refs kagent-dev#2704 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
Three steps in the agent rail spec opened a dropdown, paused for 400ms, and then clicked a menu item without anything having waited for it to exist. A fixed pause is long enough on an idle machine and not on a loaded one, so the delete step failed about a third of the time under a full parallel run — reaching for a menu item whose dropdown had not opened yet. Waiting for the item is both shorter and stricter: Playwright already refuses to click something still animating, so the pause was never what made the click safe, only what made it late. Step 1 of the same test had the wait and did not flake, which is what pointed at the other three. Found while rebasing the substrate paging work, which shifted load timing enough to expose it. The rate on that branch goes from 3 failures in 9 runs to 0 in 8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
…selves Review found seven places where the code said something that was not true of it. Two change behaviour; the rest are the words. The workers table numbered its multi-sort priorities backwards. antd applies sorters by *descending* `multiple`, so the highest number leads — as the actors and the two inline tables already had it — and the workers gave IP the highest. Shift-clicking Pool then IP grouped by IP, against the comment two lines above saying pool leads. `busyWorkerCount` and `workerCount` are one fraction on one tile and were scoped by different keys: busy pods came from actors filtered by their template's atespace, the total from workers filtered by their pod's namespace. An actor in atespace `team` on a pod in namespace `kagent` counted in the numerator and not the denominator, rendering "1/0". The rest are documentation that had drifted from the code beneath it: - `GetSubstrateSummaryResponse.enabled` promised the Kubernetes-derived fields were still answered with ate-api unconfigured. The call returns before it reads them, as `GetSubstrateStatus` does. - The partial-read banner said actor templates come from Kubernetes. They come from ate-api, and a failed template listing is one of the three reads that raises that banner — so it claimed an empty table was complete. - The page and the mock both said a page carrying rows *and* an error was impossible. Filling one page can take several ate-api pages when a namespace narrows it, and a failure part-way keeps what it collected. The warning now says the read did not finish rather than that it failed, and the state is recorded in DEFERRED.md as having no fixture. - The actors' empty state said ate-api reported nothing in scope, where a page can also end with no in-scope rows and more pages behind it. - The int64 inventory in `wire.ts` names line numbers that the schema had moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
A second review pass. One correctness fix, two the reader would have been misled by, and two comments that described something the code does not do. `collectSubstratePage` refused to hand back a resume token unless it had collected a row, where what it meant was "unless it advanced past a page". Filling one page can read several of ate-api's, and a read can advance past one while keeping nothing from it — every row out of scope, or a page ate-api answered empty while still holding a token, which it says it may do. A failure after that returned an empty page with no token, so `PageControls` hid itself and the rest of the list was unreachable. It now resumes from the token it reached. The actor and worker tables told a reader "ate-api reported no actors in this scope" when the read behind that page had failed, which is a claim about the cluster made on the strength of a read that did not happen. Both now say the page could not be read. `ateApiEnabled` came from the summary alone, and the summary is the read most likely to fail: it walks every ate-api page, which this branch documents as seconds on a large cluster. When it timed out while the two cheap page reads succeeded, the page told the reader their controller had no ate-api endpoint — a different problem with a different fix. The page reads carry the same flag. The multi-sort comment claimed shift-clicking two headers sorts by both. antd reads no modifier: `triggerSorter` appends whenever the clicked column and the current head both carry a number, so any second click accumulates. The behaviour is unchanged and shared by all four tables on the page; the comment now says what it is, including that the leading column has to be cycled off before another sorts alone. The summary is documented as the read to poll least often, and this page ticks it alongside the two cheap ones. That is deliberate — totals that held still while the rows moved would be two moments shown as one — so the guidance now says what it costs rather than something the page contradicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
…ages
`common.proto` has carried `PageRequest`/`PageResponse` since before this branch,
and every other paged read takes them: ListCheckpoints, ListAgentInstances,
ListAgentInstanceShares, ListScheduledRuns, ListScheduledRunExecutions. The two
substrate list calls were the only ones spelling out `page_size`, `page_token` and
`next_page_token` themselves — down to re-declaring the `lte: 100` cap that
`PageRequest.limit` already carries, so the same rule was written in two places
and could disagree.
They now take `PageRequest page` and answer with `PageResponse page`, which also
puts the UI on the shape it already uses for conversations: `page: { limit,
pageToken }` out, `response.page.nextPageToken` back.
`SubstrateListInput.PageSize` becomes an `int` for the same reason — that is what
the agent-instance and checkpoint services take, with the int32 kept for ate-api's
own request where it belongs.
No behaviour changes: the cap, the default, and the refusal above the cap are
where they were, and `GetSubstrateSummary` keeps the counts, since `PageResponse`
carries a token and nothing else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Torey Scheer <torey.scheer@solo.io>
…age reads It is not deprecated, and saying so was not this branch's call to make. The scheduled-run e2e polls it to find one actor by id and wait for it to suspend, and `system_feedback_test` calls it too — the UI stopped using it, the repository did not. Marking it `option deprecated = true`, which is how a schema says this rather than in prose, would fail `make -C go lint`: staticcheck's SA1019 flags exactly those callers. The comment now describes the size ceiling and points at the paged calls without claiming a status nobody granted it. The live spec had gone the other way. It documented the page as reading `GetSubstrateStatus` and nothing else, describing the three RPCs as having been removed — which this branch restores, so the comment said the opposite of what the page does. It also explains what a live run is for, and the reason has changed with the shape: a mock pages an array it holds in memory, while the controller passes a token through to ate-api, and only a cluster says whether that token means what the page thinks it means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
Three public endpoints went in with only service-level tests behind them, which never pass through the generated client or the interceptors. This runs all three over bufconn beside the reads that were already covered there. What only this level can say: that each is in the method-policy map, that the shared PageRequest/PageResponse survives the round trip, and that an unconfigured substrate is an empty answer rather than an error. It also pins the page-size cap where it now lives — on PageRequest.limit rather than on these requests — by asking for 101 and requiring InvalidArgument from the interceptor, before any handler and before the service's own guard can answer instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
antd's Text is a span and PageControls is an inline-flex Space, so the two sat on one line: the buttons began at the exact pixel the sentence ended, and the top margin the controls carry to separate them applied to a box already beside it rather than below. Measured on a live cluster at 105 workers, where the note ends at x=613 and the pager starts at x=613. The note is block now, which drops the controls onto their own line and lets that margin do what it was for. It also gains a smaller margin of its own, because being inline had hidden that it sits flush against the table: the rhythm is 8px from the table it describes and 12px to the controls, which are their own thing. Only visible with enough rows to page — the controls hide themselves on a single page, so the two were never on screen together until now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
toreysoloio
force-pushed
the
ui-substrate-paging
branch
from
September 10, 2026 16:02
01e2ecf to
55d7130
Compare
toreysoloio
marked this pull request as ready for review
September 10, 2026 19:32
toreysoloio
requested review from
a team,
Charlesthebird and
peterj
as code owners
September 10, 2026 19:32
…ntrols" This reverts commit 5e658ec. Signed-off-by: Torey Scheer <torey.scheer@solo.io>
… list has them Every other list on this app pages through antd's own control, which draws itself at the bottom right with its summary to the left on the same row. The substrate tables turn their pages by token rather than by number, so they carry their own control — and it sat at the bottom left, which is a second place to look for the same thing. The note saying what the sort and the search reach now takes the left of that row and the controls the right, which is the arrangement antd's `showTotal` and pager already make everywhere else. `PageControls` keeps the top margin it carries for the stacked layout it was written for — the scheduled runs page still uses it that way — so the row clears it on the control and owns the spacing itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
The bar draws a segment per actor below a count of eighty, each with a 6px floor so a single crashed actor among four hundred thousand is still something to point at, in a row that does not wrap. Its width is therefore set by the cluster rather than by the window: eighty actors want 717px, and at 1024 the sidebar expands and leaves the track 686px. The bar forced its container wider and took the page with it — at 768 by 115px, at 375 by 508px. Eighty actors on a laptop, not a synthetic number. Room is now the second limit beside the count: the track is measured and the per-actor drawing only happens while every segment fits, falling back otherwise to a segment per status sized by its share, which is what a large cluster gets anyway. Measured in a layout effect so the answer is in before the first paint rather than after a frame of the overflow this prevents, and the floor and the gap are named constants now, since the capacity is arithmetic over exactly the two numbers the CSS uses. A zero width is ignored rather than believed. The observer reports one as the element detaches, and taken at face value it overwrote a good measurement with nothing — the bar went back to drawing every actor on a track that could not hold them, which is how this first appeared to be unfixed. The test asserts the track rather than the page: these tables carry a horizontal minimum of their own, so the page scrolls sideways below about 1100px whether or not a single actor is on it, and asserting there would be asserting on that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
…e inventory Ordering and searching reached one page. A reader who sorted by status saw the first row of the hundred in front of them rather than of the cluster, and a search for an actor on the ninth page was told there were no matches — which is worse than no search at all. The page said so in three places, which made it honest and not much more useful. `ListSubstrateActors` and `ListSubstrateWorkers` now take a filter, a sort field and a direction, and answer with the matching total and the order they applied. ate-api offers none of the three, so the controller reads every one of its pages, narrows and orders all of them, and cuts the page from that. The answer is still one page, so nothing here reintroduces the message that gRPC refused to send; what it costs is a walk of the inventory per request, which is the price of an order that means the cluster. The page token becomes an offset rather than ate-api's cursor, because the order is now the controller's and is rebuilt per request: a key naming a row's position in one ordering is meaningless in the next. Every sort key ends in a unique column — the actor id, or the worker's namespace and pod — so a page boundary names exactly one row rather than dropping or repeating whatever shared the key. On the page the filter is debounced and both it and the sort are back in the read key, so a keystroke or a header click is a new read rather than a re-render of what was already fetched. The headings report the matching total again, the note beneath each table says which order was applied rather than which was asked for, and an empty search says "anywhere in this scope" because now it is true. The mock filters and orders before it cuts, in that order. Doing either after would let a page-scoped regression pass its tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
The actor and worker tables offered two ways through the same rows: a pager under the card, and a scrollbar inside a fixed-height body. Reaching row forty by one of them skips what the other turns to, and neither says so. The scrollbar was there because the page asked for a hundred rows -- the controller's maximum -- and a hundred rows had to be absorbed somehow, so the table windowed them. Twenty-five is what every other list in the app pages by and what fits on screen, so nothing needs absorbing: `virtual` and the `y` in `scroll` are gone, and `GROWING_TABLE_HEIGHT` with them. The `x` stays; it is the sum of the column widths, and without it the columns squash. Dropping `virtual` returns these two to real `table` markup. The hover- suppression rules were already class-based rather than `tr:hover > td`, so they still reach the rows -- which the spec covering all four tables confirms. The spec that asserted a virtual holder exists and is bounded now asserts the opposite. It asks whether any descendant both allows vertical overflow and overflows, rather than naming `.ant-table-body`: without a `y` antd renders no such element, so a test that named it would pass by failing to find its subject. Comments describing the old page size and the page-scoped sort went stale two commits ago and are corrected here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
…does Ten of them described the design the last few commits replaced. Two were actively misleading: The block introducing the four search boxes still argued that actors and workers "narrow the page, and nothing here pretends otherwise", three lines above the code that debounces both terms and sends them to the controller. Above the actor columns sat a whole orphaned block claiming "A comparator rather than `sorter: true`", followed immediately by the block that correctly describes `sorter: true`. Two adjacent comments contradicting each other. The rest, in the order they appear: - `ALL_NAMESPACES` cited `GetSubstrateStatusRequest`, an RPC this page no longer calls. - `MIN_POLL_SECONDS` and the poll tick both called the list reads cheap against an expensive summary. All three walk every ate-api page now; the summary is dearer only because it walks three times rather than once. - `filterRows` described narrowing four lists; two are narrowed by the read. - `byText` illustrated multi-sort with Status and Template, which are the actor table's columns and carry no comparator. - `SectionTitle` attributed both totals to the summary RPC; the paged two come from the list response's `totalSize`. - `PageWarning` said the rows beneath it may be partial. A failed walk has an inventory it cannot order or count, so the page comes back empty. - The docblock said each list read passes a token through to ate-api's paging. The token is the controller's own offset, and what three reads fixed is the size of the answer rather than the cost of producing it. - Two "hundred rows" references, from when the page was a hundred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
The backend this branch added carried far more prose than the code around it. system.proto was the outlier: 80 comment lines on a file that had none before, in a directory whose densest peer has 16. What went was narrative — bugs that no longer exist, and reasons restating the code beneath them. The eleven lines on maxATEPagesPerWalk are three; the ten on why the harness read moved out of the template listing are two saying what the split buys. What stayed is what a reader cannot infer: that scoping busy workers by atespace rather than pod namespace renders the tile as "1/0"; that every sort key ends in a unique column because a repeating last key gives a page boundary naming more than one row; that the page token is an offset because the order is rebuilt per request. On the proto, the contract — what enabled=false means, that ate_api_error is partial rather than fatal and never a database failure, what the filter matches, the limit cap, why total_size and applied_sort_* exist. substrate.go's file header went because it was also wrong: it still said ordering a page is the caller's, which is what the file stopped doing. Regenerated. The only non-comment change in the generated output is one struct field realigned by gofmt; the TypeScript has none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
`filter` is declared `max_len = 200` in system.proto and the Protovalidate interceptor refuses the request above it, so pasting anything long into the actor or worker search replaced the table with "Actors could not be read", over a Try again button that re-sent the same rejected request. The two server-backed boxes now stop at 200. Also clamps busy workers to the worker count. The two come from different ate-api walks and the walks deliberately do not gate each other, so a failed worker walk beside a successful actor one rendered the tile as "2/0". The rest is smaller: - `ateApiError` on the domain page type promised rows and a resume token beside the error. A failed walk has an inventory it can neither order nor count, so the page is empty and the token absent. A test comment repeated the same claim. - The mock ordered rows with `localeCompare` where the controller uses Go's byte-order `strings.Compare`, so fixtures could show an order no cluster would. - The actor table's `scroll.x` is documented as the sum of its column widths and was 930 against a sum of 1070. antd reads it as a minimum, so nothing was mis-rendered, but the number was wrong. - Two fields on the test fake that nothing set or asserted are gone; the case one of them was added for is covered by a test instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
toreysoloio
force-pushed
the
ui-substrate-paging
branch
from
September 10, 2026 19:34
d24a9d3 to
206305c
Compare
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.
workers table with enough entries to trigger pagination
🤖 written by Claude (start)
Changelog
Substrate actors and workers load on clusters too large for a single inventory read: each list is paged, sorted and searched server-side, across the whole inventory.
Closes #2704
Testing
cd ui && VITE_API_MODE=mock yarn dev, open/substrate.7f21in Actors: the heading counts every match in the scope, not just the ones on this page.Additional Notes
ate-api offers paging and nothing else — no order, no filter, no total — so the controller reads every one of its pages, applies both, then cuts the page it answers with. That costs a walk of the inventory per request, which is the price of a sort and a search that mean the cluster rather than the rows on screen; only a page and some integers cross the wire, which is what
GetSubstrateStatuscould not do at 410,110 actors.The Workers table loses its Actor column (#2709): ate-api's
Workercarries no actor reference, so the column readidleon every real cluster. How much of the fleet is busy is a tile instead.Removing the walk needs an upstream change: ate-api stores each row as an opaque
proto byteablob, so status, template and pod are invisible to SQL. Promoting them to columns — a migration plus a Go backfill, since Postgres cannot decode the blob — would make the filter aWHERE, the sort anORDER BYand the totals acount(*), and the API here already has that shape, so the controller would stop walking and pass the parameters through. Two smaller ones stand alone: exposing the actor reference onWorkerneeds no migration, since the indexedworker_assignmentstable already holds it, and would restore the column above; and aGetSubstrateActor(id)call would remove the last caller needing a whole-inventory read, lettingGetSubstrateStatusbe deprecated.🤖 written by Claude (end)