Fixed a crash when multiple retained components are created with different keys #813
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (3)
decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt (3)
46-60: Refactor Repeated Test Setup to Improve MaintainabilityThe test methods
WHEN_recreated_THEN_old_lifecycle_destroyedandWHEN_recreated_THEN_new_lifecycle_resumedcontain similar setup code involving the creation ofTestOwnerinstances, retained components, and event logging. Consider extracting common setup steps into a helper method or utilizing a setup function annotated with@Beforeto reduce duplication and enhance readability.Also applies to: 68-76
227-289: Consolidate Back Callback Registration in Back Handler TestsThe back handler tests (
GIVEN_enabled_BackCallback_registered_in_all_retained_components_WHEN_onBackPressed_THEN_last_callback_calledand others) have repetitive code for registeringBackCallbackinstances and initializing flags. Refactoring this repeated code into a helper function or using parameterized tests could make the code cleaner and easier to maintain.
291-305: Clarify Default Parameter Usage in Helper FunctionThe
retainedComponenthelper function uses default values for parameters likekey,discardSavedState,isStateSavingAllowed, andisChangingConfigurations. While defaults can simplify calls, explicitly specifying these parameters in test methods can improve readability and prevent unintended behaviors due to implicit values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt(1 hunks)decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt(1 hunks)decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentSingleTest.kt(1 hunks)deps.versions.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentSingleTest.kt
🔇 Additional comments (6)
deps.versions.toml (1)
5-5: Verify the essenty library update and its changes
The update from 2.2.1 to 2.3.0 might be related to the component retention fixes. Let's verify the changes and compatibility.
Consider documenting the reason for this version update in the PR description, especially if it's related to the component retention fixes.
✅ Verification successful
Based on the release information from the essenty library's version 2.3.0, I can now generate the final response:
The essenty library update is directly related to the PR's objective
The version 2.3.0 of essenty specifically adds a key parameter to StateKeeper Android extensions (#182), which aligns perfectly with the PR's goal of implementing unique keys for each retainedComponent to fix the crash when multiple retained components are created with the same key.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the essenty library's changelog and release information
# Fetch release information for version 2.3.0
gh api repos/arkivanov/Essenty/releases | jq '.[] | select(.tag_name=="2.3.0") | {tag_name, body}'
# Check for any breaking changes or migration guides in docs
fd -e md -e txt . | xargs rg -i "migration|upgrade|breaking.?changes" 2>/dev/null
# Look for any existing usages of essenty that might need updates
rg "import.*essenty"
Length of output: 50660
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt (3)
100-102: Prevent duplicate retained components with the same key
The added check ensures that an exception is thrown if another retained component is already registered with the same key, effectively preventing key collisions.
105-106: Proper initialization of StateKeeper and InstanceKeeper
Initializing stateKeeper and instanceKeeper with the appropriate parameters enhances state management and ensures consistency within the component lifecycle.
111-113: Ensure proper cleanup of discarded retained instances
Removing and destroying the retained instance when discardRetainedInstance is true prevents potential memory leaks and ensures that outdated instances do not persist.
decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt (2)
22-30: Test Correctly Validates Exception Handling for Duplicate Keys
The test WHEN_same_key_reused_THEN_throws_IllegalStateException effectively checks that reusing the same key throws an IllegalStateException, ensuring key uniqueness is enforced.
1-305: Comprehensive Test Suite Enhances Code Reliability
The RetainedComponentMultipleTest class provides extensive coverage of retained component behaviors, including lifecycle events, state management, instance retention, and back button handling. The well-structured tests effectively ensure robustness and correctness of the retained component functionality.
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt
Outdated
Show resolved
Hide resolved
arkivanov
force-pushed
the
fix-multiple-retained-components
branch
from
c2730fe to
c36b9a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/TestOwner.kt (1)
25-26: Add KDoc to document the isChangingConfigurations propertyThe property simulates Android's Activity.isChangingConfigurations behavior, but this isn't immediately clear without documentation. Consider adding KDoc to explain its purpose and relationship to Android's lifecycle.
+ /** + * Simulates Android's Activity.isChangingConfigurations behavior. + * Used to determine if the recreation is due to configuration changes, + * affecting how retained components and ViewModels are handled. + */ var isChangingConfigurations: Boolean = truedecompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt (1)
168-181: Consider adding test-only documentationThe methods are correctly annotated with
@SuppressLint("VisibleForTests"), but consider adding documentation to clarify their test-only nature.Add KDoc comments like this:
+ /** + * Test-only method for handling back navigation events. + * @hide + */ @SuppressLint("VisibleForTests") override fun handleOnBackStarted(backEvent: BackEventCompat) {decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt (2)
96-97: Use unique keys for StateKeeper and InstanceKeeper registrationsYou are registering state and instances with the same key
"key"in multiple retained components (ctx1andctx2). While each component has its ownStateKeeperandInstanceKeeper, using identical keys can lead to confusion or potential issues if the isolation is not handled as expected. It's a good practice to use distinct keys within each component to ensure clarity and prevent any unforeseen conflicts.Consider updating the keys to be unique per component:
// StateKeeper registrations in tests: - ctx1.stateKeeper.register(key = "key") { "saved_state_1" } + ctx1.stateKeeper.register(key = "key1") { "saved_state_1" } - ctx2.stateKeeper.register(key = "key") { "saved_state_2" } + ctx2.stateKeeper.register(key = "key2") { "saved_state_2" } // Consuming the state: - val restoredState1 = ctx1.stateKeeper.consume<String>(key = "key") + val restoredState1 = ctx1.stateKeeper.consume<String>(key = "key1") - val restoredState2 = ctx2.stateKeeper.consume<String>(key = "key") + val restoredState2 = ctx2.stateKeeper.consume<String>(key = "key2") // InstanceKeeper usage: - val instance11 = ctx1.instanceKeeper.getOrCreate(key = "key", factory = ::TestInstance) + val instance11 = ctx1.instanceKeeper.getOrCreate(key = "key1", factory = ::TestInstance) - val instance12 = ctx2.instanceKeeper.getOrCreate(key = "key", factory = ::TestInstance) + val instance12 = ctx2.instanceKeeper.getOrCreate(key = "key2", factory = ::TestInstance)Also applies to: 126-127, 146-147, 180-181, 198-199
258-271: Clarify dependency on BackCallback registration orderIn the test
GIVEN_enabled_BackCallback_registered_in_all_retained_components_WHEN_onBackPressed_THEN_last_callback_called, the invocation of the back callbacks depends on the order in which they are registered—the last registered enabled callback is called. This implicit reliance on registration order could lead to confusion or maintenance issues in the future if additional callbacks are added.Consider adding comments to clarify this behavior or explicitly defining the priority to make the test intentions clear:
+ // The last registered BackCallback takes precedence ctx1.backHandler.register(BackCallback { isCalled1 = true }) ctx2.backHandler.register(BackCallback { isCalled2 = true })Alternatively, if the order is significant to functionality, consider refactoring to manage callback priority explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt(3 hunks)decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt(1 hunks)decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentSingleTest.kt(5 hunks)decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/TestOwner.kt(1 hunks)deps.versions.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentSingleTest.kt
- deps.versions.toml
🔇 Additional comments (4)
decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/TestOwner.kt (2)
33-33: LGTM! Well-structured implementation of configuration change simulation
The implementation correctly simulates Android's configuration change behavior by setting the flag before state saving and using it to determine ViewModelStore handling. This provides precise control for testing different recreation scenarios.
25-33: Verify test coverage for multiple retained components
The new configuration change simulation capabilities should be leveraged to ensure comprehensive testing of multiple retained components with different keys.
✅ Verification successful
Let me check the actual test implementations to verify the coverage.
Let me check the test coverage for different scenarios in RetainedComponentMultipleTest.kt to ensure comprehensive testing.
Based on the test coverage analysis, I can now provide the final response:
Test coverage for multiple retained components is comprehensive
The implementation includes thorough test coverage for multiple retained components with different scenarios:
- Configuration changes with both
isChangingConfigurations = true/false - Multiple components with different keys being retained and recreated
- Lifecycle state verification for multiple components
- State saving/restoration with different configurations
- Edge cases like duplicate keys and state discarding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new isChangingConfigurations property is being used in retained component tests
# Look for tests using isChangingConfigurations
echo "Checking usage in retained component tests..."
rg -l "isChangingConfigurations" --type kotlin | grep -i "test"
# Look for multiple retained component test scenarios
echo "Checking multiple retained component test coverage..."
ast-grep --pattern 'fun test$$$RetainedComponent$$$'
Length of output: 693
Script:
#!/bin/bash
# Check the implementation of retained component tests
echo "Checking RetainedComponentMultipleTest.kt..."
rg "class RetainedComponentMultipleTest" -A 10 decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
echo "\nChecking test methods in RetainedComponentMultipleTest.kt..."
ast-grep --pattern 'fun test$$$($$$) {
$$$
}' decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
echo "\nChecking isChangingConfigurations usage in tests..."
rg "isChangingConfigurations" -B 2 -A 2 decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
Length of output: 4212
Script:
#!/bin/bash
# Get all test method names and their context
echo "Checking all test methods in RetainedComponentMultipleTest.kt..."
rg "fun test" -B 1 -A 1 decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
# Check for lifecycle testing
echo "\nChecking lifecycle-related test coverage..."
rg "lifecycle" -B 1 -A 1 decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
# Check for state keeper testing
echo "\nChecking state keeper test coverage..."
rg "stateKeeper" -B 1 -A 1 decompose/src/androidUnitTest/kotlin/com/arkivanov/decompose/RetainedComponentMultipleTest.kt
Length of output: 3698
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt (2)
112-117: LGTM: Proper cleanup of discarded instances
The implementation correctly handles the cleanup sequence for discarded instances, ensuring proper lifecycle management and resource cleanup.
101-103: LGTM: Effective duplicate key prevention
The new check prevents crashes by detecting duplicate component registration early. This directly addresses the issue described in #807.
Let's verify this handles all duplicate key scenarios:
✅ Verification successful
Duplicate key check is comprehensive and correctly implemented
The verification shows that the duplicate key check in RetainedComponent.kt is the only place where savedStateRegistry.getSavedStateProvider is used for validation. The only other usage of the registry is in DeeplinkUtils.kt for a different purpose (deep link state registration). This confirms that the new check is the sole gatekeeper for preventing duplicate component registration.
The documentation in various navigation components (slots, stacks, panels, pages) consistently emphasizes unique key requirements, and this implementation aligns with that architectural requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other potential key validation or component registration patterns
# that might bypass this check
# Look for other component registration points
rg "savedStateRegistry\.getSavedStateProvider|savedStateRegistry\.registerSavedStateProvider"
# Look for other key validation patterns
rg "check.*key|require.*key|assert.*key"
Length of output: 25777
decompose/src/androidMain/kotlin/com/arkivanov/decompose/RetainedComponent.kt
Show resolved
Hide resolved
Fixes: #807.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
essentylibrary to 2.3.0 for improved functionality.TestOwnerclass.