Merge branch 'main' into GH-1543

webapp/src/components/gallery/__snapshots__/galleryCard.test.tsx.snap

@@ -38,14 +38,14 @@ exports[`src/components/gallery/GalleryCard with a comment content should match
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
       <div
         class="Menu noselect left"
       >
@@ -177,14 +177,14 @@ exports[`src/components/gallery/GalleryCard with an image content should match s
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
       <div
         class="Menu noselect left"
       >
@@ -352,14 +352,14 @@ exports[`src/components/gallery/GalleryCard with many contents should match snap
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
       <div
         class="Menu noselect left"
       >
@@ -495,14 +495,14 @@ exports[`src/components/gallery/GalleryCard with many images content should matc
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
       <div
         class="Menu noselect left"
       >
@@ -640,14 +640,14 @@ exports[`src/components/gallery/GalleryCard without block content return Gallery
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
     </div>
     <div
       class="gallery-item"
@@ -680,14 +680,14 @@ exports[`src/components/gallery/GalleryCard without block content return Gallery
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
     </div>
     <div
       class="gallery-item"
@@ -720,14 +720,14 @@ exports[`src/components/gallery/GalleryCard without block content return Gallery
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
     </div>
     <div
       class="gallery-item"
@@ -760,14 +760,14 @@ exports[`src/components/gallery/GalleryCard without block content return Gallery
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
     </div>
     <div
       class="gallery-item"
@@ -800,14 +800,14 @@ exports[`src/components/gallery/GalleryCard without block content should match s
       class="MenuWrapper optionsMenu"
       role="button"
     >
-      <div
+      <button
         class="Button IconButton"
-        role="button"
+        type="button"
       >
         <i
           class="CompassIcon icon-dots-horizontal OptionsIcon"
         />
-      </div>
+      </button>
       <div
         class="Menu noselect left"
       >

webapp/src/undoManager.test.ts

@@ -47,6 +47,59 @@ test('Basic undo/redo', async () => {
     expect(undoManager.redoDescription).toBe(undefined)
 })
 
+test('Basic undo/redo response dependant', async () => {
+    expect(undoManager.canUndo).toBe(false)
+    expect(undoManager.canRedo).toBe(false)
+    expect(undoManager.currentCheckpoint).toBe(0)
+
+    const blockIds = [2, 1]
+    const blocks: Record<string, any> = {}
+
+    const newBlock = await undoManager.perform(
+        async () => {
+            const responseId = blockIds.pop() // every time we run the action a new ID is obtained
+            const block: Record<string, any> = {id: responseId, title: 'Sample'}
+            blocks[block.id] = block
+            return block
+        },
+        async (block: Record<string, any>) => {
+            delete blocks[block.id]
+        },
+        'test',
+    )
+
+    // should insert the block and return the new block for its use
+    expect(undoManager.canUndo).toBe(true)
+    expect(undoManager.canRedo).toBe(false)
+    expect(blocks).toHaveProperty('1')
+    expect(blocks[1]).toEqual(newBlock)
+
+    // should correctly remove the block based on the info gathered in
+    // the redo function
+    await undoManager.undo()
+    expect(undoManager.canUndo).toBe(false)
+    expect(undoManager.canRedo).toBe(true)
+    expect(blocks).not.toHaveProperty('1')
+
+    // when redoing, as the function has side effects the new id will
+    // be different
+    await undoManager.redo()
+    expect(undoManager.canUndo).toBe(true)
+    expect(undoManager.canRedo).toBe(false)
+    expect(blocks).not.toHaveProperty('1')
+    expect(blocks).toHaveProperty('2')
+    expect(blocks[2].id).toEqual(2)
+
+    // when undoing, the undo manager has saved the new id internally
+    // and it removes the right block
+    await undoManager.undo()
+    expect(undoManager.canUndo).toBe(false)
+    expect(undoManager.canRedo).toBe(true)
+    expect(blocks).not.toHaveProperty('2')
+
+    await undoManager.clear()
+})
+
 test('Grouped undo/redo', async () => {
     expect(undoManager.canUndo).toBe(false)
     expect(undoManager.canRedo).toBe(false)

webapp/src/undomanager.ts

@@ -2,10 +2,11 @@
 // See LICENSE.txt for license information.
 interface UndoCommand {
     checkpoint: number
-    undo: () => Promise<void>
+    undo: (value?: any) => Promise<void>
     redo: () => Promise<void>
     description?: string
     groupId?: string
+    value?: any
 }
 
 //
@@ -50,30 +51,36 @@ class UndoManager {
         }
         this.isExecuting = true
 
-        await command[action]()
+        if (action === 'redo') {
+            command.value = await command[action]()
+        } else {
+            await command[action](command.value)
+        }
 
         this.isExecuting = false
         return this
     }
 
     async perform(
-        redo: () => Promise<void>,
+        redo: () => Promise<any>,
-        undo: () => Promise<void>,
+        undo: (value?: any) => Promise<void>,
         description?: string,
         groupId?: string,
         isDiscardable = false,
-    ): Promise<UndoManager> {
+    ): Promise<any> {
-        await redo()
+        const value = await redo()
-        return this.registerUndo({undo, redo}, description, groupId, isDiscardable)
+        this.registerUndo({undo, redo}, description, groupId, value, isDiscardable)
+        return value
     }
 
     registerUndo(
         command: {
-            undo: () => Promise<void>,
+            undo: (value?: any) => Promise<void>,
             redo: () => Promise<void>
         },
         description?: string,
         groupId?: string,
+        value?: any,
         isDiscardable = false,
     ): UndoManager {
         if (this.isExecuting) {
@@ -97,6 +104,7 @@ class UndoManager {
             redo: command.redo,
             description,
             groupId,
+            value,
         }
         this.commands.push(internalCommand)
 

webapp/src/wsclient.ts

@@ -34,10 +34,10 @@ export const ACTION_UPDATE_CLIENT_CONFIG = 'UPDATE_CLIENT_CONFIG'
 export interface MMWebSocketClient {
     conn: WebSocket | null;
     sendMessage(action: string, data: any, responseCallback?: () => void): void /* eslint-disable-line @typescript-eslint/no-explicit-any */
+    setFirstConnectCallback(callback: () => void): void
     setReconnectCallback(callback: () => void): void
     setErrorCallback(callback: (event: Event) => void): void
     setCloseCallback(callback: (connectFailCount: number) => void): void
-    connectionId: string
 }
 
 type OnChangeHandler = (client: WSClient, blocks: Block[]) => void
@@ -57,8 +57,6 @@ class WSClient {
     onChange: OnChangeHandler[] = []
     onError: OnErrorHandler[] = []
     onConfigChange: OnConfigChangeHandler[] = []
-    private mmWSMaxRetries = 100
-    private mmWSRetryDelay = 300
     private notificationDelay = 100
     private reopenDelay = 3000
     private updatedBlocks: Block[] = []
@@ -163,10 +161,19 @@ class WSClient {
     open(): void {
         if (this.client !== null) {
             // configure the Mattermost websocket client callbacks
+            const onConnect = () => {
+                Utils.log('WSClient in plugin mode, reusing Mattermost WS connection')
+
+                for (const handler of this.onStateChange) {
+                    handler(this, 'open')
+                }
+                this.state = 'open'
+            }
+
             const onReconnect = () => {
                 Utils.logWarn('WSClient reconnected')
 
-                this.open()
+                onConnect()
                 for (const handler of this.onReconnect) {
                     handler(this)
                 }
@@ -204,34 +211,11 @@ class WSClient {
                 }
             }
 
+            this.client.setFirstConnectCallback(onConnect)
             this.client.setErrorCallback(onError)
             this.client.setCloseCallback(onClose)
             this.client.setReconnectCallback(onReconnect)
 
-            // WSClient needs to ensure that the Mattermost client has
-            // correctly stablished the connection before opening
-            let retries = 0
-            const setPluginOpen = () => {
-                if (this.client?.connectionId !== '') {
-                    Utils.log('WSClient in plugin mode, reusing Mattermost WS connection')
-
-                    for (const handler of this.onStateChange) {
-                        handler(this, 'open')
-                    }
-                    this.state = 'open'
-                    return
-                }
-
-                retries++
-                if (retries <= this.mmWSMaxRetries) {
-                    Utils.log('WSClient Mattermost Websocket not ready, retrying')
-                    setTimeout(setPluginOpen, this.mmWSRetryDelay)
-                } else {
-                    Utils.logError('WSClient error on open: Mattermost Websocket client is not ready')
-                }
-            }
-
-            setPluginOpen()
             return
         }