feat: setup eslint, prettier and husky#90
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical configuration issue in your ESLint setup that directly affects the core requirement that ESLint works correctly. In eslint.config.mjs, you both use extends: ['js/recommended'] and manually spread ...js.configs.recommended.rules into rules, which can cause the same ruleset to be applied twice and lead to confusing or conflicting behavior; you should pick a single approach and remove the duplication (for example, keep extends and drop the manual spread, or vice versa). Also in eslint.config.mjs, you add eslintConfigPrettier as a bare array element in the flat config; this likely does not integrate with flat config as intended, so you need to verify the package’s flat-config usage and either spread its exported config or wrap it correctly so that it actually disables conflicting formatting rules and properly coordinates ESLint with Prettier. These two issues are configuration-level and can prevent ESLint/Prettier from behaving as required by the task, so they must be fixed before the tooling setup can be considered complete. The rest of your Playwright page objects and tests look clean and consistent with the current rules, which is great and suggests you’re on the right track. Once you correct the ESLint flat config usage and ensure Prettier integration is wired according to the package docs, your solution should satisfy the task’s ESLint/Prettier configuration requirements.
✨ 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
| js, | ||
| playwright, | ||
| }, | ||
| extends: ['js/recommended'], |
There was a problem hiding this comment.
You both extend 'js/recommended' and spread js.configs.recommended.rules into rules. This can lead to the recommended rules being applied twice and potentially overridden in unexpected ways; pick one mechanism (either extends or manual spread) to keep the config consistent.
| 'playwright/expect-expect': 'off', | ||
| }, | ||
| }, | ||
| eslintConfigPrettier, |
There was a problem hiding this comment.
Including eslintConfigPrettier as a raw array element may not be correct for flat config; verify the package’s flat-config usage and either spread it into the rule set or wrap it properly so that it actually disables conflicting formatting rules and integrates with Prettier as intended.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because your ESLint/Prettier flat-config is still not wired according to the required theory/docs, which is a core part of this task and thus a blocking issue. In eslint.config.mjs, you currently treat @eslint/js as a plugin and manually spread js.configs.recommended.rules; in flat config you should instead spread the full config object, e.g. add ...js.configs.recommended as a separate entry in the exported config array so that environments and other settings are applied correctly [CHECKLIST ITEM #11, #16]. In the same file, you also use ...eslintConfigPrettier.rules, but for flat config, eslint-config-prettier is meant to be integrated by spreading its dedicated flat export (or equivalent config object) as its own config block, not by cherry‑picking .rules, so right now it may not reliably disable all formatting‑related rules [CHECKLIST ITEM #12, #17, #18]. To fix this, adjust the configuration to:
- remove
plugins: { js, playwright }and instead rely on...js.configs.recommendedplus your Playwright config, and - integrate
eslint-config-prettierusing its documented flat-config entry (for example,...eslintConfigPrettier.flator a similar export), added as a separate object late in the config array so it can turn off conflicting rules.
Your page objects and tests look good and don’t introduce tooling issues, so once you correct the ESLint flat-config wiring for@eslint/jsandeslint-config-prettier, your solution should fully satisfy the task requirements.
✨ 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
| plugins: { | ||
| js, | ||
| playwright, |
There was a problem hiding this comment.
This plugins: { js, playwright } usage is not how @eslint/js is meant to be wired in flat config; the package exposes config objects (e.g. js.configs.recommended) that should be spread into the config array rather than registered as a plugin, so this doesn’t follow the theory/docs and triggers checklist item #16 about correct eslint.config.mjs setup.
| playwright, | ||
| }, | ||
| rules: { | ||
| ...js.configs.recommended.rules, |
There was a problem hiding this comment.
Spreading ...js.configs.recommended.rules manually duplicates what you’d normally get by spreading js.configs.recommended as a config object; with flat config, you should spread the full config (...js.configs.recommended) instead of cherry-picking .rules, to keep the configuration consistent with the theory instructions (checklist item #11/#16).
| ...playwright.configs['flat/recommended'].rules, | ||
| 'no-unused-vars': ['error', { argsIgnorePattern: '^page$' }], | ||
| 'playwright/expect-expect': 'off', | ||
| ...eslintConfigPrettier.rules, |
There was a problem hiding this comment.
Using ...eslintConfigPrettier.rules assumes the flat-config integration is just a rules object, but eslint-config-prettier has specific guidance for flat config and is typically used by spreading its exported config into the config array; as written, it likely doesn’t disable all formatting-related rules and violates checklist item #17 about correctly integrating Prettier with ESLint.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine the tooling setup. You’ve done well to adopt ESLint’s flat config and to include both @eslint/js and Playwright via their recommended configs, and your Playwright page objects/tests are clean and focused on the task domain. The main issues to improve are in eslint.config.mjs: you’re still re‑spreading pluginJs.configs.recommended.rules and playwright.configs['flat/recommended'].rules inside a single rules object, instead of relying solely on the full flat config objects and then adding only your custom overrides [CHECKLIST ITEM #11, #16]. You also need to integrate eslint-config-prettier as a separate flat config entry near the end of the array so ESLint doesn’t enforce formatting rules that conflict with Prettier [CHECKLIST ITEM #12, #17, #18], and set up lint-staged (wired from Husky) so git commit runs ESLint/Prettier and blocks commits on errors. As you iterate, focus on simplifying the ESLint config composition, adding the Prettier flat config, and ensuring Husky + lint-staged enforce the checks automatically. Overall, you’re very close; these are great learning opportunities around modern tooling, and your work so far shows solid progress.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| ...pluginJs.configs.recommended, | ||
| ...playwright.configs['flat/recommended'], | ||
| rules: { | ||
| ...pluginJs.configs.recommended.rules, |
There was a problem hiding this comment.
This still treats @eslint/js primarily as a source of rules and merges them into a single config object. Per the task and previous review (CHECKLIST focus on ESLint configuration correctness), you should spread the full pluginJs.configs.recommended as a separate config block (e.g., another object in the exported array) so that its environment and language options are applied correctly, rather than manually merging pluginJs.configs.recommended.rules into one rules object.
| ...playwright.configs['flat/recommended'], | ||
| rules: { | ||
| ...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.
Similarly, you are manually spreading playwright.configs['flat/recommended'].rules into the rules object. For flat config, the Playwright recommended config should be included as its own config entry (like you already do with ...playwright.configs['flat/recommended']), without separately spreading its .rules property, to avoid duplicating or mis‑configuring rules.
| this.signInButton = page.getByRole('button', { name: 'Sign in' }); | ||
| this.errorMessage = page.getByRole('list').nth(1); | ||
| } | ||
|
|
||
| async open() { |
There was a problem hiding this comment.
This config still manually re-spreads pluginJs.configs.recommended.rules and playwright.configs['flat/recommended'].rules inside a single object, rather than composing the flat configs as separate entries. Per the task requirements and previous high-priority guidance, you should rely on ...pluginJs.configs.recommended plus the Playwright flat config object directly (as separate blocks or correctly merged), and avoid treating @eslint/js as just a rules plugin. This is blocking proper flat-config behavior for ESLint.
| import { expect } from '@playwright/test'; | ||
|
|
||
| export class SignUpPage { | ||
| constructor(page) { | ||
| this.page = page; | ||
| this.usernameField = page.getByPlaceholder("Username"); | ||
| this.emailField = page.getByPlaceholder("Email"); | ||
| this.passwordField = page.getByPlaceholder("Password"); | ||
| this.signUpButton = page.getByRole("button", { name: "Sign up" }); | ||
| this.errorMessage = page.getByRole("list").nth(1); | ||
| this.usernameField = page.getByPlaceholder('Username'); | ||
| this.emailField = page.getByPlaceholder('Email'); | ||
| this.passwordField = page.getByPlaceholder('Password'); | ||
| this.signUpButton = page.getByRole('button', { name: 'Sign up' }); | ||
| this.errorMessage = page.getByRole('list').nth(1); | ||
| } |
There was a problem hiding this comment.
You no longer integrate Prettier here at all, but the task and checklist require Prettier to be wired into ESLint via its flat config export (e.g. spreading the eslint-config-prettier flat config as a separate object late in this array). Without that, ESLint may still enforce formatting rules that conflict with Prettier, which violates the requirement that Prettier be set up using the theory instructions.
| await signInPage.open(); | ||
| }); | ||
|
|
||
| test("Assert error message for empty password", async () => { | ||
| await signInPage.fillEmailField("test@gmail.com"); | ||
| test('Assert error message for empty password', async () => { | ||
| await signInPage.fillEmailField('test@gmail.com'); | ||
| await signInPage.clickSignInButton(); | ||
| await signInPage.assertErrorMessageContainsText(`password:can\'t be blank`); | ||
| await signInPage.assertErrorMessageContainsText(`password:can't be blank`); | ||
| }); | ||
|
|
||
| test("Assert error message for empty email", async ({ page }) => { | ||
| await signInPage.fillPasswordField("newpass123!"); | ||
| test('Assert error message for empty email', async () => { | ||
| await signInPage.fillPasswordField('newpass123!'); | ||
| await signInPage.clickSignInButton(); | ||
| await signInPage.assertErrorMessageContainsText(`email:can't be blank`); |
There was a problem hiding this comment.
You correctly spread pluginJs.configs.recommended and the Playwright flat recommended config, but then you also re-spread pluginJs.configs.recommended.rules and playwright.configs['flat/recommended'].rules inside rules. This duplication goes against the flat-config pattern from the theory, where you should rely on the full config objects rather than cherry-picking .rules. Removing the extra .rules spreads and keeping only your custom overrides here (no-unused-vars, max-len, etc.) would align better with the required ESLint setup.
| await signUpPage.open(); | ||
| }); | ||
|
|
||
| test("Assert error message for empty username in Sign up form", async () => { | ||
| await signUpPage.fillEmailField("test@gmail.com"); | ||
| await signUpPage.fillPasswordField("newpass123!"); | ||
| test('Assert error message for empty username in Sign up form', async () => { |
There was a problem hiding this comment.
You spread pluginJs.configs.recommended here (which already includes its own rules), but then below you also spread pluginJs.configs.recommended.rules into rules. This duplicates rule wiring and goes against the flat-config guidance that the full @eslint/js recommended config should be used as-is rather than cherry-picking its rules. Consider removing the explicit ...pluginJs.configs.recommended.rules and only overriding or adding the few custom rules you need.
| }); | ||
|
|
||
| test("Assert error message for empty username in Sign up form", async () => { | ||
| await signUpPage.fillEmailField("test@gmail.com"); | ||
| await signUpPage.fillPasswordField("newpass123!"); | ||
| test('Assert error message for empty username in Sign up form', async () => { | ||
| await signUpPage.fillEmailField('test@gmail.com'); | ||
| await signUpPage.fillPasswordField('newpass123!'); | ||
| await signUpPage.clickSignUpButton(); | ||
|
|
||
| await signUpPage.assertErrorMessageContainsText( | ||
| `username:Username must start with a letter, have no spaces, and be 2 - 40 characters.` | ||
| `username:Username must start with a letter, have no spaces, and be 2 - 40 characters.`, | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Similarly, you spread the full Playwright flat recommended config here and then re-spread playwright.configs['flat/recommended'].rules into rules below. This redundancy isn’t needed in flat config and can be confusing; the Playwright config already defines its rules. You should rely on the single spread of the Playwright flat config and only add your custom Playwright rule overrides explicitly.
|
|
||
| await homePage.assertYourFeedTabIsVisible(); | ||
| }) | ||
| await signUpPage.open(); | ||
| await signUpPage.fillUsernameField(user.username); | ||
| await signUpPage.fillEmailField(user.email); | ||
| await signUpPage.fillPasswordField(user.password); | ||
| await signUpPage.clickSignUpButton(); | ||
|
|
There was a problem hiding this comment.
There is no sign here (or in other shared files) of Husky and Lint-Staged setup that runs ESLint/Prettier on git commit and blocks commits on errors, which is explicitly required by the task. Make sure you have the appropriate Husky hook and lint-staged configuration in the repository so commits only succeed when ESLint passes.
No description provided.