cabal-club / cabal-desktop

Desktop client for Cabal, the p2p/decentralized/local-first chat platform.
https://cabal.chat
GNU Affero General Public License v3.0
843 stars 77 forks source link

Fix duplicate messages #314

Closed nikolaiwarner closed 3 years ago

nikolaiwarner commented 3 years ago

It seems that Desktop was getting an event from cabal-client about mentions being included or not in the message but didnt handle that correctly and instead showed a full extra message. This PR filters out "mention" messages to prevent showing duplicate messages.

nikolaiwarner commented 3 years ago

fixes #277

cblgh commented 3 years ago

edit: see updated reply below

hm i dunno how cabal-client is sending an additional message just for the mentions? any chance you could send a screenshot of what you were logging to find this out? might be that this patch does fix it and i just can't figure out where things are going wrong :3

these lines are the only place where we emit on a new message / handle mentions afaicr. my fear with this patch is that messages which are direct mentions will get dropped, e.g. message 1 in the exchange below would get filtered out:

  1. [cblgh] nikolaiwarner: hey what's up?
  2. [nikolaiwarner] oh you know, just being rad
  3. [cblgh] cool

-> you would never see me asking what's up :<


patch moved to reply below

cblgh commented 3 years ago

i think i might have found the root cause? seems like there might be (at least) two ways to trigger this bug: one stemming from cabal-client, and one stemming from cabal-desktop.

cabal-client: looks like there might be some conflict where a message listener can be added to the same channel more than once, see the cabal-client patch

cabal-desktop: i think it might be better to completely stub out the publish-message event, as it seems that (in combination with the new-message event, which fires after you have locally posted a message) it might cause a duplicate to be added in certain cases—typing many messages rapidly after each other could trigger it.

so doing:

// add to action.js, https://github.com/cabal-club/cabal-desktop/blob/master/app/actions.js#L691-L696
name: 'publish-message',
action: () => {
  // don't do anything on publish message (new-message takes care of it)
}

finally: this snippet is a decent safeguard against ui bugs in the future. i tried it out locally (without having the two patches from above applied, so that i could repro the duplicated messages) and it works :)

// add to messages.js, https://github.com/cabal-club/cabal-desktop/blob/master/app/containers/messages.js#L40
  const seen = {}
  const messages = (props.messages ?? []).filter((message) => {
    const mid = message.key + message.message.seq
    if (typeof seen[mid] === "undefined") {
      seen[mid] = true
      return true
    }
    return false
  })
nikolaiwarner commented 3 years ago

thanks @cblgh! i've updated the PR with your suggestions!

nikolaiwarner commented 3 years ago

nice!