Vine proof of concept view + upload/record - #3354
Conversation
|
WIP view only vine proof of concept screenshot 
|
|
Tinkering with addition and publication of vines/videos. Its not ready just yet |
|
e054012 allows for publication of vines Vine posted via iOS damus https://damus.io/nevent1qqsr4tj4mk5ygaeryqdsqpxdfxvqtu665evjy5uczj3xmnke45leq8gg8p97g |
alltheseas
changed the title
danieldaquino
added
the
pr-in-queue
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Vine short‑video support: new Nostr kind (34236), Vine UI (timeline, cards, full‑screen pager), feed model with relay subscription and prefetching, Vine composer and upload flow, async video processing (metadata stripping), feature toggles/settings, and feature‑managed relay controls. Changes
Sequence DiagramssequenceDiagram
actor User
participant ContentView
participant VineTimelineView
participant VineFeedModel
participant NostrNetworkManager
participant RelayPool
participant Divine_Relay as "Divine Relay"
User->>ContentView: Select Vines timeline
ContentView->>VineTimelineView: init(damus_state)
VineTimelineView->>VineFeedModel: create & subscribe()
VineFeedModel->>NostrNetworkManager: ensureRelayConnected(vineRelay)
NostrNetworkManager->>NostrNetworkManager: add to featureManagedRelays
NostrNetworkManager->>RelayPool: connect to vineRelay
RelayPool->>Divine_Relay: establish connection
Divine_Relay-->>RelayPool: stream events (kind 34236)
RelayPool-->>VineFeedModel: deliver events
VineFeedModel->>VineFeedModel: dedupe, order, paginate
VineFeedModel-->>VineTimelineView: update vines list
VineTimelineView-->>User: display feed
sequenceDiagram
actor User
participant VineComposerView
participant MediaPicker
participant ImageProcessing
participant VineBlossomUploadService
participant VineEventBuilder
participant NostrNetwork
User->>VineComposerView: pick media
VineComposerView->>MediaPicker: present
MediaPicker-->>VineComposerView: picked URL
VineComposerView->>ImageProcessing: generateMediaUpload (async)
ImageProcessing->>ImageProcessing: processVideo async
ImageProcessing->>ImageProcessing: exportVideoStrippingSensitiveMetadata
ImageProcessing-->>VineComposerView: sanitized media URL
VineComposerView->>VineBlossomUploadService: upload video
VineBlossomUploadService-->>VineComposerView: upload response
VineComposerView->>VineEventBuilder: build event
VineEventBuilder-->>VineComposerView: signed event
VineComposerView->>NostrNetwork: publish event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@damus/ContentView.swift`:
- Around line 166-170: Remove the in-body DispatchQueue.main.async state
mutation that checks selected_timeline and damus.settings.vines_feature_enabled
(the if block that sets self.selected_timeline = .home) and instead add an
.onChange modifier on the top-level view in MainContent's body that observes
either damus.settings.vines_feature_enabled or selected_timeline (or both) and
performs the timeline fallback there; reference the selected_timeline `@State` and
damus.settings.vines_feature_enabled within the onChange closure to set
selected_timeline = .home when vines are disabled and selected_timeline ==
.vines to avoid mutating state during body evaluation.
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift`:
- Around line 142-160: disconnectRelay currently removes any relay present in
pool (via pool.get_relay and pool.remove_relay) which can delete user-configured
relays; add ownership tracking in NostrNetworkManager by introducing a private
Set<RelayURL> (e.g., featureManagedRelays) and update ensureRelayConnected(_:
RelayURL) to insert the relay into featureManagedRelays when you call
pool.add_relay, then change disconnectRelay(_: RelayURL) to only call
pool.remove_relay if the relay is in featureManagedRelays and remove it from
that set after successful removal; reference the existing methods
ensureRelayConnected, disconnectRelay, pool.get_relay, pool.add_relay,
pool.remove_relay and the PostingTimelineView managedRelayConnection to locate
related behavior.
In `@damus/Features/Labs/Views/DamusLabsExperiments.swift`:
- Around line 16-17: Make the two SwiftUI `@State` properties private to satisfy
SwiftLint's private_swiftui_state rule: change the declarations of
show_vines_explainer and show_vine_prefetch_explainer in the
DamusLabsExperiments view to be private (e.g., `@State` private var ...), and
update any internal references within that view accordingly so external code is
not relying on those properties.
In `@damus/Features/Relays/Views/UserRelaysView.swift`:
- Around line 30-50: Wrap the "Divine Relay" Section containing Toggle(...)
(which uses state.settings.enable_vine_relay and setDivineRelayEnabled) in a
conditional check so it only renders when the Vines feature flag is enabled
(state.settings.vines_feature_enabled); i.e., in UserRelaysView guard the
Section with if state.settings.vines_feature_enabled { ... } so the toggle is
hidden for users who haven't enabled Vines.
In `@damus/Features/Timeline/Views/PostingTimelineView.swift`:
- Around line 889-898: The function normalizedURL(_:) silently rewrites
"apt.openvine.co" to "api.openvine.co", which is fragile; update the
implementation by either removing the hardcoded replacement and relying on
upstream data fixes, or if keeping it for backward compatibility, add a clear
comment above normalizedURL(_:) explaining why this specific replacement exists,
how long it should be retained, and referencing the upstream bug/relay/publisher
issue so future maintainers can remove it once fixed; ensure the rest of
normalizedURL(_:)'s behavior (trimming, URL(string:), and scheme check) remains
unchanged.
- Around line 181-572: The file contains seven Vine-related types bundled into
PostingTimelineView.swift which makes it unmaintainable; split them into focused
files under damus/Features/Vines/: create VineTimelineView.swift (containing
VineTimelineView), VineFeedModel.swift (VineFeedModel), VineVideo.swift
(VineVideo model), VineCard.swift (VineCard and VineMetadataRow), and
VineFullScreen.swift (VineFullScreenPager and VineFullScreenPage), move each
type into its new file preserving their implementation and imports, adjust
access levels (private/internal/public) as needed so PostingTimelineView still
compiles, update any references/usages (e.g., VineTimelineView initializer and
VineFeedModel usage) and run a build to fix import or visibility errors.
- Around line 336-350: noteAppeared(at:) currently starts Task.detached that
reads self.vines (a `@Published` main-actor property) off the main actor causing a
data race; fix by collecting the target playback URLs on the main actor before
creating the detached task: inside noteAppeared(at:) (while still on `@MainActor`)
compute targets = [index, index+1], read
damus_state.settings.prefetch_vines_on_cellular into allowCellular, iterate
targets to build an array of non-nil URLs from self.vines[target].playbackURL,
then start Task.detached { await self?.prefetch(url: url, allowCellular:
allowCellular) } iterating the pre-collected URLs so the detached task no longer
accesses vines directly.
- Around line 529-542: The prefetch(url:allowCellular:) function issues a
URLSession.data request and discards the result, relying on server HTTP caching
which is not reliable; change prefetch to write the downloaded data into the
app's disk video cache using VideoCache.standard (use its API such as
maybe_cached_url_for(video_url:) or the appropriate write method) so prefetched
content is persisted with the existing 1-day expiry; update prefetch to fetch
the data, store it into VideoCache.standard for the given URL, and still call
unmarkPrefetching(url) in the same finally/cleanup path to avoid leaving entries
marked prefetched.
In `@damus/Features/Vines/Creation/VineComposerView.swift`:
- Around line 224-260: The call to videoMetadata(for:) runs on the `@MainActor` in
uploadSelectedMedia and blocks the main thread; move the metadata extraction
into the background Task (or a new Task.detached) so metadata is computed
off-main before uploadService.uploadVideo is awaited. Concretely, keep the
keypair check and uploadPhase changes on the main actor, but compute let
metadata = videoMetadata(for: media.localURL) inside the Task.detached (before
calling uploadService.uploadVideo), then pass that metadata into
makeDescriptor/response handling and back to MainActor to set mediaDescriptor,
vineIdentifier and uploadPhase.
In `@damus/Shared/Media/Images/ImageProcessing.swift`:
- Around line 43-54: processVideo currently falls back to
saveVideoToTemporaryFolder when exportVideoStrippingSensitiveMetadata fails,
which can return the original file with GPS metadata and violates the "never
leak location data" contract; change processVideo to stop silently copying raw
videos and instead return nil (or propagate an error) when
exportVideoStrippingSensitiveMetadata fails so callers know sanitization failed,
or implement a reliable secondary metadata-strip step before any fallback—update
processVideo, exportVideoStrippingSensitiveMetadata usage, and calling code to
handle a nil/error return rather than assuming the returned URL is sanitized.
- Around line 163-189: The exportVideoStrippingSensitiveMetadata function blocks
the calling thread with DispatchSemaphore.wait, causing UI freezes; make it
asynchronous and non-blocking by replacing the semaphore pattern with Swift
concurrency (e.g., withCheckedContinuation or an async wrapper around
AVAssetExportSession) so exportVideoStrippingSensitiveMetadata returns
asynchronously (async -> Bool) and callers (processVideo and
generateMediaUpload) are updated to async; then ensure EditPictureControl.upload
and the PostView button action invoke the async pipeline off the main thread
(wrap in Task { await ... } or propagate async up the call chain). Also add a
cancellable timeout inside the async export wrapper to call
exportSession.cancelExport() on timeout and return false as a safe fallback.
In `@damusTests/NostrEventTests.swift`:
- Around line 125-131: The test named testExpiredVineIsSkipped is misleading
because it creates an expired event via makeVineEvent(tags:
VineFixtures.expired) and then asserts VineVideo(event: expired) is non-nil and
checks expirationTimestamp; either rename the test to reflect that it verifies
parsing (suggested name: testExpiredVineParsesExpirationTimestamp) or change the
assertions to verify filtering behavior by invoking the feed/filtering function
(where expired items are dropped) and asserting the VineVideo is excluded;
update references to the test name and adjust assertions accordingly (look for
testExpiredVineIsSkipped, makeVineEvent, VineFixtures.expired, VineVideo, and
expirationTimestamp).
- Around line 101-123: The two tests (testReplacementKeepsNewestEvent and
testReplacementKeepsOldestWhenOlder) call the actor VineTestFeed from outside
its isolation; make each test async and add await when invoking feed.apply(...)
and when reading feed.vines (including feed.vines.first?.title) so all accesses
to the VineTestFeed actor (apply and vines) are awaited to satisfy actor
isolation under strict concurrency.
🧹 Nitpick comments (18)
damus/Features/Follows/Models/Contacts.swift (2)
17-24: Pre-existing:eventanddelegateare not guarded by the new lock.These properties can be read/written from multiple threads and aren't protected. This is outside the scope of the current change (the lock targets
friends/friend_of_friends/pubkey_to_our_friends), but worth noting for a future pass.
40-50: Missing docstrings on modified methods.Per coding guidelines, added or modified code should have docstring coverage. Most of the lock-guarded methods (e.g.,
get_friend_list,get_friend_of_friends_list,add_friend_pubkey,is_friend, etc.) lack documentation. The names are self-documenting, but a brief note on thread-safety guarantees would be helpful for future contributors.As per coding guidelines, "Ensure docstring coverage for any code added or modified".
damus/Shared/Media/Images/ImageProcessing.swift (1)
98-100:.processed_videois now re-processed — naming is misleading.The
processed_videocase name implies the video has already been fully processed, yet it now goes throughprocessVideo(re-encode + metadata strip). This adds non-trivial latency (full re-encode atAVAssetExportPresetHighestQuality) for videos that may have already been transcoded.If the intent is that all videos must be sanitized before upload regardless, consider renaming the enum case to clarify that it hasn't been metadata-stripped yet, or documenting why double-processing is acceptable.
damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift (1)
148-150: Silently swallowed error onadd_relay— consider logging.
try?discards any failure fromadd_relay. While the only current error isRelayAlreadyExists(benign here given the TOCTOU race with the guard above), silently swallowing it makes debugging harder. Ado/catchthat logs unexpected errors would be more robust.Proposed improvement
- try? await pool.add_relay(descriptor) - await pool.connect(to: [relayURL]) + do { + try await pool.add_relay(descriptor) + await pool.connect(to: [relayURL]) + } catch RelayError.RelayAlreadyExists { + // Already present — just ensure it's connected + await pool.connect(to: [relayURL]) + } catch { + Log.warning("Failed to add relay %@: %@", for: .relay_connectivity, relayURL.absoluteString, error.localizedDescription) + }damusTests/Fixtures/VineFixtures.swift (1)
10-77: Fixture file looks solid with good scenario coverage.Covers classic import, multi-imeta fallback, replacement, muted, expired, and repost scenarios — nice breadth for a POC.
Minor: per coding guidelines, consider adding a brief docstring to the enum and each fixture describing the scenario it represents, to help future contributors understand the test intent at a glance. As per coding guidelines,
**/*.swift: "Ensure docstring coverage for any code added or modified."damus/Features/Timeline/Models/HomeModel.swift (1)
645-647: Unstructured fire-and-forget tasks lose event ordering guarantees.Spawning an independent
Task {@mainactorin … }per event means the main-actor executor can reorder completions relative to arrival order. For the home timeline this is likely acceptable becauseEventHolder.insertsorts by timestamp, but it's worth noting that any logic inprocess_eventthat depends on sequential delivery (e.g. dedup viaalready_reposted) could see subtle races when two events for the same repost target arrive back-to-back.If ordering becomes an issue, consider an
AsyncStream-based serial queue feeding the main actor instead of per-event tasks.damus/Features/Timeline/Views/MainTabView.swift (1)
82-86: Keyboard shortcuts have a gap when Vines is disabled.When
vines_feature_enabledis false, the tab bar renders Home (⌘1), Search (⌘3), Notifications (⌘4) — skipping⌘2. Consider assigning shortcuts dynamically or using sequential values to avoid confusing keyboard/iPad users.damus/Features/Relays/Views/UserRelaysView.swift (1)
56-65: Missing docstring onsetDivineRelayEnabled.As per coding guidelines, added or modified code should have docstring coverage.
Proposed fix
+ /// Updates the Divine Relay setting and connects or disconnects the relay accordingly. private func setDivineRelayEnabled(_ enabled: Bool) {damus/ContentView.swift (2)
165-165:immersiveTimelineis computed identically in bothMainContentandbody.Consider extracting this into a computed property on
ContentViewto avoid duplication.Proposed refactor
+ private var immersiveTimeline: Bool { + selected_timeline == .home || selected_timeline == .vines + }Then replace the local
let immersiveTimeline = …in bothMainContentandbody.Also applies to: 249-249
185-190: Redundant fallback for.vineswhen feature is disabled.The guard on line 166 already redirects to
.homewhen vines is disabled, so theelsebranch renderingPostingTimelineViewshould be unreachable in steady state. If you move the guard to.onChangeas suggested above, consider simplifying this to only renderVineTimelineView(since the tab won't be selectable when disabled).damusTests/NostrEventTests.swift (1)
150-169: Consider whetherVineTestFeedneeds to be anactor.Since the synchronous test helpers (
testReplacementKeeps…) don't benefit from actor isolation, and the only async test (testMutedAuthorFiltered) could work with a simple class, a plainclasswith@MainActorisolation (or no isolation) would simplify the test code and avoid theawaitceremony. The productionVineFeedModel.handle(event:)already runs on@MainActor.damus/Features/Settings/Models/UserSettingsStore.swift (1)
115-148: Missing docstrings on new Vine settings.The coding guidelines require docstring coverage for added code. The three new
@Settingproperties and thevines_feature_enabledcomputed property lack documentation. Other settings in this file (e.g.,nozaps,undistractMode,enable_experimental_local_relay_model) set a good precedent with///doc comments.Suggested docstrings
+ /// Whether the Vine feature is enabled by the user (Damus Labs experiment). `@Setting`(key: "enable_vine_feature", default_value: false) var enable_vine_feature: Bool + /// Whether the app should connect to the Divine relay for Vine video content. `@Setting`(key: "enable_vine_relay", default_value: true) var enable_vine_relay: Bool ... + /// Whether to allow prefetching Vine videos on cellular/expensive network connections. `@Setting`(key: "prefetch_vines_on_cellular", default_value: false) var prefetch_vines_on_cellular: Bool + /// Resolved flag: always `true` in DEBUG builds; otherwise defers to `enable_vine_feature`. var vines_feature_enabled: Bool {As per coding guidelines: "Ensure docstring coverage for any code added or modified."
damus/Features/Vines/Creation/VineComposerView.swift (4)
12-28: ManualEquatableconformance is unnecessary.Swift can auto-synthesize
Equatablefor enums whose associated values are themselvesEquatable. SinceStringisEquatable, this entire== (lhs:rhs:)implementation can be removed by simply declaringenum UploadPhase: Equatable.
173-173:.autocapitalization(.none)is deprecated.Use
.textInputAutocapitalization(.never)instead, which is the non-deprecated replacement available since iOS 15.
330-357:publishVine()dismisses immediately without confirming the post was dispatched.
notify(.post(.post(post)))is fire-and-forget;dismiss()is called right after. If there's any async processing downstream that could fail, the user won't see an error. Acceptable for a POC, but worth noting for future hardening — consider awaiting confirmation or at minimum adding a brief delay / completion callback before dismissing.
262-271: Replace deprecatedAVURLAssetAPIs with async alternatives to prevent main-thread blocking.
asset.tracks(withMediaType:)andasset.durationwere deprecated in iOS 16 in favor of async APIs:try await asset.loadTracks(withMediaType:)andtry await asset.load(.duration). The function is currently called on the main thread (line 233) and should be converted toasync, allowing the call site to properly await it within the existingTask.detachedblock.damus/Features/Timeline/Views/PostingTimelineView.swift (2)
415-426: O(n log n) sort on every incoming event during streaming.
vines.sort(...)is called for each event received viahandle(event:). With the initialfilter.limit = 200, this means up to 200 sorts on a growing array. Consider using binary-search insertion (Array.insert(at:)) to maintain sort order in O(log n) per event, or batch events and sort once at EOSE.Example: binary insertion
// Replace the sort with an insertion at the correct position: let insertionIndex = vines.firstIndex(where: { $0.createdAt < video.createdAt }) ?? vines.endIndex vines.insert(video, at: insertionIndex)
1190-1195:RelativeDateTimeFormatterre-created on every view render.Both
VineCard.relativeDateandVineFullScreenPage.relativeDateallocate a newRelativeDateTimeFormattereach time they're evaluated. Since these are computed properties in SwiftUI views, they run on every re-render. Use a sharedstatic letformatter to avoid repeated allocations.Suggested fix
+private let sharedRelativeDateFormatter: RelativeDateTimeFormatter = { + let f = RelativeDateTimeFormatter() + f.unitsStyle = .short + return f +}() + private var relativeDate: String { - let formatter = RelativeDateTimeFormatter() - formatter.unitsStyle = .short let date = Date(timeIntervalSince1970: TimeInterval(vine.createdAt)) - return formatter.localizedString(for: date, relativeTo: Date()) + return sharedRelativeDateFormatter.localizedString(for: date, relativeTo: Date()) }Also applies to: 1362-1367
| struct VineTimelineView: View { | ||
| let damus_state: DamusState | ||
| @StateObject private var model: VineFeedModel | ||
| @State private var presentingFullScreen = false | ||
| @State private var fullScreenIndex = 0 | ||
|
|
||
| init(damus_state: DamusState) { | ||
| self.damus_state = damus_state | ||
| _model = StateObject(wrappedValue: VineFeedModel(damus_state: damus_state)) | ||
| } | ||
|
|
||
| var body: some View { | ||
| ScrollView { | ||
| LazyVStack(spacing: 24) { | ||
| if let message = model.relayMessage { | ||
| infoBanner(text: message) | ||
| } | ||
| ForEach(Array(model.vines.enumerated()), id: \.1.id) { index, vine in | ||
| VineCard( | ||
| vine: vine, | ||
| damus_state: damus_state, | ||
| onAppear: { model.noteAppeared(at: index) }, | ||
| onOpenFullScreen: { | ||
| fullScreenIndex = index | ||
| presentingFullScreen = true | ||
| } | ||
| ) | ||
| } | ||
| if model.vines.isEmpty && !model.isLoading && model.relayMessage == nil { | ||
| Text("No Vine videos yet. Pull down to refresh.") | ||
| .font(.footnote) | ||
| .foregroundColor(.secondary) | ||
| .frame(maxWidth: .infinity, alignment: .center) | ||
| .padding(.vertical, 32) | ||
| } | ||
| } | ||
| .padding(.horizontal) | ||
| .padding(.bottom, 32) | ||
| } | ||
| .background(DamusColors.adaptableWhite) | ||
| .refreshable { await model.refresh() } | ||
| .overlay { | ||
| if model.isLoading { | ||
| ProgressView() | ||
| .padding() | ||
| .background(RoundedRectangle(cornerRadius: 14).fill(Color(uiColor: .systemBackground))) | ||
| .shadow(radius: 4) | ||
| } | ||
| } | ||
| .onAppear { model.subscribe() } | ||
| .onDisappear { model.stop(disconnect: true) } | ||
| .onReceive(damus_state.settings.objectWillChange) { _ in | ||
| model.handleSettingsChange() | ||
| } | ||
| .damus_full_screen_cover($presentingFullScreen, damus_state: damus_state) { | ||
| VineFullScreenPager( | ||
| model: model, | ||
| damus_state: damus_state, | ||
| initialIndex: fullScreenIndex, | ||
| onClose: { presentingFullScreen = false } | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private func infoBanner(text: String) -> some View { | ||
| HStack(alignment: .top, spacing: 12) { | ||
| Image(systemName: "bolt.horizontal.circle") | ||
| .foregroundColor(.purple) | ||
| Text(text) | ||
| .font(.footnote) | ||
| .foregroundColor(.secondary) | ||
| Spacer() | ||
| } | ||
| .padding() | ||
| .background( | ||
| RoundedRectangle(cornerRadius: 16) | ||
| .fill(Color(uiColor: .secondarySystemBackground)) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private final class VineFeedModel: ObservableObject { | ||
| @Published private(set) var vines: [VineVideo] = [] | ||
| @Published var isLoading: Bool = false | ||
| @Published var relayMessage: String? = nil | ||
|
|
||
| private let pageSize = 40 | ||
| private let damus_state: DamusState | ||
| private var streamTask: Task<Void, Never>? | ||
| private var lastSeenTimestamp: UInt32? | ||
| private var managedRelayConnection = false | ||
| private let pathMonitor = NWPathMonitor() | ||
| private let pathQueue = DispatchQueue(label: "io.damus.vines.network") | ||
| @MainActor private var pathIsExpensive = false | ||
| @MainActor private var pathIsConstrained = false | ||
| @MainActor private var prefetchingURLs: Set<URL> = [] | ||
| @MainActor private var oldestTimestamp: UInt32? | ||
| @MainActor private var isLoadingOlder = false | ||
| @MainActor private var hasMoreOlder = true | ||
|
|
||
| init(damus_state: DamusState) { | ||
| self.damus_state = damus_state | ||
| pathMonitor.pathUpdateHandler = { [weak self] path in | ||
| Task { @MainActor in | ||
| self?.pathIsExpensive = path.isExpensive | ||
| self?.pathIsConstrained = path.isConstrained | ||
| } | ||
| } | ||
| pathMonitor.start(queue: pathQueue) | ||
| } | ||
|
|
||
| func subscribe() { | ||
| stop() | ||
| streamTask = Task { | ||
| await self.loadInitialPage() | ||
| await self.stream() | ||
| } | ||
| } | ||
|
|
||
| func stop(disconnect: Bool = false) { | ||
| streamTask?.cancel() | ||
| streamTask = nil | ||
| if disconnect { | ||
| Task { | ||
| await self.disconnectManagedRelayIfNeeded() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func refresh() async { | ||
| await MainActor.run { | ||
| vines.removeAll() | ||
| lastSeenTimestamp = nil | ||
| oldestTimestamp = nil | ||
| hasMoreOlder = true | ||
| } | ||
| subscribe() | ||
| } | ||
|
|
||
| func handleSettingsChange() { | ||
| guard damus_state.settings.enable_vine_relay else { | ||
| stop(disconnect: true) | ||
| Task { @MainActor in | ||
| relayMessage = NSLocalizedString("Enable the Divine relay in Settings ▸ Relays to see Vine videos.", comment: "Message shown when the Vine relay is disabled.") | ||
| vines.removeAll() | ||
| isLoading = false | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if streamTask == nil { | ||
| subscribe() | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| func noteAppeared(at index: Int) { | ||
| maybeLoadOlder(after: index) | ||
| guard shouldPrefetchVideos else { return } | ||
| let targets = [index, index + 1] | ||
| let allowCellular = damus_state.settings.prefetch_vines_on_cellular | ||
| Task.detached(priority: .background) { [weak self] in | ||
| guard let self else { return } | ||
| for target in targets { | ||
| guard self.vines.indices.contains(target), | ||
| let url = self.vines[target].playbackURL else { continue } | ||
| await self.prefetch(url: url, allowCellular: allowCellular) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func stream() async { | ||
| guard damus_state.settings.enable_vine_relay else { | ||
| await MainActor.run { | ||
| relayMessage = NSLocalizedString("Enable the Divine relay in Settings ▸ Relays to see Vine videos.", comment: "Message shown when the Vine relay is disabled.") | ||
| isLoading = false | ||
| } | ||
| return | ||
| } | ||
|
|
||
| let alreadyConnected = await MainActor.run { | ||
| damus_state.nostrNetwork.getRelay(.vineRelay) != nil | ||
| } | ||
| await damus_state.nostrNetwork.ensureRelayConnected(.vineRelay) | ||
| if !alreadyConnected { | ||
| await MainActor.run { | ||
| self.managedRelayConnection = true | ||
| } | ||
| } | ||
|
|
||
| await MainActor.run { | ||
| relayMessage = nil | ||
| isLoading = true | ||
| } | ||
|
|
||
| var filter = NostrFilter(kinds: [.vine_short]) | ||
| filter.limit = 200 | ||
| let now = UInt32(Date().timeIntervalSince1970) | ||
| filter.until = now | ||
| if let lastSeenTimestamp { | ||
| filter.since = lastSeenTimestamp | ||
| } else { | ||
| filter.since = now > 604800 ? now - 604800 : 0 | ||
| } | ||
|
|
||
| for await item in damus_state.nostrNetwork.reader.advancedStream(filters: [filter], to: [.vineRelay]) { | ||
| if Task.isCancelled { break } | ||
| switch item { | ||
| case .event(let lender): | ||
| await lender.justUseACopy({ await self.handle(event: $0) }) | ||
| case .ndbEose, .networkEose, .eose: | ||
| await MainActor.run { self.isLoading = false } | ||
| } | ||
| } | ||
|
|
||
| await MainActor.run { | ||
| self.isLoading = false | ||
| } | ||
| } | ||
|
|
||
| private func handle(event: NostrEvent) async { | ||
| let canonical = canonicalEvent(for: event) | ||
| guard let video = VineVideo(event: canonical.base, repostSource: canonical.repost) else { | ||
| Log.debug("Skipping Vine event %s (failed to parse)", for: .timeline, canonical.base.id.hex()) | ||
| return | ||
| } | ||
| let shouldInclude = await MainActor.run { | ||
| should_show_event(state: damus_state, ev: canonical.base) | ||
| } | ||
| guard shouldInclude else { | ||
| Log.debug("Filtered Vine event %s via should_show_event", for: .timeline, canonical.base.id.hex()) | ||
| return | ||
| } | ||
|
|
||
| await MainActor.run { | ||
| if let index = vines.firstIndex(where: { $0.dedupeKey == video.dedupeKey }) { | ||
| if vines[index].createdAt >= video.createdAt { | ||
| return | ||
| } | ||
| vines[index] = video | ||
| } else { | ||
| vines.append(video) | ||
| } | ||
| vines.sort { $0.createdAt > $1.createdAt } | ||
| lastSeenTimestamp = max(lastSeenTimestamp ?? 0, video.createdAt) | ||
| } | ||
| } | ||
|
|
||
| private func canonicalEvent(for event: NostrEvent) -> (base: NostrEvent, repost: NostrEvent?) { | ||
| guard event.known_kind == .boost else { | ||
| return (event, nil) | ||
| } | ||
|
|
||
| if let inner = event.get_inner_event(cache: damus_state.events), | ||
| inner.known_kind == .vine_short { | ||
| return (inner, event) | ||
| } | ||
| return (event, nil) | ||
| } | ||
|
|
||
| private func disconnectManagedRelayIfNeeded() async { | ||
| let shouldDisconnect = await MainActor.run { self.managedRelayConnection } | ||
| guard shouldDisconnect else { return } | ||
| await damus_state.nostrNetwork.disconnectRelay(.vineRelay) | ||
| await MainActor.run { self.managedRelayConnection = false } | ||
| } | ||
|
|
||
| private func loadInitialPage() async { | ||
| let start = CFAbsoluteTimeGetCurrent() | ||
| await MainActor.run { | ||
| isLoading = true | ||
| vines.removeAll() | ||
| } | ||
| let events = await fetchPage(before: nil) | ||
| await MainActor.run { | ||
| applyPage(events, reset: true) | ||
| isLoading = false | ||
| Log.info("Vines initial page loaded %d events in %.2fs", for: .timeline, events.count, CFAbsoluteTimeGetCurrent() - start) | ||
| } | ||
| } | ||
|
|
||
| private func loadOlderPage() async { | ||
| let before = await MainActor.run { self.oldestTimestamp } | ||
| guard let before else { return } | ||
| let start = CFAbsoluteTimeGetCurrent() | ||
| let events = await fetchPage(before: before > 0 ? before - 1 : 0) | ||
| if events.isEmpty { | ||
| await MainActor.run { | ||
| self.hasMoreOlder = false | ||
| self.isLoadingOlder = false | ||
| Log.debug("Vines older page empty at timestamp %u", for: .timeline, before) | ||
| } | ||
| return | ||
| } | ||
| await MainActor.run { | ||
| applyPage(events, reset: false) | ||
| Log.info("Vines older page loaded %d events in %.2fs", for: .timeline, events.count, CFAbsoluteTimeGetCurrent() - start) | ||
| } | ||
| } | ||
|
|
||
| private func fetchPage(before: UInt32?) async -> [NostrEvent] { | ||
| var filter = NostrFilter(kinds: [.vine_short]) | ||
| filter.limit = UInt32(pageSize) | ||
| let now = UInt32(Date().timeIntervalSince1970) | ||
| filter.until = before ?? now | ||
| return await damus_state.nostrNetwork.reader.query(filters: [filter], to: [.vineRelay], timeout: .seconds(10)) | ||
| } | ||
|
|
||
| @MainActor | ||
| private func applyPage(_ events: [NostrEvent], reset: Bool) { | ||
| var videos = events.compactMap { VineVideo(event: $0) } | ||
| videos.sort { $0.createdAt > $1.createdAt } | ||
| if reset { | ||
| vines = videos | ||
| } else { | ||
| let newVideos = videos.filter { video in | ||
| !vines.contains(where: { $0.dedupeKey == video.dedupeKey }) | ||
| } | ||
| vines.append(contentsOf: newVideos) | ||
| vines.sort { $0.createdAt > $1.createdAt } | ||
| if newVideos.isEmpty { | ||
| hasMoreOlder = false | ||
| } | ||
| Log.debug("Vines older page appended %d new events (filtered %d duplicates)", for: .timeline, newVideos.count, videos.count - newVideos.count) | ||
| } | ||
| if let newest = vines.first?.createdAt { | ||
| lastSeenTimestamp = max(lastSeenTimestamp ?? 0, newest) | ||
| } | ||
| if let oldest = vines.last?.createdAt { | ||
| oldestTimestamp = oldest | ||
| } | ||
| if hasMoreOlder { | ||
| hasMoreOlder = videos.count == pageSize | ||
| } | ||
| isLoadingOlder = false | ||
| } | ||
|
|
||
| @MainActor | ||
| private var shouldPrefetchVideos: Bool { | ||
| if pathIsConstrained { | ||
| return false | ||
| } | ||
| if pathIsExpensive && !damus_state.settings.prefetch_vines_on_cellular { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| @MainActor | ||
| private func prefetch(url: URL, allowCellular: Bool) async { | ||
| guard markPrefetching(url) else { return } | ||
| var request = URLRequest(url: url) | ||
| request.allowsExpensiveNetworkAccess = allowCellular | ||
| request.allowsConstrainedNetworkAccess = allowCellular | ||
| request.timeoutInterval = 15 | ||
| do { | ||
| _ = try await URLSession.shared.data(for: request) | ||
| } catch { | ||
| Log.debug("Vine prefetch failed for %s: %s", for: .timeline, url.absoluteString, error.localizedDescription) | ||
| } | ||
| unmarkPrefetching(url) | ||
| } | ||
|
|
||
| @MainActor | ||
| private func markPrefetching(_ url: URL) -> Bool { | ||
| if prefetchingURLs.contains(url) { | ||
| return false | ||
| } | ||
| prefetchingURLs.insert(url) | ||
| return true | ||
| } | ||
|
|
||
| @MainActor | ||
| private func unmarkPrefetching(_ url: URL) { | ||
| prefetchingURLs.remove(url) | ||
| } | ||
|
|
||
| @MainActor | ||
| private func maybeLoadOlder(after index: Int) { | ||
| guard hasMoreOlder, !isLoadingOlder else { return } | ||
| if index >= vines.count - 5 { | ||
| isLoadingOlder = true | ||
| Task { | ||
| await self.loadOlderPage() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| deinit { | ||
| pathMonitor.cancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
~1,200 lines of new Vine types packed into PostingTimelineView.swift.
This file now contains 7 new types (VineTimelineView, VineFeedModel, VineVideo, VineCard, VineMetadataRow, VineFullScreenPager, VineFullScreenPage) that have nothing to do with PostingTimelineView itself. This makes the file very hard to navigate, test in isolation, and maintain.
Consider splitting into the existing damus/Features/Vines/ directory structure, e.g.:
VineTimelineView.swift+VineFeedModel.swiftVineVideo.swift(model)VineCard.swift,VineFullScreenPager.swift,VineFullScreenPage.swift(views)
Also applies to: 574-1018, 1020-1368
🤖 Prompt for AI Agents
In `@damus/Features/Timeline/Views/PostingTimelineView.swift` around lines 181 -
572, The file contains seven Vine-related types bundled into
PostingTimelineView.swift which makes it unmaintainable; split them into focused
files under damus/Features/Vines/: create VineTimelineView.swift (containing
VineTimelineView), VineFeedModel.swift (VineFeedModel), VineVideo.swift
(VineVideo model), VineCard.swift (VineCard and VineMetadataRow), and
VineFullScreen.swift (VineFullScreenPager and VineFullScreenPage), move each
type into its new file preserving their implementation and imports, adjust
access levels (private/internal/public) as needed so PostingTimelineView still
compiles, update any references/usages (e.g., VineTimelineView initializer and
VineFeedModel usage) and run a build to fix import or visibility errors.
|
the first commit I see adds locks to places that doesn't need locks... not a good start |
|
rebasing, addressing this, all other feedback
|
- Recognize vine_short Nostr events (kind 34236) - Add Vine relay setting and move VideoCache to shared location - Implement VineFeedModel with relay subscription and event handling - Parse VineVideo from Nostr event tags (imeta, title, stats, origin) - Handle Vine reposts and surface metadata in UI - Add dedicated Vine tab and timeline to Home - Guard Contacts mutations with lock for concurrency safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
- Expand Vine parsing with fixture tests and fallback URL handling - Add EventActionBar, profile display names, and reporting menu - Implement full-screen vertical pager with video playback - Add network-aware video prefetch with URLSession fallback - Implement cursor-based pagination for older Vine events - Gate Vine tab behind Labs toggle - Disconnect Divine relay when Vine tab closes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
- Fix P0 critical issues: replace locks with actors, fix async blocking - Strip GPS metadata from video attachments before upload - Reprocess imported videos to remove EXIF location data - Handle picker dismissal and transcode Vine videos - Add task cancellation and timeout to video export - Add Vine integration fixture tests and pagination telemetry - Force Vine feature on in debug builds Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
- Add relay ownership tracking to prevent deleting user-configured relays - Move state mutation out of SwiftUI body evaluation - Move video metadata extraction off main thread - Fix Vine prefetch to persist to VideoCache correctly - Fix actor isolation in Vine replacement tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
- Extract Vine types from PostingTimelineView into dedicated files - Add @unchecked Sendable to VineVideo with documented justification - Replace NSLock with @mainactor isolation in NostrNetworkManager - Remove vine relay from bootstrap list (on-demand via ensureRelayConnected) - Track and cancel prefetch tasks in VineFeedModel.stop() - Add docstrings, accessibility annotations, and unit tests - Address CodeRabbit and AGENTS.md review feedback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Closes damus-io#3619 Changelog-Added: Added experimental Vine video viewer to Damus Labs Signed-off-by: alltheseas <alltheseas@users.noreply.github.com>
b779b61 to
204438f
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@damus.xcodeproj/project.pbxproj`:
- Around line 4271-4272: There are duplicate Creation group and
VineComposerView.swift file references (group IDs 048E0CF9 and 048E0CDD,
fileRefs 048E0CF8 and 048E0CDC); remove the stray duplicate group (048E0CF9) and
its fileRef (048E0CF8) so only the intended Creation group (048E0CDD) and
fileRef (048E0CDC) remain under the Vines hierarchy, then add a PBXBuildFile
entry for the remaining VineComposerView.swift (referencing 048E0CDC) and
include that PBXBuildFile entry in the appropriate target’s PBXSourcesBuildPhase
so the file is compiled.
- Line 1527: Remove the orphaned PBXBuildFile entry with ID
D73E5EFE2C6A97F4007EB227 from the PBXBuildFile section and also delete the
matching reference to that ID from the Sources build phase where it appears (the
duplicate entry in the build phase). Ensure no other references to
D73E5EFE2C6A97F4007EB227 remain in the project file so the project.pbxproj stays
consistent.
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift`:
- Around line 272-287: ensureRelayConnected currently marks relays that already
exist in the pool as feature-managed (via featureManagedRelays.insert(relayURL))
and so user-configured relays can be later removed by disconnectRelay; change
the logic in ensureRelayConnected(_:): if await pool.get_relay(relayURL) != nil
just return without inserting into featureManagedRelays, and only insert into
featureManagedRelays after you actually add the relay (i.e. after a successful
try await pool.add_relay(descriptor) or by verifying the relay was newly present
post-add/connect); reference ensureRelayConnected, pool.get_relay(_:),
pool.add_relay(_:), pool.connect(to:), featureManagedRelays, and
disconnectRelay(_:) when making this change.
In `@damus/Features/Settings/Models/UserSettingsStore.swift`:
- Around line 118-119: The default for the relay setting is inconsistent with
the vine feature; change the `@Setting` for enable_vine_relay so its default_value
is false (matching enable_vine_feature's opt-in behavior) to make both features
opt-in by default; update the `@Setting` declaration for enable_vine_relay in
UserSettingsStore (the property named enable_vine_relay) accordingly and run
tests/verify UI behavior since the relay toggle remains hidden until
enable_vine_feature is true.
In `@damus/Features/Timeline/Views/MainTabView.swift`:
- Around line 82-86: The keyboard shortcuts are hardcoded per TabButton so when
settings.vines_feature_enabled is false the sequence skips "2"; to fix, build
the visible tab list dynamically in MainTabView (or compute an incremental index
when creating each TabButton) and assign each TabButton.keyboardShortcut using
the position in that visibleTabs array (e.g., index + 1 as the numeric shortcut
string) instead of fixed "2"/"3"/"4"; update the TabButton creation logic that
references timeline: .vines / .search / .notifications, selected, nstatus,
settings, action to iterate visible tabs and set keyboardShortcut based on their
computed order.
In `@damus/Features/Vines/Creation/VineComposerView.swift`:
- Around line 274-296: convertVideoToMP4IfNeeded currently calls
exporter.exportAsynchronously with no timeout, so a stalled AVAssetExportSession
can never resume the continuation; add a 30s timeout: after creating exporter,
schedule a timeout task (e.g., DispatchQueue.main.asyncAfter or a
Task.sleep-backed Task) that calls exporter.cancelExport() and resumes the
continuation with nil if the exporter hasn't completed, and in the
exportAsynchronously completion branch resume with destinationURL on .completed
and nil on other statuses; ensure you guard against double-resume (track a
Boolean flag or use continuation.resume only once) so continuation is always
resumed exactly once.
In `@damus/Features/Vines/Models/VineFeedModel.swift`:
- Line 24: lastSeenTimestamp is currently written inside MainActor.run but read
off-main in stream(), causing a potential race; fix by making lastSeenTimestamp
MainActor-isolated (annotate the property with `@MainActor` private var
lastSeenTimestamp: UInt32? and update callers to await reads) or, alternatively,
ensure stream() reads it inside await MainActor.run { ... } before building the
filter; reference the property lastSeenTimestamp, the stream() method, and the
existing MainActor.run write locations when applying the change.
- Around line 22-23: The data race occurs because prefetchTasks is mutated from
both `@MainActor` code (noteAppeared) and non-isolated code (stop called from
subscribe/refresh/handleSettingsChange); fix by making stop()
MainActor-isolated: add `@MainActor` to the stop() declaration so all mutations of
prefetchTasks happen on the MainActor (or alternatively annotate the
prefetchTasks property with `@MainActor` and wrap the cleanup in MainActor.run if
you prefer async cleanup) — update the stop() implementation (and any direct
accesses to prefetchTasks inside it) to assume MainActor isolation and adjust
callers if necessary.
- Around line 301-331: In prefetch(url:allowCellular:) replace
URLSession.shared.data(for:) with URLSession.shared.download(for: request) so
the response is streamed to a temporary file instead of buffered in memory; on
success check the HTTPURLResponse status code, get VideoCache.standard and its
url_to_cached_url(url:) target, then move (or copy) the temporary downloaded
file to the cachedURL using FileManager (ensuring atomic move and proper error
handling), and still call unmarkPrefetching in the defer; preserve
request.allowsExpensiveNetworkAccess, request.allowsConstrainedNetworkAccess,
and timeoutInterval and log errors similarly.
- Around line 111-118: Currently completed prefetch tasks are never removed
because isCancelled only marks cancellations; change the tracking to remove
tasks when they finish by mapping URLs to their Task (e.g., a Dictionary<URL,
Task<Void, Never>> or a Set of in-flight URLs) and inside the Task.detached
closure (the work started in Task.detached and the call to
prefetch(url:allowCellular:)) remove the entry for that URL on normal completion
(use defer or a finally-style removal) so finished tasks are cleared from
prefetchTasks/inFlight map; also keep any existing cancellation cleanup (remove
cancelled tasks) as a fallback.
In `@damus/Features/Vines/Models/VineVideo.swift`:
- Around line 439-460: The format detection in imetaEntries(in:) incorrectly
treats the whole payload as inline if any payload element contains a space;
update the heuristic to decide inline vs alternating by inspecting the first
payload element (e.g., check if payload.first contains a space and splits into
exactly two parts) or attempt to parse both formats in one pass (try splitting
each element into key/value when it matches "key value", otherwise fall back to
consuming alternating pairs from the iterator); modify the logic around
usesInlineFormat and the subsequent parsing branches in imetaEntries to use this
more accurate check so values with spaces (like alt-text) aren’t misclassified.
In `@damus/Features/Vines/Views/VineCard.swift`:
- Around line 34-35: The top-level accessibility modifier
.accessibilityElement(children: .combine) applied alongside
accessibilityLabel(Text(vine.altText ?? vine.title)) on the VineCard is
collapsing the whole card into one element and hiding interactive children;
remove or relocate that modifier so interactive controls remain individually
accessible: delete or move .accessibilityElement(children: .combine) from the
top-level view in VineCard.swift, then, if needed, apply
.accessibilityElement(children: .combine) only to non-interactive subviews
(e.g., image/description areas) while leaving the overflow menu, the "Reveal"
button, the "Open backup stream" button and the EventActionBar as separate
accessibility elements.
- Around line 201-206: The computed property authorDisplayName currently calls
damus_state.profiles.lookup(id: vine.event.pubkey) synchronously on the main
thread; move that DB lookup out of the view body by removing the synchronous
lookup from authorDisplayName and instead introduce a state-backed async load
(e.g. `@State` private var authorDisplayName: String = vine.authorDisplay), then
in init/onAppear/.task start a background Task (or
Task.detached/DispatchQueue.global) that calls damus_state.profiles.lookup(id:
vine.event.pubkey) off the main thread, compute
Profile.displayName(profile:pubkey) there, and update the `@State` value via
MainActor.run so the view updates; ensure authorDisplayName property now just
returns the `@State` value and no longer performs the blocking
damus_state.profiles.lookup call.
In `@damus/Features/Vines/Views/VineFullScreenPage.swift`:
- Around line 44-53: The fallback-button is shown whenever vine.fallbackURL
exists even if the primary video is playing; update the UI so the Button is only
shown when the primary stream is unavailable or not playing (e.g., guard on
vine.primaryURL == nil or check the playback state like player.isPlaying ==
false) or alternatively rename the Label text from NSLocalizedString("Open
backup stream", ...) to something neutral like "Open in Browser" to avoid
implying an error; modify the conditional around the Button in
VineFullScreenPage (the block referencing vine.fallbackURL and the Label/Button)
to implement one of these fixes.
In `@damus/Features/Vines/Views/VineFullScreenPager.swift`:
- Around line 15-16: The current index-based `@State` private var selection: Int
in VineFullScreenPager breaks when model.vines mutates; change selection to
store a stable identifier (e.g. `@State` private var selectionID: Vine.ID or
selectionKey: String using vine.dedupeKey), update all Page/TabView tags from
the enumerated index to the vine.id/dedupeKey, and update any places that
read/write selection to map between the ID and the current array index (use
model.vines.firstIndex(where: { $0.id == selectionID }) or similar) so selection
remains stable across inserts/removals; adjust functions/logic that previously
relied on selection (e.g., computing currentVine, jumping pages) to use the
stable ID-to-index lookup.
In `@damus/Features/Vines/Views/VineTimelineView.swift`:
- Around line 28-37: fullScreenIndex is vulnerable to array mutations because it
uses the enumerated offset from the ForEach; instead pass a stable identifier
and/or clamp the index when opening the pager: change VineCard's
onOpenFullScreen to send the tapped vine's id (vine.id) rather than the current
enumerated index, then update the presentation logic in VineFullScreenPager to
resolve that id to a current index (find the index in model.vines by comparing
vine.id and clamp to 0..<model.vines.count) before using it; alternatively, if
you keep fullScreenIndex, ensure VineFullScreenPager validates and clamps
fullScreenIndex against model.vines.count on appear to avoid out-of-bounds or
stale selections.
In `@damus/Shared/Media/Models/MediaPicker.swift`:
- Around line 141-153: In attemptAcquireResourceAndChooseMedia, ensure
dispatchGroup.leave() is always called even when the async fallback returns nil:
when url.startAccessingSecurityScopedResource() succeeds call chooseMedia as
before and leave the group; when using Task and fallback(url) returns nil make
sure to call dispatchGroup.leave() before exiting the Task (or use a
defer/ensure pattern inside the Task) so the group is balanced; update the Task
fallback path accordingly and audit other early-return guards (e.g., the guard
let url = item as? URL) to likewise always call dispatchGroup.leave() on all
exit paths.
🧹 Nitpick comments (20)
.beads/issues.jsonl (1)
1-25: Consider whether.beads/should be committed to the repository.This JSONL file tracks agent-internal issue state (all closed or in-progress items from the development session). Committing it adds ~25 records of transient development metadata to the repo. Unless
.beads/is intended as a permanent project artifact, consider adding it to.gitignoreor removing it before merge to keep the repository clean.#!/bin/bash # Check if .beads is in .gitignore or if there's a metadata file explaining its purpose cat .gitignore 2>/dev/null | grep -i bead cat .beads/metadata.json 2>/dev/nulldamus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift (1)
284-284: Silently discardingadd_relayerrors withtry?.While
RelayPool.add_relaycurrently only throwsRelayAlreadyExists, silently ignoring the error means the code proceeds toconnectand marks the relay as feature-managed even if addition failed for an unexpected reason. Consider at least logging the error.Proposed improvement
- try? await pool.add_relay(descriptor) + do { + try await pool.add_relay(descriptor) + } catch { + Log.warning("Failed to add relay %s: %s", for: .networking, relayURL.id as CVarArg, error.localizedDescription as CVarArg) + return + }damus/Features/Relays/Views/UserRelaysView.swift (1)
57-67: No feedback on toggle failure — consider showing an error if relay connect/disconnect fails.
setDivineRelayEnabledfires the async relay operation in aTaskbut doesn't handle errors. IfensureRelayConnectedordisconnectRelayfails silently, the toggle will show the relay as enabled/disabled while the actual connection state differs. For a POC this is acceptable, but worth a TODO.damus/Features/Vines/Creation/VineComposerView.swift (2)
12-28: CustomEquatableconformance is unnecessary — Swift auto-synthesizes it for enums withEquatableassociated values.
StringisEquatable, soUploadPhasegets synthesis for free. This manual implementation can be removed.Proposed simplification
- enum UploadPhase: Equatable { + enum UploadPhase: Equatable { case idle case uploading case uploaded case failed(String) - - static func == (lhs: UploadPhase, rhs: UploadPhase) -> Bool { - switch (lhs, rhs) { - case (.idle, .idle), (.uploading, .uploading), (.uploaded, .uploaded): - return true - case let (.failed(lhsMessage), .failed(rhsMessage)): - return lhsMessage == rhsMessage - default: - return false - } - } }
263-272: Use async/await AVURLAsset APIs to avoid deprecation warnings.The synchronous
.tracks(withMediaType:)and.durationproperties are deprecated as of iOS 16. Since this code already runs on a background thread (Task.detached), replace these with the async equivalents:
asset.tracks(withMediaType:)→await asset.loadTracks(withMediaType:)asset.duration→await asset.load(.duration)track.naturalSize→await track.load(.naturalSize)track.preferredTransform→await track.load(.preferredTransform)The function would need to become
asyncto accommodate these changes.damus/Features/Settings/Models/UserSettingsStore.swift (1)
115-148: Missing docstrings on new settings properties.Per coding guidelines, all added or modified code should have docstring coverage. The three new
@Settingproperties and thevines_feature_enabledcomputed property lack documentation. Consider adding brief///comments like the existing ones (e.g., Line 260–261 forundistractMode). As per coding guidelines, "Ensure docstring coverage for any code added or modified."damus/Shared/Media/Images/ImageProcessing.swift (2)
165-212: Async export with timeout — well structured, but add a docstring.The
withTaskGroup+withCheckedContinuationpattern correctly races the export against a 30-second timeout, andcancelAll()properly tears down the losing task. The continuation safety is maintained becausewithTaskGroupawaits all children before returning.One note:
exportSession.cancelExport()is called on line 206 even when the export already failed (not just on timeout). This is harmless but could be guarded with a more precise condition.As per coding guidelines, "Ensure docstring coverage for any code added or modified" — this private function lacks a
///docstring.
58-68: RemovesaveVideoToTemporaryFolder— it is unused dead code.The function is never called anywhere in the codebase and can be safely deleted.
damus.xcodeproj/project.pbxproj (1)
7039-7045: Remove Vine UI files from ShareExtension target.The Vine UI files (VineCard, VineFullScreenPage, VineFullScreenPager, VineMetadataRow, VineTimelineView, VineFeedModel) are included in ShareExtension's Sources build phase but are not used anywhere in the extension code. Share extensions have minimal footprint requirements, and including these views unnecessarily increases the extension's binary size.
damus/Features/Vines/Models/VineVideo.swift (3)
409-417: Redundantkey == "pm-report"check.
"pm-report".hasPrefix("pm-")is alreadytrue, so the|| key == "pm-report"clause is dead logic.Proposed simplification
- if key == "proof" || key.hasPrefix("pm-") || key == "pm-report" { + if key == "proof" || key.hasPrefix("pm-") {
267-278:NSDataDetectorinstantiation on everycollectContentURLscall.
NSDataDetector(types:)allocates anNSRegularExpressioninternally. SinceVineVideoinit may be called in a tight loop (e.g.,applyPagewith 40 events), consider hoisting the detector to astatic letto avoid repeated allocation.Proposed change
+ private static let linkDetector: NSDataDetector? = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue) + private static func collectContentURLs(from content: String?, into candidates: inout [URL: MediaCandidate]) { guard let content, !content.isEmpty else { return } - guard let detector = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue) else { return } + guard let detector = linkDetector else { return } let range = NSRange(content.startIndex..<content.endIndex, in: content)
280-288:collectFallbackURLsiterates every value in every tag — intentionally broad.When
candidateMapis empty, this scans all tag values as potential URLs. WhilenormalizedURLfilters out non-http(s) strings, this could inadvertently pick up unrelated URLs (e.g., relay URLs in"r"tags, profile links). The behavior is guarded by thecandidateMap.isEmptycheck on Line 172, so the blast radius is limited.Worth a brief inline comment explaining the intent to future maintainers.
damusTests/Fixtures/VineFixtures.swift (2)
5-5: AI tool attribution in the file header.The
Created by OpenAI Codexline is atypical for project file headers. Consider replacing with the actual author or contributor name for consistency with the rest of the codebase.
73-77:repostfixture doesn't model an actual Nostr repost event.This fixture is a standard vine event, identical in structure to
replacementOriginal. A Nostr repost (kind 6 /.boost) wraps an inner event. To test thecanonicalEvent(for:)/ repost path inVineFeedModel, you'd need a fixture that represents the outer boost event containing the inner vine event. The tests currently useVineVideo(event:, repostSource:)with a separate event, which is valid, but naming this fixturerepostis misleading since it's just a regular vine.damus/Features/Vines/Models/VineFeedModel.swift (1)
184-195: O(n log n) sort on every incoming event during streaming.
vines.sortis called insidehandle(event:)for each event received. During the initial stream, this could process up to 200 events (Line 146), resulting in quadratic total work. Consider deferring the sort or using binary insertion.damusTests/NostrEventTests.swift (2)
219-222: DuplicatedmakeVineEventhelper across both test classes.The identical helper appears in
VineVideoTests(Line 219) andVineFeedModelTests(Line 308). Extract it to a shared file-level or extension-level helper to avoid duplication.Also, the force unwrap
NostrEvent(...)!will crash without a useful message if event creation fails. Consider usingXCTUnwrapin a throwing helper for better test diagnostics.Proposed extraction
+// MARK: - Shared Vine Test Helpers + +private func makeVineEvent(content: String = "", tags: [[String]], createdAt: UInt32 = UInt32(Date().timeIntervalSince1970)) -> NostrEvent { + let keypair = generate_new_keypair().to_keypair() + return NostrEvent(content: content, keypair: keypair, kind: NostrKind.vine_short.rawValue, tags: tags, createdAt: createdAt)! +}Then remove the per-class
makeVineEventmethods.Also applies to: 308-311
329-367:VineTestFeedmirrorsVineFeedModelcore logic — divergence risk.The
applyandapplyPagemethods duplicate the dedup/sort logic fromVineFeedModel. If the production logic changes (e.g., dedup strategy, sort order), these tests could pass while production behavior differs. This is acceptable for a POC but consider adding a comment or a TODO to keep these in sync.damus/ContentView.swift (1)
165-166:immersiveTimelineis computed in two separate places with identical logic.The same expression
selected_timeline == .home || selected_timeline == .vinesappears in bothMainContent(Line 165) andbody(Line 256). If the set of immersive timelines grows, one site could be updated while the other is missed.Consider extracting this into a single computed property on
ContentView.♻️ Proposed refactor
+ /// Whether the currently selected timeline should use immersive (edge-to-edge) layout. + private var immersiveTimeline: Bool { + selected_timeline == .home || selected_timeline == .vines + } + func MainContent(damus: DamusState) -> some View { - let immersiveTimeline = selected_timeline == .home || selected_timeline == .vines - return VStack { + VStack {And in
body:var body: some View { - let immersiveTimeline = selected_timeline == .home || selected_timeline == .vines - - return VStack(alignment: .leading, spacing: 0) { + VStack(alignment: .leading, spacing: 0) {Also applies to: 256-258
damus/Features/Vines/Views/VineTimelineView.swift (2)
61-62: Disconnecting from the relay on every disappear may hurt perceived performance.
onDisappearfires on every tab switch, so each visit to Vines incurs a reconnect cost. Consider deferring disconnect (e.g., with a short delay that is cancelled if the user returns quickly) or disconnecting only when the parent view is truly torn down.
17-20: Missing docstring oninit. As per coding guidelines, ensure docstring coverage for any code added or modified.
| 048E0CF92F3C5D2E00106E91 /* Creation */, | ||
| 048E0CE72F3C5C9500106E91 /* Vines */, |
There was a problem hiding this comment.
Duplicate Creation group and VineComposerView.swift file references.
Under the Features group, two Creation groups are added:
048E0CF9(Line 4271) — a standalone sibling ofVines, containing its ownVineComposerView.swift(048E0CF8, Line 2052).048E0CDD— nested insideVines/Creation(Lines 3045–3052), containing a differentVineComposerView.swiftreference (048E0CDC, Line 2044).
This results in two Creation folders and two separate file references for VineComposerView.swift. Additionally, neither VineComposerView.swift appears in any PBXBuildFile section, so neither is compiled. This looks like a rebasing artifact — remove the duplicate group and consolidate to a single file reference, and add the appropriate build file entry if the file should be compiled.
🤖 Prompt for AI Agents
In `@damus.xcodeproj/project.pbxproj` around lines 4271 - 4272, There are
duplicate Creation group and VineComposerView.swift file references (group IDs
048E0CF9 and 048E0CDD, fileRefs 048E0CF8 and 048E0CDC); remove the stray
duplicate group (048E0CF9) and its fileRef (048E0CF8) so only the intended
Creation group (048E0CDD) and fileRef (048E0CDC) remain under the Vines
hierarchy, then add a PBXBuildFile entry for the remaining
VineComposerView.swift (referencing 048E0CDC) and include that PBXBuildFile
entry in the appropriate target’s PBXSourcesBuildPhase so the file is compiled.
| /// Ensures the relay pool is connected to a specific relay, adding it if necessary. | ||
| /// | ||
| /// Used by feature-specific code (e.g., Vines) to connect to relays that are | ||
| /// not part of the user's configured relay list. Tracks the relay so that | ||
| /// ``disconnectRelay(_:)`` can safely remove it later. | ||
| func ensureRelayConnected(_ relayURL: RelayURL) async { | ||
| if await pool.get_relay(relayURL) != nil { | ||
| await MainActor.run { featureManagedRelays.insert(relayURL) } | ||
| return | ||
| } | ||
|
|
||
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | ||
| try? await pool.add_relay(descriptor) | ||
| await pool.connect(to: [relayURL]) | ||
| await MainActor.run { featureManagedRelays.insert(relayURL) } | ||
| } |
There was a problem hiding this comment.
ensureRelayConnected marks pre-existing (user-configured) relays as feature-managed, allowing disconnectRelay to remove them.
When the relay already exists in the pool (Lines 278-280), it's unconditionally added to featureManagedRelays. If the user independently configured wss://relay.divine.video, toggling the Vine feature off would call disconnectRelay and remove their relay.
Only insert into featureManagedRelays when you actually added the relay:
Proposed fix
func ensureRelayConnected(_ relayURL: RelayURL) async {
if await pool.get_relay(relayURL) != nil {
- await MainActor.run { featureManagedRelays.insert(relayURL) }
+ // Relay already exists — don't claim ownership
return
}
let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite)
try? await pool.add_relay(descriptor)
await pool.connect(to: [relayURL])
await MainActor.run { featureManagedRelays.insert(relayURL) }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Ensures the relay pool is connected to a specific relay, adding it if necessary. | |
| /// | |
| /// Used by feature-specific code (e.g., Vines) to connect to relays that are | |
| /// not part of the user's configured relay list. Tracks the relay so that | |
| /// ``disconnectRelay(_:)`` can safely remove it later. | |
| func ensureRelayConnected(_ relayURL: RelayURL) async { | |
| if await pool.get_relay(relayURL) != nil { | |
| await MainActor.run { featureManagedRelays.insert(relayURL) } | |
| return | |
| } | |
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | |
| try? await pool.add_relay(descriptor) | |
| await pool.connect(to: [relayURL]) | |
| await MainActor.run { featureManagedRelays.insert(relayURL) } | |
| } | |
| /// Ensures the relay pool is connected to a specific relay, adding it if necessary. | |
| /// | |
| /// Used by feature-specific code (e.g., Vines) to connect to relays that are | |
| /// not part of the user's configured relay list. Tracks the relay so that | |
| /// ``disconnectRelay(_:)`` can safely remove it later. | |
| func ensureRelayConnected(_ relayURL: RelayURL) async { | |
| if await pool.get_relay(relayURL) != nil { | |
| // Relay already exists — don't claim ownership | |
| return | |
| } | |
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | |
| try? await pool.add_relay(descriptor) | |
| await pool.connect(to: [relayURL]) | |
| await MainActor.run { featureManagedRelays.insert(relayURL) } | |
| } |
🤖 Prompt for AI Agents
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift` around
lines 272 - 287, ensureRelayConnected currently marks relays that already exist
in the pool as feature-managed (via featureManagedRelays.insert(relayURL)) and
so user-configured relays can be later removed by disconnectRelay; change the
logic in ensureRelayConnected(_:): if await pool.get_relay(relayURL) != nil just
return without inserting into featureManagedRelays, and only insert into
featureManagedRelays after you actually add the relay (i.e. after a successful
try await pool.add_relay(descriptor) or by verifying the relay was newly present
post-add/connect); reference ensureRelayConnected, pool.get_relay(_:),
pool.add_relay(_:), pool.connect(to:), featureManagedRelays, and
disconnectRelay(_:) when making this change.
| @Setting(key: "enable_vine_relay", default_value: true) | ||
| var enable_vine_relay: Bool |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "enable_vine_relay" --type swift -C3Repository: damus-io/damus
Length of output: 3650
🏁 Script executed:
# Also check how enable_vine_feature is used for comparison
rg -n "enable_vine_feature" --type swift -C3Repository: damus-io/damus
Length of output: 2554
🏁 Script executed:
# Search for relay-related code that might use these settings
rg -n "vine.*relay|relay.*vine" --type swift -i -C2Repository: damus-io/damus
Length of output: 10334
Consider aligning enable_vine_relay default with enable_vine_feature for consistency.
Since enable_vine_feature defaults to false (opt-in) but enable_vine_relay defaults to true (opt-out), users who enable the vine feature will have the Divine Relay auto-connected by default. While the relay toggle is hidden in the UI unless the vine feature is enabled, the asymmetric defaults means the relay becomes opt-out rather than opt-in. For consistency, consider defaulting enable_vine_relay to false so both features follow the same opt-in pattern.
🤖 Prompt for AI Agents
In `@damus/Features/Settings/Models/UserSettingsStore.swift` around lines 118 -
119, The default for the relay setting is inconsistent with the vine feature;
change the `@Setting` for enable_vine_relay so its default_value is false
(matching enable_vine_feature's opt-in behavior) to make both features opt-in by
default; update the `@Setting` declaration for enable_vine_relay in
UserSettingsStore (the property named enable_vine_relay) accordingly and run
tests/verify UI behavior since the relay toggle remains hidden until
enable_vine_feature is true.
| if settings.vines_feature_enabled { | ||
| TabButton(timeline: .vines, img: "vine", selected: $selected, nstatus: nstatus, settings: settings, action: action).keyboardShortcut("2") | ||
| } | ||
| TabButton(timeline: .search, img: "search", selected: $selected, nstatus: nstatus, settings: settings, action: action).keyboardShortcut("3") | ||
| TabButton(timeline: .notifications, img: "notification-bell", selected: $selected, nstatus: nstatus, settings: settings, action: action).keyboardShortcut("4") |
There was a problem hiding this comment.
Keyboard shortcuts skip "2" when vines tab is hidden.
When vines_feature_enabled is false, the tab bar goes from shortcut "1" (home) to "3" (search) to "4" (notifications), skipping "2". Consider dynamically assigning shortcuts so they remain sequential when the vines tab is absent.
🤖 Prompt for AI Agents
In `@damus/Features/Timeline/Views/MainTabView.swift` around lines 82 - 86, The
keyboard shortcuts are hardcoded per TabButton so when
settings.vines_feature_enabled is false the sequence skips "2"; to fix, build
the visible tab list dynamically in MainTabView (or compute an incremental index
when creating each TabButton) and assign each TabButton.keyboardShortcut using
the position in that visibleTabs array (e.g., index + 1 as the numeric shortcut
string) instead of fixed "2"/"3"/"4"; update the TabButton creation logic that
references timeline: .vines / .search / .notifications, selected, nstatus,
settings, action to iterate visible tabs and set keyboardShortcut based on their
computed order.
| private var authorDisplayName: String { | ||
| if let profile = try? damus_state.profiles.lookup(id: vine.event.pubkey) { | ||
| return Profile.displayName(profile: profile, pubkey: vine.event.pubkey).displayName | ||
| } | ||
| return vine.authorDisplay | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the profiles.lookup implementation
rg -n "func lookup" --type swift -A 10 | head -100Repository: damus-io/damus
Length of output: 6454
🏁 Script executed:
#!/bin/bash
# Find the Profiles class definition
rg -n "class Profiles" --type swift -B 2 -A 20Repository: damus-io/damus
Length of output: 3833
🏁 Script executed:
#!/bin/bash
# Search for where profiles is initialized/created to understand its type
rg -n "var profiles" --type swift | head -20Repository: damus-io/damus
Length of output: 532
🏁 Script executed:
#!/bin/bash
# Find lookup methods specifically in Profiles.swift
rg -n "func lookup" damus/Core/Nostr/Profiles.swift -A 15Repository: damus-io/damus
Length of output: 2182
🏁 Script executed:
#!/bin/bash
# Get full Profiles.swift file to understand its structure
wc -l damus/Core/Nostr/Profiles.swiftRepository: damus-io/damus
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Find ndb.lookup_profile implementation
rg -n "func lookup_profile\(" damus/Core/Nostr -A 20 --type swiftRepository: damus-io/damus
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Also check if lookup operations use SafeNdbTxn which might indicate safe concurrency handling
rg -n "lookup_profile" nostrdb/Ndb.swift -A 12 --type swiftRepository: damus-io/damus
Length of output: 4941
🏁 Script executed:
#!/bin/bash
# Find withNdb implementation
rg -n "func withNdb" nostrdb/Ndb.swift -A 8 --type swiftRepository: damus-io/damus
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Find SafeNdbTxn definition and implementation
rg -n "struct SafeNdbTxn\|class SafeNdbTxn" --type swift -A 15Repository: damus-io/damus
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check what ndb_get_profile_by_pubkey is (likely C function binding)
rg -n "ndb_get_profile_by_pubkey" --type swiftRepository: damus-io/damus
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Find SafeNdbTxn with different search
rg -n "SafeNdbTxn" --type swift | head -20Repository: damus-io/damus
Length of output: 1752
🏁 Script executed:
#!/bin/bash
# Understand what keepNdbOpen does
rg -n "keepNdbOpen" --type swift -B 2 -A 15 | head -50Repository: damus-io/damus
Length of output: 4818
🏁 Script executed:
#!/bin/bash
# Check the actual view body evaluation context - is this called on main thread?
rg -n "authorDisplayName" damus/Features/Vines/Views/VineCard.swift -B 5 -A 5Repository: damus-io/damus
Length of output: 1199
🏁 Script executed:
#!/bin/bash
# Check if ndb operates as an in-memory cache or if it actually performs I/O
rg -n "mmap\|disk\|file\|sqlite" nostrdb/Ndb.swift -i | head -20Repository: damus-io/damus
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at the actual C binding to understand the nature of ndb_get_profile_by_pubkey
fd -e h -e swift -path "*nostrdb*" | xargs grep -l "ndb_get_profile_by_pubkey" 2>/dev/null | head -5Repository: damus-io/damus
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Double check: is keepNdbOpen dispatching to background queue or executing synchronously?
cat nostrdb/NdbUseLock.swift | sed -n '37,45p'Repository: damus-io/damus
Length of output: 695
Move database lookup to background thread to avoid blocking main thread during view rendering.
The profiles.lookup(id:) call in authorDisplayName executes database queries synchronously on the main thread. When this computed property is evaluated during SwiftUI view body rendering (especially during scrolling), it blocks the UI. The entire call chain—profiles.lookup() → ndb.lookup_profile() → withNdb() → keepNdbOpen()—executes synchronously on the current thread and directly accesses nostrdb via the C function ndb_get_profile_by_pubkey().
Use @MainActor on the computed property or dispatch the lookup to a background task to prevent main thread blocking:
private var authorDisplayName: String {
if let profile = try? damus_state.profiles.lookup(id: vine.event.pubkey) {
return Profile.displayName(profile: profile, pubkey: vine.event.pubkey).displayName
}
return vine.authorDisplay
}Should be refactored to load profile asynchronously outside the view body evaluation.
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Views/VineCard.swift` around lines 201 - 206, The
computed property authorDisplayName currently calls
damus_state.profiles.lookup(id: vine.event.pubkey) synchronously on the main
thread; move that DB lookup out of the view body by removing the synchronous
lookup from authorDisplayName and instead introduce a state-backed async load
(e.g. `@State` private var authorDisplayName: String = vine.authorDisplay), then
in init/onAppear/.task start a background Task (or
Task.detached/DispatchQueue.global) that calls damus_state.profiles.lookup(id:
vine.event.pubkey) off the main thread, compute
Profile.displayName(profile:pubkey) there, and update the `@State` value via
MainActor.run so the view updates; ensure authorDisplayName property now just
returns the `@State` value and no longer performs the blocking
damus_state.profiles.lookup call.
| if let fallback = vine.fallbackURL { | ||
| Button { | ||
| openURL(fallback) | ||
| } label: { | ||
| Label(NSLocalizedString("Open backup stream", comment: "Action to open a fallback Vine video URL when the main stream fails."), systemImage: "arrow.up.right.square") | ||
| .font(.caption) | ||
| } | ||
| .buttonStyle(.borderedProminent) | ||
| .tint(.white.opacity(0.2)) | ||
| } |
There was a problem hiding this comment.
"Open backup stream" button appears even when the primary video is playing fine.
The button is shown whenever vine.fallbackURL is non-nil, regardless of primary playback status. This may confuse users who see a working video alongside a button that implies something is broken. Consider gating this on the primary URL being nil, or renaming to something like "Open in browser" to clarify it's an alternative rather than a recovery action.
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Views/VineFullScreenPage.swift` around lines 44 - 53,
The fallback-button is shown whenever vine.fallbackURL exists even if the
primary video is playing; update the UI so the Button is only shown when the
primary stream is unavailable or not playing (e.g., guard on vine.primaryURL ==
nil or check the playback state like player.isPlaying == false) or alternatively
rename the Label text from NSLocalizedString("Open backup stream", ...) to
something neutral like "Open in Browser" to avoid implying an error; modify the
conditional around the Button in VineFullScreenPage (the block referencing
vine.fallbackURL and the Label/Button) to implement one of these fixes.
| @State private var selection: Int | ||
|
|
There was a problem hiding this comment.
Index-based selection breaks when model.vines changes during full-screen playback.
selection is an Int index (Line 15) and pages are tagged with their enumerated index (Line 31). When the vines array is mutated — e.g., a new vine arrives via streaming or an older page is loaded and inserted — the indices shift. The selection value becomes stale: the user sees a different vine jump into view, or the index falls out of bounds.
Use the vine's stable id (or dedupeKey) as the selection type and tag instead:
Proposed fix — use stable ID for selection
- `@State` private var selection: Int
+ `@State` private var selection: String
init(model: VineFeedModel, damus_state: DamusState, initialIndex: Int, onClose: `@escaping` () -> Void) {
self._model = ObservedObject(wrappedValue: model)
self.damus_state = damus_state
- self._selection = State(initialValue: initialIndex)
+ let initialID = model.vines.indices.contains(initialIndex) ? model.vines[initialIndex].id : ""
+ self._selection = State(initialValue: initialID)
self.onClose = onClose
}
...
- ForEach(Array(model.vines.enumerated()), id: \.1.id) { index, vine in
+ ForEach(model.vines) { vine in
VineFullScreenPage(vine: vine, damus_state: damus_state)
.frame(width: geo.size.width, height: geo.size.height)
.rotationEffect(.degrees(-90))
- .tag(index)
+ .tag(vine.id)
}
...
.onAppear {
- model.noteAppeared(at: selection)
+ if let idx = model.vines.firstIndex(where: { $0.id == selection }) {
+ model.noteAppeared(at: idx)
+ }
}
- .onChange(of: selection) { idx in
- model.noteAppeared(at: idx)
+ .onChange(of: selection) { id in
+ if let idx = model.vines.firstIndex(where: { $0.id == id }) {
+ model.noteAppeared(at: idx)
+ }
}Also applies to: 26-32, 50-55
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Views/VineFullScreenPager.swift` around lines 15 - 16,
The current index-based `@State` private var selection: Int in VineFullScreenPager
breaks when model.vines mutates; change selection to store a stable identifier
(e.g. `@State` private var selectionID: Vine.ID or selectionKey: String using
vine.dedupeKey), update all Page/TabView tags from the enumerated index to the
vine.id/dedupeKey, and update any places that read/write selection to map
between the ID and the current array index (use model.vines.firstIndex(where: {
$0.id == selectionID }) or similar) so selection remains stable across
inserts/removals; adjust functions/logic that previously relied on selection
(e.g., computing currentVine, jumping pages) to use the stable ID-to-index
lookup.
| ForEach(Array(model.vines.enumerated()), id: \.1.id) { index, vine in | ||
| VineCard( | ||
| vine: vine, | ||
| damus_state: damus_state, | ||
| onAppear: { model.noteAppeared(at: index) }, | ||
| onOpenFullScreen: { | ||
| fullScreenIndex = index | ||
| presentingFullScreen = true | ||
| } | ||
| ) |
There was a problem hiding this comment.
Potential stale fullScreenIndex if vines array mutates before pager renders.
fullScreenIndex is set from the enumerated() offset. If model.vines changes (e.g., a refresh completing, deduplication removing items) between the tap and the full-screen cover appearing, the index could point to a different vine or be out of bounds. Consider clamping it in VineFullScreenPager or passing the vine's ID instead.
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Views/VineTimelineView.swift` around lines 28 - 37,
fullScreenIndex is vulnerable to array mutations because it uses the enumerated
offset from the ForEach; instead pass a stable identifier and/or clamp the index
when opening the pager: change VineCard's onOpenFullScreen to send the tapped
vine's id (vine.id) rather than the current enumerated index, then update the
presentation logic in VineFullScreenPager to resolve that id to a current index
(find the index in model.vines by comparing vine.id and clamp to
0..<model.vines.count) before using it; alternatively, if you keep
fullScreenIndex, ensure VineFullScreenPager validates and clamps fullScreenIndex
against model.vines.count on appear to avoid out-of-bounds or stale selections.
| private func attemptAcquireResourceAndChooseMedia(url: URL, fallback: @escaping (URL) async -> URL?, unprocessedEnum: (URL) -> PreUploadedMedia, processedEnum: @escaping (URL) -> PreUploadedMedia, orderId: String) { | ||
| if url.startAccessingSecurityScopedResource() { | ||
| // Have permission from system to use url out of scope | ||
| print("Acquired permission to security scoped resource") | ||
| self.chooseMedia(unprocessedEnum(url), orderId: orderId) | ||
| } else { | ||
| // Need to copy URL to non-security scoped location | ||
| guard let newUrl = fallback(url) else { return } | ||
| self.chooseMedia(processedEnum(newUrl), orderId: orderId) | ||
| Task { | ||
| guard let newUrl = await fallback(url) else { return } | ||
| self.chooseMedia(processedEnum(newUrl), orderId: orderId) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
dispatchGroup.leave() is skipped when the async fallback returns nil.
If fallback(url) returns nil, the guard exits the Task without calling chooseMedia, so dispatchGroup.leave() is never invoked for that item. This means dispatchGroup.notify on Line 122 will never fire, silently dropping all selected media.
This is a pre-existing issue (the synchronous path had the same problem), but the new async fallback makes failure more likely (e.g., video transcoding errors, cancellation). Consider ensuring the group is always left:
🐛 Proposed fix
Task {
- guard let newUrl = await fallback(url) else { return }
- self.chooseMedia(processedEnum(newUrl), orderId: orderId)
+ guard let newUrl = await fallback(url) else {
+ self.dispatchGroup.leave()
+ return
+ }
+ self.chooseMedia(processedEnum(newUrl), orderId: orderId)
}You may also want to audit the other early-return paths (e.g., guard let url = item as? URL on Line 67) for the same issue.
🤖 Prompt for AI Agents
In `@damus/Shared/Media/Models/MediaPicker.swift` around lines 141 - 153, In
attemptAcquireResourceAndChooseMedia, ensure dispatchGroup.leave() is always
called even when the async fallback returns nil: when
url.startAccessingSecurityScopedResource() succeeds call chooseMedia as before
and leave the group; when using Task and fallback(url) returns nil make sure to
call dispatchGroup.leave() before exiting the Task (or use a defer/ensure
pattern inside the Task) so the group is balanced; update the Task fallback path
accordingly and audit other early-return guards (e.g., the guard let url = item
as? URL) to likewise always call dispatchGroup.leave() on all exit paths.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
damus/Shared/Media/Images/ImageProcessing.swift (1)
58-68:⚠️ Potential issue | 🟡 MinorRemove
saveVideoToTemporaryFolderfunction.This function is defined but never called anywhere in the codebase. It should be removed.
🤖 Fix all issues with AI agents
In @.beads/metadata.json:
- Around line 1-4: Add a gitignore entry to prevent committing the SQLite DB
referenced in .beads/metadata.json by updating the repository .gitignore to
ignore beads.db (and preferably any database files under the .beads directory);
add either "beads.db" and/or ".beads/beads.db" (or a broader pattern like
".beads/*.db") to .gitignore, save, and commit the change so the generated
SQLite file cannot be accidentally checked in.
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift`:
- Around line 283-286: The code silently swallows errors from pool.add_relay
which can leave state inconsistent; change the try? await
pool.add_relay(descriptor) to explicit error handling: call await
pool.add_relay(descriptor) inside do/catch, handle the specific
RelayAlreadyExists error by treating it as success (so you can still proceed to
connect), rethrow or log and abort for other errors, and only insert relayURL
into featureManagedRelays after add_relay either succeeded or you confirmed the
relay already exists; keep pool.connect(to:) execution gated on successful
add/confirmed-existence. Use the symbols RelayPool.RelayDescriptor,
pool.add_relay(...), pool.connect(to:), and
featureManagedRelays.insert(relayURL) to locate and update the logic.
In `@damus/Features/Vines/Creation/VineComposerView.swift`:
- Around line 11-387: The file lacks required docstrings; add concise Swift
documentation comments (/// ...) to the VineComposerView type and the specified
methods: publishVine(), handlePickedMedia(_:), uploadSelectedMedia(_:), and
convertVideoToMP4IfNeeded(localURL:) describing their purpose, parameters (where
applicable), and return behavior; place the docstrings immediately above the
struct declaration and each method/ computed property (e.g., above private func
publishVine(), private func handlePickedMedia(_:), `@MainActor` private func
uploadSelectedMedia(_:), and private func convertVideoToMP4IfNeeded(localURL:)
-> URL?) following the existing comment style used elsewhere so the new file
meets the docstring coverage guideline.
- Around line 173-174: In VineComposerView, replace the deprecated Text modifier
call `.autocapitalization(.none)` with the iOS 15+ API
`.textInputAutocapitalization(.never)` where the view chain currently also uses
`.disableAutocorrection(true)`; locate the modifier chain on the text input in
VineComposerView and swap the autocapitalization modifier name and value to
`.textInputAutocapitalization(.never)` while leaving
`.disableAutocorrection(true)` intact.
In `@damus/Features/Vines/Models/VineVideo.swift`:
- Around line 280-288: collectFallbackURLs currently treats every tag value as a
possible media URL which yields many false-positive playback candidates;
restrict it to only consider tag entries that are likely videos by first
checking tag identifiers and URL path extensions before calling normalizedURL
and addCandidate. Update collectFallbackURLs to: inspect the tag key/name (from
NostrEvent.tags) and only attempt parsing values for known video-related tags
(e.g., "video", "media", "file", "source") or when the URL's pathExtension
(lowercased) matches a whitelist of video extensions (mp4, webm, mov, m3u8, ts,
mkv, avi); then call normalizedURL and mediaKind(for:) and addCandidate as
before, skipping common image/audio extensions to avoid thumbnails/profile
images.
🧹 Nitpick comments (15)
.beads/issues.jsonl (1)
1-29: Consider whether this issue-tracking export belongs in the repository.This file is a development workflow artifact from a "beads" issue tracker containing 29 internal task records (commit SHAs, line-number references, close reasons, developer decisions). It has no impact on the application but adds noise to the repo history. Additionally, line 24 contains an
in_progressissue (damus-g0f— "Squash vine-phase1 into 5 logical commits"), suggesting this was exported before work was complete.If this is intentionally version-controlled as a lightweight project management tool, consider adding
.beads/to the project's documentation so contributors understand its purpose. Otherwise, consider adding.beads/to.gitignore.AGENTS.md (1)
49-73: Consider documenting thebd synccommand.The new "Landing the Plane" workflow references
bd sync(Line 62) without explanation. Contributors (or agents) unfamiliar with this tool won't know what it does or how to install it. A brief inline note or link to documentation would help.damus/Features/Vines/Creation/VineComposerView.swift (1)
12-28: CustomEquatableonUploadPhaseis unnecessary.Swift auto-synthesizes
Equatablefor enums whose associated values are allEquatable. SinceStringisEquatable, you can remove the manual conformance:Proposed fix
- enum UploadPhase: Equatable { + enum UploadPhase: Equatable { case idle case uploading case uploaded case failed(String) - - static func == (lhs: UploadPhase, rhs: UploadPhase) -> Bool { - switch (lhs, rhs) { - case (.idle, .idle), (.uploading, .uploading), (.uploaded, .uploaded): - return true - case let (.failed(lhsMessage), .failed(rhsMessage)): - return lhsMessage == rhsMessage - default: - return false - } - } }damus/Features/Vines/Models/VineVideo.swift (2)
409-417: Redundant condition:key == "pm-report"is already matched bykey.hasPrefix("pm-").
"pm-report".hasPrefix("pm-")is always true, so the third condition is dead code.Suggested simplification
- if key == "proof" || key.hasPrefix("pm-") || key == "pm-report" { + if key == "proof" || key.hasPrefix("pm-") {
267-278:NSDataDetectoris re-created on everyVineVideoinit — consider caching or documenting the cost.
NSDataDetector(types:)compilation is not free. SincecollectContentURLsis called frominit?for every event, consider making the detector astatic letto avoid repeated allocation. As per coding guidelines, docstring coverage should be ensured for added code.Proposed optimization
+ private static let linkDetector: NSDataDetector? = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue) + private static func collectContentURLs(from content: String?, into candidates: inout [URL: MediaCandidate]) { guard let content, !content.isEmpty else { return } - guard let detector = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue) else { return } + guard let detector = linkDetector else { return } let range = NSRange(content.startIndex..<content.endIndex, in: content)damus/Features/Settings/Models/UserSettingsStore.swift (1)
115-148: Missing docstrings for new vine-related settings.The three new
@Settingproperties and thevines_feature_enabledcomputed property lack documentation. Other settings in this file (e.g.,enable_experimental_local_relay_modelat line 278) follow the pattern of having///docstrings. As per coding guidelines, docstring coverage should be ensured for any added code.damusTests/NostrEventTests.swift (2)
219-222:makeVineEventhelper is duplicated across both test classes.
VineVideoTests.makeVineEvent(line 219) andVineFeedModelTests.makeVineEvent(line 308) are identical. Extract into a shared file-level helper or a common extension to avoid drift.Proposed consolidation
+// MARK: - Shared Test Helpers + +private func makeVineEvent(content: String = "", tags: [[String]], createdAt: UInt32 = UInt32(Date().timeIntervalSince1970)) -> NostrEvent { + let keypair = generate_new_keypair().to_keypair() + return NostrEvent(content: content, keypair: keypair, kind: NostrKind.vine_short.rawValue, tags: tags, createdAt: createdAt)! +} final class VineVideoTests: XCTestCase { - // MARK: - Helpers - private func makeVineEvent(...) -> NostrEvent { ... } } final class VineFeedModelTests: XCTestCase { - // MARK: - Helpers - private func makeVineEvent(...) -> NostrEvent { ... } }Also applies to: 308-311
354-366:applyPage(non-reset) always keeps existing entries over incoming duplicates, unlikeapplywhich replaces with newer.In
apply, a newer event replaces an older one with the samededupeKey. InapplyPage(reset: false), existing entries always win regardless of timestamp. This asymmetry appears intentional for cursor-based pagination (older pages appended later), but worth documenting in theVineTestFeeddocstring to prevent future confusion.damus/Features/Vines/Views/VineFullScreenPager.swift (1)
17-22: Missing docstring oninit.As per coding guidelines, added code should have docstring coverage. The initializer lacks documentation explaining its parameters (especially
initialIndexsemantics andonCloselifecycle).damus/Features/Vines/Views/VineCard.swift (2)
208-219:formatCountdisplays "1000.0K" for values near 999,950.Values in the range [999,950, 999,999] will round to "1000.0K" due to
%.1fformatting, which looks odd. Consider switching to integer formatting or handling this boundary:Suggested fix
private func formatCount(_ value: Int) -> String { let number = Double(value) let thousand = number / 1_000 let million = number / 1_000_000 if million >= 1.0 { - return String(format: "%.1fM", million) + return million >= 10 ? String(format: "%.0fM", million) : String(format: "%.1fM", million) } else if thousand >= 1.0 { - return String(format: "%.1fK", thousand) + return thousand >= 100 ? String(format: "%.0fK", thousand) : String(format: "%.1fK", thousand) } else { return "\(value)" } }
10-11: Missing docstrings on private members.The struct-level docstring is present, but per coding guidelines, added/modified code should have docstring coverage. Several private computed properties and methods (
header,videoBody,metadataRows,shouldBlurContent,authorDisplayName,formatCount,reportVine) lack documentation.damus/Features/Vines/Views/VineTimelineView.swift (2)
61-62: Rapid tab switching causes connect/disconnect churn on the Divine relay.
onAppearcallssubscribe()(which connects the relay) andonDisappearcallsstop(disconnect: true)(which disconnects). If the user rapidly switches tabs, this triggers repeated relay connection cycles. Consider debouncing the disconnect or only disconnecting after a brief delay.Suggested approach
-.onDisappear { model.stop(disconnect: true) } +.onDisappear { + // Delay disconnect to avoid churn on rapid tab switches + model.stop(disconnect: false) + Task { `@MainActor` in + try? await Task.sleep(nanoseconds: 2_000_000_000) + // Only disconnect if still not subscribed + if model.streamTask == nil { + await model.disconnectManagedRelayIfNeeded() + } + } +}This is a rough sketch — the exact approach depends on exposing the right API from
VineFeedModel.
76-91: Missing docstring oninfoBanner.Per coding guidelines, added code should have docstring coverage.
damus/Shared/Media/Images/ImageProcessing.swift (1)
165-212: Timeout logic withwithTaskGroupis well-structured, but has a subtle concern on cancellation draining.The async export with a 30-second timeout via
withTaskGroupis a solid replacement for the oldDispatchSemaphore. One consideration:After
group.next()returns,withTaskGroupwon't return to the caller until all child tasks complete (the group drains). If the timeout fires first andexportSession.cancelExport()is called, the export task'swithCheckedContinuationblocks until AVAssetExportSession fires its completion handler in response to the cancel. In practice this should be near-instant, but if the export session hangs, the function will be stuck despite the "timeout."If robustness against a hung export session matters, consider adding an additional escape hatch (e.g., resuming the continuation with
falseafter a secondary delay), though in practiceAVAssetExportSession.cancelExport()is reliable.damus/Features/Vines/Models/VineFeedModel.swift (1)
17-18:isLoadingandrelayMessagelackprivate(set)unlikevines.
vineson Line 16 correctly uses@Published private(set), butisLoadingandrelayMessageare fully writable from outside. For consistency and encapsulation, restrict external mutation:Suggested fix
- `@Published` var isLoading: Bool = false - `@Published` var relayMessage: String? = nil + `@Published` private(set) var isLoading: Bool = false + `@Published` private(set) var relayMessage: String? = nil
| { | ||
| "database": "beads.db", | ||
| "jsonl_export": "issues.jsonl" | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if beads.db exists in repo
echo "=== Checking if beads.db exists in repo ==="
fd 'beads.db' 2>/dev/null || echo "beads.db not found"
echo ""
echo "=== Checking .gitignore for beads patterns ==="
if [ -f .gitignore ]; then
rg -n 'beads' .gitignore || echo "No beads pattern found in .gitignore"
else
echo ".gitignore file not found"
fi
echo ""
echo "=== Full .gitignore content ==="
cat .gitignore 2>/dev/null || echo ".gitignore not accessible"Repository: damus-io/damus
Length of output: 357
Add beads.db to .gitignore.
The .beads/metadata.json file references beads.db, which is a SQLite database that should not be committed to version control. While beads.db doesn't currently exist in the repository, it's not excluded in .gitignore. Add a pattern to prevent accidental commits if the beads tool generates this database during development.
🤖 Prompt for AI Agents
In @.beads/metadata.json around lines 1 - 4, Add a gitignore entry to prevent
committing the SQLite DB referenced in .beads/metadata.json by updating the
repository .gitignore to ignore beads.db (and preferably any database files
under the .beads directory); add either "beads.db" and/or ".beads/beads.db" (or
a broader pattern like ".beads/*.db") to .gitignore, save, and commit the change
so the generated SQLite file cannot be accidentally checked in.
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | ||
| try? await pool.add_relay(descriptor) | ||
| await pool.connect(to: [relayURL]) | ||
| await MainActor.run { featureManagedRelays.insert(relayURL) } |
There was a problem hiding this comment.
Silently swallowing add_relay errors may leave inconsistent state.
try? await pool.add_relay(descriptor) discards the error. If a TOCTOU race causes RelayAlreadyExists, the method still proceeds to connect and marks the relay as feature-managed without confirming it was actually added. Consider handling the error or at least guarding the subsequent lines:
Proposed fix
let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite)
- try? await pool.add_relay(descriptor)
- await pool.connect(to: [relayURL])
- await MainActor.run { featureManagedRelays.insert(relayURL) }
+ do {
+ try await pool.add_relay(descriptor)
+ await pool.connect(to: [relayURL])
+ await MainActor.run { featureManagedRelays.insert(relayURL) }
+ } catch {
+ Log.warning("Failed to add feature relay %s: %s", for: .networking, relayURL.absoluteString, error.localizedDescription)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | |
| try? await pool.add_relay(descriptor) | |
| await pool.connect(to: [relayURL]) | |
| await MainActor.run { featureManagedRelays.insert(relayURL) } | |
| let descriptor = RelayPool.RelayDescriptor(url: relayURL, info: .readWrite) | |
| do { | |
| try await pool.add_relay(descriptor) | |
| await pool.connect(to: [relayURL]) | |
| await MainActor.run { featureManagedRelays.insert(relayURL) } | |
| } catch { | |
| Log.warning("Failed to add feature relay %s: %s", for: .networking, relayURL.absoluteString, error.localizedDescription) | |
| } |
🤖 Prompt for AI Agents
In `@damus/Core/Networking/NostrNetworkManager/NostrNetworkManager.swift` around
lines 283 - 286, The code silently swallows errors from pool.add_relay which can
leave state inconsistent; change the try? await pool.add_relay(descriptor) to
explicit error handling: call await pool.add_relay(descriptor) inside do/catch,
handle the specific RelayAlreadyExists error by treating it as success (so you
can still proceed to connect), rethrow or log and abort for other errors, and
only insert relayURL into featureManagedRelays after add_relay either succeeded
or you confirmed the relay already exists; keep pool.connect(to:) execution
gated on successful add/confirmed-existence. Use the symbols
RelayPool.RelayDescriptor, pool.add_relay(...), pool.connect(to:), and
featureManagedRelays.insert(relayURL) to locate and update the logic.
| struct VineComposerView: View { | ||
| enum UploadPhase: Equatable { | ||
| case idle | ||
| case uploading | ||
| case uploaded | ||
| case failed(String) | ||
|
|
||
| static func == (lhs: UploadPhase, rhs: UploadPhase) -> Bool { | ||
| switch (lhs, rhs) { | ||
| case (.idle, .idle), (.uploading, .uploading), (.uploaded, .uploaded): | ||
| return true | ||
| case let (.failed(lhsMessage), .failed(rhsMessage)): | ||
| return lhsMessage == rhsMessage | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Environment(\.dismiss) private var dismiss | ||
|
|
||
| let damus_state: DamusState | ||
|
|
||
| @State private var showingMediaPicker = false | ||
| @State private var selectedUpload: MediaUpload? | ||
| @State private var mediaDescriptor: VineMediaDescriptor? | ||
| @State private var uploadPhase: UploadPhase = .idle | ||
| @State private var showingCamera = false | ||
|
|
||
| @State private var vineIdentifier: String = "" | ||
| @State private var titleText: String = "" | ||
| @State private var captionText: String = "" | ||
| @State private var summaryText: String = "" | ||
| @State private var hashtagsInput: String = "" | ||
| @State private var contentWarning: String = "" | ||
| @State private var altText: String = "" | ||
| @State private var originSource: String = "" | ||
| @State private var originIdentifier: String = "" | ||
| @State private var originDetail: String = "" | ||
| @State private var referenceURL: String = "" | ||
|
|
||
| @State private var isPublishing = false | ||
|
|
||
| private let uploadService = VineBlossomUploadService() | ||
|
|
||
| var body: some View { | ||
| NavigationStack { | ||
| Form { | ||
| clipSection | ||
| metadataSection | ||
| advancedSection | ||
| } | ||
| .navigationTitle(NSLocalizedString("New Vine", comment: "Navigation title for the Vine composer view.")) | ||
| .navigationBarTitleDisplayMode(.inline) | ||
| .toolbar { | ||
| ToolbarItem(placement: .cancellationAction) { | ||
| Button(NSLocalizedString("Cancel", comment: "Button title to dismiss the Vine composer without publishing.")) { | ||
| dismiss() | ||
| } | ||
| } | ||
| ToolbarItem(placement: .confirmationAction) { | ||
| Button(NSLocalizedString("Publish", comment: "Button title to publish the new Vine event.")) { | ||
| publishVine() | ||
| } | ||
| .disabled(!canPublish) | ||
| } | ||
| } | ||
| .sheet(isPresented: $showingMediaPicker) { | ||
| MediaPicker( | ||
| mediaPickerEntry: .postView, | ||
| onMediaSelected: nil, | ||
| onMediaPicked: { media in | ||
| handlePickedMedia(media) | ||
| } | ||
| ) | ||
| } | ||
| .sheet(isPresented: $showingCamera) { | ||
| CameraController( | ||
| uploader: damus_state.settings.default_media_uploader, | ||
| imagesOnly: false, | ||
| mode: .handle_video { url in | ||
| showingCamera = false | ||
| handlePickedMedia(.processed_video(url)) | ||
| } | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private var clipSection: some View { | ||
| Section(NSLocalizedString("Clip", comment: "Section title for the selected Vine clip.")) { | ||
| if let descriptor = mediaDescriptor, | ||
| let url = descriptor.sources.first?.url { | ||
| VStack(alignment: .leading, spacing: 6) { | ||
| Text(url.lastPathComponent) | ||
| .font(.headline) | ||
| if let duration = descriptor.duration { | ||
| Text(String(format: NSLocalizedString("Duration: %.1fs", comment: "Label describing the video duration."), duration)) | ||
| .font(.caption) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| if let size = descriptor.dimensions { | ||
| Text("\(Int(size.width))×\(Int(size.height))") | ||
| .font(.caption2) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| } | ||
| } else { | ||
| Text(NSLocalizedString("Attach a vertical clip (mp4/m3u8).", comment: "Placeholder text before a Vine clip is selected.")) | ||
| .foregroundColor(.secondary) | ||
| } | ||
|
|
||
| Button { | ||
| showingMediaPicker = true | ||
| } label: { | ||
| Label(NSLocalizedString("Choose Video", comment: "Button to open the media picker for Vine clips."), systemImage: "film") | ||
| } | ||
|
|
||
| Button { | ||
| showingCamera = true | ||
| } label: { | ||
| Label(NSLocalizedString("Record Video", comment: "Button to open the Vine camera recorder."), systemImage: "video.fill") | ||
| } | ||
| .disabled(isUploadingVideo) | ||
|
|
||
| switch uploadPhase { | ||
| case .idle: | ||
| EmptyView() | ||
| case .uploading: | ||
| HStack(spacing: 8) { | ||
| ProgressView() | ||
| Text(NSLocalizedString("Uploading to Blossom…", comment: "Status label shown while uploading Vine video to Blossom.")) | ||
| } | ||
| case .uploaded: | ||
| Label(NSLocalizedString("Upload complete.", comment: "Status label shown when Vine upload finishes successfully."), systemImage: "checkmark.circle") | ||
| .foregroundColor(.green) | ||
| case .failed(let message): | ||
| Label(message, systemImage: "exclamationmark.triangle") | ||
| .foregroundColor(.red) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private var metadataSection: some View { | ||
| Section(NSLocalizedString("Details", comment: "Section title for Vine metadata fields.")) { | ||
| TextField(NSLocalizedString("Title", comment: "Placeholder for Vine title field."), text: $titleText) | ||
| TextField(NSLocalizedString("Caption", comment: "Placeholder for Vine caption field."), text: $captionText, prompt: Text(NSLocalizedString("Describe your Vine…", comment: "Prompt for Vine caption field."))) | ||
| TextField(NSLocalizedString("Summary (optional)", comment: "Placeholder for Vine summary field."), text: $summaryText) | ||
| TextField(NSLocalizedString("Hashtags (comma separated)", comment: "Placeholder for Vine hashtag field."), text: $hashtagsInput) | ||
| } | ||
| } | ||
|
|
||
| private var advancedSection: some View { | ||
| Section(NSLocalizedString("Advanced", comment: "Section title for optional Vine metadata fields.")) { | ||
| TextField(NSLocalizedString("Identifier (optional)", comment: "Placeholder for Vine identifier/d-tag field."), text: $vineIdentifier) | ||
| TextField(NSLocalizedString("Content warning (optional)", comment: "Placeholder for the Vine content warning field."), text: $contentWarning) | ||
| TextField(NSLocalizedString("Alt text (optional)", comment: "Placeholder for Vine alternative text field."), text: $altText) | ||
| TextField(NSLocalizedString("Origin source", comment: "Placeholder for Vine origin source field."), text: $originSource) | ||
| TextField(NSLocalizedString("Origin identifier", comment: "Placeholder for Vine origin identifier field."), text: $originIdentifier) | ||
| TextField(NSLocalizedString("Origin detail", comment: "Placeholder for Vine origin detail field."), text: $originDetail) | ||
| TextField(NSLocalizedString("Reference link", comment: "Placeholder for Vine reference link field."), text: $referenceURL) | ||
| .keyboardType(.URL) | ||
| .autocapitalization(.none) | ||
| .disableAutocorrection(true) | ||
| } | ||
| } | ||
|
|
||
| private var canPublish: Bool { | ||
| guard damus_state.keypair.privkey != nil else { return false } | ||
| guard mediaDescriptor != nil else { return false } | ||
| guard case .uploaded = uploadPhase else { return false } | ||
| return !titleText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty && !isPublishing | ||
| } | ||
|
|
||
| private var isUploadingVideo: Bool { | ||
| if case .uploading = uploadPhase { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private func handlePickedMedia(_ media: PreUploadedMedia) { | ||
| Task { | ||
| guard var upload = await generateMediaUpload(media) else { | ||
| await MainActor.run { | ||
| uploadPhase = .failed(NSLocalizedString("Unable to process media.", comment: "Error shown when Vine composer cannot process selected media.")) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| guard case .video(let localURL) = upload else { | ||
| await MainActor.run { | ||
| uploadPhase = .failed(NSLocalizedString("Please select a video clip.", comment: "Error shown when user selects a non-video media for Vine composer.")) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if let convertedURL = await convertVideoToMP4IfNeeded(localURL: localURL) { | ||
| upload = .video(convertedURL) | ||
| } else if localURL.pathExtension.lowercased() != "mp4" { | ||
| await MainActor.run { | ||
| uploadPhase = .failed(NSLocalizedString("Unable to convert clip to MP4.", comment: "Error shown when Vine composer cannot transcode to MP4 for upload.")) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| await MainActor.run { | ||
| selectedUpload = upload | ||
| } | ||
| await uploadSelectedMedia(upload) | ||
| } | ||
| } | ||
|
|
||
| @MainActor | ||
| private func uploadSelectedMedia(_ media: MediaUpload) async { | ||
| guard let keypair = damus_state.keypair.privkey != nil ? damus_state.keypair : nil else { | ||
| uploadPhase = .failed(NSLocalizedString("A signing key is required to upload.", comment: "Error shown when trying to upload a Vine without a private key.")) | ||
| return | ||
| } | ||
| uploadPhase = .uploading | ||
| mediaDescriptor = nil | ||
|
|
||
| Task.detached { | ||
| // Extract metadata on background thread to avoid blocking main thread | ||
| let metadata = self.videoMetadata(for: media.localURL) | ||
|
|
||
| do { | ||
| let response = try await uploadService.uploadVideo( | ||
| fileURL: media.localURL, | ||
| mimeType: media.mime_type, | ||
| keypair: keypair | ||
| ) | ||
| let descriptor = self.makeDescriptor(from: response, mimeType: media.mime_type, videoMetadata: metadata) | ||
| await MainActor.run { | ||
| self.mediaDescriptor = descriptor | ||
| if self.vineIdentifier.isEmpty { | ||
| self.vineIdentifier = response.videoID | ||
| } | ||
| self.uploadPhase = .uploaded | ||
| } | ||
| } catch { | ||
| await MainActor.run { | ||
| if let vineError = error as? VineBlossomUploadError { | ||
| self.uploadPhase = .failed(vineError.localizedDescription) | ||
| } else { | ||
| self.uploadPhase = .failed(error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func videoMetadata(for url: URL) -> (duration: TimeInterval?, dimensions: CGSize?) { | ||
| let asset = AVURLAsset(url: url) | ||
| let durationSeconds = CMTimeGetSeconds(asset.duration) | ||
| var dimensions: CGSize? | ||
| if let track = asset.tracks(withMediaType: .video).first { | ||
| let size = track.naturalSize.applying(track.preferredTransform) | ||
| dimensions = CGSize(width: abs(size.width), height: abs(size.height)) | ||
| } | ||
| return (durationSeconds.isFinite ? durationSeconds : nil, dimensions) | ||
| } | ||
|
|
||
| private func convertVideoToMP4IfNeeded(localURL: URL) async -> URL? { | ||
| return await withCheckedContinuation { continuation in | ||
| let asset = AVAsset(url: localURL) | ||
| guard let exporter = AVAssetExportSession(asset: asset, presetName: AVAssetExportPresetHighestQuality) else { | ||
| continuation.resume(returning: localURL.pathExtension.lowercased() == "mp4" ? localURL : nil) | ||
| return | ||
| } | ||
|
|
||
| let destinationURL = generateUniqueTemporaryMediaURL(fileExtension: "mp4") | ||
| exporter.outputURL = destinationURL | ||
| exporter.outputFileType = .mp4 | ||
| exporter.shouldOptimizeForNetworkUse = true | ||
| exporter.metadataItemFilter = AVMetadataItemFilter.forSharing() | ||
| exporter.exportAsynchronously { | ||
| switch exporter.status { | ||
| case .completed: | ||
| continuation.resume(returning: destinationURL) | ||
| default: | ||
| continuation.resume(returning: nil) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func makeDescriptor(from response: VineBlossomUploadResponse, mimeType: String, videoMetadata: (duration: TimeInterval?, dimensions: CGSize?)) -> VineMediaDescriptor { | ||
| var sources: [VineMediaDescriptor.Source] = [] | ||
| func append(_ url: URL?, kind: VineMediaDescriptor.Source.Kind) { | ||
| guard let url else { return } | ||
| sources.append(.init(url: url, kind: kind)) | ||
| } | ||
| append(response.primaryURL, kind: sourceKind(for: response.primaryURL)) | ||
| append(response.streamingMP4URL, kind: .mp4) | ||
| append(response.streamingHLSURL, kind: .hls) | ||
| append(response.fallbackURL, kind: .fallback) | ||
| return VineMediaDescriptor( | ||
| sources: sources, | ||
| mimeType: mimeType, | ||
| thumbnailURL: response.thumbnailURL, | ||
| blurhash: nil, | ||
| dimensions: videoMetadata.dimensions, | ||
| duration: videoMetadata.duration, | ||
| fileSize: nil, | ||
| sha256: response.videoID | ||
| ) | ||
| } | ||
|
|
||
| private func sourceKind(for url: URL) -> VineMediaDescriptor.Source.Kind { | ||
| switch url.pathExtension.lowercased() { | ||
| case "m3u8": | ||
| return .hls | ||
| case "mp4": | ||
| return .mp4 | ||
| default: | ||
| return .legacy | ||
| } | ||
| } | ||
|
|
||
| private func publishVine() { | ||
| guard let descriptor = mediaDescriptor else { return } | ||
| let metadata = VineDraftMetadata( | ||
| identifier: vineIdentifier.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty ? nil : vineIdentifier, | ||
| title: titleText.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| caption: captionText.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| summary: summaryText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty ? nil : summaryText, | ||
| hashtags: parsedHashtags(), | ||
| publishedAt: Date(), | ||
| contentWarning: contentWarning.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty ? nil : contentWarning, | ||
| altText: altText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty ? nil : altText, | ||
| origin: originDescriptor(), | ||
| references: parsedReferences(), | ||
| participants: [], | ||
| expiration: nil, | ||
| extraTags: [] | ||
| ) | ||
|
|
||
| let builder = VineEventBuilder(metadata: metadata, media: descriptor) | ||
| guard let post = builder.makePost() else { | ||
| uploadPhase = .failed(NSLocalizedString("Failed to build Vine event.", comment: "Error shown when Vine builder fails to produce an event.")) | ||
| return | ||
| } | ||
|
|
||
| isPublishing = true | ||
| notify(.post(.post(post))) | ||
| dismiss() | ||
| } | ||
|
|
||
| private func parsedHashtags() -> [String] { | ||
| let characterSet = CharacterSet(charactersIn: ",") | ||
| return hashtagsInput | ||
| .components(separatedBy: characterSet.union(.whitespacesAndNewlines)) | ||
| .map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } | ||
| .filter { !$0.isEmpty } | ||
| } | ||
|
|
||
| private func parsedReferences() -> [URL] { | ||
| guard let url = URL(string: referenceURL.trimmingCharacters(in: .whitespacesAndNewlines)), | ||
| !referenceURL.isEmpty else { | ||
| return [] | ||
| } | ||
| return [url] | ||
| } | ||
|
|
||
| private func originDescriptor() -> VineOriginDescriptor? { | ||
| let trimmedSource = originSource.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !trimmedSource.isEmpty else { return nil } | ||
| let identifier = originIdentifier.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let detail = originDetail.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return VineOriginDescriptor( | ||
| source: trimmedSource, | ||
| identifier: identifier.isEmpty ? nil : identifier, | ||
| detail: detail.isEmpty ? nil : detail | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing docstrings on the entire new file.
The coding guidelines require "Ensure docstring coverage for any code added or modified." This new file has no docstrings on the struct, its computed properties, or any of its methods. At minimum, add a brief docstring to VineComposerView, publishVine(), handlePickedMedia(_:), uploadSelectedMedia(_:), and convertVideoToMP4IfNeeded(localURL:).
As per coding guidelines, **/*.swift: "Ensure docstring coverage for any code added or modified."
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Creation/VineComposerView.swift` around lines 11 - 387,
The file lacks required docstrings; add concise Swift documentation comments
(/// ...) to the VineComposerView type and the specified methods: publishVine(),
handlePickedMedia(_:), uploadSelectedMedia(_:), and
convertVideoToMP4IfNeeded(localURL:) describing their purpose, parameters (where
applicable), and return behavior; place the docstrings immediately above the
struct declaration and each method/ computed property (e.g., above private func
publishVine(), private func handlePickedMedia(_:), `@MainActor` private func
uploadSelectedMedia(_:), and private func convertVideoToMP4IfNeeded(localURL:)
-> URL?) following the existing comment style used elsewhere so the new file
meets the docstring coverage guideline.
| .autocapitalization(.none) | ||
| .disableAutocorrection(true) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SwiftUI autocapitalization deprecated iOS 15 textInputAutocapitalization replacement
💡 Result:
In SwiftUI, TextField/TextEditor’s .autocapitalization(_:) (which takes a UITextAutocapitalizationType) is deprecated as of iOS 15—use .textInputAutocapitalization(_:) instead. [1]
TextField("Email", text: $email)
.textInputAutocapitalization(.never) // iOS 15+TextInputAutocapitalization values are: .sentences (default), .words, .characters, and .never. [2]
For TextEditor on iOS 15+:
TextEditor(text: $comment)
.textInputAutocapitalization(.never)[3]
Sources: [1] [2] [3]
🏁 Script executed:
cat -n damus/Features/Vines/Creation/VineComposerView.swift | sed -n '165,180p'Repository: damus-io/damus
Length of output: 1462
Replace deprecated .autocapitalization(.none) with .textInputAutocapitalization(.never).
The autocapitalization(_:) modifier was deprecated in iOS 15. Use .textInputAutocapitalization(.never) instead.
Proposed fix
.keyboardType(.URL)
- .autocapitalization(.none)
+ .textInputAutocapitalization(.never)
.disableAutocorrection(true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .autocapitalization(.none) | |
| .disableAutocorrection(true) | |
| .textInputAutocapitalization(.never) | |
| .disableAutocorrection(true) |
🤖 Prompt for AI Agents
In `@damus/Features/Vines/Creation/VineComposerView.swift` around lines 173 - 174,
In VineComposerView, replace the deprecated Text modifier call
`.autocapitalization(.none)` with the iOS 15+ API
`.textInputAutocapitalization(.never)` where the view chain currently also uses
`.disableAutocorrection(true)`; locate the modifier chain on the text input in
VineComposerView and swap the autocapitalization modifier name and value to
`.textInputAutocapitalization(.never)` while leaving
`.disableAutocorrection(true)` intact.
- Parse each imeta tag as a separate media variant (was flattened) - Weight url and fallback URLs equally per spec - Remove non-spec video source extraction (standalone url, streaming, r/e/i tags) - Add segment, text-track, and bitrate/service parsing - Content URL fallback now only matches video file extensions - Remove video resolution from card UI, fix duration display - Space out EventActionBar icons in VineCard Signed-off-by: alltheseas <alltheseas@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@damus/Features/Vines/Views/VineCard.swift`:
- Around line 204-215: The formatCount(_:) function can produce "1000.0K" for
values just under 1,000,000; change the logic in formatCount to detect when the
thousand-based rounding would hit 1000.0K and instead return "1.0M". For
example, before returning the "%.1fK" result in formatCount, compute the
rounded-to-one-decimal thousand value (or test thousand >= 999.95) and if that
rounded value >= 1000.0, return the million-format string (e.g., "1.0M");
otherwise fall back to the existing K or raw value branches.
🧹 Nitpick comments (5)
damus/Features/Vines/Models/VineVideo.swift (3)
366-374: Redundant"pm-report"check in proof tag filter.
key == "pm-report"is already covered bykey.hasPrefix("pm-"). The extra check is harmless but unnecessary.♻️ Suggested simplification
- if key == "proof" || key.hasPrefix("pm-") || key == "pm-report" { + if key == "proof" || key.hasPrefix("pm-") {
390-402: Missing docstrings onintTagValueandtagValuehelpers.Per coding guidelines, all added code should have docstring coverage. These two helpers lack documentation.
📝 Suggested docstrings
+ /// Returns the integer value of the first tag matching `key`, or `nil` if absent or non-numeric. private static func intTagValue(_ key: String, in event: NostrEvent) -> Int? { guard let value = tagValue(key, in: event) else { return nil } return Int(value) } + /// Returns the first value associated with a tag whose key matches `key`, or `nil` if not found. private static func tagValue(_ key: String, in event: NostrEvent) -> String? {As per coding guidelines: "Ensure docstring coverage for any code added or modified".
39-60:VineOriginandVineProoflack doc comments.
VideoSegmentandTextTrack(lines 62–74) have doc comments, butVineOriginandVineProofdo not. As per coding guidelines: "Ensure docstring coverage for any code added or modified".damusTests/NostrEventTests.swift (2)
219-222:makeVineEventhelper is duplicated across both test classes.The identical helper exists in both
VineVideoTests(line 219) andVineFeedModelTests(line 308). Consider extracting it to a shared extension or free function in the test target.♻️ Extract shared helper
Add a shared helper at file scope or in a test utilities file:
/// Creates a minimal Vine event for testing. private func makeVineEvent(content: String = "", tags: [[String]], createdAt: UInt32 = UInt32(Date().timeIntervalSince1970)) -> NostrEvent { let keypair = generate_new_keypair().to_keypair() return NostrEvent(content: content, keypair: keypair, kind: NostrKind.vine_short.rawValue, tags: tags, createdAt: createdAt)! }Then remove the duplicate from both classes.
Also applies to: 308-311
225-228: Test doubles mirror production logic — consider testing production code directly.
VineTestFeedandPrefetchGateduplicate core algorithms fromVineFeedModelrather than exercising the production code paths. If the production deduplication or prefetch gating logic diverges, these tests won't catch regressions. The docstrings acknowledge this tradeoff. When feasible, consider refactoringVineFeedModelto expose testable units that can be exercised without requiringDamusState.Also applies to: 314-327, 329-366
- Add .beads/*.db to .gitignore to prevent committing SQLite files - Replace try? with explicit do/catch in ensureRelayConnected to avoid silent failures and inconsistent state - Add docstrings to VineComposerView and key methods - Replace deprecated .autocapitalization(.none) with .textInputAutocapitalization(.never) - Note: collectFallbackURLs was already removed in NIP-71 refactor Signed-off-by: alltheseas <alltheseas@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Closing in favor of consolidated Linux-style topic branches. This work will be audited against AGENTS.md requirements and cherry-picked into ordered topic branches. See #3624 for tracking. |
|
Closing this as #3619 was closed. |
3 participants
Summary
Adds a Vine short-video viewer to Damus as an experimental Labs feature. Users can browse archived Vine videos served over Nostr (NIP-71 kind 34236) from the Divine relay, with full-screen playback, prefetch, pagination, and reporting.
Commits
eef7731dAdd Vine video support: relay, parser, feed, and tab — Recognize vine_short Nostr events, implement VineFeedModel with relay subscription, parse VineVideo from event tags, handle reposts, add dedicated Vine tab79e08a66Add Vine UI: full-screen pager, prefetch, pagination, and reporting — Full-screen vertical pager, network-aware video prefetch, cursor-based pagination, EventActionBar, reporting menu, Labs gatec53a7656Harden Vine: fix data races, strip GPS metadata, transcode imports — Replace locks with actors, fix async blocking, strip GPS from video attachments, task cancellation/timeout, fixture tests11851dbcFix Vine bugs: relay ownership, threading, and test isolation — Relay ownership tracking, move state mutation out of SwiftUI body, video metadata off main thread, actor isolation fixes204438f9Refactor Vine into separate files and address review feedback — Extract types from PostingTimelineView, @unchecked Sendable, @mainactor over NSLock, remove vine relay from bootstrap, docstrings, accessibility, unit tests6861eb9aAlign VineVideo parser with NIP-71 spec — Per-variant imeta parsing, equal url/fallback weighting, remove non-spec tag extraction, add segment/text-track/bitrate supportKey design decisions
imetatag parsed as a separate media variant;urlandfallbackweighted equally per spec;segment,text-track, andbitratesupportedensureRelayConnectedwhen the Vine tab opens; removed from bootstrap list so non-Vine users are unaffected@unchecked Sendable(documented justification), NostrNetworkManager uses@MainActorisolation instead of NSLockTODO
Checklist
Experimental Feature Checklist
Changelog-Added: Added experimental feature <X> to Damus LabsTest plan
damusTests— all VineVideo and VineFeedModel tests pass