SED-4778 Execution queries data while on performance tab#1400
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the AltExecutionRefreshActivityService to manage and optimize the refresh activities of various components (such as test cases, keywords, errors, and trees) based on their active state. It also refactors the routing configuration by removing the named nodeDetails router outlet in favor of standard routing paths. A review comment highlights a critical issue in AltReportNodesSummaryStateService where filtering out the active state before switchMap and smartSwitchMap prevents request cancellation and proper query re-triggering when switching tabs; it is recommended to handle the active state check inside the mapping functions instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const forecastSummaryTotal$ = combineLatest([ | ||
| isActive$, | ||
| this._executionState.execution$, | ||
| this.isFullRangeSelection$ | ||
| ]).pipe( | ||
| filter(([isActive,]) => isActive), | ||
| switchMap(([, execution, isFullRangeSelected]) => { | ||
| if (execution.status !== 'RUNNING' || !isFullRangeSelected) { | ||
| return of(undefined); | ||
| } | ||
| return this._viewService | ||
| .getView(this.statusDistributionViewId, execution.id!) | ||
| .pipe(map((status) => status as ExecutionSummaryDto)); | ||
| }), | ||
| map((summary) => summary?.countForecast), | ||
| ); | ||
|
|
||
| const summaryTimeSeries$ = combineLatest([ | ||
| isActive$, | ||
| this._executionState.execution$, | ||
| this._executionState.timeRangeSelection$, | ||
| ]).pipe( | ||
| filter(([isActive,]) => isActive), | ||
| map(([,execution, range]) => ({execution, range})), | ||
| smartSwitchMap( | ||
| (curr, prev) => { | ||
| return ( | ||
| curr.execution?.id !== prev?.execution?.id || | ||
| !this._dateUtils.areTimeRangeSelectionsEquals(curr.range, prev?.range) | ||
| ); | ||
| }, | ||
| ({execution, range}) => { | ||
| const timeRange = convertPickerSelectionToTimeRange(range, execution, this._executionId()); | ||
| if (!timeRange) { | ||
| return of(undefined); | ||
| } | ||
| const bucketRequest = this.createFetchBucketRequest(execution, timeRange); | ||
| this.summaryInProgressInternal$.next(true); | ||
| return this._timeSeriesService.getReportNodesTimeSeries(bucketRequest).pipe( | ||
| catchError(() => of(undefined)), | ||
| finalize(() => this.summaryInProgressInternal$.next(false)), | ||
| ); | ||
| }, | ||
| this._destroyRef, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Filtering out isActive before switchMap and smartSwitchMap prevents the stream from cancelling pending requests when switching away from the tab, and prevents triggering a new query when switching back to the tab.
We should pass isActive into switchMap and smartSwitchMap and handle the conditional logic inside the mapping functions.
const forecastSummaryTotal$ = combineLatest([
isActive$,
this._executionState.execution$,
this.isFullRangeSelection$
]).pipe(
switchMap(([isActive, execution, isFullRangeSelected]) => {
if (!isActive || execution.status !== 'RUNNING' || !isFullRangeSelected) {
return of(undefined);
}
return this._viewService
.getView(this.statusDistributionViewId, execution.id!)
.pipe(map((status) => status as ExecutionSummaryDto));
}),
map((summary) => summary?.countForecast),
);
const summaryTimeSeries$ = combineLatest([
isActive$,
this._executionState.execution$,
this._executionState.timeRangeSelection$,
]).pipe(
smartSwitchMap(
(curr, prev) => {
return (
curr[0] !== prev?.[0] ||
curr[1]?.id !== prev?.[1]?.id ||
!this._dateUtils.areTimeRangeSelectionsEquals(curr[2], prev?.[2])
);
},
([isActive, execution, range]) => {
if (!isActive) {
return of(undefined);
}
const timeRange = convertPickerSelectionToTimeRange(range, execution, this._executionId());
if (!timeRange) {
return of(undefined);
}
const bucketRequest = this.createFetchBucketRequest(execution, timeRange);
this.summaryInProgressInternal$.next(true);
return this._timeSeriesService.getReportNodesTimeSeries(bucketRequest).pipe(
catchError(() => of(undefined)),
finalize(() => this.summaryInProgressInternal$.next(false)),
);
},
this._destroyRef,
),
);References
- In RxJS, to prevent an outer stream from completing upon an error in an inner stream (e.g., an HTTP request inside a
switchMap), applycatchErrorto the inner stream. This keeps the outer stream alive for subsequent emissions.
There was a problem hiding this comment.
Not sure that this remark is relevant for us:
-
For me it's fine to keep ongoing request on tab's leave. As result stream is shared ongoing request's response will be stored, and user will see it when he will return to the tab, before new request will be performed. It's better option that show nothing.
-
Activity flags are triggers during navigation. When user opens the tab where activity switch to true - it will trigger the new request.
neogucky
left a comment
There was a problem hiding this comment.
I optimised some formatting, otherwise its great!
No description provided.