Skip to content

Add key storage toggle to Encryption settings #29310

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

Merged
merged 92 commits into from
Mar 14, 2025
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Feb 18, 2025

#29113 minus a couple of things that got split out

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)

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should use CryptoEvent.KeyBackupStatus. That said, maybe this is good enough and we should just correct the comments, and move on.

Comment on lines 195 to 196
// of megolm key backup has changed on the user's account (there's no event emitted for megolm
// key backup enabled state changing, so we use this instead).
Copy link
Member

Choose a reason for hiding this comment

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

there's no event emitted for megolm key backup enabled state changing

I don't think that's actually true: isn't CryptoEvent.KeyBackupStatus what you need?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, not sure why I didn't spot that before. Changed it to use that and all seems to be working nicely.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise!

Comment on lines 189 to 192
// Note that this could potentially update the UI while the user is trying to do something, although
// if their account data is changing then it implies that they're changing encryption related things
// on another device. This code is written with the assumption that it's better for the UI to refresh
// and be up to date with whatever changes they've made.
Copy link
Member

Choose a reason for hiding this comment

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

this still talks about account data, which I think is incorrect

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, good catch.

Which has gained nowrap due to 917d53a
@dbkr dbkr added this pull request to the merge queue Mar 14, 2025
Merged via the queue into develop with commit be3778b Mar 14, 2025
32 checks passed
@dbkr dbkr deleted the dbkr/key_storage_toggle_2 branch March 14, 2025 09:06
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.

3 participants