Skip to content
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

Fix errors in switching chatroom when user leaves group or chatroom #2186

Merged

Conversation

Silver-IT
Copy link
Member

@Silver-IT Silver-IT requested a review from taoeffect July 11, 2024 08:50
@Silver-IT Silver-IT self-assigned this Jul 11, 2024
@Silver-IT Silver-IT linked an issue Jul 11, 2024 that may be closed by this pull request
Copy link

cypress bot commented Jul 11, 2024

Passing run #2694 ↗︎

0 114 8 0 Flakiness 0

Details:

Merge 6dec75b into f73ce63...
Project: group-income Commit: f83c034910 ℹ️
Status: Passed Duration: 10:44 💡
Started: Jul 18, 2024 1:36 AM Ended: Jul 18, 2024 1:47 AM

Review all test suite changes for PR #2186 ↗︎

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment improvement and a question

frontend/model/contracts/group.js Outdated Show resolved Hide resolved
frontend/views/containers/chatroom/ChatMixin.js Outdated Show resolved Hide resolved
Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @Silver-IT!

I left a comment here for a minor change request.

I also found a serious bug:

I decided to test leaving a chatroom voluntarily and being kicked out of the chatroom to see if both worked.

  • To do this, I created a group with two users, u1 and u2.
  • I had each user create two chatrooms, so 4 chatrooms in total.
  • Then, each user added the other user to the chatrooms they created.
  • Next, for each of the chatrooms each user created, I kicked the other user out of one of the chatrooms, and had the other user leave voluntarily.
  • Finally, in separate session tabs, I had both users log in (so u1 is logged in to [Tab1] and [Tab3] and u2 is logged in to [Tab2] and [Tab4]

I started to record toward the end of this, and you can see the result:

Screenshot 2024-07-12 at 3 06 50 PM

When the users left the chartroom voluntarily, this resulted in an error when they logged back in on a new fresh session tab.

[chelonia/contract/release] Invalid negative reference count for z9brRu3VKg2ifBpQHXqLZyCa3aqw5Kv397h2XECS2com8ug8dQM4 [captureLogs.js:36:30](http://localhost:3000/frontend/model/captureLogs.js)
[leaveChatRoomAction] Error releasing chatroom z9brRu3VKg2ifBpQHXqLZyCa3aqw5Kv397h2XECS2com8ug8dQM4 Error: Invalid negative reference count
leave-room-bug.compressed.mov

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @Silver-IT! I tested it and the previous error is gone now!

One comment left.

Comment on lines 351 to 365
// NOTE: using 'state/vuex/state' instead of 'chelonia/rootState' to fetch
// the identityContractID because although both 'chelonia/rootState' and
// 'state/vuex/state' both have the same logged in information
// all calls to 'state/vuex/state' will need to be replaced in the future
// with something else. Using 'state/vuex/state' here now makes it easier
// to find these calls in the future.
// `chelonia/rootState` contains that same information but it's a bad
// choice because:
// 1. Once sandboxing is implemented, it may need to be an async call
// 2. Generally, contracts should _not_ access the root state because
// it makes it difficult or impossible to contain them (meaning there
// would be no point in sandboxing)
// Instead, in the future contracts will have an 'environment', provided
// by Chelonia, which will include global / environment / ambient
// information they need.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple problems here.

  1. The code for which this was a comment was moved. Now this is a .then handler that does nothing and the comment here is not for anything.
  2. The code below with .catch is also nonsensical.

So to fix this, please move this comment to where you moved sbp('state/vuex/state').loggedIn.identityContractID, because it must stay with it. In this case, above line 1265.

Then remove the .then that does nothing, and replace the two .catch with one:

  }).catch((e) => {
    if (
      leavingGroup &&
      (e?.name === 'ChelErrorSignatureKeyNotFound' || (
        e?.name === 'GIErrorUIRuntimeError' &&
        (
          ['ChelErrorSignatureKeyNotFound', 'GIErrorMissingSigningKeyError'].includes(e?.cause?.name) ||
          e?.cause?.name === 'GIChatroomNotMemberError'
        )
      ))
    ) {
      // This is fine; it just means we were removed by someone else
      return
    }
    console.warn('[gi.contracts/group] Error sending chatroom leave action', e)
  })

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions...

frontend/model/contracts/chatroom.js Outdated Show resolved Hide resolved
frontend/model/contracts/chatroom.js Outdated Show resolved Hide resolved
Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @Silver-IT ! Approved!

@taoeffect taoeffect merged commit 2beeecc into master Jul 18, 2024
4 checks passed
@taoeffect taoeffect deleted the 2185-switching-chatroom-doesnt-work-when-the-user-leaves-it branch July 18, 2024 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching chatroom doesn't work when the user leaves it
2 participants