This should avoid some memory allocations.
We also use build_person_matcher to avoid
repeating the same logic over and over
again to process the query into termlets.
We also remove people.get_all_persons() and
people.person_matches_query().
This may actually be a slowdown for the worst case
scenario, but it sets us up to be able to easily
short circuit the removal of diacritic characters
for users that have pure ascii names.
For example, czo has lots of names like this:
- Tim Abbott
- Steve Howell
Since they're pure ascii, we can do a one-time
check. A subsequent commit will show how we use
this.
This looks like simple code cleanup, but it's more
than that.
The code cleanup here is that we don't have three
callbacks to get a list of typeaheads for bootstrap.
Instead, we just have one function that does all the
main work.
And then the speedup comes from the fact we no longer
need to remove diacritics from the query for every
time through our loop of seeing if a person matches
the query.
It's a bit subtle to see in the diff, but these are
the relevant lines:
const matcher = exports.get_person_or_user_group_matcher(query);
const filtered_results = _.filter(people_and_groups, matcher);
Before this, bootstrap was doing $.grep, and we'd have
to reinitialize the matcher for every person.
If you profile this before and after, you'll see that
remove_diacritics gets called fewer times.
To profile this, you want to loads lots of users into
your DB and try to autocomplete "Extra", as in "Extra1 User".
If you try to autocomplete something else, then my patch
won't really help, and `remove_diacritics` will still
show up as expensive. Because it is that expensive a function.
These had to be done in tandem, since they were
both kinda coupled to the function that is now
called query_matches_name_description.
(This commit slightly negatively impacts PM
lookups, but this is addressed in the subsequent
commit, which makes PMs much faster. The impact
is super minimal--it's just an extra function
dispatch.)
This may seem silly now, since we are returning a function
that still dispatches over all flavors of search for
every item, but subsequent commits will make it obvious
why I'm doing this.
We want to do our own matching of items, rather than
just giving a callback to bootstrap, which does $.grep
on all the items.
Doing our own matching gives us flexibility for future
improvements like custom data structures for searching
through big amounts of data. Even in the short term
we can speed up searches by pulling expensive operations
outside the grep/filter call.
This architecture has been in place for our search
bar since ~2014.
We have ~5 years of proof that we'll probably never
extend Dict with more options.
Breaking the classes into makes both a little faster
(no options to check), and we remove some options
in FoldDict that are never used (from/from_array).
A possible next step is to fine-tune the Dict to use
Map internally.
Note that the TypeScript types for FoldDict are now
more specific (requiring string keys). Of course,
this isn't really enforced until we convert other
modules to TS.
We had a potentially nasty bug where we
weren't guaranteeing that all/stream/everyone
collated in consistent ways inside of
`compare_people_for_relevance`, which can
send certain types of sort algorithms into
an infinite loop. I doubt this ever happened
in practice, but it's obviously worth fixing.
Now we also have a clear tiebreaker between
any two all/everyone/stream mentions, which
is the idx field.
Finally, this should be a bit more efficient.
This name was misleading, since this code is used
in sort_recipients, which happens when you, for
example, autocomplete persons in the "To:" box
when composing (and has nothing to do with
mentioning).
This makes it a bit easier to find common patterns,
plus it sets us up to pull the calls even further
up the stack.
The first rule of dealing with user data is sanitize
at the edges, not deep down in some function that
has many callers. Putting this code so deep down
in the stack means it's more likely to be called in
a loop.
This moves clean_query into all the callers
of query_matches_source_attrs.
This doesn't change anything performance-wise,
but it sets up future commits.
This change is easy--we only had one caller.
This change means any query going against a
target with multiple `match_attrs`, such as
user names (first name, last) only has to
clean the query once per person.
We want to mostly deprecate this function (see
the comment I added), so I gave it a more specific
name.
Ideally I'd just fix `stream_create`, but it does
use this function in a couple places, and it's helpful
to reuse the same sort here. In one place stream_create
actually unshifts the "me" user back to the top of the
list, which makes sense for its use case.
If two user_ids in a recent huddle have ids
that sort lexically differently than numerically,
such as 7 and 66, then we were creating two
different buckets in pm_conversations.
This regression was introduced in
263ac0eb45 on
November 21, 2019.
Instead of having our callers pass in a possibly
non-canonical version of a user_ids_string, just
have them pass in a list.
The next commit will canonicalize the sort.
The only thing get_color() does is look
up a sub:
exports.get_color = function (stream_name) {
const sub = exports.get_sub(stream_name);
if (sub === undefined) {
return stream_color.default_color;
}
return sub.color;
};
So if we have a sub already, there's no point
calling the helper.
Obviously, this isn't a huge deal, but it happens
N times during page load.
This should make any operation on subscribed
streams faster (we won't need to filter out
unsubscribed streams every time).
I started writing this before I realized we
had a bug where we call `subscribed_streams`
in a nested loop.
After fixing the bugs, this is not as much of
a bottleneck, but it's still a speedup in many
important places:
* build left sidebar
* every keystroke in search bar
* first keystroke in making #stream_links
* every keystroke in compose stream box
The streams settings code is kinda complicated.
It does a non-deterministic sort of the "others"
bucket when you add elements to the left panel.
They get hidden, anyway. Our values() call now
puts subscribed streams first. It never guaranteed
order, but putting subscribed streams first is
probably a good behavior for most situations.
This defers O(N*S) operations, where
N = number of streams
S = number of subscribers per stream
In many cases we never do an O(N) operation on
a stream. Exceptions include:
- checking stream links from the compose box
- editing a stream
- adding members to a newly added stream
An operation that used to be O(N)--computing
the number of subscribers--is now O(1), and we
don't even pay O(N) on a one-time basis to
compute it (not counting the cost to build the
array from JSON, but we have to do that).
Calling `set_filter_out_inactives` is expensive, since we
count up the number of subscribed streams, which iterates
through all your streams, creates a new list of subscribed
streams, then counts them.
In my dev setup, I created 700 streams, and this shaved
about 700ms off of the initial call to `build_stream_list`.
If we aren't showing users emails, then we don't
want to use emails in the search.
And if we are showing users emails, we want to
search on the email that's displayed to them.
For admins this will be delivery_email.
For regular users we arguably shouldn't search
on emails either, since it mostly causes confusion,
but this commit just preserves the current
behavior for those users (unless `show_email` is
false).
We want to be able to unit test this value,
since it's conditional on several factors:
- am I an admin?
- can non-admins view emails?
- do we have delivery_email for the user?
I'm mocking show_email in the tests, since the
show_email code is in `settings_org` and
kind of hard to unit test. It's not impossible,
but it's too much for this commit. (Either
we need to extract it out to a nice file or
deal with mocking jQuery. That module is
mostly data-oriented, so it would be nice
to have something like `settings_config` that
is actually pure data.)
This was duplicate code. I'm moving it to people
for pragmatic reasons--it's hard to unit test stuff
in settings_users.js due to all the jQuery.
It's also nice to have all people-related search
code in one place, just for auditing purposes.
It appears c28c3015 caused a regression where we
set `email` to undefined if a user does not have
`delivery_email` set, and this causes filtering
of users to fail for admins doing user settings.
This fixes only one of the issues reported in
issue #13554.
There's probably no easy fix to scrolling taking
long, but I think fixing search will mostly
address that complaint.
The Rust folks seem to agree with me that the
search results are too noisy. If I search for
"s" I get:
* names like Steve (good)
* names like Jesse (noisy)
* anybody with s in their email (super noisy)
Here is the relevant code:
return (
item.full_name.toLowerCase().indexOf(value) >= 0 ||
email.toLowerCase().indexOf(value) >= 0
);
We now can call is_ascii only once per search termlet
when we are filtering multiple persons on the same
query. (This requires the caller to use
`build_person_matcher` outside a loop or before
a `_.filter` call.)
This is not a major speedup, but we do a couple
simple things here:
- trim the query outside the function we
build (that might be called multiple times)
- don't split names before we possibly
early-exit with an email match
This will allow use to change some O(N) behavior
to O(1) where we are performing the same query
on a bunch of people. (Subsequent commits will
actually take advantage of this prefactoring.)
Once we have max_items results, stop trying
to get more items.
This should really help large realms when
you do a search on streams that turns up
more than N streams (where N is about 12).
We won't even bother to find people.
This class gives us more control over attaching
suggestions to our eventual result. The main
thing we do now is remove duplicates as they're
encountered.
This will make sense in the follow up commit,
where we can short circuit actions as soon as
we get enough results.
This has a few benefits:
- we remove some duplicate code
- we can see finalize_results in profiles
It turns out finalize_results is expensive
for some searches. If the search itself doesn't
do a ton of work but returns a lot of results,
we see it in finalize_results. It brings to
attention that we should be truncating items
earlier instead of doing lots of unnecessary
work.
This isn't a huge speedup, but it's an easy
code change.
We remove the two-liner highlight_with_escaping,
which was only called in one place, and when
we inline it into the caller, we can pull the
first line, which builds the regex, out of the
loop.
The code we removed in highlight_with_escaping
is exactly the same code as in
highlight_with_escaping_and_regex.
I actually copy/pasted this code five years
ago and am now removing the duplication. :)
When we're highlighting all the people that show
up in a search from the search bar, we need
to fairly expensively build a regex from the
query:
query = query.toLowerCase();
query = query.replace(/[\-\[\]{}()*+?.,\\\^$|#\s]/g, '\\$&');
const regex = new RegExp('(^' + query + ')', 'ig');
Even though the final regex is presumably cached, we
still needed to do that `query.replace` for every person.
Even for relatively small numbers of persons, this would
show up in profiles as expensive.
Now we just build the query once by using a pattern
where you call a function outside the loop to build
an inner function that's used in the loop that closes
on the `query` above. The diff probably shows this
better than I explained it here.
This fixes a cross-site scripting vulnerability in the upcoming Inline
URL Previews feature found by Graham Bleaney and Ibrahim Mohamed using
Pysa.
This commit doesn't get a CVE because the bug was present in a code
path introduced in the 2.1.x development branch, so it doesn't impact
any Zulip release.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The streams:all adveritsement notice in search should only appear
after we've already received the response from the server, to avoid a
mix of problems ranging from misplaced loading indicator to scrolling
issues to the notice just being distracting while you're waiting for
the server to return results.
We need to add a pre_scroll_cont parameter to the message_fetch API,
since adding this notice would otherwise potentially throw off the
scroll positioning logic for which message to select.
Fixes#13441.
In 452e226ea2 and
648a60baf6, we changed how `search:`
narrows work to:
(1) Never mark messages as read inside searches (search:)
(2) Take you to the bottom, not the first unread, if a `near:` or
similar wasn't specified.
This is far better behavior for these use cases, because in these
narrows, you can't actually see all the context around the target
messages, so marking them as read is counterproductive. This is
especially important in `has:mention` where you goal is likely
specifically to keep track of which threads mentioning you haven't
been read. But in many other narrows, the current behavior is
effectively (1) setting the read bit on random messages and (2) if the
search term matches many messages in a muted stream with 1000s of
unreads, making it hard or impossible to find recent search matches.
The new behavior is that any narrow that is structurally a search of
history (including everything that that isn't a stream, topic,
pm-with, "all messages" or "private messages") gets that new behavior
of being unable to mark messages as read and narrows taking you to the
latest matching messages.
A few corner cases of interest:
* `is:private` is keeping the old behavior, because users on
chat.zulip.org found it confusing for `is:private` to not mark
messages as read when one could see them all. Possibly a more
complex answer is required here.
* `near:` narrows are getting the new behavior, even if it's a stream:
+ topic: narrow. This is debatable, but is probably better than
what was happening before.
Modified significantly by tabbott for cleanliness of implementation,
this commit message, and unit tests.
Fixes#9893. Follow-up to #12556.
In 1fe4f795af, we added the
wildcard_mentions_notify setting, which controls whether wildcard
mentions should be treated as mentions for the purposes of
notifications. The original implementation focused on the more
important area of email/push notifications, and neglected to address
desktop notifications for wildcard mentions.
This change makes the wildcard_mentions_notify flag behave correctly
for desktop/sound notifications, including unit tests.
Fixes#13073.
We register ZulipRemoteUserBackend as an external_authentication_method
to make it show up in the corresponding field in the /server_settings
endpoint.
This also allows rendering its login button together with
Google/Github/etc. leading to us being able to get rid of some of the
code that was handling it as a special case - the js code for plumbing
the "next" value and the special {% if only_sso %} block in login.html.
An additional consequence of the login.html change is that now the
backend will have it button rendered even if it isn't the only backend
enabled on the server.
Adds required API and front-end changes to modify and read the
wildcard_mentions_notify field in the Subscription model.
It includes front-end code to add the setting to the user's "manage
streams" page. This setting will be greyed out when a stream is muted.
The PR also includes back-end code to add the setting the initial state of
a subscription.
New automated tests were added for the API, events system and front-end.
In manual testing, we checked that modifying the setting in the front end
persisted the change in the Subscription model. We noticed the notifications
were not behaving exactly as expected in manual testing; see
https://github.com/zulip/zulip/issues/13073#issuecomment-560263081 .
Tweaked by tabbott to fix real-time synchronization issues.
Fixes: #13429.
If a message begins with /me, we do not have any cases where the
rendered content would not begin with `<p>/me`. Thus, we can safely
remove the redundant checks both on the backend and frontend.
The "Stream settings" UI was always intended to be initialized in the
"Subscribed" tab when opened not through navigation that explicitly
aims to via "All streams". We had implemented that through how the UI
is rendered as well as the internal state tracking variable
`subscribed_only`, which was initialized to `true`.
The bug was that we didn't reset that to `true` when re-opening
"Stream settings" via a code path that calls `setup_page` (e.g. via
the menus on the left sidebar).
Ths fixes a bug where the stream-list in the stream settings would
list all streams but would show the 'Subscribed' label after
navigating to "All streams", closing "Manage streams", and then
reopening it.
Fixes#13297.
In e42c3f7418, we made the assumption
that compose_pm_pill.get_recipient() would return no users for stream
messages. It turns out, due to the confusing name of
compose_state.recipient (which we just renamed to
compose_state.private_message_recipient), this assumption was wrong.
As a result, when composing a stream message using the reply hotkeys,
we'd end up sending typing notiifcations to the person who sent the
message we're replying to as though a PM was being composed.
We fix this by avoiding passing an (expected to be unused) value for
private_message_recipient to compose_state.start.
The compose_state.recipient field was only actually the recipient for
the message if it was a private_message_recipient (in the sense of
other code); we store the stream in compose_state.stream instead.
As a result, the name was quite confusing, resulting in the
possibility of problematic correctness bugs where code assumes this
field has a valid value for stream messages. Fix this by changing it
to compose_state.private_message_recipient for clarity.
Fixes commit id 648a60baf6. When
allow_use_first_unread_when_narrowing() is false last message of
narrow is shown in view.
Comments rewritten by tabbott to explain in detail what's happening.
This simple change switches us to take advantage of the
server-maintained data for the pm_conversations system we implemented
originally for mobile use.
This should make it a lot more convenient to find historical private
message conversations, since one can effectively scroll infinitely
into the history.
We'll need to do some profiling of the backend after this is deployed
in production; it's possible we'll need to add some database indexes,
denormalization, or other optimizations to avoid making loading the
Zulip app significantly slower.
Fixes#12502.
message_id, rather than timestamps, is our standard way to sort by
time. And this refactor is important because we're about to start
using data from the server to populate this data structure.
This avoids a stream having potentially near-infinite height when
opened in a stream with a large number of unread topics; the benefit
is that you can easily access the next stream.
We show an unread count next to "more topics" to make it hard to miss
that there might be more, older topics with unread messages.
With CSS work by Anders Kaseorg.
Fixes#13087.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit was automatically generated by `tools/lint --only=eslint
--fix`, except for the `.eslintrc.json` change itself.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Updates the message editing process to do a local 'echo'.
On slow connections, now there is visual confirmation of the edit,
similar to when sending messages. The contains_backend_only_syntax
logic and check are the same as there.
We showing "(SAVING)" until the edit is completed, and on successful
edit, the word "(EDITED)" appears. There's likely useful future work
to do on making the animation experience nicer.
Substantially rewritten by tabbott to better handle corner cases and
communicate more clearly about what's happening.
Fixes: #3530.
This change makes it possible for users to control the notification
settings for wildcard mentions as a separate control from PMs and
direct @-mentions.
This commit was automatically generated by `tools/lint --only=eslint
--fix`, after an `.eslintrc.json` change.
A half dozen files were removed from the changes by tabbott pending
further work to ensure we avoid breaking valuable PRs with merge
conflicts.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Hovering over user names (and user circles for PM List) now displays
Name, Status Message and Last online time in a js tooltip.
Hovering over group names displays the names of all group members.
Unavailable users are shown as "Last active: Today".
Hovering on a user circle in the Buddy List results in a js tooltip
with Active/Idle/Offline/Unavailable for
green/orange/white/white-with-line.
Resolves#11607.
When strings are tagged for translation using `tr this`, the strings
were passed into the frontend i18n as-is (including new line and tab
characters that are not functional in the text, existing just to
format the HTML files reasonably).
This did not match the algorithm used in `manage.py makemessages` for
extracting strings for translation, which (correctly) removed that
whitespace to provide a good experience for translators. The fix is
for the `tr this` implementation to use that same whitespace-stripping
algorithm.
Tested manually by checking if those strings that were not translated
earlier were translated, and also fixed an automated test that had the
wrong result, which should help prevent regressions.
Fixes#13389.
These should work consistently with how the individual user setting
works; see the last commit.
With changes from tabbott to fix real-time sync.
Fixes#12553.
This fixes two regressions in 1946692f9a.
The first bug was actually introduced much earlier, namely that we
were not sending a `bot_owner_id` field at all for bot users without
an owner. The correct behavior would have been send `None` for the
owner field.
The second bug was simply that we needed to update the webapp to look
for the `bot_owner_id` field, rather than an old email-address format
`bot_owner` field.
Thanks to Vinit Singh for reporting this bug.
This commit was originally automatically generated using `tools/lint
--only=eslint --fix`. It was then modified by tabbott to contain only
changes to a set of files that are unlikely to result in significant
merge conflicts with any open pull request, excluding about 20 files.
His plan is to merge the remaining changes with more precise care,
potentially involving merging parts of conflicting pull requests
before running the `eslint --fix` operation.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The js_typings directory is not set up correctly for us to add new
type declarations for untyped external modules. The correct
configuration would be something like
{
"compilerOptions": {
"baseUrl": ".",
"paths": {
"*": ["js_typings/*"],
},
"typeRoots": ["js_typings"],
},
"exclude": [
"js_typings",
],
}
but that configuration is incompatible with using the same directory
for _internal_ modules like the ones declared here.
Also, correct some mistakes the generation of this list.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Set `--esModuleInterop` and `--isolatedModules` for consistency with
Babel. `tsc --init` adds `--esModuleInterop` by default.
Set `--moduleResolution node` so we can find type definitions in
modules that provide them.
Set `--forceConsistentCasingInFileNames`, which seems like a good
idea, and which `tsc --init` will add by default in TypeScript 3.7.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
`--jsx preserve` and `--removeComments false` are already the default.
`--strict` already implies `--noImplicitAny`, `--noImplicitThis`,
`--alwaysStrict`.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Commit d17b577d0c (#13321) incorrectly
transformed this line, even though I thought my script had a specific
guard against this.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Steve Howell
Anders Kaseorg
Tim Abbott
Mohit Gupta
Mateusz Mandera
Nat1405
Gaurav Thapar
Rohitt Vashishtha
joaomcarvalho
Jack Tiggleman
Vinit Singh
Dinesh
Sophie
Matheus Melo
Thomas Ip