From 026a08a77ae19e1ba402f2785be871e7270bb3a5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 3 Nov 2022 05:06:53 -0500 Subject: [PATCH] Jump forward and backward seamlessly (#121) Fix https://github.com/matrix-org/matrix-public-archive/issues/120 Follow-up to https://github.com/matrix-org/matrix-public-archive/pull/114 - Uses event permalinking (`?at=$xxx`) to continue the scroll where you should start reading again. - When we jump forwards, we make sure that we go a day back to ensure there isn't more than the page limit between where we jumped from and the day so we don't lose any messages in a gap. --- server/routes/room-routes.js | 54 +++++++++++---- test/e2e-tests.js | 125 +++++++++++++++++++++++------------ 2 files changed, 125 insertions(+), 54 deletions(-) diff --git a/server/routes/room-routes.js b/server/routes/room-routes.js index 4686fab..000c696 100644 --- a/server/routes/room-routes.js +++ b/server/routes/room-routes.js @@ -152,6 +152,7 @@ router.get( router.get( '/jump', + // eslint-disable-next-line max-statements asyncHandler(async function (req, res) { const roomIdOrAlias = getRoomIdOrAliasFromReq(req); @@ -159,15 +160,13 @@ router.get( assert(!Number.isNaN(ts), '?ts query parameter must be a number'); const dir = req.query.dir; assert(['f', 'b'].includes(dir), '?dir query parameter must be [f|b]'); - const scrollStartPosition = req.query.continue; // We have to wait for the room join to happen first before we can use the jump to // date endpoint const roomId = await ensureRoomJoined(matrixAccessToken, roomIdOrAlias, req.query.via); - let originServerTs; let eventIdForTimestamp; - + let originServerTs; try { // Find the closest day to today with messages ({ eventId: eventIdForTimestamp, originServerTs } = await timestampToEvent({ @@ -178,14 +177,15 @@ router.get( })); // The goal is to go forward 100 messages, so that when we view the room at that - // point going backwards 100 message, we end up at the perfect sam continuation spot - // in the room. + // point going backwards 100 messages, we end up at the perfect sam continuation + // spot in the room. // - // XXX: This is flawed in the fact that when we go `/messages?dir=b` it could - // backfill messages which will fill up the response before we perfectly connect and - // continue from the position they were jumping from before. When `/messages?dir=f` - // backfills, we won't have this problem anymore because any messages backfilled in - // the forwards direction would be picked up the same going backwards. + // XXX: This is flawed in the fact that when we go `/messages?dir=b` later, it + // could backfill messages which will fill up the response before we perfectly + // connect and continue from the position they were jumping from before. When + // `/messages?dir=f` backfills, we won't have this problem anymore because any + // messages backfilled in the forwards direction would be picked up the same going + // backwards. if (dir === 'f') { // Use `/messages?dir=f` and get the `end` pagination token to paginate from. And // then start the scroll from the top of the page so they can continue. @@ -198,7 +198,36 @@ router.get( limit: archiveMessageLimit, }); - originServerTs = messageResData.chunk[messageResData.chunk.length - 1]?.origin_server_ts; + if (!messageResData.chunk?.length) { + throw new StatusError( + 404, + `/messages response didn't contain any more messages to jump to` + ); + } + + const timestampOfLastMessage = + messageResData.chunk[messageResData.chunk.length - 1].origin_server_ts; + const dateOfLastMessage = new Date(timestampOfLastMessage); + + // Back track from the last message timestamp to the date boundary. This will + // gurantee some overlap with the previous page we jumped from so we don't lose + // any messages in the gap. + // + // XXX: This date boundary logic may need to change once we introduce hour + // chunks or time slices. For example if we reached into the next day but it has + // too many messages to show for a given page, we would want to back track until + // a suitable time slice boundary. Maybe we need to add a new URL parameter here + // `?time-slice=true` to indicate that it's okay to break it up by time slice + // based on previously having to view by time slice. We wouldn't want to give + const utcMidnightOfDayBefore = Date.UTC( + dateOfLastMessage.getUTCFullYear(), + dateOfLastMessage.getUTCMonth(), + dateOfLastMessage.getUTCDate() + ); + // We minus 1 from UTC midnight to get to the day before + const endOfDayBeforeDate = new Date(utcMidnightOfDayBefore - 1); + + originServerTs = endOfDayBeforeDate; } } catch (err) { const is404Error = err instanceof HTTPResponseError && err.response.status === 404; @@ -227,7 +256,8 @@ router.get( // TODO: Add query parameter that causes the client to start the scroll at the top // when jumping forwards so they can continue reading where they left off. matrixPublicArchiveURLCreator.archiveUrlForDate(roomIdOrAlias, new Date(originServerTs), { - scrollStartPosition, + // Start the scroll at the next event from where they jumped from (seamless navigation) + scrollStartEventId: eventIdForTimestamp, }) ); }) diff --git a/test/e2e-tests.js b/test/e2e-tests.js index 857d134..3e97c14 100644 --- a/test/e2e-tests.js +++ b/test/e2e-tests.js @@ -661,7 +661,7 @@ describe('matrix-public-archive', () => { let previousDayToEventMap; beforeEach(async () => { // Set this low so we can easily create more than the limit - config.set('archiveMessageLimit', 3); + config.set('archiveMessageLimit', 4); client = await getTestClientForHs(testMatrixServerUrl1); roomId = await createTestRoom(client); @@ -670,12 +670,12 @@ describe('matrix-public-archive', () => { // but don't overflow the limit on a single day basis. // // We create 4 days of messages so we can see a seamless continuation from - // page1 to page2. The page limit is 3 but each page will show 4 messages - // because we fetch one extra to determine overflow. + // page1 to page2. The page limit is 4 but each page will show up-to 5 + // messages because we fetch one extra to determine overflow. + // + // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 <-- 9 <-- 10 <-- 11 <-- 12 + // [day 1 ] [day 2 ] [day 3 ] [day 4 ] // - // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 - // [day 1] [day 2] [day 3] [day 4] - // [1st page ] [2nd page ] previousDayToEventMap = new Map(); for (let i = 1; i < 5; i++) { // The date should be just past midnight so we don't run into inclusive @@ -688,7 +688,7 @@ describe('matrix-public-archive', () => { const eventIds = await createMessagesInRoom({ client, roomId, - numMessages: 2, + numMessages: 3, prefix: `day ${i} - events in room`, timestamp: previousArchiveDate.getTime(), }); @@ -697,24 +697,37 @@ describe('matrix-public-archive', () => { }); it('can jump forward to the next activity', async () => { - // `previousDayToEventMap` maps each day to the events in that day (2 events - // per day). The page limit is 3 but each page will show 4 messages because we + // Test to make sure we can jump from the 1st page to the 2nd page forwards. + // + // `previousDayToEventMap` maps each day to the events in that day (3 events + // per day). The page limit is 4 but each page will show 5 messages because we // fetch one extra to determine overflow. // - // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 - // [day 1] [day 2] [day 3] [day 4] - // [1st page ] [2nd page ] + // In order to jump from the 1st page to the 2nd, we first jump forward 4 + // messages, then back-track to the first date boundary which is day 3. We do + // this so that we don't start from day 4 backwards which would miss messages + // because there are more than 5 messages in between day 4 and day 2. + // + // Even though there is overlap between the pages, our scroll continues from + // the event where the 1st page starts. + // + // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 <-- 9 <-- 10 <-- 11 <-- 12 + // [day 1 ] [day 2 ] [day 3 ] [day 4 ] + // [1st page ] + // |--jump-fwd-4-messages-->| + // [2nd page ] const previousArchiveDates = Array.from(previousDayToEventMap.keys()); assert.strictEqual( previousArchiveDates.length, 4, - `This test expects to work with 4 days of history, each with 2 messages and a page limit of 3 messages previousArchiveDates=${previousArchiveDates}` + `This test expects to work with 4 days of history, each with 3 messages and a page limit of 4 messages previousArchiveDates=${previousArchiveDates}` ); // Fetch messages for the 1st page (day 2 backwards) + const day2Date = previousArchiveDates[1]; const firstPageArchiveUrl = matrixPublicArchiveURLCreator.archiveUrlForDate( roomId, - previousArchiveDates[1] + day2Date ); // Set this for debugging if the test fails here archiveUrl = firstPageArchiveUrl; @@ -730,26 +743,37 @@ describe('matrix-public-archive', () => { return eventId.startsWith('$'); }); - // Assert that the first page contains 4 events (day 2 and day 1) + // Assert that the first page contains 5 events (day 2 and day 1) assert.deepEqual(eventIdsOnFirstPage, [ - // All of day 1 - ...previousDayToEventMap.get(previousArchiveDates[0]), + // Some of day 1 + ...previousDayToEventMap.get(previousArchiveDates[0]).slice(-2), // All of day 2 ...previousDayToEventMap.get(previousArchiveDates[1]), ]); - // Follow the next activity link. Aka, fetch messages for the 2nd page (day 3 - // onwards, seamless continuation from the 1st page). + // Follow the next activity link. Aka, fetch messages for the 2nd page const nextActivityLinkEl = firstPageDom.document.querySelector( '[data-testid="jump-to-next-activity-link"]' ); const nextActivityLink = nextActivityLinkEl.getAttribute('href'); // Set this for debugging if the test fails here archiveUrl = nextActivityLink; - const { data: nextActivityArchivePageHtml } = await fetchEndpointAsText(nextActivityLink); + const { data: nextActivityArchivePageHtml, res: nextActivityRes } = + await fetchEndpointAsText(nextActivityLink); const nextActivityDom = parseHTML(nextActivityArchivePageHtml); - // Assert that it's a smooth continuation to more messages with no overlap + // Assert that it's a smooth continuation to more messages + // + // First by checking where the scroll is going to start from + const urlObj = new URL(nextActivityRes.url, basePath); + const qs = new URLSearchParams(urlObj.search); + assert.strictEqual( + qs.get('at'), + // Continuing from the first event of day 3 + previousDayToEventMap.get(previousArchiveDates[2])[0] + ); + + // Then check the events are on the page correctly const eventIdsOnNextDay = [ ...nextActivityDom.document.querySelectorAll(`[data-event-id]`), ] @@ -761,34 +785,42 @@ describe('matrix-public-archive', () => { return eventId.startsWith('$'); }); - // Assert that the 2nd page contains 4 events (day 3 and day 4) + // Assert that the 2nd page contains 5 events (day 3 and day 2) assert.deepEqual(eventIdsOnNextDay, [ + // Some of day 2 + ...previousDayToEventMap.get(previousArchiveDates[1]).slice(-2), // All of day 3 ...previousDayToEventMap.get(previousArchiveDates[2]), - // All of day 4 - ...previousDayToEventMap.get(previousArchiveDates[3]), ]); }); it('can jump backward to the previous activity', async () => { - // `previousDayToEventMap` maps each day to the events in that day (2 events - // per day). The page limit is 3 but each page will show 4 messages because we + // Test to make sure we can jump from the 1st page to the 2nd page backwards + // + // `previousDayToEventMap` maps each day to the events in that day (3 events + // per day). The page limit is 4 but each page will show 5 messages because we // fetch one extra to determine overflow. // - // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 - // [day 1] [day 2] [day 3] [day 4] - // [2nd page ] [1st page ] + // The 2nd page continues from the *day* where the 1st page starts. Even + // though there is overlap between the pages, our scroll continues from the + // event where the 1st page starts. + // + // 1 <-- 2 <-- 3 <-- 4 <-- 5 <-- 6 <-- 7 <-- 8 <-- 9 <-- 10 <-- 11 <-- 12 + // [day 1 ] [day 2 ] [day 3 ] [day 4 ] + // [1st page ] + // [2nd page ] const previousArchiveDates = Array.from(previousDayToEventMap.keys()); assert.strictEqual( previousArchiveDates.length, 4, - `This test expects to work with 4 days of history, each with 2 messages and a page limit of 3 messages previousArchiveDates=${previousArchiveDates}` + `This test expects to work with 4 days of history, each with 3 messages and a page limit of 4 messages previousArchiveDates=${previousArchiveDates}` ); - // Fetch messages for the 1st page (day 4 backwards) + // Fetch messages for the 1st page (day 3 backwards) + const day3Date = previousArchiveDates[2]; const firstPageArchiveUrl = matrixPublicArchiveURLCreator.archiveUrlForDate( roomId, - previousArchiveDates[3] + day3Date ); // Set this for debugging if the test fails here archiveUrl = firstPageArchiveUrl; @@ -804,28 +836,37 @@ describe('matrix-public-archive', () => { return eventId.startsWith('$'); }); - // Assert that the first page contains 4 events (day 4 and day 3) + // Assert that the first page contains 4 events (day 3 and day 2) assert.deepEqual(eventIdsOnFirstPage, [ + // Some of day 2 + ...previousDayToEventMap.get(previousArchiveDates[1]).slice(-2), // All of day 3 ...previousDayToEventMap.get(previousArchiveDates[2]), - // All of day 4 - ...previousDayToEventMap.get(previousArchiveDates[3]), ]); - // Follow the previous activity link. Aka, fetch messages for the 2nd page (day 2 - // backwards, seamless continuation from the 1st page). + // Follow the previous activity link const previousActivityLinkEl = firstPageDom.document.querySelector( '[data-testid="jump-to-previous-activity-link"]' ); const previousActivityLink = previousActivityLinkEl.getAttribute('href'); // Set this for debugging if the test fails here archiveUrl = previousActivityLink; - const { data: previousActivityArchivePageHtml } = await fetchEndpointAsText( - previousActivityLink - ); + const { data: previousActivityArchivePageHtml, res: previousActivityRes } = + await fetchEndpointAsText(previousActivityLink); const previousActivityDom = parseHTML(previousActivityArchivePageHtml); - // Assert that it's a smooth continuation to more messages with no overlap + // Assert that it's a smooth continuation to more messages + // + // First by checking where the scroll is going to start from + const urlObj = new URL(previousActivityRes.url, basePath); + const qs = new URLSearchParams(urlObj.search); + assert.strictEqual( + qs.get('at'), + // Continuing from the first event of day 2 + previousDayToEventMap.get(previousArchiveDates[1])[0] + ); + + // Then check the events are on the page correctly const eventIdsOnPreviousDay = [ ...previousActivityDom.document.querySelectorAll(`[data-event-id]`), ] @@ -840,7 +881,7 @@ describe('matrix-public-archive', () => { // Assert that the 2nd page contains 4 events (day 2 and day 1) assert.deepEqual(eventIdsOnPreviousDay, [ // All of day 1 - ...previousDayToEventMap.get(previousArchiveDates[0]), + ...previousDayToEventMap.get(previousArchiveDates[0]).slice(-2), // All of day 2 ...previousDayToEventMap.get(previousArchiveDates[1]), ]);