Biz/dj 2788 google sheets submission doesnt work #15179
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces enhancements to dynamic property management in a React Next.js application. The changes span across multiple files, focusing on improving the handling of dynamic properties in form contexts and component interactions. The modifications include adding state management for dynamic properties, updating the structure of query keys, and adjusting action submission processes to better integrate dynamic property handling. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 2
🧹 Nitpick comments (3)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
46-46: Add trailing comma.A trailing comma in object literals or function calls can help reduce diff churn in future changes:
- dynamicPropsId: dynamicProps?.id + dynamicPropsId: dynamicProps?.id,🧰 Tools
🪛 eslint
[error] 46-47: Missing trailing comma.
(comma-dangle)
🪛 GitHub Check: Lint Code Base
[failure] 46-46:
Missing trailing commapackages/connect-react/src/hooks/form-context.tsx (2)
132-135: Consider inlining if not reused elsewhere.
queryKeyInputis assigned only once and immediately spread into the queryKey array. If it’s not reused in multiple places, you could inline it directly into thequeryKeyfor clarity.
142-142: Include trailing comma in array literals.Adding a trailing comma maintains consistency with the lint rules and eases future diffs:
queryKey: [ "dynamicProps", - queryKeyInput + queryKeyInput, ],🧰 Tools
🪛 eslint
[error] 142-143: Missing trailing comma.
(comma-dangle)
🪛 GitHub Check: Lint Code Base
[failure] 142-142:
Missing trailing comma
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/connect-react/examples/nextjs/src/app/page.tsx(3 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/hooks/form-context.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/connect-react/package.json
🧰 Additional context used
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/hooks/form-context.tsx
[failure] 142-142:
Missing trailing comma
packages/connect-react/examples/nextjs/src/app/page.tsx
[failure] 9-9:
A space is required after '{'
[failure] 9-9:
A space is required before '}'
[failure] 28-28:
Unexpected any. Specify a different type
[failure] 46-46:
Missing trailing comma
🪛 eslint
packages/connect-react/examples/nextjs/src/app/page.tsx
[error] 9-9: A space is required after '{'.
(object-curly-spacing)
[error] 9-9: A space is required before '}'.
(object-curly-spacing)
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/examples/nextjs/src/app/page.tsx
[error] 9-9: A space is required after '{' (object-curly-spacing)
🔇 Additional comments (1)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
38-38: Confirm that onUpdateDynamicProps is properly handled.Ensure that the
onUpdateDynamicPropscallback is triggered only when relevant changes to dynamic props occur, preventing unnecessary re-renders or network calls.
| const [ | ||
| dynamicProps, | ||
| setDynamicProps, | ||
| ] = useState<DynamicProps<any>>(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a more precise type.
Using any for DynamicProps<any> reduces type safety. Provide a more specific type or introduce a generic that reflects the shape of your configurable props.
🧰 Tools
🪛 eslint
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: Lint Code Base
[failure] 28-28:
Unexpected any. Specify a different type
| ComponentFormContainer, FrontendClientProvider, | ||
| } from "@pipedream/connect-react"; | ||
| import { fetchToken } from "./actions"; | ||
| import {DynamicProps} from "../../../../src"; |
There was a problem hiding this comment.
Fix spacing for import statement.
Per lint rules, a space is required after { and before } in your import statement:
-import {DynamicProps} from "../../../../src";
+import { DynamicProps } from "../../../../src";📝 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.
| import {DynamicProps} from "../../../../src"; | |
| import { DynamicProps } from "../../../../src"; |
🧰 Tools
🪛 eslint
[error] 9-9: A space is required after '{'.
(object-curly-spacing)
[error] 9-9: A space is required before '}'.
(object-curly-spacing)
🪛 GitHub Check: Lint Code Base
[failure] 9-9:
A space is required after '{'
[failure] 9-9:
A space is required before '}'
🪛 GitHub Actions: Pull Request Checks
[error] 9-9: A space is required after '{' (object-curly-spacing)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
9-9: Consider restructuring the import path.The deep relative import path (
../../../../src) suggests this component might be accessing internals it shouldn't or that the package structure could be improved.Consider:
- Moving shared types to a dedicated types package
- Publishing these types as part of the public API
- Using path aliases to avoid deep relative imports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/connect-react/examples/nextjs/src/app/page.tsx(3 hunks)packages/connect-react/src/hooks/form-context.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connect-react/src/hooks/form-context.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/connect-react/examples/nextjs/src/app/page.tsx
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 eslint
packages/connect-react/examples/nextjs/src/app/page.tsx
[error] 28-28: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/examples/nextjs/src/app/page.tsx
[error] 28-28: The {} ("empty object") type is not allowed. Consider using object for "any object" or unknown for "any value" instead.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pnpm publish
🔇 Additional comments (1)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
38-38: Verify dynamic props update handling.The addition of
onUpdateDynamicPropssuggests this is part of fixing the Google Sheets submission issue. Please ensure that dynamic properties are being correctly updated and persisted before form submission.✅ Verification successful
Dynamic props update handling is correctly implemented.
The
onUpdateDynamicPropscallback is properly typed, consistently implemented in the form context, and correctly used in the example. The implementation follows React's controlled component pattern, ensuring proper state management.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of onUpdateDynamicProps to verify consistent handling rg -l "onUpdateDynamicProps" --type typescriptLength of output: 81
Script:
#!/bin/bash # Search for onUpdateDynamicProps in TypeScript/JavaScript files rg -l "onUpdateDynamicProps" -g "*.{ts,tsx,js,jsx}" # Search for ComponentFormContainer definition rg -l "ComponentFormContainer" -g "*.{ts,tsx,js,jsx}" # Try to find the type definition or interface ast-grep --pattern 'interface $_ { $$$ onUpdateDynamicProps$$$ $$$ }'Length of output: 502
Script:
#!/bin/bash # Check the implementation in form context rg "onUpdateDynamicProps" packages/connect-react/src/hooks/form-context.tsx -A 5 # Look for the prop definition in ComponentForm rg "onUpdateDynamicProps" packages/connect-react/src/components/ComponentForm.tsx -A 5 # Check ComponentFormContainer implementation rg "onUpdateDynamicProps" packages/connect-react/src/components/ComponentFormContainer.tsx -A 5Length of output: 666
| const [ | ||
| dynamicProps, | ||
| setDynamicProps, | ||
| ] = useState<DynamicProps<{}>>(); | ||
|
|
There was a problem hiding this comment.
Improve type safety for dynamic props.
The empty object type {} is not type-safe and allows any non-nullish value. This could lead to runtime errors if the dynamic props have a specific expected shape.
Consider one of these approaches:
- ] = useState<DynamicProps<{}>>();
+ // If you expect any object:
+ ] = useState<DynamicProps<object>>();
+ // If you know the shape:
+ ] = useState<DynamicProps<{
+ // Define expected properties here
+ propertyName: PropertyType;
+ }>>();Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 eslint
[error] 28-28: The {} ("empty object") type allows any non-nullish value, including literals like 0 and "".
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead.
(@typescript-eslint/no-empty-object-type)
🪛 GitHub Actions: Pull Request Checks
[error] 28-28: The {} ("empty object") type is not allowed. Consider using object for "any object" or unknown for "any value" instead.
| userId, | ||
| actionId: "slack-send-message", | ||
| configuredProps, | ||
| dynamicPropsId: dynamicProps?.id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for missing dynamic props.
While optional chaining prevents runtime errors, we should handle the case where dynamic props are required but missing.
Consider adding validation:
await client.actionRun({
userId,
actionId: "slack-send-message",
configuredProps,
- dynamicPropsId: dynamicProps?.id,
+ dynamicPropsId: dynamicProps?.id ?? (() => {
+ throw new Error('Dynamic props are required for this action');
+ })(),
});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
Line range hint
44-53: Enhance error handling in onSubmit.The current error handling only logs to console. Consider:
- Providing user feedback for failures
- Adding specific error handling for dynamic props related failures
onSubmit={async () => { try { await client.actionRun({ userId, actionId: "slack-send-message", configuredProps, dynamicPropsId, }); + // Show success message to user } catch (error) { - console.error("Action run failed:", error); + // Determine error type and show appropriate message + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + console.error("Action run failed:", errorMessage); + // Implement user notification mechanism } }}
🧹 Nitpick comments (1)
packages/connect-react/examples/nextjs/src/app/page.tsx (1)
24-27: Consider documenting state purpose and handling undefined state.While the type safety is good, consider:
- Adding JSDoc to document the purpose of
dynamicPropsId- Handling the undefined state in the UI to prevent potential user confusion
+ // ID received from dynamic props to track the current form state const [ dynamicPropsId, setDynamicPropsId, ] = useState<string | undefined>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/examples/nextjs/src/app/page.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
packages/connect-react/examples/nextjs/src/app/page.tsx (2)
29-31: 🛠️ Refactor suggestionEnhance type safety and error handling for dynamic props.
The current implementation could be more robust:
- The type definition is inline and minimal
- No error handling for undefined id
+ interface DynamicProps { + id: string | undefined; + // Add other expected properties + } + - const handleDynamicProps = (dynamicProps: { id: string | undefined }) => { + const handleDynamicProps = (dynamicProps: DynamicProps) => { + if (!dynamicProps?.id) { + console.warn('Received dynamic props without an ID'); + } setDynamicPropsId(dynamicProps.id); };Likely invalid or redundant comment.
41-41: Verify impact on Google Sheets functionality.While these changes look correct for Slack message submission, the PR objective mentions fixing Google Sheets submission issues. Please clarify:
- How does this change address Google Sheets functionality?
- Has this been tested with Google Sheets components?
Also applies to: 49-49
WHY
Summary by CodeRabbit
New Features
Version Update
@pipedream/connect-reactpackage to version1.0.0-preview.16.Improvements