feat(permissions): implement role-based permission system #72
Conversation
- Add AccessCheckService for backend permission validation - Integrate permission checks in project, committee, and meeting services - Add writer/organizer fields to shared interfaces - Implement conditional UI rendering based on user permissions - Hide action buttons for users without write access - Add Zoom logo for meeting platform display Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
|
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. WalkthroughIntroduces access-check capability via a new server-side AccessCheckService and integrates it into project, meeting, and committee services to annotate resources with per-user access (writer/organizer). Updates shared interfaces, several UI templates to conditionally render actions based on access, meeting stepper indexing (0→1-based), registrant type field, CI secret handling, docs, and minor styles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Client (Browser/API)
participant Server as PCC Server Service (Project/Meeting/Committee)
participant ACS as AccessCheckService
participant Proxy as MicroserviceProxyService
participant LFX as LFX_V2_SERVICE (/access-check)
User->>Server: GET resources (e.g., meetings)
Server->>Server: Fetch resources from data source
Server->>ACS: addAccessToResources(req, resources, type, accessType)
ACS->>ACS: Build requests payload
ACS->>Proxy: POST /access-check { requests }
Proxy->>LFX: Forward request
LFX-->>Proxy: { results: [...] }
Proxy-->>ACS: API response
ACS->>ACS: Parse results → Map<id, hasAccess>
ACS-->>Server: Resources annotated (e.g., writer/organizer flags)
Server-->>User: Enriched resources
note over Server,User: On API error, returns resources with access=false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive role-based permission system for projects, committees, and meetings with conditional UI rendering based on user access levels. The implementation adds permission fields to interfaces, creates a centralized AccessCheckService for validation, and updates UI components to conditionally show/hide action buttons based on user permissions.
- Adds
writerfield to Project and Committee interfaces,organizerfield to Meeting interface - Implements new AccessCheckService using LFX V2 service
/access-checkendpoint for permission validation - Updates UI components to conditionally render action buttons based on user permissions
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/shared/src/interfaces/* | Added permission fields to Project, Committee, Meeting interfaces and new AccessCheck interface |
| apps/lfx-pcc/src/server/services/* | Added AccessCheckService and integrated permission checks into existing services |
| apps/lfx-pcc/src/app/modules/project/* | Updated UI components to conditionally render based on permission fields |
| apps/lfx-pcc/src/app/shared/services/meeting.service.ts | Removed debug logging statement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...x-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
Show resolved
Hide resolved
...pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html
Show resolved
Hide resolved
⏭️ E2E Tests SkippedBrowser: chromium E2E tests were skipped (no test credentials provided). Test Configuration
|
Signed-off-by: Asitha de Silva <[email protected]>
🧹 Deployment RemovedThe deployment for PR #72 has been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/project/dashboard/project-dashboard/project.component.ts (1)
540-571: Make Quick Actions menu reactive
Replace the static array with a computed signal and update the template accordingly:--- a/apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts +++ b/apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts @@ - // Menu items for quick actions - public readonly quickActionMenuItems: MenuItem[] = this.initializeQuickActionMenuItems(); + // Menu items for quick actions (reactive to project changes) + public readonly quickActionMenuItems = computed<MenuItem[]>(() => + this.initializeQuickActionMenuItems() + );--- a/apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.html +++ b/apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.html @@ - <lfx-menu [model]="quickActionMenuItems" styleClass="w-full border-0 p-0"></lfx-menu> + <lfx-menu [model]="quickActionMenuItems()" styleClass="w-full border-0 p-0"></lfx-menu>After applying, verify that toggling a user’s writer permission adds/removes the “Create Meeting” and “Create Committee” items without a full page reload.
apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts (1)
258-281: 'Public Calendar' menu item has no actionThe item is inert (no routerLink/command). Provide a command to switch to Calendar view and set visibility to public.
items.push( { label: this.meetingListView() === 'past' ? 'Upcoming Meetings' : 'Meeting History', icon: 'fa-light fa-calendar-days text-sm', command: () => this.onMeetingListViewChange(this.meetingListView() === 'upcoming' ? 'past' : 'upcoming'), }, { label: 'Public Calendar', icon: 'fa-light fa-calendar-check text-sm', - } + command: () => { + this.onVisibilityChange('public'); + this.currentView.set('calendar'); + } + } );apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
335-347: Avoid trailing separator when user isn’t organizerWhen not organizer, menu shows “Join Meeting” followed by a dangling separator.
- baseItems.push({ - separator: true, - }); + if (this.meeting()?.organizer) { + baseItems.push({ separator: true }); + }
♻️ Duplicate comments (4)
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (1)
264-266: Duplicate label: remove inner text when using label attributeKeep a single source of truth for the button label.
- <lfx-button size="small" [href]="meeting().join_url" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in" - >Join Meeting</lfx-button - > + <lfx-button size="small" [href]="meeting().join_url" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in"> + </lfx-button>apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
28-76: Stepper/test-id/indicator are out of sync (1-based vs 0-based).
- If currentStep is 1-based, the header should not add 1. Also tests still use 0-based panel ids (panel-0..panel-4), which keeps the previous mismatch noted earlier.
Apply:
- <div class="text-sm text-slate-500" data-testid="meeting-manage-step-indicator">Step {{ currentStep() + 1 }} of {{ totalSteps }}</div> + <div class="text-sm text-slate-500" data-testid="meeting-manage-step-indicator">Step {{ currentStep() }} of {{ totalSteps }}</div>And either:
- keep test ids 0-based but document it in the TS (and tests), or
- switch panel test ids to 1-based to match the visible steps.
apps/lfx-pcc/src/server/services/access-check.service.ts (2)
137-162: Return type claims writer even when accessType is organizer/viewerThis mismatches call sites and weakens type safety. Use a generic flag keyed by accessType and align lookup with composite key.
- public async addAccessToResources<T extends { uid: string }>( + public async addAccessToResources<T extends { uid: string }, A extends AccessCheckAccessType = 'writer'>( req: Request, resources: T[], resourceType: AccessCheckResourceType, - accessType: AccessCheckAccessType = 'writer' - ): Promise<(T & { writer?: boolean })[]> { + accessType: A = 'writer' as A + ): Promise<Array<T & Record<A, boolean>>> { @@ - return resources.map((resource) => ({ - ...resource, - [accessType]: accessResults.get(resource.uid) || false, - })); + return resources.map((resource) => ({ + ...resource, + [accessType]: accessResults.get(`${resourceType}:${resource.uid}#${accessType}`) || false, + })) as Array<T & Record<A, boolean>>;Add near imports (outside this hunk) for clarity:
type AccessKey = `${AccessCheckResourceType}:${string}#${AccessCheckAccessType}`;
172-188: Same type issue for single-resource variant; also align keyMirror the generic return type and composite key.
- public async addAccessToResource<T extends { uid: string }>( + public async addAccessToResource<T extends { uid: string }, A extends AccessCheckAccessType = 'writer'>( req: Request, resource: T, resourceType: AccessCheckResourceType, - accessType: AccessCheckAccessType = 'writer' - ): Promise<T & { writer?: boolean }> { - const hasAccess = await this.checkSingleAccess(req, { + accessType: A = 'writer' as A + ): Promise<T & Record<A, boolean>> { + const hasAccess = await this.checkSingleAccess(req, { resource: resourceType, id: resource.uid, access: accessType, }); return { ...resource, - [accessType]: hasAccess, - }; + [accessType]: hasAccess, + } as T & Record<A, boolean>;
🧹 Nitpick comments (23)
apps/lfx-pcc/.env.example (1)
6-13: Clarify mixed Auth0/Authelia guidance near the renamed header.The header now mentions Auth0/Authelia, but the note still says “Get these values from your Auth0 dashboard.” Suggest clarifying Authelia-specific sourcing and preserving the PCC_AUTH0_* naming for compatibility.
Apply:
-# Auth0/Authelia Authentication Configuration -# Get these values from your Auth0 dashboard +# Auth0/Authelia Authentication Configuration +# For Auth0: get these from your tenant dashboard. +# For Authelia (local): see README “Local Development” for the default client (id: lfx) and how to retrieve the secret. +# Note: variable names use PCC_AUTH0_* for backward compatibility..github/workflows/e2e-tests.yml (1)
98-99: Good addition of M2M secret; also update messaging and harden validation step.
- Add the new AUTH secret to the “E2E tests skipped” guidance so users know to configure it.
- Consider set -euo pipefail and jq -e in “Validate required secrets” to fail-fast on parsing errors.
Suggested tweaks (no diff since outside this hunk):
- In “E2E tests skipped” step, add:
- /cloudops/managed-secrets/auth0/LFX_V2_Meeting_Join_M2M (AUTH M2M configuration)
- In the validation script header, prepend:
set -euo pipefail
and change jq reads to use: jq -re '...'README.md (3)
64-71: Strengthen local secret retrieval guidance and secret generation.
- The kubectl snippet prints client secret to stdout and may land in shell history. Add a caution and an example that writes to a temp file or uses a safer method.
- Provide a concrete command to generate PCC_AUTH0_SECRET.
Apply:
- - Local Development: The default client ID is `lfx` and you can get the client secret from the k8s via `k get secrets authelia-clients -n lfx -o jsonpath='{.data.lfx}' | base64 --decode` + - Local Development: Default client ID is `lfx`. Retrieve the client secret from Kubernetes: + - Caution: avoid leaking secrets via shell history/stdout. + - Example: + ```bash + kubectl get secret authelia-clients -n lfx -o jsonpath='{.data.lfx}' | base64 --decode > /tmp/lfx-client-secret && chmod 600 /tmp/lfx-client-secret + ``` @@ - - Set `PCC_AUTH0_SECRET` to a sufficiently long random string (32+ characters) - - Generate a random 32 characters long string + - Set `PCC_AUTH0_SECRET` to a strong random string (≥32 chars) + - Example: + ```bash + openssl rand -base64 48 # ~64 chars + ```
83-86: Update wording for LFX_V2_SERVICE description.“query service endpoint” may be outdated; the PR refers to the LFX V2 service. Suggest rewording to avoid confusion.
- - Set `LFX_V2_SERVICE` to your query service endpoint + - Set `LFX_V2_SERVICE` to your LFX V2 API gateway/service endpoint
98-100: Add an explicit production warning for NODE_TLS_REJECT_UNAUTHORIZED.It’s easy to copy/paste into non-dev environments. Add a do-not-use-in-prod note.
- - Set `NODE_TLS_REJECT_UNAUTHORIZED=0` when using Authelia for local authentication + - Set `NODE_TLS_REJECT_UNAUTHORIZED=0` when using Authelia for local authentication + - Warning: do not set this in production.packages/shared/src/interfaces/project.interface.ts (1)
63-64: Mark response-only permission as readonlyPrevents accidental mutation on the client.
- /** Write access permission for current user (response only) */ - writer?: boolean; + /** Write access permission for current user (response only) */ + readonly writer?: boolean;packages/shared/src/interfaces/meeting.interface.ts (1)
89-90: Mark response-only permission as readonlySame pattern as Project.writer.
- /** Write access permission for current user (response only) */ - organizer?: boolean; + /** Write access permission for current user (response only) */ + readonly organizer?: boolean;apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (1)
132-134: Nit: add intrinsic size and lazy decoding to avoid CLSMinor perf/accessibility improvement for the Zoom logo.
- <img src="/images/zoom-logo.svg" alt="Zoom" class="w-auto h-4 mt-2 object-contain" /> + <img src="/images/zoom-logo.svg" alt="Zoom" class="w-auto h-4 mt-2 object-contain" + height="16" loading="lazy" decoding="async" />apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
28-28: Prefer LFX wrappers over raw PrimeNG in app templates.p-stepper/p-tabs/p-confirmDialog appear directly. If LFX wrappers exist, switch to them for consistency with design system.
packages/shared/src/interfaces/index.ts (1)
49-50: Drop access-check barrel re-export and update consumers
Remove the line in packages/shared/src/interfaces/index.ts:export * from './access-check.interface';and change all imports to point directly at the file, for example:
import { AccessCheckRequest, AccessCheckApiRequest, AccessCheckApiResponse, AccessCheckResourceType, AccessCheckAccessType } from '@lfx-pcc/shared/interfaces/access-check.interface';This refactor touches many files in both the app and server layers—please update each import accordingly.
packages/shared/src/interfaces/committee.interface.ts (1)
15-16: LGTM—writer is response-only metadata; verified no create/update payload builders include it.
Optional: mark as readonly to discourage client mutation:- writer?: boolean; + readonly writer?: boolean;apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.html (1)
202-211: Good: “Create Meeting” CTA gated by project writer.Consider adding a data-testid for reliable targeting, e.g., data-testid="dashboard-create-meeting-cta".
- <lfx-button + <lfx-button + data-testid="dashboard-create-meeting-cta" label="Create Meeting" icon="fa-light fa-calendar-plus" severity="secondary" size="small" [routerLink]="['meetings/create']" [relativeTo]="activatedRoute">apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html (1)
56-74: Gating admin actions by organizer looks correct.Nice narrowing with project()?.slug && meeting().organizer before showing Edit and Actions.
Optionally also render the popup menu only when organizer to keep it out of the DOM for screen readers:
- <lfx-menu #actionMenu [model]="actionMenuItems()" [popup]="true" appendTo="body"></lfx-menu> + @if (meeting().organizer) { + <lfx-menu #actionMenu [model]="actionMenuItems()" [popup]="true" appendTo="body"></lfx-menu> + }apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html (1)
122-128: Good: “Create Meeting” gated in empty state.Add a data-testid to the CTA for tests.
- <lfx-button + <lfx-button + data-testid="meeting-dashboard-create-cta" label="Create Meeting" icon="fa-light fa-calendar-plus" severity="secondary" routerLink="/project/{{ project()?.slug }}/meetings/create"></lfx-button>apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.html (2)
10-12: Add data-testid and aria-label to the Add Member buttonHelps E2E reliability and accessibility per guidelines.
- <lfx-button label="Add Member" icon="fa-light fa-user-plus" size="small" severity="secondary" (onClick)="openAddMemberDialog()"> </lfx-button> + <lfx-button + label="Add Member" + icon="fa-light fa-user-plus" + size="small" + severity="secondary" + (onClick)="openAddMemberDialog()" + [attr.data-testid]="'committee-members-add'" + [attr.aria-label]="'Add Member'"> + </lfx-button>
167-177: Colspan should adapt to voting columnsWhen voting columns are enabled, header count changes. Use dynamic colspan to avoid layout issues.
- <td colspan="6" class="text-center py-8"> + <td [attr.colspan]="committee()?.enable_voting ? 7 : 5" class="text-center py-8">apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts (1)
168-178: Ensure meetings load when project arrives asynchronouslyCurrent initialization branches on this.project() at construction time; if project resolves later, streams may remain on of([]) until manual refresh.
Consider triggering refresh when project becomes available:
// Add once in constructor after setting this.project: effect(() => { if (this.project()?.uid) { this.refresh.next(); } });Or include project dependency in the observable chain (happy to draft a full patch if you prefer).
Also applies to: 181-191
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
82-86: Type the access-enrichment helpers to reflect 'organizer'AccessCheckService currently returns T & { writer?: boolean }, which under-represents the actual flag when accessType='organizer'. Adjust generics for accuracy.
Proposed change in apps/lfx-pcc/src/server/services/access-check.service.ts:
- public async addAccessToResources<T extends { uid: string }>( + public async addAccessToResources<T extends { uid: string }, K extends AccessCheckAccessType = 'writer'>( req: Request, resources: T[], resourceType: AccessCheckResourceType, - accessType: AccessCheckAccessType = 'writer' - ): Promise<(T & { writer?: boolean })[]> { + accessType: K = 'writer' as K + ): Promise<(T & Record<K, boolean>)[]> { … - return resources.map((resource) => ({ + return resources.map((resource) => ({ ...resource, - [accessType]: accessResults.get(resource.uid) || false, + [accessType]: accessResults.get(resource.uid) || false, })); }- public async addAccessToResource<T extends { uid: string }>( + public async addAccessToResource<T extends { uid: string }, K extends AccessCheckAccessType = 'writer'>( req: Request, resource: T, resourceType: AccessCheckResourceType, - accessType: AccessCheckAccessType = 'writer' - ): Promise<T & { writer?: boolean }> { + accessType: K = 'writer' as K + ): Promise<T & Record<K, boolean>> { … - return { + return { ...resource, - [accessType]: hasAccess, + [accessType]: hasAccess, }; }apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.ts (1)
14-20: Prefer LFX tooltip wrapper over direct PrimeNG importGuideline: use LFX wrapper components instead of PrimeNG directly in apps/lfx-pcc. If an LFX Tooltip exists, switch to it.
apps/lfx-pcc/src/server/services/committee.service.ts (1)
63-69: Minor: align error path with actual requestThe error metadata path points to
/committees/${committeeId}while the request hits/query/resources. Align for clearer logs.apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
312-334: Gate actions consistently; Add Guests by writer, Edit by organizer; guard missing slugKeeps Edit for organizers per spec, but lets writers manage registrants. Also avoid pushing Edit with undefined slug.
- if (!this.pastMeeting()) { - if (this.meeting()?.organizer) { - baseItems.push({ - label: 'Add Guests', - icon: 'fa-light fa-plus', - command: () => this.openAddRegistrantModal(), - }); - - baseItems.push({ - label: this.meeting().meeting_committees && this.meeting().meeting_committees!.length > 0 ? 'Manage Committees' : 'Connect Committees', - icon: 'fa-light fa-people-group', - command: () => this.openCommitteeModal(), - }); - - const projectSlug = this.project()?.slug; - baseItems.push({ - label: 'Edit', - icon: 'fa-light fa-edit', - routerLink: ['/project', projectSlug, 'meetings', this.meeting().uid, 'edit'], - }); - } + if (!this.pastMeeting()) { + const canManageRegistrants = !!(this.meeting()?.writer || this.meeting()?.organizer); + if (canManageRegistrants) { + baseItems.push({ + label: 'Add Guests', + icon: 'fa-light fa-plus', + command: () => this.openAddRegistrantModal(), + }); + } + if (this.meeting()?.organizer) { + baseItems.push({ + label: + this.meeting().meeting_committees && this.meeting().meeting_committees!.length > 0 + ? 'Manage Committees' + : 'Connect Committees', + icon: 'fa-light fa-people-group', + command: () => this.openCommitteeModal(), + }); + const projectSlug = this.project()?.slug; + if (projectSlug) { + baseItems.push({ + label: 'Edit', + icon: 'fa-light fa-edit', + routerLink: ['/project', projectSlug, 'meetings', this.meeting().uid, 'edit'], + }); + } + }apps/lfx-pcc/src/server/services/access-check.service.ts (2)
31-55: Add dedup, batching, and TTL caching to reduce loadBefore calling the API: deduplicate identical checks, batch large inputs (e.g., chunks of 100–200), and cache results per user + “type:id#access” with short TTL and invalidation hooks for write ops. This will improve latency and avoid hammering LFX V2.
88-97: Avoid logging usernames in access_detailsUsernames may be PII; consider dropping them or guarding behind a debug flag.
📜 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 ignored due to path filters (1)
apps/lfx-pcc/public/images/zoom-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (29)
.github/workflows/e2e-tests.yml(1 hunks)README.md(1 hunks)apps/lfx-pcc/.env.example(1 hunks)apps/lfx-pcc/src/app/app.component.scss(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting.component.html(2 hunks)apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.ts(0 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.html(2 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.html(2 hunks)apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.ts(4 hunks)apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts(2 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.html(2 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts(3 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html(4 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts(8 hunks)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.ts(1 hunks)apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.html(1 hunks)apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts(3 hunks)apps/lfx-pcc/src/app/shared/services/meeting.service.ts(1 hunks)apps/lfx-pcc/src/server/services/access-check.service.ts(1 hunks)apps/lfx-pcc/src/server/services/committee.service.ts(3 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(3 hunks)apps/lfx-pcc/src/server/services/project.service.ts(3 hunks)packages/shared/src/interfaces/access-check.interface.ts(1 hunks)packages/shared/src/interfaces/committee.interface.ts(1 hunks)packages/shared/src/interfaces/index.ts(1 hunks)packages/shared/src/interfaces/meeting.interface.ts(2 hunks)packages/shared/src/interfaces/project.interface.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
packages/shared/src/interfaces/committee.interface.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/app/shared/services/meeting.service.tsapps/lfx-pcc/src/server/services/access-check.service.tspackages/shared/src/interfaces/access-check.interface.tsapps/lfx-pcc/src/server/services/project.service.tspackages/shared/src/interfaces/meeting.interface.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-pcc/src/server/services/committee.service.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-pcc/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:
packages/shared/src/interfaces/committee.interface.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/app/shared/services/meeting.service.tsapps/lfx-pcc/src/server/services/access-check.service.tspackages/shared/src/interfaces/access-check.interface.tsapps/lfx-pcc/src/server/services/project.service.tspackages/shared/src/interfaces/meeting.interface.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-pcc/src/server/services/committee.service.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all shared interfaces in the shared package
Files:
packages/shared/src/interfaces/committee.interface.tspackages/shared/src/interfaces/index.tspackages/shared/src/interfaces/access-check.interface.tspackages/shared/src/interfaces/meeting.interface.tspackages/shared/src/interfaces/project.interface.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
packages/shared/src/interfaces/committee.interface.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.htmlpackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.htmlapps/lfx-pcc/src/app/app.component.scssapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.htmlapps/lfx-pcc/src/app/shared/services/meeting.service.tsapps/lfx-pcc/src/server/services/access-check.service.tspackages/shared/src/interfaces/access-check.interface.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.htmlapps/lfx-pcc/src/server/services/project.service.tspackages/shared/src/interfaces/meeting.interface.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-pcc/src/server/services/committee.service.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.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/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-members/committee-members.component.htmlapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.htmlapps/lfx-pcc/src/app/shared/services/meeting.service.tsapps/lfx-pcc/src/server/services/access-check.service.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.htmlapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/committee.service.tsapps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.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/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.tsapps/lfx-pcc/src/app/shared/services/meeting.service.tsapps/lfx-pcc/src/server/services/access-check.service.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/committee.service.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.tsapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.tsapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.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/project/committees/components/committee-members/committee-members.component.htmlapps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.htmlapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.htmlapps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.htmlapps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html
{apps/lfx-pcc/src,packages/shared/src}/**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid barrel exports; use direct imports instead of index.ts barrels
Files:
packages/shared/src/interfaces/index.ts
🧠 Learnings (2)
📚 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 packages/shared/src/interfaces/**/*.ts : Place all shared interfaces in the shared package
Applied to files:
packages/shared/src/interfaces/index.ts
📚 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/app.component.scss
🧬 Code graph analysis (5)
apps/lfx-pcc/src/server/services/access-check.service.ts (3)
apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
MicroserviceProxyService(11-102)packages/shared/src/interfaces/access-check.interface.ts (5)
AccessCheckRequest(7-14)AccessCheckApiRequest(19-22)AccessCheckApiResponse(27-30)AccessCheckResourceType(35-35)AccessCheckAccessType(36-36)apps/lfx-pcc/src/server/helpers/logger.ts (2)
Logger(10-129)error(52-65)
apps/lfx-pcc/src/server/services/project.service.ts (2)
apps/lfx-pcc/src/server/services/access-check.service.ts (1)
AccessCheckService(13-189)apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
MicroserviceProxyService(11-102)
apps/lfx-pcc/src/server/services/committee.service.ts (3)
apps/lfx-pcc/src/server/services/access-check.service.ts (1)
AccessCheckService(13-189)apps/lfx-pcc/src/server/services/etag.service.ts (1)
ETagService(13-135)apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
MicroserviceProxyService(11-102)
apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.ts (2)
apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts (1)
Component(27-572)apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (1)
Component(31-496)
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
apps/lfx-pcc/src/server/services/access-check.service.ts (1)
AccessCheckService(13-189)
⏰ 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 (24)
apps/lfx-pcc/src/app/app.component.scss (1)
106-112: LGTM – consistent p-menu icon sizingLooks good and scoped under ::ng-deep with Tailwind utility.
packages/shared/src/interfaces/meeting.interface.ts (1)
279-282: No internal usages of ‘individual’; confirm external consumer dependencies
Repo-wide search for'individual'literals and type checks returned no results. If no downstream clients expecttype: 'individual', it’s safe to remove; otherwise reintroduce it with a@deprecatedannotation and migration guidance.apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.ts (1)
155-155: No client logic checksregistrant.type === 'individual; only the tooltip inmeeting-card.component.html(line 348) shows “Individual.” Confirm backend defaults correctly when the payload omits thetypefield.apps/lfx-pcc/src/app/shared/services/meeting.service.ts (1)
103-111: LGTM – removed noisy success tapNo functional change; keeps error handling intact.
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts (5)
156-159: LGTM – auto-title trigger adjusted (1→2)Matches the new 1-based flow.
170-170: LGTM – previousStep lower bound updated to 1Boundary check aligns with 1-based indexing.
529-529: LGTM – forward validation loop updated to start at 1Consistent with the 1-based stepper.
546-567: No legacy step-index references remain; validators and tests don’t reference steps 0–4.
312-312: Post-create step advance logic is correct The guardif (!this.isEditMode() && this.currentStep() < this.totalSteps)and subsequentthis.nextStep()call correctly advance from a 1-basedcurrentStepto the next panel when creating a meeting.apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html (1)
200-210: Edit-mode primary CTA check depends on signal read; ensure tabs binding is fixed first.Once tabs binding is corrected, currentStep() === 1 will behave as intended for “Update Meeting”. No change needed here after the tabs fix.
Run after your fix and confirm the button toggles as you switch tabs.
apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.html (1)
23-32: Good: “New Committee” gated by project writer.apps/lfx-pcc/src/app/modules/project/meetings/meeting-dashboard/meeting-dashboard.component.ts (1)
22-22: Import consolidation looks goodCleaner RxJS imports; no behavior change.
apps/lfx-pcc/src/server/services/project.service.ts (2)
39-41: Enriching projects with writer access — goodCentralized enrichment aligns with the new AccessCheckService and keeps controllers slim.
72-76: Single-project enrichment — goodConsistent with list enrichment; maintains a uniform API surface to the frontend.
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
47-51: Organizer access enrichment on list — goodCorrect use of accessType 'organizer' for meetings.
apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.html (1)
116-147: Writer-gated actions and reactive menu binding — solid
- Edit/More buttons are correctly gated by committee?.writer with test IDs.
- View Members tooltip added.
- Menu binding switched to committeeActionMenuItems() for reactivity.
Also applies to: 153-153
apps/lfx-pcc/src/app/modules/project/committees/components/committee-table/committee-table.component.ts (2)
36-37: Reactive menu items via computed — niceKeeps the menu in sync with selected committee and permissions.
82-115: Conditional Delete action — goodDelete is shown only for writers; command safely checks selectedCommittee.
apps/lfx-pcc/src/server/services/committee.service.ts (2)
46-50: Committee list enrichment — goodBatch access check keeps calls efficient and UI-friendly.
73-75: Single committee enrichment — goodConsistent with list behavior.
packages/shared/src/interfaces/access-check.interface.ts (1)
1-37: Interfaces look correct and documentedCovers resource/access unions needed by AccessCheckService. No issues.
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts (2)
349-356: Delete action gated by organizer — LGTMConfirmation modal is correctly used; styling and command wiring look good.
200-214: Align “Add Guests” CTA with write permissions
ConfirmMeetingexposes awriterflag, then update bothinitRegistrantsLabelandonRegistrantsToggleto use(this.meeting()?.writer || this.meeting()?.organizer)instead of only
organizer.apps/lfx-pcc/src/server/services/access-check.service.ts (1)
100-115: Graceful fallback on error — LGTMReturning “all false” with error logging is a safe default.
| @if (project()?.writer) { | ||
| <lfx-button | ||
| label="New Committee" | ||
| icon="fa-light fa-plus" | ||
| severity="primary" | ||
| (onClick)="openCreateDialog()" | ||
| size="small" | ||
| data-testid="committee-new-cta"> | ||
| </lfx-button> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate the empty-state “Add First Committee” CTA too.
The bottom empty-state button (Lines 149–155) is still visible to non-writers; this is inconsistent with the new gating above.
Apply:
- <lfx-button
- label="Add First Committee"
- icon="fa-light fa-people-group"
- severity="secondary"
- (onClick)="openCreateDialog()"
- data-testid="committee-add-first"></lfx-button>
+ @if (project()?.writer) {
+ <lfx-button
+ label="Add First Committee"
+ icon="fa-light fa-people-group"
+ severity="secondary"
+ (onClick)="openCreateDialog()"
+ data-testid="committee-add-first"></lfx-button>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @if (project()?.writer) { | |
| <lfx-button | |
| label="New Committee" | |
| icon="fa-light fa-plus" | |
| severity="primary" | |
| (onClick)="openCreateDialog()" | |
| size="small" | |
| data-testid="committee-new-cta"> | |
| </lfx-button> | |
| } | |
| @if (project()?.writer) { | |
| <lfx-button | |
| label="Add First Committee" | |
| icon="fa-light fa-people-group" | |
| severity="secondary" | |
| (onClick)="openCreateDialog()" | |
| data-testid="committee-add-first"></lfx-button> | |
| } |
🤖 Prompt for AI Agents
In
apps/lfx-pcc/src/app/modules/project/committees/committee-dashboard/committee-dashboard.component.html
around lines 149 to 155, the empty-state "Add First Committee" CTA is not gated
and remains visible to non-writers; wrap that button markup with the same writer
check used at the top (e.g., conditionally render the button only when
project()?.writer is true) so non-writers cannot see the empty-state CTA,
matching the gating applied to the top "New Committee" button.
| @if (committee()?.writer) { | ||
| <lfx-button | ||
| icon="fa-light fa-ellipsis-vertical" | ||
| [text]="true" | ||
| [rounded]="true" | ||
| size="small" | ||
| severity="secondary" | ||
| (onClick)="toggleMemberActionMenu($event, member, memberActionMenu)"> | ||
| </lfx-button> | ||
| } @else { | ||
| <lfx-button | ||
| icon="fa-light fa-envelope" | ||
| [text]="true" | ||
| [rounded]="true" | ||
| size="small" | ||
| severity="secondary" | ||
| [href]="`mailto:${member.email}`" | ||
| target="_blank" | ||
| pTooltip="Send Message"> | ||
| </lfx-button> | ||
| } |
There was a problem hiding this comment.
Guard mailto when email is missing; add testids and aria-labels
Avoids mailto:undefined, improves testability and a11y.
- } @else {
- <lfx-button
- icon="fa-light fa-envelope"
- [text]="true"
- [rounded]="true"
- size="small"
- severity="secondary"
- [href]="`mailto:${member.email}`"
- target="_blank"
- pTooltip="Send Message">
- </lfx-button>
- }
+ } @else if (member.email) {
+ <lfx-button
+ icon="fa-light fa-envelope"
+ [text]="true"
+ [rounded]="true"
+ size="small"
+ severity="secondary"
+ [href]="`mailto:${member.email}`"
+ target="_blank"
+ pTooltip="Send Message"
+ [attr.data-testid]="'committee-members-message-' + member.uid"
+ [attr.aria-label]="'Send email to ' + (member.first_name || '') + ' ' + (member.last_name || '')">
+ </lfx-button>
+ } @else {
+ <lfx-button
+ icon="fa-light fa-envelope-slash"
+ [text]="true"
+ [rounded]="true"
+ size="small"
+ severity="secondary"
+ [disabled]="true"
+ pTooltip="No email available"
+ [attr.data-testid]="'committee-members-noemail-' + member.uid"
+ [attr.aria-label]="'No email available'">
+ </lfx-button>
+ }Also add testids/aria-labels to the ellipsis button:
- <lfx-button
+ <lfx-button
icon="fa-light fa-ellipsis-vertical"
[text]="true"
[rounded]="true"
size="small"
severity="secondary"
- (onClick)="toggleMemberActionMenu($event, member, memberActionMenu)">
+ (onClick)="toggleMemberActionMenu($event, member, memberActionMenu)"
+ [attr.data-testid]="'committee-members-actions-' + member.uid"
+ [attr.aria-label]="'Open actions for ' + (member.first_name || '') + ' ' + (member.last_name || '')">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @if (committee()?.writer) { | |
| <lfx-button | |
| icon="fa-light fa-ellipsis-vertical" | |
| [text]="true" | |
| [rounded]="true" | |
| size="small" | |
| severity="secondary" | |
| (onClick)="toggleMemberActionMenu($event, member, memberActionMenu)"> | |
| </lfx-button> | |
| } @else { | |
| <lfx-button | |
| icon="fa-light fa-envelope" | |
| [text]="true" | |
| [rounded]="true" | |
| size="small" | |
| severity="secondary" | |
| [href]="`mailto:${member.email}`" | |
| target="_blank" | |
| pTooltip="Send Message"> | |
| </lfx-button> | |
| } | |
| @if (committee()?.writer) { | |
| <lfx-button | |
| icon="fa-light fa-ellipsis-vertical" | |
| [text]="true" | |
| [rounded]="true" | |
| size="small" | |
| severity="secondary" | |
| (onClick)="toggleMemberActionMenu($event, member, memberActionMenu)" | |
| [attr.data-testid]="'committee-members-actions-' + member.uid" | |
| [attr.aria-label]="'Open actions for ' + (member.first_name || '') + ' ' + (member.last_name || '')"> | |
| </lfx-button> | |
| } @else if (member.email) { | |
| <lfx-button | |
| icon="fa-light fa-envelope" | |
| [text]="true" | |
| [rounded]="true" | |
| size="small" | |
| severity="secondary" | |
| [href]="`mailto:${member.email}`" | |
| target="_blank" | |
| pTooltip="Send Message" | |
| [attr.data-testid]="'committee-members-message-' + member.uid" | |
| [attr.aria-label]="'Send email to ' + (member.first_name || '') + ' ' + (member.last_name || '')"> | |
| </lfx-button> | |
| } @else { | |
| <lfx-button | |
| icon="fa-light fa-envelope-slash" | |
| [text]="true" | |
| [rounded]="true" | |
| size="small" | |
| severity="secondary" | |
| [disabled]="true" | |
| pTooltip="No email available" | |
| [attr.data-testid]="'committee-members-noemail-' + member.uid" | |
| [attr.aria-label]="'No email available'"> | |
| </lfx-button> | |
| } |
| @if (project()?.writer) { | ||
| <lfx-button | ||
| label="Create Meeting" | ||
| icon="fa-light fa-calendar-plus" | ||
| severity="secondary" | ||
| size="small" | ||
| [routerLink]="['meetings/create']" | ||
| [relativeTo]="activatedRoute"> | ||
| </lfx-button> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also gate the empty-state “Add Committee” CTA.
Lines 261–262 remain unconditional. Align with writer gating to avoid exposing actions to read-only users.
- <lfx-button label="Add Committee" icon="fa-light fa-people-group" severity="secondary" size="small" (onClick)="openCreateDialog()">
+ @if (project()?.writer) {
+ <lfx-button label="Add Committee" icon="fa-light fa-people-group" severity="secondary" size="small" (onClick)="openCreateDialog()">
+ </lfx-button>
+ }
- </lfx-button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @if (project()?.writer) { | |
| <lfx-button | |
| label="Create Meeting" | |
| icon="fa-light fa-calendar-plus" | |
| severity="secondary" | |
| size="small" | |
| [routerLink]="['meetings/create']" | |
| [relativeTo]="activatedRoute"> | |
| </lfx-button> | |
| } | |
| @if (project()?.writer) { | |
| <lfx-button | |
| label="Add Committee" | |
| icon="fa-light fa-people-group" | |
| severity="secondary" | |
| size="small" | |
| (onClick)="openCreateDialog()"> | |
| </lfx-button> | |
| } |
🤖 Prompt for AI Agents
In
apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.html
around lines 202–211 (and also apply the same gating to lines 261–262), the
empty-state "Add Committee" CTA is not gated by the project()?.writer check and
therefore is shown to read-only users; wrap the "Add Committee" button/template
in the same @if (project()?.writer) conditional used for the "Create Meeting"
button (or add an *ngIf/project()?.writer equivalent) so the CTA only renders
for writer users, and ensure the relative routing attributes remain unchanged.
| <p-tab [value]="1">Meeting Details</p-tab> | ||
| <p-tab [value]="2">Manage Guests</p-tab> | ||
| </p-tablist> | ||
| <p-tabpanels> | ||
| <p-tabpanel [value]="0"> | ||
| <p-tabpanel [value]="1"> | ||
| <div class="space-y-8" data-testid="meeting-manage-single-page"> |
There was a problem hiding this comment.
Tabs wiring bug: duplicate panel value and incorrect two-way binding with a Signal.
- The second tabpanel later in the file also has [value]="1"; it should be 2.
- [(value)]="currentStep" won’t work if currentStep is a Signal. Use explicit in/out bindings.
Apply:
- <p-tabs [(value)]="currentStep">
+ <p-tabs [value]="currentStep()" (valueChange)="goToStep($event)">And fix the second panel:
- <p-tabpanel [value]="1">
+ <p-tabpanel [value]="2">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p-tab [value]="1">Meeting Details</p-tab> | |
| <p-tab [value]="2">Manage Guests</p-tab> | |
| </p-tablist> | |
| <p-tabpanels> | |
| <p-tabpanel [value]="0"> | |
| <p-tabpanel [value]="1"> | |
| <div class="space-y-8" data-testid="meeting-manage-single-page"> | |
| <!-- Replace the two-way binding on the Signal with explicit in/out bindings --> | |
| <p-tabs [value]="currentStep()" (valueChange)="goToStep($event)"> | |
| <p-tab [value]="1">Meeting Details</p-tab> | |
| <p-tab [value]="2">Manage Guests</p-tab> | |
| </p-tablist> | |
| <p-tabpanels> | |
| <!-- First panel stays at value=1 --> | |
| <p-tabpanel [value]="1"> | |
| <div class="space-y-8" data-testid="meeting-manage-single-page"> | |
| <!-- Meeting Details content --> | |
| </div> | |
| </p-tabpanel> | |
| <!-- Corrected the duplicate value from 1 to 2 --> | |
| <p-tabpanel [value]="2"> | |
| <div class="space-y-8" data-testid="meeting-manage-guest-page"> | |
| <!-- Manage Guests content --> | |
| </div> | |
| </p-tabpanel> | |
| </p-tabpanels> | |
| </p-tabs> |
🤖 Prompt for AI Agents
In
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.html
around lines 83 to 88, the second <p-tabpanel> is incorrectly using [value]="1"
(duplicate) and the tabs use two-way binding [(value)]="currentStep" which won't
work with a Signal<number>; change the second panel's value to 2 and replace the
banana-in-a-box binding with explicit input/output: bind the panel value using
the Signal getter and handle changes with the Signal setter (i.e. use
[value]="currentStep()" and (valueChange)="currentStep.set($event)" or
equivalent in your framework).
| public attachments = this.initializeAttachments(); | ||
| // Stepper state | ||
| public currentStep = signal<number>(0); | ||
| public currentStep = signal<number>(1); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Step index switch to 1-based: verify constants/templates match
Ensure TOTAL_STEPS and MEETING_STEP_TITLES are now 1-based and all template bindings use 1..TOTAL_STEPS.
🏁 Script executed:
#!/bin/bash
# Inspect step constants and any 0-based assumptions
rg -n -C2 -S "MEETING_STEP_TITLES|TOTAL_STEPS|currentStep\(\)\s*===\s*0|case 0|step-0|value=\"0\""Length of output: 3519
Fix off-by-one in step indexing
In meeting-manage.component.ts (line 615), getStepTitle uses MEETING_STEP_TITLES[step] even though currentStep is 1-based and the titles array is zero-based. Change to:
private getStepTitle(step: number): string {
return MEETING_STEP_TITLES[step - 1] || '';
}🤖 Prompt for AI Agents
In
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
around line 99, getStepTitle currently indexes MEETING_STEP_TITLES with a
1-based step causing an off-by-one; change the function to subtract 1 when
indexing (use MEETING_STEP_TITLES[step - 1]) and fallback to an empty string if
out of range.
| // Validation signals for template | ||
| public readonly canProceed = signal<boolean>(false); | ||
| public readonly canGoNext = computed(() => this.currentStep() + 1 < this.totalSteps && this.canNavigateToStep(this.currentStep() + 1)); | ||
| public readonly canGoPrevious = computed(() => this.currentStep() > 0); | ||
| public readonly isFirstStep = computed(() => this.currentStep() === 0); | ||
| public readonly isLastMeetingStep = computed(() => this.currentStep() === this.totalSteps - 2); | ||
| public readonly isLastStep = computed(() => this.currentStep() === this.totalSteps - 1); | ||
| public readonly canGoPrevious = computed(() => this.currentStep() > 1); | ||
| public readonly isFirstStep = computed(() => this.currentStep() === 1); | ||
| public readonly isLastMeetingStep = computed(() => this.currentStep() === this.totalSteps - 1); | ||
| public readonly isLastStep = computed(() => this.currentStep() === this.totalSteps); | ||
| public readonly currentStepTitle = computed(() => this.getStepTitle(this.currentStep())); |
There was a problem hiding this comment.
Bug: Next disabled on step (TOTAL_STEPS-1)
canGoNext uses +1 < TOTAL_STEPS, blocking navigation from step 4→5 (when TOTAL_STEPS=5).
- public readonly canGoNext = computed(() => this.currentStep() + 1 < this.totalSteps && this.canNavigateToStep(this.currentStep() + 1));
+ public readonly canGoNext = computed(() => this.currentStep() < this.totalSteps && this.canNavigateToStep(this.currentStep() + 1));Also guard goToStep to valid range:
if (step !== undefined && step >= 1 && step <= this.totalSteps && this.canNavigateToStep(step)) { ... }🤖 Prompt for AI Agents
In
apps/lfx-pcc/src/app/modules/project/meetings/components/meeting-manage/meeting-manage.component.ts
around lines 113–120, canGoNext incorrectly uses "this.currentStep() + 1 <
this.totalSteps" which prevents moving from step (TOTAL_STEPS-1) to the final
step; change the comparison to allow the next step when currentStep()+1 <=
this.totalSteps (and still call this.canNavigateToStep for permission). Also
harden goToStep by validating the target step before navigating: ensure step is
defined and within the inclusive range 1..this.totalSteps (e.g., step !==
undefined && step >= 1 && step <= this.totalSteps) and only then call
this.canNavigateToStep(step) and proceed.
| // Create result map | ||
| const resultMap = new Map<string, boolean>(); | ||
| const userAccessInfo: Array<{ resourceId: string; username?: string; hasAccess: boolean }> = []; | ||
|
|
||
| // Map results back to resource IDs | ||
| for (let i = 0; i < resources.length; i++) { | ||
| const resource = resources[i]; | ||
| const resultString = response.results[i]; | ||
|
|
||
| // Parse the result string format: "resource:id#access@user:username\ttrue/false" | ||
| let hasAccess = false; | ||
| let username: string | undefined; | ||
|
|
||
| if (resultString && typeof resultString === 'string') { | ||
| // Split by tab to get the boolean part | ||
| const parts = resultString.split('\t'); | ||
| if (parts.length >= 2) { | ||
| hasAccess = parts[1]?.toLowerCase() === 'true'; | ||
|
|
||
| // Extract username from the first part: "resource:id#access@user:username" | ||
| const accessPart = parts[0]; | ||
| const userMatch = accessPart?.match(/@user:(.+)$/); | ||
| if (userMatch) { | ||
| username = userMatch[1]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| resultMap.set(resource.id, hasAccess); | ||
| userAccessInfo.push({ resourceId: resource.id, username, hasAccess }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Order-dependent result mapping; parse keys and map by “type:id#access” to avoid mis-assignment
Assuming result order matches request order is brittle and breaks for mixed resource types or reordering. Parse the returned keys and build a composite map.
- // Create result map
- const resultMap = new Map<string, boolean>();
- const userAccessInfo: Array<{ resourceId: string; username?: string; hasAccess: boolean }> = [];
-
- // Map results back to resource IDs
- for (let i = 0; i < resources.length; i++) {
- const resource = resources[i];
- const resultString = response.results[i];
-
- // Parse the result string format: "resource:id#access@user:username\ttrue/false"
- let hasAccess = false;
- let username: string | undefined;
-
- if (resultString && typeof resultString === 'string') {
- // Split by tab to get the boolean part
- const parts = resultString.split('\t');
- if (parts.length >= 2) {
- hasAccess = parts[1]?.toLowerCase() === 'true';
-
- // Extract username from the first part: "resource:id#access@user:username"
- const accessPart = parts[0];
- const userMatch = accessPart?.match(/@user:(.+)$/);
- if (userMatch) {
- username = userMatch[1];
- }
- }
- }
-
- resultMap.set(resource.id, hasAccess);
- userAccessInfo.push({ resourceId: resource.id, username, hasAccess });
- }
+ // Create result map keyed by "resourceType:id#access"
+ const resultMap = new Map<string, boolean>();
+ const userAccessInfo: Array<{ resourceKey: string; username?: string; hasAccess: boolean }> = [];
+
+ for (const resultString of response.results ?? []) {
+ if (!resultString || typeof resultString !== 'string') continue;
+ const [infoPart, boolPartRaw] = resultString.split('\t');
+ const hasAccess = (boolPartRaw ?? '').trim().toLowerCase() === 'true';
+ const accessPart = (infoPart ?? '').split('@user:')[0] ?? '';
+ // accessPart example: "meeting:abc123#organizer"
+ const match = accessPart.match(/^([a-z]+):(.+?)#([a-z_]+)$/i);
+ let username: string | undefined = undefined;
+ const userMatch = (infoPart ?? '').match(/@user:(.+)$/);
+ if (userMatch) username = userMatch[1];
+
+ if (match) {
+ const [, resourceType, resourceId, access] = match;
+ const key = `${resourceType}:${resourceId}#${access}`;
+ resultMap.set(key, hasAccess);
+ userAccessInfo.push({ resourceKey: key, username, hasAccess });
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create result map | |
| const resultMap = new Map<string, boolean>(); | |
| const userAccessInfo: Array<{ resourceId: string; username?: string; hasAccess: boolean }> = []; | |
| // Map results back to resource IDs | |
| for (let i = 0; i < resources.length; i++) { | |
| const resource = resources[i]; | |
| const resultString = response.results[i]; | |
| // Parse the result string format: "resource:id#access@user:username\ttrue/false" | |
| let hasAccess = false; | |
| let username: string | undefined; | |
| if (resultString && typeof resultString === 'string') { | |
| // Split by tab to get the boolean part | |
| const parts = resultString.split('\t'); | |
| if (parts.length >= 2) { | |
| hasAccess = parts[1]?.toLowerCase() === 'true'; | |
| // Extract username from the first part: "resource:id#access@user:username" | |
| const accessPart = parts[0]; | |
| const userMatch = accessPart?.match(/@user:(.+)$/); | |
| if (userMatch) { | |
| username = userMatch[1]; | |
| } | |
| } | |
| } | |
| resultMap.set(resource.id, hasAccess); | |
| userAccessInfo.push({ resourceId: resource.id, username, hasAccess }); | |
| } | |
| // Create result map keyed by "resourceType:id#access" | |
| const resultMap = new Map<string, boolean>(); | |
| const userAccessInfo: Array<{ resourceKey: string; username?: string; hasAccess: boolean }> = []; | |
| for (const resultString of response.results ?? []) { | |
| if (!resultString || typeof resultString !== 'string') continue; | |
| const [infoPart, boolPartRaw] = resultString.split('\t'); | |
| const hasAccess = (boolPartRaw ?? '').trim().toLowerCase() === 'true'; | |
| const accessPart = (infoPart ?? '').split('@user:')[0] ?? ''; | |
| // accessPart example: "meeting:abc123#organizer" | |
| const match = accessPart.match(/^([a-z]+):(.+?)#([a-z_]+)$/i); | |
| let username: string | undefined = undefined; | |
| const userMatch = (infoPart ?? '').match(/@user:(.+)$/); | |
| if (userMatch) username = userMatch[1]; | |
| if (match) { | |
| const [, resourceType, resourceId, access] = match; | |
| const key = `${resourceType}:${resourceId}#${access}`; | |
| resultMap.set(key, hasAccess); | |
| userAccessInfo.push({ resourceKey: key, username, hasAccess }); | |
| } | |
| } |
| public async checkSingleAccess(req: Request, resource: AccessCheckRequest): Promise<boolean> { | ||
| const results = await this.checkAccess(req, [resource]); | ||
| return results.get(resource.id) || false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update single-resource lookup to composite key
Use the same composite key as the batch mapping.
- const results = await this.checkAccess(req, [resource]);
- return results.get(resource.id) || false;
+ const results = await this.checkAccess(req, [resource]);
+ const key = `${resource.resource}:${resource.id}#${resource.access}`;
+ return results.get(key) || false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async checkSingleAccess(req: Request, resource: AccessCheckRequest): Promise<boolean> { | |
| const results = await this.checkAccess(req, [resource]); | |
| return results.get(resource.id) || false; | |
| } | |
| public async checkSingleAccess(req: Request, resource: AccessCheckRequest): Promise<boolean> { | |
| const results = await this.checkAccess(req, [resource]); | |
| const key = `${resource.resource}:${resource.id}#${resource.access}`; | |
| return results.get(key) || false; | |
| } |
🤖 Prompt for AI Agents
In apps/lfx-pcc/src/server/services/access-check.service.ts around lines
124-127, the single-resource lookup uses resource.id only but the batch
checkAccess populates the results Map with a composite key; change this method
to build the identical composite key used in the batch mapping (e.g. the same
id+type/namespace concatenation or helper method used elsewhere) and call
results.get(compositeKey) (falling back to false) instead of
results.get(resource.id).
Summary
Implements a comprehensive role-based permission system for projects, committees, and meetings with conditional UI rendering based on user access levels.
Changes Made
Backend Permission Service
/access-checkendpointUI Permission States
Interface Updates
writerfield to Project and Committee interfacesorganizerfield to Meeting interface for permission trackingAccessCheckInterfacefor permission service integrationUI Enhancements
Related JIRA Tickets
Technical Details
Permission Flow
AccessCheckService.addAccessToResources()to add permission fields/access-checkendpointwriter/organizerflagsFiles Modified
Testing
Breaking Changes
None - all changes are additive and backward compatible.