Improve Component testability + add example tests #125
Conversation
…ement and event handling - Rename componentCoroutineScope -> lifecycleScope - Allow overriding lifecycleScope and useCaseDispatcher
…or unified coroutine management with FlowUseCaseExecutionScope. Use runCaching in asyncBuilder to prevent failing under TestScope.
WalkthroughThis set of changes introduces significant refactoring and improvements across the shared use case architecture, UI components, and testing infrastructure. The use case base classes ( Sequence Diagram(s)sequenceDiagram
participant UI
participant FirstComponent
participant FetchDataUseCase
participant CounterUseCase
participant Navigation
UI->>FirstComponent: Lifecycle Created
FirstComponent->>FetchDataUseCase: execute(Unit)
FetchDataUseCase-->>FirstComponent: Person (success) or Throwable (failure)
FirstComponent->>FirstComponent: Update state with person or error
loop Counter Emission
FirstComponent->>CounterUseCase: build(Args)
CounterUseCase-->>FirstComponent: Flow<Long>
FirstComponent->>FirstComponent: Update counter in state
alt Counter == ALERT_THRESHOLD
FirstComponent->>FirstComponent: Emit FirstUiEvent.Notify(count)
end
end
UI->>FirstComponent: User taps Next
FirstComponent->>Navigation: navigateToSecond()
Possibly related PRs
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/picker/FruitPickerComponent.kt (1)
26-27: Same immutability concern as inLoginComponentSee the remark in
LoginComponent.kt. The argument applies identically here.
🧹 Nitpick comments (16)
iosApp/iosApp/Views/Screen/First/FirstView.swift (1)
23-25: Avoid variable shadowing inside theswitchRe-using the name
eventfor the bound enum payload obscures the outer parameter and hampers readability.- case .notify(let event): - viewModel.notify(event: event) + case .notify(let notifyEvent): + viewModel.notify(event: notifyEvent)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestUseCase.kt (1)
6-11: 1-second delay in a test use-case slows the suite
delay(1000)is expensive for unit tests and integration tests that may execute this use-case multiple times. Consider drastically shortening the delay or injecting it as a parameter so it can be set to0L/10Lin tests while retaining realism elsewhere.- delay(1000) + // Keep fast in tests, realistic in production if needed + delay(10)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/UseCaseExecutionScope.kt (1)
27-41: Consider returning the launchedJobfor better test control
launchWithHandlernow launches inuseCaseScope—good move.
Exposing the returnedJobwould let callers cancel or join it in unit tests (or production) without needing to re-implement similar logic.- fun launchWithHandler(block: suspend CoroutineScope.() -> Unit) { - useCaseScope.launch { + fun launchWithHandler(block: suspend CoroutineScope.() -> Unit): Job { + return useCaseScope.launch { try { block()This is a non-breaking additive change that improves observability and composability.
shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/UseCaseExecutionScopeTest.kt (2)
302-308: Remove redundant@OptInannotationThe whole class is already annotated with
@OptIn(ExperimentalCoroutinesApi::class)(line 25).
Keeping the same annotation at the helper-method level provides no additional value and clutters the file.- @OptIn(ExperimentalCoroutinesApi::class) private fun TestScope.advanceTimeByCompat(delayTimeMillis: Long) {
75-78: PreferlistOfoverlistOfNotNullwhen the arguments are non-nullable
listOfNotNullis meant for constructing lists while automatically skipping nulls.
Here every literal is anInt, which is already non-nullable, so the call is semantically identical tolistOfbut slightly less clear.-val testingList = listOfNotNull(0, 1, 2, 3, 4, 5, 6, 7, 8, 9) +val testingList = listOf(0, 1, 2, 3, 4, 5, 6, 7, 8, 9)Also applies to: 96-99
androidApp/src/main/kotlin/app/futured/kmptemplate/android/ui/screen/FirstScreenUi.kt (1)
82-88: Minor Compose optimisation
AnimatedVisibilityrecomposes every timerandomPersonTextchanges.
You can avoid the nullable check duplication by using the value directly:-AnimatedVisibility(viewState.randomPersonText != null) { - viewState.randomPersonText?.let { person -> +val person = viewState.randomPersonText +AnimatedVisibility(person != null) { + person?.let {Purely a readability tweak; feel free to skip.
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt (1)
99-105: Duplicate logic – extract helperThe same execute logic is duplicated in both overloads; extracting a private
launchWithConfig(...)would reduce maintenance cost and guarantee identical behaviour.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.kt (1)
24-24: Dispatcher parameter naming drifts from the rest of the codebaseHere it’s called
workerDispatcher, elsewhere (BaseComponent, PR description) it’suseCaseDispatcher. Pick one term and stick to it to avoid confusion.Also applies to: 42-42
shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/base/BaseUseCaseExecutionScopeTest.kt (1)
32-33: ConsideradvanceUntilIdle()before cancellingCalling
advanceUntilIdle()(orrunCurrent()) prior touseCaseScope.cancel()helps surface uncompleted jobs/flaky tests and mirrors the behaviour ofrunTest {}.shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.kt (2)
61-63: Hard-codedDispatchers.Mainbreaks non-Android targets
Dispatchers.Mainisn’t available on some platforms unless extra dependencies are provided. UsewithContext(Dispatchers.Main.immediate)behind a platform check or expose a UI dispatcher dependency instead.
98-101: Map value declared asJob, storedDeferred<Result<T>>
Deferredis aJob, but the added generics can be misleading when later casting/retrieving. Consider changing the map toMutableMap<Any, Deferred<Result<*>>>or a dedicated wrapper type for clarity.iosApp/iosApp/Views/Screen/First/FirstViewModel.swift (1)
57-60: Method name clashes with toast terminology
notify(event:)setsalertText/alertVisible, yet the paired clearer is still calledhideToast(). Consider renaminghideToast()(andisAlertVisible) to match the new “notify / alert” wording to avoid confusion during future maintenance.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponent.kt (2)
27-29: Potential scope explosionEach component instantiation creates a fresh
MainScope(). In navigation-heavy flows this can multiply supervisor jobs and pressure the main dispatcher.
Pass in a shared/lifecycle–aware scope (e.g.,componentContext.mainScopeor DI-provided) and reserveMainScope()only as a last-ditch default.
64-72: Event lost if counter overshoots
Notifyis sent only oncount == 30; if the flow emits 30 + several values quickly the collector may miss 30 due to back-pressure suspension (see BaseComponent note).
Consider>= COUNTER_ALERTcheck or usingdistinctUntilChanged()on a boolean flag.shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponentTest.kt (2)
38-43: DirectTurbineinstantiation is unstable
Turbine()is an internal implementation detail; prefer the publicturbineScope {}helper orChannel/MutableSharedFlowfor recording calls to avoid future breaking changes in the library.
131-132: Implicitithurts clarityInside nested lambdas
it.intervalrelies on the outer lambda’sArgs. Introduce a named variable (args.interval) to avoid accidental shadowing and improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
androidApp/src/main/kotlin/app/futured/kmptemplate/android/ui/screen/FirstScreenUi.kt(3 hunks)gradle/libs.versions.toml(2 hunks)iosApp/iosApp/Views/Screen/First/FirstView.swift(1 hunks)iosApp/iosApp/Views/Screen/First/FirstViewModel.swift(3 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCase.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCase.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/CoroutineScopeOwner.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt(6 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.kt(3 hunks)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/UseCaseExecutionScope.kt(2 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/UseCaseExecutionScopeTest.kt(16 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/base/BaseUseCaseExecutionScopeTest.kt(2 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureFlowUseCase.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureUseCase.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFlowUseCase.kt(1 hunks)shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestUseCase.kt(1 hunks)shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/presentation/BaseComponent.kt(3 hunks)shared/feature/build.gradle.kts(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/CounterUseCase.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/FetchDataUseCase.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostComponent.kt(2 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/_template/_TemplateScreen.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.kt(3 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponent.kt(3 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstEvent.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/loginScreen/LoginComponent.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/picker/FruitPickerComponent.kt(1 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/picker/VegetablePickerComponent.kt(1 hunks)shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponentTest.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
iosApp/iosApp/Views/Screen/First/FirstView.swift (1)
iosApp/iosApp/Views/Screen/First/FirstViewModel.swift (1)
notify(57-60)
androidApp/src/main/kotlin/app/futured/kmptemplate/android/ui/screen/FirstScreenUi.kt (1)
shared/arkitekt-decompose/src/androidMain/kotlin/app/futured/arkitekt/decompose/event/EventsEffect.kt (1)
onEvent(38-42)
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt (1)
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCase.kt (2)
build(9-16)build(15-15)
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.kt (3)
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/CoroutineScopeOwner.kt (1)
getWorkerDispatcher(17-17)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCase.kt (2)
build(9-16)build(15-15)shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCase.kt (2)
build(8-14)build(13-13)
iosApp/iosApp/Views/Screen/First/FirstViewModel.swift (1)
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.kt (1)
counter(11-32)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-ios / Test
- GitHub Check: check-android / Lint & Tests
🔇 Additional comments (16)
shared/feature/build.gradle.kts (1)
66-69: New test-only dependencies look correctThe additions of
kotlinx.coroutines.testandturbinein thecommonTestconfiguration are spot-on for coroutine & Flow testing. No further action required.gradle/libs.versions.toml (1)
42-43: Turbine version catalog entry LGTM
1.2.1is the latest stable release and the coordinateapp.cash.turbine:turbineis correct. 👍Also applies to: 108-108
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/CoroutineScopeOwner.kt (1)
12-12:✅ Verification successful
Rename ripple check
The property was renamed to
useCaseScope. Ensure all implementers and call-sites were updated; otherwise, compilation will break silently in downstream modules not included in this PR.
🏁 Script executed:
#!/bin/bash # Find lingering references to the old property name rg -n "viewModelScope" --type kotlinLength of output: 36
No lingering
viewModelScopereferences foundI ran a project-wide search for the old property name and found no occurrences in Kotlin sources. The rename to
useCaseScopecan be considered complete.shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFlowUseCase.kt (1)
9-9: Interface migration acknowledgedConstructor invocation removed in accordance with
FlowUseCasebecoming an interface. Implementation remains sound.shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureFlowUseCase.kt (1)
7-12: Inheritance switch is correct and consistentThe class now correctly implements the
FlowUseCaseinterface without invoking a constructor, matching the new interface-based design. No further issues spotted here.shared/arkitekt-cr-usecases/src/commonTest/kotlin/app/futured/arkitekt/crusecases/usecases/TestFailureUseCase.kt (1)
5-8: Interface implementation looks goodThe change mirrors the
UseCaserefactor and compiles fine. No additional concerns.shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/FlowUseCase.kt (1)
9-16: Interface rewrite LGTMConverting
FlowUseCaseto a stateless interface removes hidden mutable state and simplifies testing. No further action required.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/loginScreen/LoginComponent.kt (1)
23-25: Expose only an immutableStateFlow
componentStateis most likely aMutableStateFlow.
Assigning it directly is fine as long as its type in the base class is not publicly mutable.
Please double-check the declaration ofcomponentStateinScreenComponent– it should be:protected val componentState: MutableStateFlow<S>If it happens to be
publicorinternal, external code could cast back toMutableStateFlowand mutate state bypassing the component. In that case keepasStateFlow()to guarantee read-only exposure.androidApp/src/main/kotlin/app/futured/kmptemplate/android/ui/screen/FirstScreenUi.kt (1)
53-56: Event handling looks goodSwitching to
FirstUiEvent.Notifytogether withonEventkeeps the event-filtering concise and type-safe.
No issues spotted here.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/picker/VegetablePickerComponent.kt (1)
27-27:viewStateexposure looks goodRemoving
asStateFlow()eliminates an unnecessary allocation. BecausecomponentStateis aMutableStateFlowand you keep the property typed asStateFlow, callers still get a read-only view, so no encapsulation is lost.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstEvent.kt (1)
8-13: Re-checkinternalvisibility ofcount
countis declaredinternal, so it is invisible to other Kotlin modules (e.g., Android/iOS UI layers) that may share the:featureartifact. If those layers need to format, analyse, or log the raw value, they will be forced to rely on the pre-formattedtext.
Please verify that no external consumer needs direct access; otherwise remove theinternalmodifier.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/_template/_TemplateScreen.kt (1)
44-44: Minor but solid clean-upSame comment as for the picker: dropping
asStateFlow()removes overhead while the public type remains immutable.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/navigation/signedIn/SignedInNavHostComponent.kt (2)
28-34: Good move to a singleDEFAULT_STATEconstantCentralising the initial state prevents accidental divergence across constructors. 👍
69-78: Switched tolifecycleScope– double-check lifetime
stateIn(lifecycleScope, SharingStarted.Lazily, …)ties the upstream flow to the component’s lifecycle rather than the previouscomponentCoroutineScope.
EnsurelifecycleScopereally lives as long as the component stack; otherwise the shared flow may cancel sooner than expected.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.kt (1)
23-26: DefaultMainScope()risks leaks if not cancelledCreating a fresh
MainScope()for every component is fine only when the scope is guaranteed to be cancelled inonDestroy(). Double-check thatBaseComponentreally invokeslifecycleScope.cancel(); otherwise each component keeps a liveSupervisorJoband the app will leak coroutines.shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/CounterUseCase.kt (1)
12-17: Interface refactor looks goodThe switch to a
fun interfacewith nestedArgscleans up the public surface nicely.
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/UseCase.kt
Show resolved
Hide resolved
| /** | ||
| * Map of [Job] objects used to hold and cancel existing run of any [FlowUseCase] instance. | ||
| */ | ||
| val useCaseJobPool: MutableMap<Any, Job> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak & thread-safety in useCaseJobPool
Jobs are stored but never removed; finished or cancelled jobs will accumulate in the map for the lifetime of the scope.
Additionally, MutableMap isn’t thread-safe – concurrent executions on different dispatchers may race.
@@
- val useCaseJobPool: MutableMap<Any, Job>
+ val useCaseJobPool: MutableMap<Any, Job> // consider ConcurrentHashMap on JVM / atomic wrapper on native🤖 Prompt for AI Agents
In
shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt
between lines 17 and 21, the useCaseJobPool map stores Job objects but never
removes finished or cancelled jobs, causing a potential memory leak. Also,
MutableMap is not thread-safe, risking race conditions during concurrent access.
To fix this, replace the MutableMap with a thread-safe concurrent map
implementation and ensure that jobs are removed from the map once they complete
or are cancelled to prevent accumulation.
There was a problem hiding this comment.
Can you elaborate with concrete examples?
There was a problem hiding this comment.
@matejsemancik, let me provide concrete examples of both issues:
Memory Leak Example
Consider a screen component that executes a data refresh use case multiple times:
class DataScreenComponent : BaseComponent<DataViewState, DataEvent>() {
private val refreshDataUseCase = RefreshDataUseCase()
fun onRefreshClicked() {
// This gets called every time user pulls to refresh
refreshDataUseCase.execute {
onNext { data -> updateState { copy(data = data) } }
}
}
}What happens:
- First refresh:
useCaseJobPool[refreshDataUseCase] = Job1 - Second refresh:
useCaseJobPool[refreshDataUseCase] = Job2(Job1 gets cancelled but stays in memory) - Third refresh:
useCaseJobPool[refreshDataUseCase] = Job3(Job1, Job2 still in memory) - After 100 refreshes: Map contains 1 active job + 99 cancelled jobs consuming memory
The cancelled jobs can't be garbage collected because the map holds references to them.
Thread Safety Example
Consider concurrent execution from different coroutine contexts:
class ConcurrentComponent : BaseComponent<ViewState, Event>() {
private val useCase1 = NetworkUseCase()
private val useCase2 = DatabaseUseCase()
fun loadData() {
// Both launched simultaneously on different dispatchers
launch(Dispatchers.IO) {
useCase1.execute { /* ... */ } // Thread A
}
launch(Dispatchers.Main) {
useCase2.execute { /* ... */ } // Thread B
}
}
}Race condition scenario:
// Thread A executing useCase1.execute():
if (flowUseCaseConfig.disposePrevious) {
useCaseJobPool[this]?.cancel() // Read operation
// Context switch happens here!
}
useCaseJobPool[this] = newJob // Write operation
// Thread B executing useCase2.execute() at the same time:
if (flowUseCaseConfig.disposePrevious) {
useCaseJobPool[this]?.cancel() // Read operation
}
useCaseJobPool[this] = anotherJob // Write operationThis can lead to:
- Lost updates: One thread's job overwrites another's
- Inconsistent state: Reading while another thread is writing
- Map corruption: On some platforms, concurrent writes can corrupt the internal map structure
Real-world Impact
Memory leak in long-running app:
// A chat screen that refreshes messages every 30 seconds
class ChatComponent {
fun startPeriodicRefresh() {
repeat(Int.MAX_VALUE) {
delay(30_000)
messageUseCase.execute { /* update messages */ }
// After 24 hours: ~2,880 cancelled jobs in memory
}
}
}Solutions:
- For memory leaks - Clean up completed jobs:
useCaseJobPool[this] = build(args)
.onCompletion {
useCaseJobPool.remove(this@execute) // Clean up when done
}
.launchIn(useCaseScope)- For thread safety - Use concurrent collections:
// JVM
val useCaseJobPool: ConcurrentHashMap<Any, Job> = ConcurrentHashMap()
// Multiplatform alternative
val useCaseJobPool: MutableMap<Any, Job> = Collections.synchronizedMap(mutableMapOf())There was a problem hiding this comment.
@coderabbitai Bro, it's a Map. How does map keep reference to old canceled jobs, when each execution of UseCase cancels and overwrites the job in the map?
There was a problem hiding this comment.
@matejsemancik You're absolutely right, and I apologize for the confusion! 🤦♂️
You've correctly identified the flaw in my memory leak analysis. Let me trace through what actually happens:
- First execution:
useCaseJobPool[useCase] = Job1 - Second execution:
useCaseJobPool[useCase]?.cancel()→ cancels Job1useCaseJobPool[useCase] = Job2→ overwrites the reference to Job1
Once Job1's reference is overwritten, it becomes eligible for garbage collection. The map doesn't accumulate old jobs - it replaces them. Each UseCase instance can only hold one Job reference at a time.
My memory leak concern was incorrect.
The thread safety concern could still be relevant, but only in the specific scenario where the same UseCase instance is executed concurrently from different threads:
// Thread A and Thread B both calling useCase.execute() simultaneously
// Thread A: useCaseJobPool[useCase]?.cancel()
// Thread B: useCaseJobPool[useCase]?.cancel() (might cancel Thread A's new job)
// Thread A: useCaseJobPool[useCase] = jobA
// Thread B: useCaseJobPool[useCase] = jobB (overwrites jobA)But in practice, this scenario is probably rare since components typically execute use cases in a coordinated manner.
Thanks for catching that - the map behavior is much cleaner than I initially analyzed!
🐰
(\ /)
( ._. )
o_(")(") oops, learned something!
✏️ Learnings added
Learnt from: matejsemancik
PR: futuredapp/kmp-futured-template#125
File: shared/arkitekt-cr-usecases/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt:17-21
Timestamp: 2025-06-20T08:58:15.336Z
Learning: The useCaseJobPool map in FlowUseCaseExecutionScope overwrites job references for each UseCase instance rather than accumulating them, so cancelled jobs don't cause memory leaks as they become eligible for garbage collection when their references are replaced.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
...ses/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/FlowUseCaseExecutionScope.kt
Show resolved
Hide resolved
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/FetchDataUseCase.kt
Show resolved
Hide resolved
...s/src/commonMain/kotlin/app/futured/arkitekt/crusecases/scope/SingleUseCaseExecutionScope.kt
Show resolved
Hide resolved
| private fun syncData() = fetchDataUseCase.execute { | ||
| onSuccess { person -> | ||
| componentState.update { it.copy(randomPerson = MR.strings.first_screen_random_person.format(person.name.orEmpty())) } | ||
| componentState.update { it.copy(randomPerson = person) } | ||
| } | ||
| onError { error -> | ||
| componentState.update { it.copy(randomPerson = MR.strings.first_screen_random_person.format("Failed to fetch")) } | ||
| logger.e(error) { error.message.toString() } | ||
| componentState.update { it.copy(randomPersonError = error) } | ||
| } |
There was a problem hiding this comment.
State gets out of sync after success / error flips
On success you overwrite randomPerson but leave a previous randomPersonError; on error you set the error but keep the last randomPerson.
Clear the opposite field to keep state mutually exclusive:
- onSuccess { person ->
- componentState.update { it.copy(randomPerson = person) }
+ onSuccess { person ->
+ componentState.update { it.copy(randomPerson = person, randomPersonError = null) }
}
onError { error ->
- componentState.update { it.copy(randomPersonError = error) }
+ componentState.update { it.copy(randomPerson = null, randomPersonError = error) }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun syncData() = fetchDataUseCase.execute { | |
| onSuccess { person -> | |
| componentState.update { it.copy(randomPerson = MR.strings.first_screen_random_person.format(person.name.orEmpty())) } | |
| componentState.update { it.copy(randomPerson = person) } | |
| } | |
| onError { error -> | |
| componentState.update { it.copy(randomPerson = MR.strings.first_screen_random_person.format("Failed to fetch")) } | |
| logger.e(error) { error.message.toString() } | |
| componentState.update { it.copy(randomPersonError = error) } | |
| } | |
| private fun syncData() = fetchDataUseCase.execute { | |
| onSuccess { person -> | |
| componentState.update { | |
| it.copy( | |
| randomPerson = person, | |
| randomPersonError = null | |
| ) | |
| } | |
| } | |
| onError { error -> | |
| componentState.update { | |
| it.copy( | |
| randomPerson = null, | |
| randomPersonError = error | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponent.kt
around lines 55 to 61, the state update on success only sets randomPerson but
does not clear randomPersonError, and on error only sets randomPersonError but
does not clear randomPerson. Modify the onSuccess block to set randomPerson and
clear randomPersonError, and modify the onError block to set randomPersonError
and clear randomPerson, ensuring the state fields are mutually exclusive and do
not retain stale data.
...ature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstViewState.kt
Show resolved
Hide resolved
...decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/presentation/BaseComponent.kt
Outdated
Show resolved
Hide resolved
shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/domain/CounterUseCase.kt
Show resolved
Hide resolved
| override fun FirstComponent.navigateToSecond() { | ||
| calls.add(HomeConfig.Second) | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn’t mockK be better here? Something like
val fakeNavigation = mockk<FirstScreenNavigation>(relaxed = true)
would be enough. It’ll be easier to maintain than writing everything out every time like this.
There was a problem hiding this comment.
Mockk.io does not support Kotlin native targets (only JVM and Android).
Mocking in KMP generally comes with some limitations stemming from limited relection on native targets. See for example https://mokkery.dev/ and known limitations.
In general, you have several options if you wanna fake UseCases
- Keep usecases as final classes, you can use https://mokkery.dev/ or similar library, but you are forced to make all UseCase classes
open, or apply all-open compiler plugin to them. - Hide UseCase implementations behind interface and write fakes.
- Hide UseCase implementations behind interface and use https://mokkery.dev/ or similar library, however Mokkery now works out of the box because UseCase is an interface instead of final class.
There was a problem hiding this comment.
I see, tricky. But it make sense the way you did it. But I would suggest to do fun interface only when there is a need to use it within tests
There was a problem hiding this comment.
I now realize this comment references something else, however my comment above summarizes my motivations behind this PR.
In specific case of this navigator - yes using Mokkery would be easier, as we could just do something like this
val navigation = mock<FirstScreenNavigation>()
verify { navigation.navigateToSecond() }however the navigateToSecond is extension function over FirstComponent and I haven't tried verifying extension function calls using Mokkery yet, so we would need to try what's possible
| fun cleanup() { | ||
| lifecycleRegistry.destroy() | ||
| Dispatchers.resetMain() | ||
| } |
There was a problem hiding this comment.
@BeforeTest and @AfterTest will repeat a lot in these types of tests. It would be better to create something like
object ComponentTestPreparation {
fun setup(...) { ....}
....
}
...e/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponentTest.kt
Show resolved
Hide resolved
|
Was the point of this MR to improve testability of KMP modules because if yes then I would suggest to focus on "end-to-end"/"integrations" type kind of tests where there is created component with with real usecases, real stores and only external and system services are faked (or mocked but fake is better in this case). If we want to do testing more robust then it is much better strategy in my opinion. However, this is a step in a good direction and these two things are not mutually exclusive. |
|
@SumeraMartin The main point was to allow faking/mocking UseCases without doin |
matejsemancik
changed the title
|
I just wanna say that I'm not opposed to idea of mocking using for example https://mokkery.dev/, however in both cases (faking vs. mocking) it is ideal to have UseCases implemented as interfaces for reasons stated above |
…on for standardized UI component testing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTestPreparation.kt (1)
71-71: Consider test isolation implications of global error handler modification.Setting the global error handler to an empty lambda could potentially affect other tests if they run concurrently or if the handler isn't properly restored. While this is likely acceptable in the testing context, consider documenting this behavior or using a more isolated approach if test interference becomes an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/presentation/BaseComponent.kt(3 hunks)shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.kt(3 hunks)shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTest.kt(1 hunks)shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTestPreparation.kt(1 hunks)shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponentTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- shared/feature/src/commonMain/kotlin/app/futured/kmptemplate/feature/ui/base/BaseComponents.kt
- shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/firstScreen/FirstComponentTest.kt
- shared/arkitekt-decompose/src/commonMain/kotlin/app/futured/arkitekt/decompose/presentation/BaseComponent.kt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-android / Lint & Tests
- GitHub Check: check-ios / Test
🔇 Additional comments (4)
shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTest.kt (2)
1-96: Excellent interface design for component testing infrastructure!This interface provides a well-structured foundation for testing UI components in the Decompose architecture. The design demonstrates several best practices:
- Clear separation of testing concerns with dedicated properties for scope, dispatcher, lifecycle, and context
- Proper lifecycle management with
@BeforeTestand@AfterTestannotations- Convenient extension function
runComponentTestthat simplifies test execution- Comprehensive documentation explaining the purpose and usage
- Reasonable default timeout configuration
The interface will effectively standardize component testing across the codebase and make it easier to write consistent, maintainable tests.
1-95: LGTM! Excellent interface design for component testing.The
ComponentTestinterface provides a clean and well-designed contract for testing UI components. The interface properly abstracts testing concerns and includes:
- Clear property definitions for test infrastructure
- Proper lifecycle management with BeforeTest/AfterTest annotations
- Convenient extension function for running tests
- Comprehensive documentation with usage examples
The 60-second default timeout is reasonable for component tests, and the overall API design promotes consistency across component test implementations.
shared/feature/src/commonTest/kotlin/app/futured/kmptemplate/feature/ui/base/ComponentTestPreparation.kt (2)
1-87: Solid implementation of the component testing infrastructure!This implementation provides a robust foundation for component testing with excellent attention to detail:
- Proper coroutine test setup: Uses
TestScopewithStandardTestDispatcherfor deterministic test execution- Clean lifecycle management: Creates and properly destroys
LifecycleRegistryto avoid resource leaks- Isolated test environment: Configures
Dispatchers.Mainduring tests and resets afterward to prevent test interference- Appropriate error handling: Disables
UseCaseErrorHandlerlogging during tests to reduce noise- Integration with Decompose: Correctly sets up
DefaultAppComponentContextwith proper component contextThe setup and cleanup methods follow testing best practices by ensuring each test starts with a clean state and properly disposes of resources afterward.
1-86: LGTM! Well-structured component test preparation class.The implementation is clean and follows good testing practices for Decompose-based UI components. The class properly manages test resources with appropriate setup/cleanup methods.
Generated by 🚫 Danger |
componentCoroutineScopetolifecycleScopefor better claritylifecycleScopeanduseCaseDispatcherfor overriding (this is needed to make tests work)FirstComponentTest.ktas an example of writing Component testsSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores