Show message content, sender, and timestamp in message action sheet #1624

chrisbobbe · 2025-06-24T08:29:42Z

Fixes #217.

We've had some complaints about not being able to see the timestamps of messages in a run of messages from the same sender, e.g.:

#mobile > Sent time not visible @ 💬

There is a "message sent" time shown in an upper right hand corner of the message. But when the messages are sent by bot, the time is only shown for the first message of the day. The old android app at least shown the time when tapped on the message, but I haven't found a way how to display the time here.

We have #457 for how to offer that in a really quick interaction. But this PR lets you get that information, and it aligns with a different part of the Figma, namely #217. As discussed: #mobile > Sent time not visible @ 💬

Light	Dark

This revision leaves the message completely interactable; you can open images in the lightbox, open and close spoilers, etc.; that seemed most straightforward to implement. It does not implement text selection of the content; that's requested in the Figma, and #10 is for that, and I've left a TODO for it.

chrisbobbe · 2025-06-24T08:33:03Z

This also mitigates #1142 because it gives a way to see one message by itself.

rajveermalviya

Thanks for working on this @chrisbobbe!

This looks and works great for me, one small comment below.

rajveermalviya · 2025-06-25T10:37:05Z

lib/widgets/action_sheet.dart

-                SizedBox(height: 8),
+                if (header != null)
+                  Flexible(
+                    flex: 1,


The figma design says:

Menu should grow until it fits the whole screen

But currently it doesn't, so maybe let's have a TODO for it.

Yeah; there's a note on #1077 that's relevant:

(TODO upstream issue)
Richer Flex (aka Row and Column), with more CSS flexbox features like flex-basis vs flex-grow vs flex-shrink. (This is one of the few things we miss from React Native: its Yoga layout engine has those features.)
We probably need something like this for: [etc.]

rajveermalviya · 2025-06-25T11:16:33Z

lib/widgets/action_sheet.dart

+            padding: const EdgeInsets.symmetric(horizontal: 16),
+            // TODO(#10) offer text selection; the Figma asks for it here:
+            //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-30210&m=dev
+            child: MessageContent(message: message, content: parseMessageContent(message))),


Couple of things I noticed with interactions in the message content:

There was no hero animation when opening an image.

Clicking on a narrow link, would open the narrow, but wouldn't close the action sheet (which we do for some for some cases).

Neither seem blocker for this feature though.

chrisbobbe · 2025-06-26T07:17:50Z

Thanks! I've added TODOs for all those points.

gnprice

Thanks for building this! Generally it all looks good; comments below.

gnprice · 2025-07-03T01:31:58Z

test/widgets/action_sheet_test.dart

-  await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
+  await tester.longPress(
+    find.descendant(
+      of: find.byType(MessageList),
+      matching: find.byType(MessageContent)),
+    warnIfMissed: false);


action_sheet test: Narrow a Finder (long-press for message action sheet) We're about to put a MessageContent in the action sheet itself, for #217.

This change seems fine but I'm a bit surprised it's needed. There's no action sheet in the tree at this point, right? This is the step that will cause one to appear.

Oh I think this isn't actually needed here; I should be able to simplify by dropping this commit.

Oh wait, not the whole commit, just this one change.

gnprice · 2025-07-03T01:32:54Z

lib/model/content.dart

@@ -1887,6 +1887,12 @@ class _ZulipContentParser {
  }
 }

+ZulipMessageContent parseMessageContent(Message message) {


nit: put at end of file, matching the specific-to-general ordering of the other parsing code here

gnprice · 2025-07-03T01:39:21Z

lib/widgets/action_sheet.dart

 }) {
+  // Could omit this if we need _showActionSheet outside a per-account context.
+  final store = PerAccountStoreWidget.of(pageContext);


Let's use accountIdOf instead, to make clear that this doesn't use anything else from the store.

(If it kept this store around and then used much of anything else from it in the build callback below, that'd be a bug because the store might change; in general we don't want to be keeping references to the store around beyond the lifetime of a build method or a gesture callback, and instead should look the store up anew from a context.)

gnprice · 2025-07-03T01:44:46Z

lib/widgets/action_sheet.dart

+                        child: ColoredBox(
+                          color: headerBackgroundColor ?? Colors.transparent,
+                          child: header))))


This color is currently always transparent, right? So can simplify this out, if that's the value we want.

gnprice · 2025-07-03T01:46:17Z

