Skip to content

Conversation

@asithade
Copy link
Contributor

@asithade asithade commented Sep 9, 2025

Summary

This PR implements new API endpoints for retrieving past meetings and updates the frontend service to properly filter past meetings by project, addressing the TODO comment in getPastMeetingsByProject().

Changes Made

Backend Implementation

  • Updated MeetingService to accept meetingType parameter in getMeetings() and getMeetingById() methods
  • Created PastMeetingController that handles past meeting requests using 'past_meeting' type
  • Created past-meetings routes with two endpoints:
    • GET /api/past-meetings - Lists all past meetings
    • GET /api/past-meetings/:uid - Gets specific past meeting by UID
  • Registered routes in server.ts at /api/past-meetings

Frontend Implementation

  • Added getPastMeetings() method for general past meeting queries
  • Updated getPastMeetingsByProject() to use new past-meetings endpoint (removed TODO)
  • Added getPastMeeting() method for individual past meeting retrieval

Technical Details

  • Reuses existing MeetingService logic by parameterizing meeting type
  • Maintains consistent API structure and error handling
  • No code duplication - clean separation of concerns
  • All endpoints include registrant counting logic

Test Plan

  • Code passes linting
  • Build succeeds without errors
  • Pre-commit hooks pass
  • All formatting applied

Breaking Changes

None - this is a purely additive change.

Linked Issues

Resolves LFXV2-450


Generated with Claude Code

- Update MeetingService to accept meetingType parameter for filtering
- Add PastMeetingController for handling past meeting requests
- Create past-meetings routes for GET / and GET /:uid endpoints
- Register past-meetings routes in server at /api/past-meetings
- Add getPastMeetings() method to frontend meeting service
- Update getPastMeetingsByProject() to use new past-meetings endpoint
- Add getPastMeeting() method for individual past meeting retrieval

Resolves LFXV2-450

