Skip to content

Commit dfc0002

Browse files
MRashad26MRashad26
authored
perf(meetings): eliminate N+1 committee fetches and count endpoints for project meetings (#1076)
* perf(meetings): eliminate N+1 committee fetches and add project meetings count endpoints LFXV2-2120 Win 2 — server-side batch committee name lookup: - committee.service.ts: make getCommitteesByIds public so meeting.service can use it - meeting.service.ts: rewrite getCommitteeNameMap to use a single batched query-service call (OR-tag pattern) instead of N sequential getCommitteeById calls; only committee.name is needed so the heavyweight getCommitteeById (settings + membership + access checks) is no longer invoked per meeting Win 1 — project meetings stat cards via count endpoints: - past-meeting.controller.ts: add getPastMeetingsCount (GET /past-meetings/count) backed by getMeetingsCount with type=v1_past_meeting - past-meetings.route.ts: register /count route before /:uid to avoid param capture - meeting.service.ts (frontend): add getPastMeetingsCountByProject calling the new endpoint - meetings-dashboard.component.ts: replace rawFpUpcomingMeetings().length and rawFpPastMeetings().length with dedicated count signals (initFpUpcomingCount / initFpPastCount) that call the count API directly; per-card loading signals (fpUpcomingCountLoading, fpPastCountLoading) so count cards resolve on one fast API call rather than waiting for all paginated pages - meetings-dashboard.component.html: update count-card guards to use per-card loading; recurring/recordings cards keep fpStatsLoading (tied to the raw full-page fetch they need) Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> * fix(review): move getCommitteesByIds before private methods per member-ordering lint rule LFXV2-2120 @typescript-eslint/member-ordering requires public methods to be declared before private methods. getCommitteesByIds was positioned after private methods; move it to immediately before the private section. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> * fix(review): address PR #1076 CodeRabbit review feedback LFXV2-2120 Address review comments from @coderabbitai: - meetings-dashboard.component.ts: extract shared initFpCount() helper to eliminate duplicate initFpUpcomingCount/initFpPastCount implementations; add loadingSignal.set(false) in early-return branch to prevent stuck skeleton (per Thread 2); add Observable to rxjs imports - apps/shared/services/meeting.service.ts: extract private getCountByTag() helper; consolidate getMeetingsCountByCommittee, getMeetingsCountByProject, getPastMeetingsCountByProject to delegate to it; move getCountByTag after all public methods to satisfy @typescript-eslint/member-ordering (per Thread 4) - server/services/meeting.service.ts: rewrite getCommitteeNameMap() with Promise.allSettled directly (bypassing getCommitteesByIds) so a single batch failure degrades gracefully instead of wiping all committee names (per Thread 3) Resolves 3 CodeRabbit review threads (Thread 2, 3, 4). Thread 1 (count alignment) is a false positive — responded with explanation. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> * fix(review): address PR #1076 iteration-2 review feedback LFXV2-2120 Address review comments from @audigregorie, @copilot-pull-request-reviewer, @ahmedomosanya: - meetings-dashboard.component.ts: add catchError(() => of(0)) before finalize in initFpCount — defense-in-depth matching the pattern in initializeRawFpUpcomingMeetings (per @audigregorie) - committee.service.ts: revert getCommitteesByIds from public back to private — getCommitteeNameMap in meeting.service.ts intentionally reimplements the batch fetch with Promise.allSettled (partial failure tolerance) rather than calling getCommitteesByIds which uses fail-fast Promise.all; the public modifier was misleading since no external caller materialised (per @copilot-pull-request-reviewer, @audigregorie) - meeting.service.ts: add comment documenting expected cardinality bound (≤1 batch in practice for a project's displayed meetings) (per @ahmedomosanya) Resolves 4 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> * fix(review): address PR #1076 review feedback Address review comment from @copilot-pull-request-reviewer: - server/services/meeting.service.ts:1445: remove redundant new Set() wrap on uniqueCommitteeUids — the array is already deduplicated via [...new Set(...)] at line 1429; only .filter(Boolean) adds value Resolves 1 review thread. LFXV2-2120 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> * fix(review): address PR #1076 review feedback Address review comment from @copilot-pull-request-reviewer: - server/services/meeting.service.ts: remove stale CommitteeService dependency — getCommitteeNameMap now uses microserviceProxy directly so committeeService import, field, and constructor instantiation are all dead code; removed all three Resolves 1 review thread. LFXV2-2120 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org> --------- Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
1 parent 7002010 commit dfc0002

7 files changed

Lines changed: 170 additions & 93 deletions

File tree

‎apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ <h1 class="font-display font-light text-2xl">{{ activeLens() === 'me' ? 'My Meet
112112
<i class="fa-light fa-calendar text-xl"></i>
113113
</div>
114114
<div class="flex flex-col gap-0.5">
115-
@if (fpStatsLoading()) {
115+
@if (fpUpcomingCountLoading()) {
116116
<p class="text-xl font-medium text-gray-400">&mdash;</p>
117117
} @else {
118118
<p class="text-xl font-medium text-gray-900">{{ fpUpcomingCount() }}</p>
@@ -139,7 +139,7 @@ <h1 class="font-display font-light text-2xl">{{ activeLens() === 'me' ? 'My Meet
139139
<i class="fa-light fa-clock-rotate-left text-xl"></i>
140140
</div>
141141
<div class="flex flex-col gap-0.5">
142-
@if (fpStatsLoading()) {
142+
@if (fpPastCountLoading()) {
143143
<p class="text-xl font-medium text-gray-400">&mdash;</p>
144144
} @else {
145145
<p class="text-xl font-medium text-gray-900">{{ fpPastCount() }}</p>

‎apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,8 @@ export class MeetingsDashboardComponent {
132132
// Per-lens loading flags so Me/FP recording-count pipelines cannot clobber each other on lens switch.
133133
protected readonly meRecordingsCountLoading = signal(false);
134134
protected readonly fpRecordingsCountLoading = signal(false);
135+
protected fpUpcomingCountLoading = signal(false);
136+
protected fpPastCountLoading = signal(false);
135137

136138
// Raw user meetings cached for client-side filtering (Me lens only)
137139
private rawUserMeetings: Signal<Meeting[]>;
@@ -228,11 +230,12 @@ export class MeetingsDashboardComponent {
228230
this.rawFpUpcomingMeetings = this.initializeRawFpUpcomingMeetings();
229231
this.rawFpPastMeetings = this.initializeRawFpPastMeetings();
230232

231-
// Foundation/Project lens stat cards (computed from raw FP signals, not paginated)
232-
// Only look at the active tab's loading signal — inactive tab fetches are gated off.
233+
// Foundation/Project lens stat cards
234+
// fpStatsLoading gates recurring/recordings cards (backed by raw full-page fetches).
235+
// fpUpcomingCount/fpPastCount use the count API directly — they resolve independently.
233236
this.fpStatsLoading = computed(() => (this.timeFilter() === 'past' ? this.fpPastLoading() : this.fpUpcomingLoading()));
234-
this.fpUpcomingCount = computed(() => (this.activeLens() !== 'me' ? this.rawFpUpcomingMeetings().length : 0));
235-
this.fpPastCount = computed(() => (this.activeLens() !== 'me' ? this.rawFpPastMeetings().length : 0));
237+
this.fpUpcomingCount = this.initFpUpcomingCount();
238+
this.fpPastCount = this.initFpPastCount();
236239
this.fpRecurringCount = computed(() => (this.activeLens() !== 'me' ? this.rawFpUpcomingMeetings().filter((m) => m.recurrence !== null).length : 0));
237240
this.fpRecordingsAvailableCount = this.initFpRecordingsAvailableCount();
238241

@@ -779,6 +782,40 @@ export class MeetingsDashboardComponent {
779782
});
780783
}
781784

785+
private initFpUpcomingCount(): Signal<number> {
786+
return this.initFpCount(this.fpUpcomingCountLoading, (uid) => this.meetingService.getMeetingsCountByProject(uid));
787+
}
788+
789+
private initFpPastCount(): Signal<number> {
790+
return this.initFpCount(this.fpPastCountLoading, (uid) => this.meetingService.getPastMeetingsCountByProject(uid));
791+
}
792+
793+
private initFpCount(loadingSignal: WritableSignal<boolean>, fetchFn: (uid: string) => Observable<number>): Signal<number> {
794+
const project$ = toObservable(this.project);
795+
const lens$ = toObservable(this.activeLens);
796+
797+
return toSignal(
798+
combineLatest([project$, lens$, this.refresh$]).pipe(
799+
switchMap(([project, lens]) => {
800+
if (lens === 'me' || !project?.uid) {
801+
loadingSignal.set(false);
802+
return of(0);
803+
}
804+
if (!isPlatformBrowser(this.platformId)) {
805+
loadingSignal.set(true);
806+
return of(0);
807+
}
808+
loadingSignal.set(true);
809+
return fetchFn(project.uid).pipe(
810+
catchError(() => of(0)),
811+
finalize(() => loadingSignal.set(false))
812+
);
813+
})
814+
),
815+
{ initialValue: 0 }
816+
);
817+
}
818+
782819
private initFpRecordingsAvailableCount(): Signal<number> {
783820
return toSignal(
784821
combineLatest([toObservable(this.activeLens), toObservable(this.rawFpPastMeetings)]).pipe(

‎apps/lfx-one/src/app/shared/services/meeting.service.ts‎

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -113,14 +113,7 @@ export class MeetingService {
113113

114114
/** Fetches meeting count scoped to a committee. */
115115
public getMeetingsCountByCommittee(committeeId: string): Observable<number> {
116-
const params = new HttpParams().set('tags', `committee_uid:${committeeId}`);
117-
return this.http.get<QueryServiceCountResponse>('/api/meetings/count', { params }).pipe(
118-
catchError((error) => {
119-
console.error('Failed to load meetings count:', error);
120-
return of({ count: 0 });
121-
}),
122-
map((response) => response.count)
123-
);
116+
return this.getCountByTag('/api/meetings/count', `committee_uid:${committeeId}`);
124117
}
125118

126119
/** Fetches upcoming meetings scoped to a committee. */
@@ -147,19 +140,11 @@ export class MeetingService {
147140
}
148141

149142
public getMeetingsCountByProject(uid: string): Observable<number> {
150-
const params = new HttpParams().set('tags', `project_uid:${uid}`);
151-
return this.http
152-
.get<QueryServiceCountResponse>('/api/meetings/count', { params })
153-
.pipe(
154-
catchError((error) => {
155-
console.error('Failed to load meetings count:', error);
156-
return of({ count: 0 });
157-
})
158-
)
159-
.pipe(
160-
// Extract just the count number from the response
161-
map((response) => response.count)
162-
);
143+
return this.getCountByTag('/api/meetings/count', `project_uid:${uid}`);
144+
}
145+
146+
public getPastMeetingsCountByProject(uid: string): Observable<number> {
147+
return this.getCountByTag('/api/past-meetings/count', `project_uid:${uid}`);
163148
}
164149

165150
public getRecentMeetingsByProject(uid: string): Observable<Meeting[]> {
@@ -602,4 +587,15 @@ export class MeetingService {
602587
}
603588
}
604589
}
590+
591+
private getCountByTag(endpoint: string, tag: string): Observable<number> {
592+
const params = new HttpParams().set('tags', tag);
593+
return this.http.get<QueryServiceCountResponse>(endpoint, { params }).pipe(
594+
catchError((error) => {
595+
console.error(`Failed to load count from ${endpoint}:`, error);
596+
return of({ count: 0 });
597+
}),
598+
map((response) => response.count)
599+
);
600+
}
605601
}

‎apps/lfx-one/src/server/controllers/past-meeting.controller.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,24 @@ export class PastMeetingController {
5353
}
5454
}
5555

56+
/**
57+
* GET /past-meetings/count
58+
*/
59+
public async getPastMeetingsCount(req: Request, res: Response, next: NextFunction): Promise<void> {
60+
const startTime = logger.startOperation(req, 'get_past_meetings_count', {
61+
query_params: logger.sanitize(req.query as Record<string, any>),
62+
});
63+
64+
try {
65+
const count = await this.meetingService.getMeetingsCount(req, req.query as Record<string, any>, 'v1_past_meeting');
66+
67+
logger.success(req, 'get_past_meetings_count', startTime, { count });
68+
res.json({ count });
69+
} catch (error) {
70+
next(error);
71+
}
72+
}
73+
5674
/**
5775
* GET /past-meetings/:uid
5876
*/

‎apps/lfx-one/src/server/routes/past-meetings.route.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ const pastMeetingController = new PastMeetingController();
1111
// Past meeting routes
1212
router.get('/', (req, res, next) => pastMeetingController.getPastMeetings(req, res, next));
1313

14+
// GET /past-meetings/count - get past meetings count
15+
router.get('/count', (req, res, next) => pastMeetingController.getPastMeetingsCount(req, res, next));
16+
1417
// Get past meeting participants by UID
1518
router.get('/:uid/participants', (req, res, next) => pastMeetingController.getPastMeetingParticipants(req, res, next));
1619

‎apps/lfx-one/src/server/services/committee.service.ts‎

Lines changed: 50 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1329,6 +1329,56 @@ export class CommitteeService {
13291329
});
13301330
}
13311331

1332+
/**
1333+
* Batch-fetches committee resources by UID from the query service.
1334+
* Chunks UIDs at 100 per request (URL-length guard) using `filters_or=uid:X`
1335+
* for OR semantics on data.uid. Returns a map keyed by `uid` for O(1) lookup.
1336+
*/
1337+
private async getCommitteesByIds(req: Request, uids: string[]): Promise<Map<string, Committee>> {
1338+
const unique = Array.from(new Set(uids)).filter(Boolean);
1339+
if (unique.length === 0) return new Map();
1340+
1341+
const BATCH_SIZE = 100;
1342+
const batches: string[][] = [];
1343+
for (let i = 0; i < unique.length; i += BATCH_SIZE) {
1344+
batches.push(unique.slice(i, i + BATCH_SIZE));
1345+
}
1346+
1347+
// Rethrow batch failures — returning [] would make callers treat real memberships as
1348+
// "committee not found" and silently drop them (defeats failOnPartial: true).
1349+
const batchResults = await Promise.all(
1350+
batches.map(async (batch) => {
1351+
try {
1352+
return await fetchAllQueryResources<Committee>(
1353+
req,
1354+
(pageToken) =>
1355+
this.microserviceProxy.proxyRequest<QueryServiceResponse<Committee>>(req, 'LFX_V2_SERVICE', '/query/resources', 'GET', {
1356+
type: 'committee',
1357+
filters_or: batch.map((uid) => `uid:${uid}`),
1358+
...(pageToken && { page_token: pageToken }),
1359+
}),
1360+
{ failOnPartial: true }
1361+
);
1362+
} catch (error) {
1363+
logger.warning(req, 'get_committees_by_ids', 'Batched committee fetch failed', {
1364+
batch_size: batch.length,
1365+
err: error,
1366+
});
1367+
throw error;
1368+
}
1369+
})
1370+
);
1371+
1372+
const byUid = new Map<string, Committee>();
1373+
for (const committee of batchResults.flat()) {
1374+
if (committee?.uid) {
1375+
byUid.set(committee.uid, committee);
1376+
}
1377+
}
1378+
1379+
return byUid;
1380+
}
1381+
13321382
/**
13331383
* Fetches the caller's membership row for a single committee, or null if none.
13341384
* Uses the username-tagged query so visibility is independent of which email
@@ -1419,56 +1469,6 @@ export class CommitteeService {
14191469
});
14201470
}
14211471

1422-
/**
1423-
* Batch-fetches committee resources by UID from the query service.
1424-
* Chunks UIDs at 100 per request (URL-length guard) using `filters_or=uid:X`
1425-
* for OR semantics on data.uid. Returns a map keyed by `uid` for O(1) lookup.
1426-
*/
1427-
private async getCommitteesByIds(req: Request, uids: string[]): Promise<Map<string, Committee>> {
1428-
const unique = Array.from(new Set(uids)).filter(Boolean);
1429-
if (unique.length === 0) return new Map();
1430-
1431-
const BATCH_SIZE = 100;
1432-
const batches: string[][] = [];
1433-
for (let i = 0; i < unique.length; i += BATCH_SIZE) {
1434-
batches.push(unique.slice(i, i + BATCH_SIZE));
1435-
}
1436-
1437-
// Rethrow batch failures — returning [] would make callers treat real memberships as
1438-
// "committee not found" and silently drop them (defeats failOnPartial: true).
1439-
const batchResults = await Promise.all(
1440-
batches.map(async (batch) => {
1441-
try {
1442-
return await fetchAllQueryResources<Committee>(
1443-
req,
1444-
(pageToken) =>
1445-
this.microserviceProxy.proxyRequest<QueryServiceResponse<Committee>>(req, 'LFX_V2_SERVICE', '/query/resources', 'GET', {
1446-
type: 'committee',
1447-
filters_or: batch.map((uid) => `uid:${uid}`),
1448-
...(pageToken && { page_token: pageToken }),
1449-
}),
1450-
{ failOnPartial: true }
1451-
);
1452-
} catch (error) {
1453-
logger.warning(req, 'get_committees_by_ids', 'Batched committee fetch failed', {
1454-
batch_size: batch.length,
1455-
err: error,
1456-
});
1457-
throw error;
1458-
}
1459-
})
1460-
);
1461-
1462-
const byUid = new Map<string, Committee>();
1463-
for (const committee of batchResults.flat()) {
1464-
if (committee?.uid) {
1465-
byUid.set(committee.uid, committee);
1466-
}
1467-
}
1468-
1469-
return byUid;
1470-
}
1471-
14721472
/**
14731473
* Fetches committee settings by ID.
14741474
* By default returns {} on error so callers that display settings can degrade gracefully.

‎apps/lfx-one/src/server/services/meeting.service.ts‎

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { QueryServiceMeetingType } from '@lfx-one/shared/enums';
55
import {
66
ApiResponse,
77
AttachmentDownloadUrlResponse,
8+
Committee,
89
CreateMeetingAttachmentRequest,
910
CreateMeetingRegistrantRequest,
1011
CreateMeetingRequest,
@@ -48,7 +49,6 @@ import { pollEndpoint } from '../helpers/poll-endpoint.helper';
4849
import { fetchAllQueryResources } from '../helpers/query-service.helper';
4950
import { getEffectiveEmail, getEffectiveUsername, getUsernameFromAuth, stripAuthPrefix } from '../utils/auth-helper';
5051
import { AccessCheckService } from './access-check.service';
51-
import { CommitteeService } from './committee.service';
5252
import { logger } from './logger.service';
5353
import { MicroserviceProxyService } from './microservice-proxy.service';
5454
import { ProjectService } from './project.service';
@@ -59,13 +59,11 @@ import { ProjectService } from './project.service';
5959
export class MeetingService {
6060
private accessCheckService: AccessCheckService;
6161
private microserviceProxy: MicroserviceProxyService;
62-
private committeeService: CommitteeService;
6362
private projectService: ProjectService;
6463

6564
public constructor() {
6665
this.accessCheckService = new AccessCheckService();
6766
this.microserviceProxy = new MicroserviceProxyService();
68-
this.committeeService = new CommitteeService();
6967
this.projectService = new ProjectService();
7068
}
7169

@@ -1441,22 +1439,47 @@ export class MeetingService {
14411439
unique_committees: uniqueCommitteeUids.length,
14421440
});
14431441

1444-
const results = await Promise.all(
1445-
uniqueCommitteeUids.map(async (uid) => {
1446-
try {
1447-
const committee = await this.committeeService.getCommitteeById(req, uid);
1448-
return { uid, name: committee.name };
1449-
} catch (error) {
1450-
logger.warning(req, 'get_meeting_committees', 'Committee enrichment failed; continuing without name', { committee_uid: uid, err: error });
1451-
return { uid, name: undefined };
1452-
}
1453-
})
1442+
const unique = uniqueCommitteeUids.filter(Boolean);
1443+
const BATCH_SIZE = 100;
1444+
const batches: string[][] = [];
1445+
for (let i = 0; i < unique.length; i += BATCH_SIZE) {
1446+
batches.push(unique.slice(i, i + BATCH_SIZE));
1447+
}
1448+
1449+
// Use Promise.allSettled so one transient batch failure doesn't wipe names resolved by other
1450+
// batches — mirrors the pattern in getCommitteesWithMailingList.
1451+
// In practice a project's displayed meetings touch ≤1 unique committee, so this is almost always
1452+
// a single concurrent request; batching guards only against pathological cardinality.
1453+
const results = await Promise.allSettled(
1454+
batches.map((batch) =>
1455+
fetchAllQueryResources<Committee>(
1456+
req,
1457+
(pageToken) =>
1458+
this.microserviceProxy.proxyRequest<QueryServiceResponse<Committee>>(req, 'LFX_V2_SERVICE', '/query/resources', 'GET', {
1459+
type: 'committee',
1460+
filters_or: batch.map((uid) => `uid:${uid}`),
1461+
...(pageToken && { page_token: pageToken }),
1462+
}),
1463+
{ failOnPartial: true }
1464+
)
1465+
)
14541466
);
14551467

14561468
const nameMap = new Map<string, string>();
1457-
for (const { uid, name } of results) {
1458-
if (name) {
1459-
nameMap.set(uid, name);
1469+
for (const [i, result] of results.entries()) {
1470+
if (result.status === 'fulfilled') {
1471+
for (const committee of result.value) {
1472+
if (committee?.uid && committee.name) {
1473+
nameMap.set(committee.uid, committee.name);
1474+
}
1475+
}
1476+
} else {
1477+
logger.warning(req, 'get_meeting_committees', 'Batch committee fetch failed; affected meetings will have no committee name', {
1478+
batch_index: i,
1479+
batch_size: batches[i].length,
1480+
sample_uids: batches[i].slice(0, 3),
1481+
err: result.reason,
1482+
});
14601483
}
14611484
}
14621485

0 commit comments

Comments
 (0)