Skip to content

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

Open
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:liuxiaocs7/diff-viewer-failure-states
Open

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

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Aug 29, 2026

Copy link
Copy Markdown
Member

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

Fourth surface under #3944 (one surface per PR): extend the 变更 panel
(SessionReviewPanel, via Product/Session Workbar) with the failure and edge
states that never show up in normal use. All drive the real panel through the
fake review service — no fabricated UI.

  • ChangesEmpty — branch matches base: the panel's own EmptyState.
  • ChangesLoadFailed — review.read rejects: an error Banner with 重试.
  • ChangesSourceNotGit — a source that cannot be read (not a git repo) is a
    failure Banner, not an absence; the other read reasons share this branch.
  • ChangesTruncated — snapshot-level source truncation (truncated): a large
    changeset whose file list the source capped → the 变化过多 banner.
  • ChangesFileLineCap — the per-file 500-line boundedDiff cap on one long file
    (snapshot NOT truncated) → the "另有 N 行未显示" note. Separated from source
    truncation, which is an independent authority (review feedback).
  • ChangesEdgeContent — binary, rename, deletion, no-newline-at-EOF, and a very
    long minified line. The play expands each row and asserts its own diff body
    renders, scoped per file (review feedback).

DiffCodePreview is a pure unified-diff string renderer with no edge-state props;
the failure states live in this wrapper, which is where they are exercised here.

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).

changes-empty — normal / light · narrow / dark

changes-empty normal / light changes-empty narrow / dark

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

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

changes-truncated — normal / light · narrow / dark

changes-truncated normal / light changes-truncated narrow / dark

changes-file-line-cap — normal / light · narrow / dark

changes-file-line-cap normal / light changes-file-line-cap narrow / dark

changes-edge-content — normal / light · narrow / dark

changes-edge-content normal / light changes-edge-content 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 exercising the real Workbar → Changes → SessionReviewPanel path and for avoiding fabricated error UI.

I reviewed exact head 7c3649731bedd536370bb7a798ba3afd079ac7c2. The exact-head checks are green and I found no P0 or P1 issues. The PR body, however, contains no screenshots or recordings of the five new visual states. Please attach the empty and failure states plus expanded truncated and edge-content rows at normal and narrow Workbar widths, including light and dark themes.

I found two non-blocking P2 coverage issues:

  1. ChangesTruncated combines two independent authorities into a scenario production would not create. A single 620-line file triggers the panel-level 500-line boundedDiff, but it does not set the snapshot-level truncated flag; that flag comes from the source file-count or total-diff limits. Please separate per-file truncation from source snapshot truncation, or make the fixture satisfy the real source limit.
  2. ChangesEdgeContent claims binary, rename, deletion, no-newline-at-EOF, and a long minified line, but its play only expands and verifies the binary row. Please exercise the other four bodies, especially long-line overflow and the EOF marker, or narrow the coverage claim.

For manual visual review, please show that long paths, statistics, and chevrons remain visible at narrow width, and that all five edge rows use the existing Astryx and diff-renderer language without overflow.

These are test-modeling issues rather than production UI defects, so P2 is appropriate and they do not invalidate the overall direction.

Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head, real component path, truncation authorities, Storybook evidence, CI, and severity judgment, and owns this review.

中文对照

谢谢你通过真实的 Workbar → Changes → SessionReviewPanel 路径覆盖状态,也没有伪造错误 UI。

我审查了精确 head 7c3649731bedd536370bb7a798ba3afd079ac7c2。exact-head checks 已通过,没有 P0/P1。但 PR 正文没有五个视觉状态的截图或录屏。请补充空状态、失败状态,以及展开后的 truncated 和 edge-content 行,包括正常与窄 Workbar 宽度、浅色和深色主题。

