internal_links: Add "/" before fragment identifier #874

satyam372 · 2024-08-09T16:07:16Z

Fixes: #845

This adds a '/ ' after hostname. when this Url is posted in Zulip message it gets linkified.

PIG208 · 2024-08-12T18:01:05Z

test/model/internal_link_test.dart

+    for (final testCase in testCases) {
+      final String inputUrl = testCase.$1;
+      final String expectedUrl = testCase.$2;


nit:

Suggested change

for (final testCase in testCases) {

final String inputUrl = testCase.$1;

final String expectedUrl = testCase.$2;

for (final (inputUrl, expectedUrl) in testCases) {

I can see that this helper parallels testExpectedNarrows, where the same pattern is also applicable.

PIG208 · 2024-08-12T18:01:42Z

lib/model/internal_link.dart

@@ -98,7 +98,7 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
    fragment.write('/near/$nearMessageId');
  }

-  return store.realmUrl.replace(fragment: fragment.toString());


96593ba internal_links [nit]: Add path: '/' to store.realmUrl.replace for proper Url handling
I think we don't ever use [nit] in commit summaries, so we can just drop that.

We don't have tests in this commit, and the tests added in the other commit do not fail if we remove this change. We need to do some testing here.

PIG208 · 2024-08-12T18:10:39Z

test/model/internal_link_test.dart

@@ -54,6 +54,37 @@ void main() {
    }


3010732 internal_link_test: Create 'parseInternalLink full url cases' test

We usually use internal_link test for changes like this. git log --oneline -- path/to/file is a way to look for predecessors.

PIG208 · 2024-08-12T18:13:57Z

test/model/internal_link_test.dart

+      ('https://example.com/#narrow/stream/check', 'https://example.com/#narrow/stream/check'),
+      ('https://example.com/#narrow/stream/3-mobile/topic/topic1', 'https://example.com/#narrow/stream/3-mobile/topic/topic1'),
+      ('https://another.com/#narrow/stream/mobile', 'https://another.com/#narrow/stream/mobile'),
+      ('https://example.com/#narrow/stream/check/topic/test', 'https://example.com/#narrow/stream/check/topic/test'),


I'm not sure if I understand the purpose of these test cases, as the input and the expected output are the same.

PIG208 · 2024-08-12T18:14:35Z

Thanks for the PR! Left some comments.

PIG208

Thanks for the update! Went over this and left some comments. One other suggestion is to add the tests in the same commit that makes the change.

PIG208 · 2024-09-11T22:09:50Z

lib/model/internal_link.dart

@@ -59,7 +59,7 @@ String? decodeHashComponent(String str) {
 Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
  // TODO(server-7)
  final apiNarrow = resolveDmElements(
-    narrow.apiEncode(), store.connection.zulipFeatureLevel!);
+  narrow.apiEncode(), store.connection.zulipFeatureLevel!);


Looks like the indentation change here is unintentional.

PIG208 · 2024-09-11T22:12:04Z

lib/model/internal_link.dart

@@ -95,8 +95,11 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
  if (nearMessageId != null) {
    fragment.write('/near/$nearMessageId');
  }
+  final realmUrlWithSlash = store.realmUrl.path.endsWith('/')
+      ? store.realmUrl
+      : store.realmUrl.replace(path: '${store.realmUrl.path}/');


nit:

Suggested change

: store.realmUrl.replace(path: '${store.realmUrl.path}/');

final realmUrlWithSlash = store.realmUrl.path.endsWith('/')

? store.realmUrl

: store.realmUrl.replace(path: '${store.realmUrl.path}/');

PIG208 · 2024-09-11T22:25:04Z

test/model/internal_link_test.dart

+      final messageLink = narrowLink(store, narrow, nearMessageId: 12345);
+      final urlString = messageLink.toString();
+      final fragmentIndex = urlString.indexOf('#');
+      expect(fragmentIndex, greaterThan(0));


We usually use checks for our tests. So this can be check(fragmentIndex).isGreaterThan(0); instead. This applies to the other checks here too.

PIG208 · 2024-09-11T22:30:56Z

test/model/internal_link_test.dart

+      eg.stream(streamId: 123, name: 'check'),
+    ];
+    test('narrowLink should generate the full URL correctly', () async {
+      final store = await setupStore(realmUrl: realmUrl, streams: streams);


This realmUrl is https://example.com/, which comes with a trailing /. So we might want to override this for testing.

PIG208 · 2024-09-11T22:40:19Z

test/model/internal_link_test.dart

+      expect(
+          urlString.startsWith('https://example.com/#narrow/stream/123-check/topic'),
+          isTrue
+      );


I think this test and the one before it can be combined into one. However, looking at this more, it might be simpler to just check if urlString equals the URL we expect (i.e.: 'https://example.com/#narrow/stream/123-check/topic').

Or, even better, we can add the test to test/widgets/action_sheet_test.dart, as a test similar to "copies message link to clipboard". This way we get to test it end-to-end. In this case, no change is needed for test/model/internal_link_test.dart.

PIG208 · 2024-09-11T22:42:18Z

lib/model/internal_link.dart

@@ -59,7 +59,7 @@ String? decodeHashComponent(String str) {
 Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {


Some nits on commit message:

updates -> Updates

...fragment identifier This... -> ...fragment identifier. This...

likified -> linkified

and an empty space between the paragraph and Fixes: #845.

PIG208

Thanks for the update! The implementation now looks good to me. Added some comments on the new test.

PIG208 · 2024-09-16T19:01:36Z

test/widgets/action_sheet_test.dart

@@ -553,6 +553,21 @@ void main() {
    });
  });

+
+  group('narrowLink URL generation tests', () {
+


Extra empty line here and above the group.

PIG208 · 2024-09-16T19:13:36Z

test/widgets/action_sheet_test.dart

+    testWidgets('Full Url Testing', (tester) async {
+      final message = eg.streamMessage();
+      final narrow = TopicNarrow.ofMessage(message);
+      await setupToMessageActionSheet(tester, message: message, narrow: narrow);


For this test, it matters for us that the store.realmUrl used does not end with a slash. However, the default value is "https://chat.example/" (see eg.account). We should find a way to override that when setting up the store.

PIG208 · 2024-09-16T19:18:33Z

test/widgets/action_sheet_test.dart

+      final narrow = TopicNarrow.ofMessage(message);
+      await setupToMessageActionSheet(tester, message: message, narrow: narrow);
+      final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+      final generatedLink = narrowLink(store, narrow, nearMessageId: message.id).toString();


To make this end-to-end, let's generate the link through tapCopyMessageLinkButton. We should move this test to the "CopyMessageLinkButton" group.

PIG208 · 2024-09-16T19:19:24Z

test/widgets/action_sheet_test.dart

+      const expectedPrefix = 'https://chat.example/#narrow/stream/123-stream-123/topic';
+      final truncatedLink = generatedLink.split('/example').first;
+      check(truncatedLink).equals(expectedPrefix);


nit:

Suggested change

const expectedPrefix = 'https://chat.example/#narrow/stream/123-stream-123/topic';

final truncatedLink = generatedLink.split('/example').first;

check(truncatedLink).equals(expectedPrefix);

check(generatedLink).startsWith('https://chat.example/#narrow/stream/123-stream-123/topic');

PIG208 · 2024-09-16T19:20:37Z

lib/model/internal_link.dart

@@ -96,7 +96,11 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
    fragment.write('/near/$nearMessageId');


Looks like I missed this in the last round. "Variable" in the commit summary is capitalized, but we should use "variable" instead.

satyam372 · 2024-09-19T20:01:16Z

test/widgets/action_sheet_test.dart

@@ -24,6 +24,7 @@ import '../api/fake_api.dart';
 import '../example_data.dart' as eg;
 import '../flutter_checks.dart';
 import '../model/binding.dart';
+import '../model/internal_link_test.dart';
 import '../model/test_store.dart';


Import internal_link_test.dart to use setupstore to generate link without an '/' before '#'.

satyam372 · 2024-09-19T20:06:34Z

test/widgets/action_sheet_test.dart

+      final expectedLink = narrowLink(store, narrow, nearMessageId: message.id).toString();
+      final generatedLink = (await Clipboard.getData('text/plain'))?.text ?? '';
+      final normalizedExpectedLink = expectedLink.replaceAll('123-unknown', '123-stream-123');
+      check(generatedLink).equals(normalizedExpectedLink);


before normalization, the test was failing due to mismatch.

Hmm, interesting. Looks like that could be a result of not having the stream in store. This is because expectedLink and generatedLink are generated from separate instances of PerAccountStore:
expectedLink uses the store created with setupStore above;
generatedLink uses the store set up from setupToMessageActionSheet.

How about adding an optional parameter Account? account to setupToMessageActionSheet, which can be used to override the realmUrl?

satyam372 · 2024-09-23T18:31:36Z

PTAL @PIG208

PIG208

Thanks for the revision! Left a comment regarding the output mismatch issue.

PIG208 · 2024-09-23T18:41:23Z

test/widgets/action_sheet_test.dart

+      final expectedLink = narrowLink(store, narrow, nearMessageId: message.id).toString();
+      final generatedLink = (await Clipboard.getData('text/plain'))?.text ?? '';
+      final normalizedExpectedLink = expectedLink.replaceAll('123-unknown', '123-stream-123');
+      check(generatedLink).equals(normalizedExpectedLink);


Hmm, interesting. Looks like that could be a result of not having the stream in store. This is because expectedLink and generatedLink are generated from separate instances of PerAccountStore:
expectedLink uses the store created with setupStore above;
generatedLink uses the store set up from setupToMessageActionSheet.

How about adding an optional parameter Account? account to setupToMessageActionSheet, which can be used to override the realmUrl?

satyam372 · 2024-09-26T19:31:31Z

test/widgets/action_sheet_test.dart

@@ -38,11 +38,12 @@ late FakeApiConnection connection;
 Future<void> setupToMessageActionSheet(WidgetTester tester, {
  required Message message,
  required Narrow narrow,
+  Account? account,
 }) async {


Add parameter Account so that we can override realmUrl to produce Url without '/' before '#'.

PIG208

LGTM overall! Some small comments, and I going to mark this for integration review.

PIG208 · 2024-09-27T21:12:57Z

test/widgets/action_sheet_test.dart

+    testWidgets('Full Url Testing', (tester) async {
+      final message = eg.streamMessage();
+      final narrow = TopicNarrow.ofMessage(message);
+      await setupToMessageActionSheet(tester, message: message, narrow: narrow);


We shouldn't need to setup the action sheet again. We did that at line 566.

PIG208 · 2024-09-27T21:14:13Z

test/widgets/action_sheet_test.dart

+      await tapCopyMessageLinkButton(tester);
+      await tester.pump(Duration.zero);


nit: Move this to just before the line where we define expectedLink, so the reader gets the contextual information about where the clipboard data comes from

PIG208 · 2024-09-27T21:21:22Z

test/widgets/action_sheet_test.dart

+      final customAccount = eg.selfAccount.copyWith(id: 2, realmUrl: Uri.parse('https://chat.example'));
+      await setupToMessageActionSheet(tester, message: message, narrow: narrow, account: customAccount);
+      final customAccountStore = await testBinding.globalStore.perAccount(customAccount.id);
+
+      final expectedLink = narrowLink(customAccountStore, narrow, nearMessageId: message.id).toString();


These lines are too long. Try to wrap them so that they are under 80 columns wide.

For example:

- await setupToMessageActionSheet(tester, message: message, narrow: narrow, account: customAccount); + await setupToMessageActionSheet( + tester, message: message, narrow: narrow, account: customAccount);

PIG208 · 2024-09-30T21:57:15Z

test/widgets/action_sheet_test.dart

@@ -60,8 +63,8 @@ Future<void> setupToMessageActionSheet(WidgetTester tester, {
    historyLimited: false,
    messages: [message],
  ).toJson());
-
-  await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
+  await tester.pumpAndSettle();


Looks like this can be removed, unless I missed something?

gnprice

Thanks @satyam372 for the contribution, and @PIG208 for the previous reviews!

Initial comments from me below, in addition to @PIG208's remaining comments above.

This PR also lacks the kind of test that was specified in the issue:

as part of a PR fixing this issue, we should also add a few tests that include, verbatim, the full URL they expect as the output.

It will need to add those.

gnprice · 2024-10-01T04:29:21Z

lib/model/internal_link.dart

+  final realmUrlWithSlash = store.realmUrl.path.endsWith('/')
+      ? store.realmUrl
+      : store.realmUrl.replace(path: '${store.realmUrl.path}/');


This logic seems too broad. If the realm URL looks like https://chat.example/foo, we don't have a reason to turn that to https://chat.example/foo/.

Unless https://chat.example/foo#narrow/etc fails to linkify in a Zulip message, that is. Does it?

Yes, I tested some URLs and Here are the results:

In a browser: The browser automatically adds a / before the #.

In Zulip Flutter application: When I paste the URL, it does not linkify and is not clickable.
Based on this behavior, I have added a / before the #.

The question is about what Zulip does, though, when you put such a URL into a message. That's what this issue #845 is about:

This is still a valid URL, I believe — but it looks funny, and more concretely it means that if you post it in a Zulip message it doesn't get linkified.

So I tested that just now. The answer is that it does linkify:
https://chat.zulip.org/#narrow/stream/7-test-here/topic/linkification/near/1957680

That is, if you send a message with this text:
https://chat.example/foo#narrow/etc
it becomes a link.

That's the same as what happens if you send a message with this text:
https://chat.example/#narrow/etc

By contrast if you send a message with this text:
https://chat.example#narrow/etc
it doesn't become a link. That's the reason #845 is an issue.

In Zulip Flutter application: When I paste the URL, it does not linkify and is not clickable.

This is puzzling because it's different from what I observed in my test just now. (I used the web app to send that test message — but the message content gets turned to HTML on the server, so it doesn't matter what client is used.)

Can you post a link to a test message where you found the URL did not linkify? That would make it possible to pin down what variable is controlling the difference in behavior.

The link https://chat.example/foo#narrow/etc does linkify for me. So we probably don't need the slash in this case.

I think the fix for this will just be using startswith instead of endsWith, so that we don't process https://chat.example/foo, but still handle https://chat.example#anything.

These are something that are good to have tests for.

Or even simpler than startsWith — check path.isNotEmpty. I believe a URL path will always either be empty or start with /.

gnprice · 2024-10-01T04:30:04Z

test/widgets/action_sheet_test.dart

-  await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
-  final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
+  account ??= eg.selfAccount;
+  final initialSnapshot = eg.initialSnapshot(zulipFeatureLevel: account.zulipFeatureLevel);


Does this need to start mentioning zulipFeatureLevel? If not, keep the story simpler by leaving it out.

gnprice · 2024-10-01T04:34:53Z

test/widgets/action_sheet_test.dart

@@ -616,4 +634,4 @@ void main() {
      check(mockSharePlus.sharedString).isNull();
    });
  });
-}
+}


(Not an empty line — the problem is that every line should end with a newline, and this commit is editing the file's last line by removing its terminating newline. Previous discussion: #534 (comment) )

satyam372 · 2024-10-01T18:32:52Z

Thanks @gnprice and @PIG208 for the review. just a small question.
In the current tests, we are dynamically generating and verifying the URL.
Should we be comparing the generated URL directly against a hardcoded, literal full URL string within the test (e.g., including specific realm URLs and message IDs), rather than dynamically building it?

gnprice · 2024-10-04T21:37:28Z

Yes. As the issue says:

we should also add a few tests that include, verbatim, the full URL they expect as the output.

For further questions about how to complete the PR, I recommend a chat thread in #mobile-dev-help. That may get a faster response.

satyam372 · 2024-10-11T07:25:24Z

PTAL @PIG208

PIG208

Thanks for the revision! I know there has been many review comments, but we are making some progress. I think the structure of the test looks fine. You can go to #mobile-dev-help anytime if you run into issues.

PIG208 · 2024-10-11T22:45:38Z

test/widgets/action_sheet_test.dart

@@ -616,4 +634,4 @@ void main() {
      check(mockSharePlus.sharedString).isNull();
    });
  });
-}
+}


