From 574c132460311b6e4f1cc1cce43981d0f32cfe68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Saquetim?= Date: Fri, 6 Jun 2025 16:58:42 -0300 Subject: [PATCH 1/3] DEV: Improve quoted post handling and enhance decorator stability Added `topic` tracking to posts and improved quoted post handling by linking quoted posts to their parent topic. Introduced error handling for decorators to prevent crashes and ensure smoother operation in case of unexpected issues. These changes enhance reliability and data consistency across related components. --- .../app/components/post/cooked-html.gjs | 90 ++++++++++--------- .../quote-controls.js | 32 +++++-- .../javascripts/discourse/app/models/post.js | 3 + 3 files changed, 78 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs b/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs index c2398b5d8592f..17a051b80ce6e 100644 --- a/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs +++ b/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs @@ -47,48 +47,58 @@ export default class PostCookedHtml extends Component { [...POST_COOKED_DECORATORS, ...this.extraDecorators].forEach( (decorator) => { - if (!this.#decoratorState.has(decorator)) { - this.#decoratorState.set(decorator, {}); - } - - const owner = getOwner(this); - const decorationCleanup = decorator(element, { - data: { - post: this.args.post, - cooked: this.cooked, - highlightTerm: this.highlightTerm, - isIgnored: this.isIgnored, - ignoredUsers: this.ignoredUsers, - }, - createDetachedElement: this.#createDetachedElement, - currentUser: this.currentUser, - helper, - renderNestedPostCookedHtml: ( - nestedElement, - nestedPost, - extraDecorators - ) => { - const nestedArguments = { - post: nestedPost, - streamElement: false, + try { + if (!this.#decoratorState.has(decorator)) { + this.#decoratorState.set(decorator, {}); + } + + const owner = getOwner(this); + const decorationCleanup = decorator(element, { + data: { + post: this.args.post, + cooked: this.cooked, highlightTerm: this.highlightTerm, - extraDecorators: [ - ...this.extraDecorators, - ...makeArray(extraDecorators), - ], - }; - - helper.renderGlimmer( + isIgnored: this.isIgnored, + ignoredUsers: this.ignoredUsers, + }, + createDetachedElement: this.#createDetachedElement, + currentUser: this.currentUser, + helper, + renderNestedPostCookedHtml: ( nestedElement, - curryComponent(PostCookedHtml, nestedArguments, owner) - ); - }, - owner, - state: this.#decoratorState.get(decorator), - }); - - if (typeof decorationCleanup === "function") { - this.#pendingDecoratorCleanup.push(decorationCleanup); + nestedPost, + extraDecorators, + extraArguments + ) => { + const nestedArguments = { + ...extraArguments, + post: nestedPost, + streamElement: false, + highlightTerm: this.highlightTerm, + extraDecorators: [ + ...this.extraDecorators, + ...makeArray(extraDecorators), + ], + }; + + helper.renderGlimmer( + nestedElement, + curryComponent(PostCookedHtml, nestedArguments, owner) + ); + }, + owner, + state: this.#decoratorState.get(decorator), + }); + + if (typeof decorationCleanup === "function") { + this.#pendingDecoratorCleanup.push(decorationCleanup); + } + } catch (e) { + // in case one of the decorators throws an error we want to surface it to the console but prevent + // the application from crashing + + // eslint-disable-next-line no-console + console.error(e); } } ); diff --git a/app/assets/javascripts/discourse/app/lib/post-cooked-html-decorators/quote-controls.js b/app/assets/javascripts/discourse/app/lib/post-cooked-html-decorators/quote-controls.js index 08f7c31b23587..89bea0f169d5e 100644 --- a/app/assets/javascripts/discourse/app/lib/post-cooked-html-decorators/quote-controls.js +++ b/app/assets/javascripts/discourse/app/lib/post-cooked-html-decorators/quote-controls.js @@ -1,5 +1,6 @@ import { spinnerHTML } from "discourse/helpers/loading-spinner"; import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; import escape from "discourse/lib/escape"; import highlightHTML from "discourse/lib/highlight-html"; import { iconHTML } from "discourse/lib/icon-library"; @@ -72,7 +73,8 @@ function _updateQuoteElements(aside, desc, context) { if ( topicNumber && postNumber && - topicNumber === data.post.topic_id?.toString() + topicNumber === data.post.topic_id?.toString() && + data.post.topic ) { const topicId = data.post.topic_id; const slug = data.post.topic.slug; @@ -117,8 +119,13 @@ function _updateQuoteElements(aside, desc, context) { } async function _toggleQuote(aside, context) { - const { createDetachedElement, data, renderNestedPostCookedHtml, state } = - context; + const { + createDetachedElement, + data, + renderNestedPostCookedHtml, + state, + owner, + } = context; if (state.expanding) { return; @@ -155,12 +162,21 @@ async function _toggleQuote(aside, context) { const postId = parseInt(aside.dataset.post, 10); try { - const quotedPost = await ajax(`/posts/by_number/${topicId}/${postId}`); - const post = data.post; + const quotedPost = owner + .lookup("service:store") + .createRecord( + "post", + await ajax(`/posts/by_number/${topicId}/${postId}`) + ); + + if (quotedPost.topic_id === post?.topic_id) { + quotedPost.topic = post.topic; + } + const quotedPosts = post.quoted || {}; quotedPosts[quotedPost.id] = quotedPost; - post.set("quoted", quotedPosts); + post.quoted = quotedPosts; const div = createDetachedElement("div"); div.classList.add("expanded-quote"); @@ -177,9 +193,11 @@ async function _toggleQuote(aside, context) { blockQuote.innerHTML = ""; blockQuote.appendChild(div); } catch (e) { - if ([403, 404].includes(e.jqXHR.status)) { + if (e.jqXHR && [403, 404].includes(e.jqXHR.status)) { const icon = iconHTML(e.jqXHR.status === 403 ? "lock" : "trash-can"); blockQuote.innerHTML = `
${icon}
`; + } else { + popupAjaxError(e); } } } else { diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index 515136b0a455c..8a450250c26fe 100644 --- a/app/assets/javascripts/discourse/app/models/post.js +++ b/app/assets/javascripts/discourse/app/models/post.js @@ -192,12 +192,15 @@ export default class Post extends RestModel { @trackedPostProperty post_number; @trackedPostProperty post_type; @trackedPostProperty primary_group_name; + @trackedPostProperty quoted; @trackedPostProperty read; @trackedPostProperty reply_count; @trackedPostProperty reply_to_user; @trackedPostProperty staff; @trackedPostProperty staged; @trackedPostProperty title_is_group; + @trackedPostProperty topic_id; + @trackedPostProperty topic; @trackedPostProperty trust_level; @trackedPostProperty updated_at; @trackedPostProperty user; From 1277902dfa6e8a444cb4a1f7f549527d75bbce53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Saquetim?= Date: Mon, 9 Jun 2025 15:50:23 -0300 Subject: [PATCH 2/3] Add tests for nested quote toggle functionality This commit introduces tests to ensure proper handling of nested quotes in the Glimmer Post Stream mode. It verifies that quote toggles expand and collapse correctly, including nested quotes within outer quotes. A new server fixture is also added to simulate the required post data for testing. --- .../discourse/tests/acceptance/topic-test.js | 98 +++++++++++++++++++ .../discourse/tests/fixtures/topic.js | 2 +- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js index 4fd11f1cbb5ac..731f2bae86836 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-test.js @@ -666,6 +666,12 @@ import { i18n } from "discourse-i18n"; topicFixtures["/t/280/1.json"].post_stream.posts[2] ) ); + server.get("/posts/by_number/280/5", () => + helper.response( + 200, + topicFixtures["/t/280/1.json"].post_stream.posts[4] + ) + ); }); test("The quoted content is toggled correclty", async function (assert) { @@ -711,6 +717,98 @@ import { i18n } from "discourse-i18n"; ) .hasAttribute("aria-expanded", "false"); }); + + // TODO (glimmer-post-stream): this test only works with the Glimmer Post Stream. + // When the removing the legacy code we should remove the `if` and run it unconditionally. + if (postStreamMode === "enabled") { + test("Nesting quoted content works", async function (assert) { + await visit("/t/internationalization-localization/280"); + + // outer quote + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] > .title .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "false"); + + assert + .dom("#post_7 .quote[data-topic='280'][data-post='5']") + .includesText("The problem I see here") + .doesNotIncludeText("So you could replace that lookup table"); + + await click( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] > .title .quote-controls .quote-toggle" + ); + + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] > .title .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "true"); + assert + .dom("#post_7 .quote[data-topic='280'][data-post='5']") + .includesText("The problem I see here") + .includesText("So you could replace that lookup table"); + + // nested quote + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3'] .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "false"); + assert + .dom( + "#post_7 .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3']" + ) + .includesText( + 'So you could replace that lookup table with the "de" one to get German.' + ) + .doesNotIncludeText( + "Yep, all strings are going through a lookup table.*" + ); + + await click( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3'] .quote-controls .quote-toggle" + ); + + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3'] .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "true"); + assert + .dom( + "#post_7 .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3']" + ) + .includesText( + 'So you could replace that lookup table with the "de" one to get German.' + ) + .includesText( + "Yep, all strings are going through a lookup table.*" + ); + + await click( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3'] .quote-controls .quote-toggle" + ); + + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote[data-topic='280'][data-post='3'] .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "false"); + + // outer quote + await click( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote-controls .quote-toggle" + ); + + assert + .dom( + "#post_7 .cooked .quote[data-topic='280'][data-post='5'] .quote-controls .quote-toggle" + ) + .hasAttribute("aria-expanded", "false"); + }); + } } ); diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index eb38516ae7102..e0ce9c068e100 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -874,7 +874,7 @@ export default { uploaded_avatar_id: 5253, created_at: "2013-02-07T14:08:17.493Z", cooked: - '

Looks interesting, I\'ll take a peek.

\n\n

As said on dev, the best tool I can see in terms of giving translators a proper interface and quality control would be something like GlotPress. It\'s based on the PO messages format (is that somehow related to ICU?) but looks pretty great.

\n\n

\n\n

I\'m not familiar with the term in this context, you mean keeping the English version in the code base (instead of a generic code like message_error_nametooshort ?)

', + '

Looks interesting, I\'ll take a peek.

\n\n

As said on dev, the best tool I can see in terms of giving translators a proper interface and quality control would be something like GlotPress. It\'s based on the PO messages format (is that somehow related to ICU?) but looks pretty great.

\n\n

\n\n

I\'m not familiar with the term in this context, you mean keeping the English version in the code base (instead of a generic code like message_error_nametooshort ?)

', post_number: 7, post_type: 1, updated_at: "2013-02-07T14:12:02.965Z", From 3be672a4a3fe9a078f57f823b26f38f2cbd559f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Saquetim?= Date: Tue, 10 Jun 2025 17:54:18 -0300 Subject: [PATCH 3/3] Handle cooked decorator errors differently during testing In testing environments, re-throw errors from decorators for better visibility and debugging. In non-testing scenarios, log the errors to the console to avoid crashing the application. --- .../discourse/app/components/post/cooked-html.gjs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs b/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs index 17a051b80ce6e..cfc2519f0b2dd 100644 --- a/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs +++ b/app/assets/javascripts/discourse/app/components/post/cooked-html.gjs @@ -6,6 +6,7 @@ import { htmlSafe } from "@ember/template"; import curryComponent from "ember-curry-component"; import DecoratedHtml from "discourse/components/decorated-html"; import { bind } from "discourse/lib/decorators"; +import { isRailsTesting, isTesting } from "discourse/lib/environment"; import { makeArray } from "discourse/lib/helpers"; import decorateLinkCounts from "discourse/lib/post-cooked-html-decorators/link-counts"; import decorateMentions from "discourse/lib/post-cooked-html-decorators/mentions"; @@ -94,11 +95,15 @@ export default class PostCookedHtml extends Component { this.#pendingDecoratorCleanup.push(decorationCleanup); } } catch (e) { - // in case one of the decorators throws an error we want to surface it to the console but prevent - // the application from crashing - - // eslint-disable-next-line no-console - console.error(e); + if (isRailsTesting() || isTesting()) { + throw e; + } else { + // in case one of the decorators throws an error we want to surface it to the console but prevent + // the application from crashing + + // eslint-disable-next-line no-console + console.error(e); + } } } );