Connection hover #464
Closed
Connection hover #464
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Review this PR on mrge.io |
|
✨ No issues found! Your code is sparkling clean! ✨ 🗒️ View all ignored comments in this repo
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 54b8ff2 in 3 minutes and 54 seconds
More details
- Looked at
313lines of code in5files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. packages/ui-v1/components/CopyID.tsx:202
- Draft comment:
Avoid using hardcoded values (e.g., 'right-[9px]') for tooltip arrow positioning. Consider extracting into a constant for maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. packages/ui-v1/components/PropertyListView.tsx:47
- Draft comment:
Fixed max-width for CopyID elements may truncate longer IDs unexpectedly. Confirm that this is the desired behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. packages/ui-v1/domain-components/ConnectionsCardView.tsx:111
- Draft comment:
Including truncated ID values in the memo dependency array improves UI consistency. Verify that all derived values are covered. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
4. packages/ui-v1/domain-components/tables/ConnectionTableCell.tsx:80
- Draft comment:
HoverCard integration is neat. Consider reviewing the color contrast and accessibility of tooltip content for better usability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/app/console/(authenticated)/connections/client.tsx:19
- Draft comment:
Good use of computing authMethod. Confirm that all necessary props (and potential status) are correctly passed to ConnectionTableCell. - 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 all necessary props are correctly passed, which is similar to asking them to double-check their work. This violates the rule against asking the author to confirm their intention or ensure behavior is intended.
6. packages/ui-v1/components/CopyID.tsx:203
- Draft comment:
Review the new tooltip positioning (using 'right-0' and absolute right positioning for the arrow). Ensure alignment works across varying container widths. - 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 alignment works across varying container widths, which falls under the category of asking the author to double-check or ensure behavior. This violates the rules.
7. packages/ui-v1/components/PropertyListView.tsx:40
- Draft comment:
The layout now uses a fixed 'flex items-center justify-between overflow-visible' class. Verify that removing dynamic gap classes does not negatively affect spacing, especially for properties with CopyID. - 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 a change does not negatively affect spacing. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
8. packages/ui-v1/domain-components/tables/ConnectionTableCell.tsx:80
- Draft comment:
Default version inconsistency: ConnectionCardContent defaults to 'V2' but here a fallback of 'V1' is used. Consider aligning these defaults for consistency. - 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%
Without seeing the ConnectionCardContent component, we can't verify if 'V2' is actually its default. The comment makes an assumption about code we can't see. Even if true, this is a relatively minor consistency issue that doesn't affect functionality. The PR author likely intentionally chose V1 here for a reason.
I could be wrong about the impact - version inconsistencies could lead to subtle bugs. Also, the reviewer might have correctly checked the other component.
While version inconsistencies can be problematic, we need stronger evidence before suggesting changes. Without seeing the other component, this comment is too speculative.
Delete this comment as it makes assumptions about code we can't verify and doesn't show strong evidence for the suggested change.
Workflow ID: wflow_NFAOiqxmHLZsqcbD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
- Added a Customer ID to display inside the sheet of connection - Updated ConnectionsCardView to include a logo for each connector, with fallback initials on load failure. - Refactored ConnectionTableCell to use mouse events for displaying connection details in a hover-like manner, improving user experience.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4b09007 in 1 minute and 30 seconds
More details
- Looked at
76lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. packages/ui-v1/components/CopyID.tsx:188
- Draft comment:
Removed inline comment about custom tooltip. While cleaning up redundant comments is fine, consider including a brief note on why a custom tooltip is used (for Popover compatibility) to aid future maintainers. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. packages/ui-v1/domain-components/ConnectionsCardView.tsx:40
- Draft comment:
Multiple inline comments have been removed (e.g., property extraction, enhanced header, error handling fallbacks). Ensure that sufficient context remains in the code or documentation to explain key fallback behaviors (like using initials when an image fails to load). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. packages/ui-v1/components/CopyID.tsx:191
- Draft comment:
Removed custom tooltip comment. This cleanup is acceptable if tooltip behavior remains clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/ui-v1/domain-components/ConnectionsCardView.tsx:40
- Draft comment:
Removed several inline comments in variable declarations. The defaults are self-explanatory; ensure necessary context isn’t lost. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages/ui-v1/domain-components/ConnectionsCardView.tsx:137
- Draft comment:
Removed the 'Enhanced header with logo, name and status' comment in the render. The UI seems self-explanatory, so this removal is acceptable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. packages/ui-v1/domain-components/ConnectionsCardView.tsx:146
- Draft comment:
Removed inline comment in the Image onError handler. The fallback to display initials is clear from the code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. packages/ui-v1/domain-components/ConnectionsCardView.tsx:223
- Draft comment:
Removed the comment about creating a nicer trigger element with the connector logo. The refactored code remains clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. packages/ui-v1/domain-components/ConnectionsCardView.tsx:236
- Draft comment:
Removed redundant inline comment in the popover trigger Image onError handler. The fallback behavior remains understandable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_PuYune48PXuqUyaX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9bf1696 in 1 minute and 14 seconds
More details
- Looked at
156lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/ui-v1/components/PropertyListView.tsx:37
- Draft comment:
Consider using a unique identifier instead of the index as the key in the properties.map loop to prevent potential re-render issues. - 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%
While using index as key is generally not best practice in React, there are cases where it's acceptable. Here, the properties array appears to be a simple display list with no reordering, filtering, or dynamic updates. The items don't have an obvious unique identifier field. Making this change would require modifying the PropertyItem interface and all consumers of it.
The comment points out a real React best practice. In some edge cases with list reordering or dynamic updates, using index as key could cause issues.
However, in this simple display list case, using index as key is unlikely to cause actual problems. The suggested change would require significant interface changes for minimal benefit.
The comment should be deleted as it suggests a change that would require significant interface modifications for minimal practical benefit in this specific use case.
2. packages/ui-v1/domain-components/ConnectionsCardView.tsx:90
- Draft comment:
Removal of 'disableTooltip' and 'mountDelay' props from CopyID appears intentional. Confirm that this change in UX behavior is expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to confirm if the change in UX behavior is expected, which violates the rule against asking for confirmation of intention. However, it does point out a specific change in the code, which is the removal of certain props. The comment could be rephrased to suggest checking if the removal was intentional without asking for confirmation.
3. packages/ui-v1/domain-components/ConnectionsCardView.tsx:90
- Draft comment:
Updated property labels (e.g., 'Customer ID', 'Connector Config ID') improve readability. Ensure label conventions are consistent across the application. - 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 consistency, which is similar to asking them to double-check or verify something. It doesn't provide a specific suggestion or point out a specific issue in the code. Therefore, it violates the rules.
4. packages/ui-v1/components/PropertyListView.tsx:43
- Draft comment:
Consider using a stable unique key (e.g. property.title) instead of the index in the .map to avoid potential rendering issues if the array order changes. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. packages/ui-v1/domain-components/ConnectionsCardView.tsx:88
- Draft comment:
The removal of extra property items (Platform, Auth Method, Version) and props (disableTooltip, mountDelay) from CopyID components suggests a design change. Verify that this behavior aligns with the intended UX. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. packages/ui-v1/domain-components/ConnectionsCardView.tsx:153
- Draft comment:
Using PropertyListView with the modern prop simplifies the layout of properties. Ensure this new grid layout meets the overall design requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_lCseAJ4LEZ05h4E5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
openint-bot
requested changes
Apr 9, 2025
|
|
||
| interface ConnectionTableCellProps | ||
| extends React.HTMLAttributes<HTMLDivElement> { | ||
| connection: Core['connection_select'] | ||
| useLogo?: boolean | ||
| className?: string | ||
| logo_url?: string | ||
| status?: StatusType |
There was a problem hiding this comment.
why are we redefining / repeating this information these when we already have connection above?
| category?: string | ||
| platform?: string | ||
| authMethod?: string | ||
| version?: string | ||
| } | ||
|
|
||
| export function ConnectionTableCell({ |
There was a problem hiding this comment.
We are missing a story for this. Everything in ui-v1 should have corresponding stories. apps/web no need for stories.
As many components as possible should be in UI-v1, while balancing convenience and velocity.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d82fe87 in 1 minute and 22 seconds
More details
- Looked at
66lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:9
- Draft comment:
Consider extracting the decorator into a separately defined component for better reusability and maintainability. - 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%
While extracting decorators can be good practice for complex or reused decorators, this one is quite simple and specific to this story. It's just basic layout styling. The comment feels like it's suggesting a premature abstraction. There's no evidence that this decorator will be reused elsewhere or that its current inline form is causing any maintainability issues.
The comment could be valid if there's evidence that this decorator pattern is repeated across many story files, which would justify the abstraction.
However, without seeing other story files, we can't confirm if this is a repeated pattern. The decorator is simple enough that keeping it inline is perfectly maintainable.
This comment suggests a premature abstraction without clear evidence of benefit. The current inline decorator is simple and maintainable.
2. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:26
- Draft comment:
Check if using 'logo_url' (underscore) is consistent with your naming conventions; consider using camelCase if preferred. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:55
- Draft comment:
Consider providing examples for additional props (e.g., platform, version, authMethod) in the Default story for more comprehensive coverage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:26
- Draft comment:
Consider using camelCase (e.g., 'logoUrl') instead of snake_case ('logo_url') for prop names for consistency with JS conventions, if the underlying component supports it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:22
- Draft comment:
Consider providing a default value for 'useLogo' in the story args to explicitly showcase its behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. packages/ui-v1/domain-components/tables/ConnectionTableCell.stories.tsx:30
- Draft comment:
Consider adding additional stories to demonstrate different connection statuses (e.g., 'warning', 'offline', 'destructive') to provide better documentation and testing coverage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_0804oMDEt79bRBay
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 7ad1810 in 2 minutes and 8 seconds
More details
- Looked at
157lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/app/console/(authenticated)/connections/client.tsx:25
- Draft comment:
Remove commented-out props (logo_url, authMethod) if no longer needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/api-v1/routers/connection.models.ts:41
- Draft comment:
Resolve TODO: Ensure 'status' exists as expected in zConnectionExpanded schema. - 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 suggesting to remove a TODO comment, but it's not clear if this is the right action. The TODO itself seems to be newly added in this PR, suggesting it's an intentional reminder for future work. The comment doesn't provide any concrete guidance on how to ensure the status exists or why it needs to exist.
The TODO might be intentionally added as a reminder for future work. Without more context about the system requirements, we can't be sure if the status field should be required or optional.
While the TODO might be intentional, automated comments about TODOs generally don't add value unless they provide specific guidance on how to resolve them.
Delete the comment as it doesn't provide actionable guidance and the TODO appears to be intentionally added in this PR.
3. apps/web/app/console/(authenticated)/connections/client.tsx:25
- Draft comment:
Removed 'logo_url' and 'authMethod' props from ConnectionTableCell. Confirm the component now derives these values from the connection object internally. - 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%
This comment is asking the author to confirm something about internal component behavior. This violates the rule "Do NOT ask the PR author to confirm their intention or to explain things". The comment is not pointing out a clear issue that needs fixing - it's just asking for confirmation about an implementation detail.
Maybe this change could break the component if it's not properly set up to derive these values internally? Maybe this is a legitimate concern about a breaking change?
If there was a real issue with the component not being able to handle this change, it would be caught by TypeScript errors or tests. Asking for confirmation doesn't add value.
This comment should be deleted as it violates the rule against asking authors to confirm their intentions or explain their changes. It doesn't point out a clear issue that needs fixing.
4. packages/api-v1/routers/connection.models.ts:77
- Draft comment:
Remove the debug logging statement before production release. - 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/ui-v1/domain-components/tables/ConnectionTableCell.tsx:102
- Draft comment:
Avoid leaving commented-out code and TODO blocks. Either implement the ConnectionCardContent rendering or remove the commented block. - Reason this comment was not posted:
Marked as duplicate.
6. packages/ui-v1/domain-components/tables/ConnectionTableCell.tsx:101
- Draft comment:
Consider enhancing the status display instead of rendering the raw status. A styled component or proper tooltip might improve UX. - 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 shows a clear regression from a proper component to raw status display. There's already a TODO comment indicating this is temporary. The author seems aware this needs to be fixed and has assigned it to someone specific (@santi). The automated comment is redundant with the existing TODO.
The comment might be pointing out a legitimate UI issue that could affect user experience. The raw status display could be confusing or unclear to users.
While the UI concern is valid, there's already an explicit TODO comment addressing this exact issue, making this automated comment redundant and unnecessary.
Delete the comment since the issue is already acknowledged with a TODO and assigned to a specific developer.
Workflow ID: wflow_7xwb7AWRReyzsEg1
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.
| pointerEvents: 'auto', | ||
| }}> | ||
| <div className="bg-popover w-[480px] overflow-hidden rounded-md border p-0 shadow-md"> | ||
| {status} |
There was a problem hiding this comment.
Rendering raw status value; consider using a styled status component or complete ConnectionCardContent instead of directly rendering {status}.
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
Enhance
ConnectionTableCellwith hover popover, add modern styling toPropertyListView, and updateCopyIDtooltips.ConnectionTableCellinConnectionTableCell.tsxnow shows a popover with connection details on hover.PropertyListViewinPropertyListView.tsxsupports a modern styling option with uppercase labels and grid layout.CopyIDinCopyID.tsxhas improved tooltip positioning and styling.ConnectionExpandedtype inconnection.models.tsnow includes an optionalstatusfield.ConnectionTableCell.stories.tsxadded forConnectionTableCellcomponent.client.tsxandconnection.models.ts.This description was created by
for 7ad1810. It will automatically update as commits are pushed.