fix(connect-react): Fix pre-configured props handling issues #18782
Conversation
## Issues Fixed ### Issue 1: Remote options not loading with pre-configured values - **Problem**: When mounting ComponentFormContainer with pre-configured props, remote options dropdowns showed "No options" even though the API returned data - **Root Cause**: queryDisabledIdx initialization used _configuredProps (empty) instead of actual configuredProps, incorrectly blocking queries. RemoteOptionsContainer also didn't sync cached query data with component state on remount - **Files**: form-context.tsx, RemoteOptionsContainer.tsx ### Issue 2: Optional props not auto-enabling when pre-configured - **Problem**: Optional fields with saved values were hidden when switching back to a previously configured component - **Root Cause**: enabledOptionalProps reset on component change, never re-enabling optional fields that had values - **File**: form-context.tsx ### Issue 3: Optional prop values lost during state sync - **Problem**: Optional field values were discarded during the state synchronization effect if the field wasn't enabled - **Root Cause**: Sync effect skipped disabled optional props entirely - **File**: form-context.tsx ## Fixes Applied ### form-context.tsx 1. Fixed queryDisabledIdx initialization to use configuredProps instead of _configuredProps - Changed dependency from _configuredProps to component.key - Ensures blocking index is calculated from actual current values including parent-passed props 2. Added useEffect to auto-enable optional props with values - Runs when component key or configurableProps/configuredProps change - Automatically enables any optional props that have values in configuredProps - Ensures optional fields with saved values are shown on mount 3. Modified sync effect to preserve optional prop values - Optional props that aren't enabled still have their values preserved - Prevents data loss during state synchronization ### RemoteOptionsContainer.tsx 1. Destructured data from useQuery return - Added data to destructured values to track query results 2. Modified queryFn to return pageable object - Changed from returning just raw data array to returning full newPageable state object - Enables proper state syncing 3. Added useEffect to sync pageable state with query data - Handles both fresh API calls and React Query cached returns - When cached data is returned, queryFn doesn't run but useEffect syncs the state - Ensures options populate correctly on component remount ## Expected Behavior After Fixes ✓ Remote option fields load correctly when mounting with pre-configured values ✓ Dropdown shows fetched options even when using cached data ✓ Optional fields with saved values are automatically enabled and visible ✓ No data loss when switching between components ✓ Smooth component switching with all values and options preserved 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
When a dependent field changes (e.g., Channel Type: "Channels" → "User/Direct Message"), the Channel dropdown should replace its options instead of accumulating them. The fix uses page-based logic to determine whether to replace or append options: - page === 0 (fresh query): Replace options with new data - page > 0 (pagination): Append options to existing data When dependent fields change, the useEffect resets page to 0, which triggers the queryFn to replace options instead of appending. This prevents accumulation of options from different queries. Additionally, the allValues Set is reset on fresh queries to ensure deduplication starts fresh, not carrying over values from the previous query. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When a field value changes, two /configure API calls were being made:
1. First call with empty configured_props: {}
2. Second call with correct configured_props: {field: value}
Root cause: In setConfiguredProp, updateConfiguredPropsQueryDisabledIdx was called
synchronously, updating queryDisabledIdx state before configuredProps state update
completed. This caused children to re-render twice with mismatched state.
Fix: Move queryDisabledIdx update to a reactive useEffect that watches configuredProps
changes. This ensures both state updates complete before children re-render, preventing
the duplicate API call with stale data.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Two related fixes to prevent field value loss and crashes:
1. Preserve label-value format for integer props
When integer properties with remoteOptions (like worksheetId) are selected
from dropdowns, the values are stored in label-value format: {__lv: {label, value}}.
The sync effect was incorrectly deleting these values because they weren't
pure numbers. Now preserves __lv format for remote option dropdowns.
2. Return proper pageable structure on error in RemoteOptionsContainer
When /configure returns an error, queryFn was returning [] instead of
the expected pageable object {page, data, prevContext, values}. This caused
pageable.data.map() to crash. Now returns proper structure on error to
prevent crashes and display error message correctly.
Fixes:
- Worksheet ID field no longer resets after dynamic props reload
- No more crash when clearing app field
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughResolves multiple form-state and remote-options bugs: auto-enables optional props with saved values, synchronizes query-disabled keys to real configuredProps, preserves integer label-value formats, and implements page-aware option replacement/appending plus __lv-wrapped array handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormCtx as Form Context
participant Component
participant RemoteContainer as RemoteOptionsContainer
participant Query as React Query
participant ControlSelect
User->>Component: Mount form with configuredProps
Component->>FormCtx: initialize with configuredProps
FormCtx->>FormCtx: auto-enable optional props with saved values
FormCtx->>FormCtx: update queryDisabledIdx (effect) based on configuredProps
Component->>RemoteContainer: mount dependent remote option field
RemoteContainer->>Query: fetch options (page 0)
Query-->>RemoteContainer: return pageable (data,page,total)
Note right of RemoteContainer: page==0 → replace data (dedupe per-page)
RemoteContainer->>Component: render options
User->>Component: scroll / request more
Component->>RemoteContainer: request page 1
RemoteContainer->>Query: fetch options (page 1)
Query-->>RemoteContainer: return more items
Note right of RemoteContainer: page>0 → append data (dedupe)
RemoteContainer->>Component: update options
User->>Component: select option
Component->>ControlSelect: receive rawValue (may be __lv single or array)
ControlSelect->>ControlSelect: sanitize single or map sanitize over __lv array
ControlSelect-->>Component: provide selectValue (single or array)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
[pre_merge_check_pass] The pull request description meets the template requirements by including the mandatory WHY section. The description goes beyond the template with comprehensive WHAT, Testing, and Impact sections that clearly explain the fixes applied, the testing performed with specific components (Google Sheets and Slack), and confirmation that all checks pass with no breaking changes. The description directly references the linked issue #18778 and provides sufficient context for understanding the scope of the changes. [pre_merge_check_pass] The code changes comprehensively address all six objectives from linked issue #18778. The form-context.tsx modifications fix queryDisabledIdx initialization from configuredProps [#18778 Issue 1], auto-enable optional props with saved values [Issue 2], preserve optional values during sync [Issue 3], and preserve integer label-value format [Issue 6]. The RemoteOptionsContainer.tsx changes implement page-based logic to replace vs append options [Issue 4]. The reactive queryDisabledIdx synchronization removes the synchronous update pattern [Issue 5]. All code-level requirements from the linked issue are satisfied by the documented changes. [pre_merge_check_pass] All code changes in this pull request are directly in scope and aligned with the objectives from issue #18778. The modifications to form-context.tsx, RemoteOptionsContainer.tsx, and ControlSelect.tsx each address specific bugs identified in the linked issue: queryDisabledIdx initialization, optional props handling, remote options pagination logic, and preservation of label-value formatted values. No extraneous changes outside the scope of fixing pre-configured props handling issues are present. | ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect-react/src/hooks/form-context.tsx (1)
441-448: Strengthen validation for label-value format.The check verifies
__lvexists but doesn't validate its structure. According to the type definition incomponent.ts,__lvshould contain avalueproperty:{ __lv: { value: T } }. Invalid structures could cause runtime errors.Apply this diff to add structure validation:
- if (!(value && typeof value === "object" && "__lv" in value)) { + if (!(value && typeof value === "object" && "__lv" in value && typeof value.__lv === "object" && value.__lv !== null && "value" in value.__lv)) { delete newConfiguredProps[prop.name as keyof ConfiguredProps<T>]; } else { newConfiguredProps[prop.name as keyof ConfiguredProps<T>] = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/connect-react/src/components/RemoteOptionsContainer.tsx(6 hunks)packages/connect-react/src/hooks/form-context.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (2)
packages/connect-react/src/utils/component.ts (1)
PropOptionValue(14-18)packages/connect-react/src/types.ts (1)
RawPropOption(50-56)
🔇 Additional comments (9)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (5)
100-111: LGTM! Pagination reset logic is correct.The effect appropriately resets pagination state when dependencies change, ensuring fresh queries start from page 0. This aligns with the PR objective of making data replacement vs pagination deterministic.
150-156: LGTM! Error handling improvement prevents crashes.Returning a proper pageable structure on error instead of an empty array prevents downstream crashes when accessing pageable properties. This is a good defensive coding practice.
173-177: LGTM! Deduplication logic correctly handles fresh vs paginated queries.For page 0 (fresh queries), starting with an empty set ensures old options don't accumulate. For subsequent pages, using the existing set maintains deduplication across pages. This aligns with the PR objective.
196-212: LGTM! Replace vs append logic is deterministic.The page-based branching correctly replaces data on page 0 and appends on subsequent pages, addressing the PR objective of making remote options replacement vs pagination deterministic.
221-230: LGTM! Sync effect handles cached query results.This effect ensures pageable state updates even when React Query returns cached data without running queryFn, which is necessary for correct form state on remount with pre-configured props.
packages/connect-react/src/hooks/form-context.tsx (4)
279-303: LGTM! Auto-enable logic correctly handles pre-configured optional props.The effect properly identifies optional props with saved values and enables them, ensuring previously filled optional fields are visible when editing. This addresses a key PR objective.
384-391: LGTM! Initialization fix ensures remote options load correctly.Using
configuredPropsinstead of_configuredPropsfor initialization ensures that when mounting with pre-configured values in controlled mode, remote options queries are not incorrectly blocked. This addresses the PR objective about ensuring remote options load with configuredProps.
393-400: LGTM! Reactive queryDisabledIdx prevents race conditions.Deriving queryDisabledIdx reactively from configuredProps ensures the index updates after state changes complete, preventing race conditions that caused duplicate API calls with stale data. This addresses a key PR objective.
425-432: LGTM! Preserves optional prop values during dynamic reloads.Copying existing values for disabled optional props prevents silent data loss when optional props haven't been enabled yet. This addresses the PR objective about preserving values during dynamic props reloads.
Previously, multi-select integer fields (e.g., Worksheet ID(s)) displayed "[object Object]" instead of proper labels when populated with pre-configured values. This occurred because: 1. form-context.tsx only checked for single __lv objects, not arrays 2. ControlSelect.tsx tried to sanitize entire arrays instead of individual items Changes: - form-context.tsx: Check for both single __lv objects and arrays of __lv objects to preserve multi-select values during sync - ControlSelect.tsx: Extract array contents from __lv wrapper and map each item through sanitizeOption for proper rendering This completes the fix for pre-configured props handling with remote options. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/connect-react/src/hooks/form-context.tsx (1)
141-145: Minor: queryDisabledIdx default 0 can block first remote query.Defaulting to 0 marks the first prop as disabled until the init effect runs. Use undefined to start neutral.
Same diff as suggested above at lines 142–144.
packages/connect-react/src/components/ControlSelect.tsx (1)
87-93: Bug: return current LV value(s) when rawValue is already LabelValueOption.If rawValue is already LV (after create or local edits), selectValue falls through to null. Return it directly to avoid selection flicker.
Apply:
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)); } + // Already LabelValueOption[] + return rawValue as LabelValueOption<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 // Handle both single objects and arrays wrapped in __lv const lvContent = (rawValue as Record<string, unknown>).__lv; if (Array.isArray(lvContent)) { return lvContent.map((item) => sanitizeOption(item as T)); } return sanitizeOption(lvContent as T); -} else if (!isOptionWithLabel(rawValue)) { +} 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); } +} else { + // Already a single LabelValueOption + return rawValue as LabelValueOption<T>; }Also applies to: 101-112
🧹 Nitpick comments (3)
packages/connect-react/src/hooks/form-context.tsx (2)
281-303: Auto-enable optional props: avoid unnecessary re-renders.Good call enabling saved optional fields. Minor tweak: only update state when at least one flag flips from false→true to prevent extra renders.
Apply:
- if (Object.keys(propsToEnable).length > 0) { - setEnabledOptionalProps((prev) => ({ - ...prev, - ...propsToEnable, - })); - } + setEnabledOptionalProps((prev) => { + const changed = Object.keys(propsToEnable).some((k) => !prev[k]); + if (!changed) return prev; + return { ...prev, ...propsToEnable }; + });
441-454: Integer + label-value preservation: add SDK 'lv' support.Great safeguard for __lv. Remote options may also use the SDK ‘lv’ wrapper; include it to avoid dropping valid values.
Apply:
-const isLabelValue = value && typeof value === "object" && "__lv" in value; -const isArrayOfLabelValues = Array.isArray(value) && value.length > 0 && - value.every((item) => item && typeof item === "object" && "__lv" in item); +const isLabelValue = value && typeof value === "object" && ("__lv" in (value as Record<string, unknown>) || "lv" in (value as Record<string, unknown>)); +const isArrayOfLabelValues = + Array.isArray(value) && value.length > 0 && + value.every((item) => + item && + typeof item === "object" && + ("__lv" in (item as Record<string, unknown>) || "lv" in (item as Record<string, unknown>)) + );packages/connect-react/src/components/ControlSelect.tsx (1)
93-101: Optional: also accept top-level 'lv' wrapper for symmetry.sanitizeOption supports both '__lv' and 'lv'. You can mirror that here to reduce surprises.
Apply:
-} else if (rawValue && typeof rawValue === "object" && "__lv" in (rawValue as Record<string, unknown>)) { +} else if (rawValue && typeof rawValue === "object" && ("__lv" in (rawValue as Record<string, unknown>) || "lv" in (rawValue as Record<string, unknown>))) { - const lvContent = (rawValue as Record<string, unknown>).__lv; + const wrap = rawValue as Record<string, unknown>; + const lvContent = ("__lv" in wrap ? wrap.__lv : (wrap as any).lv);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/connect-react/src/components/ControlSelect.tsx(1 hunks)packages/connect-react/src/hooks/form-context.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)
🔇 Additional comments (2)
packages/connect-react/src/hooks/form-context.tsx (1)
425-433: Preserving optional values when not shown: LGTM.This prevents silent data loss during dynamic reloads. Consider whether hidden props should also be preserved (if product wants to retain values when a field temporarily becomes hidden).
Would you like hidden (prop.hidden) values preserved too during reloads?
packages/connect-react/src/components/ControlSelect.tsx (1)
95-101: Support for __lv arrays in selectValue: nice improvement.This aligns with the new preservation logic.
| // Initialize queryDisabledIdx using actual configuredProps (includes parent-passed values in controlled mode) | ||
| // instead of _configuredProps which starts empty. This ensures that when mounting with pre-configured | ||
| // values, remote options queries are not incorrectly blocked. | ||
| updateConfiguredPropsQueryDisabledIdx(configuredProps) | ||
| }, [ | ||
| _configuredProps, | ||
| component.key, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Init queryDisabledIdx from configuredProps: good. Also initialize state to undefined to avoid transient block.
The effect correctly derives the initial idx from actual configuredProps. To eliminate the brief 0 default, initialize with undefined.
Apply:
-] = useState<number | undefined>(0);
+] = useState<number | undefined>(undefined);(lines 142–144)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/connect-react/src/hooks/form-context.tsx around lines 142–144, the
queryDisabledIdx state is initialized to 0 causing a transient block; change its
initial value to undefined so the effect that calls
updateConfiguredPropsQueryDisabledIdx(configuredProps) can set the real value on
mount. Update the state type to allow undefined if necessary and audit
downstream uses to handle undefined (use nullish checks or fallback) so no
runtime errors occur.
| // Update queryDisabledIdx reactively when configuredProps changes. | ||
| // This prevents race conditions where queryDisabledIdx updates synchronously before | ||
| // configuredProps completes its state update, causing duplicate API calls with stale data. | ||
| useEffect(() => { | ||
| updateConfiguredPropsQueryDisabledIdx(configuredProps); | ||
| }, [ | ||
| configuredProps, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Reactive sync looks right; remove synchronous duplicate to prevent double work.
Now that queryDisabledIdx derives from configuredProps reactively, also calling updateConfiguredPropsQueryDisabledIdx inside updateConfiguredProps can double-trigger downstream logic.
Apply:
const updateConfiguredProps = (configuredProps: ConfiguredProps<T>) => {
setConfiguredProps(configuredProps);
- updateConfiguredPropsQueryDisabledIdx(configuredProps);
updateConfigurationErrors(configuredProps)
};(lines 365–369)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/connect-react/src/hooks/form-context.tsx around lines 393 to 401,
reactive syncing now updates queryDisabledIdx via useEffect so the synchronous
call inside updateConfiguredProps (lines 365–369) is redundant and can
double-trigger downstream logic; remove the direct call to
updateConfiguredPropsQueryDisabledIdx from updateConfiguredProps and rely solely
on the useEffect to update queryDisabledIdx when configuredProps changes to
prevent duplicate work.
3 participants
Fixes #18778
WHY
When using
@pipedream/connect-reactin production with persistent form state (saving user configurations and repopulating forms for editing), several critical issues prevent the forms from working correctly. Users configure component forms, save those configurations, and later need to edit them - but the current implementation has issues with queryDisabledIdx initialization, optional props handling, dropdown options management, and integer value preservation that break this workflow. All details are documented in #18778.WHAT
Testing
✅ Tested with Google Sheets and Slack components
✅ Verified pre-configured props scenarios
✅ All ESLint and TypeScript checks pass
✅ Build successful
Impact
Summary by CodeRabbit