Node form data solution#151
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your work! There is a critical issue: the application does not serve the required HTML form for users to enter expense data, which means users cannot interact with your app as intended. Please add the missing HTML form (either as a static file or generated in your server code) so users can submit expenses. Also, while using fs.writeFileSync is not a blocker for this review, consider switching to the asynchronous fs.writeFile for better performance in the future. Once you add the form, your solution will meet the core requirements. Keep going—you’re almost there! 🚀
✨ 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
| return; | ||
| } | ||
|
|
||
| fs.writeFileSync(expensePath, data); |
There was a problem hiding this comment.
Issue: Using fs.writeFileSync inside an asynchronous request handler can block the event loop and degrade server performance, especially under concurrent requests. Consider using fs.writeFile (the asynchronous version) with a callback to avoid blocking the server.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! 😊 There is a critical issue that needs to be addressed before your PR can be approved: in your /add-expense POST handler, fs.writeFile is used without a callback. This means file write errors are not handled, and the response may be sent before the data is actually saved, which can lead to data loss or inconsistent behavior. Please update your code to use a callback with fs.writeFile, handle any possible errors, and only send the response after the file operation completes. Once this is fixed, your solution will meet all the requirements! Keep going—you’re almost there! 🚀
✨ 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
| return; | ||
| } | ||
|
|
||
| fs.writeFile(expensePath, data); |
There was a problem hiding this comment.
Issue: fs.writeFile is called without a callback function. This means any errors during file writing will be ignored, and the response may be sent before the file is actually written. To ensure reliability and proper error handling, provide a callback to handle possible errors and only send the response after the file operation completes.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job! 🎉 Your solution now fully meets all the requirements for the task. You have correctly addressed the critical issue with file writing by switching from fs.writeFileSync (which does not accept a callback) to the asynchronous fs.writeFile with proper error handling. This ensures that your server only responds after the file operation completes and can gracefully handle any errors that may occur during the write process.
Your code successfully serves the HTML form, processes submissions, saves the data to the correct file (db/expense.json), and displays the saved data in a well-formatted HTML page. All requirements and constraints have been met, including using only Node.js core modules, handling form data correctly, and providing readable JSON output.
While your previous approach had a critical flaw, you have now resolved it. In the future, always ensure you use asynchronous file operations when you need to handle errors or perform actions after the operation completes. This is a key aspect of robust Node.js applications.
Keep up the good work and continue to review feedback carefully—addressing issues like this is an important part of learning and improving as a developer! If you want to learn more about asynchronous file handling in Node.js, check out the Node.js fs documentation.
Well done, and keep moving forward! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| fs.writeFileSync('db/expense.json', jsonData, () => { | ||
| res.statusCode = 500; | ||
| res.end('Failed to write expense.json'); | ||
| return; | ||
| }); |
There was a problem hiding this comment.
You are using fs.writeFileSync with a callback, but the synchronous version does not accept a callback and will throw on error. This is a critical issue: switch to using fs.writeFile (the async version) with a callback to properly handle file write errors and only send the response after the write completes. This is necessary to ensure data integrity and proper error handling as required by the task.
2 participants
No description provided.