Repository navigation
fix(mobile): make the a11y focus helper's contract observable and cover the status hook - #8580
Conversation
requestA11yFocus (formerly moveA11yFocus) reports only what it can see: a mounted node handle and a dispatched focus request. Callers latch and retry on that signal, so the doc text saying the platform accepted the move described an outcome React Native does not expose. Add render coverage for useStatusAnnouncement: the platform gate and the cross-render dedupe now have iOS and Android cases beside the pure nextAnnouncement table.
iscekic
added
drain
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental delta is test-only: the reaction picker suite now pins the request latch after a dispatched focus request, a new pure suite covers Assumptions: Files Reviewed (3 files)
Previous Review Summaries (2 snapshots, latest commit 82b567c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 82b567c)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental delta is test-only cleanup: five announcement suites drop Assumptions: Files Reviewed (27 files)
Previous review (commit 792d360)Status: No Issues Found | Recommendation: Merge Executive SummaryI reviewed the 25 changed files in the mobile app: a repository-wide rename of Assumptions: Files Reviewed (25 files)
Reviewed by deepseek-flash-latest · Input: 59.7K · Output: 9.3K · Cached: 505.5K Review guidance: REVIEW.md from base branch |
iscekic
added
merge:blocked:insufficient-e2e
iscekic
removed
merge:bot
|
bot: Merged main and resolved four conflicts: announce.ts keeps HostInstance/sendAccessibilityEvent with the request-only contract; announce.test.ts tests that API; blocking-card-state.ts keeps native host refs; reaction-picker-sheet.tsx keeps TextInstance and the dispatch latch. Removed obsolete announcement mocks in a follow-up commit. Verified 365 targeted tests, mobile typecheck/lint/format/unused checks, and Android native events: each of two picker openings emitted TYPE_VIEW_FOCUSED for the Reactions title. Refreshing the question card emitted its native announcement. This proves dispatch, not TalkBack speech or confirmed focus. CI and the head review are green; the PR body now contains the current evidence and limitations. |
iscekic
added
merge:blocked:insufficient-e2e
The retry case exercised the second attempt but never asserted that a dispatched request seals the presentation. Extend it so a later layout signal cannot ask the platform again.
iscekic
removed
merge:bot
…hrows A native accessibility outage is swallowed by the announce helper, so the hook's cross-render dedupe must keep working after one failed call. Pin it in the mounted suite.
|
bot: The automated review of this head did not complete. Its check run ended with State of this head: every other check passed, the pull request is approved by a human reviewer, it has no merge conflict, and no review thread is open. The same failure happened on the previous head, while reviews of other pull requests were completing, so it looks environmental rather than related to this diff. A work session cannot re-request that check run: the API refuses a re-request from anything but the app that created it. Re-running the review from the app side is what this pull request needs next. |
…s request The helper that runs after a confirmed removal had no direct coverage: the gate for a stale id and the announce-before-focus order were untested.
iscekic
added
the
merge:bot
Evidence
Evidence: demonstrated — at this head the affected suites pass and fail when the behaviours they pin are removed, and an Android emulator run of the real reaction picker shows the renamed helper reaching the native accessibility layer, where the platform may still ignore the request.
What was wrong
truemeant the platform accepted the focus move. The implementation can only know that a mounted host existed and that the native dispatch did not throw, so a caller could read "dispatched" as "focus moved" and skip its fallback.Verification at head
665c090Commands ran on the drain work VM against this checkout.
pnpm exec vitest run src/lib/a11y src/components/pr-review/discussion/reactions-row.test.tsx src/components/pr-review/pr-review-pending-comment-row.test.ts src/components/agents/blocking-card-state-a11y.test.ts→ 9 files, 60 tests passed; the mounted announcement suite alone is 9 tests.attachment-preview-strip,permission-card,question-card,question-card-actions,session-detail-content,session-detail-queue,session-list-content,session-preview-overlay,session-history-screen) → 9 files, 308 tests passed.pnpm --filter kilo-app run typecheck→ clean. Oxlint andoxfmt --list-differenton the changed test → clean.Platform.OS === 'ios'gate from the announcement hook → the mounted suite fails on the Android silence case.expected "vi.fn()" to be called 1 times, but got 2 times.Device run (Android emulator, local stack)
Android Bundled … apps/mobile/index.js (7954 modules).QuestionCardand the real reaction picker. Picker opened twice in one recording session:TYPE_VIEW_FOCUSEDwas emitted.TYPE_VIEW_FOCUSED … ClassName: android.widget.TextView; Text: [Reactions]— the focus request reached the native view.TYPE_ANNOUNCEMENT … Text: [Agent needs your input. The composer is paused while you answer.].ReactNativeJSerror lines during the run.Limitations stated plainly
ReactionsRow's close-then-restore caller is not mounted by the development fixture, so its dispatch and its retry path are proven by its mounted suite, not by the device.Changes
moveA11yFocustorequestA11yFocusthroughout the mobile callers and the test mocks; no compatibility alias remains.truemeans a mounted host existed and the native dispatch did not throw,falsemeans the host was missing or the native call threw and was swallowed. There is no native acceptance signal.status-announcement.mounted.test.tsxin the existing mounted project: iOS announces a newly appearing status once, stays silent for an identical or whitespace-equivalent re-render, announces a changed status once, and stays silent for null, clearing and reappearing; Android stays silent through the same transitions. Keep the pure transition cases. One case covers a native announcement that throws: the helper swallows it and the hook still announces the next status.mainwhile retaining its host-instance accessibility API, and drop obsolete native mocks from the announcement suites.Proxy answers
The owner's proxy gave these answers in the owner's place.
moveA11yFocusreturns true only when a node handle existed andsetAccessibilityFocusdid not throw; the doc says true means the platform accepted the move. Which fix?Answer: Rename to requestA11yFocus + fix docs (Recommended)
Confidence: 0.60 · Precedent: Fix the root cause and the class of the issue: session 00eb2a1d3e4b, "if anything repeats the same mistake over and over again, fix the class". The name change stops future callers from misreading the boolean.
useStatusAnnouncementrender coverage live? The work-item brief namesstatus-announcement.test.ts, but that file runs in themobile-pureproject, which by config comment does no React mounting; mounted suites aresrc/**/*.mounted.test.tsxin themobile-mountedproject (precedent:src/lib/a11y/motion.mounted.test.tsx).Answer: New status-announcement.mounted.test.tsx (Recommended)
Confidence: 0.75 · Precedent: Trust the code over documents: session 269375d6e466, "those documents may be stale, only trust code". The project config puts mounted suites in *.mounted.test.tsx files.
Answer: Do not push a padding commit, and do not just wait. Re-run the failed reviewer job directly with a check rerun or a re-request comment, and keep working while it runs. If it cannot be re-triggered, the pull request already meets human-ready: CI is green, it is approved, there are no conflicts, and no review comments are open. Mark it ready for review and add a note that the reviewer failed with an environment-preparation timeout. Treat the stalled readiness judge as a workflow bug and fix it rather than end on it. Do not merge.
Confidence: 0.50 · Precedent: Never accept blocked as an end state; treat it as a workflow bug and record any real external gap in the pull request. Session 55c9c80332a7: "blocked is not an acceptable end state".
Answer: Land one genuine coverage commit (recommended)
Confidence: 0.72 · Precedent: Never accept "blocked" as an end state (session 2f71af3a787e: "a work item must converge"); do the least work possible with no drop in quality (session 7f3470e0ba6f). The test is real coverage for a caller the brief names, not padding.