refactor(meetings): migrate to global architecture #161
Conversation
Fixed recurring meetings incorrectly showing as past by refactoring to use separate API endpoints for upcoming and past meetings: - Load upcoming meetings via getMeetingsByProject() - Load past meetings via getPastMeetingsByProject() - React to timeFilter changes to call appropriate endpoint - Hide RSVP button for non-organizers on past meetings - Removed client-side time filtering (handled by API) This ensures proper meeting categorization and enables accurate counts for both upcoming and past meetings. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
Migrated project selection persistence from localStorage to cookies using ngx-cookie-service-ssr for full SSR compatibility: Project Context Changes: - Replace localStorage with SsrCookieService - Cookies work on both server and client-side - 30-day expiration, SameSite=Lax security - Fixes project context being null during SSR Meeting/Committee Dashboard Fixes: - Use combineLatest to prevent duplicate API calls - Lazy load data only for active tabs - Fix loading states to prevent "No data found" flash - Add project existence check to empty states - Remove counters from filter buttons UI Improvements: - Hide RSVP buttons for non-organizers on past meetings - Improve label consistency in committee form This enables data fetching during SSR when project is selected, eliminating loading delays and improving initial render performance. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
- Moved meetings module from /project/:slug/meetings to /meetings for improved UX - Relocated all meeting components from modules/project/meetings to modules/meetings - Added new routes: /meetings/create and /meetings/:id/edit for meeting management - Updated meeting-card component to use global meeting routes - Fixed meeting-card registrant toggle to not show add button for past meetings - Added feature flag service to sidebar for role selector control - Reordered meeting features constants to prioritize AI Summary feature BREAKING CHANGE: Meeting routes moved from project-scoped to global scope LFXV2-757 LFXV2-760 LFXV2-761 LFXV2-762 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
WalkthroughThis PR consolidates meeting management from project-scoped routes to a top-level meetings module, separates upcoming and past meetings into distinct reactive streams, migrates project context from ProjectService to ProjectContextService across multiple components, adds AI summary review functionality for past meetings, and introduces feature flagging for role selection. The old project-scoped meeting dashboard is removed entirely, with functionality relocated. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Meetings Dashboard
participant ProjectCtx as ProjectContextService
participant TimeFilter as TimeFilter Signal
participant Meetings as Meeting Service
participant Data as Upcoming/Past Streams
UI->>ProjectCtx: getProjectId()
UI->>TimeFilter: read timeFilter signal
alt TimeFilter = 'upcoming'
UI->>Meetings: getMeetingsByProject(projectId)
Meetings-->>Data: upcoming meetings[]
Data-->>UI: display upcomingMeetings with search filter
else TimeFilter = 'past'
UI->>Meetings: getPastMeetingsByProject(projectId)
Meetings-->>Data: past meetings[]
Data-->>UI: display pastMeetings with search filter
end
Note over Data,UI: finalize() clears loading state when stream completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts (1)
29-40: Feature-flaggedshowRoleSelectorsignal looks correctInjecting
FeatureFlagServiceand exposingshowRoleSelectorviagetBooleanFlag('role-selector', true)aligns with the ticket: the flag is local to the sidebar, defaults totrue, and is consumed as a signal in the template like your other computed values.If this
'role-selector'key will be reused across components, consider lifting it into a shared constants file to avoid typos and ease future renames; otherwise this is fine as-is.apps/lfx-one/src/app/modules/meetings/components/meeting-committee-modal/meeting-committee-modal.component.ts (1)
15-16: Guard committees load against missing project context to avoid noisy errorsWiring to
ProjectContextServicelooks good, but this path:this.committeeService.getCommitteesByProject(this.projectContextService.getProjectId() || '')will still call the API with an empty string when no project is selected, which will likely surface “Failed to load committees” errors to users.
Consider short‑circuiting when there is no projectId so the modal stays empty/idle instead of erroring, e.g.:
const projectId = this.projectContextService.getProjectId(); this.committees = toSignal( (projectId ? this.committeeService.getCommitteesByProject(projectId) : of([]) ).pipe( tap(() => this.committeesLoading.set(false)), catchError((error) => { if (projectId) { console.error('Failed to load committees:', error); this.messageService.add({ severity: 'error', summary: 'Error', detail: 'Failed to load committees' }); } this.committeesLoading.set(false); return of([]); }) ), { initialValue: [] } );This keeps the UI quiet when no project is in context, while preserving the existing error UX when a project is selected.
Also applies to: 39-40, 61-77
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html (1)
73-82: Remove redundant condition check.The inner
@if (!pastMeeting())is redundant because the outer condition on line 62 already ensures!pastMeeting()is true.Apply this diff to simplify the template:
- @if (!pastMeeting()) { - <lfx-button - [icon]="loading() ? 'fa-light fa-circle-notch fa-spin' : 'fa-light fa-ellipsis'" - [text]="true" - [rounded]="true" - size="small" - severity="secondary" - data-testid="meeting-actions-button" - (click)="actionMenu.toggle($event)"></lfx-button> - } + <lfx-button + [icon]="loading() ? 'fa-light fa-circle-notch fa-spin' : 'fa-light fa-ellipsis'" + [text]="true" + [rounded]="true" + size="small" + severity="secondary" + data-testid="meeting-actions-button" + (click)="actionMenu.toggle($event)"></lfx-button>apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts (1)
71-75: Consider optimizing loading state updates.Setting both
meetingsLoadingandpastMeetingsLoadingto true unconditionally may cause unnecessary UI updates, since only one stream will actually load based on the currenttimeFiltervalue. Consider setting only the relevant loading flag:public refreshMeetings(): void { - this.meetingsLoading.set(true); - this.pastMeetingsLoading.set(true); + if (this.timeFilter() === 'past') { + this.pastMeetingsLoading.set(true); + } else { + this.meetingsLoading.set(true); + } this.refresh$.next(); }
📜 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 (26)
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.html(1 hunks)apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts(2 hunks)apps/lfx-one/src/app/modules/committees/components/committee-form/committee-form.component.ts(1 hunks)apps/lfx-one/src/app/modules/meetings/components/meeting-committee-modal/meeting-committee-modal.component.ts(3 hunks)apps/lfx-one/src/app/modules/meetings/components/meeting-details/meeting-details.component.ts(3 hunks)apps/lfx-one/src/app/modules/meetings/components/meeting-type-selection/meeting-type-selection.component.ts(3 hunks)apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html(2 hunks)apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.html(1 hunks)apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.ts(9 hunks)apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.html(2 hunks)apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.ts(0 hunks)apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.html(3 hunks)apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts(5 hunks)apps/lfx-one/src/app/modules/meetings/meetings.routes.ts(1 hunks)apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html(0 hunks)apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts(0 hunks)apps/lfx-one/src/app/modules/project/meetings/meetings.routes.ts(0 hunks)apps/lfx-one/src/app/modules/project/project.routes.ts(0 hunks)apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html(7 hunks)apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts(3 hunks)apps/lfx-one/src/app/shared/components/meeting-registrants/meeting-registrants.component.html(1 hunks)apps/lfx-one/src/app/shared/components/meeting-registrants/meeting-registrants.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html(1 hunks)apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts(3 hunks)apps/lfx-one/src/app/shared/services/project-context.service.ts(3 hunks)packages/shared/src/constants/meeting.constants.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts
- apps/lfx-one/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html
- apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.ts
- apps/lfx-one/src/app/modules/project/project.routes.ts
- apps/lfx-one/src/app/modules/project/meetings/meetings.routes.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/shared/components/meeting-registrants/meeting-registrants.component.tsapps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.htmlapps/lfx-one/src/app/modules/meetings/components/meeting-details/meeting-details.component.tsapps/lfx-one/src/app/modules/meetings/components/meeting-type-selection/meeting-type-selection.component.tsapps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.tsapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.tsapps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/app/shared/components/meeting-registrants/meeting-registrants.component.htmlapps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.htmlapps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.htmlapps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts
🧬 Code graph analysis (4)
apps/lfx-one/src/app/modules/meetings/components/meeting-type-selection/meeting-type-selection.component.ts (1)
packages/shared/src/interfaces/project.interface.ts (1)
Project(54-103)
apps/lfx-one/src/app/shared/services/project-context.service.ts (2)
apps/lfx-one/src/app/shared/services/persona.service.ts (1)
Injectable(10-64)packages/shared/src/interfaces/project.interface.ts (1)
ProjectContext(167-174)
apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (2)
CreateMeetingRequest(201-224)UpdateMeetingRequest(226-249)
apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (2)
Meeting(90-167)PastMeeting(538-551)
🔇 Additional comments (34)
apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts (1)
9-9: FeatureFlagService import is appropriate and scoped correctlyImporting
FeatureFlagServicein the shared sidebar makes sense for gating UI elements that are globally visible. No issues here.apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html (1)
37-41: Persona selector gated correctly byshowRoleSelectorWrapping the persona selector in
@if (showRoleSelector())cleanly ties visibility to the new feature flag while preserving the existing layout wrapper. This matches the LFXV2-761 objective without introducing obvious regressions.packages/shared/src/constants/meeting.constants.ts (1)
50-57: LGTM! AI feature addition follows the established pattern.The
zoom_ai_enabledfeature entry is well-structured and consistent with other features in the array. Positioning it before transcript generation aligns with the PR objective to prioritize AI Summary in the feature list.apps/lfx-one/src/app/modules/meetings/meeting-join/meeting-join.component.html (2)
8-8: Confirm gap-3 removal was intentional and doesn't break layout.The
gap-3utility class was removed from line 8's outer flex container. This is inconsistent with similar flex-col structures in the same file (e.g., line 274 retainsgap-3). No CSS overrides compensating for this removal were found. Verify this change doesn't unintentionally collapse spacing between the Project Logo section and Main Meeting Card, particularly on mobile views.
275-275: I need to gather more context to verify the completeness of the changes and understand the app's i18n usage.Based on the verification results, here is the rewritten review comment:
Verify text consistency and confirm whether i18n is required for this component.
The text was updated to second-person language ("You are signed in as:"), but verification found only one location in the codebase, not two as mentioned in the summary. Additionally, the hardcoded string lacks an
i18nattribute for translation support. Angular doesn't automatically translate anything—you must mark strings for translation using the i18n attribute, and unmarked text cannot be extracted for translation.Confirm: (1) whether a second location should have been updated, and (2) if the application or this module requires internationalization support.
apps/lfx-one/src/app/shared/components/meeting-registrants/meeting-registrants.component.ts (1)
9-9: Registrant modal import migration aligns with new meetings moduleImporting
RegistrantModalComponentfrom@modules/meetings/...matches the global/meetingsrouting refactor; usages below remain unchanged, so behavior is preserved.apps/lfx-one/src/app/modules/committees/components/committee-form/committee-form.component.ts (1)
83-85: Dynamic “No Parent …” label correctly uses configurable committee labelUsing
'No Parent ' + this.committeeLabelkeeps the option in sync with the singular label configuration and avoids hard‑coding “Group”.apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.html (1)
77-85: Empty‑state now gated on project context; confirm UX for “no project selected”Condition changes mean:
- Only show “No {{ committeeLabelPlural }} found” when
project()?.projectIdexists and there are zero filtered committees.- Only show the table when there is at least one filtered committee.
- When there is no
projectId, the content area will render neither empty state nor table.If the intended UX is to avoid an empty‑state until a project is selected, this is correct. If you instead want to guide users to select a project when none is set, consider a dedicated “Select a project to view {{ committeeLabelPlural.toLowerCase() }}” state for the
!projectIdcase.apps/lfx-one/src/app/modules/meetings/components/meeting-type-selection/meeting-type-selection.component.ts (1)
14-15: Child‑project loading via ProjectContextService looks correct; validateprojectIdvsuidassumptionSwitching to
ProjectContextServiceand using:const currentProject = this.projectContextService.selectedProject(); ... const params = new HttpParams().set('tags', `parent_uid:${currentProject.projectId}`); ... return projects.filter((project) => project.uid !== currentProject.projectId && project.writer);is consistent with the new context model and keeps the behavior of excluding the current project and requiring
writerpermission.Just ensure that
ProjectContext.projectIdis indeed the same identifier used asProject.uid/parent_uidin the backend; if those ever diverge, this filter and tag construction would need to be adjusted.Also applies to: 33-34, 137-145, 149-150
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts (1)
19-20: combineLatest + finalize clean up committee loading state nicelyUsing
combineLatest([project$, this.refresh]).pipe( switchMap(([project]) => { if (!project?.projectId) { this.committeesLoading.set(false); return of([]); } this.committeesLoading.set(true); return this.committeeService.getCommitteesByProject(project.projectId).pipe( catchError((error) => { console.error('Failed to load committees:', error); return of([]); }), finalize(() => this.committeesLoading.set(false)) ); }) );gives you:
- Correct coupling of the loading spinner to the actual API call window.
- Automatic cleanup even on errors via
finalize.- A graceful “no project selected” path that clears the spinner and returns an empty list.
This aligns well with the updated HTML logic that checks
project()?.projectIdbefore showing the empty state.Also applies to: 228-251
apps/lfx-one/src/app/modules/meetings/components/meeting-details/meeting-details.component.ts (1)
20-20: LGTM - Consistent migration to ProjectContextService.The migration from
ProjectServicetoProjectContextServiceis correctly implemented with proper null checking at line 180 before using the project data.Also applies to: 51-51, 175-175
apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.html (1)
32-32: LGTM - Intentional UI simplification.The removal of count displays from the time filter buttons is consistent with the broader refactor to simplify the top bar interface.
Also applies to: 43-43
apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.html (1)
4-11: LGTM - Improved navigation UX.The addition of the back navigation block with a link to
/meetingsimproves user experience and aligns with the global routing architecture. The padding adjustment provides better spacing for the new layout.apps/lfx-one/src/app/shared/components/meeting-registrants/meeting-registrants.component.html (1)
31-36: LGTM - Improved empty state UX.The addition of an empty state message for past meetings with no participants improves user experience by providing clear feedback instead of showing a blank area.
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts (3)
39-42: LGTM - Import paths updated for component relocation.The import paths have been correctly updated to reflect the migration of meeting components from
modules/project/meetingstomodules/meetings.
175-175: LGTM - Fixes registrant modal for past meetings.The updated condition correctly prevents opening the "Add Registrant" modal for past meetings, as specified in PR objective LFXV2-762. This improves UX by blocking an action that shouldn't be available for past meetings.
433-433: LGTM - Routing updated for global architecture.The routerLink has been correctly updated to use the global
/meetingspath instead of the project-scoped path, aligning with the architectural migration described in PR objective LFXV2-760.apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.html (4)
15-15: LGTM - Updated to global routing.The routerLink has been correctly updated to use the global
/meetings/createpath, consistent with the architectural migration.
48-54: LGTM - Improved data binding for top bar.The conditional binding now correctly passes either
pastMeetings()orupcomingMeetings()to the top bar based on the selected time filter, ensuring the component receives the appropriate dataset.
62-75: LGTM - Enhanced loading states.The loading skeleton now uses separate loading flags for upcoming vs past meetings (
meetingsLoading()vspastMeetingsLoading()), providing more accurate loading feedback based on which dataset is being fetched.
76-76: LGTM - Improved empty state condition.Adding the
project()?.projectIdcheck prevents showing the "No Meetings Found" empty state when no project is selected, which improves the user experience.apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.ts (4)
8-8: LGTM - Consistent migration to ProjectContextService.The migration from
ProjectServicetoProjectContextServiceis implemented consistently across imports, dependency injection, and usage. The addition ofRouterLinksupports the back navigation button in the template.Also applies to: 44-44, 51-55, 72-72, 83-83
183-183: LGTM - Navigation updated for global routing.All navigation paths have been consistently updated to use the global
/meetingsroute, aligning with the architectural migration described in PR objective LFXV2-760.Also applies to: 389-389, 398-398, 499-499, 518-518
785-785: LGTM - Project retrieval for title generation.The code correctly attempts to retrieve the project from
selectedProject()with a fallback toselectedFoundation(), which handles both project-level and foundation-level meeting creation scenarios.
275-310: Verify project_uid is always available.The
prepareMeetingData()method now retrievesproject_uidusingprojectContextService.getProjectId()with a fallback toformValue.selectedProjectUid. Sinceproject_uidis a required field in bothCreateMeetingRequestandUpdateMeetingRequestinterfaces (lines 201-223 and 225-248 in meeting.interface.ts), ensure that at least one of these sources always provides a valid value.apps/lfx-one/src/app/modules/meetings/meetings.routes.ts (1)
15-22: The review comment is based on incorrect assumptions about Angular route guard inheritance.The
/meetings/createand/meetings/:id/editroutes are protected by the parent route'sauthGuard. In Angular routing, when a parent route hascanActivate, all its child routes inherit that guard automatically. Here, the 'meetings' path (app.routes.ts:23-24) is nested under the root '' route which hascanActivate: [authGuard](app.routes.ts:10). Therefore, both new routes are already authenticated—addingcanActivateto each child route would be redundant rather than necessary.Likely an incorrect or invalid review comment.
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html (4)
62-71: LGTM: Routing migration and past meeting guard.The migration to global routing (
/meetings/:id/edit) and the addition of the!pastMeeting()guard correctly prevent editing past meetings, aligning with PR objectives LFXV2-760 and LFXV2-762.
147-163: LGTM: AI summary review banner for organizers.The banner provides clear visibility and action for organizers to review unapproved AI summaries on past meetings. The conditional logic correctly targets the intended audience and state.
175-177: LGTM: Resources section improvements.The changes correctly prevent resource additions to past meetings (per LFXV2-762), simplify empty-state messaging, and enhance UX by displaying file sizes in tooltips for non-link attachments.
Also applies to: 181-181, 190-190
321-346: LGTM: Action buttons layout and visibility.The conditional margin, updated label logic for guest management, and hiding "See Meeting Details" for past meetings all improve UX consistency and align with the past meeting restrictions introduced in this PR.
apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts (4)
10-10: LGTM: Import updates for refactored architecture.The addition of
PastMeetingand the switch frommerge/taptocombineLatest/finalizecorrectly support the new separated data loading pattern.Also applies to: 14-14
31-35: LGTM: Signal type refinements.The separation of
upcomingMeetingsandpastMeetingsinto distinct typed signals, along with the union type forfilteredMeetings, provides clear type safety for the refactored data model.
77-122: LGTM: Upcoming meetings stream with conditional loading.The refactored approach using
combineLatestwith time filter guards andfinalizefor cleanup correctly implements on-demand loading. The sorting by current/next occurrence aligns with PR objective LFXV2-757 to fix recurring meeting classification.
152-172: LGTM: Unified filtering logic.The computed signal correctly selects between upcoming and past meetings based on the time filter, then applies search across relevant fields. The union type works seamlessly since both
MeetingandPastMeetingshare the filtered properties.
apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts
Show resolved
Hide resolved
| } @else if (pastMeeting() && !meeting().organizer) { | ||
| <!-- Show Recording and AI Summary buttons for past meetings (non-organizers) --> | ||
| <div class="flex flex-col gap-2"> | ||
| <lfx-button | ||
| class="w-full" | ||
| icon="fa-light fa-play" | ||
| label="See Recording" | ||
| size="small" | ||
| severity="secondary" | ||
| styleClass="w-full" | ||
| data-testid="see-recording-button" | ||
| (click)="openRecordingModal()"> | ||
| </lfx-button> | ||
| @if (hasSummary()) { | ||
| <lfx-button | ||
| class="w-full" | ||
| icon="fa-light fa-sparkles" | ||
| label="See AI Summary" | ||
| size="small" | ||
| severity="secondary" | ||
| styleClass="w-full" | ||
| data-testid="see-ai-summary-button" | ||
| (click)="openSummaryModal()"> | ||
| </lfx-button> | ||
| } @else { | ||
| <lfx-button | ||
| class="w-full" | ||
| icon="fa-light fa-sparkles" | ||
| label="AI Summary Pending" | ||
| size="small" | ||
| severity="secondary" | ||
| styleClass="w-full" | ||
| [disabled]="true" | ||
| data-testid="ai-summary-pending-button"> | ||
| </lfx-button> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify recording availability check for non-organizers.
Non-organizers always see the "See Recording" button for past meetings (line 281-290), while organizers only see it when hasRecording() returns true (line 238). This inconsistency may lead to broken UX if a non-organizer clicks the recording button when no recording exists.
Run the following script to check if hasRecording() is used elsewhere and verify the intended behavior:
🏁 Script executed:
#!/bin/bash
# Search for hasRecording usage patterns in meeting-card component
rg -nP -C3 'hasRecording\(\)' --type=ts --type=html -g '*meeting-card*'Length of output: 1853
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract lines 278-320 from meeting-card component to verify non-organizer section
cat -n apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html | sed -n '275,320p'
Length of output: 2150
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for openRecordingModal implementation in meeting-card component
rg -nA10 'openRecordingModal\s*\(' --type=ts -g '*meeting-card*'
Length of output: 3818
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for hasRecording and recordingShareUrl definitions in meeting-card component
rg -nP '(hasRecording|recordingShareUrl)\s*[=\(:]' --type=ts -g '*meeting-card*' | head -20
Length of output: 1144
Add hasRecording() guard to non-organizer recording button for consistency.
Non-organizers see the "See Recording" button unconditionally (line 281), while organizers only see it when hasRecording() returns true (line 238). Both ultimately check the same condition via recordingShareUrl(), but the template guard for non-organizers is missing. The button will always appear for non-organizers but silently fail if clicked when no recording exists. Add @if (hasRecording()) around the recording button for non-organizers to match organizer behavior and improve UX consistency.
🤖 Prompt for AI Agents
In
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.html
around lines 278 to 314, the "See Recording" button for non-organizers is
rendered unconditionally; wrap that button in an @if (hasRecording())
conditional (matching the organizer branch) so the recording button only appears
when recordingShareUrl()/hasRecording() indicates a recording exists, preventing
a dead button and keeping behavior consistent.
| import { inject, Injectable, signal, WritableSignal } from '@angular/core'; | ||
| import { ProjectContext } from '@lfx-one/shared/interfaces'; | ||
| import { SsrCookieService } from 'ngx-cookie-service-ssr'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Cookie‑backed context persistence looks good; double‑check getProjectId semantics and env usage
The move from localStorage to SsrCookieService with:
- 30‑day expiry
path: '/'sameSite: 'Lax'secure: process.env['NODE_ENV'] === 'production'
is a solid step toward SSR compatibility, and clearProject/clearFoundation correctly delete the same keys on the same path.
Two points worth tightening while you’re in here:
getProjectIdbehavior vs comment
/**
* Get the current project ID (sub-project if set, otherwise foundation)
*/
public getProjectId(): string {
return this.selectedProject()?.projectId || '';
}The implementation never falls back to the foundation ID, despite the doc comment. Given that multiple components (e.g., meeting‑committee modal, dashboards) are starting to depend on this helper, consider either:
- Implementing the documented behavior:
public getProjectId(): string {
return this.selectedProject()?.projectId || this.selectedFoundation()?.projectId || '';
}or updating the comment to make it explicit that only sub‑project IDs are returned.
process.envin browser builds
Using process.env['NODE_ENV'] in an Angular app depends on your build tooling defining process.env at compile/bundle time. Please confirm your Angular/Nx setup and ngx-cookie-service-ssr integration support this pattern; otherwise, consider wiring secure from the existing environment config instead.
Also applies to: 12-15, 49-52, 59-60, 76-84, 86-94
🌐 Web query:
Check whether `ngx-cookie-service-ssr` recommends using `process.env.NODE_ENV` for the `secure` flag in Angular/SSR setups, and confirm typical patterns for environment detection in Angular 17+ projects.
💡 Result:
Short answer: No — ngx-cookie-service-ssr’s docs do not tell you to base the cookie "secure" flag on process.env.NODE_ENV. The package README documents cookie behavior (e.g., it will force secure=true when sameSite='None') and SSR usage, but does not recommend using process.env.NODE_ENV to set secure. [1][2]
Typical Angular (17+) patterns:
- Use Angular environment files (src/environments/*.ts + angular.json fileReplacements) or a build-time env system and import an environment object in your code (environment.production / environment.x). This is the standard way to toggle production vs development values. [3]
- For runtime environment variables in newer toolchains, use import.meta.env (or a helper like @ngx-env) rather than relying on process.env in browser code. Many projects map import.meta.env into their environment files. [4]
- For SSR vs browser detection, use Angular’s PLATFORM_ID / isPlatformBrowser (or a window-check) rather than relying on process globals. Use this to decide server/browser-only behaviors (including whether to set secure cookies). [5]
If you want, I can show a minimal snippet demonstrating: (a) setting secure from environment.production, (b) mapping import.meta.env into environment.ts, and (c) using isPlatformBrowser to avoid browser-only calls.
Sources:
[1] ngx-cookie-service (SSR) README / docs showing SSR instructions and secure/sameSite behavior. (npm.io)
[2] npm package page for ngx-cookie-service-ssr (package/README overview). (npmjs.com)
[3] Angular CLI — Build environments / fileReplacements (environment.ts pattern). (angular.dev)
[4] Guidance on import.meta.env and modern env handling (Angular 17 context / migration notes). (iifx.dev)
[5] Angular SSR: use PLATFORM_ID / isPlatformBrowser to detect browser vs server. (angular.fr)
Citations:
- 1: https://npm.io/package/ngx-cookie-ssr?utm_source=openai
- 2: https://www.npmjs.com/package/ngx-cookie-service-ssr?utm_source=openai
- 3: https://angular.dev/tools/cli/environments?utm_source=openai
- 4: https://iifx.dev/en/articles/451730531?utm_source=openai
- 5: https://angular.fr/ssr/client-compatibility.html?utm_source=openai
Wait, that command needs the file. Let me remove skip_cloning and run without it.
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify the actual code in project-context.service.ts to confirm process.env usage
cat apps/lfx-one/src/app/shared/services/project-context.service.ts | grep -n "process.env\|secure\|persistToStorage" -A 3 -B 1
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Inspect the project-context.service.ts file to confirm process.env usage pattern
grep -n "process.env\|secure.*:" apps/lfx-one/src/app/shared/services/project-context.service.ts 2>/dev/null | head -20Length of output: 58
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for Angular environment files and build configuration
fd -type f -name "environment*.ts" apps/lfx-one/src
echo "---"
cat apps/lfx-one/angular.json 2>/dev/null | grep -A 10 "fileReplacements" | head -20
Length of output: 818
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify environment files exist and check their structure
find apps/lfx-one/src/environments -name "environment*.ts" -exec echo "=== {} ===" \; -exec head -10 {} \;
Length of output: 1544
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the getProjectId implementation and its comment
grep -n "getProjectId\|Get the current project" apps/lfx-one/src/app/shared/services/project-context.service.ts -A 3 -B 1
Length of output: 198
Fix process.env antipattern and clarify getProjectId semantics
The project already has Angular environment files configured with fileReplacements (environment.ts, environment.prod.ts, environment.staging.ts), eliminating the need for process.env in the browser code. Replace line 82:
// ❌ Current (line 82)
secure: process.env['NODE_ENV'] === 'production',
// ✅ Fix
import { environment } from '@lfx-one/shared/environments';
// ...
secure: environment.production,No official ngx-cookie-service-ssr documentation recommends using process.env.NODE_ENV for the secure flag; Angular 17+ projects use environment files with fileReplacements as the standard pattern.
Also fix the getProjectId comment/implementation mismatch (lines 62–67):
/**
* Get the current project ID (sub-project if set, otherwise foundation)
*/
public getProjectId(): string {
return this.selectedProject()?.projectId || '';
}Either implement the documented fallback or update the comment to clarify that only sub-project IDs are returned.
Also applies to: 12–15, 49–52, 59–60, 76–84, 86–94
🤖 Prompt for AI Agents
In apps/lfx-one/src/app/shared/services/project-context.service.ts around lines
4–7, 12–15, 49–52, 59–60, 62–67, 76–84 and 86–94: remove the process.env usage
and import the Angular environment object from @lfx-one/shared/environments,
then replace any secure: process.env['NODE_ENV'] === 'production' with secure:
environment.production; for getProjectId either implement the documented
fallback so it returns the selected sub-project id or, if intended to only
return sub-project ids, update the comment to reflect that (i.e., change
implementation to return selectedProject()?.projectId ||
selectedProject()?.foundationProjectId || '' for fallback, or update the JSDoc
to state it returns only the sub-project id) and apply the same
fix/clarification to the other referenced line ranges.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the meetings module from project-scoped routing to global routing architecture. The meetings feature is moved from /project/:slug/meetings to /meetings, simplifying navigation and improving user experience. Key changes include component relocation from modules/project/meetings to modules/meetings, route configuration updates, migration from localStorage to cookie-based storage for SSR compatibility, and UI improvements including feature flag support for the role selector and enhanced meeting card functionality for past meetings.
Reviewed Changes
Copilot reviewed 26 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/constants/meeting.constants.ts |
Reordered MEETING_FEATURES to prioritize AI Summary |
apps/lfx-one/src/app/shared/services/project-context.service.ts |
Migrated from localStorage to cookie storage for SSR compatibility |
apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts |
Added feature flag service for role selector control |
apps/lfx-one/src/app/shared/components/meeting-card/meeting-card.component.ts |
Updated route links and fixed registrant toggle for past meetings |
apps/lfx-one/src/app/modules/project/project.routes.ts |
Removed project-scoped meetings route |
apps/lfx-one/src/app/modules/meetings/meetings.routes.ts |
Added global meeting management routes |
apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts |
Updated to support global routing with project context |
| Multiple meeting components | Relocated from modules/project/meetings to modules/meetings with import updates |
Comments suppressed due to low confidence (3)
apps/lfx-one/src/app/modules/meetings/meeting-manage/meeting-manage.component.ts:785
- Using fallback logic
selectedProject() || selectedFoundation()without clear documentation or type safety could lead to confusion. Theprojectvariable may contain either a project or foundation object, but the subsequent code expects a project. Consider adding explicit type checking or documentation to clarify when each is used and ensure the returned object has the expected properties.
apps/lfx-one/src/app/modules/meetings/components/meeting-type-selection/meeting-type-selection.component.ts:149 - Property comparison mismatch: comparing
project.uidwithcurrentProject.projectIdmay cause the filter to fail if these represent different identifier types or if the naming is inconsistent. Verify thatprojectIdfrom the context service corresponds to theuidfield in the Project interface to ensure correct filtering.
apps/lfx-one/src/app/modules/meetings/components/meeting-details/meeting-details.component.ts:175 - No null check for
currentProjectbefore accessing its properties. The subsequent code at line 785 accessesproject.projectId, but ifselectedProject()returnsnull, this will cause a runtime error. Add null checking or provide a default value to handle cases where no project is selected.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.cookieService.set(key, JSON.stringify(project), { | ||
| expires: 30, // 30 days | ||
| path: '/', | ||
| sameSite: 'Lax', | ||
| secure: process.env['NODE_ENV'] === 'production', | ||
| }); |
There was a problem hiding this comment.
The cookie secure flag is only enabled in production based on process.env['NODE_ENV']. In Angular applications, this environment check may not work as expected during runtime since environment variables are typically replaced at build time. Consider using Angular's isDevMode() or environment.production instead for proper environment detection.
| label="Create Meeting" | ||
| size="small" | ||
| [routerLink]="['/project', project()?.slug || '', 'meetings', 'create']"> | ||
| [routerLink]="['/meetings', 'create']"> |
There was a problem hiding this comment.
The create meeting route no longer requires a project slug, but the component still depends on project()?.projectId for data loading. Without a project context, users may reach the create page but be unable to create meetings. Consider adding route guards or UI indicators to ensure users have a valid project context before accessing this route.
- Resolved conflicts in meetings-dashboard component (HTML and TS) - Resolved conflicts in meeting-card component (HTML) - Kept our routing changes while adopting main's loading state improvements - Used combineLatest approach for both upcoming and past meetings loading - Removed unused imports (merge, tap) - Added hasRecording() guard for non-organizer recording button for UX consistency LFXV2-757 Signed-off-by: Asitha de Silva <asithade@gmail.com>
Summary
Major architectural refactoring to migrate the meetings module from project-scoped routing to global routing for improved user experience and simplified navigation.
Key Changes
/project/:slug/meetingsto/meetingsmodules/project/meetingstomodules/meetings/meetings/createand/meetings/:id/editfor meeting managementBreaking Changes
Meeting routes have been moved from project-scoped to global scope:
/project/:slug/meetings/meetingsFiles Changed
Related Issues
🤖 Generated with Claude Code