DEV: Improvements to auto grid images (#29317)

This PR is a follow-up to ea1473e532. When we initially added the experimental feature for automatically adding `[grid]` to images, we add the [grid] surrounding images after all the uploads have been completed.

This can lead to confusion when `[grid]` is delayed to be added in the composer, as users may try to add grid manually leading to breakage. This also leads to issues with Discourse AI's automatic image caption feature.

**In this PR**: we simply move the logic to be added when the images are uploaded and processing. This way, `[grid]` surrounding images is added immediately. We also apply a fix for an edge-case to prevent images from being wrapped in `[grid]` when they are already inside `[grid]` tags.

app/assets/javascripts/discourse/app/lib/uppy/composer-upload.js

@@ -13,7 +13,6 @@ import {
   displayErrorForBulkUpload,
   displayErrorForUpload,
   getUploadMarkdown,
-  IMAGE_MARKDOWN_REGEX,
   isImage,
   validateUploadedFile,
 } from "discourse/lib/uploads";
@@ -256,6 +255,10 @@ export default class UppyComposerUpload {
         if (this.composerModel.privateMessage) {
           file.meta.for_private_message = true;
         }
+
+        if (isImage(file.name)) {
+          this.#consecutiveImages.push(file.name);
+        }
       });
     });
 
@@ -296,7 +299,6 @@ export default class UppyComposerUpload {
         if (this.isDestroying || this.isDestroyed) {
           return;
         }
-
         const upload = this.#inProgressUploads.find(
           (upl) => upl.id === file.id
         );
@@ -352,6 +354,14 @@ export default class UppyComposerUpload {
             file.name
           );
         });
+
+        const MIN_IMAGES_TO_AUTO_GRID = 3;
+        if (
+          this.siteSettings.experimental_auto_grid_images &&
+          this.#consecutiveImages?.length >= MIN_IMAGES_TO_AUTO_GRID
+        ) {
+          this.#autoGridImages();
+        }
       });
     });
 
@@ -367,11 +377,6 @@ export default class UppyComposerUpload {
           getUploadMarkdown(upload)
         );
 
-        // Track consecutive images for surrounding with [grid] later:
-        if (isImage(upload.url)) {
-          this.#consecutiveImages.push(markdown);
-        }
-
         cacheShortUploadUrl(upload.short_url, upload);
 
         new ComposerVideoThumbnailUppy(getOwner(this)).generateVideoThumbnail(
@@ -398,13 +403,6 @@ export default class UppyComposerUpload {
                 `${this.composerEventPrefix}:all-uploads-complete`
               );
 
-              const MIN_IMAGES_TO_AUTO_GRID = 3;
-              if (
-                this.siteSettings.experimental_auto_grid_images &&
-                this.#consecutiveImages?.length >= MIN_IMAGES_TO_AUTO_GRID
-              ) {
-                this.#autoGridImages();
-              }
               this.#displayBufferedErrors();
               this.#reset();
             }
@@ -735,20 +733,51 @@ export default class UppyComposerUpload {
   #autoGridImages() {
     const reply = this.composerModel.get("reply");
     const imagesToWrapGrid = new Set(this.#consecutiveImages);
-    const matches = reply.match(IMAGE_MARKDOWN_REGEX) || [];
 
+    const uploadingText = I18n.t("uploading_filename", {
+      filename: "%placeholder%",
+    });
+    const uploadingTextMatch = uploadingText.match(/^.*(?=: %placeholder%…)/);
+
+    if (!uploadingTextMatch || !uploadingTextMatch[0]) {
+      return;
+    }
+
+    const uploadingImagePattern = new RegExp(
+      "\\[" + uploadingTextMatch[0].trim() + ": ([^\\]]+?)\\.\\w+…\\]\\(\\)",
+      "g"
+    );
+
+    const matches = reply.match(uploadingImagePattern) || [];
     const foundImages = [];
-    matches.forEach((fullImageMarkdown) => {
-      fullImageMarkdown = fullImageMarkdown.trim();
 
-      // Check if the matched image markdown is in the imagesToWrapGrid
-      if (imagesToWrapGrid.has(fullImageMarkdown)) {
-        foundImages.push(fullImageMarkdown);
-        imagesToWrapGrid.delete(fullImageMarkdown);
+    const existingGridPattern = /\[grid\]([\s\S]*?)\[\/grid\]/g;
+    const gridMatches = reply.match(existingGridPattern);
 
-        // Check if we've found all the images
-        if (imagesToWrapGrid.size === 0) {
-          return;
+    matches.forEach((imagePlaceholder) => {
+      imagePlaceholder = imagePlaceholder.trim();
+
+      const filenamePattern = new RegExp(
+        "\\[" + uploadingTextMatch[0].trim() + ": ([^\\]]+?)\\…\\]\\(\\)"
+      );
+
+      const filenameMatch = imagePlaceholder.match(filenamePattern);
+
+      if (filenameMatch && filenameMatch[1]) {
+        const filename = filenameMatch[1];
+
+        const isWithinGrid = gridMatches?.some((gridContent) =>
+          gridContent.includes(imagePlaceholder)
+        );
+
+        if (!isWithinGrid && imagesToWrapGrid.has(filename)) {
+          foundImages.push(imagePlaceholder);
+          imagesToWrapGrid.delete(filename);
+
+          // Check if we've found all the images
+          if (imagesToWrapGrid.size === 0) {
+            return;
+          }
         }
       }
     });
@@ -778,6 +807,7 @@ export default class UppyComposerUpload {
         }
       }
     }
+
     // Clear found images for the next consecutive images:
     this.#consecutiveImages.length = 0;
     foundImages.length = 0;

spec/system/composer_uploads_spec.rb

@@ -244,5 +244,43 @@ describe "Uploading files in the composer", type: :system do
         %r{!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\)}m,
       )
     end
+
+    it "does not automatically wrap images in [grid] tags when uploading inside an existing [grid]" do
+      visit "/new-topic"
+      expect(composer).to be_opened
+
+      composer.fill_content("[grid]\n\n[/grid]")
+      composer.move_cursor_after("[grid]\n")
+
+      file_path_1 = file_from_fixtures("logo.png", "images").path
+      file_path_2 = file_from_fixtures("logo.jpg", "images").path
+      file_path_3 = file_from_fixtures("downsized.png", "images").path
+      attach_file([file_path_1, file_path_2, file_path_3]) do
+        composer.click_toolbar_button("upload")
+      end
+      expect(composer).to have_no_in_progress_uploads
+      expect(composer.composer_input.value).to match(
+        %r{\[grid\].*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*?\[/grid\]}m,
+      )
+    end
+
+    it "automatically wraps images in [grid] when site language is different" do
+      SiteSetting.default_locale = "es"
+
+      visit "/new-topic"
+      expect(composer).to be_opened
+
+      file_path_1 = file_from_fixtures("logo.png", "images").path
+      file_path_2 = file_from_fixtures("logo.jpg", "images").path
+      file_path_3 = file_from_fixtures("downsized.png", "images").path
+      attach_file([file_path_1, file_path_2, file_path_3]) do
+        composer.click_toolbar_button("upload")
+      end
+
+      expect(composer).to have_no_in_progress_uploads
+      expect(composer.composer_input.value).to match(
+        %r{\[grid\].*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*!\[.*?\]\(upload://.*?\).*?\[/grid\]}m,
+      )
+    end
   end
 end