Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical mismatch between the required output filenames and your current implementation, which will cause tests and expected behavior to fail. In src/createServer.js, the compressors/extension mapping currently uses .gzip and .deflate; per the requirements, gzip must produce files ending in .gz and deflate must produce files ending in .dfl (for example, file.txt → file.txt.gz), while brotli correctly remains .br — this violates [CHECKLIST ITEM #15] and breaks the expected output naming. Please update the mapping so that gzip → .gz, deflate → .dfl, and br → .br, and ensure the Content-Disposition filename uses these exact extensions.
Additionally, in src/createServer.js where you use pipeline(...) to stream the compressed data, the callback is currently empty, so any stream or compression errors are silently ignored; this can leave the client hanging without a clear error response. Update the pipeline callback to check the err parameter and handle failures explicitly (for example, log the error and send a 500 response or at least destroy/end the response) so that failed compression or streaming results in a proper outcome instead of a silent failure. The HTML form in src/index.html and the overall streaming/zlib setup are otherwise well done and correctly wired to /compress, so once you fix the extension mapping and add proper pipeline error handling, your solution should fully meet the task requirements. Keep going — you’re very close; these adjustments will make your implementation both correct and more robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| gzip: { ext: '.gzip', stream: zlib.createGzip }, | ||
| deflate: { ext: '.deflate', stream: zlib.createDeflate }, | ||
| br: { ext: '.br', stream: zlib.createBrotliCompress }, |
There was a problem hiding this comment.
The extension values in the compressors mapping are wrong. The task requires gzip → .gz and deflate → .dfl (only br should be .br). Using .gzip and .deflate will produce incorrect filenames and fail checklist item #15.
src/createServer.js
Outdated
| fs.createReadStream(file.filepath), | ||
| config.stream(), | ||
| res, | ||
| () => {}, |
There was a problem hiding this comment.
The pipeline callback is empty so stream errors are ignored. Check the callback's err parameter and handle errors (for example, send a 500 response or end the response) to avoid silent failures and ensure the client gets an appropriate response if compression/streaming fails.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical mismatch with the required download filename format, which can cause tests to fail. In src/createServer.js, the Content-Disposition header currently builds the filename with .${compressionType}, producing names like file.txt.gzip and file.txt.deflate instead of the required file.txt.gz, file.txt.dfl, or file.txt.br. The assignment explicitly requires those exact extensions, and your own compressors configuration already defines the correct ext values, so this is considered a blocking functional issue. To fix it, update the header to use the configured extension and quote the filename, for example: attachment; filename="${file.originalFilename}${config.ext}". Everything else (HTML form fields, routing, status codes, streaming, and zlib usage) looks solid and aligned with the requirements. Once you adjust the filename construction as described, your solution should fully meet the task expectations.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine the remaining details. You’ve done a solid job overall: the HTML form in src/index.html correctly posts to /compress with the right file and compressionType fields, and your server in src/createServer.js uses streams, zlib, and proper status codes for success and error cases. The main functional gap is in src/createServer.js where the compressors map still uses .gzip and .deflate extensions instead of the required .gz and .dfl, so the downloaded filenames won’t match the spec and tests; update those ext values accordingly. Related to this, your Content-Disposition header builds the attachment name without quotes; per the requirement, you should format it as attachment; filename="${file.originalFilename}${config.ext}" so clients see exactly file.txt.gz, file.txt.dfl, or file.txt.br even if there are spaces in the base filename. Once you adjust the extensions and quote the filename, your implementation will closely match the intended behavior and should satisfy the automated checks. Overall, you’re very close—nice work on the streaming logic and error handling; just tighten up the filename details to reach a fully polished solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| const compressors = { | ||
| gzip: { ext: '.gzip', stream: zlib.createGzip }, |
There was a problem hiding this comment.
The compressors mapping uses .gzip and .deflate as extensions. The task requires exact extensions .gz for gzip and .dfl for deflate (keep .br for brotli). Update the ext values accordingly so generated filenames match the requirement.
|
|
||
| res.writeHead(200, { | ||
| 'Content-Type': 'application/octet-stream', | ||
| 'Content-Disposition': `attachment; filename=${file.originalFilename}${config.ext}`, |
There was a problem hiding this comment.
The Content-Disposition header currently builds the filename without quotes and relies on config.ext (which needs fixing above). Quote the filename and use the configured extension, for example: attachment; filename="${file.originalFilename}${config.ext}" so the download filename is exactly file.txt.gz / file.txt.dfl / file.txt.br.
2 participants
No description provided.