Fix dynamic props in the Connect demo app #14887
Conversation
* Switch from snake to camel case to match API response * Upgrade SDK
jverce
added
bug
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
@jverce is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes involve renaming properties within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
🧹 Outside diff range and nitpick comments (4)
packages/connect-react/src/hooks/form-context.tsx (4)
12-12: LGTM: Property naming convention updated correctly.The property renaming from snake case to camel case aligns with the PR objectives and maintains consistency with the API response format.
Consider addressing the TODO comment on line 12 or documenting its purpose.
Also applies to: 16-16
Line range hint
172-177: LGTM: String validation logic is correct.The string type validation is implemented correctly. However, there's a TODO comment about app auth type that needs to be addressed.
Would you like help implementing the auth type validation for the 'app' type?
Line range hint
182-192: Consider improving variable naming for clarity.While the logic is correct, the variable name
_queryDisabledIdxcould be more descriptive to better convey its purpose.Consider renaming to something more descriptive like
firstIncompleteRequiredPropIndex.
Snake case property still present and needs to be updated
The verification confirms that there is still one instance of snake case
configurable_propsin the form-context.tsx file that needs to be updated to camelCase for consistency:let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || [];The property should be updated to
configurablePropsto match the camelCase convention.🔗 Analysis chain
Line range hint
1-300: Verify SDK upgrade impact.While the property naming changes are mostly complete (except for one instance), the SDK upgrade mentioned in the PR objectives isn't visible in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining snake_case properties and SDK version changes echo "Checking for remaining snake_case properties..." rg -g '*.{ts,tsx}' 'configurable_props' echo "Checking for SDK version changes..." git diff package.json | grep "@pipedream/sdk"Length of output: 741
Script:
#!/bin/bash # Let's check the SDK version changes in all package.json files echo "Checking SDK version in package.json files..." fd package.json --exec grep -l "@pipedream/sdk" {} \; echo "Checking git diff for these files..." fd package.json --exec sh -c 'echo "=== {} ===" && git diff {} | grep -A 2 -B 2 "@pipedream/sdk"' \; echo "Checking imports from SDK in the form-context file..." rg -A 2 "from \"@pipedream/sdk\"" packages/connect-react/src/hooks/form-context.tsxLength of output: 66054
Script:
#!/bin/bash # Let's check the SDK version changes in the specific package.json files we found echo "=== Checking SDK version in connect-react package.json ===" git diff packages/connect-react/package.json | grep -A 2 -B 2 "@pipedream/sdk" echo "=== Checking SDK version in sdk package.json ===" git diff packages/sdk/package.json | grep "version" echo "=== Checking configurable_props usage in form-context.tsx ===" cat packages/connect-react/src/hooks/form-context.tsx | grep -A 2 configurable_propsLength of output: 756
📜 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 (1)
packages/connect-react/src/hooks/form-context.tsx(5 hunks)
| const { dynamicProps } = await client.componentReloadProps(componentReloadPropsInput); | ||
| // XXX what about if null? | ||
| // TODO observation errors, etc. | ||
| if (dynamic_props) { | ||
| setDynamicProps(dynamic_props); | ||
| if (dynamicProps) { | ||
| setDynamicProps(dynamicProps); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the query function implementation.
The current implementation has a few concerns:
- The function mutates state and returns an empty array, which is an anti-pattern in React Query.
- There's no error handling for the case when
dynamicPropsis null.
Consider refactoring to:
- const { dynamicProps } = await client.componentReloadProps(componentReloadPropsInput);
- if (dynamicProps) {
- setDynamicProps(dynamicProps);
- }
- setReloadPropIdx(undefined);
- return []; // XXX ok to mutate above and not look at data?
+ const response = await client.componentReloadProps(componentReloadPropsInput);
+ if (!response.dynamicProps) {
+ throw new Error('Dynamic props not found');
+ }
+ return response.dynamicProps;Then handle the state updates in onSuccess callback:
useQuery({
// ...
onSuccess: (data) => {
setDynamicProps(data);
setReloadPropIdx(undefined);
},
});|
|
||
| // XXX fix types of dynamicProps, props.component so this type decl not needed | ||
| let configurableProps: T = dynamicProps?.configurable_props || formProps.component.configurable_props || []; | ||
| let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || []; |
There was a problem hiding this comment.
Fix inconsistent property naming and improve type safety.
The line still uses snake_case configurable_props from component prop, which is inconsistent with the camelCase convention being adopted.
- let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurable_props || [];
+ let configurableProps: T = dynamicProps?.configurableProps || formProps.component.configurableProps || [];Also, consider adding runtime type checking for the assigned value since the type assertion to T might hide potential type mismatches.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
Changelog
Summary by CodeRabbit
DynamicPropsandFormContexttypes for consistency and clarity.1.0.0-preview.7to1.0.0-preview.8.