feat: improve consistent labels, figure-wide axes, and navigation cues at the multi-panel figure overview

[jooyoungseo]

Pull Request

Description

Turns the multi-panel / facet figure lobby (the Scope.SUBPLOT area you land in before entering a subplot) into a first-class, mode-aware navigation level consistent with an individual subplot.

1. Full L-chord at the lobby — l x / l y / l z / l t / l s / l c all work (previously only t/s/c plus a lone bare t for the title).
2. Figure-wide labels + precedence — an optional figure-level axes lets a facet grid declare one shared X/Y label; announcements prefer the figure-wide value when authored, else fall back to the focused subplot. Same figure→subplot fallback for the title.
3. Description modal — d at the lobby surfaces authored figure-wide axes.
4. Lobby mode toggles — t (Text), s (Sonification), r (Review) work at the lobby; b (Braille) warns that it is unavailable there.
5. Subplot enter/exit cues — entering/leaving a subplot announces a message and plays a distinct rising/falling tone.
6. Mode-aware lobby output — lobby navigation text, the boundary cue, and the enter/exit messages all honor terse / verbose / off.

Changes Made

Keybindings (src/service/keybinding.ts)

• FIGURE_LABEL_KEYMAP: added ANNOUNCE_X/Y/Z.
• SUBPLOT_KEYMAP: removed the direct t title binding (title is l t); added TOGGLE_TEXT (t), TOGGLE_AUDIO (s), TOGGLE_BRAILLE (b), TOGGLE_REVIEW (r).

Schema (src/type/grammar.ts, src/type/state.ts, src/model/plot.ts, src/model/context.ts)

• New optional Maidr.axes?: { x?: AxisConfig; y?: AxisConfig }. Figure parses it; FigureState carries xAxis/yAxis; Context exposes figureXAxis/figureYAxis + isAuthoredAxisLabel, mirroring the figureSubtitle/isAuthoredSubtitle pattern.

Announcements (src/command/describe.ts)

• Shared resolveActiveTraceState() / labelSourcePrefix() / announceAxisLabel() helpers. Lobby precedence: authored figure-wide label wins (Figure X label is Year), else the focused subplot (Subplot 2, X label is …). AnnounceTitleCommand gains the same figure→subplot title fallback.

Description modal (src/service/description.ts): getFigureDescription populates the axes section from authored figure-wide labels.

Lobby interactions, cues & mode-awareness (src/command/move.ts, src/command/toggle.ts, src/command/subplotCue.ts, src/command/factory.ts, src/service/audio.ts, src/service/text.ts, src/service/review.ts, src/state/viewModel/reviewViewModel.ts, src/controller.ts)

• ToggleBrailleCommand warns ("Braille is not available here…" + tone) above the trace level.
• Enter (MoveToTraceContextCommand) / exit (MoveToSubplotContextCommand) play a rising/falling C5-E5-G5 arpeggio (AudioService.playSubplotEnterTone/ExitTone) and announce a mode-aware message via shared subplotCue.ts builders (OFF = tone only, terse = concise, verbose = full). The braille-mode Esc exit (ExitBrailleAndSubplotCommand) now plays the same cue + announcement — previously it dropped back to the lobby silently.
• TextService.formatFigureText is terse-aware (drops "Press ENTER" framing); a new figure out-of-bounds branch announces "No more subplots" (terse) / "No more subplots to display" (verbose) / silence (off).
• Review mode works at the lobby: ReviewService/ReviewViewModel/ToggleReviewCommand accept PlotState, and ReviewService observes the figure/subplots so the review text updates as the user navigates panels.

Docs (docs/SCHEMA.md): documents the top-level figure title/subtitle/caption/axes and lobby precedence.

Backward compatible: absent axes → empty sentinel → falls back to the focused subplot, identical to prior behavior. Inside a subplot / single-panel figures the trace's own axis/title are used unchanged.

Screenshots (if applicable)

Checklist

• I have read the Contributor Guidelines.
• I have performed a self-review of my own code and ensured it follows the project's coding standards.
• I have tested the changes locally, and all tests related to this pull request pass.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation, if applicable.
• I have added appropriate unit tests, if applicable.

Additional Notes

• Unit tests (all new/updated): announce-axis, announce-title, subplot-transitions, context-authored, service/description, service/lobbyBoundary, service/audio.subplotCue, model/figureAxes. Cover the label precedence matrix (X/Y/Z/title × figure-wide/subplot/neither × terse/verbose), the braille warning + braille-exit cue, terse/verbose/off enter/exit + boundary cues, the "no double sound on exit" invariant, and real maidr.axes → Figure.state parsing.
• Verified end-to-end in Chromium on examples/multipanel.html across verbose / terse / off, plus review-at-lobby. Full Jest suite green (921); type-check, ESLint, and production build clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HFJwss8TaoKhFFQRZNbdxU

[claude]

Code Review

Reviewed the diff (src/command/describe.ts, src/service/keybinding.ts, test/command/announce-axis.test.ts) against the stated goal: make the multi-panel figure lobby's l-chord label access (l x/y/z/t/s/c) consistent with an individual subplot.

Summary

This is a clean, well-scoped change that follows MVVC (all changes stay in the Command/Service layer, no model or view changes needed) and KISS. I traced through the logic and it holds up:

• resolveActiveTraceState() correctly walks trace → subplot.trace → figure.subplot.trace, returning null only when a populated trace isn't reachable, which callers already handle via the existing "not available" fallback path.
• labelSourcePrefix() uses FigureState.index, which Figure.state (src/model/plot.ts:211) already computes as a 1-based, visual-order index (currentIndex ?? 1) — so Subplot 2, X label is Month lines up correctly with existing "Subplot N" phrasing elsewhere, no off-by-one.
• The help-menu fix is real, not just cosmetic. generateNestedScopeHelp() (src/service/help.ts) skips a nested-scope entry whenever its command key also exists in the parent keymap, regardless of hotkey. Since SUBPLOT_KEYMAP used to define ANNOUNCE_TITLE directly, FIGURE_LABEL_KEYMAP's ANNOUNCE_TITLE was being silently dropped from the generated help — confirming the PR description's claim about l t being skipped in the auto-generated help. Removing the direct binding fixes this cleanly.
• Confirmed FIGURE_LABEL scope is only reachable from SUBPLOT scope (ACTIVATE_FIGURE_LABEL_SCOPE), which per Context.initializePlotContext/isFigureLevel only happens for multi-panel figures — so the new state.type === 'figure' branches in resolveActiveTraceState/labelSourcePrefix are exercised exactly when intended, and single-panel figures never hit this code path (so the "single-panel" case mentioned in labelSourcePrefix's doc comment is currently unreachable defensive code — harmless, but worth knowing it's not actually exercised today).
• Verified SUBPLOT_KEYMAP and the removed ANNOUNCE_TITLE binding aren't referenced anywhere else (single point of definition), so no other call site silently breaks.

Minor / non-blocking notes

1. Stale comment: the restoreScope() doc comment (describe.ts:57-60) still says "Some announce commands can be invoked from non-label scopes." After this PR, ANNOUNCE_X/Y/Z/TITLE/SUBTITLE/CAPTION are now bound only inside TRACE_LABEL/FIGURE_LABEL keymaps (no more bare t at SUBPLOT), so that guard is effectively unreachable via keybindings today. Not wrong (still correct defensive code, and commands can be constructed/executed directly e.g. in tests), just worth a follow-up comment tweak if you want the doc to reflect current reality.
2. Dead branch: the state.type === 'subplot' case in resolveActiveTraceState() doesn't appear reachable through normal navigation — Context.enterSubplot() always pushes a Trace immediately after a Subplot, so context.active (and thus context.state) never seems to settle on a bare subplot state. It's cheap, harmless, and correctly typed (needed for exhaustiveness over PlotState), so I wouldn't block on it, but flagging in case it signals a gap elsewhere.
3. Test coverage is solid for the new commands (figure-lobby prefix, terse mode, single-panel/trace-level regression, empty/no-z fallbacks). One gap: no test exercises the (currently unreachable) state.type === 'subplot' branch of resolveActiveTraceState(), and no test covers the keybinding/help-menu wiring itself (e.g. that l t now shows up and a bare t no longer does at SUBPLOT scope) — those are arguably covered by the reasoning above, but a help.ts-level assertion would make the "help menu bug" claim regression-proof.

Security / Performance

No concerns — this is pure text-formatting/keymap logic, no new I/O, no user-controlled strings beyond existing authored JSON labels (already trusted elsewhere in the codebase), and no perf-sensitive paths touched.

Nice work — the layered docstrings explaining why (facet vs. general multi-panel, terse mode, single-panel) make this easy to review and will help future readers.

[Copilot]

Pull request overview

This PR aligns multi-panel “figure lobby” label access with subplot behavior by expanding the lobby’s label chord (l …) to include axis labels and by ensuring axis/level announce commands resolve labels from the currently focused subplot’s active trace.

Changes:

• Extended FIGURE_LABEL keybindings so l x / l y / l z work in the figure lobby (mirroring TRACE_LABEL).
• Updated axis/level announce commands to resolve labels from the focused subplot’s trace and optionally prefix announcements with the subplot index in non-terse mode.
• Added unit tests covering lobby behavior for l x / l y / l z, including terse-mode behavior and fallbacks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/service/keybinding.ts	Adds lobby label-scope bindings for X/Y/Z and adjusts subplot-scope description bindings/comments.
src/command/describe.ts	Adds shared trace-resolution logic and subplot-prefixing for axis/level announcements.
test/command/announce-axis.test.ts	Introduces tests validating axis/level announcements at the multi-panel figure lobby.

[github-actions]

🔎 MAIDR example preview is ready

Open the preview: https://pr-639.maidr-preview.pages.dev

• Example gallery: https://pr-639.maidr-preview.pages.dev/examples.html
• Or open any example directly, e.g. https://pr-639.maidr-preview.pages.dev/examples/heatmap.html

Built from fc5117b. This preview URL is stable and updates automatically on every new commit to the PR.

[claude]

Code Review

Reviewed the diff (docs/SCHEMA.md, src/command/describe.ts, src/model/context.ts, src/model/plot.ts, src/service/keybinding.ts, src/type/grammar.ts, src/type/state.ts, plus new/updated tests). Overall this is a clean, well-scoped change that follows MVVC boundaries — describe.ts commands stay in the Command layer, figure-wide axis data flows Model (Figure) → FigureState → Context getters → Command, and no service/UI logic leaked into the model. Nice touches: the DEFAULT_FIGURE_AXIS = '' sentinel choice avoids the 'X'/'Y' collision problem that isAuthoredTitle has to work around, and labelSourcePrefix()/figureWideAxisLabel() are cleanly factored as shared helpers on the abstract AnnounceCommand rather than duplicated per subclass.

I traced through the help.ts nested-scope logic to confirm the stated bug: generateNestedScopeHelp skips any command key that also exists in the parent keymap, so with ANNOUNCE_TITLE present in both SUBPLOT_KEYMAP and FIGURE_LABEL_KEYMAP the nested l t entry was previously being deduped away. Removing it from SUBPLOT_KEYMAP is the correct, minimal fix and I confirmed no other reference to a direct ANNOUNCE_TITLE binding at subplot scope remains (grep across src).

Findings

Test coverage gap for the new figure-wide axes feature (most significant item).
src/model/plot.ts (Figure.xLabel/yLabel parsing from maidr.axes), src/model/context.ts (figureXAxis/figureYAxis/isAuthoredAxisLabel), and src/type/grammar.ts (Maidr.axes) are all new, but test/command/announce-axis.test.ts mocks Context entirely (figureXAxis, isAuthoredAxisLabel are stubbed jest functions). That means the actual JSON → Figure → FigureState → Context wiring for the new axes schema field is never exercised by a unit test — only the command-level precedence logic is. A bug in maidr.axes?.x?.label ?? DEFAULT_FIGURE_AXIS parsing, or in how getState() threads xAxis/yAxis through, would not be caught by the current suite. Given this is a new public schema surface (documented in SCHEMA.md), I'd suggest at least one Figure/Context-level test constructing a real Maidr object with axes: { x: { label: 'Year' } } and asserting context.figureXAxis === 'Year' and isAuthoredAxisLabel behaves correctly on the real (non-mocked) getter.

