feat(ai): make benchmarking optional - #163
Conversation
📝 WalkthroughWalkthroughAI setup now treats benchmarking as optional. Model download, activation, and benchmarking are separate actions. Unbenchmarked models use fallback cadence and timeout values. AI enablement validates readiness and shows a confirmation dialog when benchmark data is missing. ChangesAI setup and benchmarking flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionView
participant AiCategory
participant ModelPickerContainer
participant useSidecarStore
SessionView->>AiCategory: Open AI settings
AiCategory->>ModelPickerContainer: Check active model and setup state
AiCategory->>AiCategory: Show warning when benchmark data is missing
AiCategory->>useSidecarStore: Enable live AI after confirmation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements an “optional benchmark” AI setup flow: model download/selection can happen without benchmarking, and enabling AI now prompts with an explicit warning when the selected model is unbenchmarked (allowing benchmark-first or proceeding with safe defaults).
Changes:
- Decouple model download, selection, and benchmarking; introduce readiness gating for AI enablement (including “unbenchmarked” warning state).
- Update Settings → AI, Model Picker UI, onboarding tutorial copy, and in-session AI status affordance to better guide when/why to benchmark.
- Extend unit coverage and docs to reflect the optional benchmark path and fallback runtime behavior.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/ai-sidecar.test.ts | Adds coverage for benchmark-vs-live sidecar start gating behavior. |
| tests/unit/ai-sample-loop.test.ts | Updates I83 commentary to reflect optional benchmarking / active model selection changes. |
| tests/unit/ai-models.test.ts | Expands tests for selecting models without benchmarks and preserving records across switches/forget. |
| tests/unit/ai-benchmark-gate.test.ts | New unit tests for AI enable readiness mapping (loading/error/no-model/unbenchmarked/ready). |
| src/strings.ts | Adds/updates copy for optional benchmarking flow, enablement warning dialog, and picker CTAs/status. |
| src/stories/ModelPicker.stories.tsx | Updates story harness to new picker actions and adds active/unbenchmarked story states. |
| src/features/settings/categories/AiCategory.tsx | Keeps model setup available while AI is off; adds pre-enable unbenchmarked warning dialog and busy gating. |
| src/features/session/SessionView.tsx | Makes in-session AI status chip open AI settings directly (when handler present). |
| src/features/onboarding/TutorialStep.tsx | Adds an “AI is optional” tutorial card and adjusts the grid layout. |
| src/features/ai/sidecar.ts | Adds start-purpose concept and blocks benchmarking while AI is enabled (new error sentinel). |
| src/features/ai/sampleLoop.ts | Clarifies fallback cadence semantics when no benchmark exists. |
| src/features/ai/modelStore.ts | Introduces explicit selectModel and stops recordBenchmark from implicitly changing active selection. |
| src/features/ai/ModelPickerContainer.tsx | Refactors orchestration: download/select/benchmark separated; adds busy callback + imperative benchmarkSelected handle. |
| src/features/ai/ModelPicker.tsx | Updates UI to show separate Download / Use model / Benchmark actions, active-pill, and unbenchmarked messaging. |
| src/features/ai/index.ts | Re-exports new readiness helpers and picker container handle types; exports new sidecar error/purpose type. |
| src/features/ai/benchmarkGate.ts | New helper for mapping model-store state into AI enable readiness states. |
| src/features/ai/benchmark.ts | Starts sidecar with explicit purpose: 'benchmark' to enforce benchmark-vs-live separation. |
| scripts/generate-benchmark-image.ts | Updates script header comment to reflect optional benchmark framing. |
| README.md | Updates user-facing AI explanation to reflect optional benchmarking and fallback defaults. |
| PLAN.md | Updates V2 AI description to document optional benchmarks and explicit enablement warning behavior. |
| ARCHITECTURE.md | Updates AI pipeline description to document optional benchmarking and fallback request-timeout behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const aiEnabled = activeRuntime.getAiFeaturesEnabled() | ||
| if (purpose === 'benchmark' && aiEnabled) { | ||
| set({ lastError: ERR_BENCHMARK_REQUIRES_AI_OFF, status: 'idle' }) | ||
| return null | ||
| } | ||
| if (purpose !== 'benchmark' && !aiEnabled) { | ||
| set({ lastError: ERR_AI_DISABLED, status: 'idle' }) | ||
| return null | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/ai/ModelPickerContainer.tsx`:
- Around line 442-465: Wrap the post-download auto-selection in runDownload with
error handling around selectModel, using the existing onActivate pattern as
guidance. Ensure a selectModel persistence failure is caught and does not
prevent the subsequent updateCard idle reset, success toast, or cleanup that
clears setupBusy, while preserving the download-success flow and selected-state
behavior.
In `@src/features/session/SessionView.tsx`:
- Around line 1945-1958: Update the settings-opening button in the
onOpenSettings branch of SessionView to use focus-visible:outline-hidden instead
of outline-none alongside its existing focus ring, and add
aria-haspopup="dialog" to identify the dialog it opens. Leave the non-button
AiStatusChip rendering unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71b8d2ef-f81c-4836-a52f-eae302f68a3a
📒 Files selected for processing (21)
ARCHITECTURE.mdPLAN.mdREADME.mdscripts/generate-benchmark-image.tssrc/features/ai/ModelPicker.tsxsrc/features/ai/ModelPickerContainer.tsxsrc/features/ai/benchmark.tssrc/features/ai/benchmarkGate.tssrc/features/ai/index.tssrc/features/ai/modelStore.tssrc/features/ai/sampleLoop.tssrc/features/ai/sidecar.tssrc/features/onboarding/TutorialStep.tsxsrc/features/session/SessionView.tsxsrc/features/settings/categories/AiCategory.tsxsrc/stories/ModelPicker.stories.tsxsrc/strings.tstests/unit/ai-benchmark-gate.test.tstests/unit/ai-models.test.tstests/unit/ai-sample-loop.test.tstests/unit/ai-sidecar.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Installer (Windows)
- GitHub Check: Installer (macOS)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Rust (macOS)
- GitHub Check: Rust (Linux)
- GitHub Check: Rust (Windows)
- GitHub Check: Frontend
- GitHub Check: Analyze (rust)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,rs}: The application must remain local-only: never add telemetry and never instruct users to paste model files or BIP39 mnemonics into an AI service.
Do not add comments unless the reason is non-obvious; identifiers should carry meaning and code should read top-to-bottom.
Files:
tests/unit/ai-benchmark-gate.test.tsscripts/generate-benchmark-image.tssrc/features/ai/benchmarkGate.tssrc/features/onboarding/TutorialStep.tsxtests/unit/ai-sample-loop.test.tssrc/features/ai/benchmark.tssrc/features/session/SessionView.tsxsrc/features/ai/sampleLoop.tstests/unit/ai-models.test.tssrc/features/ai/sidecar.tstests/unit/ai-sidecar.test.tssrc/features/ai/modelStore.tssrc/stories/ModelPicker.stories.tsxsrc/features/ai/index.tssrc/features/ai/ModelPickerContainer.tsxsrc/features/settings/categories/AiCategory.tsxsrc/features/ai/ModelPicker.tsxsrc/strings.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript must remain strict and pass the build and type-check gates.
Files:
tests/unit/ai-benchmark-gate.test.tsscripts/generate-benchmark-image.tssrc/features/ai/benchmarkGate.tssrc/features/onboarding/TutorialStep.tsxtests/unit/ai-sample-loop.test.tssrc/features/ai/benchmark.tssrc/features/session/SessionView.tsxsrc/features/ai/sampleLoop.tstests/unit/ai-models.test.tssrc/features/ai/sidecar.tstests/unit/ai-sidecar.test.tssrc/features/ai/modelStore.tssrc/stories/ModelPicker.stories.tsxsrc/features/ai/index.tssrc/features/ai/ModelPickerContainer.tsxsrc/features/settings/categories/AiCategory.tsxsrc/features/ai/ModelPicker.tsxsrc/strings.ts
tests/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest for unit and integration tests; component tests are not currently supported because the test environment is Node without RTL/jsdom.
Files:
tests/unit/ai-benchmark-gate.test.tstests/unit/ai-sample-loop.test.tstests/unit/ai-models.test.tstests/unit/ai-sidecar.test.ts
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Keep changes narrowly scoped: do not refactor adjacent code or add abstractions for hypothetical future needs.
Use one focused change per commit with a Conventional Commit subject such asfeat:,fix:,chore:,docs:, orci:.
When implementation conflicts withPLAN.md,ARCHITECTURE.md, orDESIGN-SYSTEM.md, surface the conflict rather than silently deviating.
Files:
tests/unit/ai-benchmark-gate.test.tsscripts/generate-benchmark-image.tssrc/features/ai/benchmarkGate.tssrc/features/onboarding/TutorialStep.tsxtests/unit/ai-sample-loop.test.tssrc/features/ai/benchmark.tsARCHITECTURE.mdsrc/features/session/SessionView.tsxsrc/features/ai/sampleLoop.tstests/unit/ai-models.test.tssrc/features/ai/sidecar.tstests/unit/ai-sidecar.test.tssrc/features/ai/modelStore.tssrc/stories/ModelPicker.stories.tsxPLAN.mdREADME.mdsrc/features/ai/index.tssrc/features/ai/ModelPickerContainer.tsxsrc/features/settings/categories/AiCategory.tsxsrc/features/ai/ModelPicker.tsxsrc/strings.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: All design-token values—colors, spacing, fonts, radii, shadows, motion, and z-index—must come fromsrc/design/tokens.ts; do not use raw hex values, arbitrarypx, or inlinecubic-beziervalues outside that file.
Keep toast and notification copy insrc/strings.ts; prefer that module for user-facing strings, including JSX andaria-labeltext.
Meet WCAG AA contrast requirements for every text/background pairing in both themes; do not communicate information by color alone.
Reduced-motion support is a global kill switch, and new motion sites must be disabled by default when reduced motion is requested.
Peer wire formats and identity derivation are cross-version contracts; coordinate changes so older peers remain interoperable and stored data is not stranded.
Files:
src/features/ai/benchmarkGate.tssrc/features/onboarding/TutorialStep.tsxsrc/features/ai/benchmark.tssrc/features/session/SessionView.tsxsrc/features/ai/sampleLoop.tssrc/features/ai/sidecar.tssrc/features/ai/modelStore.tssrc/stories/ModelPicker.stories.tsxsrc/features/ai/index.tssrc/features/ai/ModelPickerContainer.tsxsrc/features/settings/categories/AiCategory.tsxsrc/features/ai/ModelPicker.tsxsrc/strings.ts
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add new documentation files unless explicitly requested; update canonical documents,
CHANGELOG.md, orISSUES.mdonly when justified.
Files:
ARCHITECTURE.mdPLAN.mdREADME.md
src/**/*.stories.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Every component must have a Storybook story, and Storybook stories must pass axe-core accessibility checks.
Files:
src/stories/ModelPicker.stories.tsx
🔇 Additional comments (29)
src/features/ai/sidecar.ts (1)
12-14: LGTM!Also applies to: 32-33, 137-150, 171-178
src/features/ai/benchmark.ts (1)
1-1: LGTM!Also applies to: 176-176
src/features/ai/benchmarkGate.ts (1)
1-22: LGTM!src/features/ai/index.ts (1)
5-5: LGTM!Also applies to: 11-15, 78-80, 178-181
src/features/ai/modelStore.ts (1)
44-45: LGTM!Also applies to: 104-104, 196-204, 244-249
tests/unit/ai-benchmark-gate.test.ts (1)
1-94: LGTM!tests/unit/ai-models.test.ts (1)
297-310: LGTM!Also applies to: 312-322, 324-334, 336-354, 356-377, 379-395
tests/unit/ai-sidecar.test.ts (1)
8-8: LGTM!Also applies to: 150-150, 163-202
src/features/ai/ModelPicker.tsx (2)
51-51: LGTM!Also applies to: 72-78, 93-93, 159-208, 253-253, 273-296, 315-315, 365-366, 513-514, 523-524, 535-541, 597-597, 622-622
146-147: 📐 Maintainability & Code QualityNo changes needed.
src/strings.tsalready defines thestrings.ai.pickerkeys used byModelPicker.tsx.src/features/ai/ModelPickerContainer.tsx (1)
3-18: LGTM!Also applies to: 44-100, 121-131, 256-257, 303-317, 366-372, 466-540, 560-571, 584-595, 642-658
src/stories/ModelPicker.stories.tsx (1)
33-41: LGTM!Also applies to: 74-81, 119-147, 166-166, 186-186, 202-202, 263-263
src/features/settings/categories/AiCategory.tsx (6)
1-1: LGTM!Also applies to: 12-19, 32-36, 47-47
70-125: LGTM!
135-160: LGTM!Also applies to: 172-173
193-193: LGTM!Also applies to: 227-279
423-423: LGTM!Also applies to: 433-505, 701-740
281-292: 🎯 Functional CorrectnessNo change needed.
benchmarkSelected()returnstrueafteronBenchmark(selected)runs and returnsfalseonly when no model spec matches the active model ID. That no-selection path is already covered by the default picker UI states rather than an explicit failed phase.src/features/onboarding/TutorialStep.tsx (1)
1-6: LGTM!Also applies to: 68-68, 92-96
src/strings.ts (4)
138-139: LGTM!Also applies to: 154-157
617-617: LGTM!
1309-1340: LGTM!
1746-1751: LGTM!Also applies to: 1762-1769, 1778-1778, 1787-1788, 1798-1799, 1813-1829, 1858-1858
ARCHITECTURE.md (1)
386-392: LGTM!Also applies to: 428-431, 457-459, 472-472
PLAN.md (1)
83-92: LGTM!Also applies to: 105-105, 147-147
README.md (1)
155-170: LGTM!scripts/generate-benchmark-image.ts (1)
3-3: LGTM!src/features/ai/sampleLoop.ts (1)
102-113: LGTM!tests/unit/ai-sample-loop.test.ts (1)
1921-1923: LGTM!
|
|
||
| // First install becomes the selected model, but never switch models as | ||
| // an in-flight download finishes while AI is enabled. That would bypass | ||
| // the unbenchmarked enable warning. A later model stays installed until | ||
| // the user explicitly clicks "Use model." | ||
| let selected = false | ||
| const modelState = useModelStore.getState() | ||
| const aiEnabled = useSettingsStore.getState().values.aiFeaturesEnabled | ||
| if (!modelState.activeModelId && !aiEnabled) { | ||
| await selectModel(spec.id) | ||
| selected = true | ||
| } | ||
|
|
||
| updateCard(spec.id, { | ||
| phase: 'idle', | ||
| downloadProgress: null, | ||
| errorMessage: null, | ||
| }) | ||
| toast.success( | ||
| selected | ||
| ? strings.ai.picker.installedAndSelectedToast(spec.displayName) | ||
| : strings.ai.picker.installedToast(spec.displayName) | ||
| ) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap the post-download selectModel call in error handling.
selectModel awaits persist, which rejects when the Tauri store write fails. runDownload does not catch that rejection, and the caller at line 477 invokes it with void runDownload(spec). Two consequences follow:
- Lines 455-459 never run, so the card stays in its last download phase instead of returning to
idle. setupBusystaystrueforever, so the container keeps reportingonBusyChange(true)and the AI toggle inAiCategorystays gated until the pane remounts.
The download itself already succeeded and recordInstalled already persisted at line 441. A failed auto-selection must not strand the card. onActivate at lines 514-535 already applies this pattern.
🐛 Proposed fix to isolate the auto-selection failure
let selected = false
const modelState = useModelStore.getState()
const aiEnabled = useSettingsStore.getState().values.aiFeaturesEnabled
if (!modelState.activeModelId && !aiEnabled) {
- await selectModel(spec.id)
- selected = true
+ try {
+ await selectModel(spec.id)
+ selected = true
+ } catch {
+ // The files are installed and recorded. A failed auto-selection
+ // leaves the user the explicit "Use model" action.
+ }
}
updateCard(spec.id, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // First install becomes the selected model, but never switch models as | |
| // an in-flight download finishes while AI is enabled. That would bypass | |
| // the unbenchmarked enable warning. A later model stays installed until | |
| // the user explicitly clicks "Use model." | |
| let selected = false | |
| const modelState = useModelStore.getState() | |
| const aiEnabled = useSettingsStore.getState().values.aiFeaturesEnabled | |
| if (!modelState.activeModelId && !aiEnabled) { | |
| await selectModel(spec.id) | |
| selected = true | |
| } | |
| updateCard(spec.id, { | |
| phase: 'idle', | |
| downloadProgress: null, | |
| errorMessage: null, | |
| }) | |
| toast.success( | |
| selected | |
| ? strings.ai.picker.installedAndSelectedToast(spec.displayName) | |
| : strings.ai.picker.installedToast(spec.displayName) | |
| ) | |
| }, | |
| // First install becomes the selected model, but never switch models as | |
| // an in-flight download finishes while AI is enabled. That would bypass | |
| // the unbenchmarked enable warning. A later model stays installed until | |
| // the user explicitly clicks "Use model." | |
| let selected = false | |
| const modelState = useModelStore.getState() | |
| const aiEnabled = useSettingsStore.getState().values.aiFeaturesEnabled | |
| if (!modelState.activeModelId && !aiEnabled) { | |
| try { | |
| await selectModel(spec.id) | |
| selected = true | |
| } catch { | |
| // The files are installed and recorded. A failed auto-selection | |
| // leaves the user the explicit "Use model" action. | |
| } | |
| } | |
| updateCard(spec.id, { | |
| phase: 'idle', | |
| downloadProgress: null, | |
| errorMessage: null, | |
| }) | |
| toast.success( | |
| selected | |
| ? strings.ai.picker.installedAndSelectedToast(spec.displayName) | |
| : strings.ai.picker.installedToast(spec.displayName) | |
| ) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/ai/ModelPickerContainer.tsx` around lines 442 - 465, Wrap the
post-download auto-selection in runDownload with error handling around
selectModel, using the existing onActivate pattern as guidance. Ensure a
selectModel persistence failure is caught and does not prevent the subsequent
updateCard idle reset, success toast, or cleanup that clears setupBusy, while
preserving the download-success flow and selected-state behavior.
| {onOpenSettings ? ( | ||
| <button | ||
| type="button" | ||
| className="rounded-md outline-none focus-visible:ring-3 focus-visible:ring-accent-ring" | ||
| onClick={() => onOpenSettings('ai')} | ||
| aria-label={strings.session.aiStatus.openSettingsAriaLabel( | ||
| strings.session.aiStatus[aiChipStatus] | ||
| )} | ||
| > | ||
| <AiStatusChip status={aiChipStatus} /> | ||
| </button> | ||
| ) : ( | ||
| <AiStatusChip status={aiChipStatus} /> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 outline-none outline-style none forced-colors behavior change
💡 Result:
In Tailwind CSS v4, the behavior of the outline-none utility was changed to address inconsistencies in how focus rings were handled, particularly concerning accessibility and forced-colors modes [1][2]. The key changes are: 1. Renaming and Splitting Functionality: The utility previously known as outline-none (which applied a 2px invisible outline to maintain accessibility in forced-colors mode) has been renamed to outline-hidden [1][2]. A new utility named outline-none has been introduced that strictly sets outline-style: none [1][3]. 2. Handling of Forced Colors: - outline-hidden is designed to hide the default browser outline in standard modes while preserving an outline when forced-colors mode is active [1][3]. This is intended to ensure that users who rely on high-contrast/forced-colors settings still have a visible focus indicator [2]. - outline-none simply removes the outline entirely, regardless of the forced-colors setting [1][3]. 3. Upgrade Path: If you are upgrading from Tailwind CSS v3 to v4, you should replace occurrences of outline-none with outline-hidden if you wish to maintain the previous behavior (i.e., preserving focus visibility in forced-colors mode) [1][2]. These changes were made to provide more predictable behavior and to resolve the issue where the previous outline-none utility was "hijacking" the CSS property name to apply a specific accessibility implementation that not all users expected [2].
Citations:
- 1: https://tailwindcss.com/docs/upgrade-guide
- 2: Replace
outline-nonewithoutline-hidden, add newoutline-nonetailwindlabs/tailwindcss#14926 - 3: https://tailwindcss.com/docs/outline-style
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
fd -a 'SessionView\.tsx|dialog\.(tsx|ts)$|package\.json$|tailwind\.config\.(js|cjs|mjs|ts)$' . | sed 's#^\./##'
echo
echo "SessionView relevant lines around 1920-1965:"
if [ -f src/features/session/SessionView.tsx ]; then
sed -n '1920,1965p' src/features/session/SessionView.tsx | cat -n
fi
echo
echo "dialog relevant focus pattern:"
if [ -f src/components/ui/dialog.tsx ]; then
sed -n '40,85p' src/components/ui/dialog.tsx | cat -n
fi
echo
echo "Search Settings gear button aria-haspopup in SessionView:"
rg -n 'aria-haspopup="dialog"|onOpenSettings|AiStatusChip|outline-none|outline-hidden|ring-3|ring-accent-ring' src/features/session/SessionView.tsx
echo
echo "Tailwind config/package clues:"
for f in package.json tailwind.config.js tailwind.config.cjs tailwind.config.mjs tailwind.config.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: scotej/studyvis
Length of output: 8400
🌐 Web query:
MDN button aria-haspopup dialog opens dialog button dialog attributes
💡 Result:
According to the ARIA specification and official MDN documentation, the aria-haspopup attribute is used to indicate that an interactive element, such as a button, triggers a popup [1][2]. When the triggered popup is a dialog, you set aria-haspopup="dialog" on the button [1][3]. Key considerations for this implementation include: Technical Requirements: 1. Role matching: Ensure the container triggered by the button has the role="dialog" (or is a native element) [1][2]. 2. Focus management: When the dialog opens, keyboard focus must be moved to an appropriate element inside the dialog (e.g., the first focusable control or a close button) [4][5]. When the dialog closes, focus should return to the button that triggered it [4][6]. 3. Labeling: The dialog should be given an accessible name, typically using aria-labelledby to point to the dialog's title [5][6]. Accessibility Context and Debate: While aria-haspopup="dialog" is technically correct according to the ARIA specification [1][7], its practical value and implementation are subjects of ongoing discussion within the accessibility community [8][9][7]: - Variable Support: Historically, some screen readers have not announced "dialog" reliably or have inconsistently treated buttons with this attribute [9][7]. - User Experience: Some experts argue that adding aria-haspopup="dialog" can lead to redundant or confusing announcements for screen reader users, potentially cluttering the experience [8][7]. Others suggest it helps inform users that an action will surface content on the screen [9]. - Recommendations: Because of these inconsistencies, some developers prefer to omit aria-haspopup="dialog" and instead ensure the button's label clearly describes its purpose (e.g., "Manage Settings" instead of "Settings"), or they use other established patterns for dialog accessibility without relying on this specific attribute [7][10]. For robust dialogs, modern development favors the native HTML element, which provides built-in modal functionality (via showModal) and handles focus trapping and the inert state automatically, often reducing the need for manual ARIA management [10].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-haspopup
- 2: https://github.com/mdn/content/blob/main/files/en-us/web/accessibility/aria/reference/attributes/aria-haspopup/index.md
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Element/ariaHasPopup
- 4: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/button_role
- 5: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/dialog_role
- 6: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/examples/dialog/
- 7: should all popup use cases have aria-haspopup on the trigger element ? w3c/aria-practices#1926
- 8: Clarify usage of aria-haspopup w3c/aria#1024
- 9: Should the button to open the dialog have aria-haspopup="dialog" w3c/aria-practices#1383
- 10: https://accessibility.build/guides/accessible-dialog
Use outline-hidden, not outline-none, with this focus ring.
This button uses outline-none with focus-visible:ring-3 focus-visible:ring-accent-ring. In Tailwind v4, outline-none removes outlines everywhere, including forced-colors; ring utilities do not provide the required focus indicator in that mode. Use the same pattern as src/components/ui/dialog.tsx, which pairs that ring with focus-visible:outline-hidden.
While here, add aria-haspopup="dialog" to match the adjacent Settings gear icon button because this button opens the same Settings dialog.
Proposed change
{onOpenSettings ? (
<button
type="button"
- className="rounded-md outline-none focus-visible:ring-3 focus-visible:ring-accent-ring"
+ className="rounded-md outline-hidden focus-visible:ring-3 focus-visible:ring-accent-ring"
onClick={() => onOpenSettings('ai')}
+ aria-haspopup="dialog"
aria-label={strings.session.aiStatus.openSettingsAriaLabel(
strings.session.aiStatus[aiChipStatus]
)}
>
<AiStatusChip status={aiChipStatus} />
</button>
) : (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {onOpenSettings ? ( | |
| <button | |
| type="button" | |
| className="rounded-md outline-none focus-visible:ring-3 focus-visible:ring-accent-ring" | |
| onClick={() => onOpenSettings('ai')} | |
| aria-label={strings.session.aiStatus.openSettingsAriaLabel( | |
| strings.session.aiStatus[aiChipStatus] | |
| )} | |
| > | |
| <AiStatusChip status={aiChipStatus} /> | |
| </button> | |
| ) : ( | |
| <AiStatusChip status={aiChipStatus} /> | |
| )} | |
| {onOpenSettings ? ( | |
| <button | |
| type="button" | |
| className="rounded-md outline-hidden focus-visible:ring-3 focus-visible:ring-accent-ring" | |
| onClick={() => onOpenSettings('ai')} | |
| aria-haspopup="dialog" | |
| aria-label={strings.session.aiStatus.openSettingsAriaLabel( | |
| strings.session.aiStatus[aiChipStatus] | |
| )} | |
| > | |
| <AiStatusChip status={aiChipStatus} /> | |
| </button> | |
| ) : ( | |
| <AiStatusChip status={aiChipStatus} /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/session/SessionView.tsx` around lines 1945 - 1958, Update the
settings-opening button in the onOpenSettings branch of SessionView to use
focus-visible:outline-hidden instead of outline-none alongside its existing
focus ring, and add aria-haspopup="dialog" to identify the dialog it opens.
Leave the non-button AiStatusChip rendering unchanged.
Summary
Safety
Validation
npm run buildnpm run lintnpm run test— 1,045 testsnpm run format:checknpm run build-storybookThe local axe runner could not launch because this workspace has no Playwright browser; the protected GitHub CI check runs it before merge.
Closes #157
Summary by CodeRabbit
New Features
Bug Fixes