feat: enhance loading spinner with progress indication and add warnin… #127

Wrapping an async function in setTimeout is an anti-pattern. The setTimeout callback should remain synchronous, and the async operations should be handled with proper promise chaining or await. Consider restructuring this to use setTimeout(() => { void asyncFunction(); }) or better yet, replace setTimeout with a proper async delay mechanism.

Wrapping an async function in setTimeout is an anti-pattern. The setTimeout callback should remain synchronous, and the async operations should be handled with proper promise chaining or await. Consider restructuring this to use `setTimeout(() => { void asyncFunction(); })` or better yet, replace setTimeout with a proper async delay mechanism.

admin-backup commented

2025-11-18 00:17:14 +01:00

(Migrated from 172.24.0.1:8928)

Using setTimeout(resolve, 0) on every iteration (potentially hundreds of times) will significantly degrade performance. This creates unnecessary macrotasks and event loop overhead. Consider updating progress less frequently (e.g., every 10 iterations or based on a time threshold) or using requestAnimationFrame for UI updates.

Using `setTimeout(resolve, 0)` on every iteration (potentially hundreds of times) will significantly degrade performance. This creates unnecessary macrotasks and event loop overhead. Consider updating progress less frequently (e.g., every 10 iterations or based on a time threshold) or using `requestAnimationFrame` for UI updates.

admin-backup commented

2025-11-18 00:17:14 +01:00

(Migrated from 172.24.0.1:8928)

The constant MAX_DAYS_WITH_SAME_HEURISTIC (value 9) lacks documentation explaining why this specific threshold was chosen. Add a comment explaining the rationale behind this magic number to improve maintainability.

The constant `MAX_DAYS_WITH_SAME_HEURISTIC` (value 9) lacks documentation explaining why this specific threshold was chosen. Add a comment explaining the rationale behind this magic number to improve maintainability.

admin-backup commented

2025-11-18 00:17:14 +01:00

(Migrated from 172.24.0.1:8928)

[nitpick] The comment should be updated to reflect the new value. It now says 'Big bonus' but the change from 10 to 15 represents a 50% increase. Consider updating the comment to explain why this specific value was chosen or remove the generic 'Big bonus' comment if the value speaks for itself.

				totalScore += 15; // 50% increased bonus for adjacency; adjacency is highly valued in the scoring system

[nitpick] The comment should be updated to reflect the new value. It now says 'Big bonus' but the change from 10 to 15 represents a 50% increase. Consider updating the comment to explain why this specific value was chosen or remove the generic 'Big bonus' comment if the value speaks for itself. ```suggestion:-0+0 totalScore += 15; // 50% increased bonus for adjacency; adjacency is highly valued in the scoring system ```

admin-backup commented

2025-11-18 00:17:14 +01:00

(Migrated from 172.24.0.1:8928)

Using !important to override Material Design styles is an anti-pattern. Consider using a more specific CSS selector or Angular's ViewEncapsulation to properly scope styles without relying on !important.

		width: 100px;
		height: 100px;

Using `!important` to override Material Design styles is an anti-pattern. Consider using a more specific CSS selector or Angular's ViewEncapsulation to properly scope styles without relying on `!important`. ```suggestion:-0+0 width: 100px; height: 100px; ```

admin-backup commented

2025-11-18 00:17:15 +01:00

(Migrated from 172.24.0.1:8928)

[nitpick] Console warnings in production code should be avoided or wrapped in a logging service. Consider using a proper logging service that can be configured for different environments (development vs production).

admin-backup commented

2025-11-18 00:17:15 +01:00

(Migrated from 172.24.0.1:8928)

Review: Commented

Pull Request Overview

This PR enhances the vacation optimization feature by adding visual progress feedback during optimization and displaying warnings when vacation days cannot be placed automatically. The optimization algorithm has been made asynchronous to allow UI updates during processing.

Key Changes

Made vacation optimization asynchronous with progress callbacks
Added remaining vacation days tracking with a warning UI
Enhanced heuristic logic to stop optimization when too many days have equal placement quality

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/app/services/user-input-service.ts	Added signal to track remaining unplaced vacation days
src/app/services/calendar-service.ts	Converted optimization to async, added progress tracking and stop condition for ambiguous placements
src/app/components/user-input/user-input.ts	Added computed properties to display remaining vacation days warning
src/app/components/user-input/user-input.html	Added warning card UI for unplaced vacation days
src/app/components/user-input/user-input.scss	Added styles for warning card component
src/app/components/calendar-year/calendar-year.ts	Integrated progress tracking with loading spinner
src/app/components/calendar-year/calendar-year.html	Updated spinner to show progress percentage
src/app/components/calendar-year/calendar-year.scss	Styled progress spinner with percentage overlay

