fix[react-devtools]: update profiling status before receiving response from backend (#31117)
We can't wait for a response from Backend, because it might take some time to actually finish profiling. We should keep a flag on the frontend side, so user can quickly see the feedback in the UI.
Ruslan Lesiutin committed
Oct 9, 2024 at 13:23 UTC
dbf80c8d7a823041d83baff8b0dca8892ce27411
5 files changed
+42
-18
packages/react-devtools-shared/src/devtools/ProfilerStore.js
+38
-14
@@ -11,6 +11,7 @@ import EventEmitter from '../events';
11
import {prepareProfilingDataFrontendFromBackendAndStore} from './views/Profiler/utils';
12
import ProfilingCache from './ProfilingCache';
13
import Store from './store';
14
+import {logEvent} from 'react-devtools-shared/src/Logger';
15
16
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
17
import type {ProfilingDataBackend} from 'react-devtools-shared/src/backend/types';
@@ -67,7 +68,12 @@ export default class ProfilerStore extends EventEmitter<{
68
69
// The backend is currently profiling.
70
// When profiling is in progress, operations are stored so that we can later reconstruct past commit trees.
70
- _isProfiling: boolean = false;
71
+ _isBackendProfiling: boolean = false;
72
+
73
+ // Mainly used for optimistic UI.
74
+ // This could be false, but at the same time _isBackendProfiling could be true
75
+ // for cases when Backend is busy serializing a chunky payload.
76
+ _isProfilingBasedOnUserInput: boolean = false;
77
78
// Tracks whether a specific renderer logged any profiling data during the most recent session.
79
_rendererIDsThatReportedProfilingData: Set<number> = new Set();
@@ -86,7 +92,8 @@ export default class ProfilerStore extends EventEmitter<{
92
super();
93
94
this._bridge = bridge;
89
- this._isProfiling = defaultIsProfiling;
95
+ this._isBackendProfiling = defaultIsProfiling;
96
+ this._isProfilingBasedOnUserInput = defaultIsProfiling;
97
this._store = store;
98
99
bridge.addListener('operations', this.onBridgeOperations);
@@ -139,8 +146,8 @@ export default class ProfilerStore extends EventEmitter<{
146
return this._rendererQueue.size > 0 || this._dataBackends.length > 0;
147
}
148
142
- get isProfiling(): boolean {
143
- return this._isProfiling;
149
+ get isProfilingBasedOnUserInput(): boolean {
150
+ return this._isProfilingBasedOnUserInput;
151
}
152
153
get profilingCache(): ProfilingCache {
@@ -151,7 +158,7 @@ export default class ProfilerStore extends EventEmitter<{
158
return this._dataFrontend;
159
}
160
set profilingData(value: ProfilingDataFrontend | null): void {
154
- if (this._isProfiling) {
161
+ if (this._isBackendProfiling) {
162
console.warn(
163
'Profiling data cannot be updated while profiling is in progress.',
164
);
@@ -186,6 +193,9 @@ export default class ProfilerStore extends EventEmitter<{
193
startProfiling(): void {
194
this._bridge.send('startProfiling', this._store.recordChangeDescriptions);
195
196
+ this._isProfilingBasedOnUserInput = true;
197
+ this.emit('isProfiling');
198
+
199
// Don't actually update the local profiling boolean yet!
200
// Wait for onProfilingStatus() to confirm the status has changed.
201
// This ensures the frontend and backend are in sync wrt which commits were profiled.
@@ -195,8 +205,12 @@ export default class ProfilerStore extends EventEmitter<{
205
stopProfiling(): void {
206
this._bridge.send('stopProfiling');
207
198
- // Don't actually update the local profiling boolean yet!
199
- // Wait for onProfilingStatus() to confirm the status has changed.
208
+ // Backend might be busy serializing the payload, so we are going to display
209
+ // optimistic UI to the user that profiling is stopping.
210
+ this._isProfilingBasedOnUserInput = false;
211
+ this.emit('isProfiling');
212
+
213
+ // Wait for onProfilingStatus() to confirm the status has changed, this will update _isBackendProfiling.
214
// This ensures the frontend and backend are in sync wrt which commits were profiled.
215
// We do this to avoid mismatches on e.g. CommitTreeBuilder that would cause errors.
216
}
@@ -229,7 +243,7 @@ export default class ProfilerStore extends EventEmitter<{
243
const rendererID = operations[0];
244
const rootID = operations[1];
245
232
- if (this._isProfiling) {
246
+ if (this._isBackendProfiling) {
247
let profilingOperations = this._inProgressOperationsByRootID.get(rootID);
248
if (profilingOperations == null) {
249
profilingOperations = [operations];
@@ -252,8 +266,8 @@ export default class ProfilerStore extends EventEmitter<{
266
267
onBridgeProfilingData: (dataBackend: ProfilingDataBackend) => void =
268
dataBackend => {
255
- if (this._isProfiling) {
256
- // This should never happen, but if it does- ignore previous profiling data.
269
+ if (this._isBackendProfiling) {
270
+ // This should never happen, but if it does, then ignore previous profiling data.
271
return;
272
}
273
@@ -289,7 +303,7 @@ export default class ProfilerStore extends EventEmitter<{
303
};
304
305
onProfilingStatus: (isProfiling: boolean) => void = isProfiling => {
292
- if (this._isProfiling === isProfiling) {
306
+ if (this._isBackendProfiling === isProfiling) {
307
return;
308
}
309
@@ -319,15 +333,25 @@ export default class ProfilerStore extends EventEmitter<{
333
});
334
}
335
322
- this._isProfiling = isProfiling;
336
+ this._isBackendProfiling = isProfiling;
337
+ // _isProfilingBasedOnUserInput should already be updated from startProfiling, stopProfiling, or constructor.
338
+ if (this._isProfilingBasedOnUserInput !== isProfiling) {
339
+ logEvent({
340
+ event_name: 'error',
341
+ error_message: `Unexpected profiling status. Expected ${this._isProfilingBasedOnUserInput.toString()}, but received ${isProfiling.toString()}.`,
342
+ error_stack: new Error().stack,
343
+ error_component_stack: null,
344
+ });
345
+
346
+ // If happened, fallback to displaying the value from Backend
347
+ this._isProfilingBasedOnUserInput = isProfiling;
348
+ }
349
350
// Invalidate suspense cache if profiling data is being (re-)recorded.
351
// Note that we clear again, in case any views read from the cache while profiling.
352
// (That would have resolved a now-stale value without any profiling data.)
353
this._cache.invalidate();
354
329
- this.emit('isProfiling');
330
-
355
// If we've just finished a profiling session, we need to fetch data stored in each renderer interface
356
// and re-assemble it on the front-end into a format (ProfilingDataFrontend) that can power the Profiler UI.
357
// During this time, DevTools UI should probably not be interactive.
packages/react-devtools-shared/src/devtools/store.js
+1
-1
@@ -324,7 +324,7 @@ export default class Store extends EventEmitter<{
324
return this._componentFilters;
325
}
326
set componentFilters(value: Array<ComponentFilter>): void {
327
- if (this._profilerStore.isProfiling) {
327
+ if (this._profilerStore.isProfilingBasedOnUserInput) {
328
// Re-mounting a tree while profiling is in progress might break a lot of assumptions.
329
// If necessary, we could support this- but it doesn't seem like a necessary use case.
330
this._throwAndEmitError(
packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js
+1
-1
@@ -98,7 +98,7 @@ function ProfilerContextController({children}: Props): React.Node {
98
getCurrentValue: () => ({
99
didRecordCommits: profilerStore.didRecordCommits,
100
isProcessingData: profilerStore.isProcessingData,
101
- isProfiling: profilerStore.isProfiling,
101
+ isProfiling: profilerStore.isProfilingBasedOnUserInput,
102
profilingData: profilerStore.profilingData,
103
supportsProfiling: store.rootSupportsBasicProfiling,
104
}),
packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js
+1
-1
@@ -39,7 +39,7 @@ export default function SettingsModal(): React.Node {
39
// Explicitly disallow it for now.
40
const isProfilingSubscription = useMemo(
41
() => ({
42
- getCurrentValue: () => profilerStore.isProfiling,
42
+ getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput,
43
subscribe: (callback: Function) => {
44
profilerStore.addListener('isProfiling', callback);
45
return () => profilerStore.removeListener('isProfiling', callback);
packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js
+1
-1
@@ -29,7 +29,7 @@ export default function SettingsModalContextToggle(): React.Node {
29
// Explicitly disallow it for now.
30
const isProfilingSubscription = useMemo(
31
() => ({
32
- getCurrentValue: () => profilerStore.isProfiling,
32
+ getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput,
33
subscribe: (callback: Function) => {
34
profilerStore.addListener('isProfiling', callback);
35
return () => profilerStore.removeListener('isProfiling', callback);