🧪 Add filtering and Sorting Issues and Files UI tests #688
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🧹 Nitpick comments (7)
tests/e2e/pages/vscode.page.ts (4)
186-199: Prefer a state-based wait over a fixed 20s sleepThe 20s sleep slows the suite and can still be flaky. Downstream calls to getListNames already wait for visible headings. Consider dropping the fixed wait and, if needed, polling for data readiness.
Apply this diff to remove the hard sleep:
await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(20000);If you observe flakiness after removal, replace it with a lightweight readiness check:
- await this.window.waitForTimeout(20000); + await expect + .poll(async () => (await this.getListNames(kind)).length, { timeout: 20000, intervals: [200, 400, 800] }) + .toBeGreaterThan(0);
201-212: getListNames ignores the kind parameter; may filter out Files itemsThe current filter requires a “Get solution” button, which might only exist for issues. That can cause getListNames('files') to return an empty list.
Apply this diff to use the kind parameter and conditionally apply the “Get solution” filter:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view + let listCards = view .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + .filter({ has: view.getByRole('heading', { level: 3 }) }); + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 30_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
214-248: Type the return value of openCategory for stronger checksHelps with IntelliSense and avoids accidental misuse of the returned shapes.
Apply this diff:
- public async openCategory() { + public async openCategory(): Promise<{ categoryBtn: Locator; menu: Locator | null; options: Locator }> {And update the import (outside this range):
// at top of file - import { expect } from '@playwright/test'; + import { expect, Locator } from '@playwright/test';
250-279: Guard the clearing loop to avoid a potential infinite loopIf the UI stops toggling the state (e.g., transient focus loss), this can spin forever. Add a max-iteration guard and fail fast with a clear error.
- for (;;) { - const n = await selected().count(); - if (n === 0) break; + for (let guard = 0; guard < 100; guard++) { + const n = await selected().count(); + if (n === 0) break; await selected().first().click(); if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { ({ categoryBtn } = await this.openCategory()); } else { await this.window.waitForTimeout(50); } } + if (await selected().count()) { + throw new Error('Failed to clear category selections within 100 attempts'); + }tests/e2e/tests/configure-and-run-analysis.test.ts (3)
44-55: Add a guard to skip when there aren’t enough issues to validate orderingIf there’s only one issue, descending may also appear “ascending,” causing a false failure.
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'descending'); const names_descending = await vscodeApp.getListNames('issues'); + if (names_descending.length < 2) { + test.skip(true, `Not enough issues to validate sorting (got ${names_descending.length}).`); + } //console.log('Issue names:', names_descending); expect(isAscending(names_descending)).toBe(false); await vscodeApp.setListKindAndSort('issues', 'ascending'); const names_ascending = await vscodeApp.getListNames('issues'); //console.log('Issue names:', names_ascending); expect(isAscending(names_ascending)).toBe(true); });
77-90: Skip when file list is too small to assert both sort directionsPrevents false negatives on minimal datasets.
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const files_ascending = await vscodeApp.getListNames('files'); + if (files_ascending.length < 2) { + test.skip(true, `Not enough files to validate sorting (got ${files_ascending.length}).`); + } expectFilesListLooksValid(files_ascending); const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); //console.log('Issue names:', files_ascending); expect(files_ascending).toEqual(asc); await vscodeApp.setListKindAndSort('files', 'descending'); const files_descending = await vscodeApp.getListNames('files'); const desc = asc.slice().reverse(); //console.log('Issue names:', files_descending); expect(files_descending).toEqual(desc); });
92-109: Reduce flakiness by searching for a unique filename; otherwise skipUsing the first filename can match multiple entries (e.g., repeated “pom.xml”). Prefer a unique candidate to guarantee a single result.
test('Files: search narrows to one; clearing expands again', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const all = await vscodeApp.getListNames('files'); - expect(all.length).toBeGreaterThan(0); - const filename = all[0]; + expect(all.length).toBeGreaterThan(0); + // Prefer a unique candidate to avoid ambiguous matches + const counts = new Map<string, number>(); + for (const n of all) counts.set(n, (counts.get(n) ?? 0) + 1); + const unique = all.find((n) => (counts.get(n) ?? 0) === 1); + if (!unique) { + test.skip(true, 'No unique file name to narrow the results to a single item.'); + } + const filename = unique!; await vscodeApp.searchViolation(filename); await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBe(1); const names1 = await vscodeApp.getListNames('files'); expect(names1).toEqual([filename]); await vscodeApp.searchViolation(''); await expect .poll(async () => (await vscodeApp.getListNames('files')).length) .toBeGreaterThan(1); const names2 = await vscodeApp.getListNames('files'); expect(names2.length).toBeGreaterThan(1); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 283-283: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (7)
tests/e2e/pages/vscode.page.ts (2)
14-15: LGTM: clear, narrow string literal types for sort order and list kindThese aliases make call sites self-documenting and reduce typo risk.
281-297: Regex construction is safely escaped; looks goodtoRegex() escapes user-provided strings, anchoring matches. The ReDoS warning from static analysis does not apply here.
tests/e2e/tests/configure-and-run-analysis.test.ts (5)
36-36: LGTM: extended wait accommodates slower environmentsAligns with prior learnings about cross-OS timing variability.
40-43: LGTM: locale-aware sort helperUsing Intl.Collator with numeric:true is the right call for natural ordering.
56-75: LGTM: pragmatic validation of file-like namesGood balance between specificity and robustness for CI data variance.
111-112: LGTM: category constants scoped and explicitMakes the filter intent clear and avoids typos.
113-137: LGTM: category bounds check with pollingThe baseline reset + per-category <= baseline invariant is a solid, low-flake assertion. Cleanup at the end is good.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
42401cb to
b53be55
Compare
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/e2e/pages/vscode.page.ts (3)
186-199: Make setListKindAndSort idempotent and replace fixed 20s sleep with a state-based wait
- Clicking unconditionally can de-select a pressed toggle in some UIs. Check aria-pressed and only click when needed.
- The fixed 20s wait is brittle and slows the suite. Prefer a wait that verifies the list is actually sorted. This new wait is unrelated to the existing, intentionally-long waits in runAnalysis(), per our learnings.
Apply this diff to harden the method:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } - const sortButton = analysisView.getByRole('button', { - name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', - }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(20000); + const ascBtn = analysisView.getByRole('button', { name: 'Sort ascending' }); + const descBtn = analysisView.getByRole('button', { name: 'Sort descending' }); + const targetBtn = order === 'ascending' ? ascBtn : descBtn; + if ((await targetBtn.getAttribute('aria-pressed')) !== 'true') { + await targetBtn.click(); + await expect(targetBtn).toHaveAttribute('aria-pressed', 'true'); + } + await expect + .poll( + async () => { + const names = await this.getListNames(kind); + if (names.length < 2) return false; + const sorted = [...names].sort((a, b) => + a.localeCompare(b, undefined, { sensitivity: 'base' }) + ); + if (order === 'descending') sorted.reverse(); + return names.join('\n') === sorted.join('\n'); + }, + { timeout: 15000 } + ) + .toBeTruthy(); }
214-248: Remove the any-cast and (optionally) return a typed shape
- The (menu as any) cast is unnecessary; type narrowing on the null-check suffices.
- Optional: add an explicit return type to make downstream usage clearer.
Apply this diff to drop the any-cast:
public async openCategory() { const view = await this.getView(KAIViews.analysisView); const categoryBtn = view.getByRole('button', { name: /^Category(?:\s*\d+)?$/i }); await categoryBtn.scrollIntoViewIfNeeded(); @@ const controlsId = await categoryBtn.getAttribute('aria-controls'); let menu = controlsId ? view.locator(`#${controlsId}`) : null; - let options = view.locator( + let options = view.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); if (menu) { - options = (menu as any).locator( + options = menu.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); }If you want to make the return type explicit, update the signature to:
public async openCategory(): Promise<{ categoryBtn: Locator; menu: Locator | null; options: Locator }>and add this import at the top of the file:
import type { Locator } from 'playwright';
250-279: Potential infinite loop when clearing radio selections; add a guard and bail-outIf the control behaves like a radio group without a “clear” option, clicking the selected item may not unselect it, causing the for(;;) loop to spin forever.
Apply this diff to bound the loop and fail fast:
- for (;;) { - const n = await selected().count(); - if (n === 0) break; + for (let guard = 0; guard < 25; guard++) { + const n = await selected().count(); + if (n === 0) break; await selected().first().click(); if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { ({ categoryBtn } = await this.openCategory()); } else { await this.window.waitForTimeout(50); } } + if (await selected().count()) { + throw new Error('Category selections could not be cleared after multiple attempts'); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 283-283: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
tests/e2e/pages/vscode.page.ts (2)
14-15: Type aliases for sorting and list kind look goodClear, self-documenting string literal types that align with the UI semantics.
281-297: Regex construction is safe (escaped and anchored); consider asserting selection took effect
- Static analysis warns about regexp-from-variable, but you escape all metacharacters and anchor the pattern, which mitigates ReDoS in this usage.
- Minor: after clicking the option, assert at least one option is selected/checked to make the helper more robust.
Apply this diff to verify selection:
await expect(opt).toBeVisible({ timeout: 5000 }); await opt.click(); await this.window.waitForTimeout(50); + await expect + .poll( + async () => + await options + .locator('[aria-checked="true"], [aria-selected="true"]') + .count(), + { timeout: 3000 } + ) + .toBeGreaterThan(0); if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); }
There was a problem hiding this comment.
Looks Good . This test runs on all PRs though . I don't think we want sorting and filtering tests to run on every PR . We can move these to a separate file . WDYT ? @hhpatel14 @abrugaro ?
hhpatel14
force-pushed
the
665-filteringIssue
branch
2 times, most recently
from
9d92192 to
8a28839
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
211-222: getListNames ignores ListKind and likely excludes Files due to “Get solution” filterThis method filters cards to those with a "Get solution" button, which fits Issues but not Files. It also ignores the kind parameter, breaking Files retrieval.
Apply this diff to honor the kind and include Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Narrow to issues when applicable; Files typically won't have "Get solution" + if (kind === 'issues') { + listCards = listCards.filter({ has: view.getByRole('button', { name: /get solution/i }) }); + } const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 30_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
🧹 Nitpick comments (3)
tests/e2e/pages/vscode.page.ts (1)
191-204: Avoid hard 20s sleep; make the interaction idempotent and wait on UI change insteadUnconditional clicks and a fixed 20s wait make tests slow and flaky. Click only when needed (based on aria-pressed) and wait for the list to change when a click happens. This keeps cross-OS robustness without a blind sleep.
Apply this diff to make the method idempotent and event-driven:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(20000); + const titles = analysisView.getByRole('heading', { level: 3 }); + const before = await titles.allInnerTexts(); + const wasPressed = (await sortButton.getAttribute('aria-pressed')) === 'true'; + if (!wasPressed) { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + // Wait for ordering/content to refresh (only when we actually toggled) + await expect + .poll(async () => (await titles.allInnerTexts()).join('\n'), { timeout: 30_000 }) + .not.toBe(before.join('\n')); + } }tests/e2e/tests/filter-and-sort-violations.test.ts (2)
49-59: Guard against degenerate lists to avoid false failuresIf there are fewer than 2 issues, verifying descending “is not ascending” will fail despite the environment being valid. Add an explicit length assertion so failures are clear and actionable.
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'descending'); const names_descending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_descending); + //console.log('Issue names:', names_descending); + expect(names_descending.length).toBeGreaterThan(1); expect(isAscending(names_descending)).toBe(false); await vscodeApp.setListKindAndSort('issues', 'ascending'); const names_ascending = await vscodeApp.getListNames('issues'); //console.log('Issue names:', names_ascending); expect(isAscending(names_ascending)).toBe(true); });
91-104: Make sorting checks meaningful for Files as wellAssert that there are at least two files before comparing ascending vs descending to ensure the test actually exercises ordering logic.
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const files_ascending = await vscodeApp.getListNames('files'); expectFilesListLooksValid(files_ascending); + expect(files_ascending.length).toBeGreaterThan(1); const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); //console.log('Issue names:', files_ascending); expect(files_ascending).toEqual(asc); await vscodeApp.setListKindAndSort('files', 'descending'); const files_descending = await vscodeApp.getListNames('files'); const desc = asc.slice().reverse(); //console.log('Issue names:', files_descending); expect(files_descending).toEqual(desc); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)tests/e2e/tests/filter-and-sort-violations.test.ts(1 hunks)tests/playwright.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🧬 Code Graph Analysis (1)
tests/e2e/tests/filter-and-sort-violations.test.ts (4)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)tests/e2e/pages/vscode.page.ts (1)
VSCode(16-412)tests/e2e/utilities/utils.ts (1)
generateRandomString(75-79)tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_PROVIDER(26-39)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (7)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
38-38: Reasonable timeout bump to handle slower analysesIncreasing the "Analysis completed" visibility timeout to 400s is sensible and still within the suite-level 600s test timeout. LGTM.
tests/playwright.config.ts (1)
33-36: New Playwright project wiring looks correctProject 'filter-and-sort-violations' is properly added and scoped via testMatch. No dependency needed since the test sets up and runs its own analysis. LGTM.
tests/e2e/pages/vscode.page.ts (3)
14-15: Type aliases for sort and list kinds are clear and helpfulThe union types improve readability and constrain valid inputs. LGTM.
228-262: Category menu handling is robustGood use of aria-expanded, aria-controls, and a bounded retry to open the menu. The fallback to global options and the poll for options’ visibility is pragmatic. LGTM.
267-296: Solid cleanup loop for category selectionsLooping until no selected options remain and re-opening the menu if it collapses avoids flakiness. Nice touch to ensure it ends collapsed. LGTM.
tests/e2e/tests/filter-and-sort-violations.test.ts (2)
111-128: Search flow is well-structured and resilientGood use of expect.poll to wait for UI to settle when narrowing and clearing search. LGTM.
137-161: Category filter assertions are pragmatic and clean up properlyBaseline > 0 check, per-category application with a cap against baseline, and final cleanup make this test robust. LGTM.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
8a28839 to
56eeacf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
211-222: getListNames currently filters out Files; honor the kind parameterThe method ignores the kind parameter and forces a “Get solution” filter typical for Issues, likely returning an empty array for Files and breaking file-based tests.
Apply this diff to use kind and include Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Narrow to issues when applicable; Files typically won't have "Get solution" + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 30_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
🧹 Nitpick comments (6)
tests/e2e/pages/vscode.page.ts (3)
191-204: Guard clicks by current pressed state to avoid accidental togglingIf the button is already selected, clicking it again can flip state on some toggle-group implementations. Guard by aria-pressed before clicking.
Apply this diff:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } await this.window.waitForTimeout(20000); }
243-253: Remove any by narrowing type; optional chain on menu resolutionAvoids any and improves type safety when resolving the controlled menu.
Apply this diff:
- const controlsId = await categoryBtn.getAttribute('aria-controls'); - let menu = controlsId ? view.locator(`#${controlsId}`) : null; + const controlsId = await categoryBtn.getAttribute('aria-controls'); + const menu = controlsId ? view.locator(`#${controlsId}`) : undefined; let options = view.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); - if (menu) { - options = (menu as any).locator( + if (menu) { + options = menu.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); }
304-310: Regex-from-variable warning looks mitigated; consider a light guardStatic analysis flags a constructed RegExp, but all metacharacters are escaped before interpolation, and anchors are used, so catastrophic backtracking risk is minimal. If you want extra hardening, add a simple length cap for string input to avoid pathological inputs.
Example guard (optional):
if (typeof name === 'string' && name.length > 200) { throw new Error('Category name too long'); }tests/e2e/tests/filter-and-sort-violations.test.ts (3)
41-44: Add a companion isDescending helper for stronger descending assertionsRelying on “not ascending” is weaker than asserting descending order directly.
Apply this diff:
const collator = new Intl.Collator(undefined, { sensitivity: 'base', numeric: true }); const isAscending = (arr: string[]) => arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) <= 0); + const isDescending = (arr: string[]) => + arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) >= 0);
49-59: Assert real descending order, not just “not ascending”This makes the descending check meaningful and resilient.
Apply this diff:
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'descending'); const names_descending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_descending); - expect(isAscending(names_descending)).toBe(false); + //console.log('Issue names (desc):', names_descending); + expect(isDescending(names_descending)).toBe(true); await vscodeApp.setListKindAndSort('issues', 'ascending'); const names_ascending = await vscodeApp.getListNames('issues'); //console.log('Issue names:', names_ascending); expect(isAscending(names_ascending)).toBe(true); });
91-104: Ensure the Files sort test is non-trivial by asserting multiple itemsWithout asserting length > 1, the sort checks can vacuously pass on empty or single-item lists.
Apply this diff:
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const files_ascending = await vscodeApp.getListNames('files'); + expect(files_ascending.length).toBeGreaterThan(1); expectFilesListLooksValid(files_ascending); const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); //console.log('Issue names:', files_ascending); expect(files_ascending).toEqual(asc); await vscodeApp.setListKindAndSort('files', 'descending'); const files_descending = await vscodeApp.getListNames('files'); const desc = asc.slice().reverse(); //console.log('Issue names:', files_descending); expect(files_descending).toEqual(desc); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)tests/e2e/tests/filter-and-sort-violations.test.ts(1 hunks)tests/playwright.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/playwright.config.ts
- tests/e2e/tests/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
🧬 Code Graph Analysis (1)
tests/e2e/tests/filter-and-sort-violations.test.ts (4)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)tests/e2e/pages/vscode.page.ts (1)
VSCode(16-412)tests/e2e/utilities/utils.ts (1)
generateRandomString(75-79)tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_PROVIDER(26-39)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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: Lint
🔇 Additional comments (3)
tests/e2e/pages/vscode.page.ts (1)
14-15: Nice: clear, focused type aliasesThe SortOrder and ListKind aliases make the new API self-documenting.
tests/e2e/tests/filter-and-sort-violations.test.ts (2)
111-128: Search flow and assertions look solidThe test meaningfully narrows to a single file and verifies clearing restores multiple results. Good use of expect.poll for stability.
137-161: Category filter baseline guard is appropriateVerifying that each single-category filter never exceeds the unfiltered baseline is a pragmatic invariant. Cleanup via clearCategorySelections is appreciated.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
56eeacf to
8f83e5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
206-222: getListNames ignores the kind and likely excludes Files due to “Get solution” filter.As written, Files will return an empty list because Files cards typically don’t have the “Get solution” button. The parameter is unused as well. This will break the new Files tests.
Apply this diff to honor the kind and include Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Issues often include a “Get solution” button; Files usually don’t. + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 3_000 }); + await expect(titles.first()).toBeVisible({ timeout: 10_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
🧹 Nitpick comments (4)
tests/e2e/pages/vscode.page.ts (2)
186-204: Make toggles idempotent and avoid fixed sleep to reduce flakiness.Clicking even when already pressed is harmless but adds unnecessary re-renders. The fixed 2s sleep can be flaky on slow/fast environments. Prefer checking aria-pressed before clicking and waiting on a UI state instead of a timeout.
Apply this diff to make the method idempotent and avoid the fixed 2s wait:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(2000); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } + // Let the view settle by observing the first title; avoids arbitrary sleeps + const titles = analysisView.getByRole('heading', { level: 3 }); + await expect(titles.first()).toBeVisible({ timeout: 10_000 }); }
298-318: Dynamic RegExp is safely escaped; false-positive ReDoS warning.You’re escaping user input before building the regex and anchoring it, which mitigates ReDoS here. Consider a tiny simplification: when a string is provided and you want exact, case-insensitive match, you could also use getByRole with name: string and exact: true in some contexts, but for filter({ hasText }), regex is appropriate.
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
50-60: Strengthen the issues sort test with a non-triviality check.If only one issue is present (or all titles equal), descending could still appear “ascending.” Assert a minimum of 2 items before checking order.
Apply this diff:
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'descending'); const names_descending = await vscodeApp.getListNames('issues'); + expect(names_descending.length).toBeGreaterThan(1); //console.log('Issue names:', names_descending); expect(isAscending(names_descending)).toBe(false); await vscodeApp.setListKindAndSort('issues', 'ascending'); const names_ascending = await vscodeApp.getListNames('issues'); + expect(names_ascending.length).toBeGreaterThan(1); //console.log('Issue names:', names_ascending); expect(isAscending(names_ascending)).toBe(true); });
92-105: Add a minimal cardinality check to avoid vacuous passes.If the Files list is empty or length 1, ordering assertions are trivial. Assert at least 2.
Apply this diff:
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const files_ascending = await vscodeApp.getListNames('files'); + expect(files_ascending.length).toBeGreaterThan(1); expectFilesListLooksValid(files_ascending); const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); //console.log('Issue names:', files_ascending); expect(files_ascending).toEqual(asc); await vscodeApp.setListKindAndSort('files', 'descending'); const files_descending = await vscodeApp.getListNames('files'); + expect(files_descending.length).toBeGreaterThan(1); const desc = asc.slice().reverse(); //console.log('Issue names:', files_descending); expect(files_descending).toEqual(desc); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🧬 Code Graph Analysis (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
🔇 Additional comments (7)
tests/e2e/pages/vscode.page.ts (2)
14-15: Nice, clear string literal unions for sort and list kinds.These tighten call sites and improve readability in tests.
224-263: Category opener looks solid; robustly locates menu and options.Good use of aria-controls, role-based fallbacks, and expect.poll to avoid racing the menu rendering.
tests/e2e/tests/configure-and-run-analysis.test.ts (5)
37-39: Extending “Analysis completed” wait to 400s is reasonable.Keeps PR checks stable across slower environments; aligns with existing long timeouts elsewhere in the suite.
42-45: Good choice: locale-aware, numeric-aware sort comparator with a tight isAscending helper.This improves robustness for human-friendly strings.
67-86: Helpful validity guard for file-like names.Covers common bare files and extensions; this is a pragmatic check to catch unexpected UI content.
112-129: Search test looks good; relies on getListNames('files') working for Files.Once getListNames is fixed to respect the kind, this test should be stable. Nice use of expect.poll for UI settling.
Would you like me to run a quick verification script after updating getListNames to ensure this test now passes locally?
138-162: Category test is sound; watch for enforced-selection UIs.If the Category menu is a radio-only group without a “none/all” option, clearCategorySelections might loop. After applying the safety cap fix in the page object, this test will fail fast instead of hanging, which is preferable.
Confirm the exact aria roles used by the Category menu (checkbox vs radio). If it enforces one selection, we may need a dedicated “All/Any” option click in clearCategorySelections.
tests/e2e/pages/vscode.page.ts
Outdated
| /** | ||
| * Clears all selected categories in the category filter dropdown in the analysis view. | ||
| */ | ||
| public async clearCategorySelections(): Promise<void> { | ||
| const view = await this.getView(KAIViews.analysisView); | ||
| let { categoryBtn } = await this.openCategory(); | ||
|
|
||
| const selected = () => | ||
| view.locator( | ||
| '[role="menuitemcheckbox"][aria-checked="true"], ' + | ||
| '[role="menuitemradio"][aria-checked="true"], ' + | ||
| '[role="menuitem"][aria-selected="true"], ' + | ||
| '[role="option"][aria-selected="true"]' | ||
| ); | ||
|
|
||
| for (;;) { | ||
| const n = await selected().count(); | ||
| if (n === 0) break; | ||
|
|
||
| await selected().first().click(); | ||
|
|
||
| if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { | ||
| ({ categoryBtn } = await this.openCategory()); | ||
| } else { | ||
| await this.window.waitForTimeout(50); | ||
| } | ||
| } | ||
|
|
||
| if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { | ||
| await categoryBtn.click(); | ||
| await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential infinite loop if the category uses radios (one selection always enforced). Add a safety cap.
The for (;;) loop clicks the first “selected” option until none remain. If the menu is a radio group without a “none” option, this may never reach zero, hanging the test.
Apply this diff to add a safety cap and try a common “clear/all” escape hatch:
public async clearCategorySelections(): Promise<void> {
const view = await this.getView(KAIViews.analysisView);
let { categoryBtn } = await this.openCategory();
const selected = () =>
view.locator(
'[role="menuitemcheckbox"][aria-checked="true"], ' +
'[role="menuitemradio"][aria-checked="true"], ' +
'[role="menuitem"][aria-selected="true"], ' +
'[role="option"][aria-selected="true"]'
);
- for (;;) {
+ let iterations = 0;
+ for (;;) {
const n = await selected().count();
if (n === 0) break;
await selected().first().click();
if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') {
({ categoryBtn } = await this.openCategory());
} else {
await this.window.waitForTimeout(50);
}
+
+ // Safety cap to avoid infinite loops on radio-only menus
+ if (++iterations > 50) {
+ // Try clicking a common “clear” or “all” option if present
+ const clearOpt = view.getByRole('menuitem', { name: /clear all|reset|all|any/i }).first();
+ if ((await clearOpt.count()) > 0) {
+ await clearOpt.click();
+ ({ categoryBtn } = await this.openCategory());
+ if ((await selected().count()) === 0) break;
+ }
+ throw new Error('clearCategorySelections: exceeded safety iterations; menu may enforce a selection.');
+ }
}
if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') {
await categoryBtn.click();
await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 });
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Clears all selected categories in the category filter dropdown in the analysis view. | |
| */ | |
| public async clearCategorySelections(): Promise<void> { | |
| const view = await this.getView(KAIViews.analysisView); | |
| let { categoryBtn } = await this.openCategory(); | |
| const selected = () => | |
| view.locator( | |
| '[role="menuitemcheckbox"][aria-checked="true"], ' + | |
| '[role="menuitemradio"][aria-checked="true"], ' + | |
| '[role="menuitem"][aria-selected="true"], ' + | |
| '[role="option"][aria-selected="true"]' | |
| ); | |
| for (;;) { | |
| const n = await selected().count(); | |
| if (n === 0) break; | |
| await selected().first().click(); | |
| if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { | |
| ({ categoryBtn } = await this.openCategory()); | |
| } else { | |
| await this.window.waitForTimeout(50); | |
| } | |
| } | |
| if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { | |
| await categoryBtn.click(); | |
| await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); | |
| } | |
| } | |
| /** | |
| * Clears all selected categories in the category filter dropdown in the analysis view. | |
| */ | |
| public async clearCategorySelections(): Promise<void> { | |
| const view = await this.getView(KAIViews.analysisView); | |
| let { categoryBtn } = await this.openCategory(); | |
| const selected = () => | |
| view.locator( | |
| '[role="menuitemcheckbox"][aria-checked="true"], ' + | |
| '[role="menuitemradio"][aria-checked="true"], ' + | |
| '[role="menuitem"][aria-selected="true"], ' + | |
| '[role="option"][aria-selected="true"]' | |
| ); | |
| let iterations = 0; | |
| for (;;) { | |
| const n = await selected().count(); | |
| if (n === 0) break; | |
| await selected().first().click(); | |
| if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { | |
| ({ categoryBtn } = await this.openCategory()); | |
| } else { | |
| await this.window.waitForTimeout(50); | |
| } | |
| // Safety cap to avoid infinite loops on radio-only menus | |
| if (++iterations > 50) { | |
| // Try clicking a common “clear” or “all” option if present | |
| const clearOpt = view.getByRole('menuitem', { name: /clear all|reset|all|any/i }).first(); | |
| if ((await clearOpt.count()) > 0) { | |
| await clearOpt.click(); | |
| ({ categoryBtn } = await this.openCategory()); | |
| if ((await selected().count()) === 0) break; | |
| } | |
| throw new Error( | |
| 'clearCategorySelections: exceeded safety iterations; menu may enforce a selection.' | |
| ); | |
| } | |
| } | |
| if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { | |
| await categoryBtn.click(); | |
| await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/pages/vscode.page.ts around lines 264 to 296, the for(;;) loop that
repeatedly clicks selected category items can hang when the control is a radio
group (one selection is always enforced); add a safety cap and a fallback “clear
all” attempt: introduce a maxAttempts counter (e.g., 50) and increment it each
iteration, and before bailing out try to locate and click a common escape item
such as a "Clear", "Clear all", "All", or "None" option (by text/content or a
known selector); if that fails and attempts exceed the cap, close the category
menu (click categoryBtn) and break the loop to avoid infinite hang, then proceed
with the existing post-loop check that closes the dropdown if still open.
8f83e5c to
c46625c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/pages/vscode.page.ts (2)
211-222: getListNames ignores the kind and likely excludes Files due to “Get solution” filterAs written, Files will be filtered out because they typically don’t have a “Get solution” button; also the parameter is unused. This will cause Files-related tests to return empty results.
Apply this diff to honor the kind and include Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Narrow to issues when applicable; Files typically won't have "Get solution" + if (kind === 'issues') { + listCards = listCards.filter({ has: view.getByRole('button', { name: /get solution/i }) }); + } const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 5_000 }); + await expect(titles.first()).toBeVisible({ timeout: 5_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
267-296: Potential infinite loop when the category uses radio-only selections — add a safety capIf the menu enforces one selection (e.g., radios without a “none”), the
for(;;)loop may never reach zero. Add a bounded attempt counter and try a “clear/all” escape hatch.Apply this diff:
public async clearCategorySelections(): Promise<void> { const view = await this.getView(KAIViews.analysisView); let { categoryBtn } = await this.openCategory(); @@ - for (;;) { + let iterations = 0; + for (;;) { const n = await selected().count(); if (n === 0) break; await selected().first().click(); if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { ({ categoryBtn } = await this.openCategory()); } else { await this.window.waitForTimeout(50); } + // Safety cap to avoid infinite loops on radio-only menus + if (++iterations > 50) { + const clearOpt = view.getByRole('menuitem', { name: /clear all|reset|all|any|none/i }).first(); + if ((await clearOpt.count()) > 0) { + await clearOpt.click(); + ({ categoryBtn } = await this.openCategory()); + if ((await selected().count()) === 0) break; + } + throw new Error('clearCategorySelections: exceeded safety iterations; menu may enforce a selection.'); + } } if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); } }
🧹 Nitpick comments (3)
tests/e2e/pages/vscode.page.ts (3)
191-204: Make kind/sort toggles idempotent to reduce flakinessAvoid clicking if the target toggle is already active. This helps when the default selection matches the requested state and prevents accidental toggling if the control de-presses on re-click.
Apply this diff:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + } await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + } await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); await this.window.waitForTimeout(2000); }
243-253: Avoidanycast; strongly type the menu scopeMinor nit: the
(menu as any)cast can be avoided by typingmenuand using a scoped locator variable.Apply this diff:
- const controlsId = await categoryBtn.getAttribute('aria-controls'); - let menu = controlsId ? view.locator(`#${controlsId}`) : null; + const controlsId = await categoryBtn.getAttribute('aria-controls'); + const menu: import('playwright').Locator | null = controlsId ? view.locator(`#${controlsId}`) : null; @@ - if (menu) { - options = (menu as any).locator( - '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' - ); - } + if (menu) { + const scope = menu; + options = scope.locator( + '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' + ); + }
302-318: Regex is escaped and anchored; low ReDoS risk despite static warningStatic analysis flags constructing a RegExp from input. Here the input is fully escaped and anchored, which mitigates catastrophic backtracking. Usage is bounded (single lookup with a 3s timeout).
No change required. If you want to silence linters, add a short comment explaining the escape and anchoring rationale.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
tests/e2e/pages/vscode.page.ts (2)
14-15: LGTM: clear, focused union literal types for sort order and list kindThe new type aliases are concise and self-documenting. Good addition.
228-262: Robust category open with retry and aria-controls scopingThe expand retries, scoping via aria-controls, and polling for visible options are solid and should behave well across variants of the dropdown.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
c46625c to
42d02d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/pages/vscode.page.ts (2)
206-223: getListNames ignores kind and likely excludes Files; also fails when there are 0 results
- Current filter only returns cards containing a “Get solution” button (typical for Issues, not Files).
- The underscore parameter is unused.
- Expecting a title when 0 results exist will throw.
Apply this diff to honor kind and handle empty lists:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Narrow to issues when applicable; Files typically won't have "Get solution" + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 6_000 }); + const count = await titles.count(); + if (count === 0) return []; + await expect(titles.first()).toBeVisible({ timeout: 30_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
264-296: Potential infinite loop when category uses radios; add safety cap and “clear/all” fallback
The for (;;) loop can hang if the menu enforces one selected option.Apply this diff to cap iterations and try a common “clear/all” escape:
public async clearCategorySelections(): Promise<void> { const view = await this.getView(KAIViews.analysisView); let { categoryBtn } = await this.openCategory(); @@ - for (;;) { + let iterations = 0; + for (;;) { const n = await selected().count(); if (n === 0) break; @@ if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { ({ categoryBtn } = await this.openCategory()); } else { await this.window.waitForTimeout(50); } + if (++iterations > 50) { + // Try clicking a common “clear/all/any/reset” item if present + const clearOpt = view + .getByRole('menuitem', { name: /clear all|clear|reset|all|any|none/i }) + .first(); + if ((await clearOpt.count()) > 0) { + await clearOpt.click(); + ({ categoryBtn } = await this.openCategory()); + if ((await selected().count()) === 0) break; + } + throw new Error( + 'clearCategorySelections: exceeded safety iterations; menu may enforce a selection.' + ); + } } if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 5000 }); } }
🧹 Nitpick comments (7)
tests/e2e/pages/vscode.page.ts (2)
186-205: Make selectors resilient and avoid fixed sleeps in setListKindAndSort
- The fixed 2s sleep can slow tests and still flake under load.
- The “Issues/Files” control can display counts (e.g., “Issues 12”); prefer regex like in Category.
- Same for sort buttons; match by regex to be robust.
Apply this diff to harden locators and remove the static wait:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); - const kindButton = analysisView.getByRole('button', { - name: kind === 'issues' ? 'Issues' : 'Files', - }); + const kindRe = + kind === 'issues' ? /^Issues(?:\s*\d+)?$/i : /^Files(?:\s*\d+)?$/i; + const kindButton = analysisView.getByRole('button', { name: kindRe }); await kindButton.click(); await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); - const sortButton = analysisView.getByRole('button', { - name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', - }); + const sortRe = + order === 'ascending' ? /^Sort ascending/i : /^Sort descending/i; + const sortButton = analysisView.getByRole('button', { name: sortRe }); await sortButton.click(); await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(2000); }Note: getListNames() already waits for list items; removing the sleep should keep tests stable while faster.
224-263: Minor: avoid any-cast and add strong typing for menu/options
Typing the menu as Locator eliminates the need for any-cast and improves readability.Apply this diff locally within this block:
- const controlsId = await categoryBtn.getAttribute('aria-controls'); - let menu = controlsId ? view.locator(`#${controlsId}`) : null; + const controlsId = await categoryBtn.getAttribute('aria-controls'); + let menu: Locator | null = controlsId ? view.locator(`#${controlsId}`) : null; @@ - if (menu) { - options = (menu as any).locator( + if (menu) { + options = menu.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); }Add the following import at the top of the file:
import type { Locator } from '@playwright/test';tests/e2e/tests/configure-and-run-analysis.test.ts (5)
42-45: Natural, locale-aware comparator is appropriate — consider hoisting to a shared test util
If other suites need ordering checks, centralizing Collator/isAscending avoids duplication.
46-61: Strengthen descending-order assertion for Issues by comparing to reversed ascending
Currently, “not ascending” is a weak check. Assert that descending equals ascending reversed, mirroring the Files test.Apply this diff to make the test stricter and symmetric:
- test('Set list kind and sort (Issues ascending and descending)', async () => { - await vscodeApp.setListKindAndSort('issues', 'descending'); - const names_descending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_descending); - expect(isAscending(names_descending)).toBe(false); - - await vscodeApp.setListKindAndSort('issues', 'ascending'); - const names_ascending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_ascending); - expect(isAscending(names_ascending)).toBe(true); - }); + test('Set list kind and sort (Issues ascending and descending)', async () => { + await vscodeApp.setListKindAndSort('issues', 'ascending'); + const names_ascending = await vscodeApp.getListNames('issues'); + expect(isAscending(names_ascending)).toBe(true); + + await vscodeApp.setListKindAndSort('issues', 'descending'); + const names_descending = await vscodeApp.getListNames('issues'); + expect(names_descending).toEqual([...names_ascending].reverse()); + });
62-86: File-name shape validator is sensible — optionally move to a shared helper
Looks good. If reused, consider placing in tests/e2e/utilities to keep tests concise.
107-129: Reduce potential flakiness by choosing a more unique search term
Using the first element can collide if the search matches substrings. Picking the longest file name reduces collisions.Apply this diff:
- const filename = all[0]; + // Choose a more unique term to minimize substring collisions + const filename = all.reduce((a, b) => (a.length >= b.length ? a : b));
131-162: Category filter test is solid — ensure accessible names match exactly
The test relies on options named “Potential”, “Optional”, “Mandatory”. If the menu uses different accessible names (e.g., includes counts), setCategoryByName’s anchored match may not find them. Confirm the exact labels, or update to partial/regex matches if needed.Would you like me to relax setCategoryByName to accept leading labels with optional counts, similar to the Category button (e.g., /^Potential(?:\s*\d+)?$/i)?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🧬 Code Graph Analysis (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (4)
tests/e2e/pages/vscode.page.ts (2)
14-15: Type aliases read well and constrain inputs appropriately — LGTM
298-318: Regex construction is safely escaped — LGTM
The variable-based regex is properly escaped and anchored; ReDoS risk is mitigated in this context.tests/e2e/tests/configure-and-run-analysis.test.ts (2)
38-38: Extending “Analysis completed” wait to 400s — LGTM
Aligns with known cross-platform slowness; consistent with prior test timeouts.
88-106: Files sorting test verifies exact order — LGTM
Good use of locale/numeric-aware Collator and equality against expected sequences.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
42d02d5 to
79df97b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/pages/vscode.page.ts (2)
211-222: getListNames ignores the ListKind parameter and may exclude Files.The method currently filters cards to only those containing a "Get solution" button, which is typical for Issues but not Files. The
_parameter indicates it's unused. This will likely cause the Files tests to fail.Apply this diff to honor the kind parameter and support both Issues and Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); let listCards = view .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + .filter({ has: view.getByRole('heading', { level: 3 }) }); + + // Only filter for "Get solution" button when looking for issues + if (kind === 'issues') { + listCards = listCards.filter({ has: view.getByRole('button', { name: /get solution/i }) }); + } + const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 6_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
267-296: Potential infinite loop in clearCategorySelections for radio button menus.The
for(;;)loop repeatedly clicks selected items until none remain. If the menu uses radio buttons without a "none" option, one selection is always enforced, causing an infinite loop.Apply this diff to add a safety cap and handle radio-only menus:
public async clearCategorySelections(): Promise<void> { const view = await this.getView(KAIViews.analysisView); let { categoryBtn } = await this.openCategory(); const selected = () => view.locator( '[role="menuitemcheckbox"][aria-checked="true"], ' + '[role="menuitemradio"][aria-checked="true"], ' + '[role="menuitem"][aria-selected="true"], ' + '[role="option"][aria-selected="true"]' ); - for (;;) { + let iterations = 0; + const maxIterations = 50; + for (;;) { const n = await selected().count(); if (n === 0) break; await selected().first().click(); if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') { ({ categoryBtn } = await this.openCategory()); } else { await this.window.waitForTimeout(50); } + + // Safety cap to avoid infinite loops on radio-only menus + if (++iterations > maxIterations) { + // Try clicking a "clear all" or similar option if present + const clearOption = view.getByRole('menuitem', { name: /clear all|reset|all|any/i }).first(); + if ((await clearOption.count()) > 0) { + await clearOption.click(); + ({ categoryBtn } = await this.openCategory()); + if ((await selected().count()) === 0) break; + } + // If we still can't clear, log warning and break + console.warn('clearCategorySelections: exceeded max iterations, menu may enforce a selection'); + break; + } } if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 5000 }); } }
🧹 Nitpick comments (2)
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
53-54: Remove or convert commented-out console.log statements.These debug statements should either be removed or converted to proper test logging if they provide value for debugging failed tests.
Apply this diff to clean up the commented code:
- //console.log('Issue names:', names_descending); + // Uncomment for debugging: console.log('Issue names:', names_descending);Or simply remove them entirely if not needed.
Also applies to: 58-59, 97-98, 103-104
117-122: Potential race condition in file search test.The test searches for
all[0]without verifying that theallarray is not empty. While line 115 checksall.length > 0, this doesn't guaranteeall[0]exists if the array changes between checks.Consider adding an explicit check or store the filename earlier:
await vscodeApp.setListKindAndSort('files', 'ascending'); const all = await vscodeApp.getListNames('files'); expect(all.length).toBeGreaterThan(0); + expect(all[0]).toBeDefined(); const filename = all[0];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🧬 Code Graph Analysis (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 304-304: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (8)
tests/e2e/tests/configure-and-run-analysis.test.ts (5)
37-39: Good adaptation based on learnings!The timeout increase from 300000ms to 400000ms aligns with the known cross-platform performance considerations for Windows environments.
42-44: LGTM! Appropriate use of Intl.Collator for natural ordering.Using
Intl.Collatorwithnumeric: truecorrectly handles alphanumeric sorting where numbers are ordered naturally (e.g., "file2" before "file10"). TheisAscendinghelper function properly checks if an array is sorted in ascending order.
67-86: Well-structured file name validation logic.The
expectFilesListLooksValidfunction provides comprehensive validation by:
- Checking for common bare file names (README, LICENSE, etc.)
- Validating file extensions with a reasonable pattern
- Providing clear error messages with the actual file name when validation fails
147-147: Good use of polling and logging for category filtering test.The test properly uses
expect.poll()with a 12-second timeout to handle async UI updates, and includes helpful console logging for debugging category counts. This aligns well with the flaky nature of UI automation.Also applies to: 157-157
138-162: Comprehensive category filter test with proper cleanup.The test thoroughly validates that category filters work correctly by:
- Establishing a baseline count with no filters
- Testing each category individually with proper cleanup between iterations
- Ensuring filtered counts never exceed the baseline
- Cleaning up selections at the end
tests/e2e/pages/vscode.page.ts (3)
303-305: RegExp escaping is correctly implemented.The
toRegexfunction properly escapes special regex characters usingreplace(/[.*+?^${}()|[\]\\]/g, '\\$&')before constructing the RegExp. This prevents ReDoS attacks from malicious input patterns. The static analysis warning can be safely ignored in this case.
191-204: Well-structured method for setting list kind and sort order.The
setListKindAndSortmethod properly:
- Clicks the appropriate kind button (Issues/Files)
- Verifies the button state with
aria-pressed="true"- Sets the sort order and verifies its state
- Includes a reasonable wait for UI updates
228-262: Robust category dropdown handling with retry logic.The
openCategorymethod implements good practices:
- Scrolls the button into view before interaction
- Includes retry logic (up to 3 attempts) for opening the dropdown
- Properly locates menu options using various ARIA roles
- Uses polling to ensure options are visible before returning
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
50-60: Wait for sort to settle via expect.poll; also drop commented logsUsing a short poll after changing sort reduces flakiness if the list updates asynchronously. The commented console logs can be removed per prior feedback.
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'descending'); - const names_descending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_descending); - expect(isAscending(names_descending)).toBe(false); + await expect + .poll(async () => isAscending(await vscodeApp.getListNames('issues')), { + timeout: 10_000, + }) + .toBe(false); await vscodeApp.setListKindAndSort('issues', 'ascending'); - const names_ascending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_ascending); - expect(isAscending(names_ascending)).toBe(true); + await expect + .poll(async () => isAscending(await vscodeApp.getListNames('issues')), { + timeout: 10_000, + }) + .toBe(true); });To verify page-object API coverage used here exists and is stable, run:
#!/bin/bash # Verify VSCode page methods and types used by tests exist set -euo pipefail echo "Looking for vscode.page.ts..." fd -a vscode.page.ts echo "Checking for VSCode class..." rg -n -C3 -P --type=ts '^\s*export\s+class\s+VSCode\b' echo "Checking for methods used by tests..." rg -n -C2 --type=ts 'setListKindAndSort\s*\(' rg -n -C2 --type=ts 'getListNames\s*\(' rg -n -C2 --type=ts 'searchViolation\s*\(' rg -n -C2 --type=ts 'clearCategorySelections\s*\(' rg -n -C2 --type=ts 'setCategoryByName\s*\(' echo "Checking for types ListKind and SortOrder..." rg -n -C2 --type=ts '\btype\s+ListKind\b|\bexport\s+type\s+ListKind\b' rg -n -C2 --type=ts '\btype\s+SortOrder\b|\bexport\s+type\s+SortOrder\b'
🧹 Nitpick comments (5)
tests/e2e/tests/configure-and-run-analysis.test.ts (5)
37-39: Consider aligning the “Analysis completed” wait to 600s to reduce CI flakinessGiven prior cross-platform timing learnings, 400s may still be borderline in slower environments. Bumping to 600s will better align with the existing 600s guardrails around analysis actions.
- await expect(vscodeApp.getWindow().getByText('Analysis completed').first()).toBeVisible({ - timeout: 400000, - }); + await expect(vscodeApp.getWindow().getByText('Analysis completed').first()).toBeVisible({ + timeout: 600000, + });
67-86: File-name validator may be too strict; broaden known bare names to avoid false negativesSome common bare files (e.g., NOTICE, CHANGELOG, CODEOWNERS, Jenkinsfile) are not recognized and could cause unnecessary failures across repos. Consider expanding the allow-list.
const knownBare = new Set([ 'README', 'LICENSE', 'Makefile', 'Dockerfile', 'BUILD', 'WORKSPACE', + 'NOTICE', + 'CHANGELOG', + 'CHANGES', + 'CODEOWNERS', + 'CONTRIBUTING', + 'COPYING', + 'AUTHORS', + 'SECURITY', + 'VERSION', + 'Gemfile', + 'Rakefile', + 'Jenkinsfile', + 'CMakeLists.txt', + 'Vagrantfile', + 'Procfile', ]);If you prefer to avoid maintaining a list, we can relax the heuristic (e.g., accept dotfiles and any basename with at least one alphanumeric) — happy to draft that alternative.
92-105: Assert non-trivial dataset and drop commented logs for clarityAdding a quick length check ensures the descending comparison is meaningful. Remove commented logs.
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const files_ascending = await vscodeApp.getListNames('files'); expectFilesListLooksValid(files_ascending); + expect(files_ascending.length).toBeGreaterThan(1); const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); - //console.log('Issue names:', files_ascending); expect(files_ascending).toEqual(asc); await vscodeApp.setListKindAndSort('files', 'descending'); const files_descending = await vscodeApp.getListNames('files'); const desc = asc.slice().reverse(); - //console.log('Issue names:', files_descending); expect(files_descending).toEqual(desc); });
112-129: Search test: ensure baseline has >1 and add explicit poll timeouts
- Using >1 baseline makes the “clearing expands again” assertion meaningful.
- Adding explicit timeouts on poll improves determinism on slow runners.
test('Files: search narrows to one; clearing expands again', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const all = await vscodeApp.getListNames('files'); - expect(all.length).toBeGreaterThan(0); + expect(all.length).toBeGreaterThan(1); const filename = all[0]; await vscodeApp.searchViolation(filename); - await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBe(1); + await expect + .poll(async () => (await vscodeApp.getListNames('files')).length, { timeout: 20_000 }) + .toBe(1); const names1 = await vscodeApp.getListNames('files'); expect(names1).toEqual([filename]); await vscodeApp.searchViolation(''); - await expect - .poll(async () => (await vscodeApp.getListNames('files')).length) - .toBeGreaterThan(1); + await expect + .poll(async () => (await vscodeApp.getListNames('files')).length, { timeout: 20_000 }) + .toBeGreaterThan(1); const names2 = await vscodeApp.getListNames('files'); expect(names2.length).toBeGreaterThan(1); });
131-162: Hard-coded category names may drift; prefer discovering categories from UI and skipping unavailable onesIf labels change (“Mandatory/Optional/Potential”), this can break otherwise healthy tests. Consider reading category names from the UI once and iterating only those present. We can add a lightweight helper on the page-object to fetch category labels and refactor this test to skip unavailable ones with a note.
Example page-object addition (outside this file):
// tests/e2e/pages/vscode.page.ts async getCategoryNames(): Promise<string[]> { const menu = this.getWindow().getByRole('menu', { name: /category/i }); await menu.click(); const items = this.getWindow().getByRole('menuitemcheckbox'); const count = await items.count(); const names: string[] = []; for (let i = 0; i < count; i++) { names.push(await items.nth(i).innerText()); } // close menu await this.getWindow().keyboard.press('Escape'); return names.map(s => s.trim()).filter(Boolean); }Refactor idea in this test (pseudocode):
- available = await vscodeApp.getCategoryNames()
- for each name in CATEGORY_NAMES: if not available.has(name) -> continue/skip with log
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🧬 Code Graph Analysis (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
42-45: Natural, numeric-aware ordering helper looks goodUsing Intl.Collator with numeric: true and sensitivity: 'base' is a solid way to assert UI sort consistently across locales.
djzager
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/pages/vscode.page.ts (1)
211-222: getListNames ignores the kind and likely excludes Files due to “Get solution” filterThis currently filters to cards containing a "Get solution" button (typical for Issues) and ignores the passed kind, so Files will often return an empty set and break the new tests.
Apply this diff to honor the kind and gracefully handle empty lists:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); - const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 6_000 }); - - const texts = await titles.allInnerTexts(); - return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } + const titles = listCards.getByRole('heading', { level: 3 }); + if ((await titles.count()) === 0) return []; + const texts = await titles.allInnerTexts(); + return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }tests/e2e/tests/configure-and-run-analysis.test.ts (1)
50-60: Strengthen assertions and use consistent camelCase naming
- Add a length > 1 check so the sort assertions are meaningful.
- Use camelCase for variable names to stay consistent with project conventions (prior feedback).
Apply this diff:
- test('Set list kind and sort (Issues ascending and descending)', async () => { - await vscodeApp.setListKindAndSort('issues', 'descending'); - const names_descending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_descending); - expect(isAscending(names_descending)).toBe(false); - - await vscodeApp.setListKindAndSort('issues', 'ascending'); - const names_ascending = await vscodeApp.getListNames('issues'); - //console.log('Issue names:', names_ascending); - expect(isAscending(names_ascending)).toBe(true); - }); + test('Set list kind and sort (Issues ascending and descending)', async () => { + await vscodeApp.setListKindAndSort('issues', 'descending'); + const issueNamesDescending = await vscodeApp.getListNames('issues'); + expect(issueNamesDescending.length).toBeGreaterThan(1); + expect(isAscending(issueNamesDescending)).toBe(false); + + await vscodeApp.setListKindAndSort('issues', 'ascending'); + const issueNamesAscending = await vscodeApp.getListNames('issues'); + expect(issueNamesAscending.length).toBeGreaterThan(1); + expect(isAscending(issueNamesAscending)).toBe(true); + });
🧹 Nitpick comments (6)
tests/e2e/pages/vscode.page.ts (3)
191-204: Avoid unnecessary toggles and replace fixed sleeps with state-based waitsClicking the already-selected kind/sort button can flip state unintentionally in some UIs. Also, the fixed 2s sleep can be flaky/slow. Guard clicks based on aria-pressed and wait for a meaningful change instead of sleeping.
Apply this diff within this method:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } - const sortButton = analysisView.getByRole('button', { + const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(2000); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } + // Optional: if you want to ensure the list updated, poll for any title text change + // const before = (await this.getListNames(kind)).join('\n'); + // await expect + // .poll(async () => (await this.getListNames(kind)).join('\n')) + // .not.toBe(before); }
228-262: Simplify dropdown option wait and drop unnecessary any-cast
- The any-cast on menu is avoidable.
- Using expect.poll to observe options is overkill; Playwright’s built-in waiting on the first option becoming visible is simpler and more robust.
Apply this diff:
- const controlsId = await categoryBtn.getAttribute('aria-controls'); - let menu = controlsId ? view.locator(`#${controlsId}`) : null; - - let options = view.locator( + const controlsId = await categoryBtn.getAttribute('aria-controls'); + const menu = controlsId ? view.locator(`#${controlsId}`) : null; + + let options = view.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); if (menu) { - options = (menu as any).locator( + options = menu.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); } - - await expect - .poll(async () => (await options.count()) > 0 && (await options.first().isVisible()), { - timeout: 6_000, - }) - .toBeTruthy(); + await options.first().waitFor({ state: 'visible', timeout: 6000 });
268-284: Minor: tighten option matching and assert dropdown closure
- Static analysis flagged regex-from-variable; you already escape input, which is good. We can avoid regex for string names altogether by using exact matching where possible.
- After clicking, assert that the dropdown actually closed.
Apply this diff:
- public async setCategoryByName(name: string | RegExp): Promise<void> { + public async setCategoryByName(name: string | RegExp): Promise<void> { const { categoryBtn, options } = await this.openCategory(); - const toRegex = (s: string) => - new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`, 'i'); + const toRegex = (s: string) => + new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\$&')}\\s*$`, 'i'); // keep safety escaping - const opt = - typeof name === 'string' - ? options.filter({ hasText: toRegex(name) }).first() - : options.filter({ hasText: name as RegExp }).first(); + const opt = + typeof name === 'string' + ? options.filter({ hasText: toRegex(name) }).first() // exact-ish match, case-insensitive + : options.filter({ hasText: name as RegExp }).first(); await expect(opt).toBeVisible({ timeout: 5000 }); await opt.click(); await this.window.waitForTimeout(50); if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); + await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); } }tests/e2e/tests/configure-and-run-analysis.test.ts (3)
66-78: Files sort check is fine; consider using isAscending and assert list size > 1Current equality check against a freshly sorted copy is correct. You can simplify by reusing isAscending and guard for > 1 items. Note: this test will fail until getListNames('files') starts returning file cards (see page fix).
Apply this diff:
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); - const files_ascending = await vscodeApp.getListNames('files'); - const asc = [...files_ascending].sort((a, b) => collator.compare(a, b)); - //console.log('Issue names:', files_ascending); - expect(files_ascending).toEqual(asc); + const filesAscending = await vscodeApp.getListNames('files'); + expect(filesAscending.length).toBeGreaterThan(1); + expect(isAscending(filesAscending)).toBe(true); await vscodeApp.setListKindAndSort('files', 'descending'); - const files_descending = await vscodeApp.getListNames('files'); - const desc = asc.slice().reverse(); - //console.log('Issue names:', files_descending); - expect(files_descending).toEqual(desc); + const filesDescending = await vscodeApp.getListNames('files'); + expect(isAscending(filesDescending)).toBe(false); });After applying the page fix for getListNames, please re-run locally to verify both tests pass.
85-102: Reduce flakiness by searching for a unique filenameSearching the first filename risks matching multiple results if the query is a substring of other names. Pick a filename that appears only once in the list.
Apply this diff:
test('Files: search narrows to one; clearing expands again', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const all = await vscodeApp.getListNames('files'); expect(all.length).toBeGreaterThan(0); - const filename = all[0]; + const counts = all.reduce<Record<string, number>>((m, n) => ((m[n] = (m[n] || 0) + 1), m), {}); + const filename = all.find((n) => counts[n] === 1) ?? all[0]; await vscodeApp.searchViolation(filename); await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBe(1); const names1 = await vscodeApp.getListNames('files'); expect(names1).toEqual([filename]); await vscodeApp.searchViolation(''); await expect .poll(async () => (await vscodeApp.getListNames('files')).length) .toBeGreaterThan(1); const names2 = await vscodeApp.getListNames('files'); expect(names2.length).toBeGreaterThan(1); });If the search is substring-based, uniqueness by equality may still collide; if flakes persist, consider enhancing the page helper to support an "exact match" search mode when the UI allows it.
104-126: Don’t hardcode category names; derive available categories from the UIRelying on ['Potential', 'Optional', 'Mandatory'] may break with profile/config changes or localization. Prefer discovering categories at runtime and iterating the ones present.
Illustrative change:
- const CATEGORY_NAMES = ['Potential', 'Optional', 'Mandatory'] as const; + // Fallback values; will be intersected with categories discovered at runtime + const CATEGORY_NAMES = ['Potential', 'Optional', 'Mandatory'] as const; ... test('Category never exceeds number of incidents for Issues', async () => { await vscodeApp.setListKindAndSort('issues', 'ascending'); await vscodeApp.searchViolation(''); const baseline = (await vscodeApp.getListNames('issues')).length; expect(baseline).toBeGreaterThan(0); console.log(`Total number of issues ${baseline}`); - for (const name of CATEGORY_NAMES) { + // Discover available categories and intersect with our expected ones + const { options } = await vscodeApp.openCategory(); + const available = (await options.allInnerTexts()).map((t) => t.trim()); + const cats = CATEGORY_NAMES.filter((c) => + available.some((a) => a.localeCompare(c, undefined, { sensitivity: 'accent' }) === 0) + ); + for (const name of cats) { await vscodeApp.setCategoryByName(name); await expect .poll(async () => (await vscodeApp.getListNames('issues')).length, { timeout: 20_000 }) .toBeLessThanOrEqual(baseline); console.log(`Category ${name} ${(await vscodeApp.getListNames('issues')).length}`); await vscodeApp.setCategoryByName(name); } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 270-270: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (3)
tests/e2e/pages/vscode.page.ts (1)
14-15: Type aliases read well and communicate intent clearlyUsing narrow string literal unions for SortOrder and ListKind improves type safety in page helpers. No concerns.
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
38-38: Increased "Analysis completed" timeout aligns with cross-platform stability learningsBumping from 300s to 400s is reasonable given prior learnings about slower environments. This should reduce flakes without masking real failures.
42-45: Good use of Intl.Collator and an isAscending helper for natural orderingThese helpers make the sort assertions more locale-friendly and numeric-aware.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
2609f73 to
b56a9c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/pages/vscode.page.ts (1)
157-170: Bug: filter toggle locator always targets “Show Filters”; fails after it becomes “Hide Filters”After the first click, the aria-label likely switches to “Hide Filters”, so the second click on the stale “Show Filters” locator can time out. Make the locator resilient to both states, or address them separately.
Apply this diff:
public async searchViolation(term: string): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); - - const toggleFilterButton = analysisView.locator('button[aria-label="Show Filters"]'); - const searchInput = analysisView.locator('input[aria-label="Search violations and incidents"]'); + const toggleBtn = analysisView.getByRole('button', { name: /Show Filters|Hide Filters/i }); + const searchInput = analysisView.locator('input[aria-label="Search violations and incidents"]'); if (await searchInput.isVisible()) { await searchInput.fill(term); return; } - await toggleFilterButton.click(); - await searchInput.fill(term); - await toggleFilterButton.click(); + await toggleBtn.click(); + await expect(searchInput).toBeVisible({ timeout: 2000 }); + await searchInput.fill(term); + // Close the panel if still open + if (await searchInput.isVisible()) { + await toggleBtn.click(); + } }
♻️ Duplicate comments (2)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
98-98: Prefer camelCase for constant names to match repo stylePast feedback suggested camelCase across the codebase. Consider using categoryNames for consistency.
Apply this diff (and update references below):
- const CATEGORY_NAMES = ['Potential', 'Optional', 'Mandatory'] as const; + const categoryNames = ['Potential', 'Optional', 'Mandatory'] as const;tests/e2e/pages/vscode.page.ts (1)
211-222: getListNames ignores the kind and likely excludes Files; use the parameter to include both kindsFiltering by the presence of a “Get solution” button is appropriate for Issues but hides Files entirely, breaking file-list tests. Also, the underscore parameter is unused.
Apply this diff:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 6_000 }); + await expect(titles.first()).toBeVisible({ timeout: 30_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
🧹 Nitpick comments (2)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
83-96: Search test looks good; minor robustness noteThe flow is solid. If you see occasional flakes, consider asserting uniqueness of the chosen filename in the pre-snapshot to avoid the edge case where multiple entries share the same title.
tests/e2e/pages/vscode.page.ts (1)
186-205: Wait for content change instead of fixed 2s sleep after sortThe 2s wait works but is brittle under load. Consider waiting for the first two items to reorder or for a known “loading” indicator to disappear to reduce flakiness.
Example approach (conceptual):
- Capture the first item’s text.
- Click the other sort button.
- Wait until the first item’s text changes or assert the order directly with getListNames(kind).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 251-251: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (5)
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
38-39: Timeout bump for 'Analysis completed' visibility is reasonable400s aligns with cross-platform variability noted in prior learnings; keeping this generous avoids flakiness on slower CI runners.
42-45: Good call using Intl.Collator with numeric for natural orderingThe helper yields locale-aware, numeric-aware comparisons; concise and reliable for these checks.
tests/e2e/pages/vscode.page.ts (3)
14-15: Type aliases for SortOrder/ListKind are clear and scoped appropriatelySimple and effective; keeps the public surface explicit.
228-243: Category dropdown handling is soundSelector is robust to “Category N” labeling, and you assert expanded state before interacting with options.
249-265: Regex from string input is safely escapedYou escape all meta characters before constructing the RegExp, which mitigates ReDoS risks flagged by static tools. Allowing a RegExp overload is useful for tests.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
b56a9c5 to
8fdd37a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
206-223: getListNames excludes Files and ignores the kind parameter; will break Files tests.The method filters to cards having a “Get solution” button (typical for Issues), so Files vanish. It also ignores the ListKind parameter by naming it
_.Apply this diff to honor the kind and include Files:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + if (kind === 'issues') { + // Issues typically expose a "Get solution" button; Files don’t. + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 6_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }Run this to find all call sites and ensure both 'issues' and 'files' are exercised:
#!/bin/bash rg -nC2 '\bgetListNames\s*\(' tests | sed -n '1,120p'
🧹 Nitpick comments (3)
tests/e2e/pages/vscode.page.ts (3)
186-205: Avoid unnecessary toggles and replace fixed 2s sleep with lighter, event-driven waits.Clicking buttons that are already active can flip states on some UIs. Also, a hard 2000 ms wait slows the suite. Guard clicks using aria-pressed and trim the sleep. Optionally, tests can validate ordering; the page object doesn’t need to enforce it.
Apply this diff:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + const kindPressed = await kindButton.getAttribute('aria-pressed'); + if (kindPressed !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - await this.window.waitForTimeout(2000); + const sortPressed = await sortButton.getAttribute('aria-pressed'); + if (sortPressed !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } + // Brief settle time; rely on test assertions for actual order verification + await this.window.waitForTimeout(300); }
224-244: Scope the options to the opened Category menu using aria-controls to avoid cross-menu matches.Constraining the search to the dropdown panel reduces false matches when multiple menus are present.
Apply this diff:
public async openCategory() { const view = await this.getView(KAIViews.analysisView); const categoryBtn = view.getByRole('button', { name: /^Category(?:\s*\d+)?$/i }); await categoryBtn.scrollIntoViewIfNeeded(); await categoryBtn.click(); await expect(categoryBtn).toHaveAttribute('aria-expanded', 'true', { timeout: 3000 }); - const options = view.locator( - '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' - ); + const menuId = await categoryBtn.getAttribute('aria-controls'); + const container = menuId ? view.locator(`#${menuId}`) : view; + const options = container.locator( + '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' + ); await expect(options.first()).toBeVisible({ timeout: 6000 }); return { categoryBtn, options }; }
245-266: Close the Category menu deterministically after selecting an option; minor regex note.
- After clicking an option, explicitly assert the dropdown closes to avoid lingering overlays.
- Regex from variable input is escaped; low ReDoS risk. If desired, avoid regex for string input by using exact text queries.
Apply this diff:
public async setCategoryByName(name: string | RegExp): Promise<void> { const { categoryBtn, options } = await this.openCategory(); const toRegex = (s: string) => new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`, 'i'); const opt = typeof name === 'string' ? options.filter({ hasText: toRegex(name) }).first() : options.filter({ hasText: name as RegExp }).first(); await expect(opt).toBeVisible({ timeout: 5000 }); await opt.click(); await this.window.waitForTimeout(50); - if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { - await categoryBtn.click(); - } + if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { + await categoryBtn.click(); + } + await expect(categoryBtn).toHaveAttribute('aria-expanded', 'false', { timeout: 2000 }); }Optional alternative (avoid regex for strings):
- const opt = - typeof name === 'string' - ? options.filter({ hasText: toRegex(name) }).first() - : options.filter({ hasText: name as RegExp }).first(); + const opt = + typeof name === 'string' + ? options.getByText(name, { exact: true }).first() + : options.filter({ hasText: name as RegExp }).first();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 251-251: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
🔇 Additional comments (1)
tests/e2e/pages/vscode.page.ts (1)
14-15: Nice: clear, narrow union types for sorting/kind.These aliases improve readability and make the page API self-documenting.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
8fdd37a to
87bb3d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/pages/vscode.page.ts (1)
214-225: getListNames ignores ListKind and filters out Files; will break new Files testsThe method currently:
- Ignores the ListKind parameter (named
_), and- Filters cards by the presence of a “Get solution” button, which is typical for Issues but not Files.
This will cause Files queries to return an empty list and make the Files sorting/search tests fail.
- async getListNames(_: ListKind): Promise<string[]> { - const view = await this.getView(KAIViews.analysisView); - const listCards = view - .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); - const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 6_000 }); - - const texts = await titles.allInnerTexts(); - return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); - } + async getListNames(kind: ListKind): Promise<string[]> { + const view = await this.getView(KAIViews.analysisView); + let listCards = view + .locator('[data-ouia-component-type="PF6/Card"]') + .filter({ has: view.getByRole('heading', { level: 3 }) }); + + // Only narrow for issues; Files typically won't have a "Get solution" button + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } + + const titles = listCards.getByRole('heading', { level: 3 }); + await expect(titles.first()).toBeVisible({ timeout: 30_000 }); + + const texts = await titles.allInnerTexts(); + return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); + }tests/e2e/tests/configure-and-run-analysis.test.ts (1)
100-125: Category test assumes partitioning; assert per-category ≤ baseline insteadSumming counts across categories and expecting equality to baseline will fail if categories overlap or the control enforces at least one selection. The acceptance criteria only requires that filtering never exceeds the total issues count.
test('Category never exceeds number of incidents for Issues', async () => { await vscodeApp.setListKindAndSort('issues', 'ascending'); await vscodeApp.searchViolation(''); const baseline = (await vscodeApp.getListNames('issues')).length; expect(baseline).toBeGreaterThan(0); //console.log(`Total number of issues: ${baseline}`); - let totalNumberOfIssues = 0; - for (const name of CATEGORY_NAMES) { await vscodeApp.setCategoryByName(name); const issuesCount = (await vscodeApp.getListNames('issues')).length; - totalNumberOfIssues += issuesCount; + expect(issuesCount).toBeLessThanOrEqual(baseline); //console.log(`Category ${name}: ${issuesCount}`); await vscodeApp.setCategoryByName(name); } - expect(totalNumberOfIssues).toBe(baseline); });Optional: derive
CATEGORY_NAMESfrom the actual dropdown to avoid drift if categories change.
🧹 Nitpick comments (5)
tests/e2e/pages/vscode.page.ts (2)
191-207: Harden list toggling and sort selection to avoid flakiness; handle count-suffixed labelsIf the currently selected kind is already pressed, clicking it again might toggle it off in some implementations. Also, some UIs append counts to the button text (e.g., “Files 12”). Use a regex and only click when not already pressed.
- public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { - const analysisView = await this.getView(KAIViews.analysisView); - const kindButton = analysisView.getByRole('button', { - name: kind === 'issues' ? 'Issues' : 'Files', - }); - - await expect(kindButton).toBeVisible({ timeout: 5_000 }); - await expect(kindButton).toBeEnabled({ timeout: 3_000 }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); - const sortButton = analysisView.getByRole('button', { - name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', - }); - await expect(sortButton).toBeVisible({ timeout: 3_000 }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); - } + public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { + const analysisView = await this.getView(KAIViews.analysisView); + const label = kind === 'issues' ? 'Issues' : 'Files'; + const kindButton = analysisView.getByRole('button', { + name: new RegExp(`^${label}(?:\\s*\\d+)?$`, 'i'), + }); + + await expect(kindButton).toBeVisible({ timeout: 5_000 }); + await expect(kindButton).toBeEnabled({ timeout: 3_000 }); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } + + const sortLabel = order === 'ascending' ? 'Sort ascending' : 'Sort descending'; + const sortButton = analysisView.getByRole('button', { + name: new RegExp(`^${sortLabel}$`, 'i'), + }); + await expect(sortButton).toBeVisible({ timeout: 3_000 }); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } + }
231-246: Scope the options to the dropdown panel when possible; fix JSDoc mismatch
- If the button exposes aria-controls, scoping the options to that menu reduces cross-menu interference.
- The JSDoc says you return “menu” but you only return { categoryBtn, options }.
public async openCategory() { const view = await this.getView(KAIViews.analysisView); const categoryBtn = view.getByRole('button', { name: /^Category(?:\s*\d+)?$/i }); await categoryBtn.scrollIntoViewIfNeeded(); await categoryBtn.click(); await expect(categoryBtn).toHaveAttribute('aria-expanded', 'true', { timeout: 3000 }); - const options = view.locator( + const menuId = await categoryBtn.getAttribute('aria-controls'); + const scope = menuId ? view.locator(`#${menuId}`) : view; + const options = scope.locator( '[role="menuitem"], [role="menuitemcheckbox"], [role="menuitemradio"], [role="option"]' ); await expect(options.first()).toBeVisible({ timeout: 6000 }); - return { categoryBtn, options }; + return { categoryBtn, options }; }Follow-up: either update the JSDoc to “…returns categoryBtn and options” or include a
menulocator in the return object.tests/e2e/tests/configure-and-run-analysis.test.ts (3)
50-61: Descending = reverse-of-ascending can be brittle on ties; add a monotonicity checkReversing ascending is strong, but if the UI breaks ties with a secondary key, exact reverse may fail even when the list is properly “non-increasing.” Add an isDescending check alongside the equality to reduce false positives.
- test('Set list kind and sort (Issues ascending and descending)', async () => { + test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'ascending'); const namesAscending = await vscodeApp.getListNames('issues'); //console.log('Issue names:', namesAscending); expect(isAscending(namesAscending)).toBe(true); await vscodeApp.setListKindAndSort('issues', 'descending'); const namesDescending = await vscodeApp.getListNames('issues'); //console.log('Issue names:', namesDescending); - expect(isAscending(namesDescending)).toBe(false); - expect(namesDescending).toEqual([...namesAscending].reverse()); + const isDescending = (arr: string[]) => + arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) >= 0); + expect(isDescending(namesDescending)).toBe(true); + expect(namesDescending).toEqual([...namesAscending].reverse()); });If you know the UI always strictly flips the comparator and keeps stability, keeping the equality check is fine.
67-78: Mirror the monotonicity guard for Files descendingSame reasoning as Issues; add a non-increasing check to catch ordering bugs without overfitting to exact reversal on ties.
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const filesAscending = await vscodeApp.getListNames('files'); //console.log('File names:', filesAscending); expect(isAscending(filesAscending)).toBe(true); await vscodeApp.setListKindAndSort('files', 'descending'); const filesDescending = await vscodeApp.getListNames('files'); //console.log('File names:', filesDescending); - expect(isAscending(filesDescending)).toBe(false); + const isDescending = (arr: string[]) => + arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) >= 0); + expect(isDescending(filesDescending)).toBe(true); expect(filesDescending).toEqual([...filesAscending].reverse()); });
85-98: Strengthen the “clear search” assertion by restoring to the baseline countYou already capture the initial list. Assert that clearing brings the count back to that baseline to detect partial resets.
test('Files: search narrows to one; clearing expands again', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); - const all = await vscodeApp.getListNames('files'); - expect(all.length).toBeGreaterThan(0); - const filename = all[0]; + const all = await vscodeApp.getListNames('files'); + const baseline = all.length; + expect(baseline).toBeGreaterThan(0); + const filename = all[0]; await vscodeApp.searchViolation(filename); await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBe(1); const names1 = await vscodeApp.getListNames('files'); expect(names1).toEqual([filename]); await vscodeApp.searchViolation(''); const names2 = await vscodeApp.getListNames('files'); - expect(names2.length).toBeGreaterThan(1); + expect(names2.length).toBe(baseline); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-15T18:28:41.561Z
Learnt from: pranavgaikwad
PR: konveyor/editor-extensions#663
File: agentic/src/cache.ts:59-66
Timestamp: 2025-08-15T18:28:41.561Z
Learning: User pranavgaikwad on PR konveyor/editor-extensions#663 requested to stop receiving review comments while debugging, indicating they find automated reviews distracting during active development/debugging phases.
Applied to files:
tests/e2e/tests/configure-and-run-analysis.test.ts
🧬 Code graph analysis (1)
tests/e2e/tests/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 254-254: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (4)
tests/e2e/pages/vscode.page.ts (2)
14-15: Type aliases look good and scoped appropriatelyClear and self-descriptive; no action needed.
252-268: Regex-from-variable warning is a false positive here; input is safely escapedYou escape all metacharacters before embedding the string into a bounded, anchored regex, so ReDoS risk is negligible. No changes needed.
tests/e2e/tests/configure-and-run-analysis.test.ts (2)
38-39: Reasonable timeout bump for “Analysis completed”Given prior learnings about cross-platform slowness, increasing to 400s is acceptable. Keep an eye on flakiness; you can raise it further if Windows runners still time out.
42-45: Natural, numeric-aware ordering helper looks goodUsing Intl.Collator with numeric: true and sensitivity: 'base' is appropriate for UI-visible strings.
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
87bb3d6 to
8e2511f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
227-244: getListNames currently excludes Files and ignores the kind parameterThis locator filters to cards that have a “Get solution” button, which is characteristic of Issues but not Files. Also, the
_parameter is unused, sogetListNames('files')will likely return an empty list and break your new tests.- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view + let listCards = view .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Only Issues reliably have the "Get solution" button; Files typically don't. + if (kind === 'issues') { + listCards = listCards.filter({ has: view.getByRole('button', { name: /get solution/i }) }); + } const titles = listCards.getByRole('heading', { level: 3 }); await expect(titles.first()).toBeVisible({ timeout: 6_000 }); const texts = await titles.allInnerTexts(); return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); }
🧹 Nitpick comments (5)
tests/e2e/pages/vscode.page.ts (1)
204-225: Harden list-kind and sort toggling to be idempotent (avoid unnecessary clicks)If the target button is already selected (aria-pressed="true"), clicking again can introduce flakiness depending on the control’s behavior. Make the action conditional.
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); await expect(kindButton).toBeVisible({ timeout: 5_000 }); await expect(kindButton).toBeEnabled({ timeout: 3_000 }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); await expect(sortButton).toBeVisible({ timeout: 3_000 }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + } }tests/e2e/tests/base/configure-and-run-analysis.test.ts (4)
43-46: Add a descending helper to avoid duplicating comparator logicYou already have a locale-aware ascending check. Introduce a companion non-increasing check and use it in descending tests to reduce brittleness.
-const collator = new Intl.Collator(undefined, { sensitivity: 'base', numeric: true }); -const isAscending = (arr: string[]) => - arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) <= 0); +const collator = new Intl.Collator(undefined, { sensitivity: 'base', numeric: true }); +const isAscending = (arr: string[]) => + arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) <= 0); +const isNonIncreasing = (arr: string[]) => + arr.every((v, i, a) => i === 0 || collator.compare(a[i - 1], v) >= 0);
51-60: Relax descending-order assertion to avoid false negatives on ties; assert meaningful list sizeRequiring exact reverse of the ascending array can fail if the UI uses a stable sort and there are ties. Check for non-increasing order instead, and ensure the list has enough items to make the sort meaningful.
test('Set list kind and sort (Issues ascending and descending)', async () => { await vscodeApp.setListKindAndSort('issues', 'ascending'); const namesAscending = await vscodeApp.getListNames('issues'); - expect(isAscending(namesAscending)).toBe(true); + expect(namesAscending.length).toBeGreaterThan(1); + expect(isAscending(namesAscending)).toBe(true); await vscodeApp.setListKindAndSort('issues', 'descending'); const namesDescending = await vscodeApp.getListNames('issues'); - expect(isAscending(namesDescending)).toBe(false); - expect(namesDescending).toEqual([...namesAscending].reverse()); + expect(namesDescending.length).toBe(namesAscending.length); + expect(isNonIncreasing(namesDescending)).toBe(true); + // Optional: ensure the element sets match, ignoring order + expect([...namesDescending].sort(collator.compare)) + .toEqual([...namesAscending].sort(collator.compare)); });
66-75: Validate file-name plausibility and avoid brittle reverse-equalityAdd lightweight file-name checks and relax descending assertion as above. This also aligns with your PR description (“verify that returned items are valid file names”).
test('Set list kind and sort (Files ascending and descending)', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const filesAscending = await vscodeApp.getListNames('files'); - expect(isAscending(filesAscending)).toBe(true); + expect(filesAscending.length).toBeGreaterThan(1); + expect(isAscending(filesAscending)).toBe(true); + // Basic plausibility checks for filenames/paths + const invalidChars = /[<>:"|?*\x00]/; // common invalids + for (const n of filesAscending) { + expect(typeof n).toBe('string'); + expect(n.trim().length).toBeGreaterThan(0); + expect(invalidChars.test(n)).toBe(false); + expect(/\r|\n/.test(n)).toBe(false); + } await vscodeApp.setListKindAndSort('files', 'descending'); const filesDescending = await vscodeApp.getListNames('files'); - expect(isAscending(filesDescending)).toBe(false); - expect(filesDescending).toEqual([...filesAscending].reverse()); + expect(filesDescending.length).toBe(filesAscending.length); + expect(isNonIncreasing(filesDescending)).toBe(true); + expect([...filesDescending].sort(collator.compare)) + .toEqual([...filesAscending].sort(collator.compare)); });
82-95: After clearing the search, wait for the list to repopulate to avoid flakinessUse expect.poll a second time to assert the list expands before reading values.
test('Files: search narrows to one; clearing expands again', async () => { await vscodeApp.setListKindAndSort('files', 'ascending'); const all = await vscodeApp.getListNames('files'); expect(all.length).toBeGreaterThan(0); const filename = all[0]; await vscodeApp.searchViolation(filename); await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBe(1); const names1 = await vscodeApp.getListNames('files'); expect(names1).toEqual([filename]); await vscodeApp.searchViolation(''); - const names2 = await vscodeApp.getListNames('files'); - expect(names2.length).toBeGreaterThan(1); + await expect.poll(async () => (await vscodeApp.getListNames('files')).length).toBeGreaterThan(1); + const names2 = await vscodeApp.getListNames('files'); + expect(names2.length).toBeGreaterThan(1); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/base/configure-and-run-analysis.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T15:15:59.771Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#591
File: tests/e2e/pages/vscode.pages.ts:169-174
Timestamp: 2025-07-21T15:15:59.771Z
Learning: In tests/e2e/pages/vscode.pages.ts, the hardcoded 15-second wait in the runAnalysis() method and the 600000ms (10-minute) timeout for the "Run Analysis" button are intentionally set to accommodate performance differences between Linux and Windows environments where tests run slower. These timeouts should not be reduced or replaced with element-based waits without understanding the cross-platform testing requirements.
Applied to files:
tests/e2e/tests/base/configure-and-run-analysis.test.ts
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🧬 Code graph analysis (1)
tests/e2e/tests/base/configure-and-run-analysis.test.ts (1)
tests/e2e/fixtures/test-repo-fixture.ts (1)
test(16-29)
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 272-272: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (linux)
- GitHub Check: Build (windows)
- GitHub Check: Build (macos)
🔇 Additional comments (4)
tests/e2e/pages/vscode.page.ts (2)
17-18: Good addition: explicit union types for sort order and list kindClear, self-documenting aliases that improve readability and reduce typos in tests.
245-264: Category open workflow looks solidRobust role-based selectors and explicit expanded-state assertions make this stable.
tests/e2e/tests/base/configure-and-run-analysis.test.ts (2)
39-40: Timeout bump to 400s for “Analysis completed” is reasonableGiven cross-platform variability, increasing this from 300s to 400s is pragmatic. Keep an eye on overall suite duration.
If Windows runners still flirt with this limit, consider gating by OS or making it configurable via env.
97-97: Category list looks correctNames align with the typical severity categories used by the extension.
tests/e2e/pages/vscode.page.ts
Outdated
| /** | ||
| * Selects a category by its name or RegExp in the category filter dropdown in the analysis view. | ||
| * @param name - The name or RegExp of the category to select. | ||
| */ | ||
| public async setCategoryByName(name: string | RegExp): Promise<void> { | ||
| const { categoryBtn, options } = await this.openCategory(); | ||
| const toRegex = (s: string) => | ||
| new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`, 'i'); | ||
|
|
||
| const opt = | ||
| typeof name === 'string' | ||
| ? options.filter({ hasText: toRegex(name) }).first() | ||
| : options.filter({ hasText: name as RegExp }).first(); | ||
| await expect(opt).toBeVisible({ timeout: 5000 }); | ||
| await opt.click(); | ||
| await this.window.waitForTimeout(50); | ||
|
|
||
| if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { | ||
| await categoryBtn.click(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
setCategoryByName does not “clear” prior selections; make checkbox menus exclusive and add a safety cap
Past discussion indicated this should clear previous selections. Current code only toggles the matched option, which can yield multi-select states and flaky counts. Add clearing behavior for checkbox-style menus and a hard safety cap to avoid infinite loops.
public async setCategoryByName(name: string | RegExp): Promise<void> {
- const { categoryBtn, options } = await this.openCategory();
+ const { categoryBtn, options } = await this.openCategory();
+ const view = await this.getView(KAIViews.analysisView);
const toRegex = (s: string) =>
new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`, 'i');
+ // If menu offers checkbox multi-select, clear previous selections first to ensure exclusivity.
+ const checked = view.locator('[role="menuitemcheckbox"][aria-checked="true"]');
+ if ((await view.locator('[role="menuitemcheckbox"]').count()) > 0) {
+ let iterations = 0;
+ for (;;) {
+ const n = await checked.count();
+ if (n === 0) break;
+ await checked.first().click();
+ // Re-open if menu closed after click
+ if ((await categoryBtn.getAttribute('aria-expanded')) !== 'true') {
+ const reopened = await this.openCategory();
+ // re-bind in case categoryBtn changed
+ (categoryBtn as any) = reopened.categoryBtn;
+ } else {
+ await this.window.waitForTimeout(50);
+ }
+ if (++iterations > 50) {
+ throw new Error('setCategoryByName: exceeded safety iterations while clearing selections.');
+ }
+ }
+ }
+
const opt =
typeof name === 'string'
? options.filter({ hasText: toRegex(name) }).first()
: options.filter({ hasText: name as RegExp }).first();
await expect(opt).toBeVisible({ timeout: 5000 });
await opt.click();
await this.window.waitForTimeout(50);
if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') {
await categoryBtn.click();
}
}Note: The regex here is fully escaped/anchored and low-risk; static analysis warning about dynamic regex can be safely ignored given bounded, trusted inputs. If preferred, we can replace it with a role-based exact-name match strategy.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 272-272: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
| test('Category never exceeds number of incidents for Issues', async () => { | ||
| await vscodeApp.setListKindAndSort('issues', 'ascending'); | ||
| await vscodeApp.searchViolation(''); | ||
|
|
||
| const baseline = (await vscodeApp.getListNames('issues')).length; | ||
| expect(baseline).toBeGreaterThan(0); | ||
|
|
||
| let totalNumberOfIssues = 0; | ||
|
|
||
| for (const name of CATEGORY_NAMES) { | ||
| await vscodeApp.setCategoryByName(name); | ||
| const issuesCount = (await vscodeApp.getListNames('issues')).length; | ||
| totalNumberOfIssues += issuesCount; | ||
| await vscodeApp.setCategoryByName(name); | ||
| } | ||
| expect(totalNumberOfIssues).toBe(baseline); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
The category test is stronger than the stated objective and depends on exclusivity that may not hold
Your PR objective says “result count does not exceed the total number of issues.” Summing counts across categories and asserting equality to baseline assumes categories are disjoint and exhaustive. That can be false and/or lead to flakes if the filter is multi-select. Align the test to the objective and use a union check.
test('Category never exceeds number of incidents for Issues', async () => {
await vscodeApp.setListKindAndSort('issues', 'ascending');
await vscodeApp.searchViolation('');
const baseline = (await vscodeApp.getListNames('issues')).length;
expect(baseline).toBeGreaterThan(0);
- let totalNumberOfIssues = 0;
+ const union = new Set<string>();
for (const name of CATEGORY_NAMES) {
await vscodeApp.setCategoryByName(name);
- const issuesCount = (await vscodeApp.getListNames('issues')).length;
- totalNumberOfIssues += issuesCount;
- await vscodeApp.setCategoryByName(name);
+ const items = await vscodeApp.getListNames('issues');
+ expect(items.length).toBeLessThanOrEqual(baseline);
+ items.forEach((n) => union.add(n));
+ // Optional: toggle back if the control is a checkbox menu
+ await vscodeApp.setCategoryByName(name);
}
- expect(totalNumberOfIssues).toBe(baseline);
+ expect(union.size).toBeLessThanOrEqual(baseline);
});Additionally, the current page method setCategoryByName does not clear previous selections (checkbox menus). The companion refactor proposed in vscode.page.ts will make this test deterministic.
🏁 Script executed:
#!/bin/bash
# Search for the implementation of setCategoryByName
rg -n "setCategoryByName" -C3
# Search for the implementation of getListNames
rg -n "getListNames" -C3
# Find where CATEGORY_NAMES is defined
rg -n "CATEGORY_NAMES" -C3Length of output: 8912
🏁 Script executed:
#!/bin/bash
# Inspect the implementation of setCategoryByName to verify its toggle behavior
sed -n '260,310p' tests/e2e/pages/vscode.page.tsLength of output: 1969
Strengthen and Correct the Category Filter Test
The current test sums issue counts across categories and asserts equality to the unfiltered baseline, which only holds if categories are both disjoint and exhaustive. In practice, issues may belong to multiple categories or none, causing this assertion to flake or be invalid. Instead, the test should:
- For each category filter, verify that its filtered count never exceeds the baseline
- Explicitly reset or clear the filter after each check to ensure isolation
- (Optional) Aggregate a union of all filtered issue IDs/names and assert the union size ≤ baseline
Suggested changes in tests/e2e/tests/base/configure-and-run-analysis.test.ts:
test('Category never exceeds number of incidents for Issues', async () => {
await vscodeApp.setListKindAndSort('issues', 'ascending');
await vscodeApp.searchViolation('');
const baseline = (await vscodeApp.getListNames('issues')).length;
expect(baseline).toBeGreaterThan(0);
- let totalNumberOfIssues = 0;
+ const seenIssues = new Set<string>();
for (const name of CATEGORY_NAMES) {
- await vscodeApp.setCategoryByName(name);
- const issuesCount = (await vscodeApp.getListNames('issues')).length;
- totalNumberOfIssues += issuesCount;
- await vscodeApp.setCategoryByName(name);
+ // Apply single-category filter
+ await vscodeApp.setCategoryByName(name);
+
+ // Verify filtered count does not exceed unfiltered baseline
+ const items = await vscodeApp.getListNames('issues');
+ expect(items.length).toBeLessThanOrEqual(baseline);
+
+ // (Optional) Track unique items for a union check
+ items.forEach((n) => seenIssues.add(n));
+
+ // Clear the category filter to isolate next iteration
+ await vscodeApp.setCategoryByName(name);
}
-
- expect(totalNumberOfIssues).toBe(baseline);
+ // (Optional) Ensure the union of all categories is still bounded by the baseline
+ expect(seenIssues.size).toBeLessThanOrEqual(baseline);
});Additionally, to guarantee deterministic toggling, the page object method setCategoryByName() should be accompanied by a clearAllCategories() or accept an explicit “on/off” parameter so that tests need not rely on double‐clicking to reset state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('Category never exceeds number of incidents for Issues', async () => { | |
| await vscodeApp.setListKindAndSort('issues', 'ascending'); | |
| await vscodeApp.searchViolation(''); | |
| const baseline = (await vscodeApp.getListNames('issues')).length; | |
| expect(baseline).toBeGreaterThan(0); | |
| let totalNumberOfIssues = 0; | |
| for (const name of CATEGORY_NAMES) { | |
| await vscodeApp.setCategoryByName(name); | |
| const issuesCount = (await vscodeApp.getListNames('issues')).length; | |
| totalNumberOfIssues += issuesCount; | |
| await vscodeApp.setCategoryByName(name); | |
| } | |
| expect(totalNumberOfIssues).toBe(baseline); | |
| }); | |
| test('Category never exceeds number of incidents for Issues', async () => { | |
| await vscodeApp.setListKindAndSort('issues', 'ascending'); | |
| await vscodeApp.searchViolation(''); | |
| const baseline = (await vscodeApp.getListNames('issues')).length; | |
| expect(baseline).toBeGreaterThan(0); | |
| const seenIssues = new Set<string>(); | |
| for (const name of CATEGORY_NAMES) { | |
| // Apply single-category filter | |
| await vscodeApp.setCategoryByName(name); | |
| // Verify filtered count does not exceed unfiltered baseline | |
| const items = await vscodeApp.getListNames('issues'); | |
| expect(items.length).toBeLessThanOrEqual(baseline); | |
| // (Optional) Track unique items for a union check | |
| items.forEach((n) => seenIssues.add(n)); | |
| // Clear the category filter to isolate next iteration | |
| await vscodeApp.setCategoryByName(name); | |
| } | |
| // (Optional) Ensure the union of all categories is still bounded by the baseline | |
| expect(seenIssues.size).toBeLessThanOrEqual(baseline); | |
| }); |
🤖 Prompt for AI Agents
In tests/e2e/tests/base/configure-and-run-analysis.test.ts around lines 104 to
120, the test incorrectly sums counts across categories and assumes categories
are disjoint; change it so that for each category you set the list kind/sort,
set the category filter, get the filtered count and assert filteredCount <=
baseline, then explicitly clear filters before the next iteration (use a new
clearAllCategories() page-object method or modify setCategoryByName(name, state)
to accept an on/off argument to deterministically enable/disable a category);
optionally, to strengthen the test add logic to collect unioned issue IDs across
categories and assert unionSize <= baseline, but the primary fix is per-category
<= baseline and explicit filter clearing between iterations.
Signed-off-by: hhpatel14 <[email protected]>
hhpatel14
force-pushed
the
665-filteringIssue
branch
from
8e2511f to
caeef8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/e2e/pages/vscode.page.ts (1)
232-243: Bug: getListNames ignores kind and filters out Files; also risks collecting hidden titlesCurrently the method ignores the ListKind argument and filters to cards with a “Get solution” button, which excludes Files and will break the Files sort/search tests. It also uses allInnerTexts(), which can include hidden items if both lists exist in the DOM.
Apply this diff to honor kind and collect only visible headings:
- async getListNames(_: ListKind): Promise<string[]> { + async getListNames(kind: ListKind): Promise<string[]> { const view = await this.getView(KAIViews.analysisView); - const listCards = view + let listCards = view .locator('[data-ouia-component-type="PF6/Card"]') - .filter({ has: view.getByRole('heading', { level: 3 }) }) - .filter({ has: view.getByRole('button', { name: /get solution/i }) }); + .filter({ has: view.getByRole('heading', { level: 3 }) }); + // Issues typically have "Get solution"; Files do not. + if (kind === 'issues') { + listCards = listCards.filter({ + has: view.getByRole('button', { name: /get solution/i }), + }); + } const titles = listCards.getByRole('heading', { level: 3 }); - await expect(titles.first()).toBeVisible({ timeout: 6_000 }); - - const texts = await titles.allInnerTexts(); - return texts.map((t) => t.replace(/\s+/g, ' ').trim()).filter(Boolean); + await expect(titles.first()).toBeVisible({ timeout: 10_000 }); + const nodes = await titles.all(); + const texts: string[] = []; + for (const n of nodes) { + if (await n.isVisible()) { + const t = (await n.innerText()).replace(/\s+/g, ' ').trim(); + if (t) texts.push(t); + } + } + return texts; }
🧹 Nitpick comments (2)
tests/e2e/pages/vscode.page.ts (2)
204-225: Avoid toggling already-selected controls; click only when needed and give UI a moment to settle after sortClicking Kind/Sort unconditionally risks flipping an already-selected toggle in some implementations. Also, a tiny post-sort settle helps avoid stale reads in subsequent assertions.
Apply this diff:
public async setListKindAndSort(kind: ListKind, order: SortOrder): Promise<void> { const analysisView = await this.getView(KAIViews.analysisView); const kindButton = analysisView.getByRole('button', { name: kind === 'issues' ? 'Issues' : 'Files', }); await expect(kindButton).toBeVisible({ timeout: 5_000 }); await expect(kindButton).toBeEnabled({ timeout: 3_000 }); - await kindButton.click(); - await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + if ((await kindButton.getAttribute('aria-pressed')) !== 'true') { + await kindButton.click(); + await expect(kindButton).toHaveAttribute('aria-pressed', 'true'); + } const sortButton = analysisView.getByRole('button', { name: order === 'ascending' ? 'Sort ascending' : 'Sort descending', }); await expect(sortButton).toBeVisible({ timeout: 3_000 }); - await sortButton.click(); - await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + if ((await sortButton.getAttribute('aria-pressed')) !== 'true') { + await sortButton.click(); + } + await expect(sortButton).toHaveAttribute('aria-pressed', 'true'); + // brief settle to avoid reading pre-sort list state + await this.window.waitForTimeout(100); }
270-285: Ensure single-selection behavior for checkbox-style menus; optional: avoid dynamic regex constructionIf the Category menu is checkbox-based (multi-select), simply clicking the target option may accumulate selections and skew counts in the new tests. Clearing existing checked menu items makes the selection exclusive and results deterministic. Dynamic regex is fine in tests, but you can avoid it altogether with exact name matching if desired.
Proposed minimal clearing inside this method:
public async setCategoryByName(name: string | RegExp): Promise<void> { - const { categoryBtn, options } = await this.openCategory(); + const { categoryBtn, options } = await this.openCategory(); + const view = await this.getView(KAIViews.analysisView); + // If the menu is checkbox-based, clear prior selections to ensure exclusivity. + const checked = view.locator('[role="menuitemcheckbox"][aria-checked="true"]'); + const count = await checked.count(); + for (let i = 0; i < count; i++) { + await checked.first().click(); + } const toRegex = (s: string) => new RegExp(`^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$`, 'i'); const opt = typeof name === 'string' ? options.filter({ hasText: toRegex(name) }).first() : options.filter({ hasText: name as RegExp }).first(); await expect(opt).toBeVisible({ timeout: 5000 }); await opt.click(); if ((await categoryBtn.getAttribute('aria-expanded')) === 'true') { await categoryBtn.click(); } }Optional alternative (avoids dynamic regex entirely when a string is passed):
// Instead of toRegex + hasText, try an exact accessible name match: const opt = typeof name === 'string' ? options.filter({ hasText: new RegExp(`^\\s*${name.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\$&')}\\s*$`, 'i') }).first() : options.filter({ hasText: name }).first(); // Or, if the items expose a stable role/name, prefer: view.getByRole('<role>', { name, exact: true })Question: Is the Category menu guaranteed to be single-select (radio) in all themes/layouts? If not, I recommend merging the clearing step above to keep tests deterministic across environments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/e2e/pages/vscode.page.ts(2 hunks)tests/e2e/tests/base/configure-and-run-analysis.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/base/configure-and-run-analysis.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T10:50:42.586Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#683
File: tests/e2e/tests/base/fix-one-issue.test.ts:32-36
Timestamp: 2025-08-14T10:50:42.586Z
Learning: In tests/e2e/tests/base/fix-one-issue.test.ts, dispatchEvent('click') is used instead of standard click() for the "Accept all changes" button because screen size constraints cause notifications to overlay the button, and scrollIntoViewIfNeeded() doesn't resolve this overlay issue. This is a necessary workaround for reliable test execution in constrained screen environments.
Applied to files:
tests/e2e/pages/vscode.page.ts
🪛 ast-grep (0.38.6)
tests/e2e/pages/vscode.page.ts
[warning] 272-272: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*$, 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (2)
tests/e2e/pages/vscode.page.ts (2)
17-18: Type aliases read well and match test semanticsSortOrder and ListKind are concise and self-explanatory; good fit for the new page-object API.
249-264: LGTM; accessible-role approach is solidopenCategory uses clear ARIA roles, scrolls into view, and asserts expanded state before enumerating options. Good foundation for stable interactions.
Filtering, sorting, search, and category behavior E2E tests
Fixes #665
This PR Adds 4 tests for the search Panel.
Prerequisites:
Run analysis first and ensure results for more than one incident (and multiple files/issues) so sort/order checks are meaningful.
Summary by CodeRabbit
Tests
Chores