Skip to content

fix(mobile): keep a choice row's name while it saves - #8513

Merged
iscekic merged 3 commits into
mainfrom
drain/choice-row-accessible-name
Oct 22, 2026
Merged

iscekic merged 3 commits into
mainfrom
drain/choice-row-accessible-name

Conversation

@iscekic

@iscekic iscekic commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — ran the app on the omp36 emulator against the local dev stack with an owned fake account, opened Profile > Code Reviewer > GitHub > Review Style, held the config save at the network boundary, and read the row's uiautomator dump node before and after the change: it read busy alone on the previous component and Lenient and Only serious problems, busy on this branch, returning to Lenient and Only serious problems once the save settled.

What changed

ChoiceRow, the shared radio/checkbox row, now sets an explicit accessibilityLabel composed from its label and description with formatList in the active app language. Rows rendered from custom children keep their own text-derived name.

React Native's Android view manager rebuilds a control's contentDescription from the accessibility label plus the states that have no node setter (busy, expanded, string checked), and it does not clear that description again when the state goes away. A row that reports a busy state without a label of its own therefore ends up describing only the state. With a label, the state is appended to the name, and clearing it returns to the name instead of leaving a stale description.

New mounted test apps/mobile/src/components/ui/choice-row.mounted.test.tsx pins the name composition, the busy round trip, the custom-children case, and the radio/checkbox roles.

End-to-end evidence

Ran the app on the omp36 emulator against the local dev stack with an owned fake account, opened Profile > Code Reviewer > GitHub > Review Style, and held the config save at the network boundary so the in-flight state could be read from uiautomator dump.

Before (main, save in flight):

Strict, Flag everything, hold a high bar | android.widget.RadioButton
Balanced, Meaningful findings without noise | android.widget.RadioButton
busy | [55,611][1025,755] | android.widget.RadioButton | checked=true
Roast, Brutally honest, entertainingly so | android.widget.RadioButton

After (this branch, same row, same bounds, save in flight):

Strict and Flag everything, hold a high bar | android.widget.RadioButton
Balanced and Meaningful findings without noise | android.widget.RadioButton
Lenient and Only serious problems, busy | [55,611][1025,755] | android.widget.RadioButton | checked=true
Roast and Brutally honest, entertainingly so | android.widget.RadioButton

Idle state after the save settles (this branch): the same row reads Lenient and Only serious problems, with no leftover state text.

Other checks on this VM: pnpm --filter kilo-app exec tsc --noEmit clean, pnpm --filter kilo-app exec oxlint clean on both files, the full mobile suite (pnpm --filter kilo-app test) passed 1242 files / 18437 tests, and the new mounted test fails against the previous component and passes with the change.

A label change has no pixel delta, so screenshots cannot demonstrate it; the accessibility-tree nodes above are the evidence.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: A11y label: How should the composed screen-reader name join the row label and its description?
    Answer: formatList (locale-aware conjunction)
    Confidence: 0.65 · Precedent: Engineering conventions: prefer existing libraries to hand-rolled code (session df491946927a, "we will try and use libraries wherever we can"); UX: mobile app always respects i18n (session dcb3f9170164)
  2. Question: Children rows: Rows built from custom children (language rows, repo toggles, tour cards, finding filters, provider choices) currently have no explicit label; none sets busy. Extend the fix there?
    Answer: Leave them on the derived name
    Confidence: 0.65 · Precedent: Speed and least work: do the least work possible with no drop in quality (session 7f3470e0ba6f, "do the least amount of work possible, with no drop in quality"); no children row sets busy, so the defect does not occur there
  3. Question: Test scope: Plan: make ChoiceRow set an explicit accessible name from label + caption (formatList) for every labelled row, leave children-based rows alone, keep the busy state. Should this ship with a new mounted regression test, or only the Android dump proof?
    Answer: Add choice-row.mounted.test.tsx
    Confidence: 0.60 · Precedent: Keep CI checks that catch real errors in PRs: session 99e88001c4b4, "we want to catch errors in PRs, as early as possible"; the Android accessibility dump during a style save is still required as end-to-end proof
  4. Question: Base refresh: CI cannot go green: every install job failed on a broken lockfile that main has since repaired (637ac1b), but GitHub re-runs reuse the stale PR merge ref, so re-runs fail identically. How should I refresh CI for this head?
    Answer: Merge main into the branch
    Confidence: 0.90 · Precedent: Git and pull requests: merge main only when a conflict or a fix on main needs it — session a247f3c19b1e, "try merging main in, that may fix the issue"

