[codex] Add stable OpenWorlds agent UI hooks - #495
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds standardized data-worldos test IDs, ARIA attributes, and dialog semantics across OpenWorlds UI components (navigation, launcher, table, settings, character modals, toasts, and camp sidebar) and adds a static test that asserts these hooks remain present. ChangesOpenWorlds UI Accessibility and Test Hooks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
100yenadmin
force-pushed
the
codex/worldos-agent-ui-hooks
branch
from
May 31, 2026 22:54
9384cbc to
698ed65
Compare
Member
Author
|
@coderabbitai review |
100yenadmin
marked this pull request as ready for review
May 31, 2026 22:55
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
viewer/openworlds/chrome.jsx (2)
269-284: ⚡ Quick winRefactor aria-label handling to avoid redundancy.
The current pattern accesses
buttonProps["aria-label"]explicitly (line 282) and then spreadsbuttonProps(line 284), which creates redundancy whenaria-labelis present inbuttonProps. The spread will override the explicit attribute with the same value.A clearer pattern would destructure
aria-labelfrombuttonPropsbefore spreading:♻️ Suggested refactor
-function IconPlate({ size = 56, label, framed = true, glyph, tone, children, onClick, active, style, testId, ...buttonProps }) { +function IconPlate({ size = 56, label, framed = true, glyph, tone, children, onClick, active, style, testId, ...buttonProps }) { + const { "aria-label": ariaLabelProp, ...restButtonProps } = buttonProps; return ( <button type="button" className={`icon-plate ${framed ? "framed" : ""} ${active ? "active" : ""}`} onClick={onClick} style={{ width: size, height: size, ...(active ? { boxShadow: `inset 0 0 0 1px var(--b-500), inset 0 0 0 3px var(--p-100), inset 0 0 0 4px var(--b-400), 0 0 20px -2px var(--gold-glow)` } : {}), ...(style || {}), }} title={label} - aria-label={buttonProps["aria-label"] || label || undefined} + aria-label={ariaLabelProp || label || undefined} data-worldos-testid={testId || undefined} - {...buttonProps} + {...restButtonProps} >🤖 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 `@viewer/openworlds/chrome.jsx` around lines 269 - 284, In IconPlate, avoid redundant aria-label by destructuring it from buttonProps (e.g., const { "aria-label": ariaLabel, ...restButtonProps } = buttonProps) and then set aria-label on the button to ariaLabel || label || undefined, and spread restButtonProps instead of buttonProps; remove the explicit buttonProps["aria-label"] usage so the spread cannot override the explicit computed value and other props still pass through.
295-309: 💤 Low valueNote: Inconsistent aria-label prop pattern between IconPlate and BrassButton.
BrassButtonuses a dedicatedariaLabelprop (line 295), whileIconPlateallowsaria-labelto be passed via the spreadbuttonProps(line 282). This inconsistency could confuse developers about which pattern to use when calling these components.Consider standardizing both components to use the same pattern—either a dedicated
ariaLabelprop or allowing it viabuttonProps. The dedicated prop approach (as used here inBrassButton) is clearer and more discoverable.🤖 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 `@viewer/openworlds/chrome.jsx` around lines 295 - 309, BrassButton and IconPlate are inconsistent: BrassButton declares an explicit ariaLabel prop while IconPlate currently accepts aria-label via the spread buttonProps; standardize to the explicit prop pattern by updating IconPlate to accept an ariaLabel prop (in its function signature), explicitly forward aria-label using aria-label={ariaLabel || undefined} on the rendered button, and ensure the spread buttonProps does not override or duplicate aria-label (drop it from the spread or prefer the explicit ariaLabel). This keeps the public API consistent (use ariaLabel) and avoids duplicate/conflicting attributes.
🤖 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 `@viewer/openworlds/screen-launcher.jsx`:
- Line 480: The JSX uses a boolean variable selected for the aria-pressed
attribute (aria-pressed={selected}); change it to pass an explicit string per
ARIA spec by converting selected to "true"/"false" (or "mixed" when applicable)
before assigning to aria-pressed in the ScreenLauncher component (or the
component/element that renders this button), e.g., replace the boolean usage
with a string conversion using either a ternary based on selected or
String(selected) so the attribute value is always a string.
In `@viewer/openworlds/screen-table.jsx`:
- Around line 1180-1186: The button currently sets aria-label to the verbose
hint which overrides the concise visible label; change the button so the
accessible name is the visible label (use aria-label={label} or remove the
aria-label prop entirely) and keep title={hint} to supply the descriptive
tooltip; update the JSX around the button element that uses onClick, title,
hint, label, data-worldos-testid and data-worldos-action-id so screen readers
hear the concise label (e.g., "Say" or "Roll d20") while the hint remains only
as the title/tooltip.
- Around line 593-609: The banner currently sets role dynamically based on
surfaceStatus but always forces aria-live="polite", which downgrades alerts;
update the JSX that renders the status banner so aria-live matches the role:
when surfaceStatus === "loading" (role "status") keep aria-live="polite", and
when rendering the error path (role "alert" for surfaceStatus !== "loading")
either remove the explicit aria-live or set aria-live="assertive" so the alert
remains assertive; adjust the conditional that builds the div (referencing
surfaceStatus and its role) to choose the appropriate aria-live value rather
than hardcoding "polite".
---
Nitpick comments:
In `@viewer/openworlds/chrome.jsx`:
- Around line 269-284: In IconPlate, avoid redundant aria-label by destructuring
it from buttonProps (e.g., const { "aria-label": ariaLabel, ...restButtonProps }
= buttonProps) and then set aria-label on the button to ariaLabel || label ||
undefined, and spread restButtonProps instead of buttonProps; remove the
explicit buttonProps["aria-label"] usage so the spread cannot override the
explicit computed value and other props still pass through.
- Around line 295-309: BrassButton and IconPlate are inconsistent: BrassButton
declares an explicit ariaLabel prop while IconPlate currently accepts aria-label
via the spread buttonProps; standardize to the explicit prop pattern by updating
IconPlate to accept an ariaLabel prop (in its function signature), explicitly
forward aria-label using aria-label={ariaLabel || undefined} on the rendered
button, and ensure the spread buttonProps does not override or duplicate
aria-label (drop it from the spread or prefer the explicit ariaLabel). This
keeps the public API consistent (use ariaLabel) and avoids duplicate/conflicting
attributes.
🪄 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: 9d27166e-54f8-4e84-9e16-984844539602
📒 Files selected for processing (8)
viewer/openworlds/camp-sidebar.jsxviewer/openworlds/chrome.jsxviewer/openworlds/screen-character.jsxviewer/openworlds/screen-launcher.jsxviewer/openworlds/screen-settings.jsxviewer/openworlds/screen-table.jsxviewer/openworlds/toast.jsxviewer/tests/test_openworlds_static.py
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 31, 2026
This was referenced May 31, 2026
100yenadmin
added a commit
that referenced
this pull request
May 31, 2026
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
1 participant
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
aria-labelanddata-worldos-testidhooks across the OpenWorlds launcher, chrome, table, settings, character, camp sidebar, and toast surfaces./movepath.chronicle-resumeis unique to the primary banner, card resume useschronicle-resume-detail, start-flow and create-submit are split, session-surface status no longer uses an app-status hook name,aria-pressedis explicit, alert live-region politeness is assertive, and action accessible names stay concise.Evidence
77722fbrebased ontomain@9d36809./Volumes/LEXAR/Codex/worldos-built-app-playtest/ui-hooks-app-proof-20260601T054934.dist/WorldOS.app, providerscripted, private art root/Users/lume/ClawDnD-val/content/worlds/_private, live campaigncamp_c44d76054d36, actor Abby, five enabled actions, writable/move, deterministic DM narration after a real move, and real image bytes for location/portrait/class probes.Tests
python3 -m pytest viewer/tests/test_openworlds_static.py -q-> 39 passed, 2 subtests passedpython3 -m pytest qa/test_macos_app_static.py -q-> 6 passed