chore(sds): optimise lookups#2745
Draft
danisharora099 wants to merge 20 commits intofeat/persistent_historyfrom
Draft
chore(sds): optimise lookups#2745danisharora099 wants to merge 20 commits intofeat/persistent_historyfrom
danisharora099 wants to merge 20 commits intofeat/persistent_historyfrom
Conversation
…istentHistory` to use composition over inheritance for history management.
…e key generation.
…ifying history storage initialization to default to `localStorage`
…mproved compatibility.
…in message channels
…d refactor `MemLocalHistory` to support optional persistent storage.
…ingDependencies`
5 tasks
size-limit report 📦
|
5 tasks
Collaborator
|
This PR should go before the persistent storage one. Because here, you are refining the interface to implement, and in persistent storage, you are adding a second implementation. |
fryorcraken
reviewed
Nov 29, 2025
| @@ -20,33 +25,13 @@ export const DEFAULT_MAX_LENGTH = 10_000; | |||
| * at next push. | |||
| */ | |||
| export interface ILocalHistory { | |||
Collaborator
There was a problem hiding this comment.
MessageChannel is the one to define the interface, hence this should be in message_channel.ts.
| addMessages(...messages: ContentMessage[]): void; | ||
| hasMessage(messageId: string): boolean; | ||
| getMessage(messageId: string): ContentMessage | undefined; | ||
| getRecentMessages(count: number): ContentMessage[]; |
Collaborator
There was a problem hiding this comment.
I assume the expectation is for this to be ordered? if so, I'd add it to the name.
| } | ||
|
|
||
| public get length(): number { | ||
| public get size(): number { |
Comment on lines
45
to
+46
| private items: ContentMessage[] = []; | ||
| private messageIndex: Map<string, ContentMessage> = new Map(); |
Collaborator
There was a problem hiding this comment.
Suggested change
| private items: ContentMessage[] = []; | |
| private messageIndex: Map<string, ContentMessage> = new Map(); | |
| private orderedMessageArray: ContentMessage[] = []; | |
| private messageMap: Map<string, ContentMessage> = new Map(); |
I don't think a Map qualify as an index because it cotnains both keys (index) and values (not a ref)
| // Remove duplicates by messageId while maintaining order | ||
| this.items = _.uniqBy(combinedItems, "messageId"); | ||
|
|
||
| this.rebuildIndex(); |
Collaborator
There was a problem hiding this comment.
You are rebuilding the index, and then you are removing items from the array, and then you are removing the removed items from the index.
I think there is a better way to do this.
danisharora099
force-pushed
the
feat/persistent_history
branch
from
75e8e12 to
e396c3b
Compare
Collaborator
|
is PR ready for review? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem / Description
ILocalHistorywas designed as an array-like interface (push,find,some,slice) which requiredO(N × M)array scans for dependency checking, which resulted in expensive lookup timesSolution
ILocalHistoryto implement a domain-specific interface:messageIndex) behindILocalHistoryforO(1)lookups for missing dependencyO(N+M)Notes
Checklist