Add callback to allow/deny navigating when browser forward/back button is used #713
Conversation
WalkthroughThe recent updates across multiple files introduce an additional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebHistoryController
participant DefaultWebHistoryController
User->>WebHistoryController: attach(StackNavigator, Value, KSerializer, Function1, Function1, Function1)
WebHistoryController->>DefaultWebHistoryController: handle attach with onWebNavigation callback
DefaultWebHistoryController->>WebHistoryController: invoke onWebNavigation callback
WebHistoryController->>User: navigation allowed/denied based on callback
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- decompose/api/android/decompose.api (1 hunks)
- decompose/api/jvm/decompose.api (1 hunks)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1 hunks)
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt (1 hunks)
- decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt (1 hunks)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (4 hunks)
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (1 hunks)
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt (1 hunks)
- deps.versions.toml (1 hunks)
Additional comments not posted (14)
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt (2)
27-29: The implementation of theforwardmethod correctly uses the browser's history API to navigate forward.
31-33: The implementation of thebackmethod correctly uses the browser's history API to navigate backward.decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt (2)
27-29: The implementation of theforwardmethod correctly uses the browser's history API to navigate forward.
31-33: The implementation of thebackmethod correctly uses the browser's history API to navigate backward.decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt (2)
35-39: The implementation of theforwardmethod correctly simulates navigating forward in the history stack.
41-45: The implementation of thebackmethod correctly simulates navigating backward in the history stack.decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1)
39-39: The addition of theallowWebNavigationCallbackparameter to theattachmethod aligns with the PR's objectives to provide more control over navigation.deps.versions.toml (1)
3-3: The update of thedecomposeversion to"3.0.0-gs"is appropriate, indicating preparation for future stable releases following significant changes.decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (2)
50-52: The implementation of theattachmethod inDefaultWebHistoryControllercorrectly integrates theallowWebNavigationCallbackparameter, providing dynamic control over navigation.
166-196: ThedoOnPopStatemethod effectively handles navigation decisions based on theallowWebNavigationCallback, aligning with the PR's objectives to provide dynamic navigation control.decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (2)
423-438: The test correctly simulates and verifies the behavior when back navigation is denied using the callback, ensuring that the history remains unchanged.
440-455: The test correctly simulates and verifies the behavior when back navigation is allowed using the callback, ensuring that the history is updated correctly.decompose/api/jvm/decompose.api (1)
329-329: The addition ofattach$defaultmethod supports backward compatibility by handling default parameters. Ensure it is correctly used whereverattachis called.decompose/api/android/decompose.api (1)
349-349: Verify the implementation ofattach$defaultfor backward compatibility.Ensure that the new
attach$defaultmethod inWebHistoryController$DefaultImplscorrectly supports default arguments without breaking existing usages. This method is critical for maintaining backward compatibility when new parameters are added to methods in Kotlin.
|
Thanks! Please allow me some time to review, as I'm on vacation currently. |
...rc/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt
Outdated
Show resolved
Hide resolved
| @@ -220,5 +246,7 @@ class DefaultWebHistoryController internal constructor( | |||
| fun go(delta: Int) | |||
| fun pushState(data: String, url: String?) | |||
| fun replaceState(data: String, url: String?) | |||
| fun forward() | |||
There was a problem hiding this comment.
Are these two methods actually used?
There was a problem hiding this comment.
back is used in the doOnPopState method, forward is not used.
There was a problem hiding this comment.
Shouldn't we use go(-1) instead of back?
There was a problem hiding this comment.
Yes, good point. That should be equivalent, per the documentation here.
…dd documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1 hunks)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (4 hunks)
Additional comments not posted (2)
decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1)
32-41: The addition ofallowWebNavigationCallbackenhances navigation control. Consider adding detailed documentation for this new parameter to help developers understand its usage and impact.decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (1)
Line range hint
50-166: The implementation ofallowWebNavigationCallbackin theattachmethod and its usage inonPopStateare well-integrated. Ensure comprehensive unit tests cover these changes to validate the behavior under various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1 hunks)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
...rc/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt
Outdated
Show resolved
Hide resolved
...ebMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Outdated
Show resolved
Hide resolved
...ebMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- decompose/api/android/decompose.api (1 hunks)
- decompose/api/jvm/decompose.api (1 hunks)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt (1 hunks)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (3 hunks)
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- decompose/src/commonMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/WebHistoryController.kt
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Additional comments not posted (6)
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (3)
423-437: The test correctly simulates the scenario where back navigation is denied by the callback. It ensures that the history stack remains unchanged, which aligns with the expected behavior.
440-455: The test correctly simulates the scenario where back navigation is allowed by the callback. It ensures that the history stack is updated correctly, which aligns with the expected behavior.
458-475: The test correctly simulates the scenario where forward navigation is denied by the callback after a back navigation. It ensures that the history stack remains unchanged, which aligns with the expected behavior.decompose/api/jvm/decompose.api (1)
329-329: The addition of a default implementation for theattachmethod in theWebHistoryController$DefaultImplsclass ensures backward compatibility and follows good practices.decompose/api/android/decompose.api (2)
344-344: The addition of theFunction1parameter to theattachmethod is a good enhancement for handling navigation control.Please ensure to document this new parameter to help developers understand its usage and impact.
349-349: The addition of theattach$defaultmethod supports backward compatibility by handling default values for the new parameter.Please verify that this integrates seamlessly with existing implementations that use the
attachmethod.
|
Looks good, thanks! Do you mind to resolve the conflict? I'm happy to approve and merge after that. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- decompose/api/android/decompose.api (1 hunks)
- decompose/api/jvm/decompose.api (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- decompose/api/android/decompose.api
- decompose/api/jvm/decompose.api
|
Oh, I think we forgot to update the API files. Could you please run |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- decompose/api/decompose.klib.api (3 hunks)
Additional comments not posted (2)
decompose/api/decompose.klib.api (2)
93-93: The updatedattachmethod signature correctly includes the new callback parameter for navigation control. Ensure all implementations of this interface are updated accordingly.
170-175: TheDefaultWebHistoryControllerclass correctly implements the updatedattachmethod and includes the newhistoryPathsproperty. Ensure this class's usage reflects the new method signature and property.
|
Thank you for working on this! |
This PR adds the capability to provide a callback to DefaultWebHistoryController.attach that will be invoked when the popstate event listener is called by the browser, e.g. when the back or forward buttons are used. The callback can be used to indicate to the DefaultWebHistoryController that the navigation should not be allowed, and it will reverse the back/forward action the browser performed to keep the navigation state at the current point in the stack. This should address #42 and can also be used for more general use cases, for example to prevent a user from navigating away from a screen with unsaved form data by using the forward/back buttons.
Summary by CodeRabbit
New Features
forward()andback()methods toDefaultWebHistoryControllerWindowfor browser history navigation.onWebNavigationcallback parameter to control web navigation inWebHistoryController.Bug Fixes
DefaultWebHistoryControllerby handlingonWebNavigationcallback.Documentation
CHANGELOG.mdto track versions starting from0.1.0(Alpha snapshot).Chores
decomposeversion to3.0.0-gsindeps.versions.toml.