messageActionSheet: Add option to copy link of message to clipboard

[ishammahajan]

There are use cases where a person wants to share a message with others
who he knows have access to a particular stream. To resolve this, in this
commit, another option is added in the action sheet which shows up when
a message is long pressed, which copies the link of the message to the
Clipboard.

I am concerned as to whether I should include tests for either of the files
since I don't see any cases where something could go wrong. I have included
translation for only English for now, because I wanted to make sure
everything else is correct. I have also amended the share functionality
to include a sweet little message which contains the link to the stream to
make it more useful. Please advise on both the tests and this.

Fixes #2623

[gnprice]

Thanks @ishammahajan !

• This code will only work for stream messages, but it looks like the option will be offered for all messages.
  • I think we should just make it work for all messages -- even in a 1:1 PM thread, you might want to send the other person in that thread a link to a specific old message.
• Relatedly 😉 , please add some unit tests for getLinkToMessage.
• Text for these options: "Copy link to message" for this one, and "Copy message" for what's currently "Copy to clipboard".
  • In particular just "copy link" is ambiguous when there's a link in the message.
• Just English is fine -- we do our translations on Transifex, and it's an easy couple of commands for me to copy strings to Transifex and to all languages.

[gnprice]

Also:

• I think I'd rather skip the "to follow this topic" etc. text.
• I'm dubious of that /<(?:.|\n)*?>/gm logic -- it doesn't seem like a great way to produce a readable plain-text version, plus it's a regexp that appears to have been written by someone who didn't understand the syntax they were using (specifically /m vs. /s.) While we're looking at this code, how about making this get the text the same way as copyToClipboard does?

[ishammahajan]

@gnprice thanks for the review!

• With changes in text, should I change the variable names for the buttons as well?
  • It seems counterproductive to do so, since it might still be easier to understand for developers to copy text to the Clipboard.
  • On the other hand, a discrepancy between what's shown on the screen vs what's there in the code might make the code unhealthy.
• I agree with the other changes, and have made them (not pushed yet, waiting for the discussion to finish on the previous first).

[jainkuniya]

nice work @ishammahajan 👍

Left few comments :)

[jainkuniya]

I guess better place for this is modelTypes.js

display_recipient: $FlowFixMe, // `string` for type stream, else PmRecipientUser[].

[ishammahajan]

Will fix. It's a trace left behind from a previous version.

[jainkuniya]

I didn't got why this change is required?

[ishammahajan]

It isn't required, but as @gnprice pointed out, the code is cleaner and more readable this way.

For runtime conditionals, what we most often do is look at a property with a name like type, whose value is one of a few fixed strings. We design the types so that knowing that value decides what other properties will exist with what types.

In the case of Message, there's one of those with value either 'stream' or 'private'.

This also makes the tests more strict.

[jainkuniya]

In other place, we are using just using zulip.com or example. I would not prefer to have one more category like goo.bar :)

[ishammahajan]

I will fix this, thanks! 🙂

[jainkuniya]

Copy, Link are capital, but to and clipboard are small 😅

Intensional or my mistake? :)

[ishammahajan]

I seem to have forgotten to fix this, thanks for the reminder! :)

[jainkuniya]

@ishammahajan It will be super great if you cloud also provide screenshots of the change (action sheet) as well.

Thanks again @ishammahajan!

[ishammahajan]

@jainkuniya open source is awesome!

Here is a screen recording! 😄

[gnprice]

Thanks @ishammahajan for these revisions!

I didn't manage to give this the time for a complete review today, sorry; will do that early next week.

One quick bit:

