use pending re(d)action timestamp to have stable reaction sorting order

also move more logic into the matrix layer, from Reaction(s)ViewModel
to PendingAnnotation

src/domain/session/room/timeline/ReactionsViewModel.js

@@ -40,14 +40,13 @@ export class ReactionsViewModel {
             }
         }
         if (pendingAnnotations) {
-            for (const [key, count] of pendingAnnotations.entries()) {
+            for (const [key, annotation] of pendingAnnotations.entries()) {
                 const reaction = this._map.get(key);
                 if (reaction) {
-                    if (reaction._tryUpdatePending(count)) {
+                    reaction._tryUpdatePending(annotation);
                     this._map.update(key);
-                    }
                 } else {
-                    this._map.add(key, new ReactionViewModel(key, null, count, this._parentTile));
+                    this._map.add(key, new ReactionViewModel(key, null, annotation, this._parentTile));
                 }
             }
         }
@@ -78,10 +77,10 @@ export class ReactionsViewModel {
 }
 
 class ReactionViewModel {
-    constructor(key, annotation, pendingCount, parentTile) {
+    constructor(key, annotation, pending, parentTile) {
         this._key = key;
         this._annotation = annotation;
-        this._pendingCount = pendingCount;
+        this._pending = pending;
         this._parentTile = parentTile;
         this._isToggling = false;
     }
@@ -101,30 +100,24 @@ class ReactionViewModel {
         return false;
     }
 
-    _tryUpdatePending(pendingCount) {
+    _tryUpdatePending(pending) {
-        if (pendingCount !== this._pendingCount) {
+        if (!pending && !this._pending) {
-            this._pendingCount = pendingCount;
-            return true;
-        }
             return false;
         }
+        this._pending = pending;
+        return true;
+    }
 
     get key() {
         return this._key;
     }
 
     get count() {
-        let count = this._pendingCount || 0;
+        return (this._pending?.count || 0) + (this._annotation?.count || 0);
-        if (this._annotation) {
-            count += this._annotation.count;
-        }
-        return count;
     }
 
     get isPending() {
-        // even if pendingCount is 0,
+        return this._pending !== null;
-        // it means we have both a pending reaction and redaction
-        return this._pendingCount !== null;
     }
 
     /** @returns {boolean} true if the user has a (pending) reaction
@@ -138,19 +131,19 @@ class ReactionViewModel {
     /** @returns {boolean} Whether the user has reacted with this key,
      * taking the local reaction and reaction redaction into account. */
     get haveReacted() {
-        // determine whether if everything pending is sent, if we have a
+        // TODO: cleanup
-        // reaction or not. This depends on the order of the pending events ofcourse,
+        return this._parentTile._entry.haveAnnotation(this.key);
-        // which we don't have access to here, but we assume that a redaction comes first
+    }
-        // if we have a remote reaction
+
-        const {isPending} = this;
+    get firstTimestamp() {
-        const haveRemoteReaction = this._annotation?.me;
+        let ts = Number.MAX_SAFE_INTEGER;
-        const haveLocalRedaction = isPending && this._pendingCount <= 0;
+        if (this._annotation) {
-        const haveLocalReaction = isPending && this._pendingCount >= 0;
+            ts = Math.min(ts, this._annotation.firstTimestamp);
-        const haveReaction = (haveRemoteReaction && !haveLocalRedaction) ||
+        }
-            // if remote, then assume redaction comes first and reaction last, so final state is reacted
+        if (this._pending) {
-            (haveRemoteReaction && haveLocalRedaction && haveLocalReaction) ||
+            ts = Math.min(ts, this._pending.firstTimestamp);
-            (!haveRemoteReaction && !haveLocalRedaction && haveLocalReaction);
+        }
-        return haveReaction;
+        return ts;
     }
 
     _compare(other) {
@@ -163,40 +156,16 @@ class ReactionViewModel {
         if (this.count !== other.count) {
             return other.count - this.count;
         } else {
-            const a = this._annotation;
+            const cmp = this.firstTimestamp - other.firstTimestamp;
-            const b = other._annotation;
-            if (a && b) {
-                const cmp = a.firstTimestamp - b.firstTimestamp;
             if (cmp === 0) {
                 return this.key < other.key ? -1 : 1;
-                } else {
+            }
             return cmp;
         }
-            } else if (a) {
-                return -1;
-            } else {
-                return 1;
-            }
-        }
     }
 
     toggleReaction(log = null) {
-        return this._parentTile.logger.wrapOrRun(log, "toggleReaction", async log => {
+        return this._parentTile.toggleReaction(this.key, log);
-            if (this._isToggling) {
-                log.set("busy", true);
-                return;
-            }
-            this._isToggling = true;
-            try {
-                if (this.haveReacted) {
-                    await log.wrap("redactReaction", log => this._parentTile._redactReaction(this.key, log));
-                } else {
-                    await log.wrap("react", log => this._parentTile._react(this.key, log));
-                }
-            } finally {
-                this._isToggling = false;
-            }
-        });
     }
 }
 
@@ -257,18 +226,6 @@ export function tests() {
     }
 
     return {
-        "haveReacted": assert => {
-            assert.equal(false, new ReactionViewModel("🚀", null, null).haveReacted);
-            assert.equal(false, new ReactionViewModel("🚀", {me: false, count: 1}, null).haveReacted);
-            assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 1}, null).haveReacted);
-            assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 2}, null).haveReacted);
-            assert.equal(true, new ReactionViewModel("🚀", null, 1).haveReacted);
-            assert.equal(false, new ReactionViewModel("🚀", {me: true, count: 1}, -1).haveReacted);
-            // pending count 0 means the remote reaction has been redacted and is sending, then a new reaction was queued
-            assert.equal(true, new ReactionViewModel("🚀", {me: true, count: 1}, 0).haveReacted);
-            // should typically not happen without a remote reaction already present, but should still be false
-            assert.equal(false, new ReactionViewModel("🚀", null, 0).haveReacted);
-        },
         // these are more an integration test than unit tests,
         // but fully test the local echo when toggling and
         // the correct send queue modifications happen

