solution#4118
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is one checklist violation and one small functional issue that can affect tests, so they should be fixed before this is merged. Functionally, your Sum component and App implementation are solid: the named export, default values for a and b, <p> usage, and the five required usages in App.jsx all match the requirements and the tests.
Critical issues that need fixing:
- In
src/components/Sum/Sum.jsx, the<p>content currently uses multiple text nodes likeSum of {a} and {b} is {a + b}. The checklist requires string interpolation inside tag content [CHECKLIST ITEM #2], so please change this to a single template literal expression, for example:<p>{`Sum of ${a} and ${b} is ${a + b}`}</p>
- In
cypress/support/commands.js, the custom commandgetByDataCycallscy.get(...)but does not return it. This breaks Cypress command chaining (e.g.,cy.getByDataCy('sum').click()), so update it toreturn cy.get([data-cy="${selector}"]);so callers can chain further commands.
Minor improvements (not blockers): you could clean up leftover comments in src/App.jsx and run Prettier/format-on-save to ensure consistent formatting. Once you address the string interpolation and the Cypress command return, your solution will fully meet the task requirements and be in great shape.
✨ 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
| // export const Sum = () => (); | ||
| export const Sum = ({ a = 0, b = 0 }) => ( | ||
| <p> | ||
| Sum of {a} and {b} is {a + b} |
There was a problem hiding this comment.
This violates checklist item #2: "[CODE STYLE] - Use string interpolation inside tag content". The component currently renders the paragraph as separate text nodes/expressions (Sum of {a} and {b} is {a + b}). Consider using a template literal inside the JSX expression so the paragraph content is a single string, for example:
<p>{`Sum of ${a} and ${b} is ${a + b}`}</p>
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical, functionality‑breaking issue in the Cypress commands, which can cause E2E tests to fail. Your React implementation looks solid: the Sum component meets the requirements (defaults, template literal, <p> output) and App.jsx uses Sum the required five times with correct props and formatting.
The blocking problem is in cypress/support/commands.js: your getByDataCy custom command does not currently return the result of cy.get(...), so code like cy.getByDataCy('sum').click() will not work because the Cypress chain is broken. To fix this, update the implementation to return the chainable, for example:
Cypress.Commands.add('getByDataCy', (selector) => {
return cy.get(`[data-cy="${selector}"]`);
});Additionally, make sure that the entry file referenced in index.html (/src/index.jsx) actually exists and mounts your React App into the #root element; otherwise the app will not start in the browser or under Cypress. Once you correct getByDataCy and verify the entry file path, re-run npm test to confirm all tests pass. Your core React work is in good shape; resolving these small but critical wiring issues will complete the task cleanly.
✨ 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
| export {}; | ||
| Cypress.Commands.add('getByDataCy', selector => { | ||
| cy.get(`[data-cy="${selector}"]`); |
There was a problem hiding this comment.
The custom command getByDataCy does not return the result of cy.get(...), so callers cannot chain further Cypress commands (e.g., cy.getByDataCy('sum').click()). Change it to: return cy.get([data-cy="${selector}"]); so it returns a Chainable. This is a high-priority fix because it breaks test chaining.
| </head> | ||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="/src/index.jsx"></script> |
There was a problem hiding this comment.
The page includes a script tag loading /src/index.jsx but I can't find that entry file in the provided files. If /src/index.jsx is missing or named differently the app will not start — ensure an entry file exists at this path and that it mounts the React App to #root.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as part of an educational override so you can keep progressing while you learn. Your React components are implemented very well: Sum uses a named export, default props, and the required string interpolation, and App renders all five required Sum usages while following the JSX style rules [CHECKLIST ITEM #1 and #2]. The main blocking issue is in index.html, where the script tag points to /src/index.jsx, but that file does not exist, so the app never mounts into the #root element (this breaks the page and the E2E tests). To fix this, create src/index.jsx and mount your App with React 18, for example:
import React from 'react';
import { createRoot } from 'react-dom/client';
import { App } from './App';
const root = createRoot(document.getElementById('root'));
root.render(<App />);Once you add that file and re-run your tests, your setup should work end-to-end. You’re very close—nice work on the components and configuration, and use this mounting issue as a chance to solidify how React apps boot from index.html.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| </head> | ||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="/src/index.jsx"></script> |
There was a problem hiding this comment.
This violates the requirement: "The entry file referenced in index.html (/src/index.jsx) must exist and mount the React App into the #root element." The script tag on this line points to /src/index.jsx, but that file is not present. Add /src/index.jsx and mount the App (example for React 18):
import React from 'react';
import { createRoot } from 'react-dom/client';
import { App } from './App';
const root = createRoot(document.getElementById('root'));
root.render(<App />);
DEMO LINK