-const shareMessage = ({ message }) => {
+const shareMessage = async ({ auth, message }) => {
+  const rawMessage = isAnOutboxMessage(message) /* $FlowFixMe: then really type O
utbox */
+    ? message.markdownContent
+    : (await getMessageContentById(auth, message.id)).raw_content;
   Share.share({
-    message: message.content.replace(/<(?:.|\n)*?>/gm, ''),
+    message: `${rawMessage}\n-- ${getLinkToMessage(auth, message)}`,
   });
 };

This is really making two quite different changes:

• adding the link
• switching from the replace to the raw_content logic for getting the message content (as I asked above -- thanks!)

Those should be separate commits; this is an example of making clear and coherent commits. That will help make both changes easier to understand. For one thing, it means the change to the message-content logic will get its own commit message, so you can properly explain what's changing and why 🙂

[gnprice]

Here is a screen recording!

So, I watched the recording, and was confused for a few seconds: it didn't look like there was a link anywhere in the message you long-pressed on, so why was there a "Copy link to clipboard" option, and why did it apparently produce a link?

Then I realized it was a link to the message. 😛 Even though I'd just been looking at this PR thread, the text of that option unambiguously read to me as "take this link here that you've pointed at, and copy it to the clipboard". (Which, with #3404, is a feature we'll even have in Zulip soon! 🎉)

I guess this makes a sort of demonstration of this bit I said above:

Text for these options: "Copy link to message" for this one, and "Copy message" for what's currently "Copy to clipboard".
In particular just "copy link" is ambiguous when there's a link in the message.

[ishammahajan]

@gnprice thanks for the review! I have separated the commits as requested and went through the link for clear and coherent commits (thanks for the link 😄).

Then I realized it was a link to the message.

It is rather confusing for me too, the first time I used slack I remember being confused because of the same reason. Copy link to clipboard seems too ambiguous in terms of what it copies.

I have rather replaced it with Copy message content and Copy message link, in the latest push. I think this makes the distinction quite clear, and even though this might not seem like proper English, I think a user will understand this intuitively (since this contains a short and sweet format of verb-noun), and will become used to this very soon. It is also the kind of simple format which makes it easy to translate and understand in other languages.

[ishammahajan]

Here are the updated screen recordings for content, link, and share!

[ishammahajan]

@gnprice (sorry for the delay on this pr.)

I've fixed some small diffs within this, the recordings do still mostly hold true, except one place where I changed the toast message for copying a message link to the more consistent Copied link to message.

Polished and ready for review!

[ishammahajan]

Bumping this for visibility.

[gnprice]

Thanks @ishammahajan for this PR and the update! Sorry for the delayed review.

fd7a9a0 messageActionSheet: Add option to copy link of message to clipboard.

• I have rather replaced it with Copy message content and Copy message link, in the latest push. I think this makes the distinction quite clear, and even though this might not seem like proper English, I think a user will understand this intuitively (since this contains a short and sweet format of verb-noun), and will become used to this very soon.

  I think these are perfectly fine English! They're sort of headline-ese, with determiners (aka articles, e.g. "the" or "a") left out -- and that's common in UI text, as well as in newspaper headlines.

  So, one means "Copy the message content", or "Copy the content of this message". Seems clear enough, though "content" is a little bit jargon-y.

  The other means "Copy the message link", or "Copy a message link". I think it's still a little ambiguous what "message link" means -- just as real headlines in headline-ese are often ambiguous. Is it "Copy a link to the message", or "Copy the link in the message"? But it's good enough I'll be happy to merge it, and we can wordsmith further later.

• In getLinkToMessage, let's have it take an Identity rather than Auth, to reduce the number of places we pass the user's API key around. (We only recently introduced this type, and haven't gone back and applied it widely, so you've seen a lot of code still uses Auth.) I looked and found I hadn't documented the idea as much as I'd like, so I just did -- see 7e687fe .

• Hmm, this logic is a bit tricky! And it's doing something it seems like the webapp also needs to do. Have you tracked down the corresponding webapp code to compare to, to look for edge cases? Would be good to point at it in a comment, too.

• There's also a lot of logic in common between stream messages and PMs. Let's deduplicate that, and have a message.type if/else that just computes a piece called, say, conversationPath, looking something like stream/123-general/topic/stuff.

• Instead of this bit:

+  message.display_recipient.forEach(user => {
+    uri += `${user.id},`;
+  });
+  uri = uri.slice(0, uri.length - 1);

how about join?

• Yikes, this calls for explanation! 🙂

+  uri += message.display_recipient.length < 4 ? '-pm' : '-group';

Why 4 -- what's that have to do with "pm" vs "group"?

• I think it'd be helpful to split out a first commit that just adds getLinkToMessage (and its tests), and then a separate one that adds it to the action sheet.

99eb302 messageActionSheet - Share function: Now returns a link along with message.

• Looks good!

382f04c messageActionSheet - Share function: Change logic to get message content.

• Thanks for pulling this out!

• These three lines getting the raw content are complex enough (and the interface of what they're supposed to accomplish, seen from the outside, is simple enough) that it'd be good to avoid duplicating them. Let's pull them out into a function like getRawMessageContent that both shareMessage and copyMessageContent can invoke.

• The commit message sounds a bit like this isn't expected to change anything ("do this exact same thing"), but I think actually the point is that the old way was just bad -- a message that had any markup at all would likely become hard to read.

0c03bc5 message-test: Fix conditionals and respective tests.

• Oof, sorry about the state of those tests. Thanks for fixing them up as needed.

• I kind of want a bit of explanation in the commit message about why the one way is better than the other. I guess I can just go quote what I said in the chat thread you linked above, though.

[gnprice]

OK, and pushed that last commit, with the display_recipient -> type change! 65c0294.

[ishammahajan]

• But it's good enough I'll be happy to merge it, and we can wordsmith further later.

I believe it's good, because it clarifies that Copy message content copies the content, which makes it inherently clear that the other copies something other than the content (which includes the link). I agree though, it's a bit tough to get around. Perhaps a solution is just to add a dummy (but functioning) function of Copy Link in Message just to make the distinction between 'Copy the link present in the message?' and 'Copy the link which redirects to the message?' -- it could be shown as disabled if there are no links in the message.

• In getLinkToMessage, let's have it take an Identity rather than Auth, to reduce the number of places we pass the user's API key around.

I am actually wondering why we need to pass the API key around lesser number of times, in the context of the app. As long as we don't give away the key, we should be fine, right? (I did change it in any case :))

• Hmm, this logic is a bit tricky! And it's doing something it seems like the webapp also needs to do. Have you tracked down the corresponding webapp code to compare to, to look for edge cases? Would be good to point at it in a comment, too.

I tried looking at what the webapp does for this, but found very little information (perhaps because I'm unfamiliar with it's architecture). Looks like it's using something like a zid parameter to put in the link, but when you do actually copy the link in the webapp, that zid parameter is nowhere to be seen.

• There's also a lot of logic in common between stream messages and PMs. Let's deduplicate that, and have a message.type if/else that just computes a piece called, say, conversationPath, looking something like stream/123-general/topic/stuff.

Certainly looks a lot cleaner! Thanks!

how about join?

Ah, another convenience function. Thanks! 🙂

• Yikes, this calls for explanation! 🙂

+  uri += message.display_recipient.length < 4 ? '-pm' : '-group';

Okay, so. For testing out how to code these URLs properly, I took a bunch of examples from the webapp (forming a lot of groups with test accounts). I found that 3 participants in a group doesn't make it a 'group' in the eyes of the server (or at least the examples led me to believe so)! The group has to have at least 4 participants to have that -group label in the link.

Hmm, so I went to check this again. With the same group. Found that in fact the link does have the -group label. I wonder why it showed false results before -- perhaps a bug fixed later on? Anyways, this is fixed.

• I think it'd be helpful to split out a first commit that just adds getLinkToMessage (and its tests), and then a separate one that adds it to the action sheet.

Sure thing!

• Let's pull them out into a function like getRawMessageContent that both shareMessage and copyMessageContent can invoke.

getRawMessageContent is already a function which fetches the raw content of a message from the api. Perhaps getMarkdownContent?

[gnprice]

I am actually wondering why we need to pass the API key around lesser number of times, in the context of the app. As long as we don't give away the key, we should be fine, right?

Yep! The idea is to make it easier to be sure we don't give away the key, by making there be a lot fewer places where we pass it around and have to think about where that information is potentially flowing to.

(I did change it in any case :))

Thanks :)

