Details

Reviewers

inka
ashoat

Commits

rCOMM505ab5e9cc2f: [lib] refactor set new session in `keyserver-reducer` to ops

Summary

Convert logic to ops approach.

Depends on D10759

Test Plan

Refresh website on web (it cause sessionChange with defined cookie).
Make sure state changes the same way as before refactor.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Jan 22 2024, 2:58 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 22 2024, 2:58 AM

kamil added a child revision: D10761: [lib] refactor login action in `keyserver-reducer` to ops.Jan 22 2024, 3:02 AM

Harbormaster completed remote builds in B25983: Diff 35915.Jan 22 2024, 3:35 AM

kamil published this revision for review.Jan 22 2024, 6:34 AM

inka accepted this revision.Jan 22 2024, 9:22 AM

This revision is now accepted and ready to land.Jan 22 2024, 9:22 AM

It's not good that my review was necessary here. Both @kamil and @inka should think about how this issue was missed. We need to be more careful and thoughtful when submitting and reviewing code. I should not have to be a phantom reviewer for every diff in the codebase, but it often feels like if I wasn't here, things would get missed...

lib/reducers/keyserver-reducer.js
129 ↗	(On Diff #35915)	In the old logic, the second replacement would keep the updated cookie from the first replacement. After your changes, it looks like the updated cookie is discarded, since the second`replace_keyserver` pulls from the non-updated `state` version (where previously it was updated). I think we should avoid queueing up two `replace_keyserver`s for the same keyserver. Can you coalesce the changes into a single op?

This revision now requires changes to proceed.Jan 22 2024, 9:52 PM

address review

minor fix

kamil added inline comments.Jan 27 2024, 9:02 AM

lib/reducers/keyserver-reducer.js
134 ↗	(On Diff #36216)	we could use _isEqual and depending on that return operation or empty array but not sure if it is worth
129 ↗	(On Diff #35915)	I completely overlooked this... Thanks for catching

Harbormaster completed remote builds in B26222: Diff 36215.Jan 27 2024, 9:17 AM

Harbormaster completed remote builds in B26223: Diff 36216.

ashoat accepted this revision.Jan 27 2024, 3:03 PM

ashoat added inline comments.

lib/reducers/keyserver-reducer.js
134 ↗	(On Diff #36216)	I actually think it's worth skipping the op, but `_isEqual` seems like overkill. Maybe just make a variable `let keyserverUpdated = false` and set it in the two conditions?