feat(dashboards): implement analytics metrics with entity-type filtering #174
Conversation
- Add Contributors Mentored metric (foundation-only) - Add Unique Contributors per Week metric (entityType support) - Update PR Weekly with entityType filtering - Update Issues Resolution with entityType filtering - Standardize query parameters to use 'slug' - Add chart padding configuration (min: 0, grace: 5%) LFXV2-783 LFXV2-784 LFXV2-785 LFXV2-786 Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
WalkthroughThis PR adds two new metrics (Contributors Mentored and Unique Contributors Weekly) to the dashboard recent progress component. It introduces foundation and project context awareness by parameterizing existing analytics methods with entityType, adds corresponding backend endpoints and service methods, and defines new data interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component as Recent Progress<br/>Component
participant Service as Analytics<br/>Service
participant Controller as Analytics<br/>Controller
participant ProjectSvc as Project<br/>Service
participant DB as Snowflake
User->>Component: Navigate dashboard (foundation/project context)
activate Component
Note over Component: Compute entityType based on<br/>selectedFoundation context
Component->>Service: getContributorsMentored(slug)
Service->>Controller: GET /contributors-mentored?slug=...
activate Controller
Controller->>ProjectSvc: getContributorsMentored(slug)
ProjectSvc->>DB: Query mentored contributors (foundation-wide)
DB-->>ProjectSvc: Weekly mentored data
ProjectSvc-->>Controller: FoundationContributorsMentoredResponse
Controller-->>Service: Response + logging
deactivate Controller
Service-->>Component: Mentored metrics
Component->>Service: getUniqueContributorsWeekly(slug, entityType)
Service->>Controller: GET /unique-contributors-weekly?slug=...&entityType=...
activate Controller
Note over Controller: Validate entityType ∈ {foundation, project}
Controller->>ProjectSvc: getUniqueContributorsWeekly(slug, entityType)
alt entityType = 'foundation'
ProjectSvc->>DB: Query foundation-wide weekly contributors
else entityType = 'project'
ProjectSvc->>DB: Query project-specific weekly contributors
end
DB-->>ProjectSvc: Weekly unique contributor rows
ProjectSvc-->>Controller: UniqueContributorsWeeklyResponse
Controller-->>Service: Response + logging
deactivate Controller
Service-->>Component: Unique contributors metrics
Component->>Component: Transform data to ProgressItemWithChart
Component->>Component: Render charts with tooltips
deactivate Component
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/shared/src/constants/progress-metrics.constants.ts (1)
224-232: Address inconsistency: embedded chartOptions lack the new scale configuration.The inline
chartOptionswithinCORE_DEVELOPER_PROGRESS_METRICSandMAINTAINER_PROGRESS_METRICSdon't include the newmin: 0, grace: '5%'configuration applied to the exported presets. This creates inconsistent chart scaling behavior across the application.If these inline options are intentionally simpler (they have
tooltip: { enabled: false }), document that distinction. Otherwise, apply the same scale configuration for consistency.Apply this pattern to update inline chartOptions (example for one instance):
chartOptions: { responsive: true, maintainAspectRatio: false, plugins: { legend: { display: false }, tooltip: { enabled: false } }, scales: { x: { display: false }, - y: { display: false }, + y: { display: false, min: 0, grace: '5%' }, }, },Also applies to: 254-262, 284-292, 319-327, 349-357, 390-398, 422-430, 516-520, 544-552, 574-582, 606-614
apps/lfx-one/src/app/modules/dashboards/components/recent-progress/recent-progress.component.ts (1)
52-73: AddentityTypeguard toinitializeContributorsMentoredDatato prevent project-context API callsThe issue is confirmed. The method lacks an
entityTypecheck before calling the analytics service, unlike related metrics (initializeProjectIssuesResolutionData,initializeProjectPullRequestsWeekly,initializeUniqueContributorsWeekly) which all extract and passentityType. The type annotationFoundationContributorsMentoredResponseconfirms foundation-only scope.Apply the suggested fix:
switchMap((projectSlug) => { + const entityType = this.entityType(); - if (!projectSlug) { + if (!projectSlug || entityType !== 'foundation') { this.loadingState.update((state) => ({ ...state, contributorsMentored: false })); return [{ data: [], totalMentored: 0, avgWeeklyNew: 0, totalWeeks: 0 }]; }This prevents unnecessary or erroneous API calls when viewed in project context.
🧹 Nitpick comments (8)
apps/lfx-one/src/server/controllers/analytics.controller.ts (3)
281-329: Slug/entityType validation for issues resolution looks correct and aligned with service API.You:
- Read
slugandentityTypefrom query.- Enforce presence via
ServiceValidationError.forField.- Constrain
entityTypeto'foundation' | 'project'.- Pass both through to
projectService.getProjectIssuesResolutionand enrich success logs withslugandentity_type.This matches the new backend signature and PR objectives for entity-type support.
You may later want a small helper to DRY the repeated slug/entityType validation across multiple endpoints.
331-377: PR weekly endpoint adopts the same robust slug/entityType pattern.The updated
getProjectPullRequestsWeeklymirrors the issues-resolution handler: required slug, strict entityType validation, and logging that includes both. The service call signature matchesProjectService.getProjectPullRequestsWeekly(slug, entityType), so this path should be safe.Same as above, consider centralizing slug/entityType parsing/validation if more endpoints adopt this pattern.
412-458: Unique-contributors-weekly controller correctly enforces scope and delegates to the service.Slug and entityType are validated in the same way as the other entity-scoped endpoints, and the success log includes both plus the main aggregates. The call into
projectService.getUniqueContributorsWeekly(slug, entityType)keeps controller and service in sync.If you add more entityType-aware endpoints, a shared validator or small helper could reduce duplication and keep accepted entity types centralized.
apps/lfx-one/src/app/shared/services/analytics.service.ts (1)
200-225: Frontend issues-resolution call now matches the backend contract.Changing
getProjectIssuesResolutionto(slug, entityType)and passing{ slug, entityType }as query params lines up with the new controller signature. The fallback object incatchErrorstill conforms toProjectIssuesResolutionResponse.You might eventually centralize the
'foundation' | 'project'union type (shared alias) so both frontend and backend stay in sync.apps/lfx-one/src/server/services/project.service.ts (1)
585-666: Entity-scoped issues-resolution queries look correct and safely parameterized.
- Daily query switches between FOUNDATION_ISSUES_RESOLUTION_DAILY and PROJECT_ISSUES_RESOLUTION_DAILY based on
entityType.- Aggregated query similarly switches between FOUNDATION_ISSUES_RESOLUTION and PROJECT_ISSUES_RESOLUTION.
- Slug is always passed as a bound parameter (
?), so no SQL injection concerns.- Aggregation combines
OPENED_ISSUES,CLOSED_ISSUES,RESOLUTION_RATE_PCT, andMEDIAN_DAYS_TO_CLOSEinto the sharedProjectIssuesResolutionResponse.This aligns with the controller and shared interface updates.
You’re selecting some extra columns (e.g.,
TOTAL_ISSUES,AVG_DAYS_TO_CLOSE) that aren’t used; trimming them later could marginally reduce data transfer and clarify intent.apps/lfx-one/src/app/modules/dashboards/components/recent-progress/recent-progress.component.ts (3)
423-452: Consider surfacing avg weekly new in Contributors Mentored UI and aligning trend behavior
transformContributorsMentoredcorrectly builds a cumulative purple line chart withvalueset tototalMentored, matching the main value requirement. However:
- The spec also calls out “average new contributors/week”; right now
avgWeeklyNewis only used to drivetrend, and that information is not surfaced directly in the UI (subtitle, tooltip, or footer).- Trend is set to
'up'only whenavgWeeklyNew > 0andundefinedotherwise. Other metrics either show'up'or'down', so this one may appear inconsistent with the rest of the dashboard.Suggestions:
- Add a tooltip or secondary text that exposes
avgWeeklyNew, e.g.Avg new per week: X.- Optionally normalize trend behavior to
'up'/'down'or confirm that suppressing “down” here is intentional.Example tooltip enhancement:
- return { - label: 'Contributors Mentored', + const avgNew = Math.round(data.avgWeeklyNew || 0); + const tooltipText = `Avg new contributors/week: ${avgNew.toLocaleString()}`; + + return { + label: 'Contributors Mentored', icon: 'fa-light fa-user-graduate', value: data.totalMentored.toString(), trend: data.avgWeeklyNew > 0 ? 'up' : undefined, subtitle: 'Total contributors mentored', + tooltipText,
455-546: Unique Contributors Weekly transform and tooltip are solid; add minor robustness to tooltip footerThe Unique Contributors per Week metric wiring looks good:
- Uses
avgUniqueContributorsas the main value with a blue bar chart.- Tooltip text from
uniqueContributorsTooltipDataexposes total, avg new, and avg returning per week, which aligns with the requirements.- Per‑bar tooltip callbacks (title/label/footer) correctly show formatted week, unique contributors, and per‑week new/returning/total active breakdown.
One small robustness nit: in the
footercallback (Lines 527-535) you accesscontext[0].dataIndexwithout a guard. In the unlikely case Chart.js passes an empty context array, this would throw, whereas your other tooltip callbacks (and the PR velocity tooltip) use optional chaining and early returns.You can align this with the safer pattern:
- footer: (context: TooltipItem<'bar'>[]) => { - try { - const dataIndex = context[0].dataIndex; + footer: (context: TooltipItem<'bar'>[]) => { + try { + const dataIndex = context[0]?.dataIndex; const weekData = chartData[dataIndex]; - return [ + if (dataIndex === undefined || !weekData) return []; + return [ `New: ${weekData.NEW_CONTRIBUTORS}`, `Returning: ${weekData.RETURNING_CONTRIBUTORS}`, `Total active: ${weekData.TOTAL_ACTIVE_CONTRIBUTORS}`, ]; } catch (e) { console.error('Error in footer callback:', e); return []; } },This keeps tooltip behavior consistent with the rest of the component and avoids potential edge‑case runtime errors.
Also applies to: 777-794
638-687: Initializers for new metrics follow existing patterns; consider future persona/entity gatingBoth
initializeContributorsMentoredDataandinitializeUniqueContributorsWeeklyDatamirror the existing initializer structure:
- Respect
projectSlugbeing unset by emitting an empty response shape and clearing the corresponding loading flag.- Set and clear specific
loadingStateflags around the analytics service calls.- Pass
entityTypecorrectly for unique contributors weekly.Beyond the earlier note about gating foundation‑only Contributors Mentored calls by
entityType, this setup looks consistent. If in the future you decide that some metrics should only load for maintainer persona, these initializers would be a natural place to add persona checks to avoid unnecessary API traffic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/lfx-one/src/app/modules/dashboards/components/recent-progress/recent-progress.component.ts(9 hunks)apps/lfx-one/src/app/shared/services/analytics.service.ts(4 hunks)apps/lfx-one/src/server/controllers/analytics.controller.ts(3 hunks)apps/lfx-one/src/server/routes/analytics.route.ts(1 hunks)apps/lfx-one/src/server/services/project.service.ts(5 hunks)packages/shared/src/constants/progress-metrics.constants.ts(4 hunks)packages/shared/src/interfaces/analytics-data.interface.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/lfx-one/src/server/controllers/analytics.controller.ts (2)
apps/lfx-one/src/server/helpers/logger.ts (2)
Logger(10-129)error(52-65)apps/lfx-one/src/server/errors/index.ts (1)
ServiceValidationError(7-7)
apps/lfx-one/src/app/shared/services/analytics.service.ts (1)
packages/shared/src/interfaces/analytics-data.interface.ts (4)
ProjectIssuesResolutionResponse(484-514)ProjectPullRequestsWeeklyResponse(550-570)FoundationContributorsMentoredResponse(612-632)UniqueContributorsWeeklyResponse(687-707)
apps/lfx-one/src/server/services/project.service.ts (1)
packages/shared/src/interfaces/analytics-data.interface.ts (7)
ProjectIssuesResolutionResponse(484-514)ProjectPullRequestsWeeklyResponse(550-570)ProjectPullRequestsWeeklyRow(520-545)FoundationContributorsMentoredResponse(612-632)FoundationContributorsMentoredRow(576-606)UniqueContributorsWeeklyResponse(687-707)UniqueContributorsWeeklyRow(638-681)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (14)
packages/shared/src/constants/progress-metrics.constants.ts (2)
48-48: LGTM: Improved chart scaling with baseline and padding.Setting
min: 0establishes a proper baseline for metric visualizations, andgrace: '5%'adds useful padding at the top for better visual spacing. Chart.js 4.5.0 supports thegraceoption, so all changes are compatible.Also applies to: 135-135, 196-196
69-69: No action required; the original concern is invalid.Chart.js documentation confirms that explicit
max/minvalues override data bounds and preventgracefrom being applied. Sincemax: 1is explicitly set, the y-axis will remain hard-capped at 1.0, andgrace: '5%'has no effect. The configuration is correct as written.Likely an incorrect or invalid review comment.
apps/lfx-one/src/server/routes/analytics.route.ts (1)
32-36: New analytics routes are consistent with existing routing pattern.Paths, HTTP verb, and controller wiring match the other analytics endpoints; nothing blocking here.
apps/lfx-one/src/server/controllers/analytics.controller.ts (1)
379-410: Contributors-mentored controller is straightforward and matches the foundation-only contract.You:
- Require
slugand surface a clear validation error when missing.- Delegate to
projectService.getContributorsMentored(slug).- Log key aggregates (
total_mentored,avg_weekly_new,total_weeks) on success.This aligns with the “foundation-only” metric design for Contributors Mentored (LFXV2-785).
packages/shared/src/interfaces/analytics-data.interface.ts (1)
572-632: Contributors-mentored interfaces align well with the Snowflake schema and API shape.
FoundationContributorsMentoredRowmirrors the FOUNDATION_CONTRIBUTORS_MENTORED_WEEKLY columns, andFoundationContributorsMentoredResponseexposes exactly the aggregates the service computes (totalMentored,avgWeeklyNew,totalWeeks). This should plug cleanly into both server and client code.apps/lfx-one/src/app/shared/services/analytics.service.ts (3)
227-250: PR weekly service method correctly forwards slug and entityType.
getProjectPullRequestsWeekly(slug, entityType)hits the updated/project-pull-requests-weeklyendpoint with the expected params and returns a well-typed fallback on error. This should keep the recent-progress UI working once callers are updated to supply entityType.
252-274: Contributors-mentored service wrapper is consistent with other endpoints.The method:
- Sends
slugas the only query param to/api/analytics/contributors-mentored.- Returns a typed empty-state response on error.
This matches the foundation-only backend contract and the new shared interface.
276-299: Unique-contributors-weekly service wrapper cleanly exposes the new metric.You correctly accept
(slug, entityType), pass them as params, and return aUniqueContributorsWeeklyResponsewith a sensible empty fallback. The API surface is consistent with the other analytics methods.apps/lfx-one/src/server/services/project.service.ts (3)
7-24: New analytics interfaces are imported appropriately.The additions of
FoundationContributorsMentoredRow/ResponseandUniqueContributorsWeeklyRow/Responseto the imports match their later usage as Snowflake result and response types. No concerns here.
668-716: PR weekly analytics now support foundation/project scopes cleanly.The unified
getProjectPullRequestsWeekly(slug, entityType):
- Switches between FOUNDATION_PULL_REQUESTS_WEEKLY and PROJECT_PULL_REQUESTS_WEEKLY, with slug correctly applied as FOUNDATION_SLUG or PROJECT_SLUG.
- Uses parameter binding with
[slug].- Computes
totalMergedPRsand a merge-time weighted average, rounding to one decimal, consistent with existing behavior.This matches the new entityType-aware controller and frontend and satisfies the PR Velocity requirements.
718-755: Contributors-mentored Snowflake query and aggregates match the defined contract.
- Query targets
FOUNDATION_CONTRIBUTORS_MENTORED_WEEKLYwithFOUNDATION_SLUG = ?, ordered byWEEK_START_DATE DESCand limited to 52 weeks.- Uses
MENTORED_CONTRIBUTOR_COUNTfrom the latest row astotalMentored.- Derives
avgWeeklyNewfromWEEKLY_MENTORED_CONTRIBUTOR_COUNT, rounded to one decimal.This lines up with
FoundationContributorsMentoredRow/Responseand the “cumulative purple line chart + average new contributors/week” requirement.apps/lfx-one/src/app/modules/dashboards/components/recent-progress/recent-progress.component.ts (3)
24-33: New analytics response imports look consistentThe additions of
FoundationContributorsMentoredResponseandUniqueContributorsWeeklyResponsehere align with the new metrics and are typed cleanly. No issues from this file’s perspective.
584-596: EntityType wiring into issues and PR velocity analytics looks correctUsing
this.entityType()to choose between foundation/project series forgetProjectIssuesResolutionandgetProjectPullRequestsWeekly(Lines 593-596, 621-624) matches the PR requirement to support both entity types through a single endpoint shape.One thing to double‑check:
toObservable(this.projectSlug)is the only reactive trigger here. If there is any flow where the entity type can change while the slug string remains the same, these streams wouldn’t refetch. If that can’t happen withProjectContextService(slug always changes along with entity type), this is fine; otherwise consider includingentityTypein the reactive source.Also applies to: 612-624
696-736: Progress items integration correctly wires new metrics by labelThe
initializeProgressItemscomputed now:
- Reads the new data signals (
contributorsMentoredData,uniqueContributorsWeeklyData,uniqueContributorsTooltipData).- Maps base metrics (maintainer vs core developer) and swaps in transformed items based on the metric
label(Lines 712-733).This matches the existing pattern for other metrics and should plug the new cards into the UI cleanly, assuming
MAINTAINER_PROGRESS_METRICSincludes the two new labels and CORE developer metrics do not. No issues from this component’s perspective.
Summary
Implements analytics metrics for the maintainer dashboard with entity-type filtering support for foundation and project-level data visualization.
New Metrics
Contributors Mentored (LFXV2-785)
GET /api/analytics/contributors-mentored?slug={slug}FOUNDATION_CONTRIBUTORS_MENTORED_WEEKLYtableUnique Contributors per Week (LFXV2-786)
GET /api/analytics/unique-contributors-weekly?slug={slug}&entityType={type}FOUNDATION_UNIQUE_CONTRIBUTORS_WEEKLYandPROJECT_UNIQUE_CONTRIBUTORS_WEEKLYtablesQuery Improvements
PR Review & Merge Velocity (LFXV2-783)
GET /api/analytics/project-pull-requests-weekly?slug={slug}&entityType={type}Open vs Closed Issues Trend (LFXV2-784)
GET /api/analytics/project-issues-resolution?slug={slug}&entityType={type}Technical Changes
Backend
Frontend
Shared Package
FoundationContributorsMentoredRow/Response,UniqueContributorsWeeklyRow/ResponseFiles Changed (7 total)
apps/lfx-one/src/app/modules/dashboards/components/recent-progress/recent-progress.component.tsapps/lfx-one/src/app/shared/services/analytics.service.tsapps/lfx-one/src/server/controllers/analytics.controller.tsapps/lfx-one/src/server/routes/analytics.route.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/constants/progress-metrics.constants.tspackages/shared/src/interfaces/analytics-data.interface.tsRelated Issues
LFXV2-783, LFXV2-784, LFXV2-785, LFXV2-786
Generated with Claude Code