Added Store#stateFlow(Lifecycle) and Store#labelsChannel(Lifecycle) API, promoted to stable
#148
Conversation
…)` API, promoted to stable
WalkthroughThis pull request introduces lifecycle-aware extensions for state flows and label channels in the MVIKotlin coroutines library. The changes add new methods to the Changes
Sequence DiagramsequenceDiagram
participant Store
participant Lifecycle
participant StateFlow/Channel
Store->>Lifecycle: Observe lifecycle
Lifecycle-->>Store: Register observer
Store->>StateFlow/Channel: Emit states/labels
Lifecycle->>Store: On destroy
Store->>StateFlow/Channel: Unsubscribe and cleanup
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
♻️ Duplicate comments (1)
mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/LabelChannelWithLifecycleTest.kt (1)
49-53: 🛠️ Refactor suggestionDuplicate infinite loop pattern
Similar to StateFlowWithLifecycleTest, both test methods use an infinite loop pattern that could be improved.
Also applies to: 68-72
🧹 Nitpick comments (6)
mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithScopeTest.kt (3)
17-33: Consider adding cleanup and testing initial state.While the test correctly verifies state collection, consider:
- Adding
scope.cancel()in afinallyblock to ensure cleanup- Explicitly testing that the initial state (0) is collected first
@Test fun WHEN_state_emitted_THEN_state_collected() { val store = TestStore() val scope = CoroutineScope(Dispatchers.Unconfined) val flow = store.stateFlow(scope) val items = ArrayList<Int>() + try { scope.launch { flow.collect { items += it } } store.stateObserver?.onNext(1) store.stateObserver?.onNext(2) store.stateObserver?.onNext(3) assertContentEquals(listOf(0, 1, 2, 3), items) + } finally { + scope.cancel() + } }
35-48: Consider testing concurrent state emissions.The tests cover basic state collection and cleanup, but consider adding a test case for concurrent state emissions to ensure thread-safety.
Would you like me to provide an example test case for concurrent state emissions?
50-77: Add KDoc to document TestStore behavior.Consider adding documentation to explain:
- The purpose of this test implementation
- The behavior of the state observer tracking
- Why certain methods are no-op
+ /** + * Test implementation of [Store] that tracks state observers and provides + * controlled state emission for testing purposes. + * + * @property state Always returns 0 as initial state + * @property stateObserver Tracks the current state observer for verification + */ private class TestStore : Store<Int, Int, Int> {mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithLifecycleTest.kt (1)
22-23: Consider using @BeforeTest for test setupBoth test methods have similar setup code. Consider extracting common setup into a @BeforeTest method to reduce duplication.
+ private lateinit var store: TestStore + private lateinit var scope: CoroutineScope + + @BeforeTest + fun setUp() { + store = TestStore() + scope = CoroutineScope(Dispatchers.Unconfined) + }Also applies to: 42-43
mvikotlin-extensions-coroutines/src/commonMain/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StoreExt.kt (2)
46-61: Implementation looks solid with proper lifecycle management!The implementation correctly:
- Initializes with current state
- Updates state through observer
- Cleans up resources on lifecycle destroy
However, consider documenting the thread-safety guarantees of the StateFlow updates.
Add a note in the documentation about thread-safety:
* Please note that the actual collection of the [StateFlow] may not be synchronous * depending on [CoroutineContext] being used. + * + * The StateFlow updates are thread-safe and atomic. * * @param lifecycle a [Lifecycle] used for cancelling the underlying [MutableStateFlow].
138-166: Consider handling channel backpressure more explicitly.While the implementation correctly manages lifecycle and cleanup, the use of
trySendcould silently drop labels if the channel is full.Consider either:
- Using
sendinstead oftrySendto apply backpressure, or- Adding documentation about potential label drops:
* Due to the nature of how channels work, it is recommended to have one [Channel] per subscriber. + * + * Note: Labels might be dropped if the channel is full (when using default BUFFERED capacity). + * Consider adjusting the capacity parameter based on your needs. * * Please note that the actual collection of the [ReceiveChannel] may not be synchronous depending on
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mvikotlin-extensions-coroutines/api/android/mvikotlin-extensions-coroutines.api(1 hunks)mvikotlin-extensions-coroutines/api/jvm/mvikotlin-extensions-coroutines.api(1 hunks)mvikotlin-extensions-coroutines/src/commonMain/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StoreExt.kt(7 hunks)mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/LabelChannelWithLifecycleTest.kt(1 hunks)mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/LabelChannelWithScopeTest.kt(1 hunks)mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowTest.kt(2 hunks)mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithLifecycleTest.kt(1 hunks)mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithScopeTest.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowTest.kt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on Linux
- GitHub Check: Build on macOS
🔇 Additional comments (9)
mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithScopeTest.kt (1)
1-13: LGTM! Well-organized imports.The imports are logically grouped and include all necessary components for testing coroutines and state flow functionality.
mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/LabelChannelWithScopeTest.kt (2)
17-17: LGTM! Clear and descriptive class name.The rename from
LabelChannelTesttoLabelChannelWithScopeTestbetter reflects the test's focus on scope-based functionality.
Line range hint
19-86: Comprehensive test coverage for the labelsChannel API.The test suite effectively covers the critical aspects of the
labelsChannelfunctionality:
- Label emission and collection
- Proper cleanup on scope cancellation
- Channel cancellation behavior
Let's verify if there are any edge cases not covered by running this analysis:
✅ Verification successful
Test coverage is comprehensive and well-structured
The test suite effectively covers all critical aspects of the
labelsChannelAPI, including both the CoroutineScope and Lifecycle variants. The implementation's simplicity, combined with thorough testing of core functionality and cleanup scenarios, provides confidence in the code's reliability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential edge cases in the Store interface implementation # that might need additional test coverage # Search for Store implementations to analyze their label handling ast-grep --pattern 'class $_ : Store<$_, $_, $_> { $$$ override fun labels($_) { $$$ } $$$ }' # Search for labelsChannel usage patterns to ensure test coverage is complete rg "labelsChannel" -A 3Length of output: 8859
mvikotlin-extensions-coroutines/src/commonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithLifecycleTest.kt (2)
53-79: TestStore implementation looks goodThe TestStore implementation correctly implements the Store interface and provides the necessary functionality for testing. The use of a nullable stateObserver with private setter is a good pattern for testing subscription behavior.
44-46: 🛠️ Refactor suggestionAvoid infinite loops in tests
The infinite loop in the test could potentially hang if the lifecycle destruction doesn't properly cancel the flow collection.
- scope.launch { - while (true) { - channel.receive() - } - } + scope.launch { + try { + flow.collect() + } catch (e: CancellationException) { + // Expected when lifecycle is destroyed + } + }Likely invalid or redundant comment.
mvikotlin-extensions-coroutines/api/jvm/mvikotlin-extensions-coroutines.api (1)
69-73: API changes look goodThe new lifecycle-aware methods are well-designed:
- They maintain backward compatibility by keeping existing methods
- They follow the same pattern for both labels and state
- They properly handle default parameters via synthetic methods
mvikotlin-extensions-coroutines/api/android/mvikotlin-extensions-coroutines.api (1)
69-73: Android API changes match JVM APIThe Android API changes are identical to the JVM API changes, maintaining consistency across platforms.
mvikotlin-extensions-coroutines/src/commonMain/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StoreExt.kt (2)
3-4: Documentation improvements and imports look good!The documentation changes better reflect the behavior of the methods, and the new imports are necessary for the lifecycle integration.
Also applies to: 23-26, 31-35
Line range hint
1-166: Verify test coverage for lifecycle-aware methods.The AI summary mentions new test classes (
LabelChannelWithLifecycleTestandStateFlowWithLifecycleTest), but let's verify their existence and coverage.✅ Verification successful
Test coverage for lifecycle-aware methods is comprehensive
Both
stateFlowandlabelsChannellifecycle-aware methods are thoroughly tested, covering resource cleanup, unsubscription, and cancellation behaviors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for new lifecycle-aware methods # Check for test files echo "Checking for test files..." fd -e kt "StateFlowWithLifecycleTest|LabelChannelWithLifecycleTest" # Look for test cases covering lifecycle destruction echo -e "\nChecking test coverage for lifecycle destruction..." rg -A 5 "doOnDestroy|lifecycle.*destroy" -g "*Test.kt"Length of output: 8679
...ommonTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/StateFlowWithLifecycleTest.kt
Show resolved
Hide resolved
...onTest/kotlin/com/arkivanov/mvikotlin/extensions/coroutines/LabelChannelWithLifecycleTest.kt
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Documentation
Tests