diff --git a/Flipcash/Core/Controllers/ConversationController.swift b/Flipcash/Core/Controllers/ConversationController.swift index e3d320973..3b475809f 100644 --- a/Flipcash/Core/Controllers/ConversationController.swift +++ b/Flipcash/Core/Controllers/ConversationController.swift @@ -2121,6 +2121,13 @@ final class ConversationController { assertion.begin() defer { assertion.end() } + // A chip that failed retryably (offline, say) is sendable; tapping Send is its retry. + for chip in chips { + if case .failed(.retryable) = chip.state { + chip.retryUpload() + } + } + let sends = ChatMediaSendPlan.mediaMessages(chips: chips, caption: caption, replyTo: repliedTo).map { message in (message: message, clientMessageID: insertPendingMedia(message, into: conversationID)) } diff --git a/Flipcash/Core/Screens/Conversation/ChatMediaUploader.swift b/Flipcash/Core/Screens/Conversation/ChatMediaUploader.swift index f0770f1db..704b9ce21 100644 --- a/Flipcash/Core/Screens/Conversation/ChatMediaUploader.swift +++ b/Flipcash/Core/Screens/Conversation/ChatMediaUploader.swift @@ -104,8 +104,9 @@ enum ChatMediaUploadError: Error { case .noMatchingConstraint, .encryptionNotAllowed, .encodingFailed: false case .sealUnavailable(let error): - // A peer key that couldn't be fetched may arrive; one shared-core refuses never will. - error is PeerKeyUnavailable + // A peer key that couldn't be fetched may arrive, as may a chat lost to the network + // (transport failures classify as suppressed); one shared-core refuses never will. + error is PeerKeyUnavailable || (error as? ServerError)?.reportingLevel == .suppressed case .rejected(let reason): switch reason { case .moderation: @@ -125,7 +126,7 @@ extension ChatMediaUploadError: ServerError { case .noMatchingConstraint, .encryptionNotAllowed, .encodingFailed: .error case .sealUnavailable(let error): - error is PeerKeyUnavailable ? .info : .error + error is PeerKeyUnavailable ? .info : (error as? ServerError)?.reportingLevel ?? .error case .rejected: .info case .failed(let error): diff --git a/Flipcash/Core/Screens/Conversation/ComposerModel.swift b/Flipcash/Core/Screens/Conversation/ComposerModel.swift index a9494076a..98dfcd3df 100644 --- a/Flipcash/Core/Screens/Conversation/ComposerModel.swift +++ b/Flipcash/Core/Screens/Conversation/ComposerModel.swift @@ -72,8 +72,9 @@ final class ComposerModel { } } - /// Whether the send button can fire: text or at least one chip, and no chip that failed. Chips - /// still uploading do not hold the send back, and an edit is text only. + /// Whether the send button can fire: text or at least one chip, and no chip that failed for + /// good. Chips still uploading or that failed retryably do not hold the send back, and an edit + /// is text only. var canSubmit: Bool { switch mode { case .new, .replying: @@ -104,12 +105,13 @@ final class ComposerModel { } } - /// Whether any staged chip failed to upload, which holds the send until it is retried or removed. + /// Whether any staged chip failed in a way another upload can't fix, which holds the send + /// until it is removed. var hasFailedChip: Bool { chips.contains { chip in switch chip.state { - case .failed: true - case .preparing, .uploading, .uploaded: false + case .failed(.notRetryable): true + case .failed(.retryable), .preparing, .uploading, .uploaded: false } } } diff --git a/FlipcashCore/Sources/FlipcashCore/Blob/BlobUploader.swift b/FlipcashCore/Sources/FlipcashCore/Blob/BlobUploader.swift index 9ad6431a7..ebe18c541 100644 --- a/FlipcashCore/Sources/FlipcashCore/Blob/BlobUploader.swift +++ b/FlipcashCore/Sources/FlipcashCore/Blob/BlobUploader.swift @@ -137,14 +137,19 @@ final class BlobUploader: Sendable { /// The MIME type every end-to-end encrypted upload declares; the server refuses any other. static let encryptedMimeType = "application/octet-stream" + /// How many finalization polls may be lost in transit back to back before the wait gives up. + static let maxConsecutiveLostPolls = 3 + /// Polls until the blob is finalized. /// /// Returns immediately when it is already ready; a rejection is terminal. A poll lost in transit - /// is retried, since iOS drops the connection when it suspends the app mid-wait. The `timeout` - /// budget is counted in polls rather than wall time, so time spent suspended doesn't use it up. + /// is retried, since iOS drops the connection when it suspends the app mid-wait, but + /// ``maxConsecutiveLostPolls`` in a row throw `ErrorBlob.network`. The `timeout` budget is + /// counted in polls rather than wall time, so time spent suspended doesn't use it up. func awaitFinalization(blobID: BlobID, owner: KeyPair) async throws { let maxPolls = max(1, Int((timeout / pollInterval).rounded(.up))) var polls = 0 + var lostInARow = 0 while true { try Task.checkCancellation() @@ -161,14 +166,21 @@ final class BlobUploader: Sendable { ]) throw ErrorBlob.rejected(reason) case .pending, .processing: - break + lostInARow = 0 } } catch ErrorBlob.network(let error) { try Task.checkCancellation() + lostInARow += 1 logger.info("Blob poll lost in transit", metadata: [ "blobId": "\(blobID)", "error": "\(error)", + "lostInARow": "\(lostInARow)", ]) + // Offline, each poll waits out its RPC deadline, so the poll budget alone would + // hold the send for minutes. + if lostInARow >= Self.maxConsecutiveLostPolls { + throw ErrorBlob.network(error) + } } guard polls < maxPolls else { diff --git a/FlipcashCore/Sources/FlipcashCore/Models/Chat/ChatMessage.swift b/FlipcashCore/Sources/FlipcashCore/Models/Chat/ChatMessage.swift index f4d8b8cd6..12a2efb71 100644 --- a/FlipcashCore/Sources/FlipcashCore/Models/Chat/ChatMessage.swift +++ b/FlipcashCore/Sources/FlipcashCore/Models/Chat/ChatMessage.swift @@ -180,6 +180,16 @@ public struct ChatMessage: Hashable, Sendable, Codable, Identifiable { } } + /// Whether a double tap on this row's bubble brings up the reaction strip on its own. Cash and + /// profile cards and bare link cards keep their instant single tap instead. + public var takesDoubleTapReaction: Bool { + guard offersReactionStrip, !rendersAsBareLinkCard else { return false } + switch content { + case .text, .media: return true + case .cash, .deleted, .unavailable, .shareProfile: return false + } + } + public init( id: String, serverID: MessageID? = nil, diff --git a/FlipcashCore/Tests/FlipcashCoreTests/BlobUploaderTests.swift b/FlipcashCore/Tests/FlipcashCoreTests/BlobUploaderTests.swift index a69223731..5a62fd0a7 100644 --- a/FlipcashCore/Tests/FlipcashCoreTests/BlobUploaderTests.swift +++ b/FlipcashCore/Tests/FlipcashCoreTests/BlobUploaderTests.swift @@ -396,7 +396,8 @@ struct BlobUploaderTests { @Test("Polls lost in transit still count toward the deadline") func lostPollsHonourTheDeadline() async throws { - let reserving = StubReserving(states: [], lostPolls: .max) + // Never three in a row, so only the overall budget ends the wait. + let reserving = StubReserving(states: [], lostAt: [1, 2, 4, 5]) let uploader = BlobUploader( reserving: reserving, transport: RecordingTransport(), @@ -413,6 +414,42 @@ struct BlobUploaderTests { #expect(await reserving.pollCount == 5) } + /// Offline, every poll waits out its RPC deadline, so the overall budget would hold the row at + /// Sending for minutes. + @Test("Three polls lost in a row give up as a network failure, well inside the budget") + func consecutiveLostPollsFailEarly() async throws { + let reserving = StubReserving(states: [], lostPolls: .max) + let uploader = BlobUploader( + reserving: reserving, + transport: RecordingTransport(), + pollInterval: .milliseconds(1), + timeout: .seconds(5) + ) + + await #expect { + try await uploader.awaitFinalization(blobID: StubReserving.blobID, owner: try Self.owner()) + } throws: { error in + guard case ErrorBlob.network = error else { return false } + return true + } + #expect(await reserving.pollCount == 3) + } + + @Test("A poll that gets through starts the lost-poll count again") + func answeredPollResetsLostStreak() async throws { + let reserving = StubReserving(states: [.processing, .processing, .ready], lostAt: [1, 2, 4, 5]) + let uploader = BlobUploader( + reserving: reserving, + transport: RecordingTransport(), + pollInterval: .milliseconds(1), + timeout: .seconds(5) + ) + + try await uploader.awaitFinalization(blobID: StubReserving.blobID, owner: try Self.owner()) + + #expect(await reserving.pollCount == 7) + } + // MARK: - Fixtures - private static let fileBytes = Data(repeating: 0xAB, count: 4096) @@ -573,17 +610,20 @@ private actor StubReserving: BlobReserving { private var states: [BlobState] private let completion: BlobState private var lostPolls: Int + private let lostAt: Set private(set) var pollCount = 0 private(set) var reserveCount = 0 private(set) var declaredSizeBytes: Int? private(set) var declaredMimeType: String? private(set) var encryptedFor: ConversationID? - /// `lostPolls` is how many polls fail in transit before the stub starts answering. - init(states: [BlobState], completion: BlobState = .processing, lostPolls: Int = 0) { + /// `lostPolls` is how many polls fail in transit before the stub starts answering; `lostAt` + /// names further polls, counted from 1, that fail in transit. + init(states: [BlobState], completion: BlobState = .processing, lostPolls: Int = 0, lostAt: Set = []) { self.states = states self.completion = completion self.lostPolls = lostPolls + self.lostAt = lostAt } func initiateExternalUpload(mimeType: String, sizeBytes: Int, encryptedFor: ConversationID?, owner: KeyPair) async throws -> ReservedUpload { @@ -614,8 +654,8 @@ private actor StubReserving: BlobReserving { func blobState(blobID: BlobID, owner: KeyPair) async throws -> BlobState { pollCount += 1 - if lostPolls > 0 { - lostPolls -= 1 + if lostPolls > 0 || lostAt.contains(pollCount) { + lostPolls = max(0, lostPolls - 1) throw ErrorBlob.network(URLError(.networkConnectionLost)) } return states.isEmpty ? .processing : states.removeFirst() diff --git a/FlipcashTests/Chat/ChatMediaUploaderTests.swift b/FlipcashTests/Chat/ChatMediaUploaderTests.swift index ddff56d7c..aa5f1813a 100644 --- a/FlipcashTests/Chat/ChatMediaUploaderTests.swift +++ b/FlipcashTests/Chat/ChatMediaUploaderTests.swift @@ -211,6 +211,30 @@ struct ChatMediaUploaderTests { #expect(blob.storeAttempts == 0) } + @Test("A chat that couldn't be fetched offline leaves the photo retryable, and reports nothing") + func offlineChatFetchIsRetryable() async { + let blob = MockChatMediaBlobStore() + var uploader = ChatMediaUploader(blob: blob) + uploader.seal = { throw ErrorGetChat.transportFailure } + + let error = await Self.uploadError(uploader) + + #expect(error?.isRetryable == true) + #expect(error?.isTerminal == false) + #expect(error?.reportingLevel == .suppressed) + #expect(blob.storeAttempts == 0) + } + + @Test("A chat the server refuses to return leaves the photo unretryable") + func deniedChatFetchIsNotRetryable() async { + var uploader = ChatMediaUploader(blob: MockChatMediaBlobStore()) + uploader.seal = { throw ErrorGetChat.denied } + + let error = await Self.uploadError(uploader) + + #expect(error?.isRetryable == false) + } + // MARK: - Gate - @Test("Camera and Photos show in an encrypted DM, a plaintext chat, and not before the chat loads") diff --git a/FlipcashTests/Chat/ChatPhotoStagingTests.swift b/FlipcashTests/Chat/ChatPhotoStagingTests.swift index 5ff29af90..0e5614ed3 100644 --- a/FlipcashTests/Chat/ChatPhotoStagingTests.swift +++ b/FlipcashTests/Chat/ChatPhotoStagingTests.swift @@ -305,12 +305,12 @@ struct ComposerOutgoingTests { } } - @Test("A failed chip holds the send") + @Test("A chip that failed for good holds the send") func failedChipHoldsSend() throws { let composer = ComposerModel() let chip = try #require(composer.stageChip(image: image(), uploader: uploader)) chip.uploadTask?.cancel() - chip.state = .failed(.retryable) + chip.state = .failed(.notRetryable) composer.draft = "caption" #expect(composer.outgoing == nil) diff --git a/FlipcashTests/Chat/ComposerChipTests.swift b/FlipcashTests/Chat/ComposerChipTests.swift index 3adb8f1b6..23dc50637 100644 --- a/FlipcashTests/Chat/ComposerChipTests.swift +++ b/FlipcashTests/Chat/ComposerChipTests.swift @@ -91,12 +91,12 @@ struct ComposerChipTests { #expect(composer.canSubmit) } - @Test("A chip that failed holds the send back until it is removed") - func failedChipDisablesSend() throws { + @Test("A chip that failed for good holds the send back until it is removed") + func unretryableChipDisablesSend() throws { let composer = ComposerModel() composer.draft = "look" let chip = try #require(composer.stageChip(image: testImage(), uploader: uploader)) - chip.state = .failed(.retryable) + chip.state = .failed(.notRetryable) #expect(!composer.canSubmit) @@ -104,6 +104,21 @@ struct ComposerChipTests { #expect(composer.canSubmit) } + @Test("A chip whose upload failed offline can still be sent") + func retryableChipStillSends() throws { + let composer = ComposerModel() + composer.draft = "look" + let chip = try #require(composer.stageChip(image: testImage(), uploader: uploader)) + chip.state = .failed(.retryable) + + #expect(composer.canSubmit) + guard case .media(let chips, caption: "look") = composer.outgoing else { + Issue.record("expected the chip to go out as media") + return + } + #expect(chips.map(\.id) == [chip.id]) + } + @Test("Clearing after a send empties both the text and the chips") func clearEmptiesChips() throws { let composer = ComposerModel() diff --git a/FlipcashTests/Chat/ConversationMediaSendTests.swift b/FlipcashTests/Chat/ConversationMediaSendTests.swift index ee5507031..3c68d8985 100644 --- a/FlipcashTests/Chat/ConversationMediaSendTests.swift +++ b/FlipcashTests/Chat/ConversationMediaSendTests.swift @@ -292,6 +292,41 @@ struct ConversationMediaSendTests { #expect(chip.progress.phase == .sent, "the retried upload brings the overlay back and ends it sent") } + @Test("Sending a photo whose upload already failed offline uploads it again and posts it") + func sendRestartsFailedUpload() async throws { + let mock = MockConversations() + let controller = makeController(mock) + let blob = MockChatMediaBlobStore() + blob.storeResults = [.failure(ErrorBlob.network(URLError(.notConnectedToInternet)))] + let chip = ComposerChip(image: testImage()) + chip.startUpload(using: ChatMediaUploader(blob: blob, backoff: [])) + _ = try? await chip.uploadTask?.value + #expect(chip.state == .failed(.retryable)) + + let sent = await controller.sendMedia([chip], caption: nil, to: conversationID) + + #expect(sent) + #expect(blob.storeAttempts == 2) + #expect(mock.sentMedia.map(\.blobID) == [MockChatMediaBlobStore.blobID]) + #expect(pendingMedia(controller).isEmpty) + } + + @Test("Sending a photo that failed for good does not upload it again") + func sendLeavesUnretryableFailure() async throws { + let mock = MockConversations() + let controller = makeController(mock) + let blob = MockChatMediaBlobStore() + blob.finalization = .failure(ErrorBlob.rejected(.moderation)) + let chip = ComposerChip(image: testImage()) + chip.startUpload(using: ChatMediaUploader(blob: blob, backoff: [])) + _ = try? await chip.uploadTask?.value + #expect(chip.state == .failed(.notRetryable)) + + #expect(!(await controller.sendMedia([chip], caption: nil, to: conversationID))) + #expect(blob.storeAttempts == 1) + #expect(pendingMedia(controller).map(\.status) == [.failed]) + } + @Test("A photo refused by moderation is not retried") func moderationIsNotRetried() async throws { let mock = MockConversations() diff --git a/FlipcashTests/Chat/ReactionUITests.swift b/FlipcashTests/Chat/ReactionUITests.swift index dd21fd467..4dea68688 100644 --- a/FlipcashTests/Chat/ReactionUITests.swift +++ b/FlipcashTests/Chat/ReactionUITests.swift @@ -65,6 +65,20 @@ struct ReactionUITests { #expect(message.offersReactionStrip == !isRedacted) } + @Test("Double-tapping a photo or a text bubble brings up the strip") + func photoAndTextTakeDoubleTap() { + let media = ChatMediaContent(blobID: nil, width: 100, height: 100, blurhash: nil, caption: nil, isRedacted: false) + #expect(Self.makeMessage(content: .media(media), sender: .other).takesDoubleTapReaction) + #expect(Self.makeMessage(content: .text("hi"), sender: .other).takesDoubleTapReaction) + } + + @Test("A redacted photo or a deleted message takes no double tap") + func ineligibleRowsTakeNoDoubleTap() { + let redacted = ChatMediaContent(blobID: nil, width: 100, height: 100, blurhash: nil, caption: nil, isRedacted: true) + #expect(!Self.makeMessage(content: .media(redacted), sender: .other).takesDoubleTapReaction) + #expect(!Self.makeMessage(content: .deleted("This message was deleted"), sender: .other).takesDoubleTapReaction) + } + @Test("The viewer's own message still in flight never offers the strip") func inFlightSendNoStrip() { let message = Self.makeMessage(content: .text("hi"), sender: .me, receipt: nil, isUnsent: true) diff --git a/FlipcashUI/Sources/FlipcashUI/Chat/ChatViewController.swift b/FlipcashUI/Sources/FlipcashUI/Chat/ChatViewController.swift index fb2a9ddfd..056c997d3 100644 --- a/FlipcashUI/Sources/FlipcashUI/Chat/ChatViewController.swift +++ b/FlipcashUI/Sources/FlipcashUI/Chat/ChatViewController.swift @@ -840,18 +840,14 @@ public final class ChatViewController: UICollectionViewController { } /// The message whose bubble is at `point` if a double tap there should present the reaction - /// strip. Only text bubbles take one: link cards, cash cards, quote panels and reaction pills - /// keep their instant single tap. + /// strip. Text and photo bubbles take one: link cards, cash cards, quote panels and reaction + /// pills keep their instant single tap. private func doubleTapTarget(at point: CGPoint) -> ChatMessage? { guard !isUpdating, !isShowingContextMenu, let indexPath = collectionView.indexPathForItem(at: point), let message = message(at: indexPath), - message.offersReactionStrip, !message.rendersAsBareLinkCard, + message.takesDoubleTapReaction, let cell = collectionView.cellForItem(at: indexPath) as? BubbleCarrying else { return nil } - switch message.content { - case .text: break - case .cash, .deleted, .unavailable, .shareProfile, .media: return nil - } let bubble = cell.liftPreviewView guard bubble.bounds.contains(bubble.convert(point, from: collectionView)) else { return nil } var view = collectionView.hitTest(point, with: nil) @@ -1366,6 +1362,19 @@ extension ChatViewController: UIGestureRecognizerDelegate { let exclusive: [UIGestureRecognizer?] = [swipeToReply.recognizer, bubbleLongPress] return !exclusive.contains { $0 === gestureRecognizer || $0 === otherGestureRecognizer } } + + /// Holds a photo's open-the-viewer tap until the double tap fails, so the second tap of a double + /// tap reacts instead of opening the photo. The double tap only receives touches on a photo that + /// can take a reaction, so any other photo still opens on a single tap with no wait. + public func gestureRecognizer(_ gestureRecognizer: UIGestureRecognizer, shouldBeRequiredToFailBy otherGestureRecognizer: UIGestureRecognizer) -> Bool { + guard gestureRecognizer === bubbleDoubleTap else { return false } + var view = otherGestureRecognizer.view + while let current = view { + if let cell = current as? ChatMediaCell { return cell.imageTap === otherGestureRecognizer } + view = current.superview + } + return false + } } // MARK: - Message menu