Finish async concurrency migration - #28
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR migrates person and image operations from completion handlers to async/throws, adds cancellation handling, applies ChangesSwift concurrency migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant View
participant MainActorTask
participant UseCase
participant PersonsGateway
participant ImagesCache
View->>MainActorTask: Start async operation
MainActorTask->>UseCase: Await person operation
UseCase->>PersonsGateway: Await gateway operation
PersonsGateway-->>UseCase: Return value or CoreError
UseCase-->>MainActorTask: Return value or CoreError
MainActorTask->>ImagesCache: Await image loading
ImagesCache-->>MainActorTask: Return UIImage or CoreError
MainActorTask-->>View: Update UI or widget entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
44740fe to
3a4937a
Compare
ae81069 to
2307078
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Widget/AgeWidgetProvider.swift (1)
92-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep widget image loading concurrent.
resolveImages()awaitsImagesCache.loadImageFromDiskOrMemoryinside each loop iteration, so line 100 cannot run before line 97 completes. This serializes image loading and can delay the widget timeline callback while waiting for a slow image decode/retrieve.Start the loads concurrently, then collect them in input order. Keep the
try?fallback for failed image loads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Widget/AgeWidgetProvider.swift` around lines 92 - 107, Update resolveImages(for:) to launch all person image loads concurrently rather than awaiting each ImagesCache.loadImageFromDiskOrMemory call inside the loop, then collect results in the original people order when constructing WidgetPerson values. Preserve nil images for missing PersonImage values or failed loads by retaining the existing try? fallback.Source: Coding guidelines
GrowingUpTests/Gateways/CoreDataPersonsGatewayTests.swift (1)
60-62: 📐 Maintainability & Code Quality | 🔵 TrivialAdd an assertion to
test_SUT_FetchPersons_ShouldSucceed.The test discards the fetched array with
_ = try await inMemoryCoreDataGateway.fetchPersons(). It only checks that the call does not throw. Add an assertion on the returned array, for example that it is empty in this fixture, to catch regressions infetchPersonsfiltering or ordering.✅ Proposed assertion
func test_SUT_FetchPersons_ShouldSucceed() async throws { - _ = try await inMemoryCoreDataGateway.fetchPersons() + let persons = try await inMemoryCoreDataGateway.fetchPersons() + XCTAssertTrue(persons.isEmpty) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GrowingUpTests/Gateways/CoreDataPersonsGatewayTests.swift` around lines 60 - 62, Update test_SUT_FetchPersons_ShouldSucceed to retain the array returned by inMemoryCoreDataGateway.fetchPersons() and assert its expected contents for this fixture, such as verifying that it is empty, while preserving the async throwing test flow.GrowingUpTests/UseCases/RemovePersonUseCaseTests.swift (1)
7-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the error-propagation test.
RemovePersonUseCaseImplementation.remove(person:)isasync throws, but this change removes its failure-path coverage. Add a test that configuresPersonsGatewaySpywith.failure(.coreDataFetchFailed)and verifies thatsut.remove(person:)throws that error.Proposed test
+ func testRemovePropagatesError() async { + let gateway = PersonsGatewaySpy() + let sut = RemovePersonUseCaseImplementation(personsGateway: gateway) + gateway.removePersonResultToBeReturned = .failure(.coreDataFetchFailed) + + do { + try await sut.remove(person: Person.createPerson()) + XCTFail("Expected removal failure") + } catch { + XCTAssertEqual(error as? CoreError, .coreDataFetchFailed) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@GrowingUpTests/UseCases/RemovePersonUseCaseTests.swift` around lines 7 - 14, Add an error-propagation test alongside testRemoveCallsGateway using PersonsGatewaySpy configured with .failure(.coreDataFetchFailed), then assert that awaiting sut.remove(person:) throws the same .coreDataFetchFailed error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@GrowingUp.xcodeproj/project.pbxproj`:
- Line 1145: Update the SwiftLint setup documentation in Readme.md to reflect
the script’s required behavior: SwiftLint must be installed for builds, and
missing SwiftLint causes the Xcode phase to fail rather than emit only a
warning. Remove or revise language describing the phase as optional or
warning-only.
- Line 1145: Update the SwiftLint invocation in the build phase shellScript to
run the strict lint command, matching scripts/lint-swift.sh, while preserving
the existing availability check and failure handling.
In
`@GrowingUp/Scenes/EditPerson/Cells/ImagePickersTableViewCell/ImagesTableViewCell.swift`:
- Around line 121-125: Bind both asynchronous image-loading tasks in
ImagesTableViewCell to the cell’s current content: track each app-image and
widget-image request or capture a content identity, then validate it before
calling drawImage so reused cells cannot display stale results. Apply this at
the app-image site in
GrowingUp/Scenes/EditPerson/Cells/ImagePickersTableViewCell/ImagesTableViewCell.swift
lines 121-125 and the widget-image site in the same file lines 140-144.
In `@GrowingUp/Scenes/PersonOverview/PersonOverviewPresenter.swift`:
- Around line 48-51: Ensure the repeating timer created in loadPerson is
invalidated when PersonOverviewPresenter is released by adding a deinit teardown
that calls timer?.invalidate(). Keep the existing refresh-time invalidation
unchanged.
---
Nitpick comments:
In `@GrowingUpTests/Gateways/CoreDataPersonsGatewayTests.swift`:
- Around line 60-62: Update test_SUT_FetchPersons_ShouldSucceed to retain the
array returned by inMemoryCoreDataGateway.fetchPersons() and assert its expected
contents for this fixture, such as verifying that it is empty, while preserving
the async throwing test flow.
In `@GrowingUpTests/UseCases/RemovePersonUseCaseTests.swift`:
- Around line 7-14: Add an error-propagation test alongside
testRemoveCallsGateway using PersonsGatewaySpy configured with
.failure(.coreDataFetchFailed), then assert that awaiting sut.remove(person:)
throws the same .coreDataFetchFailed error.
In `@Widget/AgeWidgetProvider.swift`:
- Around line 92-107: Update resolveImages(for:) to launch all person image
loads concurrently rather than awaiting each
ImagesCache.loadImageFromDiskOrMemory call inside the loop, then collect results
in the original people order when constructing WidgetPerson values. Preserve nil
images for missing PersonImage values or failed loads by retaining the existing
try? fallback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a4a3594c-09f8-4324-9c4c-0606b1592952
📒 Files selected for processing (47)
Core/Entities/AddPersonParameters.swiftCore/Entities/Person.swiftCore/EntityGateway/Cache/CachePersonsGateway.swiftCore/EntityGateway/Cache/ImagesCache.swiftCore/EntityGateway/LocalPersistance/CoreDataPersonsGateway.swiftCore/Gateways/PersonsGateway.swiftCore/UseCases/AddPersonUseCase.swiftCore/UseCases/EditPersonUseCase.swiftCore/UseCases/FetchPersonsUseCase.swiftCore/UseCases/RemovePersonUseCase.swiftGrowingUp.xcodeproj/project.pbxprojGrowingUp/Scenes/EditPerson/Cells/DateTableViewCell/DateTableViewCell.swiftGrowingUp/Scenes/EditPerson/Cells/ImagePickersTableViewCell/ImagesTableViewCell.swiftGrowingUp/Scenes/EditPerson/Cells/TextFieldTableViewCell/TextFieldTableViewCell.swiftGrowingUp/Scenes/EditPerson/Cells/ToggleTableViewCell/ToggleTableViewCell.swiftGrowingUp/Scenes/EditPerson/Create/AddPersonConfigurator.swiftGrowingUp/Scenes/EditPerson/Create/AddPersonPresenter.swiftGrowingUp/Scenes/EditPerson/Edit/EditPersonConfigurator.swiftGrowingUp/Scenes/EditPerson/Edit/EditPersonPresenter.swiftGrowingUp/Scenes/EmptyPerson/EmptyPersonConfigurator.swiftGrowingUp/Scenes/EmptyPerson/EmptyPersonPresenter.swiftGrowingUp/Scenes/EmptyPerson/EmptyPersonViewRouter.swiftGrowingUp/Scenes/PersonOverview/PersonOverviewConfigurator.swiftGrowingUp/Scenes/PersonOverview/PersonOverviewPresenter.swiftGrowingUp/Scenes/PersonOverview/PersonOverviewRouter.swiftGrowingUp/Scenes/PersonOverview/PersonOverviewViewController.swiftGrowingUp/Scenes/PersonsList/PersonsListConfigurator.swiftGrowingUp/Scenes/PersonsList/PersonsListPresenter.swiftGrowingUp/Views/VerticalButton.swiftGrowingUpTests/Gateways/CachePersonsGatewayTests.swiftGrowingUpTests/Gateways/CoreDataPersonsGatewayTests.swiftGrowingUpTests/Helpers/Gateways/PersonsGatewaySpy.swiftGrowingUpTests/Helpers/UserCases/AddPersonUseCaseSpy.swiftGrowingUpTests/Helpers/UserCases/DisplayPersonsUseCaseSpy.swiftGrowingUpTests/Helpers/UserCases/EditPersonUseCaseSpy.swiftGrowingUpTests/Helpers/UserCases/FetchPersonsUseCaseSpy.swiftGrowingUpTests/Helpers/UserCases/RemovePersonUseCaseSpy.swiftGrowingUpTests/Presenters/AddPersonPresenterTests.swiftGrowingUpTests/Presenters/EditPersonPresenterTests.swiftGrowingUpTests/Presenters/EmptyPersonPresenterTests.swiftGrowingUpTests/Presenters/PersonsListPresenterTests.swiftGrowingUpTests/Presenters/TextFieldCellPresenterTests.swiftGrowingUpTests/UseCases/AddPersonUseCaseTests.swiftGrowingUpTests/UseCases/EditPersonUseCaseTests.swiftGrowingUpTests/UseCases/FetchPersonsUseCaseTests.swiftGrowingUpTests/UseCases/RemovePersonUseCaseTests.swiftWidget/AgeWidgetProvider.swift
💤 Files with no reviewable changes (9)
- Core/UseCases/AddPersonUseCase.swift
- GrowingUpTests/Helpers/UserCases/RemovePersonUseCaseSpy.swift
- GrowingUpTests/Helpers/UserCases/FetchPersonsUseCaseSpy.swift
- GrowingUpTests/Helpers/UserCases/DisplayPersonsUseCaseSpy.swift
- Core/UseCases/EditPersonUseCase.swift
- Core/UseCases/FetchPersonsUseCase.swift
- Core/Gateways/PersonsGateway.swift
- Core/UseCases/RemovePersonUseCase.swift
- Core/EntityGateway/Cache/CachePersonsGateway.swift
1 participant
Summary
PersonsGatewayand use-case contracts async-onlyTaskusage with main-actor UI updatesStack
Depends on #27. Part 5 of the native GitHub stack rooted at #8.
Checks
scripts/check-formatting.shscripts/check-secrets.shgit diff --checkSummary by CodeRabbit