Extract BrowserHistory interface for testing #817
Conversation
WalkthroughThe changes in this pull request involve the removal of the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🔇 Additional comments (4)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt (1)
The implementation provides a well-structured set of browser history operations with:
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (2)
The Apply this diff to rename the extension functions for clarity: -private fun BrowserHistory.pushState(configuration: C) {
+private fun BrowserHistory.pushConfigurationState(configuration: C) {
pushState(items = browserHistory.getItems() + PageItem(configuration = configuration))
}
-private fun BrowserHistory.replaceState(configuration: C) {
+private fun BrowserHistory.replaceConfigurationState(configuration: C) {
replaceState(items = browserHistory.getItems().dropLast(1) + PageItem(configuration = configuration))
}
Fix potential race condition in pop state handling The temporary pop state listener setup could lead to race conditions if multiple navigation events occur rapidly. Apply this diff to handle the transition more robustly: - browserHistory.setOnPopStateListener {
- browserHistory.setOnPopStateListener(::onPopState)
- // Push new pages to the history
- for (i in firstDifferentIndex..newConfigurationStack.lastIndex) {
- browserHistory.pushState(newConfigurationStack[i])
- }
- }
+ val originalListener = ::onPopState
+ var transitionComplete = false
+ browserHistory.setOnPopStateListener { state ->
+ if (!transitionComplete) {
+ transitionComplete = true
+ browserHistory.setOnPopStateListener(originalListener)
+ // Push new pages to the history
+ for (i in firstDifferentIndex..newConfigurationStack.lastIndex) {
+ browserHistory.pushState(newConfigurationStack[i])
+ }
+ } else {
+ originalListener(state)
+ }
+ }decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (1)
The test implementation properly uses TestBrowserHistory and covers all critical scenarios:
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 (
|
arkivanov
force-pushed
the
browser-history-testing
branch
from
efac05c to
0b8a041
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt (2)
5-7: Consider adding runtime type checking for safer state castingThe current implementation uses
unsafeCastwhich could lead to runtime errors if the state is not a String. Consider adding type checking before the cast.- override val state: String? get() = window.history.state?.unsafeCast<String>() + override val state: String? get() { + val currentState = window.history.state + return when { + currentState == null -> null + currentState is String -> currentState + else -> { + console.warn("History state is not a String: $currentState") + currentState.toString() + } + } + }
13-19: Consider adding KDoc comments explaining the empty title parameterThe implementation is correct, but it would be helpful to document why an empty string is used for the title parameter, as it might not be immediately obvious to other developers.
+ /** + * Pushes a new state to the browser history. + * Note: title parameter is set to empty string as it's ignored by most browsers. + */ override fun pushState(data: String?, url: String?) { window.history.pushState(data = data, title = "", url = url) } + /** + * Replaces the current state in the browser history. + * Note: title parameter is set to empty string as it's ignored by most browsers. + */ override fun replaceState(data: String?, url: String?) { window.history.replaceState(data = data, title = "", url = url) }decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (1)
72-76: Optimize Initial History State SetupWhen initializing the history, consider batching
pushStateoperations or optimizing the loop to improve performance.decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt (1)
8-8: Prevent Potential Memory Leaks with ListenerEnsure that
onPopStateListeneris cleared after tests to avoid retaining references longer than necessary.Add a method to remove the listener when it's no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt(1 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt(6 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/BrowserHistory.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt(33 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt(0 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestWindow.kt(0 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt(1 hunks)
💤 Files with no reviewable changes (5)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestWindow.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
- decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
✅ Files skipped from review due to trivial changes (1)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt
🔇 Additional comments (12)
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt (3)
1-4: LGTM!
Package structure and imports are clean and appropriate for browser-specific implementation.
9-11: LGTM!
The navigation implementation correctly delegates to the browser's history API.
5-24: Verify test coverage and usage patterns
The implementation looks solid, but we should ensure:
- Test coverage exists for all scenarios including error cases
- The implementation is used consistently across the codebase
✅ Verification successful
Let me check the test implementation to ensure proper test coverage.
Implementation is well-tested and properly used
The verification reveals:
- Comprehensive test coverage exists through
TestBrowserHistorymock implementation - Extensive test cases cover all scenarios including:
- Push/pop state operations
- Browser back/forward navigation
- Error cases and edge conditions
- State management and history stack manipulation
- The implementation is consistently used via
DefaultWebHistoryControllerwhich properly handles all browser history operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to BrowserHistory
fd -e kt -e test ".*BrowserHistory.*Test.*"
# Search for usage patterns of BrowserHistory interface
rg "BrowserHistory" --type kotlin
Length of output: 2711
Script:
#!/bin/bash
# Check the test implementation
cat decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt
# Check the actual test usage
cat decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt
Length of output: 18403
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (4)
11-12: Import Statements Updated Appropriately
The new imports for BrowserHistory and DefaultBrowserHistory are necessary and correctly added.
20-20: Constructor Refactored to Use BrowserHistory Interface
Replacing the constructor parameter with BrowserHistory enhances flexibility and allows for better testing and customization.
Also applies to: 24-24
56-56: Verify Listener Registration for Pop State Events
Confirm that setOnPopStateListener correctly registers the onPopState callback to handle browser navigation events.
100-100: Confirm Browser Compatibility of go(delta) Method
Using browserHistory.go(delta) is essential for navigation. Verify that this method behaves consistently across different browsers and handles edge cases appropriately.
Run the following script to check for any browser-specific issues:
✅ Verification successful
Let me check the test file to understand how the go(delta) method is being tested.
Browser History Navigation Implementation is Well-Tested
The review concern about browser compatibility of go(delta) can be marked as verified. The test file demonstrates comprehensive testing of browser history navigation:
- Tests cover forward/backward navigation with different delta values (-1, 1)
- Edge cases are tested including:
- Navigation with multiple stack entries
- Back/forward navigation sequences
- Stack manipulation during navigation
- Navigation cancellation
- State preservation during navigation
- The implementation includes Node.js environment detection and skipping
The code is well-protected by a thorough test suite that verifies the behavior across various scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `go(delta)` is used appropriately in the codebase.
# Test: Search for all usages of `go(delta)` in the codebase.
# Expected Result: Ensure that `go(delta)` is called with correct delta values.
rg 'go\(\s*delta\s*=\s*[-]?\d+\s*\)'
Length of output: 1605
Script:
#!/bin/bash
# Get the content of the test file to see the test cases and assertions
cat decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt
Length of output: 16566
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/BrowserHistory.kt (1)
3-11: Interface Definition Aligns with Browser History API
The BrowserHistory interface methods mirror the standard browser history API, providing a clear contract for implementations.
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (4)
19-20: Updated Test Variables for New History Implementation
The test now correctly uses TestBrowserHistory instead of TestWindow, aligning with the changes in the main code.
62-63: Consistency in Router Push Methods
Ensure that using router.pushNew(Config(1)) is consistent with the router's API and other tests. If pushNew is a new method, verify its purpose and usage across the codebase.
Line range hint 68-79: Consider Deprecation of router.push Method
The usage of router.push(Config(0)) might be outdated. Check if router.push should be replaced with router.pushNew or another method for consistency.
Also applies to: 141-144
122-126: Ensure Pending Operations are Processed After Router Actions
Calling history.runPendingOperations() after router actions is crucial to simulate browser behavior accurately in tests.
Also applies to: 189-195, 208-214
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt
Outdated
Show resolved
Hide resolved
...ebMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Show resolved
Hide resolved
...ebMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Show resolved
Hide resolved
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
browser-history-testing
branch
from
0b8a041 to
bb0fbea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (1)
Line range hint
1-591: Overall: Well-structured test migration with comprehensive coverageThe test file has been successfully migrated to use the new BrowserHistory interface while maintaining:
- Comprehensive test coverage
- Clear test scenarios
- Good testing practices
- Consistent error handling
The changes align perfectly with the PR objective of extracting the BrowserHistory interface for testing.
Consider adding the following test scenarios in future updates:
- Concurrent history operations
- Edge cases with empty or malformed paths
- Browser-specific history limit handling
decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt (2)
13-15: Consider documenting the empty title parameter usageThe empty title parameter in both
pushStateandreplaceStatemethods might be confusing. Consider adding a comment explaining why it's intentionally left empty, or if it should be made configurable.Also applies to: 17-19
21-23: Consider handling PopState event cleanupThe current implementation sets the onpopstate handler but doesn't provide a way to clean it up. Consider adding a method to remove the listener when it's no longer needed to prevent memory leaks.
+ private var currentListener: ((state: String?) -> Unit)? = null + override fun setOnPopStateListener(listener: (state: String?) -> Unit) { - window.onpopstate = { listener(it.state?.toString()) } + currentListener = listener + window.onpopstate = { currentListener?.invoke(it.state?.toString()) } } + + fun removePopStateListener() { + currentListener = null + window.onpopstate = null + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt(1 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt(6 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt(0 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/BrowserHistory.kt(1 hunks)decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt(1 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt(33 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt(0 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestWindow.kt(0 hunks)decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt(1 hunks)
💤 Files with no reviewable changes (5)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestWindow.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/TestHistory.kt
- decompose/src/wasmJsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerWindow.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt
- decompose/src/jsMain/kotlin/com/arkivanov/decompose/router/webhistory/DefaultBrowserHistory.kt
- decompose/src/webMain/kotlin/com/arkivanov/decompose/router/webhistory/BrowserHistory.kt
- decompose/src/webTest/kotlin/com/arkivanov/decompose/router/webhistory/TestBrowserHistory.kt
🔇 Additional comments (7)
decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryControllerTest.kt (4)
8-10: LGTM: Clean migration to BrowserHistory interface
The changes effectively replace the window-based history management with the new BrowserHistory interface, maintaining a clean separation of concerns.
Also applies to: 19-20
Line range hint 568-581: LGTM: Helper methods properly adapted
The helper methods have been cleanly adapted to work with the new BrowserHistory interface while maintaining their original functionality and clarity.
Also applies to: 583-587
62-62: Verify consistent usage of pushNew across the codebase
The migration from push to pushNew appears to be systematic. Let's ensure this change is consistent across the entire codebase.
Also applies to: 122-122, 189-189, 194-194, 208-208, 213-213, 350-350, 367-367, 386-386, 405-405, 426-426
Line range hint 292-294: Consider addressing the corner case in history management
There's a noted corner case where old pages remain in the history. This behavior might be unexpected and could lead to confusion. Consider:
- Documenting this behavior in the BrowserHistory interface
- Adding a test case specifically for verifying this behavior is intended
✅ Verification successful
Let me gather more context about this corner case by examining the implementation and tests more thoroughly.
Let me check one more thing to understand if this behavior is documented in the interface or if there are related tests that verify this behavior.
The corner case is a documented and tested behavior, no action required
The corner case where old pages remain in the history is an intentionally documented behavior in both the implementation (DefaultWebHistoryController.kt) and verified by a specific test case. This occurs in a specific scenario when pages are popped but there are no new pages to push, which is a valid edge case in web browser history management. The test GIVEN_router_with_initial_stack_of_a_b_WHEN_navigate_to_c_THEN_history_is_c_b_and_c_is_active explicitly verifies this behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar corner cases in other test files
rg -B 2 -A 2 "Corner case:" --type kotlin
Length of output: 1642
Script:
#!/bin/bash
# Get more context around the corner case in the implementation
rg -B 10 -A 10 "Corner case: if there is nothing to push" decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
# Get the full test method context
rg -B 15 -A 5 "Corner case: old pages remain in the history" decompose/src/webTest/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController*.kt
Length of output: 1989
Script:
#!/bin/bash
# Check BrowserHistory interface and related documentation
rg -B 5 -A 5 "interface BrowserHistory" --type kotlin
# Look for any documentation about history state management
rg -B 3 -A 3 "pushState|replaceState" --type kotlin decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/
Length of output: 13138
decompose/src/webMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt (3)
29-31: Ensure Safe Handling of Nullable state
The getItems() function handles a nullable state by returning an empty list as fallback. Consider adding error handling for malformed JSON to prevent runtime exceptions.
189-194: Avoid Naming Conflicts with Extension Functions
The extension function names conflict with the BrowserHistory interface methods, which could lead to confusion.
71-77: Consider handling edge cases in history initialization
The history initialization logic assumes the stack always has at least one configuration. Consider adding validation:
fun init() {
- if (browserHistory.getItems().isEmpty()) {
+ if (browserHistory.getItems().isEmpty() && configurations.isNotEmpty()) {
val configurations = stack.value.configurations()
browserHistory.replaceState(configurations[0])
for (i in 1..configurations.lastIndex) {
browserHistory.pushState(configurations[i])
}
+ } else if (configurations.isEmpty()) {
+ // Handle empty stack case
+ browserHistory.replaceState(emptyList())
}
...ebMain/kotlin/com/arkivanov/decompose/router/stack/webhistory/DefaultWebHistoryController.kt
Show resolved
Hide resolved
arkivanov
force-pushed
the
browser-history-testing
branch
from
bb0fbea to
120ddcc
Compare
Summary by CodeRabbit
New Features
BrowserHistoryinterface for improved browser history management.DefaultBrowserHistoryimplementation for managing browser history.TestBrowserHistoryfor simulating browser history in tests.Bug Fixes
Removals
DefaultWebHistoryControllerWindow,TestHistory, andTestWindow.