OCPBUGS-82505: Re-enable add-flow-ci.feature e2e tests disabled for createRoot adoption#16285
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jhadvig: This pull request references Jira Issue OCPBUGS-82505, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
component/dev-console
📝 WalkthroughWalkthroughThis pull request refactors the catalog filtering logic to improve rendering performance using React's concurrent rendering features. The search keyword update is now wrapped in 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsx (1)
202-264:⚠️ Potential issue | 🟠 MajorRemove debug logging before merge — this will execute in production.
The
console.logandconsole.tablecalls within this memo will fire in production whenever a user applies search keywords or attribute filters. This:
- Exposes internal implementation details (relevance scoring algorithm, Red Hat priority logic)
- Leaks catalog metadata to the browser console (provider names, item types)
- Adds performance overhead — building
tableDataarray and serializing to console on every filter actionIf this instrumentation is needed for the e2e test debugging referenced in the PR, consider gating it behind a
process.env.NODE_ENV === 'development'check or a feature flag. Better yet, remove it entirely and rely on React DevTools or dedicated test logging.🔧 Proposed fix to remove debug logging
const filteredItems: CatalogItem[] = useMemo(() => { const filteredByAttributes = filterByAttributes(filteredBySearchItems, activeFilters); - - // Console table for final filtered results (only for operators) - if (filteredByAttributes.length > 0) { - // Check if we have active filters beyond just search and category - const hasAttributeFilters = Object.values(activeFilters).some((filterGroup) => - Object.values(filterGroup).some((filter) => filter.active), - ); - - // Only show console.table if we have search term or attribute filters - if (activeSearchKeyword || hasAttributeFilters) { - const REDHAT_PRIORITY = { - EXACT_MATCH: 2, - CONTAINS_REDHAT: 1, - NON_REDHAT: 0, - }; - - const tableData = filteredByAttributes.map((item) => { - // Ensure we have the scoring properties, calculate them if missing - const relevanceScore = activeSearchKeyword - ? (item as any).relevanceScore ?? - calculateCatalogItemRelevanceScore(activeSearchKeyword, item) - : 'N/A (No search)'; - const redHatPriority = (item as any).redHatPriority ?? getRedHatPriority(item); - - return { - Title: item.name || 'N/A', - 'Search Relevance Score': relevanceScore, - 'Is Red Hat Provider (Priority)': - redHatPriority === REDHAT_PRIORITY.EXACT_MATCH - ? `Exact Match (${REDHAT_PRIORITY.EXACT_MATCH})` - : redHatPriority === REDHAT_PRIORITY.CONTAINS_REDHAT - ? `Contains Red Hat (${REDHAT_PRIORITY.CONTAINS_REDHAT})` - : `Non-Red Hat (${REDHAT_PRIORITY.NON_REDHAT})`, - Provider: item.attributes?.provider || item.provider || 'N/A', - Type: item.type || 'N/A', - }; - }); - - // Build filter description - const activeFilterDescriptions = []; - if (activeSearchKeyword) activeFilterDescriptions.push(`Search: "${activeSearchKeyword}"`); - if (activeCategoryId !== 'all') - activeFilterDescriptions.push(`Category: ${activeCategoryId}`); - - Object.entries(activeFilters).forEach(([filterType, filterGroup]) => { - const activeFilterValues = Object.entries(filterGroup) - .filter(([, filter]) => filter.active) - .map(([, filter]) => filter.label || filter.value); - if (activeFilterValues.length > 0) { - activeFilterDescriptions.push(`${filterType}: [${activeFilterValues.join(', ')}]`); - } - }); - - const filterDescription = - activeFilterDescriptions.length > 0 ? activeFilterDescriptions.join(' + ') : 'No filters'; - - // eslint-disable-next-line no-console - console.log( - `\n🎯 FINAL Catalog Results: ${filterDescription} (${filteredByAttributes.length} matches)`, - ); - // eslint-disable-next-line no-console - console.table(tableData); - } - } - return filteredByAttributes; - }, [activeCategoryId, activeFilters, activeSearchKeyword, filteredBySearchItems]); + }, [activeFilters, filteredBySearchItems]);Note: If the debug logging is removed,
activeCategoryIdandactiveSearchKeywordcan also be dropped from the dependency array since they're only used in the logging block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsx` around lines 202 - 264, Remove the production debug logging in the CatalogView memo: eliminate the console.log and console.table calls (and the tableData construction if only used for logging) inside the filteredByAttributes block, or gate them behind a dev-only check (e.g., process.env.NODE_ENV === 'development' or a feature flag) so they never run in production; update the CatalogView logic that builds tableData and the activeFilterDescriptions accordingly and, if you remove the logging and tableData entirely, also remove activeCategoryId and activeSearchKeyword from the memo dependency array and any related unused variables (references: CatalogView component, filteredByAttributes, activeSearchKeyword, activeFilters, activeCategoryId, tableData).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsx`:
- Around line 202-264: Remove the production debug logging in the CatalogView
memo: eliminate the console.log and console.table calls (and the tableData
construction if only used for logging) inside the filteredByAttributes block, or
gate them behind a dev-only check (e.g., process.env.NODE_ENV === 'development'
or a feature flag) so they never run in production; update the CatalogView logic
that builds tableData and the activeFilterDescriptions accordingly and, if you
remove the logging and tableData entirely, also remove activeCategoryId and
activeSearchKeyword from the memo dependency array and any related unused
variables (references: CatalogView component, filteredByAttributes,
activeSearchKeyword, activeFilters, activeCategoryId, tableData).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d74c5176-da75-4993-9bbc-d3bfb58cd086
📒 Files selected for processing (2)
frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsxfrontend/packages/dev-console/integration-tests/features/e2e/add-flow-ci.feature
📜 Review details
🧰 Additional context used
🔀 Multi-repo context openshift/console-operator
[::openshift/console-operator::] pkg/console/subresource/consoleserver/types.go: lines ~112-170 — definitions/comments for developerCatalog / CatalogTypesState / DeveloperConsoleCatalogTypes (server-side console config that controls shown developer catalog categories/types).
[::openshift/console-operator::] pkg/console/subresource/consoleserver/config_builder_test.go: multiple tests (see hits at ~lines 363, 396, 435, 516, 556 and duplicates at ~1205, 1231, 1262, 1327, 1362) exercising config builder behavior for developer catalog categories/types.
[::openshift/console-operator::] vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yaml: lines ~158-247 — CRD manifests documenting developerCatalog fields (categories/types enabled/disabled).
[::openshift/console-operator::] vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go and vendor/github.com/openshift/api/console/v1/* — generated API docs referencing "developerCatalog", "catalog types", and sample/catalog-related fields.
Summary / impact:
- I did not find any frontend TypeScript/React consumers (e.g., CatalogView, CatalogQueryParams, add-flow-ci) in this repository. The console-operator repo contains server-side configuration and CRD artifacts for developerCatalog categories/types that influence what appears in the frontend catalog, but no direct code that would break from the CatalogView render/URL update changes in the PR.
- Relevant cross-repo artifacts: CRD docs and server config builder tests around developerCatalog that could affect catalog availability (categories/types), but they do not reference the frontend component APIs changed in this PR.
🔇 Additional comments (5)
frontend/packages/dev-console/integration-tests/features/e2e/add-flow-ci.feature (1)
1-1: Tag update correctly re-enables this feature in active suites.Switching to
@add-flow@smoke`` is aligned with the stated goal of bringing this E2E back into regular CI coverage.frontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsx (4)
2-2: LGTM — correct import for concurrent features.The
startTransitionAPI is appropriate for React 18's concurrent rendering model and aligns with the fix described in the PR objectives.
138-149: Good use ofstartTransitionfor input handling.Wrapping the URL update in
startTransitioncorrectly deprioritizes the navigation side-effect, preventing React from interrupting user input with a synchronous re-render. The inline comment provides solid rationale for future maintainers.This pattern ensures the SearchInput component remains stable during rapid typing—critical for both UX and e2e test stability under Cypress.
269-280: Sound architectural fix — note the async timing behavior.Moving
setFilterGroupCountsandsetCatalogTypeCountsout of the memo into auseEffectcorrectly addresses the anti-pattern of callingsetStateduring render. Under React 18's concurrent model, this was causing the DOM instability described in the PR.One subtlety: counts now update asynchronously after render, so there's a brief window where
catalogTypeCountsis{}. TheCatalogTypeSelectorcomponent (from context snippet 4) gates tab rendering ontypeCount > 0, meaning type tabs won't render during that first frame. In practice this is likely imperceptible, and the trade-off is acceptable given the concurrent rendering fix.If the flash becomes noticeable in testing,
useLayoutEffectwould run synchronously before paint—thoughuseEffectis the right default here.
194-197: Clean separation of filtering stages.Extracting the category + keyword filtering into
filteredBySearchItemsprovides a clear intermediate result that bothfilteredItems(for display) and the count computationuseEffectcan depend on. This avoids duplicating the filtering logic and makes the data flow explicit.
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
…reateRoot adoption Replace setState-inside-useMemo anti-pattern in CatalogView with derived useMemo values, preventing extra render cycles that detach DOM nodes during Cypress interactions. Wrap search keyword URL updates in startTransition, fix ConsoleSelect prop→state feedback loop, and use startTransition for ImageSearch field resets. Split .clear().type() chains in catalog and topology search helpers so Cypress re-queries the DOM between operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-82505, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test all |
|
wtf it passed?? /lgtm |
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test all |
|
@jhadvig: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jhadvig: Jira Issue Verification Checks: Jira Issue OCPBUGS-82505 Jira Issue OCPBUGS-82505 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.22 |
|
@logonoff: new pull request created: #16468 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Fix included in release 5.0.0-0.nightly-2026-05-20-101113 |
6 participants
Attempt to fix:
Summary by CodeRabbit
Release Notes
Refactor
Tests