Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the Node.js server and handling the form data submission correctly! Your code successfully saves the expenses to the JSON file. However, a few changes are needed to meet all requirements. The main issue is that the server responds to the POST request with raw JSON instead of an HTML page. The task requires returning an HTML page that displays the saved data. To fix this, please set the Content-Type header to text/html and wrap the JSON string in basic HTML tags, like <html><body><pre>...</pre></body></html>. Also, for client-side errors like missing form fields, please use the 400 Bad Request status code, as 500 Internal Server Error is meant for server-side failures. After these adjustments, your solution will be complete!
✨ 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/createServer.js
Outdated
| const BASE_PUBLIC_DIR = path.resolve('public'); | ||
|
|
||
| function sendBadRequest(res) { | ||
| res.statusCode = 500; |
There was a problem hiding this comment.
The HTTP status code 500 is generally used for "Internal Server Error". For client-side errors like a malformed request or missing data, the 400 Bad Request status code is more appropriate and conventional.
| } | ||
|
|
||
| if (req.method === 'POST' && req.url === '/add-expense') { | ||
| res.setHeader('Content-type', 'application/json'); |
There was a problem hiding this comment.
The task requires returning an HTML page, but the Content-Type is set to application/json. This header should be text/html to indicate that the response body is an HTML document.
src/createServer.js
Outdated
|
|
||
| writeStream.write(JSON.stringify(preparedData), () => { | ||
| writeStream.end(); | ||
| res.end(JSON.stringify(preparedData)); |
There was a problem hiding this comment.
According to the requirements, the server should return an HTML page that contains the well-formatted JSON. This line sends only the raw JSON string as the response. You should wrap the stringified JSON data in HTML tags, for example, <html><body><pre>${JSON.stringify(preparedData, null, 2)}</pre></body></html>.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a core requirement and a test are currently failing. In src/createServer.js, the POST /add-expense response sets Content-Type: text/html but sends raw JSON; the spec requires returning an HTML page that displays the saved data as pretty-printed JSON, and the test expects a consistent, correct header. Please wrap the JSON in minimal HTML (e.g., <html><body><pre>{pretty JSON}</pre></body></html>) and set the header appropriately before res.end (e.g., text/html). Additionally, guard the header check to avoid runtime errors: use const ct = req.headers['content-type'] || '' before ct.includes('application/json'). Finally, handle file write errors: either switch to fs.writeFile(..., cb) and send a 500 on error or attach an 'error' listener to the write stream and respond safely. Minor: standardize header casing to Content-Type. Good job on routing, validation, and static serving—once these blockers are addressed, you should be all set.
✨ 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/createServer.js
Outdated
| res.setHeader('Content-type', 'application/json'); | ||
|
|
||
| const chunks = []; | ||
| let rawData = ''; | ||
|
|
||
| req.on('data', (chunk) => { | ||
| chunks.push(chunk); | ||
| }); | ||
|
|
||
| req.on('end', () => { | ||
| rawData = Buffer.concat(chunks).toString(); | ||
|
|
||
| let preparedData = ''; | ||
|
|
||
| // extra logic: in tests cases requests are in application/json, | ||
| // but IRL, formdata usually comes as application/x-form-encoded | ||
| // hope that MA guys will re-write this task (as well as previous one) | ||
| if (req.headers['content-type'].includes('application/json')) { | ||
| preparedData = JSON.parse(rawData); | ||
| } else { | ||
| preparedData = handleRawFormData(rawData, res); | ||
| } | ||
|
|
||
| if (!preparedData.date || !preparedData.title || !preparedData.amount) { | ||
| sendBadRequest(res); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const writeStream = fs.createWriteStream( | ||
| path.resolve('db/expense.json'), | ||
| ); | ||
|
|
||
| writeStream.write(JSON.stringify(preparedData), () => { | ||
| writeStream.end(); | ||
| res.end(JSON.stringify(preparedData)); |
There was a problem hiding this comment.
You set the response header to HTML, but here the code sends raw JSON. The task requires returning an HTML page showing the saved data. Wrap the pretty-printed JSON in basic HTML (for example using a <pre> block) before calling res.end so the browser shows formatted JSON.
src/createServer.js
Outdated
| // extra logic: in tests cases requests are in application/json, | ||
| // but IRL, formdata usually comes as application/x-form-encoded | ||
| // hope that MA guys will re-write this task (as well as previous one) | ||
| if (req.headers['content-type'].includes('application/json')) { |
There was a problem hiding this comment.
This line calls .includes() on req.headers['content-type'] which will throw if the header is missing. Guard the header first, e.g. const ct = req.headers['content-type'] || '' and then check ct.includes('application/json').
src/createServer.js
Outdated
| const writeStream = fs.createWriteStream( | ||
| path.resolve('db/expense.json'), | ||
| ); | ||
|
|
||
| writeStream.write(JSON.stringify(preparedData), () => { | ||
| writeStream.end(); | ||
| res.end(JSON.stringify(preparedData)); | ||
| }); |
There was a problem hiding this comment.
You create and write to a write stream but do not handle write errors. If the filesystem write fails the response may hang or the server may crash. Consider using fs.writeFile with a callback (and respond 500 on error) or listen for the writeStream 'error' event and handle it before ending the response.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you progress, but there are a few key requirements from the previous review that still need your attention.
Great job adding the check for the content-type header to make the server more robust! However, the server must respond to the POST request with an HTML page, not raw JSON. Please remember to set the Content-Type header to text/html and wrap the saved data in a basic HTML structure, for example: <html><body><pre>YOUR_JSON_DATA</pre></body></html>.
Additionally, it's crucial to add error handling for the fs.writeFile operation. The callback function provides an error object as its first argument, which you should check to handle cases where the file can't be written.
You're very close to finishing this task. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| if (req.method === 'POST' && req.url === '/add-expense') { | ||
| res.setHeader('Content-type', 'application/json'); |
There was a problem hiding this comment.
According to the task requirements, the server should respond with an HTML page. Therefore, the Content-Type header for this response should be set to text/html.
| () => { | ||
| res.end(JSON.stringify(preparedData)); | ||
| }, |
There was a problem hiding this comment.
The callback for fs.writeFile receives an error object as its first argument. It's crucial to handle this case. If an error occurs during file writing, the server should send back an appropriate error response (e.g., status code 500).
| path.resolve('db/expense.json'), | ||
| JSON.stringify(preparedData), | ||
| () => { | ||
| res.end(JSON.stringify(preparedData)); |
There was a problem hiding this comment.
The task requires returning an HTML page that displays the saved data, but the current implementation sends a raw JSON string. The JSON data should be embedded within an HTML structure, for example: <html><body><pre>${JSON.stringify(preparedData, null, 2)}</pre></body></html>.
No description provided.