T hoguet/issue16 #139
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: nessar/vac-optimizer#139
Reference in new issue
Block a user
requested review from
@CopilotThe mat-select event handler is incorrect. The
valueChangeevent emits the new value directly, not an event object. Change$event.valueto just$event, or use the proper approach by updating the day object and emitting it. The current code will emitundefined.Direct mutation of an input signal's value is not allowed and violates Angular's signal immutability principle. Instead, create a new object with the updated property. Replace lines 93-94 with:
this.dayChange.emit({ ...this.day(), numberOfDays: value });Direct mutation of an input signal's value is not allowed and violates Angular's signal immutability principle. Instead, create a new object with the updated property. Replace lines 98-99 with:
this.dayChange.emit({ ...this.day(), expiryDate: event.value! });Directly mutating the array returned from a signal violates signal immutability. Use the signal's
updatemethod instead:this.userInputService.vacationNumberSignal.update(days => [...days, this.userInputService.createVacationDay()]);Using
anytype defeats TypeScript's type safety. The parameter type should match the actual data structure being used. Based on the code, this should beVacationsNumber(with properties cp, rtt, other), not the newVacDay[]type.Using
anytype defeats TypeScript's type safety. The parameter type should beVacationsNumber(with properties cp, rtt, other) to match how it's used throughout the method.Directly mutating the array returned from a signal violates signal immutability. Use the signal's
updatemethod instead:this.userInputService.vacationNumberSignal.update(days => days.filter((_, i) => i !== index));Type mismatch:
vacationDataisVacDay[]from the signal, butoptimizeVacationsexpects an object with{cp: number, rtt: number, other: number}properties. The spread operator{ ...vacationData }will create an object with numeric keys, not the expected structure. You need to transform theVacDay[]into the expected format before callingoptimizeVacations, or updateoptimizeVacationsto accept the new type.This test will fail because the component has a required input
daythat is not provided. You need to either provide the required input usingfixture.componentRef.setInput('day', mockVacDay)beforedetectChanges(), or addprovideAnimationsAsync()to the providers array since the component uses Material components that require animations.Empty lifecycle hook. Since
ngOnInithas no implementation, it should be removed along with theOnInitinterface from the class declaration (line 35).Unused import.
ReactiveFormsModuleis no longer used after removing the form-based implementation. This import should be removed from both the import statement and the component's imports array (line 23).Unused import and variable. The
destroy$Subject (line 38) is created and completed inngOnDestroy, but it's never actually used (no.pipe(takeUntil(this.destroy$))calls remain after form removal). Both the import and thedestroy$property can be removed, along with theOnDestroyinterface andngOnDestroymethod.Review: Commented
Pull request overview
This PR refactors the vacation management system from a simple count-based approach to a more structured day-tracking system with expiry dates, and upgrades Angular from v20 to v21. Key changes include:
VacationsNumber(simple object with cp/rtt/other counts) toVacDay[](array of vacation day objects with IDs, types, counts, and expiry dates)DayInputcomponent for managing individual vacation day entriesReviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
Comments suppressed due to low confidence (1)
src/app/components/user-input/user-input.spec.ts:40
component.userInputFormwhich no longer exists after the refactoring. The component now usesdaysOffsignal instead of a form. These tests should be updated to test the new implementation: verifydaysOffis defined, testaddDay()andremoveFromList()methods.requested review from
@Copilot@copilotopen a new pull request to apply changes based on the comments in this threadmentioned in merge request !62
@THoguetI've opened a new pull request, #62, to work on those changes. Once the pull request is ready, I'll request review from you.Input type should be "text" or "date", not "datetime". The "datetime" type is not a valid HTML input type. For date-only pickers (which this appears to be), use:
Missing
dayChangeoutput binding fromapp-day-input. TheDayInputcomponent emits changes through thedayChangeoutput, but this parent component doesn't listen to it. Without handling this event, changes made in the child component won't be properly synchronized. Add the binding:However, note that with the current direct mutation pattern in the component, you may not need this if you address the signal mutation issues identified in other comments.
Direct mutation of input signal object. Input signals return readonly values and should not be mutated directly. Instead, emit a new object with the updated value through
dayChangeoutput.Consider this approach:
[nitpick] Commented-out code should be removed. If the
mat-list-itemwrapper was removed intentionally, the closing comment should be deleted as well.Empty
mat-optionwithout value serves no clear purpose. If this is meant to be a "clear selection" option, it should have a descriptive label. If not needed, remove it:Direct mutation of signal array. The code mutates
this.daysOffwhich is the result of calling a signal (this.userInputService.vacationNumberSignal()), but this doesn't trigger signal updates. The mutation affects the underlying array but Angular's change detection won't be notified.This issue is partially acknowledged in
triggerComputation()(line 80), but the pattern of mutating and then manually triggering updates is error-prone. Consider usingupdate()instead:Empty
ngOnInit()method serves no purpose and should be removed. If theOnInitlifecycle hook isn't needed, also removeOnInitfrom the implements clause and the import.[nitpick] Commented-out code should be removed. If the
mat-list-itemwrapper was removed intentionally, the comment should be deleted as well.Type casting
day.type as unknown as DayTypeindicates a design issue. SinceSelectableDayTypeis a subset ofDayType(with values CP, RTT, OTHER), the type system should handle this naturally. Consider redesigning the types to avoid unsafe casting, or if the relationship is valid, use a direct cast:day.type as DayType.The
destroy$Subject is defined but never used sincengOnInit()no longer subscribes to observables. If not needed for cleanup, remove bothdestroy$and theOnDestroyimplementation.Select dropdown for day type is missing an accessible label. Screen readers won't be able to describe this field to users. Add a
<mat-label>:::ng-deepis deprecated and should be avoided. It pierces component encapsulation and can cause unexpected styling issues. Consider using Angular's host context or moving this style to a global stylesheet if it needs to affect Material components. Alternatively, use ViewEncapsulation.None if you need to style child components, though this is also not recommended.Unused import in component's imports array.
ReactiveFormsModuleshould be removed from the imports array since reactive forms are no longer used in this component.Direct mutation of input signal object and non-null assertion without validation. Input signals should not be mutated directly, and
event.valuemay be null for invalid dates.Consider this approach:
Direct mutation of signal array. Similar to
addDay(), this mutates the signal's underlying array without triggering updates. Useupdate()instead:The test will fail because
DayInputhas a required inputdaythat is not provided. The component requires aVacDayobject to be passed as input.Add the required input:
Missing test provider for animations. The
DayInputcomponent uses Material components that depend on animations. AddprovideAnimationsAsync()to the test configuration to avoid console warnings and potential test failures:Shallow copy doesn't deep copy the
expiryDateDate objects. SinceoptimizeVacations()modifiesnumberOfDayson the vacation days objects, and those objects contain mutableDateobjects, a shallow spread may not be sufficient if dates are mutated. While the current code appears to only mutatenumberOfDays, the comment claims "deep copy" but provides shallow copy.Consider using a proper deep copy or clarify the comment:
Missing tests for the component's key behaviors. The test suite only verifies component creation but doesn't test the
onInput()andaddEvent()methods, or the output events (dayChange,deleteEntry). Given the repository has comprehensive test coverage for other components and services, consider adding tests for:dayChangeeventdayChangeeventdeleteEntryeventThe
valueChangeevent emits only the selected value, not an object with avalueproperty. Accessing$event.valuewill result inundefined. Also, emitting just the type value instead of updating the entireVacDayobject doesn't align with how other changes are handled.Use:
Or better, follow the pattern of not mutating inputs:
No validation for negative or invalid number inputs. The HTML has
min="0", but theNumber()conversion can still produce negative values orNaNif the user enters invalid text. Consider adding validation:Type casting
vacDay.type as unknown as DayTypeindicates a design issue. SinceSelectableDayTypeenum values are defined as equal toDayTypevalues (lines 207-211), a direct cast should work:vacDay.type as DayType. The double cast throughunknownsuggests the types aren't properly aligned.Review: Commented
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 22 comments.
@copilotfix pipelinementioned in merge request !63
@THoguetI've opened a new pull request, #63, to work on those changes. Once the pull request is ready, I'll request review from you.requested review from
@Copilot@copilotfix unit test and lintermentioned in merge request !64
@THoguetI've opened a new pull request, #64, to work on those changes. Once the pull request is ready, I'll request review from you.The
daysOffgetter directly returns a signal instead of its value. This is inconsistent with Angular best practices for signals. Consider either making this a computed signal or calling the signal to return its value, e.g.,return this.userInputService.vacationNumberSignal();The clickable card uses
(click)="addDay()"on amat-cardelement instead of the button inside it. This creates a redundant click handler since the button inside already has keyboard accessibility. The card itself isn't keyboard-accessible unless additional attributes liketabindex="0"and(keydown.enter)or(keydown.space)are added. Consider moving the click handler to just the button, or add proper keyboard event handlers to the card.The number input field lacks a label, making it inaccessible to screen reader users. Consider adding a
mat-labelinside themat-form-fieldto describe what this input represents (e.g., "Number of days").The non-null assertion operator
!is used onevent.value, but there's no guarantee thatevent.valueis non-null. If the user clears the date field,event.valuecould benull, causing a runtime error. Consider handling the null case explicitly, e.g., by not emitting if the value is null, or using a default date.[nitpick] The explicit type annotation
: UserInputServiceis redundant when usinginject(), as TypeScript can infer the type. Consider simplifying to:private userInputService = inject(UserInputService);This line has the same date calculation issue. The duplicated logic appears 4 times in this file (lines 19, 25, 31, and 41). Extract this into a helper method like
private getExpiryDateOneYearFromNow(): Dateto eliminate duplication.Commented-out code should be removed rather than left in the codebase. If this
mat-list-itemwrapper is no longer needed, delete it. If it might be needed in the future, rely on version control to retrieve it.Commented-out code should be removed. See comment on line 1.
The type cast
day.type as unknown as DayTypeis a code smell. SinceSelectableDayTypevalues are derived fromDayType, this cast should be unnecessary. Consider refactoring the type relationship or using a type-safe mapping function.The component has been significantly refactored with new methods (
addDay,removeFromList,updateDay,triggerComputation) and computed signals, but the test file only addsprovideAnimationsAsync(). Consider adding tests to verify the new functionality works correctly.Using
::ng-deepis deprecated and discouraged in Angular. It's a workaround that pierces view encapsulation and can cause global style pollution. Consider using Angular Material's theming system or custom form field appearance options instead to hide the subscript wrapper.The date calculation creates multiple
new Date()objects unnecessarily. The expressionnew Date(new Date().setFullYear(...))first creates a Date, mutates it withsetFullYear(which returns a timestamp), then creates a new Date from that timestamp. This is convoluted. Use a clearer approach:const date = new Date(); date.setFullYear(date.getFullYear() + 1); return date;or simplynew Date(Date.now() + 365 * 24 * 60 * 60 * 1000)This line has the same date calculation issue as the previous occurrences. See comments on lines 19, 25, and 31.
The test suite only includes a basic creation test. The component has several behavioral methods (
onInput,onTypeChange,addEvent) that lack test coverage. Consider adding tests to verify:onInputemitsdayChangewith updatednumberOfDaysonTypeChangeemitsdayChangewith updatedtypeaddEventemitsdayChangewith updatedexpiryDatedeleteEntryoutput works correctlyOn each iteration of the while loop, the code creates a copy of
vacationDays, filters it, and sorts it. This is inefficient for large arrays. Consider moving the filtering and sorting outside the loop, or using a more efficient data structure like a priority queue to avoid repeated sorting.This line has the same date calculation issue as line 19. Consider extracting this logic into a helper method to avoid duplication and improve maintainability.
Adding
--legacy-peer-depsflag suggests there are peer dependency conflicts with Angular 21. While this may work, it bypasses npm's peer dependency validation which could hide compatibility issues. Consider documenting why this flag is necessary, or investigate resolving the underlying peer dependency conflicts.Review: Commented
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 19 comments.
Comments suppressed due to low confidence (1)
src/app/services/calendar-service.ts:925
lookForVacationmethod has been modified to filter candidate days based on the expiry date of theVacDayobject. This new behavior lacks test coverage to ensure days after the expiry date are correctly excluded from consideration.Merged by: THoguet at 2025-11-30 02:17:57 UTC
Preserved source assignment/reviewer metadata. Historical accounts stay inactive.
Source requested reviewers: admin-backup (GitLab user 3).
The linked history record retains original IDs and source dates. No historical identity was merged into nessar or granted new authentication permissions.
Preserved GitLab !61 history: source attribution, exact timestamps, discussion grouping and review positions. Historical diff snapshots: 1; visible source notes: 67.
gitlab-35-mergerequest-61.md (SHA-256
0ff43ee494c7c3e8fcc0b8dc244691ef9a8601103ead1aef0fa7b0098e04929d)gitlab-35-mergerequest-61.json (SHA-256
8863c25530d8af4f0e797b36868d94d2eb73be01dd7e6c4e50437ed99ae346e4)Old CI job execution and reports are omitted. Original source files and rollback backups remain protected.