Release d hard freeze proxies#24
Merged
magmacomputing merged 15 commits intomainfrom May 2, 2026
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughBumps monorepo/packages to 2.8.0, centralizes class immutability into a shared wrapper (defaulting to Object.freeze), and reworks Tempo parsing/config into nested planner/intl surfaces with new parse-layout utilities, engine updates, tests, docs, and related wiring changes. ChangesRelease / Monorepo
Library Immutability
Tempo Parse & Planner Rework
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as Client
participant Tempo
participant Planner as ParsePlanner
participant Composer as Engine/Composer
participant ParseEngine as Parse Engine
User->>Tempo: init/extend({ planner, intl, ... })
Tempo->>Planner: set planner (layoutOrder, preFilter)
Tempo->>Tempo: extendState/ $setConfig -> rebuild patterns/layout
User->>Tempo: parse(input) / new Tempo(value, opts)
Tempo->>Composer: compose(value, onResult, unit)
Composer->>ParseEngine: produce match(es) -> call onResult
ParseEngine-->>Tempo: aggregated results
Tempo-->>User: parsed results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/library/src/common/class.library.ts (1)
31-36: 💤 Low valueRedundant checks on
value.constructor.The
valueparameter is the class constructor itself (the class being decorated). Checkingvalue.constructoraccessesFunction.prototype, which won't have$ImmutableSkip. The last two fallback checks (lines 34-35) are effectively dead code.🧹 Suggested cleanup
addInitializer(() => { const skip = (value as any)[$ImmutableSkip] ?? (value as any).$ImmutableSkip - ?? (value.constructor as any)?.[$ImmutableSkip] - ?? (value.constructor as any)?.$ImmutableSkip ?? []; hardenClassStaticsAndPrototypes(value, wrapper, skip); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/class.library.ts` around lines 31 - 36, The addInitializer callback is checking value.constructor for $ImmutableSkip which is redundant because the decorated parameter is the class constructor itself; remove the two fallback checks that reference (value.constructor as any)?.[$ImmutableSkip] and (value.constructor as any)?.$ImmutableSkip and instead only read (value as any)[$ImmutableSkip], (value as any).$ImmutableSkip, then default to []; update the callback around addInitializer and the local skip variable to drop any references to value.constructor while preserving the default [] behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/library/src/common/class.library.ts`:
- Around line 31-36: The addInitializer callback is checking value.constructor
for $ImmutableSkip which is redundant because the decorated parameter is the
class constructor itself; remove the two fallback checks that reference
(value.constructor as any)?.[$ImmutableSkip] and (value.constructor as
any)?.$ImmutableSkip and instead only read (value as any)[$ImmutableSkip],
(value as any).$ImmutableSkip, then default to []; update the callback around
addInitializer and the local skip variable to drop any references to
value.constructor while preserving the default [] behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab9ca32d-ad08-4588-a22a-a631c36f6953
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
CHANGELOG.mdpackage.jsonpackages/library/CHANGELOG.mdpackages/library/package.jsonpackages/library/src/common/class.library.tspackages/library/src/tsconfig.jsonpackages/library/test/common/decorator.immutable-secure.test.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/package.jsonpackages/tempo/plan/doc_cleanup.mdpackages/tempo/src/tempo.class.tspackages/tempo/test/support/ci.prefilter.setup.tspackages/tempo/test/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/library/src/tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tempo/src/discrete/discrete.parse.ts (1)
271-274:⚠️ Potential issue | 🟠 MajorRename
parsePrefiltertopreFilteron lines 272 and 274.The property was renamed project-wide from
parsePrefiltertopreFilter(confirmed in type definitions and throughout the codebase), but these two references were missed. They will fail at runtime sincestate.parse.parsePrefilterno longer exists.🐛 Proposed fix
const orderedPatterns = selectLayoutPatterns(state, trim, { - enablePrefilter: state.parse.parsePrefilter === true, + enablePrefilter: state.parse.preFilter === true, onPlan: (summary) => { - if (state.parse.parsePrefilter !== true || !state.config?.debug) return; + if (state.parse.preFilter !== true || !state.config?.debug) return; if (!TempoClass) return;🤖 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 271 - 274, The two references to the removed property state.parse.parsePrefilter inside the call to selectLayoutPatterns (and its onPlan callback) should be renamed to state.parse.preFilter; update both the enablePrefilter assignment and the conditional inside onPlan to use state.parse.preFilter so they match the project-wide rename and TypeScript types (locate occurrences in the selectLayoutPatterns call and the onPlan lambda).packages/tempo/src/tempo.class.ts (1)
353-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit layout metadata from layout patterns before collecting.
packages/tempo/src/tempo.type.ts:47-57now allowslayout: { order?: string[]; preFilter?: boolean }, but this branch still treats every object-valuedlayoutas pattern entries. Passing that new shape will registerorder/preFilteras layout tokens and never updateshape.parse.layoutOrderorshape.parse.preFilter, so the advertised API compiles but behaves incorrectly.🤖 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 353 - 369, The layout branch currently treats any object value as pattern entries, so layout metadata keys (order, preFilter) get registered as tokens; update the handling in the switch case for optKey === 'layout' inside tempo.class.ts: when arg.value is an object (and not a RegExp), split its keys—if a key is 'order' or 'preFilter' set shape.parse.layoutOrder and shape.parse.preFilter respectively (creating those fields if needed) and do not call collect for them; for all other keys/values call collect(rule, value, ...) as before. Keep existing behavior for snippet/layout pattern values and RegExps and use the existing create(shape.parse, optKey) and collect(...) helpers to locate where to change.
🧹 Nitpick comments (2)
packages/tempo/test/support/lineage.test.ts (1)
3-21: 💤 Low valueConsider adding
beforeEachto reset Tempo state.Other test files in this directory (e.g.,
layout.order.test.ts,parse.prefilter.flag.test.ts) callTempo.init()in abeforeEachhook to ensure a clean state between tests. Without this, test results may be affected by state from previously run test files.💡 Suggested addition
describe('Tempo Result Lineage', () => { + beforeEach(() => { + Tempo.init(); + }); + test('new Tempo() should have Undefined in result', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/support/lineage.test.ts` around lines 3 - 21, Tests in packages/tempo/test/support/lineage.test.ts rely on global Tempo state and currently don't reset it between tests; add a beforeEach that calls Tempo.init() to ensure a clean state before each test. Locate the test suite around describe('Tempo Result Lineage', ...) and add a beforeEach hook that invokes Tempo.init() (or the appropriate static initializer on the Tempo class) so each test (the constructors new Tempo(), new Tempo(new Date()), new Tempo('20-May')) runs with a fresh Tempo environment. Ensure the import/availability of Tempo is unchanged and that the hook runs before the existing test cases.packages/tempo/test/core/timestamp.test.ts (1)
3-21: 💤 Low valueAdd
beforeEachfor state isolation.For consistency with other test files and to ensure clean state between tests.
💡 Suggested addition
describe('Tempo Millisecond Timestamp', () => { + beforeEach(() => { + Tempo.init(); + }); + test('new Tempo(ms) should resolve correctly', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/core/timestamp.test.ts` around lines 3 - 21, Tests in timestamp.test.ts lack state isolation between cases; add a beforeEach hook to reset any shared state before each test (e.g., clear or reinitialize whatever global or module-level state Tempo may rely on) so tests don't affect one another. Insert a beforeEach(...) at the top of the describe('Tempo Millisecond Timestamp', ...) block that resets relevant state used by the Tempo constructor (or re-requires/initializes the module if needed) to ensure new Tempo(ms) and new Tempo(s, { timeStamp: 'ss' }) run in a clean environment.
🤖 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/plan/parse.md`:
- Line 8: Replace the misspelled word "particularily" with "particularly" in the
sentence "This is particularily more relevant in the Parse-related options, as
we give more control (via Options) to the User." so it reads "...particularly
more relevant in the Parse-related options..." ensuring consistent
capitalization of "User" if needed.
- Line 25: The heading line contains leading whitespace before the "# Parse"
markdown heading which triggers MD023; remove the leading spaces so the line
starts directly with "#" (i.e., change the heading token for the "Parse" section
in the parse.md file to have no preceding whitespace) to conform to markdown
conventions.
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 86-100: The code only special-cases 'ms' and 'ss' but must also
handle 'us' and 'ns'; update the branch that checks unit/value to add cases for
unit === 'us' and unit === 'ns' (alongside 'ms' and 'ss'), call
Temporal.Instant.fromEpochNanoseconds(value * 1_000) for microseconds and
Temporal.Instant.fromEpochNanoseconds(value) for nanoseconds, and emit onResult
with match 'Microseconds' or 'Nanoseconds' respectively (use the existing
variables unit, value, onResult, temporal, and Temporal.Instant.* to locate
where to insert the new branches).
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1572-1573: The instance getter Tempo.config currently returns the
raw variable out, exposing live nested references from this.#local.config and
its prototype chain; change the return to the hardened snapshot by returning
proxify(out) (i.e., restore the previous proxify(out) as t.Internal.Config) so
callers get an immutable proxied snapshot rather than live references — update
the getter in tempo.class.ts to return proxify(out) instead of out and ensure
the proxify helper is imported/available where used.
- Around line 420-421: When handling the 'preFilter' write path in
tempo.class.ts (the case that sets shape.parse.preFilter), also set the legacy
key so readers still checking state.parse.parsePrefilter keep working: assign
the same Boolean(arg.value) to both shape.parse.preFilter and
shape.parse.parsePrefilter, and add a symmetric handler for incoming
'parsePrefilter' arguments that writes both keys too; this ensures code paths
like discrete.parse (which reads state.parse.parsePrefilter) and tests
initializing parsePrefilter continue to see the value until all readers are
migrated.
In `@packages/tempo/test/core/timestamp.test.ts`:
- Around line 13-20: The test constructs a Tempo from seconds without specifying
timezone, causing CI-dependent failures; update the test to pass an explicit UTC
timezone in the Tempo options (i.e., when calling new Tempo(s, { timeStamp: 'ss'
}) add the UTC/timezone option used by Tempo) so the resulting yy, mm, dd are
computed in UTC consistently across environments; update the test case around
the Tempo constructor invocation and ensure the option key matches Tempo's API
(e.g., timezone/tz) used elsewhere in the codebase.
- Around line 4-11: The test "new Tempo(ms) should resolve correctly" fails in
non-UTC environments because the timestamp 1714521600000 is midnight UTC; update
the test to construct the Tempo instance with an explicit UTC timezone so date
fields are computed in UTC (e.g., pass a timezone option when creating the
object—new Tempo(ms, { timeZone: 'UTC' })—or the equivalent Tempo API your code
provides) and keep the same expectations for t.yy, t.mm, t.dd.
In `@packages/tempo/test/engine/layout.order.test.ts`:
- Around line 114-115: The test is passing the option under the wrong key so
extendState ignores it; update the test to call Tempo.init with either the
legacy top-level key or the new layout nesting: replace Tempo.init({ parse: {
layoutOrder: ['dt','wkd'] } }) with Tempo.init({ layoutOrder: ['dt','wkd'] }) or
Tempo.init({ layout: { order: ['dt','wkd'] } }) so extendState (which only
processes top-level keys into state.config or state.parse) receives the correct
option and state.parse.layoutOrder / state.config.layout.order is set as
intended.
---
Outside diff comments:
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 271-274: The two references to the removed property
state.parse.parsePrefilter inside the call to selectLayoutPatterns (and its
onPlan callback) should be renamed to state.parse.preFilter; update both the
enablePrefilter assignment and the conditional inside onPlan to use
state.parse.preFilter so they match the project-wide rename and TypeScript types
(locate occurrences in the selectLayoutPatterns call and the onPlan lambda).
In `@packages/tempo/src/tempo.class.ts`:
- Around line 353-369: The layout branch currently treats any object value as
pattern entries, so layout metadata keys (order, preFilter) get registered as
tokens; update the handling in the switch case for optKey === 'layout' inside
tempo.class.ts: when arg.value is an object (and not a RegExp), split its
keys—if a key is 'order' or 'preFilter' set shape.parse.layoutOrder and
shape.parse.preFilter respectively (creating those fields if needed) and do not
call collect for them; for all other keys/values call collect(rule, value, ...)
as before. Keep existing behavior for snippet/layout pattern values and RegExps
and use the existing create(shape.parse, optKey) and collect(...) helpers to
locate where to change.
---
Nitpick comments:
In `@packages/tempo/test/core/timestamp.test.ts`:
- Around line 3-21: Tests in timestamp.test.ts lack state isolation between
cases; add a beforeEach hook to reset any shared state before each test (e.g.,
clear or reinitialize whatever global or module-level state Tempo may rely on)
so tests don't affect one another. Insert a beforeEach(...) at the top of the
describe('Tempo Millisecond Timestamp', ...) block that resets relevant state
used by the Tempo constructor (or re-requires/initializes the module if needed)
to ensure new Tempo(ms) and new Tempo(s, { timeStamp: 'ss' }) run in a clean
environment.
In `@packages/tempo/test/support/lineage.test.ts`:
- Around line 3-21: Tests in packages/tempo/test/support/lineage.test.ts rely on
global Tempo state and currently don't reset it between tests; add a beforeEach
that calls Tempo.init() to ensure a clean state before each test. Locate the
test suite around describe('Tempo Result Lineage', ...) and add a beforeEach
hook that invokes Tempo.init() (or the appropriate static initializer on the
Tempo class) so each test (the constructors new Tempo(), new Tempo(new Date()),
new Tempo('20-May')) runs with a fresh Tempo environment. Ensure the
import/availability of Tempo is unchanged and that the hook runs before the
existing test cases.
🪄 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: f8997a26-2fec-4c2d-a018-a5845c967148
📒 Files selected for processing (26)
packages/library/src/common/class.library.tspackages/tempo/archive/tempo.api.mdpackages/tempo/archive/tempo.types.mdpackages/tempo/package.jsonpackages/tempo/plan/parse.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.layout.resolver.ts.bakpackages/tempo/src/engine/engine.layout.ts.bakpackages/tempo/src/parse/parse.layout.tspackages/tempo/src/parse/parse.resolver.tspackages/tempo/src/plugin/term/term.season.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/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/core/timestamp.test.tspackages/tempo/test/engine/layout.order.test.tspackages/tempo/test/engine/parse.layout.test.tspackages/tempo/test/engine/parse.prefilter.flag.test.tspackages/tempo/test/instance/instance.since.rtf.test.tspackages/tempo/test/support/lineage.test.tspackages/tempo/vitest.config.ts
✅ Files skipped from review due to trivial changes (5)
- packages/tempo/src/tsconfig.json
- packages/tempo/src/support/support.index.ts
- packages/tempo/src/plugin/term/term.season.ts
- packages/tempo/test/engine/parse.layout.test.ts
- packages/tempo/test/instance/instance.since.rtf.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/package.json
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)
354-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
$setDiscovery()still dropsdiscovery.planner.
Tempo.init()routes user/global discovery objects through this method, but there is no branch here that copiesplanner.layoutOrder/planner.preFilterintoshape.config.plannerandshape.parse.planner. So planner settings work via the directextend({ planner: ... })path, but the same settings are silently ignored when they come fromTempo.init({ discovery })or the symbol-backed global discovery object.Suggested fix
// 1d. Process Internationalization if (discovery.intl || discovery.relativeTime) { const intl = discovery.intl ?? {}; if (discovery.relativeTime) intl.relativeTime = { ...intl.relativeTime, ...(discovery.relativeTime as any) }; shape.config.intl = { ...shape.config.intl, ...intl }; } + + // 1e. Process Parse Planner + if (discovery.planner) { + const planner = discovery.planner; + shape.config.planner = { ...shape.config.planner, ...planner }; + if (isDefined(planner.layoutOrder)) + shape.parse.planner.layoutOrder = normalizeLayoutOrder(planner.layoutOrder); + if (isDefined(planner.preFilter)) + shape.parse.planner.preFilter = Boolean(planner.preFilter); + }🤖 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 354 - 425, The $setDiscovery method ignores discovery.planner so planner settings passed via Tempo.init or the global discovery object are dropped; update static [$setDiscovery](shape: Internal.State, discovery?: Internal.Discovery) to detect discovery.planner and merge its properties (e.g., layoutOrder, preFilter) into shape.config.planner and also into shape.parse.planner (preserving existing values) — similar to how intl and formats are merged — ensuring discovery.planner is normalized (as object) and applied before returning res so planner settings from discovery are honored.
🤖 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/support/tempo.enum.ts`:
- Around line 250-251: The DISCOVERY enum was missing the 'relativeTime' key
causing Tempo.extend and DISCOVERY checks to ignore discovery.relativeTime; add
'relativeTime' back into the discoveryKeys array (so DISCOVERY includes it) to
restore recognition by DISCOVERY.keys()/DISCOVERY.has() and allow $setDiscovery
/ Tempo.extend to dispatch and persist relativeTime again.
In `@packages/tempo/src/support/tempo.init.ts`:
- Line 111: Remove the leftover debug console.log by deleting the line that
prints "DEBUG: extendState planner:" (the console.log referencing
options.planner), so no debug output remains; locate the usage of
options.planner within the extendState/init function (look for the options
variable and the console.log call) and remove that console.log statement only.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1555-1556: The current check returns false for an empty plain
object (it uses "return keys.length > 0;"), which causes new Tempo({}) to be
parsed as a date instead of treated as options; update the logic so that any
plain object (including {}) is considered options — e.g., change the branch that
inspects plain objects to return true for plain objects regardless of keys
length (keep the existing plain-object detection code path and replace
"keys.length > 0" with a condition that treats plain objects as options),
ensuring the Tempo constructor/new Tempo(options) path receives {} as config.
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 354-425: The $setDiscovery method ignores discovery.planner so
planner settings passed via Tempo.init or the global discovery object are
dropped; update static [$setDiscovery](shape: Internal.State, discovery?:
Internal.Discovery) to detect discovery.planner and merge its properties (e.g.,
layoutOrder, preFilter) into shape.config.planner and also into
shape.parse.planner (preserving existing values) — similar to how intl and
formats are merged — ensuring discovery.planner is normalized (as object) and
applied before returning res so planner settings from discovery are honored.
🪄 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: 6c2dc3bb-78cd-4f77-b208-2996f304d441
📒 Files selected for processing (23)
.github/workflows/ci.ymlpackages/library/src/common/type.library.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/tempo.planner.mdpackages/tempo/plan/option.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.intl.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/core/discovery-extend.test.tspackages/tempo/test/core/timestamp.test.tspackages/tempo/test/engine/layout.order.test.tspackages/tempo/test/engine/parse.prefilter.flag.test.tspackages/tempo/test/plugins/term_unified.test.tspackages/tempo/test/support/lineage.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/plan/option.md
✅ Files skipped from review due to trivial changes (8)
- packages/library/src/common/type.library.ts
- .github/workflows/ci.yml
- packages/tempo/.vitepress/config.ts
- packages/tempo/test/core/timestamp.test.ts
- packages/tempo/CHANGELOG.md
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/test/engine/parse.prefilter.flag.test.ts
- packages/tempo/test/support/lineage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/test/engine/layout.order.test.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/engine/engine.lexer.ts`:
- Around line 55-57: The weekday/month normalization currently checks
low.startsWith(key.toLowerCase()) which reverses the prefix match and breaks
abbreviations; update the match to use (key as
string).toLowerCase().startsWith(low) (i.e. swap the operands) in the loop over
enums.WEEKDAY and enums.MONTH so abbreviated inputs like "wed"/"feb" correctly
find and return the full key in the function that returns found as T.
- Around line 203-209: The current code silently substitutes non-finite
year/month/day with dateTime values (via y/m/d), which masks parse errors;
instead validate year, month, and day before constructing the Temporal object
and fail explicitly when any is not a finite number: check
Number.isFinite(year), Number.isFinite(month), Number.isFinite(day) and if any
check fails throw a descriptive Error (or return a parse-failure) citing the
offending token(s) rather than falling back, then proceed to build the
Temporal.PlainDate.from({ year, month, day }) and call .toZonedDateTime(tz)
using the extracted tz from dateTime/timeZoneId.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 192-195: The relativeTime branch currently always object-spreads
arg.value which destroys function formatters; update the case handling in
tempo.init.ts so that when arg.value is a function you assign it directly to
state.config.intl.relativeTime (preserving callables), and when it is an object
you merge it as before (using the existing hasOwn/state.config.intl logic).
Target the switch case handling for 'relativeTime' and the symbol
state.config.intl.relativeTime to implement the typeof check and conditional
assignment.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1556-1565: The current object-classification logic checks
enums.CONFIG/enums.PARSE before date-like keys, causing
Temporal.ZonedDateTimeLike inputs with config keys (e.g., timeZone, calendar) to
be misclassified as options; update the branch order in the classification
routine (the block using enums.CONFIG, enums.PARSE, enums.ZONED_DATE_TIME,
enums.DURATIONS) so the check for enums.ZONED_DATE_TIME or enums.DURATIONS runs
before the enums.CONFIG/enums.PARSE check and returns false for date-like
objects, ensuring date/time-like inputs (e.g., year/month/day with timeZone) are
treated as values rather than options.
- Around line 393-396: The merge of discovery.relativeTime into
intl.relativeTime currently spreads discovery.relativeTime and will overwrite a
function-valued formatter; change the logic in Tempo (around the block that
touches discovery.relativeTime and shape.config.intl) to detect if
discovery.relativeTime is a function and, if so, assign it directly
(intl.relativeTime = discovery.relativeTime) instead of spreading; otherwise,
for object form, perform the existing shallow merge (intl.relativeTime = {
...intl.relativeTime, ...discovery.relativeTime }) and then set
shape.config.intl = { ...shape.config.intl, ...intl } so function-valued custom
formatters are preserved.
- Around line 997-1013: The parse getter returns a secure snapshot but currently
exposes parse.planner by reference; update Tempo.parse (inside static get parse)
to deep-clone the planner object before returning (similar to how
snippet/layout/event/etc. are spread) so callers cannot mutate global
state—specifically, replace the live reference to parse.planner with a copied
object (e.g., spread its properties or clone layoutOrder and preFilter) when
building the returned object in the parse getter (refer to the parse getter, the
local variable parse = (this as any)[$Internal]().parse, and the planner
property).
🪄 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: 103e350e-c638-43f6-a5c1-1e2514fc2fb2
📒 Files selected for processing (9)
packages/tempo/src/engine/engine.lexer.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/core/constructor.core.test.tspackages/tempo/test/core/discovery-extend.test.tspackages/tempo/test/core/sandbox-factory.test.tspackages/tempo/test/core/tempo_guard.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
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/engine/engine.lexer.ts (1)
49-60:⚠️ Potential issue | 🟠 MajorWiden
prefix()'s contract or split number/calendar normalization.The exported signature specifies
prefix<T extends t.WEEKDAY | t.MONTH>, but the implementation returns keys fromenums.NUMBER,enums.WEEKDAY,enums.WEEKDAYS,enums.MONTH, andenums.MONTHS. This mismatch is already evident at line 130 (enums.WEEKDAY[weekday as t.WEEKDAY] ?? enums.WEEKDAYS[weekday as t.WEEKDAYS]), where dual casts work around the incomplete type. The type contract should either be widened to reflect all possible return types, or the function should be split to separate number matching from calendar name normalization.🤖 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 49 - 60, The prefix function's exported signature is too narrow: prefix<T extends t.WEEKDAY | t.MONTH>(...) returns keys from enums.NUMBER, enums.WEEKDAY, enums.WEEKDAYS, enums.MONTH and enums.MONTHS, causing unsafe casts elsewhere (see usage around enums.WEEKDAY[...] ?? enums.WEEKDAYS[...]). Fix by either widening the return generic/union to include number and both singular/plural calendar key enums (e.g., include t.NUMBER | t.WEEKDAY | t.WEEKDAYS | t.MONTH | t.MONTHS) or split the implementation into two functions (e.g., normalizeNumberPrefix and normalizeCalendarPrefix) so prefix only handles one domain; update all call sites to use the appropriate function or new return type and remove the dual casts.
🧹 Nitpick comments (1)
packages/tempo/src/tempo.class.ts (1)
193-199: ⚡ Quick winUse
Tempo.#dbg.warninstead ofconsole.warnfor alias-collision diagnostics.At Line 198, direct console logging bypasses the class’s configured logging path and makes warning behavior inconsistent across environments.
Suggested fix
- if (Tempo.#isAliasCollision(keys[i], keys[j])) - console.warn(`Potential period alias collision: "${keys[i]}" overlaps with existing alias(es): ${keys[j]}`); + if (Tempo.#isAliasCollision(keys[i], keys[j])) + Tempo.#dbg.warn(shape.config, `Potential period alias collision: "${keys[i]}" overlaps with existing alias(es): ${keys[j]}`);🤖 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 193 - 199, Replace the direct console.warn call used for period alias diagnostics with the class logger by calling Tempo.#dbg.warn so warnings go through the configured logging path; specifically, in the loop that builds keys from periods and checks Tempo.#isAliasCollision(keys[i], keys[j]), swap console.warn(...) for Tempo.#dbg.warn(...) using the same message string (ensure Tempo.#dbg is the class-private logger used elsewhere).
🤖 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/engine/engine.lexer.ts`:
- Around line 128-133: The code computes offset from enums.WEEKDAY /
enums.WEEKDAYS using the token weekday and then uses offset in arithmetic
(variables weekday, offset, days); add an explicit guard after computing offset
to detect when both lookups fail (offset is undefined) and throw or return a
clear parsing error instead of proceeding to compute days and calling date
arithmetic; update the logic around the weekday/offset calculation in
engine.lexer (referencing weekday, enums.WEEKDAY, enums.WEEKDAYS, parseModifier,
dateTime.dayOfWeek, dateTime.daysInWeek) to validate offset and fail early with
a descriptive error for invalid weekday tokens.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1016-1018: The snapshot uses a shallow copy for monthDay
(monthDay: { ...parse.monthDay }) so nested objects/arrays remain live; replace
that shallow spread inside the static parse snapshot construction with a
deep-clone of parse.monthDay (for example use structuredClone(parse.monthDay) or
the project's deepClone utility) so all nested members are copied, then proceed
to freeze if required; update the code path that builds the snapshot (the object
containing monthDay and planner in Tempo.parse/tempo.class.ts) to use the
deep-cloned value instead of { ...parse.monthDay }.
---
Outside diff comments:
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 49-60: The prefix function's exported signature is too narrow:
prefix<T extends t.WEEKDAY | t.MONTH>(...) returns keys from enums.NUMBER,
enums.WEEKDAY, enums.WEEKDAYS, enums.MONTH and enums.MONTHS, causing unsafe
casts elsewhere (see usage around enums.WEEKDAY[...] ?? enums.WEEKDAYS[...]).
Fix by either widening the return generic/union to include number and both
singular/plural calendar key enums (e.g., include t.NUMBER | t.WEEKDAY |
t.WEEKDAYS | t.MONTH | t.MONTHS) or split the implementation into two functions
(e.g., normalizeNumberPrefix and normalizeCalendarPrefix) so prefix only handles
one domain; update all call sites to use the appropriate function or new return
type and remove the dual casts.
---
Nitpick comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 193-199: Replace the direct console.warn call used for period
alias diagnostics with the class logger by calling Tempo.#dbg.warn so warnings
go through the configured logging path; specifically, in the loop that builds
keys from periods and checks Tempo.#isAliasCollision(keys[i], keys[j]), swap
console.warn(...) for Tempo.#dbg.warn(...) using the same message string (ensure
Tempo.#dbg is the class-private logger used elsewhere).
🪄 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: 28bcfccc-ca75-4b61-adc6-ca8b63681195
📒 Files selected for processing (4)
.release-it.jsonpackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.class.ts
💤 Files with no reviewable changes (1)
- .release-it.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/src/support/tempo.init.ts
Owner
Author
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
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.
Evaluate Hard-Freeze Proxies
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores