Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Display a warning when an unverified user's identity changes #28211
base: develop
Are you sure you want to change the base?
Display a warning when an unverified user's identity changes #28211
Changes from 9 commits
6fa98f8
9f2c1a1
0d186dd
0b1e7ad
f4b403d
4e8a990
d2b244f
8078627
5615b98
0ef342b
ccbfcea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
aside: I wondered if it would be good to have a single
updateCurrentPrompt
function which updatescurrentPrompt
based on the current state of the list. Hence we'd have something like:... and then use that in the three places we currently call
setCurrentPrompt
.It feels cognitively simpler to me, and has the advantage that we don't need to worry about someone slipping in an
async
before a call tosetCurrentPrompt
and breaking everything.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.
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.
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.
Also, consider removing
!membersNeedingApproval.has(userId)
from this condition. AFAICT,addMemberNeedingApproval
is a no-op if the user is already in the list, and removing the check would simplify this function.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.
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.
Why do we
catch
on theloadMembers()
call at line 241, but not here? Any errors thrown here will get lost.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.
Suppose we see a member join and leave in quick succession (potentially, in the same /sync response)
The join will trigger a call to
addMemberIfNeedsApproval
, which will start a call touserNeedsApproval
.The leave will run
removeMemberNeedingApproval
, which won't do much.userNeedsApproval
will complete, and the user may be added to the list of approvals needed. We now have a non-member in the list of approvals.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.
Right. Similar thing if someone leaves while we're still processing the initial room membership. Will have to think about how to solve this. I think that setting the flag in
gotVerificationStatusUpdate
might fix the join-then-leave issue, but not the leave-while-initialising problem.