feat(auth): implement persona auto-detection from membership #166
Conversation
Implement automatic persona detection based on committee membership to streamline user experience and enforce role-based access control. Changes: - Add persona-helper.ts utility for committee-based persona detection - Refactor PersonaService to support auto-detected personas (read-only when auto-detected) - Initialize persona during SSR from backend auth context - Add CommitteeService.getCommitteeMembersByCategory() method - Update AuthContext interface to include optional persona field - Hide PersonaSelector UI when persona is auto-detected - Add logout menu item to main layout - Fix organization involvement loading state display - Remove project selection requirement for committee group creation The persona detection checks Board and Maintainers committee categories with priority-based selection when users belong to multiple categories. Falls back gracefully if detection fails to ensure SSR is not blocked. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
apps/lfx-one/src/server/services/committee.service.ts (1)
408-416: Sanitize username in logs for consistency.Other methods in this service use
Logger.sanitize()for logging user-related data (see Line 296). The username should be sanitized here for consistency and to prevent potential log injection.Apply this diff:
req.log.info( - { + Logger.sanitize({ operation: 'get_committee_members_by_category', username, category, memberships_count: userMemberships.length, - }, + }), `Fetched user committee memberships for category: ${category}` );apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.ts (1)
29-48: Consider disabling the form control when persona is auto-detected.While the selector is hidden in the template when
isAutoDetected()is true, the form control remains enabled. Although not currently an issue, disabling the control would provide defense-in-depth and clearer semantics.Add an effect to disable the control when auto-detected:
import { effect } from '@angular/core'; public constructor() { this.form = new FormGroup({ persona: new FormControl<PersonaType>(this.personaService.currentPersona(), [Validators.required]), }); // Disable form control when persona is auto-detected effect(() => { if (this.isAutoDetected()) { this.form.get('persona')?.disable(); } else { this.form.get('persona')?.enable(); } }); this.form .get('persona') ?.valueChanges.pipe(takeUntilDestroyed()) .subscribe((value: PersonaType) => { // ... existing logic }); }apps/lfx-one/src/app/shared/services/persona.service.ts (1)
45-49: Replace console.warn with user-facing notification.Using
console.warnon Line 47 won't provide feedback to users attempting to change an auto-detected persona. Consider using a toast notification service (ToastModule is imported in AppComponent) to inform users why their action was blocked.Consider injecting a message/toast service:
import { MessageService } from 'primeng/api'; export class PersonaService { private readonly messageService = inject(MessageService); public setPersona(persona: PersonaType): void { if (this.isAutoDetected()) { this.messageService.add({ severity: 'info', summary: 'Persona Locked', detail: 'Your persona was automatically determined from committee membership and cannot be changed.', }); return; } // ... rest of method } }apps/lfx-one/src/server/utils/persona-helper.ts (1)
60-70: Add safeguard for priority array synchronization.The priority selection logic correctly uses
indexOfto compare persona priorities. However, if a new persona is added toCOMMITTEE_CATEGORY_TO_PERSONA(as encouraged by the comment on line 12) but not toPERSONA_PRIORITY,indexOfwill return-1, causing unexpected priority behavior.Consider adding a validation check or runtime assertion:
const PERSONA_PRIORITY: PersonaType[] = ['core-developer', 'maintainer', 'board-member']; +// Validate all mapped personas exist in priority array +Object.values(COMMITTEE_CATEGORY_TO_PERSONA).forEach(persona => { + if (!PERSONA_PRIORITY.includes(persona)) { + throw new Error(`Persona '${persona}' in category mapping but missing from PERSONA_PRIORITY`); + } +}); + /** * Fetches and determines user's persona based on committee membershipAlternatively, document the requirement clearly that both constants must be kept in sync.
packages/shared/src/constants/committees.constants.ts (3)
29-49: Confirm category value changes against backend data and persona detection logicYou’ve significantly reshaped
COMMITTEE_CATEGORIES(e.g., new values likeMaintainers,Committers,Government Advisory Council, marketing variants, SIG, TSC). BecausegetValidCommitteeCategories()(Line 142) exposes these values for validation, any persisted committee records or server-side checks that still use legacy category strings will now be treated as invalid/unknown.Given this PR also introduces persona auto-detection from committee membership, please double‑check that:
- Server‑side persona helper uses the exact same category strings for Board and Maintainers (ideally importing from this shared module, not hard‑coding), and
- Existing committee data won’t be orphaned by the new value set (or is being migrated in tandem).
Also applies to: 142-144
90-131: Good move making color map private; consider centralizing default/fallback usageMaking
COMMITTEE_TYPE_COLORSandDEFAULT_COMMITTEE_TYPE_COLORinternal is a nice encapsulation step; consumers now go throughgetCommitteeTypeColor()instead of hard‑coding badge classes.Given you already have a semantic mapping here, you might consider:
- Exposing a small typed helper (if needed later) rather than letting other modules reintroduce raw class strings, and
- Deriving
DEFAULT_COMMITTEE_TYPE_COLORfromCOMMITTEE_TYPE_COLORS.Otherto ensure they stay in lock‑step.Not blocking, just a small maintainability improvement.
Also applies to: 134-137
151-195: UnifygetCommitteeTypeColorwith semantic token map to avoid divergence
getCommitteeTypeColor’s partial‑match branches currently return raw Tailwind palette classes (bg-purple-100,bg-red-100,bg-blue-100, etc.), whileCOMMITTEE_TYPE_COLORSand the associated JSDoc talk about using LFX semantic tokens (bg-violet-100,bg-brand-100,bg-negative-100, etc.). This creates two conflicting sources of truth for the same conceptual categories (e.g., Board vs. governance vs. legal) and can drift over time.To keep things consistent and avoid maintaining two color systems, you could have the partial matches reuse the semantic map instead of hard‑coded palette strings, for example:
export function getCommitteeTypeColor(category: string | undefined): string { if (!category) return DEFAULT_COMMITTEE_TYPE_COLOR; const lowerCategory = category.toLowerCase(); // Board and governance - if (lowerCategory.includes('board')) return 'bg-purple-100 text-purple-800'; - if (lowerCategory.includes('government')) return 'bg-purple-100 text-purple-800'; + if (lowerCategory.includes('board')) return COMMITTEE_TYPE_COLORS['Board']; + if (lowerCategory.includes('government')) return COMMITTEE_TYPE_COLORS['Government Advisory Council']; // Legal and compliance - if (lowerCategory.includes('legal')) return 'bg-red-100 text-red-800'; - if (lowerCategory.includes('code of conduct')) return 'bg-red-100 text-red-800'; - if (lowerCategory.includes('product security')) return 'bg-red-100 text-red-800'; + if (lowerCategory.includes('legal')) return COMMITTEE_TYPE_COLORS['Legal Committee']; + if (lowerCategory.includes('code of conduct')) return COMMITTEE_TYPE_COLORS['Code of Conduct']; + if (lowerCategory.includes('product security')) return COMMITTEE_TYPE_COLORS['Product Security']; // Special interest groups - if (lowerCategory.includes('special interest')) return 'bg-blue-100 text-blue-800'; - if (lowerCategory.includes('expert')) return 'bg-blue-100 text-blue-800'; + if (lowerCategory.includes('special interest')) return COMMITTEE_TYPE_COLORS['Special Interest Group']; + if (lowerCategory.includes('expert')) return COMMITTEE_TYPE_COLORS['Expert Group']; // Working groups - if (lowerCategory.includes('working group')) return 'bg-orange-100 text-orange-800'; + if (lowerCategory.includes('working group')) return COMMITTEE_TYPE_COLORS['Working Group']; // Technical committees - if (lowerCategory.includes('technical steering')) return 'bg-green-100 text-green-800'; - if (lowerCategory.includes('technical oversight')) return 'bg-teal-100 text-teal-800'; - if (lowerCategory.includes('technical mailing')) return 'bg-teal-100 text-teal-800'; + if (lowerCategory.includes('technical steering')) return COMMITTEE_TYPE_COLORS['Technical Steering Committee']; + if (lowerCategory.includes('technical oversight')) return COMMITTEE_TYPE_COLORS['Technical Oversight Committee/Technical Advisory Committee']; + if (lowerCategory.includes('technical mailing')) return COMMITTEE_TYPE_COLORS['Technical Mailing List']; // Technical roles - if (lowerCategory.includes('maintainer')) return 'bg-blue-100 text-blue-800'; - if (lowerCategory.includes('committer')) return 'bg-blue-100 text-blue-800'; + if (lowerCategory.includes('maintainer')) return COMMITTEE_TYPE_COLORS['Maintainers']; + if (lowerCategory.includes('committer')) return COMMITTEE_TYPE_COLORS['Committers']; // Marketing and outreach - if (lowerCategory.includes('marketing oversight')) return 'bg-pink-100 text-pink-800'; - if (lowerCategory.includes('marketing committee')) return 'bg-pink-100 text-pink-800'; - if (lowerCategory.includes('marketing mailing')) return 'bg-pink-100 text-pink-800'; - if (lowerCategory.includes('ambassador')) return 'bg-pink-100 text-pink-800'; + if (lowerCategory.includes('marketing oversight')) return COMMITTEE_TYPE_COLORS['Marketing Oversight Committee/Marketing Advisory Committee']; + if (lowerCategory.includes('marketing committee')) return COMMITTEE_TYPE_COLORS['Marketing Committee/Sub Committee']; + if (lowerCategory.includes('marketing mailing')) return COMMITTEE_TYPE_COLORS['Marketing Mailing List']; + if (lowerCategory.includes('ambassador')) return COMMITTEE_TYPE_COLORS['Ambassador']; // Finance - if (lowerCategory.includes('finance')) return 'bg-emerald-100 text-emerald-800'; + if (lowerCategory.includes('finance')) return COMMITTEE_TYPE_COLORS['Finance Committee']; // Other/miscellaneous - if (lowerCategory.includes('other')) return 'bg-gray-100 text-gray-800'; + if (lowerCategory.includes('other')) return COMMITTEE_TYPE_COLORS['Other']; // Fallback to exact match or default return COMMITTEE_TYPE_COLORS[category as keyof typeof COMMITTEE_TYPE_COLORS] || DEFAULT_COMMITTEE_TYPE_COLOR; }That way:
- All branches use the same semantic tokens as the internal map,
- Future design‑token tweaks only need to be made in one place, and
- The JSDoc comment about semantic tokens matches actual behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/lfx-one/src/app/app.component.ts(3 hunks)apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts(2 hunks)apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts(1 hunks)apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html(1 hunks)apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.html(1 hunks)apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/persona.service.ts(1 hunks)apps/lfx-one/src/server/server.ts(3 hunks)apps/lfx-one/src/server/services/committee.service.ts(1 hunks)apps/lfx-one/src/server/utils/persona-helper.ts(1 hunks)packages/shared/src/constants/accounts.constants.ts(1 hunks)packages/shared/src/constants/committees.constants.ts(4 hunks)packages/shared/src/interfaces/auth.interface.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.htmlapps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts
🧬 Code graph analysis (6)
apps/lfx-one/src/server/utils/persona-helper.ts (2)
packages/shared/src/interfaces/persona.interface.ts (1)
PersonaType(8-8)apps/lfx-one/src/server/utils/auth-helper.ts (1)
getUsernameFromAuth(10-22)
packages/shared/src/interfaces/auth.interface.ts (1)
packages/shared/src/interfaces/persona.interface.ts (1)
PersonaType(8-8)
apps/lfx-one/src/app/shared/services/persona.service.ts (1)
packages/shared/src/interfaces/persona.interface.ts (1)
PersonaType(8-8)
apps/lfx-one/src/server/services/committee.service.ts (2)
packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(10-64)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (1)
packages/shared/src/constants/committees.constants.ts (1)
COMMITTEE_LABEL(16-19)
apps/lfx-one/src/server/server.ts (1)
apps/lfx-one/src/server/utils/persona-helper.ts (1)
fetchUserPersona(29-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (15)
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts (1)
70-73: canCreateGroup logic now matches the relaxed project-selection requirementSwitching
canCreateGroupto depend only onisMaintainer()aligns with the goal of not requiring a non-foundation project selection to enable group creation, whileopenCreateDialog()still safely guards onproject()?.uid. This looks correct and non-breaking from a runtime perspective.Please confirm this matches product expectations: maintainers can now see/use the “create group” affordance whenever they have maintainer persona, regardless of whether they’re on a foundation vs. specific project, with the uid guard handling any missing-context edge cases.
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html (2)
43-51: Verify the loading state refactoring is complete.The removal of the previous
@elseblock in favor of an unconditional carousel improves clarity. However, ensure that:
- No important fallback rendering (e.g., empty state when
primaryMetrics()is empty) was inadvertently dropped.- The carousel gracefully handles zero metrics (it will render an empty carousel, which may or may not be desired).
Please confirm the expected behavior when
primaryMetrics()returns an empty array or when data fails to load after the initial loading state.
53-114: Clarify intent of hover effect on membership tier cards only.The membership tier card variant (line 59) includes a hover effect (
hover:border-[#0094FF]), while the regular card variant (line 88) does not. This difference may be intentional if only membership tier cards are interactive; however, confirm this is not an oversight and that both card types should have consistent visual feedback.Also verify that the
metricproperties referenced in both variants—includingmetric.tier,metric.tierSince,metric.nextDuefor membership cards andmetric.chartData,metric.value,metric.subtitlefor regular cards—are properly typed and guaranteed to exist in your component's TypeScript file. Without seeing the component class, I cannot validate that all bindings align with the underlying data structure.packages/shared/src/constants/accounts.constants.ts (1)
23-23: LGTM!The new Linux Foundation account entry is properly formatted and consistent with existing entries.
apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.html (1)
4-8: LGTM!The conditional rendering correctly hides the persona selector when the persona is auto-detected, improving UX by preventing manual changes to auto-determined personas.
apps/lfx-one/src/app/app.component.ts (1)
11-11: LGTM!The PersonaService integration is well-implemented with proper SSR state hydration and initialization from the backend auth context.
Also applies to: 23-23, 51-51, 58-60
apps/lfx-one/src/server/server.ts (1)
29-29: LGTM!The persona fetching is properly integrated with non-blocking error handling and graceful fallback, ensuring SSR resilience. The try-catch pattern correctly prevents persona determination failures from breaking the page render.
Also applies to: 223-223, 249-261
apps/lfx-one/src/server/utils/persona-helper.ts (6)
1-8: LGTM: Imports are clean and properly scoped.All necessary dependencies are imported, and the license header is in place.
10-17: LGTM: Category-to-persona mapping is clear and extensible.The mapping correctly covers Board and Maintainers categories as specified in the PR objectives, with a helpful comment indicating where to add future mappings.
19-23: Clarify the 'core-developer' persona usage.The
PERSONA_PRIORITYarray includes'core-developer'and describes it as a "fallback," but no committee category inCOMMITTEE_CATEGORY_TO_PERSONAmaps to this persona. This meansfetchUserPersona()can never return'core-developer'.Verify whether:
- A committee category for core-developer should be added to the mapping
- The priority array comment and PR description are outdated
- Core-developer serves a different purpose outside this helper
25-41: LGTM: Function initialization is robust.The function properly retrieves authentication context, handles missing username gracefully with appropriate logging, and initializes necessary state for committee membership checks.
42-58: LGTM: Membership checks are thorough and well-logged.The loop correctly queries each configured committee category and accumulates matched personas. The logging provides valuable debugging information for SSR persona detection.
72-91: LGTM: Logging and error handling align with non-blocking SSR requirements.The function provides detailed logging for successful persona detection and gracefully handles errors by returning
nullwithout failing SSR, exactly as specified in the PR objectives.packages/shared/src/constants/committees.constants.ts (1)
59-71: Roles and voting statuses look consistent; keep enums in syncThe additions/adjustments in
MEMBER_ROLES(LF Staff,TAC/TOC Representative, explicitNone) andVOTING_STATUSES(Emeritus) look reasonable and keep the UI options aligned withCommitteeMemberRole/CommitteeMemberVotingStatus.Just make sure the backing enums and any API/domain validation on the server have been updated to recognize these new values so you don’t end up with “unknown role/status” cases at save time.
Also applies to: 81-87
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (1)
64-77: LGTM! Clean refactoring of filtering logic.The sequential filter approach using
let items = [...this.baseSidebarItems]followed by filter reassignments is more maintainable and composable than the previous implementation. This allows multiple filters to be applied cleanly.
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic persona detection based on committee membership to eliminate manual persona selection for committee members and enforce role-based access control. Users who are members of Board or Maintainers committees will have their personas automatically determined and locked, while others can manually select their persona.
Key Changes:
- Auto-detection system maps committee categories to personas (Board → board-member, Maintainers → maintainer) with priority-based selection
- Backend determines persona during SSR and transfers state to frontend via AuthContext
- UI hides persona selector when auto-detected and prevents manual changes
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/interfaces/auth.interface.ts |
Added optional persona field to AuthContext for SSR state transfer |
packages/shared/src/constants/committees.constants.ts |
Alphabetically sorted categories/roles/statuses and updated color mappings to semantic tokens |
packages/shared/src/constants/accounts.constants.ts |
Added The Linux Foundation account entry |
apps/lfx-one/src/server/utils/persona-helper.ts |
New utility implementing committee-based persona detection with priority system |
apps/lfx-one/src/server/services/committee.service.ts |
Added getCommitteeMembersByCategory() method for persona determination |
apps/lfx-one/src/server/server.ts |
Integrated persona detection into SSR authentication flow |
apps/lfx-one/src/app/shared/services/persona.service.ts |
Refactored to support auto-detected read-only personas |
apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.ts |
Added isAutoDetected signal exposure |
apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.html |
Conditionally hides selector when persona is auto-detected |
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html |
Restructured loading state display logic |
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts |
Removed project selection requirement from canCreateGroup permission check |
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts |
Added logout menu item to sidebar footer |
apps/lfx-one/src/app/app.component.ts |
Initialized persona from AuthContext during app bootstrap |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/lfx-one/src/app/shared/components/persona-selector/persona-selector.component.ts
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts
Outdated
Show resolved
Hide resolved
...dules/dashboards/components/organization-involvement/organization-involvement.component.html
Show resolved
Hide resolved
Add sidebar-profile feature flag to control profile menu accessibility and apply code cleanup improvements. Changes: - Add sidebar-profile feature flag (default: false) to disable profile clicking - Convert sidebarFooterItems to computed signal for reactive flag handling - Add Committers → core-developer persona mapping in persona-helper - Remove redundant try-catch in server.ts (fetchUserPersona handles errors internally) - Remove unused isNonFoundationProjectSelected signal from committee dashboard - Fix committee color inconsistency (use semantic tokens: violet, brand, positive, negative) - Update JSDoc to reflect actual valid categories (Board, Maintainers, Committers) - Add target and rel fields to SidebarMenuItem interface for explicit link control - Update AuthContext JSDoc to reflect all committee memberships (not just Board) Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts (1)
115-115: Inaccurate warning message.The warning message states "Please select a project first", but based on line 66,
this.project()can return either a selected project or a selected foundation. The message should reflect both options for accuracy.Apply this diff to correct the message:
- detail: 'Please select a project first', + detail: 'Please select a project or foundation first',
♻️ Duplicate comments (3)
apps/lfx-one/src/server/services/committee.service.ts (2)
383-383: Update JSDoc to reflect all valid categories.The JSDoc states "currently supports: 'Board', 'Maintainers'" but the
COMMITTEE_CATEGORY_TO_PERSONAmapping inapps/lfx-one/src/server/utils/persona-helper.ts(line 17) now includes'Committers'as well.Apply this diff to update the JSDoc:
- * @param category - Committee category to filter (currently supports: 'Board', 'Maintainers') + * @param category - Committee category to filter (e.g., 'Board', 'Maintainers', 'Committers')
405-405: Email case-insensitivity is correctly implemented.The email comparison on Line 405 properly uses
.toLowerCase()on both sides, addressing the case-sensitivity concern from previous reviews.apps/lfx-one/src/server/utils/persona-helper.ts (1)
14-24: Priority system is now complete.The past review comment noted that
'core-developer'was in the priority array without a corresponding category mapping. This has been resolved—theCommitterscategory mapping on line 17 now correctly maps to'core-developer'.
🧹 Nitpick comments (2)
apps/lfx-one/src/server/services/committee.service.ts (1)
385-406: Consider server-side filtering for performance.This method fetches all committee members for a category and filters client-side by username/email. For categories with many members, this could be inefficient (network payload, processing time).
If the query service supports filtering by username or email via query parameters, consider adding those filters to the initial request:
const params = { v: '1', type: 'committee_member', tags: `committee_category:${category}`, // Add if supported by query service: // username: username, // email: userEmail, };This would reduce network payload and improve response time.
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (1)
82-108: Logout implementation withtarget: '_self'navigates correctly.The logout menu item uses
url: '/logout'withtarget: '_self', which will navigate to the logout endpoint in the current window/tab (not a new tab as a previous review comment suggested). The sidebar template on line 79 respects the explicittargetproperty, so the navigation behavior is correct.However, for clarity and consistency with other actions, consider using
commandinstead:{ label: 'Logout', icon: 'fa-light fa-sign-out', command: () => { window.location.href = '/logout'; }, }This makes the navigation intent more explicit and avoids reliance on template attribute binding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.html(2 hunks)apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts(3 hunks)apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html(1 hunks)apps/lfx-one/src/server/server.ts(3 hunks)apps/lfx-one/src/server/services/committee.service.ts(1 hunks)apps/lfx-one/src/server/utils/persona-helper.ts(1 hunks)packages/shared/src/constants/committees.constants.ts(4 hunks)packages/shared/src/interfaces/auth.interface.ts(2 hunks)packages/shared/src/interfaces/components.interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/interfaces/auth.interface.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/committees/committee-dashboard/committee-dashboard.component.ts
🧬 Code graph analysis (4)
apps/lfx-one/src/server/server.ts (1)
apps/lfx-one/src/server/utils/persona-helper.ts (1)
fetchUserPersona(30-93)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (2)
packages/shared/src/constants/committees.constants.ts (1)
COMMITTEE_LABEL(16-19)apps/lfx-one/src/environments/environment.ts (1)
environment(7-21)
apps/lfx-one/src/server/utils/persona-helper.ts (2)
packages/shared/src/interfaces/persona.interface.ts (1)
PersonaType(8-8)apps/lfx-one/src/server/utils/auth-helper.ts (1)
getUsernameFromAuth(10-22)
apps/lfx-one/src/server/services/committee.service.ts (2)
packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(10-64)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)
🔇 Additional comments (10)
packages/shared/src/constants/committees.constants.ts (3)
29-49: LGTM! Committee categories expanded to support persona detection.The expanded category list correctly includes all the committee types needed for the persona auto-detection feature, including "Board" and "Maintainers" which are specifically used in the persona priority system mentioned in the PR objectives.
59-87: LGTM! Minor additions and reordering.The reordering of member roles and addition of the Emeritus voting status are straightforward and correct.
94-195: Excellent refactor that resolves the previous inconsistency issue.The color constants are now private with the function as the public API, and the color values are fully consistent between
COMMITTEE_TYPE_COLORSandgetCommitteeTypeColor(). The partial matching logic is correctly ordered from specific to general (e.g., 'marketing oversight' before 'marketing committee') to avoid false matches, and the fallback to exact matching provides good coverage for all categories.This properly addresses the past Copilot review comment about inconsistent colors between the constant and the function.
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.html (1)
7-7: LGTM!The binding changes correctly invoke
sidebarFooterItems()as a computed signal function, consistent with the refactoring in the TypeScript component.Also applies to: 26-26
packages/shared/src/interfaces/components.interface.ts (1)
276-279: LGTM!The addition of optional
targetandrelproperties enables proper handling of external links with explicit control over link behavior and security attributes.apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html (1)
79-80: LGTM!The external link attributes now correctly derive from
item.targetanditem.relwith secure defaults, properly decoupling link behavior from thedisabledstate.apps/lfx-one/src/server/utils/persona-helper.ts (1)
30-92: LGTM!The persona detection implementation is well-structured with:
- Proper error handling that doesn't block SSR
- Correct priority-based selection when users belong to multiple categories
- Safe handling of missing email (empty string fallback is guarded in the filter logic)
- Comprehensive logging for debugging
apps/lfx-one/src/server/server.ts (1)
223-223: LGTM!The persona initialization and assignment is clean and correct. The persona is initialized to
nulland then populated viafetchUserPersona, which handles errors internally without blocking SSR.Also applies to: 249-251
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (2)
39-39: LGTM!The
enableProfileClickfeature flag correctly controls the Profile menu item's disabled state, enabling gradual rollout of profile functionality.
66-79: LGTM!The refactored sidebar items computation correctly handles filtering based on feature flags and persona, using a mutable pattern that's clear and maintainable.
| this.isMaintainer = computed(() => this.personaService.currentPersona() === 'maintainer'); | ||
| this.isNonFoundationProjectSelected = computed(() => this.projectContextService.selectedProject() !== null); | ||
| this.canCreateGroup = computed(() => this.isMaintainer() && this.isNonFoundationProjectSelected()); | ||
| this.canCreateGroup = computed(() => this.isMaintainer()); |
There was a problem hiding this comment.
UX inconsistency: button enabled but action may fail.
The canCreateGroup signal only checks isMaintainer(), which enables the create button whenever the user is a maintainer. However, openCreateDialog() (lines 109-118) still requires a project or foundation to be selected, showing a warning if absent. This creates a frustrating pattern where a maintainer can see an enabled button but receives a warning upon clicking.
Consider one of these solutions:
- If the project-selection requirement should remain, restore the check to maintain consistent UX:
-this.canCreateGroup = computed(() => this.isMaintainer());
+this.canCreateGroup = computed(() => this.isMaintainer() && this.project() !== null);- If the PR objective to "remove project-selection requirement for group creation" means groups can be created without a project/foundation, then remove the check from
openCreateDialog()as well (lines 110-118).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.canCreateGroup = computed(() => this.isMaintainer()); | |
| this.canCreateGroup = computed(() => this.isMaintainer() && this.project() !== null); |
Summary
Implements automatic persona detection based on committee membership to streamline user experience and enforce role-based access control.
Core Changes
getCommitteeMembersByCategory()methodpersonafield for SSR state transferUI Updates
Technical Details
Benefits
Ticket: LFXV2-766
Generated with Claude Code