feat: add committee charter card

[themarolt]

Summary

• Adds a Charter card to the committee About tab: renders the charter URL as a link with a "Last updated by" line, distinguishes never-set from set-then-removed (dotted-underline text with a tooltip naming who removed it and when), and offers an edit affordance via a new minimal CharterDialogComponent (single URL field, reusing the same website-field URL pattern and empty-string-is-valid semantics as removal).
• Adds charter?: CommitteeCharter to the shared Committee interface and write-only charter?: { url: string } to CommitteeUpdateData.
• Confirmed the BFF (committee.service.ts's updateCommittee) needs no changes — charter rides through the existing full-replace ...committeeData spread untouched, and the backend's write-only payload type only ever reads url, silently ignoring any extra fields.

Linked ticket: LFXV2-2659

Depends on: linuxfoundation/lfx-v2-committee-service#203 — the backend PR that adds the charter field to the committee resource. This PR reads committee.charter, which doesn't exist until that one is merged and deployed; please don't merge/deploy this before that lands.

Test plan

• yarn format:check / yarn lint:check clean
• yarn test — 96 files, 1924 tests passing, including a new spec covering the three charter card states (never set / set-then-removed / set) and edit-button visibility
• yarn build clean

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Essentials

Run ID: 96e41783-a47a-4ec6-ad1b-ba059e7d2d99

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[cursor]

PR Summary

Medium Risk
Introduces committee PUT updates for charter URLs and depends on the backend charter field landing first; risk is mitigated by client validation and unchanged-value guards.

Overview
Adds committee charter support on the About tab: a new Charter card shows the external document URL (with last-updated attribution), plain “No charter yet” when never set, and a dotted-underline empty state with removal tooltip when a charter was cleared.

Editors get Add/Edit charter via a new CharterDialogComponent (URL field validated to match the committee API’s http(s) pattern and 2,048-char limit; blank URL means remove). committee-view wires editCharterRequested → dialog → saveCharter, which PUTs { charter: { url } }, toasts, refreshes the committee, and skips the API when the saved URL is unchanged (including dirty-but-same-value).

Shared types gain CommitteeCharter, PublicAuditUser, optional charter on Committee, write-only charter on CommitteeUpdateData, and CharterDialogData. Coverage includes unit specs for the card states and dialog validation, plus e2e tests for add/save and the no-op save path.

^{Reviewed by Cursor Bugbot for commit f219f2e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Pull request overview

Adds committee charter viewing, audit metadata, and URL editing to the About tab, dependent on committee-service PR #203.

Changes:

• Adds shared charter read/write types.
• Adds charter display states and editing dialog.
• Wires charter updates and rendering tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Summary / Findings
packages/shared/src/interfaces/committee.interface.ts	Defines charter data contracts.
apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.ts	Adds charter editing output and tooltip support.
apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.spec.ts	Tests charter states; nit (1 vote): removal tooltip content is not asserted.
apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html	Renders charter states; moderate (2 votes): removal details are inaccessible to keyboard users. Nit (1 vote): repeated signal reads use prohibited non-null assertions.
apps/lfx-one/src/app/modules/committees/components/charter-dialog/charter-dialog.component.ts	Implements the URL form; moderate (2 votes): validation does not match the API URL and length contract.
apps/lfx-one/src/app/modules/committees/components/charter-dialog/charter-dialog.component.html	Provides charter editing UI; moderate (2 votes): URL input lacks an accessible label.
apps/lfx-one/src/app/modules/committees/committee-view/committee-view.component.ts	Opens the dialog and persists updates.
apps/lfx-one/src/app/modules/committees/committee-view/committee-view.component.html	Connects the charter edit event.

Suppressed comments (2)

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214

• The template re-reads committee().charter with non-null assertions after the condition. This violates the repository's explicit ban on template non-null assertions (docs/reviews/frontend-checklist.md:349-354) and makes the branch depend on repeated signal reads remaining identical. Alias the URL in the @if and render that value directly.

        @if (committee().charter?.url) {
          <div class="flex-1 flex flex-col gap-1 min-w-0">
            <a
              [href]="committee().charter!.url"

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.spec.ts:77

• The test named as covering the removal tooltip only checks underline classes and never asserts the tooltip text, remover, or date. A regression that drops or misattributes the audit detail would still pass; exercise the tooltip directive and assert Removed by Alice Example on Aug 1, 2026.

    const el = emptyState();
    expect(el).not.toBeNull();
    expect(el!.textContent).toContain('No charter yet');
    expect(el!.className).toContain('underline');
    expect(el!.className).toContain('decoration-dotted');
    expect(link()).toBeNull();

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 237d16c. Configure here.}

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

packages/shared/src/interfaces/committee.interface.ts:512

• updated_by does not have the CommitteeUser contract. The linked upstream PR defines this publicly visible audit user with optional avatar, name, and username only, deliberately omitting email; CommitteeUser requires email, name, and username. This makes the shared read model and its fixtures claim data the API cannot return. Introduce a matching public-audit-user interface and update the charter fixtures to omit email.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

packages/shared/src/interfaces/committee.interface.ts:512

• This does not mirror the dependency's response contract: committee-service PR #203 returns an email-less PublicAuditUser whose avatar, name, and username fields are optional, while CommitteeUser requires email, name, and username. The current type therefore promises PII and required fields that public committee responses deliberately never provide, and the tests already mask the mismatch by inventing an email. Model the public audit shape without email.

apps/lfx-one/src/app/modules/committees/committee-view/committee-view.component.ts:565

• The new write path is not exercised by the added component tests: they instantiate only CommitteeAboutComponent and CharterDialogComponent, while no CommitteeViewComponent unit spec exists. This leaves the { charter: { url } } payload, the never-set no-op guard, and the post-save refresh unwatched. Add an About-tab E2E that saves and clears a charter, asserts the PUT body/refreshed card state, and verifies an unchanged empty value sends no PUT.

    if (url === (committee.charter?.url ?? '')) {
      return;
    }
    this.committeeService.updateCommittee(committee.uid, { charter: { url } }).subscribe({

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214

• The new template re-reads committee() and uses non-null assertions even though this repository explicitly disallows ! in templates (docs/reviews/frontend-checklist.md:349-353). Capture the truthy URL in the @if alias and bind that value; this also keeps the guard and the rendered value from coming from separate signal reads.

              [href]="committee().charter!.url"

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

packages/shared/src/interfaces/committee.interface.ts:512

• The linked committee-service contract returns updated_by as PublicAuditUser: it intentionally omits email, and avatar, name, and username are all optional. Typing it as CommitteeUser therefore misrepresents the actual public GET payload and forces consumers/tests to invent an email that will never be returned. Model the public audit shape so callers cannot assume those fields exist.

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214

• This block adds template non-null assertions here and again on lines 219 and 224, contrary to the repository rule in docs/reviews/frontend-checklist.md:349-353. Capture committee().charter once with @if (...; as charter), then branch on charter.url and read charter.url/charter.updated_at without assertions.

              [href]="committee().charter!.url"

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214

• The new link re-reads the signal with non-null assertions here and in its text binding. docs/reviews/frontend-checklist.md:349-353 explicitly disallows ! in changed templates because a prior condition does not narrow a later signal invocation. Use optional access (or alias the charter in the @if) instead.

            <a
              [href]="committee().charter!.url"

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.spec.ts:76

• This test is named as covering the removal tooltip, but it only checks underline classes; the remover/date text could disappear or regress while the test remains green. Assert the accessible tooltip text so the key set-then-removed behavior is actually pinned.

    const el = emptyState();
    expect(el).not.toBeNull();
    expect(el!.textContent).toContain('No charter yet');
    expect(el!.className).toContain('underline');
    expect(el!.className).toContain('decoration-dotted');

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/e2e/committee-about-tab.spec.ts:197

• The empty-string clear path remains uncovered: the two integration cases exercise adding a URL and saving an unchanged URL, but neither clears an existing charter and verifies the { charter: { url: '' } } PUT plus the refreshed removed state. Because clearing is a distinct write semantic central to this feature, add a case that performs that flow end to end.

apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214

• The new branch introduces template non-null assertions here and again for the URL text/date, contrary to the repository rule in docs/reviews/frontend-checklist.md:349-353. Alias committee().charter with @if (...; as charter) and then branch on charter.url, so all three reads use the narrowed alias without !.

              [href]="committee().charter!.url"

packages/shared/src/interfaces/committee.interface.ts:512

• The upstream charter contract uses an email-less PublicAuditUser: avatar, name, and username are all optional, and email is never returned. Reusing CommitteeUser here promises required fields that public responses do not contain, while the new fixtures mask the mismatch by adding email; it also leaves the attribution text blank when the optional object has no name/username. Please introduce the matching dedicated type, remove email from the fixtures, and retain a display fallback.

  updated_by?: CommitteeUser;

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/committees/components/charter-dialog/charter-dialog.component.spec.ts:90

• This test establishes that blank input is valid but never verifies the removal result. A regression in save() could close with undefined or another value while this test still passes, breaking the { charter: { url: '' } } clear contract. Click Save and assert that the dialog closes with the empty string.
  apps/lfx-one/src/app/modules/committees/components/charter-dialog/charter-dialog.component.ts:30
• Validators.maxLength counts UTF-16 code units, but the upstream Goa validator enforces this limit with utf8.RuneCountInString. Consequently, a contract-valid URL containing non-BMP path characters can be rejected here (for example, 1,020 emoji produce 1,040 upstream runes but 2,060 JavaScript code units), so this does not mirror the API contract exactly. Use a code-point-aware length validator and retain the maxlength error key used by the template.
  apps/lfx-one/src/app/modules/committees/components/committee-about/committee-about.component.html:214
• The charter URL is writer-controlled, and the upstream contract accepts any HTTP(S) host, but this binds the raw value directly to href. That bypasses the repository's user-provided-link guard (ValidExternalUrlPipe/normalizeToUrl; see apps/lfx-one/src/app/shared/pipes/valid-external-url.pipe.ts:8-13), allowing localhost/private-network links to be published to viewers. Gate the value through validExternalUrl and render a non-link fallback when it is rejected.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

apps/lfx-one/src/app/modules/committees/components/charter-dialog/charter-dialog.component.html:19

• When this validator disables Save, the error is only visual: it is neither a live region nor associated with the wrapped input, so screen-reader users receive no validation feedback. Mark the dynamically inserted message as an alert (the established pattern in the committee forms) so it is announced when the URL becomes invalid.

[audigregorie]

Code Review Summary

Well-constructed, well-tested feature — I verified the cross-repo contract claims against lfx-v2-committee-service#203 directly and they hold (URL pattern ^$|^https?://[^\s/$.?#][^\s]*$, MaxLength 2048, write shape { url }, public-audit-user field parity, and the writer's same-URL no-op carry-forward, which is what makes the BFF's full-replace mergedData echo of the read-shape charter object safe on unrelated updates). Findings: 1 Critical (template ! assertions), 1 Major (rel missing noreferrer), 2 Minor.

Outside the diff

• Deploy-order hold is load-bearing. lfx-v2-committee-service#203 is still OPEN. Against the current upstream, a charter save returns 200, shows the "Charter updated" toast, and silently persists nothing (unknown field ignored on decode). The PR body already flags this — reinforcing that this must not merge/deploy before the backend lands.
• Branch is 3 commits behind main; no overlapping files, so a rebase should be trivial.

What's done well

• The removed-state tooltip is a textbook fix for the tooltip-on-non-focusable-host KB pattern (tabindex="0" + role="note" + aria-label + tooltipEvent="both").
• The [id]-bound-vs-static fix on lfx-input-text (duplicate id on host + native input breaking label[for]) with a regression test asserting id uniqueness — a genuinely subtle a11y catch.
• The never-set no-op guard in saveCharter, plus an e2e proving dirty-but-unchanged doesn't PUT; validator parity with upstream pinned by dedicated specs.

[audigregorie]

Critical — Non-null assertions (charter!) in the template (3×)

Per docs/reviews/frontend-checklist.md §14.5 (CRITICAL for ! in templates) and KB typescript-correctness/non-null-assertion-on-async-result: [href]="committee().charter!.url", {{ committee().charter!.url }}, and {{ committee().charter!.updated_at | date }} rely on the enclosing @if for safety. It can't crash as-is, but the checklist's prescribed fix is @if (x; as y) aliasing — which the very next branch (@else if (committee().charter; as charter)) already demonstrates. Restructuring to alias charter at the outer level also collapses the repeated committee().charter?. chains in the attribution line:

Suggested change

	@if (committee().charter?.url) {
	<div class="flex-1 flex flex-col gap-1 min-w-0">
	<a
	[href]="committee().charter!.url"
	target="_blank"
	rel="noopener"
	class="text-sm text-blue-600 hover:text-blue-800 break-all"
	data-testid="committee-about-charter-link">
	{{ committee().charter!.url }}
	</a>
	<span class="text-xs text-gray-400" data-testid="committee-about-charter-updated-by"
	>Last updated by {{ committee().charter?.updated_by?.name || committee().charter?.updated_by?.username || 'someone' }} on
	{{ committee().charter!.updated_at | date: 'MMM d, y' }}</span
	>
	</div>
	} @else if (committee().charter; as charter) {
	<span
	class="text-gray-400 text-sm italic flex-1 underline decoration-dotted"
	[pTooltip]="
	'Removed by ' + (charter.updated_by?.name || charter.updated_by?.username || 'someone') + ' on ' + (charter.updated_at | date: 'MMM d, y')
	"
	tooltipPosition="top"
	tooltipEvent="both"
	tabindex="0"
	role="note"
	[attr.aria-label]="
	'Removed by ' + (charter.updated_by?.name || charter.updated_by?.username || 'someone') + ' on ' + (charter.updated_at | date: 'MMM d, y')
	"
	data-testid="committee-about-charter-empty"
	>No charter yet</span
	>
	} @else {
	<span class="text-gray-400 text-sm italic flex-1" data-testid="committee-about-charter-empty">No charter yet</span>
	}
	@if (canEdit()) {
	@if (committee().charter; as charter) {
	@if (charter.url) {
	<div class="flex-1 flex flex-col gap-1 min-w-0">
	<a
	[href]="charter.url"
	target="_blank"
	rel="noopener"
	class="text-sm text-blue-600 hover:text-blue-800 break-all"
	data-testid="committee-about-charter-link">
	{{ charter.url }}
	</a>
	<span class="text-xs text-gray-400" data-testid="committee-about-charter-updated-by"
	>Last updated by {{ charter.updated_by?.name || charter.updated_by?.username || 'someone' }} on
	{{ charter.updated_at | date: 'MMM d, y' }}</span
	>
	</div>
	} @else {
	<span
	class="text-gray-400 text-sm italic flex-1 underline decoration-dotted"
	[pTooltip]="
	'Removed by ' + (charter.updated_by?.name || charter.updated_by?.username || 'someone') + ' on ' + (charter.updated_at | date: 'MMM d, y')
	"
	tooltipPosition="top"
	tooltipEvent="both"
	tabindex="0"
	role="note"
	[attr.aria-label]="
	'Removed by ' + (charter.updated_by?.name || charter.updated_by?.username || 'someone') + ' on ' + (charter.updated_at | date: 'MMM d, y')
	"
	data-testid="committee-about-charter-empty"
	>No charter yet</span
	>
	}
	} @else {
	<span class="text-gray-400 text-sm italic flex-1" data-testid="committee-about-charter-empty">No charter yet</span>
	}

Semantics are identical (set / set-then-removed / never-set), and the removed-state branch keeps working because the outer charter alias is in scope in the inner @else. Heads-up: this range overlaps the separate one-line rel suggestion on line 216 — if you apply both, fold the noopener noreferrer change into whichever you apply second.

[audigregorie]

Major — target="_blank" link with rel="noopener" but no noreferrer

KB security/window-open-no-noopener flags <a target="_blank"> without the full rel="noopener noreferrer" pair, and the repo convention is ~110 uses of the full pair vs 3 bare noopener. noopener alone does close the reverse-tabnabbing vector (so this isn't the KB's Critical severity in effect), but without noreferrer every click leaks the full committee page URL — project/committee slug included — to the external charter host via the Referer header.

Suggested change

	rel="noopener"
	rel="noopener noreferrer"

(Overlaps the larger restructure suggestion on lines 211–245 — apply one, then fold the other's delta in by hand.)

[audigregorie]

Minor — the "Removed by … on …" attribution string is built twice

The same interpolated string is duplicated in pTooltip (here) and attr.aria-label (line 236) — a future copy tweak to one silently desynchronizes the sighted tooltip from the screen-reader announcement. Consider a single computed (e.g. charterRemovedLabel) that both attributes bind to, so the strings can't drift.

[audigregorie]

Minor — contract-bound URL pattern + maxLength inline in the component

This regex and the 2048 limit deliberately mirror upstream charterURLPattern/MaxLength(2048) — contract-bound validation patterns in this repo live in @lfx-one/shared/constants (SLACK_INCOMING_WEBHOOK_URL_PATTERN in committees.constants.ts, LINKEDIN_PROFILE_PATTERN in validation.constants.ts). Consider extracting CHARTER_URL_PATTERN / CHARTER_URL_MAX_LENGTH there so the upstream contract has one canonical home. (Counter-precedent exists — the website pattern is inline in committee-manage.component.ts — so this is a judgment call, but the contract-mirroring comment here argues for the stronger precedent.)