-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: change query persister to chrome storage, closes #5153 #5172
Conversation
402c16e
to
7e2de0c
Compare
7e2de0c
to
d11830c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix, but caution adding the /
@@ -34,7 +34,7 @@ export enum RouteUrls { | |||
ViewSecretKey = '/view-secret-key', | |||
|
|||
// nested routes must have relative paths | |||
Activity = 'activity', | |||
Activity = '/activity', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Maybe this have unintended consequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is ui bug - if refresh page being on activity tab => tab is not active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested actually, and seems all good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this will add a warning in the console, that's why I added the comment there:
// nested routes must have relative paths
I think we should solve the refresh bug differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and I'm pretty sure I had to change all of these routes here to avoid a console warning but I can't see that now after this change so I think it could be OK.
In future, if you are fixing bugs you should at least have a separate commit message for that fix saying what it is and why in case we need to revisit it
Problem in transactions caching was that txs list size is too big to be cached in local storage, so I changed persister to
chrome.storage.local