Simplify e2e tests

[amitsingh-007]

Greptile Summary

Consolidates e2e test utilities into a shared package and replaces fixed timeouts with state-based assertions. The PR extracts common test helpers (closeDialog, verifyModalVisible, fillSearchInput, openNewPageFromAction, etc.) into packages/shared/src/utils/test-helpers.ts and updates both extension and web tests to use them.

Key improvements:

• Removed hardcoded waitForTimeout calls and replaced with expect.poll() for debounced state changes
• Unified dialog/modal interaction patterns across tests after shadcn migration
• Standardized search input operations and badge count parsing
• Improved reliability of page-opening assertions with polling

The refactor reduces code duplication between extension and web test suites while making tests more resilient to timing variations.

Confidence Score: 4/5

• Safe to merge with one minor logic fix recommended for retry behavior
• The refactoring is well-structured and consolidates duplicate test utilities effectively. The main concern is a retry logic issue in bookmarks.spec.ts:158-163 where the entire action (including right-click and expects) re-executes on retry, potentially causing flakiness. Otherwise, the migration to state-based polling improves test reliability, and the shared utilities are cleanly designed.
• apps/extension/tests/specs/bookmarks.spec.ts - contains retry logic that could cause test flakiness

_{Last reviewed commit: 7b05ece}

[greptile-apps]

