From 37c2930fbf21e0bab4b18ca30b5d23c123cf87f5 Mon Sep 17 00:00:00 2001 From: Keegan George Date: Tue, 22 Oct 2024 01:15:25 +0900 Subject: [PATCH] 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. --- .../ai-summary-box.gjs | 15 ++++++++-- .../javascripts/discourse/lib/ai-streamer.js | 20 +++---------- assets/stylesheets/common/streaming.scss | 28 ++++++++++++------- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs b/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs index 97b8322f..21f98360 100644 --- a/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs +++ b/assets/javascripts/discourse/connectors/topic-map-expanded-after/ai-summary-box.gjs @@ -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}} -
+
{{#if this.loading}} {{else}} -
{{this.text}}
+
+ {{this.text}} +
{{#if this.summarizedOn}}

diff --git a/assets/javascripts/discourse/lib/ai-streamer.js b/assets/javascripts/discourse/lib/ai-streamer.js index 494788c5..aa4d37e9 100644 --- a/assets/javascripts/discourse/lib/ai-streamer.js +++ b/assets/javascripts/discourse/lib/ai-streamer.js @@ -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() { diff --git a/assets/stylesheets/common/streaming.scss b/assets/stylesheets/common/streaming.scss index 3c9c08d3..b409c197 100644 --- a/assets/stylesheets/common/streaming.scss +++ b/assets/stylesheets/common/streaming.scss @@ -8,18 +8,26 @@ } } +@mixin progress-dot { + content: "\25CF"; + 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; + animation: flashing 1.5s 3s infinite; + 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 { - content: "\25CF"; - 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; - animation: flashing 1.5s 3s infinite; - display: inline-block; - font-size: 1rem; - color: var(--tertiary-medium); + @include progress-dot; } > .progress-dot:only-child::after {