Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jul 8, 2025

This PR implements a new feature to exclude holidays from copilot metrics data, allowing users to filter out non-working days from their analysis.

Changes Made

1. Added Holiday Exclusion Support

  • New dependency: Added date-holidays package for accurate holiday detection across different locales
  • New Options properties: Added excludeHolidays (boolean) and locale (string) to the Options class
  • Automatic locale detection: If no locale is provided, the system extracts it from the Accept-Language header

2. API Integration

  • Query parameter support: Users can now pass excludeHolidays=true to exclude holidays from results
  • Locale support: Optional locale parameter (e.g., 'US', 'DE', 'FR') to specify which country's holidays to exclude
  • Mock data support: Holiday exclusion works for both real API data and mock data during development

3. Implementation Details

  • Modified server/api/metrics.ts to filter out holidays from both real API responses and mock data generation
  • Updated updateMockDataDates function to skip holidays when generating date ranges
  • Added filterHolidaysFromMetrics function to remove holiday entries from existing data

4. Updated Options Class

  • Enhanced serialization/deserialization methods to handle new properties
  • Added comprehensive validation and type safety
  • Maintained backward compatibility - existing code continues to work unchanged

5. Comprehensive Testing

  • Added extensive tests for the new Options properties
  • Created integration tests for the date-holidays library
  • All existing tests continue to pass
  • Added tests for edge cases like invalid locales and timezone handling

Usage Examples

// Exclude US holidays from metrics
const options = new Options({
  excludeHolidays: true,
  locale: 'US'
});

// Or via query parameters
GET /api/metrics?excludeHolidays=true&locale=US

// Auto-detect locale from Accept-Language header
GET /api/metrics?excludeHolidays=true
// Header: Accept-Language: en-US,en;q=0.9

Benefits

  • More accurate metrics: Excludes non-working days from productivity analysis
  • Flexible localization: Supports different countries' holiday calendars
  • Zero breaking changes: Existing functionality remains unchanged
  • Comprehensive error handling: Gracefully handles invalid locales and edge cases

Fixes #233.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.fontshare.com
    • Triggering command: node (vitest) (dns block)
  • api.fontsource.org
    • Triggering command: node (vitest) (dns block)
  • fonts.bunny.net
    • Triggering command: node (vitest) (dns block)
  • fonts.google.com
    • Triggering command: node (vitest) (dns block)
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)
  • fonts.googleapis.com
    • Triggering command: node (vitest 1) (dns block)
    • Triggering command: node (vitest 2) (dns block)
    • Triggering command: node (vitest 3) (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI changed the title [WIP] Add option to exclude holidays from copilot metrics Add option to exclude holidays from copilot metrics Jul 8, 2025
@Copilot Copilot AI requested a review from karpikpl July 8, 2025 22:38
Copilot finished work on behalf of karpikpl July 8, 2025 22:38
@karpikpl
Copy link
Collaborator

karpikpl commented Jul 8, 2025

@copilot I want to set excludeHolidays from the UI in the DateSelectorControl

Copy link
Contributor Author

Copilot AI commented Jul 8, 2025

@copilot I want to set excludeHolidays from the UI in the DateSelectorControl

I've added UI controls to the DateRangeSelector component for setting excludeHolidays from the interface. The implementation includes:

  1. Checkbox: "Exclude holidays from metrics" option
  2. Locale dropdown: Country/locale selector with 14 common options (US, DE, FR, GB, etc.)
  3. Smart UX: Locale dropdown is only enabled when holiday exclusion is checked
  4. Complete integration: Holiday options flow from UI → MainComponent → Options → API

Users can now set holiday exclusion directly from the Date Range Filter section without needing to modify URL parameters manually.

Commit: efd65eb

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.fontshare.com
    • Triggering command: node (vitest) (dns block)
    • Triggering command: /usr/local/bin/node --enable-source-maps /home/REDACTED/work/copilot-metrics-viewer/copilot-metrics-viewer/node_modules/@nuxt/cli/bin/nuxi.mjs _dev (dns block)
  • api.fontsource.org
    • Triggering command: node (vitest) (dns block)
    • Triggering command: /usr/local/bin/node --enable-source-maps /home/REDACTED/work/copilot-metrics-viewer/copilot-metrics-viewer/node_modules/@nuxt/cli/bin/nuxi.mjs _dev (dns block)
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js (dns block)
  • fonts.bunny.net
    • Triggering command: node (vitest) (dns block)
    • Triggering command: /usr/local/bin/node --enable-source-maps /home/REDACTED/work/copilot-metrics-viewer/copilot-metrics-viewer/node_modules/@nuxt/cli/bin/nuxi.mjs _dev (dns block)
  • fonts.google.com
    • Triggering command: node (vitest) (dns block)
    • Triggering command: /usr/local/bin/node --enable-source-maps /home/REDACTED/work/copilot-metrics-viewer/copilot-metrics-viewer/node_modules/@nuxt/cli/bin/nuxi.mjs _dev (dns block)
  • fonts.googleapis.com
    • Triggering command: node (vitest 2) (dns block)
    • Triggering command: node (vitest 1) (dns block)
    • Triggering command: node (vitest 3) (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of karpikpl July 8, 2025 23:09
@karpikpl karpikpl requested a review from Copilot July 9, 2025 16:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for excluding holidays from Copilot metrics by leveraging the date-holidays library and propagating new excludeHolidays and locale options through the API, model, and UI.

  • Introduce holiday filtering in server-side metrics endpoints and mock data generation
  • Extend Options model to include excludeHolidays and locale, with serialization/deserialization and query handling
  • Update UI (DateRangeSelector, MainComponent) to allow users to toggle holiday exclusion

Reviewed Changes

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

Show a summary per file
File Description
tests/weekends-filtering.spec.ts Added weekend/holiday identification tests (contains debug logs)
server/api/metrics.ts Integrated holiday filtering, locale detection, caching key updated
app/utils/dateUtils.ts Implemented isHoliday, filterHolidaysFromMetrics, parseUtcDate
app/components/DateRangeSelector.vue UI checkbox for excluding holidays, event emission updated
server/modules/authentication.ts Adjusted mock detection logic in authentication flow

Comment on lines +23 to +28
export function isHoliday(date: Date, locale: string): boolean {
try {
if (isWeekend(date)) {
return true; // Treat weekends as holidays
}
const holidays = new Holidays(locale);
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Creating a new Holidays instance on every call can be expensive; consider caching instances by locale outside of the filter loop.

Suggested change
export function isHoliday(date: Date, locale: string): boolean {
try {
if (isWeekend(date)) {
return true; // Treat weekends as holidays
}
const holidays = new Holidays(locale);
const holidaysCache = new Map<string, Holidays>();
export function isHoliday(date: Date, locale: string): boolean {
try {
if (isWeekend(date)) {
return true; // Treat weekends as holidays
}
let holidays = holidaysCache.get(locale);
if (!holidays) {
holidays = new Holidays(locale);
holidaysCache.set(locale, holidays);
}

Copilot uses AI. Check for mistakes.

@karpikpl karpikpl marked this pull request as ready for review July 9, 2025 16:34
@karpikpl karpikpl merged commit 3474b84 into main Jul 9, 2025
6 checks passed
@karpikpl karpikpl deleted the copilot/fix-233 branch July 9, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to exclude holidays from copilot metrics
2 participants