Register fields with form to enable complete error checking #14869
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Warning Rate limit exceeded@adolfo-pd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 46 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThe pull request introduces several enhancements across multiple components and hooks related to form handling. Key updates include a new asynchronous Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
Line range hint
11-14: Improve example code with proper authentication handlingThe example uses a hard-coded
userIdwhich isn't representative of real-world authentication. Consider adding a comment explaining this is for demonstration purposes only.- const userId = "my-authed-user-id"; + // NOTE: In production, userId should come from your authentication system + const userId = "my-authed-user-id"; // Demo purposes onlypackages/connect-react/src/hooks/use-app.tsx (3)
7-7: Remove unused import 'PropValue'The
PropValuetype imported from@pipedream/sdkis not used in this file. Removing unused imports helps keep the codebase clean.Apply this diff to remove the unused import:
import type { AppRequestResponse, AppResponse, ConfigurablePropApp, - PropValue, } from "@pipedream/sdk";
64-64: Remove debug statementconsole.logThe
console.logstatement appears to be leftover debugging code. It's recommended to remove it to keep the console output clean in production.Apply this diff to remove the debug statement:
export function appPropError(prop: ConfigurablePropApp, value: unknown, app: AppResponse | undefined): string | undefined { - console.log("appPropError", prop, value, app)
Line range hint
169-169: Remove unused generic type parameter 'T'The generic type parameter
<T extends ConfigurableProps>in thepropErrorsfunction is declared but not used. Removing it simplifies the function signature.Apply this diff to remove the unused type parameter:
-const propErrors = <T extends ConfigurableProps>(prop: ConfigurableProp, value: unknown): string[] => { +const propErrors = (prop: ConfigurableProp, value: unknown): string[] => {packages/connect-react/src/hooks/form-context.tsx (2)
7-7: Remove unused import 'PropValue'The
PropValuetype imported from@pipedream/sdkis not used in this file. Cleaning up unused imports enhances code readability.Apply this diff to remove the unused import:
import type { ComponentReloadPropsOpts, ConfigurableProp, ConfigurableProps, V1Component, - PropValue, } from "@pipedream/sdk";🧰 Tools
🪛 eslint
[error] 7-7: 'PropValue' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Lint Code Base
[failure] 7-7:
'PropValue' is defined but never used
169-169: Remove unused generic type parameter 'T'The generic type parameter
<T extends ConfigurableProps>in thepropErrorsfunction is not utilized. Removing it simplifies the code.Apply this diff to remove the unused type parameter:
-const propErrors = <T extends ConfigurableProps>(prop: ConfigurableProp, value: unknown): string[] => { +const propErrors = (prop: ConfigurableProp, value: unknown): string[] => {🧰 Tools
🪛 eslint
[error] 169-169: 'T' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Lint Code Base
[failure] 169-169:
'T' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/connect-react/examples/nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
packages/connect-react/examples/nextjs/src/app/page.tsx(1 hunks)packages/connect-react/src/components/ControlSubmit.tsx(2 hunks)packages/connect-react/src/components/InternalField.tsx(3 hunks)packages/connect-react/src/hooks/form-context.tsx(8 hunks)packages/connect-react/src/hooks/use-app.tsx(2 hunks)
🧰 Additional context used
🪛 eslint
packages/connect-react/src/hooks/form-context.tsx
[error] 7-7: 'PropValue' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 169-169: 'T' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/hooks/form-context.tsx
[failure] 7-7:
'PropValue' is defined but never used
[failure] 169-169:
'T' is defined but never used
🔇 Additional comments (2)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
33-38: Verify form field validation before submission
Given the PR's objective to "enable complete error checking", consider adding field validation before submitting the form.
Let's check how field validation is implemented in other examples:
packages/connect-react/src/components/InternalField.tsx (1)
Line range hint 343-344: Avoid mutating state directly when updating 'fields'
Directly mutating the fields state can lead to unexpected behavior. Instead, return a new object to maintain state immutability.
Apply this diff to avoid direct mutation:
setFields((fields) => {
- fields[field.prop.name] = field
- return fields
+ return {
+ ...fields,
+ [field.prop.name]: field,
+ };
});|
|
||
| function getCustomFields(app: AppResponse): AppCustomField[] { | ||
| const isOauth = app.auth_type === "oauth" | ||
| const userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]") |
There was a problem hiding this comment.
Handle potential JSON parsing errors
Using JSON.parse without error handling can throw exceptions if app.custom_fields_json contains invalid JSON. Consider adding error handling to prevent application crashes.
Apply this diff to add error handling:
const isOauth = app.auth_type === "oauth"
-let userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]")
+let userDefinedCustomFields = [];
+try {
+ userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]");
+} catch (error) {
+ console.error("Failed to parse custom fields JSON:", error);
+}Committable suggestion skipped: line range outside the PR's diff.
| return <input type="submit" value={submitting | ||
| ? "Submitting..." | ||
| : "Submit"} {...getProps("controlSubmit", baseStyles, props)} disabled={submitting} />; | ||
| : "Submit"} {...getProps("controlSubmit", baseStyles(propsNeedConfiguring.length || submitting), props)} disabled={propsNeedConfiguring.length || submitting} />; |
There was a problem hiding this comment.
Correct the logic for 'disabled' and 'opacity' properties
The current logic may not correctly determine the disabled state due to type differences. Also, the opacity should reflect the disabled state.
Apply this diff to fix the logic:
const disabled = propsNeedConfiguring.length > 0 || submitting;
return <input type="submit" value={submitting
? "Submitting..."
- : "Submit"} {...getProps("controlSubmit", baseStyles(propsNeedConfiguring.length || submitting), props)} disabled={propsNeedConfiguring.length || submitting} />;
+ : "Submit"} {...getProps("controlSubmit", baseStyles(disabled), props)} disabled={disabled} />;Update the baseStyles function to use disabled for opacity:
opacity: disabled
? 0.5
: undefined,Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/connect-react/CHANGELOG.md (1)
4-7: Consider expanding the changelog entry with implementation details.The changelog entry is clear and follows the established format. However, it could be more helpful to users by including:
- What determines if a form is "incomplete"
- Which components are affected (e.g., ControlSubmit)
- Any breaking changes in form behavior
# [1.0.0-preview.8] - 2024-12-09 -Disabled submit button when form is incomplete +- Disabled submit button when form validation fails or required fields are empty +- Enhanced ControlSubmit component to respect form state +- Added form field registration to improve validationpackages/connect-react/src/hooks/use-app.tsx (2)
30-41: Add JSDoc comments to improve type documentationWhile the types are well-structured, adding JSDoc comments would improve code maintainability and help other developers understand the purpose of each type.
Apply this diff:
+/** + * Extends AppResponse to include extracted custom field names + */ type AppResponseWithExtractedCustomFields = AppResponse & { extracted_custom_fields_names: string[] } +/** + * Represents a custom field configuration for an app + * @property name - The name of the custom field + * @property optional - Whether the field is optional during configuration + */ type AppCustomField = { name: string optional?: boolean } +/** + * Extends PropValue<"app"> to include OAuth specific properties + */ type OauthAppPropValue = PropValue<"app"> & { oauth_access_token?: string }
63-89: Improve error handling and code structureThe function could benefit from:
- More descriptive error messages
- Simplified conditional logic
- Explicit return types for all code paths
Consider this improved implementation:
export function appPropError(prop: ConfigurablePropApp, value: unknown, app: AppResponse | undefined): string | undefined { if (!app) { - return "app field not registered" + return "App configuration is missing or not registered" } if (!value) { - return "no app configured" + return "App value is not configured" } if (typeof value !== "object") { - return "not an app" + return "Invalid app value type: expected object" } const _value = value as PropValue<"app"> - if ("authProvisionId" in _value && !_value.authProvisionId) { - if (app.auth_type) { - if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) { - return "missing oauth token" - } - if (app.auth_type === "oauth" || app.auth_type === "keys") { - for (const cf of getCustomFields(app)) { - if (!cf.optional && !_value[cf.name]) { - return "missing custom field" - } - } - } - return "no auth provision configured" + + // Early return if authProvisionId is present + if (!("authProvisionId" in _value) || _value.authProvisionId) { + return undefined; + } + + if (!app.auth_type) { + return undefined; + } + + // OAuth specific validation + if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) { + return "OAuth access token is required but missing" + } + + // Custom fields validation for OAuth and keys auth types + if (app.auth_type === "oauth" || app.auth_type === "keys") { + const missingField = getCustomFields(app).find(cf => !cf.optional && !_value[cf.name]); + if (missingField) { + return `Required custom field "${missingField.name}" is missing` } } + + return "Authentication provision is not configured" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/src/hooks/use-app.tsx(2 hunks)
🔇 Additional comments (2)
packages/connect-react/src/hooks/use-app.tsx (2)
5-8: LGTM: Type imports are well-structured
The imported types from @pipedream/sdk are appropriate for the functionality being implemented.
43-61:
Add error handling and input validation
The function has several potential issues that need to be addressed:
- Unsafe JSON parsing (previously flagged)
- Missing validation for extracted_custom_fields_names
- Direct array mutation with spread operator
Apply this diff to fix these issues:
function getCustomFields(app: AppResponse): AppCustomField[] {
const isOauth = app.auth_type === "oauth"
- const userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]")
+ let userDefinedCustomFields: AppCustomField[] = [];
+ try {
+ userDefinedCustomFields = JSON.parse(app.custom_fields_json || "[]");
+ } catch (error) {
+ console.error("Failed to parse custom fields JSON:", error);
+ }
+
if ("extracted_custom_fields_names" in app && app.extracted_custom_fields_names) {
- const extractedCustomFields = ((app as AppResponseWithExtractedCustomFields).extracted_custom_fields_names || []).map(
+ const extractedCustomFields = Array.isArray(app.extracted_custom_fields_names)
+ ? app.extracted_custom_fields_names.map(
(name) => ({
name,
}),
- )
- userDefinedCustomFields.push(...extractedCustomFields)
+ ) : [];
+ userDefinedCustomFields = [...userDefinedCustomFields, ...extractedCustomFields];
}
return userDefinedCustomFields.map((cf: AppCustomField) => {
return {
...cf,
// if oauth, treat all as optional (they are usually needed for getting access token)
optional: cf.optional || isOauth,
}
})
}
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WHY
Summary by CodeRabbit
New Features
Bug Fixes
Documentation