Skip to content

feat(committees): add committee detail shell with overview tab and BFF endpoints#335

Closed
manishdixitlfx wants to merge 56 commits intomainfrom
feat/LFXV2-committees-detail-overview
Closed

feat(committees): add committee detail shell with overview tab and BFF endpoints#335
manishdixitlfx wants to merge 56 commits intomainfrom
feat/LFXV2-committees-detail-overview

Conversation

@manishdixitlfx
Copy link
Copy Markdown
Contributor

@manishdixitlfx manishdixitlfx commented Mar 18, 2026

Summary

  • Add committee detail shell with tabbed layout (overview, members, votes, meetings, surveys, documents)
  • Add overview tab with leadership, membership stats, and key topics
  • Add BFF endpoints for votes (with fallback), meetings, surveys, join/leave, and my-committees
  • Wire settings tab with committee configuration display
  • Wire meetings tab with upcoming/past meeting views
  • Wire votes tab with paginated table and results drawer
  • Wire members tab with full CRUD, search/filter, and governance fields
  • Add leadership dialog for assigning/removing chair and co-chair
  • Add committee create/edit form enhancements
  • Add surveys tab placeholder (content in separate PR)

Endpoints added

Method Route Description
GET /committees/my-committees User's committees with role
GET /:id/votes Committee votes (with vote-type fallback)
GET /:id/meetings Committee meetings
GET /:id/surveys Committee surveys
POST /:id/join Self-join open committee
DELETE /:id/leave Leave committee

Existing CRUD endpoints (committees, members, settings) were already present.

Test plan

  • Navigate to a committee → detail shell loads with tabs
  • Overview tab shows leadership, membership stats, key topics
  • Members tab shows filterable member list with add/edit/delete
  • Votes tab shows paginated votes with status filter and results drawer
  • Meetings tab shows upcoming/past meetings with filter toggle
  • Settings tab shows committee configuration (PMO only)
  • Leadership dialog assigns/removes chair and co-chair
  • Join/leave committee works from overview
  • BFF endpoints return correct data

LFXV2-1218

🤖 Generated with Claude Code

manishdixitlfx and others added 22 commits March 18, 2026 08:07
Rewrite committee-view component with PrimeNG tab shell and overview tab
using only getCommitteeById data. No sub-resource API calls.

- Add 6-tab layout: Overview, Members, Votes, Meetings, Surveys, Documents
- Overview tab: stats row, channels card, configurations card, leadership card
- Tab visibility: Members hidden when member_visibility=hidden, Votes hidden when voting disabled
- Loading skeleton, error state with back navigation
- Placeholder content for tabs coming in future PRs
- Computed signals for categorySeverity, breadcrumbs, joinModeLabel, leadership dates

LFXV2-1255

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…source endpoints

- Add 10 BFF sub-resource endpoints (votes, resolutions, activity, contributors,
  deliverables, discussions, events, campaigns, engagement, budget) with
  controller, routes, and service methods
- Add frontend committee service methods for all sub-resource endpoints
- Extend overview tab with behavioral-class-specific dashboard cards:
  - Governance: open votes, budget summary, resolutions
  - Oversight/Working Group: activity feed, top contributors
  - Working Group: deliverables & milestones
  - Special Interest Group: discussion threads, upcoming events
  - Ambassador Program: outreach campaigns, engagement metrics
- Add sidebar cards: org breakdown, role breakdown
- Add leadership edit UI with assign/remove dialog component
- Wire loadGroupTypeData into observable chain for proper cancellation
- Use real member data for stats (totalMembers, activeVoters, orgCount)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
- Fix committeeSignal not set for 'other' behavioral class (EMPTY → of(null))
- Fix vote DTO camelCase keys to match snake_case CommitteeVote interface
- Fix getCommitteeEngagement returning {} instead of null on failure
- Fix activeTab two-way binding: use explicit [value]/valueChange pattern
- Set committeeSignal before loading sub-resources to prevent stale state

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Port missing logic from reference branch into member-form and
committee-members components, then wire CommitteeMembersComponent
into the committee-view Members tab replacing the placeholder.

Member form: individual toggle, date range validators, clear buttons,
appointed-by select, required validators, buildOrganizationPayload,
getRawValue, SSR-safe patterns, data-testid attributes.

