Added key parameter to StateKeeper Android extensions #182
Conversation
WalkthroughThe changes in this pull request introduce new method signatures and overloads in the Changes
Possibly related PRs
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: 0
🧹 Outside diff range and nitpick comments (2)
state-keeper/src/androidMain/kotlin/com/arkivanov/essenty/statekeeper/AndroidExt.kt (1)
Line range hint
40-62: Consider using the provided key for Bundle storage.Currently, while the registry key is customizable, the inner Bundle still uses the hardcoded
KEY_STATEconstant. This could potentially cause conflicts if multiple StateKeepers use different registry keys but share the same inner Bundle key.Consider using the provided key for Bundle storage as well:
- ?.getSerializableContainer(key = KEY_STATE) + ?.getSerializableContainer(key = key) - putSerializableContainer(key = KEY_STATE, value = dispatcher.save()) + putSerializableContainer(key = key, value = dispatcher.save())state-keeper/api/android/state-keeper.api (1)
Line range hint
46-54: LGTM: Well-structured API changesThe changes to
saveablemethods are consistent with the new key parameter support. The API design:
- Maintains backward compatibility through default parameters
- Provides consistent overloads for both StateKeeper and StateKeeperOwner
- Preserves the existing method signatures while extending functionality
Consider adding KDoc comments to explain the purpose and usage of the key parameter in the source files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
state-keeper/api/android/state-keeper.api(1 hunks)state-keeper/src/androidMain/kotlin/com/arkivanov/essenty/statekeeper/AndroidExt.kt(2 hunks)
🔇 Additional comments (5)
state-keeper/src/androidMain/kotlin/com/arkivanov/essenty/statekeeper/AndroidExt.kt (4)
21-28: LGTM! Clean refactoring to support new key parameter.
The changes maintain backward compatibility by delegating to the new overload with the default KEY_STATE constant.
76-81: LGTM! Clean refactoring of the extension function.
The changes maintain backward compatibility by delegating to the new overload with the default KEY_STATE constant.
92-102: LGTM! Well-documented and properly implemented extension overload.
The new overload is cleanly implemented and maintains consistency with the existing pattern.
Line range hint 1-102: Verify existing StateKeeper usages in the codebase.
Since these changes add new overloads while maintaining backward compatibility, let's verify that existing usages are not affected.
✅ Verification successful
Let me check the test file to verify the usage patterns and ensure compatibility.
Changes are safe and maintain backward compatibility
The verification confirms that:
- The
KEY_STATEconstant is only used withinAndroidExt.ktand not referenced elsewhere - Existing usages in tests demonstrate compatibility with both extension methods (
stateKeeper()) and direct constructor calls (StateKeeper()) - The new overloads maintain the same behavior while adding optional parameters
- Test cases cover critical scenarios including state saving/restoring and the new
isSavingAllowedanddiscardSavedStateparameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing StateKeeper usages to ensure they're not affected
rg -l "StateKeeper\(" --type kotlin
# Search for any potential custom keys already in use
rg "KEY_STATE" --type kotlin
Length of output: 916
Script:
#!/bin/bash
# Check the test file content to understand usage patterns
cat state-keeper/src/androidUnitTest/kotlin/com/arkivanov/essenty/statekeeper/AndroidStateKeeperTest.kt
Length of output: 4113
state-keeper/api/android/state-keeper.api (1)
2-8: Verify the impact of new StateKeeper API signatures
The addition of String parameter for custom keys is a good enhancement. However, since this is a public API change, we should ensure:
- The changes are documented in the changelog
- Migration guide is provided for users upgrading from previous versions
- The default key value maintains backward compatibility
Let's verify the usage and documentation:
✅ Verification successful
Let me gather more information about the implementation and documentation to verify if this is a breaking change.
Let me try a different approach to check the implementation details and default values.
API changes maintain backward compatibility
The verification shows that the API changes are non-breaking:
- The original
StateKeeperfunction is preserved with the same default parameters - The new overload with
keyparameter usesKEY_STATEconstant as the default key in the original function - Both functions share the same default values for
discardSavedStateandisSavingAllowed
The changes safely extend the API by adding optional key customization while maintaining full backward compatibility with existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for changelog updates and documentation
fd -e md -e txt . | xargs rg -i "statekeeper.*key|breaking.*change"
# Look for existing usages that might need migration
rg "StateKeeper\s*\(" --type kotlin
Length of output: 1134
Script:
#!/bin/bash
# Check implementation of StateKeeper to see if it's backward compatible
ast-grep --pattern 'fun StateKeeper($$$) {
$$$
}'
# Check for any CHANGELOG or release notes
fd "CHANGELOG|RELEASE" --type f | xargs cat
# Check the default key implementation
rg "key\s*=.*default" --type kotlin
Length of output: 137
Script:
#!/bin/bash
# Check the implementation file for default values and backward compatibility
rg -A 10 "fun StateKeeper" state-keeper/src/androidMain/kotlin/com/arkivanov/essenty/statekeeper/AndroidExt.kt
# Look for any documentation or comments about the key parameter
rg -B 2 "fun StateKeeper" state-keeper/src/androidMain/kotlin/com/arkivanov/essenty/statekeeper/AndroidExt.kt
# Check for any tests that might show the default behavior
rg -A 5 "class.*Test" state-keeper/src/androidUnitTest/kotlin/com/arkivanov/essenty/statekeeper/AndroidStateKeeperTest.kt
Length of output: 1898
Summary by CodeRabbit
New Features
StateKeeperandstateKeeper, allowing users to specify a key for enhanced state management.Bug Fixes
Refactor