test(desktop): expand behavioral accessibility coverage - #4202
Conversation
17ce65d to
f7b00de
Compare
f7b00de to
18bd6bf
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for removing the unrelated Storage recovery commit and keeping the current head focused on the accessibility journey and WorkHub landmark semantics.
I reviewed exact head 18bd6bf5d846ee90aa5481b62c967579830cb6c9. The core direction is correct: AppShell already owns the application main, so the nested WorkHub surface should be a named region rather than a second main. The new Electron journey also uses real transcript, tool-result, task-ledger, streaming, and Workbar paths. The exact-head test check is green.
The PR body does not contain before/after screenshots, an accessibility-tree comparison, or a keyboard recording. Please attach evidence showing one application main plus the named WorkHub region, and a complete keyboard path through skip link, tool result, model picker, composer/Send, Graph, and streaming Stop.
I found one non-blocking P2 test-contract gap. Several controls are reached with direct Playwright .focus(), including recent tasks, the model menu item, Graph collapse, and Stop. Direct focus proves that a mounted element can receive focus, but not that a keyboard user can reach it through the real Tab order. The Issue acceptance also names reaching the Send button, while the test fills the composer and presses Enter without navigating to or activating Send.
Please start from a deterministic focus boundary and use Tab or Shift+Tab to reach the controls whose keyboard accessibility the test claims. Add one explicit keyboard activation of Send. If composer Enter is intentionally the only sending contract, state and assert that narrower contract instead.
For manual UX review, please also confirm that changing WorkHub from nested main to named region does not alter visual layout, focus restoration, or screen-reader landmark order across navigation away and back.
There are no P0 or P1 findings on the current head.
Review analysis was assisted by Codex and an independent @reviewer agent. Astro-Han verified the exact head after the force-push, production landmark owner, Electron fixture path, keyboard assertions, CI, and severity judgment, and owns this review.
中文对照
谢谢你删除了无关的 Storage recovery commit,让当前 head 收敛在 accessibility journey 和 WorkHub landmark 语义上。
我审查了精确 head 18bd6bf5d846ee90aa5481b62c967579830cb6c9。核心方向正确:AppShell 已经拥有应用级 main,内层 WorkHub 应是命名 region,而不是第二个 main。新的 Electron journey 也使用真实 transcript、tool result、task ledger、streaming 和 Workbar 路径。exact-head test 已通过。
PR 正文没有前后截图、accessibility tree 对比或键盘操作录屏。请补充证据,展示应用只有一个 main、WorkHub 是命名 region,并展示 skip link、tool result、model picker、composer/Send、Graph 和 streaming Stop 的完整键盘路径。
有一个不阻塞的 P2 测试契约缺口。recent tasks、model menu item、Graph collapse 和 Stop 等控件通过 Playwright .focus() 直接获得焦点。直接 focus 只能证明元素挂载后可以接收焦点,不能证明键盘用户能通过真实 Tab 顺序到达。Issue acceptance 还明确提到 Send button,但测试只是填充 composer 并按 Enter,没有导航到或激活 Send。
请从确定的焦点边界开始,用 Tab 或 Shift+Tab 到达测试声称可键盘访问的控件,并补一次 Send 的明确键盘激活。如果 composer Enter 才是刻意定义的唯一发送契约,也应明确说明并断言这个更窄的契约。
人工 UX 验收还需要确认:WorkHub 从嵌套 main 改为命名 region 后,不会改变视觉布局、焦点恢复或离开再返回时的屏幕阅读器 landmark 顺序。
当前 head 没有 P0/P1。
本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了 force-push 后的精确 head、生产 landmark owner、Electron fixture 路径、键盘断言、CI 和问题分级,并对本次 Review 负责。
Summary
Validation
npm run lintnpm run format:checknpm run astryx:surface-inventorynpx knip --workspace apps/desktopnpm --workspace @maka/desktop run build:with-depsnpm --workspace @maka/desktop run typecheckGIT_CONFIG_GLOBAL=/dev/null npm --workspace @maka/desktop run test:distnpx playwright test --config e2e/playwright.config.ts e2e/accessibility-coverage.spec.ts e2e/workhub-layout.spec.ts e2e/workhub-reconstruction.spec.ts(10 passed on the refreshedmainbase)npm --workspace @maka/desktop run build-storybooknpm --workspace @maka/desktop run smoke:storybookCloses #3893