fix: address Copilot review round 3 (code-point truncation, a11y live status, disabled-tab wait mirroring, test unmount, concurrent-wait test)
This commit is contained in:
@@ -150,3 +150,18 @@ needle 出现 / 超时 / 终端退出。等待期间用户在侧边栏看不到
|
||||
实时等待状态。修复需跨会话 wait feed 多路复用(连 home 会话推送或聚合
|
||||
推送),超出本特性范围,留待后续设计;跳过 API 按 uuid 全局寻址,跨会话
|
||||
跳过本身可达。
|
||||
- **Copilot review 第三轮修复(合并 rc.1 后扫描)**:
|
||||
1. `truncateNeedle` 改按 Unicode 码点截断(`Array.from`),原 UTF-16 码元
|
||||
截断会在截断点劈开代理对(emoji 渲染为替换字符);
|
||||
2. banner 消息加 `role="status"` + `aria-live="polite"`,完整未截断的
|
||||
本地化消息经 `aria-label` 提供给读屏器(不新增词典键);
|
||||
3. `terminal` tab 类型禁用时推送处理从整体跳过改为 `mirrorAgentWaits`
|
||||
——仍镜像权威 `waiting` map(等在禁用窗口内 resolve 也清 banner 状态),
|
||||
只跳过 tab 增删 reconcile,否则重启用后已开终端残留 stale banner/⏳
|
||||
直到无关推送到达;
|
||||
4. badge 测试跟踪挂载的 root 并在 `afterEach` 里先 unmount 再清 DOM
|
||||
(裸 `innerHTML = ''` 不触发 React effect 清理,store 订阅/host-feed
|
||||
effect 会泄漏进后续测试);
|
||||
5. 补并发双 `waitFor` 同 uuid 测试:快照显示最新 needle(`waits.at(-1)`)、
|
||||
`skipWait` 返回 2、两个 promise 都以各自 needle `skipped` 收敛、快照
|
||||
清空且 skipWait 幂等归零。
|
||||
|
||||
@@ -12,16 +12,27 @@ import css from './sidebar.module.css'
|
||||
/** Cap the needle shown inline; the full text rides the title tooltip. */
|
||||
const NEEDLE_DISPLAY_CAP = 80
|
||||
|
||||
/** Truncate one needle for inline display (title attr carries the full text). */
|
||||
/** Truncate one needle for inline display (title attr carries the full text).
|
||||
* Cut by Unicode code points, not UTF-16 code units — a naive slice can split
|
||||
* a surrogate pair at the cutoff and render a dangling replacement char. */
|
||||
export function truncateNeedle(needle: string): string {
|
||||
return needle.length > NEEDLE_DISPLAY_CAP ? `${needle.slice(0, NEEDLE_DISPLAY_CAP - 1)}…` : needle
|
||||
const points = Array.from(needle)
|
||||
return points.length > NEEDLE_DISPLAY_CAP
|
||||
? `${points.slice(0, NEEDLE_DISPLAY_CAP - 1).join('')}…`
|
||||
: needle
|
||||
}
|
||||
|
||||
export function TerminalWaitBanner(props: { needle: string; onSkip: () => void }) {
|
||||
const { needle, onSkip } = props
|
||||
return (
|
||||
<div className={css.terminalWaitBanner}>
|
||||
<span className={css.terminalWaitNeedle} title={needle}>
|
||||
<span
|
||||
className={css.terminalWaitNeedle}
|
||||
title={needle}
|
||||
role="status"
|
||||
aria-live="polite"
|
||||
aria-label={t('terminalWaitBanner', { needle })}
|
||||
>
|
||||
{t('terminalWaitBanner', { needle: truncateNeedle(needle) })}
|
||||
</span>
|
||||
<button type="button" className={css.terminalRetry} onClick={onSkip}>
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
*/
|
||||
import { useEffect, useRef } from 'react'
|
||||
import type { Context, SidebarSessionList } from '../../context-types.ts'
|
||||
import { reconcileAgentTerminals, type SidebarStore } from '../state.ts'
|
||||
import { mirrorAgentWaits, reconcileAgentTerminals, type SidebarStore } from '../state.ts'
|
||||
import { detectNewDirectSubagent } from '../subagent-detect.ts'
|
||||
import { detectNewJob } from '../subagent-jobs.ts'
|
||||
import { t } from '../locales.ts'
|
||||
@@ -65,7 +65,12 @@ export function useHostFeeds(feeds: {
|
||||
const list = JSON.parse(event.data) as Array<{ uuid: string; title: string; command: string; exited: boolean; waiting?: { needle: string; since: number } | null }>
|
||||
if (!Array.isArray(list)) return
|
||||
store.reduce(s => ctx.get('betterSidebar')?.isTabEnabled('terminal') === false
|
||||
? s
|
||||
// Terminal tabs are disabled: skip tab add/remove reconciliation,
|
||||
// but STILL mirror the authoritative wait map — a wait resolving
|
||||
// during the disabled window must clear its banner state, or a
|
||||
// re-enabled terminal keeps a stale banner until the next
|
||||
// unrelated push.
|
||||
? mirrorAgentWaits(s, list)
|
||||
: reconcileAgentTerminals(s, list))
|
||||
} catch {
|
||||
// Malformed push: ignore (the next push will reconcile).
|
||||
|
||||
+31
-6
@@ -687,6 +687,19 @@ function sameAgentWaits(
|
||||
return true
|
||||
}
|
||||
|
||||
/** Fold the pushed terminal snapshots into the authoritative wait map. */
|
||||
function serverWaitsOf(
|
||||
agentTerminals: ReadonlyArray<{ uuid: string; title: string; waiting?: { needle: string; since: number } | null }>,
|
||||
): Record<string, { needle: string; since: number }> {
|
||||
const serverWaits: Record<string, { needle: string; since: number }> = {}
|
||||
for (const terminal of agentTerminals) {
|
||||
if (terminal.waiting !== undefined && terminal.waiting !== null) {
|
||||
serverWaits[terminal.uuid] = { needle: terminal.waiting.needle, since: terminal.waiting.since }
|
||||
}
|
||||
}
|
||||
return serverWaits
|
||||
}
|
||||
|
||||
/**
|
||||
* Reconcile the sidebar's agent-terminal tabs with the host's live list.
|
||||
* The host pushes the current list of agent terminals (created by the model
|
||||
@@ -719,12 +732,7 @@ export function reconcileAgentTerminals(
|
||||
// waiting field simply drops the entry). A waits-only change must still
|
||||
// produce a new state — the tab add/remove no-change check alone would
|
||||
// swallow banner updates.
|
||||
const serverWaits: Record<string, { needle: string; since: number }> = {}
|
||||
for (const terminal of agentTerminals) {
|
||||
if (terminal.waiting !== undefined && terminal.waiting !== null) {
|
||||
serverWaits[terminal.uuid] = { needle: terminal.waiting.needle, since: terminal.waiting.since }
|
||||
}
|
||||
}
|
||||
const serverWaits = serverWaitsOf(agentTerminals)
|
||||
if (toAdd.length === 0 && toRemove.length === 0 && sameAgentWaits(state.agentWaits, serverWaits)) return state
|
||||
// Remove tabs whose uuids vanished from the server list (the agent closed
|
||||
// them, or the pty exited and was reaped). Reuse closeTab's leaf cleanup.
|
||||
@@ -750,6 +758,23 @@ export function reconcileAgentTerminals(
|
||||
return { ...next, agentWaits: serverWaits }
|
||||
}
|
||||
|
||||
/**
|
||||
* Mirror ONLY the authoritative agent-wait map from a push — no tab
|
||||
* add/remove reconciliation. Used while the `terminal` tab type is disabled:
|
||||
* the tab surface is frozen, but a wait that resolves during that window
|
||||
* must still clear its banner state, or a re-enabled terminal keeps a stale
|
||||
* banner/⏳ until some unrelated host event fires the next full reconcile.
|
||||
* Idempotent: a no-op when the map already matches.
|
||||
*/
|
||||
export function mirrorAgentWaits(
|
||||
state: SidebarState,
|
||||
agentTerminals: ReadonlyArray<{ uuid: string; title: string; waiting?: { needle: string; since: number } | null }>,
|
||||
): SidebarState {
|
||||
const serverWaits = serverWaitsOf(agentTerminals)
|
||||
if (sameAgentWaits(state.agentWaits, serverWaits)) return state
|
||||
return { ...state, agentWaits: serverWaits }
|
||||
}
|
||||
|
||||
// ── The per-session store ──────────────────────────────────────────────────
|
||||
|
||||
const STORAGE_PREFIX = 'dsh-sidebar:v1'
|
||||
|
||||
@@ -430,6 +430,31 @@ describe('AgentPtyRegistry', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('concurrent waits on one uuid: snapshot shows the latest needle, skipWait skips all', async () => {
|
||||
const registry = new AgentPtyRegistry(testShell())
|
||||
try {
|
||||
const uuid = registry.create('s1', 'concurrent', '', process.cwd(), 80, 24)
|
||||
// The bare shell never emits either needle → both waits miss the fast
|
||||
// paths and register their records (synchronously, before waitFor's
|
||||
// first poll await — the documented concurrent-wait contract).
|
||||
const first = registry.waitFor(uuid, 'NEVER_A_XYZ', 60_000)
|
||||
const second = registry.waitFor(uuid, 'NEVER_B_XYZ', 60_000)
|
||||
// The banner mirrors the LATEST wait (waits.at(-1)).
|
||||
expect(registry.list('s1')[0]?.waiting?.needle).toBe('NEVER_B_XYZ')
|
||||
expect(typeof registry.list('s1')[0]?.waiting?.since).toBe('number')
|
||||
// One skip transitions EVERY active wait on the terminal.
|
||||
expect(registry.skipWait(uuid)).toBe(2)
|
||||
expect(await first).toEqual({ kind: 'skipped', needle: 'NEVER_A_XYZ' })
|
||||
expect(await second).toEqual({ kind: 'skipped', needle: 'NEVER_B_XYZ' })
|
||||
// Both resolved → the wait state cleared from the snapshot.
|
||||
expect(registry.list('s1')[0]?.waiting).toBeUndefined()
|
||||
// Idempotent: nothing left to skip.
|
||||
expect(registry.skipWait(uuid)).toBe(0)
|
||||
} finally {
|
||||
registry.disposeAll()
|
||||
}
|
||||
})
|
||||
|
||||
it('fires change listeners when a wait starts and ends', async () => {
|
||||
const registry = new AgentPtyRegistry(testShell())
|
||||
try {
|
||||
|
||||
@@ -33,6 +33,11 @@ class FakeWebSocket {
|
||||
/** Unique per-test session ids (see the comment inside mountSidebar). */
|
||||
let sessionSeq = 0
|
||||
|
||||
/** Roots mounted by the current test; afterEach unmounts them (reverse
|
||||
* order) so React effect cleanup (store subscription, host-feed sockets,
|
||||
* debounced persistence) actually runs between tests. */
|
||||
const mounted: Array<() => void> = []
|
||||
|
||||
function mountSidebar(): { container: HTMLDivElement; store: ReturnType<typeof createSidebarStore>; service: BetterSidebarService; unmount: () => void } {
|
||||
vi.stubGlobal('WebSocket', FakeWebSocket)
|
||||
FakeWebSocket.instances = []
|
||||
@@ -66,18 +71,19 @@ function mountSidebar(): { container: HTMLDivElement; store: ReturnType<typeof c
|
||||
}
|
||||
const root: Root = createRoot(container)
|
||||
act(() => { root.render(createElement(Sidebar, { ctx: ctx as never, store })) })
|
||||
return {
|
||||
container,
|
||||
store,
|
||||
service,
|
||||
unmount: () => {
|
||||
const unmount = (): void => {
|
||||
act(() => { root.unmount() })
|
||||
container.remove()
|
||||
},
|
||||
}
|
||||
mounted.push(unmount)
|
||||
return { container, store, service, unmount }
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
// Unmount BEFORE wiping the DOM: root.unmount() must run the shell's
|
||||
// effect cleanups (store subscription, WS close, persistence debounce);
|
||||
// document.body.innerHTML = '' alone leaves them alive into later tests.
|
||||
for (const unmount of mounted.splice(0).reverse()) unmount()
|
||||
document.body.innerHTML = ''
|
||||
// Belt and braces (same as bottom-auto-terminal): drop any persisted layout
|
||||
// a pending 200ms debounce write left behind between tests. Fully guarded —
|
||||
|
||||
Reference in New Issue
Block a user