Conversation
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 overall structure and the core conversion logic. However, I'm requesting a few changes because there's a critical issue with how the server handles errors.
In src/createServer.js, the catch block sets the response headers for an error but never actually sends a response with res.end(). This will cause any request that fails during the conversion process to hang and eventually time out for the client. Please ensure you send an error payload and end the connection in that block.
Additionally, the tests in tests/createServer.test.js are asserting against multi-line error messages, but the task description requires single-line strings. Please update the assertions to match the requirements, for example: '"toCase" query param is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'.
Once these fixes are in place, your server will be much more robust. Keep up the great effort!
✨ 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
| }), | ||
| ); | ||
| } catch (error) { | ||
| res.writeHead(400, { 'Content-Type': 'application/json' }); |
There was a problem hiding this comment.
This catch block sets the response headers but never sends a response body or ends the connection. This will cause the request to time out on the client-side. You should call res.end() with an appropriate error payload, similar to how you handle validation errors.
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
According to the task description, the error message should be a single line: 'Text to convert is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'. This test is asserting a multi-line string which doesn't match the requirement.
tests/createServer.test.js
Outdated
| `"toCase" query param is required. Correct | ||
| request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
The task description specifies this error message as a single line. This test should be updated to assert against '"toCase" query param is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".' to align with the requirements.
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
According to the task description, the error message should be a single line: 'Text to convert is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'. This test is asserting a multi-line string which doesn't match the requirement.
tests/createServer.test.js
Outdated
| `"toCase" query param is required. Correct | ||
| request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
The task description specifies this error message as a single line. This test should be updated to assert against '"toCase" query param is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".' to align with the requirements.
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
According to the task description, the error message should be a single line: 'Text to convert is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'. This test is asserting a multi-line string which doesn't match the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job fixing the hanging request issue—the server is much more stable now!
I'm requesting a few changes to align the solution with the task requirements. The primary issue is that the validation error messages in src/createServer.js are multi-line strings, but the requirements specify they must be exact single-line strings. Please update the messages for missing text and a missing toCase parameter to match the required format, and adjust the assertions in tests/createServer.test.js accordingly.
Additionally, the try...catch block now correctly ends the response, but it should send a JSON error payload instead of an empty body to ensure the server always responds with JSON as required. 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! ✨
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
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
The task requirements state that this error message must be a single-line string. This assertion is checking for a multi-line string, which doesn't align with the requirements. Please update it to expect the specified single-line message.
tests/createServer.test.js
Outdated
| `"toCase" query param is required. Correct | ||
| request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
This is another instance where the test is asserting against a multi-line error message. Remember to keep the assertions aligned with the single-line message requirement from the task description.
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
This part of the test also needs to be updated to assert against a single-line error message, as specified in the requirements.
tests/createServer.test.js
Outdated
| `"toCase" query param is required. Correct | ||
| request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
Please also update this assertion to check for the correct single-line error message.
tests/createServer.test.js
Outdated
| `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
This assertion should also be updated to check for a single-line error message to match the task requirements.
src/createServer.js
Outdated
| message: `Text to convert is required. Correct request is: | ||
| "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
This error message is a multi-line string due to the line break. The task requirements and previous review feedback specify a single-line string: 'Text to convert is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'. The tests might need to be updated as well to assert against the correct single-line message.
src/createServer.js
Outdated
| message: `"toCase" query param is required. Correct | ||
| request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".`, |
There was a problem hiding this comment.
Similar to the other validation message, this one is also multi-line. Please ensure it matches the single-line string format specified in the requirements: '"toCase" query param is required. Correct request is: "/<TEXT_TO_CONVERT>?toCase=<CASE_NAME>".'.
src/createServer.js
Outdated
| ); | ||
| } catch (error) { | ||
| res.writeHead(400, { 'Content-Type': 'application/json' }); | ||
| res.end(); |
There was a problem hiding this comment.
It's great that you've added res.end() to fix the hanging request—that was a critical fix! To fully align with the requirements, this catch block should also send a JSON error payload in the response body. The task specifies that the server should always respond with JSON.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work fixing the validation error messages—they now match the requirements perfectly!
I am approving this submission as an educational override to help you progress, but there is one important detail to be aware of. In src/createServer.js, the catch block currently sends an empty response for unexpected errors. This happens because the errors array is empty when an error occurs after the validation checks have passed, which violates the requirement for the server to always respond with a JSON object.
To complete this, you can create a generic error payload inside the catch block to ensure a valid JSON response is always sent. For example:
const errorPayload = { errors: [{ message: 'An unexpected error occurred' }] };
res.end(JSON.stringify(errorPayload));Great job on this task! You're very close to a perfect solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| ); | ||
| } catch (error) { | ||
| res.writeHead(400, { 'Content-Type': 'application/json' }); | ||
| res.end(errors); |
There was a problem hiding this comment.
The errors array is empty when the code reaches this catch block. As a result, res.end(errors) sends an empty response body, which isn't valid JSON. According to the requirements, the server must always respond with a JSON object. You should create and send a generic JSON error payload here, ensuring it is stringified before sending.
2 participants
No description provided.