Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

process_from_server: Fix message sent in search view not appearing. 

Earlier, on sending a new message in a search view it didn't
appear in that view for the sender.

This was because the message event received by the sender
was processed by 'msg_list.view.rerender_messages' which
effectively did nothing because the message is not
rendered in the first place during local echo in that
message list view.

We can't determine locally if the message should be added
to the search narrow. So, we use maybe_add_narrowed_messages
when narrowed to such views.

This fixes the bug and the message appears in the search view.
This commit is contained in:
Prakhar Pratyush 2024-07-15 19:53:45 +05:30 committed by Tim Abbott
parent 629e6c701c
commit 3090247221
5 changed files with 176 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
tools/test-js-with-node
View File

@ -137,6 +137,7 @@ EXEMPT_FILES = make_set(
"web/src/message_edit.js",
"web/src/message_edit_history.ts",
"web/src/message_events.js",
"web/src/message_events_util.js",
"web/src/message_feed_loading.ts",
"web/src/message_feed_top_notices.ts",
"web/src/message_fetch.js",

25
web/src/echo.js
View File

@ -7,6 +7,7 @@ import * as compose_notifications from "./compose_notifications";
import * as compose_ui from "./compose_ui";
import * as local_message from "./local_message";
import * as markdown from "./markdown";
import * as message_events_util from "./message_events_util";
import * as message_lists from "./message_lists";
import * as message_live_update from "./message_live_update";
import * as message_store from "./message_store";
@ -377,7 +378,7 @@ export function update_message_lists({old_id, new_id}) {
}
export function process_from_server(messages) {
const msgs_to_rerender = [];
const msgs_to_rerender_or_add_to_narrow = [];
const non_echo_messages = [];
for (const message of messages) {
@ -424,17 +425,25 @@ export function process_from_server(messages) {
client_message.is_me_message = message.is_me_message;
client_message.submessages = message.submessages;
msgs_to_rerender.push(client_message);
msgs_to_rerender_or_add_to_narrow.push(client_message);
waiting_for_ack.delete(local_id);
}
if (msgs_to_rerender.length > 0) {
// In theory, we could just rerender messages where there were
// changes in either the rounded timestamp we display or the
// message content, but in practice, there's no harm to just
// doing it unconditionally.
if (msgs_to_rerender_or_add_to_narrow.length > 0) {
for (const msg_list of message_lists.all_rendered_message_lists()) {
msg_list.view.rerender_messages(msgs_to_rerender);
if (!msg_list.data.filter.can_apply_locally()) {
message_events_util.maybe_add_narrowed_messages(
msgs_to_rerender_or_add_to_narrow,
msg_list,
message_util.add_new_messages,
);
} else {
// In theory, we could just rerender messages where there were
// changes in either the rounded timestamp we display or the
// message content, but in practice, there's no harm to just
// doing it unconditionally.
msg_list.view.rerender_messages(msgs_to_rerender_or_add_to_narrow);
}
}
}

91
web/src/message_events.js
View File

@ -3,8 +3,6 @@ import assert from "minimalistic-assert";
import * as alert_words from "./alert_words";
import {all_messages_data} from "./all_messages_data";
import * as blueslip from "./blueslip";
import * as channel from "./channel";
import * as compose_fade from "./compose_fade";
import * as compose_notifications from "./compose_notifications";
import * as compose_state from "./compose_state";
@ -13,6 +11,7 @@ import * as direct_message_group_data from "./direct_message_group_data";
import * as drafts from "./drafts";
import * as message_edit from "./message_edit";
import * as message_edit_history from "./message_edit_history";
import * as message_events_util from "./message_events_util";
import * as message_helper from "./message_helper";
import * as message_lists from "./message_lists";
import * as message_notifications from "./message_notifications";
@ -35,86 +34,6 @@ import * as unread_ops from "./unread_ops";
import * as unread_ui from "./unread_ui";
import * as util from "./util";
function maybe_add_narrowed_messages(messages, msg_list, callback, attempt = 1) {
const ids = [];
for (const elem of messages) {
ids.push(elem.id);
}
channel.get({
url: "/json/messages/matches_narrow",
data: {
msg_ids: JSON.stringify(ids),
narrow: JSON.stringify(narrow_state.public_search_terms()),
},
timeout: 5000,
success(data) {
if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) {
// We unnarrowed or moved to Recent Conversations in the meantime.
return;
}
let new_messages = [];
const elsewhere_messages = [];
for (const elem of messages) {
if (Object.hasOwn(data.messages, elem.id)) {
util.set_match_data(elem, data.messages[elem.id]);
new_messages.push(elem);
} else {
elsewhere_messages.push(elem);
}
}
// This second call to process_new_message in the
// insert_new_messages code path is designed to replace
// our slightly stale message object with the latest copy
// from the message_store. This helps in very rare race
// conditions, where e.g. the current user's name was
// edited in between when they sent the message and when
// we hear back from the server and can echo the new
// message.
new_messages = new_messages.map((message) =>
message_helper.process_new_message(message),
);
callback(new_messages, msg_list);
unread_ops.process_visible();
compose_notifications.notify_messages_outside_current_search(elsewhere_messages);
},
error(xhr) {
if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) {
return;
}
if (xhr.status === 400) {
// This narrow was invalid -- don't retry it, and don't display the message.
return;
}
if (attempt >= 5) {
// Too many retries -- bail out. However, this means the `messages` are potentially
// missing from the search results view. Since this is a very unlikely circumstance
// (Tornado is up, Django is down for 5 retries, user is in a search view that it
// cannot apply itself) and the failure mode is not bad (it will simply fail to
// include live updates of new matching messages), just log an error.
blueslip.error(
"Failed to determine if new message matches current narrow, after 5 tries",
);
return;
}
// Backoff on retries, with full jitter: up to 2s, 4s, 8s, 16s, 32s
const delay = Math.random() * 2 ** attempt * 2000;
setTimeout(() => {
if (msg_list === message_lists.current) {
// Don't actually try again if we un-narrowed
// while waiting
maybe_add_narrowed_messages(messages, msg_list, callback, attempt + 1);
}
}, delay);
},
});
}
export function insert_new_messages(messages, sent_by_this_client) {
messages = messages.map((message) => message_helper.process_new_message(message));
@ -133,7 +52,11 @@ export function insert_new_messages(messages, sent_by_this_client) {
// new messages match the narrow, and use that to
// determine which new messages to add to the current
// message list (or display a notification).
maybe_add_narrowed_messages(messages, list, message_util.add_new_messages);
message_events_util.maybe_add_narrowed_messages(
messages,
list,
message_util.add_new_messages,
);
continue;
}
@ -504,7 +427,7 @@ export function update_messages(events) {
updated_messages.map((msg) => msg.id),
);
// For filters that cannot be processed locally, ask server.
maybe_add_narrowed_messages(
message_events_util.maybe_add_narrowed_messages(
event_messages,
message_lists.current,
message_util.add_messages,

89
web/src/message_events_util.js Normal file
View File

@ -0,0 +1,89 @@
import * as blueslip from "./blueslip";
import * as channel from "./channel";
import * as compose_notifications from "./compose_notifications";
import * as message_helper from "./message_helper";
import * as message_lists from "./message_lists";
import * as narrow_state from "./narrow_state";
import * as unread_ops from "./unread_ops";
import * as util from "./util";
// TODO: Move this function to 'message_util.ts' once #30702 is merged.
export function maybe_add_narrowed_messages(messages, msg_list, callback, attempt = 1) {
const ids = [];
for (const elem of messages) {
ids.push(elem.id);
}
channel.get({
url: "/json/messages/matches_narrow",
data: {
msg_ids: JSON.stringify(ids),
narrow: JSON.stringify(narrow_state.public_search_terms()),
},
timeout: 5000,
success(data) {
if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) {
// We unnarrowed or moved to Recent Conversations in the meantime.
return;
}
let new_messages = [];
const elsewhere_messages = [];
for (const elem of messages) {
if (Object.hasOwn(data.messages, elem.id)) {
util.set_match_data(elem, data.messages[elem.id]);
new_messages.push(elem);
} else {
elsewhere_messages.push(elem);
}
}
// This second call to process_new_message in the
// insert_new_messages code path is designed to replace
// our slightly stale message object with the latest copy
// from the message_store. This helps in very rare race
// conditions, where e.g. the current user's name was
// edited in between when they sent the message and when
// we hear back from the server and can echo the new
// message.
new_messages = new_messages.map((message) =>
message_helper.process_new_message(message),
);
callback(new_messages, msg_list);
unread_ops.process_visible();
compose_notifications.notify_messages_outside_current_search(elsewhere_messages);
},
error(xhr) {
if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) {
return;
}
if (xhr.status === 400) {
// This narrow was invalid -- don't retry it, and don't display the message.
return;
}
if (attempt >= 5) {
// Too many retries -- bail out. However, this means the `messages` are potentially
// missing from the search results view. Since this is a very unlikely circumstance
// (Tornado is up, Django is down for 5 retries, user is in a search view that it
// cannot apply itself) and the failure mode is not bad (it will simply fail to
// include live updates of new matching messages), just log an error.
blueslip.error(
"Failed to determine if new message matches current narrow, after 5 tries",
);
return;
}
// Backoff on retries, with full jitter: up to 2s, 4s, 8s, 16s, 32s
const delay = Math.random() * 2 ** attempt * 2000;
setTimeout(() => {
if (msg_list === message_lists.current) {
// Don't actually try again if we un-narrowed
// while waiting
maybe_add_narrowed_messages(messages, msg_list, callback, attempt + 1);
}
}, delay);
},
});
}

62
web/tests/echo.test.js
View File

@ -12,6 +12,7 @@ const {current_user} = require("./lib/zpage_params");
const compose_notifications = mock_esm("../src/compose_notifications");
const markdown = mock_esm("../src/markdown");
const message_lists = mock_esm("../src/message_lists");
const message_events_util = mock_esm("../src/message_events_util");
let disparities = [];
@ -39,6 +40,13 @@ message_lists.current = {
rerender_messages: noop,
change_message_id: noop,
},
data: {
filter: {
can_apply_locally() {
return true;
},
},
},
change_message_id: noop,
};
const home_msg_list = {
@ -46,6 +54,13 @@ const home_msg_list = {
rerender_messages: noop,
change_message_id: noop,
},
data: {
filter: {
can_apply_locally() {
return true;
},
},
},
preserver_rendered_state: true,
change_message_id: noop,
};
@ -123,6 +138,53 @@ run_test("process_from_server for differently rendered messages", ({override}) =
]);
});
run_test("process_from_server for messages to add to narrow", ({override}) => {
let messages_to_add_to_narrow = [];
override(message_lists.current.data.filter, "can_apply_locally", () => false);
override(message_events_util, "maybe_add_narrowed_messages", (msgs, msg_list) => {
messages_to_add_to_narrow = msgs;
assert.equal(msg_list, message_lists.current);
});
const old_value = "old_value";
const new_value = "new_value";
const waiting_for_ack = new Map([
[
"100.1",
{
content: "<p>rendered message</p>",
timestamp: old_value,
is_me_message: old_value,
submessages: old_value,
topic_links: old_value,
},
],
]);
const server_messages = [
{
local_id: "100.1",
content: "<p>rendered message</p>",
timestamp: new_value,
is_me_message: new_value,
submessages: new_value,
topic_links: new_value,
},
];
echo._patch_waiting_for_ack(waiting_for_ack);
const non_echo_messages = echo.process_from_server(server_messages);
assert.deepEqual(non_echo_messages, []);
assert.deepEqual(messages_to_add_to_narrow, [
{
content: server_messages[0].content,
timestamp: new_value,
is_me_message: new_value,
submessages: new_value,
topic_links: new_value,
},
]);
});
run_test("build_display_recipient", () => {
current_user.user_id = 123;