restoreLastLocation: use localforage, don't redir outside Calypso #14196
Conversation
mcsf
added
the
[Status] Needs Review
client/state/routing/middleware.js
Outdated
| import { ROUTE_SET } from 'state/action-types'; | ||
|
|
||
| const debug = debugFactory( 'calypso:restore-last-location' ); | ||
| const key = 'last_path'; |
There was a problem hiding this comment.
Maybe
const LAST_PATH = 'last_path`;
client/state/routing/middleware.js
Outdated
| if ( ! hasInitialized ) { | ||
| hasInitialized = true; | ||
| } | ||
| localforage.getItem( key ).then( ( lastPath ) => { |
There was a problem hiding this comment.
Since this is async, we're calling next( action ) before we can call page. It's somewhat minor, but this currently updates the behavior to show a flash of / before we're redirected to another place.
matticbot
added
[Size] M
|
Thanks for the review, @gwwar! I've addressed your feedback and added guards against non-Calypso paths (p2-p58i-5Xi). |
client/state/routing/middleware.js
Outdated
| } | ||
|
|
||
| return next( action ); | ||
| } ); |
There was a problem hiding this comment.
It's unlikely, but sometimes the gets can fail. Let's also make sure we call next( action ) when that happens.
localforage.getItem( LAST_PATH ).then( redirectToLastPath, () => { return next( action ) );
- Don't save from or redirect to a path that isn't relative to Calypso. - Don't call `next` before `page` - Move `hasInitialized` state one function down to make it a store global instead of a module global
mcsf
force-pushed
the
update/restore-last-location-localforage
branch
from
0c57981 to
ce14f89
Compare
|
@gwwar, feedback addressed. Might I get a |
? :)
mcsf
changed the title
| action.path === '/' && | ||
| ! isOutsideCalypso( lastPath ) ) { | ||
| debug( 'redir to', lastPath ); | ||
| page( lastPath ); |
There was a problem hiding this comment.
I'm still seeing a flash of the reader. Perhaps we need to return early here?
There was a problem hiding this comment.
Indeed. I think we'll play with this in another PR.
|
Thanks for the review! |
mcsf
added
[Status] Ready to Merge
and removed
[Status] Needs Review
Refinement of #14070 as suggested by @gwwar (#)
Testing
Same instructions as #14070; behavior preserved, save for the improvement that the a user should never be redirected from
/to somewhere external to Calypso.