Page MenuHomePhabricator

[native] [refactor] use toggle unread hook
ClosedPublic

Authored by benschac on Mar 14 2022, 8:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Sun, Nov 10, 12:10 AM
Unknown Object (File)
Fri, Nov 8, 7:42 AM

Details

Summary

Use useToggleUnreadStatus hook, remove old API call logic.

Screen Recording 2022-03-15 at 12.47.05 PM.gif (851×407 px, 3 MB)

Test Plan

Go to the simulator:

  • Read and unread chat thread items.
  • Make sure animations are still smooth.
  • Go to web client and API call (that toggling read and unread) is still reflected on both native client and web client.

Diff Detail

Repository
rCOMM Comm
Branch
new-update-hook
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

benschac edited the summary of this revision. (Show Details)
ashoat requested changes to this revision.Mar 14 2022, 9:11 PM
ashoat added inline comments.
native/chat/swipeable-thread.react.js
47 ↗(On Diff #10330)

You're directly calling close() in the render path

This revision now requires changes to proceed.Mar 14 2022, 9:11 PM
benschac edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Mar 15 2022, 8:26 PM

The diff is really confusing, the behavior is hard to understand and reason about, and it breaks Hooks rules

native/chat/swipeable-thread.react.js
48

This is a really confusing, unclear line. You should separate it out into two to make it more clear

48

You can't just be setting local variables like that willy-nilly from a callback. If you have some state you want to track inside a Hook, you must use useRef or useState

48

It's really weird to be setting this callback to undefined from within the callback. Even if you were doing it properly with a ref or state, it's still a confusing pattern. You should have the callback defined as a const using React.useCallback (standard), and instead have some boolean React.useRef that tracks whether a callback should be passed to useToggleUnreadStatus

This revision now requires changes to proceed.Mar 15 2022, 8:26 PM
benschac added inline comments.
native/chat/swipeable-thread.react.js
48

You're right. This was an oversight. When moving the current.close into a callback I forgot to remove the swipeableClose variable assignment.

48

Agreed. 200%, mutating state like this is awful, I should have caught it.

48

Agreed. This was an oversight.

I don't think I need to track if the callBack was passed in or not.

benschac edited the summary of this revision. (Show Details)
benschac edited the test plan for this revision. (Show Details)

diff review fix

ashoat requested changes to this revision.Mar 16 2022, 9:06 PM
ashoat added inline comments.
lib/hooks/toggle-unread-status.js
19 ↗(On Diff #10430)

Now that you're no longer setting the callback to undefined, there's no need to make useToggleUnreadStatus support an undefined / null callback

This revision now requires changes to proceed.Mar 16 2022, 9:06 PM
ashoat requested changes to this revision.Mar 17 2022, 12:52 PM
ashoat added inline comments.
lib/hooks/toggle-unread-status.js
43 ↗(On Diff #10479)

We don't need this change here, let's avoid adding to the Git history for this file unless we really need to

native/chat/swipeable-thread.react.js
46 ↗(On Diff #10479)

This accidentally got pulled back in

This revision now requires changes to proceed.Mar 17 2022, 12:52 PM
This revision is now accepted and ready to land.Mar 20 2022, 8:53 PM