msglist: Follow /with/ links through message moves

Fixes: zulip#1028

lib/api/model/narrow.dart

@@ -10,20 +10,30 @@ typedef ApiNarrow = List<ApiNarrowElement>;
 // TODO(server-7) remove [ApiNarrowDm] reference in dartdoc
 ApiNarrow resolveApiNarrowElements(ApiNarrow narrow, int zulipFeatureLevel) {
   bool hasDmElement = false;
+  bool hasWithElement = false;
   for (final element in narrow) {
     switch (element) {
-      case ApiNarrowDm(): hasDmElement = true;
+      case ApiNarrowDm():   hasDmElement = true;
+      case ApiNarrowWith(): hasWithElement = true;
       default:
     }
   }
-  if (!hasDmElement) return narrow;
+  if (!hasDmElement && !hasWithElement) return narrow;
 
   final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7)
-
-  return narrow.map((element) => switch (element) {
-    ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
-    _             => element,
-  }).toList();
+  final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9)
+
+  ApiNarrow result = narrow;
+  if (hasDmElement) {
+    result = narrow.map((element) => switch (element) {
+      ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
+      _             => element,
+    }).toList();
+  }
+  if (hasWithElement && !supportsOperatorWith) {
+    result.removeWhere((element) => element is ApiNarrowWith);
+  }
+  return result;
 }
 
 /// An element in the list representing a narrow in the Zulip API.
@@ -72,6 +82,22 @@ class ApiNarrowTopic extends ApiNarrowElement {
   );
 }
 
+/// An [ApiNarrowElement] with the 'with' operator.
+///
+/// If part of [ApiNarrow] use [resolveApiNarrowElements].
+class ApiNarrowWith extends ApiNarrowElement {
+  @override String get operator => 'with';
+
+  @override final int operand;
+
+  ApiNarrowWith(this.operand, {super.negated});
+
+  factory ApiNarrowWith.fromJson(Map<String, dynamic> json) => ApiNarrowWith(
+    json['operand'] as int,
+    negated: json['negated'] as bool? ?? false,
+  );
+}
+
 /// An [ApiNarrowElement] with the 'dm', or legacy 'pm-with', operator.
 ///
 /// An instance directly of this class must not be serialized with [jsonEncode],

lib/model/internal_link.dart

@@ -77,6 +77,8 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
         fragment.write('$streamId-$slugifiedName');
       case ApiNarrowTopic():
         fragment.write(_encodeHashComponent(element.operand));
+      case ApiNarrowWith():
+        fragment.write(element.operand.toString());
       case ApiNarrowDmModern():
         final suffix = element.operand.length >= 3 ? 'group' : 'dm';
         fragment.write('${element.operand.join(',')}-$suffix');
@@ -151,6 +153,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
 
   ApiNarrowStream? streamElement;
   ApiNarrowTopic? topicElement;
+  ApiNarrowWith? withElement;
   ApiNarrowDm? dmElement;
   Set<IsOperand> isElementOperands = {};
 
@@ -185,9 +188,12 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
         isElementOperands.add(IsOperand.fromRawString(operand));
 
       case _NarrowOperator.near: // TODO(#82): support for near
-      case _NarrowOperator.with_: // TODO(#683): support for with
         continue;
 
+      case _NarrowOperator.with_:
+        if (withElement != null) return null;
+        withElement = ApiNarrowWith(int.parse(operand, radix: 10));
+
       case _NarrowOperator.unknown:
         return null;
     }
@@ -216,7 +222,7 @@ Narrow? _interpretNarrowSegments(List<String> segments, PerAccountStore store) {
   } else if (streamElement != null) {
     final streamId = streamElement.operand;
     if (topicElement != null) {
-      return TopicNarrow(streamId, topicElement.operand);
+      return TopicNarrow(streamId, topicElement.operand, with_: withElement?.operand);
     } else {
       return ChannelNarrow(streamId);
     }

lib/model/message_list.dart

@@ -480,6 +480,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
       numAfter: 0,
     );
     if (this.generation > generation) return;
