test: improve chat test stability on CI

[serbangeorge-m]

No description provided.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactored Playwright test utilities in the chat page model to replace index-based message targeting with content-based operations. Replaced localStorage manipulation with explicit response waiting logic and updated test specs to use the new helper method signatures.

Changes

Cohort / File(s)	Summary
Chat Page Test Utilities tests/playwright/src/model/pages/chat-page.ts	Replaced clearLastUsedModel() with waitForModelResponse() for explicit response waiting. Changed message editing from index-based (clickEditOnUserMessage(index)) to content-based (clickEditOnUserMessage(messageText)). Refactored message dimming verification from index-based to content-based locator matching. Added verifyMessagesAfterEditAreNotDimmed() helper.
Chat Smoke Test Specs tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts	Removed clearLastUsedModel() setup call. Updated test cases to use waitForModelResponse() and content-based message editing. Changed assertions from index-based message graying to content-based dimming verification.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat: remember last used model in chat  #1120: Introduces waitForModelResponse() and removes clearLastUsedModel() localStorage manipulation, reversing the same test helper behavior being refactored here.
• test: improve test reliability on CI runners #1119: Modifies the same chat-page test utilities with changes to sendMessage behavior and conversation message locators.
• chore(test): add chat button lifecycle test #755: Modifies ChatPage UI controls around generation and stop button visibility checks related to message waiting logic.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The pull request has no description provided, which is a lenient check that passes as long as content is not off-topic; however, an empty description provides no context about the changes.	Add a description explaining why these test changes improve stability (e.g., replacing index-based selectors with text-based ones, removing localStorage manipulation for better isolation).

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: refactoring chat tests to improve stability on CI by replacing flaky patterns (index-based selectors, localStorage manipulation) with more reliable ones (text-based selectors, proper wait conditions).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

tests/playwright/src/model/pages/chat-page.ts (2)

326-329: Potential race condition: double-wait may exceed reasonable timeouts.

Both modelConversationMessages.first() visibility check and verifySendButtonVisible use TIMEOUTS.MODEL_RESPONSE (90s). In worst case, if the model message appears just before timeout and the send button takes additional time, total wait could approach 180s. Consider whether one of these should use a shorter timeout since once the model response appears, the send button should follow shortly.

♻️ Suggested refinement

 async waitForModelResponse(): Promise<void> {
   await expect(this.modelConversationMessages.first()).toBeVisible({ timeout: TIMEOUTS.MODEL_RESPONSE });
-  await this.verifySendButtonVisible(TIMEOUTS.MODEL_RESPONSE);
+  await this.verifySendButtonVisible(TIMEOUTS.STANDARD);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/playwright/src/model/pages/chat-page.ts` around lines 326 - 329, The
double-wait in waitForModelResponse (checking
this.modelConversationMessages.first() then calling verifySendButtonVisible)
both use TIMEOUTS.MODEL_RESPONSE causing a potential combined wait of ~180s;
update waitForModelResponse to use the long TIMEOUTS.MODEL_RESPONSE only for the
modelConversationMessages visibility and pass a much shorter timeout (e.g., a
new or existing short/ui timeout like TIMEOUTS.SHORT or TIMEOUTS.UI_UPDATE) into
verifySendButtonVisible so the send-button check does not duplicate the full
MODEL_RESPONSE duration; keep identifiers: waitForModelResponse,
modelConversationMessages.first(), verifySendButtonVisible, and
TIMEOUTS.MODEL_RESPONSE.

---

331-335: Using force: true may mask actual visibility issues.

The force: true option bypasses actionability checks. If the edit button isn't actually visible or clickable due to a UI bug, this won't catch it. Consider whether the hover should be sufficient to make the button actionable without forcing.

♻️ Suggested improvement

 async clickEditOnUserMessage(messageText: string): Promise<void> {
   const messageLocator = this.userConversationMessages.filter({ hasText: messageText });
   await messageLocator.hover();
-  await messageLocator.getByLabel('Edit message').click({ force: true });
+  const editButton = messageLocator.getByLabel('Edit message');
+  await expect(editButton).toBeVisible();
+  await editButton.click();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/playwright/src/model/pages/chat-page.ts` around lines 331 - 335, The
clickEditOnUserMessage method should not bypass actionability checks with force:
true; remove the { force: true } from the getByLabel('Edit message').click call,
ensure the hover on this.userConversationMessages.filter({ hasText: messageText
}) actually makes the "Edit message" button visible, and if necessary add an
explicit wait/assertion (e.g., waitFor or expect to be visible) on the
getByLabel('Edit message') locator before clicking so the click fails when the
button is not actionable.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/playwright/src/model/pages/chat-page.ts`:
- Around line 326-329: The double-wait in waitForModelResponse (checking
this.modelConversationMessages.first() then calling verifySendButtonVisible)
both use TIMEOUTS.MODEL_RESPONSE causing a potential combined wait of ~180s;
update waitForModelResponse to use the long TIMEOUTS.MODEL_RESPONSE only for the
modelConversationMessages visibility and pass a much shorter timeout (e.g., a
new or existing short/ui timeout like TIMEOUTS.SHORT or TIMEOUTS.UI_UPDATE) into
verifySendButtonVisible so the send-button check does not duplicate the full
MODEL_RESPONSE duration; keep identifiers: waitForModelResponse,
modelConversationMessages.first(), verifySendButtonVisible, and
TIMEOUTS.MODEL_RESPONSE.
- Around line 331-335: The clickEditOnUserMessage method should not bypass
actionability checks with force: true; remove the { force: true } from the
getByLabel('Edit message').click call, ensure the hover on
this.userConversationMessages.filter({ hasText: messageText }) actually makes
the "Edit message" button visible, and if necessary add an explicit
wait/assertion (e.g., waitFor or expect to be visible) on the getByLabel('Edit
message') locator before clicking so the click fails when the button is not
actionable.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1eba9701-76de-4d7c-8182-ba969e615f3a

📥 Commits

Reviewing files that changed from the base of the PR and between 2a09493 and 2fe2f27.

📒 Files selected for processing (2)

• tests/playwright/src/model/pages/chat-page.ts
• tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!