Fix code quality issues from PR review

- Replace unsafe type assertion (as unknown as T) with getRemoteUrl() helper
- Validate HTML assets upfront before calling registerAssets
- Add comprehensive tests for URL support in action file inputs
- Remove unused import and fix linting issues

All 266 tests pass. Fixes all issues identified in PR #8 review.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Author: nickwinder

src/__tests__/unit/inputs.test.ts

@@ -1,4 +1,4 @@
-import { isRemoteFileInput, processFileInput, validateFileInput } from '../../inputs';
+import { getRemoteUrl, isRemoteFileInput, processFileInput, validateFileInput } from '../../inputs';
 import { ValidationError } from '../../errors';
 import { Readable } from 'stream';
 import fs from 'fs';
@@ -189,6 +189,42 @@ describe('Input Processing (Node.js only)', () => {
     });
   });
 
+  describe('getRemoteUrl', () => {
+    it('should extract URL from URL string', () => {
+      const url = 'https://example.com/test.pdf';
+      expect(getRemoteUrl(url)).toBe(url);
+    });
+
+    it('should return null for file path string', () => {
+      expect(getRemoteUrl('test.pdf')).toBeNull();
+    });
+
+    it('should extract URL from UrlInput object', () => {
+      const urlInput = { type: 'url' as const, url: 'https://example.com/test.pdf' };
+      expect(getRemoteUrl(urlInput)).toBe('https://example.com/test.pdf');
+    });
+
+    it('should return null for Buffer', () => {
+      expect(getRemoteUrl(Buffer.from('test'))).toBeNull();
+    });
+
+    it('should return null for Uint8Array', () => {
+      expect(getRemoteUrl(new Uint8Array([1, 2, 3]))).toBeNull();
+    });
+
+    it('should return null for FilePathInput', () => {
+      expect(getRemoteUrl({ type: 'file-path', path: 'test.pdf' })).toBeNull();
+    });
+
+    it('should return null for BufferInput', () => {
+      expect(getRemoteUrl({ type: 'buffer', buffer: Buffer.from('test'), filename: 'test.pdf' })).toBeNull();
+    });
+
+    it('should return null for Uint8ArrayInput', () => {
+      expect(getRemoteUrl({ type: 'uint8array', data: new Uint8Array([1, 2, 3]), filename: 'test.bin' })).toBeNull();
+    });
+  });
+
   describe('processFileInput - Invalid inputs', () => {
     it('should throw for URL', async () => {
       await expect(processFileInput('https://example.com/test.pdf')).rejects.toThrow(

src/__tests__/unit/workflow.test.ts

@@ -14,6 +14,12 @@ jest.mock('../../http');
 const mockValidateFileInput = inputsModule.validateFileInput as jest.MockedFunction<
   typeof inputsModule.validateFileInput
 >;
+const mockIsRemoteFileInput = inputsModule.isRemoteFileInput as jest.MockedFunction<
+  typeof inputsModule.isRemoteFileInput
+>;
+const mockGetRemoteUrl = inputsModule.getRemoteUrl as jest.MockedFunction<
+  typeof inputsModule.getRemoteUrl
+>;
 const mockSendRequest = httpModule.sendRequest as jest.MockedFunction<
   typeof httpModule.sendRequest
 >;
@@ -31,6 +37,8 @@ describe('WorkflowBuilder', () => {
     workflow = new WorkflowBuilder(mockClientOptions);
     // Default mocks
     mockValidateFileInput.mockReturnValue(true);
+    mockIsRemoteFileInput.mockReturnValue(false);
+    mockGetRemoteUrl.mockReturnValue(null); // Default: not a URL
     mockSendRequest.mockResolvedValue({
       data: new Blob(['mock response'], { type: 'application/pdf' }) as never,
       status: 200,
@@ -102,20 +110,16 @@ describe('WorkflowBuilder', () => {
       expect(result).toBe(workflow);
     });
 
-    it('should not call registerAssets when adding a file as a URL string', () => {
-      // Mock isRemoteFileInput to return true for URL string
-      jest.spyOn(inputsModule, 'isRemoteFileInput').mockReturnValueOnce(true);
-
-      // Create a spy on the registerAssets method
-      const registerAssetsSpy = jest.spyOn(workflow as never, 'registerAssets');
-
+    it('should handle URL string input without adding to assets map', () => {
       const urlString = 'https://example.com/document.pdf';
+      // Mock getRemoteUrl to return the URL (indicating it's a remote file)
+      mockGetRemoteUrl.mockReturnValueOnce(urlString);
+
       const result = workflow.addFilePart(urlString);
 
       expect(result).toBe(workflow);
-      expect(registerAssetsSpy).not.toHaveBeenCalled();
 
-      // Verify the file part was added with the URL
+      // Verify the file part was added with the URL (not stored in assets)
       expect(
         workflow['buildInstructions'].parts[workflow['buildInstructions'].parts.length - 1],
       ).toEqual(
@@ -124,21 +128,18 @@ describe('WorkflowBuilder', () => {
         }),
       );
 
-      registerAssetsSpy.mockRestore();
+      // Verify no assets were registered (URLs are passed directly)
+      expect(workflow['assets'].size).toBe(0);
     });
 
-    it('should not call registerAssets when adding a file as a URL object', () => {
-      // Mock isRemoteFileInput to return true for URL object
-      jest.spyOn(inputsModule, 'isRemoteFileInput').mockReturnValueOnce(true);
-
-      // Create a spy on the registerAssets method
-      const registerAssetsSpy = jest.spyOn(workflow as never, 'registerAssets');
-
+    it('should handle URL object input without adding to assets map', () => {
       const urlObject: UrlInput = { type: 'url', url: 'https://example.com/document.pdf' };
+      // Mock getRemoteUrl to return the URL (indicating it's a remote file)
+      mockGetRemoteUrl.mockReturnValueOnce(urlObject.url);
+
       const result = workflow.addFilePart(urlObject);
 
       expect(result).toBe(workflow);
-      expect(registerAssetsSpy).not.toHaveBeenCalled();
 
       // Verify the file part was added with the URL
       expect(
@@ -149,7 +150,8 @@ describe('WorkflowBuilder', () => {
         }),
       );
 
-      registerAssetsSpy.mockRestore();
+      // Verify no assets were registered (URLs are passed directly)
+      expect(workflow['assets'].size).toBe(0);
     });
   });
 
@@ -171,18 +173,14 @@ describe('WorkflowBuilder', () => {
       expect(result).toBe(workflow);
     });
 
-    it('should not call registerAssets when adding HTML as a URL string', () => {
-      // Mock isRemoteFileInput to return true for URL string
-      jest.spyOn(inputsModule, 'isRemoteFileInput').mockReturnValueOnce(true);
-
-      // Create a spy on the registerAssets method
-      const registerAssetsSpy = jest.spyOn(workflow as never, 'registerAssets');
-
+    it('should handle HTML URL string input without adding to assets map', () => {
       const urlString = 'https://example.com/page.html';
+      // Mock getRemoteUrl to return the URL (indicating it's a remote file)
+      mockGetRemoteUrl.mockReturnValueOnce(urlString);
+
       const result = workflow.addHtmlPart(urlString);
 
       expect(result).toBe(workflow);
-      expect(registerAssetsSpy).not.toHaveBeenCalled();
 
       // Verify the HTML part was added with the URL
       expect(
@@ -193,21 +191,18 @@ describe('WorkflowBuilder', () => {
         }),
       );
 
-      registerAssetsSpy.mockRestore();
+      // Verify no assets were registered (URLs are passed directly)
+      expect(workflow['assets'].size).toBe(0);
     });
 
-    it('should not call registerAssets when adding HTML as a URL object', () => {
-      // Mock isRemoteFileInput to return true for URL object
-      jest.spyOn(inputsModule, 'isRemoteFileInput').mockReturnValueOnce(true);
-
-      // Create a spy on the registerAssets method
-      const registerAssetsSpy = jest.spyOn(workflow as never, 'registerAssets');
-
+    it('should handle HTML URL object input without adding to assets map', () => {
       const urlObject: UrlInput = { type: 'url', url: 'https://example.com/page.html' };
+      // Mock getRemoteUrl to return the URL (indicating it's a remote file)
+      mockGetRemoteUrl.mockReturnValueOnce(urlObject.url);
+
       const result = workflow.addHtmlPart(urlObject);
 
       expect(result).toBe(workflow);
-      expect(registerAssetsSpy).not.toHaveBeenCalled();
 
       // Verify the HTML part was added with the URL
       expect(
@@ -218,7 +213,8 @@ describe('WorkflowBuilder', () => {
         }),
       );
 
-      registerAssetsSpy.mockRestore();
+      // Verify no assets were registered (URLs are passed directly)
+      expect(workflow['assets'].size).toBe(0);
     });
   });
 
@@ -274,6 +270,54 @@ describe('WorkflowBuilder', () => {
       expect(result).toBe(workflow);
       expect(mockValidateFileInput).toHaveBeenCalledWith(xfdfFile);
     });
+
+    it('should handle URL input for watermarkImage action', () => {
+      const urlInput: UrlInput = { type: 'url', url: 'https://example.com/watermark.png' };
+      const action = BuildActions.watermarkImage(urlInput);
+
+      // Mock getRemoteUrl: first call for test.pdf (returns null), second for URL action (returns URL)
+      mockGetRemoteUrl.mockReturnValueOnce(null).mockReturnValueOnce(urlInput.url);
+
+      workflow.addFilePart('test.pdf').applyAction(action);
+
+      // Verify the action's fileInput contains the URL
+      expect(action.fileInput).toEqual(urlInput);
+
+      // Verify only the test.pdf was registered as local asset (not the watermark URL)
+      expect(workflow['assets'].size).toBe(1);
+    });
+
+    it('should handle URL input for applyInstantJson action', () => {
+      const urlInput: UrlInput = { type: 'url', url: 'https://example.com/annotations.json' };
+      const action = BuildActions.applyInstantJson(urlInput);
+
+      // Mock getRemoteUrl: first call for test.pdf (returns null), second for URL action (returns URL)
+      mockGetRemoteUrl.mockReturnValueOnce(null).mockReturnValueOnce(urlInput.url);
+
+      workflow.addFilePart('test.pdf').applyAction(action);
+
+      // Verify the action's fileInput contains the URL
+      expect(action.fileInput).toEqual(urlInput);
+
+      // Verify only the test.pdf was registered as local asset (not the JSON URL)
+      expect(workflow['assets'].size).toBe(1);
+    });
+
+    it('should handle URL input for applyXfdf action', () => {
+      const urlInput: UrlInput = { type: 'url', url: 'https://example.com/annotations.xfdf' };
+      const action = BuildActions.applyXfdf(urlInput);
+
+      // Mock getRemoteUrl: first call for test.pdf (returns null), second for URL action (returns URL)
+      mockGetRemoteUrl.mockReturnValueOnce(null).mockReturnValueOnce(urlInput.url);
+
+      workflow.addFilePart('test.pdf').applyAction(action);
+
+      // Verify the action's fileInput contains the URL
+      expect(action.fileInput).toEqual(urlInput);
+
+      // Verify only the test.pdf was registered as local asset (not the XFDF URL)
+      expect(workflow['assets'].size).toBe(1);
+    });
   });
 
   describe('output methods', () => {

src/build.ts

@@ -1,16 +1,17 @@
 import type { components } from './generated/api-types';
-import type { FileInput } from './types';
+import type { FileInputWithUrl } from './types';
 
 const DEFAULT_DIMENSION = { value: 100, unit: '%' as const };
 
 /**
- * Internal action type that holds FileInput for deferred registration
+ * Internal action type that holds FileInput for deferred registration.
+ * Supports both local files and URLs.
  */
 export interface ActionWithFileInput<
   Action extends components['schemas']['BuildAction'] = components['schemas']['BuildAction'],
 > {
   __needsFileRegistration: true;
-  fileInput: FileInput;
+  fileInput: FileInputWithUrl;
   createAction: (fileHandle: components['schemas']['FileHandle']) => Action;
 }
 
@@ -79,7 +80,7 @@ export const BuildActions = {
 
   /**
    * Create an image watermark action
-   * @param image - Watermark image
+   * @param image - Watermark image (local file or URL)
    * @param options - Watermark options
    * @param options.width - Width dimension of the watermark (value and unit, e.g. {value: 100, unit: '%'})
    * @param options.height - Height dimension of the watermark (value and unit, e.g. {value: 100, unit: '%'})
@@ -91,7 +92,7 @@ export const BuildActions = {
    * @param options.opacity - Watermark opacity (0 is fully transparent, 1 is fully opaque)
    */
   watermarkImage(
-    image: FileInput,
+    image: FileInputWithUrl,
     options: Partial<Omit<components['schemas']['ImageWatermarkAction'], 'type' | 'image'>> = {
       width: DEFAULT_DIMENSION,
       height: DEFAULT_DIMENSION,
@@ -127,10 +128,10 @@ export const BuildActions = {
 
   /**
    * Create an apply Instant JSON action
-   * @param file - Instant JSON file input
+   * @param file - Instant JSON file input (local file or URL)
    */
   applyInstantJson(
-    file: FileInput,
+    file: FileInputWithUrl,
   ): ActionWithFileInput<components['schemas']['ApplyInstantJsonAction']> {
     return {
       __needsFileRegistration: true,
@@ -146,13 +147,13 @@ export const BuildActions = {
 
   /**
    * Create an apply XFDF action
-   * @param file - XFDF file input
+   * @param file - XFDF file input (local file or URL)
    * @param options - Apply Xfdf options
    * @param options.ignorePageRotation - If true, ignores page rotation when applying XFDF data (default: false)
    * @param options.richTextEnabled - If true, plain text annotations will be converted to rich text annotations. If false, all text annotations will be plain text annotations (default: true)
    */
   applyXfdf(
-    file: FileInput,
+    file: FileInputWithUrl,
     options?: Partial<Omit<components['schemas']['ApplyXfdfAction'], 'type' | 'file'>>,
   ): ActionWithFileInput<components['schemas']['ApplyXfdfAction']> {
     return {