Committee members: isPlatformBrowser SSR guard, GroupBehavioralClass
input, isMaintainer/isMembersVisible signals, type-specific empty
states, skeleton loading, visibility gate. InviteMemberDialog deferred
to PR 5.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Add member action dialog components ported from the reference branch:
- InviteMemberDialogComponent: email invite with parsing and dedup
- JoinApplicationDialogComponent: join request form with reason field
- ApplicationReviewComponent: inline pending-application review card

Wire invite button into committee-members and application-review into
the committee-view Members tab. Add full BFF layer (routes, controller
with param validation, typed service methods) for invite and application
endpoints proxying to lfx-v2-committee-service.

LFXV2-committees

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…297

- Remove permission bypass in canInviteMembers (use canManageMembers only)
- Show invalid email addresses instead of silently dropping them
- Preserve line breaks in application reason with whitespace-pre-wrap
- Extract inline ServiceValidationError to variables in controller
- Add trimmedMinLength validator for join-application reason field
- Guard effect() to skip API calls when join_mode !== 'apply' or no writer

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Wire the CommitteeSettingsComponent into the committee detail view as a
writer-guarded Settings tab. Admins can manage visibility, voting mode,
join mode, SSO group, meeting attendees, and other feature toggles.

- Add join mode dropdown to CommitteeSettingsComponent (was missing)
- Create settings form group in committee-view with save functionality
- Populate settings form from committee data on load
- Guard settings tab behind canManageConfigurations check

LFXV2-1255

Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Wire the Meetings tab in committee detail view with real meeting data,
replacing the placeholder. Fetches meetings via MeetingService using the
committee's project_uid and filters by committee uid.

- Add upcoming/past toggle with MeetingCardComponent rendering
- Fetch meetings during committee load via getMeetingsByProject
- Add createMeeting navigation with committee context
- Handle empty states for both upcoming and past views
- Fix meeting time comparison to use current time (CodeRabbit finding)

LFXV2-1255

Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Add dedicated GET /committees/:id/meetings BFF endpoint that filters
by committee_uid instead of relying on project-level meeting queries.
Update JoinMode type to match upstream service values (invite_only,
application) and remove client-side committees?.some() filtering that
always returned empty results.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…sibility

- Fix pastCommitteeMeetings type mismatch: pass pastMeeting=false since
  committee meetings are Meeting[] not PastMeeting[] (prevents 404s)
- Fix Create Meeting CTA to use canManageConfigurations instead of !isBoardMember
- Decouple meetings fetch from committee render path with independent
  meetingsLoading signal and loading skeleton
- Add aria-pressed to Upcoming/Past toggle buttons and role="group" wrapper
- Add TODO for stale upcoming/past split recomputation
- Fix JoinMode doc comment: 'apply' → 'application' to match type
- Derive joinable from committee.join_mode instead of hardcoding false

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Align committee-manage component with reference branch:

- Add missing join_mode form control and populate logic for edit mode
- Remove console.error calls (anti-pattern per project rules)
- All sub-components, routes, BFF endpoints, and entry points already
  existed from prior PRs in the stack

LFXV2-1255

Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Replace the placeholder votes tab panel with a full implementation showing
open votes with progress bars, past/closed votes list, and recent resolutions.
Add computed signals to filter votes by status and an effect to redirect to
overview when voting is disabled.

LFXV2-XXX

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Reuse the existing VoteResultsDrawerComponent so clicking any vote card
(open or closed) in the committee view opens the same side drawer used
by the main Votes page. Also makes overview tab vote cards clickable.

LFXV2-XXX

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…e UI

Extract votes tab into CommitteeVotesListComponent that wraps VotesTableComponent
and VoteResultsDrawerComponent — same paginated table, search, status filter,
and side-drawer experience as the main Votes page, scoped to the committee via
committee_name filter. Remove card-based layout and inline drawer from
committee-view.

LFXV2-XXX

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
- Track committeeName input in both combineLatest pipelines so votes
  refetch when the committee changes without destroying the component.
- Add hideGroupFilter input to VotesTableComponent and use it from the
  committee votes list to properly hide the group dropdown instead of
  passing an empty array.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Reset the totalRecords signal to 0 in the catchError handler so the
