Skip to content

Mapper: captured sequences render as macros; a lone media key is its system action - #1328

Merged
malpern merged 1 commit into
masterfrom
fix/mapper-sequence-macro
Oct 20, 2026
Merged

malpern merged 1 commit into
masterfrom
fix/mapper-sequence-macro

Conversation

@malpern

@malpern malpern commented Oct 20, 2026

Copy link
Copy Markdown
Owner

Two findings from the phase 6 hardware check on the Air.

  • Edge case (real bug, pre-existing): a captured sequence — Mute followed by ⌃⇧⌘4 — was rendered by KanataKeyConverter.convertToKanataSequence as a bare list (mute M-S-C-4), which Kanata can't parse, so the save was refused with the bridge's generic "could not be parsed". Multi-token outputs are now always (macro …); a chord is still one token with modifier prefixes. Reproduced and pinned by testMediaKeyThenChordSavesAsAMacro against the real bridge, plus unit expectations.
  • One icon: a lone captured media key is treated as the system action it names (MapperViewModel.applyCapturedOutput → selectSystemAction), so the output tile shows the action's SF Symbol from the first moment (previously an emoji from KeyPress.displayString that changed to the symbol after the save) and the save goes through the action path. testLoneMediaKeyCaptureBecomesTheSystemAction.

KEYPATH_TEST_ENFORCE_CLEAN_SUMMARY=1 Scripts/run-tests-safe.sh: 5,793 passed, 0 app warnings/errors, 0 build warnings. check-accessibility.py clean. Doc note in rules-admission-review.md §4.5.

🤖 Generated with Claude Code

…system action

Found on the Air: Mute followed by ⌃⇧⌘4 was rendered by the generator as a
bare list "(mute M-S-C-4)", which Kanata cannot parse, so the save was
refused with the bridge's generic message. Multi-token outputs are now
always a "(macro …)". A lone captured media key is now the system action it
names (MapperViewModel.applyCapturedOutput), so the output tile shows the
action's symbol from the first moment instead of an emoji that changed to
the symbol after the save, and the save goes through the action path.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-20T14:42:11.303587Z 90babb6 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 20, 2026

Copy link
Copy Markdown

Review (diff only)

No blockers, but two things worth checking:

  1. Capture-flow state conflict (MapperViewModel+CaptureManagement.swift): Capture accumulates keys and re-applies the whole sequence on each key. If the first key is Mute, applyCapturedOutput calls selectSystemAction, which sets selectedSystemAction and clears outputSequence. When the chord arrives, the 2-key sequence goes through the else branch and sets outputSequence. That branch doesn't visibly clear selectedSystemAction. If it stays set, the UI could show the stale action and save could take the system-action path, dropping the chord. Please make the sequence branch clear selectedSystemAction, and add a test that drives the real capture path (media key, then chord). testMediaKeyThenChordSavesAsAMacro sets outputSequence directly. testLoneMediaKeyCaptureBecomesTheSystemAction calls applyCapturedOutput with the sequence already built, and that test never checks selectedSystemAction afterwards.

  2. Behavior change in KanataKeyConverter: Multi-token output now returns (macro …) instead of (…). This is right for Kanata, but any caller that relied on the old list form (e.g. a chord like cmd space passed to something expecting (multi …)) will change. Only convertToKanataSequence is covered by the new tests. Please confirm no other call site wanted the bare list.

Minor: press.keyCode >= 1000 is a magic number for media keys. A named constant or helper would be clearer.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90babb601f

ℹ️ 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".

let action = SystemActionInfo.find(byOutput: KeyMappingFormatter.toKanataFormat(sequence)),
action.isSupportedInDriverlessSession
{
selectSystemAction(action)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Defer media-action selection until capture finalizes

When a media key begins a multi-key output, sequence capture first emits the one-key prefix, so this call selects the system action and immediately starts saveSystemActionMapping because the input is already set. The later callback containing the full sequence sets outputSequence but never clears selectedSystemAction, and autosaveCapturedMapping prioritizes that action; consequently an actual Mute-then-chord capture saves only Mute rather than the new macro, while the added macro test bypasses this streaming path by assigning outputSequence directly. Classify the media key only after capture finalization, or otherwise avoid the premature save and clear the action when the sequence grows.

Useful? React with 👍 / 👎.

@malpern
malpern merged commit 5e7728a into master Oct 20, 2026
8 checks passed
@malpern
malpern deleted the fix/mapper-sequence-macro branch October 20, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant