-
Notifications
You must be signed in to change notification settings - Fork 73
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
Ensure encrypted events are not handled twice #267
Conversation
src/components/encryption.ts
Outdated
|
||
// Delete the event from the pending list | ||
this.eventsPendingSync.delete(event.event_id); | ||
let index = this.eventsPendingAS.findIndex((e) => e.event_id === event.event_id); |
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's possible we might get multiple sync events stored.
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.
questions
src/components/encryption.ts
Outdated
@@ -86,16 +86,18 @@ export class EncryptedEventBroker { | |||
private onEphemeralEvent: ((event: EphemeralEvent) => void)|undefined, | |||
private getIntent: (userId: string) => Intent | |||
) { | |||
if (this.onEphemeralEvent) { | |||
this.presenceCleanupInterval = setInterval(() => { | |||
const ts = Date.now() - 30000; |
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.
30s wants to be a config var?
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 should make it a const but I don't think there is any value in it being a config value, it's just a cache lifetime.
src/components/encryption.ts
Outdated
this.eventsPendingSync.delete(event.event_id); | ||
let index = this.eventsPendingAS.findIndex((e) => e.event_id === event.event_id); | ||
do { | ||
this.eventsPendingAS.splice(index, 1); |
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.
Would all of this be possible with this.eventsPendingAS = this.eventsPendingAS.filter((e) => e.event_id !== event.event_id)
?
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.
Yeah, I prefer that.
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
Fixes #266
This should address some problems with the encrypted event tracking