Fix bug causing generateEmbeddings to run when it shouldn't #53
Conversation
…g to index embeddings
WalkthroughWalkthroughThis update extensively enhances the indexing functionality for AI settings within the application. Key changes include the introduction of mock providers for testing and the refactoring of existing functions to improve modularity and maintainability. New enums allow for the inclusion of mock options, facilitating easier testing scenarios. Overall, these modifications significantly strengthen the application’s architecture while providing robust testing capabilities. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/embeddings.ts (1)
Line range hint
72-103: LGTM!The
indexEmbeddingsfunction is well-structured and the logic is clear.Reminder: Address the TODO comment.
The TODO comment suggests filtering out images or sending them to a vision model for summary.
Do you want me to generate the code for this TODO or open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/embeddings.test.ts (1 hunks)
- src/embeddings.ts (5 hunks)
- src/init.ts (4 hunks)
- src/mocks/mockproviders.ts (1 hunks)
- src/mocks/syscalls.ts (2 hunks)
- src/types.ts (1 hunks)
Additional context used
Path-based instructions (6)
src/mocks/mockproviders.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.src/mocks/syscalls.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.src/types.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.src/embeddings.test.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.src/init.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.src/embeddings.ts (1)
Pattern
**/*.ts: Review these typescript files for best practices, readability, and DRY.
Additional comments not posted (27)
src/mocks/mockproviders.ts (3)
10-27: LGTM! Consider adding type annotations for better clarity.The class is well-structured and simulates AI chat responses effectively. Adding type annotations to the constructor parameters and the
chatWithAImethod would improve code readability and maintainability.- constructor( - apiKey: string, - modelName: string, - baseUrl: string = "http://localhost", - ) { + constructor( + apiKey: string, + modelName: string, + baseUrl: string = "http://localhost", + ) { + super(apiKey, baseUrl, "mock", modelName); + } - async chatWithAI(options: StreamChatOptions): Promise<any> { + async chatWithAI(options: StreamChatOptions): Promise<string> {
31-46: LGTM! Consider adding type annotations for better clarity.The class is well-structured and simulates image generation effectively. Adding type annotations to the constructor parameters and the
generateImagemethod would improve code readability and maintainability.- constructor( - apiKey: string, - modelName: string, - baseUrl: string = "http://localhost", - ) { + constructor( + apiKey: string, + modelName: string, + baseUrl: string = "http://localhost", + ) { + super(apiKey, baseUrl, "mock", modelName); + } - generateImage(options: ImageGenerationOptions): Promise<string> { + generateImage(options: ImageGenerationOptions): Promise<string> {
49-67: LGTM! Consider adding type annotations for better clarity.The class is well-structured and simulates embedding generation effectively. Adding type annotations to the constructor parameters and the
_generateEmbeddingsmethod would improve code readability and maintainability.- constructor( - apiKey: string, - modelName: string, - baseUrl: string = "http://localhost", - ) { + constructor( + apiKey: string, + modelName: string, + baseUrl: string = "http://localhost", + ) { + super(apiKey, baseUrl, "mock", modelName); + } - _generateEmbeddings( - options: EmbeddingGenerationOptions, - ): Promise<Array<number>> { + _generateEmbeddings( + options: EmbeddingGenerationOptions, + ): Promise<Array<number>> {src/mocks/syscalls.ts (5)
55-57: LGTM!The implementation of the
clientStore.setcase is straightforward and correct.
58-59: LGTM!The implementation of the
clientStore.getcase is straightforward and correct.
50-51: LGTM!The implementation of the
system.invokeFunctionOnServercase is straightforward and correct.
52-53: LGTM!The implementation of the
system.invokeFunctioncase is straightforward and correct.
66-73: LGTM! Consider adding more commands as needed.The implementation of the
invokeFunctionMockfunction is straightforward and correct. Adding more commands as needed would improve its utility.src/types.ts (3)
65-66: LGTM!The addition of the
Mockvalue to theProviderenum is straightforward and correct.
71-72: LGTM!The addition of the
Mockvalue to theImageProviderenum is straightforward and correct.
79-80: LGTM!The addition of the
Mockvalue to theEmbeddingProviderenum is straightforward and correct.src/embeddings.test.ts (8)
54-63: LGTM!The test case for
canIndexPageis well-structured and verifies multiple conditions.
65-73: LGTM!The test case for
shouldIndexEmbeddingsis well-structured and verifies the expected behavior when conditions are met.
75-83: LGTM!The test case for
shouldIndexEmbeddingsis well-structured and verifies the expected behavior when not on the server.
85-97: LGTM!The test case for
shouldIndexEmbeddingsis well-structured and verifies the expected behavior whenindexEmbeddingsis disabled.
99-111: LGTM!The test case for
shouldIndexSummariesis well-structured and verifies the expected behavior when conditions are met.
113-121: LGTM!The test case for
shouldIndexSummariesis well-structured and verifies the expected behavior whenindexSummaryis disabled.
123-131: LGTM!The test case for
shouldIndexEmbeddingsis well-structured and verifies the expected behavior when no embedding models are configured.
133-141: LGTM!The test case for
shouldIndexSummariesis well-structured and verifies the expected behavior when no embedding models are configured.src/init.ts (3)
152-157: LGTM!The new case for
MockImageProviderinsetupImageProvideris well-structured.
179-185: LGTM!The new case for
MockProviderinsetupAIProvideris well-structured.
219-225: LGTM!The new case for
MockEmbeddingProviderinsetupEmbeddingProvideris well-structured.src/embeddings.ts (5)
Line range hint
25-41: LGTM!The
canIndexPagefunction is well-structured and the logic is clear.
48-55: LGTM!The
shouldIndexEmbeddingsfunction is well-structured and the logic is clear.
57-65: LGTM!The
shouldIndexSummariesfunction is well-structured and the logic is clear.
Line range hint
146-195: LGTM!The
indexSummaryfunction is well-structured and the logic is clear.
238-240: LGTM!The
generateEmbeddingsfunction is well-structured and the logic is clear.
This fixes two issues mentioned in silverbulletmd/silverbullet#1002 and #52
1:
Error dispatching event page:index to silverbullet-ai.indexEmbeddings: Cannot read properties of undefined (reading 'generateEmbeddings')2:
indexEmbeddingswas being ignoredSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores