Add UI to view read receipts #5489

chrisbobbe · 2022-09-15T11:01:38Z

This lets us view read receipts in the mobile app! 🙂

While working on this, I noticed a few nice-to-haves and orthogonal cleanups, but I've rebased those atop this feature branch to send in a followup PR, to help us concentrate on implementing the feature.

Anyway, in this PR, we:

do the usual Redux plumbing (though it was nice to see InitialDataRealm and RealmDataForUpdate have already been updated for this feature)
add a new custom Hook, useFetchedDataWithRefresh, for logic the new view calls for: a somewhat live-updating snapshot of data we fetch outside the event system, and information about staleness/failure to fetch
some refactors and a type loosening to try out a react-intl feature that should let us follow the web app in making a translated string where one word is a link
make ReadReceiptsScreen and add the action-sheet button to open it 🎉

Screenshots coming soon.

Fixes: #5367

chrisbobbe · 2022-09-15T11:16:09Z

gnprice

Thanks @chrisbobbe!

Generally this code looks good. I appreciate the automatic retry, the loading indicator (rather than false empty list) before data arrives, and the toast notices when things don't work. Small comments below.

The UI also looks fine to me, though perhaps @alya will have comments to add.

gnprice · 2022-09-16T15:31:00Z

src/reactUtils.js

+ * True just when a given state/prop/etc. has been true continuously for the
+ *   past `duration`.


Does this communicate something more specific than "just when value has been true" etc.? I.e., is there something that "a … state/prop/etc." is meant to say here?

If not, value is shorter, plus it ties it explicitly to the particular parameter it's meant to refer to.

Oh, right! Makes sense; thanks.

gnprice · 2022-09-16T15:37:43Z

src/reactUtils.js

+ * True just when a given state/prop/etc. has not changed for the past
+ *   `duration`.


This one is where the reasoning in this commit message applies:

I don't expect this to give reliable results if `value` is something mutable, like the current value of a ref. Mutating a value doesn't cause the component to rerender. So, tighten up this bit of the jsdoc, looking to usePrevious as an example:

Still, I think there's a clearer way to put this. The point is really about the meaning of "changed", more than about the nature of the value parameter; so we can leave "value" alone (for the same reasons as in the previous comment), and then clarify the specific meaning of "changed" in the paragraphs below.

This version already has such a clarification, but we can put it more in the foreground:

Can make it the first paragraph right after the summary line -- seems appropriate because it's probably the most important caveat that someone might potentially run into as a pitfall if they didn't notice it.

Could also add a few more words explicitly mentioning that if value itself is mutable, then this doesn't detect any mutations within it, if wanting to make it extra clear.

Still, I think there's a clearer way to put this.

Yeah, I agree.

The point is really about the meaning of "changed", more than about the nature of the value parameter

I think there's a point about the meaning of "changed", and also a point about the nature of the value parameter. (I don't think my revision captured either point very well, and perhaps neither point needs to be explained in a code comment, I'm not sure. 🙂)

As you've said, if value itself is mutable, then this doesn't detect any mutations within it.

If value is derived from something mutable (e.g., the current value true from a boolean ref), and that thing mutates such that the caller would pass something different (non-===) for value than it did last time, then this might detect the change. In particular, it will detect the change if the Hook is actually called with the new value. That might happen on the same render as the change, or on a later render, or never (e.g., the component unmounts or the mutation is reversed before the next render). So passing a value with this nature is very risky, and if a caller passed a useRef(…).current as value, I'd be skeptical even if they somehow got results they were satisfied with.

… and that thing mutates such that the caller would pass something different (non-===) for value than it did last time, then this might detect the change. In particular, it will detect the change if the Hook is actually called with the new value.

I see. The point you're making here is that this Hook only notices a change when the Hook is actually called, i.e. during a render -- it doesn't set up any kind of subscription to arrange to notice when something changes in between renders. (In contrast with useState, or react-redux's useSelector.)

Instead, the caller needs to arrange that. Which isn't hard -- it basically comes naturally if the caller follows normal React Hooks idioms. The value should come from something like a prop or useState state or useSelector result; and those should be immutable so that any updates deep inside them are reflected by the top-level object getting replaced with a different object, one that's !== from the old one. The main risk is probably from overusing useRef.

I think that caveat is worth mentioning in the jsdoc here, particularly because this Hook does arrange to notice (and cause a rerender on its own initiative) in a different case, namely when the given time elapses. I'll make a go at it and push another commit to the end of the PR branch.

I'll make a go at it and push another commit to the end of the PR branch.

86ea850

Yeah exactly; thanks!

gnprice · 2022-09-16T15:41:18Z

src/reactUtils.js

+ * "Changed" means last render's and this render's `value`s aren't ===.
 */
-export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean => {
+export function useHasNotChangedForMs(value: mixed, duration: number): boolean {


gnprice · 2022-09-16T15:46:32Z

src/reactUtils.js

+    // Trigger the effect just if React sees a change in `value`. In other
+    // words, just when last render's and this render's `value`s aren't ===.
+    value,


I think the logic here is good, but this comment isn't quite accurate: "just if X" means "if X, and only if X". IOW it means that the thing always happens when X, and also never happens when not X. Here, that would be true if this were the whole dependency array… but there's also duration.

Now, we have that debug assert that duration never actually changes. So it's sensible to take duration changing as a pathological case that we mostly don't worry about. But it does mean the wording here should be a bit different in order for the reader to not say "wait, but that isn't quite right -- so now I wonder what other bugs might be hiding here".

A sufficient fix would be to add a prefix: "Provided duration is constant, trigger the effect just if [… and the rest]".

Or: move this to appear after duration in the list, and then just add the prefix "Otherwise,".

Makes sense! Thanks.

gnprice · 2022-09-16T15:48:07Z

src/common/useFetchedDataWithRefresh.js

+type SuccessResult<TData> = {|
+  +data: TData,
+  +error: null,
+|};
+type FailResult = {|
+  +data: null,
+  +error: mixed,
+|};
+type Result<TData> = SuccessResult<TData> | FailResult;


nit: blank line between top-level declarations

(Or compact these onto one line each, and then they can go without the blank lines)

gnprice · 2022-09-16T16:20:48Z

src/message/ReadReceiptsScreen.js

+              'z-link': chunks => (
+                <WebLink url={new URL('/help/read-receipts', auth.realm)}>
+                  {chunks.map(chunk => (
+                    <ZulipText>{chunk}</ZulipText>


Huh, I wonder what the scenario is where this callback gets passed more than one item in chunks.

I guess perhaps if these are nested? So if you have e.g. a <foo>red <bar>thing</bar></foo>, then the foo callback gets (in English) two chunks, one of them a string "red " and one the node returned by the bar callback.

(And in a translation there might be a string after the node instead of the one before, or neither or both.)

Huh, yeah that would make sense.

gnprice · 2022-09-16T16:21:26Z

src/api/index.js

  getUserProfile,
  updateUserStatus,
  getFileTemporaryUrl,
+  getReadReceipts,


nit: this should go in the commit that adds this endpoint-binding

gnprice · 2022-09-16T16:27:50Z

src/message/ReadReceiptsScreen.js

+      if (!user) {
+        // E.g., data out of sync because we're working outside the event
+        // system. Shrug, drop this one.
+        return;


Makes sense

gnprice · 2022-09-16T16:30:15Z

src/message/ReadReceiptsScreen.js

+  // And it looks like Hermes, which we hope to switch to soon, supports it:
+  //   https://github.com/facebook/hermes/issues/23#issuecomment-1156832485
+  const userSorter = useCallback(
+    (a, b) => Intl.Collator(language).compare(a.full_name, b.full_name),


gnprice · 2022-09-16T16:36:49Z

src/message/ReadReceiptsScreen.js

+  const { latestResult, latestSuccessResult } = useFetchedDataWithRefresh(callApiMethod, 15_000);
+
+  // TODO: These vanishing toasts are easy to miss. Instead, show
+  //   latestResultIsError, isFirstLoadLate, haveStaleData, and onPressUser


Is onPressUser in this list unintentionally? Or, what would it mean to "show [it] with something more persistent"?

Ah, haha—yeah, that was accidental. I grabbed names from four code stanzas just below this, when I should've grabbed from just the first three.

chrisbobbe · 2022-09-16T22:39:40Z

Thanks for the prompt review! Revision pushed (which of course I don't expect you to review before you're back from PTO). 🙂

Based on this discussion thread: zulip#5489 (comment)

gnprice · 2022-10-08T02:50:49Z

Thanks @chrisbobbe for the revisions! The code all looks good. Two ongoing threads above which are now just about comments:
#5489 (comment)
#5489 (comment)
I've pushed a few small commits on top with drafts for those; feel free to reorder and/or squash if those look right to you.

@alya, any thoughts to add on the UI? See screenshots in Chris's first comment above: #5489 (comment)

alya · 2022-10-12T06:00:20Z

The screenshots look good to me, thanks!

Oh, sorry I didn't think of this earlier -- does zulip/zulip#22905 affect the mobile implementation?

gnprice · 2022-10-12T16:55:47Z

Oh, sorry I didn't think of this earlier -- does zulip/zulip#22905 affect the mobile implementation?

Ah, probably does, yeah.

Yep, looking at the web implementation:
https://github.com/zulip/zulip/pull/22908/files
it was all client-side. So we'll need a similar bit of logic.

…ipts) We'll use this for zulip#5367, "Make it possible to view read receipts".

…fine

…nging Based on this discussion thread: zulip#5489 (comment)

…ForMs We'll use this new, more generic Hook useHasNotChangedForMs later in this series. Keep useHasStayedTrueForMs, but have it call this new Hook. For that change to its implementation, it's helpful that useHasStayedTrueForMs has pretty thorough test coverage.

The docs are spotty on this feature, but it seems powerful and worth a try. We'll use it to put a web link on a single word in a translated message, in the style chosen by the web app.

chrisbobbe · 2022-10-19T05:56:43Z

Thanks for the review! Revision pushed.

This makes it possible to view read receipts. Fixes: zulip#5367

gnprice · 2022-10-19T17:09:36Z

Thanks! Looks good -- merging.

Added a small comment-commit on top, and tweaked a summary line:

11:  73b17179c ! 11:  468ac712e apiConstants [nfc]: Use kFooBar naming instead of FOO_BAR
    @@ Metadata
     Author: Chris Bobbe <[email protected]>
     
      ## Commit message ##
    -    apiConstants [nfc]: Use kFooBar naming instead of FOO_BAR
    +    api [nfc]: Use kFooBar naming instead of FOO_BAR, for constants
     
      ## src/api/constants.js ##
     @@
12:  adc04bfdb = 12:  8caf6dc01 ReadReceiptsScreen: Add, with TODO to show data state more visibly
 -:  --------- > 13:  868ba9c2e api [nfc]: Expand comments on kNotificationBotEmail

to have the simpler, more recurring prefix api rather than apiConstants.

chrisbobbe · 2022-10-19T17:33:47Z

Thanks!

chrisbobbe added P1 high-priority webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. server release goal Things we should try to coordinate with a major Zulip Server release. labels Sep 15, 2022

chrisbobbe requested review from alya and gnprice September 15, 2022 16:23

gnprice reviewed Sep 16, 2022

View reviewed changes

chrisbobbe force-pushed the pr-read-receipts branch from 185e421 to dcce0e8 Compare September 16, 2022 22:37

gnprice added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 8, 2022

reactUtils [nfc]: Add caveat to useHas…ForMs about value changing

86ea850

Based on this discussion thread: zulip#5489 (comment)

chrisbobbe and others added 10 commits October 18, 2022 17:20

api: Add getReadReceipts binding (GET messages/{message_id}/read_rece…

bc930f6

…ipts) We'll use this for zulip#5367, "Make it possible to view read receipts".

realm state: Store enableReadReceipts

03f7a98

action-sheets: Plumb enableReadReceipts to constructMessageActionButtons

a171c49

reactUtils [nfc]: Stop passing a type argument that Flow infers just …

bf1f452

…fine

reactUtils [nfc]: Add caveat to useHasStayedTrueForMs about value cha…

4eafc24

…nging Based on this discussion thread: zulip#5489 (comment)

reactUtils [nfc]: Convert an arrow function into a regular declaration

b44f14e

useFetchedDataWithRefresh: Add, to use for read-receipts data soon

a73285b

WebLink [nfc]: Make into formatting wrapper, using style inheritance

7c7b8c2

i18n types: Add FormatXmlElementFn, for "rich text formatting" feature

51a4ecb

The docs are spotty on this feature, but it seems powerful and worth a try. We'll use it to put a web link on a single word in a translated message, in the style chosen by the web app.

chrisbobbe force-pushed the pr-read-receipts branch from 86ea850 to adc04bf Compare October 19, 2022 05:56

chrisbobbe and others added 3 commits October 19, 2022 10:05

api [nfc]: Use kFooBar naming instead of FOO_BAR, for constants

468ac71

ReadReceiptsScreen: Add, with TODO to show data state more visibly

8caf6dc

This makes it possible to view read receipts. Fixes: zulip#5367

api [nfc]: Expand comments on kNotificationBotEmail

868ba9c

gnprice force-pushed the pr-read-receipts branch from adc04bf to 868ba9c Compare October 19, 2022 17:09

gnprice merged commit 868ba9c into zulip:main Oct 19, 2022

chrisbobbe deleted the pr-read-receipts branch October 24, 2022 06:40

This was referenced Nov 17, 2022

action-sheets types: Make Button's title/errorMessage LocalizableText #5559

Merged

action-sheets: Use navigation object instead of NavigationService #5560

Merged

gnprice mentioned this pull request Dec 29, 2022

tools/test: Add intl suite #5476

Merged

		* True just when a given state/prop/etc. has been true continuously for the
		* past `duration`.

		* True just when a given state/prop/etc. has not changed for the past
		* `duration`.

Uh oh!

Add UI to view read receipts #5489

Add UI to view read receipts #5489

Uh oh!

Conversation

chrisbobbe commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Sep 15, 2022

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!

chrisbobbe Sep 16, 2022 • 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!

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 Sep 16, 2022

Uh oh!

gnprice commented Oct 8, 2022

Uh oh!

alya commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Oct 12, 2022

Uh oh!

chrisbobbe commented Oct 19, 2022

Uh oh!

gnprice commented Oct 19, 2022

Uh oh!

chrisbobbe commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Sep 15, 2022 •

edited

Loading

chrisbobbe Sep 16, 2022 •

edited

Loading

alya commented Oct 12, 2022 •

edited

Loading