Skip to content
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

Show arguments in tool call response renderer #14424

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Show arguments in tool call response renderer #14424

merged 4 commits into from
Nov 27, 2024

Conversation

JonasHelming
Copy link
Contributor

fixed #14423

What it does

Shows a expandable ">" on tool functions in the hat if they have arguments

How to test

run a tool call and look at the chat. E.g.: "How to build this project".
Use a long-running to see the arguments while progress is shown, e.g. "how many files are in the workspace" on a big project such as theia

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming requested a review from planger November 8, 2024 23:50
fixed #14423

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to show the arguments! Thank you!

However, there are a few things I observed, which I'd like to clarify:

  • The arguments are shown again beneath the tool call result (see line 60). Is this on purpose? I found it a bit surprising:
    image

  • I think it'd be more clear that > is an expandable if we rather use a colored icon instead of the plain >, similar to the triangle we use for the entire tool call. The mouse pointer config doesn't really help a lot, because we already set this to pointer.

image

  • The JSON of the arguments is not really nicely formatted for me.

image

  • Just a matter of taste, and I don't have a strong opinion on this aspect, but I find those surrounding [ ... ] a bit superfluous. Imho there should be as few clutter as possible and it should be as close as possible to normal language without extra technical characters. Only if people are interested in looking into details, those technical details should appear. Which leads me also to the suggestion to show the arguments just as a section of the tool call result if the user unfolded them and not with the rather technical (>)? Background is also that this is likely to be used in non-IDE products.

@planger
Copy link
Contributor

planger commented Nov 11, 2024

Sorry, one more thing: It would probably be clearer if we move the renderCollapsibleArguments method below the method that calls (render) it and make it protected.

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
@JonasHelming
Copy link
Contributor Author

i decided to leav the "()" in. It looks very weird without them. In a non-technical context, I believe you would need a custom renderer anyways TBH.
A good simple test case is "Show me the package.json" it will have one call without and one with a parameter.

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
@JonasHelming
Copy link
Contributor Author

@planger Ping :-)

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision. However, for me this is now how it looks like (see line break after ...):

image

Also, if you don't put this in a pre element, the pretty printing of the JSON gets lost (see here).

@JonasHelming
Copy link
Contributor Author

JonasHelming commented Nov 21, 2024

Thanks for the revision. However, for me this is now how it looks like (see line break after ...):

image

This drives me mad. Somehow, I am almost certain, this was not the case when I committed this, and I literally do not find the reason for this. I will continue to search.

Also, if you don't put this in a pre element, the pretty printing of the JSON gets lost (see here).

This was actually on purpose. I you add pre then the argument is shown as multiple lines. However, I am fine with this, too.

@planger
Copy link
Contributor

planger commented Nov 21, 2024

@JonasHelming Thanks!

This was actually on purpose. I you add pre then the argument is shown as multiple lines. However, I am fine with this, too.

Ah ok, I was just wondering, because we stringify the JSON with 2 spaces (undefined, 2), so I expected the intention was to show it formatted across multiple lines. I'm fine with it either way.

@JonasHelming
Copy link
Contributor Author

@planger Looks wonderful now, thank you so much!

@planger planger self-requested a review November 27, 2024 07:51
Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me! 👍

@JonasHelming JonasHelming merged commit a6e788e into master Nov 27, 2024
11 checks passed
@github-actions github-actions bot added this to the 1.56.0 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Theia AI] Tool calls do not show arguments in the response renderer
2 participants