refactor: improve file organization and testability - #87
Open
itsjoshpark wants to merge 18 commits into
Open
Conversation
Every file in the project was tracked by hand: adding one meant writing a PBXFileReference, a PBXBuildFile, a group child entry, and a build phase entry, and every such edit is a merge conflict waiting to happen. That cost is about to be paid a dozen times over by the file reorganization that follows. Move the Front Row and Front Row Tests folders to PBXFileSystemSynchronizedRootGroup, which requires project format 77, so membership now follows what is on disk. FrontRowInfo.plist stays a plain reference in the main group because it lives at the repository root, outside the synchronized folder. FrontRow.entitlements is inside the folder and is listed as a membership exception so it keeps being consumed via CODE_SIGN_ENTITLEMENTS rather than copied into the bundle, matching the behavior it had when it sat in a group with no build phase entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
Extensions.swift had become a junk drawer: drop handling, NSWindow access, size math, media-selection identifiers, float comparison and timecode formatting shared one file for no reason beyond all being extensions. Split it so each file names what it is about, and drop the general-purpose View.if along the way - its single caller used it to force-unwrap fileURL, which a navigationDocument(ifLocal:) helper does without the unwrap. Pull the opposite direction where types were split that belong together. SecurityScopedBookmarkProvider joins the protocol it implements, matching how MountedVolumes already keeps its protocol and implementation in one file. AlertScene and UnopenableRecentFile move next to the alert that is the only thing that reads them, leaving PresentedViewManager as just the presentation state. WindowID moves to WelcomeWindowCoordinator, the only code that opens and dismisses those scenes. Also collapse the byte-identical showOpenFileDialog() that WelcomeView and FileCommands each carried into the one place the other open helpers live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
AGENTS.md asks for new View structs rather than computed properties, and eleven of them had accumulated across the controls bar, the welcome window and two Commands bodies. The worst were the skip buttons: five near-identical switch branches each, seventy lines that differed only in an SF Symbol name. Introduce SkipInterval, a closed enum over the four intervals the symbols exist for, and both switches collapse into one SkipButton that derives its symbol from the interval. The raw values are the same integers the old Picker offered, so the stored SkipInterval preference reads back unchanged. Extract the pieces both menus wanted so they can't drift: PlaybackSpeedButton is shared by the menu bar and the controls bar with the shortcuts left to the caller, and SubtitlePicker/AudioTrackPicker replace two copies each of the same picker. Subtitle tracks are now filtered by the containsOnlyForcedSubtitles media characteristic rather than by matching "Forced" in the display name. The name is localized, so the old test only ever worked on English media; a track merely called "Forced" without the characteristic will now be offered. Also swap PlainButtonStyle() for .plain per the static-member rule, and String(format: "%.2f×") for a FormatStyle, which gets the speed a localized decimal separator it never had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
PlayEngine and ContentView had grown logic that no test could reach: a timecode parser, a playback-speed clamp, window-fitting geometry, and the rules for when the player's chrome hides itself. All four are decisions rather than I/O, and all four were reachable only through AVPlayer or a real window. Follow what ResumePolicy already established and give each one a type of its own, with tests. Timecode.parse keeps the old reading exactly, including that a component which isn't a number counts as zero, so only a string with nothing usable in it is refused. PlaybackSpeed.clamped replaces a three-branch setter that was a clamp to 0.05...2.0 written out longhand, with the snap onto 1.0 kept because stepping up and back down by 5% has to land on normal speed or the controls bar keeps showing an indicator for a file playing normally. VideoWindowLayout.frame takes the centering and shrinking math, leaving fitToVideoSize to ask for a frame and apply it - and to stop force-unwrapping NSScreen.main while it is there. PlayerChromeVisibility takes the hover and idle rules that were spread across three handlers and a Timer in ContentView. It decides only whether the controls, titlebar and pointer should show; the view keeps the AppKit poking. That makes the cases worth being sure about checkable: reaching for the titlebar reads as leaving the window, and must not take the chrome away just as it is being aimed at; going idle must never hide a pointer that is already over another app. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
presentOpenFilePanel force-unwrapped NSApplication.shared.mainWindow to sheet the panel onto it. Opening a file is also how the app starts, and at that point there may be no main window at all - so the crash is reachable from the welcome window, which is the very screen whose job is to open a file. Sheet it where there is a window and present it unattached where there isn't. SeekSliderCell force-cast its controlView to NSSlider on every knob redraw; fall back to the superclass rect instead. asset != nil followed by asset! becomes optional chaining. Delete what nothing calls: removeRemoteCommandHandlers and removePeriodicTimeObserver, both of which tear down state owned by singletons that live as long as the process, the write half of isPresentingUnopenableRecentFileAlert, and the four AnyDropDelegate members that were never set. Drop public from an app target's own types, where it means nothing and the newer files already go without. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
AGENTS.md rules out old-style GCD, and PlayEngine was built on it: five Combine sinks, each hopping through DispatchQueue.main to land back on the actor the class is already isolated to. The hop was load-bearing rather than decorative - the sink closures are main-actor-inferred, so delivering them anywhere else would have been undefined - but consuming the same KVO publishers as async sequences from a main-actor Task gets the isolation from the language instead of from a queue. Subscription now happens on the next turn of the loop rather than synchronously, which costs nothing: publisher(for:) republishes the current value to a new subscriber, so a status that arrived in between is still seen. Two dispatch references survive on purpose, both commented where they are. AVPlayerItemDidPlayToEndTime stays on Combine because Notification isn't Sendable and so can't be carried to the actor by an async sequence, and addPeriodicTimeObserver has no async form at all - naming the main queue is the only way to say where it should call back. WindowAccessor's DispatchQueue.main.async, which only existed to wait for the view to be put in a window, becomes a Task. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
Views took PlayEngine from the environment and then called PlayEngine.shared anyway, which made the injection decorative and the #Preview blocks a lie - they set up an environment nothing read. Use what was passed in, and give the welcome window the engine too, since the Open URL sheet it presents now expects it. Commands bodies can't read the environment, so they keep the shared instances - but as stored properties, the way PlaybackCommands already did, rather than a PlayEngine.shared lookup at each of a dozen use sites. Two places genuinely can't be injected and now say so: the seek slider's cell, which AppKit instantiates itself through cellClass, and the player view's own mouse handling, which has no SwiftUI environment to read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
Both time labels picked their colour with the same inline conditional, one of which was long enough to need wrapping. Put the choice on PlayerControlColor, where the two constants already live, and both call sites become a single readable line. Also straighten the fit test in VideoWindowLayout into a named condition, and drop a @bindable shadow in ContentView that no longer had a binding to make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
The two workflows had converged on identical steps, differing only in name and trigger. Fold the pull_request trigger into CI and drop the duplicate so pushes and PRs run the same single job.
Opening a file left the controls disabled and the window at its old size. The engine's observers received the first value of each property and then nothing, so the item's move to readyToPlay never arrived - isLoaded stayed false, and presentationSize never reached fitToVideoSize. The cause is publisher(for:).values, introduced when these observers moved off Combine. A KVO publisher emits whenever the property changes and pays no attention to demand, while AsyncPublisher only keeps what arrives with a request outstanding; anything landing while the loop is busy is dropped. The duplicate filter then guaranteed the lost value was never re-sent, so the player waited forever on a status it had already passed. Bridge KVO into an AsyncStream with unbounded buffering instead, so a change can arrive late but never go missing, and dedupe in the loop where the previous value is known. The stream is seeded from the caller's context rather than through the .initial option, keeping the read on the actor that owns the object. Verified against real media rather than by inspection: a 640x360 file now sizes the window to 640x360 and enables the controls, and a 3840x2160 file shrinks to 1876x1055 with its aspect ratio intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
The unit tests can't reach the app as a running program: whether the welcome window opens, whether the menu bar is wired up, or whether a sheet presents without bringing the app down. That last one matters most, because a view reading an environment value that isn't there traps at runtime and compiles perfectly well - the Open URL sheet does exactly that, and its test fails with a crash if the welcome scene stops carrying the engine. Each launch shadows RecentDocuments through the argument domain, which overrides reads without writing to disk, so the welcome window starts in a known state and the suite leaves the recent files of whoever ran it alone. Sparkle is silenced so an update check can't take focus mid-test. Note that UI tests need a signed runner: macOS refuses to launch an unsigned one, so CODE_SIGNING_ALLOWED=NO no longer covers the whole scheme. Ad-hoc signing is enough and needs no certificate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
macOS refuses to launch an unsigned UI test runner - it reports the runner as damaged - so CODE_SIGNING_ALLOWED=NO stopped covering the whole scheme once a UI test target joined it. Ad-hoc signing satisfies the launch and needs no certificate, which is what the runner has to work with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
The player's controls and its whole Playback menu stayed disabled with a file open. The KVO bridge was taking each new value from the change dictionary, where KVO stores it as an NSNumber - and an NSNumber won't cast back to an imported @objc enum, so change.newValue was nil for AVPlayerItem.Status and AVPlayer.TimeControlStatus and every one of those changes was dropped on the floor. A CGSize bridges cleanly, which is why presentationSize kept working and the window resized correctly. That masked the rest: the item was never seen becoming ready, so isLoaded stayed false and everything gated on it stayed grey, and the play button never turned into a pause button because the time control status never moved either. Read the property back off the object instead, which is correctly typed whatever the value is. Verified against a real file: the Playback menu now enables Restart, Go to Time, frame stepping and skipping, and reports Pause while playing, matching what main does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
This reverts f334493 along with the two fixes it needed, 19cdd96 and ade7117. The rewrite was made to satisfy the rule against old-style GCD, and it replaced observers that worked. It then broke playback twice: AsyncPublisher drops KVO values that arrive while the consumer isn't waiting, and change.newValue is nil for an imported @objc enum, so the player twice failed to notice its item had become ready and left every control disabled. Neither failure was visible by reading the code, and the second one hid behind the first. The rule it was chasing was never fully satisfied either - the play-to-end notification and the periodic time observer both had to keep their dispatch queues regardless. Paying two regressions for a partial style win is the wrong trade, so take the working Combine observers back. Verified against real media: the Playback menu enables and reports Pause while playing, a 640x360 file sizes the window to 640x360, and a 3840x2160 file shrinks to 1876x1055 with its aspect ratio intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
itsjoshpark
changed the title
WindowAccessor hops through DispatchQueue.main.async only to let the view be added to a window before reading it. Nothing here needs a queue, and the callback already runs on the main actor, so a Task says the same thing without reaching for GCD. This came along with the async-sequence rewrite and was reverted with it, but it stands on its own: unlike the player's observers, no delivery semantics are involved - the closure runs once, and whether the window is there yet is already checked. Verified with a real file: the welcome window still comes up, and a 640x360 video still sizes the window to 640x360, which is what depends on this callback landing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
The rule read as an absolute ban, which invited migrating working code to satisfy it. That is what happened on this branch: the player's Combine observers were rewritten onto async sequences and broke playback twice, because the replacements differ in delivery semantics in ways no diff or compiler shows. Every other rule in that list is a syntactic swap that can be checked by reading. This one is not, so say what it actually wants - modern concurrency in new code - and say to leave working GCD alone. Note that some APIs only take a queue, since a few here have no async form at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
This also reverts c06d6fb, the ad-hoc signing that existed only so the runner could launch. Nine of the ten tests asserted that SwiftUI renders what the code plainly says - that a button exists, that Play is disabled with nothing open - which review already catches. They also matched English string literals in an app localized into twenty-four languages, so a wording change would have failed them for no defect at all. The tenth was worth having: the Open URL sheet reads the engine out of the environment, and a scene that stops providing it traps at runtime while compiling perfectly. That coverage goes with this, and there is no cheap replacement, since a Scene's environment can't be reached from a unit test. It isn't worth a second target, a signed runner and forty-five seconds of every CI run to keep. CI goes back to CODE_SIGNING_ALLOWED=NO, which is all the unit tests need - they load into the host app rather than launching a runner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
The gated check-in workflow was folded into CI earlier on this branch, but CONTRIBUTING still described the process under the old name, so the only document telling a contributor what runs on their PR pointed at a workflow that no longer exists. Also mention that the workflow runs tests, which it always did and this never said. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0141zFyVb2dxExwh7vgkXPue
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
2 participants
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.
Description:
The recent work —
RecentDocumentsStore,ResumePolicy,BookmarkProviding,UnopenableRecentFileAlert— set a clear standard: policy pulled out of I/O, protocols for the untestable parts, real tests. The older layer never caught up. This brings it in line, one concern per commit.build:file-system synchronized groupsThe project was on format 56, so every file was tracked by hand in three places — a
PBXFileReference, aPBXBuildFile, a group entry and a build phase entry apiece, and a merge conflict every time two branches added a file. Moving the two target folders to synchronized groups means membership follows what's on disk. Adding, renaming and deleting files no longer touchesproject.pbxprojat all.FrontRowInfo.pliststays a plain reference because it lives at the repository root, outside the synchronized folder.FrontRow.entitlementsis a membership exception so it keeps being consumed viaCODE_SIGN_ENTITLEMENTSrather than copied into the bundle.refactor:one concern per fileExtensions.swifthad become a junk drawer — drop handling,NSWindowaccess, size math, media-selection identifiers, float comparison and timecode formatting in one file. Split intoTimecode,PlaybackSpeed,VideoWindowLayout,MediaSelection,MediaFileDropandWindowAccessor.Pulled the other way where types that belong together were apart:
SecurityScopedBookmarkProviderjoins the protocol it implements, the wayMountedVolumesalready does;AlertSceneandUnopenableRecentFilemove next to the alert that is the only thing reading them;WindowIDmoves toWelcomeWindowCoordinator. The byte-identicalshowOpenFileDialog()thatWelcomeViewandFileCommandseach carried is now one function.View.ifis gone. It had one caller, which used it to force-unwrapfileURL; anavigationDocument(ifLocal:)modifier does the same job without the unwrap.refactor:view structs instead of computed propertiesEleven computed properties across four files, against AGENTS.md. The skip buttons were the worst: five near-identical switch branches each, seventy lines differing only in an SF Symbol name. A new
SkipIntervalenum collapses both into oneSkipButtonthat derives its symbol from the interval — and being a closed set, the symbol is guaranteed to exist. The raw values are unchanged, so the stored preference reads back as before.PlaybackSpeedButton,SubtitlePickerandAudioTrackPickerare now shared by the menu bar and the controls bar instead of written twice each.refactor:+ tests — the decisions that had no testsTimecode parsing, playback-speed clamping, window-fitting geometry and the chrome hide/show rules were reachable only through
AVPlayeror a real window. Each is now a type of its own with tests, following theResumePolicyprecedent — 27 new assertions across four suites, covering the cases worth being sure about: that reaching for the titlebar doesn't take the chrome away as it's being aimed at, and that going idle never hides a pointer already over another app.fix:the unwraps that can actually be nilpresentOpenFilePanelforce-unwrappedNSApplication.shared.mainWindow. Opening a file is also how the app starts, so the crash was reachable from the welcome window — the very screen whose job is to open a file. Also the seek slider's force cast,asset != nil/asset!, andNSScreen.main!.Dead code goes too:
removeRemoteCommandHandlers,removePeriodicTimeObserver, the write half ofisPresentingUnopenableRecentFileAlert, and fourAnyDropDelegatemembers that were never set.docs:scope the GCD rule to new codeThe rule read as an absolute ban, which invites migrating working concurrency code to satisfy it. Every other rule in that list is a syntactic swap verifiable by reading; this one changes delivery semantics, which no diff or compiler shows. It now asks for modern concurrency in new code and says to leave working GCD alone.
One behaviour change
Forced subtitles are now detected by the
containsOnlyForcedSubtitlesmedia characteristic rather than by matching"Forced"in the display name. The name is localized, so the old test only ever worked on English media. A track merely called "Forced" without the characteristic will now be offered.Verification
swift-format lintclean,analyzeclean, 53 unit tests green.Playback was also checked by hand against real media, since the unit tests don't reach it: the Playback menu enables and reports Pause while playing, a 640×360 file sizes the window to 640×360, and a 3840×2160 file shrinks to 1876×1055 with its aspect ratio intact.
Generated by Claude Code