solution #3

serhiiananiev · 2025-06-12T14:32:22Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job! 🎉 Your code meets all the core requirements and functionality for the task. All necessary components, pages, and tests are present and correctly structured. The minor issues mentioned—such as the use of waitForTimeout, missing type annotations, a typo in a test name, selector details, and small ESLint or Playwright usage improvements—do not block approval, but I encourage you to address them in future updates for even better code quality and maintainability. Keep up the excellent work! 🚀

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

Rate AI review example ↩

mateacademy-ai-mentor · 2025-06-12T14:38:27Z

eslint.config.mjs

-          code: 80,
-          comments: 80,
+          code: 800,
+          comments: 800,
          ignorePattern: 'import *',


The value for ignorePattern in the max-len rule is set to 'import *'. This may not correctly match all import statements, as * is not a valid wildcard in regular expressions. If you want to ignore all import lines, consider using a regular expression like '^import .*$' instead. This is a minor issue but could affect the intended behavior of the rule.

mateacademy-ai-mentor · 2025-06-12T14:38:27Z

src/ui/actions/articles/createArticle.js


-      return viewArticlePage.getCurrentPageUrl();
+      return baseViewArticlePage.getCurrentPageUrl();


If baseViewArticlePage.getCurrentPageUrl() is an asynchronous function (returns a Promise), you should use await here to ensure that the resolved URL is returned rather than a Promise. Please check the implementation of getCurrentPageUrl() and add await if necessary.

mateacademy-ai-mentor · 2025-06-12T14:38:27Z

src/ui/components/ArticleListItem.component.js

