Conversation
…avigation, examples, homepage, integrations, morphs, quickstart, and search functionality
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds Playwright config timeouts and maxFailures, global action/navigation timeouts, webServer timeout, many new Playwright E2E and accessibility tests plus a console-error utility, adds @axe-core/playwright devDependency, adjusts CI/test workflows/timeouts, and bumps pnpm version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Runner
participant WS as Dev WebServer
participant TR as Playwright Runner
participant B as Browser
rect rgb(250,248,255)
note left of CI: CI vs Local run differences (timeouts/env)
alt CI
CI->>WS: start server (reuseExistingServer=false, timeout=120s)
CI->>TR: set PLAYWRIGHT_TIMEOUT=300000, headless envs, run tests
else Local
CI->>WS: attach/start server (may reuse)
CI->>TR: set PLAYWRIGHT_TIMEOUT=60000, run tests
end
end
rect rgb(245,255,250)
TR->>B: launch (uses actionTimeout/navigationTimeout)
TR->>B: page.goto(url, { timeout: navigationTimeout })
B-->>TR: loadState(networkidle)
TR->>B: run assertions (DOM, links, attrs)
TR->>B: run Axe checks via @axe-core/playwright
TR->>TR: assertNoConsoleErrors collects console and asserts none
TR-->>CI: report pass/fail (maxFailures enforced)
end
sequenceDiagram
autonumber
participant TR as Test Runner
participant API as /api/search
rect rgb(255,250,240)
TR->>API: GET /api/search?q=... (single/encoded/special/long)
API-->>TR: response (status, headers, body)
alt status == 200 & content-type == application/json
TR->>TR: validate JSON shape & sample content
end
TR->>API: 5 concurrent GETs
API-->>TR: responses
TR-->>TR: assert(status < 500 && responseTime <= 5000ms)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
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 📒 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). (1)
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 |
- Updated tests in `integrations.spec.ts`, `morphs.spec.ts`, and `quickstart.spec.ts` to ensure proper visibility checks by using `.first()` for specific elements. - Changed the `rel` attribute checks for links to use the order "noreferrer noopener" for consistency. - Enhanced boolean section checks to count element existence rather than just visibility. These changes aim to improve the reliability and clarity of the test suite.
…esting' into 51-add-end-to-end-testing
- Updated tests in `morphs.spec.ts` and `quickstart.spec.ts` to ensure proper visibility checks by using `.first()` for specific elements. - Enhanced checks for the `rel` attribute order in link elements. - Modified h2 element checks to count existence rather than just visibility, accommodating optional elements. These changes aim to enhance the reliability and clarity of the test suite.
- Updated Playwright configuration for WebKit to include specific action and navigation timeouts of 60 seconds. - Modified multiple test files to set a timeout of 60 seconds for page navigation and load state checks, improving reliability in slower environments. - Enhanced accessibility tests to ensure focus visibility checks account for WebKit's limitations, verifying the presence of focusable elements when necessary. These changes aim to improve test stability and accessibility compliance across the application.
- Added global action and navigation timeouts of 60 seconds in the Playwright configuration to ensure consistent behavior across tests. - Updated multiple test files to specify timeouts for page navigation and load state checks, enhancing stability in slower environments. - Adjusted specific tests to include timeout settings for URL navigation, further improving test robustness. These changes aim to enhance the reliability and performance of the test suite.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tooling/playwright-www/tests/quickstart.spec.ts (1)
177-197: Extract duplicate console error monitoring (see accessibility.spec.ts).This console error test duplicates the logic from
accessibility.spec.ts(lines 231-251). As noted in the previous file review, extract this to a shared utility function to eliminate duplication and ease maintenance.Refer to the comment on
accessibility.spec.tslines 231-251 for the detailed refactoring suggestion.
🧹 Nitpick comments (9)
tooling/playwright-www/playwright.config.ts (1)
28-30: Remove redundant per-project timeout configurations.The
actionTimeoutandnavigationTimeoutare already set globally in the top-leveluseblock (lines 28-30). The per-project timeout configurations in the Firefox and WebKit projects are redundant and should be removed to avoid maintenance overhead and potential inconsistencies.Apply this diff to remove the redundant configurations:
name: "firefox", use: { ...devices["Desktop Firefox"], - // Firefox-specific timeout configuration - actionTimeout: 60000, - navigationTimeout: 60000, }, }, { name: "webkit", use: { ...devices["Desktop Safari"], - // WebKit-specific timeout configuration - actionTimeout: 60000, - navigationTimeout: 60000, }, },Also applies to: 42-47, 52-57
tooling/playwright-www/tests/hello-world.spec.ts (1)
31-31: Remove redundant timeout parameter.The global
navigationTimeoutis already configured to 60000ms inplaywright.config.ts(line 30), making this explicit timeout parameter unnecessary.Apply this diff to simplify:
- await page.goto("/", { timeout: 60000 }); + await page.goto("/");tooling/playwright-www/tests/examples.spec.ts (1)
90-110: Consider extracting console error filtering logic.The console error filtering pattern (lines 90-110) is duplicated across multiple test files (examples.spec.ts, morphs.spec.ts, integrations.spec.ts, homepage.spec.ts, docs-navigation.spec.ts). Consider extracting this into a shared test helper to improve maintainability.
Create a new file
tooling/playwright-www/tests/helpers.ts:import type { Page } from "@playwright/test"; export function setupConsoleErrorCollector(page: Page): string[] { const consoleErrors: string[] = []; page.on("console", (msg) => { if (msg.type() === "error") { const errorText = msg.text(); if ( !errorText.includes("403") && !errorText.includes("Failed to load resource") ) { consoleErrors.push(errorText); } } }); return consoleErrors; }Then use it in tests:
+import { setupConsoleErrorCollector } from "./helpers"; + test("should not have console errors", async ({ page }) => { - const consoleErrors: string[] = []; - page.on("console", (msg) => { - if (msg.type() === "error") { - const errorText = msg.text(); - if ( - !errorText.includes("403") && - !errorText.includes("Failed to load resource") - ) { - consoleErrors.push(errorText); - } - } - }); + const consoleErrors = setupConsoleErrorCollector(page); await page.goto("/docs/examples"); await page.waitForLoadState("networkidle"); await page.waitForTimeout(1000); expect(consoleErrors).toHaveLength(0); });tooling/playwright-www/tests/homepage.spec.ts (2)
7-8: Remove redundant timeout parameters.Multiple tests specify explicit
timeout: 60000forpage.goto()andwaitForLoadState(), but the global configuration already setsnavigationTimeout: 60000(playwright.config.ts, line 30). These explicit timeouts are redundant.Apply this pattern throughout the file:
- await page.goto("/", { timeout: 60000 }); - await page.waitForLoadState("networkidle", { timeout: 60000 }); + await page.goto("/"); + await page.waitForLoadState("networkidle");Also applies to: 22-23, 39-40, 57-58, 94-95
145-160: Consider using data-testid attributes for responsive element selection.The mobile/desktop responsive tests (lines 145-177) rely on escaped Tailwind CSS class selectors like
.sm\\:hiddenand.hidden.sm\\:block, which are fragile and tightly coupled to implementation details. If the styling approach changes, these tests will break even though functionality remains correct.Consider adding
data-testidattributes to the mobile and desktop variants in the source component:// In the component <div data-testid="star-button-mobile" className="sm:hidden"> {/* Mobile star button */} </div> <div data-testid="star-button-desktop" className="hidden sm:block"> {/* Desktop star button */} </div>Then update the tests:
- const mobileStarButton = page.locator(".sm\\:hidden a[href*='github.com']"); + const mobileStarButton = page.locator("[data-testid='star-button-mobile'] a[href*='github.com']"); await expect(mobileStarButton).toBeVisible(); - const desktopStarButton = page.locator(".hidden.sm\\:block a[href*='github.com']"); + const desktopStarButton = page.locator("[data-testid='star-button-desktop'] a[href*='github.com']"); await expect(desktopStarButton).not.toBeVisible();Also applies to: 162-177
tooling/playwright-www/tests/accessibility.spec.ts (4)
70-70: Limiting link checks to 10 may miss accessibility issues.Only the first 10 links are checked (
Math.min(linkCount, 10)). If inaccessible links exist beyond the first 10, they won't be caught.Consider one of these approaches:
- Check all links (remove the limit)
- Make the limit configurable or document why 10 is sufficient
- Sample links randomly instead of taking the first 10
-for (let i = 0; i < Math.min(linkCount, 10); i++) { +for (let i = 0; i < linkCount; i++) {
99-125: Refactor to avoid duplication with keyboard navigation test.This focus management test significantly overlaps with the keyboard navigation test (lines 80-97). Both tests press Tab and verify focus visibility with the same WebKit fallback logic.
Consider merging these tests or clearly differentiating their purposes. If the intent is to test multiple tab presses, that distinction should be clearer.
-test("should support keyboard navigation", async ({ page }) => { - // ... single tab press test -}); - -test("should have proper focus management", async ({ page }) => { - // ... multiple tab presses test -}); +test("should support keyboard navigation", async ({ page }) => { + await page.goto("/"); + await page.waitForLoadState("networkidle"); + + const interactiveElements = page.locator("a, button, input, [tabindex]"); + const elementCount = await interactiveElements.count(); + + if (elementCount > 0) { + // Test single tab + await page.keyboard.press("Tab"); + // ... verify focus + + // Test multiple tabs (up to 5) + for (let i = 1; i < Math.min(elementCount, 5); i++) { + await page.keyboard.press("Tab"); + // ... verify focus + } + } +});
127-144: Consider merging with button labels test to reduce duplication.This ARIA attributes test has significant overlap with the "accessible button labels" test (lines 44-60). Both iterate through buttons and verify that they have appropriate labels or text content.
If the intent is to test the same requirement, consider consolidating these into a single, comprehensive test.
5-6: Remove redundant explicit timeouts
The globalnavigationTimeoutandactionTimeoutinplaywright.config.tsare already set to 60 000 ms, so the{ timeout: 60000 }options onpage.gotoandpage.waitForLoadStatecan be dropped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
tooling/playwright-www/playwright.config.ts(2 hunks)tooling/playwright-www/tests/accessibility.spec.ts(1 hunks)tooling/playwright-www/tests/docs-navigation.spec.ts(1 hunks)tooling/playwright-www/tests/examples.spec.ts(1 hunks)tooling/playwright-www/tests/hello-world.spec.ts(1 hunks)tooling/playwright-www/tests/homepage.spec.ts(1 hunks)tooling/playwright-www/tests/integrations.spec.ts(1 hunks)tooling/playwright-www/tests/morphs.spec.ts(1 hunks)tooling/playwright-www/tests/quickstart.spec.ts(1 hunks)tooling/playwright-www/tests/search.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tooling/playwright-www/tests/homepage.spec.ts (5)
apps/www/components/page/sail-button.test.tsx (1)
it(14-91)apps/www/components/ui/heading.test.tsx (3)
it(11-98)user(57-70)user(38-55)apps/www/components/page/star-us-button.test.tsx (1)
it(8-53)apps/www/components/page/video-demo.test.tsx (1)
beforeEach(12-216)apps/www/components/page/logo.test.tsx (1)
afterEach(5-41)
⏰ 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: test-e2e
🔇 Additional comments (10)
tooling/playwright-www/tests/search.spec.ts (1)
1-112: LGTM! Comprehensive search API test coverage.The test suite appropriately covers various scenarios including empty queries, special characters, URL encoding, performance, and concurrent requests. The conditional JSON validation (lines 19-25) correctly handles cases where the endpoint may not return JSON.
tooling/playwright-www/tests/morphs.spec.ts (1)
115-124: Good practice: Flexible rel attribute validation.The regex pattern at line 122 correctly handles both possible orderings of the
relattribute values ("noopener noreferrer" or "noreferrer noopener"), which is more robust than checking for exact matches.tooling/playwright-www/tests/integrations.spec.ts (1)
1-177: LGTM! Thorough integration page coverage.The test suite comprehensively covers both VS Code and JetBrains integration pages, including marketplace links, extension descriptions, and proper page structure validation. The cross-page tests (lines 109-148) efficiently verify consistency across both pages.
tooling/playwright-www/tests/docs-navigation.spec.ts (1)
1-200: LGTM! Comprehensive documentation navigation coverage.The test suite thoroughly validates navigation across all documentation pages, including proper titles, external link behavior, and structural consistency. The iterative approach efficiently tests multiple pages while maintaining clear, focused test cases.
tooling/playwright-www/tests/accessibility.spec.ts (2)
52-59: Approve the button accessibility check with minor note.The logic correctly validates that all buttons have either
aria-labelor visible text content, which is essential for screen reader accessibility.Minor note: This check overlaps with the ARIA attributes test (lines 127-144). Consider consolidating if the test suite becomes harder to maintain.
80-97: Approve keyboard navigation test with good browser compatibility handling.The test correctly handles WebKit's limitations with the
:focusselector by providing a fallback that checks for focusable elements instead.tooling/playwright-www/tests/quickstart.spec.ts (4)
4-15: LGTM! Clean page load test.The test correctly verifies the quickstart page loads and displays expected content with appropriate heading and description checks.
22-24: Approve the defensive count-based check.Good use of
.count()to verify "Install" elements exist without requiring visibility, which is more resilient to styling changes or lazy loading.
92-99: Approve conditional navigation pattern.The defensive
isVisible()check before clicking prevents test failures if the link doesn't exist on the page. This pattern is well-suited for optional content.
150-152: Verify if explicit timeout is necessary.The test uses
waitForURLwith an explicit 10-second timeout, but then immediately asserts the URL withexpect(page).toHaveURL(...). Theexpectassertion already includes a timeout, making the explicitwaitForURLpotentially redundant.Consider whether both waits are needed:
-await page.waitForURL("**/docs/how-to/load-environment-variables", { - timeout: 10000, -}); await expect(page).toHaveURL("/docs/how-to/load-environment-variables");If navigation is slow, the
expecttimeout can be configured globally or thewaitForURLcan remain as an explicit safety check.
- Set a timeout of 4 minutes for the test, test-typesafety, and test-build jobs to ensure they complete within a reasonable timeframe. - Reduced the timeout for the test-e2e job from 60 minutes to 8 minutes to better reflect expected execution time. These changes aim to enhance the efficiency and reliability of the CI/CD pipeline.
- Bump pnpm version from 10.18.2 to 10.18.3 in package.json. - Add @axe-core/playwright dependency to improve accessibility testing. - Implement accessibility checks in tests to ensure no color contrast violations are present. These changes aim to enhance the testing framework and ensure compliance with accessibility standards.
- Replaced manual console error tracking with a utility function `assertNoConsoleErrors` in both `accessibility.spec.ts` and `quickstart.spec.ts`. - This change simplifies the tests and improves readability while maintaining the same functionality. These updates aim to enhance the maintainability of the test suite.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tooling/playwright-www/tests/utils/console-errors.ts (2)
16-27: Detach console listener and avoid arbitrary 1s sleep (reduce flakiness).
- Clean up the event handler to prevent leaks or duplicate captures across multiple calls on the same Page.
- Prefer a shorter settle window; or use polling instead of a full 1000ms sleep.
Apply:
- page.on("console", (msg) => { - if (msg.type() === "error") { - // Filter out known non-critical errors - const errorText = msg.text(); - if ( - !errorText.includes("403") && - !errorText.includes("Failed to load resource") - ) { - consoleErrors.push(errorText); - } - } - }); - - await page.goto(url); - await page.waitForLoadState("networkidle"); - await page.waitForTimeout(1000); - - expect(consoleErrors).toHaveLength(0); + const handler = (msg: any) => { + if (msg.type() === "error") { + const errorText = msg.text(); + if ( + !errorText.includes("403") && + !errorText.includes("Failed to load resource") + ) { + consoleErrors.push(errorText); + } + } + }; + page.on("console", handler); + try { + await page.goto(url); + await page.waitForLoadState("networkidle"); + // small settle window; adjust if needed + await page.waitForTimeout(500); + expect(consoleErrors).toHaveLength(0); + } finally { + page.off("console", handler); + }Also applies to: 29-34
21-25: Make ignore-list configurable (future-proof).Pass an optional array of ignore patterns (strings or regex) so tests can suppress known noisy messages without editing this utility.
tooling/playwright-www/tests/quickstart.spec.ts (2)
92-101: Harden URL assertions (tolerate trailing slashes/base paths and auto-wait).Use regex with
toHaveURLto avoid brittle exact matches and ensure auto-waiting.@@ - await expect(page).toHaveURL("/docs/integrations/vscode"); + await expect(page).toHaveURL(/\/docs\/integrations\/vscode\/?$/); @@ - await expect(page).toHaveURL("/docs/integrations/jetbrains"); + await expect(page).toHaveURL(/\/docs\/integrations\/jetbrains\/?$/); @@ - await expect(page).toHaveURL("/docs/how-to/load-environment-variables"); + await expect(page).toHaveURL(/\/docs\/how-to\/load-environment-variables\/?$/);Also applies to: 106-114, 143-155
5-86: Reduce duplication withtest.beforeEach(optional).Many tests start with
goto("/docs/quickstart")+waitForLoadState("networkidle"). Consider abeforeEachthat navigates once per test for maintainability.tooling/playwright-www/tests/accessibility.spec.ts (1)
7-9: Rely on global timeouts instead of per-call overrides.Inline
{ timeout: 60000 }can drift fromplaywright.config.ts. Prefer defaults unless there’s a known slow path.Also applies to: 47-49, 130-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tooling/playwright-www/tests/accessibility.spec.ts(1 hunks)tooling/playwright-www/tests/quickstart.spec.ts(1 hunks)tooling/playwright-www/tests/utils/console-errors.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tooling/playwright-www/tests/utils/console-errors.ts (1)
tooling/playwright-www/tests/hello-world.spec.ts (3)
consoleErrors(22-44)msg(25-29)page(3-20)
tooling/playwright-www/tests/quickstart.spec.ts (1)
tooling/playwright-www/tests/utils/console-errors.ts (1)
assertNoConsoleErrors(10-34)
tooling/playwright-www/tests/accessibility.spec.ts (1)
tooling/playwright-www/tests/utils/console-errors.ts (1)
assertNoConsoleErrors(10-34)
⏰ 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: test-e2e
🔇 Additional comments (1)
tooling/playwright-www/tests/accessibility.spec.ts (1)
1-4: Nice: shared console error utility adoption.Good reuse via
assertNoConsoleErrors; avoids duplication and keeps filters centralized.
…oved testing reliability - Added a step to verify that the web server is running before executing end-to-end tests. - Updated the end-to-end test commands to use `pnpm run` instead of `pnpm turbo run` for consistency. - Increased global timeout settings in the Playwright configuration for both the test suite and individual tests when running in CI. These changes aim to improve the reliability and efficiency of the testing process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tooling/playwright-www/playwright.config.ts (2)
44-49: Remove redundant Firefox timeout configuration.The
actionTimeoutandnavigationTimeoutvalues specified here duplicate the global settings already defined in theuseconfiguration (lines 31-32). Since these values are identical, Firefox will inherit them automatically without explicit declaration.Apply this diff to remove the redundancy:
{ name: "firefox", - use: { - ...devices["Desktop Firefox"], - // Firefox-specific timeout configuration - actionTimeout: 60000, - navigationTimeout: 60000, - }, + use: { ...devices["Desktop Firefox"] }, },
54-59: Remove redundant WebKit timeout configuration.The
actionTimeoutandnavigationTimeoutvalues specified here duplicate the global settings already defined in theuseconfiguration (lines 31-32). Since these values are identical, WebKit will inherit them automatically without explicit declaration.Apply this diff to remove the redundancy:
{ name: "webkit", - use: { - ...devices["Desktop Safari"], - // WebKit-specific timeout configuration - actionTimeout: 60000, - navigationTimeout: 60000, - }, + use: { ...devices["Desktop Safari"] }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(5 hunks)tooling/playwright-www/playwright.config.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
🧰 Additional context used
🧬 Code graph analysis (1)
tooling/playwright-www/playwright.config.ts (1)
tooling/playwright-www/tests/hello-world.spec.ts (1)
consoleErrors(22-44)
⏰ 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: test-e2e
🔇 Additional comments (3)
tooling/playwright-www/playwright.config.ts (3)
16-17: LGTM! Appropriate test suite timeout for E2E tests.The global timeout configuration (5 minutes on CI, 1 minute locally) is reasonable for the new end-to-end test suite.
30-32: LGTM! Global action and navigation timeouts set appropriately.The 60-second timeouts for actions and navigation provide adequate time for E2E test operations across all browsers.
88-88: LGTM! webServer timeout configured appropriately.The webServer startup timeout (2 minutes on CI, 1 minute locally) provides adequate time for the dev server to initialize before tests run.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tooling/playwright-www/playwright.config.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tooling/playwright-www/playwright.config.ts (1)
tooling/playwright-www/tests/hello-world.spec.ts (2)
consoleErrors(22-44)page(3-20)
⏰ 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: test-e2e
- Modified the `test:e2e` command in `turbo.json` to include a dependency on the `^build` task, ensuring that build artifacts are available before running tests. - Increased verbosity of the end-to-end test command in the GitHub Actions workflow by adding the `-vv` flag, providing more detailed output during test execution. These changes aim to enhance the reliability and clarity of the testing process.
…ed on CI environment - Modified the `reuseExistingServer` setting in the Playwright configuration to be false in CI environments, improving test isolation and reliability. - This change aims to enhance the efficiency of the testing process by ensuring a fresh server instance is used during tests when running in CI.
…es/ark.env into 51-add-end-to-end-testing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tooling/playwright-www/playwright.config.ts (1)
18-19: Local timeout remains too short for the comprehensive e2e suite.With the addition of comprehensive tests covering accessibility, documentation navigation, examples, homepage, integrations, morphs, quickstart, and search functionality, 60 seconds is insufficient for local runs. Developers will encounter timeouts before the suite completes, making local testing unreliable.
Consider matching the CI timeout locally or increasing to at least 3-5 minutes:
- timeout: process.env.CI ? 300000 : 60000, // 5 minutes on CI, 1 minute locally + timeout: 300000, // 5 minutes for both CI and local runs
🧹 Nitpick comments (1)
tooling/playwright-www/playwright.config.ts (1)
46-51: Remove redundant Firefox-specific timeouts.The Firefox project explicitly sets
actionTimeout: 60000andnavigationTimeout: 60000, which are identical to the global settings defined on lines 33-34. This duplication increases maintenance overhead without adding value.Consider removing the redundant timeout configuration:
{ name: "firefox", use: { ...devices["Desktop Firefox"], - // Firefox-specific timeout configuration - actionTimeout: 60000, - navigationTimeout: 60000, }, },If Firefox genuinely requires different timeouts in the future, they can be re-added with different values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tooling/playwright-www/playwright.config.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tooling/playwright-www/playwright.config.ts (1)
tooling/playwright-www/tests/hello-world.spec.ts (2)
consoleErrors(22-44)page(3-20)
⏰ 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: test-e2e
🔇 Additional comments (1)
tooling/playwright-www/playwright.config.ts (1)
106-106: Approve the webServer timeout increase.The 120-second timeout provides adequate buffer for the dev server to start, especially in CI environments or when dependencies need to be built.
…25 compatibility - Modified the GitHub Actions workflow to conditionally exclude the `www` app from tests and builds when using Node 25 or higher, addressing compatibility issues with fumadocs. - Updated Playwright configuration to adjust action and navigation timeouts for improved test performance. These changes aim to enhance the reliability of the testing process while ensuring compatibility with the latest Node.js versions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/tests.yml (1)
17-17: Timeout values remain insufficient—reconsider the guidance from the previous review.The job-level timeouts have been incremented (5 min for unit/type/build; 10 min for E2E) but still risk aborting routine CI operations. The enriched summary confirms the E2E timeout was reduced from 60→10 minutes, a substantial cut. On standard GitHub Actions runners,
pnpm install, Playwright browser downloads, server boot, and the expanded test suite easily consume 15+ minutes.The prior review explicitly recommended ≥15 min for unit/type/build and ≥20 min for E2E. Please reconsider restoring more conservative margins to prevent flaky CI failures due to timeouts rather than actual test failures.
Apply this diff to restore safer timeouts:
- timeout-minutes: 5 + timeout-minutes: 15For test-e2e (line 121):
- timeout-minutes: 10 + timeout-minutes: 30Also applies to: 56-56, 79-79, 121-121
tooling/playwright-www/playwright.config.ts (1)
18-19: Verify the local timeout is sufficient for the full test suite.A previous comment raised concerns that 60 seconds may be too short for the comprehensive e2e suite during local development. If you've tested the full suite locally and it completes within this limit, this configuration is fine. Otherwise, consider increasing the local timeout to match CI (300 seconds) to prevent deterministic timeouts.
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
43-53: Node 25 compatibility guard: Consider a simpler approach or validate the condition.The
bcutility for floating-point comparison is correct but adds a dependency. A simpler POSIX-compliant alternative using integer comparison exists, though the current logic is sound.Verify: Does the shell on the GitHub Actions runner have
bcavailable by default?You could simplify this to:
NODE_VERSION=$(node -v | cut -dv -f2 | cut -d. -f1) if (( NODE_VERSION >= 25 )); then # ... fiThis extracts just the major version without requiring
bc.Also applies to: 108-118
tooling/playwright-www/playwright.config.ts (1)
46-52: Remove redundant Firefox timeout configuration.The Firefox-specific
actionTimeoutandnavigationTimeoutduplicate the global values defined on lines 32-34. Since these are identical to the defaults that Firefox will inherit anyway, the explicit overrides are unnecessary and can cause confusion about whether Firefox actually requires special handling.Apply this diff to remove the redundant configuration:
{ name: "firefox", - use: { - ...devices["Desktop Firefox"], - // Firefox-specific timeout configuration - actionTimeout: 15000, - navigationTimeout: 30000, - }, + use: { ...devices["Desktop Firefox"] }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(5 hunks)tooling/playwright-www/playwright.config.ts(4 hunks)
⏰ 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: test-e2e
🔇 Additional comments (6)
.github/workflows/tests.yml (2)
133-134: Turbo base/head fallback is well-formed.The use of
github.event.pull_request.base.sha || github.event.beforeas a fallback is a defensive pattern that handles both PR and push contexts correctly.
161-167: E2E environment configuration looks appropriate.The environment variables (CI, PLAYWRIGHT_TIMEOUT, DEBUG, PLAYWRIGHT_HEADLESS) are well-suited for Playwright E2E runs. The 300s (5 min) per-action timeout provides adequate headroom for individual Playwright actions. Verbose mode (
-vv) on PR runs will aid debugging without cluttering main-branch logs.Also applies to: 172-176
tooling/playwright-www/playwright.config.ts (4)
16-17: Comment is now accurate.The comment correctly describes the
maxFailuressetting and addresses the previous concern about the misleading reference to workers.
32-34: Action and navigation timeouts are well-configured.The 15-second action timeout and 30-second navigation timeout are appropriate and leave sufficient budget for test setup and assertions. This addresses the previous concern about timeouts matching the global test timeout.
105-105: Server reuse strategy is correct.The configuration now properly allows reusing an existing dev server locally while ensuring a fresh server in CI. This addresses the previous concern about port conflicts during local development.
106-106: Web server timeout is appropriate.The 120-second timeout provides sufficient time for the dev server to start while preventing indefinite hangs if startup fails.
- Removed conditional logic for Node 25 compatibility in test and build steps, streamlining the workflow. - Updated node-version matrix to exclude "latest" until fumadocs resolves Node 25 support issues, with a reference to the related GitHub issue. These changes aim to enhance the clarity and maintainability of the CI configuration.
1 participant
Add comprehensive Playwright tests for accessibility, documentation navigation, examples, homepage, integrations, morphs, quickstart, and search functionality
Summary by CodeRabbit
Tests
Chores