fix(ui): clarify capability loading activity - #3584
Conversation
edfa2e9 to
60dbe0e
Compare
60dbe0e to
8b7a175
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Review at exact head 8b7a175599713e07de4551b2d556415956434dfb.
APPROVE. No P0–P2.
The thing I most wanted to check on a PR about "loading activity" is whether the loading state can lie — whether the UI can sit spinning while nothing is actually in flight. It can't, and the reason is that this PR doesn't introduce a loading state machine at all: in-flight presentation stays on the existing tool-call lifecycle, so the terminal conditions are the tool call's own (result, error, timeout). There's no independent flag that could get stranded.
The naming fallback is the neat part. resolveToolDisplayName (display-name.ts:37-45) tries describeLoadToolResult first and falls back to loadToolDisplayName(locale):
return describeLoadToolResult(item.args, value, locale)?.actionLabel
?? loadToolDisplayName(locale);Before the result arrives, describeLoadToolResult returns null and the row shows a neutral "启用能力 / Enable capabilities" — it names the kind of work without claiming progress it can't observe. When the result lands, the row swaps to the specific action label. A row that says nothing false while waiting is better than one that invents a status.
Empty, error and loading stay visually distinct, which is the failure I'd expect on a change like this: a zero-capability success renders the same card with an explicit 0 项能力可用 / 0 capabilities available, a failure produces a non-{loaded:[...]} shape that falls through to the generic preview with the row in errored, and loading is the spinner with the neutral label. Three genuinely different situations, three different renderings.
Two smaller things worth noting as correct:
- The technical-details disclosure uses
dl/dt/ddrather than generic divs, andgroupId/toolIdsgo through control-character stripping plusredactSecretsbefore being shown incode. - On the runtime side,
toModelOutputstrips the group metadata and keeps only loaded ids, so the display metadata lands in the persisted result without burning provider context. That split is easy to get wrong in the direction of shipping presentation data to the model.
One observation, not a finding: loadToolGroupKind's heuristic for historical results that carry no metadata could mis-bucket an unusual custom group. The blast radius is the icon and the label text, with a generic fallback behind it — purely decorative, no behavioural consequence.
Layout not verified here: the new card layout (icon column + summary + collapsible details) was not rendered on real hardware in this review environment. I'd suggest a maintainer with a GUI eyeball the collapse interaction at a narrow width before this is considered fully covered.
Verification: exact-head test is completed/success, and I cross-checked with gh run list --commit that this commit really has only that one workflow rather than a missing gate. ui 217/217 including the new presentation assertions and the legacy regression that the old namespace string is no longer rendered; runtime tool-availability 9/9 including the toModelOutput stripping assertion; the runtime-host UDS integration case for unknown client capability passes end to end.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up at the same head 8b7a175599713e07de4551b2d556415956434dfb — one [P3], nothing that changes the approval.
[P3] The two new !important declarations are overriding this PR's own reset — packages/ui/src/styles.css:1016, 1033-1046
.maka-load-tool-preview p { margin: 0; } has specificity (0,1,1), while .maka-load-tool-description and .maka-load-tool-count are (0,1,0). So their margin-top loses the cascade to the reset added a few lines above, and !important is what wins it back.
Worth noting because the obvious reading is that these are fighting Astryx — they aren't. Nothing here is contending with a vendor rule, which is also why this carries no upgrade risk. It's local cascade debt, and it can go away without !important at all: narrow the reset to the elements that actually need it (e.g. .maka-load-tool-preview > p, or scope it to the body/text slot rather than every descendant p), or give the two rules a naturally more specific selector.
For context on why this stayed at P3: main currently carries exactly two !important declarations in this stylesheet, so doubling that count is a real trend worth catching early — but the rendered result is correct today and nothing downstream depends on the override.
Separately, the design-system coupling question was checked deliberately here and came back clean: the preview is injected through Astryx's public ChatToolCallItem.resultDetail slot, with no vendor class or internal-DOM selectors, all new markup and CSS namespaced under .maka-load-tool-*, and only the shared radius token borrowed. No patch-package delta either. That's the reason this is a P3 and not a structural objection.
Summary
load_toolsresults while keeping provider output compactBefore
After
Both screenshots use the same 1160 x 768 Electron window, task, expanded tool row, content, and zoom level.
Verification
npm --workspace @maka/runtime testnpm --workspace @maka/ui testnpm --workspace @maka/runtime-host run typechecknpm --workspace @maka/desktop run typechecknpm run check:asf-headersgit diff --check origin/main...HEAD