feat(profile): nats-based user profile updates with oidc fields #111
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUnifies profile updates into a single user_metadata flow across client, server, and shared interfaces. Renames profile fields (e.g., first_name→given_name, state→state_province). Replaces dual PATCH routes with one /api/profile endpoint calling a new server UserService that updates via NATS. Adjusts templates/layouts and meeting link construction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ProfileEditComponent (UI)
participant CUS as Client UserService
participant API as PATCH /api/profile
participant CTRL as profile.controller.updateUserMetadata
participant SUS as Server UserService
participant NATS as NATS
participant AUTH as Auth Service
UI->>CUS: Build ProfileUpdateRequest { user_metadata }
CUS->>API: PATCH /api/profile (user_metadata)
API->>CTRL: Route request
CTRL->>CTRL: Extract/validate token
CTRL->>SUS: updateUserMetadata(req, updates)
SUS->>SUS: validateUserMetadata(metadata)
SUS->>NATS: request(USER_METADATA_UPDATE, encoded payload)
NATS->>AUTH: Deliver update request
AUTH-->>NATS: Update response (success/error)
NATS-->>SUS: Msg (encoded)
SUS->>SUS: Decode/parse response
SUS-->>CTRL: UserMetadataUpdateResponse
alt success
CTRL-->>API: 200 { updated_fields, message }
else error
CTRL-->>API: Error mapped (401/403/404/422/503)
end
API-->>CUS: HTTP response
CUS-->>UI: Resolve submission result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR implements NATS-based user profile updates with OIDC-compliant field names, consolidating profile management into a unified architecture while improving error handling and service separation.
Key Changes
- Refactored user profile interfaces to use OIDC-compliant field names (job_title, state_province, postal_code, given_name, family_name)
- Implemented NATS Request/Reply pattern for user metadata updates with proper timeout handling
- Consolidated profile update endpoints from separate user/details endpoints to a single unified endpoint
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/user-profile.interface.ts | Added OIDC-compliant UserMetadata interface and removed deprecated interfaces |
| packages/shared/src/enums/nats.enum.ts | Added USER_METADATA_UPDATE subject and cleaned up interface definitions |
| apps/lfx-one/src/server/services/user.service.ts | New service handling NATS-based user metadata updates with validation |
| apps/lfx-one/src/server/controllers/profile.controller.ts | Replaced separate user/details endpoints with unified metadata update controller |
| apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts | Updated form fields and service calls to use new OIDC field names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Refactor user profile interfaces to OIDC-compliant field names - Add NATS messaging for user metadata updates via auth-service - Refactor NatsService to pure infrastructure pattern - Consolidate shared interfaces and remove duplicates - Improve error handling with proper error classes - Update frontend forms to use new field names - Minor UI improvements to project layout JIRA: LFXV2-633, LFXV2-634, LFXV2-635, LFXV2-636, LFXV2-637 Signed-off-by: Asitha De Silva <adesilva@linuxfoundation.org> Signed-off-by: Asitha de Silva <asithade@gmail.com>
asithade
force-pushed
the
feat/LFXV2-633
branch
from
be77f91 to
c0e2bdd
Compare
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
apps/lfx-one/src/server/services/user.service.ts (2)
33-36: Message mismatch: say 'Username is required'.Validation checks updates.username, but the error says 'User ID'. Align the message.
- throw new Error('User ID is required'); + throw new Error('Username is required');
99-106: Replaceanywith a typed metadata shape.Use Partial for validation input to preserve type-safety.
-import { UserMetadataUpdateRequest, UserMetadataUpdateResponse } from '@lfx-one/shared/interfaces'; +import { UserMetadata, UserMetadataUpdateRequest, UserMetadataUpdateResponse } from '@lfx-one/shared/interfaces'; @@ - public validateUserMetadata(metadata: any): boolean { + public validateUserMetadata(metadata: Partial<UserMetadata>): boolean {apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
229-231: Add transitional fallback for job_title.For mixed deployments, fall back to legacy 'title' when 'job_title' is absent.
- job_title: profile.profile?.job_title || '', + job_title: profile.profile?.job_title || profile.profile?.title || '',
🧹 Nitpick comments (14)
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html (2)
20-35: Add data-testid to new title/description elementsFor reliable tests, add data-testid attributes to the newly added UI. As per coding guidelines.
- <h1 class="text-2xl font-display font-semibold text-gray-900 mb-3"> + <h1 class="text-2xl font-display font-semibold text-gray-900 mb-3" data-testid="project-layout-title"> {{ projectTitle() }} </h1> @@ - <p class="text-gray-600 text-sm mb-6 max-w-3xl leading-relaxed"> + <p class="text-gray-600 text-sm mb-6 max-w-3xl leading-relaxed" data-testid="project-layout-description"> {{ description }} </p>
24-31: Avoid calling methods in templates (use signals/getters)projectTitle() and projectDescription() in templates can thrash change detection. Prefer signals/computed or assign values to template variables in TS.
apps/lfx-one/src/server/errors/index.ts (1)
5-5: Add a type guard for AuthorizationError to keep parityYou re-export AuthorizationError but don’t provide a type guard. Add it alongside the others.
export { BaseApiError } from './base.error'; -export { AuthenticationError, AuthorizationError } from './authentication.error'; +export { AuthenticationError, AuthorizationError } from './authentication.error'; export { MicroserviceError } from './microservice.error'; export { ServiceValidationError, ResourceNotFoundError } from './service-validation.error'; // Type guards for error identification -import { AuthenticationError } from './authentication.error'; +import { AuthenticationError, AuthorizationError } from './authentication.error'; import { BaseApiError } from './base.error'; import { MicroserviceError } from './microservice.error'; import { ServiceValidationError } from './service-validation.error'; @@ export function isAuthenticationError(error: unknown): error is AuthenticationError { return error instanceof AuthenticationError; } +export function isAuthorizationError(error: unknown): error is AuthorizationError { + return error instanceof AuthorizationError; +}apps/lfx-one/src/server/services/supabase.service.ts (1)
21-22: Map Supabase profile rows to UserMetadata (avoid leaking/raw shapes)You now return UserMetadata but pass through the raw profiles row. Prefer explicit mapping to only OIDC metadata fields; it prevents accidental shape drift and eases future changes. Also confirm DB columns have been migrated to OIDC names.
Proposed minimal mapping helper:
// inside SupabaseService private toUserMetadata(row: any): UserMetadata { if (!row) return null; const { name, given_name, family_name, job_title, organization, country, state_province, city, address, postal_code, phone_number, t_shirt_size, picture, zoneinfo, } = row || {}; return { name, given_name, family_name, job_title, organization, country, state_province, city, address, postal_code, phone_number, t_shirt_size, picture, zoneinfo, }; }Then:
-const data = await response.json(); -return data?.[0] || null; +const data = await response.json(); +return this.toUserMetadata(data?.[0]) || null;And:
-const result = await response.json(); -return result?.[0]; +const result = await response.json(); +return this.toUserMetadata(result?.[0]);Please also validate the profiles table uses OIDC column names (given_name, family_name, state_province, postal_code, t_shirt_size). If not, add a mapping translation until the DB migration lands.
Also applies to: 560-579, 584-612
apps/lfx-one/src/server/services/project.service.ts (2)
210-253: Harden NATS error handling: use error.code (Timeout/NoResponders) instead of message substringsString matching on error.message is brittle. The NATS client exposes error.code (e.g., TIMEOUT, NO_RESPONDERS/503). Check the code and treat those as not-found; rethrow others.
Suggested change:
- } catch (error) { - serverLogger.error({ error: error instanceof Error ? error.message : error, slug }, 'Failed to resolve project slug via NATS'); - - // If it's a timeout or no responder error, treat as not found - if (error instanceof Error && (error.message.includes('timeout') || error.message.includes('503'))) { + } catch (error: any) { + const message = error instanceof Error ? error.message : String(error); + const code = (error && (error.code || error?.cause?.code)) || ''; + serverLogger.error({ error: message, code, slug }, 'Failed to resolve project slug via NATS'); + + // If it's a timeout or no-responders error, treat as not found + if (String(code).toUpperCase().includes('TIMEOUT') || code === 503 || String(code).toUpperCase().includes('NO_RESPONDERS')) { return { projectId: '', slug, exists: false, }; } throw error; }Optionally, import and compare with the NATS ErrorCode enum if available in your client.
4-6: Verify logger import and prefer a single logging surface
- Importing serverLogger from ../server can risk circular deps; ensure it’s a pure export and not initialized after ProjectService.
- Consider using req.log consistently in request paths for correlation; use serverLogger only outside request scope.
Also applies to: 10-11
apps/lfx-one/src/server/services/nats.service.ts (2)
31-35: Use NATS RequestOptions type and preserve caller-provided options.Type the options param to the library’s RequestOptions and merge with default timeout.
-import { connect, NatsConnection, Msg, StringCodec, Codec } from 'nats'; +import { connect, NatsConnection, Msg, StringCodec, Codec, RequestOptions } from 'nats'; @@ - public async request(subject: string, data: Uint8Array, options?: { timeout?: number }): Promise<Msg> { + public async request(subject: string, data: Uint8Array, options?: RequestOptions): Promise<Msg> { @@ - const requestOptions = { - timeout: options?.timeout || NATS_CONFIG.REQUEST_TIMEOUT, - }; + const requestOptions: RequestOptions = { + timeout: options?.timeout ?? NATS_CONFIG.REQUEST_TIMEOUT, + ...options, + }; @@ - return await connection.request(subject, data, requestOptions); + return await connection.request(subject, data, requestOptions);Also applies to: 38-39
20-22: Avoid creating a new StringCodec per call.Cache a single codec instance; return it here.
- public getCodec(): Codec<string> { - return StringCodec(); - } + public getCodec(): Codec<string> { + return this.sc; + }Add this class field (outside the shown range):
private readonly sc = StringCodec();apps/lfx-one/src/app/shared/services/user.service.ts (2)
13-18: Import the response type for stronger typing.Include the update response interface so callers get typed fields like updated_fields/message.
- ProfileUpdateRequest, + ProfileUpdateRequest, + UserMetadataUpdateResponse,
45-50: Type the update response.Return Observable to align with backend contract.
- public updateUserProfile(data: ProfileUpdateRequest): Observable<any> { - return this.http.patch('/api/profile', data).pipe(take(1)); + public updateUserProfile(data: ProfileUpdateRequest): Observable<UserMetadataUpdateResponse> { + return this.http.patch<UserMetadataUpdateResponse>('/api/profile', data).pipe(take(1)); }apps/lfx-one/src/server/services/user.service.ts (3)
22-29: JSDoc params are out of sync with signature.The doc mentions userId and token params that aren’t in the signature. Update the JSDoc to match (req, updates).
186-189: Redundant timeout; NatsService already defaults it.You can omit passing timeout here unless you want a per-call override.
- const response = await this.natsService.request(NatsSubjects.USER_METADATA_UPDATE, codec.encode(requestPayload), { - timeout: NATS_CONFIG.REQUEST_TIMEOUT, - }); + const response = await this.natsService.request( + NatsSubjects.USER_METADATA_UPDATE, + codec.encode(requestPayload) + );
225-233: Avoid string matching for timeout/availability detection.Prefer checking a structured error property (e.g., error.code) from NATS errors instead of message.includes('timeout'|'503').
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
87-98: Align FE validators with BE limits.Server-side checks allow up to 100/200 chars on some fields; FE uses 50/100/20. Consider centralizing constraints in shared constants to avoid drift.
Also applies to: 90-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts(2 hunks)apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html(1 hunks)apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.html(7 hunks)apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts(6 hunks)apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/user.service.ts(2 hunks)apps/lfx-one/src/server/controllers/profile.controller.ts(2 hunks)apps/lfx-one/src/server/errors/index.ts(1 hunks)apps/lfx-one/src/server/routes/profile.route.ts(1 hunks)apps/lfx-one/src/server/services/nats.service.ts(1 hunks)apps/lfx-one/src/server/services/project.service.ts(3 hunks)apps/lfx-one/src/server/services/supabase.service.ts(3 hunks)apps/lfx-one/src/server/services/user.service.ts(1 hunks)packages/shared/src/enums/index.ts(1 hunks)packages/shared/src/enums/nats.enum.ts(1 hunks)packages/shared/src/interfaces/index.ts(0 hunks)packages/shared/src/interfaces/member.interface.ts(1 hunks)packages/shared/src/interfaces/project.interface.ts(1 hunks)packages/shared/src/interfaces/user-profile.interface.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/shared/src/interfaces/index.ts
🧰 Additional context used
📓 Path-based instructions (7)
packages/shared/src/enums/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all enums in the shared package at packages/shared/src/enums
Files:
packages/shared/src/enums/nats.enum.tspackages/shared/src/enums/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
packages/shared/src/enums/nats.enum.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/routes/profile.route.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/server/services/user.service.tspackages/shared/src/interfaces/user-profile.interface.tsapps/lfx-one/src/app/shared/services/user.service.tspackages/shared/src/interfaces/member.interface.tspackages/shared/src/enums/index.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/errors/index.tsapps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/supabase.service.tsapps/lfx-one/src/server/services/nats.service.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
packages/shared/src/enums/nats.enum.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/routes/profile.route.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/server/services/user.service.tspackages/shared/src/interfaces/user-profile.interface.tsapps/lfx-one/src/app/shared/services/user.service.tspackages/shared/src/interfaces/member.interface.tspackages/shared/src/enums/index.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/errors/index.tsapps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.tsapps/lfx-one/src/app/layouts/project-layout/project-layout.component.htmlapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.htmlapps/lfx-one/src/server/services/supabase.service.tsapps/lfx-one/src/server/services/nats.service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
packages/shared/src/enums/nats.enum.tsapps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.tsapps/lfx-one/src/server/routes/profile.route.tspackages/shared/src/interfaces/project.interface.tsapps/lfx-one/src/server/services/user.service.tspackages/shared/src/interfaces/user-profile.interface.tsapps/lfx-one/src/app/shared/services/user.service.tspackages/shared/src/interfaces/member.interface.tspackages/shared/src/enums/index.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/server/errors/index.tsapps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.tsapps/lfx-one/src/server/services/project.service.tsapps/lfx-one/src/server/services/supabase.service.tsapps/lfx-one/src/server/services/nats.service.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all TypeScript interfaces in the shared package at packages/shared/src/interfaces
Files:
packages/shared/src/interfaces/project.interface.tspackages/shared/src/interfaces/user-profile.interface.tspackages/shared/src/interfaces/member.interface.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use barrel exports; always use direct imports for standalone components
Files:
packages/shared/src/enums/index.tsapps/lfx-one/src/server/errors/index.ts
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.htmlapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.html
🧠 Learnings (1)
📚 Learning: 2025-09-16T03:32:46.518Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T03:32:46.518Z
Learning: Applies to packages/shared/src/enums/**/*.ts : Place all enums in the shared package at packages/shared/src/enums
Applied to files:
packages/shared/src/enums/index.ts
🧬 Code graph analysis (7)
apps/lfx-one/src/server/services/user.service.ts (3)
apps/lfx-one/src/server/services/nats.service.ts (2)
NatsService(13-133)request(31-49)packages/shared/src/interfaces/user-profile.interface.ts (2)
UserMetadataUpdateRequest(149-153)UserMetadataUpdateResponse(158-164)packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)
apps/lfx-one/src/app/shared/services/user.service.ts (1)
packages/shared/src/interfaces/user-profile.interface.ts (1)
ProfileUpdateRequest(142-144)
apps/lfx-one/src/server/controllers/profile.controller.ts (2)
apps/lfx-one/src/server/services/user.service.ts (1)
UserService(15-244)packages/shared/src/interfaces/user-profile.interface.ts (1)
UserMetadataUpdateRequest(149-153)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
packages/shared/src/interfaces/user-profile.interface.ts (2)
UserMetadata(121-136)ProfileUpdateRequest(142-144)
apps/lfx-one/src/server/services/project.service.ts (3)
packages/shared/src/interfaces/project.interface.ts (1)
ProjectSlugToIdResponse(142-146)packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)apps/lfx-one/src/server/server.ts (1)
serverLogger(318-318)
apps/lfx-one/src/server/services/supabase.service.ts (1)
packages/shared/src/interfaces/user-profile.interface.ts (1)
UserMetadata(121-136)
apps/lfx-one/src/server/services/nats.service.ts (1)
packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (20)
packages/shared/src/interfaces/project.interface.ts (1)
139-146: LGTM! Well-designed interface for project slug resolution.The
ProjectSlugToIdResponseinterface follows OIDC-compliant patterns and provides clear semantics for the project slug-to-id resolution flow. The three fields (projectId,slug,exists) give consumers both the resolved ID and existence confirmation.packages/shared/src/interfaces/user-profile.interface.ts (2)
118-136: LGTM! OIDC-compliant field names improve interoperability.The
UserMetadatainterface adopts standard OIDC claim names (given_name,family_name,job_title,state_province,postal_code,t_shirt_size), which aligns with the PR objectives (LFXV2-633) and ensures compatibility with OIDC-based auth services.
138-164: Breaking change verified: no remaining references
No occurrences of ProfileDetails, UpdateUserProfileRequest, UpdateProfileDetailsRequest, or legacy profile fields in consuming code.packages/shared/src/enums/index.ts (1)
7-7: LGTM! Follows the established pattern for enum exports.The export of
nats.enumconsolidates NATS-related enums in the shared package barrel, making the newUSER_METADATA_UPDATEsubject accessible to consumers throughout the codebase.Based on learnings.
apps/lfx-one/src/app/layouts/profile-layout/profile-layout.component.ts (2)
148-148: LGTM! Field name updated to OIDC-compliantjob_title.The change from
profile.profile.titletoprofile.profile.job_titlealigns with the OIDC standard claim names defined in theUserMetadatainterface.
159-159: LGTM! Field name updated to OIDC-compliantstate_province.The change from
profile.profile.statetoprofile.profile.state_provincecorrectly implements the OIDC-standard field naming for the location assembly logic.packages/shared/src/interfaces/member.interface.ts (1)
4-4: LGTM! Import refactored to use barrel export.The change from a specific module import to the barrel export
'../enums'aligns with the PR's consolidation objectives (LFXV2-636) and improves import consistency across the shared package.packages/shared/src/enums/nats.enum.ts (1)
9-9: New NATS subject looks goodSubject naming and placement align with the shared enums consolidation. No issues.
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.html (1)
40-41: OIDC field renames are consistentBindings match UserMetadata: given_name, family_name, job_title, state_province, postal_code, t_shirt_size. Looks correct.
Please verify:
- profile-edit.component.ts form controls use these exact names and validators.
- The backend update payload expects these OIDC fields and returns the same shape on load.
Also applies to: 54-55, 100-101, 170-171, 182-183, 225-226, 272-273
apps/lfx-one/src/server/services/project.service.ts (1)
116-137: LGTM on delegating slug resolution to a private helperClear separation of concerns and logging. Good use of NATS_CONFIG timeout and subject enum.
apps/lfx-one/src/server/routes/profile.route.ts (1)
20-21: Unified PATCH /api/profile wiring and auth verified ProfileController.updateUserMetadata is defined/exported and profileRouter is mounted behind authMiddleware.apps/lfx-one/src/server/controllers/profile.controller.ts (9)
4-4: LGTM! Imports align with new architecture.The new imports (UserMetadataUpdateRequest, error classes, UserService) support the NATS-based update flow and improved error handling as intended by the refactor.
Also applies to: 7-7, 10-10
17-18: LGTM! Service dependencies properly initialized.Both SupabaseService and UserService are instantiated as class properties, following a consistent pattern for service dependencies.
98-109: LGTM! Token extraction supports multiple auth sources.The token extraction correctly handles both bearer tokens and OIDC access tokens with appropriate fallback, and validates token presence before proceeding.
112-123: LGTM! Username extraction follows established pattern.Username extraction and validation are consistent with other methods in this controller, using getUsernameFromAuth helper and ServiceValidationError for missing authentication.
126-139: LGTM! Body validation enforces required update data.The validation correctly ensures that user_metadata is provided in the request body, preventing empty update requests from reaching the service layer.
142-156: LGTM! Metadata validation properly delegated to service layer.The validation logic is correctly delegated to UserService, which performs comprehensive field validation (t-shirt sizes, phone numbers, postal codes, URLs, and length constraints). Validation errors are properly caught and wrapped in ServiceValidationError.
159-166: LGTM! Service call correctly structured.The update request is properly constructed with required fields (token, username, user_metadata) and the service call passes the request object for logging context, aligning with the NATS-based update pattern.
169-179: LGTM! Success response properly structured.The success path returns appropriate HTTP 200 status with a clear response structure including success flag, message, and updated_fields array for client feedback.
235-239: LGTM! Catch-all error handling follows Express best practices.The try-catch block properly logs unexpected errors and passes them to the next middleware, following standard Express error handling patterns.
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts
Show resolved
Hide resolved
LFXV2-633 LFXV2-636 LFXV2-637 - Remove outdated field mapping comments from profile-edit component - Add proper type imports for ProfileUpdateRequest and UserMetadata - Fix error message accuracy (Username vs User ID) - Replace any type with proper UserMetadata type in validation 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
224-236: Potential data loss: Inconsistent field name handling during migration.The form population logic shows an inconsistency that could cause data loss:
- Lines 224-225 correctly map from old field names (
profile.user.first_name,profile.user.last_name)- Lines 227, 230, 233, 235 expect new field names (
profile.profile?.job_title,profile.profile?.state_province, etc.)This inconsistency suggests the backend response (
CombinedProfile) may still use old field names in theprofileobject, similar to theuserobject. The past review comment on line 227 confirms this concern, suggestingprofile.profile?.titlemight still be returned.If the backend hasn't fully migrated to new field names, these fields will be empty when loading existing profiles.
Apply this diff to add fallback logic for all migrated fields:
this.profileForm.patchValue({ given_name: profile.user.first_name || '', family_name: profile.user.last_name || '', username: profile.user.username || '', - job_title: profile.profile?.job_title || '', + job_title: profile.profile?.job_title || profile.profile?.title || '', organization: profile.profile?.organization || '', country: countryValue, - state_province: profile.profile?.state_province || '', + state_province: profile.profile?.state_province || profile.profile?.state || '', city: profile.profile?.city || '', address: profile.profile?.address || '', - postal_code: profile.profile?.postal_code || '', + postal_code: profile.profile?.postal_code || profile.profile?.zipcode || '', phone_number: profile.profile?.phone_number || '', - t_shirt_size: profile.profile?.t_shirt_size || '', + t_shirt_size: profile.profile?.t_shirt_size || profile.profile?.tshirt_size || '', });Additionally, verify the backend migration status:
#!/bin/bash # Description: Check CombinedProfile interface and backend response structure to confirm field names # Search for CombinedProfile interface definition echo "=== CombinedProfile Interface Definition ===" rg -nP --type=ts -A 20 'interface\s+CombinedProfile\b' # Search for UserProfile interface that might be part of CombinedProfile echo -e "\n=== UserProfile Interface Definition ===" rg -nP --type=ts -A 30 'interface\s+UserProfile\b' # Search for any backend response handling that maps old to new field names echo -e "\n=== Field Name Mappings ===" rg -nP --type=ts -C 3 '(title|state|zipcode|tshirt_size).*(?:job_title|state_province|postal_code|t_shirt_size)' # Check if there are DTOs or response types for the profile endpoint echo -e "\n=== Profile Response/DTO Types ===" ast-grep --pattern 'export interface $NAME { $$$ profile?: $TYPE $$$ }'apps/lfx-one/src/server/services/user.service.ts (2)
28-90: Missing metadata validation before NATS request.The
validateUserMetadatamethod is defined (lines 97-164) but never invoked. Validation should occur before sending the update request via NATS to fail fast on invalid input.Apply this diff to add validation:
req.log.info( { has_username: !!updates.username, has_metadata: !!updates.user_metadata, metadata_fields: updates.user_metadata ? Object.keys(updates.user_metadata) : [], }, 'Attempting to update user metadata' ); + // Validate user metadata if provided + if (updates.user_metadata) { + this.validateUserMetadata(updates.user_metadata); + } + // Send the request via NATS const response = await this.sendUserMetadataUpdate(updates);
28-90: Express.Request missing log property augmentation.The code uses
req.log(lines 40, 54, 62, 74), but according to the past review,apps/lfx-one/src/types/express.d.tsonly declaresbearerToken. You must extendExpress.Requestwith alogproperty to make TypeScript compile.Add the following to
apps/lfx-one/src/types/express.d.ts:import type { Logger } from 'pino'; declare global { namespace Express { interface Request { bearerToken?: string; log: Logger; } } }apps/lfx-one/src/server/controllers/profile.controller.ts (1)
184-231: String-based error categorization remains fragile.The error categorization logic (lines 194, 202, 209, 216) uses case-sensitive
.includes()checks onresponse.error, which is brittle:
- Case sensitivity: "Authentication failed" vs "authentication failed" won't match
- Ambiguity: Substring matches can cause false positives
- Maintainability: No structured error codes for reliable categorization
Consider extending the NATS contract to include structured error codes:
In packages/shared/src/interfaces/user-profile.interface.ts:
export enum UserMetadataErrorCode { SERVICE_UNAVAILABLE = 'SERVICE_UNAVAILABLE', AUTHENTICATION_FAILED = 'AUTHENTICATION_FAILED', AUTHORIZATION_FAILED = 'AUTHORIZATION_FAILED', NOT_FOUND = 'NOT_FOUND', VALIDATION_FAILED = 'VALIDATION_FAILED', INTERNAL_ERROR = 'INTERNAL_ERROR', } export interface UserMetadataUpdateResponse { success: boolean; username: string; message?: string; updated_fields?: string[]; error?: string; error_code?: UserMetadataErrorCode; // Add this field }In this file (lines 184-231), use switch on error_code:
let error: any; switch (response.error_code) { case UserMetadataErrorCode.SERVICE_UNAVAILABLE: error = new MicroserviceError(/*...*/); break; case UserMetadataErrorCode.AUTHENTICATION_FAILED: error = new AuthenticationError(/*...*/); break; case UserMetadataErrorCode.AUTHORIZATION_FAILED: error = new AuthorizationError(/*...*/); break; case UserMetadataErrorCode.NOT_FOUND: error = new ResourceNotFoundError(/*...*/); break; case UserMetadataErrorCode.VALIDATION_FAILED: error = ServiceValidationError.forField(/*...*/); break; default: error = new MicroserviceError(/*...*/); }If adding
error_codeisn't immediately feasible, at minimum normalize with.toLowerCase()and use word boundaries:const errorLower = response.error?.toLowerCase() || ''; if (/\bauthentication\b/.test(errorLower) || /\btoken\b/.test(errorLower)) { error = new AuthenticationError(/*...*/); } else if (/\bpermission\b/.test(errorLower) || /\bforbidden\b/.test(errorLower)) { error = new AuthorizationError(/*...*/); } // ... etc
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
155-158: Remove unnecessary type assertion.The type cast
as UserMetadataon line 157 is unnecessary because:
userMetadatais already correctly typed asPartial<UserMetadata>- All fields in
UserMetadatainterface are optional- The type assertion bypasses TypeScript's type checking without adding value
Apply this diff to simplify the code:
- // Prepare update data - only send user_metadata const updateData: ProfileUpdateRequest = { - user_metadata: userMetadata as UserMetadata, + user_metadata: userMetadata, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts(6 hunks)apps/lfx-one/src/server/controllers/profile.controller.ts(2 hunks)apps/lfx-one/src/server/services/user.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/services/user.service.tsapps/lfx-one/src/server/controllers/profile.controller.tsapps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts
🧬 Code graph analysis (3)
apps/lfx-one/src/server/services/user.service.ts (3)
apps/lfx-one/src/server/services/nats.service.ts (2)
NatsService(13-133)request(31-49)packages/shared/src/interfaces/user-profile.interface.ts (3)
UserMetadataUpdateRequest(149-153)UserMetadataUpdateResponse(158-164)UserMetadata(121-136)packages/shared/src/constants/api.constants.ts (1)
NATS_CONFIG(28-43)
apps/lfx-one/src/server/controllers/profile.controller.ts (2)
apps/lfx-one/src/server/services/user.service.ts (1)
UserService(15-242)packages/shared/src/interfaces/user-profile.interface.ts (2)
ProfileUpdateRequest(142-144)UserMetadataUpdateRequest(149-153)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (1)
packages/shared/src/interfaces/user-profile.interface.ts (2)
UserMetadata(121-136)ProfileUpdateRequest(142-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (10)
apps/lfx-one/src/app/modules/profile/edit/profile-edit.component.ts (3)
9-9: LGTM: Import statements align with refactored interfaces.The imports correctly reference the new unified
ProfileUpdateRequestandUserMetadatainterfaces from the shared package, supporting the migration to OIDC-compliant field naming.Also applies to: 19-19
81-98: LGTM: Form controls correctly use OIDC-compliant field names.The form definition properly aligns with the
UserMetadatainterface, using the new field names (given_name,family_name,job_title,state_province,postal_code,t_shirt_size) with appropriate validators.
109-112: LGTM: State clearing logic correctly handles field name migration.The logic appropriately clears
state_provincewhen the country is not "United States", preventing invalid state/country combinations using the updated field name.apps/lfx-one/src/server/services/user.service.ts (5)
1-10: LGTM: License header and imports are correct.The required license header is present, and all imports are properly organized.
12-20: LGTM: Clean service initialization.The constructor properly initializes the NatsService dependency.
97-164: LGTM: Comprehensive validation logic.The validation method properly checks all metadata fields with appropriate constraints:
- T-shirt sizes validated against allowed values
- Phone and postal code formats validated with regex
- Picture URL validated using URL constructor
- Field lengths checked against reasonable limits
The throw-on-error pattern is appropriate for validation methods.
169-172: LGTM: Proper resource cleanup.The shutdown method correctly delegates to the NATS service for cleanup.
178-241: LGTM: Robust NATS communication with proper error handling.The method correctly:
- Encodes the request payload
- Sends via NATS with timeout
- Decodes and parses the response
- Handles timeout/unavailability errors specifically
- Returns appropriate error responses
The string-based error detection (line 224) is acceptable for this internal method since it's handling NATS-specific errors.
apps/lfx-one/src/server/controllers/profile.controller.ts (2)
4-4: LGTM: Proper imports and service integration.The new imports for
ProfileUpdateRequest,UserMetadataUpdateRequest,AuthorizationError, andUserServiceare correctly added and the service is properly initialized.Also applies to: 7-7, 10-10, 18-18
91-164: LGTM: Comprehensive validation and request preparation.The method correctly:
- Extracts authentication token from multiple sources
- Validates all required fields (token, username, user_metadata)
- Invokes
userService.validateUserMetadatabefore sending the update (lines 142-156)- Constructs proper error responses with appropriate context
- Prepares the NATS request payload
Summary
Changes
User Profile Interface Refactoring
NATS Integration
lfx.auth-service.user_metadata.updateService Architecture
Interface Consolidation
Error Handling Improvements
Frontend Updates
JIRA Tickets
Test Plan
Migration Notes