fix(mobile): name the source-control Back controls and split the dock's Close from Back (OTA phase C, C4.3) (#21739)

* fix(mobile): name the custom-key drawer's Back for the accessibility tree (OTA phase C, C4.3)

`CustomKeyModal`'s Back is a bare `Pressable` with a label and no role, so
a screen reader has nothing to announce it as. It is reachable today from
the session sheets and the terminal shortcut settings, so this is a gap
now, not only inside the page.

It surfaces here because `screenTree` takes a screen's directory: C4.4
registering the review route puts the whole of `src/components` under the
Back rule. Fixing it in the PR that registers would make a route entry
carry unrelated accessibility work, so the census gains a case that holds
the arriving trees to the same two rules before the rows land. Red first
it printed exactly what the rule would:

  src/components/CustomKeyModal.tsx:191 role=none label=Back

Once `PAGE_SERVED_SCREENS` has the two rows, `CONTROLS` covers these trees
and the new case becomes a second reading of the same thing; it says so.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(mobile): split the source-control header's Close from its Back (OTA phase C, C4.3)

One `Pressable` served both modes — `onPress={onBack}` with a conditional
label, `X` docked and `ChevronLeft` full-screen. The Back census reads a
control by what its handler does, so it sees that one as a Back and then
requires a label starting with "Back", in a mode where the control
dismisses the dock beside the terminal. The cheap way to go green is to
call a close "Back", which satisfies the rule by making the wording wrong.

So the modes become two controls. Embedded presses `onClose` and is named
"Close source control"; otherwise it presses `onBack` and is named "Back
to session". Both carry the button role. The panel stops choosing by mode
and passes both handlers; the dock keeps exactly the behaviour it had, its
dismiss still `onRequestClose` falling back to a pop.

Probed before it was written: run through the census's own predicate, the
post-split shape yields one back control rather than two — `onClose`
matches neither the handler pattern nor a declaration this file holds,
being a destructured prop — so the Close is invisible to the rule and the
presence precondition still holds on the Back.

The component test is what the census cannot do. Nothing else pins this:
no golden names this component and no parity census covers
`src/source-control`. Red first against the single control, both cases
failed on the missing role. And renaming the Close to "Back to close
source control" — the gaming this split exists to prevent — reds the
component test while leaving the census green, which is the whole argument
for having both.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* fix(mobile): assert a Back per arriving tree, and name the refresh control (OTA phase C, C4.3 round 1)

The arriving-trees presence case counted controls over the union of both
trees, so one tree answered for the other. Per tree now, in the shape the
block above it already uses per screen module.

Red first, with the mutation round 1 named: rename the route branch's
handler to `onDismiss` and its label to `Return to session`, and the only
Back in `src/source-control` disappears. The per-tree case names that
tree. The same mutation against the union count passes all six cases,
which is what the change is for.

The refresh control had a label and no role, so react-native-web renders
a `div` carrying `aria-label` and a screen reader announces no control.
Its two siblings in this header already carry one.

Two claims in my round-1 report were wrong and I am the reason the body
carried them. The Close pressing `onBack` reds the census's label rule as
well as the component test, not the component test alone; and no fixture
of the post-split shape exists — the shape is read from the real file.
Both were stated from reasoning rather than from a run.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

* test(mobile): assert an arriving Back per screen, not per tree (OTA phase C, C4.3 round 2)

Round 1 moved the arriving-trees presence assertion from the union to
each tree, which was not far enough. `src/components` holds two Backs, so
the tree answers for both: renaming `MobileDiffReviewHeader`'s handler to
`onDismiss` and its label to `Return to session` leaves every case in the
file green, with `CustomKeyModal.tsx:191` standing in for the screen that
just lost its Back. Reproduced before the change — six passed with the
review header's Back gone.

Per screen module now, which is what the table above already does and for
the same reason its docstring gives: a directory with more than one
control cannot say which one a rule was written about. The two modules
are named and the trees derive from them, so the pair C4.4 adds to
`PAGE_SERVED_SCREENS` is the same pair spelled once here.

Both mutations red on the new case and name the file: the review header's
rename names `src/components/MobileDiffReviewHeader.tsx`, and round 1's
source-control rename still names its own, so this does not trade one
cover for another.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Jinwoo Hong
2026-09-19 20:51:47 -04:00
committed by GitHub
co-authored by Claude
parent ee354a35d7
commit 65cde9bb80
5 changed files with 163 additions and 12 deletions
+1
View File
@@ -191,6 +191,7 @@ export function CustomKeyModal({ visible, onClose, onKeysChanged, onManageShortc
<Pressable
style={({ pressed }) => [styles.backButton, pressed && styles.backButtonPressed]}
onPress={onBack}
accessibilityRole="button"
accessibilityLabel="Back"
>
<ChevronLeft size={18} color={colors.textSecondary} />
@@ -204,3 +204,48 @@ describe('Back controls in the screens the page serves', () => {
).toEqual([])
})
})
/**
* The trees a route not yet registered will bring under the rule, held to it before it does.
*
* `screenTree` takes a screen's directory, so registering the review route puts the whole of
* `src/components` under these two rules and the source-control route puts `src/source-control`.
* Fixing that in the PR that registers would make a route entry carry unrelated accessibility
* work; fixing it here means C4.4 adds two rows to the table above and nothing else moves.
*
* This describe is what the rows replace: once they are in `PAGE_SERVED_SCREENS`, `CONTROLS`
* covers these trees and the cases below become a second reading of the same thing.
*
* The modules, with their trees derived, for the reason the table above gives per screen: a tree
* holds more than one Back, so presence asserted over the tree lets one answer for another.
* `src/components` has two, and the review header's could have been renamed into a dismiss with
* `CustomKeyModal`'s standing in for it.
*/
const ARRIVING_SCREENS = [
'src/source-control/MobileSourceControlHeader.tsx',
'src/components/MobileDiffReviewHeader.tsx'
]
const ARRIVING_TREES = [...new Set(ARRIVING_SCREENS.map(screenTree))]
const ARRIVING = ARRIVING_TREES.flatMap((tree) => backControlsUnder(tree))
describe('Back controls in the trees a registered route will add', () => {
it('finds a control in each arriving screen, so the rules below cannot pass vacuously', () => {
expect(ARRIVING_SCREENS.filter((screen) => backControlsIn(screen).length === 0)).toEqual([])
})
it('gives every one of them the button role', () => {
expect(
ARRIVING.filter((control) => !control.role.known || control.role.value !== 'button').map(
describeControl
)
).toEqual([])
})
it('names every one of them in the apps own wording for Back', () => {
expect(
ARRIVING.filter(
(control) => !control.label.known || !/^Back\b/.test(control.label.value)
).map(describeControl)
).toEqual([])
})
})
@@ -0,0 +1,85 @@
import { createElement } from 'react'
import { act, create, type ReactTestRenderer } from 'react-test-renderer'
import { afterEach, describe, expect, it, vi } from 'vitest'
import { MobileSourceControlHeader } from './MobileSourceControlHeader'
vi.mock('react-native', () => ({
Pressable: 'Pressable',
Text: 'Text',
View: 'View'
}))
vi.mock('lucide-react-native', () => ({
ChevronLeft: 'ChevronLeft',
ExternalLink: 'ExternalLink',
RefreshCw: 'RefreshCw',
X: 'X'
}))
vi.mock('./mobile-source-control-styles', () => ({ styles: new Proxy({}, { get: () => ({}) }) }))
/**
* The dock closes and the route goes back, and only one of them is a Back control.
*
* Nothing else pins this: no golden names this component and no parity census covers
* `src/source-control`. The Back census reads what a control is called, so a single control
* serving both modes has to be named "Back" in a mode where it dismisses a dock — which satisfies
* the rule by making the wording wrong.
*/
describe('the source-control header control', () => {
let tree: ReactTestRenderer | null = null
afterEach(() => {
act(() => tree?.unmount())
tree = null
})
function render(embedded: boolean, handlers: { onBack: () => void; onClose: () => void }) {
let rendered: ReactTestRenderer | null = null
act(() => {
rendered = create(
createElement(MobileSourceControlHeader, {
embedded,
worktreeLabel: 'wt',
ioBusy: false,
onRefresh: () => {},
...handlers
})
)
})
if (rendered === null) {
throw new Error('the header did not render')
}
tree = rendered
return rendered
}
const labelled = (rendered: ReactTestRenderer, label: string) =>
rendered.root.findAll((node) => node.props.accessibilityLabel === label)
it('goes back from the route, named as a Back and pressing onBack', () => {
const calls: string[] = []
const rendered = render(false, {
onBack: () => calls.push('back'),
onClose: () => calls.push('close')
})
const control = labelled(rendered, 'Back to session').at(0)
expect(control?.props.accessibilityRole).toBe('button')
act(() => control?.props.onPress())
expect(calls).toEqual(['back'])
expect(labelled(rendered, 'Close source control')).toHaveLength(0)
})
it('closes the dock when embedded, named as a Close and pressing onClose', () => {
const calls: string[] = []
const rendered = render(true, {
onBack: () => calls.push('back'),
onClose: () => calls.push('close')
})
const control = labelled(rendered, 'Close source control').at(0)
expect(control?.props.accessibilityRole).toBe('button')
act(() => control?.props.onPress())
expect(calls).toEqual(['close'])
expect(labelled(rendered, 'Back to session')).toHaveLength(0)
})
})
@@ -7,7 +7,10 @@ type Props = {
embedded: boolean
worktreeLabel: string
ioBusy: boolean
/** Pops the route. The embedded dock does not pop anything, which is why `onClose` is separate. */
onBack: () => void
/** Dismisses the dock beside the terminal, which is a close and must not be called Back. */
onClose: () => void
onRefresh: () => void
// When set (PR segment ready with a host URL), show open-on-web flush-right of
// the title so the control stays visible while the PR body scrolls.
@@ -20,24 +23,36 @@ export function MobileSourceControlHeader({
worktreeLabel,
ioBusy,
onBack,
onClose,
onRefresh,
onOpenPrWeb,
prNumber = null
}: Props) {
return (
<View style={styles.topBar}>
<Pressable
style={({ pressed }) => [styles.backButton, pressed && styles.backButtonPressed]}
onPress={onBack}
hitSlop={8}
accessibilityLabel={embedded ? 'Close source control' : 'Back to session'}
>
{embedded ? (
{/* Two controls, not one with a conditional label: the dock's dismiss is a close, and a
single control serving both modes has to be named Back in a mode where it closes. */}
{embedded ? (
<Pressable
style={({ pressed }) => [styles.backButton, pressed && styles.backButtonPressed]}
onPress={onClose}
hitSlop={8}
accessibilityRole="button"
accessibilityLabel="Close source control"
>
<X size={22} color={colors.textSecondary} strokeWidth={2.2} />
) : (
</Pressable>
) : (
<Pressable
style={({ pressed }) => [styles.backButton, pressed && styles.backButtonPressed]}
onPress={onBack}
hitSlop={8}
accessibilityRole="button"
accessibilityLabel="Back to session"
>
<ChevronLeft size={22} color={colors.textSecondary} strokeWidth={2.2} />
)}
</Pressable>
</Pressable>
)}
<View style={styles.titleBlock}>
<Text style={styles.title} numberOfLines={1}>
Source Control
@@ -70,6 +85,7 @@ export function MobileSourceControlHeader({
onPress={onRefresh}
disabled={ioBusy}
hitSlop={8}
accessibilityRole="button"
accessibilityLabel="Refresh source control"
>
<RefreshCw size={18} color={colors.textSecondary} strokeWidth={2.1} />
@@ -214,8 +214,11 @@ export function MobileSourceControlPanel({
void refetchPr({ includeDetails: false })
}, [activeTab, isHostedRepo, loadStatus, refetchPr])
// Embedded mode docks beside the terminal: close the dock instead of popping a route; skip safe-area chrome (the dock column owns it).
const onBack = embedded ? (onRequestClose ?? (() => router.back())) : () => router.back()
// Embedded mode docks beside the terminal: close the dock instead of popping a route; skip
// safe-area chrome (the dock column owns it). Two handlers rather than one chosen by mode,
// because the header renders a Close or a Back and they are not the same control.
const onBack = () => router.back()
const onClose = onRequestClose ?? (() => router.back())
// Chromeless PR body has no header, so surface open-on-web on the hub chrome while the PR segment is active.
const prWebUrl =
activeTab === 'pr' &&
@@ -231,6 +234,7 @@ export function MobileSourceControlPanel({
worktreeLabel={worktreeLabel}
ioBusy={ioBusy}
onBack={onBack}
onClose={onClose}
onRefresh={onRefresh}
onOpenPrWeb={prWebUrl ? () => openMobilePrUrl(prWebUrl) : undefined}
prNumber={prWebNumber}