From dbf80c8d7a823041d83baff8b0dca8892ce27411 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Wed, 9 Oct 2024 13:23:23 +0100 Subject: [PATCH] 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. --- .../src/devtools/ProfilerStore.js | 52 ++++++++++++++----- .../src/devtools/store.js | 2 +- .../views/Profiler/ProfilerContext.js | 2 +- .../devtools/views/Settings/SettingsModal.js | 2 +- .../Settings/SettingsModalContextToggle.js | 2 +- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/ProfilerStore.js b/packages/react-devtools-shared/src/devtools/ProfilerStore.js index a55487ec19a37..5ed7e69f294b9 100644 --- a/packages/react-devtools-shared/src/devtools/ProfilerStore.js +++ b/packages/react-devtools-shared/src/devtools/ProfilerStore.js @@ -11,6 +11,7 @@ import EventEmitter from '../events'; import {prepareProfilingDataFrontendFromBackendAndStore} from './views/Profiler/utils'; import ProfilingCache from './ProfilingCache'; import Store from './store'; +import {logEvent} from 'react-devtools-shared/src/Logger'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type {ProfilingDataBackend} from 'react-devtools-shared/src/backend/types'; @@ -67,7 +68,12 @@ export default class ProfilerStore extends EventEmitter<{ // The backend is currently profiling. // When profiling is in progress, operations are stored so that we can later reconstruct past commit trees. - _isProfiling: boolean = false; + _isBackendProfiling: boolean = false; + + // Mainly used for optimistic UI. + // This could be false, but at the same time _isBackendProfiling could be true + // for cases when Backend is busy serializing a chunky payload. + _isProfilingBasedOnUserInput: boolean = false; // Tracks whether a specific renderer logged any profiling data during the most recent session. _rendererIDsThatReportedProfilingData: Set = new Set(); @@ -86,7 +92,8 @@ export default class ProfilerStore extends EventEmitter<{ super(); this._bridge = bridge; - this._isProfiling = defaultIsProfiling; + this._isBackendProfiling = defaultIsProfiling; + this._isProfilingBasedOnUserInput = defaultIsProfiling; this._store = store; bridge.addListener('operations', this.onBridgeOperations); @@ -139,8 +146,8 @@ export default class ProfilerStore extends EventEmitter<{ return this._rendererQueue.size > 0 || this._dataBackends.length > 0; } - get isProfiling(): boolean { - return this._isProfiling; + get isProfilingBasedOnUserInput(): boolean { + return this._isProfilingBasedOnUserInput; } get profilingCache(): ProfilingCache { @@ -151,7 +158,7 @@ export default class ProfilerStore extends EventEmitter<{ return this._dataFrontend; } set profilingData(value: ProfilingDataFrontend | null): void { - if (this._isProfiling) { + if (this._isBackendProfiling) { console.warn( 'Profiling data cannot be updated while profiling is in progress.', ); @@ -186,6 +193,9 @@ export default class ProfilerStore extends EventEmitter<{ startProfiling(): void { this._bridge.send('startProfiling', this._store.recordChangeDescriptions); + this._isProfilingBasedOnUserInput = true; + this.emit('isProfiling'); + // Don't actually update the local profiling boolean yet! // Wait for onProfilingStatus() to confirm the status has changed. // This ensures the frontend and backend are in sync wrt which commits were profiled. @@ -195,8 +205,12 @@ export default class ProfilerStore extends EventEmitter<{ stopProfiling(): void { this._bridge.send('stopProfiling'); - // Don't actually update the local profiling boolean yet! - // Wait for onProfilingStatus() to confirm the status has changed. + // Backend might be busy serializing the payload, so we are going to display + // optimistic UI to the user that profiling is stopping. + this._isProfilingBasedOnUserInput = false; + this.emit('isProfiling'); + + // Wait for onProfilingStatus() to confirm the status has changed, this will update _isBackendProfiling. // This ensures the frontend and backend are in sync wrt which commits were profiled. // We do this to avoid mismatches on e.g. CommitTreeBuilder that would cause errors. } @@ -229,7 +243,7 @@ export default class ProfilerStore extends EventEmitter<{ const rendererID = operations[0]; const rootID = operations[1]; - if (this._isProfiling) { + if (this._isBackendProfiling) { let profilingOperations = this._inProgressOperationsByRootID.get(rootID); if (profilingOperations == null) { profilingOperations = [operations]; @@ -252,8 +266,8 @@ export default class ProfilerStore extends EventEmitter<{ onBridgeProfilingData: (dataBackend: ProfilingDataBackend) => void = dataBackend => { - if (this._isProfiling) { - // This should never happen, but if it does- ignore previous profiling data. + if (this._isBackendProfiling) { + // This should never happen, but if it does, then ignore previous profiling data. return; } @@ -289,7 +303,7 @@ export default class ProfilerStore extends EventEmitter<{ }; onProfilingStatus: (isProfiling: boolean) => void = isProfiling => { - if (this._isProfiling === isProfiling) { + if (this._isBackendProfiling === isProfiling) { return; } @@ -319,15 +333,25 @@ export default class ProfilerStore extends EventEmitter<{ }); } - this._isProfiling = isProfiling; + this._isBackendProfiling = isProfiling; + // _isProfilingBasedOnUserInput should already be updated from startProfiling, stopProfiling, or constructor. + if (this._isProfilingBasedOnUserInput !== isProfiling) { + logEvent({ + event_name: 'error', + error_message: `Unexpected profiling status. Expected ${this._isProfilingBasedOnUserInput.toString()}, but received ${isProfiling.toString()}.`, + error_stack: new Error().stack, + error_component_stack: null, + }); + + // If happened, fallback to displaying the value from Backend + this._isProfilingBasedOnUserInput = isProfiling; + } // Invalidate suspense cache if profiling data is being (re-)recorded. // Note that we clear again, in case any views read from the cache while profiling. // (That would have resolved a now-stale value without any profiling data.) this._cache.invalidate(); - this.emit('isProfiling'); - // If we've just finished a profiling session, we need to fetch data stored in each renderer interface // and re-assemble it on the front-end into a format (ProfilingDataFrontend) that can power the Profiler UI. // During this time, DevTools UI should probably not be interactive. diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 8af997d9287ef..9f4343beabf43 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -324,7 +324,7 @@ export default class Store extends EventEmitter<{ return this._componentFilters; } set componentFilters(value: Array): void { - if (this._profilerStore.isProfiling) { + if (this._profilerStore.isProfilingBasedOnUserInput) { // Re-mounting a tree while profiling is in progress might break a lot of assumptions. // If necessary, we could support this- but it doesn't seem like a necessary use case. this._throwAndEmitError( diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js index f4ebc26a07259..1ef7bb2b7e5c7 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js @@ -98,7 +98,7 @@ function ProfilerContextController({children}: Props): React.Node { getCurrentValue: () => ({ didRecordCommits: profilerStore.didRecordCommits, isProcessingData: profilerStore.isProcessingData, - isProfiling: profilerStore.isProfiling, + isProfiling: profilerStore.isProfilingBasedOnUserInput, profilingData: profilerStore.profilingData, supportsProfiling: store.rootSupportsBasicProfiling, }), diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js index f6652ada3a860..023b342a0cb9a 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModal.js @@ -39,7 +39,7 @@ export default function SettingsModal(): React.Node { // Explicitly disallow it for now. const isProfilingSubscription = useMemo( () => ({ - getCurrentValue: () => profilerStore.isProfiling, + getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput, subscribe: (callback: Function) => { profilerStore.addListener('isProfiling', callback); return () => profilerStore.removeListener('isProfiling', callback); diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js index 306f0715111ba..94fa5c411181e 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle.js @@ -29,7 +29,7 @@ export default function SettingsModalContextToggle(): React.Node { // Explicitly disallow it for now. const isProfilingSubscription = useMemo( () => ({ - getCurrentValue: () => profilerStore.isProfiling, + getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput, subscribe: (callback: Function) => { profilerStore.addListener('isProfiling', callback); return () => profilerStore.removeListener('isProfiling', callback);