-
Notifications
You must be signed in to change notification settings - Fork 109
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
Secure backup UI #1662
Secure backup UI #1662
Conversation
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1662 +/- ##
===========================================
+ Coverage 63.04% 63.50% +0.46%
===========================================
Files 1223 1260 +37
Lines 31465 32508 +1043
Branches 6480 6738 +258
===========================================
+ Hits 19837 20645 +808
- Misses 8639 8744 +105
- Partials 2989 3119 +130
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have a few comments, but they don't need to be done in this PR and some are more questions than issues found.
Also, it's super weird that when logging out, the backup upload state is always 'waiting' and displays the warning message.
var isLastSession by remember { mutableStateOf(false) } | ||
LaunchedEffect(Unit) { | ||
isLastSession = encryptionService.isLastDevice().getOrNull() ?: false | ||
encryptionService.waitForBackupUploadSteadyState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not part of this PR, but the API is kind of weird, couldn't we just return the encryptionService.backupUploadStateStateFlow
in a single call, instead of collecting the value somewhere and starting the subscription somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can try to improve this.
@@ -195,6 +197,13 @@ private fun RoomListContent( | |||
onDismissClicked = { state.eventSink(RoomListEvents.DismissRequestVerificationPrompt) } | |||
) | |||
} | |||
} else if (state.displayRecoveryKeyPrompt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use when
? WDYT?
val recoveryKeyVisualTransformation = remember { | ||
RecoveryKeyVisualTransformation() | ||
} | ||
OutlinedTextField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: neither hardware nor software Enter key triggers the 'continue' action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can add this
onDone() | ||
} | ||
} | ||
HeaderFooterPage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no content, I guess there are no designs? I didn't see any in Figma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this: https://www.figma.com/file/0MMNu7cTOzLOlWb7ctTkv3/Element-X?node-id=12998%3A37632&mode=dev but maybe out of date...
BottomMenu( | ||
state = state, | ||
onSaveClicked = { key -> | ||
context.startSharePlainTextIntent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a bit weird to use a share intent? Does it allow you to download it to a file or just copy the key? I couldn't test this myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will propose to share the key as a String.
Remaining part of #1648 (see description there)
Screenshot are getting recorded.