fix: validation for text key #14669
fix: validation for text key #14669
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
area/text-editor
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14669 +/- ##
==========================================
- Coverage 95.76% 95.76% -0.01%
==========================================
Files 1914 1914
Lines 24949 24947 -2
Branches 2857 2856 -1
==========================================
- Hits 23892 23890 -2
Misses 798 798
Partials 259 259 ☔ View full report in Codecov by Sentry. |
![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)
frontend/packages/text-editor/src/TextList.tsx (1)
34-37: LGTM! The implementation correctly fixes the case sensitivity issue.The function now properly handles text key validation with different casings, which directly addresses the reported issue. A few suggestions for improvement:
- Add JSDoc documentation to clarify the function's purpose and parameters.
- Consider a more efficient single-pass implementation.
Here's a suggested improvement with documentation and optimized implementation:
+/** + * Checks if a new text ID already exists in the list, excluding the old text ID. + * The comparison is case-insensitive. + * @param newTextId - The new text ID to check + * @param oldTextId - The current text ID to exclude from the check + * @returns true if the new text ID exists (case-insensitive), false otherwise + */ -const idExists = (newTextId: string, oldTextId: string): boolean => - textIds - .filter((textId: string) => textId.toLowerCase() !== oldTextId.toLowerCase()) - .some((textId: string) => StringUtils.areCaseInsensitiveEqual(textId, newTextId)); +const idExists = (newTextId: string, oldTextId: string): boolean => { + const oldTextLower = oldTextId.toLowerCase(); + return textIds.some((textId: string) => + textId.toLowerCase() !== oldTextLower && + StringUtils.areCaseInsensitiveEqual(textId, newTextId) + ); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/packages/text-editor/src/TextList.tsx(1 hunks)frontend/packages/text-editor/src/TextRow.test.tsx(1 hunks)frontend/packages/text-editor/src/TextRow.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/text-editor/src/TextRow.test.tsx
- frontend/packages/text-editor/src/TextRow.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
🔇 Additional comments (1)
frontend/packages/text-editor/src/TextList.tsx (1)
33-33: LGTM! Good use of memoization.The memoization of text keys is correctly implemented and will prevent unnecessary recalculations when other props change.
Description
User won't receive an error anymore when updating the text key with uppercase
For some reason code changes show that I removed this, but it doesn't exist in main neither.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Bug Fixes
Tests