Release c layout order planner#23
Merged
magmacomputing merged 36 commits intomainfrom Apr 30, 2026
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (94)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVersion 2.7.0 release restructures Tempo configuration by consolidating date-region and relative-time options into nested Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 10 minutes and 5 seconds.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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/support/tempo.register.ts (1)
31-50:⚠️ Potential issue | 🟠 Major
registryReset()can delete defaults it can no longer restore.The new non-extensible guard only applies during restore. Lines 31-37 still purge configurable properties first, so a non-extensible registry can lose default keys and then only log a warning on Lines 46-50 instead of being reset. Handle non-extensible objects before the purge, or overwrite existing properties in place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.register.ts` around lines 31 - 50, registryReset currently deletes configurable properties on [state, target] before checking extensibility, which can remove defaults that cannot be redefined; update the purge/restore logic (look for the loop iterating [state, target] and the subsequent Reflect.ownKeys(defaults) restore) to first check Object.isExtensible(obj) before deleting properties — if obj is non-extensible, skip the deletion step and instead attempt to update existing properties in place by using Object.getOwnPropertyDescriptor(defaults, key) and Object.defineProperty(obj, key, desc) only when the property already exists (otherwise log a clear warning); alternatively move the extensibility guard into the initial purge loop so non-extensible objects are not mutated and will be handled during restore.
🧹 Nitpick comments (2)
packages/tempo/test/setup.prefilter.ts (1)
1-9: Consolidate prefilter setup ownership to one setup module.This file and
packages/tempo/test/ci.prefilter.setup.tsboth implement parsePrefilter CI wiring. Keeping one source of truth will prevent setup drift and ambiguous behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/setup.prefilter.ts` around lines 1 - 9, This file duplicates CI prefilter wiring already implemented in ci.prefilter.setup.ts; consolidate ownership by removing this setup or delegating to the single existing module: delete or disable the Tempo.init({ parsePrefilter: true }) block guarded by TEMPO_PREFILTER_CI in this file and ensure tests import or run the central ci.prefilter.setup module instead so only one place configures Tempo.init and the TEMPO_PREFILTER_CI behavior.packages/tempo/test/sandbox-factory.test.ts (1)
27-29: Remove stale commented mock-clear lines.These commented lines are no longer needed and make the test noisier without adding value.
🧹 Suggested cleanup
- // // Clear previous calls to ensure assertions are local to this test - // (console.warn as any).mockClear(); - // (console.error as any).mockClear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/sandbox-factory.test.ts` around lines 27 - 29, Remove the stale commented mock-clear lines—specifically the commented calls "(console.warn as any).mockClear();" and "(console.error as any).mockClear();" in the test file; delete these commented lines so the test is cleaner and contains only active setup/teardown code (no other changes needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 57-58: The "Run end-to-end benchmark" step mixes stdout and stderr
into bench-output.json which breaks subsequent JSON parsing; change the command
invocation (npx tsx --conditions=development bench/bench.parse.prefilter.e2e.ts
> bench-output.json 2> bench-error.log) so stdout goes only to bench-output.json
and stderr is written to a separate file (bench-error.log), and update any
artifact upload steps to include both files so errors are preserved separately.
In `@packages/tempo/doc/migration-guide.md`:
- Around line 112-114: The doc claim that "old top-level keys still work as
fallbacks" is too broad and incorrect; update the migration note to state that
only the deprecated aliases handled in the Tempo class constructor (specifically
rtfFormat and rtfStyle) are currently accepted as fallbacks and that
mdyLocales/mdyLayouts are not treated as aliases, so users must migrate those
keys to the new nested structure; reference the Tempo constructor's
deprecated-alias handling for rtfFormat/rtfStyle and adjust the wording to
accurately list which legacy keys are supported and which must be changed.
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 169-172: Replace the deprecated rtfFormat option passed to
Tempo(...).since: locate the example where rtf is created and used (const rtf =
new Intl.RelativeTimeFormat(...); and the call Tempo(entry.ts).since(null, {
rtfFormat: rtf })), and change the options to use the grouped API by passing the
RelativeTimeFormat's format function instead (e.g. Tempo(entry.ts).since(null, {
relativeTime: rtf.format.bind(rtf) }) or relativeTime: (value, unit) =>
rtf.format(value, unit))). Ensure the option key is relativeTime and remove
rtfFormat.
In `@packages/tempo/plan/release-c-prefilter-summary.md`:
- Around line 57-62: Update the summary narrative to match the actual benchmark
values shown in the bullets: replace the stale integrated end-to-end delta
`-1.31%` with the observed `-0.91%` and remove the phrase that claims the
end-to-end benchmark "needs to be added"; ensure both occurrences of this
paragraph (the earlier summary block and the duplicate later block) are updated
so the text consistently reflects the `-0.91%` integrated end-to-end result and
that the benchmark is already recorded.
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 149-155: The code incorrectly falls back to dateTime instead of
config.anchor and can leave year undefined; update the num(...) call in
engine.lexer.ts to use config.anchor (when present and a
Temporal.ZonedDateTime-like object) as the default source for year/month/day
instead of dateTime, and normalize empty-string inputs for yy, mm, dd (treat ''
as missing) so num receives defined values; ensure the fallback order is
(provided yy/mm/dd) -> config.anchor.year/month/day -> dateTime.year/month/day
so year is always defined before any later call like year.toString().
In `@packages/tempo/src/support/tempo.enum.ts`:
- Around line 249-250: The discovery enum was changed to 'plugins' which breaks
existing discovery payloads using 'plugin'; update the discoveryKeys passed to
enumify (the discoveryKeys constant used to create DISCOVERY via enumify) to
include 'plugin' (or include both 'plugin' and 'plugins') so DISCOVERY.has(...)
in tempo.class.ts continues to recognize legacy discovery: { plugin: ... }
payloads; make the minimal change in the discoveryKeys array to restore backward
compatibility.
- Around line 235-236: Add the missing "parsePrefilter" enum member to the
static OPTION and PARSE enums so they match the dynamic Default-derived keys;
specifically, include 'parsePrefilter' in the optionKeys const used to build
OPTION (the const optionKeys and export const OPTION = enumify(...)) and add the
corresponding entry/name to the PARSE enum definition so enums.OPTION.has(key)
and parse-related checks in tempo.class.ts (around lines referencing
enums.OPTION and PARSE) will recognize parsePrefilter. Ensure spelling matches
the existing uses in tempo.default.ts, tempo.class.ts, and tempo.init.ts.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 151-153: extendState() currently only handles the 'parsePrefilter'
arg and ignores 'monthDay', so add a branch for the 'monthDay' override that
routes it through the same normalization/shadowing path used by the new grouped
monthDay option; specifically, in the switch where 'parsePrefilter' is handled,
add a case 'monthDay' that assigns the normalized value to state.parse.monthDay
using the same helper or logic used when New Tempo(...) sets monthDay (e.g.,
call the existing normalizeMonthDay/parse-monthDay helper or replicate that
normalization) so parse(value, { monthDay: ... }) and new Tempo(..., { monthDay:
... }) follow identical behavior.
In `@packages/tempo/src/support/tempo.register.ts`:
- Around line 87-89: The current merge logic for arrays uses reference equality
(current.includes(v)), which fails for tuple layouts like MONTH_DAY.layouts that
are arrays-of-arrays; change the dedupe to use a deep equality check when
comparing elements so structurally identical arrays are treated as duplicates
(e.g., replace includes with current.some(existing => deepEqual(existing, v))
and add a small deep-equals helper that compares arrays elementwise or use an
existing isEqual utility); ensure this updated check is used where current and
val arrays are merged so resolveLayoutOrder() receives only unique layouts.
In `@packages/tempo/src/support/tempo.util.ts`:
- Around line 119-125: The dotted-name fallback for placeholders (inside the
isNullish branch) skips user overrides in state.parse.layout; update the
fallback chain in the block that computes res (the code using getSymbol(prefix),
snippet, state.parse.snippet and Layout) to also check state.parse.layout for
both pToken and prefix before falling back to the built-in Layout. In other
words, when computing res in that branch, add checks for
state.parse.layout[pToken as keyof Layout] and state.parse.layout[prefix as
keyof Layout] in the appropriate order so overridden layouts are used prior to
Layout.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 492-537: The method Tempo.#resolveMonthDay is mutating the public
MonthDay.locales (replacing strings with {locale,timeZones} objects) which
breaks the public contract exposed via Tempo.parse.monthDay; change it to
preserve the original locales shape and store the computed/resolved locale
objects in a separate internal cache or field (e.g. Tempo.#resolvedMonthDay or
add an internal property like resolvedLocales on the returned object), so keep
the returned value's locales as string[] (or same public type) while moving the
resolvedLocales/timeZones array into a new internal-only property and update any
consumers (e.g. `#isMonthDay` and parsing code) to read the resolved data from
that new internal property instead of relying on mutated MonthDay.locales.
- Around line 855-879: The code allows the original discovery object to leak
into the later options merge, overwriting the symbol stored in
state.config.discovery; fix by normalizing discovery to a symbol and ensuring
any options passed to (SandboxTempo as any)[$setConfig] (and the object merged
into globalThis) use that symbol instead of the original object—e.g., create a
normalizedDiscovery symbol after you detect an object, use that symbol when
assigning (globalThis as any)[normalizedDiscovery] = data and when setting
state.config.discovery, and pass discovery: normalizedDiscovery (not the raw
object) into $setConfig and any options merges so the symbol-backed slot is
preserved (references: discovery variable, data merge, state.config.discovery,
(SandboxTempo as any)[$setConfig]).
In `@packages/tempo/test/ci.prefilter.setup.ts`:
- Around line 3-4: The one-time call Tempo.init({ parsePrefilter: true }) is
being overridden by later per-test Tempo.init() calls; wrap or replace
Tempo.init so it always forces parsePrefilter=true by capturing the original
Tempo.init (e.g., const _init = Tempo.init) and exposing a new
Tempo.init(...opts) that merges opts with { parsePrefilter: true } before
delegating to the original; ensure the wrapper is applied at module-load time so
all subsequent test-suite calls to Tempo.init preserve parsePrefilter, and
reference the Tempo.init symbol in your change and any test helpers that import
Tempo.
In `@packages/tempo/test/duration.lazy.test.ts`:
- Around line 6-11: The test must enforce deterministic preconditions: before
calling t.until(...) explicitly disable "catch" mode and ensure the
DurationModule is not loaded so the call throws; update the test to call the API
that turns off catch mode (e.g., Tempo.setCatchMode(false) or
t.setCatchMode(false)) and/or explicitly unload or verify the DurationModule is
absent (e.g., t.unloadModule('DurationModule') or assert
!t.hasModule('DurationModule')) before invoking until, then assert expect(() =>
t.until('2024-01-02')).toThrow(/Tempo: DurationModule not loaded/) and
expect(console.error).toHaveBeenCalled().
In `@packages/tempo/test/parse.prefilter.numeric-safety.test.ts`:
- Around line 17-35: The tests assume layout-planner matching for pure numeric
inputs, but parse() currently short-circuits integer-like strings into a
numeric-prefilter (BigInt) before selectLayoutPatterns() runs; update the two
failing tests either by using non-pure-numeric inputs so selectLayoutPatterns()
is exercised (e.g., add a delimiter like '59-05-31' or '11:05:59'), or change
the expectations to reflect the numeric-prefilter path (i.e., do not expect
first?.match to be 'yearMonthDayShort' or 'hourMinuteSecond' and assert the
parser's numeric-prefilter outcome instead); reference the Tempo class and
selectLayoutPatterns()/discrete.parse numeric-prefilter behavior when making the
change.
In `@packages/tempo/test/setup.console-spy.ts`:
- Around line 4-9: This file uses vitest globals (vi, beforeEach, afterAll) but
doesn't import them; add explicit imports to match other setup files by
importing { vi, beforeEach, afterAll } from 'vitest' at the top so IDEs and
readers can resolve symbols used alongside the declared global _consoleSpies and
ReturnType<typeof vi.spyOn>; keep existing eslint comment and global declaration
unchanged.
In `@packages/tempo/test/static.methods.test.ts`:
- Around line 62-64: The test's assertion is too weak—update the assertion in
the test that calls Tempo.init({ monthDay: { locales: ['en-PH'] } }) to verify
the override explicitly by checking Tempo.parse.monthDay.locales contains the
provided locale; for example assert that Tempo.parse.monthDay.locales includes
'en-PH' (or use an arrayContaining-style check) instead of only checking length
> 0 so the test fails if the locale override isn't applied.
---
Outside diff comments:
In `@packages/tempo/src/support/tempo.register.ts`:
- Around line 31-50: registryReset currently deletes configurable properties on
[state, target] before checking extensibility, which can remove defaults that
cannot be redefined; update the purge/restore logic (look for the loop iterating
[state, target] and the subsequent Reflect.ownKeys(defaults) restore) to first
check Object.isExtensible(obj) before deleting properties — if obj is
non-extensible, skip the deletion step and instead attempt to update existing
properties in place by using Object.getOwnPropertyDescriptor(defaults, key) and
Object.defineProperty(obj, key, desc) only when the property already exists
(otherwise log a clear warning); alternatively move the extensibility guard into
the initial purge loop so non-extensible objects are not mutated and will be
handled during restore.
---
Nitpick comments:
In `@packages/tempo/test/sandbox-factory.test.ts`:
- Around line 27-29: Remove the stale commented mock-clear lines—specifically
the commented calls "(console.warn as any).mockClear();" and "(console.error as
any).mockClear();" in the test file; delete these commented lines so the test is
cleaner and contains only active setup/teardown code (no other changes needed).
In `@packages/tempo/test/setup.prefilter.ts`:
- Around line 1-9: This file duplicates CI prefilter wiring already implemented
in ci.prefilter.setup.ts; consolidate ownership by removing this setup or
delegating to the single existing module: delete or disable the Tempo.init({
parsePrefilter: true }) block guarded by TEMPO_PREFILTER_CI in this file and
ensure tests import or run the central ci.prefilter.setup module instead so only
one place configures Tempo.init and the TEMPO_PREFILTER_CI behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f739870-a4f8-4c3b-9dff-9d9ec23a72e6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (57)
.github/workflows/ci.ymlCHANGELOG.mdpackage.jsonpackages/library/package.jsonpackages/library/src/common/proxy.library.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/archive/tempo.api.mdpackages/tempo/doc/archive/tempo.types.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.month-day.mdpackages/tempo/package.jsonpackages/tempo/plan/.WISHLIST.mdpackages/tempo/plan/release-c-prefilter-summary.mdpackages/tempo/rollup.config.jspackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.planner.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.entry.tspackages/tempo/src/tempo.type.tspackages/tempo/test/ci.prefilter.setup.tspackages/tempo/test/constructor.core.test.tspackages/tempo/test/duration.lazy.test.tspackages/tempo/test/engine.layout.test.tspackages/tempo/test/engine.planner.test.tspackages/tempo/test/error-handling.test.tspackages/tempo/test/infinite-loop.test.tspackages/tempo/test/instance.set.test.tspackages/tempo/test/instance.since.rtf.test.tspackages/tempo/test/issue-fixes.test.tspackages/tempo/test/month-day.test.tspackages/tempo/test/parse.prefilter.flag.test.tspackages/tempo/test/parse.prefilter.numeric-safety.test.tspackages/tempo/test/proof.test.tspackages/tempo/test/sandbox-factory.test.tspackages/tempo/test/setup.console-spy.tspackages/tempo/test/setup.prefilter.tspackages/tempo/test/static.getters.test.tspackages/tempo/test/static.methods.test.tspackages/tempo/test/term-dispatch.core.test.tspackages/tempo/test/term-shorthand.test.tspackages/tempo/test/term_unified.test.tspackages/tempo/test/ticker.term.core.test.tspackages/tempo/typedoc.jsonpackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (7)
- packages/tempo/test/term_unified.test.ts
- packages/tempo/test/instance.set.test.ts
- packages/tempo/test/issue-fixes.test.ts
- packages/tempo/test/ticker.term.core.test.ts
- packages/tempo/test/term-dispatch.core.test.ts
- packages/tempo/plan/.WISHLIST.md
- packages/tempo/test/constructor.core.test.ts
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/tempo/src/engine/engine.lexer.ts (1)
154-158:⚠️ Potential issue | 🟠 MajorNormalize
config.anchorbefore the Temporal check.This only uses the anchor when it is already Temporal-like, but
packages/tempo/src/module/module.duration.tsLine 94 passesanchor: this, andpackages/tempo/src/tempo.class.tsstores that value unchanged. Partial inputs on anchored paths will still fall back todateTimeinstead of the caller's anchor.Suggested fix
- const anchor = (config && isTemporal(config.anchor)) ? config.anchor : undefined; + const rawAnchor = config?.anchor; + const anchor = isTemporal(rawAnchor) + ? rawAnchor + : (typeof rawAnchor?.toDateTime === 'function' ? rawAnchor.toDateTime() : undefined); const fallbackYear = isDefined(anchor?.year) ? anchor.year : dateTime.year; const fallbackMonth = isDefined(anchor?.month) ? anchor.month : dateTime.month; const fallbackDay = isDefined(anchor?.day) ? anchor.day : dateTime.day;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/engine/engine.lexer.ts` around lines 154 - 158, The fallback logic uses config.anchor only when isTemporal(config.anchor) is true, so callers passing non-temporal anchors (e.g., anchor: this) are ignored; normalize config.anchor to its Temporal-like form before the isTemporal check and then compute anchor, fallbackYear, fallbackMonth, and fallbackDay from that normalized value. Update the anchor initialization in engine.lexer.ts (the anchor variable and subsequent fallbackYear/fallbackMonth/fallbackDay) to first run the same normalization/conversion used elsewhere (the module.duration/tempo codepath that accepts "anchor: this") so partial anchored inputs correctly fall back to the caller's anchor.packages/tempo/src/tempo.class.ts (1)
823-848:⚠️ Potential issue | 🔴 CriticalSandbox discovery needs a registered key.
create()can now hand$setConfigeitherSymbol('TempoSandbox')or a raw string. That does not round-trip:Symbol.keyFor()returnsundefinedfor local symbols, andTempo.discoverylater resolves strings viaSymbol.for(...). In both cases the sandbox can lose its discovery payload. This is the same symbol-backed discovery problem as before, just through a different normalization path.Suggested fix
- const normalizedDiscovery = (isObject(options.discovery) && !isSymbol(options.discovery)) || isUndefined(options.discovery) - ? Symbol('TempoSandbox') - : options.discovery as string | symbol; + const normalizedDiscovery = isSymbol(options.discovery) + ? options.discovery + : Symbol.for( + isString(options.discovery) + ? options.discovery + : `tempo.discovery.sandbox.${++Tempo.#usrCount}` + );#!/bin/bash set -euo pipefail rg -n -C2 'normalizedDiscovery|Symbol\\.keyFor\\(optVal\\)|static get discovery|#getConfig' packages/tempo/src/tempo.class.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` around lines 823 - 848, The sandbox discovery is being set to an unregistered local symbol (Symbol('TempoSandbox')) which breaks round-tripping via Symbol.keyFor and Tempo.discovery resolution; change the normalization in create() so normalizedDiscovery is a registered symbol/string: use Symbol.for('TempoSandbox') (or the original options.discovery if it's a string or a symbol already registered) and ensure the same registered symbol is used for (globalThis as any)[normalizedDiscovery], state.config.discovery and the call to (SandboxTempo as any)[$setConfig]; update any code paths that create the default discovery to use Symbol.for so Symbol.keyFor/Tempo.discovery resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/library/src/common/object.library.ts`:
- Around line 36-45: The current equality branch in isObject uses
isFunction(left.keys) and then reads values via left[k], which causes Map/Set
comparisons to be incorrect; update the logic in the object equality branch
(symbols: isObject, isFunction, ownKeys, isEqual, left.keys) to either restrict
the keys-path to plain objects (e.g., only when Object.getPrototypeOf(left) is
Object.prototype) or add explicit branches for Map and Set: for Map compare
sizes and iterate keys using left.get(k) vs right.get(k) (and handle missing
keys), and for Set compare sizes and ensure every element in left is present in
right (using right.has). Ensure you no longer access Map/Set values via left[k]
when left.keys exists.
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 227-230: The bypass check builds a dynamic RegExp from
user-provided alias keys (the local.some loop creating new RegExp(String(key),
'i') and testing trim), which allows metacharacters and ReDoS; change it to
perform a literal, case-insensitive match by escaping keys before using RegExp
or by comparing lowercased strings (e.g., normalize trim and String(key) to
lowercase and use includes/indexOf) so the match is treated as plain text rather
than a regex; update the bypass/local.some logic accordingly to ensure alias
keys are sanitized and matched literally.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 192-194: The switch branch setting state.parse.pivot = arg.value
must validate and coerce arg.value to a numeric year offset before storing it;
update the 'pivot' case to parse the incoming value (e.g., Number/parseInt) and
ensure it yields a finite integer in the expected range (or reject/throw or set
a sane default) so engine.lexer.ts can safely treat state.parse.pivot as a
numeric two-digit-year offset; locate the 'pivot' case and perform the numeric
coercion and validation there (state.parse.pivot) rather than assigning
arg.value verbatim.
In `@packages/tempo/src/support/tempo.register.ts`:
- Around line 31-39: The reset routine in registryReset() currently skips symbol
keys by filtering with isSymbol(key), which leaves symbol-keyed registries (like
layout/snippet) intact; to fix, stop excluding symbols—remove or change the
.filter(key => !isSymbol(key)) so Reflect.ownKeys(obj) yields both string and
symbol keys, then continue to get the descriptor and delete configurable
properties as before (references: function registryReset, variables state and
target, helper isSymbol).
- Around line 85-105: The merge function currently updates only target for
nested object merges and array appends, causing STATE (st) to fall out of sync;
update merge so that when encountering an existing object (isObject(current) &&
isObject(val)) you ensure st[key] exists as an object (create and set via
setProperty if needed) and call merge(current, val, st[key]) so nested changes
mirror into STATE, and when appending to arrays (Array.isArray(current) &&
Array.isArray(val)) also ensure st[key] exists as an array and push the same
newly-added items into st[key] (using isEqual checks consistent with current) so
array additions are reflected in STATE; keep existing behavior for the top-level
missing-key branch that already sets st[key].
In `@packages/tempo/src/tempo.class.ts`:
- Around line 850-853: The current code unconditionally replaces
state.parse.monthDay with resolveMonthDay(options.discovery.monthDay,
Tempo.MONTH_DAY), which can overwrite values (like monthDay.active) previously
computed by $setConfig; instead, compute the resolved discovery value via
resolveMonthDay(options.discovery.monthDay, Tempo.MONTH_DAY) and merge it into
the existing state.parse.monthDay (e.g., shallow/deep merge or
Object.assign/spread) so that previously-configured fields such as
monthDay.active are preserved while discovery-provided fields are applied.
- Around line 396-398: The 'monthDay' branch in tempo.class.ts currently calls
resolveMonthDay(arg.value, Tempo.MONTH_DAY) which ignores instance/sandbox
additions; change it to resolve against the current parser state by merging with
state.parse.monthDay (same approach used in tempo.init.ts) so
shape.parse.monthDay = resolveMonthDay(arg.value, <merged monthDay registry>),
e.g. combine state.parse.monthDay with arg.value defaults before passing to
resolveMonthDay to preserve instance-specific entries.
---
Duplicate comments:
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 154-158: The fallback logic uses config.anchor only when
isTemporal(config.anchor) is true, so callers passing non-temporal anchors
(e.g., anchor: this) are ignored; normalize config.anchor to its Temporal-like
form before the isTemporal check and then compute anchor, fallbackYear,
fallbackMonth, and fallbackDay from that normalized value. Update the anchor
initialization in engine.lexer.ts (the anchor variable and subsequent
fallbackYear/fallbackMonth/fallbackDay) to first run the same
normalization/conversion used elsewhere (the module.duration/tempo codepath that
accepts "anchor: this") so partial anchored inputs correctly fall back to the
caller's anchor.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 823-848: The sandbox discovery is being set to an unregistered
local symbol (Symbol('TempoSandbox')) which breaks round-tripping via
Symbol.keyFor and Tempo.discovery resolution; change the normalization in
create() so normalizedDiscovery is a registered symbol/string: use
Symbol.for('TempoSandbox') (or the original options.discovery if it's a string
or a symbol already registered) and ensure the same registered symbol is used
for (globalThis as any)[normalizedDiscovery], state.config.discovery and the
call to (SandboxTempo as any)[$setConfig]; update any code paths that create the
default discovery to use Symbol.for so Symbol.keyFor/Tempo.discovery resolve
correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77a8c71d-0b4c-4375-b536-397b7afbc145
📒 Files selected for processing (32)
.github/workflows/ci.ymlpackages/library/src/common/array.library.tspackages/library/src/common/class.library.tspackages/library/src/common/object.library.tspackages/library/src/common/symbol.library.tspackages/tempo/doc/migration-guide.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/package.jsonpackages/tempo/plan/release-c-prefilter-summary.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.layout.resolver.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/support/tempo.symbol.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/ci.prefilter.setup.tspackages/tempo/test/duration.core.test.tspackages/tempo/test/duration.lazy.test.tspackages/tempo/test/event.test.tspackages/tempo/test/number-words.core.test.tspackages/tempo/test/parse.prefilter.flag.test.tspackages/tempo/test/parse.prefilter.numeric-safety.test.tspackages/tempo/test/sandbox-factory.test.tspackages/tempo/test/static.methods.test.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/tempo/test/duration.core.test.ts
✅ Files skipped from review due to trivial changes (6)
- packages/library/src/common/symbol.library.ts
- packages/library/src/common/array.library.ts
- packages/tempo/test/number-words.core.test.ts
- packages/tempo/plan/release-c-prefilter-summary.md
- packages/tempo/doc/migration-guide.md
- packages/tempo/test/parse.prefilter.numeric-safety.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/tempo/test/ci.prefilter.setup.ts
- packages/tempo/src/support/support.index.ts
- packages/tempo/src/support/tempo.default.ts
- packages/tempo/test/parse.prefilter.flag.test.ts
- packages/tempo/vitest.config.ts
- packages/tempo/package.json
- packages/tempo/test/duration.lazy.test.ts
- .github/workflows/ci.yml
- packages/tempo/src/support/tempo.util.ts
- packages/tempo/src/tempo.type.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/tempo/src/plugin/term/term.quarter.ts (1)
25-25: Guarditm.yearas numeric before adding.Line 25 is null-safe now, but it can still coerce incorrectly if
itm.yearis non-numeric at runtime. Prefer a numeric guard foritm.yeartoo.Suggested diff
- if (isNumber(itm.fiscal)) itm.fiscal += (itm.year ?? 0); + if (isNumber(itm.fiscal)) itm.fiscal += (isNumber(itm.year) ? itm.year : 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/term/term.quarter.ts` at line 25, The current increment uses (itm.year ?? 0) which can coerce non-numeric values; update the logic so you only add if itm.year is numeric. Replace the line that reads if (isNumber(itm.fiscal)) itm.fiscal += (itm.year ?? 0); with a guarded add using the same helper (isNumber): check isNumber(itm.year) and add itm.year, otherwise add 0 — e.g. if (isNumber(itm.fiscal)) itm.fiscal += isNumber(itm.year) ? itm.year : 0; to avoid runtime coercion.packages/tempo/src/discrete/discrete.parse.ts (1)
305-310: Remove commented-out anchor injection code.This debug remnant is now obsolete since
parseDateinengine.lexer.tshandles anchor fallback internally. Consider removing to reduce noise.🧹 Suggested cleanup
dateTime = parseWeekday(groups, dateTime, (TempoClass as any)?.[sym.$dbg], state.config); - - // Inject anchor into config for parseDate - // const prevAnchor = state.config.anchor; - // state.config.anchor = state.anchor ?? dateTime; dateTime = parseDate(groups, dateTime, (TempoClass as any)?.[sym.$dbg], state.config, state.parse["pivot"]); - // state.config.anchor = prevAnchor; - dateTime = parseTime(groups, dateTime);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 305 - 310, Remove the obsolete commented-out anchor injection lines around the parseDate call: delete the three commented lines that set/restore state.config.anchor (the lines with "const prevAnchor = state.config.anchor;", "state.config.anchor = state.anchor ?? dateTime;", and "state.config.anchor = prevAnchor;") since parseDate (called as parseDate(groups, dateTime, (TempoClass as any)?.[sym.$dbg], state.config, state.parse["pivot"])) now handles anchor fallback internally; keep the active parseDate invocation and related references to TempoClass, sym.$dbg, state.config, and state.parse["pivot"] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/library/src/common/assertion.library.ts`:
- Around line 13-14: Restore the generic-preserving type-guard signatures for
the listed guards so they narrow the caller's type instead of returning concrete
primitives: change signatures like export const isString = (obj: any): obj is
string to generic forms (e.g., export const isString = <T>(obj: T): obj is T &
string) and do the same for isNumber, isArray, isObject, isDate, isNull,
isNullish, isUndefined, isMap, and isSet while keeping the existing isType(...)
implementation calls (refer to symbols isString, isNumber, isArray, isObject,
isDate, isNull, isNullish, isUndefined, isMap, isSet).
In `@packages/tempo/src/module/module.duration.ts`:
- Around line 121-122: The fallback resolution for the relative-time formatter
skips a function-valued config.relativeTime; update the rtf resolution logic
(the expression assigning rtf that references rtOptions, rtConfig,
opts['rtfFormat'], and (this as any).config['rtfFormat']) to also check for a
function on (this as any).config.relativeTime (or config.relativeTime) the same
way you check rtOptions as a function, so a function-valued config.relativeTime
is used before falling back to rtConfig.format, opts['rtfFormat'], or rtfFormat
in the instance config.
---
Nitpick comments:
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 305-310: Remove the obsolete commented-out anchor injection lines
around the parseDate call: delete the three commented lines that set/restore
state.config.anchor (the lines with "const prevAnchor = state.config.anchor;",
"state.config.anchor = state.anchor ?? dateTime;", and "state.config.anchor =
prevAnchor;") since parseDate (called as parseDate(groups, dateTime, (TempoClass
as any)?.[sym.$dbg], state.config, state.parse["pivot"])) now handles anchor
fallback internally; keep the active parseDate invocation and related references
to TempoClass, sym.$dbg, state.config, and state.parse["pivot"] unchanged.
In `@packages/tempo/src/plugin/term/term.quarter.ts`:
- Line 25: The current increment uses (itm.year ?? 0) which can coerce
non-numeric values; update the logic so you only add if itm.year is numeric.
Replace the line that reads if (isNumber(itm.fiscal)) itm.fiscal += (itm.year ??
0); with a guarded add using the same helper (isNumber): check
isNumber(itm.year) and add itm.year, otherwise add 0 — e.g. if
(isNumber(itm.fiscal)) itm.fiscal += isNumber(itm.year) ? itm.year : 0; to avoid
runtime coercion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62c69bc6-9eec-42b4-ab27-c3a1fd7b82d5
📒 Files selected for processing (9)
packages/library/src/common/assertion.library.tspackages/library/src/common/object.library.tspackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/tempo.class.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/src/tempo.class.ts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/tempo.class.ts (1)
779-799:⚠️ Potential issue | 🟠 Major
Tempo.extend(discoveryObject)doesn't apply the new discovery keys.This branch still updates only
numbers,timeZones, andformats.init()goes through$setDiscovery(), butTempo.extend({ monthDay: ... })andTempo.extend({ relativeTime: ... })are no-ops for the live state after this PR.Suggested direction
- // handle other discovery keys directly - if (discovery.numbers) registryUpdate('NUMBER', discovery.numbers) - if (discovery.timeZones) { - const tzs = Object.fromEntries(ownEntries(discovery.timeZones).map(([k, v]) => [k.toString().toLowerCase(), v])); - registryUpdate('TIMEZONE', tzs) - } - if (discovery.formats) { - (this as any)[$Internal]().config.formats = (this as any)[$Internal]().config.formats.extend(discovery.formats) as t.FormatRegistry; - registryUpdate('FORMAT', discovery.formats) - } + // keep direct discovery handling in sync with init() + const directOptions = (this as any)[$setDiscovery]((this as any)[$Internal](), discovery); + if (!isEmpty(directOptions)) + (this as any)[$setConfig]((this as any)[$Internal](), directOptions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` around lines 779 - 799, The current branch only handles numbers, timeZones and formats so new discovery keys (e.g. monthDay, relativeTime) are never applied to the live registries; update the logic to iterate the incoming discovery object for all keys in DISCOVERY and call registryUpdate for each present key (similar to how numbers/timeZones/formats are handled), merging or extending values into the internal config where applicable (use (this as any)[$Internal]().config and extend formats as already done), and keep the existing symbol-assignment block that calls (this as any)[$setConfig] to set discovery; reference registryUpdate, DISCOVERY, (this as any)[$Internal](), and (this as any)[$setConfig] to locate where to add the generic application of discovery keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 507-510: The code currently sets match.isAnchored to true when
state.parse.anchor is defined; instead, stop deriving the boolean from
state.parse.anchor and set match.anchor to state.parse.anchor only as metadata,
while deriving match.isAnchored from state.parse.isAnchored; update the block
that references state.parse.anchor and match.isAnchored (in discrete.parse.ts)
to assign match.anchor = state.parse.anchor if defined, but set match.isAnchored
= state.parse.isAnchored (or leave unchanged if already set) so anchoring
semantics come from state.parse.isAnchored rather than the presence of
state.parse.anchor.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 41-44: The current monthDay merge in tempo.init (the monthDay
property built from ...((isObject(baseState?.parse.monthDay)... and
...((isObject(Default.monthDay)...)) uses a shallow spread that allows
Default.monthDay to overwrite inherited extensions and leaves resolvedLocales
stale; change this to recompute monthDay from the merged locales/timezones
rather than shallow-spreading: first merge/resolve the locales/timezones (the
same merged source used elsewhere in init(..., baseState)), then derive/rebuild
monthDay programmatically from that merged locale data (or perform a deep-merge
of the parsed month-day entries) so sandbox/local extensions in
baseState?.parse.monthDay are preserved and resolvedLocales stays in sync with
the merged locales/timezones.
- Around line 199-200: The public parse(...) helper's anchor option is being
stored on state.config by extendState() but ParseEngine.parse() only reads
dateTime and state.anchor, so parse(..., { anchor }) is ignored; fix by ensuring
the anchor is propagated: either have extendState() set state.anchor when it
detects an anchor in state.config, or update ParseEngine.parse() to fall back to
reading state.config.anchor when state.anchor is undefined (check symbols:
extendState, state.config, state.anchor, ParseEngine.parse, and the public parse
helper) so the anchor option is honored.
In `@packages/tempo/src/support/tempo.register.ts`:
- Around line 33-41: The current guard skips deletion when an object is
non-extensible, but Object.preventExtensions does not prevent deleting
configurable own properties; in registryReset (the block iterating [state,
target]) remove the Object.isExtensible(obj) check so the Reflect.ownKeys(...)
-> .filter(... isSymbol/sym ...) -> getOwnPropertyDescriptor(obj, key) -> if
(desc?.configurable) delete obj[key] flow runs for all objects (preserving the
configurable check and symbol exclusion via isSymbol and sym) so non-extensible
registries have their dynamic keys removed and reset stays idempotent.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 450-453: The current case 'relativeTime' branch drops
function-valued configs; update it to handle both functions and objects: if
typeof optVal === 'function' assign shape.config.relativeTime = optVal (with
appropriate typing/cast), else if isObject merge as currently done
(shape.config.relativeTime = { ...shape.config.relativeTime, ...(optVal as any)
}); keep the break and existing symbols (case 'relativeTime', optVal, isObject,
shape.config.relativeTime) so function formatters passed to new Tempo(..., {
relativeTime: fn }) are preserved.
- Around line 825-836: The current normalization uses Symbol.for('TempoSandbox')
for object/undefined/local-symbol discovery, causing all such sandboxes to share
the same global key; change the branch that now returns
Symbol.for('TempoSandbox') to instead create a unique, non-registered symbol
(e.g., Symbol() or Symbol(`TempoSandbox#${uniqueId}`)) so each sandbox gets its
own symbol key. Update the normalizedDiscovery assignment logic (the same
expression that inspects discovery / options.discovery) to return a fresh Symbol
when discovery is an object, undefined, or a local symbol (i.e., when
Symbol.keyFor(options.discovery) is falsy), leaving the
string/Symbol.for(discovery) branch unchanged; keep using normalizedDiscovery
when writing to globalThis so each sandbox writes to its own unique slot.
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 779-799: The current branch only handles numbers, timeZones and
formats so new discovery keys (e.g. monthDay, relativeTime) are never applied to
the live registries; update the logic to iterate the incoming discovery object
for all keys in DISCOVERY and call registryUpdate for each present key (similar
to how numbers/timeZones/formats are handled), merging or extending values into
the internal config where applicable (use (this as any)[$Internal]().config and
extend formats as already done), and keep the existing symbol-assignment block
that calls (this as any)[$setConfig] to set discovery; reference registryUpdate,
DISCOVERY, (this as any)[$Internal](), and (this as any)[$setConfig] to locate
where to add the generic application of discovery keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5999293-dc31-4eb2-9aac-8ad248f1615d
📒 Files selected for processing (12)
packages/library/src/common/assertion.library.tspackages/library/src/common/coercion.library.tspackages/library/src/common/object.library.tspackages/tempo/doc/tempo.debugging.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/instance.result.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/library/src/common/object.library.ts
- packages/library/src/common/assertion.library.ts
- packages/tempo/src/tempo.type.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/tempo.class.ts (1)
437-439:⚠️ Potential issue | 🟠 MajorPreserve local (non-registered) discovery symbols instead of coercing to
Symbol.keyFor.At Line 438,
Symbol.keyFor()returnsundefinedfor local symbols, losing sandbox discovery identity. Related code at lines 859-863 and 827 intentionally detects local symbols and createsSymbol('TempoSandbox')for them, showing that local symbols should be preserved.Proposed fix
case 'discovery': - setProperty(shape.config, 'discovery', isSymbol(optVal) ? Symbol.keyFor(optVal) as string : optVal); + setProperty( + shape.config, + 'discovery', + isSymbol(optVal) ? (Symbol.keyFor(optVal) ?? optVal) : optVal + ); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` around lines 437 - 439, The current assignment in the discovery case coerces all Symbols to Symbol.keyFor(optVal), which returns undefined for local symbols and loses sandbox identity; update the logic in the discovery branch that calls setProperty(shape.config, 'discovery', ...) so that when optVal is a Symbol you use Symbol.keyFor(optVal) only if it returns a non-undefined global key, otherwise preserve and pass through the original local Symbol (i.e., keep optVal for local symbols); reference the symbols/functions isSymbol, Symbol.keyFor and the setProperty call in tempo.class.ts to locate and change this behavior.
♻️ Duplicate comments (1)
packages/tempo/test/support/setup.console-spy.ts (1)
4-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake this file a module before using
declare global.
declare globalis only valid from module scope, but this setup file still has no import/export. Addexport {}or the Vitest imports so the augmentation type-checks.♻️ Suggested fix
+import { afterAll, beforeEach, vi } from 'vitest'; + // Global console suppression for all tests // (You can comment out lines to allow specific console methods)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/support/setup.console-spy.ts` around lines 4 - 28, The file uses declare global (augmenting the global scope for var _consoleSpies and then referencing globalThis._consoleSpies) but is not a module; make the file a module by adding a top-level export (e.g. export {}) or importing from Vitest so the declare global augmentation type-checks correctly; keep the existing symbols (declare global, globalThis._consoleSpies, the vi.spyOn calls, afterAll and beforeEach) unchanged and only add the minimal export/import line at the top of the file.
🧹 Nitpick comments (1)
packages/tempo/test/core/discovery-extend.test.ts (1)
3-30: ⚡ Quick winIsolate global state in this suite to avoid order-dependent failures.
These tests mutate global registries/config; add a reset hook so they don’t leak into neighboring tests.
💡 Proposed change
describe('Discovery in Extend', () => { + beforeEach(() => { + Tempo.init({ timeZone: 'UTC', locale: 'en-US' }); + }); + it('should apply monthDay discovery via extend', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/core/discovery-extend.test.ts` around lines 3 - 30, The tests mutate global Tempo registries (Tempo.MONTH_DAY, Tempo.config, Tempo.formats) so add a test-level reset to isolate state: call the library's reset/restore method (e.g., Tempo.reset() or equivalent) in a beforeEach/afterEach hook surrounding the describe block to restore defaults between tests; ensure hooks call the actual symbol that resets global configuration and registries so Tempo.extend changes in one spec cannot leak into others.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 408-410: In the $setConfig handler for case 'pivot' you're
directly assigning Number(arg.value) into shape.parse["pivot"], which permits
NaN or out-of-range values; update the code to mirror the validation used by the
init extension path: convert arg.value to a numeric value, check
Number.isFinite(value) && !Number.isNaN(value) and validate it against the same
min/max bounds used in the init extension path (or clamp to those bounds), and
only assign shape.parse["pivot"] when the value passes validation (otherwise
throw or return an error/log and skip assignment). Ensure you reference and
reuse the same validation logic or helper used by the init extension path so
both places behave identically.
- Around line 220-224: The Intl.Locale construction is unguarded and can throw
for invalid locale strings; wrap the creation of Intl.Locale(locale) in a
try/catch and fall back to a safe default (e.g., use this.#locale() or 'en-US')
while logging or handling the error, so that the code using shape.config ->
locale (and derived const intl) never throws at runtime; update the block that
defines const intl to validate/normalize locale first and only call new
Intl.Locale on a guaranteed-valid value, using Tempo.MONTH_DAY /
shape.parse.monthDay as before.
In `@packages/tempo/test/support/ci.prefilter.setup.ts`:
- Around line 6-7: The delegated call in Tempo.init currently hard-codes Tempo
as the receiver which breaks subclass polymorphism; update the delegation to
call _init with the current receiver (use this) so subclasses created via
Tempo.create() initialize their own isolated state — e.g., replace the
_init.call(Tempo, ...) invocation with _init.call(this, { ...options,
parsePrefilter: true }) so Tempo.init preserves the original this context.
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 437-439: The current assignment in the discovery case coerces all
Symbols to Symbol.keyFor(optVal), which returns undefined for local symbols and
loses sandbox identity; update the logic in the discovery branch that calls
setProperty(shape.config, 'discovery', ...) so that when optVal is a Symbol you
use Symbol.keyFor(optVal) only if it returns a non-undefined global key,
otherwise preserve and pass through the original local Symbol (i.e., keep optVal
for local symbols); reference the symbols/functions isSymbol, Symbol.keyFor and
the setProperty call in tempo.class.ts to locate and change this behavior.
---
Duplicate comments:
In `@packages/tempo/test/support/setup.console-spy.ts`:
- Around line 4-28: The file uses declare global (augmenting the global scope
for var _consoleSpies and then referencing globalThis._consoleSpies) but is not
a module; make the file a module by adding a top-level export (e.g. export {})
or importing from Vitest so the declare global augmentation type-checks
correctly; keep the existing symbols (declare global, globalThis._consoleSpies,
the vi.spyOn calls, afterAll and beforeEach) unchanged and only add the minimal
export/import line at the top of the file.
---
Nitpick comments:
In `@packages/tempo/test/core/discovery-extend.test.ts`:
- Around line 3-30: The tests mutate global Tempo registries (Tempo.MONTH_DAY,
Tempo.config, Tempo.formats) so add a test-level reset to isolate state: call
the library's reset/restore method (e.g., Tempo.reset() or equivalent) in a
beforeEach/afterEach hook surrounding the describe block to restore defaults
between tests; ensure hooks call the actual symbol that resets global
configuration and registries so Tempo.extend changes in one spec cannot leak
into others.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdcbb6a1-3573-471e-ba84-d1a873a4ed41
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (93)
package.jsonpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/tempo.class.tspackages/tempo/test/README.mdpackages/tempo/test/core/accessors.test.tspackages/tempo/test/core/config.test.tspackages/tempo/test/core/constructor.core.test.tspackages/tempo/test/core/constructor.shorthand.test.tspackages/tempo/test/core/custom-options.test.tspackages/tempo/test/core/discovery-extend.test.tspackages/tempo/test/core/discovery.getters.test.tspackages/tempo/test/core/discovery_security.test.tspackages/tempo/test/core/dispose.core.test.tspackages/tempo/test/core/global-options.test.tspackages/tempo/test/core/runtime_brand.test.tspackages/tempo/test/core/sandbox-factory.test.tspackages/tempo/test/core/static.getters.test.tspackages/tempo/test/core/static.methods.test.tspackages/tempo/test/core/static.options.test.tspackages/tempo/test/core/static.test.tspackages/tempo/test/core/storage.test.tspackages/tempo/test/core/symbol-discovery.test.tspackages/tempo/test/core/tempo_from.test.tspackages/tempo/test/core/tempo_guard.test.tspackages/tempo/test/discrete/compact.time.test.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/discrete/standalone.test.tspackages/tempo/test/discrete/standalone_parse.test.tspackages/tempo/test/engine/engine.layout.test.tspackages/tempo/test/engine/engine.planner.test.tspackages/tempo/test/engine/layout.order.test.tspackages/tempo/test/engine/meridiem.test.tspackages/tempo/test/engine/month-day.test.tspackages/tempo/test/engine/parse.prefilter.flag.test.tspackages/tempo/test/engine/parse.prefilter.numeric-safety.test.tspackages/tempo/test/engine/pattern.default.test.tspackages/tempo/test/engine/pattern.weekday.test.tspackages/tempo/test/engine/tempo_regexp.test.tspackages/tempo/test/engine/timezone_offset.test.tspackages/tempo/test/instance/instance.add.test.tspackages/tempo/test/instance/instance.convert.test.tspackages/tempo/test/instance/instance.format.test.tspackages/tempo/test/instance/instance.result.test.tspackages/tempo/test/instance/instance.set.test.tspackages/tempo/test/instance/instance.since.rtf.test.tspackages/tempo/test/instance/instance.since.test.tspackages/tempo/test/instance/instance.until.test.tspackages/tempo/test/instance/lazy.test.tspackages/tempo/test/instance/relative_date.test.tspackages/tempo/test/issues/infinite-loop.test.tspackages/tempo/test/issues/issue-fixes.test.tspackages/tempo/test/issues/repro_bracket.test.tspackages/tempo/test/issues/repro_hang.test.tspackages/tempo/test/plugins/debug_term.test.tspackages/tempo/test/plugins/duration.core.test.tspackages/tempo/test/plugins/duration.lazy.test.tspackages/tempo/test/plugins/duration.static.test.tspackages/tempo/test/plugins/duration.test.tspackages/tempo/test/plugins/event.test.tspackages/tempo/test/plugins/fiscal-cycle.core.test.tspackages/tempo/test/plugins/number-words.core.test.tspackages/tempo/test/plugins/plugin.test.tspackages/tempo/test/plugins/plugin_registration.test.tspackages/tempo/test/plugins/reactive_registration.test.tspackages/tempo/test/plugins/slick.shorthand.test.tspackages/tempo/test/plugins/slick.verification.test.tspackages/tempo/test/plugins/term-dispatch.core.test.tspackages/tempo/test/plugins/term-shorthand.test.tspackages/tempo/test/plugins/term.test.tspackages/tempo/test/plugins/term_unified.test.tspackages/tempo/test/plugins/ticker.active.test.tspackages/tempo/test/plugins/ticker.hang.test.tspackages/tempo/test/plugins/ticker.options.test.tspackages/tempo/test/plugins/ticker.patterns.test.tspackages/tempo/test/plugins/ticker.pulse.test.tspackages/tempo/test/plugins/ticker.stop.test.tspackages/tempo/test/plugins/ticker.term.core.test.tspackages/tempo/test/plugins/ticker_cold_start.test.tspackages/tempo/test/support/ci.prefilter.setup.tspackages/tempo/test/support/error-handling.test.tspackages/tempo/test/support/guard.test.tspackages/tempo/test/support/library-import.test.tspackages/tempo/test/support/primitive.test.tspackages/tempo/test/support/proof.test.tspackages/tempo/test/support/season_metadata.test.tspackages/tempo/test/support/setup.console-spy.tspackages/tempo/test/support/soft_freeze.test.tspackages/tempo/test/support/symbol-import.test.tspackages/tempo/test/support/tempo-import.test.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (7)
- packages/tempo/test/plugins/term-dispatch.core.test.ts
- packages/tempo/test/instance/instance.set.test.ts
- packages/tempo/test/issues/issue-fixes.test.ts
- packages/tempo/test/plugins/term_unified.test.ts
- packages/tempo/test/plugins/duration.core.test.ts
- packages/tempo/test/plugins/ticker.term.core.test.ts
- packages/tempo/test/core/constructor.core.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/test/core/static.getters.test.ts
- packages/tempo/test/plugins/number-words.core.test.ts
- packages/tempo/test/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/tempo/vitest.config.ts
- packages/tempo/src/support/tempo.register.ts
Owner
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
This was referenced May 1, 2026
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.
Release -C of the re-working of layout Order processing
Summary by CodeRabbit
Release Notes - Version 2.7.0
New Features
monthDayandrelativeTimeoptions replacemdyLocales/mdyLayoutsandrtfFormat/rtfStyleparsePrefilterflag for optimized parsing performancesetCatchMode(),unloadModule(),hasModule()parse.anchoraccessor tracking reference pointsBug Fixes
Intl.LocalediagnosticsDocumentation