Add support for showing Poll Widgets.

[ishammahajan]

Widgets support has been in great demand from users (as it was
implemented in the webapp first, and we haven't supported its
display since support was added to the API).

This PR adds the poll widget support, and modularises widgets
so it will be easier to add other widgets if required. It uses the
newly introduced (c174a0) getOwnUser to get the user_id of the
current user in order to check it against the key provided in
submessages (which are only widgets for now) of type vote, to see
if the current user has voted on the option in that poll.

Styling has been duplicated from and merged with .reaction, while
the borders of the .widget class has been changed to match the
.static-timestamp class. You will also see a Poll Widget marker in the
bottom right of the box which shows it.

Fixes #3205 .

[gnprice]

Thanks @ishammahajan!

I just played with this a bit in the app, and with a different user in the webapp, on the same poll. Thoughts on the UI and functionality first -- mainly a number of style tweaks:

... you know what, I'll make this a thread on #mobile. That seems like the best venue for this part, because other people may have useful feedback too.

[gnprice]

Comments on the UX are here:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/poll.20widget/near/768137

Reading the code now.

c174a0b user selectors: Add selector for retrieving current user.

• Cool, this looks helpful to add!

• Let's avoid adding a use of NULL_USER; see its jsdoc 😉 . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)

  In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently 🙂

509d8e0 widget poll: Add basic support to show widgets from available data.

• I like the idea to move widgetAsHtml to a new file!

• nit: widget: Message | Outbox -- I'd call this just message. I feel like "widget" should refer to something more specific -- i.e. it's not that the message is a widget, but rather has a widget.

• Here:

  // This condition will never practically happen as it has been confirmed in caller
  // -- just there for Flow's happiness
  if (!widget.submessages) {
    return '';
  }

When a condition will actually never happen, I'd rather not have code that only executes if it happens 🙂 . For one thing -- is returning '' really a good way to handle it if it somehow did?

One solution is a $FlowFixMe on a specific line that Flow complains at.

Here, I think a good solution is to listen to the type error as a signal that your interfaces don't match your logic -- you're in a condition where submessages is nonempty, but you're passing the whole message which is mostly not about that. How about passing submessages itself? And content.

• Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

• Important! Please use the HTML-escaping template template-tag for all HTML.

  Without that, this has an HTML injection vulnerability where it inserts option.option. To illustrate, here's some options added by a different user, as seen in the webapp:

  

  and then how that renders under the PR's current version:

  

• Let's also replace the pieces.push pattern with template-tagged template literals. That makes fewer templates, making it easier to consistently use the tag; and it also makes the nesting of <div>, </div>, etc., easier to see.

• I think a helpful simplification would be to do submessages.map(submessage => JSON.parse(submessage.content)) up front.

• It's kind of confusing how pollOptions is built:

  • The initialization with a filter/map sequence, very functional-programming-style, makes it look like that should be the final, never-mutated data structure.
  • And indeed if I look for references to pollOptions, they all look like reads -- find, then forEach to build HTML.
  • But the pollVotes stuff must be doing something... and that variable doesn't appear in the HTML-building, hmm... aha, the pollOptions.find is secretly a setup for mutating the option's data!
  • A small change that would help: say const pollOptions = [], so it obviously gets mutated later. Then have a forEach instead of map.

• I think the other uses of filter and map would also be improved by replacing with straightforward loop logic. Also we can then have just one loop, with a conditional on type, processing all the submessages in order. That's the canonical order, after all -- because that's what we'd get if we processed them incrementally as they happened.

