Skip to content

feat(dashboards): add platform performance table#724

Open
mrautela365 wants to merge 11 commits into
mainfrom
feat/LFXV2-1644-platform-performance
Open

feat(dashboards): add platform performance table#724
mrautela365 wants to merge 11 commits into
mainfrom
feat/LFXV2-1644-platform-performance

Conversation

@mrautela365
Copy link
Copy Markdown
Contributor

Summary

  • Add Platform Performance table to the Performance Marketing tab, showing per-ad-platform breakdown (Google Ads, LinkedIn Ads, Microsoft Ads, etc.)
  • New Snowflake query aggregates PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH by CHANNEL with spend, revenue, ROAS, clicks, impressions, CTR, CPC, conversion rate, and conversions
  • Includes a TOTAL footer row and horizontal scroll for narrow viewports
  • Uses neutral performance labels (EXCELLENT, GOOD, AVERAGE, EMERGING) — no negative/red-flag labels while data quality is being iterated on
  • Adds PaidPlatformBreakdown interface (backend) and PlatformPerformanceRow view-model (frontend)

Test plan

  • Navigate to /foundation/marketing-impact?project=tlf → Performance Marketing tab
  • Verify Platform Performance table appears below the project breakdown table
  • Verify all 11 columns render correctly (PLATFORM, SPEND, REVENUE, ROAS, CLICKS, IMPRESSIONS, CTR, CPC, CONV. RATE, CONVERSIONS, PERFORMANCE)
  • Verify TOTAL row shows aggregated values
  • Verify performance badges show only neutral/positive labels (EXCELLENT, GOOD, AVERAGE, EMERGING)
  • Verify table scrolls horizontally on narrow viewports
  • Verify empty state renders when no platform data is available

LFXV2-1644

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 17, 2026 21:09
@mrautela365 mrautela365 requested a review from a team as a code owner May 17, 2026 21:09
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds platform-level performance metrics end-to-end: server query and response shape, shared TypeScript contracts for performance categories and platform data, client-side mapping and aggregation into platform rows and a TOTAL row with ROAS bucketing, template rendering for a Platform performance table, and null-safe attribution revenue aggregation.

Changes

Platform Performance Metrics and Performance Marketing Tab

Layer / File(s) Summary
Shared data contracts for performance marketing
packages/shared/src/interfaces/analytics-data.interface.ts, packages/shared/src/interfaces/marketing-impact.interface.ts
Defines PaidProjectPerformance union, introduces PaidPlatformBreakdown, updates PaidProjectBreakdown.performance to the new type, extends SocialReachResponse with optional platformBreakdown, re-exports performance type, adds FunnelStage, and defines PlatformPerformanceRow view model.
Server-side platform metrics query and response
apps/lfx-one/src/server/services/project.service.ts
Adds a Snowflake platform-by-CHANNEL query, runs it in parallel with existing queries with graceful catch-on-failure, maps rows into normalized platform metrics (spend/revenue/ROAS/CTR/CPC/convRate/conversions), updates ROAS-to-performance bucketing, and includes platformBreakdown in success and fallback responses.
Performance Marketing Tab component logic
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts
Updates RxJS loading/error handling, replaces performance buckets (POOR/NO_REVENUE → AVERAGE/EMERGING), adds platformRows/platformTotals signals, formats platform rows with CTR/CPC/convRate, and computes a TOTAL row with zero-division guards and ROAS-derived performance.
Performance Marketing Tab template
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html
Renders the Platform performance card when hasPlatforms() is true, iterates platformRows() with performanceClass and data-testid, conditionally renders an aggregated TOTAL row, and shows an empty-state when no platform data exists.

Attribution Section Null-Safety Improvements

Layer / File(s) Summary
Attribution Section revenue aggregation and code organization
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts
Relocates private static revenueKeyByModel and updates revenue aggregation to use nullish coalescing (ch[revenueKey] ?? 0), ensuring missing revenue fields contribute 0 to totals and per-row revenue.

Sequence Diagram(s)