The newline still seems to be missing. Could you add it there?

PIG208 · 2024-10-11T23:00:47Z

lib/model/internal_link.dart

+  final realmUrlWithSlash = store.realmUrl.path.endsWith('/')
+      ? store.realmUrl
+      : store.realmUrl.replace(path: '${store.realmUrl.path}/');


The link https://chat.example/foo#narrow/etc does linkify for me. So we probably don't need the slash in this case.

I think the fix for this will just be using startswith instead of endsWith, so that we don't process https://chat.example/foo, but still handle https://chat.example#anything.

These are something that are good to have tests for.

PIG208 · 2024-10-11T23:03:21Z

test/widgets/action_sheet_test.dart

+
+      await tapCopyMessageLinkButton(tester);
+      await tester.pump(Duration.zero);
+      const expectedLink = 'https://chat.zulip.org/#narrow/stream/123-stream-123/topic/example.20topic.20341/near/13579';


These expectedLinks look good to me.

PIG208 · 2024-10-11T23:03:34Z

test/widgets/action_sheet_test.dart

+    testWidgets('Copies message link with verbatim, full URL output', (tester) async {
+      final stream = eg.stream(streamId: 123, name: 'stream 123');
+      final message = eg.streamMessage(
+          id: 13579, stream: stream, topic: 'example topic 341');


nit: indentation, and some other places in this file.

PIG208 · 2024-10-11T23:04:13Z

test/widgets/action_sheet_test.dart

+  });
+
+  group('Message link verbatim URL tests', () {
+


nit: we can remove the line here.

PIG208 · 2024-10-11T23:08:34Z

test/widgets/action_sheet_test.dart

+          id: 98765, stream: stream, topic: 'Whats up? 123!');
+      final narrow = TopicNarrow.ofMessage(message);
+      final account = eg.selfAccount.copyWith(
+          realmUrl: Uri.parse('https://chat.zulip.org'));


It should be fine to reuse the same set of stream and topic, because we are more interested in the variants of realmUrl. In addition to this one, let's also try something like https://chat.zulip.org/path and https://chat.zulip.org/. Since most of these tests will be similar, extracting a helper will be simplify the code.

PIG208 · 2024-10-11T23:09:25Z

test/widgets/action_sheet_test.dart

@@ -551,6 +554,93 @@ void main() {
      final expectedLink = narrowLink(store, narrow, nearMessageId: message.id).toString();
      check(await Clipboard.getData('text/plain')).isNotNull().text.equals(expectedLink);
    });
+
+    testWidgets('Full Url Testing', (tester) async {


Now that we have the new tests, we should be able to remove this one.

Updates the URL construction to include a trailing slash before the fragment identifier. This ensures that the URL is properly formatted and makes the url linkified. Co-authored-by: Greg Price <[email protected]> Fixes: zulip#845

satyam372 · 2024-10-17T19:21:10Z

PTAL @PIG208

satyam372 · 2024-10-22T06:26:15Z

Please review @gnprice.

gnprice

Looking again at this PR, the commit still doesn't make it at all clear to me that it makes things work the way we want them to. When sending a PR to Zulip (or really anywhere), it's important to make the code clear and the explanations clear. See the Zulip guide on submitting reviewable PRs:
https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html

This has been a long thread, because @PIG208 and I have already spent a substantial amount of time working with you to try to improve the PR. But given the PR's current state, it would still take a substantial amount more work to get it to be something that we can merge.

It's particularly concerning that even the latest revision introduces new problems that should have been easy to catch if you re-read your changes and think through what they're doing. (I've flagged two such problems below.) As our guide says, it's important to review your own work before asking others to do so.

Closing this PR because it's not on track to become something we can merge with a reasonable amount of effort from the core team.

Based on this thread and #1013, I think you don't yet have the skills to learn to contribute productively in the Zulip project. You're welcome to try again after at least 6 months.

I recommend you spend time working on other projects, and reading a lot of code, especially reading code in well-maintained codebases like Zulip or upstream Flutter (i.e., any of the main repositories at https://github.com/zulip or https://github.com/flutter). This advice from our contributing guide is applicable to a wide variety of codebases:

Set up a development environment following the instructions in the project README.

Start reading recent commits to see the code we’re writing. Use either a graphical Git viewer like gitk, or git log -p with the “secret” to reading its output.

Pick some of the code that appears in those Git commits and that looks interesting. Use your IDE to visit that code and to navigate to related code, reading to see how it works and how the codebase is organized.

I recommend also investing effort in clearly explaining why your changes work correctly. As our guide to reviewable PRs says, that begins with clearly reasoning through that question yourself:

When you write code, you should make sure that you understand why it works as intended. This is the foundation for being able to explain your proposed changes to others.

Even in personal projects where nobody else is reading your explanations, taking the time to clearly write them down will help you write higher-quality code. It will also be useful practice for contributing to projects like Zulip where there are other people you need to explain your changes to.

gnprice · 2024-10-22T21:50:00Z

lib/model/internal_link.dart

+  final realmUrlWithSlash = store.realmUrl.path.isNotEmpty
+      ? store.realmUrl
+      : store.realmUrl.replace(path: '${store.realmUrl.path}/');


In the "else" case here, the path will have to be empty. So this expression is unnecessarily complicated.

gnprice · 2024-10-22T21:50:53Z

test/widgets/action_sheet_test.dart

+    testWidgets('Copies link with custom realm path', (tester) async {
+      await testMessageLink(
+      tester: tester,
+      realmUrl: 'https://chat.example/foo',


This is difficult to read because it's missing indentation:

Suggested change

testWidgets('Copies link with custom realm path', (tester) async {

await testMessageLink(

tester: tester,

realmUrl: 'https://chat.example/foo',

testWidgets('Copies link with custom realm path', (tester) async {

await testMessageLink(

tester: tester,

realmUrl: 'https://chat.example/foo',

The same is true of each of the other test cases.

gnprice added the maintainer review PR ready for review by Zulip maintainers label Aug 11, 2024

gnprice assigned PIG208 Aug 11, 2024

gnprice requested a review from PIG208 August 11, 2024 22:25

gnprice unassigned PIG208 Aug 11, 2024

PIG208 reviewed Aug 12, 2024

View reviewed changes

satyam372 marked this pull request as draft August 15, 2024 06:14

satyam372 force-pushed the add-trailing-slash branch 2 times, most recently from 231512d to e617802 Compare September 11, 2024 18:14

PIG208 self-requested a review September 11, 2024 18:15

satyam372 marked this pull request as ready for review September 11, 2024 19:10

PIG208 reviewed Sep 11, 2024

View reviewed changes

gnprice assigned PIG208 Sep 12, 2024

satyam372 marked this pull request as draft September 13, 2024 05:13

satyam372 force-pushed the add-trailing-slash branch 3 times, most recently from 74b44f3 to 0bc2e39 Compare September 15, 2024 20:35

satyam372 marked this pull request as ready for review September 15, 2024 20:57

PIG208 reviewed Sep 16, 2024

View reviewed changes

satyam372 marked this pull request as draft September 16, 2024 19:47

satyam372 force-pushed the add-trailing-slash branch from 0bc2e39 to 9a69094 Compare September 19, 2024 19:33

satyam372 commented Sep 19, 2024

View reviewed changes

satyam372 marked this pull request as ready for review September 19, 2024 20:07

PIG208 reviewed Sep 23, 2024

View reviewed changes

satyam372 marked this pull request as draft September 23, 2024 19:14

satyam372 force-pushed the add-trailing-slash branch 2 times, most recently from bc34132 to 51a53fb Compare September 26, 2024 19:07

satyam372 commented Sep 26, 2024

View reviewed changes

satyam372 marked this pull request as ready for review September 27, 2024 20:33

PIG208 reviewed Sep 27, 2024

View reviewed changes

PIG208 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 Sep 27, 2024

satyam372 force-pushed the add-trailing-slash branch 2 times, most recently from 11d5c2f to 3169ff4 Compare September 28, 2024 10:58

PIG208 reviewed Sep 30, 2024

View reviewed changes

gnprice reviewed Oct 1, 2024

View reviewed changes

satyam372 marked this pull request as draft October 2, 2024 17:56

satyam372 force-pushed the add-trailing-slash branch from 3169ff4 to 68e8325 Compare October 8, 2024 09:55

satyam372 requested a review from PIG208 October 8, 2024 10:15

satyam372 marked this pull request as ready for review October 8, 2024 10:15

PIG208 reviewed Oct 11, 2024

View reviewed changes

satyam372 marked this pull request as draft October 12, 2024 10:09

satyam372 force-pushed the add-trailing-slash branch 2 times, most recently from ba6047e to d4733c3 Compare October 14, 2024 06:23

satyam372 force-pushed the add-trailing-slash branch from d4733c3 to 6c884ee Compare October 14, 2024 06:40

satyam372 requested a review from PIG208 October 14, 2024 07:01

satyam372 marked this pull request as ready for review October 14, 2024 07:01

gnprice reviewed Oct 22, 2024

View reviewed changes

gnprice closed this Oct 22, 2024

gnprice mentioned this pull request Nov 7, 2024

Include / after hostname in generated links #845

Closed

PIG208 mentioned this pull request Nov 13, 2024

internal_link: Always include a "/" after hostname #1059

Merged

		@@ -59,7 +59,7 @@ String? decodeHashComponent(String str) {
		Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {

		@@ -96,7 +96,11 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) {
		fragment.write('/near/$nearMessageId');

		await tapCopyMessageLinkButton(tester);
		await tester.pump(Duration.zero);

internal_links: Add "/" before fragment identifier #874

internal_links: Add "/" before fragment identifier #874

Conversation

satyam372 commented Aug 9, 2024

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

PIG208 commented Aug 12, 2024

PIG208 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

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Sep 16, 2024 • 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

satyam372 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyam372 commented Sep 23, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 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

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 Oct 8, 2024 • edited by PIG208 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyam372 commented Oct 1, 2024

gnprice commented Oct 4, 2024

satyam372 commented Oct 11, 2024

PIG208 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

satyam372 commented Oct 17, 2024

satyam372 commented Oct 22, 2024

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

PIG208 Sep 16, 2024 •

edited

Loading

satyam372 Sep 19, 2024 •

edited

Loading

gnprice Oct 8, 2024 •

edited by PIG208

Loading

gnprice Oct 12, 2024 •

edited

Loading