• Another tool that would simplify some of this code: use Map for pollOptions, instead of an array. Then in particular

  pollOptions.find(option => option.id.toString() === votedOption)
  

  gets a lot more straightforward.

  (The current PR version is similar to lots of our existing code -- there's a lot of places Map would help.)

• What's the corresponding webapp code? It'd be helpful to point to it here, to aid comparisons.

• Naming: Let's s/title/question/g . That's what it is... and I think the border-bottom in this CSS (discussed in chat, linked above) is the sort of thing that would make more sense if it really were a "title" instead of a "question", so perhaps an example of this name leading one's thinking astray. 🙂

be9d884 submessages: Add vote ability to poll widgets.

e29641b widgets: Convert .widget class to a superclass.

(I'll leave these two here for today; the above, plus the UX comments in chat linked above, should be enough for the first round.)

[ishammahajan]

(so I wrote this entire reply, and then it got erased because the browser closed, so the replies below will be terse in order to be efficient)

• Let's avoid adding a use of NULL_USER; see its jsdoc 😉 . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)
  In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently 🙂

Thanks! I saw those commits initially, but was reluctant to reject NULL_USER's use because the alternatives in my mind were very complicated to understand. Fixed now.

• i.e. it's not that the message is a widget, but rather has a widget.

Makes sense. Fixed!

Here, I think a good solution is to listen to the type error as a signal that your interfaces don't match your logic -- you're in a condition where submessages is nonempty, but you're passing the whole message which is mostly not about that. How about passing submessages itself? And content.

I agree, thanks! Fixed!

• Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

Sure, done!

• Important! Please use the HTML-escaping template template-tag for all HTML

My bad, I assumed that option.option was markdown (as I mistakenly pointed out in the comment containing its type.

/**
 * Option submessages are of type -
 * {
 *    type: 'new_option',
 *    idx: number,
 *    option: string (Question String),
 * }
 */

• A small change that would help: say const pollOptions = [], so it obviously gets mutated later. Then have a forEach instead of map.

Sure! That would help out a lot.

• Let's also replace the pieces.push pattern with template-tagged template literals. That makes fewer templates, making it easier to consistently use the tag; and it also makes the nesting of <div>, </div>, etc., easier to see.

Well, I tried to make it as template tagged as possible, but couldn't achieve it completely because we need to use a for loop to cover all the options anyways. Do you perhaps mean that there need to be functions which return perhaps a Map of option (html) objects which can then be joined?

• Another tool that would simplify some of this code: use Map for pollOptions,

Is there any particular reason why you're recommending Map? Is it somehow faster performance wise? -- ah, you're saying it'll be more simple. Done!

• Naming: Let's s/title/question/g . That's what it is... and I think the border-bottom in this CSS (discussed in chat, linked above) is the sort of thing that would make more sense

Done! 😄

(I constructed some automation for rebasing PRs since code review usually involves a lot of rebasing and committing changed to a previous commit, which takes up a lot of time. That took a lot of time this round. Hopefully next round should be much swifter than normal -- noticed a saving of about 20-30 seconds in each of the last three changes I committed to this PR.)

[ishammahajan]

(the failure in CI doesn't seem to be related to this PR.)

[gnprice]

Thanks @ishammahajan for these revisions!

(so I wrote this entire reply, and then it got erased because the browser closed, so the replies below will be terse in order to be efficient)

Ouch, yeah. That is a frustrating limitation in GitHub. 😢 The same thing happens to me sometimes and makes me tempted to write my comments in my own editor first... but never quite often enough to get me to pay the overhead of actually doing that.

Hmm, reflecting on it, I think this is one of the reasons I've shifted to more often posting several separate comments if I have a lot to say. It limits just how much work I can lose if that happens.

• Let's avoid adding a use of NULL_USER; see its jsdoc wink . That in turn refers to some discussion and examples which you can see in the Git history with e.g. git lsp --reverse 25125db94^..e22596c24 (where lsp aka log --stat -p.)
  In this case, I think a good model for it is getActiveAccount and others in accountSelectors. I should maybe update that comment to point to those examples; I added them more recently slightly_smiling_face

Thanks! I saw those commits initially, but was reluctant to reject NULL_USER's use because the alternatives in my mind were very complicated to understand. Fixed now.

Looks great, thanks!

I think if I understand you right, the alternatives to NULL_USER (at least this alternative) are no longer very complicated in your mind. But if not, I'd be glad to explain more and/or answer followup questions 🙂

About to merge that commit. I've also added a few new commits on top, mostly building on this in ways that came to mind when I was looking at it:

• One delivers on what I said above about updating that comment
• One adds jsdoc on this and some related selectors (I've added jsdoc on a lot of this file since you sent this PR)
• One adds another selector getOwnUserId which this made easy to add -- @jainkuniya, this turns out to relate to something we were just talking about today 😄, so take a look
• One adjusts the use of caching

Next, I'll take a look at the rest of the branch.

[gnprice]

About to merge that commit. I've also added a few new commits on top

(Done, as 3c25ad1..75b8a3f )

[gnprice]

931df06fb widget poll: Add basic support to show widgets from available data.

• Style nit, in commit message: c174a0 is too short as a short-id for a commit. Very short short-ids risk becoming ambiguous as the history grows. Also GitHub's web UI won't linkify them: c174a0

  I always write 9 digits, to make it very unlikely to ever become ambiguous. Saying git config --global core.abbrev 9 will make Git print that many whenever it prints a short-id, which helps.

  By default Git abbreviates to 7 digits instead; so does GitHub; and that's also the minimum for GitHub's web UI to linkify it. So that's the minimum.

• • Important! Please use the HTML-escaping template template-tag for all HTML

  My bad, I assumed that option.option was markdown (as I mistakenly pointed out in the comment containing its type.

  I think you're correct about that! That's in fact key to why this is a problem. 😛 I haven't really explained this very clearly, and it's important (in programming in general, and in this particular code), so let me try again.

  First: though I haven't re-reproed it to confirm, I believe the same bug I demonstrated above is still present in your new version. The added use of template is exactly right, but then the $!$ re-introduces the bug. Would you try the repro in my screenshots above, and play around with that? I think that'd be a helpful hands-on exercise toward understanding this.

  Then: to say more explicitly what I meant to say above, all code that produces HTML should use template.

  • By "produces HTML", I don't mean anything about the output's possible values -- like whether they might contain something like <strong>, or <script>, as a substring.
  • By "produces HTML", I mean that whatever consumes the output (the function's caller, or whatever) treats it as HTML. The central example of how to "treat it as HTML" is to stick it straight into the HTML fed to the WebView, without further transformation. In general the output of all our ...AsHtml functions will be treated as HTML -- we stick them into bigger and bigger pieces of HTML until the whole thing finally goes to the WebView.

  Logically, "HTML" here is essentially a type. It's a subtype of string, with a meaning like "string that is OK to treat as HTML and won't break something if we do". We unfortunately don't have the type-checker helping us keep straight how we're handling this type, but the ways to reason about it are essentially the same as for any other type.

  In particular, think of all the ...AsHtml functions as returning type "HTML", not the broader type string. Then when working inside of one, ask yourself: how do I know the value I'm returning is really a proper "HTML" ("string we can use as HTML without it being a bug") and not just any old string?

  • You can get a real "HTML" type by calling any other ...AsHtml function, because that's what they return.
  • Or with a string literal like '<div />', where you can look right at the source code and say "yep, that definitely is OK to use as HTML".
  • Or... by calling template. That's its job.
    • In particular, inside of template if you say ${...}, that takes a plain old string as argument... and it encodes it into HTML. This is the single primitive for turning plain strings into HTML -- any other function with a type like (string) => HTML ultimately uses template to do it (unless it contains a type error.) This lets us centralize in one place (the template function) the responsibility for doing that correctly.
    • There's also $!${...}. But: in that case you should think of the parameter type as "HTML". It's a type error (just one that Flow sadly doesn't help us catch) to pass an ordinary string to $!${...}.

  So: without template, there was a type error because you were pushing into pieces -- which is an Array<HTML> -- a plain string returned by plain "``".

  And then in this version, the template fixed that type error; but now you're passing option.option to $!${...}, which requires type HTML... and as you say, option.option is not type HTML. (It's some arbitrary text written by potentially any other user.) So that's a type error.

  Does that help? I'd be glad to answer followup questions -- there are other aspects I could describe, but this is looking long already so I want to check in and gauge what would be helpful for you.

• One helpful workaround strategy for the fact that the type-checker doesn't help us with this distinction is what we've done with all those ...AsHtml functions: when something has type HTML, put ...Html at the end of its name. This can make it much easier to look locally at any given bit of code and either confirm it's well-typed, or notice someplace that's potentially wrong -- where it can only be well-typed if some name isn't observing that convention.

  I'd recommend doing that with the local names here, too: contentHtml, pollWidgetAsHtml, htmlPieces.

... I have another remark to make, but I'll hit send at this point first 😅

[gnprice]

931df06fb widget poll: Add basic support to show widgets from available data.

(cont'd)

• • Looks like pollWidget has a natural separation between computing pollOptions, and then using that to compute the HTML. I find that observation pretty helpful in reading it. Let's make it explicit by pulling out a function that computes pollOptions (including the voters bit.)

  Sure, done!

  Thanks!

  I think I didn't succeed in explaining what I had in mind the first time, though. The most important part here is actually that the voters bit gets moved into that separate function too -- i.e. the pollVotes.forEach loop, which goes and constructs the voters property on each value in pollOptions.

  That's partly because one of the really confusing things in this logic is the interaction between the code that computes pollOptions, and the code after that that goes back and mutates it. When that interaction is contained within a smaller function, it has a smaller effect on understanding the code.

  It's also partly because the natural separation I mentioned is between pulling the data out of submessages -- i.e., computing pollOptions, in its final form, including the completed voters properties -- and then using that data to make HTML. Moving just one phase of the computation into its own function separates the logic in a way that's not as helpful for understanding it.

[ishammahajan]

I think if I understand you right, the alternatives to NULL_USER (at least this alternative) are no longer very complicated in your mind. But if not, I'd be glad to explain more and/or answer followup questions 🙂

That's true! I'm pretty sure I understand it (at least enough that if I refer to this PR again, I will be able to solve further problems). Thanks!

• One adds another selector getOwnUserId which this made easy to add -- @jainkuniya, this turns out to relate to something we were just talking about today 😄, so take a look

That sounds good!

• Style nit, in commit message: c174a0 is too short as a short-id for a commit. Very short short-ids risk becoming ambiguous as the history grows. Also GitHub's web UI won't linkify them: c174a0

I'll fix that. Thanks!

• By "produces HTML", I mean that whatever consumes the output (the function's caller, or whatever) treats it as HTML. The central example of how to "treat it as HTML" is to stick it straight into the HTML fed to the WebView, without further transformation.

Oh that makes it crystal clear!

• Does that help?

Definitely! Thanks so much for the long explanation! 😄

• I'd recommend doing that with the local names here, too: contentHtml, pollWidgetAsHtml, htmlPieces.

Makes a lot more sense, fixing. :)

... I have another remark to make, but I'll hit send at this point first 😅

Which reminds me, I should probably do the same.

[ishammahajan]

@gnprice Thanks for the review! :)

• That's partly because one of the really confusing things in this logic is the interaction between the code that computes pollOptions, and the code after that that goes back and mutates it.

Ah, that makes so much more sense, again. It's fixed in my WD now.

• Moving just one phase of the computation into its own function separates the logic in a way that's not as helpful for understanding it.

Yeah, I don't remember why I thought that would even be a good idea now.

[gnprice]

Thanks @ishammahajan for the revisions -- this looks much nicer! 😄

d616773 widget poll: Add basic support to show widgets from available data.

Just a few comments at this stage -- and none of them critical like the HTML injection was. 😉

  const pollOptions = submessages
    .filter(submessage => JSON.parse(submessage.content).type === 'new_option')
    .reduce((options, option) => {
      const rawOption = JSON.parse(option.content);
      const optionId = rawOption.idx;
      options.set(optionId.toString(), {
        id: optionId,
        option: rawOption.option,
        voters: [],
      });
      return options;
    }, new Map());

• This is another example of the "false reduce" which we used to have all over, but which I recently purged from our codebase. ("Another" because I also mentioned this pattern just now on another PR: Add recent_private_conversations key to /register. #3535 (comment) .) Notice how the reducer takes options in... but then always returns the same options, and does its work by mutating it. That's the opposite of what reduce is intended for, making it confusing to read.

  A direct translation would be to use forEach, with an explicit accumulator variable:

  const pollOptions = new Map();
  submessages
    .filter(submessage => JSON.parse(submessage.content).type === 'new_option')
    .forEach(option => {
      const rawOption = JSON.parse(option.content);
      const optionId = rawOption.idx;
      pollOptions.set(optionId.toString(), {
        id: optionId,
        option: rawOption.option,
        voters: [],
      });
    });

I think a further improvement would be to fuse the two loops:

  const pollOptions = new Map();
  submessages.forEach(submessage => {
    const data = JSON.parse(submessage.content);
    if (data.type !== 'new_option') {
      return;
    }
    const optionId = data.idx;
    pollOptions.set(optionId.toString(), {
      id: optionId,
      option: data.option,
      voters: [],
    });
  });

• One of my previous comments is I think still applicable:

  I think a helpful simplification would be to do submessages.map(submessage => JSON.parse(submessage.content)) up front.

  In particular note the only thing we ever do with submessages is pull off content from an element, and then hand that to JSON.parse. So the result of that map could replace everywhere we use submessages.

  And on the other hand the current version can end up parsing a given submessage's content quite a few times.

• Another one:

  I think the other uses of filter and map would also be improved by replacing with straightforward loop logic. Also we can then have just one loop, with a conditional on type, processing all the submessages in order. That's the canonical order, after all -- because that's what we'd get if we processed them incrementally as they happened.

  This is a lot like the changes I illustrate above for removing the false reduce. Perhaps it's easier to understand what I had in mind here with those examples? I think the refactoring you've done to make extractPollOptions might also help.

  Again, though, this one isn't critical. If it's still unclear, the easiest way to explain may be for me to illustrate by merging, and then adding a commit or two on top to make these refactors 🙂

• Similarly I'd still like to replace the htmlPieces.push / join pattern with just template literals -- but I will probably just attempt that as a refactor on top after I merge this, and then that'll be an illustration to help explain what I mean.

  let pollOptions = new Map();

• This is a dead store; just make it const later when it's assigned.

Quick thoughts outside of src/webview/html/:

@@ -69,6 +70,7 @@ export type BackgroundData = $ReadOnly<{
   flags: FlagsState,
   mute: MuteState,
   ownEmail: string,
+  ownUser: User,

• A nice followup would be to drop ownEmail in favor of ownUser.email.

• I haven't yet read the CSS closely.

eb62358 submessages: Add vote ability to poll widgets.

+  if (target.matches('.poll-button')) {
+    const optionElement = target.closest('.poll-option-container');
+    if (!optionElement) {
+      throw new Error('Corresponding option element not found.');
+    }

• Can we stick the data-option-id on the .poll-button instead? That'd simplify this a bit.

  (If there's some reason that would make something else complicated, though, then no need.)

• When you rebase, note c4fdd65; the api usage has gotten a bit cleaner still, and you'll want to update to match.

            key: [ownUser.user_id, event.optionId].join(),

• Huh, this... inserts a comma, I guess? TIL.

  I'd rather be explicit. 🙂 Writing

`${ownUser.user_id},${event.optionId}`

isn't any longer, and I think clearer about the format of the string.

+        sendSubmessage(
+          auth,
+          event.messageId,
+          JSON.stringify({
+            type: 'vote',
+            key: [ownUser.user_id, event.optionId].join(),
+            vote: event.vote,
+          }),
+        );

• Hmm, what source were you able to consult for seeing what the form of this request should be? Specifically what should go into the last argument, the JSON blob content. I don't see any details about votes on poll widgets in the widgets doc you helpfully linked to in the API-binding jsdoc.

  Even if it's an unsatisfying answer like "the webapp code" or "the network tab of browser devtools using the webapp", that's useful context. And if it was source code, you can mention the particular file to look at.

(OK, stopping here.)

[gnprice]

(I just edited the PR description to add a cross-reference to the issue, #3205.

That was already in one of the commits in the branch, which is important because the commits become our primary record of what we did... but because GitHub 🤷‍♂️ doesn't use that information as well as it could, it's separately important to mention any fixed issues in the PR description. In particular, this causes the issue thread to have a useful link back here to the PR.)

[gnprice]

(The issue is P1, so this is too.)

[scaredmushroom]

What is the state on this pr?
I would really like to have this feature.

Can I help somehow?

[WesleyAC]

Closing this now that #4704 is in — thanks for the work on it, though, it was useful to see, and ee1e74e especially was something that I took directly from this PR :)