fix: improve visibility checks and remove unnecessary load state…#8037
fix: improve visibility checks and remove unnecessary load state…#8037mikeallisonJS merged 3 commits intomainfrom
Conversation
… waits in journey and card level actions
mikeallisonJS
requested review from
kiran-redhat and
tataihono
as code owners
mikeallisonJS
changed the title
WalkthroughReplaces generic page load-state waits with explicit visibility waits and more specific locators for journey card/button/frame interactions in E2E page objects and tests; updates element targeting order (.first() → .last()) and refines error-handling flows and click timing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as E2E Test
participant PageObj as Page Object
participant Frame as Browser Frame
participant Element as UI Element
Note over Test,PageObj #D6EAF8: High-level test action triggers page-object method
Test->>PageObj: clickOnJourneyCard()/clickCreateCustomJourney()
PageObj->>Frame: locate frame -> wait for specific element visible (timeout)
Frame->>Element: overlay-image / background-image becomes visible
PageObj->>Element: click (force/with timeout)
Element-->>PageObj: click result / navigation
PageObj-->>Test: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
|
View your CI Pipeline Execution ↗ for commit c9d46d1
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/journeys-admin-e2e/src/pages/journey-page.ts (1)
246-253: Remove redundant visibility check.Lines 246-248 and 249-253 both wait for the same button to be visible with the same 150-second timeout. The
waitFor()call already ensures the element is visible, making the subsequentexpect().toBeVisible()redundant.Apply this diff to remove the redundancy:
await this.page .locator('div[data-testid="JourneysAdminContainedIconButton"] button') .waitFor({ state: 'visible', timeout: 150000 }) - await expect( - this.page.locator( - 'div[data-testid="JourneysAdminContainedIconButton"] button' - ) - ).toBeVisible({ timeout: 150000 })apps/journeys-admin-e2e/src/pages/card-level-actions.ts (1)
199-206: Remove redundant visibility check.Lines 200-202 and 203-205 both wait for the same StrategyItem button to be visible with the same timeout. The
waitFor()call already ensures visibility, making the subsequentexpect().toBeVisible()redundant.Apply this diff to remove the redundancy:
+ async waitUntilJourneyCardLoaded() { await this.page .locator('div[data-testid="StrategyItem"] button') .waitFor({ state: 'visible', timeout: sixtySecondsTimeout }) - await expect( - this.page.locator('div[data-testid="StrategyItem"] button') - ).toBeVisible({ timeout: sixtySecondsTimeout }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/journeys-admin-e2e/src/pages/card-level-actions.ts(2 hunks)apps/journeys-admin-e2e/src/pages/journey-page.ts(2 hunks)apps/journeys-e2e/src/e2e/journeys.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apps/journeys-admin-e2e/src/pages/journey-page.tsapps/journeys-admin-e2e/src/pages/card-level-actions.tsapps/journeys-e2e/src/e2e/journeys.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/journeys-admin-e2e/src/pages/journey-page.tsapps/journeys-admin-e2e/src/pages/card-level-actions.tsapps/journeys-e2e/src/e2e/journeys.spec.ts
apps/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/apps.mdc)
apps/**/*.{js,jsx,ts,tsx}: Always use MUI over HTML elements; avoid using CSS or tags.
Use descriptive variable and function/const names. Also, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.
Implement accessibility features on elements. For example, a tag should have a tabindex=“0”, aria-label, on:click, and on:keydown, and similar attributes.
Files:
apps/journeys-admin-e2e/src/pages/journey-page.tsapps/journeys-admin-e2e/src/pages/card-level-actions.tsapps/journeys-e2e/src/e2e/journeys.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/journeys-admin-e2e/src/pages/journey-page.ts (1)
1076-1088: LGTM! Improved synchronization for frame interactions.The explicit visibility wait for the frame element before clicking ensures reliable interaction with nested iframe content, which is more robust than relying on generic load-state waits.
apps/journeys-e2e/src/e2e/journeys.spec.ts (2)
8-11: LGTM! Robust role-based locator improves test reliability.Using
getByRolewith an explicit visibility wait is more resilient than direct selectors and aligns with accessibility best practices. This change improves test stability by ensuring the element is ready before interaction.
15-18: LGTM! Granular assertions improve test diagnostics.Splitting the assertions provides clearer test feedback when failures occur, making it easier to identify which specific element is missing or not visible.
apps/journeys-admin-e2e/src/pages/card-level-actions.ts (3)
31-42: LGTM! Explicit visibility wait improves reliability.Adding the visibility wait before clicking the card overlay ensures the frame content is fully loaded and interactive, which is more reliable than depending on generic page load states.
44-59: LGTM! Targeted selector for video card interactions.The more specific selector targeting the background-image within the overlay is appropriate for video cards, and the explicit visibility wait ensures the element is ready before the forced click.
478-516: Verify the intentional change from.first()to.last()for element targeting.This method uses
.last()when selecting elements for deletion (lines 492, 509). While using.last()can avoid index shifting as elements are removed from the DOM, I cannot independently verify whether this represents a change from.first()or confirm the intended behavior. Additionally, the test that calls this method (Video - create, update & deletein card-level-actions.spec.ts) is marked astest.skip, so this code path is currently not actively tested.Please confirm:
- Whether this represents a change from
.first()to.last()- That the deletion order is correct for your test scenarios and doesn't cause unintended side effects
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/journeys-e2e/src/e2e/journeys.spec.ts (1)
18-18: Consider consolidating viewport assertions.The separate assertion for
JourneysTypographyis clear but creates sequential viewport checks. If both elements must be visible together, consider combining them.If consolidation is preferred:
await expect( page.getByRole('heading', { name: 'Fact or Fiction' }) ).toBeInViewport() - await expect(page.getByTestId('JourneysTypography')).toBeInViewport() + await expect.soft(page.getByTestId('JourneysTypography')).toBeInViewport()Or use
Promise.all()for parallel checks:await Promise.all([ expect(page.getByRole('heading', { name: 'Fact or Fiction' })).toBeInViewport(), expect(page.getByTestId('JourneysTypography')).toBeInViewport() ])However, keeping them separate provides clearer failure messages, so this is purely a performance micro-optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/journeys-e2e/src/e2e/journeys.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apps/journeys-e2e/src/e2e/journeys.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/journeys-e2e/src/e2e/journeys.spec.ts
apps/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/apps.mdc)
apps/**/*.{js,jsx,ts,tsx}: Always use MUI over HTML elements; avoid using CSS or tags.
Use descriptive variable and function/const names. Also, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.
Implement accessibility features on elements. For example, a tag should have a tabindex=“0”, aria-label, on:click, and on:keydown, and similar attributes.
Files:
apps/journeys-e2e/src/e2e/journeys.spec.ts
🧬 Code graph analysis (1)
apps/journeys-e2e/src/e2e/journeys.spec.ts (1)
apps/journeys-admin-e2e/src/fixtures/authenticated.ts (1)
expect(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/journeys-e2e/src/e2e/journeys.spec.ts (2)
15-17: Excellent use of semantic role-based selector.Using
getByRole('heading', { name: 'Fact or Fiction' })is more accessible and follows Playwright best practices for semantic element targeting.
8-11: Explicit visibility wait improves test reliability.The change from a generic load state wait to an explicit visibility wait on the href-based locator
a[href="/fact-or-fiction"]is a solid improvement. Code search confirms this selector is unique in the codebase with no duplicates.
…g locator in journeys spec
2 participants
… waits in journey and card level actions
Summary by CodeRabbit