Going back to emitter

app/client/components/Comm.ts

@@ -370,7 +370,7 @@ export class Comm extends dispose.Disposable implements GristServerAPI, DocListA
 
       // Another asynchronous message that's not a response. Broadcast it as an event.
       if (ValidEvent.guard(message.type)) {
-        log.debug("Comm: Triggering event " + message.type);
+        log.debug("Comm: Triggering event " + message.type, message.data);
         this.trigger(message.type, message);
       } else {
         log.warn("Comm: Server message of unknown type " + message.type);

app/client/components/GristDoc.ts

@@ -75,7 +75,7 @@ import {LocalPlugin} from "app/common/plugin";
 import {StringUnion} from 'app/common/StringUnion';
 import {TableData} from 'app/common/TableData';
 import {getGristConfig} from 'app/common/urlUtils';
-import {DocStateComparison} from 'app/common/UserAPI';
+import {AttachmentTransferStatus, DocStateComparison} from 'app/common/UserAPI';
 import {AttachedCustomWidgets, IAttachedCustomWidget, IWidgetType, WidgetType} from 'app/common/widgetTypes';
 import {CursorPos, UIRowId} from 'app/plugin/GristAPI';
 import {
@@ -207,6 +207,8 @@ export class GristDoc extends DisposableWithEvents {
 
   public isTimingOn = Observable.create(this, false);
 
+  public attachmentTransfer = Observable.create(this, null as AttachmentTransferStatus|null);
+
   /**
    * Checks if it is ok to show raw data popup for currently selected section.
    * We can't show raw data if:
@@ -896,6 +898,8 @@ export class GristDoc extends DisposableWithEvents {
       }
     } else if (message.data.timing) {
       this.isTimingOn.set(message.data.timing.status !== 'disabled');
+    } else if (message.data.attachmentTransfer) {
+      this.attachmentTransfer.set(message.data.attachmentTransfer);
     }
   }
 

app/client/ui/DocumentSettings.ts

@@ -31,7 +31,6 @@ import {not, propertyCompare} from 'app/common/gutil';
 import {getCurrency, locales} from 'app/common/Locales';
 import {isOwner, isOwnerOrEditor} from 'app/common/roles';
 import {
-  AttachmentTransferStatus,
   DOCTYPE_NORMAL,
   DOCTYPE_TEMPLATE,
   DOCTYPE_TUTORIAL,
@@ -239,15 +238,16 @@ export class DocSettingsPage extends Disposable {
       const id = use(this._docInfo.documentSettingsJson).attachmentStoreId;
       return id ? EXTERNAL : INTERNAL;
     });
-    storageType.onWrite(async (val) => {
-      await this._gristDoc.docApi.setAttachmentStore(val);
+    storageType.onWrite(async (type) => {
+      // We use this method, instead of updating the observable directly, to ensure that the
+      // active doc has a chance to send us updates about the transfer.
+      await this._gristDoc.docApi.setAttachmentStore(type);
     });
     const storageOptions = [{value: INTERNAL, label: 'Internal'}, {value: EXTERNAL, label: 'External'}];
 
-    const transferStatus = Observable.create<AttachmentTransferStatus|null>(this, null);
-    const locationSummary = Computed.create(this, use => use(transferStatus)?.locationSummary || null);
-
-    const inProgress = Computed.create(this, use => !!use(transferStatus)?.status.isRunning);
+    const transfer = this._gristDoc.attachmentTransfer;
+    const locationSummary = Computed.create(this, use => use(transfer)?.locationSummary);
+    const inProgress = Computed.create(this, use => !!use(transfer)?.status.isRunning);
     const allInCurrent = Computed.create(this, use => {
       const summary = use(locationSummary);
       const current = use(storageType);
@@ -265,10 +265,16 @@ export class DocSettingsPage extends Disposable {
       return currentInternal && (use(inProgress) || !use(allInCurrent));
     });
 
-    const refreshStatus = () => this._gristDoc.docApi.getAttachmentTransferStatus().then(s => {
-      if (this.isDisposed()) { return; }
-      transferStatus.set(s);
-    });
+    const loadStatus = async () => {
+      if (transfer.get()) {
+        return;
+      }
+      const status = await this._gristDoc.docApi.getAttachmentTransferStatus();
+      if (transfer.get()) {
+        return;
+      }
+      transfer.set(status);
+    };
 
     const checkAvailableStores = () => this._gristDoc.docApi.getAttachmentStores().then(s => {
       if (s.length === 0) {
@@ -280,30 +286,17 @@ export class DocSettingsPage extends Disposable {
 
     const beginTransfer = async () => {
       await this._gristDoc.docApi.transferAllAttachments();
-      await refreshStatus();
     };
 
     const attachmentsReady = Observable.create(this, false);
 
-    refreshStatus()
-      .then(checkAvailableStores)
-      .then(refreshStatus)
+    Promise.all([
+        loadStatus(),
+        checkAvailableStores(),
+      ])
       .then(() => attachmentsReady.set(true))
       .catch(reportError);
 
-    (async () => {
-      // Start polling for status updates every 1 seconds when transfer is in progress.
-      while(!this.isDisposed()) {
-        try {
-          await refreshStatus();
-          await new Promise(resolve => setTimeout(resolve, 200));
-        } catch (err) {
-          reportError(err);
-          break;
-        }
-      }
-    })().catch(reportError);
-
     return dom.create(AdminSection, t('Attachment storage'), [
       dom.create(AdminSectionItem, {
         id: 'preferredStorage',
@@ -321,17 +314,6 @@ export class DocSettingsPage extends Disposable {
           ]),
           dom.maybe(inProgress, () => [
             cssButton(
-              hoverTooltip(
-                dom('span',
-                  dom.domComputed(transferStatus, (status) => {
-                    return t(
-                      'There are {{count}} attachments left to transfer',
-                      {count: status?.status.pendingTransferCount}
-                    );
-                  }),
-                  testId('transfer-status-tooltip')
-                )
-              ),
               cssLoadingSpinner(
                 loadingSpinner.cls('-inline'),
                 cssLoadingSpinner.cls('-disabled'),

app/common/CommTypes.ts

@@ -4,6 +4,7 @@ import {FilteredDocUsageSummary} from 'app/common/DocUsage';
 import {Product} from 'app/common/Features';
 import {UserProfile} from 'app/common/LoginSessionAPI';
 import {StringUnion} from 'app/common/StringUnion';
+import {AttachmentTransferStatus} from 'app/common/UserAPI';
 
 export const ValidEvent = StringUnion(
   'docListAction', 'docUserAction', 'docShutdown', 'docError',
@@ -109,7 +110,8 @@ export interface CommDocChatter extends CommMessageBase {
     // about other users of the document.
     timing?: {
       status: 'active'|'disabled';
-    }
+    },
+    attachmentTransfer?: AttachmentTransferStatus;
   };
 }
 

app/common/UserAPI.ts

@@ -579,7 +579,6 @@ export interface DocAPI {
    * Sets the attachment storage used by the document.
    */
   setAttachmentStore(type: AttachmentStore): Promise<void>;
-
   /**
    * Lists available external attachment stores. For now it contains at most one store.
    * If there is one store available it means that external storage is configured and can be used by this document.

app/server/lib/ActiveDoc.ts

@@ -81,7 +81,7 @@ import {MetaRowRecord, SingleCell} from 'app/common/TableData';
 import {TelemetryEvent, TelemetryMetadataByLevel} from 'app/common/Telemetry';
 import {FetchUrlOptions, UploadResult} from 'app/common/uploads';
 import {Document as APIDocument, DocReplacementOptions,
-        DocState, DocStateComparison, NEW_DOCUMENT_CODE} from 'app/common/UserAPI';
+        DocState, DocStateComparison, NEW_DOCUMENT_CODE, AttachmentTransferStatus} from 'app/common/UserAPI';
 import {convertFromColumn} from 'app/common/ValueConverter';
 import {guessColInfo} from 'app/common/ValueGuesser';
 import {parseUserAction} from 'app/common/ValueParser';
@@ -400,6 +400,12 @@ export class ActiveDoc extends EventEmitter {
       _options?.doc,
     );
 
+    const notifier = this.sendAttachmentTransferStatusNotification.bind(this);
+
+    this._attachmentFileManager
+      .on(AttachmentFileManager.events.TRANSFER_STARTED, notifier)
+      .on(AttachmentFileManager.events.TRANSFER_COMPLETED, notifier);
+
     // Our DataEngine is a separate sandboxed process (one sandbox per open document,
     // corresponding to one process for pynbox, more for gvisor).
     // The data engine runs user-defined python code including formula calculations.
@@ -959,8 +965,11 @@ export class ActiveDoc extends EventEmitter {
   /**
    * Returns a summary of pending attachment transfers between attachment stores.
    */
-  public attachmentTransferStatus() {
-    return this._attachmentFileManager.transferStatus();
+  public async attachmentTransferStatus() {
+    return {
+      status: this._attachmentFileManager.transferStatus(),
+      locationSummary: await this._attachmentFileManager.locationSummary(),
+    };
   }
 
   /**
@@ -976,14 +985,15 @@ export class ActiveDoc extends EventEmitter {
    */
   @ActiveDoc.keepDocOpen
   public async allAttachmentTransfersCompleted() {
-      await this._attachmentFileManager.allTransfersCompleted();
+    await this._attachmentFileManager.allTransfersCompleted();
   }
 
 
   public async setAttachmentStore(docSession: OptDocSession, id: string | undefined): Promise<void> {
     const docSettings = await this._getDocumentSettings();
     docSettings.attachmentStoreId = id;
     await this._updateDocumentSettings(docSession, docSettings);
+    await this.sendAttachmentTransferStatusNotification(await this.attachmentTransferStatus());
   }
 
   /**
@@ -993,7 +1003,7 @@ export class ActiveDoc extends EventEmitter {
    */
   public async setAttachmentStoreFromLabel(docSession: OptDocSession, label: string | undefined): Promise<void> {
     const id = label === undefined ? undefined : this._attachmentStoreProvider?.getStoreIdFromLabel(label);
-    return this.setAttachmentStore(docSession, id);
+    await this.setAttachmentStore(docSession, id);
   }
 
   public async getAttachmentStore(): Promise<string | undefined> {
@@ -1934,11 +1944,9 @@ export class ActiveDoc extends EventEmitter {
     });
   }
 
-  public async sendAttachmentTransferStatusNotification() {
+  public async sendAttachmentTransferStatusNotification(attachmentTransfer: AttachmentTransferStatus) {
     await this.docClients.broadcastDocMessage(null, 'docChatter', {
-      attachmentTransfer: {
-        ...this.attachmentTransferStatus(),
-      },
+      attachmentTransfer
     });
   }
 

app/server/lib/AttachmentFileManager.ts

@@ -1,4 +1,4 @@
-import {DocAttachmentsLocation} from 'app/common/UserAPI';
+import {AttachmentTransferStatus, DocAttachmentsLocation} from 'app/common/UserAPI';
 import {
   AttachmentStoreDocInfo,
   DocPoolId,
@@ -11,7 +11,8 @@ import {DocStorage} from 'app/server/lib/DocStorage';
 import log from 'app/server/lib/log';
 import {LogMethods} from 'app/server/lib/LogMethods';
 import {MemoryWritableStream} from 'app/server/utils/MemoryWritableStream';
-import {Readable} from 'node:stream';
+import {EventEmitter} from 'events';
+import {Readable} from 'stream';
 
 export interface AddFileResult {
   fileIdent: string;
@@ -76,7 +77,13 @@ export class AttachmentRetrievalError extends Error {
  * they'll eventually be cleaned up when the document pool is deleted.
  *
  */
-export class AttachmentFileManager {
+export class AttachmentFileManager extends EventEmitter {
+
+  public static events = {
+    TRANSFER_STARTED: 'transfer-started',
+    TRANSFER_COMPLETED: 'transfer-completed',
+  };
+
   // _docPoolId is a critical point for security. Documents with a common pool id can access each others' attachments.
   private readonly _docPoolId: DocPoolId | null;
   private readonly _docName: string;
@@ -102,6 +109,7 @@ export class AttachmentFileManager {
     private _storeProvider: IAttachmentStoreProvider | undefined,
     _docInfo: AttachmentStoreDocInfo | undefined,
   ) {
+    super();
     this._docName = _docStorage.docName;
     this._docPoolId = _docInfo ? getDocPoolIdFromDocInfo(_docInfo) : null;
   }
@@ -240,6 +248,7 @@ export class AttachmentFileManager {
 
   private async _performPendingTransfers() {
     try {
+      await this._notifyAboutStart();
       while (this._pendingFileTransfers.size > 0) {
         // Map.entries() will always return the most recent key/value from the map, even after a long async delay
         // Meaning we can safely iterate here and know the transfer is up to date.
@@ -262,9 +271,24 @@ export class AttachmentFileManager {
       }
     } finally {
       await this._docStorage.requestVacuum();
+      await this._notifyAboutEnd();
     }
   }
 
+  private async _notifyAboutStart() {
+    this.emit(AttachmentFileManager.events.TRANSFER_STARTED, {
+      locationSummary: await this.locationSummary(),
+      status: {pendingTransferCount: this._pendingFileTransfers.size, isRunning: true}
+    } as AttachmentTransferStatus);
+  }
+
+  private async _notifyAboutEnd() {
+    this.emit(AttachmentFileManager.events.TRANSFER_COMPLETED, {
+      locationSummary: await this.locationSummary(),
+      status: {pendingTransferCount: this._pendingFileTransfers.size, isRunning: false}
+    } as AttachmentTransferStatus);
+  }
+
   private async _addFileToLocalStorage(
     fileIdent: string,
     fileData: Buffer

app/server/lib/AttachmentStoreProvider.ts

@@ -75,7 +75,7 @@ export class AttachmentStoreProvider implements IAttachmentStoreProvider {
     });
 
     const storeIds = Array.from(this._storeDetailsById.keys());
-    log.info(`AttachmentStoreProvider initialised with stores: ${storeIds}`);
+    log.info(`AttachmentStoreProvider initialized with stores: ${storeIds}`);
   }
 
   public getStoreIdFromLabel(label: string): string {
@@ -150,13 +150,13 @@ export async function makeTempFilesystemStoreSpec(
 }
 
 const settings = appSettings.section("attachmentStores");
-const ATTACHMENT_STORE_MODE = settings.flag("mode").readString({
+const GRIST_EXTERNAL_ATTACHMENTS_MODE = settings.flag("mode").readString({
   envVar: "GRIST_EXTERNAL_ATTACHMENTS_MODE",
   defaultValue: "none",
 });
 
 export function getConfiguredStandardAttachmentStore(): string | undefined {
-  switch (ATTACHMENT_STORE_MODE) {
+  switch (GRIST_EXTERNAL_ATTACHMENTS_MODE) {
     case 'snapshots':
       return 'snapshots';
     case 'test':
@@ -167,7 +167,7 @@ export function getConfiguredStandardAttachmentStore(): string | undefined {
 }
 
 export async function getConfiguredAttachmentStoreConfigs(): Promise<IAttachmentStoreConfig[]> {
-  if (ATTACHMENT_STORE_MODE === 'snapshots') {
+  if (GRIST_EXTERNAL_ATTACHMENTS_MODE === 'snapshots') {
     const snapshotProvider = create.getAttachmentStoreOptions().snapshots;
     // This shouldn't happen - it could only happen if a version of Grist removes the snapshot provider from ICreate.
     if (snapshotProvider === undefined) {
@@ -182,7 +182,7 @@ export async function getConfiguredAttachmentStoreConfigs(): Promise<IAttachment
     }];
   }
   // TODO This mode should be removed once stores can be configured fully via env vars.
-  if(ATTACHMENT_STORE_MODE === 'test') {
+  if(GRIST_EXTERNAL_ATTACHMENTS_MODE === 'test') {
     return [{
       label: 'test-filesystem',
       spec: await makeTempFilesystemStoreSpec(),

app/server/lib/DocApi.ts

@@ -525,22 +525,13 @@ export class DocWorkerApi {
     // Starts transferring all attachments to the named store, if it exists.
     this._app.post('/api/docs/:docId/attachments/transferAll', isOwner, withDoc(async (activeDoc, req, res) => {
       await activeDoc.startTransferringAllAttachmentsToDefaultStore();
-      const locationSummary = await activeDoc.attachmentLocationSummary();
-
       // Respond with the current status to allow for immediate UI updates.
-      res.json({
-        status: activeDoc.attachmentTransferStatus(),
-        locationSummary,
-      });
+      res.json(await activeDoc.attachmentTransferStatus());
     }));
 
     // Returns the status of any current / pending attachment transfers
     this._app.get('/api/docs/:docId/attachments/transferStatus', isOwner, withDoc(async (activeDoc, req, res) => {
-      const locationSummary = await activeDoc.attachmentLocationSummary();
-      res.json({
-        status: activeDoc.attachmentTransferStatus(),
-        locationSummary,
-      });
+      res.json(await activeDoc.attachmentTransferStatus());
     }));
 
     this._app.get('/api/docs/:docId/attachments/store', isOwner,