Handle Zulip internal links #305

sirpengi · 2023-09-15T16:17:54Z

Adds support to recognize internal UI links, such as links to other streams or topics.

sirpengi · 2023-09-15T16:21:33Z

The NarrowOperator enum in lib/model/internal_link.dart should be a private class (It should be _NarrowOperator) but then the JsonEnum annotations would create a const _$_NarrowOperatorEnumMap that the analyzer complains about:

info: The constant name '_$_NarrowOperatorEnumMap' isn't a lowerCamelCase identifier. (constant_identifier_names at [zulip] lib/model/internal_link.g.dart:11)

gnprice · 2023-09-21T04:46:06Z

Thanks @sirpengi!

Since I was on vacation the past few days, and then preoccupied today with that Sentry-induced crash bug in zulip-mobile that we fixed with v27.213, here's just some initial comments from a quick skim, for things that will help me review more efficiently when I sit down to a full review.

Some of this code is transcribed from zulip-mobile. Please mention the specifics of that in the commit messages — which files within zulip-mobile, and which code in the commit (like which functions, or "the test cases", or whatever the case may be).
I seem to recall there was a test case in zulip-mobile that we discovered in our call last week didn't actually test what it appeared to, and that functionality has actually never worked. (IIRC it was some form of link that was already historical in 2017, when the people who were working on zulip-mobile at that point wrote that test and the corresponding code.) What test case was that?

Apart from that one, do these tests include all the test cases from zulip-mobile for parsing internal links?

sirpengi · 2023-09-21T14:09:44Z

Thanks @sirpengi!

Since I was on vacation the past few days, and then preoccupied today with that Sentry-induced crash bug in zulip-mobile that we fixed with v27.213, here's just some initial comments from a quick skim, for things that will help me review more efficiently when I sit down to a full review.
* Some of this code is transcribed from zulip-mobile. Please mention the specifics of that in the commit messages — which files within zulip-mobile, and which code in the commit (like which functions, or "the test cases", or whatever the case may be).

Completed, and ready for you to review!

* I seem to recall there was a test case in zulip-mobile that we discovered in our call last week didn't actually test what it appeared to, and that functionality has actually never worked. (IIRC it was some form of link that was already historical in 2017, when the people who were working on zulip-mobile at that point wrote that test and the corresponding code.) What test case was that?

