MXWAR-76: add framework-agnostic Playwright e2e foundation for login#100
MXWAR-76: add framework-agnostic Playwright e2e foundation for login#100devvaansh wants to merge 2 commits intoopenMF:devfrom
Conversation
📝 WalkthroughWalkthroughAdds a Playwright-based end-to-end testing foundation: config, TypeScript setup, page-object base and LoginPage, typed selectors, test suite for login flows, .gitignore entries, and README documentation for running and porting tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,230,255,0.5)
participant Dev as Developer (Test Runner)
participant PW as Playwright Test
end
rect rgba(220,255,200,0.5)
participant Browser as Headless Browser
participant AppServer as Dev/Vite Server
participant Backend as Backend/API
end
Dev->>PW: run `npm run test:e2e`
PW->>AppServer: start webServer (build+preview or dev)
AppServer-->>PW: server ready
PW->>Browser: launch browser (chromium)
Browser->>AppServer: navigate to baseURL (/login)
PW->>Browser: run LoginPage interactions (fill, click, toggles)
Browser->>Backend: submit login request (if not skipped)
Backend-->>Browser: auth response
Browser-->>PW: page navigation / state changes
PW->>Dev: test results & reporters (html, list)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
devvaansh
force-pushed
the
MXWAR-76-e2e-framework-agnostic-login
branch
from
47134fb to
123f072
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
playwright/pages/BasePage.ts (1)
59-60: Avoidnetworkidleas the shared base-page default.This is a flaky default for a reusable POM base: once a page adds polling, analytics, or any long-lived request, every subclass that forgets to override
waitForLoad()can start timing out. A lighter default plus a page-specific ready sentinel is usually safer here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/pages/BasePage.ts` around lines 59 - 60, The BasePage.waitForLoad() uses 'networkidle' which is flaky as a shared default; change the implementation of BasePage.waitForLoad (method waitForLoad in class BasePage) to use a lighter wait state such as 'load' or 'domcontentloaded' (e.g., await this.page.waitForLoadState('load')) and update callers/tests to override waitForLoad in page-specific subclasses when they require a stronger sentinel (e.g., polling/analytics pages should implement their own ready sentinel or call waitForLoadState('networkidle') locally); ensure the method signature remains async waitForLoad(): Promise<void> and add a brief comment on BasePage explaining subclasses must override for page-specific readiness.playwright.config.ts (1)
23-24: Extract the dev server port to a single constant to prevent Playwright waiting on the wrong address.The
baseURLandwebServer.urlare hardcoded tohttp://localhost:5173, but the local command is justnpm run devwithout explicit port enforcement. Vite defaults to 5173, but if that port is already in use, Vite will auto-increment to 5174, 5175, etc. When this happens, Playwright continues waiting on 5173 and the test suite hangs.The fix extracts a shared port constant and enforces it with
--strictPortso Vite fails immediately if the port is unavailable, rather than silently choosing an alternative.Proposed change
+const port = 5173; +const baseURL = `http://localhost:${port}`; + export default defineConfig({ testDir: './playwright/tests', @@ use: { - baseURL: 'http://localhost:5173', + baseURL, @@ webServer: { command: isCI - ? 'npm run build && npx vite preview --port 5173' - : 'npm run dev', - url: 'http://localhost:5173', + ? `npm run build && npx vite preview --port ${port}` + : `npm run dev -- --port ${port} --strictPort`, + url: baseURL,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 23 - 24, Extract the dev server port into a single constant (e.g., DEV_SERVER_PORT = 5173) at the top of playwright.config.ts and use it to construct baseURL (`baseURL = \`http://localhost:${DEV_SERVER_PORT}\``) and webServer.url; update the webServer.command to pass `--port ${DEV_SERVER_PORT} --strictPort` when running `npm run dev` (or replace the command with `vite --port ${DEV_SERVER_PORT} --strictPort` if directly invoking Vite) so Vite fails instead of auto-incrementing ports; ensure the config's use.baseURL and webServer.url reference this constant (symbols: DEV_SERVER_PORT, use.baseURL, webServer, webServer.command).playwright/tests/login.spec.ts (2)
31-34: Strengthen the page-title assertion.
toBeTruthy()is very loose and won’t catch incorrect but non-empty titles. Prefer a stable expected string/pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login.spec.ts` around lines 31 - 34, The test named 'should have proper page title' uses page.title() and asserts expect(title).toBeTruthy(), which is too weak; replace that assertion with a concrete expectation (e.g., expect(title).toBe('Expected Title') for a known exact title or expect(title).toMatch(/Partial or regex pattern/) for a stable pattern) so the test fails on incorrect but non-empty titles; update the assertion in the test function that calls page.title() accordingly.
102-122: Deduplicate backend-skip guard across backend-only tests.The same skip check is repeated in each test. Group these tests under one backend-dependent
describeblock with a single guard.Refactor sketch
- test('should successfully login', async () => { - test.skip(skipBackendTests, 'Skipping backend-dependent test (SKIP_BACKEND_TESTS set).'); + test.describe('backend-dependent login flows', () => { + test.skip(skipBackendTests, 'Skipping backend-dependent test (SKIP_BACKEND_TESTS set).'); + + test('should successfully login', async () => { + await loginPage.loginAndWaitForDashboard('mifos', 'password'); + await loginPage.assertLoginSuccess(); + }); - await loginPage.loginAndWaitForDashboard('mifos', 'password'); - await loginPage.assertLoginSuccess(); - }); + test('should handle invalid credentials', async () => { + await loginPage.login('mifos', 'wrongpassword'); + await loginPage.assertValidationError(); + await loginPage.assertOnLoginPage(); + }); - test('should handle invalid credentials', async () => { - test.skip(skipBackendTests, 'Skipping backend-dependent test (SKIP_BACKEND_TESTS set).'); - - await loginPage.login('mifos', 'wrongpassword'); - await loginPage.assertValidationError(); - await loginPage.assertOnLoginPage(); - }); - - test('codegen baseline: login with mifos credentials', async () => { - test.skip(skipBackendTests, 'Skipping backend-dependent test (SKIP_BACKEND_TESTS set).'); - - await loginPage.login('mifos', 'password'); - await loginPage.assertLoginSuccess(); + test('codegen baseline: login with mifos credentials', async () => { + await loginPage.login('mifos', 'password'); + await loginPage.assertLoginSuccess(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/tests/login.spec.ts` around lines 102 - 122, Group the three backend-dependent tests under a single describe block and apply the skip guard once at the describe level instead of repeating test.skip in each test: wrap the tests in describe('backend-dependent: login tests', () => { ... }) and call test.skip(skipBackendTests, 'Skipping backend-dependent test (SKIP_BACKEND_TESTS set).') at the start of that describe (or use test.describe.configure({ mode: 'serial' }) with a conditional skip) so the contained tests (those invoking loginPage.login, loginPage.loginAndWaitForDashboard, loginPage.assertLoginSuccess, loginPage.assertValidationError, loginPage.assertOnLoginPage) no longer need individual skip calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Around line 31-33: Remove the global JSON Accept header configuration from the
Playwright config by deleting the Accept: 'application/json' entry under
extraHTTPHeaders (symbol: extraHTTPHeaders) in playwright.config.ts; tests and
API calls already set appropriate headers via the application's HTTP client
(src/lib/http-client.ts) so remove this redundant global header to restore
realistic browser behavior.
In `@playwright/pages/login.page.ts`:
- Around line 18-24: The selectors in the SELECTORS constant (LoginSelectors)
are brittle because they rely on English text and Tailwind classes; replace
those with stable, locale- and style-independent hooks (e.g., data-testid or
data-qa attributes) used by the Login component, or pin the Playwright locale
explicitly if English strings are required. Update SELECTORS.usernameInput,
.passwordInput, .loginButton, .errorMessage, .loadingIndicator and any other
selectors referenced at lines noted (related to remember/forgot/show-password)
to use data-testid/data-qa attributes (or other semantic attributes the app
exposes) and change tests to query by those attributes (or add the attributes in
the component). Ensure LoginSelectors type matches the new keys and that tests
still target the same logical elements (usernameInput, passwordInput,
loginButton, errorMessage, loadingIndicator).
In `@playwright/tests/login.spec.ts`:
- Around line 12-13: The current skipBackendTests flag uses
!!env.SKIP_BACKEND_TESTS which treats the string "false" as truthy; change the
parsing to interpret SKIP_BACKEND_TESTS explicitly (e.g., const skipBackendTests
= String(env.SKIP_BACKEND_TESTS).toLowerCase() === 'true' or use JSON.parse with
a safe fallback) so only an explicit true value disables backend tests; update
the code references to env and skipBackendTests accordingly.
- Around line 36-42: The test "should support keyboard navigation" currently
checks visibility and can pass falsely; change the final assertion to verify
focus instead of visibility by replacing
expect(loginPage.loginButton).toBeVisible() with an assertion that the locator
is focused (e.g., expect(loginPage.loginButton).toBeFocused()). Ensure the test
still simulates the Tab presses (page.keyboard.press('Tab')) as before and, if
necessary, adjust the number of Tab presses so the loginPage.loginButton locator
receives focus.
---
Nitpick comments:
In `@playwright.config.ts`:
- Around line 23-24: Extract the dev server port into a single constant (e.g.,
DEV_SERVER_PORT = 5173) at the top of playwright.config.ts and use it to
construct baseURL (`baseURL = \`http://localhost:${DEV_SERVER_PORT}\``) and
webServer.url; update the webServer.command to pass `--port ${DEV_SERVER_PORT}
--strictPort` when running `npm run dev` (or replace the command with `vite
--port ${DEV_SERVER_PORT} --strictPort` if directly invoking Vite) so Vite fails
instead of auto-incrementing ports; ensure the config's use.baseURL and
webServer.url reference this constant (symbols: DEV_SERVER_PORT, use.baseURL,
webServer, webServer.command).
In `@playwright/pages/BasePage.ts`:
- Around line 59-60: The BasePage.waitForLoad() uses 'networkidle' which is
flaky as a shared default; change the implementation of BasePage.waitForLoad
(method waitForLoad in class BasePage) to use a lighter wait state such as
'load' or 'domcontentloaded' (e.g., await this.page.waitForLoadState('load'))
and update callers/tests to override waitForLoad in page-specific subclasses
when they require a stronger sentinel (e.g., polling/analytics pages should
implement their own ready sentinel or call waitForLoadState('networkidle')
locally); ensure the method signature remains async waitForLoad(): Promise<void>
and add a brief comment on BasePage explaining subclasses must override for
page-specific readiness.
In `@playwright/tests/login.spec.ts`:
- Around line 31-34: The test named 'should have proper page title' uses
page.title() and asserts expect(title).toBeTruthy(), which is too weak; replace
that assertion with a concrete expectation (e.g., expect(title).toBe('Expected
Title') for a known exact title or expect(title).toMatch(/Partial or regex
pattern/) for a stable pattern) so the test fails on incorrect but non-empty
titles; update the assertion in the test function that calls page.title()
accordingly.
- Around line 102-122: Group the three backend-dependent tests under a single
describe block and apply the skip guard once at the describe level instead of
repeating test.skip in each test: wrap the tests in describe('backend-dependent:
login tests', () => { ... }) and call test.skip(skipBackendTests, 'Skipping
backend-dependent test (SKIP_BACKEND_TESTS set).') at the start of that describe
(or use test.describe.configure({ mode: 'serial' }) with a conditional skip) so
the contained tests (those invoking loginPage.login,
loginPage.loginAndWaitForDashboard, loginPage.assertLoginSuccess,
loginPage.assertValidationError, loginPage.assertOnLoginPage) no longer need
individual skip calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7329671e-20c4-4d97-97f1-252c6d63e140
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitignorepackage.jsonplaywright.config.tsplaywright/README.mdplaywright/pages/BasePage.tsplaywright/pages/login.page.tsplaywright/tests/login.spec.tsplaywright/types/selectors.tstsconfig.playwright.json
playwright.config.ts
Outdated
| extraHTTPHeaders: { | ||
| Accept: 'application/json' | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(playwright\.config|test)" | head -20Repository: openMF/mifos-x-web-app-react
Length of output: 123
🏁 Script executed:
# Get the full context of playwright.config.ts
wc -l playwright.config.tsRepository: openMF/mifos-x-web-app-react
Length of output: 95
🏁 Script executed:
# Read the full file
cat -n playwright.config.tsRepository: openMF/mifos-x-web-app-react
Length of output: 2004
🏁 Script executed:
# Search for any references to Accept header or extraHTTPHeaders in the codebase
rg -i "accept.*header|extraHTTPHeaders" --type ts --type jsRepository: openMF/mifos-x-web-app-react
Length of output: 116
🏁 Script executed:
# Check if there are any API helpers or utilities that handle requests
fd -e ts -e js | xargs grep -l "api\|request\|fetch\|axios" | head -10Repository: openMF/mifos-x-web-app-react
Length of output: 225
🏁 Script executed:
# Check the login test to see how it uses the configuration
cat -n playwright/tests/login.spec.tsRepository: openMF/mifos-x-web-app-react
Length of output: 5594
🏁 Script executed:
# Check the HTTP client implementations
cat -n src/lib/http-client.tsRepository: openMF/mifos-x-web-app-react
Length of output: 1823
🏁 Script executed:
# Check axios configuration
cat -n src/lib/axios.tsRepository: openMF/mifos-x-web-app-react
Length of output: 1700
🏁 Script executed:
# Search for any API endpoint setup or server-side header expectations
rg -i "accept" --type ts --type js -B 2 -A 2Repository: openMF/mifos-x-web-app-react
Length of output: 479
Remove the global JSON Accept header from browser traffic.
This sets Accept: 'application/json' for all page navigations and resource requests, which doesn't match real browser behavior. More importantly, the Accept: 'application/json' header is already configured in the HTTP client (src/lib/http-client.ts, line 33) for API requests via axios, making the global setting redundant. The tests will continue working without the global header since API calls already have proper headers set through the application's HTTP client.
Proposed change
- extraHTTPHeaders: {
- Accept: 'application/json'
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extraHTTPHeaders: { | |
| Accept: 'application/json' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` around lines 31 - 33, Remove the global JSON Accept
header configuration from the Playwright config by deleting the Accept:
'application/json' entry under extraHTTPHeaders (symbol: extraHTTPHeaders) in
playwright.config.ts; tests and API calls already set appropriate headers via
the application's HTTP client (src/lib/http-client.ts) so remove this redundant
global header to restore realistic browser behavior.
playwright/pages/login.page.ts
Outdated
| const SELECTORS: LoginSelectors = { | ||
| usernameInput: 'input[name="username"]', | ||
| passwordInput: 'input[name="password"]', | ||
| loginButton: 'submit|log\\s*in', | ||
| errorMessage: '.text-red-500', | ||
| loadingIndicator: 'button[type="submit"]:disabled' | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the login page file
fd -t f login.page.tsRepository: openMF/mifos-x-web-app-react
Length of output: 102
🏁 Script executed:
# Check if file exists and get its content
cat -n playwright/pages/login.page.tsRepository: openMF/mifos-x-web-app-react
Length of output: 4927
🏁 Script executed:
# Also check the Login.tsx file to verify translation usage
cat -n src/pages/login/Login.tsx | head -250Repository: openMF/mifos-x-web-app-react
Length of output: 11152
🏁 Script executed:
# Verify the line ranges and get exact content around the mentioned lines
sed -n '41,62p' playwright/pages/login.page.tsRepository: openMF/mifos-x-web-app-react
Length of output: 666
Use locale- and style-stable selectors for the login flow.
These selectors are tied to English copy (submit|log in, remember me, forgot password, show password|hide password) or a Tailwind utility class (.text-red-500). The Login component renders most of this text through translation keys (i18n), so a locale switch or visual restyle will break the suite without any behavior regression. Prefer stable hooks like data-testid attributes, or pin the Playwright locale if English copy is intentional.
Also applies to lines: 41-43, 51-53, 56-57, 59-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/pages/login.page.ts` around lines 18 - 24, The selectors in the
SELECTORS constant (LoginSelectors) are brittle because they rely on English
text and Tailwind classes; replace those with stable, locale- and
style-independent hooks (e.g., data-testid or data-qa attributes) used by the
Login component, or pin the Playwright locale explicitly if English strings are
required. Update SELECTORS.usernameInput, .passwordInput, .loginButton,
.errorMessage, .loadingIndicator and any other selectors referenced at lines
noted (related to remember/forgot/show-password) to use data-testid/data-qa
attributes (or other semantic attributes the app exposes) and change tests to
query by those attributes (or add the attributes in the component). Ensure
LoginSelectors type matches the new keys and that tests still target the same
logical elements (usernameInput, passwordInput, loginButton, errorMessage,
loadingIndicator).
playwright/tests/login.spec.ts
Outdated
| const env = (globalThis as { process?: { env?: Record<string, string | undefined> } }).process?.env ?? {}; | ||
| const skipBackendTests = !!env.SKIP_BACKEND_TESTS; |
There was a problem hiding this comment.
Parse SKIP_BACKEND_TESTS explicitly to avoid accidental test skips.
!!env.SKIP_BACKEND_TESTS treats "false" as truthy, so backend tests are skipped when the variable is set but intended to be false.
Proposed fix
const env = (globalThis as { process?: { env?: Record<string, string | undefined> } }).process?.env ?? {};
-const skipBackendTests = !!env.SKIP_BACKEND_TESTS;
+const skipBackendTests = ['1', 'true', 'yes'].includes(
+ (env.SKIP_BACKEND_TESTS ?? '').trim().toLowerCase(),
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const env = (globalThis as { process?: { env?: Record<string, string | undefined> } }).process?.env ?? {}; | |
| const skipBackendTests = !!env.SKIP_BACKEND_TESTS; | |
| const env = (globalThis as { process?: { env?: Record<string, string | undefined> } }).process?.env ?? {}; | |
| const skipBackendTests = ['1', 'true', 'yes'].includes( | |
| (env.SKIP_BACKEND_TESTS ?? '').trim().toLowerCase(), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/tests/login.spec.ts` around lines 12 - 13, The current
skipBackendTests flag uses !!env.SKIP_BACKEND_TESTS which treats the string
"false" as truthy; change the parsing to interpret SKIP_BACKEND_TESTS explicitly
(e.g., const skipBackendTests = String(env.SKIP_BACKEND_TESTS).toLowerCase() ===
'true' or use JSON.parse with a safe fallback) so only an explicit true value
disables backend tests; update the code references to env and skipBackendTests
accordingly.
playwright/tests/login.spec.ts
Outdated
| test('should support keyboard navigation', async ({ page }) => { | ||
| await page.keyboard.press('Tab'); | ||
| await page.keyboard.press('Tab'); | ||
| await page.keyboard.press('Tab'); | ||
|
|
||
| await expect(loginPage.loginButton).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Keyboard-navigation test currently allows false positives.
This test validates visibility, not focus movement. Since the login button is already visible, broken tab order can still pass.
Suggested assertion upgrade
test('should support keyboard navigation', async ({ page }) => {
await page.keyboard.press('Tab');
await page.keyboard.press('Tab');
await page.keyboard.press('Tab');
- await expect(loginPage.loginButton).toBeVisible();
+ await expect(loginPage.loginButton).toBeFocused();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should support keyboard navigation', async ({ page }) => { | |
| await page.keyboard.press('Tab'); | |
| await page.keyboard.press('Tab'); | |
| await page.keyboard.press('Tab'); | |
| await expect(loginPage.loginButton).toBeVisible(); | |
| }); | |
| test('should support keyboard navigation', async ({ page }) => { | |
| await page.keyboard.press('Tab'); | |
| await page.keyboard.press('Tab'); | |
| await page.keyboard.press('Tab'); | |
| await expect(loginPage.loginButton).toHaveFocus(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/tests/login.spec.ts` around lines 36 - 42, The test "should
support keyboard navigation" currently checks visibility and can pass falsely;
change the final assertion to verify focus instead of visibility by replacing
expect(loginPage.loginButton).toBeVisible() with an assertion that the locator
is focused (e.g., expect(loginPage.loginButton).toBeFocused()). Ensure the test
still simulates the Tab presses (page.keyboard.press('Tab')) as before and, if
necessary, adjust the number of Tab presses so the loginPage.loginButton locator
receives focus.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/pages/login.page.ts (1)
51-63: Selectors bypass the typed SELECTORS contract, breaking the porting pattern.
rememberMeCheckbox,forgotPasswordButton, andpasswordVisibilityToggleuse hardcoded English regex patterns instead of theSELECTORSmap. This contradicts the documented principle that "only theSELECTORSblock should differ across frameworks" and theLoginSelectorsinterface doesn't include these fields.When porting to Angular, developers will need to modify these getters in addition to the
SELECTORSblock, defeating the framework-agnostic goal.♻️ Extend LoginSelectors to include all framework-specific selectors
In
types/selectors.ts:export interface LoginSelectors { usernameInput: string passwordInput: string loginButton: string errorMessage: string loadingIndicator?: string + rememberMeCheckbox: string + forgotPasswordButton: string + passwordVisibilityToggle: string }Then update
SELECTORSin this file:const SELECTORS: LoginSelectors = { usernameInput: 'input[name="username"]', passwordInput: 'input[name="password"]', loginButton: 'submit|log\\s*in', errorMessage: '.text-red-500', loadingIndicator: 'button[type="submit"]:disabled', + rememberMeCheckbox: 'remember me', + forgotPasswordButton: 'forgot password', + passwordVisibilityToggle: 'show password|hide password', }And update the getters to use
SELECTORS:get rememberMeCheckbox(): Locator { - return this.page.getByRole('checkbox', { name: /remember me/i }) + return this.page.getByRole('checkbox', { name: new RegExp(SELECTORS.rememberMeCheckbox, 'i') }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/pages/login.page.ts` around lines 51 - 63, The three getters rememberMeCheckbox, forgotPasswordButton, and passwordVisibilityToggle bypass the SELECTORS contract by using hardcoded regexes; extend the LoginSelectors interface (in types/selectors.ts) to include fields for rememberMe, forgotPassword, and passwordVisibilityToggle, add corresponding entries to the SELECTORS map in this file, and update the three getters to reference SELECTORS.rememberMe / SELECTORS.forgotPassword / SELECTORS.passwordVisibilityToggle (or the exact names you add) so all framework-specific selectors live only in the SELECTORS block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@playwright/pages/login.page.ts`:
- Around line 51-63: The three getters rememberMeCheckbox, forgotPasswordButton,
and passwordVisibilityToggle bypass the SELECTORS contract by using hardcoded
regexes; extend the LoginSelectors interface (in types/selectors.ts) to include
fields for rememberMe, forgotPassword, and passwordVisibilityToggle, add
corresponding entries to the SELECTORS map in this file, and update the three
getters to reference SELECTORS.rememberMe / SELECTORS.forgotPassword /
SELECTORS.passwordVisibilityToggle (or the exact names you add) so all
framework-specific selectors live only in the SELECTORS block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dd0af77-ec29-42f6-b708-c5ef78cc969b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitignorepackage.jsonplaywright.config.tsplaywright/README.mdplaywright/pages/BasePage.tsplaywright/pages/login.page.tsplaywright/tests/login.spec.tsplaywright/types/selectors.tstsconfig.playwright.json
🚧 Files skipped from review as they are similar to previous changes (7)
- playwright.config.ts
- .gitignore
- playwright/pages/BasePage.ts
- package.json
- playwright/types/selectors.ts
- tsconfig.playwright.json
- playwright/tests/login.spec.ts
1 participant
Description
This PR introduces a framework-agnostic E2E testing foundation for the React web app to improve regression safety and enable scalable cross-framework test reuse.
The main goal is to keep tests stable as the product grows while minimizing maintenance cost between Angular and React implementations.
To support this, the PR establishes a typed Page Object + selector-contract pattern where test intent remains shared and only selector mappings differ per framework.
This also adds higher-value login coverage beyond basic form checks, including:
remember-me behavior
password visibility toggle behavior
persistence checks for tenant/server local state
These additions were made because login state and tenant context are critical in Mifos workflows and are common regression points in multi-tenant environments.
The PR also introduces CI-friendly behavior so backend-dependent tests can be conditionally skipped via environment flag, allowing the same suite to serve both lightweight UI validation and full-stack runs.
Related issues and discussion
##MXWAR-76
Screenshots, if any
Screen.Recording.2026-03-18.at.1.19.56.AM.mov
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
CONTRIBUTING.md.Summary by CodeRabbit
Documentation
Tests
New Features
Chores