Skip to content

Pack audit fixes — categories, nav-dependency copy, Delete test#320

Merged
malpern merged 1 commit intomasterfrom
chore/pack-audit-fixes
Apr 25, 2026
Merged

Pack audit fixes — categories, nav-dependency copy, Delete test#320
malpern merged 1 commit intomasterfrom
chore/pack-audit-fixes

Conversation

@malpern
Copy link
Copy Markdown
Owner

@malpern malpern commented Apr 24, 2026

Summary

Three follow-ups from the post-Fun-Layer audit:

1. Category cleanup

Pack Before After
Home Row Mods Gallery (stale) Ergonomics
Auto Shift Symbols Productivity Ergonomics (reduces reach)
Window Snapping Productivity Navigation (spatial)
Mission Control Productivity Navigation (spatial)

Gallery now groups more evenly — ~3 packs per category across Ergonomics / Navigation / Layers plus the single-remap Productivity set. Vim Navigation is no longer alone.

2. Nav-dependency copy

Five packs need a Leader pack (Vim Navigation / KindaVim / Neovim Terminal) on for Space→nav activation, but didn't say so. Added a short dependency line to each shortDescription:

  • Window Snapping
  • Delete Enhancement
  • Numpad
  • Symbol
  • Function

Mission Control already had this from the collision fix in #319.

3. Delete Enhancement runtime behavior test

Was the only pack without one. Asserts Space + bspc → del (default Forward Delete preset).

Test plan

  • swift test passes (420 tests)
  • Gallery sections show the new category grouping
  • Installing Numpad/Symbol/Fun/Window/Delete standalone shows the dependency explanation in Pack Detail

🤖 Generated with Claude Code

Three follow-ups from the post-Fun-Layer audit:

* **Categories**: move Home Row Mods from the stale "Gallery" label to
  "Ergonomics"; add Auto Shift Symbols there too (both reduce reach).
  Move Window Snapping and Mission Control from "Productivity" to
  "Navigation" — they're both spatial navigation. Vim Navigation is no
  longer the lone pack in its category; Gallery now shows ~three packs
  each in Ergonomics / Navigation / Layers plus Productivity.
* **Nav-dependency copy**: add a line to Window Snapping, Mission
  Control (already done), Delete Enhancement, Numpad, Symbol, and Fun
  shortDescriptions explaining that Space-to-nav activation requires a
  Leader pack (Vim Navigation, KindaVim, or Neovim Terminal) to be on.
  Without this the user could install e.g. Numpad standalone, hold
  Space, get nothing, and not know why.
* **Delete Enhancement runtime behavior test**: covers the only pack
  that lacked one. Asserts Space + bspc → `del` (default Forward Delete
  preset).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review — PR #320: Pack audit fixes (categories, nav-dependency copy, Delete test)

Overview

Three focused follow-ups from the post-Fun-Layer audit: category reclassification for four packs, dependency copy added to five shortDescription fields, and a runtime behavior test for Delete Enhancement. The changes are well-scoped and low-risk.


✅ What's good

  • Category changes are well-reasoned. Moving Window Snapping and Mission Control to Navigation (spatial operations) and Auto Shift Symbols to Ergonomics (reduces reach) all make semantic sense. The gallery now groups more evenly.
  • Dependency notes improve UX. Users installing Numpad/Symbol/Fun/Delete standalone now know upfront they need a Leader pack on. Previously silent failure.
  • New test follows established patterns. The testDeleteEnhancementLeaderBspcEmitsForwardDelete test matches the structure and timing of other nav-layer tests (Vim Nav's 300ms hold, 30ms tap/release). The test comment correctly explains both the mechanism and the assertion.
  • Gallery category removal is clean. The GalleryView groups sections dynamically via Dictionary(grouping: PackRegistry.starterKit, by: \.category) — so dropping the last "Gallery" pack produces no empty section, no UI regression.

🔍 Issues to fix

1. Inconsistent "on" in dependency copy (Window Snapping)

All five packs should use identical phrasing, but Window Snapping drops "on":

Pack Current wording
Delete Enhancement "Requires a Leader pack **on** (Vim Navigation, …)"
Window Snapping "Requires a Leader pack (Vim Navigation, …)" ← missing "on"
Numpad "Requires a Leader pack **on** (Vim Navigation, …)"
Symbol "Requires a Leader pack **on** (Vim Navigation, …)"
Function "Requires a Leader pack **on** (Vim Navigation, …)"

PackRegistry.swift line ~248 — add "on" to match the other four.

2. New test only covers the positive path

The pack's headline promise is two things: "Regular Delete stays as-is" and "Leader+bspc → del." The test only asserts the second. Compare with testEscapeRemapTapEmitsCaps, which checks both the positive and the negative:

XCTAssertTrue(output.contains("caps"), ...)
XCTAssertFalse(output.contains("esc"), ...)

A second assertion (or a companion test) that plain bspc still emits bspc — without the Leader — would make this a proper regression guard for the first half of the promise. Suggested addition:

// Verify base-case: plain bspc should NOT produce 'del'
let baseEvents = try simulate(
    collectionIDs: [RuleCollectionIdentifier.deleteRemap],
    script: "↓bspc 🕐30 ↑bspc 🕐30"
)
XCTAssertFalse(outputKeys(baseEvents).contains("del"),
               "Plain bspc without Leader should NOT produce del; got: \(outputKeys(baseEvents))")

💡 Pre-existing note (no action required now)

category is String rather than a typed enum — a future typo (e.g. "Ergonomis") would silently create an orphan section. Worth a follow-up ticket, but this PR isn't the place to fix it.


Summary

Two actionable items before merge:

  1. Add missing "on" in Window Snapping's dependency note (one-word fix).
  2. Add a negative assertion (or companion test) confirming plain bspc is untouched without the Leader.

Everything else looks correct and consistent with project conventions.

@malpern malpern merged commit 9e74d9a into master Apr 25, 2026
3 checks passed
@malpern malpern deleted the chore/pack-audit-fixes branch April 25, 2026 01:39
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