Page MenuHomePhabricator

[native] Update the existing Context API for Markdown to encompass AppNavigator
ClosedPublic

Authored by rohan on Nov 1 2022, 5:45 AM.
Tags
None
Referenced Files
F3376959: D5515.id18028.diff
Wed, Nov 27, 3:05 AM
F3376951: D5515.id18787.diff
Wed, Nov 27, 3:01 AM
F3376926: D5515.id18281.diff
Wed, Nov 27, 2:56 AM
F3376807: D5515.id18740.diff
Wed, Nov 27, 2:19 AM
F3376795: D5515.id19051.diff
Wed, Nov 27, 2:14 AM
F3376758: D5515.id18009.diff
Wed, Nov 27, 2:01 AM
F3374930: D5515.vs18028.id18032.diff
Tue, Nov 26, 5:23 PM
F3374929: D5515.id18008.diff
Tue, Nov 26, 5:23 PM

Details

Summary

This diff handles refactoring the existing MarkdownContext API to encompass AppNavigator - this means that when each text-message now uses the MarkdownContext, that context is not unique-per-message. To solve this, the methods in the API take in messageID parameters, that maps a message to a boolean state in the Provider. The following was done in this diff in order to replicate existing behavior:

  1. Update the existing Context in markdown-context.js to use React.useState so we can update the state of a message and re-render automatically. This is of type Object, where the keys are messageIDs and the values are booleans.
  2. Introduce a clearMessageData method to clear data on unmount
  3. Create a markdown-context-provider.react.js. This allows the Context to track what message is being pressed and if it should be blocked from the tooltip appearing.
  4. Use this Context in ConnectedTextMessage to adjust linkIsBlockingPresses
  5. Pass in the messageID to MarkdownLink through a MessageContext so onPress, we can call setLinkModalActive or setLinkPressActive with the specific messageID being tapped
  6. Encompass App.Navigator within MarkdownContextProvider

This is Step 3 of https://linear.app/comm/issue/ENG-2072/maintain-the-spoiler-state-upon-opening-textmessagetooltipmodal

Depends on D5446

Test Plan
  1. Checkout local master and test link taps and when the tooltip appears.
  2. Switch to this spoiler working state and simulate the same behavior to check that the tooltip does not appear when a link is touched.
  3. Behind the scenes, also log the Objects from the Provider useState hook in order to check that the boolean states are being updated for the correct Message.
  4. After that, check that the clearMessageData works as expected and data does not get persisted when a chat is swiped out of, and then re-entered.

Diff Detail

Repository
rCOMM Comm
Branch
spoiler
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision is now accepted and ready to land.Nov 7 2022, 7:26 AM

