Channel link autocomplete #1902

sm-sayedi · 2025-10-08T17:05:09Z

Fixes-partly: #124 (topic link autocomplete will be its own PR)

Screenshots

Empty query	A query written	An option chosen

Autocompleting channels with problematic characters	Channel with problematic characters chosen	Message sent

Screen recording

Channel.autocomplete.recording.mov

gnprice · 2025-10-09T20:42:51Z

Exciting! Thanks for building this.

Just to record here what I said on the team call yesterday: for this PR, we can start the reviews in parallel with you writing the tests. So I'd suggest going ahead and adding those docs and comments next — then once you consider the PR all ready except for the tests, just mention that here and add the "maintainer review" label.

sm-sayedi · 2025-10-09T21:06:53Z

Thanks @gnprice for mentioning this. This is now ready for an initial review. (While working on the first todo, I realized that there were a few other places that needed some changes, which caused a delay 😀)

sm-sayedi · 2025-10-10T05:29:27Z

(just rebased atop main with conflicts resolved)

sm-sayedi · 2025-10-21T19:40:12Z

@chrisbobbe Pushed a new revision with tests included.

chrisbobbe · 2025-10-22T23:41:31Z

Thanks for this, and apologies for my delay in reviewing! Here's a review of the first six commits:

05c8437 channel: Add isRecentlyActive field
5a6b96a api: Update ChannelDeleteEvent to match new API changes
607dc8b basic: Add tryCast method
c9d282e store: Call AutocompleteViewManager.handleUserGroupRemove/UpdateEvent
c5f6c77 autocomplete [nfc]: Remove User(Group) params from _rankUser(Group)Result
a459122 autocomplete [nfc]: Move _matchName up to AutocompleteQuery

plus some comments on later commits where I happened to see something interesting. 🙂

chrisbobbe

Oh no—thanks for the ping in DMs, somehow I didn't actually submit that review yesterday! Here it is.

chrisbobbe · 2025-10-22T17:09:06Z

test/example_data.dart

    historyPublicToSubscribers: historyPublicToSubscribers ?? true,
    messageRetentionDays: messageRetentionDays,
    channelPostPolicy: channelPostPolicy ?? ChannelPostPolicy.any,
+    isRecentlyActive: isRecentlyActive ?? false,


I think true might be a more natural default value for this?

Changed to true. The false default value was to make the tests in "ranking across signals" and "final results end-to-end" of "ChannelLinkAutocompleteView" group a little less verbose.🙂

chrisbobbe · 2025-10-22T20:58:03Z

lib/api/model/model.dart

  @JsonKey(name: 'stream_post_policy')
  ChannelPostPolicy? channelPostPolicy; // TODO(server-10) remove
  // final bool isAnnouncementOnly; // deprecated for `channelPostPolicy`; ignore
+  bool? isRecentlyActive; // TODO(server-10)


channel: Add isRecentlyActive field

Since we're already not matching the order in the API doc (see e.g. #1894 (comment) ), I'd put this next to the related-looking field streamWeeklyTraffic, perhaps just above it without an empty line in between.

Similarly elsewhere in this commit.

chrisbobbe · 2025-10-22T21:05:59Z

lib/api/model/events.dart

+    required super.id,
+    required this.streams,
+    required this.streamIds,
+  }) : assert(streams != null || streamIds != null);


We try to reserve assert for invariants that are our own responsibility, i.e. those that won't be broken except for some broken logic in the client. Here, the invariant exists, but it's one that can be broken by something out of our control, in particular a badly-behaving server.

Also, asserts don't run in production, so this won't work as "crunchy shell" validation. It makes sense to want such validation, so the "soft center" of the app can rely on this invariant. But let's do it in ChannelDeleteEvent.fromJson; for an example to follow, see DeleteMessageEvent.fromJson.

chrisbobbe · 2025-10-22T21:07:03Z

lib/api/model/events.dart


-  final List<ZulipStream> streams;
+  final List<ZulipStreamId>? streams; // TODO(server-10): remove
+  final List<int>? streamIds; // TODO(server-10): remove nullability


nit: we'd normally say TODO(server-10) make required or just TODO(server-10).

chrisbobbe · 2025-10-22T21:20:41Z

lib/model/channel.dart

-          streams.remove(stream.streamId);
-          streamsByName.remove(stream.name);
-          subscriptions.remove(stream.streamId);
+        final channelIds = event.streamIds ?? event.streams!.map((e) => e.streamId);


Oh actually, we can make this nicer by encapsulating this conditional in the API-binding layer—ChannelDeleteEvent can have just final List<int> streamIds (maybe channelIds, as the more modern name?), and it can read its value depending on what the JSON looks like.

Something like this (untested)?:

/// A [ChannelEvent] with op `delete`: https://zulip.com/api/get-events#stream-delete @JsonSerializable(fieldRename: FieldRename.snake) class ChannelDeleteEvent extends ChannelEvent { @override @JsonKey(includeToJson: true) String get op => 'delete'; @JsonKey(readValue: _readChannelIds) final List<int> channelIds; // TODO(server-10) simplify away; rely on stream_ids static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) { final channelIds = json['stream_ids'] as List<int>?; if (channelIds != null) return channelIds; final channels = json['streams'] as List<dynamic>; return channels .map((c) => (c as Map<String, dynamic>)['stream_id'] as int) .toList(); } ChannelDeleteEvent({ required super.id, required this.channelIds, }); factory ChannelDeleteEvent.fromJson(Map<String, dynamic> json) => _$ChannelDeleteEventFromJson(json); @override Map<String, dynamic> toJson() => _$ChannelDeleteEventToJson(this); }

(In that code, the crunchy-shell validation is done by the final channels = json['streams'] as List<dynamic>; line, which will throw if both .stream_ids and .streams are absent in the json.)

Changed to the new version. One thing that this does is that in toJson, there will be an entry with key channel_ids; not exactly what the server gives us (stream_ids or streams). Should we edit the toJson method to match the server data, or is it not that important since we don't use it to send it back to the server?

Ahh, so there was a test failing in test/model/store_test.dart and the fix was to include streams in toJson.

Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart index 6a0d9ffa4..4d9c5121c 100644 --- lib/api/model/events.dart +++ lib/api/model/events.dart @@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent { // TODO(server-10) simplify away; rely on stream_ids static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) { final channelIds = json['stream_ids'] as List<dynamic>?; - if (channelIds != null) channelIds.map((id) => id as int).toList(); + if (channelIds != null) return channelIds.map((id) => id as int).toList(); final channels = json['streams'] as List<dynamic>; return channels

chrisbobbe · 2025-10-22T23:07:21Z

lib/widgets/autocomplete.dart

+              overflow: TextOverflow.ellipsis,
+              color: designVariables.contextMenuItemMeta)),
+          child: BlockContentList(
+            nodes: parseContent(channel!.renderedDescription).nodes),


Neat! Unfortunately we're not ready to show the rendered channel description; some of our content widgets will break if they appear outside the context of a message, because they use InheritedMessage.of(context), and we need to address that systematically, which is #488. See related issues:

Show channel description in channel action sheet #1896

content: Support Zulip content outside messages (even outside per-account contexts) #488

For now let's do as I did in #1877:

Not try to show the channel description here

File an issue for it and leave a TODO

Filed #1945. Also, looking at https://github.com/zulip/zulip-flutter/blob/main/lib/widgets/content.dart, it seems like InheritedMessage.of(context) is used in two places, MessageImagePreview and MessageInlineVideo, and by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now. But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now.

For this sort of detail I'd want to rely on API guarantees rather than manual testing on a current server. It's at least plausible that we could run into different behavior with some old server we still support (like 7), or even on any server, including a modern one like CZO, for a channel that was last updated when the server version was ancient, like 3 or something.

But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

Yep, this is also true :) it'll be nice to be systematic about it.

chrisbobbe · 2025-10-22T23:09:05Z

lib/model/autocomplete.dart

+  // Behavior we have that web doesn't and might like to follow:
+  // - A "word-prefixes" match quality on channel names:
+  //   see [NameMatchQuality.wordPrefixes], which we rank on.
+  //
+  // Behavior web has that seems undesired, which we don't plan to follow:
+  // - A "word-boundary" match quality on channel names:
+  //   special rank when the whole query appears contiguously
+  //   right after a word-boundary character.
+  //   Our [NameMatchQuality.wordPrefixes] seems smarter.
+  // - Ranking some case-sensitive matches differently from case-insensitive
+  //   matches. Users will expect a lowercase query to be adequate.


Am I reading web's sort_streams correctly that it also considers channel descriptions in the filtering and ranking? I don't personally think we need to do that, but it probably deserves a mention here.

chrisbobbe · 2025-10-22T23:24:43Z

lib/model/autocomplete.dart

+    return switch((tryCast<Subscription>(a), tryCast<Subscription>(b))) {
+      (Subscription(), null) => -1,
+      (null, Subscription()) =>  1,
+      (Subscription(isMuted:  false), Subscription(isMuted:   true)) => -1,
+      (Subscription(isMuted:   true), Subscription(isMuted:  false)) =>  1,
+      (Subscription(pinToTop:  true), Subscription(pinToTop: false)) => -1,
+      (Subscription(pinToTop: false), Subscription(pinToTop:  true)) =>  1,
+      _                                                              =>  0,
+    };


I'd prefer not to add and use tryCast for this, and instead do something like:

Suggested change

return switch((tryCast<Subscription>(a), tryCast<Subscription>(b))) {

(Subscription(), null) => -1,

(null, Subscription()) => 1,

(Subscription(isMuted: false), Subscription(isMuted: true)) => -1,

(Subscription(isMuted: true), Subscription(isMuted: false)) => 1,

(Subscription(pinToTop: true), Subscription(pinToTop: false)) => -1,

(Subscription(pinToTop: false), Subscription(pinToTop: true)) => 1,

_ => 0,

};

if (a is Subscription && b is! Subscription) return -1;

if (a is! Subscription && b is Subscription) return 1;

