feat(dashboards): add ROAS metrics and drawer improvements#315
Closed
mrautela365 wants to merge 8 commits intomainfrom
Closed
feat(dashboards): add ROAS metrics and drawer improvements#315mrautela365 wants to merge 8 commits intomainfrom
mrautela365 wants to merge 8 commits intomainfrom
Conversation
Add three drill-down drawer components for the ED marketing dashboard: - Website Visits: daily trend chart + domain breakdown - Email CTR: monthly trend, campaign CTR breakdown, reach vs opens - Paid Social Reach: monthly impressions, channel breakdown Backend: Snowflake queries with foundation-scoped recursive CTE for all three endpoints. Frontend: signal-based drawers with Chart.js integration following the established drawer pattern. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Fix Email CTR and Paid Social Reach card subtitles from "Last 30 days" to "Last 6 months" to match actual query time range - Fix docstring on getEmailCtr to accurately describe foundation filtering (uses CTE, not "All Projects" row) - Add missing "Email CTR" section header in analytics-data interface - Fix "Social Reach" section header that was mislabeled as "Email CTR" LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Collapse nested guards on breakdown sections — use single `> 1` check to hide entire section instead of showing empty panel - Close active drawer on foundation change to prevent stale data - Use weighted CTR (clicks/sends) instead of AVG(CTR) for campaign aggregation accuracy LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
The SQL already multiplies by 100 (SUM(clicks)/SUM(sends) * 100), so the JS mapping should only round to 2 decimals (row.AVG_CTR * 100 / 100) instead of applying another 100x factor (row.AVG_CTR * 10000 / 100). LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 - Replace complex web activities CTE with Platinum views - Add ROAS as primary metric for paid social card - Move recommended actions and insights to first fold in drawers - Add ROAS trend chart to paid social drawer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
github-actions
bot
requested review from
andrest50,
asithade,
dealako and
mauriciozanettisalomao
Contributor
WalkthroughAdds four marketing analytics drawers (Website Visits, Email CTR, Paid Social Reach, Social Media), frontend service methods and template wiring, backend routes/controllers and ProjectService methods to supply analytics data, and shared interfaces/constants/enum updates to integrate the drawers into Marketing Overview. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant MktComp as Marketing Overview<br/>(Frontend)
participant AnalyticsService as Analytics Service<br/>(Frontend)
participant AnalyticsController as Analytics Controller<br/>(Backend)
participant ProjectService as Project Service
participant Snowflake as Snowflake DB
participant Drawer as Drawer Component
User->>MktComp: Load Marketing Overview
MktComp->>AnalyticsService: forkJoin([getWebActivitiesSummary(), getEmailCtr(), getSocialReach()]) with foundationSlug
par Web Activities
AnalyticsService->>AnalyticsController: GET /web-activities-summary
AnalyticsController->>ProjectService: getWebActivitiesSummary()
ProjectService->>Snowflake: Query web activity metrics
Snowflake-->>ProjectService: Aggregated results
ProjectService-->>AnalyticsController: WebActivitiesSummaryResponse
AnalyticsController-->>AnalyticsService: HTTP 200
and Email CTR
AnalyticsService->>AnalyticsController: GET /email-ctr
AnalyticsController->>ProjectService: getEmailCtr()
ProjectService->>Snowflake: Query email CTR metrics
Snowflake-->>ProjectService: Aggregated results
ProjectService-->>AnalyticsController: EmailCtrResponse
AnalyticsController-->>AnalyticsService: HTTP 200
and Social Reach
AnalyticsService->>AnalyticsController: GET /social-reach
AnalyticsController->>ProjectService: getSocialReach()
ProjectService->>Snowflake: Query social reach metrics
Snowflake-->>ProjectService: Aggregated results
ProjectService-->>AnalyticsController: SocialReachResponse
AnalyticsController-->>AnalyticsService: HTTP 200
end
AnalyticsService-->>MktComp: Responses collected
MktComp->>MktComp: Transform data -> marketingCards
User->>MktComp: Click metric card
MktComp->>MktComp: set activeDrawer(drawerType)
MktComp->>Drawer: Render drawer with bound data
Drawer->>Drawer: Compute chart data
User->>Drawer: Close drawer
Drawer->>MktComp: visibleChange -> handleDrawerClose()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds marketing dashboard plumbing (backend + shared types + Angular UI) to support web activities, email CTR, and paid social ROAS drill-down experiences.
Changes:
- Introduces new shared interfaces and drawer types for marketing analytics responses and drawers.
- Adds new backend analytics endpoints and Snowflake queries for web activities summary, email CTR, and paid social reach/ROAS.
- Updates the Executive Director marketing overview to load live data, render metric cards, and open drill-down drawers.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/dashboard-metric.interface.ts | Adds new marketing DashboardDrawerType enum values. |
| packages/shared/src/interfaces/analytics-data.interface.ts | Adds shared response/type definitions for marketing analytics (web, email, paid social, social media). |
| packages/shared/src/constants/dashboard-metrics.constants.ts | Replaces mock marketing metric card data with empty placeholders + drawer wiring. |
| apps/lfx-one/src/server/services/project.service.ts | Implements Snowflake queries and response shaping for new marketing analytics endpoints. |
| apps/lfx-one/src/server/routes/analytics.route.ts | Registers new /web-activities-summary, /email-ctr, /social-reach routes. |
| apps/lfx-one/src/server/controllers/analytics.controller.ts | Adds controller handlers and validation for new marketing endpoints. |
| apps/lfx-one/src/app/shared/services/analytics.service.ts | Adds Angular service methods to call the new marketing endpoints with fallback defaults. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.ts | New website visits drawer component (charts + first-fold actions/insights). |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.html | Drawer template for website visits. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.ts | New paid social drawer with ROAS + spend/revenue + charts. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html | Drawer template for paid social reach/ROAS. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts | Loads marketing data, transforms cards, and manages drawer state. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.html | Adds key insights card and mounts drill-down drawer components. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.ts | New email CTR drawer component (charts + first-fold actions/insights). |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.html | Drawer template for email CTR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
![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 (3)
apps/lfx-one/src/server/services/project.service.ts (1)
1825-1825: Add JSDoc forgetSocialReachpublic method.This method is public but undocumented, unlike neighboring public APIs.
As per coding guidelines: Add JSDoc comments for public functions and exported modules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/services/project.service.ts` at line 1825, Add a JSDoc comment block for the public method getSocialReach to document its purpose, parameters and return type; specifically add a brief description of what getSocialReach does, a `@param` {string} foundationSlug description, a `@returns` {Promise<SocialReachResponse>} description of the response shape (and mention any relevant fields), and any `@throws` or error behavior if applicable, placing the block immediately above the getSocialReach method declaration so it matches neighboring public API docs.apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts (1)
108-129: Consider using an enum or constant keys instead of title string matching.Matching cards by
card.title === 'Website Visits'is fragile - if titles change, this logic breaks silently. Consider using thedrawerTypeenum which is already defined on each card.♻️ Suggested improvement using drawerType
private initMarketingCards(): Signal<DashboardMetricCard[]> { return computed(() => { const { webActivities, emailCtr, socialReach } = this.marketingData(); const loading = this.marketingDataLoading(); return MARKETING_OVERVIEW_METRICS.map((card) => { - if (card.title === 'Website Visits') { + if (card.drawerType === DashboardDrawerType.MarketingWebsiteVisits) { return this.transformWebsiteVisits(card, webActivities, loading); } - if (card.title === 'Email CTR') { + if (card.drawerType === DashboardDrawerType.MarketingEmailCtr) { return this.transformEmailCtr(card, emailCtr, loading); } - if (card.title === 'Paid Social Reach') { + if (card.drawerType === DashboardDrawerType.MarketingPaidSocialReach) { return this.transformSocialReach(card, socialReach, loading); } - if (card.title === 'Social Media') { + if (card.drawerType === DashboardDrawerType.MarketingSocialMedia) { return this.transformSocialMedia(card); } return card; }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts` around lines 108 - 129, The current initMarketingCards function matches cards by fragile title strings; update it to use the card.drawerType enum/key instead of card.title so changes to display text won't break logic. In initMarketingCards (and where MARKETING_OVERVIEW_METRICS is mapped), replace each title comparison with checks against the drawerType values (e.g., compare card.drawerType to the enum/constant for WebsiteVisits, EmailCtr, PaidSocialReach, SocialMedia) and call the same transformers (transformWebsiteVisits, transformEmailCtr, transformSocialReach, transformSocialMedia) based on those drawerType cases.apps/lfx-one/src/server/controllers/analytics.controller.ts (1)
1735-1760: Missing JSDoc comment forgetSocialReachmethod.For consistency with the other new methods (
getWebActivitiesSummary,getEmailCtr) and the coding guidelines requiring JSDoc comments for public functions, add documentation.📝 Add JSDoc comment
+ /** + * GET /api/analytics/social-reach + * Get social reach data including ROAS metrics + */ public async getSocialReach(req: Request, res: Response, next: NextFunction): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/controllers/analytics.controller.ts` around lines 1735 - 1760, Add a JSDoc block above the public method getSocialReach describing its purpose (fetch social reach for a foundation), document parameters (req: Request, res: Response, next: NextFunction), the return type (Promise<void>), and any thrown errors (e.g., ServiceValidationError when foundationSlug is missing); follow the same tag/style used by getWebActivitiesSummary and getEmailCtr (include `@public`, `@param`, `@returns`, and `@throws`) so the method documentation is consistent with other public controller methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 23-26: The import SocialMediaDrawerComponent is unresolved and
causes build failure; either add the missing component file and export
SocialMediaDrawerComponent from
../social-media-drawer/social-media-drawer.component.ts, or remove the broken
import line and remove SocialMediaDrawerComponent from the component's imports
array (the imports list that currently includes EmailCtrDrawerComponent,
PaidSocialReachDrawerComponent, SocialMediaDrawerComponent,
WebsiteVisitsDrawerComponent) so the code no longer references the missing
symbol.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.ts`:
- Around line 6-10: Update the standalone component imports to avoid barrel
exports: replace the lfxColors import from '@lfx-one/shared/constants' with a
direct import from '@lfx-one/shared/constants/colors.constants' and replace the
types SocialReachResponse, MarketingRecommendedAction, MarketingKeyInsight
(currently imported from '@lfx-one/shared/interfaces') with direct imports from
'@lfx-one/shared/interfaces/analytics-data.interface'; update the import
statements that reference these symbols (lfxColors, SocialReachResponse,
MarketingRecommendedAction, MarketingKeyInsight) accordingly so the component
imports the specific module files rather than the package barrel.
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 1754-1758: The current EXISTS subquery in project.service.ts uses
bidirectional ILIKE '%name%' (e.g., the block comparing fp.NAME and
em.PROJECT_NAME) which causes fuzzy matches and metric bleed; replace these
substring comparisons with deterministic exact-key joins—prefer using a stable
identifier such as fp.id or fp.slug matched to em.project_id or em.project_slug
(or a normalized exact key like lower(fp.slug) = lower(em.project_slug)) in the
EXISTS clause, and apply the same change to the other occurrences referenced
(the blocks used by getSocialReach and the ranges noted) so all project matching
uses exact/normalized equality instead of ILIKE.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 108-129: The current initMarketingCards function matches cards by
fragile title strings; update it to use the card.drawerType enum/key instead of
card.title so changes to display text won't break logic. In initMarketingCards
(and where MARKETING_OVERVIEW_METRICS is mapped), replace each title comparison
with checks against the drawerType values (e.g., compare card.drawerType to the
enum/constant for WebsiteVisits, EmailCtr, PaidSocialReach, SocialMedia) and
call the same transformers (transformWebsiteVisits, transformEmailCtr,
transformSocialReach, transformSocialMedia) based on those drawerType cases.
In `@apps/lfx-one/src/server/controllers/analytics.controller.ts`:
- Around line 1735-1760: Add a JSDoc block above the public method
getSocialReach describing its purpose (fetch social reach for a foundation),
document parameters (req: Request, res: Response, next: NextFunction), the
return type (Promise<void>), and any thrown errors (e.g., ServiceValidationError
when foundationSlug is missing); follow the same tag/style used by
getWebActivitiesSummary and getEmailCtr (include `@public`, `@param`, `@returns`, and
`@throws`) so the method documentation is consistent with other public controller
methods.
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Line 1825: Add a JSDoc comment block for the public method getSocialReach to
document its purpose, parameters and return type; specifically add a brief
description of what getSocialReach does, a `@param` {string} foundationSlug
description, a `@returns` {Promise<SocialReachResponse>} description of the
response shape (and mention any relevant fields), and any `@throws` or error
behavior if applicable, placing the block immediately above the getSocialReach
method declaration so it matches neighboring public API docs.
🪄 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: 1b15dd87-3d78-4de4-ba84-98ab76154135
📒 Files selected for processing (15)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.tsapps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.tsapps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.tsapps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.tsapps/lfx-one/src/app/shared/services/analytics.service.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/routes/analytics.route.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/constants/dashboard-metrics.constants.tspackages/shared/src/interfaces/analytics-data.interface.tspackages/shared/src/interfaces/dashboard-metric.interface.ts
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-315.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts (3)
94-99: Inconsistent number formatting between chart ticks andformatNumber()method.The chart tick callbacks use
.toFixed(0)for thousands (e.g.,10K), whileformatNumber()uses.toFixed(1)(e.g.,10.0K). This creates visual inconsistency between axis labels and tooltips/stat displays.Consider extracting a shared formatting utility or aligning the precision:
♻️ Proposed fix to unify formatting
protected formatNumber(num: number): string { if (num >= 1_000_000) return `${(num / 1_000_000).toFixed(1)}M`; - if (num >= 1_000) return `${(num / 1_000).toFixed(1)}K`; + if (num >= 1_000) return `${(num / 1_000).toFixed(0)}K`; return num.toLocaleString(); }Or adjust the chart callbacks to use
.toFixed(1)for consistency with the method.Also applies to: 135-140, 160-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts` around lines 94 - 99, The chart tick callbacks (the callback functions formatting axis ticks) are using .toFixed(0) while the formatNumber() method uses .toFixed(1), causing inconsistent labels; update the code to use a single shared formatting utility or change the tick callbacks to call formatNumber() (or mirror its .toFixed(1) precision) so axis ticks and tooltips/stat displays match—apply this fix to the callback blocks around the shown snippet and the similar callback occurrences referenced (around the other callback ranges).
194-200: Limited color palette may cause visual issues with more than 5 platforms.The
backgroundColorarray only contains 5 colors. If the data includes more than 5 platforms, Chart.js will cycle through the array, causing duplicate colors that may confuse users when interpreting the chart.♻️ Proposed fix to generate sufficient colors dynamically
private initPlatformChartData(): Signal<ChartData<'bar'>> { return computed(() => { const { platforms } = this.data(); const sorted = [...platforms].sort((a, b) => b.followers - a.followers); + const blueShades = [ + lfxColors.blue[700], + lfxColors.blue[600], + lfxColors.blue[500], + lfxColors.blue[400], + lfxColors.blue[300], + lfxColors.blue[200], + lfxColors.blue[100], + ]; return { labels: sorted.map((p) => p.platform), datasets: [ { data: sorted.map((p) => p.followers), - backgroundColor: [lfxColors.blue[700], lfxColors.blue[500], lfxColors.blue[400], lfxColors.blue[300], lfxColors.blue[200]], + backgroundColor: sorted.map((_, i) => blueShades[i % blueShades.length]), borderRadius: { topLeft: 0, bottomLeft: 0, topRight: 4, bottomRight: 4 }, borderSkipped: 'start', }, ], }; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts` around lines 194 - 200, The backgroundColor array is fixed to 5 entries and will repeat when there are more than 5 platforms; change the chart dataset to compute backgroundColor dynamically (use the existing sorted array length) instead of the hardcoded array. Add a helper (e.g., getColorsForCount(count: number)) in the social-media-drawer.component that takes sorted.length and returns an array of colors by first using colors from lfxColors (blue variants) and, if more are needed, generating/interpolating additional shades or extending the palette so each data item gets a unique color; then set datasets[0].backgroundColor = getColorsForCount(sorted.length) where the chart is created. Ensure getColorsForCount is used wherever the dataset config is built so the chart always has a color per platform.
31-53: Hardcoded dummy data should be replaced with real data from the input.The
recommendedActionsandkeyInsightsarrays are hardcoded placeholders. Per the PR objectives, recommended actions and key insights should be populated from actual analytics data. Consider adding these to theSocialMediaResponseinterface and thedatainput, or clearly mark this as a TODO for follow-up.Do you want me to open an issue to track replacing this dummy data with backend-driven content?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts` around lines 31 - 53, The component currently uses hardcoded arrays recommendedActions and keyInsights; replace these with real data by extending the SocialMediaResponse interface to include recommendedActions and keyInsights fields, update the component to read them from the existing data input (e.g., this.data.recommendedActions / this.data.keyInsights) with safe fallbacks (empty arrays) and remove the hardcoded dummy arrays from the SocialMediaDrawerComponent; if backend fields are not yet available, add clear TODO comments on SocialMediaResponse and the data input to ensure these props are populated later.apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html (1)
44-48: Consider using LFX design token color classes for trend indicators.The template uses standard Tailwind
text-green-600andtext-red-600, while the TypeScript component useslfxColors.emeraldandlfxColors.redfor chart styling. Per the coding guidelines, styling should use LFX design tokens for consistency.If your Tailwind configuration maps LFX tokens to utility classes (e.g.,
text-emerald-600), consider updating to maintain visual consistency with the charts.♻️ Suggested alignment with LFX tokens
- <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-600' : 'text-red-600'"> + <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-emerald-600' : 'text-red-600'"> {{ data().changePercentage > 0 ? '+' : '' }}{{ data().changePercentage }}% </span> - <i class="text-sm" [class]="data().trend === 'up' ? 'fa-light fa-arrow-up text-green-600' : 'fa-light fa-arrow-down text-red-600'"></i> + <i class="text-sm" [class]="data().trend === 'up' ? 'fa-light fa-arrow-up text-emerald-600' : 'fa-light fa-arrow-down text-red-600'"></i>As per coding guidelines: "Styling should use Tailwind CSS with PrimeUI plugin integration and LFX design tokens (colors, font-sizes)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html` around lines 44 - 48, Replace the hardcoded Tailwind color classes in the template's trend indicator with the LFX design token classes used by the component (e.g., use the mapped utility classes corresponding to lfxColors.emerald and lfxColors.red such as text-emerald-600/text-red-600 or your project's LFX token names). Update the two [class] bindings on the span and i elements that reference data().trend and data().changePercentage so they apply the LFX token utility classes instead of text-green-600/text-red-600, ensuring the visual tokens match the TypeScript chart styling in the SocialMediaDrawer component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html`:
- Around line 44-48: Replace the hardcoded Tailwind color classes in the
template's trend indicator with the LFX design token classes used by the
component (e.g., use the mapped utility classes corresponding to
lfxColors.emerald and lfxColors.red such as text-emerald-600/text-red-600 or
your project's LFX token names). Update the two [class] bindings on the span and
i elements that reference data().trend and data().changePercentage so they apply
the LFX token utility classes instead of text-green-600/text-red-600, ensuring
the visual tokens match the TypeScript chart styling in the SocialMediaDrawer
component.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts`:
- Around line 94-99: The chart tick callbacks (the callback functions formatting
axis ticks) are using .toFixed(0) while the formatNumber() method uses
.toFixed(1), causing inconsistent labels; update the code to use a single shared
formatting utility or change the tick callbacks to call formatNumber() (or
mirror its .toFixed(1) precision) so axis ticks and tooltips/stat displays
match—apply this fix to the callback blocks around the shown snippet and the
similar callback occurrences referenced (around the other callback ranges).
- Around line 194-200: The backgroundColor array is fixed to 5 entries and will
repeat when there are more than 5 platforms; change the chart dataset to compute
backgroundColor dynamically (use the existing sorted array length) instead of
the hardcoded array. Add a helper (e.g., getColorsForCount(count: number)) in
the social-media-drawer.component that takes sorted.length and returns an array
of colors by first using colors from lfxColors (blue variants) and, if more are
needed, generating/interpolating additional shades or extending the palette so
each data item gets a unique color; then set datasets[0].backgroundColor =
getColorsForCount(sorted.length) where the chart is created. Ensure
getColorsForCount is used wherever the dataset config is built so the chart
always has a color per platform.
- Around line 31-53: The component currently uses hardcoded arrays
recommendedActions and keyInsights; replace these with real data by extending
the SocialMediaResponse interface to include recommendedActions and keyInsights
fields, update the component to read them from the existing data input (e.g.,
this.data.recommendedActions / this.data.keyInsights) with safe fallbacks (empty
arrays) and remove the hardcoded dummy arrays from the
SocialMediaDrawerComponent; if backend fields are not yet available, add clear
TODO comments on SocialMediaResponse and the data input to ensure these props
are populated later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 625d3908-5f6a-49a5-99bb-5082fc4c4f99
📒 Files selected for processing (2)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.ts
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html (1)
15-24: Use LFX tokenized color classes instead of raw Tailwind palette utilities.This template hard-codes
text-gray-*,bg-gray-*,border-gray-*,text-green-*,text-red-*,text-blue-*, andtext-amber-*throughout the drawer. Please swap those for the PrimeUI/LFX token utilities used elsewhere so the drawer stays themeable and consistent. As per coding guidelines, "Styling should use Tailwind CSS with PrimeUI plugin integration and LFX design tokens (colors, font-sizes)".Also applies to: 33-47, 55-80, 89-103, 111-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html` around lines 15 - 24, Replace all raw Tailwind color utilities in social-media-drawer.component.html (e.g., text-gray-*, bg-gray-*, border-gray-*, text-green-*, text-red-*, text-blue-*, text-amber-*) with the project’s LFX/PrimeUI token classes used elsewhere so the drawer remains themeable; update class attributes on elements such as the header h2 (data-testid="social-media-drawer-title"), the close button (data-testid="social-media-drawer-close"), and all list/item/status elements referenced in this template (lines noted in the review: ~15-24, 33-47, 55-80, 89-103, 111-169) to use the equivalent tokenized utilities (replace text-gray-900 -> e.g., lfx-text-primary or the project token for primary text, text-gray-500 -> lfx-text-muted, bg-gray-100 -> lfx-bg-surface, border-gray-* -> lfx-border, text-green-* / text-red-* / text-blue-* / text-amber-* -> lfx-text-success/error/info/warn respectively) so the class names match the PrimeUI plugin tokens used across the app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html`:
- Around line 44-47: The template currently treats any non-'up' trend as a
decline; update the bindings that use data().trend and data().changePercentage
in social-media-drawer.component (the span and i elements) to handle a neutral
state when changePercentage === 0: derive a neutral branch (e.g., treat trend
=== 'neutral' or compute data().changePercentage === 0) and adjust the class
binding and icon selection accordingly so zero shows a neutral color and neutral
icon (instead of red/down) — replace the simple ternaries with a three-way
conditional or ngClass/ngIf that distinguishes 'up', 'neutral' (0%), and 'down'.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html`:
- Around line 15-24: Replace all raw Tailwind color utilities in
social-media-drawer.component.html (e.g., text-gray-*, bg-gray-*, border-gray-*,
text-green-*, text-red-*, text-blue-*, text-amber-*) with the project’s
LFX/PrimeUI token classes used elsewhere so the drawer remains themeable; update
class attributes on elements such as the header h2
(data-testid="social-media-drawer-title"), the close button
(data-testid="social-media-drawer-close"), and all list/item/status elements
referenced in this template (lines noted in the review: ~15-24, 33-47, 55-80,
89-103, 111-169) to use the equivalent tokenized utilities (replace
text-gray-900 -> e.g., lfx-text-primary or the project token for primary text,
text-gray-500 -> lfx-text-muted, bg-gray-100 -> lfx-bg-surface, border-gray-* ->
lfx-border, text-green-* / text-red-* / text-blue-* / text-amber-* ->
lfx-text-success/error/info/warn respectively) so the class names match the
PrimeUI plugin tokens used across the app.
🪄 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: 28b9f727-ca9d-4ee9-a012-59ae49e58ae0
📒 Files selected for processing (1)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html (1)
13-24: Use LFX token utilities instead of raw Tailwind palette/font-size classes.This starts in the header and repeats throughout the template with utilities like
text-gray-*,border-gray-*,bg-*,text-sm,text-lg, andtext-2xl. Please switch these to the PrimeUI/LFX tokenized classes used elsewhere so theme overrides stay consistent.As per coding guidelines, "Styling should use Tailwind CSS with PrimeUI plugin integration and LFX design tokens (colors, font-sizes)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html` around lines 13 - 24, The template uses raw Tailwind utilities (e.g., text-gray-*, bg-*, border-*, text-sm, text-lg, text-2xl) in the PaidSocialReachDrawer component header (look for data-testid="paid-social-reach-drawer-header", the <h2> with data-testid="paid-social-reach-drawer-title", the subtitle <p>, the close <button> with (click)="onClose()", and the <i> icon); update those class attributes to use the project's PrimeUI/LFX tokenized utility classes instead of raw palette/font-size classes so theme overrides apply consistently (replace text-gray-500/text-gray-900 with the equivalent LFX text color tokens, swap text-sm/text-lg/text-xl with the LFX font-size tokens, and replace bg-*/hover:bg-* and any border colors with the LFX background/border token classes), keeping existing structure and data-testid attributes intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html`:
- Around line 46-64: The template in paid-social-reach-drawer.component.html
only renders Total Impressions and either ROAS or Month-over-Month but never
shows Revenue; update the drawer template to include a Revenue stat block
alongside Total Impressions (and ensure it appears in both branches of the
conditional that render ROAS/changePercentage), using the existing helpers like
formatNumber(data().revenue) and referencing data().revenue so the summary
surfaces revenue alongside ROAS and/or impressions; ensure the new block follows
the same markup pattern (flex-1 card with label and formatted value) and that
any conditional logic that previously returned two cards now returns three when
revenue is present.
- Around line 33-50: The template currently hides the ROAS block when
data().roas === 0 because the conditional uses data().roas > 0; update the
conditional in paid-social-reach-drawer.component.html to include zero (e.g.,
use data().roas >= 0 or simply remove the >0 check) so the ROAS metric
(displayed via data().roas.toFixed(2) + 'x') is always shown even when it equals
0.00x; keep the existing changePercentage and trend logic unchanged so the +/-%
indicator still only appears when data().changePercentage !== 0.
- Around line 55-62: The template currently treats any trend !== 'up' as
negative, causing 0% to render as down; update the bindings in
paid-social-reach-drawer.component.html to explicitly handle three states (up,
down, flat/neutral) using data().trend and data().changePercentage: change the
class binding on the percentage span and the icon binding to use a conditional
that checks for 'up' => green/up, 'down' => red/down, else => neutral (e.g.,
gray and a neutral icon or no icon), and adjust the displayed sign so you only
prefix '+' for values > 0 (no sign for 0). Target the two affected elements: the
percentage span and the <i> icon.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html`:
- Around line 13-24: The template uses raw Tailwind utilities (e.g.,
text-gray-*, bg-*, border-*, text-sm, text-lg, text-2xl) in the
PaidSocialReachDrawer component header (look for
data-testid="paid-social-reach-drawer-header", the <h2> with
data-testid="paid-social-reach-drawer-title", the subtitle <p>, the close
<button> with (click)="onClose()", and the <i> icon); update those class
attributes to use the project's PrimeUI/LFX tokenized utility classes instead of
raw palette/font-size classes so theme overrides apply consistently (replace
text-gray-500/text-gray-900 with the equivalent LFX text color tokens, swap
text-sm/text-lg/text-xl with the LFX font-size tokens, and replace
bg-*/hover:bg-* and any border colors with the LFX background/border token
classes), keeping existing structure and data-testid attributes intact.
🪄 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: 16112cea-57c3-4d50-b9da-a90c55a7b970
📒 Files selected for processing (1)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html
MRashad26
requested changes
Mar 25, 2026
Contributor
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Code Review: Marketing Analytics Dashboard
This PR adds marketing analytics capabilities (web activities, email CTR, paid social reach, social media) with new backend endpoints and frontend drawer components. While the feature scope is well-defined, there are 5 critical and 16 major issues that must be addressed before merging.
Critical Issues (5)
- S-1: New Snowflake queries use schemas (
ANALYTICS.PLATINUM.*,ANALYTICS.SILVER_DIM.*) with zero precedent in this codebase - S-2: Bidirectional ILIKE fuzzy join will produce false positive matches
- S-3:
getWebActivitiesSummarypasses foundation slug toPROJECT_SLUGfilter (semantic mismatch) - S-4: Insufficient input validation on
foundationSlugenables ILIKE pattern manipulation - Q-1: Hardcoded dummy data (fake Twitter 125K followers, etc.) will display as real data in production
Major Issues (16)
- S-5: No visible auth middleware on new analytics routes exposing financial data
- S-6: "Recursive" CTE only fetches 2 levels of project nesting
- S-7: Financial data (spend/revenue) exposed without data classification review
- A-1: Missing
OnPushchange detection on all new components - A-2:
toSignalpipeline missingfinalizesafety net for loading state - A-3: Outer
catchErroris dead code (inner catches prevent it from firing) - A-4: Drawer
[visible]one-way binding will desync on overlay close - C-1 to C-6: Multiple instances of raw HTML elements instead of shared components (
lfx-button,lfx-table,lfx-tag,lfx-message,lfx-card) - Q-2 to Q-4: Significant code duplication (
formatNumberx4, chart options 800+ lines, template markup x4)
Contributor
Author
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
Related
Test plan
Generated with Claude Code