feat(ui): rewrite the web interface on Vite, and give conversations a first-class API - #2569
Merged
Conversation
Charlesthebird
force-pushed
the
charlesthebird/uiRewrite
branch
from
August 25, 2026 23:43
9d67c52 to
6426683
Compare
github-actions
Bot
added
enhancement
Charlesthebird
force-pushed
the
charlesthebird/uiRewrite
branch
4 times, most recently
from
August 26, 2026 00:25
06e71ff to
11aaee0
Compare
Collaborator
Author
kagent-ui-crud.mp4🤖 written by Claude |
Collaborator
Author
kagent-ui-agent-chat.mp4🤖 written by Claude |
Charlesthebird
force-pushed
the
charlesthebird/uiRewrite
branch
4 times, most recently
from
August 26, 2026 01:50
8264ec6 to
ec60b82
Compare
Charlesthebird
marked this pull request as ready for review
August 26, 2026 02:21
EItanya
reviewed
Aug 26, 2026
| }, nil | ||
| } | ||
|
|
||
| func (s *grpcServer) RenameAgentInstance(ctx context.Context, request *apiv1alpha1.RenameAgentInstanceRequest) (*apiv1alpha1.RenameAgentInstanceResponse, error) { |
Contributor
There was a problem hiding this comment.
Do we still need this? I think we're doing auto-suspend/resume
| // InterruptActiveAgentInstanceTask fails the expected task and records an | ||
| // interruption. It returns false if that task is no longer active. | ||
| InterruptActiveAgentInstanceTask(context.Context, string, string) (bool, error) | ||
| // AbandonActiveAgentInstanceTask cancels the expected task, releasing the |
Contributor
There was a problem hiding this comment.
This is A LOT of new queries, do we need all of these just to get the UI off the ground. These are the sort of thing that are harder to roll back
| return &agentTemplateServer{service: service, maxMessageBytes: maxMessageBytes} | ||
| } | ||
|
|
||
| func (s *agentTemplateServer) ListAgentTemplates(ctx context.Context, request *apiv1alpha1.ListAgentTemplatesRequest) (*apiv1alpha1.ListAgentTemplatesResponse, error) { |
Contributor
There was a problem hiding this comment.
All of the CRUD functionality is identical, can we use generics?
| if config.AgentService != nil { | ||
| apiv1alpha1.RegisterAgentServiceServer(grpcServer, newAgentServer(config.AgentService, config.MaxMessageBytes)) | ||
| } | ||
| if config.AgentTemplateService != nil { |
Contributor
There was a problem hiding this comment.
if x == nil is a claudism, this can't be nil here
Comment on lines
+216
to
+218
| if s.config.GrpcWebRouter == nil { | ||
| return next | ||
| } |
Contributor
There was a problem hiding this comment.
claudism, no need for this
| // more than one binary serving HTTP beside that server, and a second copy of | ||
| // the rule would drift. | ||
| // | ||
| // Optional: left nil, this server behaves exactly as it did before one existed. |
Contributor
There was a problem hiding this comment.
Is it actually nil though, don't we pass it every time?
Contributor
There was a problem hiding this comment.
This is also identical to harness.Service. Can we use generics?
Contributor
There was a problem hiding this comment.
A concrete way to keep this small is the prototype pattern used by skv2: parameterize the service with T client.Object and L client.ObjectList, pass &v1alpha3.Harness{} / &v1alpha3.HarnessList{} (or the AgentTemplate equivalents), and allocate with prototype.DeepCopyObject().(T). See https://github.com/solo-io/skv2/blob/main/pkg/client/client.go. That should share the CRUD mechanics without reflection or constructor plumbing; only the resource-specific spec/status transition needs a small callback.
| // | ||
| // Asked rather than assumed: a session share reaching the A2A gateway must not be | ||
| // treated as authority over an instance that happens to share its id space. | ||
| func (s *ShareContext) IsForAgentInstance(instanceID string) bool { |
Contributor
There was a problem hiding this comment.
Sessions are dead, just replace this
EItanya
reviewed
Aug 26, 2026
| rpc ListHarnesses(ListHarnessesRequest) returns (ListHarnessesResponse); | ||
| rpc GetHarness(GetHarnessRequest) returns (GetHarnessResponse); | ||
| rpc CreateHarness(CreateHarnessRequest) returns (CreateHarnessResponse); | ||
| rpc UpdateHarness(UpdateHarnessRequest) returns (UpdateHarnessResponse); |
Contributor
There was a problem hiding this comment.
Can we omit GetHarness and UpdateHarness from this PR? The rewritten UI calls list, create, and delete but has no consumer for either RPC. They pull later CLI and apply API surface into the UI cutover and add proto, generated, handler, service, policy, and test code speculatively. Add them with the consumer that needs them.
EItanya
reviewed
Aug 26, 2026
|
|
||
| // substrateCache memoises the substrate reads for substrateCacheTTL. | ||
| // | ||
| // The singleflight group is the other half of the point: without it, the three |
Contributor
There was a problem hiding this comment.
This cache does not collapse the three reads described here: summary, actors, and workers deliberately use different keys, and singleflight only coalesces equal keys. Its 400ms TTL also expires before normal polling, while the UI client already deduplicates identical reads. Can we delete this cache and compute the requested result directly until measurements show duplicate same-key calls are a real production cost? That removes the any cache, locking, eviction, and about 280 lines including tests.
EItanya
reviewed
Aug 26, 2026
| @@ -0,0 +1,720 @@ | |||
| package system | |||
Contributor
There was a problem hiding this comment.
Can we split the Substrate inventory scalability work into a separate PR? The new summary/paged RPCs, streaming selector, cache, proto surface, and tests are roughly two thousand handwritten/test lines plus generated output. This is a substantial independent backend feature and makes the UI transport/conversation changes much harder to review atomically.
EItanya
reviewed
Aug 26, 2026
| // Optional display name. Omit it to create an unnamed conversation. Unvalidated | ||
| // because empty is the ordinary case: a conversation is usually named later, or | ||
| // never. | ||
| string name = 5; |
Contributor
There was a problem hiding this comment.
This request-intrinsic validation belongs in the proto, per the shared Protovalidate interceptor. max_len handles the 200-character bound and CEL can reject surrounding whitespace and control characters while allowing empty names. That lets us delete validateName; its UTF-8 check is redundant because protobuf strings are already valid UTF-8. The new template and harness filter validation should move here as well rather than being duplicated in the service.
EItanya
reviewed
Aug 26, 2026
| // working state so a reply can be delivered, returning the task as it was | ||
| // parked and whether this call claimed it. A second caller is refused, which | ||
| // is what stops a duplicate reply being delivered twice. | ||
| ClaimParkedAgentInstanceTask(context.Context, string, string) (*a2a.Task, bool, error) |
Contributor
There was a problem hiding this comment.
ClaimParkedAgentInstanceTask and RestoreParkedAgentInstanceTask are added and extensively tested, but no production caller uses either one. prepareReply still does GetAgentInstanceTask followed by StoreAgentInstanceTaskEvent, so two concurrent replies can both observe the parked state and both dispatch. Please either make prepareReply use the claim and restore replay guard, or remove the unused interface, SQL, implementation, and tests; the current version pays for both approaches while retaining the race.
… first-class API The Next.js app is replaced by a Vite + React 19 single-page app. Routing is React Router's, reads go through SWR, components come from antd 6 and styling from Emotion. Settings reach the app at runtime from `window.environmentVariables` rather than being frozen into a build, so one image serves every deployment. The pages follow the v1alpha3 model rather than the one the old app was written for. An agent is not a resource: it is what exists once a Harness admits an AgentTemplate, read out of `AgentTemplate.status.harnesses[]`. The agents landing page says so, in an overview that maps the four concepts — AgentTemplate, Harness, Agent, AgentInstance — onto the Agent Substrate words for the same things, and its three tabs are the way to each. An agent's own page lists its conversations, and a conversation is the chat. Chat runs over A2A on gRPC-Web, which needed a server-side half: - `AgentInstance` gains a name, end to end — migration, sqlc, store, proto, service, gRPC handler and policy — so a conversation can be called something. Additive: an empty name renders by id. - `ListAgentInstances` takes a query rather than six positional arguments, and can narrow to one agent by template and harness. The pair is resolved through `prepared_revision`, so no new column is needed and rows written before the filter existed still match. - The A2A gateway serves an instance in any state for reads, so a suspended conversation can still be opened and its transcript read. - A share resolves to the share *and* the instance's owner, because that is what a share grants: the reader stays themselves, and the token widens what they may read. - The gRPC server can hand its gRPC-Web handler to the HTTP server, so a browser reaches the services over the origin it was served from. The split happens outside the router's middleware chain: that chain is for REST-shaped handlers, and a gRPC-Web frame neither survives it nor needs it. Test coverage is 376 unit tests and 88 browser tests, all against the real pages. What could not be covered honestly is written down in `ui/playwright/DEFERRED.md` with the surface each spec is waiting on, rather than committed as a skipped test — a skipped spec reads as coverage and that list does not. `ui/dev-scripts/` builds a Kind cluster with kagent on it in one command, for anyone who wants to try the app against a real backend. Without a cluster, `ENABLE_MOCK_UI=true` serves every page from in-browser fixtures, and each such page says on itself that the data is not real. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
A tool call's payload reshuffled its properties while it was being read. The args arrive as a protobuf `Struct` flattened to plain JSON, and a `Struct` carries no field order — for a Go map, a different one every marshal — so re-reading the transcript during a live turn redrew an unchanged payload in a new order each time. Its keys are sorted at every depth now; array order is left alone, because there the order is the data. The fixture supplies its args out of alphabetical order on purpose, so the assertion tests the sort rather than the order it was handed. Reloading a conversation just started sent its opening message a second time. `AgentNewChatPage` hands the first message to `AgentChatPage` in `location.state`, which the browser keeps in the session history entry rather than in memory: it came back with the page, and the effect that sends it fired again. The comment there already said the entry had to be cleared once the turn was under way; it never was. Three places the page knew which box the reader was about to type in and made them click it first: the new-conversation page, a conversation opened from the rail, and a parked question whose single prose field is the only thing that can end the turn. That field takes Enter now, and hands the caret back to the composer afterwards — it exists only because the composer could not end the turn, so once it has, the next thing typed is an ordinary message. Only the single-field shape: with several questions, taking the caret would be choosing which one gets answered first, and Enter would send the one still being filled in. An open conversation needs an effect rather than `autoFocus`, because its composer mounts disabled while the instance is still being fetched, and a focus before the box can be typed in is no focus at all. It fires once per conversation, so one coming back from suspended cannot take the caret from wherever the reader has since put it. The notes under the conversation, model, MCP server, prompt and template tables explaining which side of the wire narrowed their rows are gone. The constraint they documented is real and now lives only in `playwright/DEFERRED.md`, rewritten to say so: these lists narrow in the browser, which is honest only while the RPC returns every row. The dashboard's recent-activity card is retitled "Recent agent conversations", which is what it lists. `KAGENT_DEV_CONTROLLER_URL` can be set in `.env` now, where a reader would look for it. `vite.config.ts` read it only from `process.env`, and Vite does not put `.env` values there, so a value put in the file did nothing. `.env.example` documents pointing it at the UI pod's nginx on 8080, which needs no port-forward beyond the one `dev-scripts/setup-cluster.sh` already holds open, and leaves it commented: unset still means the controller on 8083. And `.gitignore` covers `.next/`, `next-env.d.ts` and `playwright/test-results/`, left behind by the app this one replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
EItanya
force-pushed
the
charlesthebird/uiRewrite
branch
from
August 26, 2026 14:42
b589ca8 to
d03656f
Compare
…arallel `workers: 1` in CI was buying real stability, and this is what it was hiding. Seven full-suite runs at high concurrency produced five failures on five different tests, about one per run. Each is now diagnosed rather than retried away. Two were the suite's own fault, and were passing for the wrong reason rather than merely flaking. `agents: pressing a row looks different from hovering it` and `mcp servers: the row and its expand control behave as one` read `getComputedStyle` in the same tick as the `mouse.down()` that was supposed to change it. antd transitions a table cell over 200ms, so — measured on an idle machine — the value immediately after the press is still `rgba(0, 0, 0, 0)` and only reaches `rgba(109, 40, 217, 0.3)` some 400ms later. The "a press must not look like a hover" assertion was therefore comparing two unpainted frames and passing because they happened to differ; the same read under load returned the same colour twice and failed. A claim that something changes now polls until it does, and a claim that nothing changes waits the transition out first — there is no event for a transition that never starts, which is why the second is a duration. `helpers/style` carries the measurements. Both assertions were checked against a deliberately broken `:active` rule afterwards, so polling has not made them unfalsifiable. Two more measured the machine and reported it as the app. Both substrate polling tests counted re-reads inside a fixed window with no margin: three within 2.2s at a half-second rate, which allows four, and two within 2.2s at a one-second rate, which allows exactly two. One late tick failed either. They wait for the re-reads now. The cadence itself is deliberately not asserted — it cannot be, from outside, without also asserting the hardware — and the exact end of the claim is kept where it belongs: at zero the timer must not fire at all, and "never" does not depend on how fast anything is. The fifth was budget, not logic: `Test timeout of 30000ms exceeded` partway through a six-step journey that had done nothing wrong. The mock suite gets sixty seconds. `expect.timeout` stays at its default five, so a genuinely missing element is still found quickly and a real failure does not sit here spending the larger budget. One remains unattributed — a share link taking longer than fifteen seconds to appear on Firefox under twelve workers. It is starvation rather than a race, and `retries` stays at two deliberately: raising concurrency without keeping the net would trade a slow suite for a suite that fails on other people's pull requests. CI now takes a share of the runner rather than a count. `ubuntu-latest` is four cores, where a flat number would thrash with two engines running, and a bigger runner should get the benefit without another edit. Two `list-filters` tests are gone, and no coverage with them. `FilterBar.test.tsx` already owns the bar's own behaviour in eleven cases — a pill per chosen value, a pill removing only its own filter, the last pill taking the parameter with it, clear dropping the lot, the term reaching the address — and those claims were being made a second time in a browser, in two engines, at a page load each. What is left is what a jsdom test cannot say: that a page wired the bar to its own read, that the view survives a real reload, that a genuinely server-side filter is sent to the server, and that two writes in one tick do not lose one of each other. The file's own header said the pages carried a note about where narrowing happens; those notes were removed in the previous commit, so it said something untrue and now describes what is there. Also reworded the agents list note, which read as though the configurations rather than the agents were available. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Reported as a defect: in a conversation several turns long, sending a message and then clicking away from the browser and back put an earlier agent reply underneath the newest one. It took a couple of attempts to provoke and a refresh cleared it, which made it look like a rendering fault. It was not. The gateway does not name an agent reply, and `messagesFromTask` gave an unnamed one an id from a process-wide counter. So the same reply came back as `history-1` on one read and `history-3` on the next. `useLiveTranscript` re-reads the transcript on `visibilitychange` — and every four seconds while the tab is visible — and the merge treats the id as identity, so on every re-read the copy already on screen stopped matching what the server sent and was kept as a *local addition*. Local additions are appended after the server's messages, on the reasoning that the only way to have one is to have just sent it. A reload dropped the local copy and reset the counter together, which is exactly why refreshing put it right. Ids for unnamed messages and artifacts are derived from the task and the position in it now. Position is stable for the same payload and unaffected by the task gaining later messages, which is all the merge needs. The counter stays where it belongs: on the stream, where an id is minted once for a message being written and upserted under it. The test asserts two reads of the *same* task, because one read cannot show this. Without the fix it reports `['m1', 'history-1', 'artifact-2']` becoming `['m1', 'history-3', 'artifact-4']` — the named message stable, the unnamed ones renamed underneath it. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The rail rendered in whatever order `ListAgentInstances` answered in, which is an order in no particular order — so a conversation started a minute ago could sit anywhere in the list. By when a conversation was **started**, and deliberately not by when it was last spoken in, which is the more useful ordering and is not available here. `AgentInstance.updatedAt` looks like the field for it and is not: `agent_instance` carries no timestamp columns at all, the two on the message come out of the row's serialized blob, and `UpdatedAt` is written in exactly two places: when the instance is created, and when it goes `CREATING` to `READY`. Sending a message never touches it, so ordering by it would be creation order wearing a better name. The signal that would answer it is `agent_instance_task.updated_at`, bumped on every task upsert. It is on the task table and `ListAgentInstances` does not return it, and reaching it from the browser costs one `ListTasks` per row — which is why `useConversationTitles` budgets thirty of those for titles alone. When the read grows a last-activity timestamp, the comparator is the one thing that needs to change; the note against it says so. Ties break on the id, so equal timestamps give one fixed order rather than whatever the sort happened to do with them. A rail that reshuffled equal rows between reads would be the same defect arriving by a different door. Two things this shook out. The suspended fixture's timestamp is now load-bearing: it is older than its named sibling and renders first when nothing sorts, so the new test cannot pass against an unsorted rail — checked by removing the sort. And the delete test was targeting `.first()`, an unstated dependency on render order: sorting made it the *open* conversation, and deleting the conversation you are looking at navigates away, so the test counted rows on a page it had left. It names the sibling now, which is the row it was always about — one that goes without taking the page with it. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
… nothing reads The settings documentation described another product rather than this one, in a public repository, and named three of its configuration keys to do it. `UI_BACKEND_HOST` and `UI_BACKEND_TOKEN` read as *this* application's backend, which is the opposite of what they were: an API an extension reaches instead of `API_BASE_URL`. `UI_BACKEND_HOST` was documented as a management plane and `LOCAL_CLUSTER_NAME` as the cluster that management plane is installed on, with `mgmt-cluster` as the example value and the same string in `env.test.ts`. That is another product's architecture, and this application has no opinion about it. None of the three is named here any more, because none of them is this repository's business. An extension's settings are a prefix now rather than a list: anything called `EXTENSION_*` is passed through by the dev server and by `scripts/init.sh`, and read back with `readEnv`, which already took any key against an already-open record. So an extension gains a setting by naming one, with no change here — where before, a public repository had to be edited to add a key nothing in it reads. The four `OIDC_*` keys are gone rather than renamed. Nothing here read them: they appeared only in the pass-through list, and `git log -S` puts them in `ui/` for the first time in the rewrite. The comment against them claimed the app "runs the authorization code flow itself rather than expecting an authentication proxy in front of it", which is the reverse of the truth — the one implemented source is `oauth2ProxyAuthSource`, there is no PKCE anywhere in the app, and what it supports is exactly the proxy that comment disclaimed, configured through `SSO_REDIRECT_PATH`. `.env.example` carried twelve lines of setup guidance for that absent flow, down to a Keycloak URL and a realm belonging to somebody's own machine. The prefix is still bounded, which is the whole point of `CORE_ENV_KEYS` being a list: the dev server inlines these into the document, so a wholesale copy of the environment would publish every credential on the machine into the HTML. A variable now has to be deliberately named for an extension — but only that deliberately, so a stray `EXTENSION_` in a shell will be inlined too. The comment says so rather than implying a guarantee it does not give. `init.sh` was run against a value carrying quotes, a backslash and a `<script>` element to check the escaping still holds when the keys are no longer known in advance, and against an environment with none set, which must not leave a trailing comma. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
`refactor: simplify UI backend support` removed `GetSubstrateSummary`, `ListSubstrateActors` and `ListSubstrateWorkers`, and `GetSubstrateStatus` returns the whole inventory in one message again. `api/grpc/operations.ts` keeps the four operation names and answers all of them from that one read, filtering and sorting in memory. `DEFERRED.md` was written against the other shape and had it exactly backwards. It described the substrate tables as the honest, server-paged half of a contrast, pointed at `ListSubstrateActors` and `ListSubstrateWorkers` as "a worked precedent to copy rather than a design to invent", and named `substrate.spec.ts` as the assertion drawing the line between them. All three of those RPCs are gone, so the precedent is gone with them: whoever pages one of these lists is now designing the request, and the commentary that had already solved the unique-last-sort-key problem is only in git history. What the file says now is what is true. The debt is every list rather than three of them, with `GetSubstrateStatus` added to the table of reads that need paging, searching and sorting. The counts argument is recorded as history rather than as a live reason, since totals over a whole inventory are simply true. And the ceiling that split this read in the first place is named, because it is back: a cluster of 410,110 actors produced a response gRPC refused to send. Two assertions in `substrate.spec.ts` are flagged rather than changed. "The searches are the server's, and a match is found wherever it is" now passes over an in-memory filter, and "the paged tables do not pretend to sort, and the inline ones do" withholds a sort from tables that could honestly offer one. Both still pass, which is the problem: the behaviour they check survives having every row in the browser, so nothing objected when the reason for it went away. Whether the tables should now sort is a design call for whoever owns that page, not a rename. Also fixed two passages that had drifted for unrelated reasons: the auto-titling section claimed the agent conversation table is "sorted and searched server-side", which the section below it correctly says is the browser's, and a pointer to a note under that table that was removed two commits ago. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
…arrows Two assertions were written against the paged shape and outlived it. Both still passed, which is the problem: the behaviour they check survives having every row in the browser, so nothing objected when the reason for it went away. "The searches are the server's, and a match is found wherever it is" is now a search the browser runs over an in-memory copy — `GetSubstrateStatus` answers all four lists from one message and `api/grpc/operations.ts` filters it. The property is unchanged and worth keeping; only the reason it holds changed, from "the server searched everything" to "the browser has everything", and the title says the property rather than the mechanism now. "The paged tables do not pretend to sort, and the inline ones do" withheld a sort because a client-side sorter over one page shows the last status on that page rather than in the cluster. Nothing is paged any more, so a sorter on those tables would be as honest as the one the templates table keeps. The sorters have not come back, so the test now pins what the page does rather than asserting a principle, and says out loud that whether those tables should sort again is an open decision. It is kept rather than deleted because it is still load-bearing in one direction: if these reads are paged again, a sorter added meanwhile becomes exactly the half-truth described above, and this is what objects. One step also claimed the actors "arrive grouped by status, which is the server's order". The grouping is still there and still worth having — an unordered list moves rows under the pointer between polls — but it is `operations.ts` that imposes it now. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
`Rename` named the verb from the reader's side while every sibling on `AgentInstanceService` names the operation from the API's — and the write is an update of one field, which is what the new name says. Renamed end to end rather than at the edges: the RPC and its request and response messages, the store interface and its postgres implementation, the service's own store dependency, the gRPC handler, the policy entry that authorises it as `AccessUpdate`, the sqlc query, the browser client's call, the mock transport's handler, and the tests around all of it. Not a breaking change, which is the only reason it is worth doing at this point in the branch: `main` has no `RenameAgentInstance` — it arrives with this rewrite — so `make proto-breaking` compares against a service that never carried the old name. Checked rather than assumed. Generated output was regenerated, not edited: `make proto-generate` for the Go, TypeScript and Python artifacts, and `make -C go sqlc-generate` for the query code. Both were run against the unmodified tree first to confirm they produce no drift of their own, so the diff in `go/api/gen`, `ui/src/generated` and `go/core/internal/database/gen` is only this rename. The UI keeps `RenameConversationButton` and its "Rename" wording. Renaming is what a reader does to a conversation; `UpdateAgentInstanceName` is what the API calls the write that records it, and the two names describing the same act from different sides is the point rather than an inconsistency. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Renaming lived in one place: the agent's conversations table. So renaming the conversation you were reading meant leaving it, finding it in a list, renaming it there and coming back — for the one field on the record its reader owns. It is offered on the conversation itself now, from the row's action menu in the rail and from the details modal, which is the same objection the details modal was made for: reference that costs a navigation is reference nobody consults. The dialog moved out of the button rather than being copied. `RenameConversationButton` was a tooltip, a button and a modal in one component, and a menu item has nothing to hang a tooltip on while a row in a description table has nothing to hang a button on — so what the three surfaces share is `RenameConversationDialog`, which owns the write, the validation and the reasoning about both. The button is now the button-shaped wrapper and keeps its props and its test id, so the table's existing coverage still drives it unchanged. The details modal did not show the name at all, which is a strange omission for the record of a thing named by its reader — and it is what the pencil needed to sit beside. It is the first row now, before the id, because it is what a reader opened the modal to check. Unnamed reads as `Untitled · 6f1c9d20` rather than "not reported": an unnamed conversation is the ordinary case and is identified by its id everywhere else, where "not reported" would suggest the controller failed to send something. Two details worth stating because both were decisions: The rename dialog opens *over* the details modal, which is usually a smell. Editing the name in place in that table was the alternative, and it means a second copy of the field's validation and of the write behind it — and `instanceFields` exists in the first place because two copies of this record drifted. antd stacks them correctly and returns focus to the modal on close. The dialog resets its draft when it opens rather than when it mounts. Its callers render it beside a menu item and a table cell rather than conditionally, so it stays mounted between openings, and a draft left over from a cancelled edit would be sitting there the next time — reading as a name the conversation does not have. `destroyOnHidden` clears the modal's contents and not this state, which lives a level above it. The new test drives both routes and asserts the rail row's text and the modal's own row rather than the toast, because a success message says the app believes the write landed. Checked by removing each control in turn and confirming it fails. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
EItanya
approved these changes
Aug 26, 2026
It lived in `ui/dev-scripts`, which put it in the web app's directory while it stands up a whole cluster: a Kind node, a local registry, substrate and its CA and JWT pools, the controller, and an agent to talk to. The UI image is one of ten steps. `scripts/` is where the repository's other cluster tooling already is, `scripts/kind` included. It works unchanged from there, which is luck rather than design and worth recording: the script resolves the repository as `dirname/../..` and then `cd`s to it, so every path inside it is repo-root-relative — and `scripts/setup-cluster` is the same depth below the root as `ui/dev-scripts` was. Checked by resolving that expression from the new location rather than by reasoning about it. Not run end to end. It calls `make create-kind-cluster`, which would take down a cluster somebody is using. What was verified is that it parses, that the repository root resolves correctly from its new home, and that every repo path it names still exists — which turned up two that never did. `ui/HANDOFF.md` is cited twice by the script and once by `FilterBar.test.tsx`, and has no history in this repository at all: `git log --all` knows nothing about it. It was somebody's working notes, and three comments were pointing readers at it. The script now describes what it follows — the steps CI runs, plus the two a developer needs and CI does not — and names the README beside it. The test's pointer went to the place that actually carries the note it wanted, `playwright/tests/lists/list-filters.spec.ts`, which explains the invisible second listbox rc-select renders. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Renaming from the details modal left the rail beside it showing the old name, and
renaming from the rail left the modal showing the old one. Each surface reads the
conversation separately — the chat page reads the open one as `agentInstances.get` and
the rail reads the list as `agentInstances.list` — and each rename refreshed only the
read it was started from. Both were right about their own read and both looked broken.
Invalidated by key rather than wired surface to surface. `useInvalidateConversations`
asks SWR to revalidate anything keyed `agentInstances.*`, which it already knows the
readers of, so this removes plumbing instead of adding it: the rail's
`onRenamed={conversations.refresh}`, the modal's `instance.refresh` and the table's
`conversations.refresh` are all gone, and `onRenamed` is now optional. The alternative
was handing every surface a callback that refreshes every other surface's read, which
grows with each new place a conversation appears — and the dashboard below would have
been the third.
The dialog also stopped taking an `open` prop and is mounted only while renaming. It
had needed an effect to reset the draft each time it opened, or a cancelled edit was
still in the field the next time, reading as a name the conversation does not have —
and that effect was a lint error, `set-state-in-effect`, because setting state
synchronously in an effect cascades renders. Mounting it with the state it should have
is the same fix without the effect.
The dashboard's recent list shows names now. That card lists `AgentInstance` rows,
which are conversations, and rendered each one as a bare eight-character id on the
reasoning that "an agent has no name" — the comment said so. A conversation has one,
so it reads through `conversationTitle` like the rail and the table do: the name
somebody chose, the title derived from the first message, or "Untitled" beside the
short id, in that order. The derived titles cost one task read per unnamed row and
there are at most five of them, well inside the budget `useConversationTitles`
enforces at thirty for the rail.
The dashboard card had no test at all. The new one asserts the property rather than a
fixture's particular name — no row's link text is bare hex — which is what tells the
old behaviour from the new whichever conversations happen to be recent.
Both were checked by reverting: the sync test fails with the invalidation removed, and
the dashboard test fails against `shortInstanceId`. The sync test was then run sixteen
times over eight workers, because the assertion it makes depends on a revalidation
landing and a flaky one would have been worse than none.
Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The script ended holding one port-forward, the UI on 8080. So running the dev server against the cluster it had just built needed `KAGENT_DEV_CONTROLLER_URL` pointed at 8080 in `ui/.env` — and without that line the failure was quiet in the worst way: the page loaded, and every read failed with `ECONNREFUSED 127.0.0.1:8083` in a log nobody was watching, which reads as a broken backend rather than a missing forward. It forwards the controller on 8083 too, which is where `yarn dev` looks by default. A second `kubectl` is cheaper than a setting every reader has to be told about, so there is nothing to configure now. The env var stays documented as the thing it actually is: worth setting when the nginx hop is what you want to exercise, since at 8080 the dev server takes the same path a deployed build takes. Two forwards cannot both be the foreground process, so they are backgrounded and the shell waits on them, with a trap so Ctrl-C takes both down — without it the script would exit leaving orphans holding the ports, and the next run would fail on an address already in use. Waited on with a `kill -0` poll rather than `wait -n`. `wait -n` is bash 4.3 and this is a script for a Mac, where the system bash is 3.2 and it is a syntax error; plain `wait` would sit on a dead forward until the other one went too. The poll stops as soon as either one dies, which is the point: a forward that has gone means whatever depended on it is failing, and saying so beats leaving one working and one silently absent. Tested with `sleep` standing in for `kubectl`, so nothing touched a real cluster: one process dying exits the loop and the trap kills the survivor with no orphans, and SIGTERM kills both. The true Ctrl-C path is reasoned rather than demonstrated — a background job has SIGINT ignored and macOS has no `setsid` — but a terminal sends it to the whole foreground group, so both children get it directly and the trap runs anyway. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
EItanya
approved these changes
Aug 26, 2026
Eighty-six lines for five settings, most of it reasoning that belongs where the code is rather than in the file somebody copies to get started. Fifty now, with every setting still there and still commented out. What stayed is the part a reader cannot infer: that mock mode is off unless asked for and overrides every backend setting when it is on, that `KAGENT_DEV_CONTROLLER_URL` already matches what `setup-cluster` forwards so a cluster from that script needs nothing set, and that `EXTENSION_*` passes through untouched. What went was the history — which default used to be the other way round and why, what the failure looked like — and a placeholder key that existed only to show a naming pattern the sentence above it already describes. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
One file, no readme, and a name that reads like developer tooling now that the cluster setup script has moved to `scripts/setup-cluster` — which invites deleting it. It is the image's entrypoint: it renders the deployment's settings into `env-config.js` from the pod's environment and execs nginx, and without it the container has nothing to run. Two sentences and the distinction that makes it obvious: runtime, not tooling. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The note pointed at the root `scripts/` as a fact about the current layout, which is the half that does not stop the next one landing here. It says it as a rule now: a developer script belongs at the repository root whatever part of the repo it is for, and this directory is only what ships inside the image. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Drops both sentences added on top of the entrypoint description: the rule about where a new script belongs, and the "runtime, not tooling" framing. What is left is the fact — developer scripts live in the root `scripts/` — which is the half that was worth saying. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
The page reads `GetSubstrateStatus` and nothing else. It briefly read three RPCs instead — a summary and a page each of actors and workers — and those were removed again, so `api/grpc/operations.ts` now answers all four of its operations from that one response and narrows in memory. The mock suite covers that shape happily, which is the problem: the fixtures were written for whichever shape was current, and on this project every defect found by pointing the app at a real backend was a place where a fixture had taught a shape the controller does not use. `live: every page loads against the cluster and reports no failure` already visits `/substrate`, but it cannot tell a page that read the answer from a page that reached the controller and understood none of it — both draw the same tiles, one of them full of zeros. This asserts the counts and the worker rows, which the chart's own pool always provides. Actors need a conversation to exist and are deliberately left out. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
github-actions
Bot
added
enhancement
A conversation where the agent asks something with `ask_user` rendered every answer above the question it answered. Read back from a cluster, one such task is three reader turns in `history` and seven agent entries in `artifacts`: "ask me a question" opened it, then two rounds of call, pending result and answered result, then the closing reply. The gateway puts every reader message in the first list and every agent message in the second, and `messagesFromTask` concatenated them — so any task holding more than one reader turn, which `ask_user` guarantees, showed all of one side and then all of the other. Nothing in the response says how to interleave them, which is filed as #2584: every message in a task carries the task's single `status.timestamp`; artifact ids are UUIDv7 and sort correctly while `history` ids are minted by the sending client and are UUIDv4, carrying no time; and the answer's `ask_user_response` id and the call's own `call_…` id do not refer to each other. So `interleaveTaskMessages` infers, from position only. The *n*th answer answers the *n*th round — the runtime pairs answers positionally and an instance holds one non-terminal task at a time, so they cannot be answered out of order — and within a round it goes before the last `ask_user` result, the one reporting what was answered, which puts it after the call and after the result that was still pending. An answer it cannot place is appended rather than dropped: a transcript missing a turn is worse than one holding it in the wrong place. It is inference and the module says so, with the issue named and the instruction to delete it rather than build on it once the gateway grows an ordering key. The tests are built from the shape read off the cluster, including the case the obvious rule gets wrong: the last round runs to the end of the task, so "insert at the end of the round" puts the answer below the agent's closing reply. One thing this cannot reach, also in #2584. The prose question the reader answers is on the parked task's status message and is persisted nowhere, so it is absent from a replayed conversation entirely — visible as a question that sits at the foot of the transcript until a refresh, then disappears. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
…irect to it
`/agent-templates` is a redirect: the router answers it with `<Navigate
to="/agents?tab=templates" replace />`, kept so an address somebody already has still
resolves. Five places inside the app navigated through it anyway, which spends a
render on a URL that is replaced in the same tick.
That transient URL was the recurring flake. `agent templates: deleting one says what
it costs` clicked "Back to templates" and then waited for `/agent-templates`, which
either matched during that one render or never matched at all. Reproduced at twelve
workers: three failures in twenty-four, every one of them `waitForURL` giving up with
`waiting for navigation until "load"` on a URL that had already been replaced. Nothing
at twelve workers after the fix.
The mock suite's test timeout goes back to Playwright's default thirty seconds. It was
raised to sixty on the reading that this same test running out of budget was a report
about contention, and that was wrong twice: the step was waiting for something that
was never going to arrive, so no budget would have helped, and a mock-backed suite
needing more than thirty seconds for one test is saying something is stuck — which is
worth hearing rather than absorbing. The note against it now says so, so it does not
get raised again for the same reason.
Internal navigation uses `agentTemplatesTab`, the address the reader actually ends up
at. The redirect stays for the address it exists for.
Two navigations are deliberately left going through it, because fixing them is a
different change: `AgentTemplateDetailsPage` and `AgentTemplateNewPage` both build
`${paths.agentTemplates}?namespace=…`, and the redirect does not carry query
parameters across — so the namespace they pass is dropped today, redirect or no
redirect.
Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Creating or deleting an agent template sent the reader back to the list unfiltered. Both call sites asked for the filter and neither got it, for two independent reasons on the same line. `paths.agentTemplates` is `/agent-templates`, which the router answers with `<Navigate to="/agents?tab=templates" replace />`. `Navigate` with a literal `to` does not carry the incoming search params, so `?namespace=…` was gone before the list rendered. And the list narrows on `ns` — `FILTER_IDS` in `AgentTemplatesPage` — so `namespace` would have been ignored even if it had survived. Either fault alone loses the filter, and the page looks reasonable either way. Both now navigate to `agentTemplatesTab` with `&ns=`, which is the address the reader ends up at and the parameter the list reads. Nothing in the app goes through that redirect any more; it stays for the addresses people already have. Nothing asserted the list came back narrowed, which is how two faults shared one line unnoticed. Both journeys check it now, and check it twice: the address carries `ns`, and the pill for it is on screen. The URL alone would pass while the list ignored the parameter, which is precisely the second fault. Signed-off-by: Nicholas Bucher <behappy54321@gmail.com>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 written by Claude (start)
Summary
Rebuilds the web interface on Vite + React 19 — React Router, SWR, antd 6, Emotion. It is a static bundle behind nginx with no server process; settings come from
window.environmentVariables, rewritten by the container on every start, so one image serves every deployment.The application API is reached over gRPC-Web:
grpcserver.WebHandlerwraps the existing*grpc.Server, and the HTTP server routes gRPC-Web requests to it ahead of its middleware chain.Note
Reading the diff. 616 of the 696 files are
ui/, which is the rewrite's tree — read it as a new app, not as a diff. The other 80 are three things:helm/— the UI pod stops running a Next.js server and becomes nginx serving a static bundle, songinx.conf,supervisord.conf,ui-deployment.yamland the UI values change together with their tests.go/andproto/— the gRPC-Web seam (grpcserver/grpcweb.go,httpserver/server.go,app.go) and the fiveAgentInstancechanges below, plus generated proto and sqlc output.CLAUDE.mdand.nvmrc— the repo guide's UI section, and the pinned Node version.Testing this PR
One command builds a Kind cluster and installs this checkout on it — controller, UI and agent runtime all built from the working tree, over the chart's published images. It ends holding two port-forwards, the UI on 8080 and the controller on 8083, so the last thing it prints is a working URL and
cd ui && yarn devneeds nothing configured../scripts/setup-cluster/setup-cluster.sh # ~25 min, mostly image buildshttp://localhost:8080
Tip
The script also leaves one agent on the cluster — an
assistanttemplate on akagentharness — so Agents has something in it and you can send a message straight away, without creating anything first.Warning
make create-kind-cluster && make helm-installdoes not work, and fails silently five different ways — including that the chart installs published images, so none of your changes are on the cluster while everything looks healthy.scripts/setup-cluster/README.mdcovers each one, and the dev-server loop for iterating.UI Extensions
The app declares vendor extension points anyone can use to add to it or restyle it, all in one configuration object:
navItemsordernavOverridesroutesrouteHandlesslotsformFieldstableColumnsapiprovidersthemeshellbrandingproviderIconsagentLinksInstalling one is two edits: build a
VendorExtensionConfig, then pointsrc/vendorExtensions/activeConfig.tsat it. Overriding theme tokens restyles the app's own components, not just the extension's.Note
📖
ui/docs/vendor-extensions.md— every extension point and what it receives. Worth reading before reviewing thevendorExtensions/tree.Substrate
The pages follow the CRDs. An Agent is derived, not a resource — a
Harness×AgentTemplatepair read fromAgentTemplate.status.harnesses[], so there is no "New agent" button. The landing page explains the four concepts over three tabs; an agent's page lists its conversations, and a conversation is anAgentInstance.Five additive server-side changes, none affecting an existing caller:
AgentInstancegains anameUpdateAgentInstanceName. A column, so the write touches only the column. Renamed from the conversations table, the rail's action menu or the details modal; the rail lists newest first.ListAgentInstancestakes a queryprepared_revision.app.godefaults the A2A gatewayTest Coverage
385 unit tests. The browser suite runs in Chromium and Firefox, plus a Chromium pass with the example extension installed, and in parallel in CI.
Follow Ups
UI Playwright E2Eworkflow that did that is removed here, along with the harness it drove (playwright/scripts/setup.sh,playwright/mocks/server.mjs); restoring it is follow-up work.yarn test:pw:livecovers a few journeys against a real cluster in the meantime.ui/playwright/DEFERRED.mdlists the rest, and the surface each spec waits on. Nothing is committed as a skipped test.Note
go test ./...fails one pre-existing test on macOS —TestFetchSourceReusesExistingMaterialization,/varvs/private/var, in a package this change does not touch.Upgrade Notes
Caution
Breaking. The chart no longer sets
NEXT_PUBLIC_BACKEND_URL,BACKEND_INTERNAL_URLorBACKEND_GRPC_URL, and dropsui.backendInternalUrl,ui.backendGrpcUrlandui.volumes.nextjsCache.The oauth2-proxy
skip-auth-regexnow names/assets/andenv-config.jsinstead of the Next.js paths, which no longer exist.Also included, unrelated to the rewrite:
helm/tools/grafana-mcpnow passes-allowed-hosts. The server rejects any Host header it was not told about, so one reached over the cluster network answered the MCP handshake withForbidden— theRemoteMCPServersatAccepted=Falseand every agent using it failed to build its tool set. Found while testing the tools pages; happy to split it out if preferred.🤖 written by Claude (end)