FIX: Decouple DOM manipulation from SummaryStreamer (#844)
Previously, when we added smooth streaming animation to summarization (https://github.com/discourse/discourse-ai/pull/778) we used the same logic and lib we did for AI Bot. However, since `AiSummaryBox` is an Ember component, the direct DOM manipulation done in the streamer (`SummaryUpdater`) would often result in issues with summarization where sometimes summarization updates would hang, especially on the last result. This is likely due to the DOM manipulation being done in the streamer being incongruent with Ember's way of rendering. In this PR, we remove the direct DOM manipulation done in the lib `SummaryUpdater` in favour of directly updating the properties in `AiSummaryBox` using the `componentContext`. Instead of messing with Ember's rendered DOM, passing the updates and allowing the component to render the updates directly should likely prevent further issues with summarization. The bug itself is quite difficult to repro and also difficult to test, so no tests have been added to this PR. But I will be manually testing and assessing for any potential issues.
This commit is contained in:
parent
e768fa877e
commit
37c2930fbf
|
@ -8,6 +8,7 @@ import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";
|
|||
import { next } from "@ember/runloop";
|
||||
import { service } from "@ember/service";
|
||||
import DButton from "discourse/components/d-button";
|
||||
import concatClass from "discourse/helpers/concat-class";
|
||||
import { ajax } from "discourse/lib/ajax";
|
||||
import { shortDateNoYear } from "discourse/lib/formatter";
|
||||
import dIcon from "discourse-common/helpers/d-icon";
|
||||
|
@ -32,6 +33,7 @@ export default class AiSummaryBox extends Component {
|
|||
@tracked outdated = false;
|
||||
@tracked canRegenerate = false;
|
||||
@tracked loading = false;
|
||||
@tracked isStreaming = false;
|
||||
oldRaw = null; // used for comparison in SummaryUpdater in lib/ai-streamer
|
||||
finalSummary = null;
|
||||
|
||||
|
@ -147,9 +149,11 @@ export default class AiSummaryBox extends Component {
|
|||
};
|
||||
this.loading = false;
|
||||
|
||||
this.isStreaming = true;
|
||||
streamSummaryText(topicSummary, this);
|
||||
|
||||
if (update.done) {
|
||||
this.isStreaming = false;
|
||||
this.text = this.finalSummary;
|
||||
this.summarizedOn = shortDateNoYear(
|
||||
moment(topicSummary.updated_at, "YYYY-MM-DD HH:mm:ss Z")
|
||||
|
@ -212,11 +216,18 @@ export default class AiSummaryBox extends Component {
|
|||
{{/if}}
|
||||
</header>
|
||||
|
||||
<article class="ai-summary-box">
|
||||
<article
|
||||
class={{concatClass
|
||||
"ai-summary-box"
|
||||
(if this.isStreaming "streaming")
|
||||
}}
|
||||
>
|
||||
{{#if this.loading}}
|
||||
<AiSummarySkeleton />
|
||||
{{else}}
|
||||
<div class="generated-summary cooked">{{this.text}}</div>
|
||||
<div class="generated-summary cooked">
|
||||
{{this.text}}
|
||||
</div>
|
||||
{{#if this.summarizedOn}}
|
||||
<div class="summarized-on">
|
||||
<p>
|
||||
|
|
|
@ -152,9 +152,9 @@ export class SummaryUpdater extends StreamUpdater {
|
|||
set streaming(value) {
|
||||
if (this.element) {
|
||||
if (value) {
|
||||
this.element.classList.add("streaming");
|
||||
this.componentContext.isStreaming = true;
|
||||
} else {
|
||||
this.element.classList.remove("streaming");
|
||||
this.componentContext.isStreaming = false;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -163,18 +163,7 @@ export class SummaryUpdater extends StreamUpdater {
|
|||
this.componentContext.oldRaw = value;
|
||||
const cooked = await cook(value);
|
||||
|
||||
// resets animation
|
||||
this.element.classList.remove("streaming");
|
||||
void this.element.offsetWidth;
|
||||
this.element.classList.add("streaming");
|
||||
|
||||
const cookedElement = document.createElement("div");
|
||||
cookedElement.innerHTML = cooked;
|
||||
|
||||
if (!done) {
|
||||
addProgressDot(cookedElement);
|
||||
}
|
||||
await this.setCooked(cookedElement.innerHTML);
|
||||
await this.setCooked(cooked);
|
||||
|
||||
if (done) {
|
||||
this.componentContext.finalSummary = cooked;
|
||||
|
@ -182,8 +171,7 @@ export class SummaryUpdater extends StreamUpdater {
|
|||
}
|
||||
|
||||
async setCooked(value) {
|
||||
const cookedContainer = this.element.querySelector(".generated-summary");
|
||||
cookedContainer.innerHTML = value;
|
||||
this.componentContext.text = value;
|
||||
}
|
||||
|
||||
get raw() {
|
||||
|
|
|
@ -8,11 +8,10 @@
|
|||
}
|
||||
}
|
||||
|
||||
article.streaming .cooked {
|
||||
.progress-dot::after {
|
||||
@mixin progress-dot {
|
||||
content: "\25CF";
|
||||
font-family: Söhne Circle, system-ui, -apple-system, Segoe UI, Roboto,
|
||||
Ubuntu, Cantarell, Noto Sans, sans-serif;
|
||||
font-family: Söhne Circle, system-ui, -apple-system, Segoe UI, Roboto, Ubuntu,
|
||||
Cantarell, Noto Sans, sans-serif;
|
||||
line-height: normal;
|
||||
margin-left: 0.25rem;
|
||||
vertical-align: baseline;
|
||||
|
@ -20,6 +19,15 @@ article.streaming .cooked {
|
|||
display: inline-block;
|
||||
font-size: 1rem;
|
||||
color: var(--tertiary-medium);
|
||||
}
|
||||
|
||||
.streaming .cooked p:last-child::after {
|
||||
@include progress-dot;
|
||||
}
|
||||
|
||||
article.streaming .cooked {
|
||||
.progress-dot::after {
|
||||
@include progress-dot;
|
||||
}
|
||||
|
||||
> .progress-dot:only-child::after {
|
||||
|
|
Loading…
Reference in New Issue