-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/UI fix oauth custom #436
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
Warning Rate Limit Exceeded@naelob has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 21 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update enhances client-side configuration management for connectors, introducing new components for improved display and management. It includes advanced form handling, state management techniques, UI enhancements like removing the "system" theme option, and documentation updates for API key generation and self-hosting guides. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
apps/client-ts/src/components/Configuration/CustomConnectorPage.tsx (1)
9-9
: Clarify the intended use ofdefaultLayout
as it is set toundefined
. If it's meant to be configurable, consider providing a default value or handling its absence.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
apps/client-ts/package.json
is excluded by!**/*.json
pnpm-lock.yaml
is excluded by!pnpm-lock.yaml
,!**/*.yaml
Files selected for processing (21)
- apps/client-ts/src/app/(Dashboard)/configuration/page.tsx (4 hunks)
- apps/client-ts/src/components/Configuration/AddAuthCredentialsForm.tsx (8 hunks)
- apps/client-ts/src/components/Configuration/Connector/ConnectorDisplay.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/Connector/ConnectorLayout.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/Connector/ConnectorList.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/Connector/VerticalSelector.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/Connector/useConnector.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/CustomConnectorPage.tsx (1 hunks)
- apps/client-ts/src/components/Configuration/authColumns.tsx (1 hunks)
- apps/client-ts/src/components/Nav/theme-toggle.tsx (1 hunks)
- apps/client-ts/src/components/Provider/provider.tsx (1 hunks)
- apps/client-ts/src/components/RootLayout/index.tsx (2 hunks)
- apps/client-ts/src/components/ui/resizable.tsx (1 hunks)
- apps/client-ts/src/components/ui/scroll-area.tsx (1 hunks)
- apps/client-ts/src/components/ui/textarea.tsx (1 hunks)
- apps/client-ts/src/hooks/mutations/useConnectionStrategyAuthCredentials.tsx (2 hunks)
- apps/client-ts/src/hooks/mutations/useUpdateConnectionStrategy.tsx (2 hunks)
- apps/client-ts/src/state/verticalStore.ts (1 hunks)
- docs/core-concepts/auth.mdx (2 hunks)
- docs/open-source/self-host-guide.mdx (1 hunks)
- packages/shared/src/utils.ts (3 hunks)
Files not reviewed due to errors (4)
- apps/client-ts/src/hooks/mutations/useUpdateConnectionStrategy.tsx (no review received)
- docs/core-concepts/auth.mdx (no review received)
- apps/client-ts/src/app/(Dashboard)/configuration/page.tsx (no review received)
- apps/client-ts/src/components/Configuration/Connector/ConnectorDisplay.tsx (no review received)
Files skipped from review due to trivial changes (4)
- apps/client-ts/src/components/Configuration/authColumns.tsx
- apps/client-ts/src/components/Nav/theme-toggle.tsx
- apps/client-ts/src/hooks/mutations/useConnectionStrategyAuthCredentials.tsx
- docs/open-source/self-host-guide.mdx
Additional comments not posted (16)
apps/client-ts/src/components/Configuration/Connector/useConnector.tsx (1)
13-15
: The implementation ofuseConnector
using Jotai is clean and follows best practices for state management in React.apps/client-ts/src/state/verticalStore.ts (1)
8-11
: The Zustand store for managing vertical state is correctly implemented and follows best practices for state management in modern React applications.apps/client-ts/src/components/Provider/provider.tsx (1)
Line range hint
1-17
: The setup of React Query withQueryClientProvider
andReactQueryDevtools
is correctly implemented and follows best practices for managing server state in React applications.apps/client-ts/src/components/ui/textarea.tsx (1)
8-24
: The implementation of theTextarea
component usingReact.forwardRef
and utility classes for styling is well-done and follows best practices for reusable React components.apps/client-ts/src/components/Configuration/CustomConnectorPage.tsx (1)
6-36
: The implementation ofCustomConnectorPage
using Zustand for state management and React hooks for local state and effects is well-structured and follows best practices for React component design.apps/client-ts/src/components/ui/scroll-area.tsx (1)
8-48
: The implementation ofScrollArea
andScrollBar
using Radix UI components and utility classes for styling is well-done and follows best practices for creating custom scrollable areas in React applications.apps/client-ts/src/components/ui/resizable.tsx (1)
8-45
: The implementation ofResizablePanelGroup
,ResizablePanel
, andResizableHandle
usingreact-resizable-panels
and utility classes for styling is well-done and follows best practices for creating resizable components in React applications.apps/client-ts/src/components/Configuration/Connector/VerticalSelector.tsx (1)
24-74
: The implementation ofVerticalSelector
using Zustand for state management, Radix UI for the popover component, and utility classes for styling is well-done and follows best practices for creating interactive UI components in React applications.apps/client-ts/src/components/Configuration/Connector/ConnectorList.tsx (2)
12-70
: LGTM! TheConnectorList
function correctly utilizes theuseConnector
hook and handles the rendering of items efficiently.
72-79
: Ensure all necessary badge variants are handled. Currently, only 'oauth2' has a specific variant.apps/client-ts/src/components/RootLayout/index.tsx (1)
15-23
: Ensure the theme state is correctly managed across the application as the theme logic has been updated.apps/client-ts/src/components/Configuration/Connector/ConnectorLayout.tsx (1)
22-74
: LGTM! TheConnectorLayout
function effectively manages layout and state with appropriate event handlers.apps/client-ts/src/components/Configuration/AddAuthCredentialsForm.tsx (2)
45-45
: Renamed variableToUpdateToggle
toupdateToggle
aligns with JavaScript naming conventions.
108-108
: Removal of toast message function inuseConnectionStrategyAuthCredentialsMutation
simplifies the user experience by removing unnecessary notifications.packages/shared/src/utils.ts (2)
1-1
: Ensure the new import is utilized effectively in the file.Verification successful
The new import
categoriesVerticals
is effectively utilized in the file.
categoriesVerticals
is imported from./providers
.- It is used in a
forEach
loop within the file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'categoriesVerticals' is used in the file. # Test: Search for 'categoriesVerticals' usage. Expect: At least one occurrence. rg --type typescript 'categoriesVerticals' packages/shared/src/utils.tsLength of output: 106
Script:
#!/bin/bash # Description: Verify that 'categoriesVerticals' is used in the file. # Test: Search for 'categoriesVerticals' usage. Expect: At least one occurrence. rg --type ts 'categoriesVerticals' packages/shared/src/utils.tsLength of output: 160
2164-2202
: Review the implementation ofprovidersArray
function for handling vertical filtering.The function correctly handles the optional
vertical
parameter to filter providers based on the specified vertical. It uses existing utility functions effectively to fetch and transform provider data.
} | ||
|
||
const configAtom = atom<Config>({ | ||
selected: `crm-${providersArray("crm")[0].name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling the case where providersArray("crm")
returns an empty array to avoid runtime errors.
- selected: `crm-${providersArray("crm")[0].name}`,
+ selected: providersArray("crm").length > 0 ? `crm-${providersArray("crm")[0].name}` : 'default-crm',
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
selected: `crm-${providersArray("crm")[0].name}`, | |
selected: providersArray("crm").length > 0 ? `crm-${providersArray("crm")[0].name}` : 'default-crm', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
docs/images/custom-oauth.png
is excluded by!**/*.png
,!**/*.png
docs/images/webhooks.mp4
is excluded by!**/*.mp4
,!**/*.mp4
Files selected for processing (4)
- docs/glossary/linked-account-object.mdx (1 hunks)
- docs/recipes/add-custom-provider-creds.mdx (1 hunks)
- docs/recipes/catch-connection-token.mdx (1 hunks)
- docs/webhooks/overview.mdx (5 hunks)
inside your sofwtare's developer account.{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct the spelling error in "software".
- inside your sofwtare's developer account.
+ inside your software's developer account.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Before getting started, make sure you've created a custom oAuth application | |
inside your sofwtare's developer account.{" "} | |
Before getting started, make sure you've created a custom oAuth application | |
inside your software's developer account.{" "} |
- `id_linked_user` is the **id** of the user inside **our** system. | ||
- `linked_user_origin_id` is the **id** of the user inside **your** system. | ||
- `alias` is the name of your system/company. <br/> | ||
|
||
It holds information about **user's data** | ||
that exist in your **own product**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider varying the sentence structure to enhance readability.
- - `id_linked_user` is the **id** of the user inside **our** system.
- - `linked_user_origin_id` is the **id** of the user inside **your** system.
- - `alias` is the name of your system/company. <br/>
+ - The **id** of the user inside **our** system is `id_linked_user`.
+ - Inside **your** system, the user's **id** is `linked_user_origin_id`.
+ - `alias` represents the name of your system/company. <br/>
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
- `id_linked_user` is the **id** of the user inside **our** system. | |
- `linked_user_origin_id` is the **id** of the user inside **your** system. | |
- `alias` is the name of your system/company. <br/> | |
It holds information about **user's data** | |
that exist in your **own product**. | |
- The **id** of the user inside **our** system is `id_linked_user`. | |
- Inside **your** system, the user's **id** is `linked_user_origin_id`. | |
- `alias` represents the name of your system/company. <br/> | |
It holds information about **user's data** | |
that exist in your **own product**. |
|
||
// Match the raw body to content type application/json | ||
// If you are using Express v4 - v4.16 you need to use body-parser, not express, to retrieve the request body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comment about Express middleware to reflect newer versions.
- // If you are using Express v4 - v4.16 you need to use body-parser, not express, to retrieve the request body
+ // For Express v4.17.0 and later, use express.json() to parse JSON bodies.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
// If you are using Express v4 - v4.16 you need to use body-parser, not express, to retrieve the request body | |
// For Express v4.17.0 and later, use express.json() to parse JSON bodies. |
contact_data = event.data | ||
# Then define and call a method to handle the successful contact creation. | ||
# handle_contact_creation_succeeded(contact_data) | ||
elif event.type == 'crm.company.created': | ||
company_data = event.data | ||
company_data = event.data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adopt Python naming conventions for variable names.
- contact_data = event.data
- company_data = event.data
+ contact_data = event['data']
+ company_data = event['data']
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
contact_data = event.data | |
# Then define and call a method to handle the successful contact creation. | |
# handle_contact_creation_succeeded(contact_data) | |
elif event.type == 'crm.company.created': | |
company_data = event.data | |
company_data = event.data | |
contact_data = event['data'] | |
# Then define and call a method to handle the successful contact creation. | |
# handle_contact_creation_succeeded(contact_data) | |
elif event.type == 'crm.company.created': | |
company_data = event['data'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Out of diff range and nitpick comments (2)
apps/client-ts/src/components/Configuration/AddAuthCredentialsForm.tsx (2)
45-45
: Consider using consistent import grouping and spacing.It's a good practice to group similar imports together and separate them with a newline for better readability. For example, group all hooks together, all utility functions together, etc.
348-608
: Ensure accessibility and usability in form interactions.Consider adding aria-labels and roles where appropriate to enhance accessibility, especially in complex UI components like
Popover
andSelect
.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/api/swagger/swagger-spec.json
is excluded by!**/*.json
Files selected for processing (10)
- apps/client-ts/src/components/Configuration/AddAuthCredentialsForm.tsx (12 hunks)
- apps/client-ts/src/components/Configuration/Connector/ConnectorDisplay.tsx (1 hunks)
- apps/client-ts/src/hooks/mutations/useConnectionStrategy.tsx (1 hunks)
- apps/client-ts/src/hooks/mutations/useConnectionStrategyAuthCredentials.tsx (3 hunks)
- packages/api/src/@core/connections-strategies/connections-strategies.controller.ts (2 hunks)
- packages/api/src/@core/connections-strategies/dto/create-connections-strategies.dto.ts (1 hunks)
- packages/api/src/@core/connections-strategies/dto/get-connection-cs-credentials.dto.ts (1 hunks)
- packages/api/src/@core/webhook/dto/webhook.dto.ts (1 hunks)
- packages/api/src/@core/webhook/webhook.controller.ts (2 hunks)
- packages/api/src/@core/webhook/webhook.service.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/client-ts/src/hooks/mutations/useConnectionStrategyAuthCredentials.tsx
Additional comments not posted (6)
packages/api/src/@core/webhook/webhook.controller.ts (1)
64-79
: The implementation of theverifyEvent
endpoint is secure and well-documented. Good use ofJwtAuthGuard
and Swagger annotations.apps/client-ts/src/hooks/mutations/useConnectionStrategy.tsx (1)
13-13
: Ensure that theid_cs
field is always provided and validated to avoid potential issues with undefined values.packages/api/src/@core/connections-strategies/connections-strategies.controller.ts (1)
39-45
: The implementation ofcreateConnectionStrategy
is secure and correctly uses DTOs for data transfer. Good job on ensuring type safety and validation.packages/api/src/@core/webhook/webhook.service.ts (2)
22-27
: ThegenerateSignature
method is implemented securely using HMAC with SHA-256. This is a good choice for ensuring data integrity.
263-277
: TheverifyPayloadSignature
method correctly implements signature verification, which is crucial for the security of webhook processing.apps/client-ts/src/components/Configuration/AddAuthCredentialsForm.tsx (1)
108-108
: Ensure thatuseConnectionStrategyAuthCredentialsMutation
is used correctly.
@@ -166,7 +164,7 @@ const AddAuthCredentialsForm = (prop : propType) => { | |||
) | |||
} | |||
|
|||
},[]) | |||
},[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add prop.performUpdate
to the dependency array of useEffect
.
This ensures that the effect re-runs whenever prop.performUpdate
changes, which is likely the intended behavior given the logic inside the effect.
@@ -215,7 +213,7 @@ | |||
{ | |||
updateCS({ | |||
id_cs:prop.data?.id_cs, | |||
ToUpdateToggle: false, | |||
updateToggle: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring the repeated mutation logic.
The mutation logic for different authentication strategies is very similar and could be abstracted into a helper function to reduce redundancy and improve maintainability.
Also applies to: 257-257, 306-306
export class SignatureVerificationDto { | ||
payload: { [key: string]: any }; | ||
signature: string; | ||
secret: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding more specific types for payload
to enhance type safety and predictability of the data structure.
<div className="flex flex-col"> | ||
<FormField | ||
name="scope" | ||
control={form.control} | ||
render={({ field }) => ( | ||
<FormItem> | ||
<FormLabel className="flex flex-col">Scope</FormLabel> | ||
<FormControl> | ||
<Input {...field} placeholder="Enter Scopes" /> | ||
</FormControl> | ||
<FormMessage /> | ||
</FormItem> | ||
)} | ||
/> | ||
</div> | ||
<div className="flex flex-col"> | ||
<FormLabel className="flex flex-col">Redirect URI</FormLabel> | ||
<div className="flex gap-2 mt-1"> | ||
<Input value="localhost:3000/connections/oauth/callback" readOnly /> | ||
<Button type="button" onClick={handleCopy}> | ||
{copied ? 'Copied!' : ( | ||
<> | ||
<p className="mr-1">Copy</p> | ||
<svg width="15" height="15" viewBox="0 0 15 15" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M5 2V1H10V2H5ZM4.75 0C4.33579 0 4 0.335786 4 0.75V1H3.5C2.67157 1 2 1.67157 2 2.5V12.5C2 13.3284 2.67157 14 3.5 14H11.5C12.3284 14 13 13.3284 13 12.5V2.5C13 1.67157 12.3284 1 11.5 1H11V0.75C11 0.335786 10.6642 0 10.25 0H4.75ZM11 2V2.25C11 2.66421 10.6642 3 10.25 3H4.75C4.33579 3 4 2.66421 4 2.25V2H3.5C3.22386 2 3 2.22386 3 2.5V12.5C3 12.7761 3.22386 13 3.5 13H11.5C11.7761 13 12 12.7761 12 12.5V2.5C12 2.22386 11.7761 2 11.5 2H11Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path></svg> | ||
</> | ||
)} | ||
</Button> | ||
</div> | ||
</div> | ||
</> | ||
} | ||
{ | ||
item.authStrategy == AuthStrategy.api_key && | ||
<> | ||
<div className="flex flex-col"> | ||
<FormField | ||
name="api_key" | ||
control={form.control} | ||
render={({field}) => ( | ||
<FormItem> | ||
<FormLabel className="flex flex-col">API Key</FormLabel> | ||
<FormControl> | ||
<PasswordInput {...field} placeholder="Enter API Key" /> | ||
</FormControl> | ||
<FormMessage/> | ||
</FormItem> | ||
)} | ||
/> | ||
</div> | ||
</> | ||
} | ||
{ | ||
item.authStrategy == AuthStrategy.basic && | ||
<> | ||
<div className="flex flex-col"> | ||
<FormField | ||
name="username" | ||
control={form.control} | ||
render={({field}) => ( | ||
<FormItem> | ||
<FormLabel className="flex flex-col">Username</FormLabel> | ||
<FormControl> | ||
<PasswordInput {...field} placeholder="Enter API Key" /> | ||
</FormControl> | ||
<FormMessage/> | ||
</FormItem> | ||
)} | ||
/> | ||
</div> | ||
<div className="flex flex-col"> | ||
<FormField | ||
name="secret" | ||
control={form.control} | ||
render={({field}) => ( | ||
<FormItem> | ||
<FormLabel className="flex flex-col">Secret</FormLabel> | ||
<FormControl> | ||
<PasswordInput {...field} placeholder="Enter API Key" /> | ||
</FormControl> | ||
<FormMessage/> | ||
</FormItem> | ||
)} | ||
/> | ||
</div> | ||
</> | ||
} | ||
<Button type="submit" className="mt-4">Save</Button> | ||
</form> | ||
</Form> | ||
</div> | ||
<Separator className="mt-auto" /> | ||
</div> | ||
) : ( | ||
<div className="p-8 text-center text-muted-foreground"> | ||
No connector selected | ||
</div> | ||
)} | ||
</div> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring ConnectorDisplay
into smaller components to improve readability and maintainability. This component handles multiple responsibilities, which could be separated into more focused sub-components.
No description provided.