Added the new web history API with nested navigation support #821
Conversation
WalkthroughThe pull request introduces significant enhancements to 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: 13
🧹 Outside diff range and nitpick comments (50)
sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt (1)
Line range hint
112-117: Consider adding test cases for deep linking functionality.The new web navigation feature lacks test coverage. Consider adding test cases that:
- Verify component initialization with various deep link URL formats
- Test navigation state when deep links are provided
- Validate the interaction between deep links and tab navigation
Here's a suggested test case structure:
@Test fun WHEN_created_with_deepLink_THEN_correct_tab_active() { val component = createComponent( deepLinkUrl = "/tabs/counters" // Add appropriate URL format ) component.stack.assertActiveInstance<CountersChild>() } @Test fun WHEN_created_with_invalid_deepLink_THEN_defaults_to_menu() { val component = createComponent( deepLinkUrl = "/invalid/path" ) component.stack.assertActiveInstance<MenuChild>() }sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt (1)
14-18: Consider documenting experimental API usage.The implementation looks correct, but since this component uses experimental features:
- Consider adding KDoc comments explaining that this component uses experimental web navigation features
- Document any potential limitations or behavioral differences of the NoOp implementation
Here's a suggested documentation addition:
+/** + * Preview implementation of RootComponent that provides a no-op web navigation. + * Note: Uses experimental Decompose API for web navigation support. + */ @OptIn(ExperimentalDecomposeApi::class) class PreviewRootComponent : RootComponent, ComponentContext by PreviewComponentContext, WebNavigationOwner.NoOp {sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt (1)
6-7: LGTM! Good architectural improvementsThe changes to use ID-based selection instead of indices and making
imagesan override property are good architectural improvements. This approach:
- Makes the component more suitable for web navigation (IDs are more stable than indices)
- Improves interface compliance with explicit override
- Provides better separation of concerns by injecting images instead of generating them internally
sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt (2)
17-47: Consider extracting the base URL constant and improving test names.The test methods could benefit from some minor improvements:
- Extract the duplicated base URL into a constant
- Consider making test names more URL-centric since we're testing URL-based navigation
class RootComponentIntegrationTest { + private companion object { + private const val BASE_URL = "https://example.com" + } - fun WHEN_created_with_deeplink_empty_THEN_TabsChild_active() { + fun WHEN_created_with_root_url_THEN_TabsChild_active() { - val component = createComponent(deepLink = Url(url = "https://example.com")) + val component = createComponent(deepLink = Url(url = BASE_URL))
Line range hint
1-61: Consider documenting the DeepLink to Url migration path.This change represents a significant API transition from
DeepLinktoUrl. Consider adding migration documentation or deprecation warnings to help users transition their existing code.Suggestions:
- Add a migration guide in the documentation
- Consider providing a compatibility layer or utility functions to help users migrate from
DeepLinktoUrl- Document any behavioral differences between the old and new APIs
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt (1)
20-29: Consider handling additional edge cases in snakeCase()The implementation works for basic camelCase to snake_case conversion, but could be enhanced to handle more edge cases.
Consider this improved implementation:
internal fun String.snakeCase(): String = buildString { + if (isEmpty()) return@buildString + var previousIsUpperCase = false for (c in this@snakeCase) { - if (c.isUpperCase() && isNotEmpty()) { + val isUpperCase = c.isUpperCase() + if (isUpperCase && isNotEmpty() && (!previousIsUpperCase || + (previousIsUpperCase && this@snakeCase.getOrNull(this@snakeCase.indexOf(c) + 1)?.isLowerCase() == true))) { append('_') } - append(c.lowercaseChar()) + previousIsUpperCase = isUpperCase } }This handles:
- Consecutive uppercase letters (e.g., "XMLParser" -> "xml_parser")
- Acronyms followed by words (e.g., "myXMLParser" -> "my_xml_parser")
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt (1)
Line range hint
14-24: Consider documenting the NoOp behaviorWhile the implementation is correct, it would be helpful to add a KDoc comment explaining why this preview component implements
WebNavigationOwner.NoOpand what that means for testing/preview purposes.Here's a suggested addition:
@OptIn(ExperimentalDecomposeApi::class) +/** + * A preview implementation of TabsComponent that implements WebNavigationOwner.NoOp + * to indicate it doesn't handle web navigation events, as it's intended for testing + * and preview purposes only. + */ class PreviewTabsComponent : TabsComponent, WebNavigationOwner.NoOp {sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt (1)
Line range hint
8-16: Consider documenting web navigation behaviorThe interface maintains good separation of concerns while adding web navigation support. Consider adding KDoc comments to document:
- How web navigation interacts with page selection
- Expected behavior when navigating through browser history
- Implementation requirements for WebNavigationOwner
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt (1)
21-25: Consider using annotations or constants for route pathsWhile the routing comments provide valuable information, consider:
- Using annotations or constants to define routes instead of comments
- Adding route documentation for all Child variants for consistency
- Adding validation to ensure route paths are unique
This would make the routing configuration more maintainable and type-safe.
Example implementation:
sealed class Child { @Route("/tabs") class TabsChild(val component: TabsComponent) : Child() @Route("/dynamic-features") class DynamicFeaturesChild(val component: DynamicFeaturesComponent) : Child() // ... similar for other variants }sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt (1)
11-12: Add KDoc documentation for the WebDocumentTitle function.The function signature looks good, but adding documentation would help explain its purpose and usage pattern in the web navigation context.
Add documentation like:
+/** + * Sets the document title in web environments. Has no effect on other platforms. + * + * @param title The title to be displayed in the browser's title bar or tab + */ @Composable internal expect fun WebDocumentTitle(title: String)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt (2)
27-27: Consider parameterizing the title.The hardcoded title "Counters" could be made more flexible:
Consider this approach:
-internal fun CountersContent(component: CountersComponent, modifier: Modifier = Modifier) { - WebDocumentTitle(title = "Counters") +internal fun CountersContent( + component: CountersComponent, + modifier: Modifier = Modifier, + title: String = "Counters" +) { + WebDocumentTitle(title = title)This would allow for localization and dynamic titles while maintaining the default behavior.
Line range hint
27-33: Ensure title changes are announced to screen readers.When the document title changes, screen reader users should be notified.
Consider implementing an accessibility service that announces title changes to screen readers. This could be done by:
- Creating a common interface for accessibility announcements
- Implementing platform-specific announcement mechanisms
- Integrating with the WebDocumentTitle component
Would you like me to provide a detailed implementation for this accessibility enhancement?
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt (1)
20-20: LGTM! Consider standardizing the title format.The implementation of WebDocumentTitle is correct and well-placed. However, for consistency with other components (e.g., "Shared Transitions Gallery"), consider using a more descriptive title.
- WebDocumentTitle(title = "Menu") + WebDocumentTitle(title = "Decompose Sample Menu")Also applies to: 27-28
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt (1)
20-20: Consider formatting the photo title for better user experience.While using a dynamic title is good, using resourceId.name directly might expose implementation details or show unfriendly names to users.
Consider formatting the title more user-friendly:
- WebDocumentTitle(title = "Shared Transitions ${component.image.resourceId.name}") + WebDocumentTitle(title = "Shared Transitions - Photo ${component.image.id}")Also applies to: 29-30
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt (1)
Line range hint
20-34: Consider centralizing web document title management.The current implementation requires each component to manage its own document title. Consider introducing a centralized title management system that could:
- Enforce consistent title formatting across components
- Handle title updates through the navigation system
- Support hierarchical titles for nested navigation
- Provide type-safe title templates
This would align better with the PR's objective of supporting nested navigation.
Example approach:
object WebTitleTemplate { const val MAIN = "Decompose Sample - {0}" const val PHOTO = "Shared Transitions - Photo {0}" // ... other templates } @Composable fun rememberWebTitle(template: String, vararg args: Any) { WebDocumentTitle(title = template.format(*args)) }sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt (1)
37-37: Consider making the document title dynamic based on navigation stateSince this is a multi-pane layout with nested navigation, consider making the title dynamic to reflect the current navigation state (e.g., "Multi-Pane Layout - Article Title - Author Name"). This would improve the web navigation experience by providing more context in browser history.
Example implementation:
- WebDocumentTitle(title = "Multi-Pane Layout") + val panels by component.panels.subscribeAsState() + val title = buildString { + append("Multi-Pane Layout") + panels.activeChild?.instance?.let { details -> + append(" - ${details.article.title}") + details.author?.let { author -> + append(" - ${author.name}") + } + } + } + WebDocumentTitle(title = title)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt (1)
59-59: Consider making the document title dynamic based on stack stateSince this is a card stack with navigation, consider making the title dynamic to reflect the current card state (e.g., "Cards (3/5)"). This would improve the web navigation experience by providing more context in browser history.
Example implementation:
- WebDocumentTitle(title = "Cards") + val stack by component.stack.subscribeAsState() + val title = "Cards (${stack.items.size})" + WebDocumentTitle(title = title)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt (2)
38-38: Consider adding documentation for thedeepLinkUrlparameter.The constructor now includes an optional
deepLinkUrl: Url?parameter. Adding KDoc comments to explain its purpose and usage will enhance code readability and assist other developers in understanding how to initialize the component with deep links.
123-131: Add unit tests for thegetInitialPanelsfunction.The
getInitialPanelsfunction initializes panels based on thedeepLinkUrl. To ensure correct behavior across different scenarios (e.g., null URLs, valid parameters, invalid parameters), consider adding unit tests that cover these cases.Would you like assistance in creating unit tests for this function?
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt (1)
29-29: Consider renaming_pagesto align with Kotlin naming conventionsIn Kotlin, it's conventional to avoid leading underscores in property names. Consider renaming
_pagestopagesValueorpagesStatefor clarity and consistency with idiomatic Kotlin code.sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt (3)
77-78: Consider passingdeepLinkUrlwhen pushing newConfiginstances.In the navigation actions for
onPagesItemSelectedandonSharedTransitionsItemSelected, you're pushing new configurations without passing thedeepLinkUrl. If deep linking is relevant, consider passingdeepLinkUrlto maintain consistency and ensure proper navigation.Apply this diff to pass the
deepLinkUrl:onPagesItemSelected = { - nav.pushNew(Config.Pages()) + nav.pushNew(Config.Pages(deepLinkUrl = config.deepLinkUrl)) }, onSharedTransitionsItemSelected = { - nav.pushNew(Config.SharedTransitions()) + nav.pushNew(Config.SharedTransitions(deepLinkUrl = config.deepLinkUrl)) },
126-135: Handle unrecognized paths explicitly ingetInitialStack.In the
elseclause of yourwhenexpression, an unrecognized path leads toConfig.Tabs(deepLinkUrl = childUrl). Consider logging a warning or handling this case explicitly to aid in debugging and to improve the user experience when an unknown path is encountered.
Line range hint
141-153: Maintain consistency inConfigimplementations.While
Tabs,Pages, andSharedTransitionsare now data classes with adeepLinkUrlparameter,DynamicFeaturesandCustomNavigationremain data objects. For consistency, consider converting them to data classes, even if they don't currently have parameters. This can simplify future expansions and maintain a uniform code structure.Apply this diff to update the implementations:
@Serializable -data object DynamicFeatures : Config +data class DynamicFeatures(val deepLinkUrl: Url? = null) : Config @Serializable -data object CustomNavigation : Config +data class CustomNavigation(val deepLinkUrl: Url? = null) : Configdecompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt (6)
38-38: Typographical Error in Parameter DescriptionIn the KDoc for the
enableHistoryparameter, there's a typographical error. The word "enabled" should be "enables" to match the present tense.Apply this diff to correct the typo:
- * @param enableHistory a flag that enabled or disables the back stack history. + * @param enableHistory a flag that enables or disables the back stack history.
39-41: Clarify Sentence Structure for Better ReadabilityThe sentence explaining the
enableHistoryparameter can be broken into two sentences for better clarity and readability.Apply this diff to improve the documentation:
- * When `false`, the browser navigation history follows only the currently - * active child (i.e. the back stack will not be taken into account), can be useful - * for bottom tab navigation. Default value is `true`. + * When `false`, the browser navigation history follows only the currently + * active child (i.e., the back stack will not be taken into account). This can be useful + * for bottom tab navigation. + * Default value is `true`.
53-53: Improve Readability in DocumentationIn the KDoc, replace "e.g." with "e.g.," to follow standard punctuation conventions.
Apply this diff:
- * Can be used e.g. to display an alert dialog before closing a screen. Returns `true` + * Can be used, e.g., to display an alert dialog before closing a screen. Returns `true`
63-68: Specify a More Precise Return TypeThe function
childStackWebNavigationcurrently returnsWebNavigation<*>. For better type safety and consistency, consider specifying the generic typeCin the return type.Apply this diff to enhance type safety:
- ): WebNavigation<*> = + ): WebNavigation<C> =
94-101: Optimize Mapping Logic intoHistoryFunctionConsider simplifying the
toHistoryfunction by directly mapping over the desired list without the conditional expression.Apply this diff to streamline the code:
- private fun ChildStack<C, T>.toHistory(): List<HistoryItem<C>> = - (if (enableHistory) items else items.takeLast(1)).map { child -> + private fun ChildStack<C, T>.toHistory(): List<HistoryItem<C>> = + val relevantItems = if (enableHistory) items else items.takeLast(1) + relevantItems.map { child -> HistoryItem( path = pathMapper(child) ?: "", parameters = parametersMapper(child) ?: emptyMap(), key = child.configuration, child = childSelector(child), ) }
106-108: Add Error Handling for NavigationConsider adding error handling or validation when navigating with the provided
historylist to ensure that the navigator handles unexpected input gracefully.Apply this diff to include validation:
override fun navigate(history: List<C>) { + if (history.isEmpty()) { + // Handle empty history case if necessary + return + } navigator.navigate { history } }sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
32-32: Consider removingconsole.logstatement from production codeThe
console.log(root.stack.value)statement may be intended for debugging purposes. Consider removing it to avoid unnecessary console output in production.sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (2)
48-48: Be cautious with experimental APIsThe use of
@OptIn(ExperimentalDecomposeApi::class)indicates reliance on experimental features that may change in future releases. Ensure that this dependency is acceptable and consider the potential need for future adjustments.
100-100: Reminder to address the TODO commentThere is a TODO comment indicating incomplete handling of the
childUrlin the path processing withingetInitialConfig. Ensure this logic is implemented to correctly support deep linking functionality.Would you like assistance in implementing the path handling logic?
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt (1)
47-70: Simplify generic type parameters for better readabilityThe function
childPanelsWebNavigationuses multiple generic type parameters (MC,MT,DC,DT,EC,ET), which increases complexity. Consider simplifying or constraining these generics if possible to enhance readability and maintainability.decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (1)
133-176: Refactor for improved readability and maintainabilityThe
subscribemethod is quite complex with deep nesting and multiple responsibilities. Consider refactoring it into smaller, well-named functions or classes to enhance readability and facilitate easier future modifications.decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt (2)
6-7: Use standard naming conventions for test methodsThe test method names contain underscores, e.g.,
WHEN_created_with_no_items_in_root_THEN_ISE_thrown. Kotlin supports backticks for method names with spaces, which can improve readability. Alternatively, consider using camelCase to align with Kotlin conventions.
9-766: Consider grouping related tests and adding commentsThere are numerous test cases covering various scenarios. Grouping related tests and adding brief comments or documentation can improve the understandability of the test suite.
decompose/api/jvm/decompose.api (2)
184-198: Confirm visibility of generated serializer classesThe generated serializer classes like
Pages$$serializerandPanels$$serializerare marked aspublic synthetic. If they are not intended for public API consumption, consider marking them asinternalto restrict their visibility.
501-527: Verify platform-specific placement ofWebNavigationinterfaceThe
WebNavigationinterface is added in the JVM-specific API. Since web navigation can be relevant for multiple platforms, consider moving it to a common module to ensure cross-platform availability.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt (1)
3-3: LGTM! Well-structured serialization support for web navigation.The addition of
@Serializableannotation is a clean and non-breaking change that enables state persistence for web navigation history. The existing validation logic in the init block ensures data integrity is maintained during serialization/deserialization.This change forms a crucial part of the web history API, allowing navigation state to be preserved in the browser's history stack. The serialization capability will be particularly valuable for handling browser refresh and back/forward navigation.
Also applies to: 12-12
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt (1)
6-6: Consider documenting web navigation behavior in interface KDoc.The addition of
WebNavigationOwnercleanly extends the component's capabilities. However, since this is using experimental API and implements complex navigation patterns, it would be beneficial to document:
- How web navigation interacts with back handling
- Expected URL structure for the multi-pane layout
- State preservation behavior during navigation
Would you like me to provide a template for the interface documentation?
Also applies to: 14-14
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt (1)
4-4: LGTM! Consider adding documentation for web navigation.The extension of
WebNavigationOwneraligns well with the PR objectives for web history API support. The change is clean and maintains the interface's existing functionality.Consider adding KDoc comments to document the web navigation capabilities and any specific requirements or behaviors that implementers should be aware of.
Also applies to: 11-11
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt (2)
28-29: Consider performance optimization for path segment consumption.The current implementation creates a new list on every
consumePathSegmentcall.Consider using a more efficient data structure or caching mechanism if this operation is frequently used:
-fun Url.consumePathSegment(): Pair<String?, Url> = - pathSegments.firstOrNull() to copy(pathSegments = pathSegments.drop(1)) +fun Url.consumePathSegment(): Pair<String?, Url> { + val first = pathSegments.firstOrNull() ?: return null to this + return first to Url( + pathSegments = pathSegments.subList(1, pathSegments.size), + parameters = parameters + ) +}
32-36: Document internal utility functions.The internal path utility functions lack documentation explaining their purpose and usage.
Add KDoc comments:
+/** + * Converts the class name of the receiver to snake_case format. + * Internal utility for generating URL path segments. + */ internal fun Any.path(): String = this::class.simpleName?.snakeCase() ?: "" +/** + * Converts the class name of type T to snake_case format. + * Internal utility for generating URL path segments. + * @return The snake_case representation of T's class name + */ internal inline fun <reified T : Any> pathSegmentOf(): String = T::class.simpleName?.snakeCase() ?: ""sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt (1)
21-23: LGTM! Consider documenting the NoOp implementation choice.The implementation of
WebNavigationOwner.NoOpis appropriate for a preview component.Consider adding a comment explaining why
WebNavigationOwner.NoOpis used here, to help other developers understand the design decision:/** * Preview implementation of MultiPaneComponent that implements WebNavigationOwner.NoOp * since navigation is not needed in preview/testing scenarios. */decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (3)
14-20: Enhance documentation with usage examples and deep linking formatWhile the documentation explains the basic purpose, it would be beneficial to include:
- Example usage code
- Expected deep link format
- State persistence behavior
25-33: Consider potential race condition in state restorationThe state restoration logic might have a race condition if multiple tabs are opened simultaneously. Consider adding a tab-specific identifier to the storage keys.
35-40: Consider using a more robust state persistence mechanismThe current implementation uses sessionStorage which is cleared when the browser session ends. For better user experience, consider:
- Adding an option to use localStorage for persistent state
- Implementing a cleanup mechanism for old states
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt (3)
12-16: LGTM! Consider adding KDoc documentation.The class constructor is well-designed with sensible defaults for testing. Consider adding KDoc documentation to describe the purpose of this test implementation and its parameters.
+/** + * Test implementation of WebNavigation interface that uses Integer configurations. + * + * @param initialHistory The initial navigation history to set up + * @param onBeforeNavigate Callback to control navigation permission, defaults to always allow + * @param childFactory Factory function to create child navigations, defaults to no children + */ class TestWebNavigation( initialHistory: List<Int>, private val onBeforeNavigate: () -> Boolean = { true }, private val childFactory: (Int) -> TestWebNavigation? = { null }, ) : WebNavigation<Int> {
35-56: Consider handling edge cases in navigation.The navigation logic could be more robust by:
- Validating input history list (null check, empty check)
- Handling potential exceptions during config.toString()
- Adding bounds checking for large history lists
override fun navigate(history: List<Int>) { + requireNotNull(history) { "History list cannot be null" } + children.keys.removeAll { it !in history } history.forEach { config -> if (config !in children) { val child = childFactory(config) if (child != null) { children[config] = child } } } _history.value = history.map { config -> HistoryItem( key = config, path = config.toString(), parameters = emptyMap(), child = children[config]?.let(TestWebNavigation::Owner), ) } }
58-82: Enhance assertion error messages for better debugging.The assertions would benefit from more descriptive error messages to help identify test failures quickly.
fun assertHistory(configs: Iterable<Int>) { - assertContentEquals(configs, _history.value.map { it.key }) + assertContentEquals( + configs, + _history.value.map { it.key }, + "Navigation history mismatch: expected=$configs, actual=${_history.value.map { it.key }}" + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (51)
decompose/api/android/decompose.api(6 hunks)decompose/api/decompose.klib.api(6 hunks)decompose/api/jvm/decompose.api(6 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt(1 hunks)decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt(2 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt(1 hunks)sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt(1 hunks)sample/app-js-compose/src/jsMain/resources/index.html(1 hunks)sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt(2 hunks)sample/app-js/src/main/resources/index.html(1 hunks)sample/shared/compose/build.gradle.kts(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt(1 hunks)sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt(1 hunks)sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt(4 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt(5 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt(3 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt(0 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt(6 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt(1 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt(2 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt
✅ Files skipped from review due to trivial changes (5)
- sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt
- sample/app-js/src/main/resources/index.html
- sample/app-js-compose/src/jsMain/resources/index.html
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt
🧰 Additional context used
🪛 detekt (1.23.7)
decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (43)
sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt (1)
112-112: LGTM! The change maintains backward compatibility.
The addition of deepLinkUrl = null aligns with the updated DefaultTabsComponent constructor while preserving existing test behavior.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt (2)
4-4: LGTM: Import additions align with web navigation implementation.
The new imports support the web navigation functionality and experimental API usage.
Also applies to: 6-6
15-18: Verify consistent web navigation implementation across preview components.
Let's ensure this implementation pattern is consistent across other preview components.
✅ Verification successful
Implementation pattern is consistent across preview components
The verification shows consistent implementation of WebNavigationOwner.NoOp across all preview components:
- PreviewRootComponent
- PreviewTabsComponent
- PreviewMultiPaneComponent
All follow the same pattern of implementing WebNavigationOwner.NoOp interface along with their respective component interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation patterns across preview components
# Expected: Other preview components should follow similar pattern for web navigation
# Search for other preview components implementing WebNavigationOwner
echo "Checking WebNavigationOwner implementation in preview components:"
rg "class Preview.*Component.*WebNavigationOwner" -A 3
# Search for NoOp usage pattern
echo "Checking NoOp implementation pattern:"
rg "WebNavigationOwner\.NoOp" -B 2 -A 2
Length of output: 4017
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt (1)
11-12: LGTM! Verify ID propagation
The implementation correctly propagates the image ID to the callback. The change from index-based to ID-based selection is consistently applied.
Let's verify that all image IDs are properly handled throughout the navigation chain:
✅ Verification successful
Let me gather more information about the image ID handling across the codebase.
ID propagation is correctly implemented
The verification confirms proper ID handling throughout the navigation chain:
Imagedata class correctly definesid: Intas a propertyDefaultGalleryComponentproperly propagates the ID fromonImageClickedtoonImageSelectedDefaultSharedTransitionsComponentcorrectly uses the ID to create a new Photo configuration (Config.Photo(id = it))- The navigation flow maintains ID consistency from gallery selection through to photo display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how image IDs are used in the navigation flow
# Look for Image class definition and ID usage
ast-grep --pattern 'class Image {
$$$
}'
# Check for other components handling image IDs
rg -A 3 "onImageSelected.*id.*Int"
Length of output: 693
Script:
#!/bin/bash
# Search for Image data class and its usages
ast-grep --pattern 'data class Image'
# Look for image ID references in function parameters and properties
rg "id: Int" -A 2
# Check for image selection handling in interfaces and implementations
rg "onImageClicked|onImageSelected" -A 2
Length of output: 7009
sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt (1)
51-59: Verify all DefaultRootComponent usages are updated.
The parameter type change from DeepLink to Url? looks good, but we should verify that all usages of DefaultRootComponent across the codebase have been updated to use the new deepLinkUrl parameter.
✅ Verification successful
All DefaultRootComponent usages have been updated correctly
The search results show that all instantiations of DefaultRootComponent across the codebase are consistent with the new parameter type. The key findings:
- The main implementation in
DefaultRootComponent.ktcorrectly definesdeepLinkUrl: Url? = null - All actual usages in application code (Android, iOS, JS, Desktop) either:
- Pass the
deepLinkUrlparameter correctly (e.g., in JS apps withdeepLinkUrl = deepLink?.let(::Url)) - Or rely on the default null value by not specifying the parameter
- Pass the
- The test implementation in
RootComponentIntegrationTest.ktcorrectly uses the new parameter name and type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining DeepLink usage in DefaultRootComponent instantiations
ast-grep --pattern 'DefaultRootComponent($$$deepLink$$$)'
# Search for all DefaultRootComponent instantiations to verify parameter usage
rg "DefaultRootComponent\(" -A 5
Length of output: 16785
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt (1)
20-29: Verify usage patterns of snakeCase() in web navigation
Since this utility is likely used for URL path segments or state serialization, let's verify its usage patterns.
✅ Verification successful
Let me gather more context about the usage patterns by examining the files where snakeCase() is used.
Based on the gathered information, I can now provide the final response:
The snakeCase() utility is correctly implemented for URL path generation
The utility is used internally for converting class names to URL path segments in a consistent manner. The implementation correctly handles:
- Converting class names to lowercase URL-friendly segments
- Adding underscores between words for readability
- Being used only in internal path generation utilities (
path()andpathSegmentOf())
The usage is well-scoped and follows web URL conventions for path segments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of snakeCase() in the codebase
echo "Searching for snakeCase() usage patterns..."
rg -l "\.snakeCase\(\)" | while read -r file; do
echo "=== $file ==="
rg -B 2 -A 2 "\.snakeCase\(\)" "$file"
done
# Look for potential web navigation related files that might use this
echo -e "\nSearching for potential web navigation files..."
rg -l "WebNavigation|WebPath|WebSerializer"
Length of output: 4017
Script:
#!/bin/bash
# Check the content of Url.kt to understand how snakeCase is used in path generation
echo "=== Content of Url.kt ==="
cat sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt
# Check for any path-related usages in web navigation files
echo -e "\n=== Checking path generation in web navigation files ==="
rg -B 3 -A 3 "path|Path" sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt
# Look for any URL path segment handling in web navigation
echo -e "\n=== Checking URL path handling in WebNavigation ==="
rg -B 3 -A 3 "url|URL|path|Path" decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt
Length of output: 8068
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt (2)
3-3: LGTM: Required imports added for web navigation support
The new imports support the web navigation functionality and experimental API usage.
Also applies to: 5-5
11-12: Verify the implications of using experimental API
The component now implements WebNavigationOwner.NoOp and uses experimental features. A few points to consider:
- The
@OptInannotation correctly acknowledges the use of experimental API - The
NoOpimplementation suggests this is a preview/test component that intentionally doesn't handle web navigation
Let's verify the consistent usage of this pattern across other preview components:
✅ Verification successful
Let me gather more information about the implementation of other preview components to verify if they consistently implement WebNavigationOwner.NoOp.
Let me check specifically for the WebNavigationOwner interface and its usage to understand the complete context.
The experimental API usage and WebNavigationOwner.NoOp implementation are correctly applied
The verification shows consistent patterns across preview components:
- All preview components that require web navigation (
PreviewRootComponent,PreviewTabsComponent,PreviewMultiPaneComponent) correctly implementWebNavigationOwner.NoOp - The
@OptIn(ExperimentalDecomposeApi::class)annotation is consistently used in all preview components - The
NoOpimplementation is documented in the source as being specifically designed for "fake or preview component implementations"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other preview components follow the same pattern
# Expected: Other preview components should also implement WebNavigationOwner.NoOp
# Search for other preview components and their implementation
ast-grep --pattern 'class Preview$_Component : $_, WebNavigationOwner.NoOp'
# Check for consistent @OptIn usage in preview components
rg -l "class Preview.*Component" | xargs rg "@OptIn.*ExperimentalDecomposeApi"
Length of output: 588
Script:
#!/bin/bash
# Check the implementation details of all preview components
rg "class Preview.*Component.*:.*" -A 2 sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/
Length of output: 4249
Script:
#!/bin/bash
# Search for WebNavigationOwner interface definition and its usages
rg "WebNavigationOwner" -A 3
# Also check for any other components that might implement NoOp
rg "WebNavigationOwner\.NoOp" -A 2
Length of output: 39462
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt (2)
4-4: LGTM: Import added correctly
The WebNavigationOwner import is properly placed and necessary for the interface extension.
8-8: Implementation aligns with web history API objectives
The addition of WebNavigationOwner interface extends PagesComponent with web navigation capabilities while maintaining its existing contract. This change supports the PR's goal of implementing nested navigation support.
Let's verify the implementation impact:
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt (3)
4-4: LGTM!
The WebNavigationOwner import is correctly added to support the new web navigation capabilities.
13-13: Verify implementations of RootComponent
The addition of WebNavigationOwner interface is aligned with the PR objectives. Let's verify that all implementing classes properly handle this new interface requirement.
✅ Verification successful
Both implementations properly handle WebNavigationOwner interface
The implementations are correctly handling the new WebNavigationOwner interface:
- DefaultRootComponent implements it by providing the required webNavigation property
- PreviewRootComponent explicitly implements WebNavigationOwner.NoOp, which is a valid implementation for preview purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of RootComponent to verify they implement WebNavigationOwner
# Search for RootComponent implementations
ast-grep --pattern 'class $_ : $$$RootComponent$$$'
Length of output: 53
Script:
#!/bin/bash
# Let's try a different approach to find RootComponent implementations
# First, let's find the file containing RootComponent definition
fd "RootComponent.kt"
# Then search for class definitions implementing RootComponent using ripgrep
# Using -A and -B for context to better understand the implementation
rg "class.*:.*RootComponent" -A 5 -B 2
Length of output: 985
Script:
#!/bin/bash
# Let's examine the actual implementations found in the shared directory
cat "sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt"
cat "sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt"
Length of output: 7685
13-26: Verify integration with WebNavigation system
Since RootComponent is a key component in the navigation hierarchy, let's verify its integration with the web history API.
✅ Verification successful
Based on the results, I can now provide a final response as the verification is complete. The implementation shows proper integration with the web navigation system:
Web navigation integration is correctly implemented
The RootComponent properly integrates with the web navigation system through:
- Correct implementation of WebNavigationOwner interface
- Proper web navigation setup in DefaultRootComponent with childStackWebNavigation
- Appropriate child component selection for navigation handling
- Clear path mapping for web navigation states
Key implementation details:
- WebNavigation is configured with proper serialization and path mapping
- Child components (Tabs, Pages, SharedTransitions) are correctly wired for web navigation
- Navigation state handling is properly implemented for the component stack
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the integration between RootComponent and WebNavigation
# Search for WebNavigation usage in relation to RootComponent
ast-grep --pattern 'class $_ {
$$$
WebNavigation$$$
$$$
RootComponent$$$
}'
# Search for navigation state handling
rg -A 5 "WebNavigation.*RootComponent"
Length of output: 126
Script:
#!/bin/bash
# Let's try a different approach to analyze the web navigation integration
# 1. First, let's find all files that implement or use WebNavigationOwner
rg "WebNavigationOwner"
# 2. Look for WebNavigation related implementations
rg -l "class.*implements.*WebNavigation"
# 3. Search for navigation state management in RootComponent implementation
fd RootComponent.kt --exec cat {}
# 4. Look for web navigation related methods and their usage
rg "fun.*navigate|handleNavigation|onNavigation" --type kotlin
Length of output: 33872
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt (1)
24-24: LGTM! Clean and consistent implementation.
The WebDocumentTitle implementation is well-placed and uses a clear, descriptive title that aligns with the component's purpose.
Also applies to: 33-34
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt (1)
32-32: LGTM: Web document title integration
The integration of WebDocumentTitle is correctly placed and matches the TopAppBar title, which is essential for web navigation history.
Also applies to: 37-37
sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt (1)
54-54: LGTM: Web document title integration
The integration of WebDocumentTitle is correctly placed and matches the TopAppBar title, which is essential for web navigation history.
Also applies to: 59-59
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt (1)
39-46: Implementation of webNavigation is correct
The use of childPagesWebNavigation to implement webNavigation enhances deep linking and web navigation support. The integration appears correct and follows the intended design.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt (1)
50-65: Verify the childSelector mapping in webNavigation.
Ensure that the childSelector function correctly handles all child types. Returning null for certain children like CustomNavigationChild and DynamicFeaturesChild should be intentional and properly managed within the navigation logic to prevent unexpected behavior.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt (1)
80-89: Ensure All Parameters Are Utilized
Verify that all constructor parameters in StackWebNavigation are utilized appropriately within the class. Specifically, ensure that enableHistory, pathMapper, parametersMapper, onBeforeNavigate, and childSelector are actively used in the class methods.
sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
24-30: Proper use of withWebHistory for web navigation
The withWebHistory function is correctly utilized to integrate web history support with the DefaultRootComponent. The stateKeeper and deepLink parameters are appropriately handled.
sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt (2)
22-22: Appropriate use of @OptIn for experimental APIs
The code correctly opts into ExperimentalComposeUiApi and ExperimentalDecomposeApi to utilize experimental features.
27-33: Correct integration of web history using withWebHistory
The withWebHistory function is properly used to embed web history support with the DefaultRootComponent. The handling of stateKeeper and deepLink parameters is appropriate.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt (1)
1-63: Well-defined WebNavigation interface for browser navigation
The WebNavigation interface is properly defined, providing essential properties and methods for managing web navigation and browser history integration. The documentation is comprehensive and enhances understandability.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt (1)
Line range hint 28-109: Enhanced deep link support improves user navigation
The addition of deepLinkUrl and the implementation of the getInitialStack function enhance user experience by allowing direct navigation to specific images via deep links.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt (1)
1-96: Implementation of PagesWebNavigation for enhanced browser history integration
The introduction of PagesWebNavigation and the childPagesWebNavigation function provides a robust mechanism for integrating page navigation with the browser's history. The code is well-structured, and the detailed documentation aids in understanding the functionality.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (1)
79-79:
Verify implications of changing Config.MultiPane from an object to a data class
Converting Config.MultiPane from an object to a data class changes it from a singleton to multiple instances. This may impact any reference equality checks or assumptions about its uniqueness. Review usage throughout the codebase to ensure this change doesn't introduce bugs.
Run the following script to find all usages of Config.MultiPane:
✅ Verification successful
Let me search for the definition and usage patterns of Config.MultiPane to better understand the context.
Let me search specifically for the Config class definition in the sample project where we see the MultiPane usage.
Let me try one more search with a different pattern to find the Config class and its usage.
Let me try one final search to see the specific file content.
No issues with Config.MultiPane being a data class
Based on the code analysis, the change from object to data class for Config.MultiPane is safe because:
- The code only uses
Config.MultiPanefor type checking (is Config.MultiPane) and instantiation - There are no reference equality checks (
===) or value equality checks (==) - The data class is used appropriately to carry a nullable
deepLinkUrlparameter - All usages are contained within this single file and follow the sealed interface pattern correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of Config.MultiPane in the codebase.
# Search for Config.MultiPane references
rg 'Config\.MultiPane\b'
Length of output: 576
Script:
#!/bin/bash
# Find the definition of Config class and MultiPane
ast-grep --pattern 'sealed class Config {
$$$
}'
# Also search for any equality checks on Config.MultiPane
rg -B2 -A2 'Config\.MultiPane.*==' || rg -B2 -A2 'Config\.MultiPane.*==='
Length of output: 1291
Script:
#!/bin/bash
# Find Config class definition in the sample project
rg -B2 -A5 'sealed class Config' sample/shared/
# Also search for any pattern matching or when expressions with Config.MultiPane
rg -B2 -A2 'when.*Config' sample/shared/
Length of output: 92
Script:
#!/bin/bash
# Search for files containing both "Config" and "MultiPane" to locate the definition
rg -l 'Config.*MultiPane|MultiPane.*Config' sample/shared/
# Then cat the contents of any found files to see the full context
fd -t f . sample/shared/ -x cat {} \; | grep -A 10 -B 10 'sealed.*Config'
Length of output: 283
Script:
#!/bin/bash
# Get the content of the specific file where Config.MultiPane is used
cat sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt
Length of output: 4919
decompose/api/android/decompose.api (5)
Line range hint 190-218: Serialization support added for Pages class looks good
The addition of the Companion object and the Pages$$serializer class correctly implements serialization for the Pages class, following Kotlin serialization conventions.
253-257: Addition of childPagesWebNavigation function enhances web navigation capabilities
The new childPagesWebNavigation function in PagesWebNavigationKt provides web navigation support for pages, effectively integrating web navigation into the pages component.
378-382: Addition of childPanelsWebNavigation function extends web navigation support
The childPanelsWebNavigation function introduced in PanelsWebNavigationKt enhances web navigation capabilities for panels, aligning with the overall navigation enhancements.
501-505: Addition of childStackWebNavigation function complements navigation features
The childStackWebNavigation function in StackWebNavigationKt provides web navigation capabilities for child stacks, complementing the other navigation enhancements.
521-547: Introduction of WebNavigation interfaces and classes enhances web navigation framework
The addition of the WebNavigation interface, HistoryItem class, and WebNavigationOwner interfaces significantly expands the web navigation API. These additions enhance the navigation capabilities and allow for better management of navigation states and history.
decompose/api/decompose.klib.api (6)
117-138: Introduction of WebNavigation interface and HistoryItem class
The addition of the WebNavigation interface and its nested HistoryItem class provides essential functionality for web navigation, including management of navigation history and serialization support.
183-192: Addition of WebNavigationOwner interface and NoOp subinterface
The WebNavigationOwner interface and its NoOp subinterface define a standard for components that provide WebNavigation instances, with NoOp offering a default no-operation implementation. This structure enhances consistency and flexibility in components' navigation capabilities.
370-390: Serialization support added for Pages class
The inclusion of the $serializer nested class and the Companion object with a serializer method adds serialization capabilities to the Pages class, adhering to Kotlin serialization patterns.
Line range hint 498-512: Addition of web navigation functions for panels, pages, and stacks
The new functions childPanelsWebNavigation, childPagesWebNavigation, and childStackWebNavigation extend web navigation support to panels, pages, and stacks, enhancing the library's navigation capabilities and providing developers with more tools for web navigation management.
556-558: Introduction of DefaultWebHistoryController for web history management
The implementation of DefaultWebHistoryController provides concrete functionality for managing web history on supported platforms, aligning with the newly introduced web navigation features and improving state management.
558-558: Addition of withWebHistory function enhances root component navigation
The withWebHistory function facilitates web browser history navigation for root WebNavigationOwner components, improving integration with web platforms and simplifying navigation management.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt (1)
1-21: Addition of WebNavigationOwner interface and NoOp subinterface
The new WebNavigationOwner interface and its NoOp subinterface define a standard for components that own web navigation, facilitating consistency and allowing for simplified implementations in cases where full navigation functionality is not required.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt (1)
4-4: Update SharedTransitionsComponent to implement WebNavigationOwner
By extending WebNavigationOwner, SharedTransitionsComponent gains web navigation capabilities, enhancing its functionality and ensuring consistency with the new navigation framework.
Also applies to: 10-10
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt (1)
Line range hint 36-43: Consider thread-safety implications of value tracking.
The subscription handling is improved with proper cancellation support, but storing the old value in a var could be problematic in concurrent scenarios. Consider using atomic references or synchronization if this code might run in a multi-threaded context.
Let's check if this code might be accessed from multiple threads:
Consider this alternative implementation using AtomicReference if concurrent access is possible:
internal fun <T : Any> Value<T>.subscribe(observer: (new: T, old: T) -> Unit): Cancellation {
val oldRef = AtomicReference(value)
return subscribe { new ->
val old = oldRef.getAndSet(new)
observer(new, old)
}
}sample/shared/compose/build.gradle.kts (1)
52-58: LGTM! Clean separation of web and non-web targets
The bundle structure properly separates web-specific code from other targets, which is essential for the new web history API.
Let's verify no web-specific dependencies leak into non-web targets:
✅ Verification successful
The search results show that web-specific imports and dependencies are present in the common source set (commonMain), but they are properly isolated through interfaces and default implementations. The pattern observed is:
- Interfaces in common code define web navigation contracts (
WebNavigationOwner) - Default implementations handle web-specific logic
- Preview/NoOp implementations provide non-web alternatives
This is a standard way to handle web-specific features in Kotlin Multiplatform projects. The bundle structure in the build file correctly reflects this architecture by having nonWeb dependencies separate from js.
LGTM! Web dependencies are properly isolated through interfaces
The codebase follows a clean architecture pattern where web-specific functionality is abstracted behind interfaces in common code, with concrete implementations provided for each target platform. The bundle structure in build.gradle.kts correctly enforces this separation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for web-specific imports in non-web source directories
rg -g '!**/js*/**' -g '!**/wasm*/**' \
-e 'kotlinx\.browser' \
-e 'org\.w3c\.dom' \
-e 'WebNavigation' \
-e 'WebHistoryNavigation'
Length of output: 50331
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt (2)
84-89: LGTM! Clean and focused implementation.
The Owner class provides a clean implementation of WebNavigationOwner with helpful toString() override for debugging.
18-27: Consider thread safety for shared state.
While the initialization is thread-safe, the children map might need synchronization if accessed from multiple threads during testing.
Consider using a concurrent map implementation if concurrent testing is needed:
- private var children = HashMap<Int, TestWebNavigation>()
+ private var children = Collections.synchronizedMap(HashMap<Int, TestWebNavigation>())
sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt
Show resolved
Hide resolved
...red/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt
Show resolved
Hide resolved
...ared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt
Show resolved
Hide resolved
...shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt
Show resolved
Hide resolved
...in/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt
Show resolved
Hide resolved
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
Show resolved
Hide resolved
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt
Show resolved
Hide resolved
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt
Show resolved
Hide resolved
...pose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
web-history-2
branch
from
fc78ab9 to
8cd8d9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (18)
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt (1)
21-23: Consider adding documentation for WebNavigationOwner.NoOp implementationWhile using NoOp is appropriate for a preview component, it would be helpful to add a KDoc comment explaining why this implementation was chosen and what it means for preview/testing scenarios.
Here's a suggested addition:
@OptIn(ExperimentalDecomposeApi::class) +/** + * Preview implementation of MultiPaneComponent that implements WebNavigationOwner.NoOp + * to disable web navigation functionality in preview/testing scenarios. + */ class PreviewMultiPaneComponent( isMultiPane: Boolean = false, ) : MultiPaneComponent, ComponentContext by PreviewComponentContext, WebNavigationOwner.NoOp {sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt (1)
6-7: LGTM! Good move towards identifier-based navigation.The switch from passing
Imageobjects to using integer IDs in the callback is a positive architectural change that aligns well with web navigation patterns. This makes it easier to serialize navigation state and construct deep links.Consider documenting the ID format/constraints in the interface to ensure consistent usage across the codebase.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt (2)
32-38: Consider adding const values for magic numbersThe image list initialization uses hardcoded values that could be better expressed as named constants.
Consider this improvement:
+ private companion object { + private const val MAX_IMAGES = 100 + } - private val images = - List(100) { index -> + private val images = + List(MAX_IMAGES) { index ->
53-67: Clarify the purpose ofonBeforeNavigatecallbackThe
onBeforeNavigatecallback always returnsfalse. Consider adding a comment explaining why this behavior is desired, or if this is intentional.decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt (4)
6-6: Consider using backticks for test function names instead of suppressing 'TestFunctionName' warningBy using backticks in the test function names, you can write more readable and expressive names without suppressing the
TestFunctionNamewarning. This aligns with Kotlin best practices and enhances code clarity.You can remove the suppression and update the test function names as follows:
-@Suppress("TestFunctionName") class WebHistoryNavigationTest { @Test - fun WHEN_created_with_no_items_in_root_THEN_ISE_thrown() { + fun `WHEN created with no items in root THEN ISE thrown`() { // Test code } // ... other test functions ... }
7-768: Simplify test method names for better readabilityThe test method names are quite long and use underscores for separation, which can impact readability and make test reports harder to navigate. Consider simplifying the method names using descriptive phrases within backticks, which is a common practice in Kotlin for test functions.
For example:
@Test fun `created with no items in root throws ISE`() { // Test code }This approach enhances readability and makes the test suite more maintainable.
165-176: Refactor repeated initialization code into helper functionsThe initialization of
TestWebNavigationwith similar configurations is repeated across multiple tests. To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this repeated code into helper functions. This will reduce duplication and improve maintainability.For example, you can create a helper function:
private fun createTestNavigation( rootConfigs: List<Int>, childConfigs: Map<Int, List<Int>> = emptyMap() ): TestWebNavigation { return TestWebNavigation( initialHistory = rootConfigs, childFactory = { config -> childConfigs[config]?.let { TestWebNavigation(initialHistory = it) } } ) }Then, in your tests, you can use:
@Test fun `test scenario`() { val nav = createTestNavigation( rootConfigs = listOf(1), childConfigs = mapOf(1 to listOf(11)) ) // Test code... }Also applies to: 184-195, 205-216, 225-236, 245-256
764-767: Enhance assertion messages for better test diagnosticsIn the
assertHistoryfunction, consider adding custom messages to your assertions. This will provide more context when a test fails, making debugging easier.For example:
private fun assertHistory(nav: TestWebNavigation, urls: List<String>, index: Int = urls.lastIndex) { history.assertStack(urls = urls, index = index) nav.assertHistory( expectedConfigs = urls.slice(0..index), message = "Expected navigation history to be ${urls.slice(0..index)}, but was ${nav.currentHistory()}" ) }decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt (1)
57-62: Consider makingHistoryItema data classConverting
HistoryItemto adata classprovides automatically generatedequals,hashCode, andtoStringmethods, which can be helpful for debugging and state comparisons.Apply this diff to make the change:
- class HistoryItem<out T>( + data class HistoryItem<out T>( val key: T, val path: String, val parameters: Map<String, String>, val child: WebNavigationOwner?, )sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
32-32: Consider removing debug loggingThe console log statement appears to be for debugging purposes and should be removed before merging.
- console.log(root.stack.value)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (2)
100-101: Address TODO commentThe TODO comment suggests incomplete implementation or documentation. Please clarify the purpose of
pathandchildUrlvariables.Would you like me to help document these variables or implement any missing functionality?
56-63: Consider documenting childSelector behaviorThe
childSelectorimplementation returns null for most child types exceptMultiPaneChild. Consider adding documentation explaining this design decision.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt (2)
93-101: Consider adding error handling for invalid configurationsThe
toHistorymethod assumes all configurations can be properly converted to history items. Consider adding error handling for invalid configurations.private fun ChildStack<C, T>.toHistory(): List<HistoryItem<C>> = (if (enableHistory) items else items.takeLast(1)).map { child -> + try { HistoryItem( path = pathMapper(child) ?: "", parameters = parametersMapper(child) ?: emptyMap(), key = child.configuration, child = childSelector(child), ) + } catch (e: Exception) { + throw IllegalStateException("Failed to convert configuration to history item", e) + } }
106-108: Consider adding validation for history listThe
navigatemethod accepts any list of configurations without validation. Consider adding checks for empty lists or invalid configurations.override fun navigate(history: List<C>) { + require(history.isNotEmpty()) { "History list cannot be empty" } + history.forEach { config -> + require(config != null) { "Configuration cannot be null" } + } navigator.navigate { history } }decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (2)
85-92: Consider adding input validation for navigation nodesWhile the implementation is functionally correct, it would benefit from additional validation to ensure data consistency.
Consider adding these validations:
private fun <T : Any> WebNavigation<T>.navigate(nodes: List<SerializableNode>) { + require(nodes.isNotEmpty()) { "Navigation nodes list cannot be empty" } val items: List<T> = nodes.map { it.data.consumeRequired(serializer) } navigate(items) history.value.forEachIndexed { index, item -> + require(index < nodes.size) { "Navigation history size mismatch" } item.child?.webNavigation?.navigate(nodes[index].children) } }
317-360: Consider adding versioning support for serialized stateTo support future schema changes and backward compatibility, consider adding version information to the serialized state.
Apply this diff to add versioning:
@Serializable private data class SerializableNode( + val version: Int = 1, val data: SerializableContainer, val children: List<SerializableNode>, ) @Serializable private class HistoryState( + val version: Int = 1, val index: Int, val nodesContainer: SerializableContainer, )decompose/api/decompose.klib.api (1)
Line range hint
498-513: Consider documenting platform support for web navigation functionsThe web navigation functions are platform-specific but this isn't immediately obvious from the API.
Consider adding KDoc comments to clearly indicate platform support:
+/** + * Enables web navigation support for panels. + * Available only on JS and Wasm platforms. + */ final fun <A: kotlin/Any, B: kotlin/Any> childPanelsWebNavigation(...)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt (1)
76-88: Clarify variable naming in lambda expressions for better readabilityIn the
historyproperty, the lambda parameter is namedpages, which is the same as the outer variable. This can be confusing. Consider renaming the lambda parameter to improve clarity.Apply this diff to rename the lambda parameter:
override val history: Value<List<WebNavigation.HistoryItem<Pages<C>>>> = - pages.map { pages -> + pages.map { childPages -> listOf( WebNavigation.HistoryItem( - path = pathMapper(pages) ?: "", - parameters = parametersMapper(pages) ?: emptyMap(), + path = pathMapper(childPages) ?: "", + parameters = parametersMapper(childPages) ?: emptyMap(), key = Pages( - items = pages.items.map { it.configuration }, - selectedIndex = pages.selectedIndex, + items = childPages.items.map { it.configuration }, + selectedIndex = childPages.selectedIndex, ), - child = (pages.items.getOrNull(pages.selectedIndex) as? Child.Created)?.let(childSelector), + child = (childPages.items.getOrNull(childPages.selectedIndex) as? Child.Created)?.let(childSelector), ) ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (53)
decompose/api/android/decompose.api(6 hunks)decompose/api/decompose.klib.api(6 hunks)decompose/api/jvm/decompose.api(6 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt(1 hunks)decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt(2 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt(1 hunks)sample/app-ios-compose/app-ios-compose/iOSApp.swift(1 hunks)sample/app-ios/app-ios/app_iosApp.swift(1 hunks)sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt(1 hunks)sample/app-js-compose/src/jsMain/resources/index.html(1 hunks)sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt(2 hunks)sample/app-js/src/main/resources/index.html(1 hunks)sample/shared/compose/build.gradle.kts(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt(1 hunks)sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt(1 hunks)sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt(4 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt(5 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt(3 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt(0 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt(6 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt(1 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt(2 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt
🚧 Files skipped from review as they are similar to previous changes (32)
- sample/app-js/src/main/resources/index.html
- sample/app-js-compose/src/jsMain/resources/index.html
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt
- sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt
- sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt
- sample/shared/compose/build.gradle.kts
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
- decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
- sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt
- sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt
🧰 Additional context used
🪛 detekt (1.23.7)
decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (36)
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt (1)
Line range hint 1-35: Implementation looks good!
The integration of WebNavigationOwner.NoOp into this preview component is clean and appropriate. The implementation maintains its testing/preview purpose while adapting to the new web navigation architecture.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt (1)
11-12: LGTM! Verify ID usage consistency.
The implementation correctly forwards the ID to the callback.
Let's verify that other components are consistently using IDs for navigation:
✅ Verification successful
Let me gather more information to verify the ID usage consistency across the codebase.
ID usage is consistent across the codebase
The verification shows consistent ID handling in the image navigation flow:
GalleryComponentinterface definesonImageClicked(index: Int)DefaultGalleryComponentcorrectly implements this by forwarding toonImageSelected(id: Int)- The navigation handler in
DefaultSharedTransitionsComponentproperly uses the ID to push new Photo config:onImageSelected = { nav.pushNew(Config.Photo(id = it)) } - UI layer in
GalleryContentcorrectly passes the index parameter:clickable { component.onImageClicked(index = index) }
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent ID-based navigation pattern across components
# Look for other image selection handlers to ensure consistent ID usage
# Search for image selection related methods
ast-grep --pattern 'fun $FUNC_NAME($ARG: Int) {
$$$
}'
# Search for image selection related callbacks
rg -A 2 'onImageSelected.*Int.*->'
Length of output: 1060
Script:
#!/bin/bash
# Search for image-related method calls and handlers
rg -A 3 "onImageClicked|onImageSelected"
# Search for any Int parameters in image-related functions
ast-grep --pattern 'fun $FUNC_NAME($ARG_NAME: Int) {
onImage$_($ARG_NAME)
}'
# Search for Gallery component interface definition
rg -A 5 "interface.*Gallery.*Component"
Length of output: 4895
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt (5)
27-28: LGTM: Constructor updated for deep link support
The addition of the deepLinkUrl parameter aligns with the PR's objective of supporting web navigation.
42-51: LGTM: Stack navigation properly configured
The stack navigation setup correctly integrates deep linking support while maintaining existing functionality.
102-104: Validate deep link parameters to prevent navigation issues
The image ID from the deep link URL should be validated against existing images.
117-117: LGTM: Config properly updated for serialization
The change from storing Image to id in Photo config supports proper serialization for web navigation.
83-83:
Handle potential NoSuchElementException
Using first() to find an image by ID could throw an exception if the ID doesn't exist.
Consider using this safer approach:
- image = images.first { it.id == config.id },
+ image = images.find { it.id == config.id }
+ ?: throw IllegalArgumentException("Invalid image ID: ${config.id}"),Likely invalid or redundant comment.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt (6)
12-12: LGTM: Required imports added for web navigation support
The new imports align with the PR objectives and are necessary for implementing the web history API functionality.
Also applies to: 17-17, 20-20
38-38: LGTM: Deep linking support added via constructor parameter
The nullable deepLinkUrl parameter is correctly added to support deep linking functionality.
Line range hint 46-58: LGTM: Panels initialization properly configured
The panels initialization is correctly implemented with the new deep linking support.
67-71: Ensure parametersMapper returns a non-null Map
When panels.details is null, the lambda returns null. Consider returning an empty map to prevent potential NullPointerException.
132-134: LGTM: Well-structured constant definition
The article ID key constant is properly defined and scoped within the companion object.
123-130: Verify article ID validation
While the code safely handles null and invalid article IDs, consider verifying that the article exists in the database before creating the Details instance.
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt (1)
630-637: Verify the handling of onBeforeNavigate returning false
In tests where onBeforeNavigate returns false, ensure that the navigation prevention behaves as expected. Confirm that the browser history remains unchanged and that the navigation stack in TestWebNavigation reflects the correct state.
To automate this verification, you can run the following script:
Please ensure that the assertions in your tests accurately reflect the expected behavior when navigation is canceled.
Also applies to: 640-648, 651-661, 664-674
✅ Verification successful
Let me gather more information about the test assertions to verify the behavior when navigation is prevented.
Based on the test assertions and implementation, I can now provide the final response:
The handling of onBeforeNavigate=false is correctly verified
The test cases demonstrate proper verification of navigation prevention:
- The navigation stack maintains the correct state as shown by
assertHistorychecks - Browser history operations are properly tested with various scenarios (forward/backward navigation)
- The test assertions verify both the navigation stack and URL states through
assertHistory(nav = nav, urls = ...)calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that when onBeforeNavigate returns false, navigation is prevented.
# Search for instances where onBeforeNavigate is set to return false
rg 'onBeforeNavigate\s*=\s*\{\s*false\s*\}' -A 5
# Ensure that the navigation stack does not change after navigation is attempted
# This would require checking the state before and after, which can be done via test assertions
Length of output: 7581
Script:
#!/bin/bash
# Search for assertHistory function and its usage in the test file
rg -A 5 'fun assertHistory' decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt
# Search for test cases with onBeforeNavigate=false and their assertions
rg -B 5 'assertHistory.*nav.*urls' decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt
Length of output: 9734
sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt (1)
27-33: Good implementation of withWebHistory
The integration of withWebHistory in the main function is correctly done. Passing stateKeeper and converting deepLink to Url enhances deep link handling effectively.
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt (1)
71-116: Ensure correct serializers are provided
In PanelsWebNavigation, the serializers mainSerializer, detailsSerializer, and extraSerializer are crucial for proper serialization and deserialization of configurations. Verify that the correct serializers are supplied to prevent runtime exceptions or data inconsistencies.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt (2)
74-78: Deep link handling in Tabs component is correctly implemented
Passing deepLinkUrl to DefaultTabsComponent ensures proper handling of deep links within the Tabs component.
126-135: Efficient initial stack determination with getInitialStack
The getInitialStack function effectively determines the initial navigation stack based on the deepLinkUrl, enhancing deep link support throughout the application.
decompose/api/jvm/decompose.api (5)
Line range hint 170-197: Approval: Enhancements to Pages Class
The addition of the Companion object and the Pages$$serializer class to the Pages class improves serialization capabilities, aligning with the introduced web navigation support.
233-237: Approval: Introduction of PagesWebNavigationKt
The new PagesWebNavigationKt class provides necessary functionalities for web navigation of pages, enhancing the API's navigation features.
358-362: Approval: Addition of PanelsWebNavigationKt
The inclusion of PanelsWebNavigationKt adds web navigation support for panels, which is consistent with the PR objectives to enhance nested navigation.
481-485: Approval: Enhancement with StackWebNavigationKt
The introduction of StackWebNavigationKt extends stack navigation capabilities for web interfaces, contributing to the comprehensive navigation enhancements.
501-507: Approval: Introduction of WebNavigation Interface
The addition of the WebNavigation interface is a significant enhancement for managing web navigation history and actions, aligning with the goals of supporting nested navigation models.
decompose/api/android/decompose.api (5)
Line range hint 190-218: Approval: Serialization Enhancements in Pages Class
The addition of the Companion object and the Pages$$serializer class to the Pages class enhances serialization support, which is essential for the new web navigation functionalities.
253-257: Approval: Addition of PagesWebNavigationKt
The inclusion of PagesWebNavigationKt provides web navigation capabilities for pages, aligning with the PR's objective to add nested navigation support.
378-382: Approval: Introduction of PanelsWebNavigationKt
The new PanelsWebNavigationKt class offers web navigation functionalities for panels, contributing to the expanded navigation features.
501-505: Approval: Enhancement with StackWebNavigationKt
The addition of StackWebNavigationKt enhances stack navigation for web interfaces, supporting the comprehensive navigation enhancements in the API.
521-527: Approval: Introduction of WebNavigation Interface
The new WebNavigation interface and related classes expand the library's web navigation capabilities, which is crucial for the nested navigation support added in this PR.
decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt (1)
14-20:
Catch Specific Exceptions Instead of General Exception
Catching all exceptions can unintentionally suppress critical issues. It's advisable to catch more specific exceptions, such as SerializationException, to avoid masking underlying problems and to handle different exception types appropriately.
Apply this diff to catch only SerializationException:
internal fun JsonString.decodeContainer(): SerializableContainer? =
try {
Json.decodeFromString(SerializableContainer.serializer(), value)
- } catch (e: Exception) {
+ } catch (e: SerializationException) {
null
+ } catch (e: Exception) {
+ // Optionally log or handle other exceptions
+ throw e
}This change ensures that serialization errors are handled gracefully while allowing other exceptions to propagate, preserving critical error information.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
sample/app-ios/app-ios/app_iosApp.swift (1)
34-34: Approval: Update to deepLinkUrl Initialization
Replacing deepLink with deepLinkUrl: nil aligns with the updated deep link handling approach. The change is appropriate and maintains the intended functionality.
sample/app-ios-compose/app-ios-compose/iOSApp.swift (1)
29-29: Approval: Change to deepLinkUrl Parameter
Updating the initialization from deepLink to deepLinkUrl: nil reflects the new method of handling deep links within the application and aligns with the changes made in the associated components.
sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
24-30: LGTM: Proper initialization with web history support
The implementation correctly uses the new withWebHistory function to initialize the root component with state keeper and deep link support.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (1)
48-64: Verify history disabled flag
The webNavigation implementation disables history tracking (enableHistory = false). This might affect the navigation experience if history tracking is required for tabs.
✅ Verification successful
Let me gather more context about the navigation implementation in the codebase.
Let me check one more thing about the tabs implementation to understand the navigation pattern better.
Based on the gathered information, I can now provide the final response:
Disabling history in tabs navigation is intentional and correct
The enableHistory = false setting in DefaultTabsComponent is appropriate because:
- This is the only component in the codebase that explicitly disables history
- The component implements tab-based navigation where each tab maintains its own state
- Other navigation components (root, pages, multipane) handle history differently based on their specific navigation patterns
- The tabs component acts as a container where the actual navigation history is managed by the active tab's child component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other tab-based components also disable history
rg -l "enableHistory\s*=\s*false" --type kotlin
Length of output: 149
Script:
#!/bin/bash
# Check for other web navigation implementations and their history settings
rg "webNavigation.*=" --type kotlin -A 5
# Also check for any documentation or comments about web navigation history
rg -i "web.*navigation.*history" --type kotlin
Length of output: 17238
Script:
#!/bin/bash
# Check the TabsComponent interface and implementation for navigation-related code
rg -A 10 "interface TabsComponent" --type kotlin
rg -A 10 "class DefaultTabsComponent" --type kotlin
Length of output: 2964
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (2)
79-83: LGTM: Recursive navigation check is well implemented
The recursive navigation check is implemented correctly, handling both parent and child navigation permissions.
14-77:
Review state management and error handling in enableWebHistory function
Several concerns need to be addressed:
- The state deserialization lacks error handling which could lead to runtime exceptions
- The function uses a boolean flag for synchronization which might be insufficient for complex navigation scenarios
- The recursive navigation handling could potentially cause stack overflow for deeply nested structures
Apply this diff to add error handling and improve state management:
fun onPopState(state: String?) {
- val deserializedState = state?.deserializeState() ?: return
+ val deserializedState = try {
+ state?.deserializeState()
+ } catch (e: Exception) {
+ // Log the error for debugging
+ console.error("Failed to deserialize navigation state", e)
+ return
+ } ?: return
if (navigation.onBeforeNavigateRecursive()) {
isEnabled = false
navigation.navigate(deserializedState.nodesContainer.consumeNodes())
isEnabled = true
} else {
val delta = navigation.history().lastIndex - deserializedState.index
if (delta != 0) {
browserHistory.setOnPopStateListener { browserHistory.setOnPopStateListener(::onPopState) }
browserHistory.go(delta)
}
}
}Consider implementing a more robust synchronization mechanism using a state machine or mutex to handle complex navigation scenarios.
Let's verify the navigation depth handling:
decompose/api/decompose.klib.api (1)
117-138: LGTM: Well-designed API for web navigation
The WebNavigation and WebNavigationOwner interfaces are well-designed with clear separation of concerns and extensibility support.
Also applies to: 183-191
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
Outdated
Show resolved
Hide resolved
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
web-history-2
branch
from
8cd8d9c to
0137389
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (18)
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt (2)
32-38: Consider lazy initialization for the images listThe images list is eagerly initialized with 100 items during component construction. Consider using lazy initialization to defer creation until first use, especially since not all images may be needed immediately.
- private val images = + private val images by lazy { List(100) { index -> Image( id = index, resourceId = ImageResourceId.entries[index % ImageResourceId.entries.size], ) - } + }
53-67: Consider implementing meaningful navigation controlThe
onBeforeNavigatecallback always returnsfalse, which means navigation is never prevented. Consider implementing actual navigation validation logic if there are cases where navigation should be restricted (e.g., unsaved changes).- onBeforeNavigate = { false }, + onBeforeNavigate = { navigationEvent -> + // Add logic to determine if navigation should be prevented + // Example: check for unsaved changes + false + },sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt (2)
29-35: Consider extracting initialization logicThe pages initialization logic could be moved to a separate private function for better maintainability and testing.
Consider refactoring to:
- private val _pages: Value<ChildPages<ImageResourceId, KittenComponent>> = - childPages( - source = nav, - serializer = ImageResourceId.serializer(), - initialPages = { Pages(items = ImageResourceId.entries, selectedIndex = getInitialPageIndex(deepLinkUrl)) }, - childFactory = { imageType, ctx -> DefaultKittenComponent(ctx, imageType) }, - ) + private val _pages: Value<ChildPages<ImageResourceId, KittenComponent>> = createPages(deepLinkUrl) + + private fun createPages(deepLinkUrl: Url?): Value<ChildPages<ImageResourceId, KittenComponent>> = + childPages( + source = nav, + serializer = ImageResourceId.serializer(), + initialPages = { createInitialPages(deepLinkUrl) }, + childFactory = { imageType, ctx -> DefaultKittenComponent(ctx, imageType) }, + ) + + private fun createInitialPages(deepLinkUrl: Url?) = + Pages( + items = ImageResourceId.entries, + selectedIndex = getInitialPageIndex(deepLinkUrl) + )
39-46: Document experimental API usageThe code uses
@ExperimentalDecomposeApiwhich indicates potential API instability. Consider adding documentation comments about this experimental status and any migration plans.Add documentation:
+ /** + * Web navigation implementation using experimental Decompose API. + * Note: This API is experimental and may change in future releases. + * Track issue #111 for API stabilization updates. + */ @OptIn(ExperimentalDecomposeApi::class) override val webNavigation: WebNavigation<*> = childPagesWebNavigation(decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt (1)
57-62: Consider makingHistoryItemadata classSince
HistoryItemis primarily used for holding data, converting it into adata classwill automatically provideequals,hashCode,toString, andcopymethods. This aligns with Kotlin best practices for classes that are intended to be simple data carriers.sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (1)
100-100: Address the TODO comment regardingchildUrlhandlingThe TODO comment indicates that handling of
childUrlin the path is pending. Implementing this will ensure proper deep link processing and navigation.Would you like assistance in implementing the handling of
childUrlwithin thegetInitialConfigmethod?sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt (1)
7-7: Consider passing theImageobject instead of only theidtoonImageSelectedChanging the
onImageSelectedcallback to acceptid: Intinstead ofimage: Imagereduces the information available in the callback. If additional properties of theImageare needed for processing, it might be beneficial to pass the entireImageobject to retain flexibility.decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt (1)
6-9: Consider documenting the Json configurationThe Json configuration allows structured map keys, but the reason for this setting isn't immediately clear. Consider adding a comment explaining why this configuration is necessary.
sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
32-32: Consider using more descriptive loggingThe console log message could be more informative about what's being logged.
- console.log(root.stack.value) + console.log("Root stack state:", root.stack.value)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt (2)
12-57: Consider adding example usage in the documentation.The documentation is comprehensive but would benefit from a code example demonstrating typical usage patterns, especially for the
pathMapperandparametersMapperfunctions.
93-101: Consider optimizing the history mapping logic.The
toHistoryfunction could be more efficient by:
- Combining the
mapoperation withtakeLast- Using a shared empty map instance
+private val EMPTY_MAP = emptyMap<String, String>() private fun ChildStack<C, T>.toHistory(): List<HistoryItem<C>> = - (if (enableHistory) items else items.takeLast(1)).map { child -> + items.let { if (enableHistory) it else it.takeLast(1) }.map { child -> HistoryItem( path = pathMapper(child) ?: "", - parameters = parametersMapper(child) ?: emptyMap(), + parameters = parametersMapper(child) ?: EMPTY_MAP, key = child.configuration, child = childSelector(child), ) }sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt (1)
Line range hint
46-57: Consider extracting panel configuration to a separate function.The panel configuration is complex and would benefit from being extracted into a separate private function for better readability and maintainability.
+ private fun createPanelsConfiguration() = + childPanels( + source = navigation, + initialPanels = { getInitialPanels(deepLinkUrl) }, + serializers = Triple(Unit.serializer(), Details.serializer(), Extra.serializer()), + onStateChanged = { newState, _ -> _navState.onNext(newState) }, + handleBackButton = true, + mainFactory = { _, ctx -> listComponent(ctx) }, + detailsFactory = ::detailsComponent, + extraFactory = ::authorComponent, + ) - private val _panels = - childPanels( - source = navigation, - initialPanels = { getInitialPanels(deepLinkUrl) }, - serializers = Triple(Unit.serializer(), Details.serializer(), Extra.serializer()), - onStateChanged = { newState, _ -> _navState.onNext(newState) }, - handleBackButton = true, - mainFactory = { _, ctx -> listComponent(ctx) }, - detailsFactory = ::detailsComponent, - extraFactory = ::authorComponent, - ) + private val _panels = createPanelsConfiguration()sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt (2)
126-135: Consider making the path handling more type-safe.The
getInitialStackfunction uses string-based path handling which could be error-prone. Consider using a sealed class or enum for path segments to make it more type-safe.+private sealed class PathSegment(val value: String) { + object DynamicFeatures : PathSegment("dynamic-features") + object CustomNavigation : PathSegment("custom-navigation") + object Pages : PathSegment("pages") + object SharedTransitions : PathSegment("shared-transitions") + + companion object { + fun fromString(value: String): PathSegment? = + when (value) { + DynamicFeatures.value -> DynamicFeatures + CustomNavigation.value -> CustomNavigation + Pages.value -> Pages + SharedTransitions.value -> SharedTransitions + else -> null + } + } +} private fun getInitialStack(deepLinkUrl: Url?): List<Config> { val (path, childUrl) = deepLinkUrl?.consumePathSegment() ?: return listOf(Config.Tabs()) - return when (path) { - pathSegmentOf<Config.DynamicFeatures>() -> listOf(Config.Tabs(), Config.DynamicFeatures) - pathSegmentOf<Config.CustomNavigation>() -> listOf(Config.Tabs(), Config.CustomNavigation) - pathSegmentOf<Config.Pages>() -> listOf(Config.Tabs(), Config.Pages(deepLinkUrl = childUrl)) - pathSegmentOf<Config.SharedTransitions>() -> listOf(Config.Tabs(), Config.SharedTransitions(deepLinkUrl = childUrl)) - else -> listOf(Config.Tabs(deepLinkUrl = childUrl)) + return when (PathSegment.fromString(path)) { + is PathSegment.DynamicFeatures -> listOf(Config.Tabs(), Config.DynamicFeatures) + is PathSegment.CustomNavigation -> listOf(Config.Tabs(), Config.CustomNavigation) + is PathSegment.Pages -> listOf(Config.Tabs(), Config.Pages(deepLinkUrl = childUrl)) + is PathSegment.SharedTransitions -> listOf(Config.Tabs(), Config.SharedTransitions(deepLinkUrl = childUrl)) + null -> listOf(Config.Tabs(deepLinkUrl = childUrl)) } }
57-65: Consider adding error handling for invalid child types.The
childSelectorfunction assumes all child types are handled in the when expression. Consider adding an else branch or making it more explicit that all cases are covered.childSelector = { when (val child = it.instance) { is CustomNavigationChild -> null is TabsChild -> child.component is DynamicFeaturesChild -> null is PagesChild -> child.component is SharedTransitionsChild -> child.component + // All cases are covered as Child is a sealed interface } },decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (2)
15-21: Add validation for browser history state initializationThe browser history state initialization should validate that the navigation has a valid history before replacing the state.
internal fun <T : Any> enableWebHistory(navigation: WebNavigation<T>, browserHistory: BrowserHistory) { + val initialHistory = navigation.nodeHistory() + require(initialHistory.isNotEmpty()) { "Navigation history cannot be empty" } + if (browserHistory.state == null) { - browserHistory.replaceState(navigation.nodeHistory()) + browserHistory.replaceState(initialHistory) }
40-77: Document the complex navigation logicThe navigation subscription logic is complex and would benefit from documentation explaining the different navigation scenarios.
+ // Subscribe to navigation changes and handle different scenarios: + // 1. Push: Add new states to history + // 2. Pop: Remove states from history + // 3. Rewrite: Replace existing history + // 4. URL Update: Update URL without changing history navigation.subscribe( isEnabled = { isEnabled }, onPush = { history ->decompose/api/decompose.klib.api (2)
117-138: Well-designed WebNavigation interface with comprehensive functionality!The interface provides a robust foundation for web navigation with:
- History tracking via Value<List>
- Type-safe serialization support
- Navigation control methods
- Pre-navigation validation
- Nested navigation support through WebNavigationOwner in HistoryItem
The design allows for flexible implementation of different navigation patterns while maintaining type safety.
Consider documenting the following aspects for implementers:
- Thread-safety requirements for history updates
- Lifecycle management best practices
- Guidelines for handling deep linking
183-191: Clean interface design with proper separation of concerns!The WebNavigationOwner interface effectively:
- Separates navigation ownership from navigation implementation
- Provides a NoOp implementation for graceful degradation
- Uses wildcard type parameter for flexibility
This design allows for easy composition and testing.
Consider adding:
- Factory methods for common WebNavigationOwner implementations
- Extension functions for common navigation patterns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (54)
decompose/api/android/decompose.api(6 hunks)decompose/api/decompose.klib.api(6 hunks)decompose/api/jvm/decompose.api(6 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigation.kt(1 hunks)decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt(1 hunks)decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt(2 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/UrlEncodeTest.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt(1 hunks)sample/app-ios-compose/app-ios-compose/iOSApp.swift(1 hunks)sample/app-ios/app-ios/app_iosApp.swift(1 hunks)sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt(1 hunks)sample/app-js-compose/src/jsMain/resources/index.html(1 hunks)sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt(2 hunks)sample/app-js/src/main/resources/index.html(1 hunks)sample/shared/compose/build.gradle.kts(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt(1 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt(2 hunks)sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt(1 hunks)sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt(1 hunks)sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt(4 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/DefaultRootComponent.kt(5 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt(2 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt(3 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/DefaultGalleryComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt(0 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt(6 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt(1 hunks)sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt(1 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt(2 hunks)sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/Image.kt
🚧 Files skipped from review as they are similar to previous changes (36)
- sample/app-js/src/main/resources/index.html
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/utils/Utils.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/Pages.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/TabsComponent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneComponent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/MultiPaneContent.kt
- sample/app-js-compose/src/jsMain/resources/index.html
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/gallery/GalleryContent.kt
- sample/app-ios/app-ios/app_iosApp.swift
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Utils.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/cards/CardsContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/PagesComponent.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/photo/PhotoContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/SharedTransitionsComponent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/RootComponent.kt
- sample/shared/compose/src/jsMain/kotlin/com/arkivanov/sample/shared/utils/Utils.js.kt
- sample/shared/compose/src/nonWebMain/kotlin/com/arkivanov/sample/shared/utils/Utils.nonWeb.kt
- sample/shared/compose/build.gradle.kts
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/menu/MenuContent.kt
- sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/tabs/TabsComponentIntegrationTest.kt
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/Utils.kt
- sample/shared/compose/src/commonMain/kotlin/com/arkivanov/sample/shared/counters/CountersContent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/PreviewTabsComponent.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/PreviewMultiPaneComponent.kt
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
- sample/shared/shared/src/commonTest/kotlin/com/arkivanov/sample/shared/root/RootComponentIntegrationTest.kt
- decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/WebNavigationOwner.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/root/PreviewRootComponent.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/ChildStackFactory.kt
- sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/Url.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/PanelsWebNavigation.kt
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/webhistory/NoOpWebNavigation.kt
- sample/app-ios-compose/app-ios-compose/iOSApp.swift
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestWebNavigation.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigationTest.kt
🧰 Additional context used
🪛 detekt (1.23.7)
decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (26)
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/sharedtransitions/DefaultSharedTransitionsComponent.kt (4)
42-51: LGTM! Stack initialization is well implemented
The stack setup properly handles serialization, initial state from deep links, and back button navigation.
111-118: LGTM! Config serialization is well implemented
The change from storing Image reference to ID in Photo config improves serialization and follows best practices for state management.
83-83:
Handle potential NoSuchElementException when retrieving images
Using first() to find an image can throw if the ID doesn't exist. Consider using a safer approach.
93-109:
Validate deep link parameters to prevent navigation issues
The image ID from deep link parameters should be validated before creating the Config.Photo.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/pages/DefaultPagesComponent.kt (2)
22-23: LGTM: Constructor parameter addition is well-designed
The nullable deepLinkUrl parameter appropriately handles cases where deep linking may or may not be present.
64-68:
Ensure safe destructuring when consuming the path segment
The destructuring operation val (path) = deepLinkUrl?.consumePathSegment() could cause a NPE if consumePathSegment() returns null.
Consider safer implementation:
private fun getInitialPageIndex(deepLinkUrl: Url?): Int {
- val (path) = deepLinkUrl?.consumePathSegment() ?: return 0
+ val pathSegment = deepLinkUrl?.consumePathSegment() ?: return 0
+ val path = pathSegment.first
return ImageResourceId.entries.indexOfFirst { it.name == path }.takeIf { it >= 0 } ?: 0
}decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/PagesWebNavigation.kt (1)
94-95: Ensure history list is not empty before accessing history.first()
In the navigate method, history.first() is called without checking if history is not empty. Although the current implementation may guarantee a non-empty history, adding a safety check can prevent potential NoSuchElementException in case of future changes.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/tabs/DefaultTabsComponent.kt (2)
29-29: Ensure backward compatibility with constructor changes
Adding deepLinkUrl: Url? to the constructor changes its signature. This may affect existing code that instantiates DefaultTabsComponent without this parameter. Consider providing a default value or an overloaded constructor to maintain backward compatibility.
123-123: Evaluate the impact of changing MultiPane from an object to a data class
Converting Config.MultiPane from a data object to a data class changes it from a singleton to multiple instances. This may have implications if any code relies on MultiPane being a singleton. Please ensure that this change does not adversely affect the application logic.
decompose/src/webTest/kotlin/com/arkivanov/decompose/UrlEncodeTest.kt (1)
12-12: Ensure encodeURIComponent and decodeURIComponent are properly referenced
The functions encodeURIComponent and decodeURIComponent are JavaScript global functions. Since this is a Kotlin/JS test, verify that these functions are correctly accessible or consider importing them explicitly if necessary to prevent any reference errors.
decompose/src/webMain/kotlin/com/arkivanov/decompose/Utils.kt (3)
11-12: LGTM: Serialization implementation looks correct
The encodeToJson function correctly uses the SerializableContainer serializer.
14-19:
Improve exception handling in decodeContainer
The current implementation swallows all exceptions, which could mask critical issues.
Apply this diff to catch specific exceptions:
internal fun JsonString.decodeContainer(): SerializableContainer? =
try {
Json.decodeFromString(SerializableContainer.serializer(), value)
- } catch (e: Exception) {
+ } catch (e: kotlinx.serialization.SerializationException) {
null
+ } catch (e: IllegalArgumentException) {
+ null
+ } catch (e: Exception) {
+ // Unexpected exception during deserialization
+ throw e
}🧰 Tools
🪛 detekt (1.23.7)
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
23-25: Verify browser compatibility for external functions
The external functions encodeURIComponent and decodeURIComponent rely on browser APIs.
✅ Verification successful
Let me analyze the usage of these functions further by checking the web platform target configuration and any potential polyfills or fallbacks.
Let me check the web-specific implementation and usage of these functions.
Let me check the actual usage context and implementation details of these functions.
Based on the analysis of the code, I can now provide the final response:
Browser compatibility is properly handled for URI component functions
The encodeURIComponent and decodeURIComponent functions are:
- Properly declared as
externalfunctions targeting the web platform - Used in a web-specific module (
webMain) with appropriate platform checks - Tested in web-specific tests that verify encoding/decoding functionality
- Used safely in URL parameter handling within the web history navigation component
- Standard JavaScript functions with universal browser support (IE5.5+)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these functions are used with proper null/undefined checks in the codebase
# Search for direct usage of these functions
rg "encodeURIComponent|decodeURIComponent" --type kotlin
# Search for potential error handling around these calls
rg -B 2 -A 2 "encodeURIComponent|decodeURIComponent" --type kotlin
Length of output: 3159
Script:
#!/bin/bash
# Check build configuration and web-specific setup
fd "build.gradle" --exec cat {}
# Check for any web-specific initialization or platform checks
rg -B 3 -A 3 "expect|actual.*web" --type kotlin
# Look for any browser-specific checks or initialization code
rg -B 3 -A 3 "browser|window|document" --type kotlin
Length of output: 87388
Script:
#!/bin/bash
# Check web-specific implementation and usage
rg -B 3 -A 3 "actual.*encodeURIComponent|actual.*decodeURIComponent" --type kotlin
# Look for any error handling or web platform checks around these functions
fd -e kt -e js -x cat {} | rg -B 3 -A 3 "try.*encodeURIComponent|catch.*encodeURIComponent|try.*decodeURIComponent|catch.*decodeURIComponent"
# Check web platform target configuration
fd "build.gradle.kts|build.gradle" --exec grep -A 5 "js.*browser" {}
Length of output: 1313
Script:
#!/bin/bash
# Check the implementation and usage in web-specific code
rg -B 5 -A 5 "encodeURIComponent|decodeURIComponent" --type kotlin --type js
# Look for any web-specific platform checks or initialization
fd -e kt -x cat {} | rg -B 3 -A 3 "external.*fun.*Component|@JsName|@JsModule"
# Check for any web platform specific tests
fd -e kt | rg "web.*test|test.*web" | xargs cat
Length of output: 4963
sample/app-js/src/main/kotlin/com/arkivanov/sample/app/Main.kt (1)
24-30: 🛠️ Refactor suggestion
Consider handling deep link parsing errors
The deep link URL parsing could fail if the URL format is invalid.
Consider adding error handling:
withWebHistory { stateKeeper, deepLink ->
DefaultRootComponent(
componentContext = DefaultComponentContext(lifecycle = lifecycle, stateKeeper = stateKeeper),
featureInstaller = DefaultFeatureInstaller,
- deepLinkUrl = deepLink?.let(::Url),
+ deepLinkUrl = try {
+ deepLink?.let(::Url)
+ } catch (e: Exception) {
+ console.error("Invalid deep link URL: $deepLink")
+ null
+ },
)
}sample/app-js-compose/src/jsMain/kotlin/com/arkivanov/decompose/sample/app/Main.kt (1)
27-32: 🛠️ Refactor suggestion
Consider handling deep link parsing errors
Similar to the non-Compose version, the deep link URL parsing needs error handling.
Apply the same error handling pattern:
withWebHistory { stateKeeper, deepLink ->
DefaultRootComponent(
componentContext = DefaultComponentContext(lifecycle = lifecycle, stateKeeper = stateKeeper),
featureInstaller = DefaultFeatureInstaller,
- deepLinkUrl = deepLink?.let(::Url),
+ deepLinkUrl = try {
+ deepLink?.let(::Url)
+ } catch (e: Exception) {
+ console.error("Invalid deep link URL: $deepLink")
+ null
+ },
)
}decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/StackWebNavigation.kt (1)
58-78: LGTM!
The factory function is well-designed with appropriate default values and clear parameter naming.
sample/shared/shared/src/commonMain/kotlin/com/arkivanov/sample/shared/multipane/DefaultMultiPaneComponent.kt (1)
67-71: Ensure parametersMapper returns a non-null Map.
The parametersMapper lambda returns null when panels.details is null, which could lead to issues.
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt (5)
80-84: LGTM: Clean recursive implementation
The recursive navigation check is well-implemented and handles the child navigation hierarchy correctly.
235-269: LGTM: Proper URL encoding implementation
The URL handling functions correctly implement URL encoding and parameter handling.
320-328: LGTM: Well-structured serialization implementation
The SerializableNode class follows Kotlin serialization best practices with proper companion object setup.
86-93: 🛠️ Refactor suggestion
Add error handling for serialization
The serialization process should handle potential exceptions to prevent app crashes.
private fun <T : Any> WebNavigation<T>.navigate(nodes: List<SerializableNode>) {
- val items: List<T> = nodes.map { it.data.consumeRequired(serializer) }
+ val items: List<T> = try {
+ nodes.map { it.data.consumeRequired(serializer) }
+ } catch (e: Exception) {
+ // Log the error and return to maintain app stability
+ return
+ }Likely invalid or redundant comment.
22-36:
Add error handling for state deserialization
The state deserialization in onPopState should handle potential exceptions to prevent app crashes.
fun onPopState(state: String?) {
- val deserializedState = state?.deserializeState() ?: return
+ val deserializedState = try {
+ state?.deserializeState()
+ } catch (e: Exception) {
+ // Log the error and return to maintain app stability
+ return
+ } ?: returnLikely invalid or redundant comment.
decompose/api/jvm/decompose.api (1)
501-527: LGTM: Well-designed web navigation API
The WebNavigation interface and related components are well-designed with clear responsibilities and proper separation of concerns.
decompose/api/android/decompose.api (1)
521-547: LGTM: Consistent Android API implementation
The Android API declarations maintain consistency with the JVM version while properly integrating with Android components.
decompose/api/decompose.klib.api (2)
557-558: Platform-specific implementation properly isolated!
The withWebHistory function is correctly restricted to JS and Wasm-JS platforms where web history APIs are available.
Let's verify the platform support:
✅ Verification successful
Platform-specific implementation verified and correctly isolated
The verification confirms that:
- The common interface
WebHistoryControlleris platform-agnostic and properly defined in commonMain - Both JS and WasmJS have identical implementations of
withWebHistoryusing browser-specific APIs (window, sessionStorage) - The
DefaultBrowserHistoryis properly implemented with expect/actual declarations for both JS and WasmJS targets - No implementation exists for non-web platforms, as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper platform-specific implementations
# Expected: Implementation files for supported platforms only
# Search for platform-specific implementations
fd -e kt ".*WebHistory.*" --type f
Length of output: 746
Script:
#!/bin/bash
# Let's verify the actual implementations and platform restrictions
# Check the content of the common interface
cat decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt
# Check JS implementation
cat decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
# Check WasmJS implementation
cat decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/WebHistoryNavigation.kt
# Check if there are any expect/actual declarations
rg "expect|actual" decompose/src/*/kotlin/com/arkivanov/decompose/router/webhistory/
Length of output: 8441
498-498: Consistent implementation of web navigation across different models!
The functions provide a unified approach to web navigation while respecting the unique characteristics of each navigation model (panels, pages, stack).
Let's verify the consistency of navigation patterns:
Also applies to: 512-513
✅ Verification successful
Let me gather more specific information about the implementation patterns.
Let me try a different approach to examine the implementations.
Let me try one final search pattern to verify the implementations.
Consistent web navigation implementation verified across all navigation models!
Based on the code analysis, all three web navigation implementations (childPanelsWebNavigation, childPagesWebNavigation, and childStackWebNavigation) follow a consistent pattern with:
- Common core parameters: navigator, serializer, and model-specific state (panels/pages/stack)
- Consistent path and parameter mapping functionality
- Uniform child selection mechanism via
childSelector: (Child) -> WebNavigationOwner? - Proper type safety through generics
- Consistent return type of
WebNavigation<*>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent parameter patterns across navigation functions
# Expected: Similar parameter patterns for all navigation functions
# Search for navigation function declarations
rg "Web[nN]avigation.*\(" --type kotlin
Length of output: 18098
Script:
#!/bin/bash
# Get implementation details of the web navigation functions
ast-grep --pattern 'fun childPanelsWebNavigation($$$)'
ast-grep --pattern 'fun childPagesWebNavigation($$$)'
ast-grep --pattern 'fun childStackWebNavigation($$$)'
Length of output: 168
Script:
#!/bin/bash
# Search for the web navigation function implementations with context
rg "fun child.*WebNavigation" -A 10 --type kotlin
Length of output: 51
Script:
#!/bin/bash
# Search for web navigation implementations in specific files
rg "WebNavigation" --type kotlin -A 5 decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/panels/
rg "WebNavigation" --type kotlin -A 5 decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/pages/
rg "WebNavigation" --type kotlin -A 5 decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/
Length of output: 20813
Supports Child Stack, Child Pages and Child Panels navigation models. Supporting Child Slot is a bit tricky and most likely will require additional API changes, maybe later.
As part of #111.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests