autocomplete: Support user-group mentions; match users by email; rank results by match quality #1775

chrisbobbe · 2025-07-31T00:32:26Z

Fixes #236.
Fixes #233.

Screenshots coming soon.

gnprice · 2025-07-31T00:37:38Z

lib/model/autocomplete.dart

-  final Map<int, String> _normalizedNamesByUser = {};
+  final Map<int, String> _lowercaseNamesByUser = {};


autocomplete [nfc]: Renames/doc changes for explicitness about lowercase

I think the reason to call these fields and methods "normalize" rather than "lowercase" is to anticipate having them be normalized for diacritics as well as for case.

chrisbobbe · 2025-07-31T01:08:49Z

Thanks! Revision pushed, addressing that comment and also simplifying the email-match logic as discussed in the office.

gnprice

Thanks for building this! Here's a review of everything but the next-to-last commit:
15be5b4 autocomplete: Match user results by email match (prefix match)

gnprice · 2025-07-31T00:44:51Z

lib/model/autocomplete.dart

+    if (matchLength == lowercaseName.length && matchLength == _lowercase.length) {
+      return NameMatchQuality.exact;
+    } else if (matchLength == _lowercase.length) {
+      return NameMatchQuality.totalPrefix;
+    }
+
+    if (_testContainsQueryWords(lowercaseNameWords)) {


All these conditions are possible only if the query is no longer than the name, right?

So we could check _lowercase.length <= lowercaseName.length at the top, and then simplify by referring only to _lowercase.length in the loop.

gnprice · 2025-07-31T00:45:56Z

lib/model/autocomplete.dart

+    int matchLength = 0;
+    while (
+      matchLength < lowercaseName.length && matchLength < _lowercase.length
+      && lowercaseName[matchLength] == _lowercase[matchLength]
+    ) {
+      matchLength++;
+    }


Or perhaps better: this is effectively checking lowercaseName.startswith(_lowercase), right?

I think we can first check that, using that method; then distinguish .exact by checking if the lengths are equal.

gnprice · 2025-07-31T00:47:01Z

lib/model/autocomplete.dart

+    final nameMatchQuality = _matchName(
+      lowercaseName: cache.lowercaseNameForUser(user),
+      lowercaseNameWords: cache.lowercaseNameWordsForUser(user));


This seems probably cleaner to read if _matchName handles looking these up from the cache, like _testName before:

Suggested change

final nameMatchQuality = _matchName(

lowercaseName: cache.lowercaseNameForUser(user),

lowercaseNameWords: cache.lowercaseNameWordsForUser(user));

final nameMatchQuality = _matchName(user, cache);

Ah I see, the point of this comes in that later commit where user groups get matched: that's a new call site which does different lookups on the cache. OK, sure.

gnprice · 2025-07-31T00:48:21Z

test/model/autocomplete_test.dart

+      final user1 = eg.user(fullName: 'So Many Ideas');
+      final user2 = eg.user(fullName: 'Some Merry User');


I was really struggling to come up with actual normal names to use 😅

gnprice · 2025-07-31T00:56:29Z

lib/model/autocomplete.dart

+  /// The number of possible values returned by
+  /// [_rankWildcardResult] and [_rankUserResult].
+  static const _numResultRanks = 10;


nit: also _rankUserGroupResult

gnprice · 2025-07-31T01:03:09Z

lib/widgets/autocomplete.dart

+        final userGroup = store.getGroup(id);
+        if (userGroup == null) {
+          // Don't crash on theoretical race between async results-filtering
+          // and losing data for the group.
+          return;


Hmm interesting. I guess if that is possible, it probably also applies to the user case above:

final user = store.getUser(userId)!; // must exist because UserMentionAutocompleteResult

gnprice · 2025-07-31T01:10:04Z

test/model/autocomplete_test.dart

+      // The bucket-sort-by-rank step has its own tests;
+      // see "MentionAutocompleteQuery ranking.


nit: missing close-quote?

gnprice · 2025-07-31T01:15:38Z

test/model/autocomplete_test.dart

-        .deepEquals([WildcardMentionOption.all, WildcardMentionOption.topic]);
-      check(getUsersFromResults(results1.skip(2)))
-        .deepEquals([1, 5, 4, 2, 7, 3, 6]);
+      // 6. User groups by name alphabetical order.


This item isn't like the others — this ordered list is of ranking criteria, not of classes of results.

Instead, change the first item to something like:

Wildcards before individual users; user groups after.

(Well, adjusted a bit for the fact that users matched by email come after groups.)

I think email matching is short-circuited by the fact that this is an empty query, which always matches users by name.

gnprice · 2025-07-31T01:16:18Z

test/model/autocomplete_test.dart

+        isUser(2), isUser(3),
+        isUserGroup(3), isUserGroup(2),


Huh interesting — why is the order reversed between these?

For the users, 2 ("User Two") goes before 3 ("User Three") by DM recency. For the groups, 3 ("User Group Three") goes before 2 ("User Group Two") by alphabet.

Cool, makes sense; and the comment to that effect is helpful, thanks.

gnprice · 2025-07-31T01:20:50Z

test/model/compose_test.dart

@@ -316,6 +316,22 @@ hello
      check(wildcardMention(WildcardMentionOption.topic, store: store()))
        .equals('@**topic**');
    });
+
+    group('user group', () {
+      final userGroup = eg.userGroup(id: 123, name: 'Group Name');


nit: ID isn't relevant to understanding these tests, so can omit

Suggested change

final userGroup = eg.userGroup(id: 123, name: 'Group Name');

final userGroup = eg.userGroup(name: 'Group Name');

(contrasts with the user case, where it can appear in the output)

chrisbobbe · 2025-07-31T19:41:00Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! All looks good now except a couple of nits; and that email commit, which I guess I'll read next.

gnprice · 2025-07-31T19:49:17Z

lib/widgets/autocomplete.dart

-        final user = store.getUser(userId)!; // must exist because UserMentionAutocompleteResult
+        final user = store.getUser(userId);
+        if (user == null) {


autocomplete: Remove a probably-wrong null check I've been waffling on this and haven't managed to convince myself that the null-check was correct.

nit: "null assertion"? This is still something I'd call a null check — it's checking whether this is null.

gnprice · 2025-07-31T19:51:57Z

test/model/autocomplete_test.dart

+        isUser(2), isUser(3),
+        isUserGroup(3), isUserGroup(2),


Cool, makes sense; and the comment to that effect is helpful, thanks.

gnprice · 2025-07-31T19:53:35Z

lib/widgets/autocomplete.dart

        // TODO(i18n) language-appropriate space character; check active keyboard?
        //   (maybe handle centrally in `controller`)
        replacementString = '${userMention(user, silent: query.silent, users: store)} ';
      case WildcardMentionAutocompleteResult(:var wildcardOption):
        replacementString = '${wildcardMention(wildcardOption, store: store)} ';
+      case UserGroupMentionAutocompleteResult(groupId:final id):


nit: propagate name change (here and another case below)

Suggested change

case UserGroupMentionAutocompleteResult(groupId:final id):

case UserGroupMentionAutocompleteResult(:final groupId):

chrisbobbe · 2025-07-31T19:57:45Z

Thanks! Revision pushed.

gnprice

OK, and read the emails commit — just one comment, for a test I think will be quick to add.

gnprice · 2025-07-31T20:01:48Z

test/model/autocomplete_test.dart

+      checkPrecedes('so m', userGroup1, userGroup2);
+    });
+
+    test('email matched case-insensitively', () {


The one other test that'd be good to have here is that emails are matched by prefix only: so e.g. check that "[email protected]" doesn't match a query "mail" or "example" or "example.com".

I've been waffling on this and haven't managed to convince myself that the "!" was correct.

…tion

Since 9815022, this method already takes the whole store, which it uses in order to identify muted users. So it can get the autocomplete-data cache from the store just as well as its caller can.

Like we do in emoji autocomplete. This commit doesn't make any changes to the results ordering; bucketSort sorts stably, and the input is still just the wildcard results followed by the user results. Soon, though, we'd like to rank by match quality and add user-group results (for zulip#233) interleaved with user results. Bucket sorting will help us do this without making many intermediate copies of lists of results; see discussion: https://chat.zulip.org/#narrow/channel/48-mobile/topic/user-group.20mentions.20.23F233/near/2216353 Co-authored-by: Greg Price <[email protected]>

…essages prep

… test This will make it more smoothly generalize to situations where different types of results are more intermingled.

This makes this function better match the end-to-end behavior of our autocomplete ordering, now that that involves this ranking notion. Co-authored-by: Chris Bobbe <[email protected]>

This group also checks that wildcard results come before user results.

…izing

Co-authored-by: Greg Price <[email protected]>

Fixes zulip#236.

Fixes zulip#233.

chrisbobbe · 2025-08-01T03:37:36Z

Thanks, done! PTAL.

chrisbobbe requested a review from gnprice July 31, 2025 00:32

chrisbobbe assigned gnprice Jul 31, 2025

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 31, 2025

gnprice reviewed Jul 31, 2025

View reviewed changes

chrisbobbe force-pushed the pr-user-group-autocomplete branch from 1863ddd to 8ca8bdb Compare July 31, 2025 01:08

gnprice reviewed Jul 31, 2025

View reviewed changes

chrisbobbe force-pushed the pr-user-group-autocomplete branch from 8ca8bdb to ae92100 Compare July 31, 2025 19:40

gnprice reviewed Jul 31, 2025

View reviewed changes

chrisbobbe force-pushed the pr-user-group-autocomplete branch from ae92100 to 1f3d350 Compare July 31, 2025 19:57

gnprice reviewed Jul 31, 2025

View reviewed changes

chrisbobbe and others added 14 commits July 31, 2025 20:33

autocomplete: Remove a probably-wrong null assertion

c8f1b1e

I've been waffling on this and haven't managed to convince myself that the "!" was correct.

autocomplete [nfc]: Construct user result in query.testUser

ae641aa

autocomplete [nfc]: Construct wildcard result in query.testWildcardOp…

ab80f59

…tion

autocomplete [nfc]: Cut separate cache param on testUser, reuse store

70c1617

Since 9815022, this method already takes the whole store, which it uses in order to identify muted users. So it can get the autocomplete-data cache from the store just as well as its caller can.

autocomplete test [nfc]: Simplify end-to-end ranking test with pure-m…

c4ce05f

…essages prep

autocomplete test [nfc]: Use deepEquals and conditions for end-to-end…

a7ecae7

… test This will make it more smoothly generalize to situations where different types of results are more intermingled.

autocomplete test [nfc]: Apply ranking in debugCompareUsers

3e6326f

This makes this function better match the end-to-end behavior of our autocomplete ordering, now that that involves this ranking notion. Co-authored-by: Chris Bobbe <[email protected]>

autocomplete test [nfc]: Reorder mention sort/match tests to match impl

4eac4bf

autocomplete test [nfc]: Fix name for group that's not just about users

e8e3a31

This group also checks that wildcard results come before user results.

autocomplete [nfc]: Renames/doc changes for explicitness about normal…

21dbbff

…izing

autocomplete: Rank user results by name-match quality

d1d376a

Co-authored-by: Greg Price <[email protected]>

autocomplete: Match user results by email match (prefix match)

f0b7740

Fixes zulip#236.

autocomplete: Add user-group results to mention autocomplete

7cfc4ac

Fixes zulip#233.

chrisbobbe force-pushed the pr-user-group-autocomplete branch from 1f3d350 to 7cfc4ac Compare August 1, 2025 03:37

		final Map<int, String> _normalizedNamesByUser = {};
		final Map<int, String> _lowercaseNamesByUser = {};

		final user1 = eg.user(fullName: 'So Many Ideas');
		final user2 = eg.user(fullName: 'Some Merry User');

		// The bucket-sort-by-rank step has its own tests;
		// see "MentionAutocompleteQuery ranking.

	final userGroup = eg.userGroup(id: 123, name: 'Group Name');
	final userGroup = eg.userGroup(name: 'Group Name');

	case UserGroupMentionAutocompleteResult(groupId:final id):
	case UserGroupMentionAutocompleteResult(:final groupId):

autocomplete: Support user-group mentions; match users by email; rank results by match quality #1775

Are you sure you want to change the base?

autocomplete: Support user-group mentions; match users by email; rank results by match quality #1775

Uh oh!

Conversation

chrisbobbe commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 31, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 31, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 31, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 1, 2025

Uh oh!

Uh oh!

chrisbobbe commented Jul 31, 2025 •

edited

Loading