feat(profile): implement my profile redesign with overview tab#240
Merged
feat(profile): implement my profile redesign with overview tab#240
Conversation
- Add ProfileLayoutComponent with profile card and tab navigation - Add ProfileOverviewComponent with projects, identities, and skills - Add ProfileAffiliationsComponent for managing user affiliations - Add ManageSkillsComponent dialog for skill management - Add VerifyIdentitiesComponent dialog for identity verification - Add profile API endpoints for overview data (projects, identities, skills) - Add shared profile interfaces and constants - Update existing profile components to use new layout - Use toSignal with BehaviorSubject refresh pattern for reactive data LFXV2-1071 Signed-off-by: Asitha de Silva <asithade@gmail.com>
github-actions
Bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
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:
WalkthroughIntroduces a comprehensive profile management system with a new ProfileLayoutComponent shell, tab-based navigation, and child components for managing affiliations, identities, and work experience. Backend adds multiple new services (CDP, Auth0, ProfileAuth, EmailVerification, SocialVerification) supporting OAuth flows, identity verification, and work-experience/affiliation management. Routes are restructured with nested children under ProfileLayoutComponent, and new API endpoints are added for profile data operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ProfileLayout as ProfileLayoutComponent
participant FlowC as Flow C<br/>(Auth0 OAuth)
participant UserService as UserService
participant Backend as Backend<br/>(ProfileController)
participant Auth0 as Auth0 Management<br/>API
participant CDP as CDP Service
User->>ProfileLayout: Navigate to /profile<br/>(with success=profile_token_obtained)
ProfileLayout->>ProfileLayout: Read query params
ProfileLayout->>UserService: updateUserProfile(UserMetadata)
UserService->>Backend: POST /api/profile/metadata
Backend->>Auth0: Update user metadata
Auth0-->>Backend: Success
Backend-->>UserService: Success
UserService-->>ProfileLayout: Profile updated
ProfileLayout->>ProfileLayout: Show success toast<br/>Clear query params
ProfileLayout->>UserService: getCurrentUserProfile()
UserService->>Backend: GET /api/profile/current
Backend->>CDP: Fetch identities,<br/>work experience,<br/>affiliations
CDP-->>Backend: Combined data
Backend-->>UserService: ProfileHeaderData
UserService-->>ProfileLayout: Display profile header
sequenceDiagram
participant User as User
participant IdentityUI as ProfileIdentitiesComponent
participant Dialog as VerifyIdentityDialogComponent
participant EmailService as EmailVerificationService
participant NATS as NATS<br/>(auth-service)
participant Auth0 as Auth0 API
participant CDP as CDP Service
User->>IdentityUI: Click verify on unverified identity
IdentityUI->>Dialog: Open verify dialog
alt Email Provider
User->>Dialog: Enter email → Click send code
Dialog->>EmailService: sendEmailVerificationCode(email)
EmailService->>NATS: Send to<br/>lfx.auth-service.email_linking.send_verification
NATS-->>EmailService: OTP sent
User->>Dialog: Enter OTP
Dialog->>EmailService: verifyAndLinkEmail(email, otp)
EmailService->>NATS: Send to<br/>lfx.auth-service.email_linking.verify
NATS->>Auth0: Verify OTP & generate token
Auth0-->>NATS: Identity token
NATS-->>EmailService: Token + response
EmailService->>Auth0: Link identity to user
Auth0-->>EmailService: Success
EmailService-->>Dialog: Verified
else Social Provider
User->>Dialog: Click continue with provider
Dialog->>Dialog: Redirect to social OAuth flow
end
Dialog->>CDP: Create identity entry
CDP-->>Dialog: Identity verified & linked
Dialog-->>IdentityUI: Refresh identities
IdentityUI->>IdentityUI: Move identity to verified section
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive redesign of the user profile section, transitioning from a flat structure to a tabbed navigation system with a unified layout component. The changes introduce new profile views (Overview and Affiliations tabs) while refactoring existing profile pages to work within the new layout.
Changes:
- Added ProfileLayoutComponent as a shell with profile header card and tab navigation (desktop horizontal tabs, mobile dropdown)
- Implemented ProfileOverviewComponent displaying projects table, connected identities list, and skills management
- Created ProfileAffiliationsComponent for viewing organizational affiliations
- Added shared profile interfaces, types, and mock data constants
- Implemented backend API endpoints (currently returning mock data) for projects, identities, skills, and affiliations
- Refactored existing profile components (password, email, developer, manage) to remove individual navigation and work within the new layout
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/profile.interface.ts | Defines TypeScript interfaces for profile data structures including tabs, projects, identities, skills, and affiliations |
| packages/shared/src/constants/profile.constants.ts | Contains profile tab configuration and mock data for development/testing |
| packages/shared/src/interfaces/index.ts | Exports new profile interfaces |
| packages/shared/src/constants/index.ts | Exports new profile constants |
| apps/lfx-one/src/server/routes/profile.route.ts | Adds API routes for profile overview and affiliations data |
| apps/lfx-one/src/server/controllers/profile.controller.ts | Implements controller methods for new API endpoints with authentication checks |
| apps/lfx-one/src/app/shared/services/user.service.ts | Adds service methods for fetching and updating profile data |
| apps/lfx-one/src/app/modules/profile/profile.routes.ts | Restructures routing to use ProfileLayoutComponent as parent with child routes |
| apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.* | New layout shell component with profile header and tab navigation |
| apps/lfx-one/src/app/modules/profile/profile-overview/profile-overview.component.* | New overview tab showing projects, identities, and skills |
| apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.* | New affiliations tab for organizational affiliations |
| apps/lfx-one/src/app/modules/profile/components/manage-skills/manage-skills.component.* | Dialog component for managing user skills with autocomplete |
| apps/lfx-one/src/app/modules/profile/components/verify-identities/verify-identities.component.* | Dialog component for identity verification workflow |
| apps/lfx-one/src/app/modules/profile/password/profile-password.component.* | Removed profile-nav component, simplified layout |
| apps/lfx-one/src/app/modules/profile/email/profile-email.component.* | Removed profile-nav component, simplified layout |
| apps/lfx-one/src/app/modules/profile/developer/profile-developer.component.* | Removed profile-nav component, simplified layout |
| apps/lfx-one/src/app/modules/profile/manage-profile/profile-manage.component.* | Removed profile-nav component, simplified layout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| imports: [ButtonComponent, CardComponent, MessageComponent, ToastModule, ProfileNavComponent], | ||
| providers: [], | ||
| imports: [ButtonComponent, CardComponent, MessageComponent, ToastModule], | ||
| templateUrl: './profile-developer.component.html', |
There was a problem hiding this comment.
The ProfileDeveloperComponent is missing MessageService from its providers array (line 17) but injects and uses MessageService (line 22). This will cause a runtime error if MessageService is not provided at a higher level. Add providers: [MessageService] to the component decorator, as seen in other components like ProfilePasswordComponent and ProfileManageComponent.
Suggested change
| templateUrl: './profile-developer.component.html', | |
| templateUrl: './profile-developer.component.html', | |
| providers: [MessageService], |
| ToastModule, | ||
| TooltipModule, | ||
| ProfileNavComponent, | ||
| ], |
There was a problem hiding this comment.
The ProfileEmailComponent is missing providers for MessageService and ConfirmationService in its component decorator. These services are injected and used in the component (lines not shown in diff but referenced in imports), but without adding them to the providers array, they need to be provided at a higher level. For consistency with other profile components, add providers: [MessageService, ConfirmationService] to the component decorator.
Suggested change
| ], | |
| ], | |
| providers: [MessageService, ConfirmationService], |
Comment on lines
+85
to
+92
| public constructor() { | ||
| // Subscribe to tab selection changes for mobile navigation | ||
| this.tabForm.controls.selectedTab.valueChanges.pipe(takeUntilDestroyed()).subscribe((route) => { | ||
| if (route) { | ||
| this.router.navigate(['/profile', route]); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The mobile tab dropdown does not synchronize with the current route on initial load or when navigating via browser back/forward buttons. The form control selectedTab is initialized with 'overview' but it doesn't update when the route changes. You should subscribe to router events to update the dropdown value when the route changes, similar to how other components in the codebase handle route synchronization (see dev-toolbar.component.ts and main-layout.component.ts).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/lfx-one/src/app/modules/profile/developer/profile-developer.component.html (1)
40-42:⚠️ Potential issue | 🟡 MinorConflicting Tailwind CSS classes on token display element.
truncatesetswhite-space: nowrap, which conflicts withwhitespace-pre-wrapthat setswhite-space: pre-wrap. These cannot both apply. Since the token display should truncate when masked but potentially wrap when visible, consider conditional classes or remove the conflicting ones.🐛 Proposed fix
- <div class="text-sm font-mono text-gray-600 mt-1 truncate break-all whitespace-pre-wrap" data-testid="api-token-display"> + <div class="text-sm font-mono text-gray-600 mt-1 break-all" data-testid="api-token-display">If truncation is needed for long tokens, use
truncatealone withoutbreak-allorwhitespace-pre-wrap.apps/lfx-one/src/app/modules/profile/password/profile-password.component.html (1)
106-109:⚠️ Potential issue | 🟡 MinorFix requirements count mismatch in the header. The text says “3 of 4 criteria,” but five criteria are listed; update to match the actual rule to avoid user confusion.
🔧 Suggested copy fix
- <p class="text-xs font-medium text-gray-700 mb-2">Requirements (must meet at least 3 of 4 criteria):</p> + <p class="text-xs font-medium text-gray-700 mb-2">Requirements (must meet at least 3 of 5 criteria):</p>
🤖 Fix all issues with AI agents
In `@apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts`:
- Around line 95-108: initProfileData currently sets loading as a side effect
inside mapToHeaderData and on error catchError(of(null)) leaves loading true;
move loading control out of mapToHeaderData and ensure it is cleared whether the
profile loads or errors by setting loading = true before the request and using
finalize (or tap + finalize) in the pipe that wraps the getCurrentUserProfile()
observable so loading is set to false on completion/error; update
initProfileData to wrap this.userService.getCurrentUserProfile() with tap(() =>
{/* maybe no side-effects here */})/finalize(() => this.loading = false) and
remove any loading side-effects from mapToHeaderData so null from catchError
still results in loading=false.
In
`@apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.ts`:
- Around line 36-43: The comparator in computed property sortedAffiliations only
compares verified flags and returns 0 when equal, so it never sorts by start
date; update the comparator in sortedAffiliations (which uses
this.affiliations()) to, after the verified check, compare the affiliation start
date fields (e.g., a.startDate / a.start_date) and return -1/1 based on earlier
vs later dates (and handle missing/null dates consistently), so verified items
stay first and ties are ordered by start date.
In
`@apps/lfx-one/src/app/modules/profile/components/verify-identities/verify-identities.component.html`:
- Around line 23-25: The icon element currently uses [class]="identity.icon",
which overwrites the static utility classes; update the <i> element in
verify-identities.component.html to use [ngClass] so the dynamic classes from
identity.icon are merged with the existing static classes (w-5 h-5
text-slate-600 text-lg) instead of replacing them — locate the <i> tag that
references identity.icon and switch the binding from [class] to [ngClass],
preserving the static class attribute.
In
`@apps/lfx-one/src/app/modules/profile/components/verify-identities/verify-identities.component.ts`:
- Around line 57-59: The requiresAuth method currently treats only 'github' and
'linkedin' as OAuth providers, causing mismatches with getProviderLabel which
supports 'gitlab' and 'google'; update requiresAuth (in
verify-identities.component.ts) to include 'gitlab' and 'google' (or better:
implement/consume a centralized OAuth provider list or provider metadata check
such as an isOAuthProvider helper) so the flow matches getProviderLabel and any
future providers; ensure the method references the same source of truth (e.g., a
PROVIDERS_OAUTH_SET or ProviderMetadata.isOAuth(provider)) to avoid divergence.
In
`@apps/lfx-one/src/app/modules/profile/profile-overview/profile-overview.component.html`:
- Around line 96-99: The template currently uses [class]="identity.icon" which
overwrites the static utility classes on the <i> element; change the binding to
use [ngClass] so the static classes are preserved and the dynamic icon class is
added (e.g., use [ngClass] combining identity.icon with
'w-5','h-5','text-slate-600','text-center'). Update the <i> element where
identity.icon is bound inside the identities() loop (the element with
[class]="identity.icon") to use [ngClass] instead.
In `@apps/lfx-one/src/app/shared/services/user.service.ts`:
- Around line 191-193: The updateOverviewSkills method currently lacks error
handling for the HTTP PUT to '/api/profile/overview/skills'; add a catchError
operator to the pipe on updateOverviewSkills (the method that returns
Observable<{ skills: string[]; message: string }>) so failures are handled
consistently with the other service methods—either map errors into a
user-friendly Observable response like { skills: originalSkills, message:
'Failed to update skills' } or rewrap the error with a clear message via
throwError—ensuring you still call take(1) and that the returned Observable
always matches the declared shape.
In `@apps/lfx-one/src/server/controllers/profile.controller.ts`:
- Around line 668-835: The catch blocks in getOverviewProjects,
getOverviewIdentities, getOverviewSkills, updateOverviewSkills, and
getAffiliations do not call logger.error before forwarding errors; add a
logger.error call in each catch that uses the same operation key and startTime
(e.g. logger.error(req, 'get_overview_projects', startTime, { err: error }))
immediately before next(error) so the startOperation/finish logging contract is
preserved.
🧹 Nitpick comments (10)
apps/lfx-one/src/app/modules/profile/developer/profile-developer.component.ts (1)
31-32: Consider removing redundant computed signal.
isLoadingjust wrapsloading()without additional computation. The template could referenceloading()directly.♻️ Proposed simplification
- // Loading state computed from tokenInfo - public readonly isLoading = computed(() => this.loading());Then update the template to use
loading()instead ofisLoading().apps/lfx-one/src/app/modules/profile/developer/profile-developer.component.html (1)
48-56: Button label doesn't reflect visibility state.The label is always "Show" even when the token is already visible. Consider toggling the label based on
maskToken()state for better UX consistency with the icon behavior.♻️ Proposed improvement
<lfx-button type="button" size="small" [icon]="maskToken() ? 'fa-light fa-eye' : 'fa-light fa-eye-slash'" severity="secondary" - label="Show" + [label]="maskToken() ? 'Show' : 'Hide'" (onClick)="toggleTokenVisibility()" [attr.data-testid]="'token-visibility-toggle'"> </lfx-button>apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.ts (1)
64-65: Use a typed dialog result interface (andVerificationChoices).The inline
{ ... } | nullwithRecord<string, string>loses type safety and conflicts with the sharedVerificationChoicestype. Consider introducing an interface and using the shared type.♻️ Suggested refactor
+interface VerifyIdentitiesResult { + confirmedIds: string[]; + choices: VerificationChoices; +} ... - dialogRef.onClose.pipe(take(1)).subscribe((result: { confirmedIds: string[]; choices: Record<string, string> } | null) => { + dialogRef.onClose.pipe(take(1)).subscribe((result: VerifyIdentitiesResult | null) => { if (result?.confirmedIds?.length) { ... } });As per coding guidelines "Use TypeScript interfaces instead of union types for better maintainability".
apps/lfx-one/src/app/modules/profile/components/manage-skills/manage-skills.component.ts (1)
45-51: Move complex computed signals into private initializer helpers.
filteredTaxonomyandsortedSelectedSkillsare multi-step computed signals; per guidelines these should be initialized via private helper functions.♻️ Suggested refactor
- public readonly filteredTaxonomy = computed(() => { - const search = this.skillSearch().toLowerCase().trim(); - if (!search) return []; - return SKILL_TAXONOMY.filter((s) => s.toLowerCase().includes(search) && !this.selectedSkills().includes(s)).slice(0, 10); - }); - - public readonly sortedSelectedSkills = computed(() => [...this.selectedSkills()].sort((a, b) => a.localeCompare(b))); + public readonly filteredTaxonomy = this.initFilteredTaxonomy(); + public readonly sortedSelectedSkills = this.initSortedSelectedSkills(); ... + private initFilteredTaxonomy(): Signal<string[]> { + return computed(() => { + const search = this.skillSearch().toLowerCase().trim(); + if (!search) return []; + return SKILL_TAXONOMY.filter((s) => s.toLowerCase().includes(search) && !this.selectedSkills().includes(s)).slice(0, 10); + }); + } + + private initSortedSelectedSkills(): Signal<string[]> { + return computed(() => [...this.selectedSkills()].sort((a, b) => a.localeCompare(b))); + }As per coding guidelines "Use private initializer functions for complex Computed and toSignal conversions in Angular components".
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts (1)
85-92: Tab selection triggers navigation on every value change, including initialization.Subscribing to
valueChangeswill fire whenpatchValueor form reset is called, which could cause unintended navigation. Consider usingdistinctUntilChanged()or skipping initial emissions if the form value is set programmatically (e.g., syncing with the current route).♻️ Suggested improvement
public constructor() { // Subscribe to tab selection changes for mobile navigation - this.tabForm.controls.selectedTab.valueChanges.pipe(takeUntilDestroyed()).subscribe((route) => { + this.tabForm.controls.selectedTab.valueChanges.pipe( + distinctUntilChanged(), + takeUntilDestroyed() + ).subscribe((route) => { if (route) { this.router.navigate(['/profile', route]); } }); }Add
distinctUntilChangedto imports fromrxjs.apps/lfx-one/src/app/shared/services/user.service.ts (1)
155-161: Use logger service instead ofconsole.errorfor error logging.The coding guidelines specify using the logger service with the
errfield for proper error serialization. This pattern is repeated across all new methods.♻️ Proposed fix (apply similar pattern to other methods)
+import { LoggerService } from '@services/logger.service'; + export class UserService { private readonly http = inject(HttpClient); + private readonly logger = inject(LoggerService); // ... existing code ... public getOverviewProjects(): Observable<ProfileProject[]> { return this.http.get<ProfileProject[]>('/api/profile/overview/projects').pipe( catchError((error) => { - console.error('Failed to load overview projects:', error); + this.logger.warning('Failed to load overview projects', { err: error }); return of([]); }) ); }As per coding guidelines: "Use logger.warning() for recoverable errors when returning null or empty arrays from service methods" and "Always use err field for proper error serialization in logger calls, never use error field."
packages/shared/src/constants/profile.constants.ts (1)
40-77: External logo URLs in mock data may break or be unreliable.The mock data references external URLs like
landscape.cncf.io,google.com/favicon.ico, etc. These could break, be blocked by CORS, or change. Consider using local placeholder images or data URIs for development/testing stability.apps/lfx-one/src/app/modules/profile/profile-overview/profile-overview.component.ts (3)
89-96: Redundanttake(1)operator.
updateOverviewSkillsinUserServicealready appliestake(1). The additionaltake(1)here is redundant.♻️ Simplified version
private saveSkills(skills: string[]): void { this.userService .updateOverviewSkills(skills) - .pipe(take(1)) .subscribe(() => { this.refreshSkills$.next(); }); }
80-86: TODO: Identity verification API integration is pending.The modal closes without persisting verification changes. This appears intentional for the initial implementation, but the TODO should be tracked.
Would you like me to open an issue to track the identity verification API integration, or help implement the API call?
48-63: Dialog subscription should handle errors.If
ManageSkillsComponentthrows during initialization or ifsaveSkillsfails, there's no error handling. Consider adding error handling for robustness.🛡️ Add error handling
dialogRef.onClose.pipe(take(1)).subscribe((updatedSkills: string[] | null) => { if (updatedSkills) { - this.saveSkills(updatedSkills); + this.userService + .updateOverviewSkills(updatedSkills) + .subscribe({ + next: () => this.refreshSkills$.next(), + error: (err) => { + // Handle error (e.g., show toast notification) + console.error('Failed to save skills:', err); + } + }); } });
LFXV2-1071 - Fix CLAUDE.md monorepo structure (file names, counts, missing dirs, server structure) - Rewrite backend README.md with actual 13 controllers, 18 services, 13 routes - Rewrite shared package-architecture.md (30 interfaces, 30 constants, 12 utils) - Fix frontend route architecture (flat MainLayoutComponent, not project/:slug) - Update zoneless API from experimental to stable across 7 docs - Fix SSR config API (provideServerRendering with withRoutes) - Fix styling docs (tailwindcss-primeui plugin, Inter font, shared pkg sources) - Update E2E testing docs to reflect actual infrastructure state - Remove references to non-existent files (logger.ts, responder.ts, meeting-templates/) Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/architecture/backend/README.md`:
- Around line 207-208: The README contains a stray/incomplete logger.debug
statement ("logger.debug(req, 'get_committees', 'Fetching committees from
microservice', { queryParams });") that breaks formatting; remove or relocate it
into the appropriate fenced code block or complete the example so it is a valid
code sample (ensure the logger.debug line is inside the code block demonstrating
get_committees usage and matches the surrounding example context).
In `@docs/architecture/frontend/styling-system.md`:
- Around line 165-168: Remove the blank line inside the blockquote in
docs/architecture/frontend/styling-system.md so the two consecutive "> **Note**"
lines form a single contiguous blockquote; edit the block containing the
Tailwind/ESM note and the font-note (the two "> **Note**" paragraphs) to be
adjacent with no empty line between them to satisfy markdownlint MD028.
In `@docs/architecture/shared/package-architecture.md`:
- Around line 121-170: The export map example is inconsistent with the import
examples because the './utils' subpath is missing; update the "exports" object
to include a "./utils" entry (with "types", "import", "require", and "default"
pointing to the built ./dist/utils files) so the documented import pattern
import { formatDate } from '@lfx-one/shared/utils' is valid, or alternatively
remove the '@lfx-one/shared/utils' import line from the examples if utils is not
exported; edit the exports block and the import examples accordingly to keep
them in sync.
In `@docs/index.md`:
- Line 4: The docs incorrectly call Angular 19's zoneless change detection
"stable"; update any occurrences that mention "stable zoneless change detection"
to "experimental zoneless change detection" and where code or prose references
the newer stable API, replace or clarify to use
provideExperimentalZonelessChangeDetection() (the experimental API name) instead
of provideZonelessChangeDetection(), ensuring all mentions (including the intro
sentence and later examples) reflect that the API is experimental and may change
in patch releases.
🧹 Nitpick comments (3)
docs/architecture/testing/e2e-testing.md (2)
245-263: Clarify TEST_CREDENTIALS source.Line 256 references
TEST_CREDENTIALSwithout showing its definition or import. Consider adding a comment or import statement to clarify where these credentials come from.📝 Suggested clarification
// e2e/helpers/global-setup.ts +import { TEST_CREDENTIALS } from './test-config'; + async function globalSetup(config: FullConfig) { const browser = await chromium.launch();Or add a comment:
async function globalSetup(config: FullConfig) { const browser = await chromium.launch(); const context = await browser.newContext(); const page = await context.newPage(); try { // Navigate to logout to trigger authentication flow await page.goto(`${url}/logout`); + // TEST_CREDENTIALS should be defined in your test configuration // Perform authentication await AuthHelper.loginWithAuth0(page, TEST_CREDENTIALS);
380-380: Avoid hardcoding test counts in documentation.The specific count "85/85 tests passing" will become stale as the test suite evolves. Consider using a more general statement or adding a timestamp to indicate this is a snapshot.
📝 Suggested alternatives
-#### Current Status: ✅ 85/85 tests passing +#### Current Status: ✅ All tests passing (as of Feb 2026)Or:
-#### Current Status: ✅ 85/85 tests passing +#### Test Health Indicatorsdocs/architecture/backend/README.md (1)
223-294: Consider using dynamic or representative examples instead of exhaustive file listings for better long-term maintainability.The directory structure documentation enumerates every controller, service, route, and utility file with specific counts (13 controllers, 18 services, 13 routes). While currently accurate, this creates a maintenance burden as the codebase evolves—these listings will require manual updates whenever files are added or removed.
Consider alternative approaches:
- Use representative examples instead of exhaustive listings
- Add "including but not limited to" language for flexibility
- Remove specific counts that require manual updates
- Link to automated documentation tools
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
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)
docs/architecture/backend/ssr-server.md (1)
53-62:⚠️ Potential issue | 🟠 MajorInconsistent authentication strategy documentation.
The "Authentication Architecture" section (lines 53-62) states that "all routes benefit from consistent security policies" and describes a session-based approach applied at the Express middleware level. However, the new "Selective Authentication" section (lines 77-102) introduces
authRequired: falsewith "selective protection" via custom middleware, which means routes are not authenticated by default.These statements contradict each other. If the authentication strategy has changed to selective authentication, the earlier section should be updated to reflect this change, clarifying that authentication is now opt-in rather than global.
📝 Suggested documentation restructure
Consider updating lines 53-62 to align with the selective authentication approach:
### Authentication Architecture -Authentication follows a **session-based approach** using Auth0 with express-openid-connect middleware. This design choice provides: +Authentication follows a **selective session-based approach** using Auth0 with express-openid-connect middleware. This design choice provides: - **Seamless user experience** through secure session management with encrypted cookies - **Automatic token refresh** to maintain user sessions without interruption - **Flexible scope management** supporting both UI access and API authentication - **Standards compliance** following OAuth 2.0 and OpenID Connect specifications -The authentication layer operates at the Express middleware level, ensuring all routes benefit from consistent security policies. +The authentication layer operates at the Express middleware level with **selective route protection**, allowing public routes while securing protected resources through custom middleware.Also applies to: 77-102
🤖 Fix all issues with AI agents
In `@docs/architecture/backend/README.md`:
- Around line 223-294: The services directory listing in README.md omits
profile.service.ts causing the tree to show 17 services while the text says 18;
update the services section by adding an entry "├── profile.service.ts #
Profile business logic" (or similar description consistent with other entries)
into the services list so the visual tree and count match the actual codebase
and the controllers/services summary remains consistent.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-240.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Adds profile overview, identities, work experience, affiliations, and attribution tabs. Includes identity verification flows (email, social), work experience CRUD with affiliation timeline, CDP integration, and M2M token caching with 5-minute expiry buffer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (1)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts (1)
199-217:⚠️ Potential issue | 🟠 MajorLoading state remains
trueon error.When
catchErrorreturnsof(null),mapToHeaderDatais never called, soloadingstaystrueindefinitely. This was flagged in a previous review.🔧 Proposed fix
private initProfileData(): Signal<ProfileHeaderData | null> { const user$ = toObservable(this.userService.user); return toSignal( this.refreshProfile$.pipe( switchMap(() => user$.pipe( filter((user) => user !== null), switchMap(() => this.userService.getCurrentUserProfile().pipe( - map((profile: CombinedProfile) => this.mapToHeaderData(profile)), - catchError(() => of(null)) + map((profile: CombinedProfile) => { + this.loading.set(false); + return this.mapToHeaderData(profile); + }), + catchError(() => { + this.loading.set(false); + return of(null); + }) ) ) ) ) ), { initialValue: null } ); } private mapToHeaderData(profile: CombinedProfile): ProfileHeaderData { - this.loading.set(false); this.combinedProfile = profile;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts` around lines 199 - 217, initProfileData can leave the component stuck in loading=true when getCurrentUserProfile errors because catchError returns of(null) before mapToHeaderData runs; fix by ensuring loading is cleared in the observable chain (e.g. add a finalize operator or set loading=false inside catchError) so loading is turned off on both success and error. Update the pipeline in initProfileData (the chain that calls userService.getCurrentUserProfile and mapToHeaderData) to call finalize(() => this.loading.set(false)) or, if using catchError to return of(null), also call this.loading.set(false) inside the catchError handler so loading is always cleared; reference refreshProfile$, initProfileData, mapToHeaderData, and userService.getCurrentUserProfile when making the change.
🧹 Nitpick comments (23)
packages/shared/src/utils/date-time.utils.ts (1)
333-350: Consider extracting the duplicated months array.The
monthsarray is defined identically in bothisoDateToMonthYear(line 335) andabbreviatedMonthYearToIsoDate(line 345). Consider extracting it to a module-level constant for DRY consistency.♻️ Proposed refactor
+const MONTH_ABBREVIATIONS = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']; + /** * Converts an ISO date string to abbreviated month-year format (e.g., "Mar 2023") * `@param` isoDate ISO date string (e.g., "2023-03-15T00:00:00Z") * `@returns` Formatted string like "Mar 2023" */ export function isoDateToMonthYear(isoDate: string): string { const date = new Date(isoDate); - const months = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']; - return `${months[date.getUTCMonth()]} ${date.getUTCFullYear()}`; + return `${MONTH_ABBREVIATIONS[date.getUTCMonth()]} ${date.getUTCFullYear()}`; } /** * Converts an abbreviated month-year string (e.g., "Mar 2023") to ISO date (YYYY-MM-01) * `@param` monthYear Abbreviated month-year string like "Mar 2023" * `@returns` ISO date string like "2023-03-01", or the original string if parsing fails */ export function abbreviatedMonthYearToIsoDate(monthYear: string): string { - const months = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']; const [monthAbbr, year] = monthYear.split(' '); - const monthIndex = months.indexOf(monthAbbr); + const monthIndex = MONTH_ABBREVIATIONS.indexOf(monthAbbr); if (monthIndex === -1) return monthYear; return `${year}-${String(monthIndex + 1).padStart(2, '0')}-01`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/utils/date-time.utils.ts` around lines 333 - 350, Both isoDateToMonthYear and abbreviatedMonthYearToIsoDate declare the same months array; extract that array into a single module-level constant (e.g., const MONTHS = [...]) and update both functions to reference MONTHS instead of redeclaring it; ensure the constant is exported or kept local to the module as appropriate and run tests to verify behavior of isoDateToMonthYear and abbreviatedMonthYearToIsoDate remains unchanged.apps/lfx-one/src/server/services/email-verification.service.ts (1)
350-355: Consider reducing log verbosity for identity list responses.Logging the full
raw_responseandparsed_dataatinfolevel could generate verbose logs in production, especially for users with many identities. Consider usingdebuglevel instead, consistent with other methods in this service.♻️ Suggested change
- logger.info(req, 'list_identities', 'Raw NATS USER_IDENTITY_LIST response', { + logger.debug(req, 'list_identities', 'Raw NATS USER_IDENTITY_LIST response', { raw_response: responseText, parsed_success: parsed.success, parsed_data: parsed.data, parsed_error: parsed.error, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/services/email-verification.service.ts` around lines 350 - 355, The current logger.info call in the list_identities flow logs potentially large fields (raw_response and parsed_data) at info level; change this to use logger.debug for the detailed payload (raw_response and parsed_data) while keeping a concise info-level log if needed (e.g., logger.info(req, 'list_identities', 'NATS USER_IDENTITY_LIST response', { parsed_success: parsed.success, parsed_error: parsed.error })), and ensure the original logger.info(...) invocation referencing req, 'list_identities', raw_response, parsed_data, parsed_error is adjusted accordingly so verbose data is emitted only at debug level.apps/lfx-one/src/server/server.ts (2)
202-205: Consider simplifying route handler bindings.The inline arrow functions can be replaced with bound method references for cleaner code.
♻️ Simplified handler binding
const profileCallbackController = new ProfileController(); -app.get('/passwordless/callback', (req, res) => profileCallbackController.handleProfileAuthCallback(req, res)); +app.get('/passwordless/callback', profileCallbackController.handleProfileAuthCallback.bind(profileCallbackController)); -app.get('/social/callback', (req, res) => profileCallbackController.handleSocialCallback(req, res)); +app.get('/social/callback', profileCallbackController.handleSocialCallback.bind(profileCallbackController));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/server.ts` around lines 202 - 205, Replace the inline arrow functions used in the app.get routes with direct bound method references from profileCallbackController: instead of wrapping handleProfileAuthCallback and handleSocialCallback in arrow handlers, register them directly and ensure they are bound to profileCallbackController (e.g., use the controller method reference bound to the controller instance) so the methods receive the correct this context for both the '/passwordless/callback' and '/social/callback' routes.
200-205: Consider adding rate limiting to OAuth callback endpoints for defense-in-depth and DoS prevention.While these endpoints are already protected by authentication middleware (requiring valid session and CSRF state validation), adding rate limiting provides an additional layer of defense against potential denial-of-service attempts and follows security best practices for OAuth flows.
Proposed implementation using express-rate-limit
+import rateLimit from 'express-rate-limit'; + +// Rate limiter for OAuth callbacks - prevents DoS +const oauthCallbackLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 10, // limit each IP to 10 requests per windowMs + message: 'Too many callback attempts, please try again later', + standardHeaders: true, + legacyHeaders: false, +}); + // Flow C: Profile auth callback at /passwordless/callback (registered in Auth0 Profile Client) const profileCallbackController = new ProfileController(); -app.get('/passwordless/callback', (req, res) => profileCallbackController.handleProfileAuthCallback(req, res)); +app.get('/passwordless/callback', oauthCallbackLimiter, (req, res) => profileCallbackController.handleProfileAuthCallback(req, res)); // Social identity verification callback (GitHub/LinkedIn OAuth redirect) -app.get('/social/callback', (req, res) => profileCallbackController.handleSocialCallback(req, res)); +app.get('/social/callback', oauthCallbackLimiter, (req, res) => profileCallbackController.handleSocialCallback(req, res));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/server.ts` around lines 200 - 205, Add a rate-limiting middleware (e.g., express-rate-limit) and apply it to the OAuth callback routes to mitigate DoS: create a limiter with sensible defaults (short window, low max requests, standard headers, and a custom handler) and attach it to the routes that use ProfileController (the GET '/passwordless/callback' route that calls ProfileController.handleProfileAuthCallback and the GET '/social/callback' route that calls ProfileController.handleSocialCallback); ensure the limiter is applied before the controller handlers and coexists with existing auth/CSRF middleware so legitimate auth flows are unaffected.apps/lfx-one/src/app/modules/profile/attribution/profile-attribution.component.html (1)
26-26: Fixed width may conflict with grid layout on certain screen sizes.The right column has a fixed
w-[360px]width, but the parent grid useslg:grid-cols-[3fr_1fr]which expects columns to share available space. This combination may cause layout inconsistencies or horizontal overflow on screens where 360px exceeds the1frallocation.♻️ Consider using max-width or removing the fixed width
<!-- Right Column: Project Involvement --> - <div class="flex w-[360px] flex-col gap-3"> + <div class="flex w-full max-w-[360px] flex-col gap-3 lg:w-auto">Alternatively, if a fixed width is required, consider adjusting the grid definition:
- <div class="grid grid-cols-1 gap-8 lg:grid-cols-[3fr_1fr]"> + <div class="grid grid-cols-1 gap-8 lg:grid-cols-[1fr_360px]"> <!-- Left Column: Work history --> ... <!-- Right Column: Project Involvement --> - <div class="flex w-[360px] flex-col gap-3"> + <div class="flex flex-col gap-3">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/attribution/profile-attribution.component.html` at line 26, The right-column div uses a fixed width class "w-[360px]" which can conflict with the parent grid definition "lg:grid-cols-[3fr_1fr]" and cause overflow; update the div (the element with class "flex w-[360px] flex-col gap-3") to use a responsive constraint such as "max-w-[360px]" or "w-full max-w-[360px]" (or remove the fixed width entirely) so it respects the grid's fractional column sizing on different screen sizes; if a strict fixed width is required, instead adjust the parent grid from "lg:grid-cols-[3fr_1fr]" to an explicit column track that accommodates the fixed pixel width.apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.html (1)
65-72: Consider extracting the resolved org logo block to reduce duplication.The same logo rendering logic (lines 7-11 and 66-70) is duplicated between search mode and manual mode. This could be extracted to a reusable template or component.
♻️ Suggested refactor using ng-template
+<ng-template `#resolvedLogo`> + `@if` (resolvedOrg(); as resolved) { + `@if` (resolved.logo) { + <img [src]="resolved.logo" [alt]="resolved.name" class="h-8 w-8 shrink-0 rounded" data-testid="org-search-resolved-logo" /> + } + } +</ng-template> + <div class="organization-search-container"> `@if` (!manualMode()) { <div class="flex items-center gap-2"> - `@if` (resolvedOrg(); as resolved) { - `@if` (resolved.logo) { - <img [src]="resolved.logo" [alt]="resolved.name" class="h-8 w-8 shrink-0 rounded" data-testid="org-search-resolved-logo" /> - } - } + <ng-container *ngTemplateOutlet="resolvedLogo"></ng-container>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.html` around lines 65 - 72, Duplicate rendering of the resolved organization logo should be extracted into a reusable piece: create an ng-template or small presentational component for the logo block (the conditional that checks resolvedOrg() / resolved.logo and renders the <img> with src bound to resolved.logo, alt to resolved.name and attributes like class and data-testid) and replace both inline instances with that template/component; update references where used alongside lfx-input-text and nameControl() to render the shared logo block to remove duplication and keep behavior identical.apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.html (1)
130-130: Same performance concern:getMenuItems(identity)called in template.Similar to the work experience table, this method is called in the template which may trigger on every change detection cycle. Consider memoizing or pre-computing menu items.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.html` at line 130, The template calls getMenuItems(identity) for each row which can run on every change detection cycle; modify the ProfileIdentitiesComponent to precompute and cache menu items instead of calling getMenuItems from the template: add a Map or a cached property (e.g., menuItemsMap) keyed by identity id or index, compute menu items in a lifecycle hook or whenever identities change (e.g., in ngOnChanges or where identities are loaded) and expose a plain property or getter like menuItemsMap.get(identity.id) to the template, then change the template call to pass the cached menu items to <lfx-menu> instead of calling getMenuItems(identity). Ensure the caching logic updates entries when an identity changes or is removed.apps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.html (1)
97-113: Consider consolidating the duplicate submit button blocks.The add and edit submit buttons differ only by label. This can be simplified.
♻️ Simplified submit button
- `@if` (data.mode === 'add') { - <lfx-button - label="Add experience" - severity="primary" - size="small" - [disabled]="submitting() || orgSearch?.resolvingOrg()" - (onClick)="onSubmit()" - data-testid="we-form-submit" /> - } `@else` { - <lfx-button - label="Save changes" - severity="primary" - size="small" - [disabled]="submitting() || orgSearch?.resolvingOrg()" - (onClick)="onSubmit()" - data-testid="we-form-submit" /> - } + <lfx-button + [label]="data.mode === 'add' ? 'Add experience' : 'Save changes'" + severity="primary" + size="small" + [disabled]="submitting() || orgSearch?.resolvingOrg()" + (onClick)="onSubmit()" + data-testid="we-form-submit" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.html` around lines 97 - 113, The two duplicate lfx-button blocks for add vs edit can be consolidated: replace the conditional blocks that check data.mode === 'add' and the else with a single lfx-button using a label expression (e.g., data.mode === 'add' ? 'Add experience' : 'Save changes'), keeping the same attributes ([disabled]="submitting() || orgSearch?.resolvingOrg()", (onClick)="onSubmit()", severity, size, and data-testid="we-form-submit") so only the label varies while onSubmit(), submitting(), and orgSearch?.resolvingOrg() remain unchanged.apps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.html (1)
62-77: Resend button mixes[disabled]attribute with class-based disabled styling.The button uses both
[disabled]="resendCooldown() > 0"and conditional classes forcursor-not-allowed. While functional, the nativedisabledattribute on a<button>is sufficient and more accessible.♻️ Simplified disabled handling
<button - class="text-sm font-medium" - [class.cursor-pointer]="resendCooldown() === 0" - [class.text-blue-600]="resendCooldown() === 0" - [class.hover:text-blue-700]="resendCooldown() === 0" - [class.text-slate-400]="resendCooldown() > 0" - [class.cursor-not-allowed]="resendCooldown() > 0" + class="text-sm font-medium disabled:cursor-not-allowed disabled:text-slate-400" + [class.text-blue-600]="resendCooldown() === 0" + [class.hover:text-blue-700]="resendCooldown() === 0" [disabled]="resendCooldown() > 0" (click)="onResendCode()" data-testid="verify-resend">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.html` around lines 62 - 77, The button currently mixes the native disabled attribute with class-based "disabled" styling; simplify by relying on [disabled]="resendCooldown() > 0" and remove the redundant class bindings ([class.cursor-not-allowed], [class.text-slate-400], and [class.cursor-pointer]) while keeping conditional visual states for the enabled case (e.g., [class.text-blue-600] and [class.hover:text-blue-700] when resendCooldown() === 0). Ensure the (click)="onResendCode()" remains and that enabled-only classes reference the resendCooldown() === 0 condition so styling reflects the disabled attribute; keep data-testid="verify-resend" and the existing content/ICON markup unchanged.apps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.html (1)
119-119: Consider memoizinggetMenuItems()with a computed signal.While the method is technically called during each change detection cycle, the impact is minimal since the component already uses
ChangeDetectionStrategy.OnPush. However, the method could be optimized by pre-computing the menu items as a computed signal to follow Angular best practices and avoid recreating the array on each call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.html` at line 119, The template calls getMenuItems(item) each change detection; replace this with a computed signal to memoize the menu array: add a computed signal (e.g., menuItemsFor = computed((item: ItemType) => ...)) or a factory method that returns a signal for a given item inside ProfileWorkExperienceComponent and use that signal in the template instead of getMenuItems; update the template binding from [model]="getMenuItems(item)" to bind the computed signal (e.g., [model]="menuItemsFor(item)"/menuItemsFor(item)()) and remove or refactor the getMenuItems method to return the signal or be eliminated to avoid recreating the array every cycle.apps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.ts (2)
51-63: Date parsing assumes specific format.The date parsing logic assumes dates are in
"Month Year"format (e.g., "January 2024"). IfstartDateorendDatehas an unexpected format, the parsing silently falls back to empty strings, which could lead to confusing UX in edit mode.Consider adding defensive validation or logging if the format doesn't match expectations.
♻️ Example defensive approach
const startParts = exp.startDate?.split(' ') ?? []; if (startParts.length !== 2) { console.warn('Unexpected startDate format:', exp.startDate); } const startMonth = this.monthOptions.find((m) => m.label.startsWith(startParts[0] || ''))?.value || ''; const startYear = startParts[1] || '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.ts` around lines 51 - 63, The date parsing in the edit flow (variables exp.startDate, exp.endDate, startParts/endParts and monthOptions lookup in work-experience-form-dialog.component) assumes "Month Year" and silently falls back to empty strings; add defensive validation: split with optional chaining (exp.startDate?.split(' ') ?? [] / exp.endDate?.split(' ') ?? []), check parts.length === 2 and if not, emit a console.warn or logger.warn including the raw value, and still fallback safely to empty strings for startMonth/startYear/endMonth/endYear so the form remains stable in edit mode; update the isCurrently logic to use a normalized check (e.g. treat null/undefined/empty string as current) so unexpected formats don't flip the boolean unexpectedly.
26-27: Add generic type toDynamicDialogConfigfor type safety.For consistency with other dialog components and better type inference, add the generic type parameter.
♻️ Suggested improvement
- private readonly config = inject(DynamicDialogConfig); + private readonly config = inject(DynamicDialogConfig<WorkExperienceFormDialogData>);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.ts` around lines 26 - 27, Update the DynamicDialogConfig injection to include the dialog data generic for type safety: change the injection in work-experience-form-dialog.component (the line using inject(DynamicDialogConfig)) to inject DynamicDialogConfig<T> with the appropriate dialog-data interface (e.g. WorkExperienceFormDialogData) used by this dialog, create or import that interface if missing, and update any usages of config.data to the strongly typed property; ensure the generic is imported/available so TypeScript can infer data shape throughout the component.apps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.ts (2)
36-36: Unnecessarycomputed()for static value.
this.identity.provideris a plain value (not a signal), so wrapping the check incomputed()provides no reactive benefit. Consider using a simple readonly property instead.♻️ Suggested simplification
- public readonly isEmailProvider = computed(() => this.identity.provider === 'email'); + public readonly isEmailProvider: boolean = this.identity.provider === 'email';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.ts` at line 36, The isEmailProvider property is wrapped in computed() despite identity.provider being a plain value; remove the unnecessary signal wrapper and replace it with a simple readonly boolean property (e.g., public readonly isEmailProvider = this.identity.provider === 'email') so the check uses a plain property; update any references to isEmailProvider accordingly and ensure no dependent code expects it to be a signal.
131-146: MultipleonDestroycallbacks registered on repeated calls.Each call to
startResendCooldown()registers a newdestroyRef.onDestroy()callback. While this doesn't cause functional issues (callingclearCooldown()multiple times is safe), it's inefficient and could accumulate callbacks if the user resends multiple times.Consider registering the cleanup callback once during component initialization.
♻️ Suggested fix
Register the cleanup in the constructor or as a class initializer:
+ public constructor() { + this.destroyRef.onDestroy(() => this.clearCooldown()); + } + private startResendCooldown(): void { this.clearCooldown(); this.resendCooldown.set(60); this.cooldownInterval = setInterval(() => { const current = this.resendCooldown(); if (current <= 1) { this.resendCooldown.set(0); this.clearCooldown(); } else { this.resendCooldown.set(current - 1); } }, 1000); - - this.destroyRef.onDestroy(() => this.clearCooldown()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.ts` around lines 131 - 146, The startResendCooldown method currently registers destroyRef.onDestroy(() => this.clearCooldown()) each time it's called; instead, register that onDestroy callback once (e.g., in the constructor or ngOnInit) and remove the onDestroy registration from startResendCooldown so clearCooldown is invoked on component teardown only once; locate startResendCooldown and clearCooldown and move the destroyRef.onDestroy registration into the component constructor or initialization block to avoid accumulating callbacks on repeated resends.apps/lfx-one/src/app/modules/profile/components/work-experience-confirmation-dialog/work-experience-confirmation-dialog.component.ts (1)
17-18: Add generic type toDynamicDialogConfigfor type safety.Other dialog components in this PR (e.g.,
RemoveIdentityDialogComponent,VerifyIdentityDialogComponent) use typed config. Consider adding the generic type here for consistency and better type inference.♻️ Suggested improvement
- private readonly config = inject(DynamicDialogConfig); + private readonly config = inject(DynamicDialogConfig<{ updates: WorkExperienceUpdate[] }>);Then access via
this.config.data.updateswith proper typing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/work-experience-confirmation-dialog/work-experience-confirmation-dialog.component.ts` around lines 17 - 18, The DynamicDialogConfig injection in work-experience-confirmation-dialog.component should be given a specific generic type for its data payload to match other dialogs; update the injected config declaration (the private readonly config = inject(DynamicDialogConfig)) to use the appropriate generic interface/type used by RemoveIdentityDialogComponent/VerifyIdentityDialogComponent, then update usages to access typed properties (e.g., this.config.data.updates) so callers and consumers (in this component and any methods referencing config or ref) have proper type safety and inference.apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.html (1)
28-32: Consider using Tailwind color utility instead of arbitrary value.The hardcoded color
bg-[#cad5e2]is very close to Tailwind'sbg-slate-300(#cbd5e1). Using the utility class would improve consistency and maintainability.♻️ Suggested change
<div - class="flex h-[42px] w-[42px] shrink-0 items-center justify-center rounded-full bg-[`#cad5e2`] text-base font-semibold text-white" + class="flex h-[42px] w-[42px] shrink-0 items-center justify-center rounded-full bg-slate-300 text-base font-semibold text-white" data-testid="profile-avatar">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.html` around lines 28 - 32, The profile avatar div uses an arbitrary background color class "bg-[`#cad5e2`]"; replace it with the nearest Tailwind utility (e.g., "bg-slate-300") on the element with data-testid="profile-avatar" in the profile-layout component template so the avatar styling uses the standard Tailwind color token while keeping the rest of the classes and the initials() interpolation unchanged.apps/lfx-one/src/app/modules/profile/profile.routes.ts (1)
40-49: Consider adding explicitpathMatch: 'full'to redirect routes.While single-segment redirects work correctly with the default
pathMatch: 'prefix', adding explicitpathMatch: 'full'improves clarity and prevents subtle bugs if paths are later extended.♻️ Example for one redirect
- { path: 'overview', redirectTo: 'attribution' }, + { path: 'overview', redirectTo: 'attribution', pathMatch: 'full' },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/profile.routes.ts` around lines 40 - 49, The redirect route entries in profile.routes.ts (e.g., the objects with path: 'overview', 'edit', 'affiliations', etc., that have redirectTo: 'attribution' or 'identities') should include explicit pathMatch: 'full' to avoid prefix-matching surprises; update each redirect route object (the redirect entries in the routes array) to add pathMatch: 'full' so each redirect only triggers on an exact path match.apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts (1)
186-262: Consider consolidating duplicate resolution logic.
resolveCurrentEntry()andresolveOrg()have similar logic for resolving organizations and updating state. Consider extracting the common logic to reduce duplication.♻️ Suggested consolidation
+ private handleResolveResult(cdpOrg: CdpOrganization, originalName: string): OrganizationResolveResult { + const result: OrganizationResolveResult = { + id: cdpOrg.id, + name: cdpOrg.name, + logo: cdpOrg.logo, + originalName, + nameChanged: cdpOrg.name.toLowerCase() !== originalName.toLowerCase(), + }; + this.resolvedOrg.set(result); + this.resolvingOrg.set(false); + this.onOrganizationResolved.emit(result); + + // Update field text to the resolved name + this.organizationForm.get('organizationSearch')?.setValue(cdpOrg.name, { emitEvent: false }); + const nameCtrl = this.nameControl(); + if (nameCtrl && this.form().get(nameCtrl)) { + this.form().get(nameCtrl)?.setValue(cdpOrg.name); + } + return result; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts` around lines 186 - 262, Both resolveCurrentEntry() and resolveOrg() duplicate the logic that transforms cdpOrg into an OrganizationResolveResult, updates resolvedOrg/resolvingOrg, emits onOrganizationResolved, and writes the resolved name back into the form; extract that into a single private helper (e.g., applyResolvedOrganization(cdpOrg: CdpOrgType, originalName: string): OrganizationResolveResult) and a single error helper (e.g., handleResolveError(): Observable<null> or void) and call them from resolveCurrentEntry() (inside map/catchError) and resolveOrg() (inside subscribe.next/error). The helper should set this.resolvedOrg, this.resolvingOrg, emit onOrganizationResolved, and update this.organizationForm.get('organizationSearch') and the parent form control returned by this.nameControl(); replace the duplicated blocks in both resolveCurrentEntry and resolveOrg with calls to these helpers.apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.ts (1)
101-117: Consider using Angular Router for navigation.Using
window.location.hreffor navigation (lines 109, 160) causes a full page reload. If Flow C auth supports in-app handling, consider using Angular'sRouterto preserve application state.However, if the navigation is intentionally to an Express endpoint (
/api/profile/auth/start), the current approach is correct since it needs to leave the Angular app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.ts` around lines 101 - 117, Replace the hard navigation via window.location.href in the onRemove method (and the other occurrence at the same pattern) with Angular Router navigation to avoid a full page reload when the auth flow can be handled in-app: inject Router in the component constructor and call this.router.navigate or this.router.navigateByUrl with the equivalent path and returnTo parameter (e.g., navigate(['/api/profile/auth/start'], { queryParams: { returnTo: '/profile/identities' } }) or adjust to the in-app auth route), and if the Express endpoint must be hit (intentional external navigation), keep window.location.href but add a comment clarifying that external navigation is required.apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.html (1)
138-146: Placeholderhref="#"causes unwanted navigation.The "Source" link uses
href="#"which will scroll to page top when clicked. If this link should be dynamically populated later, consider:
- Using
href="javascript:void(0)"or[href]="null"to prevent navigation- Adding
(click)="$event.preventDefault()"- Or removing the
<a>tag until the actual URL is availableThe same issue exists at lines 281-289 for "View source".
♻️ Suggested fix
<a - class="flex items-center gap-1 text-[11px] font-medium text-slate-500 opacity-0 transition-opacity group-hover:opacity-100 hover:text-slate-900" - href="#" + class="flex cursor-pointer items-center gap-1 text-[11px] font-medium text-slate-500 opacity-0 transition-opacity group-hover:opacity-100 hover:text-slate-900" + [href]="seg.sourceUrl || null" target="_blank" rel="noopener noreferrer" + (click)="seg.sourceUrl ? null : $event.preventDefault()" [attr.data-testid]="'compact-source-link-' + group.id">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.html` around lines 138 - 146, The anchor with text "Source" (data-testid="'compact-source-link-' + group.id") uses href="#" causing top-of-page navigation; update the template to prevent navigation by removing the static href and either bind it to null ([attr.href]="null") or add a click handler that prevents default ((click)="$event.preventDefault()") so the link doesn't navigate when no real URL is available; apply the same change to the other "View source" anchor in the file.apps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.ts (1)
274-282: FallbackDate(0)may cause unexpected sorting behavior.When
parseDatefails to parse a date string (e.g., malformed input), it returnsnew Date(0)(January 1, 1970). This could cause entries with invalid dates to sort to unexpected positions. Consider logging a warning or using a more explicit fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.ts` around lines 274 - 282, The parseDate function currently returns new Date(0) on malformed input, which can skew sorting; change parseDate (in profile-work-experience.component.ts) to return a null (or undefined) sentinel instead of new Date(0), and add a console.warn (or use the component logger) that includes the original dateStr and that MONTH_OPTIONS was used, so invalid dates are visible in logs; also update any callers that rely on parseDate to handle the null/undefined return (e.g., skip, place at end, or use a specific sort fallback) so sorting logic no longer treats invalid dates as 1970.apps/lfx-one/src/app/modules/profile/components/affiliation-timeline-dialog/affiliation-timeline-dialog.component.ts (1)
281-292: Potential ID collision increateDefaultPeriod.
createDefaultPeriodusesperiod-${Date.now()}for IDs, which could collide if called multiple times within the same millisecond.createEmptyPeriod(line 296) correctly adds a random suffix. Consider applying the same pattern here for consistency.♻️ Proposed fix
private createDefaultPeriod(org: AffiliationEditOrg): AffiliationEditPeriod { const startParts = org.weStartDate ? org.weStartDate.split(' ') : []; const endParts = org.weEndDate ? org.weEndDate.split(' ') : []; return { - id: `period-${Date.now()}`, + id: `period-${Date.now()}-${Math.random().toString(36).slice(2, 7)}`, startMonth: startParts[0] || '', startYear: startParts[1] || '', endMonth: endParts[0] || '', endYear: endParts[1] || '', isPresent: !org.weEndDate, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/affiliation-timeline-dialog/affiliation-timeline-dialog.component.ts` around lines 281 - 292, createDefaultPeriod currently generates IDs with `period-${Date.now()}` which can collide; update createDefaultPeriod to append the same random suffix pattern used in createEmptyPeriod (e.g., include a short random string or Math.random-based token) so IDs become `period-${Date.now()}-${randomSuffix}`; modify the createDefaultPeriod function to build the id with that suffix and keep all other fields unchanged, mirroring the logic from createEmptyPeriod for consistency.apps/lfx-one/src/app/modules/profile/components/add-account-dialog/add-account-dialog.component.ts (1)
153-168:onDestroycallback registered on every cooldown start.Each call to
startResendCooldown()registers a newonDestroycallback. WhileclearCooldown()is idempotent, this accumulates unnecessary callbacks. Consider registering the cleanup once in the constructor.♻️ Proposed fix
+ public constructor() { + this.destroyRef.onDestroy(() => this.clearCooldown()); + } + private startResendCooldown(): void { this.clearCooldown(); this.resendCooldown.set(60); this.cooldownInterval = setInterval(() => { const current = this.resendCooldown(); if (current <= 1) { this.resendCooldown.set(0); this.clearCooldown(); } else { this.resendCooldown.set(current - 1); } }, 1000); - - this.destroyRef.onDestroy(() => this.clearCooldown()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/app/modules/profile/components/add-account-dialog/add-account-dialog.component.ts` around lines 153 - 168, startResendCooldown currently calls destroyRef.onDestroy every time it's invoked, causing multiple redundant onDestroy callbacks to be registered; move the one-time registration into the component constructor so cleanup is registered once. Update the constructor to call this.destroyRef.onDestroy(() => this.clearCooldown()) and remove the same call from startResendCooldown; keep clearCooldown and cooldownInterval logic unchanged so clearCooldown remains idempotent and still cancels the interval when the component is destroyed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b7a276f-28e4-4c00-be32-58fe7c22d259
📒 Files selected for processing (67)
apps/lfx-one/.env.exampleapps/lfx-one/e2e/profile-identities-verify-robust.spec.tsapps/lfx-one/e2e/profile-identities-verify.spec.tsapps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.htmlapps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.tsapps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.htmlapps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.tsapps/lfx-one/src/app/modules/profile/attribution/profile-attribution.component.htmlapps/lfx-one/src/app/modules/profile/attribution/profile-attribution.component.tsapps/lfx-one/src/app/modules/profile/components/add-account-dialog/add-account-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/add-account-dialog/add-account-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/affiliation-timeline-dialog/affiliation-timeline-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/affiliation-timeline-dialog/affiliation-timeline-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/how-affiliations-work-dialog/how-affiliations-work-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/how-affiliations-work-dialog/how-affiliations-work-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/maintainer-confirmation-dialog/maintainer-confirmation-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/maintainer-confirmation-dialog/maintainer-confirmation-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/profile-edit-dialog/profile-edit-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/profile-edit-dialog/profile-edit-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/remove-identity-dialog/remove-identity-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/remove-identity-dialog/remove-identity-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/verify-identity-dialog/verify-identity-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/work-experience-confirmation-dialog/work-experience-confirmation-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/work-experience-confirmation-dialog/work-experience-confirmation-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/work-experience-delete-dialog/work-experience-delete-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/work-experience-delete-dialog/work-experience-delete-dialog.component.tsapps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.htmlapps/lfx-one/src/app/modules/profile/components/work-experience-form-dialog/work-experience-form-dialog.component.tsapps/lfx-one/src/app/modules/profile/developer/profile-developer.component.htmlapps/lfx-one/src/app/modules/profile/email/profile-email.component.htmlapps/lfx-one/src/app/modules/profile/email/profile-email.component.tsapps/lfx-one/src/app/modules/profile/identities/profile-identities.component.htmlapps/lfx-one/src/app/modules/profile/identities/profile-identities.component.tsapps/lfx-one/src/app/modules/profile/manage-profile/profile-manage.component.htmlapps/lfx-one/src/app/modules/profile/manage-profile/profile-manage.component.tsapps/lfx-one/src/app/modules/profile/password/profile-password.component.htmlapps/lfx-one/src/app/modules/profile/password/profile-password.component.tsapps/lfx-one/src/app/modules/profile/profile.routes.tsapps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.htmlapps/lfx-one/src/app/modules/profile/work-experience/profile-work-experience.component.tsapps/lfx-one/src/app/shared/components/checkbox/checkbox.component.htmlapps/lfx-one/src/app/shared/components/checkbox/checkbox.component.tsapps/lfx-one/src/app/shared/components/organization-search/organization-search.component.htmlapps/lfx-one/src/app/shared/components/organization-search/organization-search.component.tsapps/lfx-one/src/app/shared/services/organization.service.tsapps/lfx-one/src/app/shared/services/user.service.tsapps/lfx-one/src/server/controllers/organization.controller.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/middleware/auth.middleware.tsapps/lfx-one/src/server/routes/organizations.route.tsapps/lfx-one/src/server/routes/profile.route.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/auth0.service.tsapps/lfx-one/src/server/services/cdp.service.tsapps/lfx-one/src/server/services/email-verification.service.tsapps/lfx-one/src/server/services/profile-auth.service.tsapps/lfx-one/src/server/services/social-verification.service.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/utils/m2m-token.util.tsapps/lfx-one/src/types/express.d.tspackages/shared/src/constants/api.constants.tspackages/shared/src/constants/profile.constants.tspackages/shared/src/enums/nats.enum.tspackages/shared/src/interfaces/organization.interface.tspackages/shared/src/interfaces/profile.interface.tspackages/shared/src/utils/date-time.utils.ts
💤 Files with no reviewable changes (2)
- apps/lfx-one/src/app/modules/profile/password/profile-password.component.ts
- apps/lfx-one/src/server/services/user.service.ts
✅ Files skipped from review due to trivial changes (5)
- apps/lfx-one/src/app/shared/components/checkbox/checkbox.component.ts
- apps/lfx-one/src/app/shared/components/checkbox/checkbox.component.html
- packages/shared/src/interfaces/organization.interface.ts
- packages/shared/src/enums/nats.enum.ts
- packages/shared/src/constants/api.constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/lfx-one/src/app/modules/profile/email/profile-email.component.ts
- apps/lfx-one/src/app/modules/profile/manage-profile/profile-manage.component.ts
- packages/shared/src/constants/profile.constants.ts
Comment on lines
+303
to
+305
| if (ops.length > 0) { | ||
| forkJoin(ops).pipe(take(1)).subscribe(); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent completion of affiliation patch operations.
The forkJoin completes without checking whether any patches succeeded or failed. Each operation uses catchError(() => of({ success: false })), but results aren't validated. Consider providing user feedback on success/failure.
🛡️ Proposed fix
if (ops.length > 0) {
- forkJoin(ops).pipe(take(1)).subscribe();
+ forkJoin(ops).pipe(take(1)).subscribe((results) => {
+ const allSucceeded = results.every((r) => r.success);
+ // Consider adding toast notification based on allSucceeded
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/lfx-one/src/app/modules/profile/affiliations/profile-affiliations.component.ts`
around lines 303 - 305, The forkJoin call on ops currently subscribes silently;
change it to subscribe to the emitted results and validate each operation's
outcome (each operation returns an object with success: boolean from
catchError). In the subscribe callback for forkJoin(ops).pipe(take(1)), inspect
the results array (from the forkJoin observable), detect any failed entries
(result.success === false), and surface user feedback via the existing UI
notification mechanism (e.g., show an error toast for failures and a success
toast if all succeeded) and/or trigger any follow-up refresh (e.g., reload
affiliations); update the subscription in profile-affiliations.component.ts that
wraps forkJoin(ops) to perform this result validation and feedback instead of
calling subscribe() with no handler.
Comment on lines
+134
to
+142
| public decodeAndValidateSub(accessToken: string, expectedSub: string): boolean { | ||
| const parts = accessToken.split('.'); | ||
| if (parts.length !== 3) { | ||
| return false; | ||
| } | ||
|
|
||
| const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString()); | ||
| return payload.sub === expectedSub; | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for JWT payload decoding.
JSON.parse on line 140 could throw if the JWT payload is malformed. While tokens received directly from Auth0 should be valid, defensive coding prevents crashes from unexpected data.
🛡️ Suggested fix
public decodeAndValidateSub(accessToken: string, expectedSub: string): boolean {
- const parts = accessToken.split('.');
- if (parts.length !== 3) {
+ try {
+ const parts = accessToken.split('.');
+ if (parts.length !== 3) {
+ return false;
+ }
+
+ const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString());
+ return payload.sub === expectedSub;
+ } catch {
return false;
}
-
- const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString());
- return payload.sub === expectedSub;
}📝 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.
Suggested change
| public decodeAndValidateSub(accessToken: string, expectedSub: string): boolean { | |
| const parts = accessToken.split('.'); | |
| if (parts.length !== 3) { | |
| return false; | |
| } | |
| const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString()); | |
| return payload.sub === expectedSub; | |
| } | |
| public decodeAndValidateSub(accessToken: string, expectedSub: string): boolean { | |
| try { | |
| const parts = accessToken.split('.'); | |
| if (parts.length !== 3) { | |
| return false; | |
| } | |
| const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString()); | |
| return payload.sub === expectedSub; | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/lfx-one/src/server/services/profile-auth.service.ts` around lines 134 -
142, The decodeAndValidateSub method can throw when decoding/parsing the JWT
payload; wrap the Buffer.from(parts[1], 'base64url').toString() and
JSON.parse(...) in a try-catch so malformed tokens don't crash the process and
instead cause the method to return false. Specifically, in decodeAndValidateSub,
validate parts length as you already do, then try to decode parts[1] and
JSON.parse the payload inside a try block, and on any error (invalid base64url
or JSON) catch it and return false; keep the final check as return payload.sub
=== expectedSub to preserve behavior when parsing succeeds.
Comment on lines
+95
to
+108
| const response = await fetch(tokenEndpoint, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Cache-Control': 'no-cache', | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ | ||
| grant_type: 'authorization_code', | ||
| client_id: this.clientId, | ||
| client_secret: this.clientSecret, | ||
| code, | ||
| redirect_uri: this.redirectUri, | ||
| }), | ||
| }); |
Contributor
There was a problem hiding this comment.
Add a timeout to the token exchange fetch request.
The fetch call to the Auth0 token endpoint lacks a timeout, which could cause the request to hang indefinitely if the Auth0 server is unresponsive. Other services in this PR (e.g., CdpService) consistently use AbortSignal.timeout(10000).
🛠️ Proposed fix
const response = await fetch(tokenEndpoint, {
method: 'POST',
headers: {
'Cache-Control': 'no-cache',
'Content-Type': 'application/json',
},
body: JSON.stringify({
grant_type: 'authorization_code',
client_id: this.clientId,
client_secret: this.clientSecret,
code,
redirect_uri: this.redirectUri,
}),
+ signal: AbortSignal.timeout(10000),
});📝 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.
Suggested change
| const response = await fetch(tokenEndpoint, { | |
| method: 'POST', | |
| headers: { | |
| 'Cache-Control': 'no-cache', | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| grant_type: 'authorization_code', | |
| client_id: this.clientId, | |
| client_secret: this.clientSecret, | |
| code, | |
| redirect_uri: this.redirectUri, | |
| }), | |
| }); | |
| const response = await fetch(tokenEndpoint, { | |
| method: 'POST', | |
| headers: { | |
| 'Cache-Control': 'no-cache', | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify({ | |
| grant_type: 'authorization_code', | |
| client_id: this.clientId, | |
| client_secret: this.clientSecret, | |
| code, | |
| redirect_uri: this.redirectUri, | |
| }), | |
| signal: AbortSignal.timeout(10000), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/lfx-one/src/server/services/social-verification.service.ts` around lines
95 - 108, The token exchange fetch call (const response = await
fetch(tokenEndpoint, { ... })) currently has no timeout; add a 10s AbortSignal
by passing signal: AbortSignal.timeout(10000) in the fetch options so the
request fails fast if Auth0 is unresponsive, and ensure the surrounding method
(e.g., the token exchange method in SocialVerificationService) will propagate or
handle the abort error consistently with other services like CdpService.
Comment on lines
+106
to
+110
| // Cache with 5-minute buffer before expiry | ||
| tokenCache.set(cacheKey, { | ||
| token: tokenResponse.access_token, | ||
| expiresAt: Date.now() + (tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS) * 1000, | ||
| }); |
Contributor
There was a problem hiding this comment.
Handle edge case where expires_in is shorter than the buffer.
If the token's expires_in is less than 300 seconds, the calculation expires_in - TOKEN_EXPIRY_BUFFER_SECONDS produces a negative value, resulting in immediate cache invalidation. While harmless (the token still works), it defeats caching for short-lived tokens.
🛡️ Proposed fix
// Cache with 5-minute buffer before expiry
+ const effectiveExpiresIn = Math.max(tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS, 0);
tokenCache.set(cacheKey, {
token: tokenResponse.access_token,
- expiresAt: Date.now() + (tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS) * 1000,
+ expiresAt: Date.now() + effectiveExpiresIn * 1000,
});📝 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.
Suggested change
| // Cache with 5-minute buffer before expiry | |
| tokenCache.set(cacheKey, { | |
| token: tokenResponse.access_token, | |
| expiresAt: Date.now() + (tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS) * 1000, | |
| }); | |
| // Cache with 5-minute buffer before expiry | |
| const effectiveExpiresIn = Math.max(tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS, 0); | |
| tokenCache.set(cacheKey, { | |
| token: tokenResponse.access_token, | |
| expiresAt: Date.now() + effectiveExpiresIn * 1000, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/lfx-one/src/server/utils/m2m-token.util.ts` around lines 106 - 110, The
cache expiry calculation can go negative when tokenResponse.expires_in <
TOKEN_EXPIRY_BUFFER_SECONDS causing immediate cache invalidation; in the
tokenCache.set call that builds expiresAt, clamp the adjusted lifetime to a
non-negative (or at least 1 second) value before multiplying by 1000 (e.g., use
Math.max(tokenResponse.expires_in - TOKEN_EXPIRY_BUFFER_SECONDS, 1)) so
expiresAt = Date.now() + clampedValue * 1000; keep the rest of the
tokenResponse.access_token and cache structure unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
docs/architecture/backend/README.md (1)
179-197:⚠️ Potential issue | 🟡 Minor
profile.service.tsis still missing from the services tree.This remains inconsistent with the backend service set and was already flagged earlier. Please add it to this section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/backend/README.md` around lines 179 - 197, The services list in the architecture README omits profile.service.ts; update the services tree (the block listing service files such as ai.service.ts, user.service.ts, etc.) to include an entry for profile.service.ts (e.g., add "├── profile.service.ts # Profile business logic") so the documentation matches the actual backend service set.apps/lfx-one/src/server/server.ts (1)
183-189:⚠️ Potential issue | 🟠 MajorAdd rate limiting on auth callback routes.
These callback endpoints are still unthrottled; this matches the previously reported security finding on Line 185 and should be addressed before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/src/server/server.ts` around lines 183 - 189, The auth callback routes /passwordless/callback and /social/callback are unthrottled; add a rate limiting middleware and apply it to these endpoints to mitigate brute‑force/abuse. Install or import your rate limiter (e.g., express-rate-limit) and create a conservative limiter instance (e.g., low requests per minute with a short window and standard headers/penalties), then update the route registrations to use the limiter before invoking profileCallbackController.handleProfileAuthCallback and profileCallbackController.handleSocialCallback so both routes are protected.apps/lfx-one/.env.example (1)
77-77:⚠️ Potential issue | 🟡 MinorQuote
PROFILE_SCOPEto preserve the full scope string.Line 77 still uses an unquoted value with spaces, which can be parsed incorrectly by dotenv/shell parsers.
🔧 Proposed fix
-PROFILE_SCOPE=update:current_user_metadata update:current_user_identities +PROFILE_SCOPE="update:current_user_metadata update:current_user_identities"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx-one/.env.example` at line 77, PROFILE_SCOPE currently contains an unquoted value with a space which dotenv/shell parsers can split; update the PROFILE_SCOPE entry so the entire scope string is quoted (e.g., wrap the value in double quotes) to preserve the full token "update:current_user_metadata update:current_user_identities" when parsed by dotenv and shell tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/backend/README.md`:
- Line 33: The docs incorrectly reference BaseError; update the README entry
listing the custom error hierarchy to use the actual base class name
BaseApiError (and keep the other class names AuthenticationError,
AuthorizationError, MicroserviceError, ServiceValidationError) so the
documentation matches the implementation (search for occurrences of "BaseError"
in the file and replace with "BaseApiError").
- Line 136: Update the directory counts in docs/architecture/backend/README.md
so they match the actual file tree: reconcile the headings that state "15
controllers", "15 routes", and "20 services" with the actual listed directories
("controllers/", "routes/", "services/") which currently show 13 controllers, 13
routes, and 18 services; either correct the numeric counts to reflect the
current repository contents or add/remove the listed entries so the counts and
visible trees align (apply the same fix to the other occurrences referenced near
the "controllers/" and "services/" listings).
- Line 56: The heading "#### Committee Management" causes a level jump from "##"
to "####" and fails markdown lint; change that heading to "### Committee
Management" (update the heading text in the docs/architecture/backend/README.md
where "#### Committee Management" appears) so it follows the proper sequence and
resolves MD001/CI lint errors.
- Around line 131-132: Add the required blank line separation between the
unordered list ending with "**Testing**: E2E testing with Playwright" and the
following heading "## Directory Structure" to satisfy MD032/MD022; locate the
list item text and the "## Directory Structure" heading in
docs/architecture/backend/README.md and insert a single blank line above the
heading (and ensure a blank line below the list if not already present) so the
list and heading are separated correctly.
---
Duplicate comments:
In `@apps/lfx-one/.env.example`:
- Line 77: PROFILE_SCOPE currently contains an unquoted value with a space which
dotenv/shell parsers can split; update the PROFILE_SCOPE entry so the entire
scope string is quoted (e.g., wrap the value in double quotes) to preserve the
full token "update:current_user_metadata update:current_user_identities" when
parsed by dotenv and shell tools.
In `@apps/lfx-one/src/server/server.ts`:
- Around line 183-189: The auth callback routes /passwordless/callback and
/social/callback are unthrottled; add a rate limiting middleware and apply it to
these endpoints to mitigate brute‑force/abuse. Install or import your rate
limiter (e.g., express-rate-limit) and create a conservative limiter instance
(e.g., low requests per minute with a short window and standard
headers/penalties), then update the route registrations to use the limiter
before invoking profileCallbackController.handleProfileAuthCallback and
profileCallbackController.handleSocialCallback so both routes are protected.
In `@docs/architecture/backend/README.md`:
- Around line 179-197: The services list in the architecture README omits
profile.service.ts; update the services tree (the block listing service files
such as ai.service.ts, user.service.ts, etc.) to include an entry for
profile.service.ts (e.g., add "├── profile.service.ts # Profile business
logic") so the documentation matches the actual backend service set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8621e7ed-69cd-42f5-b1c6-08010f06864a
📒 Files selected for processing (14)
CLAUDE.mdapps/lfx-one/.env.exampleapps/lfx-one/src/server/server.tsapps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/utils/m2m-token.util.tsdocs/architecture/backend/README.mddocs/architecture/backend/ssr-server.mddocs/architecture/frontend/component-architecture.mddocs/architecture/frontend/lazy-loading-preloading-strategy.mddocs/architecture/frontend/styling-system.mddocs/architecture/shared/package-architecture.mddocs/architecture/testing/e2e-testing.mdpackages/shared/src/constants/index.tspackages/shared/src/enums/nats.enum.ts
💤 Files with no reviewable changes (1)
- apps/lfx-one/src/server/services/user.service.ts
✅ Files skipped from review due to trivial changes (5)
- packages/shared/src/constants/index.ts
- packages/shared/src/enums/nats.enum.ts
- docs/architecture/shared/package-architecture.md
- docs/architecture/backend/ssr-server.md
- docs/architecture/frontend/lazy-loading-preloading-strategy.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/architecture/frontend/styling-system.md
- apps/lfx-one/src/server/utils/m2m-token.util.ts
- docs/architecture/frontend/component-architecture.md
- CLAUDE.md
- docs/architecture/testing/e2e-testing.md
- Fix polynomial ReDoS in email validation regex (profile.controller.ts) - Fix SSRF via encodeURIComponent on user-controlled path segments (cdp.service.ts) - Standardize token expiry buffer to 5 minutes (cdp.service.ts) - Wire up empty state buttons in affiliations component - Fix markdown lint errors in backend docs from merge resolution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
- Fix ReDoS in email validation regex (profile.controller.ts) - Fix SSRF via encodeURIComponent on path segments (cdp.service.ts) - Standardize token expiry buffer to 5 minutes - Wire up empty state buttons in affiliations component - Sync mobile tab dropdown with route changes (profile-layout) - Add JSON.parse error handling for Flow C session restore - Add error handlers for work experience CRUD operations - Fix duplicate HTML id for state/province field - Quote PROFILE_SCOPE in .env.example - Fix markdown lint errors in backend docs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
CodeQL flagged /passwordless/callback and /social/callback as missing
rate limiting despite app.use('/passwordless/', apiRateLimiter) being
declared earlier in the middleware chain. CodeQL's static analysis
does not reliably connect path-based app.use() to app.get() calls
registered later in the file. Applying apiRateLimiter as inline
middleware directly on the route handlers satisfies the CodeQL rule.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
MRashad26
requested changes
Mar 26, 2026
Contributor
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
PR Review — feat(profile): implement my profile redesign with overview tab
Author: asithade
Size: +9,981 / -305 across 81 files
What's good
- Flow C OAuth2 management token flow is well-structured with CSRF state params
- CDP token caching with 5-minute expiry buffer prevents unnecessary token requests
- NATS request/reply with timeout handling is solid
AbortSignal.timeout(10000)on all upstreamfetch()calls prevents hanging connectionsencodeURIComponent()on CDP API path params prevents path traversal
Issues — 3 Critical, 6 Major reported as inline comments below
…nd consistency - Fix open redirect vulnerability on returnTo param in profile auth flow - Add error toasts for silent catchError handlers in UserService - Add success/error handlers to fire-and-forget affiliation PATCH calls - Add ChangeDetectionStrategy.OnPush to 11 profile components - Convert template method calls to computed signal maps (6 methods across 4 components) - Extract duplicate resend cooldown logic into shared utility - Replace raw HTML tables with lfx-table component - Replace raw button/span elements with lfx-button and lfx-tag components Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
MRashad26
previously approved these changes
Mar 26, 2026
Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
Scope: eliminates separate CDP_AUTH0_* env vars — CDP now reuses PCC client credentials with a dedicated CDP_AUDIENCE, avoiding cross-tenant Auth0 configuration issues for deployment. Signed-off-by: Fayaz G. <5818912+fayazg@users.noreply.github.com>
mauriciozanettisalomao
approved these changes
Mar 30, 2026
🧹 Deployment RemovedThe deployment for PR #240 has been removed. |
manishdixitlfx
pushed a commit
that referenced
this pull request
Mar 31, 2026
feat(profile): implement my profile redesign with overview tab Signed-off-by: Manish Dixit <mdixit@linuxfoundation.org>
manishdixitlfx
pushed a commit
that referenced
this pull request
Apr 1, 2026
feat(profile): implement my profile redesign with overview tab
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Routes
JIRA
LFXV2-1071
🤖 Generated with Claude Code