This was a DM narrow with an email address (e.g. /#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom) that instead created a narrow object with NaNs as operands. The change was also noted in my commit message.

  Apart from that one, do these tests include all the test cases from zulip-mobile for parsing internal links?

Yes, all the test cases from mobile are also here, except for yet unimplemented narrows (is/starred, is/mentioned, etc), and some test strings were "translated" (mobile uses "jest" for some stream names and I switched it to "check" in this case).

gnprice

Thanks!

Here's a full review up through a first tranche: the three prep commits, and then the "parse internal links" commit through the lib code and part of the tests.

Left for a future round are the second half of those tests (the "Get Narrow" group), and the final "handle internal links" commit.

gnprice · 2023-09-21T19:28:13Z

lib/model/compose.dart

@@ -101,82 +101,6 @@ String wrapWithBacktickFence({required String content, String? infoString}) {
  return resultBuffer.toString();
 }

-const _hashReplacements = {


nits in commit message:

lib [nfc]: refactor narrowLink out of compose into a new internal_link package

"lib" is extremely broad as a prefix; "model" is better

perhaps better yet, use the same prefix "internal_link" as for subsequent changes to the new file

use sentence case, capitalizing the first word after the colon

"package" in the Dart world means the thing that has a pubspec.yaml and a lib/ directory, so e.g. our whole repo is one package; the noun here is "library", or simply "file"

for the verb, can say "move" instead of "refactor" — it's shorter (useful in these summary lines) and also more specific

gnprice · 2023-09-21T19:33:21Z

lib/model/narrow.dart

-    return DmNarrow(
-      allRecipientIds: [...conversation.userIds, selfUserId]..sort(),
+    return DmNarrow.withUsers(
+      conversation.userIds,


This version is less efficient, creating a Set before turning it into a List. The root cause is that it's giving up information which this method has, namely that the self-user isn't in the provided list.

The Narrow constructors are an area where I'd like to remain fairly paranoid about performance, because there are some situations where we may construct a fair number of them. So let's keep the more efficient version.

gnprice · 2023-09-21T19:35:08Z

lib/model/narrow.dart

+    return DmNarrow.withUsers(
+      [userId],
+      selfUserId: selfUserId,
+    );


Here any performance difference is probably smaller — just the creation of that singleton list. Still, the old version of withUser isn't materially more complicated than this (and in a way it's simpler, because the reader has one fewer layer of indirection to follow to see what's happening). So I think best to keep the direct version.

gnprice · 2023-09-21T19:45:29Z

lib/model/internal_link.dart

+///
+/// Returns `null` if any of the operator/operand pairs are invalid.
+///
+/// Since narrow links can combine operators in ways our Narrow type can't


nit: in dartdoc, use cross-references:

Suggested change

/// Since narrow links can combine operators in ways our Narrow type can't

/// Since narrow links can combine operators in ways our [Narrow] type can't

This makes a followable link in the IDE.

(That feature doesn't seem to exist for jsdoc, at least not in VS Code, which is why we didn't do something similar in the jsdoc of getNarrowFromNarrowLink.)

gnprice · 2023-09-21T19:46:52Z

lib/model/internal_link.dart

+/// The passed `url` must appear to be a link to a Zulip narrow on the given
+/// `realm`.


"given realm" doesn't fit here because there's no identifier realm

(in the jsdoc this is adapted from, realm was the name of a parameter)

gnprice · 2023-09-21T20:52:44Z

test/model/internal_link_test.dart

+  }
+
+  group('Validate narrow links', () {
+    test('is valid narrow link', () async {


This name is kind of confusing — many of these aren't in fact valid narrow links, and the test is that our code should agree.

In the zulip-mobile tests, the name says "isNarrowLink" because that's the name of the function being tested.

Once we have individual test calls for the individual test cases, this one probably doesn't need to exist; the outer group does the job.

gnprice · 2023-09-21T20:52:45Z

test/model/internal_link_test.dart

+  }
+
+  group('Validate narrow links', () {
+    test('is valid narrow link', () async {


This name is kind of confusing — many of these aren't in fact valid narrow links, and the test is that our code should agree.

In the zulip-mobile tests, the name says "isNarrowLink" because that's the name of the function being tested.

gnprice · 2023-09-21T20:57:33Z

test/model/internal_link_test.dart

+          Uri.parse('http://on_realm.example.com:444/#narrow/stream/check')),
+
+        // TODO: Add tests for IPv6.


There were some other tests like "punycodable host" — what became of them?

Including the commented-out test cases. I'd hope that in fact those might Just Work with Dart's Uri implementation; not sure.

gnprice · 2023-09-21T20:58:06Z

test/model/internal_link_test.dart

+
+        // TODO: Add tests for IPv6.
+
+        // These examples may seem weird, but a previous version accepted most of them.


"previous version" should clarify it means in zulip-mobile — otherwise it's confusing because this is the first version here

gnprice · 2023-09-21T21:03:56Z

test/model/internal_link_test.dart

+  await testBinding.globalStore.add(account, initialSnapshot);
+  return await testBinding.globalStore.perAccount(account.id);


We shouldn't need the global binding here; we're just passing stores around explicitly. See other test files in test/model/ for examples.

sirpengi · 2023-09-25T17:53:05Z

@gnprice ready for review again. I've refactored the test suite to match the zulip-mobile suite in order and made sure every test case was included (and the ones not supported are included but commented out with the appropriate TODO attached if available)

gnprice · 2023-09-25T18:48:32Z

build.yaml

@@ -4,5 +4,6 @@ targets:
      source_gen:combining_builder:
        options:
          ignore_for_file:
+            - constant_identifier_names


nit:

lint [nfc]: Suppress constant_identifier_names in generated files This prevents us from making private `JsonEnum` classes as the generated `EnumMap` does not follow the rule of being a lowerCamelCase identifier.

This sounds at first like it's describing what the commit does. (After all, lots of lint config changes are precisely to "prevent[] us from" doing something, making some kind of mistake.)

One way to disambiguate is to use the past tense when describing things that stopped happening with this commit. So "This was preventing us …", and "was triggering".

Separate commit-message nit: leave out "[nfc]"

This change is indeed NFC from the perspective of our app code, but the prefix already says it's about the linter; and it does change the behavior of the linter.

A "lint [nfc]:" change, by contrast, would be one that's refactoring our lint config itself. These aren't that common, but if you try git log --oneline --grep 'lint .nfc.:' in zulip-mobile you'll see a burst of them a few years ago, as our lint config there has a lot more to it.

Similarly a test-only change may say "test:", or "foo test:"; and a pure refactor of the tests will say "test [nfc]:" or "foo test [nfc]:". For examples, try the commands git log --stat -p --grep 'test:' and git log --stat -p --grep 'test .nfc.:'.

gnprice · 2023-09-25T19:00:08Z

lib/model/narrow.dart

@@ -168,6 +168,13 @@ class DmNarrow extends Narrow implements SendableNarrow {
    );
  }

+  factory DmNarrow.withUsers(List<int> userIds, {required int selfUserId}) {


nit in commit message:

Add DmNarrow.withUsers

Include a subject-area prefix: here narrow: would be good.

For discussion, see in the Zulip-wide Git style guide: https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-1
The examples there are all for the server and web app, but the principle is the same.

gnprice · 2023-09-25T19:07:09Z

lib/model/internal_link.dart

@@ -0,0 +1,282 @@
+
+import 'package:flutter/cupertino.dart';


nit: This import is broader than needed. In particular it's a widget library, and this file shouldn't need to know anything about widgets.

Android Studio has a habit of inserting this one when what you actually want can be found in a more base-layer library like flutter/foundation or flutter/widgets. (Perhaps because "cupertino" comes first alphabetically.)

gnprice · 2023-09-25T19:40:31Z

lib/model/internal_link.dart

+  if (url.hasScheme || url.hasAuthority || url.hasPort) {
+    // input that do not include scheme, host, and port
+    // are treated as on realm


It really worries me to be adding ad-hoc logic like this in the context of interpreting a URL. That's the sort of thing that creates security vulnerabilities: one system thinks the URL means one thing and refers to something trusted, but then passes the URL to a different system which interprets it as pointing to something else that in fact should not be trusted.

So any time we think there's a need for this sort of logic, we need to be very explicit about thinking through what we're actually trying to do, and why the logic we're writing will correctly do the thing we want and not have edge cases where it goes wrong.

In this instance, we're porting over code from zulip-mobile, and I don't think there's corresponding logic there. So I suspect we ultimately don't need this.

If I take this condition out, I see there are a couple of tests that fail, which I assume is related to why you added it. (And #305 (comment) sounds related too.) I recommend starting a chat thread in #mobile-team with your observations and debugging of what goes wrong without this condition; that'll be a better medium than GitHub for a debugging or "what's the right way to solve this problem" conversation.

Through our discussion we settled on running all input through realmUrl.resolve() so that they have an origin to report later. This matches the mobile behavior of running input through new URL() with a base.

Linking that chat discussion for cross-reference: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/URLs.20are.20tricky.20to.20parse/near/1647599

gnprice · 2023-09-25T19:59:26Z

lib/model/internal_link.dart

+
+  if (url.origin != store.account.realmUrl.origin) return null;
+
+  final (category, segments) = _getCategoryAndSegmentsFromFragment(url.fragment);


OK, works for me.

gnprice

Thanks for the revision!

This review covers the new revision of the same first tranche as before (#305 (review)) — so what remains is the tests starting at "General link parsing", and the last commit.

Part of this review is above as #305 (review) , because that's what GitHub does with pending comments when one replies in a subthread that's no longer visible in the "Files" tab.

gnprice · 2023-09-25T20:06:09Z

lib/model/internal_link.dart

+    final (operator, negated) = _parseOperator(segments[i]);
+    // TODO(#252): negated operators are not currently supported
+    // These should eventually return a SearchNarrow
+    if (negated == true) return null;


nit:

Suggested change

if (negated == true) return null;

if (negated) return null;

There's a lint rule for this, which apparently isn't enabled. I'll send a quick change enabling it. (→ #312)

gnprice · 2023-09-25T20:09:17Z

lib/model/internal_link.dart

+        try {
+          topic = decodeHashComponent(operand);
+        } on ArgumentError {
+          return null;
+        } on FormatException {
+          return null;
+        }


Hmm, what are the situations where this function will throw?

I feel like this should probably be encapsulated inside this decodeHashComponent function — the caller shouldn't have to know the names of exceptions it'll throw on unexpected input, and instead it should return null or something. But I'm also surprised that there are inputs it would reject.

This method relies on Uri.decodeComponent which can be easily made to throw when it encounters invalid percentage encoded values:

final cases = [ "%1", "%FF", ]; for (final input in cases) { try { Uri.decodeComponent(input); } catch (e) { print("$input: ${e.toString()}"); } } // %1: Invalid argument(s): Truncated URI // %FF: FormatException: Invalid UTF-8 byte (at offset 0)

I found the exceptional cases from digging in https://github.com/dart-lang/sdk/blob/main/sdk/lib/core/uri.dart#L3032

(But will take your note and refactor decodeHashComponent to handle these instead)

Cool, thanks for the examples. I guess that's the same behavior as JS's (or the Web platform's?) decodeURIComponent — from my browser console:

> decodeURIComponent('%1'); VM99:1 Uncaught URIError: URI malformed at decodeURIComponent (<anonymous>) at <anonymous>:1:1 > decodeURIComponent('%FF'); VM106:1 Uncaught URIError: URI malformed at decodeURIComponent (<anonymous>) at <anonymous>:1:1

Kind of messy that Uri.decodeComponent throws two different exception types there, but so it goes.

Anyway, should be encapsulated inside our decodeHashComponent helper rather than having each caller have to deal with it.

Oh and to complete your examples — here's the output:

%1: Invalid argument(s): Truncated URI %FF: FormatException: Invalid UTF-8 byte (at offset 0)

gnprice · 2023-09-25T20:11:52Z

lib/model/internal_link.dart

+int? _parseStreamOperand(String operand, PerAccountStore store) {
+  // "New" (2018) format: ${stream_id}-${stream_name} .
+  final match = RegExp(r'^(\d+)(?:-.*)?$').firstMatch(operand);
+  final newFormatStreamId = (match != null) ? int.parse(match.group(1)!, radix:10) : null;


nit: space after colon

gnprice · 2023-09-25T20:20:50Z

test/model/internal_link_test.dart

+  final account = Account(
+    id: 1001,
+    realmUrl: realmUrl,


This looks a lot like eg.selfAccount. Is there a difference that matters for these tests?

If so, it'd be good to be explicit. If not, then using the boring baseline test data is helpful for focusing the reader's attention on the meaningful parts of the test.

For these tests we need to create various accounts, sometimes with a custom realmUrl. I found an Account.copyWith that will make this clear what we're replacing outside the baseline test data.

gnprice · 2023-09-25T20:24:11Z

test/model/internal_link_test.dart

+  final store = PerAccountStore.fromInitialSnapshot(
+    account: account,
+    connection: FakeApiConnection.fromAccount(account),
+    initialSnapshot: initialSnapshot,
+  );


What we use in other test/model/ tests, with the exception of the tests for the store classes themselves, is eg.store(). Can we use that here?

gnprice · 2023-09-25T20:32:08Z

test/model/internal_link_test.dart

+      final String because = testCase.$2;
+      final Uri url = testCase.$3;
+      final Uri realmUrl = testCase.$4;
+      test(because, () async {


"because" is kind of a funny name for these strings — "description" seems more descriptive.

gnprice · 2023-09-25T20:32:40Z

test/model/internal_link_test.dart

+      test(because, () async {
+        final store = await setupStore(realmUrl: realmUrl, streams: streams);
+        final result = parseInternalLink(url, store);
+        check(because: because, result != null).equals(expected);


This "because" parameter should be redundant now that the test description has the same information.

gnprice · 2023-09-25T20:35:53Z

test/model/internal_link_test.dart

+
+      (true, 'with port',
+        Uri.parse('https://example.com:444/#narrow/stream/check'),
+        Uri.parse('https://example.com:444')),


Suggested change

Uri.parse('https://example.com:444')),

Uri.parse('https://example.com:444/')),

in order to agree with the realmUrl used for the other cases, as well as with the zulip-mobile test

gnprice · 2023-09-25T20:53:29Z

test/model/internal_link_test.dart

+      // Uri.parse performs percent-encoding of unicode domains:
+      // `Uri.parse('https://example.भारत/')` has an origin of
+      // `https://example.%E0%A4%AD%E0%A4%BE%E0%A4%B0%E0%A4%A4`


Welp, that's a bug.

Compare the behavior in a browser:

> new URL('https://example.भारत/').origin 'https://example.xn--h2brj9c'

The browser also matches the URL spec:
https://url.spec.whatwg.org/#host-parsing
where "running domain to ASCII" is the relevant step.

Browsing the Dart SDK's tracker, it looks like a good comment would be:

// Dart's [Uri] lacks IDNA or Punycode support: // https://github.com/dart-lang/sdk/issues/26284 // https://github.com/dart-lang/sdk/issues/29420

That way we're clear, at least in our notes in our own code, about what the behavior is supposed to be and what's a bug that's just out of our control.

gnprice · 2023-09-26T01:37:24Z

test/model/internal_link_test.dart

+      final String because = testCase.$2;
+      final Uri url = testCase.$3;
+      final Uri realmUrl = testCase.$4;
+      test(because, () async {


The zulip-mobile version has a bit more in the test description:

test(`${expected ? 'accept' : 'reject'} ${description}: ${url.toString()}`, () => {

I think the accept/reject part, especially, is helpful when you're looking at a list of these that failed, or passed, and trying to understand what they're saying.

sirpengi · 2023-10-04T15:11:04Z

@gnprice this is ready for review again!

gnprice · 2023-10-04T15:49:14Z

test/model/internal_link_test.dart

+      store = setupStore(realmUrl: realmUrl, streams: streams, users: users);
+    }
+    for (final testCase in testCases) {
+      final String path = testCase.$1;


This is called "path" here, but at the call sites it's either a fragment, or a trivial path / and then a fragment.

A good more-inclusive name might be urlString.

gnprice · 2023-10-04T15:50:45Z

test/model/internal_link_test.dart

+    List<ZulipStream>? streams,
+    PerAccountStore? store,
+    List<User>? users,
+  }) async {


No need for async — propagating on from #305 (comment)

I was able to remove async from the whole test

gnprice · 2023-10-04T15:54:38Z

test/model/internal_link_test.dart

+        Uri.parse('https://example.com:444/#narrow/stream/check'),
+        Uri.parse('https://example.com:444/')),
+
+      // In the following examples unicode domains will


"Unicode" is a proper name that is spelled with a capital U. This comment is in prose, so it should give things their standard capitalization.

gnprice · 2023-10-04T16:01:31Z

test/model/internal_link_test.dart

+      // In the following examples unicode domains will
+      // pass but punycode variants of unicode domains are not
+      // treated as matching because Dart's [Uri] currently


This description "In the following … matching" isn't quite right. Punycode is involved only in the first of the three failing examples; and it's not a variant, but an encoding.

I think the description can actually just be left out, though — the examples speak for themselves about what doesn't work. What this comment can add is the next few lines, which point to why they don't work.

(cf #305 (comment) )

gnprice · 2023-10-04T16:07:02Z

lib/model/internal_link.dart

+    // TODO(#252): negated operators are not currently supported
+    // These should eventually return a SearchNarrow
+    if (negated) return null;


Suggested change

// TODO(#252): negated operators are not currently supported

// These should eventually return a SearchNarrow

if (negated) return null;

// TODO(#252): negated operators are not currently supported;

// they should eventually return a SearchNarrow

if (negated) return null;

That is, I think the intent here is that the second comment line here is elaborating on the TODO, and "these" refers to negated operators; this would be a way of making that explicit. In particular the indentation groups it into the TODO.

If "these" is intended to mean something else then I guess it'd be good to make that explicit instead :-)

Hmm and then reading a bit more of the code around this: there are several other spots in this function where a similar comment applies, right?

I think the biggest one is probably the _NarrowOperator.unknown case — that's what covers plain search words, and sender:, and miscellaneous other search features. But also having multiple stream, topic, or DM elements; or having a DM element and a stream or topic element; and maybe some of the other return null cases too.

So the cleanest way to express this point may be to put a comment up somewhere like the top of the function. For example… oh, in fact we already have a discussion of this up at parseInternalLink. So can just add the TODO (with the handy issue-number reference) there, like:

/// This can also return null for some valid narrow links that our Narrow /// type *could* accurately represent. We should try to understand these /// better, but some kinds will be rare, even unheard-of: /// #narrow/stream/1-announce/stream/1-announce (duplicated operator) +// TODO(#252): handle all valid narrow links, returning a search narrow Narrow? parseInternalLink(Uri referenceUrl, PerAccountStore store) {

I think that covers it. If we set out to implement that TODO, we'll naturally go into _interpretNarrowSegments and find all the places where it's returning null instead of a narrow, and identify whether each one corresponds to a valid narrow link and so should start returning a search narrow instead.

gnprice · 2023-10-04T16:19:03Z

lib/model/internal_link.dart

+    return Uri.decodeComponent(str.replaceAll('.', '%'));
+  } on ArgumentError {
+    // as with '%1': Invalid argument(s): Truncated URI
+    return null;
+  } on FormatException {
+    // as with '%FF': FormatException: Invalid UTF-8 byte (at offset 0)


Cool, these comments are helpful — basically documenting in the code the things you learned at #305 (comment) , which aren't in upstream's documentation.

gnprice · 2023-10-04T16:22:49Z

lib/model/internal_link.dart

+  if (url.hasScheme || url.hasAuthority || url.hasPort) {
+    // input that do not include scheme, host, and port
+    // are treated as on realm


Linking that chat discussion for cross-reference: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/URLs.20are.20tricky.20to.20parse/near/1647599

gnprice

Thanks @sirpengi for the revision! I've gone through all the review threads from the last revision, and all the code except the last commit and internal_link_test.dart. This is definitely getting closer; just a few comments, below and above (#305 (review)), and most of them small.

For a subsequent review I'll read through the rest of the tests again, and the final commit "content: Handle internal links".

gnprice · 2023-10-04T16:31:28Z

lib/model/internal_link.dart

+/// Create a new `Uri` object in relation to a given realmUrl.
+///
+/// Returns `null` if `urlString` could not be parsed as a `Uri`.
+Uri? tryResolveToRealmUrl(String urlString, Uri realmUrl) {


This function's name says it tries to resolve the argument to a realm URL. That'd mean the output, if any, would be a realm URL.

But that's not what it does; it takes the realm URL as an input, and the output is something else — could be the realm URL again if urlString is e.g. "/", or could be some other URL on the realm if urlString is some other relative-URL string like "#narrow/", or could be a URL somewhere else entirely if urlString is an absolute-URL string like "https://elsewhere.example/".

Could say "on" instead of "to"; that'd be enough to make it accurate.

gnprice · 2023-10-04T16:33:26Z

lib/model/internal_link.dart

+/// Create a new `Uri` object in relation to a given realmUrl.
+///
+/// Returns `null` if `urlString` could not be parsed as a `Uri`.
+Uri? tryResolveToRealmUrl(String urlString, Uri realmUrl) {


As is, this is just called in one place, and it's not mentioned in any docs. Did you intend to refer to it somewhere, like as part of describing the requirements parseInternalLink makes on its caller?

Yes, I've now added a note in parseInternalLink to refer to this

gnprice · 2023-10-04T16:36:37Z

lib/model/internal_link.dart

+/// Decode a dot-encoded string.
+// The Zulip webapp uses this encoding in narrow-links:
+// https://github.com/zulip/zulip/blob/1577662a6/static/js/hash_util.js#L18-L25
+@visibleForTesting
+String? decodeHashComponent(String str) {


Suggested change

/// Decode a dot-encoded string.

// The Zulip webapp uses this encoding in narrow-links:

// https://github.com/zulip/zulip/blob/1577662a6/static/js/hash_util.js#L18-L25

@visibleForTesting

String? decodeHashComponent(String str) {

/// Decode a dot-encoded string; return null if malformed.

// The Zulip webapp uses this encoding in narrow-links:

// https://github.com/zulip/zulip/blob/1577662a6/static/js/hash_util.js#L18-L25

@visibleForTesting

String? decodeHashComponent(String str) {

gnprice · 2023-10-04T16:42:35Z

lib/model/internal_link.dart

+Narrow? parseInternalLink(Uri referenceUrl, PerAccountStore store) {
+  final url = store.account.realmUrl.resolveUri(referenceUrl);


Hmm interesting. Why the additional resolveUri call here?

This makes a significant difference in what the meaning of this function is. It definitely needs to be clearly reflected in the function's doc.

But also in the way this function gets called, it seems redundant — we already pass it a URL that's been resolved to an absolute URL.

It looks like there are no tests exercising this behavior, either — this diff passes all the tests:

Narrow? parseInternalLink(Uri referenceUrl, PerAccountStore store) { - final url = store.account.realmUrl.resolveUri(referenceUrl); + final url = referenceUrl;

So if we were to have this behavior, we would definitely want to have tests for it.

There was an oversight on my part, it should not be called again. Along with a doc change in #305 (comment) this line will be removed.

sirpengi · 2023-10-05T10:48:07Z

@gnprice ready again!

gnprice

Thanks @sirpengi! Reading these revisions, they all look good except one nit below.

Next I'll do the remaining reading mentioned above at #305 (review) :

For a subsequent review I'll read through the rest of the tests again, and the final commit "content: Handle internal links".

though I might not get to that today.

gnprice · 2023-10-06T00:55:50Z

lib/model/internal_link.dart

+/// Create a new `Uri` object in relation to a given realmUrl.
+///
+/// Returns `null` if `urlString` could not be parsed as a `Uri`.
+Uri? tryResolveOnRealmUrl(String urlString, Uri realmUrl) {


This commit message says:

internal_link: Add tryParseToRealmUrl

Should match the name that's in the code.

bump

(I guess it's now internal_link: Add tryParseOnRealmUrl but that still differs from the code :-) )

gnprice

Thanks again @sirpengi! I've now read through much of the remaining tests; comments below.

I haven't yet read through all of the tests closely, because this seems like enough review comments for one round. As you act on these comments, though, please do look at the rest of the file to see where similar patterns appear, and apply similar revisions there where appropriate.

I did skim through the rest of these tests, and I see they're mostly in a similar structure to the tests I read in this round — so I think once you apply these comments through the whole file, it should all be quite close to merge.

(NB there's also that one small comment just above at #305 (review) , as the first part of this round of review.)

gnprice · 2023-10-06T19:44:50Z

test/model/internal_link_test.dart

+      final String description = testCase.$2;
+      final Uri url = testCase.$3;
+      final Uri realmUrl = testCase.$4;
+      final String testName = '${expected ? 'accept': 'reject'} $description: ${url.toString()}';


nit: toString is redundant inside Dart string interpolation:

Suggested change

final String testName = '${expected ? 'accept': 'reject'} $description: ${url.toString()}';

final String testName = '${expected ? 'accept': 'reject'} $description: $url';

gnprice · 2023-10-06T19:45:54Z

test/model/internal_link_test.dart

+      final String testName = '${expected ? 'accept': 'reject'} $description: ${url.toString()}';
+      test(testName, () {


nit: and then this can be simplified by inlining testName

gnprice · 2023-10-06T19:58:30Z

test/model/internal_link_test.dart

+    test('/#narrow/stream/... returns expected StreamNarrow', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('/#narrow/stream/check', const StreamNarrow(1)),
+        ('/#narrow/stream/stream/', const StreamNarrow(5)),
+        ('/#narrow/stream/topic/', const StreamNarrow(123)),
+      ];
+      checkExpectedNarrows(testCases, store: store);


Same comment as at #305 (comment) in the original review.

Take a look in zulip-mobile with npx jest src/utils/__tests__/internalLinks-test.js --selectProjects ios, and you'll see the kind of informative output I mean. Here's part of it:

If some change breaks a scattered handful of those, the green checks turn to red X's, while the details get printed below; so you see exactly which failed and which still passed. Extremely handy for debugging.

(And although that command line looks a bit arcane, it's the same output as you'd get on tools/test if that's the only test file affected by your changes. There's also a Jest flag to force that detailed output even when more than one file is running.)

Here's the corresponding part of the output from this PR, with flutter test test/model/internal_link_test.dart -r expanded:

That'd be much less informative when something breaks.

I've found out that it's okay to have multiple layers of groups, so I've refactored the tests that each test case becomes its own test() call.

Great, thanks.

And yeah, in general, if there's something that was asked for (in a review, or in the issue description, etc.) that doesn't seem to have been possible to do, please be sure to flag it explicitly. That way we can discuss alternatives and tradeoffs — or perhaps someone will know a way it can be done after all.

gnprice · 2023-10-06T20:00:28Z

test/model/internal_link_test.dart

+    test('/#narrow/stream/... returns expected StreamNarrow', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('/#narrow/stream/check', const StreamNarrow(1)),
+        ('/#narrow/stream/stream/', const StreamNarrow(5)),
+        ('/#narrow/stream/topic/', const StreamNarrow(123)),
+      ];
+      checkExpectedNarrows(testCases, store: store);


Same comment as at #305 (comment) in the original review.

Take a look in zulip-mobile with npx jest src/utils/__tests__/internalLinks-test.js --selectProjects ios, and you'll see the kind of informative output I mean. Here's part of it:

If some change breaks a scattered handful of those, the green checks turn to red X's, while the details get printed below; so you see exactly which failed and which still passed. Extremely handy for debugging.

(And although that command line looks a bit arcane, it's the same output as you'd get on tools/test if that's the only test file affected by your changes. There's also a Jest flag to force that detailed output even when more than one file is running.)

Here's the corresponding part of the output from this PR, with flutter test test/model/internal_link_test.dart -r expanded:

That'd be much less informative when something breaks.

gnprice · 2023-10-06T20:02:44Z

test/model/internal_link_test.dart

+        ('/#narrow/stream/check/topic/test',
+          const TopicNarrow(1, 'test')),
+        ('/#narrow/stream/mobile/subject/topic/near/378333',
+          const TopicNarrow(3, 'topic')), // TODO(#82): near


These test cases don't need TODO lines — in general, the tests for some code are rarely a place that benefits from TODO comments for changes to make to that code. That's because when we go and make those changes (perhaps with the aid of TODO comments in the code-under-test itself), the tests will call attention to themselves naturally by breaking.

gnprice · 2023-10-06T20:24:12Z

test/model/internal_link_test.dart

+      check(decodeHashComponent('some.2Etext')).equals('some.text');
+
+      check(decodeHashComponent('na.C3.AFvet.C3.A9')).equals('naïveté');
+      check(decodeHashComponent('.C2.AF.5C_(.E3.83.84)_.2F.C2.AF')).equals('¯\\_(ツ)_/¯');


Can use the handy Dart feature of "raw" string literals:

Suggested change

check(decodeHashComponent('.C2.AF.5C_(.E3.83.84)_.2F.C2.AF')).equals('¯\\_(ツ)_/¯');

check(decodeHashComponent('.C2.AF.5C_(.E3.83.84)_.2F.C2.AF')).equals(r'¯\_(ツ)_/¯');

gnprice · 2023-10-06T20:30:16Z

test/model/internal_link_test.dart

+        ('/#narrow/stream/some_stream',
+          const StreamNarrow(1)),
+        ('/#narrow/stream/some.20stream',
+          const StreamNarrow(2)),


Can make these less noisy to read by saying const once up top, as const testCases = [.

Same in other places in this file.

gnprice · 2023-10-06T20:35:37Z

test/model/internal_link_test.dart

+        ('/#narrow/stream/some_stream/topic/some_topic',
+          const TopicNarrow(1, 'some_topic')),
+        ('/#narrow/stream/some_stream/topic/some.20topic',
+          const TopicNarrow(1, 'some topic')),


Then this is also a place where readability benefits from writing things in a more tabular layout. Like so:

const testCases = [ ('/#narrow/stream/some_stream', StreamNarrow(1)), ('/#narrow/stream/some.20stream', StreamNarrow(2)), ('/#narrow/stream/some.2Estream', StreamNarrow(3)), ('/#narrow/stream/some_stream/topic/some_topic', TopicNarrow(1, 'some_topic')), ('/#narrow/stream/some_stream/topic/some.20topic', TopicNarrow(1, 'some topic')), ('/#narrow/stream/some_stream/topic/some.2Etopic', TopicNarrow(1, 'some.topic')), ];

That makes it a lot easier for the eye to scan down the URLs, and see what's constant and what's changing there, and down the narrows ditto, without having the two very different-looking forms of data alternate and interfere with each other.

The same applies in a lot of other lists in this file. Generally I'd take this genre of list of data as a good reason to go to as wide as 100 columns, if that's what it takes to make the layout much more tabular like this.

(And if it's going over that, I'd go for other measures to cut down the width, like shortening the example strings from "some stream" to like "a b", in preference to giving up the tabular layout.)

In the corresponding zulip-mobile code it doesn't look like quite this kind of table, because we use Prettier there and it can't handle the intra-line spacing. But there are a lot of places like this:

expectStream('test-team', [testTeam], testTeam); expectStream('311', [numbers], numbers); expectStream('311-', [numbersHyphen], numbersHyphen); expectStream('311-help', [numbersPlus], numbersPlus); expectStream('--help', [dashdash], dashdash);

and that nearly tabular layout is a major motivation for writing those helpers like expectStream the way they are.

gnprice · 2023-10-06T23:02:37Z

test/model/internal_link_test.dart

+    test('basic', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('#narrow/stream/1-general', const StreamNarrow(1)),
+      ];
+      checkExpectedNarrows(testCases, store: store);
+    });
+
+    test('if stream not found, use stream ID anyway', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('#narrow/stream/123-topic', const StreamNarrow(123)),
+      ];
+      checkExpectedNarrows(testCases, store: store);
+    });


Compare these lines to the corresponding lines in the zulip-mobile tests:

test('basic', () => { expectStream(`${streamGeneral.stream_id}-general`, [streamGeneral], streamGeneral); }); test('if stream not found, use stream ID anyway', () => { expectStream(`${streamGeneral.stream_id}-general`, [], streamGeneral); });

In the latter, the two lines with the details of the tests are separated by 1 line of text and 2 nearly-blank lines. In this version, they're separated by 7 lines, of which 4 have text on them. That makes it a lot noisier for the eye to compare the two and see what the contrasts are. Similarly for the next few cases below.

For this sort of thing, it can really help a lot to make a small local helper function. That's what expectStream in the corresponding zulip-mobile code is doing.

gnprice · 2023-10-06T23:03:18Z

test/model/internal_link_test.dart

+    test('on malformed stream link: reject', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('#narrow/stream/-1', null),
+        ('#narrow/stream/1nonsense-general', null),
+        ('#narrow/stream/-general', null),


(Here it's OK that these are all in one test call, because that's not a regression from the zulip-mobile tests.)

sirpengi · 2023-10-09T08:51:13Z

@gnprice ready for review again. I've also in other areas tried to compact the tests as much as possible so that it's more legible in spirit with the comments provided.

gnprice · 2023-10-09T21:38:19Z

test/model/internal_link_test.dart

+    }
+  }
+
+  group('ParseInternalLink', () {


nit: being in camel case this looks like an identifier, not normal English; but we don't have any identifier by this name.

Meant to have been parseInternalLink

gnprice · 2023-10-09T21:40:47Z

test/model/internal_link_test.dart

+void main() {
+  final realmUrl = Uri.parse('https://example.com/');
+
+  checkExpectedNarrows(List<(String, Narrow?)> testCases, {


nit: since this now contains a test call, its name should say "test" rather than "check" — that convention helps when reading callers, for seeing the nesting of test calls

(I see the corresponding test file in zulip-mobile doesn't get that right, oops.)

gnprice · 2023-10-09T21:45:06Z

test/model/internal_link_test.dart

+      (true, 'with port', Uri.parse('https://example.com:444/'),
+        Uri.parse('https://example.com:444/#narrow/stream/check')),
+
+      // Dart's [Uri] currently lacks IDNA or Punycode support:
+      //   https://github.com/dart-lang/sdk/issues/26284
+      //   https://github.com/dart-lang/sdk/issues/29420
+
+      // (true, 'same domain, punycoded host', Uri.parse('https://example.भारत/'),
+      //   Uri.parse('https://example.xn--h2brj9c/#narrow/stream/check')), // FAILS
+
+      (true, 'punycodable host', Uri.parse('https://example.भारत/'),
+        Uri.parse('https://example.भारत/#narrow/stream/check')),
+
+      // (true, 'same domain, IDNA-mappable', Uri.parse('https://example.com'),
+      //   Uri.parse('https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/check')), // FAILS
+
+      (true, 'ipv4 address', Uri.parse('http://192.168.0.1/'),
+        Uri.parse('http://192.168.0.1/#narrow/stream/check')),
+
+      // (true, 'same IPv4 address, IDNA-mappable', Uri.parse('http://192.168.0.1/')),
+      //   Uri.parse('http://１𝟗𝟚。①⁶🯸．₀｡𝟭/#narrow/stream/check')), // FAILS


I think these are all more readable the way they were (which is how we have them in the zulip-mobile tests): the action is all happening in the authorities (the hosts + ports), particularly in comparing the realm URL's authority to the input URL's authority. So having those two aligned with each other is best for that.

gnprice · 2023-10-09T21:49:43Z

test/model/internal_link_test.dart

+      final testCases = [
+        ('/#narrow/stream/name/topic/',           null),
+        ('/#narrow/stream/name/unknown/operand/', null),


Suggested change

final testCases = [

('/#narrow/stream/name/topic/', null),

('/#narrow/stream/name/unknown/operand/', null),

final testCases = [

('/#narrow/stream/name/topic/', null), // missing operand

('/#narrow/stream/name/unknown/operand/', null), // unknown operator

(like the comments in the zulip-mobile version)

gnprice · 2023-10-09T22:00:00Z

test/model/internal_link_test.dart

+    test('/#narrow/stream/... returns expected StreamNarrow', () {
+      final store = setupStore(realmUrl: realmUrl, streams: streams);
+      final testCases = [
+        ('/#narrow/stream/check', const StreamNarrow(1)),
+        ('/#narrow/stream/stream/', const StreamNarrow(5)),
+        ('/#narrow/stream/topic/', const StreamNarrow(123)),
+      ];
+      checkExpectedNarrows(testCases, store: store);


Great, thanks.

And yeah, in general, if there's something that was asked for (in a review, or in the issue description, etc.) that doesn't seem to have been possible to do, please be sure to flag it explicitly. That way we can discuss alternatives and tradeoffs — or perhaps someone will know a way it can be done after all.

gnprice

Thanks for the revision! As I hoped after the last review, this is now getting quite close to merge. I've read through the whole test file, and just have the handful of comments below and above (#305 (review)), all small.

Because this PR thread has gotten long, let's break the last commit:
f6ab5e7 content: Handle internal links
out to a separate followup PR. That'll be a much shorter thread, I think, but probably not totally trivial, so best to start it fresh.

gnprice · 2023-10-09T22:15:39Z

test/model/internal_link_test.dart

+      final Uri url = realmUrl.resolve(urlString);
+      final Narrow? expected = testCase.$2;
+      test(urlString, () {
+        check(parseInternalLink(url, store!)).equals(expected);


This test helper is violating the contract prescribed in the function's docs:

/// A [Narrow] from a given URL, on `store`'s realm. /// /// `url` must already be passed through [tryResolveOnRealmUrl].

We should not do that — it undermines the value of the test.

gnprice · 2023-10-09T22:20:44Z

test/model/internal_link_test.dart

+    group('on old stream link, without realm info', () {
+      final stream = eg.stream(name: 'example');
+      final testCases = [
+        ('/#narrow/stream/${stream.name}/', StreamNarrow(stream.streamId)),
+        ('#narrow/stream/${stream.name}/',  StreamNarrow(stream.streamId)),


The description says "without realm info", but these don't have any less info about the realm than the tests above do.

In the corresponding zulip-mobile tests, the difference is that the other group has absolute URL strings like https://example.com/#narrow/stream/jest, while this has relative URL strings. That's already a fairly unnecessary distinction because the test helper there ultimately does new URL(url, 'https://example.com') before passing the URL to the code under test; the distinction dates from when the underlying code under test actually took a URL string instead of a URL object.

I think these two cases don't add anything not covered in the group above, so we can just drop this one.

gnprice · 2023-10-09T22:23:54Z

test/model/internal_link_test.dart

+      final stream = eg.stream(name: "example");
+
+      group('basic', () {
+        String mkUrl(operand) {
+          return '#narrow/stream/${stream.streamId}-general/topic/$operand';


This is a bit incongruous: the stream's name is "example" but the URL says "general". Typically the URL would accurately reflect the stream name. Our tests should generally use representative data except when the test is specifically about some way of being non-representative, so these should agree.

(And other tests above cover the possibility that the URL doesn't reflect the stream name.)

gnprice · 2023-10-09T22:27:29Z

test/model/internal_link_test.dart

+      group('on old topic link, without realm info', () {
+        final testCases = [
+          ('/#narrow/stream/${stream.name}/topic/topic', TopicNarrow(stream.streamId, 'topic')),
+          ('#narrow/stream/${stream.name}/topic/topic',  TopicNarrow(stream.streamId, 'topic')),


Similar to the stream case above, this description doesn't really fit, and this pair of cases can just be dropped.

gnprice · 2023-10-09T22:27:52Z

test/model/internal_link_test.dart

+          (mkUrl('(no.20topic)'), TopicNarrow(stream.streamId, '(no topic)')),
+          (mkUrl('google.2Ecom'), TopicNarrow(stream.streamId, 'google.com')),
+          (mkUrl('google.com'),   null),
+          (mkUrl('topic.20name'), TopicNarrow(stream.streamId, 'topic name')),


Neat, I appreciate the compactness here.

gnprice · 2023-10-09T22:31:07Z

test/model/internal_link_test.dart

+      group('on group PM link including self', () {
+        checkExpectedDmNarrow('#narrow/dm/1,2,${eg.selfUser.userId}-group');


The zulip-mobile version has a comment that's important context for understanding this test:

Suggested change

group('on group PM link including self', () {

checkExpectedDmNarrow('#narrow/dm/1,2,${eg.selfUser.userId}-group');

group('on group PM link including self', () {

// The webapp doesn't generate these, but best to handle them anyway.

checkExpectedDmNarrow('#narrow/dm/1,2,${eg.selfUser.userId}-group');

sirpengi · 2023-10-11T12:26:45Z

@gnprice ready to go again! I have split off the extra integration commit and will open another pr after this

gnprice

Thanks @sirpengi for the revision! All looks great now except three small comments below — after those are fixed, this should be all ready to merge.

Please go ahead and send the followup PR too, as the branch that includes these changes and then that one further commit. Just mention in the PR description that it's on top of this PR.

gnprice · 2023-10-11T21:13:59Z

test/model/internal_link_test.dart

+      testExpectedDmNarrow(String testCase) {
+        final expectedNarrow = DmNarrow.withUsers([1, 2],
+          selfUserId: eg.selfUser.userId);
+        testExpectedNarrows([(testCase, expectedNarrow)], users: [


Hmm, this looks wrong — it's a "test" function inside another "test" function.

… Oh, aha. This is a function definition, not a function call, of testExpectedDmNarrow. So that nesting is fine, then.

Let's give this an explicit return type (of void). That's good for function definitions in general because it makes the expectations explicit; and then I guess another benefit I hadn't previously appreciated is that it makes them more visually distinct from function calls.

Applied to the same to testExpectedStreamNarrow above this

gnprice · 2023-10-11T21:19:06Z

test/model/internal_link_test.dart

+      // (true, 'same domain, IDNA-mappable', Uri.parse('https://example.com'),
+      //   'https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/check'), // FAILS


nit: this one is flipped around compared to the others

gnprice · 2023-10-11T21:23:27Z

lib/model/internal_link.dart

+/// Create a new `Uri` object in relation to a given realmUrl.
+///
+/// Returns `null` if `urlString` could not be parsed as a `Uri`.
+Uri? tryResolveOnRealmUrl(String urlString, Uri realmUrl) {


bump

(I guess it's now internal_link: Add tryParseOnRealmUrl but that still differs from the code :-) )

…al_link file

This was preventing us from making private `JsonEnum` classes as the generated `EnumMap` does not follow the rule of being a lowerCamelCase identifier. For example, `_Foo` generates `_$_FooEnumMap` and was triggering an analyzer complaint.

sirpengi · 2023-10-12T17:04:49Z

@gnprice good to go again!

sirpengi · 2023-10-12T18:09:50Z

For reference, the trailing commit is in #318

gnprice

Thanks! Couple of spots remaining from the last review; see below.

And thanks for sending #318 — I'll take a look at that next.

gnprice · 2023-10-12T21:16:08Z

test/model/internal_link_test.dart

+void main() {
+  final realmUrl = Uri.parse('https://example.com/');
+
+  testExpectedNarrows(List<(String, Narrow?)> testCases, {


Same thing as at #305 (comment) .

gnprice · 2023-10-12T21:17:58Z

test/model/internal_link_test.dart

+      // (true, 'same domain, IDNA-mappable'
+      //   'https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/check'),
+      //   Uri.parse('https://example.com'), // FAILS


This version doesn't parse if you uncomment it.

Looking at the neighboring commented-out failures, they don't either (but more subtly) — there's a stray close-paren.

All of them had a parse issue :(
I'll make sure to be more diligent about also refactoring comments

The core process on parsing internal links (here in `lib/model/internal_link.dart`) relied heavily on the existing code in the Zulip mobile app - from `src/utils/internalLinks.js`. In fact the `_parseStreamOperand` function here is a line for line port in order to capture the same semantics when processing streams. Where the implementation differs is this new process is less restrictive on the order of operator/operand pairs: supporting `#narrow/topic/_/stream_` where mobile only accepted `#narrow/stream/_/topic/_`. Also, the mobile implementation accepted as valid narrows DM operators with an email address as the operand (`#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom`) but created an invalid narrow object (with NaNs for targets) whereas this implementation rejects them as invalid narrows. Likewise the test cases are also taken from the mobile code (`src/utils/__tests__/internalLinks-test.js`) and replicated here, save for the special narrow types (`#narrow/is/starred`) which are not yet implemented. Also, the "without realm info" cases were removed as they were made moot with every test case being passed through `tryResolveOnRealmUrl` (the mobile cases were also passed through `new Url()` with a base).

sirpengi · 2023-10-13T08:41:27Z

@gnprice ready again

gnprice · 2023-10-13T22:04:26Z

Thanks again @sirpengi for all the revisions!

This now all looks good — merging.

sirpengi requested review from gnprice and chrisbobbe September 15, 2023 16:17

sirpengi force-pushed the pr-internal-links branch from 87df3a1 to a70af4f Compare September 21, 2023 13:56

gnprice reviewed Sep 21, 2023

View reviewed changes

sirpengi force-pushed the pr-internal-links branch 4 times, most recently from 6d858c8 to 4622f63 Compare September 25, 2023 17:52

gnprice reviewed Sep 25, 2023

View reviewed changes

gnprice reviewed Sep 26, 2023

View reviewed changes

This was referenced Sep 26, 2023

lint: Enable no_literal_bool_comparisons #312

Merged

Localization sweep #306

Merged

sirpengi force-pushed the pr-internal-links branch from 4622f63 to 64d0b5f Compare October 4, 2023 15:07

sirpengi force-pushed the pr-internal-links branch from 64d0b5f to 6b4fc9a Compare October 4, 2023 15:24

gnprice reviewed Oct 4, 2023

View reviewed changes

sirpengi force-pushed the pr-internal-links branch from 6b4fc9a to 811e4ae Compare October 5, 2023 12:16

gnprice reviewed Oct 6, 2023

View reviewed changes

sirpengi force-pushed the pr-internal-links branch from 811e4ae to f6ab5e7 Compare October 9, 2023 09:13

gnprice reviewed Oct 9, 2023

View reviewed changes

sirpengi force-pushed the pr-internal-links branch from f6ab5e7 to a9eaad6 Compare October 11, 2023 11:34

sirpengi force-pushed the pr-internal-links branch 2 times, most recently from b7511f2 to 22d23d2 Compare October 11, 2023 12:03

sirpengi force-pushed the pr-internal-links branch from 22d23d2 to b50a779 Compare October 11, 2023 17:22

gnprice reviewed Oct 11, 2023

View reviewed changes

sirpengi added 5 commits October 12, 2023 17:44

internal_link [nfc]: Move narrowLink out of compose into a new intern…

2f944a0

…al_link file

store: Add streamsByName mapping

c65bab8

narrow: Add DmNarrow.withUsers

664f4a5

test: Add optional Account and InitialSnapshot to store() factory

2cf1b36

sirpengi force-pushed the pr-internal-links branch from b50a779 to 18c21fc Compare October 12, 2023 16:56

sirpengi added 2 commits October 12, 2023 18:01

internal_link: Add tryResolveOnRealmUrl

9a5ec69

internal_link: Add decodeHashComponent function

4beb7bb

sirpengi force-pushed the pr-internal-links branch from 18c21fc to c15a579 Compare October 12, 2023 17:01

sirpengi mentioned this pull request Oct 12, 2023

Integrate internal links in message contents #318

Merged

gnprice reviewed Oct 12, 2023

View reviewed changes

sirpengi force-pushed the pr-internal-links branch from c15a579 to e3a2dcc Compare October 13, 2023 08:41

gnprice merged commit e3a2dcc into zulip:main Oct 13, 2023
1 check passed

sirpengi deleted the pr-internal-links branch January 23, 2024 14:17

gnprice mentioned this pull request Jul 30, 2024

lint: Enable always_declare_return_types; fix existing places they're missing #851

Merged

	/// Since narrow links can combine operators in ways our Narrow type can't
	/// Since narrow links can combine operators in ways our [Narrow] type can't

		/// The passed `url` must appear to be a link to a Zulip narrow on the given
		/// `realm`.

		Uri.parse('http://on_realm.example.com:444/#narrow/stream/check')),

		// TODO: Add tests for IPv6.


		// TODO: Add tests for IPv6.

		// These examples may seem weird, but a previous version accepted most of them.

		await testBinding.globalStore.add(account, initialSnapshot);
		return await testBinding.globalStore.perAccount(account.id);


		if (url.origin != store.account.realmUrl.origin) return null;

		final (category, segments) = _getCategoryAndSegmentsFromFragment(url.fragment);

	Uri.parse('https://example.com:444')),
	Uri.parse('https://example.com:444/')),

		Narrow? parseInternalLink(Uri referenceUrl, PerAccountStore store) {
		final url = store.account.realmUrl.resolveUri(referenceUrl);

	final String testName = '${expected ? 'accept': 'reject'} $description: ${url.toString()}';
	final String testName = '${expected ? 'accept': 'reject'} $description: $url';

		final String testName = '${expected ? 'accept': 'reject'} $description: ${url.toString()}';
		test(testName, () {

	check(decodeHashComponent('.C2.AF.5C_(.E3.83.84)_.2F.C2.AF')).equals('¯\\_(ツ)_/¯');
	check(decodeHashComponent('.C2.AF.5C_(.E3.83.84)_.2F.C2.AF')).equals(r'¯\_(ツ)_/¯');

		group('on group PM link including self', () {
		checkExpectedDmNarrow('#narrow/dm/1,2,${eg.selfUser.userId}-group');

		// (true, 'same domain, IDNA-mappable', Uri.parse('https://example.com'),
		// 'https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/check'), // FAILS

Handle Zulip internal links #305

Handle Zulip internal links #305

Conversation

sirpengi commented Sep 15, 2023 • edited Loading

sirpengi commented Sep 15, 2023

gnprice commented Sep 21, 2023

sirpengi commented Sep 21, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Oct 5, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Sep 15, 2023 •

edited

Loading

gnprice Sep 25, 2023 •

edited

Loading

sirpengi Sep 28, 2023 •

edited

Loading

sirpengi Oct 5, 2023 •

edited

Loading