fix: pr review

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

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<string, unknown>
+  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')
+    })
+  })
+})

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', () => {

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', () => {

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<typeof vi.fn>
+
+    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.
+  })
 })

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:
  * ```

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)

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)