Skip to content

fix: Reload page after redirect flow ends to avoid reactivity issues#32766

Closed
KevLehman wants to merge 8 commits intodevelopfrom
fix/hotfix-oauth-racecond
Closed

fix: Reload page after redirect flow ends to avoid reactivity issues#32766
KevLehman wants to merge 8 commits intodevelopfrom
fix/hotfix-oauth-racecond

Conversation

@KevLehman
Copy link
Copy Markdown
Member

@KevLehman KevLehman commented Jul 11, 2024

https://rocketchat.atlassian.net/browse/SUP-619

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

PR adds an additional step to the redirect flow of OAuth logins, refreshing the page to recover reactivity on the UI.

This should fix most user inconsistencies happening after a login via OAuth (redirect). This shouldn't affect any other type of login, like, for example, popup flow.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jul 11, 2024

🦋 Changeset detected

Latest commit: 4ae5f54

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot bot commented Jul 11, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is targeting the wrong base branch. It should target 6.12.0, but it targets 6.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.

Project coverage is 55.58%. Comparing base (703af95) to head (4ae5f54).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32766      +/-   ##
===========================================
+ Coverage    55.56%   55.58%   +0.02%     
===========================================
  Files         2635     2635              
  Lines        57292    57309      +17     
  Branches     11865    11869       +4     
===========================================
+ Hits         31834    31856      +22     
+ Misses       22768    22757      -11     
- Partials      2690     2696       +6     
Flag Coverage Δ
e2e 54.32% <52.94%> (+0.03%) ⬆️
unit 72.25% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@KevLehman KevLehman requested a review from tassoevan July 12, 2024 15:03
@KevLehman KevLehman marked this pull request as ready for review July 12, 2024 15:33
@KevLehman KevLehman requested a review from a team as a code owner July 12, 2024 15:33
Copy link
Copy Markdown
Contributor

@MarcosSpessatto MarcosSpessatto left a comment

Choose a reason for hiding this comment

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

Is there any possibility of adding automated tests to cover this? Asking because I'm not sure if we can simulate that...

Also, can we add a description to the PR?

@KevLehman
Copy link
Copy Markdown
Member Author

About testing, i don't think it's possible 😬 right now. I don't think we have any oauth login test 🤔and for reliability we would need an oauth server configured for the CI (like a keycloack server). Totally doable, but a project in itself

@KevLehman KevLehman requested a review from a team as a code owner July 12, 2024 19:09
@MarcosSpessatto MarcosSpessatto added this to the 6.11 milestone Jul 15, 2024
@MarcosSpessatto MarcosSpessatto modified the milestones: 6.11, 6.12 Jul 18, 2024
@ggazzo
Copy link
Copy Markdown
Member

ggazzo commented Jul 29, 2024

could you check if this #32929 solves the case?
I was not aware this was a real issue, I though only test were facing it

@KevLehman
Copy link
Copy Markdown
Member Author

let me check

@KevLehman
Copy link
Copy Markdown
Member Author

Closing this for now as apparently linked PR fixed the issue 🎉

@KevLehman KevLehman closed this Jul 29, 2024
@tassoevan tassoevan deleted the fix/hotfix-oauth-racecond branch July 30, 2025 14:59
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.

3 participants