Implement clip share feature with spin-based creation#219
Open
briankeane wants to merge 11 commits into
Open
Conversation
* Add setup script to copy secrets from original repo * Fix arithmetic exit code bug and add dest dir guard Use $((var + 1)) instead of ((var++)) to avoid set -e aborting when post-increment evaluates to 0. Add a guard for the destination directory. * Fix DEST_DIR resolution to not abort before error guard Resolve only the script directory with cd, then append the relative path so the dest dir guard can print a friendly error if PlayolaRadio/Config is missing.
* Add search bar to station list page Filter stations by curator name or station name. Search bar pinned to bottom of screen matching library page style. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR feedback: autocapitalization, cornerRadius, segment clear - Disable autocapitalization on search TextField - Replace deprecated cornerRadius with clipShape - Clear searchText when switching segments to avoid stale results - Add test for segment-switch clearing behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- Add firstSpinId, lastSpinId, prerollMS, postrollMS to Clip model - Add SpinSummary and AiringSpinsResponse models - Add getAiringSpins endpoint to fetch spins for an airing - Update createClipForAiring to use spin IDs instead of airing IDs - Update MyAiringsPageModel to fetch spins before creating clips - Update tests for new API signatures
Contributor
Greptile SummaryThis PR implements the end-to-end clip share feature, allowing listeners to create, poll for, download, and share audio clips of their Q&A airings. It introduces the Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant MyAiringsPage
participant MyAiringsPageModel
participant API
participant PushNotifications
User->>MyAiringsPage: viewAppeared
MyAiringsPage->>MyAiringsPageModel: viewAppeared()
MyAiringsPageModel->>API: getMyListenerQuestionAirings()
MyAiringsPageModel->>API: getUserClips()
API-->>MyAiringsPageModel: [ListenerQuestionAiring]
API-->>MyAiringsPageModel: [Clip]
MyAiringsPageModel->>MyAiringsPageModel: matchClipsToAirings()
MyAiringsPageModel->>PushNotifications: scheduleAiringReminders(upcomingAirings)
User->>MyAiringsPage: Tap "Create Clip"
MyAiringsPage->>MyAiringsPageModel: createClipTapped(airing)
MyAiringsPageModel->>API: getAiringSpins(airingId)
API-->>MyAiringsPageModel: AiringSpinsResponse
MyAiringsPageModel->>MyAiringsPageModel: find firstSpin / lastSpin by airtime
MyAiringsPageModel->>API: createClipForAiring(firstSpinId, lastSpinId, 0, 0)
API-->>MyAiringsPageModel: Clip (pending/processing)
loop Poll every 3s, up to 60 times
MyAiringsPageModel->>API: getClip(clipId)
API-->>MyAiringsPageModel: Clip
alt status == completed
MyAiringsPageModel-->>MyAiringsPage: UI state → .ready(clip)
else status == failed
MyAiringsPageModel-->>MyAiringsPage: Show clipFailed alert
end
end
User->>MyAiringsPage: Tap "Download"
MyAiringsPageModel->>MyAiringsPageModel: shareTapped / downloadTapped
MyAiringsPageModel-->>MyAiringsPage: present ShareSheet
Note over PushNotifications,MyAiringsPageModel: clip_ready push notification
PushNotifications->>MyAiringsPageModel: push .myAiringsPage(model)
Last reviewed commit: "Update iOS API clien..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+238
to
+241
| } | ||
|
|
||
| pollingAiringIds.remove(airingId) | ||
| } |
Contributor
There was a problem hiding this comment.
After the polling loop exhausts its 60 iterations (~3 minutes), pollingAiringIds.remove(airingId) is called without presenting any alert. The UI will silently revert to the .noClip state (showing the "Create Clip" button again), with no indication to the user that the clip timed out. They may click "Create Clip" again, not knowing the original request is still being processed server-side.
Consider showing an alert here so the user knows the wait exceeded the expected time:
pollingAiringIds.remove(airingId)
presentedAlert = .clipTimeout // new alert case
Comment on lines
+139
to
+153
| func formattedAirtime(_ date: Date) -> String { | ||
| let dayFormatter = DateFormatter() | ||
| dayFormatter.dateFormat = "EEEE" | ||
| let dayOfWeek = dayFormatter.string(from: date) | ||
|
|
||
| let dateFormatter = DateFormatter() | ||
| dateFormatter.dateFormat = "MMM d" | ||
| let dateStr = dateFormatter.string(from: date) | ||
|
|
||
| let hourFormatter = DateFormatter() | ||
| hourFormatter.dateFormat = "h:mma" | ||
| let hour = hourFormatter.string(from: date).lowercased() | ||
|
|
||
| return "\(dayOfWeek), \(dateStr) at \(hour)" | ||
| } |
Contributor
There was a problem hiding this comment.
DateFormatter instances created on every call
DateFormatter is an expensive object to allocate and is not thread-safe, but allocating it fresh on each call inside a hot path (called once per visible row during layout) is wasteful. These should be static let properties or cached elsewhere.
Suggested change
| func formattedAirtime(_ date: Date) -> String { | |
| let dayFormatter = DateFormatter() | |
| dayFormatter.dateFormat = "EEEE" | |
| let dayOfWeek = dayFormatter.string(from: date) | |
| let dateFormatter = DateFormatter() | |
| dateFormatter.dateFormat = "MMM d" | |
| let dateStr = dateFormatter.string(from: date) | |
| let hourFormatter = DateFormatter() | |
| hourFormatter.dateFormat = "h:mma" | |
| let hour = hourFormatter.string(from: date).lowercased() | |
| return "\(dayOfWeek), \(dateStr) at \(hour)" | |
| } | |
| func formattedAirtime(_ date: Date) -> String { | |
| let dayFormatter = Self.dayFormatter | |
| let dayOfWeek = dayFormatter.string(from: date) | |
| let dateFormatter = Self.monthDayFormatter | |
| let dateStr = dateFormatter.string(from: date) | |
| let hourFormatter = Self.hourFormatter | |
| let hour = hourFormatter.string(from: date).lowercased() | |
| return "\(dayOfWeek), \(dateStr) at \(hour)" | |
| } |
Add the static formatters near the top of the class:
private static let dayFormatter: DateFormatter = {
let f = DateFormatter()
f.dateFormat = "EEEE"
return f
}()
private static let monthDayFormatter: DateFormatter = {
let f = DateFormatter()
f.dateFormat = "MMM d"
return f
}()
private static let hourFormatter: DateFormatter = {
let f = DateFormatter()
f.dateFormat = "h:mma"
return f
}()
Comment on lines
+87
to
+95
| func downloadTapped(_ airing: ListenerQuestionAiring) { | ||
| guard let clip = clips[airing.id] else { return } | ||
| guard let urlString = clip.url else { | ||
| presentedAlert = .errorDownloadingClip | ||
| return | ||
| } | ||
| let shareModel = ShareSheetModel(items: [urlString]) | ||
| mainContainerNavigationCoordinator.presentedSheet = .share(shareModel) | ||
| } |
Contributor
There was a problem hiding this comment.
downloadTapped opens a share sheet with a URL string, not a downloaded file
The "Download" button's handler passes urlString (a String) to ShareSheetModel. On iOS, sharing a string URL via UIActivityViewController will offer "Copy" and web-browser-oriented actions, but typically won't trigger a "Save to Files" or "Save to Music Library" offer that users expect when tapping a button labelled "Download". Passing a URL instead of a String enables proper file-download behaviors in the share sheet:
Suggested change
| func downloadTapped(_ airing: ListenerQuestionAiring) { | |
| guard let clip = clips[airing.id] else { return } | |
| guard let urlString = clip.url else { | |
| presentedAlert = .errorDownloadingClip | |
| return | |
| } | |
| let shareModel = ShareSheetModel(items: [urlString]) | |
| mainContainerNavigationCoordinator.presentedSheet = .share(shareModel) | |
| } | |
| func downloadTapped(_ airing: ListenerQuestionAiring) { | |
| guard let clip = clips[airing.id] else { return } | |
| guard let urlString = clip.url, let url = URL(string: urlString) else { | |
| presentedAlert = .errorDownloadingClip | |
| return | |
| } | |
| let shareModel = ShareSheetModel(items: [url]) | |
| mainContainerNavigationCoordinator.presentedSheet = .share(shareModel) | |
| } |
Comment on lines
+41
to
+52
| private var pollingTask: Task<Void, Never>? | ||
|
|
||
| // MARK: - User Actions | ||
|
|
||
| func viewAppeared() async { | ||
| await loadData() | ||
| await pushNotifications.scheduleAiringReminders(upcomingAirings) | ||
| } | ||
|
|
||
| func viewDisappeared() { | ||
| pollingTask?.cancel() | ||
| } |
Contributor
There was a problem hiding this comment.
pollingTask does not support concurrent polling for multiple airings
pollingTask is a single stored property, but pollForClipCompletion can be called concurrently for different airings. When a user taps "Create Clip" on two different past airings simultaneously, the second call overwrites pollingTask with a new Task. The reference to the first task is permanently lost — so when viewDisappeared() calls pollingTask?.cancel(), only the second task is cancelled. The first polling task keeps running (and retaining self) in the background for up to 3 minutes (60 iterations × 3 s each) after the view has been dismissed.
To fix this, store tasks keyed by airingId:
private var pollingTasks: [String: Task<Void, Never>] = [:]
func viewDisappeared() {
pollingTasks.values.forEach { $0.cancel() }
pollingTasks.removeAll()
}Then in pollForClipCompletion, assign to pollingTasks[airingId] instead of pollingTask, and remove the entry once the task finishes.
Add Clip.swift, MyAiringsPage files to pbxproj so they compile. Add rbenv + xcodeproj gem installation to Makefile setup-conductor target so Conductor agents can programmatically add files to the Xcode project.
- Replace single pollingTask with pollingTasks dictionary keyed by airingId to support concurrent polling for multiple airings - Show clipTimeout alert when polling exhausts 60 iterations instead of silently reverting to noClip state - Pass URL object instead of String to ShareSheetModel in downloadTapped for proper iOS file-save behavior - Cache DateFormatter instances as static properties instead of allocating per call
Tests calling viewAppeared() need date.now (accessed via upcomingAirings) and scheduleNotification (called by scheduleAiringReminders).
Revert URL change since ShareSheetModel.items is [String], not [Any]. Also add pre-PR checklist to CLAUDE.md.
Tests calling viewAppeared() that provide getMyListenerQuestionAirings also need date.now and pushNotifications.scheduleNotification since checkForUpcomingQuestionAirings uses both. Also update TESTING.md with guidance on providing all dependencies reached by a code path, not just the one under test.
…rings Test returns a future airing, so scheduleAiringReminders tries to schedule a notification for it.
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.
Summary
Completes the clip share feature allowing listeners to download and share audio clips of their Q&A airings. Uses spin-based clip identity instead of time ranges for natural de-duplication. Includes new MyAiringsPage, push notification handling, homepage integration, and comprehensive design system documentation.
Changes
Test Coverage
All new functionality covered with 27 tests across MyAiringsPageTests and related modules.