Minor: resolveActiveTraceState()'s state.type === 'subplot' branch looks unreachable.
Tracing Context's plotContext stack (initializePlotContext, enterSubplot, restoreNavigation), a bare Subplot is never left on top of the stack by itself — it's always pushed together with its active Trace right after, so context.state.type in practice is only ever 'figure' or 'trace' at the point these describe commands run. This mirrors the existing defensive 'subplot' fallback already in AnnounceTitleCommand.execute(), so it's consistent with existing style and harmless, just worth a second look to confirm it's intentional defensive coding rather than a sign the state model was misunderstood.

Minor/behavioral note (by design, not a bug): figure-wide label always wins over per-subplot truth.
figureWideAxisLabel() short-circuits before resolveActiveTraceState(), so once a chart author sets axes.x.label at the figure level, every subplot's l x announces that same figure-wide value even in a non-facet multi-panel figure where panels genuinely have different X axes. This is explicitly documented in SCHEMA.md and is an opt-in authoring choice, so it's fine — just flagging in case it wasn't considered for the "general multi-panel figure with differing axes" case the PR description also discusses.

Keybinding removal (t at SUBPLOT scope) is a user-facing behavior change.
Previously bare t at the multi-panel lobby announced the title; now it does nothing (no TOGGLE_TEXT or other binding fills the slot, unlike TRACE_KEYMAP where t toggles text mode). This is intentional per the PR description and I didn't find any e2e/unit tests asserting the old bare-t lobby behavior that would now fail, but it's worth calling out explicitly as a breaking keybinding change for anyone who had muscle memory around it — might be worth a CHANGELOG note if the project keeps one.

Things I verified directly

• CommandFactory already constructs AnnounceXCommand/AnnounceYCommand/AnnounceZCommand without switching on scope, so no wiring changes were needed there for the new FIGURE_LABEL_KEYMAP entries to work.
• No double restoreScope() calls on any code path in the modified commands.
• FigureState.index is confirmed 1-based (currentIndex ?? 1 in Figure.getState()), consistent with the "Subplot N" phrasing used in prefixes/fallbacks.
• New AxisConfig-based Maidr.axes type is optional and additive; existing charts that omit it fall back to DEFAULT_FIGURE_AXIS (empty string) → isAuthoredAxisLabel returns false → existing subplot-level behavior is preserved, so this looks backward compatible as claimed.

I could not execute the Jest suite / tsc --noEmit in this environment (sandboxed command execution was blocked), so I wasn't able to independently confirm the "882 tests green" and clean type-check claims — the static review above is based on reading the diff and surrounding code only.

🤖 Generated with Claude Code

[claude]

Review: feat(labels) — full L-chord + figure-wide axis labels at the multi-panel figure lobby

I read through the full diff (keybinding, model, command, service, schema, and tests) and traced the logic through Context, Figure, and AnnounceCommand to check the figure→subplot precedence end-to-end. Overall this is a clean, well-scoped change that follows the existing figureSubtitle/isAuthoredSubtitle pattern closely, and it comes with solid unit test coverage for the new precedence rules.

Strengths

• Consistent with MVVC: Figure/Context stay pure data + accessors (figureXAxis/figureYAxis/isAuthoredAxisLabel mirror the existing subtitle/caption getters exactly), and all the new precedence logic lives in the command layer (resolveActiveTraceState, labelSourcePrefix, figureWideAxisLabel in AnnounceCommand). No layer boundaries are crossed.
• Backward compatible by construction: DEFAULT_FIGURE_AXIS = '' as the sentinel, combined with isAuthoredAxisLabel, cleanly falls through to the focused-subplot behavior when axes is omitted — verified this against Figure.state in src/model/plot.ts:89-90 and 220-221.
• The keybinding change is correctly symmetric: SUBPLOT_KEYMAP's bare t is removed entirely (not remapped), which is right since Text mode isn't a lobby-level concept — the lobby now cleanly reaches everything via the l chord, matching TRACE_LABEL_KEYMAP's shape.
• state.type === 'figure' is only ever reached for genuinely multi-panel figures (Context.isFigureLevel gates on size !== 1), so single-panel figures correctly never hit the new lobby precedence paths — confirmed this against initializePlotContext/enterSubplot.
• Good new test coverage in announce-axis.test.ts for the figure-wins vs. subplot-fallback vs. terse-mode vs. empty-trace matrix, plus the title fallback and description-modal axes tests.

Minor nits (non-blocking)

1. Context.isAuthoredAxisLabel (src/model/context.ts:493-495): the label !== DEFAULT_FIGURE_AXIS check is redundant — since DEFAULT_FIGURE_AXIS is '', label.trim() !== '' already implies this. It's harmless (and mirrors the shape of isAuthoredSubtitle/isAuthoredCaption, where the second check is meaningful because those sentinels are 'unavailable', not ''), but could be simplified to just the trim check for this one.
2. AnnounceCommand.resolveActiveTraceState()'s state.type === 'subplot' branch (src/command/describe.ts:90-91) looks unreachable in practice: description.ts's own comment notes that Context.enterSubplot() always pushes a Subplot and Trace together, so context.active (and thus context.state) is never a bare Subplot. Not a bug — it's a reasonable defensive branch consistent with the "Fallback for unexpected state types (e.g. 'subplot')" comment in AnnounceTitleCommand — just flagging that it's currently untested/unreachable dead-ish code, in case it's worth a short comment noting that like the description.ts one does.
3. The new tests mock Context entirely (consistent with the existing convention in announce-metadata.test.ts), so the actual Maidr.axes → Figure.state.xAxis/yAxis → Context.figureXAxis wiring isn't exercised end-to-end by any test — only the command-level precedence logic is. This matches how figureTitle/figureSubtitle are tested elsewhere in the suite, so it's not a regression, just worth knowing the schema-parsing path itself relies on manual/type-check verification rather than a test.

Bugs / correctness

None found. I traced the figure-wins-else-subplot-fallback logic for l x/l y/l z/l t through AnnounceXCommand, AnnounceYCommand, AnnounceZCommand, and AnnounceTitleCommand, and each path calls restoreScope() exactly once regardless of branch taken (I checked this carefully since the title command was restructured with multiple early returns).

Security / Performance

No concerns — this is pure string formatting from typed model state, no user-controlled strings reach the DOM unsanitized (textViewModel.update renders as text, not HTML), and no new loops or expensive operations are introduced.

Test coverage

Good: new/updated tests cover figure-wide-wins vs. subplot-fallback vs. terse mode vs. empty state for X/Y/Z, the title fallback chain at the lobby, and the description-modal figure-wide axes (including the "only one axis authored" case). I'd call coverage solid for this change.

---

Nice work — small, well-tested, and the precedence rules are easy to follow. The two nits above are optional cleanup, not blockers.

---

🤖 Automated review by Claude

[claude]

Code Review

Reviewed the full diff (describe.ts, context.ts, plot.ts, description.ts, keybinding.ts, grammar.ts, state.ts, and the new/updated tests). This is a clean, well-scoped change that closely follows the existing figureSubtitle/isAuthoredSubtitle precedent, and the MVVC boundaries are respected throughout (no service/view logic leaked into Context, no direct Redux access from commands).

Correctness

• Verified the "unreachable" state.type === 'subplot' branch in resolveActiveTraceState() really is dead code: Context.enterSubplot() (src/model/context.ts:588-595) always pushes a Trace immediately after a Subplot, so the stack top is never a bare Subplot. Good call documenting it instead of silently deleting or leaving it unexplained.
• restoreScope() is called on every path in AnnounceXCommand/AnnounceYCommand/AnnounceZCommand/AnnounceTitleCommand, including the new early-return branches — no leaked label-scope state.
• Removing the bare t binding from SUBPLOT_KEYMAP is actually a correctness improvement, not just cosmetic: previously invoking AnnounceTitleCommand via the bare t at the lobby called restoreScope() as a no-op (scope was SUBPLOT, not *_LABEL), so nothing needed to be restored. Now it's only reachable via l t (FIGURE_LABEL scope), where restoreScope() correctly returns to SUBPLOT afterward.
• CommandFactory maps ANNOUNCE_X/ANNOUNCE_Y/ANNOUNCE_Z generically by key name, so wiring the same keys into FIGURE_LABEL_KEYMAP correctly reuses the existing AnnounceXCommand/etc. without any factory changes — confirmed in src/command/factory.ts.
• DEFAULT_FIGURE_AXIS = '' + isAuthoredAxisLabel mirrors the exact shape of isAuthoredSubtitle/isAuthoredCaption, so back-compat (absent axes → empty sentinel → falls back to focused subplot) checks out.

Minor / non-blocking

1. Test coverage asymmetry (test/command/announce-axis.test.ts): AnnounceXCommand gets full coverage (figure-wide win, subplot fallback, terse mode, trace-level, empty-state), but AnnounceYCommand is missing a terse-mode and trace-level case, and AnnounceZCommand is missing both terse-mode and trace-level cases. Given execute() is essentially duplicated per command (same branching structure, not shared beyond the base-class helpers), it'd be worth mirroring the X coverage for Y and Z so a future refactor of the shared helpers is fully guarded.

2. Keybinding comment slightly imprecise (src/service/keybinding.ts:442-444): the comment on SUBPLOT_KEYMAP says "consistent with an individual subplot (where a bare t toggles Text mode)" — but that bare-t-toggles-text behavior lives in TRACE_KEYMAP, not anything defined on SUBPLOT_KEYMAP itself. Reads a little ambiguously out of context; might be worth rewording to "consistent with trace scope, where a bare t toggles Text mode" to avoid confusion for the next person editing this file.

3. resolveActiveTraceState()'s 'subplot' branch: since it's confirmed unreachable, consider whether keeping it is worth the (small) maintenance surface vs. just asserting/throwing there — but the current choice (keep it, document why) is a reasonable and low-risk call given it mirrors the existing AnnounceTitleCommand "subplot" fallback pattern, so not requesting a change.

Architecture / style

• Follows KISS: new helpers (resolveActiveTraceState, labelSourcePrefix, figureWideAxisLabel) are small, single-purpose, and shared via the AnnounceCommand base class rather than duplicated per subclass.
• No any types introduced; NonEmptyTraceState narrowing is used correctly.
• Docs (docs/SCHEMA.md) updated in the same PR, matching the actual announcement wording (spot-checked "Figure X label is ..." / "Subplot N, X label is ..." against the code).

Overall: solid implementation, good precedent-following, and the test additions (888 total per the PR description) give reasonable confidence. The two items above are polish suggestions, not blockers.

[claude]

Review

Went through the diff (describe.ts, context.ts, plot.ts, description.ts, keybinding.ts, grammar.ts, state.ts, and the new/updated tests) plus the commit history showing iterative fixes from prior review rounds. Overall this is a well-scoped, well-tested change that follows the project's MVVC conventions closely.

Code quality / architecture

• Good adherence to MVVC: Figure (Model) parses the new axes field and exposes it via FigureState; Context exposes figureXAxis/figureYAxis/isAuthoredAxisLabel mirroring the existing figureSubtitle/isAuthoredCaption pattern; AnnounceXCommand/AnnounceYCommand/AnnounceZCommand/AnnounceTitleCommand (Command layer) consume it. No layer violations.
• resolveActiveTraceState(), labelSourcePrefix(), and figureWideAxisLabel() are a nice consolidation of logic that was previously duplicated inline across the three announce commands — reduces future drift between X/Y/Z handling.
• The DEFAULT_FIGURE_AXIS = '' sentinel is a sensible choice to distinguish "no figure-wide label authored" from a layer axis's 'X'/'Y' default, and is used consistently in Figure, Context, and DescriptionService.
• The dead-code comment on the state.type === 'subplot' branch in resolveActiveTraceState() (src/command/describe.ts:90-96) checks out — I traced Context.state/plotContext push sites (enterSubplot, lines ~126-127 and ~259-265) and confirmed a bare Subplot is never left on top of the stack; it's always paired with a Trace. Good that this was called out explicitly rather than silently left in as unreachable code.
• Removing the bare t binding from SUBPLOT_KEYMAP (src/service/keybinding.ts) is technically a keybinding behavior change, but it actually brings runtime behavior in line with what docs/CONTROLS.md already documented ("Announce plot title | L, then T"), so this looks like it's fixing a pre-existing docs/behavior mismatch rather than introducing a new one.

Potential issues

• None found that block merging. The AnnounceXCommand/AnnounceYCommand early-return branches both call restoreScope() before returning, matching the non-figure-wide branches that call it at the end of execute() — I double checked this since it'd be an easy spot to forget scope restoration, but it's handled correctly in both commands.
• Minor, non-blocking: isAuthoredAxisLabel/title/subtitle/caption all rely on .trim() on values sourced directly from user-authored JSON with no runtime schema validation. This is a pre-existing pattern (not introduced by this PR), so not a regression, just noting it's the same trust boundary as before.
• Minor test-coverage gap: there's no explicit test for AnnounceTitleCommand's subplot-title fallback in terse mode (only the non-terse case is covered in test/command/announce-title.test.ts). Given announce() already strips the prefix in terse mode and that logic is shared/covered by the X/Y terse tests, risk is low, but it would round out the new coverage.

Test coverage

• Good breadth: test/command/announce-axis.test.ts (new, 354 lines) covers figure-wide-label precedence, subplot fallback, terse mode, trace-level (single-panel) behavior, and the "not available" path for all three of X/Y/Z. test/command/announce-title.test.ts, test/model/context-authored.test.ts, and test/service/description.test.ts were extended consistently.
• Nice touch from the last commit: the trace-level test cases were moved to run under Scope.TRACE_LABEL instead of Scope.FIGURE_LABEL so they reflect realistic usage — shows real review iteration, not just coverage padding.

Security / performance

No concerns — this is pure client-side state/announcement logic with no new I/O, no user-controlled strings reaching innerHTML/eval, and no perf-sensitive hot paths touched.

Docs

docs/SCHEMA.md update for the new top-level axes field is clear and includes a runnable example. Backward compatibility is correctly preserved (optional field, sentinel default, existing single-panel/no-axes charts unaffected).

Nice work — this is a solid, incremental accessibility improvement with real test discipline behind it.

[claude]

Code Review

Reviewed the diff (docs/SCHEMA.md, src/command/describe.ts, src/model/{context,plot}.ts, src/service/{description,keybinding}.ts, src/type/{grammar,state}.ts, plus the new/updated tests). Overall this is clean, well-scoped work that follows the MVVC architecture and the existing figureTitle/isAuthoredTitle pattern closely for the new figure-wide axis concept. A few observations, nothing blocking:

Strengths

• Consistent with existing patterns. figureXAxis/figureYAxis/isAuthoredAxisLabel in Context are a faithful mirror of the pre-existing figureSubtitle/isAuthoredSubtitle pattern, including the "empty sentinel vs. authored" distinction (src/model/context.ts:459-495). DEFAULT_FIGURE_AXIS = '' is a sensible choice since (unlike layer axes, which default to 'X'/'Y') an unauthored figure-wide label should be indistinguishable from "not set."
• Correct precedence scoping. figureWideAxisLabel() (src/command/describe.ts:135-142) only returns a figure-wide value when context.state.type === 'figure', i.e., only at the lobby — inside a subplot or in a single-panel figure the state is never 'figure' (confirmed via Context.isFigureLevel/enterSubplot, which always pairs a Subplot push with a Trace push), so the "existing behavior is untouched elsewhere" claim in the docstrings checks out.
• The "defensive/unreachable" 'subplot' branch claim in resolveActiveTraceState() (src/command/describe.ts:94-96) is accurate — Context.enterSubplot() never leaves a bare Subplot on top of the stack, so this really is dead code kept only for type-narrowing completeness, consistent with the existing AnnounceTitleCommand fallback.
• Test coverage is solid for the new precedence logic: figure-wide-wins vs. subplot-fallback, terse-mode (no prefix), single-panel/trace-level passthrough, and the "not available" degrade path are all covered for X/Y/Z and title. context-authored.test.ts and description.test.ts cover the new Context accessors and the partial-axis (x only, no y) case in the description modal.

Minor / nit-level notes

1. src/service/keybinding.ts:197-199 — the comment on SUBPLOT_KEYMAP says "...consistent with trace scope, where a bare t toggles Text mode," but SUBPLOT_KEYMAP itself doesn't bind t to anything now (unlike TRACE_KEYMAP, which does bind TOGGLE_TEXT to t). The comment reads as if SUBPLOT_KEYMAP also toggles text on t, when actually a bare t at the lobby now does nothing at all (by design — it's only reachable via l t). Slightly misleading phrasing, not a functional issue.
2. Repeated figureWideAxisLabel / early-return pattern in AnnounceXCommand/AnnounceYCommand (src/command/describe.ts:176-199, 234-257) is copy-pasted between the two commands (as flagged in your own commit 839d5355, which added parity tests but didn't dedupe the branching). Given AnnounceCommand already centralizes resolveActiveTraceState/labelSourcePrefix/figureWideAxisLabel, a small shared helper like announceAxis(axis: 'x' | 'y', label: string) in the base class could collapse both execute() bodies to ~5 lines each. Not required now, but worth doing before a third axis-like command needs this shape.
3. getFigureAxes() in description.ts:119-126 builds axes via conditional spreads — correct and consistent with the rest of the file's style, just flagging that it silently omits z at the figure level (expected, since there's no figure-wide Z, but worth a one-line comment for a future reader wondering why axes.z is never set here, similar to the note already present in AnnounceZCommand's docstring).

Bugs / correctness

None found. Traced the restoreScope() call sites in the new/changed branches (X/Y figure-wide early return, X/Y/Z fallback paths, title figure→subplot fallback) and each path calls it exactly once. FigureState.index (the field labelSourcePrefix()/the title fallback read as "Subplot N") is confirmed to be the 1-indexed active subplot position (src/model/plot.ts:223, currentIndex ?? 1), matching the "Subplot 2, ..." test expectations — not the figure's own index, which would have been a subtle off-by-concept bug if swapped.

Security / performance

No concerns — this is pure read-path label resolution and formatting, no new I/O, no user-controlled strings beyond what's already trusted from the authored MAIDR JSON (same trust boundary as existing title/subtitle/caption).

Nice, focused PR — the incremental commit history (adding X/Y, then Z, then the subplot-prefix, then figure-wide precedence, then addressing self-review nits) made this easy to follow.

[claude]

Review: Full L-chord + figure-wide axis labels at the multi-panel lobby

Went through the diff (describe.ts, context.ts, plot.ts, state.ts, description.ts, keybinding.ts, grammar.ts, plus the new/updated tests) against the codebase's current architecture. Overall this is a clean, well-scoped change that follows the existing patterns closely (mirrors figureSubtitle/isAuthoredSubtitle for the new axis accessors, reuses AbstractTrace/Context conventions, and the announceAxisLabel() extraction in the last commit is a nice dedup of what was previously copy-pasted between AnnounceXCommand/AnnounceYCommand).

Correctness

• resolveActiveTraceState() / figureWideAxisLabel() / labelSourcePrefix() type-narrow PlotState correctly (verified against src/type/state.ts's FigureState | SubplotState | TraceState union) — no unsafe casts, and the 'subplot' branch in resolveActiveTraceState is dead at runtime but required for TS exhaustiveness given PlotState, so keeping it is correct rather than needless.
• The new FigureState.xAxis/yAxis fields are only constructed at one production call site (Figure.state getter in src/model/plot.ts), and it's populated correctly there; the outOfBoundsState (empty variant) doesn't need them. No other production code builds a non-empty FigureState, so there's no risk of a stale/missing-field object slipping through.
• Precedence logic (figure-wide label → focused-subplot fallback → "not available") is consistent across AnnounceXCommand/AnnounceYCommand/AnnounceZCommand/AnnounceTitleCommand, and restoreScope() is called exactly once on every path I traced (including the new subplot-title fallback branch in AnnounceTitleCommand).
• Schema addition (Maidr.axes?.{x,y}) is backward compatible: absent axes → DEFAULT_FIGURE_AXIS ('') → isAuthoredAxisLabel returns false → falls back to existing per-subplot behavior, confirmed by context-authored.test.ts.

Minor nits (non-blocking)

1. Stale doc comment on AnnounceCommand.restoreScope() (src/command/describe.ts ~L57-60): after this PR's own keybinding change (removing the bare t binding from SUBPLOT_KEYMAP), every command that calls restoreScope() (AnnounceXCommand/Y/Z/Title/Subtitle/Caption) is now bound exclusively inside TRACE_LABEL_KEYMAP/FIGURE_LABEL_KEYMAP (confirmed via grep — ANNOUNCE_X/Y/Z/TITLE/SUBTITLE/CAPTION no longer appear in any non-label keymap). So the guard's premise ("Some announce commands can be invoked from non-label scopes") no longer describes any live caller in this file. Not a functional bug (the guard is just always-true now), but worth tightening the comment so it doesn't imply a code path that no longer exists — or dropping the conditional if it's confirmed permanently dead.
2. Behavior change worth calling out explicitly: removing the direct t → ANNOUNCE_TITLE binding at the figure lobby (now only reachable via l t) is a deliberate, reasonable consistency fix, but it does change existing keyboard behavior for anyone who's built muscle memory around bare t at the lobby. Given this is an accessibility tool where keybinding stability matters a lot to daily users, might be worth a one-line callout in the release notes beyond just the PR description.
3. Small wording inconsistency (pre-existing, not introduced here): AnnounceZCommand's "not available" message says "Z-axis is not available" while the shared announceAxisLabel() helper for X/Y says "X label is not available" / "Y label is not available". Not part of this PR's scope, just noting it for a future wording-unification pass.

Test coverage

The new test/command/announce-axis.test.ts (354 lines) is thorough — covers figure-wide-wins, subplot-fallback, terse mode, trace-level (single-panel) passthrough, and the empty/unavailable case, mirrored for X, Y, and Z. announce-title.test.ts and description.test.ts additions cover the analogous title-fallback and figure-wide-axes-in-description-modal paths. I didn't spot a gap in the precedence matrix (figure-authored / subplot-fallback / neither-authored × terse/verbose).

Note on verification

I reviewed this statically (couldn't execute npm run type-check/npm test in this sandbox environment). The PR description states 888 Jest tests pass and type-check/ESLint are clean; nothing in the diff contradicts that, but confirming a green CI run before merge is still worthwhile.

Nice work overall — the schema addition is minimal and additive, MVVC boundaries are respected (all new logic lives in Context/Command/Service, nothing reaches into the UI layer), and the fallback chain is easy to follow given the docstrings.

[claude]

Code Review

Reviewed the diff (docs/SCHEMA.md, src/command/describe.ts, src/model/context.ts, src/model/plot.ts, src/service/description.ts, src/service/keybinding.ts, src/type/grammar.ts, src/type/state.ts, plus tests) against MAIDR's MVVC/CLAUDE.md conventions.

Overall

Well-scoped and consistently applied. The new figure-wide axis concept follows the same sentinel + isAuthoredX() pattern already used for title/subtitle/caption (DEFAULT_FIGURE_AXIS = '', Context.isAuthoredAxisLabel), so it reads as a natural extension rather than a bolt-on. The refactor into resolveActiveTraceState() / labelSourcePrefix() / figureWideAxisLabel() / announceAxisLabel() in AnnounceCommand cleanly removes the X/Y duplication that existed pre-PR, and the commit history shows the nits from an internal review pass (comment wording, dedup, Scope.TRACE_LABEL test correction) were already addressed.

Architecture (MVVC)

• Model (Figure/Context) only exposes data + a pure classification helper (isAuthoredAxisLabel); no UI concerns leak in. ✅
• AnnounceCommand subclasses stay in the Command layer and only touch TextViewModel/AudioService/DisplayService — consistent with existing commands in this file. ✅
• notifyStateUpdate() isn't needed here since these are read-only announce commands, not model mutations.

Correctness

• resolveActiveTraceState()'s 'subplot' branch is explicitly documented as unreachable given how Context.plotContext is built (enterSubplot always pairs a pushed Subplot with a Trace on top, so peek() never yields a bare SubplotState). I traced through initializePlotContext/enterSubplot/restoreNavigation and confirmed the stack invariant holds — the comment is accurate, not just asserted.
• labelSourcePrefix()'s Subplot ${state.index} correctly reflects live navigation, since Figure.state.index is recomputed from visualOrderMap on every read rather than cached — so the prefix updates as the user moves between panels in the lobby, as the PR description claims.
• The figure-wide vs. subplot-fallback precedence in announceAxisLabel and AnnounceTitleCommand.execute() is symmetric and matches the documented precedence in docs/SCHEMA.md and the PR description.
• Backward compatibility: an unauthored axes on the JSON produces DEFAULT_FIGURE_AXIS (''), which isAuthoredAxisLabel rejects, so existing charts fall through to the pre-existing subplot-fallback path unchanged. Confirmed by the context-authored.test.ts additions.

Minor observations (non-blocking)

• Context.isAuthoredAxisLabel checks both label.trim() !== '' and label !== DEFAULT_FIGURE_AXIS; since DEFAULT_FIGURE_AXIS is '', the second check is redundant with the first. That said, it mirrors the exact shape of isAuthoredSubtitle/isAuthoredCaption right above it, so keeping it is arguably more consistent with the existing style than trimming it down would be — not requesting a change.
• AnnounceZCommand intentionally isn't routed through announceAxisLabel/figureWideAxisLabel since there's no figure-wide z; that's called out in the docstring, which is helpful for future readers wondering why Z looks different from X/Y.
• getFigureAxes() in description.ts only ever returns x/y — worth double-checking there's no chart type where a figure-wide z might eventually make sense, but nothing in this PR requires it, and the comment already explains the reasoning.

Test coverage

New tests (announce-axis.test.ts, additions to announce-title.test.ts, context-authored.test.ts, description.test.ts) cover: figure-wide-label precedence, subplot fallback with prefix, terse-mode (no prefix), trace-level/single-panel unchanged behavior, empty-state "not available" + warning tone, and the title fallback chain. This is thorough — verbose/terse and figure/subplot/trace permutations are all exercised for X, Y, and Z. I wasn't able to execute npm run type-check / npm test in this sandbox (tool approval required), so I relied on static tracing of the state machine, but the added tests appear to directly exercise the same code paths I traced by hand and are internally consistent with the implementation.

Security / Performance

No concerns — this is pure string/state resolution logic with no new I/O, no any types introduced, and no changes to hot paths (audio/rendering).

Nice work — the commit history itself shows good incremental self-review (dedup pass, comment corrections, added terse/trace-level guards for Y/Z to match X). Nothing here blocks merging.

[claude]

Review: feat(labels) full L-chord + figure-wide axis labels at the multi-panel figure lobby

I reviewed the full diff (docs, schema/types, model/context, commands, services, and the new/updated tests) against main (base e35d0b3). Overall this is a clean, well-scoped extension that follows the existing MVVC/Observer/Command patterns closely and is thorough about backward compatibility. A few notes below — nothing blocking, mostly minor observations and one small consistency nit.

Code quality & architecture

• Nice use of the existing figureSubtitle/isAuthoredSubtitle pattern as a template for figureXAxis/figureYAxis/isAuthoredAxisLabel in src/model/context.ts — keeps the 'authored vs. default sentinel' convention consistent across title/subtitle/caption/axis.
• DEFAULT_FIGURE_AXIS = '' (empty string, vs. 'X'/'Y' for per-layer axes) is a good choice — it avoids the collision risk the code already calls out for DEFAULT_FIGURE_TITLE/DEFAULT_SUBTITLE ('unavailable'/'MAIDR Plot' could theoretically be authored as a real title).
• Collapsing AnnounceXCommand/AnnounceYCommand into the shared announceAxisLabel() helper (commit bb662cf) is a good simplification — avoids duplicating the figure-wide/fallback/terse branching three times.
• The resolveActiveTraceState() 'defensive, unreachable subplot branch' comment checks out: Context.enterSubplot() always pushes [Subplot, Trace] together (model/context.ts:588-598), and initializePlotContext/restoreNavigation never leave a bare Subplot on top of the stack either, so context.state.type === 'subplot' is indeed dead code kept only for type-narrowing completeness. Fine as documented.
• MoveToSubplotContextCommand's comment about the exit-cue/notify race is correct: TextService.update() routes subplot-type state through notification.notify() (not onChangeEmitter), and NotificationService.notify() leads to TextViewModel.notify(), the same notify reducer that the command's own notificationService.notify(buildExitMessage()) call uses. Both dispatches land in the same synchronous call stack, so React 18 batches them and only the last state.message (the exit message) is rendered/announced. No double-announcement bug.
• ToggleBrailleCommand calling audioService.playWarningTone() (not playWarningToneIfEnabled()) means the 'Braille not available' warning tone plays even when audio mode is OFF. This mirrors the existing TextViewModel.warnIfTextOff() pattern (also raw playWarningTone()), so it's consistent with precedent rather than a new bug — worth double-checking that's the intended UX (guidance warnings always audible) rather than an oversight, since most other warnings in this PR correctly use playWarningToneIfEnabled().

Potential issues / things to verify

• Keybinding help text: SUBPLOT_KEYMAP now binds t/s/b to TOGGLE_TEXT/TOGGLE_AUDIO/TOGGLE_BRAILLE (global, scope-agnostic commands), which is a good reuse of the existing scope-agnostic ToggleTextCommand/ToggleAudioCommand. Just confirm the auto-generated help modal at the lobby doesn't end up showing both the mode toggles and the l-chord entry in a confusing order for screen reader users — worth a quick manual VoiceOver/NVDA pass on the help screen for Scope.SUBPLOT, since this PR's manual verification note only mentions Chromium (not screen-reader software) for the enter/exit cues.
• resolveActiveTraceState() for the 'subplot' type: since it's confirmed unreachable, consider whether keeping it adds more confusion than value long-term — but it's clearly commented as defensive/parity with AnnounceTitleCommand, so this is a style preference, not a defect.
• Minor wording asymmetry: AnnounceZCommand's 'not available' message is 'Z-axis is not available' while X/Y use 'X label is not available' / 'Y label is not available'. This is pre-existing (not introduced by this PR) but now that Z is routed through the same labelSourcePrefix() helper as X/Y at the lobby, it might be worth aligning the wording in a follow-up for consistency ('Z label is not available'), though this may be intentionally preserved for backward compatibility with existing screen-reader user expectations/tests.

Test coverage

• Very thorough: test/command/announce-axis.test.ts (new, 354 lines) covers figure-wide-first precedence, subplot fallback with prefix, terse mode, trace-level pass-through, and the 'not available' path for X/Y/Z independently.
• test/command/subplot-transitions.test.ts (new) covers the enter/exit tone + announcement commands and the braille-lobby-warning command, including the 'no-op when not exiting from the lobby' and 'omits plot type when trace is empty' edge cases.
• test/model/context-authored.test.ts and test/service/description.test.ts additions correctly cover the new isAuthoredAxisLabel/figureXAxis/figureYAxis getters and the description modal's getFigureAxes() (both-authored, one-authored cases).
• One gap: I didn't see an end-to-end/integration-level test exercising the actual SCOPED_KEYMAP wiring (i.e., that pressing t/s/b at Scope.SUBPLOT actually resolves to the right commands via KeybindingService/CommandFactory) — the unit tests construct the commands directly. Given CommandFactory wiring was manually checked here and looks correct, this is a nice-to-have rather than a blocker.

Security / performance

• No security concerns — this is all internal state/announcement plumbing, no new external input parsing beyond the already-validated Maidr.axes optional schema field (reuses the existing AxisConfig type).
• No performance concerns — the new getters (figureXAxis, figureYAxis, isAuthoredAxisLabel) are O(1) string reads, and the new audio cues reuse the existing playMenuTone engine with no new allocations of note.

Backward compatibility

• Confirmed: absent maidr.axes leads to DEFAULT_FIGURE_AXIS ('') leads to isAuthoredAxisLabel returning false, which falls back to focused-subplot behavior identical to pre-PR. The SUBPLOT_KEYMAP change removing the bare t to ANNOUNCE_TITLE binding in favor of l t is the one user-facing behavior change — it's well-motivated (consistency with subplot/trace scope) and documented in the PR description and code comments, but is worth calling out explicitly as a breaking keybinding change for existing users who may have muscle-memory around bare t at the lobby.

Nice work overall — the commit history shows good iterative response to self-review (nits, dedup, comment corrections across the later commits), and the final state is consistent and well-tested.

[claude]

Review: feat(lobby) — consistent labels, figure-wide axes, and navigation cues at the multi-panel figure lobby

I read through the full diff (describe.ts, move.ts, toggle.ts, context.ts, plot.ts, audio.ts, description.ts, keybinding.ts, grammar.ts/state.ts, and the new/updated tests) and traced the runtime behavior rather than just the code shape. Overall this is a clean, well-scoped change that follows the MVVC architecture closely — commands stay thin, the new figure-wide-axis concept is threaded consistently through Context getters (mirroring the existing figureSubtitle/isAuthoredSubtitle pattern), and services fire events rather than reaching into Redux/React directly. Test coverage for the new precedence rules (figure-wide vs. subplot fallback, terse mode, empty state) is solid.

A few things worth double-checking, none of which look like blocking bugs:

1. Stale architectural-note comment on MoveToTraceContextCommand (src/command/move.ts:187-196)

The class-level comment says:

"this command holds references to BrailleService and DisplayService (not just Context) as a deliberate exception to the usual pattern... Do not copy this pattern without similar justification."

The class now also takes AudioService and NotificationService, which aren't mentioned. Worth updating the comment (or just dropping the "don't copy this" framing) since injecting AudioService/NotificationService directly into commands is actually the norm elsewhere in this PR (ToggleBrailleCommand, MoveToSubplotContextCommand) and in the existing codebase (ToggleAudioCommand, EnterGridCellCommand), so the note now overstates how exceptional this constructor is.

2. Double-announcement timing on subplot enter (worth a quick manual a11y pass)

MoveToTraceContextCommand.execute() can, in the same call, both (a) fire the "Entered subplot N of M, <type> plot." alert via notificationService.notify(...) and (b) call displayService.toggleFocus(Scope.BRAILLE) when braille was already enabled. That moves DOM focus to the braille textarea in the same synchronous tick as the alert-region update. The reasoning in the exit-path comment (React batching + message overriding value) is sound and I verified it against TextViewModel/Text.tsx — but that reasoning was written for the exit cue where there's no competing focus change. It'd be good to confirm with a screen reader (NVDA/JAWS) that the "Entered subplot..." announcement isn't clipped or reordered by the simultaneous focus move into braille mode, since focus changes trigger their own SR announcements independent of the role="alert" region.

3. Minor redundancy (non-issue, just a style note)

• labelSourcePrefix() re-checks this.textService.isTerse() even though announceAxisLabel()/AnnounceZCommand.execute() only calls it from the already-established "not terse" branch. Harmless, just a bit of duplicated branching.
• isAuthoredAxisLabel checks both label.trim() !== '' and label !== DEFAULT_FIGURE_AXIS, but DEFAULT_FIGURE_AXIS is '', so the second check is subsumed by the first. It mirrors the shape of isAuthoredSubtitle/isAuthoredCaption (where the sentinel isn't empty, so both checks matter there), so keeping the parallel structure is a reasonable readability trade-off — just flagging that it's not doing independent work here.

4. Test coverage gap (minor, already flagged as manually verified in the PR description)

The precedence logic (figure-wide label wins → subplot fallback → "not available") is well covered, but I didn't see a unit test for a general multi-panel figure (no figure-wide axes authored) where two different subplots have genuinely different X/Y labels, confirming the announcement updates as the user moves the cursor between panels. The PR description says this was checked manually in Chromium on examples/multipanel.html, which is reasonable, but a small test asserting resolveActiveTraceState()/announceAxisLabel() picks up the currently focused subplot's trace (not a cached one) would harden this against regressions if Context.state ever gets memoized.

What I verified and didn't flag

• enterSubplot()/exitSubplot() semantics in context.ts match the assumptions the new entry/exit-cue code relies on (entry always succeeds when state.type === 'figure' && !empty; exit is a no-op — correctly detected via the post-call scope === Scope.SUBPLOT check — on single-subplot charts).
• AudioService.playSubplotEnterTone/ExitTone reuse the existing playMenuTone engine and correctly respect AudioMode.OFF, volume 0, and suspended AudioContext, consistent with the existing menu-tone cues.
• The new t/s/b bindings added to SUBPLOT_KEYMAP don't collide with any other key in that scope, and mirror the existing TRACE/GRID_CELL/BRAILLE scope keymaps exactly.
• Figure/Context/DescriptionService figure-axis plumbing (DEFAULT_FIGURE_AXIS, xLabel/yLabel, figureXAxis/figureYAxis, getFigureAxes()) is backward compatible — omitting axes in the JSON falls through to the empty sentinel and existing single-panel/no-figure-axes behavior is untouched.
• AnnounceTitleCommand's figure→subplot fallback correctly narrows state.empty before accessing state.title/state.subplot, so there's no unsound type-narrowing bug there.

Nice work overall — the precedence rules are consistent across l x/l y/l z/l t, the docs (SCHEMA.md) were updated alongside the schema change, and the audio/text/focus interactions were clearly thought through rather than bolted on.

[claude]

Review

I read through the full diff (schema, commands, services, keybindings, and the new test suites) and traced several of the trickier interactions by hand: the label/announce precedence chain, the enter/exit cue + notification ordering, the resolveActiveTraceState/labelSourcePrefix helpers, and the figure-lobby out-of-bounds branch in TextService.update. Overall this is a well-scoped, carefully executed feature — the commit history shows several self-review passes that already caught and fixed real issues (stale comments, missing terse coverage, the braille-exit cue gap), and the new test suites (announce-axis, subplot-transitions, audio.subplotCue, lobbyBoundary, figureAxes) do a good job pinning the exact behaviors described in the PR body.

Note: I wasn't able to run npm run type-check / npm test in this sandbox (npm invocations were blocked by the approval layer), so this is a static read-through, not a verified green build — please make sure CI is fully green before merging.

What looks solid

• Precedence logic (resolveActiveTraceState, figureWideAxisLabel, announceAxisLabel in src/command/describe.ts) is consistent and well-guarded: figure-wide label → focused-subplot fallback → "not available", with terse mode short-circuiting the prefix everywhere it should. The dedup of AnnounceXCommand/AnnounceYCommand into a shared helper is a nice cleanup.
• No double-announcement bug: I traced the MoveToSubplotContextCommand/ExitBrailleAndSubplotCommand exit path through NotificationService → TextViewModel → Text.tsx. update() sets value and clears message; the later notify() only sets message (leaving value alone), and Text.tsx prefers message when present. Since both dispatches happen synchronously before React flushes, the user gets exactly one alert (the exit message) plus the exit tone — the "no double sound on exit" test correctly encodes this, and it matches actual UI behavior.
• Backward compatibility: DEFAULT_FIGURE_AXIS = '' as a distinct sentinel from the 'X'/'Y' layer-axis defaults is the right call — it lets isAuthoredAxisLabel cleanly distinguish "not authored" from an authored empty string, and unauthored axes fields fall through to existing single-panel/subplot behavior unchanged.
• Keybindings: checked SUBPLOT_KEYMAP for collisions after adding t/s/b/r — no conflicts with existing bindings (c/m/p/d/l/arrows/enter all remain unique).
• Gating ReviewService/TextService/AudioService behavior on state.type === 'figure' (rather than trying to guard on 'subplot', which per the code comments is unreachable at runtime since Context never holds a bare Subplot without a Trace on top) is consistent with how the rest of the codebase already treats that invariant.

Minor / non-blocking observations

1. AnnounceCommand.resolveActiveTraceState (src/command/describe.ts) keeps a state.type === 'subplot' branch that's documented as defensively unreachable (mirroring AnnounceTitleCommand's existing pattern). It's harmless and well-commented, but it's the second copy of "dead code kept for symmetry" in this file — if a third one shows up, it might be worth a shared type guard instead of repeating the justification comment.
2. ToggleReviewCommand.execute() now unconditionally calls reviewViewModel.toggle(this.context.state) without the old state.type === 'trace' guard. This is intentional (review should work at the lobby too) and ReviewService.toggle/update already handle non-trace PlotState via TextService.format(), so it's not a bug — just flagging that the removed guard slightly widens the surface area of what ReviewService needs to keep handling correctly if new PlotState variants are ever added.
3. subplotCue.ts's subplotEntryMessage/subplotExitMessage take a TextService purely to read isOff()/isTerse(). Small, but since these are pure functions given the mode, an alternative would be to pass the mode enum directly rather than the whole service — purely a style nit, not worth blocking on.
4. Docs (SCHEMA.md) correctly call out that figure-level axes only supports x/y (no z, since Z is inherently per-trace) — good, this matches the implementation in Figure/grammar.ts.

Test coverage

Coverage looks thorough for the new behavior: label precedence matrix (figure-wide vs. subplot vs. absent, × terse/verbose), the braille "not available" warning, enter/exit cues across text modes (including the OFF-plays-tone-only and braille-suppresses-alert cases), the figure-lobby boundary cue, and real maidr.axes → Figure.state parsing (as opposed to only mocking Context). I didn't find gaps in the described behavior matrix.

No blocking issues found. Nice work on this one — the iterative self-review commits (fixing the stale architectural comment, adding missing terse/off cases, fixing the braille-exit cue silence) suggest the diff already had a solid pass before this review.

[claude]

Review of PR #639

I reviewed the diff, checked the new precedence/fallback logic, keybinding wiring, and audio cue timing, and spot-verified a couple of claims against the source.

Summary
This PR promotes the multi-panel figure "lobby" to a mode-aware navigation level. The implementation is careful and architecturally clean: business logic stays in commands/services, Observer/Emitter boundaries are respected (MVVC), and the new subplotCue.ts message builders are pure functions. Command wiring in factory.ts is complete, dependency injection is consistent, and no any types were introduced.

Strengths

• Precedence logic is correct and thoroughly tested. describe.ts announceAxisLabel cleanly implements figure-wide -> focused-subplot -> "not available", and announce-axis.test.ts covers all branches including per-panel re-read and blank-label rejection.
• Mode-awareness is consistent across OFF/TERSE/VERBOSE in subplotCue.ts, the lobby boundary cue in text.ts, and the announce commands, with matching tests (lobbyBoundary.test.ts, subplot-transitions.test.ts).
• Enter/exit cue design avoids a data-tone trap. move.ts deliberately skips notifyStateUpdate() on entry (documented inline) to avoid a spurious data tone, playing a distinct navigational arpeggio instead.
• Defensive guards against malformed states (subplotCue.ts, text.ts, describe.ts) are reasonable and documented.

Issues

1. Duplicated "is authored title" logic. TextService.isAuthoredTitle (src/service/text.ts) reimplements Context.isAuthoredTitle (src/model/context.ts), and focusedSubplotTitle (src/command/subplotCue.ts) vs lobbySubplotTitle (src/service/text.ts) encode the same "focused subplot title" resolution twice. Three copies of the placeholder-rejection rule risk drifting if a new default sentinel is introduced later. Consider centralizing this in Context or a shared util and having TextService/subplotCue.ts delegate to it.

2. First enter/exit cue can be silently dropped when the AudioContext is still suspended. playMenuTone (src/service/audio.ts, gated at the 'state !== running' check) and the subplot enter/exit cues short-circuit while the AudioContext is suspended, and there is no resume() call anywhere in src/. If a user focuses the plot and immediately triggers subplot entry before any interaction resumes the context, the enter arpeggio is skipped while the subplot still activates -- silently. By contrast, playOscillator (the data-tone path) has no such guard. This is a minor/edge-case UX inconsistency worth a follow-up, since MAIDR is an accessibility tool and a silently-dropped cue is exactly the kind of gap that matters for screen-reader/blind users relying on audio feedback.

3. Lobby s/c semantics changed (intended per the PR description, flagging for awareness): previously bare s/c at the lobby announced subtitle/caption; now s toggles Sonification and c toggles High Contrast, with subtitle/caption moved under the l chord. This is a muscle-memory break for existing users -- worth calling out explicitly in release notes/changelog.

Test coverage gaps
Coverage is strong overall (precedence, per-panel, terse/verbose/off, enter/exit, braille-unavailable warning, braille-escape exit, no-op single-subplot). A few remaining gaps:

• No test for the volume <= 0 guard in playMenuTone for the subplot cues.
• No test for a suspended AudioContext (state !== running) -- the exact edge in Issue feat: add new architecture #2 above is unverified.
• No test for a figure-wide axis authored as a whitespace/empty string falling back to the subplot (the blank-rejection path in Context.isAuthoredAxisLabel); figureAxes.test.ts covers omitted axes but not an authored-but-blank axes.x.label: "".
• Confirm description.test.ts exercises getFigureDescription with authored figure-wide axes for the d modal at the lobby (description.ts getFigureAxes).

Nitpicks / style

• move.ts (MoveToTraceContextCommand.execute): the inline plot-type/title extraction could be lifted into a small private helper to match the pure-function style used in subplotCue.ts.
• describe.ts's labelSourcePrefix returns '' for both terse mode and non-figure states via two separate code paths -- a short comment noting both collapse to the empty prefix would help future readers.
• No security concerns: all announced text renders through React text nodes, not innerHTML. Audio/text paths are O(1) per keypress, so no performance concerns.

Nice work overall -- the precedence and mode-awareness logic is well thought out and well tested; the two flagged issues are minor/edge-case rather than blocking.

[claude]

Review: Multi-panel figure lobby — labels, figure-wide axes, and navigation cues

Solid, well-scoped feature PR. It extends the Scope.SUBPLOT "lobby" to be a first-class navigation level (full L-chord, figure-wide axes with subplot fallback, mode-aware enter/exit cues), and it does so with real discipline: MVVC boundaries are respected, the existing MoveToTraceContextCommand "don't use notifyStateUpdate(), it would double-fire audio" pattern is extended consistently rather than worked around, and the placeholder-rejection logic (isAuthoredTitle) was correctly pulled up into plot.ts as a single source of truth instead of being duplicated. Test coverage is genuinely thorough (new suites for the label precedence matrix, boundary cues, and the "no double sound on exit" invariant).

A few things worth a look:

1. Verbose enter/exit cues never speak the subplot title (possible inconsistency)

src/command/subplotCue.ts — subplotEntryMessage / subplotExitMessage both accept a title parameter, but it's only used in the TERSE branch:

if (text.isTerse()) {
  return title ? `Subplot ${index}, ${title}` : `Subplot ${index}`;
}
const suffix = plotType ? `, ${plotType} plot` : '';
return `Entered subplot ${index} of ${size}${suffix}.`;   // title dropped

So a facet panel titled "Sales in North" is announced by name in terse mode, but in verbose mode (arguably the mode that should carry the most information) the user only hears "Entered subplot 2 of 4, bar plot." — never the title, even though it was authored and is available at the call site. Since formatFigureText's existing verbose branch has the same asymmetry, this may well be intentional (verbose narrates structure, terse identifies by name, and l t is still available on demand) — but given the PR's own framing ("VERBOSE: the full transition"), it reads like an oversight rather than a deliberate choice. Worth a quick confirmation either way.

2. esc,backspace binding looks like scope creep

src/service/keybinding.ts:

- MOVE_TO_SUBPLOT_CONTEXT: key(`esc`, 'Return to Subplot', { showInHelp: false }),
+ MOVE_TO_SUBPLOT_CONTEXT: key(`esc,backspace`, 'Return to Subplot', { showInHelp: false }),

This adds Backspace as a global alias for "exit to subplot/lobby" in TRACE scope. It's gated by hotkeys.filter so it won't fire inside real <input>/<textarea> fields (other than the braille textarea/review input, which are allow-listed), so it's probably safe — but it's unrelated to the "lobby labels/axes/cues" scope described in the PR title and isn't mentioned in the description. Worth calling out explicitly (or splitting out) so reviewers don't miss that a new global hotkey was added.

3. Figure-level axes reuses the full per-layer AxisConfig type

src/type/grammar.ts:

axes?: { x?: AxisConfig; y?: AxisConfig };

AxisConfig also carries min/max/tickStep/format, none of which are read at the figure level (only .label is, per Figure's constructor and docs/SCHEMA.md). A chart author could reasonably set maidr.axes.x.min expecting it to do something and have it silently ignored. Not a bug, but consider a narrower type (e.g. Pick<AxisConfig, 'label'>) so the schema itself documents what's actually honored, rather than relying solely on the prose in SCHEMA.md.

4. Test-helper duplication

Several new/modified test files (announce-axis.test.ts, braille-escape.test.ts, scopeRestore.test.ts, subplot-transitions.test.ts, …) each hand-roll near-identical createMockAudioService / createMockNotificationService / createMockTextService helpers. Given the PR touches so many command tests with the same new constructor dependencies, factoring these into a shared test/helpers module would cut duplication and reduce the risk of the mocks drifting out of sync with the real interfaces over time.

Correctness spot-checks (no issues found)

• resolveActiveTraceState()'s 'subplot' branch is genuinely unreachable given Context.enterSubplot/exitSubplot/restoreNavigation always pair a Subplot with a Trace on the stack — the "defensive, kept for completeness" comment is accurate.
• AudioService.update() already short-circuits non-'trace' states (if (state.type !== 'trace') return;), so exitSubplot()'s notifyStateUpdate() re-firing with a non-empty figure state genuinely cannot double up with the new playSubplotExitTone() call — confirmed by the new audio.subplotCue.test.ts case.
• TextService.update()'s new figure-level out-of-bounds branch mirrors the existing trace-level branch's early-return/bookkeeping-skip behavior correctly.
• Schema/backward-compat: maidr.axes omitted → DEFAULT_FIGURE_AXIS ('') → isAuthoredAxisLabel false → falls back to focused subplot, so existing charts are unaffected — matches the PR's stated backward-compatibility claim.

Other notes

• No security concerns — this is pure client-side UX/schema surface, all new fields are optional and defaulted, no new external input parsing.
• Performance impact is negligible (a few extra string formats and short oscillator bursts per navigation action).
• I could not execute npm test / npm run build in this review environment (no node_modules, and network/git operations were sandboxed), so I've verified correctness by static reading against the actual post-merge source rather than by running the suite — please make sure CI is green before merging.

Nice work overall — the architecture-doc conventions (Observer wiring in controller.ts, Command DI, Emitter-based cues) are followed carefully throughout.

[claude]

Code Review

I read through the full diff (schema, model, commands, services, keybindings, and the new tests) and traced the key control-flow paths (Context.enterSubplot/exitSubplot, AudioService.update, TextService.update, ReviewService, keymap wiring). Overall this is a well-scoped, carefully documented feature. Architecture-wise it follows MVVC correctly: model gains a new optional field, commands stay thin, services own the behavior, and the new lobby cues are properly mode-aware (OFF/TERSE/VERBOSE). Test coverage for the new behavior is thorough (new dedicated test files for axis announce, subplot transitions, figure-wide axes parsing, lobby text/boundary cues, and audio cue oscillator counts).

A few things worth a look:

Minor / worth confirming

1. esc,backspace binding for MOVE_TO_SUBPLOT_CONTEXT (src/service/keybinding.ts:306) — adding backspace as an alternate "return to subplot" key in TRACE_KEYMAP isn't mentioned anywhere in the PR description, which is otherwise entirely about the figure lobby. Is this intentional scope creep bundled into this PR, or a leftover from testing? If intentional, it'd be good to call it out explicitly (and maybe add a one-line comment on why backspace was chosen, since it's a value judgment — some screen-reader/braille-display users may expect backspace to behave like a text-delete key rather than a navigation "back" key).

2. MoveToTraceContextCommand/MoveToSubplotContextCommand/ExitBrailleAndSubplotCommand now take 6-7 constructor args. These commands already had a documented "architectural exception" for holding service references instead of just Context; this PR adds three more (AudioService, NotificationService, TextService) to each. That's consistent with the existing exception and well-commented, but the constructors are getting long — if more cues get added to these transition points in the future, it might be worth consolidating the "announce + play tone" pattern (subplotEntryMessage/subplotExitMessage + notify + playSubplotEnter/ExitTone) into a small helper/service method so each call site doesn't need to keep threading the same 3 dependencies through constructors. Not a blocker, just a note for maintainability if this pattern grows further.

3. resolveActiveTraceState()'s 'subplot' branch is documented as unreachable (src/command/describe.ts:92-98), based on the invariant that Context never has a bare Subplot on the stack without a Trace on top. That invariant is verified correct by reading Context.enterSubplot(), but since it's load-bearing for a "kept for completeness" dead branch, consider a lightweight runtime assertion or a unit test pinning that invariant directly on Context, so a future refactor of enterSubplot/exitSubplot that breaks it would fail loudly instead of silently degrading label announcements.

4. DEFAULT_FIGURE_AXIS = '' is a slightly unusual sentinel choice (vs. the 'X'/'Y' layer-axis defaults) — it's clearly explained in the doc comment and covered by tests (including the "authored-but-blank collapses to sentinel" case), so this is fine, just flagging that it's a different convention from the layer-level default and could bite a future contributor who copies the wrong pattern. Might be worth a one-line cross-reference comment near the layer-level AxisConfig default handling pointing at DEFAULT_FIGURE_AXIS's different convention.

Things that look correct (verified, not just read)

• Context.enterSubplot() never calls notifyStateUpdate(), so AudioService/TextService don't get double-fired on entry — the commands' manual playSubplotEnterTone() + notificationService.notify(...) correctly compensate, and exitSubplot()'s notifyStateUpdate() on the figure state is confirmed (via test/service/audio.subplotCue.test.ts) to not trigger a data tone, avoiding the "double sound on exit" the tests explicitly guard against.
• state.type === 'figure' is only reachable for genuine multi-panel figures (Context.isFigureLevel gates this at construction), so labelSourcePrefix()/figureWideAxisLabel()'s single-panel fallthrough branches are correctly unreachable dead paths, not bugs.
• Keymap additions (TOGGLE_TEXT/TOGGLE_AUDIO/TOGGLE_BRAILLE/TOGGLE_REVIEW in SUBPLOT_KEYMAP, ANNOUNCE_X/Y/Z in FIGURE_LABEL_KEYMAP) don't collide with existing bindings in their respective scopes, and route through the same shared command classes used at trace level, so behavior stays consistent by construction rather than by duplicated logic.
• ReviewService/Controller.registerObservers() correctly widen from Observer<TraceState> to Observer<PlotState> and get registered on figure/subplot in addition to trace, matching TextService.format()'s existing figure/subplot branches.
• The figure-wide axes schema addition is properly narrowed to Pick<AxisConfig, 'label'> at the type level, correctly documented in docs/SCHEMA.md, and backward compatible (omitting axes preserves existing behavior, verified by test/model/figureAxes.test.ts).

Test coverage

Coverage is strong — new/updated tests exercise: figure-wide axis precedence and fallback, terse/verbose wording variants, entry/exit cue tone + message pairing (including the OFF-mode silent case and the braille-focus-suppresses-alert case), out-of-bounds lobby navigation cues, and Figure JSON parsing of the new axes field. I didn't spot an obvious gap, though I'd suggest one addition: a test pinning the "no bare Subplot on the context stack" invariant mentioned above, since two commands' correctness implicitly depends on it.

No security concerns (no new user input parsing beyond typed JSON schema fields, no DOM injection, no new external calls). No obvious performance concerns — the new logic is all O(1) state reads on user-triggered key events.

I wasn't able to run npm run type-check / npm test in this sandboxed review environment, so please make sure CI is green before merging.

[claude]

Review

I read through the full diff plus the surrounding source (describe.ts, move.ts, context.ts, plot.ts, text.ts, audio.ts, keybinding.ts, review.ts) to check the new logic against the existing invariants, not just the hunks in isolation.

Summary

This turns the multi-panel figure lobby into a mode-aware navigation level (full l-chord, figure-wide axes, mode toggles, enter/exit cues). The implementation is careful about MVVC boundaries and about why it deviates from the observer-fan-out pattern (e.g. MoveToTraceContextCommand/MoveToSubplotContextCommand explicitly injecting AudioService/NotificationService instead of relying on notifyStateUpdate(), with a doc comment explaining that a full notify would also fire the data tone). That tradeoff is well-justified and consistent with the existing precedent noted in PATTERNS.md.

Traced end-to-end and confirmed correct:

• No double audio tone on exit: AudioService.update() returns early for non-empty non-'trace' states, so exitSubplot()'s notifyStateUpdate() fan-out can't also fire a data tone alongside the new exit cue (covered by audio.subplotCue.test.ts).
• Message vs. value precedence: MoveToSubplotContextCommand calls exitSubplot() (which synchronously clears state.message via the observer chain) before calling notificationService.notify(...), so the exit message correctly wins in the role="alert" region and the visible <p> (Text.tsx prefers message over value in both). The ordering comment in move.ts matches what the code actually does.
• Context stack invariant: the new contextStackInvariant.test.ts pins that enterSubplot()/exitSubplot() always push/pop Subplot+Trace together, which is what lets resolveActiveTraceState()'s 'subplot' branch stay "defensively unreachable" — good to have this locked down given several new code paths lean on it.
• Keymap: no key collisions within SUBPLOT_KEYMAP/FIGURE_LABEL_KEYMAP, and the new backspace binding in TRACE_KEYMAP is scoped safely (never registered for BRAILLE/REVIEW scopes, and hotkeys.filter already blocks it inside real <input>/<textarea> elements).

Minor findings (non-blocking)

1. Duplicated "focused subplot title" logic — src/command/describe.ts, AnnounceTitleCommand.execute():

   const subplotTitle = !state.subplot.empty && !state.subplot.trace.empty
     ? state.subplot.trace.title
     : '';
   if (this.context.isAuthoredTitle(subplotTitle)) { ... }

   This re-implements exactly what focusedSubplotTitle(state) (new in model/plot.ts) already does, and which move.ts and text.ts both call for the same purpose. Worth reusing here too so the traversal + placeholder-rejection rule has a single call site, per the "single source of truth" rationale already used for isAuthoredTitle.

2. Edge-case asymmetry in the lobby-entry gate — src/command/move.ts, MoveToTraceContextCommand.execute():

   const lobby = before.type === 'figure' && !before.empty ? before : null;
   this.context.enterSubplot();

   Context.enterSubplot() only checks activeState.type === 'figure' (not emptiness), while the new cue/announcement gate additionally requires !before.empty. If a figure state were ever empty: true while type is still 'figure', enterSubplot() would still perform the transition (push subplot+trace, switch scope) but silently skip the enter tone and announcement. This is likely unreachable in practice (a constructed Figure always has ≥1 subplot), so low severity — just flagging the inconsistency in case an empty-figure state becomes reachable later (e.g. via replaceFigure on a live/streaming chart).

3. Schema nit: Maidr.axes (figure-wide) and MaidrLayer.axes (per-layer) reuse the same JSON key name at different nesting depths. The docs/SCHEMA.md update does disambiguate with examples, so this is just a heads-up for potential author confusion, not a code issue.

Test coverage

Strong — new tests cover the figure-lobby precedence for l x/l y/l z/l t (including terse/verbose and re-focus-tracking across panels), the enter/exit cue + announcement text in all three text modes (off/terse/verbose), the "no bare Subplot on the stack" invariant, figure-axes JSON parsing (including blank-string collapsing to the sentinel), and the audio oscillator-count assertions for the new 3-note cues. I didn't find gaps.

Security / performance

No concerns — this is UI/navigation/announcement logic with no new external input parsing beyond the already-validated Maidr.axes schema field, and no hot-path changes.

Nice work — the comments throughout (especially the "why not use notifyStateUpdate()" and the message-ordering rationale) will save the next person a debugging session.

[claude]

Review: Multi-panel figure lobby as a first-class navigation level

I reviewed this PR by reading the full diff plus the surrounding files in context (context.ts, move.ts, describe.ts, text.ts, review.ts, keybinding.ts, Text.tsx, textViewModel.ts) to check the trickier interactions (observer wiring, scope restoration, and the exit-cue/announcement ordering).

Overall

This is a well-scoped, well-tested change. It follows the MVVC architecture correctly (Model → Observer → Service → ViewModel → Redux → View), the schema addition (Maidr.axes) is backward compatible and narrowed to Pick<AxisConfig, 'label'> so authors can't accidentally rely on figure-wide min/max/tickStep/format, and the docstrings do a good job explaining why (e.g. the "no bare Subplot on the context stack" invariant, which is even pinned by a dedicated test in contextStackInvariant.test.ts). Test coverage is thorough — new suites cover the announce commands, subplot enter/exit cues in all three text modes, figure-axes parsing, lobby boundary text, and audio oscillator counts.

A few things I specifically checked and confirmed are not bugs, since they looked suspicious at first glance:

• MoveToSubplotContextCommand/ExitBrailleAndSubplotCommand call context.exitSubplot() (which synchronously fires TextViewModel.update() + clearMessage() via the observer chain) and then call notificationService.notify(exitMessage) afterward. Since Redux dispatch is synchronous and React 18 batches all of it into one render, and Text.tsx renders a single alert region that prefers message over value, the user hears exactly one exit announcement, not two. This relies on dispatch order rather than "React batching" per se, but it works correctly today.
• AudioService.update() correctly no-ops on a figure-type state, so the re-notifyStateUpdate() from exitSubplot() doesn't double-fire a data tone alongside the new exit arpeggio (covered by audio.subplotCue.test.ts).
• ReviewService/AudioService/TextService were already wired to figure/subplot-level observers (or trivially extended), so widening ReviewService from Observer<TraceState> to Observer<PlotState> doesn't introduce duplicate registrations.

Minor suggestions (non-blocking)

1. Constructor parameter growth. MoveToTraceContextCommand, MoveToSubplotContextCommand, and ExitBrailleAndSubplotCommand now each take 6-7 constructor parameters, having grown by AudioService + NotificationService + TextService in this PR alone. CommandFactory absorbs the wiring so call sites stay clean, but if another cross-cutting concern needs to be threaded through these subplot-transition commands later, it might be worth grouping the "announce this transition" dependencies (audioService, notificationService, textService) into a small shared collaborator rather than continuing to grow the parameter list 1-by-1.

2. Message-shaping logic is duplicated three ways. The "prefer the authored subplot title, else fall back to a position-based phrase, with different wording in terse vs. verbose" pattern is implemented separately in subplotCue.ts (subplotEntryMessage/subplotExitMessage) and again in TextService.formatFigureText. The exact phrasing differs enough (entry/exit cues vs. lobby nav text) that a shared helper might not be a clean fit, but it's worth a second look if a future change needs to tweak this phrasing consistently across all three call sites.

3. ToggleBrailleCommand's "not available" warning isn't mode-aware, unlike the other new lobby messages (entry/exit cues, axis/title announcements) which all branch on textService.isTerse()/isOff(). This matches the pre-existing convention for other static notificationService.notify(...) warnings elsewhere in the codebase (e.g. "No info for review"), so it's consistent rather than a bug, but since the PR description specifically calls out "mode-aware lobby output" as a goal, it might be worth a quick pass to confirm this was an intentional omission for this particular message.

Housekeeping

I wasn't able to run npm run type-check / npm test in this sandboxed environment (no node_modules, no network access), so this review is based on static reading of the full diff and the surrounding source files rather than an executed test run. Given the size and care of the added test suite, I'd still recommend confirming CI is green before merge, particularly for the new test/model/contextStackInvariant.test.ts and test/service/audio.subplotCue.test.ts.

No blocking issues found — nice work on keeping the announcement logic mode-aware and on documenting the trickier invariants inline.

[claude]

Review Summary

This is a well-scoped, thoroughly-tested feature PR that promotes the multi-panel figure lobby (Scope.SUBPLOT) to a first-class, mode-aware navigation level. I read through the full diff (docs, keybindings, schema, commands, services, model) and the ~15 new/updated test files. Overall the change follows MVVC cleanly, and the test coverage for the new behavior (entry/exit cues, terse/verbose/off wording, figure-wide axis precedence, the 'no bare Subplot on the context stack' invariant) is unusually thorough for a UX-feedback feature. A few observations, none blocking:

Architecture / design

• SubplotCue collaborator (src/command/subplotCue.ts) is a nice example of the Command layer's documented exception (holding Audio/Notification/Text services directly instead of only Context) - and it's justified with the same reasoning already established for MoveToTraceContextCommand. Good adherence to the 'don't copy without justification' note in PATTERNS.md.
• Single source of truth for placeholder rejection: moving isAuthoredTitle and adding focusedSubplotTitle into src/model/plot.ts, with Context.isAuthoredTitle delegating to it, is a solid refactor that removes the previous duplicated logic between Context and its callers.
• Ordering-dependent UX in MoveToSubplotContextCommand / ExitBrailleAndSubplotCommand: the exit cue relies on TextViewModel.update() clearing message before SubplotCue.announceExit() sets it again in the same synchronous call stack, so the spoken exit message wins over the re-announced nav text. This is explained in the comments and covered by tests, but is inherently fragile if the observer/dispatch order ever changes (e.g. if notifyStateUpdate() becomes async, or another observer is inserted between them). Worth a quick regression check whenever Context.exitSubplot()'s observer wiring changes.

Correctness

• No logic errors found in the new precedence chains (figure-wide axis vs. per-subplot fallback, figure title vs. subplot title fallback, terse/verbose/off cue wording) - they match their respective test suites exactly, including edge cases like blank/whitespace authored labels and the defensively-unreachable 'subplot' branch (now pinned by test/model/contextStackInvariant.test.ts).
• ToggleBrailleCommand and ToggleReviewCommand now operate above trace level; ReviewService/ReviewViewModel were correctly widened from Observer<TraceState> to Observer<PlotState>, and Controller registers reviewService on figure/subplot in addition to the existing trace registration - consistent with the Observer pattern used elsewhere.
• AudioService.update() already early-returns for non-trace states, so the new figure-lobby exit path cannot accidentally double up a data tone with the exit arpeggio - confirmed by test/service/audio.subplotCue.test.ts's 'no double sound on exit' test.
• The new esc,backspace binding for MOVE_TO_SUBPLOT_CONTEXT is scoped to TRACE only, and the existing hotkeys.filter already excludes non-braille/-review editable elements, so this shouldn't collide with native Backspace-to-delete behavior in inputs/textareas.

Minor nits (non-blocking)

• docs/SCHEMA.md: the new 'Top-Level Figure Properties' section duplicates some prose already present for per-layer axes a few lines above; a one-line cross-reference instead of restating the Pick<AxisConfig, label> rationale would help doc maintainability.
• subplotEntryMessage's verbose form ('Entered subplot 2 of 4, bar plot.') reads slightly awkwardly with the comma before 'bar plot' - cosmetic only.
• AnnounceCommand.resolveActiveTraceState()'s 'subplot' branch and AnnounceTitleCommand's equivalent are both marked 'defensively unreachable in practice' and duplicate the same reasoning in comments; since contextStackInvariant.test.ts now pins this invariant at the Context level, a shared reference (rather than repeating the explanation in each command) would reduce upkeep if the invariant's reasoning ever needs to change.

Test coverage
Strong - new/updated tests cover: figure-wide axis precedence and fallback (announce-axis.test.ts), title fallback (announce-title.test.ts), subplot enter/exit cues in all three text modes including the braille-suppressed alert path (subplot-transitions.test.ts, braille-escape.test.ts), the schema parsing path end-to-end (figureAxes.test.ts), the lobby out-of-bounds cue (lobbyBoundary.test.ts), terse lobby navigation text (figureLobbyText.test.ts), and the context-stack invariant several commands rely on (contextStackInvariant.test.ts). No obvious gap spotted.

I didn't run the build/test suite in this environment (no node_modules/network available), so please confirm CI is green - but nothing in the diff itself raised correctness or security concerns.

[claude]

Review: Figure lobby as a first-class navigation level

Solid, well-scoped PR. The precedence logic (figure-wide → focused-subplot → "not available"), terse/verbose/off handling, and backward compatibility for figures that omit axes all check out — I traced the code paths and cross-checked them against the new tests. A few notes below, none blocking.

Verified non-issue (worth calling out since it looked suspicious at first glance)

MoveToSubplotContextCommand.execute() and ExitBrailleAndSubplotCommand call context.exitSubplot() (which re-announces the lobby via the observer chain → TextViewModel.update()) and then cue.announceExit() (NotificationService.notify() → TextViewModel.notify()). At first this looks like it could double-announce on Esc. It doesn't: Text.tsx (src/ui/component/Text.tsx:29-33) computes a single current value that always prefers message over value, and there's exactly one role="alert" region in the app. Since notify() is dispatched after update(), the exit message always wins and only one announcement reaches the screen reader. The in-code comment at move.ts:312-318 documents this correctly — nice to have that reasoning captured inline given how non-obvious the batching behavior is.

Architecture: message-building duplicated across Service and Command layers

src/command/subplotCue.ts (subplotEntryMessage/subplotExitMessage) reimplements the same terse/verbose mode-branching and "title || Subplot N" fallback that TextService.formatFigureText (src/service/text.ts:255-270) already owns for the same lobby wording. Per this repo's MVVC guidance, text formatting/mode-awareness belongs in the Service layer — right now the phrasing and terse-mode rules for "you're at subplot N" exist in two places that can drift independently (e.g. a future terse-mode wording change would need to be applied in both text.ts and subplotCue.ts). Not a bug today since the two aren't required to match verbatim (the cue is a one-shot transition announcement, formatFigureText is the steady-state lobby description), but worth consolidating — e.g. have SubplotCue call into TextService for the shared "title-or-position" fragment rather than reimplementing it.

Minor: intentionally-dead branch

AnnounceCommand.resolveActiveTraceState() and AnnounceTitleCommand (src/command/describe.ts) keep a state.type === 'subplot' branch that's documented as unreachable given Context.enterSubplot()/exitSubplot() always push/pop Subplot+Trace together. This is good defensive coding, and it's properly pinned by test/model/contextStackInvariant.test.ts rather than left as unjustified dead code — no action needed, just flagging that I checked it and it's intentional/tested, not an oversight.

Test coverage

Coverage is strong: figure-wide vs. subplot-level precedence, terse/verbose/off across all the new announce paths, single-panel (no figure-wide code path triggered) and empty-trace fallbacks, the braille-in-lobby warning, enter/exit tones, and the stack invariant are all exercised. One gap I didn't see covered: a test asserting the description modal (d at the lobby) omits a figure axis when its authored label is blank/whitespace-only (axes.x.label: ""), mirroring the "blank label → not available" handling already tested for the l x/l y commands.

Automated checks

I wasn't able to run npm run type-check / npm test / npm run lint in this review environment (command execution was sandboxed). Please confirm CI is green before merge, particularly type-check given the new Pick<AxisConfig, 'label'> typing in grammar.ts/state.ts.

Nice attention to detail on the accessibility framing (terse vs. verbose wording, distinct enter/exit tones, mode-aware fallbacks) — this is a thoughtful extension of the existing lobby behavior.

[claude]

Review: Multi-panel figure lobby navigation (#639)

Went through the full diff plus the actual PR source (checked out via pull/639/merge) to verify the changes in context, not just the hunks. Overall this is a well-structured, well-tested feature that follows the existing MVVC/Observer/Command patterns closely. A few observations, nothing blocking.

Strengths

• Good factoring: SubplotCue cleanly centralizes the repeated "play tone → build mode-aware message → notify" sequence that three commands (MoveToTraceContextCommand, MoveToSubplotContextCommand, ExitBrailleAndSubplotCommand) now share, instead of duplicating it three times.
• Single source of truth: pulling isAuthoredTitle and focusedSubplotTitle into model/plot.ts and having Context.isAuthoredTitle delegate to it (rather than keeping a parallel copy) is exactly the right move — avoids the drift the old comment in context.ts warned about.
• contextStackInvariant.test.ts is a nice touch — it pins the "context stack never exposes a bare Subplot" invariant that several AnnounceCommand branches rely on and document as defensively unreachable. If enterSubplot/exitSubplot ever regress, this fails loudly instead of leaving a silent dead branch.
• Mode-awareness is consistent: OFF/TERSE/VERBOSE handling for the new lobby text, boundary cues, and enter/exit announcements all follow the same pattern as the existing trace-level equivalents, and it's backed by matching tests for each mode.
• Confirmed AudioService.update() already no-ops for non-'trace' states (if (state.type !== 'trace') return;), so the claim in audio.subplotCue.test.ts about "no double sound on exit" holds — exitSubplot()'s notifyStateUpdate() won't fire a data tone alongside the new falling exit cue.
• docs/SCHEMA.md updates accurately describe the new axes field and its precedence rules; matches the implementation.

Minor observations (non-blocking)

1. ReviewService/ToggleReviewCommand widened to PlotState has no dedicated tests. src/service/review.ts and src/state/viewModel/reviewViewModel.ts both changed their generic from TraceState/Observer<TraceState> to PlotState/Observer<PlotState> to support review mode at the figure lobby, but grepping test/ turns up no test file for ReviewService or ReviewViewModel at all (pre-existing gap, not introduced here, but this PR is a good opportunity to add lobby-specific coverage — e.g. "review mode at the lobby announces the subplot nav text").

2. Asymmetric guard between the two "exit to lobby" commands. MoveToSubplotContextCommand.execute() only calls cue.announceExit(...) when this.context.scope === Scope.SUBPLOT (i.e., only after confirming exitSubplot() actually did something — it's a no-op on a single-subplot chart). ExitBrailleAndSubplotCommand's multi-panel branch (src/command/move.ts ~line 384-399) calls this.context.exitSubplot() then unconditionally calls this.cue.announceExit(this.context.state), with no equivalent check. In practice this is probably safe because braille toggling should only be reachable from inside a subplot when isMultiPanel is true, but it's worth double-checking there's no path (e.g. a future keybinding scope change) where this branch runs from the lobby itself and ends up announcing an exit that didn't happen. Consider mirroring the scope === Scope.SUBPLOT guard for defensive parity, or add a comment explaining why it's not needed here specifically.

3. Hardcoded DEFAULT_FIGURE_AXIS = '' as the "unauthored" sentinel is a slightly different convention from layer axes (which default to 'X'/'Y') — this is called out well in the code comments, but it does mean an empty string is doing double duty as both "figure has no state" (Context.figureXAxis getter fallback) and "axis omitted from JSON". Not a bug (both cases correctly resolve to "not authored"), just worth flagging since a future refactor that changes what empty-string means elsewhere could silently break isAuthoredAxisLabel.

4. ToggleBrailleCommand's new warning path (src/command/toggle.ts) always fires notificationService.notify(...) regardless of text mode, while the new SubplotCue enter/exit messages are explicitly gated by TextService.isOff(). This is actually consistent with how NotificationService.notify is used everywhere else in the codebase (autoplay speed, high-contrast toggle, monitor mode, etc. — all mode-independent alerts), so not a real inconsistency, just noting it in case the intent was for this warning to also respect OFF mode like the other new lobby cues.

Not verified

I was unable to run npm run type-check, npm test, or git fetch in this sandbox (all blocked pending approval), so I could not execute the new/changed test suites or confirm a clean TypeScript build myself. The static read-through of the 27 new/changed test files looks thorough and the assertions line up with the implementation I read, but it'd be good for CI to confirm before merge if it hasn't already.

Nice work overall — the lobby now behaves as a proper first-class navigation level rather than a second-class stub, and the precedence/fallback rules (figure-wide → subplot → "not available") are implemented consistently across text, audio, description, and braille.

[claude]

Review: Multi-panel figure lobby as a first-class navigation level

Nice, well-scoped feature with strong test coverage (9 new/updated test files) and clean adherence to the MVVC/Observer patterns described in CLAUDE.md. The SubplotCue collaborator, focusedSubplotTitle()/isAuthoredTitle() extraction to plot.ts, and the resolveActiveTraceState()/labelSourcePrefix()/announceAxisLabel() sharing in AnnounceCommand are good refactors that remove duplication instead of copy-pasting the X/Y logic. The contextStackInvariant.test.ts addition is a nice touch: it pins an assumption (enterSubplot/exitSubplot never leave a bare Subplot on the stack) that several new code paths silently depend on.

Things worth double-checking

1. ToggleBrailleCommand warning tone uses a different method than the sibling "not available" warnings (src/command/toggle.ts vs src/command/describe.ts). The new lobby-level "not available" cases in AnnounceCommand.announceAxisLabel() call audioService.playWarningToneIfEnabled(), but the new braille-at-lobby warning calls audioService.playWarningTone() (unconditional, not gated by whatever setting ...IfEnabled checks). If that distinction is meaningful (e.g. a user preference to silence warning tones), this warning would ignore it while the axis-label warnings respect it. Worth confirming this is intentional rather than an oversight.

2. Dual announcement path on subplot exit. MoveToSubplotContextCommand.execute() relies on exitSubplot()'s observer chain updating TextViewModel and a separate SubplotCue.announceExit() -> NotificationService.notify() call landing in the same tick, with the comment reasoning that React batches both dispatches into one re-render so only the notify() message "wins" in the alert region. This is a reasonable approach and it's tested at the unit level, but the interaction between two separate Redux-driven aria-live/role="alert" regions updating in the same tick is exactly the kind of thing that can behave inconsistently across screen readers/browsers. Given CLAUDE.md's guidance to manually verify UI/accessibility changes, it'd be good to confirm with an actual screen reader that the exit is announced once, not twice or garbled.

3. test/model/figureAxes.test.ts - the "collapses an authored-but-blank axis label to the empty sentinel" test doesn't actually test that. It builds Figure with y: { label: ' ' } and then asserts state.yAxis.trim()).toBe(''), which is trivially true whether yAxis is '' or the raw ' '. The model actually stores the raw whitespace string as-is; collapsing to DEFAULT_FIGURE_AXIS only happens downstream via Context.isAuthoredAxisLabel(). The assertion doesn't verify the claim in the test name/docstring. Consider asserting expect(state.yAxis).toBe(' ') (documents actual raw storage) or expect(context.isAuthoredAxisLabel(state.yAxis)).toBe(false) instead.

4. Minor API asymmetry in SubplotCue: the enter tone is played by the caller (cue.playEnterTone()) separately from announceEntry(), while the exit tone is played inside announceExit() itself. This is explained in the class doc comment (braille-enabled entry suppresses the spoken alert but not the tone) so it's not a bug, just a slightly inconsistent shape that a reader skimming call sites might not expect.

Nits

• The bare t key at the SUBPLOT/lobby scope changes meaning (was "announce title", now "toggle text mode"; title moves fully behind l t). This is called out in the PR description and is intentional, but it's a user-facing keybinding change worth flagging in release notes/changelog since it affects muscle memory for existing users.
• docs/SCHEMA.md and the Maidr.axes JSDoc in grammar.ts are clear and consistent with the implementation. Good docs discipline.

Architecture / conventions

• Follows MVVC correctly: no direct Redux/UI access from Trace/Figure/Context, new ReviewService/AudioService wiring goes through the existing Observer/Controller registration pattern, and SubplotCue is a command-layer collaborator (not a service), matching the documented "deliberate exception" style already used by MoveToTraceContextCommand.
• No any usage found in the diff.
• Wiring in CommandFactory (constructor params for MoveToTraceContextCommand, MoveToSubplotContextCommand, ExitBrailleAndSubplotCommand, ToggleBrailleCommand) all match their constructors' parameter order - verified.

Overall this looks solid and ready to merge, modulo confirming points 1 and 2 above are intentional (or fixing them), and possibly tightening the test assertion in point 3.

[claude]

Review

I read through the full diff (schema, commands, model, services, keybindings, tests) and traced the trickier runtime paths (state stack invariants, audio observer chain, notification/text dispatch ordering) against the actual source rather than just the diff context. Overall this is a well-scoped, carefully-reasoned change that follows the MVVC architecture closely. A few notes below; nothing blocking.

Strengths

• Good adherence to MVVC and existing patterns. New cross-cutting logic (subplot enter/exit cues) is centralized in a dedicated SubplotCue collaborator (src/command/subplotCue.ts) rather than duplicated across the three transition commands, and it stays properly unaware of Context/braille state — the commands still own those decisions. focusedSubplotTitle() and isAuthoredTitle() were extracted to src/model/plot.ts as single sources of truth shared by describe.ts, text.ts, and subplotCue.ts, replacing what used to be a duplicated placeholder-rejection rule in Context.
• No double-audio-tone bug. I verified that AudioService.update() short-circuits for non-trace state types (src/service/audio.ts:238), so Context.exitSubplot()'s observer-chain notification and SubplotCue.announceExit()'s explicit tone don't stack. Similarly the text/notification interplay (TextViewModel.update() clearing message, then SubplotCue's notify() overwriting it) is correctly ordered so exiting a subplot produces exactly one spoken message, matching the code comments' claims.
• Context-stack invariant is real, not just asserted. The new contextStackInvariant.test.ts pins the "no bare Subplot on the stack" invariant that AnnounceCommand.resolveActiveTraceState() relies on; I traced enterSubplot/exitSubplot/restoreNavigation in context.ts and confirmed Subplot+Trace are always pushed/popped together, so the defensive 'subplot' branches are genuinely unreachable today (good practice to keep them anyway, given they're cheap and documented as defensive).
• Solid test coverage for the new behavior — mode-awareness (OFF/TERSE/VERBOSE) is exercised for entry/exit cues, axis-label precedence, title fallback, and the new lobby boundary cue. test/model/figureAxes.test.ts also covers the "authored-but-blank" edge case for figure-wide axes distinctly from the "omitted" case, which is an easy thing to get wrong.
• Backward compatible schema addition. axes on Maidr is optional and narrowed to Pick<AxisConfig, 'label'>, which is a nice touch — it makes it a compile error to try to give a figure-wide min/max/tickStep, rather than silently ignoring them.

Minor suggestions (non-blocking)

1. Test helper duplication. createMockAudioService, createMockNotificationService, createMockTextService, and createSubplotCue are re-implemented nearly identically across test/command/braille-escape.test.ts, test/command/scopeRestore.test.ts, and test/command/subplot-transitions.test.ts. Given CLAUDE.md's KISS/"avoid unnecessary duplication" guidance, these would be good candidates for a shared test/command/testHelpers.ts (or similar) if this area gets touched again.
2. backspace as a global alternate to esc (src/service/keybinding.ts:311, MOVE_TO_SUBPLOT_CONTEXT: key('esc,backspace', ...)) is scoped correctly (only bound in TRACE scope, and the hotkeys.filter allow-list means it can't leak into editable braille/review inputs since those use different Scopes) — but it's a slightly unusual choice since Backspace isn't a typical "go back" affordance outside of browser history. Worth confirming this was an explicit product/accessibility decision (e.g. screen-reader user feedback) rather than an implementation convenience, since it's a global behavior change for anyone reading a chart.
3. docs/SCHEMA.md correctly documents the new axes field and the fallback precedence — good practice keeping docs and code in the same PR.

Things I specifically checked and found correct (no action needed, noting for reviewers' confidence)

• Figure.outOfBoundsState / Subplot.outOfBoundsState never set warning, so the new !state.warning boundary branch in TextService.update() (for the figure-lobby edge cue) is reliably distinguished from the rotor-bounds warning: true path.
• MoveToTraceContextCommand captures lobby = before.type === 'figure' ... before calling enterSubplot(), and Figure.state's index/activeSubplot are computed from the same row/col that enterSubplot() pushes — so the announced "Entered subplot N of M" always reflects the subplot actually entered, not a stale position.
• ReviewService/TextService.format() handle the (currently unreachable) 'subplot'-type PlotState correctly, so widening ReviewService from Observer<TraceState> to Observer<PlotState> and dropping the state.type === 'trace' guard in ToggleReviewCommand doesn't introduce a latent crash if the stack invariant were ever violated by a future change.

No security, performance, or architecture-boundary concerns — this stays within Model/Service/Command layers appropriately, doesn't touch anything user-input-sensitive, and the added audio/DOM work (three-oscillator arpeggios) is bounded and consistent with the existing menu-tone pattern.

[xabilitylab]

🎉 This PR is included in version 3.73.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