narrow/recipient: Fix more bugs, and prep for bigger refactors#4332
Merged
chrisbobbe merged 13 commits intozulip:masterfrom Dec 10, 2020
Merged
narrow/recipient: Fix more bugs, and prep for bigger refactors#4332chrisbobbe merged 13 commits intozulip:masterfrom
chrisbobbe merged 13 commits intozulip:masterfrom
Conversation
Before this change, we have our logging code importing the module with
our Redux store, in order to get the version of the Zulip server we're
currently talking to so it can include that as context in the error
report. The logging code is naturally low in the import graph
(imported by lots of other modules) and the app's Redux store is
naturally high in the import graph (importing lots of other
modules)... so this tends to lead to import cycles and to large blobs
of import cycles.
At present, this import only participates in a couple of small cycles;
the whole blob looks like this:
$ f=src/utils/logging.js; npx flow cycle --strip-root "${f}" \
| perl -lne 'print $1 if (/"(.*?)"/)' | sort -u
src/boot/store.js
src/boot/ZulipAsyncStorage.js
src/redux-persist-migrate/index.js
src/utils/logging.js
But the immediate motivation for this change is that we'd like to add
some logging in src/utils/recipient.js. That's another module
naturally low in the import graph, so there are a number of modules
importing it that are in turn imported (transitively) by store.js,
making the cycle-blob (or "strongly-connected component": the set of
modules which have import chains both to and from a given module and
therefore each other) look like this:
src/boot/reducers.js
src/boot/store.js
src/boot/ZulipAsyncStorage.js
src/caughtup/caughtUpReducer.js
src/chat/fetchingReducer.js
src/chat/narrowsReducer.js
src/redux-persist-migrate/index.js
src/typing/typingReducer.js
src/unread/unreadHuddlesReducer.js
src/unread/unreadPmsReducer.js
src/utils/logging.js
src/utils/narrow.js
src/utils/recipient.js
And as a result it produces confusing errors in Flow, and failures in
Jest where whole imported modules are just `undefined` because they
haven't been initialized yet.
Happily, by severing the one link from logging.js to store.js, we
completely dissolve this cycle-blob! With this change, and even with
that `recipient` -> `logging` import added, neither src/boot/store.js
nor src/utils/logging.js nor any of the other modules they were in
import cycles with is part of any import cycle.
To accomplish that import-severing, we use dependency injection:
the logging module no longer knows where to find the store, and
instead that information gets handed to it. We have the store
module do this at its own initialization time; if we're logging
something before the store even exists, it wouldn't have had any
meaningful information anyway.
We'll want to use this in the next commit, where we add some `logging.error` calls in code that has test coverage: it'll allow us to (a) suppress those warnings from spewing when the tests are run, plus (b) check in the test that we really do call `logging.error`. Meanwhile, it has no effect on tests that don't specifically do something with these mocks -- which at this stage is all of our tests. By my reading of the Jest docs, we shouldn't need this here in our global setup file -- we should be able to just call `jest.mock` in the respective test files where we actually want the mocking. But I found that that didn't work for me; such calls just had no effect, and both test and application code got the real, unmocked module. That'd be good to figure out and fix in general, but fortunately this particular module is both small and of broad interest.
These fudges really don't belong here at all. For now, just start logging if they happen, so we can try to confirm that they never do. This function has a lot of call stacks that get their data from a lot of different sources, so it's complex to try to confirm that by studying source code. The test changes suppress the errors from actually printing in the normal way (to avoid meaningless noise in the test results), and then also confirm that the errors were indeed emitted.
This code was falling into a classic JS pitfall: calling `Array#sort` on an array of numbers, with the default comparison. That comparison is by *string*, so e.g. 22 sorts before 3 instead of after. Fixing this bug changes the return value of this function and its relative normalizeRecipientsAsUserIdsSansMe. The point of these functions is to use the return values as keys in our data structures, so it's important that we're consistent about them. But as it happens, the only callers are in the reducer and selector for the typing status in `state.typing`. They only need to agree with each other, so making the change in this one place suffices; and that's one of the `discardKeys` we don't persist between runs of the app, so there's no need for a migration.
This fell into a classic JS pitfall: when sorting an array of numbers, the default comparison function is by string, and you need to pass an explicit comparison function to sort the actual numbers. This means that in b022e9c we actually broke this invariant which was probably previously always true in practice, oops. Fortunately we generally avoid actually relying on that invariant; see comment on pmNarrowFromEmails (previously groupNarrow) in src/utils/narrow.js.
This exists several times over because we do it on emails, or on user IDs, or on user IDs found at a different property name. It'd be possible to deduplicate it down to just one copy with a bit more abstraction... but it's short enough that the trade wouldn't be worth it, and we haven't. Still, we can at least make all the copies look completely parallel and live right next to each other. This is not quite NFC because in the ...ByEmail case, when there's just one recipient in the original list (which, for the display_recipients of a message, happens only for a self-PM), passing it through normalizeRecipients causes the same whitespace fudges to be applied as we already do the rest of the time. Those fudges probably are never doing anything, and definitely should get removed from there one way or another; a few commits ago we added some logging on them as a step toward doing so.
We can just operate on the user IDs directly, without extra objects to hold them.
Since 97cdfac earlier this week, we already effectively say this in the jsdoc for its single-item counterpart pmNarrowFromEmail; but we neglected to update this one's jsdoc to match.
…gic. This conditional doesn't actually serve any purpose at this point, now that pmNarrowFromEmail and pmNarrowFromEmails (previously called privateNarrow and groupNarrow) explicitly do the same thing. Also simplify the name of a related variable, while we're here.
These were left behind as strays by ff872f6.
…users. Specifically cross-realm bots, like Welcome Bot and Notification Bot. Such users can perfectly well appear in a link to a PM thread, and we should handle those links correctly. They can also appear in a PM thread given by a notification. In the case of deactivated users this isn't super likely, because IIUC the message won't have been sent if the user was deactivated; but it could happen if e.g. the user was deactivated after the message and before our user got around to opening the notification.
This can only make a difference in the behavior if we have presence data for a user who's either deactivated or a cross-realm bot. I'm not sure that can ever happen... but if we do have such data, it seems clear that we ought to show it. So eliminate this one of the few remaining spots where we used the temptingly-named but incomplete data from the `getUsers*` selectors.
Which was recently renamed from getNarrowFromMessage.
gnprice
force-pushed
the
pr-recipient
branch
from
09bf944 to
2a03ba6
Compare
chrisbobbe
approved these changes
Dec 10, 2020
| @@ -0,0 +1,47 @@ | |||
| /** | |||
| * A dead-drop for select data to be included in log events. | |||
Contributor
There was a problem hiding this comment.
0b9d381 logging [nfc]: Stop importing store, by injecting the dependency.
Interesting!
| // group PM conversation excludes self -- so it's unusable if you try | ||
| // to give someone else in it a link to a particular message, say. | ||
| // * OK, unsorted: getNarrowFromMessage | ||
| // * OK, unsorted: getNarrowForReply |
Contributor
There was a problem hiding this comment.
Ah, thanks for catching this.
gnprice
added a commit
to gnprice/zulip-mobile
that referenced
this pull request
Jan 6, 2021
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows on some work prompted by #4317, begun in #4330. I have a longer branch on top of this which is still in development, which does much more to encapsulate the fiddly details of how we describe PM conversations, and as a result takes care of most of #4035 which has been the main obstacle to #3133. This PR contains the changes from that branch which are ready and stand on their own.
Among these changes are some small bugfixes, and other cleanups in areas of the code where the larger branch will do further work. This branch starts with a couple of infrastructural changes to smooth the way for using error logging in more parts of our code.