tidy/up for review#29
Open
magmacomputing wants to merge 6 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis release updates Tempo to 2.9.3: assertion predicates move to unknown-based guards, a branded ISOString and AliasContext API are added, numeric epoch resolution is unit-aware with 9-digit fractional normalization, alias resolution order and parse return shapes are changed, manifests and docs updated, and tests added/adjusted. ChangesTempo 2.9.3: Type Hardening, Fractional Resolution & Alias Context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 9
🧹 Nitpick comments (5)
packages/tempo/test/issues/issue-fixes.test.ts (1)
2-2: ⚡ Quick winUse
import typeforAliasContextto match sibling test files and stay safe underverbatimModuleSyntax.The other tests in this PR (
relative_date.test.ts,sandbox-factory.test.ts) consistently import it asimport type. This file is the only outlier.♻️ Diff
-import { AliasContext } from '#tempo/tempo.type.js'; +import type { AliasContext } from '#tempo/tempo.type.js';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/test/issues/issue-fixes.test.ts` at line 2, Replace the value import of AliasContext with a type-only import to match the other tests: change the import of AliasContext from '#tempo/tempo.type.js' to a type import (i.e., use "import type { AliasContext } ...") so the test uses a type-only reference and remains compatible with verbatimModuleSyntax; update the import statement where AliasContext is referenced in the test file to the type-only form.packages/tempo/src/engine/engine.normalizer.ts (1)
112-134: 💤 Low value
filterparam via prefix match is fine for the current key shape; consider an exact match against the alias-engine's group name to be future-proof.Today the matcher relies on regex group keys being shaped like
evt\d+_\d+/per\d+_\d+/ literalslk, sokey.startsWith(f)coincides with intent. If anyone later adds a different group key whose name happens to start withevt/per, it would unintentionally pass the filter.You could resolve the alias's actual
type(viaaliasEngine.getAlias(key)?.type) before applying the filter, but skip if not worth the indirection in the hot path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.normalizer.ts` around lines 112 - 134, The current prefix-based filter in resolveAliases risks false positives; change the check inside resolveAliases to perform an exact match against the alias-engine's group/type: for each key, resolve const alias = aliasEngine?.getAlias(key) (or similar API) and if filter is provided only continue when filter.some(f => f === key || (alias && (alias.type === f || alias.groupName === f))); if aliasEngine is missing fall back to the existing key === f || key.startsWith(f) behavior to preserve hot-path performance. Ensure you reference resolveAliases, aliasEngine and getAlias in the change.packages/tempo/src/tempo.class.ts (1)
364-374: ⚡ Quick winShallow
{ ...intl.relativeTime, ...discovery.relativeTime }silently drops a function-typedrelativeTime.If
discovery.intl.relativeTimeis a function (now permitted by the expandedIntlOptions) anddiscovery.relativeTimeis an object, line 370 spreads the function — yielding only its enumerable own properties (typically{}) — so the function is dropped without warning. Likely an unintended interaction between the two sibling shorthand inputs.♻️ Proposed handling: don't merge across function/object boundaries
if (discovery.intl || discovery.relativeTime) { const intl: t.IntlOptions = { ...discovery.intl }; if (discovery.relativeTime) { if (typeof discovery.relativeTime === 'function') { intl.relativeTime = discovery.relativeTime; - } else { - intl.relativeTime = { ...intl.relativeTime, ...discovery.relativeTime }; + } else { + // If the inherited shorthand is a function, the explicit object replaces it + // rather than being merged into a coerced empty object. + intl.relativeTime = typeof intl.relativeTime === 'function' + ? { ...discovery.relativeTime } + : { ...intl.relativeTime, ...discovery.relativeTime }; } } shape.config.intl = { ...shape.config.intl, ...intl }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 364 - 374, The code currently merges intl.relativeTime by shallow-spreading which can drop a function value; change the merging logic in the block that handles discovery.intl / discovery.relativeTime so you never attempt to spread a function. Concretely: if discovery.relativeTime is a function, set intl.relativeTime = discovery.relativeTime; else if intl.relativeTime (from discovery.intl) is a function and discovery.relativeTime is an object, keep intl.relativeTime as the function (do not merge); otherwise (both are objects or undefined) merge with { ...intl.relativeTime, ...discovery.relativeTime } and then assign shape.config.intl = { ...shape.config.intl, ...intl }. Ensure you reference the existing symbols discovery.intl, discovery.relativeTime, intl.relativeTime, and shape.config.intl when making the change.packages/tempo/doc/tempo.parse.md (1)
114-138: 💤 Low valueResolution Context section now matches the actual
AliasContextsurface.Replacing
this.with(...)withthis.set({ hour: 22, minute: 0, second: 0 })accurately reflects that the alias host exposesset/add/toNow/toDateTime(not Temporal's.with). The accessor list (hh/mi/ss) is correct.Minor optional polish (skip if not worth it): the host also exposes
yy/mm/dd/tz/cal/config— a one-line addition would make the doc exhaustive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/doc/tempo.parse.md` around lines 114 - 138, Update the "Functional Alias Context" docs to match the real AliasContext API: replace any mention of this.with(...) with this.set(...) and explicitly list the available methods this.add(duration), this.set(input), this.toNow(), this.toDateTime() and accessors this.hh / this.mi / this.ss; also ensure the example alias functions call this.set(...) and this.add(...) (referencing AliasContext, the example alias strings 'bedtime' and 'meeting', and the methods this.set and this.add). Optionally append the remaining context accessors yy/mm/dd/tz/cal/config in one line to make the surface exhaustive.packages/tempo/test/engine/timestamp_config.test.ts (1)
14-17: ⚡ Quick winRemove draft/internal-note comments from committed test code.
Line 14–Line 17 read like temporary author notes and make intent harder to follow. Keep only behavior-focused comments.
Suggested cleanup
- // 10 digits in ms mode should currently NOT short-circuit unless we improve the logic - // But wait, I'll update it to expect 1970 if it WAS treated as ms - // OR I'll use a 13-digit number for the ms test + // Use a millisecond-scale epoch value for ms mode assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/test/engine/timestamp_config.test.ts` around lines 14 - 17, The test contains draft/internal-note comments around the msVal declaration (variable msVal = 1715900000000) that should be removed; replace them with a concise, behavior-focused comment explaining the purpose of msVal (e.g., "use a 13-digit millisecond timestamp for ms-mode test") and ensure the msVal variable and its usage in the timestamp_config test remain unchanged; edit the block around msVal to remove the temporary author notes and keep only a single clear comment describing expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/CHANGELOG.md`:
- Line 16: Update the CHANGELOG entry that currently lists "Vitest 2.1.9" under
the "**Dependency Refresh**" bullet to the correct version "Vitest 4.1.5";
locate the line containing the text "Vitest 2.1.9" (the dependency-refresh
sentence) and replace that version string with "Vitest 4.1.5" so the changelog
accurately reflects the latest stable Vitest release.
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 89-98: The fractional-number branch currently uses
value.toString().split('.') which breaks on scientific notation and throws in
BigInt; replace that string-based parsing with numeric extraction: compute const
absVal = Math.abs(value), derive wholeNumber = BigInt(Math.trunc(absVal)),
derive fractionDigits = BigInt(Math.round((absVal - Math.trunc(absVal)) *
1_000_000_000)), normalize fractionDigits to be in 0..999_999_999 and if
rounding causes fractionDigits === 1_000_000_000 increment wholeNumber and set
fractionDigits to 0, then compute nano = (wholeNumber * scale) + (fractionDigits
* scale / 1_000_000_000n) and finally apply sign if value < 0; update the code
paths that assign whole, fraction and nano (symbols: value, whole,
fraction/fractionDigits, scale, nano) accordingly to avoid using
value.toString().
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 54-60: The overloads for prefix are unsound because passing plural
tokens (t.WEEKDAYS, t.MONTHS) can return plural-form keys but the signatures
force singular return types; update the declarations for the prefix function so
each plural input type returns the matching plural key type (add overloads like
prefix(str: t.WEEKDAYS): t.WEEKDAYS and prefix(str: t.MONTHS): t.MONTHS) and
keep the singular overloads and the generic fallback implementation intact so
the implementation signature still handles any input.
In `@packages/tempo/src/module/module.parse.ts`:
- Around line 255-295: The length gate wrongly prevents valid epoch integers
when a non-default unit is configured; update the isLong calculation in
module.parse.ts so that when state.config.timeStamp !== 'ms' you treat the
numeric string as an epoch candidate regardless of trim.length (i.e., set isLong
= true for non-'ms' units), then keep the existing branches that convert
strings/numbers to BigInt/Number and mark isEpoch; this change touches the
isLong variable and the type branches (type === 'String', type === 'Number', and
the isEpoch handling) so they correctly classify shorter second-based timestamps
like 946684800 as Epoch.
- Around line 186-190: The code in parse (around value.toDateTime() /
getTemporalIds) mutates state.config.timeZone and state.config.calendar, which
overwrites caller overrides for parse(value, { timeZone, calendar }) when the
input is a Tempo; instead compute tz and cal locally from getTemporalIds(res)
but do not assign them into state.config; return the arg with type
'Temporal.ZonedDateTime' and the computed res, and if compose needs the derived
tz/cal pass them in the returned arg's metadata (e.g., attach a local
zone/calendar property) rather than mutating state.config so parse for Tempo
respects caller-provided timeZone/calendar; update references to
state.config.timeZone / calendar in compose to prefer caller overrides and then
fall back to any tz/cal passed on the returned arg.
In `@packages/tempo/src/support/support.default.ts`:
- Around line 24-25: The hour alternation in the time and clock regexes (keys
time and clock in support.default.ts) is too broad (2[0-4]) and lets invalid
times like 24:30 pass; tighten the patterns so normal hours use 0–23 (2[0-3])
and only allow the literal end-of-day “24:00” (with optional :00 and fractional
seconds for clock) as a special case; update the two regexes and any equivalent
patterns used for Snippet[Token.hh] and Layout[Token.hms] to the constrained
alternation (0[0-9]|1[0-9]|2[0-3]|24:00(?:[:]?00(?:\.\d+)? )?) so only true
24:00 variants are accepted while rejecting 24:30/24:59:59.
In `@packages/tempo/src/support/support.init.ts`:
- Around line 240-242: The timeStamp branch currently calls
setProperty(state.config, optKey, arg.value) without validating units, causing
typos like "mss" to be treated as nanoseconds; update the case 'timeStamp'
handling to validate arg.value against the allowed timestamp/unit set (e.g.,
explicit list of accepted units/formats) before calling setProperty, and
reject/raise an error (or return a validation failure) for invalid values so
downstream numeric parsing cannot silently fallback to ns; locate the logic in
support.init.ts around the case 'timeStamp' and modify the handling there
(referencing case 'timeStamp', setProperty, state.config, optKey, arg.value).
In `@packages/tempo/test/engine/numeric_resolution.test.ts`:
- Around line 43-49: The test mutates global runtime config by calling
Tempo.init({ catch: true }) which can leak into other tests; change the test to
save and restore the previous config (or call Tempo.init with the prior
settings) using setup/teardown around this test so the global state is restored
after it runs, locating the call to Tempo.init in the 'should reject NaN with
custom error' test and ensuring after the test finishes the original Tempo.init
configuration is reapplied (or use beforeEach/afterEach to reset Tempo.init) to
avoid cross-test contamination.
In `@packages/tempo/test/issues/24-hour-overflow.test.ts`:
- Around line 16-24: Read the current time once into a single anchor (e.g.,
const anchor = Temporal.Now.zonedDateTimeISO()), compute the expected "tomorrow"
from that anchor, and then use that same anchor when producing the actual value
so both sides use the same reference; specifically, update the test to get
anchor before calling new Tempo('24:00'), compute tomorrow from anchor, and
either pass anchor into the Tempo constructor or call t.toDateTime(anchor)
(whichever API Tempo provides) so the test compares values derived from the
identical anchor instead of two different "now" calls.
---
Nitpick comments:
In `@packages/tempo/doc/tempo.parse.md`:
- Around line 114-138: Update the "Functional Alias Context" docs to match the
real AliasContext API: replace any mention of this.with(...) with this.set(...)
and explicitly list the available methods this.add(duration), this.set(input),
this.toNow(), this.toDateTime() and accessors this.hh / this.mi / this.ss; also
ensure the example alias functions call this.set(...) and this.add(...)
(referencing AliasContext, the example alias strings 'bedtime' and 'meeting',
and the methods this.set and this.add). Optionally append the remaining context
accessors yy/mm/dd/tz/cal/config in one line to make the surface exhaustive.
In `@packages/tempo/src/engine/engine.normalizer.ts`:
- Around line 112-134: The current prefix-based filter in resolveAliases risks
false positives; change the check inside resolveAliases to perform an exact
match against the alias-engine's group/type: for each key, resolve const alias =
aliasEngine?.getAlias(key) (or similar API) and if filter is provided only
continue when filter.some(f => f === key || (alias && (alias.type === f ||
alias.groupName === f))); if aliasEngine is missing fall back to the existing
key === f || key.startsWith(f) behavior to preserve hot-path performance. Ensure
you reference resolveAliases, aliasEngine and getAlias in the change.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 364-374: The code currently merges intl.relativeTime by
shallow-spreading which can drop a function value; change the merging logic in
the block that handles discovery.intl / discovery.relativeTime so you never
attempt to spread a function. Concretely: if discovery.relativeTime is a
function, set intl.relativeTime = discovery.relativeTime; else if
intl.relativeTime (from discovery.intl) is a function and discovery.relativeTime
is an object, keep intl.relativeTime as the function (do not merge); otherwise
(both are objects or undefined) merge with { ...intl.relativeTime,
...discovery.relativeTime } and then assign shape.config.intl = {
...shape.config.intl, ...intl }. Ensure you reference the existing symbols
discovery.intl, discovery.relativeTime, intl.relativeTime, and shape.config.intl
when making the change.
In `@packages/tempo/test/engine/timestamp_config.test.ts`:
- Around line 14-17: The test contains draft/internal-note comments around the
msVal declaration (variable msVal = 1715900000000) that should be removed;
replace them with a concise, behavior-focused comment explaining the purpose of
msVal (e.g., "use a 13-digit millisecond timestamp for ms-mode test") and ensure
the msVal variable and its usage in the timestamp_config test remain unchanged;
edit the block around msVal to remove the temporary author notes and keep only a
single clear comment describing expected behavior.
In `@packages/tempo/test/issues/issue-fixes.test.ts`:
- Line 2: Replace the value import of AliasContext with a type-only import to
match the other tests: change the import of AliasContext from
'#tempo/tempo.type.js' to a type import (i.e., use "import type { AliasContext }
...") so the test uses a type-only reference and remains compatible with
verbatimModuleSyntax; update the import statement where AliasContext is
referenced in the test file to the type-only form.
🪄 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: 3ed1f9ba-6a6b-48d5-b8be-6fb74e1e8c5c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
package.jsonpackages/library/package.jsonpackages/library/src/common/assertion.library.tspackages/library/src/common/temporal.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/index.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/core/sandbox-factory.test.tspackages/tempo/test/engine/functional_alias_chaining.test.tspackages/tempo/test/engine/numeric_resolution.test.tspackages/tempo/test/engine/timestamp_config.test.tspackages/tempo/test/instance/relative_date.test.tspackages/tempo/test/issues/24-hour-overflow.test.tspackages/tempo/test/issues/issue-fixes.test.tspackages/tempo/test/plugins/plugin.test.tspackages/tempo/test/plugins/reactive_registration.test.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/tempo/src/tempo.class.ts (1)
365-373: 💤 Low valueConsider documenting precedence when both
intl.relativeTimeandrelativeTimeare provided.The current logic silently ignores
discovery.relativeTime(when it's an object) ifintl.relativeTimeis already a function. While this prevents overwriting a function with an object (per the AI summary), users might be surprised if they provide both. A debug log or comment clarifying the precedence would help maintainability.📝 Optional: Add clarifying comment
const intl: t.IntlOptions = { ...discovery.intl }; if (discovery.relativeTime) { if (typeof discovery.relativeTime === 'function') { intl.relativeTime = discovery.relativeTime; } else if (!(typeof intl.relativeTime === 'function')) { + // Only merge object form if intl.relativeTime is not already a function intl.relativeTime = { ...intl.relativeTime, ...discovery.relativeTime }; } + // Note: if intl.relativeTime is a function and discovery.relativeTime is an object, + // the object is intentionally ignored to preserve the function }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 365 - 373, The code that merges discovery.relativeTime into intl (in Tempo class code handling shape.config.intl) silently skips discovery.relativeTime when intl.relativeTime is already a function; add a concise clarifying comment and/or a debug log that documents the precedence (i.e., "if intl.relativeTime is a function, it takes precedence over discovery.relativeTime object") near the block that checks typeof discovery.relativeTime and typeof intl.relativeTime, and optionally emit a debug/processLogger.debug call when both are present to make the behavior explicit for maintainers.packages/tempo/src/engine/engine.composer.ts (1)
82-119: 💤 Low valueFractional Number → epoch nanoseconds: addresses the prior scientific-notation crash.
Replacing the old
value.toString().split('.')parse withMath.trunc+Math.roundcorrectly avoids theBigInt('1e-9')failure mode that was raised previously, and the carry-on-rollover guard (fractionDigits === 1_000_000_000n) is the right safety net forMath.roundovershoot. The(whole * 1e9 + frac) * scale / 1e9formulation produces exact results forss/ms/usand degrades cleanly to truncation forns(where sub-nanosecond input has nowhere to go anyway).One small follow-up if you want to tighten readability: lift
scaleandmatchNameout of the chained ternaries into a small lookup, since the same unit ladder is repeated on lines 92 and 114.♻️ Optional readability tweak
- // 📏 Resolve multipliers for nanosecond conversion - const scale = unit === 'ss' ? 1_000_000_000n : (unit === 'ms' ? 1_000_000n : (unit === 'us' ? 1_000n : 1n)); + // 📏 Resolve multipliers for nanosecond conversion + const UNIT_TABLE = { ss: { scale: 1_000_000_000n, name: 'Seconds' }, ms: { scale: 1_000_000n, name: 'Milliseconds' }, us: { scale: 1_000n, name: 'Microseconds' }, ns: { scale: 1n, name: 'Nanoseconds' } } as const; + const { scale, name: matchName } = UNIT_TABLE[unit]; let nano: bigint; ... - // 🏷️ Log Result Metadata - const matchName = unit === 'ss' ? 'Seconds' : (unit === 'ms' ? 'Milliseconds' : (unit === 'us' ? 'Microseconds' : 'Nanoseconds')); onResult?.({ type, value, match: matchName });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.composer.ts` around lines 82 - 119, The code correctly handles fractional numbers but repeats the unit-to-scale and unit-to-label ternaries; refactor by creating a small lookup (e.g., map or const object) keyed by unit that provides both scale and matchName, then replace the chained ternaries used to compute `scale` and `matchName` with lookups; update uses of `scale` and `matchName` in the Number/BigInt case (around the `scale` declaration and the `onResult` call) to pull values from the lookup so the logic in that block becomes clearer and avoids duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 82-110: Add a v2.9.3 migration note explaining the breaking change
where BigInt inputs now respect the configured unit (the unit variable/option,
default 'ms') instead of being treated as raw nanoseconds; reference the
affected behavior in engine.composer.ts (the BigInt/Number case that multiplies
by scale) and include a concise example showing old vs new behavior and the
explicit workaround: pass an explicit nanosecond timestamp (e.g., construct
Tempo with timeStamp: 'ns') to restore pre-v2.9.3 semantics.
- Around line 24-26: The current use of hasOwn(config, 'timeZone') /
hasOwn(config, 'calendar') in engine.composer.ts is unreliable because init()
assigns system defaults as own properties on state.config (via support.init.ts's
Object.assign and setProperties), so derivedTz/derivedCal are never used; change
initialization or config detection so we can tell user-supplied keys from
defaults—for example, record which keys were explicitly passed in init() (e.g.,
store a Set like userProvidedKeys on state or config) or place defaults on the
prototype chain instead of as own properties; then update the selection logic in
engine.composer.ts (the finalTz/finalCal computation using hasOwn, derivedTz,
derivedCal) to check that new marker (e.g., userProvidedKeys.has('timeZone') or
Object.prototype.hasOwnProperty.call(config, ...) vs prototype check) so
explicit input timezones/calendars (derivedTz/derivedCal) are honored.
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Line 7: Remove the unused Tempo import: delete the `import Tempo from
'#tempo';` statement in engine.lexer.ts since the code uses `isTempo()` from
'#tempo/support' for type checks and does not reference `Tempo`; ensure no other
uses of the `Tempo` symbol remain in this file before committing.
In `@packages/tempo/src/module/module.parse.ts`:
- Around line 38-43: The conform function has a branch (in the
isZonedDateTimeLike + missing-term path) that returns undefined which later
causes compose (and parse) to destructure a nil arg; update that branch (and any
similar bare-undefined returns in conform) to return the standardized shape {
type, value, zone?, calendar? } (matching the other Temporal.ZonedDateTime
returns) so compose’s destructure (const { type, value, … } = arg) always
receives an object; locate the conform function and replace the bare `return
undefined as any` (and the symmetric branch noted near the
isTempo/isZonedDateTimeLike logic) with a proper object that preserves zone and
calendar when available.
- Around line 371-388: The current withState wrapper (function withState ->
inner function using fn and res) unwraps any object with a 'value' key which can
wrongly unwrap plain Groups objects; change the final unwrap logic so it only
returns res.value when res is an object that contains both 'type' and 'value'
keys (i.e., the TypeValue envelope returned by parse/conform/parseLayout),
otherwise return the whole res unchanged.
---
Nitpick comments:
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 82-119: The code correctly handles fractional numbers but repeats
the unit-to-scale and unit-to-label ternaries; refactor by creating a small
lookup (e.g., map or const object) keyed by unit that provides both scale and
matchName, then replace the chained ternaries used to compute `scale` and
`matchName` with lookups; update uses of `scale` and `matchName` in the
Number/BigInt case (around the `scale` declaration and the `onResult` call) to
pull values from the lookup so the logic in that block becomes clearer and
avoids duplication.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 365-373: The code that merges discovery.relativeTime into intl (in
Tempo class code handling shape.config.intl) silently skips
discovery.relativeTime when intl.relativeTime is already a function; add a
concise clarifying comment and/or a debug log that documents the precedence
(i.e., "if intl.relativeTime is a function, it takes precedence over
discovery.relativeTime object") near the block that checks typeof
discovery.relativeTime and typeof intl.relativeTime, and optionally emit a
debug/processLogger.debug call when both are present to make the behavior
explicit for maintainers.
🪄 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: 67f25fd1-1703-40cd-b986-97ac6fb00d59
📒 Files selected for processing (17)
packages/tempo/CHANGELOG.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/plan/licensing_architecture.mdpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.normalizer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/test/engine/numeric_resolution.test.tspackages/tempo/test/engine/timestamp_config.test.tspackages/tempo/test/issues/24-hour-overflow.test.tspackages/tempo/test/issues/issue-fixes.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/tempo/plan/licensing_architecture.md
- packages/tempo/doc/tempo.config.md
- packages/tempo/test/engine/timestamp_config.test.ts
- packages/tempo/doc/tempo.parse.md
- packages/tempo/test/issues/24-hour-overflow.test.ts
- packages/tempo/CHANGELOG.md
- packages/tempo/test/engine/numeric_resolution.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/src/support/support.init.ts
- packages/tempo/src/support/support.default.ts
- packages/tempo/src/engine/engine.normalizer.ts
Owner
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tempo/src/support/support.init.ts (1)
245-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalid
timeStampvalues are still persisted after logging.Validation now logs an error, but
setProperty(state.config, optKey, unit ?? arg.value)runs unconditionally:
- For an invalid string (e.g.
'mss'), the bad value is still written tostate.config.timeStamp. DownstreamUNIT_LOOKUP[unit] ?? UNIT_LOOKUP.msmasks this, butstate.config.get('timeStamp')will return the invalid token.- For an object form without a
.unitproperty,unitisundefined, so validation is skipped and the entire object is written tostate.config.timeStamp.logError(null, …)is inconsistent with the rest of this file (e.g. lines 150, 154 uselogError(state.config, …)).🛡️ Suggested fix
case 'timeStamp': { - const unit = isString(arg.value) ? arg.value : arg.value?.unit; - if (unit && !['ss', 'ms', 'us', 'ns'].includes(unit)) - logError(null, `Invalid timeStamp unit: ${unit}. Expected 'ss', 'ms', 'us', or 'ns'.`); - - setProperty(state.config, optKey, unit ?? arg.value); + const unit = isString(arg.value) ? arg.value : arg.value?.unit; + if (!unit || !['ss', 'ms', 'us', 'ns'].includes(unit)) { + logError(state.config, `[Tempo#extend] Invalid timeStamp unit: ${String(unit ?? arg.value)}. Expected 'ss', 'ms', 'us', or 'ns'.`); + break; + } + setProperty(state.config, optKey, unit); break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/support/support.init.ts` around lines 245 - 252, The timeStamp branch currently logs errors but still persists invalid values; change the flow in the 'timeStamp' case so you only call setProperty(state.config, optKey, ...) when the value is valid: compute unit from arg.value (string or arg.value.unit), if unit is a string and one of ['ss','ms','us','ns'] then setProperty with that unit; if arg.value is an object require a defined .unit that is valid before writing the whole object; otherwise do not persist the invalid value and call logError(state.config, ...) instead of logError(null, ...) to match the file's logging convention and avoid storing bad tokens that UNIT_LOOKUP masking would hide.
🧹 Nitpick comments (1)
packages/tempo/src/engine/engine.composer.ts (1)
96-100: 💤 Low valueRedundant NaN check.
Number.isFinite(NaN)returnsfalse, so!Number.isFinite(value)already coversNaNalong with±Infinity. TheNumber.isNaN(value)clause can be dropped.♻️ Proposed simplification
- if (type === 'Number' && (Number.isNaN(value) || !Number.isFinite(value))) { + if (type === 'Number' && !Number.isFinite(value)) { logError(config, `Invalid Tempo number: ${value}`); temporal = today; break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.composer.ts` around lines 96 - 100, The check for NaN is redundant: in the block where you validate numbers (the condition checking type === 'Number'), remove the explicit Number.isNaN(value) check and rely solely on !Number.isFinite(value) to catch NaN and ±Infinity; update the conditional that calls logError(config, `Invalid Tempo number: ${value}`) and sets temporal = today so it only tests !Number.isFinite(value) before logging and assigning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 51-63: The prefix() implementation currently truncates to three
characters which breaks prefix matching for short inputs (e.g., "Tu", "Se") and
causes num()/parseWeekday() to fail; update prefix(str) to perform
case-insensitive prefix matching against the known sets (t.WEEKDAYS and t.MONTHS
or the module’s weekday/month arrays) instead of slicing to 3 chars: normalize
the input, iterate the weekday and month lists using startsWith to find the
first matching full name, return that full name (or the original string if no
match), and keep the initial isString() guard; reference function prefix and
callers like num() and parseWeekday when making the change.
---
Duplicate comments:
In `@packages/tempo/src/support/support.init.ts`:
- Around line 245-252: The timeStamp branch currently logs errors but still
persists invalid values; change the flow in the 'timeStamp' case so you only
call setProperty(state.config, optKey, ...) when the value is valid: compute
unit from arg.value (string or arg.value.unit), if unit is a string and one of
['ss','ms','us','ns'] then setProperty with that unit; if arg.value is an object
require a defined .unit that is valid before writing the whole object; otherwise
do not persist the invalid value and call logError(state.config, ...) instead of
logError(null, ...) to match the file's logging convention and avoid storing bad
tokens that UNIT_LOOKUP masking would hide.
---
Nitpick comments:
In `@packages/tempo/src/engine/engine.composer.ts`:
- Around line 96-100: The check for NaN is redundant: in the block where you
validate numbers (the condition checking type === 'Number'), remove the explicit
Number.isNaN(value) check and rely solely on !Number.isFinite(value) to catch
NaN and ±Infinity; update the conditional that calls logError(config, `Invalid
Tempo number: ${value}`) and sets temporal = today so it only tests
!Number.isFinite(value) before logging and assigning.
🪄 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: 7f8edffc-dd84-4b97-9eaa-0c199af2b6e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonpackages/tempo/README.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/README.md
- packages/tempo/doc/migration-guide.md
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/src/tempo.class.ts
- packages/tempo/src/tempo.type.ts
- packages/tempo/package.json
- packages/tempo/src/module/module.parse.ts
Comment on lines
+51
to
63
| /** conform weekday names using prefix matching */ | ||
| export function prefix(str: t.WEEKDAY | t.WEEKDAYS): t.WEEKDAY; | ||
| /** conform month names using prefix matching */ | ||
| export function prefix(str: t.MONTH | t.MONTHS): t.MONTH; | ||
| /** conform any string to a weekday/month prefix if possible */ | ||
| export function prefix(str: string): t.WEEKDAY | t.MONTH | string; | ||
| /** implementation */ | ||
| export function prefix(str: any): any { | ||
| if (!isString(str)) return str; | ||
| const low = str.trim().toLowerCase(); | ||
| if (low === '') return str; | ||
|
|
||
| // search in weekdays and months | ||
| for (const dict of [enums.WEEKDAY, enums.WEEKDAYS, enums.MONTH, enums.MONTHS]) { | ||
| const found = Object.keys(dict).find((key: string) => (key as string).toLowerCase().startsWith(low)); | ||
| if (found) return found as T; | ||
| } | ||
|
|
||
| return str; | ||
| const val = str.trim(); | ||
| return val.charAt(0).toUpperCase() + val.slice(1, 3).toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Restore actual prefix matching in prefix().
Line 62 is now 3-character truncation, not prefix matching. Short inputs like "Tu" or "Se" no longer resolve, so num() and parseWeekday() lose abbreviated month/weekday support.
💡 Minimal fix
/** implementation */
export function prefix(str: any): any {
if (!isString(str)) return str;
- const val = str.trim();
- return val.charAt(0).toUpperCase() + val.slice(1, 3).toLowerCase();
+ const low = str.trim().toLowerCase();
+ for (const table of [enums.WEEKDAY, enums.MONTH]) {
+ const match = ownKeys(table).find(key => {
+ const normalized = key.toLowerCase();
+ return normalized.startsWith(low) || low.startsWith(normalized);
+ });
+ if (match) return match;
+ }
+ return str;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tempo/src/engine/engine.lexer.ts` around lines 51 - 63, The prefix()
implementation currently truncates to three characters which breaks prefix
matching for short inputs (e.g., "Tu", "Se") and causes num()/parseWeekday() to
fail; update prefix(str) to perform case-insensitive prefix matching against the
known sets (t.WEEKDAYS and t.MONTHS or the module’s weekday/month arrays)
instead of slicing to 3 chars: normalize the input, iterate the weekday and
month lists using startsWith to find the first matching full name, return that
full name (or the original string if no match), and keep the initial isString()
guard; reference function prefix and callers like num() and parseWeekday when
making the change.
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.
small code-fixes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests