From 8640d3c82d5a13e5b297f8b3358fbf4dcca96df1 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 15 Apr 2024 22:45:11 +1000
Subject: [PATCH] FIX: Wait for bookmark save before allowing menu button click
 (#26626)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a timing issue where, if a user (or the CI) was
on a slow network connection, clicking one of the bookmark
menu options would cause an error because we hadn't yet received
the response from the server after creating the bookmark.

It should be very smooth most of the times because (paraphrasing j.jaffeux):

a) Most likely when user clicks it’s already saved
b) If it’s not saved when user clicks, it should already be almost done so
   the perceived wait when click the reminder option should be rather short
---
 .../app/components/bookmark-menu.gjs          | 38 +++++++++++--------
 spec/system/bookmarks_spec.rb                 | 16 +++++++-
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/bookmark-menu.gjs b/app/assets/javascripts/discourse/app/components/bookmark-menu.gjs
index 07d1fd886da..a5f58e4276e 100644
--- a/app/assets/javascripts/discourse/app/components/bookmark-menu.gjs
+++ b/app/assets/javascripts/discourse/app/components/bookmark-menu.gjs
@@ -28,6 +28,7 @@ export default class BookmarkMenu extends Component {
   bookmarkManager = this.args.bookmarkManager;
   timezone = this.currentUser?.user_option?.timezone || moment.tz.guess();
   timeShortcuts = timeShortcuts(this.timezone);
+  bookmarkCreatePromise = null;
 
   @action
   setReminderShortcuts() {
@@ -79,22 +80,25 @@ export default class BookmarkMenu extends Component {
   }
 
   @action
-  async onBookmark() {
-    try {
-      await this.bookmarkManager.create();
-      // We show the menu with Edit/Delete options if the bokmark exists,
-      // so this "quicksave" will do nothing in that case.
-      // NOTE: Need a nicer way to handle this; otherwise as soon as you save
-      // a bookmark, it switches to the other Edit/Delete menu.
-      this.quicksaved = true;
-      this.toasts.success({
-        duration: 3000,
-        views: ["mobile"],
-        data: { message: I18n.t("bookmarks.bookmarked_success") },
+  onBookmark() {
+    this.bookmarkCreatePromise = this.bookmarkManager.create();
+    this.bookmarkCreatePromise
+      .then(() => {
+        // We show the menu with Edit/Delete options if the bokmark exists,
+        // so this "quicksave" will do nothing in that case.
+        // NOTE: Need a nicer way to handle this; otherwise as soon as you save
+        // a bookmark, it switches to the other Edit/Delete menu.
+        this.quicksaved = true;
+        this.toasts.success({
+          duration: 3000,
+          views: ["mobile"],
+          data: { message: I18n.t("bookmarks.bookmarked_success") },
+        });
+      })
+      .catch((error) => popupAjaxError(error))
+      .finally(() => {
+        this.bookmarkCreatePromise = null;
       });
-    } catch (error) {
-      popupAjaxError(error);
-    }
   }
 
   @action
@@ -140,6 +144,10 @@ export default class BookmarkMenu extends Component {
 
   @action
   async onChooseReminderOption(option) {
+    if (this.bookmarkCreatePromise) {
+      await this.bookmarkCreatePromise;
+    }
+
     if (option.id === TIME_SHORTCUT_TYPES.CUSTOM) {
       this._openBookmarkModal();
     } else {
diff --git a/spec/system/bookmarks_spec.rb b/spec/system/bookmarks_spec.rb
index 1f70d4bd81f..c71f8abbc13 100644
--- a/spec/system/bookmarks_spec.rb
+++ b/spec/system/bookmarks_spec.rb
@@ -7,6 +7,7 @@ describe "Bookmarking posts and topics", type: :system do
   fab!(:post_2) { Fabricate(:post, topic: topic, raw: "Some interesting post content") }
 
   let(:timezone) { "Australia/Brisbane" }
+  let(:cdp) { PageObjects::CDP.new }
   let(:topic_page) { PageObjects::Pages::Topic.new }
   let(:bookmark_modal) { PageObjects::Modals::Bookmark.new }
   let(:bookmark_menu) { PageObjects::Components::BookmarkMenu.new }
@@ -18,9 +19,11 @@ describe "Bookmarking posts and topics", type: :system do
 
   def visit_topic_and_open_bookmark_menu(post, expand_actions: true)
     topic_page.visit_topic(topic)
+    open_bookmark_menu(post, expand_actions: expand_actions)
+  end
 
+  def open_bookmark_menu(post, expand_actions: true)
     topic_page.expand_post_actions(post) if expand_actions
-
     topic_page.click_post_action_button(post, :bookmark)
   end
 
@@ -80,6 +83,17 @@ describe "Bookmarking posts and topics", type: :system do
     )
   end
 
+  it "opens the bookmark modal with the Custom... option only after the bookmark saves on slow connections" do
+    topic_page.visit_topic(topic)
+
+    cdp.with_slow_upload do
+      open_bookmark_menu(post)
+      bookmark_menu.click_menu_option("custom")
+    end
+
+    expect(bookmark_modal).to be_open
+  end
+
   describe "topic level bookmarks" do
     it "allows the topic to be bookmarked" do
       topic_page.visit_topic(topic)