feat(cloud-preview): ✨ Improve cloud preview integration#61
Conversation
AI Code Review SummaryPR: #61 (feat(cloud-preview): ✨ Improve cloud preview integration) Overall AssessmentDetected 16 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| private constructor() {} | ||
|
|
||
| private extractDownloadFileName(contentDisposition: string, fallback: string): string { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return nativeImage.createFromDataURL(imageSource); | ||
| } | ||
|
|
||
| if (imageSource.startsWith("http://") || imageSource.startsWith("https://")) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| setDownloading(true); | ||
| try { | ||
| const result = await cloudContext.downloadResult(id); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return value; | ||
| } | ||
|
|
||
| private percentDecodeToBytes(input: string): number[] { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| registerFileHandlers() | ||
| }) | ||
|
|
||
| describe('file:copyImageToClipboard', () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| FILE: { | ||
| GET_IMAGE_PATH: 'file:getImagePath', | ||
| DOWNLOAD_MARKDOWN: 'file:downloadMarkdown', | ||
| COPY_IMAGE_TO_CLIPBOARD: 'file:copyImageToClipboard', |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| data: { copied: true }, | ||
| }) | ||
| expect(mockNativeImage.createFromPath).toHaveBeenCalledWith('/tmp/page.png') | ||
| expect(mockClipboard.writeImage).toHaveBeenCalled() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| renderPage(ctx) | ||
|
|
||
| await waitFor(() => { | ||
| const img = document.querySelector('img') as HTMLImageElement |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
|
|
||
| await waitFor(() => { | ||
| const img = document.querySelector('img') |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
|
|
||
| await waitFor(() => { | ||
| const img = document.querySelector('img') |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| private constructor() {} | ||
|
|
||
| private extractDownloadFileName(contentDisposition: string, fallback: string): string { |
There was a problem hiding this comment.
[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 extractDownloadFileName via public method behavior (download result filename), covering: RFC5987 utf-8/latin1, malformed percent-encoding, quoted/unquoted plain filename, both filename* and filename precedence, empty/missing header fallback (task-${id}.pdf), mojibake repair positive/negative cases, and sanitization/path traversal/control chars.
Risk: Incorrect filenames in downloads, silent fallback to wrong names, and regressions on internationalized filenames (especially CJK/latin1 edge cases).
Confidence: 0.93
| /** | ||
| * Copy image to clipboard | ||
| */ | ||
| ipcMain.handle( |
There was a problem hiding this comment.
[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 ipcMain.handle, nativeImage, and clipboard: (1) missing source -> error, (2) data URL success, (3) file:// source conversion path, (4) plain local path success, (5) http/https rejected, (6) isEmpty() true returns validation error, (7) thrown error returns {success:false,error}.
Risk: Flaky or broken clipboard UX across platforms, accidental acceptance of disallowed inputs, and unstable renderer-facing error handling.
Confidence: 0.95
|
|
||
| setDownloading(true); | ||
| try { | ||
| const result = await cloudContext.downloadResult(id); |
There was a problem hiding this comment.
[HIGH] PDF download action calls markdown download API
The new handleDownloadPdf uses cloudContext.downloadResult(id), which appears to be the same endpoint used for markdown download, so the PDF menu item may download the wrong artifact.
Suggestion: Use a dedicated PDF download method (e.g., cloudContext.downloadPdf(id)) or pass an explicit format parameter (e.g., downloadResult(id, 'pdf')). Ensure the backend/IPC handler differentiates output type and filename/MIME.
Risk: Users selecting “Download PDF” may receive markdown instead, causing functional mismatch and confusion.
Confidence: 0.98
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('should copy image from data URL successfully', async () => { |
There was a problem hiding this comment.
[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 expect(mockNativeImage.createFromPath).not.toHaveBeenCalled() (for data URL), and similarly for other factories.
Risk: Regression in branch routing may cause incorrect parsing, extra work, or surprising side effects without failing tests.
Confidence: 0.93
| return nativeImage.createFromPath(fileURLToPath(normalizedSource)); | ||
| } | ||
|
|
||
| return nativeImage.createFromPath(normalizedSource); |
There was a problem hiding this comment.
[MEDIUM] IPC handler allows arbitrary local file path reads via copyImageToClipboard
The new IPC endpoint blocks remote URLs, but still accepts arbitrary absolute/relative local paths from renderer input. This can be abused to probe/read local files as images (or sensitive paths) through privileged main-process file access.
Suggestion: Restrict accepted sources to a trusted allowlist (e.g., only app-managed task image directories), and validate canonicalized paths against that base. Consider rejecting plain paths entirely and only accepting internally-issued IDs or prevalidated file:// paths. Return a generic error for disallowed paths.
Risk: Renderer-controlled input can cause the main process to access unintended local filesystem locations, increasing attack surface and weakening sandbox boundaries.
Confidence: 0.89
| FILE: { | ||
| GET_IMAGE_PATH: 'file:getImagePath', | ||
| DOWNLOAD_MARKDOWN: 'file:downloadMarkdown', | ||
| COPY_IMAGE_TO_CLIPBOARD: 'file:copyImageToClipboard', |
There was a problem hiding this comment.
[MEDIUM] Missing contract/integration tests for new IPC channel exposure across channels/preload/types
The new IPC operation spans channel constants, preload bridge, and TS interfaces, but no tests are shown to validate end-to-end wiring and runtime channel name consistency.
Suggestion: Add a lightweight integration test (or contract test) asserting renderer call window.api.file.copyImageToClipboard invokes the exact channel key and returns typed {copied:true} payload on success; include negative contract for error propagation.
Risk: Runtime mismatches between declared API and actual handler/channel wiring that compile but fail at execution.
Confidence: 0.90
| window.HTMLElement.prototype.scrollIntoView = vi.fn() | ||
| } | ||
|
|
||
| Object.defineProperty(navigator, 'clipboard', { |
There was a problem hiding this comment.
[MEDIUM] Global clipboard mock is not reset per test and can leak call history/implementation
navigator.clipboard.writeText is created once at module init and not re-initialized in beforeEach, so test-specific overrides (e.g., mockRejectedValueOnce) and call state may leak or become order-dependent across suites.
Suggestion: Recreate/reset clipboard mock in beforeEach similarly to other window.api mocks (or call vi.mocked(navigator.clipboard.writeText).mockReset().mockResolvedValue(undefined) in beforeEach). This keeps each test isolated and deterministic.
Risk: Flaky tests, hidden inter-test coupling, and false positives/negatives when copy-related tests are run in different orders.
Confidence: 0.94
| data: { copied: true }, | ||
| }) | ||
| expect(mockNativeImage.createFromPath).toHaveBeenCalledWith('/tmp/page.png') | ||
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
[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 expect(mockClipboard.writeImage).toHaveBeenCalledWith(fakeImage) (or the specific instance for each path/dataURL case). Keep the count assertion as secondary.
Risk: Reduced test effectiveness; regressions in argument wiring may slip through CI.
Confidence: 0.94
| data: { copied: true }, | ||
| }) | ||
| expect(mockNativeImage.createFromPath).toHaveBeenCalledWith('/tmp/page.png') | ||
| expect(mockClipboard.writeImage).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
[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 beforeEach(() => vi.clearAllMocks()) (or resetAllMocks) is explicitly present in this suite and keep assertions robust with precondition checks.
Risk: Inter-test state leakage can produce non-deterministic failures and brittle CI behavior.
Confidence: 0.85
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText('Download MD')).toBeInTheDocument() | ||
| const downloadButton = screen.getByText('Download').closest('button') |
There was a problem hiding this comment.
[LOW] Dropdown interaction test uses brittle text+closest selector pattern
Using getByText(...).closest('button') is fragile with nested markup/localization changes and can produce intermittent failures.
Suggestion: Use role-based selectors directly, e.g. getByRole('button', { name: 'Download' }), and consider userEvent.click for realistic dropdown interaction.
Risk: Minor DOM/i18n changes may break tests without functional regression.
Confidence: 0.88
1 participant
Summary
Test Plan
npm run lintnpm testnpm run dev)🤖 Generated with Codex Cli