Pack Detail parity with Rules — all 10 packs

[malpern]

Summary

Proper audit of all 10 packs against Rules' dispatch (not just config types). Found 4 gaps, not 2.

Rules dispatch has two layers

1. Config-type based — tapHoldPicker, singleKeyPicker, homeRowMods, autoShiftSymbols, layerPresetPicker, launcherGrid, chordGroups, …
2. Collection-ID specials — Window Snapping gets WindowSnappingView (convention picker + monitor canvas), Function Keys gets FunctionKeysView, Neovim/KindaVim get their own
3. Generic fallback — MappingTableContent for .table configs

What was wrong

Pack	Rules shows	Pack Detail was	Now
Auto Shift Symbols	AutoShiftCollectionView	illustrative bindings	AutoShiftCollectionView ✅
Symbol Layer	LayerPresetPickerContent	hardcoded samples	LayerPresetPickerContent ✅
Window Snapping	WindowSnappingView (convention picker + visual grid)	custom LazyVGrid	WindowSnappingView ✅
Vim Navigation / Mission Control / Numpad	MappingTableContent (shared table styling)	custom LazyVGrid	MappingTableContent ✅

Wire-through

All edits now call the same VM methods Rules calls:

• updateAutoShiftSymbolsConfig
• updateCollectionLayerPreset
• updateWindowKeyConvention (new in Pack Detail)

Install-on-first-touch with skipFinalReload so the config update is the single reload. State seeds from live collection on refreshInstallState.

Removed the custom collectionMappingsBlock (LazyVGrid) — no longer referenced.

Other packs verified parity

• Caps Lock Remap: TapHoldPickerContent ✅
• Home Row Mods: HomeRowModsCollectionView ✅
• Escape Remap / Delete Enhancement / Backup Caps Lock: SingleKeyPickerContent ✅

All 10 packs now embed the exact same SwiftUI view Rules does.

Test plan

• swift test passes (420 tests)
• Window Snapping pack — opens with Standard/Vim layout picker + U/I/J/K visual grid + L/R/C/M cards (matches Rules screenshot)
• Auto Shift Symbols pack — per-key toggles + timing slider
• Symbol Layer pack — preset picker (Mirrored + alternatives)
• Vim Nav / Mission Control / Numpad packs — MappingTableContent table matches Rules styling
• Edits in Pack Detail persist and are reflected in the Rules tab

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR achieves full Pack Detail / Rules-tab parity for Auto Shift Symbols and Symbol Layer packs by embedding the same AutoShiftCollectionView and LayerPresetPickerContent that the Rules tab already uses. The approach is correct and the pattern is consistent with the existing HomeRowMods path.

---

Positives

• Pattern consistency — applyAutoShiftEdit and applyLayerPresetEdit faithfully mirror applyHomeRowEdit: optimistic local-state update → install-on-first-touch with skipFinalReload → manager call. No surprises.
• refreshInstallState seeding — seeding autoShiftConfig / selectedLayerPresetId from the live collection on appear means Pack Detail reflects changes made in the Rules tab. Correct and necessary.
• Computed guard properties — associatedAutoShiftCollection and associatedLayerPresetCollection are clean one-liners that centralise the pattern-match logic; reusing them in both the view body and the seed helpers avoids duplication.
• skipFinalReload on first-touch install — this is the right call; the single reload from the follow-up config update is sufficient.

---

Issues / Suggestions

1. selectedLayerPresetId is written twice per user interaction (minor redundancy)

In the onSelectPreset closure:

onSelectPreset: { presetId in
    selectedLayerPresetId = presetId          // ← written here
    Task { await applyLayerPresetEdit(...) }
}

And then inside applyLayerPresetEdit:

private func applyLayerPresetEdit(presetId: String, ...) async {
    selectedLayerPresetId = presetId          // ← written again
    ...
}

The double-write is harmless (same value, SwiftUI deduplicates), but it's inconsistent with the HRM/TapHold pattern where local state is set only inside the apply* function. The closure assignment is redundant and can be removed to simplify the call site.

2. .id(selectedLayerPresetId ?? "") forces a full re-mount on every preset click

The empty-string fallback follows the existing "\(pickerTapSelection ?? "")-\(pickerHoldSelection ?? "")" precedent, so it's consistent. However, because selectedLayerPresetId is set synchronously before the Task fires, every click destroys and re-creates LayerPresetPickerContent. This is intentional for re-seeding, but worth a brief comment explaining why (like the TapHold picker comment does). Otherwise a future reader might "fix" it to a stable id and break the seeding.

3. No test coverage for the two new apply* paths

The PR description confirms 420 tests pass, but no new tests cover:

• applyAutoShiftEdit installs the pack on first touch
• applyLayerPresetEdit calls updateCollectionLayerPreset with the right ID
• refreshInstallState correctly seeds autoShiftConfig / selectedLayerPresetId from the live collection

The HRM apply path has the same gap, so this is not a regression, but these paths have meaningful side effects (installs, reloads) that are worth at least a happy-path unit test.

4. Comment verbosity in the view body

CLAUDE.md calls for comments only when the why is non-obvious. The // Auto Shift Symbols: enabled-keys toggles + timing slider + fast-typing protection... and // Layer preset picker — Symbol/Fun layers... blocks describe what the code does (already expressed by the view types) rather than why a non-obvious choice was made. They could be trimmed to a single line or dropped in favour of the computed-property names doing the work.

---

Summary

The implementation is solid and correctly extends the established pattern to two previously inconsistent packs. The redundant selectedLayerPresetId assignment and missing tests are the most actionable items; the rest are polish. Happy to approve with those addressed or as-is if the test gap is considered acceptable given the existing HRM precedent.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee9f05d3b9

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

Re-key Auto Shift editor when config changes

AutoShiftCollectionView stores enabledKeys/timeoutMs/protectFastTyping in @State initialized from config, but this wrapper keys it with .id(autoShiftCollection.id), which never changes as config changes. After refreshInstallState loads a non-default live config (lines 746-748), the child keeps stale defaults, and the next interaction can write those stale values back (e.g., reverting a previously customized timeout). The key should vary with the config (or the child should bind directly) so live state is actually reflected.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid clobbering pending layer preset during first-touch install

When a user picks a preset while the pack is off, applyLayerPresetEdit installs first and then updates the preset. That install path calls refreshInstallState, and this assignment overwrites selectedLayerPresetId with the pre-change value before the update runs. Because the picker is keyed by selectedLayerPresetId (.id(selectedLayerPresetId ?? "")), the view remounts to the old preset, making the selection appear to revert and potentially leaving the UI out of sync until reopening.

