DEV: Improve composer-messages implementation for PMs (#22529)

What does this commit do??

This commit introduces two changes:

1. As a follow up review comment to
   cc463c3e9b, we remove the top level
recipientNames cache in composer message to be a property of the
`ComposerMessage` component instead. Across components, we're more
likely to get a cache miss than a hit since we're caching the entire
recipient array so we can just drop it. If we really need this
optimisation, we should probably use a map and cache the information for
each user instead. However, the request is fairly cheap so we avoid that
optimisation for now.

2. This commit adds a debounce to `_typeReply` as well since we were not
   debouncing and the method was being called each time we received the
event.
This commit is contained in:
Alan Guo Xiang Tan 2023-07-11 10:49:27 +08:00 committed by GitHub
parent 433cb7092d
commit 91588cf938
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 32 additions and 10 deletions

View File

@ -7,13 +7,10 @@ import { not } from "@ember/object/computed";
import { ajax } from "discourse/lib/ajax"; import { ajax } from "discourse/lib/ajax";
import { inject as service } from "@ember/service"; import { inject as service } from "@ember/service";
import { tracked } from "@glimmer/tracking"; import { tracked } from "@glimmer/tracking";
import { INPUT_DELAY } from "discourse-common/config/environment";
import { debounce } from "discourse-common/utils/decorators";
let _messagesCache = {}; let _messagesCache = {};
let _recipientNames = [];
const clearRecipientNamesCache = function () {
_recipientNames.clear();
};
@classNameBindings(":composer-popup-container", "hidden") @classNameBindings(":composer-popup-container", "hidden")
export default class ComposerMessages extends Component { export default class ComposerMessages extends Component {
@ -26,6 +23,7 @@ export default class ComposerMessages extends Component {
queuedForTyping = null; queuedForTyping = null;
similarTopics = null; similarTopics = null;
usersNotSeen = null; usersNotSeen = null;
recipientNames = [];
@not("composer.viewOpenOrFullscreen") hidden; @not("composer.viewOpenOrFullscreen") hidden;
@ -51,8 +49,6 @@ export default class ComposerMessages extends Component {
this.appEvents.off("composer:find-similar", this, this._findSimilar); this.appEvents.off("composer:find-similar", this, this._findSimilar);
this.appEvents.off("composer-messages:close", this, this._closeTop); this.appEvents.off("composer-messages:close", this, this._closeTop);
this.appEvents.off("composer-messages:create", this, this._create); this.appEvents.off("composer-messages:create", this, this._create);
clearRecipientNamesCache();
} }
_closeTop() { _closeTop() {
@ -92,6 +88,7 @@ export default class ComposerMessages extends Component {
// Called after the user has typed a reply. // Called after the user has typed a reply.
// Some messages only get shown after being typed. // Some messages only get shown after being typed.
@debounce(INPUT_DELAY)
async _typedReply() { async _typedReply() {
if (this.isDestroying || this.isDestroyed) { if (this.isDestroying || this.isDestroyed) {
return; return;
@ -128,10 +125,10 @@ export default class ComposerMessages extends Component {
if ( if (
recipient_names.length > 0 && recipient_names.length > 0 &&
recipient_names.length !== _recipientNames.length && recipient_names.length !== this.recipientNames.length &&
!recipient_names.every((v, i) => v === _recipientNames[i]) !recipient_names.every((v, i) => v === this.recipientNames[i])
) { ) {
_recipientNames = recipient_names; this.recipientNames = recipient_names;
const response = await ajax( const response = await ajax(
`/composer_messages/user_not_seen_in_a_while`, `/composer_messages/user_not_seen_in_a_while`,

View File

@ -18,8 +18,16 @@ import pretender, { response } from "../helpers/create-pretender";
acceptance("Composer - Messages", function (needs) { acceptance("Composer - Messages", function (needs) {
needs.user(); needs.user();
let userNotSeenRequestCount = 0;
needs.hooks.afterEach(() => {
userNotSeenRequestCount = 0;
});
needs.pretender((server, helper) => { needs.pretender((server, helper) => {
server.get("/composer_messages/user_not_seen_in_a_while", () => { server.get("/composer_messages/user_not_seen_in_a_while", () => {
userNotSeenRequestCount += 1;
return helper.response({ return helper.response({
user_count: 1, user_count: 1,
usernames: ["charlie"], usernames: ["charlie"],
@ -31,13 +39,16 @@ acceptance("Composer - Messages", function (needs) {
test("Shows warning in composer if user hasn't been seen in a long time.", async function (assert) { test("Shows warning in composer if user hasn't been seen in a long time.", async function (assert) {
await visit("/u/charlie"); await visit("/u/charlie");
await click("button.compose-pm"); await click("button.compose-pm");
assert.false( assert.false(
exists(".composer-popup"), exists(".composer-popup"),
"composer warning is not shown by default" "composer warning is not shown by default"
); );
await triggerKeyEvent(".d-editor-input", "keyup", "Space"); await triggerKeyEvent(".d-editor-input", "keyup", "Space");
assert.true(exists(".composer-popup"), "shows composer warning message"); assert.true(exists(".composer-popup"), "shows composer warning message");
assert.true( assert.true(
query(".composer-popup").innerHTML.includes( query(".composer-popup").innerHTML.includes(
I18n.t("composer.user_not_seen_in_a_while.single", { I18n.t("composer.user_not_seen_in_a_while.single", {
@ -47,6 +58,20 @@ acceptance("Composer - Messages", function (needs) {
), ),
"warning message has correct body" "warning message has correct body"
); );
assert.strictEqual(
userNotSeenRequestCount,
1,
"ne user not seen request is made to the server"
);
await triggerKeyEvent(".d-editor-input", "keyup", "Space");
assert.strictEqual(
userNotSeenRequestCount,
1,
"does not make additional user not seen request to the server if the recipient names are the same"
);
}); });
}); });