-
Notifications
You must be signed in to change notification settings - Fork 137
feat(ui): initial canvas functionality #1667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @kapetr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers the initial implementation of Canvas functionality, a significant new feature that allows for interactive content within the user interface. It encompasses updates across the Python and TypeScript SDKs to integrate the Canvas extension, alongside the development of new UI components for rendering, managing, and interacting with Canvas-based artifacts. Users can now select text within a Canvas and submit edit requests to the agent, enhancing the interactive experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces initial Canvas functionality to the UI, which involves significant changes across both Python SDK examples and the TypeScript UI. Key updates include new UI components for displaying and interacting with canvas artifacts, a mechanism to map DOM selections back to markdown source for editing, and integration of canvas-specific message parts and API extensions. The changes are generally well-structured and introduce a powerful new feature. However, there are a few areas that require attention regarding robustness, consistency, and potential breaking changes in the TypeScript SDK.
apps/agentstack-sdk-ts/src/client/a2a/extensions/handle-agent-card.ts
Outdated
Show resolved
Hide resolved
apps/agentstack-ui/src/modules/runs/contexts/agent-demands/build-fulfillments.ts
Outdated
Show resolved
Hide resolved
5220d2f to
b2ab5ef
Compare
|
|
||
| const URI = 'https://a2a-extensions.agentstack.beeai.dev/ui/canvas/v1'; | ||
|
|
||
| const schema = z.null(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird to me. Shouldn't we rather put an empty object there? Maybe there will be some demands in the future, so let's prepare for that right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and it would simplify things on the UI. What do you think @tomkis ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a demand with empty parameters can be distinguished from a non-existent demand. But I completely agree with your concern - we should rethink how this is handled.
| if (canvasDemands !== undefined) { | ||
| const canvasEditRequest = fulfillments.canvasEditRequest(); | ||
| if (canvasEditRequest) { | ||
| fulfilledMetadata = fulfillCanvasDemand(fulfilledMetadata, canvasEditRequest); | ||
| } | ||
| } | ||
|
|
||
| const oauthRedirectUri = fulfillments.oauthRedirectUri(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you checking undefined directly here? I would probably try to make it all as similar as possible to the other functions (with regard to parameters, return values, etc.). Maybe in the future some mechanism will be created so that all this doesn't have to be called manually. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reason as the previous comment.
| const parts = handleArtifactUpdate(event); | ||
| const parts = handleArtifactUpdate(event, demands.canvasDemands !== undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good solution? Theoretically, it could just be a matter of returning the file and not the canvas itself. Shouldn't this be differentiated more at the metadata level? I'd be happy to talk about it some more. This seems fragile to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comments. Not sure i understand the comment with the file (it can still return FilePart). This essentially checks if the agent can handle canvas edit requests, meaning it will add ArtifactPart and render Canvas UI.
But I'll be happy to discuss this, it's really an initial version!
apps/agentstack-ui/src/modules/canvas/markdown/hooks/useMarkdownSelectionDialog.ts
Outdated
Show resolved
Hide resolved
apps/agentstack-ui/src/modules/canvas/markdown/hooks/useTextSelection.ts
Outdated
Show resolved
Hide resolved
apps/agentstack-ui/src/modules/canvas/markdown/utils/mapDOMSelectionToMarkdown.ts
Outdated
Show resolved
Hide resolved
apps/agentstack-ui/src/modules/canvas/markdown/utils/mapDOMSelectionToMarkdown.ts
Outdated
Show resolved
Hide resolved
| import { findWithIndex } from '#utils/helpers.ts'; | ||
| import { toMarkdownArtifact } from '#utils/markdown.ts'; | ||
|
|
||
| export function processMessageArtifactPart( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be better if the function returned parts and the array mutation took place outside the function. This is how it is handled in other cases in addMessagePart fn. I don't see any actual problem there, but it seems a little clearer to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would basically mean removing this function and copying contents into the addMessagePart. I'd prefer that as well, but it would clutter addMessagePart quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I just found the function a little difficult to read. Maybe it's primarily because of the long if conditions. But I guess it's not such a big deal. So let's leave it as it is. :-)
Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
…wn source Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]> feat(ui): finalize canvas, improve selection mapping Signed-off-by: Petr Kadlec <[email protected]> Signed-off-by: Petr Kadlec <[email protected]> Signed-off-by: Petr Kadlec <[email protected]> Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
b2ab5ef to
c568163
Compare
Signed-off-by: Petr Kadlec <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces initial Canvas functionality to the UI, enabling users to interact with artifacts displayed in a canvas-like view. Key changes include new Python SDK examples for canvas agents, TypeScript SDK updates for canvas extensions, and significant UI components for rendering and interacting with canvas artifacts. The UI now supports selecting text within markdown content to initiate edit requests, and a new context (CanvasProvider) manages active artifacts. Overall, the changes are well-structured and integrate the new feature effectively. However, there are a few areas that could be improved for robustness and clarity, particularly regarding state management and selection logic.
apps/agentstack-ui/src/modules/canvas/markdown/utils/mapDOMSelectionToMarkdown.ts
Show resolved
Hide resolved
| if message.metadata and self.spec.URI in message.metadata and message.parts: | ||
| message.parts = [part for part in message.parts if not isinstance(part.root, TextPart)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying message.parts in place within handle_incoming_message can lead to unexpected side effects if the A2AMessage object is intended to be immutable or if other parts of the system rely on its original state. It's generally safer to create a new list of parts rather than mutating the incoming message directly.
| if message.metadata and self.spec.URI in message.metadata and message.parts: | |
| message.parts = [part for part in message.parts if not isinstance(part.root, TextPart)] | |
| if message.metadata and self.spec.URI in message.metadata and message.parts: | |
| # Create a new list of parts to avoid mutating the original message object | |
| message.parts = [part for part in message.parts if not isinstance(part.root, TextPart)] |
| artifact_id: z.string(), | ||
| }); | ||
|
|
||
| export type CanvasFulfillments = z.infer<typeof responseSchema>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets rename this, it's no longer fullfilment.
| }; | ||
| } | ||
|
|
||
| const canvasEditRequest = fulfillments.canvasEditRequest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. Canvas Edit Request is not fullfilemnt.
Signed-off-by: Petr Kadlec <[email protected]>
| const inputRequiredMetadata = await resolveInputRequiredMetadata(responses); | ||
| const uiExtensionMetadata = await resolveUiExtensionMetadata(uiExtensionInputs); | ||
|
|
||
| console.log({ uiExtensionInputs, uiExtensionMetadata }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgotten console.log
Signed-off-by: Petr Kadlec <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces initial canvas functionality, a significant new feature enabling users to interact with agent-generated artifacts like code or recipes in a dedicated UI panel. The implementation spans the Python and TypeScript SDKs and the UI application, featuring artifact streaming, an editing interface with text selection, and markdown-to-DOM position mapping. The changes are extensive and well-structured. I've identified a high-severity bug in the UI's artifact streaming logic that could cause data duplication, and two medium-severity issues in the Python example files related to unsafe array access that could lead to crashes. Addressing these points will enhance the feature's robustness.
Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
Signed-off-by: Petr Kadlec <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant new functionality for a 'Canvas' UI, allowing users to select and request edits on artifacts generated by an agent. The changes span across the Python SDK, the TypeScript SDK, and the UI application, including new components, hooks, and context providers. The implementation is quite comprehensive, covering everything from the backend agent extension to the frontend selection mechanism with a floating toolbar.
My review focuses on improving robustness, performance, and correctness in the new example files and core logic. I've identified a few areas for improvement:
- In the Python example agents, there are opportunities to make artifact part access safer and to optimize regex compilation.
- In the Python canvas extension, a type check appears to be incorrect, which could lead to bugs.
- In the UI, there's a minor bug in how artifact content is constructed and an accessibility issue in a new form.
- The new documentation for the canvas feature contains a couple of errors in its code examples that could mislead developers.
Overall, this is a well-executed feature addition. The use of modern browser APIs like CSS Custom Highlights is a nice touch. Addressing the feedback will make the implementation more robust and the documentation more accurate.
apps/agentstack-ui/src/modules/canvas/markdown/CanvasEditForm.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Petr Kadlec <[email protected]>
Summary
Adds initial Canvas functionality to the UI
Linked Issues
Refs #168
Documentation
Breaking changes
TS SDK
handleInputRequired->resolveUserMetadataInputRequiredResponses->UserMetadataInputs