"Copy link to topic" in topic action sheet #1762

sm-sayedi · 2025-07-28T20:46:27Z

Screenshots

Topic action sheet	Copied link pasted

Screen recording

Screen.Recording.2025-07-29.at.1.10.16.AM.mov

Fixes: #792

chrisbobbe

Thanks! This looks good; one small comment on the icon in the action sheet.

For the tests, let's include a test for the pre-271 behavior of omitting "/with/" (see resolveApiNarrowForServer).

chrisbobbe · 2025-07-28T21:25:36Z

lib/widgets/action_sheet.dart

+
+  final TopicNarrow narrow;
+
+  @override IconData get icon => Icons.link;


I notice that this is Icons.link instead of ZulipIcons.link. Do we have a custom icon we can use here?

…Ah I think we do. Here's the SVG: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&p=f&m=dev

I see it being used for "Copy link to channel" in Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6326-96513&m=dev

Would you add a prep commit that adds that new custom icon, and another commit that changes the one existing Icons.link (in "Copy link to message") to use the new icon?

alya · 2025-07-29T06:21:21Z

Would it be a follow-up to do the equivalent of zulip/zulip#29136?

sm-sayedi · 2025-07-29T08:05:50Z

Thanks @chrisbobbe for the review! New revision pushed, with tests included. Also added the new "link" icon, with screenshots updated. PTAL.

sm-sayedi · 2025-07-29T08:08:41Z

Would it be a follow-up to do the equivalent of zulip/zulip#29136?

Yeah, I think doing that as a follow-up would be the better option and will make it easy to get this PR merged sooner.

sm-sayedi · 2025-07-29T18:08:17Z

@chrisbobbe Pushed a new revision with the small changes you suggested in today's call.

chrisbobbe

Thanks, this looks great! Just small comments below, and I see you've already updated the lastOrNull commit as discussed on our call today.

chrisbobbe · 2025-07-29T18:03:27Z

lib/widgets/action_sheet.dart

-  if (optionButtons.isEmpty) {
-    // TODO(a11y): This case makes a no-op gesture handler; as a consequence,
-    //   we're presenting some UI (to people who use screen-reader software) as
-    //   though it offers a gesture interaction that it doesn't meaningfully
-    //   offer, which is confusing. The solution here is probably to remove this
-    //   is-empty case by having at least one button that's always present,
-    //   such as "copy link to topic".
-    return;
-  }


chrisbobbe · 2025-07-29T18:04:17Z

lib/widgets/action_sheet.dart