sequenceDiagram
  participant Client as PerformanceMarketingTabComponent
  participant Service as AnalyticsService
  participant API as ProjectService.getSocialReach
  participant Snowflake as Snowflake

  Client->>Service: request social reach
  Service->>API: getSocialReach(foundationSlug)
  API->>Snowflake: platform-by-CHANNEL query (6 months)
  Snowflake-->>API: platform rows (spend,revenue,clicks,impressions,conversions)
  API->>API: compute CTR/CPC/convRate/ROAS
  API->>API: bucket ROAS -> PaidProjectPerformance
  API-->>Service: SocialReachResponse { platformBreakdown }
  Service-->>Client: platformBreakdown delivered
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(dashboards): add platform performance table' accurately summarizes the main change—adding a platform performance table to the dashboards.
Description check ✅ Passed The description is directly related to the changeset, detailing the platform performance table feature, new Snowflake query, interface additions, and a comprehensive test plan.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/LFXV2-1644-platform-performance

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts (1)

24-29: ⚡ Quick win

Consolidate to one UPPER_SNAKE_CASE revenue map constant.

These two static maps are identical and can drift; keep a single constant (e.g., REVENUE_KEY_BY_MODEL) and reference it from getRevenueKey.

♻️ Proposed cleanup
-  private static readonly revenueKeyMap: Record<AttributionModel, 'linearRevenue' | 'firstTouchRevenue' | 'lastTouchRevenue' | 'timeDecayRevenue'> = {
-    linear: 'linearRevenue',
-    firstTouch: 'firstTouchRevenue',
-    lastTouch: 'lastTouchRevenue',
-    timeDecay: 'timeDecayRevenue',
-  };
+  private static readonly REVENUE_KEY_BY_MODEL: Record<AttributionModel, 'linearRevenue' | 'firstTouchRevenue' | 'lastTouchRevenue' | 'timeDecayRevenue'> = {
+    linear: 'linearRevenue',
+    firstTouch: 'firstTouchRevenue',
+    lastTouch: 'lastTouchRevenue',
+    timeDecay: 'timeDecayRevenue',
+  };
@@
-  private static readonly revenueKeyByModel: Record<AttributionModel, 'linearRevenue' | 'firstTouchRevenue' | 'lastTouchRevenue' | 'timeDecayRevenue'> = {
-    linear: 'linearRevenue',
-    firstTouch: 'firstTouchRevenue',
-    lastTouch: 'lastTouchRevenue',
-    timeDecay: 'timeDecayRevenue',
-  };
@@
-    return AttributionSectionComponent.revenueKeyByModel[model];
+    return AttributionSectionComponent.REVENUE_KEY_BY_MODEL[model];
As per coding guidelines: Use UPPER_SNAKE_CASE for constants.

Also applies to: 47-52

🤖 Prompt for 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.

In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts`
around lines 24 - 29, Replace the two identical static maps with a single
UPPER_SNAKE_CASE constant and update callers to use it: create a single constant
named REVENUE_KEY_BY_MODEL that maps AttributionModel ->
'linearRevenue'|'firstTouchRevenue'|'lastTouchRevenue'|'timeDecayRevenue',
remove the duplicate static property revenueKeyMap (and the second identical
map), and update getRevenueKey (and any other references) to read from
REVENUE_KEY_BY_MODEL so both places use the same source of truth.
🤖 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.

Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts`:
- Around line 24-29: Replace the two identical static maps with a single
UPPER_SNAKE_CASE constant and update callers to use it: create a single constant
named REVENUE_KEY_BY_MODEL that maps AttributionModel ->
'linearRevenue'|'firstTouchRevenue'|'lastTouchRevenue'|'timeDecayRevenue',
remove the duplicate static property revenueKeyMap (and the second identical
map), and update getRevenueKey (and any other references) to read from
REVENUE_KEY_BY_MODEL so both places use the same source of truth.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9525b52b-8cfb-4a90-a4ac-c6276fcab8a8

📥 Commits

Reviewing files that changed from the base of the PR and between 3354ca1 and 3465454.

📒 Files selected for processing (16)
  • 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/attribution-section/attribution-section.component.scss
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.scss
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.ts
  • apps/lfx-one/src/server/services/project.service.ts
  • packages/shared/src/constants/marketing-impact.constants.ts
  • packages/shared/src/interfaces/analytics-data.interface.ts
  • packages/shared/src/interfaces/marketing-impact.interface.ts
  • packages/shared/src/utils/marketing-impact.utils.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds new Marketing Impact dashboard functionality by introducing a Performance Marketing tab with a Platform Performance table (plus supporting shared interfaces/constants/utils) and a new Marketing attribution section used in both the Overview and Attribution tabs.

