Harden image upload error handling and add retry logic #3490
Conversation
Replace generic "Error uploading image :(" with specific, user-actionable
error messages from the upload server. This fixes issue damus-io#3484 where users
couldn't understand why their uploads failed.
Changes:
- Add UploadError enum with typed error cases (serverError, networkError, etc.)
- Change MediaUploaderProtocol.getMediaURL to return Result<String, UploadError>
- Propagate typed errors through AttachMediaUtility and ImageUploadModel
- Display error.userMessage in PostView and EditPictureControl
- Update test mock to use new Result type
Now when a server rejects an upload (e.g., "file too large"), users see
the actual error message instead of a generic failure.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Closes damus-io#1286
Signed-off-by: alltheseas
Signed-off-by: alltheseas <[email protected]>
Fix error messages being truncated on small screens (issue damus-io#2198): - PostView: Add lineLimit(3), minimumScaleFactor(0.8), and caption font - EditPictureControl: Improve error sheet with title, centered text, and OK button 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Closes damus-io#2198 Signed-off-by: alltheseas Signed-off-by: alltheseas <[email protected]>
Surface media picker errors to the user instead of silently discarding selected images. When image processing fails (e.g., corrupted file, unsupported format), the error is now displayed to the user. Also fixes a race condition in failedCount and orderMap access when selecting multiple items by synchronizing access on the main queue. Signed-off-by: alltheseas Closes: damus-io#2198 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
When uploads fail due to network issues (timeout, connection lost, etc.), automatically retry with exponential backoff. This improves reliability on flaky mobile connections. Changes: - Add isRetryable property to UploadError for classifying transient errors - Add UploadRetryConfig struct for configuring retry behavior - Add executeWithRetry() with exponential backoff (1s, 2s delays) - Default: 2 retries for timeouts, connection losses, DNS failures - Add httpError case for HTTP status code errors - 4xx client errors (400, 401, 413) are NOT retried (permanent failures) - 5xx server errors are retried (transient failures) Signed-off-by: alltheseas Closes: damus-io#1286 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
iOS 18 introduced a regression in CGImageDestinationAddImageFromSource that causes crashes when processing HEIC images. This commit works around the issue by using CGImageSourceCreateImageAtIndex to decode the image first, then using CGImageDestinationAddImage. The workaround is only applied when the source format is HEIC/HEIF to minimize impact on other image formats. Signed-off-by: alltheseas Closes: damus-io#3484 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Add comprehensive testing infrastructure for upload retry logic: Unit Testing: - MockURLProtocol: General-purpose URLProtocol mock - RetryTestURLProtocol: Specialized mock for testing retry scenarios - UploadRetryTests: 21 unit tests covering retry behavior - Network error retries (timeout, connection lost, DNS failure) - HTTP 4xx errors are not retried (400, 413, etc.) - HTTP 5xx errors are retried with backoff - Exponential backoff timing - Max retry configuration UI Testing: - NetworkConditionSimulator: Simulates poor network conditions - Supports timeout, connection lost, DNS failure scenarios - Accessible via hidden debug gesture in PostView - DEV_TIPS.md documentation for testing workflows Signed-off-by: alltheseas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Add ImageProcessingTests with 6 tests verifying the iOS 18 workaround: - testProcessHEICImageDoesNotCrash: Core crash prevention test - testProcessHEICImageRemovesGPSData: GPS metadata stripping - testProcessHEICImagePreservesDimensions: Image dimensions preserved - testProcessMultiFrameHEICImage: Multi-frame HEIC handling - testProcessJPEGImageStillWorks: JPEG regression test - testProcessPNGImageStillWorks: PNG regression test Tests programmatically create HEIC/JPEG/PNG images using CGImageDestination so no test assets need to be bundled. Signed-off-by: alltheseas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Add TSan-enabled scheme and GitHub Actions workflow to detect data races: - damus-TSan.xcscheme: Scheme with threadSanitizerEnabled=YES - .github/workflows/tests.yml: CI workflow running unit tests and TSan - docs/DEV_TIPS.md: Documentation on running TSan locally Also fixes TestMediaUploader to properly parse error messages from server responses, matching the behavior of real MediaUploader. Signed-off-by: alltheseas 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Adds MediaPickerTests.swift with 6 tests verifying: - Error message formatting with item indices - Coordinator thread safety under concurrent updates - Correct failure count reporting via onError callback - Media delivery in selection order - Empty selection handling Also fixes ImageCacheMigrations.swift to gracefully handle test environments where app group containers are unavailable, preventing test host crashes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: alltheseas <[email protected]>
Address CodeRabbit review feedback: - cr-4/cr-6: Add NSLock synchronization to MockURLProtocol and RetryTestURLProtocol static properties to prevent data races when TSan is enabled - cr-7: Rename testUploadShowsErrorOnTimeout to testAppLaunchesWithTimeoutSimulation and testUploadSucceedsAfterRetry to testAppLaunchesWithRetrySimulation to accurately reflect test coverage (infrastructure scaffolding only) Signed-off-by: alltheseas <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@fishcakeday added a bunch of media upload hardening commits |
danieldaquino
added
the
pr-in-queue
📝 WalkthroughWalkthroughIntroduces network condition testing infrastructure, improves media upload error handling with retry logic and structured error types, adds WebSocket protocol abstraction for dependency injection, and delivers comprehensive test coverage across network scenarios, relay operations, and media uploads with new CI workflows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test
participant RelayConnection
participant MockWebSocket
participant RelayPool as Relay Pool
rect rgb(200, 220, 255)
Note over Test,RelayPool: Old Flow (Concrete WebSocket)
Test->>RelayConnection: init(url, handlers)
RelayConnection->>RelayConnection: create real WebSocket
end
rect rgb(200, 255, 220)
Note over Test,RelayPool: New Flow (Dependency Injection)
Test->>MockWebSocket: create mock instance
Test->>RelayConnection: init(url, webSocket: mock, handlers)
RelayConnection->>RelayConnection: use injected mock WebSocket
Test->>MockWebSocket: simulateConnect()
MockWebSocket->>RelayConnection: emit .connected event
RelayConnection->>RelayPool: broadcast connection state
RelayPool->>Test: notifies observers
Test->>MockWebSocket: simulateDisconnect()
MockWebSocket->>RelayConnection: emit .disconnected event
RelayConnection->>RelayPool: broadcast disconnection
RelayPool->>Test: notifies observers
end
sequenceDiagram
autonumber
participant User as User
participant PostView as Post Composer
participant MediaPicker
participant AttachMediaUtility as Media Uploader
participant RetryLogic as Retry Handler
participant UIFeedback as Error Display
User->>PostView: Attach media
PostView->>MediaPicker: open picker + onError handler
MediaPicker->>MediaPicker: user selects media
rect rgb(255, 240, 200)
Note over AttachMediaUtility,RetryLogic: Retry Loop (maxRetries)
MediaPicker->>AttachMediaUtility: upload with retryConfig
AttachMediaUtility->>RetryLogic: executeWithRetry()
RetryLogic->>RetryLogic: attempt upload
alt Success
RetryLogic->>RetryLogic: return URL
AttachMediaUtility->>MediaPicker: success
else Retryable Error (4xx/5xx)
RetryLogic->>RetryLogic: delay baseDelaySeconds × attempt
RetryLogic->>RetryLogic: retry
else Non-Retryable Error
RetryLogic->>AttachMediaUtility: return UploadError
end
end
AttachMediaUtility->>MediaPicker: emit result or error
alt All Media Processed
MediaPicker->>MediaPicker: failedCount > 0?
alt Has Failures
MediaPicker->>PostView: onError(failedCount)
PostView->>UIFeedback: display localized error
UIFeedback->>User: show formatted error message
else No Failures
MediaPicker->>PostView: onMediaSelected()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 9
🤖 Fix all issues with AI Agents
In @.github/workflows/tests.yml:
- Around line 10-33: Replace the current "Build and Test" step that runs the
inline xcodebuild pipeline with a single run: just test; specifically, in the
unit-tests job update the step whose name is "Build and Test" to use run: just
test (removing the multi-line xcodebuild invocation and its reliance on
xcpretty), and drop the separate "Select Xcode" step and any hardcoded
Xcode/simulator settings so the workflow uses the project's justfile (which
provides xcbeautify and the canonical simulator/Xcode configuration).
In @damus.xcodeproj/project.pbxproj:
- Line 1512: The PBXBuildFile entry with ID D73E5EFE2C6A97F4007EB227 is
malformed because it lacks the required fileRef and has a placeholder comment
"BuildFile in Sources"; locate the PBXBuildFile block for that ID in the
project.pbxproj and either remove the entire PBXBuildFile entry or replace it
with a valid PBXBuildFile that includes a fileRef pointing to the appropriate
PBXFileReference (e.g., the corresponding .swift/.m/.xcassets file ID), ensuring
the comment matches the referenced filename so Xcode can parse the project
successfully.
- Line 7244: Remove the malformed PBXBuildFile entry and its reference: delete
the PBXBuildFile definition with ID D73E5EFE2C6A97F4007EB227 and remove the
corresponding occurrence "D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */,"
from the Sources build phase so there is no dangling reference to that invalid
build file.
In @damus/AppAccessibilityIdentifiers.swift:
- Around line 67-74: The enum case post_composer_upload_progress in
AppAccessibilityIdentifiers is defined but unused; either remove that case from
AppAccessibilityIdentifiers or add it to the upload progress UI in PostView by
assigning the matching accessibility identifier to the progress control used
during media upload (e.g., set the ProgressView/UIActivityIndicator or custom
upload indicator’s accessibilityIdentifier to
AppAccessibilityIdentifiers.post_composer_upload_progress.rawValue where upload
progress is displayed/updated in PostView, near the existing uses of
post_composer_attach_media_button and post_composer_error_message).
In @damus/Shared/Utilities/NetworkConditionSimulator.swift:
- Around line 95-97: SimulatedNetworkProtocol.requestCount is incremented
unsafely in startLoading(), causing a data race when URLSession tasks call it
concurrently; protect increments and reads of the static requestCount (and any
logic that depends on it such as failThenSucceed) with synchronization — e.g.,
add a private serial DispatchQueue or lock inside SimulatedNetworkProtocol and
perform requestCount += 1 and the subsequent read under that lock, or replace
requestCount with an atomic-safe wrapper so that startLoading() uses the
synchronized increment/read to ensure correct counts for failThenSucceed logic.
In @damusTests/RelayPoolTests.swift:
- Around line 58-84: The test spawns an unawaited Task so assertions run after
the XCTest method returns; make testAddRelays synchronous from Swift concurrency
by removing the Task wrapper, marking the helper as async (func
testAddRelays(... ) async) and using try await directly when calling
relayPool.add_relay(descriptor); then update each caller test (e.g.,
testAddRelay_ValidRelayURL_NoErrors) to be async and call await
testAddRelays(...). Alternatively, if you must keep non-async tests, replace the
Task with an XCTestExpectation and fulfill it after the async work completes to
ensure assertions run before the test exits.
In @docs/DEV_TIPS.md:
- Line 65: Replace the bare URL "https://developer.apple.com/download/all/" on
the "Download \"Additional Tools for Xcode\"" line with a proper markdown link
by either wrapping it in angle brackets
(<https://developer.apple.com/download/all/>) or converting it to a descriptive
link like [Additional Tools for Xcode
downloads](https://developer.apple.com/download/all/) so the URL renders and is
accessible.
- Line 38: The description for the slowNetwork entry uses "3 second delay" —
update the wording to the hyphenated compound adjective "3-second delay" in the
`slowNetwork` bullet so it reads "`slowNetwork` - Adds 3-second delay before
responding".
In @scripts/throttle-network.sh:
- Around line 59-65: The error message in check_root() uses $* but the function
is called without arguments; either pass the script's original args into the
function or stop referencing them. Fix by updating the call site to pass the
script arguments (call check_root "$@") and keep the message using "$@" (change
the echo to use "sudo $0 $@"), or alternatively simplify the message to "sudo
$0" inside check_root; reference the check_root function and the call where it's
invoked and ensure you pass "$@" from the script startup (e.g., after set -e).
🧹 Nitpick comments (21)
damus/Shared/Media/ImageCacheMigrations.swift (3)
26-38: Consider adding an explanatory comment for the conditional migration path logic.The conditional logic that handles
migration1Doneand checks for app group availability is non-trivial. A brief comment explaining why both migrations are marked as done when the app group is unavailable (i.e., test environment with no cache to migrate) would improve maintainability.🔎 Suggested comment
// In test environments, app group may not be available - skip migration let oldCachePath: String if migration1Done { + // Migration1 was completed in a previous run. Try to get the path for migration2. + // If app group is now unavailable (e.g., running in test environment), + // skip migration2 by marking it done since there's no cache to migrate. guard let path = migration1KingfisherCachePath() else { // App group unavailable (e.g., test environment) - mark as done and skip defaults.set(true, forKey: migration1Key) defaults.set(true, forKey: migration2Key) return } oldCachePath = path } else { oldCachePath = migration0KingfisherCachePath() }
73-79: Add docstring per coding guidelines.The function has been modified and lacks documentation. As per coding guidelines, docstrings should be added for any code added or modified.
🔎 Suggested docstring
+/// Returns the Kingfisher cache path used in migration1, or nil if the app group is unavailable (e.g., in test environments). +/// +/// - Returns: The hardcoded path from migration1, or nil if the app group container is inaccessible. +/// - Note: The path and app group identifier are intentionally hardcoded to match the historical migration1 behavior. static private func migration1KingfisherCachePath() -> String? { // Implementation note: These are old, so they are hard-coded on purpose, because we can't change these values from the past. guard let groupURL = FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: "group.com.damus") else { return nil } return groupURL.appendingPathComponent("ImageCache").path }Based on coding guidelines, docstring coverage is required for modified code.
12-12: Add docstring for the modified migration function per coding guidelines.The
migrateKingfisherCacheIfNeeded()function has been significantly modified but lacks documentation. As per coding guidelines, docstrings should be provided for any code added or modified.🔎 Suggested docstring
+/// Migrates the Kingfisher image cache to the latest storage location if needed. +/// +/// This function performs two migrations: +/// - Migration 1: Moves cache from the default Kingfisher location to the app group shared container. +/// - Migration 2: Moves cache to the proper Library/Caches subdirectory within the app group. +/// +/// In test environments where the app group is unavailable, migrations are marked as complete and skipped. +/// The function is idempotent and safe to call multiple times. static func migrateKingfisherCacheIfNeeded() {Based on coding guidelines, docstring coverage is required for modified code.
docs/DEV_TIPS.md (1)
130-130: Optionally improve wording for conciseness.Line 130 uses "Not compatible" but could be more concise with "Incompatible."
🔎 Proposed improvement
-- Not compatible with Address Sanitizer (ASan) +- Incompatible with Address Sanitizer (ASan).beads/websocket-testing-approaches.md (1)
14-27: Optional: Add language specifiers to code blocks for better syntax highlighting.Several code blocks throughout the document lack language specifiers. While not critical for documentation, adding them improves readability in markdown viewers.
For example, lines 14-18 and 21-27 could specify
jsonortextas the language.damus/Shared/Media/Models/ImageUploadModel.swift (1)
116-125: Consider using Log instead of print for cancellation message.Line 121 uses
print()while the rest of the codebase usesLogfor consistent logging. Consider replacing with:- print("Upload cancelled by user, no feedback triggered.") + Log.debug("Upload cancelled by user, no feedback triggered.", for: .image_uploading)Also, the cast on line 118 (
as NSError?) is slightly redundant—underlying as? NSErrorwould be cleaner since you're already optional-chaining.Proposed refinement
- if case .networkError(let underlying) = error, - let nsError = underlying as NSError?, + if case .networkError(let underlying) = error, + let nsError = underlying as? NSError, nsError.domain == NSURLErrorDomain, nsError.code == NSURLErrorCancelled { - print("Upload cancelled by user, no feedback triggered.") + Log.debug("Upload cancelled by user, no feedback triggered.", for: .image_uploading) } else {damusTests/RelayIntegrationTests.swift (1)
81-107: Consider callingclose()instead of justdisconnect()on the test pool.The
testPoolis created for probing relay availability but onlydisconnect()is called. To fully clean up resources (handlers, request queues, etc.), consider usingclose().🔎 Suggested fix
do { try await testPool.add_relay(descriptor) await testPool.connect() // Wait for connection with multiple checks for _ in 0..<5 { try? await Task.sleep(for: .milliseconds(500)) if await testPool.num_connected == 1 { - await testPool.disconnect() + await testPool.close() return true } } - await testPool.disconnect() + await testPool.close() return false } catch { return false }damus/Shared/Media/Images/ImageProcessing.swift (1)
137-137: Consider usingprivateinstead offileprivate.Per SwiftLint,
privateis preferred when the function is only used within the same declaration scope. SinceremoveGPSDataFromImageis called only fromprocessImageandremoveGPSDataFromImageAndWritein this file,privatewould suffice and be slightly more restrictive.🔎 Suggested fix
-fileprivate func removeGPSDataFromImage(source: CGImageSource, url: URL) -> CGImageDestination? { +private func removeGPSDataFromImage(source: CGImageSource, url: URL) -> CGImageDestination? {damusTests/MediaPickerTests.swift (1)
140-144: Test manually duplicates production logic.Lines 140-144 manually invoke
dispatchGroup.notifyandonError, duplicating the production code pattern rather than exercising the actualCoordinator.recordFailure()method. This tests the callback mechanism but not the real error propagation path.Consider calling
coordinator.recordFailure()directly (if exposed) to test the actual production method, or document that this is an integration-style test of the callback mechanism itself.damusTests/RelayConnectionTests.swift (1)
200-231: Complex async test may be flaky.The nested
DispatchQueue.main.asyncAftercalls with multiple expectations in a loop (lines 204-227) could lead to timing-sensitive failures. Consider using a more deterministic approach withfulfillment(of:)or consolidating the async waits.The exponential backoff assertion (line 230:
8.0 = 1 * 2^3) is correct and well-documented..github/workflows/network-tests.yml (1)
97-106: Matrix job provides good coverage but overlaps with single-profile job.The matrix job runs for
3gandedgeprofiles on master push. Note that3gwill run twice on master pushes (once innetwork-tests, once inmatrix-network-tests). Consider excluding3gfrom the matrix or adjusting the trigger conditions to avoid duplicate runs.damusTests/ProfileNetworkTests.swift (1)
156-168: Test doesn't actually use Unicode characters.The test name suggests Unicode encoding verification, but the profile uses ASCII-only values (
"user_name","Test User"). Consider adding actual Unicode content to validate encoding:🔎 Suggested fix
func testProfileWithUnicodeCharacters() throws { let keypair = test_keypair_full - let profile = Profile(name: "user_name", display_name: "Test User") + let profile = Profile(name: "ユーザー", display_name: "用户 🎉") guard let event = make_metadata_event(keypair: keypair, metadata: profile) else { XCTFail("Failed to create metadata event") return } // Unicode should be properly encoded in JSON - XCTAssertTrue(event.content.contains("user_name")) + XCTAssertTrue(event.content.contains("ユーザー")) + XCTAssertTrue(event.content.contains("🎉")) }damus.xcodeproj/project.pbxproj (1)
4923-4923: Consider moving test utility to test-specific location.
NetworkConditionSimulator.swiftis placed in the mainUtilitiesgroup but appears to be test infrastructure. If it's only used for testing, consider moving it todamusTests/MockingalongsideMockURLProtocol.swiftandMockWebSocket.swiftto keep production code separate from test utilities.If it's intentionally included in production builds for debugging or diagnostics, ignore this suggestion.
damus/Shared/Media/Images/AttachMediaUtility.swift (1)
100-100: Remove trailing semicolon.SwiftLint flagged this line for having a trailing semicolon, which is not idiomatic Swift.
🔎 Proposed fix
- request.httpMethod = "POST"; + request.httpMethod = "POST"damus/Shared/Utilities/NetworkConditionSimulator.swift (4)
47-51: Remove redundant nil initialization.SwiftLint correctly notes that
var urlPattern: String? = nilis redundant since optionals default tonil.🔎 Proposed fix
- var urlPattern: String? = nil + var urlPattern: String?
77-79: Remove redundant nil initialization for static property.Same as above—
static var urlPattern: String? = nilcan omit the= nil.🔎 Proposed fix
- static var urlPattern: String? = nil + static var urlPattern: String?
144-149: Force unwrap onrequest.urlcould crash.While
request.urlbeingnilis unlikely in practice, the force unwrap on line 145 (request.url!) and the force unwrap on theHTTPURLResponseinitializer (line 149) could crash in edge cases.🔎 Proposed fix with guard
case .serverError: // Return a 500 server error + guard let url = request.url else { + let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorBadURL, userInfo: nil) + client?.urlProtocol(self, didFailWithError: error) + return + } let response = HTTPURLResponse( - url: request.url!, + url: url, statusCode: 500, httpVersion: "HTTP/1.1", headerFields: ["Content-Type": "application/json"] - )! + ) + guard let response else { + let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorBadServerResponse, userInfo: nil) + client?.urlProtocol(self, didFailWithError: error) + return + }
150-152: Use non-optionalDatainitializer.SwiftLint suggests using
Data(...)instead of"...".data(using:)!when the string is known to be valid UTF-8.🔎 Proposed fix
- let errorData = """ - {"status": "error", "message": "Simulated server error"} - """.data(using: .utf8)! + let errorData = Data(""" + {"status": "error", "message": "Simulated server error"} + """.utf8)damus/Shared/Media/Models/MediaUploader.swift (1)
239-242: Consider a more accurate error case for unexpected status values.When the server returns an unexpected status (neither "success" nor "error"), returning
.missingURLis semantically misleading. The URL isn't missing—the status is unexpected.🔎 Suggested approach
Consider adding a dedicated case like
unexpectedStatus(String)or using.serverError(message:)with a descriptive message:} else { Log.error("Unexpected response status: %{public}@", for: .image_uploading, status) - return .failure(.missingURL) + return .failure(.serverError(message: "Unexpected server response status: \(status)")) }This provides a more accurate user message when the server behaves unexpectedly.
damusTests/EventNetworkTests.swift (2)
170-202: Consider extracting shared test setup to reduce duplication.The setUp/tearDown pattern is duplicated across 6 test classes (ReactionNetworkTests, BoostNetworkTests, FollowNetworkTests, MuteListNetworkTests, BookmarkListNetworkTests, UserLookupNetworkTests). While acceptable for test clarity, a shared base class or helper could reduce maintenance overhead.
🔎 Possible approach
Create a base class with common setup:
class NetworkTestCase: XCTestCase { var pool: RelayPool! var postbox: PostBox! var mockSocket: MockWebSocket! var ndb: Ndb! let testRelayURL = RelayURL("wss://test.relay.com")! override func setUp() async throws { try await super.setUp() ndb = Ndb.test pool = RelayPool(ndb: ndb) mockSocket = MockWebSocket() let descriptor = RelayPool.RelayDescriptor(url: testRelayURL, info: .readWrite) try await pool.add_relay(descriptor, webSocket: mockSocket) postbox = PostBox(pool: pool) await pool.connect() mockSocket.simulateConnect() try await Task.sleep(for: .milliseconds(100)) } override func tearDown() async throws { await pool.close() pool = nil postbox = nil mockSocket = nil ndb = nil try await super.tearDown() } func simulateOKResponse(eventId: NoteId, success: Bool = true) { let result = CommandResult(event_id: eventId, ok: success, msg: "") postbox.handle_event(relay_id: testRelayURL, .nostr_event(.ok(result))) } }Then test classes extend it:
final class ReactionNetworkTests: NetworkTestCase { // Only reaction-specific helpers and tests }
1195-1199: Test only verifies Ndb existence, not actual caching behavior.This test asserts that
ndbis not nil, but doesn't verify that profiles are actually cached. Consider either removing this test or expanding it to insert and retrieve a profile from Ndb.🔎 Possible improvement
/// Test: Profile metadata cached in Ndb func testProfileCachedInNdb() async throws { let profile = Profile(name: "cachetest") guard let event = make_metadata_event(keypair: test_keypair_full, metadata: profile) else { XCTFail("Failed to create metadata event") return } // Process the event through Ndb try ndb.process(event: event) // Verify it can be retrieved let cached = ndb.lookup_profile(pubkey: test_keypair.pubkey) XCTAssertNotNil(cached, "Profile should be cached in Ndb") }This would actually test the caching integration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
.beads/.gitignore.beads/README.md.beads/beads-backup/.gitignore.beads/beads-backup/README.md.beads/beads-backup/config.yaml.beads/beads-backup/interactions.jsonl.beads/beads-backup/issues.jsonl.beads/beads-backup/metadata.json.beads/config.yaml.beads/interactions.jsonl.beads/issues.jsonl.beads/metadata.json.beads/websocket-testing-approaches.md.github/workflows/network-tests.yml.github/workflows/tests.ymldamus.xcodeproj/project.pbxprojdamus.xcodeproj/xcshareddata/xcschemes/damus-TSan.xcschemedamus/AppAccessibilityIdentifiers.swiftdamus/Core/Nostr/RelayConnection.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Core/Nostr/WebSocket.swiftdamus/Features/Posting/Views/PostView.swiftdamus/Features/Profile/Views/EditPictureControl.swiftdamus/Shared/Media/ImageCacheMigrations.swiftdamus/Shared/Media/Images/AttachMediaUtility.swiftdamus/Shared/Media/Images/ImageProcessing.swiftdamus/Shared/Media/Models/ImageUploadModel.swiftdamus/Shared/Media/Models/MediaPicker.swiftdamus/Shared/Media/Models/MediaUploader.swiftdamus/Shared/Utilities/NetworkConditionSimulator.swiftdamus/damusApp.swiftdamusTests/DirectMessageTests.swiftdamusTests/EditPictureControlTests.swiftdamusTests/EventHolderTests.swiftdamusTests/EventNetworkTests.swiftdamusTests/ImageProcessingTests.swiftdamusTests/MediaPickerTests.swiftdamusTests/Mocking/MockURLProtocol.swiftdamusTests/Mocking/MockWebSocket.swiftdamusTests/PostBoxTests.swiftdamusTests/ProfileNetworkTests.swiftdamusTests/RelayConnectionTests.swiftdamusTests/RelayIntegrationTests.swiftdamusTests/RelayPoolTests.swiftdamusTests/SearchNetworkTests.swiftdamusTests/UploadRetryTests.swiftdamusTests/ZapNetworkTests.swiftdamusUITests/damusUITests.swiftdocs/DEV_TIPS.mdscripts/throttle-network.shtest/strfry-test.conf
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Ensure docstring coverage for any code added, or modified
Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements
Files:
damus/AppAccessibilityIdentifiers.swiftdamusTests/EditPictureControlTests.swiftdamusTests/EventHolderTests.swiftdamusTests/RelayIntegrationTests.swiftdamus/Core/Nostr/RelayConnection.swiftdamus/Shared/Utilities/NetworkConditionSimulator.swiftdamusTests/UploadRetryTests.swiftdamusTests/RelayConnectionTests.swiftdamus/Shared/Media/ImageCacheMigrations.swiftdamusTests/MediaPickerTests.swiftdamusTests/ProfileNetworkTests.swiftdamusTests/ZapNetworkTests.swiftdamus/Shared/Media/Models/MediaPicker.swiftdamusTests/PostBoxTests.swiftdamusUITests/damusUITests.swiftdamusTests/Mocking/MockURLProtocol.swiftdamusTests/ImageProcessingTests.swiftdamus/damusApp.swiftdamusTests/RelayPoolTests.swiftdamusTests/EventNetworkTests.swiftdamus/Features/Posting/Views/PostView.swiftdamusTests/DirectMessageTests.swiftdamusTests/Mocking/MockWebSocket.swiftdamus/Core/Nostr/WebSocket.swiftdamus/Shared/Media/Images/ImageProcessing.swiftdamus/Shared/Media/Images/AttachMediaUtility.swiftdamus/Shared/Media/Models/MediaUploader.swiftdamusTests/SearchNetworkTests.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Shared/Media/Models/ImageUploadModel.swiftdamus/Features/Profile/Views/EditPictureControl.swift
🧠 Learnings (7)
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Add or update unit tests in damusTests/ alongside feature changes, especially when touching parsing, storage, or replay logic
Applied to files:
damusTests/RelayIntegrationTests.swift.beads/websocket-testing-approaches.mddamusTests/ZapNetworkTests.swiftdamusTests/PostBoxTests.swiftdamusUITests/damusUITests.swift.github/workflows/tests.ymldamusTests/EventNetworkTests.swiftdamusTests/SearchNetworkTests.swift
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Ensure docstring coverage for any code added, or modified
Applied to files:
docs/DEV_TIPS.md
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Review and follow pull_request_template.md when creating PRs for iOS Damus
Applied to files:
docs/DEV_TIPS.md
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Ensure new targets or resources integrate cleanly with the damus.xcodeproj main scheme
Applied to files:
damus.xcodeproj/xcshareddata/xcschemes/damus-TSan.xcscheme
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Applied to files:
damus.xcodeproj/project.pbxprojdamusTests/EventNetworkTests.swift
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to damusTests/__Snapshots__/** : Regenerate snapshot fixtures under damusTests/__Snapshots__ deliberately and explain updates in commit messages
Applied to files:
.github/workflows/tests.yml
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Use just build and just test for simulator builds and the primary test suite (requires xcbeautify); update or add just recipes if new repeatable workflows emerge
Applied to files:
.github/workflows/tests.yml
🧬 Code graph analysis (16)
damusTests/EditPictureControlTests.swift (1)
damus/Shared/Media/Models/MediaUploader.swift (2)
getMediaURL(211-247)mediaTypeValue(152-159)
damusTests/EventHolderTests.swift (1)
damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)
damus/Core/Nostr/RelayConnection.swift (2)
damus/Core/Nostr/WebSocket.swift (1)
disconnect(87-102)damusTests/Mocking/MockWebSocket.swift (1)
disconnect(88-94)
damusTests/RelayConnectionTests.swift (1)
damus/Core/Nostr/RelayConnection.swift (5)
connect(102-124)disconnect(126-132)send_raw(138-140)ping(82-100)disablePermanently(134-136)
damusTests/MediaPickerTests.swift (1)
damus/Shared/Media/Models/MediaPicker.swift (1)
picker(62-195)
damusTests/ProfileNetworkTests.swift (2)
nostrdb/src/bindings/rust/ndb_profile.rs (9)
name(68-73)display_name(103-108)about(82-87)picture(117-122)banner(96-101)website(75-80)lud06(145-150)lud16(89-94)nip05(124-129)damus/Core/Nostr/Nostr.swift (1)
make_test_profile(323-325)
damusTests/ZapNetworkTests.swift (4)
damusTests/ProfileNetworkTests.swift (4)
setUp(23-37)tearDown(39-46)simulateOKResponse(49-53)testOnFlushCallbackFires(262-283)damus/Shared/Utilities/Log.swift (1)
info(58-60)damusTests/Mocking/MockWebSocket.swift (2)
simulateConnect(117-119)simulateDisconnect(125-127)nostrdb/NdbNote.swift (1)
last_refid(551-553)
damus/Shared/Media/Models/MediaPicker.swift (2)
damus/Shared/Utilities/Log.swift (3)
debug(62-64)info(58-60)error(66-68)damus/Shared/Media/Images/ImageProcessing.swift (2)
generateUniqueTemporaryMediaURL(60-66)processVideo(43-45)
damusTests/PostBoxTests.swift (2)
damusTests/Mocking/MockWebSocket.swift (1)
simulateConnect(117-119)damus/Features/Posting/Models/PostBox.swift (1)
cancel_send(74-89)
damus/damusApp.swift (1)
damus/Shared/Utilities/NetworkConditionSimulator.swift (1)
configureFromLaunchArguments(36-62)
damusTests/EventNetworkTests.swift (6)
damus/Shared/Utilities/Log.swift (1)
info(58-60)damus/Core/Nostr/NostrEvent.swift (2)
make_like_event(491-513)make_boost_event(472-489)damus/Features/Chat/Models/ThreadModel.swift (2)
contains(270-272)get(280-282)nostrdb/NdbTagElem.swift (1)
id(128-131)damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)nostrdb/NdbNote.swift (1)
direct_replies(538-540)
damusTests/Mocking/MockWebSocket.swift (2)
damus/Core/Nostr/RelayConnection.swift (4)
connect(102-124)disconnect(126-132)send(142-156)ping(82-100)damus/Core/Nostr/WebSocket.swift (4)
connect(83-85)disconnect(87-102)send(104-110)ping(79-81)
damus/Shared/Media/Images/AttachMediaUtility.swift (3)
damus/Features/Profile/Views/EditPictureControl.swift (1)
from(745-748)damus/Shared/Media/Models/MediaUploader.swift (1)
getMediaURL(211-247)damusTests/EditPictureControlTests.swift (2)
getMediaURL(340-342)from(366-369)
damus/Shared/Media/Models/MediaUploader.swift (1)
damusTests/EditPictureControlTests.swift (2)
getMediaURL(340-342)from(366-369)
damusTests/SearchNetworkTests.swift (2)
damus/Features/Search/Models/SearchModel.swift (3)
tag_is_hashtag(94-97)event_matches_hashtag(85-92)event_matches_filter(99-104)damus/Shared/Utilities/EventHolder.swift (2)
set_should_queue(19-21)flush(82-104)
damus/Shared/Media/Models/ImageUploadModel.swift (1)
damus/Shared/Media/Images/AttachMediaUtility.swift (1)
create_upload_request(86-143)
🪛 LanguageTool
docs/DEV_TIPS.md
[grammar] ~38-~38: Use a hyphen to join words.
Context: ...rnet connection - slowNetwork - Adds 3 second delay before responding - `failTh...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~119-~119: The official name of this software platform is spelled with a capital “H”.
Context: ...gration The GitHub Actions workflow at .github/workflows/tests.yml runs TSan on every...
(GITHUB)
[style] ~130-~130: Consider using “incompatible” to avoid wordiness.
Context: ... - TSan adds ~5-10x runtime overhead - Not compatible with Address Sanitizer (ASan) - Some sy...
(NOT_ABLE_PREMIUM)
.beads/websocket-testing-approaches.md
[uncategorized] ~627-~627: The official name of this software platform is spelled with a capital “H”.
Context: .../TestRelay/| New directory | +500 | |.github/workflows/tests.yml` | Add relay servic...
(GITHUB)
[grammar] ~657-~657: Ensure spelling is correct
Context: ...ivity testing requires: - Latency: 100-500ms round trip - Bandwidth limits: 780 Kbps...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~917-~917: The operating system from Apple is written “macOS”.
Context: ...itespeedio/throttle 2. GitHub Actions macos runners: → Yes, sudo works. Implement...
(MAC_OS)
🪛 markdownlint-cli2 (0.18.1)
docs/DEV_TIPS.md
65-65: Bare URL used
(MD034, no-bare-urls)
.beads/websocket-testing-approaches.md
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
287-287: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
298-298: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
345-345: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
353-353: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
394-394: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
610-610: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
618-618: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
624-624: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
728-728: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Shellcheck (0.11.0)
scripts/throttle-network.sh
[warning] 59-65: check_root references arguments, but none are ever passed.
(SC2120)
🪛 SwiftLint (0.57.0)
damus/Shared/Utilities/NetworkConditionSimulator.swift
[Warning] 150-150: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 47-47: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 78-78: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
damus/Shared/Media/Images/ImageProcessing.swift
[Warning] 137-137: Prefer private over fileprivate declarations
(private_over_fileprivate)
damus/Shared/Media/Images/AttachMediaUtility.swift
[Warning] 100-100: Lines should not have trailing semicolons
(trailing_semicolon)
| unit-tests: | ||
| name: Unit Tests | ||
| runs-on: macos-14 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Select Xcode | ||
| run: sudo xcode-select -s /Applications/Xcode_15.2.app | ||
|
|
||
| - name: Build and Test | ||
| run: | | ||
| xcodebuild test \ | ||
| -scheme damus \ | ||
| -destination 'platform=iOS Simulator,name=iPhone 15,OS=17.2' \ | ||
| -resultBundlePath TestResults.xcresult \ | ||
| CODE_SIGNING_ALLOWED=NO \ | ||
| | xcpretty --color | ||
| - name: Upload Test Results | ||
| uses: actions/upload-artifact@v4 | ||
| if: always() | ||
| with: | ||
| name: test-results | ||
| path: TestResults.xcresult |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for justfile in the repository
echo "=== Checking for justfile ==="
git ls-files | grep -i justfile
if [ -f "justfile" ] || [ -f "Justfile" ]; then
echo "Found justfile, showing content:"
head -50 justfile Justfile 2>/dev/null
else
echo "No justfile found in repo"
fiRepository: damus-io/damus
Length of output: 404
🌐 Web query:
GitHub Actions macos-14 runner pre-installed software xcpretty xcode 15.2
💡 Result:
Short answer: Yes — the macos-14 GitHub Actions hosted runners include xcpretty (installed as a Ruby gem) and Xcode 15.2 is provided on the macOS runners (Xcode versions are listed/managed in the runner-images repo). [1][2][3]
Sources:
- actions/runner-images (lists macOS images, software policy and Xcode availability). [1]
- Issue noting default Xcode update to 15.2 across macOS runners. [2]
- xcpretty project (installation as a Ruby gem; commonly invoked on CI). [3]
🌐 Web query:
Xcode 15.2 iOS 17.2 simulator compatibility
💡 Result:
Short answer: Xcode 15.2 includes the iOS 17.2 SDK and supports running the iOS 17.2 simulator. For simulator runtimes newer than 17.2 you must upgrade Xcode (or install a matching simulator runtime if available). [1][2][3]
Notes/actions:
- Xcode 15.2 ships with the iOS 17.2 SDK and simulator support. [1][2]
- To add other simulator runtimes, use Xcode → Settings → Components (Simulators) to download runtimes on demand. If you need an iOS version newer than 17.2, update Xcode to a version that includes that runtime. [3]
Use just test command instead of inline xcodebuild.
The project has a justfile with established build and test recipes. Per project conventions, this workflow should execute just test instead of the hardcoded xcodebuild command. The justfile uses xcbeautify for output formatting and runs against iPhone 16e with OS 26.0, providing a consistent, maintainable testing approach.
Current issues:
- Workflow bypasses the canonical test recipe defined in
justfile - Uses
xcprettywhile the project's recipe usesxcbeautify(inconsistent formatting tool) - Hardcoded Xcode path and simulator configuration diverges from project standards
Update the workflow step to:
- name: Build and Test
run: just testThis aligns with the project's established testing patterns and simplifies CI configuration.
🤖 Prompt for AI Agents
In @.github/workflows/tests.yml around lines 10 - 33, Replace the current "Build
and Test" step that runs the inline xcodebuild pipeline with a single run: just
test; specifically, in the unit-tests job update the step whose name is "Build
and Test" to use run: just test (removing the multi-line xcodebuild invocation
and its reliance on xcpretty), and drop the separate "Select Xcode" step and any
hardcoded Xcode/simulator settings so the workflow uses the project's justfile
(which provides xcbeautify and the canonical simulator/Xcode configuration).
| D73E5EFC2C6A97F4007EB227 /* DamusAppNotificationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D78CD5972B8990300014D539 /* DamusAppNotificationView.swift */; }; | ||
| D73E5EFD2C6A97F4007EB227 /* InnerTimelineView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CE0E2B529A3ED5500DB4CA2 /* InnerTimelineView.swift */; }; | ||
| D73E5EFE2C6A97F4007EB227 /* (null) in Sources */ = {isa = PBXBuildFile; }; | ||
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; }; |
There was a problem hiding this comment.
Malformed PBXBuildFile entry missing fileRef.
This entry lacks the required fileRef attribute and has an unusual comment "BuildFile in Sources" instead of a real filename. Valid PBXBuildFile entries must reference a PBXFileReference, e.g.:
{isa = PBXBuildFile; fileRef = XXXXXXX /* SomeFile.swift */; };
This appears to be a merge artifact or project file corruption. Remove this line or replace it with a valid build file reference to prevent Xcode parsing errors.
🔎 Proposed fix
- D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; };📝 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.
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; }; |
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj at line 1512, The PBXBuildFile entry with ID
D73E5EFE2C6A97F4007EB227 is malformed because it lacks the required fileRef and
has a placeholder comment "BuildFile in Sources"; locate the PBXBuildFile block
for that ID in the project.pbxproj and either remove the entire PBXBuildFile
entry or replace it with a valid PBXBuildFile that includes a fileRef pointing
to the appropriate PBXFileReference (e.g., the corresponding .swift/.m/.xcassets
file ID), ensuring the comment matches the referenced filename so Xcode can
parse the project successfully.
| D73E5EFC2C6A97F4007EB227 /* DamusAppNotificationView.swift in Sources */, | ||
| D73E5EFD2C6A97F4007EB227 /* InnerTimelineView.swift in Sources */, | ||
| D73E5EFE2C6A97F4007EB227 /* (null) in Sources */, | ||
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */, |
There was a problem hiding this comment.
Remove reference to malformed BuildFile.
This line references the invalid PBXBuildFile entry flagged at line 1512. Both the definition and this reference should be removed.
🔎 Proposed fix
- D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */,📝 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.
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */, |
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj at line 7244, Remove the malformed
PBXBuildFile entry and its reference: delete the PBXBuildFile definition with ID
D73E5EFE2C6A97F4007EB227 and remove the corresponding occurrence
"D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */," from the Sources build
phase so there is no dangling reference to that invalid build file.
| /// The attach media button (image picker) | ||
| case post_composer_attach_media_button | ||
|
|
||
| /// Error message displayed when upload fails | ||
| case post_composer_error_message | ||
|
|
||
| /// Upload progress indicator | ||
| case post_composer_upload_progress |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of the new accessibility identifiers in SwiftUI views
echo "Searching for post_composer_attach_media_button usage:"
rg -n 'post_composer_attach_media_button' --type=swift -g '!AppAccessibilityIdentifiers.swift'
echo -e "\nSearching for post_composer_error_message usage:"
rg -n 'post_composer_error_message' --type=swift -g '!AppAccessibilityIdentifiers.swift'
echo -e "\nSearching for post_composer_upload_progress usage:"
rg -n 'post_composer_upload_progress' --type=swift -g '!AppAccessibilityIdentifiers.swift'Repository: damus-io/damus
Length of output: 530
Remove or implement post_composer_upload_progress identifier.
Two of the three new accessibility identifiers are properly used in PostView.swift:
post_composer_attach_media_button(line 170) ✓post_composer_error_message(line 331) ✓
However, post_composer_upload_progress is defined in the enum but has no corresponding usage in the codebase. Either remove this unused identifier or complete its implementation in the UI code.
🤖 Prompt for AI Agents
In @damus/AppAccessibilityIdentifiers.swift around lines 67 - 74, The enum case
post_composer_upload_progress in AppAccessibilityIdentifiers is defined but
unused; either remove that case from AppAccessibilityIdentifiers or add it to
the upload progress UI in PostView by assigning the matching accessibility
identifier to the progress control used during media upload (e.g., set the
ProgressView/UIActivityIndicator or custom upload indicator’s
accessibilityIdentifier to
AppAccessibilityIdentifiers.post_composer_upload_progress.rawValue where upload
progress is displayed/updated in PostView, near the existing uses of
post_composer_attach_media_button and post_composer_error_message).
| override func startLoading() { | ||
| SimulatedNetworkProtocol.requestCount += 1 | ||
| let currentCount = SimulatedNetworkProtocol.requestCount |
There was a problem hiding this comment.
Potential race condition on requestCount.
requestCount is a static var accessed from startLoading(), which may be called concurrently from multiple URLSession tasks. Without synchronization, this could lead to incorrect counts or data races.
For a DEBUG-only simulator, this may be acceptable, but if accurate counts matter for failThenSucceed logic, consider adding synchronization.
🔎 Proposed fix using a lock
private class SimulatedNetworkProtocol: URLProtocol {
static var condition: NetworkConditionSimulator.NetworkCondition = .timeout
static var urlPattern: String?
static var requestCount = 0
+ private static let lock = NSLock()
+
+ private static func incrementAndGetCount() -> Int {
+ lock.lock()
+ defer { lock.unlock() }
+ requestCount += 1
+ return requestCount
+ }
// ...
override func startLoading() {
- SimulatedNetworkProtocol.requestCount += 1
- let currentCount = SimulatedNetworkProtocol.requestCount
+ let currentCount = SimulatedNetworkProtocol.incrementAndGetCount()🤖 Prompt for AI Agents
In @damus/Shared/Utilities/NetworkConditionSimulator.swift around lines 95 - 97,
SimulatedNetworkProtocol.requestCount is incremented unsafely in startLoading(),
causing a data race when URLSession tasks call it concurrently; protect
increments and reads of the static requestCount (and any logic that depends on
it such as failThenSucceed) with synchronization — e.g., add a private serial
DispatchQueue or lock inside SimulatedNetworkProtocol and perform requestCount
+= 1 and the subsequent read under that lock, or replace requestCount with an
atomic-safe wrapper so that startLoading() uses the synchronized increment/read
to ensure correct counts for failThenSucceed logic.
damusTests/RelayPoolTests.swift
Outdated
| func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) { | ||
| let ndb = Ndb()! | ||
| let relayPool = RelayPool(ndb: ndb) | ||
| let info = RelayInfo(read: true, write: true) | ||
| let info: NIP65.RelayList.RelayItem.RWConfiguration = .readWrite | ||
|
|
||
| Task { | ||
| do { | ||
| for relay in urls { | ||
| guard let url = RelayURL(relay) else { | ||
| XCTFail("Invalid URL encountered: \(relay)") | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| for relay in urls { | ||
| guard let url = RelayURL(relay) else { | ||
| XCTFail("Invalid URL encountered: \(relay)") | ||
| return | ||
| let descriptor = RelayPool.RelayDescriptor(url: url, info: info) | ||
| try await relayPool.add_relay(descriptor) | ||
| } | ||
|
|
||
| let descriptor = RelayDescriptor(url: url, info: info) | ||
| try relayPool.add_relay(descriptor) | ||
| if expectedError != nil { | ||
| XCTFail("Expected \(expectedError!) error, but no error was thrown.") | ||
| } | ||
| } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists { | ||
| XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)") | ||
| } catch { | ||
| XCTFail("An unexpected error was thrown: \(error)") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-awaited Task causes test assertions to run after test completes.
Wrapping the test logic in Task { ... } without awaiting it means the XCTest method returns immediately, and assertions execute asynchronously after the test is considered complete. This can lead to false positives (test passes even when assertions fail).
🔎 Proposed fix: Convert to async test method
The function should be async and called from async test methods, or use XCTestExpectation:
-func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) {
+func testAddRelays(urls: [String], expectedError: RelayPool.RelayError? = nil) async {
let ndb = Ndb()!
let relayPool = RelayPool(ndb: ndb)
let info: NIP65.RelayList.RelayItem.RWConfiguration = .readWrite
- Task {
- do {
- for relay in urls {
- guard let url = RelayURL(relay) else {
- XCTFail("Invalid URL encountered: \(relay)")
- return
- }
-
- let descriptor = RelayPool.RelayDescriptor(url: url, info: info)
- try await relayPool.add_relay(descriptor)
+ do {
+ for relay in urls {
+ guard let url = RelayURL(relay) else {
+ XCTFail("Invalid URL encountered: \(relay)")
+ return
}
- if expectedError != nil {
- XCTFail("Expected \(expectedError!) error, but no error was thrown.")
- }
- } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists {
- XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)")
- } catch {
- XCTFail("An unexpected error was thrown: \(error)")
+ let descriptor = RelayPool.RelayDescriptor(url: url, info: info)
+ try await relayPool.add_relay(descriptor)
+ }
+
+ if expectedError != nil {
+ XCTFail("Expected \(expectedError!) error, but no error was thrown.")
}
+ } catch let error as RelayPool.RelayError where expectedError == .RelayAlreadyExists {
+ XCTAssertEqual(error, expectedError!, "Expected RelayAlreadyExists error, got \(error)")
+ } catch {
+ XCTFail("An unexpected error was thrown: \(error)")
}
}Then update the test methods:
func testAddRelay_ValidRelayURL_NoErrors() async {
await testAddRelays(urls: ["wss://relay.damus.io"])
}🤖 Prompt for AI Agents
In @damusTests/RelayPoolTests.swift around lines 58 - 84, The test spawns an
unawaited Task so assertions run after the XCTest method returns; make
testAddRelays synchronous from Swift concurrency by removing the Task wrapper,
marking the helper as async (func testAddRelays(... ) async) and using try await
directly when calling relayPool.add_relay(descriptor); then update each caller
test (e.g., testAddRelay_ValidRelayURL_NoErrors) to be async and call await
testAddRelays(...). Alternatively, if you must keep non-async tests, replace the
Task with an XCTestExpectation and fulfill it after the async work completes to
ensure assertions run before the test exits.
| - `timeout` - Simulates request timeout after 2 seconds | ||
| - `connectionLost` - Simulates connection lost error | ||
| - `notConnected` - Simulates no internet connection | ||
| - `slowNetwork` - Adds 3 second delay before responding |
There was a problem hiding this comment.
Fix hyphenation in network condition description.
Line 38 uses "3 second delay" but should use "3-second delay" (compound adjective before noun requires hyphen).
🔎 Proposed fix
-- `slowNetwork` - Adds 3 second delay before responding
+- `slowNetwork` - Adds 3-second delay before responding📝 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.
| - `slowNetwork` - Adds 3 second delay before responding | |
| - `slowNetwork` - Adds 3-second delay before responding |
🧰 Tools
🪛 LanguageTool
[grammar] ~38-~38: Use a hyphen to join words.
Context: ...rnet connection - slowNetwork - Adds 3 second delay before responding - `failTh...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In @docs/DEV_TIPS.md at line 38, The description for the slowNetwork entry uses
"3 second delay" — update the wording to the hyphenated compound adjective
"3-second delay" in the `slowNetwork` bullet so it reads "`slowNetwork` - Adds
3-second delay before responding".
| Apple's Network Link Conditioner tool simulates real-world network conditions. This is useful for manual testing of the entire app under degraded network. | ||
|
|
||
| **Setup on macOS (for Simulator testing):** | ||
| 1. Download "Additional Tools for Xcode" from https://developer.apple.com/download/all/ |
There was a problem hiding this comment.
Format bare URL as a proper markdown link.
The URL on line 65 should be wrapped in angle brackets or converted to a markdown link for proper rendering and accessibility.
🔎 Proposed fix
-1. Download "Additional Tools for Xcode" from https://developer.apple.com/download/all/
+1. Download "Additional Tools for Xcode" from <https://developer.apple.com/download/all/>📝 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.
| 1. Download "Additional Tools for Xcode" from https://developer.apple.com/download/all/ | |
| 1. Download "Additional Tools for Xcode" from <https://developer.apple.com/download/all/> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
65-65: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In @docs/DEV_TIPS.md at line 65, Replace the bare URL
"https://developer.apple.com/download/all/" on the "Download \"Additional Tools
for Xcode\"" line with a proper markdown link by either wrapping it in angle
brackets (<https://developer.apple.com/download/all/>) or converting it to a
descriptive link like [Additional Tools for Xcode
downloads](https://developer.apple.com/download/all/) so the URL renders and is
accessible.
scripts/throttle-network.sh
Outdated
| check_root() { | ||
| if [[ $EUID -ne 0 ]]; then | ||
| echo "Error: This script must be run with sudo" | ||
| echo " sudo $0 $*" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Fix: check_root references $* but no arguments are passed.
The error message uses $* to suggest the correct command, but check_root is called without arguments. Pass "$@" from the caller or use a different approach.
🔎 Suggested fix
check_root() {
if [[ $EUID -ne 0 ]]; then
echo "Error: This script must be run with sudo"
- echo " sudo $0 $*"
+ echo " sudo $0 ${ORIGINAL_ARGS[*]}"
exit 1
fi
}And at the start of the script after set -e:
set -e
+
+# Save original arguments for error messages
+ORIGINAL_ARGS=("$@")Alternatively, pass arguments explicitly:
start)
- check_root
+ check_root "$@"
start_throttle "$2"
;;
stop)
- check_root
+ check_root "$@"
stop_throttle
;;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 59-65: check_root references arguments, but none are ever passed.
(SC2120)
🤖 Prompt for AI Agents
In @scripts/throttle-network.sh around lines 59 - 65, The error message in
check_root() uses $* but the function is called without arguments; either pass
the script's original args into the function or stop referencing them. Fix by
updating the call site to pass the script arguments (call check_root "$@") and
keep the message using "$@" (change the echo to use "sudo $0 $@"), or
alternatively simplify the message to "sudo $0" inside check_root; reference the
check_root function and the call where it's invoked and ensure you pass "$@"
from the script startup (e.g., after set -e).
alltheseas
force-pushed
the
pr/upload-error-handling
branch
from
8759cc5 to
2bbc20e
Compare
2 participants
Summary
Hardens the image/video upload pipeline with improved error handling, retry logic, and comprehensive test coverage. This addresses user-reported upload failures on iOS 18 devices.
Commit Overview
Checklist
Standard PR Checklist
Closes:orFixes:tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patchesTest Report
Automated Tests
35 tests pass across 3 test suites:
Thread Sanitizer: All 35 tests pass with TSan enabled — no race conditions detected.
Manual Testing
Device: iPhone 17 Pro Simulator
iOS: 26.2
Branch: pr/upload-error-handling
Network Condition Testing
Tested with Network Link Conditioner:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Testing
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.