Biz/dj 2786 fix issue with loading dynamic props #15142
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request involves two primary changes: updating the version of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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: 1
🧹 Nitpick comments (1)
packages/connect-react/src/hooks/form-context.tsx (1)
138-138: Add a trailing comma to the array for compliance with the style rulesThis line triggers a lint error about the missing trailing comma. Update the array as follows:
- queryKey: [ - queryKey - ], + queryKey: [ + queryKey, + ],🧰 Tools
🪛 eslint
[error] 138-139139: Missing trailing comma.
(comma-dangle)
🪛 GitHub Check: Lint Code Base
[failure] 138-138138:
Missing trailing comma
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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
🪛 eslint
packages/connect-react/src/hooks/form-context.tsx
[error] 132-132132: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 132-132132: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/hooks/form-context.tsx
[failure] 132-132132:
Expected newline between test and consequent of ternary expression
[failure] 132-132132:
Expected newline between consequent and alternate of ternary expression
[failure] 138-138138:
Missing trailing comma
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/src/hooks/form-context.tsx
[error] 132-132132: Expected newline between test and consequent of ternary expression (multiline-ternary)
| @@ -129,12 +129,13 @@ | |||
| configuredProps, | |||
| dynamicPropsId: dynamicProps?.id, | |||
| }; | |||
| const queryKey = reloadPropIdx ? `dynamicProps:${reloadPropIdx}` : "dynamicProps" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the ternary expression to satisfy lint requirements
Currently, the linter complains about the multiline ternary usage on this line. Splitting the condition, the true branch, and false branch across separate lines will fix the issue:
-const queryKey = reloadPropIdx ? `dynamicProps:${reloadPropIdx}` : "dynamicProps"
+const queryKey = reloadPropIdx
+ ? `dynamicProps:${reloadPropIdx}`
+ : "dynamicProps";📝 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 queryKey = reloadPropIdx ? `dynamicProps:${reloadPropIdx}` : "dynamicProps" | |
| const queryKey = reloadPropIdx | |
| ? `dynamicProps:${reloadPropIdx}` | |
| : "dynamicProps"; |
🧰 Tools
🪛 eslint
[error] 132-132132: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 132-132132: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
🪛 GitHub Check: Lint Code Base
[failure] 132-132132:
Expected newline between test and consequent of ternary expression
[failure] 132-132132:
Expected newline between consequent and alternate of ternary expression
🪛 GitHub Actions: Pull Request Checks
[error] 132-132132: Expected newline between test and consequent of ternary expression (multiline-ternary)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
packages/connect-react/src/hooks/form-context.tsx (2)
Line range hint
141-155: Consider enhancing the query's enabled condition.The current implementation enables the query only when
reloadPropIdx != null. However, based on the PR objectives, we might want to consider additional conditions to handle edge cases:
- Initial load of dynamic props
- Forced reloads
- Recovery from errors
enabled: reloadPropIdx != null, // TODO or props.dynamicPropsId && !dynamicProps + enabled: Boolean( + // Enable on explicit reload request + reloadPropIdx != null || + // Enable on initial load when we have a dynamic props ID but no data + (formProps.dynamicPropsId && !dynamicProps) || + // Enable when configured props change and we need to reload + (dynamicProps?.id && Object.keys(configuredProps).length > 0) + ),This enhancement would better handle the Google Sheets scenario mentioned in the PR description, where dynamic properties need to be fetched multiple times.
🧰 Tools
🪛 eslint
[error] 142-143: Missing trailing comma.
(comma-dangle)
🪛 GitHub Check: Lint Code Base
[failure] 142-142:
Missing trailing comma🪛 GitHub Actions: Pull Request Checks
[error] 142-142: Missing trailing comma (comma-dangle)
Line range hint
136-155: Add error handling for dynamic props loading.The current implementation lacks error handling for the
useQueryhook. Consider adding error handling to:
- Handle network failures
- Provide feedback to users
- Implement retry logic for transient failures
const { isFetching: dynamicPropsQueryIsFetching, - // TODO error + error, + isError, } = useQuery({ queryKey: [ "dynamicProps", queryKeyInput, ], queryFn: async () => { const { dynamicProps } = await client.componentReloadProps(componentReloadPropsInput); - // XXX what about if null? - // TODO observation errors, etc. + if (!dynamicProps) { + throw new Error('Failed to load dynamic properties'); + } if (dynamicProps) { formProps.onUpdateDynamicProps?.(dynamicProps); setDynamicProps(dynamicProps); } setReloadPropIdx(undefined); return []; // XXX ok to mutate above and not look at data? }, + retry: (failureCount, error) => { + // Retry up to 3 times for network errors + return failureCount < 3 && error instanceof Error; + }, + onError: (error) => { + console.error('Failed to load dynamic properties:', error); + // Optionally trigger a UI notification + }, enabled: reloadPropIdx != null, });🧰 Tools
🪛 eslint
[error] 142-143: Missing trailing comma.
(comma-dangle)
🪛 GitHub Check: Lint Code Base
[failure] 142-142:
Missing trailing comma🪛 GitHub Actions: Pull Request Checks
[error] 142-142: Missing trailing comma (comma-dangle)
🧹 Nitpick comments (1)
packages/connect-react/src/hooks/form-context.tsx (1)
132-134: Consider adding type safety and optimizing the query key structure.While spreading
componentReloadPropsInputworks, consider:
- Creating a dedicated type for the query key
- Explicitly selecting only the properties that affect the query result
+type DynamicPropsQueryKey = { + userId: string; + componentId: string; + configuredProps: ConfiguredProps<any>; + dynamicPropsId?: string; +}; -const queryKeyInput = { - ...componentReloadPropsInput, -} +const queryKeyInput: DynamicPropsQueryKey = { + userId, + componentId, + configuredProps, + dynamicPropsId: dynamicProps?.id, +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/src/hooks/form-context.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/hooks/form-context.tsx
[failure] 142-142:
Missing trailing comma
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/src/hooks/form-context.tsx
[error] 142-142: Missing trailing comma (comma-dangle)
| "dynamicProps", | ||
| queryKeyInput |
There was a problem hiding this comment.
Fix the missing trailing comma.
Add a trailing comma to maintain consistency with the project's style guide.
queryKey: [
"dynamicProps",
- queryKeyInput
+ queryKeyInput,
],📝 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.
| "dynamicProps", | |
| queryKeyInput | |
| "dynamicProps", | |
| queryKeyInput, |
🧰 Tools
🪛 GitHub Check: Lint Code Base
[failure] 142-142:
Missing trailing comma
🪛 GitHub Actions: Pull Request Checks
[error] 142-142: Missing trailing comma (comma-dangle)
WHY
Dynamic props aren't loading (this only happens if we need to reload props more than once, eg. Google sheets, where we fetch dynamicProps for the first time after selecting the sheet. But we also need to fetch the dynamic props after selecting a worksheet, and so on.) The root cause appears to be
useQuery, which only executesqueryFnonce for a given queryKey. Ideally, useQuery should run once for a givenreloadPropIdx. So far the only thing that works is invalidating thequeryKey.Summary by CodeRabbit
Version Update
@pipedream/connect-reactpackage version from1.0.0-preview.15to1.0.0-preview.16Improvements