CONSOLE-5066: Refactor ResourceEventStream for Performance and Impersonation Support#16027
CONSOLE-5066: Refactor ResourceEventStream for Performance and Impersonation Support#16027krishagarwal278 wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@krishagarwal278: This pull request references CONSOLE-5066 which is a valid 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: krishagarwal278 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 |
|
@krishagarwal278: This pull request references CONSOLE-5066 which is a valid 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. |
📝 WalkthroughWalkthroughThe events component migrates from websocket-driven in-memory streaming (WSFactory, watchURL) to the standard Kubernetes watch hook (useK8sWatchResource). The new implementation establishes a data pipeline: fetch EventKind[] with explicit loading and error states, compute sorted events via sortEvents, then derive filtered events by applying kind, type, and text filters. UI rendering shifts from state-based to data-driven using eventsLoaded and eventsLoadError flags. Websocket lifecycle management, configuration, and local state mutations are removed. Public component signatures remain unchanged while the underlying data flow transitions to Kubernetes list-then-watch semantics. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/public/components/events.tsx`:
- Around line 392-399: The pause toggle no longer stops updates because
useK8sWatchResource keeps fetching; either remove the toggle UI or implement
client-side freezing: when toggling to paused (active false) capture a snapshot
(e.g., pausedEvents state) from eventsData and have sortedEvents derive from
pausedEvents when active is false and from eventsData when active is true;
update toggleStream to set active and populate/clear pausedEvents accordingly,
keep using sortEvents(...).slice(0, maxMessages) on the chosen source
(eventsData or pausedEvents) so the visible list stops changing while paused.
🧹 Nitpick comments (2)
frontend/public/components/events.tsx (2)
401-403: Redundant.slice(0, maxMessages)call.
sortedEventsis already limited tomaxMessages(line 398), andfilterEventscan only reduce the count. The second slice is always a no-op.♻️ Remove redundant slice
const filteredEvents = useMemo(() => { - return filterEvents(sortedEvents, { kind, type, filter, textFilter }).slice(0, maxMessages); + return filterEvents(sortedEvents, { kind, type, filter, textFilter }); }, [sortedEvents, kind, type, filter, textFilter]);
423-437: Terminology inconsistency with new implementation.The status messages reference "stream" terminology (
"Error connecting to event stream","Streaming events..."), but the implementation now uses list-then-watch semantics rather than true streaming. Consider updating for accuracy:📝 Update status text to reflect list-then-watch
if (eventsLoadError) { statusBtnTxt = ( <span className="co-sysevent-stream__connection-error"> - {t('Error connecting to event stream')} + {t('Error loading events')} </span> ); sysEventStatus = <ErrorLoadingEvents />; } else if (!eventsLoaded) { statusBtnTxt = <span>{t('Loading events...')}</span>; sysEventStatus = <Loading />; } else if (active) { - statusBtnTxt = <span>{t('Streaming events...')}</span>; + statusBtnTxt = <span>{t('Watching for events...')}</span>; } else { - statusBtnTxt = <span>{t('Event stream is paused.')}</span>; + statusBtnTxt = <span>{t('Event updates paused.')}</span>; }Note: If you keep the pause toggle, ensure the i18n keys are updated in the translation files as well.
| // Sort events and limit to maxMessages | ||
| // Note: We keep the events visible even when paused (active=false) | ||
| const sortedEvents = useMemo(() => { | ||
| if (!eventsData) { | ||
| return []; | ||
| } | ||
| return () => { | ||
| ws.current?.destroy(); | ||
| }; | ||
| }, [namespace, fieldSelector, mock, t]); | ||
| return sortEvents(eventsData).slice(0, maxMessages); | ||
| }, [eventsData]); |
There was a problem hiding this comment.
Pause/play toggle no longer controls data updates.
The active state and toggleStream function remain, but with useK8sWatchResource, the watch continues fetching updates regardless of the toggle state. Previously with WebSocket, pausing would stop receiving new events. Now, the "pause" button only changes the status text—the underlying data continues to update in the background.
This is a behavioral change that may confuse users who expect pausing to freeze the event list. Consider one of:
- Remove the toggle entirely if pause functionality isn't meaningful with list-then-watch semantics.
- Implement client-side freezing by capturing a snapshot of
eventsDatawhen paused:
🔧 Option 2: Client-side pause implementation
+ const [pausedSnapshot, setPausedSnapshot] = useState<EventKind[] | null>(null);
+
+ const toggleStream = () => {
+ setActive((prev) => {
+ const newActive = !prev;
+ if (!newActive) {
+ // Capture snapshot when pausing
+ setPausedSnapshot(eventsData ?? []);
+ } else {
+ // Clear snapshot when resuming
+ setPausedSnapshot(null);
+ }
+ return newActive;
+ });
+ };
// Sort events and limit to maxMessages
- // Note: We keep the events visible even when paused (active=false)
const sortedEvents = useMemo(() => {
- if (!eventsData) {
+ const dataSource = pausedSnapshot ?? eventsData;
+ if (!dataSource) {
return [];
}
- return sortEvents(eventsData).slice(0, maxMessages);
- }, [eventsData]);
+ return sortEvents(dataSource).slice(0, maxMessages);
+ }, [pausedSnapshot, eventsData]);Also applies to: 405-407
🤖 Prompt for AI Agents
In `@frontend/public/components/events.tsx` around lines 392 - 399, The pause
toggle no longer stops updates because useK8sWatchResource keeps fetching;
either remove the toggle UI or implement client-side freezing: when toggling to
paused (active false) capture a snapshot (e.g., pausedEvents state) from
eventsData and have sortedEvents derive from pausedEvents when active is false
and from eventsData when active is true; update toggleStream to set active and
populate/clear pausedEvents accordingly, keep using sortEvents(...).slice(0,
maxMessages) on the chosen source (eventsData or pausedEvents) so the visible
list stops changing while paused.
|
@krishagarwal278: The following test 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. |
2 participants
Summary: Refactored the ResourceEventStream component to use standard Kubernetes watch hooks so that we reduce API server load and correctly support user impersonation. Replaced the custom WebSocket logic with the useK8sWatchResource hook.
Reviewer's Checklist:
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor