-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix contact autocomplete when no legacy accounts are loaded #2824
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,7 +69,7 @@ export default class ParticipantsTextField extends React.Component<ParticipantsT | |
| const CustomComponent = p.customComponent; | ||
| if (CustomComponent) return <CustomComponent token={p} />; | ||
| if (p instanceof Contact) { | ||
| return <Menu.NameEmailContent name={p.fullName()} email={p.email} key={p.id} />; | ||
| return <Menu.NameEmailContent name={p.fullName()} email={p.email} key={p.id || p.email} />; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you say what this fixes? I'd guess contacts synthesized from thread participants sometimes lack an Generated by Claude Code |
||
| } else if (p instanceof ContactGroup) { | ||
| return p.name; | ||
| } | ||
|
|
@@ -234,7 +234,7 @@ export default class ParticipantsTextField extends React.Component<ParticipantsT | |
| ContactStore.searchContactGroups(input), | ||
| ContactStore.searchContacts(input), | ||
| ]) | ||
| ).flat() | ||
| ).flat() as Contact[] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cast isn't accurate — the array really does contain If Generated by Claude Code |
||
| } | ||
| shouldBreakOnKeydown={this._shouldBreakOnKeydown} | ||
| onInputTrySubmit={this._onInputTrySubmit} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,44 @@ import DatabaseStore from './database-store'; | |
| import { AccountStore } from './account-store'; | ||
| import ComponentRegistry from '../../registries/component-registry'; | ||
| import { ContactGroup } from 'mailspring-exports'; | ||
| import { Thread } from '../models/thread'; | ||
| import { | ||
| SearchQueryToken, | ||
| TextQueryExpression, | ||
| ToQueryExpression, | ||
| } from '../../services/search/search-query-ast'; | ||
|
|
||
| export const contactSearchFetchLimit = (limit: number, accountCount: number) => | ||
| limit * Math.max(accountCount, 1); | ||
|
Comment on lines
+15
to
+16
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This guard is reasonable defensively, but I don't think
A useful cross-check: if So I'd expect this change to be a no-op for the bug you reported. Happy to be proven wrong if you're seeing Generated by Claude Code |
||
|
|
||
| export const contactsMatchingEmailPrefix = (threads: Thread[], _search: string) => { | ||
| const search = _search.trim().toLowerCase(); | ||
| const byEmail = new Map<string, Contact>(); | ||
|
|
||
| for (const thread of threads) { | ||
| for (const participant of thread.participants || []) { | ||
| const email = (participant.email || '').trim().toLowerCase(); | ||
| if (email.startsWith(search) && !byEmail.has(email)) { | ||
| byEmail.set(email, participant); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return Array.from(byEmail.values()); | ||
| }; | ||
|
|
||
| export const prioritizeContactsMatchingEmailPrefix = (contacts: Contact[], _search: string) => { | ||
| const search = _search.trim().toLowerCase(); | ||
| const emailMatches: Contact[] = []; | ||
| const otherMatches: Contact[] = []; | ||
|
|
||
| for (const contact of contacts) { | ||
| const email = (contact.email || '').trim().toLowerCase(); | ||
| (email.startsWith(search) ? emailMatches : otherMatches).push(contact); | ||
| } | ||
|
|
||
| return emailMatches.concat(otherMatches); | ||
| }; | ||
|
|
||
| /** | ||
| Public: ContactStore provides convenience methods for searching contacts and | ||
|
|
@@ -36,17 +74,21 @@ class ContactStore extends MailspringStore { | |
| // | ||
| // Returns an {Array} of matching {Contact} models | ||
| // | ||
| searchContacts(_search: string, options: { limit?: number } = {}) { | ||
| async searchContacts(_search: string, options: { limit?: number } = {}) { | ||
| const limit = Math.max(options.limit ? options.limit : 5, 0); | ||
| const search = _search.toLowerCase(); | ||
| const search = _search.trim().toLowerCase(); | ||
|
|
||
| const accountCount = AccountStore.accounts().length; | ||
| const extensions = ComponentRegistry.findComponentsMatching({ | ||
| role: 'ContactSearchResults', | ||
| }); | ||
|
|
||
| if (!search || search.length === 0) { | ||
| return Promise.resolve([]); | ||
| return []; | ||
| } | ||
|
|
||
| if (limit === 0) { | ||
| return []; | ||
| } | ||
|
Comment on lines
+90
to
92
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This branch is unreachable. On line 78, Generated by Claude Code |
||
|
|
||
| // Note that we ask for LIMIT * accountCount because we want to | ||
|
|
@@ -55,27 +97,52 @@ class ContactStore extends MailspringStore { | |
| // (which is very slow), we just ask for more items. | ||
| const query = DatabaseStore.findAll<Contact>(Contact) | ||
| .search(search) | ||
| .limit(limit * accountCount) | ||
| .limit(contactSearchFetchLimit(limit, accountCount)) | ||
| .where(Contact.attributes.refs.greaterThan(0)) | ||
| .where(Contact.attributes.hidden.equal(false)) | ||
| .order(Contact.attributes.refs.descending()); | ||
|
|
||
| return query.then(async (_results) => { | ||
| let results = this._distinctByEmail(this._omitFindInMailDisabled(_results)); | ||
| for (const ext of extensions) { | ||
| results = await ext.findAdditionalContacts(search, results); | ||
| } | ||
| if (results.length > limit) { | ||
| results.length = limit; | ||
| } | ||
| return results; | ||
| }) as any as Promise<Contact[]>; | ||
| const historySearch = this._searchSentRecipientContacts(search, limit).catch((err) => { | ||
| console.warn('Unable to search sent-recipient history for autocomplete', err); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this sits on the keystroke path, a failure here would log once per character typed. Generated by Claude Code |
||
| return []; | ||
| }); | ||
|
|
||
| const [_results, historyResults] = await Promise.all([query, historySearch]); | ||
| let results = this._distinctByEmail( | ||
| this._omitFindInMailDisabled(historyResults.concat(_results)) | ||
| ); | ||
| for (const ext of extensions) { | ||
| results = await ext.findAdditionalContacts(search, results); | ||
| } | ||
| results = prioritizeContactsMatchingEmailPrefix(this._distinctByEmail(results), search); | ||
| if (results.length > limit) { | ||
| results.length = limit; | ||
| } | ||
|
Comment on lines
+110
to
+120
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lines change the ranking of every autocomplete result, for everyone — which I think deserves to be an explicit product decision rather than a side effect of the fix. Two compounding effects:
Then If prefix-boosting is desirable (it might well be!), I'd suggest making it a tiebreaker within the Generated by Claude Code |
||
| return results; | ||
| } | ||
|
|
||
| _searchSentRecipientContacts(search: string, limit: number): Promise<Contact[]> { | ||
| if (search.length < 2) { | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| const recipientSearch = new ToQueryExpression( | ||
| new TextQueryExpression(new SearchQueryToken(search)) | ||
| ); | ||
| const threadLimit = Math.min(Math.max(limit * 20, 100), 500); | ||
|
|
||
| return DatabaseStore.findAll<Thread>(Thread) | ||
| .structuredSearch(recipientSearch) | ||
| .where(Thread.attributes.lastMessageSentTimestamp.greaterThan(new Date(0))) | ||
| .order(Thread.attributes.lastMessageSentTimestamp.descending()) | ||
| .limit(threadLimit) | ||
| .then((threads) => contactsMatchingEmailPrefix(threads, search).slice(0, limit)); | ||
| } | ||
|
Comment on lines
+124
to
140
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the part I'd most want to see split out and reworked — it's a substantial new feature rather than a limit fix, and as written I think it will cause noticeable typing lag on large mailboxes. Three specific issues: 1. It runs synchronously on the render thread, on every keystroke. 2. The FTS subselect is unbounded. 3. It doesn't actually restrict to people you've written to. Combined with the prefix filter below, the query casts a wide net ( Generated by Claude Code |
||
|
|
||
| topContacts({ limit = 5 } = {}) { | ||
| const accountCount = AccountStore.accounts().length; | ||
| return DatabaseStore.findAll<Contact>(Contact) | ||
| .limit(limit * accountCount) | ||
| .limit(contactSearchFetchLimit(limit, accountCount)) | ||
| .where(Contact.attributes.refs.greaterThan(0)) | ||
| .where(Contact.attributes.hidden.equal(false)) | ||
| .order(Contact.attributes.refs.descending()) | ||
|
|
||
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.
These assertions restate the one-line implementation of
contactSearchFetchLimitrather than exercising the behavior that regressed, so they wouldn't catch a recurrence of the reported bug.The
ContactStoresuite below is stillxdescribe'd, sosearchContactsitself has no coverage at all. If you're up for it, re-enabling that suite and adding a case for your scenario — asupport@…contact that should be returned for the querysupport— would be far more valuable than the helper tests, and would give us a failing test to confirm the root cause against.Generated by Claude Code