Refactor selected note state from flux to redux #1851

belcherj · 2020-01-21T20:12:54Z

Fix

This PR moves the selected note state from flux to redux. It also eliminates
the need for selectedNoteId in appState.

Test

Test the note selection flow
Tets the remote note change flows
Test note share flow
Test note publish
Test trash flow
Test tag selection
Test search

Review

Only one developer is required to review these changes, but anyone can perform the review.

Release

RELEASE-NOTES.txt was updated with:

Refactored selected note state 1851

lib/note-editor/index.tsx

lib/state/ui/middleware.ts

dmsnell

You picked a hard one to start with 😀

I know we have some poor examples in the code already where our action creators are imperative and they suggest that we should move more logic into the middleware but I don't think there's any need in this PR to make any changes to the middleware - it looked like all of the code there could exist solely in the selectedNote reducer.

If that doesn't make sense I'd be happy to delve further into it but would you mind trying to make that change here? It will probably feel better after that.

lib/state/ui/reducer.ts

lib/dialogs/share/index.tsx

lib/note-list/index.tsx

lib/note-toolbar/index.tsx

lib/state/action-types.ts

lib/state/index.ts

lib/state/ui/middleware.ts

lib/state/ui/reducer.ts

lib/flux/app-state.ts

lib/note-info/index.tsx

lib/state/ui/reducer.ts

lib/types.ts

package.json

lib/state/index.ts

dmsnell

It looks like we need to rebase this ontop of the changes introduced in #1855.

Specifically noting the need in action-types where we're introducing a new string action-type export.

This PR moves the selected note state from flux to redux. It also eliminates the need for selectedNoteId in appState.

belcherj · 2020-01-29T14:57:19Z

@dmsnell This is ready for more review.

lib/app-layout/index.tsx

lib/note-editor/index.tsx

lib/state/ui/actions.ts

lib/app.tsx

- Add type annotations where we're introducing new state - Remove references to `noteUpdatedRemotely` - Remove ability to `selectNote(null)` - Remove unused `tags` prop

dmsnell · 2020-01-29T18:54:16Z

@belcherj I added a new commit to try out the changes I proposed.

Added type annotations where we're introducing new things
Renamed setSelectedNote to selectNote to focus more on the action we're doing rather than the data we're changing
Removed the tags prop
Removed lingering references to noteUpdatedRemotely

I'll be thoroughly testing this later on but I need a break from it at the moment

belcherj · 2020-01-30T19:39:24Z

I have tested this a bunch today. I think it is ready to roll.

dmsnell

Thanks for everything here; it's really exciting to see state finally cleaning up. According to your own testing please merge at your convenience.

In #1851 we refactored the selected note state but during testing missed a reference to `state.appState.note` which should have been updated to `state.ui.note`. Nothing crashed in the app because it was being used for its truthiness but the result was that when checking if Markdown was enabled for any given note the answer would always end up as "no." In this patch we're restoring the state reference which fixes that bug and restores the preview button in the note toolbar.

Resolves #1879 It appears that we removed a flow in #1851 which updated app state in response to updates from the note editor[1]. While this didn't impact the operation of the editor it did present itself as a bug when previewing markdown-rendered notes. The editor is stateful on its own and so continued to function independently of the state of the selected note's content. The preview, however, grabs the contents of the selected note whenever it's activated and then renders those into a DOM node. This means that it showed the old contents of the note unless you navigated to another note and back, which swapped out the selected note with the new updated contents. In this patch, which currently is probably wrong in some way, we're updating the selected note when calling `onChangeContent` so that we don't get out of sync with the data sources. Hopefully at the end of our state cleanup and stabilization project this will disappear but for now this fix catches the gap that left split data sources. Probably there were other bugs we didn't discover as a result of this. Mainly we need to test and audit for traces of cycles/loops in the update so that we don't start spinning or overwriting data we just entered. In my brief testing the easy cases worked as I expected them to simply and singly. [1]: https://github.com/Automattic/simplenote-electron/pull/1851/files#diff-3c87750a7cc32e0127447ffc78f5c0d5L300

* Fix: Update selected note when making changes in the editor view Resolves #1879 It appears that we removed a flow in #1851 which updated app state in response to updates from the note editor[1]. While this didn't impact the operation of the editor it did present itself as a bug when previewing markdown-rendered notes. The editor is stateful on its own and so continued to function independently of the state of the selected note's content. The preview, however, grabs the contents of the selected note whenever it's activated and then renders those into a DOM node. This means that it showed the old contents of the note unless you navigated to another note and back, which swapped out the selected note with the new updated contents. In this patch, which currently is probably wrong in some way, we're updating the selected note when calling `onChangeContent` so that we don't get out of sync with the data sources. Hopefully at the end of our state cleanup and stabilization project this will disappear but for now this fix catches the gap that left split data sources. Probably there were other bugs we didn't discover as a result of this. Mainly we need to test and audit for traces of cycles/loops in the update so that we don't start spinning or overwriting data we just entered. In my brief testing the easy cases worked as I expected them to simply and singly. [1]: https://github.com/Automattic/simplenote-electron/pull/1851/files#diff-3c87750a7cc32e0127447ffc78f5c0d5L300 * Handle tag updates too by always re-selecting note on updates from the server When tags were being updated in the pap they also came through this function but they weren't coming through `onUpdateContent`, meaning that our fix overlooked tags and probably also system tags and the like. In this patch we're no longer selectively updating the note if it has a remote patch (indicating that it came from another device). Instead, we're always updating and selectively setting `hasRemoteUpdate` based on that knowledge. This ensures that we maintain consistency with local updates.

belcherj added [status] needs code review labels Jan 21, 2020

belcherj requested a review from a team January 21, 2020 20:12

belcherj self-assigned this Jan 21, 2020