• Hmm, this logic is a bit tricky! And it's doing something it seems like the webapp also needs to do. Have you tracked down the corresponding webapp code to compare to, to look for edge cases? Would be good to point at it in a comment, too.

I tried looking at what the webapp does for this, but found very little information (perhaps because I'm unfamiliar with it's architecture). Looks like it's using something like a zid parameter to put in the link, but when you do actually copy the link in the webapp, that zid parameter is nowhere to be seen.

I suspect zid is a name for the message ID.

Reading the code in that screenshot, I see that it puts up the "Copied!" alert message, but I can't actually tell how it's causing something to get copied to the clipboard, or deciding what should get copied.

Perhaps ask in #frontend? Someone there should be able to help find the relevant bit of code, so we can compare.

7b93477 messages utils: Add getLinkToMessage function.

• This looks good! Thanks for separating the commit.

• One more thing, though: I'd still like to be able to compare to the webapp's corresponding logic.

4a3bd46 messageActionSheet: Add option to copy link of message to clipboard.

• identityOfAuth(auth).realm,

  When you're right about to just pull out the realm property, it's fine to take it directly as auth.realm. Where identityOfAuth is really helpful is where you're going to pass the result off to some other code located somewhere else, that the reader doesn't have in front of them.

+  /* $FlowFixMe: message will always be of type Message */

