Details

Reviewers

ashoat
tomek
varun

Commits

rCOMM89cbd85b1c2a: [native] only display remove device button if current device is primary device

Summary

pretty self-explanatory. only the primary device should be able to remove other devices. we hide the remove device button if user is not on their primary device.

Test Plan

confirmed that the button only appears if i'm on my primary device

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Sep 6 2024, 10:45 AM

varun added a parent revision: D13125: [native] add ability to remove devices from linked devices bottom sheet.Sep 6 2024, 10:46 AM

Harbormaster completed remote builds in B31467: Diff 43945.Sep 6 2024, 11:02 AM

varun requested review of this revision.Sep 6 2024, 11:02 AM

bartek added inline comments.Sep 9 2024, 12:22 AM

native/profile/linked-devices-list-item.react.js
20–22	Can't you reuse `isPrimary`? Their values should be equal
native/profile/linked-devices.react.js
38–40	Not sure which way of determining primary device is better: The one based on index (simple) The one you introduced with effect (I guess more reliable?)
65	Nit, kinda unrelated: if we need only deviceID, `getContentSigningKey()` doesn't require Identity context

ashoat requested changes to this revision.Sep 9 2024, 12:43 PM

ashoat added inline comments.

native/profile/linked-devices-bottom-sheet.react.js
142	Should be avoided for the same reasons described for ternary here
native/profile/linked-devices.react.js
38–40	I think just using the index is better

This revision now requires changes to proceed.Sep 9 2024, 12:43 PM

bartek commandeered this revision.Oct 17 2024, 3:16 AM

bartek edited reviewers, added: varun; removed: bartek.

Removed ternary from JSX
Fixed logic to disallow removing self device

bartek added inline comments.Oct 17 2024, 7:24 AM

native/profile/linked-devices-list-item.react.js
39	I'd exit early here Currently, when selected device isn't removable (primary device), an empty bottom sheet is displayed and it looks broken. Instead, it would be better not to open it, or at least display some placeholder text.

bartek marked an inline comment as not done.Oct 17 2024, 7:24 AM

Harbormaster completed remote builds in B32297: Diff 45267.Oct 17 2024, 7:40 AM

bartek added a child revision: D13747: [native] Improve device labels on device list screen.Oct 17 2024, 7:43 AM

ashoat accepted this revision.Oct 17 2024, 9:01 AM

ashoat added inline comments.

native/profile/linked-devices.react.js
25 ↗	(On Diff #45267)	Given we only need `index` to check if the device is primary, I think it would be more readable to determine that at the callsite, and then pass in `isPrimary`
41 ↗	(On Diff #45267)	You could use the `isPrimary` variable here
62–64 ↗	(On Diff #45267)	When you have two states that are updated at the same time and read from the same place, it's good practice to combine them into one

This revision is now accepted and ready to land.Oct 17 2024, 9:01 AM

ashoat added inline comments.Oct 17 2024, 9:02 AM

native/profile/linked-devices-list-item.react.js
39	Good catch – yeah, I think we should do this

bartek mentioned this in D13125: [native] add ability to remove devices from linked devices bottom sheet.Oct 18 2024, 2:22 AM

Feedback: Added the early-exit condition, merged useState, simplified variables

Harbormaster completed remote builds in B32303: Diff 45274.Oct 18 2024, 3:04 AM

ashoat added inline comments.Oct 18 2024, 6:28 AM

native/profile/linked-devices.react.js
30 ↗	(On Diff #45274)	Should we do the same and replace `thisDeviceID` with `isThisDevice`?
48–49 ↗	(On Diff #45274)	React state should always be read-only. We should never allow mutating the state object directly, as this will not trigger a rerender

Due to issues in D13125, I ended up checking out this stack locally. While testing I found the experience confusing due to inconsistency between the behavior of primary and non-primary device pills

native/profile/linked-devices-list-item.react.js
40–42 ↗	(On Diff #45274)	It's weird that this button remains pressable even though it does nothing. Can you make the `TouchableOpacity` no longer pressable when `!shouldAllowDeviceRemoval`, eg. with `disabled={true}`?
96 ↗	(On Diff #45274)	I think it's confusing to the viewer that most of these pills can be pressed, but the one for the primary device can't be. There's no indication to explain why the primary device pill doesn't respond to presses, despite the other pills responding. What do you think of adding a pencil icon to the right of the pills that can be edited? We could use the same `SWMansionIcon` that we use in `EditSettingButton`.