Skip to content
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

Fixed issues with loading the app from cold when deep linking #3201

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

devinbinnie
Copy link
Member

Summary

While working on #3200 I found an issue with deep linking when booting the app from cold. The application would fail to load the initial server, and the dropdown menu would also not load. This was caused by the main window creation call happening too early, before a bunch of other setup code could be run.

This PR fixes the issue by having the application wait for the main window to be created normally, before attempting to deep link.

Fixed issues with loading the app from cold when deep linking

@devinbinnie devinbinnie added the 2: Dev Review Requires review by a core committer label Nov 11, 2024
@devinbinnie devinbinnie requested review from larkox and a team November 11, 2024 16:07
@devinbinnie devinbinnie requested review from larkox and removed request for larkox November 11, 2024 16:07
@devinbinnie
Copy link
Member Author

devinbinnie commented Nov 11, 2024

@amyblais This might be a case for a dot release for v5.10 - as the app is somewhat unusable when users get into this state. The server will load on v5.10, but the dropdown fails.

@amyblais amyblais added this to the v5.10.0 milestone Nov 11, 2024
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Nov 11, 2024
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Thanks for the chance to take a peek! One question below.

MainWindow.show();
ViewManager.handleDeepLink(deeplinkingUrl);
} else {
MainWindow.on(MAIN_WINDOW_CREATED, () => ViewManager.handleDeepLink(deeplinkingUrl));
Copy link
Member

Choose a reason for hiding this comment

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

Are there any race conditions at play? (Could MainWindow get created between our get and on registration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I went to go double-check, I found another instance where we're calling show() before app.ready() which is really not good.

However, other callers of this are also waiting for the ready event, so as long as we're calling it after that there shouldn't be any race conditions as that's when we expect the window to be registered.

The ready event docs if you're interested: https://www.electronjs.org/docs/latest/api/app#event-ready

@devinbinnie devinbinnie added the Build Apps for PR Builds signed builds for testing label Nov 11, 2024
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM, just one small thing to verify.

src/main/app/utils.ts Show resolved Hide resolved
@devinbinnie devinbinnie added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Nov 12, 2024
@yasserfaraazkhan yasserfaraazkhan added the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Nov 13, 2024
Copy link

Here are the test results below:

Test Summary for Linux on commit d0355cb

The following known failed tests have been fixed on Linux:
- menu/view MM-T820 should open Developer Tools For Application Wrapper for main window

Test Summary for macOS on commit d0355cb

The following known failed tests have been fixed on macOS:
- popup MM-T2827_1 should be able to select all in popup windows

@github-actions github-actions bot removed the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Nov 13, 2024
@amyblais amyblais added 4: Reviews Complete All reviewers have approved the pull request QA Review Done and removed 3: QA Review Requires review by a QA tester labels Nov 13, 2024
@devinbinnie devinbinnie merged commit 1fe94eb into master Nov 13, 2024
55 of 56 checks passed
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

@devinbinnie devinbinnie deleted the deep_link_boot_bug branch November 13, 2024 15:46
mattermost-build pushed a commit that referenced this pull request Nov 13, 2024
* Fixed issues with loading the app from cold when deep linking

* Don't call show() before the window is created on Windows

(cherry picked from commit 1fe94eb)
@mattermost-build mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Nov 13, 2024
devinbinnie added a commit that referenced this pull request Nov 13, 2024
…#3205)

* Fixed issues with loading the app from cold when deep linking

* Don't call show() before the window is created on Windows

(cherry picked from commit 1fe94eb)

Co-authored-by: Devin Binnie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Build Apps for PR Builds signed builds for testing CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone QA Review Done release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants