Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Playwright E2E infrastructure and Docker config, new E2E tests and page objects for disclaimer, Data Hub, and start-session flows, minor testability attributes on the Disclaimer UI, and fixture updates that add page model mappings and auth polling. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant App as Web App
participant Storage as localStorage/Auth
Browser->>App: Navigate to /dashboard (auth injected)
App->>Storage: Read disclaimer acceptance flag
alt not accepted
App->>Browser: Render Disclaimer dialog
Browser->>App: Click "Accept"
App->>Storage: Set disclaimer accepted
App->>Browser: Close dialog, render Dashboard
else accepted
App->>Browser: Render Dashboard
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
david-roper
force-pushed
the
e2e-testing
branch
from
25cbf2b to
55aa5c0
Compare
david-roper
force-pushed
the
e2e-testing
branch
from
7a1a5cd to
7b6c4ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
apps/web/src/providers/DisclaimerProvider.tsx (1)
17-18: Inconsistent test ID casing:"Disclaimer-dialog"vs"accept-disclaimer".
Disclaimer-dialog/Disclaimer-dialog-contentuse a PascalCase prefix while the button IDs are fully lowercase kebab. Pick one convention and apply it across all test IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/providers/DisclaimerProvider.tsx` around lines 17 - 18, Test IDs in the DisclaimerProvider are inconsistent: the Dialog and Dialog.Content use PascalCase IDs ("Disclaimer-dialog", "Disclaimer-dialog-content") while the buttons use lowercase kebab ("accept-disclaimer"); update the Dialog data-test-id attributes used in the JSX (the <Dialog ...> and <Dialog.Content ...> elements around isDisclaimerAccepted) to match the kebab-case convention (e.g., "disclaimer-dialog" and "disclaimer-dialog-content") so all test IDs are consistent.testing/e2e/src/pages/start-session.page.ts (1)
30-30: Hardcoded DOB and session date in both fill methods.Both
fillCustomIdentifierandfillSessionFormhardcode'1990-01-01'for DOB and'2026-01-01'for session date. These values are duplicated across two methods — extracting them as class-level constants (or parameters with defaults) would reduce future maintenance friction.♻️ Suggested refactor
+ private static readonly DEFAULT_DOB = '1990-01-01'; + private static readonly DEFAULT_SESSION_DATE = '2026-01-01'; async fillCustomIdentifier(customIdentifier: string, sex: string) { ... - await dateOfBirthField.fill('1990-01-01'); + await dateOfBirthField.fill(StartSessionPage.DEFAULT_DOB); ... - await sessionDate.fill('2026-01-01'); + await sessionDate.fill(StartSessionPage.DEFAULT_SESSION_DATE); } async fillSessionForm(firstName: string, lastName: string, sex: string) { ... - await dateOfBirthField.fill('1990-01-01'); + await dateOfBirthField.fill(StartSessionPage.DEFAULT_DOB); ... - await sessionDate.fill('2026-01-01'); + await sessionDate.fill(StartSessionPage.DEFAULT_SESSION_DATE); }Also applies to: 37-37, 56-56, 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/pages/start-session.page.ts` at line 30, Both fillCustomIdentifier and fillSessionForm currently hardcode dates ('1990-01-01' and '2026-01-01') in calls like dateOfBirthField.fill and sessionDateField.fill; extract these literals to single sources of truth by introducing class-level constants (e.g., DEFAULT_DOB and DEFAULT_SESSION_DATE) or method parameters with defaults and replace the hardcoded strings in fillCustomIdentifier and fillSessionForm (and any other occurrences at the noted locations) to use those constants/parameters so the values are not duplicated.testing/e2e/src/pages/datahub/datahub.page.ts (1)
7-7:rowActionsTriggeris currently dead code.The only usage site is commented out in
2.2-datahub.spec.ts. Either remove it now or re-enable the assertion — keeping an unused public locator adds noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/pages/datahub/datahub.page.ts` at line 7, The public Locator property rowActionsTrigger in datahub.page.ts is dead code; remove the declaration (readonly rowActionsTrigger) from the Datahub page class OR re-enable the commented assertion in 2.2-datahub.spec.ts that references it so the locator is exercised; update any imports/usages accordingly and run tests to ensure no remaining references to rowActionsTrigger cause compile errors.testing/e2e/src/helpers/fixtures.ts (1)
60-65: Auth-file poll has unexplained magic numbers.50 attempts × 100 ms = 5 s max wait. Fine as-is, but extracting the constants with names makes the intent clearer and easier to adjust later.
♻️ Suggested refactor
+ const MAX_ATTEMPTS = 50; + const POLL_INTERVAL_MS = 100; let attempts = 0; - while (!fs.existsSync(authStorageFile) && attempts < 50) { - await new Promise((resolve) => setTimeout(resolve, 100)); + while (!fs.existsSync(authStorageFile) && attempts < MAX_ATTEMPTS) { + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); attempts++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/helpers/fixtures.ts` around lines 60 - 65, The polling loop that waits for authStorageFile uses unexplained magic numbers (50 and 100) in the while loop; replace them with named constants (e.g., const MAX_ATTEMPTS and const POLL_INTERVAL_MS) defined near the top of the function/module and use them in the loop (while (!fs.existsSync(authStorageFile) && attempts < MAX_ATTEMPTS) and await new Promise(r => setTimeout(r, POLL_INTERVAL_MS))); keep the attempts counter and behavior the same but use the named constants so intent and adjustability are clear, and update any related comments to reference the constants instead of raw numbers.testing/e2e/src/2.2-datahub.spec.ts (1)
8-8: Remove the commented-out assertion or add a tracking note.
//await expect(datahubPage.rowActionsTrigger).toBeVisible();is dead test code. Either restore it with a real assertion or delete it (and the correspondingrowActionsTriggerlocator onDatahubPage).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/2.2-datahub.spec.ts` at line 8, The commented-out assertion `//await expect(datahubPage.rowActionsTrigger).toBeVisible();` is dead test code—either re-enable it with a real expectation or remove it and its associated locator; specifically, either restore the assertion in the spec using `datahubPage.rowActionsTrigger` (ensuring the locator on the DatahubPage class is valid and used), or delete this commented line and remove the `rowActionsTrigger` locator from the DatahubPage class to avoid unused code and keep tests clean.testing/e2e/src/1.2-accept-disclaimer.spec.ts (1)
12-12: Hardcoded localStorage schema is fragile.
{ state: { isDisclaimerAccepted: false }, version: 1 }mirrors the app's Zustand persist schema. If the store key, version number, or state shape changes, the app will silently ignore this value (or migrate it), the disclaimer may not appear, and the test will continue to pass—for the wrong reasons.Consider driving this string from a shared constant exported alongside the app store, or at minimum add a comment pointing to the exact source location so a schema change prompts a test update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/1.2-accept-disclaimer.spec.ts` at line 12, The test writes a fragile hardcoded persisted schema via localStorage.setItem('app', JSON.stringify({ state: { isDisclaimerAccepted: false }, version: 1 })); instead of mirroring the app store; change the test to import the store's canonical persist key and default persisted state (e.g., APP_PERSIST_KEY and DEFAULT_PERSIST_STATE or an exported helper from the app store module) and use JSON.stringify to set that value while overriding state.isDisclaimerAccepted = false; if a shared constant cannot be added yet, add a one-line comment pointing to the exact exported store symbol that defines the persist schema so future schema changes trigger test updates.testing/e2e/src/2.3-start-session.spec.ts (2)
12-17: Extract duplicatedaddInitScriptlocalStorage setup tobeforeEach.The identical block is copy-pasted into two tests. A
beforeEachinside a merged describe block removes the duplication and ensures consistent setup.♻️ Proposed refactor
test.describe('start session', () => { + test.beforeEach(async ({ page }) => { + await page.addInitScript(() => { + localStorage.setItem( + 'app', + JSON.stringify({ state: { isDisclaimerAccepted: true, isWalkthroughComplete: true }, version: 1 }) + ); + }); + }); + test('should fill subject personal information input', async ({ getPageModel, page }) => { - await page.addInitScript(() => { - localStorage.setItem( - 'app', - JSON.stringify({ state: { isDisclaimerAccepted: true, isWalkthroughComplete: true }, version: 1 }) - ); - }); ... }); test('should fill custom identifier input', async ({ getPageModel, page }) => { - await page.addInitScript(() => { - localStorage.setItem( - 'app', - JSON.stringify({ state: { isDisclaimerAccepted: true, isWalkthroughComplete: true }, version: 1 }) - ); - }); ... }); });Also applies to: 57-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/2.3-start-session.spec.ts` around lines 12 - 17, Two tests duplicate the same page.addInitScript localStorage setup; move that initialization into a shared beforeEach to remove duplication and ensure consistency. Add a beforeEach in the surrounding describe block that calls page.addInitScript(...) to set localStorage with the JSON ({ state: { isDisclaimerAccepted: true, isWalkthroughComplete: true }, version: 1 }) so both tests reuse the setup, and remove the duplicate page.addInitScript calls from the individual tests; reference the existing addInitScript usage and the test-level page variable when implementing the beforeEach.
72-72: Stale comment — copy-pasted from the personal-info test.
// Fill the subject first name fielddescribes the wrong action; it should reference the custom identifier.- // Fill the subject first name field + // Fill the custom identifier field🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/2.3-start-session.spec.ts` at line 72, Replace the stale comment "// Fill the subject first name field" in the 2.3-start-session.spec.ts test with a comment that correctly describes the action for the custom identifier (e.g., "// Fill the subject custom identifier field" or "// Fill the custom identifier field") so the comment matches the code that fills the custom identifier input in this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testing/e2e/playwright.config.ts`:
- Line 84: The webServer.timeout value in the Playwright config is set to 5_000
which is too short for CI; update the webServer.timeout (and the other
occurrences referenced) to a higher default (e.g., 60_000) and make it
configurable via an environment variable (PLAYWRIGHT_SERVER_TIMEOUT) by
reading/parsing the env var and falling back to 60_000, replacing the hard-coded
5_000 literal used in the Playwright config's webServer.timeout settings so CI
has enough time to start API/Gateway/Web services.
In `@testing/e2e/playwright.docker.config.ts`:
- Around line 71-87: The Playwright webServer entries currently omit
reuseExistingServer, causing Playwright to error if services are already
running; update each webServer object in the webServer array (the three entries
that use command: 'true' and urls referencing appPort and gatewayPort) to
include reuseExistingServer: true so Playwright will reuse the existing services
instead of attempting to bind the URLs.
In `@testing/e2e/src/2.2-datahub.spec.ts`:
- Around line 3-4: The test suite's test.describe label currently reads
'dashboard' but the tests exercise /datahub, so update the test.describe call to
'datahub' and also rename the individual test title in the test('should display
the dashboard header', ...) to 'should display the Data Hub header' so the suite
and test names match the /datahub behavior (locate the test.describe and the
test(...) invocation in the file).
In `@testing/e2e/src/2.3-start-session.spec.ts`:
- Line 3: Two test suites use the identical title test.describe('start
session'), causing ambiguous suite paths; either merge the two
test.describe('start session') blocks into a single suite (moving the contained
tests into one describe) or rename one of them to a unique title (e.g.,
test.describe('start session - <context>')) and update any nested test titles
accordingly so Playwright reports distinct suite paths; locate the duplicate
blocks by searching for test.describe('start session') and apply the merge or
rename to the block(s) found.
- Line 47: The test currently asserts a hard-coded session date using the
literal in the expect call (await
expect(sessionDate).toHaveValue('2026-01-01')), which will break as time
advances; change the assertion to compute the expected date dynamically (e.g.,
obtain today's date in the test, format it to YYYY-MM-DD) and use that computed
value in the toHaveValue check instead of the hard-coded string, and update the
second occurrence at the other assertion (line ~83) as well—look for the symbols
sessionDate and the toHaveValue assertions in 2.3-start-session.spec.ts and
replace the literal with a variable like expectedDate derived from new Date()
(or your project date util) before the expectation.
- Line 8: The assertion expect(startSessionPage.sessionForm).toBeDefined() is
ineffective because Playwright Locator is always truthy; replace it with an
async Playwright assertion that checks DOM state, e.g. await
expect(startSessionPage.sessionForm).toBeVisible() (or await
expect(startSessionPage.sessionForm).toHaveCount(1) if there may be multiple),
ensuring the surrounding test function is async and you import/use Playwright's
expect so the awaitable matcher runs against startSessionPage.sessionForm.
---
Nitpick comments:
In `@apps/web/src/providers/DisclaimerProvider.tsx`:
- Around line 17-18: Test IDs in the DisclaimerProvider are inconsistent: the
Dialog and Dialog.Content use PascalCase IDs ("Disclaimer-dialog",
"Disclaimer-dialog-content") while the buttons use lowercase kebab
("accept-disclaimer"); update the Dialog data-test-id attributes used in the JSX
(the <Dialog ...> and <Dialog.Content ...> elements around isDisclaimerAccepted)
to match the kebab-case convention (e.g., "disclaimer-dialog" and
"disclaimer-dialog-content") so all test IDs are consistent.
In `@testing/e2e/src/1.2-accept-disclaimer.spec.ts`:
- Line 12: The test writes a fragile hardcoded persisted schema via
localStorage.setItem('app', JSON.stringify({ state: { isDisclaimerAccepted:
false }, version: 1 })); instead of mirroring the app store; change the test to
import the store's canonical persist key and default persisted state (e.g.,
APP_PERSIST_KEY and DEFAULT_PERSIST_STATE or an exported helper from the app
store module) and use JSON.stringify to set that value while overriding
state.isDisclaimerAccepted = false; if a shared constant cannot be added yet,
add a one-line comment pointing to the exact exported store symbol that defines
the persist schema so future schema changes trigger test updates.
In `@testing/e2e/src/2.2-datahub.spec.ts`:
- Line 8: The commented-out assertion `//await
expect(datahubPage.rowActionsTrigger).toBeVisible();` is dead test code—either
re-enable it with a real expectation or remove it and its associated locator;
specifically, either restore the assertion in the spec using
`datahubPage.rowActionsTrigger` (ensuring the locator on the DatahubPage class
is valid and used), or delete this commented line and remove the
`rowActionsTrigger` locator from the DatahubPage class to avoid unused code and
keep tests clean.
In `@testing/e2e/src/2.3-start-session.spec.ts`:
- Around line 12-17: Two tests duplicate the same page.addInitScript
localStorage setup; move that initialization into a shared beforeEach to remove
duplication and ensure consistency. Add a beforeEach in the surrounding describe
block that calls page.addInitScript(...) to set localStorage with the JSON ({
state: { isDisclaimerAccepted: true, isWalkthroughComplete: true }, version: 1
}) so both tests reuse the setup, and remove the duplicate page.addInitScript
calls from the individual tests; reference the existing addInitScript usage and
the test-level page variable when implementing the beforeEach.
- Line 72: Replace the stale comment "// Fill the subject first name field" in
the 2.3-start-session.spec.ts test with a comment that correctly describes the
action for the custom identifier (e.g., "// Fill the subject custom identifier
field" or "// Fill the custom identifier field") so the comment matches the code
that fills the custom identifier input in this test.
In `@testing/e2e/src/helpers/fixtures.ts`:
- Around line 60-65: The polling loop that waits for authStorageFile uses
unexplained magic numbers (50 and 100) in the while loop; replace them with
named constants (e.g., const MAX_ATTEMPTS and const POLL_INTERVAL_MS) defined
near the top of the function/module and use them in the loop (while
(!fs.existsSync(authStorageFile) && attempts < MAX_ATTEMPTS) and await new
Promise(r => setTimeout(r, POLL_INTERVAL_MS))); keep the attempts counter and
behavior the same but use the named constants so intent and adjustability are
clear, and update any related comments to reference the constants instead of raw
numbers.
In `@testing/e2e/src/pages/datahub/datahub.page.ts`:
- Line 7: The public Locator property rowActionsTrigger in datahub.page.ts is
dead code; remove the declaration (readonly rowActionsTrigger) from the Datahub
page class OR re-enable the commented assertion in 2.2-datahub.spec.ts that
references it so the locator is exercised; update any imports/usages accordingly
and run tests to ensure no remaining references to rowActionsTrigger cause
compile errors.
In `@testing/e2e/src/pages/start-session.page.ts`:
- Line 30: Both fillCustomIdentifier and fillSessionForm currently hardcode
dates ('1990-01-01' and '2026-01-01') in calls like dateOfBirthField.fill and
sessionDateField.fill; extract these literals to single sources of truth by
introducing class-level constants (e.g., DEFAULT_DOB and DEFAULT_SESSION_DATE)
or method parameters with defaults and replace the hardcoded strings in
fillCustomIdentifier and fillSessionForm (and any other occurrences at the noted
locations) to use those constants/parameters so the values are not duplicated.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
david-roper
force-pushed
the
e2e-testing
branch
from
d51672b to
f0e8fdc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
testing/e2e/src/pages/datahub/datahub.page.ts (1)
6-6:pageHeaderis duplicated across page objects — consider hoisting toAppPage.Both
DatahubPageandStartSessionPagedeclare an identicalpage.getByTestId('page-header')locator. IfAppPagedoesn't already expose it, moving it there eliminates the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/pages/datahub/datahub.page.ts` at line 6, Both DatahubPage and StartSessionPage declare an identical locator (pageHeader = page.getByTestId('page-header')); hoist this shared Locator to the base AppPage instead: add a readonly pageHeader: Locator property and initialize it from page.getByTestId('page-header') in AppPage's constructor, then remove the duplicate pageHeader declarations from DatahubPage and StartSessionPage so they inherit the shared locator; update any references in those classes to use the inherited pageHeader.testing/e2e/src/pages/start-session.page.ts (1)
19-66: Extract the shared tail of bothfill*methods into a private helper.Lines 21–38 of
fillCustomIdentifierand lines 45–65 offillSessionFormare near-identical:dateOfBirthField(hardcoded'1990-01-01'),sexSelector,sessionTypeSelector(hardcoded'Retrospective'), andsessionDate(today). Any future change — e.g. parameterising the DOB or session type — needs to land in two places.♻️ Proposed refactor
+ private async fillCommonSessionFields(sex: string): Promise<void> { + const dateOfBirthField = this.sessionForm.locator('[name="subjectDateOfBirth"]'); + const sexSelector = this.sessionForm.locator('[name="subjectSex"]'); + const sessionTypeSelector = this.sessionForm.locator('[name="sessionType"]'); + const sessionDate = this.sessionForm.locator('[name="sessionDate"]'); + + await dateOfBirthField.waitFor({ state: 'visible' }); + await dateOfBirthField.fill('1990-01-01'); + + await sexSelector.selectOption(sex); + await sessionTypeSelector.selectOption('Retrospective'); + + await sessionDate.waitFor({ state: 'visible' }); + await sessionDate.fill(new Date().toISOString().split('T')[0]!); + } + async fillCustomIdentifier(customIdentifier: string, sex: string): Promise<void> { const subjectIdField = this.sessionForm.locator('[name="subjectId"]'); - const dateOfBirthField = this.sessionForm.locator('[name="subjectDateOfBirth"]'); - const sexSelector = this.sessionForm.locator('[name="subjectSex"]'); - const sessionTypeSelector = this.sessionForm.locator('[name="sessionType"]'); - const sessionDate = this.sessionForm.locator('[name="sessionDate"]'); await subjectIdField.waitFor({ state: 'visible' }); await subjectIdField.fill(customIdentifier); - await dateOfBirthField.waitFor({ state: 'visible' }); - await dateOfBirthField.fill('1990-01-01'); - await sexSelector.selectOption(sex); - await sessionTypeSelector.selectOption('Retrospective'); - await sessionDate.waitFor({ state: 'visible' }); - const expectedSessionDate = new Date().toISOString().split('T')[0]!; - await sessionDate.fill(expectedSessionDate); + await this.fillCommonSessionFields(sex); } async fillSessionForm(firstName: string, lastName: string, sex: string): Promise<void> { const firstNameField = this.sessionForm.locator('[name="subjectFirstName"]'); const lastNameField = this.sessionForm.locator('[name="subjectLastName"]'); - const dateOfBirthField = this.sessionForm.locator('[name="subjectDateOfBirth"]'); - const sexSelector = this.sessionForm.locator('[name="subjectSex"]'); - const sessionTypeSelector = this.sessionForm.locator('[name="sessionType"]'); - const sessionDate = this.sessionForm.locator('[name="sessionDate"]'); await firstNameField.waitFor({ state: 'visible' }); await firstNameField.fill(firstName); await lastNameField.waitFor({ state: 'visible' }); await lastNameField.fill(lastName); - await dateOfBirthField.waitFor({ state: 'visible' }); - await dateOfBirthField.fill('1990-01-01'); - await sexSelector.selectOption(sex); - await sessionTypeSelector.selectOption('Retrospective'); - await sessionDate.waitFor({ state: 'visible' }); - const expectedSessionDate = new Date().toISOString().split('T')[0]!; - await sessionDate.fill(expectedSessionDate); + await this.fillCommonSessionFields(sex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/pages/start-session.page.ts` around lines 19 - 66, Both fillCustomIdentifier and fillSessionForm duplicate the same tail logic; extract the repeated steps (setting dateOfBirthField to '1990-01-01', selecting sex via sexSelector, selecting sessionType 'Retrospective' via sessionTypeSelector, and filling sessionDate with today's date) into a private helper (e.g., setCommonSessionFields or fillSharedSessionTail) and call it from both fillCustomIdentifier and fillSessionForm; the helper should accept the sex value (and optionally DOB/sessionType if you want them parameterised later), locate/waitFor the dateOfBirthField, sexSelector, sessionTypeSelector, and sessionDate, perform the same await/fill/selectOption operations, and return when done so both original methods only handle their unique field logic before delegating to the helper.testing/e2e/playwright.docker.config.ts (1)
31-31:maxFailures: 1will abort the full run on the first flaky test — consider settingretriesalongside it.With
workers: 1and noretries, a single transient failure halts the entire suite, leaving downstream pipelines unexercised. Settingretries: 1orretries: 2lets Playwright distinguish actual failures from noise before hitting themaxFailuresgate, which is especially valuable in Docker where startup jitter is common.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/playwright.docker.config.ts` at line 31, The Playwright config currently sets maxFailures: 1 which will abort the entire run on the first transient failure; update the test runner configuration near the maxFailures setting (and check the workers setting) to add retries: 1 (or 2) so flaky/docker-startup jitter is retried before maxFailures triggers, ensuring better pipeline resilience—look for the maxFailures property and the workers property in the Playwright config to add the retries key alongside them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testing/e2e/src/helpers/fixtures.ts`:
- Around line 61-65: The hardcoded polling loop that waits for authStorageFile
(the while loop using attempts and setTimeout) has a fixed ceiling of 50×100ms
(5s) which is too tight for slow CI; refactor to replace the literals with
configurable values (e.g. MAX_POLL_ATTEMPTS and POLL_INTERVAL_MS or read from
env like E2E_POLL_INTERVAL_MS/E2E_MAX_ATTEMPTS) and use those variables in the
loop so tests can increase the wait in CI/Docker without changing code; update
any related tests/helpers to use the new constants/config and keep the same loop
logic around authStorageFile and attempts.
In `@testing/e2e/src/pages/start-session.page.ts`:
- Around line 77-78: The test currently forces the click on submitButton (await
submitButton.click({ force: true })), which hides real UI issues; replace the
forced click with a stable sequence: wait for any known overlay/animation to
disappear (e.g., wait for the overlay selector to be hidden or for submitButton
to be actionable/visible/enabled) and then perform a normal click on
submitButton without force; locate the use of submitButton.click in
start-session.page.ts and update it to explicitly wait for the overlay/animation
or use Playwright's waitFor/expect to ensure actionability before calling
submitButton.click().
---
Nitpick comments:
In `@testing/e2e/playwright.docker.config.ts`:
- Line 31: The Playwright config currently sets maxFailures: 1 which will abort
the entire run on the first transient failure; update the test runner
configuration near the maxFailures setting (and check the workers setting) to
add retries: 1 (or 2) so flaky/docker-startup jitter is retried before
maxFailures triggers, ensuring better pipeline resilience—look for the
maxFailures property and the workers property in the Playwright config to add
the retries key alongside them.
In `@testing/e2e/src/pages/datahub/datahub.page.ts`:
- Line 6: Both DatahubPage and StartSessionPage declare an identical locator
(pageHeader = page.getByTestId('page-header')); hoist this shared Locator to the
base AppPage instead: add a readonly pageHeader: Locator property and initialize
it from page.getByTestId('page-header') in AppPage's constructor, then remove
the duplicate pageHeader declarations from DatahubPage and StartSessionPage so
they inherit the shared locator; update any references in those classes to use
the inherited pageHeader.
In `@testing/e2e/src/pages/start-session.page.ts`:
- Around line 19-66: Both fillCustomIdentifier and fillSessionForm duplicate the
same tail logic; extract the repeated steps (setting dateOfBirthField to
'1990-01-01', selecting sex via sexSelector, selecting sessionType
'Retrospective' via sessionTypeSelector, and filling sessionDate with today's
date) into a private helper (e.g., setCommonSessionFields or
fillSharedSessionTail) and call it from both fillCustomIdentifier and
fillSessionForm; the helper should accept the sex value (and optionally
DOB/sessionType if you want them parameterised later), locate/waitFor the
dateOfBirthField, sexSelector, sessionTypeSelector, and sessionDate, perform the
same await/fill/selectOption operations, and return when done so both original
methods only handle their unique field logic before delegating to the helper.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/src/providers/DisclaimerProvider.tsxtesting/e2e/playwright.docker.config.tstesting/e2e/src/1.2-accept-disclaimer.spec.tstesting/e2e/src/2.2-datahub.spec.tstesting/e2e/src/2.3-start-session.spec.tstesting/e2e/src/helpers/fixtures.tstesting/e2e/src/pages/datahub/datahub.page.tstesting/e2e/src/pages/start-session.page.tstesting/e2e/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- testing/e2e/src/2.2-datahub.spec.ts
- apps/web/src/providers/DisclaimerProvider.tsx
- testing/e2e/tsconfig.json
- testing/e2e/src/1.2-accept-disclaimer.spec.ts
- testing/e2e/src/2.3-start-session.spec.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works on a playwright e2e test suite for ODC
Summary by CodeRabbit
Tests
Chores