Skip to content

fix: show user logout reason message#8061

Merged
jackkav merged 1 commit intoKong:developfrom
ryan-willis:fix-logout-message-passing
Oct 15, 2024
Merged

fix: show user logout reason message#8061
jackkav merged 1 commit intoKong:developfrom
ryan-willis:fix-logout-message-passing

Conversation

@ryan-willis
Copy link
Contributor

@ryan-willis ryan-willis commented Oct 9, 2024

I noticed that there's a message attached to the app URI when a session expires:

[main] Open Deep Link URL insomnia://app/auth/login?message=Your+session+has+expired.+Please+reauthenticate+to+continue+using+Insomnia+Sync.

Here's the result of the change:
Screenshot 2024-10-09 at 15 28 22

I also included the ability to specify a custom timeout for fetch calls to the API; it's not in use right now but was helpful when debugging some connectivity issues

@filfreire filfreire requested a review from a team October 10, 2024 08:25
@jackkav
Copy link
Contributor

jackkav commented Oct 14, 2024

Timeout: I see no reason not to add the timeout option but we should take caution before modifying it in production as it can contribute to outage conditions.
localStorage: Since we don't persist any other x-insomnia-command contents to locaStorage what the rationale to start doing it in this case? Can we make that clearer from the implementation?

Comment on lines +67 to +74
const logoutMessage = window.localStorage.getItem('logoutMessage');
useEffect(() => {
if (logoutMessage) {
window.localStorage.removeItem('logoutMessage');
setMessage(logoutMessage);
}
}, [logoutMessage]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a useLocalstorage.ts file containing this hook

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed its also deleting after reading, which make its it different to the hook.

@ryan-willis
Copy link
Contributor Author

ryan-willis commented Oct 14, 2024

@jackkav

localStorage: Since we don't persist any other x-insomnia-command contents to locaStorage what the rationale to start doing it in this case? Can we make that clearer from the implementation?

The rationale was to imitate prior art: we persist contents to localStorage in the /app/open/organization case at the end of the switch block, which has the same pattern where a temporary value is set in localStorage, then removed later.
https://github.com/Kong/insomnia/blob/develop/packages/insomnia/src/ui/routes/root.tsx#L218
https://github.com/Kong/insomnia/blob/develop/packages/insomnia/src/ui/routes/organization.tsx#L282

The other method I thought of was to pass parameters through the /auth/logout action function, so if that's a better pattern to follow or clears up the intent I can make that change

origin,
headers,
onlyResolveOnSuccess = false,
timeout = INSOMNIA_FETCH_TIME_OUT,
Copy link
Contributor

@jackkav jackkav Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not generally encouraged to use default args in this codebase, as we prefer fallbacks defined in the logic where we can co locate them with other fallbacks or logic. onlyResolveOnSuccess is an exception, I'll see if there is a way to avoid these

@jackkav jackkav merged commit 3878f5b into Kong:develop Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants