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

Add mechanism to check only one instance of the app is running #11416

Merged
merged 11 commits into from
Aug 22, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Aug 16, 2023

This isn't used yet, but will form part of the solution to element-hq/element-web#25157.


This change is marked as an internal change (Task), so will not be included in the changelog.

@richvdh richvdh added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 16, 2023
This isn't used yet, but will form part of the solution to
element-hq/element-web#25157.
@t3chguy
Copy link
Member

t3chguy commented Aug 16, 2023

Shouldn't this be at the js-sdk level given the issue is multiple js-sdk instances connected to the same underlying storages?

@richvdh
Copy link
Member Author

richvdh commented Aug 16, 2023

Shouldn't this be at the js-sdk level given the issue is multiple js-sdk instances connected to the same underlying storages?

Well, maybe. But it's going to have to be called by the application, so at best it could be a utility that the js-sdk provides.

As I see it: it's part of the js-sdk's contract that only one instance is connected at once. It's up to the application to enforce that.

@richvdh richvdh force-pushed the rav/element-r/tab_lockout/01_session_lock branch from b3bee0d to 203ddd2 Compare August 16, 2023 13:07
@richvdh richvdh marked this pull request as ready for review August 16, 2023 14:01
@richvdh richvdh requested review from a team as code owners August 16, 2023 14:01
@richvdh
Copy link
Member Author

richvdh commented Aug 16, 2023

The coverage metric is a lie. It's underreporting because I've had to disable coverage reporting on the new file.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Can you exclude it from the coverage reqs so Sonar passes you using https://github.com/vector-im/element-web/blob/develop/CONTRIBUTING.md#tests

Comment on lines +41 to +43
// getSessionLock is piped into a different JS context via stringification, and the coverage functionality is
// not available in that contest. So, turn off coverage instrumentation for it.
"!<rootDir>/src/utils/SessionLock.ts",
Copy link
Member Author

Choose a reason for hiding this comment

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

As I wrote in #element-dev, I've yet to think of a better solution to this.

sonar-project.properties Outdated Show resolved Hide resolved
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

We normally prevent dead code from entering the repo via automation, the automation is faulty but even so I'm not sure we should be adding code before we actually use it - https://github.com/matrix-org/matrix-react-sdk/actions/runs/5880603401/job/15947257227?pr=11416

@richvdh
Copy link
Member Author

richvdh commented Aug 16, 2023

well, don't mind keeping this PR on hold for now. I just wanted to keep the low-level mechanics separate from the application-level changes.

src/utils/SessionLock.ts Outdated Show resolved Hide resolved
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane otherwise, might it be worth considering namespacing in the ping/owner/claimant keys for when we eventually have multi-account support given presumably we'll be able to support opening different accounts in different tabs without this lockout

Comment on lines +188 to +190
const sleepPromise = new Promise((resolve) => {
setTimeout(resolve, remaining, undefined);
});
Copy link
Member

Choose a reason for hiding this comment

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

js-sdk utils has a sleep function which does just this

@richvdh richvdh requested review from t3chguy and removed request for t3chguy August 16, 2023 15:52
@richvdh
Copy link
Member Author

richvdh commented Aug 17, 2023

might it be worth considering namespacing in the ping/owner/claimant keys for when we eventually have multi-account support given presumably we'll be able to support opening different accounts in different tabs without this lockout

I'm not convinced it will, tbh.

I guess it depends how we expect multi-account support to work - in particular around what happens when you open and close tabs. Suppose I go to https://app.element.io and log in to two accounts. If I then open a new tab on https://app.element.io, then I expect the new tab to be connected to those same two accounts, right?

So the upshot is that all element-web tabs on the same host/browser are connected to the same set of accounts, and we only want one of them.

src/utils/SessionLock.ts Outdated Show resolved Hide resolved
@richvdh richvdh added this pull request to the merge queue Aug 22, 2023
Merged via the queue into develop with commit d13b6e1 Aug 22, 2023
76 checks passed
@richvdh richvdh deleted the rav/element-r/tab_lockout/01_session_lock branch August 22, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants