share logic whether have reacted already between basemsgtile & reactvm

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

@@ -72,7 +72,7 @@ export class ReactionsViewModel {
         return this._reactions;
     }
 
-    getReactionViewModelForKey(key) {
+    getReaction(key) {
         return this._map.get(key);
     }
 }
@@ -127,10 +127,32 @@ class ReactionViewModel {
         return this._pendingCount !== null;
     }
 
-    get haveReacted() {
+    /** @returns {boolean} true if the user has a (pending) reaction
+     *    already for this key, or they have a pending redaction for
+     *    the reaction, false if there is nothing pending and
+     *    the user has not reacted yet. */
+    get isActive() {
         return this._annotation?.me || this.isPending;
     }
 
+    /** @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
+        // reaction or not. This depends on the order of the pending events ofcourse,
+        // 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;
+        const haveRemoteReaction = this._annotation?.me;
+        const haveLocalRedaction = isPending && this._pendingCount <= 0;
+        const haveLocalReaction = isPending && this._pendingCount >= 0;
+        const haveReaction = (haveRemoteReaction && !haveLocalRedaction) ||
+            // if remote, then assume redaction comes first and reaction last, so final state is reacted
+            (haveRemoteReaction && haveLocalRedaction && haveLocalReaction) ||
+            (!haveRemoteReaction && !haveLocalRedaction && haveLocalReaction);
+        return haveReaction;
+    }
+
     _compare(other) {
         // the comparator is also used to test for equality by sortValues, if the comparison returns 0
         // given that the firstTimestamp isn't set anymore when the last reaction is removed,
@@ -166,23 +188,10 @@ class ReactionViewModel {
             }
             this._isToggling = true;
             try {
-                // determine whether if everything pending is sent, if we have a
+                if (this.haveReacted) {
-                // reaction or not. This depends on the order of the pending events ofcourse,
+                    await log.wrap("redactReaction", log => this._parentTile._redactReaction(this.key, log));
-                // 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;
-                const haveRemoteReaction = this._annotation?.me;
-                const haveLocalRedaction = isPending && this._pendingCount <= 0;
-                const haveLocalReaction = isPending && this._pendingCount >= 0;
-                const haveReaction = (haveRemoteReaction && !haveLocalRedaction) ||
-                    // if remote, then assume redaction comes first and reaction last, so final state is reacted
-                    (haveRemoteReaction && haveLocalRedaction && haveLocalReaction) ||
-                    (!haveRemoteReaction && !haveLocalRedaction && haveLocalReaction);
-                log.set({status: haveReaction ? "redact" : "react", haveLocalRedaction, haveLocalReaction, haveRemoteReaction, haveReaction, remoteCount: this._annotation?.count, pendingCount: this._pendingCount});
-                if (haveReaction) {
-                    await this._parentTile.redactReaction(this.key, log);
                 } else {
-                    await this._parentTile.react(this.key, log);
+                    await log.wrap("react", log => this._parentTile._react(this.key, log));
                 }
             } finally {
                 this._isToggling = false;
@@ -247,8 +256,22 @@ export function tests() {
         return tiles;
     }
 
-    // these are more an integration test than unit tests, but fully tests the local echo when toggling
     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
         "toggling reaction with own remote reaction": async assert => {
             // 1. put message and reaction in storage
             const messageEvent = withTextBody("Dogs > Cats", createEvent("m.room.message", "!abc", bob));
@@ -279,7 +302,7 @@ export function tests() {
             queue.pendingEvents.subscribe(queueObserver);
             tiles.subscribe(new ListObserver());
             const messageTile = findInIterarable(tiles, e => !!e); // the other entries are mapped to null
-            const reactionVM = messageTile.reactions.getReactionViewModelForKey("🐶");
+            const reactionVM = messageTile.reactions.getReaction("🐶");
             // 5. test toggling
             // make sure the preexisting reaction is counted
             assert.equal(reactionVM.count, 1);
@@ -344,7 +367,7 @@ export function tests() {
             // 5.1. set reaction, should send a new reaction as there is none yet
             await messageTile.react("🐶");
             // now there should be a reactions view model
-            const reactionVM = messageTile.reactions.getReactionViewModelForKey("🐶");
+            const reactionVM = messageTile.reactions.getReaction("🐶");
             let reactionTxnId;
             {
                 assert.equal(reactionVM.count, 1);

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

@@ -124,7 +124,23 @@ export class BaseMessageTile extends SimpleTile {
     }
 
     react(key, log = null) {
-        return this.logger.wrapOrRun(log, "react", async log => {
+        return this.logger.wrapOrRun(log, "react", log => {
+            const keyVM = this.reactions?.getReaction(key);
+            if (keyVM?.haveReacted) {
+                log.set("already_reacted", true);
+                return;
+            }
+            return this._react(key, log);
+        });
+    }
+
+    async _react(key, log) {
+        // This will also block concurently adding multiple reactions,
+        // but in practice it happens fast enough.
+        if (this._pendingReactionChangeCallback) {
+            log.set("ongoing", true);
+            return;
+        }
         const redaction = this._entry.getAnnotationPendingRedaction(key);
         const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve);
         if (redaction && !redaction.pendingEvent.hasStartedSending) {
@@ -134,16 +150,34 @@ export class BaseMessageTile extends SimpleTile {
             await this._room.sendEvent("m.reaction", this._entry.annotate(key), null, log);
         }
         await updatePromise;
-        });
+        this._pendingReactionChangeCallback = null;
     }
 
     redactReaction(key, log = null) {
+        return this.logger.wrapOrRun(log, "redactReaction", log => {
+            const keyVM = this.reactions?.getReaction(key);
+            if (!keyVM?.haveReacted) {
+                log.set("not_yet_reacted", true);
+                return;
+            }
+            return this._redactReaction(key, log);
+        });
+    }
+
+    async _redactReaction(key, log) {
+        // This will also block concurently removing multiple reactions,
+        // but in practice it happens fast enough.
+        if (this._pendingReactionChangeCallback) {
+            log.set("ongoing", true);
+            return;
+        }
         return this.logger.wrapOrRun(log, "redactReaction", async log => {
             const entry = await this._entry.getOwnAnnotationEntry(this._timeline, key);
             if (entry) {
                 const updatePromise = new Promise(resolve => this._pendingReactionChangeCallback = resolve);
                 await this._room.sendRedaction(entry.id, null, log);
                 await updatePromise;
+                this._pendingReactionChangeCallback = null;
             } else {
                 log.set("no_reaction", true);
             }
@@ -155,6 +189,15 @@ export class BaseMessageTile extends SimpleTile {
         if (!annotations && !pendingAnnotations) {
             if (this._reactions) {
                 this._reactions = null;
+                // The update comes in async because pending events are mapped in the timeline
+                // to pending event entries using an AsyncMappedMap, because in rare cases, the target
+                // of a redaction needs to be loaded from storage in order to know for which message
+                // the reaction needs to be removed. The SendQueue also only adds pending events after
+                // storing them first.
+                // This makes that if we want to know the local echo for either react or redactReaction is available,
+                // we need to async wait for the update call. In theory the update can also be triggered
+                // by something else than the reaction local echo changing (e.g. from sync),
+                // but this is very unlikely and deemed good enough for now.
                 this._pendingReactionChangeCallback && this._pendingReactionChangeCallback();
             }
         } else {

src/platform/web/ui/session/room/timeline/ReactionsView.js

@@ -31,9 +31,11 @@ export class ReactionsView extends ListView {
 
 class ReactionView extends TemplateView {
     render(t, vm) {
-        const haveReacted = vm => vm.haveReacted;
         return t.button({
-            className: {haveReacted, isPending: vm => vm.isPending},
+            className: {
+                active: vm => vm.isActive,
+                isPending: vm => vm.isPending
+            },
         }, [vm.key, " ", vm => `${vm.count}`]);
     }