Conversation
WalkthroughUpdates Playwright E2E tests in Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (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 |
| await addFilter(page, "dateRange"); | ||
| const entriesListRespPromise = page.waitForResponse( | ||
| (response) => response.url().includes("outOfOfficeEntriesList") && response.status() === 200 | ||
| ); |
There was a problem hiding this comment.
Incorrect uses od waitforresponses, causing flakiness
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/playwright/out-of-office.e2e.ts (1)
304-312: Don’t reuse resolved waitForResponse promises; create a fresh one before each triggering action.Re‑awaiting an already‑resolved promise is a no‑op and can reintroduce flakes when opening the modal again or after user switches. Create new waiters each time, before click/navigate.
Example fixes:
@@ - // add another entry - await entriesListRespPromise; - await page.getByTestId("add_entry_ooo").click(); - await reasonListRespPromise; + // add another entry + const reasonListRespPromise2 = page.waitForResponse( + (response) => response.url().includes("outOfOfficeReasonList?batch=1") && response.status() === 200 + ); + await page.getByTestId("add_entry_ooo").click(); + await reasonListRespPromise2; await page.locator('[data-testid="date-range"]').click(); await selectToAndFromDates(page, "11", "24");@@ - await member1User?.apiLogin(); - await page.goto("/settings/my-account/out-of-office"); - await page.waitForLoadState("domcontentloaded"); - await entriesListRespPromise; - await addOOOButton.click(); - await reasonListRespPromise; + await member1User?.apiLogin(); + const entriesListRespPromise2 = page.waitForResponse( + (response) => response.url().includes("outOfOfficeEntriesList") && response.status() === 200 + ); + await page.goto("/settings/my-account/out-of-office"); + await entriesListRespPromise2; + const reasonListRespPromise2 = page.waitForResponse( + (response) => response.url().includes("outOfOfficeReasonList?batch=1") && response.status() === 200 + ); + await addOOOButton.click(); + await reasonListRespPromise2;Also applies to: 346-355, 384-387, 429-437, 481-486
🧹 Nitpick comments (4)
apps/web/playwright/out-of-office.e2e.ts (4)
526-527: Duplicate await on the same promise.Consecutive awaits on the same resolved promise add no value and obscure intent. Remove the duplicates (or create distinct waiters if two fetches are expected).
- await legacyListMembersRespPromise; - await legacyListMembersRespPromise; + await legacyListMembersRespPromise;- await legacyListMembersRespPromise; - await legacyListMembersRespPromise; + await legacyListMembersRespPromise;- await legacyListMembersRespPromise; - await legacyListMembersRespPromise; + await legacyListMembersRespPromise;Also applies to: 544-545, 557-558
226-227: Use visibility assertions, not toBeTruthy, on locators.toBeTruthy() on a Locator always passes; it doesn’t assert DOM state. Use toBeVisible().
- await expect(page.getByTestId("away-emoji")).toBeTruthy(); + await expect(page.getByTestId("away-emoji")).toBeVisible();- await expect(page.locator(`text=${t("booking_redirect_infinite_not_allowed")}`)).toBeTruthy(); + await expect(page.locator(`text=${t("booking_redirect_infinite_not_allowed")}`)).toBeVisible();- expect(page.locator(`text=${t("success_deleted_entry_out_of_office")}`)).toBeTruthy(); + await expect(page.locator(`text=${t("success_deleted_entry_out_of_office")}`)).toBeVisible();Also applies to: 486-487, 569-570
59-60: Stray ‘}’ in team slug template.The extra brace likely leaks into slug; trim it.
- slug: `test-insights-${Date.now()}-${randomString(5)}}`, + slug: `test-insights-${Date.now()}-${randomString(5)}`,Also applies to: 122-123
837-861: Optional: factor a small helper to DRY out “entries list” waits.You repeatedly wait for outOfOfficeEntriesList. Consider a tiny helper (e.g., awaitEntriesList(page)) to reduce duplication and mistakes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/web/playwright/out-of-office.e2e.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/out-of-office.e2e.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/playwright/out-of-office.e2e.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/playwright/out-of-office.e2e.ts
🧬 Code graph analysis (1)
apps/web/playwright/out-of-office.e2e.ts (1)
apps/web/playwright/filter-helpers.ts (1)
addFilter(11-14)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
- GitHub Check: Tests / Unit
🔇 Additional comments (2)
apps/web/playwright/out-of-office.e2e.ts (2)
781-783: Good fix: set waiter before firing the request.Creating the response waiter before calling addFilter removes the race. LGTM.
842-849: Good fix: pre-arm waiters for both filter open and preset click.Both waits are now created before their triggering actions. LGTM.
E2E results are ready! |
✅ Actions performedSummary regeneration triggered. |
2 participants
Updates out-of-office.e2e.ts test to create the response wait promise (page.waitForResponse for outOfOfficeEntriesList) before triggering the dateRange filter. The sequence now sets up the response listener prior to calling addFilter(page, "dateRange"), ensuring the intended network response is awaited.