DEV: Improve reactivity of user-tips and remove runloop workarounds (#23897)

Previously, the `user-tips` service included a couple of calls to `next()`. These were introduced to work around errors like

```
You attempted to update `availableTips` on `<UserTips:ember659>`, but it had already been used previously in the same computation
```

These errors come from the fact that various `<UserTip>` components are rendering at slightly different times in the runloop and stepping on each other. Normally this doesn't happen in Ember, but the implementation details of our 'Widget' system and its 'RenderGlimmer' helper mean that RenderGlimmer components are rendered later than normal Ember components. Using `next()` avoids the problem because it means that all the updates are scheduled together in the following runloop interation.

However, the use of `next()` can create some subtle timing issues, which have been evident in the recent flakiness of some qunit tests. This commit makes a few changes to improve the situation:

1. Use a TrackedMap to provide fine-grained `shouldRender()` reactivity for each user-tip id. That means that different user tips will not be trying to update the same piece of tracked state (previously the entire `availableTips` array was `@tracked`, and was completely re-assigned every time a new `<UserTip>` was rendered

2. Avoid reassigning any tracked state unless the value has actually changed

3. Remove the `next()` workarounds
This commit is contained in:
David Taylor 2023-10-11 14:03:31 +01:00 committed by GitHub
parent 6970c7dc13
commit 597ef11195
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 31 additions and 24 deletions

View File

@ -15,13 +15,15 @@ export default class UserTip extends Component {
@service tooltip;
registerTip = modifier(() => {
this.userTips.addAvailableTip({
const tip = {
id: this.args.id,
priority: this.args.priority ?? 0,
});
};
this.userTips.addAvailableTip(tip);
return () => {
this.userTips.removeAvailableTip({ id: this.args.id });
this.userTips.removeAvailableTip(tip);
};
});
@ -69,7 +71,7 @@ export default class UserTip extends Component {
});
get shouldRenderTip() {
return this.userTips.renderedId === this.args.id;
return this.userTips.shouldRender(this.args.id);
}
<template>

View File

@ -1,6 +1,5 @@
import { tracked } from "@glimmer/tracking";
import { next } from "@ember/runloop";
import Service, { inject as service } from "@ember/service";
import { TrackedMap } from "@ember-compat/tracked-built-ins";
import { disableImplicitInjections } from "discourse/lib/implicit-injections";
import Site from "discourse/models/site";
import { isTesting } from "discourse-common/config/environment";
@ -10,15 +9,17 @@ export default class UserTips extends Service {
@service site;
@service currentUser;
@tracked availableTips = [];
@tracked renderedId;
#availableTips = new Set();
#renderedId;
#shouldRenderMap = new TrackedMap();
computeRenderedId() {
if (this.availableTips.find((tip) => tip.id === this.renderedId)) {
return this.renderedId;
#updateRenderedId() {
const tipsArray = [...this.#availableTips];
if (tipsArray.find((tip) => tip.id === this.#renderedId)) {
return;
}
return this.availableTips
const newId = tipsArray
.sortBy("priority")
.reverse()
.find((tip) => {
@ -26,23 +27,26 @@ export default class UserTips extends Service {
return tip.id;
}
})?.id;
if (this.#renderedId !== newId) {
this.#shouldRenderMap.delete(this.#renderedId);
this.#shouldRenderMap.set(newId, true);
this.#renderedId = newId;
}
}
shouldRender(id) {
return this.#shouldRenderMap.get(id);
}
addAvailableTip(tip) {
next(() => {
this.availableTips = [...this.availableTips, tip];
this.renderedId = this.computeRenderedId();
});
this.#availableTips.add(tip);
this.#updateRenderedId();
}
removeAvailableTip(tip) {
next(() => {
this.availableTips = this.availableTips.filter((availableTip) => {
return tip.id !== availableTip.id;
});
this.renderedId = this.computeRenderedId();
});
this.#availableTips.delete(tip);
this.#updateRenderedId();
}
canSeeUserTip(tipId) {
@ -89,7 +93,8 @@ export default class UserTips extends Service {
return;
}
this.removeAvailableTip({ id: tipId });
const tipObj = [...this.#availableTips].find((t) => t.id === tipId);
this.removeAvailableTip(tipObj);
// Update list of seen user tips.
let seenUserTips = this.currentUser.user_option?.seen_popups || [];