Page MenuHomePhabricator

[lib] refactor keyserver-related actions in `master-reducer` to ops
ClosedPublic

Authored by kamil on Feb 2 2024, 3:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Mon, Dec 23, 9:52 PM
Unknown Object (File)
Wed, Dec 11, 10:44 PM
Unknown Object (File)
Wed, Nov 27, 10:58 AM
Subscribers

Details

Summary

Convert logic to ops approach.

Depends on D10933

Test Plan

change logic to make those condition truthy and check if state was properly set

Diff Detail

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

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Feb 2 2024, 3:30 AM
kamil added inline comments.
lib/reducers/master-reducer.js
142 ↗(On Diff #36583)

ops are added to storeOperations in D10785

inka requested changes to this revision.Feb 5 2024, 2:14 AM
inka added inline comments.
lib/reducers/master-reducer.js
138–144 ↗(On Diff #36583)

It seems wrong to change the keyserver store on every action, even if no actual changes are being made. This means everything that depends on the keyserver store will be re-rendered all the time.
Can we only change the keyserverStore if replaceOperations.length > 0?

Actually, why aren't we doing that with all ops?

This revision now requires changes to proceed.Feb 5 2024, 2:14 AM
lib/reducers/master-reducer.js
138–144 ↗(On Diff #36583)

It's probably because we don't pass the whole keyserver store to processStoreOperations. If we did that, we would naturally return the value that was passed in, if the array of ops is empty.

address review

lib/reducers/master-reducer.js
138–144 ↗(On Diff #36583)

It seems wrong to change the keyserver store on every action, even if no actual changes are being made. This means everything that depends on the keyserver store will be re-rendered all the time.
Can we only change the keyserverStore if replaceOperations.length > 0?

Good call with that, updating

Actually, why aren't we doing that with all ops?

It's probably because we don't pass the whole keyserver store to processStoreOperations. If we did that, we would naturally return the value that was passed in, if the array of ops is empty.

We modify the state the same way it was modified in previous logic, the code is just moved to processStoreOperations function - I went through the stack and the only difference is here: D10760 where we should return the old state when keyserverUpdated = false, not processing ops with an empty array. But since we have ops we can easily check if something changed and optimize something a bit, I could put up a diff refactoring processStoreOperations to return the whole keyserver store.

lib/reducers/master-reducer.js
138–144 ↗(On Diff #36583)

I could put up a diff refactoring processStoreOperations to return the whole keyserver store.

I think that would be better. But if, as you say, the logic didn't change, it was just moved, I think we can do it as a followup. Up to you

This revision is now accepted and ready to land.Feb 6 2024, 3:10 AM
lib/reducers/master-reducer.js
138–144 ↗(On Diff #36583)