Skip to content

Biz/dj 2825 remote options gets unset when loading configuredprops #15401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bzwrk merged 14 commits into from
Jan 27, 2025
Merged

Biz/dj 2825 remote options gets unset when loading configuredprops #15401

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b5e1a3a
Refactor ControlSelect to fix custom string input
bzwrk Jan 24, 2025
640f2d3
Fix custom string input state update
bzwrk Jan 24, 2025
5c320a7
Populate prev value of remote options on page load
bzwrk Jan 24, 2025
b9f775b
Pass prevValues to RemoteOptionsContainer
bzwrk Jan 24, 2025
e426ed6
Update package
bzwrk Jan 24, 2025
3eacf9b
Pass prevValues to RemoteOptionsContainer
bzwrk Jan 24, 2025
838b189
Ensure propsNeedConfiguring is updated after field registration
bzwrk Jan 24, 2025
34ca0da
Remove checkPropsNeedConfiguring call
bzwrk Jan 24, 2025
324d958
Cleanup
bzwrk Jan 24, 2025
acc6b47
Cleanup
bzwrk Jan 24, 2025
538182d
Merge branch 'master' into biz/dj-2825-remote-options-gets-unset-when…
bzwrk Jan 24, 2025
3a44c77
Update version and changelog
bzwrk Jan 24, 2025
238bfea
Commit pnpm-lock.yaml
bzwrk Jan 24, 2025
3803fdb
Remove unused prevValue
bzwrk Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/connect-react/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
<!-- markdownlint-disable MD024 -->
# Changelog

# [1.0.0-preview.24] - 2025-01-24

- Fix the bug where inputting multiple strings into an array prop would merge the strings into one
- Fix custom string input for remote options
- Fix the reloading of a previously selected remote option when re-rendering the form component

# [1.0.0-preview.23] - 2025-01-22

- Show the prop label instead of the value after selecting from a dropdown for string array props
Expand Down
2 changes: 1 addition & 1 deletion packages/connect-react/examples/nextjs/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/connect-react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@pipedream/connect-react",
"version": "1.0.0-preview.23",
"version": "1.0.0-preview.24",
"description": "Pipedream Connect library for React",
"files": [
"dist"
Expand Down
4 changes: 2 additions & 2 deletions packages/connect-react/src/components/Control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ export function Control<T extends ConfigurableProps, U extends ConfigurableProp>
} = props;
const { queryDisabledIdx } = form;
const {
prop, idx,
prop, idx, value,
} = field;
const app = "app" in field.extra
? field.extra.app
: undefined;

if (prop.remoteOptions || prop.type === "$.discord.channel") {
return <RemoteOptionsContainer queryEnabled={queryDisabledIdx == null || queryDisabledIdx >= idx} />;
return <RemoteOptionsContainer prevValues={value} queryEnabled={queryDisabledIdx == null || queryDisabledIdx >= idx} />;
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Type incompatibility detected with prevValues prop

The RemoteOptionsContainer explicitly types prevValues as never, indicating it should not receive any value. However, the Control component is attempting to pass a value to it. Either:

  • Remove the prevValues prop from the Control component's usage, or
  • Update RemoteOptionsContainerProps to accept the appropriate type if the prop is needed
🔗 Analysis chain

Verify type safety of the prevValues prop.

While the addition of prevValues={value} aligns with the PR's objective to maintain remote options state, please ensure:

  1. The RemoteOptionsContainer component properly handles undefined values
  2. The type of value matches what RemoteOptionsContainer expects for prevValues

Let's verify the type compatibility:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for RemoteOptionsContainer's type definition and usage
ast-grep --pattern 'type $_ = {
  $$$
  prevValues?: $_
  $$$
}'

# Search for all usages to verify consistent typing
rg -A 2 'RemoteOptionsContainer.*prevValues'

Length of output: 759

}

