Persist stable recording timestamps and use them for recordingDate - #87
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a16081418
ℹ️ 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".
| } | ||
|
|
||
| do { | ||
| try data.write(to: recordingTimestampMetadataURL(for: url), options: .atomic) |
There was a problem hiding this comment.
persistRecordingCapturedAt now creates a .recordingmeta sidecar for every recording, but there is no corresponding cleanup path for that file type (for example, EnhancedFileManager.deleteRecording removes only the audio file and .location, and the fallback deletes in RecordingsListView/TranscriptViews also remove only known file types). This leaves orphaned metadata files behind whenever a recording is deleted, which introduces silent storage growth and stale on-disk artifacts over time.
Useful? React with 👍 / 👎.
…estamp Address review feedback: the new .recordingmeta sidecar files were not being cleaned up when recordings were deleted, causing silent storage growth. Add cleanup to all four deletion paths (EnhancedFileManager, TranscriptViews, RecordingArchiveService, orphan cleanup). Also fix generateAppRecordingDisplayName() to use recordingStartedAt so the display name timestamp matches the persisted recordingDate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf90b7d12
ℹ️ 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".
| if let recordingStartedAt { | ||
| return recordingStartedAt |
There was a problem hiding this comment.
currentRecordingDate(for:) currently returns recordingStartedAt for any URL, so a start time from one session can be applied to a different file. This is reachable when stopRecording() kicks off mergeRecordingSegments() asynchronously and the user starts a new recording before merge persistence runs: recordingStartedAt now points to the new session, and the merge path stamps the older merged file with the wrong recordingDate. That can reorder recordings and corrupt historical capture times; the in-memory timestamp should only be used for the matching active URL (or sidecar should be preferred for other URLs).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude finished @bisonbet's task in 1m 47s —— View job
Addressing review comment: Scope start timestamp to the recording URL
- Gather context and read relevant files
- Implement fix: change
recordingStartedAtto a URL-keyed tuple - Update
currentRecordingDate(for:)andgenerateAppRecordingDisplayName() - Commit and push changes
What changed:
recordingStartedAt was Date? — a bare timestamp with no URL binding. The race condition: stopRecording() kicks off mergeRecordingSegments() async, user starts a new recording before merge persistence runs, and currentRecordingDate(for: mergedURL) returns the new session's start time.
Fix: Changed recordingStartedAt: Date? → recordingStartedAt: (url: URL, date: Date)? in AudioRecorderViewModel.swift. currentRecordingDate(for:) now only uses the in-memory timestamp when the URL matches the active recording:
// Before
if let recordingStartedAt {
return recordingStartedAt // returned for ANY url
}
// After
if let entry = recordingStartedAt, let url, entry.url == url {
return entry.date // only returned for the matching active URL
}
// Falls through to sidecar read for all other URLsFor any URL that isn't the currently active recording, the method now falls through to read the persisted .recordingmeta sidecar — which is exactly what was already written at recording start via persistRecordingCapturedAt. All nil-clearing sites are unchanged (assigning nil to an optional tuple works identically).
Commit: b6c66b6
|
Change recordingStartedAt from Date? to (url: URL, date: Date)? so currentRecordingDate(for:) only returns the in-memory start time when the provided URL matches the active recording URL. Previously, an async merge that completed after a new recording started would pick up the new session's timestamp for the old merged file, corrupting its recordingDate. Co-authored-by: Tim Champ <bisonbet@users.noreply.github.com>
1 participant
Motivation
recordingDatematches the true capture time.Description
recordingStartedAtproperty toAudioRecorderViewModeland set it at the start ofsetupRecording()vialet recordingStartDate = Date().RecordingTimestampMetadatastruct andpersistRecordingCapturedAt(_:for:)which writes a.recordingmetafile.currentRecordingDate(for:)to prefer the in-memory timestamp, then read the sidecar.recordingmeta, and finally fall back toDate().audioRecorderDidFinishRecording, interruption recovery, unprocessed/background recovery, merged-segments completion, and live-transcription save) to passcurrentRecordingDate(for:)toRecordingWorkflowManager.createRecording(...)and to clearrecordingStartedAtafter persistence.Testing
xcodebuild -list -project 'BisonNotes AI/BisonNotes AI.xcodeproj', but it failed in this environment becausexcodebuildis not available.Persist explicit recording start timestamps and use them for recording dates).Codex Task