OCPBUGS-86894: reduce startup API calls and prioritize critical fetches#16533
OCPBUGS-86894: reduce startup API calls and prioritize critical fetches#16533logonoff wants to merge 2 commits into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-86894, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@logonoff: This pull request references Jira Issue OCPBUGS-86894, which is valid. 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. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughUser resource loading now uses a stable watch descriptor; coFetch calls gain explicit priorities (high for early auth, low for deferred Swagger/terminal checks); delayed post-resource Swagger scheduling was removed; an auth-pending skeleton, translations, and styles were added. ChangesUser resource watch update
Fetch Request Handling and Prioritization
Auth-pending skeleton, translations, and styles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
component/shared
|
Review and prowpy retest monitoring |
logonoff
force-pushed
the
OCPBUGS-86894-useuser
branch
from
7e6f324 to
3d08b43
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/hooks/__tests__/useUser.spec.ts (1)
81-95: ⚡ Quick winAdd coverage for the watch-error path.
useUseronly dispatchessetUserResourcewhenuserResourceLoaded && userResourceData && !userResourceError. The new tests cover the success and impersonation cases, but there's no test asserting that a watch error (e.g.[null, true, someError]) suppresses the dispatch. That branch is the most likely to regress silently.💚 Suggested test to cover the error branch
+ it('should not dispatch setUserResource when the watch reports an error', () => { + const mockUser = { username: 'testuser@example.com' }; + + mockUseSelector.mockReturnValueOnce(mockUser).mockReturnValueOnce(null); + mockUseK8sWatchResource.mockReturnValue([null, true, new Error('forbidden')]); + + renderHook(() => useUser()); + + expect(mockDispatch).not.toHaveBeenCalled(); + });Want me to open an issue to track this coverage gap?
Also applies to: 152-167
🤖 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/packages/console-shared/src/hooks/__tests__/useUser.spec.ts` around lines 81 - 95, Add a test in useUser.spec.ts to cover the watch-error branch by simulating useUser with an existing user (mockUseSelector returns the user) and mockUseK8sWatchResource returning [null, true, someError] (i.e., userResourceLoaded true but userResourceError present), then renderHook(() => useUser()) and assert that mockDispatch was NOT called with the setUserResource action (type 'setUserResource', payload { userResource: ... }); this ensures useUser's conditional (userResourceLoaded && userResourceData && !userResourceError) correctly suppresses dispatch on watch errors.
🤖 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.
Nitpick comments:
In `@frontend/packages/console-shared/src/hooks/__tests__/useUser.spec.ts`:
- Around line 81-95: Add a test in useUser.spec.ts to cover the watch-error
branch by simulating useUser with an existing user (mockUseSelector returns the
user) and mockUseK8sWatchResource returning [null, true, someError] (i.e.,
userResourceLoaded true but userResourceError present), then renderHook(() =>
useUser()) and assert that mockDispatch was NOT called with the setUserResource
action (type 'setUserResource', payload { userResource: ... }); this ensures
useUser's conditional (userResourceLoaded && userResourceData &&
!userResourceError) correctly suppresses dispatch on watch errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2cb42c1-2fff-4d38-b16b-71173bd47241
📒 Files selected for processing (7)
frontend/packages/console-shared/src/hooks/__tests__/useUser.spec.tsfrontend/packages/console-shared/src/hooks/useUser.tsfrontend/packages/webterminal-plugin/src/components/cloud-shell/cloud-shell-utils.tsfrontend/public/actions/k8s.tsfrontend/public/components/app.tsxfrontend/public/module/k8s/__tests__/swagger.spec.tsfrontend/public/module/k8s/swagger.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/webterminal-plugin/src/components/cloud-shell/cloud-shell-utils.ts
- frontend/public/module/k8s/tests/swagger.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/public/components/app.tsx
- frontend/packages/console-shared/src/hooks/useUser.ts
- frontend/public/actions/k8s.ts
- frontend/public/module/k8s/swagger.ts
logonoff
force-pushed
the
OCPBUGS-86894-useuser
branch
2 times, most recently
from
bfa581e to
aecb812
Compare
openshift-ci
Bot
added
the
kind/i18n
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/public/components/app.tsx`:
- Around line 88-92: The probe currently removes the co-auth-pending class only
on success (coFetch(...).then(() =>
document.documentElement.classList.remove('co-auth-pending'))), and swallows
errors in catch, leaving the class set on failures; update the promise handling
for coFetch to ensure
document.documentElement.classList.remove('co-auth-pending') runs on both
success and failure (use a .finally() or call the same remove in both .then and
.catch) and keep the catch logic for any error handling/logging as needed so the
class is always cleared.
In `@frontend/public/style/_layout.scss`:
- Around line 42-52: The reduced-motion media query currently only sets
animation-duration to 0s but leaves the 20s animation delay on
.co-page-skeleton__slow-msg, so update the `@media` (prefers-reduced-motion:
reduce) block to also cancel the delay (e.g., set animation: none or set
animation-delay: 0s in addition to animation-duration: 0s) for the
.co-page-skeleton__slow-msg selector to ensure the recovery hint appears
immediately for users who prefer reduced motion.
🪄 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: 692edb77-4ee9-4122-8703-6931a22e739e
📒 Files selected for processing (12)
frontend/packages/console-app/src/components/detect-context/DetectContext.tsxfrontend/packages/console-shared/src/hooks/__tests__/useUser.spec.tsfrontend/packages/console-shared/src/hooks/useUser.tsfrontend/packages/webterminal-plugin/src/components/cloud-shell/cloud-shell-utils.tsfrontend/public/actions/k8s.tsfrontend/public/components/app.tsxfrontend/public/components/cluster-settings/_cluster-settings.scssfrontend/public/index.htmlfrontend/public/locales/en/public.jsonfrontend/public/module/k8s/__tests__/swagger.spec.tsfrontend/public/module/k8s/swagger.tsfrontend/public/style/_layout.scss
✅ Files skipped from review due to trivial changes (3)
- frontend/public/components/cluster-settings/_cluster-settings.scss
- frontend/public/module/k8s/swagger.ts
- frontend/packages/webterminal-plugin/src/components/cloud-shell/cloud-shell-utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/public/actions/k8s.ts
- frontend/packages/console-shared/src/hooks/useUser.ts
- frontend/public/module/k8s/tests/swagger.spec.ts
- frontend/packages/console-shared/src/hooks/tests/useUser.spec.ts
- useUser: switch from useK8sGet to useK8sWatchResource so multiple hook instances share a single watch instead of each fetching users/~ independently (22 calls → 1) - swagger: add fetch priority 'low' to openapi/v2 so the 3 MB download yields to critical resources during startup - app.tsx: add fetch priority 'high' to the early auth check - cloud-shell-utils: add fetch priority 'low' to terminal/available so it yields to critical startup resources - actions/k8s: remove duplicate fetchSwagger call since app.tsx already handles initial fetch and polling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Also only render the skeleton if the user is confirmed to be able to access the k8s api. This way, PageSkeleton has no chance to load for unauthenticated users prior to the login page redirect Also x2 extract the skip link into its own component so we can suspend it
logonoff
force-pushed
the
OCPBUGS-86894-useuser
branch
from
aecb812 to
632e26c
Compare
|
@logonoff: The following tests failed, say
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. |
3 participants
Analysis / Root cause:
Profiling startup network activity revealed that the
useUserhook (which fetchesusers/~from the k8s API) usesuseK8sGet— a hook with no cross-instance deduplication. Every component that callsuseUser()(directly or viauseTelemetry()) triggers an independent fetch. With ~10+ component instances mounting during startup, this produced 22 duplicate API calls to the same endpoint.Additionally, the 3 MB
openapi/v2fetch and multipleterminal/availablechecks were competing with critical startup resources for bandwidth, with no fetch priority hints to let the browser schedule them appropriately.Solution description:
useUser: Switch fromuseK8sGettouseK8sWatchResource, which deduplicates at the Redux/watch infrastructure level. All hook instances share a single underlying watch, reducingusers/~calls from 22 to 1. This also correctly handles impersonation changes since the watch infrastructure reconnects when auth context changes.Fetch Priority hints (Fetch Priority API):
priority: 'high'on the early auth check (api/kubernetes/api) so the browser prioritizes the authentication redirect checkpriority: 'low'onopenapi/v2(3 MB, used only for form field descriptions — not needed for initial render)priority: 'low'onterminal/availablechecks (not render-critical)Remove duplicate
fetchSwaggercall fromactions/k8s.ts—app.tsxalready handles the initial fetch and 5-minute polling, so the second call 10 seconds after discovery was redundant.Performance results
Tested under simulated Fast 3G (1.5 Mbps, 562ms RTT) + 4x CPU throttle, 5 runs each, browser cache cleared between runs. App-ready measured as time until
[data-test="page-heading"] h1is visible.users/~callsterminal/availablecallsScreenshots / screen recording:
Test setup:
Two console instances running against the same cluster:
:9000— this PR (OCPBUGS-86894-useuserbranch):9001— HEAD ofmainBoth running over HTTP/1.1 with auth disabled, localhost bridge, same OCP 5.0 cluster.
Benchmarked with Playwright using Chrome DevTools Protocol for CPU/network throttling.
Test cases:
users/~is fetched exactly once during startup (was 22x)terminal/availableusespriority: 'low'and yields to critical resourcesopenapi/v2usespriority: 'low'and downloads after critical resourcesyarn testpassesBrowser conformance:
Additional info:
The
useK8sGethook creates a newuseState+useEffectper instance with no shared state — every component that mounts independently fetches the same resource.useK8sWatchResourceuses the console's watch infrastructure which deduplicates at the Redux store level, so multiple hook instances share a single underlying fetch/watch.The Fetch Priority API (
priority: 'low'/'high') is supported in Chrome 101+, Edge 101+, and Safari 17.2+. Unsupported browsers silently ignore the hint with no behavioral change.Summary by CodeRabbit
Performance Improvements
Updates
UI / UX
Tests