Fix pre-configured props handling issues v2 #18872
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>
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>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Warning Rate limit exceeded@dannyroosevelt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughPR adds trailing newlines to three app files, introduces a new label-value utility module, updates ControlSelect to detect and handle __lv-wrapped arrays and objects, and modifies form-context to auto-enable optional props, preserve optional values (including label-value integer forms), and make queryDisabledIdx initialization reactive. Changes
Sequence Diagram(s)sequenceDiagram
participant C as ControlSelect
participant U as LabelValueUtils
participant S as Sanitizer
participant K as Caller
C->>C: receive rawValue
C->>U: hasLabelValueFormat(rawValue)?
alt array of __lv
C->>S: sanitize each element
S-->>C: sanitized array
C-->>K: return sanitized array
else single __lv object
C->>S: sanitize object
S-->>C: sanitized value
C-->>K: return sanitized value
else not label-value
C-->>K: return original value
end
sequenceDiagram
participant M as Component Mount
participant E as Effects
participant FS as FormState
participant Q as QueryController
M->>E: initialize with configuredProps & enabledOptionalProps
E->>FS: enable optional props that have saved values
E->>FS: set queryDisabledIdx from configuredProps
E-->>Q: allow/trigger queries based on queryDisabledIdx
note right of E: Separate effects keep updates reactive and reduce races
FS->>FS: preserve values for disabled optional props
FS->>FS: retain __lv format for integer remote options
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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)
384-400: Consider adding missing dependencies to useEffect hooks.The effects at lines 384-391 and 393-400 call
updateConfiguredPropsQueryDisabledIdx, which accessesconfigurablePropsandoptionalPropIsEnabledfrom the outer scope. These should be included in the dependency arrays to prevent potential stale closure issues.While the current implementation likely works due to the interconnected state updates, adding these dependencies would follow React best practices and make the code more robust to future changes.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/alienvault/alienvault.app.mjs(1 hunks)components/beyond_presence/beyond_presence.app.mjs(1 hunks)components/callhippo/callhippo.app.mjs(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(1 hunks)packages/connect-react/src/hooks/form-context.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jverce
PR: PipedreamHQ/pipedream#18187
File: packages/connect-react/src/components/Errors.tsx:16-19
Timestamp: 2025-08-27T16:48:48.776Z
Learning: In the connect-react package refactoring PR, maintain current runtime behavior even when type patterns are not ideal - prioritize behavioral consistency over type safety improvements when explicitly requested by the maintainer.
🧬 Code graph analysis (1)
packages/connect-react/src/components/ControlSelect.tsx (1)
packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)
⏰ 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). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
🔇 Additional comments (7)
components/callhippo/callhippo.app.mjs (1)
11-11: LGTM! Formatting improvement.Adding a trailing newline follows best practices for text files.
components/alienvault/alienvault.app.mjs (1)
11-11: LGTM! Formatting improvement.Adding a trailing newline follows best practices for text files.
components/beyond_presence/beyond_presence.app.mjs (1)
11-11: LGTM! Formatting improvement.Adding a trailing newline follows best practices for text files.
packages/connect-react/src/components/ControlSelect.tsx (1)
94-101: LGTM! Enhanced handling of __lv-wrapped values.The logic correctly handles both single objects and arrays wrapped in
__lvformat. This aligns with the broader PR changes to properly preserve label-value structures for remote options.packages/connect-react/src/hooks/form-context.tsx (3)
279-303: LGTM! Valuable enhancement for pre-configured props.This effect ensures that optional fields with saved values are automatically shown when mounting with pre-configured props. The batch update approach and state merging logic are well-implemented.
425-432: LGTM! Prevents losing pre-configured optional prop values.Preserving values for optional props that haven't been enabled yet ensures that pre-configured data isn't lost, even when those fields aren't currently visible. This works well with the auto-enable effect to properly restore form state.
441-453: LGTM! Proper preservation of label-value structures for integer fields.The logic correctly identifies and preserves remote option values stored in
__lvformat (both single objects and arrays), while removing incorrectly formatted integer values. This aligns with the ControlSelect changes to properly handle__lv-wrapped values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/connect-react/src/hooks/form-context.tsx (2)
279-304: Consider removingenabledOptionalPropsfrom the dependency array.The effect sets
enabledOptionalPropsand also includes it in the dependency array. While the guard condition prevents infinite loops, including state that the effect modifies in its own dependencies is unconventional and can be confusing to maintainers.The effect should run when
component.key,configurableProps, orconfiguredPropschange. The checks inside already protect against re-enabling already-enabled props, so removingenabledOptionalPropsfrom the deps should be safe and clearer.Apply this diff to simplify the dependency array:
}, [ component.key, configurableProps, configuredProps, - enabledOptionalProps, ]);
446-461: Consider extracting label-value detection logic into a utility function.The
__lvformat detection logic is specific and testable. Since the AI summary mentions similar handling inControlSelect.tsx, extracting this into a shared utility would reduce duplication and make the logic easier to test and maintain.Consider creating a utility function like:
// utils/label-value.ts export function isLabelValueWrapped(value: unknown): boolean { if (!value || typeof value !== "object") return false; return "__lv" in value; } export function isArrayOfLabelValueWrapped(value: unknown): boolean { if (!Array.isArray(value) || value.length === 0) return false; return value.every((item) => item && typeof item === "object" && "__lv" in item); } export function hasLabelValueFormat(value: unknown): boolean { return isLabelValueWrapped(value) || isArrayOfLabelValueWrapped(value); }Then simplify the logic to:
if (prop.type === "integer" && typeof value !== "number") { - 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); - - if (!(isLabelValue || isArrayOfLabelValues)) { + if (!hasLabelValueFormat(value)) { 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/ControlSelect.tsx(1 hunks)packages/connect-react/src/hooks/form-context.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connect-react/src/components/ControlSelect.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/connect-react/src/hooks/form-context.tsx (1)
components/streak/actions/update-box-field-value/update-box-field-value.mjs (1)
prop(49-49)
⏰ 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). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (4)
packages/connect-react/src/hooks/form-context.tsx (4)
385-395: LGTM!The initialization logic correctly uses
configuredPropsinstead of_configuredProps, ensuring that when mounting in controlled mode with pre-configured values, the query disabled index is properly initialized. The comments clearly explain the rationale.
396-405: LGTM - dual effect pattern handles initialization and updates correctly.The separation of initialization (lines 385-395) and reactive updates (lines 396-405) is well-explained. Both effects call the same
updateConfiguredPropsQueryDisabledIdxfunction, which is idempotent, so overlapping calls during mount are safe.
430-437: LGTM - preserves optional prop values correctly.The logic now preserves values for optional props even when they're not enabled, preventing data loss when mounting with pre-configured props. The comment clearly explains the intent.
467-474: LGTM - theisEqualcheck prevents infinite loops.Adding
configuredPropsto the dependency array (line 473) of an effect that updatesconfiguredPropsis safe because of the guard at line 467 that only callsupdateConfiguredPropswhen there are actual changes. This is a critical safeguard.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/connect-react/src/utils/label-value.ts (1)
34-43: Consider reusingisLabelValueWrappedto avoid duplication.The validation logic in lines 38-42 duplicates the checks from
isLabelValueWrapped. Consider refactoring to reuse the existing function.Apply this diff:
export function isArrayOfLabelValueWrapped(value: unknown): boolean { if (!Array.isArray(value)) return false; if (value.length === 0) return false; - return value.every((item) => - item && - typeof item === "object" && - "__lv" in item && - (item as Record<string, unknown>).__lv != null); + return value.every((item) => isLabelValueWrapped(item)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/connect-react/src/components/ControlSelect.tsx(2 hunks)packages/connect-react/src/hooks/form-context.tsx(7 hunks)packages/connect-react/src/utils/label-value.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/connect-react/src/components/ControlSelect.tsx
- packages/connect-react/src/hooks/form-context.tsx
⏰ 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). (4)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (3)
packages/connect-react/src/utils/label-value.ts (3)
16-22: LGTM!The guard sequence is well-structured and handles edge cases appropriately.
36-36: Verify empty array handling is intentional.The function returns
falsefor empty arrays. While documented in the JSDoc, this may be unexpected—semantically, an empty array could be considered a valid "array of label-value wrapped objects" (with zero elements). Please confirm this behavior aligns with how the utilities are used inControlSelect.tsxandform-context.tsx.
55-57: LGTM!Clean composition of the two utility functions with clear semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/connect-react/src/hooks/form-context.tsx (1)
511-526: Refactor to avoid unnecessary effect re-runs.Including
optionalPropSetEnabledin the dependency array causes this effect to run on every render because the function is not memoized. While the logic has guards to prevent infinite loops, this is inefficient.Consider one of these solutions:
Option 1: Remove the function from dependencies (with justification)
}, [ component.key, configurableProps, configuredProps, enabledOptionalProps, - optionalPropSetEnabled, + // eslint-disable-next-line react-hooks/exhaustive-deps + // optionalPropSetEnabled is stable and doesn't need to be in deps ]);Option 2: Memoize
optionalPropSetEnabledwithuseCallback(preferred for React best practices)Wrap the function definition at line 488 with
useCallbackand appropriate dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/connect-react/src/components/ControlSelect.tsx(2 hunks)packages/connect-react/src/hooks/form-context.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/connect-react/src/hooks/form-context.tsx (1)
packages/connect-react/src/utils/label-value.ts (1)
hasLabelValueFormat(55-57)
packages/connect-react/src/components/ControlSelect.tsx (3)
packages/connect-react/src/utils/label-value.ts (2)
isLabelValueWrapped(16-22)isArrayOfLabelValueWrapped(34-43)packages/connect-react/src/utils/type-guards.ts (1)
sanitizeOption(135-175)packages/connect-react/src/types.ts (1)
RawPropOption(50-56)
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/src/hooks/form-context.tsx
[error] 363-363: ESLint: 'multiline-ternary' rule violation. Expected newline between test and consequent of ternary expression.
🪛 GitHub Check: Lint Code Base
packages/connect-react/src/hooks/form-context.tsx
[failure] 363-363:
Expected newline between consequent and alternate of ternary expression
[failure] 363-363:
Expected newline between test and consequent of ternary expression
⏰ 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). (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (5)
packages/connect-react/src/components/ControlSelect.tsx (2)
19-30: LGTM! Clean import organization.The imports are well-organized, combining related types and utilities appropriately.
95-107: Excellent handling of __lv-wrapped values.The logic correctly unwraps label-value formatted values from remote options, handling both single wrapped objects and arrays of wrapped objects. The progressive checks (single wrapped → array of wrapped → unwrapped) provide clear control flow and preserve backward compatibility.
packages/connect-react/src/hooks/form-context.tsx (3)
366-375: Verify the intentional omission ofconfiguredPropsfrom dependencies.The initialization effect calls
updateConfiguredPropsQueryDisabledIdx(configuredProps)but doesn't includeconfiguredPropsin the dependency array. While there's a separate reactive effect below (lines 377-386) that handlesconfiguredPropschanges, this separation could lead to subtle timing issues ifconfiguredPropschanges during the initial render before the reactive effect runs.Consider whether
configuredPropsshould be included in this initialization effect's dependencies, or add a comment explaining why the dual-effect pattern is necessary.
377-386: Good defensive pattern to prevent race conditions.The reactive update effect properly handles
configuredPropschanges and includes clear documentation explaining the race condition prevention. The dependencies are appropriate for this reactive pattern.
411-418: Excellent preservation of optional prop values.This change correctly prevents data loss for optional props that haven't been enabled yet. The logic and comments are clear.
2 participants
branched off of #18782 to resolve conflicts
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores