Page MenuHomePhabricator
Differential D11643

[web] Show a popup when client_version_unsupported
ClosedPublic
Actions

Authored by inka on Fri, Apr 12, 1:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 7:29 PM
Unknown Object (File)
Mon, Apr 22, 12:55 PM
Unknown Object (File)
Mon, Apr 22, 11:03 AM
Unknown Object (File)
Sun, Apr 21, 6:41 AM
Unknown Object (File)
Sun, Apr 21, 4:06 AM
Unknown Object (File)
Sat, Apr 20, 3:47 PM
Unknown Object (File)
Sat, Apr 20, 12:57 PM
Unknown Object (File)
Sat, Apr 20, 11:44 AM
View All 21 Files
Subscribers
ashoat

Details

Reviewers
tomek
ginsu
ashoat
Commits
rCOMMe54b3a74315b: [web] Show a popup when client_version_unsupported
Summary

issue: [[ https://linear.app/comm/issue/ENG-6128/\[web\]-add-a-ui-element-to-inform-the-user-that-the-client-version-is | ENG-6128 ]]
The text is based on the text shown on native:

'App out of date',
message:
  'Your app version is pretty old, and the server doesn’t know how ' +
  `to speak to it anymore. Please use the ${platformStore} to update!`,

Screenshot 2024-04-11 at 16.47.54.png (1×1 px, 103 KB)

Note that I didn't have any designs for this, so if you think this doesn't look right, let me know!

EDIT:
After some review the modal now looks like this:

image.png (900×1 px, 88 KB)

Test Plan

Tested that if the server sends client_version_unsupported the modal shows up.
Tested that it works on web and desktop

image.png (1×2 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka created this revision.Fri, Apr 12, 1:40 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFri, Apr 12, 1:40 AM
inka added a parent revision: D11632: [lib][web][native] Pass preRequestUserState into invalidSessionRecovery.Fri, Apr 12, 1:40 AM
inka edited the summary of this revision. (Show Details)
inka attached a referenced file: F1524374: Screenshot 2024-04-11 at 16.47.54.png. (Show Details)
inka added a reviewer: ashoat.Fri, Apr 12, 1:43 AM

Adding @ashoat to review copy, and because I didn't have designs

inka edited the summary of this revision. (Show Details)Fri, Apr 12, 1:44 AM
inka edited the summary of this revision. (Show Details)Fri, Apr 12, 1:55 AM
inka edited the summary of this revision. (Show Details)
inka edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B28183: Diff 39072.Fri, Apr 12, 1:59 AM
inka requested review of this revision.Fri, Apr 12, 1:59 AM
ashoat requested changes to this revision.Fri, Apr 12, 8:21 AM
ashoat added inline comments.
web/components/version-handler.react.js
20–27 ↗(On Diff #39072)

What is this subheader for? I don't know why it's necessary

I'm guessing it's for indicating which keyserver the message came from, but I think that's confusing to the user. It doesn't really seem to affect what they need to do (refresh the page). Maybe we can remove this?

37–38 ↗(On Diff #39072)

Looks like this copy is mostly from the native version of the same message, so I think it's good.

The only new part is "Please reload the app." We should consider two cases:

  1. Desktop: do users know that it's possible to "reload" the app by using a refresh keyboard shortcut?
  2. Web: will users be confused about "reloading" the "app", as opposed to "refreshing" a "page"?

Not a huge deal, and I'm not sure we need to do much here... most users will probably figure it out. If the desktop users end up just closing and restarting the app, that should work too I think.

This revision now requires changes to proceed.Fri, Apr 12, 8:21 AM
inka added inline comments.Mon, Apr 15, 2:35 AM
web/components/version-handler.react.js
20–27 ↗(On Diff #39072)

Yes, this subheader was supposed to inform the user which server sent the error. But I can remove it if it is unnecessary.

37–38 ↗(On Diff #39072)

How about Please reload the app for desktop and Please refresh the page for web?
I feel like the users will know to either close and reopen or use the shortcut on desktop.

inka updated this revision to Diff 39108.Mon, Apr 15, 2:37 AM

Address review

inka edited the summary of this revision. (Show Details)EditedMon, Apr 15, 2:37 AM

Now the modal looks like this:

image.png (900×1 px, 68 KB)

inka attached a referenced file: F1548897: image.png. (Show Details)Mon, Apr 15, 2:39 AM
Harbormaster completed remote builds in B28213: Diff 39108.Mon, Apr 15, 2:57 AM
tomek added a comment.Mon, Apr 15, 4:22 AM

Can't we simply reload the page when the client version is unsupported?

inka added a comment.Mon, Apr 15, 4:29 AM
In D11643#334927, @tomek wrote:

Can't we simply reload the page when the client version is unsupported?

I don't know if we want to be forcing users to have the page reloaded if some secondary keyserver sends an error... This would for example delete an entry they are creating

ashoat accepted this revision.Mon, Apr 15, 7:02 AM

Accepting, but I have some change requests inline. If anything is unclear, please re-request review :)

How about Please reload the app for desktop and Please refresh the page for web?
I feel like the users will know to either close and reopen or use the shortcut on desktop.

Sounds good to me! Can you amend the Test Plan to confirm that you've tested both?

I don't know if we want to be forcing users to have the page reloaded if some secondary keyserver sends an error... This would for example delete an entry they are creating

I agree

web/components/version-handler.react.js
48–50 ↗(On Diff #39108)

This seems risky... you appear to be pushing a new modal every time this effect runs, which I think could lead to multiple of the same modal being pushed. Can you instead change the code so you only push the modal when the change appears? I think you'll need a prevConnectionIssueRef

63–65 ↗(On Diff #39108)

Can this be memoized on keyserverIDs?

This revision is now accepted and ready to land.Mon, Apr 15, 7:02 AM
inka edited the test plan for this revision. (Show Details)Mon, Apr 15, 7:12 AM
inka updated this revision to Diff 39129.Mon, Apr 15, 9:11 AM

Memoize list of components, only display the alert once for each keyserver

inka requested review of this revision.Mon, Apr 15, 9:13 AM

Requesting review because now:

  1. We can still see this modal more than once - for other keyservers. Is this that we want??
  2. For the auth keyserver, the user is logged out. If they dismiss the modal, but ignore it and log in, they will be logged out and will NOT see the modal again. Is this ok or should I make logging in reset this state?
Harbormaster completed remote builds in B28229: Diff 39129.Mon, Apr 15, 9:28 AM
ashoat added a comment.Mon, Apr 15, 9:49 AM
In D11643#335050, @inka wrote:

Requesting review because now:

  1. We can still see this modal more than once - for other keyservers. Is this that we want??
  2. For the auth keyserver, the user is logged out. If they dismiss the modal, but ignore it and log in, they will be logged out and will NOT see the modal again. Is this ok or should I make logging in reset this state?
  1. Hmm... ideally, no. But I'm not sure how complicated it would be to address this, and whether it should be prioritized.
  2. A bit confused about the scenario you describe. How is the user able to log in after being logged out, if they can't auth with the authoritative keyserver? Doesn't the authoritative keyserver make the version check during auth?
web/components/version-handler.react.js
54 ↗(On Diff #39129)

I think this code is incorrect. You're only updating prevConnectionIssueRef when connectionIssue === 'client_version_unsupported', so prevConnectionIssueRef.current will always be 'client_version_unsupported' after the first call

inka added a comment.Tue, Apr 16, 1:53 AM

A bit confused about the scenario you describe. How is the user able to log in after being logged out, if they can't auth with the authoritative keyserver? Doesn't the authoritative keyserver make the version check during auth?

Oh, you are right, sorry. My testing method didn't correctly reflect this. So this is not an issue

web/components/version-handler.react.js
54 ↗(On Diff #39129)

Do we want to show the modal again if somehow connectionIssue changes from client_version_unsupported to something else and then back?
Don't we want to show it one time?

inka updated this revision to Diff 39140.Tue, Apr 16, 3:04 AM

Show only one popup

Harbormaster completed remote builds in B28234: Diff 39140.Tue, Apr 16, 3:20 AM
ashoat accepted this revision.Tue, Apr 16, 4:45 AM
ashoat added inline comments.
web/components/version-handler.react.js
43 ↗(On Diff #39140)

Let's rename this to hasShownModalRef

54 ↗(On Diff #39129)

Ah, I see what you're saying. We can keep your code, but let's rename the ref to more accurately reflect what it's actually tracking

This revision is now accepted and ready to land.Tue, Apr 16, 4:45 AM
inka updated this revision to Diff 39142.Tue, Apr 16, 4:50 AM

Rename

Harbormaster completed remote builds in B28236: Diff 39142.Tue, Apr 16, 5:08 AM
Closed by commit rCOMMe54b3a74315b: [web] Show a popup when client_version_unsupported (authored by inka). · Explain WhyTue, Apr 16, 5:18 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMe54b3a74315b: [web] Show a popup when client_version_unsupported.

Revision Contents
Changeset List

PathSize
web/
2 lines
components/
21 lines
56 lines

Diff 39149

View Options

web/app.react.js

Loading...
View Options

web/components/version-handler.css

Loading...
View Options

web/components/version-handler.react.js

Loading...