Page MenuHomePhabricator

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

Authored by kamil on Jan 22 2024, 2:58 AM.
Tags
None
Referenced Files
F3276042: D10760.diff
Sat, Nov 16, 7:17 AM
Unknown Object (File)
Wed, Nov 6, 6:56 PM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Unknown Object (File)
Oct 15 2024, 11:17 AM
Subscribers

Details

Summary

Convert logic to ops approach.

Depends on D10759

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

Diff Detail

Repository
rCOMM Comm
Branch
publish-ks-sql-2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jan 22 2024, 6:34 AM
This revision is now accepted and ready to land.Jan 22 2024, 9:22 AM
ashoat requested changes to this revision.Jan 22 2024, 9:52 PM

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_keyservers 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
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

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?

This revision is now accepted and ready to land.Jan 27 2024, 3:03 PM