Hmm -- it sounds obvious when you put it that way, doesn't it?

But if Flow doesn't think it's obvious, it's a good idea to ask: does Flow know something here that I don't? 😉 After all, basically a type-checker's entire job is to diligently follow around the most obvious facts.

What does Flow think this message's type is?

Look around at the similar functions in this file -- there's a few different ways they handle the type they're passed for message.

In this case, IIUC it really can be a non-Message in your current version.

255fe5e messageActionSheet - Share function: Now returns a link along with message.

• (No change, I think -- still looks good.)

f7a93d3 messageActionSheet: Change logic for getting raw message content.

• Thanks for the refactor! This looks good.

• Just a remaining nit about the helper function's name:

  getRawMessageContent is already a function which fetches the raw content of a message from the api. Perhaps getMarkdownContent?

  Ah indeed. I think it'd actually be good to use the same name, though -- we can say api.getRawMessageContent for the API call. After all, this function basically does exactly the same thing as that one if the message happens to be one the server has; it just does something else if it's still in the outbox.

  They don't have to have the same name. But given that they're juxtaposed here in the same place so you might think about both of them at the same time... whatever contrast there is in the names, it's very helpful if it has something to do with the actual contrast between the functions.

  If one says "raw message" and the other "markdown", then it sounds like there's some subtle contrast between what "raw message" means and what "markdown" means, and that's the contrast between the functions. But in reality "raw message" and "markdown" are being used as synonyms here, and the contrast between the functions is a totally unrelated contrast, of being an API binding that talks straight to the server vs. being this wrapper that makes the interface a bit nicer and also covers outbox messages. So two synonyms are being used to stand for an unrelated contrast, which is confusing.

  One good test to apply to a pair of names like this: if you see both names, and you know what the two functions do but you've forgotten which is which... can you tell from the names which function is which? If the names are being helpful, you should be able to.

  So for example, getRawMessageContent vs. api.getRawMessageContent passes that test very well.

[ishammahajan]

• When you're right about to just pull out the realm property, it's fine to take it directly as auth.realm. Where identityOfAuth is really helpful is where you're going to pass the result off to some other code located somewhere else, that the reader doesn't have in front of them.

Oh. Sorry, what I did was just stupid 😆 .

Hmm -- it sounds obvious when you put it that way, doesn't it?

Ah, what I meant was, that message will never be of type Outbox. I think that's the problem flow was pointing me towards anyways. It was a little confusing the way I put it 😅 . Even that though, is probably not true, because it can obviously be an Outbox.

Hmm... It seems like the parameter which we would have missed, stream_id (if we considered only Message and the message was actually an Outbox) , does actually appear in when I use the feature in the debug build in a message which I have just sent (see this recording). Wonder why this is the case.

• So for example, getRawMessageContent vs. api.getRawMessageContent passes that test very well.

Thanks, for that convenience feature (import api) and the very detailed explanation! I'll make the edit! 🙂

[gnprice]

I saw a chat thread go by where it looks like you successfully located the webapp's code corresponding to getLinkToMessage.

What did you learn -- what does that code look like? In particular, does it have the same behavior in all edge cases?

Hmm... It seems like the parameter which we would have missed, stream_id (if we considered only Message and the message was actually an Outbox) , does actually appear in when I use the feature in the debug build in a message which I have just sent (see this recording). Wonder why this is the case.

We only use Outbox until the message has actually reached the server and come back to us from the server -- then it gets replaced by the server's version, a Message. Messages we send come back as message events, producing EVENT_NEW_MESSAGE actions, the same as messages sent elsewhere; and in outboxReducer you'll see we drop the corresponding Outbox object when we get an EVENT_NEW_MESSAGE.

In the recording, the spinner on the message has disappeared by the time you long-press on it. The spinner means we're still trying to send the message -- we show it precisely for Outbox objects.

I recommend using the Chrome DevTools to see the Redux state. That gives you a more direct way of seeing exactly what data we have. It's a log, so it also means you won't have to rush to look before the spinner disappears, or hack a workaround to prevent the update.

[ishammahajan]

@gnprice thanks for the review!

What did you learn -- what does that code look like? In particular, does it have the same behavior in all edge cases?

So I found that https://github.com/zulip/zulip/blob/master/static/js/hash_util.js was the file which contained the code for generating the link, specifically #L134.

exports.by_conversation_and_time_uri = function (message) {
    var absolute_url = window.location.protocol + "//" +
        window.location.host + "/" +
        window.location.pathname.split('/')[1];

    var suffix = "/near/" + exports.encodeHashComponent(message.id);

    if (message.type === "stream") {
        return absolute_url +
            exports.by_stream_topic_uri(message.stream_id, util.get_message_topic(message)) +
            suffix;
    }

    return absolute_url + people.pm_perma_link(message) + suffix;
};

The by_stream_topic_uri method found in the same file, as well as the people.pm_perma_link method found in the file named https://github.com/zulip/zulip/blob/master/static/js/people.js#L436 both did not handle edge cases, leading me to believe that there were none. I have also made our function in src/utils/message.js in compliance with both the aforementioned methods.

Messages we send come back as message events, producing EVENT_NEW_MESSAGE actions, the same as messages sent elsewhere; and in outboxReducer you'll see we drop the corresponding Outbox object when we get an EVENT_NEW_MESSAGE.

Ah, thanks. That makes sense! 😄

[gnprice]

Thanks @ishammahajan for the updates! Looking back at this.

0fccac9 messages utils: Add getLinkToMessage function.

So I found that https://github.com/zulip/zulip/blob/master/static/js/hash_util.js was the file which contained the code for generating the link

Cool, thanks. A quick pointer to this as a comment in the code itself would be helpful too -- like this:

// Compare `by_conversation_and_time_uri` in the webapp's `static/js/hash_util.js`.

The by_stream_topic_uri method found in the same file, as well as the people.pm_perma_link method found in the file named https://github.com/zulip/zulip/blob/master/static/js/people.js#L436 both did not handle edge cases, leading me to believe that there were none.

Hmmm. So here's the stream-message case:

exports.by_stream_topic_uri = function (stream_id, topic) {
    return "#narrow/stream/" + exports.encode_stream_id(stream_id) +
           "/topic/" + exports.encodeHashComponent(topic);
};

