feat(meetings): replace link extraction with attachment api (LFXV2-743) #154
Conversation
Replace the existing implementation where links are extracted from meeting descriptions with the attachment API that supports link-type attachments. Changes: - Remove extractUrlsWithDomains usage and importantLinks signals from meeting-join and meeting-card components - Update templates to display both file and link attachments uniformly - Add saveLinkAttachments() method to create link attachments from important_links FormArray - Add populateExistingLinks() to load existing links when editing - Add deleteLinkAttachment() handler for tracking link deletions - Update saveLinkAttachments() to only create new links (without uid) - Fix icon alignment with consistent w-8 width for both file and link icons Benefits: - Eliminates duplicate link display from extracted URLs and attachments - Provides consistent CRUD operations for all meeting resources - Links persist as structured data with proper tracking Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
|
Caution Review failedThe pull request is closed. 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. WalkthroughDifferentiates link vs file attachments in templates and download behavior, removes extracted importantLinks, introduces staged attachment deletions with undo and link-attachment handling, updates tooltip usage, consolidates binary API client requests, and adds a download query flag on the attachment endpoint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ResourcesSummary
participant MeetingManage
participant Backend
Note over ResourcesSummary,MeetingManage `#E6F0FF`: Client-side staged deletion with undo
User->>ResourcesSummary: Click "Delete" on attachment
ResourcesSummary->>MeetingManage: emit deleteLinkAttachment(id) or mark pending locally
MeetingManage->>MeetingManage: add id to pendingAttachmentDeletions
ResourcesSummary->>User: Show "Marked for deletion" state (undo button)
User->>ResourcesSummary: Click "Undo"
ResourcesSummary->>MeetingManage: emit undoDeleteAttachment(id)
MeetingManage->>MeetingManage: remove id from pendingAttachmentDeletions
ResourcesSummary->>User: Restore normal state
Note over MeetingManage,Backend `#FFF4E6`: Save commits staged operations (delete/upload/link)
User->>MeetingManage: Save meeting
MeetingManage->>Backend: deletePendingAttachments()
Backend-->>MeetingManage: deletion results
MeetingManage->>Backend: savePendingAttachments() (uploads)
Backend-->>MeetingManage: upload results
MeetingManage->>Backend: saveLinkAttachments()
Backend-->>MeetingManage: link creation results
MeetingManage->>MeetingManage: handleAttachmentOperationsResults()
MeetingManage->>User: Aggregate success/warning/error messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Comment |
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the client-side URL extraction from meeting descriptions with a proper attachment API that supports link-type attachments. The change provides a cleaner architecture for managing meeting resources by treating links as first-class attachments with full CRUD operations.
Key Changes:
- Removed client-side URL extraction logic (
extractUrlsWithDomains) from meeting components - Implemented link attachment creation via
createAttachmentFromUrlAPI - Added pending deletion tracking for attachments with undo functionality
- Refactored API client service to consolidate binary and non-binary request handling
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
api-client.service.ts |
Consolidated executeRequest and executeBinaryRequest into a single method with a binary flag option |
meeting.controller.ts |
Added query parameter support for inline vs download content disposition |
meeting-card.component.ts |
Removed importantLinks signal and URL extraction logic |
meeting-card.component.html |
Unified attachment display for both file and link types |
meeting-resources-summary.component.ts |
Added link deletion tracking and removed immediate file upload |
meeting-resources-summary.component.html |
Enhanced UI with pending deletion state visualization and undo functionality |
meeting-manage.component.ts |
Implemented comprehensive attachment operations (deletions, uploads, links) with tracking |
meeting-manage.component.html |
Connected pending deletion inputs/outputs to resource summary component |
project.component.html |
Replaced native title attribute with PrimeNG pTooltip directive |
upcoming-committee-meeting.component.html |
Replaced native title attribute with PrimeNG pTooltip directive |
meeting-join.component.ts |
Removed importantLinks signal and URL extraction |
meeting-join.component.html |
Updated to display link attachments with consistent icon styling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...oject/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html
Show resolved
Hide resolved
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html
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: 2
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.html (1)
52-63: MissingdeleteLinkAttachmentbinding in edit mode breaks link deletionIn create (stepper) mode,
lfx-meeting-resources-summarywires:
[pendingAttachmentDeletions]="pendingAttachmentDeletions()"(deleteAttachment)="deleteAttachment($event)"(undoDeleteAttachment)="undoDeleteAttachment($event)"(deleteLinkAttachment)="deleteLinkAttachment($event)"In edit mode, the same component only wires
pendingAttachmentDeletions,deleteAttachment, andundoDeleteAttachment, but omits(deleteLinkAttachment).Since
MeetingResourcesSummaryComponent.removeLink()emitsdeleteLinkAttachmentwhen a link has auid, those events will be dropped in edit mode and existing link attachments won’t be marked/deleted on save.Recommend adding the missing binding to the edit-mode usage:
- <lfx-meeting-resources-summary + <lfx-meeting-resources-summary [form]="form()" [meetingId]="meetingId()" [existingAttachments]="attachments()" [isEditMode]="isEditMode()" [deletingAttachmentId]="deletingAttachmentId()" [pendingAttachmentDeletions]="pendingAttachmentDeletions()" (goToStep)="goToStep($event)" (deleteAttachment)="deleteAttachment($event)" - (undoDeleteAttachment)="undoDeleteAttachment($event)"> + (undoDeleteAttachment)="undoDeleteAttachment($event)" + (deleteLinkAttachment)="deleteLinkAttachment($event)">Also applies to: 114-124
🧹 Nitpick comments (5)
apps/lfx-one/src/server/services/api-client.service.ts (2)
54-76: Binary request header construction and timeout wiring look good; minor duplication onlyThe binaryRequest flow correctly:
- Builds a fresh headers map with User-Agent, optional Authorization, and merges customHeaders.
- Uses the shared timeout via
AbortSignal.timeout(this.config.timeout).- Delegates to
executeRequest<Buffer>(..., { binary: true }), which keeps the binary handling centralized.Only minor notes:
- Header construction (User-Agent + Authorization + customHeaders) now lives in both
binaryRequestandmakeRequest. If this starts to evolve (e.g., extra common headers, tracing IDs), consider extracting a small helper to avoid divergence over time.- This path intentionally doesn’t set
Accept, which makes sense for generic binary responses; if you ever need a specificAcceptfor some endpoints, wiring that viacustomHeadersshould be straightforward.
142-211: UnifiedexecuteRequestfor JSON and binary is solid; consider light future‑proofingThe refactored
executeRequestcleanly:
- Handles both JSON and binary via the
options.binaryflag.- Returns
ApiResponse<Buffer>correctly when invoked frombinaryRequest.- Differentiates operations in error metadata (
api_client_binary_request,api_client_binary_timeout,api_client_binary_network_errorvs non-binary), which should help observability.- Avoids leaking unreadable error bodies by only setting
errorBodywhen JSON parsing succeeds.Two small, non-blocking suggestions:
- To make the private API a bit more robust against future misuse (e.g., someone passing
undefinedexplicitly), you could allowoptionsto be optional and normalize it at the top:- private async executeRequest<T>(url: string, requestInit: RequestInit, options: { binary: boolean } = { binary: false }): Promise<ApiResponse<T>> { + private async executeRequest<T>( + url: string, + requestInit: RequestInit, + options?: { binary?: boolean } + ): Promise<ApiResponse<T>> { + const isBinary = options?.binary === true;…and then reference
isBinaryinstead ofoptions.binaryin the rest of the method.
- Since operation strings are now more granular, double‑check any dashboards/alerts that rely on the previous operation names to ensure they still capture binary failures as expected.
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html (1)
187-197: Resources attachment rendering aligns with new link/file modelThe conditional
hrefand icon logic forattachment.type === 'link'vs files is correct and matches the join page, so links won’t be double-counted and files still route through the API endpoint.One minor UX refinement to consider: for link-type attachments, the tooltip currently always shows a file size (often
0 B). You might want to omit the size or show link-specific info instead whenattachment.type === 'link'.apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html (1)
69-151: Re-check gating of view/download buttons onattachment.linkThe pending-deletion styling and undo/delete controls look solid, but this condition is potentially fragile:
@if (attachment.link && !isPendingDeletion(attachment.uid)) { <!-- view + download links to /api/meetings/... --> }Since the actual
hrefattributes always use the/api/meetings/{{meetingId}}/attachments/{{uid}}URL, tying the visibility of these actions toattachment.linkmay hide view/download for any attachments wherelinkis unset (for example, plain file attachments).If
attachment.linkis not guaranteed to be truthy for every entry inexistingAttachments(), consider relaxing this to:@if (!isPendingDeletion(attachment.uid)) { ... }or gating on
meetingId()instead.Please confirm how
existingAttachmentsis populated and whetherattachment.linkis always set there, to avoid unintentionally hiding file actions.apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
359-365: Consider refining thehasPendingLinkscheck.The current check
importantLinksArray.length > 0counts all links in the FormArray, including existing links with auid. While this doesn't cause functional issues (sincesaveLinkAttachmentsfilters for new links without auid), it may trigger unnecessary processing when only existing links are present.Consider aligning the check with the filter logic in
saveLinkAttachments:-const hasPendingLinks = importantLinksArray.length > 0; +const hasPendingLinks = importantLinksArray.value.some((link: any) => link.title && link.url && !link.uid);
📜 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 (12)
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html(1 hunks)apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.ts(1 hunks)apps/lfx-one/src/app/modules/project/committees/components/upcoming-committee-meeting/upcoming-committee-meeting.component.html(1 hunks)apps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.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(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts(6 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/server/controllers/meeting.controller.ts(2 hunks)apps/lfx-one/src/server/services/api-client.service.ts(5 hunks)
🧰 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.htmlapps/lfx-one/src/app/modules/project/committees/components/upcoming-committee-meeting/upcoming-committee-meeting.component.htmlapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.htmlapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.tsapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.html
🧬 Code graph analysis (3)
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 (1)
PendingAttachment(56-73)
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)
apps/lfx-one/src/server/services/api-client.service.ts (1)
packages/shared/src/interfaces/api.interface.ts (1)
ApiResponse(21-30)
🔇 Additional comments (15)
apps/lfx-one/src/server/controllers/meeting.controller.ts (3)
1010-1011: Clear documentation of the new query parameter.The comment accurately describes the download parameter behavior.
1079-1079: Sound disposition logic.The ternary expression correctly defaults to 'inline' when download is not explicitly 'true', which is appropriate for file viewing behavior.
1073-1080: Correct implementation of Content-Disposition with RFC 5987 encoding.The header construction properly uses the disposition value and correctly encodes the filename for international character support.
apps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.html (1)
186-186: Tooltip migration on meeting/committee links looks goodUsing
pTooltipon the truncated anchors keeps hover text while aligning with the rest of the dashboard’s tooltip usage. No issues spotted.Also applies to: 245-245
apps/lfx-one/src/app/modules/project/committees/components/upcoming-committee-meeting/upcoming-committee-meeting.component.html (1)
7-12: Consistent tooltip behavior for upcoming committee meetingBinding
pTooltipto the same title fallback as the link text preserves UX while standardizing tooltip usage. Looks correct.apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html (1)
239-253: Link vs file attachment handling is consistent and correct
hrefand icon selection based onattachment.type === 'link'align with the shared meeting card behavior and correctly route files through the API download URL. This fulfills the unified attachment/link handling objective on the join page.apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts (1)
538-541: Total resources count correctly simplified to attachments only
initTotalResourcesCount()now derives fromthis.attachments().length, dropping the previous dependency on extracted “important links”. This matches the new model where links are first-class attachments and avoids double-counting.apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.ts (1)
29-30: New pending-deletion and link-deletion wiring is coherent
pendingAttachmentDeletions+isPendingDeletion()cleanly externalize which attachment UIDs are marked for deletion and drive the edit-mode styling.undoDeleteAttachmentanddeleteLinkAttachmentoutputs give the parent just enough hooks to manage deferred file deletions and link-attachment deletions without this component knowing API details.- Updating
form().get('attachments')wheneverpendingAttachmentschanges keeps the form model in sync with the internal signal.- Emitting
deleteLinkAttachmentonly when a link has a non-nulluidensures newly added links are treated as creates, not deletes.This structure fits the PR’s phased attachment/link flow and looks sound.
Also applies to: 54-56, 74-76, 112-114, 121-122, 136-137, 145-152
apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.ts (1)
21-21: LGTM: Clean removal of link extraction imports.The import statement has been properly updated to remove
extractUrlsWithDomainsand related declarations, aligning with the removal of theimportantLinksfeature throughout the codebase.apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (6)
105-105: Good addition: Staged deletion workflow.The
pendingAttachmentDeletionssignal enables a staged deletion workflow with undo capability, improving the user experience by deferring destructive operations until save.
222-234: LGTM: Deletion queuing implementation.The refactored deletion methods correctly implement the staged deletion pattern:
deleteAttachmentqueues deletions without immediate executionundoDeleteAttachmentprovides rollback capabilitydeleteLinkAttachmenthandles form-based link removalThe signal updates properly use immutable operations.
367-403: Verify the orchestration handles all error scenarios.The chained operations (deletions → uploads → links) correctly use
switchMapfor sequential execution. The error handler at line 391 catches failures, but the user message may not clearly indicate which operation failed.Consider verifying that partial failures at each stage are properly reported to users. For example, if deletions succeed but uploads fail, does the message accurately reflect this?
424-515: Well-structured result aggregation.The
handleAttachmentOperationsResultsmethod comprehensively handles all scenarios:
- Full success with detailed operation counts
- Partial success with granular reporting
- Complete failure with appropriate warnings
- Individual failure logging for debugging
The conditional clearing of
pendingAttachmentDeletions(lines 510-512) correctly preserves failed deletions for retry.
817-839: LGTM: Robust batch deletion implementation.The
deletePendingAttachmentsmethod correctly:
- Parallelizes deletion requests using
mergeMap- Handles individual failures without failing the entire batch
- Returns structured results for aggregation
The error handling pattern ensures partial success scenarios are properly tracked.
867-892: LGTM: Link attachment creation logic.The
saveLinkAttachmentsmethod correctly:
- Filters for new links without a
uid(line 871), preserving existing attachments- Parallelizes creation requests using
mergeMap- Handles individual failures gracefully
- Returns structured results matching the pattern used in other methods
The separation between new links (created) and existing links (preserved via
uid) is properly implemented.
...x-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Show resolved
Hide resolved
- Add ImportantLinkFormValue interface for form-specific link data - Replace any types with proper TypeScript interfaces - Fix pending deletions logic to clear on completion without failures - Fix attachment view/download buttons to check type instead of link field LFXV2-743 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 (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
638-671: Critical: Race condition prevents existing links from populating in edit mode.The previous review correctly identified this race condition, which remains unfixed. When
populateFormWithMeetingDatais called (line 638) via the meeting load subscription (lines 143-151), it immediately invokespopulateExistingLinks()(line 639). However, attachments load independently through a separate observable chain starting atattachmentsRefresh$(line 897-910), which adds an extra async layer.Since there is no effect watching the
attachmentssignal to re-runpopulateExistingLinks()when data arrives, existing link attachments will not populate into theimportant_linksFormArray if the meeting HTTP call completes before the attachments HTTP call.Fix: Add an effect in the constructor to watch the attachments signal:
public constructor() { // Subscribe to form value changes and update validation signals this.form() .valueChanges.pipe(takeUntilDestroyed(this.destroyRef)) .subscribe(() => { this.updateCanProceed(); }); // Effect for step changes only - handles validation effect(() => { // Access the signal to create dependency this.currentStep(); // Update validation when step changes this.updateCanProceed(); }); + // Effect to populate existing links when attachments load in edit mode + effect(() => { + const attachments = this.attachments(); + const meeting = this.meeting(); + if (this.isEditMode() && meeting && attachments.length > 0) { + const importantLinksArray = this.form().get('important_links') as FormArray; + // Only populate if form array is empty to avoid duplicates + if (importantLinksArray.length === 0) { + this.populateExistingLinks(); + } + } + }); // Separate subscription for meeting data changes - populates form only once toObservable(this.meeting)
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html (1)
111-127: Consider adding an "Open Link" action for link-type attachments.Currently, only
type === 'file'attachments show view/download actions. Link-type attachments (type === 'link') have no way to open the URL directly from this UI, which may degrade UX. Users would need to copy the link from the display or navigate elsewhere.Consider adding an "Open Link" action for link attachments:
<div class="flex items-center gap-2"> @if (attachment.type === 'file' && !isPendingDeletion(attachment.uid)) { <a [href]="'/api/meetings/' + meetingId() + '/attachments/' + attachment.uid" target="_blank" class="h-6 w-6 p-0 text-slate-400 hover:text-green-500 flex items-center justify-center" data-testid="view-attachment-btn" title="View"> <i class="fa-light fa-eye w-3 h-3"></i> </a> <a [href]="'/api/meetings/' + meetingId() + '/attachments/' + attachment.uid + '?download=true'" class="h-6 w-6 p-0 text-slate-400 hover:text-blue-500 flex items-center justify-center" data-testid="download-attachment-btn" title="Download"> <i class="fa-light fa-download w-3 h-3"></i> </a> } + @if (attachment.type === 'link' && !isPendingDeletion(attachment.uid)) { + <a + [href]="attachment.link" + target="_blank" + rel="noopener noreferrer" + class="h-6 w-6 p-0 text-slate-400 hover:text-blue-500 flex items-center justify-center" + data-testid="open-link-btn" + title="Open Link"> + <i class="fa-light fa-arrow-up-right-from-square w-3 h-3"></i> + </a> + } @if (isPendingDeletion(attachment.uid)) {apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
511-513: Consider clearing pending deletions after all attempts, not just successes.Currently,
pendingAttachmentDeletionsis only cleared whentotalDeleteFailures === 0(line 511). If all deletion attempts fail, the pending array retains stale state. While the component navigates away immediately (line 515), conceptually it's cleaner to clear the queue after processing regardless of outcome, or at least clear the successfully deleted IDs.Consider clearing processed deletions:
- // Clear pending deletions when operations complete without failures - if (totalDeleteFailures === 0 && this.pendingAttachmentDeletions().length > 0) { - this.pendingAttachmentDeletions.set([]); + // Clear pending deletions that were successfully processed + if (result.deletions.successes > 0 || result.deletions.failures.length > 0) { + // Remove successfully deleted IDs from pending array + if (result.deletions.successes > 0 && result.deletions.failures.length === 0) { + this.pendingAttachmentDeletions.set([]); + } else if (result.deletions.failures.length > 0) { + // Keep only the failed deletions in pending array for potential retry + this.pendingAttachmentDeletions.set(result.deletions.failures); + } }
📜 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/components/meeting-manage/meeting-manage.component.ts(9 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html(1 hunks)packages/shared/src/interfaces/meeting.interface.ts(1 hunks)
🧰 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.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html
🧬 Code graph analysis (1)
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)packages/shared/src/interfaces/meeting.interface.ts (1)
ImportantLinkFormValue(61-70)
🔇 Additional comments (7)
packages/shared/src/interfaces/meeting.interface.ts (1)
57-70: LGTM! Well-designed interface for link-attachment tracking.The
ImportantLinkFormValueinterface is properly typed with clear JSDoc documentation. Theuid: string | nullfield correctly distinguishes between new links (null) and existing link attachments (present), aligning well with the PR's objective to manage links as attachment records.apps/lfx-one/src/app/modules/project/meetings/components/meeting-resources-summary/meeting-resources-summary.component.html (1)
70-108: LGTM! Clear visual feedback for staged deletions.The conditional styling effectively communicates the pending deletion state with red tones, opacity reduction, line-through text, and an explicit "Marked for deletion" indicator. This provides excellent UX for the undo workflow.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (5)
26-26: LGTM! Proper imports and signal initialization.The
ImportantLinkFormValueimport andpendingAttachmentDeletionssignal are correctly typed and initialized.Also applies to: 106-106
223-235: LGTM! Clean staged deletion implementation.The deletion methods correctly queue attachment IDs for later processing and provide an undo mechanism, aligning well with the staged deletion UX flow.
360-413: LGTM! Well-orchestrated attachment operations.The sequential chaining of
deletePendingAttachments→savePendingAttachments→saveLinkAttachmentsusingswitchMapensures correct operation order. Error handling is present with appropriate user feedback.
818-840: LGTM! Robust batch deletion implementation.The method correctly processes queued deletions in parallel with
mergeMap, handles individual errors gracefully, and aggregates results for reporting. The error handling ensures partial failures don't break the entire operation.
868-893: LGTM! Type-safe link attachment creation.The method correctly filters for new links (those without
uid) and uses theImportantLinkFormValue[]type cast for proper type safety. The parallel processing with error handling per link ensures robust operation.
Summary
Replace the existing implementation where links are extracted from meeting descriptions with the attachment API that supports link-type attachments.
Changes
Phase 1: Remove Link Extraction
extractUrlsWithDomainsutility usage from meeting-join and meeting-card componentsimportantLinkssignals that parsed URLs from descriptionsfa-arrow-up-right-from-squareicon with consistentw-8widthPhase 2: Save Links as Attachments
saveLinkAttachments()method to create link attachments fromimportant_linksFormArrayhandleMeetingSuccess()to chain link creation after file uploadscreateAttachmentFromUrlAPI endpointPhase 3: Handle Link Deletion
populateExistingLinks()to load existing link attachments into FormArray when editinguidfield to track existing attachmentsdeleteLinkAttachment()handler to mark links for deletion when removedsaveLinkAttachments()to only create new links (withoutuid)uidare preserved and not recreatedTechnical Details
Data Flow:
createAttachmentFromUrl→ attachment records withtype: 'link'uid, only create new onesuidto deletion queue → delete on saveComponents Modified:
meeting-join.component- Display link attachmentsmeeting-card.component- Display link attachments in card viewmeeting-manage.component- CRUD operations for link attachmentsmeeting-resources-summary.component- Link form managementBenefits
Testing
JIRA
LFXV2-743
🤖 Generated with Claude Code