feat(search): implement user search for meeting registrants #110
Conversation
- LFXV2-622: Implement backend user search API - Created SearchService for querying meeting registrants and committee members - Added search controller and routes at /api/search/users - Integrated with query microservice for resource fetching - LFXV2-623: Create reusable user search component - Built Angular component with autocomplete and debounced search - Populates parent form controls on user selection - Includes manual entry option in footer - LFXV2-624: Enhance autocomplete component with template support - Added custom item and footer template support - Fixed template context passing for proper data binding - LFXV2-625: Integrate user search into registrant form - Added toggle between search and manual entry modes - Fixed email validation on user selection - Styled with professional divider and transitions Signed-off-by: Asitha de Silva <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches❌ 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 comprehensive user search functionality for meeting registrants, allowing users to search for existing registrants or committee members and auto-populate form fields, with a fallback option for manual entry.
- Created backend search service and API endpoints for querying users across meeting registrants and committee members
- Built reusable frontend user search component with debounced autocomplete functionality
- Enhanced the registrant form to toggle between search mode and manual entry with proper state management
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/utils/form.utils.ts | Added utility functions for form control validation updates with configurable parameters |
| packages/shared/src/interfaces/search.interface.ts | Defined TypeScript interfaces for user search functionality including request/response structures |
| apps/lfx-one/src/server/services/search.service.ts | Implemented backend search service to query and transform user data from microservices |
| apps/lfx-one/src/server/server.ts | Registered new search routes in the Express application |
| apps/lfx-one/src/server/routes/search.route.ts | Created API routing for user search endpoints |
| apps/lfx-one/src/server/controllers/search.controller.ts | Implemented search controller with input validation and error handling |
| apps/lfx-one/src/server/controllers/public-meeting.controller.ts | Enhanced logging and error handling for meeting data retrieval |
| apps/lfx-one/src/app/shared/services/search.service.ts | Created frontend service to interact with user search API endpoints |
| apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts | Built reusable user search component with debouncing and form integration |
| apps/lfx-one/src/app/shared/components/user-search/user-search.component.html | Template for user search autocomplete with custom item and footer templates |
| apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts | Added debouncing to existing organization search for consistency |
| apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts | Enhanced autocomplete component to support custom item and footer templates |
| apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.html | Added template support for custom item and footer rendering |
| apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts | Integrated user search functionality with toggle between search and manual entry modes |
| apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html | Updated template to support both search interface and individual form fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts
Outdated
Show resolved
Hide resolved
✅ 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: 6
🧹 Nitpick comments (14)
packages/shared/src/utils/form.utils.ts (1)
21-29: Consider triggering form-level validators and handling nested controls
- After updating each control, call
form.updateValueAndValidity({ onlySelf, emitEvent })
to run any FormGroup-level validators (e.g.{ validators: futureDateTimeValidator() }inmeeting-manage.component.ts).- Current logic only iterates one level down—nested FormArrays/FormGroups (e.g.
new FormArray([])inmeeting-manage.component.ts) won’t have their child controls updated. Introduce a recursive traversal if you need to validate those too.apps/lfx-one/src/app/shared/components/user-search/user-search.component.html (1)
19-44: Add data-testid attributes to template sections.Consider adding
data-testidattributes to the main container divs in the item, empty, and footer templates for better test coverage and component verification.As per coding guidelines.
Example additions:
<ng-template #item let-user> - <div class="w-full flex justify-between items-center gap-2"> + <div class="w-full flex justify-between items-center gap-2" data-testid="user-search-item"> <div class="flex flex-col"><ng-template #empty> - <div class="p-3 text-center"> + <div class="p-3 text-center" data-testid="user-search-empty"> <p class="text-sm text-gray-600">No users found</p><ng-template #footer> - <div class="p-2 border-t border-gray-200 text-center"> + <div class="p-2 border-t border-gray-200 text-center" data-testid="user-search-footer"> <button type="button" class="text-sm text-primary hover:underline" data-testid="user-search-manual-entry-button" (click)="onEnterManually()">Enter details manually</button>apps/lfx-one/src/app/shared/services/search.service.ts (2)
18-32: Consider surfacing errors instead of silently returning empty arrays.The current implementation catches errors and returns an empty array, making it impossible for callers to distinguish between "no results found" and "an error occurred." This can mask issues like network failures or authorization problems.
Consider either:
- Re-throwing errors after logging so callers can handle them appropriately
- Returning a discriminated union or result type that includes error state
- Using a centralized logging service instead of
console.errorExample discriminated union approach:
// Option 1: Let errors propagate public searchUsers(name: string, type: 'committee_member' | 'meeting_registrant'): Observable<UserSearchResult[]> { if (!name || !type) { return of([]); } const params = new HttpParams().set('name', name).set('type', type).set('limit', '20'); return this.http.get<UserSearchResponse>('/api/search/users', { params }).pipe( map((response) => response.results || []) // Let error propagate to caller ); }
37-50: Same error handling concern applies here.This method has the same silent error swallowing issue as
searchUsers. Consider the same refactoring approaches mentioned in the previous comment.Additionally, consider extracting the default limit (20) and offset (0) as class constants to avoid duplication.
private readonly DEFAULT_LIMIT = 20; private readonly DEFAULT_OFFSET = 0; public searchUsersAdvanced(params: UserSearchParams): Observable<UserSearchResponse> { const httpParams = new HttpParams() .set('name', params.name) .set('type', params.type) .set('limit', (params.limit || this.DEFAULT_LIMIT).toString()) .set('offset', (params.offset || this.DEFAULT_OFFSET).toString()); return this.http.get<UserSearchResponse>('/api/search/users', { params: httpParams }); // Let errors propagate or handle explicitly }apps/lfx-one/src/server/services/search.service.ts (2)
23-29: Consider aligning default limit with client service.The server-side default limit is 50, while the client-side service (
apps/lfx-one/src/app/shared/services/search.service.tslines 23, 41) uses a default of 20. This inconsistency could cause confusion.Since the client always sends an explicit limit, the server default won't typically be used, but aligning the defaults would make the API contract clearer.
Consider using the same default value (20) on both client and server:
const queryParams = { v: 1, name: params.name, type: params.type, - limit: params.limit || 50, + limit: params.limit || 20, offset: params.offset || 0, };
84-89: Consider clarifying "total" field semantics in documentation.The
totalfield is set toresults.length(the count of returned results), not the total count of matching records in the database. This is consistent with theUserSearchResponseinterface, but callers might expecttotalto represent all matching records across all pages.The current implementation is acceptable since the interface documentation describes it as "Total number of matching users" (which could mean "in this response"). However, if confusion arises, consider:
- Updating interface documentation to clarify this means "results in this response"
- Or implementing true total count if the backend supports it
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts (4)
29-40: Initialize manual mode if registrant is already set pre-subscriptionGuard for the case where the parent sets a non-null registrant before the subscription attaches.
public constructor() { + // Handle initial value eagerly + if (this.registrant()) { + this.showIndividualFields.set(true); + } // If we have an existing registrant, show individual fields immediately // Using toObservable to specifically track only registrant changes
47-52: Run validators after selectionAfter the child populates controls, ensure validators run and UI reflects state.
public handleUserSelection(_user: UserSearchResult): void { // The search component has already populated the form controls // Now show the individual input fields this.showIndividualFields.set(true); - this.form().markAsDirty(); + const form = this.form(); + form.markAsDirty(); + form.markAllAsTouched(); + form.updateValueAndValidity({ emitEvent: false }); }
57-65: Reset preserving host without duplicate writes; reset state cleanlyUse FormGroup.reset with value map to avoid a second setValue; also reset pristine/touched for a clean manual-entry start.
public switchToManualEntry(): void { // Clear the form for manual entry - const hostValue = this.form().get('host')?.value; // Preserve host checkbox state - this.form().reset(); - this.form().get('host')?.setValue(hostValue); + const hostValue = this.form().get('host')?.value ?? false; // Preserve host checkbox state + this.form().reset({ host: hostValue }, { emitEvent: false }); + this.form().markAsPristine(); + this.form().markAsUntouched(); // Show individual fields for manual entry this.showIndividualFields.set(true); }
70-78: DRY the reset logic and keep form state consistent when returning to searchMirror the improvements from manual-entry and consider extracting a small helper.
public backToSearch(): void { // Reset form but preserve host checkbox - const hostValue = this.form().get('host')?.value; - this.form().reset(); - this.form().get('host')?.setValue(hostValue); + const hostValue = this.form().get('host')?.value ?? false; + this.form().reset({ host: hostValue }, { emitEvent: false }); + this.form().markAsPristine(); + this.form().markAsUntouched(); // Switch back to search mode this.showIndividualFields.set(false); }apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts (4)
24-26: Reuse central type for searchTypeAvoid duplicating the union; tie it to the shared interface to keep types in sync. As per coding guidelines.
- public searchType = input.required<'committee_member' | 'meeting_registrant'>(); + public searchType = input.required<UserSearchResult['type']>();
91-103: Guard against redundant setValue in effectAvoid unnecessary writes by checking current value before setting.
- if (emailControlValue && emailControlValue.trim()) { - this.userSearchForm.get('userSearch')?.setValue(emailControlValue, { emitEvent: false }); - } + if (emailControlValue && emailControlValue.trim()) { + const ctrl = this.userSearchForm.get('userSearch'); + if (ctrl?.value !== emailControlValue) { + ctrl?.setValue(emailControlValue, { emitEvent: false }); + } + }
165-185: Reset form state on clearAlso reset pristine/touched and validators so UX returns to a clean slate.
public onSearchClear(): void { this.userSearchForm.get('userSearch')?.setValue(''); // Clear all form controls if they are specified const parentForm = this.form(); @@ if (controlName && parentForm.get(controlName)) { parentForm.get(controlName)?.setValue(null); } }); + parentForm.markAsPristine(); + parentForm.markAsUntouched(); + parentForm.updateValueAndValidity({ emitEvent: false }); }
56-72: Unify debouncing to avoid double‐latency (user-search.component.ts)
The wrapper<lfx-autocomplete>defaults to[delay]=300msand[minLength]=1while yourvalueChangespipeline also doesdebounceTime(300)and filters out<2chars, stacking ~600ms before any request. Choose one:
- Option A: drop the Rx
debounceTime(300)inuser-search.component.tsand rely on the Autocomplete’sdelay/minLength.- Option B: disable the PrimeNG delay by calling
<lfx-autocomplete [delay]="0" [minLength]="2">and keep your RxdebounceTime(300)/threshold.
📜 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 (15)
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.html(1 hunks)apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts(2 hunks)apps/lfx-one/src/app/shared/components/user-search/user-search.component.html(1 hunks)apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/search.service.ts(1 hunks)apps/lfx-one/src/server/controllers/public-meeting.controller.ts(1 hunks)apps/lfx-one/src/server/controllers/search.controller.ts(1 hunks)apps/lfx-one/src/server/routes/search.route.ts(1 hunks)apps/lfx-one/src/server/server.ts(2 hunks)apps/lfx-one/src/server/services/search.service.ts(1 hunks)packages/shared/src/interfaces/search.interface.ts(1 hunks)packages/shared/src/utils/form.utils.ts(1 hunks)
🔥 Files not summarized due to errors (2)
- apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts: Error: Server error: no LLM provider could handle the message
- apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/routes/search.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/app/shared/components/organization-search/organization-search.component.tsapps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.tsapps/lfx-one/src/app/shared/services/search.service.tspackages/shared/src/interfaces/search.interface.tsapps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.tsapps/lfx-one/src/server/server.tspackages/shared/src/utils/form.utils.tsapps/lfx-one/src/app/shared/components/user-search/user-search.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/routes/search.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/app/shared/components/user-search/user-search.component.htmlapps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.htmlapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/app/shared/components/organization-search/organization-search.component.tsapps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.tsapps/lfx-one/src/app/shared/services/search.service.tspackages/shared/src/interfaces/search.interface.tsapps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.tsapps/lfx-one/src/server/server.tspackages/shared/src/utils/form.utils.tsapps/lfx-one/src/app/shared/components/user-search/user-search.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/routes/search.route.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/server/controllers/public-meeting.controller.tsapps/lfx-one/src/app/shared/components/organization-search/organization-search.component.tsapps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.tsapps/lfx-one/src/app/shared/services/search.service.tspackages/shared/src/interfaces/search.interface.tsapps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.tsapps/lfx-one/src/server/server.tspackages/shared/src/utils/form.utils.tsapps/lfx-one/src/app/shared/components/user-search/user-search.component.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/shared/components/user-search/user-search.component.htmlapps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.htmlapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html
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/search.interface.ts
🧬 Code graph analysis (6)
apps/lfx-one/src/server/routes/search.route.ts (1)
apps/lfx-one/src/server/controllers/search.controller.ts (1)
SearchController(14-101)
apps/lfx-one/src/server/controllers/search.controller.ts (2)
apps/lfx-one/src/server/services/search.service.ts (1)
SearchService(12-90)packages/shared/src/interfaces/search.interface.ts (1)
UserSearchParams(83-92)
apps/lfx-one/src/app/shared/services/search.service.ts (2)
apps/lfx-one/src/server/services/search.service.ts (1)
SearchService(12-90)packages/shared/src/interfaces/search.interface.ts (3)
UserSearchResult(48-77)UserSearchResponse(98-105)UserSearchParams(83-92)
apps/lfx-one/src/server/services/search.service.ts (4)
packages/shared/src/interfaces/search.interface.ts (3)
UserSearchParams(83-92)UserSearchResponse(98-105)UserSearchResult(48-77)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)packages/shared/src/interfaces/meeting.interface.ts (1)
MeetingRegistrant(260-301)packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(10-64)
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts (4)
apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts (1)
Component(16-161)apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts (1)
Component(15-198)packages/shared/src/interfaces/meeting.interface.ts (1)
MeetingRegistrant(260-301)packages/shared/src/interfaces/search.interface.ts (1)
UserSearchResult(48-77)
apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts (4)
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts (1)
Component(15-79)apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts (1)
Component(9-48)apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts (1)
Component(16-161)packages/shared/src/interfaces/search.interface.ts (1)
UserSearchResult(48-77)
⏰ 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 (16)
apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts (1)
50-53: Nice debounce addition.This 300 ms debounce smooths out keystrokes and trims redundant service calls while keeping the existing flow intact.
apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts (2)
17-18: LGTM! Template support properly implemented.The new
itemTemplateandfooterTemplateContentChild declarations follow the same pattern as the existingemptyTemplate, ensuring consistency and proper content projection.
27-27: Good addition for debounced search.The
delayinput with a default of 300ms provides sensible debouncing for search operations, aligning with the PR's requirement for debounced autocomplete.apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.html (2)
29-32: LGTM! Item template context properly configured.The item template correctly uses
{ $implicit: item }context, enabling consumers to access the item vialet-itemsyntax in their templates.
34-37: LGTM! Footer template properly implemented.The footer template follows the same conditional rendering pattern as the other templates and enables persistent actions in the autocomplete dropdown.
packages/shared/src/interfaces/search.interface.ts (3)
48-77: LGTM! Well-structured unified user interface.The
UserSearchResultinterface properly combines fields from bothMeetingRegistrantandCommitteeMemberwith appropriate nullable types and clear documentation. The discriminated union using thetypeliteral enables type-safe handling of different user sources.
83-92: LGTM! Clean search parameters interface.The
UserSearchParamsinterface provides clear search criteria with optional pagination support.
98-105: LGTM! Standard API response wrapper.The
UserSearchResponseinterface follows common API pagination patterns with results array and optional metadata fields.apps/lfx-one/src/server/server.ts (2)
27-27: LGTM! Search router properly imported.The import follows the established pattern for other route modules.
207-207: LGTM! Search route properly mounted in protected API section.The search router is correctly mounted at
/api/searchafter authentication middleware and before the error handler, placing it in the protected API routes section. This ensures search functionality requires authentication.apps/lfx-one/src/app/shared/components/user-search/user-search.component.html (2)
19-31: LGTM! Clear user item presentation.The item template effectively displays user information with name, optional organization, and email in a well-structured layout.
33-37: LGTM! Clear empty state message.The empty template provides appropriate user feedback when no results are found.
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html (1)
44-132: LGTM! Individual fields section is well-structured.The individual fields section properly:
- Uses conditional rendering for the back button based on registrant state
- Includes all required form fields with proper bindings
- Shows validation messages conditionally
- Follows the data-testid naming convention as per coding guidelines
- Maintains good accessibility with labels
apps/lfx-one/src/server/services/search.service.ts (1)
39-82: LGTM! Resource transformation is well-implemented.The transformation logic properly:
- Handles both
meeting_registrantandcommittee_membertypes- Maps fields correctly to the
UserSearchResultinterface- Uses appropriate null handling for optional fields (job_title, organization, username)
- Sets type-specific fields correctly (committee only for committee_member)
apps/lfx-one/src/server/routes/search.route.ts (1)
1-14: No duplicate mounting ofsearchRouterin server.ts
ThesearchRouteris imported once and registered exactly once at/api/search(line 207), so no duplicate route handlers exist.apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts (1)
1-3: Headers present and correctLicense and SPDX headers look good.
...e/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/shared/components/user-search/user-search.component.html
Show resolved
Hide resolved
apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts
Show resolved
Hide resolved
- Add email-based search using tags parameter - Support searching by email format detection - Simplify registrant form handleUserSelection method - Update placeholder text for clarity - Enhance search controller validation for name or tags - Remove unused imports and parameters - Clean up unused searchUsersAdvanced method LFXV2-622 LFXV2-623 LFXV2-625 Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html (1)
14-28: Fix searchType to target meeting registrants. The registrant form must query the meeting registrant dataset, butsearchType="committee_member"still points to committee members and breaks the flow—switch it to"meeting_registrant"so the search returns the correct people.apps/lfx-one/src/server/controllers/search.controller.ts (1)
78-85: Validate limit/offset query params before building search params.parseIntcan yieldNaN; we still pass that through tosearchParams. Guard both fields (ensure finite, non-negative numbers) before assignment, otherwise treat them asundefinedor reject the request.apps/lfx-one/src/server/services/search.service.ts (1)
63-82: LGTM!Committee member transformation is correct. The code after line 62 implicitly handles the else case (when
params.type !== 'meeting_registrant'). While the past review suggested an explicitelseclause, it's unnecessary here sinceparams.typeis constrained to two values by the interface, making the control flow clear without it.
🧹 Nitpick comments (1)
apps/lfx-one/src/server/services/search.service.ts (1)
15-17: Consider dependency injection for better testability.The constructor directly instantiates
MicroserviceProxyService, which makes unit testing harder (requires mocking the constructor). Consider accepting the proxy service as a constructor parameter to enable easier mocking and follow dependency injection patterns.Example refactor:
- public constructor() { - this.microserviceProxy = new MicroserviceProxyService(); + public constructor(microserviceProxy?: MicroserviceProxyService) { + this.microserviceProxy = microserviceProxy || new MicroserviceProxyService(); }
📜 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 (8)
apps/lfx-one/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.html(1 hunks)apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/search.service.ts(1 hunks)apps/lfx-one/src/server/controllers/search.controller.ts(1 hunks)apps/lfx-one/src/server/services/search.service.ts(1 hunks)packages/shared/src/interfaces/search.interface.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/lfx-one/src/app/modules/project/meetings/components/meeting-registrants/meeting-registrants.component.html
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/interfaces/search.interface.ts
- apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts
- apps/lfx-one/src/app/shared/services/search.service.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/modules/project/meetings/components/registrant-form/registrant-form.component.html
**/*.{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/app/modules/project/meetings/components/registrant-form/registrant-form.component.htmlapps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.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:
apps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/server/services/search.service.tsapps/lfx-one/src/server/controllers/search.controller.tsapps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts
🧬 Code graph analysis (3)
apps/lfx-one/src/server/services/search.service.ts (4)
packages/shared/src/interfaces/search.interface.ts (3)
UserSearchParams(83-94)UserSearchResponse(100-107)UserSearchResult(48-77)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(58-61)packages/shared/src/interfaces/meeting.interface.ts (1)
MeetingRegistrant(260-301)packages/shared/src/interfaces/member.interface.ts (1)
CommitteeMember(10-64)
apps/lfx-one/src/server/controllers/search.controller.ts (2)
apps/lfx-one/src/server/services/search.service.ts (1)
SearchService(12-91)packages/shared/src/interfaces/search.interface.ts (1)
UserSearchParams(83-94)
apps/lfx-one/src/app/modules/project/meetings/components/registrant-form/registrant-form.component.ts (4)
apps/lfx-one/src/app/shared/components/user-search/user-search.component.ts (1)
Component(15-198)apps/lfx-one/src/app/shared/components/autocomplete/autocomplete.component.ts (1)
Component(9-48)apps/lfx-one/src/app/shared/components/organization-search/organization-search.component.ts (1)
Component(16-161)packages/shared/src/interfaces/meeting.interface.ts (1)
MeetingRegistrant(260-301)
🔇 Additional comments (5)
apps/lfx-one/src/server/services/search.service.ts (5)
1-2: LGTM!License header is present and correctly formatted.
4-7: LGTM!Imports are clean and all appear necessary for the service functionality.
22-30: LGTM!Query parameter construction is clean with appropriate defaults. The hardcoded
v: 1is acceptable but consider extracting to a constant if the API version needs to be managed centrally.
32-38: LGTM!Microservice proxy call is correctly typed and structured.
41-62: LGTM!Meeting registrant transformation logic correctly maps all fields to the unified
UserSearchResultformat with appropriate null handling.
| return { | ||
| results, | ||
| total: results.length, | ||
| has_more: results.length === queryParams.limit, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Incorrect total calculation breaks pagination semantics.
Line 87 sets total: results.length, but according to the interface definition (packages/shared/src/interfaces/search.interface.ts), total should represent "Total number of matching users" across all pages, not just the current page size. This breaks pagination UIs that need to show "X of Y results" or calculate page counts.
The query microservice response (QueryServiceResponse) does not appear to include a total count field. You need to either:
- Request the total count from the backend (if available via a separate field or query), or
- Document that
totalis not available and useundefinedinstead ofresults.length.
Additionally, consider whether has_more can be reliably determined without a true total count.
Verify the query microservice response structure:
🏁 Script executed:
#!/bin/bash
# Check if QueryServiceResponse includes total count or metadata fields
ast-grep --pattern $'export interface QueryServiceResponse$_$ {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for QueryServiceResponse definition
rg -n "export interface QueryServiceResponse" -t tsLength of output: 103
🏁 Script executed:
#!/bin/bash
# Show full QueryServiceResponse fields
rg -n -C3 "export interface QueryServiceResponse" packages/shared/src/interfaces/api.interface.tsLength of output: 236
Supply a real total count instead of using the page length.
QueryServiceResponse only provides resources (no total‐count field), so total: results.length misrepresents the overall result set. Update the backend call (or issue a separate count query) to return the true total before setting total, and continue using has_more as a page‐full heuristic.
🤖 Prompt for AI Agents
In apps/lfx-one/src/server/services/search.service.ts around lines 85 to 89, the
code sets total to results.length which only reflects the page size rather than
the overall matching count; change the implementation to obtain the real total
from the search backend (extend the QueryServiceResponse to include a totalCount
or call the backend’s count endpoint) or run a separate count query before
returning, then set total to that returned totalCount while keeping has_more as
(results.length === queryParams.limit) as a page-full heuristic; update any
types/interfaces to reflect the new total field and use that value in the return
object.
Description
This PR implements comprehensive user search functionality for meeting registrants, allowing users to search and select existing registrants or enter details manually.
JIRA Tickets
Changes Made
Backend
/api/search/usersFrontend Components
Features
Testing Instructions
Screenshots