refactor: streamline document identifier handling in Editor and SuperConverter

- Updated the Editor class to simplify document modification tracking and promote to GUID when necessary.
- Refactored methods for retrieving document identifiers, ensuring clarity and consistency in handling GUIDs and hashes.
- Deprecated outdated methods for backward compatibility while enhancing telemetry data retrieval.
- Adjusted related tests to accommodate asynchronous changes in document identifier generation.

Author: caio-pizzol

packages/super-editor/src/core/Editor.js

@@ -917,7 +917,7 @@ export class Editor extends EventEmitter {
     return SuperConverter.getDocumentGuid(doc);
   }
 
-  // Deprecated method for backward compatibility
+  // Deprecated
   /**
    * @deprecated use setDocumentVersion instead
    */
@@ -1361,8 +1361,14 @@ export class Editor extends EventEmitter {
       return;
     }
 
-    // Document modified - promote to GUID if needed
-    this.#ensureDocumentGuid();
+    // Track document modifications and promote to GUID if needed
+    if (transaction.docChanged && this.converter) {
+      if (!this.converter.documentGuid) {
+        this.converter.promoteToGuid();
+        console.debug('Document modified - assigned GUID:', this.converter.documentGuid);
+      }
+      this.converter.documentModified = true;
+    }
 
     this.emit('update', {
       editor: this,
@@ -1371,34 +1377,19 @@ export class Editor extends EventEmitter {
   }
 
   /**
-   * Ensure document has a permanent GUID if it's been modified
-   * @private
-   * @returns {void}
+   * Get document identifier for telemetry (async - may generate hash)
+   * @returns {Promise<string>} GUID for modified docs, hash for unmodified
    */
-  #ensureDocumentGuid() {
-    if (!this.converter) return;
-
-    // If we only have a hash, promote to GUID
-    if (this.converter.hasTemporaryId()) {
-      this.converter.promoteToGuid();
-      console.debug('Document modified - assigned permanent GUID');
-    }
+  async getDocumentIdentifier() {
+    return (await this.converter?.getDocumentIdentifier()) || null;
   }
 
   /**
-   * Get the document GUID
-   * @returns {string|null} The unique document GUID
+   * Get permanent document GUID (sync - only for modified documents)
+   * @returns {string|null} GUID or null if document hasn't been modified
    */
   getDocumentGuid() {
-    return this.converter?.getDocumentGuid() || null;
-  }
-
-  /**
-   * Get document identifier for telemetry (works for both modified and unmodified)
-   * @returns {string|null} GUID for modified docs, hash for unmodified
-   */
-  getDocumentIdentifier() {
-    return this.converter?.getDocumentIdentifier() || null;
+    return this.converter?.documentGuid || null;
   }
 
   /**
@@ -1410,18 +1401,23 @@ export class Editor extends EventEmitter {
   }
 
   /**
-   * Get telemetry data including document identifier
-   * @returns {Object} Telemetry data object
+   * Get telemetry data (async because of lazy hash generation)
    */
-  getTelemetryData() {
+  async getTelemetryData() {
     return {
-      documentId: this.getDocumentIdentifier(), // Hash or GUID
+      documentId: await this.getDocumentIdentifier(),
       isModified: this.isDocumentModified(),
-      isPermanentId: !this.converter?.hasTemporaryId(),
+      isPermanentId: !!this.converter?.documentGuid,
       version: this.converter?.getSuperdocVersion(),
     };
   }
 
+  // Deprecated for backward compatibility
+  getDocumentId() {
+    console.warn('getDocumentId is deprecated, use getDocumentGuid instead');
+    return this.getDocumentGuid();
+  }
+
   /**
    * Get attrs of the currently selected node or mark.
    * @param {String} nameOrType
@@ -1457,8 +1453,7 @@ export class Editor extends EventEmitter {
     return {
       ...json,
       metadata: {
-        documentId: this.getDocumentIdentifier(),
-        documentGuid: this.converter?.documentGuid || null, // Only if modified
+        documentGuid: this.converter?.documentGuid || null,
         isModified: this.isDocumentModified(),
         version: this.converter?.getSuperdocVersion() || null,
       },
@@ -1618,6 +1613,7 @@ export class Editor extends EventEmitter {
     const json = this.#prepareDocumentForExport(comments);
 
     // Export the document to DOCX
+    // GUID will be handled automatically in converter.exportToDocx if document was modified
     const documentXml = await this.converter.exportToDocx(
       json,
       this.schema,

packages/super-editor/src/core/super-converter/SuperConverter.js

@@ -132,10 +132,10 @@ class SuperConverter {
     this.fileSource = params?.fileSource || null;
     this.documentId = params?.documentId || null;
 
-    // Two-tier identification system
+    // Document identification
     this.documentGuid = null; // Permanent GUID for modified documents
     this.documentHash = null; // Temporary hash for unmodified documents
-    this.documentModified = false; // Track if document has been modified
+    this.documentModified = false; // Track if document has been edited
 
     // Parse the initial XML, if provided
     if (this.docx.length || this.xml) this.parseFromXml();
@@ -166,8 +166,8 @@ class SuperConverter {
     if (!this.initialJSON) this.initialJSON = this.parseXmlToJson(this.xml);
     this.declaration = this.initialJSON?.declaration;
 
-    // Resolve document identifier (GUID or hash)
-    this.resolveDocumentIdentifier();
+    // Only resolve existing GUIDs synchronously (no hash generation yet)
+    this.resolveDocumentGuid();
   }
 
   parseXmlToJson(xml) {
@@ -322,90 +322,96 @@ class SuperConverter {
   }
 
   /**
-   * Resolve document identifier - check for existing GUID or generate hash
+   * Resolve existing document GUID (synchronous)
    */
-  resolveDocumentIdentifier() {
-    // Only run this once during initialization
-    if (this.documentGuid || this.documentHash) return;
-
-    // 1. Check for Microsoft's docId (READ ONLY - never modify settings.xml)
-    try {
-      this.getDocumentInternalId(); // This sets this.documentInternalId
-      if (this.documentInternalId) {
-        this.documentGuid = this.documentInternalId.replace(/[{}]/g, '');
-        return;
-      }
-    } catch {
-      // Continue to next check
+  resolveDocumentGuid() {
+    // 1. Check Microsoft's docId (READ ONLY)
+    const microsoftGuid = this.getMicrosoftDocId();
+    if (microsoftGuid) {
+      this.documentGuid = microsoftGuid;
+      return;
     }
 
     // 2. Check our custom property
     const customGuid = SuperConverter.getStoredCustomProperty(this.docx, 'DocumentGuid');
     if (customGuid) {
       this.documentGuid = customGuid;
-      return;
     }
+    // Don't generate hash here - do it lazily when needed
+  }
 
-    // 3. Generate hash for unmodified document (telemetry only)
-    this.documentHash = this.generateDocumentHash();
+  /**
+   * Get Microsoft's docId from settings.xml (READ ONLY)
+   */
+  getMicrosoftDocId() {
+    this.getDocumentInternalId(); // Existing method
+    if (this.documentInternalId) {
+      return this.documentInternalId.replace(/[{}]/g, '');
+    }
+    return null;
   }
 
   /**
-   * Generate hash for unmodified documents
-   * @returns {string|null} Hash-based temporary identifier
+   * Generate document hash for telemetry (async, lazy)
    */
-  generateDocumentHash() {
-    if (!this.fileSource) return null;
+  async generateDocumentHash() {
+    if (!this.fileSource) return `HASH-${Date.now()}`;
 
     try {
       let buffer;
+
       if (Buffer.isBuffer(this.fileSource)) {
         buffer = this.fileSource;
       } else if (this.fileSource instanceof ArrayBuffer) {
         buffer = Buffer.from(this.fileSource);
+      } else if (this.fileSource instanceof Blob || this.fileSource instanceof File) {
+        const arrayBuffer = await this.fileSource.arrayBuffer();
+        buffer = Buffer.from(arrayBuffer);
       } else {
-        // For File or Blob objects
-        buffer = Buffer.from(this.fileSource);
+        return `HASH-${Date.now()}`;
       }
 
       const hash = crc32(buffer);
       return `HASH-${hash.toString('hex').toUpperCase()}`;
     } catch (e) {
       console.warn('Could not generate document hash:', e);
-      return `HASH-${Date.now()}`; // Fallback
+      return `HASH-${Date.now()}`;
     }
   }
 
   /**
-   * Get document identifier (GUID or hash)
-   * @returns {string|null} Either permanent GUID or temporary hash
+   * Get document identifier (GUID or hash) - async for lazy hash generation
    */
-  getDocumentIdentifier() {
-    return this.documentGuid || this.documentHash || null;
+  async getDocumentIdentifier() {
+    if (this.documentGuid) {
+      return this.documentGuid;
+    }
+
+    if (!this.documentHash && this.fileSource) {
+      this.documentHash = await this.generateDocumentHash();
+    }
+
+    return this.documentHash;
   }
 
   /**
-   * Check if this is a temporary identifier
-   * @returns {boolean}
+   * Check if using temporary ID
    */
   hasTemporaryId() {
-    const id = this.getDocumentIdentifier();
-    return id && id.startsWith('HASH-');
+    // Has temporary ID if no GUID but has hash (or could generate one)
+    return !this.documentGuid && !!(this.documentHash || this.fileSource);
   }
 
   /**
-   * Convert temporary hash to permanent GUID
-   * @returns {string} The new or existing GUID
+   * Promote from hash to GUID on first edit
    */
   promoteToGuid() {
     if (this.documentGuid) return this.documentGuid;
 
-    // Generate new GUID
-    this.documentGuid = uuidv4();
+    this.documentGuid = this.getMicrosoftDocId() || uuidv4();
     this.documentModified = true;
     this.documentHash = null; // Clear temporary hash
 
-    console.debug('Document promoted from hash to GUID:', this.documentGuid);
     return this.documentGuid;
   }
 
@@ -650,18 +656,17 @@ class SuperConverter {
     // Update the rels table
     this.#exportProcessNewRelationships([...params.relationships, ...commentsRels, ...headFootRels]);
 
-    // Only store GUID in custom.xml if document was modified
-    if (this.documentModified && this.documentGuid) {
-      // Store SuperDoc version (indicates document was edited)
-      SuperConverter.setStoredSuperdocVersion(this.convertedXml);
-
-      // Store document GUID in custom.xml (never modify settings.xml)
-      this.documentGuid = SuperConverter.setStoredCustomProperty(
-        this.convertedXml,
-        'DocumentGuid',
-        this.documentGuid,
-        true, // preserve existing
-      );
+    // Store SuperDoc version
+    SuperConverter.setStoredSuperdocVersion(this.convertedXml);
+
+    // Store document GUID if document was modified
+    if (this.documentModified || this.documentGuid) {
+      if (!this.documentGuid) {
+        this.documentGuid = this.getMicrosoftDocId() || uuidv4();
+      }
+
+      // Always store in custom.xml (never modify settings.xml)
+      SuperConverter.setStoredCustomProperty(this.convertedXml, 'DocumentGuid', this.documentGuid, true);
     }
 
     // Update the numbering.xml
@@ -921,6 +926,12 @@ class SuperConverter {
     this.addedMedia = processedData;
   }
 
+  // Deprecated methods for backward compatibility
+  static getStoredSuperdocId(docx) {
+    console.warn('getStoredSuperdocId is deprecated, use getDocumentGuid instead');
+    return SuperConverter.getDocumentGuid(docx);
+  }
+
   static updateDocumentVersion(docx, version) {
     console.warn('updateDocumentVersion is deprecated, use setStoredSuperdocVersion instead');
     return SuperConverter.setStoredSuperdocVersion(docx, version);

packages/super-editor/src/core/super-converter/SuperConverter.test.js

@@ -72,29 +72,33 @@ describe('SuperConverter Document GUID', () => {
       expect(converter.hasTemporaryId()).toBe(false);
     });
 
-    it('generates hash for unmodified document without GUID', () => {
+    it('generates hash for unmodified document without GUID', async () => {
       const fileSource = Buffer.from('test file content');
       const converter = new SuperConverter({
         docx: mockDocx,
         fileSource,
       });
 
-      const identifier = converter.getDocumentIdentifier();
+      // getDocumentIdentifier is now async
+      const identifier = await converter.getDocumentIdentifier();
       expect(identifier).toMatch(/^HASH-/);
       expect(converter.hasTemporaryId()).toBe(true);
       expect(converter.getDocumentGuid()).toBeNull();
     });
   });
 
   describe('GUID Promotion', () => {
-    it('promotes hash to GUID when document is modified', () => {
+    it('promotes hash to GUID when document is modified', async () => {
       const fileSource = Buffer.from('test file content');
       const converter = new SuperConverter({
         docx: mockDocx,
         fileSource,
       });
 
-      // Initially has hash
+      // Generate hash first (async)
+      await converter.getDocumentIdentifier();
+
+      // Now check if has temporary ID
       expect(converter.hasTemporaryId()).toBe(true);
 
       // Promote to GUID