Wait for previous navigation done before navigating to param with new updated value #73
Conversation
…h new updated value
|
WalkthroughThe recent update enhances the Changes
Related issues
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 (
|
✅ Deploy Preview for sveltekit-search-params ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/lib/sveltekit-search-params.ts (2 hunks)
Additional comments: 1
src/lib/sveltekit-search-params.ts (1)
- 341-349: The logic to delay the
_setfunction execution until navigation is complete is a smart approach to solving the issue at hand. However, there are a few considerations:
- Recursion: The recursive call to
_setwithin the subscription callback (line 344) is an interesting solution but could potentially lead to issues if the navigation status changes are frequent or if there are multiple rapid successive navigations. It's important to ensure that this does not create an infinite loop or excessive stack depth under certain conditions.- Unsubscription: The subscription to the
navigatingstore within the_setfunction is correctly managed with anunsubscribecall (line 345). This is good practice and helps prevent memory leaks.Consider adding safeguards to prevent potential issues with recursion and ensure that the solution scales well with frequent or complex navigation patterns.
src/lib/sveltekit-search-params.ts
Outdated
| let isNavigating = false; | ||
| if (browser) { | ||
| navigating.subscribe((nav) => { | ||
| isNavigating = nav?.type === 'goto'; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The investigation into the usage of navigating.subscribe within src/lib/sveltekit-search-params.ts reveals that there is indeed an explicit unsubscription mechanism in place within the _set function, which addresses the concern about potential memory leaks by ensuring proper cleanup. This demonstrates a level of care in managing subscriptions to the navigating store. However, the initial concern regarding the potential performance impact of multiple subscriptions, especially in applications with frequent navigation updates, remains valid. It's recommended to continue monitoring and optimizing subscription management to the navigating store to mitigate any potential performance issues.
Analysis chain
The implementation of subscribing to the navigating store to set isNavigating based on the navigation type is a clever way to handle the issue of rapid successive navigation commands. However, there are a couple of points to consider:
- Performance: Subscribing to the
navigatingstore every time thequeryParamfunction is called could potentially lead to performance issues, especially in applications with frequent navigation updates. It might be more efficient to manage the subscription at a higher level or use a shared state to track navigation status. - Unsubscription: It's good practice to unsubscribe from Svelte stores when the subscription is no longer needed to prevent memory leaks. In this case, the subscription created on line 332 does not seem to be explicitly unsubscribed from. While Svelte might handle this automatically in some scenarios, explicitly managing subscriptions is generally safer.
Consider optimizing the subscription management to the navigating store to improve performance and ensure proper cleanup.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there's a shared navigation status management implemented elsewhere in the codebase
rg "navigating.subscribe" --type ts
Length of output: 194
Script:
#!/bin/bash
# Extract more details about the second occurrence of navigating.subscribe and its surrounding code
rg -C 5 "const unsubscribe = navigating.subscribe" --type ts
Length of output: 785
|
Hey thanks for the contribution...i think this could be a good idea but i'm a little worried about not unsubscribing from navigating. I think a better approach would be to subscribe to navigating inside each builder function (basically If you need guidance on this feel free to hit me up. Also it would be wonderful if you could write a failing test that pass after the fix, again if you need guidance on this i can help you 😄 |
|
Hey! Thanks and glad you like the suggested approach :) Yes subscribing globally feels a bit tricky indeed, will try to fix that. Also the test sounds good indeed! Haven't explored testing yet here so will do so when I have a bit more time on my hands. Will let you know if I run into any trouble Also do you know why Netlify action is failing? Seems to be having trouble installing dependencies for some reason.. |
I think they are failing because you are not in my organization but should not be a problem at all. Let me know if you need any help with testing or in general to fix this. |
|
Actually is failing because I've updated sveltekit and netlify project is still running on node 16...will fix that |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- playground/src/routes/paramOnNavigateBug/+page.svelte (1 hunks)
- src/lib/sveltekit-search-params.ts (5 hunks)
- tests/index.test.ts (1 hunks)
Additional comments: 4
playground/src/routes/paramOnNavigateBug/+page.svelte (1)
- 12-15: The button's click handler correctly updates
param1, demonstrating proper use of Svelte's reactive store assignments.src/lib/sveltekit-search-params.ts (2)
- 338-346: The logic to delay updates during navigation by checking
isNavigatingis a good approach. However, consider optimizing the subscription management to thenavigatingstore to prevent potential performance issues and ensure proper cleanup. Specifically, ensure that subscriptions are unsubscribed when no longer needed.- 440-451: Ensure consistent application of subscription management practices throughout the file. Specifically, apply an unsubscription mechanism for all subscriptions to the
navigatingstore to prevent memory leaks and optimize performance.tests/index.test.ts (1)
- 136-154: The test case correctly verifies that changing multiple parameters updates the URL as expected. To further improve the test's robustness, consider adding assertions for the initial state of
param1andparam2before the button click. This ensures that the test covers the scenario where parameters are updated from their initial values.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- playground/src/routes/paramOnNavigateBug/+page.svelte (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- playground/src/routes/paramOnNavigateBug/+page.svelte
|
Hmm went a bit further with this one and it looks like the current solution won't solve it in the case described in #71. The problem is that in my app, the new query param would mount after the other one was updated. But in this case, 1 param directly depends on and is subscribed to the updates of the other param. Making updating really fast. Too fast for navigating to even start working. See some debug logs locally:
In the playground test case I added, navigating gets triggered for both param updates, before navigating state is updated
I'm pretty deep in it but unfortunately not sure how to solve this one 😓 |
Mmm interesting...i will try to investigate this a bit myself. This is definitely a bit of an edge case but it's definitely something worth investing into fixing. |
|
Ok i've investigating this a bit and yeah there's no way to wait for the navigation without disrupting the rest of the flow and there's no way to know that you want to do that just by looking at the assignment. Maybe the best option would be another api. I will try as soon as i have time. |
|
Yeah it's definitely a very tricky one! I wish I had some more time to look into it as well.. hope you can find a nice solution.. |
This PR tries to solve the bug explained in issue: #71
The problem is that navigation is "overwritten" when there is another store update right after the previous one (for example when a component mounts that has a
bind:valueto a new store that usesqueryParam.The way I managed to solve it is by subscribing to the
navigatingstore provided by Sveltekit. This way you can know if it's currently navigating to a different url or page. Then when this is the case in the_setaction, I just subscribe to the same store again there to wait until navigating is back to false, and then perform the same_setaction. This solved it for me!Let me know if this is a viable solution. I'm no expert in Sveltekit's internals so I have no idea if this is a proper/safe fix with or without any side effects. I'm just creating a simple app that doesn't have any server routing. But this did solve it in my case.
Also I'm not sure if the same bug appears in the other provided store builder
queryParameters. I'm not using that one but looking at the code it looks very similar, so maybe a similar fix will work there. But not sure. Let me know if this is any good 😅Summary by CodeRabbit
queryParamfunction to handle navigation states effectively.param2whenparam1changes using query parameters.