Generated with [Claude Code](https://claude.ai/code)

Signed-off-by: Asitha de Silva <[email protected]>
Copilot AI review requested due to automatic review settings September 9, 2025 19:23
@asithade asithade requested a review from jordane as a code owner September 9, 2025 19:23
@coderabbitai
Copy link

coderabbitai bot commented Sep 9, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a past meetings feature across client and server: new client service methods and endpoint usage, a server controller with list/detail handlers for past meetings, routing for /api/past-meetings, and server MeetingService updates to accept a meeting type parameter enabling past meeting retrieval.

Changes

Cohort / File(s) Summary
Client service: past meetings APIs
apps/lfx-pcc/src/app/shared/services/meeting.service.ts
Adds getPastMeetings(params?) and getPastMeeting(id); updates getPastMeetingsByProject to call the new endpoint; includes error handling and state update via tap.
Server controller: past meetings
apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
New PastMeetingController with getPastMeetings and getPastMeetingById; fetches meetings of type 'past_meeting', enriches with registrant counts, uses structured logging and error forwarding.
Server routing and mount points
apps/lfx-pcc/src/server/routes/past-meetings.route.ts, apps/lfx-pcc/src/server/server.ts
Adds router for GET / and GET /:uid; mounts at /api/past-meetings (appears in both non-protected and protected API sections).
Server service: meeting type parameterization
apps/lfx-pcc/src/server/services/meeting.service.ts
Extends getMeetings and getMeetingById signatures to accept a meetingType parameter (default 'meeting'); internal queries use provided type enabling 'past_meeting'.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as Client UI
  participant Api as API /api/past-meetings
  participant Ctl as PastMeetingController
  participant Svc as MeetingService (server)
  Note over UI,Api: List past meetings
  User->>UI: View Past Meetings
  UI->>Api: GET /api/past-meetings?...
  Api->>Ctl: route
  Ctl->>Svc: getMeetings(req, query, 'past_meeting')
  Svc-->>Ctl: Meeting[]
  loop for each meeting
    Ctl->>Svc: getMeetingRegistrants(uid)
    Svc-->>Ctl: registrants
    Ctl->>Ctl: compute counts
  end
  Ctl-->>Api: enriched Meeting[]
  Api-->>UI: 200 JSON
  UI-->>User: Render list
Loading
sequenceDiagram
  autonumber
  actor User
  participant UI as Client UI
  participant Api as API /api/past-meetings/:uid
  participant Ctl as PastMeetingController
  participant Svc as MeetingService (server)
  Note over UI,Api: Past meeting details
  User->>UI: Open Past Meeting
  UI->>Api: GET /api/past-meetings/:uid
  Api->>Ctl: route (+validate UID)
  Ctl->>Svc: getMeetingById(req, uid, 'past_meeting')
  Svc-->>Ctl: Meeting
  opt registrant enrichment
    Ctl->>Svc: getMeetingRegistrants(uid)
    Svc-->>Ctl: registrants
    Ctl->>Ctl: attach counts
  end
  Ctl-->>Api: Meeting (with counts)
  Api-->>UI: 200 JSON
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Pre-merge checks (5 passed)

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change by indicating the addition of past-meetings API endpoints alongside the necessary frontend integration, making it clear and specific without extra noise or generic wording.
Linked Issues Check ✅ Passed All coding objectives from issue LFXV2-450 are met: the backend MeetingService now accepts a meetingType parameter, a PastMeetingController with the two required endpoints is implemented and registered in the routes and server, the endpoints include registrant counting logic, and the frontend service adds getPastMeetings, updates getPastMeetingsByProject, and introduces getPastMeeting.
Out of Scope Changes Check ✅ Passed All changes are directly related to implementing past-meetings functionality as specified in the linked issue, with no unrelated files or features introduced outside the scope of adding these endpoints and frontend support.
Description Check ✅ Passed The pull request description thoroughly outlines both backend and frontend changes related to past-meetings functionality, matching the summarized file modifications and objectives without straying into unrelated topics.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 605a1bf and 49f2342.

📒 Files selected for processing (1)
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
⏰ 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: E2E Tests / Playwright E2E Tests
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/LFXV2-450

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds new API endpoints specifically for retrieving past meetings, providing a clean separation between current and past meeting functionality. The implementation reuses existing MeetingService logic by parameterizing the meeting type.

  • Adds backend endpoints for past meetings at /api/past-meetings and /api/past-meetings/:uid
  • Updates MeetingService to accept configurable meeting types with 'past_meeting' support
  • Implements frontend service methods to consume the new past meeting endpoints

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
apps/lfx-pcc/src/server/services/meeting.service.ts Adds meetingType parameter to support both regular and past meeting queries
apps/lfx-pcc/src/server/server.ts Registers new past-meetings route handler
apps/lfx-pcc/src/server/routes/past-meetings.route.ts Defines REST endpoints for past meeting operations
apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts Implements controller logic for handling past meeting requests
apps/lfx-pcc/src/app/shared/services/meeting.service.ts Adds frontend methods for consuming past meeting endpoints

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
apps/lfx-pcc/src/app/shared/services/meeting.service.ts (1)

41-48: DRY: factor shared GET+catchError logic

getMeetings() and getPastMeetings() are identical aside from the path. A tiny helper reduces duplication.

+  private fetchMeetings(endpoint: string, params?: HttpParams): Observable<Meeting[]> {
+    return this.http.get<Meeting[]>(endpoint, { params }).pipe(
+      catchError((error) => {
+        console.error(`Failed to load from ${endpoint}:`, error);
+        return of([]);
+      })
+    );
+  }
   public getPastMeetings(params?: HttpParams): Observable<Meeting[]> {
-    return this.http.get<Meeting[]>('/api/past-meetings', { params }).pipe(
-      catchError((error) => {
-        console.error('Failed to load past meetings:', error);
-        return of([]);
-      })
-    );
+    return this.fetchMeetings('/api/past-meetings', params);
   }
apps/lfx-pcc/src/server/services/meeting.service.ts (2)

41-46: Parametric meetingType is solid; guard the type

Default preserves back-compat. Consider narrowing meetingType to known literals to prevent accidental misuse.

-  public async getMeetings(req: Request, query: Record<string, any> = {}, meetingType: string = 'meeting'): Promise<Meeting[]> {
+  type MeetingResourceType = 'meeting' | 'past_meeting';
+  public async getMeetings(req: Request, query: Record<string, any> = {}, meetingType: MeetingResourceType = 'meeting'): Promise<Meeting[]> {
     const params = {
       ...query,
       type: meetingType,
     };

64-67: Same here for getMeetingById

Apply the same literal-typing to keep the API tight.

-  public async getMeetingById(req: Request, meetingUid: string, meetingType: string = 'meeting'): Promise<Meeting> {
+  public async getMeetingById(req: Request, meetingUid: string, meetingType: 'meeting' | 'past_meeting' = 'meeting'): Promise<Meeting> {
     const params = {
       type: meetingType,
       tags: `meeting_uid:${meetingUid}`,
     };
apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (2)

19-58: Optional: cap concurrency on registrant fetches

For large result sets, N parallel calls can hammer the backend. Consider batching (e.g., size 8–16) or a small p-limit.


81-83: Avoid magic string for meeting type

Define a constant (or reuse a shared enum/type) to prevent typos across handlers.

Example (apply near class top):

const PAST_MEETING_TYPE = 'past_meeting' as const;
// ...
const meeting = await this.meetingService.getMeetingById(req, uid, PAST_MEETING_TYPE);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a78f2d5 and 605a1bf.

📒 Files selected for processing (5)
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts (3 hunks)
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (1 hunks)
  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts (1 hunks)
  • apps/lfx-pcc/src/server/server.ts (2 hunks)
  • apps/lfx-pcc/src/server/services/meeting.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Prefer TypeScript interfaces over union types when modeling shapes and contracts

Files:

  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts
  • apps/lfx-pcc/src/server/services/meeting.service.ts
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
  • apps/lfx-pcc/src/server/server.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not nest ternary expressions

Files:

  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts
  • apps/lfx-pcc/src/server/services/meeting.service.ts
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
  • apps/lfx-pcc/src/server/server.ts
apps/lfx-pcc/src/**/*.{ts,html}

📄 CodeRabbit inference engine (CLAUDE.md)

Use LFX wrapper components instead of importing PrimeNG components directly

Files:

  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts
  • apps/lfx-pcc/src/server/services/meeting.service.ts
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
  • apps/lfx-pcc/src/server/server.ts
**/*.{ts,tsx,js,jsx,html,scss,css}

📄 CodeRabbit inference engine (CLAUDE.md)

Include required license headers on all source files

Files:

  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts
  • apps/lfx-pcc/src/server/services/meeting.service.ts
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
  • apps/lfx-pcc/src/server/server.ts
apps/lfx-pcc/src/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces

Files:

  • apps/lfx-pcc/src/server/routes/past-meetings.route.ts
  • apps/lfx-pcc/src/server/services/meeting.service.ts
  • apps/lfx-pcc/src/app/shared/services/meeting.service.ts
  • apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts
  • apps/lfx-pcc/src/server/server.ts
🧠 Learnings (1)
📚 Learning: 2025-09-05T18:09:48.791Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints

Applied to files:

  • apps/lfx-pcc/src/server/server.ts
🧬 Code graph analysis (4)
apps/lfx-pcc/src/server/routes/past-meetings.route.ts (1)
apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (1)
  • PastMeetingController (13-117)
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (1)
  • Meeting (75-142)
apps/lfx-pcc/src/app/shared/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (1)
  • Meeting (75-142)
apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (3)
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
  • MeetingService (26-419)
apps/lfx-pcc/src/server/helpers/logger.ts (2)
  • Logger (10-129)
  • error (52-65)
apps/lfx-pcc/src/server/helpers/validation.helper.ts (1)
  • validateUidParameter (30-49)
⏰ 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: Code Quality Checks
🔇 Additional comments (7)
apps/lfx-pcc/src/app/shared/services/meeting.service.ts (3)

41-48: Good addition: client surface for past meetings

Endpoint, typing, and error handling mirror existing getMeetings(). Looks consistent.


86-86: Delegation to past endpoint is correct

Swapping to getPastMeetings(params) keeps the project filter behavior intact.


99-107: Detail fetch mirrors existing meeting flow

Parity with getMeeting() (tap, error propagation) looks good.

apps/lfx-pcc/src/server/services/meeting.service.ts (1)

41-46: Confirm downstream supports past_meeting type
Verify the meeting-service API accepts type=past_meeting for both list and detail calls in past-meeting.controller.ts (around lines 25 and 82).

apps/lfx-pcc/src/server/routes/past-meetings.route.ts (1)

11-16: Router wiring looks correct

Handlers are bound via lambdas, avoiding this-binding pitfalls.

apps/lfx-pcc/src/server/server.ts (1)

23-23: Mounted under protected /api with bearer extraction

Import and mount order are correct: extractBearerToken precedes /api/past-meetings.

Also applies to: 206-207

apps/lfx-pcc/src/server/controllers/past-meeting.controller.ts (1)

91-104: Good: per-request registrant count failure is isolated

Nested try/catch ensures detail route remains robust when registrant fetch fails.

@github-actions
Copy link

github-actions bot commented Sep 9, 2025

✅ E2E Tests Passed

Browser: chromium
Status: passed

All E2E tests passed successfully.

Test Configuration

- Extract registrant counting logic into private helper method
- Fix Promise.all error handling in getPastMeetings to prevent 500s
- Default registrant counts to 0 when getMeetingRegistrants fails
- Remove code duplication between getPastMeetings and getPastMeetingById
- Ensure individual meeting failures don't break entire list requests

Generated with [Claude Code](https://claude.ai/code)

Signed-off-by: Asitha de Silva <[email protected]>
@asithade asithade merged commit 60b83fa into main Sep 9, 2025
9 of 10 checks passed
@asithade asithade deleted the feat/LFXV2-450 branch September 9, 2025 22:27
@github-actions
Copy link

github-actions bot commented Sep 9, 2025

🧹 Deployment Removed

The deployment for PR #78 has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants