fix: correctly store playback positions to the file being played#79
Draft
itsjoshpark wants to merge 7 commits into
Draft
fix: correctly store playback positions to the file being played#79itsjoshpark wants to merge 7 commits into
itsjoshpark wants to merge 7 commits into
Conversation
Opening a file while another was loaded could write the incoming file's resume position under the outgoing file's URL, so reopening the first file jumped to a time it was never left at. `fileURL` was only assigned in the `.readyToPlay` sink, delivered asynchronously, while the periodic time observer began reporting the new item's time as soon as it was swapped in. Resetting the save throttle to zero on open guaranteed the first tick crossed the save interval whenever the resume target exceeded it, making the misattribution reliable rather than rare. Adopt the new file's identity and clear its time before swapping the item in, suppress saves for the duration of the swap, and seed the throttle with the position playback actually starts from. Also fix two ways stale positions accumulated: - `applicationWillTerminate` re-saved the end position of a file watched to completion, undoing the play-to-end observer's cleanup. Move the end-of-playback check into `persistCurrentPlaybackPosition` so every caller inherits it. - Recent entries dropped at launch because their bookmark no longer resolved, or trimmed off the end of the list, left their positions behind forever. Reconcile against the recents list on load and write the pruned bookmark list back. Extract the resume thresholds into `ResumePolicy` and add a test target covering them, the position store, and the orphan cleanup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Recents and positions were one logical record split across two stores, joined by a URL string that is not a stable identity. PlaybackPositionStore keyed on url.absoluteString, RecentDocumentsStore matched on URL equality of a bookmark-resolved URL, and PlayEngine guarded with recentURLs.contains(fileURL) — three places that had to agree with nothing forcing them to. Every removal path had to remember to clear the matching position by hand, and each of the three bugs fixed in 0c6b5b6 came out of that seam. Collapse both stores into one RecentDocument keyed by a UUID, with the position as a field. Orphaned positions are now impossible to express: removing a record takes its position with it, so retainOnly, all three clearPosition call sites, and the init-time reconciliation are gone. This also fixes the remaining identity bug by construction. FileOpening recorded the URL the caller supplied while PlayEngine held the bookmark-resolved one, so positions silently stopped saving whenever they differed. There is now one URL, resolved once. Opening becomes two-phase. prepareToOpen resolves without touching the list; confirmOpened commits only once playback has started. An unplayable file therefore never reaches NSDocumentController, which has no API to retract a single entry. Security-scoped access becomes a ScopedAccess whose lifetime is the grant, released at the commit point rather than at the top of openFile — so a failed open no longer revokes access to the file still playing. Bookmarks are now resolved lazily at open rather than at launch. Eager resolution could not distinguish a deleted file from one on an unmounted volume and erased both, taking their positions with it. A missing file stays listed until opened, then routes to the existing removal alert. Also fixed along the way: - FileCommands read the store statically, registering no observation, so the Open Recent menu could miss changes. - The spatial audio demo called PlayEngine directly and never presented the player window. Remote URLs now have an explicit openRemoteAndPresent path instead of relying on bookmark creation throwing. Existing recents and positions are dropped; the legacy defaults keys are removed on launch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
itsjoshpark
changed the title
Adds a Test step to both workflows and documents the command in AGENTS.md. Kept separate from Build and Analyze rather than folded into one xcodebuild invocation: derived data persists across steps in a job, so the test step compiles only the test bundle (12 files vs 42) and costs a few seconds, while a failure still points at the right phase. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The separate Test step was justified on failure attribution, but xcodebuild already prints a marker per action, so a broken test reports BUILD SUCCEEDED / ANALYZE SUCCEEDED / TEST FAILED from a single invocation. The step split only added a red X in the Actions UI, paid for with four near-identical YAML blocks across two workflows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
No description provided.