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(conversation): adjust broadcasted data between tabs #12557

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

Antreesy
Copy link
Contributor

☑️ Resolves

Fix #12548

  • pass options: { all } to leader tab, when force refresh;
  • do not fetch anything on secondary tabs, pass message to force refresh via broadcast channel;
  • do not fetch anything on leaving conversation;
  • broadcast federation invites count to secondary tabs;

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏡 After

Screencast.from.21.06.2024.15.58.27.webm

🏁 Checklist

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Solution looks good

src/components/LeftSidebar/LeftSidebar.vue Show resolved Hide resolved
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

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

Tested

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

A room signaling message with roomid: "" is sent by the signaling server when the current participant leaves a room (as a response to the message sent by the client, note that should-refresh-conversations is not directly triggered by _doLeaveRoom as seems to be implied from the second commit message, it is indirectly triggered). However, that message can be sent by the signaling server too to the active participants of a room if that room is deleted.

If the participant is a registered user the client will also receive a roomlist signaling message with type: "disinvite", which will cause a redirection to spreed/not-found. However, if the particiant is a guest no other signaling message will be received to notify the client about the current room being deleted.

Due to that, if should-refresh-conversations is not emitted when a room signaling message with roomid: "" is sent by the signaling server the UI is not immediately updated for guests when public conversations are deleted (although it will be updated at most after 30 seconds, but that smells like legacy code that might be cleaned at some point).

Similarly, if a video verification conversation is deleted by the moderator without even joining the conversation the UI for the guest will never reflect that the conversation is deleted. If the moderator joins the conversation and then leaves (which automatically deletes the conversation) it will, but only because when the moderator leaves a roomlist message with type: "update" will be sent, which will also cause the conversation to be fetched again (and as the conversation is automatically deleted it will no longer exist once the request reaches the server, so the UI will be properly updated on the response).

Would it be possible to emit should-refresh-conversations even when roomid: "" but ignore the event in those places in which an update is not needed in that case? Or maybe emit a different event when roomid: "" that could be explicitly listened to if needed?

@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 1, 2024

Thanks for the review, @danxuliu ! It is indeed more complex problem than it was expected by me 👀

We have 4 emiiters of emit('should-refresh-conversations' event in total (all in signaling.js), so I assume we can provide each emit with a meaningful payload to distinct it on frontend.
My suggestion is to handle it separately. I'll drop the blocking commit for now and open an issue

Antreesy added 2 commits July 1, 2024 18:01
…of conversations

- pass options: all to leader tab
- do not fetch anything on secondary tabs

Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the fix/12548/tabs-broadcast branch from e186594 to d5d5138 Compare July 1, 2024 16:02
@Antreesy Antreesy requested a review from danxuliu July 1, 2024 16:07
@Antreesy Antreesy dismissed danxuliu’s stale review July 3, 2024 14:19

Aligned with Daniel, as blocking commit is removed

@Antreesy Antreesy merged commit 52cf408 into main Jul 3, 2024
46 checks passed
@Antreesy Antreesy deleted the fix/12548/tabs-broadcast branch July 3, 2024 14:52
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 3, 2024

/backport to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversation list not updated when not joined a room
3 participants