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.
This commit is contained in:
Keegan George 2024-12-05 04:41:34 +09:00 committed by GitHub
parent 938d4c018c
commit d6beac48f8
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 212 additions and 73 deletions

View File

@ -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}}
</div>

View File

@ -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"]

View File

@ -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"

View File

@ -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

View File

@ -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 =
"<p>La lluvia en España se queda principalmente en el avión.</p>";
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);
});
});

View File

@ -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: [],
},
];