MS-1148 Sync state use case simplification spike#1495
Conversation
7391149 to
bdd4b1c
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the sync state management code to improve readability and maintainability. The main changes involve extracting configuration observation logic into a dedicated use case, simplifying the Ticker component, removing unused code, and streamlining test setup.
Key Changes
- Extracted
ObserveConfigurationChangesUseCase: Configuration management logic (project state, module counts, tokenization) has been moved into a separate use case, reducing complexity inObserveSyncInfoUseCase - Simplified
Tickercomponent: Converted from an interface-implementation pattern to a concrete class with a more flexible API that accepts anyDurationinstead of being hardcoded to one minute - Removed unused code: Eliminated the
combine9Flow extension and associated tests, along with redundant module tokenization tests
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
infra/core/src/test/java/com/simprints/core/tools/time/TickerTest.kt |
Updated tests to work with new Ticker concrete class and observeTicks(Duration) method |
infra/core/src/test/java/com/simprints/core/tools/extentions/FlowExtTest.kt |
Removed test for unused combine9 function |
infra/core/src/test/java/com/simprints/core/lifecycle/AppForegroundStateTrackerTest.kt |
Updated test to inject dispatcher dependency |
infra/core/src/main/java/com/simprints/core/tools/time/TickerImpl.kt |
Removed implementation class as Ticker is now a concrete class |
infra/core/src/main/java/com/simprints/core/tools/time/Ticker.kt |
Converted from interface to concrete class with flexible observeTicks(Duration) method |
infra/core/src/main/java/com/simprints/core/tools/extentions/Flow.ext.kt |
Removed unused combine9 extension function |
infra/core/src/main/java/com/simprints/core/lifecycle/AppForegroundStateTracker.kt |
Added main dispatcher injection and flowOn operator for thread safety |
infra/core/src/main/java/com/simprints/core/CoreModule.kt |
Removed provideTicker() method as Ticker now uses constructor injection |
feature/dashboard/src/test/java/com/simprints/feature/dashboard/settings/syncinfo/usecase/ObserveSyncInfoUseCaseTest.kt |
Extensive refactoring to use new ObserveConfigurationChangesUseCase and helper methods for test setup |
feature/dashboard/src/test/java/com/simprints/feature/dashboard/settings/syncinfo/usecase/ObserveConfigurationChangesUseCaseTest.kt |
New test file for the extracted configuration observation use case |
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/usecase/ObserveSyncInfoUseCase.kt |
Refactored to use extracted configuration use case, simplified progress calculation, improved code organization |
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/usecase/ObserveConfigurationChangesUseCase.kt |
New use case that encapsulates configuration observation, module counting, and tokenization logic |
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/modulecount/ModuleCountViewHolder.kt |
Updated to display localized "Total Records" text for the first item instead of an empty string |
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/SyncInfoFragment.kt |
Simplified adapter submission by directly using ModuleCount instead of converting from SyncInfoModuleCount |
feature/dashboard/src/main/java/com/simprints/feature/dashboard/settings/syncinfo/SyncInfo.kt |
Changed visibility to internal, removed SyncInfoModuleCount in favor of ModuleCount, simplified data structures |
Comments suppressed due to low confidence (1)
infra/core/src/test/java/com/simprints/core/tools/time/TickerTest.kt:3
- Avoid using wildcard imports (
import com.google.common.truth.Truth.*). Use explicit imports instead (e.g.,import com.google.common.truth.Truth.assertThat) to improve code readability and avoid potential naming conflicts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Showing only event sync progress | ||
| eventSyncState.isSyncInProgress() && totalEvents > 0 -> eventProgressProportion | ||
|
|
||
| // Sync haven't started |
There was a problem hiding this comment.
Grammar error in comment: "Sync haven't started" should be "Sync hasn't started".
| // Sync haven't started | |
| // Sync hasn't started |
| return@combine syncInfo | ||
| }.onRecordSyncComplete { delay(timeMillis = SYNC_COMPLETION_HOLD_MILLIS) } | ||
| .onImageSyncComplete { delay(timeMillis = SYNC_COMPLETION_HOLD_MILLIS) } | ||
| .flowOn(dispatcher) // upstream flows does a lot of computation |
There was a problem hiding this comment.
The comment should be updated to use correct grammar: "upstream flows does" should be "upstream flows do".
| .flowOn(dispatcher) // upstream flows does a lot of computation | |
| .flowOn(dispatcher) // upstream flows do a lot of computation |
bdd4b1c to
bbef1ca
Compare
|



JIRA ticket
Unfortunately, I was unable to find a way to implement any meaningful changes on a larger scale. The business logic part does exactly what it should, and while it is a bit verbose, there are a lot of cases to cover. Additional simplification may arise from architectural changes in the data layer/sync management, but this remains to be seen.
Notable changes
Testing guidance
Additional work checklist