Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on setting up the Node.js server and the HTML form. While the GET request works correctly, the POST request handling has a few critical issues that need to be addressed before this can be approved.
Here are the key areas to revise:
- Incorrect Data Parsing: Your server expects JSON, but the HTML form sends data as
application/x-www-form-urlencoded, which causesJSON.parse()to fail. Consider usingURLSearchParamsto correctly parse the request body insrc/createServer.js. - Data Overwriting: The
expense.jsonfile is overwritten with each new submission, losing all previous data. To fix this, you should read the existing data from the file, add the new expense to an array, and then write the entire array back. - Incorrect Response Format: The server should return an HTML page containing the formatted JSON, not a raw JSON response. Please set the
Content-Typetotext/htmland wrap the JSON in an HTML element like<pre>.
Addressing these points will get the core functionality working as required. You're on the right track!
✨ 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
| let expense; | ||
|
|
||
| try { | ||
| expense = JSON.parse(body); |
There was a problem hiding this comment.
A standard HTML form submission sends data in application/x-www-form-urlencoded format (e.g., key1=value1&key2=value2), not JSON. JSON.parse() will fail with this format. You should parse the URL-encoded string instead. Consider using the built-in URLSearchParams class to handle the body string.
|
|
||
| const dbPath = path.resolve('db', 'expense.json'); | ||
|
|
||
| fs.writeFileSync(dbPath, JSON.stringify(expense, null, 2), 'utf-8'); |
There was a problem hiding this comment.
This implementation overwrites expense.json with the new expense on every submission, so only the last submitted expense is stored. To save a history of expenses, you should read the existing data from the file, add the new expense to a list (array), and then write the entire updated list back to the file.
src/createServer.js
Outdated
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
The task requires returning an 'HTML page with well formatted JSON'. Currently, you are returning a raw JSON response (Content-Type: application/json). The response should be an HTML document (Content-Type: text/html) that contains the formatted JSON, for example, inside a <pre> tag.
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/add-expense" method="POST"> |
There was a problem hiding this comment.
A standard HTML form submission sends data as application/x-www-form-urlencoded. However, the server is expecting the data in application/json format, as it uses JSON.parse() to process the request body. You'll need to use JavaScript to intercept the form's submit event, create a JSON object from the form data, and send it using the fetch API.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this pull request! You've successfully fixed the data parsing and response format issues. I am requesting changes for one critical issue that remains.
Currently, your code in src/createServer.js overwrites the db/expense.json file with each new submission. This means previous expense entries are lost, which goes against a core requirement to save all submitted data.
To resolve this, you need to modify the logic to first read the contents of the file, parse the JSON into an array, add the new expense to it, and then write the entire updated array back to the file. This will ensure all expenses are preserved.
✨ 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
| // NOTE: | ||
| // We intentionally overwrite expense.json on each request | ||
| // because task tests expect the file to contain a single expense object. | ||
| // In a real application we would store an array of expenses. | ||
| fs.writeFileSync(dbPath, JSON.stringify(expense, null, 2), 'utf-8'); |
There was a problem hiding this comment.
This line overwrites the expense.json file on every submission, which goes against a core requirement of the task. Instead of overwriting, you need to preserve the history of expenses.
To fix this, you should:
- Read the contents of
db/expense.jsonbefore writing to it. - Parse the contents into an array. If the file is empty or doesn't exist, you can start with an empty array
[]. - Add the new
expenseobject to this array. - Write the entire updated array back to the file.
Finally, make sure the JSON displayed on the response page is the full array of expenses, not just the single new one.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the HTML form and server setup! I am approving this submission as an educational override to help you move forward. The critical issue to fix is that db/expense.json is being overwritten on each submission. To preserve all entries, you need to read the file, parse the JSON into an array, add the new expense, and then write the entire array back. Also, ensure db/expense.json starts as an empty array ([]). You're very close to a complete solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| // because task tests expect the file to contain | ||
| // a single expense object. | ||
| // In a real application we would store an array of expenses. | ||
| fs.writeFileSync(dbPath, JSON.stringify(expense, null, 2), 'utf-8'); |
There was a problem hiding this comment.
This implementation still overwrites the expense.json file with each new submission, which goes against the core requirement to preserve all previous entries. The correct logic is to read the file first, parse its JSON content into an array, add the new expense to that array, and then write the entire updated array back to the file.
| { | ||
| "date": "2024-01-25", | ||
| "title": "Test Expense", | ||
| "amount": "100" | ||
| } No newline at end of file |
There was a problem hiding this comment.
To support saving multiple expenses, the root of this JSON file should be an array. For example, it could start as an empty array []. This will allow your server logic to read the file, parse it into an array, add new expenses, and write the full array back, preserving all entries.
There was a problem hiding this comment.
But tests doesn't pass with it
2 participants
Form data (with Node.js)