UI rework polish - #2666
Conversation
Two behaviours the page offered twice, both visible on the same screen. Row hover is opt-in through `clickable-table-row`, and the rule that suppresses it everywhere else was written as `tr:hover > td`. The actors and workers tables are antd `virtual` tables, whose body is divs — no `tr`, no `td` — so those two went on lighting up under the pointer while the pools and templates beside them, and every other static table in the app, had stopped. The rule now matches the cell class antd actually paints the hover from, which exists in both bodies; the pointer and pressed rules for clickable rows are rewritten the same way so the mirror bug cannot appear. Selected rows keep their own hover. The actor and worker columns carried a header of the page's own: a button around the title, a bare arrow beside it, nothing outside those few words to click. It avoided antd's `sorter` because a comparator reorders the page in hand, and one page out of 410,110 reordered is not the cluster sorted. The concern was right and the remedy left a reader two headers to learn. `sorter: true` is antd's header with no comparator behind it, so the whole cell is the target, the chevrons show the direction, and the table still reorders nothing — the order goes out with the next read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
The agent-template form carried no required marker on any field while refusing to save without a name and a model configuration, and the model form's API key was required to create with nothing on screen saying so. Both marks are conditional, because both fields are. The template form renders as text on the details page, where an asterisk would be asking a reader for something the template already has; the model form's API key is needed to create and not to edit, which its label already says in words and now says in the marker too. Every other authoring surface was checked against its own validation rather than for the presence of marks. The harness, MCP server and prompt forms already agreed — including the MCP namespace, which is genuinely optional there and rightly unmarked. A new spec walks all six surfaces and asserts what is marked *and* what is not: a test that only checked the marks would pass just as well on a form that marked every field. It pins the two conditional cases too. `routes` in the Playwright helper gains the two new-resource addresses it was missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
`suggestName` fell back to `remote-server` or `tool-server` whenever it had no host or package to derive a name from, and the new-server page re-applies the suggestion on every field change while the name is untouched. So opening the wizard and switching to Command wrote `tool-server` into the empty Name field — as did typing a namespace or flipping TLS, with `remote-server`. What had been a placeholder became a value the reader appeared to have chosen. The suggestion is now empty when there is nothing behind it, which is what the field's placeholder is for. Typing a URL or a package still fills the name in, and switching kind now clears a suggestion the other branch cannot support rather than swapping in a made-up one. Two unit tests cover it: the derivation both kinds already had, and the empty case that regressed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Torey Scheer <torey.scheer@solo.io>
There was a problem hiding this comment.
Reviewed db0b844. All three fixes work.
One blocking concern. Although sorter: true is the right choice here (a comparator would only reorder the current page), the comments written around it credit a server round-trip that never happens. operations.ts sorts the whole inventory in the browser.
Nothing else blocks.
Checked
- ✅ Reverting the hover selector to
> tr…:hover > tdmakes the new spec fail in both browsers (rgb(221, 215, 231)vsrgba(0, 0, 0, 0)). - ✅ The new
<th>header carriestabindex="0",aria-description="sortable",aria-label, andaria-sortonce sorted, and Enter activates it.
🤖 written by Claude
| * Each column's `key` is the server's own sort field, which is what lets the change | ||
| * handler send `columnKey` straight on. | ||
| */ | ||
| const actorColumns: ColumnsType<SubstrateActorEntry> = useMemo( |
There was a problem hiding this comment.
Level: 🟠 Medium · Not Blocking
pagedSortChange casts columnKey to the sort field, so nothing catches a key that isn't one (exactly the key: "pod" → "workerPod" bug fixed at L975).
| const actorColumns: ColumnsType<SubstrateActorEntry> = useMemo( | |
| const actorColumns: (ColumnsType<SubstrateActorEntry>[number] & { | |
| key: SubstrateActorSortField; | |
| })[] = useMemo( |
workerColumns at L994 takes the same shape.
🤖 written by Claude
| /* Marked required only while the form authors: read-only is the details | ||
| page showing a template that already has a name, and an asterisk there | ||
| would be asking a reader for something. Same for the model below. */ | ||
| required={!readOnly} |
There was a problem hiding this comment.
[nit] Level: 🟡 Low · Not Blocking
isCreate and readOnly are exclusive across both call sites (AgentTemplateNewPage.tsx:127 passes isCreate with no readOnly, AgentTemplateDetailsPage.tsx:532 passes isCreate={false}), so this is always true.
| /* Marked required only while the form authors: read-only is the details | |
| page showing a template that already has a name, and an asterisk there | |
| would be asking a reader for something. Same for the model below. */ | |
| required={!readOnly} | |
| required |
The read-only rationale belongs on the Model configuration item below, which is the genuinely conditional one.
🤖 written by Claude
| .ant-table-tbody | ||
| > tr.ant-table-row:not(.clickable-table-row):hover | ||
| > td { | ||
| .ant-table-row:not(.clickable-table-row):not(.ant-table-row-selected) |
There was a problem hiding this comment.
[nit] Level: 🟡 Low · Not Blocking
The only rowSelection in the app is AgentPage's conversations table, where every selectable row already carries clickable-table-row (non-openable rows get a disabled checkbox), so this :not() can never match.
| .ant-table-row:not(.clickable-table-row):not(.ant-table-row-selected) | |
| .ant-table-row:not(.clickable-table-row) |
🤖 written by Claude
| await row.hover(); | ||
| // Waited out rather than polled: the claim is that nothing happens, and there is no | ||
| // event for a transition that never starts. See `helpers/style`. | ||
| const hovered = (await settledPaint(cell)).background; |
There was a problem hiding this comment.
Level: 🟡 Low · Not Blocking
settledPaint is a fixed 350ms wait, making this the one assertion here that can flake on a loaded CI box.
An await expect(cell).toHaveClass(/ant-table-cell-row-hover/) after the hover would at least make a missed hover fail loudly rather than pass.
🤖 written by Claude
| * more by clicking the words inside one is a page a reader has to learn twice. What is | ||
| * not antd's is the sorting: the columns below declare `sorter: true`, which is the | ||
| * form that gives the header its control and no comparator to run, so the table | ||
| * reorders nothing. The order goes out with the next read, and the rows come back in |
There was a problem hiding this comment.
Level: 🔴 High · Blocking
No order is sent anywhere: GetSubstrateStatus takes only a namespace, and localPage (api/grpc/operations.ts:1419) sorts the whole inventory in the browser before slicing a page.
So sorter: true is right for the opposite reason to the one given here. The read has already ordered every row, so a comparator would re-sort only the hundred on screen.
Users meet the same claim as the literal "Sorted by the server:" at L479.
🤖 written by Claude
| field: SubstrateWorkerSortField; | ||
| order: SubstrateSortOrder; | ||
| }>({ field: "default", order: "asc" }); | ||
| const [actorSort, setActorSort] = useState<PagedSort<SubstrateActorSortField>>({ |
There was a problem hiding this comment.
Level: 🟠 Medium · Not Blocking
sortField/sortOrder are part of the SWR key (api/hooks/useSubstrate.ts:71), so every header click refetches the entire inventory to reorder rows the browser already holds.
🤖 written by Claude
| ), | ||
| title: "Status", | ||
| key: "status", | ||
| sorter: true, |
There was a problem hiding this comment.
Level: 🟡 Low · Not Blocking
Actor status and worker pool fall through to the same sort key as default in localPage (api/grpc/operations.ts:1425, :1461), so these two headers move the label and reorder nothing.
🤖 written by Claude
| .ant-table-row.clickable-table-row:active | ||
| > .ant-table-cell { | ||
| background: ${theme.color.primary}4D; | ||
| } |
There was a problem hiding this comment.
[nit] Level: 🟡 Low · Not Blocking
The expand-icon rule just below at L107 still keys off tr.clickable-table-row (the stale selector this PR fixed above it).
🤖 written by Claude
Charlesthebird
left a comment
Charlesthebird
left a comment
There was a problem hiding this comment.
I tested this locally as well - I think the red required star on the form fields could say "Required" and be a bit larger.
And the "Remote URL"/"Command" tab buttons on the form are a bit too low contrast.
I have some other design notes/ideas for the UI but won't put them all in this review. Thanks for adding this!
2 participants
AgentTemplateFormandModelFormthat were missing itsubstrate tables being more uniform:
agent template /model form inputs being marked required:
