feat(dashboards): add ED marketing dashboard with north star metrics and snowflake data#287
Closed
mrautela365 wants to merge 17 commits intomainfrom
Closed
feat(dashboards): add ED marketing dashboard with north star metrics and snowflake data#287mrautela365 wants to merge 17 commits intomainfrom
mrautela365 wants to merge 17 commits intomainfrom
Conversation
github-actions
bot
requested review from
andrest50,
asithade,
dealako and
mauriciozanettisalomao
|
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 four marketing drill‑down drawers (Website Visits, Email CTR, Paid Social Reach, Social Media), refactors Marketing Overview to reactive Signals with drawer state, adds backend analytics endpoints/services and shared analytics interfaces/enum values, and enforces TLF-only context for board‑scoped personas. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant MO as MarketingOverview
participant AS as AnalyticsService
participant AC as AnalyticsController
participant PS as ProjectService
participant DB as Database
participant Drawer as DrawerComponent
User->>MO: Load dashboard / interact
MO->>AS: getWebActivitiesSummary(foundationSlug)
MO->>AS: getEmailCtr(foundationSlug)
MO->>AS: getSocialReach(foundationSlug)
par Parallel backend fetch
AS->>AC: HTTP requests
AC->>PS: getWebActivitiesSummary / getEmailCtr / getSocialReach
PS->>DB: Execute aggregation queries
DB-->>PS: Return aggregated rows
PS-->>AC: Structured responses
AC-->>AS: JSON responses
end
AS-->>MO: Observables emit (with fallbacks)
MO->>MO: Transform responses -> marketingCards (Signals)
User->>MO: Click metric card
MO->>MO: Set activeDrawer
MO->>Drawer: Bind data to selected drawer
Drawer->>Drawer: Compute chart signals and render
User->>Drawer: Close
Drawer-->>MO: visibleChange -> clear activeDrawer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds live marketing analytics (website visits, email CTR, paid social reach) to the Executive Director dashboard, including new Snowflake-backed API endpoints and drill-down drawers, plus tighter persona-to-foundation context enforcement for board-scoped personas.
Changes:
- Introduces new marketing drawer types + replaces marketing metric cards’ mock data with API-driven placeholders and drawer wiring.
- Adds server-side Snowflake queries and
/api/analytics/*endpoints for web activities, email CTR, and paid social reach. - Adds Angular client service methods and new Executive Director drill-down drawer components to display the marketing metrics.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/dashboard-metric.interface.ts | Adds new DashboardDrawerType values for marketing drawers. |
| packages/shared/src/interfaces/analytics-data.interface.ts | Adds shared response/row interfaces for marketing analytics endpoints. |
| packages/shared/src/constants/dashboard-metrics.constants.ts | Updates marketing overview cards to use empty chart data + drawer types (removes mock chart/value data). |
| apps/lfx-one/src/server/services/project.service.ts | Adds Snowflake queries + transforms for web activities, email CTR, and paid social reach. |
| apps/lfx-one/src/server/routes/analytics.route.ts | Registers new marketing analytics routes. |
| apps/lfx-one/src/server/controllers/analytics.controller.ts | Adds controller handlers for the new marketing endpoints. |
| apps/lfx-one/src/app/shared/services/persona.service.ts | Enforces TLF-only context for board-scoped personas during initialization and persona changes. |
| apps/lfx-one/src/app/shared/services/analytics.service.ts | Adds client-side API calls for the new marketing analytics endpoints. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/* | New drawer component to display web activities summary and breakdown charts. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/* | New drawer component to display paid impressions trend + channel breakdown. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/* | New drawer component to display CTR trend + campaign breakdown + sends vs opens. |
| apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/* | Wires marketing cards to live data, adds loading/click handling, and mounts the new drawers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts
Outdated
Show resolved
Hide resolved
.../dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
apps/lfx-one/src/app/shared/services/persona.service.ts (1)
64-70: Consider unifying withisTlfOnlyPersonato eliminate duplication.The logic
persona === 'board-member' || persona === 'executive-director'is duplicated in theisTlfOnlyPersonacomputed signal (lines 21-24). If board-scoped personas evolve, both locations must be updated.♻️ Proposed refactor to unify the logic
// Centralized check for personas that require TLF-only context public readonly isTlfOnlyPersona = computed(() => { - const persona = this.currentPersona(); - return persona === 'board-member' || persona === 'executive-director'; + return this.isBoardScopedPersona(this.currentPersona()); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/shared/services/persona.service.ts` around lines 64 - 70, The board-scoped check is duplicated between isBoardScopedPersona(persona: PersonaType) and the isTlfOnlyPersona computed signal; extract the shared logic into a single reusable predicate (e.g., a function or constant like isBoardScoped or BOARD_SCOPED_PERSONAS) and have both isBoardScopedPersona and the isTlfOnlyPersona signal call that shared symbol so updates to the set of board-scoped personas only need to be made in one place.packages/shared/src/interfaces/analytics-data.interface.ts (1)
2690-2697: RenametotalReachto the metric this response actually carries.The raw rows are
TOTAL_IMPRESSIONS, and the paid social drawer labels the same value as impressions. Keeping the shared contract astotalReachmakes reach/impressions easy to conflate for the next consumer.♻️ Suggested contract rename
export interface SocialReachResponse { - totalReach: number; + totalImpressions: number; changePercentage: number; trend: 'up' | 'down'; monthlyData: number[]; monthlyLabels: string[]; channelGroups: SocialReachChannelGroup[]; }Update the paid social drawer consumers to read
totalImpressionsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/interfaces/analytics-data.interface.ts` around lines 2690 - 2697, The SocialReachResponse interface's totalReach field should be renamed to totalImpressions to match the underlying metric (raw rows: TOTAL_IMPRESSIONS); update the interface definition (SocialReachResponse) replacing totalReach with totalImpressions, then update all consumers and type references (e.g., paid social drawer components/handlers) to read totalImpressions instead of totalReach, adjust any mappings that populate this object (where TOTAL_IMPRESSIONS is used), and update related tests/serializers to use the new property name.apps/lfx-one/src/server/routes/analytics.route.ts (1)
134-141: Consider a short-lived cache for these marketing endpoints.The overview now loads these together, so every dashboard open or foundation switch can fan out into three Snowflake reads. A small TTL cache keyed by foundation would cut warehouse load and improve perceived dashboard latency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/routes/analytics.route.ts` around lines 134 - 141, The three marketing endpoints registered via router.get ('/web-activities-summary' -> analyticsController.getWebActivitiesSummary, '/email-ctr' -> analyticsController.getEmailCtr, '/social-reach' -> analyticsController.getSocialReach) should use a short-lived in-process cache keyed by foundation (and relevant query params) to avoid fanning out three Snowflake reads on every dashboard load; add a small TTL cache (e.g., 30–60s) either in the route handler wrapper or inside each controller method, compose the cache key from foundation id + request params, return cached response when present, and fall back to executing the controller logic and storing the result on miss; ensure the cache is safe for short-term metrics (no long-term persistence) and document TTL choice.apps/lfx-one/src/app/shared/services/analytics.service.ts (1)
840-852: Add the missing doc block forgetSocialReach().This is a new public
AnalyticsServicemethod, but it is the only new marketing API here without the JSDoc header used by the adjacent methods. Please document the parameter and response contract the same way asgetWebActivitiesSummary()andgetEmailCtr().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/app/shared/services/analytics.service.ts` around lines 840 - 852, Add a JSDoc header for the public method getSocialReach(foundationSlug: string) describing the parameter and the response contract (SocialReachResponse) similar to the adjacent methods getWebActivitiesSummary() and getEmailCtr(); include `@param` for foundationSlug, a short description of what the method returns, and an `@returns` annotation describing the Observable<SocialReachResponse> shape (totalReach, changePercentage, trend, monthlyData, monthlyLabels, channelGroups) so the public API is documented and consistent with coding guidelines.apps/lfx-one/src/server/controllers/analytics.controller.ts (1)
1735-1760: DocumentgetSocialReach()like the adjacent handlers.This new public controller method is the only marketing endpoint here without the JSDoc block that describes the route/query contract.
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/controllers/analytics.controller.ts` around lines 1735 - 1760, Add a JSDoc block above the public method getSocialReach in analytics.controller.ts matching the style of adjacent handlers: document the HTTP method/path, describe the purpose, list the query parameter foundationSlug (type string, required) and the expected response shape (totalReach, monthlyData), and include any thrown errors (ServiceValidationError) and examples if present in neighboring blocks; ensure the JSDoc tags (`@param`, `@returns`, `@throws`) and formatting follow the existing controller JSDoc conventions so the new method's comment is consistent with adjacent handlers.apps/lfx-one/src/server/services/project.service.ts (2)
1883-1884: Add the missing doc block forgetSocialReach().
getWebActivitiesSummary()andgetEmailCtr()both describe scope and return shape, but this new public service method drops that JSDoc header.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` around lines 1883 - 1884, Add a JSDoc header for the public method getSocialReach(foundationSlug: string): Promise<SocialReachResponse> describing the method's scope/purpose (e.g., "Fetch paid social impressions/reach for a foundation"), the parameter (foundationSlug) and the return shape (SocialReachResponse), mirroring the style and level of detail used in getWebActivitiesSummary() and getEmailCtr(); place the JSDoc immediately above the getSocialReach declaration and include `@param` and `@returns` tags referencing SocialReachResponse.
1679-1680: Avoid adding new DEBUG logs in server services.These request-less
logger.debug()calls add more DEBUG traffic on the shared service path. Please remove them or gate them to local development so the new endpoints do not reintroduce staging/production noise.Based on learnings, "suppress DEBUG-level logs in non-local (production/staging) environments across service code. Ensure that only INFO-level and above (WARN, ERROR) are emitted in non-local environments."
Also applies to: 1789-1790, 1883-1884
🤖 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` around lines 1679 - 1680, Remove the unconditional debug-only logging added inside the service method getWebActivitiesSummary (and the similar debug calls around the other noted locations) — either delete those logger.debug(...) calls or wrap them behind a local-dev guard so they only run in local development (use the existing environment check/util your project uses, e.g., an isLocal/isDevelopment flag or NODE_ENV check). Specifically, locate the logger.debug invocation inside the getWebActivitiesSummary method and the analogous debug calls at the other referenced spots and replace them with a conditional that emits DEBUG logs only when running in a local/dev environment, ensuring production/staging keep INFO+ only.
🤖 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/email-ctr-drawer/email-ctr-drawer.component.html`:
- Around line 62-73: The panel currently renders whenever
data().campaignGroups.length > 0 but hides the chart if length === 1, creating
an empty panel; update the logic in email-ctr-drawer.component.html so that when
data().campaignGroups.length === 1 you still render a meaningful visualization
(either render the single-bar chart using <lfx-chart> with campaignChartData()
and campaignChartOptions, or show a one-row fallback message/component) instead
of leaving it blank; apply the same fix to the analogous guards in the website
visits and paid social drawer templates to avoid duplicated empty panels.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.ts`:
- Around line 7-14: The component currently imports PrimeNG's DrawerModule
directly (DrawerModule in the imports array of EmailCtrDrawerComponent); instead
create a shared wrapper component (e.g., DrawerComponent) under
apps/lfx-one/src/app/shared/components/drawer/ that internally uses PrimeNG's
DrawerModule but exposes an LFX-styled API, export that wrapper so it can be
imported standalone, then remove the direct DrawerModule import from
email-ctr-drawer.component.ts and replace it with the new DrawerComponent in the
component imports array (keep ChartComponent as-is); ensure the wrapper's
selector/inputs match how drawers are used here so consuming components need no
further changes.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 195-201: The card subtitle "Last 30 days" is incorrect because
transformSocialReach builds totalReach from six monthly buckets; update
transformSocialReach in marketing-overview.component.ts to either (A) display
the most recent month instead of the six-month aggregate by pulling the latest
monthly bucket from the SocialReachResponse (e.g., use data.monthlyBuckets or
data.latestMonth value if present) and set value/subtitle accordingly, or (B)
keep the aggregate totalReach but change the subtitle string to reflect the real
window (e.g., "Last 6 months"); modify the logic around value/subtitle in
transformSocialReach to implement one of these two fixes and ensure
changePercentage behavior remains consistent.
- Around line 115-129: The pipeline keeps the previous foundation's drill-down
UI while new data loads; before the switchMap that triggers forkJoin (inside the
toSignal(combineLatest(...)) flow that uses this.browserReady,
this.selectedFoundationSlug$, and this.marketingDataLoading), clear any open
drill-down state/drawer so stale payloads aren't rendered—add a tap prior to
switchMap that calls the component method or state update that closes the drawer
(e.g., invoke a closeDrilldown/closeDrawer method or set the drilldown
signal/state to null) so the UI is reset before marketingDataLoading begins and
new data is fetched.
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 1852-1879: The current response builds campaignGroups from
campaignResult.rows but only includes campaignName, avgCtr, totalSends and
totalClicks; to support the reach-vs-opens grouped bar chart, include
per-campaign open totals (e.g., add totalOpens: row.TOTAL_OPENS) or add a
dedicated campaign-level dataset (e.g., campaignDataset =
campaignResult.rows.map(r => ({ campaignName: r.PROJECT_NAME, totalSends:
r.TOTAL_SENDS, totalOpens: r.TOTAL_OPENS, avgCtr: Math.round(r.AVG_CTR * 10000)
/ 100 }))). Update the mapping where campaignGroups is created (reference:
campaignGroups and campaignResult.rows) and return the new field (or enrich
campaignGroups) so the front-end can key sends and opens by campaignName.
- Around line 1705-1737: The DOMAIN_GROUP CASE in domain_classified is
classifying global LF hosts (LF Corporate / LF Events / LF Training / LFX
Platform) for every foundation; restrict those classifications to the
appropriate scope by tying the host ownership to project_domains and the
requested foundation: update the LEFT JOIN to include the foundation identifier
(use the same foundation param used by the endpoint) and change the WHEN clauses
so they only return the LF Corporate/Events/Training/LFX Platform labels when
the host is owned by that foundation (e.g., pd.FOUNDATION_ID = :foundationId) or
when the request is global; otherwise return NULL (so only pd.DOMAIN_HOST ->
'Project Websites' or NULL apply for foundation-scoped requests). Ensure you
reference domain_classified, project_domains, WEB_ACTIVITIES, and DOMAIN_GROUP
when making the change.
- Around line 1822-1838: Replace the unweighted AVG in campaignQuery with a
weighted CTR: compute WEIGHTED_CTR by summing each campaign's OVERALL_CTR
multiplied by its TOTAL_SENDS and dividing by the sum of TOTAL_SENDS (handle
zero total sends safely), select that alias instead of AVG(em.OVERALL_CTR) and
use the new WEIGHTED_CTR for ordering in the query so high-volume campaigns are
appropriately weighted; update the SELECT and ORDER BY in the query that uses
foundationCte and the ANALYTICS.PLATINUM.EMAIL_MARKETING_OVERALL_KPIS fields
em.OVERALL_CTR and em.TOTAL_SENDS accordingly.
In `@packages/shared/src/interfaces/analytics-data.interface.ts`:
- Around line 2690-2697: The SocialReachResponse interface's trend union
currently only allows 'up' | 'down', which prevents representing a neutral
month-over-month change; update the trend type in SocialReachResponse to include
a neutral value (e.g., add 'flat') so callers can map it to neutral styling, and
make the identical change to the other duplicate SocialReachResponse declaration
referenced in the diff (the second occurrence around the later block) so both
contracts accept the neutral state.
---
Nitpick comments:
In `@apps/lfx-one/src/app/shared/services/analytics.service.ts`:
- Around line 840-852: Add a JSDoc header for the public method
getSocialReach(foundationSlug: string) describing the parameter and the response
contract (SocialReachResponse) similar to the adjacent methods
getWebActivitiesSummary() and getEmailCtr(); include `@param` for foundationSlug,
a short description of what the method returns, and an `@returns` annotation
describing the Observable<SocialReachResponse> shape (totalReach,
changePercentage, trend, monthlyData, monthlyLabels, channelGroups) so the
public API is documented and consistent with coding guidelines.
In `@apps/lfx-one/src/app/shared/services/persona.service.ts`:
- Around line 64-70: The board-scoped check is duplicated between
isBoardScopedPersona(persona: PersonaType) and the isTlfOnlyPersona computed
signal; extract the shared logic into a single reusable predicate (e.g., a
function or constant like isBoardScoped or BOARD_SCOPED_PERSONAS) and have both
isBoardScopedPersona and the isTlfOnlyPersona signal call that shared symbol so
updates to the set of board-scoped personas only need to be made in one place.
In `@apps/lfx-one/src/server/controllers/analytics.controller.ts`:
- Around line 1735-1760: Add a JSDoc block above the public method
getSocialReach in analytics.controller.ts matching the style of adjacent
handlers: document the HTTP method/path, describe the purpose, list the query
parameter foundationSlug (type string, required) and the expected response shape
(totalReach, monthlyData), and include any thrown errors
(ServiceValidationError) and examples if present in neighboring blocks; ensure
the JSDoc tags (`@param`, `@returns`, `@throws`) and formatting follow the existing
controller JSDoc conventions so the new method's comment is consistent with
adjacent handlers.
In `@apps/lfx-one/src/server/routes/analytics.route.ts`:
- Around line 134-141: The three marketing endpoints registered via router.get
('/web-activities-summary' -> analyticsController.getWebActivitiesSummary,
'/email-ctr' -> analyticsController.getEmailCtr, '/social-reach' ->
analyticsController.getSocialReach) should use a short-lived in-process cache
keyed by foundation (and relevant query params) to avoid fanning out three
Snowflake reads on every dashboard load; add a small TTL cache (e.g., 30–60s)
either in the route handler wrapper or inside each controller method, compose
the cache key from foundation id + request params, return cached response when
present, and fall back to executing the controller logic and storing the result
on miss; ensure the cache is safe for short-term metrics (no long-term
persistence) and document TTL choice.
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 1883-1884: Add a JSDoc header for the public method
getSocialReach(foundationSlug: string): Promise<SocialReachResponse> describing
the method's scope/purpose (e.g., "Fetch paid social impressions/reach for a
foundation"), the parameter (foundationSlug) and the return shape
(SocialReachResponse), mirroring the style and level of detail used in
getWebActivitiesSummary() and getEmailCtr(); place the JSDoc immediately above
the getSocialReach declaration and include `@param` and `@returns` tags referencing
SocialReachResponse.
- Around line 1679-1680: Remove the unconditional debug-only logging added
inside the service method getWebActivitiesSummary (and the similar debug calls
around the other noted locations) — either delete those logger.debug(...) calls
or wrap them behind a local-dev guard so they only run in local development (use
the existing environment check/util your project uses, e.g., an
isLocal/isDevelopment flag or NODE_ENV check). Specifically, locate the
logger.debug invocation inside the getWebActivitiesSummary method and the
analogous debug calls at the other referenced spots and replace them with a
conditional that emits DEBUG logs only when running in a local/dev environment,
ensuring production/staging keep INFO+ only.
In `@packages/shared/src/interfaces/analytics-data.interface.ts`:
- Around line 2690-2697: The SocialReachResponse interface's totalReach field
should be renamed to totalImpressions to match the underlying metric (raw rows:
TOTAL_IMPRESSIONS); update the interface definition (SocialReachResponse)
replacing totalReach with totalImpressions, then update all consumers and type
references (e.g., paid social drawer components/handlers) to read
totalImpressions instead of totalReach, adjust any mappings that populate this
object (where TOTAL_IMPRESSIONS is used), and update related tests/serializers
to use the new property name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55ac0a11-022c-429c-8720-dd01ba9fbaba
📒 Files selected for processing (16)
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/app/shared/services/persona.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
...es/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.html
Outdated
Show resolved
Hide resolved
...ules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.ts
Show resolved
Hide resolved
.../dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts
Show resolved
Hide resolved
.../dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts (2)
139-222: Consider extracting common chart configuration to reduce duplication.All three transform methods build nearly identical
chartDataobjects with the sameborderColor,fill,tension,borderWidth, andpointRadiusvalues. A helper would reduce repetition.Example helper
private buildLineChartData(data: number[], labels: string[]): ChartData<'line'> { return { labels, datasets: [ { data, borderColor: lfxColors.blue[500], backgroundColor: hexToRgba(lfxColors.blue[500], 0.1), fill: true, tension: 0.4, borderWidth: 2, pointRadius: 0, }, ], }; }Then each transform method can call
this.buildLineChartData(data.dailyData, data.dailyLabels)instead of repeating the object literal.🤖 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 139 - 222, The three transformer methods (transformWebsiteVisits, transformEmailCtr, transformSocialReach) duplicate the same chart dataset configuration; extract that repeated object into a helper like buildLineChartData(labels: string[], data: number[]) and have each transformer call this helper when constructing chartData (use the existing labels/data properties: dailyLabels/dailyData and monthlyLabels/monthlyData), leaving the conditional logic that falls back to card.chartData intact; update references in those three functions to return the helper result instead of the inline dataset literal.
69-81: String-based card matching is fragile; prefer matching bydrawerType.Using
card.title === 'Website Visits'couples the transform logic to display strings. If a title changes for localization or UX reasons, this breaks silently. Match oncard.drawerTypeinstead.Suggested fix
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); } 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 69 - 81, Change the fragile string-based matching in the MARKETING_OVERVIEW_METRICS map to use a stable identifier: replace checks like card.title === 'Website Visits' with checks against card.drawerType (or the appropriate enum/value on the card object) and call the same transformer methods (transformWebsiteVisits, transformEmailCtr, transformSocialReach) based on card.drawerType; update the mapping in MarketingOverviewComponent (marketing-overview.component.ts) so each branch switches on card.drawerType instead of title, ensuring the card definitions include the correct drawerType values.
🤖 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 93-102: The default objects defaultEmailCtr and defaultSocialReach
currently set trend: 'up' which can show an incorrect positive indicator when no
data exists; update both defaultEmailCtr and defaultSocialReach (the default
objects used in marketing-overview.component.ts) to either set trend to 'flat'
(preferred once supported) or remove/leave trend undefined so the UI doesn't
display a misleading "up" state — adjust the EmailCtrResponse and the
corresponding social reach default object accordingly and ensure any consumer
code (render logic) can handle a missing/flat trend.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 139-222: The three transformer methods (transformWebsiteVisits,
transformEmailCtr, transformSocialReach) duplicate the same chart dataset
configuration; extract that repeated object into a helper like
buildLineChartData(labels: string[], data: number[]) and have each transformer
call this helper when constructing chartData (use the existing labels/data
properties: dailyLabels/dailyData and monthlyLabels/monthlyData), leaving the
conditional logic that falls back to card.chartData intact; update references in
those three functions to return the helper result instead of the inline dataset
literal.
- Around line 69-81: Change the fragile string-based matching in the
MARKETING_OVERVIEW_METRICS map to use a stable identifier: replace checks like
card.title === 'Website Visits' with checks against card.drawerType (or the
appropriate enum/value on the card object) and call the same transformer methods
(transformWebsiteVisits, transformEmailCtr, transformSocialReach) based on
card.drawerType; update the mapping in MarketingOverviewComponent
(marketing-overview.component.ts) so each branch switches on card.drawerType
instead of title, ensuring the card definitions include the correct drawerType
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a701bbc-269c-46d0-93bc-c7643c897c5a
📒 Files selected for processing (3)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/interfaces/analytics-data.interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/server/services/project.service.ts
.../dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.html (1)
40-44: Consider handling a neutral trend state.The current logic defaults any non-
'up'trend to red/down styling. IfchangePercentageis 0 ortrendis'neutral'/undefined, this shows misleading red styling.🎨 Optional: Add neutral state handling
- <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-600' : 'text-red-600'"> + <span class="text-2xl font-semibold" [ngClass]="{ + 'text-green-600': data().trend === 'up', + 'text-red-600': data().trend === 'down', + 'text-gray-600': data().trend !== 'up' && data().trend !== 'down' + }"> {{ 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> + `@if` (data().trend === 'up' || data().trend === 'down') { + <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> + }🤖 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/email-ctr-drawer/email-ctr-drawer.component.html` around lines 40 - 44, The span and icon bindings use a binary up/down check and render red for any non-'up' trend, so update the template logic around data(), trend and changePercentage to explicitly handle a neutral state (e.g., trend === 'neutral' or changePercentage === 0): change the span [class] binding to choose among 'text-green-600' (up), 'text-red-600' (down), and a neutral class like 'text-gray-500' (neutral), and update the icon [class] binding similarly to show an up icon for 'up', a down icon for 'down', and a neutral icon (or no color) for neutral; also ensure comparisons explicitly check for 'up' and 'down' rather than treating anything not 'up' as down so undefined/0 maps to neutral.apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts (1)
93-111: Defaulttrend: 'up'could be misleading for empty data states.When no data is available, both
defaultEmailCtranddefaultSocialReachdefault totrend: 'up'. While the impact is low since the cards show no values in this state, consider using'flat'(if the interface supports it) or conditionally omitting the trend indicator in the UI when data is absent.🤖 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 93 - 111, The defaults defaultEmailCtr (type EmailCtrResponse) and defaultSocialReach (type SocialReachResponse) set trend: 'up' for empty-data states; change the default trend to 'flat' (or another neutral value your UI supports) in both defaultEmailCtr and defaultSocialReach, and if the type disallows 'flat' update the type to accept it or make trend optional; additionally update the component UI logic in MarketingOverviewComponent to omit or hide the trend indicator when monthlyData/monthlyLabels are empty so the neutral default doesn't render misleadingly.
🤖 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/server/services/project.service.ts`:
- Around line 1864-1869: The avgCtr value is being multiplied twice (SQL already
does *100 and the mapping does Math.round(row.AVG_CTR * 10000) / 100), so update
the mapping that builds campaignGroups to stop the extra scaling: change the
expression for avgCtr to round the already-percent value (e.g.,
Math.round(row.AVG_CTR * 100) / 100 or just Number(row.AVG_CTR.toFixed(2)))
instead of multiplying by 10000; alternatively, remove the * 100 in the SQL that
produces AVG_CTR to match monthlyQuery and keep the current mapping—adjust
either the SQL or the mapping for consistency (refer to campaignGroups and
row.AVG_CTR, and the monthlyQuery for the alternate approach).
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/email-ctr-drawer/email-ctr-drawer.component.html`:
- Around line 40-44: The span and icon bindings use a binary up/down check and
render red for any non-'up' trend, so update the template logic around data(),
trend and changePercentage to explicitly handle a neutral state (e.g., trend ===
'neutral' or changePercentage === 0): change the span [class] binding to choose
among 'text-green-600' (up), 'text-red-600' (down), and a neutral class like
'text-gray-500' (neutral), and update the icon [class] binding similarly to show
an up icon for 'up', a down icon for 'down', and a neutral icon (or no color)
for neutral; also ensure comparisons explicitly check for 'up' and 'down' rather
than treating anything not 'up' as down so undefined/0 maps to neutral.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 93-111: The defaults defaultEmailCtr (type EmailCtrResponse) and
defaultSocialReach (type SocialReachResponse) set trend: 'up' for empty-data
states; change the default trend to 'flat' (or another neutral value your UI
supports) in both defaultEmailCtr and defaultSocialReach, and if the type
disallows 'flat' update the type to accept it or make trend optional;
additionally update the component UI logic in MarketingOverviewComponent to omit
or hide the trend indicator when monthlyData/monthlyLabels are empty so the
neutral default doesn't render misleadingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd9120b0-8da9-4217-878a-40f2acedef6e
📒 Files selected for processing (5)
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/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/website-visits-drawer/website-visits-drawer.component.htmlapps/lfx-one/src/server/services/project.service.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/lfx-one/src/server/services/project.service.ts (2)
1705-1730: Add a one-line “why” comment above this CASE.This branch intentionally mixes global LF buckets with foundation-scoped project websites, which is easy to misread as inconsistent scoping. A short inline note here would save future churn on the same question.
As per coding guidelines: "Document complex logic with inline comments explaining the why," and based on learnings this method intentionally aggregates global LF traffic for the shared LF buckets while only "Project Websites" is foundation-scoped.
🤖 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` around lines 1705 - 1730, Add a one-line "why" comment immediately above the CASE inside the domain_classified CTE explaining that this CASE intentionally mixes global LF buckets (e.g., 'LF Corporate', 'LF Events', 'LFX Platform', etc.) with a foundation-scoped 'Project Websites' bucket (checked via pd.DOMAIN_HOST) so readers understand the aggregation scope difference; place the comment near the CASE that starts in domain_classified to make the intent clear for future maintainers.
1883-1884: Add a JSDoc block forgetSocialReach.This is a new public method, but unlike the two neighboring additions it has no public API doc. Please add the same brief
@param/@returnsblock here.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` around lines 1883 - 1884, Add a JSDoc block above the public async getSocialReach(foundationSlug: string): Promise<SocialReachResponse> method that documents it as a public API; include a brief description and `@param` {string} foundationSlug explanation and `@returns` {Promise<SocialReachResponse>} description (mirror the style used on the two neighboring methods' JSDoc blocks) so the public method is properly documented per guidelines.
🤖 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/server/services/project.service.ts`:
- Line 1680: The logger.debug call in get_web_activities_summary should be gated
to local-only execution: wrap the logger.debug invocation(s) (e.g., the
logger.debug call named in get_web_activities_summary and the other debug calls
later in the file) with the same local-environment guard used elsewhere in the
service code (use the existing helper or check used across services for local
dev vs prod/staging, e.g., the project’s isLocal/isLocalEnvironment/isLocalDev
guard or process.env.NODE_ENV !== 'production'), so DEBUG logs are emitted only
in local development; apply the same change to the other logger.debug
occurrences referenced in this file.
- Around line 1760-1777: The date label generation is vulnerable to timezone
shifts because you create Date objects from date-only strings (dateKey / new
Date(dateStr)) and call toLocaleDateString without a timeZone; change the
parsing and formatting to be UTC-safe by constructing the Date as UTC (e.g.,
derive a Date via Date.UTC or append "T00:00:00Z" to dateKey when creating the
Date in the dailyLabels mapping) and pass timeZone: 'UTC' to toLocaleDateString;
update all places using new Date(row.SESSION_DATE), dateKey creation, and the
dailyLabels mapping (references: dateKey, new Date(row.SESSION_DATE), new
Date(dateStr), and toLocaleDateString) so labels always reflect the intended UTC
date.
---
Nitpick comments:
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 1705-1730: Add a one-line "why" comment immediately above the CASE
inside the domain_classified CTE explaining that this CASE intentionally mixes
global LF buckets (e.g., 'LF Corporate', 'LF Events', 'LFX Platform', etc.) with
a foundation-scoped 'Project Websites' bucket (checked via pd.DOMAIN_HOST) so
readers understand the aggregation scope difference; place the comment near the
CASE that starts in domain_classified to make the intent clear for future
maintainers.
- Around line 1883-1884: Add a JSDoc block above the public async
getSocialReach(foundationSlug: string): Promise<SocialReachResponse> method that
documents it as a public API; include a brief description and `@param` {string}
foundationSlug explanation and `@returns` {Promise<SocialReachResponse>}
description (mirror the style used on the two neighboring methods' JSDoc blocks)
so the public method is properly documented per guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc97202d-f15f-42a9-82c0-e63f716808fa
📒 Files selected for processing (1)
apps/lfx-one/src/server/services/project.service.ts
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-287.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Contributor
Code Review FeedbackGreat work overall — license headers, Critical1. Drawer data-flow pattern breaks established convention The existing drawer pattern in this codebase uses lazy-loading — each drawer fetches its own data when opened via This PR eagerly pre-fetches ALL three marketing datasets in the parent
Fix: Each drawer should inject 2. The WITH foundation_projects AS (
SELECT PROJECT_ID, SLUG FROM ANALYTICS.SILVER_DIM.PROJECTS WHERE SLUG = ?
UNION ALL
SELECT c.PROJECT_ID, c.SLUG FROM ANALYTICS.SILVER_DIM.PROJECTS c
INNER JOIN ANALYTICS.SILVER_DIM.PROJECTS p ON c.PARENT_PROJECT_SLUG = p.SLUG
WHERE p.SLUG = ? OR p.PARENT_PROJECT_SLUG = ?
)This only captures foundation + direct children + grandchildren. If the project hierarchy goes 3+ levels deep, projects will be missed. The existing Snowflake queries use pre-aggregated Also: this CTE is copy-pasted 3 times, and the email/social variants select Questions:
3. ILIKE fuzzy matching is dangerous for project name joins WHERE fp.NAME ILIKE '%' || em.PROJECT_NAME || '%'
OR em.PROJECT_NAME ILIKE '%' || fp.NAME || '%'This bidirectional fuzzy match will produce false positives. E.g., a project named "Go" would match "Google Cloud", "MongoDB", etc. Short project names are particularly vulnerable. Fix: Use exact matching on a shared key (project ID or slug), or at minimum equality on Major4. In {{ formatNumber(data().totalReach) }}Method calls in templates execute on every change detection cycle. The existing codebase uses Fix: Use a computed signal for the formatted value. 5. No error handling in new Existing methods like 6. The 7. Missing JSDoc on The other two new controller methods have proper JSDoc documenting the route. Minor8. Duplicate default values — Default response objects are defined in 3 places (frontend service 9. Inconsistent 10. 11. |
5 tasks
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/constants/dashboard-metrics.constants.ts (1)
162-165:⚠️ Potential issue | 🟡 MinorUpdate stale docblock for marketing metrics data source.
This comment still says the section is mock-data-only, but the current implementation is drawer/data-driven for multiple marketing cards. Keeping this stale is misleading for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/constants/dashboard-metrics.constants.ts` around lines 162 - 165, Update the stale docblock above the marketing overview metrics to reflect the current implementation: replace the note that says "mock data" and "will be populated from live API" with a concise description stating that these metrics are now drawer/data-driven and populated from the marketing cards data source (e.g., the MARKETING_OVERVIEW_METRICS or similarly named constant in this file), and remove or reword any misleading future-live-API language so maintainers know the metrics are driven by the drawer/data model rather than static mocks.
🧹 Nitpick comments (4)
apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.html (1)
33-39: Move these formatted labels into computed display state.Angular evaluates template expressions during change detection, and the style guide recommends moving non-trivial template logic into TypeScript/computed state. Expose preformatted strings/signals here instead of calling
formatNumber(...)inline. (angular.dev)🤖 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/website-visits-drawer/website-visits-drawer.component.html` around lines 33 - 39, Move the formatting out of the template by adding computed/display properties in the component class (e.g., create totalSessionsDisplay and totalPageViewsDisplay on WebsiteVisitsDrawerComponent as computed signals or getters that call formatNumber(data().totalSessions) and formatNumber(data().totalPageViews)), update the template to bind those new properties instead of calling formatNumber(...) inline, and ensure the computed values react to the same data() signal so change detection updates correctly.apps/lfx-one/src/app/shared/services/analytics.service.ts (1)
804-816: Pull the empty marketing responses into shared constants.These three
catchErrorbranches are all hand-maintaining zero-value payloads inline. Centralizing them asEMPTY_*constants will make later field additions much less error-prone and keep the app-side defaults aligned across all marketing drawers.Also applies to: 823-857
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/shared/services/analytics.service.ts` around lines 804 - 816, Create shared EMPTY_* constants for the zero-value marketing responses and replace the hard-coded return values in the catchError of getWebActivitiesSummary (and the other two similar methods referenced in the review) to return those constants; specifically, add an exported constant like EMPTY_WEB_ACTIVITIES_SUMMARY with the same shape as WebActivitiesSummaryResponse and update the catchError to return of(EMPTY_WEB_ACTIVITIES_SUMMARY) (do the analogous change for the other two methods that currently inline their empty payloads so all marketing defaults are centralized).apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.ts (1)
90-95: Unify compact-number formatting across chart ticks and drawer metrics.The current logic mixes
Kprecision (toFixed(0)in axis ticks vstoFixed(1)informatNumber), so the same value can render differently in the same drawer.♻️ Suggested refactor
+ private formatCompactNumber(value: number, fractionDigits = 1): string { + if (value >= 1_000_000) return `${(value / 1_000_000).toFixed(fractionDigits)}M`; + if (value >= 1_000) return `${(value / 1_000).toFixed(fractionDigits)}K`; + return value.toLocaleString(); + } 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`; - return num.toLocaleString(); + return this.formatCompactNumber(num, 1); }Then use
this.formatCompactNumber(Number(value), 1)in both tick callbacks.Also applies to: 128-133, 153-157
🤖 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/website-visits-drawer/website-visits-drawer.component.ts` around lines 90 - 95, Replace the inline compact-number logic in the tick callback with a call to the component's shared formatter so formatting is consistent: use this.formatCompactNumber(Number(value), 1) inside the axis tick callback (currently using Number(value) with toFixed logic) and do the same for the other tick/metric callbacks referenced (the similar blocks around the ranges noted), referencing the existing formatCompactNumber method to ensure all chart ticks and drawer metrics use the same precision and suffix rules.apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts (1)
113-127: Use a stable identifier instead of title string matching for card transforms.Dispatching by
card.titleis brittle. Switching tocard.drawerType(ortestId) avoids breakage from label changes/localization.♻️ Suggested refactor
return MARKETING_OVERVIEW_METRICS.map((card) => { - if (card.title === 'Website Visits') { - return this.transformWebsiteVisits(card, webActivities, loading); - } - if (card.title === 'Email CTR') { - return this.transformEmailCtr(card, emailCtr, loading); - } - if (card.title === 'Paid Social Reach') { - return this.transformSocialReach(card, socialReach, loading); - } - if (card.title === 'Social Media') { - return this.transformSocialMedia(card); - } - return card; + switch (card.drawerType) { + case DashboardDrawerType.MarketingWebsiteVisits: + return this.transformWebsiteVisits(card, webActivities, loading); + case DashboardDrawerType.MarketingEmailCtr: + return this.transformEmailCtr(card, emailCtr, loading); + case DashboardDrawerType.MarketingPaidSocialReach: + return this.transformSocialReach(card, socialReach, loading); + case DashboardDrawerType.MarketingSocialMedia: + return this.transformSocialMedia(card); + default: + 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 113 - 127, Replace brittle title-based dispatching in the MARKETING_OVERVIEW_METRICS mapping by using a stable identifier field (e.g., card.drawerType or card.testId) instead of card.title: update the mapping logic that currently checks card.title === 'Website Visits' / 'Email CTR' / 'Paid Social Reach' / 'Social Media' to check card.drawerType (or testId) values and call the same transform functions (transformWebsiteVisits, transformEmailCtr, transformSocialReach, transformSocialMedia) accordingly; ensure MARKETING_OVERVIEW_METRICS entries include the chosen stable identifier so the new checks reliably route each card to the correct transform function.
🤖 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/email-ctr-drawer/email-ctr-drawer.component.html`:
- Around line 37-44: The arrow icon currently uses data().trend even when
changePercentage === 0, so update the template logic in
email-ctr-drawer.component.html to only render a directional icon when
data().changePercentage !== 0; specifically change the i element's condition to
check both data().changePercentage !== 0 and data().trend === 'up' (or 'down')
so a flat month shows no arrow (or a neutral state), and ensure the displayed
sign for the percentage still uses data().changePercentage > 0 as it does now.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 220-227: The Email CTR card currently formats changePercentage
without an explicit sign while other cards (e.g., Social Reach at lines 255-256)
include a "+"; update transformEmailCtr (and the other marketing card
transformers referenced) to use a shared formatter helper (e.g.,
formatPercentageDelta or similar) to produce a consistent string like "+5%" or
"-3%" (and handle zero/undefined by returning undefined), and replace the inline
formatting logic in transformEmailCtr and the corresponding functions so all
cards use that helper for changePercentage.
- Around line 166-179: The current toSignal/combineLatest pipeline in
marketing-overview.component (the stream that calls
analyticsService.getWebActivitiesSummary, getEmailCtr and getSocialReach inside
the forkJoin) eagerly fetches all drawer analytics on every foundation change;
instead remove those three calls from the switchMap so the parent flow only
updates minimal state (e.g., marketingDataLoading/activeDrawer) and defer
fetching to the drawer-open handler tied to activeDrawer changes. Implement lazy
per-drawer fetch logic in the component method or effect that handles drawer
opens (observe activeDrawer or the drawerOpen handler) to call the appropriate
analyticsService method (getWebActivitiesSummary, getEmailCtr, or
getSocialReach) for the opened drawer and cache results per
foundationSlug+drawerType to avoid refetching; ensure errors/loading state are
handled per-drawer rather than in the parent combineLatest stream.
- Around line 65-85: The component currently exposes a static
SocialMediaResponse named socialMediaData and displays it as live data (loading:
false); change this to explicit mock/demo data and avoid rendering it as real
metrics by renaming to mockSocialMediaData (or adding an isDemo flag) and set
the component state to indicate demo data (e.g., loading: true or demoMode =
true) so the UI shows a clear “demo/sample data” badge or placeholder instead of
live metrics; alternatively wire the component to the real data source via the
existing service/fetch method and remove the hardcoded socialMediaData so only
fetched results (not the static SocialMediaResponse) are rendered.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/paid-social-reach-drawer/paid-social-reach-drawer.component.html`:
- Around line 33-64: The template currently hides the ROAS block whenever
data().roas is 0, which conflates "zero ROAS" with "no ROAS data"; change the
conditional in paid-social-reach-drawer.component.html to check presence rather
than >0 (e.g., use data().roas !== null && data().roas !== undefined or a
dedicated flag like data().hasRoas) so that a legitimate 0 value still renders
the ROAS KPI (keep data().changePercentage and data().trend logic unchanged and
still use formatNumber for totals).
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-drawer.component.html`:
- Around line 32-49: The three summary cards currently force a single-row
layout; update the container with a responsive breakpoint (e.g., switch from
"flex gap-4" to a stacked-on-narrow layout like "flex flex-col md:flex-row
gap-4" or use "flex flex-wrap") and ensure each card uses full width on narrow
screens (e.g., replace fixed "flex-1" with "w-full md:flex-1" or similar) so the
elements inside the cards (Total Followers, Platforms Active, Follower Growth)
stack cleanly on phone-width drawers; modify the div with
data-testid="social-media-drawer-stats" and the three child card divs to apply
these responsive classes.
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 1676-1723: Wrap the Snowflake query execution in
getWebActivitiesSummary with a try/catch that captures errors from
this.snowflakeService.execute (both summaryQuery and dailyQuery), logs a
structured warning via logger.warn or logger.warning including operation context
("getWeb_activities_summary"), foundationSlug, and the caught error, and
rethrows or maps the error per the service's classified error pattern; apply the
same try/catch + logger.warning pattern to the other two methods getEmailCtr and
getSocialReach around their snowflakeService.execute calls so each Snowflake
operation has an operation-specific structured warning log and follows the
project's error-handling conventions.
- Around line 1744-1762: The query uses bidirectional wildcard name matching
(fp.NAME ILIKE '%...%' OR em.PROJECT_NAME ILIKE '%...%') which returns false
positives; change the join/filter in monthlyQuery (and the other campaign
queries around the same area) to match on a stable identifier or exact
normalized name equality instead: join
ANALYTICS.PLATINUM.EMAIL_MARKETING_OVERALL_KPIS (alias em) to
foundation_projects (alias fp) using a shared key if available (e.g.,
fp.PROJECT_KEY = em.PROJECT_KEY), otherwise compare normalized values (e.g.,
LOWER(TRIM(fp.NAME)) = LOWER(TRIM(em.PROJECT_NAME))) rather than using ILIKE
with wildcards so the filter is exact and indexable; update the corresponding
WHERE/EXISTS conditions in monthlyQuery and the other two query strings
mentioned.
- Around line 1802-1804: The month-over-month delta is rounded to whole percent
in getEmailCtr but to one decimal in getSocialReach, causing inconsistent UI;
update the changePercentage calculation in getEmailCtr (and the other identical
block that uses currentCtr, previousCtr, changePercentage) to use one-decimal
precision instead of Math.round to an integer—compute the percent difference as
((currentCtr - previousCtr) / previousCtr) * 100 and round to one decimal (e.g.,
use toFixed(1) or Math.round(x * 10) / 10) so both getEmailCtr and
getSocialReach display the same single-decimal precision.
---
Outside diff comments:
In `@packages/shared/src/constants/dashboard-metrics.constants.ts`:
- Around line 162-165: Update the stale docblock above the marketing overview
metrics to reflect the current implementation: replace the note that says "mock
data" and "will be populated from live API" with a concise description stating
that these metrics are now drawer/data-driven and populated from the marketing
cards data source (e.g., the MARKETING_OVERVIEW_METRICS or similarly named
constant in this file), and remove or reword any misleading future-live-API
language so maintainers know the metrics are driven by the drawer/data model
rather than static mocks.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/marketing-overview/marketing-overview.component.ts`:
- Around line 113-127: Replace brittle title-based dispatching in the
MARKETING_OVERVIEW_METRICS mapping by using a stable identifier field (e.g.,
card.drawerType or card.testId) instead of card.title: update the mapping logic
that currently checks card.title === 'Website Visits' / 'Email CTR' / 'Paid
Social Reach' / 'Social Media' to check card.drawerType (or testId) values and
call the same transform functions (transformWebsiteVisits, transformEmailCtr,
transformSocialReach, transformSocialMedia) accordingly; ensure
MARKETING_OVERVIEW_METRICS entries include the chosen stable identifier so the
new checks reliably route each card to the correct transform function.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.html`:
- Around line 33-39: Move the formatting out of the template by adding
computed/display properties in the component class (e.g., create
totalSessionsDisplay and totalPageViewsDisplay on WebsiteVisitsDrawerComponent
as computed signals or getters that call formatNumber(data().totalSessions) and
formatNumber(data().totalPageViews)), update the template to bind those new
properties instead of calling formatNumber(...) inline, and ensure the computed
values react to the same data() signal so change detection updates correctly.
In
`@apps/lfx-one/src/app/modules/dashboards/executive-director/components/website-visits-drawer/website-visits-drawer.component.ts`:
- Around line 90-95: Replace the inline compact-number logic in the tick
callback with a call to the component's shared formatter so formatting is
consistent: use this.formatCompactNumber(Number(value), 1) inside the axis tick
callback (currently using Number(value) with toFixed logic) and do the same for
the other tick/metric callbacks referenced (the similar blocks around the ranges
noted), referencing the existing formatCompactNumber method to ensure all chart
ticks and drawer metrics use the same precision and suffix rules.
In `@apps/lfx-one/src/app/shared/services/analytics.service.ts`:
- Around line 804-816: Create shared EMPTY_* constants for the zero-value
marketing responses and replace the hard-coded return values in the catchError
of getWebActivitiesSummary (and the other two similar methods referenced in the
review) to return those constants; specifically, add an exported constant like
EMPTY_WEB_ACTIVITIES_SUMMARY with the same shape as WebActivitiesSummaryResponse
and update the catchError to return of(EMPTY_WEB_ACTIVITIES_SUMMARY) (do the
analogous change for the other two methods that currently inline their empty
payloads so all marketing defaults are centralized).
🪄 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: 4471ae17-6ca9-4ee9-9212-ba731d0310db
📒 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/social-media-drawer/social-media-drawer.component.htmlapps/lfx-one/src/app/modules/dashboards/executive-director/components/social-media-drawer/social-media-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/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
Comment on lines
+37
to
+44
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Month-over-Month</span> | ||
| <div class="flex items-center gap-2"> | ||
| <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-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> | ||
| </div> |
There was a problem hiding this comment.
Hide the arrow when the month is flat.
With changePercentage === 0, this still renders an up/down icon from trend, so a flat month is shown as movement. Use the same non-zero guard the ROAS card already uses, or render a neutral state here.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 41-41: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 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/email-ctr-drawer/email-ctr-drawer.component.html`
around lines 37 - 44, The arrow icon currently uses data().trend even when
changePercentage === 0, so update the template logic in
email-ctr-drawer.component.html to only render a directional icon when
data().changePercentage !== 0; specifically change the i element's condition to
check both data().changePercentage !== 0 and data().trend === 'up' (or 'down')
so a flat month shows no arrow (or a neutral state), and ensure the displayed
sign for the percentage still uses data().changePercentage > 0 as it does now.
Comment on lines
+65
to
+85
| protected readonly socialMediaData: SocialMediaResponse = { | ||
| totalFollowers: 284_500, | ||
| totalPlatforms: 5, | ||
| changePercentage: 8.3, | ||
| trend: 'up', | ||
| platforms: [ | ||
| { platform: 'Twitter/X', followers: 125_000, engagementRate: 3.2, postsLast30Days: 45, impressions: 1_250_000, iconClass: 'fa-brands fa-x-twitter' }, | ||
| { platform: 'LinkedIn', followers: 89_000, engagementRate: 4.8, postsLast30Days: 28, impressions: 890_000, iconClass: 'fa-brands fa-linkedin' }, | ||
| { platform: 'YouTube', followers: 42_000, engagementRate: 2.1, postsLast30Days: 8, impressions: 520_000, iconClass: 'fa-brands fa-youtube' }, | ||
| { platform: 'Mastodon', followers: 18_500, engagementRate: 5.4, postsLast30Days: 32, impressions: 185_000, iconClass: 'fa-brands fa-mastodon' }, | ||
| { platform: 'Bluesky', followers: 10_000, engagementRate: 6.1, postsLast30Days: 22, impressions: 95_000, iconClass: 'fa-brands fa-bluesky' }, | ||
| ], | ||
| monthlyData: [ | ||
| { month: 'Oct 2025', totalFollowers: 248_000, totalEngagements: 42_000 }, | ||
| { month: 'Nov 2025', totalFollowers: 256_000, totalEngagements: 45_500 }, | ||
| { month: 'Dec 2025', totalFollowers: 261_000, totalEngagements: 38_000 }, | ||
| { month: 'Jan 2026', totalFollowers: 270_000, totalEngagements: 48_200 }, | ||
| { month: 'Feb 2026', totalFollowers: 278_000, totalEngagements: 51_000 }, | ||
| { month: 'Mar 2026', totalFollowers: 284_500, totalEngagements: 53_400 }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
Don’t render hardcoded social metrics as live dashboard data.
socialMediaData is static and foundation-agnostic, but it is displayed as real analytics (loading: false). This can mislead users when they switch foundations.
Also applies to: 278-303
🤖 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 65 - 85, The component currently exposes a static
SocialMediaResponse named socialMediaData and displays it as live data (loading:
false); change this to explicit mock/demo data and avoid rendering it as real
metrics by renaming to mockSocialMediaData (or adding an isDemo flag) and set
the component state to indicate demo data (e.g., loading: true or demoMode =
true) so the UI shows a clear “demo/sample data” badge or placeholder instead of
live metrics; alternatively wire the component to the real data source via the
existing service/fetch method and remove the hardcoded socialMediaData so only
fetched results (not the static SocialMediaResponse) are rendered.
Comment on lines
+166
to
+179
| return toSignal( | ||
| combineLatest([toObservable(this.browserReady), this.selectedFoundationSlug$]).pipe( | ||
| filter(([ready, slug]) => ready && !!slug), | ||
| map(([, slug]) => slug), | ||
| tap(() => { | ||
| this.marketingDataLoading.set(true); | ||
| this.activeDrawer.set(null); | ||
| }), | ||
| switchMap((foundationSlug) => | ||
| forkJoin({ | ||
| webActivities: this.analyticsService.getWebActivitiesSummary(foundationSlug), | ||
| emailCtr: this.analyticsService.getEmailCtr(foundationSlug), | ||
| socialReach: this.analyticsService.getSocialReach(foundationSlug), | ||
| }).pipe( |
There was a problem hiding this comment.
Avoid eager prefetch of all drawer analytics in the parent flow.
Line 175-Line 178 executes three analytics calls on every foundation change, even when no drawer is opened. This creates unnecessary Snowflake load and prevents per-drawer loading/error behavior. Prefer lazy fetch per drawer open (with per-foundation caching if needed).
🤖 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 166 - 179, The current toSignal/combineLatest pipeline in
marketing-overview.component (the stream that calls
analyticsService.getWebActivitiesSummary, getEmailCtr and getSocialReach inside
the forkJoin) eagerly fetches all drawer analytics on every foundation change;
instead remove those three calls from the switchMap so the parent flow only
updates minimal state (e.g., marketingDataLoading/activeDrawer) and defer
fetching to the drawer-open handler tied to activeDrawer changes. Implement lazy
per-drawer fetch logic in the component method or effect that handles drawer
opens (observe activeDrawer or the drawerOpen handler) to call the appropriate
analyticsService method (getWebActivitiesSummary, getEmailCtr, or
getSocialReach) for the opened drawer and cache results per
foundationSlug+drawerType to avoid refetching; ensure errors/loading state are
handled per-drawer rather than in the parent combineLatest stream.
Comment on lines
+220
to
+227
| private transformEmailCtr(card: DashboardMetricCard, data: EmailCtrResponse, loading: boolean): DashboardMetricCard { | ||
| return { | ||
| ...card, | ||
| loading, | ||
| value: data.currentCtr > 0 ? `${data.currentCtr.toFixed(1)}%` : undefined, | ||
| subtitle: data.currentCtr > 0 ? 'Last 6 months' : undefined, | ||
| changePercentage: data.changePercentage !== 0 ? `${data.changePercentage}%` : undefined, | ||
| trend: data.trend, |
There was a problem hiding this comment.
Standardize percentage delta formatting across marketing cards.
Email CTR omits explicit + while Social Reach includes it. Use one formatter helper so all cards present change consistently.
💡 Suggested fix
+ private formatPercentDelta(delta: number): string | undefined {
+ if (delta === 0) return undefined;
+ return `${delta > 0 ? '+' : ''}${delta}%`;
+ }
private transformEmailCtr(card: DashboardMetricCard, data: EmailCtrResponse, loading: boolean): DashboardMetricCard {
return {
...card,
loading,
value: data.currentCtr > 0 ? `${data.currentCtr.toFixed(1)}%` : undefined,
subtitle: data.currentCtr > 0 ? 'Last 6 months' : undefined,
- changePercentage: data.changePercentage !== 0 ? `${data.changePercentage}%` : undefined,
+ changePercentage: this.formatPercentDelta(data.changePercentage),
trend: data.trend,
...
};
}
private transformSocialReach(card: DashboardMetricCard, data: SocialReachResponse, loading: boolean): DashboardMetricCard {
return {
...card,
loading,
value: this.getSocialReachValue(data),
subtitle: this.getSocialReachSubtitle(data),
- changePercentage: data.changePercentage !== 0 ? `${data.changePercentage > 0 ? '+' : ''}${data.changePercentage}%` : undefined,
+ changePercentage: this.formatPercentDelta(data.changePercentage),
trend: data.trend,
...
};
}Also applies to: 255-256
🤖 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 220 - 227, The Email CTR card currently formats changePercentage
without an explicit sign while other cards (e.g., Social Reach at lines 255-256)
include a "+"; update transformEmailCtr (and the other marketing card
transformers referenced) to use a shared formatter helper (e.g.,
formatPercentageDelta or similar) to produce a consistent string like "+5%" or
"-3%" (and handle zero/undefined by returning undefined), and replace the inline
formatting logic in transformEmailCtr and the corresponding functions so all
cards use that helper for changePercentage.
Comment on lines
+33
to
+64
| @if (data().roas > 0) { | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">ROAS</span> | ||
| <div class="flex items-center gap-2"> | ||
| <span class="text-2xl font-semibold text-gray-900">{{ data().roas.toFixed(2) }}x</span> | ||
| @if (data().changePercentage !== 0) { | ||
| <span class="text-sm font-medium" [class]="data().trend === 'up' ? 'text-green-600' : 'text-red-600'"> | ||
| <i [class]="data().trend === 'up' ? 'fa-light fa-arrow-up' : 'fa-light fa-arrow-down'"></i> | ||
| {{ data().changePercentage > 0 ? '+' : '' }}{{ data().changePercentage }}% | ||
| </span> | ||
| } | ||
| </div> | ||
| </div> | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Total Impressions</span> | ||
| <span class="text-2xl font-semibold text-gray-900">{{ formatNumber(data().totalReach) }}</span> | ||
| </div> | ||
| } @else { | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Total Impressions</span> | ||
| <span class="text-2xl font-semibold text-gray-900">{{ formatNumber(data().totalReach) }}</span> | ||
| </div> | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Month-over-Month</span> | ||
| <div class="flex items-center gap-2"> | ||
| <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-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> | ||
| </div> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
Don't treat roas === 0 as “no ROAS data.”
apps/lfx-one/src/server/services/project.service.ts returns roas as 0 both when there is no spend and when there was spend but zero attributed revenue. With the current @if (data().roas > 0) guard, that second case hides the ROAS KPI entirely and swaps in a different stat, which misreports a real outcome as missing data.
🩹 Safer guard
- `@if` (data().roas > 0) {
+ `@if` (data().totalSpend > 0 || data().monthlyRoas.length > 0) {🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 33-33: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 41-41: Special characters must be escaped : [ > ].
(spec-char-escape)
[error] 59-59: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 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 33 - 64, The template currently hides the ROAS block whenever
data().roas is 0, which conflates "zero ROAS" with "no ROAS data"; change the
conditional in paid-social-reach-drawer.component.html to check presence rather
than >0 (e.g., use data().roas !== null && data().roas !== undefined or a
dedicated flag like data().hasRoas) so that a legitimate 0 value still renders
the ROAS KPI (keep data().changePercentage and data().trend logic unchanged and
still use formatNumber for totals).
Comment on lines
+32
to
+49
| <div class="flex gap-4" data-testid="social-media-drawer-stats"> | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Total Followers</span> | ||
| <span class="text-2xl font-semibold text-gray-900">{{ formatNumber(data().totalFollowers) }}</span> | ||
| </div> | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Platforms Active</span> | ||
| <span class="text-2xl font-semibold text-gray-900">{{ data().totalPlatforms }}</span> | ||
| </div> | ||
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | ||
| <span class="text-sm text-gray-500">Follower Growth</span> | ||
| <div class="flex items-center gap-2"> | ||
| <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-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> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Make the summary cards stack on narrow drawers.
This row always renders as three columns. On the phone-width drawer sizes configured above, the cards get squeezed hard enough that the label/value pairs will wrap awkwardly or truncate. Add a stack/wrap breakpoint here before shipping the responsive view.
📱 Minimal fix
- <div class="flex gap-4" data-testid="social-media-drawer-stats">
+ <div class="flex flex-col gap-4 sm:flex-row" data-testid="social-media-drawer-stats">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div class="flex gap-4" data-testid="social-media-drawer-stats"> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Total Followers</span> | |
| <span class="text-2xl font-semibold text-gray-900">{{ formatNumber(data().totalFollowers) }}</span> | |
| </div> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Platforms Active</span> | |
| <span class="text-2xl font-semibold text-gray-900">{{ data().totalPlatforms }}</span> | |
| </div> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Follower Growth</span> | |
| <div class="flex items-center gap-2"> | |
| <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-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> | |
| </div> | |
| </div> | |
| <div class="flex flex-col gap-4 sm:flex-row" data-testid="social-media-drawer-stats"> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Total Followers</span> | |
| <span class="text-2xl font-semibold text-gray-900">{{ formatNumber(data().totalFollowers) }}</span> | |
| </div> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Platforms Active</span> | |
| <span class="text-2xl font-semibold text-gray-900">{{ data().totalPlatforms }}</span> | |
| </div> | |
| <div class="flex-1 flex flex-col gap-1 p-4 border border-gray-200 rounded-lg"> | |
| <span class="text-sm text-gray-500">Follower Growth</span> | |
| <div class="flex items-center gap-2"> | |
| <span class="text-2xl font-semibold" [class]="data().trend === 'up' ? 'text-green-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> | |
| </div> | |
| </div> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 45-45: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 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 32 - 49, The three summary cards currently force a single-row
layout; update the container with a responsive breakpoint (e.g., switch from
"flex gap-4" to a stacked-on-narrow layout like "flex flex-col md:flex-row
gap-4" or use "flex flex-wrap") and ensure each card uses full width on narrow
screens (e.g., replace fixed "flex-1" with "w-full md:flex-1" or similar) so the
elements inside the cards (Total Followers, Platforms Active, Follower Growth)
stack cleanly on phone-width drawers; modify the div with
data-testid="social-media-drawer-stats" and the three child card divs to apply
these responsive classes.
Comment on lines
+1676
to
+1723
| public async getWebActivitiesSummary(foundationSlug: string): Promise<WebActivitiesSummaryResponse> { | ||
| logger.debug(undefined, 'get_web_activities_summary', 'Fetching web activities summary from Snowflake', { foundation_slug: foundationSlug }); | ||
|
|
||
| // Query 1: Total sessions & page views per domain classification | ||
| const summaryQuery = ` | ||
| SELECT | ||
| LF_SUB_DOMAIN_CLASSIFICATION, | ||
| SUM(TOTAL_SESSIONS_LAST_30_DAYS) AS TOTAL_SESSIONS, | ||
| SUM(TOTAL_PAGE_VIEWS_LAST_30_DAYS) AS TOTAL_PAGE_VIEWS | ||
| FROM ANALYTICS.PLATINUM.WEB_ACTIVITIES_SUMMARY | ||
| WHERE PROJECT_SLUG = ? | ||
| GROUP BY LF_SUB_DOMAIN_CLASSIFICATION | ||
| ORDER BY TOTAL_SESSIONS DESC | ||
| `; | ||
|
|
||
| // Query 2: Daily sessions for trend chart | ||
| const dailyQuery = ` | ||
| SELECT | ||
| ACTIVITY_DATE, | ||
| SUM(DAILY_SESSIONS) AS DAILY_SESSIONS | ||
| FROM ANALYTICS.PLATINUM.WEB_ACTIVITIES_BY_PROJECT | ||
| WHERE PROJECT_SLUG = ? | ||
| GROUP BY ACTIVITY_DATE | ||
| ORDER BY ACTIVITY_DATE ASC | ||
| `; | ||
|
|
||
| const [summaryResult, dailyResult] = await Promise.all([ | ||
| this.snowflakeService.execute<{ LF_SUB_DOMAIN_CLASSIFICATION: string; TOTAL_SESSIONS: number; TOTAL_PAGE_VIEWS: number }>(summaryQuery, [foundationSlug]), | ||
| this.snowflakeService.execute<{ ACTIVITY_DATE: string; DAILY_SESSIONS: number }>(dailyQuery, [foundationSlug]), | ||
| ]); | ||
|
|
||
| const domainGroups = summaryResult.rows.map((row) => ({ | ||
| domainGroup: row.LF_SUB_DOMAIN_CLASSIFICATION || 'Other', | ||
| totalSessions: row.TOTAL_SESSIONS, | ||
| totalPageViews: row.TOTAL_PAGE_VIEWS, | ||
| })); | ||
|
|
||
| const totalSessions = domainGroups.reduce((sum, g) => sum + g.totalSessions, 0); | ||
| const totalPageViews = domainGroups.reduce((sum, g) => sum + g.totalPageViews, 0); | ||
|
|
||
| const dailyData = dailyResult.rows.map((row) => row.DAILY_SESSIONS); | ||
| const dailyLabels = dailyResult.rows.map((row) => { | ||
| const date = new Date(row.ACTIVITY_DATE); | ||
| return date.toLocaleDateString('en-US', { month: 'short', day: 'numeric' }); | ||
| }); | ||
|
|
||
| return { totalSessions, totalPageViews, domainGroups, dailyData, dailyLabels }; | ||
| } |
There was a problem hiding this comment.
Add structured warning logs around the new Snowflake queries.
getWebActivitiesSummary(), getEmailCtr(), and getSocialReach() all execute Snowflake work without the try/catch + logger.warning(...) pattern already used elsewhere in this service. If one of these queries starts failing in production, we'll get the exception but lose the operation-specific warning context that makes these incidents diagnosable. As per coding guidelines, "Structured logging with Pino should be used for all backend logging" and "Error handling should follow the classified error architecture documented in error-handling-architecture.md".
Also applies to: 1731-1823, 1825-1944
🤖 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` around lines 1676 -
1723, Wrap the Snowflake query execution in getWebActivitiesSummary with a
try/catch that captures errors from this.snowflakeService.execute (both
summaryQuery and dailyQuery), logs a structured warning via logger.warn or
logger.warning including operation context ("getWeb_activities_summary"),
foundationSlug, and the caught error, and rethrows or maps the error per the
service's classified error pattern; apply the same try/catch + logger.warning
pattern to the other two methods getEmailCtr and getSocialReach around their
snowflakeService.execute calls so each Snowflake operation has an
operation-specific structured warning log and follows the project's
error-handling conventions.
Comment on lines
+1802
to
+1804
| const currentCtr = monthlyData[monthlyData.length - 1]; | ||
| const previousCtr = monthlyData.length >= 2 ? monthlyData[monthlyData.length - 2] : currentCtr; | ||
| const changePercentage = previousCtr > 0 ? Math.round(((currentCtr - previousCtr) / previousCtr) * 100) : 0; |
There was a problem hiding this comment.
Use one precision rule for marketing deltas.
getEmailCtr() rounds month-over-month change to a whole percent, while getSocialReach() rounds to one decimal place. These values render side-by-side in the new drawers, so the mismatch looks accidental. Normalize both methods to the same precision.
Also applies to: 1915-1917
🤖 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` around lines 1802 -
1804, The month-over-month delta is rounded to whole percent in getEmailCtr but
to one decimal in getSocialReach, causing inconsistent UI; update the
changePercentage calculation in getEmailCtr (and the other identical block that
uses currentCtr, previousCtr, changePercentage) to use one-decimal precision
instead of Math.round to an integer—compute the percent difference as
((currentCtr - previousCtr) / previousCtr) * 100 and round to one decimal (e.g.,
use toFixed(1) or Math.round(x * 10) / 10) so both getEmailCtr and
getSocialReach display the same single-decimal precision.
Add new executive-director persona and dedicated marketing dashboard with Marketing Overview cards, My Meetings, and Pending Actions sections. Key changes: - New ED dashboard component reusing foundation-health, my-meetings, and pending-actions shared components - Extended foundation-health with customFilterOptions, additionalCards, and hideBuiltInCards inputs for reusable metric card display - Added executive-director to PersonaType and persona options - Consolidated TLF-only enforcement into PersonaService - Migrated FilterPillOption interface to shared package - Added trend/changePercentage support to metric-card component LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
- Centralize board-scoped persona checks: sidebar and dev-toolbar now use personaService.isBoardScopedPersona() instead of inline logic - Remove redundant takeUntilDestroyed() inside toSignal() in ED dashboard - Fix misleading JSDoc on FilterPillOption.id - Update HTML comment from "Board Member" to "Board-Level Persona" - Document availableProjects timing in enforceTlfOnlyContext() LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Clear stale foundation selection in enforceTlfOnlyContext() to prevent a previously selected non-TLF foundation from persisting when availableProjects is not yet populated during early initialization. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Revert the PersonaService consolidation and restore the original inline board-member check pattern used across the codebase. Each component now adds `|| persona === 'executive-director'` alongside the existing `board-member` check, matching the established pattern. This preserves the existing code structure that the engineering team is familiar with, rather than introducing a new centralized pattern. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 - Filter empty slug from activeFoundationSlug$ to prevent API calls with empty string before a foundation is selected - Remove filter pill categories (memberships, events, education, projectOperations) that have no backing cards in the ED dashboard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 - Centralize TLF-only persona check into PersonaService.isTlfOnlyPersona computed signal, replacing inline checks across 5 files - Create dedicated MarketingOverviewComponent instead of repurposing FoundationHealthComponent with hideBuiltInCards/additionalCards inputs - Revert all FoundationHealthComponent changes back to main (keep focused on foundation analytics only) - Add empty slug filter to selectedFoundationSlug$ to prevent API calls before a foundation is selected - Make refresh$ private, remove empty scss, fix section comment naming, remove unnecessary computed wrapper on selectedFoundation - Add reserved comment to MetricCategory for future ED dashboard categories Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 - Remove non-functional filter pills from MarketingOverviewComponent since all cards are marketing category - Use isTlfOnlyPersona() in PersonaService.setPersona() instead of inline check for consistency - Restore original ordering in persona-selector: set foundation before setPersona() so navigation has correct context Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
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>
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Add 4 board-level KPI cards (Engaged Community, Member Acquisition & CAC, Member Retention & NRR, Flywheel Conversion Rate) with drill-down drawers above the Marketing Overview section. Uses mock data — real Snowflake queries will be wired in a follow-up. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
Replace CTE-based Email CTR queries with Platinum table queries (email_ctr_summary, email_ctr_by_month). Add real Social Media endpoint using social_media_overview, social_media_platform_breakdown, and social_media_follower_trend tables. Remove inline mock data. LFXV2-1220 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
mrautela365
force-pushed
the
feat/LFXV2-1220-ed-dashboard
branch
from
738b595 to
94fcf95
Compare
mrautela365
changed the title
4 tasks
Contributor
Author
|
Superseded by a 5-PR split for easier review:
Merge order: #345 → #346 → #347 → #348 → #349 → #350 Each PR builds cleanly and can be reviewed independently. The combined diff matches the original PR #287 content. |
🧹 Deployment RemovedThe deployment for PR #287 has been removed. |
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
PLATINUM.EMAIL_CTR_SUMMARY/EMAIL_CTR_BY_MONTH; add real Social Media endpoint usingPLATINUM.SOCIAL_MEDIA_OVERVIEW/SOCIAL_MEDIA_PLATFORM_BREAKDOWN/SOCIAL_MEDIA_FOLLOWER_TRENDTest plan
/dashboardswhen executive-director persona is selected🤖 Generated with Claude Code