fix(desktop): expand Computer Use semantic UI coverage - #2640
Conversation
867d893 to
5cf74d5
Compare
c0547c1 to
9d35265
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWhat problem this solvesThis PR completes desktop Computer Use accessibility coverage. It adds stable landmarks, localized accessible names, correct ARIA state, modal focus ownership, and hidden-surface focus exclusion. It covers settings, modules, conversations, dialogs, overlays, workbar panels, generated content, runtime surfaces, and browser interactions. Source of truthThe PR extends the existing source of truth. Renderer components provide native HTML, ARIA semantics, and localized copy. Storybook and Electron tests inspect the resulting Chromium accessibility tree. The PR does not add a runtime DOM walker, observer, repair loop, OCR, model loading, or runtime dependency. Solution size and complexityThe changes form the smallest coherent solution shown by the supplied diff. Production code adds only the semantics and copy required for accessible states. The AX-tree auditor, Storybook state coverage, Electron E2E coverage, browser smoke test, and CI change detection validate both source-defined semantics and rendered behavior. Simplification opportunitiesNo safe deletion or simplification is evident. Removing the AX-tree auditor or the new Storybook and Electron states would weaken regression coverage. The added copy properties support required landmark, conversation, control, and scroll-action labels. Complexity delta
Test and copy-contract maintenance increases. This increase is necessary for the stated coverage. Total maintenance complexity remains justified. Risks and validationThe PR changes user-visible accessibility behavior through landmark structure, region names, control names, focus behavior, and localized text. Material changes in these areas require independent human review under repository policy. The PR changes the The Storybook smoke flow changes viewport selection and validates the full CDP accessibility tree. The Electron lane adds browser The objectives report passing unit, typecheck, Storybook, Playwright, browser smoke, formatting, and AX audit checks. Native macOS AX and Windows UIA packaged-app checks remain release gates. Required-check status is unverified here because direct check output was not provided. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughChangesThe PR adds native accessibility regions and localized labels across renderer pages. It adds Storybook and Playwright coverage for overlays, settings, conversations, workbar panels, runtime surfaces, and narrow layouts. It also adds accessibility-tree validation to Storybook and Electron smoke checks. Accessibility semantics and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The accessibility expansion adds semantic landmarks, labels, and automated audits, but the current head can leave browser validation hanging when a read stalls, while narrow-viewport coverage and ambiguous or overly revealing accessible names remain unresolved. Merge should wait for these bounded issues to be fixed or explicitly accepted by the owners. Sequence Diagram(s)sequenceDiagram
participant Storybook
participant Chromium
participant AccessibilityTreeAudit
participant ElectronE2E
Storybook->>Chromium: Render stories and complete play steps
Chromium->>AccessibilityTreeAudit: Provide full accessibility tree
AccessibilityTreeAudit-->>Storybook: Return audit findings
ElectronE2E->>Chromium: Navigate desktop accessibility surfaces
Chromium->>AccessibilityTreeAudit: Provide runtime accessibility tree
AccessibilityTreeAudit-->>ElectronE2E: Return audit findings
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9d35265 to
2711e16
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/module-pages.tsx (1)
25-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse one source for the Skills region label.
When
hubHeaderis absent,SkillsPagepassescopy.skillsto this fallback at Line [69]. The loadedSkillsModuleMainusesgetSkillsCopy(locale).page.titleatpackages/ui/src/skills-panel.tsxLine [575]. If these fields differ, the region name changes during lazy loading.Compute one label and pass it to both states, or verify that the two fields are contractually identical and keep one source of truth.
As per path instructions, keep the loading and loaded semantic label at one existing source of truth instead of maintaining parallel copy fields.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25638710-cdac-4a05-b9d5-66dc7579209f
📒 Files selected for processing (13)
apps/desktop/e2e/accessibility-coverage.spec.tsapps/desktop/src/renderer/mcp-page.tsxapps/desktop/stories/accessibility-overlays.stories.tsxapps/desktop/stories/module-hubs.stories.tsxdocs/computer-use-foundation-contract.mdpackages/ui/src/astryx-copy.tspackages/ui/src/astryx-i18n.tsxpackages/ui/src/chat-view.tsxpackages/ui/src/conversation-copy.tspackages/ui/src/module-pages.tsxpackages/ui/src/skills-panel.tsxpackages/ui/stories/accessibility-dialogs.stories.tsxscripts/storybook-visual-smoke.mjs
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Pull request overview
This PR strengthens Desktop “Computer Use” accessibility coverage by aligning renderer semantics (single main landmark with named sub-regions), localizing Astryx app-shell accessibility strings, and adding/expanding automated AX-tree audits in both Electron e2e and Storybook smoke.
Changes:
- Replace nested module/chat
<main>landmarks with named regions (<section aria-label=...>/role="region") to enforce “one main per window”. - Extend Storybook smoke to run
playfunctions, run narrow stories at a narrow viewport, and CDP-scan the full AX tree for unnamed actionable nodes + multiplemainlandmarks. - Add new Storybook fixtures and a new Electron e2e spec to validate actionable AX node naming and localized skip-link behavior across key surfaces.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/storybook-visual-smoke.mjs | Runs Storybook plays, sets narrow viewport for *narrow* stories, and audits the Chromium AX tree via CDP. |
| packages/ui/stories/accessibility-dialogs.stories.tsx | Adds “final state” dialog stories (rename, scheduled task create, Mermaid fullscreen) for AX coverage. |
| packages/ui/src/skills-panel.tsx | Changes nested main to a named region to avoid multiple main landmarks. |
| packages/ui/src/module-pages.tsx | Updates module wrappers/fallbacks from main to named regions to keep a single app-shell main. |
| packages/ui/src/conversation-copy.ts | Adds localized ARIA labels for new-task surface region and per-conversation region naming. |
| packages/ui/src/chat-view.tsx | Converts chat surface wrapper from main to named regions for proper landmark structure. |
| packages/ui/src/astryx-i18n.tsx | Adds Astryx app-shell a11y string overrides (skip link + mobile navigation) for zh locale. |
| packages/ui/src/astryx-copy.ts | Extends Astryx zh copy with app-shell strings used by the override map. |
| docs/computer-use-foundation-contract.md | Documents the renderer semantic source contract and the 3-layer validation approach. |
| apps/desktop/stories/module-hubs.stories.tsx | Adds a narrow Scheduled Tasks story to exercise below-breakpoint semantics. |
| apps/desktop/stories/accessibility-overlays.stories.tsx | Adds overlay stories (side-chat close, onboarding QR, WeChat QR) for AX-tree coverage. |
| apps/desktop/src/renderer/mcp-page.tsx | Changes nested main to a named region to avoid multiple main landmarks. |
| apps/desktop/e2e/accessibility-coverage.spec.ts | Adds Electron AX-tree coverage asserting named actionable controls across settings, modules, overlays, composer, and workbar. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d0cf0b7 to
1040c4f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/storybook-visual-smoke.mjs:26
ACTIONABLE_AX_ROLESis duplicated here and inapps/desktop/e2e/accessibility-coverage.spec.ts. Because this list defines what the AX audits consider “actionable”, keeping two copies risks drift (e.g., one audit flagging nodes the other ignores). Consider extracting the role list + small helpers (axString) into a shared utility (or exporting from one place) so Storybook smoke and Electron e2e stay consistent.
const ACTIONABLE_AX_ROLES = new Set([
'button',
'checkbox',
'combobox',
'link',
'menuitem',
'menuitemcheckbox',
'menuitemradio',
'option',
'radio',
'searchbox',
'slider',
'spinbutton',
'switch',
'tab',
'textbox',
'treeitem',
]);
1040c4f to
74226a5
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
74226a5 to
536dc88
Compare
|
Follow-up verification for Addressed the Memory P2 and the deeper review findings:
Local verification on the final rebased head:
Memory actionBefore (accessible identity leaked into visible copy): 
After (visible copy remains stable; identity stays AX-only): 
Localized skip linkBefore (base branch default English copy): 
After (Chinese app-shell copy): 
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The PR substantially improves semantic coverage and, importantly, keeps the production cost to native HTML/ARIA while centralizing the test-only audit. I found one false-negative in that shared gate and one non-blocking naming tradeoff. The first-principles fix is small: define an empty accessibility tree by exposed content, not by the presence of ignored CDP records; for repeated actions, prefer stable human-readable context over raw storage identities.
AI-assisted review disclosure: Codex verified these observations against the current head, the shared AX audit, Storybook and Electron E2E call sites, and localized action-name producers. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs; broader test-scope claims without a concrete failure were discarded. No local tests were run.
中文复核
这个 PR 明显补强了语义覆盖，而且生产代码只使用原生 HTML/ARIA，把复杂度集中在测试审计层，方向很好。当前保留一个审计假阴性和一个不阻塞的命名取舍：空 AX tree 应按“没有 exposed 内容”判定，而不是只看 CDP 是否返回 ignored 记录；重复操作名称应优先使用稳定、可理解的上下文，而不是原始内部 ID。
本次为 AI 辅助审查：Codex 在最新 head 上核验共享 AX audit、Storybook/Electron E2E 调用点及本地化 action-name producer；另使用两次独立 reviewer 与一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查，没有具体失败证据的宽泛测试范围指控已剔除。未运行本地测试。
|
/agentic_review |
Code Review by Qodo
1. Overlay audit races dismissal
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the accessibility pass and the before/after evidence. I re-reviewed exact head 95439ebe1bf2653ad2cad78a641bb1e14d793ec7; the AI provenance and UI screenshot gates are satisfied, but the three existing unresolved threads remain current and actionable, so I am keeping this at COMMENT.
The two approval blockers are the existing P2 threads:
- The AX-tree audit checks the raw source-node count, so an all-ignored tree reports zero exposed nodes but still passes. Please base the empty-tree failure on
exposedNodes(sourceNodes).lengthand add an all-ignored fixture. - The E2E uses a generic dialog locator and does not wait for Keyboard Shortcuts to disappear before asserting the Command Palette. Please wait for the named Keyboard Shortcuts dialog to be hidden, then assert the named Command Palette dialog.
The existing P3 about UUID-like internal IDs in spoken copy/edit/answer labels is also still valid; a stable ordinal plus short human context would be preferable, with the internal ID retained only as data.
I did not duplicate any inline finding. The cancelled test_workspaces job also needs a green rerun before merge readiness.
AI-assisted review disclosure: OpenAI Codex performed the exact-head code, thread, screenshot, provenance, and CI analysis; I verified the reproduced AX-tree behavior, severity, deduplication, and live state before posting.
中文说明
当前三个未解决线程都仍有效。两个 P2 分别是：全 ignored 的 AX tree 会被错误放过；E2E 没等“键盘快捷键”对话框消失就用通用 dialog 断言“命令面板”。另一个 P3 是读屏标签暴露内部 UUID。截图和 AI provenance 已合规，但两个 P2 与 CI 重跑完成前不能批准。
95439eb to
676320c
Compare
1f49117 to
98f0f73
Compare
|
Final follow-up on head
Verification of the product diff: UI tests 197/197; Desktop workspace tests 975/975; AX audit contract 11/11; CI planner 18/18; focused Electron accessibility 3/3; the previously failing prompt-rail E2E passes; browser observe→act→effect 17/17; Storybook render/AX smoke 149/149; lint, format, typecheck, and builds pass. The full core CI and dependency audit passed on the immediately preceding product-identical head; the current head only adds the latest upstream test fixes and has restarted those checks. The separate Windows release lane remains affected by the repository-wide packaged-renderer CDP port race tracked and fixed in #3241: across three runs the same package passed upgrade verification on one attempt and failed CDP discovery at different packaged-renderer checkpoints on others, including logs where Chromium announced No production polling, global listener, dependency, or asset-loading path was added. |
89a0788 to
8c7c747
Compare
There was a problem hiding this comment.
Posted in error — please disregard this review entirely. Its body and inline comments belong to #3255 and were sent here by a scripting mistake on my side. Nothing in it applies to this PR. Sorry for the noise; the actual review of this PR follows separately. The inline comments below have been rewritten to the same retraction.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 8c7c7477. (The head moved while I was reviewing; I confirmed every file this review touches is byte-identical to 89a0788e, so the force-push was a rebase onto main and the findings carry over unchanged.)
This earns the landmark and dialog-naming layer convincingly — I enumerated every <main> and role="main" in the tree at this head and the "one primary main per window" claim holds, including the mutual exclusion between the Astryx shell landmark and the settings surface (the shell goes aria-hidden/inert when obscured). The ChatSurfaceLayout and ToolOutputSurface call sites are complete. CSS is unaffected: every main reference is a class selector, none an element selector.
What it does not earn is the layer its title claims — "every actionable element has an unambiguous name" — and the new audit gate cannot demonstrate that it does. scripts/ax-tree-audit.mjs scopes its ambiguity check relatively: namedScope walks up three levels for a named ancestor and otherwise falls back to the first few text nodes of the subtree. Two buttons with byte-identical names pass as long as the surrounding prose differs. That is the mechanism by which the code-block copy buttons (inline) survive a fully green audit, and it is worth knowing before this gate is cited as evidence of coverage in future reviews.
The architectural note is about the copy seam. actionsAriaLabel and answerActionsAriaLabel changed from constants to functions — a signature change on a public copy contract. That is a fine change, but it silently breaks every exact-name selector downstream, and it has already broken one: session-list-panel.stories.tsx:139 still queries [aria-label="任务操作"] exactly, now matches nothing, and fails open through ?.. The PR body's non-additive-changes checklist says "described under Summary above", but this signature change, the Button label/children switch in MemoryEntryList, the story-id viewport heuristic in storybook-visual-smoke.mjs:123, and the weakening of chat-turn-steering-order.test.ts from markup counting to textContent counting are not individually called out. The copy-contract signature change in particular belongs in the body.
Two contract observations rather than findings: removing embed=true from the Storybook smoke means play functions now execute across the whole catalogue in CI — stated in the body, and the largest behavioural change here; and e2e/accessibility-coverage.spec.ts imports ../../../scripts/ax-tree-audit.mjs across a workspace boundary that apps/desktop/package.json does not declare, compensated for in ci-test-plan's E2E_DRIVING_SCRIPTS. Both look deliberate.
Reviewed with Claude Opus as an analysis assistant. Every claim above and below was verified against the source at this head — including the Astryx dist output for the CodeBlock and Button naming behaviour — except where a finding states otherwise. No test run was performed.
Apologies for the retracted review above: an unrelated PR's comments were posted here by a scripting mistake on my side and have been rewritten as retractions.
Generated-by: Codex
Generated-by: Codex
8c7c747 to
22af3b7
Compare
hqhq1025
changed the title
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 22af3b72. All eight findings from my previous round are fixed, most of them more thoroughly than I asked. One new P3, inline.
- P1, per-turn message ordinal — fixed by deletion, which was the better of the two options I offered.
messageNumberis gone fromUserMessageBody,MessageCopyButton,ConversationSegment,splitTimelineAtUserMessages, and frommessageActionAriaLabel's signature in both locales. Excerpt plus timestamp carries the disambiguation on its own, and no false position is asserted any more. - P2, code-block copy buttons all named alike — fixed, and extended past what I flagged.
outputActionIdentitynow reachesToolCodeBlockon all four branches andLoadToolResultPreview, which I had missed. Implementing it as scoped@astryx.codeBlock.*locale overrides rather than a new prop on the Astryx component is the right seam. - P2, silently broken story selector — fixed properly.
[aria-label$="任务操作"], plus the fixture now throws when the row or the menu button is missing instead of no-opping through?., plus a newActiveTaskActionsOpenstory whoseplaywaits for the menu and asserts amenuitemby name. The opened-menu nodes are now genuinely audited rather than counted as covered. - P2, "complete" vs. what the gate measures — resolved by narrowing the claim, which is the honest fix. The document is now "Computer Use Semantic UI Coverage", scopes "covered" to Computer Use operability, and states outright that live-region and announcement completeness are not measured and need separate accessibility testing. The PR title was narrowed to match.
- P3, memory entry ordinal — fixed.
memoryEntryActionIdentitycomposes title, origin label, and a stable timestamp (falling back to a content excerpt), so the name no longer moves when the list is filtered — and the 80-character truncation now carries an ellipsis. - P3, composed names bypassing the copy seam — fixed at both sites.
copy.actionAriaLabelandentryActionArialive in the copy tables with per-locale punctuation (：vs:), isomorphic tomessageActionAriaLabel. - P3, unreachable
props.kindfallback — removed. Still safe: the copy button only renders whencommandis non-empty, soactionIdentitycannot be empty. - P3, unexplained
getComputedStylestub — answered rather than deleted, which is the right outcome given the answer. The comment names Astryx 0.4's Spinner resolving its inherited canvas colour during render, so the stub is a real requirement and no longer reads as rebase residue.
The capability-audit-strip story that escaped the landmark sweep was also converted to <section> with a label, so the story tree's landmark shape matches production again.
I checked the one thing this approach could plausibly have broken: AstryxLocaleProvider merges {...base, ...scopedOverrides} rather than replacing, so wrapping a CodeBlock does not strip the rest of the Astryx locale bundle. What it does do is discard an ancestor's scoped overrides — see the inline note. Hook order is fine: useUiLocale and useMemo both run before the if (!props.code) return null early return.
Reviewed with Claude Opus as an analysis assistant. Every disposition above was verified by reading source at this head and diffing against the head I previously reviewed, including astryx-i18n.tsx's merge semantics; no tests or stories were executed.
Generated-by: Codex
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at e7ba0cc9. The P3 from my previous round is fixed correctly. No remaining findings.
AstryxLocaleProvider now reads the ambient InternationalizationContext and merges base → inherited → scoped, so a nested provider is additive instead of rebuilding from the locale defaults. Two details make it a better fix than the minimum:
- Spreading
ambient.overridesinto the returned map preserves the outer provider's entries for other locales, not just the active one. - Forwarding
messagesanddirwhenambient.locale === localemeans the inner provider no longer resets direction and catalog while inheriting overrides — which the narrow fix would have left broken.
I checked the case this could have regressed: with no provider above, useContext yields Astryx's default { locale: 'en', direction: 'ltr', messages: {} }, so ambient.locale cannot throw, and passing that same messages: {} through is exactly the shape resolve() already falls through from to the shipped English catalog. Isolated renders — markdown-body.test.ts mounts AstryxLocaleProvider directly — are unaffected.
The added test asserts the real property: an outer and an inner override both resolve in the inner subtree. That is the assertion that would have caught the original defect.
Reviewed with Claude Opus as an analysis assistant; verified by reading the change, the new test, and the Astryx dist context default at this head. No tests were executed.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at e7ba0cc9. All eight findings from my first round were fixed, and the follow-up P3 about nested Astryx locale providers is fixed correctly too — the provider now inherits the ambient overrides and forwards messages/dir, with a test asserting that an outer and an inner override both resolve. Detail in the two previous reviews; nothing outstanding from my side.
Summary
mainlandmark per Electron window and stable named regions for the measured conversation, module, settings, dialog, workbar, generated-content, and runtime surfacesmainlandmarks, unnamed dialogs, and missing checked/selected/expanded/value stateWebContentsViewobserve -> semantic ref -> fill/click -> effect smoke in CI, while reusing the existing E2E build to avoid duplicate workspace compilationScope
This PR improves Computer Use semantic operability: target identity, state, focus, action discovery, and effect verification across the measured source-defined and runtime states.
It does not claim complete WCAG conformance or complete assistive-technology behavior. Live-region and announcement completeness are not measured by this gate. Native macOS AX and Windows UIA packaged-app spot checks remain release gates, and third-party page markup, Chromium's PDF plugin, native file dialogs, and OS permission dialogs remain platform/provider owned.
Review-fix delta
ToolCodeBlock.actionIdentitycontract and scoped Astryx CodeBlock copy labels to each owning tool call, including connector fallback outputAstryxLocaleProviderscopes additive by composing ambient messages, direction, and overrides before applying the inner scopemaingetComputedStyleshim: Astryx 0.4 Spinner reads inherited canvas colorThese are deliberate contract changes rather than additive compatibility layers: the message copy function signature loses the incorrect ordinal argument, Button keeps a full accessible
labelwhile children preserve short visible copy, and Storybook uses fixture identity only in test code.Verification
Current rebased head (
e7ba0cc90, based onmain@de091d638):node --test scripts/ax-tree-audit.test.mjs(11 passed)npm --workspace @maka/ui test(187 passed, including nested provider composition)npm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build-storybooknpm --workspace @maka/desktop run smoke:storybook(149 stories, including play functions and AX audits)node --test apps/desktop/dist/main/__tests__/import-tasks-settings-page.test.js(15 passed)git diff --checkThe branch's broader Electron accessibility, Browser semantic-action, and packaging checks remain in CI.
AI use
Select exactly one:
Tool(s) and scope: Codex audited the accessibility surfaces, implemented semantic and test changes, resolved review findings, rebased the branch, updated documentation, and ran verification. The commits include the required
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?