+    await this.step(`Assert article title '${title}' is visible`, async () => {
+      const articleTitle = this.page.locator('h1').filter({
+        hasText: title,
+        visible: true,


The 'visible' property is not a valid option for Playwright's locator filter. If you want to ensure the element is visible, you should use Playwright's visibility assertions or the '.isVisible()' method separately.

mateacademy-ai-mentor · 2025-06-12T14:38:27Z

src/ui/components/ArticleListItem.component.js

+    await this.step(`Click on article title '${title}'`, async () => {
+      const articleTitle = this.page.locator('h1').filter({
+        hasText: title,
+        visible: true,


The 'visible' property is not a valid option for Playwright's locator filter. Please remove it and use Playwright's visibility checks or assertions as needed.

mateacademy-ai-mentor · 2025-06-12T14:38:27Z

src/ui/components/header/InternalHeader.js

@@ -8,6 +8,7 @@ export class InternalHeader extends BaseHeader {
    super(page, userId);
    this.#newArticleLink = this.page.getByRole('link', { name: 'New Article' });
    this.#settingsLink = this.page.getByRole('link', { name: 'Sign in' });


The selector for the settings link uses { name: 'Sign in' }. For an internal header (when the user is signed in), the link is usually labeled 'Settings', not 'Sign in'. Please verify and update the selector if necessary to ensure correct behavior.

mateacademy-ai-mentor · 2025-06-12T14:38:28Z

tests/articles/createArticle/createArticleWithRequiredFields.spec.js

@@ -13,7 +13,7 @@ test.beforeEach(async ({ page, user, logger }) => {
 test('Creat an article with required fields', async ({


There is a typo in the test name: 'Creat an article with required fields'. It should be 'Create an article with required fields'.

mateacademy-ai-mentor · 2025-06-12T14:38:28Z

tests/articles/viewArticle/notLoggedInUserCanOpenTheArticleCreatedByAnotherUser.spec.js

+  await pages[1].waitForLoadState('domcontentloaded');
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);


There are multiple consecutive calls to waitForTimeout and reload. Excessive use of these can slow down your tests and may indicate a need for a more robust waiting strategy (e.g., waiting for a specific element or condition). Consider replacing these with more targeted waits if possible, or add a comment explaining why they are necessary.

mateacademy-ai-mentor · 2025-06-12T14:38:28Z

tests/articles/viewArticle/notLoggedInUserCanViewArticleCreatedByAnotherUser.spec.js

+  await pages[1].waitForLoadState('domcontentloaded');
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);


There are multiple consecutive calls to waitForTimeout and reload. Excessive use of these can slow down your tests and may indicate a need for a more robust waiting strategy (e.g., waiting for a specific element or condition). Consider replacing these with more targeted waits if possible, or add a comment explaining why they are necessary.

mateacademy-ai-mentor · 2025-06-12T14:38:28Z

tests/articles/viewArticle/userCanFindOwnArticleByPopulatTagsSection.spec.js

+}) => {
+  await internalViewArticlePage.internalHeader.clickHomeLink();
+  await internalHomePage.popularTags.clickTag(articleWithOneTag.tags[0]);
+  await page.waitForTimeout(3000);


The use of waitForTimeout(3000) can slow down your tests and may indicate a need for a more robust waiting strategy (e.g., waiting for a specific element or condition). Consider replacing this with a targeted wait if possible.

mateacademy-ai-mentor · 2025-06-12T14:38:28Z

tests/articles/viewArticle/userCanViewOwnArticleInGlobalFeed.spec.js

+}) => {
+  await internalViewArticlePage.internalHeader.clickHomeLink();
+  await internalHomePage.globalFeed.open();
+  await page.waitForTimeout(3000);


The use of waitForTimeout(3000) can slow down your tests and may indicate a need for a more robust waiting strategy (e.g., waiting for a specific element or condition). Consider replacing this with a targeted wait if possible.

mateacademy-ai-mentor

Review Summary

Thank you for your hard work! 🚧 While your solution meets most core requirements and is well-structured, there are several critical issues that need to be addressed before approval:

Incorrect import of expect: In src/ui/components/header/BaseHeader.js, expect is imported from allure-playwright instead of your project's Playwright helper. This can cause runtime errors and must be fixed by importing from the correct path (e.g., ../../common/helpers/pw).
Selector issue for settings link: In src/ui/components/header/InternalHeader.js, the settings link is assigned to the element with the name 'Sign in', but it should likely be 'Settings'. Please verify and update the selector to ensure correct functionality.
Missing TypeScript type annotations: In tests/_fixtures/fixturesArticle.ts, the properties in the generic type for base.extend are missing their types. Please specify types for all properties to ensure proper type checking and maintainability.
ESLint max-len rule configuration: The ignorePattern option is not standard for the max-len rule. Please review the ESLint documentation and update your configuration to use a valid option or regex pattern if you want to ignore import statements.
Minor issues: There are some minor issues, such as a typo in a test name ('Creat' instead of 'Create') and excessive use of waitForTimeout and reload in some tests. While these do not block approval, addressing them will improve code quality and test reliability.

Once you resolve the critical issues above, your PR will be ready for approval! Keep up the great work—you're almost there! If you need help with any of the changes, feel free to ask. 😊

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

eslint.config.mjs

-          code: 80,
-          comments: 80,
+          code: 800,
+          comments: 800,
          ignorePattern: 'import *',


The ignorePattern option is not a standard option for the max-len rule in ESLint. If your intention is to ignore import statements, you should use a valid regex pattern (e.g., '^import\\s.+\\sfrom\\s.+;$'). Please check ESLint documentation for the correct usage of this option.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

src/ui/components/header/BaseHeader.js

@@ -1,3 +1,4 @@
+import { expect } from 'allure-playwright';


Issue: 'expect' is imported from 'allure-playwright', but it should be imported from '../../common/helpers/pw' (or your project's Playwright expect helper). This may cause runtime errors if 'expect' is not available from 'allure-playwright'. Please correct the import path.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

src/ui/components/header/InternalHeader.js

@@ -8,6 +8,7 @@ export class InternalHeader extends BaseHeader {
    super(page, userId);
    this.#newArticleLink = this.page.getByRole('link', { name: 'New Article' });
    this.#settingsLink = this.page.getByRole('link', { name: 'Sign in' });


Issue: The settings link is being assigned to the element with the name 'Sign in'. For an internal header, this should likely be 'Settings' instead. Please verify the selector and update it to match the correct link.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

tests/_fixtures/fixturesArticle.ts

  articleWithoutTags;
  articleWithOneTag;
  createArticlePage;
-  viewArticlePage;
+  baseViewArticlePage;
+  editArticlePage;
+  externalViewArticlePage;
+  internalViewArticlePage;


TypeScript issue: The properties in the generic type for base.extend are missing their types. For example, use articleWithoutTags: Type; instead of just articleWithoutTags;. Please specify the types for all properties to ensure proper type checking.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

tests/articles/createArticle/createArticleWithRequiredFields.spec.js

@@ -13,7 +13,7 @@ test.beforeEach(async ({ page, user, logger }) => {
 test('Creat an article with required fields', async ({


Typo: The test name has a typo. It should be 'Create an article with required fields' instead of 'Creat an article with required fields'.

mateacademy-ai-mentor · 2025-06-12T14:43:16Z

tests/articles/viewArticle/notLoggedInUserCanOpenTheArticleCreatedByAnotherUser.spec.js

+  await pages[1].waitForLoadState('domcontentloaded');
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);


Issue: There are multiple consecutive calls to waitForTimeout and reload on pages[1]. Unless strictly required by the application logic or checklist, this is excessive and can make the test slow and brittle. Consider removing or reducing these calls to improve test efficiency and reliability.

mateacademy-ai-mentor · 2025-06-12T14:43:17Z

tests/articles/viewArticle/notLoggedInUserCanViewArticleCreatedByAnotherUser.spec.js

+  await pages[1].waitForLoadState('domcontentloaded');
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);
+  await pages[1].reload();
+  await pages[1].reload();
+  await pages[1].waitForTimeout(2000);


Issue: There are multiple consecutive calls to waitForTimeout and reload on pages[1]. Unless strictly required by the application logic or checklist, this is excessive and can make the test slow and brittle. Consider removing or reducing these calls to improve test efficiency and reliability.

solution

66404a8

mateacademy-ai-mentor approved these changes Jun 12, 2025

View reviewed changes

mateacademy-ai-mentor suggested changes Jun 12, 2025

View reviewed changes


		return viewArticlePage.getCurrentPageUrl();
		return baseViewArticlePage.getCurrentPageUrl();

		@@ -13,7 +13,7 @@ test.beforeEach(async ({ page, user, logger }) => {
		test('Creat an article with required fields', async ({

solution #3

Are you sure you want to change the base?

solution #3

Uh oh!

Conversation

serhiiananiev commented Jun 12, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!