feat: add test for dependencies #119
Conversation
WalkthroughAdds a global Playwright Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test
participant Page as Playwright Page
participant Helper as waitForPageLoad
rect rgba(200,230,255,0.35)
Test->>Page: goto(component URL)
Test->>Helper: waitForPageLoad(Page, componentName)
activate Helper
Helper->>Page: wait for progress bars hidden (per-bar timeout)
Helper->>Page: wait for #sidebar-root attached
Helper->>Page: wait for heading(componentName) visible
Helper->>Page: wait for load state 'load'
Helper-->>Test: resolved
deactivate Helper
end
sequenceDiagram
autonumber
participant Test as Test
participant Ui as UiComponent
participant Plugin as DependenciesUiPlugin
participant Page as Playwright Page
rect rgba(220,255,220,0.25)
Test->>Ui: getDependencies()
Ui-->>Test: DependenciesUiPlugin
Test->>Page: goto(component dependencies URL)
Test->>Plugin: checkAllBoxesPresent(Page)
Plugin->>Page: assert titles visible
Test->>Plugin: checkRelationsTitle(Page)
Plugin->>Page: assert "Relations" visible
Test->>Plugin: checkNodesPresent(Page)
Plugin->>Page: assert component & gitops nodes present
Test->>Plugin: goToGitopsDependency(Page)
Plugin->>Page: click gitops node -> waitForPageLoad(Page, componentName-gitops)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Provide constructive feedback focused on testing reliability, security, and maintainability: ensure waitForPageLoad timeouts and selectors are resilient to avoid flakiness, validate that video/screenshot defaults do not capture or retain sensitive data, centralize page-object strings to ease maintenance, prefer explicit selector timeouts where navigation assumptions are critical, and add unit/IT-level assertions around plugin wiring to catch injection regressions. 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. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)tests/**/*.test.ts📄 CodeRabbit inference engine (README.md)
Files:
{tests/ui/**/*.ts,src/ui/**/*.ts}📄 CodeRabbit inference engine (README.md)
Files:
tests/**⚙️ CodeRabbit configuration file
Files:
**/*.test.ts⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-07-31T07:43:28.355ZApplied to files:
🧬 Code graph analysis (1)tests/ui/ui.test.ts (2)
⏰ 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)
🔇 Additional comments (6)
✨ 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
added
configuration
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui/ui.test.ts (1)
29-37: beforeAll signature is invalid; remove the stray title argumentPlaywright’s test.beforeAll expects a function (and optional timeout). Passing an empty string will cause a type/runtime error.
Apply this diff:
- test.beforeAll('', async ({ testItem }) => { + test.beforeAll(async ({ testItem }) => { console.log('Running UI test for:', testItem); const componentName = testItem.getName(); const imageName = `${componentName}`; console.log(`Creating component: ${componentName}`); // Assign the already created component component = await UiComponent.new(componentName, testItem, imageName); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
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 (6)
playwright.config.ts(2 hunks)src/ui/common.ts(1 hunks)src/ui/page-objects/dependencies_po.ts(1 hunks)src/ui/plugins/dependencies/dependenciesUiPlugin.ts(1 hunks)src/ui/uiComponent.ts(6 hunks)tests/ui/ui.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
{src,tests}/ui/**
📄 CodeRabbit Inference Engine (README.md)
Place all UI-related files under either src/ui or tests/ui directories
Files:
src/ui/uiComponent.tssrc/ui/page-objects/dependencies_po.tssrc/ui/common.tstests/ui/ui.test.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
src/ui/**
⚙️ CodeRabbit Configuration File
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/uiComponent.tssrc/ui/page-objects/dependencies_po.tssrc/ui/common.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
src/ui/page-objects/**/*.ts
📄 CodeRabbit Inference Engine (README.md)
Store all Page Object Model files under src/ui/page-objects
Files:
src/ui/page-objects/dependencies_po.ts
src/ui/page-objects/**/*_po.ts
📄 CodeRabbit Inference Engine (README.md)
Name each Page Object file with a _po.ts suffix
Files:
src/ui/page-objects/dependencies_po.ts
playwright.config.ts
⚙️ CodeRabbit Configuration File
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
tests/ui/ui.test.ts
📄 CodeRabbit Inference Engine (README.md)
Keep the main UI automation test file at tests/ui/ui.test.ts
Files:
tests/ui/ui.test.ts
tests/**
⚙️ CodeRabbit Configuration File
Focus on test coverage, maintainability, proper test structure, test isolation, mock usage, async test patterns, and test stability. Ensure tests are deterministic and properly clean up after themselves.
Files:
tests/ui/ui.test.ts
**/*.test.ts
⚙️ CodeRabbit Configuration File
Ensure comprehensive test coverage, proper assertions, test isolation, async pattern correctness, mock usage, and test maintainability. Review for potential flaky tests and race conditions.
Files:
tests/ui/ui.test.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit Inference Engine (README.md)
src/ui/plugins/**/*.ts: Store plugin-related UI functionality under src/ui/plugins, organized by plugin type (e.g., git, ci); file name should match the plugin’s short name
Classes defined in src/ui/plugins must include either Ui or plugin in their class names
Files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
🧠 Learnings (5)
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to src/ui/plugins/**/*.ts : Classes defined in src/ui/plugins must include either Ui or plugin in their class names
Applied to files:
src/ui/uiComponent.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to src/ui/plugins/**/*.ts : Store plugin-related UI functionality under src/ui/plugins, organized by plugin type (e.g., git, ci); file name should match the plugin’s short name
Applied to files:
src/ui/uiComponent.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to src/ui/page-objects/**/*_po.ts : Name each Page Object file with a _po.ts suffix
Applied to files:
src/ui/page-objects/dependencies_po.ts
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
tests/ui/ui.test.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
PR: redhat-appstudio/tssc-test#72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
tests/ui/ui.test.ts
🧬 Code Graph Analysis (3)
src/ui/uiComponent.ts (2)
src/rhtap/core/component.ts (1)
Component(14-253)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-42)
tests/ui/ui.test.ts (2)
src/ui/common.ts (1)
waitForPageLoad(18-27)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-42)
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
src/ui/page-objects/dependencies_po.ts (1)
DependenciesPO(6-9)
🔇 Additional comments (1)
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
5-18: Plugin placement & naming compliant with guidelinesClass name and file path follow the src/ui/plugins conventions; includes "Ui" and "Plugin" in the class name and is organized under the plugin short name.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui/ui.test.ts (1)
16-24: Fix the suite comment for clarity"This test should not" reads incomplete. Tighten phrasing to avoid confusion for future maintainers.
- * This test should not - * 1. Login to the UI - * TODO: - * 2. Find a component in the UI + * Notes: + * 1) This suite does not perform UI login (auth is handled by storageState). + * 2) TODO: Add step to locate component in UI when navigation patterns stabilize.
♻️ Duplicate comments (7)
src/ui/page-objects/dependencies_po.ts (1)
6-9: Centralize remaining literals (node testId and gitops suffix) in the POMove the 'node' testId and '-gitops' suffix from the plugin into this PO to reduce brittleness and ease future UI changes.
export const DependenciesPO = { titles: ['Depends on components', 'Depends on resources', 'Has subcomponents', 'Provided APIs', 'Consumed APIs'], relationsTitle: 'Relations', + nodeTestId: 'node', + gitopsSuffix: '-gitops', } as const;src/ui/uiComponent.ts (1)
64-66: Decoupling: passing only componentName is the right callDropping unused repo URLs reduces coupling and keeps the plugin focused on UI concerns. Nice.
tests/ui/ui.test.ts (2)
118-123: Relations title check uses text — correct, since it’s not a headingGood alignment with the UI reality (Relations isn’t a heading element).
4-6: Use a type-only import for the plugin to avoid unnecessary runtime dependencyThis import is only used for typing the variable. Switch to a type-only import to keep the runtime graph lean and prevent accidental cycles.
-import { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin'; +import type { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin';src/ui/plugins/dependencies/dependenciesUiPlugin.ts (3)
14-18: Tighten heading match and add explicit return type to reduce flakinessUse exact matching for headings and declare Promise for clarity and consistency.
Apply this diff:
- async checkAllBoxesPresent(page: Page) { + async checkAllBoxesPresent(page: Page): Promise<void> { for (const title of DependenciesPO.titles) { - await expect(page.getByRole('heading', { name: title })).toBeVisible(); + await expect(page.getByRole('heading', { name: title, exact: true })).toBeVisible(); } }
24-28: Make node locators stricter, consistent, and DRY; add explicit return typeUnify on has + getByText({ exact: true }) instead of hasText to avoid substring matches; reuse a helper to reduce duplication; optionally assert uniqueness to catch duplicates early.
Apply this diff:
- async checkNodesPresent(page: Page) { - const componentNode = page.getByTestId('node').filter({ has: page.getByText(this.componentName, { exact: true }) }); - await expect(componentNode).toBeVisible(); - await expect(page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` })).toBeVisible(); - } + async checkNodesPresent(page: Page): Promise<void> { + const componentNode = this.nodeByName(page, this.componentName); + await expect(componentNode).toBeVisible(); + // Optionally ensure uniqueness: + // await expect(componentNode).toHaveCount(1); + + const gitopsNode = this.nodeByName(page, `${this.componentName}-gitops`); + await expect(gitopsNode).toBeVisible(); + // Optionally ensure uniqueness: + // await expect(gitopsNode).toHaveCount(1); + }To support the above, add a small helper inside this class and import Locator:
- import { expect, Page } from '@playwright/test'; + import { expect, Page, Locator } from '@playwright/test'; @@ export class DependenciesUiPlugin { private readonly componentName: string; @@ constructor( componentName: string, ) { this.componentName = componentName; } + + private nodeByName(page: Page, name: string): Locator { + return page + .getByTestId('node') + .filter({ has: page.getByText(name, { exact: true }) }); + }Optional follow-up (outside this file) to centralize test IDs and avoid hard-coding:
// src/ui/page-objects/dependencies_po.ts export const DependenciesPO = { titles: ['Depends on components', 'Depends on resources', 'Has subcomponents', 'Provided APIs', 'Consumed APIs'], relationsTitle: 'Relations', testIds: { node: 'node' }, };Then use page.getByTestId(DependenciesPO.testIds.node) here.
30-35: Stabilize post-click flow; reuse the strict locator and declare return typeReuse the helper for consistency and avoid hasText; keep the deterministic wait via waitForPageLoad. If the click triggers an in-page update (not navigation), this still works because waitForPageLoad waits on spinners and a heading; verify the expected heading matches
${componentName}-gitops.Apply this diff:
- async goToGitopsDependency(page: Page) { - const nodeLocator = page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` }); - await expect(nodeLocator).toBeVisible(); - await nodeLocator.click(); - await waitForPageLoad(page, `${this.componentName}-gitops`); - } + async goToGitopsDependency(page: Page): Promise<void> { + const nodeLocator = this.nodeByName(page, `${this.componentName}-gitops`); + await expect(nodeLocator).toBeVisible(); + await nodeLocator.click(); + await waitForPageLoad(page, `${this.componentName}-gitops`); + }Run this quick audit to standardize selectors and confirm navigation waits after page.goto are consistently applied across UI tests:
#!/bin/bash set -euo pipefail echo "Occurrences of hasText: (prefer has + getByText(..., { exact: true }))" rg -nP --glob '{src,tests}/ui/**' '\bhasText\s*:' echo echo "Heading assertions missing exact: true:" rg -nP --glob '{src,tests}/ui/**' "getByRole\(\s*'heading'\s*,\s*\{\s*name\s*:\s*[^}]*\}\s*\)" -C2 echo echo "Calls to page.goto() with surrounding context (verify waitForPageLoad is used right after):" rg -nP --glob '{src,tests}/ui/**' -C3 "page\.goto\s*\("
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
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 (6)
playwright.config.ts(1 hunks)src/ui/common.ts(1 hunks)src/ui/page-objects/dependencies_po.ts(1 hunks)src/ui/plugins/dependencies/dependenciesUiPlugin.ts(1 hunks)src/ui/uiComponent.ts(6 hunks)tests/ui/ui.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
{src,tests}/ui/**
📄 CodeRabbit inference engine (README.md)
Place all UI-related files under either src/ui or tests/ui directories
Files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.tstests/ui/ui.test.tssrc/ui/common.ts
src/ui/page-objects/**/*.ts
📄 CodeRabbit inference engine (README.md)
Store all Page Object Model files under src/ui/page-objects
Files:
src/ui/page-objects/dependencies_po.ts
src/ui/page-objects/**/*_po.ts
📄 CodeRabbit inference engine (README.md)
Name each Page Object file with a _po.ts suffix
Files:
src/ui/page-objects/dependencies_po.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.tssrc/ui/common.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Store plugin-related UI functionality under src/ui/plugins, organized by plugin type (e.g., git, ci); file name should match the plugin’s short name
Classes defined in src/ui/plugins must include either Ui or plugin in their class names
Files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
tests/ui/ui.test.ts
📄 CodeRabbit inference engine (README.md)
Keep the main UI automation test file at tests/ui/ui.test.ts
Files:
tests/ui/ui.test.ts
tests/**
⚙️ CodeRabbit configuration file
Focus on test coverage, maintainability, proper test structure, test isolation, mock usage, async test patterns, and test stability. Ensure tests are deterministic and properly clean up after themselves.
Files:
tests/ui/ui.test.ts
**/*.test.ts
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper assertions, test isolation, async pattern correctness, mock usage, and test maintainability. Review for potential flaky tests and race conditions.
Files:
tests/ui/ui.test.ts
🧠 Learnings (8)
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to src/ui/page-objects/**/*_po.ts : Name each Page Object file with a _po.ts suffix
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T09:14:32.201Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: src/ui/plugins/dependencies/dependenciesUiPlugin.ts:26-28
Timestamp: 2025-08-21T09:14:32.201Z
Learning: In the dependencies UI page object for TSSC E2E testing framework, the Relations title (DependenciesPO.relationsTitle) is not a heading element, so getByRole('heading') should not be used for this element.
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to src/ui/plugins/**/*.ts : Store plugin-related UI functionality under src/ui/plugins, organized by plugin type (e.g., git, ci); file name should match the plugin’s short name
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.tstests/ui/ui.test.ts
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to src/ui/plugins/**/*.ts : Classes defined in src/ui/plugins must include either Ui or plugin in their class names
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.ts
📚 Learning: 2025-08-19T11:28:24.837Z
Learnt from: jsmid1
PR: redhat-appstudio/tssc-test#101
File: src/ui/plugins/docs/docsUiPlugin.ts:26-28
Timestamp: 2025-08-19T11:28:24.837Z
Learning: The HTML `<strong>` element has an implicit ARIA role of `strong`, making `page.getByRole('strong')` a valid selector in Playwright tests for TSSC E2E testing framework.
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T08:57:44.934Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.934Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
tests/ui/ui.test.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
PR: redhat-appstudio/tssc-test#72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
tests/ui/ui.test.ts
🧬 Code graph analysis (3)
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (2)
src/ui/page-objects/dependencies_po.ts (1)
DependenciesPO(6-9)src/ui/common.ts (1)
waitForPageLoad(18-31)
src/ui/uiComponent.ts (2)
src/rhtap/core/component.ts (1)
Component(14-253)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
tests/ui/ui.test.ts (3)
src/ui/uiComponent.ts (1)
UiComponent(22-118)src/ui/common.ts (2)
waitForPageLoad(18-31)openTab(33-36)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
🔇 Additional comments (9)
src/ui/page-objects/dependencies_po.ts (1)
1-9: Good PO extraction; selectors are readable and colocatedThe PO name and placement follow the conventions; titles and relations label centralized here keep tests maintainable.
playwright.config.ts (1)
107-110: Global artifacts on failure configured correctly; aligns with CI security stanceSetting screenshot: 'only-on-failure' and video: 'retain-on-failure' at the global use level is correct and will apply to all projects. Thanks for keeping trace disabled per CI security guidance.
src/ui/uiComponent.ts (2)
20-33: Constructor injection for DependenciesUiPlugin improves testability and cohesionImporting and wiring the DependenciesUiPlugin via constructor aligns with the existing pattern for Docs/Git and keeps responsibilities clean.
109-117: Getter matches existing style and docs are clearExplicit public modifier and JSDoc keep the API consistent.
tests/ui/ui.test.ts (4)
59-61: Good use of waitForPageLoad after navigationThis reduces flakiness when checking the Git "View Source" link.
81-89: Docs checks are now guarded by waitForPageLoad — goodThis should mitigate rendering races on slower clusters.
124-132: Explicit skip path for unsupported Git providers is clearGood defensive guard; avoids false failures when the Git UI plugin is not available.
138-143: Great: reusing openTab and waitForPageLoad for GitOps DocsOnce openTab is updated to target role=tab and assert selection (see common.ts), this step becomes even more robust.
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
5-12: Plugin naming and placement align with guidelinesClass name includes “Ui”/“Plugin” and file path matches the plugin’s short name. Good adherence to repo conventions.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (13)
src/ui/page-objects/dependencies_po.ts (1)
6-9: Centralize commonly used identifiers (testId/suffix) in the POMove hard-coded literals from the plugin into the PO to reduce brittleness and improve maintainability. This also makes selector updates cheaper if the UI changes.
export const DependenciesPO = { titles: ['Depends on components', 'Depends on resources', 'Has subcomponents', 'Provided APIs', 'Consumed APIs'], relationsTitle: 'Relations', -} + nodeTestId: 'node', + gitopsSuffix: '-gitops', +} as constFollow-up: update plugin usages to reference DependenciesPO.nodeTestId and DependenciesPO.gitopsSuffix.
src/ui/common.ts (2)
18-31: Make waitForPageLoad deterministic; avoid snapshot waitsSnapshotting progress bars with .all() can miss spinners that appear slightly later. Prefer dynamic absence assertions and unify waits. Also assert heading exactness and wait for networkidle to reduce flakes.
export async function waitForPageLoad(page: Page, name: string) { - const progressBars = page.getByRole('progressbar'); - // Get all progressbar elements and wait until all are hidden - const bars = await progressBars.all(); - if (bars.length > 0) { - await Promise.all( - bars.map(bar => expect(bar).toBeHidden({ timeout: 10000 })) - ); - } - - await expect(page.getByTestId('sidebar-root')).toBeAttached({ timeout: 5000 }); - await expect(page.getByRole('heading', { name: name })).toBeVisible({ timeout: 20000 }); - await page.waitForLoadState(); + const progressBars = page.getByRole('progressbar'); + // Wait until no progress bars remain (handles appear/disappear cycles) + await expect(progressBars).toHaveCount(0, { timeout: 20000 }); + + await expect(page.getByTestId('sidebar-root')).toBeVisible({ timeout: 20000 }); + await expect(page.getByRole('heading', { name, exact: true })).toBeVisible({ timeout: 20000 }); + await page.waitForLoadState('networkidle'); }
33-36: Select tabs by role and assert activationTarget the tab role directly and assert aria-selected to confirm the click actually activated the tab. This reduces misclicks and improves reliability.
export async function openTab(page: Page, tabName: string) { - const tab = page.getByRole('tablist').getByText(tabName); - await tab.click(); + const tab = page.getByRole('tablist').getByRole('tab', { name: tabName, exact: true }); + await tab.click(); + await expect(tab).toHaveAttribute('aria-selected', 'true'); }src/ui/plugins/dependencies/dependenciesUiPlugin.ts (4)
14-18: Tighten heading match and add explicit return typeExact matching prevents accidental partial matches; explicit return types improve clarity.
- async checkAllBoxesPresent(page: Page) { - for (const title of DependenciesPO.titles) { - await expect(page.getByRole('heading', { name: title })).toBeVisible(); - } - } + async checkAllBoxesPresent(page: Page): Promise<void> { + for (const title of DependenciesPO.titles) { + await expect(page.getByRole('heading', { name: title, exact: true })).toBeVisible(); + } + }
20-22: Non-heading “Relations”: use exact text and return typeKeep using getByText (since it's not a heading), but add exactness and an explicit return type.
- async checkRelationsTitle(page: Page) { - await expect(page.getByText( DependenciesPO.relationsTitle )).toBeVisible(); - } + async checkRelationsTitle(page: Page): Promise<void> { + await expect(page.getByText(DependenciesPO.relationsTitle, { exact: true })).toBeVisible(); + }
24-28: Harden node locators; avoid hasText substring matchesUse the page object constants and exact text matching to minimize false positives. Add an explicit return type.
- async checkNodesPresent(page: Page) { - const componentNode = page.getByTestId('node').filter({ has: page.getByText(this.componentName, { exact: true }) }); - await expect(componentNode).toBeVisible(); - await expect(page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` })).toBeVisible(); - } + async checkNodesPresent(page: Page): Promise<void> { + const componentNode = page + .getByTestId(DependenciesPO.nodeTestId) + .filter({ has: page.getByText(this.componentName, { exact: true }) }); + await expect(componentNode).toBeVisible(); + // Optionally enforce uniqueness: + // await expect(componentNode).toHaveCount(1); + + const gitopsNode = page + .getByTestId(DependenciesPO.nodeTestId) + .filter({ has: page.getByText(`${this.componentName}${DependenciesPO.gitopsSuffix}`, { exact: true }) }); + await expect(gitopsNode).toBeVisible(); + // Optionally enforce uniqueness: + // await expect(gitopsNode).toHaveCount(1); + }
30-35: Stabilize navigation to GitOps dependency and align with PO constantsUse exact text via has + getByText, return type, and PO constants. Keeping waitForPageLoad is good for determinism.
- async goToGitopsDependency(page: Page) { - const nodeLocator = page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` }); - await expect(nodeLocator).toBeVisible(); - await nodeLocator.click(); - await waitForPageLoad(page, `${this.componentName}-gitops`); - } + async goToGitopsDependency(page: Page): Promise<void> { + const nodeLocator = page + .getByTestId(DependenciesPO.nodeTestId) + .filter({ has: page.getByText(`${this.componentName}${DependenciesPO.gitopsSuffix}`, { exact: true }) }); + await expect(nodeLocator).toBeVisible(); + await nodeLocator.click(); + await waitForPageLoad(page, `${this.componentName}${DependenciesPO.gitopsSuffix}`); + }tests/ui/ui.test.ts (6)
4-5: Use a type-only import for DependenciesUiPlugin to keep the test runtime leanYou only use DependenciesUiPlugin as a type. Make it a type-only import to avoid pulling the module at runtime and reduce the risk of circular imports.
Apply this diff:
-import { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin'; +import type { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin';If you adopt the attribute-based assertions suggested below, ensure expect is in scope. If the fixtures don’t re-export it, import it explicitly:
+import { expect } from '@playwright/test';
67-69: Add a concrete CI assertion to avoid false positivesRight now the test only navigates. Assert the CI tab is active (or a known CI widget is present) so failures are actionable.
Apply this diff:
test('verify CI provider on CI tab', async ({ page }) => { await page.goto(`${component.getComponentUrl()}/ci`, { timeout: 20000 }); await waitForPageLoad(page, component.getCoreComponent().getName()); + await test.step('Assert CI tab is active', async () => { + const ciTab = page.getByRole('tab', { name: 'CI' }); + await expect(ciTab).toHaveAttribute('aria-selected', 'true'); + }); });Note: If expect is not already available via fixtures, add
import { expect } from '@playwright/test'to the imports.
145-147: Drop unnecessary await; getDependencies() is synchronousgetDependencies() returns a value, not a Promise. Make the hook synchronous for simplicity and slight speedup.
Apply this diff:
- test.beforeAll(async () => { - dependencies = await component.getDependencies(); - }); + test.beforeAll(() => { + dependencies = component.getDependencies(); + });
149-154: Fast-fail: assert Dependencies tab is active after navigationCatches routing issues early and makes failures more diagnosable.
Apply this diff:
test('test dependency', async ({ page }) => { await page.goto(`${component.getComponentUrl()}/dependencies`, { timeout: 20000, }); await waitForPageLoad(page, component.getCoreComponent().getName()); + await test.step('Assert Dependencies tab is active', async () => { + const depTab = page.getByRole('tab', { name: 'Dependencies' }); + await expect(depTab).toHaveAttribute('aria-selected', 'true'); + });
155-164: Add a quick page-heading assertion before deep checksFail fast if navigation landed on the wrong subpage.
Apply this diff:
- await test.step('Check all boxes', async () => { + await test.step('Assert Dependencies heading', async () => { + await page.getByRole('heading', { name: 'Dependencies' }) + .waitFor({ state: 'visible', timeout: 20000 }); + }); + await test.step('Check all boxes', async () => { await dependencies.checkAllBoxesPresent(page); }, {timeout: 30000});This version uses locator.waitFor to avoid introducing a new expect import.
175-177: Replace placeholder with a real GitOps CI assertionPlaceholders mask missing coverage. Either remove or implement a minimal, stable check.
Apply this diff:
- await test.step('Check Gitops CI tab', async () => { - // TBD - }); + await test.step('Check GitOps CI tab is active', async () => { + await openTab(page, 'CI'); + await waitForPageLoad(page, `${component.getCoreComponent().getName()}-gitops`); + const ciTab = page.getByRole('tab', { name: 'CI' }); + await expect(ciTab).toHaveAttribute('aria-selected', 'true'); + });Note: Add
import { expect } from '@playwright/test'to the imports if not already available via fixtures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (6)
playwright.config.ts(1 hunks)src/ui/common.ts(1 hunks)src/ui/page-objects/dependencies_po.ts(1 hunks)src/ui/plugins/dependencies/dependenciesUiPlugin.ts(1 hunks)src/ui/uiComponent.ts(7 hunks)tests/ui/ui.test.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
UI entity class names should include 'Ui' or 'plugin' to distinguish them from backend entities
Files:
src/ui/common.tssrc/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.ts
{tests/ui/**/*.ts,src/ui/**/*.ts}
📄 CodeRabbit inference engine (README.md)
When entering secrets in UI flows, blur the input fields before typing to avoid exposing values in screenshots
Files:
src/ui/common.tssrc/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.tstests/ui/ui.test.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/common.tssrc/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.ts
src/ui/page-objects/**/*_po.ts
📄 CodeRabbit inference engine (README.md)
Each page object identifier file must be placed under src/ui/page-objects and have a _po.ts suffix
Files:
src/ui/page-objects/dependencies_po.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Classes in src/ui/plugins must include either 'Ui' or 'plugin' in their class names
Plugin file names in src/ui/plugins should match the short name of the plugin
Files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (README.md)
Never print secrets in test logs
Files:
tests/ui/ui.test.ts
tests/**
⚙️ CodeRabbit configuration file
Focus on test coverage, maintainability, proper test structure, test isolation, mock usage, async test patterns, and test stability. Ensure tests are deterministic and properly clean up after themselves.
Files:
tests/ui/ui.test.ts
**/*.test.ts
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper assertions, test isolation, async pattern correctness, mock usage, and test maintainability. Review for potential flaky tests and race conditions.
Files:
tests/ui/ui.test.ts
🧠 Learnings (8)
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to src/ui/page-objects/**/*_po.ts : Each page object identifier file must be placed under src/ui/page-objects and have a _po.ts suffix
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T09:14:32.201Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: src/ui/plugins/dependencies/dependenciesUiPlugin.ts:26-28
Timestamp: 2025-08-21T09:14:32.201Z
Learning: In the dependencies UI page object for TSSC E2E testing framework, the Relations title (DependenciesPO.relationsTitle) is not a heading element, so getByRole('heading') should not be used for this element.
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to src/ui/plugins/**/*.ts : Classes in src/ui/plugins must include either 'Ui' or 'plugin' in their class names
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.tssrc/ui/uiComponent.ts
📚 Learning: 2025-08-19T11:28:24.837Z
Learnt from: jsmid1
PR: redhat-appstudio/tssc-test#101
File: src/ui/plugins/docs/docsUiPlugin.ts:26-28
Timestamp: 2025-08-19T11:28:24.837Z
Learning: The HTML `<strong>` element has an implicit ARIA role of `strong`, making `page.getByRole('strong')` a valid selector in Playwright tests for TSSC E2E testing framework.
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T08:57:44.934Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.934Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Set logging level lower than TRACE during tests to avoid capturing sensitive data (e.g., in network requests)
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to tests/**/*.test.ts : Never print secrets in test logs
Applied to files:
playwright.config.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
PR: redhat-appstudio/tssc-test#72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
tests/ui/ui.test.ts
🧬 Code graph analysis (3)
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (2)
src/ui/page-objects/dependencies_po.ts (1)
DependenciesPO(6-9)src/ui/common.ts (1)
waitForPageLoad(18-31)
src/ui/uiComponent.ts (2)
src/rhtap/core/component.ts (1)
Component(14-253)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
tests/ui/ui.test.ts (3)
src/ui/uiComponent.ts (1)
UiComponent(24-136)src/ui/common.ts (2)
waitForPageLoad(18-31)openTab(33-36)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
⏰ 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: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (8)
playwright.config.ts (1)
107-110: Global artifacts-on-failure: LGTM and aligned with security stanceGood call setting screenshots/videos only on failure at the global use level. This also respects the no-trace-in-CI guidance to avoid leaking sensitive information in traces.
src/ui/uiComponent.ts (3)
29-37: Wiring DependenciesUiPlugin into UiComponent: looks goodThe new private field, constructor injection, and assignment are consistent with existing plugin patterns and keep concerns separated.
72-74: Factory integration of DependenciesUiPlugin: good instantiation pointConstructing DependenciesUiPlugin in UiComponent.new mirrors how other UI plugins are created. Keeps composition centralized and predictable.
133-135: Getter for Dependencies plugin is consistent and documentedPublic accessor matches style of other getters and the JSDoc clarifies its purpose. No issues.
tests/ui/ui.test.ts (4)
58-60: Good: standardized on waitForPageLoad after navigationThis reduces flakiness by relying on a single readiness heuristic across tests.
80-89: Docs flow readiness looks solidAdding waitForPageLoad and splitting assertions into scoped test.steps improves stability and debuggability.
179-184: Good: GitOps Docs flow uses tab navigation plus readiness waitConsistent with the rest of the suite and lowers flake risk.
30-33: Redact full object logging in UI testsI’ve verified that the only place we’re printing a full fixture object is in tests/ui/ui.test.ts. Please update that call to log only the component name:
- Location: tests/ui/ui.test.ts:30
- console.log('Running UI test for:', testItem); + // Avoid logging the full fixture object to prevent secret leakage + console.log(`Running UI test for: ${testItem.getName()}`);I ran a scan across the codebase for console statements containing sensitive keywords; no other test files log full objects or secret values. The remaining logs in src/api/… only emit literal messages or resource names (not secret contents), so they don’t pose a leakage risk.
| use: { | ||
| screenshot: 'only-on-failure', | ||
| video: 'retain-on-failure', | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Be explicit about disabling Playwright traces
To guard against accidental trace enablement via overrides, explicitly set trace to 'off'. This keeps CI safe while still allowing local devs to override via CLI when needed.
use: {
screenshot: 'only-on-failure',
video: 'retain-on-failure',
+ trace: 'off',
},📝 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.
| use: { | |
| screenshot: 'only-on-failure', | |
| video: 'retain-on-failure', | |
| }, | |
| use: { | |
| screenshot: 'only-on-failure', | |
| video: 'retain-on-failure', | |
| trace: 'off', | |
| }, |
🤖 Prompt for AI Agents
In playwright.config.ts around lines 107 to 110, the Playwright "use"
configuration sets screenshot and video options but does not explicitly disable
traces; add trace: 'off' to the use block to ensure traces are disabled by
default (preventing accidental CI trace collection) while still allowing local
overrides via CLI.
| export class DependenciesUiPlugin { | ||
| private readonly componentName: string; | ||
|
|
||
| constructor( | ||
| componentName: string, | ||
| ) { | ||
| this.componentName = componentName; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use parameter property to reduce boilerplate
You can declare and assign componentName in the constructor signature to simplify the class.
-export class DependenciesUiPlugin {
- private readonly componentName: string;
-
- constructor(
- componentName: string,
- ) {
- this.componentName = componentName;
- }
+export class DependenciesUiPlugin {
+ constructor(private readonly componentName: string) {}📝 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.
| export class DependenciesUiPlugin { | |
| private readonly componentName: string; | |
| constructor( | |
| componentName: string, | |
| ) { | |
| this.componentName = componentName; | |
| } | |
| export class DependenciesUiPlugin { | |
| constructor(private readonly componentName: string) {} | |
| // …rest of class methods… | |
| } |
🤖 Prompt for AI Agents
In src/ui/plugins/dependencies/dependenciesUiPlugin.ts around lines 5 to 12, the
class declares a private readonly componentName field and then assigns it in the
constructor; replace that boilerplate by using a TypeScript parameter property:
remove the explicit field declaration and change the constructor signature to
take the parameter as private readonly componentName: string so the property is
declared and assigned automatically (ensure any references continue to work).
| @@ -11,7 +13,7 @@ const test = createBasicFixture(); | |||
| * A complete test scenario for RHTAP UI plugins test: | |||
| * | |||
| * This test suite check the plugin in the UI and uses the component from backend e2e test. | |||
| * This test should not | |||
| * This test should not | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix incomplete doc sentence for clarity
The comment reads as unfinished. Make the intent explicit.
Apply this diff:
- * This test should not
+ * This test intentionally does not:📝 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.
| * This test should not | |
| * This test intentionally does not: |
🤖 Prompt for AI Agents
In tests/ui/ui.test.ts around line 16, the inline comment "This test should not"
is an incomplete sentence; replace it with a complete, explicit sentence that
clearly states the test's intent (for example: "This test should not modify
global state and must run in isolation" or "This test should not perform network
requests"), choosing wording that accurately reflects what the test must avoid;
update the comment to that full sentence so the intent is unambiguous.
tests/ui/ui.test.ts
Outdated
| await test.step('Check gitops git link', async () => { | ||
| // Skip test for not yet supported git providers | ||
| if (component.getGit() === undefined) { | ||
| console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); | ||
| test.skip(); | ||
| return; | ||
| } | ||
| await component.getGit()!.checkViewSourceLink(page); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid test.skip inside a nested step; guard the step instead
Calling test.skip mid-test marks the entire test as skipped after work has already run, which skews reporting. Guard the step and proceed without skipping the whole test.
Apply this diff:
- await test.step('Check gitops git link', async () => {
- // Skip test for not yet supported git providers
- if (component.getGit() === undefined) {
- console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`);
- test.skip();
- return;
- }
- await component.getGit()!.checkViewSourceLink(page);
- });
+ if (component.getGit()) {
+ await test.step('Check gitops "View Source" link', async () => {
+ await component.getGit()!.checkViewSourceLink(page);
+ });
+ } else {
+ console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`);
+ }📝 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.
| await test.step('Check gitops git link', async () => { | |
| // Skip test for not yet supported git providers | |
| if (component.getGit() === undefined) { | |
| console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); | |
| test.skip(); | |
| return; | |
| } | |
| await component.getGit()!.checkViewSourceLink(page); | |
| }); | |
| if (component.getGit()) { | |
| await test.step('Check gitops "View Source" link', async () => { | |
| await component.getGit()!.checkViewSourceLink(page); | |
| }); | |
| } else { | |
| console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); | |
| } |
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 (3)
src/ui/common.ts (1)
27-39: Tighten Quick Start dismissal and avoid false clicksMinor stability improvement: ensure the Hide button is matched exactly before clicking to avoid text collisions.
Apply this diff:
- const hideButton = page.getByRole('button', { name: 'Hide' }); + const hideButton = page.getByRole('button', { name: 'Hide', exact: true });tests/ui/ui.test.ts (2)
40-47: Prefer waitForPageLoad for home page readiness to standardize load checksUnifies page-load behavior and reduces flakiness by using the shared helper.
test('open developer hub', async ({ page }) => { await page.goto(component.getCoreComponent().getDeveloperHub().getUrl(), { timeout: 20000, }); - await page - .getByRole('heading', { name: CommonPO.welcomeTitle }) - .waitFor({ state: 'visible', timeout: 20000 }); + await waitForPageLoad(page, CommonPO.welcomeTitle); });
31-37: Remove sensitive logging and clarify setup comment in UI testsThe grep scan confirms there are two places in tests/ui/ui.test.ts where we log the full testItem object (which can expose secrets) and a generic comment that could be clearer about what’s happening. Let’s update both to improve security, clarity, and maintainability.
Affected location:
- tests/ui/ui.test.ts, around lines 31–37
Suggested changes:
- console.log('Running UI test for:', testItem); const componentName = testItem.getName(); - console.log(`Creating component: ${componentName}`); + console.log(`Starting UI test for component: ${componentName}`); - // Assign the already created component + // Initialize the UiComponent wrapper for the existing backend component (we’re not creating a new backend here) component = await UiComponent.new(componentName, testItem, componentName);• We've removed the first log that prints out testItem to avoid leaking any sensitive fields.
• We’ve updated the second log to reference only componentName for clearer, non-sensitive output.
• The comment now explicitly states that we’re wrapping an existing backend component, improving onboarding clarity.
♻️ Duplicate comments (15)
playwright.config.ts (1)
107-110: Explicitly disable Playwright traces to avoid accidental leakage in CIGiven prior guidance not to retain traces in CI, set trace: 'off' to guard against overrides in child projects or env-based merges.
Apply this diff:
use: { screenshot: 'only-on-failure', video: 'retain-on-failure', + trace: 'off', },src/ui/page-objects/dependencies_po.ts (1)
6-9: Centralize frequently used selectors and suffixes to reduce brittlenessThe plugin hard-codes testId 'node' and the '-gitops' suffix. Exporting them here improves maintainability and keeps selector drift in one place.
Apply this diff:
export const DependenciesPO = { titles: ['Depends on components', 'Depends on resources', 'Has subcomponents', 'Provided APIs', 'Consumed APIs'], relationsTitle: 'Relations', + nodeTestId: 'node', + gitopsSuffix: '-gitops', }src/ui/common.ts (2)
41-54: Avoid snapshotting progress bars; wait for dynamic absence and network idleSnapshotting locators with .all() can miss late-appearing spinners. Prefer dynamic absence and a deterministic networkidle wait.
Apply this diff:
export async function waitForPageLoad(page: Page, name: string) { - const progressBars = page.getByRole('progressbar'); - // Get all progressbar elements and wait until all are hidden - const bars = await progressBars.all(); - if (bars.length > 0) { - await Promise.all( - bars.map(bar => expect(bar).toBeHidden({ timeout: 10000 })) - ); - } - - await expect(page.getByTestId('sidebar-root')).toBeAttached({ timeout: 5000 }); - await expect(page.getByRole('heading', { name: name })).toBeVisible({ timeout: 20000 }); - await page.waitForLoadState(); + const progressBars = page.getByRole('progressbar'); + // Wait until no progress bars remain (handles appear/disappear cycles) + await expect(progressBars).toHaveCount(0, { timeout: 20000 }); + + await expect(page.getByTestId('sidebar-root')).toBeVisible({ timeout: 20000 }); + await expect(page.getByRole('heading', { name, exact: true })).toBeVisible({ timeout: 20000 }); + await page.waitForLoadState('networkidle'); }
56-59: Select tabs by role and assert activation to prevent misclicksTarget role='tab' with an exact name and verify aria-selected after the click to reduce flakiness.
Apply this diff:
-export async function openTab(page: Page, tabName: string) { - const tab = page.getByRole('tablist').getByText(tabName); - await tab.click(); -} +export async function openTab(page: Page, tabName: string): Promise<void> { + const tab = page.getByRole('tablist').getByRole('tab', { name: tabName, exact: true }); + await tab.click(); + await expect(tab).toHaveAttribute('aria-selected', 'true'); +}src/ui/plugins/dependencies/dependenciesUiPlugin.ts (4)
20-22: Relations title is not a heading; ensure exact text match and add return typeAvoid role-based selection here, and prefer exact text matching.
Apply this diff:
- async checkRelationsTitle(page: Page) { - await expect(page.getByText( DependenciesPO.relationsTitle )).toBeVisible(); - } + async checkRelationsTitle(page: Page): Promise<void> { + await expect(page.getByText(DependenciesPO.relationsTitle, { exact: true })).toBeVisible(); + }
5-12: Use a parameter property to reduce boilerplateSimplify by declaring componentName in the constructor parameter list.
Apply this diff:
-export class DependenciesUiPlugin { - private readonly componentName: string; - - constructor( - componentName: string, - ) { - this.componentName = componentName; - } +export class DependenciesUiPlugin { + constructor(private readonly componentName: string) {}
14-18: Make heading matches exact and declare return typeExact heading matching reduces accidental partial matches; explicit return type aids readability.
Apply this diff:
- async checkAllBoxesPresent(page: Page) { + async checkAllBoxesPresent(page: Page): Promise<void> { for (const title of DependenciesPO.titles) { - await expect(page.getByRole('heading', { name: title })).toBeVisible(); + await expect(page.getByRole('heading', { name: title, exact: true })).toBeVisible(); } }
24-28: Tighten node locators and avoid hasText; reference selectors from the POUse has: getByText(..., { exact: true }) for precision and centralize 'node' and '-gitops' via DependenciesPO. Also unify quotes.
Apply this diff:
- async checkNodesPresent(page: Page) { - const componentNode = page.getByTestId('node').filter({ has: page.getByText(this.componentName, { exact: true }) }); - await expect(componentNode).toBeVisible(); - await expect(page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` })).toBeVisible(); - } + async checkNodesPresent(page: Page): Promise<void> { + const componentNode = page + .getByTestId(DependenciesPO.nodeTestId) + .filter({ has: page.getByText(this.componentName, { exact: true }) }); + await expect(componentNode).toBeVisible(); + + const gitopsNode = page + .getByTestId(DependenciesPO.nodeTestId) + .filter({ has: page.getByText(`${this.componentName}${DependenciesPO.gitopsSuffix}`, { exact: true }) }); + await expect(gitopsNode).toBeVisible(); + }tests/ui/ui.test.ts (7)
17-17: Fix incomplete doc sentence for clarityMake the intent explicit to keep the suite’s contract unambiguous.
- * This test should not + * This test intentionally does not:
4-6: Consolidate common imports; use a type-only import for DependenciesUiPluginReduce runtime deps and circular import risk by erasing the plugin symbol at compile time; also merge the two common imports for maintainability.
-import { hideQuickStartIfVisible, openTab } from '../../src/ui/common'; -import { waitForPageLoad } from '../../src/ui/common'; -import { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin'; +import { hideQuickStartIfVisible, openTab, waitForPageLoad } from '../../src/ui/common'; +import type { DependenciesUiPlugin } from '../../src/ui/plugins/dependencies/dependenciesUiPlugin';
68-70: Add a concrete assertion on the CI page to avoid false positivesRight now the test only navigates. Assert the CI tab is active (or a known CI widget is visible) to validate rendering.
test('verify CI provider on CI tab', async ({ page }) => { await page.goto(`${component.getComponentUrl()}/ci`, { timeout: 20000 }); await waitForPageLoad(page, component.getCoreComponent().getName()); + await test.step('Assert CI tab is active', async () => { + const ciTab = page.getByRole('tab', { name: 'CI' }); + await expect(ciTab).toHaveAttribute('aria-selected', 'true'); + }); });
149-155: Drop unnecessary await and async in beforeAll when fetching Dependencies plugingetDependencies() returns a value, not a Promise. Keep the hook synchronous.
- test.beforeAll(async () => { - dependencies = await component.getDependencies(); - }); + test.beforeAll(() => { + dependencies = component.getDependencies(); + });
156-161: Fast-fail by asserting the Dependencies tab is active after navigationCatches routing issues early and tightens the signal.
test('test dependency', async ({ page }) => { await page.goto(`${component.getComponentUrl()}/dependencies`, { timeout: 20000, }); await waitForPageLoad(page, component.getCoreComponent().getName()); + await test.step('Assert Dependencies tab is active', async () => { + const depTab = page.getByRole('tab', { name: 'Dependencies' }); + await expect(depTab).toHaveAttribute('aria-selected', 'true'); + });
172-181: Avoid test.skip inside a nested step; guard the step insteadSkipping mid-test after prior work skews reporting and can hide partial execution. Guard the step with an if and only run it when supported.
- await test.step('Check gitops git link', async () => { - // Skip test for not yet supported git providers - if (component.getGit() === undefined) { - console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); - test.skip(); - return; - } - await component.getGit()!.checkViewSourceLink(page); - }); + if (component.getGit()) { + await test.step('Check gitops "View Source" link', async () => { + await component.getGit()!.checkViewSourceLink(page); + }); + } else { + console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); + }
182-185: Replace placeholder with a real CI assertion (or remove it)A no-op step always passes and weakens coverage.
- await test.step('Check Gitops CI tab', async () => { - // TBD - }); + await test.step('Check GitOps CI tab is active', async () => { + await openTab(page, 'CI'); + const ciTab = page.getByRole('tab', { name: 'CI' }); + await expect(ciTab).toHaveAttribute('aria-selected', 'true'); + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (6)
playwright.config.ts(1 hunks)src/ui/common.ts(2 hunks)src/ui/page-objects/dependencies_po.ts(1 hunks)src/ui/plugins/dependencies/dependenciesUiPlugin.ts(1 hunks)src/ui/uiComponent.ts(7 hunks)tests/ui/ui.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/ui/page-objects/**/*_po.ts
📄 CodeRabbit inference engine (README.md)
Each page object identifier file must be placed under src/ui/page-objects and have a _po.ts suffix
Files:
src/ui/page-objects/dependencies_po.ts
src/ui/**/*.ts
📄 CodeRabbit inference engine (README.md)
UI entity class names should include 'Ui' or 'plugin' to distinguish them from backend entities
Files:
src/ui/page-objects/dependencies_po.tssrc/ui/common.tssrc/ui/uiComponent.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
{tests/ui/**/*.ts,src/ui/**/*.ts}
📄 CodeRabbit inference engine (README.md)
When entering secrets in UI flows, blur the input fields before typing to avoid exposing values in screenshots
Files:
src/ui/page-objects/dependencies_po.tssrc/ui/common.tssrc/ui/uiComponent.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.tstests/ui/ui.test.ts
src/ui/**
⚙️ CodeRabbit configuration file
Focus on page object pattern implementation, element selection strategies, UI test stability, maintainability, proper wait conditions, error handling, and flaky test prevention. Ensure tests are resilient to UI changes.
Files:
src/ui/page-objects/dependencies_po.tssrc/ui/common.tssrc/ui/uiComponent.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
src/ui/plugins/**/*.ts
📄 CodeRabbit inference engine (README.md)
src/ui/plugins/**/*.ts: Classes in src/ui/plugins must include either 'Ui' or 'plugin' in their class names
Plugin file names in src/ui/plugins should match the short name of the plugin
Files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (README.md)
Never print secrets in test logs
Files:
tests/ui/ui.test.ts
tests/**
⚙️ CodeRabbit configuration file
Focus on test coverage, maintainability, proper test structure, test isolation, mock usage, async test patterns, and test stability. Ensure tests are deterministic and properly clean up after themselves.
Files:
tests/ui/ui.test.ts
**/*.test.ts
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper assertions, test isolation, async pattern correctness, mock usage, and test maintainability. Review for potential flaky tests and race conditions.
Files:
tests/ui/ui.test.ts
🧠 Learnings (8)
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to src/ui/page-objects/**/*_po.ts : Each page object identifier file must be placed under src/ui/page-objects and have a _po.ts suffix
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T09:14:32.201Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: src/ui/plugins/dependencies/dependenciesUiPlugin.ts:26-28
Timestamp: 2025-08-21T09:14:32.201Z
Learning: In the dependencies UI page object for TSSC E2E testing framework, the Relations title (DependenciesPO.relationsTitle) is not a heading element, so getByRole('heading') should not be used for this element.
Applied to files:
src/ui/page-objects/dependencies_po.tssrc/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-08-21T08:57:44.934Z
Learnt from: Katka92
PR: redhat-appstudio/tssc-test#119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.934Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Set logging level lower than TRACE during tests to avoid capturing sensitive data (e.g., in network requests)
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to tests/**/*.test.ts : Never print secrets in test logs
Applied to files:
playwright.config.ts
📚 Learning: 2025-08-25T11:24:45.518Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-25T11:24:45.518Z
Learning: Applies to src/ui/plugins/**/*.ts : Classes in src/ui/plugins must include either 'Ui' or 'plugin' in their class names
Applied to files:
src/ui/uiComponent.ts
📚 Learning: 2025-08-19T11:28:24.837Z
Learnt from: jsmid1
PR: redhat-appstudio/tssc-test#101
File: src/ui/plugins/docs/docsUiPlugin.ts:26-28
Timestamp: 2025-08-19T11:28:24.837Z
Learning: The HTML `<strong>` element has an implicit ARIA role of `strong`, making `page.getByRole('strong')` a valid selector in Playwright tests for TSSC E2E testing framework.
Applied to files:
src/ui/plugins/dependencies/dependenciesUiPlugin.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
PR: redhat-appstudio/tssc-test#72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
tests/ui/ui.test.ts
🧬 Code graph analysis (3)
src/ui/uiComponent.ts (2)
src/rhtap/core/component.ts (1)
Component(14-253)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
src/ui/plugins/dependencies/dependenciesUiPlugin.ts (2)
src/ui/page-objects/dependencies_po.ts (1)
DependenciesPO(6-9)src/ui/common.ts (1)
waitForPageLoad(41-54)
tests/ui/ui.test.ts (3)
src/ui/uiComponent.ts (1)
UiComponent(24-136)src/ui/common.ts (3)
waitForPageLoad(41-54)hideQuickStartIfVisible(22-39)openTab(56-59)src/ui/plugins/dependencies/dependenciesUiPlugin.ts (1)
DependenciesUiPlugin(5-36)
⏰ 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: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (6)
playwright.config.ts (1)
107-110: Correct placement for artifacts configuration under useGood move centralizing screenshot/video defaults at the global use level. This ensures consistent artifact retention across all projects without duplications or ignored keys.
src/ui/uiComponent.ts (2)
133-135: Getter for Dependencies plugin looks goodPublic getter is consistent with other accessors and supports the new workflow cleanly.
72-74: Factory wiring is soundInstantiating DependenciesUiPlugin with the component name and threading it through the UiComponent constructor is straightforward and low-coupling.
tests/ui/ui.test.ts (3)
59-61: LGTM: Consistent use of waitForPageLoad on Overview navigationThis aligns with the unified page-load strategy and should reduce flakiness.
81-82: LGTM: Added waitForPageLoad before Docs assertionsGood call; this will mitigate race conditions with Docs rendering.
186-191: LGTM: GitOps docs validation is robustOpening the Docs tab, waiting for page readiness with the GitOps name, and asserting content via the plugin is a stable pattern.
| async goToGitopsDependency(page: Page) { | ||
| const nodeLocator = page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` }); | ||
| await expect(nodeLocator).toBeVisible(); | ||
| await nodeLocator.click(); | ||
| await waitForPageLoad(page, `${this.componentName}-gitops`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Stabilize navigation to GitOps dependency and reuse centralized selectors
Reuse the stricter locator and page-object constants; keep the deterministic wait you added.
Apply this diff:
- async goToGitopsDependency(page: Page) {
- const nodeLocator = page.getByTestId("node").filter({ hasText: `${this.componentName}-gitops` });
- await expect(nodeLocator).toBeVisible();
- await nodeLocator.click();
- await waitForPageLoad(page, `${this.componentName}-gitops`);
- }
+ async goToGitopsDependency(page: Page): Promise<void> {
+ const nodeLocator = page
+ .getByTestId(DependenciesPO.nodeTestId)
+ .filter({ has: page.getByText(`${this.componentName}${DependenciesPO.gitopsSuffix}`, { exact: true }) });
+ await expect(nodeLocator).toBeVisible();
+ await nodeLocator.click();
+ await waitForPageLoad(page, `${this.componentName}${DependenciesPO.gitopsSuffix}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/ui/plugins/dependencies/dependenciesUiPlugin.ts around lines 30 to 35,
replace the ad-hoc locator with the centralized, stricter page-object selector
constant (use the shared node test-id/selector from the page object module and a
dedicated "gitops" item text constant) so the locator is deterministic and
reusable; keep the await expect(...).toBeVisible(), await click(), and the
existing waitForPageLoad(page, `${this.componentName}-gitops`) call to maintain
the deterministic wait behavior; ensure you import the selector/constant from
the page-object module and update the locator usage to reference those constants
instead of building the selector inline.
| private dependencies!: DependenciesUiPlugin; | ||
|
|
||
| private constructor(component: Component, git: GitPlugin | undefined, docs: DocsUiPlugin, registry: RegistryPlugin | undefined) { | ||
| private constructor(component: Component, git: GitPlugin | undefined, docs: DocsUiPlugin, registry: RegistryPlugin | undefined, dependencies: DependenciesUiPlugin) { | ||
| this.component = component; | ||
| this.git = git; | ||
| this.docs = docs; | ||
| this.registry = registry; | ||
| this.dependencies = dependencies; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Drop definite assignment (!) and make dependency field readonly
The field is always initialized via the constructor. Prefer a readonly declaration and remove the definite assignment assertion for stronger typing.
Apply this diff:
- private dependencies!: DependenciesUiPlugin;
+ private readonly dependencies: DependenciesUiPlugin;📝 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.
| private dependencies!: DependenciesUiPlugin; | |
| private constructor(component: Component, git: GitPlugin | undefined, docs: DocsUiPlugin, registry: RegistryPlugin | undefined) { | |
| private constructor(component: Component, git: GitPlugin | undefined, docs: DocsUiPlugin, registry: RegistryPlugin | undefined, dependencies: DependenciesUiPlugin) { | |
| this.component = component; | |
| this.git = git; | |
| this.docs = docs; | |
| this.registry = registry; | |
| this.dependencies = dependencies; | |
| } | |
| private readonly dependencies: DependenciesUiPlugin; | |
| private constructor( | |
| component: Component, | |
| git: GitPlugin | undefined, | |
| docs: DocsUiPlugin, | |
| registry: RegistryPlugin | undefined, | |
| dependencies: DependenciesUiPlugin | |
| ) { | |
| this.component = component; | |
| this.git = git; | |
| this.docs = docs; | |
| this.registry = registry; | |
| this.dependencies = dependencies; | |
| } |
🤖 Prompt for AI Agents
In src/ui/uiComponent.ts around lines 29 to 37, the private field dependencies
uses a definite assignment assertion (!) but is always set in the constructor;
remove the '!' and mark the field as readonly (private readonly dependencies:
DependenciesUiPlugin) so the compiler knows it is initialized and immutable, and
keep the constructor assignment as-is to satisfy initialization.
| public getComponentUrl(): string { | ||
| const developerHubUrl = this.component.getDeveloperHub().getUrl(); | ||
| const componentName = this.component.getName(); | ||
| return `${developerHubUrl}/catalog/default/component/${componentName}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL-encode component names to handle special characters
If component names contain spaces or special characters, the current URL can break. Encode the segment.
Apply this diff:
public getComponentUrl(): string {
const developerHubUrl = this.component.getDeveloperHub().getUrl();
const componentName = this.component.getName();
- return `${developerHubUrl}/catalog/default/component/${componentName}`;
+ return `${developerHubUrl}/catalog/default/component/${encodeURIComponent(componentName)}`;
}📝 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.
| public getComponentUrl(): string { | |
| const developerHubUrl = this.component.getDeveloperHub().getUrl(); | |
| const componentName = this.component.getName(); | |
| return `${developerHubUrl}/catalog/default/component/${componentName}`; | |
| } | |
| public getComponentUrl(): string { | |
| const developerHubUrl = this.component.getDeveloperHub().getUrl(); | |
| const componentName = this.component.getName(); | |
| return `${developerHubUrl}/catalog/default/component/${encodeURIComponent(componentName)}`; | |
| } |
🤖 Prompt for AI Agents
In src/ui/uiComponent.ts around lines 121 to 125, the component name is inserted
into the URL without encoding which breaks URLs when names contain spaces or
special characters; update the method to URL-encode the componentName (e.g. use
encodeURIComponent on the name) before composing the URL so the returned string
safely encodes any unsafe characters, keeping the rest of the path unchanged.
|
/retest |
tests/ui/ui.test.ts
Outdated
| await test.step('Check gitops git link', async () => { | ||
| // Skip test for not yet supported git providers | ||
| if (component.getGit() === undefined) { | ||
| console.warn(`Skipping Git test as testing ${component.getCoreComponent().getGit().getGitType()} is not supported`); | ||
| test.skip(); | ||
| return; | ||
| } | ||
| await component.getGit()!.checkViewSourceLink(page); | ||
| }); |
There was a problem hiding this comment.
Could we move this step under Verify Git describe section?
|
@Katka92: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-rcpb7Test results analysis<not enabled> |
Test dependencies tab.
Add screenshots and videos to artifacts if a test fails.
Create a common
waitForPageLoadmethod and use it across all UI tests when a page is loaded (page.goto()).Summary by CodeRabbit
Tests
Chores
Style