+    _adjustTopicPermalinkNarrow(narrow, result.messages.firstOrNull);
     store.reconcileMessages(result.messages);
     store.recentSenders.handleMessages(result.messages); // TODO(#824)
     for (final message in result.messages) {
@@ -493,11 +494,45 @@ class MessageListView with ChangeNotifier, _MessageSequence {
     notifyListeners();
   }
 
+  /// Update [narrow] for the result of a "with" narrow (topic permalink) fetch.
+  ///
+  /// To avoid an extra round trip, the server handles [ApiNarrowWith]
+  /// by returning results from the indicated message's current stream/topic
+  /// (if the user has access),
+  /// even if that differs from the narrow's stream/topic filters
+  /// because the message was moved.
+  ///
+  /// If such a "redirect" happened, this helper updates the stream and topic
+  /// in [narrow] to match the message's current conversation.
+  /// It also removes the "with" component from [narrow]
+  /// whether or not a redirect happened.
+  ///
+  /// See API doc:
+  ///   https://zulip.com/api/construct-narrow#message-ids
+  void _adjustTopicPermalinkNarrow(Narrow narrow_, Message? someFetchedMessageOrNull) {
+    if (narrow_ is! TopicNarrow || narrow_.with_ == null) return;
+
+    switch (someFetchedMessageOrNull) {
+      case null:
+        // This can't be a redirect; a redirect can't produce an empty result.
+        // (The server only redirects if the message is accessible to the user,
+        // and if it is, it'll appear in the result, making it non-empty.)
+        narrow = narrow_.sansWith();
+      case StreamMessage(:final streamId, :final topic):
+        narrow = TopicNarrow(streamId, topic);
+      case DmMessage(): // TODO(log)
+        assert(false);
+    }
+  }
+
   /// Fetch the next batch of older messages, if applicable.
   Future<void> fetchOlder() async {
     if (haveOldest) return;
     if (fetchingOlder) return;
     assert(fetched);
+    assert(narrow is! TopicNarrow
+      // We only intend to send "with" in [fetchInitial]; see there.
+      || (narrow as TopicNarrow).with_ == null);
     assert(messages.isNotEmpty);
     _fetchingOlder = true;
     _updateEndMarkers();

lib/model/narrow.dart

@@ -92,14 +92,17 @@ class ChannelNarrow extends Narrow {
 }
 
 class TopicNarrow extends Narrow implements SendableNarrow {
-  const TopicNarrow(this.streamId, this.topic);
+  const TopicNarrow(this.streamId, this.topic, {this.with_});
 
   factory TopicNarrow.ofMessage(StreamMessage message) {
     return TopicNarrow(message.streamId, message.topic);
   }
 
   final int streamId;
   final String topic;
+  final int? with_;
+
+  TopicNarrow sansWith() => TopicNarrow(streamId, topic);
 
   @override
   bool containsMessage(Message message) {
@@ -108,22 +111,26 @@ class TopicNarrow extends Narrow implements SendableNarrow {
   }
 
   @override
-  ApiNarrow apiEncode() => [ApiNarrowStream(streamId), ApiNarrowTopic(topic)];
+  ApiNarrow apiEncode() => [
+    ApiNarrowStream(streamId),
+    ApiNarrowTopic(topic),
+    if (with_ != null) ApiNarrowWith(with_!),
+  ];
 
   @override
   StreamDestination get destination => StreamDestination(streamId, topic);
 
   @override
-  String toString() => 'TopicNarrow($streamId, $topic)';
+  String toString() => 'TopicNarrow($streamId, $topic, with: $with_)';
 
   @override
   bool operator ==(Object other) {
     if (other is! TopicNarrow) return false;
-    return other.streamId == streamId && other.topic == topic;
+    return other.streamId == streamId && other.topic == topic && other.with_ == with_;
   }
 
   @override
-  int get hashCode => Object.hash('TopicNarrow', streamId, topic);
+  int get hashCode => Object.hash('TopicNarrow', streamId, topic, with_);
 }
 
 /// The narrow for a direct-message conversation.

test/api/route/messages_test.dart

@@ -188,6 +188,11 @@ void main() {
         {'operator': 'stream', 'operand': 12},
         {'operator': 'topic', 'operand': 'stuff'},
       ]));
+      checkNarrow(const TopicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
+        {'operator': 'stream', 'operand': 12},
+        {'operator': 'topic', 'operand': 'stuff'},
+        {'operator': 'with', 'operand': 1},
+      ]));
       checkNarrow(const MentionsNarrow().apiEncode(), jsonEncode([
         {'operator': 'is', 'operand': 'mentioned'},
       ]));
@@ -203,6 +208,11 @@ void main() {
       checkNarrow([ApiNarrowDm([123, 234])], jsonEncode([
         {'operator': 'pm-with', 'operand': [123, 234]},
       ]));
+      connection.zulipFeatureLevel = 270;
+      checkNarrow(const TopicNarrow(12, 'stuff', with_: 1).apiEncode(), jsonEncode([
+        {'operator': 'stream', 'operand': 12},
+        {'operator': 'topic', 'operand': 'stuff'},
+      ]));
       connection.zulipFeatureLevel = eg.futureZulipFeatureLevel;
     });
   });

test/model/internal_link_test.dart

@@ -173,12 +173,12 @@ void main() {
       const testCases = [
         ('/#narrow/stream/check/topic/test',                 TopicNarrow(1, 'test')),
         ('/#narrow/stream/mobile/subject/topic/near/378333', TopicNarrow(3, 'topic')),
-        ('/#narrow/stream/mobile/subject/topic/with/1',      TopicNarrow(3, 'topic')),
+        ('/#narrow/stream/mobile/subject/topic/with/1',      TopicNarrow(3, 'topic', with_: 1)),
         ('/#narrow/stream/mobile/topic/topic/',              TopicNarrow(3, 'topic')),
         ('/#narrow/stream/stream/topic/topic/near/1',        TopicNarrow(5, 'topic')),
-        ('/#narrow/stream/stream/topic/topic/with/22',       TopicNarrow(5, 'topic')),
+        ('/#narrow/stream/stream/topic/topic/with/22',       TopicNarrow(5, 'topic', with_: 22)),
         ('/#narrow/stream/stream/subject/topic/near/1',      TopicNarrow(5, 'topic')),
-        ('/#narrow/stream/stream/subject/topic/with/333',    TopicNarrow(5, 'topic')),
+        ('/#narrow/stream/stream/subject/topic/with/333',    TopicNarrow(5, 'topic', with_: 333)),
         ('/#narrow/stream/stream/subject/topic',             TopicNarrow(5, 'topic')),
       ];
       testExpectedNarrows(testCases, streams: streams);

test/model/message_list_test.dart

@@ -19,6 +19,7 @@ import '../example_data.dart' as eg;
 import '../fake_async.dart';
 import '../stdlib_checks.dart';
 import 'content_checks.dart';
+import 'narrow_checks.dart';
 import 'recent_senders_test.dart' as recent_senders_test;
 import 'test_store.dart';
 
@@ -95,6 +96,9 @@ void main() {
     final someStream = eg.stream();
     const someTopic = 'some topic';
 
+    final otherStream = eg.stream();
+    const otherTopic = 'other topic';
+
     final smokeCases = [
       (desc: 'CombinedFeedNarrow', narrow: const CombinedFeedNarrow(),
         generateMessages: (i) => eg.streamMessage()),
@@ -103,6 +107,18 @@ void main() {
         generateMessages: (i) => eg.streamMessage(
           stream: someStream,
           topic: someTopic)),
+      (desc: 'topic permalink, message was not moved',
+        narrow: TopicNarrow(someStream.streamId, someTopic, with_: 1),
+        generateMessages: (int i) => eg.streamMessage(
+          id: i == 0 ? 1 : null,
+          stream: someStream,
+          topic: someTopic)),
+      (desc: 'topic permalink, message was moved',
+        narrow: TopicNarrow(someStream.streamId, someTopic, with_: 1),
+        generateMessages: (int i) => eg.streamMessage(
+          id: i == 0 ? 1 : null,
+          stream: otherStream,
+          topic: otherTopic)),
     ];
 
     for (final case_ in smokeCases) {
@@ -177,6 +193,44 @@ void main() {
       check(model).messages.length.equals(1);
       recent_senders_test.checkMatchesMessages(store.recentSenders, messages);
     });
+
+    group('topic permalinks', () {
+      test('if redirect, we follow it and remove "with" element', () async {
+        await prepare(narrow: TopicNarrow(someStream.streamId, someTopic, with_: 1));
+        connection.prepare(json: newestResult(
+          foundOldest: false,
+          messages: [eg.streamMessage(id: 1, stream: otherStream, topic: otherTopic)],
+        ).toJson());
+        final fetchFuture = model.fetchInitial();
+        check(model).fetched.isFalse();
+
+        checkNotNotified();
+        await fetchFuture;
+        checkNotifiedOnce();
+        check(model).narrow.isA<TopicNarrow>()
+          ..streamId.equals(otherStream.streamId)
+          ..topic.equals(otherTopic)
+          ..with_.isNull();
+      });
+
+      test('if no redirect, we still remove "with" element', () async {
+        await prepare(narrow: TopicNarrow(someStream.streamId, someTopic, with_: 1));
+        connection.prepare(json: newestResult(
+          foundOldest: false,
+          messages: [eg.streamMessage(id: 1, stream: someStream, topic: someTopic)],
+        ).toJson());
+        final fetchFuture = model.fetchInitial();
+        check(model).fetched.isFalse();
+
+        checkNotNotified();
+        await fetchFuture;
+        checkNotifiedOnce();
+        check(model).narrow.isA<TopicNarrow>()
+          ..streamId.equals(someStream.streamId)
+          ..topic.equals(someTopic)
+          ..with_.isNull();
+      });
+    });
   });
 
   group('fetchOlder', () {

test/model/narrow_checks.dart

@@ -7,6 +7,12 @@ extension NarrowChecks on Subject<Narrow> {
   Subject<ApiNarrow> get apiEncode => has((x) => x.apiEncode(), 'apiEncode()');
 }
 
+extension TopicNarrowChecks on Subject<TopicNarrow> {
+  Subject<int> get streamId => has((x) => x.streamId, 'streamId');
+  Subject<String> get topic => has((x) => x.topic, 'topic');
+  Subject<int?> get with_ => has((x) => x.with_, 'with_');
+}
+
 extension DmNarrowChecks on Subject<DmNarrow> {
   Subject<List<int>> get allRecipientIds => has((x) => x.allRecipientIds, 'allRecipientIds');
   Subject<List<int>> get otherRecipientIds => has((x) => x.otherRecipientIds, 'otherRecipientIds');