Conversation
| // Handle successful completion | ||
| if (render.state === "DONE") { | ||
| return { | ||
| content: [], |
There was a problem hiding this comment.
I think we should remove text content everywhere, because we use the same data as in structuredContent, just stringyfied.
There was a problem hiding this comment.
it's explicitly said this is needed for backward compatibility on the spec:
There was a problem hiding this comment.
I know but I didn't notice any difference.. Do you think that depends on lcp clients or what?
There was a problem hiding this comment.
sure it depends on the client
| @@ -0,0 +1,3 @@ | |||
| import env from "./env"; | |||
|
|
|||
| export const PLAINLY_APP_URL = env.PLAINLY_API_URL.replace("api.", "app."); | |||
There was a problem hiding this comment.
come on we could also have this in the .env file or?
| .describe("The render ID returned from the `render_item` tool."), | ||
| }; | ||
|
|
||
| const Output = { |
There was a problem hiding this comment.
shouldn't this be same output as the render item? should be unifed imo?
There was a problem hiding this comment.
idk, more or less it is the same, why you think it should be exactly the same?
|
|
||
| // Handle successful completion | ||
| if (render.state === "DONE") { | ||
| return { |
There was a problem hiding this comment.
keep text, would also add that 👍
- uri to the video output
- name can be resolved from filename render stuff
- mime type known by content type (also available)
- description: link to a final video output
- no annotations
{
"type": "resource_link",
"uri": "file:///project/src/main.rs",
"name": "main.rs",
"description": "Primary application entry point",
"mimeType": "text/x-rust",
"annotations": {
"audience": ["assistant"],
"priority": 0.9
}
}
There was a problem hiding this comment.
I hardcoded these, but it always point to local file, see here
#3 (comment)
| return { | ||
| content: [], | ||
| structuredContent: { | ||
| message: "Render is processing. Please wait and check back later.", |
There was a problem hiding this comment.
maybe a time to wait before checking back. If throttled longest, queued & in-progress shorter?
There was a problem hiding this comment.
Client always needs a user input to be able to invoke tool, so you need to ask him if the render is finished, that's why I added render page URL so you can navigate and see the details there
| // Status information | ||
| message: z | ||
| .string() | ||
| .describe("A human-readable message for the render status."), |
There was a problem hiding this comment.
why is this better, than the state description in the tool?
There was a problem hiding this comment.
I am even thinking of getting state out of the output, because this message should be sufficient and combine some states in case of "processing"
| const output = { | ||
| renderId: render.id, | ||
| renderDetailsPageUrl: `https://app.test.plainlyvideos.com/dashboard/renders/${render.id}`, | ||
| renderDetailsPageUrl: `${PLAINLY_APP_URL}/dashboard/renders/${render.id}`, |
There was a problem hiding this comment.
why did not you add this as the resource link also?
There was a problem hiding this comment.
same reason as above, resource links are always pointing to local file with download icon.. so I guess it's used for a different usecase. Also, you can ask for render page url and you will get a nice link in the response, so not sure what are benefits of resources
ivansenic
left a comment
ivansenic
left a comment
There was a problem hiding this comment.
Let's fix that content and merge this, is there anything else needed?
Co-authored-by: Ivan Senic <senic.ivan@gmail.com>
2 participants
No description provided.