refactor: extract SessionAutosaveCoordinator from AppDelegate - #217
Merged
Conversation
Autosave owned seven stored properties and ten methods on AppDelegate, tangled in among focus repair, window routing and shortcut dispatch. Its state is genuinely self-contained -- a timer, a debounce flag, a deferred-retry flag, a content fingerprint, and the last-typing timestamp -- so it moves out cleanly. Dependencies go in as closures (snapshot provider, save, terminating, XCTest-detection) rather than a back-reference to AppDelegate. A back-reference would have relocated the coupling rather than removed it, and would have dragged in the window-context dictionary that deliberately is not being extracted. Behaviour is unchanged: intervals, quiet period and retry semantics all move verbatim. The three pure policy statics move with it; saveSessionSnapshot, which is not autosave-only, now calls one of them across types. Two details worth knowing: AppDelegate holds the coordinator as a non-optional `let` so the keystroke path stays cheap -- a lazy var would add a check on every keyDown. Swift forbids capturing self in a closure before super.init() completes, so it is built with inert placeholders and reconfigured as the statement immediately after super.init(). Nothing can observe the gap: no event loop turn happens inside init, and recordTypingActivity is a plain property write that does not touch the injected closures. recordTypingActivity stays a single property write, now reached through one extra property hop from WindowSwizzles.programa_sendEvent. Adds tests for orchestration that previously had none -- only the pure statics were covered. Injecting the dependencies is what made the timer, debounce, deferred retry and quiet-period deferral drivable at all. Refs #187
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #187's AppDelegate half. Only this one of the three proposed extractions — see below.
What this does
Autosave owned seven stored properties and ten methods on
AppDelegate, sitting among focus repair, window routing and shortcut dispatch. Its state is genuinely self-contained — a timer, a debounce flag, a deferred-retry flag, a content fingerprint, and the last-typing timestamp — so it moves out cleanly.AppDelegate.swiftloses ~225 lines.Behaviour is unchanged. Intervals, quiet period, retry semantics all move verbatim. This is a move, not a redesign.
Why the other two extractions aren't here
The issue proposes
SessionAutosaveCoordinator,MainWindowRegistryandShortcutRouter. Investigation found only the first is a clean self-contained change, and the repo owner decided to do only that one.MainWindowRegistry— the public surface is narrow (4 accessors, ~35 external call sites), butmainWindowContextsis touched at 60+ sites spanning ~4,300 lines, interleaved with focus repair, session snapshotting and window lifecycle. Extracting it in one PR yields a registry that still needs AppDelegate-shaped context injected back in, which relocates the coupling rather than removing it. It's a multi-PR sequence or nothing.ShortcutRouter—Sources/ShortcutRouting.swift(#208) already extracted the only genuinely pure logic. What remains is ~55 flatif matchConfiguredShortcut(…) { perform effect; return true }branches, roughly 15-20% decision and the rest effect, fused per branch. Turning that into a pure router means rewriting every branch to return an intent plus a second dispatch step — a redesign of a precedence-ordered table with documented ordering-sensitivity, in exactly the code #186's tests exist to protect.Worth re-scoping the issue rather than forcing those.
Design
Dependencies are injected as closures — snapshot provider, save, terminating, XCTest-detection — rather than a back-reference to
AppDelegate. A back-reference would have dragged in the very window-context dictionary that deliberately isn't being extracted.Two details worth a reviewer's attention:
AppDelegateholds the coordinator as a non-optionalletso the keystroke path stays cheap — alazy varwould add a check on every keyDown, which is the sort of cost #183 has spent today removing. Swift forbids capturingselfin a closure beforesuper.init()completes, so it's constructed with inert placeholders and reconfigured as the statement immediately after. The gap is unobservable: no event-loop turn happens insideinit, andrecordTypingActivityis a plain property write that never touches the injected closures, so it behaves correctly even mid-gap.Two dependencies beyond the obvious ones: XCTest-detection, because the timer branches on it and omitting it would change behaviour; and the existing
sessionPersistenceQueuepassed in rather than duplicated, since it's shared with non-autosave save paths.Hot path
recordTypingActivity()runs on every keyDown fromprograma_sendEvent. Still a single property write, now one extra property hop away.Test plan
Orchestration had no coverage before — only the three pure policy statics were tested. Injecting the dependencies is what made the rest drivable.
SessionPersistenceTestsstatic-policy tests still pass, retargeted to the new typexcodebuild -scheme programa→** BUILD SUCCEEDED **(verified from log contents, independently re-run)-scheme programa-unit build-for-testing→** TEST BUILD SUCCEEDED **, both new files confirmed compiled