Changes:

  • Extend Social Reach API response to include a platform-level paid performance breakdown and surface it in a new Performance Marketing UI tab (with totals row + horizontal scroll).
  • Introduce a reusable Attribution section with model selector and channel revenue-share table, and embed it into the Overview tab.
  • Add shared marketing-impact types/constants and trend formatting helpers used by KPI cards.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/shared/src/utils/marketing-impact.utils.ts Adds shared trend helper utilities for KPI trend direction/color + percent-change formatting.
packages/shared/src/interfaces/marketing-impact.interface.ts Introduces new Marketing Impact view-model/types (attribution model, rows, funnel stage, performance labels).
packages/shared/src/interfaces/analytics-data.interface.ts Extends SocialReachResponse with optional platformBreakdown and adds PaidPlatformBreakdown type.
packages/shared/src/constants/marketing-impact.constants.ts Adds attribution model options and funnel stage filter options.
apps/lfx-one/src/server/services/project.service.ts Adds Snowflake query for channel/platform breakdown and updates paid performance labels returned by the API.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.ts Wires new Attribution + Performance Marketing tab components into the Marketing Impact page.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.html Adds conditional rendering for Attribution and Performance Marketing tabs.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.html Updates KPI card markup (icon wrapper + new testids).
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts Implements Performance Marketing tab data fetching, KPI cards, project rows, platform rows, and totals computation.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.scss Adds stylesheet file (currently header only).
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html Adds Performance Marketing tab UI including project table + platform performance table with total row.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts Refactors overview KPI computation and embeds the Attribution section; uses shared trend helpers.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html Adds KPI empty state and renders Attribution section below overview KPI cards.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts Adds Attribution section component with model selector and computed table rows.
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.scss Adds stylesheet file (currently header only).
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html Adds Attribution section UI with model selector, alert banner, skeleton, table, and empty state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/lfx-one/src/server/services/project.service.ts
@mrautela365
Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed

Responses

  • project.service.ts:2436: The getPaidPerformance label change from POOR/NO REVENUE to AVERAGE/EMERGING is intentional. The shared PaidProjectPerformance type and the UI performanceClassMap were updated in the same PR. This is a deliberate product decision to use neutral labels while data accuracy is being validated.
  • attribution-section.component.ts:52: The duplicate revenueKeyMap has been removed in PR feat(dashboards): show all sections on overview tab #725 (cf1a547). Only revenueKeyByModel is retained.

Threads Resolved

2 of 2 review threads addressed and resolved.