(You'll also want to git pull --rebase and run arc diff again to address issue with Android CI)

native/chat/text-message.react.js
219–220

linkModalActive and linkPressActive shouldn't have a key for id if a thread is opened for the first time and the message has not been clicked - this allows us in a condition where we have (null || null) ?? false to default to the boolean false. That's my understanding (feel free to correct!)

Rebase for CI & address comments

Rebase and resolve conflicts

ashoat requested changes to this revision.Nov 21 2022, 11:02 AM
ashoat added inline comments.
native/chat/text-message.react.js
222–223 ↗(On Diff #18281)

Not all messageInfos have id set – see here

This invariant triggers as soon as the user sends a message

You should be using messageID or messageKey functions here, depending on what you want to happen if both id and localID are set

This revision now requires changes to proceed.Nov 21 2022, 11:02 AM

Simplify

native/chat/text-message.react.js
247–251 ↗(On Diff #18281)

Use messageKey() instead of extracting the id directly from the props,
and also simplify the clearMarkdownContextData` call

native/chat/text-message.react.js
249 ↗(On Diff #18699)

In D5541, this is eventually moved to InnerTextMessage - for this diff though, I think it makes more sense to keep it here in order to not "combine" the two diffs and to isolate this diff to one specific task.

This revision is now accepted and ready to land.Nov 21 2022, 6:43 PM
ashoat requested changes to this revision.Nov 22 2022, 6:11 AM
ashoat added inline comments.
native/chat/text-message.react.js
249 ↗(On Diff #18699)

Actually let's move the MessageContext.Provider in this diff. There's no reason D5541 needs to exist... it's the very next diff in the stack, and all it's doing is moving where this MessageContext.Provider is being rendered. Forcing your reviewers to have to review both of these diffs is wasting their time... please just squash the two commits together, update this diff, and abandon D5541

This revision now requires changes to proceed.Nov 22 2022, 6:11 AM
native/markdown/markdown-context-provider.react.js
33–35 ↗(On Diff #18699)

These setters don't need to update the whole state, do they? Why not just have the function take two things: the key as well as the new value for that key. Then all of the callers of these setters won't need to get the current state of the map.

As an example, take useDisplayLinkPrompt. Instead of this:

const setLinkModalActive = markdownContext?.setLinkModalActive;
const linkModalActive = markdownContext?.linkModalActive;
const onDismiss = React.useCallback(() => {
  if (linkModalActive) {
    setLinkModalActive?.({ ...linkModalActive, [messageKey]: false });
  }
}, [setLinkModalActive, linkModalActive, messageKey]);

We could have this:

invariant(markdownContext, "markdownContext should exist");
const { setLinkModalActive } = markdownContext;
const onDismiss = React.useCallback(() => {
  setLinkModalActive(messageKey, false);
}, [setLinkModalActive, messageKey]);
native/navigation/app-navigator.react.js
107 ↗(On Diff #18699)

This isn't high enough in the hierarchy. It needs to be an ancestor of ChatContextProvider, since that component renders a ChatItemHeightMeasurer which will render InnerTextMessage et. al. Let's move this to root.react.js

(Your functionality shouldn't have any bearing on the height of the chat item, but it's still better to make your context accessible even from there, so that the downstream code can properly assume that the context will always be available and doesn't have to do any conditional checks)

Thanks for the thorough review @ashoat. I'm going to put up a revision to address relocating MarkdownContextProvider to root.react.js to be an ancestor of ChatContextProvider and include the move from using MessageContext in text-message.react.js to inner-text-message.react.js.

I've left a response to your comment about the setters for link clicks, and I'll leave this diff available for a revision if my reasoning doesn't justify.

native/chat/text-message.react.js
249 ↗(On Diff #18699)

Makes sense, I've abandoned D5541 and will put the changes here in the next revision.

native/markdown/markdown-context-provider.react.js
33–35 ↗(On Diff #18699)

My reasoning for wanted the setters to specifically force a state update was for the need for linkIsBlockingPresses in TextMessage (https://github.com/CommE2E/comm/blob/rohan/native-spoilers/native/chat/text-message.react.js#L228) - the branch isn't fully up to date but that line is still present.

My thoughts were that since we are determining whether linkIsBlockingPresses should be true or false based on whether the respective messageKey is present and true in, for example, linkModalActive, once we modify linkModalActive (when the link is pressed), we should force an update so linkIsBlockingPresses can pull from the updated objects and pass as a prop to the TextMessage component.

native/navigation/app-navigator.react.js
107 ↗(On Diff #18699)

Ah, that would be my misunderstanding of the ancestry present, I've taken a deeper look through react dev tools and this comment makes more sense - it also explains the need for the optional chaining / checking in D5539. I'll go ahead and make this change, and then in D5539 I should be able to address the feedback as well afterwards

  1. Relocate MarkdownContextProvider to be an ancestor of ChatContextProvider.
  1. Use MessageContext solely in inner-text-message.react.js, instead of in text-message.react.js
ashoat requested changes to this revision.Nov 22 2022, 2:47 PM
ashoat added inline comments.
native/markdown/markdown-context-provider.react.js
33–35 ↗(On Diff #18699)

I don't understand what you're referring to with "specifically force a state update"... I don't think there is any such difference between your approach and my proposed approach. I am simply suggesting moving the spread operation upstream into setLinkModalActive instead of requiring callers to always have to do it themselves

This revision now requires changes to proceed.Nov 22 2022, 2:47 PM
native/markdown/markdown-context-provider.react.js
33–35 ↗(On Diff #18699)

Ah, in that case I agree - moving it upstream would probably make more sense so it's in one place. Just one clarifying thought after thinking about the design implementation a little bit, do we really need the ... operator in retrospect for links? I'm thinking we could just do the following since I'm starting to doubt if need to keep track of every single link in the thread.

setLinkModalActive?.({ [messageKey]: false });

Since in text-message.react.js we already default to false if messageKey is not present in the object:

const linkIsBlockingPresses =
      (linkModalActive[key] || linkPressActive[key]) ?? false;

Ah yeah I agree. You could probably just leave setLinkModalActive as-is and then call setLinkModalActive({ [messageKey]: true }) when setting and setLinkModalActive({}) when unsetting

native/markdown/markdown-link.react.js
13 ↗(On Diff #18740)

Does this need to be optional?

native/markdown/markdown-link.react.js
13 ↗(On Diff #18740)

Shouldn't have to, I'm going to fix it in the revision

native/markdown/markdown-link.react.js
13 ↗(On Diff #18740)

I think we should also be able to remove the optional chaining for MessageContext as well now that it's encompassing InnerTextMessage, as I took a look in dev tools and the hierarchy seems to be: MarkdownLink --> Markdown --> InnerTextMessage --> ...

native/markdown/markdown-link.react.js
13 ↗(On Diff #18740)

Yeah the optional chaining can be removed!

Update use of setLinkModalActive and setLinkPressActive, and remove
optional chaining for MessageContext

ashoat requested changes to this revision.Nov 23 2022, 8:56 AM
ashoat added inline comments.
native/markdown/markdown-link.react.js
19 ↗(On Diff #18784)

? can be removed here

67 ↗(On Diff #18784)

Here too

81 ↗(On Diff #18784)

Here too

This revision now requires changes to proceed.Nov 23 2022, 8:56 AM

Removed missed optional chaining in MarkdownLink

I restarted both keyserver and native and I've been testing both existing messages and sending new messages, none of the invariants are being thrown from my end (in regards to Message/Markdown Contexts), so I think the new hierarchy is looking to be the right choice

This revision is now accepted and ready to land.Nov 23 2022, 10:11 AM

I ended up needing something like MessageContext in a stack I am working on to address the gestures/press experience issues, but I called it TextMessageMarkdownContext. Don't worry about addressing the comments below... I suspect you'll end up needing to rebase on top of my stack anyways, so you can just get rid of MessageContext when that happens

native/chat/message-context.react.js
9 ↗(On Diff #18787)

Line is too long

9 ↗(On Diff #18787)

Default should be null or undefined so that the invariant on React.getContext(MessageContext) has some meaning. Otherwise you could get a "silent failure" here where some downstream code thinks everything is fine but actually messageKey is an empty string and we're in a broken state

Rebase and resolve conflicts in root.react.js

I ended up needing something like MessageContext in a stack I am working on to address the gestures/press experience issues, but I called it TextMessageMarkdownContext. Don't worry about addressing the comments below... I suspect you'll end up needing to rebase on top of my stack anyways, so you can just get rid of MessageContext when that happens

Sounds good, thanks for the heads up

ashoat requested changes to this revision.Nov 30 2022, 11:12 AM

Let's rebase this on my stack and replace MessageContext with a new property on TextMessageMarkdownContext

This revision now requires changes to proceed.Nov 30 2022, 11:12 AM

Rebase (pending testing since my dev environment isn't working properly
again)

Accepting assuming you did the testing

native/markdown/markdown-link.react.js
58 ↗(On Diff #19035)

In what scenario will this not be set? Is it when it's rendering for height-measuring?

This revision is now accepted and ready to land.Nov 30 2022, 2:27 PM
native/markdown/markdown-link.react.js
58 ↗(On Diff #19035)

Yeah exactly from my understanding of the hierarchy.

Remove messageKey from inner-text-message.react.js

This revision was landed with ongoing or failed builds.Nov 30 2022, 3:37 PM
This revision was automatically updated to reflect the committed changes.