Change Child#key type from Any to String and promote it to stable #957
Conversation
WalkthroughChange key types from Any/Object to String across Child, ChildItem, ChildStack and related components; introduce per-item String keys, persist and restore them via SavedChildState, propagate keys through factories and navigators, update compose/android extensions to use Child.key, and add tests/utilities ensuring unique, stable string keys. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-11-09T18:14:31.048ZApplied to files:
📚 Learning: 2025-05-28T22:32:21.630ZApplied to files:
📚 Learning: 2025-08-31T18:04:37.338ZApplied to files:
📚 Learning: 2025-08-31T18:07:47.010ZApplied to files:
🧬 Code graph analysis (3)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (3)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (6)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
🪛 Gitleaks (8.30.0)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt[high] 405-405: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 406-406: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 407-407: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 408-408: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (20)
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. Comment |
arkivanov
changed the title
arkivanov
force-pushed
the
child-key-3
branch
4 times, most recently
from
98293c2 to
83b9511
Compare
arkivanov
force-pushed
the
child-key-3
branch
from
83b9511 to
34a97d7
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt (1)
232-255: Breaking serialization change inSavedState.childStaterequires migration strategy
SavedState.childStatechanged fromList<SerializableContainer?>toList<ChildrenNavigator.SavedChildState>(lines 269–273). This alters the on-disk serialization schema used bySavedState.serializer()at lines 240 and 257.Apps with persisted
SavedStatedata will fail to deserialize on upgrade—kotlinx.serialization cannot map the old list element type to the new nestedSavedChildStatestructure. There is no try-catch, fallback, or version migration in place.Either:
- Accept that old
childrenstate is invalidated on upgrade, or- Introduce backward compatibility (e.g., a versioned format, a new field, or custom deserialization logic that tolerates the old shape).
Also applies to: 269–273
🧹 Nitpick comments (3)
extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt (1)
166-169: Consider aligningInactiveChild.keytype withActiveChild.key.
ActiveChild.keyis typed asString(line 161), butInactiveChild.keyremains typed asAny(line 167). SinceInactiveChildis always created with a String key fromcurrentChild.key(line 118), consider updating the type for consistency:private class InactiveChild( - val key: Any, + val key: String, val savedState: SparseArray<Parcelable> )extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt (1)
75-77: Consider updatingKeys.settype toSet<String>for consistency.The
Keysclass usesSet<Any>, but it's only ever instantiated and updated withSet<String>fromgetKeys()andretainStates(currentKeys: Set<String>). Aligning the type would improve type safety:private class Keys( - var set: Set<Any> + var set: Set<String> )decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (1)
222-223: Minor: Missing blank line between test functions.There's a missing blank line between
GIVEN_multiple_children_WHEN_recreated_THEN_keys_not_changed(ending at line 222) andGIVEN_multiple_children_with_same_configurations_WHEN_recreated_THEN_keys_not_changed(starting at line 223), which is inconsistent with the rest of the file.assertEquals(keys, children.value.keys) } + @Test fun GIVEN_multiple_children_with_same_configurations_WHEN_recreated_THEN_keys_not_changed() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt(2 hunks)decompose/api/android/decompose.api(2 hunks)decompose/api/decompose.klib.api(2 hunks)decompose/api/jvm/decompose.api(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt(4 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt(7 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt(2 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt(0 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt(4 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt(2 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt(5 hunks)extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt(1 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt(1 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt(2 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt(4 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt(11 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt(2 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt(2 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt(4 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt(4 hunks)
💤 Files with no reviewable changes (1)
- decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
📚 Learning: 2025-11-09T18:14:31.048Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
Applied to files:
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktextensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.ktdecompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.ktextensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.ktdecompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.ktdecompose/api/jvm/decompose.apiextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.ktdecompose/api/decompose.klib.apidecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.ktdecompose/api/android/decompose.api
📚 Learning: 2025-05-28T22:32:21.630Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 880
File: docs/extensions/compose.md:429-431
Timestamp: 2025-05-28T22:32:21.630Z
Learning: In Decompose documentation, the correct import path for `subscribeAsState` in the Child Items navigation example is `com.arkivanov.decompose.extensions.compose.subscribeAsState`, not the jetbrains-specific variant. The import paths vary depending on the specific compose variant being used (general compose vs jetbrains/multiplatform compose).
Applied to files:
extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktextensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.ktextensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.ktextensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.ktextensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.ktextensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt
📚 Learning: 2025-08-31T18:04:37.338Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:78-85
Timestamp: 2025-08-31T18:04:37.338Z
Learning: In Android Vanilla Ice Cream predictive back animations, only the exitModifier should honor the swipe edge direction. The enterModifier intentionally uses a consistent negative translationX offset (-size.width * 0.2F) regardless of the swipe edge, as per the official Android specification.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
📚 Learning: 2025-08-31T18:07:47.010Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:113-121
Timestamp: 2025-08-31T18:07:47.010Z
Learning: In Android Vanilla Ice Cream predictive back animations, the exit translationX stop value in the lerp should be consistently positive (width * 0.2F) regardless of swipe edge direction, as per the official Android specification. This is intentional design and should not be mirrored for right-edge gestures.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
🧬 Code graph analysis (4)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (3)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (2)
stateOf(98-99)navigate(91-93)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt (1)
navigate(209-215)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
navigate(139-142)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (2)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (2)
stack(275-279)child(272-273)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (1)
child(223-224)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
randomKey(44-45)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (4)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimation.kt (2)
Child(131-151)child(374-380)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt (1)
child(31-32)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1)
child(115-116)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/StackAnimationDirectionsTest.kt (1)
child(69-70)
🔇 Additional comments (33)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt (1)
7-7: LGTM! Clean introduction of theKeyedinterface for test configurations.The new
Keyedinterface withkey: Stringproperty enforces the String-typed key contract at the test level, and the key propagation intoRouterState()correctly usesit.keyfor both active and backstack items. This aligns well with the PR's objective of changingChild#keyfromAnytoString.Also applies to: 18-32, 33-36
extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt (1)
128-128: LGTM! Simplified key retrieval using the new String-typed key.Directly accessing
activeChild.keyinstead of callingkeyHashString()is cleaner and aligns with the PR's migration to String keys.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt (1)
20-52: LGTM! Key parameter correctly added and propagated.The
key: Stringparameter is properly added to the factory'sinvokemethod and correctly passed through toChildItem.Created. This ensures consistent key propagation throughout the child creation pipeline.extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt (1)
32-32: LGTM! Correct migration to String keys for state management.Using
child.keydirectly and mapping toChild<*, *>::keyproperly leverages the new String-typed keys forSaveableStateProviderand state retention.Also applies to: 55-56
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (2)
588-591: LGTM! Config correctly implementsKeyedinterface.The
Configdata class now properly implementsTestStackRouter.Keyedwithkey = value.toString(). This allows testing scenarios with duplicate configurations (e.g., lines 100-112 withConfig(0), Config(0)), which aligns with the PR's goal of fixing the "Key XYZ was used multiple times" crash.
356-356: LGTM! Assertions updated to use stack value API.The assertions correctly access configurations via
router.stack.value.configurations()instead of the previous direct property access, maintaining consistency with the updated router API.Also applies to: 394-394, 436-436, 453-453
decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt (1)
3-9: List.keys helper looks correct and aligns with the new key modelThe
keysextension cleanly exposesList<Child<*, *>>keys asList<String>and matches the newChild.key: StringAPI. It’s a good fit for the new uniqueness assertions in tests.Also applies to: 35-37
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt (1)
31-33: Explicit key in test helper matches newChild.CreatedAPISetting
key = configurationkeeps the tests compiling against the newChild.Createdsignature and guarantees stable, readable keys for these scenarios where configurations are unique.extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt (1)
402-408: Key propagation through panel wrappers and empty children looks consistentForwarding
keyinasPanelChild()and assigning fixed, distinct keys toEmptyChild1..4ensures all panelChildStacks work with stableStringkeys, including the placeholder entries used for layout/predictive back. This keeps saveable state and equality behavior coherent with the rest of the new key model.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt (1)
6-13: Addingkey: StringtoChildItemFactoryaligns factory API with keyed childrenIncluding
key: Stringin the factory signature is consistent withChildItem.Created/Child.keynow being keyed byString. The parameter ordering (configuration, key, then saved/instance-keeper context) also keeps identity-related data grouped.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt (1)
215-235: State mapping now correctly preserves keys intoChildPanelsThe updated
stateMapperthat:
- Filters to
Child.Created,- Derives
(Panel.* instance, key)pairs, and- Reconstructs
ChildPanelswithChild.Created(config, instance, key)ensures panel-level children keep their
Stringkeys from the router layer. That’s consistent with the new keyed model and with how the compose extensions expect keys to behave.decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (1)
8-9: Test base now enforces key uniqueness across all children assertions
- Making
before()opengives derived tests control over setup while still allowing reuse of the defaultlifecycle.resume(); subclasses should callsuper.before()if they override it.- Wiring
assertKeysUnique()intoassertChildren()plus the newkeyshelper ensures every test that checks children viaassertChildrenalso validates the “keys are unique” invariant, which is important for the newString-keyed model.This is a good, low-friction way to guard the new key semantics across the test suite.
Also applies to: 20-21, 36-39, 101-117
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
258-279: LGTM! Test helpers and type parameters correctly updated for String keys.The test infrastructure is properly updated to use
Stringkeys throughout. The helper functionschild(key: String)andstack(vararg keys: String)are consistent with the patterns in other test files likeSimpleStackAnimationTest.ktandChildrenTest.kt.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt (1)
20-37: LGTM! Clean promotion of key parameter to stable API with String type.The constructor changes properly reflect the shift to
Stringkeys. The secondary constructor correctly delegates key derivation toChild.Created, which usesconfiguration.hashString(). The TODO comment for@VisibleForTestingis a reasonable forward-looking note for API hygiene.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1)
115-122: LGTM! Test helpers correctly updated for String-keyed stacks.The
child()andstack()helper functions are properly updated to useStringkeys and are consistent with the patterns inDefaultStackAnimationTest.ktandChildrenTest.kt.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
203-229: LGTM! Test infrastructure properly migrated to String keys.The
setContent,routerState, andchildhelpers are correctly updated to work withStringkeys. The default content lambda at line 206 now usesit.keydirectly instead oftoString(), which is cleaner with the newStringkey type.decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt (2)
44-63: Destroyed class properly updated with String key and binary compatibility.The
Destroyedclass mirrors theCreatedclass pattern correctly. The@ExperimentalDecomposeApiannotation is retained on the primary constructor (presumably for other reasons), while the key property itself is now stable as aString.
18-42: LGTM! Core API change fromAnytoStringkey with proper binary compatibility.The key type change is well-implemented:
- Primary constructors now take
Stringkeys- Secondary constructors derive keys via
configuration.hashString()for convenience- Binary compatibility is preserved with
@Deprecatedcopy overloads that convertAnytoStringhashString()extension function properly provides deterministic, stable string representations by combining class name and hash code in base-36 formatBased on learnings, this change to the previously experimental
Child.keyproperty is acceptable.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
44-45: ExperimentalUuidApi will remain in experimental state through Kotlin 2.2; stabilization proposed for 2.3.The
randomKey()function useskotlin.uuid.Uuid, which remains experimental as of Kotlin 2.2 with no immediate stabilization. A community discussion (Sep 15, 2025) proposes moving theUuidclass to stable in Kotlin 2.3, but this is not yet released. Since this is internal API with proper@OptInannotation, the experimental status is acceptable; however, monitor the Kotlin 2.3 release timeline for potential stabilization.extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt (1)
30-30: LGTM! Simplified key resolution using the new stableChild.keyproperty.The default key function now directly uses
Child::keyinstead ofkeyHashString, which aligns with the PR's objective of promotingChild.keyto stable with aStringtype. This simplifies the key resolution while maintaining compatibility sinceStringis assignable toAny.Also applies to: 56-56
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (2)
20-23: Good setup for duplicate configuration testing.Enabling
duplicateConfigurationsEnabledinbefore()is appropriate since several tests (lines 44-52, 66-74, 201-210, 224-233) specifically validate key uniqueness when multiple children have the same configuration.
26-233: Comprehensive test coverage for key uniqueness and stability.The tests thoroughly cover:
- Key uniqueness on creation and navigation (add/remove/replace)
- Key stability across shuffling, configuration changes, and recreation
- Edge cases with duplicate configurations
This provides excellent regression protection for the new String-based key system.
decompose/api/decompose.klib.api (2)
526-526: API surface correctly updated to useStringkeys.The key type change from
AnytoStringis consistently applied across:
Child.key(line 526)Child.Created.key(line 537)Child.Destroyed.key(line 559)Based on learnings,
Child.keywas previously marked withExperimentalDecomposeApi, making this breaking change acceptable within the experimental API surface.Also applies to: 537-537, 559-559
544-544: Verify backward compatibility ofcopymethods acceptingAnyfor key.The
copymethods at lines 544 and 565 acceptkotlin/Anyfor the key parameter:
copy(#A1, #B1, kotlin/Any): Child<#A1, #B1>(line 544)copy(#A1, kotlin/Any): Child<#A1, kotlin/Nothing>(line 565)This appears to be for backward compatibility, but the key is
Stringelsewhere. Please verify this is intentional and document the behavior if theAnyis coerced toStringinternally.Also applies to: 565-565
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (4)
279-283: Well-designedSavedChildStatefor key persistence.The
@Serializabledata class properly encapsulates the key and saved state, enabling deterministic key restoration across process recreation. The nullablesavedStatecorrectly handles the case where aDestroyedchild has no state to save.
125-137: Clean state serialization with per-item key preservation.The
saveChildState()method correctly maps each item to aSavedChildState, preserving:
- The item's key for identity restoration
- The state from
stateKeeperDispatcher.save()forCreateditems- The cached
savedStateforDestroyeditemsThis ensures keys remain stable across save/restore cycles.
188-200: Correct key generation strategy for new items.New items (when
oldItems[key]isnull) correctly receive a freshrandomKey(), while existing items preserve their original keys. This ensures:
- Key uniqueness for new children
- Key stability for existing children across navigation changes
70-94: State restoration correctly uses saved keys.The
restore()method properly reconstructs items usingsavedState.keyfor bothDestroyedandCreateditems, ensuring key stability across process recreation.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (3)
223-224: Test helper correctly constructs string-keyed children.The
child(key: String)helper createsChild.Createdwith:
- A fixed configuration (
"Config")- A unique instance (
Any())- The provided
keyparameterThis pattern aligns with similar test helpers in the codebase (e.g.,
SimpleStackAnimationTest.kt:114-115,StackAnimationDirectionsTest.kt:68-69).
199-202: Content function correctly useskeyfor child identification.The default content function now uses
it.key(line 201) for theChildcomposable's name parameter, which is appropriate since keys are now stable String values that uniquely identify children.
211-215: Router state builder correctly uses keys as both configuration and key.The
routerState(vararg keys: String)helper now uses the string key as both the configuration (implicitly viachild(key)) and the identity key. This simplifies test setup while maintaining proper child identification.decompose/api/android/decompose.api (1)
8-8: Android API changes consistent with JVM API.The Android API changes mirror the JVM API changes exactly, ensuring consistent behavior across platforms. The type narrowing from
ObjecttoStringfor child keys is applied uniformly.Also applies to: 13-13, 16-16, 19-20, 24-24, 31-31, 33-33, 36-37, 42-42, 572-572
decompose/api/jvm/decompose.api (1)
8-8: Public API change: Child key narrowed from Object to String.The return type of
getKey()has been changed fromObjecttoStringacrossChild,Child.Created, andChild.Destroyed. Backward-compatible constructors have been retained (2-parameter forCreated, 1-parameter forDestroyed, 2-parameter forChildStack).No call sites in the codebase call
getKey()or rely on Object-typed keys. All constructor invocations properly use String keys or the backward-compatible constructors. The experimental API status justifies this change.
...ommonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
child-key-3
branch
from
34a97d7 to
9bf763a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
70-97: Avoid silent truncation inrestore()when saved state list size mismatches nav state.
Line 74 useszip, which will drop the tail if sizes differ, potentially restoring the wrong key/state pairing (or skipping children) with no signal. Prefer a guard + safe fallback.private fun restore(navState: N, savedStates: List<SavedChildState>) { + if (navState.children.size != savedStates.size) { + // Corrupted/obsolete saved state — restore without per-item state rather than mis-applying keys. + retainedInstance.onDestroy() + switch(newStates = navState.children) + return + } val retainedItems = items.takeUnless(List<*>::isEmpty)?.mapIndexed(::Pair)?.toMap(HashMap()) items.clear() navState.children.zip(savedStates).forEachIndexed { index, (childNavState, savedState) -> items += restoreItem( status = childNavState.status, getDestroyedItem = { Destroyed( configuration = childNavState.configuration, key = savedState.key, savedState = savedState.savedState, ) }, getCreatedItem = { childItemFactory( configuration = childNavState.configuration, key = savedState.key, savedState = savedState.savedState, instanceKeeperDispatcher = retainedItems?.remove(index)?.instanceKeeperDispatcher, ) } ) } retainedItems?.values?.forEach { it.instanceKeeperDispatcher?.destroy() } }
🧹 Nitpick comments (4)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt (1)
18-32:toRouterState()assumes a non-empty stack; consider enforcing or guarding.
last()/dropLast(1)will throw on an emptyList<C>. If an empty stack is impossible by convention in these tests, consider asserting early (e.g.,require(isNotEmpty())) to make failures clearer; otherwise, handle empty stacks explicitly. Key propagation itself looks correct.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (2)
211-215: HardenrouterState(...)helpers against empty input (avoid.last()crash).
Line 213 and Line 219 will throw if a future test accidentally callsrouterState()with no args. Even though it’s private test code, adding a guard makes failures clearer.private fun routerState(vararg keys: String): ChildStack<String, Any> = - ChildStack( + require(keys.isNotEmpty()) { "routerState(keys) requires at least one key" }.let { + ChildStack( active = child(key = keys.last()), backStack = keys.dropLast(1).map { child(key = it) }, - ) + ) + } private fun routerState(vararg stack: Child.Created<String, Any>): ChildStack<String, Any> = - ChildStack( + require(stack.isNotEmpty()) { "routerState(stack) requires at least one child" }.let { + ChildStack( active = stack.last(), backStack = stack.dropLast(1), - ) + ) + }Also applies to: 217-221
223-224: Optional: setconfiguration = keyinchild()to avoid unrealistic “all configs equal” stacks.
Not required for these tests (they’re key-focused), but makingconfigurationunique improves representativeness and reduces the chance of config-based behavior accidentally masking issues later.private fun child(key: String): Child.Created<String, Any> = - Child.Created(configuration = "Config", instance = Any(), key = key) + Child.Created(configuration = key, instance = Any(), key = key)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
279-284:SavedChildStateplacement/visibility: please confirm intended serialization compatibility surface.
Now that keys are becoming stable, this serialized shape becomes harder to change without migration (even ifChildrenNavigatoritself isinternal). Consider documenting/locking the schema expectations wherever the statekeeper persistence is wired. (Based on learnings, earlier key changes were acceptable under experimental API; with stability, the bar is higher.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt(2 hunks)decompose/api/android/decompose.api(2 hunks)decompose/api/decompose.klib.api(2 hunks)decompose/api/jvm/decompose.api(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt(4 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt(7 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt(2 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt(0 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt(4 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt(2 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt(5 hunks)extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt(2 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt(1 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt(3 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt(4 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt(11 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt(2 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt(3 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt(4 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt(4 hunks)
💤 Files with no reviewable changes (1)
- decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt
- extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt
- extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
📚 Learning: 2025-11-09T18:14:31.048Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
Applied to files:
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.ktdecompose/api/decompose.klib.apidecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.ktdecompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.ktextensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.ktdecompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.ktdecompose/api/jvm/decompose.apidecompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.ktdecompose/api/android/decompose.api
📚 Learning: 2025-05-28T22:32:21.630Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 880
File: docs/extensions/compose.md:429-431
Timestamp: 2025-05-28T22:32:21.630Z
Learning: In Decompose documentation, the correct import path for `subscribeAsState` in the Child Items navigation example is `com.arkivanov.decompose.extensions.compose.subscribeAsState`, not the jetbrains-specific variant. The import paths vary depending on the specific compose variant being used (general compose vs jetbrains/multiplatform compose).
Applied to files:
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.ktextensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.ktextensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt
📚 Learning: 2025-08-31T18:04:37.338Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:78-85
Timestamp: 2025-08-31T18:04:37.338Z
Learning: In Android Vanilla Ice Cream predictive back animations, only the exitModifier should honor the swipe edge direction. The enterModifier intentionally uses a consistent negative translationX offset (-size.width * 0.2F) regardless of the swipe edge, as per the official Android specification.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
📚 Learning: 2025-08-31T18:07:47.010Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:113-121
Timestamp: 2025-08-31T18:07:47.010Z
Learning: In Android Vanilla Ice Cream predictive back animations, the exit translationX stop value in the lerp should be consistently positive (width * 0.2F) regardless of swipe edge direction, as per the official Android specification. This is intentional design and should not be mirrored for right-edge gestures.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
🧬 Code graph analysis (4)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (3)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (2)
stateOf(98-99)navigate(91-93)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt (1)
navigate(209-215)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
navigate(139-142)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (1)
child(223-224)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
randomKey(44-45)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (4)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (5)
routerState(216-220)routerState(222-226)setContent(203-214)Child(231-239)child(228-229)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/lifecycle/LifecycleControllerTest.kt (1)
setContent(93-96)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt (1)
child(31-32)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1)
child(115-116)
🪛 Gitleaks (8.30.0)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
[high] 405-405: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 406-406: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 407-407: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 408-408: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build on macOS
- GitHub Check: Build on Linux
🔇 Additional comments (23)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt (2)
34-36:Keyedinterface placement/shape looks good for test utilities.Simple contract (
val key: String) aligns with the move to stable string keys and keeps the test router decoupled from production key-generation logic.
7-32: All call sites pass non-empty lists with Keyed-implementing configs. No issues found across 33 TestStackRouter instantiations in DefaultWebHistoryControllerTest.kt—every call provides at least one Config element, and Config properly implements TestStackRouter.Keyed.extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt (1)
32-32: LGTM! Key type migration correctly applied.The changes consistently replace the hash-derived key with the actual
keyproperty (now typed as String) for state management. TheSaveableStateProvider,getKeys(), and internalKeysclass all correctly handleStringkeys.Also applies to: 55-56, 76-76
decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt (1)
3-3: LGTM! Useful test utility added.The new
keysextension property provides a convenient way to extract keys from lists ofChildobjects for test assertions, correctly returningList<String>to match the new key type.Also applies to: 37-37
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt (1)
47-47: LGTM! Consistent with stable API changes.The experimental stack implementation correctly mirrors the changes in the stable version, with all key handling updated to use
Stringtype. TheKeysclass type issue flagged in previous reviews has been properly resolved.Also applies to: 83-84, 104-104
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt (1)
403-403: LGTM! Key propagation correctly implemented.The changes properly propagate the
keyfield through panel child creation. The explicit keys forEmptyChild1-4are configuration identifiers, not sensitive credentials—the static analysis "generic-api-key" warnings are false positives and can be safely ignored.Also applies to: 405-408
decompose/api/decompose.klib.api (1)
413-413: LGTM! Public API surface correctly updated for String-typed keys.The API signatures consistently migrate key parameters and return types from
kotlin/Anytokotlin/StringacrossChild,Child.Created,Child.Destroyed, andChildStack. This is an expected breaking change as the PR promotesChild#keyto stable with a more specific type.Based on learnings, changes to Child.key runtime values are acceptable within the experimental API surface that is now being stabilized.
Also applies to: 526-526, 530-530, 537-537, 541-544, 552-552, 559-559, 562-565
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt (1)
32-32: LGTM! Test helper updated for new API.The
child()helper function correctly includes the newkeyparameter when creatingChild.Createdinstances, using the configuration value as the key (appropriate for test scenarios).decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (1)
8-8: LGTM! Excellent addition of key uniqueness validation.The changes enhance test coverage by:
- Making
before()open for subclass customization- Adding
assertKeysUnique()to validate the invariant that all child keys must be unique- Leveraging the new
keystest utilityThis validation directly addresses the crash mentioned in the PR description ("Key XYZ was used multiple times") by ensuring tests catch duplicate keys early.
Also applies to: 20-20, 37-37, 112-117
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt (1)
20-37: LGTM! Constructor updated for String-typed keys.The implementation correctly:
- Adds a 3-argument constructor accepting
key: Stringand passes it toChild.Created- Updates the 2-argument constructor to delegate to
Child.Createdwith the appropriate parameters- Includes a TODO note for future visibility restriction of the 2-argument form (reasonable planning)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
258-279: LGTM! Clean migration of test helpers to String-based keys.The helper functions
defaultStackAnimation(),child(), andstack()are correctly updated to useStringkeys, consistent withSimpleStackAnimationTest.ktand the broader API changes. All test cases properly use string literals for key comparisons.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
203-229: LGTM! Test infrastructure correctly migrated to String-based keys.The test helpers (
setContent(),routerState(),child()) are properly updated to useStringkeys, and thechild()implementation aligns with the pattern inChildrenTest.kt. The default content lambdaChild(name = it.key)correctly leverages the string key for display.decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt (3)
39-42: Good binary compatibility strategy.The deprecated
copyoverload acceptingAnyand converting viakey.toString()maintains binary compatibility for existing callers while migrating to the newStringtype.
44-63: Consistent implementation forDestroyedclass.The
Destroyedclass mirrors the same pattern asCreated:Stringkey,hashString()derivation, and binary-compatible deprecatedcopyoverload. The implementation is consistent.
29-33:hashString()extension is properly defined and suitable for key generation.The
hashString()extension function exists inUtils.ktand produces stable identifiers by combining the class name with a base-36 representation of the hash code. This is appropriate for deriving configuration keys.decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (3)
17-23: Good test setup enabling duplicate configurations.The
@BeforeTestmethod properly enablesduplicateConfigurationsEnabledto test edge cases where multiple children share the same configuration but require distinct keys.
166-186: Excellent shuffle and lifecycle tests.These tests verify that keys remain stable when children are reordered or their lifecycle states change, which is critical for animation correctness and state preservation.
188-234: Comprehensive recreation and configuration change tests.The tests validating key persistence across
recreate()calls (both with and without configuration changes) and with duplicate configurations ensure the fix for "Key XYZ was used multiple times" crash is properly covered.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (2)
33-175: Test migration toStringkeys is consistent and aligns with the new API.
All updated call sites now drive identity viaChild.Created.key(e.g.,routerState("A", "B")), which matches the PR intent and should better cover the “duplicate key” regression surface.
247-261: ParameterizedStackAnimation<String, Any>?coverage looks good post-migration.
Keeps broad animation coverage while exercising the new key type end-to-end.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (3)
37-46: Key propagation intoChild.Created/Destroyedis the right fix direction for Compose identity.
Mappingitem.keythrough to publicChildobjects ensures downstream UI can reliably key content onChild.key.
125-137:saveChildState()now persisting(key, state)is the needed behavioral change.
This directly supports stable restoration and prevents accidental key regeneration across recreation.
160-201: Key lifecycle looks consistent: reuse on resurrect, generate on first sight, preserve on destroy.
- Line 181 reuses
child.keywhen transitioning Destroyed→Created.- Line 190/196 generates new keys for truly new items.
- Line 236 preserves the key when moving Created→Destroyed.
This aligns with “stable per-item identity” rather than “derived from configuration”.Also applies to: 231-237
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt
Show resolved
Hide resolved
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
child-key-3
branch
from
9bf763a to
ce9f8dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
70-97: Restore can silently drop children ifsavedStates.size != navState.children.size(due tozip)
This should be a hard failure (or a deliberate fallback) to avoid partially restored navigators and hard-to-debug state corruption.private fun restore(navState: N, savedStates: List<SavedChildState>) { + check(savedStates.size == navState.children.size) { + "Saved child state size mismatch: savedStates=${savedStates.size}, navState.children=${navState.children.size}" + } + val retainedItems = items.takeUnless(List<*>::isEmpty)?.mapIndexed(::Pair)?.toMap(HashMap()) items.clear() navState.children.zip(savedStates).forEachIndexed { index, (childNavState, savedState) -> items += restoreItem( status = childNavState.status, getDestroyedItem = { Destroyed( configuration = childNavState.configuration, key = savedState.key, savedState = savedState.savedState, ) }, getCreatedItem = { childItemFactory( configuration = childNavState.configuration, key = savedState.key, savedState = savedState.savedState, instanceKeeperDispatcher = retainedItems?.remove(index)?.instanceKeeperDispatcher, ) } ) }
♻️ Duplicate comments (3)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
44-45: Re-check whetherExperimentalUuidApiopt-in is still necessary (prior discussion).If this repo’s supported Kotlin/stdlib no longer marks
Uuid.random()as experimental, you can drop the opt-in + import:-import kotlin.uuid.ExperimentalUuidApi import kotlin.uuid.Uuid ... -@OptIn(ExperimentalUuidApi::class) internal fun randomKey(): String = Uuid.random().toString()In Kotlin 2.2+ stdlib, is `kotlin.uuid.Uuid.random()` still annotated with `@ExperimentalUuidApi`? Please cite the official API docs for the Kotlin version used by this repo.decompose/api/jvm/decompose.api (1)
5-45: JVM API surface changes match the newkey: Stringmodel
This aligns with the intended promotion to stableStringkeys.Also applies to: 548-553
decompose/api/android/decompose.api (1)
5-45: Android API surface changes match the newkey: Stringmodel
Looks consistent with the JVM/KLIB dumps.Also applies to: 568-573
🧹 Nitpick comments (4)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
272-279: Harden test helper against emptystack()inputs.
stack(vararg keys: String)useskeys.last()and will throw on empty input; adding a guard makes failures clearer if future tests call it incorrectly.private fun stack(vararg keys: String): ChildStack<String, Any> = - ChildStack( - active = child(keys.last()), - backStack = keys.dropLast(1).map(::child), - ) + ChildStack( + active = child( + keys.lastOrNull() + ?: error("stack() must be called with at least one key") + ), + backStack = keys.dropLast(1).map(::child), + )decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt (1)
20-25: (Optional) Consider rejecting blank keys at the factory boundary.override fun invoke( configuration: C, key: String, savedState: SerializableContainer?, instanceKeeperDispatcher: InstanceKeeperDispatcher? ): ChildItem.Created<C, T> { + require(key.isNotBlank()) { "ChildItem key must not be blank" } val componentLifecycleRegistry = LifecycleRegistry() ... }extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
216-220: HardenrouterState(vararg keys)against empty input (clearer failures).private fun routerState(vararg keys: String): ChildStack<String, Any> = ChildStack( - active = child(key = keys.last()), + active = child( + key = keys.lastOrNull() + ?: error("routerState() must be called with at least one key") + ), backStack = keys.dropLast(1).map { child(key = it) }, )decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt (1)
21-33: Defaulting key viaconfiguration.hashString()can still collide / be unstable depending on implementation
IfChild.Created(configuration, instance)/Child.Destroyed(configuration)are used outside tests, a hash-based default can still produce duplicates (and may not be stable across versions/platforms if it’s derived fromhashCode). Consider strongly discouraging use (KDoc / deprecate) or making these constructors test-only sooner than 4.0.Also applies to: 44-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt(2 hunks)decompose/api/android/decompose.api(2 hunks)decompose/api/decompose.klib.api(2 hunks)decompose/api/jvm/decompose.api(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt(4 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt(7 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt(2 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt(2 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt(0 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt(4 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt(1 hunks)decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt(2 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt(5 hunks)extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt(2 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt(1 hunks)extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt(3 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt(4 hunks)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt(11 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt(2 hunks)extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt(3 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt(4 hunks)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt(4 hunks)
💤 Files with no reviewable changes (1)
- decompose/src/commonTest/kotlin/com/arkivanov/decompose/KeyHashStringTest.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt
- decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/TestStackRouter.kt
- decompose-test-utils/src/commonMain/kotlin/com/arkivanov/decompose/testutils/TestUtils.kt
- extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStack.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt
- extensions-android/src/main/java/com/arkivanov/decompose/extensions/android/stack/StackRouterView.kt
- extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/pages/ChildPages.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
📚 Learning: 2025-11-09T18:14:31.048Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 954
File: decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt:38-44
Timestamp: 2025-11-09T18:14:31.048Z
Learning: The Child.key property in Decompose is marked with ExperimentalDecomposeApi, so changes to its runtime value (e.g., from configuration to ItemKey) are acceptable breaking changes within the experimental API surface.
Applied to files:
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.ktdecompose/api/decompose.klib.apidecompose/api/android/decompose.apiextensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.ktdecompose/api/jvm/decompose.apidecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt
📚 Learning: 2025-05-28T22:32:21.630Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 880
File: docs/extensions/compose.md:429-431
Timestamp: 2025-05-28T22:32:21.630Z
Learning: In Decompose documentation, the correct import path for `subscribeAsState` in the Child Items navigation example is `com.arkivanov.decompose.extensions.compose.subscribeAsState`, not the jetbrains-specific variant. The import paths vary depending on the specific compose variant being used (general compose vs jetbrains/multiplatform compose).
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.ktextensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.ktextensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.ktdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt
📚 Learning: 2025-08-31T18:04:37.338Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:78-85
Timestamp: 2025-08-31T18:04:37.338Z
Learning: In Android Vanilla Ice Cream predictive back animations, only the exitModifier should honor the swipe edge direction. The enterModifier intentionally uses a consistent negative translationX offset (-size.width * 0.2F) regardless of the swipe edge, as per the official Android specification.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
📚 Learning: 2025-08-31T18:07:47.010Z
Learnt from: arkivanov
Repo: arkivanov/Decompose PR: 923
File: extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/predictiveback/AndroidPredictiveBackAnimatableV2.kt:113-121
Timestamp: 2025-08-31T18:07:47.010Z
Learning: In Android Vanilla Ice Cream predictive back animations, the exit translationX stop value in the lerp should be consistently positive (width * 0.2F) regardless of swipe edge direction, as per the official Android specification. This is intentional design and should not be mirrored for right-edge gestures.
Applied to files:
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt
🧬 Code graph analysis (3)
extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (4)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/stack/ChildStackTest.kt (1)
child(31-32)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
child(272-273)extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (1)
child(115-116)extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/PredictiveBackGestureTest.kt (1)
child(521-522)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (3)
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (2)
stateOf(98-99)navigate(91-93)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt (1)
navigate(209-215)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
navigate(139-142)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
randomKey(44-45)
🪛 Gitleaks (8.30.0)
extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt
[high] 405-405: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 406-406: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 407-407: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 408-408: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build on macOS
- GitHub Check: Build on Linux
🔇 Additional comments (19)
extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/animation/SimpleStackAnimationTest.kt (2)
30-76: LGTM! String key migration is consistent.The type parameter change from
SimpleStackAnimation<Int, Any>toSimpleStackAnimation<String, Any>is correctly propagated throughout the test. All integer key literals have been consistently replaced with string equivalents ("1", "2"), and the test logic remains unchanged.
115-122: LGTM! Helper functions correctly updated.The
childandstackhelper functions have been properly migrated to use string keys, with signatures and return types consistently updated. The explicit passing of thekeyparameter toChild.Created(line 116) aligns with the PR's goal of promoting string keys to stable.extensions-compose/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/stack/ChildrenTest.kt (1)
34-262: LGTM! Test migration to String keys is thorough and correct.The test suite has been successfully updated to use String-based keys throughout. The coverage includes various navigation scenarios (push/pop, state restoration, disposal), and the string keys are clear and maintainable. The helper methods appropriately propagate the String key type through test infrastructure.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenFactory.kt (1)
272-272: LGTM! Internal state persistence updated correctly.The change from
List<SerializableContainer?>toList<ChildrenNavigator.SavedChildState>aligns with the per-item key persistence mechanism introduced in this PR. Since this is a private class, the change has no impact on public API consumers.extensions-compose/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/stack/Children.kt (1)
32-76: LGTM! Compose state saving simplified with String keys.The migration from
keyHashStringto directkeyusage is cleaner and more straightforward. TheSaveableStateProvidernow receives the String key directly, eliminating the need for hash string conversion. The internalKeysclass correctly updated toSet<String>.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItemFactory.kt (1)
8-13: LGTM! Factory interface correctly extended with String key parameter.The addition of the
key: Stringparameter to the factory interface enables proper key propagation during child creation. The parameter placement (after configuration, before optional parameters) is logical and maintains good API ergonomics.decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenTestBase.kt (1)
37-117: LGTM! Enhanced test infrastructure with key uniqueness validation.The addition of
assertKeysUnique()provides essential validation for the String key approach, ensuring no duplicate keys exist across children. Integrating this check intoassertChildren()means it runs automatically for all tests using this base class, catching key collision issues early. Makingbefore()open allows subclasses to extend initialization as needed.extensions-compose-experimental/src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/experimental/panels/ChildPanels.kt (1)
403-408: LGTM! Panel children correctly updated with String keys.The changes properly propagate String keys through panel child creation. The explicit keys for
EmptyChildplaceholders ("empty-child-1"through"empty-child-4") provide stable identifiers for state restoration.Note: The static analysis warnings about "Generic API Key" are false positives—these are clearly just identifier strings for empty panel placeholders, not actual API keys or secrets.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/ChildPanelsFactory.kt (1)
223-231: LGTM! Panel state mapper correctly preserves String keys.The
stateMapperhas been properly updated to extract and propagate keys alongside configuration and instance. The pattern of pairing instances with their keys and then deconstructing forChild.Createdconstruction ensures keys are preserved through the panel creation flow.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStack.kt (1)
20-37: LGTM! ChildStack constructors updated to String keys.The primary constructor correctly accepts
key: Stringinstead ofAny, aligning with the PR objective to promote String keys to stable API. The 2-argument constructor now relies onChild.Createdwithout an explicit key parameter, with a TODO noting it should be marked@VisibleForTestingin version 4.0—a reasonable future cleanup item.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/animation/DefaultStackAnimationTest.kt (1)
27-68: String-key migration in tests is consistent and readable.Also applies to: 70-257, 258-263, 272-273
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/DefaultChildItemFactory.kt (1)
20-52: Key propagation is correctly wired intoChildItem.Created.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildItem.kt (1)
11-41:ChildItemnow carrieskey: Stringconsistently across Created/Destroyed.extensions-compose-experimental/src/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/experimental/stack/ChildStackTest.kt (1)
35-284: Test updates align with String-key semantics and cover the important “same key, new instance” case.decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt (2)
25-187: Coverage is solid for “add/remove/replace/shuffle” key stability + uniqueness (incl. duplicate configurations).
188-234: Good coverage for key stability across configuration-change vs full recreate.decompose/src/commonMain/kotlin/com/arkivanov/decompose/Child.kt (1)
35-42: Binary-compatcopy(..., key: Any)shims look correct
Coercing legacyAnykeys viatoString()is a reasonable compatibility bridge and keeps call-sites linkable.Also applies to: 56-63
decompose/api/decompose.klib.api (1)
411-414: KLIB ABI dump updates look consistent withkey: Stringpromotion
No issues spotted in the shown declarations.Also applies to: 520-566
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/children/ChildrenNavigator.kt (1)
125-137: TheSavedChildStatedata class with@Serializableand nullableSerializableContainer?field works correctly across all Kotlin Multiplatform targets without requiring@Contextualor custom serializers.SerializableContaineris designed by Essenty to work seamlessly with kotlinx-serialization across JVM, iOS, Web, and other targets. The codebase demonstrates this through multiple working examples: sample applications successfully serialize/deserialize state on JVM and iOS, test code usesSerializableContainer.serializer()across targets, and no compilation or runtime issues exist. No changes needed.
decompose/src/commonTest/kotlin/com/arkivanov/decompose/router/children/ChildrenKeysTest.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
child-key-3
branch
from
ce9f8dd to
91fc658
Compare
This should fix the occasional "Key XYZ was used multiple times" crash in ChildStack composable.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.