Refactor OnstartAsyncService with modular runners #1539
Conversation
Refactors OnstartAsyncService to use a modular runner-based architecture, moving startup tasks into individual OnstartAsyncRunnable types. Adds ReleaseAlertService and updates RootSceneViewModel to use new services for release alerts and app rating. Updates service factories, dependency injection, and tests to support the new structure, improving maintainability and testability.
Summary of ChangesHello @gemdev111, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural refactor of the application's asynchronous startup processes. By transitioning to a modular, runner-based system, the PR aims to enhance the organization, testability, and maintainability of the app's initial setup routines. It also integrates a new service for managing app update alerts, ensuring users are promptly informed about new releases. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the application's startup logic. Moving to a modular, runner-based architecture for OnstartAsyncService greatly improves separation of concerns, testability, and maintainability. The extraction of logic into ReleaseAlertService and the cleanup of various services are also excellent changes that simplify the view model and other components.
My review includes a critical fix for a potential issue with optional handling in the new ReleaseAlertService, along with its corresponding update at the call site. I've also included a medium-severity suggestion to improve dependency injection in the ServicesFactory.
Overall, this is a high-quality refactoring that sets a good foundation for future startup tasks.
| public func checkForUpdate() async -> Release? { | ||
| guard let release = try? await appReleaseService.getNewestRelease() else { return nil } | ||
| guard preferences.skippedReleaseVersion != release.version else { return nil } | ||
| return release | ||
| } |
There was a problem hiding this comment.
This function has a potential issue with double optionals and error handling. If appReleaseService.getNewestRelease() returns Release? and can throw, then try? ... results in Release??. The guard let only unwraps the outer optional, leaving release as Release?, which would cause a compile error on release.version.
Additionally, using try? silences important errors (like network issues), making debugging harder. It's better to let the function throw and handle errors at the call site.
| public func checkForUpdate() async -> Release? { | |
| guard let release = try? await appReleaseService.getNewestRelease() else { return nil } | |
| guard preferences.skippedReleaseVersion != release.version else { return nil } | |
| return release | |
| } | |
| public func checkForUpdate() async throws -> Release? { | |
| guard let release = try await appReleaseService.getNewestRelease() else { return nil } | |
| guard preferences.skippedReleaseVersion != release.version else { return nil } | |
| return release | |
| } |
| private func checkForUpdate() async { | ||
| guard let release = await releaseAlertService.checkForUpdate() else { return } | ||
| updateVersionAlertMessage = makeUpdateAlert(for: release) | ||
| } |
There was a problem hiding this comment.
Following the proposed change in ReleaseAlertService.checkForUpdate() to make it a throwing function, this call site should be updated to handle potential errors, for instance by logging them. This improves error diagnostics for failures in checking for app updates.
private func checkForUpdate() async {
do {
guard let release = try await releaseAlertService.checkForUpdate() else { return }
updateVersionAlertMessage = makeUpdateAlert(for: release)
} catch {
debugLog("Failed to check for app update: \(error)")
}
}| assetsService: assetsService, | ||
| deviceService: deviceService, | ||
| bannerSetupService: bannerSetupService, | ||
| assetStore: assetsService.assetStore, |
There was a problem hiding this comment.
The assetStore is being passed to ImportAssetsService here, but assetsService (passed on the previous line) already contains a public assetStore property. This is redundant. For cleaner dependency injection, consider refactoring ImportAssetsService to only take assetsService and access the store via assetsService.assetStore. This would simplify its initialization here.
| for runner in runners { | ||
| do { | ||
| try await runner.run(config: config) | ||
| } catch { | ||
| debugLog("\(type(of: runner)) failed: \(error)") | ||
| } | ||
| } |
There was a problem hiding this comment.
The runners are currently executed sequentially. Since many of these startup tasks appear to be independent, you could run them in parallel using a TaskGroup to potentially speed up the app startup process. This would execute all startup tasks concurrently.
Before making this change, please ensure that runners accessing shared resources (like assetStore) are thread-safe or are grouped to execute sequentially to avoid race conditions.
await withTaskGroup(of: Void.self) { group in
for runner in runners {
group.addTask {
do {
try await runner.run(config: config)
} catch {
debugLog("\(type(of: runner)) failed: \(error)")
}
}
}
}
| import DeviceService | ||
| import Primitives | ||
|
|
||
| public struct DeviceUpdateRunner: OnstartAsyncRunnable { |
There was a problem hiding this comment.
OnstartAsyncRunnable should be generic protocol like JobRunnable
| } | ||
|
|
||
| public func run(config: ConfigResponse?) async throws { | ||
| try await deviceService.update() |
There was a problem hiding this comment.
I don't think we need DeviceUpdateRunner
Combine them? |
Device update functionality is now handled directly in RootSceneViewModel using DeviceService, eliminating the need for DeviceUpdateRunner and its test kit. Updated dependency injection and service factory accordingly to streamline device update handling.
…etsRunner Merge two related runners into a single runner that: - Sets swappable assets based on supported chains (always) - Updates swap assets from API when version changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replaces OnstartAsyncRunnable protocol with AsyncRunnable, consolidates FiatAssetsUpdateRunner and SwapAssetsRunner into a single AssetsUpdateRunner, and updates OnstartAsyncService to use the new runner protocol. Updates related test kits and tests to reflect these changes, improving runner extensibility and simplifying asset update logic.
Deleted the runnersExecuteInOrder test and its associated helper classes from OnstartAsyncServiceTests.swift. This simplifies the test suite by removing order tracking logic.
Improvements:
closes #1538