feat(dashboards): add attribution channel tooltips, drop mom deltas#1126
feat(dashboards): add attribution channel tooltips, drop mom deltas#1126mrautela365 wants to merge 7 commits into
Conversation
PR SummaryLow Risk Overview Attribution table — Each consolidated channel row can show a PrimeNG info tooltip driven by new shared Overview tab — Fetches marketing attribution alongside other KPIs; Attributed Revenue is the sum of linear channel revenue so it matches the attribution table below, with no MoM delta. The embedded attribution section accepts KPI deltas — MoM is removed or gated where it was misleading: ROAS on Overview and Performance Marketing; web sessions only when there is a valid prior 4-week window; email CTR shows — when there were no sends and suppresses MoM unless the latest month had sends. Performance marketing — Funnel filter pills, Reviewed by Cursor Bugbot for commit 9623150. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Marketing Impact dashboard now loads attribution data for overview KPIs, displays descriptions for known attribution channels, and removes funnel-stage filtering and columns from performance marketing tables. ChangesMarketing Impact dashboard
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OverviewTab
participant AnalyticsService
participant KpiCards
participant AttributionSection
OverviewTab->>AnalyticsService: Request marketing attribution
AnalyticsService-->>OverviewTab: Return channel attribution data
OverviewTab->>KpiCards: Sum linearRevenue and render attributed revenue
OverviewTab->>AttributionSection: Pass attributionOverride and loadingOverride
AttributionSection->>AttributionSection: Render channel descriptions in tooltips
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add per-channel info tooltips to the Marketing attribution table so viewers know what each consolidated channel groups (notably that 'Direct & Unknown' combines direct visits and sessions with no identifiable source). Definitions live in a shared constant. Suppress misleading month-over-month deltas on KPI cards where the delta tracks a different metric than the value or has no honest prior window: Attributed Revenue, ROAS, Web Sessions, Email CTR. Wire the Attributed Revenue headline to Linear attribution so it agrees with the attribution table. Remove the redundant funnel filter/STAGE column from the performance-marketing tab. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Refines Marketing Impact attribution clarity and KPI reporting.
Changes:
- Adds attribution-channel definition tooltips.
- Uses linear-attributed revenue and suppresses misleading deltas.
- Removes funnel-stage filtering and columns.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
marketing-impact.interface.ts |
Adds attribution data to overview KPIs. |
marketing-impact.constants.ts |
Adds channel descriptions; removes funnel options. |
performance-marketing-tab.component.ts |
Removes funnel filtering and ROAS delta. |
performance-marketing-tab.component.html |
Removes funnel controls and stage columns. |
overview-tab.component.ts |
Refines KPI sources and deltas. |
attribution-section.component.ts |
Provides channel tooltip descriptions. |
attribution-section.component.html |
Renders channel information tooltips. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
688fcfb to
41d4109
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html`:
- Around line 66-71: Update the attribution tooltip trigger in the attribution
section template to be keyboard-focusable, preferably by using a button with
type="button" while preserving the existing tooltip, accessible label, styling,
and test identifier; alternatively add tabindex="0" to the existing icon.
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts`:
- Around line 85-105: The Attributed Revenue card is incorrectly gated by
data.revenueImpact, causing it to disappear when only attribution data is
available. Move the attributedRevenue calculation and cards.push block into an
if (data.attribution) block, while keeping the ROAS card under
data.revenueImpact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51575a9b-81e3-470c-bed4-f4a85ae6681b
📒 Files selected for processing (7)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.tspackages/shared/src/constants/marketing-impact.constants.tspackages/shared/src/interfaces/marketing-impact.interface.ts
Address review comments from copilot and coderabbit: - overview-tab: guard the Attributed Revenue card on data.attribution instead of data.revenueImpact, so it still renders for projectWebsites (where revenueImpact is intentionally null); render an empty/failed attribution response as a dash, not a misleading $0. - overview-tab + attribution-section: pass the already-fetched attribution response into the child via a new attributionOverride input so the Overview tab no longer issues a duplicate getMarketingAttribution query. - attribution-section: make the channel tooltip icon keyboard-focusable (role/tabindex/focus ring) and trigger on both hover and focus. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review feedback addressedCommit: acc3820 Changes
Threads resolved5 of 5 unresolved threads addressed (from copilot and coderabbit). check-types + lint pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts`:
- Around line 76-89: Update initAttributionData so a stale attributionOverride
cannot bypass fetching when selectedPeriod, focusProgram, or foundationSlug
changes. Clear or invalidate the override while the parent request is loading,
or associate it with the current filter inputs and only short-circuit when they
match; preserve the override path for responses matching the active request and
keep loading true until new data arrives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0bf2128-750c-4a30-bdc4-48c33e46d13b
📒 Files selected for processing (4)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts
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>
Review feedback addressedCommit: 49c55aa Changes
Threads resolved4 of 4 addressed. check-types + lint pass. |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Hey @mrautela365 👋
This round tightens up the attribution tooltip work nicely — the keyboard-focusable tooltip, the attributionOverride/loadingOverride fix to stop the duplicate fetch and stale-data flash, and the funnel-column removal are all clean, well-reasoned changes with good inline comments explaining the "why" (calendar zero-fill, linear-attribution consistency, lumpy paid-spend data). The prior review round's 5 findings (duplicate fetch, wrong gating field on Attributed Revenue, hover-only tooltip, stale override, Web Sessions false 0%) are all confirmed fixed via your replies and CodeRabbit's own "Addressed"/"Confirmed" follow-ups — I re-verified each against the current diff and agree they're resolved.
One issue carried over from a previous round was not actually fixed, and I've flagged it inline with the proof: the Email CTR "has sends" check (overview-tab.component.ts:163) uses .some() across the entire zero-filled monthly series instead of checking only the current/trailing month, which is what the server's own momChangePercentage logic and the sibling email-tab.component.ts component both do. This is the same finding GitHub Copilot raised earlier in this PR's history — it appears to have been missed rather than intentionally left as-is.
I did not find any security-relevant issues in this diff (frontend-only dashboard/display change, no new auth, API routes, or data-access boundaries touched). No new test coverage was added for the attributionOverride/loadingOverride gating logic or the three touched components, but that mirrors a pre-existing gap across this whole feature area rather than a regression introduced here — worth a follow-up, not a blocker.
Issue summary:
- 🔴 Blocking: 1 — Email CTR
hasSendscheck inspects the whole monthly series instead of just the current month (carried over, unaddressed since an earlier round) - 🟡 Minor: 0
- ⚪ Nit: 0
- ❔ Question: 0
Final decision: 🔴 Needs changes before approval — one confirmed blocking correctness bug; everything else from this and prior rounds checks out.
Address blocking review from dealako (and copilot): the hasSends check used .some() across the whole calendar-zero-filled monthlySends series, so an earlier month with sends made a zero current month read as active — rendering a CTR value and a 'Needs review' badge when the current scope has no sends. Check only the trailing (current) month, matching the server's momChangePercentage contract and the sibling email-tab pattern. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review feedback addressedCommit: 613c2f9 Change
Thanks @dealako for the detailed blocking catch — re-requesting your review. check-types + lint pass. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Address review comment from cursor: getMarketingAttribution already
catches HTTP errors in AnalyticsService and emits { channels: [],
projects: [] }, so the component-level catchError(() => of(null)) on the
forkJoin arm was unreachable and produced an inconsistent empty shape.
Remove it; the empty channel list renders as a dash as intended.
LFXV2-2023
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Review feedback addressedCommit: 4594069
check-types + lint pass. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4594069. Configure here.
Address review comment from cursor: for a preset range (last-3, YTD) currentCtr is a period aggregate that is valid when any month in the range had sends, but gating the value on only the trailing month forced a dash when that month was zero despite real range activity. Branch on the selection: a single-month view still gates the value on the trailing (current) month (per the earlier month-view fix), while a range gates it on any month in the series having sends. The MoM delta stays gated on the trailing month being active in both cases, matching the server's momChangePercentage contract. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
|
@dealako thanks — addressed the item you flagged. Commit: a16d5f7 The Email CTR value gating now depends on the selection type:
check-types + lint pass. Re-requesting your review. |
Address review comment from copilot: weeklyTrend.length >= 8 alone does not guarantee a valid MoM denominator. The server leaves sessionMomChangePct at 0 when the prior 4-week session total is 0 (to avoid divide-by-zero), so length alone still rendered a bogus 0.0% MoM. Replicate the server's prior4 > 0 guard on the client by summing the prior-4-week sessions from weeklyTrend and only showing the delta when that total is positive. LFXV2-2023 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts:145
- A full eight-row series still does not guarantee a valid comparison: the server leaves
sessionMomChangePctat0when the prior four weeks sum to zero (project.service.ts:5347-5353). In that case this renders0.0% MoMeven when the recent window has traffic, incorrectly presenting an undefined zero-baseline comparison as flat. Include the prior-window denominator when deciding whether the delta is available.
const hasComparisonWindow = (br.weeklyTrend?.length ?? 0) >= 8;
const noComparison = !hasComparisonWindow && br.sessionMomChangePct === 0;
const momPct = noComparison ? null : br.sessionMomChangePct;
| // The MoM delta always requires the latest month to be active (the | ||
| // server's momChangePercentage compares the trailing two months), so it | ||
| // stays gated on lastMonthActive regardless of the selection type. | ||
| const momPct = lastMonthActive ? ec.momChangePercentage : null; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts:142
- The PR says the Web Sessions MoM delta is being dropped because it describes a different metric, but this still renders it whenever a prior window exists.
totalMonthlySessionsis a classification-filtered precomputed 30-day total, whilesessionMomChangePctis computed from an unclassified recent-four-weeks trend (project.service.ts:5257-5275, 5346-5352), so the gating does not make the comparison valid. Keep this KPI’s MoM fields neutral/null as intended.
// sessionMomChangePct is 0 both for a genuine flat month AND when there is
// no valid comparison (the server needs >= 8 weeks of trend AND a positive
// prior-4-week total — it leaves the pct at 0 on a zero prior window to
// avoid divide-by-zero). Replicate both server conditions so a 0 is only
// shown when it is a real "0.0% MoM"; otherwise suppress the delta.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts:177
- Email CTR is also listed among the MoM deltas this PR removes, but an active trailing month still gets
momChangePercentageand can receive the “Needs review” badge. For preset ranges this is especially misleading becausecurrentCtris the whole-period aggregate while the delta compares only the trailing two months. Suppress this delta consistently.
// The MoM delta always requires the latest month to be active (the
// server's momChangePercentage compares the trailing two months), so it
// stays gated on lastMonthActive regardless of the selection type.
const momPct = lastMonthActive ? ec.momChangePercentage : null;
Summary
Marketing Impact dashboard refinements:
ATTRIBUTION_CHANNEL_DESCRIPTIONS).FUNNEL_STAGE_OPTIONSconstant).Notes
MARKETING_ATTRIBUTIONmodel already stores them as a singleDirect / Unknownchannel. The tooltip is an interim clarification; a true split requires an lf-dbt model change to emit them as distinct channels.Verification
yarn check-typespasses across both packages.