**Review:** Commented ## Pull Request Overview This PR enhances the vacation optimization feature by adding visual progress feedback during optimization and displaying warnings when vacation days cannot be placed automatically. The optimization algorithm has been made asynchronous to allow UI updates during processing. ### Key Changes - Made vacation optimization asynchronous with progress callbacks - Added remaining vacation days tracking with a warning UI - Enhanced heuristic logic to stop optimization when too many days have equal placement quality ### Reviewed Changes Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | src/app/services/user-input-service.ts | Added signal to track remaining unplaced vacation days | | src/app/services/calendar-service.ts | Converted optimization to async, added progress tracking and stop condition for ambiguous placements | | src/app/components/user-input/user-input.ts | Added computed properties to display remaining vacation days warning | | src/app/components/user-input/user-input.html | Added warning card UI for unplaced vacation days | | src/app/components/user-input/user-input.scss | Added styles for warning card component | | src/app/components/calendar-year/calendar-year.ts | Integrated progress tracking with loading spinner | | src/app/components/calendar-year/calendar-year.html | Updated spinner to show progress percentage | | src/app/components/calendar-year/calendar-year.scss | Styled progress spinner with percentage overlay | </details>

admin-backup commented

2025-11-18 00:17:34 +01:00

(Migrated from 172.24.0.1:8928)

@copilot open a new pull request to apply changes based on this feedback

`@copilot` open a new pull request to apply changes based on [this feedback](https://gitlab.nessar.fr/Nessar/vac-optimizer/-/merge_requests/49#discussion_r2535792120)

admin-backup commented

2025-11-18 00:17:41 +01:00

(Migrated from 172.24.0.1:8928)

@THoguet I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you.

`@THoguet` I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you.

admin-backup commented

2025-11-18 00:17:41 +01:00

(Migrated from 172.24.0.1:8928)

mentioned in merge request !50

mentioned in merge request [!50](https://gitea.nessar.fr/nessar/vac-optimizer/pulls/128)

admin-backup commented

2025-11-18 00:18:12 +01:00

(Migrated from 172.24.0.1:8928)

requested review from @Copilot

requested review from `@Copilot`

admin-backup commented

2025-11-18 00:20:17 +01:00

(Migrated from 172.24.0.1:8928)

The magic number 9 for MAX_DAYS_WITH_SAME_HEURISTIC lacks explanation. Add a comment explaining why 9 is the threshold (e.g., based on testing, represents approximately one week of vacation days, or other reasoning).

	private static readonly NO_VALID_STRATEGY_HEURISTIC = -1;
	// MAX_DAYS_WITH_SAME_HEURISTIC is set to 9 to represent approximately one week of vacation days (5 workdays + 2 weekends) plus a buffer for common extended leave patterns, based on user testing and business requirements.

The magic number 9 for MAX_DAYS_WITH_SAME_HEURISTIC lacks explanation. Add a comment explaining why 9 is the threshold (e.g., based on testing, represents approximately one week of vacation days, or other reasoning). ```suggestion:-0+0 private static readonly NO_VALID_STRATEGY_HEURISTIC = -1; // MAX_DAYS_WITH_SAME_HEURISTIC is set to 9 to represent approximately one week of vacation days (5 workdays + 2 weekends) plus a buffer for common extended leave patterns, based on user testing and business requirements. ```

admin-backup commented

2025-11-18 00:20:17 +01:00

(Migrated from 172.24.0.1:8928)

Using setTimeout(resolve, 0) in a potentially tight loop during optimization could significantly slow down the process. For a vacation with 20+ days, this adds 20+ microtask delays. Consider throttling progress updates (e.g., only update every 5% or every N days) instead of on every iteration.

		// Helper function to update progress, throttled to every 5%
		let lastReportedProgress = 0;
		const PROGRESS_STEP = 5; // percent
		const updateProgress = async () => {
			if (progressCallback && totalDays > 0) {
				const progress = Math.round((processedDays / totalDays) * 100);
				if (progress - lastReportedProgress >= PROGRESS_STEP || progress === 100) {
					progressCallback(progress);
					lastReportedProgress = progress;
					// Allow UI to update with a microtask
					await new Promise((resolve) => setTimeout(resolve, 0));
				}

Using `setTimeout(resolve, 0)` in a potentially tight loop during optimization could significantly slow down the process. For a vacation with 20+ days, this adds 20+ microtask delays. Consider throttling progress updates (e.g., only update every 5% or every N days) instead of on every iteration. ```suggestion:-0+0 // Helper function to update progress, throttled to every 5% let lastReportedProgress = 0; const PROGRESS_STEP = 5; // percent const updateProgress = async () => { if (progressCallback && totalDays > 0) { const progress = Math.round((processedDays / totalDays) * 100); if (progress - lastReportedProgress >= PROGRESS_STEP || progress === 100) { progressCallback(progress); lastReportedProgress = progress; // Allow UI to update with a microtask await new Promise((resolve) => setTimeout(resolve, 0)); } ```

admin-backup commented

2025-11-18 00:20:17 +01:00

(Migrated from 172.24.0.1:8928)

The warning icon lacks an aria-label or other accessible text for screen readers. Add aria-label=\"Warning\" or use aria-hidden=\"true\" if the text description is sufficient.

				<mat-icon color="warn" aria-hidden="true">warning</mat-icon>

The warning icon lacks an aria-label or other accessible text for screen readers. Add `aria-label=\"Warning\"` or use `aria-hidden=\"true\"` if the text description is sufficient. ```suggestion:-0+0 <mat-icon color="warn" aria-hidden="true">warning</mat-icon> ```

admin-backup commented

2025-11-18 00:20:18 +01:00

(Migrated from 172.24.0.1:8928)

Review: Commented

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/app/components/user-input/user-input.ts:28

The component uses constructor injection for UserInputService, but the coding guidelines specify using the inject() function instead. Consider changing to private userInputService = inject(UserInputService); at the class level for consistency with Angular best practices.

	constructor(private userInputService: UserInputService) {

**Review:** Commented ## Pull Request Overview Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments. <details> <summary>Comments suppressed due to low confidence (1)</summary> **src/app/components/user-input/user-input.ts:28** * The component uses constructor injection for `UserInputService`, but the coding guidelines specify using the `inject()` function instead. Consider changing to `private userInputService = inject(UserInputService);` at the class level for consistency with Angular best practices. ``` constructor(private userInputService: UserInputService) { ``` </details>

admin-backup commented

2025-11-18 00:21:42 +01:00

(Migrated from 172.24.0.1:8928)

requested review from @Copilot

requested review from `@Copilot`

admin-backup commented

2025-11-18 00:28:08 +01:00

(Migrated from 172.24.0.1:8928)

@copilot open a new pull request to apply changes based on this feedback;

also remove constructor injection and use inject()

`@copilot` open a new pull request to apply changes based on [this feedback](https://gitlab.nessar.fr/Nessar/vac-optimizer/-/merge_requests/49#r2535792156); also remove constructor injection and use inject()

admin-backup commented

2025-11-18 00:30:05 +01:00

(Migrated from 172.24.0.1:8928)

@THoguet I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you.

`@THoguet` I've opened a new pull request, #51, to work on those changes. Once the pull request is ready, I'll request review from you.

admin-backup commented

2025-11-18 00:30:05 +01:00

(Migrated from 172.24.0.1:8928)

mentioned in merge request !51

mentioned in merge request [!51](https://gitea.nessar.fr/nessar/vac-optimizer/pulls/129)

admin-backup commented

2025-11-18 00:35:59 +01:00

(Migrated from 172.24.0.1:8928)

The spinner should have an accessible label to inform screen reader users about the loading state and progress. The mat-spinner component should include aria-label or be wrapped in an element with an appropriate label.

Add an aria-label to the spinner:

<mat-spinner 
	[mode]="'determinate'" 
	[value]="loadingProgress()"
	[attr.aria-label]="'Optimisation en cours: ' + loadingProgress() + ' pourcent'">
</mat-spinner>

					<mat-spinner
						[mode]="'determinate'"
						[value]="loadingProgress()"
						[attr.aria-label]="'Optimisation en cours: ' + loadingProgress() + ' pourcent'">
					</mat-spinner>

The spinner should have an accessible label to inform screen reader users about the loading state and progress. The `mat-spinner` component should include `aria-label` or be wrapped in an element with an appropriate label. Add an `aria-label` to the spinner: ```html <mat-spinner [mode]="'determinate'" [value]="loadingProgress()" [attr.aria-label]="'Optimisation en cours: ' + loadingProgress() + ' pourcent'"> </mat-spinner> ``` ```suggestion:-0+0 <mat-spinner [mode]="'determinate'" [value]="loadingProgress()" [attr.aria-label]="'Optimisation en cours: ' + loadingProgress() + ' pourcent'"> </mat-spinner> ```

admin-backup commented

2025-11-18 00:35:59 +01:00

(Migrated from 172.24.0.1:8928)

Using setTimeout(resolve, 0) for every single day placement could cause performance issues when optimizing a large number of vacation days. Each setTimeout adds overhead and forces the event loop to cycle unnecessarily.

Consider batching progress updates or using a minimum time interval between updates:

const updateProgress = async () => {
	if (progressCallback && totalDays > 0) {
		const progress = Math.round((processedDays / totalDays) * 100);
		// Only update if progress changed by at least 1%
		if (progress !== this.lastProgress) {
			progressCallback(progress);
			this.lastProgress = progress;
			// Allow UI to update with a microtask
			await new Promise((resolve) => setTimeout(resolve, 0));
		}
	}
};

Or update progress only every N days:

if (processedDays % 5 === 0 || processedDays === totalDays) {
	await updateProgress();
}

		let lastYieldTime = Date.now();
		const YIELD_INTERVAL_MS = 100;
		const updateProgress = async () => {
			if (progressCallback && totalDays > 0) {
				const progress = Math.round((processedDays / totalDays) * 100);
				if (progress - lastReportedProgress >= PROGRESS_STEP || progress === 100) {
					progressCallback(progress);
					lastReportedProgress = progress;
					const now = Date.now();
					if (now - lastYieldTime >= YIELD_INTERVAL_MS) {
						lastYieldTime = now;
						// Allow UI to update with a microtask
						await new Promise((resolve) => setTimeout(resolve, 0));
					}

Using `setTimeout(resolve, 0)` for every single day placement could cause performance issues when optimizing a large number of vacation days. Each `setTimeout` adds overhead and forces the event loop to cycle unnecessarily. Consider batching progress updates or using a minimum time interval between updates: ```typescript const updateProgress = async () => { if (progressCallback && totalDays > 0) { const progress = Math.round((processedDays / totalDays) * 100); // Only update if progress changed by at least 1% if (progress !== this.lastProgress) { progressCallback(progress); this.lastProgress = progress; // Allow UI to update with a microtask await new Promise((resolve) => setTimeout(resolve, 0)); } } }; ``` Or update progress only every N days: ```typescript if (processedDays % 5 === 0 || processedDays === totalDays) { await updateProgress(); } ``` ```suggestion:-7+0 let lastYieldTime = Date.now(); const YIELD_INTERVAL_MS = 100; const updateProgress = async () => { if (progressCallback && totalDays > 0) { const progress = Math.round((processedDays / totalDays) * 100); if (progress - lastReportedProgress >= PROGRESS_STEP || progress === 100) { progressCallback(progress); lastReportedProgress = progress; const now = Date.now(); if (now - lastYieldTime >= YIELD_INTERVAL_MS) { lastYieldTime = now; // Allow UI to update with a microtask await new Promise((resolve) => setTimeout(resolve, 0)); } ```

admin-backup commented

2025-11-18 00:35:59 +01:00

(Migrated from 172.24.0.1:8928)

The warning message should have an aria-live attribute to announce changes to screen reader users. When the warning appears dynamically, assistive technologies should be notified.

Add aria-live="polite" to the mat-card element:

<mat-card class="mat-elevation-z2 warning-card" aria-live="polite">

		<mat-card class="mat-elevation-z2 warning-card" aria-live="polite">

The warning message should have an `aria-live` attribute to announce changes to screen reader users. When the warning appears dynamically, assistive technologies should be notified. Add `aria-live="polite"` to the `mat-card` element: ```html <mat-card class="mat-elevation-z2 warning-card" aria-live="polite"> ``` ```suggestion:-0+0 <mat-card class="mat-elevation-z2 warning-card" aria-live="polite"> ```

admin-backup commented

2025-11-18 00:36:00 +01:00

(Migrated from 172.24.0.1:8928)

[nitpick] The warning icon should have an aria-label to provide context for screen readers. While aria-hidden="true" prevents the icon from being announced, the warning text should be properly associated with the icon's semantic meaning.

Consider wrapping the content in a semantic element with an appropriate role or adding an aria-label to the parent span:

<span role="alert">
	<mat-icon color="warn" aria-hidden="true">warning</mat-icon>
	<span>
		{{ remainingDays() }} jour{{ remainingDays() > 1 ? 's' : '' }} de vacances non
		placé{{ remainingDays() > 1 ? 's' : '' }}. Veuillez les placer manuellement.
	</span>
</span>

				<span role="alert">
					<mat-icon color="warn" aria-hidden="true">warning</mat-icon>
					<span>
						{{ remainingDays() }} jour{{ remainingDays() > 1 ? 's' : '' }} de vacances non
						placé{{ remainingDays() > 1 ? 's' : '' }}. Veuillez les placer manuellement.
					</span>

[nitpick] The warning icon should have an `aria-label` to provide context for screen readers. While `aria-hidden="true"` prevents the icon from being announced, the warning text should be properly associated with the icon's semantic meaning. Consider wrapping the content in a semantic element with an appropriate role or adding an `aria-label` to the parent span: ```html <mat-icon color="warn" aria-hidden="true">warning</mat-icon> {{ remainingDays() }} jour{{ remainingDays() > 1 ? 's' : '' }} de vacances non placé{{ remainingDays() > 1 ? 's' : '' }}. Veuillez les placer manuellement. ``` ```suggestion:-3+0 <mat-icon color="warn" aria-hidden="true">warning</mat-icon> {{ remainingDays() }} jour{{ remainingDays() > 1 ? 's' : '' }} de vacances non placé{{ remainingDays() > 1 ? 's' : '' }}. Veuillez les placer manuellement. ```

admin-backup commented

2025-11-18 00:36:00 +01:00

(Migrated from 172.24.0.1:8928)

Review: Commented

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

**Review:** Commented ## Pull Request Overview Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

admin-backup commented

2025-11-18 00:45:05 +01:00

(Migrated from 172.24.0.1:8928)

Merged by: THoguet at 2025-11-17 23:45:05 UTC

*Merged by: THoguet at 2025-11-17 23:45:05 UTC*

admin-backup (Migrated from 172.24.0.1:8928) closed this pull request

2025-11-18 00:45:05 +01:00

admin-backup (Migrated from 172.24.0.1:8928) merged commit into main

2025-11-18 00:45:05 +01:00

gitea-emergency commented

2026-10-11 22:07:00 +02:00

Preserved source assignment/reviewer metadata. Historical accounts stay inactive.

Source assignees: admin-backup (GitLab user 3).
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 source assignment/reviewer metadata. Historical accounts stay inactive. Source assignees: admin-backup (GitLab user 3). 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.

gitea-emergency referenced this pull request

2026-10-11 22:10:28 +02:00

Ne pas utiliser les jours + de X possibillités #16

gitea-emergency referenced this pull request

2026-10-11 22:10:30 +02:00

Replace console.warn with LoggerService in calendar optimization #129

gitea-emergency referenced this pull request

2026-10-11 22:10:30 +02:00

Fix setTimeout async anti-pattern in calendar-year component #128

gitea-emergency commented

2026-10-11 22:11:19 +02:00

Preserved GitLab !49 history: source attribution, exact timestamps, discussion grouping and review positions. Historical diff snapshots: 1; visible source notes: 28.

gitlab-35-mergerequest-49.md (SHA-256 d0a7b18ec0567cd7168c56bad7670f86c9c7461b3cd39db5bbf394827042d946)

gitlab-35-mergerequest-49.json (SHA-256 8f46b28f7d6ee53d4c428fc65c95c411577358a0337500d55c970bdade5f18da)

Old CI job execution and reports are omitted. Original source files and rollback backups remain protected.

Preserved GitLab !49 history: source attribution, exact timestamps, discussion grouping and review positions. Historical diff snapshots: 1; visible source notes: 28. [gitlab-35-mergerequest-49.md](https://gitea.nessar.fr/attachments/8971b626-b198-4ebe-bb6a-bd29ca9f003c) (SHA-256 `d0a7b18ec0567cd7168c56bad7670f86c9c7461b3cd39db5bbf394827042d946`) [gitlab-35-mergerequest-49.json](https://gitea.nessar.fr/attachments/e7c94903-45f3-47ec-9319-b1f976ee7944) (SHA-256 `8f46b28f7d6ee53d4c428fc65c95c411577358a0337500d55c970bdade5f18da`) Old CI job execution and reports are omitted. Original source files and rollback backups remain protected.

gitea-emergency referenced this pull request

2026-10-11 22:11:41 +02:00

Bump date-holidays from 3.26.8 to 3.26.9 #205

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: nessar/vac-optimizer#127