Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement of the task is not yet met and this affects the main functionality shown to the user. Your server currently responds to POST /add-expense with Content-Type: application/json and raw JSON; the specification requires returning an HTML page (text/html; charset=utf-8) that displays the expense as pretty-printed JSON inside something like a <pre> block, e.g. using JSON.stringify(expense, null, 2) (this is a critical functional mismatch).
Critical issues to fix:
- In
src/createServer.js, update the response for successfulPOST /add-expenseso it sends HTML that embeds the formatted JSON instead of raw JSON with anapplication/jsoncontent type. - Make the JSON detection more robust by changing the content-type check (near where you call
JSON.parse) to something likecontentType && contentType.startsWith('application/json')so that values likeapplication/json; charset=utf-8are correctly treated as JSON. - Ensure
public/index.htmlexists and contains a form thatPOSTs to/add-expensewith fields nameddate,title, andamount, so the front-end requirement is clearly satisfied.
Minor but helpful improvement:
- When writing to
db/expense.json, consider usingJSON.stringify(expense, null, 2)so the stored file is human-readable.
You’ve already done solid work wiring up the Node.js HTTP server, routing, validation, and file writing; once you adjust the response to be HTML, improve the content-type check, and verify the form, your solution will align well with all the 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 querystring = require('querystring'); | ||
|
|
||
| const dataPath = path.resolve(__dirname, '..', 'db', 'expense.json'); | ||
| const htmlPath = path.resolve(__dirname, '..', 'public', 'index.html'); |
There was a problem hiding this comment.
The server expects to serve public/index.html so the app can show the HTML form. Ensure public/index.html exists and contains an HTML form with date, title, and amount fields that POSTs to /add-expense (this is required by the task).
src/createServer.js
Outdated
| try { | ||
| const contentType = req.headers['content-type']; | ||
| const expense = | ||
| contentType === 'application/json' |
There was a problem hiding this comment.
Comparing contentType === 'application/json' is brittle because headers often include a charset (for example application/json; charset=utf-8). Use a safer check like contentType && contentType.startsWith('application/json') before JSON.parse to avoid mis-detection.
src/createServer.js
Outdated
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
You currently respond with Content-Type: application/json and raw JSON. The requirement is to return an HTML page containing well-formatted JSON. Change the response to text/html; charset=utf-8 and include the pretty-printed JSON (for example JSON.stringify(expense, null, 2) inside a <pre> block) so the browser shows formatted JSON inside an HTML page.
src/createServer.js
Outdated
| return res.end('Missing fields'); | ||
| } | ||
|
|
||
| fs.writeFileSync(dataPath, JSON.stringify(expense)); |
There was a problem hiding this comment.
You write compact JSON with JSON.stringify(expense). Consider writing pretty JSON to disk using JSON.stringify(expense, null, 2) so db/expense.json is human-readable (not strictly required but helpful).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two core requirements of the task are not yet fully met, which will prevent the app from behaving as specified.
Critical issues that must be fixed (blockers):
- In
src/createServer.js, when a POST/add-expenserequest is detected as JSON (isJson === true), you currently respond withContent-Type: application/jsonand raw JSON. The specification requires that all successful submissions return an HTML page that shows the pretty-printed JSON, so this branch needs to sendContent-Type: 'text/html; charset=utf-8'and embedprettyJsoninside a<pre>(similar to your form-encoded branch). - The server’s GET handler serves
public/index.html, but that file is missing from the project snapshot. You need to addpublic/index.htmlcontaining a form like<form method="POST" action="/add-expense">with inputs nameddate,title, andamount, so that users can submit expenses through the browser.
What you did well and minor suggestions:
- Writing
db/expense.jsonusingJSON.stringify(expense, null, 2)is correct and satisfies the “human-readable JSON file” requirement; keep that as is. - As a future improvement (not blocking), consider refining validation so that
0is allowed as an amount and using asyncfs.writeFileinstead ofwriteFileSyncto avoid blocking the event loop.
Once you update the JSON response to return HTML and add the public/index.html form with the required fields, your solution will align well 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
src/createServer.js
Outdated
| if (isJson) { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
When the request is detected as JSON (isJson true) the handler currently responds with Content-Type: application/json and raw JSON. The specification requires returning an HTML page with well formatted JSON. Change this branch so it returns an HTML page (set Content-Type: 'text/html; charset=utf-8') and embed prettyJson inside a <pre> so the JSON is displayed pretty-printed to the user.
| if (req.method === 'GET' && req.url === '/') { | ||
| fs.readFile(htmlPath, (err, data) => { | ||
| if (err) { | ||
| res.writeHead(500); | ||
|
|
||
| return res.end('Error loading index.html'); | ||
| } | ||
| res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); | ||
| res.end(data); | ||
| }); |
There was a problem hiding this comment.
The GET handler serves public/index.html. I couldn't find public/index.html in the project snapshot; add this file. It must contain a form that POSTs to /add-expense and include inputs named date, title, and amount (for example: <form method="POST" action="/add-expense"> with name="date", name="title", name="amount"). Without it the GET / route and the form requirement will fail.
| const prettyJson = JSON.stringify(expense, null, 2); | ||
|
|
||
| fs.writeFileSync(dataPath, prettyJson); |
There was a problem hiding this comment.
Good: you already create prettyJson with JSON.stringify(expense, null, 2) and write it to db/expense.json, which makes the stored file human-readable as required. Keep this behavior.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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 a few remaining details. You’ve done a solid job implementing the Node.js server: it correctly serves an HTML page, handles POST /add-expense for both form and JSON bodies, and writes pretty-printed JSON to db/expense.json.
There are a couple of important improvements to make next:
- For JSON POST requests in
src/createServer.js, whenisJsonRequestis true you should respond withContent-Type: text/html; charset=utf-8and an HTML page that embedsprettyJsoninside a<pre>element, matching the form-encoded branch instead of returning raw JSON. - Ensure
public/index.htmlactually exists and contains a<form method="POST" action="/add-expense">with inputs nameddate,title, andamount, so browser submissions work correctly. - Update your validation so that
0is accepted as a validamount(e.g., checkexpense.amount == null || expense.amount === ''instead of a generic falsy check), and consider usingfs.writeFileinstead offs.writeFileSyncto avoid blocking the event loop.
Overall, your structure and main flow are correct; refining these details will bring your solution fully in line with the requirements and help you build good habits for production-ready Node.js code.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (isJsonRequest) { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
Blocking: when isJsonRequest is true you send Content-Type: application/json and raw JSON. The task requires returning an HTML page that shows the pretty-printed JSON inside a <pre> element (same as the form-encoded branch). Change this branch to send Content-Type: 'text/html; charset=utf-8' and return an HTML document embedding prettyJson.
| fs.readFile(htmlPath, (err, data) => { | ||
| if (err) { | ||
| res.writeHead(500); | ||
|
|
||
| return res.end('Error loading index.html'); | ||
| } | ||
| res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); | ||
| res.end(data); |
There was a problem hiding this comment.
Ensure the GET handler's file exists: you read and serve public/index.html here — make sure that file is present in the project and contains a <form method="POST" action="/add-expense"> with inputs named date, title, and amount, otherwise browser submissions won't work.
| ? JSON.parse(body) | ||
| : querystring.parse(body); | ||
|
|
||
| if (!expense.date || !expense.title || !expense.amount) { |
There was a problem hiding this comment.
Validation issue: this check treats falsy expense.amount as missing, which will reject 0 or "0. If 0is a valid amount, change the check to test for null/empty (for example:expense.amount == null || expense.amount === ''`) instead of a simple falsy check.
|
|
||
| const prettyJson = JSON.stringify(expense, null, 2); | ||
|
|
||
| fs.writeFileSync(dataPath, prettyJson); |
There was a problem hiding this comment.
Suggestion: you use fs.writeFileSync to save the JSON which blocks the event loop. While functional, prefer fs.writeFile (async) to avoid blocking, e.g. fs.writeFile(dataPath, prettyJson, err => { ... }).
No description provided.