Repository navigation
Extract Ghostty trigger decoding + config-store reload from AppDelegate into CmuxGhosttyShortcutDecoding - #6183
azooz2003-bit wants to merge 1 commit into
Conversation
…te into CmuxGhosttyShortcutDecoding Pull the byte-identical pieces of AppDelegate's configuration-reload domain into a new leaf package, CmuxGhosttyShortcutDecoding (depends on nothing). What moved: - The ~85-line ghostty-trigger -> shortcut decode logic from storedShortcutFromGhosttyTrigger becomes a tested value-object decoder: GhosttyTriggerShortcutDecoder over GhosttyTriggerInput / GhosttyTriggerPhysicalKey / GhosttyModifierMask, producing GhosttyTriggerShortcut. The glyph table (←/→/↑/↓, letters, digits, punctuation), the catch-all rejection, the unicode-scalar lowercasing, and the "bogus empty trigger" rule all live in the package now. - reloadCmuxConfigStores becomes CmuxConfigStoreReloadCoordinator (@mainactor @observable), which sequences per-window store loadAll() (deduped by object identity) and the window-title refresh through two injected protocol seams, CmuxConfigStoreReloading and CmuxConfigStoreReloadEnvironment. Seams (constructor-injected, no god-type reach in the package): - CmuxConfigStoreReloading: CmuxConfigStore conforms (empty extension; existing loadAll() satisfies it). - CmuxConfigStoreReloadEnvironment: AppDelegate conforms, exposing its mainWindowContexts stores and refreshWindowTitlesAcrossMainWindows() behind a read-only seam. The coordinator holds the environment WEAKLY (AppDelegate owns the coordinator) so there is no retain cycle. App side stays byte-identical: - storedShortcutFromGhosttyTrigger is now a thin adapter: lift the C ghostty_input_trigger_s into GhosttyTriggerInput, decode, map back onto the app-target StoredShortcut. The GhosttyKit-boundary key translation lives in a new app-target file (GhosttyTriggerPhysicalKey+GhosttyKit.swift) as a failable init, so AppDelegate.swift shrinks rather than grows. - reloadCmuxConfigStores forwards to the coordinator; same iteration/dedupe order, same refresh, same #if DEBUG cmuxDebugLog("cmuxConfig.reload source=... stores=...") via the coordinator's onReload hook. Menu-item wiring (menuNeedsUpdate, configureReloadConfigurationMenuItem) and the quit-warning / goto-split stored-property reads were left in the app target: they are bound to NSMenu/NSApp.mainMenu, the app-target KeyboardShortcutSettings, and the keystroke-critical handleCustomShortcut path, so a byte-identical thin forward is not sound without dragging AppKit/GhosttyKit into a package. Tests: 13 behavior tests (decoder glyphs/mods/rejections over all physical keys; coordinator dedupe/refresh/report and weak-environment teardown) using fakes for the seams. Package wired into cmux.xcodeproj (6 pbxproj entries mirroring CmuxControlSocket); AppDelegate.swift budget ratcheted 17778 -> 17729. NOTE: full app build + tests validated by GitHub CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 4 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extracts the Ghostty trigger-decoding and config-store reload orchestration from
Confidence Score: 4/5This is a structural refactor that is byte-identical to the original behavior; the new package has no external dependencies and 13 passing tests covering all decode paths and coordinator edge cases. The extraction is clean and well-tested. Two issues in CmuxConfigStoreReloadCoordinator.swift are worth addressing before merge: public import Observation inadvertently re-exports the Observation module to all consumers of the package, and @observable is applied to a class with no trackable stored state — neither causes a runtime regression today, but the first quietly widens the package's API surface. Packages/CmuxGhosttyShortcutDecoding/Sources/CmuxGhosttyShortcutDecoding/CmuxConfigStoreReloadCoordinator.swift — the public import and @observable annotations. Important Files Changed
Sequence DiagramsequenceDiagram
participant AD as AppDelegate
participant CSRC as CmuxConfigStoreReloadCoordinator
participant Env as CmuxConfigStoreReloadEnvironment (AppDelegate)
participant Store as CmuxConfigStore (×N, deduped)
AD->>CSRC: reload(source:)
CSRC->>Env: reloadableConfigStores
Env-->>CSRC: [CmuxConfigStoreReloading]
loop each distinct store (ObjectIdentifier dedup)
CSRC->>Store: loadAll()
end
CSRC->>Env: refreshWindowTitlesAfterConfigReload()
CSRC->>AD: "onReload(source, storeCount) [#if DEBUG]"
Note over AD: storedShortcutFromGhosttyTrigger
AD->>AD: GhosttyTriggerPhysicalKey(ghosttyPhysicalKey:)
AD->>CSRC: GhosttyTriggerShortcutDecoder.decode(GhosttyTriggerInput)
CSRC-->>AD: GhosttyTriggerShortcut?
AD-->>AD: StoredShortcut?
Reviews (1): Last reviewed commit: "Extract Ghostty trigger decoding + confi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,54 @@ | |||
| public import Observation | |||
There was a problem hiding this comment.
Observation
With InternalImportsByDefault enabled in Package.swift, public import Observation re-exports the Observation module to every importer of CmuxGhosttyShortcutDecoding. This means app-target files that import CmuxGhosttyShortcutDecoding will silently gain access to withObservationTracking, Observable, and the rest of the Observation API without explicitly importing it — an unintended expansion of the package's public API surface. A plain import Observation (which defaults to internal under InternalImportsByDefault) is sufficient: @Observable only needs Observation at compile time for the coordinator itself, not at the use sites of the package.
| @Observable | ||
| public final class CmuxConfigStoreReloadCoordinator { |
There was a problem hiding this comment.
@Observable with no observable stored state
CmuxConfigStoreReloadCoordinator is decorated @Observable, but neither of its stored properties — environment (a weak existential set once in init) and onReload (a let closure) — represent state that mutates after initialization in a way that observation tracking would surface. The @Observable macro synthesizes an _$observationRegistrar and per-property access / mutation hooks that will never fire, adding dead boilerplate with no current SwiftUI observer. If this is a placeholder for future observable state (e.g. a reload-in-progress flag), a comment explaining the intent would help; otherwise the annotation is misleading.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Superseded by #6227 — consolidated into the existing CmuxTerminalCore package (no new micro-package) per over-engineering review. The extraction is preserved there. |
This branch was successfully deployed
1 participant
Pulls the byte-identical, cleanly-separable pieces of AppDelegate's configuration-reload domain into a new leaf package,
CmuxGhosttyShortcutDecoding(no dependencies).What moved
storedShortcutFromGhosttyTriggerbecomes a tested value-object decoder:GhosttyTriggerShortcutDecoderoverGhosttyTriggerInput/GhosttyTriggerPhysicalKey/GhosttyModifierMask, producingGhosttyTriggerShortcut. The glyph table (←/→/↑/↓, letters, digits, punctuation), catch-all rejection, unicode-scalar lowercasing, and the "bogus empty trigger" rule now live in the package.reloadCmuxConfigStoresbecomesCmuxConfigStoreReloadCoordinator(@MainActor @Observable), which sequences per-window storeloadAll()(deduped by object identity) and the window-title refresh through injected seams.Seams (constructor-injected, no god-type reach in the package)
CmuxConfigStoreReloading:CmuxConfigStoreconforms via an empty extension (its existingloadAll()satisfies it).CmuxConfigStoreReloadEnvironment:AppDelegateconforms, exposing itsmainWindowContextsstores andrefreshWindowTitlesAcrossMainWindows()behind a read-only seam. The coordinator holds the environment weakly (AppDelegate owns the coordinator), so no retain cycle.Byte-identical notes
storedShortcutFromGhosttyTriggeris now a thin adapter: lift the Cghostty_input_trigger_sintoGhosttyTriggerInput, decode, map back onto the app-targetStoredShortcut. The GhosttyKit-boundary key translation lives in a new app-target file (GhosttyTriggerPhysicalKey+GhosttyKit.swift) as a failable init, soAppDelegate.swiftshrinks instead of growing.reloadCmuxConfigStoresforwards to the coordinator: same iteration/dedupe order, same title refresh, same#if DEBUG cmuxDebugLog("cmuxConfig.reload source=... stores=...")via the coordinator'sonReloadhook.Left in the app target (intentional)
Menu-item wiring (
menuNeedsUpdate,configureReloadConfigurationMenuItem) and the quit-warning / goto-split stored-property reads were not extracted: they are bound toNSMenu/NSApp.mainMenu, the app-targetKeyboardShortcutSettings, and the keystroke-criticalhandleCustomShortcutpath, so a byte-identical thin forward is not sound without dragging AppKit/GhosttyKit into a package. The largest clean subset (the decode logic + config-store reload orchestration) was extracted instead.Tests
13 behavior tests with fakes for the seams: decoder glyphs/mods/rejections over every physical key; coordinator dedupe/refresh/report and weak-environment teardown.
swift build+swift testgreen in the package.Stats
cmux.xcodeproj(6 pbxproj entries mirroringCmuxControlSocket).AppDelegate.swiftbudget ratcheted 17778 → 17729.NOTE: full app build + cmux test targets are validated by GitHub CI.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracts Ghostty trigger-to-shortcut decoding and config-store reload orchestration out of AppDelegate into a new leaf package,
CmuxGhosttyShortcutDecoding, keeping behavior byte-identical while adding focused tests and simplifying app wiring.GhosttyTriggerShortcutDecoderoverGhosttyTriggerInput/GhosttyTriggerPhysicalKey/GhosttyModifierMask, producingGhosttyTriggerShortcutwith the same glyph table, unicode lowercasing, catch-all rejection, and empty-trigger rule.CmuxConfigStoreReloadCoordinator(@MainActor @Observable) to dedupeloadAll()across windows and refresh titles via injected seams:CmuxConfigStoreReloadingandCmuxConfigStoreReloadEnvironment(held weakly,onReloadfor debug).AppDelegatenow liftsghostty_input_trigger_sintoGhosttyTriggerInput, decodes, and maps toStoredShortcut; reloads forward to the coordinator. Bridge added inGhosttyTriggerPhysicalKey+GhosttyKit.swift;CmuxConfigStoreconforms toCmuxConfigStoreReloading.Written for commit 73aebaa. Summary will update on new commits.