feat(meetings): migrate attachment management to lfx v2 api #144
Conversation
Migrate meeting attachments from Supabase to LFX V2 API endpoints with comprehensive functionality improvements. **Backend Changes:** - Add meeting attachment controller with create, list, get, and download endpoints - Implement Query Service integration for attachment listing with proper filtering - Add FormData support in ApiClientService for multipart file uploads - Fix content-type handling by using file_content_type field - Implement RFC 5987 encoding for Content-Disposition headers to handle special characters in filenames - Add proper error handling and logging for attachment operations - Restructure download logic to prioritize file retrieval over metadata **Frontend Changes:** - Convert attachment loading to reactive pattern using Angular signals - Update meeting service to use new /api/meetings/:uid/attachments endpoints - Add PendingAttachment interface for upload state tracking - Implement proper error handling and user feedback for attachment operations - Update all attachment display components to use new data structure - Add proper download links with target="_blank" and download attributes **Key Fixes:** - Fix attachment filtering to show correct attachments per meeting using tags parameter - Fix file download Content-Disposition header to properly encode filenames with spaces and special characters - Fix content-type preservation for uploaded files (e.g., PNG, PDF) - Make metadata fetching non-blocking in download endpoint Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
- Keep uploaded files visible in edit mode with checkmark indicator - Prevent duplicate uploads of already-uploaded files - Fix attachment creation in create mode on step 4 - Ensure proper navigation to guest management step after saving attachments This fixes issues where files would disappear after upload in edit mode and attachments wouldn't be created when creating new meetings. Files now show a green checkmark when successfully uploaded and remain visible in the pending attachments list. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
github-actions
bot
requested review from
asithade,
dealako and
mauriciozanettisalomao
|
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. WalkthroughThis PR migrates meeting attachments from a Supabase storage-based system to a V2 microservice API-based approach. The attachment data model is restructured (id→uid, file_name→name, introducing type and link fields), and frontend components are updated to use the new properties and API download paths. Backend controllers and services are added to handle attachment CRUD operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant MeetingManageComponent
participant MeetingService
participant MicroserviceProxy
participant ApiClient
participant V2API as V2 API Server
Frontend->>MeetingManageComponent: Upload file
MeetingManageComponent->>MeetingService: createFileAttachment(meetingId, file)
MeetingService->>MicroserviceProxy: proxyBinaryRequest(V2_MEETINGS, path)
MicroserviceProxy->>ApiClient: binaryRequest(POST, url, FormData)
ApiClient->>V2API: POST /meetings/{uid}/attachments (multipart/form-data)
V2API-->>ApiClient: MeetingAttachment {uid, name, type, link?}
ApiClient-->>MicroserviceProxy: Buffer response
MicroserviceProxy-->>MeetingService: Parsed MeetingAttachment
MeetingService-->>MeetingManageComponent: Observable<MeetingAttachment>
MeetingManageComponent->>MeetingManageComponent: Update PendingAttachment (link, uploaded=true)
rect rgb(100, 150, 200)
note right of Frontend: Download attachment
Frontend->>Frontend: href=/api/meetings/{uid}/attachments/{attachmentUid}
Frontend->>MeetingController: GET /:uid/attachments/:attachmentId
MeetingController->>MeetingService: getMeetingAttachment(meetingUid, attachmentUid)
MeetingService->>MicroserviceProxy: proxyBinaryRequest(GET)
MicroserviceProxy->>V2API: GET /meetings/{uid}/attachments/{attachmentUid}
V2API-->>MicroserviceProxy: Buffer (file data)
MicroserviceProxy-->>MeetingService: Buffer
MeetingService-->>MeetingController: Buffer
MeetingController->>MeetingController: Set Content-Type & Content-Disposition headers
MeetingController-->>Frontend: Stream file
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates meeting attachment management from Supabase to the LFX V2 API, implementing a comprehensive solution for both file uploads and link attachments with improved upload status tracking in the UI.
Key Changes
- Backend: New meeting attachment endpoints in controller and service for creating, retrieving, and deleting attachments with FormData handling for file uploads
- Frontend: Updated meeting service to use LFX V2 API, enhanced upload UX with spinner/checkmark indicators, and fixed attachment saving logic in wizard create mode
- Interface Updates: Modified
MeetingAttachmentinterface to use new field names (uid, meeting_uid, name, type) and addeduploadedfield toPendingAttachmentfor state tracking
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/interfaces/meeting-attachment.interface.ts |
Updated interface with new field names and added uploaded field to PendingAttachment; removed UploadFileResponse interface |
apps/lfx-one/src/server/services/supabase.service.ts |
Removed Supabase storage methods and attachment management code as part of migration |
apps/lfx-one/src/server/services/meeting.service.ts |
Added new methods for attachment CRUD operations via LFX V2 API |
apps/lfx-one/src/server/services/api-client.service.ts |
Enhanced to handle FormData with proper Content-Type and Content-Length headers |
apps/lfx-one/src/server/routes/meetings.route.ts |
Simplified routes by delegating to controller methods; removed old Supabase-based endpoints |
apps/lfx-one/src/server/controllers/meeting.controller.ts |
Added comprehensive attachment endpoint handlers with validation |
apps/lfx-one/src/app/shared/services/meeting.service.ts |
Updated to use new LFX V2 API endpoints; added createFileAttachment and updated createAttachmentFromUrl |
apps/lfx-one/src/app/shared/components/meeting-card/*.html |
Updated to use new field names (uid, name, type) and support both file and link attachments |
apps/lfx-one/src/app/shared/components/dashboard-meeting-card/*.html |
Updated to use new field names and proper download links |
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/* |
Enhanced upload logic to distinguish between edit and create modes; added direct upload in edit mode |
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/* |
Fixed attachment saving flow in create mode and navigation to step 5; improved attachment refresh logic |
apps/lfx-one/src/app/modules/meetings/meeting-join/*.html |
Updated to use new attachment field names and support link attachments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
| // Create FormData for multipart/form-data request | ||
| const formDataClass = (await import('form-data')).default; | ||
| const formData = new formDataClass(); | ||
| formData.append('type', attachmentData.type); | ||
| formData.append('name', attachmentData.name); | ||
|
|
||
| // If file data is provided, include it (base64 encoded file from client) | ||
| if (attachmentData.file) { | ||
| const buffer = Buffer.from(attachmentData.file, 'base64'); | ||
| formData.append('file', buffer, { | ||
| filename: attachmentData.name, | ||
| contentType: attachmentData.file_content_type || 'application/octet-stream', | ||
| }); | ||
| } | ||
|
|
||
| // If link is provided instead of file, include it | ||
| if (attachmentData.link) { | ||
| formData.append('link', attachmentData.link); | ||
| } |
There was a problem hiding this comment.
There's no validation to ensure that when type is 'file', the file field is provided, or when type is 'link', the link field is provided. This could lead to incomplete attachment records. Consider adding validation:
// After type validation
if (attachmentData.type === 'file' && !attachmentData.file) {
const validationError = ServiceValidationError.forField('file', 'File data is required for file-type attachments', {
operation: 'create_meeting_attachment',
service: 'meeting_controller',
path: req.path,
});
Logger.error(req, 'create_meeting_attachment', startTime, validationError);
return next(validationError);
}
if (attachmentData.type === 'link' && !attachmentData.link) {
const validationError = ServiceValidationError.forField('link', 'Link URL is required for link-type attachments', {
operation: 'create_meeting_attachment',
service: 'meeting_controller',
path: req.path,
});
Logger.error(req, 'create_meeting_attachment', startTime, validationError);
return next(validationError);
}| private uploadFilesDirectly(files: File[], meetingId: string): void { | ||
| Array.from(files).forEach((file) => { | ||
| const validationError = this.validateFile(file); | ||
| if (validationError) { | ||
| this.messageService.add({ | ||
| severity: 'error', | ||
| summary: 'File Upload Error', | ||
| detail: validationError, | ||
| life: 5000, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const pendingAttachment: PendingAttachment = { | ||
| id: crypto.randomUUID(), | ||
| fileName: file.name, | ||
| file: file, | ||
| fileSize: file.size, | ||
| mimeType: file.type, | ||
| uploading: true, | ||
| }; | ||
|
|
||
| this.pendingAttachments.update((current) => [...current, pendingAttachment]); | ||
|
|
||
| // Upload directly to LFX V2 API | ||
| this.meetingService.createFileAttachment(meetingId, file).subscribe({ | ||
| next: (attachment) => { | ||
| // Mark as successfully uploaded instead of removing | ||
| this.pendingAttachments.update((current) => current.map((pa) => (pa.id === pendingAttachment.id ? { ...pa, uploading: false, uploaded: true } : pa))); | ||
| this.messageService.add({ | ||
| severity: 'success', | ||
| summary: 'Success', | ||
| detail: `File "${file.name}" uploaded successfully`, | ||
| life: 3000, | ||
| }); | ||
| // Emit event to parent to refresh attachments list | ||
| this.uploadSuccess.emit(attachment); | ||
| }, | ||
| error: (error) => { | ||
| this.pendingAttachments.update((current) => | ||
| current.map((pa) => (pa.id === pendingAttachment.id ? { ...pa, uploading: false, uploadError: error.message || 'Upload failed' } : pa)) | ||
| ); | ||
| console.error(`Failed to upload ${file.name}:`, error); | ||
| this.messageService.add({ | ||
| severity: 'error', | ||
| summary: 'Upload Failed', | ||
| detail: `Failed to upload "${file.name}". Please try again.`, | ||
| life: 5000, | ||
| }); | ||
| }, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The duplicate file detection only checks against pendingAttachments() but not against existingAttachments(). In edit mode, this allows users to upload files that have the same name as already-uploaded files, potentially causing confusion. Consider also checking against existing attachments when in edit mode to prevent duplicate file names.
| } catch { | ||
| // Fallback: try to use the FormData directly as a stream | ||
| const contentLength = data.getLengthSync?.(); | ||
| if (contentLength) { | ||
| headers['Content-Length'] = String(contentLength); | ||
| } | ||
| requestInit.body = data as any; | ||
| } |
There was a problem hiding this comment.
The error handling in the FormData buffer conversion swallows all errors silently. If getBuffer() fails, the catch block falls back to using the FormData directly, but there's no logging or indication that an error occurred. Consider adding error logging to help with debugging:
} catch (error) {
// Log the error for debugging
console.warn('Failed to get FormData buffer, falling back to stream:', error);
// Fallback: try to use the FormData directly as a stream
...
}| // If file data is provided, include it (base64 encoded file from client) | ||
| if (attachmentData.file) { | ||
| const buffer = Buffer.from(attachmentData.file, 'base64'); | ||
| formData.append('file', buffer, { | ||
| filename: attachmentData.name, | ||
| contentType: attachmentData.file_content_type || 'application/octet-stream', | ||
| }); | ||
| } |
There was a problem hiding this comment.
File size and MIME type validation is only performed on the frontend and can be easily bypassed. The backend should validate the decoded file size before creating the buffer and uploading to prevent:
- Denial of service attacks with excessively large files
- Upload of potentially dangerous file types
Consider adding validation:
// After decoding the base64 file
if (attachmentData.file) {
const buffer = Buffer.from(attachmentData.file, 'base64');
// Validate file size (e.g., 10MB limit)
const MAX_FILE_SIZE = 10 * 1024 * 1024;
if (buffer.length > MAX_FILE_SIZE) {
const validationError = ServiceValidationError.forField('file', `File size exceeds maximum of ${MAX_FILE_SIZE / 1024 / 1024}MB`, {
operation: 'create_meeting_attachment',
service: 'meeting_controller',
path: req.path,
});
Logger.error(req, 'create_meeting_attachment', startTime, validationError);
return next(validationError);
}
// Validate content type if provided
if (attachmentData.file_content_type) {
const ALLOWED_TYPES = [/* list of allowed MIME types */];
if (!ALLOWED_TYPES.includes(attachmentData.file_content_type)) {
const validationError = ServiceValidationError.forField('file_content_type', 'File type not allowed', {
operation: 'create_meeting_attachment',
service: 'meeting_controller',
path: req.path,
});
Logger.error(req, 'create_meeting_attachment', startTime, validationError);
return next(validationError);
}
}
formData.append('file', buffer, { ... });
}| [deletingAttachmentId]="deletingAttachmentId()" | ||
| [meetingId]="meetingId()" | ||
| (goToStep)="goToStep($event)" | ||
| (deleteAttachment)="deleteAttachment($event)"> |
There was a problem hiding this comment.
The uploadSuccess event is bound in the stepper mode (line 60) but not in the single-page mode (line 119). This means that when a file is uploaded in edit mode using the single-page layout, the onAttachmentUploadSuccess handler won't be called, and the newly uploaded attachment won't be added to the attachments list immediately. Consider adding the event binding for consistency:
<lfx-meeting-resources-summary
[form]="form()"
[existingAttachments]="attachments()"
[isEditMode]="isEditMode()"
[deletingAttachmentId]="deletingAttachmentId()"
[meetingId]="meetingId()"
(goToStep)="goToStep($event)"
(deleteAttachment)="deleteAttachment($event)"
(uploadSuccess)="onAttachmentUploadSuccess($event)">
</lfx-meeting-resources-summary>| (deleteAttachment)="deleteAttachment($event)"> | |
| (deleteAttachment)="deleteAttachment($event)" | |
| (uploadSuccess)="onAttachmentUploadSuccess($event)"> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
822-846: Add state update to mark successfully uploaded attachments.The
uploadedflag is never set for successful uploads in the meeting-manage component. AftersavePendingAttachmentscompletes successfully, you must updatependingAttachmentswithuploaded: truefor each successful file. Without this, the filter on line 824 will not exclude already-uploaded files on subsequent saves, creating a duplicate upload vulnerability.Update
handleAttachmentResultsto mark successful attachments as uploaded before navigating or moving to the next step:// After logging failures, add: successes.forEach((success) => { this.pendingAttachments.update((current) => current.map((pa) => (pa.file?.name === success.fileName ? { ...pa, uploaded: true } : pa)) ); });
🧹 Nitpick comments (3)
apps/lfx-one/src/app/shared/services/meeting.service.ts (3)
249-268: Replaceanytype with a proper interface.The
attachmentDataobject on line 253 uses theanytype, reducing type safety. Define an interface for the file attachment payload to ensure compile-time validation.Consider defining an interface like:
interface CreateFileAttachmentPayload { type: 'file'; name: string; file: string; // base64 encoded file_content_type: string; }Then use it:
- const attachmentData = { + const attachmentData: CreateFileAttachmentPayload = { type: 'file',
270-287: Replaceanytype and remove outdated comment.Line 274 uses
anytype forattachmentData, and the comment on lines 271-273 references file-type fields that are not used in this method.Apply this diff:
public createAttachmentFromUrl(meetingId: string, name: string, url: string): Observable<MeetingAttachment> { - // Build attachment data based on the API schema - // For link-type attachments: type, name, link (and optionally description) - // For file-type attachments: type, name, file, file_name, file_content_type - const attachmentData: any = { + // Create link-type attachment + const attachmentData = { type: 'link' as const, name: name, link: url,
249-268: Consider adding file size validation.Before encoding files to base64 (which increases size by ~33%), validate the file size to prevent uploading excessively large files that may exceed API limits or cause performance issues.
Example check before encoding:
const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB if (file.size > MAX_FILE_SIZE) { return throwError(() => new Error(`File size exceeds ${MAX_FILE_SIZE / 1024 / 1024}MB limit`)); }
📜 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 (15)
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html(2 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html(2 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(8 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html(2 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts(4 hunks)apps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.html(1 hunks)apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html(1 hunks)apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/meeting.service.ts(1 hunks)apps/lfx-one/src/server/controllers/meeting.controller.ts(1 hunks)apps/lfx-one/src/server/routes/meetings.route.ts(1 hunks)apps/lfx-one/src/server/services/api-client.service.ts(2 hunks)apps/lfx-one/src/server/services/meeting.service.ts(1 hunks)apps/lfx-one/src/server/services/supabase.service.ts(0 hunks)packages/shared/src/interfaces/meeting-attachment.interface.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/lfx-one/src/server/services/supabase.service.ts
🧰 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/app/shared/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/services/api-client.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.tspackages/shared/src/interfaces/meeting-attachment.interface.tsapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.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/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.htmlapps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.htmlapps/lfx-one/src/server/services/api-client.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.htmlapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.tsapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.htmlpackages/shared/src/interfaces/meeting-attachment.interface.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.htmlapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/routes/meetings.route.tsapps/lfx-one/src/server/services/api-client.service.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/server/controllers/meeting.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.tspackages/shared/src/interfaces/meeting-attachment.interface.tsapps/lfx-one/src/app/shared/services/meeting.service.tsapps/lfx-one/src/server/services/meeting.service.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/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.htmlapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.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-attachment.interface.ts
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 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/shared/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.tsapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html
🧬 Code graph analysis (5)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (2)
packages/shared/src/interfaces/meeting-attachment.interface.ts (1)
MeetingAttachment(8-31)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts (1)
importantLinksFormArray(38-40)
apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
apps/lfx-one/src/server/errors/index.ts (1)
ServiceValidationError(7-7)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts (1)
packages/shared/src/interfaces/meeting-attachment.interface.ts (2)
MeetingAttachment(8-31)PendingAttachment(56-73)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting-attachment.interface.ts (1)
MeetingAttachment(8-31)
apps/lfx-one/src/server/services/meeting.service.ts (1)
packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (8)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
57-61: Good call wiring upload success to refresh statePassing
meetingIdplus the newuploadSuccessoutput keeps create-mode UX aligned with the updated resources summary component—nice touch.apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts (1)
678-691: Reactive attachment stream looks solidThe move to derive attachments from the meeting signal (with a graceful empty fallback) keeps the card in sync without redundant fetches. Nice upgrade.
apps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.html (1)
83-99: Attachment rendering handles files vs. links nicelyDifferentiating download links from external resources (unique track keys, conditional download attr, correct icon) keeps the dashboard card consistent with the new attachment model. Looks good.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (5)
152-168: LGTM: Signal-based attachments with proper change detection.The subscription properly updates the attachments signal with a new array reference (line 167), ensuring Angular detects changes within ng-template contexts like p-step-panel. The use of
takeUntilDestroyedprevents memory leaks.
250-253: LGTM: Clean integration point for attachment uploads.This public method provides a clean integration point for child components to notify the parent of successful uploads, updating the signal reactively.
506-513: LGTM: Navigation logic correctly handles create mode on step 4.In create mode on step 4 (Resources), the flow advances to step 5 (Manage Guests) after saving attachments, rather than immediately returning to the meetings list. This aligns with the PR objectives.
536-537: LGTM: Efficient direct signal mutation for deletion.Directly filtering the attachments signal is more efficient than triggering a refresh observable and avoids a redundant API call.
371-446: All edge cases properly handled in the refactored flow.Verification confirms the implementation correctly addresses each concern:
Partially uploaded files: The
savePendingAttachments()method filters attachments using!attachment.uploading && !attachment.uploadError && !attachment.uploaded && attachment.file, explicitly excluding already-uploaded files.Empty FormArray: The
saveImportantLinks()method checksif (!importantLinksFormArray || importantLinksFormArray.length === 0)and returns early.Race conditions: The observable chain completes with
take(1)and navigation occurs only afterhandleAttachmentResults()processes the combined outcome, preventing concurrent operations.Additionally, the
handleAttachmentResults()method comprehensively handles three scenarios: all successes, partial successes with failures, and complete failures, providing proper feedback for each case.
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html
Outdated
Show resolved
Hide resolved
| if (!attachmentData.type || !attachmentData.name) { | ||
| const validationError = ServiceValidationError.fromFieldErrors( | ||
| { | ||
| type: !attachmentData.type ? 'Type is required' : [], | ||
| name: !attachmentData.name ? 'Name is required' : [], | ||
| }, | ||
| 'Attachment data validation failed', | ||
| { | ||
| operation: 'create_meeting_attachment', | ||
| service: 'meeting_controller', | ||
| path: req.path, | ||
| } | ||
| ); | ||
|
|
||
| Logger.error(req, 'create_meeting_attachment', startTime, validationError); | ||
| return next(validationError); | ||
| } | ||
|
|
||
| // Create FormData for multipart/form-data request | ||
| const formDataClass = (await import('form-data')).default; | ||
| const formData = new formDataClass(); | ||
| formData.append('type', attachmentData.type); | ||
| formData.append('name', attachmentData.name); | ||
|
|
||
| // If file data is provided, include it (base64 encoded file from client) | ||
| if (attachmentData.file) { | ||
| const buffer = Buffer.from(attachmentData.file, 'base64'); | ||
| formData.append('file', buffer, { | ||
| filename: attachmentData.name, | ||
| contentType: attachmentData.file_content_type || 'application/octet-stream', | ||
| }); | ||
| } | ||
|
|
||
| // If link is provided instead of file, include it | ||
| if (attachmentData.link) { | ||
| formData.append('link', attachmentData.link); | ||
| } |
There was a problem hiding this comment.
Ensure file vs. link payload validation before proxying
We only require type and name, so a type: 'file' payload without file (or a type: 'link' without link) gets proxied to LFX and comes back as a 400/422 from the downstream API. Catching this at the BFF gives us clearer error messaging and avoids an unnecessary round‑trip. Please add explicit guards so that file uploads require attachmentData.file and link uploads require attachmentData.link, returning a local validation error when those conditions aren’t met.
🤖 Prompt for AI Agents
In apps/lfx-one/src/server/controllers/meeting.controller.ts around lines 949 to
985, the controller currently only validates type and name but does not enforce
that type === 'file' requires attachmentData.file and type === 'link' requires
attachmentData.link; add explicit guards after the existing type/name check that
if attachmentData.type === 'file' and no attachmentData.file, or
attachmentData.type === 'link' and no attachmentData.link, you construct a
ServiceValidationError.fromFieldErrors with an appropriate field error (e.g.
file: 'File is required' or link: 'Link is required'), call Logger.error(req,
'create_meeting_attachment', startTime, validationError) and return
next(validationError) so the BFF returns a clear local validation error instead
of proxying to downstream.
- Add PastMeetingAttachment interface to shared package - Add getPastMeetingAttachments method to backend meeting service - Add getPastMeetingAttachments controller method to past meeting controller - Add GET /past-meetings/:uid/attachments route - Add getPastMeetingAttachments method to frontend meeting service This enables fetching attachments for past meetings via the LFX V2 API using the Query Service with type 'past_meeting_attachment'. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
… into andrest50/meeting-attachments
…ing refactor Removed meeting attachment display and management logic from multiple components in preparation for a comprehensive refactor. This includes: - Removed attachment sections from meeting join component - Removed attachment display from dashboard meeting card - Removed attachment handling from meeting card component - Removed attachment upload logic from meeting manage component - Removed attachment display from meeting resources summary These changes prepare the codebase for a cleaner implementation of attachment functionality that will be introduced in subsequent commits. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
217-248: Remove or deprecate the unuseduploadAttachmentmethod.The
uploadAttachmentmethod (lines 217-248) is dead code with zero callers throughout the codebase. It targets the legacy endpoint/attachments/uploadwhilecreateFileAttachment(lines 250-269) targets the current/attachmentsendpoint. Since all file uploads now usecreateFileAttachment,uploadAttachmentshould either be removed entirely or marked with@deprecatedif maintaining backward compatibility is required.packages/shared/src/interfaces/meeting-attachment.interface.ts (1)
37-50: Remove unusedCreateMeetingAttachmentRequestinterface.This interface is not used anywhere in the codebase. Ripgrep search confirms zero references outside its definition. It appears to be superseded by the
MeetingAttachmentinterface and should be removed to maintain code clarity.
♻️ Duplicate comments (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts (1)
166-192: Duplicate file detection should check existing attachments.The duplicate file check (lines 179-184) only validates against
pendingAttachments()but not againstexistingAttachments(). In edit mode, this allows users to upload files with the same name as already-uploaded files.Apply this diff to check both pending and existing attachments:
- // Check for duplicate filenames in current session + // Check for duplicate filenames in current session and existing attachments const currentFiles = this.pendingAttachments(); const isDuplicate = currentFiles.some((attachment) => attachment.fileName === file.name && !attachment.uploadError); + // In edit mode, also check against existing attachments + if (this.isEditMode()) { + const existingFiles = this.existingAttachments(); + const isDuplicateExisting = existingFiles.some((attachment) => attachment.name === file.name); + if (isDuplicateExisting) { + return `A file named "${file.name}" already exists for this meeting.`; + } + } + if (isDuplicate) { return `A file named "${file.name}" has already been selected for upload.`; }
🧹 Nitpick comments (3)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
469-470: Consider removing or clarifying the comment.The comment states "Directly remove the deleted attachment from the signal array" but the actual code triggers a refresh via
attachmentsRefresh$.next()rather than directly manipulating the array. The comment may be misleading.Apply this diff to clarify the comment:
- // Directly remove the deleted attachment from the signal array + // Trigger attachments list refresh after successful deletion this.attachmentsRefresh$.next();apps/lfx-one/src/app/shared/services/meeting.service.ts (2)
271-288: Improve type safety for attachment data.The
attachmentDatavariable is typed asany(line 275), which reduces type safety. Consider defining a specific interface or inline type for the link attachment payload.Apply this diff to improve type safety:
public createAttachmentFromUrl(meetingId: string, name: string, url: string): Observable<MeetingAttachment> { - // Build attachment data based on the API schema - // For link-type attachments: type, name, link (and optionally description) - // For file-type attachments: type, name, file, file_name, file_content_type - const attachmentData: any = { + const attachmentData: { type: 'link'; name: string; link: string } = { type: 'link', name: name, link: url, };
337-339: Consider adding error handling for consistency.Similar methods like
getMeetingAttachments(line 208) includecatchErrorto handle failures gracefully. Consider adding the same pattern here to return an empty array on error.Apply this diff to add error handling:
public getPastMeetingAttachments(pastMeetingUid: string): Observable<PastMeetingAttachment[]> { - return this.http.get<PastMeetingAttachment[]>(`/api/past-meetings/${pastMeetingUid}/attachments`); + return this.http.get<PastMeetingAttachment[]>(`/api/past-meetings/${pastMeetingUid}/attachments`).pipe( + catchError((error) => { + console.error(`Failed to load attachments for past meeting ${pastMeetingUid}:`, error); + return of([]); + }) + ); }
📜 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 (16)
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html(2 hunks)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-manage/meeting-manage.component.ts(4 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html(2 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts(2 hunks)apps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.html(1 hunks)apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html(1 hunks)apps/lfx-one/src/app/shared/services/meeting.service.ts(3 hunks)apps/lfx-one/src/server/controllers/meeting.controller.ts(1 hunks)apps/lfx-one/src/server/controllers/past-meeting.controller.ts(1 hunks)apps/lfx-one/src/server/routes/meetings.route.ts(1 hunks)apps/lfx-one/src/server/routes/past-meetings.route.ts(1 hunks)apps/lfx-one/src/server/services/api-client.service.ts(3 hunks)apps/lfx-one/src/server/services/meeting.service.ts(1 hunks)apps/lfx-one/src/server/services/microservice-proxy.service.ts(1 hunks)packages/shared/src/interfaces/meeting-attachment.interface.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html
- apps/lfx-one/src/server/services/api-client.service.ts
- apps/lfx-one/src/server/services/meeting.service.ts
- apps/lfx-one/src/app/shared/components/dashboard-meeting-card/dashboard-meeting-card.component.html
- apps/lfx-one/src/server/controllers/meeting.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 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-resources-summary/meeting-resources-summary.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html
🧬 Code graph analysis (3)
apps/lfx-one/src/server/services/microservice-proxy.service.ts (2)
packages/shared/src/interfaces/api.interface.ts (1)
MicroserviceUrls(36-39)packages/shared/src/constants/api.constants.ts (1)
DEFAULT_QUERY_PARAMS(12-15)
apps/lfx-one/src/app/shared/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting-attachment.interface.ts (2)
MeetingAttachment(8-31)PastMeetingAttachment(79-104)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
packages/shared/src/interfaces/meeting-attachment.interface.ts (1)
MeetingAttachment(8-31)
🔇 Additional comments (14)
apps/lfx-one/src/server/routes/past-meetings.route.ts (1)
29-30: LGTM!The new route for past meeting attachments follows the established pattern and correctly delegates to the controller method.
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html (1)
238-250: LGTM!The attachment rendering correctly migrates to the new data model (uid, name) and constructs download URLs using the new API endpoint pattern.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html (1)
68-112: LGTM!The template correctly migrates to the new attachment data model (uid, name, link) and properly constructs API URLs for download and delete operations.
apps/lfx-one/src/server/controllers/past-meeting.controller.ts (1)
257-298: LGTM!The new
getPastMeetingAttachmentscontroller method follows the established pattern with proper validation, logging, and error handling.apps/lfx-one/src/server/services/microservice-proxy.service.ts (1)
90-136: LGTM!The new
proxyBinaryRequestmethod correctly handles binary responses and follows the established pattern for proxy methods, including proper error transformation and query parameter merging.apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (2)
226-230: LGTM!The attachment lookup correctly uses the new
uidandnameproperties from the migrated data model.
755-780: LGTM!The attachment save logic correctly:
- Filters for unsaved attachments with the
fileproperty- Uses the new
createFileAttachmentAPI- Handles successes and failures appropriately
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html (1)
269-279: LGTM!The attachment rendering correctly:
- Uses
attachment.uidfor tracking- Distinguishes between link and file types
- Constructs appropriate download URLs for file attachments
- Displays attachment metadata with proper tooltips
packages/shared/src/interfaces/meeting-attachment.interface.ts (3)
8-31: LGTM! Clean migration to V2 schema.The interface correctly adopts UID-based identifiers and introduces type discrimination for file vs. link attachments, which improves type safety and clarity.
56-73: LGTM! Upload state tracking properly implemented.The addition of the
file: Filefield anduploaded?: booleanflag enables proper file handling and upload state tracking, which aligns with the improved upload UX mentioned in the PR objectives.
75-104: LGTM! Comprehensive past meeting attachment interface.The new interface properly represents past meeting attachments with complete metadata. Note the field naming difference (
content_typevsmime_typeinMeetingAttachment), which likely reflects the different backend API schemas for current vs. past meetings.apps/lfx-one/src/app/shared/services/meeting.service.ts (2)
250-269: LGTM! Proper file attachment upload implementation.The method correctly handles file upload to the V2 API with proper observable patterns (
deferfor lazy execution), error handling, and appropriate data structure including thetype: 'file'discriminator.
478-491: LGTM! Clean helper function.The
readFileAsBase64helper properly encapsulates file reading logic with appropriate error handling, making thecreateFileAttachmentmethod cleaner and more maintainable.apps/lfx-one/src/server/routes/meetings.route.ts (1)
56-64: LGTM! Clean controller delegation with proper route ordering.The attachment routes are well-structured and properly delegate to controller methods. The route ordering is correct—the more specific
/metadataroute (line 60) is placed before the generic/:attachmentIdroute (line 62), preventing path matching conflicts.
| public readonly existingAttachments = input<MeetingAttachment[]>([]); | ||
| public readonly isEditMode = input<boolean>(false); | ||
| public readonly deletingAttachmentId = input<string | null>(null); | ||
| public readonly meetingId = input<string | null>(null); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify meetingId is always provided when needed.
The new meetingId input is required for file uploads but is nullable. Ensure that file uploads are only initiated when meetingId is available.
Run the following script to verify meetingId usage:
🏁 Script executed:
#!/bin/bash
# Check where MeetingResourcesSummaryComponent is used and how meetingId is passed
ast-grep --pattern $'<lfx-meeting-resources-summary
$$$
[meetingId]=$_
$$$
>'Length of output: 0
I'll investigate the meetingId usage more thoroughly by searching for the component selector and examining the component implementation.
🏁 Script executed:
#!/bin/bash
# Search for lfx-meeting-resources-summary component usage in templates
rg -n 'lfx-meeting-resources-summary' --type=html --type=ts -A 3 -B 1Length of output: 3906
🏁 Script executed:
#!/bin/bash
# Look at the component implementation to understand file upload logic
fd -e ts -e html -path '*/meeting-resources-summary/*' -type fLength of output: 168
🏁 Script executed:
#!/bin/bash
# Search for file upload handling in the component directory
rg -n 'upload\|file' 'apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/' -A 2 -B 2Length of output: 0
Let me examine the component files directly and check the template more carefully:
🏁 Script executed:
#!/bin/bash
# Get all files in the meeting-resources-summary directory
fd . 'apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/' -type fLength of output: 170
🏁 Script executed:
#!/bin/bash
# Read the component TypeScript file
cat 'apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts'Length of output: 10881
🏁 Script executed:
#!/bin/bash
# Read the component HTML template
cat 'apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html'Length of output: 9694
🏁 Script executed:
#!/bin/bash
# Check the full context of component usage in meeting-manage, especially around meetingId
rg -n 'meetingId' 'apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/' -B 3 -A 3Length of output: 18969
Pass [meetingId] from parent to child component and implement proper null checks before file uploads.
The component uses this.meetingId()! (with non-null assertion) in the file upload handler (onFileSelect) and in the download link template, but the parent component never passes [meetingId] to this child component. In create mode, meetingId is null until the meeting is created, so file upload attempts will fail. Additionally, the component's template constructs a download URL using meetingId() without null safety.
Fix:
- In
meeting-manage.component.html(lines 52 and 110), add[meetingId]="meetingId()"binding to bothlfx-meeting-resources-summaryuses - In
meeting-resources-summary.component.ts, replace the non-null assertion with a proper guard inonFileSelect():if (!this.meetingId()) { this.messageService.add({ severity: 'error', summary: 'Upload Error', detail: 'Meeting must be created before uploading files', life: 5000, }); return; }
- In the template, wrap the download link in a guard:
@if (meetingId()) { ... }
🤖 Prompt for AI Agents
In
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts
around line 29 and related templates, the component currently uses
this.meetingId()! without safety and the parent never passes meetingId, causing
uploads/downloads to fail in create mode; to fix, 1) update the parent
meeting-manage.component.html at the two component usages (around lines 52 and
110) to add the binding [meetingId]="meetingId()", 2) in
meeting-resources-summary.component.ts replace the non-null assertion in
onFileSelect() with a null-check that shows an error toast and returns early if
!this.meetingId(), and 3) update the component template to conditionally render
the download link only when meetingId() is truthy (wrap link with an if guard).
|
|
||
| // Start the upload | ||
| this.meetingService.uploadFileToStorage(file).subscribe({ | ||
| this.meetingService.createFileAttachment(this.meetingId()!, file).subscribe({ |
There was a problem hiding this comment.
Guard against null meetingId before file upload.
The non-null assertion operator (!) on this.meetingId() will throw if the value is null. Consider adding an explicit check before starting the upload.
Apply this diff to add proper null checking:
- this.meetingService.createFileAttachment(this.meetingId()!, file).subscribe({
+ const meetingId = this.meetingId();
+ if (!meetingId) {
+ this.pendingAttachments.update((current) =>
+ current.map((pa) => (pa.id === pendingAttachment.id ? { ...pa, uploading: false, uploadError: 'Meeting ID is required' } : pa))
+ );
+ return;
+ }
+ this.meetingService.createFileAttachment(meetingId, file).subscribe({Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts
around line 111, remove the non-null assertion on this.meetingId() and add an
explicit null check: capture const meetingId = this.meetingId(); if meetingId is
null/undefined, abort the upload (return early and optionally surface a
user-facing error/toast or disable the upload control); otherwise call
this.meetingService.createFileAttachment(meetingId, file).subscribe(...) so the
API is only invoked with a validated id.
Added binary request handling to ApiClientService and MicroserviceProxyService to properly download file attachments without JSON parsing. Updated all meeting attachment methods in MeetingService to use req.log instead of Logger helper for consistent structured logging. Changes: - Add binaryRequest() method to ApiClientService for Buffer responses - Add proxyBinaryRequest() method to MicroserviceProxyService - Update getMeetingAttachment() to use microservice proxy - Simplify custom headers logic in makeRequest() - Replace Logger calls with req.log in all attachment methods: - createMeetingAttachment() - getMeetingAttachment() - deleteMeetingAttachment() - getMeetingAttachmentMetadata() - getMeetingAttachments() - getPastMeetingAttachments() Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <andrest2455@gmail.com>
andrest50
force-pushed
the
andrest50/meeting-attachments
branch
from
5473052 to
e0ddfa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
111-119: MissinguploadSuccessevent binding in single-page mode.The
lfx-meeting-resources-summaryin EDIT MODE does not emit theuploadSuccessevent handler. When a file is uploaded in single-page edit mode, newly uploaded attachments won't be added to the attachments list immediately without this binding. For consistency with the stepper mode and to ensure proper UX in edit mode, add the missing event binding:<lfx-meeting-resources-summary [form]="form()" [meetingId]="meetingId()" [existingAttachments]="attachments()" [isEditMode]="isEditMode()" [deletingAttachmentId]="deletingAttachmentId()" (goToStep)="goToStep($event)" (deleteAttachment)="deleteAttachment($event)" + (uploadSuccess)="onAttachmentUploadSuccess($event)"> </lfx-meeting-resources-summary>apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
977-984: Validate file size to prevent DoS attacks.The base64-encoded file is decoded into a Buffer without size validation, allowing arbitrarily large files to be uploaded. This can lead to memory exhaustion and denial-of-service attacks.
Apply this diff to add file size validation:
// If file data is provided, include it (base64 encoded file from client) if (attachmentData.file) { const buffer = Buffer.from(attachmentData.file, 'base64'); + + // Validate file size (e.g., 10MB limit) + const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB + if (buffer.length > MAX_FILE_SIZE) { + const validationError = ServiceValidationError.forField( + 'file', + `File size exceeds maximum of ${MAX_FILE_SIZE / 1024 / 1024}MB`, + { + operation: 'create_meeting_attachment', + service: 'meeting_controller', + path: req.path, + } + ); + Logger.error(req, 'create_meeting_attachment', startTime, validationError); + return next(validationError); + } + formData.append('file', buffer, { filename: attachmentData.name, contentType: attachmentData.file_content_type || 'application/octet-stream', }); }
🧹 Nitpick comments (6)
apps/lfx-one/src/server/services/api-client.service.ts (2)
121-143: Consider adding an Accept header for binary requests.Unlike
makeRequestwhich setsAccept: 'application/json', the binary request path doesn't set an Accept header. While this may be intentional, adding an explicit Accept header could improve clarity:private async makeBinaryRequest(method: string, url: string, bearerToken?: string, customHeaders?: Record<string, string>): Promise<ApiResponse<Buffer>> { const headers: Record<string, string> = { ['User-Agent']: 'LFX-PCC-Server/1.0', + Accept: '*/*', // or 'application/octet-stream' for binary content };
207-268: Significant code duplication with executeRequest.The
executeBinaryRequestmethod duplicates approximately 90% of the logic fromexecuteRequest(lines 145-205). The only meaningful difference is the response body handling (ArrayBuffer→Buffer vs text→JSON).Consider extracting the shared error handling and request execution logic:
private async executeRequest<T>(url: string, requestInit: RequestInit): Promise<ApiResponse<T>> { return this.executeRequestWithParser(url, requestInit, async (response) => { const text = await response.text(); return text ? JSON.parse(text) : null; }, 'api_client_request'); } private async executeBinaryRequest(url: string, requestInit: RequestInit): Promise<ApiResponse<Buffer>> { return this.executeRequestWithParser(url, requestInit, async (response) => { const arrayBuffer = await response.arrayBuffer(); return Buffer.from(arrayBuffer); }, 'api_client_binary_request'); } private async executeRequestWithParser<T>( url: string, requestInit: RequestInit, parser: (response: Response) => Promise<T>, operation: string ): Promise<ApiResponse<T>> { try { const response = await fetch(url, requestInit); if (!response.ok) { // ... shared error handling ... } const data = await parser(response); return { data, status: response.status, statusText: response.statusText, headers: Object.fromEntries(response.headers.entries()), }; } catch (error: unknown) { // ... shared error handling ... } }This would improve maintainability and ensure consistent error handling across both request types.
apps/lfx-one/src/server/services/meeting.service.ts (1)
854-854: Consider stronger typing for attachmentData parameter.The parameter is typed as
any, but the comment on line 865 indicates it should be a FormData object. Consider using a more specific type or at minimum adding a type assertion/check for better type safety.Apply this diff to improve type clarity:
- public async createMeetingAttachment(req: Request, meetingUid: string, attachmentData: any): Promise<any> { + public async createMeetingAttachment(req: Request, meetingUid: string, attachmentData: unknown): Promise<any> {apps/lfx-one/src/server/controllers/meeting.controller.ts (3)
952-969: Add validation for type field values.The current validation only checks that
typeexists but doesn't verify it's either 'file' or 'link'. This allows invalid values to be proxied to the downstream API, resulting in unclear error messages.Apply this diff to validate the type field:
// Validate attachment data if (!attachmentData.type || !attachmentData.name) { const validationError = ServiceValidationError.fromFieldErrors( { type: !attachmentData.type ? 'Type is required' : [], name: !attachmentData.name ? 'Name is required' : [], }, 'Attachment data validation failed', { operation: 'create_meeting_attachment', service: 'meeting_controller', path: req.path, } ); Logger.error(req, 'create_meeting_attachment', startTime, validationError); return next(validationError); } + + // Validate type value + if (attachmentData.type !== 'file' && attachmentData.type !== 'link') { + const validationError = ServiceValidationError.forField( + 'type', + 'Type must be either "file" or "link"', + { + operation: 'create_meeting_attachment', + service: 'meeting_controller', + path: req.path, + } + ); + Logger.error(req, 'create_meeting_attachment', startTime, validationError); + return next(validationError); + }
971-989: Add type-specific field validation.The code doesn't validate that
type: 'file'attachments have afilefield or thattype: 'link'attachments have alinkfield. This allows incomplete payloads to be proxied downstream, resulting in API errors instead of clear local validation errors.Apply this diff after the type value validation:
+ // Validate type-specific fields + if (attachmentData.type === 'file' && !attachmentData.file) { + const validationError = ServiceValidationError.forField( + 'file', + 'File data is required for file-type attachments', + { + operation: 'create_meeting_attachment', + service: 'meeting_controller', + path: req.path, + } + ); + Logger.error(req, 'create_meeting_attachment', startTime, validationError); + return next(validationError); + } + + if (attachmentData.type === 'link' && !attachmentData.link) { + const validationError = ServiceValidationError.forField( + 'link', + 'Link URL is required for link-type attachments', + { + operation: 'create_meeting_attachment', + service: 'meeting_controller', + path: req.path, + } + ); + Logger.error(req, 'create_meeting_attachment', startTime, validationError); + return next(validationError); + } + // Create FormData for multipart/form-data request
1043-1061: Consider parallel metadata fetch or fix comment.The comment on line 1047 states "fetch in parallel with file data," but the implementation is sequential: the file data is awaited on line 1045 before the metadata fetch begins on line 1052.
Either update the comment to reflect the sequential nature, or refactor to actually fetch in parallel:
Option 1: Fix the comment
- // Get metadata to set proper filename (fetch in parallel with file data, but don't fail if metadata fails) + // Get metadata to set proper filename (best-effort, don't fail if metadata fetch fails)Option 2: Make it truly parallel
- // Get attachment file data via LFX V2 API (downloads file) - // The LFX V2 API returns the file with proper Content-Type and Content-Disposition headers - const attachmentData = await this.meetingService.getMeetingAttachment(req, uid, attachmentId); - - // Get metadata to set proper filename (fetch in parallel with file data, but don't fail if metadata fails) + // Fetch file data and metadata in parallel + const [attachmentData, metadataResult] = await Promise.allSettled([ + this.meetingService.getMeetingAttachment(req, uid, attachmentId), + this.meetingService.getMeetingAttachmentMetadata(req, uid, attachmentId) + ]); + + if (attachmentData.status === 'rejected') { + throw attachmentData.reason; + } + + const buffer = attachmentData.value; let filename = 'download'; let contentType = 'application/octet-stream'; - try { - const metadata = await this.meetingService.getMeetingAttachmentMetadata(req, uid, attachmentId); + if (metadataResult.status === 'fulfilled') { + const metadata = metadataResult.value; filename = metadata.name || filename; contentType = metadata.mime_type || metadata.content_type || contentType; - } catch (metadataError) { + } else { Logger.warning(req, 'get_meeting_attachment_metadata', 'Failed to fetch metadata, using defaults', { meeting_uid: uid, attachment_id: attachmentId, - error: metadataError instanceof Error ? metadataError.message : metadataError, + error: metadataResult.reason instanceof Error ? metadataResult.reason.message : metadataResult.reason, }); }Then update the variable name in subsequent uses from
attachmentDatatobuffer.
📜 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-manage/meeting-manage.component.html(2 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(2 hunks)apps/lfx-one/src/server/controllers/meeting.controller.ts(1 hunks)apps/lfx-one/src/server/services/api-client.service.ts(3 hunks)apps/lfx-one/src/server/services/meeting.service.ts(1 hunks)apps/lfx-one/src/server/services/microservice-proxy.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-one/src/server/services/microservice-proxy.service.ts
- apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 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-manage/meeting-manage.component.html
🧬 Code graph analysis (3)
apps/lfx-one/src/server/services/api-client.service.ts (2)
packages/shared/src/interfaces/api.interface.ts (1)
ApiResponse(21-30)apps/lfx-one/src/server/errors/index.ts (1)
MicroserviceError(6-6)
apps/lfx-one/src/server/controllers/meeting.controller.ts (1)
apps/lfx-one/src/server/errors/index.ts (1)
ServiceValidationError(7-7)
apps/lfx-one/src/server/services/meeting.service.ts (1)
packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
🔇 Additional comments (12)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
54-54: ✓ Correctly pass meetingId to resources component for attachment operations.Both instances of
lfx-meeting-resources-summarynow receive themeetingId()binding, enabling the resources component to perform create/read/delete attachment operations against the V2 API per the PR migration objectives.Also applies to: 113-113
apps/lfx-one/src/server/services/api-client.service.ts (2)
37-55: Verify if request body support is needed for binary requests.The
binaryRequestmethod signature includes POST, PUT, and PATCH methods but doesn't accept adataparameter. This prevents sending request bodies with these HTTP methods, which is inconsistent with the regularrequest()method.Is this limitation intentional? If binary responses might be needed for POST/PUT/PATCH requests with payloads (e.g., uploading a file and receiving binary confirmation), consider adding a
dataparameter:public binaryRequest( type: 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE', url: string, bearerToken?: string, query?: Record<string, any>, + data?: any, customHeaders?: Record<string, string> ): Promise<ApiResponse<Buffer>> { const fullUrl = this.getFullUrl(url, query); - return this.makeBinaryRequest(type, fullUrl, bearerToken, customHeaders); + return this.makeBinaryRequest(type, fullUrl, bearerToken, data, customHeaders); }
58-116: FormData handling looks solid.The FormData detection using duck typing and conditional header handling is appropriate for the Node.js
form-datapackage. The buffer conversion with Content-Length calculation correctly addresses Node.js fetch limitations.Note: The error handling in the catch block (lines 104-111) was already flagged in a previous review regarding silent error swallowing.
apps/lfx-one/src/server/services/meeting.service.ts (5)
906-943: LGTM!The binary file download implementation correctly uses
proxyBinaryRequestto fetch the file as a Buffer and includes appropriate logging of the file size.
951-985: LGTM!The delete operation follows the established pattern with proper error handling and logging.
987-1027: LGTM!The metadata fetch implementation is straightforward and follows the service's established patterns.
1035-1083: LGTM!The query-based attachment fetching follows the same pattern as other resource queries in this service (e.g., getMeetingRegistrants, getPastMeetingParticipants) with proper error handling and logging.
1091-1139: LGTM!The past meeting attachments fetch mirrors the getMeetingAttachments implementation with the appropriate resource type and tags.
apps/lfx-one/src/server/controllers/meeting.controller.ts (4)
1093-1142: LGTM!The delete attachment endpoint follows the controller's established patterns with proper validation and error handling.
1144-1193: LGTM!The metadata endpoint correctly validates parameters and delegates to the service layer.
1198-1233: LGTM!The get attachments list endpoint is implemented correctly with proper validation.
1238-1273: LGTM!The past meeting attachments endpoint mirrors the meeting attachments implementation with appropriate validation.
Ticket
LFXV2-711
Summary
Migrate meeting attachment management from Supabase to LFX V2 API, implementing complete backend endpoints and updating frontend components to support the new API integration.
Changes
Backend
Frontend
uploadedfield toPendingAttachmentinterface for upload state trackingGenerated with Claude Code