[MM-63224] Add incompatible server screen by devinbinnie · Pull Request #3348 · mattermost/desktop

devinbinnie · 2025-02-20T18:35:41Z

Summary

To help ease the transition for some users on older server versions that have run into compatibility issues with the newest Desktop App version, we have implement a screen with a check for the server version, that will show up when the user tries to use the Desktop App with an older version. This screen also includes a link to a forum post that can help users or system admins upgrade their servers.

Ticket Link

https://mattermost.atlassian.net/browse/MM-63224
Closes #3333

Device Information

This PR was tested on: macOS Sonoma

Screenshots

Release Note

Add incompatible version screen

esethna

Thanks Devin, looks like it's replacing "Mattermost" with "Electron" incorrectly

devinbinnie · 2025-02-20T18:39:56Z

Thanks Devin, looks like it's replacing "Mattermost" with "Electron" incorrectly

Sorry, that's there for whitelabelling. In the built version it will say "Mattermost". I'll replace the screenshots.

lieut-data

Two thoughts, but non-blocking and will defer to you @devinbinnie.

lieut-data · 2025-02-20T19:11:24Z

 export const DEFAULT_ACADEMY_LINK = 'https://academy.mattermost.com/';
 export const DEFAULT_TE_REPORT_PROBLEM_LINK = 'https://mattermost.com/pl/report-a-bug';
 export const DEFAULT_EE_REPORT_PROBLEM_LINK = 'https://support.mattermost.com/hc/en-us/requests/new';
+export const DEFAULT_UPGRADE_LINK = 'https://forum.mattermost.com/t/mattermost-desktop-app-5-11-important-compatibility-notice/22599';


Should we use a permalink here instead?

@esethna Thoughts? Would be better to set this up now so we can change it later once we want to link to a doc entry.

Yes for sure, we should pl so we can redirect as needed in the future

lieut-data · 2025-02-20T19:12:52Z

+            const serverInfo = ServerManager.getRemoteInfo(this.view.server.id);
+            if (serverInfo?.serverVersion && semver.lt(serverInfo.serverVersion, '9.4.0')) {
+                MainWindow.sendToRenderer(LOAD_INCOMPATIBLE_SERVER, this.id, loadURL.toString());
+                this.emit(LOAD_FAILED, this.id, 'Incompatible server version', loadURL.toString());
+                this.status = Status.ERROR;
+                return;
+            }


Should this logic default to showing the error /unless/ we successfully get the remote info and confirm it's >= 9.4.0? Worried about edge cases with old servers that we won't know about vs. confidence in the server being new enough for this request and check to work reliably.

Yeah that makes sense, the only thing I worry about is if we somehow don't get the configuration from the API and then they get the incompatible screen on a seemingly working server. I think though if we're somehow ending up not getting that info into the app that would be a different bug we'd want to expose, so let's go your way.

If we default it to show, what happens if the user is on a spotty connection, is there a chance the version check fails and returns the error screen locking the user out? @devinbinnie

Also for QA, I propose we spot check this on a couple newer release versions to ensure we aren't getting a false positive and displaying the error screen, especially on the v9.5 v9.11 v10.5 ESRs cc// @yasserfaraazkhan

It would be a very small chance, but easily verifiable via refreshing. I think Jesse's point might cover more cases since it's clear we might have users on much much older versions.

Should we add to the bullet list to attempt refreshing the app in that case if that would be a troubleshooting step?

For example:

Other than a spotty connection are there other cases where we might show this screen incorrectly sending the user down the wrong path (downgrading) to resolution ?

For example:

Server is unreachable momentarily

Server is down for maintenance
etc

Worried we might be showing this screen in way too many cases that result in the user spending energy on unhelpful resolution steps that will make them more frustrated vs a generic error screen

So to clarify, the only case where we'd be showing this screen is if with the two requests made to the server that check for version and just hit the homepage, if the version check one fails but the homepage one succeeds and is able to load the app. They should happen within milliseconds of each other.

The app makes somewhere around 50 requests to load the app, and if any one of them were to fail, the app would be in a degraded state. I think we might be trying to cover too many potential cases here, and not just focusing on the main issue of the server upgrade issue.

Remember that this needs to go into a patch release, so we want to minimize the amount of checks needed here. I think what we have is more than sufficient and will be plenty robust for all users.

So to clarify, the only case where we'd be showing this screen is if with the two requests made to the server that check for version and just hit the homepage, if the version check one fails but the homepage one succeeds and is able to load the app.

Got it, thanks

larkox

LGTM

devinbinnie · 2025-02-21T15:17:28Z

@yasserfaraazkhan Going to defer QA since this will be tested with the dot release

mattermost-build · 2025-02-21T15:17:53Z

Cherry pick is scheduled.

* [MM-63224] Add incompatible server screen * Fixed issue where init isn't called on no server case * Amend check (cherry picked from commit 6fa5508)

* [MM-63224] Add incompatible server screen * Fixed issue where init isn't called on no server case * Amend check (cherry picked from commit 6fa5508) Co-authored-by: Devin Binnie <52460000+devinbinnie@users.noreply.github.com>

[MM-63224] Add incompatible server screen

93cc414

devinbinnie added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Feb 20, 2025

devinbinnie added this to the v5.11.0 milestone Feb 20, 2025

devinbinnie requested review from a team, esethna, lieut-data and yasserfaraazkhan and removed request for a team February 20, 2025 18:35

mm-cloud-bot added release-note labels Feb 20, 2025

esethna requested changes Feb 20, 2025

View reviewed changes

devinbinnie requested a review from larkox February 20, 2025 18:40

Fixed issue where init isn't called on no server case

a580266

lieut-data approved these changes Feb 20, 2025

View reviewed changes

devinbinnie requested a review from esethna February 20, 2025 19:14

Amend check

0d5535a

esethna removed the 1: PM Review Requires review by a product manager label Feb 20, 2025

esethna approved these changes Feb 20, 2025

View reviewed changes

amyblais mentioned this pull request Feb 21, 2025

v5.11.1 Desktop dot release docs mattermost/docs#7760

Merged

larkox approved these changes Feb 21, 2025

View reviewed changes

devinbinnie removed the 2: Dev Review Requires review by a core committer label Feb 21, 2025

devinbinnie added QA Deferred and removed 3: QA Review Requires review by a QA tester labels Feb 21, 2025

devinbinnie removed the request for review from yasserfaraazkhan February 21, 2025 15:17

devinbinnie added the 4: Reviews Complete All reviewers have approved the pull request label Feb 21, 2025

devinbinnie merged commit 6fa5508 into master Feb 21, 2025

devinbinnie deleted the MM-63224 branch February 21, 2025 15:17

mattermost-build pushed a commit that referenced this pull request Feb 21, 2025

[MM-63224] Add incompatible server screen (#3348)

e9ad451

* [MM-63224] Add incompatible server screen * Fixed issue where init isn't called on no server case * Amend check (cherry picked from commit 6fa5508)

mattermost-build mentioned this pull request Feb 21, 2025

Automated cherry pick of #3348 #3349

Merged

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 Feb 21, 2025

Conversation

devinbinnie commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Device Information

Screenshots

Release Note

Uh oh!

esethna left a comment

Choose a reason for hiding this comment

Uh oh!

devinbinnie commented Feb 20, 2025

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esethna Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esethna Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larkox left a comment

Choose a reason for hiding this comment

Uh oh!

devinbinnie commented Feb 21, 2025

Uh oh!

mattermost-build commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

devinbinnie commented Feb 20, 2025 •

edited

Loading

esethna Feb 20, 2025 •

edited

Loading

esethna Feb 20, 2025 •

edited

Loading