Page MenuHomePhabricator

[native] Blocking navigation in edit mode in ChatRouter
ClosedPublic

Authored by kuba on Apr 26 2023, 4:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 8:55 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Unknown Object (File)
Wed, Apr 24, 7:27 AM
Subscribers

Details

Summary

We prevent user from i.g. changing thread when we are in edit mode.

Test Plan

Run the app on iOS and Android:

  • checked if we can't navigate when we are in edit mode,
  • checked if we can navigate after leaving exit mode.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
native/chat/chat-input-bar.react.js
883–887 ↗(On Diff #27920)

Can we merge these to make the code shorter?

native/navigation/nav-selectors.js
334 ↗(On Diff #27920)

Why not cast to RemoveEditMode?

kuba marked an inline comment as done.

Address review & fix issue with nested threads (unblocking the navigation didn't work correctly)

native/navigation/nav-selectors.js
334 ↗(On Diff #27920)

I tried doing that, but Flow blocks it:

const removeEditMode: RemoveEditMode = messageListRoute.params.removeEditMode;

Cannot assign `messageListRoute.params.removeEditMode` to `removeEditMode` because  mixed [1] is incompatible with  `RemoveEditMode` [2].Flow(incompatible-type)

This is the only workaround I've found.

tomek requested changes to this revision.Jun 23 2023, 5:39 AM

Overall, looks ok, but I have a couple of questions.

native/chat/chat-input-bar.react.js
414 ↗(On Diff #28042)

For me it feels a little more natural to have previous state check on the left hand side - we're used to reading left to right

754–756 ↗(On Diff #28042)

Why do we early exit here?

905 ↗(On Diff #28042)

Is it a good idea to store a function in navigation?

native/chat/chat-router.js
137 ↗(On Diff #28042)

Why do we need this condition?

140 ↗(On Diff #28042)

This is a potential bug, really hard to debug. Not sure how it works for navigation, but for redux, dispatching an action from a reducer causes some state updates to get lost. In this case it should be probably fine, as we're returning lastState (why?). But we should be really careful about it and verify this thoroughly.

native/navigation/nav-selectors.js
334 ↗(On Diff #27920)

Maybe we should somehow verify that messageListRoute is of correct type and its params are MessageListParams?

This revision now requires changes to proceed.Jun 23 2023, 5:39 AM
kuba marked 5 inline comments as done.

address review

native/chat/chat-input-bar.react.js
754–756 ↗(On Diff #28042)

Otherwise, the user would lose his draft when he navigates away and discards his edit changes.

905 ↗(On Diff #28042)

It was suggested here.

The hard part here is how to handle the discard action... I took a look at the router code and couldn't find any options for dispatching an action from the router
One option would be to change disableNavigation: boolean into the dispatch function. That way we could call it directly from here
The downside is that React Navigation will complain that it can't serialize a function, but I think that should be fine. It only affects us in dev mode, where we persist the React Navigation state for developer convenience (so that if you refresh, you're still on the same page)

native/chat/chat-router.js
137 ↗(On Diff #28042)

Here, it only applies to adding screens to the stack. Removing screens from the stack is already handled by the beforeRemove event.

140 ↗(On Diff #28042)

We are returning lastState to prevent the navigation (docs).

tomek added 1 blocking reviewer(s): ashoat.
tomek added inline comments.
native/chat/chat-router.js
140 ↗(On Diff #28042)

And how about dispatching an action from within the "reducer"? Is it safe?

native/navigation/nav-selectors.js
320–322 ↗(On Diff #28101)

It might be more readable using function syntax

kuba marked an inline comment as done.

Change to function syntax

Remove calling dispatch from reducer

native/chat/chat-input-bar.react.js
889–892 ↗(On Diff #28138)

We return here true because we want to dispatch this action immediately. In that way, we don't need to dispatch the action here, because it is handled by the router.

native/chat/chat-router.js
140 ↗(On Diff #28042)

Great catch. It wasn't safe. In the updated version, there is a slightly different approach, which addresses this issue.

tomek added inline comments.
native/chat/chat-input-bar.react.js
883–884 ↗(On Diff #28138)

Wondering how maintainable this boolean return is. From a caller's point of view, it isn't obvious what this value mean. We can address that by changing function name to something more explicit, or by changing return value to e.g. 'ignore_action' | 'reduce_action'. What do you think?

ashoat requested changes to this revision.Jun 27 2023, 7:48 PM

This diff is doing a lot of things, and could probably be decomposed in four different diffs. At this point we don't have time for that unfortunately.

I am not really sure that the changes are both complete and necessary, so I will have to rely on your test plan. Can you extend the Test Plan to mention the various different things you tested to conclude each change was necessary?

native/chat/chat-input-bar.react.js
877 ↗(On Diff #28138)

Why don't we do anything if the route isn't focused?

883–884 ↗(On Diff #28138)

I agree

897 ↗(On Diff #28138)

The definition of this function introduces an unnecessary function alias. Can you please remove the unnecessary alias using this patch?

diff --git a/native/utils/edit-messages-utils.js b/native/utils/edit-messages-utils.js
index c85f888bb..dc066b031 100644
--- a/native/utils/edit-messages-utils.js
+++ b/native/utils/edit-messages-utils.js
@@ -14,9 +14,7 @@ function exitEditAlert(onDiscard: () => void): void {
       {
         text: 'Discard edit',
         style: 'destructive',
-        onPress: () => {
-          onDiscard();
-        },
+        onPress: onDiscard,
       },
     ],
   );
1359 ↗(On Diff #28138)
  1. Are you sure that ChatInputBar will re-render when focus changes? The ReactNav docs don't mention this approach, and instead indicate you should use useIsFocused(). However, the docs warn:

The useIsFocused hook will cause our component to re-render when we focus and unfocus a screen. Using this hook component may introduce unnecessary component re-renders as a screen comes in and out of focus. This could cause issues depending on the type of action we're calling on focusing. Hence we recommend to use this hook only if you need to trigger a re-render.

  1. Since you only need "spot checks" and don't need to rerender when things change, it seems like you could pass the isFocused function itself to the component, and call it when necessary to get the current focus state. I'm not sure this will work, but I suspect if the current code has an issue, that this might help.

Can you try to confirm if a change in isFocused triggers a rerender, and if not can you try pass the isFocused function in directly?

native/chat/chat-router.js
137 ↗(On Diff #28138)

Why are we only blocking forward navigation, and not blocking backwards navigation?

This revision now requires changes to proceed.Jun 27 2023, 7:48 PM
native/chat/chat-router.js
137 ↗(On Diff #28138)

Ah, I guess it's handled with the beforeRemove event. Can you add a code comment explaining that? Alternately, it might more simple to use the same mechanism for blocking all navigation, instead of splitting into two codepaths.

native/chat/chat-input-bar.react.js
897 ↗(On Diff #28138)

Looks like this is done in D8271

native/chat/chat-input-bar.react.js
1359 ↗(On Diff #28138)

We can also consider using useFocusEffect but that might be impossible as most of the component is written using a class

tomek requested changes to this revision.Jun 29 2023, 6:34 AM
tomek added inline comments.
native/chat/chat-input-bar.react.js
1359 ↗(On Diff #28138)

My testing seems to confirm that every time focus change a component is rerendered. But The warning won't be there if that was guaranteed, so it's safer to avoid relying on this.

Passing isFocused function is harder, as we're now checking not if a screen is currently focused, but also if it was focused previously.
It seems possible to avoid relying on prev is focused value - we could use something from state instead.

Still, if a rerender isn't guaranteed, passing this function won't change anything, as we need componentDidUpdate to be called.

kuba marked 5 inline comments as done.

Address review: changed removeEditMode return type

native/chat/chat-input-bar.react.js
877 ↗(On Diff #28138)

I must have added it when I was adding it to the blockNavigation function.

It is unnecessary here because we don't block the navigation when we are not focused.

I have removed it.

kuba added inline comments.
native/chat/chat-input-bar.react.js
1359 ↗(On Diff #28138)

Not sure if this is the correct approach:

I added listeners for blur and focus events (docs) and moved the logic to them.

native/chat/chat-router.js
137 ↗(On Diff #28138)

I have a quick try to remove the beforeRemove listener and block it here. Simply changing that causes an infinite loop of alerts (pressing any button on the alert opens the new one). I will add a comment.

kuba marked 2 inline comments as done.

Added blur and focus listeners

Great work – I think this solution looks almost perfect! Can you make sure to address the inline comments before landing? If there's any ambiguity, or if my suggestion doesn't work for some reason, please feel free to re-request review

native/chat/chat-input-bar.react.js
198–199 ↗(On Diff #28346)

Might be nice to order these in the same order as the invocations below (with the new ones below the old one)

911–912 ↗(On Diff #28346)

This change will allow us to get rid of the isFocused prop. We're only doing spot checks here, so we don't need it

1382 ↗(On Diff #28346)

We don't need this prop anymore, and I'm skeptical that it can be relied on anyways. Can you remove it?

native/chat/chat-router.js
137 ↗(On Diff #28138)

Thanks for explaining!

kuba marked 3 inline comments as done.

Address review

ashoat added inline comments.
native/chat/chat-input-bar.react.js
910 ↗(On Diff #28391)

Nit: shorthand

kuba marked an inline comment as done.

Shorthand

tomek added inline comments.
native/chat/chat-input-bar.react.js
347 ↗(On Diff #28432)

We can early exit when !navigation. By doing that we should be able to avoid invariants

This revision is now accepted and ready to land.Jul 6 2023, 5:19 AM
kuba marked an inline comment as done.

Early exit in componentDidMount