-
Notifications
You must be signed in to change notification settings - Fork 0
feat(cloud-preview): ✨ Improve cloud preview integration #61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,109 @@ class CloudService { | |
|
|
||
| private constructor() {} | ||
|
|
||
| private extractDownloadFileName(contentDisposition: string, fallback: string): string { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Missing unit tests for complex Content-Disposition filename parsing and encoding fallbacks A large, multi-branch parsing pipeline replaced a simple filename regex, but no tests are shown to validate behavior across valid/invalid headers, charsets, and fallback interactions. Suggestion: Add focused unit tests for Risk: Incorrect filenames in downloads, silent fallback to wrong names, and regressions on internationalized filenames (especially CJK/latin1 edge cases). Confidence: 0.93 [From SubAgent: testing]
|
||
| const rfc5987Name = this.parseRFC5987Filename(contentDisposition); | ||
| if (rfc5987Name) { | ||
| return this.sanitizeDownloadFileName(rfc5987Name, fallback); | ||
| } | ||
|
|
||
| const plainName = this.parsePlainFilename(contentDisposition); | ||
| if (!plainName) { | ||
| return this.sanitizeDownloadFileName(fallback, fallback); | ||
| } | ||
|
|
||
| const repairedName = this.tryRepairUtf8Mojibake(plainName); | ||
| return this.sanitizeDownloadFileName(repairedName || plainName, fallback); | ||
| } | ||
|
|
||
| private parseRFC5987Filename(contentDisposition: string): string | null { | ||
| const match = contentDisposition.match(/filename\*\s*=\s*([^;]+)/i); | ||
| if (!match) return null; | ||
|
|
||
| const rawValue = match[1]?.trim(); | ||
| if (!rawValue) return null; | ||
|
|
||
| const unquoted = rawValue.replace(/^"(.*)"$/, '$1'); | ||
| const parts = unquoted.match(/^([^']*)'[^']*'(.*)$/); | ||
| if (!parts) return null; | ||
|
|
||
| const charset = (parts[1] || 'utf-8').trim().toLowerCase(); | ||
| const encodedValue = parts[2] || ''; | ||
|
|
||
| try { | ||
| if (charset === 'utf-8' || charset === 'utf8') { | ||
| return decodeURIComponent(encodedValue); | ||
| } | ||
|
|
||
| const bytes = this.percentDecodeToBytes(encodedValue); | ||
| if (charset === 'iso-8859-1' || charset === 'latin1') { | ||
| return Buffer.from(bytes).toString('latin1'); | ||
| } | ||
| return Buffer.from(bytes).toString('utf8'); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private parsePlainFilename(contentDisposition: string): string | null { | ||
| const match = contentDisposition.match(/filename\s*=\s*("(?:\\.|[^"])*"|[^;]+)/i); | ||
| if (!match) return null; | ||
|
|
||
| let value = match[1]?.trim(); | ||
| if (!value) return null; | ||
|
|
||
| if (value.startsWith('"') && value.endsWith('"')) { | ||
| value = value.slice(1, -1).replace(/\\"/g, '"'); | ||
| } | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| private percentDecodeToBytes(input: string): number[] { | ||
This comment was marked as outdated.
Sorry, something went wrong. |
||
| const bytes: number[] = []; | ||
| for (let i = 0; i < input.length; i++) { | ||
| const ch = input[i]; | ||
| if (ch === '%' && i + 2 < input.length) { | ||
| const hex = input.slice(i + 1, i + 3); | ||
| const parsed = Number.parseInt(hex, 16); | ||
| if (!Number.isNaN(parsed)) { | ||
| bytes.push(parsed); | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } | ||
| bytes.push(input.charCodeAt(i)); | ||
| } | ||
| return bytes; | ||
| } | ||
|
|
||
| private tryRepairUtf8Mojibake(input: string): string | null { | ||
| const hasCjk = /[\u4e00-\u9fff\u3040-\u30ff\uac00-\ud7af]/.test(input); | ||
| if (hasCjk) return null; | ||
|
|
||
| const latinSupplementCount = Array.from(input).filter((ch) => { | ||
| const code = ch.charCodeAt(0); | ||
| return code >= 0x00c0 && code <= 0x00ff; | ||
| }).length; | ||
| if (latinSupplementCount < 2) return null; | ||
|
|
||
| const repaired = Buffer.from(input, 'latin1').toString('utf8'); | ||
| if (!repaired) return null; | ||
|
|
||
| const repairedHasCjk = /[\u4e00-\u9fff\u3040-\u30ff\uac00-\ud7af]/.test(repaired); | ||
| const roundTrip = Buffer.from(repaired, 'utf8').toString('latin1') === input; | ||
| if (repairedHasCjk && roundTrip) { | ||
| return repaired; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private sanitizeDownloadFileName(input: string, fallback: string): string { | ||
| // Sanitize: extract basename and strip control/reserved characters | ||
| // eslint-disable-next-line no-control-regex | ||
| return path.basename(input).replace(/[\u0000-\u001f<>:"|?*]/g, '_') || fallback; | ||
| } | ||
|
|
||
| private normalizeCheckoutStatus(data: any): PaymentCheckoutStatusApiResponse | null { | ||
| if (!data || typeof data !== 'object') { | ||
| return null; | ||
|
|
@@ -454,11 +557,8 @@ class CloudService { | |
| } | ||
|
|
||
| const contentDisposition = res.headers.get('Content-Disposition') || ''; | ||
| const match = contentDisposition.match(/filename="?([^";\n]+)"?/); | ||
| const rawName = match ? match[1] : `task-${id}.pdf`; | ||
| // Sanitize: extract basename and strip control/reserved characters | ||
| // eslint-disable-next-line no-control-regex | ||
| const fileName = path.basename(rawName).replace(/[\u0000-\u001f<>:"|?*]/g, '_') || `task-${id}.pdf`; | ||
| const fallbackName = `task-${id}.pdf`; | ||
| const fileName = this.extractDownloadFileName(contentDisposition, fallbackName); | ||
|
|
||
| const buffer = await res.arrayBuffer(); | ||
| return { success: true, data: { buffer, fileName } }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,16 @@ const mockDialog = { | |
| showSaveDialog: vi.fn() | ||
| } | ||
|
|
||
| const mockClipboard = { | ||
| writeImage: vi.fn(), | ||
| } | ||
|
|
||
| const mockNativeImage = { | ||
| createFromPath: vi.fn(), | ||
| createFromDataURL: vi.fn(), | ||
| createFromBuffer: vi.fn(), | ||
| } | ||
|
|
||
| const mockFs = { | ||
| existsSync: vi.fn(), | ||
| mkdirSync: vi.fn(), | ||
|
|
@@ -36,7 +46,9 @@ const mockIpcMain = { | |
| // Mock modules | ||
| vi.mock('electron', () => ({ | ||
| ipcMain: mockIpcMain, | ||
| dialog: mockDialog | ||
| dialog: mockDialog, | ||
| clipboard: mockClipboard, | ||
| nativeImage: mockNativeImage, | ||
| })) | ||
|
|
||
| vi.mock('path', () => ({ | ||
|
|
@@ -66,6 +78,7 @@ vi.mock('../../../../shared/ipc/channels.js', () => ({ | |
| FILE: { | ||
| GET_IMAGE_PATH: 'file:getImagePath', | ||
| DOWNLOAD_MARKDOWN: 'file:downloadMarkdown', | ||
| COPY_IMAGE_TO_CLIPBOARD: 'file:copyImageToClipboard', | ||
| SELECT_DIALOG: 'file:selectDialog', | ||
| UPLOAD: 'file:upload', | ||
| UPLOAD_FILE_CONTENT: 'file:uploadFileContent' | ||
|
|
@@ -87,11 +100,112 @@ describe('File Handler', () => { | |
| mockFileLogic.getUploadDir.mockReturnValue('/uploads') | ||
| mockFs.statSync.mockReturnValue({ size: 1024 }) | ||
| mockFs.existsSync.mockReturnValue(true) | ||
| const fakeImage = { isEmpty: vi.fn(() => false) } | ||
| mockNativeImage.createFromPath.mockReturnValue(fakeImage) | ||
| mockNativeImage.createFromDataURL.mockReturnValue(fakeImage) | ||
| mockNativeImage.createFromBuffer.mockReturnValue(fakeImage) | ||
|
|
||
| const { registerFileHandlers } = await import('../file.handler.js') | ||
| registerFileHandlers() | ||
| }) | ||
|
|
||
| describe('file:copyImageToClipboard', () => { | ||
This comment was marked as outdated.
Sorry, something went wrong. |
||
| it('should copy image from local path successfully', async () => { | ||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, '/tmp/page.png') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: true, | ||
| data: { copied: true }, | ||
| }) | ||
| expect(mockNativeImage.createFromPath).toHaveBeenCalledWith('/tmp/page.png') | ||
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Clipboard tests do not verify the image object passed to writeImage New success-path tests only check that writeImage was invoked, not that it received the created native image. A handler bug could call writeImage with the wrong object (or undefined) while these tests still pass. Suggestion: Strengthen assertions with Risk: Reduced test effectiveness; regressions in argument wiring may slip through CI. Confidence: 0.94 [From SubAgent: general]
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Potential flakiness from missing mock reset expectations around clipboard/nativeImage call counts Call-count assertions on shared mocks are vulnerable if cleanup/reset policy changes or if setup invokes these mocks unexpectedly. Suggestion: Ensure Risk: Inter-test state leakage can produce non-deterministic failures and brittle CI behavior. Confidence: 0.85 [From SubAgent: testing]
|
||
| }) | ||
|
|
||
| it('should copy image from data URL successfully', async () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Missing assertion that only one nativeImage factory is used per source type Tests validate successful path selection but not exclusivity, so a bug invoking multiple decoding branches could pass unnoticed. Suggestion: For each input type test, add negative assertions such as Risk: Regression in branch routing may cause incorrect parsing, extra work, or surprising side effects without failing tests. Confidence: 0.93 [From SubAgent: testing]
|
||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, 'data:image/png;base64,abcd') | ||
|
|
||
| expect(result.success).toBe(true) | ||
| expect(mockNativeImage.createFromDataURL).toHaveBeenCalledWith('data:image/png;base64,abcd') | ||
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('should copy image from file URL successfully', async () => { | ||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, 'file:///tmp/page.png') | ||
|
|
||
| expect(result.success).toBe(true) | ||
| expect(mockNativeImage.createFromPath).toHaveBeenCalledWith(expect.stringContaining('page.png')) | ||
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('should return error when image source is missing', async () => { | ||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, '') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: 'Image source is required', | ||
| }) | ||
| }) | ||
|
|
||
| it('should reject remote image URLs', async () => { | ||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, 'https://cdn.example.com/page.png') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: 'Remote image URLs are not allowed', | ||
| }) | ||
| expect(mockClipboard.writeImage).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should return error when image is empty', async () => { | ||
| mockNativeImage.createFromPath.mockReturnValueOnce({ | ||
| isEmpty: vi.fn(() => true), | ||
| }) | ||
|
|
||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, '/tmp/empty.png') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: 'Image data is empty or invalid', | ||
| }) | ||
| expect(mockClipboard.writeImage).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should return error when nativeImage creation throws', async () => { | ||
| mockNativeImage.createFromPath.mockImplementationOnce(() => { | ||
| throw new Error('createFromPath failed') | ||
| }) | ||
|
|
||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, '/tmp/bad.png') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: 'createFromPath failed', | ||
| }) | ||
| expect(mockClipboard.writeImage).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should return error when clipboard write throws', async () => { | ||
| mockClipboard.writeImage.mockImplementationOnce(() => { | ||
| throw new Error('clipboard failed') | ||
| }) | ||
|
|
||
| const handler = handlers.get('file:copyImageToClipboard') | ||
| const result = await handler!({}, '/tmp/page.png') | ||
|
|
||
| expect(result).toEqual({ | ||
| success: false, | ||
| error: 'clipboard failed', | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('file:getImagePath', () => { | ||
| it('should return image path and exists status', async () => { | ||
| mockFs.existsSync.mockReturnValue(true) | ||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.