From f45a27703d94221d3701b3b1fc29b3cd29e1e5dd Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 9 Sep 2022 11:09:15 +0100 Subject: [PATCH 1/2] bugfix: fix a sliding sync bug which could cause rooms to loop With a Jest regression test. --- src/stores/RoomViewStore.tsx | 9 ++++++ test/stores/RoomViewStore-test.tsx | 48 ++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/src/stores/RoomViewStore.tsx b/src/stores/RoomViewStore.tsx index c9324edc5d3..33949378cc3 100644 --- a/src/stores/RoomViewStore.tsx +++ b/src/stores/RoomViewStore.tsx @@ -60,6 +60,8 @@ const INITIAL_STATE = { joinError: null as Error, // The room ID of the room currently being viewed roomId: null as string, + // The room ID being subscribed to (in Sliding Sync) + subscribingRoomId: null as string, // The event to scroll to when the room is first viewed initialEventId: null as string, @@ -286,6 +288,7 @@ export class RoomViewStore extends Store { SlidingSyncManager.instance.setRoomVisible(this.state.roomId, false); } this.setState({ + subscribingRoomId: payload.room_id, roomId: payload.room_id, initialEventId: null, initialEventPixelOffset: null, @@ -300,6 +303,12 @@ export class RoomViewStore extends Store { // set this room as the room subscription. We need to await for it as this will fetch // all room state for this room, which is required before we get the state below. await SlidingSyncManager.instance.setRoomVisible(payload.room_id, true); + // Whilst we were subscribing another room was viewed, so stop what we're doing and + // unsubscribe + if (this.state.subscribingRoomId !== payload.room_id) { + SlidingSyncManager.instance.setRoomVisible(this.state.roomId, false); + return; + } // Re-fire the payload: we won't re-process it because the prev room ID == payload room ID now dis.dispatch({ ...payload, diff --git a/test/stores/RoomViewStore-test.tsx b/test/stores/RoomViewStore-test.tsx index b505fc6e6db..d848f035572 100644 --- a/test/stores/RoomViewStore-test.tsx +++ b/test/stores/RoomViewStore-test.tsx @@ -20,6 +20,8 @@ import { RoomViewStore } from '../../src/stores/RoomViewStore'; import { Action } from '../../src/dispatcher/actions'; import * as testUtils from '../test-utils'; import { flushPromises, getMockClientWithEventEmitter } from '../test-utils'; +import SettingsStore from '../../src/settings/SettingsStore'; +import { SlidingSyncManager } from '../../src/SlidingSyncManager'; const dispatch = testUtils.getDispatchForStore(RoomViewStore.instance); @@ -47,7 +49,7 @@ describe('RoomViewStore', function() { beforeEach(function() { jest.clearAllMocks(); - mockClient.credentials = { userId: "@test:example.com" }; + mockClient.credentials = { userId: userId }; mockClient.joinRoom.mockResolvedValue(room); mockClient.getRoom.mockReturnValue(room); mockClient.isGuest.mockReturnValue(false); @@ -58,7 +60,7 @@ describe('RoomViewStore', function() { it('can be used to view a room by ID and join', async () => { dispatch({ action: Action.ViewRoom, room_id: '!randomcharacters:aser.ver' }); - dispatch({ action: 'join_room' }); + dispatch({ action: Action.JoinRoom }); await flushPromises(); expect(mockClient.joinRoom).toHaveBeenCalledWith('!randomcharacters:aser.ver', { viaServers: [] }); expect(RoomViewStore.instance.isJoining()).toBe(true); @@ -78,11 +80,51 @@ describe('RoomViewStore', function() { expect(RoomViewStore.instance.getRoomId()).toBe(roomId); // join the room - dispatch({ action: 'join_room' }); + dispatch({ action: Action.JoinRoom }); expect(RoomViewStore.instance.isJoining()).toBeTruthy(); await flushPromises(); expect(mockClient.joinRoom).toHaveBeenCalledWith(alias, { viaServers: [] }); }); + + describe('Sliding Sync', function() { + + beforeEach(() => { + jest.spyOn(SettingsStore, 'getValue').mockImplementation((settingName, roomId, value) => { + return settingName === "feature_sliding_sync"; // this is enabled, everything else is disabled. + }); + RoomViewStore.instance.reset(); + }); + + it("subscribes to the room", async () => { + const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue(Promise.resolve("")); + const subscribedRoomId = "!sub1:localhost"; + dispatch({ action: Action.ViewRoom, room_id: subscribedRoomId }); + await flushPromises(); + await flushPromises(); + expect(RoomViewStore.instance.getRoomId()).toBe(subscribedRoomId); + expect(setRoomVisible).toHaveBeenCalledWith(subscribedRoomId, true); + }); + + // Regression test for an in-the-wild bug where rooms would rapidly switch forever in sliding sync mode + it("doesn't get stuck in a loop if you view rooms quickly", async () => { + const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue(Promise.resolve("")); + const subscribedRoomId = "!sub2:localhost"; + const subscribedRoomId2 = "!sub3:localhost"; + dispatch({ action: Action.ViewRoom, room_id: subscribedRoomId }); + dispatch({ action: Action.ViewRoom, room_id: subscribedRoomId2 }); + // sub(1) then unsub(1) sub(2) + expect(setRoomVisible).toHaveBeenCalledTimes(3); + await flushPromises(); + await flushPromises(); + // this should not churn, extra call to allow unsub(1) + expect(setRoomVisible).toHaveBeenCalledTimes(4); + // flush a bit more to ensure this doesn't change + await flushPromises(); + await flushPromises(); + expect(setRoomVisible).toHaveBeenCalledTimes(4); + }); + + }); }); From 9be29f983d92e94ebdfcec78409ed6ac1e6f0339 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 9 Sep 2022 11:12:12 +0100 Subject: [PATCH 2/2] Linting --- test/stores/RoomViewStore-test.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/stores/RoomViewStore-test.tsx b/test/stores/RoomViewStore-test.tsx index d848f035572..cfdd3ab88f1 100644 --- a/test/stores/RoomViewStore-test.tsx +++ b/test/stores/RoomViewStore-test.tsx @@ -89,7 +89,6 @@ describe('RoomViewStore', function() { }); describe('Sliding Sync', function() { - beforeEach(() => { jest.spyOn(SettingsStore, 'getValue').mockImplementation((settingName, roomId, value) => { return settingName === "feature_sliding_sync"; // this is enabled, everything else is disabled. @@ -98,7 +97,9 @@ describe('RoomViewStore', function() { }); it("subscribes to the room", async () => { - const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue(Promise.resolve("")); + const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue( + Promise.resolve(""), + ); const subscribedRoomId = "!sub1:localhost"; dispatch({ action: Action.ViewRoom, room_id: subscribedRoomId }); await flushPromises(); @@ -109,7 +110,9 @@ describe('RoomViewStore', function() { // Regression test for an in-the-wild bug where rooms would rapidly switch forever in sliding sync mode it("doesn't get stuck in a loop if you view rooms quickly", async () => { - const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue(Promise.resolve("")); + const setRoomVisible = jest.spyOn(SlidingSyncManager.instance, "setRoomVisible").mockReturnValue( + Promise.resolve(""), + ); const subscribedRoomId = "!sub2:localhost"; const subscribedRoomId2 = "!sub3:localhost"; dispatch({ action: Action.ViewRoom, room_id: subscribedRoomId }); @@ -125,6 +128,5 @@ describe('RoomViewStore', function() { await flushPromises(); expect(setRoomVisible).toHaveBeenCalledTimes(4); }); - }); });