We were queuing up individual messages to be flagged as
read on the server before this change, and we used debounce
to avoid sending individual POSTs, but this created delays
that were ripe for race conditions.
Now we batch them in the caller and use throttle instead.
This now prevents us from slamming the server with lots of
individual requests, without as many opportunities for races.
(Note that we still have some possibility of race conditions,
but they should be rare now, and other commits will address
some of the other contributors to read/unread glitches.)
Once we convert message.flags to more specific boolean attributes
like message.mentioned and message.alerted, we should get rid of
the `flags` attribute, as it will only confuse debugging.
We no longer set message.flags in the local echo path.
In the markdown parsing step, we just set message.mentioned
directly.
And then we change `insert_new_messages` to no longer
convert flags to booleans, and move that code to only
happen for incoming server message events.
We want to call `set_message_booleans` as soon as we
get data from the server, to avoid confusion about whether
`flags` is the authoritative field.
This commit has callers to `add_message_metadata` call
`set_message_booleans`.
This also sets us up to **not** call `set_message_booleans`
in the local echo codepath, where we can just have the
markdown processor set booleans natively.
In all cases the value of `flags` we were passing in was
actually `message.flags` (although it was slightly obscured in
one place), so now we just pass in `message`.
(We also move a tiny bit of defensive code to set `flags`
into `set_message_booleans`.)
In the JS code, we now use `message.unread` universally as
the indicator of whether a message is unread, rather than
the `message.flags` array that gets passed down to us
from the server.
In particular, we use the unread flag for filtering when
you search.
A lot of this commit is just removing logic to add/remove
"read" from `message.flags` and updating tests.
We also explicitly set `message.unread` to `false` inside of
`unread.mark_as_read()` and no longer have `unread.set_flag()`.
(Some of the callers to `unread.set_flag` were also calling
`unread.mark_as_read`, which was updating the `message`
object, so now we just have `unread.mark_as_read` update
the `message` object. And then unread_ops.mark_all_as_read()
was already calling unread.declare_bankruptcy().)
This adds two similar functions to simplify
our batch processing of unread messages.
unread.get_unread_messages
unread.get_unread_message_ids
They are used to simplify two functions that loop
over messages. Before this change, the functions
would short circuit the loop to ignore messages
that were already read; now they just use the
helpers before the loop.
We now make direct calls to `send_flags_update` for
seting collapsed/uncollapsed, rather than going through
the `batched_updaters` mechanism.
Also the methods in `message_flags` to set
collapsed/uncollapsed now take a single message.
We now do all of the main logic for starring/unstarring
a message in `message_flags.toggle_starred`:
* mark the message as read (just in case)
* update the UI (i.e. the green star in the message)
* update the server
The calling code in both the click handler and the hotkey
handler remains simple--they just handle minor details like
finding the message and clearing popovers.
For updating the server, we now call the new
`send_flag_update` helper.
And we continue to delegate some of the logic to
`ui.update_starred`, but we remove some code there that's
now pushed up to `message_flags.toggle_starred`.
This change should be mostly transparent to users, but it
does remove some inconsistent behaviors between the click
handler and the hotkey handler. Before this change, the
click handler was more aggressive about updating the UI
and marking the message as read. For people using the "*"
key to star/unstar, they probably would only have noticed
different behavior on a slow connection or in an edge
case scenario where only half of the message was onscreen.
More importantly, by simplifying how we talk to the server,
this eliminated up to a one-second lag due to the debounce
logic in the batch_updater code. The complicated debounce
logic is only really needed for batch-updating "read"
messages, and it was overkill and sluggish for starring
messages.
Last but not least, we add defensive code for the local
echo case. (Users have to wait till the message gets acked
to star it.)
Wait until the server acks a message before we enable
the message popover menu. This prevents a whole class
of bugs related to re-drawing the message and changing
the message id, and it also makes room for a little
spinner in the future.
Users with decent internet connections will generally
get server responses before they can click on the
chevron or hit esc/i, anyway.
Trying to collapse a locally echoed message is a rare
thing to do, and it was buggy before this due to races
between the server acking the original message and the
user flipping the collapsed flag.
We now calculate `should_display_collapse` and
`should_display_uncollapse` in the JS code and simplify
the template by eliminating all the inline if/else
logic.
(Note that we are about to disable the message menu
altogether for locally echoed messages, so this change
is partly future-proofing for when we put the menu
back for more innocuous commands.)
Adds type "embedded bot" to bot creation menu. Lets
users select a bot to run from a list of bots.
Currently, this list is hard-coded into the backend.
This diff is nothing but dedentation -- it's empty under
`git diff -b`. These with-statements are only needed for
a pretty narrow scope of code, so make that clear in the
source.
There are two different things you need to patch in order to get error
emails (at `/emails`) in dev. Flip one of them in dev all the time,
and make the comment on the other a bit more explicit.
When I added this "Deployed code" feature to the error reporting,
I apparently hadn't worked out enough of how this code works to
realize that `notify_server_error` may be in a different process,
at a different time and potentially even on a different machine
from the actual error being reported.
Given that architecture, all the data about the error must be computed
in `AdminNotifyHandler`, before sending the report through the queue,
or else it risks being wrong. The job of `notify_server_error` and
friends is only to format the data and send it off. So, move the
implementation of this feature in order to do that.
(@showell added some "nocoverage" directives here for code that
is hard to test (exceptions being thrown, deployment files not
existing) and that was originally part of a file that didn't
require 100% coverage)
Steve Howell
derAnfaenger
Abhijeet Kaur
Xavier Cooney
nyan-salmon
Viraat Chandra
fredfishgames
greysome
cPhost
Rishi Gupta
Vishnu Ks
Greg Price