CONSOLE-5427: Fix NS_BINDING_ABORTED noise, lifecycle cache coupling, and table row overlap#16773
CONSOLE-5427: Fix NS_BINDING_ABORTED noise, lifecycle cache coupling, and table row overlap#16773perdasilva wants to merge 7 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@perdasilva: This pull request explicitly references no jira issue. 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/olm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes distinguish aborted console fetches from other failures, revise lifecycle request caching and stale-update handling, add coverage for lifecycle hook states, and adjust lifecycle table column sizing and badge layout spacing. ChangesAbort handling and lifecycle caching
Lifecycle table rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant useOperatorLifecycle
participant lifecycleCache
participant coFetchJSON
useOperatorLifecycle->>lifecycleCache: request cache key
lifecycleCache->>coFetchJSON: fetch lifecycle data when no promise exists
coFetchJSON-->>lifecycleCache: resolve data or reject error
lifecycleCache-->>useOperatorLifecycle: return shared result
useOperatorLifecycle-->>useOperatorLifecycle: update state only while active
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
When useOperatorLifecycle re-runs (deps changed) while a fetch is in-flight, fetchLifecycleData returns the existing cached promise (which was created with the previous AbortController's signal). When the old controller is then aborted, that promise rejects with AbortError. The .catch handler only checked !controller.signal.aborted (the NEW controller, which is not aborted), so the AbortError was incorrectly surfaced as a real error, producing NS_BINDING_ERROR noise in the browser console. Fix: also skip setError when err.name === 'AbortError', while still calling setLoading(false) so the loading state settles correctly. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fix A — console-fetch.ts: Don't console.warn for AbortErrors. Aborted fetches are expected when AbortController.abort() is called (component unmount, dep change), not failures. The warn produced NS_BINDING_ABORTED noise in Firefox devtools. Fix B — useOperatorLifecycle.ts: When fetchLifecycleData returns a cached in-flight promise tied to a different caller's signal, chain a .catch() that detects if the original was aborted by someone else while our signal is still live and retries with a fresh request. Previously, the spurious AbortError would propagate to the hook's .catch and leave the row in an empty non-loading state. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
f74b63a to
4068438
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-prow-playwright-e2e.sh (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUsage comment doesn't list the new
feature-gatescenario.The script now accepts
feature-gateas a scenario (line 62), but the usage comment (line 10) still says[e2e|release|smoke]and the scenario list (lines 13-14) omits it.📝 Proposed fix
# ./test-prow-playwright-e2e.sh [e2e|release|smoke|feature-gate] [arguments passed to: playwright test ...] # # Scenarios (first argument; default: e2e): # e2e, release — full Playwright suite (default project / config) # smoke — Playwright smoke project (--project=smoke) +# feature-gate — Playwright tests tagged `@feature-gate` (--grep `@feature-gate`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-prow-playwright-e2e.sh` around lines 10 - 14, Update the usage documentation for test-prow-playwright-e2e.sh to include feature-gate in the accepted scenario list and describe its behavior alongside e2e, release, and smoke, matching the scenario handling implemented in the script.
🧹 Nitpick comments (2)
test-prow-e2e-techpreview.sh (1)
18-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
ARTIFACT_DIRvalidation for consistency with the Playwright techpreview script.The Playwright techpreview script (
test-prow-playwright-e2e-techpreview.sh, lines 22-30) validates thatARTIFACT_DIRis set, absolute, and not/. This script lacks that validation. Withset -u, an unsetARTIFACT_DIRfails cryptically at line 20; an empty or relative value would silently produce a brokenINSTALLER_DIR.🛡️ Proposed fix
set -exuo pipefail +ARTIFACT_DIR=${ARTIFACT_DIR:-/tmp/artifacts} INSTALLER_DIR=${INSTALLER_DIR:=${ARTIFACT_DIR}/installer} + +if [ -z "$ARTIFACT_DIR" ]; then + echo "Error: ARTIFACT_DIR is not set" >&2 + exit 1 +fi +case "$ARTIFACT_DIR" in + /) echo "Error: ARTIFACT_DIR must not be '/'" >&2; exit 1 ;; + /*) ;; + *) echo "Error: ARTIFACT_DIR must be an absolute path, got: $ARTIFACT_DIR" >&2; exit 1 ;; +esac +export ARTIFACT_DIR +mkdir -p "${ARTIFACT_DIR}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-prow-e2e-techpreview.sh` around lines 18 - 20, Add validation immediately after set -exuo pipefail in the techpreview script to ensure ARTIFACT_DIR is set, non-empty, absolute, and not /. Keep the existing INSTALLER_DIR defaulting behavior, but make it run only after validation so unset, empty, or relative values fail with a clear error.frontend/integration-tests/tsconfig.json (1)
5-5: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the Cypress path from
include
frontend/integration-tests/tsconfig.jsoncan keeptypes: ["cypress"]; the../node_modules/cypressglob only widens the set of files TypeScript scans.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/integration-tests/tsconfig.json` at line 5, Update the include configuration in frontend/integration-tests/tsconfig.json to remove the ../node_modules/cypress entry, leaving only the integration test TypeScript glob. Preserve the existing types configuration for Cypress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-16-techpreview-e2e-test-filtering-design.md`:
- Around line 75-79: Update the spec’s techPreviewOnly fixture description to
match the implementation in the techPreviewOnly fixture: query the FeatureGate
custom resource through k8sClient.getClusterCustomResource and skip unless
spec.featureSet equals TechPreviewNoUpgrade, removing the page.evaluate and
window.SERVER_FLAGS.techPreview behavior.
In `@frontend/integration-tests/support/tech-preview-guard.ts`:
- Around line 26-27: Update requireFeatureGate in tech-preview-guard.ts so it
evaluates enabled gates only from the FeatureGate status entry matching
spec.featureSet, rather than aggregating all feature sets. Fetch the full
FeatureGate resource and filter its parsed JSON in JavaScript, matching the
active-feature-set handling in the Playwright requireFeatureGate fixture.
- Around line 14-22: Remove the top-level before() side effect in
tech-preview-guard.ts and export the guard as a named function such as
requireTechPreview(). Keep its existing featureSet check and skip behavior, and
require callers to explicitly invoke it when they need the TechPreview-only
guard while allowing requireFeatureGate imports to run on graduated or otherwise
enabled clusters.
In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`:
- Around line 54-57: Update the AbortError recovery in fetchLifecycleData to
delete cacheKey only if the cache still references the rejected promise being
handled, preserving a retry installed by another caller; otherwise reuse the
existing retry. Add a regression test covering three callers sharing the aborted
promise and verifying only the first retry is retained.
---
Outside diff comments:
In `@test-prow-playwright-e2e.sh`:
- Around line 10-14: Update the usage documentation for
test-prow-playwright-e2e.sh to include feature-gate in the accepted scenario
list and describe its behavior alongside e2e, release, and smoke, matching the
scenario handling implemented in the script.
---
Nitpick comments:
In `@frontend/integration-tests/tsconfig.json`:
- Line 5: Update the include configuration in
frontend/integration-tests/tsconfig.json to remove the ../node_modules/cypress
entry, leaving only the integration test TypeScript glob. Preserve the existing
types configuration for Cypress.
In `@test-prow-e2e-techpreview.sh`:
- Around line 18-20: Add validation immediately after set -exuo pipefail in the
techpreview script to ensure ARTIFACT_DIR is set, non-empty, absolute, and not
/. Keep the existing INSTALLER_DIR defaulting behavior, but make it run only
after validation so unset, empty, or relative values fail with a clear error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: affb4291-ecb3-465d-a419-2f0563bb14dc
📒 Files selected for processing (16)
.gitignoredocs/superpowers/plans/2026-07-16-techpreview-e2e-test-filtering.mddocs/superpowers/specs/2026-07-16-techpreview-e2e-test-filtering-design.mdfrontend/.eslintignorefrontend/e2e/fixtures/index.tsfrontend/e2e/tests/olm/olmv1-catalog.spec.tsfrontend/integration-tests/.eslintrc.cjsfrontend/integration-tests/support/tech-preview-guard.tsfrontend/integration-tests/tsconfig.jsonfrontend/packages/console-shared/src/utils/__tests__/console-fetch.spec.tsfrontend/packages/console-shared/src/utils/console-fetch.tsfrontend/packages/operator-lifecycle-manager/src/hooks/__tests__/useOperatorLifecycle.spec.tsfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.tstest-prow-e2e-techpreview.shtest-prow-playwright-e2e-techpreview.shtest-prow-playwright-e2e.sh
|
/retest |
When N callers share the same aborted cached promise, the previous fix unconditionally deleted the cache before retrying, causing each caller to start its own redundant fetch and overwrite the previous retry. Fix: capture the promise reference before .catch() and only delete when the cache still holds that exact promise. The first caller to run the handler starts the single retry; all later callers see a newer promise in the cache and reuse it via the recursive fetchLifecycleData call. Adds a three-caller regression test. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The NS_BINDING_ABORTED errors are inherent to the abort-on-unmount pattern: every row unmount (sort, filter, navigation) cancels its in-flight lifecycle request, Firefox logs the cancellation, and the cache-sharing fixes we added are complex responses to a self-inflicted problem. Root fix: let requests complete naturally. - Remove AbortController, abortRef, cleanup abort, and all signal-passing code from useOperatorLifecycle. - Simplify fetchLifecycleData: no more AbortError special-casing, Fix-B retry logic, or multi-caller stomp guard — just deduplicate concurrent requests via the module-level cache and let every request reach the backend exactly once. - When a component unmounts mid-flight the request finishes, populates the cache, and React silently discards the resulting state update. The next render of the same operator (after sort/filter/remount) gets an instant cache hit. - Drop the abort-specific test cases; replace with deduplication and cache-hit tests that describe the new invariants. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`:
- Around line 100-112: Guard the fetchLifecycleData callbacks in the
useOperatorLifecycle effect with an active/cancelled flag, and mark the effect
inactive during cleanup so superseded requests cannot update data, error, or
loading state. Preserve request caching and return the cleanup function from the
effect. Add a regression test that rerenders from A to B, resolves B before A,
and verifies B remains displayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7995f5f3-1484-4bc8-933b-56ba13a73fd8
📒 Files selected for processing (2)
frontend/packages/operator-lifecycle-manager/src/hooks/__tests__/useOperatorLifecycle.spec.tsfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts
The Cluster compatibility and Support phase columns shared the same responsive class as Provided APIs (pf-m-hidden pf-m-visible-on-xl) with no explicit width. When lifecycle data loads successfully and badge content renders (e.g. "Incompatible", "General availability") instead of the "-" fallback, the unconstrained columns compete with Provided APIs for space, causing content to overflow and overlap. Fix: add pf-m-width-15 to both lifecycle columns so they each claim a fixed 15% of table width, keeping them bounded and leaving Provided APIs its natural allocation. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
perdasilva
changed the title
Two independent rendering bugs introduced once lifecycle requests reliably complete (AbortController removed): 1. Virtualized table row overlap (SupportPhaseBadge height change) SupportPhaseBadge rendered two different heights: Active state showed a badge button on line 1 and the end date on line 2 (~52 px total), while NoData and SelfSupport states showed a single line (~20-28 px). When lifecycle data arrived asynchronously after the initial render, CellMeasurer detected the height delta, invalidated its cache, and called recomputeGridSize(). In the one render frame before VirtualTableBody repositioned all subsequent rows, those rows still sat at their stale Y coordinates, causing visible content overlap in the Installed Operators table (e.g. the Serverless row Provided APIs appearing inside Package Server's row space). Fix: introduce DateLineSpacer — an aria-hidden, visibility:hidden div containing a single space. Every branch of SupportPhaseBadge now renders the same two-line structure (badge/status line + date/spacer line), so CellMeasurerCache never observes a height change when lifecycle data loads and the virtualizer never needs to reposition rows. 2. Stale effect callbacks clobbering state after prop change (A→B) useOperatorLifecycle fire-and-forgot the fetchLifecycleData promise with no active guard. If the component re-rendered with a new operator (packageName changed A→B), operator A's in-flight .then/.catch callbacks still held a live setState reference and could overwrite B's already-landed data with stale results. The "React ignores updates after unmount" comment only covers a fully unmounted component; a prop change keeps the same instance alive. Fix: declare `let active = true` before setLoading, gate both .then and .catch on `if (active)`, and return a cleanup that sets active = false. The underlying network request and cache population are intentionally preserved — only state updates are suppressed for superseded invocations. Adds a regression test that rerenders A→B, resolves B before A, and asserts B remains displayed after A's late arrival. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
104b009 to
6c8b96c
Compare
perdasilva
changed the title
perdasilva
changed the title
|
@perdasilva: This pull request references CONSOLE-5427 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
|
@perdasilva: 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. |
2 participants
Analysis / Root cause:
Part 1 — Console noise and AbortError propagation
When `useOperatorLifecycle` re-runs (e.g. subscription loads after CSV, or
multiple components share the same cache key), `fetchLifecycleData` returns the
cached in-flight promise tied to the previous caller's AbortController signal.
When that old signal is aborted, the AbortError propagates to the new caller
whose signal is still live — three problems result:
with `NS_BINDING_ABORTED` noise on the Installed Operators page.
controller), not whether the error was an AbortError, so it incorrectly
surfaced the error in UI state.
stalled in an empty non-loading state instead of retrying.
The root fix is to drop AbortController entirely from `useOperatorLifecycle`:
requests complete naturally, React silently discards state updates after unmount,
and the cache gives instant hits on re-mount.
Part 2 — Virtualized table row overlap
With reliable lifecycle data loading (no aborts), rows whose `SupportPhaseBadge`
shows the Active state (badge + end date = ~52 px, two lines) are taller than
rows still showing NoData/SelfSupport (~20–28 px, one line). The Installed
Operators table uses `VirtualizedTableBody` backed by `react-virtualized`'s
`CellMeasurerCache`. When lifecycle data arrives asynchronously after the row's
initial measurement, `CellMeasurer` detects the height change, invalidates its
cache entry, and calls `recomputeGridSize()`. In the one render frame before
`VirtualTableBody` repositions all subsequent rows, those rows still sit at
their stale Y coordinates, causing visible content overlap (e.g. the Serverless
row's Provided APIs column appearing inside the Package Server row's space).
Part 3 — Stale effect callbacks clobbering state after prop change
`useOperatorLifecycle` fire-and-forgot the `fetchLifecycleData` promise with
no active guard. If the component re-rendered with a new operator (packageName
changed A→B), operator A's in-flight `.then`/`.catch` callbacks still held a
live `setState` reference and could overwrite B's already-landed data with stale
results. The "React ignores updates after unmount" reasoning only covers a fully
unmounted component; a prop change keeps the same instance alive.
Solution description:
they are expected cancellations, not failures.
signal-passing code. Simplify `fetchLifecycleData` to just deduplicate
concurrent requests via the module-level cache. Requests complete naturally;
React silently drops state updates after unmount; the next render gets a cache
hit. Guard the effect's `.then`/`.catch` callbacks with `let active = true` and
return a cleanup that sets `active = false`, so superseded requests (prop change
A→B) cannot overwrite current state. The underlying fetch and cache population
are preserved; only state updates are suppressed.
Compatibility and Support Phase column headers so badge content does not
overflow horizontally into the Provided APIs column (`table-layout: fixed`).
`aria-hidden`, `visibility:hidden` div. Every branch of `SupportPhaseBadge`
(Active, SelfSupport, NoData, fallback) now renders the same two-line
structure (badge/status line + date/spacer line), so `CellMeasurerCache`
never observes a height change when lifecycle data loads and the virtualizer
never repositions rows.
Screenshots / screen recording:
Test setup:
Tested locally against a cluster-bot cluster with `--tech-preview=true --olm-lifecycle-metadata=true`.
Test cases:
Browser conformance: N/A
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UI Improvements