Comment thread packages/shared/src/interfaces/analytics-data.interface.ts Outdated
Comment thread packages/shared/src/interfaces/analytics-data.interface.ts Outdated
@mrautela365 mrautela365 force-pushed the feat/LFXV2-1644-platform-performance branch from 3465454 to f70e092 Compare May 18, 2026 17:49
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/server/services/project.service.ts`:
- Line 2512: The function currently only attaches platformBreakdown on the happy
path, while an early short-circuit return (the pre-aggregation/empty-data path)
and the older return path still return the old shape; update all return paths in
the function that builds the response so they always include the new breakdown
arrays (platformBreakdown and the corresponding channelBreakdown/channel-level
array) even when monthlySeries or other data is missing. Locate the response
object assembly (where platformBreakdown was added and the earlier short-circuit
return occurs) and ensure both the early return and the fallback return include
empty arrays for platformBreakdown and channelBreakdown instead of omitting or
leaving them undefined.
🪄 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: 3c07e02e-435b-4052-91f5-061fc598e8d1

📥 Commits

Reviewing files that changed from the base of the PR and between 3465454 and f70e092.

📒 Files selected for processing (11)
  • 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/attribution-section/attribution-section.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.scss
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.ts
  • apps/lfx-one/src/server/services/project.service.ts
  • packages/shared/src/constants/marketing-impact.constants.ts
  • packages/shared/src/interfaces/analytics-data.interface.ts
  • packages/shared/src/interfaces/marketing-impact.interface.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.scss
🚧 Files skipped from review as they are similar to previous changes (7)
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.html
  • packages/shared/src/interfaces/analytics-data.interface.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html
  • packages/shared/src/constants/marketing-impact.constants.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts

Comment thread apps/lfx-one/src/server/services/project.service.ts
mrautela365 added a commit that referenced this pull request May 18, 2026
- performance-marketing-tab: remove dead @if guard, add
  normalizePerformance() to validate API boundary values
- analytics-data.interface: narrow PaidPlatformBreakdown.performance
  to PaidProjectPerformance, collapse multi-line JSDoc
- attribution-section: remove dead revenueKeyMap declaration

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings May 18, 2026 18:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment thread packages/shared/src/interfaces/analytics-data.interface.ts Outdated
Comment thread apps/lfx-one/src/server/services/project.service.ts Outdated
mrautela365 added a commit that referenced this pull request May 18, 2026
- Fix circular type dependency: move PaidProjectPerformance definition
  to analytics-data.interface.ts, re-export from marketing-impact
- Eliminate duplicate Snowflake query: derive channelGroups from
  platformPerfResult instead of running a separate channelQuery
- Fix ROAS/impressions MoM swap: assign changePercentage to ROAS card,
  compute impressions MoM from monthlyData via computeMomPct helper
- Fix loading race condition: replace finalize with tap + catchError
  to prevent premature loading.set(false) on switchMap cancellation
- Add projectBreakdown/platformBreakdown to early return and catch
  fallback paths in getSocialReach

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365
Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed

Commit: 28b64ed

Changes Made

  • analytics-data.interface.ts / marketing-impact.interface.ts: Fixed circular type dependency — moved PaidProjectPerformance definition to analytics-data.interface.ts, re-exported from marketing-impact.interface.ts (per copilot[bot])
  • project.service.ts: Eliminated duplicate Snowflake query — removed channelQuery and derived channelGroups from platformPerfResult.rows instead, saving one Snowflake round-trip (per copilot[bot])
  • project.service.ts: Added projectBreakdown: [] and platformBreakdown: [] to early-return and catch fallback paths (per coderabbitai[bot])
  • performance-marketing-tab.component.ts: Fixed loading race condition — replaced finalize with tap + catchError to prevent premature loading.set(false) on switchMap cancellation (per copilot[bot])
  • performance-marketing-tab.component.ts: Fixed ROAS/impressions MoM swap — changePercentage now assigned to ROAS card, impressions MoM computed from monthlyData (per copilot[bot])

Threads Resolved

5 of 5 unresolved threads addressed in this iteration.

mrautela365 added a commit that referenced this pull request May 18, 2026
- performance-marketing-tab: remove dead @if guard, add
  normalizePerformance() to validate API boundary values
- analytics-data.interface: narrow PaidPlatformBreakdown.performance
  to PaidProjectPerformance, collapse multi-line JSDoc
- attribution-section: remove dead revenueKeyMap declaration

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
mrautela365 added a commit that referenced this pull request May 18, 2026
- Fix circular type dependency: move PaidProjectPerformance definition
  to analytics-data.interface.ts, re-export from marketing-impact
- Eliminate duplicate Snowflake query: derive channelGroups from
  platformPerfResult instead of running a separate channelQuery
- Fix ROAS/impressions MoM swap: assign changePercentage to ROAS card,
  compute impressions MoM from monthlyData via computeMomPct helper
- Fix loading race condition: replace finalize with tap + catchError
  to prevent premature loading.set(false) on switchMap cancellation
- Add projectBreakdown/platformBreakdown to early return and catch
  fallback paths in getSocialReach

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365 mrautela365 force-pushed the feat/LFXV2-1644-platform-performance branch from 28b64ed to 1570106 Compare May 18, 2026 19:13
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/performance-marketing-tab/performance-marketing-tab.component.html`:
- Around line 127-140: The TOTAL row leaves the PERFORMANCE cell empty; use the
values provided by platformTotals() to render the performance badge: in the
template block that uses `@if` (platformTotals(); as totals) replace the empty
<div ... role="cell"></div> with markup that displays totals.performance and
applies totals.performanceClass (or equivalent CSS class) so the badge appears
for totals just like for individual rows; ensure you reference
totals.performance and totals.performanceClass when building the badge element
inside the row for the PERFORMANCE column.
🪄 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: 171644cf-f35f-4c7f-aa50-b9ca12667c59

📥 Commits

Reviewing files that changed from the base of the PR and between dd3fad9 and 1570106.

📒 Files selected for processing (6)
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts
  • apps/lfx-one/src/server/services/project.service.ts
  • packages/shared/src/interfaces/analytics-data.interface.ts
  • packages/shared/src/interfaces/marketing-impact.interface.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/lfx-one/src/server/services/project.service.ts
  • apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts

Copilot AI review requested due to automatic review settings May 18, 2026 19:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread apps/lfx-one/src/server/services/project.service.ts
Comment thread apps/lfx-one/src/server/services/project.service.ts
mrautela365 and others added 3 commits May 18, 2026 13:00
Add platform-level performance breakdown table aggregating paid campaign
data by ad channel (Google Ads, LinkedIn Ads, etc.) with spend, revenue,
ROAS, clicks, impressions, CTR, CPC, conversion rate, and conversions.
Includes TOTAL footer row, horizontal scroll for narrow viewports, and
neutral performance labels (EXCELLENT, GOOD, AVERAGE, EMERGING) to avoid
premature red flags while data quality is being validated.

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- performance-marketing-tab: remove dead @if guard, add
  normalizePerformance() to validate API boundary values
- analytics-data.interface: narrow PaidPlatformBreakdown.performance
  to PaidProjectPerformance, collapse multi-line JSDoc
- attribution-section: remove dead revenueKeyMap declaration

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Fix circular type dependency: move PaidProjectPerformance definition
  to analytics-data.interface.ts, re-export from marketing-impact
- Eliminate duplicate Snowflake query: derive channelGroups from
  platformPerfResult instead of running a separate channelQuery
- Fix ROAS/impressions MoM swap: assign changePercentage to ROAS card,
  compute impressions MoM from monthlyData via computeMomPct helper
- Fix loading race condition: replace finalize with tap + catchError
  to prevent premature loading.set(false) on switchMap cancellation
- Add projectBreakdown/platformBreakdown to early return and catch
  fallback paths in getSocialReach

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
@mrautela365 mrautela365 force-pushed the feat/LFXV2-1644-platform-performance branch from 2fbe027 to 4129052 Compare May 18, 2026 20:01
- Use type-only import for PaidProjectPerformance
- Add ROAS_RAW column for unrounded classification
- Deduplicate normalizePerformance/getRoasPerformance calls

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings May 18, 2026 21:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread packages/shared/src/interfaces/marketing-impact.interface.ts Outdated
Comment thread apps/lfx-one/src/server/services/project.service.ts
dealako and others added 2 commits May 18, 2026 15:22
Use the same rounded ROAS value for both display and performance
classification so the label always matches the visible number.
Remove duplicate PaidProjectPerformance re-export.

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings May 18, 2026 22:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Comment thread apps/lfx-one/src/server/services/project.service.ts Outdated
Comment thread packages/shared/src/interfaces/analytics-data.interface.ts
Align platform breakdown and totals ROAS formatting with
the KPI cards and project rows by appending the x suffix.
Add blank line after import in analytics-data interface.

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings May 19, 2026 03:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

apps/lfx-one/src/server/services/project.service.ts:2265

  • The CTR/CPC/CONV_RATE expressions combine DIV0(...) with NULLIF(..., 0). DIV0 already guards divide-by-zero (returns 0 when denominator is 0), but NULLIF turns 0 into NULL, which makes DIV0 return NULL instead. Consider dropping the NULLIF wrappers (or dropping DIV0 if you intentionally want NULL) to keep the SQL semantics clear and avoid unexpected NULLs in the result set.
        ROUND(DIV0(SUM(CLICKS), NULLIF(SUM(IMPRESSIONS), 0)) * 100, 2) AS CTR,
        ROUND(DIV0(SUM(SPEND), NULLIF(SUM(CLICKS), 0)), 2) AS CPC,
        ROUND(DIV0(SUM(CONV), NULLIF(SUM(CLICKS), 0)) * 100, 2) AS CONV_RATE,

apps/lfx-one/src/server/services/project.service.ts:2386

  • platformPerfQuery is treated as optional (caught and converted to rows: []), but channelGroups is now derived exclusively from platformPerfResult.rows. This means a platform query failure will silently wipe out the impressions-by-channel data too (regression vs having a dedicated channel query). Consider either making platformPerfQuery non-optional so failures surface, or adding a lightweight fallback query just for channelGroups when the platform breakdown query fails.
      const channelGroups = platformPerfResult.rows
        .map((row) => ({
          channel: row.CHANNEL,
          totalImpressions: row.IMPRESSIONS,
        }))
        .sort((a, b) => b.totalImpressions - a.totalImpressions);

Comment thread apps/lfx-one/src/server/services/project.service.ts Outdated
@dealako
Copy link
Copy Markdown
Contributor

dealako commented May 19, 2026

Hey Misha — thanks for the fast turnaround on the review. I went through each item against the latest commit (8636f1b) and everything lands cleanly. 👏

👏 Nice work

  • The normalizePerformance helper is a slightly better solution than what I suggested — adding .trim() and the typed Set membership check makes the boundary genuinely tamper-proof against casing/whitespace drift from Snowflake.
  • Routing the TOTAL row's classification through getRoasPerformance(totalRoas) instead of reusing a row value is a nice touch; it keeps the aggregate label honest.
  • The interface narrowing on PaidPlatformBreakdown.performance closes the loop with PaidProjectBreakdown — both breakdown types now carry the same narrow union.

✅ Resolved

  1. Dead @if (row.performanceClass) guard — dropped on all three badge spans (project row, platform row, totals row). Confirmed at performance-marketing-tab.component.html:68, 119, 140.
  2. Unchecked as PaidProjectPerformance casts — replaced with normalizePerformance() + validPerformance Set at performance-marketing-tab.component.ts:45, 271. Both initProjectRows and initPlatformRows route through it; the assertions are gone.
  3. PaidPlatformBreakdown.performance narrow type — now PaidProjectPerformance at analytics-data.interface.ts:2711. Compiler will now flag any drift at the API boundary.
  4. Multi-line JSDoc on PaidPlatformBreakdown — collapsed to the single-line version verbatim.
  5. Dead revenueKeyMap — gone; only revenueKeyByModel remains in attribution-section.component.ts. (Came in via the main merge from feat(dashboards): show all sections on overview tab #725, as you noted earlier.)

⚠️ Partially addressed

None.

❌ Still open

None.

🔍 New observations (non-blocking)

  • Minor: in initProjectRows (performance-marketing-tab.component.ts:193-194) the map callback calls this.normalizePerformance(p.performance) twice — once for performance and once inside getPerformanceClass(...). The platform-row callback right below it already caches the result into a local perf const. Mirroring that pattern here is a one-line tidy; happy to leave for a follow-up.

Overall

Approving. All five comments are closed out with code that matches (or improves on) the intent, no regressions introduced. Feel free to merge once CI is green.

dealako
dealako previously approved these changes May 19, 2026
Copy link
Copy Markdown
Contributor

@dealako dealako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. See my comment ablve.

Add subtitle to platform performance table noting revenue/ROAS use
linear attribution. Add backend comment documenting the intentional
attribution model split between KPI headlines (first-touch) and
drill-down tables (linear).

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Unify all social reach queries on LAST_TOUCH_REVENUE from the
PAID_SOCIAL_REACH_BY_PROJECT_CHANNEL_MONTH table. This eliminates
the attribution model mismatch between KPI headlines (was
first-touch) and drill-down tables (was linear). ROAS MoM is now
computed from two completed months instead of the pre-computed
ROAS_MOM_PCT column.

LFXV2-1644

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Copilot AI review requested due to automatic review settings May 19, 2026 04:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

const [impressionsResult, roasKpiResult, monthlyRoasResult, monthlyImpressionsResult, projectPerfResult, platformPerfResult] = await Promise.all([
this.snowflakeService.execute<{ TOTAL_IMPRESSIONS: number; TOTAL_SPEND: number; TOTAL_REVENUE: number }>(impressionsQuery, [foundationSlug]),
this.snowflakeService.execute<{ ROAS: number; ROAS_MOM_PCT: number }>(roasKpiQuery, [foundationSlug]),
this.snowflakeService.execute<{ CAMPAIGN_MONTH: string; ROAS: number }>(roasKpiQuery, [foundationSlug, foundationSlug]),
Comment on lines +2322 to +2354
this.snowflakeService
.execute<{
CHANNEL: string;
SPEND: number;
REVENUE: number;
ROAS: number;
CLICKS: number;
IMPRESSIONS: number;
CTR: number;
CPC: number;
CONV_RATE: number;
CONVERSIONS: number;
}>(platformPerfQuery, [foundationSlug])
.catch((error) => {
logger.warning(undefined, 'get_social_reach', 'Optional platform breakdown query failed, degrading gracefully', {
foundation_slug: foundationSlug,
err: error,
});
return {
rows: [] as {
CHANNEL: string;
SPEND: number;
REVENUE: number;
ROAS: number;
CLICKS: number;
IMPRESSIONS: number;
CTR: number;
CPC: number;
CONV_RATE: number;
CONVERSIONS: number;
}[],
};
}),
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants