Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds context annotations to user messages by extracting annotation data from IIIF Canvas objects and including it as contextual information in chat interactions.
Key changes:
- Enhanced message formatting to include Canvas annotation context when the Canvas changes
- Refactored BaseProvider to support system prompt generation and plugin state management
- Updated message structure to include Canvas context information
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/providers/userTokenProvider/index.tsx | Added annotation extraction logic and context injection into user messages |
| src/plugin/context/index.ts | Updated exports to include PluginContextStore type |
| src/plugin/base_provider.tsx | Added plugin state management and system prompt generation methods |
| src/plugin/Panel/index.tsx | Refactored to use BaseProvider's system prompt generation |
| src/plugin/Panel/ChatInput/index.tsx | Added Canvas context to message structure |
| package.json | Added dedent dependency for template string formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * Use an arrow function so `this` references the `UserTokenProvider` class | ||
| */ | ||
| #format_message(message: Message) { | ||
| #format_message = (message: Message, index: number, messages: Message[]): CoreMessage => { |
There was a problem hiding this comment.
The method signature has changed from a private method to an arrow function property with additional parameters. This is a breaking change that affects the method's binding and parameters. Consider maintaining backward compatibility or documenting this breaking change.
| #format_message = (message: Message, index: number, messages: Message[]): CoreMessage => { | |
| // Backward compatibility: provide previous method signature as a private method | |
| #format_message(message: Message): CoreMessage { | |
| // Call the new arrow function with default values for index and messages | |
| return this.#format_message_arrow(message, 0, []); | |
| } | |
| // New implementation with additional parameters | |
| #format_message_arrow = (message: Message, index: number, messages: Message[]): CoreMessage => { |
| const prevMessages = messages.slice(0, index); | ||
| const lastUserMessage = prevMessages.findLast((m) => m.role === "user"); |
There was a problem hiding this comment.
The prevMessages.slice(0, index) operation creates a new array for every message being processed, and findLast searches through all previous messages. This results in O(n²) complexity when processing multiple messages. Consider caching the last user message or restructuring to avoid repeated array operations.
| const canvas = this.plugin_state.vault.serialize<Canvas>( | ||
| { | ||
| type: "Canvas", | ||
| id: message.context.canvas.id, | ||
| }, | ||
| serializeConfigPresentation3, | ||
| ); |
There was a problem hiding this comment.
Canvas serialization is performed for every user message when the Canvas changes. This could be expensive for large Canvas objects. Consider caching the serialized Canvas or extracting annotations separately to avoid full serialization overhead.
src/plugin/base_provider.tsx
Outdated
There was a problem hiding this comment.
Potential array access error if getLabelByUserLanguage() returns an empty array. The [0] access will return undefined, but the fallback ?? "N/A" won't trigger since undefined is not nullish in this context when chained with optional chaining.
| ## Title: ${getLabelByUserLanguage(manifest.label ?? undefined)[0] ?? "N/A"} | |
| ## Summary: ${manifest.summary ? getLabelByUserLanguage(manifest.summary)[0] : "N/A"} | |
| ## Title: ${getLabelByUserLanguage(manifest.label ?? undefined)?.[0] ?? "N/A"} | |
| ## Summary: ${manifest.summary ? getLabelByUserLanguage(manifest.summary)?.[0] ?? "N/A" : "N/A"} |
Closes #24 by adding annotations into the user message as context