Useful? React with 👍 / 👎.

[claude]

Code Review

Overview

This is a well-scoped, purposeful PR. The goal is clear: replace four diverged Pack Detail views with the same SwiftUI components already used in the Rules tab. The approach is consistent with the existing homeRowModsConfig / applyHomeRowEdit pattern, which makes the code easy to follow.

---

Issues to address before merging

1. Missing optimistic update in applyWindowConventionEdit

applyAutoShiftEdit updates autoShiftConfig immediately (optimistic), preventing a UI flash. applyWindowConventionEdit has no local state equivalent — WindowSnappingView reads collection.windowKeyConvention ?? .standard from the live collection. Depending on how quickly the live collection updates after updateWindowKeyConvention returns, there may be a momentary snap-back to the previous convention.

Either add a local @State private var windowConvention: WindowKeyConvention = .standard (seeded in refreshInstallState) or document why the live-collection path is fast enough that it won't matter.

2. applyLayerPresetEdit sets state before install, which install may overwrite

private func applyLayerPresetEdit(presetId: String, collectionID: UUID) async {
    selectedLayerPresetId = presetId          // optimistic
    if !isInstalled {
        await install(skipFinalReload: true)  // could call refreshInstallState internally?
    }
    await kanataManager.updateCollectionLayerPreset(collectionID, presetId: presetId)
}

If install(skipFinalReload: true) triggers any state refresh (directly or via notifications), selectedLayerPresetId will be overwritten by the old live value before the updateCollectionLayerPreset call runs. Worth a quick trace to verify skipFinalReload fully gates this path.

3. Fragile anonymous tuple in MappingTableContent mapping

MappingTableContent(
    mappings: collection.mappings.map {
        ($0.input, $0.output, $0.shiftedOutput, $0.ctrlOutput,
         $0.description, $0.sectionBreak, isInstalled, $0.id, nil)
    }
)

Positional tuples are fragile — if MappingTableContent's parameter type ever reorders or adds a field, this silently produces wrong data (Swift won't error if element types match by position). If a named struct or typealias exists for this tuple, use it. If not, a follow-up to introduce one would reduce future maintenance risk.

---

Minor observations (no action required, but worth knowing)

Double pattern-match in liveAutoShiftConfig()

liveAutoShiftConfig() calls associatedAutoShiftCollection (which matches .autoShiftSymbols), then immediately re-matches case let .autoShiftSymbols(cfg) = collection.configuration to extract the payload. Two enum switches on the same value. Not a bug, just slightly redundant — could be folded into one computed property if you ever revisit this area.

Growing if-else dispatch chain

The contentBlock now has five or more branches. Each new pack type adds another arm. A configuration-type-based dispatch table would scale better, but this is clearly follow-up work — not a blocker for this PR.

Verbose inline comments

A few of the new // blocks are multi-sentence explanations of what the branch does rather than why. Per project conventions, the what is self-evident from the code. The why (e.g., "same view the Rules tab uses") is worth keeping; the rest could be trimmed.

.id(selectedLayerPresetId ?? "") fallback

An empty-string fallback means SwiftUI sees the same "" ID for all nil states. This is fine today (the view only renders when associatedLayerPresetCollection != nil), but if the guard ever changes, the nil → non-nil transition would not force a remount. A more explicit sentinel like "none" would make intent clearer.

---

Test coverage

No new tests in the diff, which is expected for pure view-dispatch parity work — testing SwiftUI branch selection is impractical at the unit level. The manual test plan in the PR description covers the golden path. One suggestion: verify that editing in Pack Detail and then navigating to the Rules tab shows the same persisted state (round-trip), since that's the core invariant this PR is establishing.

---

Summary

The core approach is sound and follows established patterns in the codebase. The two issues worth addressing before merge are the missing optimistic update in applyWindowConventionEdit (potential visual inconsistency) and the install/state-refresh ordering in applyLayerPresetEdit (potential race on first-touch). The anonymous tuple is lower priority but worth a note. Otherwise this is a clean parity fix.

[claude]

Code Review — Pack Detail parity with Rules (all 10 packs)

Overview

Solid audit work. Finding 4 gaps instead of 2, then fixing all of them in a single PR is the right approach. The core strategy — embed the exact same SwiftUI view the Rules tab uses instead of maintaining a separate custom UI — eliminates an entire class of future drift bugs. The embeddedEditor helper is a clean DRY extraction of the repeated opacity + animation wrapper.

---

Bugs

1. Redundant nil guard inside the help button action (PackDetailView.swift, help button block)

if helpResourceName != nil {           // outer guard
    Button {
        if helpResourceName != nil {   // ← redundant; already guaranteed by outer `if`
            showingHomeRowModsHelp = true
        }
    } ...
}

The inner check is dead code — the outer if already guarantees non-nil. Just call showingHomeRowModsHelp = true directly inside the action.

2. Help sheet is hardcoded, not driven by helpResourceName

.sheet(isPresented: $showingHomeRowModsHelp) {
    MarkdownHelpSheet(resource: "home-row-mods", title: "Home Row Mods")
}

helpResourceName returns "home-row-mods" today for HRM only, so this works now. But helpResourceName is a switch designed to grow — the next developer who adds a second pack to that switch will get a ? button that silently opens HRM content. The sheet should read from helpResourceName:

.sheet(isPresented: $showingHomeRowModsHelp) {
    if let resource = helpResourceName {
        MarkdownHelpSheet(resource: resource, title: pack.name)
    }
}

3. selectedLayerPresetId is set twice on every user selection

In bindingsBlock:

onSelectPreset: { presetId in
    selectedLayerPresetId = presetId          // set here…
    Task { await applyLayerPresetEdit(presetId: presetId, ...) }
}

Inside applyLayerPresetEdit:

private func applyLayerPresetEdit(presetId: String, ...) async {
    selectedLayerPresetId = presetId          // …and set again here

Pick one place. The closure set is synchronous and eager (good for instant UI feedback); the applyLayerPresetEdit set is async and redundant. Either drop the in-closure assignment and let the apply function own state, or drop the apply-function assignment and keep only the closure one.

---

Inconsistency

4. Window Snapping uses ID equality while all other new branches use config-type pattern matching

// New branches (consistent):
case .autoShiftSymbols = collection.configuration  ✅
case .layerPresetPicker = collection.configuration  ✅

// Window Snapping (outlier):
collection.id == RuleCollectionIdentifier.windowSnapping  ⚠️

If this is intentional (e.g., Window Snapping doesn't have a distinct config type that would break for other collections), add a comment explaining why ID equality is used here instead of the pattern-match style. Without it, the next person refactoring this chain will either "fix" it incorrectly or wonder if it's a bug.

---

Minor / Nits

5. Lost comment about the TapHold .id feedback loop

The removed comment explained why updating local state on user click is "harmless":

User clicks update our local state, which bumps the id, which re-seeds — harmless because the new seed matches what the user just clicked.

That's non-obvious. The preserved truncation leaves the why unclear. Worth keeping.

6. .id(selectedLayerPresetId ?? "") empty-string fallback

An empty string ID is technically valid but semantically opaque. "layer-preset-unset" or similar would be clearer for debugging, and avoids any (theoretical) collision if SwiftUI ever sees two views with id("").

---

What's well done

• embeddedEditor is the right abstraction — it eliminated 4 separate opacity/animation duplications and centralises the InsetBackPlane styling contract.
• refreshInstallState seeds all three new local state vars (autoShiftConfig, selectedLayerPresetId) from live collection state, matching the pattern for homeRowModsConfig. This is consistent.
• The Window Snapping horizontalPadding: 12 override preserving the Rules tab's specific inset is exactly the kind of detail that matters for visual parity.
• skipFinalReload on install-on-first-touch is the right call — avoids a double reload race.

---

Test coverage

No new tests for the 4 added branches. Given these are SwiftUI @ViewBuilder paths, unit-testing them is awkward, but the test plan checklist in the PR description covers the golden paths manually. If there's a UI test harness for PackDetailView, adding a smoke test that the correct view type is rendered per pack type would help catch future regressions when new packs are added.

Summary: Two real bugs (hardcoded sheet resource, redundant double-set), one clear inconsistency to document or fix (Window Snapping ID check), and a few minor nits. Nothing blocking once bugs 1–3 are addressed.

[claude]

Code Review — Pack Detail parity + ConfigReloadCoordinator deferred retry

Two separate concerns in this diff. Both deserve individual attention.

---

Part 1 — ConfigReloadCoordinator.swift (not covered in prior review)

This part adds silent cooldown detection and a deferred retry. It's the higher-risk change.

Fragile string matching

private func isCooldownBlockMessage(_ message: String) -> Bool {
    message.contains("Reload blocked") && message.contains("cooldown")
}

This is doing semantic work (suppressing a user-visible error notification) based on a substring match. If the message format ever changes — capitalisation, wording, locale — this silently stops working: cooldown blocks start emitting error toasts again or real failures start getting swallowed (if matching too broadly). Recommend pinning to a constant or using a typed error instead:

// In whatever produces the error message:
static let cooldownBlockedMessage = "Reload blocked: cooldown active"

// In the coordinator:
message == KanataReloadError.cooldownBlockedMessage

Potential data race on deferredReloadTask

ConfigReloadCoordinator is a final class (not an actor). deferredReloadTask is mutated from two places:

1. scheduleDeferredReloadAfterCooldown() — sets it synchronously from whatever context called the TCP reload
2. The Task closure itself — sets it to nil after the retry completes

There's no synchronisation. If two rapid edits fire, the second call cancels the first task and replaces deferredReloadTask, but the first task's self.deferredReloadTask = nil line still runs 3.2 s later and silently nils the second task's reference — not the task itself, but its handle. The task still runs; you just lose the ability to cancel it. Low probability, but worth at least a comment or making the coordinator an actor.

Deferred retry swallows its own failure

await self.triggerReload()
self.deferredReloadTask = nil

If triggerReload() fails again (still throttled, TCP down, etc.) the error is dropped silently. The user sees no feedback that their config write never actually reached kanata. At minimum, log the outcome; ideally surface it if the retry also fails.

Hidden async path complicates debugging

CLAUDE.md's bug investigation protocol warns: "Check actual logs first — don't theorize before reading logs." A 3.2 s deferred retry that fires outside the original call chain is exactly the kind of thing that produces confusing logs (config write appears to succeed, kanata picks up a change ~3 s later with no obvious cause). The AppLogger line is good; consider making the log message include the original error that triggered the retry so the causal chain is clear.

---

Part 2 — PackDetailView.swift

The prior Claude review covered the main structural points well. A few gaps:

Help sheet uses hardcoded resource name, not helpResourceName

// State var:
@State private var showingHomeRowModsHelp = false

// Sheet:
.sheet(isPresented: $showingHomeRowModsHelp) {
    MarkdownHelpSheet(resource: "home-row-mods", title: "Home Row Mods")
}

But the helpResourceName computed property exists specifically to make this generic. When a second pack gets help content, helpResourceName will return its resource name, but the sheet will still render the HRM content. Fix: drive the sheet off helpResourceName directly. Also rename showingHomeRowModsHelp → showingHelpSheet since the mechanism is no longer HRM-specific.

Redundant nil guard inside the button action

if helpResourceName != nil {       // outer guard
    Button {
        if helpResourceName != nil { // ← dead — already inside outer guard
            showingHomeRowModsHelp = true
        }
    }
}

The inner check is unreachable. Remove it.

selectedLayerPresetId double-write (echoing prior review for completeness)

The closure sets it, then applyLayerPresetEdit sets it again. Harmless but inconsistent with the HRM/TapHold pattern. Remove the closure assignment and let applyLayerPresetEdit own the write.

---

Summary

The Pack Detail parity work (the main thrust of the PR) is clean and follows the established pattern correctly. The ConfigReloadCoordinator changes are the riskier addition — the string-matching suppression, missing synchronisation on deferredReloadTask, and silent retry failure are all fixable with small changes. The help-sheet hardcoding in PackDetailView is a latent bug waiting for the next pack to expose it.

Actionable items ranked by priority:

1. Fix the help sheet to use helpResourceName (breaks correctness for future packs)
2. Add typed error/constant for cooldown detection (fragile string match)
3. Log or surface deferred retry failures (silent data loss)
4. Remove the inner if helpResourceName != nil dead code
5. Remove the double-write of selectedLayerPresetId

[claude]

Code Review — Pack Detail parity with Rules (all 10 packs)

Overview

Solid structural work. Replacing the bespoke collectionMappingsBlock LazyVGrid with MappingTableContent and routing Auto Shift, Layer Preset, and Window Snapping through the same view components Rules uses is the right call — single source of truth, consistent styling, and less divergence to maintain. The embeddedEditor wrapper is a clean DRY extraction.

---

Bugs / Correctness

1. Double nil check in the help button action (PackDetailView.swift)

The button action re-checks helpResourceName != nil even though it's already inside an outer if helpResourceName != nil branch:

if helpResourceName != nil {
    Button {
        if helpResourceName != nil {   // ← redundant
            showingHomeRowModsHelp = true
        }
    }
}

The inner guard is dead code — just showingHomeRowModsHelp = true in the action.

2. Help sheet hardcodes resource instead of using helpResourceName

.sheet(isPresented: $showingHomeRowModsHelp) {
    MarkdownHelpSheet(resource: "home-row-mods", title: "Home Row Mods")
}

The computed helpResourceName already returns "home-row-mods" for Home Row Mods — but the sheet ignores it and re-hardcodes the value. When a second pack gets a ? button (it will), you'd need a second @State bool and a second .sheet. Use helpResourceName to drive both the resource and title dynamically, and rename the state var to something pack-agnostic (showingHelpSheet).

3. selectedLayerPresetId is set twice per edit

In bindingsBlock:

onSelectPreset: { presetId in
    selectedLayerPresetId = presetId         // set #1
    Task { await applyLayerPresetEdit(...) }
}

And inside applyLayerPresetEdit:

selectedLayerPresetId = presetId             // set #2

The inline assignment in onSelectPreset can be removed since applyLayerPresetEdit handles it. (Opposite of issue #2 — here the function is the right place.)

---

Code Quality

4. NSObject inheritance added without explanation (GalleryWindowController.swift)

-final class GalleryWindowController {
+final class GalleryWindowController: NSObject {

The closure-based addObserver(forName:object:queue:) variant used here does not require NSObject. If this was added to fix a compile error, a brief comment is warranted; otherwise it should be reverted — unnecessary NSObject inheritance brings in ObjC KVO/KVC surface for free.

5. .map used for side-effect observer removal (GalleryWindowController.swift)

self?.willCloseObserver.map(NotificationCenter.default.removeObserver)

Optional.map is for transforms, not side effects — this is non-idiomatic and the Void result is silently discarded. Prefer:

if let obs = self?.willCloseObserver {
    NotificationCenter.default.removeObserver(obs)
}
self?.willCloseObserver = nil

6. isCooldownBlockMessage is fragile (ConfigReloadCoordinator.swift)

message.contains("Reload blocked") && message.contains("cooldown")

This couples the retry logic to kanata's exact error message strings. A kanata update that rephrases the message silently disables the deferred retry. Worth adding a // NOTE: tied to kanata's exact error text — update if kanata changes comment, or better, defining the matched substrings as private constants so they're visible at a glance.

7. Magic number for cooldown duration (ConfigReloadCoordinator.swift)

try? await Task.sleep(nanoseconds: 3_200_000_000)

The 3.2 s value is derived from kanata's 3 s cooldown + slop, but nothing in the code documents that relationship. A named constant (kCooldownSlop) or inline comment would make the intent clear for the next person who touches this.

---

Minor

• showingHomeRowModsHelp (covered above in refactor: remove unused lifecycle state machine, keep KanataState #2) — name is pack-specific. Rename to showingHelpSheet for future packs.
• The deferredReloadTask property and its mutation across async boundaries in ConfigReloadCoordinator (a plain final class, not an actor) could race if scheduleDeferredReloadAfterCooldown is invoked concurrently. Worth a brief audit of the call sites to confirm it's always called from a single actor context.

---

What's Good

• embeddedEditor wrapper is a clean extraction — opacity + animation were duplicated 4× before, now defined once.
• install(skipFinalReload: true) + single subsequent config-update reload is the correct pattern per ADR-031.
• Observer scoped to newWindow (not a global observer) in GalleryWindowController — won't fire on unrelated window closes.
• Removing collectionMappingsBlock entirely rather than keeping it around "just in case" — good discipline.

---

Priority fixes before merge: issues #1, #2, #3 (correctness / forward-compatibility). The rest are style/robustness nits.

[claude]

Code Review — PR #318: Pack Detail parity with Rules

A thorough audit fixing 4 real gaps (not 2). The approach is correct throughout: embed the same view the Rules tab uses, delegate to the same VM methods, seed local state from the live collection. Below I've broken feedback by file since the diff spans four different areas.

---

ConfigReloadCoordinator.swift — Cooldown deferred-retry

This is the most interesting new logic and it works correctly. A few notes:

✅ @MainActor makes the task mutation safe.
scheduleDeferredReloadAfterCooldown creates an unstructured Task that inherits the class's @MainActor isolation, so deferredReloadTask?.cancel() / = nil mutations are serialised on the main actor. No data race.

⚠️ isCooldownBlockMessage is brittle string matching.

message.contains("Reload blocked") && message.contains("cooldown")

This couples to whatever exact words kanata or the TCP layer happen to use today. If the message text ever changes the deferred-retry silently stops working and real errors start being silently swallowed (the else branch is the notification path). Consider:

• A typed error enum / error code on TCPReloadResult so the caller can match on .cooldownThrottle rather than a substring
• Or at minimum a static let cooldownMarker = "Reload blocked" constant so there's one place to update

Minor: comment says "kanata" but this is the TCP layer's own throttle. The comment in scheduleDeferredReloadAfterCooldown says "3s cooldown + a little slop so the safety check passes" — worth confirming this 3.2s constant is derived from ReloadSafetyMonitor's actual configured cooldown, not a copy-paste guess. If the safety monitor's window changes, this will regress silently.

---

GalleryWindowController.swift — Overlay hide/restore

The hide-on-open / restore-on-close pattern is correct and matches how Settings does it.

Minor: unconventional Optional.map as a conditional call.

self?.willCloseObserver.map(NotificationCenter.default.removeObserver)
self?.willCloseObserver = nil

Optional.map as a conditional side-effect call is valid Swift but surprises readers expecting map to produce a value. A straight if let is clearer and makes the intent obvious:

if let obs = self?.willCloseObserver {
    NotificationCenter.default.removeObserver(obs)
}
self?.willCloseObserver = nil

Minor: NSObject inheritance added as a prerequisite.
The comment explaining why NSObject is now required (selector-based or typed addObserver API) would help a future reader — right now the change appears silently.

---

RulesSummaryView+MappingTable.swift — Chord input formatting

The chord case ("Lctl Lmet Lalt Up" → "⌃⌘⌥↑") is a real gap and the approach is reasonable.

⚠️ .components(separatedBy: " ").first relies on an internal macModifiers value format.

return macName.components(separatedBy: " ").first ?? macName

This assumes values in macModifiers are always "⌘ Cmd" (symbol-space-word). If the dict ever gains entries that aren't space-separated (e.g. a bare "⌘"), the .first silently returns the whole string, which is probably correct, but the assumption is invisible. A comment noting the expected format of macModifiers values would make this self-documenting.

Minor: empty-token guard.
key.split(separator: " ") on "Lctl Lmet" (double space) produces empty tokens; macModifiers[""] is nil so they fall through to formatModifierPrefixNotation("", ...) which presumably handles it fine, but it's worth a quick sanity check.

---

PackDetailView.swift — Pack Detail parity

The existing review covers the main points well. Two additional items not yet flagged:

⚠️ Redundant double nil-check in the help button.

if helpResourceName != nil {          // outer guard — always true inside here
    Button {
        if helpResourceName != nil {  // ← always true; can be removed
            showingHomeRowModsHelp = true
        }
    }

The inner check is dead code. Since helpResourceName is a computed var (not a let), the compiler doesn't eliminate it, so it re-evaluates on every tap (harmless but confusing). Drop the inner guard.

Minor: magic padding 12 for WindowSnapping.

embeddedEditor(horizontalPadding: 12) {
    WindowSnappingView(...)

The comment says "Rules uses horizontal padding of 12" but there's nothing binding this to the Rules tab's actual value. If Rules is ever adjusted, Pack Detail silently drifts. Either share a named constant or leave a cross-reference comment with the exact file/line in Rules this mirrors.

Minor: showingHomeRowModsHelp state name is too specific.
The state variable is controlled by helpResourceName which is already pack-agnostic. Naming it showingHelpSheet keeps it open as more packs gain help articles without a rename.

---

Summary

Area	Severity	Item
ConfigReloadCoordinator	⚠️ Medium	isCooldownBlockMessage string-matching fragility; consider typed error
ConfigReloadCoordinator	⚠️ Medium	Confirm 3.2s constant tracks ReloadSafetyMonitor's actual window
GalleryWindowController	Minor	Optional.map as side-effect; prefer if let
MappingTable	Minor	Document macModifiers value format assumption
PackDetailView	Minor	Remove redundant inner if helpResourceName != nil
PackDetailView	Minor	Bind or document the 12 padding constant
PackDetailView	Minor	Rename showingHomeRowModsHelp → showingHelpSheet

The correctness and architecture are solid. The string-matching cooldown detection is the only item I'd want addressed before merge — the rest are polish.

[claude]

Code Review — PR #318: Pack Detail parity with Rules (all 10 packs)

Overview

Good, well-scoped PR. The core idea — having Pack Detail embed the exact same SwiftUI views the Rules tab uses, rather than bespoke LazyVGrid read-only versions — is the right call. The embeddedEditor helper is a clean extraction. The Mission Control redesign (3-modifier chord → Leader + single key) is a genuine UX win.

Raising a handful of issues below ranging from a silent bug to style/naming nits.

---

🐛 Bugs / Correctness

1. Silent double-set of selectedLayerPresetId (PackDetailView.swift)

In bindingsBlock, the closure sets selectedLayerPresetId before launching the Task, then applyLayerPresetEdit sets it again as its first line:

onSelectPreset: { presetId in
    selectedLayerPresetId = presetId          // ← set #1 (sync)
    Task { await applyLayerPresetEdit(...) }
}

func applyLayerPresetEdit(presetId: String, ...) async {
    selectedLayerPresetId = presetId          // ← set #2 (redundant)
    ...
}

This is harmless right now (both values are identical), but applyLayerPresetEdit looks self-contained from its signature. Remove the pre-Task assignment at the call site or the one inside the function — pick one owner.

2. isCooldownBlockMessage is fragile (ConfigReloadCoordinator.swift)

private func isCooldownBlockMessage(_ message: String) -> Bool {
    message.contains("Reload blocked") && message.contains("cooldown")
}

This matches on the kanata server's human-readable error string. If the server wording ever changes ("reload throttled", "Rate limited", etc.), the deferred retry silently stops firing and the user's config write is lost with no error feedback. Consider using a dedicated error code or a constant defined alongside wherever this message is generated, rather than pattern-matching a free-text string.

3. deferredReloadTask has no actor isolation (ConfigReloadCoordinator.swift)

deferredReloadTask is a mutable stored property, and both scheduleDeferredReloadAfterCooldown and its Task closure read/write it without isolation:

private var deferredReloadTask: Task<Void, Never>?

private func scheduleDeferredReloadAfterCooldown() {
    deferredReloadTask?.cancel()              // read + write
    deferredReloadTask = Task { [weak self] in
        ...
        self?.deferredReloadTask = nil        // write from Task body
    }
}

If ConfigReloadCoordinator is not already @MainActor-isolated or actor-wrapped, rapid concurrent reloads can race here. Add @MainActor to the class (or at minimum to these two methods) or make the property nonisolated(unsafe) with a note that access is serialised by the caller — whichever reflects reality.

---

⚠️ Code Quality

4. Redundant nil check in the help button (PackDetailView.swift ~line 135)

if helpResourceName != nil {
    Button {
        if helpResourceName != nil {   // ← dead code, always true here
            showingHomeRowModsHelp = true
        }
    } ...
}

The outer guard already ensures we're inside a non-nil branch. Remove the inner check.

5. showingHomeRowModsHelp is misnamed

The variable is attached to the .sheet and toggled for any pack with a help resource, not just Home Row Mods. Rename to showingPackHelp (or isShowingHelp) so it doesn't need updating when more packs gain help content.

6. Hard-coded 3.2 s cooldown

try? await Task.sleep(nanoseconds: 3_200_000_000)

If the TCP reload cooldown window changes, this constant needs a matching update in a completely different file. Define a shared constant (ConfigReloadCoordinator.cooldownDuration) and reference it in both places.

---

🧪 Test Coverage

7. No test for the deferred-retry path

scheduleDeferredReloadAfterCooldown is non-trivial async logic (cancellation, coalescing, retry). The PR updates PackRuntimeBehaviorTests for the Mission Control redesign — it would be good to add at least one unit test covering: (a) a cooldown-blocked reload triggers a retry, and (b) rapid edits coalesce into one retry rather than N.

8. New applyAutoShiftEdit / applyWindowConventionEdit / applyLayerPresetEdit paths are untested

These are the new install-on-first-touch + config update flows. Even lightweight async unit tests (mock kanataManager, assert VM methods called) would be valuable here given how easy it is for the skipFinalReload dance to break silently.

---

Minor Nits

• expandOutputForMultiWrapping is an instance method but uses Self.convertSingleKeyToForkFormat — it could be static func to signal it has no instance dependency.
• The chord-formatting code in RulesSummaryView+MappingTable.swift calls .components(separatedBy: " ").first to strip the text label from macName. A comment explaining why (the map value includes a full label like "⌘ Cmd") would help a future reader avoid "fixing" it.
• Mission Control key collision avoidance is documented in a comment but is purely manual. As more layer-based packs are added, a collision detector in RuleCollectionCatalog tests would be more reliable than prose comments.

---

Summary

The architecture is sound and the embeddedEditor extraction is clean. Resolve the deferredReloadTask isolation issue and the cooldown string-matching brittleness before merging — both are correctness issues in the reload path. The rest are polish/naming. Good PR overall.

[claude]

Code Review — PR #318: Pack Detail parity with Rules (all 10 packs)

Overview

This PR delivers the intended parity goal and bundles several useful fixes (modifier expansion, deferred reload, overlay suppression, HUD sizing). The core Pack Detail refactor is clean and well-structured. A few things are worth addressing before merge.

---

Bugs / Correctness

1. expandOutputForMultiWrapping — case-sensitivity mismatch
KanataConfiguration+BlockBuilders.swift lines 46–47:

let modifierPrefixes = ["C-", "M-", "A-", "S-", "RA-", "RM-", "RC-", "RS-", "AG-"]
guard modifierPrefixes.contains(where: { trimmed.uppercased().hasPrefix($0) }) else {

The prefix list is already uppercase, but trimmed.uppercased() converts the whole string including the key name ("C-up".uppercased() = "C-UP"). This works for detection but then passes the original trimmed (e.g. "C-up") down to convertSingleKeyToForkFormat — which then re-does uppercasing on the prefix. The two functions use different uppercase strategies. Consider deriving modifierPrefixes directly from the same modifierMap dictionary that convertSingleKeyToForkFormat uses so the prefix list can never drift.

2. isCooldownBlockMessage string-matching is fragile
ConfigReloadCoordinator.swift lines 118–120:

private func isCooldownBlockMessage(_ message: String) -> Bool {
    message.contains("Reload blocked") && message.contains("cooldown")
}

This silently breaks if the Kanata TCP error message format ever changes. The suppressed toast is a real UX behaviour change (user gets no feedback when a write doesn't reach Kanata yet). Recommend a structured error type or at minimum a constant for the expected string, and a comment referencing where that string originates from in the Kanata/TCP layer.

3. scheduleDeferredReloadAfterCooldown — cancellation not checked properly

try? await Task.sleep(nanoseconds: 3_200_000_000)
guard let self, !Task.isCancelled else { return }

Using try? swallows the cancellation error from Task.sleep. The guard !Task.isCancelled check after a successful sleep is a second-best approximation — a rapid sequence of edits during the sleep window may not correctly coalesce. Using try await Task.sleep(...) (without try?) and catching CancellationError explicitly would make the intent clear and the behaviour correct.

4. OverlayAppSuppressor — MainActor.assumeIsolated in notification closure
OverlayAppSuppressor.swift line 68:

queue: .main
) { [weak self] _ in
    MainActor.assumeIsolated {
        self?.applyForCurrentApp()
    }
}

addObserver(forName:object:queue:using:) with queue: .main dispatches the closure to the main queue asynchronously — it does NOT guarantee the closure runs within the @MainActor isolation domain. MainActor.assumeIsolated will crash at runtime if it's called outside a @MainActor context. Use Task { @MainActor in ... } (the same pattern the willCloseObserver in GalleryWindowController uses) instead.

---

Code Quality

5. PR scope
The PR bundles at least 5 independent changes:

• Pack Detail view parity (stated goal)
• OverlayAppSuppressor + Figma suppression feature
• Mission Control chord → Leader redesign
• Deferred reload on cooldown
• Modifier expansion fix + HUD sizing

If any one of these needs a revert, the others come with it. The modifier expansion fix in particular looks like a standalone bug fix that would be safer to merge first.

6. appIcon(for:) is a stub
ExperimentalSettingsSection.swift:

private func appIcon(for _: String) -> String { "app" }

The parameter is ignored. Either resolve this (try NSWorkspace.urlForApplication → NSImage(contentsOf:)) or remove the method and inline the "app" literal so it's obvious it's a placeholder rather than looking like deliberate per-app logic.

7. Help button state naming
PackDetailView.swift:

@State private var showingHomeRowModsHelp = false

But the sheet is gated by helpResourceName != nil, which could theoretically apply to future packs. The variable name ties the mechanism to Home Row Mods specifically. Rename to showingPackHelp to match the abstraction already established by helpResourceName.

8. Redundant nil check in header
PackDetailView.swift lines 451–454:

if helpResourceName != nil {
    Button {
        if helpResourceName != nil { // ← redundant
            showingHomeRowModsHelp = true
        }
    }

The inner if helpResourceName != nil is redundant — the button is already inside the outer guard.

9. 9-element tuple in MappingTableContent call

MappingTableContent(
    mappings: collection.mappings.map {
        ($0.input, $0.output, $0.shiftedOutput, $0.ctrlOutput,
         $0.description, $0.sectionBreak, isInstalled, $0.id, nil)
    }
)

The trailing nil is opaque — what field is it? If MappingTableContent's tuple signature ever changes, this silently compiles to the wrong shape. A named struct or at minimum a comment on the nil would help.

---

Architecture / Design

10. Mission Control behavior change is buried in the PR
The chord → Leader redesign is a user-visible breaking change for existing users who have Mission Control installed (their muscle memory of three-modifier chords is now broken). Consider a migration note or an isSystemDefault: false guard that at least re-confirms the new activation pattern to the user on first use.

11. GalleryWindowController now inherits NSObject
The NSObject inheritance is needed for the NSObjectProtocol conformance required by addObserver. The comment says "same pattern Settings uses" but doesn't note the inheritance change. A one-line comment — or just hoisting the observer to a let held as a closure variable — would keep the diff self-explanatory.

---

Test Coverage

• The Mission Control test is properly updated to match the new Leader design. ✅
• The modifier expansion fix (expandOutputForMultiWrapping, convertSingleKeyToForkFormat) has no new unit tests. Given that the PR comment calls out a prior implementation that "never expanded anything," a small table-driven test (e.g. "C-up" → "(multi lctl up)", "M-S-z" → ..., plain "down" passthrough) would guard against regression.
• OverlayAppSuppressor and isCooldownBlockMessage have no tests. These are low-complexity but their correctness is load-bearing for UX (silently suppressed toasts; overlay disappearing under Figma).

---

Minor Nits

• ExperimentalSettingsSection initialises suppressedBundleIDs from PreferencesService.shared directly rather than via the injected services object already in scope — inconsistent with the rest of that view.
• The positionWindow refactor in ContextHUDController moves let screenFrame earlier, which is a clean improvement. ✅
• embeddedEditor helper in PackDetailView is a nice DRY win. ✅

---

Summary

Area	Status
Pack Detail parity goal	✅ achieved cleanly
Modifier expansion fix	✅ correct approach, needs a unit test
MainActor.assumeIsolated crash risk	⚠️ fix before merge
Cooldown string-matching fragility	⚠️ low risk but worth hardening
Task cancellation in deferred reload	⚠️ subtle but should be fixed
Mission Control breaking change	⚠️ needs a migration note
Stub appIcon	ℹ️ minor
Redundant nil-check	ℹ️ minor

The core Pack Detail refactor is solid. The main items to address before merge are the MainActor.assumeIsolated crash risk in OverlayAppSuppressor, the try?-swallowed cancellation in the deferred reload task, and ideally a unit test for the modifier expansion fix.

🤖 Generated with Claude Code

[claude]

Code Review — PR #318: Pack Detail parity with Rules (all 10 packs)

Overview

This PR achieves true visual and functional parity between Pack Detail and the Rules tab for all 10 packs. The core strategy — replacing bespoke LazyVGrid implementations with the same shared view components Rules uses — is correct and eliminates a drift surface. The PR also ships three unrelated but related features: per-app overlay suppression (Figma default), deferred TCP reload after cooldown, and a modifier-expansion bug fix for release-layer multi actions. All four areas are well-motivated.

---

🐛 Critical Bug — SoundManager.shouldSuppress() infinite recursion

File: Sources/KeyPathAppKit/Utilities/SoundManager.swift, new shouldSuppress() method

private func shouldSuppress() -> Bool {
    if shouldSuppress() { return true }   // ← calls itself → stack overflow
    ...
}

The first line is almost certainly meant to be:

if TestEnvironment.isRunningTests { return true }

The original per-method guard was if TestEnvironment.isRunningTests { ... return }. When this was consolidated into shouldSuppress(), the placeholder call was left as shouldSuppress() instead of TestEnvironment.isRunningTests. As written, every sound call will crash the app at runtime the moment it hits this path.

---

Issues

1. Redundant nil check in the help button (PackDetailView.swift, ~line 452)

if helpResourceName != nil {            // outer guard
    Button {
        if helpResourceName != nil {    // ← always true here, dead check
            showingHomeRowModsHelp = true
        }
    }
}

Drop the inner guard.

2. showingHomeRowModsHelp is misnamed

The state var and the sheet it drives are now the general "pack help sheet" (keyed by helpResourceName), but the name implies Home Row Mods specifically. Rename to showingPackHelp to match its actual scope, especially since helpResourceName is already designed to be extensible.

3. GalleryWindowController: NSObject — unexplained addition

The class now inherits NSObject with no comment explaining why. The NotificationCenter.addObserver(forName:object:queue:using:) block-based API works fine without NSObject. If there's a specific reason (e.g., KVO, isEqual, hash for some identity check), it should be documented. If not, it should be removed — unnecessary NSObject inheritance adds hidden overhead and makes the class @objc-reachable.

4. Mission Control is a breaking behavior change

The mapping changed from lctl+lmet+lalt+{direction} chords to Leader(Space)+{letter}. The PR is self-consistent and the test is updated, but users who already had Mission Control enabled will silently have their shortcuts changed on next install. Consider a migration note in the pack's longDescription or a version bump + changelog entry so users aren't surprised.

5. expandOutputForMultiWrapping — case-sensitive prefix list, case-insensitive check

let modifierPrefixes = ["C-", "M-", "A-", "S-", "RA-", ...]
guard modifierPrefixes.contains(where: { trimmed.uppercased().hasPrefix($0) }) else {

This works, but it's fragile: $0 is already uppercase, so trimmed.uppercased().hasPrefix($0) is equivalent to a case-insensitive match. If a lowercase prefix like "c-" ever crept into the list, the match would fail silently. Either uppercase the list once (let uppercasedPrefixes = modifierPrefixes.map { $0.uppercased() }) or use an explicit case-insensitive .range(of:options:.caseInsensitive) call so intent is unambiguous.

6. ConfigReloadCoordinator.deferredReloadTask race on re-entrant cancel

private func scheduleDeferredReloadAfterCooldown() {
    deferredReloadTask?.cancel()
    deferredReloadTask = Task { [weak self] in
        try? await Task.sleep(...)
        guard let self, !Task.isCancelled else { return }
        ...
        self.deferredReloadTask = nil
    }
}

If scheduleDeferredReloadAfterCooldown is called twice in rapid succession, the second call cancels the first task but the first task may have already passed the !Task.isCancelled guard (co-operatively cancelled tasks only check cancellation at suspension points). The try? await Task.sleep does throw CancellationError, so guard !Task.isCancelled at the end is actually redundant. The sleep's try? already ensures the body after it won't run on cancellation. Minor, but clarifying the comment would help future readers.

---

Positive notes

• embeddedEditor wrapper is a clean DRY reduction — 5 separate VStack+Divider patterns collapsed into one modifier with consistent opacity animation. Well done.
• OverlayAppSuppressor is cleanly isolated, uses MainActor.assumeIsolated correctly in the queue: .main block, and the two notification center instances (NSWorkspace.shared.notificationCenter vs NotificationCenter.default) are correctly matched in both start() and stop().
• isCooldownBlockMessage + deferred reload is the right fix: silent data loss on cooldown was a real user-visible regression. The coalescing via deferredReloadTask?.cancel() is correct.
• The modifier expansion logic in KanataConfiguration+BlockBuilders correctly identifies the root cause (prefix notation vs value matching) and the comment explaining the kanata release-layer+multi interaction is exactly the kind of comment CLAUDE.md asks for.
• refreshInstallState seeding autoShiftConfig and selectedLayerPresetId from live collection on appear is the right pattern and mirrors homeRowModsConfig.

---

Summary

The pack parity work is correct and ready. Block on the SoundManager.shouldSuppress() infinite recursion — it will crash any user who plays a sound while in a suppressed app (or any time the app check fires). The other items are improvements but not blockers.

[claude]

Code Review — PR #318: Pack Detail parity with Rules (all 10 packs)

Overview

This PR does solid work: it closes 4 pack-detail/Rules gaps, introduces per-app overlay suppression (Figma default), redesigns Mission Control from 3-modifier chords to Leader + single key, fixes a silent Kanata multi-wrapping bug, and adds deferred reload on cooldown blocks. The structural direction is correct. Several items below need a look before merge.

---

Bugs / Correctness

1. convertSingleKeyToForkFormat uses $0.prefix — verify modifierMap type

KanataConfiguration+BlockBuilders.swift line ~67:

if let (prefix, modKey) = modifierMap.first(where: { uppercasedKey.hasPrefix($0.prefix) }) {

The old code used $0.key.lowercased() (dictionary key accessor). The new code uses $0.prefix, which only compiles if modifierMap is a collection of structs/named-tuples with a .prefix property — not a plain Dictionary. This diff doesn't show whether modifierMap's type changed. If it's still a [String: ...] dict this is a compile error masked by the test run not covering that specific path. Please confirm with a grep or show the declaration.

2. Cooldown block detection via string matching is fragile

ConfigReloadCoordinator.swift:

private func isCooldownBlockMessage(_ message: String) -> Bool {
    message.contains("Reload blocked") && message.contains("cooldown")
}

If Kanata ever changes this error string the deferred-retry silently stops working and the user's config write is lost with no feedback. A typed error enum or a sentinel constant would be more resilient. At minimum, add a comment pointing to the exact Kanata source string this matches.

3. deferredReloadTask has a potential data race

ConfigReloadCoordinator is a final class (not an actor). deferredReloadTask is mutated from scheduleDeferredReloadAfterCooldown() and from inside the Task closure (self.deferredReloadTask = nil). If ConfigReloadCoordinator isn't @MainActor-bound or Sendable-safe, this is a Swift concurrency data race. Add @MainActor to the class or make the property atomic.

Also minor: the try? on Task.sleep silences CancellationError, so the guard !Task.isCancelled check is the correct continuation guard — but the pattern is subtle. A comment explaining why try? is used here (to let the guard handle it) would help future readers.

---

Code Quality

4. Redundant nil-check inside Button action (PackDetailView.swift ~line 454):

if helpResourceName != nil {
    Button {
        if helpResourceName != nil {   // ← already guarded by the outer `if`
            showingHomeRowModsHelp = true
        }
    }

Remove the inner guard.

5. showingHomeRowModsHelp is too narrow a name

The helpResourceName computed property is generic (returns nil for non-HRM packs today, but is designed to expand). The state var and sheet are also generic. Rename to showingHelp to match the abstraction level.

6. appIcon(for:) is a misleading stub (ExperimentalSettingsSection.swift):

private func appIcon(for _: String) -> String {
    "app"   // always ignores its argument
}

Either inline "app" at the call site and delete this function, or add a // TODO: comment. A named function that ignores its argument suggests real behavior that doesn't exist.

7. .map used for side-effects (GalleryWindowController.swift, OverlayAppSuppressor.swift):

activationObserver.map(NSWorkspace.shared.notificationCenter.removeObserver)

Optional.map is for transforming values, not executing side effects. This works but is unconventional and trips up readers. Use if let instead.

8. 9-element unnamed tuple in MappingTableContent (PackDetailView.swift ~line 609):

mappings: collection.mappings.map {
    ($0.input, $0.output, $0.shiftedOutput, $0.ctrlOutput,
     $0.description, $0.sectionBreak, isInstalled, $0.id, nil)
}

Positional 9-tuples are fragile — if MappingTableContent's init signature changes, this silently passes values in the wrong slots. This should use a named struct/typealias or at minimum named tuple labels.

---

Performance

9. SoundManager.shouldSuppress() hits NSWorkspace on every sound event

if let front = NSWorkspace.shared.frontmostApplication?.bundleIdentifier,
   PreferencesService.shared.overlaySuppressedBundleIDs.contains(front)

NSWorkspace.frontmostApplication does a synchronous process-info lookup on every call. SoundManager plays sounds in tight sequences (layer-up → layer-down). The lookup result could be cached at app-activation granularity via OverlayAppSuppressor and queried as a simple Bool flag here. Low priority for now, but worth noting.

---

Architecture / Design

10. OverlayAppSuppressor uses MainActor.assumeIsolated in notification callbacks

activationObserver = NSWorkspace.shared.notificationCenter.addObserver(
    ...
    queue: .main
) { [weak self] _ in
    MainActor.assumeIsolated {
        self?.applyForCurrentApp()
    }
}

queue: .main guarantees the block runs on the main queue, so MainActor.assumeIsolated is correct here. This is fine as-is, just flagging for awareness: if the queue ever changes, this becomes an unsafe force-assumption. A comment like // queue: .main above makes this safe would help.

11. GalleryWindowController now inherits from NSObject — document why

The class gained : NSObject as part of this PR. The comment on showWindow explains what the overlay hide does, but doesn't explain why NSObject inheritance was added. (Was it required for the NotificationCenter observer pattern? The observer is a closure so that shouldn't be the reason.) A one-line comment on the class declaration clarifying this avoids the next reader wondering.

---

Mission Control redesign — behavioral note

The switch from 3-modifier chords to Leader + single key is a big UX improvement. Two things to verify in testing:

• Conflict with Vim Navigation when both packs are enabled: both claim Space → nav layer. The comment says the generator deduplicates, but that path should be explicitly tested (enable both packs; confirm nav layer activates correctly; confirm m/e/[/]/b/c don't clobber Vim Nav's nav-layer keys).
• c key in nav layer: Vim Navigation's collision-avoidance comment lists avoided keys, but c isn't in the explicit Vim Nav list. Verify c (Notification Center) doesn't clash with any Vim Nav command.

---

Test Coverage

The updated testMissionControlLeaderMEmitsCtrlUp correctly tests the new design. The expandOutputForMultiWrapping logic doesn't appear to have a direct unit test — a table-driven test covering C-up → "(multi lctl up)", M-S-z → ..., plain "down" → "down", and "(push-msg...)" → pass-through would prevent future regression.

---

Summary

Must fix before merge: Items 1, 2, 4.
Should fix: 3, 5, 6, 7, 8.
Nice to have: 9, 10, 11, and the expandOutputForMultiWrapping unit test.

The core Pack Detail parity work and the suppressor architecture are solid. The Mission Control redesign is a genuine improvement. Clean up the fragile spots above and this is in good shape.

🤖 Generated with Claude Code