-
Notifications
You must be signed in to change notification settings - Fork 2.2k
✨enhancement: auto focus always allow action from tool approval dialog and add req parameters #5836
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
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.
Caution
Changes requested ❌
Reviewed everything up to df4b645 in 2 minutes and 5 seconds. Click for details.
- Reviewed
312
lines of code in17
files - Skipped
0
files when reviewing. - Skipped posting
7
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/components/ui/button.tsx:8
- Draft comment:
Ensure the custom focus classes (like 'focus:accent-[0px]') work as intended with your Tailwind configuration. Double-check that they produce the desired visual focus effect. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to double-check the behavior of custom focus classes, which violates the rule against asking for confirmation or double-checking. It does not provide a specific suggestion or point out a clear issue.
2. web-app/src/containers/ThreadContent.tsx:107
- Draft comment:
The removal of the explicit height ('h-3/4') on DialogContent may affect layout on some screens. Confirm that the responsive design remains as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm that the responsive design remains as intended after a change. This violates the rule against asking the author to confirm their intention or ensure behavior is intended.
3. web-app/src/containers/dialogs/ToolApproval.tsx:61
- Draft comment:
The new block for displaying toolParameters is a useful enhancement. If the parameters JSON becomes large or complex, consider additional formatting or a collapsible UI for readability. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%
<= threshold50%
The comment is purely informative, acknowledging the enhancement and suggesting a potential improvement for readability. However, it doesn't ask for a specific change or highlight a potential issue with the current implementation. It doesn't align with the rules for actionable feedback.
4. web-app/src/containers/dialogs/ToolApproval.tsx:96
- Draft comment:
AutoFocus on the 'Always Allow' button addresses the UX concern. Verify that this focus behavior does not conflict with other focus management within the dialog. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that the focus behavior does not conflict with other focus management, which is against the rules. It does not provide a specific suggestion or point out a specific issue.
5. web-app/src/hooks/useToolApproval.ts:55
- Draft comment:
The showApprovalModal function now accepts an optional toolParameters argument, which improves flexibility. The change looks correct. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, stating that a function now accepts an optional argument and that the change looks correct. It doesn't provide any actionable feedback or suggestions for improvement.
6. web-app/src/locales/en/tool-approval.json:11
- Draft comment:
The new 'parameters' translation key has been added consistently. Make sure similar updates are reflected in all locale files to maintain uniform UI language. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and suggests ensuring consistency across locale files, which violates the rule against asking the PR author to ensure something is done. It doesn't provide a specific code suggestion or point out a specific issue.
7. web-app/src/components/ui/button.tsx:23
- Draft comment:
Typo alert: In the default size variant, the class string contains "has-[>svg]:px-3". It looks like this might be a typo and perhaps should be "[&>svg]:px-3" to correctly target svg children? - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_nVyeuumz6w7sWTL7
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM
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.
Important
Looks good to me! 👍
Reviewed 2c8e2c1 in 58 seconds. Click for details.
- Reviewed
45
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/lib/completion.ts:332
- Draft comment:
The showModal parameter signature is reformatted into multiple lines. This change is stylistic and improves readability. Ensure that this formatting aligns with the project's style guidelines. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. web-app/src/lib/completion.ts:387
- Draft comment:
The code parses toolCall.function.arguments with a try-catch block earlier and stores the result in 'toolParameters', but then JSON.parse is called again when passing arguments to callTool. Consider reusing the parsed 'toolParameters' to avoid duplicate parsing and potential runtime errors if the JSON is malformed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_wz2cpHSrrfA1UUdt
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed acd1e5d in 1 minute and 24 seconds. Click for details.
- Reviewed
246
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/components/ui/__tests__/button.test.tsx:5
- Draft comment:
Optional: The import of '@testing-library/jest-dom' may be redundant if it's already included globally in your test setup. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. web-app/src/components/ui/__tests__/button.test.tsx:197
- Draft comment:
Verify that the focus-visible class names ('focus-visible:border-primary', 'focus-visible:ring-2', 'focus-visible:ring-primary/60') match your updated design tokens. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
3. web-app/src/components/ui/__tests__/button.test.tsx:67
- Draft comment:
Confirm that the expected class 'size-8' for the 'icon' variant is correct and intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. web-app/src/components/ui/__tests__/button.test.tsx:111
- Draft comment:
Consider also testing forwarded refs using React.createRef() to ensure the DOM node is correctly attached. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_c8gkMeDk8QI69kSS
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
This pull request introduces several changes across multiple files to enhance tool approval functionality, improve UI consistency, and update localization strings. The most significant changes include adding support for tool parameters in the tool approval modal, refining button focus styles, and updating translations for tool-related text across multiple languages.
Enhancements to Tool Approval Functionality:
ToolApproval
component anduseToolApproval
hook. Tool parameters are now passed to the modal and displayed if available. (web-app/src/containers/dialogs/ToolApproval.tsx
: [1] [2] [3];web-app/src/hooks/useToolApproval.ts
: [4] [5] [6] [7];web-app/src/lib/completion.ts
: [8] [9]UI Refinements:
button.tsx
to improve accessibility and visual consistency, including changes to focus-visible and focus states. (web-app/src/components/ui/button.tsx
: web-app/src/components/ui/button.tsxL8-R19)DialogContent
andTextarea
styles inThreadContent.tsx
to improve layout and responsiveness. (web-app/src/containers/ThreadContent.tsx
: web-app/src/containers/ThreadContent.tsxL107-R113)Localization Updates:
web-app/src/locales/*
: [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Fixes Issues
Self Checklist
Important
Enhance tool approval by adding tool parameter support, refining UI elements, and updating localization strings across multiple languages.
ToolApproval
component anduseToolApproval
hook.showApprovalModal
inuseToolApproval.ts
to accepttoolParameters
.postMessageProcessing
incompletion.ts
to handle tool parameters.button.tsx
for better accessibility.DialogContent
andTextarea
styles inThreadContent.tsx
for improved layout.This description was created by
for acd1e5d. You can customize this summary. It will automatically update as commits are pushed.