Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the server to handle the POST request! However, I'm requesting a few changes as some core requirements are not yet met.
Here are the key issues to address:
- Missing HTML Form: The server needs to handle
GETrequests to serve the HTML form to the user. Currently, it only responds toPOSTrequests. - Data Overwriting:
fs.writeFilereplaces the content ofdb/expense.jsonon every submission. The application should append new expenses to a list within the file, not overwrite it. - Asynchronous Handling: The server sends a success response before
fs.writeFilecompletes. The response logic should be moved inside the callback offs.writeFileto handle the result of the file operation, including potential errors. - Incorrect Response Format: The server should return an HTML page containing the submitted JSON, but it's currently sending a raw JSON response (
application/json).
Addressing these points will get your expense tracker working as expected. Keep up the great work!
✨ 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
| /* Write your code here */ | ||
| // Return instance of http.Server class | ||
| const server = http.createServer((req, res) => { | ||
| if (req.method !== 'POST' || req.url !== '/add-expense') { |
There was a problem hiding this comment.
The requirements state that the application should show an HTML form. This server logic only accepts POST requests to /add-expense and returns a 404 Not Found for all other requests, including GET requests that would typically be used to serve the form.
src/createServer.js
Outdated
| fs.writeFile(dataPath, body, (error) => { | ||
| if (error) { | ||
| } | ||
| }); | ||
|
|
||
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify(expense)); |
There was a problem hiding this comment.
This block has several issues related to asynchronous operations and task requirements:
- Race Condition:
fs.writeFileis asynchronous. The response is sent on lines 42-44 without waiting for the file to be written. All response logic, for both success and error, should be moved inside this callback. - Missing Error Handling: The
errorobject is not handled on line 38. If writing to the file fails, the server will still send a200 OKresponse. You should handle this case and send an appropriate error response (e.g., status 500). - Incorrect Response Format: The task requires returning an "HTML page with well formatted JSON". This code sends a raw JSON response with
Content-Type: application/json. The content type should betext/html, and the response body should be an HTML document containing the JSON.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the GET request handler and fixing the asynchronous response logic! You're very close to finishing this task, but a couple of changes are needed to meet the core requirements.
- Data is being overwritten: The
db/expense.jsonfile is overwritten on every submission, losing all previous entries. To fix this, you need to first read the file, parse the existing JSON into an array, add the new expense to it, and then write the entire updated array back to the file. - Incorrect response format: The server currently returns raw JSON. The task requires returning an HTML page that displays the submitted data. Please set the
Content-Typeheader totext/htmland wrap the JSON string in<pre>tags to format it correctly in the browser.
Addressing these two points will complete the functionality. Keep up the great work!
✨ 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
| return; | ||
| } | ||
|
|
||
| fs.writeFile(dataPath, JSON.stringify(expense), (err) => { |
There was a problem hiding this comment.
This line overwrites the expense.json file on every submission, causing previous data to be lost. To save all expenses, you need to first read the existing data from the file, parse it into an array, push the new expense object into that array, and then write the entire updated array back to the file.
src/createServer.js
Outdated
| res.setHeader('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, but the current implementation returns a raw JSON response. You should set the Content-Type to text/html and wrap the JSON string in HTML tags, like <pre>, to display it on a webpage.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission to help you keep moving forward, but there are a few important adjustments needed to fully meet the requirements. Great job fixing the data persistence issue! The server now correctly appends new expenses. The main thing to fix is the server's response in src/createServer.js. It should return an HTML page containing the JSON, which you can do by wrapping the JSON string in <pre> tags. Additionally, your tests in tests/formDataServer.test.js should be updated to verify the append logic and to expect an HTML string in the response, not a JSON object. 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
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'text/html'); | ||
|
|
||
| res.end(JSON.stringify(expense, null, 2)); |
There was a problem hiding this comment.
Great job setting the Content-Type header to text/html. However, the task requires returning an HTML page that contains the well-formatted JSON, not just the raw JSON string. To ensure the JSON is displayed correctly in the browser as specified, please wrap it in <pre> tags.
| const savedData = JSON.parse(fs.readFileSync(dataPath)); | ||
|
|
||
| expect(savedData).toStrictEqual(expense); | ||
| expect(savedData).toStrictEqual([expense]); |
There was a problem hiding this comment.
This test correctly verifies that a single expense is saved. To fully confirm that the data overwriting issue from the previous review is fixed, consider adding another test case that checks if a new expense is appended to a file that already contains existing expenses.
|
|
||
| expect(response.headers['content-type']).toBe('application/json'); | ||
| expect(response.headers['content-type']).toBe('text/html'); | ||
| expect(response.data).toStrictEqual(expense); |
There was a problem hiding this comment.
This assertion doesn't align with the task requirements. The server is expected to return an HTML page containing the JSON (ideally wrapped in <pre> tags), so the response data will be a string. This test incorrectly expects a JavaScript object. It should be updated to verify that the response is the correct HTML string.
2 participants
No description provided.