Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because several critical blockers prevent the app from meeting the requirements and functioning end-to-end.
- Endpoint mismatches: In
src/createServer.jsthe POST route is/add-expense, butsrc/index.htmlposts to/submit-expense. Align these paths so submissions reach the handler (change one side to match the other). Similarly, the client fetches/db/expense.jsonwhile the server serves/expenses; make these consistent. - POST response format: After saving, the server returns
application/json. The requirement is to return an HTML page with pretty-printed JSON embedded (e.g.,<pre>${JSON.stringify(obj, null, 2)}</pre>) andContent-Type: text/html. - Data persistence: The server overwrites
db/expense.jsonwith a single object. Load the existing data as an array (fallback to[]), push the new expense, and write back withJSON.stringify(arr, null, 2)so previous entries aren’t lost and the client can render a list. - File system readiness: Ensure the
dbdirectory exists before writing (e.g.,fs.mkdirSync(path.dirname(DATA_PATH), { recursive: true })) to avoid runtime errors on first save.
Minor improvements: return an empty array when no data on GET, keep success/error responses as HTML for consistency, and re-test the full flow. You’ve got a solid foundation (serving index.html, parsing JSON); once these changes are in, you’ll meet the spec.
✨ 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 (req.method === 'POST' && req.url === '/add-expense') { |
There was a problem hiding this comment.
POST route is /add-expense here, but the client in src/index.html posts to /submit-expense. This mismatch will prevent the handler from receiving submissions. Make the endpoints consistent (either change the client to /add-expense or change this route to /submit-expense). See client code for the URL used: src/index.html.
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(obj)); |
There was a problem hiding this comment.
After successfully saving the expense the server responds with Content-Type application/json and raw JSON. The task requires returning an HTML page with well-formatted (pretty-printed) JSON embedded (for example inside a <pre>). Update this response to Content-Type: text/html and send an HTML document that contains JSON.stringify(obj, null, 2) inside a <pre> so it is human-readable.
| } | ||
|
|
||
|
|
||
| fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2)); |
There was a problem hiding this comment.
The code currently writes the new expense object directly to db/expense.json (overwriting it). The client expects an array of expenses when rendering (it calls res.json() and iterates/prints as an array). To support multiple expenses, read existing content (if any) into an array, push the new expense, then write the updated array back with JSON.stringify(array, null, 2). This prevents losing previous entries and matches the client's expectation.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const DATA_PATH = path.resolve('db', 'expense.json'); |
There was a problem hiding this comment.
DATA_PATH is db/expense.json but the code doesn't ensure the db directory exists. If db/ is missing, fs.writeFileSync will throw. Before writing, create the directory if needed, e.g. fs.mkdirSync(path.dirname(DATA_PATH), { recursive: true }). This change ensures saving to db/expense.json always succeeds.
|
|
||
| return; | ||
| } | ||
| if (req.method === 'GET' && req.url === '/expenses') { |
There was a problem hiding this comment.
The GET endpoint serving saved data is /expenses and it returns JSON. However the client attempts to fetch /db/expense.json to load all expenses (see src/index.html). Either serve the file at /db/expense.json or change the client to request /expenses. Also ensure the returned structure is an array (empty array [] when no data) rather than an empty object {}, so JSON.stringify on the client renders a list correctly.
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data) |
There was a problem hiding this comment.
You send JSON from the client with Content-Type: application/json and the server currently parses JSON — that is OK if you intend to use JSON. Just ensure both sides agree on JSON and the URL. If you prefer a normal HTML form POST (no JS), you'd need to change client and server to use application/x-www-form-urlencoded parsing.
| }); | ||
|
|
||
| form.reset(); | ||
| loadExpenses(); |
There was a problem hiding this comment.
After the POST you await fetch(...) but ignore the response and then call loadExpenses() which fetches the stored file. The task requires that after saving the app "return an HTML page with well formatted JSON." Currently your client ignores any HTML response from the server and expects to read JSON from a file. To satisfy the requirement, the server should respond to the POST with an HTML page (Content-Type text/html) containing pretty-printed JSON (e.g. inside a <pre>). The client should then display that HTML (or navigate to it) rather than just calling loadExpenses(). See the task description about returning an HTML page after saving.
| const arr = await res.json(); | ||
| expensesDiv.innerHTML = `<pre>${JSON.stringify(arr, null, 2)}</pre>`; |
There was a problem hiding this comment.
The client expects an array of expenses (const arr = await res.json(); and then JSON.stringify(arr, null, 2)). The server currently writes a single object to db/expense.json in its POST handler (no aggregation). Decide whether the file should hold an array (and append new expenses) or a single object. For the client to display a list, prefer storing an array and updating it on each POST (read existing array, push new entry, write back with JSON.stringify(array, null, 2)).
| try { | ||
| const res = await fetch('/db/expense.json'); | ||
| const arr = await res.json(); | ||
| expensesDiv.innerHTML = `<pre>${JSON.stringify(arr, null, 2)}</pre>`; | ||
| } catch { | ||
| expensesDiv.innerHTML = 'No expenses yet.'; |
There was a problem hiding this comment.
Consider error handling and UX: loadExpenses() shows 'No expenses yet.' only on fetch failure. If the server responds with an empty file or a single object instead of an array, res.json() may throw or produce unexpected results. Make sure the server returns valid JSON arrays or the client handles single-object responses appropriately.
| loadExpenses(); | ||
| }); | ||
|
|
||
| loadExpenses(); |
There was a problem hiding this comment.
Minor: you call loadExpenses() on page load which is good. After you align endpoints and response formats, test the full flow: load page, submit form, server saves to db/expense.json (ensure db exists), and the app displays the HTML response with pretty JSON or loads the /expenses JSON array as intended.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES — The server currently blocks core functionality and doesn’t meet required behaviors.
Critical issues to fix:
- In
src/createServer.js, areturninside the GET/handler makes the POST logic unreachable. Move body parsing and save logic into a dedicatedPOST /submit-expenseroute and ensure it only runs for POST requests. - Align endpoints with the client: the form submits to
/submit-expense, and the client loads saved data (change client to fetch/expensesor serve/db/expense.json). - Persist expenses as an array in
db/expense.json: read existing file (fallback to[]), push the new expense, and write backJSON.stringify(array, null, 2). Create thedbdirectory if missing (fs.mkdirSync(path.dirname(DATA_PATH), { recursive: true })). - After a successful POST, respond with an HTML page containing pretty-printed JSON (
Content-Type: text/htmland<pre>${JSON.stringify(obj, null, 2)}</pre>), notapplication/json. - Keep and execute validation for
date,title,amountin the POST handler; on failure, return an HTML error response. Also, make GET/expensesreturn[]when no file exists and handle JSON parse errors gracefully.
Minor improvements:
- Consider coercing
amountto a Number and validating!Number.isNaN(amount)for consistency.
You’ve got the right structure on the client and good validation intent; once the routing, persistence, and response format are fixed, the app will meet the spec.
✨ 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
| res.end('file not found'); | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
This return; makes all code after it unreachable, so the request-body parsing and save logic that follow will never run. Move POST handling out of the GET / branch (or remove this return) and implement a dedicated POST route (e.g., if (req.method === 'POST' && req.url === '/submit-expense')) so form submissions are actually handled. This is critical to meet the requirement that the app accepts submitted form data via POST.
| const chunks = []; | ||
|
|
||
| req.on('data', (chunk) => chunks.push(chunk)); | ||
|
|
||
| req.on('end', () => { | ||
| try { | ||
| const buffer = Buffer.concat(chunks).toString(); | ||
| const obj = JSON.parse(buffer); |
There was a problem hiding this comment.
Body-parsing code lives here inside the GET / branch (collecting data events and parsing JSON). It must be executed only for POST requests. Create a separate POST route and move this logic there. Also ensure you check the request URL (client posts to /submit-expense) so the server handles the correct endpoint.
| if (!obj.date || !obj.title || !obj.amount) { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end('Invalid expense data'); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Validation for required fields (date, title, amount) is present — good — but because this block is currently unreachable it never runs. Keep this validation in the POST handler, and when validation fails return an HTML response (not plain text) or at least an HTML error page so responses remain consistent with the requirement that POST returns HTML containing JSON.
| } | ||
|
|
||
|
|
||
| fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2)); |
There was a problem hiding this comment.
This line overwrites db/expense.json with a single object. The spec requires saving submitted expense data to db/expense.json and preserving previous entries. Instead, read the existing file (fallback to an empty array []), push the new expense, and write back JSON.stringify(array, null, 2). Also ensure the db directory exists before writing (e.g., fs.mkdirSync(path.dirname(DATA_PATH), { recursive: true })) to avoid first-run errors.
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(obj)); |
There was a problem hiding this comment.
After save the server responds with Content-Type: application/json. The requirement states the response after form submission must be an HTML page containing well-formatted (pretty-printed) JSON. Change this response to send an HTML document (for example, res.writeHead(200, { 'Content-Type': 'text/html' }) and include <pre>${JSON.stringify(obj, null, 2)}</pre> in the body) so the client receives HTML with pretty-printed JSON.
|
|
||
| async function loadExpenses() { | ||
| try { | ||
| const res = await fetch('/db/expense.json'); |
There was a problem hiding this comment.
The client fetches saved expenses from /db/expense.json (used in loadExpenses). Ensure the server serves that path or change the client to fetch the server endpoint that provides stored expenses (for example /expenses). Right now this path may not exist on the server, so the client will show "No expenses yet." if it can't fetch the file.
| await fetch('/submit-expense', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data) | ||
| }); |
There was a problem hiding this comment.
The form is submitted with fetch('/submit-expense', { method: 'POST', ... }). Ensure the server exposes a matching POST /submit-expense route. If the server uses a different route (e.g. /add-expense or /expenses), change one side so they match; otherwise the POST will never reach the server handler. Also confirm the server parses application/json request bodies.
| const data = { | ||
| title: form.title.value, | ||
| date: form.date.value, | ||
| amount: form.amount.value |
There was a problem hiding this comment.
Mapping of form inputs to the data object (title, date, amount) is correct and matches the required fields. Consider whether amount should be converted to a Number (Number(form.amount.value)) before sending or handle parsing on the server; either is acceptable as long as the server-side storage is consistent.
| }); | ||
|
|
||
| form.reset(); | ||
| loadExpenses(); |
There was a problem hiding this comment.
After the fetch POST completes the code calls loadExpenses() to refresh the shown list. This is sensible, but note: the requirement states the server must "return an HTML page with well formatted JSON" after form submission. Because the client sends the form via fetch, the browser will not automatically navigate to or display that HTML response. Decide which behavior you want: (a) keep fetch and let the server return HTML (tests can still validate the response), and rely on loadExpenses() to refresh the list; or (b) submit the form as a normal form (no fetch) so the browser navigates to the HTML response returned by the server. Make sure chosen approach aligns with automated tests/requirements.
| try { | ||
| const res = await fetch('/db/expense.json'); | ||
| const arr = await res.json(); | ||
| expensesDiv.innerHTML = `<pre>${JSON.stringify(arr, null, 2)}</pre>`; |
There was a problem hiding this comment.
Displaying expenses with JSON.stringify(arr, null, 2) inside a <pre> is correct and produces the required pretty-printed JSON for the UI. Keep this as-is once the server writes an array to db/expense.json.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you progress while learning. You’ve set up the HTML form with the required fields and started server-side parsing and file I/O — great foundation. Critical fixes needed in src/createServer.js: 1) close the try/catch for reading index (around the GET '/' handler) by sending a body (e.g., res.statusCode=404; res.end('Not found')) and return so the GET logic doesn’t continue. 2) Move the request-body parsing and save logic (lines ~24–49) into a dedicated POST route like if (req.method === 'POST' && req.url === '/submit-expense') { ... } so GET '/' only serves the HTML. 3) Validate and coerce amount with const amount = Number(obj.amount) and ensure !Number.isNaN(amount), plus validate date/title; send a 400 HTML error if invalid. 4) Persist expenses as an array: read existing JSON (fallback []), push the new expense, mkdirSync the db directory recursively, then write JSON.stringify(array, null, 2). 5) After saving, respond with Content-Type: text/html and return an HTML page that pretty-prints the saved data inside
.... Also align endpoints with the client: either serve GET /db/expense.json or change the client to fetch /expenses; if you keep /db/expense.json, return [] when missing and handle corrupt JSON safely. On the client (src/index.html), coerce amount to Number before sending, check res.ok before res.json(), and consider reading the POST’s HTML response via res.text() for better feedback. Nice progress — with these adjustments, your app will match the requirements and work reliably.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } catch { | ||
| res.writeHead(404, { 'Content-Type': 'text/plain' }); | ||
|
|
||
|
|
There was a problem hiding this comment.
The catch after reading the index file sets a 404 header but never sends a body or returns. This leaves the request flow in an invalid state and allows the body-parsing logic below to execute inside the GET '/' handler. You must close the catch properly (send a response body like res.end('Not found')) and return so subsequent code doesn't run inside this branch. This is a critical syntax/flow bug that must be fixed.
| const chunks = []; | ||
|
|
||
| req.on('data', (chunk) => chunks.push(chunk)); | ||
|
|
||
| req.on('end', () => { | ||
| try { | ||
| const buffer = Buffer.concat(chunks).toString(); | ||
| const obj = JSON.parse(buffer); | ||
|
|
||
| if (!obj.date || !obj.title || !obj.amount) { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end('Invalid expense data'); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
|
|
||
| fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2)); | ||
|
|
||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(obj)); | ||
| } catch { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end('Invalid JSON'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The code that collects chunks, parses the request body and writes the expense (lines 24–49) is currently placed inside the GET '/' handler. Move this whole block into a dedicated POST handler such as if (req.method === 'POST' && req.url === '/submit-expense') { ... }. The POST handler should only run for POST requests and the GET '/' handler should only serve the HTML form. This fixes the routing and makes the server behave as required.
| const buffer = Buffer.concat(chunks).toString(); | ||
| const obj = JSON.parse(buffer); | ||
|
|
||
| if (!obj.date || !obj.title || !obj.amount) { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end('Invalid expense data'); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
You parse the incoming body with JSON.parse(buffer) but there's no coercion/validation of amount. Per the requirements validate date, title, and amount and coerce amount to a Number (const amount = Number(obj.amount)) and confirm !Number.isNaN(amount). If validation fails, return an HTML error response with a 400 status. Right now only presence checks are used which is insufficient.
| } | ||
|
|
||
|
|
||
| fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2)); |
There was a problem hiding this comment.
You call fs.writeFileSync(DATA_PATH, JSON.stringify(obj, null, 2)), which overwrites the file with a single object. The app must persist expenses as an array. Read the existing file (fallback to []), push the new expense, ensure the db directory exists (fs.mkdirSync(path.dirname(DATA_PATH), { recursive: true })), then write back JSON.stringify(array, null, 2). This ensures previous expenses are preserved.
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| res.end(JSON.stringify(obj)); |
There was a problem hiding this comment.
After saving you respond with Content-Type: 'application/json' and return raw JSON. The task requires returning an HTML page containing pretty-printed JSON. Update the response to Content-Type: 'text/html' and send an HTML body containing the formatted JSON, e.g. <pre>${JSON.stringify(savedObj, null, 2)}</pre>.
|
|
||
| async function loadExpenses() { | ||
| try { | ||
| const res = await fetch('/db/expense.json'); |
There was a problem hiding this comment.
The client attempts to load saved expenses from /db/expense.json. Ensure your server actually serves that path (or change this client fetch to match the server endpoint). The current server implementation in the project listens for /expenses (not /db/expense.json) which leads to a mismatch; align them so the fetch succeeds. See server file for reference .
| async function loadExpenses() { | ||
| try { | ||
| const res = await fetch('/db/expense.json'); | ||
| const arr = await res.json(); |
There was a problem hiding this comment.
You call await res.json() without checking the response status. If the file is missing the fetch may return 404 and res.json() will throw. Consider checking if (!res.ok) throw new Error('No expenses') before parsing, or handle res.status explicitly so the UI can show a friendly message rather than relying on the catch block alone.
| const data = { | ||
| title: form.title.value, | ||
| date: form.date.value, | ||
| amount: form.amount.value |
There was a problem hiding this comment.
When building the data object you send amount as a string. Coerce it to a Number before sending (e.g. amount: Number(form.amount.value)) and validate it (ensure it's not NaN) — this simplifies server-side validation and meets the task requirement to treat amount numerically.
| await fetch('/submit-expense', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(data) | ||
| }); |
There was a problem hiding this comment.
You await fetch('/submit-expense', ...) but do not check the response. The server is required to return an HTML page with formatted JSON after saving; consider reading the response text (const text = await res.text()) and displaying it or handling non-OK statuses (if (!res.ok) { /* show error */ }). Also ensure your server actually implements POST /submit-expense and responds with Content-Type: text/html containing pretty-printed JSON per the task description .
| }); | ||
|
|
||
| form.reset(); | ||
| loadExpenses(); |
There was a problem hiding this comment.
After submitting you immediately call loadExpenses() which re-fetches the JSON file. That's fine, but if the server returns the required HTML response for the POST it will be ignored. Decide whether you want to show the server's returned HTML to the user (recommended for debugging/requirements compliance) or rely only on reloading the JSON file. Either way, handle errors from the POST request and show feedback to the user.
No description provided.