Skip to content

Commit 49c55aa

Browse files
committed
fix(review): propagate parent loading to attribution section
Address review comments from cursor and copilot: - attribution-section: when a parent supplies attributionOverride, the child no longer flips to the empty state while the parent request is in flight. Add a loadingOverride input; in override mode the skeleton is driven by the parent's loading state so stale/null overrides render the skeleton, not 'No attribution data available'. - overview-tab: pass loading() into the section as loadingOverride. - overview-tab: distinguish a genuine flat 0.0% Web Sessions MoM from a no-comparison-window 0 using weeklyTrend length (>= 8 weeks), so a real flat month shows its delta instead of being suppressed. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
1 parent acc3820 commit 49c55aa

3 files changed

Lines changed: 24 additions & 12 deletions

File tree

apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ export class AttributionSectionComponent {
4848
// Overview tab, which also renders this section), it passes it in so we reuse
4949
// it instead of issuing a duplicate request. `undefined` means "self-fetch".
5050
public readonly attributionOverride = input<MarketingAttributionResponse | null | undefined>(undefined);
51+
// When using a parent override, the parent also passes its in-flight state so
52+
// we show the skeleton (not the empty state) while the parent request runs and
53+
// its override is still the initial/previous-period `null`.
54+
public readonly loadingOverride = input<boolean>(false);
5155

5256
// === Forms ===
5357
protected readonly modelForm = this.fb.nonNullable.group({
@@ -57,14 +61,18 @@ export class AttributionSectionComponent {
5761
protected readonly modelOptions: AttributionModelOption[] = ATTRIBUTION_MODEL_OPTIONS;
5862

5963
// === WritableSignals ===
60-
protected readonly loading = signal(false);
64+
// Tracks the component's own fetch; the template reads `loading` (below), which
65+
// also folds in the parent's loadingOverride when running in override mode.
66+
private readonly selfLoading = signal(false);
6167

6268
// === Computed Signals ===
6369
protected readonly attributionData: Signal<MarketingAttributionResponse | null> = this.initAttributionData();
6470
protected readonly selectedModel: Signal<AttributionModel> = this.initSelectedModel();
6571
protected readonly channelRows: Signal<AttributionChannelRow[]> = this.initChannelRows();
6672
protected readonly totalRevenue: Signal<string> = this.initTotalRevenue();
6773
protected readonly hasData = computed(() => this.channelRows().length > 0);
74+
// In override mode, defer to the parent's in-flight state; otherwise use our own.
75+
protected readonly loading = computed(() => (this.attributionOverride() !== undefined ? this.loadingOverride() : this.selfLoading()));
6876

6977
// === Protected Methods ===
7078
/** Tooltip text describing what a consolidated channel groups; empty when no definition exists (no tooltip shown). */
@@ -82,20 +90,21 @@ export class AttributionSectionComponent {
8290
return toSignal(
8391
combineLatest([override$, slug$, focus$, period$]).pipe(
8492
switchMap(([override, slug, focus, period]) => {
85-
// A parent-supplied response short-circuits our own fetch (no duplicate query).
93+
// A parent-supplied response short-circuits our own fetch (no duplicate
94+
// query). Loading is driven by the parent's loadingOverride in this mode.
8695
if (override !== undefined) {
87-
this.loading.set(false);
96+
this.selfLoading.set(false);
8897
return of(override);
8998
}
9099
if (!slug) {
91-
this.loading.set(false);
100+
this.selfLoading.set(false);
92101
return of(null);
93102
}
94-
this.loading.set(true);
103+
this.selfLoading.set(true);
95104
const classification = FOCUS_TO_CLASSIFICATION[focus];
96105
return this.analyticsService.getMarketingAttribution(slug, classification, period || undefined).pipe(
97106
catchError(() => of(null)),
98-
finalize(() => this.loading.set(false))
107+
finalize(() => this.selfLoading.set(false))
99108
);
100109
})
101110
),

apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,5 +48,6 @@ <h3 class="text-base font-semibold text-gray-900">{{ summaryTitle() }}</h3>
4848
[foundationName]="foundationName()"
4949
[focusProgram]="focusProgram()"
5050
[attributionOverride]="overviewKpiData().attribution"
51+
[loadingOverride]="loading()"
5152
data-testid="overview-tab-attribution-section" />
5253
}

apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,14 @@ export class OverviewTabComponent {
132132

133133
if (data.brandReach) {
134134
const br = data.brandReach;
135-
// sessionMomChangePct defaults to 0 when there is no valid prior window
136-
// to compare against (fewer than 8 weeks of trend, or an empty prior
137-
// period), so a bare 0 is indistinguishable from "no comparison". Treat
138-
// 0 as "nothing to report" and suppress the delta rather than show a
139-
// misleading "0.0% MoM".
140-
const momPct = br.sessionMomChangePct !== 0 ? br.sessionMomChangePct : null;
135+
// sessionMomChangePct is 0 both for a genuine flat month AND when there is
136+
// no valid prior window (the server needs recent-4 vs prior-4 weeks). Use
137+
// the trend length to tell them apart: with a full comparison window (>= 8
138+
// weeks) a 0 is a real "0.0% MoM"; without one, a 0 means "no comparison"
139+
// and the delta is suppressed.
140+
const hasComparisonWindow = (br.weeklyTrend?.length ?? 0) >= 8;
141+
const noComparison = !hasComparisonWindow && br.sessionMomChangePct === 0;
142+
const momPct = noComparison ? null : br.sessionMomChangePct;
141143
cards.push({
142144
id: 'web-sessions',
143145
label: 'Total Web Sessions',

0 commit comments

Comments
 (0)