Skip to content

test(desktop): story-cover the terminal panel's failure and edge states - #4190

Open
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:liuxiaocs7/terminal-panel-failure-states
Open

test(desktop): story-cover the terminal panel's failure and edge states#4190
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:liuxiaocs7/terminal-panel-failure-states

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Aug 29, 2026

Copy link
Copy Markdown
Member

test(desktop): story-cover the terminal panel's failure and edge states

Fifth surface under #3944 (one surface per PR): extend the 终端 panel
(SessionTerminalPanel, via Product/Session Workbar) with the states the
fresh-shell Terminal story never shows, driving the real panel through the fake
terminal service.

  • TerminalLoadFailed — terminal.attach resolves to null: the load-failed
    Banner over the terminal surface.
  • TerminalLongScrollback — hundreds of lines of scrollback (very many rows).
  • TerminalRichOutput — coloured, bold, and carriage-return progress output.
  • TerminalWriteFailed — a keystroke whose terminal.write rejects raises the
    write-failed Banner; input is driven through xterm's helper textarea, so the
    state is reachable from a mounted story (review feedback).

bridge() gains terminalAttach / terminalBuffer / terminalWriteFails
options. The empty state needs a null-ref terminal tab the workbar never
creates, so it is left out rather than faked.

Refs #3944, #3893

Generative tooling

Claude Code contributed substantially here — it authored these Storybook stories. The human contributor of record reviewed and submitted the change, and each commit carries a Generated-by: Claude Code trailer per CONTRIBUTING.md.

Visual evidence

