Refine cinematic onboarding keyboard

[malpern]

Summary

• refine the Metal keyboard hero with MacBook-like materials, illuminated legends, directional light, and a slower dark-to-light entrance
• match target key typography, capitalization, paired symbols, and pressed-key feedback across dark and light states
• complete the optional first-success flow in onboarding, preserve the native accessible fallback, and keep reduced-motion behavior deterministic

Fixes #954

Validation

• TEST_FILTER="GenericPackConfigTests/(testCapsLockPackInstallsTheSelectedCatalogConfiguration|testQuickLauncherBuildsOnTheCatalogCapsLockInstall)" ./Scripts/run-tests-safe.sh
• KEYPATH_SNAPSHOTS=1 TEST_FILTER="FirstSuccessOnboardingSnapshotTests" ./Scripts/run-tests-safe.sh
• python3 Scripts/check-accessibility.py
• ./Scripts/ui-deploy.sh plus installed-app replay and exact onboarding visual inspection
• REQUIRE_NOTARIZED=0 REQUIRE_STAPLED=0 ./Scripts/verify-installed-app.sh
• ./Scripts/test-full.sh: 5,012 tests passed; three unrelated existing Home Row timing snapshots fail because current output is 1300x586 while their baselines are 1300x638. This branch does not touch those tests or images.
• ./Scripts/review-gate.sh: remote review gate selected; GitHub claude-review must pass before merge

[claude]

Automated review (diff-only; note the supplied diff was truncated after 60000 bytes, so some later hunks were not reviewed).

Bug: hardcoded US-QWERTY shift symbols applied regardless of active keymap

Sources/KeyPathAppKit/UI/KeyboardStage/KeyboardStageSceneBuilder.swift (new shiftedNumberRowLabel(for:), ~line 444) maps digit/punctuation labels to their US shift-row symbols (1→!, -→_, etc.) and applies this whenever KeyCode.numberRow.contains(key.keyCode) is true, regardless of which LogicalKeymap produced the base label.

