Solution#370
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 one required route behavior is currently broken and will cause tests to fail, even though many parts of your server are implemented well.
Critical issues to fix:
- In
src/createServer.js, in the branch that handles paths starting with/file/, a request to/file/makesrelativePathan empty string and leads tofs.readFile('./public/'), which is a directory read and not the required behavior. The spec states that/file/must servepublic/index.html, so update this branch so that whenrelativePathis empty (orreq.url === '/file/'), you instead setrelativePath = 'index.html'before reading from disk. - In the part of
src/createServer.jsthat handles paths not starting with/file/, you currently setres.statusCode = 400before sending the hint message. The requirements only ask for a hint message intext/plain, and typical expected behavior (and likely the tests) use a 200 status here, so adjust this branch to respond with status 200 while keeping the same hint text.
Important but less critical improvement:
3. In your 404 handling (inside the fs.readFile error callback), you send error.message back to the client, which exposes internal details like ENOENT. The requirement only specifies a 404 with text/plain, so replace this with a consistent message such as File not found while keeping res.statusCode = 404 and the text/plain content type.
Overall your handling of ../ (400), // (404), content types for .html/.css, and the basic /file hint path looks solid; addressing the issues above will bring your solution fully in line with the specification and make the behavior more predictable and test-friendly.
✨ 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 relativePath = filePath.slice('/file/'.length); | ||
| let fileData; | ||
|
|
||
| try { | ||
| fileData = fs.readFileSync(`./public/${relativePath}`); |
There was a problem hiding this comment.
If the request is '/file/' the computed relativePath becomes an empty string and the code attempts to read ./public/ (a directory), causing an error. The spec requires that /file/ should return public/index.html. Add a special-case (or treat empty relativePath) to serve ./public/index.html.
| } else if (!filePath.startsWith('/file/')) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('path to file should start with /file/'); |
There was a problem hiding this comment.
The branch for requests that don't start with /file/ sets res.statusCode = 400. The requirements ask to return a hint message (text/plain) for such paths — typically with a 200 status. Change the status code here to return a hint response consistent with the spec/examples.
| } catch (error) { | ||
| res.statusCode = 404; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end(error.message); |
There was a problem hiding this comment.
On file-not-found you send error.message from the filesystem. That leaks an implementation-specific message (like ENOENT). The requirement only mandates a 404 with text/plain; consider returning a clearer, consistent message such as 'File not found'.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed because two behaviors in src/createServer.js still conflict with the task requirements and will cause tests to fail.
- Non-
/file/paths status code: In the branch where you handle requests whosepathnamedoes not start with/file/(theif (!pathname.startsWith('/file/'))block insrc/createServer.js), you currently setres.statusCode = 400while sending the correct hint text. Per the requirements, any non-/file/path must return the hint message with status 200 andContent-Type: text/plain. To fix this, keep the same hint body and headers, but change the status code from400to200in that branch. - Leaking internal error messages on file read errors: In your
try/catchfor reading files, thecatchblock setsres.statusCode = 404but sendserror.messageback to the client. The checklist expects a stable, non-internal message for missing files (e.g.File not found) with status 404 andContent-Type: text/plain. Update thecatchblock so that it always writes a fixed message likeFile not foundinstead oferror.message, keeping the status code and content-type as they are.
Aside from these points, your server correctly restricts access to public, blocks ../ and //, serves the right files for /file/ paths, and sets appropriate content types for .html and .css files, which is all solid work. As a minor, non-blocking improvement, you could consider setting relativePath = 'index.html' explicitly for /file/ and harmonizing your hint messages for consistency, but these are not required for passing. Once you adjust the status code for non-/file/ requests and replace error.message with a consistent 404 message, your solution should fully meet the specification.
✨ 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
| } else if (!filePath.startsWith('/file/')) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('path to file should start with /file/'); |
There was a problem hiding this comment.
The branch for paths that do not start with /file/ sets res.statusCode = 400. Per the task requirements (hint for non-/file/ path), this should respond with a hint message in text/plain and status 200 (not 400). Please change the status code to 200 while keeping the hint text.
| } catch (error) { | ||
| res.statusCode = 404; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end(error.message); |
There was a problem hiding this comment.
In the catch block you set res.statusCode = 404 but send error.message back to the client. Do not expose internal error text; replace the response body with a consistent message such as File not found and keep Content-Type: text/plain and status 404.
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 it.
You’ve implemented the core behavior very well: the server correctly serves files from ./public, blocks ../ and // in paths, returns public/index.html for /file/, and uses a stable text/plain 404 response for missing files. The main remaining issue is in src/createServer.js, in the branch that handles requests whose pathname does not start with /file/ (the if (!filePath.startsWith('/file/')) condition near the top of the handler): there you set res.statusCode = 400, but the requirement says those requests must return the hint message with status code 200 and Content-Type: text/plain. To fix this, keep sending the same hint body in that branch but change the status code from 400 to 200 so it matches the specification and the tests. As minor improvements, you could parse the URL with new URL(req.url, 'http://localhost') and use pathname for checks (to handle query strings correctly), and add a sensible default Content-Type for file types other than .html or .css. Overall, your solution demonstrates a solid grasp of routing and file serving; with that status code tweak and a couple of small refinements, it will be fully aligned with the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| return; | ||
| } else if (!filePath.startsWith('/file/')) { | ||
| res.statusCode = 400; |
There was a problem hiding this comment.
Per the requirements, any request whose pathname does NOT start with /file/ must return the hint message with status code 200 and Content-Type: text/plain. Here you set res.statusCode = 400. Change this to 200 so the response matches the spec and tests.
3 participants
No description provided.