paginator doesn't show a stale count when the API call fails.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Adds a 5th stat card displaying the observer count alongside Members,
Voting Reps, Organizations, and Join Mode. Observers are a distinct
role class tracked separately by EDs and governance stakeholders.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Adds a scannable chip/tag list of key topics below the group description.
Allows stakeholders to assess a working group's focus areas at a glance
without reading a full paragraph.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Adds a collapsed summary card for the most recent meeting to the overview
tab. EDs and governance stakeholders check meeting activity first — this
surfaces that signal without requiring navigation to the Meetings tab.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…ments

- Sort past meetings by start_time descending before selecting most recent
- Compose summary fetch into RxJS chain to eliminate unmanaged subscription
- Gate meeting summary card on summary.approved for non-admin users
- Remove LFXV2-XXXX placeholder from key topics comment

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
…render path

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
… flow

- Add GET /committees/:id/surveys BFF endpoint with committee_uid filtering
- Add committee-surveys-list component with loading/error/empty states
- Add signal-driven tab navigation (Members | Surveys) to committee view
- Add "Create Survey" button gated by isBoardMember permission
- Add committee preselection in survey create form from query params

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
@manishdixitlfx manishdixitlfx requested a review from a team as a code owner March 18, 2026 15:48
Copilot AI review requested due to automatic review settings March 18, 2026 15:48
…rveys-tab

feat(committees): add surveys tab with committee filtering and create survey flow

Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
error: (error) => this.handleCommitteeError(error, 'update'),
error: () => this.handleCommitteeError('update'),
});
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent error swallowing — All console.error calls were removed from error handlers here (and in several other places in this file) but no replacement logging or telemetry was added. If there's a global HTTP error interceptor that handles this, this is fine — but if not, debugging production issues will be very difficult since errors are silently discarded. Please verify centralized error handling covers these cases, or consider keeping at minimum a structured log call.

return [];
}

const { resources: voteResources } = await this.microserviceProxy.proxyRequest<
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded page_size of 100 may silently truncate results — The TODO acknowledges this, and the warning log is a good safety net. However, for committees with heavy vote activity, users will see incomplete data with no indication in the UI. Consider either:

  1. Implementing cursor-based pagination to fetch all results
  2. Surfacing a "showing first 100 votes" indicator in the UI
  3. At minimum, bumping this to a higher limit with a follow-up ticket to paginate properly

// SPDX-License-Identifier: MIT

