Fix Neovim Terminal detail page rendering the Home Row Mods editor

[malpern]

Micah's visual once-over of the new Neovim Terminal detail page (#893) caught it rendering the Home Row Mods interactive keyboard instead of the pack's motions. Two compounding causes, both fixed:

1. isHomeRowModsPack was a heuristic that misfires on Vim-flavored packs

The dispatch's first branch matched any pack with ≥4 bindings on home-row keys (PackDetailView.swift:475). Neovim's bindings include j, k, l, d — exactly 4. Replaced with an exact associatedCollectionID == homeRowMods check. Verified no other pack matched the heuristic (every other home-row-adjacent pack has empty bindings), so this is behavior-preserving outside the bug.

2. The pack's explicit bindings were the wrong shape

#891 added 19 explicit PackBindingTemplates to satisfy testPackBindingsMatchCollectionMappings — but the idiomatic shape for table-style packs (Vim Navigation, Mission Control, Numpad) is empty bindings, which routes Pack Detail to MappingTableContent rendering the collection's mappings directly: single source of truth, identical styling to the Rules tab. Reverted to []. The invariant test skips empty-bindings packs by design, so it stays green.

Result

The Neovim detail page now shows the activation hint + the full 19-motion table — same presentation as Vim Navigation's page.

Verification

• PackCollectionIntegration / PackOwnership / PackRegistry suites green
• Deployed via quick-deploy; awaiting Micah's visual re-check
• swiftlint warnings in output are pre-existing on untouched lines

[claude]

Code Review

Overview

This PR fixes a rendering bug where the Neovim Terminal detail page incorrectly showed the Home Row Mods interactive keyboard editor instead of the pack's 19-motion table. The PR description is excellent — clear root cause analysis, two independent fixes, each with good justification.

---

Change 1: isHomeRowModsPack (PackDetailView.swift)

Verdict: ✅ Correct fix, well motivated.

The old heuristic (inputs.intersection(homeRowKeys).count >= 4) was inherently fragile — it tested structure rather than identity. Any pack with bindings on j/k/l/d would misfire, and Neovim Terminal triggered exactly that.

The replacement is unambiguous:

pack.associatedCollectionID == RuleCollectionIdentifier.homeRowMods

This is strictly more correct. The comment is appropriate and preserves the "why" for future maintainers.

Minor nit: The new check handles nil correctly because == on Optional<RuleCollectionIdentifier> returns false when the left side is nil — no regression for packs without an associatedCollectionID. No issue, just confirming it's safe.

---

Change 2: Neovim Terminal bindings reverted to [] (PackRegistry.swift)

Verdict: ✅ Correct idiom, properly documented.

The empty-bindings convention (MappingTableContent path) is already established by Vim Navigation, Mission Control, and Numpad. Reverting to it is the right call — single source of truth, consistent styling with the Rules tab, no duplication to keep in sync.

The test gate (testPackBindingsMatchCollectionMappings) correctly skips empty-binding packs:

if !pack.bindings.isEmpty, !collection.mappings.isEmpty {
    XCTAssertEqual(pack.bindings.count, collection.mappings.count, ...)
}

So the test suite stays green by design, not by accident. The new comment naming the convention and other packs that follow it is a nice touch.

---

Potential Concerns

1. Test coverage for isHomeRowModsPack: There's no unit test that asserts isHomeRowModsPack == true only for the HRM pack. The logic is now so simple (one equality check) that a dedicated test is low-priority, but if a future pack accidentally sets associatedCollectionID == .homeRowMods, the bug would be silent. Consider a test in PackCollectionIntegrationTests asserting exactly one pack in starterKit has associatedCollectionID == .homeRowMods — preventing the same misfire from recurring via a different route.

2. Convention discoverability: The empty-bindings idiom for table-style packs is documented in a comment on the neovimTerminal entry, but not near the Pack struct definition or PackBindingTemplate. A future contributor adding a new table-style pack might not see the pattern and repeat the Fix double-fulfill crash in testSingleKeyCaptureLifecycle (unblocks full-coverage lane) #891 mistake. A short doc comment on Pack.bindings explaining the two modes (explicit list vs. empty-for-collection-routing) would help.

3. Visual re-check still pending: The PR notes "awaiting Micah's visual re-check." This should be confirmed before merge since the fix is UI rendering correctness.

---

Summary

Both fixes are minimal, correct, and well-explained. The PR reduces complexity (−29 lines, removes a fragile heuristic) and aligns the Neovim pack with the established idiom. The two follow-ups worth considering before close: confirm Micah's visual sign-off, and consider a targeted integration test pinning which packs are "home row mods packs" to guard future regressions.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad1ca8ea05

ℹ️ 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]

Preserve Neovim key suggestions after emptying bindings

When this pack's bindings are emptied, Pack.affectedKeys becomes empty because it is derived only from bindings, and PackRegistry.packsTargeting(kanataKey:) only considers affectedKeys plus suggestedForKeys. Since this pack does not add any suggestedForKeys, selecting any Neovim motion key such as h, j, k, or / in the Mapper inspector will no longer surface the Neovim Terminal pack, even though it did before via the explicit bindings. Add representative suggestedForKeys when moving the mappings to the collection table.

Useful? React with 👍 / 👎.

[claude]

Code Review — Fix Neovim Terminal detail page

Overview: Small, focused bug fix with two cleanly scoped changes: swap a fragile binding-count heuristic for an exact ID check, and revert the Neovim Terminal pack to the idiomatic empty-bindings pattern shared with Vim Navigation, Mission Control, and Numpad.

---

PackDetailView.swift — isHomeRowModsPack

Correct direction. The old heuristic was a layering violation: it inferred pack identity from pack content, which meant any pack with 4+ home-row bindings (including Vim-flavored ones) would accidentally get the HRM editor. The new exact-ID check is deterministic and won't drift.

One minor concern: if associatedCollectionID is Optional, the comparison is still correct (nil → false), but if a future pack targets homeRowMods through a different collection ID alias or the enum case is renamed, this silently breaks with no test to catch it. Consider a comment noting "must stay in sync with RuleCollectionIdentifier.homeRowMods" or a simple unit test asserting that the HRM pack returns true and a Vim-flavored pack returns false.

---

PackRegistry.swift — Neovim Terminal bindings: []

Logically sound — the table-style pattern (single source of truth in the collection) is the right model here, and the comment clearly explains the convention.

The one gap worth flagging: testPackBindingsMatchCollectionMappings now skips Neovim Terminal because the invariant test short-circuits on empty bindings. After PR #891 added those 19 bindings specifically to keep that test green, reverting to [] loses the only automated assertion that the collection's 19 motions are correct. No immediate bug, but if a future edit silently changes the collection mappings, nothing catches it.

Suggestion: add a focused test (or extend an existing one) asserting the Neovim Terminal collection has exactly 19 mappings and that the pack's associatedCollectionID resolves to a non-nil collection — keeping the spirit of the old invariant without requiring duplicated binding templates.

---

Minor points

• The bindings: [] comment is clear and concise — good.
• The isHomeRowModsPack comment accurately describes why the heuristic failed, which is useful, but consider trimming the inline comment to just the behavioral contract for future readers: "Exact match — avoids misfiring on Vim-flavored packs that share home-row keys."
• Performance: the new isHomeRowModsPack is O(1) vs the old Set intersection — negligible in practice but a nice side-effect.

---

Summary

Correctness	✅ Both root causes correctly addressed
Test coverage	⚠️ testPackBindingsMatchCollectionMappings now skips Neovim Terminal — consider a collection-level assertion
Style/conventions	✅ Follows table-style pattern; comment explains the idiom
Security/performance	✅ No concerns

The fix is ready to merge as-is; the test gap is low-risk but worth a follow-up ticket if not addressed here.