Skip to content

Fix E2E tests broken by redesigned navigation menu#18315

Merged
FelixMalfait merged 5 commits intomainfrom
fix/e2e-settings-menu-selectors
Mar 2, 2026
Merged

Fix E2E tests broken by redesigned navigation menu#18315
FelixMalfait merged 5 commits intomainfrom
fix/e2e-settings-menu-selectors

Conversation

@FelixMalfait
Copy link
Copy Markdown
Member

Summary

  • Settings selector: The Settings navigation item is now rendered as a <button> (via NavigationDrawerItem with onClick) instead of an <a> link (with to). Updated leftMenu.ts POM and create-kanban-view.spec.ts to use getByRole('button', { name: 'Settings' }).
  • create-record URL field: The Linkedin field interaction was missing an initial label click to trigger the hover portal rendering. Added recordFieldList.getByText('Linkedin').first().click() before the value click, matching the pattern used by the working Emails field.

Test plan

  • E2E signup_invite_email.spec.ts passes (uses leftMenu.goToSettings())
  • E2E create-kanban-view.spec.ts passes (uses Settings click directly)
  • E2E create-record.spec.ts passes (Linkedin URL field interaction)
  • Existing passing E2E tests remain green

Made with Cursor

Settings is now rendered as a button (NavigationDrawerItem with onClick)
instead of a link (NavigationDrawerItem with to), so update selectors
from getByRole('link') to getByRole('button'). Also fix create-record
test's Linkedin field interaction to match the working Emails pattern
(click label first to trigger hover portal, then click value).

Made-with: Cursor
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 1, 2026

Greptile Summary

Updated E2E test selectors to match the redesigned navigation menu where Settings changed from an anchor link to a button element. The PR also fixed the LinkedIn URL field interaction by adding an initial label click to trigger the hover portal rendering, matching the existing pattern used for the Emails field.

  • Changed Settings selector from getByRole('link') to getByRole('button') in POM and test files
  • Added missing label click for LinkedIn field interaction in create-record.spec.ts
  • All changes are minimal, focused test fixes with no logic or security concerns

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes are straightforward test selector updates that correctly reflect UI changes. The Settings element role change from link to button is consistently applied across all files, and the LinkedIn field fix follows the established pattern used for the Emails field.
  • No files require special attention

Important Files Changed

Filename Overview
packages/twenty-e2e-testing/lib/pom/leftMenu.ts Updated Settings tab selector from link to button role, correctly reflecting the redesigned navigation menu
packages/twenty-e2e-testing/tests/create-kanban-view.spec.ts Updated Settings selector to use button role instead of link, consistent with POM changes
packages/twenty-e2e-testing/tests/create-record.spec.ts Added initial label click for LinkedIn field to trigger hover portal rendering, matching the Emails field pattern

Last reviewed commit: 3c88708

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Made-with: Cursor
Add label click before Phone field value click (same pattern as
Emails/Linkedin fix). Add networkidle wait before second goToSettings()
in signup test to prevent flakiness after signup flow completes.

Made-with: Cursor
networkidle hangs due to persistent connections (WebSocket/polling).
Wait for the Settings button to be visible instead, which is the actual
precondition for the click.

Made-with: Cursor
await ratingContainer.locator('svg').nth(3).click({force: true});

// Fill phone field
await recordFieldList.getByText('Phones').first().click();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The .click() call on line 119 is missing the await keyword. This is inconsistent with other asynchronous calls in the test file, creating potential flakiness.
Severity: MEDIUM

Suggested Fix

Add the await keyword before the .click() call on line 119 to ensure the asynchronous operation completes before the test proceeds. The line should be await recordFieldList.getByText('Work Preference').first().click({force: true});.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/twenty-e2e-testing/tests/create-record.spec.ts#L96

Potential issue: In the E2E test, a `.click()` call on line 119 is not awaited. Since
`click()` is an asynchronous Playwright function, failing to `await` it means the test
execution will not wait for the action to complete. This introduces a potential race
condition and can lead to flaky tests. While the subsequent operation is independent,
this violates the established pattern in the file and Playwright best practices. Any
errors thrown by the click action would also not be properly handled, potentially
masking issues.

@FelixMalfait FelixMalfait merged commit 8d47d8a into main Mar 2, 2026
66 of 68 checks passed
@FelixMalfait FelixMalfait deleted the fix/e2e-settings-menu-selectors branch March 2, 2026 09:52
@twenty-eng-sync
Copy link
Copy Markdown

Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant