feat(meetings): add recording and summary features for past meetings #125
Conversation
- Add PastMeetingRecording interfaces to shared package - Implement OpenSearch query for past meeting recordings - Create recording modal with URL copy and view functionality - Add recording icon to past meeting cards - Select session with largest total_size for share URL - Add View Recording button to open URL directly in new tab [LFXV2-667] Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
- Add PastMeetingSummary interfaces to shared package - Implement OpenSearch query for past meeting summaries - Create summary modal with HTML content rendering - Add summary icon to past meeting cards - Display AI-generated summary with proper formatting - Fix recording modal URL display to show beginning of URL - Replace input with div element to control scroll position - Add click-to-select functionality for URL copying [LFXV2-667] Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
Allow users to edit AI-generated meeting summaries and approve them for viewing by others. Summaries can be edited independently of approval status, and approval is a separate action requiring explicit user confirmation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughAdds past-meeting recording and AI-summary features: UI buttons and modals, MeetingCard signals/effects to fetch/manage data, client MeetingService endpoints, server routes/controllers/services with ETag update logic, and new shared TypeScript interfaces for recordings and summaries. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Card as MeetingCard
participant Client as Client MeetingService
participant Server as Backend API
participant RModal as RecordingModal
participant SModal as SummaryModal
Note over User,Card: View Recording (client-side)
User->>Card: Click "View Recording"
Card->>Card: openRecordingModal()
Card->>RModal: open with shareUrl, meetingTitle
RModal->>User: display URL / Copy / Open
Note over User,Server: View/Edit/Approve Summary (round-trip)
User->>Card: Click "View Summary"
Card->>Client: getPastMeetingSummary(pastMeetingUid)
Client->>Server: GET /past-meetings/:uid/summary
Server-->>Client: PastMeetingSummary or 404/null
Client-->>Card: summary data
Card->>SModal: open with summary data
alt Edit & Save
User->>SModal: Edit -> Save
SModal->>Client: updatePastMeetingSummary(uid, summaryUid, edited)
Client->>Server: PUT /past-meetings/:uid/summary/:summaryUid (with ETag)
Server-->>Client: Updated summary
Client-->>SModal: updated summary -> SModal closes with updated payload
end
alt Approve
User->>SModal: Approve
SModal->>Client: approvePastMeetingSummary(uid, summaryUid)
Client->>Server: PUT /past-meetings/:uid/summary/:summaryUid {approved:true}
Server-->>Client: Approved summary
Client-->>SModal: updated approved state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
requested review from
asithade,
audigregorie,
dealako and
mauriciozanettisalomao
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR adds recording and summary features for past meetings, implementing functionality to view Zoom recordings and AI-generated meeting summaries with editing and approval capabilities.
Key Changes:
- Added backend and frontend support for fetching and displaying meeting recordings from past meetings
- Implemented summary viewing, editing, and approval workflow with ETag-based concurrency control
- Created modal components for recording and summary display with appropriate user interactions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/meeting.interface.ts | Added TypeScript interfaces for recording sessions, files, summaries, and update requests |
| apps/lfx-one/src/server/services/meeting.service.ts | Implemented service methods to fetch recordings, summaries, and update summaries with ETag support |
| apps/lfx-one/src/server/routes/past-meetings.route.ts | Added API routes for recording, summary retrieval, and summary updates |
| apps/lfx-one/src/server/controllers/past-meeting.controller.ts | Created controller methods for handling recording and summary requests with validation |
| apps/lfx-one/src/app/shared/services/meeting.service.ts | Added client-side service methods for fetching and updating recordings and summaries |
| apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts | Created modal component for viewing, editing, and approving meeting summaries |
| apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html | Template for summary modal with edit mode and approval UI |
| apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts | Created modal component for displaying and sharing recording URLs |
| apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.html | Template for recording modal with URL display and copy functionality |
| apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts | Integrated recording and summary features into meeting cards with fetch and display logic |
| apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html | Added UI buttons for viewing recordings and summaries on past meeting cards |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
andrest50
changed the title
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-125.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: 5
🧹 Nitpick comments (10)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html (1)
52-73: Icon-only buttons: ensure accessible labels.Buttons are icon-only; add ariaLabel (or equivalent on lfx-button) so screen readers announce the action. Keep current data-testid naming for local consistency.
- <lfx-button + <lfx-button + ariaLabel="View recording" icon="fa-light fa-video" ... - <lfx-button + <lfx-button + ariaLabel="View summary" icon="fa-light fa-file-lines"apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.html (1)
11-23: Make share URL field focusable and properly labeled.Use a readonly input tied to the label for keyboard/a11y and simpler selection.
- <label class="block text-sm font-medium text-gray-700 mb-2">Recording Share URL</label> - <div class="flex items-center gap-2"> - <div - class="flex-1 px-3 py-2 text-sm border border-gray-300 rounded-md bg-white text-gray-900 focus:outline-none focus:ring-2 focus:ring-blue-500 overflow-x-auto whitespace-nowrap cursor-text select-all" - data-testid="share-url-input" - (click)="selectUrl($event)"> - {{ shareUrl }} - </div> + <label for="share-url" class="block text-sm font-medium text-gray-700 mb-2">Recording Share URL</label> + <div class="flex items-center gap-2"> + <input + id="share-url" + type="text" + readonly + [value]="shareUrl" + class="flex-1 px-3 py-2 text-sm border border-gray-300 rounded-md bg-white text-gray-900 focus:outline-none focus:ring-2 focus:ring-blue-500" + data-testid="share-url-input" + (focus)="$event.target.select()" + />apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts (1)
28-35: Harden selection logic (type correctness).event.target can be a Text node. Type it as Node and use selectAllChildren for robustness.
- public selectUrl(event: MouseEvent): void { - const target = event.target as HTMLElement; - const selection = window.getSelection(); - const range = document.createRange(); - range.selectNodeContents(target); - selection?.removeAllRanges(); - selection?.addRange(range); - } + public selectUrl(event: MouseEvent): void { + const target = (event.target as Node) ?? this.config?.data?.shareUrl; + const selection = window.getSelection(); + if (!target || !selection) return; + const range = document.createRange(); + range.selectNodeContents(target); + selection.removeAllRanges(); + selection.addRange(range); + }apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts (3)
79-88: Handle ETag/precondition conflicts distinctly.On 412/409, prompt the user to reload or merge changes instead of a generic error.
- error: (error: unknown) => { - this.messageService.add({ - severity: 'error', - summary: 'Error', - detail: 'Failed to update summary', - }); + error: (error: any) => { + const status = error?.status; + const conflict = status === 412 || status === 409; + this.messageService.add({ + severity: 'error', + summary: conflict ? 'Edit conflict' : 'Error', + detail: conflict + ? 'Summary was updated elsewhere. Please refresh and retry.' + : 'Failed to update summary', + });
98-107: Avoid console noise; rely on toast/logging service.Remove console.error or gate behind env flag to keep logs clean in production.
- console.error('Failed to update summary:', error); + // Optional: route to a central logging service if needed ... - console.error('Failed to approve summary:', error); + // Optional: route to a central logging service if neededAlso applies to: 109-116
34-41: Minor UX: prevent no-op saves and return richer close payload.Disable Save when content unchanged/empty to avoid unnecessary calls; include updatedAt timestamp on close if available.
- public readonly isSaving: WritableSignal<boolean> = signal(false); + public readonly isSaving: WritableSignal<boolean> = signal(false); + public readonly canSave: Signal<boolean> = computed(() => + (this.editedContent().trim() !== '' && this.editedContent() !== this.originalContent()) && !this.isSaving() + );And in template (summary-modal.component.html):
-<lfx-button label="Save" size="small" severity="primary" [loading]="isSaving()" data-testid="save-edit-button" (click)="saveEdit()"></lfx-button> +<lfx-button label="Save" size="small" severity="primary" [disabled]="!canSave()" [loading]="isSaving()" data-testid="save-edit-button" (click)="saveEdit()"></lfx-button>Also applies to: 120-127
packages/shared/src/interfaces/meeting.interface.ts (1)
651-676: Avoid leaking sensitive fields by default.PastMeetingSummary includes password. If the client does not strictly need it, consider omitting this field from responses or marking optional to discourage accidental use on the frontend.
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
170-181: Set no-store caching for sensitive artifacts.Recording share URLs and summaries may be sensitive/ephemeral. Add Cache-Control headers to prevent caching by intermediaries.
// Send the recording data to the client - res.json(recording); + res.set('Cache-Control', 'private, no-store, must-revalidate'); + res.json(recording);// Send the summary data to the client - res.json(summary); + res.set('Cache-Control', 'private, no-store, must-revalidate'); + res.json(summary);Also applies to: 244-246
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
325-335: Only coerce 404 to null; surface other errors to UI.Swallowing all non-404 errors as null hides 401/403/5xx. Prefer rethrow for non-404 so callers can display proper error states.
return this.http.get<PastMeetingRecording>(`/api/past-meetings/${pastMeetingUid}/recording`).pipe( catchError((error) => { if (error.status === 404) { return of(null); } console.error(`Failed to load recording for past meeting ${pastMeetingUid}:`, error); - return of(null); + return throwError(() => error); }) );return this.http.get<PastMeetingSummary>(`/api/past-meetings/${pastMeetingUid}/summary`).pipe( catchError((error) => { if (error.status === 404) { return of(null); } console.error(`Failed to load summary for past meeting ${pastMeetingUid}:`, error); - return of(null); + return throwError(() => error); }) );Also applies to: 337-347
apps/lfx-one/src/server/services/meeting.service.ts (1)
460-474: Constrain query for determinism and efficiency.You take the first resource; add limit=1 and an order to ensure it’s the latest and reduce payload.
- const params = { + const params = { type: 'past_meeting_recording', tags: `past_meeting_uid:${pastMeetingUid}`, + limit: 1, + order: 'updated_at.desc', };- const params = { + const params = { type: 'past_meeting_summary', tags: `past_meeting_uid:${pastMeetingUid}`, + limit: 1, + order: 'updated_at.desc', };Also applies to: 517-529
📜 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 (11)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts(5 hunks)apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/meeting.service.ts(2 hunks)apps/lfx-one/src/server/controllers/past-meeting.controller.ts(2 hunks)apps/lfx-one/src/server/routes/past-meetings.route.ts(1 hunks)apps/lfx-one/src/server/services/meeting.service.ts(2 hunks)packages/shared/src/interfaces/meeting.interface.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/server/routes/past-meetings.route.tsapps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.tsapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.htmlpackages/shared/src/interfaces/meeting.interface.tsapps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/server/routes/past-meetings.route.tsapps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.tsapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tspackages/shared/src/interfaces/meeting.interface.tsapps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/routes/past-meetings.route.tsapps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.tsapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/server/controllers/past-meeting.controller.tspackages/shared/src/interfaces/meeting.interface.tsapps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all TypeScript interfaces in the shared package at packages/shared/src/interfaces
Files:
packages/shared/src/interfaces/meeting.interface.ts
🧬 Code graph analysis (6)
apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
Component(46-772)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(594-615)PastMeetingSummary(651-676)UpdatePastMeetingSummaryRequest(682-687)
apps/lfx-one/src/server/services/meeting.service.ts (2)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(594-615)PastMeetingSummary(651-676)UpdatePastMeetingSummaryRequest(682-687)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(594-615)PastMeetingSummary(651-676)UpdatePastMeetingSummaryRequest(682-687)
apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
Component(46-772)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (2)
PastMeetingRecording(594-615)PastMeetingSummary(651-676)
🔇 Additional comments (8)
apps/lfx-one/src/server/routes/past-meetings.route.ts (1)
26-28: Endpoint parity confirmed—no action required.The client's
approvePastMeetingSummarycallsupdatePastMeetingSummarywith{ approved: true }, which correctly maps to the PUT/:uid/summary/:summaryUidroute on the server. Approval is handled via the payload flag, not a separate endpoint—this is proper parity.packages/shared/src/interfaces/meeting.interface.ts (2)
621-635: Confirm “edited_content can be empty” contract across layers.Interfaces state edited_content can be empty. Ensure controllers/UI treat empty string as a valid, intentional override (not falsy). See controller and component comments.
594-616: LGTM on recording model additions.Shapes and field docs look solid and map cleanly to Zoom concepts.
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
149-201: Endpoint behavior and logging look solid.UID validation, 404 handling, and structured logs are consistent.
Also applies to: 203-255
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
349-356: LGTM on update/approve surface.Client delegates ETag handling to the server; API is straightforward and matches controller routes.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (2)
314-339: Recording URL derivation looks good.Largest session by total_size is selected; null-safety on sessions is handled.
267-313: Modal wiring and state sync look correct.Guards, dialog data, and onClose updates are clean.
apps/lfx-one/src/server/services/meeting.service.ts (1)
579-599: LGTM on ETag-based update flow.Fetch-with-ETag + update-with-ETag pattern is consistent with existing services.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
Outdated
Show resolved
Hide resolved
...one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts
Outdated
Show resolved
Hide resolved
...x-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html
Show resolved
Hide resolved
...lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts
Show resolved
Hide resolved
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
andrest50
force-pushed
the
andrest50/display-meeting-recordings
branch
from
cf3909c to
04e720a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
287-293: Validation fix correctly applied; consider adding runtime type guards.The field presence check using the
inoperator is now correct and allows empty strings foredited_content. The issue raised in the previous review has been properly resolved.However, consider adding runtime type validation for additional safety when called by external clients:
// Validate field presence if (!('edited_content' in body) && !('approved' in body)) { res.status(400).json({ error: 'Bad Request', message: 'Either edited_content or approved must be provided', }); return; } // Validate types if ('edited_content' in body && typeof body.edited_content !== 'string') { res.status(400).json({ error: 'Bad Request', message: 'edited_content must be a string', }); return; } if ('approved' in body && typeof body.approved !== 'boolean') { res.status(400).json({ error: 'Bad Request', message: 'approved must be a boolean', }); return; }
📜 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 (1)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/server/controllers/past-meeting.controller.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/server/controllers/past-meeting.controller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/controllers/past-meeting.controller.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(594-615)PastMeetingSummary(651-676)UpdatePastMeetingSummaryRequest(682-687)
⏰ 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: build-and-push
🔇 Additional comments (3)
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (3)
6-6: LGTM: Imports correctly include new shared interfaces.The new imports for
PastMeetingRecording,PastMeetingSummary, andUpdatePastMeetingSummaryRequestare properly typed and align with the shared interface definitions.
149-201: LGTM: Well-structured endpoint following established patterns.The
getPastMeetingRecordingmethod correctly validates the UID, fetches the recording, returns 404 when not found, and logs appropriately. The implementation is consistent with other controller methods.
203-255: LGTM: Summary retrieval endpoint correctly implemented.The
getPastMeetingSummarymethod follows the same reliable pattern asgetPastMeetingRecording, with appropriate logging of approval-related metadata.
Signed-off-by: Andres Tobon <andrest2455@gmail.com>
…king Add id attribute to meeting cards using meeting UID to enable deep linking to specific meetings via URL hash fragments. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html (1)
76-76: Usedata-testidinstead ofidfor test targeting.Per coding guidelines, Angular components should use
data-testidattributes for reliable test targeting with the naming convention[section]-[component]-[element].Apply this diff to follow the guideline:
- [attr.id]="'meeting-' + meeting.uid" + [attr.data-testid]="'meeting-list-card-' + meeting.uid"As per coding guidelines
📜 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 (3)
apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html(1 hunks)apps/lfx-one/src/server/services/meeting.service.ts(2 hunks)packages/shared/src/interfaces/meeting.interface.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/server/services/meeting.service.tspackages/shared/src/interfaces/meeting.interface.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/server/services/meeting.service.tsapps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.htmlpackages/shared/src/interfaces/meeting.interface.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/services/meeting.service.tspackages/shared/src/interfaces/meeting.interface.ts
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all TypeScript interfaces in the shared package at packages/shared/src/interfaces
Files:
packages/shared/src/interfaces/meeting.interface.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/server/services/meeting.service.ts (2)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(596-617)PastMeetingSummary(653-678)UpdatePastMeetingSummaryRequest(684-689)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
🔇 Additional comments (5)
packages/shared/src/interfaces/meeting.interface.ts (1)
550-688: LGTM!The new interfaces are well-structured and properly documented. They correctly model past meeting recordings and summaries with appropriate type safety, consistent naming conventions, and thorough JSDoc comments. The placement in the shared package follows coding guidelines.
apps/lfx-one/src/server/services/meeting.service.ts (4)
11-12: LGTM!The new interface imports are correctly sourced from the shared package and properly support the new recording and summary methods.
Also applies to: 14-14, 17-17
464-518: LGTM!The method correctly implements recording retrieval with proper error handling and logging. The pattern follows existing methods in the service (like
getPastMeetingParticipants), returningnullfor missing recordings and gracefully handling errors.
520-574: LGTM!The method correctly implements summary retrieval with consistent error handling and logging patterns. The implementation mirrors
getPastMeetingRecording, ensuring consistency across the service.
576-631: LGTM!The method correctly implements ETag-based optimistic locking for summary updates, following the established pattern used in other update methods (e.g.,
updateMeetingRegistrant). Error handling, logging, and sanitization are properly implemented.
| // Fetch recording for past meetings | ||
| effect(() => { | ||
| if (this.pastMeeting() && this.meeting().uid) { | ||
| this.fetchRecording(); | ||
| } | ||
| }); | ||
|
|
||
| // Fetch summary for past meetings | ||
| effect(() => { | ||
| if (this.pastMeeting() && this.meeting().uid) { | ||
| this.fetchSummary(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
To get the summary and recording, we should be using "toSignal" so that it's automatically loaded when the element is displayed.
The current implementation reloads the recording and summary anytime there is a signal that is updated which could be a problem.
| }); | ||
| } | ||
|
|
||
| private fetchRecording(): void { |
There was a problem hiding this comment.
We generally do not subscribe for get calls and instead use toSignal to load it async
| return largestSession.share_url || null; | ||
| } | ||
|
|
||
| private fetchSummary(): void { |
There was a problem hiding this comment.
We generally do not subscribe for get calls and instead use toSignal to load it async
| // Injected services | ||
| private readonly clipboard = inject(Clipboard); | ||
| private readonly messageService = inject(MessageService); | ||
| private readonly ref = inject(DynamicDialogRef); |
There was a problem hiding this comment.
Can you rename this to dialogRef just so it's clear what this is when used. It's a little vague for a name.
| private readonly clipboard = inject(Clipboard); | ||
| private readonly messageService = inject(MessageService); | ||
| private readonly ref = inject(DynamicDialogRef); | ||
| private readonly config = inject(DynamicDialogConfig); |
| } | ||
| } | ||
|
|
||
| public openRecording(): void { |
| @if (isEditMode()) { | ||
| <div class="bg-gray-50 rounded-lg p-4 border border-gray-200" data-testid="summary-edit-container"> | ||
| <label class="block text-sm font-medium text-gray-700 mb-2">Edit Summary</label> | ||
| <textarea |
There was a problem hiding this comment.
We should be using the shared lfx-textarea component and reactive forms here.
| private readonly ref = inject(DynamicDialogRef); | ||
| private readonly config = inject(DynamicDialogConfig); |
There was a problem hiding this comment.
Rename to be more precise: dialogRef, dialogConfig
| catchError((error) => { | ||
| if (error.status === 404) { | ||
| return of(null); | ||
| } | ||
| console.error(`Failed to load recording for past meeting ${pastMeetingUid}:`, error); | ||
| return of(null); | ||
| }) |
There was a problem hiding this comment.
Move these to the callers instead of the service. Have the callers handle errors
| catchError((error) => { | ||
| if (error.status === 404) { | ||
| return of(null); | ||
| } | ||
| console.error(`Failed to load summary for past meeting ${pastMeetingUid}:`, error); | ||
| return of(null); |
There was a problem hiding this comment.
Move this logic to the caller for pipes
…ary features - Use toSignal instead of effect for recording/summary data loading - Move recording/summary initialization to ngOnInit lifecycle hook - Replace click handler with native href for recording links - Convert summary modal to use reactive forms with lfx-textarea - Move error handling from service to component callers Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
325-339: Add error handling for consistency with existing service methods.These new methods lack error handling, which is inconsistent with similar methods in this service (e.g.,
getMeetings,getPastMeetings,getMeetingAttachmentsall includecatchError). While a past review suggested moving error handling to callers, the established pattern in this service is to handle errors at the service level.Consider adding error handling to match the existing pattern:
public getPastMeetingRecording(pastMeetingUid: string): Observable<PastMeetingRecording> { - return this.http.get<PastMeetingRecording>(`/api/past-meetings/${pastMeetingUid}/recording`); + return this.http.get<PastMeetingRecording>(`/api/past-meetings/${pastMeetingUid}/recording`).pipe( + catchError((error) => { + console.error(`Failed to load recording for past meeting ${pastMeetingUid}:`, error); + return throwError(() => error); + }) + ); } public getPastMeetingSummary(pastMeetingUid: string): Observable<PastMeetingSummary> { - return this.http.get<PastMeetingSummary>(`/api/past-meetings/${pastMeetingUid}/summary`); + return this.http.get<PastMeetingSummary>(`/api/past-meetings/${pastMeetingUid}/summary`).pipe( + catchError((error) => { + console.error(`Failed to load summary for past meeting ${pastMeetingUid}:`, error); + return throwError(() => error); + }) + ); } public updatePastMeetingSummary(pastMeetingUid: string, summaryUid: string, updateData: UpdatePastMeetingSummaryRequest): Observable<PastMeetingSummary> { - return this.http.put<PastMeetingSummary>(`/api/past-meetings/${pastMeetingUid}/summary/${summaryUid}`, updateData); + return this.http.put<PastMeetingSummary>(`/api/past-meetings/${pastMeetingUid}/summary/${summaryUid}`, updateData).pipe( + take(1), + catchError((error) => { + console.error(`Failed to update summary ${summaryUid} for past meeting ${pastMeetingUid}:`, error); + return throwError(() => error); + }) + ); }
📜 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 (6)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts(6 hunks)apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/meeting.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.ts
- apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.html
- apps/lfx-one/src/app/modules/project/meetings/components/summary-modal/summary-modal.component.ts
- apps/lfx-one/src/app/modules/project/meetings/components/recording-modal/recording-modal.component.html
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
PR: linuxfoundation/lfx-v2-ui#125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
🧬 Code graph analysis (2)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (3)
PastMeetingRecording(596-617)PastMeetingSummary(653-678)UpdatePastMeetingSummaryRequest(684-689)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (2)
PastMeetingRecording(596-617)PastMeetingSummary(653-678)
⏰ 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: build-and-push
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
🧹 Deployment RemovedThe deployment for PR #125 has been removed. |
Summary
Changes
Recording Display (LFXV2-667)
Summary Features (LFXV2-668)
edited_contentover originalcontentTechnical Details
Demo
Screen.Recording.2025-10-21.at.2.43.53.PM.mov
Tickets
🤖 Generated with Claude Code