fix(onboarding): ensure lastActiveTeamId is persisted before redirecting after T&C#8931
Open
kiran-redhat wants to merge 26 commits intomainfrom
Open
fix(onboarding): ensure lastActiveTeamId is persisted before redirecting after T&C#8931kiran-redhat wants to merge 26 commits intomainfrom
kiran-redhat wants to merge 26 commits intomainfrom
Conversation
…o suites
Root causes addressed (e2e/page objects only — no app code changed):
1. LoginPage.waitUntilDiscoverPageLoaded: was asserting JourneysAdminContainedIconButton
directly, which is only rendered when an active team exists. Now waits for
NavigationListItemProjects first (reliable shell signal), then auto-selects
the first real team via the TeamSelect dropdown when needed, before asserting
the button. This unblocks profile, analytics, and logout tests.
2. CustomizationMediaPage.clickNextButton: was calling click() on
CustomizeFlowNextButton without first waiting for it to be enabled. The
Language screen starts with the button disabled while data loads. Now waits
for toBeEnabled() before clicking.
3. youtube-video.spec.ts: template 8d4c24c3-5fe0-428d-b221-af9e46975933 no
longer exists on the daily-e2e deployment. Updated TEMPLATE_ID to
00dc45d7-9d37-434e-bbc8-7c89eeb6229a ("Youtube Video Upload" template that
has customizable media in the flow).
4. customization-media-page.ts waitForAutoSubmitError: selector
'.Mui-error, .MuiFormHelperText-root.Mui-error' matched both the input
container div and the helper text p, causing a strict mode violation.
Narrowed to p.MuiFormHelperText-root.Mui-error.
5. journey-page.ts clickAnalyticsIconInCustomJourneyPage: AnalyticsItem
locator matched multiple elements when the account had several journeys.
Now scoped to the journey card containing existingJourneyName.
6. profile-page.ts verifyLogoutToastMsg: the 5s default timeout was too short.
The toast appears and disappears before page navigates to sign-in.
Wrapped in try/catch with 10s timeout; verifyloggedOut() remains the
authoritative logout assertion.
Made-with: Cursor
Replaced the internal data-testid selector with getByRole('heading', { name:
'Create Custom Journey' }) so the locator targets the text the user sees in
the UI, following the rule that element references should use their visible
label rather than implementation-level test IDs.
Made-with: Cursor
Remove all defensive patterns (fallback loops, try/catch guards, .first() escape hatches) from e2e page objects and replace with direct assertions that reflect exact user expectations. Add .cursor/rules/e2e-testing.mdc to enforce this standard going forward. Made-with: Cursor
The app logout() immediately navigates via window.location.href with no enqueueSnackbar call, so no toast is ever shown. Remove verifyLogoutToastMsg and its call site; verifyloggedOut() is the correct requirement check. Made-with: Cursor
After OTP validation, router.push triggers SSR for terms-and-conditions which chains validateEmail mutation → initAndAuthApp → checkConditionalRedirect with multiple DB round-trips. On cold Vercel serverless functions this exceeds 60s. Raise the assertion timeout to 90s to match real startup time. Made-with: Cursor
- Raise waitUntilDiscoverPageLoaded to 90s in login-page and register-Page to handle cold Vercel SSR + TeamProvider Apollo query on first hit - Lower clickCreateCustomJourney from 150s to 90s to comply with new rule - Raise waitForAutoSubmitError to 90s for cold-start YouTube validation delay - Remove unused seventySecondsTimeout constant from register-Page - Add wait-time rule to e2e-testing.mdc: max 90s, comment required on any timeout that exceeds the default 30s Made-with: Cursor
Add explicit rule that page.waitForTimeout and setTimeout are forbidden. All synchronisation must use Playwright's built-in auto-waiting assertions. Made-with: Cursor
- global-setup: hit root + sign-in 3x in parallel so multiple Lambda instances are warm before any beforeAll hook runs, reducing cold-start flakiness for waitUntilDiscoverPageLoaded - journey-page: add explicit toBeVisible(90s) before clickShareButtonInJourneyPage so the 20s global actionTimeout is not exceeded on cold journey page load Made-with: Cursor
- Updated e2e-testing.mdc to mandate the use of Playwright's auto-waiting for all waits, banning hard waits and providing clear examples of good and bad practices. - Simplified global setup by removing redundant parallel health checks for sign-in, ensuring only the base URL is checked for readiness. Made-with: Cursor
…-level actions - Updated image selection and verification methods to improve clarity and reliability, replacing generic checks with specific assertions for custom and gallery images. - Refactored button click logic in the add block drawer to use precise test IDs, reducing ambiguity and improving test accuracy. - Simplified image source retrieval by introducing a dedicated method for handling image library thumbnails. Made-with: Cursor
…fix-more-e2e-failing-tests
…ing after T&C - In TermsAndConditions, await the updateLastActiveTeamId DB mutation before router.push. Previously it ran inside Promise.allSettled, so on cold Vercel instances the write could be silently missed and the next page would load with activeTeam === null (missing “Create Custom Journey”). - In journeys-admin-e2e, update the login/register “discover page loaded” waits to assert the app shell is ready (nav visible + TeamSelect enabled) and, for new users, require “Create Custom Journey” to be enabled (timeout justified for cold starts).
…ing after T&C - In TermsAndConditions, await the updateLastActiveTeamId DB mutation before router.push. Previously it ran inside Promise.allSettled, so on cold Vercel instances the write could be silently missed and the next page would load with activeTeam === null (missing “Create Custom Journey”). - In journeys-admin-e2e, update the login/register “discover page loaded” waits to assert the app shell is ready (nav visible + TeamSelect enabled) and, for new users, require “Create Custom Journey” to be enabled (timeout justified for cold starts).
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:
WalkthroughE2E image assertions replaced with URL-pattern checks; login/register flows tightened and RNG entropy increased; several test timeouts extended; onboarding and terms flows now await DB persistence before navigation; TeamProvider gains single-team recovery logic and related tests/mocks updated. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Onboarding/Terms Component
participant API as GraphQL Mutations
participant Router as Next/router
participant Query as Apollo query.refetch
UI->>API: await journeyDuplicate()
API-->>UI: duplication result
UI->>API: await updateLastActiveTeamId(teamId)
API-->>UI: mutation completes
UI->>Router: router.push(redirect or onboarding target)
UI->>Query: query.refetch()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 21faf63
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
![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: 1
🧹 Nitpick comments (4)
apps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx (1)
66-67: Wrap async flow intry/finallyso loading state always recovers.With
updateLastActiveTeamIdnow awaited, a mutation failure can leaveloadingstucktruebecausesetLoading(false)runs only on the success path.♻️ Proposed refactor
- setLoading(true) - await journeyProfileCreate() + setLoading(true) + try { + await journeyProfileCreate() @@ - await updateLastActiveTeamId({ - variables: { - input: { lastActiveTeamId: teamId } - } - }) + await updateLastActiveTeamId({ + variables: { + input: { lastActiveTeamId: teamId } + } + }) @@ - setActiveTeam(team) - } - setLoading(false) + setActiveTeam(team) + } + } finally { + setLoading(false) + } }Also applies to: 106-115, 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx` around lines 66 - 67, The loading state set by setLoading(true) around the async calls (e.g., journeyProfileCreate and updateLastActiveTeamId) can remain true on error; wrap the async flow in a try/finally so setLoading(false) always runs. Locate the blocks in TermsAndConditions.tsx where setLoading(true) precedes await journeyProfileCreate() and where updateLastActiveTeamId is awaited (also the blocks around lines 106-115 and 133) and change them to use try { await ... } finally { setLoading(false) } while preserving any success-path logic and error propagation.apps/journeys-admin-e2e/src/pages/card-level-actions.ts (1)
71-85: Avoid expanding test-id-only targeting for Add Block optionsThis change makes selection more deterministic, but it doubles down on
data-testid-driven selectors in e2e flows. Prefer scoping to the visible drawer and clicking by role/name (“Image”, “Spacer”, etc.) so selectors track what users see.As per coding guidelines:
apps/**/*e2e*/**/*.ts: Reference elements by the label the user sees in the UI — use getByRole, getByPlaceholder, and similar accessible queries instead of CSS selectors, data-testid, or class names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts` around lines 71 - 85, The test currently forces data-testid-driven selection using testIdMap and this.page.locator which violates e2e accessibility guidelines; replace the data-testid logic so you scope to the visible settings drawer (use an accessible query to find the drawer, e.g., getByRole('dialog') or getByRole with the drawer's accessible name) and then find the add-block button by role and accessible name (use getByRole('button', { name: buttonName }) or locator.getByRole) instead of using testIdMap or CSS data-testid selectors; update the code that computes button (the testIdMap, testId, and button variables) to instead locate the drawer element and call drawer.getByRole('button', { name: buttonName }) so the selector matches what users see.apps/journeys-admin-e2e/src/pages/register-Page.ts (1)
171-172: Use visible “Projects” role query instead of test id hereThis readiness check should use a user-facing selector (role/name) instead of
getByTestId('NavigationListItemProjects')to match the e2e selector standard.As per coding guidelines:
apps/**/*e2e*/**/*.ts: Reference elements by the label the user sees in the UI — use getByRole, getByPlaceholder, and similar accessible queries instead of CSS selectors, data-testid, or class names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/register-Page.ts` around lines 171 - 172, The readiness check currently uses this.page.getByTestId('NavigationListItemProjects'); replace that data-testid selector with an accessible user-facing query such as this.page.getByRole('link', { name: /Projects/i }).toBeVisible({ timeout: 90000 }) so the e2e uses the visible label; update the assertion in the same method (where this.page.getByTestId('NavigationListItemProjects') appears) to use getByRole/getByText with the "Projects" name.apps/journeys-admin-e2e/src/pages/login-page.ts (1)
36-37: Prefer role/name selectors for Projects readiness checksThese waits currently depend on
getByTestId('NavigationListItemProjects'); use the user-visible control instead (e.g., role + “Projects” name) to align with the e2e selector policy.♻️ Suggested change
- await expect( - this.page.getByTestId('NavigationListItemProjects') - ).toBeVisible({ timeout: 90000 }) + await expect( + this.page.getByRole('button', { name: 'Projects', exact: true }) + ).toBeVisible({ timeout: 90000 }) ... - await expect( - this.page.getByTestId('NavigationListItemProjects') - ).toBeVisible({ timeout: 90000 }) + await expect( + this.page.getByRole('button', { name: 'Projects', exact: true }) + ).toBeVisible({ timeout: 90000 })As per coding guidelines:
apps/**/*e2e*/**/*.ts: Reference elements by the label the user sees in the UI — use getByRole, getByPlaceholder, and similar accessible queries instead of CSS selectors, data-testid, or class names.Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/login-page.ts` around lines 36 - 37, Replace the data-testid-based wait using getByTestId('NavigationListItemProjects') with an accessible query that matches the user-visible control, e.g. use this.page.getByRole('link', { name: 'Projects' }).toBeVisible({ timeout: 90000 }) (or getByRole('button', { name: 'Projects' }) if it’s a button); update both occurrences that reference getByTestId('NavigationListItemProjects') (the lines around the existing toBeVisible call and the similar usage at the other occurrence) so the e2e checks use getByRole + name "Projects" instead of data-testid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts`:
- Around line 263-274: The locator in imageLibraryThumbnail currently uses
.first() to pick between two SettingsDrawer matches; instead narrow the
SettingsDrawer by requiring the ImageLibrary-specific component: when locating
'div[data-testid="SettingsDrawer"]' (which already uses
button[aria-label="close-image-library"]), add has:
this.page.locator('div[data-testid="ImageUpload"]') to disambiguate, then select
'div[data-testid="ImageBlockThumbnail"] img' (remove .first()). This replaces
the brittle .first() approach with a precise check for the ImageUpload component
inside imageLibraryThumbnail.
---
Nitpick comments:
In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts`:
- Around line 71-85: The test currently forces data-testid-driven selection
using testIdMap and this.page.locator which violates e2e accessibility
guidelines; replace the data-testid logic so you scope to the visible settings
drawer (use an accessible query to find the drawer, e.g., getByRole('dialog') or
getByRole with the drawer's accessible name) and then find the add-block button
by role and accessible name (use getByRole('button', { name: buttonName }) or
locator.getByRole) instead of using testIdMap or CSS data-testid selectors;
update the code that computes button (the testIdMap, testId, and button
variables) to instead locate the drawer element and call
drawer.getByRole('button', { name: buttonName }) so the selector matches what
users see.
In `@apps/journeys-admin-e2e/src/pages/login-page.ts`:
- Around line 36-37: Replace the data-testid-based wait using
getByTestId('NavigationListItemProjects') with an accessible query that matches
the user-visible control, e.g. use this.page.getByRole('link', { name:
'Projects' }).toBeVisible({ timeout: 90000 }) (or getByRole('button', { name:
'Projects' }) if it’s a button); update both occurrences that reference
getByTestId('NavigationListItemProjects') (the lines around the existing
toBeVisible call and the similar usage at the other occurrence) so the e2e
checks use getByRole + name "Projects" instead of data-testid.
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 171-172: The readiness check currently uses
this.page.getByTestId('NavigationListItemProjects'); replace that data-testid
selector with an accessible user-facing query such as
this.page.getByRole('link', { name: /Projects/i }).toBeVisible({ timeout: 90000
}) so the e2e uses the visible label; update the assertion in the same method
(where this.page.getByTestId('NavigationListItemProjects') appears) to use
getByRole/getByText with the "Projects" name.
In
`@apps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx`:
- Around line 66-67: The loading state set by setLoading(true) around the async
calls (e.g., journeyProfileCreate and updateLastActiveTeamId) can remain true on
error; wrap the async flow in a try/finally so setLoading(false) always runs.
Locate the blocks in TermsAndConditions.tsx where setLoading(true) precedes
await journeyProfileCreate() and where updateLastActiveTeamId is awaited (also
the blocks around lines 106-115 and 133) and change them to use try { await ...
} finally { setLoading(false) } while preserving any success-path logic and
error propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66a343e2-e22f-4b4a-bf30-61f86590a5c4
📒 Files selected for processing (5)
apps/journeys-admin-e2e/src/e2e/discover/card-level-actions.spec.tsapps/journeys-admin-e2e/src/pages/card-level-actions.tsapps/journeys-admin-e2e/src/pages/login-page.tsapps/journeys-admin-e2e/src/pages/register-Page.tsapps/journeys-admin/src/components/TermsAndConditions/TermsAndConditions.tsx
Comment on lines
+263
to
274
| private imageLibraryThumbnail() { | ||
| // The ImageLibrary Drawer is identified by its close button (aria-label hardcoded | ||
| // to "close-image-library" for all settings drawers). Use .first() because the | ||
| // background-image properties panel can also be open at the same time, creating a | ||
| // second matching drawer — both show the same img src, so first() is fine. | ||
| return this.page | ||
| .locator('div[data-testid="SettingsDrawer"]', { | ||
| has: this.page.locator('button[aria-label="close-image-library"]') | ||
| }) | ||
| .locator('div[data-testid="ImageBlockThumbnail"] img') | ||
| .first() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the specific file and lines in question
wc -l apps/journeys-admin-e2e/src/pages/card-level-actions.tsRepository: JesusFilm/core
Length of output: 1867
🏁 Script executed:
# Read the file content around lines 263-274
sed -n '260,280p' apps/journeys-admin-e2e/src/pages/card-level-actions.tsRepository: JesusFilm/core
Length of output: 1056
🏁 Script executed:
# Search for ImageUpload data-testid usage in the codebase
rg 'ImageUpload' --type ts --type tsx -A 2 -B 2Repository: JesusFilm/core
Length of output: 85
🏁 Script executed:
# Search for ImageUpload in the codebase more broadly
rg 'ImageUpload' -A 2 -B 2 | head -100Repository: JesusFilm/core
Length of output: 8448
🏁 Script executed:
# Search for background-image properties panel or settings drawer contexts
rg 'background-image|SettingsDrawer' -A 1 -B 1 | head -100Repository: JesusFilm/core
Length of output: 8587
🏁 Script executed:
# Check the drawer structure and context to understand the visual hierarchy
rg 'data-testid="SettingsDrawer"' -A 5 -B 5Repository: JesusFilm/core
Length of output: 10048
🏁 Script executed:
# Search specifically for ImageUpload data-testid in UI components
rg 'data-testid.*ImageUpload|ImageUpload.*data-testid' -A 2 -B 2Repository: JesusFilm/core
Length of output: 1725
🏁 Script executed:
# Search for ImageBlockThumbnail to understand the structure
rg 'ImageBlockThumbnail' -A 3 -B 3Repository: JesusFilm/core
Length of output: 30540
🏁 Script executed:
# Look for image library drawer or image upload related components
rg 'image.*library|library.*image' -i --type ts --type tsx -A 2 -B 2 | head -80Repository: JesusFilm/core
Length of output: 85
Don't use .first() to mask ambiguous drawer matches
The locator matches two drawers when both image library and background-image properties panels are open. Disambiguate using ImageUpload component (unique to the image library drawer) instead of .first().
♻️ Suggested change
- private imageLibraryThumbnail() {
- // The ImageLibrary Drawer is identified by its close button (aria-label hardcoded
- // to "close-image-library" for all settings drawers). Use .first() because the
- // background-image properties panel can also be open at the same time, creating a
- // second matching drawer — both show the same img src, so first() is fine.
- return this.page
- .locator('div[data-testid="SettingsDrawer"]', {
- has: this.page.locator('button[aria-label="close-image-library"]')
- })
- .locator('div[data-testid="ImageBlockThumbnail"] img')
- .first()
- }
+ private imageLibraryThumbnail() {
+ const imageLibraryDrawer = this.page.locator('div[data-testid="SettingsDrawer"]', {
+ has: this.page.locator('div[data-testid="ImageUpload"]')
+ })
+ return imageLibraryDrawer.locator('div[data-testid="ImageBlockThumbnail"] img')
+ }📝 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.
Suggested change
| private imageLibraryThumbnail() { | |
| // The ImageLibrary Drawer is identified by its close button (aria-label hardcoded | |
| // to "close-image-library" for all settings drawers). Use .first() because the | |
| // background-image properties panel can also be open at the same time, creating a | |
| // second matching drawer — both show the same img src, so first() is fine. | |
| return this.page | |
| .locator('div[data-testid="SettingsDrawer"]', { | |
| has: this.page.locator('button[aria-label="close-image-library"]') | |
| }) | |
| .locator('div[data-testid="ImageBlockThumbnail"] img') | |
| .first() | |
| } | |
| private imageLibraryThumbnail() { | |
| const imageLibraryDrawer = this.page.locator('div[data-testid="SettingsDrawer"]', { | |
| has: this.page.locator('div[data-testid="ImageUpload"]') | |
| }) | |
| return imageLibraryDrawer.locator('div[data-testid="ImageBlockThumbnail"] img') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/journeys-admin-e2e/src/pages/card-level-actions.ts` around lines 263 -
274, The locator in imageLibraryThumbnail currently uses .first() to pick
between two SettingsDrawer matches; instead narrow the SettingsDrawer by
requiring the ImageLibrary-specific component: when locating
'div[data-testid="SettingsDrawer"]' (which already uses
button[aria-label="close-image-library"]), add has:
this.page.locator('div[data-testid="ImageUpload"]') to disambiguate, then select
'div[data-testid="ImageBlockThumbnail"] img' (remove .first()). This replaces
the brittle .first() approach with a precise check for the ImageUpload component
inside imageLibraryThumbnail.
…navigation verification - Updated the random number generation for account registration to use a more secure method with randomBytes, ensuring uniqueness. - Refined the page navigation verification for the Terms step by switching to a stable element-based assertion, improving reliability against translation and markup changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/journeys-admin-e2e/src/pages/login-page.ts (2)
81-102:⚠️ Potential issue | 🟠 MajorDon’t reload away the bad first-load state.
This retry loop can turn a broken initial redirect into a passing test after refresh, which hides the exact regression this PR is supposed to catch. Assert the final team state on the initial navigation instead of using
waitForTimeout()+reload().♻️ Proposed fix
async assertCreatedTeamDiscoverState(expectedTeamTitle?: string) { const teamSelect = await this.getTeamSelectCombobox() - for (let attempt = 0; attempt < 3; attempt++) { - if ((await teamSelect.innerText()).includes('Shared With Me')) { - if (attempt === 2) break - await this.page.waitForTimeout(5000) - await this.page.reload() - await expect( - this.page.getByTestId('NavigationListItemProjects') - ).toBeVisible({ timeout: 90000 }) - await expect( - this.page - .getByTestId('TeamSelect') - .locator('[aria-haspopup="listbox"]') - ).toBeEnabled({ timeout: 90000 }) - continue - } - break - } await expect(teamSelect).not.toContainText('Shared With Me', { - timeout: 1000 + timeout: 90000 }) if (expectedTeamTitle != null && expectedTeamTitle.trim() !== '') { await expect(teamSelect).toContainText(expectedTeamTitle) }As per coding guidelines, "Always use soft waits (auto-waiting through Playwright built-ins like expect, toBeVisible, toBeEnabled, page.goto) — never use hard waits like waitForTimeout, setTimeout, sleep, or polling loops".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/login-page.ts` around lines 81 - 102, The retry loop in assertCreatedTeamDiscoverState (using getTeamSelectCombobox, teamSelect, page.waitForTimeout, and page.reload) masks first-load failures by refreshing the page; remove the loop and hard wait/reload, and instead rely on Playwright auto-waits: after navigating, assert the initial UI state by expecting page.getByTestId('NavigationListItemProjects') toBeVisible(...) and page.getByTestId('TeamSelect').locator('[aria-haspopup="listbox"]').toBeEnabled(...), then assert teamSelect does not contain 'Shared With Me' with an appropriate timeout; delete the reload/waitForTimeout/loop logic so the test fails on a bad first-load state.
41-46:⚠️ Potential issue | 🟡 MinorLine 46 still times out before the documented load path can finish.
Line 35 says the cold-start SSR + Apollo path can take more than 65s, but this TeamSelect readiness check still fails after 30s. That can make admin login flake before the team query settles.
🔧 Suggested fix
await expect( this.page.getByTestId('TeamSelect').locator('[aria-haspopup="listbox"]') - ).toBeEnabled({ timeout: 30000 }) + ).toBeEnabled({ timeout: 90000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/login-page.ts` around lines 41 - 46, The TeamSelect readiness check times out at 30s while cold-start SSR + Apollo can exceed 65s; update the wait to allow the longer load window by increasing the timeout on the TeamSelect readiness assertion (the this.page.getByTestId('TeamSelect').locator('[aria-haspopup="listbox"]') .toBeEnabled call) to at least 65000–70000 ms (or replace with an equivalent longer wait such as page.waitForSelector with a longer timeout) so the test waits for the teams query to settle before proceeding.
🧹 Nitpick comments (1)
apps/journeys-admin-e2e/src/pages/login-page.ts (1)
34-46: Prefer user-visible locators for the discover-page gates.These waits and the new TeamSelect helper are still anchored to
data-testid/CSS selectors (NavigationListItemProjects,TeamSelect,[aria-haspopup="listbox"],[role="combobox"]). That keeps the page object coupled to DOM internals instead of what the user sees. Please switch these to the accessible role/label queries the UI exposes.As per coding guidelines, "Reference elements by the label the user sees in the UI — use getByRole, getByPlaceholder, and similar accessible queries instead of CSS selectors, data-testid, or class names".
Also applies to: 69-70, 111-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin-e2e/src/pages/login-page.ts` around lines 34 - 46, The waits in waitUntilDiscoverPageLoaded (and the TeamSelect usage) rely on data-testid/CSS selectors (NavigationListItemProjects, TeamSelect locator '[aria-haspopup="listbox"]') which couples tests to DOM internals; replace these with accessible queries exposed to users — use this.page.getByRole(...) or getByLabel(...) with the visible role/label for the "Projects" navigation item and the Team select control (e.g., the combobox or button label shown in UI) so the test waits for user-visible elements to be visible/enabled; update the equivalent checks referenced by the TeamSelect helper and the other occurrences noted (similar replacements around the later waits) to use getByRole/getByLabel/getByPlaceholder as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts`:
- Around line 17-20: The generated high-entropy suffix (randomNumber) is
currently module-scoped and gets overwritten by each new Register instance; move
it to an instance property by declaring this.randomNumber (or similar) and
initializing it inside the Register constructor (use the same
dayjs().format(...) + randomBytes(8).toString('hex') expression). Update any
references to randomNumber in the class (e.g., email/fallback team title
generation, stored expectations) to use this.randomNumber so each Register
instance retains its own unique suffix.
---
Duplicate comments:
In `@apps/journeys-admin-e2e/src/pages/login-page.ts`:
- Around line 81-102: The retry loop in assertCreatedTeamDiscoverState (using
getTeamSelectCombobox, teamSelect, page.waitForTimeout, and page.reload) masks
first-load failures by refreshing the page; remove the loop and hard
wait/reload, and instead rely on Playwright auto-waits: after navigating, assert
the initial UI state by expecting page.getByTestId('NavigationListItemProjects')
toBeVisible(...) and
page.getByTestId('TeamSelect').locator('[aria-haspopup="listbox"]').toBeEnabled(...),
then assert teamSelect does not contain 'Shared With Me' with an appropriate
timeout; delete the reload/waitForTimeout/loop logic so the test fails on a bad
first-load state.
- Around line 41-46: The TeamSelect readiness check times out at 30s while
cold-start SSR + Apollo can exceed 65s; update the wait to allow the longer load
window by increasing the timeout on the TeamSelect readiness assertion (the
this.page.getByTestId('TeamSelect').locator('[aria-haspopup="listbox"]')
.toBeEnabled call) to at least 65000–70000 ms (or replace with an equivalent
longer wait such as page.waitForSelector with a longer timeout) so the test
waits for the teams query to settle before proceeding.
---
Nitpick comments:
In `@apps/journeys-admin-e2e/src/pages/login-page.ts`:
- Around line 34-46: The waits in waitUntilDiscoverPageLoaded (and the
TeamSelect usage) rely on data-testid/CSS selectors (NavigationListItemProjects,
TeamSelect locator '[aria-haspopup="listbox"]') which couples tests to DOM
internals; replace these with accessible queries exposed to users — use
this.page.getByRole(...) or getByLabel(...) with the visible role/label for the
"Projects" navigation item and the Team select control (e.g., the combobox or
button label shown in UI) so the test waits for user-visible elements to be
visible/enabled; update the equivalent checks referenced by the TeamSelect
helper and the other occurrences noted (similar replacements around the later
waits) to use getByRole/getByLabel/getByPlaceholder as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0906143-3de9-4fa4-97a9-db5d7d3f36ea
📒 Files selected for processing (2)
apps/journeys-admin-e2e/src/pages/login-page.tsapps/journeys-admin-e2e/src/pages/register-Page.ts
Comment on lines
+17
to
+20
| activeTeamTitle = '' | ||
| constructor(page: Page) { | ||
| this.page = page | ||
| randomNumber = | ||
| dayjs().format('DDMMYYhhmmss') + | ||
| Math.floor(Math.random() * (100 - 999 + 1) + 999) | ||
| randomNumber = `${dayjs().format('DDMMYYhhmmssSSS')}${randomBytes(8).toString('hex')}` |
Contributor
There was a problem hiding this comment.
Keep the generated suffix on the instance, not the module.
The higher-entropy value is good, but randomNumber is still shared module state, and every new Register() rewrites it. If two register flows run in the same worker, later calls in the older instance can pick up the newer suffix, so emails, fallback team titles, and stored expectations drift across tests.
♻️ Proposed fix
-let randomNumber = ''
const thirtySecondsTimeout = 30000
export class Register {
readonly page: Page
+ readonly randomNumber: string
name: string
userEmail: string
activeTeamTitle = ''
constructor(page: Page) {
this.page = page
- randomNumber = `${dayjs().format('DDMMYYhhmmssSSS')}${randomBytes(8).toString('hex')}`
+ this.randomNumber = `${dayjs().format('DDMMYYhhmmssSSS')}${randomBytes(8).toString('hex')}`
}- this.userEmail = `playwright${randomNumber}@example.com`
+ this.userEmail = `playwright${this.randomNumber}@example.com`- .fill(testData.register.userName + randomNumber, {
+ .fill(testData.register.userName + this.randomNumber, {- const fallbackTeamTitle = `Team ${randomNumber}`
+ const fallbackTeamTitle = `Team ${this.randomNumber}`- .fill(testData.teams.teamName + randomNumber, { timeout: 60000 })
+ .fill(testData.teams.teamName + this.randomNumber, { timeout: 60000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/journeys-admin-e2e/src/pages/register-Page.ts` around lines 17 - 20, The
generated high-entropy suffix (randomNumber) is currently module-scoped and gets
overwritten by each new Register instance; move it to an instance property by
declaring this.randomNumber (or similar) and initializing it inside the Register
constructor (use the same dayjs().format(...) + randomBytes(8).toString('hex')
expression). Update any references to randomNumber in the class (e.g.,
email/fallback team title generation, stored expectations) to use
this.randomNumber so each Register instance retains its own unique suffix.
- Updated the CustomizationMediaPage to validate YouTube URL input by checking for user-visible error messages instead of relying on MUI error classes. - Refactored the LoginPage and Register classes to use more consistent element selection methods, replacing test IDs with role-based selectors for better maintainability. - Added helper methods to streamline the retrieval of navigation items and team selection triggers, enhancing code readability and reducing duplication.
- Updated the VideosSection component to reset the YouTube URL error state when the input is cleared. - Improved error handling logic to ensure that the video block is validated before proceeding with the YouTube link processing. - Refactored e2e tests in CustomizationMediaPage to utilize role-based selectors for better maintainability and to validate user-visible error messages for invalid YouTube URLs.
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.
Summary by CodeRabbit
Bug Fixes
New Features
Tests