Repository navigation
fix(shell): honour reduced motion, announce answers, and test the resize geometry - #91
Merged
Merged
Conversation
The remaining items from the second critique run that did not need a product
decision.
**Renderer coverage for the resize arithmetic.** `ResizableLayout`'s geometry was
inline, so the only way to exercise it was to drag a mouse — and it was wrong in a
way nothing could catch: `ArrowLeft` grew the left panel and shrank the right one,
so the two seams told a screen-reader user the opposite of what they had pressed.
Typecheck passed. Lint passed. The second critique run found it by reading.
Everything pure moved to `components/layouts/panelGeometry.ts`: the constraint
math, the golden-ratio split, the stored-layout parser, and the key-to-width
mapping. `test/panelGeometry.test.ts` covers all of it — including the arrow
direction on both seams, the `Shift` step, `Home`/`End`, the clamps, and the
parser against absent, corrupt, `NaN`, `Infinity` and zero inputs.
I checked that these tests actually fail on the buggy version before keeping them:
reintroducing the inverted flag turns three of them red. One thing that did *not*
catch it, and is worth knowing: the invariant "the two seams move oppositely" holds
when both are flipped together, so only assertions on the absolute direction detect
an inversion.
**The guard's allowlist predicate is tested too**, because CI found that bug and a
local run could not. `isChartSurfaceAllowed` is exported (with an entrypoint guard
so importing does not run the scan) and `test/designGuard.test.ts` asserts both
separator styles — reintroducing the forward-slash-only version turns two tests
red. The same file also asserts `--list` still names every rule, so one cannot be
deleted silently, and that a plain run stays green.
**`prefers-reduced-motion` is honoured.** The app pulses rows and cursors while work
is in flight and animates every popover, and none of it was conditional on the
setting. Transitions and decorative animation now collapse and scroll behaviour
goes instant; spinners keep turning, slowed rather than stopped, because a spinner
is a status indicator and removing it removes information rather than motion. The
`!important` in that block is load-bearing and says so in a comment: a `*` selector
cannot outrank Tailwind's utility classes.
**The transcript announces completion, never tokens.** It previously announced
nothing at all: an answer streamed into a plain div with no `aria-live`, so the
product's entire output arrived silently. A `role="status"` region now reports when
a turn ends, with the node keyed so an identical message re-announces. A live region
on the transcript itself would chatter on every token — worse than the silence it
replaces.
**The tab close control is a sibling of its tab, not a child.** It was a
`role="button" tabIndex={0}` span nested inside a `role="tab"` (itself a `<button>`),
with its own Enter/Space handling duplicating the trigger's. Because the tablist
uses a roving tabindex, that inner `tabIndex={0}` also added a second focus stop per
open notebook. It is now a real `<button>` with an accessible name, positioned
beside the trigger rather than inside it. (A focusable control inside `role="tablist"`
is still not ideal — the fully correct structure needs the tab strip rebuilt so the
close lives outside the list, which is a visual change I could not verify here.)
DESIGN.md records the geometry module and what the tests do and do not catch, the
motion and announcement rules, the tab-strip structure, and the guard's own test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Closes the four items from the second critique run that did not need a product decision: renderer coverage for the resize arithmetic,
prefers-reduced-motion, the missingaria-liveon the transcript, and the nested interactive element in the tab strip.Stacked on #90 — it builds on that branch's code, so the base is
fix/restore-panel-gutter. GitHub retargets it tomainautomatically when #90 merges. Merge #90 first.Why: the resize arithmetic had no way to fail loudly
ResizableLayout's geometry was inline, so the only way to exercise it was to drag a mouse — and it was wrong in a way nothing could catch.ArrowLeftgrew the left panel and shrank the right one, so the two seams told a screen-reader user the opposite of what they had pressed. Typecheck passed. Lint passed. The second critique run found it by reading, which is not a reliable place to catch arithmetic.Everything pure moved to
components/layouts/panelGeometry.ts: the constraint math, the golden-ratio split, the stored-layout parser, and the key-to-width mapping.test/panelGeometry.test.tscovers all of it — the arrow direction on both seams, theShiftstep,Home/End, the clamps at both ends, and the parser against absent, corrupt,NaN,Infinityand zero inputs.I verified the tests fail on the buggy version before keeping them. Reintroducing the inverted flag turns three red. One thing worth knowing, because it is a trap: the invariant "the two seams move oppositely" stays true when both are flipped together, so it cannot detect an inversion. Only assertions on the absolute direction can. That weak test is kept as documentation of intent, not as the guard.
The guard's own predicate is tested, because CI found that bug and a local run could not
isChartSurfaceAllowedis now exported (with an entrypoint guard, so importing the module does not run the scan), andtest/designGuard.test.tsasserts a POSIX path, a Windows path, a relative Windows path, and that other files are still rejected on both platforms. Reintroducing the forward-slash-only version turns two tests red.The same file asserts
--liststill names every rule — so one cannot be deleted silently — and that a plain run stays green.prefers-reduced-motionis honouredThe app pulses rows and cursors while work is in flight and animates every popover and dialog, and none of it was conditional on the user's setting. Transitions and decorative animation now collapse and scroll behaviour goes instant.
Spinners keep turning, slowed rather than stopped: a spinner is a status indicator, not decoration, and collapsing it removes information rather than motion. The
!importantin that block is load-bearing and says so in a comment — a*selector cannot outrank Tailwind's utility classes, which is the whole point of a global preference override.The transcript announces completion, never tokens
It previously announced nothing: an answer streamed into a plain div with no
aria-liveand no busy state, so the product's entire output arrived silently and had to be hunted by cursor. Arole="status"region now reports when a turn ends.It deliberately does not sit on the transcript. A live region there would re-announce on every token — worse than the silence it replaces. The node is keyed so an identical message still re-announces, since setting the same string twice is a no-op for a screen reader.
The tab close control is a sibling of its tab, not a child
It was a
role="button" tabIndex={0}span nested inside arole="tab"— itself a<button>— with its ownEnter/Spacehandling duplicating the trigger's. Because the tablist uses a roving tabindex, that innertabIndex={0}also added a second focus stop per open notebook, so the tab order grew with the number of tabs for no reason.It is now a real
<button>with an accessible name and a focus ring, positioned beside the trigger rather than inside it. The chip still looks the same: the trigger gainedpr-7for the space the close control occupies.Residual, stated rather than hidden: a focusable control inside
role="tablist"is still not ideal — the fully correct structure needs the tab strip rebuilt so the close lives outside the list, which is a purely visual change I could not verify in this environment. Flagging it rather than pretending the structure is now exemplary.How was this tested?
npm test— 73/73 (was 50; 23 added here).npm run typecheck— passes.npm run lint— 0 errors, 112 warnings (unchanged).npm run check:design— no violations.npx electron-vite build— builds.npx prettier --check— clean.impeccable detect --json src—[].Not verified on screen: the reduced-motion rendering, the completion announcement as a screen reader would hear it, and the tab chip after the close control moved. All three are behaviour I can only reason about from here. The geometry and the guard predicate are covered by tests; these are not.
Checklist
npm run typecheckpasses.npm run buildpasses.Desktop / build changes