fix: custom fields subscription #1329
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to several components, enhancing the handling of multi-option parameters and improving the user interface. Key changes include the addition of a new utility function for processing parameter values, updates to how parameters are retrieved from search parameters as arrays instead of strings, and the inclusion of an icon to improve the dropdown menu's visual representation. Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/client/src/common/components/view-params-editor/ParamInput.tsxOops! Something went wrong! :( ESLint: 8.56.0 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "/apps/client".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in "apps/client/.eslintrc". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/client/src/common/components/view-params-editor/ParamInput.tsx (1)
120-120: Consider using Chakra UI's built-in icon handlingWhile the chevron addition improves UX, consider these enhancements:
-{paramField.title} <IoChevronDown style={{ display: 'inline' }} /> +{paramField.title} <IoChevronDown aria-label="toggle menu" display="inline" />apps/client/src/features/operator/Operator.tsx (1)
117-118: LGTM! The changes effectively resolve the subscription parsing issues.The switch to
searchParams.getAll()elegantly handles both uppercase letters and spaces in custom fields by avoiding string manipulation.Consider these improvements for better clarity and type safety:
- // subscriptions is a MultiSelect and may have multiple values - const subscriptions = searchParams.getAll('subscribe').filter((value) => Object.hasOwn(customFields, value)); + // Get selected custom field IDs from the URL parameters + const selectedCustomFieldIds = searchParams.getAll('subscribe').filter((fieldId: string): fieldId is keyof typeof customFields => + Object.hasOwn(customFields, fieldId) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/client/src/common/components/view-params-editor/ParamInput.tsx(2 hunks)apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx(1 hunks)apps/client/src/features/operator/Operator.tsx(1 hunks)
🔇 Additional comments (5)
apps/client/src/common/components/view-params-editor/ParamInput.tsx (3)
16-16: LGTM: Icon import for enhanced UI
The addition of the chevron icon aligns with the PR objective to improve the visual indication of the combobox functionality.
125-126: LGTM: Robust value handling
The improved onChange handler correctly manages both single and multiple selections while maintaining type safety. The direct use of paramState ensures consistent value handling.
112-113: LGTM: Improved parameter handling
The switch to searchParams.getAll() resolves the issues with uppercase letters and spaces in custom fields by preserving the original values without case manipulation.
Let's verify the fix handles problematic cases:
✅ Verification successful
Parameter handling change is safe and correctly implemented
The verification confirms that:
- The switch from
searchParams.get()tosearchParams.getAll()inParamInput.tsxis appropriate for handling multiple values - No case transformations (
.toLowerCase()) are used in URL parameter handling in the view components - String splits that could affect space handling are not used for URL parameters in the view components
- Other parameter handling in viewer components uses
searchParams.get()appropriately for single-value parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining lowercase transformations in parameter handling
rg -i "\.toLowerCase\(\)" --type tsx --type ts
# Search for potential string splits that might affect space handling
rg "\.split\(['\"]" --type tsx --type ts
Length of output: 149
Script:
#!/bin/bash
# Search for any remaining lowercase transformations in parameter handling
rg -i "\.toLowerCase\(\)"
# Search for potential string splits that might affect space handling
rg "\.split\(['\"]"
# Look for other parameter handling patterns
ast-grep --pattern 'searchParams.get($$$)'
Length of output: 11677
apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx (2)
58-58: LGTM! Good refactoring of value handling logic.
The extraction of value handling logic into a separate function improves code organization and maintainability.
63-79: Verify case sensitivity requirements across the application.
Before implementing the suggested case normalization, we should verify that no other components depend on case-sensitive values.
| /** Utility function contains logic to add a value into the searchParams object */ | ||
| function handleValueString(id: string, value: string) { | ||
| const maybeMultipleValues = value.split(','); | ||
|
|
||
| // we need to check if the value contains comma separated list, for the case of the multi-select data | ||
| if (Array.isArray(maybeMultipleValues) && maybeMultipleValues.length > 1) { | ||
| const added = new Set(); | ||
| maybeMultipleValues.forEach((v) => { | ||
| if (!added.has(v)) { | ||
| added.add(v); | ||
| newSearchParams.append(id, v); | ||
| } | ||
| }); | ||
| } else { | ||
| newSearchParams.set(id, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add value sanitization to fully address the PR objectives.
The new utility function handles multiple values well, but it's missing some critical sanitization steps mentioned in the PR objectives:
- Handling of uppercase letters in custom fields
- Proper handling of spaces in multiple fields
Consider applying this improvement:
function handleValueString(id: string, value: string) {
- const maybeMultipleValues = value.split(',');
+ const maybeMultipleValues = value.split(',').map(v => v.trim().toLowerCase());
if (Array.isArray(maybeMultipleValues) && maybeMultipleValues.length > 1) {
const added = new Set();
maybeMultipleValues.forEach((v) => {
- if (!added.has(v)) {
+ if (v && !added.has(v)) { // Skip empty values
added.add(v);
newSearchParams.append(id, v);
}
});
} else {
- newSearchParams.set(id, value);
+ const sanitizedValue = value.trim().toLowerCase();
+ if (sanitizedValue) {
+ newSearchParams.set(id, sanitizedValue);
+ }
}
}These changes will:
- Trim whitespace from values
- Convert values to lowercase for consistency
- Skip empty values after splitting
📝 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.
| /** Utility function contains logic to add a value into the searchParams object */ | |
| function handleValueString(id: string, value: string) { | |
| const maybeMultipleValues = value.split(','); | |
| // we need to check if the value contains comma separated list, for the case of the multi-select data | |
| if (Array.isArray(maybeMultipleValues) && maybeMultipleValues.length > 1) { | |
| const added = new Set(); | |
| maybeMultipleValues.forEach((v) => { | |
| if (!added.has(v)) { | |
| added.add(v); | |
| newSearchParams.append(id, v); | |
| } | |
| }); | |
| } else { | |
| newSearchParams.set(id, value); | |
| } | |
| } | |
| /** Utility function contains logic to add a value into the searchParams object */ | |
| function handleValueString(id: string, value: string) { | |
| const maybeMultipleValues = value.split(',').map(v => v.trim().toLowerCase()); | |
| // we need to check if the value contains comma separated list, for the case of the multi-select data | |
| if (Array.isArray(maybeMultipleValues) && maybeMultipleValues.length > 1) { | |
| const added = new Set(); | |
| maybeMultipleValues.forEach((v) => { | |
| if (v && !added.has(v)) { // Skip empty values | |
| added.add(v); | |
| newSearchParams.append(id, v); | |
| } | |
| }); | |
| } else { | |
| const sanitizedValue = value.trim().toLowerCase(); | |
| if (sanitizedValue) { | |
| newSearchParams.set(id, sanitizedValue); | |
| } | |
| } | |
| } |
A few issues here