if ("options" in prop && prop.options) {
Expand Down
154 changes: 109 additions & 45 deletions packages/connect-react/src/components/ControlSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { useMemo } from "react";
import {
useEffect,
useMemo, useState,
} from "react";
import Select, {
Props as ReactSelectProps, components,
} from "react-select";
Expand Down Expand Up @@ -28,6 +31,26 @@ export function ControlSelect<T>({
const {
select, theme,
} = useCustomize();
const [
selectOptions,
setSelectOptions,
] = useState(options);
const [
rawValue,
setRawValue,
] = useState(value);

useEffect(() => {
setSelectOptions(options)
}, [
options,
])

useEffect(() => {
setRawValue(value)
}, [
value,
])

const baseSelectProps: BaseReactSelectProps<never, never, never> = {
styles: {
Expand All @@ -40,7 +63,7 @@ export function ControlSelect<T>({
};

const selectValue = useMemo(() => {
let ret = value;
let ret = rawValue;
if (ret != null) {
if (Array.isArray(ret)) {
// if simple, make lv (XXX combine this with other place this happens)
Expand All @@ -51,7 +74,7 @@ export function ControlSelect<T>({
label: o,
value: o,
}
for (const item of options) {
for (const item of selectOptions) {
if (item.value === o) {
obj = item;
break;
Expand All @@ -62,23 +85,28 @@ export function ControlSelect<T>({
ret = lvs;
}
} else if (typeof ret !== "object") {
const lvOptions = options?.[0] && typeof options[0] === "object";
const lvOptions = selectOptions?.[0] && typeof selectOptions[0] === "object";
if (lvOptions) {
for (const item of options) {
if (item.value === value) {
for (const item of selectOptions) {
if (item.value === rawValue) {
ret = item;
break;
}
}
} else {
ret = {
label: rawValue,
value: rawValue,
}
}
} else if (ret.__lv) {
ret = ret.__lv
}
}
return ret;
}, [
value,
options,
rawValue,
selectOptions,
]);

const LoadMore = ({
Expand All @@ -105,58 +133,94 @@ export function ControlSelect<T>({
}

const handleCreate = (inputValue: string) => {
options.unshift({
label: inputValue,
value: inputValue,
})
const createOption = (input: unknown) => {
if (typeof input === "object") return input
return {
label: input,
value: input,
}
}
const newOption = createOption(inputValue)
let newRawValue = newOption
const newSelectOptions = selectOptions
? [
newOption,
...selectOptions,
]
: [
newOption,
]
setSelectOptions(newSelectOptions);
if (prop.type.endsWith("[]")) {
if (Array.isArray(rawValue)) {
newRawValue = [
...rawValue.map(createOption),
newOption,
]
} else {
newRawValue = [
newOption,
]
}
}
setRawValue(newRawValue)
handleChange(newRawValue)
};

const handleChange = (o: unknown) => {
if (o) {
if (Array.isArray(o)) {
if (typeof o[0] === "object" && "value" in o[0]) {
const vs = [];
for (const _o of o) {
if (prop.withLabel) {
vs.push(_o);
} else {
vs.push(_o.value);
}
}
onChange(vs);
} else {
onChange(o);
}
} else if (typeof o === "object" && "value" in o) {
if (prop.withLabel) {
onChange({
__lv: o,
});
} else {
onChange(o.value);
}
} else {
throw new Error("unhandled option type"); // TODO
}
} else {
onChange(undefined);
}
}
Comment on lines +170 to +200
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and type checking.

The error handling and type checking in handleChange could be improved:

  1. The error message is not descriptive enough
  2. The type checking logic could be simplified using TypeScript type guards
+ type SelectOption = { value: unknown; label: string };
+ 
+ function isSelectOption(o: unknown): o is SelectOption {
+   return typeof o === "object" && o !== null && "value" in o;
+ }
+ 
  const handleChange = (o: unknown) => {
    if (o) {
      if (Array.isArray(o)) {
-        if (typeof o[0] === "object" && "value" in o[0]) {
+        if (o.length > 0 && isSelectOption(o[0])) {
          const vs = [];
          for (const _o of o) {
            if (prop.withLabel) {
              vs.push(_o);
            } else {
              vs.push(_o.value);
            }
          }
          onChange(vs);
        } else {
          onChange(o);
        }
-      } else if (typeof o === "object" && "value" in o) {
+      } else if (isSelectOption(o)) {
        if (prop.withLabel) {
          onChange({
            __lv: o,
          });
        } else {
          onChange(o.value);
        }
      } else {
-        throw new Error("unhandled option type"); // TODO
+        throw new Error(`Unhandled option type: ${typeof o}. Expected an array or an object with 'value' property.`);
      }
    } else {
      onChange(undefined);
    }
  }
📝 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.

Suggested change
const handleChange = (o: unknown) => {
if (o) {
if (Array.isArray(o)) {
if (typeof o[0] === "object" && "value" in o[0]) {
const vs = [];
for (const _o of o) {
if (prop.withLabel) {
vs.push(_o);
} else {
vs.push(_o.value);
}
}
onChange(vs);
} else {
onChange(o);
}
} else if (typeof o === "object" && "value" in o) {
if (prop.withLabel) {
onChange({
__lv: o,
});
} else {
onChange(o.value);
}
} else {
throw new Error("unhandled option type"); // TODO
}
} else {
onChange(undefined);
}
}
type SelectOption = { value: unknown; label: string };
function isSelectOption(o: unknown): o is SelectOption {
return typeof o === "object" && o !== null && "value" in o;
}
const handleChange = (o: unknown) => {
if (o) {
if (Array.isArray(o)) {
if (o.length > 0 && isSelectOption(o[0])) {
const vs = [];
for (const _o of o) {
if (prop.withLabel) {
vs.push(_o);
} else {
vs.push(_o.value);
}
}
onChange(vs);
} else {
onChange(o);
}
} else if (isSelectOption(o)) {
if (prop.withLabel) {
onChange({
__lv: o,
});
} else {
onChange(o.value);
}
} else {
throw new Error(`Unhandled option type: ${typeof o}. Expected an array or an object with 'value' property.`);
}
} else {
onChange(undefined);
}
}


const additionalProps = {
onCreateOption: prop.remoteOptions
? handleCreate
: undefined,
}

const MaybeCreatableSelect = isCreatable
? CreatableSelect
: Select;
return (
<MaybeCreatableSelect
inputId={id}
instanceId={id}
options={options}
options={selectOptions}
value={selectValue}
isMulti={prop.type.endsWith("[]")}
isClearable={true}
required={!prop.optional}
{...props}
{...selectProps}
onCreateOption={handleCreate}
onChange={(o) => {
if (o) {
if (Array.isArray(o)) {
if (typeof o[0] === "object" && "value" in o[0]) {
const vs = [];
for (const _o of o) {
if (prop.withLabel) {
vs.push(_o);
} else {
vs.push(_o.value);
}
}
onChange(vs);
} else {
onChange(o);
}
} else if (typeof o === "object" && "value" in o) {
if (prop.withLabel) {
onChange({
__lv: o,
});
} else {
onChange(o.value);
}
} else {
throw new Error("unhandled option type"); // TODO
}
} else {
onChange(undefined);
}
}}
{...additionalProps}
onChange={handleChange}
/>
);
}
5 changes: 4 additions & 1 deletion packages/connect-react/src/components/RemoteOptionsContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@

export type RemoteOptionsContainerProps = {
queryEnabled?: boolean;
prevValues?: never;
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect type definition for prevValues.

The type never makes this prop unusable. If this prop is intended to store previous values, consider using a more appropriate type that matches the value structure of your form field.

-  prevValues?: never;
+  prevValues?: unknown;  // Replace with actual type based on your form field value structure

Committable suggestion skipped: line range outside the PR's diff.

};

export function RemoteOptionsContainer({ queryEnabled }: RemoteOptionsContainerProps) {
export function RemoteOptionsContainer({
queryEnabled, prevValues,

Check failure on line 15 in packages/connect-react/src/components/RemoteOptionsContainer.tsx

View workflow job for this annotation

GitHub Actions / Lint Code Base 

'prevValues' is defined but never used. Allowed unused args must match /^_/u
}: RemoteOptionsContainerProps) {
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Type definition contradicts component usage

The review comment is valid and reveals additional issues:

  1. prevValues is typed as never in props type but being passed a value in Control.tsx
  2. The parameter is unused in the implementation

Please address these inconsistencies by either:

  1. Implementing the prevValues logic and updating its type definition to match the expected value type, or
  2. Removing both the parameter and its usage in Control.tsx
🔗 Analysis chain

Address unused parameter and clarify implementation.

The prevValues parameter is added but never used in the implementation. Based on the PR title suggesting a fix for "remote options gets unset when loading configuredProps", this parameter seems important.

Please either:

  1. Implement the logic using prevValues to fix the unset remote options issue, or
  2. Remove the parameter if it's not needed.

Let's verify if this component is used with the prevValues prop:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for RemoteOptionsContainer usage with prevValues prop
ast-grep --pattern 'RemoteOptionsContainer $props={$_}'

Length of output: 57

Script:

#!/bin/bash
# Search for any usage of RemoteOptionsContainer
rg "RemoteOptionsContainer" -A 3 -B 3

# Also check the implementation to understand the component
cat packages/connect-react/src/components/RemoteOptionsContainer.tsx

Length of output: 8607

🧰 Tools
🪛 eslint

[error] 15-15: 'prevValues' is defined but never used. Allowed unused args must match /^_/u.

(@typescript-eslint/no-unused-vars)

🪛 GitHub Check: Lint Code Base

[failure] 15-15:
'prevValues' is defined but never used. Allowed unused args must match /^_/u

🪛 GitHub Actions: Pull Request Checks

[error] 15-15: 'prevValues' is defined but never used. Allowed unused args must match /^_/u

const client = useFrontendClient();
const {
userId,
Expand Down
3 changes: 1 addition & 2 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading