solution#365
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed before this solution can be considered complete, mainly around matching the exact HTTP behavior required by the spec. The /file/ route currently treats an empty relative path as a directory and returns a 404; instead, when the pathname is exactly /file/, you must map it to public/index.html so that /file/ serves the same file as /file/index.html. For any request whose pathname is exactly /file or does not start with /file/ (for example /anything), the server should return the specified hint message with Content-Type: text/plain instead of a 400 Bad request response. The duplicate-slash check is currently using rawUrl.includes('//'), which can falsely trigger on query strings; you should parse the URL, inspect only the pathname (e.g., urlPath), and check that for // as required. On successful file reads you set statusCode = 200 but do not set Content-Type; you need to detect the extension (e.g., via path.extname) and set text/html for .html files and text/css for .css files before res.end(data). In the decodeURIComponent error handler you correctly set status 400, but you must also set Content-Type: text/plain to satisfy the requirement that all error responses use plain text. Apart from these points, your use of path.resolve/path.relative to confine requests to public and your 404 handling are solid, so fixing these behaviors will bring your server fully in line with the task requirements.
✨ 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
| const server = http.createServer((req, res) => { | ||
| const rawUrl = req.url; | ||
|
|
||
| if (rawUrl.includes('//')) { |
There was a problem hiding this comment.
You check for duplicated slashes using rawUrl.includes('//'). The requirement says to check the pathname for // — do this after extracting urlPath (or check urlPath) so query strings won't cause false positives.
| const relativeRaw = urlPath.slice('/file/'.length); | ||
|
|
||
| let relative; | ||
|
|
||
| try { | ||
| relative = decodeURIComponent(relativeRaw); |
There was a problem hiding this comment.
When the request is /file/ the relativeRaw will be an empty string; per requirements you must return public/index.html. Right now the code will try to read the directory (and likely return 404). Detect an empty relative (or a trailing /) and use index.html as the target file.
| } catch { | ||
| res.statusCode = 400; | ||
| res.end('Bad request'); |
There was a problem hiding this comment.
In the decodeURIComponent catch you set res.statusCode = 400 but do not set a Content-Type header. All error responses must use text/plain, so set content-type: text/plain before res.end here.
| res.statusCode = 200; | ||
| res.end(data); |
There was a problem hiding this comment.
On successful file reads you set statusCode = 200 but never set Content-Type. The spec requires .html -> text/html and .css -> text/css. Determine the file extension (e.g. with path.extname(filePath)) and set the appropriate content-type header before res.end(data).
| if (urlPath !== '/file') { | ||
| res.statusCode = 400; | ||
| res.setHeader('content-type', 'text/plain'); | ||
| res.end('Bad request'); | ||
|
|
||
| return; |
There was a problem hiding this comment.
This branch handles requests not equal to /file by returning 400 Bad request. The specification requires that requests whose pathname does not start with /file/ (for example /anything) return a hint message (with text/plain). Adjust the logic so non-/file/ paths return the hint message, and keep /file itself returning the hint as implemented below.
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 functional gap in how non-/file/ paths are handled, which breaks the specified behavior. In src/createServer.js, after you parse the URL and handle duplicate slashes, the branch that currently returns a 400 Bad Request for any pathname that is not exactly /file should instead return the hint message with status 200 and Content-Type: text/plain (e.g., for /anything, /foo, etc.), while keeping the existing behavior that /file itself returns the same hint. Concretely, only paths that start with /file/ should be routed into the file-serving logic; everything else (including /anything) should short-circuit to the hint response. This change is required to satisfy the task requirements that all non-/file/ paths (and exactly /file) return the instructional hint text rather than an error. Additionally, the path.extname(urlPath) check located after the /file branch is redundant (it always evaluates to false for /file and never affects real file requests) and can be safely removed to simplify the control flow. Once you adjust the routing logic for non-/file/ paths and clean up that unused extension check, your server will align with the expected behavior and should pass the remaining tests.
✨ 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
|
|
||
| if (urlPath !== '/file') { | ||
| res.statusCode = 400; | ||
| res.setHeader('content-type', 'text/plain'); | ||
| res.end('Bad request'); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
This block returns a 400 for any request whose pathname is not exactly /file. According to the spec, any pathname that does NOT start with /file/ (for example /anything) should return the hint message with Content-Type: text/plain (not a 400). You should change the logic so that only requests starting with /file/ go to the file-serving branch; all other paths (including /anything) should return the hint. Keep the existing behavior for /file which should also return the hint.
|
|
||
| if (path.extname(urlPath)) { | ||
| res.statusCode = 400; | ||
| res.setHeader('content-type', 'text/plain'); | ||
| res.end('Bad request'); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
This check for path.extname(urlPath) runs only when urlPath === '/file' (due to the previous condition) and will always be false for /file. It's redundant and can be removed to simplify the control flow. Alternatively, if you intended additional validation, consider adjusting its placement or condition so it affects relevant requests only.
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 continue progressing while you practice server routing and HTTP behavior.
You did a solid job implementing the /file/ file-serving behavior: files are read from the public folder only, path traversal like ../ is blocked, duplicate slashes correctly return 404, missing files return 404 with text/plain, and .html/.css files are sent with the right Content-Type.
The main critical issue is in src/createServer.js near the top of your request handler, where the condition that checks the pathname causes any path that is not exactly /file (for example /anything or /foo) to return 400 Bad Request instead of the required hint message with status 200 and Content-Type: text/plain [CHECKLIST ITEM #5].
To fix this, adjust the routing so that only paths starting with /file/ go into the file-serving branch (for example by checking something like pathname.startsWith('/file/')), while all other paths — including the exact /file path and any non-/file/ paths — immediately return the hint text with a 200 response.
As a minor improvement, you could centralize the hint response in one place so every non-/file/ request uses the same logic, and consider adding a default Content-Type for other file types if that fits your goals.
Overall, your handling of edge cases and HTTP status codes shows good attention to detail, and with the routing fix in place this server will closely match the requirements.
Keep up the good work—this is a strong foundation, and refining the routing logic is a great next step in your learning.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (urlPath !== '/file') { | ||
| res.statusCode = 400; | ||
| res.setHeader('content-type', 'text/plain'); | ||
| res.end('Bad request'); | ||
|
|
||
| return; |
There was a problem hiding this comment.
This block returns 400 Bad Request for any pathname that is not exactly /file. That makes requests like /anything return 400, which violates the requirements — non-/file/ paths (and /file) should return the hint message with status 200 and Content-Type: text/plain. Change the logic so only paths that start with /file/ go into the file-serving branch; everything else should short-circuit to the hint response.
2 participants
No description provided.