Conversation
github-actions
bot
requested review from
andrest50,
asithade,
dealako,
jordane and
mauriciozanettisalomao
6 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds member-visibility gating, persona-based management checks, null-safe dialog handling, mailto menu navigation, conditional voting columns, skeleton empty/loading states, testids, date-range validators and clear helpers, and a typed MemberFormValue payload. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommitteeMembers as CommitteeMembersComponent
participant Persona as PersonaService
participant Dialog as DialogService/MemberForm
participant API as MemberService
participant Server
User->>CommitteeMembers: open members view / click Add
CommitteeMembers->>Persona: request persona info
Persona-->>CommitteeMembers: persona roles (board/maintainer)
alt members visible or manager
CommitteeMembers->>User: render members table (voting cols if enabled), Add button (if canManage)
User->>CommitteeMembers: Click "Add Member"
CommitteeMembers->>Dialog: open MemberForm (null-safe)
Dialog->>User: display form
User->>Dialog: Submit form (getRawValue → MemberFormValue)
Dialog->>API: POST member payload
API->>Server: send request
Server-->>API: response
API-->>Dialog: success
Dialog-->>CommitteeMembers: onClose (refresh)
CommitteeMembers->>API: fetch members
else hidden
CommitteeMembers->>User: show members-hidden-placeholder
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
6 tasks
4 tasks
9 tasks
…anced form - Add MemberFormValue interface to shared package for typed form values - Add governance columns (appointed by, voting status, role dates) to members table - Add member_visibility gate to control column visibility based on committee settings - Add data-testid attributes for reliable test targeting - Add organization search with autocomplete in member form - Add individual toggle to disable org fields when member has no org affiliation - Add date pickers for role and voting status start/end dates - Add LinkedIn profile field to member form - Add ClearableEndDateValidator for cross-field date validation - Add buildOrganizationPayload helper for org data serialization - Use getRawValue() to capture disabled field values in form submission LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
manishdixitlfx
force-pushed
the
feat/LFXV2-committees-members-form
branch
from
ca6f5f6 to
914c1fb
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the committees “Members” experience by extending the member form (new governance + org/individual fields, better validation, test IDs) and tightening members-tab visibility/UX behavior.
Changes:
- Added a shared
MemberFormValuetype to representFormGroup.getRawValue()output for the member dialog. - Enhanced the member form UI/logic (individual toggle, org autocomplete, appointed-by select, date-range validation,
getRawValue()on submit, moredata-testids). - Updated the members tab component with
member_visibilitygating, persona-aware permissions, improved empty/loading states, and SSR-safe mailto action handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/interfaces/member.interface.ts |
Adds MemberFormValue interface for typed member-form raw values. |
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts |
Implements individual/org toggle logic, raw-value submission, organization payload builder, and date range validators. |
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.html |
Updates member form UI with individual toggle, required markers, date-range controls, appointed-by select, and test IDs. |
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts |
Adds persona/platform awareness and member_visibility-based visibility gating. |
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html |
Adds hidden-members placeholder, improved empty/loading states, and refactors header/actions layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+247
to
+251
| organization: new FormControl('', [Validators.required]), | ||
| organization_url: new FormControl(''), | ||
| role: new FormControl('', this.committee?.enable_voting ? [Validators.required] : []), | ||
| voting_status: new FormControl('', this.committee?.enable_voting ? [Validators.required] : []), | ||
| appointed_by: new FormControl(''), |
Comment on lines
+266
to
+270
| private static dateRangeValidator(startKey: string, endKey: string) { | ||
| return (group: AbstractControl): ValidationErrors | null => { | ||
| const start = group.get(startKey)?.value; | ||
| const end = group.get(endKey)?.value; | ||
| if (start && end && new Date(start) > new Date(end)) { |
| <i class="fa-light fa-circle-notch fa-spin text-4xl text-gray-400"></i> | ||
| <td> | ||
| @if (member.organization?.website) { | ||
| <a [href]="member.organization.website" target="_blank" class="text-primary hover:text-primary-600 text-sm font-sans"> |
Comment on lines
+98
to
+102
| @if (committee()?.enable_voting) { | ||
| <th> | ||
| <span class="text-sm font-sans text-gray-500">Role</span> | ||
| </th> | ||
| <th> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (1)
37-37:⚠️ Potential issue | 🟠 MajorForm validators for
roleandvoting_statusare not set correctly due to initialization order.The
formsignal is created before the constructor assignsthis.committee, sothis.committee?.enable_votingis falsy whencreateMemberFormGroup()runs. The form ends up with empty validators for these fields instead of[Validators.required], allowing submission without values even when voting is enabled.Move form initialization to after
this.committeeis assigned:Suggested fix
- public form = signal<FormGroup>(this.createMemberFormGroup()); + public form = signal<FormGroup>(new FormGroup({})); public constructor() { // Initialize config-based properties this.isEditing = this.config.data?.isEditing || false; this.memberId = this.config.data?.memberId; this.member = this.config.data?.member; this.committee = this.config.data?.committee; this.wizardMode = this.config.data?.wizardMode || false; + this.form.set(this.createMemberFormGroup()); // Initialize form with data when component is created this.initializeForm();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` at line 37, The form signal is initialized before the constructor sets this.committee, causing createMemberFormGroup() to see this.committee?.enable_voting as falsy and omit Validators.required for the role and voting_status controls; move the form initialization out of the field declaration and into the constructor (after this.committee is assigned) so createMemberFormGroup() can correctly add [Validators.required] to the 'role' and 'voting_status' controls when this.committee.enable_voting is true; update any references to the public form = signal<FormGroup>(...) declaration to instead assign the signal inside the constructor and keep createMemberFormGroup(), role, voting_status, and Validators.required logic unchanged.
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (1)
14-15: Use direct shared-package imports here too.These new
@lfx-one/shared/*barrel imports go against the repo rule for component files.As per coding guidelines,
**/*.{ts,tsx}: Shared package imports must use direct imports for standalone components, not barrel exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 14 - 15, Replace the barrel imports with direct shared-package imports: instead of importing APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES from '@lfx-one/shared/constants' and Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from '@lfx-one/shared/interfaces', import each symbol from its specific module path in the shared package (e.g., the exact constants file and the exact interfaces file) so the component uses direct imports rather than the '@lfx-one/shared/*' barrels.apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts (1)
15-16: Use direct shared-package imports here.These new
@lfx-one/shared/*barrel imports go against the repo rule for component files.As per coding guidelines,
**/*.{ts,tsx}: Shared package imports must use direct imports for standalone components, not barrel exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts` around lines 15 - 16, The component is importing from barrel paths '@lfx-one/shared/constants' and '@lfx-one/shared/interfaces'—replace those barrel imports with direct shared-package file imports so the component uses the package’s explicit module paths; update the imports that bring in COMMITTEE_LABEL and the types Committee, CommitteeMember, GroupBehavioralClass in committee-members.component.ts to import each symbol from its specific file within the shared package (e.g., the concrete constants and interfaces modules) rather than the top-level barrel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html`:
- Around line 98-105: The members table only conditionally renders Role and
Voting Status when committee()?.enable_voting is true but does not render the
new governance fields captured by the form (appointed_by and the role/voting
date ranges), so update the committee-members component template to include
table columns and cells for appointed_by, role start/end and voting start/end
(use the existing committee()?.enable_voting condition and the table row
rendering for each member) and bind them to the member properties (e.g.,
member.appointed_by, member.role, member.role_start_date, member.role_end_date,
member.voting_start_date, member.voting_end_date) so saved values are visible;
keep the same styling structure as the existing <th>/<span> and cell rendering
used for Role and Voting Status.
- Around line 125-128: The anchor in committee-members.component.html that
renders member.organization.name uses [href]="member.organization.website" with
target="_blank" and must include rel="noopener noreferrer" to prevent
window.opener access; update the <a> element that references
member.organization.website (the link showing {{ member.organization.name }}) to
add rel="noopener noreferrer" alongside the existing target attribute.
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Around line 92-95: The computed isMembersVisible currently treats a null
committee as visible because visibility becomes undefined and undefined !==
'hidden' is true; update the getter so it defaults the visibility to 'hidden'
when committee() is null/undefined (e.g., derive visibility from
this.committee()?.member_visibility ?? 'hidden') and then evaluate return
visibility !== 'hidden' || this.canManageMembers(); reference: isMembersVisible
computed, committee(), member_visibility, canManageMembers().
---
Outside diff comments:
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Line 37: The form signal is initialized before the constructor sets
this.committee, causing createMemberFormGroup() to see
this.committee?.enable_voting as falsy and omit Validators.required for the role
and voting_status controls; move the form initialization out of the field
declaration and into the constructor (after this.committee is assigned) so
createMemberFormGroup() can correctly add [Validators.required] to the 'role'
and 'voting_status' controls when this.committee.enable_voting is true; update
any references to the public form = signal<FormGroup>(...) declaration to
instead assign the signal inside the constructor and keep
createMemberFormGroup(), role, voting_status, and Validators.required logic
unchanged.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Around line 15-16: The component is importing from barrel paths
'@lfx-one/shared/constants' and '@lfx-one/shared/interfaces'—replace those
barrel imports with direct shared-package file imports so the component uses the
package’s explicit module paths; update the imports that bring in
COMMITTEE_LABEL and the types Committee, CommitteeMember, GroupBehavioralClass
in committee-members.component.ts to import each symbol from its specific file
within the shared package (e.g., the concrete constants and interfaces modules)
rather than the top-level barrel.
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 14-15: Replace the barrel imports with direct shared-package
imports: instead of importing APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN,
MEMBER_ROLES, VOTING_STATUSES from '@lfx-one/shared/constants' and Committee,
CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from
'@lfx-one/shared/interfaces', import each symbol from its specific module path
in the shared package (e.g., the exact constants file and the exact interfaces
file) so the component uses direct imports rather than the '@lfx-one/shared/*'
barrels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa07a21b-4375-4373-8614-db86428f531e
📒 Files selected for processing (5)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.tsapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.htmlapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.tspackages/shared/src/interfaces/member.interface.ts
Comment on lines
+98
to
105
| @if (committee()?.enable_voting) { | ||
| <th> | ||
| <span class="text-sm font-sans text-gray-500">Role</span> | ||
| </th> | ||
| <th> | ||
| <span class="text-sm font-sans text-gray-500">Voting Status</span> | ||
| </th> | ||
| } |
Contributor
There was a problem hiding this comment.
Render the rest of the new governance fields in the members table.
The form now captures and submits appointed_by plus the role/voting date ranges, but this table still only renders Role and Voting Status. Those values stay invisible after save, so the governance-column part of this PR is still incomplete.
Also applies to: 133-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html`
around lines 98 - 105, The members table only conditionally renders Role and
Voting Status when committee()?.enable_voting is true but does not render the
new governance fields captured by the form (appointed_by and the role/voting
date ranges), so update the committee-members component template to include
table columns and cells for appointed_by, role start/end and voting start/end
(use the existing committee()?.enable_voting condition and the table row
rendering for each member) and bind them to the member properties (e.g.,
member.appointed_by, member.role, member.role_start_date, member.role_end_date,
member.voting_start_date, member.voting_end_date) so saved values are visible;
keep the same styling structure as the existing <th>/<span> and cell rendering
used for Role and Voting Status.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts (1)
92-95:⚠️ Potential issue | 🟠 MajorFail closed when committee visibility settings are not loaded.
Line 93 and Line 94 currently make members visible when
committee()isnullbecauseundefined !== 'hidden'evaluates true. Default to hidden until committee data is available.🛡️ Suggested fix
- this.isMembersVisible = computed(() => { - const visibility = this.committee()?.member_visibility; - return visibility !== 'hidden' || this.canManageMembers(); - }); + this.isMembersVisible = computed(() => { + const committee = this.committee(); + if (!committee) { + return false; + } + return committee.member_visibility !== 'hidden' || this.canManageMembers(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts` around lines 92 - 95, The computed isMembersVisible currently treats a null committee as visible because undefined !== 'hidden' is true; update the logic to fail closed by first checking the committee exists (e.g., const c = this.committee()) and only then evaluate c.member_visibility !== 'hidden' || this.canManageMembers(); ensure you reference and change the isMembersVisible computed selector (and keep use of canManageMembers()) so members remain hidden until committee data is loaded.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (1)
71-104: Add JSDoc to the newly exposed public form handlers.
onIndividualToggle,clearRoleDates,clearVotingDates, andonDateChangeare public and newly introduced; add brief JSDoc to document intent/usage.As per coding guidelines "Add JSDoc comments for public functions and exported modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 71 - 104, Add concise JSDoc comments to each newly public method to satisfy the coding guideline: document intent and usage for onIndividualToggle, clearRoleDates, clearVotingDates, and onDateChange; for each, add a brief description, note that they operate on the reactive form (this.form()), and include `@public` and `@returns` {void} (no params) or equivalent tags where your project's JSDoc style requires them; keep comments one or two lines each and place them directly above the respective method declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Line 16: The import in committee-members.component.ts currently pulls
Committee, CommitteeMember, and GroupBehavioralClass from the barrel
`@lfx-one/shared/interfaces`; replace that single barrel import with direct module
imports for each interface (use the exact module paths that export Committee,
CommitteeMember, and GroupBehavioralClass in the shared package) so the
standalone component follows the repo rule against barrel imports—update the
import statement(s) referencing Committee, CommitteeMember, and
GroupBehavioralClass accordingly.
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 14-15: The imports using barrel paths should be replaced with
direct shared-module imports: replace the constants import that brings in
APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES
from '@lfx-one/shared/constants' with direct paths to the module that actually
exports those constants, and replace the interfaces import that brings in
Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from
'@lfx-one/shared/interfaces' with the concrete module paths that export those
interfaces; update the import statements in member-form.component.ts to
reference the specific files that define APPOINTED_BY_OPTIONS,
LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES and the files exporting
Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue so the
standalone component no longer uses barrel imports.
---
Duplicate comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Around line 92-95: The computed isMembersVisible currently treats a null
committee as visible because undefined !== 'hidden' is true; update the logic to
fail closed by first checking the committee exists (e.g., const c =
this.committee()) and only then evaluate c.member_visibility !== 'hidden' ||
this.canManageMembers(); ensure you reference and change the isMembersVisible
computed selector (and keep use of canManageMembers()) so members remain hidden
until committee data is loaded.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 71-104: Add concise JSDoc comments to each newly public method to
satisfy the coding guideline: document intent and usage for onIndividualToggle,
clearRoleDates, clearVotingDates, and onDateChange; for each, add a brief
description, note that they operate on the reactive form (this.form()), and
include `@public` and `@returns` {void} (no params) or equivalent tags where your
project's JSDoc style requires them; keep comments one or two lines each and
place them directly above the respective method declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a01f679d-a690-48ef-8b18-cf8fdec3af5c
📒 Files selected for processing (5)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.tsapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.htmlapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.tspackages/shared/src/interfaces/member.interface.ts
✅ Files skipped from review due to trivial changes (1)
- apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.html
| import { TableComponent } from '@components/table/table.component'; | ||
| import { COMMITTEE_LABEL } from '@lfx-one/shared/constants'; | ||
| import { Committee, CommitteeMember } from '@lfx-one/shared/interfaces'; | ||
| import { Committee, CommitteeMember, GroupBehavioralClass } from '@lfx-one/shared/interfaces'; |
Contributor
There was a problem hiding this comment.
Use direct shared imports instead of the interfaces barrel in this standalone component.
Line 16 imports from @lfx-one/shared/interfaces; please switch to direct interface module paths to match repo import rules for standalone components.
As per coding guidelines "Shared package imports must use direct imports for standalone components, not barrel exports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`
at line 16, The import in committee-members.component.ts currently pulls
Committee, CommitteeMember, and GroupBehavioralClass from the barrel
`@lfx-one/shared/interfaces`; replace that single barrel import with direct module
imports for each interface (use the exact module paths that export Committee,
CommitteeMember, and GroupBehavioralClass in the shared package) so the
standalone component follows the repo rule against barrel imports—update the
import statement(s) referencing Committee, CommitteeMember, and
GroupBehavioralClass accordingly.
Comment on lines
+14
to
+15
| import { APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES } from '@lfx-one/shared/constants'; | ||
| import { Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue } from '@lfx-one/shared/interfaces'; |
Contributor
There was a problem hiding this comment.
Use direct shared imports instead of barrel imports in this standalone component.
Line 14 and Line 15 import from @lfx-one/shared/constants and @lfx-one/shared/interfaces barrels. Please switch to direct module paths per repo convention for standalone components.
As per coding guidelines "Shared package imports must use direct imports for standalone components, not barrel exports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`
around lines 14 - 15, The imports using barrel paths should be replaced with
direct shared-module imports: replace the constants import that brings in
APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES
from '@lfx-one/shared/constants' with direct paths to the module that actually
exports those constants, and replace the interfaces import that brings in
Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from
'@lfx-one/shared/interfaces' with the concrete module paths that export those
interfaces; update the import statements in member-form.component.ts to
reference the specific files that define APPOINTED_BY_OPTIONS,
LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES and the files exporting
Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue so the
standalone component no longer uses barrel imports.
- Default isMembersVisible to false while committee is loading (fail closed for privacy — was flashing members before committee metadata arrived) - Add rel="noopener noreferrer" to org website link (tabnabbing prevention) - Move createMemberFormGroup() into constructor body after this.committee is assigned from config.data, so enable_voting validators are correctly applied LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (1)
14-15: 🛠️ Refactor suggestion | 🟠 MajorUse direct shared imports instead of barrel imports.
Lines 14-15 import from
@lfx-one/shared/constantsand@lfx-one/shared/interfacesbarrel files. For standalone components, use direct module paths per the coding guidelines.As per coding guidelines "Shared package imports must use direct imports for standalone components, not barrel exports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 14 - 15, Replace the barrel imports in member-form.component.ts by importing the constants and interfaces directly from their source modules: instead of importing APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN, MEMBER_ROLES, VOTING_STATUSES from '@lfx-one/shared/constants' and Committee, CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from '@lfx-one/shared/interfaces', update the import statements to reference the specific module files that export those symbols (the concrete files within the shared package that define those constants and interfaces) so the component uses direct shared imports per the coding guidelines.apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html (1)
98-105:⚠️ Potential issue | 🟠 MajorGovernance columns still incomplete - only Role and Voting Status are rendered.
The form captures
appointed_by,role_start,role_end,voting_status_start, andvoting_status_end, but this table only displaysRoleandVoting Status. The saved governance data remains invisible to users.Consider adding columns for the date ranges and appointed-by field, or deferring this to a follow-up PR if intentional.
Also applies to: 133-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html` around lines 98 - 105, The table conditional rendering under committee()?.enable_voting only shows "Role" and "Voting Status" columns while the form captures governance fields appointed_by, role_start, role_end, voting_status_start and voting_status_end; update the template (committee-members.component.html) to add corresponding header columns and cells for appointed_by and the two date ranges (role_start/role_end and voting_status_start/voting_status_end) in both places noted (lines ~98-105 and ~133-140) so saved governance data is visible, or if intentional, add a TODO comment and defer column rendering to a follow-up PR; ensure the cell bindings reference the member model properties used by the form.
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (2)
36-37: Simplify the signal type annotation.The type
ReturnType<typeof signal<FormGroup>>is verbose and potentially confusing. Use the explicitSignal<FormGroup>type or rely on TypeScript inference.♻️ Suggested fix
- public form!: ReturnType<typeof signal<FormGroup>>; + public form!: Signal<FormGroup>;And add the import:
import { Component, inject, signal, Signal } from '@angular/core';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 36 - 37, Replace the verbose type for the form signal property — change the declaration of the public form from ReturnType<typeof signal<FormGroup>> to use the explicit Signal<FormGroup> type (or omit the type and rely on inference) and add an import for Signal from `@angular/core` alongside signal (e.g., import { Component, inject, signal, Signal } from '@angular/core'); update the property in member-form.component.ts (public form) so it uses Signal<FormGroup> or is untyped to simplify the code.
269-278: Consider a clearer validation error key.The generated error key
role_start_after_role_endreads awkwardly. Consider a more intuitive key likeinvalidDateRangeor structure it as an object with the field names.♻️ Optional improvement
private static dateRangeValidator(startKey: string, endKey: string) { return (group: AbstractControl): ValidationErrors | null => { const start = group.get(startKey)?.value; const end = group.get(endKey)?.value; if (start && end && new Date(start) > new Date(end)) { - return { [`${startKey}_after_${endKey}`]: true }; + return { invalidDateRange: { startKey, endKey } }; } return null; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 269 - 278, The dateRangeValidator currently returns an awkward error key (`${startKey}_after_${endKey}`); update the validator (dateRangeValidator) to return a clearer, structured error such as { invalidDateRange: { start: startKey, end: endKey } } or { dateRange: { start: startKey, end: endKey } } instead of the concatenated string, and ensure any consumers (form error checks) read the new shape (invalidDateRange or dateRange) when displaying/handling validation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html`:
- Around line 98-105: The table conditional rendering under
committee()?.enable_voting only shows "Role" and "Voting Status" columns while
the form captures governance fields appointed_by, role_start, role_end,
voting_status_start and voting_status_end; update the template
(committee-members.component.html) to add corresponding header columns and cells
for appointed_by and the two date ranges (role_start/role_end and
voting_status_start/voting_status_end) in both places noted (lines ~98-105 and
~133-140) so saved governance data is visible, or if intentional, add a TODO
comment and defer column rendering to a follow-up PR; ensure the cell bindings
reference the member model properties used by the form.
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 14-15: Replace the barrel imports in member-form.component.ts by
importing the constants and interfaces directly from their source modules:
instead of importing APPOINTED_BY_OPTIONS, LINKEDIN_PROFILE_PATTERN,
MEMBER_ROLES, VOTING_STATUSES from '@lfx-one/shared/constants' and Committee,
CommitteeMember, CreateCommitteeMemberRequest, MemberFormValue from
'@lfx-one/shared/interfaces', update the import statements to reference the
specific module files that export those symbols (the concrete files within the
shared package that define those constants and interfaces) so the component uses
direct shared imports per the coding guidelines.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 36-37: Replace the verbose type for the form signal property —
change the declaration of the public form from ReturnType<typeof
signal<FormGroup>> to use the explicit Signal<FormGroup> type (or omit the type
and rely on inference) and add an import for Signal from `@angular/core` alongside
signal (e.g., import { Component, inject, signal, Signal } from
'@angular/core'); update the property in member-form.component.ts (public form)
so it uses Signal<FormGroup> or is untyped to simplify the code.
- Around line 269-278: The dateRangeValidator currently returns an awkward error
key (`${startKey}_after_${endKey}`); update the validator (dateRangeValidator)
to return a clearer, structured error such as { invalidDateRange: { start:
startKey, end: endKey } } or { dateRange: { start: startKey, end: endKey } }
instead of the concatenated string, and ensure any consumers (form error checks)
read the new shape (invalidDateRange or dateRange) when displaying/handling
validation errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8778f55d-d617-4b01-934e-527902b48314
📒 Files selected for processing (3)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.tsapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts
asithade
requested changes
Mar 19, 2026
| <tr> | ||
| <td [attr.colspan]="committee()?.enable_voting ? 6 : 4" class="text-center py-8"> | ||
| <!-- Loading skeleton rows --> | ||
| <div class="flex flex-col gap-3 animate-pulse px-4"> |
Contributor
There was a problem hiding this comment.
Custom skeleton instead of PrimeNG <p-skeleton>.
Problem: The loading state uses hand-rolled skeleton markup with animate-pulse and bg-gray-200 divs.
Why it's a problem: The rest of the codebase (route-loading.component, metric-card.component) uses <p-skeleton> from primeng/skeleton for loading states. Custom skeletons create visual inconsistency (different animation timing, color, border-radius) and bypass theming provided by PrimeNG.
Fix: Replace the custom animate-pulse / bg-gray-200 div block with <p-skeleton> elements matching the table row layout. Import SkeletonModule from primeng/skeleton in the component's imports array.
| command: () => window.open(`mailto:${this.selectedMember()?.email}`, '_blank'), | ||
| command: () => { | ||
| if (isPlatformBrowser(this.platformId)) { | ||
| window.open(`mailto:${this.selectedMember()?.email}`, '_blank'); |
Contributor
There was a problem hiding this comment.
Use url on MenuItem instead of window.open for mailto.
Problem: The "Send Message" menu item uses window.open with a mailto: link, wrapped in an isPlatformBrowser guard.
Why it's a problem: PrimeNG's MenuItem supports a url property that renders a native <a href>, which is more accessible (supports right-click, copy link), doesn't require JavaScript execution, and is SSR-safe without needing the isPlatformBrowser guard. The template already uses [href] for the same mailto link in the non-manager case, so this is inconsistent.
Fix: Replace the command callback with url: \mailto:${this.selectedMember()?.email}`andtarget: '_blank'on theMenuItem. Remove the isPlatformBrowserimport andPLATFORM_ID` injection if no longer used elsewhere in the component.
| this.form().updateValueAndValidity(); | ||
| } | ||
|
|
||
| public onDateChange(): void { |
Contributor
There was a problem hiding this comment.
onDateChange() is redundant — Angular re-runs group validators automatically.
Problem: onDateChange() calls this.form().updateValueAndValidity() and is bound to (onSelect) on all four calendar inputs.
Why it's a problem: Angular's reactive forms already re-run group-level validators (including the dateRangeValidator registered on the FormGroup) automatically when any child control's value changes. The manual updateValueAndValidity() call is redundant.
Fix: Remove the onDateChange() method and remove all four (onSelect)="onDateChange()" bindings from the template. The date range validation will continue to work without them.
| email: new FormControl('', [Validators.required, Validators.email]), | ||
| job_title: new FormControl(''), | ||
| linkedin_profile: new FormControl('', [Validators.pattern(LINKEDIN_PROFILE_PATTERN)]), | ||
| is_individual: new FormControl(false), |
Contributor
There was a problem hiding this comment.
is_individual creates unnecessary complexity for a UI-only toggle.
Problem: is_individual is a UI-only form control that toggles whether the organization field is required/disabled. But organization was already optional (no validator) before this PR. This PR adds Validators.required to organization and then adds this checkbox + onIndividualToggle() + takeUntilDestroyed subscription to bypass that requirement — creating complexity to solve a problem it introduced. The field is never sent to the API; buildOrganizationPayload just checks it to return null.
Why it's a problem: It adds a form control, a valueChanges subscription, a toggle handler that enables/disables controls and swaps validators, and a MemberFormValue interface field — all for something the API doesn't know about. If "org is required unless explicitly individual" is a real business rule, it should come from the upstream API contract.
Fix: Clarify with product whether organization is truly required. If not, remove is_individual, remove Validators.required from organization, and let users simply leave it blank. If it is a real requirement, keep the validation but simplify — consider using a reactive disabled state driven by a signal rather than imperative enable/disable/clearValidators/setValidators calls.
- Replace custom animate-pulse skeleton with PrimeNG <p-skeleton> for consistency - Replace window.open mailto with MenuItem.url (more accessible, SSR-safe natively) - Remove isPlatformBrowser/PLATFORM_ID (no longer needed) - Remove redundant onDateChange() — Angular re-runs group validators automatically - Remove is_individual toggle and make organization optional (simplifies form, removes unnecessary complexity for a UI-only field the API doesn't know about) - Remove Validators.required from organization control - Remove is_individual from MemberFormValue interface LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
MRashad26
reviewed
Mar 19, 2026
Contributor
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
Review — 3 inline findings (1 blocking, 2 major)
| [rounded]="true" | ||
| size="small" | ||
| severity="secondary" | ||
| [href]="`mailto:${member.email}`" |
Contributor
There was a problem hiding this comment.
[Blocking] Template literal in Angular binding — will fail at runtime
[href]="`mailto:${member.email}`"Angular template expressions don't support JavaScript template literals (backtick strings). This will throw a template parse error at runtime.
Fix:
[href]="'mailto:' + member.email"| this.refreshMembers(); | ||
| }, | ||
| error: (error) => { | ||
| error: () => { |
Contributor
There was a problem hiding this comment.
[Major] Error variable discarded — delete failures are completely silent
error: () => {
this.isDeleting.set(false);
// error not captured at allThe error parameter was removed along with console.error, but no replacement logging or telemetry was added. In production, delete failures will be invisible to debugging. The member-form component in this same PR correctly types errors as HttpErrorResponse — apply the same pattern here:
error: (err: HttpErrorResponse) => {
this.isDeleting.set(false);
console.error('Failed to delete member:', err);
// ... existing messageService.addOr better yet, wire it into a centralized error handler if one exists.
| public isDeleting: WritableSignal<boolean>; | ||
| public memberActionMenuItems: MenuItem[] = []; | ||
| public committeeLabel = COMMITTEE_LABEL; | ||
| public isBoardMember: Signal<boolean>; |
Contributor
There was a problem hiding this comment.
[Major] Signals declared uninitialized, assigned in constructor — deviates from component-organization.md
Per .claude/rules/component-organization.md, computed signals should be initialized inline or via private init functions:
// Current (constructor assignment):
public isBoardMember: Signal<boolean>;
public isMaintainer: Signal<boolean>;
public canManageMembers: Signal<boolean>;
public isMembersVisible: Signal<boolean>;
// ... assigned later in constructor bodyShould be:
public readonly isBoardMember = computed(() => this.personaService.currentPersona() === 'board-member');
public readonly isMaintainer = computed(() => this.personaService.currentPersona() === 'maintainer');
public readonly canManageMembers = computed(() => !this.isBoardMember() && (!!this.committee()?.writer || this.isMaintainer()));
public readonly isMembersVisible = computed(() => {
const committee = this.committee();
if (!committee) return false;
return committee.member_visibility !== 'hidden' || this.canManageMembers();
});This makes dependencies visible at declaration site and adds readonly for safety.
- Fix template literal in [href] binding — use string concatenation instead of backtick syntax which Angular templates don't support (runtime parse error) - Type delete error as HttpErrorResponse and restore console.error for debugging - Move computed signals (isBoardMember, isMaintainer, canManageMembers, isMembersVisible) to inline declarations with readonly, per component-organization.md pattern - Remove WritableSignal import (no longer needed) LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts (1)
260-267:⚠️ Potential issue | 🟠 Major
mailtoURL is evaluated once at initialization — will not reflect the selected member.The
urlproperty is set wheninitializeMemberActionMenuItems()runs inngOnInit, at which pointthis.selectedMember()isnull. The URL becomes static (mailto:undefinedormailto:null) and won't update when a different member is selected.Consider using
commandinstead to dynamically construct the URL, or rebuild the menu items whenselectedMemberchanges:🐛 Proposed fix using command callback
return [ { label: 'Send Message', icon: 'fa-light fa-envelope', - url: `mailto:${this.selectedMember()?.email}`, - target: '_blank', + command: () => { + const email = this.selectedMember()?.email; + if (email) { + window.open(`mailto:${email}`, '_blank'); + } + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts` around lines 260 - 267, The menu's mailto URL is computed once in initializeMemberActionMenuItems() using this.selectedMember() (null at ngOnInit), so the Send Message item becomes a static "mailto:undefined"; change the item to use a dynamic command callback instead (replace the url property with a command: () => { const m = this.selectedMember(); if (m?.email) window.open(`mailto:${m.email}`, '_blank'); }) or alternatively regenerate the MenuItem list whenever the selection changes (call initializeMemberActionMenuItems() after updating selectedMember) so the mailto is always built from the current selectedMember().
♻️ Duplicate comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html (1)
159-159:⚠️ Potential issue | 🔴 CriticalTemplate literal syntax not supported in Angular template bindings.
Angular template expressions don't support JavaScript template literals (backtick strings). This will cause a template parse error at runtime.
🐛 Proposed fix
- [href]="`mailto:${member.email}`" + [href]="'mailto:' + member.email"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html` at line 159, The href binding in committee-members.component.html uses a JS template literal which Angular templates don't support; replace the backtick expression in the [href] binding with an Angular-compatible expression such as concatenation or interpolation so the mailto link is built from member.email (e.g., use [href] with string concatenation "'mailto:' + member.email" or use href with interpolation "mailto:{{member.email}}") to fix the template parse error.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts (1)
34-36: Consider initializing form signal inline.The
form!property with definite assignment assertion works since it's immediately assigned in the constructor, but initializing inline would be cleaner and avoid the assertion:public form = signal<FormGroup>(this.createMemberFormGroup());However, this requires
committeeto be available, which it is afterthis.committee = this.config.data?.committeeruns. The current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts` around lines 34 - 36, The form signal is currently declared with a definite-assignment assertion (public form!: ReturnType<typeof signal<FormGroup>>) though it can be initialized inline; update the declaration to initialize using signal<FormGroup>(this.createMemberFormGroup()) so the form is created immediately, ensuring this.committee is assigned first (after this.committee = this.config.data?.committee in the constructor) or move the committee assignment before the inline initialization; reference the form property and the createMemberFormGroup method to locate the change and ensure no runtime ordering issues with committee/config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Around line 260-267: The menu's mailto URL is computed once in
initializeMemberActionMenuItems() using this.selectedMember() (null at
ngOnInit), so the Send Message item becomes a static "mailto:undefined"; change
the item to use a dynamic command callback instead (replace the url property
with a command: () => { const m = this.selectedMember(); if (m?.email)
window.open(`mailto:${m.email}`, '_blank'); }) or alternatively regenerate the
MenuItem list whenever the selection changes (call
initializeMemberActionMenuItems() after updating selectedMember) so the mailto
is always built from the current selectedMember().
---
Duplicate comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html`:
- Line 159: The href binding in committee-members.component.html uses a JS
template literal which Angular templates don't support; replace the backtick
expression in the [href] binding with an Angular-compatible expression such as
concatenation or interpolation so the mailto link is built from member.email
(e.g., use [href] with string concatenation "'mailto:' + member.email" or use
href with interpolation "mailto:{{member.email}}") to fix the template parse
error.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.ts`:
- Around line 34-36: The form signal is currently declared with a
definite-assignment assertion (public form!: ReturnType<typeof
signal<FormGroup>>) though it can be initialized inline; update the declaration
to initialize using signal<FormGroup>(this.createMemberFormGroup()) so the form
is created immediately, ensuring this.committee is assigned first (after
this.committee = this.config.data?.committee in the constructor) or move the
committee assignment before the inline initialization; reference the form
property and the createMemberFormGroup method to locate the change and ensure no
runtime ordering issues with committee/config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b4a0a36-253c-4187-b1b1-41d39573acd2
📒 Files selected for processing (5)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.tsapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.htmlapps/lfx-one/src/app/modules/committees/components/member-form/member-form.component.tspackages/shared/src/interfaces/member.interface.ts
✅ Files skipped from review due to trivial changes (1)
- packages/shared/src/interfaces/member.interface.ts
MenuItem.url is evaluated once at ngOnInit when selectedMember is null, producing a static "mailto:undefined". Switch back to command callback with window.location.href for dynamic member resolution. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
MRashad26
previously approved these changes
Mar 19, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts (1)
255-261:⚠️ Potential issue | 🟠 MajorRecompute the menu model from the clicked member.
initializeMemberActionMenuItems()snapshots signal-derived values into a plain array. Because it only runs once, Line 260 is built before any row is selected, so the "Send Message" action staysmailto:undefined; the same pattern also leaves the remove item's disabled state stale. Rebuild the array whentoggleMemberActionMenu()sets the active member, or pass the clicked member into the initializer before opening the menu.Possible fix
public toggleMemberActionMenu(event: Event, member: CommitteeMember, menuComponent: MenuComponent): void { event.stopPropagation(); this.selectedMember.set(member); + this.memberActionMenuItems = this.initializeMemberActionMenuItems(member); menuComponent.toggle(event); } -private initializeMemberActionMenuItems(): MenuItem[] { +private initializeMemberActionMenuItems(member: CommitteeMember | null = this.selectedMember()): MenuItem[] { return [ { label: 'Send Message', icon: 'fa-light fa-envelope', - url: `mailto:${this.selectedMember()?.email}`, + disabled: !member?.email, + url: member?.email ? `mailto:${member.email}` : undefined, target: '_blank', },Also applies to: 275-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts` around lines 255 - 261, The menu items are built once by initializeMemberActionMenuItems() using signal-derived values, causing stale mailto and disabled states; change initializeMemberActionMenuItems to accept the clicked member (e.g., member: CommitteeMember) or rebuild the array inside toggleMemberActionMenu() after you set the active member so the 'Send Message' url uses member.email and the remove menu item's disabled state is computed from that member (also update the similar block at lines ~275-279). Locate initializeMemberActionMenuItems, toggleMemberActionMenu, and selectedMember references and ensure the menu array is recreated with the current member before opening the action menu.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts (1)
23-23: Wrap the newSkeletondependency instead of importing PrimeNG directly.Please verify whether the repo already has an LFX skeleton wrapper and use that here; otherwise this component is expanding the raw PrimeNG UI surface again.
As per coding guidelines "All PrimeNG components must be wrapped in LFX components for UI library independence".
Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts` at line 23, The component currently imports PrimeNG's Skeleton directly; replace that raw import by using the project's LFX skeleton wrapper (search for a symbol like LfxSkeleton, LfxSkeletonComponent, or SkeletonWrapper) and import that instead in committee-members.component (also replace the other direct import at the second occurrence). If no wrapper exists, add a small LFX wrapper component (e.g., LfxSkeletonComponent) that internally uses PrimeNG's Skeleton and re-exports the same input/output API, then update committee-members.component to import and use that wrapper rather than importing 'primeng/skeleton' directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Around line 255-261: The menu items are built once by
initializeMemberActionMenuItems() using signal-derived values, causing stale
mailto and disabled states; change initializeMemberActionMenuItems to accept the
clicked member (e.g., member: CommitteeMember) or rebuild the array inside
toggleMemberActionMenu() after you set the active member so the 'Send Message'
url uses member.email and the remove menu item's disabled state is computed from
that member (also update the similar block at lines ~275-279). Locate
initializeMemberActionMenuItems, toggleMemberActionMenu, and selectedMember
references and ensure the menu array is recreated with the current member before
opening the action menu.
---
Nitpick comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts`:
- Line 23: The component currently imports PrimeNG's Skeleton directly; replace
that raw import by using the project's LFX skeleton wrapper (search for a symbol
like LfxSkeleton, LfxSkeletonComponent, or SkeletonWrapper) and import that
instead in committee-members.component (also replace the other direct import at
the second occurrence). If no wrapper exists, add a small LFX wrapper component
(e.g., LfxSkeletonComponent) that internally uses PrimeNG's Skeleton and
re-exports the same input/output API, then update committee-members.component to
import and use that wrapper rather than importing 'primeng/skeleton' directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a22d5e0-1667-49b2-99a2-7a299d065ec5
📒 Files selected for processing (2)
apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.htmlapps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/modules/committees/components/committee-members/committee-members.component.html
asithade
reviewed
Mar 19, 2026
Contributor
There was a problem hiding this comment.
Mailto link should use MenuItem.url instead of imperative window.location.href.
Problem: The "Send Message" menu item still uses imperative JavaScript (window.location.href = \mailto:${email}`) inside a commandcallback. While this is better than the previouswindow.open` approach, it bypasses PrimeNG's built-in link handling.
Why it matters: PrimeNG's MenuItem interface has a url property that renders the item as a native <a> element. Using url is simpler, SSR-safe (no window reference), and follows how PrimeNG menu items are designed to handle navigation.
Fix: Replace the command callback with the url property. Set url dynamically when selecting a member:
// In toggleMemberActionMenu or wherever memberActionMenuItems is built:
{
label: 'Send Message',
icon: 'fa-light fa-envelope',
url: `mailto:${member.email}`,
}Remove the command callback entirely — PrimeNG will render this as a native <a href="mailto:..."> link automatically.
Rebuild memberActionMenuItems in toggleMemberActionMenu with the selected member so MenuItem.url is set to the correct mailto: at click time. PrimeNG renders this as a native <a href> — SSR-safe, accessible, no window reference. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
asithade
approved these changes
Mar 19, 2026
jordancclive
pushed a commit
that referenced
this pull request
Mar 19, 2026
…anced form (#342) * feat(committees): refactor members tab with governance fields and enhanced form - Add MemberFormValue interface to shared package for typed form values - Add governance columns (appointed by, voting status, role dates) to members table - Add member_visibility gate to control column visibility based on committee settings - Add data-testid attributes for reliable test targeting - Add organization search with autocomplete in member form - Add individual toggle to disable org fields when member has no org affiliation - Add date pickers for role and voting status start/end dates - Add LinkedIn profile field to member form - Add ClearableEndDateValidator for cross-field date validation - Add buildOrganizationPayload helper for org data serialization - Use getRawValue() to capture disabled field values in form submission LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address review findings on members tab PR - Default isMembersVisible to false while committee is loading (fail closed for privacy — was flashing members before committee metadata arrived) - Add rel="noopener noreferrer" to org website link (tabnabbing prevention) - Move createMemberFormGroup() into constructor body after this.committee is assigned from config.data, so enable_voting validators are correctly applied LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address human review findings on members tab - Replace custom animate-pulse skeleton with PrimeNG <p-skeleton> for consistency - Replace window.open mailto with MenuItem.url (more accessible, SSR-safe natively) - Remove isPlatformBrowser/PLATFORM_ID (no longer needed) - Remove redundant onDateChange() — Angular re-runs group validators automatically - Remove is_individual toggle and make organization optional (simplifies form, removes unnecessary complexity for a UI-only field the API doesn't know about) - Remove Validators.required from organization control - Remove is_individual from MemberFormValue interface LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address MRashad26 review findings on members tab - Fix template literal in [href] binding — use string concatenation instead of backtick syntax which Angular templates don't support (runtime parse error) - Type delete error as HttpErrorResponse and restore console.error for debugging - Move computed signals (isBoardMember, isMaintainer, canManageMembers, isMembersVisible) to inline declarations with readonly, per component-organization.md pattern - Remove WritableSignal import (no longer needed) LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use dynamic command for mailto instead of static url MenuItem.url is evaluated once at ngOnInit when selectedMember is null, producing a static "mailto:undefined". Switch back to command callback with window.location.href for dynamic member resolution. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use MenuItem.url for mailto with dynamic member rebuild Rebuild memberActionMenuItems in toggleMemberActionMenu with the selected member so MenuItem.url is set to the correct mailto: at click time. PrimeNG renders this as a native <a href> — SSR-safe, accessible, no window reference. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> --------- Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jordan Clive <jclive@linuxfoundation.org>
manishdixitlfx
added a commit
that referenced
this pull request
Mar 31, 2026
…anced form (#342) * feat(committees): refactor members tab with governance fields and enhanced form - Add MemberFormValue interface to shared package for typed form values - Add governance columns (appointed by, voting status, role dates) to members table - Add member_visibility gate to control column visibility based on committee settings - Add data-testid attributes for reliable test targeting - Add organization search with autocomplete in member form - Add individual toggle to disable org fields when member has no org affiliation - Add date pickers for role and voting status start/end dates - Add LinkedIn profile field to member form - Add ClearableEndDateValidator for cross-field date validation - Add buildOrganizationPayload helper for org data serialization - Use getRawValue() to capture disabled field values in form submission LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address review findings on members tab PR - Default isMembersVisible to false while committee is loading (fail closed for privacy — was flashing members before committee metadata arrived) - Add rel="noopener noreferrer" to org website link (tabnabbing prevention) - Move createMemberFormGroup() into constructor body after this.committee is assigned from config.data, so enable_voting validators are correctly applied LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address human review findings on members tab - Replace custom animate-pulse skeleton with PrimeNG <p-skeleton> for consistency - Replace window.open mailto with MenuItem.url (more accessible, SSR-safe natively) - Remove isPlatformBrowser/PLATFORM_ID (no longer needed) - Remove redundant onDateChange() — Angular re-runs group validators automatically - Remove is_individual toggle and make organization optional (simplifies form, removes unnecessary complexity for a UI-only field the API doesn't know about) - Remove Validators.required from organization control - Remove is_individual from MemberFormValue interface LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address MRashad26 review findings on members tab - Fix template literal in [href] binding — use string concatenation instead of backtick syntax which Angular templates don't support (runtime parse error) - Type delete error as HttpErrorResponse and restore console.error for debugging - Move computed signals (isBoardMember, isMaintainer, canManageMembers, isMembersVisible) to inline declarations with readonly, per component-organization.md pattern - Remove WritableSignal import (no longer needed) LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use dynamic command for mailto instead of static url MenuItem.url is evaluated once at ngOnInit when selectedMember is null, producing a static "mailto:undefined". Switch back to command callback with window.location.href for dynamic member resolution. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use MenuItem.url for mailto with dynamic member rebuild Rebuild memberActionMenuItems in toggleMemberActionMenu with the selected member so MenuItem.url is set to the correct mailto: at click time. PrimeNG renders this as a native <a href> — SSR-safe, accessible, no window reference. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> --------- Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MRashad26
pushed a commit
that referenced
this pull request
Apr 1, 2026
…anced form (#342) * feat(committees): refactor members tab with governance fields and enhanced form - Add MemberFormValue interface to shared package for typed form values - Add governance columns (appointed by, voting status, role dates) to members table - Add member_visibility gate to control column visibility based on committee settings - Add data-testid attributes for reliable test targeting - Add organization search with autocomplete in member form - Add individual toggle to disable org fields when member has no org affiliation - Add date pickers for role and voting status start/end dates - Add LinkedIn profile field to member form - Add ClearableEndDateValidator for cross-field date validation - Add buildOrganizationPayload helper for org data serialization - Use getRawValue() to capture disabled field values in form submission LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address review findings on members tab PR - Default isMembersVisible to false while committee is loading (fail closed for privacy — was flashing members before committee metadata arrived) - Add rel="noopener noreferrer" to org website link (tabnabbing prevention) - Move createMemberFormGroup() into constructor body after this.committee is assigned from config.data, so enable_voting validators are correctly applied LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address human review findings on members tab - Replace custom animate-pulse skeleton with PrimeNG <p-skeleton> for consistency - Replace window.open mailto with MenuItem.url (more accessible, SSR-safe natively) - Remove isPlatformBrowser/PLATFORM_ID (no longer needed) - Remove redundant onDateChange() — Angular re-runs group validators automatically - Remove is_individual toggle and make organization optional (simplifies form, removes unnecessary complexity for a UI-only field the API doesn't know about) - Remove Validators.required from organization control - Remove is_individual from MemberFormValue interface LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address MRashad26 review findings on members tab - Fix template literal in [href] binding — use string concatenation instead of backtick syntax which Angular templates don't support (runtime parse error) - Type delete error as HttpErrorResponse and restore console.error for debugging - Move computed signals (isBoardMember, isMaintainer, canManageMembers, isMembersVisible) to inline declarations with readonly, per component-organization.md pattern - Remove WritableSignal import (no longer needed) LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use dynamic command for mailto instead of static url MenuItem.url is evaluated once at ngOnInit when selectedMember is null, producing a static "mailto:undefined". Switch back to command callback with window.location.href for dynamic member resolution. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use MenuItem.url for mailto with dynamic member rebuild Rebuild memberActionMenuItems in toggleMemberActionMenu with the selected member so MenuItem.url is set to the correct mailto: at click time. PrimeNG renders this as a native <a href> — SSR-safe, accessible, no window reference. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> --------- Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
MRashad26
pushed a commit
that referenced
this pull request
Apr 1, 2026
…anced form (#342) * feat(committees): refactor members tab with governance fields and enhanced form - Add MemberFormValue interface to shared package for typed form values - Add governance columns (appointed by, voting status, role dates) to members table - Add member_visibility gate to control column visibility based on committee settings - Add data-testid attributes for reliable test targeting - Add organization search with autocomplete in member form - Add individual toggle to disable org fields when member has no org affiliation - Add date pickers for role and voting status start/end dates - Add LinkedIn profile field to member form - Add ClearableEndDateValidator for cross-field date validation - Add buildOrganizationPayload helper for org data serialization - Use getRawValue() to capture disabled field values in form submission LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address review findings on members tab PR - Default isMembersVisible to false while committee is loading (fail closed for privacy — was flashing members before committee metadata arrived) - Add rel="noopener noreferrer" to org website link (tabnabbing prevention) - Move createMemberFormGroup() into constructor body after this.committee is assigned from config.data, so enable_voting validators are correctly applied LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address human review findings on members tab - Replace custom animate-pulse skeleton with PrimeNG <p-skeleton> for consistency - Replace window.open mailto with MenuItem.url (more accessible, SSR-safe natively) - Remove isPlatformBrowser/PLATFORM_ID (no longer needed) - Remove redundant onDateChange() — Angular re-runs group validators automatically - Remove is_individual toggle and make organization optional (simplifies form, removes unnecessary complexity for a UI-only field the API doesn't know about) - Remove Validators.required from organization control - Remove is_individual from MemberFormValue interface LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): address MRashad26 review findings on members tab - Fix template literal in [href] binding — use string concatenation instead of backtick syntax which Angular templates don't support (runtime parse error) - Type delete error as HttpErrorResponse and restore console.error for debugging - Move computed signals (isBoardMember, isMaintainer, canManageMembers, isMembersVisible) to inline declarations with readonly, per component-organization.md pattern - Remove WritableSignal import (no longer needed) LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use dynamic command for mailto instead of static url MenuItem.url is evaluated once at ngOnInit when selectedMember is null, producing a static "mailto:undefined". Switch back to command callback with window.location.href for dynamic member resolution. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> * fix(committees): use MenuItem.url for mailto with dynamic member rebuild Rebuild memberActionMenuItems in toggleMemberActionMenu with the selected member so MenuItem.url is set to the correct mailto: at click time. PrimeNG renders this as a native <a href> — SSR-safe, accessible, no window reference. LFXV2-1190 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> --------- Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MemberFormValueinterface to@lfx-one/sharedfor typed form valuesmember_visibilitygate to control column visibility based on committee settingsClearableEndDateValidatorfor cross-field date validationgetRawValue()to capture disabled field values in form submissiondata-testidattributes for reliable test targetingSplit Context
This is PR 1 of 2 splitting #335 into focused PRs:
BFF endpoints for meetings/surveys and votes/surveys tab components are deferred to a follow-up effort (tabs remain as "Coming in a future PR" placeholders).
Test Plan
member_visibilityallowsdata-testidattributes present on key elementsyarn lint && yarn buildpasses ✅LFXV2-1190
🤖 Generated with Claude Code