Task solution#57
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the Page Object Model and writing comprehensive tests! The code is clean and well-structured.
However, this submission requires a few changes because the core requirement of automating checks on commit has not been met. While the ESLint and Prettier configurations are present, Husky and Lint-Staged are not set up to run them automatically. To fix this, you'll need to add the lint-staged configuration and the prepare script for Husky to your package.json to enable the pre-commit hook [CHECKLIST ITEM #3] & [CHECKLIST ITEM #4].
As a minor improvement, you can also clean up eslint.config.js by removing the redundant rules that are already included from the base configurations.
Once the pre-commit hook is working as expected, this will be ready for approval!
✨ 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
| ...pluginJs.configs.recommended.rules, | ||
| 'no-unused-vars': 'error', | ||
| 'max-len': [ | ||
| 'error', | ||
| { | ||
| code: 80, | ||
| comments: 80, | ||
| }, | ||
| ], | ||
| ...playwright.configs['flat/recommended'].rules, |
There was a problem hiding this comment.
Spreading the recommended rules from pluginJs and playwright here is redundant. You've already included these rules by spreading the entire configuration objects on lines 11 and 12. The rules object should only contain new rules or overrides for the existing ones, not the entire base set again.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up Husky and Lint-Staged, and for writing such clean tests! I'm requesting one change before this can be approved.
In tests/signUp/signUpNegative.spec.js, the long error message is split using a template literal. This introduces newline characters and extra spaces, which will cause the test assertion to fail because the expected text won't match the actual error message from the application. To fix this while keeping the line length compliant, please concatenate the string parts using the + operator.
As a minor cleanup, you can also remove the redundant 'no-unused-vars' rule from eslint.config.js, as it's already included in the recommended configuration you're using. Once that test assertion is fixed, this will be ready to go!
✨ 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
| ...pluginJs.configs.recommended, | ||
| ...playwright.configs['flat/recommended'], | ||
| rules: { | ||
| 'no-unused-vars': 'error', |
There was a problem hiding this comment.
This rule is already included in pluginJs.configs.recommended, so this line is redundant and can be removed to make the configuration cleaner.
| `username:Username must start with a letter, | ||
| have no spaces, and be 2 - 40 characters.`, |
There was a problem hiding this comment.
By splitting the string using a multi-line template literal, you've introduced a newline character (\n) and extra spaces into the expected error message. This will likely cause the assertion to fail because the actual error message from the application is probably a single, continuous line of text.
To fix this while respecting the max-len rule, you can concatenate the strings using the + operator.
Replaced the multi-line template literal with string concatenation so the expected text is a single line (no \n or extra spaces). The assertion now expects: username:Username must start with a letter, have no spaces, and be 2 - 40 characters. The concatenation keeps the line under the 80-character limit. 2. eslint.config.js Removed the redundant 'no-unused-vars': 'error' rule, since it’s already part of pluginJs.configs.recommended.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this pull request! You've successfully addressed all the feedback from the previous review, resulting in a clean and well-configured project. The ESLint, Prettier, and Husky setups are correctly implemented, and the test assertion fix was a great catch. [CHECKLIST ITEM #1] [CHECKLIST ITEM #2] [CHECKLIST ITEM #3] Your code is in great shape and meets all the project requirements, so I am approving your solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.