-  }
+  optionButtons.add(CopyTopicLinkButton(
+    narrow: TopicNarrow(channelId, topic,
+      // TODO(#1499): use the/a max message id of the topic


Is there still a TODO here? A previous commit started using lastOrNull of a list of message IDs.

Yeah, I think we will still need that TODO as the previous commit partially addressed the "max message id" case and did it only for when the topic action sheet is opened through the app bar title (it's still partial becuase the last message of topic list view is still not gauranteed to be the most recent message of topic). Then for other places where the sheet is opened from, like through the recipient header or in inbox view (which currently don't pass the max message id), the best option will be to get the most recent message id from a central place.

I think no further change is actually needed in this direction.

The recipient header uses the message ID of the message directly under it. When that's how the user got to the topic action sheet, having the /with/ operand point to that message seems fine and good — it means the link will continue to link to the conversation that that message belongs to, if things get split up.

The inbox uses the latest unread message ID in the conversation, right? When there are any unreads, the latest unread will almost always be the latest message. In the unusual situations where it isn't, and if the latest (but not unread) message later gets split to a different conversation from the latest unread, I'm not sure which one would be better for the link to point to.

chrisbobbe · 2025-07-29T18:14:07Z

test/widgets/action_sheet_test.dart

+      }
+
+      group('copies topic link to clipboard', () {
+        testWidgets('FL >= 271 -> link contains "with" operator', (tester) async {


nit: When there's a pair of tests for "legacy" vs. "current" behavior, I like to consider the "current" behavior as boring/default/unmarked, so for example naming the test like

testWidgets('copies message link to clipboard', (tester) async {

, and consider the "legacy" one as interesting/unusual/marked:

testWidgets('FL < 271 -> link doesn\'t contain "with" operator', (tester) async {

In the future, when we stop supporting servers with the legacy behavior, "message link" will always include "with" in its meaning, and when we clear out the backwards-compatibility code and its corresponding test, the remaining test won't need any changes to make it look natural.

chrisbobbe · 2025-07-29T18:20:19Z

test/widgets/action_sheet_test.dart

+          final expectedLink = narrowLink(store,
+            TopicNarrow(someChannel.streamId, TopicName(someTopic), with_: message.id)
+          ).toString();


How about a quick check(expectedLink.toString().contains('with')).isTrue(), and isFalse in the "FL < 271" test? Just to satisfy a reader that narrowLink handles the feature-level condition correctly and they don't need to check that detail in its implementation or tests.

(No need for a TODO to remove that check in the default/current test when clearing out backwards-compatibility code; it won't be as necessary in that future, but it's just one line so the cost is pretty minimal.)

sm-sayedi · 2025-07-29T18:56:03Z

Thanks @chrisbobbe for the review. New changes pushed.

chrisbobbe

Thanks, LGTM! Marking for Greg's review.

In the next commit(s), when creating topic permalinks, we try to use max message id of the topic, if possible. If we cannot find the max message id, then I think better to use a message id near to max message id. Related CZO discussions: https://chat.zulip.org/#narrow/channel/101-design/topic/redirects.20from.20near.20links/near/1460694 https://chat.zulip.org/#narrow/channel/101-design/topic/redirects.20from.20near.20links/near/1774403

gnprice

Thanks @sm-sayedi for taking care of this, and @chrisbobbe for the previous reviews! Just small comments below.

gnprice · 2025-07-29T20:28:34Z

test/widgets/action_sheet_test.dart

+        ).toString();
+        check(expectedLink.toString().contains('/with/')).isTrue();


nit: redundant toString

I'd probably remove the first one, and let expectedLink be a Uri object.

gnprice · 2025-07-29T20:30:08Z

test/widgets/action_sheet_test.dart

+        await prepare(channel: someChannel, topic: someTopic,
+          zulipFeatureLevel: 270);
+        await showFromRecipientHeader(tester, message: message);


Is there a reason this test uses showFromRecipientHeader while its counterpart above uses showFromAppBar?

Because these two are meant to demonstrate a contrast in the behavior depending on the feature level, it's best if their setup is all the same except for the feature level.

gnprice · 2025-07-29T20:51:15Z

lib/widgets/action_sheet.dart

-  }
+  optionButtons.add(CopyTopicLinkButton(
+    narrow: TopicNarrow(channelId, topic,
+      // TODO(#1499): use the/a max message id of the topic


I think no further change is actually needed in this direction.

The recipient header uses the message ID of the message directly under it. When that's how the user got to the topic action sheet, having the /with/ operand point to that message seems fine and good — it means the link will continue to link to the conversation that that message belongs to, if things get split up.

The inbox uses the latest unread message ID in the conversation, right? When there are any unreads, the latest unread will almost always be the latest message. In the unusual situations where it isn't, and if the latest (but not unread) message later gets split to a different conversation from the latest unread, I'm not sure which one would be better for the link to point to.

Fixes: zulip#792

sm-sayedi · 2025-07-29T21:57:58Z

Thanks @gnprice for the review. New revision pushed. PTAL.

gnprice · 2025-07-30T01:57:34Z

Thanks! Looks good; merging.

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Jul 28, 2025

sm-sayedi assigned chrisbobbe Jul 28, 2025

sm-sayedi requested a review from chrisbobbe July 28, 2025 20:47

chrisbobbe reviewed Jul 28, 2025

View reviewed changes

sm-sayedi force-pushed the 792-topic-link branch from fbae311 to 0ec065a Compare July 29, 2025 07:56

sm-sayedi requested a review from chrisbobbe July 29, 2025 08:06

sm-sayedi force-pushed the 792-topic-link branch from 0ec065a to abe1afb Compare July 29, 2025 10:13

sm-sayedi mentioned this pull request Jul 29, 2025

"Copy link to channel" in channel action sheet #1766

Merged

sm-sayedi force-pushed the 792-topic-link branch 2 times, most recently from e6fde8e to 2454342 Compare July 29, 2025 18:06

chrisbobbe reviewed Jul 29, 2025

View reviewed changes

sm-sayedi force-pushed the 792-topic-link branch from 2454342 to c82a5fb Compare July 29, 2025 18:55

sm-sayedi requested a review from chrisbobbe July 29, 2025 20:08

chrisbobbe approved these changes Jul 29, 2025

View reviewed changes

chrisbobbe requested a review from gnprice July 29, 2025 20:20

chrisbobbe assigned gnprice and unassigned chrisbobbe Jul 29, 2025

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

gnprice force-pushed the 792-topic-link branch from c82a5fb to 5b2474f Compare July 29, 2025 20:25

gnprice reviewed Jul 29, 2025

View reviewed changes

action_sheet: Add "Copy link to topic" button

8792876

Fixes: zulip#792

sm-sayedi force-pushed the 792-topic-link branch from 5b2474f to 8792876 Compare July 29, 2025 21:55

sm-sayedi requested a review from gnprice July 29, 2025 21:58

gnprice merged commit 8792876 into zulip:main Jul 30, 2025
1 check passed

laurynmm mentioned this pull request Jul 30, 2025

help: Update for mobile "Copy link to..." actions. zulip/zulip#35539

Merged

12 tasks


		final TopicNarrow narrow;

		@override IconData get icon => Icons.link;

		).toString();
		check(expectedLink.toString().contains('/with/')).isTrue();

"Copy link to topic" in topic action sheet #1762

"Copy link to topic" in topic action sheet #1762

Uh oh!

Conversation

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

Uh oh!

Screenshots

Screen recording

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Jul 29, 2025

Uh oh!

sm-sayedi commented Jul 29, 2025

Uh oh!

sm-sayedi commented Jul 29, 2025

Uh oh!

sm-sayedi commented Jul 29, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi commented Jul 29, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm-sayedi commented Jul 29, 2025

Uh oh!

Uh oh!

gnprice commented Jul 30, 2025

Uh oh!

Uh oh!

sm-sayedi commented Jul 28, 2025 •

edited

Loading

sm-sayedi Jul 29, 2025 •

edited

Loading