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 toast for recovery keys being out of sync #28946

Open
wants to merge 57 commits into
base: develop
Choose a base branch
from
Open

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Jan 8, 2025

Also rejigs the toast logic to hopefully make it more sensible: hopefully this is along the right lines, nobody seemed to object when I asked, at least.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

@@ -141,7 +141,7 @@ Please see LICENSE files in the repository root for full details.
}

.mx_Toast_description {
max-width: 272px;
max-width: 366px;
Copy link
Member Author

@dbkr dbkr Jan 15, 2025

Choose a reason for hiding this comment

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

This toast wants to be wider according to the designs, so we need a bigger toaster. It does mean some other toasts get wider too: design, is this desired?

Copy link
Member

Choose a reason for hiding this comment

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

@element-hq/design

@dbkr dbkr changed the title Very early WIP of rejigged e2e error toast code Add toast for recovery keys out of sync Jan 15, 2025
@dbkr dbkr marked this pull request as ready for review January 15, 2025 15:06
@dbkr dbkr requested review from a team as code owners January 15, 2025 15:06
@dbkr dbkr changed the title Add toast for recovery keys out of sync Add toast for recovery keys being out of sync Jan 15, 2025
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

@@ -141,7 +141,7 @@ Please see LICENSE files in the repository root for full details.
}

.mx_Toast_description {
max-width: 272px;
max-width: 366px;
Copy link
Member

Choose a reason for hiding this comment

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

@element-hq/design

@t3chguy t3chguy requested a review from a team January 15, 2025 15:13
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. A couple of optional comments.

await page.evaluate(async () => {
const removeCachedSecrets = new Promise((resolve) => {
const request = window.indexedDB.open("matrix-js-sdk::matrix-sdk-crypto");
request.onsuccess = async (event: Event & { target: { result: IDBDatabase } }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be async? Caution is required when mixing Indexed DB with async, so if we can remove asyncs it would make me less nervous. (I think the wider Promise that is awaited is OK, since no async work is done inside an IndexedDB callback.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, not my code (I just moved it) but I agree it doesn't look like it, and the test passes without it.

src/DeviceListener.ts Outdated Show resolved Hide resolved
src/DeviceListener.ts Outdated Show resolved Hide resolved
isCurrentDeviceTrusted,
defaultKeyId,
});
showSetupEncryptionToast(SetupKind.SET_UP_ENCRYPTION);
Copy link
Member

Choose a reason for hiding this comment

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

Will users end up in an unsolvable loop if we do this? Any guesses as to what state they are in?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't say for sure that they won't, some users do report loops at the moment which would imply the process of setting up encryption doesn't completely start from scratch. At least we will have the state of the variables we use here logged in a rageshake now.

dbkr and others added 2 commits January 15, 2025 16:23
Co-authored-by: Andy Balaam <[email protected]>
Co-authored-by: Andy Balaam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants