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.

Sign up for GitHub

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

Jump to bottom

Enhance: Add external tools from query to tool catalog #329

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Enhance: Add external tools from query to tool catalog #329

merged 1 commit into from
Aug 30, 2024

Conversation

ryanhopperlowe
Copy link
Contributor

This PR enhances the inline tool catalog

  • Shrinks tool icons to make items more compact
  • adds query to fetch tools from api and append them to list
  • improves focus logic to always forward necessary keystrokes to chat input
  • removes cli tools from featured tools list

njhale
njhale approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

Looking great so far. Thanks for taking this over!

Left a few comments. Feel free to address them in follow up PRs though.

(Note: seems like this needs a rebase. I'd also squash your commits s.t. we don't have any merge commits in the mix.)

components/chat/chatBar/search/catalog.tsx Outdated Show resolved Hide resolved
components/chat/chatBar/search/catalog.tsx Outdated Show resolved Hide resolved
components/chat/chatBar/search/catalog.tsx Outdated Show resolved Hide resolved
@StrongMonkey
Copy link
Contributor

@ryanhopperlowe I got this error when go to My Assistant page...do you get the same error?

2024-08-29T17:07:27.489Z [client] [ERROR] Uncaught Error: async/await is not yet supported in Client Components, only Server Components. This error is often caused by accidentally adding `'use client'` to a module that was originally written for the server.
2024-08-29T17:07:27.491Z [client] [ERROR] The above error occurred in the <Router> component:

    at Router (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:207:11)
    at ErrorBoundaryHandler (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:113:9)
    at ErrorBoundary (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:160:11)
    at AppRouter (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:577:13)
    at ServerRoot (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/app-index.js:112:27)
    at Root (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/app-index.js:117:11)

React will try to recreate this component tree from scratch using the error boundary you provided, ErrorBoundaryHandler.

@ryanhopperlowe ryanhopperlowe marked this pull request as draft August 29, 2024 20:59
@ryanhopperlowe
Copy link
Contributor Author

Converting to draft to make new changes

@ryanhopperlowe ryanhopperlowe marked this pull request as ready for review August 29, 2024 23:42
@ryanhopperlowe
Copy link
Contributor Author

@ryanhopperlowe I got this error when go to My Assistant page...do you get the same error?

2024-08-29T17:07:27.489Z [client] [ERROR] Uncaught Error: async/await is not yet supported in Client Components, only Server Components. This error is often caused by accidentally adding `'use client'` to a module that was originally written for the server.
2024-08-29T17:07:27.491Z [client] [ERROR] The above error occurred in the <Router> component:

    at Router (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:207:11)
    at ErrorBoundaryHandler (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:113:9)
    at ErrorBoundary (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/error-boundary.js:160:11)
    at AppRouter (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/components/app-router.js:577:13)
    at ServerRoot (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/app-index.js:112:27)
    at Root (webpack-internal:///(app-pages-browser)/./node_modules/next/dist/client/app-index.js:117:11)

React will try to recreate this component tree from scratch using the error boundary you provided, ErrorBoundaryHandler.

I'm not getting that error

StrongMonkey
StrongMonkey approved these changes Aug 30, 2024
View reviewed changes
components/chat/chatBar/search/catalog.tsx Outdated
},
],
'Working with Local Files': [
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to comment out instead of removing?

Copy link
Contributor Author

@ryanhopperlowe ryanhopperlowe Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

these items could be added back in in the near future, didn't want to create that much typing work if that was the case

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryanhopperlowe - I'm with @StrongMonkey here - go ahead and delete these. It is easy enough to pull these out of git history when and if we want to restore them.

I'll have to admit: commented out code is a bit of a pet peeve of mine.

@StrongMonkey
Copy link
Contributor

@ryanhopperlowe As long as it doesn't error out I am good. I was seeing the error when pulling your branch to do some smoke tests.

njhale
njhale reviewed Aug 30, 2024
View reviewed changes
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
StrongMonkey
StrongMonkey approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@StrongMonkey StrongMonkey left a comment

Choose a reason for hiding this comment

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

Synced offline, LGTM

cjellick
cjellick requested changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@cjellick cjellick left a comment

Choose a reason for hiding this comment

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

Just the one comment - id prefer the commented out code to get deleted.

WHen you merge this, make sure you squash your commits.

cjellick
cjellick approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@cjellick cjellick left a comment

Choose a reason for hiding this comment

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

I added the commit to remove the commented out code, thinking I'd then just merge it, but it needs rebased and I don't want to do that and potential break something. Please rebase and squash and merge first thing tomorrow

@cjellick cjellick mentioned this pull request Aug 30, 2024
Closed
@njhale njhale mentioned this pull request Aug 30, 2024
Closed
@ryanhopperlowe ryanhopperlowe force-pushed the enhance/dynamic-tool-add--rhl branch from a904bdb to c75dee4 Compare August 30, 2024 14:14
@ryanhopperlowe ryanhopperlowe force-pushed the enhance/dynamic-tool-add--rhl branch 2 times, most recently from 484f923 to 28f1a83 Compare August 30, 2024 15:28
@njhale @ryanhopperlowe
enhance: add better catalog search to chat bar
1276695 
Signed-off-by: Nick Hale <[email protected]>
Signed-off-by: Ryan Hopper-Lowe <[email protected]>
@ryanhopperlowe ryanhopperlowe force-pushed the enhance/dynamic-tool-add--rhl branch from 28f1a83 to 1276695 Compare August 30, 2024 15:36
@ryanhopperlowe ryanhopperlowe merged commit 2cdf28f into gptscript-ai:main Aug 30, 2024
1 check passed
@njhale njhale mentioned this pull request Aug 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhale njhale njhale approved these changes

@cjellick cjellick cjellick approved these changes

@StrongMonkey StrongMonkey StrongMonkey approved these changes

@thedadams thedadams thedadams approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ryanhopperlowe @StrongMonkey @cjellick @thedadams @njhale