// Some browsers zealously URI-decode the contents of
// window.location.hash.  So we hide our URI-encoding
// by replacing % with . (like MediaWiki).
exports.encodeHashComponent = function (str) {
    return encodeURIComponent(str)
        .replace(/\./g, '%2E')
        .replace(/%/g, '.');
};

exports.encode_stream_id = function (stream_id) {
    // stream_data appends the stream name, but it does not do the
    // URI encoding piece
    var slug = stream_data.id_to_slug(stream_id);

    return exports.encodeHashComponent(slug);
};

// and in `stream_data.js`:
exports.id_to_slug = function (stream_id) {
    var name = exports.maybe_get_stream_name(stream_id) || 'unknown';

    // The name part of the URL doesn't really matter, so we try to
    // make it pretty.
    name = name.replace(' ', '-');

    return stream_id + '-' + name;
};

Some interesting cases I see there include:

• topic has a .
• topic has a %
• topic has something encodeURIComponent will escape -- like / or # or 👋 (aka '\ud83d\udc4b')
• stream name each of the above too
• stream ID doesn't exist
• stream name has a

Here's a test that exercises some of those:

  test('properly encodes topics', () => {
    const { realm } = eg.selfAccount;
    const message = eg.streamMessage({
      stream_id: 123,
      display_recipient: 'general',
      subject: '.%%2E/#\ud83d\udc4b',
    });
    expect(getLinkToMessage(realm, message)).toEqual(
      `${realm}/#narrow/stream/123-general/topic/.2E.25.252E.2F.23.F0.9F.91.8B`,
    );
  });

I got the expected result from sending a message with that topic, and looking at the narrow URL in the webapp.

Indeed it fails 🙂 :

    Expected: "https://zulip.example.org/#narrow/stream/123-general/topic/.2E.25.252E.2F.23.F0.9F.91.8B"
    Received: "https://zulip.example.org/#narrow/stream/123-general/topic/.%25%252E/#%F0%9F%91%8B/near/1234789"

So, please:

• Add a test like that, and another one or several tests for the interesting stream cases.
• Add logic to make those tests pass.
• Take another look at the code with these examples in mind, and see if there's anything else I missed. 😉

Note that much of the missing logic is basically inverses of functions like parseTopicOperand in the new internalLinks.js, since my recent work on that logic that fixed #2760. I think it'd probably be helpful to have functions like unparseTopicOperand located right next to their inverses.

2086e1e messageActionSheet: Add option to copy link of message to clipboard.

• It looks like message here still can be an Outbox value. Do you believe it can't? If so, what's your reasoning for why not?

3e764bf messageActionSheet - Share function: Now returns a link along with message.

56f0d78 messageActionSheet: Change logic for getting raw message content.

• One quick thing I notice here:

-    message: `${message.content.replace(/<(?:.|\n)*?>/gm, '')}\n\n-- ${getLinkToMessage(
-      auth,
-      message,
-    )}`,
+    /* $FlowFixMe: message will always be of type Message */
+    message: `${rawMessage}\n\n-- ${getLinkToMessage(auth.realm, message)}`,

Old and new versions pass different types of argument to getLinkToMessage. Surely either before or after is a type error, right?

(I'm guessing this came from an omission when rebasing.)

[chrisbobbe]

This would be a very nice feature to finish, if possible! It would be a boost to people’s productivity on chat.zulip.org; I really like linking from topic to topic to make important connections and leave a trail for others (or future me) to follow when debugging or comparing related feature ideas. I can’t really write code during my commute, but I can do that! 🙂

[chrisbobbe]

Thanks for this, @ishammahajan!

I'd like to take over working on this PR, if that's OK with you, and make those tweaks Greg requested in August, since this would be a really nice feature, and you've said you're quite busy, which is very understandable. Again, thank you so much for doing most of the work on this! 🙂 So I'll assign the issue to me, fix merge conflicts with the current master, work on it, and make my own PR to replace this one.

[chrisbobbe]

Closing, to resume work in #3865. But I'll refer back to the conversation here.