From 94702e523e222dfd8838d959399937285f3e2aa2 Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Tue, 17 Dec 2024 14:03:59 +0100 Subject: [PATCH 1/7] fix: message duplication when some messages have the same creation timestamp --- src/thread.ts | 1 + src/utils.ts | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/thread.ts b/src/thread.ts index f8f39beef..6ae2545e7 100644 --- a/src/thread.ts +++ b/src/thread.ts @@ -354,6 +354,7 @@ export class Thread { sortedArray: replies, sortDirection: 'ascending', selectValueToCompare: (reply) => reply.created_at.getTime(), + selectKey: (reply) => reply.id, }); const actualIndex = diff --git a/src/utils.ts b/src/utils.ts index ba63af8b6..ad244f681 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -311,6 +311,7 @@ export function formatMessage({ needle, sortedArray, + selectKey, selectValueToCompare = (e) => e, sortDirection = 'ascending', }: { @@ -325,6 +326,7 @@ export const findIndexInSortedArray = ({ * selectValueToCompare: (message) => message.created_at.getTime() * ``` */ + selectKey?: (arrayElement: T) => string; selectValueToCompare?: (arrayElement: T) => L | T; /** * @default ascending @@ -353,11 +355,9 @@ export const findIndexInSortedArray = ({ const comparableMiddle = selectValueToCompare(sortedArray[middle]); - // if (comparableNeedle === comparableMiddle) return middle; - if ( (sortDirection === 'ascending' && comparableNeedle < comparableMiddle) || - (sortDirection === 'descending' && comparableNeedle > comparableMiddle) + (sortDirection === 'descending' && comparableNeedle >= comparableMiddle) ) { right = middle - 1; } else { @@ -365,6 +365,18 @@ export const findIndexInSortedArray = ({ } } + // In case there are several array elements with the same comparable value, search around the insertion + // point to possibly find an element with the same key. If found, prefer it. + // This, for example, prevents duplication of messages with the same creation date. + if (selectKey) { + const needleKey = selectKey(needle); + for (let i = left; sortedArray[i] === comparableNeedle; i += sortDirection === 'ascending' ? -1 : +1) { + if (selectKey(sortedArray[i]) === needleKey) { + return i; + } + } + } + return left; }; @@ -410,6 +422,7 @@ export function addToMessageList( sortDirection: 'ascending', // eslint-disable-next-line @typescript-eslint/no-non-null-assertion selectValueToCompare: (m) => m[sortBy]!.getTime(), + selectKey: (m) => m.id, }); // message already exists and not filtered with timestampChanged, update and return From a182f0d2686c3461a3b4cd48b585a862a797500e Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Tue, 17 Dec 2024 14:08:55 +0100 Subject: [PATCH 2/7] fix: there's only one case for updating the message now --- src/utils.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index ad244f681..9ebc71f05 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -426,16 +426,14 @@ export function addToMessageList( }); // message already exists and not filtered with timestampChanged, update and return - if (!timestampChanged && newMessage.id) { - if (newMessages[insertionIndex] && newMessage.id === newMessages[insertionIndex].id) { - newMessages[insertionIndex] = newMessage; - return newMessages; - } - - if (newMessages[insertionIndex - 1] && newMessage.id === newMessages[insertionIndex - 1].id) { - newMessages[insertionIndex - 1] = newMessage; - return newMessages; - } + if ( + !timestampChanged && + newMessage.id && + newMessages[insertionIndex] && + newMessage.id === newMessages[insertionIndex].id + ) { + newMessages[insertionIndex] = newMessage; + return newMessages; } // do not add updated or deleted messages to the list if they already exist or come with a timestamp change From b2dec4c2cdd5fc4da019160c2e509bab00af2bec Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Tue, 17 Dec 2024 14:28:10 +0100 Subject: [PATCH 3/7] fix: out of bounds --- src/utils.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/utils.ts b/src/utils.ts index 9ebc71f05..73c36976a 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -370,7 +370,12 @@ export const findIndexInSortedArray = ({ // This, for example, prevents duplication of messages with the same creation date. if (selectKey) { const needleKey = selectKey(needle); - for (let i = left; sortedArray[i] === comparableNeedle; i += sortDirection === 'ascending' ? -1 : +1) { + const step = sortDirection === 'ascending' ? -1 : +1; + for ( + let i = left + step; + 0 <= i && i < sortedArray.length && selectValueToCompare(sortedArray[i]) === comparableNeedle; + i += step + ) { if (selectKey(sortedArray[i]) === needleKey) { return i; } From cf39e41898b4f1f1392ecff0e8a4a0d4edfb7c5f Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Wed, 18 Dec 2024 16:36:49 +0100 Subject: [PATCH 4/7] add tests --- test/unit/utils.test.ts | 146 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 1 deletion(-) diff --git a/test/unit/utils.test.ts b/test/unit/utils.test.ts index 04f107804..93be91c47 100644 --- a/test/unit/utils.test.ts +++ b/test/unit/utils.test.ts @@ -3,7 +3,7 @@ import { v4 as uuidv4 } from 'uuid'; import { generateMsg } from './test-utils/generateMessage'; -import { addToMessageList, formatMessage } from '../../src/utils'; +import { addToMessageList, findIndexInSortedArray, formatMessage } from '../../src/utils'; import type { FormatMessageResponse, MessageResponse } from '../../src'; @@ -105,3 +105,147 @@ describe('addToMessageList', () => { expect(messagesAfter[4]).to.equal(newMessage); }); }); + +describe('findIndexInSortedArray', () => { + it('finds index in the middle of haystack (asc)', () => { + const needle = 5; + const haystack = [1, 2, 3, 4, 6, 7, 8, 9]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'ascending' }); + expect(index).to.eq(4); + }); + + it('finds index at the top of haystack (asc)', () => { + const needle = 0; + const haystack = [1, 2, 3, 4, 6, 7, 8, 9]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'ascending' }); + expect(index).to.eq(0); + }); + + it('finds index at the bottom of haystack (asc)', () => { + const needle = 10; + const haystack = [1, 2, 3, 4, 6, 7, 8, 9]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'ascending' }); + expect(index).to.eq(8); + }); + + it('in a haystack with duplicates, prefers index closer to the bottom (asc)', () => { + const needle = 5; + const haystack = [1, 5, 5, 5, 5, 5, 8, 9]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'ascending' }); + expect(index).to.eq(6); + }); + + it('in a haystack with duplicates, look up an item by key (asc)', () => { + const haystack: [key: string, value: number][] = [ + ['one', 1], + ['five-1', 5], + ['five-2', 5], + ['five-3', 5], + ['nine', 9], + ]; + + const selectKey = (tuple: [key: string, value: number]) => tuple[0]; + const selectValue = (tuple: [key: string, value: number]) => tuple[1]; + + expect( + findIndexInSortedArray({ + needle: ['five-1', 5], + sortedArray: haystack, + sortDirection: 'ascending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(1); + + expect( + findIndexInSortedArray({ + needle: ['five-2', 5], + sortedArray: haystack, + sortDirection: 'ascending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(2); + + expect( + findIndexInSortedArray({ + needle: ['five-3', 5], + sortedArray: haystack, + sortDirection: 'ascending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(3); + }); + + it('finds index in the middle of haystack (desc)', () => { + const needle = 5; + const haystack = [9, 8, 7, 6, 4, 3, 2, 1]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'descending' }); + expect(index).to.eq(4); + }); + + it('finds index at the top of haystack (desc)', () => { + const needle = 10; + const haystack = [9, 8, 7, 6, 4, 3, 2, 1]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'descending' }); + expect(index).to.eq(0); + }); + + it('finds index at the bottom of haystack (desc)', () => { + const needle = 0; + const haystack = [9, 8, 7, 6, 4, 3, 2, 1]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'descending' }); + expect(index).to.eq(8); + }); + + it('in a haystack with duplicates, prefers index closer to the top (desc)', () => { + const needle = 5; + const haystack = [9, 8, 5, 5, 5, 5, 5, 1]; + const index = findIndexInSortedArray({ needle, sortedArray: haystack, sortDirection: 'descending' }); + expect(index).to.eq(2); + }); + + it('in a haystack with duplicates, look up an item by key (desc)', () => { + const haystack: [key: string, value: number][] = [ + ['nine', 9], + ['five-1', 5], + ['five-2', 5], + ['five-3', 5], + ['one', 1], + ]; + + const selectKey = (tuple: [key: string, value: number]) => tuple[0]; + const selectValue = (tuple: [key: string, value: number]) => tuple[1]; + + expect( + findIndexInSortedArray({ + needle: ['five-1', 5], + sortedArray: haystack, + sortDirection: 'descending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(1); + + expect( + findIndexInSortedArray({ + needle: ['five-2', 5], + sortedArray: haystack, + sortDirection: 'descending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(2); + + expect( + findIndexInSortedArray({ + needle: ['five-3', 5], + sortedArray: haystack, + sortDirection: 'descending', + selectKey, + selectValueToCompare: selectValue, + }), + ).to.eq(3); + }); +}); From 41275d9bef77bcac6caec589031d11932dc2c6d6 Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Wed, 18 Dec 2024 16:44:12 +0100 Subject: [PATCH 5/7] add jsdoc --- src/utils.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 73c36976a..acb78dad9 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -318,15 +318,25 @@ export const findIndexInSortedArray = ({ needle: T; sortedArray: readonly T[]; /** - * In array of objects (like messages), pick a specific - * property to compare needle value to. + * In an array of objects (like messages), pick a unique property identifying + * an element. It will be used to find a direct match for the needle element + * in case compare values are not unique. * * @example * ```ts - * selectValueToCompare: (message) => message.created_at.getTime() + * selectKey: (message) => message.id * ``` */ selectKey?: (arrayElement: T) => string; + /** + * In an array of objects (like messages), pick a specific + * property to compare the needle value to. + * + * @example + * ```ts + * selectValueToCompare: (message) => message.created_at.getTime() + * ``` + */ selectValueToCompare?: (arrayElement: T) => L | T; /** * @default ascending From 51ed4f8bfd5f2a3ea1adc478d4d84aa2cc0284f9 Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Wed, 18 Dec 2024 16:45:53 +0100 Subject: [PATCH 6/7] simplify usage in threads --- src/thread.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/thread.ts b/src/thread.ts index 6ae2545e7..66d315b48 100644 --- a/src/thread.ts +++ b/src/thread.ts @@ -357,15 +357,12 @@ export class Thread { selectKey: (reply) => reply.id, }); - const actualIndex = - replies[index]?.id === message.id ? index : replies[index - 1]?.id === message.id ? index - 1 : null; - - if (actualIndex === null) { + if (replies[index]?.id !== message.id ? index : null) { return; } const updatedReplies = [...replies]; - updatedReplies.splice(actualIndex, 1); + updatedReplies.splice(index, 1); this.state.partialNext({ replies: updatedReplies, From 17f8269c543266c59939915df0fa8849995bb122 Mon Sep 17 00:00:00 2001 From: Matvei Andrienko Date: Wed, 18 Dec 2024 17:11:54 +0100 Subject: [PATCH 7/7] fix typo --- src/thread.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/thread.ts b/src/thread.ts index 66d315b48..24bbe072b 100644 --- a/src/thread.ts +++ b/src/thread.ts @@ -357,7 +357,7 @@ export class Thread { selectKey: (reply) => reply.id, }); - if (replies[index]?.id !== message.id ? index : null) { + if (replies[index]?.id !== message.id) { return; }