From 21b9d49303fb27b1e8050a5b71432e4074efa5e1 Mon Sep 17 00:00:00 2001 From: "heecheol.park" Date: Mon, 11 May 2026 11:57:15 +0900 Subject: [PATCH 1/2] refactor(cli): migrate comment command to withClient with onError hook The comment command was the only client-using command left with manual analytics + getConfig + assertWritable + ConfluenceClient + try/catch scaffolding after #181, because its catch block needed to print API response details and inline-comment-specific hints. Add an `onError(error, ...actionArgs)` option to withClient that runs between the standard `Error:` log line and process.exit, and extend handleCommandError with a matching `onExtra` callback. The hook is wrapped in try/catch so a throwing hint path cannot break the error flow. Move the comment command's API-response dump and inline-meta hint into the hook. Existing behavior (output ordering, exit code, failure tracking) is preserved. Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/confluence.js | 157 +++++++++++++++++++++++----------------------- 1 file changed, 78 insertions(+), 79 deletions(-) diff --git a/bin/confluence.js b/bin/confluence.js index d091d9d..afe1d81 100755 --- a/bin/confluence.js +++ b/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 ', 'Original inline selection text') .option('--inline-marker-ref ', 'Inline marker reference (optional)') .option('--inline-properties ', '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 From 4cf2c06228247912beb9e159075575af678c2516 Mon Sep 17 00:00:00 2001 From: "heecheol.park" Date: Mon, 11 May 2026 11:57:22 +0900 Subject: [PATCH 2/2] test(cli): add unit tests for withClient wrapper The withClient helper sits on the shared error/analytics/client path for every CLI command that talks to the server but had no direct test coverage. Add six cases covering: context injection ({ client, config, analytics } + forwarded action args), the writable-on-readOnly exit path, failure tracking on handler throw, the new onError hook (invocation order before process.exit, action-arg forwarding, throw isolation), and getConfig failure being treated as a tracked failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/with-client.test.js | 133 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 tests/with-client.test.js diff --git a/tests/with-client.test.js b/tests/with-client.test.js new file mode 100644 index 0000000..5ed6189 --- /dev/null +++ b/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); + }); +});