feat: Add network details for session replay on iOS #4891
3 issues
code-review: Found 3 issues (1 high, 2 medium)
High
NSDictionary indexer assignment will throw runtime exception - `src/Sentry/Platforms/Cocoa/Extensions/CocoaExtensions.cs:207-220`
The code creates an immutable NSDictionary<NSString, NSObject> on line 207 and then attempts to mutate it via indexer assignment on lines 216 and 220 (dict[key] = ...). In Cocoa, NSDictionary is immutable and does not support indexed assignment - this will throw a runtime exception. The existing code in this file correctly uses NSDictionary.FromObjectsAndKeys() with a temporary C# Dictionary (see lines 153-170) or NSMutableDictionary for mutation.
Medium
Duplicate test methods defined for ANDROID platform - `test/Sentry.Tests/SentryHttpMessageHandlerTests.cs:617-667`
The test method HandleResponse_SpanExists_AddsReplayBreadcrumbData is defined twice - once in the #if ANDROID || IOS || MACCATALYST block (lines 617-667) and again in the #if ANDROID block (lines 697-740). Similarly, HandleResponse_NoSpanExists_NoReplayBreadcrumbData is duplicated. When compiled for ANDROID, this will cause a compiler error due to duplicate method definitions.
Synchronous Send_Executed_FailedRequestsCaptured test removed without replacement - `test/Sentry.Tests/SentryHttpMessageHandlerTests.cs:614-635`
The test Send_Executed_FailedRequestsCaptured that validates the failed request handler is invoked for synchronous HTTP calls has been removed. The async counterpart SendAsync_Executed_FailedRequestsCaptured (line 334) still exists. This reduces test coverage for the synchronous path, which is a deliberate feature of the handler (the file header comment notes all tests should cover both sync and async methods).
Duration: 1m 8s · Tokens: 425.8k in / 6.3k out · Cost: $0.79 (+merge: $0.00)
Annotations
Check failure on line 220 in src/Sentry/Platforms/Cocoa/Extensions/CocoaExtensions.cs
sentry-warden / warden: code-review
NSDictionary indexer assignment will throw runtime exception
The code creates an immutable `NSDictionary<NSString, NSObject>` on line 207 and then attempts to mutate it via indexer assignment on lines 216 and 220 (`dict[key] = ...`). In Cocoa, `NSDictionary` is immutable and does not support indexed assignment - this will throw a runtime exception. The existing code in this file correctly uses `NSDictionary.FromObjectsAndKeys()` with a temporary C# `Dictionary` (see lines 153-170) or `NSMutableDictionary` for mutation.
Check warning on line 667 in test/Sentry.Tests/SentryHttpMessageHandlerTests.cs
sentry-warden / warden: code-review
Duplicate test methods defined for ANDROID platform
The test method `HandleResponse_SpanExists_AddsReplayBreadcrumbData` is defined twice - once in the `#if ANDROID || IOS || MACCATALYST` block (lines 617-667) and again in the `#if ANDROID` block (lines 697-740). Similarly, `HandleResponse_NoSpanExists_NoReplayBreadcrumbData` is duplicated. When compiled for ANDROID, this will cause a compiler error due to duplicate method definitions.
Check warning on line 635 in test/Sentry.Tests/SentryHttpMessageHandlerTests.cs
sentry-warden / warden: code-review
Synchronous Send_Executed_FailedRequestsCaptured test removed without replacement
The test `Send_Executed_FailedRequestsCaptured` that validates the failed request handler is invoked for synchronous HTTP calls has been removed. The async counterpart `SendAsync_Executed_FailedRequestsCaptured` (line 334) still exists. This reduces test coverage for the synchronous path, which is a deliberate feature of the handler (the file header comment notes all tests should cover both sync and async methods).