feat(meetings): implement time-based join control and user info #81
Conversation
- Add frontend validation to disable join button based on early_join_time_minutes - Implement server-side time validation for meeting join URL access - Append user information (name, organization) as query parameters to join URLs - Make form reactive to update join URL parameters in real-time LFXV2-460, LFXV2-461, LFXV2-462, LFXV2-463 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
|
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. WalkthroughAdds client-side join-window gating and join-URL augmentation with user/form params, updates the meeting UI to reflect join eligibility, and enforces join-window checks plus registrant counting on the public meeting server endpoints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as MeetingComponent (Client)
participant S as PublicMeetingController (Server)
participant MS as MeetingService
Note over UI: init signals: canJoinMeeting, joinUrlWithParams
UI->>UI: compute canJoinMeeting(start_time, early_join_time_minutes)
UI->>UI: build joinUrlWithParams(join_url, user/form)
U->>UI: Click "Join"
alt canJoinMeeting == true
alt meeting.join_url exists
UI->>U: Open joinUrlWithParams()
else no join_url
UI->>S: POST /public/meetings/:id/join-url
S->>S: isWithinJoinWindow()
alt within window
S->>MS: getMeetingJoinUrl(...)
MS-->>S: join_url
S-->>UI: join_url
UI->>U: Open augmented URL
else outside window
S-->>UI: 400 ValidationError (timing)
UI->>U: Show early-join message
end
end
else canJoinMeeting == false
UI->>U: Disable button / show early-join message
end
sequenceDiagram
autonumber
participant S as PublicMeetingController
participant MS as MeetingService
participant L as Logger
Note over S: getMeetingById flow
S->>MS: getMeetingRegistrants(meeting.uid)
MS-->>S: registrants[]
S->>S: compute committee_members_count, individual_registrants_count
alt meeting.visibility == PUBLIC && not restricted && isWithinJoinWindow()
S->>MS: handleJoinUrlForPublicMeeting(...)
MS-->>S: join_url
else
S->>L: log skipping join_url fetch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 implements comprehensive time-based meeting join control and user information tracking to improve the meeting experience. The implementation includes frontend validation that prevents early joining, backend validation for security, and enhanced URL generation with user parameters.
Key changes include:
- Added time-based access control that validates users can only join within the allowed timeframe
- Implemented reactive form handling for real-time URL parameter updates
- Enhanced join URLs with user information query parameters
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| public-meeting.controller.ts | Added time validation logic and helper methods for join window checking |
| meeting.component.ts | Implemented reactive signals for join validation and dynamic URL generation with user parameters |
| meeting.component.html | Updated UI to show timing restrictions and use new computed signals for join functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
✅ 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: 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-pcc/src/server/controllers/public-meeting.controller.ts (1)
185-199: Don’t require a password when the meeting has no password.Current logic rejects missing
passwordeven ifmeeting.passwordis empty/undefined.- private validateMeetingPassword(password: string, meetingPassword: string, operation: string, req: Request, next: NextFunction, startTime: number): boolean { - if (!password || !validatePassword(password, meetingPassword)) { - Logger.error(req, operation, startTime, new Error('Invalid password parameter')); - const validationError = ServiceValidationError.forField('password', 'Invalid password', { - operation, - service: 'public_meeting_controller', - path: req.path, - }); - next(validationError); - return false; - } - return true; - } + private validateMeetingPassword( + password: string | undefined, + meetingPassword: string | undefined, + operation: string, + req: Request, + next: NextFunction, + startTime: number + ): boolean { + // No password set on meeting -> nothing to validate + if (!meetingPassword) return true; + // Meeting requires a password but none provided + if (!password) { + Logger.validation(req, operation, [{ field: 'password', message: 'Password is required' }], { path: req.path }); + next(ServiceValidationError.forField('password', 'Password is required', { + operation, service: 'public_meeting_controller', path: req.path, + })); + return false; + } + // Provided but incorrect + if (!validatePassword(password, meetingPassword)) { + Logger.validation(req, operation, [{ field: 'password', message: 'Invalid password' }], { path: req.path }); + next(ServiceValidationError.forField('password', 'Invalid password', { + operation, service: 'public_meeting_controller', path: req.path, + })); + return false; + } + return true; + }
🧹 Nitpick comments (7)
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts (1)
119-133: Use validation logging for expected early-join rejections.This path represents a user-timing validation, not a system error. Prefer
Logger.validation(...)to reduce error-noise; keep metadata consistent.- Logger.error(req, 'post_meeting_join_url', startTime, new Error('Meeting join not available yet')); + Logger.validation(req, 'post_meeting_join_url', [ + { field: 'timing', message: `Join not available until ${earlyJoinMinutes} minutes before start` } + ], { meeting_uid: id })apps/lfx-pcc/src/app/modules/meeting/meeting.component.ts (4)
20-22: Follow LFX wrapper policy for PrimeNG and drop unused Tooltip import.Per guidelines, use LFX wrapper components instead of importing PrimeNG directly; also
TooltipModuleisn’t used.-import { MessageService } from 'primeng/api'; -import { ToastModule } from 'primeng/toast'; -import { TooltipModule } from 'primeng/tooltip'; +import { MessageService } from 'primeng/api'; // if there is an LFX wrapper for toasts/tooltips, prefer that +// import { LfxToastModule } from '@lfx/wrappers/toast'; // example, if available +// Remove if not used: +// import { TooltipModule } from 'primeng/tooltip';
104-106: Surface server validation message for early-join rejections.Backend returns a timing validation; show that message when available.
- this.messageService.add({ severity: 'error', summary: 'Error', detail: error.error }); + const detail = + error?.validation_errors?.[0]?.message ?? + error?.error ?? + 'Unable to join the meeting.'; + this.messageService.add({ severity: 'error', summary: 'Error', detail });
223-237: Consider centralizing early-join minutes for UI reuse.Expose a computed
earlyJoinMinutes = meeting()?.early_join_time_minutes || 10and reuse in the template to avoid duplicating the default logic.
254-282: Build URLs with the URL API; avoidunescapeand hardcoded?handling.
unescapeis deprecated; useTextEncoderto base64-encode Unicode safely.- Using
URLhandles existing query strings and fragments robustly.- Optional: guard
btoafor SSR.- private buildJoinUrlWithParams(joinUrl: string, formValues?: { name: string; email: string; organization: string }): string { + private buildJoinUrlWithParams(joinUrl: string, formValues?: { name: string; email: string; organization: string }): string { if (!joinUrl) { return joinUrl; } @@ - // Create base64 encoded version - const encodedName = btoa(unescape(encodeURIComponent(displayName))); + // Create base64 encoded version (Unicode-safe; SSR-safe fallback) + const toBase64 = (s: string) => { + try { + // Browser path + // eslint-disable-next-line no-undef + return btoa(String.fromCharCode(...new TextEncoder().encode(s))); + } catch { + try { + // Node/SSR path if Buffer exists + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const B: any = (globalThis as any).Buffer; + return B ? B.from(s, 'utf-8').toString('base64') : ''; + } catch { + return ''; + } + } + }; + const encodedName = toBase64(displayName); @@ - // Build query parameters - const queryParams = new HttpParams().set('uname', displayName).set('un', encodedName); - const queryString = queryParams.toString(); - - // Append to URL, handling existing query strings - if (joinUrl.includes('?')) { - return `${joinUrl}&${queryString}`; - } - return `${joinUrl}?${queryString}`; + // Build URL safely (handles existing query/fragments) + try { + const url = new URL(joinUrl, window.location.origin); + url.searchParams.set('uname', displayName); + url.searchParams.set('un', encodedName); + return url.toString(); + } catch { + // Fallback if URL constructor fails (unlikely for https links) + const qp = new HttpParams().set('uname', displayName).set('un', encodedName).toString(); + return joinUrl.includes('?') ? `${joinUrl}&${qp}` : `${joinUrl}?${qp}`; + } }apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (2)
184-199: Add test IDs to the logged-in Join button and status for reliable E2E targeting.Follow
[section]-[component]-[element]naming (e.g.,join-loggedin-button,join-loggedin-status).- <div + <div class="flex items-center justify-between p-3 rounded-lg" [ngClass]="{ 'bg-amber-50 border border-amber-200': !canJoinMeeting(), 'bg-blue-50 border border-blue-200': canJoinMeeting() }"> - <div class="flex items-center gap-3 text-sm" [ngClass]="{ 'text-blue-700': canJoinMeeting(), 'text-amber-700': !canJoinMeeting() }"> + <div class="flex items-center gap-3 text-sm" data-testid="join-loggedin-status" [ngClass]="{ 'text-blue-700': canJoinMeeting(), 'text-amber-700': !canJoinMeeting() }"> @@ - <lfx-button + <lfx-button + data-testid="join-loggedin-button" size="small" [href]="canJoinMeeting() ? joinUrlWithParams() : undefined" [disabled]="!canJoinMeeting()" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in"></lfx-button>
318-320: Use LFX toast wrapper instead of raw<p-toast>.Aligns with “use LFX wrapper components” guideline and centralizes theming/behavior.
If an
lfx-toast(or similar) exists in this repo, replace<p-toast>with the wrapper and drop direct PrimeNG module imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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-pcc/src/app/modules/meeting/meeting.component.html(6 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting.component.ts(5 hunks)apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts(4 hunks)
🧰 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/server/controllers/public-meeting.controller.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/server/controllers/public-meeting.controller.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.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/server/controllers/public-meeting.controller.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting.component.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/server/controllers/public-meeting.controller.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting.component.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/server/controllers/public-meeting.controller.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.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.component.html
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts (1)
apps/lfx-pcc/src/server/helpers/logger.ts (1)
Logger(10-129)
⏰ 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 (8)
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts (4)
61-67: Gating public join URL by the join window looks good.Keeps URLs hidden until allowed. Nice alignment with frontend gating.
236-251: Join-window helper LGTM.Consistent with frontend; defaulting to 10 minutes.
236-251: Confirm upper-bound semantics (post-start joins).
now >= earliestJoinTimeallows joining indefinitely after start. If joins should be limited to start+duration(+grace), add an upper bound.Would you like a patch that enforces
now <= endTime + graceMinutes?
41-44: Incorrect — getMeetingRegistrants already returns an array; no change requiredapps/lfx-pcc/src/server/services/meeting.service.ts (getMeetingRegistrants, ~line 213) extracts
resourcesand returnsresources.map(resource => resource.data), so the callers’ use of.filter/.lengthis valid.Likely an incorrect or invalid review comment.
apps/lfx-pcc/src/app/modules/meeting/meeting.component.ts (2)
81-90: Pre-check forcanJoinMeeting()is a good UX improvement.Prevents unnecessary network calls and shows clear feedback.
239-252: ReactivejoinUrlWithParamsdesign looks good.Correctly recomputes on
join_urlor form signal changes.apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (2)
110-114: Renaming to “Guests” and simplifying counts looks good.Clearer label and single “invited” metric.
134-135: Duration copy LGTM.Straightforward minutes display.
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (1)
319-319: Replace PrimeNG with the LFX toast wrapper (mandatory refactor)Project currently uses PrimeNG Toast directly and imports ToastModule; no LFX toast wrapper/service was found — migrate to an LFX toast wrapper (or add one) and remove direct PrimeNG Toast usage.
- Replace occurrences:
- apps/lfx-pcc/src/app/modules/meeting/meeting.component.html — line 319:
- apps/lfx-pcc/src/app/app.component.html — line 3:
- Update/remove related imports:
- apps/lfx-pcc/src/app/modules/meeting/meeting.component.ts — imports ToastModule / MessageService from primeng.
- apps/lfx-pcc/src/app/app.component.ts — imports ToastModule from primeng.
Suggested (placeholder) change:
- <p-toast></p-toast> + <lfx-toast></lfx-toast>If no lfx-toast exists, add an lfx-toast component/module that wraps the toast API and update modules/usages to import the wrapper instead of primeng's ToastModule.
♻️ Duplicate comments (3)
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (3)
110-110: Cosmetic rename should be out-of-scope for this PR.Changing “Attendees” to “Guests” is unrelated to time-based join control/user-info work. Please revert or move to a focused UI copy PR.
- <span>Guests</span> + <span>Attendees</span>
134-134: Keep duration formatting changes out of this feature PR.Switching “m” to “minutes” isn’t part of join control. Please revert or ship separately.
- <div class="text-sm font-sans">{{ meeting().duration }} minutes</div> + <div class="text-sm font-sans">{{ meeting().duration }}m</div>
277-307: Mirror test IDs on guest actions and status; fallback minutes already good.Add the same stable selectors used for the logged-in path.
- <div class="flex items-center" [ngClass]="{ 'justify-between': !canJoinMeeting(), 'justify-end': canJoinMeeting() }"> + <div class="flex items-center" data-testid="join-guest-actions" [ngClass]="{ 'justify-between': !canJoinMeeting(), 'justify-end': canJoinMeeting() }"> @if (!canJoinMeeting()) { - <div class="flex items-center gap-2 text-sm" [ngClass]="{ 'text-blue-700': canJoinMeeting(), 'text-amber-700': !canJoinMeeting() }"> + <div class="flex items-center gap-2 text-sm" data-testid="join-guest-status" [ngClass]="{ 'text-blue-700': canJoinMeeting(), 'text-amber-700': !canJoinMeeting() }"> <i class="fa-light fa-clock text-amber-600"></i> @@ - <div class="flex items-center"> + <div class="flex items-center"> @if (meeting().join_url) { - <lfx-button + <lfx-button + data-testid="join-guest-button" size="small" [href]="joinForm.invalid || !canJoinMeeting() ? undefined : joinUrlWithParams()" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in" [disabled]="joinForm.invalid || !canJoinMeeting()"></lfx-button> } @else { - <lfx-button + <lfx-button + data-testid="join-guest-button" size="small" severity="primary" label="Join Meeting" [disabled]="joinForm.invalid || !canJoinMeeting()" icon="fa-light fa-sign-in" (click)="onJoinMeeting()"></lfx-button> }
🧹 Nitpick comments (4)
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (4)
148-164: Add test IDs to the logged-in user card.New UI block should expose stable selectors for tests.
- <div class="bg-blue-50 p-3 rounded-lg"> + <div class="bg-blue-50 p-3 rounded-lg" data-testid="join-auth-user-card"> <div class="flex items-start justify-between"> <div class="flex items-center gap-3"> - <div class="w-8 h-8 bg-blue-400 rounded-full flex items-center justify-center"> + <div class="w-8 h-8 bg-blue-400 rounded-full flex items-center justify-center" data-testid="join-auth-avatar"> <i class="fa-light fa-user text-white text-xs"></i> </div>
166-181: Expose join-status for tests; keep the minutes fallback.Add test IDs so E2E can assert gating states.
- <div - class="flex items-center justify-between p-3 rounded-lg" + <div + class="flex items-center justify-between p-3 rounded-lg" + data-testid="join-auth-status" [ngClass]="{ 'bg-amber-50 border border-amber-200': !canJoinMeeting(), 'bg-blue-50 border border-blue-200': canJoinMeeting() }"> <div class="flex items-center gap-3 text-sm" [ngClass]="{ 'text-blue-700': canJoinMeeting(), 'text-amber-700': !canJoinMeeting() }"> @if (canJoinMeeting()) { <i class="fa-light fa-check-circle"></i> - <span class="font-sans">Ready to join as {{ user.name }}</span> + <span class="font-sans" data-testid="join-auth-status-text">Ready to join as {{ user.name }}</span> } @else {
184-199: Add test IDs to the logged-in join buttons.Provide stable selectors regardless of join_url presence.
- <lfx-button + <lfx-button + data-testid="join-auth-button" size="small" [href]="canJoinMeeting() ? joinUrlWithParams() : undefined" [disabled]="!canJoinMeeting()" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in"></lfx-button> @@ - <lfx-button + <lfx-button + data-testid="join-auth-button" size="small" severity="primary" label="Join Meeting" [disabled]="!canJoinMeeting()" icon="fa-light fa-sign-in" (click)="onJoinMeeting()"></lfx-button>
218-229: Add test IDs to the sign-in CTA block.Helpful for end-to-end flows that cover auth redirect.
- <div class="bg-blue-50 border border-blue-200 p-3 rounded-lg"> + <div class="bg-blue-50 border border-blue-200 p-3 rounded-lg" data-testid="join-signin-card"> @@ - <a + <a + data-testid="join-signin-button" size="small" class="inline-flex items-center gap-1 px-2 py-1 bg-gray-100 hover:bg-gray-200 text-xs rounded transition-colors" [href]="'/login?returnTo=' + returnTo()">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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/app/modules/meeting/meeting.component.html(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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.component.html
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.component.html
**/*.{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.component.html
⏰ 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
🧹 Deployment RemovedThe deployment for PR #81 has been removed. |
Summary
This PR implements comprehensive meeting join functionality improvements with time-based access control and user information tracking:
✅ Features Implemented
Time-Based Access Control (LFXV2-460)
early_join_time_minutesbefore start timecanJoinMeetingcomputed signal handles timing logicBackend Join URL Validation (LFXV2-461)
isWithinJoinWindowhelper methodUser Information Query Parameters (LFXV2-462)
uname(display name) andun(base64 encoded) to join URLsReactive Form Updates (LFXV2-463)
🔧 Technical Changes
Frontend (
meeting.component.ts&.html)canJoinMeetingcomputed signal for time validationjoinUrlWithParamscomputed signal for dynamic URL generationformValuessignal for reactive form trackingbuildJoinUrlWithParamsmethod with HttpParamsBackend (
public-meeting.controller.ts)isWithinJoinWindowhelper methodgetMeetingByIdto conditionally fetch join URLpostMeetingJoinUrlto validate timing and return appropriate errors📊 Impact
🎯 Related JIRA Tickets
🤖 Generated with Claude Code