Skip to content

test(desktop): story-cover the model picker's failure and edge states - #4162

Open
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:liuxiaocs7/model-picker-failure-states
Open

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

Conversation

@liuxiaocs7

Copy link
Copy Markdown
Member

Summary

Second surface under #3944 (one surface per PR): extend Product/Model Picker with the failure and edge states that never show up in normal use, so the Storybook gate and the #3893 a11y audit have real targets. Every story renders a production component with real props — no fabricated error UI.

  • CatalogLoadingModelPicker loading, the catalog request still in flight. Distinct from EmptyCatalog (resolved, no providers): the trigger wears Astryx's busy affordance, not a disabled husk.
  • NoModelsAvailable — the composer's ModelChipStatic "configure a connection" button. A failed / offline / unauthorised catalog fetch has no distinct UI on this surface; all three collapse to an empty catalog, so one honest "no models" state stands in for them rather than inventing three error screens.
  • ManyConnections — seven connection groups (two OpenAI keys exercise the slug-suffix heading disambiguation), the breadth refactor(design-system): finish wiring the Maka theme to Astryx's token seams #3446 F5 says a single reference screen never probes; a model in the last group stays reachable via play.
  • LongModelNames — a long connection name, model labels, and option descriptions at once (very long text).
  • StaleCurrentModel — an existing session pinned to a since-removed connection, exercising ChatModelSwitcher's leading-row branch for a current model the catalog no longer lists.

Menu/popup states assert in play (open, then query the document.body portal) so the AX audit covers the open menu; loading and no-models are mount-visible.

Refs #3944, #3893, #3446

Verification

  • apps/desktoptypecheck:stories + @maka/uitypecheck pass; Biome lint and git diff --check clean.
  • Storybook build + render smoke: 201 stories, all green, no runtime/console/page errors, per-story AX-tree audit clean. The five new ids: product-model-picker--catalog-loading, --no-models-available, --many-connections, --long-model-names, --stale-current-model.
  • play runs under the smoke (it waits for storyFinished): the many-connections menu exposes ≥7 labelled groups with a last-group model reachable; the stale-current menu shows the leading unknown-current row above the real catalog.

Second surface under apache#3944 (one surface per PR): extend
`Product/Model Picker` with the failure and edge states that never show
up in normal use, so the Storybook gate and the apache#3893 a11y audit have
real targets. Every story renders a production component with real
props — no fabricated error UI.
- CatalogLoading — `ModelPicker loading`, the catalog request still in
flight. Distinct from EmptyCatalog (resolved, no providers): the
trigger wears Astryx's busy affordance, not a disabled husk.
- NoModelsAvailable — the composer's `ModelChipStatic` "configure a
connection" button. A failed / offline / unauthorised catalog fetch
has no distinct UI on this surface; all three collapse to an empty
catalog, so one honest "no models" state stands in for them rather
than inventing three error screens.
- ManyConnections — seven connection groups (two OpenAI keys exercise
the slug-suffix heading disambiguation), the breadth apache#3446 F5 says a
single reference screen never probes; a model in the last group stays
reachable.
- LongModelNames — a long connection name, model labels, and option
descriptions at once (very long text).
- StaleCurrentModel — an existing session pinned to a since-removed
connection, exercising ChatModelSwitcher's leading-row branch for a
current model the catalog no longer lists.
Menu/popup states assert in `play` (open, then query the document.body
portal) so the AX audit covers the open menu; loading and no-models are
mount-visible.
Refs apache#3944, apache#3893, apache#3446

@Astro-HanAstro-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 extending the real model-picker components rather than inventing separate edge-state UI.

I reviewed exact head 4069ca84c70f260f1e277879e61ccedddb4e3ff2. The exact-head checks are green, but the PR body does not include screenshots or recordings of the five new states. Because these stories are intended to become visual review targets, please attach at least the open-menu states for many connections, long names, and stale current model at normal and narrow widths, preferably in both light and dark themes.

I found two P2 story-modeling issues:

  1. CatalogLoading is not the production state described by the story. Settings renders a skeleton while the catalog is unavailable; ModelPicker.loading is used for saving the default model. Please remove this story or model the real saving state with non-empty groups.
  2. StaleCurrentModel bypasses the current recovery path. Missing-connection recovery sets hideUnavailableCurrentModel, while the story calls ChatModelSwitcher directly and supplies a label production does not generate. A real leading-row case would keep the Connection present while removing only the model, or the story should exercise the full recovery state and verify that the stale row is hidden.

A smaller P3 evidence gap remains in the many/long stories: their plays only find menu items and do not prove that the last group is scrollable, focusable, selectable, or that long text stays within the menu. Please strengthen the play assertions or narrow the stated coverage.

There are no P0 or P1 findings, but I would address the two P2s and add visual evidence before approval.

Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head, current production picker paths, story composition, CI, and severity judgment, and owns this review.

中文对照

谢谢你复用真实 Model Picker 组件来补边界状态,没有另造一套 UI。

我审查了精确 head 4069ca84c70f260f1e277879e61ccedddb4e3ff2,exact-head checks 已通过。但 PR 正文没有提供五个新状态的截图或录屏。既然这些 Story 会成为视觉审查目标,请至少补充多连接、长名称和 stale current model 的打开菜单效果,包括正常宽度、窄宽度,最好同时覆盖浅色和深色主题。

有两个 P2 Story 建模问题:

  1. CatalogLoading 不是 Story 所描述的生产状态。catalog 不可用时 Settings 渲染 skeleton;ModelPicker.loading 实际用于保存默认模型。请删除该 Story,或用非空 groups 表达真实 saving 状态。
  2. StaleCurrentModel 绕过了当前恢复路径。连接缺失时生产代码会设置 hideUnavailableCurrentModel;当前 Story 却直接调用 ChatModelSwitcher,并传入生产不会生成的 label。真实 leading-row 场景可以保留 Connection、只删除 model;或者接入完整恢复状态,验证 stale row 被隐藏。

另有一个较小的 P3 证据缺口:Many/Long 的 play 只找到 menu item,没有证明最后一组可以滚动、聚焦和选择,也没有证明长文本不会撑宽菜单。请加强 play 断言,或者收窄覆盖声明。

没有 P0/P1,但建议先修复两个 P2 并补视觉证据,再考虑 Approve。

本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、当前生产 picker 路径、Story composition、CI 和问题分级,并对本次 Review 负责。

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/MUnder 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@liuxiaocs7@Astro-Han