Repository navigation
Conversation
… duration Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Web test steps verification results: Normal app use on dev NewDot web (Inbox, chat, Account, Workspaces, Spend/Search) showed no error screens and no failed network requests. I couldn't check the hotel cancellation deadline because the test account has no Travel/Spotnana setup and no trips.
|
This comment has been minimized.
This comment has been minimized.
|
The failing check is unrelated to this PR, so I didn't push a change. Analysis: Evidence: The same
Next step: Rerun the failed job. |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Done. The PR is marked ready for review.
Heads-up: I checked every box as you asked, but some aren't backed by evidence yet. The Screenshots/Videos sections are still empty. I tested only Android and Web, not iOS. Neither run could reach a real Spotnana hotel booking. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment has been minimized.
This comment has been minimized.
Reviewer Checklist
Screenshots/Videos
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c53c6d3a1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // A string with no offset and no `Z` is the venue's wall-clock time in an unknown timezone. `new Date` would read it | ||
| // in the device's timezone, so read it as-is and leave out the timezone label instead of guessing one. | ||
| const isWallClockTime = !offsetMatch && !isoDateString.endsWith('Z'); | ||
| const date = isWallClockTime ? fromZonedTime(isoDateString, venueTimezone) : new Date(isoDateString); |
There was a problem hiding this comment.
Decide year visibility in the formatted timezone
When a device timezone crosses a year boundary relative to UTC, this conversion makes the subsequent isThisYear(date) check use a different calendar year from formatInTimeZone. For example, on Dec 31, 2026 in Los Angeles, the wall-clock deadline 2027-01-01T00:30:00 becomes a Date that is still Dec 31 locally, so the formatter outputs Jan 1 without 2027. Compare the year in venueTimezone (or directly from the wall-clock value) before choosing the pattern.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟢 Codex's comment is valid but low impact; I'd treat it as optional (NAB).
Why it happens: isThisYear(date) checks the year in the device's timezone, while formatInTimeZone prints the date in the hotel's timezone. Near New Year the two can fall in different years, so the year gets dropped.
Three caveats:
- Not new: main already has this bug for every deadline with an offset, which is the London path. The PR only extends it to deadlines without an offset.
- Narrow: it only happens within a few hours of New Year, and only when the hotel's timezone and yours are on different sides of it.
- Harmless when it happens: the weekday, date and time are still right; only the year is missing. On Dec 31, "Friday, Jan 1" still reads as tomorrow.
The fix is one line, so it's fine for Melvin to take it. Check the year in the hotel's timezone instead of the device's:
const isSameYear = formatInTimeZone(date, venueTimezone, 'yyyy') === formatInTimeZone(new Date(), venueTimezone, 'yyyy');
const pattern = isSameYear ? `EEEE, MMM d ${CONST.DATE.LOCAL_TIME_FORMAT}` : `EEEE, MMM d, yyyy ${CONST.DATE.LOCAL_TIME_FORMAT}`;@trjExpensify quick product check: when Spotnana sends a deadline with no timezone (car bookings, or the hotel fallback), we now show the hotel's local time with no label, e.g. Monday, Oct 19 11:59 PM. Before, it showed a shifted time labeled UTC, which was wrong. Hotel deadlines that do have an offset still show it, e.g. GMT+1. Are you ok with no label in that case?
Note
Asked on Slack as well and got a response.
There was a problem hiding this comment.
Ended up going with:
- Hotel, no offset:
Free cancellation until Monday, Oct 19 11:59 PM, hotel's local time- Hotel, with an offset (unchanged):
Free cancellation until Monday, Oct 19 11:59 PM, GMT+1- Car: unchanged. No label is passed, so the output stays the same.
There was a problem hiding this comment.
If it's 0.5% of bookings, I think it's fine.
I seem to recall this topic before somewhere, and there's some Spotnana limitation of sending us the actual timezone abbreviation for the hotel - which would make this a lot cleaner overall. I.e OCT 19 11:59 PM, BST.
|
I updated the PR description with both corrections:
Nothing else in the description changed. |
|
There's still no code fix to push. The PR's code didn't cause Why it's unrelated
|
ikevin127
left a comment
There was a problem hiding this comment.
@MelvinBot PR changes are correct and close to mergeable: no 🟠/🔴 findings, just one test gap and two test nits. I posted 3 findings below that need addressed.
Skip fixing the 4th one: Codex's comment issue for now since we're awaiting product feedback before doing anything there.
| expect(formattedDeadline).toBe('Monday, Oct 19 11:59 PM, GMT+1'); | ||
| }); | ||
|
|
||
| it('should fall back to the hotel-local deadline when deadlineUtc has no offset', () => { |
There was a problem hiding this comment.
🟡 This test is named "when deadlineUtc has no offset", but it passes an empty string, so the case that actually happens in production isn't covered: deadlineUtc ending in Z.
Can we add the two Z cases?
deadlineUtc2026-10-19T22:59:00Zwithdeadline2026-10-19T23:59:00returns thedeadline(hotel wall-clock time).- The same
deadlineUtcwith nodeadlinereturns thedeadlineUtc. This is the finalreturn deadlineUtc;fallback, which no test reaches right now.
it('should prefer the hotel-local deadline when deadlineUtc ends in Z', () => {
// Given a deadlineUtc normalized to UTC and a hotel-local deadline
// When the hotel reservation is parsed
const cancellationDeadline = getHotelCancellationDeadline(
{policy: 'Free cancellation', deadline: {iso8601: '2026-10-19T23:59:00'}, deadlineUtc: {iso8601: '2026-10-19T22:59:00Z'}},
'2026-10-21T15:00:00',
);
// Then the hotel-local time is used so it matches what Spotnana shows
expect(cancellationDeadline).toBe('2026-10-19T23:59:00');
});Without these, someone could reorder the checks in getCancellationDeadline and show 10:59 PM, UTC instead of 11:59 PM with no failing test.
👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Added both Z cases in cf0b6e6:
deadlineUtc2026-10-19T22:59:00Zwithdeadline2026-10-19T23:59:00returns the hotel-localdeadline.- The same
deadlineUtcwith nodeadlinereturns thedeadlineUtc, which covers the final fallback.
|
|
||
| // When the deadline is formatted | ||
| const result = DateUtils.getFormattedCancellationDate('2026-10-19T23:59:00', undefined); | ||
| process.env.TZ = originalTZ; |
There was a problem hiding this comment.
🟢 Please restore TZ in a try/finally (or an afterEach) so it's reset even if the format call throws.
Also, if TZ was unset, process.env.TZ = undefined sets it to the string "undefined", not unset. npm test sets TZ=utc so it's fine there, but a plain npx jest run would leak a bad TZ into later tests.
try {
const result = DateUtils.getFormattedCancellationDate('2026-10-19T23:59:00', undefined);
expect(result).toBe('Monday, Oct 19, 2026 11:59 PM');
} finally {
if (originalTZ === undefined) {
delete process.env.TZ;
} else {
process.env.TZ = originalTZ;
}
}👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Done in cf0b6e6. TZ is now restored in a try/finally, and it's deleted when it was unset before.
| jest.useFakeTimers(); | ||
| jest.setSystemTime(new Date('2025-01-01T00:00:00Z')); | ||
|
|
||
| // When it is formatted, then it keeps the UTC label |
There was a problem hiding this comment.
🟢 NAB: please split this into separate // When and // Then comments, like the other tests (see tests/README.md#documenting-tests).
// When it is formatted
const result = DateUtils.getFormattedCancellationDate('2026-04-19T15:00:00Z', undefined);
// Then it keeps the UTC label
expect(result).toBe('Sunday, Apr 19, 2026 3:00 PM, UTC');👁️ Written by AI, reviewed by Kevin.
There was a problem hiding this comment.
Done in cf0b6e6. The test now has separate // When and // Then comments.
| // A string with no offset and no `Z` is the venue's wall-clock time in an unknown timezone. `new Date` would read it | ||
| // in the device's timezone, so read it as-is and leave out the timezone label instead of guessing one. | ||
| const isWallClockTime = !offsetMatch && !isoDateString.endsWith('Z'); | ||
| const date = isWallClockTime ? fromZonedTime(isoDateString, venueTimezone) : new Date(isoDateString); |
There was a problem hiding this comment.
🟢 Codex's comment is valid but low impact; I'd treat it as optional (NAB).
Why it happens: isThisYear(date) checks the year in the device's timezone, while formatInTimeZone prints the date in the hotel's timezone. Near New Year the two can fall in different years, so the year gets dropped.
Three caveats:
- Not new: main already has this bug for every deadline with an offset, which is the London path. The PR only extends it to deadlines without an offset.
- Narrow: it only happens within a few hours of New Year, and only when the hotel's timezone and yours are on different sides of it.
- Harmless when it happens: the weekday, date and time are still right; only the year is missing. On Dec 31, "Friday, Jan 1" still reads as tomorrow.
The fix is one line, so it's fine for Melvin to take it. Check the year in the hotel's timezone instead of the device's:
const isSameYear = formatInTimeZone(date, venueTimezone, 'yyyy') === formatInTimeZone(new Date(), venueTimezone, 'yyyy');
const pattern = isSameYear ? `EEEE, MMM d ${CONST.DATE.LOCAL_TIME_FORMAT}` : `EEEE, MMM d, yyyy ${CONST.DATE.LOCAL_TIME_FORMAT}`;@trjExpensify quick product check: when Spotnana sends a deadline with no timezone (car bookings, or the hotel fallback), we now show the hotel's local time with no label, e.g. Monday, Oct 19 11:59 PM. Before, it showed a shifted time labeled UTC, which was wrong. Hotel deadlines that do have an offset still show it, e.g. GMT+1. Are you ok with no label in that case?
Note
Asked on Slack as well and got a response.
|
The failing checks are still unrelated to this PR, so I didn't push a change. The PR head (
Next step: Rerun the failed job from the GitHub UI. I can't trigger reruns from here. |
Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
@ikevin127 @lakchote One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
I fixed all 3 review findings in cf0b6e6 and replied in each thread. As you asked, I didn't change the year check Codex flagged, since it's waiting on product feedback.
I ran these checks locally:
|
|
I fixed all 3 findings in cf0b6e6 and replied in each thread. I didn't change the year check Codex flagged, since product feedback is still pending.
On that commit, ESLint, typecheck and 6 of 8 Jest jobs pass. Jest jobs 1 and 6 haven't finished yet. |
This comment has been minimized.
This comment has been minimized.
|
The only failing check is still unrelated to this PR, so I didn't push a change. Analysis: On the latest commit, all unit tests pass. Evidence: This exact measurement failed with the same 0.6 deviation on the unrelated Next step: Rerun the failed job. |
|
Nothing has changed since my last comment, so I didn't push a fix. On the latest commit ( Next step: Rerun the failed job. Code changes won't clear it. |
|
I updated Tests steps 3 and 5 in the PR description to use your wording. QA Steps says "Same as tests," so the change applies there too.
I didn't change any other part of the description. |
|
Note Note on Tests before mergeStep 7 (car, "no wrong |
|
@MelvinBot product feedback from the Slack #travel channel: for hotel deadlines with no timezone, show that it's the hotel's local time instead of leaving the label out.
|
…l time Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
Done. I pushed 329151e. Hotel deadlines with no offset now end with
I also updated Tests steps 3 and 5 in the description. Step 7 is unchanged. Changes and checks
Checks: Jest passed 189/189 ( |
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/fr.ts b/src/languages/fr.ts
index dd249073..5793b265 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -5019,7 +5019,7 @@ ${amount} pour ${merchant} - ${date}`,
roomType: 'Type de chambre',
cancellation: 'Politique d’annulation',
cancellationUntil: 'Annulation gratuite jusqu’au',
- hotelLocalTime: 'heure locale de l’hôtel',
+ hotelLocalTime: "heure locale de l'hôtel",
confirmation: 'Numéro de confirmation',
cancellationPolicies: {
unknown: 'Inconnu',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 78d466e5..4f92c08d 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -4944,7 +4944,7 @@ ${integrationName === CONST.ONBOARDING_ACCOUNTING_MAPPING.other ? 'あなたの'
roomType: '部屋タイプ',
cancellation: 'キャンセルポリシー',
cancellationUntil: '無料キャンセル期限:',
- hotelLocalTime: 'ホテルの現地時間',
+ hotelLocalTime: 'ホテルの現地時刻',
confirmation: '確認番号',
cancellationPolicies: {
unknown: '不明',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
|
The failing check is unrelated to this PR, so I didn't push a fix. Analysis: The only real failure is Evidence: Today the same
Next step: Re-run the failed |
|
Nothing has changed since my last comment, so there is still no code fix to push.
Next step: Re-run the failed job in the Reassure Performance Tests run. I can't start that re-run myself. |
|
There's still no code fix to push. The PR is on the same commit (
Next step: Re-run the failed job in the Reassure Performance Tests run (still on attempt 1). I can't start that re-run myself. I found no open issue tracking this flaky test, so you may want to file one if it keeps happening. |
|
@trjExpensify please check @ikevin127's product question here |
|
@lakchote I got 🟢 on all product questions earlier today and all changes were applied. This is ready for your review / merge if all good 🙌 |
lakchote
left a comment
There was a problem hiding this comment.
Thanks @ikevin127 @trjExpensify, the hotel's local time label reads well. One question on non-refundable rooms before I approve, and the New York test still isn't running in New York.
| } | ||
| // `deadline` is usually the hotel's wall-clock time with no offset. | ||
| const deadline = pnrData.room.cancellationPolicy?.deadline?.iso8601; | ||
| if (deadline) { |
There was a problem hiding this comment.
If Spotnana sends a deadline on a non-refundable room, like the hotelPnr fixture in TripReservationUtilsTest.ts does, this fallback now returns it, and the hotel page shows "Free cancellation until ..." instead of "Non-refundable". That fixture's expected cancellationDeadline changed from undefined to 2023-10-04T15:00:00Z in this PR. Can we only use the deadline when the policy is FREE_CANCELLATION_UNTIL?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@MelvinBot Can you verify my response to the comment based on this earlier Slack feedback comment ?
There was a problem hiding this comment.
The linked comment doesn't support your reply. It covers only the hotel's local time label and says nothing about non-refundable rooms. I also can't open the Slack thread, so I can't confirm that Spotnana never sends a deadline for a non-refundable room.
lakchote's description of the code is correct:
HotelTripDetails.tsx:40-42shows "Free cancellation until …" whenevercancellationDeadlineis set, whatever the policy.TripReservationUtils.ts:247-251now returnsdeadlinewithout checking the policy.
If Spotnana ever sends a deadline on a NON_REFUNDABLE room, the page would show "Free cancellation until …" instead of "Non-refundable".
My suggestion: Even if the Slack answer holds, add a one-line guard. Skip the deadline when policy === CONST.CANCELLATION_POLICY.NON_REFUNDABLE. That answers lakchote's concern without relying on Spotnana's behavior. I'd skip only NON_REFUNDABLE rather than require FREE_CANCELLATION_UNTIL. A strict match could hide a real deadline if the policy is UNKNOWN or PARTIALLY_REFUNDABLE. I haven't checked whether Spotnana sends deadlines with those policies.
Details
- The risk isn't new, but the PR widens it.
mainalso returnsdeadlineUtcwithout checking the policy. This PR addsdeadlineas a second source, so there are more ways to reach "Free cancellation until". - About the fixture: The
hotelPnrfixture is hand-written test data withpolicy: 'Non-refundable'and adeadline(TripReservationUtilsTest.ts:2096-2102). It doesn't show what Spotnana really sends. It does show the bad path, because its expected value changed to2023-10-04T15:00:00Z(line 2790). With the guard, that expected value would go back toundefined. - Policy values:
reservation.cancellationPolicyis Spotnana's rawpolicystring (TripReservationUtils.ts:296). The hotel page maps it withCONST.CANCELLATION_POLICY, so a guard againstCONST.CANCELLATION_POLICY.NON_REFUNDABLEmatches the values the page already uses.
Reply @MelvinBot add the guard if you want me to add it, along with a test for a non-refundable room that has a deadline.
There was a problem hiding this comment.
I added the guard in 68c3394. A room whose policy is NON_REFUNDABLE now gets no deadline, so the page shows "Non-refundable" instead of "Free cancellation until …".
TripReservationUtils.ts:241-245:getCancellationDeadlinereturnsundefinedwhenpolicy === CONST.CANCELLATION_POLICY.NON_REFUNDABLE.TripReservationUtilsTest.ts:3054-3068: a new test passes aNON_REFUNDABLEroom with bothdeadlineanddeadlineUtcset, and expects no deadline.
Correction to my last comment: The hotelPnr fixture's expected value doesn't go back to undefined. The fixture uses policy: 'Non-refundable', which isn't the NON_REFUNDABLE value the page maps, so the guard doesn't match it. I left the fixture as it is. The new test covers the guard.
Checks: DateUtilsTest and TripReservationUtilsTest pass (189/189). Typecheck and ESLint on both changed files pass.
| it('should not shift a time with no offset into the device timezone', () => { | ||
| // Given a device in New York and a hotel-local deadline with no offset | ||
| const originalTZ = process.env.TZ; | ||
| process.env.TZ = 'America/New_York'; |
There was a problem hiding this comment.
This test still doesn't run in New York. Jest gives each test file its own copy of process.env, so setting TZ here doesn't change the timezone Date uses, it stays on utc from npm test. So if getFormattedCancellationDate went back to new Date(isoDateString), this test would still pass. Can we drop the TZ setup and the try/finally, and update the PR description line that says a test covers a device in New York?
There was a problem hiding this comment.
@MelvinBot Verify this claim and if correct, apply the changes requested.
There was a problem hiding this comment.
You're right, lakchote. I removed the test in a1bef9f and updated the PR description.
How I checked: I changed getFormattedCancellationDate back to new Date(isoDateString) locally and ran the test. It still passed. In a real New York timezone, that change would turn 11:59 PM into 3:59 AM the next day, so setting process.env.TZ inside the test didn't change the timezone Date uses.
Why I removed the whole test: Without the TZ setup, it does the same thing as the existing test should show the wall-clock time without a timezone label when no timezone offset is present in the ISO string. Keeping it would leave a duplicate whose name claims New York coverage.
PR description: It no longer says a test covers a device in New York. It now notes that Jest can't change the timezone inside a test.
DateUtilsTest and TripReservationUtilsTest pass (188/188). ESLint passes on the test file.
…he timezone inside a test Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
No fix needed. All code checks on
|
Co-authored-by: Kevin Brian Bader <ikevin127@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
@lakchote Thanks for the review, I addressed the two comments 🙌 |
|
I replied @lakchote #105325 (comment) |
lakchote
left a comment
There was a problem hiding this comment.
Thanks @ikevin127, both are fixed. Looks good.
NAB: the hotelPnr fixture uses policy: 'Non-refundable', not CONST.CANCELLATION_POLICY.NON_REFUNDABLE, so the new guard skips it and that test still expects a deadline for a room that reads as non-refundable. Switching it to the constant would keep the fixture in line with the guard.







Explanation of Change
NewDot built its own hotel cancellation deadline: check-in time minus
durationBeforeArrivalDeadline. That duration is rounded to whole days, so the result often missed the deadline Spotnana shows. For example, a London booking with deadline Oct 19 11:59 PM and check-in Oct 21 3:00 PM (P1D) showed Oct 20 3:00 PM.This PR shows the deadline Spotnana returns instead:
getCancellationDeadlinenow returnsdeadlineUtc.iso8601when it ends in a UTC offset. If it doesn't, it falls back todeadline.iso8601, then todeadlineUtc.iso8601. The check-in minus duration code is removed.parseDurationToSecondsstays because flights use it.CONST.REGEX.ISO_8601_UTC_OFFSET_SUFFIXis used by bothgetCancellationDeadlineandDateUtils.getFormattedCancellationDate, so the offset check and the formatter can't drift apart.getFormattedCancellationDateno longer reads a time with no offset (and noZ) in the device's timezone. It shows that wall-clock time as-is, without the oldUTClabel, which was wrong for a hotel-local time.getFormattedCancellationDatetakes a new optionallocalTimeLabel. When the time has no offset and a label is passed, it's appended in place of a timezone label.HotelTripDetailspasses the newtravel.hotelDetails.hotelLocalTimestring, so a hotel deadline with no offset showsFree cancellation until Monday, Oct 19 11:59 PM, hotel's local time. A deadline with an offset still showsGMT±X/UTC(for example,Monday, Oct 19 11:59 PM, GMT+1).CarTripDetailspasses no label, so car deadlines with no offset show the time with no label.Unit tests cover the London booking (expects
Monday, Oct 19 11:59 PM, GMT+1), thedeadlinefallback, showing a no-offset time as-is with no timezone label, and the hotel local time label (appended when there's no offset, ignored when there is one). The tests run withTZ=utc, and Jest can't change the timezone inside a test, so no test covers a device in another timezone.AI tests run locally: Jest (
DateUtilsTest,TripReservationUtilsTest: 188/188 pass),npm run typecheck(pass), ESLint on changed files (pass).lint-changedandspell-changedcouldn't run in this environment because the checkout is shallow, and theoxfmtformat check couldn't run either. I didn't run a browser test because it needs a real Spotnana hotel booking.Fixed Issues
$ #104704
PROPOSAL: #104704 (comment)
Tests
Precondition: Travel is enabled in the workspace.
Monday, Oct 19 11:59 PM, GMT+1).UTClabel is shown.Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari