From 6a33e5154d9f7648191a8d3373b5d2ea2c65e6f7 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 19 Jun 2025 11:56:00 +0200 Subject: [PATCH] DEV: makes ai menu helper a standalone menu (#1434) The current menu was rendering inside the post text toolbar (on desktop). This is not ideal as the post text toolbar rendering is conditioned on the presence of text selection, when you click a button on the toolbar, by design of the web browsers you will lose your text selection, making all of this super tricky. This commit makes desktop and mobile behave in the same way by rendering their own menu and capturing the quote state when we render the post text selection toolbar, this allows us to reason a much simpler way about the AI helper. This commit also removes what appears to be an unused file and corrects which was seemingly copy/paste mistakes. :warning: Technical note, this commit is correcting the message bus subscription which amongst other things allows to write specs which are not flaky. However due to the current implementation we have a channel per post, which means we need to serialize on last message bus id per post. We have two possible solutions here: - subscribe at the topic level - refactor the code to be able to use `MessageBus.last_ids` to be able to grab multiple posts at once instead of having to call `MessageBus.last_id` and done one Redis call per post --------- Co-authored-by: Keegan George --- .../components/ai-post-helper-menu.gjs | 52 +++---- .../ai-post-helper-trigger.gjs | 133 ++++++------------ .../lib/virtual-element-from-caret-coords.js | 45 ------ lib/ai_helper/entry_point.rb | 6 + spec/system/ai_helper/ai_post_helper_spec.rb | 50 ++++++- .../components/ai_post_helper_menu.rb | 13 +- 6 files changed, 123 insertions(+), 176 deletions(-) delete mode 100644 assets/javascripts/discourse/lib/virtual-element-from-caret-coords.js diff --git a/assets/javascripts/discourse/components/ai-post-helper-menu.gjs b/assets/javascripts/discourse/components/ai-post-helper-menu.gjs index 898edb67..8edacb48 100644 --- a/assets/javascripts/discourse/components/ai-post-helper-menu.gjs +++ b/assets/javascripts/discourse/components/ai-post-helper-menu.gjs @@ -14,7 +14,6 @@ import concatClass from "discourse/helpers/concat-class"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; import { bind } from "discourse/lib/decorators"; -import { withPluginApi } from "discourse/lib/plugin-api"; import { sanitize } from "discourse/lib/text"; import { clipboardCopy } from "discourse/lib/utilities"; import { i18n } from "discourse-i18n"; @@ -44,6 +43,9 @@ export default class AiPostHelperMenu extends Component { @tracked lastSelectedOption = null; @tracked isSavingFootnote = false; @tracked supportsAddFootnote = this.args.data.supportsFastEdit; + @tracked + channel = + `/discourse-ai/ai-helper/stream_suggestion/${this.args.data.quoteState.postId}`; @tracked smoothStreamer = new SmoothStreamer( @@ -78,23 +80,6 @@ export default class AiPostHelperMenu extends Component { @tracked _activeAiRequest = null; - constructor() { - super(...arguments); - - withPluginApi((api) => { - api.registerValueTransformer( - "post-text-selection-prevent-close", - ({ value }) => { - if (this.menuState === this.MENU_STATES.result) { - return true; - } - - return value; - } - ); - }); - } - get footnoteDisabled() { return this.streaming || !this.supportsAddFootnote; } @@ -167,16 +152,17 @@ export default class AiPostHelperMenu extends Component { @bind subscribe() { - const channel = `/discourse-ai/ai-helper/stream_suggestion/${this.args.data.quoteState.postId}`; - this.messageBus.subscribe(channel, this._updateResult); + this.messageBus.subscribe( + this.channel, + (data) => this._updateResult(data), + this.args.data.post + .discourse_ai_helper_stream_suggestion_last_message_bus_id + ); } @bind unsubscribe() { - this.messageBus.unsubscribe( - "/discourse-ai/ai-helper/stream_suggestion/*", - this._updateResult - ); + this.messageBus.unsubscribe(this.channel, this._updateResult); } @bind @@ -239,7 +225,7 @@ export default class AiPostHelperMenu extends Component { data: { location: "post", mode: option.name, - text: this.args.data.selectedText, + text: this.args.data.quoteState.buffer, post_id: this.args.data.quoteState.postId, custom_prompt: this.customPromptValue, client_id: this.messageBus.clientId, @@ -292,12 +278,10 @@ export default class AiPostHelperMenu extends Component { @action closeMenu() { - if (this.site.mobileView) { - return this.args.close(); - } - - const menu = this.menu.getByIdentifier("post-text-selection-toolbar"); - return menu?.close(); + // reset state and close + this.suggestion = ""; + this.customPromptValue = ""; + return this.args.close(); } @action @@ -317,9 +301,9 @@ export default class AiPostHelperMenu extends Component { const credits = i18n( "discourse_ai.ai_helper.post_options_menu.footnote_credits" ); - const withFootnote = `${this.args.data.selectedText} ^[${sanitizedSuggestion} (${credits})]`; + const withFootnote = `${this.args.data.quoteState.buffer} ^[${sanitizedSuggestion} (${credits})]`; const newRaw = result.raw.replace( - this.args.data.selectedText, + this.args.data.quoteState.buffer, withFootnote ); @@ -338,7 +322,7 @@ export default class AiPostHelperMenu extends Component { (and this.site.mobileView (eq this.menuState this.MENU_STATES.options)) }}
- {{@data.selectedText}} + {{@data.quoteState.buffer}}
{{/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 d429fbe3..2a0ec521 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 @@ -3,8 +3,7 @@ import { tracked } from "@glimmer/tracking"; import { action } from "@ember/object"; import { service } from "@ember/service"; import DButton from "discourse/components/d-button"; -import virtualElementFromTextRange from "discourse/lib/virtual-element-from-text-range"; -import eq from "truth-helpers/helpers/eq"; +import { selectedRange } from "discourse/lib/utilities"; import AiPostHelperMenu from "../../components/ai-post-helper-menu"; import { showPostAIHelper } from "../../lib/show-ai-helper"; @@ -13,27 +12,27 @@ export default class AiPostHelperTrigger extends Component { return showPostAIHelper(outletArgs, helper); } - @service site; @service menu; - @tracked menuState = this.MENU_STATES.triggers; - @tracked showMainButtons = true; - @tracked showAiButtons = true; @tracked postHighlighted = false; @tracked currentMenu = this.menu.getByIdentifier( "post-text-selection-toolbar" ); - MENU_STATES = { - triggers: "TRIGGERS", - options: "OPTIONS", + // capture the state at the moment the toolbar is rendered + // so we ensure change of state (selection change for example) + // is not impacting the menu data + menuData = { + ...this.args.outletArgs.data, + quoteState: { + buffer: this.args.outletArgs.data.quoteState.buffer, + opts: this.args.outletArgs.data.quoteState.opts, + postId: this.args.outletArgs.data.quoteState.postId, + }, + post: this.args.outletArgs.post, + selectedRange: selectedRange(), }; - willDestroy() { - super.willDestroy(...arguments); - this.removeHighlightedText(); - } - highlightSelectedText() { const postId = this.args.outletArgs.data.quoteState.postId; const postElement = document.querySelector( @@ -44,14 +43,7 @@ export default class AiPostHelperTrigger extends Component { return; } - this.selectedText = this.args.outletArgs.data.quoteState.buffer; - - const selection = window.getSelection(); - if (!selection.rangeCount) { - return; - } - - const range = selection.getRangeAt(0); + const range = this.menuData.selectedRange; // Split start/end text nodes at their range boundary if ( @@ -97,11 +89,10 @@ export default class AiPostHelperTrigger extends Component { // Replace textNode with highlighted clone const clone = textNode.cloneNode(true); highlight.appendChild(clone); - textNode.parentNode.replaceChild(highlight, textNode); } - selection.removeAllRanges(); + window.getSelection().removeAllRanges(); this.postHighlighted = true; } @@ -110,16 +101,7 @@ export default class AiPostHelperTrigger extends Component { return; } - const postId = this.args.outletArgs.data.quoteState.postId; - const postElement = document.querySelector( - `article[data-post-id='${postId}'] .cooked` - ); - - if (!postElement) { - return; - } - - const highlightedSpans = postElement.querySelectorAll( + const highlightedSpans = document.querySelectorAll( "span.ai-helper-highlighted-selection" ); @@ -133,65 +115,40 @@ export default class AiPostHelperTrigger extends Component { @action async showAiPostHelperMenu() { - this.highlightSelectedText(); - if (this.site.mobileView) { - this.currentMenu.close(); + await this.currentMenu.close(); - await this.menu.show(virtualElementFromTextRange(), { - 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"] - : ["bottom-start"], - trapTab: false, - closeOnScroll: false, - modalForMobile: true, - data: this.menuData, - }); - } - - this.showMainButtons = false; - this.menuState = this.MENU_STATES.options; - } - - get menuData() { - // Streamline of data model to be passed to the component when - // instantiated as a DMenu or a simple component in the template - return { - ...this.args.outletArgs.data, - quoteState: { - buffer: this.args.outletArgs.data.quoteState.buffer, - opts: this.args.outletArgs.data.quoteState.opts, - postId: this.args.outletArgs.data.quoteState.postId, + await this.menu.show(this.currentMenu.trigger, { + identifier: "ai-post-helper-menu", + component: AiPostHelperMenu, + interactive: true, + trapTab: false, + closeOnScroll: false, + modalForMobile: true, + data: this.menuData, + placement: "top-start", + fallbackPlacements: ["bottom-start"], + updateOnScroll: false, + onClose: () => { + this.removeHighlightedText(); }, - post: this.args.outletArgs.post, - selectedText: this.selectedText, - }; + }); + + await this.currentMenu.destroy(); + + this.highlightSelectedText(); } } diff --git a/assets/javascripts/discourse/lib/virtual-element-from-caret-coords.js b/assets/javascripts/discourse/lib/virtual-element-from-caret-coords.js deleted file mode 100644 index eb0b76c9..00000000 --- a/assets/javascripts/discourse/lib/virtual-element-from-caret-coords.js +++ /dev/null @@ -1,45 +0,0 @@ -class VirtualElementFromCaretCoords { - constructor(caretCoords, offset = [0, 0]) { - this.caretCoords = caretCoords; - this.offset = offset; - this.updateRect(); - } - - updateRect() { - const [xOffset, yOffset] = this.offset; - this.rect = { - top: this.caretCoords.y + yOffset, - right: this.caretCoords.x, - bottom: this.caretCoords.y + yOffset, - left: this.caretCoords.x + xOffset, - width: 0, - height: 0, - x: this.caretCoords.x, - y: this.caretCoords.y, - toJSON() { - return this; - }, - }; - return this.rect; - } - - getBoundingClientRect() { - return this.rect; - } - - getClientRects() { - return [this.rect]; - } - - get clientWidth() { - return this.rect.width; - } - - get clientHeight() { - return this.rect.height; - } -} - -export default function virtualElementFromCaretCoords(caretCoords, offset) { - return new VirtualElementFromCaretCoords(caretCoords, offset); -} diff --git a/lib/ai_helper/entry_point.rb b/lib/ai_helper/entry_point.rb index cef7e2a4..41ce1780 100644 --- a/lib/ai_helper/entry_point.rb +++ b/lib/ai_helper/entry_point.rb @@ -73,6 +73,12 @@ module DiscourseAi scope.user.in_any_groups?(SiteSetting.ai_auto_image_caption_allowed_groups_map) end, ) { object.auto_image_caption } + + plugin.add_to_serializer( + :post, + :discourse_ai_helper_stream_suggestion_last_message_bus_id, + include_condition: -> { SiteSetting.ai_helper_enabled && scope.authenticated? }, + ) { MessageBus.last_id("/discourse-ai/ai-helper/stream_suggestion/#{object.id}") } end end end diff --git a/spec/system/ai_helper/ai_post_helper_spec.rb b/spec/system/ai_helper/ai_post_helper_spec.rb index 67bca366..14c1b576 100644 --- a/spec/system/ai_helper/ai_post_helper_spec.rb +++ b/spec/system/ai_helper/ai_post_helper_spec.rb @@ -30,6 +30,7 @@ RSpec.describe "AI Post helper", type: :system, js: true do Group.find_by(id: Group::AUTO_GROUPS[:admins]).add(user) assign_fake_provider_to(:ai_helper_model) SiteSetting.ai_helper_enabled = true + Jobs.run_immediately! sign_in(user) end @@ -87,13 +88,52 @@ RSpec.describe "AI Post helper", type: :system, js: true do end it "pre-fills fast edit with proofread text" do - skip("Test is flaky in CI, possibly some timing issue?") if ENV["CI"] select_post_text(post_3) post_ai_helper.click_ai_button DiscourseAi::Completions::Llm.with_prepared_responses([proofread_response]) do post_ai_helper.select_helper_model(mode) - wait_for { fast_editor.has_content?(proofread_response) } - expect(fast_editor).to have_content(proofread_response) + expect(page.find("#fast-edit-input")).to have_content(proofread_response) + end + end + end + + context "when using explain mode" do + let(:mode) { DiscourseAi::AiHelper::Assistant::EXPLAIN } + let(:explain_response) { "This is about pie." } + + it "shows the explanation in the AI helper" do + select_post_text(post) + post_ai_helper.click_ai_button + + DiscourseAi::Completions::Llm.with_prepared_responses([explain_response]) do + post_ai_helper.select_helper_model(mode) + expect(post_ai_helper).to have_suggestion_value(explain_response) + end + end + + context "with footnotes enabled" do + before do + SiteSetting.enable_markdown_footnotes = true + SiteSetting.display_footnotes_inline = true + end + + it "allows adding the explanation as a footnote to the post" do + select_post_text(post) + post_ai_helper.click_ai_button + + DiscourseAi::Completions::Llm.with_prepared_responses([explain_response]) do + post_ai_helper.select_helper_model(mode) + + expect(post_ai_helper).to have_suggestion_value(explain_response) + + post_ai_helper.click_add_footnote + + expect(post_ai_helper).to have_no_post_ai_helper + + expect(post.reload.raw).to include( + "^[#{explain_response} (#{I18n.t("js.discourse_ai.ai_helper.post_options_menu.footnote_credits")})]", + ) + end end end end @@ -128,13 +168,11 @@ RSpec.describe "AI Post helper", type: :system, js: true do end it "pre-fills fast edit with proofread text" do - skip("Test is flaky in CI, possibly some timing issue?") if ENV["CI"] select_post_text(post_3) find(".quote-edit-label").click DiscourseAi::Completions::Llm.with_prepared_responses([proofread_response]) do find(".btn-ai-suggest-edit", visible: :all).click - wait_for { fast_editor.has_content?(proofread_response) } - expect(fast_editor).to have_content(proofread_response) + expect(page.find("#fast-edit-input")).to have_content(proofread_response) end end end diff --git a/spec/system/page_objects/components/ai_post_helper_menu.rb b/spec/system/page_objects/components/ai_post_helper_menu.rb index b14089f0..5a907604 100644 --- a/spec/system/page_objects/components/ai_post_helper_menu.rb +++ b/spec/system/page_objects/components/ai_post_helper_menu.rb @@ -9,7 +9,6 @@ module PageObjects SHARE_SELECTOR = ".quote-sharing" AI_HELPER_SELECTOR = ".ai-post-helper" - AI_HELPER_MOBILE_SELECTOR = ".ai-post-helper-menu-content" TRIGGER_SELECTOR = "#{AI_HELPER_SELECTOR}__trigger" OPTIONS_SELECTOR = ".ai-helper-options" LOADING_SELECTOR = ".ai-helper-context-menu__loading" @@ -28,6 +27,10 @@ module PageObjects find("#{OPTIONS_SELECTOR} .btn[data-name=\"#{mode}\"]").click end + def has_suggestion_value?(value) + page.has_css?("#{SUGGESTION_SELECTOR}__text", text: value) + end + def suggestion_value find("#{SUGGESTION_SELECTOR}__text").text end @@ -41,11 +44,11 @@ module PageObjects end def has_mobile_post_ai_helper? - page.has_css?(AI_HELPER_MOBILE_SELECTOR) + page.has_css?(".fk-d-menu-modal #{AI_HELPER_SELECTOR}") end def has_no_mobile_post_ai_helper? - page.has_no_css?(AI_HELPER_MOBILE_SELECTOR) + page.has_no_css?(".fk-d-menu-modal #{AI_HELPER_SELECTOR}") end def has_post_ai_helper? @@ -81,6 +84,10 @@ module PageObjects page.has_no_css?(QUOTE_SELECTOR) || page.has_no_css?(EDIT_SELECTOR) || page.has_no_css?(SHARE_SELECTOR) end + + def has_suggestions? + page.has_css?(SUGGESTION_SELECTOR) + end end end end