Replace moment.js with date-fns v3 compatibility shim#93
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces ion2-calendar modals with Ionic AlertController-based single-date and date-range pickers, adds shared alert helpers, adds a local date-fns shim, remaps ChangesCalendar Replacement and date-fns Migration
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant presentDateAlert
participant AlertController
participant dateFnsShim
User->>Component: click single-date selector
Component->>presentDateAlert: presentDateAlert(alertCtrl, defaultDate)
presentDateAlert->>AlertController: create alert with date input
AlertController->>User: show date input dialog
User->>AlertController: select date and confirm
AlertController->>presentDateAlert: onDidDismiss(role: 'confirm', data)
presentDateAlert->>dateFnsShim: normalize/format selected date
presentDateAlert->>Component: return date string
Component->>Component: update form control/state, call service
sequenceDiagram
participant User
participant Component
participant presentDateRangeAlert
participant AlertController
participant dateFnsShim
User->>Component: click date-range selector
Component->>presentDateRangeAlert: presentDateRangeAlert(alertCtrl, defaultRange)
presentDateRangeAlert->>AlertController: create alert with from/to inputs
AlertController->>User: show range input dialog
User->>AlertController: select from & to, confirm
AlertController->>presentDateRangeAlert: onDidDismiss(role: 'confirm', data)
presentDateRangeAlert->>dateFnsShim: normalize from/to dates
presentDateRangeAlert->>Component: return { from, to } or null
Component->>Component: set from_date/to_date controls and range
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related Issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/app/pages/logged-in/candidate/candidate-view/candidate-view.page.ts (1)
1724-1726: Ensure date format consistency and use backend response if provided.The date returned from
presentDateAlertis in ISO 8601 format (YYYY-MM-DD) from the HTML date input and is passed directly toupdateCivilExpiry(line 1724). While this format is standard and likely matches backend expectations, the code should:
- Verify that the
YYYY-MM-DDformat from the date picker matches what the backend API expects for the civil expiry date field- If the backend returns a normalized/formatted date in the response, use that value instead of assigning the input
datedirectly tocandidate_civil_expiry_date(line 1726)Patterns elsewhere in the codebase use explicit formatting like
format(parseISO(date), 'yyyy-MM-dd')before sending dates to the backend, which would make the format expectation clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/pages/logged-in/candidate/candidate-view/candidate-view.page.ts` around lines 1724 - 1726, The date from presentDateAlert (HTML input) should be normalized and the backend's response used when available: before calling candidateService.updateCivilExpiry(parse/format the selected date to the expected ISO 'yyyy-MM-dd' format—e.g., via parseISO/format or equivalent—and pass that to updateCivilExpiry; then, in the subscribe callback, if the backend returns a canonical/normalized date in res (e.g., res.candidate_civil_expiry_date or res.data), assign that returned value to this.candidate.candidate_civil_expiry_date instead of directly using the local input date, falling back to the formatted input only if the response does not include a date.src/app/util/date-alert.ts (1)
21-50: ⚡ Quick winConsider validating that
from≤toin date range selection.The function returns the selected date range without validating that the start date comes before the end date. Users can select invalid ranges, which downstream code must handle.
Consider adding validation before returning:
Optional: Add range validation
if (result.role !== 'confirm' || !values?.from || !values?.to) { return null; } + // Validate date order + if (values.from > values.to) { + // Option 1: Swap them + return { from: values.to, to: values.from }; + // Option 2: Return null to reject invalid range + // return null; + } return { from: values.from, to: values.to, };Choose between auto-swapping (better UX) or rejecting invalid ranges (explicit validation).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/util/date-alert.ts` around lines 21 - 50, presentDateRangeAlert currently returns the picked values without checking that the start is before the end; update presentDateRangeAlert to parse/normalize values.from and values.to into Date objects (reuse toInputDate or create parse helper) and enforce from <= to: either auto-swap the two values before returning or reject by returning null and showing an error toast/alert. Ensure the function still returns DateRangeSelection with normalized Date instances (not raw strings) and handle any invalid parse cases by treating them as null.package.json (1)
57-57: Consider evaluating an upgrade to date-fns@4.1.0 when compatible.Version 3.6.0 has no known security vulnerabilities. However, a newer major version (4.1.0) is available. The current caret constraint (^3.6.0) restricts updates to the 3.x series. Review the changelog for breaking changes and compatibility implications before upgrading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 57, Update the package.json dependency constraint for "date-fns" from "^3.6.0" to "^4.1.0" and run your package installer (npm/yarn/pnpm) to update lockfiles; then run the test and build suite and fix any breaking API/type changes that surface (search for usages of date-fns functions across the codebase and adjust calls/imports per the date-fns v4 changelog). Ensure to commit the updated package.json and lockfile only after tests pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/components/date-range-refinement-list/date-range-refinement-list.component.ts`:
- Line 220: Replace loose equality comparisons with strict equality in
date-range-refinement-list.component: change any occurrences comparing the
variable named "string" to 'l' (e.g., if (string =='l')) to use === (if (string
=== 'l')) in the functions/methods where that check appears (lines around the
comparison and the similar occurrence at the other location). Ensure both
instances are updated to avoid type-coercion bugs.
In `@src/app/pages/logged-in/candidate/candidate-view/candidate-view.page.ts`:
- Line 1719: presentDateAlert is being called with
this.candidate.candidate_civil_expiry_date without checking for null/undefined;
update the call site in candidate-view.page.ts so you first check if
this.candidate and this.candidate.candidate_civil_expiry_date exist and only
pass a valid Date (or undefined/null fallback) to presentDateAlert (e.g.,
compute const defaultDate = this.candidate?.candidate_civil_expiry_date ? new
Date(this.candidate.candidate_civil_expiry_date) : undefined; then call
presentDateAlert(this.alertCtrl, defaultDate)) to avoid passing a null/undefined
value directly.
In `@src/app/util/date-alert.ts`:
- Around line 9-19: The toInputDate function currently blindly replaces hyphens
in any string input which breaks ISO datetime strings like
"2024-01-15T10:30:00"; change the logic in toInputDate so you only replace
hyphens when the input is a date-only string (e.g. match /^\d{4}-\d{2}-\d{2}$/)
and leave other strings (ISO datetimes) untouched before passing to new Date;
keep the rest of the flow (fallback to today on invalid date) unchanged.
In `@src/app/util/date-fns-shim.ts`:
- Around line 33-43: The current format function's sequential .replace calls (in
the function named format) can corrupt literal text (e.g., "dd days") because
short-token replacements run after longer ones; fix this by doing a single
global replacement that matches tokens as a single pass: build a regex like
/(yyyy|MMM|MM|dd|d|HH|hh|mm|ss|a)/g and replace via a mapping callback that
returns the correct string for each token (compute monthShort, pad, twelveHour,
etc. beforehand and use the matched token to look up the formatted piece). This
preserves literal text and avoids the sequential-replace corruption.
---
Nitpick comments:
In `@package.json`:
- Line 57: Update the package.json dependency constraint for "date-fns" from
"^3.6.0" to "^4.1.0" and run your package installer (npm/yarn/pnpm) to update
lockfiles; then run the test and build suite and fix any breaking API/type
changes that surface (search for usages of date-fns functions across the
codebase and adjust calls/imports per the date-fns v4 changelog). Ensure to
commit the updated package.json and lockfile only after tests pass.
In `@src/app/pages/logged-in/candidate/candidate-view/candidate-view.page.ts`:
- Around line 1724-1726: The date from presentDateAlert (HTML input) should be
normalized and the backend's response used when available: before calling
candidateService.updateCivilExpiry(parse/format the selected date to the
expected ISO 'yyyy-MM-dd' format—e.g., via parseISO/format or equivalent—and
pass that to updateCivilExpiry; then, in the subscribe callback, if the backend
returns a canonical/normalized date in res (e.g.,
res.candidate_civil_expiry_date or res.data), assign that returned value to
this.candidate.candidate_civil_expiry_date instead of directly using the local
input date, falling back to the formatted input only if the response does not
include a date.
In `@src/app/util/date-alert.ts`:
- Around line 21-50: presentDateRangeAlert currently returns the picked values
without checking that the start is before the end; update presentDateRangeAlert
to parse/normalize values.from and values.to into Date objects (reuse
toInputDate or create parse helper) and enforce from <= to: either auto-swap the
two values before returning or reject by returning null and showing an error
toast/alert. Ensure the function still returns DateRangeSelection with
normalized Date instances (not raw strings) and handle any invalid parse cases
by treating them as null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02efa9c3-75f9-41e0-8682-e994322caf4f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/app/app.module.tssrc/app/components/date-range-refinement-list/date-range-refinement-list.component.tssrc/app/pages/logged-in/candidate/candidate-view/candidate-view.page.tssrc/app/pages/logged-in/leave-request/leave-request.page.tssrc/app/pages/logged-in/leave/leave-request-form/leave-request-form.page.tssrc/app/pages/logged-in/transfer/transfer-form/transfer-form.page.tssrc/app/util/date-alert.tssrc/app/util/date-fns-shim.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/app/app.module.ts
|
I addressed the CodeRabbit feedback in commits 299e4e6 and 56dc940.\n\nChanges after review:\n- Replaced the loose date-range selector comparisons with strict equality.\n- Made the candidate civil-expiry date picker null-safe.\n- Normalized the selected civil-expiry date to yyyy-MM-dd before sending it to the backend, and now prefer a canonical date from the response when present.\n- Fixed date-alert parsing so date-only strings are parsed as local dates without breaking ISO datetime strings.\n- Updated date range selection to normalize values and auto-swap from/to if the user selects them in reverse order.\n- Reworked the date-fns format shim to replace tokens in a single pass, avoiding literal-text corruption from sequential replacements.\n\nVerification on the updated head:\n- npx tsc -p tsconfig.app.json --noEmit --pretty false\n- grep -R -E 'moment|ion2-calendar' -n src package.json package-lock.json returned no matches\n- git diff --check\n\nI intentionally kept date-fns at ^3.6.0 because issue #31 explicitly asks for date-fns v3; moving to v4 would be outside this PR's scope. Could you please re-check when the review rate limit clears? |
|
Update after the latest fixes: CodeRabbit is now passing, all 4 review threads are resolved, and the PR is mergeable from the fork branch. I don't have permission to merge into the protected upstream master branch, so this is ready for maintainer review/merge when available. |
|
Addressed the remaining CodeRabbit feedback in commits 299e4e6 and 56dc940. The latest head keeps the PR on my fork branch (sypham98-prog/studenthub-staff:fix/issue-31-replace-moment-date-fns), so no push to the protected upstream branch is needed. Fixed items:
Re-verified on the current head:
@coderabbitai review |
|
(ﾐ^ ω ^ﾐ)✨ ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/util/date-alert.ts`:
- Around line 9-25: parseDateInput duplicates DATE_ONLY_RE parsing logic; remove
the custom regex branch and reuse the shim's parseISO helper instead: keep the
Date instance fast-path (if value instanceof Date return value), then call
parseISO(String(value)) for all non-Date inputs (or otherwise use the existing
date-fns toDate/parseISO exported helper) and return that result, and delete
DATE_ONLY_RE and the manual new Date(...) construction; reference functions
parseDateInput and parseISO.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4021d4fe-4c70-4c31-b418-2dbeca2e7451
📒 Files selected for processing (4)
src/app/components/date-range-refinement-list/date-range-refinement-list.component.tssrc/app/pages/logged-in/candidate/candidate-view/candidate-view.page.tssrc/app/util/date-alert.tssrc/app/util/date-fns-shim.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/pages/logged-in/candidate/candidate-view/candidate-view.page.ts
- src/app/components/date-range-refinement-list/date-range-refinement-list.component.ts
|
Addressed the latest CodeRabbit parsing refactor in 0e704cc. date-alert now reuses the date-fns shim parseISO helper instead of duplicating DATE_ONLY_RE parsing, and the shim parseISO signature accepts Date | number | string so existing date/timestamp inputs remain supported. Verification: npx tsc -p tsconfig.app.json --noEmit --pretty false; git diff --check; grep -R -n -E 'moment|ion2-calendar' src package.json package-lock.json returned no matches. @coderabbitai review |
|
(ﾐ≧ ω ≦ﾐ)✨ ✅ Actions performedReview triggered.
|
1 participant
Fixes #31.\n\nSummary:\n- Upgrades date-fns dependency to ^3.6.0 and removes moment plus ion2-calendar from package files.\n- Removes remaining ion2-calendar usages by using Ionic alert-based date/date-range helpers.\n- Adds a local date-fns compatibility shim mapped through tsconfig so the Angular 15 / TypeScript 4.8 app can compile while package.json carries date-fns v3.\n\nVerification:\n- grep -R -E 'moment|ion2-calendar' -n src package.json package-lock.json returned no matches.\n- npx tsc -p tsconfig.app.json --noEmit --pretty false passed.\n- git diff --check passed.\n\nNote: full ng build was attempted with progress disabled, but it exceeded 300 seconds in this WSL environment before completion.
Summary by CodeRabbit
New Features
Chores