lib/widgets/action_sheet.dart

+  _showActionSheet(pageContext,
+    optionButtons: optionButtons,
+    header: _MessageActionSheetHeader(message: message));


nit: put header before option buttons; it appears before them physically, and logically they feel like the payload and this more like some decoration, so those two ways of ordering agree with each other

gnprice · 2025-07-03T01:49:22Z

lib/widgets/action_sheet.dart

+      color: designVariables.bgMessageRegular,
+      padding: EdgeInsets.symmetric(vertical: 4),
+      child: Column(
+        mainAxisSize: MainAxisSize.max,


Hmm interesting. What's the effect of this parameter?

Oops, I think this isn't wanted; will remove. (I'm not noticing it doing anything, and can't think of why we'd want it.)

gnprice · 2025-07-03T01:58:13Z

lib/widgets/action_sheet.dart

+            padding: const EdgeInsets.symmetric(horizontal: 16),
+            // TODO(#10) offer text selection; the Figma asks for it here:
+            //   https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-30210&m=dev
+            child: MessageContent(message: message, content: parseMessageContent(message))),


At first I'd thought this wouldn't be possible without more work toward issue #488: - #488 "content: Support Zulip content outside messages (even outside per-account contexts)". But it turns out that our content widgets don't assume a MessageList ancestor; it's sufficient that we provide a per-account context, via PerAccountStoreWidget, and a per-message context, via InheritedMessage (created by MessageContent).

How does this interact with muted users, #1560? That's one place where some related code, at least, relies on state that lives at MessageList.

(This is probably an instance where we're paying some cost for the technical debt of not yet having merged those muted-users PRs.)

Indeed, this calls for some work to allow SenderRow to appear where there isn't an ancestor MessageListPage. Will fix.

gnprice · 2025-07-03T02:00:43Z

test/widgets/action_sheet_test.dart

@@ -850,6 +855,44 @@ void main() {
  });

  group('message action sheet', () {
+    group('header', () {
+      testWidgets('message sender and content shown', (tester) async {


Is it easy to also add a check that the timestamp is shown? That's the motivating use case that prompted us to actually go and implement this feature now, so it'd be good to test it too 🙂

(But I know controlling times and time zones is an annoying area in tests still, so if it's not easy then we can skip it in favor of shipping, with I guess a TODO comment here in the tests.)

I started writing a test for this just now, then realized we don't show the full date :) is that okay, do you think? You could be looking at a message from a different day and see "1:54 PM" here, and I wonder if that might confuse people into thinking it was sent today.

Hmm, good catch!

That does sound potentially confusing, so I think it'd be better to avoid it. Perhaps show the date (as if in a date separator or recipient header) and then the time? Or perhaps a refinement: skip the date if it's today.

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

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Comments below.

gnprice · 2025-07-03T21:30:05Z

test/widgets/action_sheet_test.dart

-            await tester.longPress(find.byType(MessageContent), warnIfMissed: false);
+            await tester.longPress(
+              find.descendant(
+                of: find.byType(MessageList),
+                matching: find.byType(MessageContent)),


Same question as at #1624 (comment) , I think. In this one, there was an action sheet at a previous step of the test, but we already hit a button on it and then even used the compose box to save a message edit. So it seems like the story of the test calls for the action sheet to not be here now — which is why we need this step, after all, to bring it back.

It looks like nothing above waits for the action sheet to actually disappear after the button is hit, though; instead there's just await tester.pump(Duration.zero);. Maybe have that wait longer?

Good catch; yes, thanks for spotting this.

gnprice · 2025-07-03T21:41:24Z

test/widgets/action_sheet_test.dart

@@ -850,6 +855,44 @@ void main() {
  });

  group('message action sheet', () {
+    group('header', () {
+      testWidgets('message sender and content shown', (tester) async {


Hmm, good catch!

That does sound potentially confusing, so I think it'd be better to avoid it. Perhaps show the date (as if in a date separator or recipient header) and then the time? Or perhaps a refinement: skip the date if it's today.

gnprice · 2025-07-03T21:44:49Z

test/widgets/action_sheet_test.dart

+        ).findsOne();
+      });
+
+      testWidgets('muted sender', (tester) async {


nit: clarify the expected outcome (as it's the opposite of what you might expect if you just look at the test name):

Suggested change

testWidgets('muted sender', (tester) async {

testWidgets('muted sender also shown', (tester) async {

gnprice · 2025-07-03T21:51:14Z

lib/widgets/message_list.dart

+      && (MessageListPage.maybeRevealedMutedMessagesOf(context)
+                          ?.isMutedMessageRevealed((message as Message).id) == false);


This feels a bit hard to follow; and the behavior when there is no MessageListPage gets kind of buried here, whereas I feel like it should be more explicit and get a brief comment.

I'll send a quick PR to pull this whole condition out as a private method, which you can rebase atop to add this piece.

Sent as #1664.

…ting Thanks Greg for noticing that we were missing this part in the story of the test: zulip#1624 (comment) We're about to put a MessageContent in the action sheet itself, for zulip#217, and without this change, we'd get a failure later in the test when two MessageContents were found when one was expected (one in the message list and one in the action sheet).

We'd like to make SenderRow public, for use where there isn't a MessageListPage ancestor and we don't want to show the "unrevealed" state.

We're about to give _showActionSheet a `Widget? header` param, which we'll place as a child of this new Column.

For explicitness. All three callers are already passing a "page context" i.e. the result of PageRoot.contextOf.

See Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-29966&m=dev See also a different header variant in Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-26993&m=dev At first I'd thought this wouldn't be possible without more work toward issue zulip#488: - zulip#488 "content: Support Zulip content outside messages (even outside per-account contexts)". But it turns out that our content widgets don't assume a MessageList ancestor; it's sufficient that we provide a per-account context, via PerAccountStoreWidget, and a per-message context, via InheritedMessage (created by MessageContent). Fixes: zulip#217

chrisbobbe · 2025-07-04T01:46:17Z

Thanks! Revision pushed, and I've put some thoughts about the timestamp format in a dartdoc.

chrisbobbe requested a review from rajveermalviya June 24, 2025 08:30

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jun 24, 2025

rajveermalviya approved these changes Jun 25, 2025

View reviewed changes

chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 8bc6031 to 6a2649a Compare June 26, 2025 07:17

rajveermalviya 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 Jun 30, 2025

rajveermalviya assigned gnprice Jun 30, 2025

rajveermalviya requested a review from gnprice June 30, 2025 16:14

gnprice reviewed Jul 3, 2025

View reviewed changes

chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from 6a2649a to c98481d Compare July 3, 2025 21:09

gnprice reviewed Jul 3, 2025

View reviewed changes

gnprice mentioned this pull request Jul 3, 2025

msglist [nfc]: Pull out a _showAsMuted method #1664

Merged

chrisbobbe added 10 commits July 3, 2025 17:22

content [nfc]: Move parseMessageContent here, from msglist; make public

ea2e7fc

msglist [nfc]: s/{r,maybeR}evealMutedMessagesOf

55037c8

We'd like to make SenderRow public, for use where there isn't a MessageListPage ancestor and we don't want to show the "unrevealed" state.

msglist [nfc]: Make SenderRow public

fbdb42c

action_sheet [nfc]: Pull out a designVariables local

bf86e18

action_sheet [nfc]: Refactor some vertical padding, toward header param

d3e9be0

We're about to give _showActionSheet a `Widget? header` param, which we'll place as a child of this new Column.

action_sheet [nfc]: In _showActionSheet, s/context/pageContext/

3dbd612

For explicitness. All three callers are already passing a "page context" i.e. the result of PageRoot.contextOf.

action_sheet: In _showActionSheet, wrap content in PerAccountStoreWidget

40b7b5e

msglist [nfc]: Factor out MessageTimestampDisplay

0036539

chrisbobbe force-pushed the pr-message-content-in-action-sheet branch from c98481d to 5e56d4c Compare July 4, 2025 01:45

	testWidgets('muted sender', (tester) async {
	testWidgets('muted sender also shown', (tester) async {

		&& (MessageListPage.maybeRevealedMutedMessagesOf(context)
		?.isMutedMessageRevealed((message as Message).id) == false);

Show message content, sender, and timestamp in message action sheet #1624

Are you sure you want to change the base?

Show message content, sender, and timestamp in message action sheet #1624

Uh oh!

Conversation

chrisbobbe commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jun 24, 2025

Uh oh!

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

chrisbobbe commented Jun 26, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 3, 2025

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!

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!

chrisbobbe commented Jul 4, 2025

Uh oh!

Uh oh!

chrisbobbe commented Jun 24, 2025 •

edited

Loading