Mvvm r clean architecture - #226
Merged
Merged
Conversation
- Move all networking source files (NetworkManager, interceptors, models, services) from AIChat/Services/Networking into NetworkingKit/Sources/NetworkingKit - Add Package.swift with iOS 17 minimum deployment target - Replace LogManagerProtocol dependency in NetworkManager with a lightweight @sendable eventHandler closure, keeping the package free of app dependencies - Wire the local package into the Xcode project and link it to AIChat + AIChatTests targets - Bridge LogManager to the new eventHandler in Dependencies.swift via AnyLoggableEvent - Update NetworkManagerTests to import NetworkingKit directly Co-Authored-By: Claude <noreply@anthropic.com>
- RetryHandler: fix off-by-one error (0...retries → 0..<retries) to honour the configured maxRetries limit exactly; remove now-unreachable fallback throw at end of loop - NetworkManager: remove @observable (conflicts with Sendable on a non-observable networking type); add explicit public init to NetworkEvent so external consumers can construct values - NetworkManager.executeWithRetry: route through self.execute() instead of service.execute() so requestStart/Success/Failed events are emitted consistently during retry attempts - LoggingInterceptor: change logToConsole default from true → false so the package does not print to stdout in production consumers by default Co-Authored-By: Claude <noreply@anthropic.com>
When maxRetries=0 is passed (or configured), the loop `for attempt in 0..<0` would skip entirely, leaving the operation never executed and falling through to throw a "Retry failed" error unconditionally. Wrapping with `max(1, ...)` guarantees at least one attempt is always made regardless of the configured retry count. https://claude.ai/code/session_019DRKfCoC2yMPKrJpehMzc1
…iption Error.localizedDescription returns a non-optional String in Swift, so the `?? ""` fallback on NetworkError.localizedDescription was unnecessary. https://claude.ai/code/session_017YN1Ro2Y2ZrxkKnSTNkhDC
- Extract shared `networkEventHandler` helper in Dependencies.swift to eliminate duplicated event-handler closure across mock/dev/prod configs - Remove unnecessary local logManager copies (mockLog, devLog, prodLog) - Wire LoggingInterceptor's customLogger to logManager in dev config so network request/response logs are actually forwarded (previously a no-op) - Remove vestigial `@testable import AIChat` from 5 test files that only use types from NetworkingKit - Document `@unchecked Sendable` rationale on URLSessionNetworkService and LoggingInterceptor - Document deinit shared-session behaviour on URLSessionNetworkService - Document RetryHandler.execute maxRetries: 0 / always-at-least-once contract in API docs - Enable StrictConcurrency experimental feature in Package.swift to validate Sendable usage at the package level Co-Authored-By: Claude <noreply@anthropic.com>
Break the devLogger closure onto multiple lines to stay within the 160-character limit enforced by SwiftLint. Co-Authored-By: Claude <noreply@anthropic.com>
Specify that @observable injects a private ObservationRegistrar (itself Sendable) that the strict-concurrency checker cannot see, which is the precise reason @unchecked is required despite the conformance being sound. Co-Authored-By: Claude <noreply@anthropic.com>
URLSession has been concurrency-safe since Swift 5.7 / iOS 16. The real reason @unchecked is required is that URLSession is an Objective-C final class whose Sendable conformance cannot be inferred by the Swift compiler from its declaration alone. Co-Authored-By: Claude <noreply@anthropic.com>
The parameter name `maxRetries` implied retry-count semantics (attempts after the first), but the implementation treated it as a total-attempts count, making the doc comment and name contradict each other. - Rename `execute(maxRetries:)` → `execute(maxAttempts:)` so the name matches the documented "total attempts including the first" semantics. - Rename internal variable `retries` → `totalAttempts` for clarity. - Update `NetworkManager.executeWithRetry(maxRetries:)` call site to translate retry count → total attempts: `maxAttempts: maxRetries + 1`. - Update `nil` fallback: `configuration.maxRetries + 1` so the default behaviour (e.g. 3 retries → 4 attempts) stays consistent. - Update error message to report `totalAttempts`. `RetryConfiguration.maxRetries` and `shouldRetry(error:attempt:)` are unchanged — they correctly use retry-count semantics. Co-Authored-By: Claude <noreply@anthropic.com>
Remove the `attempt < configuration.maxRetries` guard from shouldRetry() so that callers passing a custom maxAttempts to execute() are no longer silently capped at configuration.maxRetries. The outer loop bound `attempt < totalAttempts - 1` in execute() is already the authoritative gate on total attempts; the duplicate guard in shouldRetry was preventing extra retries from ever firing. https://claude.ai/code/session_0152ZteuhXgAVbb2Mbyh5xVq
The attempt parameter was unused, causing shouldRetry to always return true for retryable error types regardless of how many attempts had been made. Added a guard that returns false when attempt >= maxRetries. Fixes the failing test: test_whenMaxRetriesExceeded_thenShouldNotRetry https://claude.ai/code/session_01LxHM1v1X7d8rdtBaJ5Qkjj
The protocol methods are async and actor-agnostic — forcing @mainactor on the protocol unnecessarily contaminates all callers with main-actor requirements. Removing it lets callers run network work on any executor. https://claude.ai/code/session_01RiESwf1EcSWKQcJSjKAjbj
…style Bumps swift-tools-version to 6.2, adds macOS(.v14) platform, removes explicit source paths (layout matches convention), and drops the now-redundant StrictConcurrency experimental setting (Swift 6 default). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Registers NetworkingKit alongside SamuraiLogging and other local packages in the Xcode workspace so it appears as a first-class package in the navigator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
shouldRetry(error:attempt:) was always guarding against configuration.maxRetries, so execute(maxAttempts:) calls with a higher limit would stop retrying too early. Added a maxRetries parameter (defaulting to configuration.maxRetries) and pass totalAttempts - 1 from execute so both the shouldRetry guard and the outer loop bound agree on the effective retry ceiling. https://claude.ai/code/session_012oALNaqxfWw82zd6hwaLVW
Avoid actor isolation warnings in the networking test helper by marking the static request handler as `nonisolated(unsafe)`.
Add unit tests that exercise shouldRetry(error:attempt:maxRetries:) with explicit overrides both above and below configuration.maxRetries, and integration-style tests for execute(maxAttempts:) that assert the interceptor retries up to the overridden ceiling and stops exactly there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cover override behavior in shouldRetry and execute, and add coverage for maxAttempts clamping to a single attempt.
Contributor
📝 WalkthroughWalkthroughExtracted the networking layer into a new Swift Package Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NetworkManager
participant RetryHandler
participant NetworkService
participant URLSession
participant EventHandler
Client->>NetworkManager: execute(request)
NetworkManager->>EventHandler: emit(requestStart)
NetworkManager->>RetryHandler: execute(operation, maxAttempts?)
RetryHandler->>NetworkService: execute(request)
NetworkService->>URLSession: perform HTTP request
URLSession-->>NetworkService: response
NetworkService-->>RetryHandler: NetworkResponse / NetworkError
RetryHandler-->>NetworkManager: success / throw
NetworkManager->>EventHandler: emit(requestSuccess/requestFailed)
NetworkManager-->>Client: return response / throw
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the networking layer by moving it from the main app target into a new standalone local Swift package called NetworkingKit. The app's dependency injection has been updated to use this new module, bridging network events to the existing logging system via a new event handler. Feedback focuses on restoring public API documentation that was lost during the migration and simplifying the devLogger implementation in the dependency container.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0e1e3da9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
SamuraiLogging/Sources/SamuraiLogging/LogManager.swift (1)
10-19: Remove@Observable— it provides no value without observable properties.The class has no
@Publishedor mutable properties, so@Observableinjects anObservationRegistrarfor no purpose. Since all methods are markednonisolatedand only read the immutableservicesarray, the implementation is safely callable from any isolation context (SwiftUI views,@Sendableclosures, etc.). Removing@Observableeliminates unnecessary complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SamuraiLogging/Sources/SamuraiLogging/LogManager.swift` around lines 10 - 19, Remove the unnecessary `@Observable` attribute from the LogManager declaration: delete the `@Observable` annotation above the public final class LogManager so the compiler no longer injects an ObservationRegistrar; leave the existing `@unchecked` Sendable comment, the immutable services array, and the nonisolated methods (e.g., any nonisolated functions that read services) unchanged so the class continues to be safely callable from any isolation context.NetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swift (1)
3-4: Consider adding@unchecked Sendabledocumentation.For consistency with
LoggingInterceptor, consider adding a doc comment explaining why@unchecked Sendableis safe here (immutableheaderNameand@SendableconstrainedtokenProvider).📝 Suggested documentation
/// Request interceptor that adds authentication headers +/// +/// `@unchecked Sendable`: `headerName` is an immutable `let` constant and +/// `tokenProvider` is constrained to `@Sendable`, making the type safe to use +/// across isolation domains. public final class AuthInterceptor: RequestInterceptor, `@unchecked` Sendable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swift` around lines 3 - 4, Add a doc comment to AuthInterceptor explaining why `@unchecked` Sendable is safe: mention that headerName is immutable and tokenProvider is constrained to a `@Sendable` closure (so no shared mutable state), mirroring the rationale used in LoggingInterceptor; place this documentation above the AuthInterceptor declaration and reference the headerName and tokenProvider properties and the `@unchecked` Sendable annotation.NetworkingKit/Sources/NetworkingKit/NetworkManager.swift (1)
53-63: Consider deriving default retry count fromRetryConfiguration.default.The hardcoded
maxRetries: 3duplicates the value fromRetryConfiguration.default.maxRetries. If the default configuration changes, these convenience methods will become inconsistent.Reference the configuration default
public func executeWithRetry( _ request: NetworkRequest ) async throws -> NetworkResponse { - try await executeWithRetry(request, maxRetries: 3) + try await executeWithRetry(request, maxRetries: RetryConfiguration.default.maxRetries) } public func executeWithRetry<T: Decodable>( _ request: NetworkRequest, responseType: T.Type ) async throws -> T { - try await executeWithRetry(request, responseType: responseType, decoder: JSONDecoder(), maxRetries: 3) + try await executeWithRetry(request, responseType: responseType, decoder: JSONDecoder(), maxRetries: RetryConfiguration.default.maxRetries) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/NetworkManager.swift` around lines 53 - 63, The two convenience overloads of executeWithRetry currently hardcode maxRetries: 3; update them to derive the retry count from RetryConfiguration.default.maxRetries instead (replace the literal 3 with RetryConfiguration.default.maxRetries) so executeWithRetry(_ request: NetworkRequest) and executeWithRetry<T: Decodable>(_ request: NetworkRequest, responseType: T.Type) remain consistent with the RetryConfiguration default.NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift (1)
93-93: Theattempt < totalAttempts - 1check is redundant.The
shouldRetrymethod already includesguard attempt < effectiveMax(whereeffectiveMax = totalAttempts - 1), so ifshouldRetryreturnstrue, the second condition is guaranteed to be true.Simplify the condition
- if shouldRetry(error: error, attempt: attempt, maxRetries: totalAttempts - 1) && attempt < totalAttempts - 1 { + if shouldRetry(error: error, attempt: attempt, maxRetries: totalAttempts - 1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift` at line 93, Remove the redundant attempt bounds check from the conditional in RetryInterceptor; rely solely on shouldRetry(error:attempt:maxRetries:) which already guards attempt < effectiveMax (effectiveMax = totalAttempts - 1). Update the if statement that currently uses shouldRetry(... ) && attempt < totalAttempts - 1 to call only shouldRetry(error: error, attempt: attempt, maxRetries: totalAttempts - 1), referencing the shouldRetry function and the attempt/totalAttempts variables so the logic remains correct and simpler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift`:
- Around line 103-106: The throw NetworkError.unknown("Retry failed: exhausted
\(totalAttempts) attempts") at the end of RetryInterceptor is dead code because
the retry loop always returns or throws; remove that unreachable throw or
replace it with a clear compiler hint (e.g., fatalError("unreachable")) to
document the invariant. Locate the retry loop in the RetryInterceptor method
that contains the retry logic and either delete the final throw
NetworkError.unknown(...) statement or swap it for a fatalError/unreachable
comment so the intent is explicit.
---
Nitpick comments:
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swift`:
- Around line 3-4: Add a doc comment to AuthInterceptor explaining why
`@unchecked` Sendable is safe: mention that headerName is immutable and
tokenProvider is constrained to a `@Sendable` closure (so no shared mutable
state), mirroring the rationale used in LoggingInterceptor; place this
documentation above the AuthInterceptor declaration and reference the headerName
and tokenProvider properties and the `@unchecked` Sendable annotation.
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift`:
- Line 93: Remove the redundant attempt bounds check from the conditional in
RetryInterceptor; rely solely on shouldRetry(error:attempt:maxRetries:) which
already guards attempt < effectiveMax (effectiveMax = totalAttempts - 1). Update
the if statement that currently uses shouldRetry(... ) && attempt <
totalAttempts - 1 to call only shouldRetry(error: error, attempt: attempt,
maxRetries: totalAttempts - 1), referencing the shouldRetry function and the
attempt/totalAttempts variables so the logic remains correct and simpler.
In `@NetworkingKit/Sources/NetworkingKit/NetworkManager.swift`:
- Around line 53-63: The two convenience overloads of executeWithRetry currently
hardcode maxRetries: 3; update them to derive the retry count from
RetryConfiguration.default.maxRetries instead (replace the literal 3 with
RetryConfiguration.default.maxRetries) so executeWithRetry(_ request:
NetworkRequest) and executeWithRetry<T: Decodable>(_ request: NetworkRequest,
responseType: T.Type) remain consistent with the RetryConfiguration default.
In `@SamuraiLogging/Sources/SamuraiLogging/LogManager.swift`:
- Around line 10-19: Remove the unnecessary `@Observable` attribute from the
LogManager declaration: delete the `@Observable` annotation above the public final
class LogManager so the compiler no longer injects an ObservationRegistrar;
leave the existing `@unchecked` Sendable comment, the immutable services array,
and the nonisolated methods (e.g., any nonisolated functions that read services)
unchanged so the class continues to be safely callable from any isolation
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b55a10c5-ec84-477b-bcf6-daaba3a8df3c
📒 Files selected for processing (30)
AIChat.xcodeproj/project.pbxprojAIChat.xcworkspace/contents.xcworkspacedataAIChat/App/Dependencies.swiftAIChat/Services/Networking/NetworkManager.swiftAIChatTests/Services/Networking/RetryHandlerTests.swiftNetworkingKit/.gitignoreNetworkingKit/Package.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/LoggingInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/RequestInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Models/HTTPMethod.swiftNetworkingKit/Sources/NetworkingKit/Models/NetworkError.swiftNetworkingKit/Sources/NetworkingKit/Models/NetworkRequest.swiftNetworkingKit/Sources/NetworkingKit/Models/NetworkResponse.swiftNetworkingKit/Sources/NetworkingKit/NetworkManager.swiftNetworkingKit/Sources/NetworkingKit/Services/MockNetworkService.swiftNetworkingKit/Sources/NetworkingKit/Services/NetworkServiceProtocol.swiftNetworkingKit/Sources/NetworkingKit/Services/URLSessionNetworkService.swiftNetworkingKit/Tests/NetworkingKitTests/AuthInterceptorTests.swiftNetworkingKit/Tests/NetworkingKitTests/LoggingInterceptorTests.swiftNetworkingKit/Tests/NetworkingKitTests/MockNetworkServiceTests.swiftNetworkingKit/Tests/NetworkingKitTests/NetworkErrorTests.swiftNetworkingKit/Tests/NetworkingKitTests/NetworkManagerTests.swiftNetworkingKit/Tests/NetworkingKitTests/NetworkRequestTests.swiftNetworkingKit/Tests/NetworkingKitTests/NetworkResponseTests.swiftNetworkingKit/Tests/NetworkingKitTests/RetryHandlerTests.swiftNetworkingKit/Tests/NetworkingKitTests/URLSessionNetworkServiceTestHelpers.swiftNetworkingKit/Tests/NetworkingKitTests/URLSessionNetworkServiceTests.swiftSamuraiLogging/Sources/SamuraiLogging/LogManager.swift
💤 Files with no reviewable changes (2)
- AIChatTests/Services/Networking/RetryHandlerTests.swift
- AIChat/Services/Networking/NetworkManager.swift
Add parameter and return docs for the protocol execute method. Document initializer arguments for mock and URLSession services.
Clarify why `@unchecked Sendable` is safe for the immutable header name and `@Sendable` token provider closure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift (1)
109-112:⚠️ Potential issue | 🟡 MinorLine 111 is unreachable dead code.
The loop at lines 95-108 will always terminate via
return(line 97) orthrow(lines 105, 107). There is no execution path that allows the loop to complete normally and reach line 111.Consider removing this unreachable statement or replacing it with a compiler hint to document the invariant.
Suggested fix
} } - - throw NetworkError.unknown("Retry failed: exhausted \(totalAttempts) attempts") }Or if you prefer a defensive fallback:
- throw NetworkError.unknown("Retry failed: exhausted \(totalAttempts) attempts") + // This line should be unreachable; the loop always returns or throws. + preconditionFailure("Retry loop exited without returning or throwing after \(totalAttempts) attempts")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift` around lines 109 - 112, The final throw NetworkError.unknown("Retry failed: exhausted \(totalAttempts) attempts") after the retry loop is unreachable because the loop always returns or throws; remove that dead statement or replace it with a clear compiler/runtime invariant (e.g., replace with preconditionFailure("unreachable: exhausted attempts") or fatalError(...)) in the RetryInterceptor retry logic so the intent is documented; reference the retry loop inside RetryInterceptor and the symbols totalAttempts and NetworkError.unknown when locating the code to change.
🧹 Nitpick comments (1)
SamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/MixpanelServiceTests.swift (1)
27-40: Consider verifying that.infoevents are actually filtered out, not just that they don't crash.Per the implementation in
MixpanelService.swift:60,.infoevents are explicitly dropped viaguard event.type != .info else { return }. These tests only verify no crash occurs, but don't confirm the filtering behavior. If the guard were accidentally removed, these tests would still pass.To properly verify the contract documented in
LogType(that.infoevents are "not considered analytics"), consider introducing a mock or spy for the Mixpanel instance to assert thattrack()is never called for.infoevents.💡 Example approach using a protocol and mock
// In production code, extract a protocol: protocol MixpanelTracking { func track(event: String?, properties: [String: MixpanelType]?) } // In tests, create a spy: final class MockMixpanel: MixpanelTracking { var trackCalled = false func track(event: String?, properties: [String: MixpanelType]?) { trackCalled = true } } // Then assert: `@Test`("trackEvent filters out .info events") func test_whenTrackingInfoEvent_thenMixpanelIsNotCalled() { let mock = MockMixpanel() let service = MixpanelService(instance: mock) let event = AnyLoggableEvent(eventName: "info_event", type: .info) service.trackEvent(event: event) `#expect`(mock.trackCalled == false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/MixpanelServiceTests.swift` around lines 27 - 40, Add assertions that Mixpanel's tracking method is never invoked for .info events instead of only ensuring no crash: introduce an injectable protocol (e.g., MixpanelTracking) used by MixpanelService and replace concrete Mixpanel instantiation in tests with a spy/mock (e.g., MockMixpanel) that records calls to track(event:properties:); then update tests test_whenTrackingInfoEvent_thenDoesNotCrash and test_whenTrackingInfoScreen_thenDoesNotCrash (or create new tests) to construct MixpanelService with the mock, call trackEvent(event: AnyLoggableEvent(eventName: ..., type: .info)) and trackScreen(event: ...), and assert the mock's trackCalled flag (or call count) remains false to verify .info events are filtered out by MixpanelService.trackEvent / trackScreen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AIChat/App/Dependencies.swift`:
- Around line 169-185: The devLogger currently forwards raw log messages
(including response headers) into logManager.trackEvent, risking PII leakage;
update the code that constructs devLogger (used by LoggingInterceptor passed
into URLSessionNetworkService and NetworkManager) to sanitize or mask sensitive
response header values before calling logManager.trackEvent, or switch devLogger
to only write to console/diagnostic logger while ensuring trackEvent is called
with a redacted message; make changes around the devLogger closure and its
invocation so LoggingInterceptor still receives a logger but any message sent to
logManager.trackEvent has sensitive header values (e.g., Set-Cookie,
Authorization, tokens) removed or replaced with masked placeholders.
---
Duplicate comments:
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift`:
- Around line 109-112: The final throw NetworkError.unknown("Retry failed:
exhausted \(totalAttempts) attempts") after the retry loop is unreachable
because the loop always returns or throws; remove that dead statement or replace
it with a clear compiler/runtime invariant (e.g., replace with
preconditionFailure("unreachable: exhausted attempts") or fatalError(...)) in
the RetryInterceptor retry logic so the intent is documented; reference the
retry loop inside RetryInterceptor and the symbols totalAttempts and
NetworkError.unknown when locating the code to change.
---
Nitpick comments:
In
`@SamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/MixpanelServiceTests.swift`:
- Around line 27-40: Add assertions that Mixpanel's tracking method is never
invoked for .info events instead of only ensuring no crash: introduce an
injectable protocol (e.g., MixpanelTracking) used by MixpanelService and replace
concrete Mixpanel instantiation in tests with a spy/mock (e.g., MockMixpanel)
that records calls to track(event:properties:); then update tests
test_whenTrackingInfoEvent_thenDoesNotCrash and
test_whenTrackingInfoScreen_thenDoesNotCrash (or create new tests) to construct
MixpanelService with the mock, call trackEvent(event:
AnyLoggableEvent(eventName: ..., type: .info)) and trackScreen(event: ...), and
assert the mock's trackCalled flag (or call count) remains false to verify .info
events are filtered out by MixpanelService.trackEvent / trackScreen.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5588297-968c-4a6a-9e0a-5b910ff03f32
📒 Files selected for processing (14)
AIChat/App/Dependencies.swiftAIChat/Components/ViewModifiers/AppearAnalyticsViewModifier.swiftAIChatTests/Services/Auth/AuthManagerTests.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/LoggingInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/RequestInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Models/NetworkRequest.swiftNetworkingKit/Sources/NetworkingKit/Models/NetworkResponse.swiftNetworkingKit/Sources/NetworkingKit/Services/MockNetworkService.swiftNetworkingKit/Sources/NetworkingKit/Services/NetworkServiceProtocol.swiftNetworkingKit/Sources/NetworkingKit/Services/URLSessionNetworkService.swiftSamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/MixpanelServiceTests.swiftSamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/SamuraiLoggingMixpanelTests.swift
💤 Files with no reviewable changes (2)
- AIChat/Components/ViewModifiers/AppearAnalyticsViewModifier.swift
- SamuraiLoggingMixpanel/Tests/SamuraiLoggingMixpanelTests/SamuraiLoggingMixpanelTests.swift
✅ Files skipped from review due to trivial changes (2)
- NetworkingKit/Sources/NetworkingKit/Models/NetworkResponse.swift
- NetworkingKit/Sources/NetworkingKit/Models/NetworkRequest.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- NetworkingKit/Sources/NetworkingKit/Services/NetworkServiceProtocol.swift
- NetworkingKit/Sources/NetworkingKit/Interceptors/RequestInterceptor.swift
- NetworkingKit/Sources/NetworkingKit/Interceptors/AuthInterceptor.swift
- NetworkingKit/Sources/NetworkingKit/Interceptors/LoggingInterceptor.swift
- NetworkingKit/Sources/NetworkingKit/Services/URLSessionNetworkService.swift
- NetworkingKit/Sources/NetworkingKit/Services/MockNetworkService.swift
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift (1)
26-37:⚠️ Potential issue | 🔴 CriticalGuard
maxRetries + 1against integer overflow.
RetryConfigurationis public with a publicmaxRetries: Intfield that accepts arbitrary integer values. Theinitmethod (lines 26-38) performs no validation. Line 93 then evaluatesconfiguration.maxRetries + 1unconditionally, which crashes at runtime ifmaxRetries == Int.max. Validate or clamp the retry count in the initializer, or guard the addition at line 93.🐛 Possible fix
- let totalAttempts = max(1, maxAttempts ?? (configuration.maxRetries + 1)) + let defaultRetries = max(0, configuration.maxRetries) + let defaultAttempts = defaultRetries == Int.max ? Int.max : defaultRetries + 1 + let totalAttempts = max(1, maxAttempts ?? defaultAttempts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift` around lines 26 - 37, The initializer for RetryConfiguration accepts an arbitrary Int for maxRetries but later the code unconditionally computes configuration.maxRetries + 1 which can overflow if maxRetries == Int.max; update the public init (the RetryConfiguration.init) to validate and clamp maxRetries to a safe range (e.g., ensure it's non‑negative and at most Int.max - 1) so that any subsequent addition (configuration.maxRetries + 1) cannot overflow, and store the clamped value back to self.maxRetries.
🧹 Nitpick comments (2)
NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift (2)
41-46: Consider making backoff jitter configurable.The delay calculation is fully deterministic, so concurrent 429/5xx failures will retry in lockstep. Adding jitter here would smooth retry bursts and reduce load spikes during recovery.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift` around lines 41 - 46, The delay(for attempt:) currently returns a deterministic backoff; make jitter configurable by adding a jitter parameter (e.g., jitterFactor: Double or jitterSeconds: TimeInterval) to the RetryInterceptor, then apply a random perturbation to the computed delay inside delay(for attempt:)—for example multiply by a random value in (1 - jitterFactor)...(1 + jitterFactor) or add a random offset up to jitterSeconds—clamp the final value with min(delay, maxDelay) and ensure non-negative results; reference the exponentialBackoff, baseDelay, maxDelay and delay(for:) symbols when making the change.
58-61: Document the public attempt contract.
shouldRetryis now public, but the comment still only describes the error. Please spell out thatattemptis zero-based and thatmaxRetriesexcludes the initial attempt; the tests inNetworkingKit/Tests/NetworkingKitTests/RetryHandlerTests.swiftalready depend on that contract, and callers can otherwise end up off by one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift` around lines 58 - 61, Update the public doc comment on the shouldRetry(error:attempt:maxRetries:) method to explicitly state the attempt parameter is zero-based (0 for the first try) and that maxRetries is the count of additional retries after the initial attempt (i.e., it excludes the initial attempt), so callers and implementers observe the same off-by-one contract used by NetworkingKitTests/NetworkingKitTests/RetryHandlerTests.swift; modify the comment above public func shouldRetry(error: NetworkError, attempt: Int, maxRetries: Int? = nil) to include these two clarifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift`:
- Around line 26-37: The initializer for RetryConfiguration accepts an arbitrary
Int for maxRetries but later the code unconditionally computes
configuration.maxRetries + 1 which can overflow if maxRetries == Int.max; update
the public init (the RetryConfiguration.init) to validate and clamp maxRetries
to a safe range (e.g., ensure it's non‑negative and at most Int.max - 1) so that
any subsequent addition (configuration.maxRetries + 1) cannot overflow, and
store the clamped value back to self.maxRetries.
---
Nitpick comments:
In `@NetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift`:
- Around line 41-46: The delay(for attempt:) currently returns a deterministic
backoff; make jitter configurable by adding a jitter parameter (e.g.,
jitterFactor: Double or jitterSeconds: TimeInterval) to the RetryInterceptor,
then apply a random perturbation to the computed delay inside delay(for
attempt:)—for example multiply by a random value in (1 - jitterFactor)...(1 +
jitterFactor) or add a random offset up to jitterSeconds—clamp the final value
with min(delay, maxDelay) and ensure non-negative results; reference the
exponentialBackoff, baseDelay, maxDelay and delay(for:) symbols when making the
change.
- Around line 58-61: Update the public doc comment on the
shouldRetry(error:attempt:maxRetries:) method to explicitly state the attempt
parameter is zero-based (0 for the first try) and that maxRetries is the count
of additional retries after the initial attempt (i.e., it excludes the initial
attempt), so callers and implementers observe the same off-by-one contract used
by NetworkingKitTests/NetworkingKitTests/RetryHandlerTests.swift; modify the
comment above public func shouldRetry(error: NetworkError, attempt: Int,
maxRetries: Int? = nil) to include these two clarifications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b3b0b3c-0d2e-444e-8daf-d2d2efeb6fbb
📒 Files selected for processing (2)
NetworkingKit/Sources/NetworkingKit/Interceptors/LoggingInterceptor.swiftNetworkingKit/Sources/NetworkingKit/Interceptors/RetryInterceptor.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- NetworkingKit/Sources/NetworkingKit/Interceptors/LoggingInterceptor.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit