Adding pagination to dropdowns, misc bug fixes #18726
Conversation
And fixing load more button for remoteOptions dropdown
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds paginated infinite-scroll to SelectApp/SelectComponent and remote-options loading; refactors hooks to expose loadMore/hasMore/isLoadingMore, introduces pagination utilities, stabilizes Select components with memoization/refs, and bumps package to 2.1.0 with changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Select as Select (App/Component)
participant Hook as useApps/useComponents
participant API as Backend API
User->>Select: Open menu / scroll to bottom
Select->>Hook: onMenuScrollToBottom()
alt hasMore && !isLoadingMore
Note right of Select #E8F6FF: Show "Loading more..." indicator
Hook->>API: GET list?page=next&limit=50
API-->>Hook: { items, nextPage, hasMore }
Hook-->>Select: append items (state update)
else No more pages
Select-->>User: No additional loading
end
sequenceDiagram
autonumber
participant Parent as Parent Props
participant ROC as RemoteOptionsContainer
participant Query as Data Query
participant Store as Accumulated State
Parent-->>ROC: props (account, inputs)
ROC->>ROC: derive queryKeyInput & isQueryEnabled
alt isQueryEnabled
ROC->>Query: fetch page with context + limit
Query-->>ROC: { options, nextContext, error? }
ROC->>Store: merge/accumulate options & values (dedupe)
ROC->>ROC: update context and canLoadMore
else disabled / account changed
ROC->>Store: clear/reset accumulated state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…pedreamHQ/pipedream into danny/connect-react/adding-pagination
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connect-react/CHANGELOG.md (1)
20-22: Package name mismatch for SDK in docs vs dependency.Changelog says “@pipedreamhq/pipedream-sdk” while package.json depends on “@pipedream/sdk”. Align the reference to avoid confusion.
-Use the new v2 version of the Pipedream SDK (i.e. `@pipedreamhq/pipedream-sdk`). +Use the new v2 version of the Pipedream SDK (i.e. `@pipedream/sdk`).
🧹 Nitpick comments (9)
packages/connect-react/package.json (2)
33-38: Consider updating react-select to latest 5.10.x for scrolling fixes.Infinite scroll benefits from react-select 5.9–5.10 fixes (scroll/keyboard, A11y). Bumping from ^5.8.2→^5.10.x can improve UX and stability.
46-49: Optional: add React 19 to peer range.If you intend to support React 19 soon, extend peers to include ^19.0.0 for react and react-dom.
- "peerDependencies": { - "react": "^16.8.0 || ^17.0.0 || ^18.0.0", - "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0" - } + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", + "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + }packages/connect-react/src/hooks/use-components.tsx (1)
38-41: Optional: strengthen typing for nextPage to avoid any-cast.If SDK exposes a paginated type with
getNextPage()andhasNextPage(), use it instead ofunknown/any.- const [ nextPage, setNextPage ] = useState<unknown>(null); + // Example: replace PaginatedResponse<Component> with the correct SDK type + // import type { PaginatedResponse } from "@pipedream/sdk"; + const [ nextPage, setNextPage ] = useState<PaginatedResponse<Component> | null>(null); @@ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const nextPageData = await (nextPage as any).getNextPage(); + const nextPageData = await nextPage.getNextPage();Please confirm the exact paginated SDK type name to wire here.
Also applies to: 73-95
packages/connect-react/src/components/SelectApp.tsx (1)
156-175: Optional: improve loading announcement accessibility.Add aria-live to the loading indicator for SRs.
- <div style={{ + <div aria-live="polite" style={{ padding: "8px 12px", textAlign: "center", color: "#666", fontSize: "14px", }}>packages/connect-react/src/hooks/use-apps.tsx (3)
37-40: Type the paginated response to dropunknown/anyand catch mistakes at compile-timeDefine a minimal page type and use it for state and loadMore to remove casts and improve safety.
+// Minimal page shape used from SDK pagination +type PagedPage<T> = { + data?: T[]; + hasNextPage: () => boolean; + getNextPage: () => Promise<PagedPage<T>>; +}; ... - ] = useState<unknown>(null); + ] = useState<PagedPage<App> | null>(null); ... - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const nextPageData = await (nextPage as any).getNextPage(); + const nextPageData = await nextPage.getNextPage();Optionally, type useQuery as
useQuery<PagedPage<App>>and adjustUseAppsResultto extendUseQueryResult<PagedPage<App>, Error>minus data.Would you like me to apply the same typing and initial-load fix to use-components.tsx for parity?
Also applies to: 72-90
72-90: Guard against concurrent loadMore calls (race on rapid scroll)onMenuScrollToBottom can fire multiple times quickly; setState is async, so two calls may pass the
isLoadingMorecheck before it flips.Use a ref-based in-flight guard:
+ const inFlightRef = useRef(false); const loadMore = useCallback(async () => { - if (!nextPage || !hasMore || isLoadingMore) return; + if (!nextPage || !hasMore || isLoadingMore || inFlightRef.current) return; setIsLoadingMore(true); + inFlightRef.current = true; try { const nextPageData = await nextPage.getNextPage(); ... } catch (err) { console.error("Error loading more apps:", err); } finally { setIsLoadingMore(false); + inFlightRef.current = false; } }, [ nextPage, hasMore, isLoadingMore, ]);
12-17: Expose accurate query types instead ofunknown
UseAppsResult = Omit<UseQueryResult<unknown,...>>loses type info and can break consumers spreading query props.Prefer:
-export type UseAppsResult = Omit<UseQueryResult<unknown, Error>, "data"> & { +export type UseAppsResult = Omit<UseQueryResult<PagedPage<App>, Error>, "data"> & { apps: App[]; isLoadingMore: boolean; hasMore: boolean; loadMore: () => Promise<void>; };packages/connect-react/src/components/RemoteOptionsContainer.tsx (2)
238-329: Avoid side effects inside state updaters; compute “found new options” outside
foundNewOptionsis mutated inside thesetAccumulatedValuesupdater. Updaters should be pure; in React Strict Mode, they can run twice, making this unreliable.Refactor to:
- Build a Set from current accumulatedData (or keep a ref).
- Compute additions in local variables.
- Then call setAccumulatedValues(newSet) and setAccumulatedData([...prev, ...additions]).
- Derive
canLoadMorefromadditions.length.I can provide a focused diff if you prefer a ref-based approach (accumulatedValuesRef) to also avoid extra re-renders.
83-89: Use a ref for accumulated values instead of state to avoid extra rendersYou don’t read
_accumulatedValuesanywhere; updating it still triggers a re-render. Replace withuseRef<Set<PropOptionValue>>(new Set())and update the ref in the effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(3 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(5 hunks)packages/connect-react/src/components/SelectApp.tsx(3 hunks)packages/connect-react/src/components/SelectComponent.tsx(2 hunks)packages/connect-react/src/hooks/use-apps.tsx(1 hunks)packages/connect-react/src/hooks/use-components.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/SelectApp.tsxpackages/connect-react/src/components/ControlSelect.tsxpackages/connect-react/src/components/SelectComponent.tsx
🧬 Code graph analysis (6)
packages/connect-react/src/components/SelectApp.tsx (1)
packages/connect-react/src/hooks/use-apps.tsx (1)
useApps(22-103)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (4)
packages/connect-react/src/types.ts (2)
RawPropOption(50-56)ConfigureComponentContext(58-58)packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/utils/type-guards.ts (2)
isString(35-37)sanitizeOption(135-175)packages/connect-react/src/components/ControlSelect.tsx (1)
ControlSelect(36-248)
packages/connect-react/src/hooks/use-components.tsx (1)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
packages/connect-react/src/components/SelectComponent.tsx (1)
packages/connect-react/src/hooks/use-components.tsx (1)
useComponents(23-104)
packages/connect-react/src/hooks/use-apps.tsx (2)
types/src/index.ts (1)
App(170-178)packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
packages/connect-react/package.json (2)
3-3: Version bump looks good and aligns with CHANGELOG 2.1.0.
9-17: Verify dist artifact paths match build output
After running the Vite build (with vite-plugin-dts), confirm that packages/connect-react/dist contains connect-react.es.js, connect-react.umd.js, connect-react.umd.d.ts and that package.json’s browser, module, types, and exports fields point to these files.packages/connect-react/src/components/ControlSelect.tsx (1)
151-153: No changes needed: LoadMoreButton uses onChangepackages/connect-react/src/components/SelectComponent.tsx (1)
47-56: Load-more trigger logic looks good
handleMenuScrollToBottomcorrectly gates onhasMoreandisLoadingMore, and wiring viaonMenuScrollToBottomis idiomatic for react-select.Please confirm SelectApp.tsx mirrors the same dependency fix for its MenuList customization to avoid a similar stale closure.
Also applies to: 95-98
packages/connect-react/src/components/RemoteOptionsContainer.tsx (2)
373-379: Load more button signal looks good
showLoadMoreButtoncorrectly derives fromisFetching,error, andcanLoadMore.
405-417: Gate manual refetch behindisQueryEnabled
- onInputChange: wrap
refetch()inif (isQueryEnabled)to prevent fetching when disabled.- onMenuOpen: remove the existing
refetch()underdisableQueryDisabling && !isQueryEnabledso no manual fetch occurs while the query is disabled.
If “demo” mode (disableQueryDisabling) should still trigger a fetch, reset your pagination state before callingrefetch().
packages/connect-react/src/components/RemoteOptionsContainer.tsx
Outdated
Show resolved
Hide resolved
| // Memoize custom components to prevent remounting | ||
| // Note: Don't include isLoadingMore in deps - it's read from closure | ||
| // and MenuList will re-render naturally when parent re-renders | ||
| const customComponents = useMemo(() => ({ | ||
| MenuList: (props: MenuListProps<Component>) => ( | ||
| <MenuList {...props}> | ||
| {props.children} | ||
| {isLoadingMore && ( | ||
| <div style={{ | ||
| padding: "8px 12px", | ||
| textAlign: "center", | ||
| color: "#666", | ||
| fontSize: "14px", | ||
| }}> | ||
| Loading more {componentType === "action" | ||
| ? "actions" | ||
| : "triggers"}... | ||
| </div> | ||
| )} | ||
| </MenuList> | ||
| ), | ||
| IndicatorSeparator: () => null, | ||
| }), [ | ||
| componentType, | ||
| MenuList, | ||
| ]); |
There was a problem hiding this comment.
Stale isLoadingMore in MenuList due to memo deps; loading indicator may not update
customComponents is memoized without isLoadingMore, so the closure captures a stale value and the text won’t toggle.
Include isLoadingMore (and optionally hasMore if used later) in deps:
- const customComponents = useMemo(() => ({
+ const customComponents = useMemo(() => ({
MenuList: (props: MenuListProps<Component>) => (
<MenuList {...props}>
{props.children}
{isLoadingMore && (
<div style={{ ... }}>
Loading more {componentType === "action" ? "actions" : "triggers"}...
</div>
)}
</MenuList>
),
IndicatorSeparator: () => null,
- }), [
- componentType,
- MenuList,
- ]);
+ }), [componentType, MenuList, isLoadingMore]);📝 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.
| // Memoize custom components to prevent remounting | |
| // Note: Don't include isLoadingMore in deps - it's read from closure | |
| // and MenuList will re-render naturally when parent re-renders | |
| const customComponents = useMemo(() => ({ | |
| MenuList: (props: MenuListProps<Component>) => ( | |
| <MenuList {...props}> | |
| {props.children} | |
| {isLoadingMore && ( | |
| <div style={{ | |
| padding: "8px 12px", | |
| textAlign: "center", | |
| color: "#666", | |
| fontSize: "14px", | |
| }}> | |
| Loading more {componentType === "action" | |
| ? "actions" | |
| : "triggers"}... | |
| </div> | |
| )} | |
| </MenuList> | |
| ), | |
| IndicatorSeparator: () => null, | |
| }), [ | |
| componentType, | |
| MenuList, | |
| ]); | |
| // Memoize custom components to prevent remounting | |
| // Note: Don't include isLoadingMore in deps - it's read from closure | |
| // and MenuList will re-render naturally when parent re-renders | |
| const customComponents = useMemo(() => ({ | |
| MenuList: (props: MenuListProps<Component>) => ( | |
| <MenuList {...props}> | |
| {props.children} | |
| {isLoadingMore && ( | |
| <div style={{ | |
| padding: "8px 12px", | |
| textAlign: "center", | |
| color: "#666", | |
| fontSize: "14px", | |
| }}> | |
| Loading more {componentType === "action" | |
| ? "actions" | |
| : "triggers"}... | |
| </div> | |
| )} | |
| </MenuList> | |
| ), | |
| IndicatorSeparator: () => null, | |
| }), [componentType, MenuList, isLoadingMore]); |
🤖 Prompt for AI Agents
In packages/connect-react/src/components/SelectComponent.tsx around lines 58 to
83, the useMemo closure for customComponents captures isLoadingMore from an
outer scope but doesn't list it in the dependency array, causing the loading
indicator to become stale; update the memo dependencies to include isLoadingMore
(and hasMore if it's used to control the indicator) alongside componentType and
MenuList so the MenuList component remounts/re-renders when loading state
changes.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
packages/connect-react/src/hooks/use-apps.tsx (1)
96-102: Hide raw querydatafrom public surface to avoid confusionType omits
databut the returned object still spreads it. Removedataat source.- return { - ...query, - apps: allApps, - isLoadingMore, - hasMore, - loadMore, - }; + const { data: _data, ...restQuery } = query; + return { + ...restQuery, + apps: allApps, + isLoadingMore, + hasMore, + loadMore, + };Also consider typing the query (and
nextPage) with the SDK’s paginated type to drop theanycast ingetNextPage().packages/connect-react/src/components/RemoteOptionsContainer.tsx (2)
83-89: Remove unusedaccumulatedValuesstateYou only use the setter as a side-channel (now removed above). Drop this state to simplify.
- // State variable unused - we only use the setter and derive values from prevValues - const [ - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _accumulatedValues, - setAccumulatedValues, - ] = useState<Set<PropOptionValue>>(new Set()); + // Removed unused accumulatedValues state
407-412: Avoid immediate refetch on input change; rely on query key (or debounce)Calling
refetch()right aftersetQuery(v)can issue a request with the stale query, then another when the key updates. Prefer letting React Query refetch on key change, optionally with debounce.onInputChange(v) { if (prop.useQuery) { setQuery(v); - refetch(); } },Optionally debounce
setQuery(e.g., 300ms) to reduce load when typing.packages/connect-react/src/components/SelectApp.tsx (1)
98-106: Minor perf: lazy-load logosAdd loading and decoding hints to reduce layout/CPU cost when many options render.
- <img + <img + loading="lazy" + decoding="async" src={`https://pipedream.com/s.v0/${optionProps.data.id}/logo/48`}Do the same in SingleValue.
Also applies to: 119-126
packages/connect-react/src/hooks/use-components.tsx (3)
42-44: Prevent duplicate page fetches under rapid scroll eventsState-based guards aren’t synchronous; quick successive calls can race. Add a ref guard and reset it on input changes.
// Track previous input to detect when query params actually change const prevInputRef = useRef<string>(); + const isLoadingMoreRef = useRef(false); // Reset pagination ONLY when query params change, not on refetches useEffect(() => { const inputKey = JSON.stringify(input ?? {}); const isNewQuery = prevInputRef.current !== inputKey; if (query.data && isNewQuery) { prevInputRef.current = inputKey; setAllComponents(query.data.data || []); setHasMore(query.data.hasNextPage()); setNextPage(query.data); setIsLoadingMore(false); + isLoadingMoreRef.current = false; } }, [ query.data, input, ]); const loadMore = useCallback(async () => { - if (!nextPage || !hasMore || isLoadingMore) return; + if (!nextPage || !hasMore || isLoadingMoreRef.current) return; - setIsLoadingMore(true); + isLoadingMoreRef.current = true; + setIsLoadingMore(true); try { // eslint-disable-next-line @typescript-eslint/no-explicit-any const nextPageData = await (nextPage as any).getNextPage(); setAllComponents((prev) => [ ...prev, ...(nextPageData.data || []), ]); setHasMore(nextPageData.hasNextPage()); setNextPage(nextPageData); } catch (err) { console.error("Error loading more components:", err); } finally { - setIsLoadingMore(false); + setIsLoadingMore(false); + isLoadingMoreRef.current = false; } }, [ nextPage, hasMore, - isLoadingMore, ]);Also applies to: 61-67, 73-90
97-104: Align return type with runtime by omitting data from spreadYou’re returning ...query which includes data at runtime, even though the exported type omits it. Drop data explicitly to avoid confusion.
- return { - ...query, - components: allComponents, - isLoadingMore, - hasMore, - loadMore, - }; + const { data: _ignored, ...rest } = query; + return { + ...rest, + components: allComponents, + isLoadingMore, + hasMore, + loadMore, + };
45-54: Optional: gate the query when required inputs are missingIf ComponentsListRequest requires fields (e.g., appId or account), add enabled to avoid 4xxs and wasted fetches.
Do you expect input to sometimes be incomplete? If yes, consider:
useQuery({ queryKey: ["components", input], enabled: Boolean(input && /* required props */), queryFn: () => client.components.list({ limit: 50, ...input }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(3 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(5 hunks)packages/connect-react/src/components/SelectApp.tsx(3 hunks)packages/connect-react/src/components/SelectComponent.tsx(2 hunks)packages/connect-react/src/hooks/use-apps.tsx(1 hunks)packages/connect-react/src/hooks/use-components.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/SelectApp.tsxpackages/connect-react/src/components/ControlSelect.tsx
🧬 Code graph analysis (6)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (4)
packages/connect-react/src/types.ts (2)
RawPropOption(50-56)ConfigureComponentContext(58-58)packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/utils/type-guards.ts (2)
isString(35-37)sanitizeOption(135-175)packages/connect-react/src/components/ControlSelect.tsx (1)
ControlSelect(36-248)
packages/connect-react/src/hooks/use-apps.tsx (2)
types/src/index.ts (1)
App(170-178)packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
packages/connect-react/src/components/SelectComponent.tsx (1)
packages/connect-react/src/hooks/use-components.tsx (1)
useComponents(23-104)
packages/connect-react/src/components/SelectApp.tsx (1)
packages/connect-react/src/hooks/use-apps.tsx (1)
useApps(22-103)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
packages/connect-react/src/hooks/use-components.tsx (1)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
packages/connect-react/CHANGELOG.md (1)
5-15: Changelog entry reads wellAccurately describes pagination/infinite scroll and the async options reset fix. LGTM.
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
141-151: Query key excludesdynamicPropsId— confirm cache semanticsOmitting
dynamicPropsIdfrom the key may reuse cached results across different dynamic prop definitions, risking stale options. IfdynamicPropsIdmaterially changes the output, include it in the key.Would you like a quick repo scan to see how often
dynamicPropsIdchanges across a session and whether it correlates with different option sets?
packages/connect-react/src/components/RemoteOptionsContainer.tsx
Outdated
Show resolved
Hide resolved
| // Memoize custom components to prevent remounting | ||
| // Note: Don't include isLoadingMore in deps - it's read from closure | ||
| // and components will re-render naturally when parent re-renders | ||
| const customComponents = useMemo(() => ({ | ||
| Option: (optionProps: OptionProps<App>) => ( | ||
| <Option {...optionProps}> | ||
| <div style={{ | ||
| display: "flex", | ||
| gap: 10, | ||
| }}> | ||
| <img | ||
| src={`https://pipedream.com/s.v0/${optionProps.data.id}/logo/48`} | ||
| style={{ | ||
| height: 24, | ||
| width: 24, | ||
| }} | ||
| alt={optionProps.data.name} | ||
| /> | ||
| <span style={{ | ||
| whiteSpace: "nowrap", | ||
| }}>{optionProps.data.name}</span> | ||
| </div> | ||
| </Option> | ||
| ), | ||
| SingleValue: (singleValueProps: SingleValueProps<App>) => ( | ||
| <SingleValue {...singleValueProps}> | ||
| <div style={{ | ||
| display: "flex", | ||
| gap: 10, | ||
| alignItems: "center", | ||
| }}> | ||
| <img | ||
| src={`https://pipedream.com/s.v0/${singleValueProps.data.id}/logo/48`} | ||
| style={{ | ||
| height: 24, | ||
| width: 24, | ||
| }} | ||
| alt={singleValueProps.data.name} | ||
| /> | ||
| <span style={{ | ||
| whiteSpace: "nowrap", | ||
| }}> | ||
| {singleValueProps.data.name} | ||
| </span> | ||
| </div> | ||
| </SingleValue> | ||
| ), | ||
| MenuList: (props: MenuListProps<App>) => ( | ||
| <MenuList {...props}> | ||
| {props.children} | ||
| {isLoadingMore && ( | ||
| <div style={{ | ||
| padding: "8px 12px", | ||
| textAlign: "center", | ||
| color: "#666", | ||
| fontSize: "14px", | ||
| }}> | ||
| Loading more apps... | ||
| </div> | ||
| )} | ||
| </MenuList> | ||
| ), | ||
| IndicatorSeparator: () => null, | ||
| }), [ | ||
| Option, | ||
| SingleValue, | ||
| MenuList, | ||
| ]); |
There was a problem hiding this comment.
“Loading more apps…” won’t update due to stale closure in memoized components
customComponents is memoized without isLoadingMore; MenuList reads isLoadingMore from closure, so the indicator can get stuck/off. Pass state via selectProps to keep components stable and reactive.
Apply within this block:
MenuList: (props: MenuListProps<App>) => (
<MenuList {...props}>
{props.children}
- {isLoadingMore && (
+ {(props.selectProps as any).isLoadingMore && (
<div style={{
padding: "8px 12px",
textAlign: "center",
color: "#666",
fontSize: "14px",
}}>
Loading more apps...
</div>
)}
</MenuList>
),And pass the flag through Select props so it appears in selectProps:
<Select
/* ...existing props... */
components={customComponents}
options={apps || []}
isLoadingMore={isLoadingMore} // pass-through for custom MenuList to read
onMenuScrollToBottom={handleMenuScrollToBottom}
/>Alternative (simpler but recreates components on each toggle): include isLoadingMore in the deps array.
-}), [ Option, SingleValue, MenuList ]);
+}), [ Option, SingleValue, MenuList, isLoadingMore ]);Based on learnings
Also applies to: 161-165
🤖 Prompt for AI Agents
In packages/connect-react/src/components/SelectApp.tsx around lines 88-155 (and
apply same change near 161-165), the memoized customComponents captures
isLoadingMore in a stale closure so the MenuList "Loading more apps..."
indicator doesn't update; change MenuList to read the flag from
props.selectProps (e.g., const loading = props.selectProps?.isLoadingMore)
instead of the outer isLoadingMore variable, and then when rendering the Select
component (lines ~161-165) pass isLoadingMore as a prop on Select
(isLoadingMore={isLoadingMore}) so it becomes available on selectProps; do not
add isLoadingMore to the useMemo deps to keep components stable.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
83-110: selectValue returns null for LV objects; return early to avoid flickerWhen rawValue is already a LabelValueOption, the function falls through to null. Return it directly to keep selection stable (e.g., after handleCreate before upstream state sync).
const selectValue: LabelValueOption<T> | LabelValueOption<T>[] | null = useMemo(() => { if (rawValue == null) { return null; } - if (Array.isArray(rawValue)) { + // If already a LabelValue option, return it + if (isOptionWithLabel(rawValue as unknown)) { + return sanitizeOption(rawValue as T); + } + + if (Array.isArray(rawValue)) { // if simple, make lv (XXX combine this with other place this happens) if (!isOptionWithLabel(rawValue[0])) { return rawValue.map((o) => selectOptions.find((item) => item.value === o) || sanitizeOption(o as T)); } } else if (rawValue && typeof rawValue === "object" && "__lv" in (rawValue as Record<string, unknown>)) {
♻️ Duplicate comments (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
225-316: Race on foundNewOptions causes canLoadMore to flip false prematurelyMutating foundNewOptions inside one state updater and reading it outside is racy; canLoadMore can be set false even when new items exist. Make the update atomic.
- // Sync query data into accumulated state - useEffect(() => { - if (!queryData) return; - - // Handle errors - if (queryData.error) { - setError(queryData.error); - return; - } - - setError(undefined); - - // Store the context for the next page - setNextContext(queryData.context); - - // Determine if this is a fresh query or pagination - const isFirstPage = page === 0; - - // Track if we found new options (for canLoadMore logic) - let foundNewOptions = false; - - // Update values set - setAccumulatedValues((prevValues) => { - const baseValues = isFirstPage - ? new Set<PropOptionValue>() - : prevValues; - const newValues = new Set(baseValues); - - for (const o of queryData.options) { - const value = extractOptionValue(o); - if (value === null) { - console.warn("Skipping invalid option:", o); - continue; - } - - if (!newValues.has(value)) { - newValues.add(value); - foundNewOptions = true; - } - } - - return newValues; - }); - - // Update accumulated data independently - setAccumulatedData((prevData) => { - const baseData = isFirstPage - ? [] - : prevData; - const newOptions: RawPropOption[] = []; - const tempValues = new Set<PropOptionValue>(); - - // Build temp values set from existing data for deduplication - if (!isFirstPage) { - for (const o of baseData) { - const value = extractOptionValue(o); - if (value !== null) { - tempValues.add(value); - } - } - } - - for (const o of queryData.options) { - const value = extractOptionValue(o); - if (value === null) continue; - - if (!tempValues.has(value)) { - tempValues.add(value); - newOptions.push(o); - } - } - - if (!newOptions.length) { - return prevData; - } - - return [ - ...baseData, - ...newOptions, - ] as RawPropOption[]; - }); - - // Update canLoadMore flag after processing - if (!foundNewOptions) { - setCanLoadMore(false); - } - }, [ - queryData, - page, - dataUpdatedAt, - queryKeyString, - ]); + // Sync query data into accumulated state (atomic) + useEffect(() => { + if (!queryData) return; + + if (queryData.error) { + setError(queryData.error); + return; + } + setError(undefined); + setNextContext(queryData.context); + + const isFirstPage = page === 0; + + setAccumulatedData((prevData) => { + const baseData = isFirstPage ? [] : prevData; + const seen = new Set<PropOptionValue>(); + for (const o of baseData) { + const v = extractOptionValue(o); + if (v !== null) seen.add(v); + } + const newOptions: RawPropOption[] = []; + for (const o of queryData.options) { + const v = extractOptionValue(o); + if (v === null) { + console.warn("Skipping invalid option:", o); + continue; + } + if (!seen.has(v)) { + seen.add(v); + newOptions.push(o); + } + } + setCanLoadMore(newOptions.length > 0); + if (!newOptions.length) return baseData; + return [...baseData, ...newOptions] as RawPropOption[]; + }); + }, [queryData, page]);Optionally remove the now-unused accumulatedValues state.
🧹 Nitpick comments (2)
packages/connect-react/src/components/ControlSelect.tsx (1)
138-143: Also merge selectProps.components and add it to depsCallers may pass components via selectProps. Merge them to preserve overrides and include in deps.
- const mergedComponents = { - ...(props.components ?? {}), - ...(componentsOverride ?? {}), - }; + const mergedComponents = { + ...(props.components ?? {}), + ...(selectProps?.components ?? {}), + ...(componentsOverride ?? {}), + }; @@ - }, [ - props.components, - componentsOverride, - ]); + }, [ + props.components, + selectProps?.components, + componentsOverride, + ]);Also applies to: 165-167
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
394-399: Consider debouncing refetch on input typingRefetching on each keystroke can thrash the API. Debounce input or refetch inside a debounced effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/connect-react/src/components/ControlSelect.tsx(3 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(4 hunks)packages/connect-react/src/components/SelectApp.tsx(3 hunks)packages/connect-react/src/components/SelectComponent.tsx(2 hunks)packages/connect-react/src/hooks/use-apps.tsx(1 hunks)packages/connect-react/src/hooks/use-components.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/RemoteOptionsContainer.tsx
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/SelectComponent.tsxpackages/connect-react/src/components/ControlSelect.tsxpackages/connect-react/src/components/SelectApp.tsx
🧬 Code graph analysis (6)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (5)
packages/connect-react/src/types.ts (2)
RawPropOption(50-56)ConfigureComponentContext(58-58)packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)packages/connect-react/src/components/InternalField.tsx (1)
onChange(45-47)packages/connect-react/src/components/ControlSelect.tsx (1)
ControlSelect(36-251)
packages/connect-react/src/hooks/use-apps.tsx (2)
types/src/index.ts (1)
App(170-178)packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
packages/connect-react/src/components/SelectComponent.tsx (1)
packages/connect-react/src/hooks/use-components.tsx (1)
useComponents(23-104)
packages/connect-react/src/hooks/use-components.tsx (1)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
packages/connect-react/src/components/SelectApp.tsx (1)
packages/connect-react/src/hooks/use-apps.tsx (1)
useApps(22-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
🔇 Additional comments (4)
packages/connect-react/src/hooks/use-components.tsx (1)
56-71: Good fix for initial-load when input is undefinedNormalizing with input ?? null prevents the first run from being skipped. Looks correct.
packages/connect-react/src/components/SelectApp.tsx (1)
66-72: Good use of ref to avoid stale isLoadingMore in MenuListReading isLoadingMore via a ref keeps components stable and the indicator accurate. Solid approach.
Also applies to: 140-156
packages/connect-react/src/components/SelectComponent.tsx (1)
43-49: Stale-closure issue handled correctly with ref-backed indicatorThe ref pattern ensures “Loading more …” reflects current state without remounting MenuList.
Also applies to: 69-92
packages/connect-react/src/hooks/use-apps.tsx (1)
57-57: Initial load issue resolvedThe past review comments flagged that
JSON.stringify(input)returnsundefinedwheninputisundefined, preventing initial load. Your fix withinput ?? nullcorrectly normalizesundefinedto a stable"null"string, soisNewQueryistrueon first load andallAppsgets populated.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/connect-react/src/hooks/use-apps.tsx (1)
1-152: Consider extracting shared pagination logic.The pagination implementation in
use-apps.tsxis nearly identical touse-components.tsx. While the current duplication is acceptable and working correctly, consider extracting the shared pagination logic into a reusable custom hook to improve maintainability.For example, a generic
usePaginatedQueryhook could accept the query function and return the pagination state and methods, reducing duplication across similar hooks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/connect-react/src/components/RemoteOptionsContainer.tsx(4 hunks)packages/connect-react/src/hooks/use-apps.tsx(1 hunks)packages/connect-react/src/hooks/use-components.tsx(1 hunks)packages/connect-react/src/utils/pagination.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/RemoteOptionsContainer.tsx
🧬 Code graph analysis (3)
packages/connect-react/src/hooks/use-apps.tsx (2)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)packages/connect-react/src/utils/pagination.ts (3)
PaginatedPage(1-5)isPaginatedPage(7-21)clonePaginatedPage(23-26)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (5)
packages/connect-react/src/types.ts (2)
RawPropOption(50-56)ConfigureComponentContext(58-58)packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)packages/connect-react/src/components/InternalField.tsx (1)
onChange(45-47)packages/connect-react/src/components/ControlSelect.tsx (1)
ControlSelect(36-251)
packages/connect-react/src/hooks/use-components.tsx (2)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)packages/connect-react/src/utils/pagination.ts (3)
PaginatedPage(1-5)isPaginatedPage(7-21)clonePaginatedPage(23-26)
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/src/components/RemoteOptionsContainer.tsx
[error] 159-159: ESLint error: '_page' is assigned a value but never used. (no-unused-vars). Command: pnpm exec eslint packages/connect-react/CHANGELOG.md packages/connect-react/package.json packages/connect-react/src/components/ControlSelect.tsx packages/connect-react/src/components/RemoteOptionsContainer.tsx packages/connect-react/src/components/SelectApp.tsx packages/connect-react/src/components/SelectComponent.tsx packages/connect-react/src/hooks/use-apps.tsx packages/connect-react/src/hooks/use-components.tsx packages/connect-react/src/utils/pagination.ts
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/utils/pagination.ts
[failure] 23-23:
A space is required after ','
[failure] 7-7:
A space is required after ','
packages/connect-react/src/hooks/use-apps.tsx
[failure] 92-92:
A linebreak is required before ']'
[failure] 92-92:
A linebreak is required after '['
packages/connect-react/src/components/RemoteOptionsContainer.tsx
[failure] 160-160:
'_prevContext' is assigned a value but never used
[failure] 159-159:
'_page' is assigned a value but never used
packages/connect-react/src/hooks/use-components.tsx
[failure] 93-93:
A linebreak is required before ']'
[failure] 93-93:
A linebreak is required after '['
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pnpm publish
🔇 Additional comments (11)
packages/connect-react/src/hooks/use-components.tsx (4)
1-27: LGTM! Clean type definition for paginated components.The new
UseComponentsResulttype properly exposes pagination state (isLoadingMore,hasMore,loadMore,loadMoreError) alongside the components data, enabling infinite scroll behavior.
32-66: LGTM! Proper state management and memory leak prevention.The use of
isMountedRefto prevent state updates after unmount is a solid pattern that addresses potential memory leaks in async operations.
104-143: LGTM! Robust pagination implementation with proper guards.The
loadMorefunction has comprehensive safety checks:
- Type guard validation with
isPaginatedPage- Mount status checks to prevent state updates after unmount
- Proper error handling with
loadMoreErrorstate- Guard against concurrent loads
145-152: LGTM! Clean public API for paginated components.The return value properly exposes the accumulated components and all pagination state, making it straightforward for consumers to implement infinite scroll.
packages/connect-react/src/components/RemoteOptionsContainer.tsx (5)
27-31: LGTM! Proper option value extraction using sanitizer.The
extractOptionValuehelper correctly usessanitizeOptionto handle SDK wrapper formats (lv/__lv) before extracting the value, preventing false "invalid option" warnings.
84-139: LGTM! Proper memoization prevents unnecessary re-computations.The memoized values (
configuredPropsUpTo,accountValue,componentConfigureInput,queryKeyInput) have correct dependency arrays and prevent expensive re-computations.
168-184: LGTM! Proper query enabling logic with account validation.The
isQueryEnabledlogic correctly ensures that when anappprop exists, the query only runs after the account is configured, preventing unnecessary or invalid API calls.
218-225: LGTM! Options are properly merged.The implementation correctly merges both
optionsandstringOptionsarrays using spread syntax, ensuring neither set is lost.
236-327: Add unit tests coveringcanLoadMorepagination logic
No existing tests validate that when a paginated response returns no new options,canLoadMoreis correctly set to false under React 18 automatic batching. Add tests forRemoteOptionsContainerto simulate both first‐page and subsequent‐page scenarios and assert thecanLoadMoreflag flips as expected.packages/connect-react/src/hooks/use-apps.tsx (2)
103-142: LGTM! Robust pagination with comprehensive safety checks.The
loadMoreimplementation properly handles:
- Type validation with
isPaginatedPageguard- Memory leak prevention with mount checks
- Error handling with exposed
loadMoreErrorstate- Concurrent load prevention
144-151: LGTM! Clean public API for paginated apps.The return value properly exposes the accumulated apps and all pagination state, making it straightforward for consumers to implement infinite scroll.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
83-114: Fix selectValue: arrays and __lv wrapper mishandled; value can reset/null, multi-select breaks
- Arrays of LV options fall through to return null.
- __lv may hold arrays; current code sanitizes as a single option.
- Causes flicker/clearing and wrong shape for isMulti.
Apply this diff to robustly unwrap and normalize single/array values:
- const selectValue: LabelValueOption<T> | LabelValueOption<T>[] | null = useMemo(() => { - if (rawValue == null) { - return null; - } - - if (Array.isArray(rawValue)) { - // if simple, make lv (XXX combine this with other place this happens) - if (!isOptionWithLabel(rawValue[0])) { - return rawValue.map((o) => - selectOptions.find((item) => item.value === o) || sanitizeOption(o as T)); - } - } else if (rawValue && typeof rawValue === "object" && "__lv" in (rawValue as Record<string, unknown>)) { - // Extract the actual option from __lv wrapper and sanitize to LV - return sanitizeOption(((rawValue as Record<string, unknown>).__lv) as T); - } else if (!isOptionWithLabel(rawValue)) { - const lvOptions = selectOptions?.[0] && isOptionWithLabel(selectOptions[0]); - if (lvOptions) { - for (const item of selectOptions) { - if (item.value === rawValue) { - return item; - } - } - } else { - return sanitizeOption(rawValue as T); - } - } - - return null; - }, [ - rawValue, - selectOptions, - ]); + const selectValue: LabelValueOption<T> | LabelValueOption<T>[] | null = useMemo(() => { + const unwrap = (v: unknown): unknown => { + if (v && typeof v === "object" && "__lv" in (v as Record<string, unknown>)) { + return (v as Record<string, unknown>).__lv as unknown; + } + return v; + }; + const toLV = (val: unknown): LabelValueOption<T> => { + if (isOptionWithLabel(val)) { + return sanitizeOption(val as LabelValueOption<T>); + } + const match = selectOptions.find((item) => item.value === val); + return match ?? sanitizeOption(val as T); + }; + + const candidate = unwrap(rawValue); + if (candidate == null) return null; + + if (Array.isArray(candidate)) { + if (candidate.length === 0) return []; + return candidate.map((v) => toLV(v)); + } + + return toLV(candidate); + }, [rawValue, selectOptions]);
🧹 Nitpick comments (7)
packages/connect-react/src/components/ControlSelect.tsx (1)
133-137: Also merge selectProps.components to preserve caller overridesselectProps.components are overridden later by components={finalComponents}. Merge them into mergedComponents and add dep.
- const mergedComponents = { - ...(props.components ?? {}), - ...(componentsOverride ?? {}), - }; + const mergedComponents = { + ...(props.components ?? {}), + ...(selectProps?.components ?? {}), + ...(componentsOverride ?? {}), + }; @@ - ], [ - props.components, - componentsOverride, - ]); + ], [ + props.components, + componentsOverride, + selectProps?.components, + ]);Also applies to: 159-162
packages/connect-react/src/hooks/use-apps.tsx (2)
73-73: Consider extracting the page size limit as a named constant.The hard-coded limit of 50 is a magic number that appears in multiple pagination implementations. Extract it to a shared constant for consistency and maintainability.
+const DEFAULT_PAGE_SIZE = 50; + const query = useQuery({ queryKey: [ "apps", input, ], queryFn: () => client.apps.list({ - limit: 50, + limit: DEFAULT_PAGE_SIZE, ...input, }), });
108-111: Redundant runtime validation.Since
nextPageis already typed asPaginatedPage<App> | nulland you're checking!nextPageon line 106, theisPaginatedPagevalidation on line 108 is redundant. The TypeScript type system and your null check already provide this guarantee.const loadMore = useCallback(async () => { if (!nextPage || !hasMore || isLoadingMore) return; - if (!isPaginatedPage<App>(nextPage)) { - setLoadMoreError(new Error("Next page response is not paginated")); - return; - } - setIsLoadingMore(true);Alternatively, if you want defense-in-depth against SDK breaking changes, keep the check but move it after the
getNextPage()call to validate the response rather than the input.packages/connect-react/src/components/RemoteOptionsContainer.tsx (4)
77-82: Consider consistent unused variable handling.Lines 163-164 use
voidstatements to suppress unused variable warnings, while here you use an eslint-disable comment. For consistency across the file, consider using the same approach in both places.- // State variable unused - we only use the setter and derive values from prevValues const [ - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _accumulatedValues, setAccumulatedValues, ] = useState<Set<PropOptionValue>>(new Set()); + // We only use the setter, not the value itselfOr apply the void pattern from lines 163-164 if you prefer that style.
135-139: Redundant memoization layer.
queryKeyInputis a memoized wrapper that simply returnscomponentConfigureInputwithout any transformation. This adds cognitive overhead without clear benefit.Consider using
componentConfigureInputdirectly in the query key:- const queryKeyInput = useMemo(() => { - return componentConfigureInput; - }, [ - componentConfigureInput, - ]); - // ... const { data: queryData, isFetching, refetch, dataUpdatedAt, } = useQuery<ConfigurePropResult>({ queryKey: [ "componentConfigure", - queryKeyInput, + componentConfigureInput, ],Then update line 152 and other references accordingly.
163-164: Unconventional unused variable suppression.Using
voidstatements to suppress linter warnings is valid but unusual. The more common patterns are:
- Prefix with underscore and add eslint-disable comment (as you did on line 79-80)
- Use a comment like
// @ts-expect-error unused- Remove the variables entirely:
const { page, prevContext, ...rest } = ...without the underscore prefixesCurrent implementation works but may confuse readers unfamiliar with this pattern.
238-329: Consider derivingcanLoadMorefrom actual accumulated data.The current implementation mutates
foundNewOptionsinside thesetAccumulatedValuesupdater (line 275) then reads it at line 321. While this works because React executes updater functions synchronously, it's fragile and harder to reason about.The previous review suggested computing
canLoadMoreinsidesetAccumulatedDataby checking ifnewOptions.length > 0. This would be more robust and clearer:// Sync query data into accumulated state useEffect(() => { if (!queryData) return; if (queryData.error) { setError(queryData.error); return; } setError(undefined); setNextContext(queryData.context); const isFirstPage = page === 0; - let foundNewOptions = false; - setAccumulatedValues((prevValues) => { - const baseValues = isFirstPage ? new Set<PropOptionValue>() : prevValues; - const newValues = new Set(baseValues); - for (const o of queryData.options) { - const value = extractOptionValue(o); - if (value === null) { - console.warn("Skipping invalid option:", o); - continue; - } - if (!newValues.has(value)) { - newValues.add(value); - foundNewOptions = true; - } - } - return newValues; - }); setAccumulatedData((prevData) => { const baseData = isFirstPage ? [] : prevData; - const newOptions: RawPropOption[] = []; - const tempValues = new Set<PropOptionValue>(); + const seen = new Set<PropOptionValue>(); - if (!isFirstPage) { - for (const o of baseData) { - const value = extractOptionValue(o); - if (value !== null) { - tempValues.add(value); - } - } + for (const o of baseData) { + const value = extractOptionValue(o); + if (value !== null) seen.add(value); } + const newOptions: RawPropOption[] = []; for (const o of queryData.options) { const value = extractOptionValue(o); - if (value === null) continue; - if (!tempValues.has(value)) { - tempValues.add(value); + if (value === null) { + console.warn("Skipping invalid option:", o); + continue; + } + if (!seen.has(value)) { + seen.add(value); newOptions.push(o); } } + // Derive canLoadMore from actual new items + setCanLoadMore(newOptions.length > 0); + if (!newOptions.length) return prevData; return [...baseData, ...newOptions] as RawPropOption[]; }); - - if (!foundNewOptions) { - setCanLoadMore(false); - } - }, [queryData, page]); + }, [queryData, page, dataUpdatedAt]);Also consider removing
queryKeyStringfrom the dependency array (line 328) since changes toqueryDataalready capture query key changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/connect-react/src/components/ControlSelect.tsx(3 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(4 hunks)packages/connect-react/src/components/SelectApp.tsx(3 hunks)packages/connect-react/src/components/SelectComponent.tsx(2 hunks)packages/connect-react/src/hooks/use-apps.tsx(1 hunks)packages/connect-react/src/hooks/use-components.tsx(1 hunks)packages/connect-react/src/utils/pagination.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-27T16:47:38.177Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/ControlSelect.tsx:138-145
Timestamp: 2025-08-27T16:47:38.177Z
Learning: In the connect-react package, changes should be non-behavioral during the SDK migration. The LoadMore functionality in ControlSelect should maintain its current runtime behavior.
Applied to files:
packages/connect-react/src/components/ControlSelect.tsxpackages/connect-react/src/components/SelectApp.tsxpackages/connect-react/src/components/SelectComponent.tsx
📚 Learning: 2025-08-27T17:25:10.425Z
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/utils/type-guards.ts:23-33
Timestamp: 2025-08-27T17:25:10.425Z
Learning: In the connect-react package, the isOptionWithLabel type guard intentionally restricts value types to string|number for runtime filtering purposes, even though LabelValueOption<T> allows any T. This runtime behavior should be preserved over type safety improvements.
Applied to files:
packages/connect-react/src/components/RemoteOptionsContainer.tsx
🧬 Code graph analysis (6)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/types.ts (1)
LabelValueOption(40-42)
packages/connect-react/src/hooks/use-apps.tsx (3)
types/src/index.ts (1)
App(170-178)packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)packages/connect-react/src/utils/pagination.ts (3)
PaginatedPage(1-5)isPaginatedPage(7-21)clonePaginatedPage(23-26)
packages/connect-react/src/hooks/use-components.tsx (2)
packages/connect-react/src/hooks/frontend-client-context.tsx (1)
useFrontendClient(13-21)packages/connect-react/src/utils/pagination.ts (3)
PaginatedPage(1-5)isPaginatedPage(7-21)clonePaginatedPage(23-26)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (5)
packages/connect-react/src/types.ts (2)
RawPropOption(50-56)ConfigureComponentContext(58-58)packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)packages/connect-react/src/components/InternalField.tsx (1)
onChange(45-47)packages/connect-react/src/components/ControlSelect.tsx (1)
ControlSelect(36-246)
packages/connect-react/src/components/SelectApp.tsx (1)
packages/connect-react/src/hooks/use-apps.tsx (1)
useApps(31-154)
packages/connect-react/src/components/SelectComponent.tsx (1)
packages/connect-react/src/hooks/use-components.tsx (1)
useComponents(32-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
🔇 Additional comments (12)
packages/connect-react/src/components/ControlSelect.tsx (1)
118-126: Good: stable refs for callbacks/flags and wrapper preserves MenuList overridesRef-based onLoadMore/showLoadMore avoids remounts; wrapping ParentMenuList respects consumer overrides. Matches non-behavioral goal.
Based on learnings
Also applies to: 127-129, 139-151
packages/connect-react/src/components/SelectComponent.tsx (1)
52-61: Infinite scroll + stable MenuList indicator looks solid
- Guarded loadMore with hasMore/!isLoadingMore.
- Ref-based isLoadingMore avoids remounts; deps are minimal.
LGTM.
Also applies to: 63-86, 88-101
packages/connect-react/src/utils/pagination.ts (1)
1-26: Utility is clean and typed; clone preserves prototypeType guard checks methods; clone keeps methods via prototype. Lint-friendly generics. LGTM.
packages/connect-react/src/components/SelectApp.tsx (1)
79-89: Solid pagination integration; stable loading indicator
- Debounced search + onMenuScrollToBottom guarded correctly.
- Ref for isLoadingMore prevents remounts; custom components memoized.
LGTM.
Also applies to: 90-155, 161-175
packages/connect-react/src/hooks/use-components.tsx (1)
21-27: Hook implements robust pagination with mount guards and reset-on-input-change
- isMountedRef prevents setState after unmount.
- Proper reset when input or data identity changes.
- Concurrency guard on loadMore.
LGTM.
Also applies to: 32-50, 56-67, 79-104, 106-145, 147-155
packages/connect-react/src/hooks/use-apps.tsx (3)
1-26: LGTM! Clean pagination type definition.The UseAppsResult type correctly extends the query result while adding pagination-specific fields (apps, isLoadingMore, hasMore, loadMore, loadMoreError).
78-103: LGTM! Proper pagination reset logic.The effect correctly:
- Detects input and data changes using refs
- Validates the response is paginated before processing
- Clones the paginated page to avoid mutation
- Resets all pagination state appropriately
146-153: LGTM! Complete return signature.The return object correctly spreads the underlying query metadata and exposes all pagination-related fields defined in UseAppsResult.
packages/connect-react/src/components/RemoteOptionsContainer.tsx (4)
21-31: LGTM! Correct option value extraction.The helper now properly uses
sanitizeOptionto handle SDK-wrapped options (lv/__lv), addressing the previous review concern.Based on learnings
189-236: LGTM! Robust query function.The query function correctly:
- Returns structured ConfigurePropResult
- Handles error parsing with fallback
- Merges both options and stringOptions arrays (addressing previous review)
- Uses isQueryEnabled to prevent unnecessary requests
331-376: LGTM! Proper reset and account-aware behavior.The effects correctly:
- Reset pagination only when external query params change (not on page/context changes)
- Clear the field value when the connected account changes
- Refetch data when account changes to a non-null value, handling cases where the queryKey returns to a previous value
378-425: LGTM! Correct rendering logic.The component correctly:
- Derives
showLoadMoreButtonfrom loading/error/pagination state- Uses
isQueryEnabledto control placeholder and disabled state appropriately- Sanitizes options before passing to ControlSelect
- Handles the disableQueryDisabling edge case for stress testing
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Chores