Skip to content

Commit adb1cd0

Browse files
jackfranklinDevtools-frontend LUCI CQ
authored andcommitted
RPP: allow insights to be highlighted
We had feedback for the Insights AI flow that the "Insight: LCP by Phase" button did nothing. This CL updates it so that when clicked it reveals the insight by: 1. Opening the sidebar. 2. Expanding the insight. 3. Highlighting it using the same highlight in the network panel. Fixed: 409337524 Change-Id: Ieff6e835155d7a156c12cede2e7be99daaa87f26 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6438856 Commit-Queue: Jack Franklin <[email protected]> Reviewed-by: Ergün Erdoğmuş <[email protected]> Auto-Submit: Jack Franklin <[email protected]>
1 parent dbf253d commit adb1cd0

File tree

7 files changed

+118
-10
lines changed

7 files changed

+118
-10
lines changed

front_end/panels/ai_assistance/AiAssistancePanel.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1005,6 +1005,10 @@ export class AiAssistancePanel extends UI.Panel.Panel {
10051005
const trace = new SDK.TraceObject.RevealableEvent(event);
10061006
return Common.Revealer.reveal(trace);
10071007
}
1008+
if (context instanceof AiAssistanceModel.InsightContext) {
1009+
const item = context.getItem();
1010+
return Common.Revealer.reveal(item);
1011+
}
10081012
// Node picker is using linkifier.
10091013
}
10101014

front_end/panels/timeline/TimelinePanel.ts

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -715,13 +715,17 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
715715
});
716716
}
717717

718-
#setActiveInsight(insight: TimelineComponents.Sidebar.ActiveInsight|null): void {
719-
// When an insight is selected, ensure that the 3P checkbox is disabled
720-
// to avoid dimming interference.
718+
/**
719+
* Activates an insight and ensures the sidebar is open too.
720+
* Pass `highlightInsight: true` to flash the insight with the background highlight colour.
721+
*/
722+
#setActiveInsight(insight: TimelineComponents.Sidebar.ActiveInsight|null, opts: {
723+
highlightInsight: boolean,
724+
} = {highlightInsight: false}): void {
721725
if (insight) {
722726
this.#splitWidget.showBoth();
723727
}
724-
this.#sideBar.setActiveInsight(insight);
728+
this.#sideBar.setActiveInsight(insight, {highlight: opts.highlightInsight});
725729
this.flameChart.setActiveInsight(insight);
726730

727731
if (insight) {
@@ -2796,6 +2800,19 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
27962800
this.flameChart.setSelectionAndReveal(null);
27972801
this.flameChart.selectDetailsViewTab(Tab.Details, null);
27982802
}
2803+
2804+
/**
2805+
* Used to reveal an insight - and is called from the AI Assistance panel when the user clicks on the Insight context button that is shown.
2806+
* Revealing an insight should:
2807+
* 1. Ensure the sidebar is open
2808+
* 2. Ensure the insight is expanded
2809+
* (both of these should be true in the AI Assistance case)
2810+
* 3. Flash the Insight with the highlight colour we use in other panels.
2811+
*/
2812+
revealInsight(insightModel: Trace.Insights.Types.InsightModel): void {
2813+
const insightSetKey = insightModel.navigationId ?? Trace.Types.Events.NO_NAVIGATION;
2814+
this.#setActiveInsight({model: insightModel, insightSetKey}, {highlightInsight: true});
2815+
}
27992816
}
28002817

28012818
export const enum State {
@@ -3007,6 +3024,13 @@ export class EventRevealer implements Common.Revealer.Revealer<SDK.TraceObject.R
30073024
}
30083025
}
30093026

