feat(api): add meeting update functionality #57
Conversation
- Add updateMeeting method to MeetingService with ETag support for safe concurrent updates - Add updateMeeting controller method with comprehensive validation - Update routes to use controller pattern instead of direct database calls - Support editType parameter for recurring meetings (single/future) - Maintain automatic organizer management - Follow established microservice proxy patterns Resolves LFXV2-378 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
|
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. WalkthroughMoves PUT /meetings/:id update logic into a new MeetingController.updateMeeting and adds MeetingService.updateMeeting with ETag-based concurrency and username enrichment; routes now delegate to the controller. Frontend form population enables recording-related controls when editing. Minor import reordering and a CLAUDE.md doc addition. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Client/UI
participant R as Router
participant C as MeetingController
participant S as MeetingService
participant E as ETagService
participant API as Meetings API
UI->>R: PUT /meetings/:id (body, ?editType)
R->>C: updateMeeting(req, res)
C->>C: Validate id, title, start_time, project_uid, duration, timezone, editType
alt invalid
C-->>UI: 400 Bad Request
else valid
C->>S: updateMeeting(req, id, payload, editType)
S->>E: fetchWithETag(path)
E->>API: GET /meetings/:id
API-->>E: 200 Current meeting + ETag
E-->>S: meeting, etag
S->>S: merge payload (dedupe organizers + username)
S->>E: updateWithETag(path[?editType], payload, etag)
E->>API: PUT /meetings/:id (If-Match: etag)
API-->>E: 200 Updated meeting
E-->>S: updated meeting
S-->>C: updated meeting
C-->>UI: 200 Updated meeting
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR implements meeting update functionality for the LFX PCC v3 application with ETag-based concurrency control to prevent lost updates when multiple users edit the same meeting.
- Adds comprehensive meeting update capabilities with proper validation and error handling
- Implements ETag concurrency control to prevent race conditions during updates
- Migrates PUT endpoint from direct database calls to controller pattern for consistency
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| meeting.service.ts | Added updateMeeting method with ETag support and organizer management |
| etag.service.ts | Fixed import order for consistency |
| meetings.ts | Replaced inline PUT handler with controller pattern |
| meeting.controller.ts | Added comprehensive updateMeeting handler with validation |
| meeting-manage.component.ts | Enabled form controls for meeting recording options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...x-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (5)
404-408: Comment and behavior mismatch: controls are always enabled in edit mode, but the comment implies a conditional on recording_enabledThe code unconditionally enables transcript/youtube/zoom controls during edit, which contradicts the comment and may bypass prior UX rules tying transcript/YouTube to recording toggles. If the intent is to always allow editing these fields in edit mode, fix the comment. If not, gate by recording_enabled.
Minimal fix (comment only):
- // If recording_enabled is true, enable controls for transcript_enabled and youtube_upload_enabled + // In edit mode, enable controls so their values are included in the update payloadOptional: enforce prior dependency on recording_enabled (keeps zoom AI independent):
- this.form().get('transcript_enabled')?.enable(); - this.form().get('youtube_upload_enabled')?.enable(); - this.form().get('zoom_ai_enabled')?.enable(); + if (meeting.recording_enabled) { + this.form().get('transcript_enabled')?.enable(); + this.form().get('youtube_upload_enabled')?.enable(); + } else { + this.form().get('transcript_enabled')?.disable(); + this.form().get('youtube_upload_enabled')?.disable(); + } + this.form().get('zoom_ai_enabled')?.enable();
226-234: Error toast says “create” in edit modeWhen project is missing, the error message always says “create a meeting”. Make it mode-aware.
- detail: 'Project information is required to create a meeting.', + detail: `Project information is required to ${this.isEditMode() ? 'update' : 'create'} a meeting.`,
272-275: Hardcoded editType='single' prevents “future” edits for recurring meetingsThe backend supports editType single/future; UI hardcodes single. At minimum, respect a query param to allow callers to choose. Ideally, prompt users when updating a recurring meeting.
Minimal wiring to honor ?editType=future:
- const operation = this.isEditMode() - ? this.meetingService.updateMeeting(this.meetingId()!, baseMeetingData as UpdateMeetingRequest, 'single') + const selectedEditType = + (this.route.snapshot.queryParamMap.get('editType') as 'single' | 'future') ?? 'single'; + const operation = this.isEditMode() + ? this.meetingService.updateMeeting(this.meetingId()!, baseMeetingData as UpdateMeetingRequest, selectedEditType) : this.meetingService.createMeeting(baseMeetingData as CreateMeetingRequest);Follow-up: do you want a confirmation dialog when editing a recurring meeting (“This event” vs “This and future events”)? I can draft it.
481-519: Form defaults: disabled feature controls are fine; consider centralizing the “enable on edit” ruleRight now the enablement logic lives in populateFormWithMeetingData; keep it single-sourced to avoid divergent states if you later toggle these controls elsewhere. No code change required now, just a heads-up for maintainability.
71-75: Minor typing nit: avoid ad-hoc string literal unions for modesYou’re using 'create' | 'edit' inline. Consider a shared enum/type to avoid repetition across components/services.
Proposed shared type (in shared/types.ts):
export type MeetingFormMode = 'create' | 'edit';Then:
- public mode = signal<'create' | 'edit'>('create'); + public mode = signal<MeetingFormMode>('create');apps/lfx-pcc/src/server/controllers/meeting.controller.ts (3)
143-154: Normalize editType to lowercase and pass the normalized value throughoutProtects against inputs like “Single”/“FUTURE” and avoids surprising 400s.
- const { editType } = req.query; + const { editType } = req.query; + const normalizedEditType = + typeof editType === 'string' ? editType.toLowerCase() : undefined; ... - if (editType && !['single', 'future'].includes(editType as string)) { + if (normalizedEditType && !['single', 'future'].includes(normalizedEditType)) { Logger.error(req, 'update_meeting', startTime, new Error('Invalid edit type for meeting update'), { - provided_edit_type: editType, + provided_edit_type: editType, }); Responder.badRequest(res, 'Edit type must be "single" or "future"', { code: 'INVALID_EDIT_TYPE', }); return; } - const meeting = await this.meetingService.updateMeeting(req, id, meetingData, editType as 'single' | 'future'); + const meeting = await this.meetingService.updateMeeting( + req, + id, + meetingData, + normalizedEditType as 'single' | 'future' | undefined + ); Logger.success(req, 'update_meeting', startTime, { meeting_id: id, project_uid: meeting.project_uid, title: meeting.title, - edit_type: editType || 'single', + edit_type: normalizedEditType || 'single', });Also applies to: 195-207, 207-217
165-181: Confirm strict “required fields” policy for updatesRequiring project_uid, start_time, duration, timezone, and title on updates may be stricter than necessary if the microservice supports partial updates. If the upstream demands full objects, keep as-is; otherwise consider allowing partial payloads and merging with the fetched record (service already fetches the current state).
If partial updates are acceptable, we can relax validation to “at least one updatable field present” and merge server-side. I can draft that.
137-140: Minor type hygiene: avoid repeated inline unions for editTypeConsider introducing a shared EditType in @lfx-pcc/shared/interfaces to avoid repeating 'single' | 'future' in multiple places (controller, service, UI), which reduces drift.
Proposed addition (shared):
export type EditType = 'single' | 'future';Then import and use EditType in signatures.
apps/lfx-pcc/src/server/services/meeting.service.ts (4)
124-127: Build the path with URLSearchParams to avoid manual string concatSafer and clearer, even if current values are simple.
- let path = `/meetings/${meetingId}`; - if (editType) { - path += `?editType=${editType}`; - } + const qs = editType ? `?${new URLSearchParams({ editType }).toString()}` : ''; + const path = `/meetings/${meetingId}${qs}`;
104-144: Add unit tests for ETag update flow and organizer mergeGiven the concurrency and merge logic, targeted tests will prevent regressions:
- Ensures If-Match header is sent on update.
- Verifies organizers are merged and de-duplicated, and no nulls leak.
- Verifies editType is forwarded in the request path.
I can scaffold Jest tests for MeetingService.updateMeeting with a mocked MicroserviceProxyService/ETagService if helpful.
107-110: Type capture: fetchWithETag result is used for more than etagNit: consider naming to reflect usage, e.g., const { etag, data: existing } = … to improve readability when later merging organizers.
107-140: Shared type for editType in signatureSame nit as in the controller: prefer a shared EditType alias/enum to avoid repeating unions across layers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(1 hunks)apps/lfx-pcc/src/server/controllers/meeting.controller.ts(2 hunks)apps/lfx-pcc/src/server/routes/meetings.ts(1 hunks)apps/lfx-pcc/src/server/services/etag.service.ts(1 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript interfaces instead of union types for better maintainability
Files:
apps/lfx-pcc/src/server/services/etag.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tsapps/lfx-pcc/src/server/routes/meetings.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
License headers are required on all source files
Files:
apps/lfx-pcc/src/server/services/etag.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tsapps/lfx-pcc/src/server/routes/meetings.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/server/services/etag.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tsapps/lfx-pcc/src/server/routes/meetings.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/services/meeting.service.ts (3)
packages/shared/src/interfaces/meeting.interface.ts (2)
UpdateMeetingRequest(203-226)Meeting(112-176)apps/lfx-pcc/src/server/utils/auth-helper.ts (1)
getUsernameFromAuth(10-50)apps/lfx-pcc/src/server/helpers/logger.ts (1)
Logger(11-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (2)
apps/lfx-pcc/src/server/services/etag.service.ts (1)
5-5: LGTM — import order only; no functional impactThe reorder does not change runtime. Headers handling and error typing remain consistent.
apps/lfx-pcc/src/server/routes/meetings.ts (1)
171-172: Route refactor to controller pattern — good consolidationDelegating PUT /meetings/:id to MeetingController aligns with the existing GET/POST pattern and centralizes validation and logging.
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
- Filter out null/undefined values from existing organizers - Use Set to prevent duplicate organizers - Only add current user if username exists - Maintain all valid existing organizers Updates LFXV2-378 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
404-410: Enable dependent controls only when recording is on — good fix.This addresses the prior feedback to gate transcript/YouTube/Zoom AI toggles behind recording_enabled.
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
114-127: Organizer merge drops client-provided organizers — risk of data loss.The update payload ignores
meetingData.organizersand only carries forward existing organizers plus the current user. This will overwrite client-supplied organizer updates and can’t remove/replace organizers intentionally. Merge existing + incoming + current user, de-dup, and filter falsy.Apply this diff:
- // Create organizers array ensuring no duplicates or null values - const existingOrganizers = data.organizers || []; - const organizersSet = new Set(existingOrganizers.filter((organizer) => organizer != null)); - - // Add current user as organizer if username exists and not already included - if (username) { - organizersSet.add(username); - } - - // Include organizers in the update payload - const updatePayload = { - ...meetingData, - organizers: Array.from(organizersSet), - }; + // Merge organizers: existing + incoming + current user; de-dupe and filter + const existingOrganizers = Array.isArray(data.organizers) ? data.organizers : []; + const incomingOrganizers = Array.isArray(meetingData.organizers) ? meetingData.organizers : []; + const mergedOrganizers = Array.from( + new Set([ + ...existingOrganizers, + ...incomingOrganizers, + ...(username ? [username] : []), + ]) + ).filter(Boolean) as string[]; + + // Include organizers only if we have any after merge; keep other fields as-is + const updatePayload: UpdateMeetingRequest = { + ...meetingData, + ...(mergedOrganizers.length ? { organizers: mergedOrganizers } : {}), + };
🧹 Nitpick comments (2)
apps/lfx-pcc/src/server/services/meeting.service.ts (2)
140-150: Log accuracy: prefer listing organizers and omit defaulted edit_type in logs.Current logging always sets
edit_typeto'single'even when not provided, and logs a singleorganizer. Prefer reflecting actual inputs and the merged organizers for traceability.req.log.info( { operation: 'update_meeting', meeting_id: meetingId, project_uid: updatedMeeting.project_uid, title: updatedMeeting.title, - edit_type: editType || 'single', - organizer: username || 'none', + ...(editType ? { edit_type: editType } : {}), + organizers: updatePayload.organizers, }, 'Meeting updated successfully' );
133-137: Optional Refactor: Safer Query Construction inmeeting.service.ts– In apps/lfx-pcc/src/server/services/meeting.service.ts (around lines 133–137), replace the manual string-concatenation of the path with an encoded ID and
URLSearchParams.
– Confirmed that the controller unwrapsreq.query.editType(camelCase), notedit_type, so the param name is correct.Proposed diff:
- let path = `/meetings/${meetingId}`; - if (editType) { - path += `?editType=${editType}`; - } + // Step 2: Update meeting with ETag, safely encoding the ID and building query params + let path = `/meetings/${encodeURIComponent(meetingId)}`; + const qp = new URLSearchParams(); + if (editType) qp.set('editType', editType); + const qs = qp.toString(); + if (qs) path += `?${qs}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CLAUDE.md(1 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(2 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript interfaces instead of union types for better maintainability
Files:
apps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
License headers are required on all source files
Files:
apps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/services/meeting.service.ts (3)
packages/shared/src/interfaces/meeting.interface.ts (2)
UpdateMeetingRequest(203-226)Meeting(112-176)apps/lfx-pcc/src/server/utils/auth-helper.ts (1)
getUsernameFromAuth(10-50)apps/lfx-pcc/src/server/helpers/logger.ts (2)
Logger(11-135)etag(92-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (3)
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
231-231: Clearer, context-aware error copy. LGTM.Dynamic wording for create vs update improves UX and aligns with edit flow.
apps/lfx-pcc/src/server/services/meeting.service.ts (2)
4-4: Imports look correct and consistent with usage.No issues with added types and auth helper import.
Also applies to: 8-8
108-113: ETag fetch + user enrichment — implementation aligns with concurrency requirements.Fetch with ETag and enriching with the current user is the right approach for lost-update protection and organizer preservation.
Summary
Implements the ability to update meetings in the LFX PCC v3 application with proper ETag-based concurrency control to prevent lost updates.
Changes
updateMeetingmethod with ETag support for safe concurrent updatesKey Features
editTypeparameter ('single' or 'future')Technical Implementation
Files Changed
apps/lfx-pcc/src/server/controllers/meeting.controller.ts- Added updateMeeting methodapps/lfx-pcc/src/server/routes/meetings.ts- Updated to use controller patternapps/lfx-pcc/src/server/services/meeting.service.ts- Added updateMeeting with ETag supportapps/lfx-pcc/src/server/services/etag.service.ts- Fixed header usage for updatesapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts- Frontend update supportTesting
Related JIRA Ticket
LFXV2-378
Generated with Claude Code