perf(committees): eliminate N+1 upstream count calls in groups list#1060
perf(committees): eliminate N+1 upstream count calls in groups list#1060MRashad26 wants to merge 3 commits into
Conversation
…FXV2-2120) GET /api/committees and my-committees previously fired 2 upstream API calls per committee (getCommitteeMembersCount + getMailingListCountByCommittee). For a foundation like AAIF with many groups this created a burst of 2N simultaneous count requests on every page load, causing the slowness reported in ED view. Fix: - Trust the committee-service-indexed `total_members` field that is already present in each committee resource returned by the query service (CommitteeBaseWithReadonlyAttributes in the upstream Goa design) — no separate count call needed. - Replace N individual mailing-list count calls with a single batched query using the query service `tags` array parameter (OR semantics), chunked at 100 UIDs per request. Cost drops from O(N) to O(1) upstream requests for both getCommittees and getMyCommittees. The old getMailingListCountByCommittee private helper is retained for potential future use but is no longer called from the list paths. Signed-off-by: Mohammed Rashad <mrashad@contractor.linuxfoundation.org> Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCommittee data loading now uses batched mailing-list lookup and indexed member totals, the dashboard fetches committee UIDs through a lightweight endpoint for non-Me scope, and project metadata lookup batches are larger. ChangesCommittee enrichment and UID loading
Project metadata batching
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-1060.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/lfx-one/src/server/services/committee.service.ts (4)
1674-1680: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline object type used as generic argument instead of a named interface.
{ committee_uid?: string }is repeated as an inline type literal for bothfetchAllQueryResourcesandQueryServiceResponse. Extracting a small named interface (e.g.interface MailingListResource { committee_uid?: string }) would satisfy the project's TS convention and avoid duplicating the shape.As per coding guidelines, "Use interfaces for object shape definitions in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/committee.service.ts` around lines 1674 - 1680, The mailing list query uses the same inline object shape twice in `committee.service.ts`, which violates the TypeScript convention to prefer interfaces for object shapes. Extract `{ committee_uid?: string }` into a named interface (for example, a small resource type used by `fetchAllQueryResources` and `QueryServiceResponse`) and update the `mailingLists` fetch in `committee.service.ts` to reference that interface instead of repeating the literal.Source: Coding guidelines
1653-1663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale leftover JSDoc block before the new method's own docstring.
Lines 1653-1656 ("Fetches count of mailing lists associated with a specific committee...") describe
getMailingListCountByCommittee's old per-committee approach, but they now sit directly abovegetCommitteesWithMailingList's own JSDoc (1657-1663), which documents the new batched approach. This orphaned comment block should be removed to avoid confusing future readers about which method it describes.🧹 Proposed fix
- /** - * Fetches count of mailing lists associated with a specific committee. - * Used to determine the has_mailing_list flag for committee list views. - */ /** * Returns the subset of the provided committee UIDs that have at least one associated * `groupsio_mailing_list` resource. Uses a single batched query with OR-tag semantics🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/committee.service.ts` around lines 1653 - 1663, Remove the stale JSDoc block in committee.service.ts that still describes the old getMailingListCountByCommittee behavior and leave only the docstring for getCommitteesWithMailingList. This orphaned comment sits directly before the new method’s own documentation, so delete the outdated “Fetches count of mailing lists...” lines to keep the comment accurately attached to getCommitteesWithMailingList and avoid confusion.
964-993: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the two independent batched lookups concurrently.
getCommitteesByIds(committeeUids)(line 969) andgetCommitteesWithMailingList(committeeUids)(line 973) both depend only oncommitteeUids, not on each other's result, yet they're awaited sequentially. Running them in parallel shaves one round-trip off the exact path this PR is optimizing.⚡ Proposed fix
- const committeeUids = Array.from(membershipMap.keys()); - const committees = await this.getCommitteesByIds(req, committeeUids); - - // Batch check mailing-list presence for all committee UIDs in one query instead of N - // per-committee count calls. total_members is already indexed by the committee-service. - const committeesWithMailingList = committeeUids.length > 0 ? await this.getCommitteesWithMailingList(req, committeeUids) : new Set<string>(); + const committeeUids = Array.from(membershipMap.keys()); + + // Fetch committee resources and mailing-list presence in parallel — both depend only + // on committeeUids, not on each other's result. + const [committees, committeesWithMailingList] = await Promise.all([ + this.getCommitteesByIds(req, committeeUids), + committeeUids.length > 0 ? this.getCommitteesWithMailingList(req, committeeUids) : Promise.resolve(new Set<string>()), + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/committee.service.ts` around lines 964 - 993, The two batched lookups in committee.service’s get_my_committees flow are independent but currently awaited one after the other. Update the logic around getCommitteesByIds and getCommitteesWithMailingList so both requests are started together and then awaited together, keeping the existing empty-list guard for mailing lists. Preserve the rest of the enrichment path that uses committeeUids, committees, and committeesWithMailingList unchanged.
1697-1711: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
getMailingListCountByCommitteehelper. It has no remaining call sites, so deleting it will reduce dead code and keep the service easier to maintain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/committee.service.ts` around lines 1697 - 1711, Remove the dead helper getMailingListCountByCommittee from committee.service.ts since it has no remaining callers. Delete the private method entirely, and make sure nothing else in CommitteeService references it or depends on its logger/microserviceProxy logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/src/server/services/committee.service.ts`:
- Around line 1664-1696: The batching in getCommitteesWithMailingList currently
wraps the entire loop in one try/catch, so one failing batch stops all later
batches and hides partial results; refactor it so each batch handles its own
failure and other batches still run, ideally using the same
Promise.all/per-batch error-handling approach as getCommitteesByIds. Also update
the recoverable failure log in getCommitteesWithMailingList from logger.debug to
logger.warning so the fallback is visible while still returning the found Set.
---
Nitpick comments:
In `@apps/lfx-one/src/server/services/committee.service.ts`:
- Around line 1674-1680: The mailing list query uses the same inline object
shape twice in `committee.service.ts`, which violates the TypeScript convention
to prefer interfaces for object shapes. Extract `{ committee_uid?: string }`
into a named interface (for example, a small resource type used by
`fetchAllQueryResources` and `QueryServiceResponse`) and update the
`mailingLists` fetch in `committee.service.ts` to reference that interface
instead of repeating the literal.
- Around line 1653-1663: Remove the stale JSDoc block in committee.service.ts
that still describes the old getMailingListCountByCommittee behavior and leave
only the docstring for getCommitteesWithMailingList. This orphaned comment sits
directly before the new method’s own documentation, so delete the outdated
“Fetches count of mailing lists...” lines to keep the comment accurately
attached to getCommitteesWithMailingList and avoid confusion.
- Around line 964-993: The two batched lookups in committee.service’s
get_my_committees flow are independent but currently awaited one after the
other. Update the logic around getCommitteesByIds and
getCommitteesWithMailingList so both requests are started together and then
awaited together, keeping the existing empty-list guard for mailing lists.
Preserve the rest of the enrichment path that uses committeeUids, committees,
and committeesWithMailingList unchanged.
- Around line 1697-1711: Remove the dead helper getMailingListCountByCommittee
from committee.service.ts since it has no remaining callers. Delete the private
method entirely, and make sure nothing else in CommitteeService references it or
depends on its logger/microserviceProxy logic.
🪄 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: f141c51a-6767-483d-a8bf-c1c087c37280
📒 Files selected for processing (1)
apps/lfx-one/src/server/services/committee.service.ts
There was a problem hiding this comment.
Pull request overview
This PR is a backend performance fix for the committees ("Groups") list endpoints in the Express BFF. Previously, GET /api/committees and GET /api/committees/my-committees fired 2 upstream count calls per committee (member count + mailing-list count) after the initial list query, producing a burst of 2N upstream requests per page load and slow load times (notably in ED view for large foundations). The change replaces those per-committee calls with the already-indexed total_members field and a single batched mailing-list presence query.
Changes:
- Drop per-committee
getCommitteeMembersCountcalls ingetCommitteesandgetMyCommittees; use the indexedcommittee.total_members ?? 0instead. - Add
getCommitteesWithMailingList, a batchedgroupsio_mailing_listlookup (chunked at 100 UIDs, OR-tag semantics) that replaces the N individual mailing-list count calls. - Enrich each committee/
MyCommitteewithhas_mailing_listderived from the batched result set.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/server/services/committee.service.ts:1711
getMailingListCountByCommitteeis now dead code — this change removed its only caller, and a repository search finds no other references. The publicgetCommitteeMembersCount(around line 452) is similarly unused after this change. Consider removing both helpers (and the stray JSDoc above) to avoid leaving unused query paths behind.
private async getMailingListCountByCommittee(req: Request, committeeId: string): Promise<number> {
try {
const { count } = await this.microserviceProxy.proxyRequest<QueryServiceCountResponse>(req, 'LFX_V2_SERVICE', '/query/resources/count', 'GET', {
type: 'groupsio_mailing_list',
tags: `committee_uid:${committeeId}`,
});
return count;
} catch (error) {
logger.debug(req, 'get_mailing_list_count_by_committee', 'Failed to fetch mailing list count, defaulting to 0', {
committee_uid: committeeId,
error: error instanceof Error ? error.message : 'Unknown error',
});
return 0;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…groups (LFXV2-2120) Three follow-up improvements to the groups page in ED/project view: - committee-dashboard: non-me-lens skips full getMyCommittees enrichment; member badge UIDs are now fetched via a new lightweight /my-committee-uids endpoint that only reads committee_member records (no committee detail or project fan-out) - committee.controller/route: add GET /committees/my-committee-uids backed by the existing committeeService.getMyCommitteeUids — returns string[] of UIDs - project.service: raise enrichWithProjectData batchSize from 10 to 50, collapsing sequential upstream rounds from ceil(N/10) to ceil(N/50) for project enrichment Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts (1)
319-345: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider scoping
getMyCommitteeUids()by project when in 'project' mode.For non-Me lens,
getMyCommitteeUids()is called without aprojectUid, so it always fetches the full set of the user's committee memberships across every project rather than just the ones relevant to the active project. This is functionally correct (the Set is only used via.has(uid)), but for orgs where a user belongs to many committees org-wide, it fetches more data than needed. The siblinginitializeMyCommittees()already derives a project/foundationscope$for this same branch — reusingscope.uidwhenscope.mode === 'project'(foundation mode can't be scoped this way since the endpoint filters by a singleproject_uidtag and would incorrectly exclude sub-project memberships) would tighten the query.♻️ Proposed refactor to scope by project when applicable
private initializeMyCommitteeUids(): Signal<Set<string>> { const fromFull$ = toObservable(computed(() => new Set(this.myCommittees().map((c) => c.uid)))); const isMeLens$ = toObservable(this.isMeLens); const refresh$ = toObservable(this.refresh); + const project$ = toObservable(this.project); + const isFoundationContext$ = toObservable(this.isFoundationContext); return toSignal( - combineLatest([isMeLens$, refresh$]).pipe( - switchMap(([isMeLens]) => { + combineLatest([isMeLens$, refresh$, project$, isFoundationContext$]).pipe( + switchMap(([isMeLens, , project, isFoundation]) => { if (isMeLens) { return fromFull$; } - return this.committeeService.getMyCommitteeUids().pipe( + const projectUid = !isFoundation ? (project?.uid ?? undefined) : undefined; + return this.committeeService.getMyCommitteeUids(projectUid).pipe( map((uids) => new Set(uids)), catchError(() => of(new Set<string>())) ); }) ), { initialValue: new Set<string>() } ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts` around lines 319 - 345, The non-Me branch in initializeMyCommitteeUids currently calls committeeService.getMyCommitteeUids() without any scope, so it fetches all committee memberships org-wide. Reuse the same scope logic already used in initializeMyCommittees, and when scope.mode is project pass scope.uid into getMyCommitteeUids() so the membership Set is limited to the active project; keep foundation mode unscoped because the endpoint would filter out sub-project memberships. Preserve the existing catchError and Set<string> handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts`:
- Around line 319-345: The non-Me branch in initializeMyCommitteeUids currently
calls committeeService.getMyCommitteeUids() without any scope, so it fetches all
committee memberships org-wide. Reuse the same scope logic already used in
initializeMyCommittees, and when scope.mode is project pass scope.uid into
getMyCommitteeUids() so the membership Set is limited to the active project;
keep foundation mode unscoped because the endpoint would filter out sub-project
memberships. Preserve the existing catchError and Set<string> handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2136db2-3126-4b28-81a1-de885a293828
📒 Files selected for processing (5)
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-one/src/app/shared/services/committee.service.tsapps/lfx-one/src/server/controllers/committee.controller.tsapps/lfx-one/src/server/routes/committees.route.tsapps/lfx-one/src/server/services/project.service.ts
Address review comments from @copilot-pull-request-reviewer, @coderabbitai: - committee.service.ts: fix getCommitteesWithMailingList — ml.committee_uid is always undefined; committee UIDs live in ml.committees[].uid per the GroupsIOMailingList interface (per @copilot-pull-request-reviewer) - committee.service.ts: convert sequential batch loop to Promise.allSettled so one failing chunk does not suppress mailing-list flags for unrelated batches; move recoverable-failure log from logger.debug to logger.warning (per @coderabbitai, @copilot-pull-request-reviewer) - committee.service.ts: remove stray duplicate JSDoc block left above getCommitteesWithMailingList from the prior method (per @copilot-pull-request-reviewer) Resolves 3 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 17ce56a Changes Made
Threads Resolved3 of 3 unresolved threads addressed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd committee.service.ts apps/lfx-one/src/server/services --exec cat -n {} \; | sed -n '1,400p' | grep -n "getCommitteesWithMailingList\|Promise.allSettled\|logger.warning\|committee_uid\|ml.committees"Length of output: 927 🏁 Script executed: #!/bin/bash
FILE=$(fd committee.service.ts apps/lfx-one/src/server/services)
wc -l "$FILE"
grep -n "getCommitteesWithMailingList\|Promise.allSettled\|logger.warning\|logger.debug\|committee_uid\|ml.committees\|GroupsIOMailingList" "$FILE"
echo "---"
sed -n '/getCommitteesWithMailingList/,/^ }/p' "$FILE" | head -100Length of output: 944 FILE="apps/lfx-one/src/server/services/committee.service.ts" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/server/services/committee.service.ts:1708
getMailingListCountByCommitteeis now unused (the list views switched togetCommitteesWithMailingList, and this method isprivate, so nothing outside the class can call it). Removing it will avoid dead code and reduce future confusion about which approach is canonical.
private async getMailingListCountByCommittee(req: Request, committeeId: string): Promise<number> {
try {
const { count } = await this.microserviceProxy.proxyRequest<QueryServiceCountResponse>(req, 'LFX_V2_SERVICE', '/query/resources/count', 'GET', {
type: 'groupsio_mailing_list',
tags: `committee_uid:${committeeId}`,
| // Enrich with mailing-list presence via a single batched query (OR-tag semantics). | ||
| // total_members is already indexed by the committee-service as part of | ||
| // CommitteeBaseWithReadonlyAttributes — no separate per-committee count call needed. |
| * Returns the subset of the provided committee UIDs that have at least one associated | ||
| * `groupsio_mailing_list` resource. Uses a single batched query with OR-tag semantics | ||
| * (the query service `tags` parameter is ArrayOf(String) with OR logic) instead of one | ||
| * count call per committee — cost is O(1) upstream requests regardless of list size, | ||
| * chunked at 100 UIDs per request for URL-length safety. |
| return this.committeeService.getMyCommitteeUids().pipe( | ||
| map((uids) => new Set(uids)), | ||
| catchError(() => of(new Set<string>())) | ||
| ); |
Summary
Root cause:
GET /api/committeesandGET /api/committees/my-committeesfired 2 upstream API calls per committee (member count + mailing-list count) after the initial list query. For a large foundation like AAIF with many groups, this created a burst of 2N simultaneous upstream requests on every page load — the primary cause of slow load times in ED view.Fix 1 —
total_members: The committee-service already indexestotal_membersas part ofCommitteeBaseWithReadonlyAttributes(confirmed in upstream Goa design). Dropped the per-committeegetCommitteeMembersCountcount call and used the field already present in the indexed resource.Fix 2 —
has_mailing_list: Replaced N individualgroupsio_mailing_listcount calls with a single batched query using the query servicetags: ArrayOf(String)OR-semantic parameter. Cost drops from O(N) to O(1) upstream requests per page load, chunked at 100 UIDs per request.Before:
getCommittees(N committees)→ 1 list query + 2N count calls + 1 access checkAfter:
getCommittees(N committees)→ 1 list query + 1 mailing-list batch query + 1 access checkSame improvement applies to
getMyCommittees.Test plan
total_memberscount displays correctly on committee cardsFixes LFXV2-2120
🤖 Generated with Claude Code