-
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 1 commit
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,63 @@ 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).toHaveBeenCalled() | ||
|
||
| }) | ||
|
|
||
| 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).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| 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 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() | ||
| }) | ||
| }) | ||
|
|
||
| describe('file:getImagePath', () => { | ||
| it('should return image path and exists status', async () => { | ||
| mockFs.existsSync.mockReturnValue(true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { ipcMain, dialog } from "electron"; | ||
| import { ipcMain, dialog, clipboard, nativeImage } from "electron"; | ||
| import path from "path"; | ||
| import fs from "fs"; | ||
| import taskRepository from "../../../core/domain/repositories/TaskRepository.js"; | ||
|
|
@@ -7,6 +7,23 @@ import { ImagePathUtil } from "../../../core/infrastructure/adapters/split/index | |
| import { IPC_CHANNELS } from "../../../shared/ipc/channels.js"; | ||
| import type { IpcResponse } from "../../../shared/ipc/responses.js"; | ||
|
|
||
| async function createImageFromSource(imageSource: string) { | ||
| if (imageSource.startsWith("data:image/")) { | ||
| return nativeImage.createFromDataURL(imageSource); | ||
| } | ||
|
|
||
| if (imageSource.startsWith("http://") || imageSource.startsWith("https://")) { | ||
|
||
| const response = await fetch(imageSource); | ||
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch image: ${response.status}`); | ||
| } | ||
| const buffer = Buffer.from(await response.arrayBuffer()); | ||
| return nativeImage.createFromBuffer(buffer); | ||
| } | ||
|
|
||
| return nativeImage.createFromPath(imageSource); | ||
| } | ||
|
|
||
| /** | ||
| * Register all file-related IPC handlers | ||
| */ | ||
|
|
@@ -96,6 +113,31 @@ export function registerFileHandlers() { | |
| } | ||
| ); | ||
|
|
||
| /** | ||
| * Copy image to clipboard | ||
| */ | ||
| ipcMain.handle( | ||
|
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 IPC handler tests for copyImageToClipboard input classes and error paths New Electron IPC behavior adds security-sensitive source validation and environment-dependent image creation, but no tests are shown for accepted/rejected sources and response contract stability. Suggestion: Add handler-level tests with mocked Risk: Flaky or broken clipboard UX across platforms, accidental acceptance of disallowed inputs, and unstable renderer-facing error handling. Confidence: 0.95 [From SubAgent: testing]
|
||
| IPC_CHANNELS.FILE.COPY_IMAGE_TO_CLIPBOARD, | ||
| async (_, imageSource: string): Promise<IpcResponse> => { | ||
| try { | ||
| if (!imageSource) { | ||
| return { success: false, error: "Image source is required" }; | ||
| } | ||
|
|
||
| const image = await createImageFromSource(imageSource); | ||
| if (image.isEmpty()) { | ||
| return { success: false, error: "Image data is empty or invalid" }; | ||
| } | ||
|
|
||
| clipboard.writeImage(image); | ||
| return { success: true, data: { copied: true } }; | ||
| } catch (error: any) { | ||
| console.error("[IPC] file:copyImageToClipboard error:", error); | ||
| return { success: false, error: error.message || "Failed to copy image" }; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| /** | ||
| * File selection dialog | ||
| * @param allowOffice - If true, includes Office file types in the filter | ||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.