Storybook stories captured with Playwright (hosted on the fork's release assets).

terminal-load-failed — normal / light · narrow / dark

terminal-load-failed normal / light terminal-load-failed narrow / dark

terminal-long-scrollback — normal / light · narrow / dark

terminal-long-scrollback normal / light terminal-long-scrollback narrow / dark

terminal-rich-output — normal / light · narrow / dark

terminal-rich-output normal / light terminal-rich-output narrow / dark

terminal-write-failed — normal / light · narrow / dark

terminal-write-failed normal / light terminal-write-failed narrow / dark

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping this to the existing Workbar terminal story seam. The current head only merges the latest main; the substantive story diff remains the reviewed one.

I reviewed exact head 96a713a1380ae128d86643d151393eea5718f42e. The exact-head test check is now green, and I found no P0 or P1 issues. The PR body still contains no screenshots or recordings of the new terminal states. Please attach the load-failed banner, hydrated long scrollback, and ANSI/carriage-return output after xterm has fully mounted, at normal and narrow Workbar widths.

I found one non-blocking P2 coverage gap. The PR says the write-failed state cannot be reached from a mounted story because live xterm keystrokes are unavailable. The existing accessibility runtime-surface story already demonstrates input through .xterm-helper-textarea with userEvent.keyboard, while SessionTerminalPanel has a real write-error branch. The state is therefore reachable. Please add a rejecting terminal.write fake plus a focused play, or narrow the PR claim rather than declaring the state unreachable.

The null-ref empty state is a much lower-value legacy recovery path and does not need to expand this PR.

For visual review, please confirm that 600 lines hydrate without layout breakage, ANSI colors and emphasis remain readable in both themes, carriage-return progress resolves correctly, and the failure Banner follows existing Astryx styling.

Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head, current-main-only delta, production terminal path, existing xterm input seam, CI, and severity judgment, and owns this review.

中文对照

谢谢你把改动限定在既有 Workbar terminal Story seam。当前 head 只是合并最新 main,实质 Story diff 与已审查版本一致。

我审查了精确 head 96a713a1380ae128d86643d151393eea5718f42e。exact-head test 已恢复绿色,没有 P0/P1。PR 正文仍没有新 terminal 状态的截图或录屏。请在 xterm 完全挂载后,补充 load-failed Banner、长 scrollback 和 ANSI/回车重绘输出,包括正常与窄 Workbar 宽度。

有一个不阻塞的 P2 覆盖缺口。PR 认为 mounted Story 无法输入 live xterm keystroke,因此 write-failed 不可达;但现有 accessibility runtime-surface Story 已经通过 .xterm-helper-textareauserEvent.keyboard 完成输入,SessionTerminalPanel 也存在真实 write-error 分支。这个状态实际上可达。请增加 rejecting terminal.write fake 和聚焦 play;或者收窄 PR 声明,不要把它描述为不可达。

null-ref empty state 只是更低价值的 legacy recovery 路径,不需要因此扩大本 PR。

人工视觉验收还需确认:600 行内容 hydrate 后布局稳定;ANSI 颜色和强调在浅色、深色主题下清晰;回车进度重绘结果正确;failure Banner 使用既有 Astryx 样式。

本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、仅合并 current-main 的增量、生产 terminal 路径、既有 xterm 输入 seam、CI 和问题分级,并对本次 Review 负责。

@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/terminal-panel-failure-states branch 2 times, most recently from cea78e8 to 5e8c377 Compare August 30, 2026 09:20

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening this after the earlier review. The current PR-owned diff now covers the four meaningful terminal states through the existing Workbar story seam, without reimplementing the product panel.

The previous P2 is closed: TerminalWriteFailed drives input through xterm's mounted .xterm-helper-textarea, makes the real terminal.write boundary reject, and asserts the production write-failed Banner. The PR body also now includes normal/light and narrow/dark evidence for load failure, long scrollback, ANSI/carriage-return output, and write failure. Leaving the low-value null-ref empty state out is the right scope decision.

I found no P0/P1 issues. The exact-head test check is green, and this is Storybook coverage only; it does not change product UI behavior. Approved at 5e8c377de5b25637234611e020f0f3b56574e01e. GitHub currently reports the branch as conflicting with main, so please rebase and let us incrementally confirm the resulting exact head before merge.

AI-assisted review (OpenAI Codex). I verified the PR-owned diff, mounted xterm input path, production Banner path, evidence update, CI, and merge state.

中文对照

感谢你根据之前的反馈收敛实现。当前 PR 自有 diff 已经通过既有 Workbar Story seam 覆盖四个有价值的终端状态,没有重新实现产品面板。

旧 P2 已闭合:TerminalWriteFailed 通过已挂载 xterm 的 .xterm-helper-textarea 输入,让真实 terminal.write 边界拒绝,并断言生产实现的写入失败 Banner。PR 正文也补齐了 load failure、长 scrollback、ANSI/回车重绘和 write failure 的 normal/light、narrow/dark 证据。没有为了完整矩阵强行加入低价值的 null-ref empty state,范围合理。

未发现 P0/P1,exact-head test 已通过;本 PR 只增加 Storybook 覆盖,不改变产品 UI 行为。已批准 5e8c377de5b25637234611e020f0f3b56574e01e。GitHub 当前仍报告它与 main 冲突,因此请先 rebase,我们会在新 exact head 上做增量确认后再合并。

本次为 AI 辅助审查(OpenAI Codex)。我核对了 PR 自有 diff、mounted xterm 输入路径、生产 Banner 路径、证据更新、CI 和合并状态。

Fifth surface under apache#3944 (one surface per PR): extend the 终端 panel
(SessionTerminalPanel, via Product/Session Workbar) with the states the
fresh-shell Terminal story never shows, driving the real panel through the fake
terminal service.

- TerminalLoadFailed — `terminal.attach` resolves to null: the load-failed
  Banner over the terminal surface.
- TerminalLongScrollback — hundreds of lines of scrollback (very many rows).
- TerminalRichOutput — coloured, bold, and carriage-return progress output.
- TerminalWriteFailed — a keystroke whose `terminal.write` rejects raises the
  write-failed Banner; input is driven through xterm's helper textarea, so the
  state is reachable from a mounted story (review feedback).

`bridge()` gains `terminalAttach` / `terminalBuffer` / `terminalWriteFails`
options. The empty state needs a null-ref terminal tab the workbar never
creates, so it is left out rather than faked.

Refs apache#3944, apache#3893

Generated-by: Claude Code
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/terminal-panel-failure-states branch from 5e8c377 to e472974 Compare August 30, 2026 13:04

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at exact head e472974ba773c10d02fb5b17992af571d0a1f23f; no P0-P3 findings.

This revision keeps the change in the existing SessionTerminalPanel Storybook seam and covers the four intended states: attach/load failure, 600-line scrollback, ANSI plus carriage-return rendering, and an actual mounted-xterm write rejection driven through .xterm-helper-textarea. I traced the failure fakes to the production attach and write-error branches rather than story-only replicas.

Validation: clean dependency install; npm run build:test; Desktop story typecheck; changed-file Biome; git diff --check; Storybook production build; and the visual smoke over all 238 stories, including the four new terminal stories. The current head is directly based on current main, GitHub reports MERGEABLE/CLEAN, and the exact-head test check is green.

Residual limitation: I did not independently capture or compare the submitted visual screenshots pixel by pixel; the Storybook smoke verified renderability and play assertions.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@liuxiaocs7
liuxiaocs7 requested a review from Astro-Han August 30, 2026 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants