diff --git a/.changelog/5969-fix-livestream-resent-revision.md b/.changelog/5969-fix-livestream-resent-revision.md new file mode 100644 index 0000000000..5e024a99ed --- /dev/null +++ b/.changelog/5969-fix-livestream-resent-revision.md @@ -0,0 +1,3 @@ +### Fixed + +- Fixed livestream revisions re-sent with the same ID shown twice, in PR [#5969](https://github.com/microsoft/BotFramework-WebChat/pull/5969), by [@OEvgeny](https://github.com/OEvgeny) diff --git a/__tests__/html2/livestream/resentRevision.entity.html b/__tests__/html2/livestream/resentRevision.entity.html new file mode 100644 index 0000000000..65b27bd06c --- /dev/null +++ b/__tests__/html2/livestream/resentRevision.entity.html @@ -0,0 +1,10 @@ + + + + resentRevision (entity): redirects to ?streamingFormat=entity + + + + diff --git a/__tests__/html2/livestream/resentRevision.entity.html.snap-1.png b/__tests__/html2/livestream/resentRevision.entity.html.snap-1.png new file mode 100644 index 0000000000..5c66fcaefb Binary files /dev/null and b/__tests__/html2/livestream/resentRevision.entity.html.snap-1.png differ diff --git a/__tests__/html2/livestream/resentRevision.html b/__tests__/html2/livestream/resentRevision.html new file mode 100644 index 0000000000..e6433b2535 --- /dev/null +++ b/__tests__/html2/livestream/resentRevision.html @@ -0,0 +1,142 @@ + + + + + + + + + + + + +
+ + + + diff --git a/__tests__/html2/livestream/resentRevision.html.snap-1.png b/__tests__/html2/livestream/resentRevision.html.snap-1.png new file mode 100644 index 0000000000..5c66fcaefb Binary files /dev/null and b/__tests__/html2/livestream/resentRevision.html.snap-1.png differ diff --git a/packages/core/src/reducers/activities/sort/upsert.howToWithLivestream.spec.ts b/packages/core/src/reducers/activities/sort/upsert.howToWithLivestream.spec.ts index 682f33afe5..e5616eacf8 100644 --- a/packages/core/src/reducers/activities/sort/upsert.howToWithLivestream.spec.ts +++ b/packages/core/src/reducers/activities/sort/upsert.howToWithLivestream.spec.ts @@ -513,3 +513,138 @@ scenario('re-sending an older revision of a finalized livestream in a grouping w expect(state.sortedActivities).toEqual([activityToExpectation(activity3, 1_000)]); }); }); + +scenario('re-sending a revision of an ongoing livestream in a grouping', bdd => { + const activity1 = buildActivity( + { + channelData: { streamSequence: 1, streamType: 'streaming' }, + id: 'a-00001', + text: 'A quick', + timestamp: new Date(1_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + const activity2 = buildActivity( + { + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'A quick brown fox', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + // Same ID with a changed text, e.g. delivered again after a reconnect. + const activity2Changed = buildActivity( + { + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'a-00002:CHANGED', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + bdd + .given('an ongoing livestream session with 2 revisions in a part grouping', () => + upsert({ Date }, upsert({ Date }, INITIAL_STATE, activity1), activity2) + ) + .when('the second revision is re-sent with a changed text', state => upsert({ Date }, state, activity2Changed)) + .then('`activityMap` should have the changed revision', (_, state) => { + expect(state.activityMap.get('_:a-00002' as LocalId)).toEqual({ + activity: activityToExpectation(activity2Changed), + activityLocalId: '_:a-00002' as LocalId, + logicalTimestamp: 2_000, + type: 'activity' + } satisfies ActivityMapEntry); + }) + .and('should keep 2 revisions in the livestream session', (_, state) => { + expect(state.livestreamSessionMap.get('a-00001' as LivestreamSessionId)).toEqual({ + activities: [ + expect.objectContaining({ activityLocalId: '_:a-00001' as LocalId }), + expect.objectContaining({ activityLocalId: '_:a-00002' as LocalId }) + ], + finalized: false, + logicalTimestamp: 1_000 + } satisfies LivestreamSessionMapEntry); + }) + .and('`sortedActivities` should have the changed revision once', (_, state) => { + expect(state.sortedActivities).toEqual([ + activityToExpectation(activity1, 1_000), + activityToExpectation(activity2Changed, 2_000) + ]); + }); +}); + +scenario('re-sending a middle revision of an ongoing livestream in a grouping', bdd => { + const activity1 = buildActivity( + { + channelData: { streamSequence: 1, streamType: 'streaming' }, + id: 'a-00001', + text: 'A quick', + timestamp: new Date(1_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + const activity2 = buildActivity( + { + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'A quick brown fox', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + const activity3 = buildActivity( + { + channelData: { streamId: 'a-00001', streamSequence: 3, streamType: 'streaming' }, + id: 'a-00003', + text: 'A quick brown fox jumped', + timestamp: new Date(3_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + // Same ID as the middle revision with a changed text, e.g. delivered again after a reconnect. + const activity2Changed = buildActivity( + { + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'a-00002:CHANGED', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }, + { isPartOf: [{ '@id': '_:how-to:00001', '@type': 'HowTo' }], position: 1 } + ); + + bdd + .given('an ongoing livestream session with 3 revisions in a part grouping', () => + upsert({ Date }, upsert({ Date }, upsert({ Date }, INITIAL_STATE, activity1), activity2), activity3) + ) + .when('the second revision is re-sent with a changed text', state => upsert({ Date }, state, activity2Changed)) + .then('`activityMap` should have the changed revision', (_, state) => { + expect(state.activityMap.get('_:a-00002' as LocalId)).toEqual({ + activity: activityToExpectation(activity2Changed), + activityLocalId: '_:a-00002' as LocalId, + logicalTimestamp: 2_000, + type: 'activity' + } satisfies ActivityMapEntry); + }) + .and('`sortedActivities` should have the changed revision once, before the third revision', (_, state) => { + expect(state.sortedActivities).toEqual([ + // Positions are not asserted: the slow path may resequence them, only the order matters. + activityToExpectation(activity1), + activityToExpectation(activity2Changed), + activityToExpectation(activity3) + ]); + }); +}); diff --git a/packages/core/src/reducers/activities/sort/upsert.livestream.spec.ts b/packages/core/src/reducers/activities/sort/upsert.livestream.spec.ts index 9d9cc9524b..edb69ec4d0 100644 --- a/packages/core/src/reducers/activities/sort/upsert.livestream.spec.ts +++ b/packages/core/src/reducers/activities/sort/upsert.livestream.spec.ts @@ -416,3 +416,134 @@ scenario('upserting a livestream session', bdd => { expect(state.sortedActivities).toEqual([activityToExpectation(activity4, 1_000)]); }); }); + +scenario('re-sending a revision of an ongoing livestream session', bdd => { + const activity1 = buildActivity({ + channelData: { streamSequence: 1, streamType: 'streaming' }, + id: 'a-00001', + text: 'A quick', + timestamp: new Date(1_000).toISOString(), + type: 'typing' + }); + + const activity2 = buildActivity({ + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'A quick brown fox', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }); + + // Same ID with a changed text, e.g. delivered again after a reconnect. + const activity2Changed = buildActivity({ + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'a-00002:CHANGED', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }); + + bdd + .given('an ongoing livestream session with 2 revisions', () => + upsert({ Date }, upsert({ Date }, INITIAL_STATE, activity1), activity2) + ) + .when('the second revision is re-sent with a changed text', state => upsert({ Date }, state, activity2Changed)) + .then('`activityMap` should have the changed revision', (_, state) => { + expect(state.activityMap.get('_:a-00002' as LocalId)).toEqual({ + activity: activityToExpectation(activity2Changed), + activityLocalId: '_:a-00002' as LocalId, + logicalTimestamp: 2_000, + type: 'activity' + } satisfies ActivityMapEntry); + }) + .and('should keep 2 revisions in the livestream session', (_, state) => { + expect(state.livestreamSessionMap).toEqual( + new Map([ + [ + 'a-00001' as LivestreamSessionId, + { + activities: [ + { + activityLocalId: '_:a-00001' as LocalId, + logicalTimestamp: 1_000, + sequenceNumber: 1, + type: 'activity' + }, + { + activityLocalId: '_:a-00002' as LocalId, + logicalTimestamp: 2_000, + sequenceNumber: 2, + type: 'activity' + } + ], + finalized: false, + logicalTimestamp: 1_000 + } + ] + ]) + ); + }) + .and('`sortedActivities` should have the changed revision once', (_, state) => { + expect(state.sortedActivities).toEqual([ + activityToExpectation(activity1, 1_000), + activityToExpectation(activity2Changed, 2_000) + ]); + }); +}); + +scenario('re-sending a middle revision of an ongoing livestream session', bdd => { + const activity1 = buildActivity({ + channelData: { streamSequence: 1, streamType: 'streaming' }, + id: 'a-00001', + text: 'A quick', + timestamp: new Date(1_000).toISOString(), + type: 'typing' + }); + + const activity2 = buildActivity({ + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'A quick brown fox', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }); + + const activity3 = buildActivity({ + channelData: { streamId: 'a-00001', streamSequence: 3, streamType: 'streaming' }, + id: 'a-00003', + text: 'A quick brown fox jumped', + timestamp: new Date(3_000).toISOString(), + type: 'typing' + }); + + // Same ID as the middle revision with a changed text, e.g. delivered again after a reconnect. + const activity2Changed = buildActivity({ + channelData: { streamId: 'a-00001', streamSequence: 2, streamType: 'streaming' }, + id: 'a-00002', + text: 'a-00002:CHANGED', + timestamp: new Date(2_000).toISOString(), + type: 'typing' + }); + + bdd + .given('an ongoing livestream session with 3 revisions', () => + upsert({ Date }, upsert({ Date }, upsert({ Date }, INITIAL_STATE, activity1), activity2), activity3) + ) + .when('the second revision is re-sent with a changed text', state => upsert({ Date }, state, activity2Changed)) + .then('`activityMap` should have the changed revision', (_, state) => { + expect(state.activityMap.get('_:a-00002' as LocalId)).toEqual({ + activity: activityToExpectation(activity2Changed), + activityLocalId: '_:a-00002' as LocalId, + logicalTimestamp: 2_000, + type: 'activity' + } satisfies ActivityMapEntry); + }) + .and('`sortedActivities` should have the changed revision once, before the third revision', (_, state) => { + expect(state.sortedActivities).toEqual([ + // Positions are not asserted: the slow path may resequence them, only the order matters. + activityToExpectation(activity1), + activityToExpectation(activity2Changed), + activityToExpectation(activity3) + ]); + }); +}); diff --git a/packages/core/src/reducers/activities/sort/upsert.ts b/packages/core/src/reducers/activities/sort/upsert.ts index 61e9d616e3..a424ffe771 100644 --- a/packages/core/src/reducers/activities/sort/upsert.ts +++ b/packages/core/src/reducers/activities/sort/upsert.ts @@ -81,7 +81,7 @@ function upsert(ponyfill: Pick, state: State, activ // #region Streaming fast path // - // For revision 2..N-1 of an existing, non-finalized livestream session without HowTo grouping: + // For a new revision 2..N-1 of an existing, non-finalized livestream session without HowTo grouping: // avoid the heavier full rebuild path, including sortedChatHistoryList recomputation, // computeSortedActivities, and full position resequencing. This is still not constant-time: // the fast path continues to clone Maps and update sortedActivities, but it avoids the @@ -91,9 +91,11 @@ function upsert(ponyfill: Pick, state: State, activ const existingSession = state.livestreamSessionMap.get(sessionId); const finalized = activityLivestreamingMetadata.type === 'final activity'; + // A re-sent revision (same local ID) takes the slow path, which replaces its previous entry. if ( existingSession && !existingSession.finalized && + !existingActivity && !finalized && !getPartGroupingMetadataMap(activity).has('HowTo') ) { @@ -231,7 +233,8 @@ function upsert(ponyfill: Pick, state: State, activ // #endregion - // Slow path: full recalculation for non-streaming, first/final revisions, reorders, or HowTo grouping. + // Slow path: full recalculation for non-streaming, first/final revisions, re-sent revisions, reorders, + // or HowTo grouping. const nextActivityIdToLocalIdMap = new Map(state.activityIdToLocalIdMap); const nextActivityMap = new Map(state.activityMap); const nextClientActivityIdToLocalIdMap = new Map(state.clientActivityIdToLocalIdMap); @@ -292,7 +295,10 @@ function upsert(ponyfill: Pick, state: State, activ const nextLivestreamSessionMapEntry = { activities: Object.freeze( insertSorted( - livestreamSessionMapEntry ? livestreamSessionMapEntry.activities : [], + // Remove the previous entry of a re-sent revision, otherwise it is shown twice. + livestreamSessionMapEntry + ? livestreamSessionMapEntry.activities.filter(entry => entry.activityLocalId !== activityLocalId) + : [], Object.freeze({ activityLocalId, logicalTimestamp,