ChoiceRow now composes an explicit accessibilityLabel from its label and
caption, so the shared row control always has a name of its own. React
Native rebuilds a control's content description from that label plus any
state without a node setter, so the name now survives the row's busy
state instead of being replaced by it, and the state clears back to the
name rather than leaving a stale description.

Rows rendered from custom children keep their own text-derived names.
The new mounted test pins the composition, the busy round trip, the
children case, and the radio/checkbox roles.
@iscekic iscekic added drain Work item from the findings drain explorer:android Finding surfaced by the Android explorer labels Oct 21, 2026
@kilo-code-bot

kilo-code-bot Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The only change since the previous review is a one-line reformat of the already-reviewed accessibilityLabel expression in ChoiceRow; no behavior, test, or file-set change was introduced, so no new findings.

Assumptions: [INFERENCE] I did not run the mobile test, lint, or typecheck suites; the PR author reports they are clean and that the new mounted test fails against the previous component. The formatList(namedParts, i18n.language) call resolves through listFormat (apps/mobile/src/lib/format.ts:87), whose cache is bounded by locale, so no memory growth is added by this PR. The failing CI checks stop in pnpm install --frozen-lockfile on a @ungap/structured-clone lockfile snapshot mismatch; the PR diff touches only two files and no lockfile, so that base issue is outside this change.

Files Reviewed (2 files)
  • apps/mobile/src/components/ui/choice-row.tsx
  • apps/mobile/src/components/ui/choice-row.mounted.test.tsx
Previous Review Summary (commit 224a97c)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 224a97c)

Status: No Issues Found | Recommendation: Merge

Executive Summary

The change adds an explicit accessibilityLabel to the shared choice row using the existing locale-aware formatList helper (same pattern as form-field-a11y.ts), and the new mounted test matches the repo's *.mounted.test.tsx conventions with no leak-prone state.

Assumptions: [INFERENCE] I did not run the mobile test/lint/typecheck suites; the React Native contentDescription behavior described in the PR body was not independently reproduced. The listFormat cache is bounded by locale, so no memory growth is introduced. I verified that no children-based ChoiceRow caller sets busy, so leaving those rows on their derived name matches the stated intent.

Files Reviewed (2 files)
  • apps/mobile/src/components/ui/choice-row.tsx
  • apps/mobile/src/components/ui/choice-row.mounted.test.tsx

Reviewed by deepseek-flash-latest · Input: 40.9K · Output: 2.7K · Cached: 287K

Review guidance: REVIEW.md from base branch main

@iscekic

iscekic commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

bot: The failing checks all stop in pnpm install --frozen-lockfile, before any code in this change runs:

Scope: all 67 workspace projects
Lockfile is up to date, resolution step is skipped
[ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY] Broken lockfile: no entry for '@ungap/structured-clone@1.3.0' in pnpm-lock.yaml

The merge ref of this branch with main carries a lockfile whose snapshot still names @ungap/structured-clone@1.3.0 while the packages section only holds 1.4.0, so the base is broken independently of this change; the same failure hits every install-based job (changes, gate, ci-wiring, typecheck, lint, format-check, check-unused, i18n-leftover, mobile-test). Nothing in this branch touches pnpm-lock.yaml.

I will re-run the failed checks once the base lockfile is repaired on main; the local verification for this change is already in the body (mobile suite, typecheck, lint, emulator accessibility dump before/after).

@iscekic iscekic added the merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. label Oct 22, 2026
@iscekic
iscekic enabled auto-merge October 22, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain explorer:android Finding surfaced by the Android explorer merge:bot Bot-owned review; merge when ready once readiness passes and a review approves.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants