Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the form handling and data saving logic! Your application correctly processes the requests and saves the data. However, one change is required to meet all the task's specifications. The server's response to a POST request currently sends raw JSON data, but the requirement is to return a full HTML page that displays this JSON. To fix this, you'll need to set the response Content-Type to text/html and wrap the JSON string within an HTML structure, for example, using <pre> tags. Once that's updated, your submission will be ready for approval!
✨ 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 asking for revisions because one core requirement of the task is not yet satisfied: the POST response must be a full HTML page with Content-Type: text/html that displays the formatted JSON payload. In src/createServer.js, within the POST handler (e.g., for /add-expense), you currently set Content-Type to application/json and call res.end(JSON.stringify(expense)), which returns raw JSON instead of an HTML document. You should change the response header to text/html and replace the raw JSON response with the existing commented-out HTML block that wraps the JSON string in a <pre> tag, ensuring the server sends a valid HTML page. For example, build an HTML string like <!DOCTYPE html>...<pre>${formattedJson}</pre>...</html> and send that as the body after setting res.setHeader('Content-Type', 'text/html'). As a minor improvement (not blocking), consider adding error handling around writing to db/expense.json so the server can respond gracefully if a file write fails. Overall, your form handling, validation, and file saving logic look solid—once you adjust the POST response to be proper HTML, the solution will align with the full set of 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
| fs.writeFileSync(filePath, JSON.stringify(expense, null, 2)); | ||
|
|
||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'application/json'); |
There was a problem hiding this comment.
The POST response sets Content-Type to 'application/json' here, but the task requires the POST response to be a full HTML page and the Content-Type to be 'text/html'. Change this header to 'text/html'.
| /* | ||
| res.setHeader('Content-Type', 'text/html'); | ||
| res.end(` | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <body> | ||
| <pre><code>${JSON.stringify(expense, null, 2)}</code></pre> | ||
| </body> | ||
| </html> | ||
| `); */ |
There was a problem hiding this comment.
There is a commented-out HTML block below that wraps the JSON in a
tag and matches the requirement to return an HTML page showing the formatted JSON. Uncomment and use this block (and ensure the header is set to 'text/html' before sending).
| </html> | ||
| `); */ | ||
|
|
||
| return res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
Do not return raw JSON with res.end(JSON.stringify(expense)) — replace this call with sending the full HTML response that contains the formatted JSON so the POST response is a valid HTML page as required.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved as an educational override to help you keep progressing while you address a couple of remaining issues. You’ve done a solid job setting up the Node.js server, handling both GET and POST requests, validating the form data, and writing the expense information to db/expense.json. The most important issue is in src/createServer.js inside the POST handler where you currently set Content-Type to application/json and call res.end(JSON.stringify(expense)); the requirement is to return a full HTML page, so you should change the header to text/html and send an HTML document that wraps JSON.stringify(expense, null, 2) in a <pre> tag instead of returning raw JSON. Specifically, you can uncomment and use the existing HTML block in that file (which already wraps the pretty-printed JSON in <pre>...</pre>) and remove the direct JSON response so that the browser receives proper HTML. A smaller improvement is around file writing: fs.writeFileSync is currently used without error handling, so consider wrapping it in a try/catch or switching to fs.writeFile with a callback/promise so you can send a 500 status and a simple HTML error page if saving to db/expense.json fails. With these adjustments, your server will better match the task’s requirements and be more robust, and you’re clearly on the right track—nice work so far.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| fs.writeFileSync(filePath, JSON.stringify(expense, null, 2)); | ||
|
|
||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'application/json'); |
There was a problem hiding this comment.
The handler sets the response Content-Type to 'application/json'. The task requires the POST response to be a full HTML page with 'Content-Type: text/html' that displays the formatted JSON. Change this header accordingly before sending the response.
|
|
||
| */ | ||
|
|
||
| return res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
This line returns raw JSON (res.end(JSON.stringify(expense))). Replace it with sending an HTML document that wraps the pretty-printed JSON in a
(for example, use the existing commented-out HTML block). The response must be a full HTML page, not raw JSON.
| /* | ||
|
|
||
| res.setHeader('Content-Type', 'text/html'); | ||
| res.end(` | ||
| <!DOCTYPE html> | ||
| <html> | ||
| <body> | ||
| <pre><code>${JSON.stringify(expense, null, 2)}</code></pre> | ||
| </body> | ||
| </html> | ||
| `); |
There was a problem hiding this comment.
There is an existing commented-out HTML block below which is exactly what you should send as the POST response (it wraps JSON.stringify(expense, null, 2) in a
). Uncomment and use that block (after ensuring the response header is set to 'text/html') and remove the raw JSON response.
|
|
||
| const filePath = path.join(__dirname, '../db/expense.json'); | ||
|
|
||
| fs.writeFileSync(filePath, JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
You write the expense file synchronously with fs.writeFileSync. Consider adding error handling (or use fs.writeFile with a callback/promise) so that file write failures do not crash the server and you can respond with an appropriate error status/message.
2 participants
No description provided.