Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your submission is well-structured with good security checks, which is great to see. However, changes are needed because the server doesn't handle a few key URL paths correctly, which is a core part of the task.
Specifically, in src/validate.js:
- Requests to
/file/should serveindex.html, but your current logic causes a 404 error. This is because the path replacement results in'/', which isn't being mapped to the default file. - The hint message is required for any path that doesn't start with
/file/. Your code incorrectly returns a 400 'Bad Request' for paths like/index.htmlinstead of showing the hint.
Please revise the path validation logic to address these two cases. You're very close to a complete solution!
✨ 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
src/validate.js
Outdated
| if ( | ||
| !normalizedUrl.pathname.startsWith('/file/') && | ||
| /\.[a-z0-9]+$/i.test(normalizedUrl.pathname) | ||
| ) { | ||
| return { | ||
| status: 400, | ||
| data: 'Bad Request', | ||
| }; | ||
| } |
There was a problem hiding this comment.
This block of code prevents the server from sending the required hint message for certain paths. The task requires that any path not starting with /file/ should trigger the hint. This check incorrectly returns a 400 'Bad Request' for paths like /index.html, instead of letting the logic on line 38 handle it.
src/validate.js
Outdated
| }; | ||
| } | ||
|
|
||
| const filePath = normalizedUrl.pathname.replace('/file', '') || 'index.html'; |
There was a problem hiding this comment.
This logic doesn't correctly handle requests for /file/. The expression '/file/'.replace('/file', '') evaluates to '/', which is not a falsy value, so the || 'index.html' part is skipped. The code then attempts to read a directory, which fails and results in a 404 error instead of serving public/index.html as required.
src/createServer.js
Outdated
| /* Write your code here */ | ||
| // Return instance of http.Server class | ||
| return http.createServer((req, res) => { | ||
| const validated = v.validate(req.url); |
There was a problem hiding this comment.
The imported validate function doesn't handle all required paths correctly. A request to /file/ should serve index.html, but the current logic in validate.js causes this request to fail with a 404 error.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! The server is much more robust now. However, a couple of changes are still needed as two core requirements for path handling are not yet met.
- Paths that do not start with
/file/(like/index.html) are returning a 400 "Bad Request" instead of the required hint message. Please review the validation logic insrc/validate.jsto ensure the hint is always shown for these cases. - A request to
/file(without the trailing slash) is incorrectly returning the hint message. It should servepublic/index.html, just like/file/does.
You are very close to a complete solution! Once these two validation conditions are adjusted, everything should work as expected.
✨ 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
Excellent progress on this! I'm approving your solution as an educational override so you can continue moving forward, but there are a couple of key issues to review.
Your server is very close, but it currently doesn't handle the /file/ route (with a trailing slash) correctly—it should serve index.html. Also, paths that start with /file but are not valid file routes (e.g., /filesomething) should return a hint message, but they are currently resulting in a 404. Strengthening your path validation logic will fix this.
To catch these kinds of bugs, your test suite could be improved by adding tests for the /file/ route and for invalid paths that should trigger the hint. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.