Skip to content

Conversation

@amolmjoshi93
Copy link
Contributor

@amolmjoshi93 amolmjoshi93 commented Dec 30, 2025

feat: Show holiday validation errors near input fields instead of flash message

- Update BulkHolidayService to collect field-level validation errors
  instead of raising exceptions, returning success/failure boolean
- Modify HolidaysController#update to return field_errors in response
- Add error state management in Holidays component to track and display
  field-level errors for both public and optional holidays
- Update EditHolidays component to show error messages near name inputs
  with red border styling
- Skip generic toast in axios interceptor when field_errors are present
- Update HolidayInfo name validation to allow hyphens and apostrophes
  (e.g., "New Year's Day", "Labor-Day") with descriptive error message
- Fix net_days calculation in TimeoffEntries::IndexService to preserve
  negative values for leave balance display
- Update related specs for BulkHolidayService and HolidayInfo model

Closes #2027 

Summary by CodeRabbit

  • New Features

    • Allow apostrophes and hyphens in holiday names.
    • Inline, per-item error messages appear next to holiday entries during edits.
  • Bug Fixes

    • Bulk holiday updates now fail on validation errors and surface field-level messages.
    • API error handling avoids generic toasts when field-level errors exist.
    • Time-off summaries show signed day/hour labels and "Overdrawn" states.
  • Chores

    • Upgraded Ruby runtime and container configs; added DOCKER_CONTAINER env and init adjustment.
    • Updated macOS setup docs.
  • Tests

    • Added/updated tests for validations, bulk updates, timeoff labeling, and PDF stubbing.

✏️ Tip: You can customize this high-level summary in your review settings.

…sh message

- Update BulkHolidayService to collect field-level validation errors
  instead of raising exceptions, returning success/failure boolean
- Modify HolidaysController#update to return field_errors in response
- Add error state management in Holidays component to track and display
  field-level errors for both public and optional holidays
- Update EditHolidays component to show error messages near name inputs
  with red border styling
- Skip generic toast in axios interceptor when field_errors are present
- Update HolidayInfo name validation to allow hyphens and apostrophes
  (e.g., "New Year's Day", "Labor-Day") with descriptive error message
- Fix net_days calculation in TimeoffEntries::IndexService to preserve
  negative values for leave balance display
- Update related specs for BulkHolidayService and HolidayInfo model
@amolmjoshi93 amolmjoshi93 linked an issue Dec 30, 2025 that may be closed by this pull request
@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • Gemfile.lock is excluded by !**/*.lock

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

Backend bulk-holiday flow now validates per-item, collects indexed field_errors, and returns boolean success; controller renders 200 on success or 422 with field_errors on failure. Frontend suppresses generic toast when field_errors exist and maps/show per-item inline errors. Model name regex now allows hyphens and apostrophes.

Changes

Cohort / File(s) Summary
Model Validation
app/models/holiday_info.rb
Relaxed name regex to /\A[a-zA-Z\s\-']+\z/ and added descriptive error message allowing hyphens and apostrophes.
Bulk Holiday Service
app/services/bulk_holiday_service.rb
Added @errors nested hash and attr_accessor :holiday, :errors; process validates per-index, collects indexed field_errors, rolls back on errors, and returns boolean success; added errors_present?.
Controller
app/controllers/internal_api/v1/holidays_controller.rb
update now instantiates BulkHolidayService, calls process, renders success notice on true or field_errors with status 422 on false.
API Layer
app/javascript/src/apis/api.ts
Error handler skips generic Toastr.error when error.response?.data?.field_errors exists; preserves 401/423 flows and promise rejection.
Frontend — Edit UI
app/javascript/src/components/Profile/Organization/Holidays/EditHolidays/index.tsx
Imported ErrorSpan; EditHolidays accepts holidayErrors and optionalHolidayErrors (defaults {}) and conditionally shows inline error UI per index.
Frontend — Parent Component
app/javascript/src/components/Profile/Organization/Holidays/index.tsx
Tracks holidayErrors/optionalHolidayErrors, clears per-item errors on input, wraps save in try/catch, maps API field_errors to per-index error maps and passes them to EditHolidays.
Service Tests
spec/services/bulk_holiday_service_spec.rb
Added specs asserting process returns true on success and returns false with populated indexed errors for invalid add/update names.
Model Tests
spec/models/holiday_info_spec.rb
Added tests for apostrophes/hyphens acceptance, rejection of invalid chars, and descriptive error message.
Timeoff Entries (logic & tests)
app/services/timeoff_entries/index_service.rb, spec/services/timeoff_entries/index_service_spec.rb
Switched to signed net-hours arithmetic; revised label logic for zero/overdrawn/positive balances; added zero/overdrawn tests and updated expectations.
Ruby / Docker / Env & Docs
.ruby-version, Gemfile, .devcontainer/Dockerfile, Dockerfile.local, Dockerfile.test, deployment/fly/Dockerfile, docker-compose.yml, docs/docs/contributing-guide/setup/macos.md
Bumped Ruby 3.3.10 → 3.4.8, updated gem constraints/additions, added DOCKER_CONTAINER=true env for services, and aligned docs/setup.
Other Tests / Fixtures
spec/models/timeoff_entry_spec.rb
Adjusted fixtures and tests for optional-holiday quotas and deterministic dates; broader test-scope changes.
Test Helpers
spec/support/pdf_generation_helpers.rb
Added test helper stubbing FerrumPdf.render_pdf to return a fake PDF unless tests opt-in for real PDFs.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant EditUI as EditHolidays (Client)
    participant ParentUI as Holidays (Client)
    participant API as API Client
    participant Controller as HolidaysController
    participant Service as BulkHolidayService
    participant Model as HolidayInfo

    User->>EditUI: edit inputs / submit
    EditUI->>ParentUI: notify changes (clears index error)
    ParentUI->>API: POST /internal_api/v1/holidays (payload)
    API->>Controller: HTTP request

    Controller->>Service: BulkHolidayService.new(...).process()
    rect rgba(220,235,255,0.5)
      Service->>Model: validate add/update items (per-index)
      Model-->>Service: validation results
      Service->>Service: collect indexed field_errors
      Service->>Service: check errors_present?
    end

    alt errors_present
      Service-->>Controller: return false
      Controller-->>API: 422 JSON { field_errors: {...} }
    else success
      Service-->>Controller: return true
      Controller-->>API: 200 JSON { notice }
    end

    API->>ParentUI: response (with/without field_errors)
    alt field_errors present
      Note right of ParentUI: API client skips generic Toastr.error
      ParentUI->>ParentUI: map field_errors -> holidayErrors / optionalHolidayErrors
      ParentUI->>EditUI: render with error props
      EditUI->>User: show inline ErrorSpan at index
    else no field_errors
      ParentUI->>User: generic Toastr.error (if applicable)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • apoorv1316
  • shalapatil

Poem

🐰 I hopped through names with hyphens and ticks,
Now errors nestle where the input afflicts.
Indexed whispers show which name went wrong,
Service rolls back and sings the error song.
I nibble bugs — holidays fixed, bright and strong!

🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes out-of-scope changes: Ruby version upgrade (3.3.10 to 3.4.8), gem updates, Docker configuration changes, PDF generation stubbing, and unrelated timeoff net_days calculation fixes that are not required by issue #2027. Separate the Ruby version upgrade, gem updates, Docker changes, and timeoff net_days fixes into a dedicated PR to keep this PR focused on holiday validation error display and special character support.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: showing holiday validation errors near input fields instead of a flash message, which is the primary objective.
Linked Issues check ✅ Passed The pull request successfully addresses both requirements from #2027: allows special characters (hyphens, apostrophes) in holiday names and displays field-level validation errors near inputs instead of generic flash messages.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@amolmjoshi93
Copy link
Contributor Author

After fixes the error will be shown like:

Screenshot 2025-12-30 174308

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
app/services/bulk_holiday_service.rb (1)

40-66: Consider extracting shared validation and error collection logic.

The validation and error collection pattern is duplicated between add_holiday_info and update_holiday_info. Extracting this logic into a helper method would improve maintainability.

Example refactor
private

def validate_and_save_with_errors(holiday_info, error_category, index)
  unless holiday_info.valid?
    errors[error_category][index] = holiday_info.errors.messages
    return false
  end
  
  unless holiday_info.save
    errors[error_category][index] = holiday_info.errors.messages
    return false
  end
  
  true
end

def add_holiday_info
  return if holiday_params[:add_holiday_infos].blank?

  holiday_params[:add_holiday_infos].each_with_index do |info, index|
    holiday_info = holiday.holiday_infos.build(info)
    validate_and_save_with_errors(holiday_info, :add_holiday_infos, index)
  end
end

def update_holiday_info
  return if holiday_params[:update_holiday_infos].blank?

  holiday_params[:update_holiday_infos].each_with_index do |info, index|
    holiday_info = holiday.holiday_infos.find_by(id: info[:id])
    next unless holiday_info
    
    holiday_info.assign_attributes(info)
    validate_and_save_with_errors(holiday_info, :update_holiday_infos, index)
  end
end
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bc587a0 and 0848402.

📒 Files selected for processing (9)
  • app/controllers/internal_api/v1/holidays_controller.rb
  • app/javascript/src/apis/api.ts
  • app/javascript/src/components/Profile/Organization/Holidays/EditHolidays/index.tsx
  • app/javascript/src/components/Profile/Organization/Holidays/index.tsx
  • app/models/holiday_info.rb
  • app/services/bulk_holiday_service.rb
  • app/services/timeoff_entries/index_service.rb
  • spec/models/holiday_info_spec.rb
  • spec/services/bulk_holiday_service_spec.rb
🧰 Additional context used
🧬 Code graph analysis (6)
spec/models/holiday_info_spec.rb (1)
app/models/holiday_info.rb (1)
  • include (25-66)
app/controllers/internal_api/v1/holidays_controller.rb (2)
app/controllers/concerns/current_company_concern.rb (1)
  • current_company (10-14)
app/services/bulk_holiday_service.rb (1)
  • process (14-23)
app/javascript/src/components/Profile/Organization/Holidays/index.tsx (2)
app/controllers/internal_api/v1/holidays_controller.rb (2)
  • index (3-34)
  • index (4-9)
app/controllers/internal_api/v1/companies_controller.rb (1)
  • index (6-14)
app/javascript/src/apis/api.ts (2)
app/javascript/src/StyledComponents/index.tsx (1)
  • Toastr (26-26)
app/javascript/src/StyledComponents/Toastr/index.tsx (1)
  • Toastr (81-84)
app/javascript/src/components/Profile/Organization/Holidays/EditHolidays/index.tsx (1)
app/javascript/src/common/ErrorSpan/index.tsx (1)
  • ErrorSpan (3-5)
spec/services/bulk_holiday_service_spec.rb (1)
app/services/bulk_holiday_service.rb (1)
  • process (14-23)
⏰ 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: Running tests
🔇 Additional comments (15)
app/javascript/src/apis/api.ts (1)

43-53: LGTM! Clean separation of error handling concerns.

The conditional check correctly prevents duplicate error messaging by skipping the generic toast when field-level errors are present. This allows components to handle their own field-specific errors inline while maintaining the generic toast for other error scenarios.

app/javascript/src/components/Profile/Organization/Holidays/EditHolidays/index.tsx (3)

45-46: LGTM! Well-typed error props.

The error props are correctly typed as Record<number, string> with sensible defaults, enabling type-safe access throughout the component.


120-137: LGTM! Effective inline error display for public holidays.

The implementation correctly:

  • Applies red border styling to inputs with errors
  • Changes label color to red for visual consistency
  • Displays the error message inline using ErrorSpan
  • Maintains clean conditional rendering

280-297: LGTM! Consistent error handling for optional holidays.

The error handling pattern is identical to public holidays, ensuring a consistent user experience across both holiday types.

app/javascript/src/components/Profile/Organization/Holidays/index.tsx (3)

52-58: LGTM! Clean error state initialization.

The error state maps are properly typed and initialized, providing a foundation for field-level error tracking.


207-222: LGTM! Responsive error clearing improves UX.

Clearing errors as the user types provides immediate feedback and prevents stale error messages from lingering after the user corrects an issue.


400-404: LGTM! Error props correctly propagated.

The error maps are properly passed to the EditHolidays component, completing the error display flow.

app/models/holiday_info.rb (1)

34-36: LGTM! Validation correctly extended with clear messaging.

The updated regex properly allows letters, spaces, hyphens, and apostrophes, enabling holiday names like "New Year's Day" and "Labor-Day". The descriptive error message clearly communicates the validation rule to users.

app/controllers/internal_api/v1/holidays_controller.rb (1)

15-20: LGTM! Clean success/failure path separation.

The updated logic correctly:

  • Branches on the service's boolean return value
  • Returns field-level errors with appropriate 422 status on failure
  • Maintains the success notice on the happy path

This enables the frontend to display granular validation errors inline.

spec/models/holiday_info_spec.rb (1)

22-33: LGTM! Comprehensive test coverage for updated validation.

The tests thoroughly validate:

  • Apostrophes and hyphens are permitted
  • Invalid characters are rejected
  • The descriptive error message is returned

This ensures the validation rules work as intended and provides regression protection.

spec/services/bulk_holiday_service_spec.rb (3)

37-39: LGTM! Success path verification.

The test confirms that the service returns true on successful processing, establishing the expected behavior for the happy path.


101-131: LGTM! Thorough validation error testing for add operations.

The test properly verifies:

  • process returns false when validation fails
  • No invalid data is persisted
  • Errors are structured correctly with field-level detail

This ensures the error handling contract between backend and frontend is reliable.


133-172: LGTM! Comprehensive validation error testing for update operations.

The test correctly validates:

  • process returns false on validation failure
  • Existing data remains unchanged
  • Per-field error details are captured

This complements the add operation tests and ensures consistent error handling across both scenarios.

app/services/bulk_holiday_service.rb (2)

5-5: LGTM: Clean error tracking initialization.

The attr_accessor :errors and the initialized nested hash structure provide clear separation between add and update operation errors.

Also applies to: 11-11


14-27: LGTM: Proper transaction handling with conditional rollback.

The boolean return value and controlled rollback using ActiveRecord::Rollback follow Rails best practices. The all-or-nothing behavior ensures data consistency when validation errors occur.

- Replace save! with save in BulkHolidayService to collect database errors gracefully
- Fix occurrence-aware error mapping in Holidays component for duplicate names
- Enhance leave balance labels to show "Overdrawn by X" for negative balances
- Add test coverage for zero and negative leave balance scenarios
- Update aws-sdk-s3 to 1.209.0 to resolve CVE-2025-14762 security vulnerability

This ensures consistent error collection patterns, clearer user feedback for
overdrawn leave balances, and addresses security vulnerabilities.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/services/bulk_holiday_service.rb (1)

31-38: Inconsistent error handling for holiday record operations.

The create_or_update_holiday method uses update! and create! (lines 34, 36), which raise exceptions on validation failures or constraint violations. This is inconsistent with the graceful error-collection pattern established in add_holiday_info and update_holiday_info.

If the Holiday model has validations that fail (e.g., on year, enable_optional_holidays, or no_of_allowed_optional_holidays), or if database constraints are violated, the exception will escape the service and the controller cannot return field_errors for these issues.

🔎 Proposed fix
  def create_or_update_holiday
    @holiday = current_company.holidays.find_by(year:)
    if holiday
-     @holiday.update!(holiday_params[:holiday])
+     unless @holiday.update(holiday_params[:holiday])
+       errors[:holiday] = @holiday.errors.messages
+       return
+     end
    else
-     @holiday = current_company.holidays.create!(holiday_params[:holiday])
+     @holiday = current_company.holidays.build(holiday_params[:holiday])
+     unless @holiday.save
+       errors[:holiday] = @holiday.errors.messages
+       return
+     end
    end
  end

You'll also need to update errors_present? to check for errors[:holiday]:

  def errors_present?
-   errors[:add_holiday_infos].present? || errors[:update_holiday_infos].present?
+   errors[:holiday].present? || errors[:add_holiday_infos].present? || errors[:update_holiday_infos].present?
  end

And initialize the :holiday key in @errors:

- @errors = { add_holiday_infos: {}, update_holiday_infos: {} }
+ @errors = { holiday: {}, add_holiday_infos: {}, update_holiday_infos: {} }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0848402 and 3610e8c.

⛔ Files ignored due to path filters (1)
  • Gemfile.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • Gemfile
  • app/javascript/src/components/Profile/Organization/Holidays/index.tsx
  • app/services/bulk_holiday_service.rb
  • app/services/timeoff_entries/index_service.rb
  • spec/services/timeoff_entries/index_service_spec.rb
🧰 Additional context used
🧬 Code graph analysis (3)
app/services/bulk_holiday_service.rb (1)
app/controllers/internal_api/v1/holidays_controller.rb (3)
  • holiday_params (25-33)
  • index (3-34)
  • index (4-9)
spec/services/timeoff_entries/index_service_spec.rb (1)
app/services/timeoff_entries/index_service.rb (2)
  • working_hours_per_day (201-205)
  • process (22-31)
app/javascript/src/components/Profile/Organization/Holidays/index.tsx (1)
app/controllers/internal_api/v1/holidays_controller.rb (2)
  • index (3-34)
  • index (4-9)
⏰ 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: Running tests
🔇 Additional comments (13)
Gemfile (1)

82-82: Version constraint correctly addresses CVE-2025-14762.

The aws-sdk-s3 constraint of >= 1.208.0 aligns with official security guidance from AWS (AWS-2025-032), RubySec, and Snyk, which all confirm 1.208.0 as the patched version.

app/services/timeoff_entries/index_service.rb (1)

68-86: LGTM! Zero and negative balance handling is correct.

The signed arithmetic for net_days and extra_hours (lines 68-69) combined with the conditional label logic (lines 71-86) correctly handles:

  • Zero balance: "0 hours"
  • Negative balance (overdrawn): uses absolute values to avoid Ruby modulo quirks with negative dividends
  • Positive balance: standard days/hours formatting

The implementation properly addresses the previous review concern about missing test coverage and unclear negative balance handling.

spec/services/timeoff_entries/index_service_spec.rb (3)

303-343: LGTM! Zero balance test coverage is comprehensive.

The test correctly sets up a scenario where allocation exactly matches timeoff taken, and verifies the "0 hours" label is produced.


345-416: LGTM! Negative balance (overdrawn) test coverage is thorough.

Both test contexts correctly verify the overdrawn scenarios:

  • Hours only: produces "Overdrawn by X hours" when under one day
  • Days and hours: produces "Overdrawn by X days Y hours" for larger overdrawn amounts

The test arithmetic correctly produces negative net_duration values that exercise the service's negative-balance logic.


244-245: LGTM! Test expectation correctly updated for overdrawn format.

The test properly verifies that when a user has a negative balance (joined mid-year with per-week allocation), the service produces the new "Overdrawn by X days Y hours" label format. The test arithmetic using net_hours.abs correctly mirrors the service implementation.

app/javascript/src/components/Profile/Organization/Holidays/index.tsx (4)

52-58: LGTM!

The error state initialization is clean and well-typed. Using Record<number, string> correctly models the index-to-error-message mapping needed for field-level validation feedback.


207-222: LGTM!

The error clearing logic provides good UX by immediately removing the error message when users start typing. The implementation correctly removes only the specific field's error while preserving others.


287-403: LGTM!

The error handling implementation is robust:

  • Clears stale errors before each save attempt
  • Uses occurrence-aware matching (lines 313-350) to correctly map errors even when multiple holidays share the same name
  • Falls back to ID-based lookup for existing holidays (lines 371-398)
  • Properly separates national and optional holiday errors

The duplicate-name edge case flagged in previous reviews has been addressed with the Set-based tracking approach.


437-441: LGTM!

Error state is correctly passed to the EditHolidays component, enabling inline error display for each field.

app/services/bulk_holiday_service.rb (4)

5-5: LGTM!

The error tracking setup is well-structured: exposing errors via attr_accessor allows the controller to access per-field validation failures, and the nested hash initialization matches the expected API response format.

Also applies to: 11-11


14-27: LGTM!

The process method implements a clean error-driven flow:

  • Wraps all operations in a transaction for atomicity
  • Rolls back on validation failures to prevent partial saves
  • Returns a boolean success indicator for the controller to branch on

The errors_present? helper provides a clear predicate for error checking.


40-52: LGTM!

The error collection pattern is robust:

  • Validates before attempting to save
  • Captures both validation errors (lines 45-48) and persistence failures (lines 49-51)
  • Uses save instead of save! to gracefully handle DB constraint violations
  • Indexes errors by payload position for accurate field-level error mapping

The save! issue flagged in previous reviews has been addressed.


55-71: LGTM!

The update logic mirrors the add pattern with appropriate ID-based lookup:

  • Validates before saving to catch model-level errors
  • Gracefully handles save failures by collecting errors
  • Skips gracefully when holiday_info is not found
  • Maintains consistency with the error collection approach

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
config/initializers/ferrum_pdf.rb (1)

51-69: Fix syntax error in browser_path assignment.

Line 67 has incomplete syntax: browser_path: is not assigned a value.

🔎 Proposed fix
     FerrumPdf.browser = Ferrum::Browser.new(
       headless: true, # Run browser in headless mode
       timeout: 60, # Maximum time to wait for browser operations
       process_timeout: 60, # Maximum time to wait for browser process to start
-      browser_path: # Use the auto-detected browser path
+      browser_path: browser_path # Use the auto-detected browser path
     )
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3610e8c and 8f2874f.

⛔ Files ignored due to path filters (1)
  • Gemfile.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • .devcontainer/Dockerfile
  • .ruby-version
  • Dockerfile.local
  • Dockerfile.test
  • Gemfile
  • config/initializers/ferrum_pdf.rb
  • deployment/fly/Dockerfile
  • docker-compose.yml
  • docs/docs/contributing-guide/setup/macos.md
  • spec/models/timeoff_entry_spec.rb
✅ Files skipped from review due to trivial changes (1)
  • .ruby-version
⏰ 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: Running tests
🔇 Additional comments (14)
spec/models/timeoff_entry_spec.rb (3)

8-22: LGTM! Good use of deterministic dates.

The fixture setup correctly configures no_of_allowed_optional_holidays: 1 and uses beginning_of_year + N.days for predictable, quarter-aligned test dates. The addition of third_optional_holiday properly enables testing the "exceeded maximum" validation.

Minor style note: the create calls inside the let blocks (lines 11, 16, 21) have inconsistent indentation compared to the surrounding code, but this is cosmetic.


30-79: LGTM! Well-structured test setup with clear comments.

The test setup correctly covers all validation scenarios:

  • First optional holiday saves successfully.
  • Same-day conflict with a different optional holiday.
  • Duplicate application of the same holiday on a different day.
  • Exceeding the maximum (1) allowed optional holidays with a third holiday.

The descriptive comments on lines 31, 41, 51, and 61 clarify the intent of each test case.


114-118: LGTM! Validation test correctly verifies the max optional holidays constraint.

The test properly validates that attempting to add a third optional holiday (when no_of_allowed_optional_holidays: 1 and one entry already exists) triggers the expected error.

docs/docs/contributing-guide/setup/macos.md (1)

22-26: Documentation update aligns with Ruby version upgrade.

The documentation correctly reflects the Ruby 3.4.8 upgrade, ensuring consistency with the infrastructure changes.

deployment/fly/Dockerfile (1)

2-2: Deployment configuration updated consistently.

The Ruby version update in the deployment Dockerfile aligns with the broader version upgrade across the repository.

Dockerfile.local (1)

2-2: Local development environment updated consistently.

The base image update aligns with the repository-wide Ruby 3.4.8 upgrade.

Gemfile (3)

142-143: Correct addition of csv and observer gems for Ruby 3.4+.

In Ruby 3.4+, csv and observer were removed from the default gems and must be added explicitly. This change is necessary and correct.


6-6: All gems are compatible with Ruby 3.4.8.

The Gemfile.lock confirms that all dependencies, including those with native extensions (pg, puma, nokogiri, image_processing), have been resolved and are compatible with Ruby 3.4.8p72.


82-82: Version 1.208.0 exists and is secure.

Version 1.208.0 of aws-sdk-s3 is confirmed on RubyGems and is the fix for CVE-2025-14762 (Medium severity, missing cryptographic key commitment in S3 Encryption Client). The constraint >= 1.208.0 appropriately protects against this vulnerability which affected earlier versions.

.devcontainer/Dockerfile (1)

2-2: Development container updated consistently.

The devcontainer configuration aligns with the Ruby 3.4.8 upgrade across the repository.

config/initializers/ferrum_pdf.rb (1)

32-49: Docker container support added appropriately.

Expanding the browser initialization to include Docker environments (via ENV["DOCKER_CONTAINER"]) aligns well with the docker-compose.yml changes and ensures PDF generation works correctly in containerized development.

docker-compose.yml (2)

32-32: DOCKER_CONTAINER flag enables PDF generation in containers.

Adding DOCKER_CONTAINER=true to the app service correctly signals the FerrumPdf initializer to use container-appropriate browser configuration for PDF generation.


54-54: Webpack service configured consistently.

The DOCKER_CONTAINER=true environment variable is added to the webpack service for consistency, though it primarily benefits the app service's PDF generation functionality.

Dockerfile.test (1)

2-2: Ruby 3.4.8 is a valid, stable release with no compatibility issues.

Ruby 3.4.8 was released December 17, 2025, and is officially available. The project's Gemfile explicitly specifies ruby "3.4.8" and all dependencies, including Rails 7.1.5.1, are compatible with this version. No breaking changes or version conflicts detected.

@amolmjoshi93 amolmjoshi93 requested a review from nisusam January 5, 2026 11:59
amolmjoshi93 added a commit that referenced this pull request Jan 14, 2026
Apply fix from PR #2028 - when net_days is 0, show only hours
instead of '0 days X hours' format.
…tation

- Add PDF generation stub helper to prevent Ferrum timeout in tests
  * Create spec/support/pdf_generation_helpers.rb to stub FerrumPdf.render_pdf
  * Fixes 10 failing specs related to invoice mailers, downloads, and reports
  * Tests no longer require headless Chrome to generate actual PDFs

- Fix timeoff entries index service spec expectation
  * Correct label calculation in spec/services/timeoff_entries/index_service_spec.rb:232
  * Test was expecting "Overdrawn by" label but calculation showed positive balance
  * Update test to dynamically calculate label based on actual net_hours value
- Update httparty from 0.21.0 to 0.24.2
  * Fixes CVE-2025-68696: SSRF vulnerability that could lead to API key leakage
  * GHSA: GHSA-hm5p-x4rq-38w4

- Update shakapacker from 6.0.0 to 9.5.0
  * Fixes GHSA-96qw-h329-v5rg: Environment variable leak via EnvironmentPlugin
  * Prevents secrets from being exposed to client-side bundles

All security vulnerabilities resolved per bundle-audit scan.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@Gemfile`:
- Line 57: Replace the open-ended version constraint on the gem "shakapacker" in
the Gemfile with a pessimistic (tilde) constraint so it allows patch/minor
updates within the 9.x series but prevents accidental upgrades to major version
10; locate the gem declaration gem "shakapacker" and update its version
specifier accordingly to lock to 9.x while still permitting 9.5.0+
patches/minors.
🧹 Nitpick comments (2)
Gemfile (2)

82-82: Consider using pessimistic version constraint for aws-sdk-s3.

The open-ended >= constraint could introduce breaking changes from future major releases. AWS SDK follows semantic versioning, so a pessimistic constraint would be safer.

-gem "aws-sdk-s3", ">= 1.208.0", require: false
+gem "aws-sdk-s3", "~> 1.208", require: false

140-140: Consider using pessimistic version constraint for httparty.

Same concern with open-ended constraints—future major versions could introduce breaking changes.

-gem "httparty", ">= 0.24.0"
+gem "httparty", "~> 0.24"


# Webpack bundler for rails
gem "shakapacker", "6.0.0"
gem "shakapacker", ">= 9.5.0"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

shakapacker 9.5 migration guide breaking changes version 6

💡 Result:

Summary: key breaking changes and migration steps when moving from Shakapacker v6 → v9.5.

What changed (most important):

  • Renamed “webpacker” → “shakapacker” across config, env vars, binstubs, rake tasks, and helpers (config/webpacker.yml → config/shakapacker.yml, WEBPACKER_* → SHAKAPACKER_*). [1][4]
  • You must upgrade both the Ruby gem and the npm package (keep versions aligned and run bundle update + your JS package manager). [2][3]
  • Default webpack config API changed: replace old webpackConfig usage with generateWebpackConfig() (or use globalMutableWebpackConfig to retain v6 behavior). [4]
  • Static asset paths: top-level directories are no longer stripped — update image_pack_tag/asset references to include the top-level folder (e.g., images/ or static/images/). [4]
  • SWC/transform behavior: newer v9.x releases include SWC-related configuration changes (users of SWC loose mode should review the SWC breaking-change notes in the v9 docs). [1]
  • Security/ENV exposure fix in v9.5: default build no longer exposes all process.env to client bundles — only NODE_ENV, RAILS_ENV, WEBPACK_SERVE are exposed; public vars must use SHAKAPACKER_PUBLIC_ prefix. Rotate any secrets if previously exposed. (Upgrade to v9.5.0+ recommended.) [2][7]

Basic migration steps:

  1. Update gem and npm package to matching shakapacker versions (use exact versions, not broad ranges). Run bundle update shakapacker and npm/yarn/pnpm install. [2][3]
  2. Run the install/binstub task to regenerate files: rails shakapacker:install or rake shakapacker:install (and run rake shakapacker:binstubs if present), then remove old bin/webpacker* files. [4][5]
  3. Rename config and environment variables (config/webpacker.yml → config/shakapacker.yml; WEBPACKER_* → SHAKAPACKER_*). Search/replace Ruby constant Webpacker → Shakapacker where used. [4]
  4. Update code that imported webpackConfig to call generateWebpackConfig() (or import globalMutableWebpackConfig). [4]
  5. Update asset helper references for any top-level static directories. [4]
  6. Review SWC and env-exposure settings; explicitly allow public env vars via SHAKAPACKER_PUBLIC_* if needed and rotate any secrets if you were on versions before v9.5. [1][7]
  7. Run full test/build: rm -rf node_modules/.cache, bundle exec rails assets:precompile, run dev server/bin tasks and test app. [1][3]

Docs / references:

  • Shakacode docs — v7/v9 upgrade notes & guidance. [1][4]
  • GitHub shakapacker repo (upgrade/installation guidance). [5]
  • Rubygems/npm release info showing v9.5 and release notes. [2][3]
  • Security advisory / v9.5 note (env exposure fix). [7]

If you want, I can fetch and paste exact changelog lines for v9.5 and the SWC breaking-change section.


Tighten version constraint for shakapacker.

The open-ended >= 9.5.0 constraint could pull in future major versions (e.g., v10) with additional breaking changes. Use the pessimistic constraint operator instead:

-gem "shakapacker", ">= 9.5.0"
+gem "shakapacker", "~> 9.5.0"

This constrains updates to compatible versions (v9.5.0 through v9.x) while still allowing patch and minor releases within the v9 series.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
gem "shakapacker", ">= 9.5.0"
gem "shakapacker", "~> 9.5.0"
🤖 Prompt for AI Agents
In `@Gemfile` at line 57, Replace the open-ended version constraint on the gem
"shakapacker" in the Gemfile with a pessimistic (tilde) constraint so it allows
patch/minor updates within the 9.x series but prevents accidental upgrades to
major version 10; locate the gem declaration gem "shakapacker" and update its
version specifier accordingly to lock to 9.x while still permitting 9.5.0+
patches/minors.

…-not-allowed-in-holiday-list

 # Conflicts:
 #	Gemfile
 #	Gemfile.lock
 #	app/services/timeoff_entries/index_service.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Special Chars not allowed in holiday list

2 participants