Conversation
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughUpdated Cypress tests for the MCP tab to avoid stale DOM references by re-querying server rows via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Issues
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
2 times, most recently
from
0a161bc to
8584df2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts (1)
422-427:⚠️ Potential issue | 🟡 MinorRe-query the server row again after saving the tools modal.
Line 427 reuses
freshServerRowafterfindSaveButton().click()closes the tools modal. That is the same stale-subject pattern this PR fixes elsewhere; the later reopen flow at Lines 569-576 already re-queries before clicking.Proposed fix
cy.step('Save tool selection'); mcpToolsModal.findSaveButton().click(); mcpToolsModal.find().should('not.exist'); cy.step('Re-open tools modal to verify persistence'); - freshServerRow.findToolsButton().click(); + const reopenedServerRow = playgroundPage.mcpTab.getServerRow(serverName, serverUrl); + reopenedServerRow.findToolsButton().should('exist').and('not.have.attr', 'aria-disabled'); + reopenedServerRow.findToolsButton().click(); mcpToolsModal.find().should('be.visible');As per coding guidelines,
packages/*/frontend/src/__tests__/cypress/**/*.{ts,js}tests should “ensure idempotency.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts` around lines 422 - 427, The test reuses the stale subject freshServerRow after closing the modal, causing flaky behavior; after mcpToolsModal.findSaveButton().click() and mcpToolsModal.find().should('not.exist'), re-query the server row (i.e., reassign or fetch a new freshServerRow using the same selector/helper used originally) before calling freshServerRow.findToolsButton().click() so the click operates on a fresh DOM subject rather than the stale one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts`:
- Line 384: The test helper is passing { timeout: 30000 } to
.should('be.visible') which Cypress ignores; move the timeout into the query
command by updating the MCP tab helpers so that findMCPServersTable (and any
cy.findByTestId wrappers used by verifyMCPTabVisible) accept an options
parameter and forward it to cy.findByTestId(..., { timeout: 30000 }), then
remove the timeout from the .should() call in verifyMCPTabVisible; update all
calls to playgroundPage.mcpTab.verifyMCPTabVisible (and other helpers that rely
on findMCPServersTable) to rely on the helper’s forwarded options rather than
passing timeout to .should().
---
Outside diff comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts`:
- Around line 422-427: The test reuses the stale subject freshServerRow after
closing the modal, causing flaky behavior; after
mcpToolsModal.findSaveButton().click() and
mcpToolsModal.find().should('not.exist'), re-query the server row (i.e.,
reassign or fetch a new freshServerRow using the same selector/helper used
originally) before calling freshServerRow.findToolsButton().click() so the click
operates on a fresh DOM subject rather than the stale one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 747a5365-ed68-467c-9fa7-4fd8f654ea17
📒 Files selected for processing (1)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
from
5362ee2 to
bb3df70
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7340 +/- ##
=======================================
Coverage 65.04% 65.05%
=======================================
Files 2458 2458
Lines 76443 76443
Branches 19289 19289
=======================================
+ Hits 49726 49733 +7
+ Misses 26717 26710 -7 see 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
2 times, most recently
from
93cfaf4 to
98fc68d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/playgroundPage/mcpTab.ts (1)
39-47: De-duplicate table-visibility assertion.
openMCPTabandverifyMCPTabVisiblenow carry identicalfindMCPServersTable({ timeout: 30000 }).should('exist').and('be.visible')logic. HaveopenMCPTabdelegate so the timeout/assertion contract lives in one place.Proposed refactor
openMCPTab(): void { // Click the MCP tab to show the servers table this.clickMCPTab(); - this.findMCPServersTable({ timeout: 30000 }).should('exist').and('be.visible'); - } - - verifyMCPTabVisible(): void { - this.findMCPServersTable({ timeout: 30000 }).should('exist').and('be.visible'); + this.verifyMCPTabVisible(); + } + + verifyMCPTabVisible(): void { + this.findMCPServersTable({ timeout: 30000 }).should('exist').and('be.visible'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/playgroundPage/mcpTab.ts` around lines 39 - 47, The two methods duplicate the same table-visibility assertion; refactor so openMCPTab delegates to verifyMCPTabVisible: keep the single assertion in verifyMCPTabVisible using findMCPServersTable({ timeout: 30000 }).should('exist').and('be.visible'), and change openMCPTab to call clickMCPTab() then call verifyMCPTabVisible() (removing the duplicated findMCPServersTable call from openMCPTab); ensure references to openMCPTab remain compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/playgroundPage/mcpTab.ts`:
- Around line 39-47: The two methods duplicate the same table-visibility
assertion; refactor so openMCPTab delegates to verifyMCPTabVisible: keep the
single assertion in verifyMCPTabVisible using findMCPServersTable({ timeout:
30000 }).should('exist').and('be.visible'), and change openMCPTab to call
clickMCPTab() then call verifyMCPTabVisible() (removing the duplicated
findMCPServersTable call from openMCPTab); ensure references to openMCPTab
remain compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f403c335-3e43-4919-8967-1ecd7bd0719d
📒 Files selected for processing (2)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/playgroundPage/mcpTab.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gen-ai/frontend/src/tests/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
from
98fc68d to
9699903
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts (2)
477-478: RedundantfindToolsButton()lookup before.click().
findToolsButton()is invoked twice back-to-back. Chain the click onto the existing assertion to keep a single query, matching the pattern at line 387.♻️ Proposed refactor
- freshServerRow.findToolsButton().should('exist').and('not.have.attr', 'aria-disabled'); - freshServerRow.findToolsButton().click(); + freshServerRow + .findToolsButton() + .should('exist') + .and('not.have.attr', 'aria-disabled') + .click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts` around lines 477 - 478, The test calls freshServerRow.findToolsButton() twice; replace the second lookup by chaining .click() onto the assertion to avoid duplicate queries — i.e., use freshServerRow.findToolsButton().should('exist').and('not.have.attr', 'aria-disabled').click(); so the single selector (findToolsButton) performs both the assertion and the click.
576-578: Chain thefindToolsButton()assertions to avoid three separate queries.Each
findToolsButton()call re-queries the DOM. Cypress retries assertions on the same subject, so chaining is equivalent, cheaper, and less prone to subject re-resolution between assertions.♻️ Proposed refactor
- reopenServerRow.findToolsButton().should('exist'); - reopenServerRow.findToolsButton().should('not.have.attr', 'aria-disabled'); - reopenServerRow.findToolsButton().click(); + reopenServerRow + .findToolsButton() + .should('exist') + .and('not.have.attr', 'aria-disabled') + .click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts` around lines 576 - 578, Replace the three separate reopenServerRow.findToolsButton() calls with a single chained call: call reopenServerRow.findToolsButton() once, then chain .should('exist').and('not.have.attr', 'aria-disabled') and finally .click(); this keeps the same assertions and click but avoids re-querying the DOM between assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts`:
- Around line 477-478: The test calls freshServerRow.findToolsButton() twice;
replace the second lookup by chaining .click() onto the assertion to avoid
duplicate queries — i.e., use
freshServerRow.findToolsButton().should('exist').and('not.have.attr',
'aria-disabled').click(); so the single selector (findToolsButton) performs both
the assertion and the click.
- Around line 576-578: Replace the three separate
reopenServerRow.findToolsButton() calls with a single chained call: call
reopenServerRow.findToolsButton() once, then chain
.should('exist').and('not.have.attr', 'aria-disabled') and finally .click();
this keeps the same assertions and click but avoids re-querying the DOM between
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5edbe5c0-9c14-4ee2-bbad-f336cf765fcc
📒 Files selected for processing (2)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/playgroundPage/mcpTab.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/playgroundPage/mcpTab.ts
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
from
197b968 to
e665ec4
Compare
…ferences The comprehensive tool selection test was failing because after closing the success modal, the table component remounts and the old serverRow reference becomes stale. Re-querying the server row after the modal closes ensures we get a fresh reference to the DOM element, fixing the 'element not found' timeout error. This matches the pattern used in other tests that don't have issues with stale references because they re-query elements after significant UI changes.
…nces Fix flaky test by re-querying the server row after modal closes to avoid stale DOM element references. After the success modal closes, the table component may remount, making the old serverRow reference stale. Re-querying ensures we get a fresh reference to the DOM element before interacting with the tools button. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… to avoid stale references Apply the same stale element fix to the "comprehensive tool selection operations" test that was previously applied to the auto-unlock test. Prevents Cypress timeout errors when clicking tools button after modal closes and components remount. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Apply stale element fixes to two more locations: - First test: Re-query server row after success modal closes before clicking tools button - Auto-unlock test: Use freshServerRow instead of old serverRow when re-opening tools modal These follow the same pattern as previous fixes to prevent Cypress timeout errors from stale DOM references. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…n in comprehensive test
Add .should('exist').and('not.have.attr', 'aria-disabled') assertions before clicking the tools button in the comprehensive tool selection test. This ensures Cypress waits for the element to be ready before attempting to click, preventing timeout errors when the element is still mounting.
Follows the same defensive pattern used in the auto-unlock test.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…al closes
Replace direct findMCPServersTable().should('be.visible') calls with
verifyMCPTabVisible() method which includes a 30-second timeout to
handle component remount delays after success modal closes.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…dal in comprehensive test Add verifyMCPTabVisible() call before querying server row when re-opening the tools modal to verify selection persistence. This prevents timeout errors when the MCP table hasn't fully remounted after closing the previous modal. Completes the fix started in previous commits by applying the same defensive pattern to the final location that was missing the visibility check. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…es in MCP tool selection test Forces immediate row query after table visibility check using .then() callback, eliminating stale element references without explicit timeouts. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ale references Query for the row and tools button directly within the table element captured in .then() callback, avoiding lazy page object selectors that can become stale after table remounts. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ned element Use .within() to scope findByTestId search to children of the table row, ensuring the tools button is found inside the row element. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…s in MCP tools button After modal closes and MCP table remounts, directly query for the tools button using its testID instead of going through the page object's lazy selector. This avoids stale element references and ensures the element is found after the DOM updates. The testID uses serverUrl which matches server.id (set in transformMCPServerData), so direct query should reliably find the element. Related: RHOAIENG-49598
…omprehensive test Replace the direct cy.findByTestId() approach with the page object getServerRow() method that was already working in other parts of the test. The .and() chaining was returning undefined, causing the .click() to fail. Split assertions into separate calls to allow proper Cypress retries and avoid stale element references. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…Tab tests
1. Fix verifyMCPTabVisible() timeout handling:
- Move timeout to .should('exist', { timeout: 30000 }) so Cypress honors it
- Chain with .and('be.visible') per openMCPTab() pattern
- Cypress ignores timeout when passed directly to .should('be.visible')
2. Use centralized verifyMCPTabVisible() helper:
- Replace inline .findMCPServersTable().should('be.visible')
- Ensures consistent remount-tolerant visibility checks
3. Re-query server rows after modal closes:
- Add fresh row queries in 45-tools and 40-tools warning tests
- Prevents stale element references after verifyMCPTabVisible()
- Follows same pattern used elsewhere in the test file
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Simplify verifyMCPTabVisible() to use a single should() assertion with timeout parameter instead of chaining exist and be.visible. This ensures the 30-second timeout is properly applied to the visibility check, improving reliability when waiting for the MCP servers table after modal interactions and tab remounts. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…erence After saving tool selection and closing the modal in the auto-unlock test, re-query the server row before clicking the tools button again. This prevents stale element references that can cause flaky test failures when the DOM updates after modal interactions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…check Restore the original chained assertion pattern instead of the simplified version. The chained approach ensures the element exists in the DOM before checking visibility, which is more reliable when waiting for components that may remount after modal interactions. The timeout applies to the existence check, and visibility is verified immediately after with Cypress built-in retry logic. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…b page object Update MCP tab page object to properly handle Cypress timeout options and remove code duplication between openMCPTab and verifyMCPTabVisible. - Add optional timeout parameter to findMCPServersTable that forwards to cy.findByTestId instead of passing to .should() where it is ignored - Refactor openMCPTab to delegate to verifyMCPTabVisible to maintain single source of truth for table visibility assertion This fixes flaky test behavior where the 30-second timeout was not being applied correctly to element queries. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… page object
Replace custom { timeout?: number } type with Partial<Cypress.Loggable & Timeoutable>
to match Cypress's standard type signature. This fixes TypeScript compilation errors
and properly supports all Cypress query options.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
toklumpp
force-pushed
the
flaky-genai-tool-selection-mock
branch
from
e665ec4 to
617d166
Compare
1 participant
Description
Fix flaky mcpTab test
How Has This Been Tested?
Run the flaky test multiple times
Test Impact
Only tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
[ ] Included any necessary screenshots or gifs if it was a UI change.[ ] Included tags to the UX team if it was a UI/UX change.After the PR is posted & before it merges:
mainSummary by CodeRabbit