-
Notifications
You must be signed in to change notification settings - Fork 427
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
fix(capping): reup capping on connection creation #2674
Conversation
WalkthroughThe changes involve updates to error handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Middleware
participant Server
User->>Frontend: Create Integration
Frontend->>Middleware: Validate Integration
Middleware-->>Frontend: Integration Validated
Frontend->>Server: Setup Connection
Server-->>Frontend: Connection Setup Successful
Frontend-->>User: Integration Created
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/frontend/lib/index.ts (7 hunks)
- packages/server/lib/hooks/hooks.ts (1 hunks)
- packages/server/lib/middleware/resource-capping.middleware.ts (1 hunks)
- packages/server/lib/routes.ts (3 hunks)
- packages/server/lib/utils/asyncWrapper.ts (1 hunks)
- packages/types/lib/api.ts (1 hunks)
- packages/types/lib/flow/http.api.ts (1 hunks)
- packages/webapp/src/pages/Connection/Create.tsx (1 hunks)
Additional comments not posted (18)
packages/server/lib/middleware/resource-capping.middleware.ts (4)
2-2
: LGTM!The code changes are approved.
5-6
: LGTM!The code changes are approved.
7-16
: LGTM!The code changes are approved.
19-20
: LGTM!The code changes are approved.
packages/types/lib/flow/http.api.ts (1)
31-31
: LGTM! The code change aligns with the PR objectives.The removal of the
ApiError<'resource_capped'>
variant from theError
property of thePostPreBuiltDeploy
type is consistent with the PR objectives and the AI-generated summary. This change indicates that the API will no longer handle or report errors related to resource capping in the context of pre-built deployments.Clients of the API should be aware that they will no longer receive specific feedback regarding resource limitations during deployment processes. They may need to update their error handling logic accordingly.
The remaining error types in the union type (
ApiError<'unknown_provider'>
,ApiError<'failed_to_deploy', Error[]>
, andApiError<'unknown_flow'>
) are still relevant and appropriate for thePostPreBuiltDeploy
endpoint.packages/types/lib/api.ts (1)
23-24
: LGTM!The addition of the
ApiError<'resource_capped'>
error type to theResDefaultErrors
union is a valuable enhancement to the API's error handling capabilities. It allows the system to provide more specific feedback to clients when a resource has reached its capacity limit.The change seamlessly integrates with the existing error types without affecting their handling, enriching the API's response options while maintaining the overall structure of the
ResDefaultErrors
type.The code changes are approved.
packages/server/lib/hooks/hooks.ts (2)
54-56
: LGTM!The addition of the early return condition enhances the function's robustness and efficiency by explicitly handling the case where
scriptConfigs
is empty. This avoids unnecessary iterations over an empty array.
58-59
: Also applies to: 61-66packages/server/lib/routes.ts (5)
29-29
: LGTM!The import of
RequestHandler
type fromexpress
is approved.
88-88
: LGTM!The type annotation for the
apiAuth
array is approved.
89-93
: LGTM!The type annotation for the
adminAuth
array is approved.
94-96
: LGTM!The type annotations for the
apiPublicAuth
andwebAuth
arrays are approved.
20-20
: Verify the impact of replacingauthCheck
withresourceCapping
middleware.Ensure that the
resourceCapping
middleware provides the necessary authentication and authorization checks for the API public authentication flow.Run the following script to verify the middleware usage:
Verification successful
Verified: The
resourceCapping
middleware is correctly integrated into the API public authentication flow.The
resourceCapping
middleware is used in theapiPublicAuth
array inpackages/server/lib/routes.ts
, confirming its role in the authentication process. It is defined inpackages/server/lib/middleware/resource-capping.middleware.ts
, indicating its functionality is implemented as expected.
packages/server/lib/routes.ts
: Usage inapiPublicAuth
array.packages/server/lib/middleware/resource-capping.middleware.ts
: Middleware definition.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `resourceCapping` middleware in the API public authentication flow. # Test: Search for the middleware usage in the file. Expect: Only occurrences in the `apiPublicAuth` array. rg --type typescript $'resourceCapping'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of `resourceCapping` middleware in the API public authentication flow. # Test: Search for the middleware usage in the file. Expect: Only occurrences in the `apiPublicAuth` array. rg --type ts 'resourceCapping'Length of output: 499
packages/frontend/lib/index.ts (4)
100-100
: LGTM!The code changes are approved. The updated error handling mechanism enhances the specificity of the error information being passed to the
AuthError
, potentially allowing for more precise error handling and debugging.
312-312
: LGTM!The code changes are approved. The updated error handling mechanism is consistent with the previous code segment and enhances the specificity of the error information being passed to the
AuthError
.
333-333
: LGTM!The code changes are approved. The updated error handling mechanism is consistent with the previous code segments and enhances the specificity of the error information being passed to the
AuthError
.Also applies to: 354-354
375-375
: LGTM!The code changes are approved. The updated error handling mechanism is consistent with the previous code segments and enhances the specificity of the error information being passed to the
AuthError
.Also applies to: 396-396, 417-417
packages/webapp/src/pages/Connection/Create.tsx (1)
242-242
: LGTM!Resetting the
serverErrorMessage
state to an empty string when an existingintegration
is detected is a good practice. It clears any previous server error messages before setting up the connection configuration parameters and authentication mode based on the current integration, improving the user experience by preventing stale error messages from persisting when a valid integration is loaded.
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.
one comment + a question of any
generic type but looks good otherwise
packages/server/lib/hooks/hooks.ts
Outdated
@@ -51,18 +51,19 @@ export const connectionCreationStartCapCheck = async ({ | |||
} | |||
|
|||
const scriptConfigs = await getSyncConfigsWithConnections(providerConfigKey, environmentId); | |||
if (scriptConfigs.length <= 0) { | |||
return 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.
not sure this check is useful. If empty the for loop below will immediately exit
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.
yeah indeed
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/server/lib/hooks/hooks.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/server/lib/hooks/hooks.ts
Describe your changes
Fixes https://linear.app/nango/issue/NAN-1638/capping-on-connection-creation-is-not-working-anymore
Reup capping
Fix error handling
Unfortunately, we have mixed public and internal API, during error handling rework it changed the API response for both :/
It was not broken but just displaying empty error
UI: fix error not resetting when changing integration
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores