[codex] Fix web interface review issues#3
Merged
jamiepine merged 1 commit intospacedriveapp:mainfrom Apr 12, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAccessibility and styling updates across components: added focus-visible rings and aria-labels, narrowed transition utilities, semantic element/keyboard handling adjustments, FormControl refactor to clone children and merge aria-describedby, and removal of the rehype-raw plugin from Markdown rendering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/primitives/src/TabBar.tsx (1)
95-103:⚠️ Potential issue | 🟠 MajorClose control is still not an accessible, semantic action target.
Line 95-Line 103 uses a clickable
<span>inside a parent<button>, which leaves the close action without proper keyboard/button semantics and creates an interactive-within-interactive pattern. Please make the close affordance a real button control with an explicit accessible name, and place it outside the parent button’s interactive element structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/primitives/src/TabBar.tsx` around lines 95 - 103, The close affordance is implemented as a clickable <span> nested inside a parent interactive <button>; replace that span with a real <button> element (e.g., the close control in TabBar.tsx where the span is rendered), move it so it is not a descendant of the parent tab button to avoid interactive-within-interactive, give it an explicit accessible name via aria-label (or visually hidden text) such as "Close tab", preserve the existing onClick handler behavior (including e.stopPropagation()) on the new button, and keep the same styling/positioning so the visual layout is unchanged while ensuring proper keyboard and semantic accessibility.packages/ai/src/TaskCreateForm.tsx (1)
48-57:⚠️ Potential issue | 🟠 MajorAdd programmatic labels for the title and description fields.
The controls at Line 57 and Line 87 still depend on placeholder text only. That’s not a sufficient accessible name and can break form usability for screen-reader users.
♿ Suggested fix
-import { useCallback, useState } from "react"; +import { useCallback, useId, useState } from "react"; @@ export function TaskCreateForm({ @@ }: TaskCreateFormProps) { + const titleInputId = useId(); + const descriptionInputId = useId(); const [title, setTitle] = useState(""); const [description, setDescription] = useState(""); @@ <div className={clsx("flex flex-col gap-2", className)}> <div className="flex items-center gap-2"> + <label htmlFor={titleInputId} className="sr-only"> + Task title + </label> <input + id={titleInputId} type="text" @@ </div> + <label htmlFor={descriptionInputId} className="sr-only"> + Description + </label> <textarea + id={descriptionInputId} value={description}Also applies to: 82-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai/src/TaskCreateForm.tsx` around lines 48 - 57, The title and description inputs in TaskCreateForm lack programmatic labels and rely only on placeholders; add accessible names by giving each control an id and associating a visible or visually-hidden <label> (or aria-label/aria-labelledby) with that id for both the title input (controlled by title/setTitle and onKeyDown handling calling handleSubmit/onCancel) and the description textarea (controlled by description/setDescription); ensure the ids are unique (e.g., title-input, description-input) and update any tests/styles that depend on markup so screen readers receive a proper label for each field.
🧹 Nitpick comments (1)
packages/ai/src/TaskRow.tsx (1)
33-38: Refine keyboard activation parity for the row button role.Line 34 currently activates on
keydownfor both Space and Enter, repeating while the key is held. Align with native button semantics: Enter activates onkeydown, Space activates onkeyup, and ignore repeat events.Suggested patch
const isRowInteractive = typeof onClick === "function"; const handleRowKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => { - if (e.key === "Enter" || e.key === " ") { + if (e.repeat) return; + if (e.key === "Enter") { e.preventDefault(); onClick?.(task); } }; + const handleRowKeyUp = (e: React.KeyboardEvent<HTMLDivElement>) => { + if (e.repeat) return; + if (e.key === " ") { + e.preventDefault(); + onClick?.(task); + } + }; return ( <div - onKeyDown={isRowInteractive ? handleRowKeyDown : undefined} + onKeyDown={isRowInteractive ? handleRowKeyDown : undefined} + onKeyUp={isRowInteractive ? handleRowKeyUp : undefined}Also applies to: line 45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai/src/TaskRow.tsx` around lines 33 - 38, The row keyboard handlers currently activate on keydown for both Space and Enter and don't ignore repeat events; update handleRowKeyDown to ignore e.repeat and only activate on Enter (preventDefault + onClick(task) on keydown for Enter), and add/adjust handleRowKeyUp (or the existing keyup handler referenced) to activate on Space on keyup (preventDefault + onClick(task)) while also ignoring repeat events; ensure both handlers use the same onClick?.(task) call and maintain accessibility semantics for role="button".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/forms/src/Form.tsx`:
- Around line 93-103: The code currently overwrites any aria-describedby on the
child when cloning (computed as describedBy using formDescriptionId and
formMessageId); update the clone to preserve and merge the child's existing
aria-describedby with describedBy instead of replacing it: read
existingDescribed = (children.props && children.props['aria-describedby']) as
string | undefined, split and combine unique tokens from existingDescribed and
describedBy (space-separated), trim and join them into a final string, and pass
that merged value to cloneElement along with id: formItemId and aria-invalid:
!!error so that cloneElement(children, { id: formItemId, 'aria-describedby':
mergedDescribed, 'aria-invalid': !!error }) preserves any child-provided
descriptions.
---
Outside diff comments:
In `@packages/ai/src/TaskCreateForm.tsx`:
- Around line 48-57: The title and description inputs in TaskCreateForm lack
programmatic labels and rely only on placeholders; add accessible names by
giving each control an id and associating a visible or visually-hidden <label>
(or aria-label/aria-labelledby) with that id for both the title input
(controlled by title/setTitle and onKeyDown handling calling
handleSubmit/onCancel) and the description textarea (controlled by
description/setDescription); ensure the ids are unique (e.g., title-input,
description-input) and update any tests/styles that depend on markup so screen
readers receive a proper label for each field.
In `@packages/primitives/src/TabBar.tsx`:
- Around line 95-103: The close affordance is implemented as a clickable <span>
nested inside a parent interactive <button>; replace that span with a real
<button> element (e.g., the close control in TabBar.tsx where the span is
rendered), move it so it is not a descendant of the parent tab button to avoid
interactive-within-interactive, give it an explicit accessible name via
aria-label (or visually hidden text) such as "Close tab", preserve the existing
onClick handler behavior (including e.stopPropagation()) on the new button, and
keep the same styling/positioning so the visual layout is unchanged while
ensuring proper keyboard and semantic accessibility.
---
Nitpick comments:
In `@packages/ai/src/TaskRow.tsx`:
- Around line 33-38: The row keyboard handlers currently activate on keydown for
both Space and Enter and don't ignore repeat events; update handleRowKeyDown to
ignore e.repeat and only activate on Enter (preventDefault + onClick(task) on
keydown for Enter), and add/adjust handleRowKeyUp (or the existing keyup handler
referenced) to activate on Space on keyup (preventDefault + onClick(task)) while
also ignoring repeat events; ensure both handlers use the same onClick?.(task)
call and maintain accessibility semantics for role="button".
🪄 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
Run ID: c39793d0-46dc-49d9-9e03-9cec5157e985
📒 Files selected for processing (16)
packages/ai/src/ChatComposer.tsxpackages/ai/src/Markdown.tsxpackages/ai/src/MessageBubble.tsxpackages/ai/src/TaskCreateForm.tsxpackages/ai/src/TaskRow.tsxpackages/explorer/src/GridItem.tsxpackages/explorer/src/TagPill.tsxpackages/forms/src/Form.tsxpackages/primitives/src/Button.tsxpackages/primitives/src/CircleButton.tsxpackages/primitives/src/Input.tsxpackages/primitives/src/NumberStepper.tsxpackages/primitives/src/SearchBar.tsxpackages/primitives/src/Select.tsxpackages/primitives/src/ShinyButton.tsxpackages/primitives/src/TabBar.tsx
💤 Files with no reviewable changes (1)
- packages/ai/src/Markdown.tsx
vsumner
force-pushed
the
codex/web-guidelines-fixes
branch
from
0982540 to
5350cce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transition-allusages with explicit transitions and add stronger focus-visible states.Validation
bun install --frozen-lockfilebun run typecheckinpackages/ai,packages/forms,packages/explorer, andpackages/primitivesbun run buildinpackages/ai,packages/forms,packages/explorer, andpackages/primitivesgit diff --checkNotes
Root
bun run typecheckreaches the changed packages successfully, then fails in.storybookbecause its typecheck command invokes baretsc --noEmitand prints TypeScript help.examples/showcasealso has existing type errors aroundTooltip,ToolCall, and duplicatereact-hook-formtypes.