From fc92f356ed90d80fe2c55cd5e638cf051a36badf Mon Sep 17 00:00:00 2001 From: eeee0717 Date: Thu, 18 Dec 2025 10:06:34 +0800 Subject: [PATCH] fix: pr review --- .../lanTransfer/LanTransferClientService.ts | 2 +- .../LanTransferClientService.test.ts | 137 ++++++++++++++++++ .../__tests__/binaryProtocol.test.ts | 14 ++ .../__tests__/handlers/connection.test.ts | 109 +++++++++++++- .../__tests__/handlers/fileTransfer.test.ts | 62 +++++++- .../services/lanTransfer/binaryProtocol.ts | 4 +- .../lanTransfer/handlers/fileTransfer.ts | 4 +- src/main/services/lanTransfer/index.ts | 2 +- 8 files changed, 323 insertions(+), 11 deletions(-) create mode 100644 src/main/services/lanTransfer/__tests__/LanTransferClientService.test.ts diff --git a/src/main/services/lanTransfer/LanTransferClientService.ts b/src/main/services/lanTransfer/LanTransferClientService.ts index a9335e8f4..68b6a82b9 100644 --- a/src/main/services/lanTransfer/LanTransferClientService.ts +++ b/src/main/services/lanTransfer/LanTransferClientService.ts @@ -43,7 +43,7 @@ const logger = loggerService.withContext('LanTransferClientService') * LAN Transfer Client Service * * Handles outgoing file transfers to LAN peers via TCP. - * Protocol v3 with streaming mode (no per-chunk acknowledgment). + * Protocol v1 with streaming mode (no per-chunk acknowledgment). */ class LanTransferClientService { private socket: Socket | null = null diff --git a/src/main/services/lanTransfer/__tests__/LanTransferClientService.test.ts b/src/main/services/lanTransfer/__tests__/LanTransferClientService.test.ts new file mode 100644 index 000000000..60eb8476c --- /dev/null +++ b/src/main/services/lanTransfer/__tests__/LanTransferClientService.test.ts @@ -0,0 +1,137 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// Mock dependencies before importing the service +vi.mock('node:net', async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + createConnection: vi.fn() + } +}) + +vi.mock('electron', () => ({ + app: { + getName: vi.fn(() => 'Cherry Studio'), + getVersion: vi.fn(() => '1.0.0') + } +})) + +vi.mock('../../LocalTransferService', () => ({ + localTransferService: { + getPeerById: vi.fn() + } +})) + +vi.mock('../../WindowService', () => ({ + windowService: { + getMainWindow: vi.fn(() => ({ + isDestroyed: () => false, + webContents: { + send: vi.fn() + } + })) + } +})) + +// Import after mocks +import { localTransferService } from '../../LocalTransferService' + +describe('LanTransferClientService', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + describe('connectAndHandshake - validation', () => { + it('should throw error when peer is not found', async () => { + vi.mocked(localTransferService.getPeerById).mockReturnValue(undefined) + + const { lanTransferClientService } = await import('../LanTransferClientService') + + await expect( + lanTransferClientService.connectAndHandshake({ + peerId: 'non-existent', + type: 'connect' + }) + ).rejects.toThrow('Selected LAN peer is no longer available') + }) + + it('should throw error when peer has no port', async () => { + vi.mocked(localTransferService.getPeerById).mockReturnValue({ + id: 'test-peer', + name: 'Test Peer', + addresses: ['192.168.1.100'], + updatedAt: Date.now() + }) + + const { lanTransferClientService } = await import('../LanTransferClientService') + + await expect( + lanTransferClientService.connectAndHandshake({ + peerId: 'test-peer', + type: 'connect' + }) + ).rejects.toThrow('Selected peer does not expose a TCP port') + }) + + it('should throw error when no reachable host', async () => { + vi.mocked(localTransferService.getPeerById).mockReturnValue({ + id: 'test-peer', + name: 'Test Peer', + port: 12345, + addresses: [], + updatedAt: Date.now() + }) + + const { lanTransferClientService } = await import('../LanTransferClientService') + + await expect( + lanTransferClientService.connectAndHandshake({ + peerId: 'test-peer', + type: 'connect' + }) + ).rejects.toThrow('Unable to resolve a reachable host for the peer') + }) + + }) + + describe('cancelTransfer', () => { + it('should not throw when no active transfer', async () => { + const { lanTransferClientService } = await import('../LanTransferClientService') + + // Should not throw, just log warning + expect(() => lanTransferClientService.cancelTransfer()).not.toThrow() + }) + }) + + describe('dispose', () => { + it('should clean up resources without throwing', async () => { + const { lanTransferClientService } = await import('../LanTransferClientService') + + // Should not throw + expect(() => lanTransferClientService.dispose()).not.toThrow() + }) + }) + + describe('sendFile', () => { + it('should throw error when not connected', async () => { + const { lanTransferClientService } = await import('../LanTransferClientService') + + await expect(lanTransferClientService.sendFile('/path/to/file.zip')).rejects.toThrow( + 'No active connection. Please connect to a peer first.' + ) + }) + }) + + describe('HANDSHAKE_PROTOCOL_VERSION', () => { + it('should export protocol version', async () => { + const { HANDSHAKE_PROTOCOL_VERSION } = await import('../LanTransferClientService') + + expect(HANDSHAKE_PROTOCOL_VERSION).toBe('1') + }) + }) +}) diff --git a/src/main/services/lanTransfer/__tests__/binaryProtocol.test.ts b/src/main/services/lanTransfer/__tests__/binaryProtocol.test.ts index cf9d432df..e65a0d71a 100644 --- a/src/main/services/lanTransfer/__tests__/binaryProtocol.test.ts +++ b/src/main/services/lanTransfer/__tests__/binaryProtocol.test.ts @@ -13,6 +13,8 @@ describe('binaryProtocol', () => { beforeEach(() => { writtenBuffers = [] mockSocket = Object.assign(new EventEmitter(), { + destroyed: false, + writable: true, write: vi.fn((buffer: Buffer) => { writtenBuffers.push(Buffer.from(buffer)) return true @@ -79,6 +81,18 @@ describe('binaryProtocol', () => { const expectedTotalLen = 1 + 2 + Buffer.from(transferId).length + 4 + data.length expect(totalLen).toBe(expectedTotalLen) }) + + it('should throw error when socket is not writable', () => { + mockSocket.writable = false + + expect(() => sendBinaryChunk(mockSocket, 'test-id', 0, Buffer.from('data'))).toThrow('Socket is not writable') + }) + + it('should throw error when socket is destroyed', () => { + mockSocket.destroyed = true + + expect(() => sendBinaryChunk(mockSocket, 'test-id', 0, Buffer.from('data'))).toThrow('Socket is not writable') + }) }) describe('BINARY_TYPE_FILE_CHUNK', () => { diff --git a/src/main/services/lanTransfer/__tests__/handlers/connection.test.ts b/src/main/services/lanTransfer/__tests__/handlers/connection.test.ts index 9ac98f23a..bed36aa73 100644 --- a/src/main/services/lanTransfer/__tests__/handlers/connection.test.ts +++ b/src/main/services/lanTransfer/__tests__/handlers/connection.test.ts @@ -1,11 +1,15 @@ -import { describe, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' +import type { Socket } from 'node:net' + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { buildHandshakeMessage, createDataHandler, getAbortError, HANDSHAKE_PROTOCOL_VERSION, - pickHost + pickHost, + waitForSocketDrain } from '../../handlers/connection' // Mock electron app @@ -28,7 +32,7 @@ describe('connection handlers', () => { expect(typeof message.platform).toBe('string') }) - it('should use protocol version 3', () => { + it('should use protocol version 1', () => { expect(HANDSHAKE_PROTOCOL_VERSION).toBe('1') }) }) @@ -137,6 +141,105 @@ describe('connection handlers', () => { expect(lines).toEqual(['{"type":"test"}']) }) + + it('should throw error when buffer exceeds MAX_LINE_BUFFER_SIZE', () => { + const handler = createDataHandler(vi.fn()) + + // Create a buffer larger than 1MB (MAX_LINE_BUFFER_SIZE) + const largeData = 'x'.repeat(1024 * 1024 + 1) + + expect(() => handler.handleData(Buffer.from(largeData))).toThrow('Control message too large') + }) + + it('should reset buffer after exceeding MAX_LINE_BUFFER_SIZE', () => { + const lines: string[] = [] + const handler = createDataHandler((line) => lines.push(line)) + + // Create a buffer larger than 1MB + const largeData = 'x'.repeat(1024 * 1024 + 1) + + try { + handler.handleData(Buffer.from(largeData)) + } catch { + // Expected error + } + + // Buffer should be reset, so lineBuffer should be empty + expect(handler.lineBuffer).toBe('') + }) + }) + + describe('waitForSocketDrain', () => { + let mockSocket: Socket & EventEmitter + + beforeEach(() => { + mockSocket = Object.assign(new EventEmitter(), { + destroyed: false, + writable: true, + write: vi.fn(), + off: vi.fn(), + removeAllListeners: vi.fn() + }) as unknown as Socket & EventEmitter + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + it('should throw error when abort signal is already aborted', async () => { + const abortController = new AbortController() + abortController.abort(new Error('Already aborted')) + + await expect(waitForSocketDrain(mockSocket, abortController.signal)).rejects.toThrow('Already aborted') + }) + + it('should throw error when socket is destroyed', async () => { + mockSocket.destroyed = true + const abortController = new AbortController() + + await expect(waitForSocketDrain(mockSocket, abortController.signal)).rejects.toThrow('Socket is closed') + }) + + it('should resolve when drain event is emitted', async () => { + const abortController = new AbortController() + + const drainPromise = waitForSocketDrain(mockSocket, abortController.signal) + + // Emit drain event after a short delay + setImmediate(() => mockSocket.emit('drain')) + + await expect(drainPromise).resolves.toBeUndefined() + }) + + it('should reject when close event is emitted', async () => { + const abortController = new AbortController() + + const drainPromise = waitForSocketDrain(mockSocket, abortController.signal) + + setImmediate(() => mockSocket.emit('close')) + + await expect(drainPromise).rejects.toThrow('Socket closed while waiting for drain') + }) + + it('should reject when error event is emitted', async () => { + const abortController = new AbortController() + + const drainPromise = waitForSocketDrain(mockSocket, abortController.signal) + + setImmediate(() => mockSocket.emit('error', new Error('Network error'))) + + await expect(drainPromise).rejects.toThrow('Network error') + }) + + it('should reject when abort signal is triggered', async () => { + const abortController = new AbortController() + + const drainPromise = waitForSocketDrain(mockSocket, abortController.signal) + + setImmediate(() => abortController.abort(new Error('User cancelled'))) + + await expect(drainPromise).rejects.toThrow('User cancelled') + }) }) describe('getAbortError', () => { diff --git a/src/main/services/lanTransfer/__tests__/handlers/fileTransfer.test.ts b/src/main/services/lanTransfer/__tests__/handlers/fileTransfer.test.ts index 4119cd044..94cf3f1f7 100644 --- a/src/main/services/lanTransfer/__tests__/handlers/fileTransfer.test.ts +++ b/src/main/services/lanTransfer/__tests__/handlers/fileTransfer.test.ts @@ -1,10 +1,28 @@ +import { EventEmitter } from 'node:events' import type * as fs from 'node:fs' +import type { Socket } from 'node:net' -import { describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { abortTransfer, cleanupTransfer, createTransferState, formatFileSize } from '../../handlers/fileTransfer' +import { abortTransfer, cleanupTransfer, createTransferState, formatFileSize, streamFileChunks } from '../../handlers/fileTransfer' import type { ActiveFileTransfer } from '../../types' +// Mock binaryProtocol +vi.mock('../../binaryProtocol', () => ({ + sendBinaryChunk: vi.fn().mockReturnValue(true) +})) + +// Mock connection handlers +vi.mock('./connection', () => ({ + waitForSocketDrain: vi.fn().mockResolvedValue(undefined), + getAbortError: vi.fn((signal, fallback) => { + const reason = (signal as AbortSignal & { reason?: unknown }).reason + if (reason instanceof Error) return reason + if (typeof reason === 'string' && reason.length > 0) return new Error(reason) + return new Error(fallback) + }) +})) + // Note: validateFile and calculateFileChecksum tests are skipped because // the test environment has globally mocked node:fs and node:os modules. // These functions are tested through integration tests instead. @@ -149,4 +167,44 @@ describe('fileTransfer handlers', () => { expect(formatFileSize(1.5 * 1024 * 1024)).toBe('1.5 MB') }) }) + + // Note: streamFileChunks tests require careful mocking of fs.createReadStream + // which is globally mocked in the test environment. These tests verify the + // streaming logic works correctly with mock streams. + describe('streamFileChunks', () => { + let mockSocket: Socket & EventEmitter + let mockProgress: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + + mockSocket = Object.assign(new EventEmitter(), { + destroyed: false, + writable: true, + write: vi.fn().mockReturnValue(true), + cork: vi.fn(), + uncork: vi.fn() + }) as unknown as Socket & EventEmitter + + mockProgress = vi.fn() + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + it('should throw when abort signal is already aborted', async () => { + const transfer = createTransferState('test-id', 'test.zip', 1024, 'checksum') + transfer.abortController.abort(new Error('Already cancelled')) + + await expect( + streamFileChunks(mockSocket, '/fake/path.zip', transfer, transfer.abortController.signal, mockProgress) + ).rejects.toThrow() + }) + + // Note: Full integration testing of streamFileChunks with actual file streaming + // requires a real file system, which cannot be easily mocked in ESM. + // The abort signal test above verifies the early abort path. + // Additional streaming tests are covered through integration tests. + }) }) diff --git a/src/main/services/lanTransfer/binaryProtocol.ts b/src/main/services/lanTransfer/binaryProtocol.ts index fc1f57a0b..864a8b95b 100644 --- a/src/main/services/lanTransfer/binaryProtocol.ts +++ b/src/main/services/lanTransfer/binaryProtocol.ts @@ -1,12 +1,12 @@ import type { Socket } from 'node:net' /** - * Binary protocol constants (v3) + * Binary protocol constants (v1) */ export const BINARY_TYPE_FILE_CHUNK = 0x01 /** - * Send file chunk as binary frame (protocol v3 - streaming mode) + * Send file chunk as binary frame (protocol v1 - streaming mode) * * Frame format: * ``` diff --git a/src/main/services/lanTransfer/handlers/fileTransfer.ts b/src/main/services/lanTransfer/handlers/fileTransfer.ts index 86ea61e13..c469a5842 100644 --- a/src/main/services/lanTransfer/handlers/fileTransfer.ts +++ b/src/main/services/lanTransfer/handlers/fileTransfer.ts @@ -175,7 +175,7 @@ export function sendFileEnd(ctx: FileTransferContext, transferId: string): void } /** - * Stream file chunks to the receiver (v3 streaming mode - no per-chunk acknowledgment). + * Stream file chunks to the receiver (v1 streaming mode - no per-chunk acknowledgment). */ export async function streamFileChunks( socket: Socket, @@ -201,7 +201,7 @@ export async function streamFileChunks( const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk) bytesSent += buffer.length - // Send chunk as binary frame (v3 streaming) with backpressure handling + // Send chunk as binary frame (v1 streaming) with backpressure handling const canContinue = sendBinaryChunk(socket, transferId, chunkIndex, buffer) if (!canContinue) { await waitForSocketDrain(socket, abortSignal) diff --git a/src/main/services/lanTransfer/index.ts b/src/main/services/lanTransfer/index.ts index ad6cd2862..12f3c38af 100644 --- a/src/main/services/lanTransfer/index.ts +++ b/src/main/services/lanTransfer/index.ts @@ -1,7 +1,7 @@ /** * LAN Transfer Client Module * - * Protocol: v3.0 (streaming mode) + * Protocol: v1.0 (streaming mode) * * Features: * - Binary frame format for file chunks (no base64 overhead)