feat(dashboards): show all sections on overview tab#725
Open
mrautela365 wants to merge 23 commits into
Open
Conversation
Contributor
|
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:
WalkthroughAdds six marketing-impact dashboard tabs (attribution, email, performance-marketing, web-activity, social-accounts, social-listening), shared view-models/constants and trend helpers, refactors overview KPI aggregation/formatting, updates KPI-card test IDs/section support, and wires tabs into the marketing-impact shell. ChangesMarketing Impact Dashboard Extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts (1)
24-29: ⚡ Quick winRemove duplicate attribution-model revenue maps to avoid drift.
revenueKeyMapandrevenueKeyByModelcontain the same mapping, but only one is used. Keeping both invites inconsistent updates later.♻️ Proposed cleanup
export class AttributionSectionComponent { - private static readonly revenueKeyMap: Record<AttributionModel, 'linearRevenue' | 'firstTouchRevenue' | 'lastTouchRevenue' | 'timeDecayRevenue'> = { - linear: 'linearRevenue', - firstTouch: 'firstTouchRevenue', - lastTouch: 'lastTouchRevenue', - timeDecay: 'timeDecayRevenue', - }; - // === Services === private readonly analyticsService = inject(AnalyticsService); private readonly fb = inject(FormBuilder);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, There are two identical maps defined (revenueKeyMap and revenueKeyByModel) which risks divergence; remove the duplicate and keep a single source of truth (either revenueKeyMap or revenueKeyByModel), update all references in AttributionSectionComponent to use the remaining symbol (e.g., revenueKeyMap) and delete the other definition, and run/verify compilation and unit tests to ensure no remaining references to the removed name.
🤖 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.ts`:
- Around line 165-173: The filter callback in
performance-marketing-tab.component.ts allows rows with empty funnelStage to
pass through for specific funnel filters; update the predicate in the
.filter((p) => { ... }) used in the PerformanceMarketingTab component so that
when funnel !== 'all' you explicitly reject missing or unknown stages (check
p.funnelStage?.toLowerCase() === '' or 'unknown' and return false), then
continue to check funnel === 'tofu' / 'mofu' / 'bofu' using
stage.startsWith('tofu') or exact matches; keep funnel === 'all' returning true.
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.ts`:
- Around line 41-53: The toSignal pipeline for slug$ (used in toSignal(...))
currently lets errors from analyticsService.getWebActivitiesSummary(slug) bubble
up; wrap that observable with catchError(() => of(null)) (before finalize(() =>
this.loading.set(false))) so failures return null instead of throwing, and
update imports to include catchError from 'rxjs'; keep the existing
loading.set(true/false) logic around getWebActivitiesSummary to match the
error-handling pattern used in social-accounts-tab and social-listening-tab.
In `@packages/shared/src/utils/marketing-impact.utils.ts`:
- Around line 36-56: The current guards in trendDirection, trendColorClass, and
formatChangePct only check for null/NaN and therefore allow Infinity/-Infinity
to pass through; update each function to treat non-finite numbers as invalid by
using Number.isFinite(pct) (or an equivalent isFinite check) in the initial
condition (e.g., replace the Number.isNaN(pct) check with !Number.isFinite(pct))
so Infinity values return the neutral color/trend and formatChangePct returns
null instead of producing "Infinity%".
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts`:
- Around line 24-29: There are two identical maps defined (revenueKeyMap and
revenueKeyByModel) which risks divergence; remove the duplicate and keep a
single source of truth (either revenueKeyMap or revenueKeyByModel), update all
references in AttributionSectionComponent to use the remaining symbol (e.g.,
revenueKeyMap) and delete the other definition, and run/verify compilation and
unit tests to ensure no remaining references to the removed name.
🪄 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: 0a685349-47fe-43aa-b742-ab8f2f7b2dbb
📒 Files selected for processing (25)
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.scssapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.scssapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.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.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.scssapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.scssapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.scssapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.tspackages/shared/src/constants/marketing-impact.constants.tspackages/shared/src/interfaces/marketing-impact.interface.tspackages/shared/src/utils/marketing-impact.utils.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the Marketing Impact dashboard so the Overview tab becomes a stacked summary of all marketing sections while preserving individual focused tabs.
Changes:
- Adds dedicated section/tab components for attribution, performance marketing, email, web activity, social accounts, and social listening.
- Embeds all sections into the Overview tab and routes individual tab selections to the matching component.
- Adds shared view-model types, constants, and trend formatting helpers for the new marketing-impact UI.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/utils/marketing-impact.utils.ts |
Adds shared trend direction/color/percentage formatting helpers. |
packages/shared/src/interfaces/marketing-impact.interface.ts |
Adds view-model interfaces and marketing-impact tab/filter types. |
packages/shared/src/constants/marketing-impact.constants.ts |
Adds attribution model and funnel-stage option constants. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.ts |
Imports new tab/section components for the parent dashboard. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/marketing-impact.component.html |
Replaces placeholder tabs with real focused section components. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.ts |
Renames KPI data handling and imports embedded section components. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html |
Embeds all marketing sections below Overview KPI cards. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.ts |
Adds attribution data fetching/model selection and channel row shaping. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.html |
Adds attribution section UI/table. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.scss |
Adds component stylesheet header. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.ts |
Adds paid campaign KPI and project table logic. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.html |
Adds performance marketing tab UI. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/performance-marketing-tab/performance-marketing-tab.component.scss |
Adds component stylesheet header. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.ts |
Adds email KPI, email type, and campaign table logic. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.html |
Adds email marketing tab UI. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.scss |
Adds component stylesheet header. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.ts |
Adds web activity KPI and domain row logic. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.html |
Adds web activity tab UI. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.ts |
Adds social media KPI and platform row logic. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.html |
Adds social accounts tab UI. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.scss |
Adds component stylesheet header. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.ts |
Adds brand health KPI, sentiment, project, and mention logic. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.html |
Adds social listening tab UI. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.scss |
Adds component stylesheet header. |
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.html |
Generalizes KPI card test IDs and icon styling. |
Comments suppressed due to low confidence (2)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.html:77
- This visual table lacks table, row, columnheader, and cell semantics, so assistive technologies may read the campaign columns as unrelated text. Use a semantic table or add the same ARIA table roles used by the attribution/performance tables in this PR.
<div class="flex flex-col gap-0" data-testid="email-campaigns-table">
<!-- Table header -->
<div class="flex items-center gap-3 border-b border-gray-200 pb-2">
<span class="flex-1 text-[11px] font-semibold tracking-wide text-gray-400">CAMPAIGN</span>
<span class="w-24 text-[11px] font-semibold tracking-wide text-gray-400">TYPE</span>
<span class="w-20 text-right text-[11px] font-semibold tracking-wide text-gray-400">SENDS</span>
<span class="w-20 text-right text-[11px] font-semibold tracking-wide text-gray-400">OPENS</span>
<span class="w-20 text-right text-[11px] font-semibold tracking-wide text-gray-400">OPEN RATE</span>
<span class="w-16 text-right text-[11px] font-semibold tracking-wide text-gray-400">CTR</span>
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html:54
- The embedded sections do not receive the selected month, and a search shows
selectedMonthis only used to render labels, not to parameterize any data request. Changing the month picker will therefore leave these newly added Overview sections showing the same data under a different month label; pass the selected month through and include it in the section queries, or make the picker unavailable until month-scoped data is supported.
<lfx-attribution-section [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-attribution-section" />
<lfx-performance-marketing-tab [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-performance-marketing" />
<lfx-email-tab [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-email" />
<lfx-web-activity-tab [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-web-activity" />
<lfx-social-accounts-tab [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-social-accounts" />
<lfx-social-listening-tab [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-social-listening" />
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
Review Feedback AddressedCommit: cf1a547 Changes Made
No Code Change Needed
Threads Resolved11 of 11 review threads addressed and resolved. |
7 tasks
Contributor
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/sparkline-kpi-card/sparkline-kpi-card.component.html`:
- Line 13: Update the badge data-testid to be section-scoped to prevent
collisions by including the section identifier alongside the KPI id; replace the
current [attr.data-testid]="'kpi-card-badge-' + kpi().id" with a compound key
that includes the section id (for example "'kpi-card-badge-' + section().id +
'-' + kpi().id"), referencing the existing kpi() and section() accessors used in
this component so tests can target badges uniquely per section.
🪄 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: 8be1bbf9-621c-49e0-8d3b-bd880eae2187
📒 Files selected for processing (12)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/attribution-section/attribution-section.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/email-tab/email-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.htmlapps/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.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-accounts-tab/social-accounts-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/social-listening-tab/social-listening-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/sparkline-kpi-card/sparkline-kpi-card.component.tsapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.htmlapps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.tspackages/shared/src/utils/marketing-impact.utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html
- packages/shared/src/utils/marketing-impact.utils.ts
- apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/web-activity-tab/web-activity-tab.component.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/components/performance-marketing-tab/performance-marketing-tab.component.ts
Contributor
Author
Review Feedback Addressed (iteration 3)Commit: f2267d1 Changes Made
No Change Needed
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
mrautela365
force-pushed
the
feat/LFXV2-1644-overview-all-sections
branch
from
ce5bf01 to
5de8ffd
Compare
MRashad26
reviewed
May 18, 2026
Contributor
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Strict pass against ~/LFX/code-enforcer-agent.md. The architectural shift (Overview tab now embeds all six focused-tab components) introduces a perf/duplication concern in the data layer that's worth surfacing before this lands. The earlier computeMomPct extraction into shared utils is a nice follow-through on the PR #720 review.
mrautela365
added a commit
that referenced
this pull request
May 18, 2026
- overview-tab: remove embedded sub-tab components that caused duplicate HTTP requests (per MRashad26) - overview-tab: remove unused sub-tab component imports - social-listening: sort merged mentions by date descending to prevent positive-mention bias in top-5 list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Contributor
Author
Review Feedback AddressedCommit: 48d4cdb Changes Made
No Change Needed
Threads Resolved2 of 2 unresolved threads addressed in this iteration. |
mrautela365
added a commit
that referenced
this pull request
May 18, 2026
- overview-tab: remove embedded sub-tab components that caused duplicate HTTP requests (per MRashad26) - overview-tab: remove unused sub-tab component imports - social-listening: sort merged mentions by date descending to prevent positive-mention bias in top-5 list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
mrautela365
force-pushed
the
feat/LFXV2-1644-overview-all-sections
branch
from
5aae313 to
aecd2e5
Compare
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>
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>
Embed all tab components (attribution, performance marketing, email, web activity, social accounts, social listening) into the overview tab so users see the full dashboard summary. Individual tabs still show the focused detail view. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Guard non-finite percentages (Infinity/-Infinity) in trend utils - Add catchError to web-activity-tab data stream - Fix funnel stage filter to exclude empty/unknown stages - Add ARIA table roles to email, social-accounts, web-activity, social-listening tables - Add section input to sparkline-kpi-card for unique data-testids - Remove duplicate revenueKeyMap from attribution-section LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Use testId() + '-badge' instead of 'kpi-card-badge-' + kpi().id so the badge test selector is unique across sections on the Overview tab. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Email CTR changePercentage is computed vs 6-month average, not MoM. Relabel suffix from 'MoM' to 'vs avg' in email-tab and overview-tab. - Treat values that round to 0.0% as neutral (no trend arrow/color) to prevent misleading '-0.0%' with a down arrow. 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>
LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Replace off-brand bg-emerald-100/text-emerald-600 with the standard bg-green-100/text-green-600 used by all other marketing KPI cards. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Move computeMomPct to @lfx-one/shared/utils alongside the other trend helpers. Remove local copies from email-tab and performance-marketing-tab. Also cache .at() values in email-tab open-rate calculation to eliminate non-null assertions. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- overview-tab: remove embedded sub-tab components that caused duplicate HTTP requests (per MRashad26) - overview-tab: remove unused sub-tab component imports - social-listening: sort merged mentions by date descending to prevent positive-mention bias in top-5 list Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Weight engagement rate by impressions so high-volume platforms contribute proportionally to the headline KPI. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Switch email type and campaign row data-testid attributes from raw field values (emailType, name) to $index for stable, predictable selectors that avoid collisions from duplicate names. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Drop the last (current, potentially incomplete) month from the monthly impressions array before computing MoM percentage, so the comparison only uses completed months — matching the ROAS KPI query's cutoff behavior. LFXV2-1644 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
mrautela365
force-pushed
the
feat/LFXV2-1644-overview-all-sections
branch
from
58b3dbf to
aa01e9c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/app/modules/dashboards/marketing-impact/components/overview-tab/overview-tab.component.html:45
- The PR description says the Overview tab embeds all marketing impact sections (KPI → Attribution → Performance Marketing → Email → Web Activity → Social Accounts → Social Listening), but this template still only renders the KPI grid plus the attribution section. Either embed the remaining sections here (in the described order) or update the PR title/description to match the actual behavior.
@for (kpi of performanceSummaryKpis(); track kpi.id) {
<lfx-sparkline-kpi-card [kpi]="kpi" section="overview" />
} @empty {
<div class="col-span-full text-center py-8 text-sm text-gray-500" data-testid="overview-tab-kpi-empty">
No performance data available for this period.
</div>
}
}
</div>
</div>
<lfx-attribution-section [foundationSlug]="foundationSlug()" [foundationName]="foundationName()" data-testid="overview-tab-attribution-section" />
| 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
83
to
95
| switchMap((slug) => { | ||
| if (!slug) { | ||
| this.loading.set(false); | ||
| return of(null); | ||
| } | ||
| this.loading.set(true); | ||
| return this.analyticsService.getSocialReach(slug).pipe( | ||
| catchError(() => of(null)), | ||
| finalize(() => this.loading.set(false)) | ||
| tap(() => this.loading.set(false)), | ||
| catchError(() => { | ||
| this.loading.set(false); | ||
| return of(null); | ||
| }) | ||
| ); |
Contributor
|
Hi Misha — this is a clean, well-structured PR with strong review responsiveness. Full pass below. 1. Independent Review[minor] Impressions MoM compares partial current month to full prior month —
|
| Concern | Resolving commit |
|---|---|
Duplicate getEmailCtr HTTP request |
287cc1f1 |
| Duplicate render trees (all 6 tabs embedded in Overview) | 287cc1f1 |
4. Summary
Misha, the body of work here is impressive — the marketing impact dashboard has gone from concept to a polished, production-ready feature across a tight sequence of PRs. The review responsiveness has been excellent: 15+ commits addressing bot and human feedback with clear commit messages and per-thread acknowledgment.
Issue counts: 0 blocking · 2 minor · 2 nits
Final decision: ✅ Approved with minor comments. The two minor items (#impressions MoM, #PR description) are worth addressing before merge — the MoM issue in particular can surface misleading drops for end users in the first week of each month. Neither is blocking if the team is comfortable shipping as-is, but I'd encourage a quick fix for the impressions query cutoff.
🤖 Generated with Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
foundationSluginput)Test plan
/foundation/marketing-impact?project=tlf→ Overview tabLFXV2-1644
🤖 Generated with Claude Code