mute: Maintain muted-topics state by stream ID, not name#5223
mute: Maintain muted-topics state by stream ID, not name#5223gnprice merged 28 commits intozulip:mainfrom
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, glad to have this! See a few small comments below.
| origValuePrototype === (Object.prototype: $FlowIssue) | ||
| || origValuePrototype === (Array.prototype: $FlowIssue), | ||
| 'unexpected class', | ||
| `stringify: unexpected class: ${origValuePrototype.constructor.name}`, |
|
|
||
| case REGISTER_COMPLETE: | ||
| return action.data.muted_topics || initialState; | ||
| return convert(action.data.muted_topics ?? [], getStreamsByName(globalState)); |
There was a problem hiding this comment.
This is where the mute reducer depends on the state after another reducer has been applied (the streams reducer), right? Should we comment about that here? It looks like the requirement is covered in a new test ('when mute data is provided init state with it: end-to-end') so that's good.
I'm wary of falling into a pattern where some reducers depend on the output of other reducers when processing the same action. Taken too far, there's some risk of getting tangled up into a dependency cycle, right?
I don't see another good way of doing what we want to do here, though. Happily, this case doesn't seem like it'll hang around forever. Eventually, all supported servers will give us the data by ID and we can drop this dependency. Could add a comment to mention that, in src/boot/reducers.js, right? (Either now or later when we have a threshold for when servers start sending the new shape.)
There was a problem hiding this comment.
Good thought, yeah, this should have a comment.
I'm wary of falling into a pattern where some reducers depend on the output of other reducers when processing the same action. Taken too far, there's some risk of getting tangled up into a dependency cycle, right?
In principle, yes. I'm not too worried about this for a couple of reasons:
- I think there's a fairly natural rough layering of the things in the model: first of all there is realm-wide information like version, URL, and settings; then there are users, and streams; then relationships between those like subscriptions and muted-topics (and between users like muted-users); then messages which are sent by users and to streams or users. So mostly we should be able to just follow that.
- The web app manages it. 🙂 Here's what that looks like -- it's actually mixed in with initializing all the UI, like setting click handlers and so on:
https://github.com/zulip/zulip/blob/8e0633578863a5c144ffce86b462d0b318282383/static/js/ui_init.js#L544-L629
If we do ever run into a cycle between these wanting to run after each other, like A depends on B depends on A, we can always resolve it by breaking up REGISTER_COMPLETE into two actions: A handles the first action to set up whatever it is B actually needs, then B runs, then A handles the second action to do whatever it needed B for. (And if there isn't such a way to break up what either A or B needs to do, then that'd be a more fundamental problem with whatever data-schema choices we were considering that led to that, and we'd figure out an alternative.)
Eventually, all supported servers will give us the data by ID and we can drop this dependency. Could add a comment to mention that, in src/boot/reducers.js, right? (Either now or later when we have a threshold for when servers start sending the new shape.)
It's true that will happen eventually. It'll be a couple of years at least, though -- the server doesn't support that yet, and won't in 5.0 which is about to come out, so 18 months after whenever we release 6.0. So it is important that this be a reasonable design for us to live with.
For the reasons above, though, I think having this kind of ordering is fine. It's natural for many things to depend on users and streams; also on the server version and realm URL, and potentially some server or realm settings. So we might add some more ordering like this, and might always have some.
| // No need for Immutable, because -- the server API actually doesn't support | ||
| // incremental changes! When something changes, it sends the entire new list. |
There was a problem hiding this comment.
No need for Immutable, because […]
I wondered! 😅
| for (const [topic, msgIds] of streamData) { | ||
| const isMuted = isTopicMuted( | ||
| streamId, | ||
| (subscriptionsById.get(streamId) || NULL_SUBSCRIPTION).name, |
There was a problem hiding this comment.
- […] NULL_SUBSCRIPTION […]🎉
| value = this.default(); | ||
| this.map.set(key, value); |
There was a problem hiding this comment.
Huh—😮—looking at the defaultdict doc, I guess this is what it does too.
I guess callers should be careful to make sure default_ stays pretty steady/predictable in what it returns across the lifetime of the DefaultMap. Otherwise I could imagine some annoying bugs where a value at a key accidentally depends on some state at the time the key was first read, if the key was missing then.
I like how you've named this method getOrCreate and mentioned the "create" part in the jsdoc; I don't think this needs anything else to be clear enough about the behavior.
Looking at the examples in the doc, it seems normal to pass something like the list constructor. Which is very steady/predictable in what it returns. 😛
There was a problem hiding this comment.
It looks like Immutable.Map's .get() takes an optional notSetValue. I'm not sure if that's a much better API to model this on—callers may accidentally vary what they pass for notSetValue with each .get() call and run into bugs that way. But at least there's not this odd possibility of depending on some state at first-read-if-not-set time.
There was a problem hiding this comment.
Yeah, I think the right way to use defaultdict always involves giving it a factory that makes something like an empty container, or returns a constant -- something that manifestly doesn't depend on any data that can change.
It'd be possible to do something twisty like a factory that ends up depending on some state, but that wouldn't be a good use case for defaultdict -- and I don't think it's a tempting one in practice, as I've never seen any code like that.
It looks like
Immutable.Map's.get()takes an optionalnotSetValue.
That does something quite different -- it doesn't alter the underlying collection (nor return a modified collection). Its Python analog would be dict.get, on the plain dict type:
https://docs.python.org/3/library/stdtypes.html#dict.get
What is more like defaultdict or this DefaultMap is the notSetValue parameter to Immutable.Map#update:
https://immutable-js.com/docs/v4.0.0/Map/#update()
Hmm, oddly that doesn't seem to have a description. But I believe the behavior is basically as if it were temporarily a DefaultMap with a factory returning notSetValue.
I think that notSetValue API choice is a reasonable compromise for the sake of having just the one Map type and having that feature still exist. But I think it is quite prone to bugs of this kind:
I'm not sure if that's a much better API—callers may accidentally vary what they pass for
notSetValuewith each.get()call and run into bugs that way.
| // @flow strict-local | ||
|
|
||
| /** | ||
| * A wrapper around Map, akin to Python's `defaultdict`. |
There was a problem hiding this comment.
src/utils/defaultMap.js
For the filename, I could go either way between DefaultMap and defaultMap. I didn't mean to set a precedent with zulipVersion, and sometimes I've thought ZulipVersion might be better. 😛 This is a small thing and not worth fretting over though.
There was a problem hiding this comment.
Hmm, yeah, I didn't actually really think about that.
If I'm naming the file after the class, I think it's best for it to have the same name as the class. I'll make it DefaultMap.js.
Call sites updated automatically, with:
$ perl -i -0pe '
s<\{ \.\.\.eg\.makeStream\(\{ (.*?) \}\), (stream_id: \d+) \}>
<eg.makeStream({ $2, $1 })>g
' src/**/*.js
(chris: cherry-picked from Greg's not-yet-merged zulip#5223)
gnprice
force-pushed
the
pr-mute-state
branch
from
ab929a9 to
186dc39
Compare
These event actions are based on what's in the event from the server, not what's in our internal state. These happen to be the same at the moment, but won't stay that way; so have this say what it really means.
After all, it appears not only in the initial data but also in events.
The term "mute" is used for both topics and users, so best to be explicit.
Call sites updated automatically, with:
$ perl -i -0pe '
s<\{ \.\.\.eg\.makeStream\(\{ (.*?) \}\), (stream_id: \d+) \}>
<eg.makeStream({ $2, $1 })>g
' src/**/*.js
Call sites updated mostly automatically, with:
$ perl -i -0pe '
s<\{ \.\.\.eg\.makeUser\(\{ (.*?) \}\), (email: \S+) \}>
<eg.makeUser({ $1, $2 })>g
' src/**/*.js
$ perl -i -0pe '
s<\{ \.\.\.eg\.makeUser\(\), (email: \S+) \}>
<eg.makeUser({ $1 })>g
' src/**/*.js
plus manually handling the one call site each in render-test
and fetchActions-test.
Now all eg.makeUser call sites are specifying their extra properties in the argument, rather than with their own spread.
…nd use We already have one test where we want to customize `color`; we'll add more soon, plus some for `pin_to_top`.
Now all eg.makeSubscription call sites are using the argument to customize the result, rather than spreading it.
…uxState This decouples it from changes in the respective data structures for these various parts of our state.
Call sites updated automatically, like so:
$ perl -i -0pe '
s<\{\s* \.\.\.eg\.action\.register_complete,\s*
data:\s* \{\s*
\.\.\.eg\.action\.register_complete\.data,\s*
unread_msgs:\s* \{\s*
\.\.\.eg\.action\.register_complete\.data\.unread_msgs,\s*
(.*?)
\},?\s*
\},?\s*
\}>
<eg.mkActionRegisterComplete({ unread_msgs: { $1 } })>xsg
' src/**/*.js
$ perl -i -0pe '
s<\{\s* \.\.\.eg\.action\.register_complete,\s*
data:\s* \{\s*
\.\.\.eg\.action\.register_complete\.data,
(.*?)
\},?\s*
\}>
<eg.mkActionRegisterComplete({ $1 })>xsg
' src/**/*.js
$ perl -i -0pe '
s<\bdeepFreeze\(\s*
(
eg\.mkActionRegisterComplete\(\{ .*? \}\)
) ,?\s*
\)>
<$1>xsg
' src/**/*.js
$ tools/fmt
…dInNarrow The work these used to do got moved a while ago into getShownMessagesForNarrow.
This is some legacy code that's been barely touched since 2017. And it turns out... not a single one of these test cases has the data in the form it actually appears in. Or for that matter in a form the data *could* possibly appear in in order for this feature to work. Happily, when we bring the type-checker to bear on this file, it immediately tells us about that! Do so, and fix the brokenness.
Less boilerplate, so it's easier to see what the tests are actually doing, and also so there's less to edit to adapt to changes.
Following the, er, model of unreadModel.
Now the tests no longer try to construct a MuteState directly; the data structure is encapsulated in the model.
And mention "stringify", to give a bit more context.
We'll use these in the muted-topics model, because the server doesn't send incremental updates for them anyway.
That is, split the case where the previous state is void, and so the action must be the store initialization action, from the case where it's any other action and the previous state must be of its usual object type. This makes for a bit more boilerplate, and one more line to update when adding or removing a sub-reducer. But both the split versions are simpler, and that will help us add new complications that we need.
This will let us alter the data structure so that it relies on stream IDs instead of stream names.
This represents one of the last significant pieces left for zulip#3918.
In the previous commit, we already stopped looking at this argument in favor of the stream ID. This change should be NFC except in the case where the given stream didn't exist in our data structures, at the several call sites that were looking it up in order to find the name. Those now simply go and look up the mute status, where they presumably find nothing. In particular if it's possible to reach the call site in constructTopicActionButtons in that case, then previously we would crash; now we offer to mute the topic.
Nothing radical, but we have these two instances of this pattern now and I think they both get a bit simpler to read this way.
gnprice
force-pushed
the
pr-mute-state
branch
from
186dc39 to
2f581fd
Compare
2 participants
This represents one of the last significant pieces left for #3918.
Much of the prep work is in converting some tests to our current style, which is more robust to making this kind of change without having to go and make a lot of messy changes to test data. In particular we add type-checking to about 20% of our remaining test code that lacked it, including
unreadSelectors-testwhich was the largest such file. And the old mute tests turn out to have been completely nonsense (which went undiscovered because they lacked types), so we fix that.The changes here are focused on how we store the data internally and convert it at the edge, independent of what form the server provides it in -- so this is independent of zulip/zulip#21015 which is for the server to start providing this data with stream IDs instead of stream names. In the future when that issue is resolved, we can start using the new protocol with new servers, but we'll still want to have this logic handling the old protocol until we've dropped support for all server versions that exist today.