Setup commands abstraction again and use it to handle connection delete #402
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hint in AppHeader
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
|
✨ No issues found! Your code is sparkling clean! ✨ 🗒️ View all ignored comments in this repo
|
openint-bot
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 413208e in 3 minutes and 29 seconds
More details
- Looked at
1263lines of code in31files - Skipped
1files when reviewing. - Skipped posting
42drafted comments based on config settings.
1. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:40
- Draft comment:
Consider checking for loading and error states for orgList.userMemberships.data before using it to build orgCommands. This can prevent issues when the data is not yet available. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already handles the null case safely with ??. Loading state would just mean no org commands temporarily which is fine. Error state would propagate up naturally. Adding explicit handling wouldn't improve the user experience meaningfully. The current implementation is appropriately defensive.
Maybe there are edge cases where showing a loading state or error message would improve UX? The error might be silent without explicit handling.
The command bar is a supplementary UI element - temporary absence of org commands during loading or errors is an acceptable degraded state. The ?? operator provides sufficient safety.
The comment should be removed as the code already handles the null case appropriately with ??, and explicit loading/error handling would add complexity without meaningful benefit.
2. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:90
- Draft comment:
The onSettled callback in useConnectionCommands contains commented-out code regarding setting query parameters. Consider updating or removing the TODO to clarify intended behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is pointing out a legitimate issue - there's unclear/incomplete code here. However, the comment doesn't provide any concrete suggestion for improvement. Without more context about the connection system and what connector_name means, we can't be sure what the right solution is. The comment is just restating what's obvious from the code comments.
The code comments might be intentionally left as a reminder for future work. Maybe the connector_name parameter isn't needed at all.
Even if intentional, having unclear TODOs and commented code is a code quality issue that should be addressed. However, the review comment doesn't add any value beyond what's already obvious from reading the code.
The comment should be deleted as it just restates what's obvious from the code comments without providing actionable guidance.
3. apps/web/app/(v1)/connect-v1/client.tsx:32
- Draft comment:
The console.log inside ModuleWrapper (line 32) is useful for debugging but should likely be removed or conditioned before production deployment. - 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.
4. apps/web/app/(v1)/connect-v1/page.tsx:85
- Draft comment:
There is a TODO about avoiding duplicate construction of input parameters for MyConnectionsClient. If known approaches exist, consider documenting or implementing them. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/web/vcommands/vcommand-definitions.ts:21
- Draft comment:
There is commented-out code for 'pipeline:create'. If this functionality is not needed, consider removing the commented code to improve clarity. - 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.
6. packages/ui-v1/components/command/command-components.tsx:97
- Draft comment:
The global keyboard shortcut registration in the CommandBar (lines 97-106) uses metaKey combined with 'p'/'k'. Verify that these shortcuts do not conflict with browser or OS-level shortcuts. - 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 shortcuts do not conflict with browser or OS-level shortcuts. This falls under asking the author to double-check things, which is against the rules.
7. packages/ui-v1/navigation/app-sidebar.tsx:75
- Draft comment:
The active state check for sidebar items relies on a simple string comparison. Consider using a more robust check (e.g. parsing URL segments) if your routing structure becomes more complex. - Reason this comment was not posted:
Comment was on unchanged code.
8. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:40
- Draft comment:
Consider handling the possibility that orgList.userMemberships.data is undefined explicitly and ensure error states are handled if organization details are missing. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. apps/web/app/(v1)/connect-v1/client.tsx:23
- Draft comment:
The parameter list for wrapConnectorClientModule ends with a trailing comma; consider removing it for consistency. - 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.
10. apps/web/app/(v1)/connect-v1/client.tsx:32
- Draft comment:
Several console.log statements are present; consider using a proper logging framework and removing debug logs in production. - 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.
11. apps/web/app/(v1)/connect-v1/page.tsx:85
- Draft comment:
There is a TODO about duplicate construction of input parameters; consider refactoring to improve maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
12. apps/web/app/(v1)/console/(authenticated)/layout.tsx:20
- Draft comment:
Wrapping the layout in GlobalCommandBarProvider is a good pattern; ensure that nesting contexts in async layouts doesn't affect performance. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a specific change doesn't affect performance, which is against the rules. It doesn't provide a specific suggestion or ask for a test to be written. It is more of a general cautionary note.
13. apps/web/package.json:20
- Draft comment:
Dependency on "@openint/commands" added; confirm that the workspace version is compatible with all consuming code. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm compatibility of a dependency, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
14. apps/web/vcommands/GlobalCommandBar.tsx:29
- Draft comment:
Check if window.CommandBar exists before using its API in the loop to avoid runtime errors. - 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.
15. apps/web/vcommands/vcommand-definitions.ts:29
- Draft comment:
Test conditionally included debug connection commands thoroughly to avoid exposure in production. - 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.
16. packages/api-v1/routers/connection.ts:236
- Draft comment:
The refresh logic casts connection as any; consider refactoring with a more precise type to ease future maintenance. - 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.
17. packages/api-v1/routers/connection.ts:299
- Draft comment:
Commented-out code for formatting connections is present; if obsolete, consider removing it to clean up the code. - Reason this comment was not posted:
Marked as duplicate.
18. packages/commands/command-types.ts:12
- Draft comment:
The CommandDraft type uses _infer defaulting to {}; ensure that command definitions include valid Zod schemas to avoid issues. - 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.
19. packages/shadcn/__stories__/command.stories.tsx:6
- Draft comment:
Consider disabling verbose console logs in Storybook examples to avoid cluttering the output. - Reason this comment was not posted:
Comment was on unchanged code.
20. packages/ui-v1/components/command/command-components.tsx:97
- Draft comment:
The global keyboard shortcut effect listens for meta+P or meta+K; ensure that shortcut conflicts are resolved and accessibility implications are evaluated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that shortcut conflicts are resolved and accessibility implications are evaluated. This falls under the category of asking the author to double-check things, which is against the rules.
21. packages/ui-v1/components/command/command-components.tsx:137
- Draft comment:
In CommandButton, ensure errors in cmd.execute do not leave the UI in an inconsistent state. - Reason this comment was not posted:
Comment was on unchanged code.
22. packages/ui-v1/components/command/command-components.tsx:217
- Draft comment:
Memoizing command grouping in CommandContent depends on initialParams and definitions; ensure they remain stable to prevent unnecessary re-renders. - 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.
23. packages/ui-v1/components/tabs/index.tsx:38
- Draft comment:
Consider adding aria-labels to TabsTrigger elements for improved accessibility. - 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.
24. packages/ui-v1/global.css:40
- Draft comment:
Ensure that the order of @source imports in global.css doesn't override necessary styles and is properly bundled in production. - 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.
25. packages/ui-v1/navigation/app-header.tsx:20
- Draft comment:
The title extraction from the pathname may fail for unexpected structures; consider using a robust utility function to handle edge cases. - 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.
26. packages/ui-v1/navigation/app-sidebar.tsx:75
- Draft comment:
Active link check compares trimmed URL strings; verify that this approach works correctly for nested routes and trailing slashes. - 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 the approach, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue.
27. packages/ui-v1/package.json:14
- Draft comment:
Ensure that dependency version ranges in package.json are compatible with the rest of the monorepo. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure compatibility of dependency version ranges, which falls under the rule of not asking the author to double-check or ensure compatibility. It does not provide a specific suggestion or point out a specific issue.
28. packages/ui/index.ts:2
- Draft comment:
The barrel file re-exporting components is straightforward; confirm that excluded files are purposefully left out for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm their intention regarding the exclusion of files in a barrel file. This violates the rule against asking the author to confirm their intention or to ensure the behavior is intended.
29. apps/web/package.json:75
- Draft comment:
There's a slight grammatical issue in the comment on line 75. Instead of "v10 try to import stuff from v11", you could consider rephrasing it for clarity, e.g., "v10 may attempt to import modules from v11, due to a pnpm quirk." This will make it more readable. - 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.
30. apps/web/vcommands/GlobalCommandBar.tsx:122
- Draft comment:
There's a minor typographical error in the comment on line 122: 'No-way to undo addRouter'. Consider changing it to 'No way to undo addRouter' for improved clarity. - 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.
31. apps/web/vcommands/vcommand-definitions.ts:54
- Draft comment:
In the description for the pipeline:delete command, consider adding a comma after 'However' for better readability (e.g., 'However, this will delete any incremental sync state...'). - 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.
32. apps/web/vcommands/vcommand-definitions.ts:100
- Draft comment:
In the description for the connection:delete command, consider adding a comma after 'However' to improve clarity (e.g., 'However, this will delete any incremental sync state...'). - 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.
33. apps/web/vcommands/vcommand-definitions.ts:77
- Draft comment:
Consider capitalizing 'SQL' in the title for the connection:navigate_sql command (i.e., change 'Run sql' to 'Run SQL') to better reflect common usage of the acronym. - 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.
34. packages/api-v1/routers/connection.ts:74
- Draft comment:
There is a minor typographical issue in the comment at lines 74-76. The phrase "other settings sensitive value stripping" is a bit awkward; consider rephrasing it for clarity, e.g., "NOTE: in the future, we should also strip other sensitive values (such as API keys) from the settings and abstract this logic." - 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.
35. packages/commands/command-fns.ts:75
- Draft comment:
Typographical suggestion: Consider capitalizing 'react' to 'React' in the comment on line 75 to correctly reference React hooks. - 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.
36. packages/commands/package.json:10
- Draft comment:
Typographical note: The dependency '@opensdks/util-zod' uses a different prefix compared to the other dependencies (@openint/*). Please verify if this is intentional or if it’s a typo. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
According to the rules, we should NOT comment on dependency changes or anything related to dependencies. The rules explicitly state to ignore dependency-related issues. Additionally, the comment is phrased as a verification request ("Please verify if this is intentional"), which is also explicitly discouraged.
Maybe the different prefix could indicate a serious architectural issue or a genuine mistake that needs attention.
The rules are very clear about not commenting on dependencies, and the comment's phrasing as a verification request makes it doubly problematic. Different dependency prefixes are common in JavaScript projects.
This comment should be deleted as it violates two rules: commenting on dependencies and asking for verification.
37. packages/shadcn/ui/command.tsx:49
- Draft comment:
There appears to be an extra '**:' at the beginning of the Command component's className (line 49). Please verify if this is intentional; if not, consider removing it. - 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.
38. packages/ui-v1/components/DataTable.tsx:72
- Draft comment:
The error message in the useDataTableContext hook says 'useDataTable must be used within a DataTableProvider'. Given that the hook is named 'useDataTableContext', it would be clearer to update the error message to 'useDataTableContext must be used within a DataTableProvider'. - 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.
39. packages/ui-v1/components/DataTable.tsx:190
- Draft comment:
The class 'size-full' in the div on line 190 might be a typo. If the intention was to make the div take full width, consider using 'w-full' (or the appropriate Tailwind class) instead. - 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.
40. packages/ui-v1/components/command/command-components.tsx:86
- Draft comment:
There is a minor grammatical issue in the comment above the CommandBar function. The line currently reads: 'Automatically registers keyboard shortcut and show command in a dialog'. Consider changing it to: 'Automatically registers the keyboard shortcut and shows the command dialog' to improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The rules state we should not make purely informative comments. While this is a real grammatical issue, it's a very minor documentation change that doesn't affect functionality. The current wording is understandable even if not perfect. This seems like the kind of nitpicky comment we want to avoid.
The grammar could genuinely make the docs clearer for non-native English speakers. Bad documentation can be a real issue.
While documentation clarity is important, this is an extremely minor improvement that doesn't meaningfully impact understanding. We should focus on more substantial issues.
Delete this comment as it's too minor and purely informative without requiring any real code changes.
41. packages/ui-v1/global.css:44
- Draft comment:
Typo: "simplied" should be changed to "simplified". This typo appears in the comment (line 44) and in the CSS selector (::simplied at line 47). - 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.
42. packages/ui-v1/global.css:193
- Draft comment:
Typo: "eason" should be corrected to "reason" in the comment on line 193. - 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_DImhLCi0QEzKGiFv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| ), | ||
| ), | ||
| ), | ||
| // items: await Promise.all( |
There was a problem hiding this comment.
The formatting of the connection objects uses a commented-out section with Promise.all. Clean up or finalize this code to avoid leaving dead/commented code in production.
| .string() | ||
| .describe('The ID of the connection to delete'), | ||
| }), | ||
| execute: async ({params}) => { |
There was a problem hiding this comment.
The useConnectionCommands function uses a bare window.confirm for deletion confirmation; consider using a custom modal for improved UX.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 42ef960 in 1 minute and 48 seconds
More details
- Looked at
93lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:74
- Draft comment:
Removed placeholder comment is fine, but consider adding a brief note on the grouping of connection commands for future clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/app/(v1)/console/(authenticated)/layout.tsx:21
- Draft comment:
Wrapping AppLayout in ClientApp makes sense. Ensure passing token this way is secure and that defaulting to empty string is acceptable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/web/app/(v1)/console/(authenticated)/page.tsx:84
- Draft comment:
Removing the ClientApp wrapper around AddConnection and ConnectionList may affect client-only hooks. Confirm this change is intended to shift these components to server-side rendering. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid concern about client-side hooks, but it's speculative and asks for confirmation. The presence of server components and continued use of initialData props suggests this was likely an intentional architectural change. The comment violates the rule about not asking authors to confirm their intentions.
The components might actually break without ClientApp wrapper if they use client-side hooks. We don't have access to the component implementations to verify.
Even if there could be issues, asking for confirmation violates our rules. If there were hook issues, they would be caught immediately during testing or build.
Delete the comment as it violates the rule about asking authors to confirm their intentions, and any real issues would be caught during testing/build.
4. apps/web/app/(v1)/console/(authenticated)/page.tsx:100
- Draft comment:
Rendering both client and server versions of AddConnection and ConnectionList in the same page could be confusing. Verify if this duplication is intentional. - 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.
5. apps/web/app/(v1)/console/(authenticated)/layout.tsx:14
- Draft comment:
Avoid leaving console.log with potentially sensitive viewer data in production. Consider removing or gating the debug log. - Reason this comment was not posted:
Comment was on unchanged code.
6. apps/web/app/(v1)/console/(authenticated)/page.tsx:63
- Draft comment:
The withRandomDelay function comment indicates a 0–2 sec delay, but the implementation uses Math.random()*4000 (0–4 sec). Align the delay multiplier to match the comment or update the comment. - 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.
7. apps/web/app/(v1)/console/(authenticated)/page.tsx:84
- Draft comment:
Removal of the wrapper for AddConnection and ConnectionList may affect client-only behavior. Also, the duplicate sections labeled 'AddConnection' and 'ConnectionList' might confuse users—ensure this duplication is intentional and clearly differentiated. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment has two parts: 1) ClientApp wrapper removal and 2) duplicate sections. For the ClientApp concern, without seeing the ClientApp component code or understanding its purpose, I can't verify if its removal is problematic. For the duplicates, they appear intentional as they're clearly separated and use different components (client vs server versions).
I don't have visibility into the ClientApp component's purpose or implementation. The duplicates could be confusing to users if there's no visual distinction beyond hr tags.
However, the duplicate sections use different components (client vs server) and are visually separated. This appears to be an intentional comparison or testing setup.
The comment should be deleted as it makes assumptions about ClientApp's importance without evidence, and the duplication appears intentional with clear separation between client/server versions.
Workflow ID: wflow_c01gY3lpVguzhn8e
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dd3a7eb in 1 minute and 53 seconds
More details
- Looked at
192lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:145
- Draft comment:
Remove or handle the commented-out TRPC mutations in 'connection:check' and 'connection:refresh' commands to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
2. apps/web/app/(v1)/console/(authenticated)/connections/client.tsx:81
- Draft comment:
deleteConn is included in the dependency array for columnsWithActions even though it isn’t used inside the cell rendering. Consider removing it to avoid unnecessary recomputation. - 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.
3. apps/web/app/(v1)/connect-v1/client.tsx:15
- Draft comment:
The order of props in CommandPopover was adjusted; ensure that ctx and initialParams are correctly passed as intended. - 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.
4. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:112
- Draft comment:
Using native window.confirm and alert() for user feedback can lead to a suboptimal UX. Consider replacing these with in-app modals or toast notifications for consistency. - 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.
5. apps/web/app/(v1)/console/(authenticated)/connections/client.tsx:98
- Draft comment:
The useMemo dependency array includes 'deleteConn', but this mutation is no longer used in rendering (replaced by CommandPopover). Consider removing it from dependencies to avoid unnecessary recomputations. - 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_p0uWC8Xmkj33OMvJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| connection_id: z.string().describe('The ID of the connection to copy'), | ||
| }), | ||
| execute: async ({params}) => { | ||
| await navigator.clipboard.writeText(params.connection_id) |
There was a problem hiding this comment.
Consider adding error handling for clipboard operations in 'connection:copyId'. If navigator.clipboard.writeText fails, the user won't get any feedback.
| }), | ||
| execute: async ({params}) => { | ||
| try { | ||
| // await trpc.checkConnection.mutate({id: params.connection_id}) |
There was a problem hiding this comment.
The 'connection:check' and 'connection:refresh' commands contain commented-out TRPC mutation calls and rely solely on alert() for feedback. Consider implementing proper backend calls and using a unified, non-blocking notification system instead of alerts.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 55c04a6 in 1 minute and 48 seconds
More details
- Looked at
110lines of code in6files - Skipped
1files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:148
- Draft comment:
Removed destructuring of params in execute for 'connection:check'. If API calls are re-enabled later, include the params in the signature. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is informative, explaining a change that was made and suggesting a future action if a certain condition occurs. It doesn't ask for confirmation or suggest a specific code change. It seems to be more of a note for future reference rather than a useful comment for the current PR.
2. apps/web/app/(v1)/GlobalCommandBarProvider.tsx:163
- Draft comment:
Same note for 'connection:refresh': removed params destructuring. Ensure this change is intentional if you plan to use connection_id later. - 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.
3. apps/web/app/(v1)/console/(authenticated)/connections/client.tsx:80
- Draft comment:
The dependency array for useMemo includes 'deleteConn' which is not used inside the memoized value. Consider removing or updating dependencies. - 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.
4. apps/web/app/(v1)/console/(authenticated)/connections/client.tsx:98
- Draft comment:
The dependency array for the useMemo hook includes 'deleteConn', which is not used within the memoized callback. Remove it if unnecessary to avoid unintended re-renders. - 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.
5. packages/commands/command-types.ts:2
- Draft comment:
The hack import for 'IconName' from a relative path suggests a dependency coupling issue. Consider refactoring the dependency structure to import 'IconName' from the appropriate package. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is addressing a real issue that was just introduced in the diff. The author already acknowledges this is a hack with their comment. The automated comment essentially just restates what the author already knows and documented. The comment doesn't provide any specific guidance on how to fix it beyond what's already acknowledged.
The comment might be pointing out an important architectural issue that needs addressing. Bad dependency management can lead to maintenance problems.
While the issue is real, the author is already aware of it as evidenced by their "Hack alert" comment. The automated comment adds no new information or specific solution.
Delete the comment since it only restates what the author has already acknowledged in their own comment without providing additional actionable guidance.
6. packages/commands/package.json:10
- Draft comment:
The removal of '@openint/ui-v1' from devDependencies appears intentional. Please double-check that this change does not negatively impact development, testing, or build processes that may rely on that package. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment asks the PR author to double-check the impact of removing a dependency, which violates the rule against asking for confirmation or ensuring behavior. It does not provide a specific suggestion or point out a specific issue.
7. apps/web/app/(v1)/console/(authenticated)/page.tsx:64
- Draft comment:
Typo in the comment: It currently says 'Random delay between 0-2 seconds' even though the code uses 'Math.random() * 4000' (implying up to 4 seconds). Consider updating the comment to match the code if 4000ms (4 seconds) is intended, or adjust the multiplier if the intended delay is 2 seconds. - 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_cu4ieIO47fuyuKN3
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| params: z.object({ | ||
| connection_id: z.string().describe('The ID of the connection to check'), | ||
| }), | ||
| execute: () => { |
There was a problem hiding this comment.
The 'connection:check' and 'connection:refresh' commands no longer destructure and use the 'params' (e.g. connection_id). This may cause issues when a command is invoked with parameters. Consider restoring parameter usage.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 321292d in 42 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. packages/ui-v1/components/WebhookInput.tsx:1
- Draft comment:
Good inclusion of the 'use client' directive to ensure client-side rendering for this component. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/ui-v1/components/WebhookInput.tsx:1
- Draft comment:
The 'use client' directive is now added, which is correct given that the component uses client-side hooks and browser APIs. Make sure this client-only behavior is intentional. - 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 their intention regarding the 'use client' directive. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
Workflow ID: wflow_HIffjDXXCNqxIQW3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
pellicceama
pushed a commit
that referenced
this pull request
Jul 16, 2025
…te (#402) * Adding separate commands package * Popver, Dialog and inline variants for commands * Test out with keyboard shortcuts * Command components story ready * Remove previous ui-v0 commands * Commands to switch organization * Navigate command also * Introduce CommandContext and use it for the cmd+k hint in AppHeader * move command into its own file * Use command button inside connect also * hideGroupHeadings and initialParams, CommandButton later * Fixing global commands provider hierarchy * Add scaffolding for a bunch more connection actions * Fix types and build * use client
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces a command abstraction layer with
GlobalCommandBarProviderand integrates command handling into the UI usingCommandPopoverfor enhanced interaction.GlobalCommandBarProviderinGlobalCommandBarProvider.tsxto manage command definitions and context.useCommandDefinitionMapto define organization, navigation, and connection commands.useConnectionCommandsfor connection-related actions like delete, copy ID, explore, check, and refresh.CommandPopoverinclient.tsxandconnections/client.tsxfor connection management.AppLayoutwithGlobalCommandBarProviderinlayout.tsxto provide command context.page.tsxto useGlobalCommandBarProviderfor command handling in the console.@openint/commandspackage topackage.jsonfor command handling.packages/commandsand updates imports accordingly.ui-v1components to use new command abstractions and UI elements.This description was created by
for 321292d. It will automatically update as commits are pushed.