What's New Modal: Improve version comparison logic to handle prerelease version transitions

[ivan-ottinger]

Related issues

• Resolves STU-449
• Related discussion: p1746514247326159-slack-C06DRMD6VPZ

Proposed Changes

• update the What's New modal display logic to make the modal rendered in the following extra cases:
  • transitions from one prerelease to another prerelease version (e.g. 1.5.1-beta1 to 1.5.1-beta2)
  • transitions from prerelease to stable version (and vice-versa) (e.g. 1.5.1-beta1 to 1.5.2)
• add related unit tests

Testing Instructions

1. Check out the PR branch and build the app with npm start.
2. Head over to the appdata-v1.json and try changing the lastSeenVersion in it and then reload the app.

For example (if the current app version (in package.json) is 1.5.2-beta2):

• change the lastSeenVersion to 1.5.2-beta3 → modal should render
• change the lastSeenVersion to 1.5.2 → modal should render

3. All related tests should pass (npm test src/stores/tests/app-version-api.test.ts).

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors?

[sejas]

It works as described, and I see the what's new modal when I try a different beta.

I wonder if we could simplify all these logic by using something like:

const cleanLastSeen = semver.valid(
				semver.coerce( lastSeenVersion, { includePrerelease: true } )
			);
			const cleanCurrent = semver.valid(
				semver.coerce( currentVersion, { includePrerelease: true } )
			);

That would return stable versions and also betas like 1.5.2-beta2, which we could compare:

semver.gt( cleanLastSeen , cleanCurrent ) || forceNewVersion

For me it only has sense to see the what's new modal if I go from one version to a higher version. So I wouldn't display the modal when lastSeen is 1.5.2-beta3 and current is 1.5.2-beta2. Not sure if this was already discussed.

[epeicher]

Thanks for improving this! I have tested it, and it works as advertised, I can see the modal as expected. LGTM!

I also think the suggestion by Antonio above is worth exploring.

[katinthehatsite]

I think it makes sense to me to have a modal when we transition from the prerelease to stable version but I am wondering if we need to display it when we transition from one prerelease version to another prerelease considering that we can access it from the topbar and see the modal?

I am guessing one advantage is that when we produce the beta for testers, they will be able to see the modal as well.

[ivan-ottinger]

Good question! We could leave the modal displayed on prerelease updates first and if we find it annoying, we could then adjust the logic further.

[katinthehatsite]

It terms of functionality, the code works as expected 👍 I left one question but it is not a strong preference on my end.

[gcsecsey]

Thanks for improving this @ivan-ottinger! 🙌 I can see the modal rendered on the release transitions as described. 🚢

[ivan-ottinger]

Thank you for your reviews and feedback, everyone! ❤️

It works as described, and I see the what's new modal when I try a different beta.

I wonder if we could simplify all these logic by using something like:

const cleanLastSeen = semver.valid(
				semver.coerce( lastSeenVersion, { includePrerelease: true } )
			);
			const cleanCurrent = semver.valid(
				semver.coerce( currentVersion, { includePrerelease: true } )
			);

That would return stable versions and also betas like 1.5.2-beta2, which we could compare:

semver.gt( cleanLastSeen , cleanCurrent ) || forceNewVersion

Thank you for the suggestion, Antonio!

The issue I see with semver.gt is that it does not distinguish between major/minor/patch versions. So even we could use it for the logic partially, we would still need to get more granular to filter out patch version change for which we don't want to display the modal.

For me it only has sense to see the what's new modal if I go from one version to a higher version. So I wouldn't display the modal when lastSeen is 1.5.2-beta3 and current is 1.5.2-beta2. Not sure if this was already discussed.

I have thought about this when we were implementing the initial logic, but then did not proceed to cater for this case. The reason is that moving from higher to lower version shouldn't normally happen and I did not want to complicate the logic further.

With that said, I think we could indeed consider refactoring the whole logic and make it more readable. → Created a new task: STU-516 so we can move forward with this PR. 🙂

[sejas]

@ivan-ottinger, thanks for considering my suggestion.

The issue I see with semver.gt is that it does not distinguish between major/minor/patch versions. So even we could use it for the logic partially, we would still need to get more granular to filter out patch version change for which we don't want to display the modal.

I think semver.gt correctly distinguishes between any two versions. I created a quick table, and it seems to work as we expect, except for betas higher than 9. We could change our beta naming to fix that, or we could consider it as an edgecase for this case as we never cerated so many betas for any release.

Here is a CodeSanbdox: https://codesandbox.io/p/sandbox/knlpqy

[sejas]

Ivan, Thanks for clarifying. I didn't realized that we want the minor versions to return false to avoid displaying the What's new modal. All good. You can forget my last comment xD.

[ivan-ottinger]

No worries at all! I think I could have been more clear in my last comment. When I re-read it again, it indeed wasn't communicating what I had on mind. 😅

I think semver.gt correctly distinguishes between any two versions. I created a quick table, and it seems to work as we expect, except for betas higher than 9. We could change our beta naming to fix that, or we could consider it as an edgecase for this case as we never cerated so many betas for any release.

Just adding for others (since we discussed this with Antonio already): What I meant was that semver.gt will still return true even if we are comparing for the minor version, e.g. 1.5.2 vs 1.5.1. In that case it will still return true.

But for our use, we want the modal to not display if the update is just for patch version. That is the reason we would still need additional check for that. 🙂