/**
* Status of a committee join application
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted committee-application.interface.ts — This removes CommitteeJoinApplication and CreateCommitteeJoinApplicationRequest types and their barrel export. Please verify nothing else in the monorepo or downstream consumers imports these types. If they were genuinely unused, this cleanup is fine — but if any other package or branch (especially the stacked PRs) references them, this will break.

- Restore console.error in committee-manage error handler for debugging
- Bump vote fallback page_size from 100 to 500 to reduce truncation risk
- Add LFXV2-1218 ticket reference to pagination TODO

LFXV2-1218

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
niravpatel27
niravpatel27 previously approved these changes Mar 19, 2026
dealako
dealako previously approved these changes Mar 19, 2026
Remove 9 BFF endpoints that call upstream APIs which don't exist yet:
- engagement, budget (not in committee-service OpenAPI spec)
- resolutions, activity, contributors, deliverables, discussions,
  events, campaigns (query resource types not indexed)
- PATCH channels update (not supported by upstream PUT-only contract)

Keep votes endpoint (has working fallback via vote resource type)
and surveys tab placeholder for future PR.

LFXV2-1218

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Surveys tab now uses a placeholder — the component import is no longer
needed and was causing an NG8113 build warning.

LFXV2-1218

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
Copy link
Copy Markdown
Contributor

@MRashad26 MRashad26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — 8 inline findings (2 original unresolved + 6 new)

* Fetches meetings associated with a committee.
*/
public async getCommitteeMeetings(req: Request, committeeId: string, query: Record<string, any> = {}): Promise<PaginatedResponse<Meeting>> {
try {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Original #1 — still present] Record<string, any> instead of typed query params

Per development-rules.md: "Use TypeScript types and interfaces for all data". There are 5 occurrences of Record<string, any> across this PR for query params. Define typed interfaces instead:

interface CommitteeMeetingQuery { page_size?: number; page_token?: string; order_by?: string; }
interface CommitteeSurveyQuery { page_size?: number; status?: string; }

This applies to both getCommitteeMeetings and getCommitteeSurveys signatures, as well as the controller callsites.


// Dashboard sub-resource methods — error handling is done at the component layer
public getCommitteeVotes(committeeId: string): Observable<CommitteeVote[]> {
return this.http.get<CommitteeVote[]>(`/api/committees/${committeeId}/votes`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Original #9 — not addressed] getCommitteeVotes() is dead code — never called from any component

The BFF exposes GET /committees/:id/votes and this client method calls it. However, committee-votes-list.component.ts completely ignores both and instead calls voteService.getVotesByProjectPaginated() + voteService.getVotesCountByProject() with a committee_name filter.

This means:

  • This method is unreachable dead code
  • The BFF getCommitteeVotes endpoint is also unreachable
  • Two different filtering strategies exist (server: committee_uid tag, client: committee_name filter)

Either wire the component to use this method (and the BFF endpoint), or remove both.

// Assign all responses to votes_for so UI progress bars reflect the
// real response count instead of showing misleading zeros.
votes_for: totalResponses,
votes_against: 0,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #1] votes_for assigned total responses — misleading data

The fallback vote mapping assigns ALL responses as affirmative:

votes_for: totalResponses,
votes_against: 0,
votes_abstain: 0,

This makes the UI show 100% approval for every vote. The comment says "so UI progress bars reflect the real response count instead of showing misleading zeros" — but showing 100% approval is equally misleading.

Consider either:

  1. Exposing total_responses without pretending they're all affirmative
  2. Adding a breakdown_available: false flag so the UI can render a "breakdown unavailable" state instead of fake vote counts

});
} catch (error) {
logger.warning(req, 'get_committee_votes', 'Failed to fetch committee votes, returning empty', {
committee_uid: committeeId,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #2] Silent error swallowing across multiple layers

This catch returns [] with a logger.warning — meaning upstream 500 errors are silently converted to empty results. The same pattern exists in getCommitteeMeetings (returns { data: [] }). Combined with client-side catchError(() => of([])) in committee-surveys-list and committee-votes-list, errors are swallowed at every layer.

The user sees an empty state with no error indication. The surveys list at least has a loadError signal — votes list doesn't.

At minimum, the BFF should propagate 5xx errors (only catch 4xx gracefully). Let components decide how to handle errors rather than hiding them.

* Fetches surveys for a specific committee by committee_uid
*/
public async getCommitteeSurveys(req: Request, committeeId: string, query: Record<string, any> = {}): Promise<Survey[]> {
logger.debug(req, 'get_committee_surveys', 'Fetching surveys for committee', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #3] Record<string, any> in survey service — same typing issue

public async getCommitteeSurveys(req: Request, committeeId: string, query: Record<string, any> = {}): Promise<Survey[]> {

Same as the committee service — should use a typed interface for the query params instead of Record<string, any>.

}

const surveys = await this.surveyService.getCommitteeSurveys(req, id, req.query as Record<string, any>);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #5] Unsafe req.query passthrough — parameter injection risk

const surveys = await this.surveyService.getCommitteeSurveys(req, id, req.query as Record<string, any>);

In the survey service, req.query is spread into the upstream params:

const params = { ...query, type: 'survey', tags: `committee_uid:${committeeId}` };

Since query is spread before type and tags, a request like ?type=user&tags=admin would be overridden — but query can still inject arbitrary params that get passed upstream (e.g. ?page_size=999999).

The getCommitteeMeetings service correctly whitelists allowed params. Apply the same pattern here:

const allowedParams = ['page_size', 'page_token', 'status'];
const sanitized: Record<string, string> = {};
for (const key of allowedParams) { if (query[key]) sanitized[key] = String(query[key]); }

});

if (voteResources.length >= 500) {
logger.warning(req, 'get_committee_votes', 'Vote results may be truncated — page_size limit reached', {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #6] Page truncation logged but silently returned

if (voteResources.length >= 500) {
  logger.warning(req, 'get_committee_votes', 'Vote results may be truncated...');
}

Logs a server warning but returns the truncated data without any indication to the client. The UI will show the first 500 votes as if that's the complete set.

Consider returning a truncated: true flag in the response, or implementing cursor-based pagination (the TODO comment at line 415 acknowledges this). At minimum, add a response header or metadata field so the client can show a "showing first 500 results" notice.

takeUntilDestroyed()
)
.subscribe();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New #8] Component bypasses dedicated BFF endpoint — creates dead code and dual strategies

This component calls voteService.getVotesCountByProject() and voteService.getVotesByProjectPaginated() with a committee_name filter. However, the BFF has a dedicated GET /committees/:id/votes endpoint (with fallback logic and committee_uid tag filtering), and the client has CommitteeService.getCommitteeVotes().

This means:

  • BFF getCommitteeVotes is dead code (no caller)
  • Client CommitteeService.getCommitteeVotes() is dead code (no caller)
  • Server filters by committee_uid tag, but this component filters by committee_name string — different strategies that may produce different results

Either wire this component to use the dedicated committee votes endpoint, or remove the BFF endpoint and client method to avoid confusion.

Copy link
Copy Markdown
Contributor

@asithade asithade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — 13 findings

Good progress on the committee detail shell. The BFF endpoints, tab layout, and member form enhancements are well-structured. Flagging the items below for cleanup before merge.

See inline comments for details.

this.refresh.update((v) => v + 1);
}

public createSurvey(): void {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code — createSurvey() is never called.

Problem: This method is defined but never referenced from the template or any other component.

Why it's a problem: Dead code adds confusion — future developers may assume it's wired up somewhere.

Fix: Either remove the method entirely, or if it's intended for a "Create Survey" button in the surveys tab, wire it to a button in committee-view.component.html with a click handler.

private handleCommitteeError(operation: 'create' | 'update', error?: unknown): void {
this.submitting.set(false);

console.error(`Failed to ${operation} committee:`, error);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover console.error.

Problem: handleCommitteeError still calls console.error here.

Why it's a problem: Every other error handler modified in this PR removed console.error in favor of toast-only feedback (e.g., createMemberOperation, the two error: () handlers above). Leaving this one creates an inconsistency and leaks error details to the browser console in production.

Fix: Remove the console.error line. The messageService.add() toast below already handles user-facing feedback. If server-side visibility is needed, the backend controller already logs the error.

@@ -38,9 +38,6 @@ <h2 class="text-lg font-semibold text-gray-900 mb-2">Something Went Wrong</h2>
<lfx-breadcrumb [model]="breadcrumbItems()" data-testid="committee-view-breadcrumb"></lfx-breadcrumb>
<div class="flex items-center gap-3">
<h1 data-testid="committee-view-name">{{ committee()?.name }}</h1>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Was removing the lock icon (fa-light fa-lock) for private/non-public committees intentional? Previously there was an @if (!committee()?.public) block showing a lock icon next to the committee name here. It's been removed with no replacement or mention in the PR description.

import { Component, computed, inject, input, signal, Signal } from '@angular/core';
import { takeUntilDestroyed, toObservable, toSignal } from '@angular/core/rxjs-interop';
import { CardComponent } from '@components/card/card.component';
import { VOTE_LABEL } from '@lfx-one/shared';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barrel import instead of direct path.

Problem: VOTE_LABEL is imported from @lfx-one/shared (barrel export) while all other imports in this PR and the codebase use direct paths like @lfx-one/shared/constants.

Why it's a problem: Project rules require direct imports for standalone components — no barrel exports. Barrel imports can also cause larger bundle sizes due to tree-shaking issues.

Fix: Change to import { VOTE_LABEL } from '@lfx-one/shared/constants'.

// Server-side enrichment will be added in a follow-up PR.
// ── Leadership ──
/** Committee chair (derived from member with role "Chair") */
chair?: CommitteeLeadership | null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommitteeLeadership is redundant, chair/co_chair don't exist upstream, and the dialog is dead code.

Problem (4 issues):
(a) The CommitteeLeadership interface (line 109) is a redundant subset of CommitteeMember — it has uid, first_name, last_name, email, organization, and an elected_date field that is just CommitteeMember.role.start_date renamed.
(b) Committee.chair and Committee.co_chair fields added here do not exist on the upstream committee service response. No server-side enrichment populates them, so they will always be undefined. Adding phantom fields to a shared interface that represents an API contract is misleading.
(c) The AssignLeadershipDialogComponent is never opened — no component calls dialogService.open(AssignLeadershipDialogComponent, ...).
(d) CommitteeUpdateData (line 277) also adds chair/co_chair, but the upstream PUT/PATCH endpoint doesn't accept them.

Why it's a problem: The shared Committee interface should match the upstream API contract. Adding fields the API never returns creates a false contract. The redundant CommitteeLeadership type and unused dialog add dead code.

Fix: Remove chair/co_chair from Committee and CommitteeUpdateData interfaces. Remove CommitteeLeadership interface and LeadershipRole type. Use CommitteeMember | null if a typed reference to the leader is needed — derive leadership at the component level by filtering the members list by role.name === 'Chair' / 'Vice Chair'. If the dialog is deferred to a follow-up PR, note that explicitly in the PR description and don't add the interface fields until the enrichment exists.

this.form().updateValueAndValidity();
}

public onDateChange(): void {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const { resources: voteResources } = await this.microserviceProxy.proxyRequest<
QueryServiceResponse<{
uid: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline type literal should be a named interface.

Problem: This inline type { uid: string; name: string; status: string; end_time: string; committee_uid: string; num_response_received?: number; total_voting_request_invitations?: number; } is used as the generic param for proxyRequest<QueryServiceResponse<...>>.

Why it's a problem: Per project rules (component-organization.md, development-rules.md), all interfaces belong in @lfx-one/shared/interfaces, not inlined in service methods. Inline types are not reusable, not discoverable, and bypass the shared package convention.

Fix: Extract this into a named interface (e.g., VoteQueryResource) in packages/shared/src/interfaces/ and import it.

}

return voteResources
.filter((r) => r.data.committee_uid === committeeId)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client-side filtering is unnecessary — use tags for server-side filtering.

Problem: The fallback path fetches up to 500 votes for the entire project via parent: \project:${projectUid}`and then filters client-side here with.filter((r) => r.data.committee_uid === committeeId)`.

Why it's a problem: The query service supports tags filtering — getCommitteeSurveys in this same PR uses tags: \committee_uid:${committeeId}`to filter server-side. Fetching all project votes and filtering in memory is wasteful, won't scale for projects with many votes, and the 500page_size` cap means results can be silently truncated (there's even a warning log for this).

Fix: Use tags: \committee_uid:${committeeId}`in the query params instead ofparent: `project:${projectUid}`, matching the pattern used by getCommitteeSurveys`. This eliminates the client-side filter, the need for the 500 page_size workaround, and the truncation warning.


const params = {
...sanitizedQuery,
committee_uid: committeeId,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

committee_uid is not a valid upstream parameter for meetings.

Problem: The params object includes committee_uid: committeeId, but the upstream meeting service does not accept committee_uid as a query parameter. The meeting service lists meetings by project_uid only — committee association is embedded within the meeting object's committees array. This param is either silently ignored (returning unfiltered results) or could cause an error.

Why it's a problem: If ignored, the endpoint returns all project meetings, not just committee-scoped ones — the frontend would show meetings from other committees. If the query service is used instead, it supports tags filtering like the survey endpoint does.

Fix: Verify which upstream service this actually hits. If it goes through the query service, use tags: \committee_uid:${committeeId}`for server-side filtering (matching thegetCommitteeSurveyspattern). If it goes through the meeting service directly, filter client-side by checkingmeeting.committees?.some(c => c.uid === committeeId)`, and document why server-side filtering isn't possible.

@manishdixitlfx
Copy link
Copy Markdown
Contributor Author

Superseded by 4 focused PRs

This PR has been split into 4 smaller, independently reviewable PRs to unblock the review process:

PR Scope Lines Status
#341 Backend BFF endpoints for meetings & surveys ~310 Ready for review
#343 Votes & surveys tab components ~430 Ready for review (depends on #341)
#342 Members tab & member form enhancements ~700 Ready for review (independent)
#344 Leadership dialog & committee view polish ~380 Draft (depends on #341 & #342)

Merge order

#341 (Backend BFF)          #342 (Members + Form)
     |                           |
     v                           |  (independent, review in parallel)
#343 (Votes + Surveys)          |
     |                           |
     +───────────┬───────────────+
                 v
           #344 (Leadership + Polish)

Review fixes applied across all PRs

  • Removed votes BFF endpoint (votes-list uses VoteService directly)
  • Record<string, string> type safety on query params
  • Validate before logger.startOperation in surveys handler
  • Removed duplicate logger.error in catch blocks
  • Added loadError signal + error state UI to votes-list
  • Query param whitelisting in meetings and surveys endpoints

Closing this PR in favor of the split PRs above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants