refactor(cli): finish withClient migration — comment + tests

bin/confluence.js

@@ -38,16 +38,21 @@ function assertNoBodyForFolder(type, options) {
   }
 }
 
-function handleCommandError(analytics, commandName, error) {
+function handleCommandError(analytics, commandName, error, onExtra = null) {
   analytics.track(commandName, false);
   console.error(chalk.red('Error:'), error.message);
+  if (onExtra) {
+    try { onExtra(error); } catch { /* keep error path robust if hint code throws */ }
+  }
   process.exit(1);
 }
 
 // Wraps a command action with the standard analytics + client + error pipeline.
 // The handler still calls analytics.track(name, true) on success so it can opt
 // into alternative tracking keys (e.g. *_cancel, *_dry_run).
-function withClient(commandName, handler, { writable = false } = {}) {
+// `onError(error, ...actionArgs)` runs between the "Error:" log line and
+// process.exit, for commands that need to print extra diagnostics.
+function withClient(commandName, handler, { writable = false, onError = null } = {}) {
   return async (...actionArgs) => {
     const analytics = new Analytics();
     try {
@@ -56,7 +61,8 @@ function withClient(commandName, handler, { writable = false } = {}) {
       const client = new ConfluenceClient(config);
       await handler({ client, config, analytics }, ...actionArgs);
     } catch (error) {
-      handleCommandError(analytics, commandName, error);
+      const extra = onError ? (err) => onError(err, ...actionArgs) : null;
+      handleCommandError(analytics, commandName, error, extra);
     }
   };
 }
@@ -1072,96 +1078,89 @@ program
   .option('--inline-original-selection <text>', 'Original inline selection text')
   .option('--inline-marker-ref <ref>', 'Inline marker reference (optional)')
   .option('--inline-properties <json>', 'Inline properties JSON (advanced)')
-  .action(async (pageId, options) => {
-    const analytics = new Analytics();
-    let location = null;
-    try {
-      const config = getConfig(getProfileName());
-      assertWritable(config);
-      const client = new ConfluenceClient(config);
-
-      let content = '';
+  .action(withClient('comment_create', async ({ client, analytics }, pageId, options) => {
+    let content = '';
 
-      if (options.file) {
-        if (!fs.existsSync(options.file)) {
-          throw new Error(`File not found: ${options.file}`);
-        }
-        content = fs.readFileSync(options.file, 'utf8');
-      } else if (options.content) {
-        content = options.content;
-      } else {
-        throw new Error('Either --file or --content option is required');
+    if (options.file) {
+      if (!fs.existsSync(options.file)) {
+        throw new Error(`File not found: ${options.file}`);
       }
+      content = fs.readFileSync(options.file, 'utf8');
+    } else if (options.content) {
+      content = options.content;
+    } else {
+      throw new Error('Either --file or --content option is required');
+    }
 
-      location = (options.location || 'footer').toLowerCase();
-      if (!['inline', 'footer'].includes(location)) {
-        throw new Error('Location must be either "inline" or "footer".');
-      }
+    const location = (options.location || 'footer').toLowerCase();
+    if (!['inline', 'footer'].includes(location)) {
+      throw new Error('Location must be either "inline" or "footer".');
+    }
 
-      let inlineProperties = {};
-      if (options.inlineProperties) {
-        try {
-          const parsed = JSON.parse(options.inlineProperties);
-          if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
-            throw new Error('Inline properties must be a JSON object.');
-          }
-          inlineProperties = { ...parsed };
-        } catch (error) {
-          throw new Error(`Invalid --inline-properties JSON: ${error.message}`);
+    let inlineProperties = {};
+    if (options.inlineProperties) {
+      try {
+        const parsed = JSON.parse(options.inlineProperties);
+        if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
+          throw new Error('Inline properties must be a JSON object.');
         }
+        inlineProperties = { ...parsed };
+      } catch (error) {
+        throw new Error(`Invalid --inline-properties JSON: ${error.message}`);
       }
+    }
 
-      if (options.inlineSelection) {
-        inlineProperties.selection = options.inlineSelection;
-      }
-      if (options.inlineOriginalSelection) {
-        inlineProperties.originalSelection = options.inlineOriginalSelection;
-      }
-      if (options.inlineMarkerRef) {
-        inlineProperties.markerRef = options.inlineMarkerRef;
-      }
+    if (options.inlineSelection) {
+      inlineProperties.selection = options.inlineSelection;
+    }
+    if (options.inlineOriginalSelection) {
+      inlineProperties.originalSelection = options.inlineOriginalSelection;
+    }
+    if (options.inlineMarkerRef) {
+      inlineProperties.markerRef = options.inlineMarkerRef;
+    }
 
-      if (Object.keys(inlineProperties).length > 0 && location !== 'inline') {
-        throw new Error('Inline properties can only be used with --location inline.');
-      }
+    if (Object.keys(inlineProperties).length > 0 && location !== 'inline') {
+      throw new Error('Inline properties can only be used with --location inline.');
+    }
 
-      const parentId = options.parent;
+    const parentId = options.parent;
 
-      if (location === 'inline') {
-        const hasSelection = inlineProperties.selection || inlineProperties.originalSelection;
-        if (!hasSelection && !parentId) {
-          throw new Error('Inline comments require --inline-selection or --inline-original-selection when starting a new inline thread.');
+    if (location === 'inline') {
+      const hasSelection = inlineProperties.selection || inlineProperties.originalSelection;
+      if (!hasSelection && !parentId) {
+        throw new Error('Inline comments require --inline-selection or --inline-original-selection when starting a new inline thread.');
+      }
+      if (hasSelection) {
+        if (!inlineProperties.originalSelection && inlineProperties.selection) {
+          inlineProperties.originalSelection = inlineProperties.selection;
         }
-        if (hasSelection) {
-          if (!inlineProperties.originalSelection && inlineProperties.selection) {
-            inlineProperties.originalSelection = inlineProperties.selection;
-          }
-          if (!inlineProperties.markerRef) {
-            inlineProperties.markerRef = `comment-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
-          }
+        if (!inlineProperties.markerRef) {
+          inlineProperties.markerRef = `comment-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
         }
       }
+    }
 
-      const result = await client.createComment(pageId, content, options.format, {
-        parentId,
-        location,
-        inlineProperties: location === 'inline' ? inlineProperties : null
-      });
+    const result = await client.createComment(pageId, content, options.format, {
+      parentId,
+      location,
+      inlineProperties: location === 'inline' ? inlineProperties : null
+    });
 
-      console.log(chalk.green('✅ Comment created successfully!'));
-      console.log(`ID: ${chalk.blue(result.id)}`);
-      if (result.container?.id) {
-        console.log(`Page ID: ${chalk.blue(result.container.id)}`);
-      }
-      if (result._links?.webui) {
-        const url = client.toAbsoluteUrl(result._links.webui);
-        console.log(`URL: ${chalk.gray(url)}`);
-      }
+    console.log(chalk.green('✅ Comment created successfully!'));
+    console.log(`ID: ${chalk.blue(result.id)}`);
+    if (result.container?.id) {
+      console.log(`Page ID: ${chalk.blue(result.container.id)}`);
+    }
+    if (result._links?.webui) {
+      const url = client.toAbsoluteUrl(result._links.webui);
+      console.log(`URL: ${chalk.gray(url)}`);
+    }
 
-      analytics.track('comment_create', true);
-    } catch (error) {
-      analytics.track('comment_create', false);
-      console.error(chalk.red('Error:'), error.message);
+    analytics.track('comment_create', true);
+  }, {
+    writable: true,
+    onError: (error, _pageId, options) => {
       if (error.response?.data) {
         const detail = typeof error.response.data === 'string'
           ? error.response.data
@@ -1174,13 +1173,13 @@ program
         .filter(Boolean);
       const needsInlineMeta = ['matchIndex', 'lastFetchTime', 'serializedHighlights']
         .every((key) => errorKeys.includes(key));
+      const location = (options?.location || 'footer').toLowerCase();
       if (location === 'inline' && needsInlineMeta) {
         console.error(chalk.yellow('Inline comment creation requires editor highlight metadata (matchIndex, lastFetchTime, serializedHighlights).'));
         console.error(chalk.yellow('Try replying to an existing inline comment or use footer comments instead.'));
       }
-      process.exit(1);
     }
-  });
+  }));
 
 // Comment delete command
 program

tests/with-client.test.js

@@ -0,0 +1,133 @@
+jest.mock('../lib/config', () => ({
+  getConfig: jest.fn(),
+  initConfig: jest.fn(),
+  listProfiles: jest.fn(),
+  setActiveProfile: jest.fn(),
+  deleteProfile: jest.fn(),
+  isValidProfileName: jest.fn(),
+}));
+
+jest.mock('../lib/confluence-client', () => {
+  const ClientMock = jest.fn();
+  ClientMock.createLocalConverter = jest.fn();
+  return ClientMock;
+});
+
+const { getConfig } = require('../lib/config');
+const ConfluenceClient = require('../lib/confluence-client');
+const Analytics = require('../lib/analytics');
+
+const { _test: { withClient } } = require('../bin/confluence');
+
+describe('withClient wrapper', () => {
+  let trackSpy;
+  let exitSpy;
+  let errorSpy;
+
+  beforeEach(() => {
+    getConfig.mockReset();
+    getConfig.mockReturnValue({
+      readOnly: false,
+      domain: 'example.test',
+      token: 't',
+      authType: 'bearer',
+      apiPath: '/rest/api',
+      protocol: 'https'
+    });
+
+    ConfluenceClient.mockClear();
+
+    trackSpy = jest.spyOn(Analytics.prototype, 'track').mockImplementation(() => {});
+    exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {
+      throw new Error('process.exit called');
+    });
+    errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    trackSpy.mockRestore();
+    exitSpy.mockRestore();
+    errorSpy.mockRestore();
+  });
+
+  test('invokes handler with { client, config, analytics } and forwards action args', async () => {
+    const handler = jest.fn().mockResolvedValue();
+    const action = withClient('read', handler);
+
+    await action('PAGE-1', { format: 'text' });
+
+    expect(handler).toHaveBeenCalledTimes(1);
+    const [ctx, pageId, options] = handler.mock.calls[0];
+    expect(ctx).toEqual(expect.objectContaining({
+      client: expect.anything(),
+      config: expect.objectContaining({ readOnly: false }),
+      analytics: expect.anything(),
+    }));
+    expect(pageId).toBe('PAGE-1');
+    expect(options).toEqual({ format: 'text' });
+    expect(ConfluenceClient).toHaveBeenCalledTimes(1);
+    expect(exitSpy).not.toHaveBeenCalled();
+  });
+
+  test('writable: true on read-only profile exits with 1 without invoking handler', async () => {
+    getConfig.mockReturnValue({ readOnly: true });
+    const handler = jest.fn();
+    const action = withClient('create', handler, { writable: true });
+
+    await expect(action('title', 'KEY', {})).rejects.toThrow('process.exit called');
+    expect(handler).not.toHaveBeenCalled();
+    expect(ConfluenceClient).not.toHaveBeenCalled();
+    expect(exitSpy).toHaveBeenCalledWith(1);
+  });
+
+  test('handler throw tracks failure, logs Error message, and exits 1', async () => {
+    const handler = jest.fn().mockRejectedValue(new Error('boom'));
+    const action = withClient('search', handler);
+
+    await expect(action('q', {})).rejects.toThrow('process.exit called');
+    expect(trackSpy).toHaveBeenCalledWith('search', false);
+    expect(errorSpy).toHaveBeenCalledWith(
+      expect.stringContaining('Error:'),
+      'boom'
+    );
+    expect(exitSpy).toHaveBeenCalledWith(1);
+  });
+
+  test('onError receives error + forwarded action args and runs before exit', async () => {
+    const onError = jest.fn();
+    const handler = jest.fn().mockRejectedValue(new Error('api fail'));
+    const action = withClient('comment_create', handler, { writable: true, onError });
+
+    await expect(action('PAGE-1', { location: 'inline' })).rejects.toThrow('process.exit called');
+
+    expect(onError).toHaveBeenCalledTimes(1);
+    const [err, pageId, options] = onError.mock.calls[0];
+    expect(err.message).toBe('api fail');
+    expect(pageId).toBe('PAGE-1');
+    expect(options).toEqual({ location: 'inline' });
+
+    const onErrorOrder = onError.mock.invocationCallOrder[0];
+    const exitOrder = exitSpy.mock.invocationCallOrder[0];
+    expect(onErrorOrder).toBeLessThan(exitOrder);
+  });
+
+  test('onError throwing does not block process.exit', async () => {
+    const onError = jest.fn(() => { throw new Error('hint crashed'); });
+    const handler = jest.fn().mockRejectedValue(new Error('api fail'));
+    const action = withClient('comment_create', handler, { onError });
+
+    await expect(action('PAGE-1', {})).rejects.toThrow('process.exit called');
+    expect(exitSpy).toHaveBeenCalledWith(1);
+  });
+
+  test('config loading failure is caught and reported as a failure track', async () => {
+    getConfig.mockImplementation(() => { throw new Error('no config'); });
+    const handler = jest.fn();
+    const action = withClient('read', handler);
+
+    await expect(action('PAGE-1', {})).rejects.toThrow('process.exit called');
+    expect(handler).not.toHaveBeenCalled();
+    expect(trackSpy).toHaveBeenCalledWith('read', false);
+    expect(exitSpy).toHaveBeenCalledWith(1);
+  });
+});