Biz/dj 2800 add async options pagination in connect react #15248
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces updates to 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 (4)
✅ Files skipped from review due to trivial changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
🔭 Outside diff range comments (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
Line range hint
122-133: Improve canLoadMore logic.The
setCanLoadMoreis always set to true when options exist, but it should consider if there are more pages available.- setCanLoadMore(true) + setCanLoadMore(!!res.context) return options; } if (stringOptions?.length) { const options = []; for (const stringOption of stringOptions) { options.push({ label: stringOption, value: stringOption, }); } - setCanLoadMore(true) + setCanLoadMore(!!res.context) return options; }
🧹 Nitpick comments (4)
packages/connect-react/src/components/LoadMoreButton.tsx (2)
4-7: Consider enhancing ButtonProps interface.The props interface could be improved to support loading states and accessibility.
export type ButtonProps = { enabled: boolean; onClick: () => void; + isLoading?: boolean; + 'aria-label'?: string; };
31-33: Add aria-label for better accessibility.The "Load More" button should have an aria-label for better screen reader support.
- <button disabled={!enabled} onClick={onClick} type="button" {...getProps("loadMoreButton", baseStyles, props)}> + <button + disabled={!enabled} + onClick={onClick} + type="button" + aria-label="Load more items" + {...getProps("loadMoreButton", baseStyles, props)} + > Load More </button>packages/connect-react/src/components/ControlSelect.tsx (1)
Line range hint
137-139: Improve error message for unhandled option type.The error message could be more descriptive to help with debugging.
- throw new Error("unhandled option type"); // TODO + throw new Error(`Unhandled option type. Expected array or object with 'value' property, received: ${typeof o}`);packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
42-48: Simplify state management by combining related states.The
page,context, andpageablestates could be combined into a single state object for better maintainability.- const [page, setPage] = useState<number>(0); - const [context, setContext] = useState<never | undefined>(undefined); - const [pageable, setPageable] = useState({ - page: 0, - prevContext: undefined, - }) + const [pagination, setPagination] = useState<{ + currentPage: number; + nextPage: number; + context: never | undefined; + }>({ + currentPage: 0, + nextPage: 0, + context: undefined, + });
📜 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 (6)
packages/connect-react/examples/nextjs/package.json(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(5 hunks)packages/connect-react/src/components/LoadMoreButton.tsx(1 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(6 hunks)packages/connect-react/src/hooks/customization-context.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/connect-react/package.json
🧰 Additional context used
🪛 eslint
packages/connect-react/src/components/ControlSelect.tsx
[error] 79-79: 'children' is missing in props validation
(react/prop-types)
[error] 94-94: 'components' is missing in props validation
(react/prop-types)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
packages/connect-react/src/hooks/customization-context.tsx (1)
36-43: LGTM! Clean integration of LoadMoreButton.The LoadMoreButton is correctly integrated into the customization context.
packages/connect-react/examples/nextjs/package.json (2)
10-10: Verify SDK version compatibility with new pagination featuresThe SDK version has been updated to ^1.1.4. Let's verify this version includes the required features for async options pagination.
✅ Verification successful
Version update is appropriate for example project
The update from ^1.0.6 to ^1.1.4 follows semantic versioning principles and represents a compatible change. As an example project, it's good practice to use the latest stable version of dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check SDK changelog and package.json for pagination-related changes # Check SDK package for pagination-related exports fd "package.json" --exec rg -A 5 "pagination|async.*options" {} # Look for pagination-related changes in recent commits git log -n 10 --oneline --grep="pagination" --grep="async.*options"Length of output: 65
Script:
#!/bin/bash # Search for SDK imports and pagination-related code rg "@pipedream/sdk" -A 5 # Search for pagination-related code in the example project rg -t js -t tsx "async.*options|pagination" packages/connect-react/examples/nextjs/Length of output: 46273
Script:
#!/bin/bash # Search for ControlSelect implementation and pagination-related code ast-grep --pattern 'export function ControlSelect() { $$$ }' # Search for async options or pagination in component props rg -t ts -t tsx "async.*options|pagination" packages/connect-react/src/Length of output: 166
12-13: Verify React RC version compatibilityThe project uses a specific React RC version (19.0.0-rc-66855b96-20241106). Please confirm if this is intentional or if we should use a stable version instead.
✅ Verification successful
RC version usage is intentionally isolated to NextJS example
The React RC version is only used in the NextJS example, while the rest of the codebase uses stable versions. This isolation suggests it's an intentional choice for testing or demonstrating compatibility with React 19, and doesn't affect the main package or other examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for React version consistency across the monorepo # Find all package.json files and check React versions fd "package.json" --exec rg "react.*19\.0\.0-rc-66855b96-20241106" {}Length of output: 162
Script:
#!/bin/bash # Show all React version declarations across package.json files echo "=== React versions across package.json files ===" fd "package.json" --exec rg '"react": ".*"' {} fd "package.json" --exec rg '"react-dom": ".*"' {} echo -e "\n=== Checking documentation for React version requirements ===" rg -i "react.*version|react.*requirement" -g "!package*.json" -g "*.md"Length of output: 659
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
78-90: 🛠️ Refactor suggestionMove LoadMore component outside and add proper type definitions.
The inline LoadMore component has several issues:
- Missing proper type definitions
- ESLint warnings being suppressed
- Defined within render which creates a new component on each render
+ type LoadMoreProps = { + children: React.ReactNode; + innerProps?: Record<string, unknown>; + } & React.ComponentProps<typeof components.MenuList>; + + const LoadMore = ({ + children, + innerProps, + ...props + }: LoadMoreProps) => { -const LoadMore = ({ - // eslint-disable-next-line react/prop-types - children, ...props -}) => {Also, consider moving this component to a separate file for better maintainability.
🧹 Nitpick comments (4)
packages/connect-react/src/components/LoadMoreButton.tsx (1)
4-6: Enhance ButtonProps interface for better flexibility and accessibility.The current props interface is too minimal. Consider adding common button props and accessibility attributes.
export type ButtonProps = { onChange: () => void; + isLoading?: boolean; + disabled?: boolean; + className?: string; + 'aria-label'?: string; };packages/connect-react/src/components/ControlSelect.tsx (1)
92-99: Avoid mutating props object directly.The components object is being modified directly. Create a new object instead.
- const props = select.getProps("controlSelect", baseSelectProps) - if (showLoadMoreButton) { - props.components = { - // eslint-disable-next-line react/prop-types - ...props.components, - MenuList: LoadMore, - } - } + const props = { + ...select.getProps("controlSelect", baseSelectProps), + ...(showLoadMoreButton && { + components: { + ...select.getProps("controlSelect", baseSelectProps).components, + MenuList: LoadMore, + } + }) + };packages/connect-react/src/components/RemoteOptionsContainer.tsx (2)
135-144: Handle edge cases in data accumulation.The data accumulation logic needs to handle edge cases and prevent duplicates.
const data = [ ...pageable.data, - ..._options, + ..._options.filter(newOption => + !pageable.data.some(existingOption => + existingOption.value === newOption.value + ) + ), ] setPageable({ page: page + 1, prevContext: res.context, data, })
149-151: Improve loading state handling in showLoadMoreButton.The
showLoadMoreButtonfunction should consider theisLoadingMorestate.const showLoadMoreButton = () => { - return !isFetching && !error && canLoadMore() + return !isFetching && !isLoadingMore && !error && canLoadMore() }
📜 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/components/ControlSelect.tsx(5 hunks)packages/connect-react/src/components/LoadMoreButton.tsx(1 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
packages/connect-react/src/components/RemoteOptionsContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
85-92:⚠️ Potential issueAdd error handling and prevent potential race conditions.
The
onLoadMorefunction needs error handling and should prevent multiple concurrent calls.+ const [isLoadingMore, setIsLoadingMore] = useState(false); + - const onLoadMore = () => { + const onLoadMore = async () => { + if (isLoadingMore) return; + try { + setIsLoadingMore(true); + setPageable(prev => ({ + ...prev, + prevContext: {}, + })); + await refetch(); + } catch (error) { + setError({ + name: "Error", + message: "Failed to load more items", + }); + } finally { + setIsLoadingMore(false); + } }
🧹 Nitpick comments (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
170-172: Optimize showLoadMoreButton with useMemo.Consider memoizing this function to prevent unnecessary re-renders.
- const showLoadMoreButton = () => { + const showLoadMoreButton = useMemo(() => { return !isFetching && !error && canLoadMore - } + }, [isFetching, error, canLoadMore]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (3)
32-56: 🛠️ Refactor suggestionConsolidate state management and improve type safety.
The current implementation has several state management issues:
pageandcontextstates duplicate data already present inpageablecontexttype (never | undefined) is problematic- Multiple states could be consolidated for better maintainability
Consider consolidating the states and improving type safety:
+ type Option = { + label: string; + value: string | number; + }; + + interface PageableState { + page: number; + prevContext: Record<string, unknown>; + data: Option[]; + values: Set<string | number>; + canLoadMore: boolean; + } + - const [page, setPage] = useState<number>(0); - const [canLoadMore, setCanLoadMore] = useState<boolean>(true); - const [context, setContext] = useState<never | undefined>(undefined); - const [pageable, setPageable] = useState({ - page: 0, - prevContext: {}, - data: [], - values: new Set(), - }); + const [pageable, setPageable] = useState<PageableState>({ + page: 0, + prevContext: {}, + data: [], + values: new Set(), + canLoadMore: true, + });Likely invalid or redundant comment.
Line range hint
123-165: Improve type safety and simplify options handling logic.The options handling logic needs improvement in several areas:
- Missing TypeScript types for API response
- Complex nested conditions
- Growing Set of values without cleanup
Consider this refactor:
+ interface ComponentConfigureResponse { + options?: Option[]; + stringOptions?: string[]; + context?: Record<string, unknown>; + errors?: string[]; + } + - let _options = [] - if (options?.length) { - _options = options; - } - if (stringOptions?.length) { - const options = []; - for (const stringOption of stringOptions) { - options.push({ - label: stringOption, - value: stringOption, - }); - } - _options = options; - } + const _options = [ + ...(options ?? []), + ...(stringOptions?.map(str => ({ label: str, value: str })) ?? []), + ]; + // Reset values set when query changes + useEffect(() => { + setPageable(prev => ({ + ...prev, + values: new Set(), + data: [], + page: 0, + canLoadMore: true, + })); + }, [query]);Also, verify the memory usage of the growing Set:
✅ Verification successful
The review comment's concerns and suggestions are valid
The codebase analysis confirms:
- The
Setof values grows without cleanup as new options are loaded- There's no query-dependent reset mechanism in place
- The suggested cleanup in a
useEffecthook would prevent memory growthThe original review's refactoring suggestion should be implemented to ensure proper cleanup of the accumulated values when the query changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential memory leaks in other components using similar patterns rg -l 'new Set\(' | xargs rg 'useEffect.*\[.*query.*\]' -C 5Length of output: 62
Script:
#!/bin/bash # First find files using Set constructor echo "Files using Set:" rg "new Set\(" --type typescript echo -e "\nEffect hooks in these files:" rg "useEffect\(" --type typescript -A 5 -B 2 echo -e "\nQuery-dependent state updates:" rg "setPageable|setState.*query" --type typescript -A 3 -B 3Length of output: 442
Script:
#!/bin/bash # Find TypeScript files and search patterns echo "Files using Set:" fd -e ts -e tsx | xargs rg "new Set\(" echo -e "\nEffect hooks with query dependencies:" fd -e ts -e tsx | xargs rg "useEffect.*\[.*query.*\]" -C 5 echo -e "\nPageable state updates:" fd -e ts -e tsx | xargs rg "setPageable\(" -C 3Length of output: 1838
188-190: Verify ControlSelect component prop types.Ensure that the new props are properly typed in the ControlSelect component.
✅ Verification successful
Props are properly typed in ControlSelect component
The
showLoadMoreButton,onLoadMore, andoptionsprops used in RemoteOptionsContainer match the prop types defined in ControlSelect component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check ControlSelect component prop types ast-grep --pattern 'interface $name { $$$ showLoadMoreButton?: boolean; onLoadMore?: () => void; $$$ }'Length of output: 113
Script:
#!/bin/bash # Find ControlSelect component definition ast-grep --pattern 'interface ControlSelectProps { $$$ }' # Also search for the component itself ast-grep --pattern 'const ControlSelect = $$$' # Backup search using ripgrep rg "ControlSelect" -A 10Length of output: 11006
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/connect-react/src/components/ControlSelect.tsx (4)
77-89: Move LoadMore component outside ControlSelect.The LoadMore component should be defined outside the main component for better maintainability and reusability.
+ type LoadMoreProps = { + children: React.ReactNode; + innerProps?: Record<string, unknown>; + } & React.ComponentProps<typeof components.MenuList>; + + const LoadMore = ({ + children, + innerProps, + ...props + }: LoadMoreProps) => { + return ( + <components.MenuList {...props}> + {children} + <div className="pt-4"> + <LoadMoreButton onChange={onLoadMore}/> + </div> + </components.MenuList> + ); + }; export function ControlSelect<T>({ isCreatable, options, selectProps, showLoadMoreButton, onLoadMore, }: ControlSelectProps<T>) { - const LoadMore = ({ - children, ...props - }) => { - return ( - <components.MenuList {...props}> - { children } - <div className="pt-4"> - <LoadMoreButton onChange={onLoadMore}/> - </div> - </components.MenuList> - ) - }
94-95: Remove unnecessary eslint-disable comment.After adding proper TypeScript types, this eslint-disable comment won't be needed.
- // eslint-disable-next-line react/prop-types
131-133: Use a more descriptive property name instead of__lv.The property name
__lvis cryptic. Consider using a more descriptive name that explains its purpose, such aslabelValueorselectedOption.- onChange({ - __lv: o, - }); + onChange({ + labelValue: o, // or selectedOption: o + });
Line range hint
138-138: Improve error handling for unhandled option types.The current error message "unhandled option type" with a TODO comment could be more descriptive. Consider including the actual type received in the error message and handling all possible cases.
- throw new Error("unhandled option type"); // TODO + throw new Error(`Unhandled option type: ${typeof o}. Expected an object with 'value' property or an array.`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/connect-react/src/components/ControlSelect.tsx(5 hunks)
🔇 Additional comments (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
17-18: LGTM! Well-structured type definitions for pagination.The new optional props
showLoadMoreButtonandonLoadMoreare properly typed and maintain backward compatibility.
bzwrk
deleted the
biz/dj-2800-add-async-options-pagination-in-connect-react
branch
WHY
Summary by CodeRabbit
New Features
LoadMoreButtoncomponent for improved user interaction.Dependencies
@pipedream/sdkto version^1.1.4.@pipedream/connect-reactto version1.0.0-preview.19.Improvements
LoadMoreButtonin the component library.