OINT-1289: WIP bringing back modal add #561
Conversation
Bug Report
Comments? Email us. |
|
CodeAnt AI is reviewing your PR. |
|
Your mrge subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use mrge. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
codeant-ai
bot
added
the
size:L
|
✨ No issues found! Your code is sparkling clean! ✨ 🗒️ View all ignored comments in this repo
|
Pull Request Feedback 🔍
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR transitions from a tabbed interface to a modal-based approach for managing connections, with significant UI/UX changes across multiple components.
- Removes problematic
TabsClientcomponent that caused server-side route reloads, replacing with modal navigation - Improves empty state UI in
MyConnections.client.tsxwith better messaging and responsive grid layout (2 cols mobile, 4 desktop) - Potential issue with hardcoded
auth_type: 'OAUTH2'type assertion inMyConnections.client.tsxthat could cause runtime errors - Removal of
Spinnercomponent and stories without clear replacement path needs attention to prevent breaking loading states - Export changes in domain-components/index.ts require migration plan to prevent breaking existing imports
10 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| return ( | ||
| <ConnectorConnectContainer | ||
| connectorName={conn.connector_name as ConnectorName} | ||
| connector={{auth_type: 'OAUTH2'} as any} |
There was a problem hiding this comment.
logic: Hardcoding auth_type as 'OAUTH2' and using type assertion is unsafe. Should handle different auth types properly.
| setIsLoading(true) | ||
| setSearchParams({view: 'add'}, {shallow: false}) | ||
| }} |
There was a problem hiding this comment.
style: Setting loading state before navigation is unnecessary since component will unmount. Remove setIsLoading(true).
| setIsLoading(true) | |
| setSearchParams({view: 'add'}, {shallow: false}) | |
| }} | |
| setSearchParams({view: 'add'}, {shallow: false}) | |
| }} |
| const [isOpen, setIsOpen] = useState(true) | ||
| const [_, setSearchParams] = useMutableSearchParams() |
There was a problem hiding this comment.
logic: isOpen is initialized to true but there's no way to reopen the modal if closed. Consider making this controlled by parent component.
| onPress={() => { | ||
| if (isConnecting) { | ||
| return | ||
| } | ||
| handleSelect() | ||
| handleConnect() | ||
| }} |
There was a problem hiding this comment.
style: handleSelect() and handleConnect() could race if both modify state. Consider combining into single handler.
| </Button> | ||
| </div> | ||
|
|
||
| <div className="mt-4"></div> |
There was a problem hiding this comment.
style: Empty div with mt-4 can be replaced by adding margin to DataTileView component.
![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 a1a6635 in 2 minutes and 52 seconds. Click for details.
- Reviewed
978lines of code in10files - Skipped
0files when reviewing. - Skipped posting
16draft 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. apps/web/app/connect/AddConnection.client.tsx:102
- Draft comment:
Consider using a more unique key than${card.connectorName}-${index}to avoid key collisions when duplicate connector names occur. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. apps/web/app/connect/AddConnection.client.tsx:93
- Draft comment:
The onScroll handler updates isFocused based on scrollTop; consider debouncing or throttling for performance on long lists. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. apps/web/app/connect/MyConnections.client.tsx:26
- Draft comment:
The local isLoading state appears to be manually toggled; consider relying more on Suspense or ensuring consistent loading state management. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. apps/web/app/connect/page.tsx:87
- Draft comment:
Overriding searchParams with payload.connect_options is powerful; ensure that the keys being overridden are safe and expected. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
5. apps/web/app/console/(authenticated)/connect/page.client.tsx:75
- Draft comment:
Error handling in ConnectEmbedPreview could be enhanced to provide more detailed user feedback for errors. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. apps/web/app/connect/AddConnection.client.tsx:93
- Draft comment:
Consider debouncing the onScroll event to avoid potential performance issues during rapid scrolling. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. apps/web/app/connect/AddConnection.client.tsx:155
- Draft comment:
Duplicated rendering logic for mobile and desktop ConnectorDisplay; consider abstracting the shared logic to reduce duplication. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
8. apps/web/app/connect/MyConnections.client.tsx:26
- Draft comment:
Local isLoading state appears redundant given the use of Suspense. Consider relying on Suspense for managing loading state to simplify the component. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
9. apps/web/app/connect/page.tsx:117
- Draft comment:
Sanitize themeVariables values before injecting them via dangerouslySetInnerHTML to mitigate potential XSS vulnerabilities. - 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. packages/ui-v1/components/index.ts:11
- Draft comment:
Removed export of Spinner; ensure that all consumers now use LoadingSpinner instead. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
11. packages/ui-v1/domain-components/index.ts:7
- Draft comment:
Removed LinkConnectorModal export. Confirm that no parts of the codebase depend on this component. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
12. apps/web/app/console/(authenticated)/connect/page.client.tsx:11
- Draft comment:
Good update: replaced Spinner with LoadingSpinner for consistent UX. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. apps/web/app/connect/AddConnection.client.tsx:155
- Draft comment:
There is a typographical error in the comment on line 155: 'note sure' should likely be 'not sure'. - 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% While the typo fix is technically correct, fixing typos in comments (especially TODO comments) provides very little value. The meaning is still clear despite the typo. This kind of nitpicking can create noise in PR reviews and distract from more important issues. The rules state to only keep comments that require clear code changes. The typo could theoretically make the comment slightly harder to read or search for. Documentation quality is important. The meaning is still perfectly clear despite the typo, and this is just a temporary TODO comment, not user-facing documentation. The value add is minimal. Delete this comment as it points out a trivial typo in a comment that doesn't impact code functionality or readability in any meaningful way.
14. apps/web/app/connect/page.tsx:338
- Draft comment:
Typo found: 'IMprove' should be 'Improve' in the comment. Please fix the capitalization. - 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/app/connect/page.tsx:287
- Draft comment:
Typo found: 'crease new connections' should be 'create new connections' in the TODO comment. Please update this. - 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. apps/web/app/console/(authenticated)/connect/page.client.tsx:59
- Draft comment:
Typographical error: The word 'stragiht' should be corrected to 'straight' for 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.
Workflow ID: wflow_I8efkna385yLZxE4
You can customize 
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| <Button | ||
| variant="default" | ||
| onClick={() => setSearchParams({view: 'add'}, {shallow: true})}> | ||
| Add your first integration | ||
| size="lg" | ||
| className="font-medium" | ||
| onClick={() => { | ||
| setIsLoading(true) | ||
| setSearchParams({view: 'add'}, {shallow: false}) | ||
| }} | ||
| disabled={isLoading}> | ||
| Add First Integration | ||
| </Button> |
There was a problem hiding this comment.
Suggestion: Declare and initialize the isLoading state to avoid runtime errors. [possible bug]
| <Button | |
| variant="default" | |
| onClick={() => setSearchParams({view: 'add'}, {shallow: true})}> | |
| Add your first integration | |
| size="lg" | |
| className="font-medium" | |
| onClick={() => { | |
| setIsLoading(true) | |
| setSearchParams({view: 'add'}, {shallow: false}) | |
| }} | |
| disabled={isLoading}> | |
| Add First Integration | |
| </Button> | |
| const [isLoading, setIsLoading] = useState(false) | |
| ... | |
| <Button | |
| size="lg" | |
| className="font-medium" | |
| onClick={() => { | |
| setIsLoading(true) | |
| setSearchParams({ view: 'add' }, { shallow: false }) | |
| }} | |
| disabled={isLoading}> | |
| Add First Integration | |
| </Button> |
|
CodeAnt AI finished reviewing your PR. |
Rodri77
force-pushed
the
restore-modal-work
branch
from
a1a6635 to
49a8c77
Compare
Bug Report
Comments? Email us. |
Bug ReportName: Unnecessary Full Page Refresh on Modal Close Comments? Email us. |
Rodri77
force-pushed
the
restore-modal-work
branch
from
da08688 to
39c6239
Compare
Bug ReportFixed setSearchParams in AddConnectionClient to use shallow routing.
Removed Spinner.stories.tsx file.
Comments? Email us. |
There was a problem hiding this comment.
PR Summary
Implemented modal-based connection management with improved UI/UX and loading states.
- Added responsive grid layout in
AddConnection.client.tsxwith mobile (1 col) and desktop (2 col) views - Improved search functionality with dynamic filtering and focus states in modal header
- Implemented empty state handling with clear messaging when no integrations match search
- Consolidated loading states using
LoadingSpinneracross all components for consistency - Added proper error boundary handling with fallback states in
AddConnectionCardPrefetch
10 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| {/* NOTE/TODO: note sure how to remove duplication, | ||
| its there as we're using tailwind for consistent breakpoints with AddConnectionClient */} |
There was a problem hiding this comment.
style: Consider extracting common ConnectorDisplay logic into a shared component to avoid duplication between mobile and desktop views
| {filteredCards.map((card, index) => ( | ||
| <div key={`${card.connectorName}-${index}`}> |
There was a problem hiding this comment.
style: Using array index as part of key prop could cause issues with React's reconciliation if cards are reordered
| {filteredCards.map((card, index) => ( | |
| <div key={`${card.connectorName}-${index}`}> | |
| {filteredCards.map((card) => ( | |
| <div key={card.connectorName}> |
Rodri77
changed the title
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 39c6239 in 2 minutes and 42 seconds. Click for details.
- Reviewed
965lines of code in10files - Skipped
0files when reviewing. - Skipped posting
7draft 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. apps/web/app/connect/AddConnection.client.tsx:59
- Draft comment:
The onOpenChange callback only handles the false (closed) case. Ensure this behavior is intentional or add handling/documentation for when the dialog is opened. - 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 dialog's open state is fully controlled by the isOpen state variable. The onOpenChange handler only needs to handle closing because opening is handled by the isOpen state which is set to true initially. There's no need to handle the open case in onOpenChange because the component controls when the dialog opens through isOpen=true. This is a valid pattern in React for controlled components. I could be wrong about whether there are edge cases where the dialog could be opened through other means that should be handled. The component might be part of a larger system I don't fully understand. The code shows this is a controlled component where opening is handled by the isOpen state. The onOpenChange handler only needs to handle closing since that's the only external event that needs to update the internal state. The comment should be deleted because the current implementation is a valid controlled component pattern and there's no clear issue with only handling the close case in onOpenChange.
2. apps/web/app/connect/MyConnections.client.tsx:26
- Draft comment:
The local isLoading state is set to true and immediately set to false in useEffect. Consider removing this redundant state in favor of relying on the suspense query’s loading 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.
3. apps/web/app/connect/page.tsx:291
- Draft comment:
When mapping available connectors for cards, consider using a unique identifier (e.g., ccfg.id) instead of the index to generate keys. This helps maintain stability if the list order changes. - Reason this comment was not posted:
Comment was on unchanged code.
4. apps/web/app/console/(authenticated)/connect/page.client.tsx:165
- Draft comment:
For improved accessibility, consider adding ARIA labels or descriptive text to interactive elements like the link containing the ChevronLeftIcon. - 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/components/index.ts:11
- Draft comment:
Good cleanup—the removal of the redundant Spinner export is appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. packages/ui-v1/domain-components/index.ts:1
- Draft comment:
Removal of the LinkConnectorModal export from the domain-components index looks good; ensure this component isn’t referenced elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. apps/web/app/connect/AddConnection.client.tsx:150
- Draft comment:
Typographical error in comment: 'note sure how to remove duplication, its there as we're using tailwind for consistent breakpoints with AddConnectionClient'. Consider changing to "not sure" instead of "note sure" and "it's there" instead of "its there". - 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% While the comment is technically correct about the typos, it's addressing a TODO comment that is meant to be temporary and informal. TODOs are typically internal notes for developers and don't need to meet the same quality bar as production code. The typos don't impact functionality or code quality. The typos could make the TODO comment slightly less clear to other developers. Poor writing in comments, even TODOs, could be seen as reducing code quality. The meaning of the TODO is still clear despite the typos, and focusing on spelling in temporary developer notes is overly pedantic and doesn't add meaningful value. This comment should be deleted as it's overly focused on minor typos in a temporary TODO comment rather than substantive code issues.
Workflow ID: wflow_V9muR5yaAcLQSJk3
You can customize
| {/* NOTE/TODO: note sure how to remove duplication, | ||
| its there as we're using tailwind for consistent breakpoints with AddConnectionClient */} | ||
| {/* Mobile view (visible below md breakpoint) */} | ||
| <div className="md:hidden"> |
There was a problem hiding this comment.
Consider extracting the duplicated rendering logic for ConnectorDisplay in the mobile (lines 150–170) and desktop (lines 172–190) views into a shared component.
…to restore-modal-work
Bug ReportName|Severity|Example test case|Description\n---|---|---|---\nLosing connector_names filter on modal close|Medium|1. Navigate to connect?connector_names=stripe. 2. Click 'Add Integration'. 3. Close the modal. 4. Observe that all connectors are displayed, not just Stripe.|When the Add Integration modal is closed, the connector_names filter is lost, causing all connections to be displayed in the Manage Integrations view. Comments? Email us. |
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 |
User description
Important
This PR introduces a modal-based integration addition flow, refactors the main connect page, and removes deprecated components, enhancing the UI and code maintainability.
AddConnectionClientcomponent for modal-based integration addition with search and filtering inAddConnection.client.tsx.page.tsxto use conditional rendering instead of tab-based navigation.MyConnections.client.tsxwith new empty state, grid layout, andLoadingSpinner.SpinnerandLinkConnectorModalcomponents.LinkConnectorModalstories.SpinnerwithLoadingSpinnerinpage.client.tsxandMyConnections.client.tsx.This description was created by
for 39c6239. You can customize this summary. It will automatically update as commits are pushed.
CodeAnt-AI Description
AddConnectionClientcomponent, featuring search, filtering, and responsive layouts.viewsearch parameter.LoadingSpinner.Spinner,LinkConnectorModal) and their exports and stories, cleaning up unused code.This PR modernizes the integration management UI by introducing a modal for adding integrations, improving the user experience and code maintainability. It also removes outdated components and streamlines the main connect page logic, resulting in a cleaner and more intuitive interface.
Changes walkthrough
9 files
AddConnection.client.tsx
Add modal-based integration addition and refactor connection card UIapps/web/app/connect/AddConnection.client.tsx
AddConnectionClientcomponent to handle modal-basedintegration addition with search/filter and responsive layout.
AddConnectionCardto useConnectorDisplayfor consistent UIand improved selection logic.
modal and search.
MyConnections.client.tsx
Improve integrations list UI and loading/empty statesapps/web/app/connect/MyConnections.client.tsx
SpinnerwithLoadingSpinnerfor loading states.connections.
page.client.tsx
Remove tab-based navigation and related client logicapps/web/app/connect/page.client.tsx
TabsClientcomponent and related tab navigation logic.page.tsx
Refactor connect page to use modal and conditional renderingapps/web/app/connect/page.tsx
viewsearch param.
AddConnectionClientmodal for adding integrations.Spinner.tsx
Remove deprecated Spinner componentpackages/ui-v1/components/Spinner.tsx
Spinnercomponent as it is no longer used.index.ts
Remove Spinner export from components indexpackages/ui-v1/components/index.ts
Spinnercomponent.LinkConnectorModal.tsx
Remove LinkConnectorModal componentpackages/ui-v1/domain-components/LinkConnectorModal.tsx
LinkConnectorModalcomponent, replaced by new modal logicin
AddConnectionClient.LinkConnectorModal.stories.tsx
Remove LinkConnectorModal Storybook storiespackages/ui-v1/domain-components/stories/LinkConnectorModal.stories.tsx
LinkConnectorModal.index.ts
Remove LinkConnectorModal export from domain-components indexpackages/ui-v1/domain-components/index.ts
LinkConnectorModalcomponent.1 files
page.client.tsx
Use LoadingSpinner in embed preview loading stateapps/web/app/console/(authenticated)/connect/page.client.tsx
SpinnerwithLoadingSpinnerfor loading state in embedpreview.
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.