HS-1653425 Add accessible name and id to ASR comment textarea#1849
Conversation
The Additional Salary Request additional-info textarea had no label,
aria-label, or id, leaving QA automation to rely on a brittle
page.locator('textarea') selector. Add a stable id (asr-additional-info)
and a localized aria-label so it can be targeted via getByRole('textbox',
{ name: 'Additional Information' }) or #asr-additional-info.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bundle sizes [mpdx-react]Compared against 4199ea3 No significant changes found |
wjames111
left a comment
wjames111
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review — Verdict: ✅ APPROVED WITH SUGGESTIONS
Mode: standard · Agents: Architecture, Testing, Standards, UX, Financial (domain) + cross-examination debate
Clean, well-tested, net-positive accessibility change. The comment textarea previously had no accessible name; this PR adds one plus a stable id, with a colocated test asserting both. No blockers. One medium-priority suggestion and a couple of minor nits, none of which block merge.
Risk
1/10 (LOW) · 2 files · +13 −1 · Reviewer level: ANY
Findings
| Sev | Tier | Location | Note |
|---|---|---|---|
| 5.0 | Medium | ApprovalProcess.tsx:64-68 |
Prefer the label prop (as sibling ContactInformation.tsx does) over inputProps['aria-label'] — gives a visible, persistent label. Consistency/polish, not a defect. |
| 3.5 | Suggestion | ApprovalProcess.test.tsx:102 |
Redundant toBeInTheDocument() after getByRole({ name }). Harmless. |
| 2.5 | Suggestion | ApprovalProcess.tsx:65 |
Static hardcoded id — low duplicate-id risk (instances don't co-mount). |
Notes
- Dependency surface:
ApprovalProcessalso renders inCalculationReports/SubmitModal.tsx:95(SalaryCalculator), not just the three ASR FormVersions — but no two instances co-mount, so the staticiddoesn't collide. - Financial agent: No financial/currency/aggregation code despite the
AdditionalSalaryRequest/path trigger. - Standards: All applicable checklist items PASS (named export, localized
aria-label, noany/new Date()/debug output). - Debate: UX initially rated the label-mechanism issue 6.5; after cross-examination both UX and Architecture converged on ~5.0 (it over-penalized what is a net a11y improvement, and WCAG 2.5.3 doesn't actually trigger since there's no visible label text).
Severity < 7 findings can be dismissed by replying /dismiss: <reason> on the inline comment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: LOW (1/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is low risk
- No blocking issues were found
- All suggestions have been posted as review comments for the developer to consider
If you believe this PR needs human review, dismiss this approval and request a review manually.
wjames111
added
On Staging
|
Preview branch generated at https://hs-1653425.d3dytjb8adxkk5.amplifyapp.com |
1 participant
Description
id(asr-additional-info) and a localizedaria-label(Additional Information) to the Additional Salary Request "Additional Information / Optional Comments" textarea inApprovalProcess.tsx.<textarea>rendered in the ASR form, so QA automation was forced to use a brittlepage.locator('textarea')selector. It can now target the field reliably viagetByRole('textbox', { name: 'Additional Information' })or#asr-additional-info.<textarea>.Related ticket: https://secure.helpscout.net/conversation/3357987199/1653425?viewId=8086747
Requested by QA Automation (CruGlobal/qa_automation PR #23,
mpdx_playwright_tests/tests/asr/asrPage.js).Testing
<textarea>element hasid="asr-additional-info"andaria-label="Additional Information".getByRole('textbox', { name: 'Additional Information' }).Checklist:
/pr-reviewcommand locally and fixed any relevant suggestions🤖 Generated with Claude Code