Skip to content

Develop#4110

Open
shymkivvasyl wants to merge 14 commits into
mate-academy:developfrom
shymkivvasyl:develop
Open

Develop#4110
shymkivvasyl wants to merge 14 commits into
mate-academy:developfrom
shymkivvasyl:develop

Conversation

@shymkivvasyl
Copy link
Copy Markdown

DEMO LINK and add it to the PR description.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are a couple of issues that can break functionality and one explicit checklist violation that must be addressed.

Critical issues that must be fixed:

  1. In cypress/support/commands.js, the declare global { ... } block is TypeScript-only syntax and will cause a syntax error when Cypress loads this plain JavaScript support file. Remove or comment out this block, or move these type declarations into a .d.ts/TypeScript file if you want to keep them.
  2. In the same file (cypress/support/commands.js), the custom command getByDataCy currently calls cy.get(...) without returning it. This breaks Cypress command chaining. Update it to return cy.get(...) so test code like cy.getByDataCy('foo').click() continues to work as expected.
  3. In src/components/Sum/Sum.jsx, the paragraph text currently uses multiple JSX interpolations (Sum of {a} and {b} is {suma}), which violates [CHECKLIST ITEM #2] – Use string interpolation inside tag content. Change it to use a single template literal inside the tag, for example: <p>{Sum of ${a} and ${b} is ${a + b}}</p>.

Your core implementation of the Sum component and App usage looks correct, and the tests cover the required scenarios nicely; once you fix the Cypress support file and the interpolation style, this will 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +27 to +36
// declare global {
// namespace Cypress {
// interface Chainable {
// login(email: string, password: string): Chainable<void>
// drag(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// dismiss(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// visit(originalFn: CommandOriginalFn, url: string, options: Partial<VisitOptions>): Chainable<Element>
// }
// }
// }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declare global { ... } block is TypeScript declaration syntax and is not valid in a plain JavaScript file. It will cause a syntax error when Cypress loads this support file. Either remove/comment these lines or move the declarations into a .d.ts/TypeScript file (or convert this file to TypeScript).

export {};

Cypress.Commands.add('getByDataCy', selector => {
cy.get(`[data-cy="${selector}"]`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom command getByDataCy does not return the Cypress chainable. Change cy.get(...) to return cy.get(...) so callers can continue chaining commands (matching how byDataCy returns a value).

Comment thread src/components/Sum/Sum.jsx Outdated

return (
<p>
Sum of {a} and {b} is {suma}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #2: "[CODE STYLE] - Use string interpolation inside tag content". The paragraph currently uses separate JSX expressions (Sum of {a} and {b} is {suma}). Consider using a template literal inside the tag content, for example:

<p>{`Sum of ${a} and ${b} is ${a + b}`}</p>

This will satisfy the required code style.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are a couple of critical issues that will break your Cypress tests, plus one checklist violation to address. First, in cypress/support/commands.js, the declare global { ... } block uses TypeScript-only syntax inside a plain JavaScript file; Cypress will throw a syntax error when loading this file, so you need to either remove/comment out that block or move those type declarations into a .d.ts file (for example, cypress/support/index.d.ts). Second, in the same file, your custom command getByDataCy currently calls cy.get(...) but does not return it; update it to return cy.get(...) so that command chaining like cy.getByDataCy('foo').click() continues to work correctly. Finally, in src/components/Sum/Sum.jsx, the <p> tag content violates [CHECKLIST ITEM #2] because it uses multiple JSX interpolations; change it to a single template literal, e.g. (<p>{Sum of ${a} and ${b} is ${a + b}}</p>). Once you fix these points, your functional implementation of Sum, App, and the tests should be in great shape—nice work on covering all the required scenarios.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +27 to +36
// declare global {
// namespace Cypress {
// interface Chainable {
// login(email: string, password: string): Chainable<void>
// drag(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// dismiss(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// visit(originalFn: CommandOriginalFn, url: string, options: Partial<VisitOptions>): Chainable<Element>
// }
// }
// }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains a TypeScript-only declare global { ... } block which will cause a syntax error when Cypress loads this plain JavaScript support file. Fix by removing or commenting out the block, or moving the type declarations into a .d.ts/TypeScript declaration file. (As stated previously: "In cypress/support/commands.js, remove or comment out the declare global { ... } block, or move these type declarations into a .d.ts/TypeScript file, because declare global is TypeScript-only syntax and will cause a syntax error when Cypress loads this plain JavaScript support file.")

Comment on lines +40 to +41
Cypress.Commands.add('getByDataCy', selector => {
cy.get(`[data-cy="${selector}"]`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom command getByDataCy calls cy.get(...) but does not return it. Update it to return cy.get(...) so Cypress command chaining continues to work (e.g., cy.getByDataCy('foo').click()). (As stated previously: "In cypress/support/commands.js, update the custom command getByDataCy to return cy.get(...) ... so Cypress command chaining continues to work.")

Comment thread src/components/Sum/Sum.jsx Outdated
Comment on lines +1 to +3
export const Sum = ({ a = 0, b = 0 }) => (
<p>
Sum of {a} and {b} is {a + b}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates [CHECKLIST ITEM #2] - "Use string interpolation inside tag content". Replace the multiple JSX interpolations with a single template literal, for example: `

{`Sum of ${a} and ${b} is ${a + b}`}

` so the tag contains one interpolated string node rather than several separate JSX expressions.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as part of an educational override so you can keep progressing while learning, but there are two important Cypress issues to fix. Overall, your React implementation looks solid: Sum is a named export with default values for a and b, and App correctly renders the five required <Sum /> instances with clean, one-line JSX where appropriate [CHECKLIST ITEM #1, #2]. In cypress/support/commands.js, the declare global { ... } block (around the TypeScript-style typings) is not valid JavaScript and will cause Cypress to throw a syntax error when loading this file; move those type declarations into a .d.ts file such as cypress/support/index.d.ts, or remove/comment them out. In the same file, your getByDataCy custom command currently calls cy.get(...) without returning it; update it to return cy.get( \\[data-cy="${selector}"] ) so that chains like cy.getByDataCy('foo').click() work correctly. Once you address these two points, your E2E and component tests should be able to run smoothly, and the rest of your configuration (ESLint, Vite, GitHub Actions, and the Cypress specs) is in good shape. Keep up the good work—your core implementation and test coverage are strong, and these Cypress tweaks are a great opportunity to deepen your understanding of tooling details.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +27 to +36
// declare global {
// namespace Cypress {
// interface Chainable {
// login(email: string, password: string): Chainable<void>
// drag(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// dismiss(subject: string, options?: Partial<TypeOptions>): Chainable<Element>
// visit(originalFn: CommandOriginalFn, url: string, options: Partial<VisitOptions>): Chainable<Element>
// }
// }
// }
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains a TypeScript declaration block (declare global { ... }) inside a plain JavaScript file. That is TypeScript-only syntax and will cause Cypress to throw a syntax error when loading this support file. Move these type declarations into a .d.ts file (for example cypress/support/index.d.ts) or remove/comment this block.

Comment on lines +40 to +41
Cypress.Commands.add('getByDataCy', selector => {
cy.get(`[data-cy="${selector}"]`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom command getByDataCy calls cy.get(...) but does not return it. Update it to return the chainable (for example: Cypress.Commands.add('getByDataCy', selector => { return cy.get([data-cy="${selector}"]) })) so that command chaining like cy.getByDataCy('foo').click() works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants