feat(ui): my engagement pages standardisation#492
Merged
Conversation
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:
WalkthroughReplaces numerous bespoke empty-state and tab UIs with a reusable EmptyStateComponent and CardTabsBarComponent; converts several status/category filters from form controls to tab/pill state; adds foundation/project filter wiring to committee/list tables; standardizes table pagination, name-link styling, and due-date labeling across multiple dashboards. (34 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Standardises the “My Engagement” (Me lens) UI across multiple dashboards by introducing shared empty states, consistent tab/pill controls, and aligned table/paginator styling/behaviour.
Changes:
- Introduces shared UI building blocks (
lfx-empty-state,lfx-card-tabs-bar) and applies them across Meetings, Votes, Surveys, Mailing Lists, Documents, Events, and My Activity. - Updates table/paginator styling and behaviour (hover handling, selectable rows cursor, conditional paginator controls, rows-per-page visibility).
- Normalises status handling from API (case-insensitive status mapping) and adds “due in …” labelling in votes/surveys.
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/utils/survey.utils.ts | Normalises survey status casing before mapping to display status. |
| packages/shared/src/constants/documents.constants.ts | Updates document “recording” tag icon styling for consistency. |
| apps/lfx-one/src/styles.scss | Adds global PrimeNG datatable hover + paginator variable/layout overrides and a neutral tag style. |
| apps/lfx-one/src/server/services/supabase.service.ts | Adds a Supabase service file (currently stubbed) to unblock compilation. |
| apps/lfx-one/src/app/shared/pipes/poll-status-severity.pipe.ts | Normalises poll status casing before severity mapping. |
| apps/lfx-one/src/app/shared/pipes/poll-status-label.pipe.ts | Normalises poll status casing before label mapping. |
| apps/lfx-one/src/app/shared/pipes/due-date-label.pipe.ts | Adds a pipe for “Due in X days/weeks/months” labelling. |
| apps/lfx-one/src/app/shared/components/table/table.component.ts | Adjusts paginator-related inputs and adds computed single-page detection. |
| apps/lfx-one/src/app/shared/components/table/table.component.scss | Adds table link pattern styling, selectable row transitions, and paginator layout helpers. |
| apps/lfx-one/src/app/shared/components/table/table.component.html | Adds conditional paginator style classes (no RPP / single page). |
| apps/lfx-one/src/app/shared/components/empty-state/empty-state.component.ts | Introduces reusable empty state component API (icon/title/subtitle/CTA). |
| apps/lfx-one/src/app/shared/components/empty-state/empty-state.component.html | Implements empty state layout with optional card wrapper and CTA modes. |
| apps/lfx-one/src/app/shared/components/card-tabs-bar/card-tabs-bar.component.ts | Introduces a standard card top-bar with filter pills + projected actions. |
| apps/lfx-one/src/app/shared/components/card-tabs-bar/card-tabs-bar.component.html | Renders the pills bar and right-side action slot. |
| apps/lfx-one/src/app/modules/votes/votes-dashboard/votes-dashboard.component.ts | Switches votes dashboard empty states to lfx-empty-state. |
| apps/lfx-one/src/app/modules/votes/votes-dashboard/votes-dashboard.component.html | Replaces bespoke empty-state markup with shared component for project + Me lens. |
| apps/lfx-one/src/app/modules/votes/components/votes-table/votes-table.component.ts | Replaces status dropdown with tab pills; adds local filtering for Me lens + reset flow. |
| apps/lfx-one/src/app/modules/votes/components/votes-table/votes-table.component.html | Updates layout, due-date display, CTA buttons, and empty messages to shared empty state. |
| apps/lfx-one/src/app/modules/surveys/surveys-dashboard/surveys-dashboard.component.ts | Switches surveys dashboard empty states to lfx-empty-state. |
| apps/lfx-one/src/app/modules/surveys/surveys-dashboard/surveys-dashboard.component.html | Replaces bespoke empty-state markup with shared component for project + Me lens. |
| apps/lfx-one/src/app/modules/surveys/components/surveys-table/surveys-table.component.ts | Replaces status dropdown with open/closed tab pills; adds reset flow + due labelling. |
| apps/lfx-one/src/app/modules/surveys/components/surveys-table/surveys-table.component.html | Updates table layout and empty messages to shared empty state + tab bar header. |
| apps/lfx-one/src/app/modules/my-activity/my-activity-dashboard/my-activity-dashboard.component.ts | Adds shared empty state support to My Activity dashboard. |
| apps/lfx-one/src/app/modules/my-activity/my-activity-dashboard/my-activity-dashboard.component.html | Adds votes/surveys empty states when no activity exists. |
| apps/lfx-one/src/app/modules/my-activity/components/votes-table/votes-table.component.html | Applies the shared “name-link” styling class for consistency. |
| apps/lfx-one/src/app/modules/my-activity/components/surveys-table/surveys-table.component.html | Applies the shared “name-link” styling class for consistency. |
| apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.ts | Adds isFiltered + reset logic and imports shared empty state. |
| apps/lfx-one/src/app/modules/meetings/meetings-dashboard/meetings-dashboard.component.html | Replaces “No results”/empty meeting states with lfx-empty-state and tab-based top bar. |
| apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.ts | Replaces time dropdown with filter pills and simplifies form wiring. |
| apps/lfx-one/src/app/modules/meetings/meetings-dashboard/components/meetings-top-bar/meetings-top-bar.component.html | Renders time filter pills and removes the dropdown. |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.ts | Adds isFiltered computation and shared empty-state import. |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.html | Adds Me lens empty state when no lists and not filtered. |
| apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.ts | Adds filter detection, reset behaviour, and conditional RPP options. |
| apps/lfx-one/src/app/modules/mailing-lists/components/mailing-list-table/mailing-list-table.component.html | Updates table styling, neutral tags, paginator behaviour, and empty messages. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/my-events-dashboard.component.ts | Moves tab pills to a shared tabs bar; adds placeholder + reset utilities. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/my-events-dashboard.component.html | Wraps tabs + filters + list into a single card and adds request CTA in the tab bar. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/events-table/events-table.component.ts | Adds row-click navigation and conditional rows-per-page options. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/events-table/events-table.component.html | Updates table styling, adds row selection, and adjusts action/certificate columns. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/events-list/events-list.component.ts | Separates unfiltered stats fetching from filtered table fetching; adds shared empty-state support. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/events-list/events-list.component.html | Adds filtered vs true-empty empty states for upcoming/past tabs. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/event-request-list/event-request-list.component.ts | Adds RPP options + hasData helper and updates empty-state configuration. |
| apps/lfx-one/src/app/modules/events/my-events-dashboard/components/event-request-list/event-request-list.component.html | Replaces table “emptymessage” with top-level empty state and updates header styling. |
| apps/lfx-one/src/app/modules/events/foundation-event-dashboard/components/events-table/events-table.component.html | Applies shared “name-link” styling and minor typography adjustments. |
| apps/lfx-one/src/app/modules/events/components/events-top-bar/events-top-bar.component.ts | Adds configurable search placeholder input. |
| apps/lfx-one/src/app/modules/events/components/events-top-bar/events-top-bar.component.html | Binds placeholder to the new input. |
| apps/lfx-one/src/app/modules/events/components/discover-events-button/discover-events-button.component.html | Minor template formatting change for data-testid attribute. |
| apps/lfx-one/src/app/modules/documents/documents-dashboard/documents-dashboard.component.ts | Adds source tab pills, reset flow, and shared empty-state import; updates filtering model. |
| apps/lfx-one/src/app/modules/documents/documents-dashboard/documents-dashboard.component.html | Adds primary/filtered empty states and moves source filter to tabs bar. |
| apps/lfx-one/src/app/modules/committees/components/committee-table/committee-table.component.ts | Adds filter detection, reset, optional foundation/project filters, and conditional RPP options. |
| apps/lfx-one/src/app/modules/committees/components/committee-table/committee-table.component.html | Updates filters, typography, neutral tags, paginator behaviour, and empty messages. |
| apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts | Adjusts option derivation in Me lens and adds shared empty-state import. |
| apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.html | Replaces Me lens cards/empty states with table + shared empty-state patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nunoeufrasio
force-pushed
the
feat/me-lens-ui-improvements
branch
from
c97ed2a to
825ccd8
Compare
🧹 Deployment RemovedThe deployment for PR #492 has been removed. |
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-492.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nunoeufrasio
force-pushed
the
feat/me-lens-ui-improvements
branch
from
1a8c3e1 to
77c3cef
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
apps/lfx-one/src/app/modules/documents/documents-dashboard/documents-dashboard.component.ts:144
initDocuments()scopes the API call withproject?.uidfromProjectContextService.activeContext. In themelens,activeContextcan still be set (e.g., selected foundation/project), so this will unexpectedly filter “My Documents” to a single context, which contradicts the UI copy that says “across all foundations.” Consider reintroducingLensServicehere and only passingproject_uidwhen infoundation/projectlenses; forme, callgetMyDocuments()without a project UID (or update the header/description to match the scoped behavior).
private initDocuments(): Signal<MyDocumentItem[]> {
return toSignal(
toObservable(this.project).pipe(
switchMap((project) => {
this.loading.set(true);
return this.documentService.getMyDocuments(project?.uid).pipe(
catchError(() => of([] as MyDocumentItem[])),
finalize(() => this.loading.set(false))
);
})
apps/lfx-one/src/app/modules/committees/components/committee-table/committee-table.component.ts:110
joinGroup()is still implemented (and was updated totake(1)), but there is no longer any template path that calls it (the join/request column was removed). Either restore the join/request UI for non-managers or remove the now-dead code to avoid carrying unused behavior and services.
public joinGroup(committee: Committee): void {
const joinMode = committee.join_mode || 'closed';
switch (joinMode) {
case 'open':
this.committeeService.joinCommittee(committee.uid).pipe(take(1)).subscribe({
next: () => {
this.messageService.add({
severity: 'success',
summary: 'Joined',
detail: `You have joined "${committee.name}"`,
});
this.refresh.emit();
},
error: () => {
this.messageService.add({
severity: 'error',
summary: 'Error',
detail: `Failed to join "${committee.name}"`,
});
},
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
apps/lfx-one/src/app/modules/documents/documents-dashboard/documents-dashboard.component.ts:144
- DocumentsDashboard no longer considers the active lens when fetching documents: initDocuments() always calls getMyDocuments(project?.uid). This will incorrectly scope the “My Engagement” view if a project context is selected, and it can also cause project-lens navigation to show unscoped “my documents” when no project is selected. Consider reintroducing LensService and selecting projectUid based on lens (me => undefined, non-me => require project?.uid and avoid fetching until selected).
// === Computed Signals ===
protected readonly project = this.projectContextService.activeContext;
protected readonly searchQuery: Signal<string> = this.initSearchQuery();
protected readonly foundationFilter: Signal<string | null> = this.initFoundationFilter();
protected readonly groupFilter: Signal<string | null> = this.initGroupFilter();
protected readonly meetingFilter: Signal<string | null> = this.initMeetingFilter();
protected readonly mailingListFilter: Signal<string | null> = this.initMailingListFilter();
protected readonly documents: Signal<MyDocumentItem[]> = this.initDocuments();
protected readonly filteredDocuments: Signal<MyDocumentItem[]> = this.initFilteredDocuments();
protected readonly rppOptions = computed<number[] | undefined>(() => (this.filteredDocuments().length > 10 ? [10, 25, 50] : undefined));
protected readonly foundationOptions: Signal<{ label: string; value: string | null }[]> = this.initFoundationOptions();
protected readonly groupOptions: Signal<{ label: string; value: string | null }[]> = this.initGroupOptions();
protected readonly meetingOptions: Signal<{ label: string; value: string | null }[]> = this.initMeetingOptions();
protected readonly mailingListOptions: Signal<{ label: string; value: string | null }[]> = this.initMailingListOptions();
// === Protected Methods ===
protected onSourceTabChange(tab: string): void {
this.sourceTab.set(tab);
}
protected resetFilters(): void {
this.filterForm.reset({ search: '', foundation: null, group: null, meeting: null, mailingList: null });
this.sourceTab.set('all');
}
protected openDocument(doc: MyDocumentItem): void {
if (!doc.url) return;
try {
const url = new URL(doc.url);
if (['http:', 'https:'].includes(url.protocol)) {
window.open(doc.url, '_blank', 'noopener,noreferrer');
}
} catch {
// Invalid URL — silently ignore
}
}
// === Private Initializers ===
private initSearchQuery(): Signal<string> {
return toSignal(
this.filterForm.controls.search.valueChanges.pipe(
debounceTime(300),
distinctUntilChanged(),
map((v) => v ?? ''),
startWith('')
),
{ initialValue: '' }
);
}
private initFoundationFilter(): Signal<string | null> {
return toSignal(this.filterForm.controls.foundation.valueChanges.pipe(startWith<string | null>(null)), { initialValue: null });
}
private initGroupFilter(): Signal<string | null> {
return toSignal(this.filterForm.controls.group.valueChanges.pipe(startWith<string | null>(null)), { initialValue: null });
}
private initMeetingFilter(): Signal<string | null> {
return toSignal(this.filterForm.controls.meeting.valueChanges.pipe(startWith<string | null>(null)), { initialValue: null });
}
private initMailingListFilter(): Signal<string | null> {
return toSignal(this.filterForm.controls.mailingList.valueChanges.pipe(startWith<string | null>(null)), { initialValue: null });
}
private initDocuments(): Signal<MyDocumentItem[]> {
return toSignal(
toObservable(this.project).pipe(
switchMap((project) => {
this.loading.set(true);
return this.documentService.getMyDocuments(project?.uid).pipe(
catchError(() => of([] as MyDocumentItem[])),
finalize(() => this.loading.set(false))
);
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
apps/lfx-one/src/app/modules/documents/documents-dashboard/documents-dashboard.component.ts:144
initDocuments()scopes the request toproject?.uidfromProjectContextService.activeContext. In the 'me' lens,activeContextcan still be the currently selected project (seeProjectContextService.initActiveContext()), which will unintentionally filter “My Documents” down to a single project even though the page copy says it shows documents across all foundations. Consider reintroducing lens-aware scoping (e.g., passundefinedwhenactiveLens() === 'me', otherwise pass the selected project UID) so the Me lens always fetches unscoped results.
private initDocuments(): Signal<MyDocumentItem[]> {
return toSignal(
toObservable(this.project).pipe(
switchMap((project) => {
this.loading.set(true);
return this.documentService.getMyDocuments(project?.uid).pipe(
catchError(() => of([] as MyDocumentItem[])),
finalize(() => this.loading.set(false))
);
})
apps/lfx-one/src/app/modules/committees/components/committee-table/committee-table.component.ts:152
joinGroup()is now unused (there are no template references after the Status/Join column removal). This leaves dead code and suggests the “join from list” capability may have been inadvertently dropped. Either removejoinGroup()(and related messaging) or restore the UI wiring that calls it so the method remains meaningful.
public joinGroup(committee: Committee): void {
const joinMode = committee.join_mode || 'closed';
switch (joinMode) {
case 'open':
this.committeeService.joinCommittee(committee.uid).pipe(take(1)).subscribe({
next: () => {
this.messageService.add({
severity: 'success',
summary: 'Joined',
detail: `You have joined "${committee.name}"`,
});
this.refresh.emit();
},
error: () => {
this.messageService.add({
severity: 'error',
summary: 'Error',
detail: `Failed to join "${committee.name}"`,
});
},
});
break;
case 'application':
this.messageService.add({
severity: 'info',
summary: 'Apply to Join',
detail: `"${committee.name}" requires an application. This feature is coming soon.`,
});
break;
case 'invite_only':
this.messageService.add({
severity: 'info',
summary: 'Invite Only',
detail: `"${committee.name}" is invite-only. Ask an existing member to invite you.`,
});
break;
case 'closed':
default:
this.messageService.add({
severity: 'warn',
summary: 'Closed',
detail: `"${committee.name}" is not currently accepting new members.`,
});
break;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
PR Review Resolution SummaryAll Copilot review comments have been addressed. Below is a full account of what was fixed, what was already correct, and what is tracked as a follow-up. ✅ Fixed — commit
|
| Comment | File | Fix applied |
|---|---|---|
categoryTabOptions using opt.value for label — drops category counts |
committee-table.component.ts |
Changed to opt.label so pills show e.g. "Technical (3)" |
dueDateLabel pipe evaluated twice per row |
votes-table.component.html, surveys-table.component.html |
Used @let dueDateLabelValue to compute once and reuse in both @if and interpolation |
empty-state subtitle has whitespace-nowrap — clips long text on mobile |
empty-state.component.html |
Removed whitespace-nowrap so subtitle wraps naturally |
<i> has both [class] and static class — static classes may be overridden |
documents-dashboard.component.html |
Merged into single [class] binding: [class]="(doc.source | myDocumentSourceTag: 'icon') + ' text-gray-900 text-base'" |
| Documents page header hardcoded "My Documents" — misleading in project lens | documents-dashboard.component.html |
Removed "My" prefix; description updated to "Documents, links, and attachments across groups, meetings, and foundations." |
Download Certificate button uses (click) not (onClick), triggers row-select |
events-table.component.html |
Switched to (onClick) with $event.stopPropagation() |
✔️ Already correct — no change needed
| Comment | Evidence |
|---|---|
| Grammar: "filters criteria" in empty states | All empty state subtitles already read "Try adjusting your filter criteria" on this branch |
| "Search travel fundings..." placeholder | Placeholder already reads "Search travel funding..." |
registrationUrl null guard |
Button already conditionally rendered with !event.isRegistered && event.registrationUrl |
DueDateLabelPipe can return "Due in NaN months" |
Pipe already has if (Number.isNaN(due.getTime())) return ''; guard |
🔖 Acknowledged — tracked as follow-up (out of scope for this PR)
| Comment | Reason |
|---|---|
SupabaseService committed as a stub |
Pre-existing in main; not introduced by this PR. Separate ticket to implement or gate properly. |
Paginator CSS overrides duplicated in table.component.scss and styles.scss |
Intentional defence-in-depth while global styles are stabilising. Consolidation tracked as a follow-up refactor. |
initializeStatsUpcoming() fetches 1000 events just for stat cards |
Needs a dedicated lightweight stats endpoint. Architectural change, out of scope for this standardisation PR. |
Events table [rows] hardcoded to 10 with rowsPerPageOptions |
Table uses server-side lazy pagination — actual page size is controlled by the backend cursor. Client-side binding alignment tracked as follow-up. |
| Non-manager "Status" (join) column removed from committee table | Intentional design decision. joinGroup() is retained for a dedicated join flow to be wired in a follow-up ticket. |
(onRowSelect) fires when action buttons are clicked |
Partially resolved for Download Certificate (stopPropagation). Broader fix for all action cells tracked as follow-up. |
openDocument() for file downloads uses window.open instead of download endpoint |
Dedicated download handler with anchor-download behaviour tracked as follow-up improvement. |
🧹 Deployment RemovedThe deployment for PR #492 has been removed. |
This reverts commit d101ef6.
Prevents the row-select handler from firing when clicking the Register action button, avoiding unintended navigation alongside the registration URL. Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Nuno Eufrasio <nmeufrasio@gmail.com>
nunoeufrasio
force-pushed
the
feat/me-lens-ui-improvements
branch
from
235d21e to
c88c89a
Compare
🧹 Deployment RemovedThe deployment for PR #492 has been removed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+111
to
+132
| .p-paginator { | ||
| justify-content: flex-start !important; | ||
| padding-top: 1.5rem !important; | ||
|
|
||
| .p-paginator-first { order: 1; } | ||
| .p-paginator-prev { order: 2; } | ||
| .p-paginator-pages { order: 3; } | ||
| .p-paginator-next { order: 4; } | ||
| .p-paginator-last { order: 5; } | ||
|
|
||
| .p-paginator-current { | ||
| order: 6; | ||
| margin-left: auto; | ||
| font-size: 0.875rem !important; | ||
| color: #6b7280 !important; | ||
| } | ||
|
|
||
| .p-paginator-rpp-dropdown { | ||
| order: 7; | ||
| margin-left: 1.5rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
The .p-paginator layout overrides here are global (unscoped) and use !important, so they will affect every PrimeNG paginator in the app (e.g., settings tables, meeting details, etc.), not just the My Engagement pages described in the PR. Consider scoping these rules to a wrapper class (e.g., .lfx-table / .lfx-engagement) or moving them into the table component styles only, to avoid unintended UI regressions elsewhere.
| [text]="true" | ||
| severity="primary" | ||
| [attr.data-testid]="testId()" | ||
| [attr.data-testid]="testId()" |
There was a problem hiding this comment.
This attribute line appears to have lost its indentation and no longer matches the surrounding template formatting. Since the repo uses Prettier on *.html, it’s likely to be rewritten (or fail format:check if enforced). Reformat this block (or rerun Prettier) to keep the template consistent.
Suggested change
| [attr.data-testid]="testId()" | |
| [attr.data-testid]="testId()" |
pageTitle()/pageDescription() don't exist on the component — use documentLabel.plural which is the actual available property. Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Nuno Eufrasio <nmeufrasio@gmail.com>
Contributor
Author
Review response summaryAddressed all Copilot comments — here's a quick overview: Already correct on this branch
Fixed in this PR
Intentional / tracked as follow-up
Generated with Claude Code |
MRashad26
reviewed
Apr 21, 2026
| if (initial !== 'upcoming') { | ||
| this.searchForm.get('timeFilter')?.setValue(initial, { emitEvent: false }); | ||
| } | ||
| effect(() => { |
Contributor
There was a problem hiding this comment.
CRITICAL — effect() used for form mutation
Problem: Lines 51–56 use effect() to sync the searchQuery input signal back into the search FormControl:
effect(() => {
const query = this.searchQuery();
if (this.searchForm.get('search')?.value !== query) {
this.searchForm.get('search')?.setValue(query, { emitEvent: false });
}
});Why it is a problem: CLAUDE.md prohibits effect() for anything other than simple logging. Mutating a form control inside effect() is a side effect — it runs outside Angular's reactive graph, is difficult to test, and can trigger unexpected re-renders or infinite loops in zoneless mode if the form change feeds back into a signal that the effect depends on.
Suggested fix: Replace with toObservable() + takeUntilDestroyed() + tap():
public constructor() {
// Sync parent searchQuery input → local form control
toObservable(this.searchQuery)
.pipe(
takeUntilDestroyed(),
tap((query) => {
if (this.searchForm.get('search')?.value !== query) {
this.searchForm.get('search')?.setValue(query, { emitEvent: false });
}
})
)
.subscribe();
// Emit local changes upward (existing)
this.searchForm
.get('search')
?.valueChanges.pipe(takeUntilDestroyed())
.subscribe((value) => this.searchQueryChange.emit(value || ''));
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
134
to
143
| private initDocuments(): Signal<MyDocumentItem[]> { | ||
| const lens$ = toObservable(this.lensService.activeLens); | ||
|
|
||
| return toSignal( | ||
| combineLatest([toObservable(this.project), lens$]).pipe( | ||
| switchMap(([project, lens]) => { | ||
| // On non-Me lenses, require a project/foundation selection | ||
| if (lens !== 'me' && !project?.uid) { | ||
| this.loading.set(false); | ||
| return of([] as MyDocumentItem[]); | ||
| } | ||
|
|
||
| toObservable(this.project).pipe( | ||
| switchMap((project) => { | ||
| this.loading.set(true); | ||
|
|
||
| // Me lens: fetch all documents (no project filter) | ||
| // Foundation/Project lens: scope to selected project | ||
| const projectUid = lens === 'me' ? undefined : project?.uid; | ||
| return this.documentService.getMyDocuments(projectUid).pipe( | ||
| return this.documentService.getMyDocuments(project?.uid).pipe( | ||
| catchError(() => of([] as MyDocumentItem[])), | ||
| finalize(() => this.loading.set(false)) | ||
| ); |
There was a problem hiding this comment.
initDocuments() now calls getMyDocuments(project?.uid) even when there is no active project/foundation context. Previously this page avoided fetching in non-Me lenses until a context was selected; with the current logic, switching to a non-Me lens with no selection will fetch unscoped “all my documents”, which is likely incorrect for those lenses. Consider restoring the guard (skip fetch and return [] / keep loading=false) when project?.uid is missing for foundation/project lenses, or explicitly key the fetch behavior off the active lens so Me-lens remains unscoped but other lenses require a context.
- meetings-dashboard: convert isFiltered from bare type annotation + constructor assignment to private initializer pattern per CLAUDE.md component-organization rules - events-top-bar: replace effect() with toObservable subscription to sync searchQuery input into form control without side-effecting signals - committee-table: change isBoardMember from public to protected readonly to match access modifier convention for computed signals; remove unused Signal type import Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Nuno Eufrasio <nmeufrasio@gmail.com>
Contributor
Author
Review resolution — code standards findings (MRashad26 round 1 & 2)Fixed in commit 46fff52: 1.
|
🧹 Deployment RemovedThe deployment for PR #492 has been removed. |
MRashad26
approved these changes
Apr 21, 2026
🧹 Deployment RemovedThe deployment for PR #492 has been removed. |
4 tasks
josep-reyero
added a commit
that referenced
this pull request
May 20, 2026
Add 9 patterns flagged as missed/underweighted by the H-02 KB coverage audit (2026-05-19): - code-truthiness/kpi-label-data-source-mismatch (PRs #718-#725) - code-truthiness/chart-series-data-field-mismatch (PRs #433/#443/#452) - code-truthiness/rounded-zero-delta-with-direction-arrow (#435/#508) - code-truthiness/markdown-fence-missing-language (#338/#418/#464) - code-truthiness/form-validator-mismatches-api-or-flag (#296/#319) - frontend-state-and-timing/async-context-not-ready-when-consumed (#279/#345) - templates-and-accessibility/primeng-bypass-lfx-wrapper (#326/#335/#356/#357) - templates-and-accessibility/no-wrap-truncates-dynamic-label (#485/#492/#495) - templates-and-accessibility/tooltip-on-non-focusable-host (#255/#257/#258/#447/#469/#713) Extend server-request-handling/replaceState-loses-history-state to explicitly cover raw history.replaceState (PR #578) in addition to the Angular Location.replaceState case. Read-when sections + intros updated on code-truthiness, frontend-state-and-timing, and templates-and-accessibility so the learnings-review skill routes the new patterns. The 8 "citation cleanup" and 11 "weak/noisy" pre-existing entries H-02 also flagged are deferred — IDs identified via a separate subagent investigation; cleanup/quarantine requires owner judgement and is not in this commit. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
dealako
added a commit
that referenced
this pull request
May 21, 2026
) * feat(claude): add /lfx-self-serve-self-review skill and pre-PR gate LFXV2-1827 — Phase 0 of the LFX Skills Refactor plan. - Add .claude/skills/lfx-self-serve-self-review/SKILL.md: a forked-context pre-PR self-review skill (context: fork, agent: code-standards-enforcer). Seven phases: parse base + compute local diff, load rules/hooks/arch docs/checklists, validate local PR-shape sanity (branch name, JIRA, conventional commits, rebased, DCO + GPG signing), flag protected files, run code-standards enforcement, validate upstream API contracts, emit a structured findings report with verdict NOT READY / READY WITH CHANGES / READY. Autonomous — no /review chaining, no gh pr mutation. - Symlink the four shared checklists (backend, frontend, shared-and-sql, docs) from lfx-review-pr/references/ so they stay single-source. - CLAUDE.md: replace "Pre-PR validation" with "Work cycle — mandatory before opening a PR". Leads with a CRITICAL callout that running the self-review is non-negotiable; positions /preflight as the downstream mechanical check. Add a non-negotiable entry to "What NOT to do". - skill-guidance.md: route "ready for PR" / "is my branch ready?" intents to /lfx-self-serve-self-review. Disambiguate /lfx-review-pr as the existing-PR-by-number skill. /lfx-review-pr is intentionally left unchanged — it is invoked by a reviewer in a fresh session against an open PR, so the cold-context property comes for free from session boundaries and the existing in-main orchestration preserves enforcer/metadata parallelism. Plan: lfx-architecture-scratch/2026-05-DevX-Time-to-Merge/lfx-builder-skills-refactor.md §2 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * style(claude): apply prettier formatting to self-review SKILL.md LFXV2-1827 — preflight format step added blank lines after markdown headings in the report-structure code block. Whitespace only, no content change. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * feat(claude): split pre-PR review into self-review + pr-readiness Reshape the pre-PR review lifecycle into two specialised skills with a shared code-review subagent. Triad: - /lfx-self-serve-self-review — pre-PR, forked, code-convention audit via the shared code-standards-enforcer agent. - /lfx-self-serve-pr-readiness — pre-PR, forked, no subagent. PR-shape sanity + bot-reviewer simulation against bundled empirical checklists. - /lfx-review-pr — post-PR, main thread. Uses the same agent for code review and owns the post-PR layer (verify prior comments, draft-then-approve, /review invocation). The agent absorbs everything shared between the two code-review consumers: rule library, severity vocab, false-positives, findings JSON shape, mode-dispatched diff computation, upstream API procedure, protected-file flagging via live hook parsing. A mode flag (local|pr) in the first line of the user prompt selects the fetch path; the rest of the playbook is identical. Pre-PR scope split: pr-readiness is the bot-feedback parity layer (catch what CodeRabbit / Copilot would flag before the PR opens); self-review is the project-convention audit. The post-PR reviewer does NOT replicate the bot pass — CodeRabbit and Copilot run their own analysis once the PR is open. Code checklists moved from .claude/skills/lfx-review-pr/references/ to docs/reviews/ — they are project documentation, not skill internals. The agent's system prompt marks them mandatory. Bot-finds: 16 .md files under .claude/skills/lfx-self-serve-pr-readiness/references/bot-finds/, authored from ~30 PRs sampled (2026-03-15 to 2026-05-15) of CodeRabbit + GitHub Copilot inline comments. 55 patterns total, each traced to a specific PR-comment citation or a documented CodeRabbit / Copilot category. Categories: security, type safety, async correctness, accessibility, docs-comments drift, testing, log-metric correctness, cookie trust, RxJS / signals timing, guards / interceptors ordering, route auth surface, query-param hardening, Snowflake row-shape, template binding traps, sanitizer & public URLs, OTEL & rate-limit drift. CLAUDE.md work-cycle gate updated to mandate both pre-PR skills before /preflight and push. LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): rename code-standards-enforcer agent to lfx-self-serve-code-reviewer Renames the shared review subagent for clearer scoping and matches the repo-prefixed naming used by the consuming skills. Updates all references across CLAUDE.md, skill bodies, and skill reference files. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * feat(claude): split pre-PR review into pre-commit + pre-PR phases Extracts the bot-rubric / knowledge-base portion of /lfx-self-serve-pr-readiness into a new /lfx-self-serve-learnings-review skill, and shifts the work-cycle gate so: - Pre-commit (every commit, parallel): /lfx-self-serve-self-review AND /lfx-self-serve-learnings-review run as forked subagents. - Pre-PR (once): /lfx-self-serve-pr-readiness audits PR shape only (branch, JIRA, commits, DCO + GPG, rebase, diff size). The rename better matches the actual lifecycle — KB review is code-level and benefits from running often on small diffs; PR-shape is a once-per-PR check that doesn't change mid-development. The "learnings" framing leaves room for human-flagged patterns and codebase gotchas alongside the empirical bot patterns. References moved via git mv from pr-readiness/references/ to learnings-review/references/. CLAUDE.md work-cycle section and skill-guidance.md updated to reflect the new triad shape. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): consolidate learnings-review KB to 8 files, rename bot-finds → pr-knowledge Consolidates the 16-file bot-finds knowledge base into 8 semantically coherent buckets and renames the directory to `pr-knowledge/` to reflect that the KB now spans both bot-flagged and human-flagged patterns. The 16 → 8 mapping: - security.md ← security + sanitizer-and-public-urls + cookie-trust - typescript-correctness.md ← type-safety + async-correctness - templates-and-accessibility.md ← accessibility + template-binding-traps - frontend-state-and-timing.md ← rxjs-signals-timing - server-request-handling.md ← route-auth-surface + guards-interceptors-ordering + query-param-hardening - observability-and-logging.md ← otel-and-rate-limit-drift + log-metric-correctness - data-and-snowflake.md ← snowflake-rowshape-schema - code-truthiness.md ← docs-comments-drift + testing Dedup merges (2 patterns collapsed; net 55 → 53 patterns): - error-message-identity-leak (security) + error-reveals-auth-state (route-auth-surface) → single entry in security.md (both cite PR #636). - raw-query-param-string-cast (type-safety) + raw-query-string-cast (query-param-hardening) → single entry in server-request-handling.md (both cite PR #665 project.controller.ts:774). Updates SKILL.md Phase 3 routing table, bot-rubric.md category index, and rule-ID prefixes throughout (bot-finds/<file>/<pattern> → pr-knowledge/<file>/<pattern>). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): flatten learnings-review references (drop pr-knowledge subdir) Moves the 8 pattern files from references/pr-knowledge/ up to references/ to keep the skill structure flat. Anthropic's published skill structure docs only show flat-or-nested-with-scripts/ examples for references/ — no explicit support for nested category folders. The 8 files now sit peer-to-peer with bot-rubric.md and known-false-positives.md. Rule-ID prefixes simplified from pr-knowledge/<file>/<pattern> → <file>/<pattern>; routing-table column header renamed "Bot-find file" → "Pattern file"; SKILL.md, bot-rubric.md, known-false-positives.md, and the architecture-scratch team-review doc all updated. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): drop bot framing in learnings-review KB Renames bot-rubric.md → review-rubric.md and rewrites residual bot framing throughout the KB and skill body: "Bot-flagged patterns" → "Automated review patterns"; "the bots flag" → "CodeRabbit + Copilot flag"; "Bot-quirk noise" → "Review-automation quirks"; etc. The KB now consistently uses pr-learnings / pr-knowledge framing for its own concept, and CodeRabbit + Copilot by name when referring to the upstream tooling whose published rubrics we union. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): address PR #707 review feedback Resolves five valid bot review comments from CodeRabbit + Copilot: 1. Remove stale "PR-shape sanity" claims from self-review and lfx-review-pr skill descriptions and bodies — the lfx-self-serve-code-reviewer agent only returns code | upstream-api | protected-files categories. PR-shape lives in /lfx-self-serve-pr-readiness (pre-PR) and is walked by /lfx-review-pr in its own Phase 4. Self-review report drops the now-empty PR-shape table. 2. Fix wrong docs/reviews/pr-shape.md path in .claude/agents/lfx-self-serve-code-reviewer.md — actual location is .claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md. 3. Add staged-but-uncommitted handling to the agent's mode:local — the pre-commit framing was incompatible with the previous "abort if no commits between base and HEAD" behaviour. Now audits the union of committed and staged diffs. 4. Replace ambiguous `$MB` shell-variable pattern with three-dot `<base>...HEAD` syntax in git diff commands (lets git compute the merge-base internally — no shared shell variable required). Applied to the agent, learnings-review Phase 2, and pr-readiness Phase 2. 5. Add explicit base-name normalization step ("if <base> has no slash, prefix with origin/") so plain `main` doesn't resolve to a possibly- stale local branch after `git fetch origin`. Also fixes the failing markdown-lint and Code Quality (prettier) checks in CI: MD029 ordered-list prefix, MD032 lists-around-blanks, MD040 fenced-code-block language hints, and prettier formatting across the modified files. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): address late-pass Copilot review on lfx-review-pr + self-review Resolves four additional Copilot findings on PR #707: - lfx-review-pr/SKILL.md: Phase 1 now fetches PR metadata (title / body / headRefName / baseRefName / author / additions / deletions) and captures BASE_REF, HEAD_REF, AUTHOR variables. Phases 3–5 reference these consistently instead of pulling unresolved <headRefName> / <author> placeholders that would never be populated when those phases run in parallel with the background agent. (Phase 4 path-references also switched to /tmp/pr-<N>-meta.json captured in Phase 1.) - self-review/SKILL.md: H1 heading renamed from "Pre-PR Self-Review" to "Pre-commit Self-Review" so it matches the frontmatter description and the CLAUDE.md work-cycle positioning. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * feat(claude): extend learnings-review KB with 14 patterns from older PRs Back-fills 2 months of older PR history (2026-01-15 → 2026-03-14) on top of the existing 2-month sample, bringing total coverage to ~4 months (~68 PRs, ~754 inline CodeRabbit + Copilot review comments). Pattern count: 53 → 67. New patterns added to 7 of the 8 category files; 2 new entries also added to known-false-positives.md. Most additions land in security.md (recurring auth-state / public-route patterns) and server-request-handling.md (controller-level findings). Files stay under the ~150-line guidance. Patterns admitted only when they appeared in ≥2 PRs OR were single-PR but high-severity (security / data-integrity / runtime-crash). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): extract bot-rubric to dedicated agent, drop fork meta-talk Moves the unioned CodeRabbit + Copilot review rubric out of references/review-rubric.md (deleted in the prior commit) into a dedicated bot-rubric-agent that backs /lfx-self-serve-learnings-review. The agent's system prompt now carries the severity mapping, 8-bucket category index, behavioural guidance, what-it-doesn't-cover boundary, and source provenance — loaded ambiently rather than re-read from a reference file on every invocation. Mirrors how /lfx-self-serve-self-review uses lfx-self-serve-code-reviewer. The learnings-review skill body keeps all its phases (parse args, compute diff, load references, KB pass, cross-check, false-positives, render report) — only the rubric content moves out. Mandatory section rewritten to be explicit that the audit reads ONLY the pattern files whose "Read when" condition matches the diff, not every file. Removes the "if you emit findings without reading every reference relevant to the diff" wording that read as "read everything just to be safe". Also drops harness meta-talk ("this skill runs context: fork with no subagent", "this skill runs in a forked context using the X subagent type") from learnings-review, self-review, and pr-readiness — the skill body doesn't need to know its own execution mechanics; that's the harness's concern. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): bot-rubric-agent becomes the actual review prompt The agent file previously held a calibration/index document (severity mapping, 8-bucket category table, behavioural guidance). That's not a review prompt — it's metadata. Rewriting as a real review prompt: a senior-engineer role, five concrete review areas (security, performance, code quality, architecture, testing), JSON output format, severity scale, a procedure the agent follows end-to-end (compute diff, route KB files, review pass, cross-check, drop false positives, return JSON). The KB cross-check is layered on top — the agent applies generic review areas AND quotes specific KB pattern entries when matches appear. KB-cited findings must quote their source; generic findings must cite file + line. The skill body becomes a thin orchestrator: parse args, hand off to the agent, render the report. All review-area knowledge, severity calibration, routing, and cross-check discipline now lives in the agent's system prompt where it's loaded ambiently. Skill descriptions scrubbed of rubric-provenance framing — the agent just is the rubric. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): move KB routing + cross-check into skill, agent stays generic Rebalances responsibilities between the bot-rubric-agent and the learnings-review skill body. The agent is now a generic comprehensive code-review rubric (role, five review areas, severity scale, output format) — it knows how to review but doesn't know which files to load or which routing rules apply. The skill body owns the operational concerns: which pattern files are mandatory, which are conditional on changed-file paths (Phase 3 routing table), how to compute the diff (committed + staged, base normalization, three-dot syntax), the cross-check discipline, the false-positive drop phase, and the report rendering. The agent picks up KB pattern files when the skill loads them and follows the cross-check rule generically ("if a finding matches a provided pattern entry, quote it; otherwise cite a review area sub-rule"). Source-of-truth for the routing lives in one place — the skill body — so adding a new pattern category only requires updating the skill's routing table, not the agent. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): collapse pre-commit skills into self-contained subagents The Skill tool runs synchronously and can't be invoked in parallel — so the previous "spawn both pre-commit skills in parallel" framing was aspirational, not real. The Agent tool DOES support run_in_background, so the only way to get actual parallel pre-commit reviews is to make the reviewers spawnable agents, not skills. Deletes the two pre-commit skill wrappers: - /lfx-self-serve-self-review (skill) → removed - /lfx-self-serve-learnings-review (skill) → removed And folds their orchestration (arg parsing, KB file routing, report rendering) into the two subagents they used to wrap: - lfx-self-serve-code-reviewer (existing agent): adds an Inputs section (mode / base / number / extra), and mode-dispatched output — mode:local renders a markdown review report directly; mode:pr returns JSON for /lfx-review-pr to compose into a draft review. Each agent is now self-contained — no references to other skills or agents. - bot-rubric-agent (existing agent): adds arg parsing (base, extra), diff computation (committed + staged, three-dot syntax, base normalization), KB file routing table (moved from the deleted skill body), KB cross-check discipline, false-positive drop, and markdown rendering. Self-contained. The KB pattern files move from .claude/skills/lfx-self-serve-learnings-review/references/ to .claude/pr-knowledge/ — peer to the agent, no orphan skill-shaped directory. Path references updated throughout. CLAUDE.md work-cycle rewritten to spawn both subagents in parallel via the Agent tool (run_in_background: true) — the parallelism claim is now literally true rather than aspirational. skill-guidance.md updated to advertise the two subagents alongside the surviving skills. /lfx-self-serve-pr-readiness and /lfx-review-pr keep their skill form (they don't need parallelism); /lfx-review-pr still spawns lfx-self-serve-code-reviewer in mode:pr unchanged. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): rename bot-rubric-agent → lfx-self-serve-learnings-reviewer The agent grew well past "just the bot rubric" — it now owns arg parsing, diff computation (committed + staged, three-dot syntax, base normalization), KB file routing, the senior-engineer review rubric (security / performance / code-quality / architecture / testing), KB cross-check discipline, false-positive drop, and markdown report rendering. "bot-rubric-agent" undersells it. Renames the agent file and all references: - .claude/agents/bot-rubric-agent.md → lfx-self-serve-learnings-reviewer.md - Frontmatter `name:`, H1 heading, and all cross-references throughout CLAUDE.md, skill-guidance.md, pr-readiness/SKILL.md, the lfx-self-serve-code-reviewer agent's scope-boundaries section, and the pr-knowledge KB files' "Cross-checked by..." cross-refs. Naming rationale: pairs symmetrically with lfx-self-serve-code-reviewer (both end -reviewer, both prefixed lfx-self-serve-), reflects the KB content the agent reviews against (the "learnings" accumulated from past PR review comments at .claude/pr-knowledge/), and drops the bot framing entirely. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): emphasize "wait for both background subagents before committing" The pre-commit work-cycle spawns two subagents in background (run_in_background: true) for parallel execution. The risk: Claude spawns them, then proceeds to git commit before either notification arrives. The reviews then come in too late to act on. Strengthens the wait step: - New "What NOT to do" bullet: don't run git commit while either pre-commit subagent is still running in the background. - Pre-commit phase step 3 rewritten to be explicit: "WAIT — do not run git commit until both background tasks have completed and surfaced their reports. You'll get one completion notification per agent. Do not proceed to step 4 with only one report in." - Rerun step (5) restated to require waiting on the re-spawned pair too. - Closing line: "The cost of waiting is small (each runs in seconds); the cost of committing on incomplete review is wasted reviewer time." Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): move pr-knowledge KB from .claude/ to docs/reviews/knowledge-base/ The empirical pattern knowledge base lived at .claude/pr-knowledge/ — buried under Claude-machinery. Moving to docs/reviews/knowledge-base/ puts it where the existing code-convention checklists live (peer to docs/reviews/{frontend,backend,shared-and-sql,docs}-checklist.md), so both review surfaces sit together as discoverable team-facing review documentation rather than agent-internal config. The lfx-self-serve-learnings-reviewer agent's Step 2 routing table updated to read from the new path. CLAUDE.md work-cycle, skill-guidance, and the team-review snapshot all updated to match. 9 KB files renamed via git mv; only path strings changed in the referencing files. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): move code reviewers from pre-commit to post-commit Flip the reviewer pair (lfx-self-serve-code-reviewer + lfx-self-serve-learnings-reviewer) from a pre-commit gate that synchronously blocks the commit window to a post-commit pair spawned in the background via the Agent tool. Devs commit first, fire the pair off with run_in_background: true, and keep building. Findings from a returned pair are rolled into the next commit; the safety bound shifts to the PR boundary, where the queue is drained and any remaining CRITICAL / reasonable SHOULD_FIX must be addressed before opening for review. Both agents already diff cumulatively against origin/main, so a later review pair sees the full state and supersedes any earlier in-flight one — no coordination logic needed. Multiple concurrent pairs are normal during fast iteration. Updates: CLAUDE.md work cycle and "What NOT to do" bullets, .claude/agents/* descriptions and bodies, .claude/rules/skill-guidance.md routing + trigger phrases + cowork section, and .claude/skills/lfx-self-serve-pr-readiness SKILL.md description, body, and companion-skills note. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): clean up pre→post-commit drift in reviewer subagents Both post-commit review subagents had three stale references missed in the pre→post-commit flip (commit e5202093): - code-reviewer body line 49 still framed `mode: local` as pre-commit; rewritten to describe the post-commit invocation while preserving mid-edit staged-diff handling. - code-reviewer report-template H1 still printed "(pre-commit)"; flipped to "(post-commit)". - learnings-reviewer report-template H1 still printed "Pre-commit code review"; flipped to "Post-commit code review". Net effect of the drift: every report either agent emitted under the new workflow was self-labeled as pre-commit, contradicting CLAUDE.md and the frontmatter descriptions. Caught by the post-commit review pair spawned against e5202093. Also picks up a prettier reflow on .claude/rules/skill-guidance.md table alignment (header row trailing-whitespace) flagged in the same review. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(reviews): add prototype-pollution KB pattern from PR #673 Add a new SHOULD_FIX entry to docs/reviews/knowledge-base/security.md: `security/prototype-pollution-via-dynamic-key-assignment`. Captures the pattern that PR #673 surfaced via Copilot — a recursive plain-object walker assigning keys via `result[key] = ...` over `Object.entries` can be coerced into `Object.prototype` pollution if the input carries an own-property literally named `__proto__`, `constructor`, or `prototype`. A plain-object guard on the input does NOT close the hole; dangerous keys still get copied through. Surfaced during the calibration test of the new post-commit reviewer pair against PR #673 — bots on the real PR caught this, our pair did not, so the KB needs the pattern entry for future cross-check pickup. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(reviews): tighten prototype-pollution KB entry against PR #673 source Three evidence-fidelity fixes flagged by the post-commit code-reviewer on 71f66590: - Drop the fabricated `:32` line anchor. The Copilot comment on PR #673 has `line: null` (file-level after rebase) with `original_line: 32` — citing it as `object.utils.ts:32` is half-true and inconsistent with the rest of the KB's anchoring convention. Cite the file only. - Replace the silently-paraphrased Copilot quote tail with the verbatim text. The original ends with the prototype-pollution rationale and the `Object.create(null)` / "safely defining dangerous keys" recommendations; the paraphrase mangled both inside quote marks. - Reorder the Fix to lead with `Object.defineProperty(result, key, ...)` — this is what PR #673 actually merged, and the in-code comment ("Use defineProperty to bypass setters (e.g. __proto__) and prevent prototype pollution when keys come from untrusted sources like JSON.parse") is now quoted as the empirical rationale. Explicit-skip and `Object.create(null)` stay as alternatives. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude,reviews): address bot review findings on PR #707 Verified each finding against the source before accepting. Six fixes, one removal, two stale-reply candidates (replies handled separately, not in this commit). 1. `.claude/skills/lfx-review-pr/SKILL.md` — three fixes Copilot flagged: - Phase 1 now adds `files` to `gh pr view --json` so Phase 4's external-refs check has the changed-file list it needs. - Document explicitly that Bash tool invocations are isolated — env vars do NOT survive across calls. Each later phase that uses BASE_REF/HEAD_REF/AUTHOR re-derives them via `jq -r` from /tmp/pr-<N>-meta.json at the top of its own Bash invocation. - Phase 4 commit-subject extraction now uses `git log --format='%H %s' "origin/$BASE_REF..origin/$HEAD_REF"` instead of `gh api .../commits --jq '.[].commit.message'`. The latter returns the full message (subject + body) which false-fails the conventional-commit regex on body content. 2. `.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md` + `CLAUDE.md` — Copilot caught a real factual error: both files claimed commitlint enforces a 72-character header cap, but commitlint.config.mjs only extends @commitlint/config-angular and doesn't override `header-max-length` (config-angular default is 100). Empirical confirmation: 20 of the last 50 commits on main have subjects > 72 chars (up to 79) — they all merged. The 72 is a team style guideline, not a commitlint enforcement. Both files now say so. 3. `docs/reviews/knowledge-base/known-false-positives.md` — remove the "Generated with Claude Code attribution missing" entry. Copilot flagged it as contradicting `.claude/rules/development-rules.md` ("Always prepend 'Generated with [Claude Code]' if you assisted with the code"). Empirical confirmation: 29 source files under `apps/lfx-one/src/` currently carry the attribution. The false-positive filter would have suppressed legitimate findings on AI-assisted files. Drop the entry; let the bots flag it; humans judge per-file based on whether the file was AI-assisted. 4. `docs/reviews/knowledge-base/security.md` — two CodeRabbit Major findings on KB accuracy plus a CI prettier failure: - `[innerHTML]` pattern wording rewritten. Old text said "Angular doesn't auto-sanitize `[innerHTML]`" — wrong. Angular's default HTML sanitizer (SecurityContext.HTML, active on `[innerHTML]`) removes `<script>` and dangerous attributes. The real risk surface the entry captures is (a) `DomSanitizer.bypassSecurityTrustHtml` disabling sanitization, or (b) user content flowing into a code path where bypass is later added. Reworded accordingly; severity unchanged. - `bypassSecurityTrustUrl` fix wording corrected. Old text said to "bind via `[innerHTML]` or `[attr.href]`" — `[innerHTML]` is wrong; it pairs with `bypassSecurityTrustHtml`, not the URL variant. Now bind via `[href]` or `[attr.href]`, never `[innerHTML]`. - Prettier reformatted four lines (two semantic edits + two unrelated `*...*` → `_..._` italic-style conversions). File now passes `prettier --check`. 5. `docs/reviews/knowledge-base/typescript-correctness.md` — Copilot caught the KB recommending a non-existent helper (`parseLocalDate(s)` in `@lfx-one/shared/utils/date`). The shared package already exposes `parseLocalDateString(dateStr): Date` in `packages/shared/src/utils/date-time.utils.ts` (re-exported via `@lfx-one/shared/utils`). Fix now points to the existing helper. Two threads (CLAUDE.md outdated KB path, learnings-reviewer.md:56 KB path) refer to claims about old paths (`.claude/pr-knowledge/`) that were already updated in commit 11d5ba81 — those are stale and will be replied/resolved without a code change. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): use pull-refspec fetch in lfx-review-pr for fork PRs Phase 1 of `/lfx-review-pr` previously did `git fetch origin "$BASE_REF" "$HEAD_REF"` and downstream phases read files via `git show "origin/$HEAD_REF:<file>"`. That assumes the PR's head ref is reachable under `origin/*`, which is false for PRs opened from forks — `headRefName` points to a branch on the fork's remote, not the upstream `origin`. Switched to GitHub's `pull/<N>/head` refspec, which mirrors the PR head into the upstream repo regardless of whether the PR came from a fork. The fetch creates a local ref at `refs/pr/<N>/head`, and all downstream phases (3, 4, including merge-base / git log / git show) reference it uniformly. `HEAD_REF` is kept in the shell-state caveat only because the branch-name regex check (Phase 4) still needs the literal branch name. Resolves the final Copilot finding on PR #707 (.claude/skills/lfx-review-pr/SKILL.md:48). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): propagate pull-refspec fix to code-reviewer agent + pr-shape The previous commit (38e586a6) switched `/lfx-review-pr`'s skill body to use GitHub's `pull/<N>/head` refspec so the PR-head fetch works for fork PRs. Two downstream consumers still carried the old `origin/<headRefName>` pattern and would re-introduce the same fork-PR bug: - `.claude/agents/lfx-self-serve-code-reviewer.md` `mode: pr` — `/lfx-review-pr` Phase 2 spawns this exact agent, so on a fork PR the agent's diff fetch + `git show` reads would fail (or silently audit a stale local ref). Now uses the same refspec + `refs/pr/<N>/head`. - `.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md:43` post-PR rebase recipe — still showed the legacy form. Updated to use `refs/pr/<N>/head` and pointed contributors at the skill that mediates the fetch. Also dropped the unused `isCrossRepository` field from the skill's metadata fetch (the refspec approach deliberately works uniformly so nothing branches on the fork flag), and added a note explaining why the local destination namespace is `refs/pr/<N>/head` (no collision with `refs/heads/` or `refs/remotes/origin/`). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): drop redundant "Why post-commit" rationale from work cycle The CRITICAL banner above already states what the rationale subsection explained (post-commit + drain at PR boundary, work cycle bound by the PR not the commit). The "Post-commit (after every commit, parallel, asynchronous)" and "Pre-PR (drain the queue, then open)" subsections below restate the same operating model in actionable form. Removing the rationale tightens the work-cycle block to two purely-operational subsections without losing any guidance. Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(reviews): correct stale KB references to validation helper + LensService API Two new Copilot findings on PR #707 pointed out stale identifiers in the post-commit reviewer's knowledge base: - `server-request-handling.md` referenced `apps/lfx-one/src/server/helpers/validation.ts` in two places (the Fix for `raw-query-string-cast` at line 131 and the Detect for `missing-typeof-string-validation` at line 153). The file is actually `validation.helper.ts`. Updated both occurrences. - `templates-and-accessibility.md` recommended `LensService.getCurrentLens()` to fetch the active lens, but that method doesn't exist. `LensService` exposes `activeLens: Signal<Lens>` (at `apps/lfx-one/src/app/shared/services/lens.service.ts:23`). Rewrote the Fix to point at the signal call. The post-commit reviewer pair would have cited these files in future reviews and sent contributors searching for non-existent paths / APIs. Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): scope post-commit reviewer pair to pre-PR + add subagent guidance The post-commit reviewer pair (`lfx-self-serve-code-reviewer` + `lfx-self-serve-learnings-reviewer`) is now scoped explicitly to the pre-PR phase. Once the PR is open and the bot-iteration loop is live, CodeRabbit + Copilot auto-trigger on every push and become the audit surface — stacking the subagent pair on top adds latency without proportional signal and makes the iteration loop too slow. Changes: - CLAUDE.md "Work cycle" CRITICAL banner now has two paragraphs: the pre-PR mandate (unchanged in spirit) and the post-PR carve-out. - New "Post-PR iteration" subsection captures the bot-driven loop: wait for bots, triage, fix(review) commit, reply + resolve, push, repeat. - "What NOT to do" bullets re-scoped to make the pre-PR/post-PR boundary explicit. - `.claude/rules/skill-guidance.md` mirrors the carve-out in the subagent table, trigger phrases, and Cowork-sessions guidance. - New "Lean on subagents" paragraph in Working mode points at the full delegation surface (Explore for searches, parallel Agent calls for independent investigations, context-heavy reads, the LFX reviewer pair). - Tightened the additions to keep total CLAUDE.md line count flat: banner rephrased, redundant Post-commit step 6 folded into step 5, closing paragraph dropped (its content lives in the banner now). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): convert review subagents to launcher-wrapped skills Replaces the two per-repo review agents with skills that wrap a launcher instruction ("Launch a subagent in the background with the following instructions:") around the verbatim playbook. Skills package more naturally for cross-cwd distribution (e.g. via a Claude Code plugin), while agents are bound to the primary cwd's .claude/agents/ and don't surface from sibling repos opened via --add-dir. New skills (each launches a general-purpose subagent in the background with run_in_background: true): - .claude/skills/lfx-self-serve-code-review/SKILL.md - .claude/skills/lfx-self-serve-learnings-review/SKILL.md Both launcher bodies include a "Launcher discipline — non-negotiable" callout making explicit that the parent must pass the body verbatim when constructing the Agent prompt — no summarizing, condensing, or pre-routing based on the diff (the playbook owns its own diff-routing logic in Step 2; trimming it collapses Step 4 cross-check, drifts severity calibration, and breaks the documented output templates). The same discipline note is propagated to CLAUDE.md and .claude/rules/skill-guidance.md so the requirement appears at every entry point. References updated: - CLAUDE.md — Lean on subagents, What NOT to do, Work cycle (post-commit step 2 + drain-queue steps), launcher-discipline callout - .claude/rules/skill-guidance.md — Skills table, post-commit section, trigger phrases, Cowork-sessions guidance, verbatim requirement - .claude/skills/lfx-review-pr/SKILL.md — Phase 2 invokes the lfx-self-serve-code-review skill (the skill body spawns the subagent) instead of spawning the agent directly; description + Phase 3/6/7/8 references retitled accordingly - .claude/skills/lfx-self-serve-pr-readiness/SKILL.md — description and Companion skills references updated - docs/reviews/knowledge-base/{data-and-snowflake,known-false-positives, security}.md — KB citations point at the new skill names Deletes (file content preserved in the new skill bodies; git records as renames): - .claude/agents/lfx-self-serve-code-reviewer.md - .claude/agents/lfx-self-serve-learnings-reviewer.md Lands on refactor/skills-launcher (off feat/LFXV2-1827) so PR #707 is not affected. No JIRA ticket attached yet — branch is for follow-up work pending a ticket. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): compact review skills, switch to code-reviewer subagent Two changes to the post-commit review pair, plus the rule-surface patches that keep audit coverage whole after the compaction. Skill compaction - code-review SKILL.md: 456 -> 262 lines. Drop the inlined Step 5 checklist (Angular component / frontend service / SSR / backend three-file / service / controller / logging / auth / route / shared / TypeScript / Tailwind / general rules) -- the playbook's own Step 5 preamble already admitted the bullets duplicated `.claude/rules/*.md` and the four `docs/reviews/` checklists, which Step 2 loads anyway. Collapse the old Step 3+4 into a single audit-pass-with-cross-check. - learnings-review SKILL.md: 207 -> 158 lines. Drop the generic rubric (the security / performance / code-quality / architecture / testing categories). Procedure is now pure KB matching: load routed pattern files, run each entry's `**Detect:**` rule, emit findings only when a pattern entry is quotable. Severity comes from the entry header, not a parallel scale. Subagent type - Both skills now launch `subagent_type: code-reviewer` instead of `general-purpose`. The built-in code-reviewer agent is already disposed for code review and has the full toolset. - CLAUDE.md (2 spots), skill-guidance.md, lfx-review-pr SKILL.md updated to match. Rule-surface gap fills (preserve audit coverage) A coverage audit caught four real signals that lived only in the stripped Step 5 checklist. Adding them to the loaded rule surface so the subagent still picks them up via Step 2: - frontend-checklist.md sec. 13: HttpClient calls must target a real `/api/...` endpoint (no mock/placeholder URLs). - backend-checklist.md new sec. 12: external API calls go through `MicroserviceProxyService.proxyRequest()` (no raw fetch/axios); reads use `/query/resources`, writes use `/itx/...`. - commit-workflow.md: no Claude co-author trailers. Note: the old SSR rule (`typeof window !== 'undefined'`) was a conflict with `.claude/rules/ssr-safety.md` (which says use `isPlatformBrowser`). The new playbook delegates SSR to ssr-safety.md as the canonical source. Branch isolation: this stays on `refactor/skills-launcher`. PR #707 is untouched. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): move review skills to Critical/Important output system Two related cleanups to the post-commit review pair. Mix-and-match subagent types - code-review keeps `subagent_type: code-reviewer` — its rule-surface disposition aligns with our constrained-audit mode. - learnings-review flips back to `subagent_type: general-purpose`. The built-in code-reviewer agent's review disposition was competing with the KB-only gate; general-purpose has no review disposition, so the KB-match rule in Step 3 is uncontested. - CLAUDE.md and skill-guidance.md updated to differentiate. Unified output system (Critical / Important / suppressed below 80) Both playbooks now use the same severity terms throughout: - Severities: Critical (confidence 90-100) and Important (80-89). Findings below 80 are suppressed. - code-review's JSON contract for `mode: pr` now emits `"severity": "Critical | Important"` with an explicit `"confidence"` field. - Protected-files and unverified upstream-api findings emit at `severity: Important` regardless of the confidence floor (deterministic flags, not quality judgments). - Severity calibration section rewritten with Critical/Important tiers. - code-review's `mode: local` output spec retains the three-section structure (Protected files / Upstream API / Findings) so deterministic flags stay surfaced separately from code findings. Tidiness - Dropped references to the built-in code-reviewer agent's system prompt. - Dropped "verdict" language (NOT READY / READY WITH CHANGES / READY) — the new system uses Critical/Important severities, not a verdict ladder. - Dropped the cross-system severity-mapping subsection (no longer needed). Note: two `CRITICAL/SHOULD_FIX/NIT` literals remain in learnings-review (Step 2 pattern entry format example + Step 3 confidence mapping bullet). Both are KB-content references — the KB files themselves still use those header labels. Migrating the KB to Critical/Important headers is a separate follow-up. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(reviews): migrate KB severity labels to Critical/Important/Nit Two related changes. KB migration - Every pattern entry header in `docs/reviews/knowledge-base/*.md` updated: `CRITICAL` → `Critical`, `SHOULD_FIX` → `Important`, `NIT` → `Nit`. - 72 occurrences across 8 KB files migrated via word-boundary substitution. - `known-false-positives.md` had no severity labels (different structure) — unchanged. - The learnings-review playbook's KB-content references (Step 2 entry format example + Step 3 confidence mapping bullet) now use the new labels to match what the subagent will read from disk. code-review tidying - Restored the Nit tier in the Severity calibration section (Critical / Important / Nit). - Marked the per-tier examples as illustrative, not exhaustive — apply the same severity bucket to any violation that fits the same pattern of impact. - Restored protected-files findings to `severity: Nit` (informational presence flag, bypasses the ≥80 confidence floor). Unverified upstream contracts stay at `severity: Important`. - JSON contract for `mode: pr` now emits `"severity": "Critical | Important | Nit"`. Not in scope for this commit (flagged separately): the PR-shape findings system (`pr-readiness`, `lfx-review-pr` Phase 4) still emits CRITICAL/SHOULD_FIX/NIT — that's a separate audit stream. CLAUDE.md and skill-guidance.md work-cycle prose still says "address every CRITICAL" when describing post-commit pair findings (now stale; the pair emits Critical/Important). The four `docs/reviews/*-checklist.md` files use a distinct parenthesized labeling convention `(SHOULD FIX)`. Migrate any of these separately if desired. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): review pair audits latest commit by default; drop mode dispatch Behavior change - Both `/lfx-self-serve-code-review` and `/lfx-self-serve-learnings-review` default to auditing only the latest commit (`git show HEAD`) rather than the cumulative diff against `origin/main`. Each commit gets reviewed exactly once, by the pair invoked after it. - A new optional `pr: <N>` arg overrides the diff source to a full PR. Use case: `/lfx-review-pr` calls the code-review skill with `pr: <N>`. Code-review structural cleanup - Removed the `mode: local` vs `mode: pr` mode dispatch. The skill now has a single flow with a brief PR override block in Step 1. - Removed the JSON output format. Single markdown report covering Protected files / Upstream API validation / Findings (Critical / Important grouping, parser-friendly bullet format). - Removed the `base: <ref>` parameter (no use case left when default is HEAD~1..HEAD and PR mode is governed by GitHub's base ref). - Removed the Severity mapping subsection (was a cross-system bridge, redundant now). Downstream propagation - `/lfx-review-pr` Phase 2 now passes `pr: <N>` instead of `mode: pr` and consumes a markdown report (not JSON). Phase 6/7/8 embed the report verbatim under a "Code review" section. Inline comments derive from PR-shape findings + parsing the report's Findings bullets. - `/lfx-review-pr` frontmatter description updated to match. - `CLAUDE.md` work-cycle prose updated: "wait for every in-flight pair" (each covers one commit, no superseding); "Multiple in-flight pairs" paragraph reframed; severity prose migrated to Critical/Important to match what the pair actually emits. - `skill-guidance.md` mode references dropped; severity prose migrated. Compaction - code-review SKILL.md: 244 → 189 lines (-23%). - learnings-review SKILL.md: 142 → 132 lines (-7%). - lfx-review-pr SKILL.md: 245 → 235 lines. Workflow implication: with per-commit reviews instead of cumulative, the pre-PR drain step requires waiting for every in-flight pair (each covers one commit independently), not just the latest. The bots on the open PR catch any cumulative-pattern issues that per-commit review misses. Not in scope (still uses CRITICAL/SHOULD_FIX/NIT): `/lfx-self-serve-pr- readiness` and `/lfx-review-pr` Phase 4 PR-shape findings, the four `docs/reviews/*-checklist.md` parenthesized labels. Migrate separately if desired. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): correct review-pair Step 1 commands for latest-commit semantics Three issues caught while auditing whether the post-commit-pair playbooks actually have what they need to review the latest commit: 1. `git show HEAD` does NOT include the stat block by default (only the patch). The playbooks claimed "full diff + stat" but Step 6's report header asks for "files changed and additions/deletions" — without the stat, the subagent has no source for that. Step 2's path-conditional document routing also needs the changed-file list, which the stat block provides cleanly. Fix: `git show --stat HEAD` (single command gives stat header + full diff). 2. No empty-commit handling. A `git commit --allow-empty` would produce a HEAD with 0 files changed. The previous cumulative-diff playbook had an explicit abort; the latest-commit version lost it. Fix: abort with `No changes to review in the latest commit.` when the stat block shows `0 files changed`. 3. No defense against the `code-reviewer` agent's built-in disposition. Its system prompt defaults to "review unstaged changes from git diff." Our playbook overrides with `git show HEAD`, but didn't explicitly say "ignore unstaged / staged work" — leaving a window for the agent to fall back partially and pull in WIP changes alongside the commit. Fix: explicit "Do not include unstaged or staged work-in-progress — review HEAD's diff, nothing else." Applied to both playbooks for consistency (learnings-review uses general-purpose so the risk is lower, but the instruction is cheap insurance). The rest of the playbooks (Step 2 routing, Step 3 audit pass, Step 4 upstream API validation, Step 5 protected files, Step 6 report) is already scoped correctly for single-commit review — no leftover cumulative-diff assumptions found. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): pass -p so git show --stat emits the patch da78e1bb intended `git show --stat HEAD` as "stat + full diff in one command" — but --stat without -p suppresses the patch (verified: 0 diff markers vs 2 with -p). The latest-commit playbook left the subagent with only the stat block, breaking Step 3's per-file audit. Add -p to both review playbooks, tighten the surrounding prose to make the stat-then-patch ordering explicit, and reword the empty-commit abort as absence-of-shortstat-line — `git commit --allow-empty` produces no stat block at all, not a "0 files changed" line. Caught by the post-commit review pair on da78e1bb. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(claude): pin review subagents to Opus (currently 4.7) Both `/lfx-self-serve-code-review` and `/lfx-self-serve-learnings-review` now pass `model: "opus"` explicitly when launching their background subagents. Opus 4.7 is the current resolution; the Agent tool's `model` enum (`sonnet | opus | haiku`) picks the latest in the family. Why explicit: - code-reviewer agent has two plugin-shipped variants — `feature-dev` declares `model: sonnet`, `pr-review-toolkit` declares `model: opus`. Plugin load order decides which wins, and we don't want Sonnet for this audit. The explicit override at the launch site guarantees Opus regardless of which variant is active. - general-purpose has no default model — without the override it inherits from the parent, which is environment-dependent. The Agent tool resolves `"opus"` to the latest Opus model available, so the playbook stays correct as new Opus versions ship. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): two-mode review skills + pre-PR full-branch sweep Collapse review skills from three modes to two and add a pre-PR cross-commit drift check. Skills (code-review + learnings-review) - Drop `pr: <N>` mode. The skills now have only two modes: - Default: `git show --stat -p HEAD` (latest commit) — post-commit pair. - `base: <ref>`: cumulative `<ref>...HEAD` diff — pre-PR full-branch sweep AND `/lfx-review-pr`. - This is the same `base:` we removed earlier, re-added because the two-mode design is genuinely simpler than three (one fewer branch in Step 1, no PR-fetching logic in the skill itself). /lfx-review-pr - No longer passes `pr: <N>` to code-review. Now reads `baseRefName` from `/tmp/pr-<N>-meta.json`, fetches `origin/<baseRefName>` fresh, and passes `base: origin/<baseRefName>` to the code-review skill. - Requires HEAD to be the PR's branch (`git rev-parse --abbrev-ref HEAD` must equal `headRefName`). Author scenarios pass naturally; external reviewers run `gh pr checkout <N>` first. Phase 1 verifies and exits with instructions if the check fails. - The PR-specific bits (Phase 3 prior-comment verification via `refs/pr/<N>/head`, Phase 4 PR-shape walk) are unchanged. CLAUDE.md work cycle - New Pre-PR step 3: full-branch sweep with `base: origin/main`, only when the branch has more than one commit. Rationale: per-commit reviews can miss cross-commit drift (issue introduced in commit N only made dangerous by commit N+2's changes — neither's individual review surfaces it). Single-commit branches skip — already covered by the post-commit pair. - CRITICAL banner: requires sweep alongside drained queue + pr-readiness before opening a PR. - "What NOT to do": added "Open a multi-commit PR without running the pre-PR full-branch sweep". - Launcher discipline args mention updated from `mode, base, extra` to `extra, base` (drops the gone `mode:`). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): clean up work-cycle prose (explicit skills, no in-flight pair) Tighten the post-commit and pre-PR sections in CLAUDE.md and propagate fixes to skill-guidance.md and pr-readiness/SKILL.md. Post-commit - Step 2 rewritten: name both skills explicitly with their disposition (which subagent type each launches), say "no args" up front since the default audits the just-made commit, and a tighter launcher-discipline paragraph (verbatim + why). - Step 5 reframed without "in-flight pairs" — "it's fine to keep committing while reviews are still running." Pre-PR - Step 1 reframed as "wait for every running review to complete" (no more "in-flight review pair" jargon). - Step 3 (full-branch sweep) names both skills explicitly with the `"base: origin/main"` arg, and a brief launcher-discipline reminder. Other propagation - CRITICAL banner: "every in-flight pair" → "every running review". - "What NOT to do": same swap. - skill-guidance.md: post-commit-skills table row for code-review dropped the removed `pr: <N>` mention; learnings-review row dropped the old "comprehensive code-review rubric" framing (now empirical- pattern matching). Both rows now mention the `base: origin/main` pre- PR sweep option. Cowork-section "drain the queue" instruction adds the full-branch sweep step. - pr-readiness/SKILL.md: drains "every running review" + checks the full-branch sweep ran on multi-commit branches; severity prose migrated to Critical / Important. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): reorder CLAUDE.md sections + drop redundant scope-line - CLAUDE.md: move "What NOT to do" to after the Work cycle section so the work cycle reads first. Two minor blank-line spacings between the launcher-discipline paragraph and the next numbered step. - code-review SKILL.md: drop trailing redundant "If a finding fits one of those surfaces, drop it." — the bulleted scope-boundaries lines above already say the same thing. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(review): address Copilot threads + failing checks - known-false-positives.md: correct step reference (Step 5 → Step 4) to match the learnings-review skill body. - learnings-review SKILL.md: wrap the finding-template line in a fenced code block so the nested backtick around <rule-id> stops tripping markdown-lint MD038 (no-space-in-code). - Prettier-format four files flagged by the format:check CI step. PR description already reflects the per-commit (non-cumulative) post-commit behavior — the Copilot thread on CLAUDE.md fired against a stale revision. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): split code-review into general + convention layers Reshape the post-commit code-review skill so it does two layered passes instead of pure rule-citation against the documented surface: - Step 1: compute the diff. - Step 2 (NEW): general code review on the raw diff via the code-reviewer subagent's native disposition, run BEFORE loading any repo docs so it stays untainted by repo-specific framing. No source citation required. - Step 3: convention audit, with 3.1 loading the docs and 3.2 walking the audit with cross-check discipline (every finding quotes a loaded source). - Step 4: upstream API contract validation (backend only). - Step 5: render report. Three sections in order — General review / Upstream API / Repo conventions. The previous design constrained the agent to rule-citation only, dropping unsourced findings. That filtered out legitimate bugs that don't violate a documented rule and forced the bots to catch them post-PR. Splitting the cross-check discipline by section (general review: native concrete- failure-mode standard; conventions: quote-the-rule) lets the agent lead with its strength while keeping the hallucination guard where it matters. Protected files moves out of code-review into PR-readiness — it's a deterministic shape concern, not a code-quality judgment, and only needs to fire once on the cumulative branch state. Added as pr-shape/protected-files-touched (SHOULD_FIX, sec. 7) in pr-shape.md, parsed from the guard-protected-files.sh hook. Renumbered the PR-only items to 8/9. Cross-refs updated in skill-guidance.md, the lfx-review-pr playbook, and CLAUDE.md's post-commit step description. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): trim Step 1 / Step 5 / Severity in code-review playbook The diff-computation, report-rendering, and severity-calibration sections were verbose enough to dilute the actual instructions. Compressed: - Step 1 (Compute the diff): collapse the two mode blocks to single command lines and drop the redundant prose. The bash recipe is short enough to be self-documenting. - Step 5 (Render the report): replace per-section prose with a one-line bullet template per section. The header + three-section structure stays; the noise around it doesn't. - Severity calibration: keep the three-bucket definitions but drop redundant qualifiers in the examples. Total drop ~30 lines without losing any operational instruction. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * refactor(claude): replace base:<ref> arg with branch keyword, hardcode main The `base: <ref>` arg on the review skills was over-parameterized. In practice every caller passed `origin/main`: - Pre-PR full-branch sweep: `base: origin/main`. - `/lfx-review-pr`: `base: origin/<baseRefName>`, which is `origin/main` for every PR in this repo. Replace with a single `branch` keyword (presence-flag) that triggers full-branch mode against a hardcoded `origin/main`. Drops one parameter, one parsing path, and the framing of "cumulative diff" — it's just the branch's diff against main. Affected: - code-review SKILL: `branch` keyword, `origin/main...HEAD` hardcoded. - learnings-review SKILL: same. - lfx-review-pr Phase 2: passes `branch` instead of `base:`. - lfx-review-pr Phase 1: fetches main + the PR's actual baseRefName (kept for Phase 4's rebase check, which still needs the PR's own base; only the code-review audit is hardcoded to main). - Work-cycle in CLAUDE.md + skill-guidance.md + pr-readiness: updated the pre-PR sweep invocation to use `branch`. Tested the new commands locally — default mode (`git show --stat -p HEAD`) and full-branch mode (`git fetch origin && git diff origin/main...HEAD`) both work as expected on this branch. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * fix(review): address Copilot threads on PR 707 - lfx-review-pr Phase 1: replace branch-name precondition with a commit-SHA check (`git rev-parse HEAD == git rev-parse refs/pr/<N>/head`) so it works under detached HEAD or `gh pr checkout` collisions where the local branch name differs from the PR's headRefName. - lfx-review-pr Phases 6/7/8: update the report-section names from the old "Protected files / Upstream API / Findings" to the actual "General review / Upstream API / Repo conventions" emitted by the current code-review playbook. Update the inline-comment parser hint so it handles both bullet shapes (Repo-conventions adds `_Source:_`, General review doesn't). - lfx-review-pr References-used: drop `guard-protected-files.sh` — the code-review subagent no longer parses it; protected-files detection moved to `/lfx-self-serve-pr-readiness` and is walked by this skill body in Phase 4. - typescript-correctness/snake-vs-camel-case KB pattern: allow camelCase fields inline when their JSDoc explicitly marks them as UI-populated / view-only (the `Committee.behavioralClass` + `Committee.classDisplay` convention). The previous wording would false-flag that pattern. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Pepe Garcia-Reyero Sais <106924814+josep-reyero@users.noreply.github.com> * fix(review): address second Copilot batch on PR 707 - lfx-review-pr Phase 3: replace the literal `repos/{owner}/{repo}/...` placeholders with `repos/$REPO/...`, where `$REPO` is derived inline via `gh repo view --json nameWithOwner --jq '.nameWithOwner'`. Add `$REPO` to the shell-state caveat alongside `$BASE_REF` / `$HEAD_REF` / `$AUTHOR`. The Phase 1 `gh repo view` line stays as a sanity check with an accurate comment (later phases re-derive inline). - KB headers ("Read when:" lines on 8 pattern files): replace the stale "Cross-checked by Phase 5" reference with "Cross-checked in Steps 3-4 of the learnings-review playbook (KB-match gate in Step 3, false- positive filter in Step 4)". Matches the current Steps 1-6 numbering. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(reviews): apply H-02 audit findings — 9 new patterns Add 9 patterns flagged as missed/underweighted by the H-02 KB coverage audit (2026-05-19): - code-truthiness/kpi-label-data-source-mismatch (PRs #718-#725) - code-truthiness/chart-series-data-field-mismatch (PRs #433/#443/#452) - code-truthiness/rounded-zero-delta-with-direction-arrow (#435/#508) - code-truthiness/markdown-fence-missing-language (#338/#418/#464) - code-truthiness/form-validator-mismatches-api-or-flag (#296/#319) - frontend-state-and-timing/async-context-not-ready-when-consumed (#279/#345) - templates-and-accessibility/primeng-bypass-lfx-wrapper (#326/#335/#356/#357) - templates-and-accessibility/no-wrap-truncates-dynamic-label (#485/#492/#495) - templates-and-accessibility/tooltip-on-non-focusable-host (#255/#257/#258/#447/#469/#713) Extend server-request-handling/replaceState-loses-history-state to explicitly cover raw history.replaceState (PR #578) in addition to the Angular Location.replaceState case. Read-when sections + intros updated on code-truthiness, frontend-state-and-timing, and templates-and-accessibility so the learnings-review skill routes the new patterns. The 8 "citation cleanup" and 11 "weak/noisy" pre-existing entries H-02 also flagged are deferred — IDs identified via a separate subagent investigation; cleanup/quarantine requires owner judgement and is not in this commit. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> * docs(claude): address PR review feedback Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> --------- Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com> Signed-off-by: Pepe Garcia-Reyero Sais <106924814+josep-reyero@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: David Deal <ddeal@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.
What
Standardises the My Engagement section (Me lens) across all pages — Meetings, Committees, Votes, Surveys, Mailing Lists, Documents, and Events.
How
lfx-empty-statecomponenttdinstead oftrto avoid a PrimeNGtransitionspecificity conflictChecklist
Generated with Claude Code