Review and update prop validation logic for accuracy #14986
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 23 minutes and 37 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 selected for processing (1)
WalkthroughThe changes introduced in this pull request enhance the error handling and validation logic within the 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 (
|
…d-despite-all-props-configured
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/connect-react/examples/nextjs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
packages/connect-react/src/hooks/form-context.tsx(4 hunks)packages/connect-react/src/hooks/use-app.tsx(1 hunks)packages/connect-react/src/utils/component.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect-react/src/utils/component.ts
[error] 84-84: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🔇 Additional comments (2)
packages/connect-react/src/hooks/form-context.tsx (1)
175-209: Refactored validation enhances modularity and readability
The refactoring of the propErrors function to utilize separate validation functions for each prop type (appPropErrors, booleanPropErrors, integerPropErrors, etc.) improves code modularity. This approach makes the validation logic more maintainable and easier to extend in the future.
packages/connect-react/src/hooks/use-app.tsx (1)
Line range hint 10-19: Simplification of useApp hook improves clarity
Updating the useApp hook to use the GetAppResponse type and removing unused types and functions streamlines the codebase. This simplification reduces complexity and enhances maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/connect-react/src/utils/component.ts (3)
52-62: Add explicit return for the success caseThe function implicitly returns
undefinedwhen no errors are found. For better clarity and maintainability, consider adding an explicit return.export function arrayPropErrors(opts: ValidationOpts<ConfigurablePropStringArray>): string[] | undefined { const _values = valuesFromOptions(opts.value) if (!opts.prop.default && typeof _values === "undefined") { return [ "required", ] } if (!opts.prop.default && Array.isArray(_values) && !_values.length) return [ "empty array", ] + return undefined }
94-105: Add explicit return for consistencySimilar to
arrayPropErrors, add an explicit return for the success case to maintain consistency across validation functions.export function stringPropErrors(opts: ValidationOpts<ConfigurablePropString>): string[] | undefined { const _value = valueFromOption(opts.value) if (!opts.prop.default) { if (typeof _value === "undefined" || _value == null) return [ "required", ] if (!String(_value).length) return [ "string must not be empty", ] } + return undefined }
144-184: Consider breaking down the validation logicThe
appPropErrorsfunction handles multiple validation scenarios, making it complex to maintain. Consider breaking it down into smaller, focused functions.Example refactoring:
function validateOAuthToken(value: OauthAppPropValue): string[] { return !value.oauth_access_token ? ["missing oauth token"] : [] } function validateCustomFields(app: App, value: AppPropValueWithCustomFields<AppCustomField[]>): string[] { const customFields = getCustomFields(app) return customFields .filter(cf => !cf.optional && !value[cf.name]) .map(cf => `missing custom field: ${cf.name}`) } export function appPropErrors(opts: ValidationOpts<ConfigurablePropApp>): string[] | undefined { const { app, value } = opts if (!app) return ["app field not registered"] if (!value) return ["no app configured"] if (typeof value !== "object") return ["not an app"] const _value = value as PropValue<"app"> if (!("authProvisionId" in _value && !_value.authProvisionId)) return undefined if (!app.auth_type) return undefined const errors = [] if (app.auth_type === "oauth") { errors.push(...validateOAuthToken(_value as OauthAppPropValue)) } if (app.auth_type === "oauth" || app.auth_type === "keys") { errors.push(...validateCustomFields(app, _value as AppPropValueWithCustomFields<AppCustomField[]>)) } if (app.auth_type !== "none") { errors.push("no auth provision configured") } return errors.length > 0 ? errors : undefined }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/src/utils/component.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect-react/src/utils/component.ts
[error] 85-85: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🔇 Additional comments (3)
packages/connect-react/src/utils/component.ts (3)
5-16: LGTM: Well-structured type definition and safe value extraction
The PropOptionValue type and valueFromOption function are well-implemented with proper type safety and handling of edge cases.
107-123: LGTM: Well-structured type definitions
The custom fields types are well-defined and make good use of TypeScript features for type safety.
77-79:
Address existing issues with conditional logic and NaN check
Two issues need to be addressed as previously noted:
- The conditional logic needs parentheses for proper evaluation
- The NaN check should use Number.isNaN
Apply these fixes:
-if (!prop.default && value == null || typeof value === "undefined") return [
+if (!prop.default && (value == null || typeof value === "undefined")) return [
"required",
]
-if (_value !== _value) return [
+if (Number.isNaN(_value)) return [
"not a number",
]Also applies to: 85-87
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/connect-react/CHANGELOG.md (1)
6-7: Consider adding more details to the changelog entries.While the entries are clear, they could be more specific about:
- What inconsistencies were fixed in the prop validation
- How the string prop validation was relaxed (e.g., what behavior to expect)
This would help users better understand the impact of these changes.
Example of more detailed entries:
-Make prop validation more consistent with app behavior -Relax validation of string props when value is not a string +Make prop validation more consistent with app behavior by [specific changes] +Relax validation of string props to handle non-string values by [specific behavior]packages/connect-react/src/utils/component.ts (2)
11-16: Improve type safety with type predicatesConsider using a type predicate to improve type narrowing and make the code more type-safe.
+type isPropOptionValue = <T>(value: unknown): value is PropOptionValue<T> => { + return typeof value === "object" && value !== null && "__lv" in value; +} + -export function valueFromOption<T>(value: T | PropOptionValue<T>): T | undefined | null { +export function valueFromOption<T>(value: T | PropOptionValue<T>): T { - if (typeof value === "object" && value && "__lv" in value) { + if (isPropOptionValue<T>(value)) { return (value as PropOptionValue<T>).__lv.value } return value }
145-185: Improve readability with early returns and error message organizationThe function has deeply nested conditionals that make it hard to follow. Consider restructuring with early returns and organizing error messages better.
export function appPropErrors(opts: ValidationOpts<ConfigurablePropApp>): string[] | undefined { const { app, value } = opts + const errors: string[] = [] + if (!app) { return ["app field not registered"] } if (!value) { return ["no app configured"] } if (typeof value !== "object") { return ["not an app"] } + const _value = value as PropValue<"app"> - if ("authProvisionId" in _value && !_value.authProvisionId) { - if (app.auth_type) { - const errs = [] - if (app.auth_type === "oauth" && !(_value as OauthAppPropValue).oauth_access_token) { - errs.push("missing oauth token") - } - if (app.auth_type === "oauth" || app.auth_type === "keys") { - const customFields = getCustomFields(app) - const _valueWithCustomFields = _value as AppPropValueWithCustomFields<typeof customFields> - for (const cf of customFields) { - if (!cf.optional && !_valueWithCustomFields[cf.name]) { - errs.push(`missing custom field: ${cf.name}`) - } - } - } - if (app.auth_type !== "none") - errs.push("no auth provision configured") - return errs + + if (!("authProvisionId" in _value) || _value.authProvisionId) { + return undefined + } + + if (!app.auth_type) { + return undefined + } + + if (app.auth_type === "oauth") { + if (!(_value as OauthAppPropValue).oauth_access_token) { + errors.push("missing oauth token") } } + + if (app.auth_type === "oauth" || app.auth_type === "keys") { + const customFields = getCustomFields(app) + const _valueWithCustomFields = _value as AppPropValueWithCustomFields<typeof customFields> + for (const cf of customFields) { + if (!cf.optional && !_valueWithCustomFields[cf.name]) { + errors.push(`missing custom field: ${cf.name}`) + } + } + } + + if (app.auth_type !== "none") { + errors.push("no auth provision configured") + } + + return errors.length > 0 ? errors : undefined }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/utils/component.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/connect-react/package.json
🔇 Additional comments (3)
packages/connect-react/CHANGELOG.md (1)
4-5: LGTM! Version and date format are correct.
The version number follows semantic versioning, and the preview designation is appropriate for breaking changes.
packages/connect-react/src/utils/component.ts (2)
25-45:
Fix duplicate push operation and improve error handling
There are several issues in this function:
- Line 35 causes duplicate push when the value is already pushed in line 33
- Generic error messages make debugging difficult
- Missing explicit return type annotation
-export function valuesFromOptions<T>(value: unknown | T[] | PropOptions<T>): T[] {
+export function valuesFromOptions<T>(value: unknown | T[] | PropOptions<T>): T[] {
if (typeof value === "object" && value && "selectedOptions" in value && Array.isArray(value.selectedOptions)) {
const selectedOptions = value.selectedOptions as PropOption<T>[]
const results: T[] = []
for (const so of selectedOptions) {
if (typeof so === "object" && so && "emitValue" in so) {
const emitValue = so.emitValue as T | PropOptionValue<T>
if (typeof emitValue === "object" && emitValue && "__lv" in emitValue) {
results.push(emitValue.__lv.value)
+ continue
}
- results.push(emitValue as T)
+ results.push(emitValue as T)
} else {
- throw "unexpected value"
+ throw new Error("Invalid option structure: missing emitValue property")
}
}
return results
}
if (!Array.isArray(value))
- throw "unexpected value"
+ throw new Error("Invalid value: expected array or PropOptions object")
return value as T[]
}Likely invalid or redundant comment.
125-143:
Add error handling and avoid array mutation
The function has two issues:
- Missing error handling for JSON.parse
- Mutates array with push
function getCustomFields(app: App): 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)
+ userDefinedCustomFields = []
+ }
if ("extracted_custom_fields_names" in app && app.extracted_custom_fields_names) {
const extractedCustomFields = ((app as AppWithExtractedCustomFields).extracted_custom_fields_names || []).map(
(name) => ({
name,
}),
)
- userDefinedCustomFields.push(...extractedCustomFields)
+ userDefinedCustomFields = [...userDefinedCustomFields, ...extractedCustomFields]
}
return userDefinedCustomFields.map((cf: AppCustomField) => {
return {
...cf,
optional: cf.optional || isOauth,
}
})
}Likely invalid or redundant comment.
adolfo-pd
deleted the
adolfo/money-1953-submit-button-not-enabled-despite-all-props-configured
branch
WHY
Summary by CodeRabbit
New Features
errorsproperty in the form context.Bug Fixes
Refactor
useApphook by updating type dependencies and removing unnecessary code.Chores