-
Notifications
You must be signed in to change notification settings - Fork 52
Fix: Case-insensitive Cyrillic search & nickname lookup logic (#140) #1764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
ts/state/ducks/search.ts
Outdated
| const queryLower = queryRaw.toLocaleLowerCase(); | ||
|
|
||
| // Fetch all conversations to filter in memory using JS. | ||
| const allConversations = await Data.getAllConversations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be way too slow and expensive for a large database.
This has to happen on the database itself, through searchConversations.
Can you make it work that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bilb Thanks for the feedback regarding performance! You were absolutely right.
I've discarded the in-memory filtering logic.
I refactored the solution to keep the logic strictly within the database.
Ready for re-review!
| db.function('NOCASE_JS', { deterministic: true }, (text: unknown) => { | ||
| if (typeof text !== 'string') { | ||
| return ''; | ||
| } | ||
| return text.toLowerCase(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the issue now. I thought sqlite3 could handle those, but it seems we don't have the plugin for it (ICU).
In this case, I think your memory approach was actually better but with a few tweaks.
Don't do a Data.getAllConversations in a36100a#diff-955956e9166338a3c122082f7848584c67e47ed13be9a8faec27f08c4d9684c3L84-L105 as this is quite slow on a large db.
Those conversations are already cached. You should be able to do your search on them directly with something like
function convoMatchesSearch(convo, searchTerm) {
const searchTermLower = searchTerm.toLowerCase();
return (
convo.id.toLowerCase().includes(searchTermLower) ||
convo.nickname?.toLowerCase().includes(searchTermLower) ||
convo.displayNameInProfile?.toLowerCase().includes(searchTermLower)
);
}That you can use with
for (const id in window.inboxStore.getState().conversations.conversationLookup) {
const convo = window.inboxStore.getState().conversations.conversationLookup[id];
if (convoMatchesSearch(convo, searchTerm)) {
results.push(convo);
}
}This should be enough.
Once you've updated your PR, I'll give it a go on large DB to make sure the performance are fine
First time contributor checklist:
Contributor checklist:
devbranchyarn readyrun passes successfully (more about tests here)Describe briefly what your pull request changes. Focus on the value provided to users.
Fixes #140 issue
Previously, the search relied on SQL
LIKEqueries, which are often case-sensitive for non-ASCII characters (e.g., Russian). Searching for "борис" would not find "БОРИС".Fix: Switched contact filtering to in-memory JavaScript using
.toLocaleLowerCase(), which handles Unicode case-folding correctly.Test approach: