Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
You’re very close—POST handling, saving to db/expense.json, and returning pretty-printed JSON in HTML are implemented well. However, there are a couple of critical blockers that affect correctness and task compliance:
- Critical: The required HTML form (
public/index.html) is missing or not verifiable. The GET handler serves frompublic/, but I couldn’t findpublic/index.htmlcontaining a form withmethod="POST"and fields nameddate,title,amount. This is required for task completion. Please add it and ensure the names match exactly. - Critical correctness: Date validation is brittle in
src/createServer.js(e.g.,date.toISOString().startsWith(obj.date)). This may reject valid inputs (including standardYYYY-MM-DDfrom<input type="date">). Simplify to checking!isNaN(date.getTime())or normalize to comparedate.toISOString().slice(0,10)with the providedYYYY-MM-DD.
Minor improvements (not blocking but recommended):
- Rename
cheackValidation→checkValidationfor clarity. - Remove the empty
if (fs.existsSync(filePath)) { }block. - Parse the body based on
Content-Type(or validateapplication/x-www-form-urlencoded) and return a clear error if mismatched. - Use correct MIME types when serving static files and handle missing
Hostheader when constructingURL.
Fix the above critical items, and this should meet the requirements. Nice work on the core flow and persistence—just a few adjustments to make it robust and compliant.
✨ 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
| /* Write your code here */ | ||
| // Return instance of http.Server class | ||
| const server = http.createServer(async (req, res) => { | ||
| function cheackValidation(obj) { |
There was a problem hiding this comment.
The function name has a small typo: cheackValidation. Consider renaming to checkValidation for clarity and consistency. This is not a functional bug but makes the code easier to read and maintain.
src/createServer.js
Outdated
|
|
||
| const date = new Date(obj.date); | ||
|
|
||
| if (isNaN(date.getTime()) || !date.toISOString().startsWith(obj.date)) { |
There was a problem hiding this comment.
This date validation is brittle: date.toISOString().startsWith(obj.date) assumes the incoming date string matches the start of an ISO string. That may reject otherwise valid inputs (or inputs that include time/zones). Consider using a simpler check such as !isNaN(date.getTime()) or normalizing accepted formats before comparing. The task only requires the date field to be a valid date.
src/createServer.js
Outdated
| }); | ||
|
|
||
| req.on('end', () => { | ||
| const body = new URLSearchParams(rawBody); |
There was a problem hiding this comment.
Parsing the body with new URLSearchParams(rawBody) only works for application/x-www-form-urlencoded bodies (typical from an HTML form). If you may receive JSON, or other content types, parse based on req.headers['content-type']. At minimum, consider validating that Content-Type is application/x-www-form-urlencoded before using URLSearchParams so malformed input yields a clear error.
|
|
||
| res.end( | ||
| `<pre>${err.message}</pre> <pre>${JSON.stringify( | ||
| { |
There was a problem hiding this comment.
The error response always includes { error: 'Missing required fields', ... } which can be misleading because validation may fail for other reasons (e.g. invalid amount or date format). Make the error message reflect the actual validation failure (you already include err.message above; consider using that in the JSON as well).
src/createServer.js
Outdated
|
|
||
| fs.writeFileSync(filePath, JSON.stringify(data, null, 2)); | ||
|
|
||
| if (fs.existsSync(filePath)) { |
There was a problem hiding this comment.
This if (fs.existsSync(filePath)) { } block is empty and has no effect. Remove it to keep the code clean.
src/createServer.js
Outdated
| } | ||
|
|
||
| if (req.method === 'GET') { | ||
| const normalizedUrl = new url.URL( |
There was a problem hiding this comment.
Constructing the URL with http://${req.headers.host} assumes req.headers.host is present. In some environments the Host header may be missing and this can throw or produce an incorrect URL. Consider falling back to localhost or handling a missing host header gracefully.
| }); | ||
|
|
||
| fsStream.on('open', () => { | ||
| res.writeHead(200, { 'Content-Type': 'text/html' }); |
There was a problem hiding this comment.
When serving static files you always set Content-Type: 'text/html'. That is fine for HTML but incorrect for other file types (CSS, JS, images). Consider detecting the MIME type by file extension (or using a small lookup) so responses have the correct Content-Type header.
src/createServer.js
Outdated
| const origin = | ||
| path.basename(normalizedUrl.pathname.slice(1)) || 'index.html'; | ||
|
|
||
| const originPathName = path.join(__dirname, '..', 'public', origin); |
There was a problem hiding this comment.
The GET handler serves a file from ../public/<name>; the task requires an HTML form with date, title, and amount fields. I could not find public/index.html in the provided files to confirm the form exists and is correct. Ensure public/index.html exists and contains the required form (method POST, fields named date, title, amount).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because one core requirement isn’t met: after a successful POST, the server must return an HTML page with the submitted data as pretty-printed JSON, but it currently responds with Content-Type: application/json and raw JSON. Please update src/createServer.js (POST handler) to set Content-Type: text/html and render an HTML document containing <pre>${JSON.stringify(data, null, 2)}</pre>.
Critical fixes required:
- Replace the JSON response with an HTML page containing pretty-printed JSON (lines where the POST response is sent). This is an explicit task requirement and a blocker.
- Improve Content-Type checking: many clients include a charset (e.g.,
application/x-www-form-urlencoded; charset=utf-8). UsestartsWith(...)or split on;to avoid false 400s, and adjust the 400 error message to be generic (e.g., include the received content-type). - Fix URL base construction to avoid
http://undefined: compute abasefirst usingreq.headers.host ? ... : 'http://localhost:5701'before callingnew URL.
What’s good: your form is correct, the server validates inputs, and data is persisted to db/expense.json. After addressing the above, this should fully meet the requirements. Keep it up!
✨ 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.writeHead(200, { 'Content-Type': 'application/json' }); | ||
|
|
||
| res.end(JSON.stringify(data, null, 2)); |
There was a problem hiding this comment.
Critical: The POST handler responds with Content-Type: application/json and sends the raw JSON body. The task requires an HTML response that contains the submitted data formatted as well-formatted (pretty-printed) JSON. Replace the JSON response with an HTML document, for example:
<!doctype html>
<html>
<body>
<pre>{pretty-printed JSON}</pre>
</body>
</html>Use JSON.stringify(data, null, 2) to produce the pretty-printed JSON and set the response header to Content-Type: text/html. Update the code at these lines so the client gets an HTML page containing the JSON rather than raw application/json text. This fixes non-compliance with the task requirement to "return an HTML page with well formatted JSON" .
src/createServer.js
Outdated
| if (contentType === 'application/json') { | ||
| data = JSON.parse(rawBody); | ||
| } | ||
|
|
||
| if (contentType === 'application/x-www-form-urlencoded') { | ||
| data = Object.fromEntries(new URLSearchParams(rawBody)); | ||
| } | ||
|
|
||
| if (data === null) { | ||
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end('Invalid JSON'); |
There was a problem hiding this comment.
Content-Type handling is too strict: the code checks contentType === 'application/json' and contentType === 'application/x-www-form-urlencoded'. Many clients send a charset parameter (e.g. application/x-www-form-urlencoded; charset=utf-8), which would cause data to remain null and produce a 400. Consider matching using contentType && contentType.startsWith('application/json') or normalizing by splitting on ; and trimming. Also the error message returned when data === null is 'Invalid JSON' but the request might be form-urlencoded — make the error message more generic (e.g. 'Unsupported Content-Type' or include the received content-type) to aid debugging.
src/createServer.js
Outdated
| const normalizedUrl = new url.URL( | ||
| req.url || '', | ||
| `http://${req.headers.host}` || 'http://localhost:5701', | ||
| ); |
There was a problem hiding this comment.
Robustness: constructing the URL base as `http://${req.headers.host}` || 'http://localhost:5701' doesn't work as intended because the template string is evaluated first; if req.headers.host is undefined the template yields 'http://undefined' (truthy) and the fallback won't be used. Compute the base before passing to new URL, e.g.:
const base = req.headers.host ? `http://${req.headers.host}` : 'http://localhost:5701';
const normalizedUrl = new URL(req.url || '', base);This avoids http://undefined when Host is missing.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational override: Approving to help you progress while you learn. Great work implementing the form and server: the POST flow validates fields, writes to db/expense.json, and returns nicely formatted JSON wrapped in HTML.
Critical fixes to address next:
- Potential crash at src/createServer.js line ~47:
contentType.includes(...)can run whencontentTypeis undefined. Guard it:const contentType = req.headers['content-type'] || ''and normalize safely before calling string methods. This prevents server crashes when the header is missing. - Fragile normalization at the same area: prefer
const normalized = contentType ? contentType.split(';')[0].trim() : ''and then usenormalized === 'application/json'ornormalized.startsWith('application/json')for robust matching. - Uncaught JSON parse error: wrap
JSON.parse(rawBody)in try/catch and return a 400 HTML page explaining the parse issue (include the error message or raw body) to avoid crashing on malformed JSON. - Consistency with HTML responses: when rejecting unsupported content types, send
Content-Type: text/htmland present the message in HTML (e.g., using<pre>), including bothnormalizedand the received header value.
Minor improvements:
- Use the detected mime type for GET responses instead of always
text/html.
You’re very close—solid structure and clear validation. Fixing the above will make your server robust and test-friendly. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (req.method === 'POST') { | ||
| let rawBody = ''; | ||
|
|
||
| const contentType = req.headers['content-type']; |
There was a problem hiding this comment.
Potential crash: contentType is read from req.headers['content-type'] but may be undefined. Right after that you call contentType.includes(...) (line 47) which will throw if the header is missing. Guard the value before calling string methods or provide a default string. For example, normalize safely using const contentType = req.headers['content-type'] || '' or check if (!contentType) { ... } first. This is critical because a missing header will crash the request handler and break the server.
| const normalizedContentType = contentType.includes(';') | ||
| ? contentType.split(';')[0] | ||
| : contentType; |
There was a problem hiding this comment.
Fragile normalization and matching: const normalizedContentType = contentType.includes(';') ? contentType.split(';')[0] : contentType; will throw when contentType is undefined (see previous comment). Also it's clearer and more robust to split and trim once, then compare using exact match or startsWith. Example approach: const normalized = contentType ? contentType.split(';')[0].trim() : '' and then check if (normalized === 'application/json') or if (normalized.startsWith('application/json')). This avoids false negatives from charset suffixes and avoids runtime errors.
| if (normalizedContentType === 'application/json') { | ||
| data = JSON.parse(rawBody); |
There was a problem hiding this comment.
Uncaught JSON parse: data = JSON.parse(rawBody); is not wrapped in try/catch. Malformed JSON in the request body will throw and crash the handler. Wrap the parse in a try/catch and return a 400 HTML response explaining the parse error (and include the raw body or error message for debugging). Keep error responses in HTML to be consistent with the app’s requirement to return HTML pages.
| res.writeHead(400, { 'Content-Type': 'text/plain' }); | ||
| res.end(`Unsupported Content-Type: ${normalizedContentType}`); |
There was a problem hiding this comment.
Unsupported Content-Type response is sent as text/plain (res.writeHead(400, { 'Content-Type': 'text/plain' });) and the message is Unsupported Content-Type: ${normalizedContentType}. Per the task requirement the app should return HTML pages for responses; prefer Content-Type: text/html and present the error inside HTML (e.g. <pre>...</pre>). Also include the actual received header (could be undefined) in the message so it's clear what was received, e.g. received: ${String(contentType)}.
| res.writeHead(200, { 'Content-Type': 'text/html' }); | ||
|
|
||
| res.end(`<pre>${JSON.stringify(data, null, 2)}</pre>`); |
There was a problem hiding this comment.
Correct behavior: on successful POST you set Content-Type: 'text/html' and return the pretty-printed JSON inside <pre>...</pre>. This satisfies the requirement to return an HTML page with well-formatted JSON and is implemented correctly here. Keep this as-is.
Form data (with Node.js)
Implement an app that
db/expense.jsonfileRead the guideline before start