有两个不阻塞的 P2 覆盖问题:

  1. ChangesTruncated 把两个独立 authority 组合成了生产不会生成的场景。单个 620 行文件只会触发 panel 的 500 行 boundedDiff;snapshot-level truncated 来自 source 文件数量或总 diff 上限。请拆开 per-file truncation 和 source snapshot truncation,或者让 fixture 真正满足 source limit。
  2. ChangesEdgeContent 声称覆盖 binary、rename、deletion、EOF 无换行和超长 minified line,但 play 实际只展开并验证 binary。请验证其余四种 body,尤其是长行溢出和 EOF marker;否则应收窄覆盖声明。

人工视觉验收还需要确认:窄宽度下长路径、统计数字和 chevron 保持可见,五种 edge row 都继续使用既有 Astryx 与 diff renderer 语言且没有溢出。

这些是测试建模问题,不是生产 UI 缺陷,因此定为 P2,不否定整体方向。

本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、真实组件路径、截断 authority、Storybook 证据、CI 和问题分级,并对本次 Review 负责。

@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/diff-viewer-failure-states branch from 7c36497 to 9d3aec8 Compare August 29, 2026 20:09
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Thanks for the review. Both P2s are addressed in the pushed revision (force-updated to 9d3aec8):

  1. Truncation authorities separated. ChangesTruncated now models only source-level snapshot truncation — a 24-file changeset with truncated: true → the 变化过多 banner. The per-file 500-line boundedDiff cap moved to a new ChangesFileLineCap story: one 620-line file with truncated: false, whose play asserts the 变化过多 banner is absent while the "另有 N 行未显示" note appears. The two are no longer combined into a scenario production wouldn't create.
  2. ChangesEdgeContent now exercises all five bodies. The play expands each row in turn and asserts its own diff body (scoped per file, since collapsed rows keep their diff mounted): the binary meta line, rename from, the deleted function body, the \ No newline at end of file marker, and the long minified line.

Smoke re-run: 202 stories green, per-story AX-tree audit clean.

On visual evidence: I'm working in a headless/CI environment and can't attach screenshots to the PR directly. The states render deterministically in build-storybook under stable ids (product-session-workbar--changes-empty / --changes-load-failed / --changes-source-not-git / --changes-truncated / --changes-file-line-cap / --changes-edge-content). Happy to capture and host PNGs somewhere if there's a preferred channel, or you can pull the branch and screenshot at normal/narrow widths in light/dark.

@liuxiaocs7
liuxiaocs7 requested a review from Astro-Han August 29, 2026 20:20
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Added a Visual evidence section to the PR description (screenshots on the fork's release assets): the empty state, the load-failure banner, source-level truncation, the per-file line cap (expanded), and the edge-content rows (expanded) — normal/light and narrow/dark.

Fourth surface under apache#3944 (one surface per PR): extend the 变更 panel
(SessionReviewPanel, via Product/Session Workbar) with the failure and edge
states that never show up in normal use. All drive the real panel through the
fake review service — no fabricated UI.

- ChangesEmpty — branch matches base: the panel's own EmptyState.
- ChangesLoadFailed — `review.read` rejects: an error Banner with 重试.
- ChangesSourceNotGit — a source that cannot be read (not a git repo) is a
  failure Banner, not an absence; the other read reasons share this branch.
- ChangesTruncated — snapshot-level source truncation (`truncated`): a large
  changeset whose file list the source capped → the 变化过多 banner.
- ChangesFileLineCap — the per-file 500-line `boundedDiff` cap on one long file
  (snapshot NOT truncated) → the "另有 N 行未显示" note. Separated from source
  truncation, which is an independent authority (review feedback).
- ChangesEdgeContent — binary, rename, deletion, no-newline-at-EOF, and a very
  long minified line. The play expands each row and asserts its own diff body
  renders, scoped per file (review feedback).

DiffCodePreview is a pure unified-diff string renderer with no edge-state props;
the failure states live in this wrapper, which is where they are exercised here.

Refs apache#3944, apache#3893


Generated-by: Claude Code
@liuxiaocs7
liuxiaocs7 force-pushed the liuxiaocs7/diff-viewer-failure-states branch from 9d3aec8 to 13a3e03 Compare August 29, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants