You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "colaforniaw (via GitHub)" <gi...@apache.org> on 2026/09/08 10:44:14 UTC
[PR] feat(desktop): readable tool rows, WebFetch citations, and on-demand full retained output [maka]
colaforniaw opened a new pull request, #5030:
URL: https://github.com/apache/maka/pull/5030
## Summary
- 折叠行从“调用参数”变成“语义摘要” —— `Read /a/b/c/file.ts offset=120 limit=50` 变成 `file.ts · L120+50`；`Grep`/`Glob` 在有统计信息时显示返回数量；纯成功回执显示“成功”。
- 截断不再是死胡同 —— 被预览省略的内容可通过“打开完整输出”在 Files 面板查看和复制；归档读取上限为 4 MiB，仅展示实际保留的内容。
- `WebFetch` 成功结果从整页文本墙变成引用卡 —— 显示标题、外链、字节数、行数和截断提示；正文不在界面中展开，失败结果保留可读错误。
Fixes #4863
## Before / After
### 1. 整轮工具区：从参数噪音到语义摘要
<!-- 使用同一段多工具对话、相同输入和 zh-CN 语言设置，分别在 main 和本分支截图；路径先脱敏。 -->
| Before | After |
| --- | --- |
| <!-- 粘贴修改前图片 --> | <!-- 粘贴修改后图片 --> |
折叠行突出文件、操作和返回数量；无内容结果与纯成功回执不再提供冗余展开入口。
### 2. 截断预览：完整保留内容可达
<!-- 修改前：被省略内容不可达的预览；修改后：点击“打开完整输出”并进入 Files 面板的短 GIF，包含复制按钮。 -->
| Before | After |
| --- | --- |
| <!-- 粘贴修改前图片 --> | <!-- 粘贴修改后图片或 GIF --> |
被预览省略的内容不再止于截断提示，可在 Files 面板查看和复制完整保留内容。
### 3. WebFetch：从正文文本墙到引用卡
<!-- 同一 URL、相同返回正文；修改前展示 Markdown 正文，修改后展示标题、外链和统计信息。 -->
| Before | After |
| --- | --- |
| <!-- 粘贴修改前图片 --> | <!-- 粘贴修改后图片 --> |
整页正文变成紧凑引用卡，用户通过外链访问来源，而不是在对话中展开正文。
### 4. 终端输出：主动阅读不再被自动滚动打断
<!-- 使用同一段持续输出；修改后录制向上滚动、出现“跳到底部”按钮、点击恢复跟随的过程。 -->
| Before | After |
| --- | --- |
| <!-- 粘贴修改前图片或 GIF --> | <!-- 粘贴修改后 GIF --> |
向上阅读时暂停末尾跟随，点击“跳到底部”后恢复。
其他预览细节：
- JSON 预览优先呈现诊断信息，并准确标记被省略字段。
- Diff 预览在可行时回退到完整变更块边界。
- PTY 截断提示与快照正文分离，不再混入可复制的终端文本。
## 实现要点
- 归档读取复用现有会话、来源、大小和 SHA-256 校验，经 `read_archive_chunk` 分块传输至 Files 面板；兼容当前与旧版归档引用。
- 连续分块复用已校验的传输快照，避免每块重复读取与哈希。快照按连接、会话和引用隔离，最多保留 4 份，每份上限 4 MiB，有效期 60 秒。
- 传输完成、读取失败、连接断开或观察到会话移除时释放快照；过期项在后续归档查询时清理。
- 保留存储格式、权限校验和既有查询行为；新增归档查询类型，协议兼容 epoch 从 133 升至 134。
- 保留敏感信息脱敏、既有专用预览、时间顺序分组及 Astryx 展开与无障碍交互。
## Verification
- Core、UI 交互、Desktop IPC/client 和 Runtime Host 定向检查通过。
- 传输快照修改后，15 项 Host 归档与协议定向测试通过。
- 4 MiB 归档回归测试验证 32 KiB 分块的完整字节保留，并将底层读取次数从 128 次降至 1 次。
- Desktop 构建及全工作区类型检查通过。
- 暂存文件 Biome、ASF 许可头、协议 epoch 和空白检查通过。
完整仓库 lint、format、build、test 和 Desktop/UI Knip 检查尚未由现有验证记录确认；真实窗口视觉验证尚待完成。
## 待完成
- [ ] 补充上述前后对比图片或 GIF，并完成真实窗口视觉验证。
- [ ] 补齐贡献指南要求的完整本地检查，并更新验证记录。
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: OpenAI Codex contributed implementation, regression tests, review-feedback analysis, local verification, and this PR description. The commit includes `Generated-by: OpenAI Codex`; retain it in the squash commit.
## Checklist
- [x] Tests cover the change and fail without it
- [ ] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): improve tool output rendering and interaction [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5030:
URL: https://github.com/apache/maka/pull/5030#discussion_r3986645642
##########
packages/runtime-host/src/server/artifact-coordinator.ts:
##########
@@ -321,13 +356,92 @@ export class HostArtifactCoordinator {
): Promise<OperationOutcome<'artifact.query'>> {
try {
if ((await this.#sessions.probeSessionRemoval(input.sessionId)).kind !== 'present') {
+ for (const [key, transfer] of this.#archiveTransfers) {
+ if (transfer.sessionId === input.sessionId) this.#archiveTransfers.delete(key);
+ }
return notFound('artifact.query', 'Session was not found');
}
let sharedGrantId: string | undefined;
if (context.principalKind === 'session_guest') {
sharedGrantId = await this.#sharedArtifactGrantId(context.principal, input);
if (!sharedGrantId) return notFound('artifact.query', 'Artifact was not found');
}
+ if (input.kind === 'read_archive_chunk') {
+ const unavailable = (reason: ArtifactReadFailureReason) =>
+ querySuccess(
+ encodeArtifactQueryResult({
+ kind: 'archive_unavailable',
+ sessionId: input.sessionId,
+ reason,
+ }),
+ );
+ const identity = parseToolResultArchiveResourceRef(input.ref);
+ if (!identity) return unavailable('not_allowed');
+ if (identity.originalBytes > TOOL_RESULT_ARCHIVE_MAX_BYTES) return unavailable('too_large');
+ if (input.offset > identity.originalBytes) return invalidQuery('Archive offset is invalid');
Review Comment:
**P3** Off by one: `offset === originalBytes` passes and returns an `ok` empty chunk with `nextOffset: null`. `originalBytes` is always > 0 and the decoder guarantees `nextOffset < totalBytes`, so no client will ever ask for it — but `>=` is the rule you mean.
<details><summary>中文</summary>
差一：`offset === originalBytes` 会通过，返回一个 `ok` 的空 chunk 且 `nextOffset: null`。`originalBytes` 恒大于 0，解码器又保证 `nextOffset < totalBytes`，所以没有客户端会请求这个偏移——但你要表达的规则是 `>=`。
</details>
##########
packages/runtime-host/src/server/artifact-coordinator.ts:
##########
@@ -321,13 +356,92 @@ export class HostArtifactCoordinator {
): Promise<OperationOutcome<'artifact.query'>> {
try {
if ((await this.#sessions.probeSessionRemoval(input.sessionId)).kind !== 'present') {
+ for (const [key, transfer] of this.#archiveTransfers) {
+ if (transfer.sessionId === input.sessionId) this.#archiveTransfers.delete(key);
+ }
return notFound('artifact.query', 'Session was not found');
}
let sharedGrantId: string | undefined;
if (context.principalKind === 'session_guest') {
sharedGrantId = await this.#sharedArtifactGrantId(context.principal, input);
if (!sharedGrantId) return notFound('artifact.query', 'Artifact was not found');
}
+ if (input.kind === 'read_archive_chunk') {
+ const unavailable = (reason: ArtifactReadFailureReason) =>
+ querySuccess(
+ encodeArtifactQueryResult({
+ kind: 'archive_unavailable',
+ sessionId: input.sessionId,
+ reason,
+ }),
+ );
+ const identity = parseToolResultArchiveResourceRef(input.ref);
+ if (!identity) return unavailable('not_allowed');
+ if (identity.originalBytes > TOOL_RESULT_ARCHIVE_MAX_BYTES) return unavailable('too_large');
+ if (input.offset > identity.originalBytes) return invalidQuery('Archive offset is invalid');
+ if (!this.#readArchive) return unavailable('read_failed');
+ const now = this.#now();
+ for (const [key, transfer] of this.#archiveTransfers) {
+ if (transfer.expiresAt <= now) this.#archiveTransfers.delete(key);
+ }
+ const key = JSON.stringify([context.connectionId, input.sessionId, input.ref]);
+ let transfer = this.#archiveTransfers.get(key);
+ if (input.offset === 0 || transfer?.ref !== input.ref) {
Review Comment:
**P3** `transfer?.ref !== input.ref` is dead: `key` already contains `input.ref` (line 387), so a retrieved entry's ref is necessarily equal.
The `input.offset === 0` half is worth dropping too. The ref is content-addressed and `bodySha256` is verified on every read, so a re-read cannot return different bytes; freshness is the TTL's job. Forcing a fresh full read at the start of every pagination run is also the single biggest contributor to the amplification in the review body. `if (!transfer) { … }` removes the dead code and that cost together.
<details><summary>中文</summary>
`transfer?.ref !== input.ref` 是死条件：`key` 在 387 行就含了 `input.ref`，取到的条目 ref 必然相等。
`input.offset === 0` 那一半也该删。ref 是内容寻址的，每次读都校验 `bodySha256`，重读不可能返回不同字节；新鲜度由 TTL 负责。而在每轮分页开始时强制一次全量重读，正是正文里那个放大效应最大的单一来源。改成 `if (!transfer) { … }` 同时消掉死代码和这份开销。
</details>
##########
packages/ui/src/tool-activity/result-projection.ts:
##########
@@ -23,6 +23,51 @@ import type { ToolActivityItem } from '../materialize.js';
import { formatQuietJsonValue } from './builtin-preview.js';
import { isConnectorTool } from './display-name.js';
import { getToolActivityCopy } from './copy.js';
+import { redactSecrets } from '../redact.js';
+import { formatBytes, readResultText, webFetchReference } from './preview-utils.js';
+
+function isSuccessReceipt(result: ToolActivityItem['result']): boolean {
+ if (result?.kind !== 'json' || !result.value || typeof result.value !== 'object' || Array.isArray(result.value)) return false;
+ const record = result.value as Record<string, unknown>;
+ const keys = Object.keys(record);
+ return keys.length > 0 && keys.every((key) =>
+ key === 'ok' && record.ok === true
+ || key === 'status' && (record.status === 'completed' || record.status === 'success'),
+ );
+}
+
+export function toolHasDetail(item: ToolActivityItem): boolean {
+ if (item.outputChunks?.length || item.outputTruncated) return true;
+ if (item.status !== 'completed' && item.args !== undefined) return true;
+ if (item.status === 'completed' && isSuccessReceipt(item.result)) return false;
Review Comment:
**P2** (reachability ①) This short-circuits before the args check on line 41, which only covers `status !== 'completed'`. So a tool whose result is just `{ok: true}` and whose content lives entirely in its args ends up with no disclosure arrow and nowhere showing the args.
Verified: `mcp__notes__append` with args `{entries: ['buy milk','call bob'], notebook: 'personal'}` and result `{ok:true}` renders as `"mcp__notes__appendSucceeded"`, `aria-expanded` is `null`, and neither `buy milk` nor `notebook` nor `personal` appears anywhere in the markup. The collapsed target can't recover it either — the default branch only looks for the scalar keys `query/pattern/url/name/title/path/file/id`.
Suppressing a redundant success receipt is right; it just shouldn't swallow the args with it. `return item.args !== undefined;` is enough — `ToolCallDetail`'s `hasSharedPanelContent` already handles the args branch.
<details><summary>中文</summary>
（可达①）这一句在 41 行的 args 检查**之前**短路，而那个检查只覆盖 `status !== 'completed'`。于是一个结果只有 `{ok:true}`、内容全在 args 里的工具，完成后既没有展开箭头，也没有任何地方显示 args。
已验证：`mcp__notes__append`，args 为 `{entries: ['buy milk','call bob'], notebook: 'personal'}`，result `{ok:true}` → 行文本 `"mcp__notes__appendSucceeded"`，`aria-expanded` 为 `null`，整段 markup 里搜不到 `buy milk` / `notebook` / `personal`。折叠标题也救不回来——默认分支只找 `query/pattern/url/name/title/path/file/id` 这几个标量键。
省掉冗余的成功回执是对的，只是不该把 args 一起吞掉。`return item.args !== undefined;` 就够了——`ToolCallDetail` 的 `hasSharedPanelContent` 已经能走 args 分支。
</details>
##########
packages/ui/src/tool-activity/result-projection.ts:
##########
@@ -23,6 +23,51 @@ import type { ToolActivityItem } from '../materialize.js';
import { formatQuietJsonValue } from './builtin-preview.js';
import { isConnectorTool } from './display-name.js';
import { getToolActivityCopy } from './copy.js';
+import { redactSecrets } from '../redact.js';
+import { formatBytes, readResultText, webFetchReference } from './preview-utils.js';
+
+function isSuccessReceipt(result: ToolActivityItem['result']): boolean {
+ if (result?.kind !== 'json' || !result.value || typeof result.value !== 'object' || Array.isArray(result.value)) return false;
+ const record = result.value as Record<string, unknown>;
+ const keys = Object.keys(record);
+ return keys.length > 0 && keys.every((key) =>
+ key === 'ok' && record.ok === true
+ || key === 'status' && (record.status === 'completed' || record.status === 'success'),
+ );
+}
+
+export function toolHasDetail(item: ToolActivityItem): boolean {
+ if (item.outputChunks?.length || item.outputTruncated) return true;
+ if (item.status !== 'completed' && item.args !== undefined) return true;
+ if (item.status === 'completed' && isSuccessReceipt(item.result)) return false;
+ if (item.result?.kind === 'text') return item.result.text.trim().length > 0;
+ return item.result !== undefined;
+}
+
+export function toolResultStats(item: ToolActivityItem, locale: UiLocale): string | undefined {
+ if (item.status !== 'completed') return undefined;
+ const copy = getToolActivityCopy(locale);
+ const result = item.result;
+ if (result?.kind === 'file_write') return formatBytes(result.bytes);
+ if (isSuccessReceipt(result)) return copy.result.success;
+ if (item.toolName === 'WebFetch') return webFetchReference(result?.kind === 'text' ? result.text : '', item.args).title;
+ if (item.toolName === 'Read' && item.args && typeof item.args === 'object' && 'path' in item.args && typeof item.args.path === 'string') {
+ const name = redactSecrets(item.args.path.split(/[\\/]/).pop() ?? item.args.path);
+ const text = readResultText(result);
+ return text === undefined ? name : copy.detail.lines(text === '' ? 0 : text.split('\n').length);
Review Comment:
**P3** `text.split('\n').length` counts one line too many for any file ending in a newline, which is most source files — a 10-line file reads "11 lines". The `text === ''` case is already special-cased, so the edge was considered; this one was missed. Same expression at `tool-result-preview.tsx:158`.
`(text.endsWith('\n') ? text.slice(0, -1) : text).split('\n').length` fixes both, with `''` still taking the zero branch. Note the existing test pins the current behaviour with `['one\n', 2]`, so that changes too.
<details><summary>中文</summary>
`text.split('\n').length` 对以换行结尾的文件多算一行，而绝大多数源码文件都是这样——10 行的文件显示成 "11 lines"。`text === ''` 已经特判过，说明边界是考虑过的，只是漏了这个。`tool-result-preview.tsx:158` 是同一个表达式。
`(text.endsWith('\n') ? text.slice(0, -1) : text).split('\n').length` 两处都能修，`''` 仍走 0 分支。注意现有测试用 `['one\n', 2]` 把当前行为钉死了，要一起改。
</details>
##########
apps/desktop/src/renderer/features/workbar/ports.ts:
##########
@@ -125,6 +126,7 @@ export type WorkbarOpenArtifactResult =
};
export interface WorkbarArtifactsService {
+ readToolResult?(sessionId: string, identity: ToolResultArchiveIdentity): Promise<ArtifactTextReadResult>;
Review Comment:
**P2** `readToolResult?` is optional, but `WorkbarArtifactsService` has exactly one implementer (`create-workbar-services.ts:71`). The `?` exists so the storybook stub at `session-workbar.stories.tsx:783-790` needn't provide it, and it buys a permanently reachable `'unavailable'` state in production code (`tool-output-preview.tsx:44`'s `?.` → `raw === undefined` → `setResult('unavailable')`).
A fixture shouldn't weaken the port. Drop the `?`, add `readToolResult: async () => ({ ok: true, text: archiveText })` to the story stub, and then `tool-output-preview.tsx:44` loses its `?.` and the `'unavailable'` branch at line 87 deletes.
<details><summary>中文</summary>
`readToolResult?` 是可选的，但 `WorkbarArtifactsService` 只有一个实现者（`create-workbar-services.ts:71`）。这个 `?` 的存在只是为了让 `session-workbar.stories.tsx:783-790` 的 storybook 桩不必提供它，代价是生产代码里多了一个永远可达的 `'unavailable'` 状态（`tool-output-preview.tsx:44` 的 `?.` → `raw === undefined` → `setResult('unavailable')`）。
夹具不该削弱 port。删掉 `?`，给 story 桩加上 `readToolResult: async () => ({ ok: true, text: archiveText })`，然后 `tool-output-preview.tsx:44` 的 `?.` 和 87 行的 `'unavailable'` 分支都能删。
</details>
##########
packages/runtime-host/src/server/artifact-coordinator.ts:
##########
@@ -321,13 +356,92 @@ export class HostArtifactCoordinator {
): Promise<OperationOutcome<'artifact.query'>> {
try {
if ((await this.#sessions.probeSessionRemoval(input.sessionId)).kind !== 'present') {
+ for (const [key, transfer] of this.#archiveTransfers) {
+ if (transfer.sessionId === input.sessionId) this.#archiveTransfers.delete(key);
+ }
return notFound('artifact.query', 'Session was not found');
}
let sharedGrantId: string | undefined;
if (context.principalKind === 'session_guest') {
sharedGrantId = await this.#sharedArtifactGrantId(context.principal, input);
if (!sharedGrantId) return notFound('artifact.query', 'Artifact was not found');
}
+ if (input.kind === 'read_archive_chunk') {
+ const unavailable = (reason: ArtifactReadFailureReason) =>
+ querySuccess(
+ encodeArtifactQueryResult({
+ kind: 'archive_unavailable',
+ sessionId: input.sessionId,
+ reason,
+ }),
+ );
+ const identity = parseToolResultArchiveResourceRef(input.ref);
+ if (!identity) return unavailable('not_allowed');
+ if (identity.originalBytes > TOOL_RESULT_ARCHIVE_MAX_BYTES) return unavailable('too_large');
+ if (input.offset > identity.originalBytes) return invalidQuery('Archive offset is invalid');
+ if (!this.#readArchive) return unavailable('read_failed');
+ const now = this.#now();
+ for (const [key, transfer] of this.#archiveTransfers) {
+ if (transfer.expiresAt <= now) this.#archiveTransfers.delete(key);
+ }
+ const key = JSON.stringify([context.connectionId, input.sessionId, input.ref]);
+ let transfer = this.#archiveTransfers.get(key);
+ if (input.offset === 0 || transfer?.ref !== input.ref) {
+ this.#archiveTransfers.delete(key);
+ while (this.#archiveTransfers.size >= MAX_ARCHIVE_TRANSFERS) {
+ this.#archiveTransfers.delete(this.#archiveTransfers.keys().next().value!);
+ }
+ transfer = {
+ connectionId: context.connectionId,
+ sessionId: input.sessionId,
+ ref: input.ref,
+ expiresAt: now + ARCHIVE_TRANSFER_TTL_MS,
+ body: Promise.resolve()
+ .then(() =>
+ this.#readArchive!({
+ ...identity,
+ sessionId: input.sessionId,
+ maxBytes: TOOL_RESULT_ARCHIVE_MAX_BYTES,
Review Comment:
**P3** The only other caller, `readToolResultArchiveResource` (`tool-result-archive-resource.ts:233`), passes `maxBytes: identity.originalBytes`. Passing the 4 MiB cap here turns the ledger reader's `maxBytes < originalBytes → too_large` guard into a no-op and makes the legacy path read against 4 MiB rather than the size the ref itself declares. It can't over-read in practice (`sizeBytes !== originalBytes → size_mismatch` fires first, and byte length is checked again), so this is about not running two conventions on one read path. Use `identity.originalBytes`.
<details><summary>中文</summary>
唯一的另一个调用方 `readToolResultArchiveResource`（`tool-result-archive-resource.ts:233`）传的是 `maxBytes: identity.originalBytes`。这里传 4 MiB 上限会让 ledger reader 的 `maxBytes < originalBytes → too_large` 守卫变成空转，也让 legacy 路径按 4 MiB 而不是 ref 自己声明的尺寸去读。实际读不超（`sizeBytes !== originalBytes → size_mismatch` 会先挡住，byte length 也会再校验一次），所以这条只关乎别在同一条读路径上跑两套约定。改成 `identity.originalBytes`。
</details>
##########
packages/runtime/src/shell-run-tool-result.ts:
##########
@@ -258,10 +257,10 @@ function takePrioritizedText(text: string, budget: number): { text: string; trun
function takeTailText(text: string, budget: number): { text: string; truncated: boolean } {
if (Buffer.byteLength(text, 'utf8') <= budget) return { text, truncated: false };
if (budget <= 0) return { text: '', truncated: text.length > 0 };
- const markerBytes = Buffer.byteLength(TRUNCATED_MARKER, 'utf8');
+ const markerBytes = Buffer.byteLength(PTY_TRUNCATED_MARKER, 'utf8');
Review Comment:
**P2** (reachability ①) `takeTailText` prepends `PTY_TRUNCATED_MARKER` in-band, and the model already receives the structured answer alongside it. Measured on `bashToolResultToModelOutput`: the JSON carries `"truncated": true` in the same object as the marked-up `scrollback`. The marker is redundant for the model, and on the display side only one of three views removes it — `ptyTuiTerminalRows` / `ptyTuiTerminalView` / `ptyCompactTerminalLine` don't, so the CLI transcript (`pi-transcript-tools.ts:292/360/645/714`) renders it as a real output line and it eats 6 lines of budget:
```
tui : ["[terminal snapshot truncated to fit the output limit]","line1","line2","done"]
human: "line1\nline2\ndone"
```
`return { text: sliceUtf8Tail(text, budget), truncated: true }` without the prefix makes all three views agree, and then the `PTY_TRUNCATED_MARKER` export and `stripTruncatedMarker` both delete — which is the WebFetch convention this same PR adopts. If the marker stays, move the strip into `textRows()` so the three views share one removal.
<details><summary>中文</summary>
（可达①）`takeTailText` 把 `PTY_TRUNCATED_MARKER` in-band 前置，而模型本来就同时拿到了结构化答案。实测 `bashToolResultToModelOutput`：JSON 里 `"truncated": true` 和带标记的 `scrollback` 在同一个对象里。对模型而言这个标记是冗余的；显示侧三个视图只有一个剔除它——`ptyTuiTerminalRows` / `ptyTuiTerminalView` / `ptyCompactTerminalLine` 都不剔，所以 CLI transcript（`pi-transcript-tools.ts:292/360/645/714`）会把它当成一行真实输出渲染，还挤占 6 行预算。
改成不带前缀的 `return { text: sliceUtf8Tail(text, budget), truncated: true }`，三个视图自动一致，`PTY_TRUNCATED_MARKER` 的导出和 `stripTruncatedMarker` 随之都能删——这正是同一个 PR 在 WebFetch 上采用的约定。若坚持保留标记，就把剔除放进 `textRows()`，让三个视图共用一次。
</details>
##########
apps/desktop/src/renderer/features/workbar/tools/artifacts/tool-output-preview.tsx:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type { ArtifactReadFailureReason } from '@maka/core/artifacts';
+import { useEffect, useRef, useState } from 'react';
+import { Button, getToolActivityCopy, useClipboardCopyFeedback, useUiLocale, redactSecrets, type ToolOutputOpenRequest } from '@maka/ui';
+import { ArrowLeft, ICON_SIZE } from '@maka/ui/icons';
+import { formatSavedToolJson } from '@maka/core/tool-quiet-preview';
+import { getArtifactCopy } from '../../../../locales/artifact-copy.js';
+import { TextFilePreview } from './artifact-preview.js';
+import { useWorkbarServices } from '../../services-context.js';
+
+export function ToolOutputPreview(props: { request: ToolOutputOpenRequest; onClose(): void }) {
+ const locale = useUiLocale();
+ const copy = getToolActivityCopy(locale);
+ const artifactCopy = getArtifactCopy(locale);
+ const [result, setResult] = useState<{ text: string; partial: boolean } | 'failed' | 'unavailable' | ArtifactReadFailureReason>();
+ const [attempt, setAttempt] = useState(0);
+ const region = useRef<HTMLDivElement>(null);
+ const feedback = useClipboardCopyFeedback();
+ const { artifacts } = useWorkbarServices();
+ useEffect(() => {
+ let cancelled = false;
+ const source = props.request.source;
+ const load = source.kind === 'text'
+ ? Promise.resolve(source.text)
+ : (async () => {
+ const result = await artifacts.readToolResult?.(
+ source.sessionId,
+ source.identity,
+ );
+ return result?.ok ? result.text : result;
+ })();
+ load.then((raw) => {
+ if (cancelled) return;
+ if (raw === undefined) { setResult('unavailable'); return; }
+ if (typeof raw !== 'string') { setResult(raw.reason); return; }
+ let parsed: unknown;
+ try { parsed = JSON.parse(raw); } catch { /* Plain output is valid too. */ }
+ let partial = false;
+ // ArchiveRead wraps a retained page in protocol metadata. Only unwrap
+ // successful body responses; ordinary JSON and diagnostics stay intact.
+ if (props.request.toolName === 'ArchiveRead' && parsed && typeof parsed === 'object' && 'kind' in parsed && parsed.kind === 'tool_result_archive'
Review Comment:
**P2** (reachability ②) The `ArchiveRead` envelope is re-implemented here with string literals — `kind === 'tool_result_archive'`, `operation === 'read' | 'query'`, `content`, `hasMore`, `offset`, `lineOffset`. Its producer is `packages/runtime/src/tool-result-archive-resource.ts` (`pagedContent` at :480, `pagedLines` at :682). That is a second authority for the shape, living in the desktop app, with no test on this side: rename a field on the producer and the viewer silently degrades to showing the raw JSON envelope.
Export `unwrapArchiveReadPage(value): { content: string; partial: boolean } | undefined` from `tool-result-archive-resource.ts` next to the builders and call it here — the existing `tool-result-archive-resource.test.ts` then covers both sides.
<details><summary>中文</summary>
（可达②）`ArchiveRead` 信封在这里用字符串字面量重新实现了一遍——`kind === 'tool_result_archive'`、`operation === 'read' | 'query'`、`content`、`hasMore`、`offset`、`lineOffset`。它的生产者是 `packages/runtime/src/tool-result-archive-resource.ts`（`pagedContent` 在 :480，`pagedLines` 在 :682）。这就是这个形状的第二个权威，住在 desktop app 里，且这一侧没有任何测试：生产者改个字段名，查看器就静默退化成显示原始 JSON 信封。
从 `tool-result-archive-resource.ts` 的构造函数旁边导出 `unwrapArchiveReadPage(value): { content: string; partial: boolean } | undefined`，在这里调用它——现有的 `tool-result-archive-resource.test.ts` 就同时覆盖了两侧。
</details>
##########
packages/ui/src/tool-activity/tool-result-preview.tsx:
##########
@@ -212,29 +257,45 @@ export function ToolResultPreview(props: {
if (content.kind === 'json') {
// No language: quiet text must stay contiguous (tokenizer splits words).
- const quiet = formatQuietJsonValue(content.value, locale);
+ const quiet = formatBoundedQuietJsonValue(content.value, locale);
return (
<div data-kind="json">
- <ToolCodeBlock
- code={formatUserVisibleToolText(quiet.body, locale)}
- title={quiet.headline ? formatUserVisibleToolText(quiet.headline, locale) : undefined}
+ <ToolTextPreview
+ toolName={props.toolName}
+ text={formatUserVisibleToolText(quiet.body, locale)}
+ truncated={quiet.truncated}
+ savedText={JSON.stringify(content.value)}
+ heading={quiet.headline
+ ? formatUserVisibleToolText(quiet.headline, locale)
+ : props.heading}
actionIdentity={props.actionIdentity}
/>
</div>
);
}
if (content.kind === 'text') {
- const copy = getToolActivityCopy(locale).result;
- const { body, capped } = capLines(formatUserVisibleToolText(redactSecrets(content.text), locale));
- const code = capped > 0 ? `${body}\n\n${copy.hiddenLines(capped)}` : body;
+ if (props.toolName === 'WebFetch' && !props.failed) {
+ return <WebFetchPreview content={content} args={props.args} />;
+ }
return (
<div data-kind="text">
- <ToolCodeBlock code={code} actionIdentity={props.actionIdentity} />
+ <ToolTextPreview
+ // No heading: the collapsed row above already carries the invocation,
+ // and it can be an archive ref — repeating it here wraps a raw URI
+ // across the block's title slot.
+ text={formatUserVisibleToolText(redactSecrets(content.text), locale)}
+ savedText={content.text}
+ actionIdentity={props.actionIdentity}
+ />
</div>
);
}
+ if (content.kind === 'summary') {
Review Comment:
**P3** The new `summary` branch was inserted directly above the comment `// image / summary / unknown — show a compact descriptor…`, which is now wrong — `summary` no longer takes that path. Drop `summary` from the comment.
While here, `content.original` on this branch has no fallback. It's required by the type, but since the path is new it's cheap to guard alongside the malformed-result fixes in the review body.
<details><summary>中文</summary>
新增的 `summary` 分支插在了注释 `// image / summary / unknown — show a compact descriptor…` 的正上方，这条注释现在是错的——`summary` 已经不走那条路径了。把 `summary` 从注释里删掉。
顺带：这个分支上的 `content.original` 没有兜底。类型上它是必填，但既然这条路径是新加的，跟正文里那两处畸形结果的修复一起加个兜底很省事。
</details>
##########
packages/ui/src/tool-activity/tool-text-preview.tsx:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useContext, useEffect, useRef, useSyncExternalStore, type ComponentProps } from 'react';
+import { createTranscriptScrollAuthority } from '../transcript-scroll-authority.js';
+import { Button } from '@astryxdesign/core';
+import { FileText, ICON_SIZE } from '../icons.js';
+import { useUiLocale } from '../locale-context.js';
+import { redactSecrets } from '../redact.js';
+import type { ToolOutputSource } from './tool-result-context.js';
+import { ToolResultHostContext } from './tool-result-context.js';
+import { getToolActivityCopy } from './copy.js';
+import { ToolCodeBlock } from './tool-code-block.js';
+import { capLines } from './preview-utils.js';
+
+/** Reading retained output belongs to the host's artifact viewer. */
+export function SavedToolOutput(props: {
+ source: ToolOutputSource;
+ toolName?: string;
+ actionIdentity?: string;
+}) {
+ const copy = getToolActivityCopy(useUiLocale()).detail;
+ const host = useContext(ToolResultHostContext);
+ return <span title={!host ? copy.unavailable : undefined}><Button variant="secondary" size="sm" label={copy.viewSaved}
+ icon={<FileText size={ICON_SIZE.chrome} aria-hidden="true" />}
+ isDisabled={!host}
+ onClick={(event) => {
+ // Pointer activation does not focus buttons in every browser. The host
+ // records this element before transferring focus into the viewer.
+ event.currentTarget.focus();
+ host?.({ title: props.actionIdentity ?? copy.viewSaved, toolName: props.toolName,
+ source: props.source });
+ }}
+ /></span>;
+}
+
+/** Full in-memory text in a height-bounded CodeBlock; only structured JSON
+ * truncation keeps an escape hatch to a separate viewer. */
+export function ToolTextPreview(props: {
+ text: string;
+ heading?: string;
+ actionIdentity?: string;
+ truncated?: boolean;
+ toolName?: string;
+ savedText?: string;
+}) {
+ const copy = getToolActivityCopy(useUiLocale()).detail;
+ const safe = redactSecrets(props.text);
+ const capped = capLines(safe, { chars: 120_000 });
+ const code = capped.body;
+ const previewTruncated = capped.capped > 0 || capped.hiddenChars > 0;
+ const needsSavedOutput = props.truncated || previewTruncated;
+ return (
+ <>
+ <ToolCodeBlock code={code} title={props.heading} actionIdentity={props.actionIdentity} />
+ {needsSavedOutput && <>
+ <p className="maka-tool-output-note">{copy.previewTruncated}</p>
+ <SavedToolOutput toolName={props.toolName}
+ source={{ kind: 'text', text: props.savedText ?? safe }} actionIdentity={props.actionIdentity} />
+ </>}
+ </>
+ );
+}
+
+
+/** The transcript's scroll owner, scoped to one tool output viewport. */
+export function ToolOutputScroller(props: ComponentProps<'pre'>) {
+ const { role = 'region', 'aria-label': ariaLabel, tabIndex = 0, ...preProps } = props;
+ const ref = useRef<HTMLPreElement>(null);
+ const authority = useRef<ReturnType<typeof createTranscriptScrollAuthority> | undefined>(undefined);
+ authority.current ??= createTranscriptScrollAuthority({ explicitResume: true });
+ const scroll = authority.current;
+ const snapshot = useSyncExternalStore(scroll.subscribe, scroll.getSnapshot, scroll.getSnapshot);
+ const copy = getToolActivityCopy(useUiLocale());
+ useEffect(() => scroll.attach(ref.current), [scroll]);
+ useEffect(() => {
+ if (scroll.getSnapshot().pinned) scroll.pinToTail();
+ }, [props.children, scroll]);
+ return <div className="maka-tool-output-scroller">
+ <pre {...preProps} ref={ref} role={role} aria-label={ariaLabel ?? copy.detail.outputRegion} tabIndex={tabIndex} />
+ {!snapshot.pinned && <Button className="maka-tool-output-jump" variant="ghost" size="sm" label={copy.detail.jumpToBottom}
Review Comment:
**P3** Wrong snapshot field. With `explicitResume: true`, `pinned` never returns to true on its own, so once a reader scrolls away the button stays mounted even after they scroll back to the bottom by hand — and clicking it then does nothing visible.
The authority already maintains exactly the right field: `awayFromTail`, with `BUTTON_THRESHOLD_PX = 100`, the same threshold the transcript's own return-to-bottom button uses. `{snapshot.awayFromTail && …}` also puts the inner and outer buttons on one visibility rule.
<details><summary>中文</summary>
用错了 snapshot 字段。配上 `explicitResume: true` 之后 `pinned` 再也不会自己变回 true，所以读者一旦滚开，即使手动滚回底部按钮仍然挂着，点它也看不到任何变化。
authority 已经维护了正好合适的字段：`awayFromTail`，阈值 `BUTTON_THRESHOLD_PX = 100`，和 transcript 自己那颗回到底部按钮同一套。改成 `{snapshot.awayFromTail && …}` 还能让内外两颗按钮遵循同一条可见性规则。
</details>
##########
apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts:
##########
@@ -76,6 +79,23 @@ export function registerRuntimeHostArtifactsIpc(
(_event, sessionId: string, artifactId: string) =>
deps.client.readArtifactBinary(sessionId, artifactId),
);
+ handleReconnectableRead(
+ deps.ipcMain,
+ 'artifacts:readToolResult',
+ async (_event, sessionId: string, identity: ToolResultArchiveIdentity): Promise<ArtifactTextReadResult> => {
+ if (!identity || (!identity.resourceRef && typeof identity.artifactId !== 'string')
Review Comment:
**P3** (reachability ③) `{ artifactId, resourceRef }` with both present is accepted and `resourceRef` silently wins, while `ToolResultArchiveIdentity` declares them mutually exclusive (`artifactId?: never`) — and the PR's own new test at `runtime-host-artifacts-ipc-main.test.ts:57` constructs exactly that forbidden combination. Confirmed by running: `{artifactId:'a', resourceRef:'maka://archive/b/<sha>/5'}` reaches the Host as the `b` ref.
Low impact — the Host re-parses, the reader re-verifies the sha, and the renderer already reaches arbitrary artifact ids through `artifacts:readText`. But the guard should reject what the type says can't exist: add `|| (identity.resourceRef !== undefined && identity.artifactId !== undefined)`.
<details><summary>中文</summary>
（可达③）`{ artifactId, resourceRef }` 两者都在时会被接受且 `resourceRef` 静默胜出，而 `ToolResultArchiveIdentity` 声明它们互斥（`artifactId?: never`）——PR 自己新增的测试 `runtime-host-artifacts-ipc-main.test.ts:57` 构造的正是这个被禁止的组合。运行确认：`{artifactId:'a', resourceRef:'maka://archive/b/<sha>/5'}` 到达 Host 时是 `b` 那个 ref。
影响很小——Host 会重新解析，reader 会重新校验 sha，而且渲染侧本来就能经 `artifacts:readText` 取到任意 artifact id。但 guard 应该拒绝类型声称不可能存在的东西：加上 `|| (identity.resourceRef !== undefined && identity.artifactId !== undefined)`。
</details>
##########
apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts:
##########
@@ -50,7 +53,7 @@ type RuntimeHostAttachmentPreviewIpcDeps = Pick<
'ipcMain' | 'client'
>;
-const ATTACHMENT_PREVIEW_LIMIT_EXCEEDED = Symbol("attachment-preview-limit-exceeded");
+const ARTIFACT_READ_LIMIT_EXCEEDED = Symbol("artifact-read-limit-exceeded");
Review Comment:
**P3** `ATTACHMENT_PREVIEW_LIMIT_EXCEEDED` → `ARTIFACT_READ_LIMIT_EXCEEDED` is unrelated churn. The symbol is still thrown and caught only inside `registerRuntimeHostAttachmentPreviewIpc` (:186, :191), the new archive path never touches it, and the broader name is now less accurate than the old one. Revert all three lines.
<details><summary>中文</summary>
`ATTACHMENT_PREVIEW_LIMIT_EXCEEDED` → `ARTIFACT_READ_LIMIT_EXCEEDED` 是无关改动。这个符号仍然只在 `registerRuntimeHostAttachmentPreviewIpc` 内部抛出和捕获（:186、:191），新的归档路径从不碰它，而更宽泛的名字现在反而比旧名字更不准确。三行都回退。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): improve tool output rendering and interaction [maka]
Posted by "colaforniaw (via GitHub)" <gi...@apache.org>.
colaforniaw closed pull request #5030: feat(desktop): improve tool output rendering and interaction
URL: https://github.com/apache/maka/pull/5030
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): improve tool output rendering and interaction [maka]
Posted by "colaforniaw (via GitHub)" <gi...@apache.org>.
colaforniaw commented on PR #5030:
URL: https://github.com/apache/maka/pull/5030#issuecomment-5629044620
@me2seeks Thanks for the thorough review. I’ve addressed the P2 issue and removed the unused props, copy, branches, and styling noted in the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): improve tool output rendering and interaction [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5030:
URL: https://github.com/apache/maka/pull/5030#discussion_r3960627104
##########
packages/ui/src/tool-activity/result-projection.ts:
##########
@@ -23,6 +23,52 @@ import type { ToolActivityItem } from '../materialize.js';
import { formatQuietJsonValue } from './builtin-preview.js';
import { isConnectorTool } from './display-name.js';
import { getToolActivityCopy } from './copy.js';
+import { redactSecrets } from '../redact.js';
+import { formatBytes, readResultText, webFetchReference } from './preview-utils.js';
+
+function isSuccessReceipt(result: ToolActivityItem['result']): boolean {
+ if (result?.kind === 'file_write') return true;
Review Comment:
**[P2] Keep the full destination path inspectable for Write results**
Could we retain a detail view for `file_write` results? This is a normal Write outcome when the runtime does not return a diff (for example, a 70,000-byte write). The collapsed row now shows only the basename, and treating this result as a status-only receipt disables its disclosure, so a write to `/repo/packages/backend/config.json` leaves no way to inspect the full destination from the tool row. This makes same-named files difficult to distinguish. I compared the same result against the base: the full path and disclosure were present there, and both are absent here. Keeping the compact basename while preserving the full path in the existing detail view would address it.
##########
packages/ui/src/tool-activity/tool-result-preview.tsx:
##########
@@ -212,29 +257,45 @@ export function ToolResultPreview(props: {
if (content.kind === 'json') {
// No language: quiet text must stay contiguous (tokenizer splits words).
- const quiet = formatQuietJsonValue(content.value, locale);
+ const quiet = formatBoundedQuietJsonValue(content.value, locale);
return (
<div data-kind="json">
- <ToolCodeBlock
- code={formatUserVisibleToolText(quiet.body, locale)}
- title={quiet.headline ? formatUserVisibleToolText(quiet.headline, locale) : undefined}
+ <ToolTextPreview
+ toolName={props.toolName}
+ text={formatUserVisibleToolText(quiet.body, locale)}
+ truncated={quiet.truncated}
+ savedText={JSON.stringify(content.value)}
+ heading={quiet.headline
+ ? formatUserVisibleToolText(quiet.headline, locale)
+ : props.heading}
actionIdentity={props.actionIdentity}
/>
</div>
);
}
if (content.kind === 'text') {
- const copy = getToolActivityCopy(locale).result;
- const { body, capped } = capLines(formatUserVisibleToolText(redactSecrets(content.text), locale));
- const code = capped > 0 ? `${body}\n\n${copy.hiddenLines(capped)}` : body;
+ if (props.toolName === 'WebFetch' && !props.failed) {
+ return <WebFetchPreview content={content} args={props.args} />;
+ }
return (
<div data-kind="text">
- <ToolCodeBlock code={code} actionIdentity={props.actionIdentity} />
+ <ToolTextPreview
+ // No heading: the collapsed row above already carries the invocation,
+ // and it can be an archive ref — repeating it here wraps a raw URI
+ // across the block's title slot.
+ text={formatUserVisibleToolText(redactSecrets(content.text), locale)}
Review Comment:
**Non-blocking: could we measure the long-text path?**
This now passes the complete text through `ToolTextPreview` to `CodeBlock`, whereas the previous path capped it at 500 lines. The 208px height limit bounds the visible area, but the complete text still reaches the renderer. I have not reproduced a slowdown, so this is a request for validation rather than a confirmed regression. Could we compare base and head with a representative large retained text result (including many short lines), checking expansion time and responsiveness while scrolling? If the cost is material, we could keep a bounded inline preview and use the existing saved-output viewer for the full text; if the measurements are healthy, recording them would be sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] feat(desktop): improve tool output rendering and interaction [maka]
Posted by "colaforniaw (via GitHub)" <gi...@apache.org>.
colaforniaw commented on PR #5030:
URL: https://github.com/apache/maka/pull/5030#issuecomment-5693354091
Closing this without merging.
In hindsight this PR bundles too many independent concerns — archived full-output access (new IPC + validation), WebFetch citations, shell tail-following, text preview bounding, tool labels, and typography — which makes it fragile to land and hard to review meaningfully, and the overall value doesn't justify that risk in one shot.
I'll follow up with a better sequence instead:
1. A no-behavior-change refactor that restructures how tool results are rendered (per-tool renderers behind a registry, shared bounded-preview primitives), so future changes touch one tool at a time.
2. The individual improvements as separate, single-concern PRs on top of that base — typography, WebFetch citations, shell tail-following, etc. — each small and independently revertable.
3. The archived full-output viewer as its own dedicated PR, since it deserves focused review of the IPC surface and validation.
Thanks for the early looks — the feedback here will fold into the split-up work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org