3027+
export class InsightRevealer implements Common.Revealer.Revealer<Utils.InsightAIContext.ActiveInsight> {
3028+
async reveal(revealable: Utils.InsightAIContext.ActiveInsight): Promise<void> {
3029+
await UI.ViewManager.ViewManager.instance().showView('timeline');
3030+
TimelinePanel.instance().revealInsight(revealable.insight);
3031+
}
3032+
}
3033+
30103034
export class ActionDelegate implements UI.ActionRegistration.ActionDelegate {
30113035
handleAction(context: UI.Context.Context, actionId: string): boolean {
30123036
const panel = context.flavor(TimelinePanel);

front_end/panels/timeline/components/Sidebar.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import * as Common from '../../../core/common/common.js';
77
import type * as Trace from '../../../models/trace/trace.js';
8+
import * as RenderCoordinator from '../../../ui/components/render_coordinator/render_coordinator.js';
89
import * as UI from '../../../ui/legacy/legacy.js';
910

1011
import {SidebarAnnotationsTab} from './SidebarAnnotationsTab.js';
@@ -120,8 +121,10 @@ export class SidebarWidget extends UI.Widget.VBox {
120121
);
121122
}
122123

123-
setActiveInsight(activeInsight: ActiveInsight|null): void {
124-
this.#insightsView.setActiveInsight(activeInsight);
124+
setActiveInsight(activeInsight: ActiveInsight|null, opts: {
125+
highlight: boolean,
126+
}): void {
127+
this.#insightsView.setActiveInsight(activeInsight, opts);
125128

126129
if (activeInsight) {
127130
this.#tabbedPane.selectTab(SidebarTabs.INSIGHTS);
@@ -147,8 +150,16 @@ class InsightsView extends UI.Widget.VBox {
147150
this.#component.insights = data;
148151
}
149152

150-
setActiveInsight(active: ActiveInsight|null): void {
153+
setActiveInsight(active: ActiveInsight|null, opts: {highlight: boolean}): void {
151154
this.#component.activeInsight = active;
155+
if (opts.highlight && active) {
156+
// Wait for the rendering of the component to be done, otherwise we
157+
// might highlight the wrong insight. The UI needs to be fully
158+
// re-rendered before we can highlight the newly-expanded insight.
159+
void RenderCoordinator.done().then(() => {
160+
this.#component.highlightActiveInsight();
161+
});
162+
}
152163
}
153164
}
154165

front_end/panels/timeline/components/SidebarInsightsTab.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import * as Utils from '../utils/utils.js';
1717
import * as Insights from './insights/insights.js';
1818
import type {ActiveInsight} from './Sidebar.js';
1919
import stylesRaw from './sidebarInsightsTab.css.js';
20-
import type {SidebarSingleInsightSetData} from './SidebarSingleInsightSet.js';
20+
import type {SidebarSingleInsightSet, SidebarSingleInsightSetData} from './SidebarSingleInsightSet.js';
2121

2222
// TODO(crbug.com/391381439): Fully migrate off of constructed style sheets.
2323
const styles = new CSSStyleSheet();
@@ -194,6 +194,19 @@ export class SidebarInsightsTab extends HTMLElement {
194194
// clang-format on
195195
}
196196

197+
highlightActiveInsight(): void {
198+
if (!this.#activeInsight) {
199+
return;
200+
}
201+
// Find the right set for this insight via the set key.
202+
const set = this.#shadow?.querySelector<SidebarSingleInsightSet>(
203+
`devtools-performance-sidebar-single-navigation[data-insight-set-key="${this.#activeInsight.insightSetKey}"]`);
204+
if (!set) {
205+
return;
206+
}
207+
set.highlightActiveInsight();
208+
}
209+
197210
#render(): void {
198211
if (!this.#parsedTrace || !this.#insights) {
199212
Lit.render(Lit.nothing, this.#shadow, {host: this});
@@ -219,6 +232,7 @@ export class SidebarInsightsTab extends HTMLElement {
219232
220233
const contents = html`
221234
<devtools-performance-sidebar-single-navigation
235+
data-insight-set-key=${id}
222236
.data=${data}>
223237
</devtools-performance-sidebar-single-navigation>
224238
`;

front_end/panels/timeline/components/SidebarSingleInsightSet.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import * as Lit from '../../../ui/lit/lit.js';
1414
import * as VisualLogging from '../../../ui/visual_logging/visual_logging.js';
1515
import {md} from '../utils/Helpers.js';
1616

17+
import type {BaseInsightComponent} from './insights/BaseInsightComponent.js';
1718
import {shouldRenderForCategory} from './insights/Helpers.js';
1819
import * as Insights from './insights/insights.js';
1920
import type {ActiveInsight} from './Sidebar.js';
@@ -90,8 +91,7 @@ export interface SidebarSingleInsightSetData {
9091
* "enable experimental performance insights" experiment. This is used to enable
9192
* us to ship incrementally without turning insights on by default for all
9293
* users. */
93-
const EXPERIMENTAL_INSIGHTS: ReadonlySet<string> = new Set([
94-
]);
94+
const EXPERIMENTAL_INSIGHTS: ReadonlySet<string> = new Set([]);
9595

9696
type InsightNameToComponentMapping =
9797
Record<string, typeof Insights.BaseInsightComponent.BaseInsightComponent<Trace.Insights.Types.InsightModel>>;
@@ -126,6 +126,8 @@ export class SidebarSingleInsightSet extends HTMLElement {
126126
readonly #shadow = this.attachShadow({mode: 'open'});
127127
#renderBound = this.#render.bind(this);
128128

129+
#activeInsightElement: BaseInsightComponent<Trace.Insights.Types.InsightModel>|null = null;
130+
129131
#data: SidebarSingleInsightSetData = {
130132
insights: null,
131133
insightSetKey: null,
@@ -136,6 +138,7 @@ export class SidebarSingleInsightSet extends HTMLElement {
136138
};
137139

138140
#dismissedFieldMismatchNotice = false;
141+
#activeHighlightTimeout = -1;
139142

140143
set data(data: SidebarSingleInsightSetData) {
141144
this.#data = data;
@@ -146,6 +149,26 @@ export class SidebarSingleInsightSet extends HTMLElement {
146149
this.#render();
147150
}
148151

152+
disconnectedCallback(): void {
153+
window.clearTimeout(this.#activeHighlightTimeout);
154+
}
155+
156+
highlightActiveInsight(): void {
157+
if (!this.#activeInsightElement) {
158+
return;
159+
}
160+
// First clear any existing highlight that is going on.
161+
this.#activeInsightElement.removeAttribute('highlight-insight');
162+
window.clearTimeout(this.#activeHighlightTimeout);
163+
164+
requestAnimationFrame(() => {
165+
this.#activeInsightElement?.setAttribute('highlight-insight', 'true');
166+
this.#activeHighlightTimeout = window.setTimeout(() => {
167+
this.#activeInsightElement?.removeAttribute('highlight-insight');
168+
}, 2_000);
169+
});
170+
}
171+
149172
#metricIsVisible(label: 'LCP'|'CLS'|'INP'): boolean {
150173
if (this.#data.activeCategory === Trace.Insights.Types.InsightCategory.ALL) {
151174
return true;
@@ -389,6 +412,11 @@ export class SidebarSingleInsightSet extends HTMLElement {
389412
const component = html`<div>
390413
<${componentClass.litTagName}
391414
.selected=${this.#data.activeInsight?.model === model}
415+
${Lit.Directives.ref(elem => {
416+
if(this.#data.activeInsight?.model === model && elem) {
417+
this.#activeInsightElement = elem as BaseInsightComponent<Trace.Insights.Types.InsightModel>;
418+
}
419+
})}
392420
.model=${model}
393421
.bounds=${insightSet.bounds}
394422
.insightSetKey=${insightSetKey}

front_end/panels/timeline/components/insights/baseInsightComponent.css

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,22 @@
44
* found in the LICENSE file.
55
*/
66

7+
@keyframes insight-highlight-fade-out {
8+
from {
9+
background-color: var(--sys-color-yellow-container);
10+
}
11+
12+
to {
13+
background-color: transparent;
14+
}
15+
}
16+
17+
:host([highlight-insight]) {
18+
.insight {
19+
animation: insight-highlight-fade-out 2s 0s;
20+
}
21+
}
22+
723
.insight {
824
display: block;
925
position: relative;

front_end/panels/timeline/timeline-meta.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,3 +366,14 @@ Common.Revealer.registerRevealer({
366366
return new Timeline.TimelinePanel.EventRevealer();
367367
},
368368
});
369+
370+
Common.Revealer.registerRevealer({
371+
contextTypes() {
372+
return maybeRetrieveContextTypes(Timeline => [Timeline.Utils.InsightAIContext.ActiveInsight]);
373+
},
374+
destination: Common.Revealer.RevealerDestination.TIMELINE_PANEL,
375+
async loadRevealer() {
376+
const Timeline = await loadTimelineModule();
377+
return new Timeline.TimelinePanel.InsightRevealer();
378+
},
379+
});

0 commit comments

Comments
 (0)