fix(ui-react): use semantic DropZone story text#2885
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDropZone now announces accepted-file counts through a localized, polite ARIA status region. Accessibility guidance, tests, default strings, and story styling were updated to cover the new behavior. ChangesAccessible DropZone selection feedback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DropZone
participant processFiles
participant statusRegion
DropZone->>processFiles: process selected files
processFiles->>DropZone: return accepted file batch
DropZone->>statusRegion: render localized count with status attributes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/ui-react/src/components/forms/DropZone.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/ui-react/src/components/forms/__tests__/DropZone.test.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
|
Admiral dispatch: deckhand-w2 is taking the independent exact-head source/test pass at ec155cc. Verify DropZone file-list hierarchy, behavior, states, and accessibility remain unchanged; publish evidence without editing, approving, arming, or merging. |
|
Independent exact-head verification — PASS with host-only Storybook limitation. Required/head checked: Exact scope/semantic review: Commands and results:
CI comparison: the PR's current Global-UX run passes its light/dark Storybook test runners and all four axe audit shards (and its aggregate gate); the later ui-react CI no-diff build/test/lint run also passes. PR remains open, unarmed, and unmerged. |
|
Admiral dispatch: bosun-w2 is taking the independent UX/accessibility judgment at exact head ec155cc. Review DropZone file-list contrast/hierarchy, interaction and states, reflow, RTL/i18n, and accessible semantics against the existing exact-head PASS and CI evidence. Post APPROVE or CHANGES; no edits, arming, or merge. |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — independent UX-surface review at exact head ec155cc21e799c01fe5faeb1c17f636c735ffa63.
Blocking P1 — accepted files are not exposed as a status message (WCAG 2.2 AA 4.1.3). The Default story conditionally inserts the selected-file list after browse/drop, but the list has neither role="status" nor an aria-live property. The rejected path correctly exposes its error with role="alert"; the accepted path gives a screen-reader user no programmatic confirmation of the result. The green static axe runs do not cover this state because the story initially renders with no selected files, so the changed list is absent from their DOM.
Required before approval: preserve the list semantics while making the accepted selection a polite live status (for example, aria-live="polite" plus an appropriate atomicity choice), and add a focused interaction/accessibility assertion for the accepted state.
Independent passing evidence:
- Footprint is exactly one color-token substitution in this story; production CSS resolves
.text-muted-foregroundto the semantic token in both themes. - Measured against the actual Storybook canvas: muted file text is 9.74:1 in light (
#444/#fff) and 11.57:1 in dark (#d0d0d0/#0f172a). It remains clearly subordinate to the prompt but legible. The file list is outside the disabled DropZone'sopacity-50wrapper, so actual file content does not acquire disabled styling. - Focus-ring contrast is 4.79:1 light and 8.97:1 dark. The component retains Enter/Space activation, browse and dashed drop affordances, localized accessible naming,
aria-disabled, and an alert for rejection/error. - The changed utility adds no geometry or directional properties: file-list flow, small-screen wrapping behavior, and RTL layout are unchanged.
- Focused DropZone tests: 22/22 pass. UI token discipline: 0 findings. Production Storybook preview build: 2,967 modules built. Exact-head CI light/dark Storybook axe shards and the UX aggregate are green. I accounted for deckhand-w2's exact-head PASS, but made the status-message judgment independently.
The PR was open, unmerged, and unarmed when this review was submitted.
|
Admiral ruling: Bosun w2’s accepted-file status-message P1 is accepted. PR 2885 remains unarmed. The retained issue-2559 branch must preserve list semantics and the semantic text token while exposing successful accepted-file selection as a polite live status with an appropriate atomicity contract, plus a focused interaction/accessibility regression. The static empty-state axe pass is insufficient. Route the amendment back to original author deckhand-w4 after its current Switch correction releases. |
|
Admiral dispatch: deckhand-w2 is amending retained DropZone head ec155cc to preserve selected-file list semantics and the semantic text token while exposing successful accepted-file selection as a polite live status with appropriate atomicity and focused interaction/accessibility coverage. Keep unarmed for independent verification and Bosun w2 re-review. |
|
Admiral harvest: corrected DropZone head |
|
Exact-head verification finding — NOT PASS (head The source behavior is correct on inspection: valid click and drop paths both update one separate, atomic polite Independent local exact-head evidence:
Fresh exact-head CI confirms typecheck, lint, coverage tests, build, all ui-react light/dark Storybook axe runners, and all four axe-audit shards passed. However, both required Local Storybook build could not complete because Storybook's manager cache hit a Windows sandbox filesystem-access error while resolving directories; this is recorded as environment-only, not attributed to the PR. |
|
Admiral dispatch: Bosun w3 is assigned independent corrected-head UX/accessibility review at |
|
Admiral ruling: deckhand-w1’s exact-head no-diff finding is accepted. DropZone source/live-region behavior passes, but both required ui-react jobs are red after canonical generation. Deckhand-w1 is assigned the retained branch from |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — independent corrected-head UX/accessibility review at exact head f1740373636408b99127124fb0d153a7be1f1c5c.
Blocking P1 — repeated accepted selections with the same count are silent. The new live status is driven only by acceptedFileCount. A common replacement flow such as one-file drop → one-file browse sets React state 1 → 1; React has no reason to commit a DOM change, so the polite live region has no new mutation for assistive technology to announce. The focused regression covers one two-file drop only, not click selection, singular, repeated equal-count selection, or the one-live-region invariant. Keep one concise atomic status, but make every successful selection produce a new announcement without accumulating duplicate or stale live regions, and pin drop + click, singular + plural, and repeated same-count behavior in tests.
Blocking release gate — both required ui-react CI / Build, test, lint, no-diff gate runs fail at this exact head. The Node-20 jobs rebuild packages/ui-react/dist/index.cjs from committed hash 52986e90 to 6daa0954 and report stale dist. A local Node-24 package build passed and produced no tracked diff, which does not override the required Node-20 gate; reconcile and commit the authoritative generated output.
The accepted blocker is otherwise corrected for an initial selection: click and drop converge on processFiles; the separate role="status" is polite and atomic; tn supplies the CLDR-aware singular/plural contract; the visible ul/li list remains intact; and oversize rejection remains role="alert". Keyboard/focus, localized accessible name and instructions, disabled behavior, file constraints, drag affordance, RTL direction, and responsive geometry are unchanged. No prop or other public API is added.
The only visible change remains the story token substitution. Production theme values give the file list 9.74:1 light contrast (#444 on #fff) and 11.57:1 dark contrast (#d0d0d0 on #0f172a), preserving a subordinate but legible hierarchy. Exact-head screenshot regression and Global-UX aggregate are green. Exact-head production Storybook/axe passed 196/196 suites and 898/898 tests in both light and dark; those static passes cannot detect the equal-count live-update failure.
Independent local exact-head evidence: DropZone 23/23, typecheck pass, token discipline 0 findings, package build pass, diff check clean. Local Storybook remains environment-blocked by the known Windows manager-cache parent-directory access fault. I accounted for deckhand-w1's source/test pass and its no-diff finding, while making the UX judgment independently.
PR remains open, unarmed, and unmerged.
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — authoritative dispatched-head verdict for f1740373636408b99127124fb0d153a7be1f1c5c only.
The PR advanced to a45df9710eef6a97eba0d615d28e58cfb53971cb during submission, so the preceding detailed review was attached by GitHub to the newer head despite documenting and testing f174037…. This review object is explicitly anchored to the required dispatched head; it is not an adjudication of a45df971….
Blocking findings at f174037…:
acceptedFileCountstores only the count. Replacing one accepted file with another sets1 → 1, causes no React DOM commit, and therefore produces no new polite live-region announcement. Preserve one status region, but make every successful click/drop selection produce a content change; cover click/drop, singular/plural, repeated equal-count selection, and exactly one live region.- Both required Node-20
ui-react CI / Build, test, lint, no-diff gateruns failed because the build changedpackages/ui-react/dist/index.cjs(52986e90 → 6daa0954). Reconcile the authoritative generated output.
Otherwise, the initial-selection amendment is sound: one concise polite atomic status, CLDR-aware tn keys, preserved ul/li list semantics, preserved rejection role="alert", no public API addition, and unchanged keyboard/focus, disabled, drag/drop, file constraints, RTL/reflow, and visible hierarchy. Local evidence: focused 23/23, typecheck pass, token discipline 0 findings, package build pass. Exact-head production light/dark Storybook axe passed 196 suites / 898 tests each, screenshot regression passed, and Global-UX aggregate passed. Local Storybook remained blocked by the Windows manager-cache sandbox fault.
PR was unarmed and unmerged at the dispatched-head verdict.
|
Admiral dispatch: Bosun w3 is assigned corrected-head re-review at a45df97. Reproduce every-selection live-region mutation across click/drop, singular/plural and repeated equal-count cases, exactly-one status, rejection semantics, shipped artifacts, Node-20 no-diff, production Storybook/axe, and fresh CI. Keep unarmed. |
|
Recovered Deckhand w1 correction evidence for head a45df97: only canonical ESM, CJS, and declaration artifacts were committed; DropZone 23/23, typecheck, token discipline, canonical build, ADR 0090 no-diff, and fresh CI including light/dark Storybook axe passed. PR remains unarmed under Bosun w3 corrected-head review. |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — independent corrected-head UX/accessibility re-review at exact head a45df9710eef6a97eba0d615d28e58cfb53971cb.
Blocking P1 — repeated successful selections with the same accepted count still do not mutate the polite status. DropZone.tsx:34,54,156-161 stores and renders only acceptedFileCount. A successful replacement that keeps the count equal sets React state 1 → 1 or 2 → 2, so React commits no status-content mutation and assistive technology receives no new selection confirmation. The focused regression at DropZone.test.tsx:46 covers one initial two-file drop only; it does not exercise click, singular, repeated equal-count replacement, or DOM mutation.
Direct exact-head reproduction against the shipped dist/index.js with a MutationObserver:
- one-file drop:
"1 file selected", one status region; - one-file click replacement: same text, 0 status mutations, one status region;
- two-file drop:
"2 files selected", 1 status mutation, one status region; - two-file click replacement: same text, 0 status mutations, one status region;
- oversized drop: one
role="alert"; the single prior success status remains and does not mutate.
Keep exactly one concise role="status" with polite/atomic semantics, but make every successful click/drop selection change its content without appending duplicate or stale live regions. Pin click and drop, singular and plural, repeated equal-count replacement, rejection, and exactly-one-status behavior in the focused test.
The second accepted blocker is corrected: canonical ESM/CJS/DTS artifacts are committed. Local production build completed and the ADR 0090 JS/CJS/DTS no-diff comparison passed; both exact-head Node-20 ui-react CI runs are green. Focused DropZone tests are 23/23, typecheck passes, and token discipline reports 0 findings.
Everything else remains sound: the visible ul/li file list is preserved; tn provides the localized singular/plural contract; rejection remains an alert; the localized button name/instructions, Enter/Space activation, focus order, disabled guards, native accept/multiple, maxSize, dashed drag affordance, outer RTL direction, reflow geometry, and public props are unchanged. The semantic list token resolves at 9.33:1 light and 12.15:1 dark; focus-ring contrast is 4.79:1 and 8.97:1. Exact-head production Storybook axe passed 196/196 suites and 898/898 tests in both light and dark, and all fresh CI is green; those static axe passes cannot detect the equal-count live-update failure. Local Storybook build is host-blocked by the known Windows parent-directory access fault, not a product finding.
PR is open, unarmed (autoMergeRequest: null), and unmerged.
|
Admiral harvest: Bosun w3 CHANGES is accepted at corrected head a45df97. The generated-artifact/no-diff blocker is resolved and all exact-head gates are green, but repeated successful equal-count replacements produce no live-region mutation, so assistive technology receives no new success announcement. Keep unarmed pending a focused correction. |
|
Admiral dispatch: Deckhand w1 is amending retained head |
|
Deckhand w1 correction checkpoint pushed (PR remains unarmed): Corrected head:
Fresh CI was triggered by this push. Full ui-react, canonical build/artifact/no-diff, and Storybook/axe evidence remain pending CI or a healthy local dependency environment. |
|
Correction: the exact pushed head is |
|
Admiral harvest: Deckhand w1 pushed the repeated equal-count announcement correction at exact head |
![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
🤖 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 `@packages/ui-react/src/components/forms/__tests__/DropZone.test.tsx`:
- Around line 61-77: Update the test case “replaces the polite status for
repeated equal-count dropped selections” to keep the status element reference
and capture/assert the inner text container instead. Verify the status region
remains stable while its inner span is replaced and still displays “1 file
selected.”
- Around line 123-146: Update the repeated equal-count selection test around the
status assertions to target the inner keyed span rather than the stable
role="status" container. Preserve the single-status and text-content checks, but
capture the span from the first selection and assert that the span after the
second selection has a different identity.
In `@packages/ui-react/src/components/forms/DropZone.tsx`:
- Around line 164-177: Keep the live-region <p> container stable in DropZone.tsx
by moving its key to an inner span wrapping the translated announcement text.
Update both test cases in
packages/ui-react/src/components/forms/__tests__/DropZone.test.tsx at lines
61-77 and 123-146 to compare screen.getByRole('status').firstElementChild
identity instead of the status element.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f907844-5985-4031-9251-dbafd50870de
⛔ Files ignored due to path filters (3)
packages/ui-react/dist/index.cjsis excluded by!**/dist/**,!**/dist/**packages/ui-react/dist/index.d.tsis excluded by!**/dist/**,!**/dist/**packages/ui-react/dist/index.jsis excluded by!**/dist/**,!**/dist/**
📒 Files selected for processing (5)
packages/ui-core/Contracts/DataEntry/DropZone.Accessibility.mdpackages/ui-react/src/components/forms/DropZone.stories.tsxpackages/ui-react/src/components/forms/DropZone.tsxpackages/ui-react/src/components/forms/__tests__/DropZone.test.tsxpackages/ui-react/src/i18n/catalog.ts
|
Correction complete at 95ad4bd. Regenerated only canonical dist/index.js and dist/index.cjs; fresh CI is green, including both Node-20 ui-react no-diff jobs, full Build & Test, and production Storybook/axe in light and dark. Local focused scope/type/lint/coverage/build plus token and barrel gates passed. Remains unarmed for independent source/test and Bosun UX re-review. |
|
Admiral dispatch: Machub deckhand-b is independently verifying corrected DropZone head |
ctwoodwa
left a comment
ctwoodwa
left a comment
There was a problem hiding this comment.
CHANGES — immutable head 95ad4bd2ec3a499d2e120b33b7ad950ac15f0403.
Blocking accessibility semantics: DropZone.tsx:164-167 creates the populated role="status" only after a successful selection and keys the status element itself by announcementId. The first announcement therefore arrives in a newly inserted, already-populated polite live region, and every later selection replaces that live-region container. That is not a reliable status-announcement lifecycle: the live region must exist before its content changes; the dynamic-insertion exception commonly afforded to role="alert" does not apply to polite role="status". See the ARIA live-region guidance: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Guides/Live_regions and W3C ARIA22: https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA22.html.
The focused regressions currently lock in the defect: DropZone.test.tsx:68-75 and :134-144 assert that the status container is replaced. Keep one empty, stable role="status" / atomic sr-only container mounted from initial render; update its text (or a keyed inner child) after selection. Update repeated click/drop tests to assert the container stays identical while its announcement content changes. Preserve the selected-file list, rejection role="alert", semantic text token, and catalog plural keys.
Exact-head evidence:
- PASS focused DropZone: 28/28.
- PASS full ui-react: 328 files, 7,277 tests; scope-bleed, typecheck, lint, build, and reproducible JS/CJS/DTS no-diff.
- PASS local logical-CSS, label-continuity, token-discipline, barrel, locale, and a11y-story coverage gates; no changed-file findings in advisory concept/small-screen/breakpoint reports.
- PASS exact-head CI: both ui-react no-diff jobs; Storybook/axe light + dark; all four axe shards; visual regression; component-doc drift; Global-UX aggregate. Static axe cannot prove screen-reader announcement timing.
- Local Storybook build reproduced the known Windows Storybook/esbuild manager-cache access denial; no product finding is inferred from that host limitation.
- Scope is the accepted accessibility amendment plus required dist regeneration; PR remains clean, open, and unarmed.
The story token and visible behavior otherwise pass: text-muted-foreground is semantic/dual-theme, the selected-file list remains a real list outside disabled opacity, and the new count copy uses the catalog tn singular/plural seam.
a72cc1b to
947c6d2
Compare
1 participant
Summary
Verification
Closes #2559
Summary by CodeRabbit
Accessibility
UI Improvements
Localization