Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES — there’s a critical blocker in query parameter handling.
Critical issue to fix
- File:
src/createServer.js, around line 19- What’s wrong: When a query key appears multiple times, the code assigns an array to
query[key]. The requirement states all query values must be strings, and if a key repeats, only a single string should be included (last occurrence wins). - Why it’s a problem: This violates the core requirement “query parameter values must be strings,” and will cause incorrect output for duplicate keys.
- How to fix: When iterating over
URLSearchParams, always assign a string and overwrite previous values so the last one wins. For example, usefor (const [k, v] of searchParams) { query[k] = String(v); }and avoid collecting arrays likequery[k] = [...(query[k] || []), v]. Alternatively, usesearchParams.getAll(k)only if you then take the last item as a string:query[k] = arr[arr.length - 1].
- What’s wrong: When a query key appears multiple times, the code assigns an array to
What’s good
- Path splitting, filtering empty segments, handling multiple slashes, response structure, and JSON formatting all look correct. Great job on these parts.
Please address the query handling so that duplicate keys result in a single string value (last occurrence). Once that’s fixed, this should be ready to approve. Keep up the good 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
| for (const key of searchParams) { | ||
| const values = sanitizedUrl.searchParams.getAll(key); | ||
|
|
||
| searchParamsResult[key] = values.length > 1 ? values : values[0]; |
There was a problem hiding this comment.
The requirements specify that query parameter values must be strings, and if a key appears multiple times, only a single string value should be included (e.g., last occurrence wins). This line assigns an array if there are multiple values, which violates the requirements. You should only assign a string value for each key, not an array.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your server is very close and well-implemented overall, but there’s one critical blocker to meet the requirements.
Critical issue to fix (blocks requirements):
- Repeated query key handling returns the first value instead of the last.
- Location:
src/createServer.js(look where you build thequeryobject fromsearchParams) - Why it’s wrong: The requirement states that for repeated keys (e.g.,
?a=1&a=2), you must return the last value as a string. UsingsearchParams.get(key)always returns the first occurrence, which violates this rule. - How to fix: Iterate over all entries and assign in order so the last occurrence wins, e.g.:
const query = {}; for (const [key, value] of searchParams) { query[key] = value; }- Alternatively, use
const values = searchParams.getAll(key); query[key] = values[values.length - 1];
- Location:
What you did well:
- Correct URL parsing with
URL/URLSearchParams. - Proper handling of path segments and multiple slashes, and correct JSON response with
partsandquery. - Clean, robust code that handles empty paths and missing queries.
Once you adjust the repeated key handling so the last value is used, your solution should fully comply. Great work—this is a small, targeted change to complete the task!
✨ 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
| const searchParamsResult = [...searchParamsKeys].reduce( | ||
| (acc, current) => ({ | ||
| ...acc, | ||
| [current]: searchParams.get(current), |
There was a problem hiding this comment.
The current approach to extracting query parameters uses searchParams.get(current), which always returns the first value for a key. The requirements specify that if a query parameter appears multiple times, only the last value should be used (as a string). To fix this, consider iterating over all entries in searchParams and assigning each key-value pair to the result object, so the last occurrence wins. For example:
const query = {};
for (const [key, value] of searchParams) {
query[key] = value;
}This will ensure that for repeated keys, the last value is used, matching the requirements.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational approval: You’ve done a strong job overall—clean server setup, correct JSON shape, proper Content-Type, robust path splitting (handles multiple/leading/trailing slashes), and solid use of URL/URLSearchParams.
Critical issue to address (Checklist 3.2):
- Location: src/createServer.js (where you build
searchParamsResult) - Issue: Using
searchParams.get(current)returns the first value for repeated keys (e.g.,?a=1&a=2yieldsa: "1"), but the requirement specifies the last value must win. - Why:
URLSearchParams#getalways returns the first occurrence. The spec for this task requires last occurrence to be used. - How to fix (hint): Iterate over all entries and assign in order so later values overwrite earlier ones. For example:
const query = {}; for (const [key, value] of searchParams) { query[key] = value; // last one wins }
Minor note: Consider a brief comment explaining why “last value wins” is intentional for future readers.
You’re very close—great progress! Apply the small query handling tweak and you should be fully compliant.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const searchParamsResult = [...searchParamsKeys].reduce( | ||
| (acc, current) => ({ | ||
| ...acc, | ||
| [current]: searchParams.get(current), |
There was a problem hiding this comment.
The construction of searchParamsResult uses searchParams.get(current), which always returns the first value for a repeated key. The requirements specify that for repeated keys (e.g., ?a=1&a=2), the last value should be used. Consider iterating over all entries and assigning values in order so the last occurrence wins, e.g.,
const query = {};
for (const [key, value] of searchParams) {
query[key] = value;
}This will ensure that the last value for each key is used, as required by the checklist item 3.2.
No description provided.