Fix/booking timezone static services#17
Closed
Tung-Quan wants to merge 2 commits into
Closed
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe PR introduces a timezone-aware appointment scheduling system and refactors the web Services page. A new ChangesTimezone-Aware Appointment System
Web App Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/services/_tests_/appointment.service.test.ts (1)
280-296:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake timezone explicit in this suite to prevent env-coupled test behavior.
This expectation now encodes timezone semantics, so set
process.env.APP_TIMEZONE = "America/Chicago"in suite setup (and restore after) to avoid cross-test/env drift.🤖 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 `@apps/api/src/services/_tests_/appointment.service.test.ts` around lines 280 - 296, Tests for findConfirmedAppointmentsForCheckIn rely on a specific timezone; set process.env.APP_TIMEZONE = "America/Chicago" in this describe block's setup and restore the original value in teardown to avoid env-coupled failures. Use Jest hooks (beforeAll/afterAll or beforeEach/afterEach) inside the describe("findConfirmedAppointmentsForCheckIn", ...) to assign process.env.APP_TIMEZONE, call appointmentService.findConfirmedAppointmentsForCheckIn(...) as-is, and then restore the previous process.env.APP_TIMEZONE value so other tests are unaffected.
🧹 Nitpick comments (2)
apps/api/src/utils/_tests_/timezone.util.test.ts (1)
18-24: ⚡ Quick winAdd DST boundary assertions for conversion safety.
Given
localDateTimeToDate/getLocalDayRangecomplexity, one standard-date case is thin. Add spring/fall DST boundary tests (23h/25h local day) to catch regressions early.🤖 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 `@apps/api/src/utils/_tests_/timezone.util.test.ts` around lines 18 - 24, Add tests that assert behavior across DST boundary days for parseLocalDateTimeToDate and related helpers: create two new test cases (spring-forward day producing a 23-hour local day and fall-back day producing a 25-hour local day) that call parseLocalDateTimeToDate/localDateTimeToDate and also use getLocalDayRange to assert the day length (23 or 25 hours) and verify the parsed Date still formats to the correct local date/time via formatLocalDate/formatLocalTime; reference the functions parseLocalDateTimeToDate, localDateTimeToDate, and getLocalDayRange so reviewers can find where to add the assertions.apps/api/src/controllers/appointment.controller.ts (1)
103-108: ⚡ Quick winAdd
timeZoneoption toIntl.DateTimeFormatinstead of re-parsing the date string.The code unnecessarily converts the date to a formatted string via
formatLocalDate(), then parses it back into a Date object just to reformat it. This double-conversion is inefficient and couples the timezone handling indirectly.Instead, directly format the date with the timezone option:
date: new Intl.DateTimeFormat("en-GB", { day: "2-digit", month: "2-digit", year: "numeric", timeZone: getAppTimeZone(), }).format(scheduledAt), time: formatLocalTime(scheduledAt),This is simpler, avoids unnecessary parsing, and makes the timezone handling explicit.
🤖 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 `@apps/api/src/controllers/appointment.controller.ts` around lines 103 - 108, The current code re-formats scheduledAt by calling formatLocalDate() and parsing that string into a Date before using Intl.DateTimeFormat, which is inefficient and implicit about timezone handling; update the date formatting to call Intl.DateTimeFormat directly with scheduledAt and add the timeZone option (use getAppTimeZone()) instead of re-parsing, and keep time formatted via formatLocalTime(scheduledAt); specifically change the block that uses formatLocalDate(scheduledAt) to call new Intl.DateTimeFormat("en-GB", { day: "2-digit", month: "2-digit", year: "numeric", timeZone: getAppTimeZone() }).format(scheduledAt) while leaving formatLocalTime(scheduledAt) intact.
🤖 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 `@apps/web/src/pages/Services.tsx`:
- Around line 24-93: serviceCategories no longer includes a category with title
"beverage", which makes the derived beverageCategory and the BeverageSection
render branch unreachable; either add a beverage entry to the static catalog
(e.g., include an object in serviceCategories with title: 'beverage' and
appropriate items built via serviceItem) or remove the beverageCategory
derivation and the BeverageSection render branch (references: serviceCategories,
beverageCategory, BeverageSection, and the string 'beverage') so the UI and data
model stay in sync.
---
Outside diff comments:
In `@apps/api/src/services/_tests_/appointment.service.test.ts`:
- Around line 280-296: Tests for findConfirmedAppointmentsForCheckIn rely on a
specific timezone; set process.env.APP_TIMEZONE = "America/Chicago" in this
describe block's setup and restore the original value in teardown to avoid
env-coupled failures. Use Jest hooks (beforeAll/afterAll or
beforeEach/afterEach) inside the describe("findConfirmedAppointmentsForCheckIn",
...) to assign process.env.APP_TIMEZONE, call
appointmentService.findConfirmedAppointmentsForCheckIn(...) as-is, and then
restore the previous process.env.APP_TIMEZONE value so other tests are
unaffected.
---
Nitpick comments:
In `@apps/api/src/controllers/appointment.controller.ts`:
- Around line 103-108: The current code re-formats scheduledAt by calling
formatLocalDate() and parsing that string into a Date before using
Intl.DateTimeFormat, which is inefficient and implicit about timezone handling;
update the date formatting to call Intl.DateTimeFormat directly with scheduledAt
and add the timeZone option (use getAppTimeZone()) instead of re-parsing, and
keep time formatted via formatLocalTime(scheduledAt); specifically change the
block that uses formatLocalDate(scheduledAt) to call new
Intl.DateTimeFormat("en-GB", { day: "2-digit", month: "2-digit", year:
"numeric", timeZone: getAppTimeZone() }).format(scheduledAt) while leaving
formatLocalTime(scheduledAt) intact.
In `@apps/api/src/utils/_tests_/timezone.util.test.ts`:
- Around line 18-24: Add tests that assert behavior across DST boundary days for
parseLocalDateTimeToDate and related helpers: create two new test cases
(spring-forward day producing a 23-hour local day and fall-back day producing a
25-hour local day) that call parseLocalDateTimeToDate/localDateTimeToDate and
also use getLocalDayRange to assert the day length (23 or 25 hours) and verify
the parsed Date still formats to the correct local date/time via
formatLocalDate/formatLocalTime; reference the functions
parseLocalDateTimeToDate, localDateTimeToDate, and getLocalDayRange so reviewers
can find where to add the assertions.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17a2d4f8-2081-4874-ac3b-ee08d0c343f1
📒 Files selected for processing (11)
AGENTS.mdapps/api/src/controllers/appointment.controller.tsapps/api/src/services/_tests_/appointment.service.test.tsapps/api/src/services/appointment.service.tsapps/api/src/utils/_tests_/timezone.util.test.tsapps/api/src/utils/timezone.util.tsapps/web/src/components/services/ServiceCategory.tsxapps/web/src/pages/Services.tsxapps/web/src/utils/dateStringHandler.tsdeploy/lightsail/api.env.exampledocs/deploy-lightsail.md
💤 Files with no reviewable changes (1)
- apps/web/src/components/services/ServiceCategory.tsx
Comment on lines
+24
to
+93
| const serviceCategories: ServiceListCategory[] = [ | ||
| { | ||
| title: 'Pedicure', | ||
| items: [ | ||
| serviceItem('Day-by-Day pedicure', '$30'), | ||
| serviceItem('Hydra Glow pedicure', '$45'), | ||
| serviceItem('The Stella pedicure', '$65'), | ||
| serviceItem('Cosmic Veil pedicure', '$75'), | ||
| serviceItem('Seasonal pedicure', '$65'), | ||
| serviceItem('Star Dust pedicure', '$95'), | ||
| serviceItem('Additional Gel polish', '(+ $20)'), | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Manicure', | ||
| items: [ | ||
| serviceItem('Day-by-Day manicure', '$20'), | ||
| serviceItem('Hydra Glow manicure', '$35'), | ||
| serviceItem('The Stella manicure', '$50'), | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Nail Enhancement', | ||
| items: [ | ||
| serviceItem('Dipping Powder', '$40+'), | ||
| serviceItem('Acrylic Fullset', '$50+'), | ||
| serviceItem('Acrylic Fill', '$40+'), | ||
| serviceItem('Gel Manicure', '$40'), | ||
| serviceItem('Gel X', '$65+'), | ||
| serviceItem('Builder Gel', '$60'), | ||
| serviceItem('Tap Gel', '$60'), | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Add-on services', | ||
| items: [ | ||
| serviceItem('French Tip', '$15+'), | ||
| serviceItem('Length', '$5+'), | ||
| serviceItem('Shaping', '$5+'), | ||
| serviceItem('Trim/reshape', '$10+'), | ||
| serviceItem('Soak off', '$15+'), | ||
| serviceItem('Regular polish change', '$15'), | ||
| serviceItem('Gel polish change', '$30'), | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Waxing services', | ||
| items: [ | ||
| serviceItem('Eyebrows', '$12'), | ||
| serviceItem('Lip', '$10'), | ||
| serviceItem('Chin', '$10+'), | ||
| serviceItem('Sideburn', '$15+'), | ||
| serviceItem('Full Face', '$35+'), | ||
| serviceItem('Under arms', '$20+'), | ||
| serviceItem('Half Arms', '$30+'), | ||
| serviceItem('Full Arms', '$40+'), | ||
| serviceItem('Half Legs', '$40+'), | ||
| serviceItem('Full Legs', '$55+'), | ||
| ], | ||
| }, | ||
| { | ||
| title: 'Kids services', | ||
| items: [ | ||
| serviceItem('Kids pedicure', '$20'), | ||
| serviceItem('Kids manicure', '$15'), | ||
| serviceItem('Kids hands polish change', '$8'), | ||
| serviceItem('Kids toe polish change', '$10'), | ||
| ], | ||
| }, | ||
| ] |
There was a problem hiding this comment.
Add the missing beverage data or remove the unreachable section.
beverageCategory is still derived and rendered later, but this static catalog never defines any category whose title matches "beverage". That means the BeverageSection branch at Lines 137-145 is now dead and the page silently drops that section after this refactor.
🤖 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 `@apps/web/src/pages/Services.tsx` around lines 24 - 93, serviceCategories no
longer includes a category with title "beverage", which makes the derived
beverageCategory and the BeverageSection render branch unreachable; either add a
beverage entry to the static catalog (e.g., include an object in
serviceCategories with title: 'beverage' and appropriate items built via
serviceItem) or remove the beverageCategory derivation and the BeverageSection
render branch (references: serviceCategories, beverageCategory, BeverageSection,
and the string 'beverage') so the UI and data model stay in sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Changes
Configuration
APP_TIMEZONEis set in deployment environment files.