src/domain/session/room/timeline/tiles/BaseMessageTile.js

@@ -125,8 +125,7 @@ export class BaseMessageTile extends SimpleTile {
 
     react(key, log = null) {
         return this.logger.wrapOrRun(log, "react", log => {
-            const keyVM = this.reactions?.getReaction(key);
+            if (this._entry.haveAnnotation(key)) {
-            if (keyVM?.haveReacted) {
                 log.set("already_reacted", true);
                 return;
             }
@@ -141,7 +140,7 @@ export class BaseMessageTile extends SimpleTile {
             log.set("ongoing", true);
             return;
         }
-        const redaction = this._entry.getAnnotationPendingRedaction(key);
+        const redaction = this._entry.pendingAnnotations?.get(key)?.redactionEntry;
         try {
             const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve);
             if (redaction && !redaction.pendingEvent.hasStartedSending) {
@@ -159,7 +158,7 @@ export class BaseMessageTile extends SimpleTile {
     redactReaction(key, log = null) {
         return this.logger.wrapOrRun(log, "redactReaction", log => {
             const keyVM = this.reactions?.getReaction(key);
-            if (!keyVM?.haveReacted) {
+            if (!this._entry.haveAnnotation(key)) {
                 log.set("not_yet_reacted", true);
                 return;
             }
@@ -170,11 +169,16 @@ export class BaseMessageTile extends SimpleTile {
     async _redactReaction(key, log) {
         // This will also block concurently removing multiple reactions,
         // but in practice it happens fast enough.
+
+        // TODO: remove this as we'll protect against reentry in the SendQueue
         if (this._pendingReactionChangeCallback) {
             log.set("ongoing", true);
             return;
         }
-        const entry = await this._entry.getOwnAnnotationEntry(this._timeline, key);
+        let entry = this._entry.pendingAnnotations?.get(key)?.annotationEntry;
+        if (!entry) {
+            entry = await this._timeline.getOwnAnnotationEntry(this._entry.id, key);
+        }
         if (entry) {
             try {
                 const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve);
@@ -188,6 +192,16 @@ export class BaseMessageTile extends SimpleTile {
         }
     }
 
+    toggleReaction(key, log = null) {
+        return this.logger.wrapOrRun(log, "toggleReaction", async log => {
+            if (this._entry.haveAnnotation(key)) {
+                await log.wrap("redactReaction", log => this._redactReaction(key, log));
+            } else {
+                await log.wrap("react", log => this._react(key, log));
+            }
+        });
+    }
+
     _updateReactions() {
         const {annotations, pendingAnnotations} = this._entry;
         if (!annotations && !pendingAnnotations) {

src/matrix/room/timeline/PendingAnnotation.js

@@ -0,0 +1,76 @@
+/*
+Copyright 2021 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+export class PendingAnnotation {
+    constructor() {
+        // TODO: use simple member for reaction and redaction as we can't/shouldn't really have more than 2 entries
+        // this contains both pending annotation entries, and pending redactions of remote annotation entries 
+        this._entries = [];
+    }
+
+    get firstTimestamp() {
+        return this._entries.reduce((ts, e) => {
+            if (e.isRedaction) {
+                return ts;
+            }
+            return Math.min(e.timestamp, ts);
+        }, Number.MAX_SAFE_INTEGER);
+    }
+
+    get annotationEntry() {
+        return this._entries.find(e => !e.isRedaction);
+    }
+
+    get redactionEntry() {
+        return this._entries.find(e => e.isRedaction);
+    }
+
+    get count() {
+        return this._entries.reduce((count, e) => {
+            return count + (e.isRedaction ? -1 : 1);
+        }, 0);
+    }
+
+    add(entry) {
+        this._entries.push(entry);
+    }
+
+    remove(entry) {
+        const idx = this._entries.indexOf(entry);
+        if (idx === -1) {
+            return false;
+        }
+        this._entries.splice(idx, 1);
+        return true;
+    }
+
+    get willAnnotate() {
+        const lastEntry = this._entries.reduce((lastEntry, e) => {
+            if (!lastEntry || e.pendingEvent.queueIndex > lastEntry.pendingEvent.queueIndex) {
+                return e;
+            }
+            return lastEntry;
+        }, null);
+        if (lastEntry) {
+            return !lastEntry.isRedaction;
+        }
+        return false;
+    }
+
+    get isEmpty() {
+        return this._entries.length === 0;
+    }
+}

src/matrix/room/timeline/PendingAnnotations.js

@@ -1,74 +0,0 @@
-/*
-Copyright 2021 The Matrix.org Foundation C.I.C.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-export class PendingAnnotations {
-    constructor() {
-        this.aggregatedAnnotations = new Map();
-        // this contains both pending annotation entries, and pending redactions of remote annotation entries 
-        this._entries = [];
-    }
-
-    /** adds either a pending annotation entry, or a remote annotation entry with a pending redaction */
-    add(entry) {
-        const {key} = (entry.redactingEntry || entry).relation;
-        if (!key) {
-            return;
-        }
-        const count = this.aggregatedAnnotations.get(key) || 0;
-        const addend = entry.isRedaction ? -1 : 1;
-        this.aggregatedAnnotations.set(key, count + addend);
-        this._entries.push(entry);
-    }
-
-    /** removes either a pending annotation entry, or a remote annotation entry with a pending redaction */
-    remove(entry) {
-        const idx = this._entries.indexOf(entry);
-        if (idx === -1) {
-            return;
-        }
-        this._entries.splice(idx, 1);
-        const {key} = (entry.redactingEntry || entry).relation;
-        let count = this.aggregatedAnnotations.get(key);
-        if (count !== undefined) {
-            const addend = entry.isRedaction ? 1 : -1;
-            count += addend;
-            this.aggregatedAnnotations.set(key, count);
-        }
-        if (!this._entries.length) {
-            this.aggregatedAnnotations.clear();
-        }
-    }
-
-    findForKey(key) {
-        return this._entries.find(e => {
-            if (e.relation?.key === key) {
-                return e;
-            }
-        });
-    }
-
-    findRedactionForKey(key) {
-        return this._entries.find(e => {
-            if (e.redactingEntry?.relation?.key === key) {
-                return e;
-            }
-        });
-    }
-
-    get isEmpty() {
-        return this._entries.length === 0;
-    }
-}

src/matrix/room/timeline/Timeline.js

@@ -416,7 +416,7 @@ export function tests() {
                 relatedEventId: entry.id
             }}));
             await poll(() => timeline.entries.length === 2);
-            assert.equal(entry.pendingAnnotations.get("👋"), 1);
+            assert.equal(entry.pendingAnnotations.get("👋").count, 1);
             const reactionEntry = getIndexFromIterable(timeline.entries, 1);
             // 3. add redaction to timeline
             pendingEvents.append(new PendingEvent({data: {
@@ -429,11 +429,11 @@ export function tests() {
             }}));
             // TODO: await nextUpdate here with ListObserver, to ensure entry emits an update when pendingAnnotations changes
             await poll(() => timeline.entries.length === 3);
-            assert.equal(entry.pendingAnnotations.get("👋"), 0);
+            assert.equal(entry.pendingAnnotations.get("👋").count, 0);
             // 4. cancel redaction
             pendingEvents.remove(1);
             await poll(() => timeline.entries.length === 2);
-            assert.equal(entry.pendingAnnotations.get("👋"), 1);
+            assert.equal(entry.pendingAnnotations.get("👋").count, 1);
             // 5. cancel reaction
             pendingEvents.remove(0);
             await poll(() => timeline.entries.length === 1);
@@ -507,7 +507,7 @@ export function tests() {
                 relatedEventId: reactionEntry.id
             }}));
             await poll(() => timeline.entries.length >= 3);
-            assert.equal(messageEntry.pendingAnnotations.get("👋"), -1);
+            assert.equal(messageEntry.pendingAnnotations.get("👋").count, -1);
         },
         "local reaction gets applied after remote echo is added to timeline": async assert => {
             const messageEntry = new EventEntry({event: withTextBody("hi bob!", withSender(alice, createEvent("m.room.message", "!abc"))),
@@ -533,7 +533,7 @@ export function tests() {
             await poll(() => timeline.entries.length === 2);
             const entry = getIndexFromIterable(timeline.entries, 0);
             assert.equal(entry, messageEntry);
-            assert.equal(entry.pendingAnnotations.get("👋"), 1);
+            assert.equal(entry.pendingAnnotations.get("👋").count, 1);
         },
         "local reaction removal gets applied after remote echo is added to timeline with reaction not loaded": async assert => {
             const messageId = "!abc";
@@ -570,7 +570,7 @@ export function tests() {
             await poll(() => timeline.entries.length === 2);
             // 5. check that redaction was linked to reaction target
             const entry = getIndexFromIterable(timeline.entries, 0);
-            assert.equal(entry.pendingAnnotations.get("👋"), -1);
+            assert.equal(entry.pendingAnnotations.get("👋").count, -1);
         },
         "decrypted entry preserves content when receiving other update without decryption": async assert => {
             // 1. create encrypted and decrypted entry

src/matrix/room/timeline/entries/BaseEventEntry.js

@@ -17,7 +17,7 @@ limitations under the License.
 import {BaseEntry} from "./BaseEntry.js";
 import {REDACTION_TYPE} from "../../common.js";
 import {createAnnotation, ANNOTATION_RELATION_TYPE, getRelationFromContent} from "../relations.js";
-import {PendingAnnotations} from "../PendingAnnotations.js";
+import {PendingAnnotation} from "../PendingAnnotation.js";
 
 /** Deals mainly with local echo for relations and redactions,
  * so it is shared between PendingEventEntry and EventEntry */
@@ -65,14 +65,22 @@ export class BaseEventEntry extends BaseEntry {
             if (relationEntry.isRelatedToId(this.id)) {
                 if (relationEntry.relation.rel_type === ANNOTATION_RELATION_TYPE) {
                     if (!this._pendingAnnotations) {
-                        this._pendingAnnotations = new PendingAnnotations();
+                        this._pendingAnnotations = new Map();
                     }
-                    this._pendingAnnotations.add(entry);
+                    const {key} = (entry.redactingEntry || entry).relation;
+                    if (key) {
+                        let annotation = this._pendingAnnotations.get(key);
+                        if (!annotation) {
+                            annotation = new PendingAnnotation();
+                            this._pendingAnnotations.set(key, annotation);
+                        }
+                        annotation.add(entry);
                         return "pendingAnnotations";
                     }
                 }
             }
         }
+    }
     
     /**
         deaggregates local relation or a local redaction of a remote relation.
@@ -92,8 +100,13 @@ export class BaseEventEntry extends BaseEntry {
             const relationEntry = entry.redactingEntry || entry;
             if (relationEntry.isRelatedToId(this.id)) {
                 if (relationEntry.relation?.rel_type === ANNOTATION_RELATION_TYPE && this._pendingAnnotations) {
-                    this._pendingAnnotations.remove(entry);
+                    const {key} = (entry.redactingEntry || entry).relation;
-                    if (this._pendingAnnotations.isEmpty) {
+                    if (key) {
+                        let annotation = this._pendingAnnotations.get(key);
+                        if (annotation.remove(entry) && annotation.isEmpty) {
+                            this._pendingAnnotations.delete(key);
+                        }
+                        if (this._pendingAnnotations.size === 0) {
                             this._pendingAnnotations = null;
                         }
                         return "pendingAnnotations";
@@ -101,6 +114,7 @@ export class BaseEventEntry extends BaseEntry {
                 }
             }
         }
+    }
 
     async abortPendingRedaction() {
         if (this._pendingRedactions) {
@@ -128,19 +142,29 @@ export class BaseEventEntry extends BaseEntry {
         return id && this.relatedEventId === id;
     }
 
+    haveAnnotation(key) {
+        const haveRemoteReaction = this.annotations?.[key]?.me || false;
+        const pendingAnnotation = this.pendingAnnotations?.get(key);
+        const willAnnotate = pendingAnnotation?.willAnnotate || false;
+        /*
+        We have an annotation in these case:
+        - remote annotation with me, no pending
+        - remote annotation with me, pending redaction and then annotation
+        - pending annotation without redaction after it
+        */
+        return (haveRemoteReaction && (!pendingAnnotation || willAnnotate)) ||
+            (!haveRemoteReaction && willAnnotate);
+    }
+
     get relation() {
         return getRelationFromContent(this.content);
     }
 
     get pendingAnnotations() {
-        return this._pendingAnnotations?.aggregatedAnnotations;
+        return this._pendingAnnotations;
     }
 
-    async getOwnAnnotationEntry(timeline, key) {
+    get annotations() {
-        return this._pendingAnnotations?.findForKey(key);
+        return null; //overwritten in EventEntry
-    }
-
-    getAnnotationPendingRedaction(key) {
-        return this._pendingAnnotations?.findRedactionForKey(key);
     }
 }

src/matrix/room/timeline/entries/EventEntry.js

@@ -139,13 +139,89 @@ export class EventEntry extends BaseEventEntry {
     get annotations() {
         return this._eventEntry.annotations;
     }
+}
 
-    async getOwnAnnotationEntry(timeline, key) {
+import {withTextBody, withContent, createEvent} from "../../../../mocks/event.js";
-        const localId = await super.getOwnAnnotationEntry(timeline, key);
+import {Clock as MockClock} from "../../../../mocks/Clock.js";
-        if (localId) {
+import {PendingEventEntry} from "./PendingEventEntry.js";
-            return localId;
+import {PendingEvent} from "../../sending/PendingEvent.js";
-        } else {
+import {createAnnotation} from "../relations.js";
-            return timeline.getOwnAnnotationEntry(this.id, key);
+
+export function tests() {
+    let queueIndex = 0;
+    const clock = new MockClock();
+
+    function addPendingReaction(target, key) {
+        queueIndex += 1;
+        target.addLocalRelation(new PendingEventEntry({
+            pendingEvent: new PendingEvent({data: {
+                eventType: "m.reaction",
+                content: createAnnotation(target.id, key),
+                queueIndex,
+                txnId: `t${queueIndex}`
+            }}),
+            clock
+        }));
+        return target;
+    }
+
+    function addPendingRedaction(target, key) {
+        const pendingReaction = target.pendingAnnotations?.get(key)?.annotationEntry;
+        let redactingEntry = pendingReaction;
+        // make up a remote entry if we don't have a pending reaction and have an aggregated remote entry
+        if (!pendingReaction && target.annotations[key].me) {
+            redactingEntry = new EventEntry({
+                event: withContent(createAnnotation(target.id, key), createEvent("m.reaction", "!def"))
+            });
+        }
+        queueIndex += 1;
+        target.addLocalRelation(new PendingEventEntry({
+            pendingEvent: new PendingEvent({data: {
+                eventType: "m.room.redaction",
+                relatedTxnId: pendingReaction ? pendingReaction.id : null,
+                relatedEventId: pendingReaction ? null : redactingEntry.id,
+                queueIndex,
+                txnId: `t${queueIndex}`
+            }}),
+            redactingEntry,
+            clock
+        }));
+        return target;
+    }
+
+    function remoteAnnotation(key, me, count, obj = {}) {
+        obj[key] = {me, count};
+        return obj;
+    }
+
+    return {
+        // testing it here because parent class always assumes annotations is null
+        "haveAnnotation": assert => {
+            const msgEvent = withTextBody("hi!", createEvent("m.room.message", "!abc"));
+            const e1 = new EventEntry({event: msgEvent});
+            assert.equal(false, e1.haveAnnotation("🚀"));
+            const e2 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", false, 1)});
+            assert.equal(false, e2.haveAnnotation("🚀"));
+            const e3 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)});
+            assert.equal(true, e3.haveAnnotation("🚀"));
+            const e4 = new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 2)});
+            assert.equal(true, e4.haveAnnotation("🚀"));
+            const e5 = addPendingReaction(new EventEntry({event: msgEvent}), "🚀");
+            assert.equal(true, e5.haveAnnotation("🚀"));
+            const e6 = addPendingRedaction(new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)}), "🚀");
+            assert.equal(false, e6.haveAnnotation("🚀"));
+            const e7 = addPendingReaction(
+                addPendingRedaction(
+                    new EventEntry({event: msgEvent, annotations: remoteAnnotation("🚀", true, 1)}),
+                "🚀"),
+            "🚀");
+            assert.equal(true, e7.haveAnnotation("🚀"));
+            const e8 = addPendingRedaction(
+                addPendingReaction(
+                    new EventEntry({event: msgEvent}),
+                "🚀"),
+            "🚀");
+            assert.equal(false, e8.haveAnnotation("🚀"));
         }
     }
 }

src/matrix/room/timeline/entries/PendingEventEntry.js

@@ -23,7 +23,10 @@ export class PendingEventEntry extends BaseEventEntry {
         this._pendingEvent = pendingEvent;
         /** @type {RoomMember} */
         this._member = member;
-        this._clock = clock;
+        // try to come up with a timestamp that is around construction time and
+        // will be roughly sorted by queueIndex, so it can be used to as a secondary
+        // sorting dimension for reactions
+        this._timestamp = clock.now() - (100 - pendingEvent.queueIndex);
         this._redactingEntry = redactingEntry;
     }
 
@@ -64,7 +67,7 @@ export class PendingEventEntry extends BaseEventEntry {
     }
 
     get timestamp() {
-        return this._clock.now();
+        return this._timestamp;
     }
 
     get isPending() {