fix: add missing id generation call to image validator, fix tests for image validator

Author: harbournick

packages/super-editor/src/core/super-validator/validators/state/nodes/image/image-validator.test.js

@@ -1,71 +1,127 @@
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
-import { createImageNodeValidator } from './image-validator.js';
-import * as rules from './rules/image-rid.js';
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { ensureValidImageRID } from './rules/index.js';
 
-describe('createImageNodeValidator', () => {
-  const mockEditor = {};
-  const mockLogger = { debug: vi.fn() };
-  const mockTransaction = {};
+describe('ensureValidImageRID', () => {
+  let mockEditor;
+  let mockTransaction;
+  let mockLogger;
 
   beforeEach(() => {
-    vi.spyOn(rules, 'ensureValidImageRID').mockImplementation(() => ({
-      modified: false,
-      results: [],
-    }));
+    mockTransaction = {
+      setNodeMarkup: vi.fn(),
+    };
+
+    mockLogger = {
+      debug: vi.fn(),
+    };
+
+    mockEditor = {
+      converter: {
+        docxHelpers: {
+          findRelationshipIdFromTarget: vi.fn(),
+          insertNewRelationship: vi.fn(),
+        },
+      },
+    };
   });
 
-  afterEach(() => {
-    vi.restoreAllMocks();
-  });
+  it('does nothing when rId is already present', () => {
+    const images = [
+      {
+        node: { attrs: { rId: 'r1', src: 'image.png' } },
+        pos: 0,
+      },
+    ];
 
-  it('should define requiredElements with image node', () => {
-    const validator = createImageNodeValidator({ editor: mockEditor, logger: mockLogger });
-    expect(validator.requiredElements).toEqual({
-      nodes: ['image'],
-    });
+    const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
+
+    expect(result.modified).toBe(false);
+    expect(result.results).toHaveLength(0);
+    expect(mockTransaction.setNodeMarkup).not.toHaveBeenCalled();
   });
 
-  it('should call ensureValidImageRID with image array', () => {
-    const validator = createImageNodeValidator({ editor: mockEditor, logger: mockLogger });
+  it('reuses existing rId when found', () => {
+    mockEditor.converter.docxHelpers.findRelationshipIdFromTarget.mockReturnValue('existing-rId');
+
+    const images = [
+      {
+        node: { attrs: { src: 'image1.png' } },
+        pos: 5,
+      },
+    ];
 
-    const analysis = { image: [{ attrs: { rId: 'r1' } }] };
-    validator(mockTransaction, analysis);
+    const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
-    expect(rules.ensureValidImageRID).toHaveBeenCalledWith(analysis.image, mockEditor, mockTransaction, mockLogger);
+    expect(result.modified).toBe(true);
+    expect(result.results[0]).toContain('Added missing rId to image at pos 5');
+    expect(mockTransaction.setNodeMarkup).toHaveBeenCalledWith(5, undefined, {
+      src: 'image1.png',
+      rId: 'existing-rId',
+    });
+
+    expect(mockLogger.debug).toHaveBeenCalledWith('Reusing existing rId for image:', 'existing-rId', 'at pos:', 5);
   });
 
-  it('should return modified = false and empty results if rule returns no issues', () => {
-    const validator = createImageNodeValidator({ editor: mockEditor, logger: mockLogger });
+  it('creates new rId when not found', () => {
+    mockEditor.converter.docxHelpers.findRelationshipIdFromTarget.mockReturnValue(null);
+    mockEditor.converter.docxHelpers.insertNewRelationship.mockReturnValue('new-rId');
 
-    const result = validator(mockTransaction, { image: [] });
+    const images = [
+      {
+        node: { attrs: { src: 'new-image.png' } },
+        pos: 2,
+      },
+    ];
 
-    expect(result).toEqual({
-      modified: false,
-      results: [],
-    });
-  });
+    const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
+
+    expect(result.modified).toBe(true);
+    expect(result.results[0]).toBe('Added missing rId to image at pos 2');
 
-  it('should return correct modified and results from rule', () => {
-    rules.ensureValidImageRID.mockReturnValueOnce({
-      modified: true,
-      results: [{ message: 'Missing rId', nodePos: 5 }],
+    expect(mockTransaction.setNodeMarkup).toHaveBeenCalledWith(2, undefined, {
+      src: 'new-image.png',
+      rId: 'new-rId',
     });
 
-    const validator = createImageNodeValidator({ editor: mockEditor, logger: mockLogger });
+    expect(mockLogger.debug).toHaveBeenCalledWith(
+      'Creating new rId for image at pos:',
+      2,
+      'with src:',
+      'new-image.png',
+    );
+  });
 
-    const result = validator(mockTransaction, { image: [{ attrs: {} }] });
+  it('skips images with no src', () => {
+    const images = [
+      {
+        node: { attrs: {} },
+        pos: 3,
+      },
+    ];
 
-    expect(result).toEqual({
-      modified: true,
-      results: [{ message: 'Missing rId', nodePos: 5 }],
-    });
+    const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
+
+    expect(result.modified).toBe(false);
+    expect(result.results).toHaveLength(0);
+    expect(mockTransaction.setNodeMarkup).not.toHaveBeenCalled();
   });
 
-  it('should not fail if analysis.image is undefined', () => {
-    const validator = createImageNodeValidator({ editor: mockEditor, logger: mockLogger });
+  it('handles multiple images with mixed outcomes', () => {
+    mockEditor.converter.docxHelpers.findRelationshipIdFromTarget
+      .mockReturnValueOnce(null)
+      .mockReturnValueOnce('reused-rId');
+
+    mockEditor.converter.docxHelpers.insertNewRelationship.mockReturnValue('created-rId');
+
+    const images = [
+      { node: { attrs: { src: 'img-a.png' } }, pos: 0 },
+      { node: { attrs: { src: 'img-b.png' } }, pos: 10 },
+    ];
 
-    validator(mockTransaction, {}); // no image key
+    const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
-    expect(rules.ensureValidImageRID).toHaveBeenCalledWith([], mockEditor, mockTransaction, mockLogger);
+    expect(result.modified).toBe(true);
+    expect(result.results).toHaveLength(2);
+    expect(mockTransaction.setNodeMarkup).toHaveBeenCalledTimes(2);
   });
 });

packages/super-editor/src/core/super-validator/validators/state/nodes/image/rules/image-rid.js

@@ -27,7 +27,7 @@ export function ensureValidImageRID(images, editor, tr, logger) {
 
       // If we still don't have an rId, create a new relationship
       if (!newId) {
-        newId = editor.converter.docxHelpers.getNewRelationshipId(editor);
+        newId = editor.converter.docxHelpers.insertNewRelationship(src, 'image', editor);
         logger.debug('Creating new rId for image at pos:', pos, 'with src:', src);
       }
 

packages/super-editor/src/core/super-validator/validators/state/nodes/image/rules/image-rid.test.js

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
-import { ensureValidImageRID } from './index.js';
+import { ensureValidImageRID } from './image-rid.js';
 
 describe('ensureValidImageRID', () => {
   let mockEditor;
@@ -19,17 +19,17 @@ describe('ensureValidImageRID', () => {
       converter: {
         docxHelpers: {
           findRelationshipIdFromTarget: vi.fn(),
-          getNewRelationshipId: vi.fn(),
+          insertNewRelationship: vi.fn(),
         },
       },
     };
   });
 
-  it('does nothing if image has both rId and src', () => {
+  it('does nothing if image already has rId and src', () => {
     const images = [
       {
-        node: { attrs: { rId: 'rId1', src: 'image.png' } },
-        pos: 10,
+        node: { attrs: { rId: 'r123', src: 'image.png' } },
+        pos: 5,
       },
     ];
 
@@ -40,30 +40,22 @@ describe('ensureValidImageRID', () => {
     expect(mockTransaction.setNodeMarkup).not.toHaveBeenCalled();
   });
 
-  it('adds a new rId when missing and cannot find existing', () => {
-    mockEditor.converter.docxHelpers.findRelationshipIdFromTarget.mockReturnValue(null);
-    mockEditor.converter.docxHelpers.getNewRelationshipId.mockReturnValue('new-rId');
-
+  it('skips image nodes with no src', () => {
     const images = [
       {
-        node: { attrs: { src: 'image.png' } },
-        pos: 5,
+        node: { attrs: {} },
+        pos: 8,
       },
     ];
 
     const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
-    expect(result.modified).toBe(true);
-    expect(result.results).toEqual(['Added missing rId to image at pos 5']);
-    expect(mockTransaction.setNodeMarkup).toHaveBeenCalledWith(5, undefined, {
-      src: 'image.png',
-      rId: 'new-rId',
-    });
-
-    expect(mockLogger.debug).toHaveBeenCalledWith('Creating new rId for image at pos:', 5, 'with src:', 'image.png');
+    expect(result.modified).toBe(false);
+    expect(result.results).toHaveLength(0);
+    expect(mockTransaction.setNodeMarkup).not.toHaveBeenCalled();
   });
 
-  it('reuses existing rId when found via findRelationshipIdFromTarget', () => {
+  it('reuses existing rId if found via findRelationshipIdFromTarget', () => {
     mockEditor.converter.docxHelpers.findRelationshipIdFromTarget.mockReturnValue('existing-rId');
 
     const images = [
@@ -76,7 +68,7 @@ describe('ensureValidImageRID', () => {
     const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
     expect(result.modified).toBe(true);
-    expect(result.results[0]).toMatch(/Added missing rId/);
+    expect(result.results[0]).toBe('Added missing rId to image at pos 2');
     expect(mockTransaction.setNodeMarkup).toHaveBeenCalledWith(2, undefined, {
       src: 'img.jpg',
       rId: 'existing-rId',
@@ -85,37 +77,54 @@ describe('ensureValidImageRID', () => {
     expect(mockLogger.debug).toHaveBeenCalledWith('Reusing existing rId for image:', 'existing-rId', 'at pos:', 2);
   });
 
-  it('skips image nodes with no src', () => {
+  it('creates new rId when not found', () => {
+    mockEditor.converter.docxHelpers.findRelationshipIdFromTarget.mockReturnValue(null);
+    mockEditor.converter.docxHelpers.insertNewRelationship.mockReturnValue('new-rId');
+
     const images = [
       {
-        node: { attrs: {} },
-        pos: 8,
+        node: { attrs: { src: 'new-img.png' } },
+        pos: 3,
       },
     ];
 
     const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
-    expect(result.modified).toBe(false);
-    expect(result.results).toEqual([]);
-    expect(mockTransaction.setNodeMarkup).not.toHaveBeenCalled();
+    expect(result.modified).toBe(true);
+    expect(result.results[0]).toBe('Added missing rId to image at pos 3');
+    expect(mockTransaction.setNodeMarkup).toHaveBeenCalledWith(3, undefined, {
+      src: 'new-img.png',
+      rId: 'new-rId',
+    });
+
+    expect(mockLogger.debug).toHaveBeenCalledWith('Creating new rId for image at pos:', 3, 'with src:', 'new-img.png');
   });
 
-  it('handles multiple image nodes correctly', () => {
+  it('handles multiple images with mixed outcomes', () => {
     mockEditor.converter.docxHelpers.findRelationshipIdFromTarget
-      .mockReturnValueOnce(null)
-      .mockReturnValueOnce('found-rId');
+      .mockReturnValueOnce(null) // first image -> not found
+      .mockReturnValueOnce('reused-rId'); // second image -> found
 
-    mockEditor.converter.docxHelpers.getNewRelationshipId.mockReturnValue('new-rId');
+    mockEditor.converter.docxHelpers.insertNewRelationship.mockReturnValue('created-rId');
 
     const images = [
-      { node: { attrs: { src: 'one.png' } }, pos: 1 },
-      { node: { attrs: { src: 'two.png' } }, pos: 2 },
+      { node: { attrs: { src: 'a.png' } }, pos: 0 },
+      { node: { attrs: { src: 'b.png' } }, pos: 10 },
     ];
 
     const result = ensureValidImageRID(images, mockEditor, mockTransaction, mockLogger);
 
     expect(result.modified).toBe(true);
     expect(result.results).toHaveLength(2);
+
     expect(mockTransaction.setNodeMarkup).toHaveBeenCalledTimes(2);
+    expect(mockTransaction.setNodeMarkup).toHaveBeenNthCalledWith(1, 0, undefined, {
+      src: 'a.png',
+      rId: 'created-rId',
+    });
+    expect(mockTransaction.setNodeMarkup).toHaveBeenNthCalledWith(2, 10, undefined, {
+      src: 'b.png',
+      rId: 'reused-rId',
+    });
   });
 });