feat: add tap-hold-opposite-hand-release action

[malpern]

Summary

A release-time variant of tap-hold-opposite-hand. Same signature, options, and defhands dependency. The only difference is that it waits for the interrupting key's press+release before committing, which avoids misfires on fast same-hand rolls where keystrokes briefly overlap.

This closes the main gap between kanata HRM behavior and ZMK's "timeless home-row mods" recipe.

Checklist

• Documentation in docs/config.adoc
• Example in cfg_samples/kanata.kbd
• Tests (5 sim tests)

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a well-structured addition that fills a real gap for ZMK-style timeless home-row mods. The implementation is correct and the tests demonstrate the key behavioral difference from tap-hold-opposite-hand. A few issues worth addressing before merge:

---

Bugs / Correctness

Stale development comment left in test (tap_hold_tests.rs)

tap_hold_opposite_hand_release_basic has leftover notes from development:

// Note: 's' is same-hand but not in defsrc, use a key in defsrc.
// Actually, 'a' is the tap-hold key itself. Let me use a different config.

These look like in-editor scratch notes. They should be removed — a test comment should describe intent, not thinking-out-loud during authorship.

---

Code Quality / Duplication

parse_tap_hold_opposite_hand_release is a near-verbatim copy

defhands.rs now has two ~150-line parse functions that differ only in which runtime closure they call. Every option arm (timeout, same-hand, neutral, unknown-hand, neutral-keys, require-prior-idle) is copy-pasted. If a new option is added to tap-hold-opposite-hand in the future, it would need to be added in two places.

A shared helper accepting a function-pointer/flag would eliminate the duplication:

fn parse_tap_hold_opposite_hand_inner(
    ac_params: &[SExpr],
    s: &ParserState,
    wait_for_release: bool,
) -> Result<&'static KanataAction> { ... }

Similarly, custom_tap_hold_opposite_hand_release is structurally identical to custom_tap_hold_opposite_hand — the only delta is the release-wait guard added after finding the press event. Both functions could be unified with a wait_for_release: bool parameter, or the release check could be factored into a small inline helper.

---

Performance (hot path)

O(n²) release scan (custom_tap_hold.rs:261)

if !queued.clone().copied().any(|q| q.event() == release) {
    continue;
}

For each press event in the queue, we clone and scan the remainder of the queue for a matching release — O(n) per press, O(n²) total. The existing custom_tap_hold_release does the same thing (line 63), so this is consistent with the codebase, but it's worth noting the pattern since this function runs on every key event. In practice the queue depth is low (bounded by rollover), so this is unlikely to matter, but if the team ever revisits the QueuedIter design a linear pass would be possible.

---

Unrelated changes mixed in

The PR includes three changes that appear unrelated to tap-hold-opposite-hand-release:

1. karabiner-driverkit 0.2.1 → 0.3.0 (Cargo.toml) — This is a semver-minor bump of a macOS-only dependency. Worth a separate commit note or PR so the bump is visible in the changelog independently.

2. has_device_filter fall-through fix (src/oskbd/macos.rs:106) — This looks like a legitimate bug fix (preventing silent fall-back to all-devices when a filter yields no matches), but it changes observable behavior and deserves its own entry in the changelog / PR description.

3. device_hash: 0 struct field additions (src/oskbd/macos.rs:53,128) — Required by an upstream struct change (karabiner-driverkit 0.3.0). Grouping this with the dependency bump commit would make the relationship clear.

None of these are blockers, but mixing unrelated fixes makes git blame / bisect harder.

---

Minor

• The cfg_samples/kanata.kbd example is fully commented out with ;;. That's fine for a sample, but a runnable (uncommented) example helps users copy-paste and validate. Consider leaving one concrete working example uncommented alongside the explanatory comment.

• Default behavior when no same-hand option is specified is Tap. This matches tap-hold-opposite-hand. Consider a one-liner in the docs noting the defaults explicitly, since the release-timing difference from the press variant makes the "no same-hand option" case particularly important to understand.

---

Test coverage

The 5 simulation tests cover the main cases well. Missing coverage:

• (neutral tap/hold) with a neutral-keys list
• (same-hand hold) behavior
• (same-hand ignore) — should fall through to timeout

These aren't blocking, but they'd round out the test suite and prevent regressions if the option parsing is ever refactored.

[claude]

Code Review: tap-hold-opposite-hand-release

Overall this is a clean, well-tested addition. The feature fills a real gap (ZMK-style timeless HRM behavior), the tests are comprehensive, and the docs are consistent with the existing tap-hold-opposite-hand entry. A few things worth discussing:

---

1. Significant code duplication — main concern

custom_tap_hold_opposite_hand_release in custom_tap_hold.rs is identical to custom_tap_hold_opposite_hand except for six lines that check the release event. The same is true for the ~140-line parser functions in defhands.rs.

For the core function, a simple refactor would be to add a wait_for_release: bool parameter to a shared inner implementation:

fn custom_tap_hold_opposite_hand_impl(
    hand_map: &'static HandMap,
    same_hand: DecisionBehavior,
    neutral_behavior: DecisionBehavior,
    unknown_hand: DecisionBehavior,
    neutral_keys: &'static [OsCode],
    wait_for_release: bool,
    a: &Allocations,
) -> &'static CustomTapHoldFn {
    a.sref(move |mut queued: QueuedIter, coord: KCoord| {
        let (_row, col) = coord;
        let waiting_hand = hand_map.get(col);
        while let Some(q) = queued.next() {
            if !q.event().is_press() { continue; }
            let (i, j) = q.event().coord();
            if i != REAL_KEY_ROW { continue; }
            if wait_for_release {
                let release = Event::Release(i, j);
                if !queued.clone().copied().any(|q| q.event() == release) { continue; }
            }
            // ... shared decision logic
        }
        (None, false)
    })
}

Then both public functions become one-liners delegating to _impl. For defhands.rs, the parser option block (timeout, same-hand, neutral, unknown-hand, neutral-keys, require-prior-idle) is identical in both functions and could be extracted into a shared parse_opposite_hand_options helper.

---

2. Loop pattern inconsistency (for vs while let)

The original custom_tap_hold_opposite_hand uses for q in queued, while the new function uses while let Some(q) = queued.next(). The difference is necessary (you need the mutable borrow for queued.clone() inside the loop), but it's worth noting this difference in a comment so future readers understand why the patterns differ. If you do the shared-impl refactor above, the original would also switch to while let, making this consistent.

---

3. O(n²) scan — acceptable but worth a comment

if !queued.clone().copied().any(|q| q.event() == release) {
    continue;
}

For each press event in the queue, this is an O(n) scan of the remaining queue — O(n²) overall in the worst case. This same pattern is used in custom_tap_hold_release and custom_tap_hold_release_trigger_tap_release, so it's consistent with the codebase and fine for keyboard queues (which are tiny in practice). A short comment acknowledging the assumption ("queue is always small") would help future maintainers who might be tempted to "optimize" it unnecessarily.

---

4. cfg_sample is commented out

All other examples in cfg_samples/kanata.kbd are live, not commented out. The comment says it requires defhands, but defhands is already defined earlier in that file (defhands was added with tap-hold-opposite-hand). Could this be uncommented as an actual working example?

---

5. Correctness of the core logic ✓

The release-detection logic is correct:

• The outer loop finds press events in the queue.
• queued.clone().copied().any(...) checks if the matching release appears after the press in the queue (clone starts at the current iterator position, which is past the press).
• If no release is found, the function returns (None, false) and waits — the WaitingState machinery will call it again when more events arrive.
• The release event is not consumed from the queue, so it's processed normally after the hold/tap decision is made.

The multi-key scenario is also handled correctly: if keys A and B are both pressed without releases, but then A is released, the decision is based on A's hand position — the first key to complete its press+release cycle wins.

---

6. macOS changes appear unrelated

The commits fix(macos): don't intercept all devices... and fix(macos): improve --list output for BLE devices and bump driverkit to 0.3.0 look like they may be cherry-picks from upstream kanata (the commit messages even reference PR numbers #1986, #1987). These are good fixes, but mixing them with the new feature makes this PR harder to review and bisect. Were these meant to be in a separate PR, or is this intentional to rebase on upstream fixes?

The has_device_filter fix in macos.rs is a meaningful behavior change: previously, if an include/exclude list matched nothing, kanata would fall back to grabbing all devices. The fix prevents that. This is correct, but it's a potentially breaking change for users with misconfigured device filters who were relying on the fallback. Worth calling out in release notes.

---

7. Tests ✓

Eight test cases covering: basic opposite-hand → hold, same-hand → tap, no-interrupt timeout (both default and (timeout hold)), press-time vs. release-time comparison, require-prior-idle, (same-hand hold), (same-hand ignore), and neutral-keys. This is solid coverage of the decision matrix.

---

Summary: The feature logic is correct and the tests are thorough. The main ask before merge would be to tackle the parser code duplication in defhands.rs — 140 lines of near-identical code is a real maintenance burden. The custom_tap_hold.rs duplication is smaller but the shared-impl refactor is straightforward. Happy to discuss any of the above.

[claude]

Code Review: tap-hold-opposite-hand-release

Overall this is a clean, well-motivated addition that closes a real gap in HRM behavior. The structure follows existing patterns and the test suite is solid. A few things worth addressing before merging:

---

🔴 Potential Bug: Iterator lookahead in the hot path

parser/src/cfg/custom_tap_hold.rs, new function

while let Some(q) = queued.next() {
    if !q.event().is_press() { continue; }
    // ...
    let release = Event::Release(i, j);
    if !queued.clone().copied().any(|q| q.event() == release) {
        continue;  // ← keeps looping but we already consumed this press from `queued`
    }

When queued.clone().any(...) returns false, we continue the outer loop — but the queued.next() for this press event was already consumed. This means a press event whose release hasn't arrived yet is silently dropped from the queue instead of being re-evaluated on the next call. If the queue contains [press_j, press_k, release_j] (two presses before the release), this could skip press_j and only see press_k.

The original custom_tap_hold_opposite_hand avoids this by making the decision on press without lookahead. Please add a test with an interleaved event sequence like d:a d:j d:k u:j u:k to verify the behavior is correct.

---

🟡 Parser code duplication (~130 lines)

parse_tap_hold_opposite_hand_release in defhands.rs is a near-exact copy of parse_tap_hold_opposite_hand. All the option-parsing boilerplate (timeout, same-hand, neutral, unknown-hand, neutral-keys, require-prior-idle) is duplicated.

Suggest extracting a shared parse_opposite_hand_options(...) helper that both parsers call. This keeps the two parsers thin wrappers around a common options parser, and avoids the risk of the two diverging silently in the future.

---

🟡 Stale error message in new parser

if matches!(tap_action, Action::HoldTap { .. }) {
    bail_expr!(
        &ac_params[1],
        "tap-hold does not work in the tap-action of tap-hold"   // ← generic
    );
}

This was copied from the original. Since users will see this error for tap-hold-opposite-hand-release, the message should say so. Minor but confusing to debug.

---

🟡 Unrelated changes bundled in the PR

The src/oskbd/macos.rs and src/main_lib/mod.rs changes — the device-filter fallback fix and the Karabiner warning — are good fixes but unrelated to the new action. The Cargo.toml karabiner-driverkit bump (0.2.1 → 0.3.0) also appears to be a build-fix dependency on the macOS changes. Bundling them here makes the diff harder to bisect if a regression appears. Would you consider splitting them into a separate PR?

---

🟢 What's done well

• Core logic is correct and minimal. The release-wait pattern (queued.clone().any(...)) is a natural extension of the press-time logic.
• Test coverage is good. 8 sim tests covering the main paths: opposite-hand, same-hand tap/hold/ignore, timeout, neutral-keys, require-prior-idle, and press-time vs release-time comparison.
• API is consistent. Same signature and options as tap-hold-opposite-hand; no surprises for existing users.
• Documentation is accurate. The table entry in config.adoc clearly describes the behavioral difference.
• The device-filter bug fix (has_device_filter guard) is correct and worth keeping — just better as a separate commit/PR.

---

Suggested additional test cases

• Interleaved events: d:a d:j d:k u:j u:k (two interrupts, first released second)
• (unknown-hand ignore) with a key not in defhands
• (neutral ignore) with a neutral-key (should fall through to unknown-hand)
• Tap-hold key released before the interrupt key's release (should this be tap or hold?)

---

Summary: The feature logic is sound. The iterator-lookahead consumed-event issue is the most important thing to verify or fix. The parser duplication and unrelated macOS changes are the other two items worth addressing before merge.