Biz/dj 2780 add support for configurationerror in connect react #15479
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces minor formatting changes by appending newlines at the end of several .mjs files. Additionally, it enhances error handling and debugging in the connect-react package. Key updates include extending component prop types with new optional properties ( Changes
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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 (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
…rror-in-connect-react
…rror-in-connect-react
…rror-in-connect-react
![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
🧹 Nitpick comments (15)
packages/connect-react/src/components/Description.tsx (1)
37-37: Implement or remove the TODO comment.The TODO comment for app type description needs to be addressed.
Would you like me to help implement the description for the app type or create an issue to track this task?
packages/connect-react/src/components/Field.tsx (2)
68-68: Consider documenting the debugging feature.The conditional rendering of the
Errorscomponent based onenableDebuggingis a good practice for controlling debug output. However, it would be helpful to document this feature for other developers.Consider adding a comment above this line explaining when and why debugging should be enabled:
+ {/* Render error details when debugging is enabled through the form context */} { enableDebugging && <Errors field={field} form={form} /> }
8-11: Document the enableDebugging prop in FieldProps.Since
enableDebuggingis now a critical part of the error handling flow, it should be documented in the type definition.Consider adding JSDoc comments to the type definition:
export type FieldProps<T extends ConfigurableProp> = { + /** The form context containing form-wide state and methods */ form: FormContext; + /** The field context containing field-specific state, including debug settings */ field: FormFieldContext<T>; };packages/connect-react/src/hooks/form-context.tsx (4)
27-27: Consider creating a dedicated error interface rather than usingRecord<string, string>Defining a named interface (e.g.,
SdkError { name: string; message: string }) may enhance clarity, self-documentation, and type safety for SDK errors.
41-41: Slight naming inconsistency for a booleanConsider renaming this property to something like
isDebugModeordebugEnabledfor improved clarity, reflecting its boolean nature.
99-102: Clarify how you intend to manage the lifespan of these errorsStoring all SDK errors in a growing array can lead to unbounded accumulation. Consider whether you need to periodically clear old entries or track only the latest errors. Introducing a dedicated error management strategy can prevent memory bloat.
294-301: Re-checking error state on multiple dependenciesThis
useEffecttriggers wheneverconfiguredProps,reloadPropIdx, orqueryDisabledIdxchange. While correct, keep in mind it can re-run frequently in certain dynamic scenarios. Monitor performance or consider debouncing if it becomes expensive.packages/connect-react/src/components/Errors.tsx (2)
19-25: Consider adding type guard for errors object.The null checks are good, but consider adding a type guard to ensure type safety when accessing
errors[prop.name].- if (!Object.keys(errors || {}).length) { + if (!errors || !Object.keys(errors).length) { return null; } - if (!errors[prop.name]) { + if (!Array.isArray(errors[prop.name])) { return null }
38-41: Consider using CSS-in-JS or styled-components.While inline styles work, consider using CSS-in-JS or styled-components for better maintainability and reusability.
packages/connect-react/src/components/Alert.tsx (1)
10-12: Consider using CSS variables for consistent styling.While the styling changes look good, consider using CSS variables for values like border radius and widths to maintain consistency across components.
const baseStyles = { - width: "100%", - borderRadius: "5px", + width: "var(--pd-alert-width, 100%)", + borderRadius: "var(--pd-alert-radius, 5px)",packages/connect-react/src/components/InternalField.tsx (1)
47-49: Consider documenting the error structure.While the implementation is good, consider adding JSDoc comments to document the expected structure of the
errorsobject.+ /** + * @property {Record<string, string[]>} errors - Map of field names to error messages + * @property {boolean} enableDebugging - Flag to enable debug mode + */ errors, enableDebugging,packages/connect-react/examples/nextjs/src/app/page.tsx (2)
28-31: Consider using a more specific error type.Instead of using
unknown, consider defining a specific type for SDK errors to improve type safety.+type SDKError = { + message: string; + code?: string; + details?: unknown; +}; - ] = useState<unknown[] | unknown | undefined>(undefined); + ] = useState<SDKError[] | SDKError | undefined>(undefined);
51-60: Add error logging middleware.Consider adding error logging middleware to capture and report SDK errors consistently.
+const logError = (error: unknown) => { + // Add your error logging logic here + console.error("Action run failed:", error); +}; try { const result = await client.runAction({ userId, actionId: "slack-send-message", configuredProps, dynamicPropsId, }); setSdkErrors(result) } catch (error) { setSdkErrors(error as unknown) - console.error("Action run failed:", error); + logError(error); }packages/connect-react/src/components/InternalComponentForm.tsx (1)
44-60: Simplify error handling logic.The error handling implementation can be improved for better maintainability.
Consider this simplified implementation:
- useEffect(() => { - if (submitting) setSdkErrors([]) - else { - if (__sdkErrors && __sdkErrors.length) { - setSdkErrors(__sdkErrors.map((e) => { - return { - type: "alert", - alertType: "error", - content: `# ${e.name}\n${e.message}`, - } - })) - } - } - }, [ - __sdkErrors, - submitting, - ]); + useEffect(() => { + setSdkErrors(submitting ? [] : + __sdkErrors?.map(e => ({ + type: "alert", + alertType: "error", + content: `# ${e.name}\n${e.message}`, + })) ?? [] + ); + }, [__sdkErrors, submitting]);🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/connect-react/CHANGELOG.md (1)
4-6: Enhance changelog entry with more details.While the entry correctly documents the feature, it could be more descriptive to help users understand the changes.
Consider expanding the changelog entry:
# [1.0.0-preview.28] - 2025-02-05 -Surface SDK errors in the form +Surface SDK errors in the form: +- Added support for displaying SDK errors through the new `sdkErrors` prop +- Added `enableDebugging` flag to control error visibility +- Errors are automatically cleared during form submission
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/connect-react/examples/nextjs/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
components/buysellads/buysellads.app.mjs(1 hunks)components/humanlayer/humanlayer.app.mjs(1 hunks)components/planhat/planhat.app.mjs(1 hunks)components/storerocket/storerocket.app.mjs(1 hunks)packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/README.md(1 hunks)packages/connect-react/examples/nextjs/package.json(1 hunks)packages/connect-react/examples/nextjs/src/app/page.tsx(2 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/Alert.tsx(1 hunks)packages/connect-react/src/components/ComponentForm.tsx(1 hunks)packages/connect-react/src/components/Description.tsx(1 hunks)packages/connect-react/src/components/Errors.tsx(1 hunks)packages/connect-react/src/components/Field.tsx(2 hunks)packages/connect-react/src/components/InternalComponentForm.tsx(5 hunks)packages/connect-react/src/components/InternalField.tsx(2 hunks)packages/connect-react/src/hooks/form-context.tsx(9 hunks)packages/connect-react/src/hooks/form-field-context.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- components/storerocket/storerocket.app.mjs
- components/buysellads/buysellads.app.mjs
- components/planhat/planhat.app.mjs
- components/humanlayer/humanlayer.app.mjs
- packages/connect-react/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect-react/src/components/InternalComponentForm.tsx
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
🔇 Additional comments (17)
packages/connect-react/src/components/Description.tsx (2)
3-5: LGTM!The import statement reorganization improves readability while maintaining functionality.
16-17: Consider addressing the XXX comment about component naming.The comment raises a valid point about component naming and context dependencies. Consider either:
- Renaming to
FieldDescriptionfor clarity- Documenting the context requirements more explicitly
Would you like to discuss the proposed naming changes and their implications?
packages/connect-react/src/components/Field.tsx (1)
24-26: LGTM! Clean props destructuring.The addition of
enableDebuggingto the destructured props is well-structured and follows TypeScript best practices.packages/connect-react/src/hooks/form-context.tsx (6)
104-107: Same type mismatch concerns as aboveYou’re using
__enableDebugging === trueto derive a boolean from a prop currently typed as a string inComponentFormProps. This logic may fail or always be false ifenableDebuggingis actually a string. Please see the earlier comment regarding updating the prop’s type definition.
158-158: Relying on react-query for error handlingThis async call is wrapped by react-query, which should handle errors gracefully via the
queryFnlogic. Just ensure that failures are caught and fed back intohandleSdkErrorsor another dedicated routine if needed.
160-168: Verify merging ofobservationsand__errorsRight now, you handle observations first and then fallback to
__errors. If bothobservationsand__errorsexist, some errors may be overlooked. Verify this logic to ensure you surface all SDK errors consistently.
269-283: New function for updating configuration errors looks goodThe function properly segregates validation for hidden/optional props and records them in
errorsaccordingly. This helps keep your regular form validations distinct from SDK-level errors stored insdkErrors. Good approach.
302-307: Clear approach for syncing external SDK errorsInvoking
handleSdkErrorsagain when__sdkErrorschanges ensures synchronization with external error triggers. This is a straightforward and effective way to stay in sync with newly surfaced SDK issues.
552-553: Attributes appear consistent with the new error handling flowExposing
sdkErrorsandenableDebuggingin the returned context object is a logical extension of your overall debugging strategy. Just be mindful of the earlier suggestion to unify the type definition forenableDebugging.packages/connect-react/src/hooks/form-field-context.tsx (1)
19-20: Expanded context properties align with the new debugging and error handling approachThe addition of
errorsandenableDebugginghelps each field manage local debugging states and error messages. This change is consistent with the broader updates in theFormContext.packages/connect-react/src/components/Errors.tsx (2)
4-6: LGTM! Good use of type safety for error alerts.The addition of
ConfigurablePropAlerttype andAlertcomponent improves type safety and provides a consistent error display mechanism.
30-36: LGTM! Well-structured error transformation.Good job transforming error strings into structured alert objects with consistent types.
packages/connect-react/src/components/InternalField.tsx (1)
18-19: LGTM! Good addition of error handling context.Adding
errorsandenableDebuggingto form context enhances error handling capabilities.packages/connect-react/src/components/InternalComponentForm.tsx (2)
39-42: LGTM! State management for SDK errors.The state initialization for SDK errors is well-structured and properly typed.
164-164: LGTM! Conditional error rendering.The error rendering is properly guarded by the
enableDebuggingflag.packages/connect-react/examples/nextjs/package.json (1)
10-10: Verify compatibility with the new SDK version.The SDK version has been updated to support the new error handling features.
✅ Verification successful
🌐 Web query:
What are the breaking changes in @pipedream/sdk version 1.3.2 compared to 1.1.4?Length of output: 5103
SDK Compatibility Update Confirmed
The upgrade to
@pipedream/sdkversion 1.3.2 introduces several breaking changes compared to version 1.1.4. In particular, you’ll need to ensure that your codebase reflects these changes:
- Component Structure: Transition from directly exported async functions to using
defineComponent({ async run({ steps, $ }) { ... } }).- Data Exporting: Replace implicit
thisassignments with explicit$.export("key", value)calls.- Workflow Control: Replace
$end("Message")with$.flow.exit("Message")for managing early termination.- Module Imports: Adopt ES module import syntax instead of using CommonJS
require.- Authentication Handling: Ensure that app connections are set via explicit prop definitions.
Please review all parts of the code (especially custom step definitions and error handling sections) in the Next.js example to confirm they conform to these updated patterns.
packages/connect-react/README.md (1)
152-156: LGTM! Clear and comprehensive documentation.The documentation for the new props is well-written and properly explains their purpose and relationship.
| const handleSdkErrors = (o: unknown[] | unknown | undefined) => { | ||
| let newErrors = [ | ||
| ...sdkErrors, | ||
| ] | ||
| if (!o) return | ||
| const handleObservationErrors = (observation: never) => { | ||
| const name: string = observation.err?.name | ||
| const message: string = observation.err?.message | ||
| if (name && message) return { | ||
| name, | ||
| message, | ||
| } as Record<string, string> | ||
| } | ||
|
|
||
| if (Array.isArray(o) && o.length > 0) { | ||
| for (let i = 0; i < o.length; i++) { | ||
| const item = o[i] | ||
| if (typeof item === "string") { | ||
| try { | ||
| const json = JSON.parse(item) | ||
| const name = json.name | ||
| const message = json.message | ||
| if (name && message) { | ||
| newErrors.push({ | ||
| name, | ||
| message, | ||
| } as Record<string, string>) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (e) { | ||
| // pass | ||
| } | ||
| } else if (typeof item === "object" && "name" in item && "message" in item) { | ||
| const name = item.name | ||
| const message = item.message | ||
| if (name && message) { | ||
| newErrors.push({ | ||
| name, | ||
| message, | ||
| } as Record<string, string>) | ||
| } | ||
| } else if (typeof item === "object" && item.k === "error") { | ||
| const res = handleObservationErrors(item) | ||
| if (res) newErrors.push(res) | ||
| } | ||
| } | ||
| } else if (typeof o === "object" && "os" in o || "observations" in o) { | ||
| const os = o.os || o.observations | ||
| if (Array.isArray(os) && os.length > 0) { | ||
| newErrors.push( | ||
| ...os.filter((it) => it.k === "error") | ||
| .map(handleObservationErrors) | ||
| .filter((e) => e !== undefined), | ||
| ) | ||
| } | ||
| } else if (typeof o === "object" && "message" in o) { | ||
| // Handle HTTP errors thrown by the SDK | ||
| try { | ||
| const json = JSON.parse(o.message) | ||
| const data = json.data | ||
| if (data && "observations" in data) { | ||
| const obs = data.observations || [] | ||
| if (obs && obs.length > 0) { | ||
| const res = obs.filter((it) => it.k === "error") | ||
| ?.map(handleObservationErrors) | ||
| ?.filter((e) => e !== undefined) || [] | ||
| newErrors.push( | ||
| ...res, | ||
| ) | ||
| } | ||
| } else if (data && "error" in data && "details" in data) { | ||
| newErrors.push({ | ||
| name: data.error, | ||
| message: JSON.stringify(data.details), | ||
| // message: ` // TODO: It would be nice to render the JSON in markdown | ||
| // \`\`\`json | ||
| // ${JSON.stringify(data.details)} | ||
| // \`\`\` | ||
| // `, | ||
| // }) | ||
| }) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (e) { | ||
| // pass | ||
| } | ||
| } else { | ||
| newErrors = [] | ||
| } | ||
| setSdkErrors(newErrors) | ||
| } |
There was a problem hiding this comment.
Potential data loss when encountering unrecognized error formats
In the else { newErrors = [] } branch near line 526, the code clears all existing errors if the incoming object doesn't match recognized patterns. This might inadvertently discard previously collected errors. Consider skipping unrecognized inputs instead:
- } else {
- newErrors = []
+ } else {
+ // Skip unrecognized error formats instead of clearing existing errorsBreaking this function into modular parsers or employing a stricter typed approach can improve maintainability.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSdkErrors = (o: unknown[] | unknown | undefined) => { | |
| let newErrors = [ | |
| ...sdkErrors, | |
| ] | |
| if (!o) return | |
| const handleObservationErrors = (observation: never) => { | |
| const name: string = observation.err?.name | |
| const message: string = observation.err?.message | |
| if (name && message) return { | |
| name, | |
| message, | |
| } as Record<string, string> | |
| } | |
| if (Array.isArray(o) && o.length > 0) { | |
| for (let i = 0; i < o.length; i++) { | |
| const item = o[i] | |
| if (typeof item === "string") { | |
| try { | |
| const json = JSON.parse(item) | |
| const name = json.name | |
| const message = json.message | |
| if (name && message) { | |
| newErrors.push({ | |
| name, | |
| message, | |
| } as Record<string, string>) | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| } catch (e) { | |
| // pass | |
| } | |
| } else if (typeof item === "object" && "name" in item && "message" in item) { | |
| const name = item.name | |
| const message = item.message | |
| if (name && message) { | |
| newErrors.push({ | |
| name, | |
| message, | |
| } as Record<string, string>) | |
| } | |
| } else if (typeof item === "object" && item.k === "error") { | |
| const res = handleObservationErrors(item) | |
| if (res) newErrors.push(res) | |
| } | |
| } | |
| } else if (typeof o === "object" && "os" in o || "observations" in o) { | |
| const os = o.os || o.observations | |
| if (Array.isArray(os) && os.length > 0) { | |
| newErrors.push( | |
| ...os.filter((it) => it.k === "error") | |
| .map(handleObservationErrors) | |
| .filter((e) => e !== undefined), | |
| ) | |
| } | |
| } else if (typeof o === "object" && "message" in o) { | |
| // Handle HTTP errors thrown by the SDK | |
| try { | |
| const json = JSON.parse(o.message) | |
| const data = json.data | |
| if (data && "observations" in data) { | |
| const obs = data.observations || [] | |
| if (obs && obs.length > 0) { | |
| const res = obs.filter((it) => it.k === "error") | |
| ?.map(handleObservationErrors) | |
| ?.filter((e) => e !== undefined) || [] | |
| newErrors.push( | |
| ...res, | |
| ) | |
| } | |
| } else if (data && "error" in data && "details" in data) { | |
| newErrors.push({ | |
| name: data.error, | |
| message: JSON.stringify(data.details), | |
| // message: ` // TODO: It would be nice to render the JSON in markdown | |
| // \`\`\`json | |
| // ${JSON.stringify(data.details)} | |
| // \`\`\` | |
| // `, | |
| // }) | |
| }) | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| } catch (e) { | |
| // pass | |
| } | |
| } else { | |
| newErrors = [] | |
| } | |
| setSdkErrors(newErrors) | |
| } | |
| const handleSdkErrors = (o: unknown[] | unknown | undefined) => { | |
| let newErrors = [ | |
| ...sdkErrors, | |
| ] | |
| if (!o) return | |
| const handleObservationErrors = (observation: never) => { | |
| const name: string = observation.err?.name | |
| const message: string = observation.err?.message | |
| if (name && message) return { | |
| name, | |
| message, | |
| } as Record<string, string> | |
| } | |
| if (Array.isArray(o) && o.length > 0) { | |
| for (let i = 0; i < o.length; i++) { | |
| const item = o[i] | |
| if (typeof item === "string") { | |
| try { | |
| const json = JSON.parse(item) | |
| const name = json.name | |
| const message = json.message | |
| if (name && message) { | |
| newErrors.push({ | |
| name, | |
| message, | |
| } as Record<string, string>) | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| } catch (e) { | |
| // pass | |
| } | |
| } else if (typeof item === "object" && "name" in item && "message" in item) { | |
| const name = item.name | |
| const message = item.message | |
| if (name && message) { | |
| newErrors.push({ | |
| name, | |
| message, | |
| } as Record<string, string>) | |
| } | |
| } else if (typeof item === "object" && item.k === "error") { | |
| const res = handleObservationErrors(item) | |
| if (res) newErrors.push(res) | |
| } | |
| } | |
| } else if (typeof o === "object" && "os" in o || "observations" in o) { | |
| const os = o.os || o.observations | |
| if (Array.isArray(os) && os.length > 0) { | |
| newErrors.push( | |
| ...os.filter((it) => it.k === "error") | |
| .map(handleObservationErrors) | |
| .filter((e) => e !== undefined), | |
| ) | |
| } | |
| } else if (typeof o === "object" && "message" in o) { | |
| // Handle HTTP errors thrown by the SDK | |
| try { | |
| const json = JSON.parse(o.message) | |
| const data = json.data | |
| if (data && "observations" in data) { | |
| const obs = data.observations || [] | |
| if (obs && obs.length > 0) { | |
| const res = obs.filter((it) => it.k === "error") | |
| ?.map(handleObservationErrors) | |
| ?.filter((e) => e !== undefined) || [] | |
| newErrors.push( | |
| ...res, | |
| ) | |
| } | |
| } else if (data && "error" in data && "details" in data) { | |
| newErrors.push({ | |
| name: data.error, | |
| message: JSON.stringify(data.details), | |
| // message: ` // TODO: It would be nice to render the JSON in markdown | |
| // \`\`\`json | |
| // ${JSON.stringify(data.details)} | |
| // \`\`\` | |
| // `, | |
| // }) | |
| }) | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| } catch (e) { | |
| // pass | |
| } | |
| } else { | |
| // Skip unrecognized error formats instead of clearing existing errors | |
| } | |
| setSdkErrors(newErrors) | |
| } |
There was a problem hiding this comment.
I'm not so sure about this one though.
There was a problem hiding this comment.
@jverce You're right to be skeptical. The current approach of clearing errors for unrecognized formats is likely more appropriate here. It ensures a clean error state and prevents stale errors from persisting, which is especially important in a form context. I'll retract my suggestion.
There was a problem hiding this comment.
I agree with the above coderabbitai response
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| dynamicProps, observations, errors: __errors, | ||
| } = result | ||
|
|
||
| // Prioritize errors from observations over the errors array |
There was a problem hiding this comment.
In the screenrecording that i shard here https://pipe-dream.slack.com/archives/C03NA8B4VA9/p1738714965904509, you can see what happens at 1:10 when we merge the two. In the builder we only show the configuration error, which comes from observations. The screenshots in this PR is what you get if we don't merge. I'm not sure if merging adds more context to the error. A lot of times it's confusing and not the actual error.
| const handleSdkErrors = (o: unknown[] | unknown | undefined) => { | ||
| let newErrors = [ | ||
| ...sdkErrors, | ||
| ] | ||
| if (!o) return | ||
| const handleObservationErrors = (observation: never) => { | ||
| const name: string = observation.err?.name | ||
| const message: string = observation.err?.message | ||
| if (name && message) return { | ||
| name, | ||
| message, | ||
| } as Record<string, string> | ||
| } | ||
|
|
||
| if (Array.isArray(o) && o.length > 0) { | ||
| for (let i = 0; i < o.length; i++) { | ||
| const item = o[i] | ||
| if (typeof item === "string") { | ||
| try { | ||
| const json = JSON.parse(item) | ||
| const name = json.name | ||
| const message = json.message | ||
| if (name && message) { | ||
| newErrors.push({ | ||
| name, | ||
| message, | ||
| } as Record<string, string>) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (e) { | ||
| // pass | ||
| } | ||
| } else if (typeof item === "object" && "name" in item && "message" in item) { | ||
| const name = item.name | ||
| const message = item.message | ||
| if (name && message) { | ||
| newErrors.push({ | ||
| name, | ||
| message, | ||
| } as Record<string, string>) | ||
| } | ||
| } else if (typeof item === "object" && item.k === "error") { | ||
| const res = handleObservationErrors(item) | ||
| if (res) newErrors.push(res) | ||
| } | ||
| } | ||
| } else if (typeof o === "object" && "os" in o || "observations" in o) { | ||
| const os = o.os || o.observations | ||
| if (Array.isArray(os) && os.length > 0) { | ||
| newErrors.push( | ||
| ...os.filter((it) => it.k === "error") | ||
| .map(handleObservationErrors) | ||
| .filter((e) => e !== undefined), | ||
| ) | ||
| } | ||
| } else if (typeof o === "object" && "message" in o) { | ||
| // Handle HTTP errors thrown by the SDK | ||
| try { | ||
| const json = JSON.parse(o.message) | ||
| const data = json.data | ||
| if (data && "observations" in data) { | ||
| const obs = data.observations || [] | ||
| if (obs && obs.length > 0) { | ||
| const res = obs.filter((it) => it.k === "error") | ||
| ?.map(handleObservationErrors) | ||
| ?.filter((e) => e !== undefined) || [] | ||
| newErrors.push( | ||
| ...res, | ||
| ) | ||
| } | ||
| } else if (data && "error" in data && "details" in data) { | ||
| newErrors.push({ | ||
| name: data.error, | ||
| message: JSON.stringify(data.details), | ||
| // message: ` // TODO: It would be nice to render the JSON in markdown | ||
| // \`\`\`json | ||
| // ${JSON.stringify(data.details)} | ||
| // \`\`\` | ||
| // `, | ||
| // }) | ||
| }) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (e) { | ||
| // pass | ||
| } | ||
| } else { | ||
| newErrors = [] | ||
| } | ||
| setSdkErrors(newErrors) | ||
| } |
There was a problem hiding this comment.
I'm not so sure about this one though.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/connect-react/src/components/InternalComponentForm.tsx (1)
164-164: Improve error rendering logic.The error rendering logic can be improved by extracting it into a separate component for better maintainability.
- { enableDebugging && sdkErrors?.map((e, idx) => <Alert prop={e} key={idx}/>)} + { enableDebugging && sdkErrors?.length > 0 && ( + <div role="alert" aria-label="SDK Errors"> + {sdkErrors.map((error, idx) => ( + <Alert + key={`sdk-error-${idx}`} + prop={error} + /> + ))} + </div> + )}types/src/index.ts (1)
406-411: Add JSDoc comments to the Observation type.The
Observationtype would benefit from documentation explaining the purpose of each field.+/** + * Represents an observation event in the system. + * @property ts - Timestamp of the observation. + * @property k - Key or type of the observation. + * @property h - Optional human-readable description. + * @property err - Optional error associated with the observation. + */ export type Observation = { ts: number; k: string; h?: string; err?: Error; }packages/connect-react/src/hooks/form-context.tsx (1)
449-463: Add error type validation.The error parsing functions should validate the shape of the error object using type guards.
+interface JsonError { + name: string; + message: string; +} + +const isJsonError = (json: unknown): json is JsonError => + typeof json === "object" && + json !== null && + "name" in json && + "message" in json && + typeof json.name === "string" && + typeof json.message === "string" + const errorFromString = (item: string, ret: SdkError[]) => { try { const json = JSON.parse(item) + if (!isJsonError(json)) return + const err: SdkError = { name: json.name, message: json.message, } - if (err.name && err.message) { - ret.push(err) - } + ret.push(err) } catch (e) { // pass } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/connect-react/README.md(1 hunks)packages/connect-react/examples/nextjs/src/app/page.tsx(2 hunks)packages/connect-react/src/components/ComponentForm.tsx(1 hunks)packages/connect-react/src/components/InternalComponentForm.tsx(5 hunks)packages/connect-react/src/hooks/form-context.tsx(10 hunks)types/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connect-react/src/components/ComponentForm.tsx
🧰 Additional context used
📓 Learnings (1)
packages/connect-react/src/hooks/form-context.tsx (1)
Learnt from: jverce
PR: PipedreamHQ/pipedream#15479
File: packages/connect-react/src/components/ComponentForm.tsx:23-24
Timestamp: 2025-02-05T21:58:03.118Z
Learning: In the connect-react package, the `enableDebugging` property should be of type `boolean` as it's used for toggling debugging features and conditional rendering.
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/README.md
[warning] File ignored because of a matching ignore pattern. Use '--no-ignore' to disable file ignore settings or use '--no-warn-ignored' to suppress this warning
types/src/index.ts
[error] 1-1: Definition for rule '@typescript-eslint/ban-types' was not found
🪛 Biome (1.9.4)
packages/connect-react/src/components/InternalComponentForm.tsx
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (5)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
28-31: LGTM! State management for SDK response.The state is correctly initialized as
undefinedand properly typed withunknown | undefined.packages/connect-react/src/components/InternalComponentForm.tsx (2)
1-18: LGTM! Import organization.The imports are well-organized and correctly typed.
39-42: LGTM! State initialization for SDK errors.The state is correctly initialized as an empty array and properly typed with
ConfigurablePropAlert[].types/src/index.ts (1)
401-404: LGTM! SDK error type definition.The
SdkErrortype is well-defined with required properties.packages/connect-react/src/hooks/form-context.tsx (1)
30-30: LGTM! Form context type definitions.The
sdkErrorsandenableDebuggingproperties are correctly added to theFormContexttype.Also applies to: 44-44
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/connect-react/src/components/Errors.tsx (1)
40-40: Consider using a more stable key for the Alert components.While the implementation follows the previous review suggestion about narrow screen readability, using array indices as React keys might cause issues with component reconciliation if the error array changes.
Consider using a combination of the error content and index to create a more stable key:
- <div className="pd-errors" style={baseStyles}>{formattedErrors.map((fe, idx: number) => <Alert prop={fe} key={idx}/>)}</div> + <div className="pd-errors" style={baseStyles}>{formattedErrors.map((fe, idx: number) => <Alert prop={fe} key={`${fe.content}-${idx}`}/>)}</div>packages/connect-react/src/types.ts (1)
6-11: Consider adding JSDoc comments to improve type documentation.The
Observationtype could benefit from documentation explaining the purpose of each field (ts, k, h, err).+/** + * Represents an observation event with timestamp and optional error information. + * @property ts - Timestamp of the observation + * @property k - Key/type of the observation + * @property h - Optional human-readable description + * @property err - Optional error object + */ export type Observation = { ts: number; k: string; h?: string; err?: Error; }packages/connect-react/src/hooks/form-context.tsx (1)
102-111: Ensure consistent state initialization.The state initialization for
enableDebuggingcould be simplified and made more consistent with other state initializations in the file.- const [ - enableDebugging - , - ] = useState<boolean>(__enableDebugging === true) + const [enableDebugging, setEnableDebugging] = useState<boolean>(__enableDebugging === true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/connect-react/src/components/Errors.tsx(1 hunks)packages/connect-react/src/hooks/form-context.tsx(10 hunks)packages/connect-react/src/hooks/form-field-context.tsx(1 hunks)packages/connect-react/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connect-react/src/hooks/form-field-context.tsx
🧰 Additional context used
📓 Learnings (1)
packages/connect-react/src/hooks/form-context.tsx (1)
Learnt from: jverce
PR: PipedreamHQ/pipedream#15479
File: packages/connect-react/src/components/ComponentForm.tsx:23-24
Timestamp: 2025-02-05T21:58:03.118Z
Learning: In the connect-react package, the `enableDebugging` property should be of type `boolean` as it's used for toggling debugging features and conditional rendering.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
🔇 Additional comments (10)
packages/connect-react/src/components/Errors.tsx (5)
4-4: LGTM!The new imports are necessary for the enhanced error handling implementation.
Also applies to: 6-6
14-17: LGTM!The initialization with default values follows the previous review suggestion, making the code cleaner and reducing the need for multiple null checks.
19-21: LGTM!The simplified error check follows the previous review suggestion, making the code more concise.
26-32: LGTM!The error formatting logic is well-structured and type-safe, properly mapping error strings to
ConfigurablePropAlertobjects.
34-37: LGTM!The grid layout provides a clean and structured presentation for error messages.
packages/connect-react/src/types.ts (1)
1-4: LGTM! Well-structured error type.The
SdkErrortype follows the standard error object pattern with required name and message properties.packages/connect-react/src/hooks/form-context.tsx (4)
17-19: LGTM! Clean import of new types.The imports are properly structured and types are correctly imported from the types file.
23-45: LGTM! Well-structured FormContext type.The FormContext type is properly extended with new properties:
sdkErrorsfor SDK-related errorsenableDebuggingfor debugging features
297-309: LGTM! Proper effect dependencies.The useEffect hooks correctly specify their dependencies and handle updates to sdkResponse and configuration changes.
442-542: 🛠️ Refactor suggestionRefactor error handling logic for better maintainability.
The
handleSdkErrorsfunction is complex and handles multiple error formats. Consider breaking it down into smaller, more focused functions using a strategy pattern.+type ErrorHandler = { + canHandle: (input: unknown) => boolean; + handle: (input: unknown) => SdkError[]; +}; +class StringErrorHandler implements ErrorHandler { + canHandle = (input: unknown): input is string => + typeof input === "string"; + + handle(input: string): SdkError[] { + try { + const json = JSON.parse(input); + return this.validateError({ + name: json.name, + message: json.message, + }); + } catch { + return []; + } + } + + private validateError(err: Partial<SdkError>): SdkError[] { + return err.name && err.message ? [err as SdkError] : []; + } +} +const errorHandlers: ErrorHandler[] = [ + new StringErrorHandler(), + // Add other handlers +]; const handleSdkErrors = (sdkResponse: unknown[] | unknown | undefined) => { if (!sdkResponse) return; const newErrors = Array.isArray(sdkResponse) ? sdkResponse.flatMap(item => { const handler = errorHandlers.find(h => h.canHandle(item)); return handler ? handler.handle(item) : []; }) : errorHandlers .find(h => h.canHandle(sdkResponse)) ?.handle(sdkResponse) ?? []; setSdkErrors(newErrors); };Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/connect-react/src/components/Errors.tsx (4)
13-25: Consider further simplifying the error checks.The error checks can be combined into a single return statement for better readability.
- if (!enableDebugging) { - return null - } - - if (!errors[prop.name]) { - return null - } + if (!enableDebugging || !errors[prop.name]) { + return null; + }
30-36: Add type safety to error mapping.Consider adding type assertions or type guards to ensure type safety in the error mapping.
- const formattedErrors: ConfigurablePropAlert[] = errors[prop.name].map((e) => { + const formattedErrors: ConfigurablePropAlert[] = errors[prop.name].map((e: string) => { return { type: "alert", alertType: "error", content: e, - } + } as const; })
38-41: Consider adding responsive grid styles.The current grid layout might not be optimal for all screen sizes.
const baseStyles = { display: "grid", - gridTemplateColumns: "max-content", + gridTemplateColumns: "minmax(0, max-content)", + gap: "0.5rem", }
43-49: Consider memoizing FormattedErrors component.Since FormattedErrors is a component defined within another component, it should be memoized to prevent unnecessary re-renders.
+ const FormattedErrors = React.memo(() => { - const FormattedErrors = () => { return <>{formattedErrors.map((fe, idx: number) => <Alert prop={fe} key={idx}/>)}</> - } + })packages/connect-react/src/components/InternalComponentForm.tsx (3)
1-22: Consider extracting constants to a separate file.The
alwaysShowSdkErrorsarray could be moved to a constants file for better maintainability and reusability.Consider creating a new file
constants.ts:export const SDK_ERROR_TYPES = { ALWAYS_SHOW: ['ConfigurationError'], } as const;
39-39: Optimize error visibility check.The current implementation filters the array on every render. Consider using
somefor better performance.- const showSdkErrors = enableDebugging || __sdkErrors.filter((e) => alwaysShowSdkErrors.includes(e.name)).length > 0 + const showSdkErrors = enableDebugging || __sdkErrors.some((e) => alwaysShowSdkErrors.includes(e.name))
170-170: Improve SDK error rendering readability.The current inline rendering could be extracted for better readability.
- { showSdkErrors && sdkErrors?.map((e, idx) => <Alert prop={e} key={idx}/>)} + { showSdkErrors && ( + <div className="sdk-errors"> + {sdkErrors?.map((e, idx) => ( + <Alert prop={e} key={idx}/> + ))} + </div> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/connect-react/src/components/Errors.tsx(1 hunks)packages/connect-react/src/components/Field.tsx(1 hunks)packages/connect-react/src/components/InternalComponentForm.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connect-react/src/components/Field.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect-react/src/components/InternalComponentForm.tsx
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
🔇 Additional comments (1)
packages/connect-react/src/components/Errors.tsx (1)
1-11: LGTM!Clean and well-structured imports and type definitions.
| const [ | ||
| sdkErrors, | ||
| setSdkErrors, | ||
| ] = useState<ConfigurablePropAlert[]>([]) | ||
|
|
||
| useEffect(() => { | ||
| if (submitting) setSdkErrors([]) | ||
| else { | ||
| if (__sdkErrors && __sdkErrors.length) { | ||
| setSdkErrors(__sdkErrors.map((e) => { | ||
| return { | ||
| type: "alert", | ||
| alertType: "error", | ||
| content: `# ${e.name}\n${e.message}`, | ||
| } as ConfigurablePropAlert | ||
| })) | ||
| } | ||
| } | ||
| }, [ | ||
| __sdkErrors, | ||
| submitting, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve SDK error state management.
The current implementation has several areas for improvement:
- Use optional chaining as suggested by static analysis.
- Add error type safety.
- Consider memoizing the error transformation.
useEffect(() => {
if (submitting) setSdkErrors([])
else {
- if (__sdkErrors && __sdkErrors.length) {
+ if (__sdkErrors?.length) {
+ const transformError = (e: typeof __sdkErrors[number]): ConfigurablePropAlert => ({
+ type: "alert",
+ alertType: "error",
+ content: `# ${e.name}\n${e.message}`,
+ });
+
- setSdkErrors(__sdkErrors.map((e) => {
- return {
- type: "alert",
- alertType: "error",
- content: `# ${e.name}\n${e.message}`,
- } as ConfigurablePropAlert
- }))
+ setSdkErrors(__sdkErrors.map(transformError))
}
}
}, [__sdkErrors, submitting]);📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [ | |
| sdkErrors, | |
| setSdkErrors, | |
| ] = useState<ConfigurablePropAlert[]>([]) | |
| useEffect(() => { | |
| if (submitting) setSdkErrors([]) | |
| else { | |
| if (__sdkErrors && __sdkErrors.length) { | |
| setSdkErrors(__sdkErrors.map((e) => { | |
| return { | |
| type: "alert", | |
| alertType: "error", | |
| content: `# ${e.name}\n${e.message}`, | |
| } as ConfigurablePropAlert | |
| })) | |
| } | |
| } | |
| }, [ | |
| __sdkErrors, | |
| submitting, | |
| ]); | |
| const [ | |
| sdkErrors, | |
| setSdkErrors, | |
| ] = useState<ConfigurablePropAlert[]>([]) | |
| useEffect(() => { | |
| if (submitting) setSdkErrors([]) | |
| else { | |
| if (__sdkErrors?.length) { | |
| const transformError = (e: typeof __sdkErrors[number]): ConfigurablePropAlert => ({ | |
| type: "alert", | |
| alertType: "error", | |
| content: `# ${e.name}\n${e.message}`, | |
| }); | |
| setSdkErrors(__sdkErrors.map(transformError)) | |
| } | |
| } | |
| }, [ | |
| __sdkErrors, | |
| submitting, | |
| ]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
bzwrk
deleted the
biz/dj-2780-add-support-for-configurationerror-in-connect-react
branch
Before
After
Summary by CodeRabbit
New Features
Style
Chores