This isn't gated to the QWERTY-US preview path. FirstSuccessKeyboardHero.body in FirstSuccessOnboardingDialog.swift resolves keymap from the user's actual @AppStorage keymap preference (LogicalKeymap.resolve(id: selectedKeymapID)) and passes it straight into KeyboardStageSceneBuilder.make(layout:keymap:...). Since AZERTY/QWERTZ/Colemak/Dvorak/Workman/Graphite keymaps have empty extraLabels for the number row, displayLabel falls back to the physical/default digit label, which then gets forced through the US-only shift table — producing incorrect legends (e.g. AZERTY's shift+top-row is a digit, not US punctuation) for any user not on QWERTY-US.

Suggested fix: derive the shifted symbol from the active keymap (if it exposes a shifted-label lookup) instead of a fixed US table, or only apply shiftedNumberRowLabel when keymap == .qwertyUS.

Minor / lower-confidence observations (not blocking)

• KeyboardStageProjection.swift: the new asymmetric horizontal/vertical zoom math (verticalZoom, then post-hoc visibleWidth *= verticalZoom / horizontalZoom) has no direct unit test covering the horizontalZoom > 1 branch pre/post this change — worth a regression test given it affects camera framing across all keyboard-stage moments.
• The large KeyboardStage.metal rewrite (area-light shadows, GGX microfacet specular, value-noise grain) is difficult to verify for correctness from source alone (no compute-side test can catch shader logic errors); relying on the updated snapshot tests for visual regression coverage seems reasonable here, but flagging that this is effectively unverified beyond "compiles + snapshots updated."

I did not find other actionable correctness/reliability/security issues in the supplied portion of the diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1ccbbe0b2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Derive shifted legends from the selected keymap

When the selected logical keymap is System with a non-US input source, this synthesizes US shifted symbols from the base label—for example, a French or Japanese 2 is shown with @ even though SystemKeyLabelProvider.currentShiftLabels contains the actual shifted character. Use the selected keymap's shifted-label data instead of the hard-coded US mapping so the onboarding keyboard remains consistent with the user's active keymap.

AGENTS.md reference: AGENTS.md:L303-L306

Useful? React with 👍 / 👎.

[malpern]

Fixed in 2b2e33a. Shifted number-row legends now come from the selected LogicalKeymap. The dynamic System keymap carries SystemKeyLabelProvider.currentShiftLabels, QWERTY defines its own US shift map, and layouts without shifted data no longer receive synthesized US symbols. Added scene and provider regression coverage.

[claude]

Reviewed the supplied diff (truncated after 60000 bytes — did not see the tail of KeyboardStageSceneTests.swift, SystemKeyLabelProviderTests.swift, or design-qa.md, so those are unreviewed).

Possible regression: Sources/KeyPathAppKit/UI/Rules/FirstSuccessOnboardingDialog.swift — .onDisappear used to call both keyboardInput.stop() and keyboardEntrance.settle(); this PR drops the settle() call:

.onDisappear {
    keyboardInput.stop()
}

If keyboardEntrance is reused across presentations of the dialog (rather than being recreated), leaving its phase un-settled when the view disappears could mean the entrance animation resumes mid-transition (or replays from a stale pending/in-flight phase) the next time the dialog appears, instead of starting from a clean settled state. Given KeyboardStageEntrancePresentation explicitly models isSettled/isAnimating phases, this looks like it could be an unintentional drop rather than a deliberate behavior change — worth confirming this was intentional (e.g. tested with dialog dismiss → re-show) since nothing else in the diff explains why the settle call was removed.

Everything else in the diff (shader tuning, palette/zoom constants, LogicalKeymap.shiftLabels, WizardButtonBar/SecondaryButton styling additions) reads as visual/cosmetic tuning with corresponding test updates and no other correctness issues spotted in the visible portion of the diff.

[malpern]

Addressed the re-presentation concern in the latest commit: the dialog again settles an in-flight keyboard entrance on disappear, while the existing pending-phase guard preserves the first visible-frame animation. KeyboardStageEntranceTests passes.

[claude]

Reviewed the supplied diff (note: it was truncated after 60000 bytes, so KeyboardStageSceneTests.swift's new ~100 lines and anything past that point weren't visible to me).

Mostly a visual/shader polish pass with matching snapshot + timing test updates (e.g. transitionDuration 0.55→0.75 is correctly reflected in KeyboardStageEntranceTests). Two non-blocking correctness/maintainability notes:

1. Duplicated keycode tables risk drift. LogicalKeymap.swift's private KeyCode enum (grave/one…equal) and KeyboardStageSceneBuilder.swift's new private KeyCode.numberRow set both hardcode the same virtual keycodes independently. If either is edited later (e.g. a layout fix) without updating the other, the shift-label overlay (keymap.shiftLabels[key.keyCode]) will silently stop matching for the affected key rather than failing loudly. Worth pulling the number-row keycode list into one shared source of truth.

2. shiftLabels is only populated for QWERTY / the system-provided keymap. Other built-in LogicalKeymaps (AZERTY, Dvorak, etc.) default to shiftLabels: [:], so the new stacked primary/secondary number-row legend never appears for them — e.g. AZERTY genuinely requires Shift to type digits, so the omission is more noticeable there than a cosmetic gap. If that's intentional scope-limiting for this PR, fine, but it'd be good to confirm rather than have it be an unnoticed gap.

No functional/security/reliability issues found in the reviewed portion otherwise (the Metal shader math for the new GGX/Smith/Fresnel specular terms checks out against the standard Cook-Torrance formulas, and divisions are guarded with max(...)).

[claude]

Reviewed the supplied diff (truncated after 60000 bytes, so the AZERTY/Dvorak/Colemak sections of LogicalKeymap.swift and the tail of the Metal shader/tests were not visible to me — worth a self-check there).

Findings:

1. Legend font-weight risk — KeyboardStageLegendAtlasRasterizer.font(size:weight:) (Metal/KeyboardStageLegendAtlas.swift): the previous implementation deliberately built a CTFontDescriptor with an explicit kCTFontWeightTrait override to force non-regular weights on the system font. The new code instead does NSFont.systemFont(ofSize:weight:).fontName → CTFontCreateWithName(...). System-font weight variants are not always exposed under distinct resolvable PostScript names across macOS versions, so round-tripping through fontName can silently collapse .light/.medium back to the default weight — exactly the problem the old trait-based code worked around. Worth confirming visually (beyond the updated snapshots) that .light (the new case) actually renders lighter than .regular on the OS versions you support.

2. onScrollGeometryChange availability (FirstSuccessOnboardingDialog.swift, FirstSuccessLessonCopy): this SwiftUI API requires macOS 15+. If KeyPath's deployment target is lower, this either needs an @available/version gate or the target has already moved to 15+ — please confirm, since it's not obvious from this diff alone.

3. Fragile number-row heuristic duplicated in two places: both KeyboardStageSemanticOverlay.swift (usesNumberRowStack) and KeyboardStageLegendAtlasRasterizer independently infer "is this a stacked number-row legend" from primary.count == 1 && secondary?.count == 1. This works today only because number-row keys happen to be the sole producer of 1-char/1-char legend pairs. Any future legend with single-character primary+secondary (unrelated to the number row) will unintentionally get the number-row layout/sizing, and the two copies of the heuristic can drift. Recommend carrying an explicit flag on KeyboardStageLegend (set once in KeyboardStageSceneBuilder) instead of re-deriving it from string length in the renderer and the overlay view.

4. Minor: KeyCode.numberRow/shift-related key codes are now defined independently in both LogicalKeymap.swift and KeyboardStageSceneBuilder.swift — consider a single shared source of truth to avoid the two lists silently diverging.

No blocking correctness issues found in the visible portion beyond the above; the shader/projection math changes look like deliberate, self-consistent visual tuning.

[claude]

Reviewed the supplied diff (truncated after ~60000 bytes, so FirstSuccessOnboardingDialog.swift, WizardButtonBar.swift, WizardDesignSystem.swift, and the tail of KeyboardStageMetalRenderer.swift — including the new materialKeyFrame/atlasAlignment helpers referenced but not shown — were not visible and could not be checked).

No high-confidence correctness/reliability/security bugs found in the reviewable portion. This is mostly Metal shader and layout polish; the logic changes I could trace (easing in KeyboardStageEntrance.frontX, the horizontal/vertical zoom split in KeyboardStageProjection, edgeLegendAlignment in KeyboardStageSceneBuilder, and the shared KeyCode enum extension) are internally consistent and guarded against div-by-zero / empty-collection edge cases.

One minor, non-blocking observation:

• LogicalKeymap.shiftLabels (Sources/KeyPathAppKit/Models/LogicalKeymap.swift) is only populated for the QWERTY keymap; every other keymap (AZERTY, Dvorak, Colemak, international layouts, etc.) gets the [:] default. KeyboardStageSceneBuilder's new number-row shift-legend lookup (keymap.shiftLabels[key.keyCode]) degrades gracefully for those (falls back to the unshifted label, no secondary glyph), so it's not a crash/regression risk — just worth confirming this is intentional scope for now rather than an oversight, since non-QWERTY users will see number keys without shifted symbols shown while QWERTY users do.

Nothing else stood out as actionable in the visible diff.

[claude]

Reviewed the supplied diff (Metal/keyboard-stage visual overhaul + LogicalKeymap shift-label support). Note: the diff was truncated after KeyboardStageMetalRenderer.swift, so WizardButtonBar.swift, WizardDesignSystem.swift, FirstSuccessOnboardingDialog.swift, the rest of KeyboardStageMetalRenderer.swift, and the test files were not visible/reviewed here.

Findings from the visible portion:

1. Duplicated number-row keycode mapping (maintenance risk). LogicalKeymap.swift's private KeyCode enum (grave/one/two/.../equal) and KeyboardStageSceneBuilder.swift's new KeyCode.numberRow Set<UInt16> both hardcode the same physical keycodes (18,19,20,21,23,22,26,28,25,29,27,24) independently, in two different files. If either list is ever edited (e.g., adding/removing a key from the shift-label set) without updating the other, the shifted-legend lookup (keymap.shiftLabels[key.keyCode]) will silently stop matching for the affected key — no compile error, just a wrong/missing stacked legend. Consider sharing one source of truth for "which physical keys are the number row."

2. shiftLabels is only wired up for QWERTY + the dynamic system layout. The new LogicalKeymap initializer defaults shiftLabels to [:], and the diff only populates it for the hardcoded QWERTY keymap and the dynamic (provider.currentShiftLabels) case. Other built-in keymaps (AZERTY, Dvorak, Colemak, etc., per the file's static list) aren't touched in this diff, so they'll silently fall back to plain digits with no stacked shift legend on the number row — i.e. the new visual feature is inconsistent across layouts. Worth confirming this is intentional/tracked as follow-up rather than an oversight, and adding a test asserting the fallback behavior for a non-QWERTY layout.

No correctness/security issues found in the Metal shader math itself (checked the new keypath_area_shadow/GGX helpers for potential normalize((0,0)) NaNs from the various castOffset/light-direction terms — all the mix() ranges used avoid the zero vector).