Page MenuHomePhabricator

[web] Fix a bug where incorrect navigation state prop was used in navigation panel
ClosedPublic

Authored by tomek on May 19 2022, 7:04 AM.
Tags
None
Referenced Files
F3262104: D4091.id.diff
Fri, Nov 15, 11:33 PM
Unknown Object (File)
Thu, Nov 14, 10:22 AM
Unknown Object (File)
Thu, Nov 14, 10:22 AM
Unknown Object (File)
Thu, Nov 14, 10:22 AM
Unknown Object (File)
Thu, Nov 14, 10:22 AM
Unknown Object (File)
Fri, Nov 8, 9:55 AM
Unknown Object (File)
Sat, Nov 2, 3:34 AM
Unknown Object (File)
Mon, Oct 28, 3:03 AM

Details

Summary

This bug manifested itself when adding danger zone - the second navigation item on settings page. The issue was that we were always using state.navInfo.tab to determine which navigation item should be highlighted. This approach worked correctly in app switcher and appeared to work correctly in settings switcher - but only because there was only one item.

The solution to this issue is to allow deciding which part of the state should be used. For the settings switcher the correct state is located at state.navInfo.settingsSection. It is implemented by adding a new prop to NavigationPanel.Container which is a function that takes the AppState and returns the current nav panel.

This works great and is easy to use but has one big issue. We no longer can safely type NavigationPanelItemProps as tab prop now depends on tabSelector return value. From high level, what we want is to have a parent-children relationship where both parent and children are generic components using the same type parameter. I tried solving that https://flow.org/try/#0JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcARFDvmQNwBQdMAnmFnAIJhgDKMyMbALxwA3gF96jFmwByyAG7AA5v2AQAdgAVk6rABsAkgJCbiYVAB4AKgD44wkXThwA1HwBGALjhWANE9dcAAtgPQATSnVvbDwYADoZCDCsfwkGAgBXdXw1dTg5RRUYXO1dQ2NrGwAKALAzVG8C5VUNUv0jLBN6yv8ASmiqeMTk0QDcDVR4EThg0IisPLF7ODqIc3pnShgMqDzZ8Mj6MQZmVnyFZuLWnX0AYQ0+YF0oUzXLW2XHZzdkd259LD4aDeTg8PgCex2PwBFz7eZRTCDOK3EIHBbsKBQZBMCwAfhi+DiAFE9J0FjALKcsBACOdCi0tDdyp1KjYbKlJAB6TlwW46dQQeBKLDwOFxVbmOIeODuQHIDKoNgSrCwJhwaXAdAgTWoJ5KOBPODagAeWDCUjOTSKJSZ93Uj2er3MACZKp8YR5-qSgVAQVxePwhFD-N84ZEBrFkaj4RisTj8UiSWT7RYAs4CfF7uANOSrNILFaGW1mV03qyQ3A2Ry6EA but without success. I also found an issue https://github.com/facebook/flow/issues/8582 that indicates that other people have the same problem.

For now, I decided that using string is better than having generic NavigationPanelItem<T> with typeof NavigationPanelItem because that would effectively mean we're using any.

Test Plan

Open chat and switch apps a couple of times - every time the current tab should be highlighted. Open settings http://localhost/comm/settings/account/ and verify that the account tab is highlighted.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.May 19 2022, 7:08 AM

Seems to work correctly.

Did you consider putting the selectors inside e.g. nav-selectors.js? Probably it doesn't matter, and now they are close to the place where they are used, but maybe it would be better to leave *.react.js files just for react components?

Is there currently any way to open account settings by navigating in the app, or will it be possible just after all finishing the whole account panel?
If no, maybe in such cases in the future it would be good to include the path like http://localhost/comm/settings/account/ in the test plan to make the test a bit easier?

This revision is now accepted and ready to land.May 19 2022, 8:40 AM

Thanks for the thoughtful explanation on the type issue, @palys-swm! Linking flow.com/try is very helpful... saves me the effort of trying the same thing :)

Did you consider putting the selectors inside e.g. nav-selectors.js? Probably it doesn't matter, and now they are close to the place where they are used, but maybe it would be better to leave *.react.js files just for react components?

I was considering it but wasn't sure. After giving it a second thought, it makes sense!

Is there currently any way to open account settings by navigating in the app, or will it be possible just after all finishing the whole account panel?

Danger zone is the last part required before we can replace the old settings. So it will be available soon.

If no, maybe in such cases in the future it would be good to include the path like http://localhost/comm/settings/account/ in the test plan to make the test a bit easier?

Yeah, I can do that if that's helpful.

Move selectors to nav-selectors

This revision now requires review to proceed.May 20 2022, 8:41 AM

Looks good to me. Also arc patchd the diff and confirmed that it worked as expected:

9eef.png (1×1 px, 85 KB)


Adding @ashoat as blocking reviewer since this diff involves flow.

This revision is now accepted and ready to land.May 20 2022, 12:02 PM
This revision now requires review to proceed.May 20 2022, 12:02 PM

I haven't looked too deeply myself, but as noted above @palys-swm's exploration looks thorough and he tried exactly the thing I would've tried, plus he found other people reporting the same problem. This is a case where detailed notes by the diff author save the reviewer(s) a lot of time

This revision is now accepted and ready to land.May 20 2022, 2:13 PM