fix(ui): early join time parse int value #88
Conversation
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. WalkthroughPrepared meeting payload now parses Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as MeetingCardComponent
participant Env as environment.urls.home
participant CB as Clipboard
participant Msg as MessageService
User->>UI: Click "Copy Meeting"
UI->>Env: build URL (home + meeting.uid)
UI->>CB: writeText(meetingUrl)
CB-->>UI: success
UI->>Msg: show success toast("Meeting Link Copied", "The meeting link has been copied to your clipboard")
Msg-->>User: visual feedback
sequenceDiagram
participant Caller
participant Comp as MeetingManageComponent
participant Parser as parseInt
Caller->>Comp: submit form (formValue)
Comp->>Parser: parseInt(formValue.early_join_time_minutes, 10)
alt result is NaN
Parser-->>Comp: NaN
Comp->>Comp: use DEFAULT_EARLY_JOIN_TIME
else numeric
Parser-->>Comp: number (including 0)
Comp->>Comp: set early_join_time_minutes to number
end
Comp->>Caller: send prepared meeting payload (numeric early_join_time_minutes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 fixes a potential type conversion issue in the MeetingManageComponent by ensuring the early_join_time_minutes value is properly parsed as an integer before applying the default value.
- Adds
parseInt()to convert the form input value to a number before fallback logic
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...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
Signed-off-by: Asitha de Silva <asithade@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
542-542: Preserve 0 when patching form defaults.Using
||here will replace a valid 0 with the default. Prefer nullish coalescing.Apply:
- early_join_time_minutes: meeting.early_join_time_minutes || DEFAULT_EARLY_JOIN_TIME, + early_join_time_minutes: meeting.early_join_time_minutes ?? DEFAULT_EARLY_JOIN_TIME,
📜 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-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
packages/shared/src/constants/meeting.constants.ts (1)
DEFAULT_EARLY_JOIN_TIME(135-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (1)
319-319: Avoid coalescing 0 — parse with radix and explicit NaN checkparseInt(...) || DEFAULT_EARLY_JOIN_TIME converts a valid 0 to the default and omits the radix; use Number.parseInt(..., 10) and Number.isNaN to preserve 0. Confirm whether 0 minutes is a valid value per product spec. If 0 is valid, replace with:
@@ - return { + const parsedEarlyJoin = Number.parseInt(String(formValue.early_join_time_minutes), 10); + const earlyJoinTimeMinutes = Number.isNaN(parsedEarlyJoin) ? DEFAULT_EARLY_JOIN_TIME : parsedEarlyJoin; + return { @@ - early_join_time_minutes: parseInt(formValue.early_join_time_minutes) || DEFAULT_EARLY_JOIN_TIME, + early_join_time_minutes: earlyJoinTimeMinutes,
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
205-214: Harden link building and handle copy failures.
- Guard when
uidis missing.- Use URL-join to avoid double slashes from
environment.urls.home.- Check
clipboard.copyresult and show a fallback/warn toast.Apply this diff:
- public copyMeetingLink(): void { - const meetingLink = environment.urls.home + '/meetings/' + this.meeting().uid; - this.clipboard.copy(meetingLink); - this.messageService.add({ - severity: 'success', - summary: 'Meeting Link Copied', - detail: 'The meeting link has been copied to your clipboard', - }); - } + public copyMeetingLink(): void { + const uid = this.meeting()?.uid; + if (!uid) { + this.messageService.add({ + severity: 'warn', + summary: 'Meeting Unavailable', + detail: 'Meeting link is not available yet.', + }); + return; + } + const base = (environment as any)?.urls?.home || window.location.origin; + const meetingLink = new URL(`/meetings/${uid}`, base).toString(); + const copied = this.clipboard.copy(meetingLink); + this.messageService.add( + copied + ? { + severity: 'success', + summary: 'Meeting Link Copied', + detail: 'The meeting link has been copied to your clipboard', + } + : { + severity: 'warn', + summary: 'Copy Failed', + detail: `Could not copy to clipboard. Please copy manually: ${meetingLink}`, + } + ); + }Also, if product expects the copied link to include a join password for private meetings, confirm requirements before appending query params to avoid leaking secrets.
📜 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 (2)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
🔇 Additional comments (4)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html (1)
48-49: Click handler hookup looks good.Button already has a stable data-testid; no a11y or guideline concerns here.
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (3)
4-4: Correct dependency import for clipboard.Good use of CDK Clipboard.
51-51: Module registered correctly.Adding
ClipboardModuleto the standalone imports is correct.
62-62: Injection pattern is correct.
inject(Clipboard)is appropriate in a standalone component.
| import { ButtonComponent } from '@components/button/button.component'; | ||
| import { ExpandableTextComponent } from '@components/expandable-text/expandable-text.component'; | ||
| import { MenuComponent } from '@components/menu/menu.component'; | ||
| import { environment } from '@environments/environment'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify environment variable shape across builds.
Ensure all env files define urls.home to avoid runtime undefined when composing the link.
🏁 Script executed:
#!/bin/bash
# Verify that all environment*.ts files define urls.home
fd -t f -i 'environment*.ts' | xargs -I {} sh -c 'echo "--- {} ---"; rg -n -C2 "urls\\s*:\\s*\\{[^}]*home" "{}" || echo "urls.home NOT FOUND"'Length of output: 72
Missing environment.urls.home — add to env files or guard usage.
apps/lfx-one/src/environments/environment.ts does not define urls.home; add a urls: { home: '…' } entry (and ensure any other build-specific environment*.ts include the same key) or guard access where it's used to prevent runtime undefined.
🤖 Prompt for AI Agents
In
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
around line 17, the code imports environment but assumes environment.urls.home
exists; update all environment files to include a urls object with a home key
(e.g., urls: { home: '<home-url>' }) or, alternatively, guard usage in this
component by checking environment?.urls?.home before using it and provide a safe
fallback (e.g., empty string or configured default) so runtime errors are
avoided.
🧹 Deployment RemovedThe deployment for PR #88 has been removed. |
This pull request makes a small but important update to the
MeetingManageComponentto ensure that theearly_join_time_minutesvalue is always treated as a number. The change parses the input as an integer before applying the default value, which helps prevent potential bugs from string inputs.early_join_time_minutesis parsed as an integer when creating or updating a meeting (meeting-manage.component.ts).https://linuxfoundation.atlassian.net/browse/LFXV2-510