Release/parse engine#18
Merged
magmacomputing merged 6 commits intomainfrom Apr 22, 2026
Merged
Conversation
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 10 seconds. ⌛ 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 (52)
📝 WalkthroughWalkthroughThis PR extracts parsing into a standalone Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant parse as parse(value, options)
participant runtime as getRuntime()
participant init as init()
participant state as ParseEngine
participant temporal as Temporal API
Caller->>parse: parse(value, options)
parse->>runtime: getRuntime()
alt runtime.state undefined
parse->>init: init()
init->>runtime: runtime.state = {...}
end
parse->>parse: create local state (proto from runtime.state)
parse->>parse: extendState(local state, options)
parse->>state: ParseEngine.parse(state, value)
state->>state: apply ignorePattern -> normalize input
state->>temporal: construct ZonedDateTime
temporal-->>state: ZonedDateTime
state-->>parse: result
parse-->>Caller: Temporal.ZonedDateTime
sequenceDiagram
actor Developer
participant Tempo as Tempo
participant runtime as getRuntime()
participant init as init()
participant instance as Tempo#resolve
Developer->>Tempo: Tempo.init(options)
Tempo->>runtime: getRuntime()
alt no runtime.state
Tempo->>init: init(options)
init->>runtime: set runtime.state
else
runtime-->>Tempo: runtime.state
end
Developer->>Tempo: new Tempo(...)
Tempo->>instance: `#resolve`() (deferred parse)
instance->>runtime: read runtime.state.ignorePattern
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Poem
🚥 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 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.
Actionable comments posted: 16
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/support/tempo.runtime.ts (1)
13-26:⚠️ Potential issue | 🟡 MinorDoc drift: class-level JSDoc still mentions
BRIDGE.You already updated
getRuntime()'s comment to say$Bridge(Line 118), but the class-level block still referencesthe BRIDGE symbol above(Lines 15, 21). Worth renaming for consistency so readers don't chase aBRIDGEidentifier that no longer exists in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.runtime.ts` around lines 13 - 26, Update the class-level JSDoc in TempoRuntime to replace references to the old BRIDGE identifier with the new $Bridge naming used elsewhere: change mentions like "the BRIDGE symbol above" and "globalThis[BRIDGE]" in the block to reference "$Bridge" (and "globalThis[$Bridge]" where appropriate) so the docs match the updated getRuntime() comment and the scoped-runtime descriptions; ensure references to TempoRuntime.createScoped() and the non-enumerable/non-configurable descriptor language remain intact.packages/tempo/src/tempo.class.ts (1)
736-811:⚠️ Potential issue | 🟠 Major
Tempo.init()unconditionally overwrites state built by runtimeinit(), duplicating initialization logic and introducing a bug.Lines 743–744 fetch/create the runtime state, but lines 749–774 immediately replace
Tempo.#global.parseandTempo.#global.configwith freshmarkConfigobjects, discarding the work performed bypackages/tempo/src/support/tempo.init.ts. This duplication creates maintenance burden and a concrete issue:
Missing
patternproperty: The runtimeinit()initializesparse.patternas an emptyMap(line 39, tempo.init.ts), butTempo.init()omits it. Code at line 474 (tempo.class.ts) assumes the property exists on the prototype chain for shadowing to work correctly; ifTempo.init()runs first,patternwill be undefined.Divergent locale resolution:
init()uses(getDateTimeFormat() as any).locale ?? 'en-US', whileTempo.init()usesTempo.#locale()with canonicalLocale validation and navigator fallbacks—both correct but inconsistent.mdyLocales processing differs:
init()stores a plain array, whileTempo.init()transforms viaTempo.#mdyLocales()into objects with{ locale, timeZones }structure. The Tempo version is more complete.Consider consolidating initialization: have runtime
init()establish the complete base state (includingpattern), and layer Tempo-specific customizations (locale resolver, mdyLocales transformation, discovery wiring) on top rather than rebuilding from scratch.🤖 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 736 - 811, Tempo.init() is replacing the runtime state produced by getRuntime()/init() (overwriting Tempo.#global.parse and Tempo.#global.config) which drops properties like parse.pattern and duplicates/ diverges locale/mdy processing; instead, change Tempo.init to reuse and extend the runtime state: call const state = rt.state ?? init() and only patch or augment missing/Tempo-specific pieces (e.g. ensure parse.pattern exists if missing, apply Tempo.#mdyLocales to state.parse.mdyLocales, replace locale resolution via Tempo.#locale() only if state.config.locale is absent or needs canonicalizing, and set Tempo.#global.config via Tempo.#setConfig/Tempo.#setDiscovery rather than reconstructing a fresh config object); locate the logic around Tempo.init, Tempo.#global.parse, Tempo.#global.config, Tempo.#locale, Tempo.#mdyLocales, and Tempo.#setConfig to implement these non-destructive merges so runtime init() remains authoritative.
🧹 Nitpick comments (10)
packages/tempo/src/tempo.type.ts (1)
12-12: Duplicate type alias — reuse the importedIgnore.
Ignoreis already imported on line 12 fromtempo.default.jsand is defined identically asstring | string[] | (() => string | string[]). The localIgnoresalias at line 233 duplicates it.Proposed fix
- /** structured configuration for Global Discovery via Symbol.for('$Tempo') */ - type Ignores = string | string[] | (() => string | string[]); export interface Discovery { ... - /** noise words to ignore during parsing via Tempo.ignore() */ ignore?: Ignores + /** noise words to ignore during parsing via Tempo.ignore() */ ignore?: IgnoreAlso applies to: 233-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.type.ts` at line 12, Remove the duplicate local type alias Ignores and reuse the already imported Ignore from tempo.default.js: delete the Ignores declaration (the alias defined around the previous Ignores symbol) and replace all references/usages of Ignores in this file with Ignore so the file uses the single imported type instead of redefining it.packages/tempo/test/standalone_parse.test.ts (1)
6-16: Minor: midnight boundary flake.
parse('tomorrow')and the localTemporal.Now.zonedDateTimeISO()are sampled at slightly different instants; if the test runs across a date rollover, the assertions onyear/month/daymay flap. Samplingnowbeforeparseand using it as a fixed anchor (if the API supports it) — or just accepting ±1 day — avoids the flake.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/standalone_parse.test.ts` around lines 6 - 16, The test flakes because parse('tomorrow') and Temporal.Now.zonedDateTimeISO() are taken at different instants; sample now before calling parse and use that fixed value as the anchor when invoking parse (if parse accepts an anchor) or compute the expected tomorrow from that sampled now and compare to zdt; reference the parse(...) call, the Temporal.Now.zonedDateTimeISO() sampling (use a local variable now), and the zdt assertions so you replace the current interleaved sampling with a single anchored sample.packages/tempo/src/tempo.class.ts (1)
396-402:#setIgnoresruns twice per#setConfigcall.It's invoked directly here (Line 400) and again as the first statement of
#setPatterns(Line 462), which is called immediately after on Line 402. The second pass is idempotent, but redundant allocation of theignorePatternRegExp happens on every configuration change. Suggest picking one site — probably#setPatternssince it's the centralized pattern rebuilder — and dropping the other.🤖 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 396 - 402, Tempo.#setIgnores is being called twice during configuration: once directly in the `#setConfig` flow and again immediately inside Tempo.#setPatterns which is invoked next, causing redundant RegExp allocation; remove the duplicate call by deleting the direct invocation of Tempo.#setIgnores from the `#setConfig` sequence (keep the call inside Tempo.#setPatterns so ignorePattern rebuilding remains centralized), and ensure any initialization assumptions in `#setPatterns` still hold when it is the sole caller.packages/tempo/index.md (1)
260-262: Optional: hoist the de-duplicated TZ list to acomputed.
[...new Set(commonZones)]allocates a newSetand array on every render. SincecommonZonesis effectively static, consider extracting acomputed(() => [...new Set(commonZones.value)])(or just de-duplicate once at initialization) to avoid the churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/index.md` around lines 260 - 262, The template currently reconstructs the de-duplicated timezone array each render via [...new Set(commonZones)], which causes unnecessary allocations; create a computed property (e.g., uniqueZones or dedupedCommonZones) that returns computed(() => [...new Set(commonZones.value)]) or de-duplicate once at init, then change the <option v-for="tz in [...new Set(commonZones)]"> to iterate over that computed (referencing commonZones and selectedTz) so the select (tempo-tz-select) uses the stable de-duplicated list.packages/tempo/src/support/tempo.init.ts (1)
66-72: FallbackasTypediverges from the real helper's output shape.The inline fallback returns
{ type: typeof v, value: v }(lowercase JS typeof strings), whereas the realasTypefrom#library/type.library.jsreturns capitalized tags ('String','Object','Array', …). TodayextendStateonly readsarg.value, so it's benign, but if anyone later introduces a branch onarg.type, the fallback path will silently diverge. Consider either removing the fallback (requireLibraryto be present) or normalizing capitalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.init.ts` around lines 66 - 72, The fallback asType implementation diverges from the real Library.asType by returning lowercase typeof tags; update the fallback used where getRuntime().modules['Library'] is missing so asType returns the same shape/capitalized type tags as the real helper (e.g., "String", "Object", "Array", "RegExp", "Undefined", etc.) and preserves value under value; locate the inline fallback near getRuntime().modules['Library'] and replace the asType arrow with logic that maps JS typeof/Array/RegExp checks to the capitalized type strings to match the real asType output expected by code that may read arg.type (e.g., extendState).packages/tempo/src/support/tempo.runtime.ts (1)
46-48: Consider a narrower type forstate.
state?: anyopts the runtime out of type-checking for what is now the primary cross-module configuration surface. Even a lightstate?: t.Internal.State(or a dedicatedRuntimeStateinterface matching whatinit()assembles) would protect callers likepackages/tempo/src/tempo.parse.tsandpackages/tempo/src/plugin/module/module.parse.tsfrom silent shape drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.runtime.ts` around lines 46 - 48, Replace the overly-broad "state?: any" on the runtime class with a tighter type: either use the existing t.Internal.State type if it matches the structure assembled in init(), or define a dedicated RuntimeState interface that mirrors init()'s assembled shape and use "state?: RuntimeState"; update the runtime property declaration (state) and any related signatures (e.g., init()) so callers like tempo.parse.ts and module.parse.ts get compile-time shape checking and adjust imports/exports to reference the new type.packages/tempo/src/plugin/module/module.parse.ts (2)
350-355: Minor: doublegetRuntime().modules['Tempo']lookup on one line.
srcresolvesgetRuntime().modules['Tempo']twice in the same ternary (forisEventtrue and false branches) whileTempoClassalready holds the value earlier in this method. Prefer reusing a single local:♻️ Suggested cleanup
- const src = isGlobal ? (isEvent ? (getRuntime().modules['Tempo'] as any)[sym.$Internal]().parse.event : (getRuntime().modules['Tempo'] as any)[sym.$Internal]().parse.period) : (isEvent ? state.parse.event : state.parse.period); + const globalParse = isGlobal ? (TempoClass as any)?.[sym.$Internal]?.().parse : undefined; + const src = isGlobal + ? (isEvent ? globalParse?.event : globalParse?.period) + : (isEvent ? state.parse.event : state.parse.period);Also gracefully handles the standalone path where
TempoClassmay be undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.parse.ts` around lines 350 - 355, The code does a double getRuntime().modules['Tempo'] lookup when computing src and doesn't handle TempoClass being undefined; reuse the already-available TempoClass variable (or retrieve it once into a local) and derive an internalParse = TempoClass ? (TempoClass[sym.$Internal]().parse) : undefined, then set src by selecting internalParse.event/internalParse.period when isGlobal and falling back to state.parse.event/state.parse.period otherwise; ensure you guard against TempoClass/ internalParse being undefined before calling ownEntries.
19-22:withStatesilently drops the first argument whenthisis a Tempo.When invoked via
Tempo#parsestyle (boundthis),args.shift()isn't called, so callers must never pass astatepositionally in that mode. Conversely, when called standalone, the first arg is consumed as state regardless of its shape. A lightweight sanity check (e.g., thatstate?.config && state?.parsewhen!isTempo(this)) would make misuse surface immediately instead of turning into NPEs deep inside_ParseEngine.*.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.parse.ts` around lines 19 - 22, withState currently silently drops or misinterprets the first argument when called with a bound Tempo this, leading to confusing NPEs; modify the wrapper (withState) so that after computing state (const state = isTempo(this) ? (this as any)[sym.$Internal]() : args.shift()) you perform a lightweight sanity check when !isTempo(this) — e.g. assert state is an object and has the expected shape (state?.config && state?.parse) — and throw a clear TypeError mentioning withState/Tempo#parse if the check fails; keep the existing behavior for the isTempo(this) branch and ensure the error message references _ParseEngine to help debugging.packages/tempo/src/plugin/module/module.term.ts (1)
415-433: ThegetSafeFallbackStepcall at lines 423 and 431 is ineffective and can be removed.
getSafeFallbackStepreturns1for indices ≤2 and0for finer units, but the step object builderstepVal === 0 ? 1 : stepValalways coerces the result to1. SincegetSafeFallbackStepis called only in these two locations (no other callers in the codebase), and both ignore the0signal, the function contributes nothing. Replace with direct{ [${stepUnit}s]: 1 }and remove the call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.term.ts` around lines 415 - 433, In getStep (inside module.term.ts) the calls to getSafeFallbackStep are redundant because the result is always coerced to 1; replace both call sites so the function returns a literal step object like { [`${stepUnit}s`]: 1 } (for the rollover and fallback branches) and remove or stop referencing getSafeFallbackStep here; if getSafeFallbackStep has no other callers you can delete it entirely, otherwise leave it but stop calling it from getStep.packages/tempo/src/support/tempo.util.ts (1)
16-17:createsilently throws on missing prototype chain entry.
Object.create(proto(obj)[name])will raiseTypeError: Object prototype may only be an Object or nullifnamedoes not resolve to an object on the parent prototype. For an@internalhelper this may be acceptable, but a narrower type or an explicit guard would make the failure mode obvious at call sites.Optional hardening
-export const create = <T extends object>(obj: object, name: string): T => Object.create(proto(obj)[name]); +export const create = <T extends object>(obj: object, name: string): T => { + const parent = proto(obj)?.[name]; + if (parent === null || (typeof parent === 'object')) return Object.create(parent); + throw new TypeError(`create(): prototype['${name}'] is not an object`); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.util.ts` around lines 16 - 17, The create helper (create<T extends object>(obj: object, name: string)) currently calls Object.create(proto(obj)[name]) which will throw a generic TypeError if the prototype entry is missing or not an object; add an explicit guard that reads const entry = proto(obj)[name] and if entry is not an object and not null throw a clear, descriptive Error (or return a narrower typed result/optional) before calling Object.create(entry) so failure modes are explicit at call sites; reference the create function and proto(...) access when implementing the check and error message.
🤖 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/doc/installation.md`:
- Around line 112-117: Update the "Latest" bullet in the Versioning Policy to
use a concrete CDN URL instead of the ambiguous `@magmacomputing/tempo/...`
specifier: change the example to
`https://cdn.jsdelivr.net/npm/@magmacomputing/tempo/...` (matching the JSDelivr
format used in the "JSDelivr" bullet) and add a short note that JSDelivr will
resolve a missing version tag to the latest published release so omitting the
version yields the latest release.
- Around line 49-70: The three browser bundle filenames referenced are
inconsistent—packages/tempo/doc/installation.md uses dist/tempo.bundle.esm.js,
README.md uses dist/tempo.bundle.js (UMD), and public/esm_full.index.html uses
dist/tempo.index.js—so pick one canonical URL for the ESM browser entry and
another for the UMD entry and update all three locations to match; specifically,
decide whether the ESM browser bundle should be named tempo.bundle.esm.js (or
tempo.index.js) and ensure packages/tempo/doc/installation.md, README.md, and
public/esm_full.index.html all reference that same ESM filename, and likewise
unify the UMD filename across those files if a UMD example is kept.
In `@packages/tempo/doc/tempo.config.md`:
- Around line 169-176: Clarify whether the ignore option merges with or replaces
defaults in the docs: update the example around Tempo.init and new Tempo to
either (A) explicitly include the default token 'at' in the ignore array (e.g.,
show ['at','o-clock']) if ignore replaces defaults, or (B) add a short sentence
stating that Tempo.init and instance options merge with the default ignore list
if merging is intended; reference the symbols Tempo.init and the Tempo
constructor/new Tempo(...) so readers know where the behavior applies.
In `@packages/tempo/index.md`:
- Around line 116-117: The ticker call is passing timeZone as a top-level
Ticker.Options field which is ignored; update the Tempo.ticker invocation so
timeZone is nested under the seed option (so it is propagated to the Tempo
instance) — e.g., change the options object passed to Tempo.ticker (the one with
seconds: 1) to include seed: { timeZone: selectedTz.value } instead of a
top-level timeZone; ensure any other calls like new Tempo({ timeZone: ... })
remain correct and that the sync callback binding uses the ticker returned by
Tempo.ticker.
In `@packages/tempo/package.json`:
- Around line 56-59: The package.json sideEffects array is missing an entry for
the built parse bundle, so add the glob "**/tempo.parse.js" to the sideEffects
array (mirroring the existing "**/tempo.index.js" entry) so the emitted
dist/tempo.parse.js (from src/tempo.parse.ts which imports
`#library/temporal.polyfill.js`) cannot be tree-shaken away; update the
package.json sideEffects list to include "**/tempo.parse.js".
In `@packages/tempo/public/esm_full.index.html`:
- Around line 207-213: The visible code sample shows a lite pattern using
imports from '@magmacomputing/tempo/core' and '@magmacomputing/tempo/mutate' and
calls Tempo.extend(MutateModule), but the running page actually loads the full
bundle via '@magmacomputing/tempo' in the importmap; either update the importmap
to add entries for the subpaths ('@magmacomputing/tempo/core' and
'@magmacomputing/tempo/mutate') mapping them to the correct bundle modules, or
change the visible snippet to import from '@magmacomputing/tempo' and remove the
Tempo.extend usage to match the executed <script type="module"> so users copying
the snippet won't hit "Failed to resolve module specifier" errors.
- Around line 231-239: The importmap JSON in the <script type="importmap"> block
is invalid due to a trailing comma after the "@magmacomputing/tempo" entry and
duplicated closing braces; fix the importmap by removing the trailing comma and
keeping a single closing brace pair so the JSON is valid and the subsequent
import of "Tempo" from "@magmacomputing/tempo" resolves correctly.
In `@packages/tempo/README.md`:
- Around line 1-29: The README uses inline style="" attributes on HTML elements
(<table>, <tr>, <td>, <h1>, <p>, <img>) which GitHub strips; update README.md by
removing style attributes and converting to native markdown (use "# Tempo" for
the title, a normal paragraph for the tagline, and markdown table syntax or a
simple badge line for the shields) or at minimum keep only supported HTML
attributes (e.g., align, width, alt) on the <img> and table elements so the
content renders on GitHub/npm; edit the HTML block that contains the <table> /
<h1> / <p> / <img> to replace styled elements with markdown equivalents and
remove the style="" occurrences.
- Around line 53-59: Replace the two occurrences of the Deno install command
that read "deno add `@magmacomputing/tempo`" with the explicit npm-prefixed form
"deno add npm:`@magmacomputing/tempo`" so the README entry and the installation
doc clearly indicate the npm source; update both instances flagged in the
review.
In `@packages/tempo/src/core.index.ts`:
- Line 4: Remove the re-export of parse from core to avoid loading the parse
module when importing the lightweight public entrypoint; edit core.index.ts to
delete the line exporting parse (the export { parse } from './tempo.parse.js'
statement) so core no longer imports tempo.parse.js, and rely on the dedicated
public path ./parse and the full entrypoint tempo.index.ts which already exposes
parse.
In `@packages/tempo/src/plugin/module/module.parse.ts`:
- Around line 378-395: The code currently constructs a real Tempo instance via
new (TempoClass as any)(dateTime, state.config) for every function-typed
handler; change this to always use the lightweight plain host object (the one
providing add, set, toNow, toDateTime and sym.$isTempo) as the handler
this-context when invoking (definition as Function).call(host) to avoid
expensive parsing. If you detect a real Tempo instance is actually required,
only instantiate it with a deferred mode (e.g., new TempoClass(dateTime, {
...state.config, mode: 'defer' })) rather than the full eager config; keep the
rest of the result-handling (isTempo/isZonedDateTime/toDateTime/state.zdt
fallback) unchanged.
In `@packages/tempo/src/plugin/plugin.util.ts`:
- Around line 8-13: getHost currently prefers the global TempoClass over the
actual instance constructor which causes Tempo subclasses to be treated as base
Tempo at clone boundaries; update getHost so that when isTempo(t) is true it
returns the instance's constructor ((t as any).constructor) and only falls back
to getRuntime().modules['Tempo'] if the instance constructor is missing, while
keeping the existing behavior for function/class inputs (isFunction/isClass) and
using the runtime TempoClass as fallback for non-instance inputs; modify the
branches in getHost accordingly to prefer the instance constructor for Tempo
instances.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 79-100: The switch cases for
'snippet'|'layout'|'event'|'period'|'ignore' declare const rule which is
currently scoped to the entire switch and triggers Biome's noSwitchDeclarations;
fix by wrapping the entire case body in its own block (add { ... } around the
code inside that case) so declarations like const rule (and any future locals)
are block-scoped; ensure the block encloses the hasOwn(state.parse, optKey)
check, the assignment state.parse[optKey] = create(state.parse, optKey), the
const rule = state.parse[optKey] line, the ['snippet','layout'] branch using
collect(arg.value, ...) and the else branch using
asArray(arg.value).forEach(...) with Object.assign and rule[elm] = elm.
In `@packages/tempo/src/support/tempo.util.ts`:
- Around line 38-56: The line target ??= {} inside collect is misleading and
creates a local object that never escapes; remove that guard so collect only
mutates the provided target (delete the target ??= {} line), keep the parameter
typed as Record<symbol, any>, and ensure callers always pass a real object (or,
if you prefer the alternate fix, change collect to accept target?:
Record<symbol, any> and return the target after populating it so the fallback
object is observable).
In `@packages/tempo/src/tempo.parse.ts`:
- Around line 18-32: The parse function currently mutates the caller's options
and fails to brand the prototype-shadowed state objects; fix by creating a local
copy (e.g., localOptions = { ...options }) and apply the default mode on that
copy instead of doing options.mode ??= 'strict', then pass localOptions into
extendState; additionally, after creating state.config and state.parse with
Object.create in parse, call markConfig(state.config) and
markConfig(state.parse) to apply the expected branding (and ensure markConfig is
imported from `#library/symbol.library.js`); keep all other logic (getRuntime,
init, extendState) unchanged.
In `@packages/tempo/test/standalone_parse.test.ts`:
- Around line 38-51: This test calls Tempo.init which mutates the shared
singleton runtime state and can leak into other tests; fix by either marking the
test to run in isolation (e.g., convert the test to a sequential test or place
it in an isolated vitest environment) or by explicitly resetting Tempo's global
state around the test: add a beforeEach/afterEach that calls Tempo.init(...)
with a known baseline configuration (clearing any custom event registrations
like 'party-time' and restoring defaults such as ignore/mdyLocales) so
parse('party-time') only sees the intended mapping during this test and no state
bleeds to other tests.
---
Outside diff comments:
In `@packages/tempo/src/support/tempo.runtime.ts`:
- Around line 13-26: Update the class-level JSDoc in TempoRuntime to replace
references to the old BRIDGE identifier with the new $Bridge naming used
elsewhere: change mentions like "the BRIDGE symbol above" and
"globalThis[BRIDGE]" in the block to reference "$Bridge" (and
"globalThis[$Bridge]" where appropriate) so the docs match the updated
getRuntime() comment and the scoped-runtime descriptions; ensure references to
TempoRuntime.createScoped() and the non-enumerable/non-configurable descriptor
language remain intact.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 736-811: Tempo.init() is replacing the runtime state produced by
getRuntime()/init() (overwriting Tempo.#global.parse and Tempo.#global.config)
which drops properties like parse.pattern and duplicates/ diverges locale/mdy
processing; instead, change Tempo.init to reuse and extend the runtime state:
call const state = rt.state ?? init() and only patch or augment
missing/Tempo-specific pieces (e.g. ensure parse.pattern exists if missing,
apply Tempo.#mdyLocales to state.parse.mdyLocales, replace locale resolution via
Tempo.#locale() only if state.config.locale is absent or needs canonicalizing,
and set Tempo.#global.config via Tempo.#setConfig/Tempo.#setDiscovery rather
than reconstructing a fresh config object); locate the logic around Tempo.init,
Tempo.#global.parse, Tempo.#global.config, Tempo.#locale, Tempo.#mdyLocales, and
Tempo.#setConfig to implement these non-destructive merges so runtime init()
remains authoritative.
---
Nitpick comments:
In `@packages/tempo/index.md`:
- Around line 260-262: The template currently reconstructs the de-duplicated
timezone array each render via [...new Set(commonZones)], which causes
unnecessary allocations; create a computed property (e.g., uniqueZones or
dedupedCommonZones) that returns computed(() => [...new Set(commonZones.value)])
or de-duplicate once at init, then change the <option v-for="tz in [...new
Set(commonZones)]"> to iterate over that computed (referencing commonZones and
selectedTz) so the select (tempo-tz-select) uses the stable de-duplicated list.
In `@packages/tempo/src/plugin/module/module.parse.ts`:
- Around line 350-355: The code does a double getRuntime().modules['Tempo']
lookup when computing src and doesn't handle TempoClass being undefined; reuse
the already-available TempoClass variable (or retrieve it once into a local) and
derive an internalParse = TempoClass ? (TempoClass[sym.$Internal]().parse) :
undefined, then set src by selecting internalParse.event/internalParse.period
when isGlobal and falling back to state.parse.event/state.parse.period
otherwise; ensure you guard against TempoClass/ internalParse being undefined
before calling ownEntries.
- Around line 19-22: withState currently silently drops or misinterprets the
first argument when called with a bound Tempo this, leading to confusing NPEs;
modify the wrapper (withState) so that after computing state (const state =
isTempo(this) ? (this as any)[sym.$Internal]() : args.shift()) you perform a
lightweight sanity check when !isTempo(this) — e.g. assert state is an object
and has the expected shape (state?.config && state?.parse) — and throw a clear
TypeError mentioning withState/Tempo#parse if the check fails; keep the existing
behavior for the isTempo(this) branch and ensure the error message references
_ParseEngine to help debugging.
In `@packages/tempo/src/plugin/module/module.term.ts`:
- Around line 415-433: In getStep (inside module.term.ts) the calls to
getSafeFallbackStep are redundant because the result is always coerced to 1;
replace both call sites so the function returns a literal step object like {
[`${stepUnit}s`]: 1 } (for the rollover and fallback branches) and remove or
stop referencing getSafeFallbackStep here; if getSafeFallbackStep has no other
callers you can delete it entirely, otherwise leave it but stop calling it from
getStep.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 66-72: The fallback asType implementation diverges from the real
Library.asType by returning lowercase typeof tags; update the fallback used
where getRuntime().modules['Library'] is missing so asType returns the same
shape/capitalized type tags as the real helper (e.g., "String", "Object",
"Array", "RegExp", "Undefined", etc.) and preserves value under value; locate
the inline fallback near getRuntime().modules['Library'] and replace the asType
arrow with logic that maps JS typeof/Array/RegExp checks to the capitalized type
strings to match the real asType output expected by code that may read arg.type
(e.g., extendState).
In `@packages/tempo/src/support/tempo.runtime.ts`:
- Around line 46-48: Replace the overly-broad "state?: any" on the runtime class
with a tighter type: either use the existing t.Internal.State type if it matches
the structure assembled in init(), or define a dedicated RuntimeState interface
that mirrors init()'s assembled shape and use "state?: RuntimeState"; update the
runtime property declaration (state) and any related signatures (e.g., init())
so callers like tempo.parse.ts and module.parse.ts get compile-time shape
checking and adjust imports/exports to reference the new type.
In `@packages/tempo/src/support/tempo.util.ts`:
- Around line 16-17: The create helper (create<T extends object>(obj: object,
name: string)) currently calls Object.create(proto(obj)[name]) which will throw
a generic TypeError if the prototype entry is missing or not an object; add an
explicit guard that reads const entry = proto(obj)[name] and if entry is not an
object and not null throw a clear, descriptive Error (or return a narrower typed
result/optional) before calling Object.create(entry) so failure modes are
explicit at call sites; reference the create function and proto(...) access when
implementing the check and error message.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 396-402: Tempo.#setIgnores is being called twice during
configuration: once directly in the `#setConfig` flow and again immediately inside
Tempo.#setPatterns which is invoked next, causing redundant RegExp allocation;
remove the duplicate call by deleting the direct invocation of Tempo.#setIgnores
from the `#setConfig` sequence (keep the call inside Tempo.#setPatterns so
ignorePattern rebuilding remains centralized), and ensure any initialization
assumptions in `#setPatterns` still hold when it is the sole caller.
In `@packages/tempo/src/tempo.type.ts`:
- Line 12: Remove the duplicate local type alias Ignores and reuse the already
imported Ignore from tempo.default.js: delete the Ignores declaration (the alias
defined around the previous Ignores symbol) and replace all references/usages of
Ignores in this file with Ignore so the file uses the single imported type
instead of redefining it.
In `@packages/tempo/test/standalone_parse.test.ts`:
- Around line 6-16: The test flakes because parse('tomorrow') and
Temporal.Now.zonedDateTimeISO() are taken at different instants; sample now
before calling parse and use that fixed value as the anchor when invoking parse
(if parse accepts an anchor) or compute the expected tomorrow from that sampled
now and compare to zdt; reference the parse(...) call, the
Temporal.Now.zonedDateTimeISO() sampling (use a local variable now), and the zdt
assertions so you replace the current interleaved sampling with a single
anchored sample.
🪄 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: b95db080-71ae-41e9-a2a4-0c66aa0b92b6
⛔ Files ignored due to path filters (1)
packages/tempo/img/docs-banner.pngis excluded by!**/*.png
📒 Files selected for processing (34)
packages/library/src/common/pledge.class.tspackages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/public/esm_core.index.htmlpackages/tempo/public/esm_full.index.htmlpackages/tempo/src/core.index.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/module/module.format.tspackages/tempo/src/plugin/module/module.mutate.tspackages/tempo/src/plugin/module/module.parse.tspackages/tempo/src/plugin/module/module.term.tspackages/tempo/src/plugin/plugin.type.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/plugin/term/term.index.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.runtime.tspackages/tempo/src/support/tempo.symbol.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.index.tspackages/tempo/src/tempo.parse.tspackages/tempo/src/tempo.type.tspackages/tempo/test/runtime_brand.test.tspackages/tempo/test/standalone_parse.test.tspackages/tempo/test/tempo_regexp.test.tspackages/tempo/vitest.config.ts
Comment on lines
+49
to
+70
| ```html | ||
| <script type="importmap"> | ||
| { | ||
| "imports": { | ||
| "jsbi": "https://cdn.jsdelivr.net/npm/jsbi@4.3.0/dist/jsbi.mjs", | ||
| "@js-temporal/polyfill": "https://cdn.jsdelivr.net/npm/@js-temporal/polyfill@0.5/dist/index.esm.js", | ||
| "@magmacomputing/tempo": "https://cdn.jsdelivr.net/npm/@magmacomputing/tempo@2/dist/tempo.bundle.esm.js" | ||
| } | ||
| } | ||
| </script> | ||
| ``` | ||
|
|
||
| ### 2. Implementation | ||
| ```html | ||
| <script type="module"> | ||
| import '@js-temporal/polyfill'; | ||
| import { Tempo } from '@magmacomputing/tempo'; | ||
|
|
||
| const t = new Tempo('tomorrow'); | ||
| console.log(t.format('{mon} {day}')); | ||
| </script> | ||
| ``` |
There was a problem hiding this comment.
Browser bundle filenames are inconsistent across docs/demos.
doc/installation.mdline 55:dist/tempo.bundle.esm.jsREADME.mdline 66:dist/tempo.bundle.js(UMD) for ESM-prototyping wordingpublic/esm_full.index.htmlline 236:dist/tempo.index.js
All three purport to be "the ESM entry for browsers." Please pick one canonical URL per scenario (ESM vs. UMD) and align the three locations, otherwise users following different docs will hit 404s or wrong module formats.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/doc/installation.md` around lines 49 - 70, The three browser
bundle filenames referenced are inconsistent—packages/tempo/doc/installation.md
uses dist/tempo.bundle.esm.js, README.md uses dist/tempo.bundle.js (UMD), and
public/esm_full.index.html uses dist/tempo.index.js—so pick one canonical URL
for the ESM browser entry and another for the UMD entry and update all three
locations to match; specifically, decide whether the ESM browser bundle should
be named tempo.bundle.esm.js (or tempo.index.js) and ensure
packages/tempo/doc/installation.md, README.md, and public/esm_full.index.html
all reference that same ESM filename, and likewise unify the UMD filename across
those files if a UMD example is kept.
There was a problem hiding this comment.
Actionable comments posted: 10
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.type.ts (1)
229-230:⚠️ Potential issue | 🟡 MinorOmit
ignorefrom runtimeConfigtypes.
ignorebelongs inParse(parse-time configuration), notConfig(runtime configuration). However,OptionsKeepdoes not currently omit it, causingConfigto incorrectly includeignoreas a required field. All codebase usage confirmsignoreis parse-specific (state.parse.ignore,shape.parse.ignorePattern).Proposed fix
- export type OptionsKeep = Omit<BaseOptions, "mdyLocales" | "mdyLayouts" | "pivot" | "snippet" | "layout" | "event" | "period" | "value"> + export type OptionsKeep = Omit<BaseOptions, "mdyLocales" | "mdyLayouts" | "pivot" | "snippet" | "layout" | "event" | "period" | "ignore" | "value">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.type.ts` around lines 229 - 230, OptionsKeep currently includes parse-only key "ignore", causing runtime Config types to require parse-only fields; update the OptionsKeep type (the Omit<BaseOptions,...> declaration) to also exclude "ignore" so parse-time-only properties are removed from runtime types (this will propagate to the Config type and related usages like state.parse.ignore and shape.parse.ignorePattern). Ensure you only remove "ignore" from OptionsKeep's omission list and do not change BaseOptions itself.
♻️ Duplicate comments (1)
packages/tempo/doc/installation.md (1)
49-56:⚠️ Potential issue | 🟡 MinorRe-verify canonical browser ESM filename across docs before release.
Line 55 uses
tempo.bundle.esm.js; please confirm this is the canonical browser ESM artifact and consistent with other user-facing examples (especially demo pages) to avoid 404/wrong-format guidance.#!/bin/bash # Verify all documented/browser references to Tempo bundle filenames and compare with exports metadata. fd -t f "README.md|installation.md|esm_full.index.html|package.json" packages/tempo echo "---- Bundle filename references ----" rg -n -C2 'tempo\.bundle\.esm\.js|tempo\.bundle\.js|tempo\.index\.js' packages/tempo echo "---- Package export entry points ----" rg -n -C3 '"exports"|"\."|"\./core"|tempo\.index\.js|core\.index\.js' packages/tempo/package.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/installation.md` around lines 49 - 56, Confirm whether the canonical browser ESM artifact is tempo.bundle.esm.js and ensure all user-facing docs and demo pages reference the exact filename; check the package's exports (package.json) for the published browser ESM entry and update installation.md (the importmap entry referencing "@magmacomputing/tempo": ".../tempo.bundle.esm.js") and any other files (README.md, esm_full.index.html, demo pages) to match the export name, replacing occurrences of tempo.bundle.esm.js/tempo.bundle.js/tempo.index.js as needed so they match the actual published artifact and avoid 404s.
🤖 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/README.md`:
- Line 21: Several section headings use ### immediately after the top-level
title; update them to use sequential heading levels (##) instead. Replace
occurrences like "### ⚡ Quick Start" and the other headers listed ("###" forms
at the other sections) with "##" so the document progresses from # to ##,
keeping all subsequent subheadings nested correctly (e.g., change any sibling
### headers to ## to restore proper heading hierarchy).
In `@packages/tempo/src/plugin/module/module.parse.ts`:
- Around line 459-462: The code only converts textual month names to numbers
when TempoClass is present, leaving names like "Jan" unconverted in groups["mm"]
and causing parse failures; update the conversion to use the MONTH enum from
enums (or import enums.MONTH at module scope) as a fallback when TempoClass is
not registered: in the block handling groups["mm"] (and using prefix()), if
TempoClass is missing use enums.MONTH[mm] (or the imported MONTH) to lookup the
numeric value and pad it the same way so groups["mm"] is always a zero-padded
numeric string before parseDate()/num() runs.
In `@packages/tempo/src/plugin/module/module.term.ts`:
- Around line 437-445: The loop's safety branch advances jump but never updates
next, leaving while (next.epochNanoseconds <= zdt.epochNanoseconds) potentially
stuck; inside the iterations > 50 branch (where jump is advanced using
termObj.define.call(new (getHost(instance))(jump, ...), getStep(...))), assign
next to the new jump (or recompute next the same way it is in the else branch)
after updating jump so next reflects the advanced position; update references to
next, jump, iterations, termObj.define, getHost, getStep and zdt accordingly to
ensure the loop can terminate.
- Around line 38-40: The code accesses symbol handlers like
Tempo[sym.$termError]?.(...) without guarding that the base object Tempo may be
undefined; update all occurrences (e.g., in module.term.ts where you check if
(!termObj)) to use optional chaining on the base object
(Tempo?.[sym.$termError]?.(...)) so the call short-circuits when Tempo is
undefined; apply the same change for the other symbol handler access patterns
listed (all 12 occurrences) to prevent TypeError in standalone parse contexts.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 105-108: The timeZone override handler only updates
config.timeZone but leaves the hemisphere/sphere stale; modify the 'timeZone'
case in tempo.init.ts so after computing zone (using enums.TIMEZONE[zone] ??
arg.value) you also update state.config.sphere to the corresponding hemisphere
by deriving it the same way init() does (either setProperty(state.config,
'sphere', <derivedSphere>) or call the existing helper that computes sphere from
the zone), ensuring config.sphere stays in sync with config.timeZone.
- Around line 91-95: The code currently constructs user-provided regexes with
new RegExp(v) inside collect(...) (when optKey === 'snippet'), which opens
extendState()/Options to catastrophic backtracking; either add a
validation/sanitization step before creating RegExp (e.g., enforce max pattern
length, reject suspicious quantifier sequences, run a regex-safety check
library) and throw/omit invalid patterns, or avoid constructing RegExp
altogether and require callers to pass safe RegExp objects; also update public
API docs for extendState()/Options to state that snippet patterns must be
trusted or pre-validated. Ensure you change the branch where optKey ===
'snippet' (and references to isRegExp and collect) to perform the
validation/safe-construction and surface a clear error when validation fails.
- Around line 119-121: In the 'case "pivot"' branch where you currently do
state.parse.pivot = Number(arg.value), validate arg.value before mutating state:
coerce to a number (e.g., const v = Number(arg.value)), check Number.isFinite(v)
(and optionally Number.isInteger(v) or a desired range), and only assign
state.parse.pivot = v when the check passes; otherwise reject the input (throw
an error, return a validation failure, or leave state unchanged) so NaN or
empty-string coercion to 0 cannot corrupt two-digit year behavior.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 451-454: The discovery.ignore value is being applied by calling
Tempo.init({ ignore }) inside an already-running Tempo.init which returns early
due to `#lifecycle.initialising`; instead, compute the ignore value (const ignore
= isFunction(discovery.ignore) ? discovery.ignore() : discovery.ignore) and
merge it into the existing init options object passed to the current Tempo.init
invocation (avoid calling Tempo.init recursively); remove the nested
Tempo.init({ ignore }) call and ensure the merged options are used by the
Tempo.init logic so discovery-level ignore is honored.
- Around line 1143-1166: The code currently only marks this.#errored when an
exception is thrown, but `#parse`() can return undefined silently; update `#resolve`
so that immediately after assigning this.#zdt = this.#parse(...), check if
this.#zdt is undefined and if so set this.#errored = true and handle it the same
way as the catch branch: if this.#local.config.catch === true log an error via
Tempo.#dbg.error(this.#local.config, <message>) and assign this.#zdt =
this.#now.toZonedDateTimeISO('UTC'), otherwise log then throw a new Error (e.g.
"Tempo parse returned undefined") so callers don't silently get a UTC fallback
while isValid remains true; reference the `#resolve` method, this.#zdt,
this.#parse, this.#errored, this.#local.config.catch, Tempo.#dbg.error and
this.#now.toZonedDateTimeISO('UTC').
In `@packages/tempo/test/storage.test.ts`:
- Around line 15-16: The test currently calls registryReset() which only clears
registries/plugins but not runtime state; replace that call with (or add) a call
to Tempo.init() so runtime.state is fully reset between tests. Update the test
to call Tempo.init() (await if async) instead of registryReset(), and ensure any
necessary imports/usages of Tempo.init() are added where registryReset() was
referenced.
---
Outside diff comments:
In `@packages/tempo/src/tempo.type.ts`:
- Around line 229-230: OptionsKeep currently includes parse-only key "ignore",
causing runtime Config types to require parse-only fields; update the
OptionsKeep type (the Omit<BaseOptions,...> declaration) to also exclude
"ignore" so parse-time-only properties are removed from runtime types (this will
propagate to the Config type and related usages like state.parse.ignore and
shape.parse.ignorePattern). Ensure you only remove "ignore" from OptionsKeep's
omission list and do not change BaseOptions itself.
---
Duplicate comments:
In `@packages/tempo/doc/installation.md`:
- Around line 49-56: Confirm whether the canonical browser ESM artifact is
tempo.bundle.esm.js and ensure all user-facing docs and demo pages reference the
exact filename; check the package's exports (package.json) for the published
browser ESM entry and update installation.md (the importmap entry referencing
"@magmacomputing/tempo": ".../tempo.bundle.esm.js") and any other files
(README.md, esm_full.index.html, demo pages) to match the export name, replacing
occurrences of tempo.bundle.esm.js/tempo.bundle.js/tempo.index.js as needed so
they match the actual published artifact and avoid 404s.
🪄 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: ab5b0aad-1227-4073-8138-a7d16ae5c125
📒 Files selected for processing (20)
packages/tempo/README.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/public/esm_full.index.htmlpackages/tempo/src/core.index.tspackages/tempo/src/plugin/module/module.parse.tspackages/tempo/src/plugin/module/module.term.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.runtime.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.parse.tspackages/tempo/src/tempo.type.tspackages/tempo/test/standalone_parse.test.tspackages/tempo/test/storage.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/public/esm_full.index.html
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/tempo/src/tempo.parse.ts
- packages/tempo/src/core.index.ts
- packages/tempo/test/standalone_parse.test.ts
- packages/tempo/doc/tempo.config.md
- packages/tempo/src/support/tempo.runtime.ts
- packages/tempo/index.md
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/tempo/README.md (1)
1-11:⚠️ Potential issue | 🟡 MinorInline HTML styling likely won’t render on GitHub/npm surfaces (regression).
Line 1-11 and Line 18-24 rely on
style="", which is commonly sanitized away in README renderers. The visual layout may degrade unexpectedly for users viewing package docs.Do GitHub README and npm package README renderers strip inline HTML style attributes, and what HTML/Markdown formatting is reliably supported?Suggested README-safe rewrite
-<div style="display: flex; align-items: stretch; gap: 24px; margin-bottom: 24px;"> - ... -</div> +<p align="left"> + <img src="./img/logo.svg" width="90" height="90" alt="Tempo Logo" /> +</p> + +# Tempo +**The Professional Date-Time Library for the Temporal API** + +[](https://opensource.org/licenses/MIT) +[](https://tc39.es/proposal-temporal/) +[](https://www.typescriptlang.org/) +[](https://nodejs.org/api/esm.html) +[](https://magmacomputing.github.io/magma/)Also applies to: 18-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/README.md` around lines 1 - 11, The README uses inline style attributes in the HTML header block (the outer div, img tag, h1 and nested divs) which are stripped by many renderers; remove all style="" attributes and convert the header to README-safe markup: place the logo image on its own line using a plain <img> tag (or Markdown image syntax), use a Markdown heading for "Tempo" (e.g., "# Tempo") and a plain italic or bold subtitle line for "The Professional Date-Time Library for the Temporal API" instead of styled divs, and eliminate the flex container HTML; update both the block containing the logo/title (the top div) and the similar block at lines 18-24 to use this README-safe approach so rendering on GitHub/npm is consistent.packages/tempo/src/plugin/module/module.term.ts (2)
496-496:⚠️ Potential issue | 🟠 MajorMissing optional chain on
Tempobase — one occurrence was missed.Every other
Tempo?.[sym.$termError]?.(...)site in this file was updated, but Line 496 still readsTempo[sym.$termError]?.(instance.config, unit). In standalone-parse contexts whereTempocan beundefined(seemodule.parse.tsobtainingTempoClassfromgetRuntime().modules['Tempo']), this line will throw aTypeErrorinstead of gracefully returningnull.🐛 Proposed fix
- if (!current) { - Tempo[sym.$termError]?.(instance.config, unit); - return null; - } + if (!current) { + Tempo?.[sym.$termError]?.(instance.config, unit); + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.term.ts` at line 496, The call to Tempo[sym.$termError]?.(instance.config, unit) can throw when Tempo is undefined; change it to use optional chaining on the base—i.e. call Tempo?.[sym.$termError]?.(instance.config, unit)—so the invocation safely no-ops when Tempo is undefined; update the occurrence in module.term.ts where instance.config and unit are passed to the sym.$termError handler to match the other updated sites.
436-447:⚠️ Potential issue | 🟠 MajorDead
if/else— safety-valve atiterations > 50is now a no-op.After the earlier fix that assigned
next = jumpin the safety branch, both arms of theif (++iterations > 50) { … } else { … }now execute identical bodies. The branch no longer provides any safety behaviour and the loop is only bounded by the outerwhilecondition (which depends onjump.add(getStep(...))always advancingnextpastzdt). IfgetStepever returns a zero-like step, the loop becomes infinite.Either collapse the branches into a single body or restore a true bail-out (emit a term error and return
null) once the iteration budget is exceeded — consistent with the other safety valves in this file (e.g., lines 471-474).🐛 Proposed fix (restore bail-out semantics)
let iterations = 0; while (next.epochNanoseconds <= zdt.epochNanoseconds) { - if (++iterations > 50) { // Safety-Valve: prevent infinite look-ahead - const currentRange = termObj.define.call(new (getHost(instance))(jump, { ...instance.config, mode: 'strict' }), false); - jump = jump.add(getStep(currentRange)); - next = jump; - } else { - const currentRange = termObj.define.call(new (getHost(instance))(jump, { ...instance.config, mode: 'strict' }), false); - jump = jump.add(getStep(currentRange)); - next = jump; - } + if (++iterations > 200) { // Safety-Valve: prevent infinite look-ahead + Tempo?.[sym.$termError]?.(instance.config, unit); + return null; + } + const currentRange = termObj.define.call(new (getHost(instance))(jump, { ...instance.config, mode: 'strict' }), false); + jump = jump.add(getStep(currentRange)); + next = jump; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.term.ts` around lines 436 - 447, The safety check on iterations is dead because both the true and false branches perform the same work; restore the intended bail-out: in the loop that increments iterations (variables iterations, next, jump, zdt) inside module.term (where termObj.define.call, getHost(instance) and getStep(currentRange) are used), replace the current identical if/else with logic that, when ++iterations > 50, triggers the same bail-out used elsewhere (emit/return a term error and return null) instead of continuing to advance jump/next; otherwise keep the existing advance behavior (compute currentRange via termObj.define.call(new (getHost(instance))(jump, {...instance.config, mode:'strict'}), false), then set jump = jump.add(getStep(currentRange)) and next = jump).
🧹 Nitpick comments (4)
packages/tempo/src/support/tempo.init.ts (2)
142-142: Optional: avoid unconditionally rebuilding patterns.
setPatterns(state)rebuilds every compiled layout regex, numeric snippets, and the ignore pattern on everyextendStatecall, even when the options only touchedanchor,calendar,locale, ormode(none of which feed into pattern compilation). For hot paths like standaloneparse(...)whereextendStateruns per-call, this is measurable overhead on large layout sets.Consider tracking whether any of
snippet | layout | event | period | ignorewere mutated during the loop, and skippingsetPatternsotherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.init.ts` at line 142, The current extendState always calls setPatterns(state) which rebuilds regexes even when unrelated options (anchor, calendar, locale, mode) changed; modify extendState to track whether any of the pattern-affecting sections (snippet, layout, event, period, ignore) were actually mutated during the loop (e.g., set a boolean like patternsDirty when handling additions/updates/removals for snippet/layout/event/period/ignore) and only call setPatterns(state) when patternsDirty is true; reference the existing setPatterns(state) and extendState functions and ensure changes to those five keys flip the flag, leaving setPatterns skipped for changes to anchor/calendar/locale/mode.
67-74: Tightenstatetyping and make the library-module fallback safer.Two issues in the signature and prelude:
state: anydiscards every typing guarantee on the very surface the rest of the PR is introducing (t.Internal.State). All call sites already pass a properly-typed state. Narrow it:-export function extendState(state: any, options: t.Options) { +export function extendState(state: t.Internal.State, options: t.Options) {The
modules['Library'] ?? {…}fallback is an all-or-nothing swap. If a consumer registers a partialLibrarymodule that exposes, say, onlyisStringandisObject, the destructuredasType/isRegExp/isUndefinedwill silently beundefinedand the subsequentasType(optVal)/isRegExp(v)calls will throw a crypticTypeError. Either destructure with per-field fallbacks or import these helpers directly (they're already used statically elsewhere in this file's siblings):- const { isString, isObject, isUndefined, isRegExp, asType } = getRuntime().modules['Library'] ?? { - isString: (v: any) => typeof v === 'string', - isObject: (v: any) => typeof v === 'object' && v !== null, - isUndefined: (v: any) => v === undefined, - isRegExp: (v: any) => v instanceof RegExp, - asType: (v: any) => ({ type: Object.prototype.toString.call(v).slice(8, -1), value: v }) - }; + const lib = getRuntime().modules['Library'] ?? {}; + const isString = lib.isString ?? ((v: any) => typeof v === 'string'); + const isObject = lib.isObject ?? ((v: any) => typeof v === 'object' && v !== null); + const isUndefined = lib.isUndefined ?? ((v: any) => v === undefined); + const isRegExp = lib.isRegExp ?? ((v: any) => v instanceof RegExp); + const asType = lib.asType ?? ((v: any) => ({ type: Object.prototype.toString.call(v).slice(8, -1), value: v }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.init.ts` around lines 67 - 74, Change extendState's signature from state: any to state: t.Internal.State and tighten local typing; then avoid the all-or-nothing modules['Library'] fallback by providing per-field fallbacks when destructuring (e.g. const { isString = (v)=>..., isObject = ..., isUndefined = ..., isRegExp = ..., asType = (v)=>... } = getRuntime().modules['Library'] ?? {}), or alternatively import the helper functions directly and use them instead of destructuring the module. Update references to isString/isObject/isUndefined/isRegExp/asType within extendState to rely on these safe fallbacks so calls like asType(optVal) cannot be undefined.packages/tempo/src/support/tempo.util.ts (1)
48-64: Minor: stale comment and out-of-order helper.Two small readability issues in this file:
- Line 49 inline-comment claims
asTypeis imported above "actually" — it isn't (onlyisSymbol, isUndefined, isString, isRegExp, isNullish, isRegExpLikeare imported from#library/type.library.js). The comment is now misleading; either drop it or reinstate theasTypeimport and use it.isEmptyis declared as aconstarrow at line 164, but referenced fromsetPatternson line 142. It works today becausesetPatternsis hoisted and only invoked after module init completes, but it's fragile (any future eager top-level call would hit the TDZ) and reads backwards. Move the helper above its first use.♻️ Suggested tweaks
- const itm = { type: Object.prototype.toString.call(value).slice(8, -1), value }; // inline asType to avoid dependency loop if needed, but we have it imported above actually + const itm = { type: Object.prototype.toString.call(value).slice(8, -1), value }; // inline asType to avoid a circular dependency with type.library+const isEmpty = (v: any) => !v || (Array.isArray(v) && v.length === 0) || (typeof v === 'object' && Object.keys(v).length === 0); + /** `@internal` build RegExp patterns into the state */ export function setPatterns(state: t.Internal.State) { ... } - -const isEmpty = (v: any) => !v || (Array.isArray(v) && v.length === 0) || (typeof v === 'object' && Object.keys(v).length === 0);Also applies to: 127-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.util.ts` around lines 48 - 64, Remove or update the misleading inline comment in collect so it no longer claims `asType` is imported (either delete the comment or actually import and use `asType` from the type library and replace the manual toString logic), and relocate the `isEmpty` const declaration so it appears before its first use in `setPatterns` (move the `isEmpty` arrow function above the `setPatterns` declaration) to avoid TDZ/ordering fragility.packages/tempo/src/support/tempo.default.ts (1)
154-156: Nit: consider freezing theIgnoredefault list.
Guardis already declaredas const(line 165);Ignoreat line 154 is a plain mutable array. Sinceinit()clones it viaasArray(Ignore)this is not a bug today, but any future direct consumer of the export could mutate the default noise-word set. For symmetry withGuard, preferas const.-export const Ignore = ['at', 'the', 'o-clock', 'o\'clock', 'on', 'in', 'of', 'by', 'for', 'to']; +export const Ignore = ['at', 'the', 'o-clock', 'o\'clock', 'on', 'in', 'of', 'by', 'for', 'to'] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.default.ts` around lines 154 - 156, Change the exported default Ignore list to an immutable tuple by appending "as const" to the array literal (i.e., make the value exported by the symbol Ignore readonly) so consumers can’t mutate the default noise-word set; this mirrors the existing Guard declaration and keeps init()’s asArray(Ignore) behavior intact while preventing future accidental mutations.
🤖 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/bin/parse.ts`:
- Around line 12-18: The idle timer created in resetIdle (variable idleTimer and
function resetIdle) is keeping the Node process alive; after calling setTimeout
to schedule the 1-hour shutdown, call .unref() on the returned timer so the
timer won’t keep the event loop alive and the REPL can exit immediately on stdin
close; ensure idleTimer is still assigned the result of setTimeout before
calling idleTimer.unref() so the existing behavior and message remain unchanged.
In `@packages/tempo/package.json`:
- Around line 192-195: The "clean" npm script currently uses a global failure
suppression that also hides failures from rm -rf dist; update the "clean" script
so that only the TypeScript cleanup is tolerant of failure by keeping the
unconditional removal of dist (rm -rf dist) but wrapping only the tsc -b --clean
invocation with the failure suppression — i.e., ensure rm -rf dist runs normally
and (tsc -b --clean) is the command with the || true fallback so stale dist
deletions cannot be skipped.
In `@packages/tempo/README.md`:
- Around line 40-43: The README example uses Tempo.ticker() which isn't in the
default core registration (tempo.index.ts only registers
Parse/Mutate/Format/Duration/Terms), so update either the docs or the
registration: either change the Quick Start snippet to use a guaranteed core API
(e.g., Tempo.parse/format usage) or add the ticker module to the default core
registration in packages/tempo/src/tempo.index.ts (register the ticker provider
alongside Parse/Mutate/Format/Duration/Terms) so Tempo.ticker() will be
available to users; reference Tempo.ticker and the default registration in
tempo.index.ts when making the change.
In `@packages/tempo/src/plugin/module/module.lexer.ts`:
- Around line 24-37: Guard against empty/whitespace strings before treating
values as numbers or running prefix-resolution: in num() trim the incoming val
and return acc unchanged if trimmed val is '' (so Number('')/Number(' ') are
not treated as 0), then proceed with Number.isFinite(Number(val)) on the trimmed
value; in prefix() trim its input and return undefined/unchanged for empty
strings so key.startsWith('') cannot match and enums lookups won't misfire;
finally, in parseDate() replace the nullish coalescing for mm with an explicit
fallback that uses dateTime.month when mm is falsy/empty (e.g., after trimming)
to ensure blank mm doesn't become month: 0. Reference functions: num(),
prefix(), parseDate(), and enums.NUMBER/MONTH/WEEKDAY.
---
Duplicate comments:
In `@packages/tempo/README.md`:
- Around line 1-11: The README uses inline style attributes in the HTML header
block (the outer div, img tag, h1 and nested divs) which are stripped by many
renderers; remove all style="" attributes and convert the header to README-safe
markup: place the logo image on its own line using a plain <img> tag (or
Markdown image syntax), use a Markdown heading for "Tempo" (e.g., "# Tempo") and
a plain italic or bold subtitle line for "The Professional Date-Time Library for
the Temporal API" instead of styled divs, and eliminate the flex container HTML;
update both the block containing the logo/title (the top div) and the similar
block at lines 18-24 to use this README-safe approach so rendering on GitHub/npm
is consistent.
In `@packages/tempo/src/plugin/module/module.term.ts`:
- Line 496: The call to Tempo[sym.$termError]?.(instance.config, unit) can throw
when Tempo is undefined; change it to use optional chaining on the base—i.e.
call Tempo?.[sym.$termError]?.(instance.config, unit)—so the invocation safely
no-ops when Tempo is undefined; update the occurrence in module.term.ts where
instance.config and unit are passed to the sym.$termError handler to match the
other updated sites.
- Around line 436-447: The safety check on iterations is dead because both the
true and false branches perform the same work; restore the intended bail-out: in
the loop that increments iterations (variables iterations, next, jump, zdt)
inside module.term (where termObj.define.call, getHost(instance) and
getStep(currentRange) are used), replace the current identical if/else with
logic that, when ++iterations > 50, triggers the same bail-out used elsewhere
(emit/return a term error and return null) instead of continuing to advance
jump/next; otherwise keep the existing advance behavior (compute currentRange
via termObj.define.call(new (getHost(instance))(jump, {...instance.config,
mode:'strict'}), false), then set jump = jump.add(getStep(currentRange)) and
next = jump).
---
Nitpick comments:
In `@packages/tempo/src/support/tempo.default.ts`:
- Around line 154-156: Change the exported default Ignore list to an immutable
tuple by appending "as const" to the array literal (i.e., make the value
exported by the symbol Ignore readonly) so consumers can’t mutate the default
noise-word set; this mirrors the existing Guard declaration and keeps init()’s
asArray(Ignore) behavior intact while preventing future accidental mutations.
In `@packages/tempo/src/support/tempo.init.ts`:
- Line 142: The current extendState always calls setPatterns(state) which
rebuilds regexes even when unrelated options (anchor, calendar, locale, mode)
changed; modify extendState to track whether any of the pattern-affecting
sections (snippet, layout, event, period, ignore) were actually mutated during
the loop (e.g., set a boolean like patternsDirty when handling
additions/updates/removals for snippet/layout/event/period/ignore) and only call
setPatterns(state) when patternsDirty is true; reference the existing
setPatterns(state) and extendState functions and ensure changes to those five
keys flip the flag, leaving setPatterns skipped for changes to
anchor/calendar/locale/mode.
- Around line 67-74: Change extendState's signature from state: any to state:
t.Internal.State and tighten local typing; then avoid the all-or-nothing
modules['Library'] fallback by providing per-field fallbacks when destructuring
(e.g. const { isString = (v)=>..., isObject = ..., isUndefined = ..., isRegExp =
..., asType = (v)=>... } = getRuntime().modules['Library'] ?? {}), or
alternatively import the helper functions directly and use them instead of
destructuring the module. Update references to
isString/isObject/isUndefined/isRegExp/asType within extendState to rely on
these safe fallbacks so calls like asType(optVal) cannot be undefined.
In `@packages/tempo/src/support/tempo.util.ts`:
- Around line 48-64: Remove or update the misleading inline comment in collect
so it no longer claims `asType` is imported (either delete the comment or
actually import and use `asType` from the type library and replace the manual
toString logic), and relocate the `isEmpty` const declaration so it appears
before its first use in `setPatterns` (move the `isEmpty` arrow function above
the `setPatterns` declaration) to avoid TDZ/ordering fragility.
🪄 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: d3dfda73-14df-4810-bd1d-597cc24368d6
📒 Files selected for processing (25)
.coderabbit.yamlpackages/tempo/.vitepress/config.tspackages/tempo/README.mdpackages/tempo/bin/parse.tspackages/tempo/demo/3-modular-granular.htmlpackages/tempo/demo/4-error-no-plugin.htmlpackages/tempo/doc/tempo.enumerators.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/module/module.duration.tspackages/tempo/src/plugin/module/module.lexer.tspackages/tempo/src/plugin/module/module.parse.tspackages/tempo/src/plugin/module/module.term.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.parse.tspackages/tempo/src/tempo.type.tspackages/tempo/test/plugin.test.tspackages/tempo/test/reactive_registration.test.tspackages/tempo/test/storage.test.ts
✅ Files skipped from review due to trivial changes (8)
- packages/tempo/doc/tempo.modularity.md
- packages/tempo/test/plugin.test.ts
- packages/tempo/.vitepress/config.ts
- packages/tempo/test/reactive_registration.test.ts
- .coderabbit.yaml
- packages/tempo/doc/tempo.enumerators.md
- packages/tempo/doc/tempo.parse.md
- packages/tempo/index.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/test/storage.test.ts
- packages/tempo/src/tempo.parse.ts
- packages/tempo/src/plugin/module/module.parse.ts
Owner
Author
|
@coderabbitai please review |
|
✅ Actions performedFull review triggered. |
Owner
Author
|
ParseEngine |
This was referenced Apr 22, 2026
Merged
Merged
Merged
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.
split of 'parse' functionality
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests