feat(meeting): refactor meeting module and update environment config #86
Conversation
- Refactor meeting module with child routes and dedicated components (LFXV2-487) - Split meeting component into separate join and not-found components - Implement child routes for better modularity and error handling - Add proper authentication flow and form validation for meeting join - Update environment configuration to replace profile URLs with support URLs (LFXV2-488) - Fix header component by removing hardcoded URL reference LFXV2-487 LFXV2-488 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. WalkthroughReplaces the direct meetings route with a lazy-loaded meetings route exposing MEETING_ROUTES (/:id and /not-found). Adds MeetingNotFoundComponent. Renames MeetingComponent → MeetingJoinComponent, adds fetch/error navigation to /meetings/not-found, updates recurrence tooltip/test id, and replaces environment.urls.profile with urls.support; header no longer links to profile. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant MeetingFeature as Meetings Feature (MEETING_ROUTES)
participant Join as MeetingJoinComponent
participant API as Meeting API
participant NotFound as MeetingNotFoundComponent
User->>Router: Navigate to /meetings/:id
Router->>MeetingFeature: Lazy-load MEETING_ROUTES
MeetingFeature->>Join: Load MeetingJoinComponent for :id
rect rgba(200,230,255,0.25)
Note over Join: fetch public meeting
Join->>API: getPublicMeeting(id)
API-->>Join: meeting data or error
alt success
Join-->>User: render meeting join UI
else 400/403/404 or missing id
Join->>Router: navigate('/meetings/not-found')
Router->>MeetingFeature: resolve not-found route
MeetingFeature->>NotFound: Load MeetingNotFoundComponent
NotFound-->>User: show not-found page
else other error
Join-->>Join: rethrow/error propagate
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the meeting module for better modularity and error handling while updating environment configuration to replace profile URLs with support URLs across all environments.
- Replaced single meeting component with modular structure including dedicated join and not-found components
- Updated routing from direct component loading to lazy-loaded child routes with proper error handling
- Replaced profile URLs with Linux Foundation JIRA Service Desk support URLs across all environment configurations
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| environment files | Replace profile URLs with support URLs pointing to JIRA Service Desk |
| header.component.ts | Remove hardcoded environment URL reference and unused import |
| meeting.routes.ts | New routing configuration for meeting module with child routes |
| meeting-not-found.component.ts | New component for handling meeting not found scenarios |
| meeting-not-found.component.html | Template for meeting not found error page |
| meeting-join.component.ts | Refactored meeting component with enhanced error handling and navigation |
| meeting-join.component.html | Updated template with recurrence summary tooltip |
| app.routes.ts | Updated main routing to use lazy-loaded meeting routes |
Comments suppressed due to low confidence (1)
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.html:1
- Missing space between CSS classes. Should be 'sm:items-center flex-col' instead of 'sm:items-centerflex-col'.
<!-- Copyright The Linux Foundation and each contributor to LFX. -->
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Asitha de Silva <[email protected]>
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts (3)
208-211: Guard against undefined meeting in computed return URL.During initial load and on error paths,
this.meeting()is undefined. Accessing.uidwill throw.Proposed change (outside the changed hunk):
private initializeReturnTo(): Signal<string | undefined> { return computed(() => { const uid = this.meeting()?.uid; const pwd = this.password(); if (!uid) return undefined; return pwd ? `${environment.urls.home}/meetings/${uid}?password=${pwd}` : `${environment.urls.home}/meetings/${uid}`; }); }
222-224: Respect explicit 0-minute early join window.
|| 10converts a valid0to 10. Use nullish coalescing.- const earlyJoinMinutes = meeting.early_join_time_minutes || 10; // Default to 10 minutes + const earlyJoinMinutes = meeting.early_join_time_minutes ?? 10; // Default to 10 minutes
96-104: Pass the provided password signal to the join-url API.
this.meeting().passwordmay be absent or sanitized by the API. Use the capturedpasswordquery param.- .getPublicMeetingJoinUrl(this.meeting().uid, this.meeting().password, { + .getPublicMeetingJoinUrl(this.meeting().uid, this.password(), { email: this.authenticated() ? this.user()?.email : this.joinForm.get('email')?.value, })
🧹 Nitpick comments (7)
apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.html (2)
19-23: Align data-testid to naming convention.Follow
[section]-[component]-[element]. Suggest updates below.Apply:
- <h1 ... data-testid="error-title">Meeting Not Found</h1> + <h1 ... data-testid="meeting-not-found-error-title">Meeting Not Found</h1> ... - <p ... data-testid="error-description"> + <p ... data-testid="meeting-not-found-error-description"> ... - <lfx-button routerLink="/" ... data-testid="go-home-button"> + <lfx-button routerLink="/" ... data-testid="meeting-not-found-go-home-button"> ... - <lfx-button + <lfx-button [href]="supportUrl" ... - data-testid="contact-support-button" + data-testid="meeting-not-found-contact-support-button"Also applies to: 49-66
12-15: Mark decorative icons as aria-hidden.Improves a11y for screen readers.
Apply, e.g.:
-<i class="fa-light fa-calendar-xmark ..."></i> +<i class="fa-light fa-calendar-xmark ..." aria-hidden="true"></i>(Repeat for other purely decorative elements.)
Also applies to: 29-44, 51-53, 61-63
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.ts (1)
6-15: Add default redirect for bare /meetings.Optional UX: redirect empty path to not-found (or another desired default).
Apply:
export const MEETING_ROUTES: Routes = [ + { + path: '', + pathMatch: 'full', + redirectTo: 'not-found', + }, { path: 'not-found', loadComponent: () => import('./meeting-not-found/meeting-not-found.component').then((m) => m.MeetingNotFoundComponent), }, { path: ':id', loadComponent: () => import('./meeting-join/meeting-join.component').then((m) => m.MeetingJoinComponent), }, ];apps/lfx-pcc/src/app/app.routes.ts (1)
15-17: LGTM: switch to lazy-loaded meeting routes.Matches the refactor and supports nested not-found/id routes. Consider preloading if meeting nav is frequent.
Optionally:
{ path: 'meetings', - loadChildren: () => import('./modules/meeting/meeting.routes').then((m) => m.MEETING_ROUTES), + loadChildren: () => import('./modules/meeting/meeting.routes').then((m) => m.MEETING_ROUTES), + data: { preload: true, preloadDelay: 500 }, },apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.ts (1)
18-19: Prefer nullish coalescing over logical OR for fallback.Use
??so a non-null empty string (if ever configured) doesn’t trigger the mailto fallback.- public readonly supportUrl = environment.urls.support || 'mailto:[email protected]'; + public readonly supportUrl = environment.urls.support ?? 'mailto:[email protected]';apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts (2)
260-265: Avoid deprecated unescape() for UTF‑8 base64.
unescapeis deprecated andbtoabreaks on non‑ASCII. Use TextEncoder.Proposed helper (outside the changed hunk):
function toBase64Utf8(s: string): string { const bytes = new TextEncoder().encode(s); let bin = ''; for (const b of bytes) bin += String.fromCharCode(b); return btoa(bin); }And here:
- const encodedName = btoa(unescape(encodeURIComponent(displayName))); + const encodedName = toBase64Utf8(displayName);
107-109: More resilient error detail for join failures.
error.errormay not exist or may be an object. Prefer message fallback.- this.messageService.add({ severity: 'error', summary: 'Error', detail: error.error }); + const detail = typeof error?.error === 'string' ? error.error + : error?.message ?? 'Unable to join meeting. Please try again.'; + this.messageService.add({ severity: 'error', summary: 'Error', detail });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/lfx-pcc/src/app/app.routes.ts(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.html(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts(5 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.html(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting.routes.ts(1 hunks)apps/lfx-pcc/src/app/shared/components/header/header.component.ts(0 hunks)apps/lfx-pcc/src/environments/environment.dev.ts(1 hunks)apps/lfx-pcc/src/environments/environment.prod.ts(1 hunks)apps/lfx-pcc/src/environments/environment.staging.ts(1 hunks)apps/lfx-pcc/src/environments/environment.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/lfx-pcc/src/app/shared/components/header/header.component.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.tsapps/lfx-pcc/src/environments/environment.staging.tsapps/lfx-pcc/src/environments/environment.dev.tsapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.tsapps/lfx-pcc/src/environments/environment.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/environments/environment.prod.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.tsapps/lfx-pcc/src/environments/environment.staging.tsapps/lfx-pcc/src/environments/environment.dev.tsapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.tsapps/lfx-pcc/src/environments/environment.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/environments/environment.prod.ts
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.htmlapps/lfx-pcc/src/environments/environment.staging.tsapps/lfx-pcc/src/environments/environment.dev.tsapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.tsapps/lfx-pcc/src/environments/environment.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/environments/environment.prod.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.htmlapps/lfx-pcc/src/environments/environment.staging.tsapps/lfx-pcc/src/environments/environment.dev.tsapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.tsapps/lfx-pcc/src/environments/environment.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/environments/environment.prod.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.routes.tsapps/lfx-pcc/src/environments/environment.staging.tsapps/lfx-pcc/src/environments/environment.dev.tsapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.tsapps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.tsapps/lfx-pcc/src/environments/environment.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/environments/environment.prod.ts
apps/lfx-pcc/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.html: Add data-testid attributes to new components for reliable test targeting
Follow data-testid naming convention: [section]-[component]-[element]
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.html
🧠 Learnings (1)
📚 Learning: 2025-09-05T18:09:48.792Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.792Z
Learning: Applies to apps/lfx-pcc/src/**/*.{ts,html} : Use LFX wrapper components instead of importing PrimeNG components directly
Applied to files:
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts
🧬 Code graph analysis (2)
apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.ts (4)
apps/lfx-pcc/src/environments/environment.dev.ts (1)
environment(4-10)apps/lfx-pcc/src/environments/environment.prod.ts (1)
environment(4-10)apps/lfx-pcc/src/environments/environment.staging.ts (1)
environment(4-10)apps/lfx-pcc/src/environments/environment.ts (1)
environment(4-10)
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts (2)
apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.ts (1)
Component(11-19)packages/shared/src/interfaces/meeting.interface.ts (1)
Meeting(75-144)
⏰ 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: E2E Tests / Playwright E2E Tests
🔇 Additional comments (9)
apps/lfx-pcc/src/environments/environment.dev.ts (1)
8-9: LGTM: dev env now exposes urls.support.apps/lfx-pcc/src/environments/environment.prod.ts (1)
8-9: LGTM: prod env now exposes urls.support.apps/lfx-pcc/src/app/modules/meeting/meeting.routes.ts (1)
7-15: LGTM: lazy-loaded child routes with correct ordering.
not-foundbefore:idavoids accidental capture by the param route.apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.html (1)
39-43: No LFX tooltip wrapper found — add wrapper or keep pTooltip; standardize data-testid
- Verification: repo contains many direct PrimeNG pTooltip usages; no lfxTooltip directive was found (examples: apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.html:41, apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html:33, apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-details/meeting-details.component.html:262).
- Action: either (A) add an LFX tooltip wrapper (e.g. [lfxTooltip]) and replace pTooltip usages, or (B) keep pTooltip but rename the test id in apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.html from recurring-badge to meeting-join-recurring-badge to match naming convention.
apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.ts (3)
11-16: Component wiring looks solid.Selector, standalone setup, and LFX wrapper components usage are consistent with the module refactor.
1-2: License header present and correct.SPDX + copyright line adhere to guidelines.
1-19: Verify environments consistently expose urls.support and remove legacy urls.profile.Found apps/lfx-pcc/src/environments/environment.ts with support: 'https://jira.linuxfoundation.org/plugins/servlet/desk' and no 'profile' key in that file; repo-wide scan in the sandbox failed (shell errors). Run a repo-wide search for 'urls.profile' across all environment*.ts files and confirm removal from staging.
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts (2)
11-12: Recurrence badge/pipe integration looks good.Pipe import + tooltip wiring align with the UI requirements.
Also applies to: 38-39
181-193: Badge mapping OK.Handles null types and provides sensible defaults; safe against casing.
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/meeting/meeting-join/meeting-join.component.ts
Show resolved
Hide resolved
apps/lfx-pcc/src/app/modules/meeting/meeting-not-found/meeting-not-found.component.html
Show resolved
Hide resolved
🧹 Deployment RemovedThe deployment for PR #86 has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-pcc/src/environments/environment.staging.ts (1)
7-8: Optional: type the environment shape to prevent drift across env files.Define a shared Environment interface so keys like
urls.home/urls.supportare consistently present or marked optional.Example:
// apps/lfx-pcc/src/environments/environment.model.ts export interface Environment { production: boolean; urls: { support: string; home?: string; }; }// apps/lfx-pcc/src/environments/environment.staging.ts import type { Environment } from './environment.model'; export const environment: Environment = { production: true, urls: { home: 'https://app.staging.lfx.dev', support: 'https://jira.linuxfoundation.org/plugins/servlet/desk', }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/lfx-pcc/src/environments/environment.staging.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/environments/environment.staging.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/environments/environment.staging.ts
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/environments/environment.staging.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/environments/environment.staging.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/environments/environment.staging.ts
⏰ 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: E2E Tests / Playwright E2E Tests
🔇 Additional comments (1)
apps/lfx-pcc/src/environments/environment.staging.ts (1)
7-8: Staging env URLs updated — LGTM; verify cross‑env usage ofurls.home.
- Good:
urls.supportpresent in apps/lfx-pcc/src/environments/{environment.ts, environment.staging.ts, environment.prod.ts, environment.dev.ts}.- Good:
urls.profileremoved from env files (no matches).- Action: consumers of
environment.urls.homecouldn't be verified (rg reported "No files were searched"); confirm by running: rg -nP '\benvironment.urls.home\b' -g '!apps/lfx-pcc/src/environments/**' -S or search forenvironment\.urls/urls\.homeusages.
Summary
This PR refactors the meeting module for improved modularity and error handling, and updates environment configuration to provide better support access.
Key Changes
Meeting Module Refactoring (LFXV2-487)
meeting.component.tswith modular structure:meeting-join/meeting-join.component.ts- Handles meeting join functionality with authentication supportmeeting-not-found/meeting-not-found.component.ts- Dedicated error page for invalid/missing meetingsmeeting.routes.ts- Child routes configuration for meeting moduleEnvironment Configuration Updates (LFXV2-488)
profileURL withsupportURL across all environmentsTechnical Details
New File Structure
Features Implemented
Testing
✅ Build successful (13.6s)
✅ Linting passed
✅ Type checking passed
✅ All existing functionality preserved
✅ New functionality validated
✅ Proper license headers included
JIRA Tickets
Review Checklist
Generated with Claude Code