make custom fields case sensitive #1242
Conversation
jwetzell
commented
Oct 4, 2024
jwetzell
commented
- removes the lower casing of custom fields
- doesn't allow duplicate custom fields from CustomFieldForm
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (3)
apps/client/src/features/app-settings/panel/feature-settings-panel/custom-fields/CustomFieldForm.tsx (1)
23-24: LGTM: New hook used correctly. Consider adding a comment for clarity.The
useCustomFieldshook is correctly used at the component's top level. To improve code readability, consider adding a brief comment explaining the purpose of this data:// Fetch existing custom fields for uniqueness validation const { data } = useCustomFields();This addition would help other developers quickly understand the intent behind using this hook.
apps/server/src/utils/__tests__/parserFunctions.test.ts (2)
Line range hint
356-461: Good addition of event linking test cases, consider adding custom fields testThe new test cases for
parseRundown()linking are valuable additions that improve the overall test coverage. However, to align more closely with the PR objectives:Consider adding a test case that includes custom fields with varying case to ensure that their case sensitivity is preserved during rundown parsing. This would help validate that the changes to custom field handling are correctly reflected in the parsed rundown.
Line range hint
1-461: Overall, changes align well with PR objectives, with room for minor improvementsThe updates to the test cases for
sanitiseCustomFieldscorrectly reflect the new case-sensitive behavior of custom fields, which is the primary objective of this PR. The preservation of case in keys and labels is now properly tested, ensuring that the implementation maintains this behavior.The addition of new test cases for event linking in
parseRundown(), while not directly related to the custom fields objective, improves the overall test coverage and is a positive change.To further strengthen the alignment with the PR objectives, consider:
- Adding a test case in
parseRundown()that includes custom fields with varying case to ensure their case sensitivity is preserved during rundown parsing.- Reviewing other parts of the codebase where custom fields are used to ensure consistent case-sensitive handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/client/src/features/app-settings/panel/feature-settings-panel/custom-fields/CustomFieldForm.tsx (3 hunks)
- apps/server/src/utils/tests/parserFunctions.test.ts (3 hunks)
- apps/server/src/utils/parserFunctions.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
apps/client/src/features/app-settings/panel/feature-settings-panel/custom-fields/CustomFieldForm.tsx (2)
9-9: LGTM: New hook import added correctly.The
useCustomFieldshook has been imported properly, maintaining consistency with other imports in the file. This addition aligns with the PR objective of modifying custom fields handling.
Line range hint
1-108: Overall implementation is solid, with one minor adjustment needed.The changes successfully implement the PR objectives of making custom fields unique and introducing case sensitivity. The new
useCustomFieldshook is correctly imported and used, and the validation logic has been updated to check for uniqueness.However, there's one minor adjustment needed in the uniqueness check to ensure proper case sensitivity handling. Once this is addressed, the implementation will fully meet the PR objectives.
Great job on the implementation! The code is clean, well-structured, and achieves the intended goals with minimal changes.
apps/server/src/utils/parserFunctions.ts (2)
313-313: LGTM: Case-sensitive custom fields implemented.The change to use
field.labeldirectly as the key fornewCustomFieldssuccessfully implements case-sensitive custom fields, aligning with the PR objective.
Line range hint
313-336: Verify impact on case-sensitive changes.The switch to case-sensitive custom fields might affect other parts of the application that rely on case-insensitive lookups.
Please run the following script to identify potential areas that might be affected:
This script will help identify areas in the codebase where custom fields are accessed, iterated, or compared. Review these areas to ensure they handle the new case-sensitive behavior correctly.
✅ Verification successful
Impact on case-sensitive changes is verified and no issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of custom fields in the codebase # Test: Look for places where custom fields are accessed echo "Searching for custom field access patterns:" rg --type typescript --type javascript 'customFields\[.*\]' -g '!*test*' # Test: Look for places where custom fields are iterated echo "\nSearching for custom field iteration patterns:" rg --type typescript --type javascript 'Object\.keys\(.*customFields.*\)' -g '!*test*' # Test: Look for places where custom fields are compared echo "\nSearching for custom field comparison patterns:" rg --type typescript --type javascript '(===|!==|==|!=).*customFields\[.*\]' -g '!*test*'Length of output: 673
Script:
#!/bin/bash # Description: Search for usage of custom fields in the codebase with corrected file types # Test: Look for places where custom fields are accessed echo "Searching for custom field access patterns:" rg --type js --type jsx --type ts --type tsx 'customFields\[.*\]' -g '!*test*' # Test: Look for places where custom fields are iterated echo "\nSearching for custom field iteration patterns:" rg --type js --type jsx --type ts --type tsx 'Object\.keys\(.*customFields.*\)' -g '!*test*' # Test: Look for places where custom fields are compared echo "\nSearching for custom field comparison patterns:" rg --type js --type jsx --type ts --type tsx '(===|!==|==|!=).*customFields\[.*\]' -g '!*test*'Length of output: 670
Script:
#!/bin/bash # Description: Search for usage of custom fields in the codebase using file extension patterns # Test: Look for places where custom fields are accessed echo "Searching for custom field access patterns:" rg 'customFields\[.*\]' --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx' -g '!*test*' # Test: Look for places where custom fields are iterated echo "\nSearching for custom field iteration patterns:" rg 'Object\.keys\(.*customFields.*\)' --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx' -g '!*test*' # Test: Look for places where custom fields are compared echo "\nSearching for custom field comparison patterns:" rg '(===|!==|==|!=).*customFields\[.*\]' --glob '*.js' --glob '*.jsx' --glob '*.ts' --glob '*.tsx' -g '!*test*'Length of output: 2109
apps/server/src/utils/__tests__/parserFunctions.test.ts (3)
282-282: LGTM: Case sensitivity preserved for custom field keysThis change correctly tests the new case-sensitive behavior of custom fields, aligning with the PR objective.
334-334: LGTM: Case-sensitive key preservation in name cohesion testThis change correctly updates the expected behavior to preserve the case of the custom field label when used as a key, which is in line with the PR objectives.
345-345: LGTM: Case sensitivity preserved in invalid entries filtering testThese changes correctly update the test case to include and expect a capitalized custom field key, ensuring that the case sensitivity is maintained when filtering invalid entries.
Also applies to: 350-350
| if (Object.keys(data).includes(value)) return 'Custom fields must be unique'; | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
Approve validation addition, but case sensitivity needs adjustment.
The new validation rule correctly implements a uniqueness check for custom fields, which aligns with the PR objective. However, the current implementation using Object.keys(data).includes(value) might not be case-sensitive.
To ensure case sensitivity, consider modifying the check as follows:
if (Object.keys(data).some(key => key.toLowerCase() === value.toLowerCase())) {
return 'Custom fields must be unique (case-insensitive)';
}This change will make the uniqueness check case-insensitive, preventing fields like "Field" and "field" from coexisting, which seems to be the intended behavior based on the PR objectives.
| const key = field.label; | ||
| if (key in newCustomFields) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider case-insensitive uniqueness check.
While the current implementation allows for case-sensitive custom fields, it might lead to confusion if two fields differ only in case (e.g., "Name" and "name"). Consider implementing a case-insensitive uniqueness check to prevent this scenario.
Here's a suggested modification:
- const key = field.label;
- if (key in newCustomFields) {
+ const key = field.label;
+ if (Object.keys(newCustomFields).some(k => k.toLowerCase() === key.toLowerCase())) {
continue;
}This change ensures that custom fields are unique regardless of case while still preserving the original case in the stored data.
📝 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 key = field.label; | |
| if (key in newCustomFields) { | |
| continue; | |
| } | |
| const key = field.label; | |
| if (Object.keys(newCustomFields).some(k => k.toLowerCase() === key.toLowerCase())) { | |
| continue; | |
| } |
|
Thank you for this, the code looks good. |
...ent/src/features/app-settings/panel/feature-settings-panel/custom-fields/CustomFieldForm.tsx
Outdated
Show resolved
Hide resolved
alex-Arc
force-pushed
the
custom-fields-formatting
branch
from
e456883 to
983d89b
Compare
alex-Arc
force-pushed
the
custom-fields-formatting
branch
from
983d89b to
7e2e158
Compare
