oint-1334 ConnectionCard and ConnectionStatus components #603
Conversation
…d Reconnect button functionality and status indicators. Add new story for Linear-inspired showcase and update fixtures for connection statuses.
Bug ReportName: Missing Storybook Story for "Unknown" Connection Status in export const Unknown: Story = {
args: { status: 'unknown' },
};Description: The Comments? Email us. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 5c48f0a in 55 seconds. Click for details.
- Reviewed
283lines of code in5files - Skipped
0files when reviewing. - Skipped posting
10draft 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. packages/ui-v1/domain-components/ConnectionCard.stories.tsx:45
- Draft comment:
Use Storybook actions instead of alert in onReconnect for better logging in stories. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/ui-v1/domain-components/ConnectionCard.stories.tsx:70
- Draft comment:
Added 'WithReconnectButton' story. Ensure the fixture status ('notion-basic') matches the expected reconnect behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/ui-v1/domain-components/ConnectionCard.stories.tsx:77
- Draft comment:
The LinearInspiredShowcase story nicely demonstrates multiple states. Verify the layout and labels are in line with design expectations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/ui-v1/domain-components/ConnectionCard.tsx:39
- Draft comment:
Destructuring pillColor alongside borderColor improves consistency. Confirm that pillColor is correctly applied in all status indicators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. packages/ui-v1/domain-components/ConnectionCard.tsx:66
- Draft comment:
Status indicator added at the top-left for non-healthy statuses. Ensure that other statuses display as intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/ui-v1/domain-components/ConnectionCard.tsx:92
- Draft comment:
Reconnect button now renders only when status is 'disconnected'. Verify this aligns with the intended user experience. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. packages/ui-v1/domain-components/ConnectionStatus.tsx:130
- Draft comment:
Improved reconnect button styling with an outline variant and animated RotateCcw icon; ensure the design specs are met. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. packages/ui-v1/domain-components/ConnectionStatusPill.stories.tsx:39
- Draft comment:
Added onClick handler for the 'disconnected' storyboard. This enhances testing of the reconnect action. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. packages/ui-v1/domain-components/__stories__/fixtures.ts:128
- Draft comment:
Fixtures now include explicit 'status' properties. Confirm that these statuses accurately reflect real connection states. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. packages/ui-v1/domain-components/ConnectionCard.tsx:76
- Draft comment:
It looks like there might be a typo in the class name "size-full" on line 76. Is "size-full" intended, or should it be "w-full" or "h-full" to correctly define the size? - 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_tFtm3NwLxItTmqJn
You can customize 
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| @@ -125,6 +125,7 @@ const connections = { | |||
| settings: {}, | |||
| disabled: false, | |||
| display_name: 'HubSpot Connection', | |||
| status: 'healthy' as const, | |||
There was a problem hiding this comment.
Adding 'status' field without proper type definition. The code is adding a 'status' property to objects that satisfy the ConnectionExpanded type, but there's no indication that this field is defined in the type. This will likely cause TypeScript type errors and could lead to runtime issues if the field is expected by other parts of the application. The status values ('healthy', 'error', 'disconnected', etc.) are being forcibly typed using 'as const' without proper validation against an expected enum or type definition.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 1 issue. Time to roll up your sleeves! 😱 🗒️ View all ignored comments in this repo
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Enhanced the ConnectionCard and ConnectionStatus components with improved reconnect functionality and status indicators, featuring a Linear-inspired design pattern.
- Added colored status indicator dot in top-left corner of
packages/ui-v1/domain-components/ConnectionCard.tsxfor non-healthy connections - Implemented rotating icon animation and outline variant for reconnect button in
packages/ui-v1/domain-components/ConnectionStatus.tsx - Added comprehensive
LinearInspiredShowcasestory inpackages/ui-v1/domain-components/ConnectionCard.stories.tsxdemonstrating various connection states - Streamlined UI by showing reconnect button only for disconnected status, removing always-visible status pill
- Added proper status fields to connection fixtures in
packages/ui-v1/domain-components/__stories__/fixtures.tsfor testing different states
5 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| export const ReconnectButton: Story = { | ||
| args: { | ||
| status: 'disconnected', | ||
| onClick: () => console.log('Reconnecting...'), |
There was a problem hiding this comment.
style: Using console.log for demo purposes. Consider using alert() for consistency with other stories or add a comment explaining the logging choice.
| {connection.status && connection.status !== 'healthy' && ( | ||
| <div className="absolute left-2 top-2 z-10"> |
There was a problem hiding this comment.
style: Double status check is redundant. connection.status !== 'healthy' implies connection.status exists
| {connection.status && connection.status !== 'healthy' && ( | |
| <div className="absolute left-2 top-2 z-10"> | |
| {connection.status !== 'healthy' && ( | |
| <div className="absolute left-2 top-2 z-10"> |
| {connection.status === 'disconnected' && ( | ||
| <div className="mt-2"> | ||
| <ConnectionStatusPill | ||
| status={connection.status} | ||
| onClick={onReconnect} | ||
| /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
logic: onReconnect prop might be undefined when status is disconnected. Add a check or make the prop required
| {connection.status === 'disconnected' && ( | |
| <div className="mt-2"> | |
| <ConnectionStatusPill | |
| status={connection.status} | |
| onClick={onReconnect} | |
| /> | |
| </div> | |
| )} | |
| {connection.status === 'disconnected' && onReconnect && ( | |
| <div className="mt-2"> | |
| <ConnectionStatusPill | |
| status={connection.status} | |
| onClick={onReconnect} | |
| /> | |
| </div> | |
| )} |
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={onClick} |
There was a problem hiding this comment.
logic: onClick handler is optional but no disabled state handling
| <CardContent | ||
| className="flex h-full flex-col items-center justify-center p-4 py-2" | ||
| onClick={onPress}> |
There was a problem hiding this comment.
logic: onClick handler on CardContent could trigger both onPress and onReconnect when clicking the reconnect button
| connection={FIXTURES.connections['google-drive-basic']} | ||
| onPress={() => {}} | ||
| /> |
There was a problem hiding this comment.
style: Empty onPress handler could be confusing. Consider removing it if not needed or adding a meaningful action.
| @@ -125,6 +125,7 @@ const connections = { | |||
| settings: {}, | |||
| disabled: false, | |||
| display_name: 'HubSpot Connection', | |||
| status: 'healthy' as const, | |||
| }, | |||
| 'hubspot-with-integration': { | |||
| id: 'conn_hubspot_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |||
There was a problem hiding this comment.
logic: Duplicate connection ID with 'hubspot-basic' (line 116)
| id: 'conn_hubspot_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |
| id: 'conn_hubspot_01HN4QZXG7YPBR8MXQT4KBWQ5P', |
| @@ -170,6 +173,7 @@ const connections = { | |||
| settings: {}, | |||
| disabled: false, | |||
| display_name: 'Notion Connection', | |||
| status: 'disconnected' as const, | |||
| }, | |||
| 'notion-with-integration': { | |||
| id: 'conn_notion_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |||
There was a problem hiding this comment.
logic: Duplicate connection ID with 'notion-basic' (line 164)
| id: 'conn_notion_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |
| id: 'conn_notion_01HN4QZXG7YPBR8MXQT4KBWQ5Q', |
| @@ -215,6 +221,7 @@ const connections = { | |||
| settings: {}, | |||
| disabled: false, | |||
| display_name: 'Google Drive Connection', | |||
| status: 'healthy' as const, | |||
| }, | |||
| 'google-drive-with-integration': { | |||
| id: 'conn_google-drive_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |||
There was a problem hiding this comment.
logic: Duplicate connection ID with 'google-drive-basic' (line 212)
| id: 'conn_google-drive_01HN4QZXG7YPBR8MXQT4KBWQ5N', | |
| id: 'conn_google-drive_01HN4QZXG7YPBR8MXQT4KBWQ5R', |
Stably Runner - Test Suite - 'Pre Merge CI Checks'Test Suite Run Result: 🟢 Success (2/2 tests passed) [dashboard] This comment was generated from stably-runner-action |
Bug Report
Comments? Email us. |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 6873218 in 1 minute and 23 seconds. Click for details.
- Reviewed
75lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft 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. packages/ui-v1/domain-components/ConnectionCard.tsx:46
- Draft comment:
Ensure that getConnectionStatusStyles returns consistent types for icon and message. If not, consider adding type-checks/memoization. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/ui-v1/domain-components/ConnectionCard.tsx:79
- Draft comment:
The condition for rendering the tooltip (connection.status && connection.status !== 'healthy') is repeated. Consider assigning it to a variable (e.g. isUnhealthy) to improve readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. packages/ui-v1/domain-components/ConnectionCard.tsx:80
- Draft comment:
Wrapping a single tooltip with TooltipProvider may be redundant. If the app already provides a TooltipProvider higher up, consider removing this one. - 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 the comment raises a valid point about potential redundancy, we don't have visibility into the broader app structure to know if there's a TooltipProvider higher up. The suggestion is speculative and requires knowledge outside this file. Following shadcn/ui patterns, having a TooltipProvider is a valid and safe approach even if potentially redundant. I might be too quick to dismiss this - redundant providers could impact performance. Also, following component library best practices is important. However, the potential performance impact is minimal, and having an extra provider is safer than assuming one exists. The comment requires too much external context to be actionable. The comment should be removed as it requires knowledge outside this file's scope to act on, and the current implementation is a safe pattern even if not optimal.
Workflow ID: wflow_tgoBTSfmRZnGXr0w
You can customize
| <StatusIcon | ||
| className={cn( | ||
| 'mt-0.5 h-4 w-4 flex-shrink-0', | ||
| connection.status === 'error' |
There was a problem hiding this comment.
The inline status icon color logic duplicates styling. Consider extracting status-to-color mapping to a helper for consistency.
Important
Add
ConnectionCardandConnectionStatusPillcomponents with enhanced UI for connection statuses and reconnect functionality, including new stories and updated fixtures.ConnectionCardcomponent with status indicator and reconnect button inConnectionCard.tsx.ConnectionStatusPillcomponent with tooltip and reconnect button inConnectionStatus.tsx.ConnectionCardinConnectionCard.stories.tsxshowcasing different connection states.ConnectionStatusPillinConnectionStatusPill.stories.tsxdemonstrating status and reconnect button.fixtures.tsto includehealthy,error,disconnected,manual, andunknown.This description was created by
for 6873218. You can customize this summary. It will automatically update as commits are pushed.