return switch((a, b)) {

(Subscription(isMuted: false), Subscription(isMuted: true)) => -1,

(Subscription(isMuted: true), Subscription(isMuted: false)) => 1,

(Subscription(pinToTop: true), Subscription(pinToTop: false)) => -1,

(Subscription(pinToTop: false), Subscription(pinToTop: true)) => 1,

_ => 0,

};

which is equivalent and doesn't add a step for the reader to interpret where null comes from and what it means. It also separates the main, headline logic from the rest, corresponding to the dartdoc's choice of what goes in its first line vs. the body:

/// Comparator that puts subscribed channels before unsubscribed ones. /// /// For subscribed channels, it puts them in the following way: /// pinned unmuted > unpinned unmuted > pinned muted > unpinned muted

(also nit: s/way/order/ in that dartdoc)

chrisbobbe · 2025-10-22T23:31:41Z

lib/model/autocomplete.dart

+  /// Comparator that puts channels with more weekly traffic first.
+  ///
+  /// A channel with undefined weekly traffic (`null`) is put after the channel
+  /// with a weekly traffic defined (even if it is zero).
+  ///
+  /// Weekly traffic is the average number of messages sent to the channel per
+  /// week, which is determined by [ZulipStream.streamWeeklyTraffic].


Suggested change

/// Comparator that puts channels with more weekly traffic first.

///

/// A channel with undefined weekly traffic (`null`) is put after the channel

/// with a weekly traffic defined (even if it is zero).

///

/// Weekly traffic is the average number of messages sent to the channel per

/// week, which is determined by [ZulipStream.streamWeeklyTraffic].

/// Comparator that puts channels with more [ZulipStream.streamWeeklyTraffic] first.

///

/// A channel with undefined weekly traffic (`null`) is put after the channel

/// with a weekly traffic defined (even if it is zero).

This is a very reasonable definition of "weekly traffic" 🙂 and so isn't likely to bitrot i.e. become incorrect over time. But since we're just using ZulipStream.streamWeeklyTraffic directly (no computations on it), let's leave that field's definition as the single place where we write its definition, so we only have to change that one thing if the meaning changes.

…I see that we haven't actually written down the field's meaning, which we might've done in dartdoc on the field. But that's quite normal and appropriate; by leaving it blank, we mean to defer to the API documentation, which is linked in the class ZulipStream dartdoc.

chrisbobbe · 2025-10-22T23:37:29Z

test/model/compose_test.dart

+      eg.stream(streamId: 5, name: 'UI [v2]'),
+      eg.stream(streamId: 6, name: r'Save $$'),
+    ];
+    store.addStreams(channels);


This should be awaited; similarly in a few other places.

Thanks to the discarded_futures lint for catching this, actually; I was playing with it for #731 this morning 🙂

sm-sayedi · 2025-10-24T19:54:11Z

Thanks @chrisbobbe for the review. Pushed new changes, PTAL.

chrisbobbe

Thanks! This is working great; comments below, this time from reading the whole branch.

chrisbobbe · 2025-10-24T22:11:10Z

lib/api/model/events.dart

+      case ChannelPropertyName.isRecentlyActive:
+        return value as bool?;


nit: bump on #1902 (comment)

chrisbobbe · 2025-10-24T22:11:22Z

test/example_data.dart

+    case ChannelPropertyName.isRecentlyActive:
+      assert(value is bool?);


nit: bump on #1902 (comment)

chrisbobbe · 2025-10-27T21:25:32Z

lib/model/channel.dart

-          streams.remove(stream.streamId);
-          streamsByName.remove(stream.name);
-          subscriptions.remove(stream.streamId);
+        final channelIds = event.streamIds ?? event.streams!.map((e) => e.streamId);


Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart index 6a0d9ffa4..4d9c5121c 100644 --- lib/api/model/events.dart +++ lib/api/model/events.dart @@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent { // TODO(server-10) simplify away; rely on stream_ids static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) { final channelIds = json['stream_ids'] as List<dynamic>?; - if (channelIds != null) channelIds.map((id) => id as int).toList(); + if (channelIds != null) return channelIds.map((id) => id as int).toList(); final channels = json['streams'] as List<dynamic>; return channels

chrisbobbe · 2025-10-27T23:19:26Z

lib/model/autocomplete.dart

+    for (final view in _channelLinkAutocompleteViews) {
+      view.reassemble();
+    }
  }


Sure! Sounds like that would help us avoid a bug like the one fixed here.

chrisbobbe · 2025-10-27T23:21:12Z

lib/model/autocomplete.dart

+  // Similar reasoning as in _mentionIntentRegex.
+  const before = r'(?<=^|\s|\p{Punctuation})';


Also could you add test cases for the strings "#124"

Ah! It seems GitHub mangled my comment: I wrote zulip/zulip-flutter#124, but it linkified it the same way as it linkifies #124, because this is a PR in zulip/zulip-flutter.

chrisbobbe · 2025-10-31T19:28:15Z

lib/model/autocomplete.dart

+    // As Web, match both '#channel' and '#**channel'. In both cases, the raw
+    // query is going to be 'channel'. Matching the second case ('#**channel')
+    // is useful when the user selects a channel from the autocomplete list, but
+    // then starts pressing "backspace" to edit the query and choose another
+    // option, instead of clearing the entire query and starting from scratch.


Interesting. Looks like web also does the corresponding thing for @-mentions: https://github.com/zulip/zulip/blob/1e78447c5/web/src/composebox_typeahead.ts#L516-L531

function filter_mention_name(current_token: string): string | undefined { if (current_token.startsWith("**")) { current_token = current_token.slice(2); } else if (current_token.startsWith("*")) { current_token = current_token.slice(1); } if (current_token.includes("*")) { return undefined; } // Don't autocomplete if there is a space following an '@' if (current_token.startsWith(" ")) { return undefined; } return current_token; }

This is maybe more important for channel/topic autocomplete, right? Because (once the topic part is implemented) you might backspace as part of figuring out how to get just a channel link without a topic. But this is a good prompt to file an issue and add a TODO for doing this with @-mention autocomplete; would you do those?

Sure, that would be an improvement. I have always missed that feature for @-mentions; filed #1967.

chrisbobbe · 2025-10-31T19:36:12Z

lib/widgets/autocomplete.dart

+    } else {
+      icon = null;
+      iconColor = null;
+      label = zulipLocalizations.unknownChannelName;


I think we normally show "(unknown channel)" in italics, to distinguish it from a potential channel with that name.

chrisbobbe · 2025-10-31T21:02:40Z

lib/widgets/autocomplete.dart

+      padding: EdgeInsetsDirectional.fromSTEB(12, 4, 10, 4),
+      child: Row(spacing: 10, children: [
+        SizedBox.square(dimension: 24,
+          child: Icon(size: 18, color: iconColor, icon)),


The rows have 44px height in the Figma, but this gives 32px height (unless increased via the device text-size setting).

To bring it up to 44px, we could structure it similarly to MentionAutocompleteItem—

Suggested change

padding: EdgeInsetsDirectional.fromSTEB(12, 4, 10, 4),

child: Row(spacing: 10, children: [

SizedBox.square(dimension: 24,

child: Icon(size: 18, color: iconColor, icon)),

padding: EdgeInsetsDirectional.fromSTEB(4, 4, 8, 4),

child: Row(spacing: 6, children: [

SizedBox.square(dimension: 36,

child: Center(

child: Icon(size: 18, color: iconColor, icon))),

—which could be helpful in a potential future where we made a generic AutocompleteItem widget that serves both kinds of autocomplete.

chrisbobbe · 2025-10-31T21:05:10Z

lib/model/compose.dart

+///
+/// [fallbackMarkdownLink] will be used if the channel name includes some faulty
+/// characters that will break normal #**channel** rendering.
+String channelLinkSyntax(ZulipStream channel, {


Just channelLink, I think; the "syntax" part is implied. (As in existing functions in this file, like quoteAndReply which creates quote-and-reply syntax, or userMention which creates user-mention syntax.)

chrisbobbe · 2025-10-31T21:22:50Z

lib/model/compose.dart

+/// Markdown link for channel, topic, or message when the channel or topic name
+/// includes characters that will break normal markdown rendering.
+///
+/// Refer to [_channelTopicFaultyCharsReplacements] for a complete list of
+/// these characters.
+// Corresponds to `topic_link_util.get_fallback_markdown_link` in Zulip web;
+//   https://github.com/zulip/zulip/blob/b42d3e77e/web/src/topic_link_util.ts#L96-L108
+String fallbackMarkdownLink({


channel: Finish channel link autocomplete for compose box

Could you separate this special character-replacement logic into its own commit? I want to see if we can ground our reasoning in API documentation. As far as that goes, there's nothing "invalid" or "faulty" about these characters appearing in channel names.

sm-sayedi · 2025-11-03T10:05:38Z

Thanks @chrisbobbe for the previous review. Pushed a new revision, with some new commits added, PTAL.

65ad611 api: Add InitialSnapshot.maxChannelNameLength
3db4161 realm: Add RealmStore.maxChannelNameLength
442054d autocomplete: Introduce AutocompleteViewManager._autocompleteViews
a6caa11 compose: Introduce PerAccountStore in ComposeController

(The media in the PR description has also been updated.)

chrisbobbe · 2025-11-04T00:08:53Z

Ah I see this has gathered some conflicts; would you resolve those please?

chrisbobbe

Thanks! A few comments below.

chrisbobbe · 2025-11-05T21:36:42Z

lib/model/autocomplete.dart

-  final Set<MentionAutocompleteView> _mentionAutocompleteViews = {};
-  final Set<TopicAutocompleteView> _topicAutocompleteViews = {};
-  final Set<EmojiAutocompleteView> _emojiAutocompleteViews = {};
+  final Set<AutocompleteView> _autocompleteViews = {};


autocomplete: Introduce `AutocompleteViewManager._autocompleteViews`

I'd prefer to separate the bugfix commit from the NFC (refactor) commit, so each can be verified independently.

chrisbobbe · 2025-11-05T21:46:25Z

lib/model/autocomplete.dart

  /// and begin the search for the initial query.
  AutocompleteView({required this.store, required QueryT query})
      : _query = query {
+    store.autocompleteViewManager.registerAutocomplete(this);


Does this change the order of this and the _startSearch() call, and if so, is that OK?

I'm not sure if this is the right place for the .registerAutocomplete call—it might be best for it to stay in the subclasses' init factories.

I think the order will not make a difference. This line adds the autocomplete view to the manager so that its reassemble method is called during hot reload, calling _startSearch again. In the new revision, I swapped its order with _startSearch so as to match its calling order with what was before the refactor.

I think staying .registerAutocomplete in subclasses' init factories won't make a difference with the current state of the codebase. Please let me know if I am missing something, I'd be glad to move it back there. 🙂

chrisbobbe · 2025-11-08T00:46:19Z

lib/widgets/compose_box.dart

+  ComposeBoxController({required this.store})
+    : content = ComposeContentController(store: store);
+
+  final PerAccountStore store;
+  final ComposeContentController content;


No need for store to be a field on ComposeBoxController, I think; the constructor can just pass it to ComposeContentController and then drop it:

Suggested change

ComposeBoxController({required this.store})

: content = ComposeContentController(store: store);

final PerAccountStore store;

final ComposeContentController content;

ComposeBoxController({required PerAccountStore store})

: content = ComposeContentController(store: store);

final ComposeContentController content;

I've felt kind of unsatisfied with the way ComposeTopicController (and now ComposeContentController) hold references to the store; it feels like the kind of detail that could be involved in bugs where we mishandle logging out, or dealing with expired event queues.

I have a local branch sketching out how things could look if we remove the store field from ComposeTopicController, and instead ask callers for a store when they want interpretations of the topic-input value (textNormalized, validationErrors, etc.). @gnprice do you want to see that? The change feels a little awkward: if controller is our ComposeBoxController, instead of saying controller.topic.textNormalized we'd say controller.topic.interpretValue(store).textNormalized. It's more verbose, and also store is often disregarded because interpretValue caches its result to save computation.

Hmm, good point re: the store changing.

What if we have it hold a BuildContext instead? Then it can use that to look up the store when needed, without complicating the API for its getters and methods.

… On the other hand: there's only one place these controllers live (outside of tests), namely _ComposeBoxState. We have an onNewStore implementation there which seems like it should be solid for keeping the store fields up to date. So I think we can be pretty confident there isn't an existing bug of this form, and I think this isn't particularly vulnerable to gaining such a bug in the future.

chrisbobbe · 2025-11-08T01:19:34Z

lib/model/autocomplete.dart

+  int get _maxLookbackForAutocompleteIntent {
+    return 1 // intent character i.e., "#"
+      + 2 // some optional characters i.e., "_" for silent mention or "**"
+      + store.maxChannelNameLength;
+  }


Suggested change

int get _maxLookbackForAutocompleteIntent {

return 1 // intent character i.e., "#"

+ 2 // some optional characters i.e., "_" for silent mention or "**"

+ store.maxChannelNameLength;

}

int get _maxLookbackForAutocompleteIntent {

return 1 // intent character i.e., "#"

+ 2 // some optional characters i.e., "_" for silent mention or "**"

// Per the API doc, maxChannelNameLength is in Unicode code points.

// We walk the string by UTF-16 code units, and there might be one or two

// of those encoding each Unicode code point.

+ 2 * store.maxChannelNameLength;

}

I think?

Also nit: "e.g.", not "i.e.": https://www.merriam-webster.com/grammar/ie-vs-eg-abbreviation-meaning-usage-difference

chrisbobbe · 2025-11-08T01:46:43Z

lib/model/compose.dart

+/// these characters.
+// Corresponds to `topic_link_util.get_fallback_markdown_link` in Zulip web;
+//   https://github.com/zulip/zulip/blob/b42d3e77e/web/src/topic_link_util.ts#L96-L108
+String fallbackMarkdownLink({


In web, when I tap the channel autocomplete option "chris-public-test-*", it gives

[#chris-public-test-*](#narrow/channel/533-chris-public-test-*)

but mobile gives

[chris-public-test-*](#narrow/channel/533-chris-public-test-*)

so I think we should start the text part with "#".

Absolutely, thanks for the catch.

Right now, this is useful in how far back from the cursor we look to find a channel-link autocomplete (actually any autocomplete) interaction in compose box.

In the following commits, this will be used as one of the criteria for sorting channels in channel link autocomplete.

There are new changes made to `stream op: delete` event in server-10: - The `streams` field which used to be an array of the just-deleted channel objects is now an array of objects which only contains IDs of the just-deleted channels (the app throws away its event queue and reregisters before this commit). - The same `streams` field is also deprecated and will be removed in a future release. - As a replacement to `streams`, `stream_ids` is introduced which is an array of the just-deleted channels IDs. Related CZO discussion: https://chat.zulip.org/#narrow/channel/378-api-design/topic/stream.20deletion.20events/near/2284969

So to call AutocompleteViewManager.unregisterEmojiAutocomplete.

…iews` This set replaces the three sets of different `AutocompleteView` subclasses, simplifying the code.

These two methods were introduced but never called.

Also, generalize the dartdoc of NameMatchQuality. For almost all types of autocompletes, the matching mechanism/quality to an autocomplete query seems to be the same with rare exceptions (at the time of writing this —— 2025-11, only the emoji autocomplete matching mechanism is different).

As of this commit, it's not yet possible in the app to initiate a channel link autocomplete interaction. So in the widgets code that would consume the results of such an interaction, we just throw for now, leaving that to be implemented in a later commit.

…rseMarkedText This was first added in 0886948, but seems to have been accidentally removed in 046ceab.

This way, subclasses can use the reference to the store for different purposes, such as using `max_topic_length` for the topic length instead of the hard-coded limit of 60, or using `max_stream_name_length` for how far back from the cursor we look to find a channel-link autocomplete interaction in compose box.

For this commit we temporarily intercept the query at the AutocompleteField widget, to avoid invoking the widgets that are still unimplemented. That lets us defer those widgets' logic to a separate later commit.

… after "#"

This will make it easy to use the fragment string in several other places, such as in the next commits where we need to create a fallback markdown link for a channel.

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=7952-30060&t=YfdW2W1p4ROsq9db-0 Fixes-partly: zulip#124

sm-sayedi · 2025-11-10T17:14:24Z

Thanks for the review. New changes pushed.

chrisbobbe

Thanks! LGTM, marking for Greg's review.

chrisbobbe · 2025-11-10T19:12:28Z

test/model/autocomplete_test.dart

+    // '🙂' is 2 utf-16 code units.
+    doTest('check ~#**🙂🙂🙂🙂🙂^', channelLink('🙂🙂🙂🙂🙂'), maxChannelName: 5);
+    doTest('check #**🙂🙂🙂🙂🙂🙂^', null, maxChannelName: 5);


Nice, these are helpful test cases.

gnprice

Thanks @sm-sayedi for building this, and @chrisbobbe for the previous reviews!

Here's a review of the first 10 commits:
fac4457 api: Add InitialSnapshot.maxChannelNameLength
fd4d629 realm: Add RealmStore.maxChannelNameLength
ca7414b api: Add ZulipStream.isRecentlyActive
5b7d81b api: Update ChannelDeleteEvent to match new API changes
5563131 autocomplete: Call EmojiAutocompleteView.reassemble where needed
89a8a40 emoji: Add EmojiAutocompleteView.dispose
b7c5278 autocomplete [nfc]: Introduce AutocompleteViewManager._autocompleteViews
9696df9 store: Call AutocompleteViewManager.handleUserGroupRemove/UpdateEvent
994944e autocomplete [nfc]: Move _matchName up to AutocompleteQuery
64d68a0 autocomplete: Add view-model ChannelLinkAutocompleteView

except for the last one's tests. Still ahead are those tests and the remaining 8 commits:
3543b71 autocomplete test [nfc]: Use MarkedTextParse as the return type of parseMarkedText
1a9bbf6 compose: Introduce PerAccountStore in ComposeController
267f7b0 autocomplete: Identify when the user intends a channel link autocomplete
b58f34c autocomplete [nfc]: Add a TODO(#1967) for ignoring starting "**" after "#"
5510c30 autocomplete test: Make setupToComposeInput accept channels param
a6d0938 internal_link [nfc]: Factor out constructing fragment in its own method
831ccf1 compose: Introduce fallbackMarkdownLink function
b654dd1 channel: Finish channel link autocomplete for compose box

gnprice · 2025-11-10T22:57:24Z

lib/api/model/events.dart

+      case ChannelPropertyName.isRecentlyActive:
+        return value as bool?;


This should be just bool, right? This property will be a bool when it exists at all.

gnprice · 2025-11-10T23:04:45Z

lib/model/channel.dart

+          final channel = streams[channelId];
+          if (channel == null) break;
+          assert(identical(streams[channel.streamId], streamsByName[channel.name]));
+          assert(subscriptions[channelId] == null
+            || identical(subscriptions[channelId], streams[channelId]));
+          streams.remove(channel.streamId);
+          streamsByName.remove(channel.name);
+          subscriptions.remove(channel.streamId);


It's kind of confusing how this alternates between channelId and channel.streamId. Those should be equal. It'd therefore be clearer to assert those are equal, and then use channelId throughout.

gnprice · 2025-11-10T23:05:54Z

lib/model/channel.dart

+          final channel = streams[channelId];
+          if (channel == null) break;
+          assert(identical(streams[channel.streamId], streamsByName[channel.name]));
+          assert(subscriptions[channelId] == null
+            || identical(subscriptions[channelId], streams[channelId]));


Similarly, since this now looks up streams[channelId] up front, it can skip repeating that lookup in these asserts.

gnprice · 2025-11-10T23:07:55Z

lib/model/channel.dart

+          final channel = streams[channelId];
+          if (channel == null) break;
+          assert(identical(streams[channel.streamId], streamsByName[channel.name]));
+          assert(subscriptions[channelId] == null
+            || identical(subscriptions[channelId], streams[channelId]));
+          streams.remove(channel.streamId);


Rather than look up the channel by ID once, then look it up again as part of remove, this can call remove up front and keep the return value.

(The existing code has asserts that lack that kind of optimization — but they're asserts, so only run in debug builds anyway and optimization is less of a concern.)

gnprice · 2025-11-10T23:26:48Z

lib/model/autocomplete.dart

+    final channelId = switch (narrow) {
+      ChannelNarrow(:var streamId) || TopicNarrow(:var streamId) => streamId,
+      DmNarrow() => null,
+      CombinedFeedNarrow()
+        || MentionsNarrow()
+        || StarredMessagesNarrow()
+        || KeywordSearchNarrow() => () {


nit: I think a switch statement (like in MentionAutocompleteView) would be easier to read here.

One reason is that the formatting would be more consistent that way:

each case gets one line (vs. here where TopicNarrow appears off on the right);

each outcome starts at the same column (vs. here where streamId, null, and the IIFE all start in quite different places).

The switch statement would also simplify diffs when adding (or removing) subclasses for Narrow: just insert (or remove) lines case FooNarrow:, vs. here where existing lines might need to be edited to add or remove a || prefix or => () { suffix. Put another way: it's helpful when the different items in a list are all formatted the same, vs. the first or last items being formatted differently.

gnprice · 2025-11-10T23:34:15Z

lib/model/autocomplete.dart

+  /// A channel with undefined weekly traffic (`null`) is put after the channel
+  /// with a weekly traffic defined (even if it is zero).


Is that behavior desired?

From the code at your handy link above (https://github.com/zulip/zulip/blob/c3fdee6ed/web/src/typeahead_helper.ts#L972-L988), it looks like web treats unknown weekly traffic equal to zero.

From the API docs for stream_weekly_traffic:

If null, the channel was recently created and there is insufficient data to estimate the average traffic.

That seems like it should rank at least as high as a value of zero.

So probably we should just match the web behavior.

gnprice · 2025-11-10T23:40:51Z

lib/model/autocomplete.dart

+    return switch ((a.streamId, b.streamId)) {
+      (int id, _) when id == composingToChannelId => -1,
+      (_, int id) when id == composingToChannelId =>  1,


When both channels match, this gives -1 but should give 0.

gnprice · 2025-11-10T23:43:25Z

lib/model/autocomplete.dart

+  static int compareByRecentActivity(ZulipStream a, ZulipStream b) {
+    return switch((a.isRecentlyActive, b.isRecentlyActive)) {
+      (true, false) => -1,
+      (false, true) =>  1,


The corresponding logic in web seems to be a bit more complicated, in stream_list_sort.ts:

export function has_recent_activity(sub: StreamSubscription): boolean { if (!filter_out_inactives || sub.pin_to_top) { // If users don't want to filter inactive streams // to the bottom, we respect that setting and don't // treat any streams as dormant. // // Currently this setting is automatically determined // by the number of streams. See the callers // to set_filter_out_inactives. return true; } return sub.is_recently_active || sub.newly_subscribed;

Can you explain why we don't match those other wrinkles?

gnprice · 2025-11-10T23:48:19Z

lib/model/autocomplete.dart

+  // Check `typeahead_helper.compare_by_activity` in Zulip web;
+  // We follow the behavior of Web but with a small difference in that Web
+  // compares "recent activity" only for subscribed channels, but we do it
+  // for unsubscribed ones too.
+  //   https://github.com/zulip/zulip/blob/c3fdee6ed/web/src/typeahead_helper.ts#L972-L988
+  static int _compareByRelevance(ZulipStream a, ZulipStream b, {


This sounds like an implementation comment, so let's put it inside the method's body.

For an example of how to present a (more complex) comparison with web's behavior, see EmojiAutocompleteView._rankResult. That might in particular be helpful when adding an answer to my question about compareByRecentActivity vs. stream_list_sort.has_recent_activity.

gnprice · 2025-11-11T00:01:04Z

lib/model/autocomplete.dart

+  // - Matching and ranking on channel descriptions but only when the query
+  //   is present (but not an exact match, total-prefix, or word-boundary match)
+  //   in the channel name. This doesn't seem to be helpful in most cases,
+  //   because it is hard for a query to be present in the name (the way
+  //   mentioned before) and also present in the description.


Weird! Sounds like a bug.

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from 5a8171d to 6671cfa Compare October 9, 2025 21:00

sm-sayedi marked this pull request as ready for review October 9, 2025 21:07

gnprice added the maintainer review PR ready for review by Zulip maintainers label Oct 9, 2025

sm-sayedi requested a review from chrisbobbe October 9, 2025 21:07

sm-sayedi assigned chrisbobbe Oct 9, 2025

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from 6671cfa to fe25a32 Compare October 10, 2025 05:28

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from fe25a32 to 480e787 Compare October 11, 2025 12:00

sm-sayedi force-pushed the 124-channel-link-autocomplete branch 2 times, most recently from 6b2fb06 to 5072dce Compare October 21, 2025 19:38

chrisbobbe reviewed Oct 23, 2025

View reviewed changes

sm-sayedi mentioned this pull request Oct 24, 2025

Show channel description in channel link autocomplete #1945

Open

sm-sayedi force-pushed the 124-channel-link-autocomplete branch 2 times, most recently from 8cde339 to ea64c45 Compare October 24, 2025 19:42

sm-sayedi requested a review from chrisbobbe October 24, 2025 19:54

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from ea64c45 to d1abf20 Compare October 24, 2025 20:19

sm-sayedi mentioned this pull request Oct 29, 2025

stream/topic autocomplete (e.g. #**mobile**) #124

Open

chrisbobbe reviewed Oct 31, 2025

View reviewed changes

sm-sayedi mentioned this pull request Nov 3, 2025

After choosing an @-mention option, backspacing should reopen autocomplete #1967

Open

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from d1abf20 to acb6615 Compare November 3, 2025 09:57

sm-sayedi requested a review from chrisbobbe November 3, 2025 10:13

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from acb6615 to 513bc80 Compare November 3, 2025 15:43

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from 513bc80 to 6d28107 Compare November 4, 2025 05:04

chrisbobbe mentioned this pull request Nov 8, 2025

compose: Add a controller.dispose call that we forgot #1979

Merged

chrisbobbe reviewed Nov 8, 2025

View reviewed changes

sm-sayedi added 18 commits November 10, 2025 11:52

api: Add InitialSnapshot.maxChannelNameLength

fac4457

realm: Add RealmStore.maxChannelNameLength

fd4d629

Right now, this is useful in how far back from the cursor we look to find a channel-link autocomplete (actually any autocomplete) interaction in compose box.

api: Add ZulipStream.isRecentlyActive

ca7414b

In the following commits, this will be used as one of the criteria for sorting channels in channel link autocomplete.

autocomplete: Call EmojiAutocompleteView.reassemble where needed

5563131

emoji: Add EmojiAutocompleteView.dispose

89a8a40

So to call AutocompleteViewManager.unregisterEmojiAutocomplete.

autocomplete [nfc]: Introduce `AutocompleteViewManager._autocompleteV…

b7c5278

…iews` This set replaces the three sets of different `AutocompleteView` subclasses, simplifying the code.

store: Call AutocompleteViewManager.handleUserGroupRemove/UpdateEvent

9696df9

These two methods were introduced but never called.

autocomplete test [nfc]: Use MarkedTextParse as the return type of pa…

3543b71

…rseMarkedText This was first added in 0886948, but seems to have been accidentally removed in 046ceab.

autocomplete: Identify when the user intends a channel link autocomplete

267f7b0

For this commit we temporarily intercept the query at the AutocompleteField widget, to avoid invoking the widgets that are still unimplemented. That lets us defer those widgets' logic to a separate later commit.

autocomplete [nfc]: Add a TODO(zulip#1967) for ignoring starting "**"…

b58f34c

… after "#"

autocomplete test: Make setupToComposeInput accept channels param

5510c30

internal_link [nfc]: Factor out constructing fragment in its own method

a6d0938

This will make it easy to use the fragment string in several other places, such as in the next commits where we need to create a fallback markdown link for a channel.

compose: Introduce fallbackMarkdownLink function

831ccf1

channel: Finish channel link autocomplete for compose box

b654dd1

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=7952-30060&t=YfdW2W1p4ROsq9db-0 Fixes-partly: zulip#124

sm-sayedi force-pushed the 124-channel-link-autocomplete branch from d4e2458 to b654dd1 Compare November 10, 2025 17:02

sm-sayedi requested a review from chrisbobbe November 10, 2025 17:14

chrisbobbe reviewed Nov 10, 2025

View reviewed changes

chrisbobbe assigned gnprice and unassigned chrisbobbe Nov 10, 2025

chrisbobbe requested a review from gnprice November 10, 2025 19:13

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Nov 10, 2025

gnprice reviewed Nov 11, 2025

View reviewed changes

		case ChannelPropertyName.isRecentlyActive:
		return value as bool?;

		case ChannelPropertyName.isRecentlyActive:
		assert(value is bool?);

		// Similar reasoning as in _mentionIntentRegex.
		const before = r'(?<=^\|\s\|\p{Punctuation})';

-      padding: EdgeInsetsDirectional.fromSTEB(12, 4, 10, 4),
-      child: Row(spacing: 10, children: [
-        SizedBox.square(dimension: 24,
-          child: Icon(size: 18, color: iconColor, icon)),
+      padding: EdgeInsetsDirectional.fromSTEB(4, 4, 8, 4),
+      child: Row(spacing: 6, children: [
+        SizedBox.square(dimension: 36,
+          child: Center(
+            child: Icon(size: 18, color: iconColor, icon))),

		/// A channel with undefined weekly traffic (`null`) is put after the channel
		/// with a weekly traffic defined (even if it is zero).

Channel link autocomplete #1902

Are you sure you want to change the base?

Channel link autocomplete #1902

Conversation

sm-sayedi commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Oct 9, 2025

Uh oh!

sm-sayedi commented Oct 9, 2025

Uh oh!

sm-sayedi commented Oct 10, 2025

Uh oh!

sm-sayedi commented Oct 21, 2025

Uh oh!

chrisbobbe commented Oct 22, 2025

Uh oh!

chrisbobbe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

sm-sayedi commented Oct 24, 2025

Uh oh!

chrisbobbe 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!

sm-sayedi commented Oct 8, 2025 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

sm-sayedi commented Nov 3, 2025 •

edited

Loading