Mvj 453 hds login #521
Merged
Mvj 453 hds login #521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add new silent_renew.html from HDS docs remove unneeded tests, add a few semiuseful reducer/action tests
henrinie-nc
commented
Sep 18, 2024
| } = this.props; | ||
| const CallbackPage = (props: Props) => { | ||
| const onSuccess = (user: User) => { | ||
| const { history } = props; | ||
| history.push(getRedirectUrlFromSessionStorage() || getRouteById(Routes.LEASES)); |
There was a problem hiding this comment.
Check if the url startswith /callback and don't store that into session storage. Could happen if auth callback from oidc server fails.
henrinie-nc
commented
Sep 18, 2024
| const { getStoredApiTokens } = useApiTokens(); | ||
|
|
||
| const setLoggedInIfApiTokenExists = useCallback(() => { | ||
| const [_error, apiToken] = getStoredApiTokens(); |
There was a problem hiding this comment.
Consider logging possible error
There was a problem hiding this comment.
Normally _error is null, so if we want to log possible errors from getStoreApiTokens, it could be logged inside an if (_error) {} block. Might help debugging, but not necessary.
juho-kettunen-nc
approved these changes
Sep 18, 2024
NC-jsAhonen
approved these changes
Sep 19, 2024
| const { getStoredApiTokens } = useApiTokens(); | ||
|
|
||
| const setLoggedInIfApiTokenExists = useCallback(() => { | ||
| const [_error, apiToken] = getStoredApiTokens(); |
There was a problem hiding this comment.
Normally _error is null, so if we want to log possible errors from getStoreApiTokens, it could be logged inside an if (_error) {} block. Might help debugging, but not necessary.
apiTokensRemovedSignal is also sent during token update process, not just on removal
oidcRenewing was not called, and nothing was returned from the callback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I would like feedback on:
Also if you see something obvious that triggers rerenders that would be nice. Not really ideal that e.g.
USER_FOUNDactions trigger multiple times. Is there a better way to define theuseEffect's (One cause is probablymapStatesToProsonApp, could see if something can be fixed there.Check the docs here: https://hds.hel.fi/components/login/