Biz/dj 2825 remote options gets unset when loading configuredprops #15401
Conversation
…-loading-configuredprops
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
Line range hint
127-146: Consider preserving options during refetch.The current implementation might lose options when configuredProps change, as the pageable state is reset with each query. Consider:
- Using the new
prevValuesparameter to preserve options during refetch- Implementing a merge strategy for existing and new options
Here's a suggested approach:
const newOptions = [] const allValues = new Set(pageable.values) +// Preserve previous values if available +if (prevValues && Array.isArray(prevValues)) { + for (const value of prevValues) { + allValues.add(typeof value === 'object' ? value.value : value) + } +} for (const o of _options || []) { const value = typeof o === "string" ? o : o.value if (allValues.has(value)) { continue } allValues.add(value) newOptions.push(o) } let data = pageable.data if (newOptions.length) { data = [ + ...(prevValues || []), // Include previous values ...pageable.data, ...newOptions, ]🧰 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
🧹 Nitpick comments (3)
packages/connect-react/src/components/ControlSelect.tsx (3)
34-53: Consider memoizing state updates to prevent unnecessary re-renders.The state management changes help preserve remote options, but the current implementation might cause unnecessary re-renders when
optionsorvaluereference equality changes but content remains the same.Consider using deep comparison or memoization:
useEffect(() => { + if (JSON.stringify(options) !== JSON.stringify(selectOptions)) { setSelectOptions(options) + } }, [options, selectOptions]) useEffect(() => { + if (JSON.stringify(value) !== JSON.stringify(rawValue)) { setRawValue(value) + } }, [value, rawValue])
Line range hint
77-83: Add early return for empty selectOptions.The loop through
selectOptionscould be simplified and made safer.- for (const item of selectOptions) { + const matchingOption = selectOptions?.find(item => item.value === o); + if (matchingOption) { + obj = matchingOption; + } - if (item.value === o) { - obj = item; - break; - } - }
202-223: Add proper TypeScript types for remote options prop.The component would benefit from explicit TypeScript types for the remote options functionality.
type ControlSelectProps<T> = { isCreatable?: boolean; options: {label: string; value: T;}[]; selectProps?: ReactSelectProps; showLoadMoreButton?: boolean; onLoadMore?: () => void; + prop: { + remoteOptions?: boolean; + type: string; + withLabel?: boolean; + optional?: boolean; + }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/connect-react/examples/nextjs/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/Control.tsx(1 hunks)packages/connect-react/src/components/ControlSelect.tsx(6 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/connect-react/package.json
🧰 Additional context used
🪛 GitHub Actions: Pull Request Checks
packages/connect-react/CHANGELOG.md
[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to disable file ignore settings or use "--no-warn-ignored" to suppress this warning
packages/connect-react/src/components/RemoteOptionsContainer.tsx
[error] 15-15: 'prevValues' is defined but never used. Allowed unused args must match /^_/u
🪛 eslint
packages/connect-react/src/components/RemoteOptionsContainer.tsx
[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
packages/connect-react/src/components/RemoteOptionsContainer.tsx
[failure] 15-15:
'prevValues' is defined but never used. Allowed unused args must match /^_/u
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pnpm publish
🔇 Additional comments (4)
packages/connect-react/CHANGELOG.md (2)
4-5: LGTM! Version and date are properly formatted.The new version entry follows the established format and maintains chronological order.
🧰 Tools
🪛 GitHub Actions: Pull Request Checks
[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to disable file ignore settings or use "--no-warn-ignored" to suppress this warning
6-8: LGTM! Bug fixes are well-documented.The changelog entries clearly describe three distinct fixes:
- String array property merging issue
- Custom string input for remote options
- Remote options reloading during form re-render (addresses the PR title issue)
🧰 Tools
🪛 GitHub Actions: Pull Request Checks
[warning] File ignored because of a matching ignore pattern. Use "--no-ignore" to disable file ignore settings or use "--no-warn-ignored" to suppress this warning
packages/connect-react/src/components/Control.tsx (1)
27-27: LGTM! Clean addition of the value prop.The addition of
valueto the destructured properties is well-typed and necessary for supporting the remote options state management.packages/connect-react/src/components/ControlSelect.tsx (1)
Line range hint
1-223: Overall implementation looks good with suggested improvements.The changes effectively address the issue of remote options getting unset by maintaining separate state for options and values. The implementation could be enhanced with the suggested improvements for type safety, performance, and error handling, but the core functionality appears solid.
|
|
||
| if (prop.remoteOptions || prop.type === "$.discord.channel") { | ||
| return <RemoteOptionsContainer queryEnabled={queryDisabledIdx == null || queryDisabledIdx >= idx} />; | ||
| return <RemoteOptionsContainer prevValues={value} queryEnabled={queryDisabledIdx == null || queryDisabledIdx >= idx} />; |
There was a problem hiding this comment.
💡 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
prevValuesprop from the Control component's usage, or - Update
RemoteOptionsContainerPropsto 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:
- The
RemoteOptionsContainercomponent properly handles undefined values - The type of
valuematches whatRemoteOptionsContainerexpects forprevValues
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 (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); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type checking.
The error handling and type checking in handleChange could be improved:
- The error message is not descriptive enough
- 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.
| 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); | |
| } | |
| } |
|
|
||
| export type RemoteOptionsContainerProps = { | ||
| queryEnabled?: boolean; | ||
| prevValues?: never; |
There was a problem hiding this comment.
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 structureCommittable suggestion skipped: line range outside the PR's diff.
| export function RemoteOptionsContainer({ | ||
| queryEnabled, prevValues, | ||
| }: RemoteOptionsContainerProps) { |
There was a problem hiding this comment.
💡 Codebase verification
Type definition contradicts component usage
The review comment is valid and reveals additional issues:
prevValuesis typed asneverin props type but being passed a value inControl.tsx- The parameter is unused in the implementation
Please address these inconsistencies by either:
- Implementing the
prevValueslogic and updating its type definition to match the expected value type, or - 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:
- Implement the logic using
prevValuesto fix the unset remote options issue, or - 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
bzwrk
deleted the
biz/dj-2825-remote-options-gets-unset-when-loading-configuredprops
branch
[1.0.0-preview.24] - 2025-01-24
Summary by CodeRabbit
Bug Fixes
Version Update
1.0.0-preview.24