From d6beac48f8e91b7a190c3fd90ed40af918f89dc1 Mon Sep 17 00:00:00 2001 From: Keegan George Date: Thu, 5 Dec 2024 04:41:34 +0900 Subject: [PATCH] DEV: Improve explain suggestion footnote replacement (#999) Previously, when clicking add footnote on an explain suggestion it would replace the selected word by finding the first occurrence of the word. This results in issues when there are more than one occurrences of a word in a post. This is not trivial to solve, so this PR instead prevents incorrect text replacements by only allowing the replacement if it's unique. We use the same logic here that we use to determine if something can be fast edited. In this PR we also update tests for post helper explain suggestions. For a while, we haven't had tests here due to streaming/timing issues, we've been skipping our system specs. In this PR, we add acceptance tests to handle this which gives us improved ability to publish message bus updates in the testing environment so that it can be better tested without issues. --- .../components/ai-post-helper-menu.gjs | 29 ++++- .../ai-post-helper-trigger.gjs | 1 + config/locales/client.en.yml | 2 +- spec/system/ai_helper/ai_post_helper_spec.rb | 71 ----------- .../acceptance/post-helper-menu-test.js | 116 ++++++++++++++++++ .../javascripts/fixtures/ai-helper-prompts.js | 66 ++++++++++ 6 files changed, 212 insertions(+), 73 deletions(-) create mode 100644 test/javascripts/acceptance/post-helper-menu-test.js create mode 100644 test/javascripts/fixtures/ai-helper-prompts.js diff --git a/assets/javascripts/discourse/components/ai-post-helper-menu.gjs b/assets/javascripts/discourse/components/ai-post-helper-menu.gjs index 9a20037b..a51c9efa 100644 --- a/assets/javascripts/discourse/components/ai-post-helper-menu.gjs +++ b/assets/javascripts/discourse/components/ai-post-helper-menu.gjs @@ -4,6 +4,7 @@ import { action } from "@ember/object"; import didInsert from "@ember/render-modifiers/modifiers/did-insert"; import willDestroy from "@ember/render-modifiers/modifiers/will-destroy"; import { service } from "@ember/service"; +import { modifier } from "ember-modifier"; import { and } from "truth-helpers"; import CookText from "discourse/components/cook-text"; import DButton from "discourse/components/d-button"; @@ -26,6 +27,7 @@ export default class AiPostHelperMenu extends Component { @service siteSettings; @service currentUser; @service menu; + @service tooltip; @tracked menuState = this.MENU_STATES.options; @tracked loading = false; @@ -38,6 +40,7 @@ export default class AiPostHelperMenu extends Component { @tracked streaming = false; @tracked lastSelectedOption = null; @tracked isSavingFootnote = false; + @tracked supportsAddFootnote = this.args.data.supportsFastEdit; MENU_STATES = { options: "OPTIONS", @@ -45,8 +48,31 @@ export default class AiPostHelperMenu extends Component { result: "RESULT", }; + showFootnoteTooltip = modifier((element) => { + if (this.supportsAddFootnote || this.streaming) { + return; + } + + const instance = this.tooltip.register(element, { + identifier: "cannot-add-footnote-tooltip", + content: I18n.t( + "discourse_ai.ai_helper.post_options_menu.footnote_disabled" + ), + placement: "top", + triggers: "hover", + }); + + return () => { + instance.destroy(); + }; + }); + @tracked _activeAiRequest = null; + get footnoteDisabled() { + return this.streaming || !this.supportsAddFootnote; + } + get helperOptions() { let prompts = this.currentUser?.ai_helper_prompts; @@ -329,8 +355,9 @@ export default class AiPostHelperMenu extends Component { @label="discourse_ai.ai_helper.post_options_menu.insert_footnote" @action={{this.insertFootnote}} @isLoading={{this.isSavingFootnote}} - @disabled={{this.streaming}} + @disabled={{this.footnoteDisabled}} class="btn-flat ai-post-helper__suggestion__insert-footnote" + {{this.showFootnoteTooltip}} /> {{/if}} diff --git a/assets/javascripts/discourse/connectors/post-text-buttons/ai-post-helper-trigger.gjs b/assets/javascripts/discourse/connectors/post-text-buttons/ai-post-helper-trigger.gjs index ef39aac5..a211ada9 100644 --- a/assets/javascripts/discourse/connectors/post-text-buttons/ai-post-helper-trigger.gjs +++ b/assets/javascripts/discourse/connectors/post-text-buttons/ai-post-helper-trigger.gjs @@ -135,6 +135,7 @@ export default class AiPostHelperTrigger extends Component { identifier: "ai-post-helper-menu", component: AiPostHelperMenu, inline: true, + interactive: true, placement: this.shouldRenderUnder ? "bottom-start" : "top-start", fallbackPlacements: this.shouldRenderUnder ? ["bottom-end", "top-start"] diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 6a27f16c..b0893c91 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -145,7 +145,6 @@ en: response_tokens: "Response tokens" cached_tokens: "Cached tokens" - ai_persona: tool_strategies: all: "Apply to all replies" @@ -395,6 +394,7 @@ en: copied: "Copied!" cancel: "Cancel" insert_footnote: "Add footnote" + footnote_disabled: "Automatic insertion disabled, click copy button and edit it in manually" footnote_credits: "Explanation by AI" fast_edit: suggest_button: "Suggest edit" diff --git a/spec/system/ai_helper/ai_post_helper_spec.rb b/spec/system/ai_helper/ai_post_helper_spec.rb index e3ef4656..93c5244c 100644 --- a/spec/system/ai_helper/ai_post_helper_spec.rb +++ b/spec/system/ai_helper/ai_post_helper_spec.rb @@ -79,77 +79,6 @@ RSpec.describe "AI Post helper", type: :system, js: true do expect(post.like_count).to eq(1) end - context "when using explain mode" do - let(:mode) { CompletionPrompt::EXPLAIN } - - let(:explain_response) { <<~STRING } - In this context, pie refers to a baked dessert typically consisting of a pastry crust and filling. - The person states they enjoy eating pie, considering it a good dessert. They note that some people wastefully - throw pie at others, but the person themselves chooses to eat the pie rather than throwing it. Overall, pie - is being used to refer the the baked dessert food item. - STRING - - skip "TODO: Streaming causing timing issue in test" do - it "shows an explanation of the selected text" do - select_post_text(post) - post_ai_helper.click_ai_button - - DiscourseAi::Completions::Llm.with_prepared_responses([explain_response]) do - expected_value = explain_response.gsub(/"/, "").strip - - post_ai_helper.select_helper_model(mode) - Jobs.run_immediately! - - wait_for(timeout: 10) do - post_ai_helper.suggestion_value.gsub(/"/, "").strip == expected_value - end - - expect(post_ai_helper.suggestion_value.gsub(/"/, "").strip).to eq(expected_value) - end - end - - it "adds explained text as footnote to post" do - select_post_text(post) - post_ai_helper.click_ai_button - - DiscourseAi::Completions::Llm.with_prepared_responses([explain_response]) do - expected_value = explain_response.gsub(/"/, "").strip - - post_ai_helper.select_helper_model(mode) - Jobs.run_immediately! - - wait_for(timeout: 10) do - post_ai_helper.suggestion_value.gsub(/"/, "").strip == expected_value - end - - post_ai_helper.click_add_footnote - expect(page.has_css?(".expand-footnote")).to eq(true) - end - end - end - end - - context "when using translate mode" do - let(:mode) { CompletionPrompt::TRANSLATE } - - let(:translated_input) { "The rain in Spain, stays mainly in the Plane." } - - skip "TODO: Streaming causing timing issue in test" do - it "shows a translation of the selected text" do - select_post_text(post_2) - post_ai_helper.click_ai_button - - DiscourseAi::Completions::Llm.with_prepared_responses([translated_input]) do - post_ai_helper.select_helper_model(mode) - - wait_for { post_ai_helper.suggestion_value == translated_input } - - expect(post_ai_helper.suggestion_value).to eq(translated_input) - end - end - end - end - context "when using proofread mode" do let(:mode) { CompletionPrompt::PROOFREAD } let(:proofread_response) do diff --git a/test/javascripts/acceptance/post-helper-menu-test.js b/test/javascripts/acceptance/post-helper-menu-test.js new file mode 100644 index 00000000..4b50f03a --- /dev/null +++ b/test/javascripts/acceptance/post-helper-menu-test.js @@ -0,0 +1,116 @@ +import { click, settled, visit } from "@ember/test-helpers"; +import { test } from "qunit"; +import { AUTO_GROUPS } from "discourse/lib/constants"; +import topicFixtures from "discourse/tests/fixtures/topic"; +import { + acceptance, + publishToMessageBus, + query, + selectText, +} from "discourse/tests/helpers/qunit-helpers"; +import { cloneJSON } from "discourse-common/lib/object"; +import aiHelperPrompts from "../fixtures/ai-helper-prompts"; + +acceptance("AI Helper - Post Helper Menu", function (needs) { + needs.settings({ + discourse_ai_enabled: true, + ai_helper_enabled: true, + post_ai_helper_allowed_groups: "1|2", + ai_helper_enabled_features: "suggestions|context_menu", + share_quote_visibility: "anonymous", + enable_markdown_footnotes: true, + display_footnotes_inline: true, + }); + needs.user({ + admin: true, + moderator: true, + groups: [AUTO_GROUPS.admins], + can_use_assistant_in_post: true, + ai_helper_prompts: aiHelperPrompts, + trust_level: 4, + }); + needs.pretender((server, helper) => { + server.get("/t/1.json", () => { + const json = cloneJSON(topicFixtures["/t/28830/1.json"]); + json.post_stream.posts[0].can_edit_post = true; + json.post_stream.posts[0].can_edit = true; + return helper.response(json); + }); + + server.get("/t/2.json", () => { + const json = cloneJSON(topicFixtures["/t/28830/1.json"]); + json.post_stream.posts[0].cooked = + "

La lluvia en España se queda principalmente en el avión.

"; + return helper.response(json); + }); + + server.post(`/discourse-ai/ai-helper/stream_suggestion/`, () => { + return helper.response({ + result: "This is a suggestio", + done: false, + }); + }); + }); + + test("displays streamed explanation", async function (assert) { + await visit("/t/-/1"); + const suggestion = "This is a suggestion that is completed"; + const textNode = query("#post_1 .cooked p").childNodes[0]; + await selectText(textNode, 9); + await click(".ai-post-helper__trigger"); + await click(".ai-helper-options__button[data-name='explain']"); + await publishToMessageBus( + `/discourse-ai/ai-helper/stream_suggestion/118591`, + { + done: true, + result: suggestion, + } + ); + assert.dom(".ai-post-helper__suggestion__text").hasText(suggestion); + }); + + async function selectSpecificText(textNode, start, end) { + const range = document.createRange(); + range.setStart(textNode, start); + range.setEnd(textNode, end); + const selection = window.getSelection(); + selection.removeAllRanges(); + selection.addRange(range); + await settled(); + } + + test("adds explained text as footnote to post", async function (assert) { + await visit("/t/-/1"); + const suggestion = "This is a suggestion that is completed"; + + const textNode = query("#post_1 .cooked p").childNodes[0]; + await selectSpecificText(textNode, 72, 77); + await click(".ai-post-helper__trigger"); + await click(".ai-helper-options__button[data-name='explain']"); + await publishToMessageBus( + `/discourse-ai/ai-helper/stream_suggestion/118591`, + { + done: true, + result: suggestion, + } + ); + + assert.dom(".ai-post-helper__suggestion__insert-footnote").isDisabled(); + }); + + test("shows translated post", async function (assert) { + await visit("/t/-/2"); + const translated = "The rain in Spain, stays mainly in the Plane."; + await selectText(query("#post_1 .cooked p")); + await click(".ai-post-helper__trigger"); + await click(".ai-helper-options__button[data-name='translate']"); + await publishToMessageBus( + `/discourse-ai/ai-helper/stream_suggestion/118591`, + { + done: true, + result: translated, + } + ); + assert.dom(".ai-post-helper__suggestion__text").hasText(translated); + }); +}); diff --git a/test/javascripts/fixtures/ai-helper-prompts.js b/test/javascripts/fixtures/ai-helper-prompts.js new file mode 100644 index 00000000..79054ef8 --- /dev/null +++ b/test/javascripts/fixtures/ai-helper-prompts.js @@ -0,0 +1,66 @@ +export default [ + { + id: -301, + name: "translate", + translated_name: "Translate to English (US)", + prompt_type: "text", + icon: "language", + location: ["composer", "post"], + }, + { + id: -303, + name: "proofread", + translated_name: "Proofread text", + prompt_type: "diff", + icon: "spell-check", + location: ["composer", "post"], + }, + { + id: -304, + name: "markdown_table", + translated_name: "Generate Markdown table", + prompt_type: "diff", + icon: "table", + location: ["composer"], + }, + { + id: -305, + name: "custom_prompt", + translated_name: "Custom Prompt", + prompt_type: "diff", + icon: "comment", + location: ["composer", "post"], + }, + { + id: -306, + name: "explain", + translated_name: "Explain", + prompt_type: "text", + icon: "question", + location: ["post"], + }, + { + id: -307, + name: "generate_titles", + translated_name: "Suggest topic titles", + prompt_type: "list", + icon: "heading", + location: ["composer"], + }, + { + id: -308, + name: "illustrate_post", + translated_name: "Illustrate Post", + prompt_type: "list", + icon: "images", + location: ["composer"], + }, + { + id: -309, + name: "detect_text_locale", + translated_name: "detect_text_locale", + prompt_type: "text", + icon: null, + location: [], + }, +];