[RPP] Move insight "shouldShow" logic from component to model

Connor Clark · Devtools-frontend LUCI CQ · commit 71c991ada764 · 2024-11-20T23:45:02.000Z
CLSCulprits is still doing this in the component, left out here since it is a non-trivial refactor. Bug: 371615739 Change-Id: I96a3c1d7de35d8e84f125a4ee8113a64d3a8752b Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6040099 Reviewed-by: Paul Irish <paulirish@chromium.org> Commit-Queue: Connor Clark <cjamcl@chromium.org>
diff --git a/front_end/models/trace/insights/CLSCulprits.ts b/front_end/models/trace/insights/CLSCulprits.ts
@@ -420,12 +420,14 @@ function getFontRootCauses(
   return rootCausesByShift;
 }
 
-function finalize(partialModel: Omit<CLSCulpritsInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<CLSCulpritsInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     CLSCulpritsInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.CLS,
+    // TODO: getTopCulprits in component needs to move to model so this can be set here.
+    shouldShow: true,
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/DocumentLatency.ts b/front_end/models/trace/insights/DocumentLatency.ts
@@ -119,12 +119,19 @@ function getCompressionSavings(request: Types.Events.SyntheticNetworkRequest): n
   return estimatedSavings < IGNORE_THRESHOLD_IN_BYTES ? 0 : estimatedSavings;
 }
 
-function finalize(partialModel: Omit<DocumentLatencyInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<DocumentLatencyInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     DocumentLatencyInsightModel {
+  let hasFailure = false;
+  if (partialModel.data) {
+    hasFailure = partialModel.data.redirectDuration > 0 || partialModel.data.serverResponseTooSlow ||
+        partialModel.data.uncompressedResponseBytes > 0;
+  }
+
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.ALL,
+    shouldShow: hasFailure,
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/FontDisplay.ts b/front_end/models/trace/insights/FontDisplay.ts
@@ -34,12 +34,13 @@ export type FontDisplayInsightModel = InsightModel<{
   }>,
 }>;
 
-function finalize(partialModel: Omit<FontDisplayInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<FontDisplayInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     FontDisplayInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.INP,
+    shouldShow: Boolean(partialModel.fonts.find(font => font.wastedTime > 0)),
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/ImageDelivery.ts b/front_end/models/trace/insights/ImageDelivery.ts
@@ -75,12 +75,13 @@ export type ImageDeliveryInsightModel = InsightModel<{
   optimizableImages: OptimizableImage[],
 }>;
 
-function finalize(partialModel: Omit<ImageDeliveryInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<ImageDeliveryInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     ImageDeliveryInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.LCP,
+    shouldShow: partialModel.optimizableImages.length > 0,
     ...partialModel,
     relatedEvents: partialModel.optimizableImages.map(image => image.request),
   };
diff --git a/front_end/models/trace/insights/InteractionToNextPaint.ts b/front_end/models/trace/insights/InteractionToNextPaint.ts
@@ -32,11 +32,12 @@ export type INPInsightModel = InsightModel<{
   highPercentileInteractionEvent?: SyntheticInteractionPair,
 }>;
 
-function finalize(partialModel: Omit<INPInsightModel, 'title'|'description'|'category'>): INPInsightModel {
+function finalize(partialModel: Omit<INPInsightModel, 'title'|'description'|'category'|'shouldShow'>): INPInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.INP,
+    shouldShow: Boolean(partialModel.longestInteractionEvent),
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/LCPDiscovery.ts b/front_end/models/trace/insights/LCPDiscovery.ts
@@ -44,7 +44,7 @@ export type LCPDiscoveryInsightModel = InsightModel<{
   earliestDiscoveryTimeTs?: Types.Timing.MicroSeconds,
 }>;
 
-function finalize(partialModel: Omit<LCPDiscoveryInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<LCPDiscoveryInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     LCPDiscoveryInsightModel {
   const relatedEvents = partialModel.lcpEvent && partialModel.lcpRequest ?
       // TODO: add entire request initiator chain?
@@ -54,6 +54,10 @@ function finalize(partialModel: Omit<LCPDiscoveryInsightModel, 'title'|'descript
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.LCP,
+    shouldShow: Boolean(
+        partialModel.lcpRequest &&
+        (partialModel.shouldIncreasePriorityHint || partialModel.shouldPreloadImage ||
+         partialModel.shouldRemoveLazyLoading)),
     ...partialModel,
     relatedEvents,
   };
diff --git a/front_end/models/trace/insights/LCPPhases.ts b/front_end/models/trace/insights/LCPPhases.ts
@@ -116,7 +116,8 @@ function breakdownPhases(
   };
 }
 
-function finalize(partialModel: Omit<LCPPhasesInsightModel, 'title'|'description'|'category'>): LCPPhasesInsightModel {
+function finalize(partialModel: Omit<LCPPhasesInsightModel, 'title'|'description'|'category'|'shouldShow'>):
+    LCPPhasesInsightModel {
   const relatedEvents = [];
   if (partialModel.lcpEvent) {
     relatedEvents.push(partialModel.lcpEvent);
@@ -128,6 +129,8 @@ function finalize(partialModel: Omit<LCPPhasesInsightModel, 'title'|'description
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.LCP,
+    // TODO: should move the component's "getPhaseData" to model.
+    shouldShow: Boolean(partialModel.phases) && (partialModel.lcpMs ?? 0) > 0,
     ...partialModel,
     relatedEvents,
   };
diff --git a/front_end/models/trace/insights/RenderBlocking.ts b/front_end/models/trace/insights/RenderBlocking.ts
@@ -151,12 +151,13 @@ function computeSavings(
   return {metricSavings, requestIdToWastedMs};
 }
 
-function finalize(partialModel: Omit<RenderBlockingInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<RenderBlockingInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     RenderBlockingInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.LCP,
+    shouldShow: partialModel.renderBlockingRequests.length > 0,
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/SlowCSSSelector.ts b/front_end/models/trace/insights/SlowCSSSelector.ts
@@ -68,12 +68,13 @@ function aggregateSelectorStats(
   return [...selectorMap.values()];
 }
 
-function finalize(partialModel: Omit<SlowCSSSelectorInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<SlowCSSSelectorInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     SlowCSSSelectorInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.ALL,
+    shouldShow: partialModel.topElapsedMs.length !== 0 && partialModel.topMatchAttempts.length !== 0,
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/ThirdParties.ts b/front_end/models/trace/insights/ThirdParties.ts
@@ -52,12 +52,14 @@ function getRelatedEvents(
   return events;
 }
 
-function finalize(partialModel: Omit<ThirdPartiesInsightModel, 'title'|'description'|'category'>):
+function finalize(partialModel: Omit<ThirdPartiesInsightModel, 'title'|'description'|'category'|'shouldShow'>):
     ThirdPartiesInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.ALL,
+    shouldShow:
+        Boolean([...partialModel.summaryByEntity.entries()].find(kv => kv[0] !== partialModel.firstPartyEntity)),
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/Viewport.ts b/front_end/models/trace/insights/Viewport.ts
@@ -36,11 +36,13 @@ export type ViewportInsightModel = InsightModel<{
   viewportEvent?: Types.Events.ParseMetaViewport,
 }>;
 
-function finalize(partialModel: Omit<ViewportInsightModel, 'title'|'description'|'category'>): ViewportInsightModel {
+function finalize(partialModel: Omit<ViewportInsightModel, 'title'|'description'|'category'|'shouldShow'>):
+    ViewportInsightModel {
   return {
     title: i18nString(UIStrings.title),
     description: i18nString(UIStrings.description),
     category: InsightCategory.INP,
+    shouldShow: partialModel.mobileOptimized === false,
     ...partialModel,
   };
 }
diff --git a/front_end/models/trace/insights/types.ts b/front_end/models/trace/insights/types.ts
@@ -64,6 +64,8 @@ export type InsightModel<R extends Record<string, unknown>> = R&{
   title: string,
   description: string,
   category: InsightCategory,
+  /** True if there is anything of interest to display to the user. */
+  shouldShow: boolean,
   relatedEvents?: Types.Events.Event[],
   warnings?: InsightWarning[],
   metricSavings?: MetricSavings,
diff --git a/front_end/panels/timeline/components/SidebarSingleInsightSet.test.ts b/front_end/panels/timeline/components/SidebarSingleInsightSet.test.ts
@@ -52,7 +52,6 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
       'LCP by phase',
       'LCP request discovery',
       'Render blocking requests',
-      'Improve image delivery',
       'Document request latency',
       'Third parties',
     ]);
diff --git a/front_end/panels/timeline/components/SidebarSingleInsightSet.ts b/front_end/panels/timeline/components/SidebarSingleInsightSet.ts
@@ -216,7 +216,7 @@ export class SidebarSingleInsightSet extends HTMLElement {
       }
 
       const model = models[name as keyof typeof models];
-      if (!model ||
+      if (!model || !model.shouldShow ||
           !shouldRenderForCategory({activeCategory: this.#data.activeCategory, insightCategory: model.category})) {
         continue;
       }
diff --git a/front_end/panels/timeline/components/insights/BaseInsightComponent.test.ts b/front_end/panels/timeline/components/insights/BaseInsightComponent.test.ts
@@ -36,6 +36,7 @@ describeWithEnvironment('BaseInsightComponent', () => {
         title: 'LCP by Phase',
         description: 'some description',
         category: Trace.Insights.Types.InsightCategory.ALL,
+        shouldShow: true,
       };
       renderElementIntoDOM(component);
 
@@ -58,6 +59,7 @@ describeWithEnvironment('BaseInsightComponent', () => {
         title: 'LCP by Phase',
         description: 'some description',
         category: Trace.Insights.Types.InsightCategory.ALL,
+        shouldShow: true,
       };
       renderElementIntoDOM(component);
 
diff --git a/front_end/panels/timeline/components/insights/CLSCulprits.ts b/front_end/panels/timeline/components/insights/CLSCulprits.ts
@@ -150,11 +150,11 @@ export class CLSCulprits extends BaseInsightComponent<CLSCulpritsInsightModel> {
     }
 
     const culpritsByShift = this.model.shifts;
-    const clusters = this.model.clusters ?? [];
-    if (!clusters.length || !this.model.worstCluster) {
+    if (!this.model.clusters.length || !this.model.worstCluster) {
       return;
     }
 
+    // TODO: getTopCulprits needs to move to model.
     const causes = this.getTopCulprits(this.model.worstCluster, culpritsByShift);
 
     const hasCulprits = causes.length > 0;
diff --git a/front_end/panels/timeline/components/insights/DocumentLatency.ts b/front_end/panels/timeline/components/insights/DocumentLatency.ts
@@ -182,10 +182,7 @@ export class DocumentLatency extends BaseInsightComponent<DocumentLatencyInsight
       return;
     }
 
-    const hasFailure = this.model.data.redirectDuration > 0 || this.model.data.serverResponseTooSlow ||
-        this.model.data.uncompressedResponseBytes > 0;
-    const output = hasFailure ? this.#renderContent() : LitHtml.nothing;
-    this.renderWithContent(output);
+    this.renderWithContent(this.#renderContent());
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/FontDisplay.ts b/front_end/panels/timeline/components/insights/FontDisplay.ts
@@ -82,9 +82,11 @@ export class FontDisplay extends BaseInsightComponent<FontDisplayInsightModel> {
   }
 
   override render(): void {
-    const shouldShow = this.model?.fonts.find(font => font.wastedTime);
-    const output = shouldShow ? this.#renderContent() : LitHtml.nothing;
-    this.renderWithContent(output);
+    if (!this.model) {
+      return;
+    }
+
+    this.renderWithContent(this.#renderContent());
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/ImageDelivery.ts b/front_end/panels/timeline/components/insights/ImageDelivery.ts
@@ -125,8 +125,11 @@ export class ImageDelivery extends BaseInsightComponent<ImageDeliveryInsightMode
   }
 
   override render(): void {
-    const output = this.#renderContent();
-    this.renderWithContent(output);
+    if (!this.model) {
+      return;
+    }
+
+    this.renderWithContent(this.#renderContent());
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/InteractionToNextPaint.ts b/front_end/panels/timeline/components/insights/InteractionToNextPaint.ts
@@ -126,10 +126,11 @@ export class InteractionToNextPaint extends BaseInsightComponent<INPInsightModel
   }
 
   override render(): void {
-    const event = this.model?.longestInteractionEvent;
+    if (!this.model?.longestInteractionEvent) {
+      return;
+    }
 
-    const output = event ? this.#renderContent(event) : LitHtml.nothing;
-    this.renderWithContent(output);
+    this.renderWithContent(this.#renderContent(this.model.longestInteractionEvent));
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/LCPDiscovery.ts b/front_end/panels/timeline/components/insights/LCPDiscovery.ts
@@ -57,11 +57,7 @@ interface LCPImageDiscoveryData {
   estimatedSavings: Trace.Types.Timing.MilliSeconds|null;
 }
 
-function getImageData(model: LCPDiscoveryInsightModel|null): LCPImageDiscoveryData|null {
-  if (!model) {
-    return null;
-  }
-
+function getImageData(model: LCPDiscoveryInsightModel): LCPImageDiscoveryData|null {
   if (model.lcpRequest === undefined) {
     return null;
   }
@@ -121,6 +117,10 @@ export class LCPDiscovery extends BaseInsightComponent<LCPDiscoveryInsightModel>
   }
 
   override createOverlays(): Overlays.Overlays.TimelineOverlay[] {
+    if (!this.model) {
+      return [];
+    }
+
     const imageResults = getImageData(this.model);
     if (!imageResults || !imageResults.discoveryDelay) {
       return [];
@@ -158,6 +158,10 @@ export class LCPDiscovery extends BaseInsightComponent<LCPDiscoveryInsightModel>
   }
 
   override getEstimatedSavingsTime(): Trace.Types.Timing.MilliSeconds|null {
+    if (!this.model) {
+      return null;
+    }
+
     return getImageData(this.model)?.estimatedSavings ?? null;
   }
 
@@ -191,6 +195,10 @@ export class LCPDiscovery extends BaseInsightComponent<LCPDiscoveryInsightModel>
   }
 
   override render(): void {
+    if (!this.model) {
+      return;
+    }
+
     const imageResults = getImageData(this.model);
     const output = imageResults ? this.#renderContent(imageResults) : LitHtml.nothing;
     this.renderWithContent(output);
diff --git a/front_end/panels/timeline/components/insights/LCPPhases.ts b/front_end/panels/timeline/components/insights/LCPPhases.ts
@@ -216,15 +216,13 @@ export class LCPPhases extends BaseInsightComponent<LCPPhasesInsightModel> {
     // clang-format on
   }
 
-  #hasDataToRender(phaseData: PhaseData[]): boolean {
-    return phaseData ? phaseData.length > 0 : false;
-  }
-
   override render(): void {
+    if (!this.model) {
+      return;
+    }
+
     const phaseData = this.#getPhaseData();
-    const shouldRender = this.#hasDataToRender(phaseData);
-    const output = shouldRender ? this.#renderContent(phaseData) : LitHtml.nothing;
-    this.renderWithContent(output);
+    this.renderWithContent(this.#renderContent(phaseData));
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/RenderBlocking.ts b/front_end/panels/timeline/components/insights/RenderBlocking.ts
@@ -83,10 +83,11 @@ export class RenderBlocking extends BaseInsightComponent<RenderBlockingInsightMo
   }
 
   override render(): void {
-    const model = this.model;
-    const hasBlockingRequests = model?.renderBlockingRequests && model.renderBlockingRequests.length > 0;
-    const output = hasBlockingRequests ? this.#renderContent() : LitHtml.nothing;
-    this.renderWithContent(output);
+    if (!this.model) {
+      return;
+    }
+
+    this.renderWithContent(this.#renderContent());
   }
 }
 
diff --git a/front_end/panels/timeline/components/insights/SlowCSSSelector.ts b/front_end/panels/timeline/components/insights/SlowCSSSelector.ts
diff --git a/front_end/panels/timeline/components/insights/ThirdParties.ts b/front_end/panels/timeline/components/insights/ThirdParties.ts
diff --git a/front_end/panels/timeline/components/insights/Viewport.ts b/front_end/panels/timeline/components/insights/Viewport.ts
diff --git a/test/interactions/goldens/linux/performance/timeline-web-dev-screenshot-frames.png b/test/interactions/goldens/linux/performance/timeline-web-dev-screenshot-frames.png

Original file line number	Diff line number	Diff line change
`@@ -151,12 +151,13 @@ function computeSavings(`
`151`	`151`	`return {metricSavings, requestIdToWastedMs};`
`152`	`152`	`}`
`153`	`153`
`154`		`-function finalize(partialModel: Omit<RenderBlockingInsightModel, 'title'\|'description'\|'category'>):`
	`154`	`+function finalize(partialModel: Omit<RenderBlockingInsightModel, 'title'\|'description'\|'category'\|'shouldShow'>):`
`155`	`155`	`RenderBlockingInsightModel {`
`156`	`156`	`return {`
`157`	`157`	`title: i18nString(UIStrings.title),`
`158`	`158`	`description: i18nString(UIStrings.description),`
`159`	`159`	`category: InsightCategory.LCP,`
	`160`	`+ shouldShow: partialModel.renderBlockingRequests.length > 0,`
`160`	`161`	`...partialModel,`
`161`	`162`	`};`
`162`	`163`	`}`
Original file line number	Diff line number	Diff line change
`@@ -68,12 +68,13 @@ function aggregateSelectorStats(`
`68`	`68`	`return [...selectorMap.values()];`
`69`	`69`	`}`
`70`	`70`
`71`		`-function finalize(partialModel: Omit<SlowCSSSelectorInsightModel, 'title'\|'description'\|'category'>):`
	`71`	`+function finalize(partialModel: Omit<SlowCSSSelectorInsightModel, 'title'\|'description'\|'category'\|'shouldShow'>):`
`72`	`72`	`SlowCSSSelectorInsightModel {`
`73`	`73`	`return {`
`74`	`74`	`title: i18nString(UIStrings.title),`
`75`	`75`	`description: i18nString(UIStrings.description),`
`76`	`76`	`category: InsightCategory.ALL,`
	`77`	`+ shouldShow: partialModel.topElapsedMs.length !== 0 && partialModel.topMatchAttempts.length !== 0,`
`77`	`78`	`...partialModel,`
`78`	`79`	`};`
`79`	`80`	`}`