fix(business-days): stop the day walk from hanging in five time zones, and show the month recipe - #561
Open
hyanmandian wants to merge 4 commits into
Open
fix(business-days): stop the day walk from hanging in five time zones, and show the month recipe#561hyanmandian wants to merge 4 commits into
hyanmandian wants to merge 4 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesBusiness-day utilities
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant getNextBusinessDay
participant addBusinessDays
participant eachLocalDay
participant isBusinessDay
Caller->>getNextBusinessDay: provide date and options
getNextBusinessDay->>addBusinessDays: request next business day
addBusinessDays->>eachLocalDay: iterate local calendar days
eachLocalDay-->>addBusinessDays: yield representable day
addBusinessDays->>isBusinessDay: evaluate holidays and weekdays
isBusinessDay-->>addBusinessDays: return business-day status
addBusinessDays-->>Caller: return Date or null
Merge Risk: ⚪ Minimal · up to The PR adds timezone-safe business-day APIs and updates existing calculations with documented exports and coverage. It is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 19 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Contributor
Tree-shaking report✅ No size regression. 6 grew out of 166 exports.
What changed (6)
All exports (166)
How this is measuredEvery export is imported alone into an esbuild consumer bundle (minified, tree-shaken) built from the head and from the base of this pull request; the sizes are the resulting bundles, gzip is their gzipped size. 🔴 marks a regression: a pre-existing export that grew more than 20% and more than 256 B, or the bundle importing every pre-existing export growing more than 5%. 🟡 is growth under the threshold, 🟢 a decrease, ⚪ no change, 🆕 an export that does not exist on the base (never a regression), 🗑️ an export that was removed. An intentional increase is accepted with the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## claude/gtin #561 +/- ##
=============================================
Coverage 100.00% 100.00%
=============================================
Files 197 198 +1
Lines 2137 2145 +8
Branches 630 632 +2
=============================================
+ Hits 2137 2145 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hyanmandian
changed the title
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
hyanmandian
added this pull request to stack #577
September 19, 2026 19:30
hyanmandian
removed this pull request from stack #577
September 22, 2026 05:37
hyanmandian
force-pushed
the
claude/business-day-helpers
branch
from
September 22, 2026 05:40
4a29787 to
84304c3
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
hyanmandian
added this pull request to stack #591
September 22, 2026 06:00
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Sep 22, 2026
hyanmandian
force-pushed
the
claude/business-day-helpers
branch
from
September 26, 2026 17:20
a2c1588 to
d16f9da
Compare
hyanmandian
force-pushed
the
claude/business-day-helpers
branch
from
September 26, 2026 18:32
d16f9da to
50d5b2b
Compare
hyanmandian
changed the title
…utating a Date addBusinessDays, subBusinessDays and differenceInBusinessDays advanced their walk with result.setDate(result.getDate() + step). That call is not guaranteed to change the local calendar day: when the neighbouring day does not exist in the zone, because it was skipped to cross the date line, the runtime re-normalizes onto the same day and the loop has a fixed point. Under TZ=Pacific/Apia, subBusinessDays(new Date(2012, 0, 5, 12), 4) and differenceInBusinessDays(new Date(2011, 11, 1), new Date(2011, 11, 31)) never return: they freeze the calling thread, and a browser tab with it, on input that is perfectly valid. Pacific/Fakaofo (30 December 2011), Pacific/Kiritimati and Pacific/Enderbury (31 December 1994) and Pacific/Kwajalein (21 August 1993) have the same five-day set of local days that no Date can carry, and the backward walks reach every one of them. The same normalization also dragged a shifted hour through the rest of the walk, which addBusinessDays masked with a final setHours: the hour came back but the minutes did not, so a walk crossing the half hour transition of Australia/Lord_Howe moved 02:15 to 02:45. The new src/_internals/each-local-day walks the days between two local calendar days, given as the Date.UTC numbers of those days, with an integer counter: it always advances, it stops after a fixed number of steps whatever the zone does, and it never mutates a Date. Each day is yielded at noon, the one time of day every existing local day has, so a caller reading the local year, month, day and weekday, which is all isBusinessDay reads, always sees the day it asked for. The five local days no zone ever had are skipped rather than yielded twice as the day the runtime resolves them to. All three utils now drive their walk with it, so there is one implementation to reason about instead of three. Behaviour is unchanged for every input that did not hang or fall on a daylight saving boundary, including the sign convention, the boundary treatment and the 1900-2099 refusal, all still pinned by the existing tests. The new time zone suites run on Node, Bun and Deno through the inTimeZone helper of the test runtime and are skipped where the process time zone cannot be changed.
…t at getHolidays memoizes a year and returns copies of the Date objects it built the first time. A Date is an instant, and the holidays of a year are local calendar days, so once the process time zone changes the memoized answer moves with it: under TZ=America/Sao_Paulo, a 2018 entry first computed under UTC turns 2 November 2018 00:00 into 1 November 21:00, and Finados stops being a holiday for the day it falls on. isBusinessDay and the walks built on it then count that day as a business day, which is how addBusinessDays(new Date(2018, 10, 1, 9, 30), 5) answered 8 November instead of 9. The memo now keeps the year, month and day of each holiday and builds the Date on the way out, in the zone the caller is in. It costs nothing: the copy on the way out already allocated one Date per holiday. The new time zone suite pins it, and the time zone suites of the business day utils stop depending on which test file warmed the memo first, which is why they passed on Node, where each test file gets its own module registry, and failed on Bun, where they share one.
…ith add/subBusinessDays
The month questions ("quinto dia útil", "último dia útil do mês") need no
utility of their own: adding n business days to the last day of the month
before gives the n-th business day of the month, and subtracting one from the
first day of the month after gives the last one. Both docs now show that
recipe under subBusinessDays, with the two ways it differs from a dedicated
function spelled out: an n beyond the business days of the month lands in the
next month, and January 1900 and December 2099 return null because the
starting day is outside the supported years. They also say this is the banking
count, not the payroll one of CLT art. 459 § 1º.
The recipe is pinned by tests: hand counted examples (Ano novo, Carnaval,
Sexta-feira Santa, Corpus Christi with and without includeOptional, a state
holiday, the month spill, the year edges and December 2011 in Pacific/Apia,
the case that used to hang) and two fast-check properties that compare it with
a brute force walk of the month through isBusinessDay, generated by the new
businessDayMonths arbitrary.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLkm9YrtAifc6XCLFVEsdH
…and sub's time caveat The day walk comment said five local days do not exist but listed four, leaving out Pacific/Fakaofo. The recipe bullet said January 1900 and December 2099 return null, when only the n-th business day of the first and the last business day of the second do. subBusinessDays said the time of day is always kept, without the caveat addBusinessDays, which it delegates to, documents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLkm9YrtAifc6XCLFVEsdH
hyanmandian
force-pushed
the
claude/business-day-helpers
branch
from
September 26, 2026 18:59
50d5b2b to
464c33d
Compare
This branch was successfully deployed
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
2 participants
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.
What
It fixes a hang in released code:
addBusinessDays,subBusinessDaysanddifferenceInBusinessDayscould loop forever in five time zones. It also shows, in the docs and in tests, how to get the n-th and the last business day of a month with the utils that already exist.This PR first proposed
getNthBusinessDay,getNextBusinessDayandgetLastBusinessDayOfMonth. They were dropped (and removed from the history, so nofeatcommit reaches the changelog) because the existing utils already answer those questions:daddBusinessDays(d, 1)(that is allgetNextBusinessDaydid)addBusinessDays(new Date(y, m, 0), n), from the last day of the month beforesubBusinessDays(new Date(y, m + 1, 1), 1), from the first day of the month afterThe one real difference, an
nbeyond the business days of the month landing in the next month instead ofnull, is documented next to the recipe (comparegetMonth()), together with the year edges (January 1900 and December 2099 returnnull, since the starting day is outside the supported years).The fix to the released utils (
fix(business-days))The walk advanced with
result.setDate(result.getDate() + step), which is not guaranteed to change the local calendar day. When the neighbouring local day does not exist, because the zone skipped it to cross the date line, the runtime re-normalizes onto the same day and the loop has a fixed point: it never terminates and freezes the calling thread (a browser tab with it) on valid input.Pacific/ApiaPacific/FakaofoPacific/KiritimatiPacific/EnderburyPacific/KwajaleinThe same normalization also dragged a shifted hour through the walk: a walk crossing the half hour transition of
Australia/Lord_Howemoved02:15to02:45.The new internal
src/_internals/each-local-daywalks the days between two local calendar days with an integer counter bounded by the distance, so it always advances and always terminates; every day is yielded at noon (the one time every existing local day has); the five missing days are skipped rather than visited twice.addBusinessDays(andsubBusinessDays) anddifferenceInBusinessDaysshare it.fix(get-holidays)memoizes the local day rather than the instant it was built at.Behaviour is unchanged for every input that did not hang or fall on a daylight saving boundary; the whole existing suite passes untouched.
The month recipe (
docs(business-days))Both
docs/utilities.mdfiles show the recipe undersubBusinessDays. It is pinned by tests inadd-business-days.test.tsandsub-business-days.test.ts: hand counted examples (Ano novo, Carnaval with and withoutincludeOptional, Sexta-feira Santa, Corpus Christi, a state holiday, the month spill, the year edges, December 2011 inPacific/Apia, the case that used to hang) and fast-check properties comparing the recipe with a brute force walk of the month throughisBusinessDay, generated by a newbusinessDayMonthsarbitrary.The payroll "quinto dia útil" of CLT art. 459 § 1º is counted with Saturdays by labour inspection; #573, on top of this PR, adds
includeSaturdayfor that and extends the recipe to it.The docs name the recipe's year edges precisely (the n-th business day of January 1900 and the last of December 2099), subBusinessDays documents the same time-of-day caveat as addBusinessDays, and the day walk comment lists Pacific/Fakaofo with the other four zones.
Verification
npm run check: passnpx vp test run --coverage: 6567 passed, 100% statements, branches, functions and linesnpm run check:unused,npm run check:duplication(0 clones),npm run check:api(no public change): passnpx commitlint --from origin/main --to HEAD: 0 problems🤖 Generated with Claude Code
https://claude.ai/code/session_01RLkm9YrtAifc6XCLFVEsdH