feat: Reaction Tries #1431
feat: Reaction Tries #1431
Conversation
Added trie Used emoji trie to handle searches and not require debounce
WalkthroughThis pull request introduces comprehensive changes across multiple files, focusing on error handling, emoji selection, and type system improvements. The modifications primarily involve replacing traditional error logging with a more user-friendly toast notification system, updating emoji-related type definitions, and implementing a new Trie data structure for efficient emoji searching. The changes enhance error reporting, improve type safety, and optimize emoji selection functionality across the application. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant ErrorHandler
participant TranslationService
participant ToastNotification
User->>Component: Perform action
alt Action fails
Component->>ErrorHandler: Capture error
ErrorHandler->>TranslationService: Get localized error message
TranslationService-->>ErrorHandler: Return translated message
ErrorHandler->>ToastNotification: Display error toast
ToastNotification->>User: Show error message
end
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![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)
containers/GroupScreenAddition.tsx (1)
Typo found in translation key "group_opertation_an_error_occurred"
The translation key contains a spelling error: "opertation" should be "operation". While the key is consistently used across files and has a corresponding translation, it should be corrected for better maintainability.
Locations requiring update:
i18n/translations/en.ts: Translation definitioncontainers/GroupScreenAddition.tsx: Error message usagecontainers/GroupScreenName.tsx: Error message usagecontainers/GroupScreenDescription.tsx: Error message usage🔗 Analysis chain
Line range hint
1-238: Verify error message translationsEnsure that the translation key "group_opertation_an_error_occurred" is properly defined in all language files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the translation key exists in all language files fd -e json -e ts "translations" | xargs rg "group_opertation_an_error_occurred"Length of output: 464
🧹 Nitpick comments (9)
containers/GroupScreenDescription.tsx (1)
60-62: Consider setting editing state after error handlingWhile the error handling is good, you might want to consider keeping the editing state active when an error occurs, allowing users to retry without having to re-enter edit mode.
const handleDescriptionChange = useCallback(async () => { try { await setGroupDescription(editedDescription); + setEditing(false); } catch (e) { captureErrorWithToast(e, { message: translate("group_opertation_an_error_occurred"), }); } }, [editedDescription, setGroupDescription]);utils/trie.ts (2)
1-11: Consider including optional data initialization in TrieNode constructor
While the current constructor sets defaults correctly, providing an optional parameter for initializing node properties (e.g., isEndOfWord, value) can help in scenarios where advanced node initialization might be needed.
61-77: Clarify potential large result sets
Invoking findAllWithPrefix could return a large list if many entries share the same prefix. Consider pagination or a maximum result limit for better performance.features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx (3)
20-20: Potentially rename for clarity
“categorizedEmojis” is a local constant that might not reflect that it’s also chunked into slices of size 6. Consider a more descriptive name (e.g., “slicedCategorizedEmojis”).
Line range hint
32-41: Improve slicing into categories
The sliceEmojis function returns new category labels based solely on an emoji’s own string. You might want to preserve the original category name for context or user UI feedback.
73-91: Potential performance considerations for large sets
Using a Set and filtering on each keystroke can be resource-intensive if the emoji set is large. It looks fine for typical emoji sets now, but be mindful of performance if you expand usage.utils/emojis/emoji-types.ts (2)
4-4: Justify addition of “v” property
The added “v” field’s meaning is unclear. Consider adding a short doc comment clarifying its usage (e.g., “vendor variant,” “version,” etc.).
9-12: Potential mismatch in property naming
The new interface name “ICategorizedEmojisRecord” might conflict with typical TypeScript naming if other interfaces or records exist. Ensure that theIprefix is consistent with your overall naming conventions.features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-row.tsx (1)
Line range hint
12-16: Consider memoizing the slice length calculation.While the type update is correct, the
slice(0, 6)operation and array padding could be optimized by extracting the magic number 6 into a constant and potentially memoizing the length check.+const MAX_EMOJI_ROW_LENGTH = 6; + export const EmojiRow: FC<EmojiRowProps> = memo(({ item, onPress }) => { const items = useMemo(() => { - const sliced: (string | IEmoji)[] = item.emojis.slice(0, 6); - while (sliced.length < 6) { + const sliced: (string | IEmoji)[] = item.emojis.slice(0, MAX_EMOJI_ROW_LENGTH); + while (sliced.length < MAX_EMOJI_ROW_LENGTH) { sliced.push(""); } return sliced; }, [item.emojis]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
containers/GroupScreenAddition.tsx(2 hunks)containers/GroupScreenDescription.tsx(2 hunks)containers/GroupScreenName.tsx(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx(3 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-row.tsx(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx(4 hunks)i18n/translations/en.ts(1 hunks)package.json(0 hunks)utils/emojis/emoji-trie.ts(1 hunks)utils/emojis/emoji-types.ts(1 hunks)utils/trie.ts(1 hunks)
💤 Files with no reviewable changes (1)
- package.json
🔇 Additional comments (13)
containers/GroupScreenName.tsx (2)
2-3: LGTM: Clean import additions for error handling
The new imports support the standardized error handling approach using toast notifications and translations.
54-56: LGTM: Improved error handling with user feedback
The error handling now provides better user feedback through toast notifications with localized messages.
containers/GroupScreenDescription.tsx (1)
1-1: LGTM: Clean import addition
The new import supports the standardized error handling approach.
containers/GroupScreenAddition.tsx (1)
44-44: LGTM: Clean import addition
The new import supports the standardized error handling approach.
utils/trie.ts (1)
93-124: Edge case with special characters
When deleting words that have characters outside typical ASCII (e.g., emojis or diacritics), ensure that both insertion and deletion use the same indexing method. Any mismatch in string character iteration across codepoints could lead to unhandled edge cases.
✅ Verification successful
Let me analyze the code further by checking how strings are handled in the Trie implementation.
Let me try a different approach to find the Trie implementation.
Let me check the complete implementation of insert and delete methods to verify string handling.
Based on the implementation, I can now provide the final response:
String handling in Trie is safe for special characters
The Trie implementation uses JavaScript's native string iteration with for...of loops which correctly handles UTF-16 surrogate pairs in emojis and other special characters. The insert method iterates characters with for (const char of word) and the delete method uses direct string indexing but only after validating the index against the string length, making it safe for all Unicode characters including emojis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for special chars usage that may indicate multi-byte indexing
rg '[^\x00-\x7F]'
Length of output: 66169
Script:
#!/bin/bash
# Search for the Trie implementation to see string handling
ast-grep --pattern 'class Trie {
$$$
}'
Length of output: 43
Script:
#!/bin/bash
# Search for the Trie class and its methods
rg "class.*Trie" -A 10
Length of output: 765
Script:
#!/bin/bash
# Get the full implementation of insert and delete methods
rg "insert|delete.*word.*" utils/trie.ts -A 20 -B 5
Length of output: 1248
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx (1)
3-3: Ensure correct import paths
Confirm that “@/utils/emojis/emoji-trie” is an alias path recognized by your bundler or TS config. If incorrectly configured, it may fail in certain build or test environments.
✅ Verification successful
Let me gather more information about the path alias configuration.
Import path is correctly configured and file exists
The @/utils/emojis/emoji-trie import is valid as:
- The tsconfig.json has
@/*alias mapping to./* - The file exists at
utils/emojis/emoji-trie.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking for relevant TS config that sets up path alias for `@/utils/emojis`
fd tsconfig.* --exec cat {} | rg 'paths'
Length of output: 60
Script:
#!/bin/bash
# Get the complete paths configuration from tsconfig
fd tsconfig.json --exec cat {} | awk '/paths/,/}/' | grep -v '^[[:space:]]*$'
# Also check if the target file exists
fd emoji-trie.ts -t f
Length of output: 884
utils/emojis/emoji-trie.ts (1)
5-5: Emoji Trie initialization
Initialization is straightforward. If you plan to dynamically update or remove emojis, ensure you handle re-insertion or deletion carefully.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-row.tsx (2)
1-1: LGTM! Type imports follow TypeScript naming conventions.
The change to use ICategorizedEmojisRecord and IEmoji interfaces follows standard TypeScript naming conventions and maintains consistency across the codebase.
6-6: LGTM! Type definition properly updated.
The EmojiRowProps interface correctly uses the new ICategorizedEmojisRecord type, maintaining type safety and consistency.
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker-list.tsx (4)
5-5: LGTM! Import statement properly updated.
The import of ICategorizedEmojisRecord is consistent with the type system changes across the codebase.
23-23: LGTM! Props type properly updated.
The EmojiRowListProps interface correctly uses the new ICategorizedEmojisRecord type for the emojis array.
Line range hint 1-1: Verify the trie data structure implementation.
While these files show proper type system updates, they don't contain the actual trie implementation mentioned in the PR objectives. Let's verify the trie implementation and its integration.
✅ Verification successful
Trie implementation is properly implemented and integrated
The trie data structure is well-implemented in utils/trie.ts with a generic implementation that supports:
- Efficient prefix-based search with
findAllWithPrefix - Full word search with
search - Insertion and deletion operations
- Type safety through generics
The implementation is correctly integrated into the emoji picker through:
utils/emojis/emoji-trie.tswhich builds the trie with emoji keywordsconversation-message-context-menu-emoji-picker.tsxwhich uses the trie for efficient emoji search
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for trie implementation and its usage
echo "Searching for trie implementation..."
rg -l "class.*Trie|function.*Trie" --type typescript
echo "Searching for trie usage..."
rg -l "emojiTrie" --type typescript
echo "Searching for emoji search/filter related functions..."
rg -l "filterEmoji|searchEmoji" --type typescript
Length of output: 514
Script:
#!/bin/bash
# Search for trie implementation and its usage without file type restriction
echo "Searching for trie implementation..."
rg -l "class.*Trie|function.*Trie"
echo "Searching for trie usage..."
rg -l "emojiTrie"
echo "Searching for emoji search/filter related functions..."
rg -l "filterEmoji|searchEmoji"
# Search for files that might contain emoji-related functionality
echo "Searching for emoji-related files..."
fd -e ts -e tsx emoji
Length of output: 1502
Script:
#!/bin/bash
# Check the trie implementation
echo "Checking trie implementation..."
cat utils/trie.ts
echo "Checking emoji trie usage..."
cat utils/emojis/emoji-trie.ts
# Check how the trie is used in the emoji picker
echo "Checking emoji picker implementation..."
cat features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-emoji-picker/conversation-message-context-menu-emoji-picker.tsx
Length of output: 8586
54-57: Consider simplifying the renderItem type and verifying the dual renderer approach.
While the type update is correct, consider:
- The intersection type could be simplified by creating a unified type
- The dual renderer approach (FlashList vs FlatList) should be verified for necessity
Consider simplifying the type:
type EmojiListRenderItem = ListRenderItem<ICategorizedEmojisRecord> & FlashListRenderItem<ICategorizedEmojisRecord>;
const renderItem: EmojiListRenderItem = useCallback(
({ item }) => <EmojiRow onPress={onPress} item={item} />,
[onPress]
);✅ Verification successful
Let me gather more information about the FlashList vs FlatList implementation to better understand the dual renderer approach.
The dual renderer approach is intentional and necessary for platform-specific performance
The code shows a deliberate platform-specific implementation with a comment explaining the reason: "Works around issue with Android not picking up scrolls". The dual type annotation is necessary because:
- The component conditionally uses different list implementations based on the platform
- Both list types are used in the same component through the
ListRendererconstant - The intersection type ensures type safety across both implementations
The type complexity is a valid tradeoff for maintaining platform-specific optimizations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for performance differences between FlatList and FlashList implementations
# Look for comments or issues discussing the renderer choice
# Search for discussions about FlashList vs FlatList performance
rg -i "flashlist.*flatlist|flatlist.*flashlist" -g "!*.{lock,json}"
# Search for performance-related comments
rg -i "performance.*list|list.*performance" -g "!*.{lock,json}"
Length of output: 12267
Script:
#!/bin/bash
# Search for the file containing the list implementation
fd "conversation-message-context-menu-emoji-picker-list.tsx" --type f --exec cat {}
# Look for any performance-related comments or configuration in the component
ast-grep --pattern 'const $_ = Platform.OS === "ios" ? BottomSheetFlashList : BottomSheetFlatList'
Length of output: 3379
| captureErrorWithToast(err, { | ||
| message: translate("group_opertation_an_error_occurred"), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider standardizing error handling across all operations
While the create operation now uses the new error handling pattern, the delete operation (onDeleteInviteLink) still doesn't handle errors. Consider standardizing error handling across all operations.
const onDeleteInviteLink = useCallback(() => {
Haptics.impactAsync();
const groupId = getV3IdFromTopic(topic);
const inviteId = getInviteIdByGroupId(groupId);
if (!inviteId) {
return;
}
- deleteGroupInvite(currentAccount, inviteId).then(() => {
+ deleteGroupInvite(currentAccount, inviteId)
+ .then(() => {
setSnackMessage(translate("group_invite_link_deleted"));
deleteLinkFromState(topic);
deleteLinkFromStore(inviteId);
deleteInviteIdByGroupId(groupId);
+ })
+ .catch((err) => {
+ captureErrorWithToast(err, {
+ message: translate("group_opertation_an_error_occurred"),
+ });
});
}, [deleteLinkFromState, topic, currentAccount]);Also consider adding haptic feedback for consistency across all operations:
const onCreateInviteLinkPress = useCallback(() => {
+ Haptics.impactAsync();
createGroupInvite(currentAccount, {
groupName: groupName ?? translate("group_invite_default_group_name"),Committable suggestion skipped: line range outside the PR's diff.
| @@ -329,6 +329,7 @@ export const en = { | |||
| approve: "Approve", | |||
| deny: "Deny", | |||
| approve_member_to_this_group: "Approve {{name}} to this group", | |||
| group_opertation_an_error_occurred: "An error occurred", | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in translation key and update its references
The translation key with the typo "opertation" is used in multiple files:
i18n/translations/en.tscontainers/GroupScreenDescription.tsxcontainers/GroupScreenAddition.tsxcontainers/GroupScreenName.tsx
Apply this diff to fix the typo and improve the error message:
- group_opertation_an_error_occurred: "An error occurred",
+ group_operation_an_error_occurred: "An error occurred while performing this group operation",Then update all references in the following files:
containers/GroupScreenDescription.tsxcontainers/GroupScreenAddition.tsxcontainers/GroupScreenName.tsx
🔗 Analysis chain
Fix typo in translation key and improve error message specificity
The translation key and message have the following issues:
- There's a typo in the key name: "opertation" should be "operation"
- The error message is too generic and doesn't provide context about what went wrong
Apply this diff to fix the typo and improve the error message:
- group_opertation_an_error_occurred: "An error occurred",
+ group_operation_an_error_occurred: "An error occurred while performing this group operation",Note: After fixing the key name, ensure to update all references to this translation key in the codebase.
Let's verify the usage of this translation key:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to the translation key with typo
rg "group_opertation_an_error_occurred"
Length of output: 426
| search(rawWord: string): T | null { | ||
| const word = rawWord.toLowerCase().trim(); | ||
| let current = this.root; | ||
|
|
||
| for (const char of word) { | ||
| if (!current.children.has(char)) { | ||
| return null; | ||
| } | ||
| current = current.children.get(char)!; | ||
| } | ||
|
|
||
| return current.isEndOfWord ? current.value : null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent normalization approach
The search method normalizes input to lowercase/trim. Ensure that all inserts also follow the same or complementary normalization so that searching for "Apple" vs. "apple" yields consistent results.
| insert(word: string, value: T): void { | ||
| let current = this.root; | ||
|
|
||
| for (const char of word) { | ||
| if (!current.children.has(char)) { | ||
| current.children.set(char, new TrieNode<T>()); | ||
| } | ||
| current = current.children.get(char)!; | ||
| } | ||
|
|
||
| current.isEndOfWord = true; | ||
| current.value = value; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Normalization mismatch between insert and search methods needs attention
The code review is valid. Looking at the implementation:
- The
searchmethod normalizes input by usingtoLowerCase()andtrim() - The
findAllWithPrefixmethod also normalizes similarly - However, the
insertmethod doesn't normalize the input, which could lead to mismatches - The emoji-trie usage shows direct insertion of keywords without normalization
This inconsistency could cause issues where:
- A search for "SMILE" or " smile " would normalize to "smile"
- But if the word was inserted as "SMILE", it won't be found because it's stored differently in the trie
Consider normalizing the input in the insert method to match the search behavior:
insert(word: string, value: T): void {
const normalizedWord = word.toLowerCase().trim();
let current = this.root;
// ... rest of the method
}🔗 Analysis chain
Review the casing or normalization strategy for “word”
Currently, the insert method does not normalize the word to lowercase or trim spaces. If you rely on consistent searches (see the search method’s normalization), consider normalizing in insert as well for consistency. Otherwise, stored words might mismatch the search behavior.
Let me check the search method's normalization strategy before making a final decision.
Let me try a different approach to find the search method.
Let me check how the trie is being used in the emoji context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching all insert calls to see if there's any external code normalizing them.
rg -A 3 '(\.insert\()'
Length of output: 190
Script:
#!/bin/bash
# Let's look at the search method implementation and any normalization
ast-grep --pattern 'search($_) {
$$$
}'
Length of output: 43
Script:
#!/bin/bash
# Let's look at the entire trie implementation to see both insert and search methods
cat utils/trie.ts
Length of output: 2766
Script:
#!/bin/bash
# Let's see how the emoji trie is implemented and used
cat utils/emojis/emoji-trie.ts
Length of output: 364
| emojis.forEach((emojiSections) => { | ||
| emojiSections.data.forEach((emoji) => { | ||
| emoji.keywords.forEach((keyword) => { | ||
| emojiTrie.insert(keyword, emoji); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Keywords need preprocessing for consistency
The Trie's search method applies toLowerCase() and trim() to the input word, but the insert method doesn't preprocess keywords. This inconsistency could lead to search mismatches. Keywords should be preprocessed before insertion using the same transformations.
🔗 Analysis chain
Verify consistent keyword usage
Each keyword is inserted as-is. Confirm whether keywords are trimmed or lower-cased at the source, matching the search approach in the Trie (which lowercases the rawWord).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking if keywords are consistent in the emojis data
rg '"keywords": \['
Length of output: 21
Script:
#!/bin/bash
# Let's check the emoji data structure and keyword handling
# First, find the emoji data file
fd emoji.*json
# Let's also check the Trie implementation for search handling
ast-grep --pattern 'class Trie {
$$$
search($_) {
$$$
}
$$$
}'
# And check how keywords are processed before insertion
rg -A 5 'keywords.*forEach'
Length of output: 360
Script:
#!/bin/bash
# Let's check the Trie's insert and search implementation
rg -A 10 'insert\(' utils/emojis/
rg -A 10 'search\(' utils/emojis/
# Also check if there's any preprocessing of keywords before insertion
rg -B 5 -A 5 'keywords' utils/emojis/
Length of output: 66472
Script:
#!/bin/bash
# Let's check the Trie implementation to see how search handles the keywords
ast-grep --pattern 'class Trie {
$$$
search($word) {
$$$
}
$$$
}'
# Also check if there's any preprocessing before insertion
ast-grep --pattern 'class Trie {
$$$
insert($word, $_) {
$$$
}
$$$
}'
Length of output: 163
Script:
#!/bin/bash
# Let's find and examine the Trie implementation file first
fd -e ts -e js trie
# Then look for search/insert method implementations
rg -A 10 'search\(' utils/
rg -A 10 'insert\(' utils/
Length of output: 1155
Added trie
Used emoji trie to handle searches and not require debounce
Simulator.Screen.Recording.-.iPhone.15.-.2024-12-22.at.08.49.03.mp4
Summary by CodeRabbit
New Features
Bug Fixes
match-sorterlibrary, streamlining emoji filtering.Documentation
Chores