_{23 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

issue: openFromContextMenu re-executes all actions (right-click, expect, click) on each retry, but clicking the same element multiple times could cause unexpected behavior or test flakiness

Suggested change

	const openFromContextMenu = async () => {
	await firstBookmark.click({ button: 'right' });
	const openOption = bookmarksPage.getByTestId('context-menu-item-open');
	await expect(openOption).toBeVisible();
	await openOption.click();
	};
	let contextMenuPage;
	for (let attempt = 0; attempt < 2; attempt++) {
	try {
	await firstBookmark.click({ button: 'right' });
	const openOption = bookmarksPage.getByTestId('context-menu-item-open');
	await expect(openOption).toBeVisible();
	contextMenuPage = await openNewPageFromAction(
	context,
	async () => await openOption.click(),
	{ timeout: 5000 }
	);
	break;

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/extension/tests/specs/bookmarks.spec.ts
Line: 158-163

Comment:
issue: `openFromContextMenu` re-executes all actions (right-click, expect, click) on each retry, but clicking the same element multiple times could cause unexpected behavior or test flakiness

```suggestion
      let contextMenuPage;
      for (let attempt = 0; attempt < 2; attempt++) {
        try {
          await firstBookmark.click({ button: 'right' });
          const openOption = bookmarksPage.getByTestId('context-menu-item-open');
          await expect(openOption).toBeVisible();
          
          contextMenuPage = await openNewPageFromAction(
            context,
            async () => await openOption.click(),
            { timeout: 5000 }
          );
          break;
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

unnecessary null check since toContain already handles null gracefully - the explicit throw adds no value

Suggested change

	const tooltipText = await tooltip.textContent();
	expect(tooltipText).toBeTruthy();
	expect(tooltipText).not.toBeNull();
	if (!tooltipText) {
	throw new Error('Expected bookmark tooltip text to be present');
	}
	const tooltipText = await tooltip.textContent();
	expect(tooltipText).not.toBeNull();
	expect(tooltipText).toContain('material');

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review.
Path: apps/web/tests/specs/bookmarks.spec.ts
Line: 195-199

Comment:
unnecessary null check since `toContain` already handles null gracefully - the explicit throw adds no value

```suggestion
    const tooltipText = await tooltip.textContent();
    expect(tooltipText).not.toBeNull();
    expect(tooltipText).toContain('material');
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

[gemini-code-assist]

Code Review

The pull request simplifies e2e tests by centralizing common test utilities into @bypass/shared/tests and removing redundant timeout configurations. This improves maintainability and reduces code duplication across test files. The changes also refine assertions and test logic for better clarity and robustness.

[gemini-code-assist]

The refactoring to use expect.poll for verifying opened pages is a significant improvement. It makes the test more resilient to timing issues by continuously checking the condition until it's met or the timeout is reached, rather than relying on fixed waits. This is particularly useful for asynchronous operations like opening new tabs.

Suggested change

	await homeExpect
	.poll(
	() => {
	const currentPages = context
	.pages()
	.filter((page) => page !== homePage);
	return currentPages
	.map((page) => page.url())
	.filter((url) => url.startsWith('http'))
	.map((url) => {
	const parsed = new URL(url);
	return `${parsed.protocol}//${parsed.host}${parsed.pathname}`;
	});
	},
	{
	timeout: 15_000,
	}
	)
	.toEqual(homeExpect.arrayContaining(['https://www.google.com/']));
	// Verify default tabs were opened (Google and Mantine)
	await homeExpect
	.poll(
	() => {
	const currentPages = context
	.pages()
	.filter((page) => page !== homePage);
	return currentPages
	.map((page) => page.url())
	.filter((url) => url.startsWith('http'))
	.map((url) => {
	const parsed = new URL(url);
	return `${parsed.protocol}//${parsed.host}${parsed.pathname}`;
	});
	},
	{
	timeout: 15_000,
	}
	)
	.toEqual(homeExpect.arrayContaining(['https://www.google.com/']));

[gemini-code-assist]

The updated assertion for baseUrls is more robust by using homeExpect.stringMatching with a regular expression. This allows for more flexible matching of URLs, accounting for potential variations like chrome-error:// pages, which can occur during navigation or extension loading. This makes the test less brittle.

    const baseUrls = context
      .pages()
      .filter((page) => page !== homePage)
      .map((page) => page.url())
      .filter(
        (url) => url.startsWith('http') || url.startsWith('chrome-error://')
      );
    homeExpect(baseUrls).toEqual(
      homeExpect.arrayContaining([
        homeExpect.stringMatching(/mantine\.dev|^chrome-error:\/\/),
      ])
    );

[gemini-code-assist]

Removing explicit timeouts from expect.poll assertions allows Playwright to use its default auto-retry mechanism, which is generally more robust and less prone to flakiness than fixed timeouts. If a specific longer timeout is needed, it should be explicitly defined and justified.

Suggested change

	.poll(() => newPage.url(), {
	timeout: 10_000,
	message: 'Page should navigate from about:blank to actual URL',
	})
	await expect
	.poll(() => newPage.url(), {
	message: 'Page should navigate from about:blank to actual URL',
	})

[gemini-code-assist]

Removing explicit waitForTimeout calls and relying on Playwright's auto-retrying assertions or explicit waits (like toBeVisible(), toBeEnabled()) is a best practice. Fixed timeouts can lead to flaky tests or unnecessarily slow execution. Playwright's assertions handle the waiting implicitly.

Suggested change

	await homePanel.setHistoryEnabled(false);

[gemini-code-assist]

Using expect.poll for verifying the history items count provides a more robust and efficient way to handle asynchronous updates. Instead of a fixed wait, it continuously checks the condition until it's met or the timeout is reached, reducing flakiness and improving test performance.

    // Verify the test sites are deleted from browser history (with retry)
    await expect
      .poll(
        async () => {
          const historyAfter = await getHistoryItems(homePage, sites);
          return historyAfter.length;
        },
        {
          message: 'History items should be deleted',
        }
      )
      .toBe(0);

[gemini-code-assist]

Removing explicit timeouts from waitForResponse() allows Playwright to use its default auto-retry mechanism, which is generally more robust and less prone to flakiness than fixed timeouts. If a specific longer timeout is needed, it should be explicitly defined and justified.

  const apiPromise = page.waitForResponse(
    (response) =>
      response.url().includes('/api/trpc/extension.latest') &&
      response.status() === 200

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 8 additional findings.