chore: save downloads to outputDir #310
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the file handling for downloads and screenshots by ensuring that file paths are derived from the provided output directory and that filenames are safely formatted in certain tools. Key changes include:
- Adding a new test to verify that download links emit the correct download output.
- Updating screenshot and PDF tools to sanitize timestamps in filenames.
- Implementing download event handling and reporting in the browser context, along with adjustments in output file generation.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/files.spec.ts | Added tests for verifying download link behavior and output directory usage. |
| src/tools/snapshot.ts | Updated screenshot filename generation using sanitizeForFilePath. |
| src/tools/pdf.ts | Updated PDF filename generation by incorporating sanitizeForFilePath. |
| src/tab.ts | Introduced a download event listener to trigger downloadStarted. |
| src/context.ts | Added logic to track downloads and display download status in snapshot output. |
| src/config.ts | Modified outputFile to use the provided name directly, shifting filename sanitization responsibility to the callers. |
| const entry = { | ||
| download, | ||
| finished: false, | ||
| outputFile: await outputFile(this.config, download.suggestedFilename()) |
There was a problem hiding this comment.
Consider wrapping download.suggestedFilename() with sanitizeForFilePath in downloadStarted for consistent file naming and to avoid potential issues with unsafe characters.
src/config.ts
Outdated
| await fs.promises.mkdir(result, { recursive: true }); | ||
| const fileName = sanitizeForFilePath(name); | ||
| return path.join(result, fileName); | ||
| return path.join(result, name); |
There was a problem hiding this comment.
Copilot has a point, this should always be here.
There was a problem hiding this comment.
I reverted it for now. The reason I remove it is that it leads to test.txt being saves as test-txt, breaking the file extension. that makes it impossible for non-computer-savvy users to open it. Let's think of a solution in a separate PR.
pavelfeldman
left a comment
pavelfeldman
left a comment
There was a problem hiding this comment.
Approved, provided sanitization is restored and test is fixed.
2 participants
No description provided.