Introduce TempoRuntime: consolidate globalThis inter-module state into a single hardened bridge#17
Conversation
Agent-Logs-Url: https://github.com/magmacomputing/magma/sessions/efee19c3-55fd-4d10-a9ee-0308691d21dc Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
…tate Agent-Logs-Url: https://github.com/magmacomputing/magma/sessions/efee19c3-55fd-4d10-a9ee-0308691d21dc Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
…om:magmacomputing/magma into copilot/improve-inter-module-communication
|
@copilot have coderabbit.ai review this pull-request |
I'm not able to directly invoke CodeRabbit AI — that requires either the CodeRabbit GitHub App being configured on the repo (it will then auto-review on every PR), or a human posting In the meantime, I ran the built-in Code Review + CodeQL Security Scan tools against the PR. Results:
To trigger a CodeRabbit review, just post |
…4b40-2f2d-4c90-a835-59ae348103a8 Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCentralized Tempo internal state into a hardened global bridge via a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Tempo as Tempo class
participant Runtime as getRuntime()
participant Bridge as globalThis[sym.$Bridge]
App->>Tempo: Tempo.init(options)
Tempo->>Runtime: getRuntime()
Runtime->>Bridge: read globalThis[sym.$Bridge]
alt Bridge has branded runtime
Bridge-->>Runtime: return existing runtime
else no runtime or not branded
Runtime->>Runtime: new TempoRuntime()
Runtime->>Bridge: Object.defineProperty(sym.$Bridge, runtime) (hardened)
end
Runtime-->>Tempo: runtime instance
Tempo->>Runtime: access pluginsDb/modules/extensions
Runtime-->>Tempo: return collections
Tempo->>App: initialization complete
sequenceDiagram
participant Module as Plugin Module
participant PluginUtil as plugin.util
participant TermUtil as term.util
participant Runtime as TempoRuntime
Module->>PluginUtil: defineInterpreterModule(name, logic, statics?)
PluginUtil->>Runtime: register module in runtime.modules
alt module defines terms
PluginUtil->>TermUtil: defineTerm(term)
TermUtil->>Runtime: Runtime.addTerm(term)
end
PluginUtil->>PluginUtil: attachStatics(TempoClass, statics)
PluginUtil-->>Module: module installed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/doc/Tempo.md (1)
32-45:⚠️ Potential issue | 🔴 CriticalFix the
@js-temporal/polyfillartifact path on line 41.The URL references
dist/index.esm.min.js, but this file does not exist in v0.5.x. The package.json specifies only the unminifieddist/index.esm.js. Change line 41 from:"@js-temporal/polyfill": "https://cdn.jsdelivr.net/npm/@js-temporal/polyfill@0.5/dist/index.esm.min.js"to:
"@js-temporal/polyfill": "https://cdn.jsdelivr.net/npm/@js-temporal/polyfill@0.5/dist/index.esm.js"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/Tempo.md` around lines 32 - 45, Update the "@js-temporal/polyfill" CDN URL in the dependency mapping to point to the actual artifact in v0.5.x by replacing the non-existent minified path "dist/index.esm.min.js" with the existing "dist/index.esm.js"; locate the entry for the key "@js-temporal/polyfill" and update its value accordingly so the package resolves correctly.
🧹 Nitpick comments (11)
packages/tempo/doc/architecture.md (1)
22-23: Clarify thatcreateScoped()bypasses the hardening guarantees.The bullet advertises
createScoped()as useful for clean test isolation, but readers comparing it with the global runtime (line 17: "External code can neither replace nor delete the runtime") may assume a scoped runtime inherits the same hardening. Worth adding a one-line caveat that scoped runtimes are not pinned toglobalThisand therefore have nodefinePropertydescriptor protection — they rely purely on lexical scoping of the returned reference.Also, since tests are called out as the motivating use case, a sentence linking to the test file(s) that demonstrate the pattern (e.g.,
plugin_registration.test.ts,duration.core.test.tsper PR description) would materially help future contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/architecture.md` around lines 22 - 23, Update the architecture doc to clarify that TempoRuntime.createScoped() bypasses the hardening guarantees: explicitly state that scoped runtimes are not pinned to globalThis, lack the defineProperty descriptor protections, and instead rely solely on the lexical reference returned (contrast with getRuntime()/globalThis[BRIDGE] behavior). Add a short caveat sentence after the createScoped bullet referencing TempoRuntime.createScoped(), BRIDGE, and globalThis, and include a final sentence linking to the example tests (plugin_registration.test.ts and duration.core.test.ts) that demonstrate this test-scoping pattern.packages/tempo/README.md (1)
136-138: Tip wording is slightly misleading for granular importmap.The advanced granular example (lines 116-134) maps three distinct
@magmacomputing/tempo@2/...URLs. The tip at lines 136-137 implies users can simply "omit the version string" to get the latest, but that applies per-URL: users must drop@2from every entry to stay consistent (otherwise a mixed-version importmap can silently load split-brain copies). Consider a one-line clarification, e.g., "omit@2from every@magmacomputing/tempo@2/...URL".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/README.md` around lines 136 - 138, Update the "CDN Versioning" tip text to clarify that for the advanced granular importmap example (the three distinct `@magmacomputing/tempo`@2/... URLs) you must remove the `@2` from every mapped URL to avoid mixed-version loading; change the sentence that currently says "omit the version string" to explicitly say "omit `@2` from every `@magmacomputing/tempo@2/...` entry so all importmap entries resolve to the same latest release."packages/tempo/src/plugin/module/module.parse.ts (2)
47-47: Minor readability nit: callback parametertshadows the module namespace import.
import * as t from '../../tempo.type.js'at line 14 exposestas a namespace used heavily throughout this file (e.g.,t.ResolvedRange,t.Range,t.Groups). Naming the.find(...)callback parameter alsotmeans that inside the arrow body,tresolves to the term object instead of the type namespace. It works today because the body only readst.key/t.scope, but any future addition that referencest.Range(or similar) inside this lambda will silently break.- const termObj = getRuntime().terms.find((t: any) => t.key === ident || t.scope === ident); + const termObj = getRuntime().terms.find((tp: any) => tp.key === ident || tp.scope === ident);🤖 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` at line 47, The find callback parameter `t` in the expression getRuntime().terms.find((t: any) => t.key === ident || t.scope === ident) shadows the imported namespace `t` from '../../tempo.type.js'; rename the callback parameter to a non-conflicting name (e.g., `term` or `termEntry`) and update its uses inside the arrow (e.g., change `t.key`/`t.scope` to `term.key`/`term.scope`) so the module-level `t` namespace (used by symbols like `t.ResolvedRange`, `t.Range`, `t.Groups`) remains accessible and unshadowed.
89-98: Consider resolvinggetRuntime().termsonce per call and reusing it.This function queries
getRuntime().termsin several places (lines 47, 95, 155). Each call hits the global bridge and re-reads the terms array. Given thatparse()runs on the hot path ofnew Tempo(...)and the runtime reference is stable for the duration of a call, a singleconst terms = getRuntime().terms;at the top ofparse()(and likewise at the top ofconform) would be marginally faster and make the dependency explicit — helpful when reading the code alongside the newTempoRuntimearchitecture.Also note: line 95 walks
Object.keys(tempo)twice (some+find). If you're already touching this block, a singleconst termKey = Object.keys(tempo).find(k => k.startsWith('#'));+if (termKey && terms.length === 0)would read more cleanly.🤖 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 89 - 98, In parse(), avoid repeated bridge calls by capturing getRuntime().terms once (e.g., const terms = getRuntime().terms) at the top of the parse function and use that local terms variable in all subsequent checks (including the check currently using getRuntime().terms.length === 0); also collapse the double Object.keys(tempo) scan into a single lookup (e.g., const termKey = Object.keys(tempo).find(k => k.startsWith('#')) and then use if (termKey && terms.length === 0) { (TempoClass as any)[sym.$termError](state.config, termKey); return undefined as any; }). Apply the same single-read pattern to conform() as well so both functions reuse a local terms variable instead of calling getRuntime().terms multiple times.packages/tempo/src/support/tempo.symbol.ts (1)
11-24: Namespacing ofSymbol.for()keys is inconsistent and introduces collision risk.
$Bridgeuses a project-namespaced key ('magmacomputing/tempo/runtime'), but all others use short un-namespaced keys ('$Tempo','$TempoRegister','$isTempo', etc.). BecauseSymbol.for()uses a process-wide registry, any other package that registersSymbol.for('$isTempo')will get the same symbol and bypass the hardening this PR introduces.However, refactoring is constrained:
architecture.md(line 25) explicitly documents that$Tempomust remain user-facing and writable onglobalThisfor Global Discovery, so it cannot be namespaced. Additionally,packages/library/src/common/type.library.tsdirectly referencesSymbol.for('$isTempo'), making any key rename a breaking change across the monorepo.Recommend: Scope only the internal-only symbols that are not documented as user-facing:
- Keep
$TempoasSymbol.for('$Tempo')(documented user-facing API per architecture.md)- Namespace the others:
$Register,$isTempo,$Interpreter,$logError,$logDebug,$termError,$dbg,$guard,$errored,$Internaltomagmacomputing/tempo/<purpose>This reduces collision surface for internal bookkeeping while honoring the documented contract for public discovery slots. Account for the library package dependency in the changelog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.symbol.ts` around lines 11 - 24, The Symbol.for() keys are inconsistent and risk global collisions; keep the public discovery symbol $Tempo as Symbol.for('$Tempo') per architecture.md, but change all internal symbols ($Register, $isTempo, $Interpreter, $logError, $logDebug, $termError, $dbg, $guard, $errored, $Internal) to namespaced keys like 'magmacomputing/tempo/<purpose>' (e.g. 'magmacomputing/tempo/register', 'magmacomputing/tempo/isTempo', etc.) in tempo.symbol.ts so internal bookkeeping uses process-scoped names, and add a brief changelog note to account for packages (eg. packages/library) that reference Symbol.for('$isTempo') to avoid breaking users.packages/tempo/src/tempo.class.ts (1)
996-996: Remove commented-out dead line.The legacy
[sym.$terms]accessor is superseded by the#termsprivate getter on line 100 that readsgetRuntime().termsdirectly. The stale commented-out line should be deleted so readers don't wonder whether there's an intentional external contract being preserved.🧹 Cleanup
- // /** `@internal` */ static get [sym.$terms](): t.TermPlugin[] { return getRuntime().terms as t.TermPlugin[] } /** `@internal` */ static get [sym.$dbg](): Logify { return Tempo.#dbg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` at line 996, Remove the dead commented-out legacy accessor line referencing [sym.$terms]; it's superseded by the private getter `#terms` which already returns getRuntime().terms, so delete the commented line to avoid confusion about an external contract being preserved.packages/tempo/src/support/tempo.runtime.ts (2)
93-99:createScoped()is currently an orphan.The doc says "Pass it explicitly to internal helpers that accept an optional
runtimeparameter," but no helper in this PR actually threads aruntimeargument —getRuntime()is called unconditionally inplugin.util.ts,tempo.register.ts,term.util.ts, etc. As-is, a scoped runtime can be constructed but cannot influence any behavior. That's fine as a forward-looking hook (PR calls out Realms as optional) but consider either: (a) wiring an optionalruntimeparam through the hot paths in a follow-up, or (b) adding a@internal/@experimentaltag so consumers don't reach for it prematurely.🤖 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 93 - 99, createScoped() currently constructs a scoped TempoRuntime but is never threaded into the runtime resolution path (getRuntime()) so it has no effect; either wire an optional runtime parameter through the main helper hot paths (update calls in plugin.util.ts, tempo.register.ts, term.util.ts and any other places that call getRuntime() to accept an optional runtime and pass it down to getRuntime or TempoRuntime consumers like register/plugin utilities), or mark createScoped() as `@internal/`@experimental in its JSDoc to signal it’s not yet supported; pick one option and apply it consistently (if wiring, add optional runtime params named runtime to signatures and propagate to getRuntime(); if annotating, add the `@internal/`@experimental tag to the createScoped() docs and update public API docs accordingly).
52-67: Nit: parameter namesymshadows the module-levelsymimport.Inside
setHook/getHook/emit, thesymparameter hides the default-importedsymobject from line 1. Harmless today since the methods don't reference the import, but it's an easy foot-gun when these are later edited. Rename tokeyorhookKeyfor clarity.🧼 Proposed rename
- setHook(sym: symbol, cb: (val: any) => void): ((val: any) => void) | undefined { - const prev = this.#hooks.get(sym); - this.#hooks.set(sym, cb); + setHook(key: symbol, cb: (val: any) => void): ((val: any) => void) | undefined { + const prev = this.#hooks.get(key); + this.#hooks.set(key, cb); return prev; } - getHook(sym: symbol): ((val: any) => void) | undefined { - return this.#hooks.get(sym); + getHook(key: symbol): ((val: any) => void) | undefined { + return this.#hooks.get(key); } - emit(sym: symbol, val: any): void { - this.#hooks.get(sym)?.(val); + emit(key: symbol, val: any): void { + this.#hooks.get(key)?.(val); }🤖 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 52 - 67, The parameter name `sym` in the methods setHook, getHook, and emit is shadowing the module-level `sym` import; rename the parameter to something like `key` or `hookKey` in all three methods and update all internal uses (e.g. this.#hooks.get(sym) / this.#hooks.set(sym, cb) / this.#hooks.get(sym)?.(val)) to use the new name so the methods still return the previous hook and invoke hooks correctly without shadowing the import.packages/tempo/src/plugin/plugin.util.ts (1)
87-99: Minor: spread ordering could let caller overrideenumerable/configurable.The descriptor object
...(isDescriptor ? val : { value: val, writable: true })is spread first, thenenumerable: false, configurable: trueare appended — so your defaults always win whenvalis a plain value. But whenvalis an accessor descriptor, a caller that passed{ get, enumerable: true }would silently have that flipped toenumerable: false. That's arguably the intended invariant (non-enumerable to dodge@Immutable), but worth a one-line comment so future readers don't spend time wondering whether the caller's flags were respected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/plugin.util.ts` around lines 87 - 99, In attachStatics, callers' accessor descriptor flags can be overridden because the spread of the incoming descriptor happens before the hardcoded enumerable: false and configurable: true; add a one-line comment above the Object.defineProperty call (or above the isDescriptor handling) stating that this ordering is intentional — we force non-enumerable/configurable behavior (to avoid exposing statics to `@Immutable` / external enumeration) — so future readers know the caller-provided enumerable/configurable flags are deliberately ignored; reference attachStatics, isDescriptor, and the Object.defineProperty usage so the comment is placed next to the relevant logic.packages/tempo/src/tempo.type.ts (1)
243-244: Potential name collision forMatch.
Internal.Match(the parse-result type) and theMatchclass/value re-exported bypackages/tempo/src/support/support.index.ts(line 33, fromtempo.default.ts) now share the same identifier. Today this is OK becausetempo.type.tsis consumed only viaimport * as tinsidetempo.class.ts, but consumers pulling types from#tempo/supportdirectly andMatchfrom the main tempo package will run into ambiguity. Consider keeping the parse-result type namespaced (e.g.,MatchResultat top level, or leave it only underInternal) to preserve a single globalMatchsurface (the class).🤖 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 243 - 244, The top-level type export "export type Match = Internal.Match;" conflicts with the runtime Match class re-exported from support.index.ts; change the top-level type to a distinct name (e.g., "export type MatchResult = Internal.Match;") or remove the top-level export and leave it namespaced as Internal.Match, then update all references (e.g., in tempo.class.ts and any other files importing t.Match) to use the new symbol ("MatchResult" or "Internal.Match") so the only global "Match" identifier remains the class exported by support.index.ts.packages/tempo/package.json (1)
25-33: Removesrc/entries fromsideEffects— they're ineffective for published consumers.The
filesfield confirms the published tarball includes onlydist/, notsrc/. The entriessrc/tempo.index.tsandsrc/support/*.tsinsideEffectshave no effect for published package consumers and only apply during local development. The existing generic patterns (**/tempo.index.js,**/*-polyfill.ts,**/module.*.js,**/module.*.ts) already cover what's shipped indist/. Removing thesrc/entries would clarify thatsideEffectsapplies to the distributed build only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/package.json` around lines 25 - 33, The sideEffects array includes src/tempo.index.ts and src/support/*.ts which are not published (the package's files field only ships dist/), so remove those two src/* entries from the "sideEffects" array to avoid listing ineffective source files; keep the existing patterns like "**/tempo.index.js", "**/*-polyfill.ts", "**/module.*.js", and "**/module.*.ts" so the sideEffects list accurately reflects shipped artifacts.
🤖 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/architecture.md`:
- Around line 11-18: Update the documentation to reference the correct module
for TempoRuntime: replace the incorrect `src/tempo.runtime.ts` mention with the
actual module imported as `#tempo/support` (where `TempoRuntime` is defined) and
keep the mention of the global symbol
`Symbol.for('magmacomputing/tempo/runtime')`; ensure the doc text points readers
to the `TempoRuntime` export from the support module rather than the wrong src
path so links/import hints resolve correctly.
In `@packages/tempo/doc/Tempo.md`:
- Around line 58-60: The tip about omitting version strings is inconsistent with
the pinned packages; update the tip in Tempo.md to either (a) explicitly list
all pinned deps (`@magmacomputing/tempo`@2, `@magmacomputing/library`@2,
`@js-temporal/polyfill`@0.5) and explain how to omit each version to use latest,
or (b) reword it to state it only illustrates `@magmacomputing/tempo` so readers
aren’t misled; then make the examples consistent between Tempo.md and
packages/tempo/README.md by aligning the importmap pins so both documents show
the same set of packages and version guidance.
In `@packages/tempo/public/bundle.index.html`:
- Around line 1-12: The file starts with script tags and lacks an HTML document
wrapper; add a proper HTML5 doctype and basic document structure (html, head,
body) above the existing importmap and module scripts so the page renders in
standards mode; ensure the importmap and module scripts remain inside the body
(or head if preferred) and include a minimal head (e.g., charset/meta) to form a
valid standalone HTML document.
In `@packages/tempo/public/esm.index.html`:
- Around line 1-18: Add a minimal HTML skeleton around the snippet (prepend
<!DOCTYPE html> and wrap the script blocks inside html/head/body with a basic
<meta charset="utf-8"> and a short <title>), ensuring the importmap and module
script remain unchanged; also add a brief inline comment near the importmap
explaining it currently only exposes "@magmacomputing/tempo/core" and
"@magmacomputing/tempo/mutate" and that consumers should extend the importmap to
include other subpaths (e.g., /format, /duration) before importing additional
modules like Tempo or MutateModule.
In `@packages/tempo/public/script.index.html`:
- Around line 1-5: Add a minimal valid HTML skeleton around the existing script
snippet so the example doesn't trigger quirks mode: insert a <!doctype html> and
wrap the content in <html> with a <head> containing at least a <meta
charset="utf-8"> and a <title>, and put the existing <script
src="...tempo.bundle.js"></script> plus the inline script (which creates const t
= new Tempo('tomorrow') and logs t.toString()) inside the <body>; keep the
script order and contents unchanged so the Tempo import and the const t usage
continue to work.
In `@packages/tempo/src/plugin/module/module.mutate.ts`:
- Around line 182-185: The mutation currently pushes a match with shape { type:
'ZonedDateTime', value: zdt.toString(), match: zdt } which diverges from
ParseEngine.result conventions; update the match entry in module.mutate.ts so it
aligns with parse entries (use type 'Temporal.ZonedDateTime', set value to the
raw zdt object not a string, and set match to a string descriptor like
'Temporal.ZonedDateTime' or the existing symbol description used elsewhere), and
keep the rest of the returned internal state (sym.$Internal, matches) intact so
downstream consumers can treat mutation results the same as parse results.
In `@packages/tempo/src/plugin/term.util.ts`:
- Around line 338-349: The registerTerm function is double-writing and accesses
a non-existent rt.terms collection; remove the manual dedupe and direct push and
rely solely on TempoRuntime.addTerm(term) (the validated, deduping API) to
persist the term to pluginsDb.terms, then keep the runtime event emission
rt.emit(sym.$Register, term); ensure you do not replace addTerm with a direct
push into rt.terms or pluginsDb.terms so malformed entries (e.g., non-string
key) remain rejected.
- Around line 320-329: The loop that builds window treats Range.year as a
relative offset unconditionally (clone.year = itm.year + targetYY), conflicting
with other modules that expect absolute years; fix by normalizing year
semantics: in the window construction (the for-loop using baseYear, list, itm,
targetYY) only treat itm.year as an offset when it clearly is an offset (e.g.,
small magnitude like between -10 and 10), otherwise treat itm.year as an
absolute year and set clone.year = itm.year; update the Range/year handling here
and leave a short comment documenting the rule so term.quarter.ts,
module.term.ts, and the resolver/toZonedDateTime callers all interpret year
consistently.
- Around line 23-36: TempoRuntime currently keeps duplicate term arrays (terms
and pluginsDb.terms) causing readers like findTermPlugin to miss entries added
via addTerm; consolidate on one canonical storage (recommend pluginsDb.terms) by
updating all readers and helpers to reference pluginsDb.terms (e.g., change
getRuntime().terms usages, including findTermPlugin, to
getRuntime().pluginsDb.terms) and remove the ad-hoc push in registerTerm (or
make addTerm also update the secondary array if you choose to keep it), ensure
dedupe logic in addTerm is the single source of truth and delete redundant
maintenance to avoid divergence.
- Around line 308-329: The yearly-cycle branch in resolveCycleWindow uses
list[0] to compute startMm/startDd but doesn't ensure the list is
chronologically sorted, causing incorrect baseYear for templates not in calendar
order; before reading startItem (and before computing baseYear) sort the list
the same way getTermRange does (i.e., order entries by month/day) so the
earliest calendar entry is used as the cycle anchor (affecting variables list,
startItem, startMm, startDd and subsequent baseYear calculation).
In `@packages/tempo/src/support/tempo.runtime.ts`:
- Around line 110-126: The instanceof check in getRuntime() fails across bundle
boundaries and causes a TypeError when re-defining the non-configurable global
slot; replace the instanceof TempoRuntime check with a cross-bundle brand-check
(e.g., test a well-known symbol property like a
Symbol.for('tempo.runtime.brand') on the existing object) and ensure
TempoRuntime instances set that brand on construction (set this[brandSymbol] =
true in the TempoRuntime constructor). In getRuntime(), if (existing &&
existing[brandSymbol]) return existing; otherwise only define the global slot if
it does not already exist (avoid redefining non-configurable properties), and
add a unit test that pre-populates globalThis[sym.$Bridge] with a mock object
that has the brand symbol to verify the adoption path.
In `@packages/tempo/test/duration.core.test.ts`:
- Around line 4-19: savedHooks is module-scoped and can accumulate across
re-imports; change the snapshot/restore to use a local array captured per-suite
instead of the module-level savedHooks: inside beforeAll create a local const
(e.g., let localSavedHooks = Array<() => void>() or assign to a newly created
array) and push the current getRuntime().resetHooks entries into that local
variable, and then in afterAll restore from that localSavedHooks and avoid
reusing/clearing the module-scoped savedHooks; update references to savedHooks
in beforeAll/afterAll to use this localSavedHooks so duplicates aren’t re-added
to getRuntime().resetHooks.
---
Outside diff comments:
In `@packages/tempo/doc/Tempo.md`:
- Around line 32-45: Update the "@js-temporal/polyfill" CDN URL in the
dependency mapping to point to the actual artifact in v0.5.x by replacing the
non-existent minified path "dist/index.esm.min.js" with the existing
"dist/index.esm.js"; locate the entry for the key "@js-temporal/polyfill" and
update its value accordingly so the package resolves correctly.
---
Nitpick comments:
In `@packages/tempo/doc/architecture.md`:
- Around line 22-23: Update the architecture doc to clarify that
TempoRuntime.createScoped() bypasses the hardening guarantees: explicitly state
that scoped runtimes are not pinned to globalThis, lack the defineProperty
descriptor protections, and instead rely solely on the lexical reference
returned (contrast with getRuntime()/globalThis[BRIDGE] behavior). Add a short
caveat sentence after the createScoped bullet referencing
TempoRuntime.createScoped(), BRIDGE, and globalThis, and include a final
sentence linking to the example tests (plugin_registration.test.ts and
duration.core.test.ts) that demonstrate this test-scoping pattern.
In `@packages/tempo/package.json`:
- Around line 25-33: The sideEffects array includes src/tempo.index.ts and
src/support/*.ts which are not published (the package's files field only ships
dist/), so remove those two src/* entries from the "sideEffects" array to avoid
listing ineffective source files; keep the existing patterns like
"**/tempo.index.js", "**/*-polyfill.ts", "**/module.*.js", and "**/module.*.ts"
so the sideEffects list accurately reflects shipped artifacts.
In `@packages/tempo/README.md`:
- Around line 136-138: Update the "CDN Versioning" tip text to clarify that for
the advanced granular importmap example (the three distinct
`@magmacomputing/tempo`@2/... URLs) you must remove the `@2` from every mapped URL
to avoid mixed-version loading; change the sentence that currently says "omit
the version string" to explicitly say "omit `@2` from every
`@magmacomputing/tempo@2/...` entry so all importmap entries resolve to the same
latest release."
In `@packages/tempo/src/plugin/module/module.parse.ts`:
- Line 47: The find callback parameter `t` in the expression
getRuntime().terms.find((t: any) => t.key === ident || t.scope === ident)
shadows the imported namespace `t` from '../../tempo.type.js'; rename the
callback parameter to a non-conflicting name (e.g., `term` or `termEntry`) and
update its uses inside the arrow (e.g., change `t.key`/`t.scope` to
`term.key`/`term.scope`) so the module-level `t` namespace (used by symbols like
`t.ResolvedRange`, `t.Range`, `t.Groups`) remains accessible and unshadowed.
- Around line 89-98: In parse(), avoid repeated bridge calls by capturing
getRuntime().terms once (e.g., const terms = getRuntime().terms) at the top of
the parse function and use that local terms variable in all subsequent checks
(including the check currently using getRuntime().terms.length === 0); also
collapse the double Object.keys(tempo) scan into a single lookup (e.g., const
termKey = Object.keys(tempo).find(k => k.startsWith('#')) and then use if
(termKey && terms.length === 0) { (TempoClass as
any)[sym.$termError](state.config, termKey); return undefined as any; }). Apply
the same single-read pattern to conform() as well so both functions reuse a
local terms variable instead of calling getRuntime().terms multiple times.
In `@packages/tempo/src/plugin/plugin.util.ts`:
- Around line 87-99: In attachStatics, callers' accessor descriptor flags can be
overridden because the spread of the incoming descriptor happens before the
hardcoded enumerable: false and configurable: true; add a one-line comment above
the Object.defineProperty call (or above the isDescriptor handling) stating that
this ordering is intentional — we force non-enumerable/configurable behavior (to
avoid exposing statics to `@Immutable` / external enumeration) — so future readers
know the caller-provided enumerable/configurable flags are deliberately ignored;
reference attachStatics, isDescriptor, and the Object.defineProperty usage so
the comment is placed next to the relevant logic.
In `@packages/tempo/src/support/tempo.runtime.ts`:
- Around line 93-99: createScoped() currently constructs a scoped TempoRuntime
but is never threaded into the runtime resolution path (getRuntime()) so it has
no effect; either wire an optional runtime parameter through the main helper hot
paths (update calls in plugin.util.ts, tempo.register.ts, term.util.ts and any
other places that call getRuntime() to accept an optional runtime and pass it
down to getRuntime or TempoRuntime consumers like register/plugin utilities), or
mark createScoped() as `@internal/`@experimental in its JSDoc to signal it’s not
yet supported; pick one option and apply it consistently (if wiring, add
optional runtime params named runtime to signatures and propagate to
getRuntime(); if annotating, add the `@internal/`@experimental tag to the
createScoped() docs and update public API docs accordingly).
- Around line 52-67: The parameter name `sym` in the methods setHook, getHook,
and emit is shadowing the module-level `sym` import; rename the parameter to
something like `key` or `hookKey` in all three methods and update all internal
uses (e.g. this.#hooks.get(sym) / this.#hooks.set(sym, cb) /
this.#hooks.get(sym)?.(val)) to use the new name so the methods still return the
previous hook and invoke hooks correctly without shadowing the import.
In `@packages/tempo/src/support/tempo.symbol.ts`:
- Around line 11-24: The Symbol.for() keys are inconsistent and risk global
collisions; keep the public discovery symbol $Tempo as Symbol.for('$Tempo') per
architecture.md, but change all internal symbols ($Register, $isTempo,
$Interpreter, $logError, $logDebug, $termError, $dbg, $guard, $errored,
$Internal) to namespaced keys like 'magmacomputing/tempo/<purpose>' (e.g.
'magmacomputing/tempo/register', 'magmacomputing/tempo/isTempo', etc.) in
tempo.symbol.ts so internal bookkeeping uses process-scoped names, and add a
brief changelog note to account for packages (eg. packages/library) that
reference Symbol.for('$isTempo') to avoid breaking users.
In `@packages/tempo/src/tempo.class.ts`:
- Line 996: Remove the dead commented-out legacy accessor line referencing
[sym.$terms]; it's superseded by the private getter `#terms` which already returns
getRuntime().terms, so delete the commented line to avoid confusion about an
external contract being preserved.
In `@packages/tempo/src/tempo.type.ts`:
- Around line 243-244: The top-level type export "export type Match =
Internal.Match;" conflicts with the runtime Match class re-exported from
support.index.ts; change the top-level type to a distinct name (e.g., "export
type MatchResult = Internal.Match;") or remove the top-level export and leave it
namespaced as Internal.Match, then update all references (e.g., in
tempo.class.ts and any other files importing t.Match) to use the new symbol
("MatchResult" or "Internal.Match") so the only global "Match" identifier
remains the class exported by support.index.ts.
🪄 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: a659b79f-3db4-456e-bdb7-229cbd5c64b2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
packages/tempo/README.mdpackages/tempo/bin/core.tspackages/tempo/doc/Tempo.mdpackages/tempo/doc/architecture.mdpackages/tempo/package.jsonpackages/tempo/public/bundle.index.htmlpackages/tempo/public/esm.index.htmlpackages/tempo/public/script.index.htmlpackages/tempo/src/core.index.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/module/module.composer.tspackages/tempo/src/plugin/module/module.duration.tspackages/tempo/src/plugin/module/module.format.tspackages/tempo/src/plugin/module/module.lexer.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.util.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/plugin/term/standard.index.tspackages/tempo/src/plugin/term/term.index.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/plugin/term/term.timeline.tspackages/tempo/src/plugin/term/term.zodiac.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.register.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.symbol.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/discovery_security.test.tspackages/tempo/test/duration.core.test.tspackages/tempo/test/instance.result.test.tspackages/tempo/test/plugin.test.tspackages/tempo/test/plugin_registration.test.tspackages/tempo/test/repro_hang.test.tspackages/tempo/test/tempo_regexp.test.tspackages/tempo/test/ticker.patterns.test.ts
💤 Files with no reviewable changes (2)
- packages/tempo/test/instance.result.test.ts
- packages/tempo/src/tempo.symbol.ts
…om:magmacomputing/magma into copilot/improve-inter-module-communication
|
@coderabbitai please review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tempo/README.md (1)
83-135:⚠️ Potential issue | 🟡 MinorMake the browser examples actually load the Temporal polyfill.
Line 84 says the browser path requires a separate Temporal polyfill, but the examples either omit it or only add an import-map entry without importing it. As written, copy-pasted snippets can still fail in browsers without native
Temporal.📝 Proposed documentation adjustment
<script type="importmap"> { "imports": { - "@magmacomputing/tempo": "https://cdn.jsdelivr.net/npm/@magmacomputing/tempo@2/dist/tempo.bundle.esm.js" + "@magmacomputing/tempo": "https://cdn.jsdelivr.net/npm/@magmacomputing/tempo@2/dist/tempo.bundle.esm.js", + "@js-temporal/polyfill": "https://cdn.jsdelivr.net/npm/@js-temporal/polyfill@0.5/dist/index.esm.js" } } </script> <script type="module"> + import '@js-temporal/polyfill'; import Tempo from '@magmacomputing/tempo'; const t = new Tempo('next friday'); console.log(t.format('{mon} {day}')); </script><script type="module"> + import '@js-temporal/polyfill'; import { Tempo } from '@magmacomputing/tempo/core'; import { MutateModule } from '@magmacomputing/tempo/mutate';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/README.md` around lines 83 - 135, Update the browser examples to actually load the Temporal polyfill: modify the Import Maps and Script Tag examples to include a real reference to `@js-temporal/polyfill` and ensure it's imported before using the Tempo class (reference symbols: Tempo, `@js-temporal/polyfill`, importmap). For the Import Maps example add an imports entry for "@js-temporal/polyfill" and import it in the <script type="module"> before importing Tempo; for the Script Tag example include a separate script tag that loads the UMD/polyfill bundle before the tempo.bundle.js so Temporal is available when constructing new Tempo instances.packages/tempo/src/tempo.class.ts (1)
632-658:⚠️ Potential issue | 🟡 MinorRe-init can duplicate entries in
pluginsDb.terms.
init()clearsTempo.#termMap(line 739) but not the underlyingpluginsDb.termsarray (now owned byTempoRuntimeand persisted across inits). It then callsTempo.#setDiscovery(Tempo.#global, rt.pluginsDb as any)(line 751), which feeds every persisted term back throughthis.extend(asArray(discovery.terms)).In
extend's TermPlugin branch (line 634-658), the only dedupe guard isif (Tempo.#termMap.has(config.key)) return;— which is empty post-clear. Line 638 then unconditionally runsTempo.#terms.push(config), butTempo.#termsnow resolves togetRuntime().pluginsDb.terms(line 100) — the very array being iterated. The result is thatpluginsDb.termsgrows duplicates on everyTempo.init()/ HMR reload.
rt.addTerm()(called later viaregisterTerm(config)at line 642) dedupes, but the direct.pushat line 638 bypasses it.Suggested dedupe
- Tempo.#terms.push(config); // update registry (BEFORE side-effects) + if (!Tempo.#terms.some(t => t.key === config.key)) + Tempo.#terms.push(config); // update registry (BEFORE side-effects) Tempo.#termMap.set(config.key, config);Or route through
getRuntime().addTerm(config)and keep onlytermMapupdates here.🤖 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 632 - 658, The extend() TermPlugin branch currently pushes directly into Tempo.#terms (Tempo.#terms.push(config)) after Tempo.#termMap is cleared by init(), which causes duplicate entries in the persisted runtime array (pluginsDb.terms) on re-init/HMR; change the logic so extend() does not directly mutate the persisted array—either perform a proper dedupe check against the runtime store before pushing or, better, route additions through the runtime API (e.g., call getRuntime().addTerm(config) or rt.addTerm(config)) and only update Tempo.#termMap/Tempo.#terms references as needed; keep registerTerm(config) for side-effects but remove or guard the direct Tempo.#terms.push to prevent duplicate inserts into pluginsDb.terms.packages/tempo/src/tempo.type.ts (1)
233-241:⚠️ Potential issue | 🟡 MinorDiscovery field rename is a silent breaking change for existing configs.
Per the PR summary, the non-deprecated
term?: TermPlugin | TermPlugin[]field was removed andterms?(previously the@deprecatedalias) is now the canonical form. Any consumer that populatedglobalThis[$Tempo].term(or passed{ term: [...] }in a Discovery object) will now silently have their terms ignored at runtime — TS will flag it, but plain-JS callers and JSON-shaped configs will not get any diagnostic.Consider either:
- Keeping both keys and normalizing inside the runtime (merge
termintoterms) for a transition release, and/or- Adding a one-shot dev-mode warning when
termis observed on a Discovery object (e.g., ingetRuntime()bootstrap / the plugin-discovery read path).At minimum this deserves a prominent entry in
doc/architecture.md/ migration notes alongside the other TempoRuntime changes.🤖 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 233 - 241, The Discovery interface rename drops support for the legacy `term` key and will silently ignore existing configs; update the runtime code that consumes Discovery (e.g., in getRuntime() / the bootstrap/plugin-discovery path that reads globalThis[$Tempo]) to normalize legacy input by checking for a `term` property and merging its value into `terms` (ensuring TermPlugin | TermPlugin[] semantics match), and emit a single dev-mode warning when `term` is present recommending the new `terms` key; ensure this normalization happens before any calls to Tempo.addTerm() / Tempo.extend() so both JS/JSON consumers keep working during the transition.
♻️ Duplicate comments (1)
packages/tempo/src/plugin/module/module.mutate.ts (1)
180-186:⚠️ Potential issue | 🟡 MinorMatch entry shape still diverges from
ParseEngine.resultconvention (unchanged from previous review).Line 182 still pushes
{ type: 'ZonedDateTime', value: zdt.toString(), match: zdt }: a non-prefixedtype, a stringified ZDT invalue, and the raw ZDT object inmatch.ParseEngine.resultentries elsewhere usetype: 'Temporal.ZonedDateTime', a raw ZDT invalue, and a string descriptor inmatch(seemodule.parse.tsline 173 and line 279). Downstream consumers ofparse.resultwill need to special-case this mutation entry.Either align it with the parse convention or give it a clearly distinct shape (e.g.
type: 'Mutation') so consumers can discriminate intentionally.Proposed alignment
- matches.push({ type: 'ZonedDateTime', value: zdt.toString(), match: zdt }); + matches.push({ type: 'Temporal.ZonedDateTime', value: zdt, match: 'Mutation' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/module/module.mutate.ts` around lines 180 - 186, The match entry pushed in module.mutate.ts currently uses { type: 'ZonedDateTime', value: zdt.toString(), match: zdt } which diverges from ParseEngine.result conventions; change the pushed shape to align with parse results (use type: 'Temporal.ZonedDateTime', value: zdt (raw ZonedDateTime), match: a string descriptor) OR explicitly mark it as a mutation (e.g., type: 'Mutation') so consumers can discriminate; update the matches.push call and the subsequent new (this.constructor as any)(...) call's sym.$Internal.matches payload to use the chosen shape so downstream code that reads ParseEngine.result sees the consistent format.
🧹 Nitpick comments (7)
packages/tempo/test/ticker.patterns.test.ts (1)
2-2: Use the support barrel forisTempo.Importing from
#tempo/supportkeeps this test aligned with the new consolidated support entrypoint instead of depending on the internal file path.♻️ Proposed import cleanup
-import { isTempo } from '#tempo/support/tempo.symbol.js'; +import { isTempo } from '#tempo/support';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/ticker.patterns.test.ts` at line 2, Update the import to use the consolidated support barrel: replace the direct import from '#tempo/support/tempo.symbol.js' with an import from '#tempo/support' and keep the same symbol name isTempo so the test in ticker.patterns.test.ts continues to reference isTempo from the new entrypoint.packages/tempo/test/discovery_security.test.ts (1)
3-3: Prefer the consolidated#tempo/supportentrypoint.This test still imports the support implementation file directly. If
registryUpdateis re-exported from the new support barrel, import from#tempo/supportso the test exercises the consolidated surface.♻️ Proposed import cleanup
-import { registryUpdate } from '#tempo/support/tempo.register.js'; +import { registryUpdate } from '#tempo/support';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/discovery_security.test.ts` at line 3, Replace the direct import from '#tempo/support/tempo.register.js' with the consolidated barrel import so the test uses the public surface: update the import statement to import { registryUpdate } from '#tempo/support' (referencing the registryUpdate symbol used in the test) so the test exercises the re-exported API from the support entrypoint.packages/tempo/test/repro_hang.test.ts (1)
3-3: Prefer the consolidated support import here too.Since
isTempois part of the support surface, importing it from#tempo/supportavoids coupling this regression test tosupport/tempo.symbol.js.♻️ Proposed import cleanup
-import { isTempo } from '#tempo/support/tempo.symbol.js'; +import { isTempo } from '#tempo/support';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/repro_hang.test.ts` at line 3, The test currently imports isTempo from the concrete module support/tempo.symbol.js; change the import to use the consolidated support entry by importing isTempo from '#tempo/support' instead (update the import statement that references isTempo in repro_hang.test.ts to import from '#tempo/support'), keeping the same named import and running tests to ensure the module resolves correctly.packages/tempo/test/runtime_brand.test.ts (1)
70-96: Consider asserting singleton stability when a non-branded bridge is present.Test 3 correctly verifies that
getRuntime()doesn't adopt a non-branded squatter, but it doesn't verify the key singleton guarantee in that fallback path — i.e., that two consecutivegetRuntime()calls return the same instance. This gap is relevant becausegetRuntime()does not currently pin a new runtime when an unbranded value occupiesglobalThis[sym.$Bridge](see comment ontempo.runtime.ts), so regressions in singleton behavior would not be caught here.Suggested extra assertion
const rt = getRuntime(); expect(rt).not.toBe(fake); expect(rt).toBeInstanceOf(TempoRuntime); + // Singleton stability: repeated calls must return the same instance + expect(getRuntime()).toBe(rt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/runtime_brand.test.ts` around lines 70 - 96, Add an assertion that getRuntime() returns the same singleton instance across consecutive calls when a non-branded object occupies globalThis[sym.$Bridge]; specifically, after using trySetupBridge(fake) and calling const rt = getRuntime(), call getRuntime() again and assert the second result is === rt and still an instance of TempoRuntime, while keeping the existing checks that rt !== fake to ensure the fake wasn't adopted.packages/tempo/src/plugin/plugin.util.ts (2)
16-32:ensureModuleterm-loaded heuristic is a little implicit.The
isTermsLoadedshort-circuit (line 20) accepts both'term'and'TermsModule'as aliases and treats "any term registered" as proof the module is loaded. That coupling silently succeeds in a subtle edge case: a caller who invokedregisterTerm(...)directly but never ranTempo.extend(TermsModule)will passensureModule('TermsModule')even thoughrt.modules['TermsModule']is undefined, and may then hit a harder-to-diagnose failure downstream ininterpretwhenhostLogicis stillundefined.Recommend either:
- Gating the
isTermsLoadedfallback onisDefined(rt.modules['TermsModule'])actually being true (terms registered are then a bonus signal, not a replacement), or- Dropping the dual-name alias and routing all callers through the canonical
'TermsModule'name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/plugin.util.ts` around lines 16 - 32, The ensureModule function's current isTermsLoaded heuristic treats either 'term' or 'TermsModule' plus any registered terms as proof the module is loaded, which can mask the case where rt.modules['TermsModule'] is undefined; update ensureModule so the terms fallback only applies when the canonical module entry actually exists: change the isTermsLoaded check to require that rt.modules['TermsModule'] (or hostLogic for 'TermsModule') is defined AND rt.pluginsDb.terms.length > 0 (i.e., treat registered terms as a bonus signal, not a replacement), or alternatively standardize callers to use only 'TermsModule' and remove the 'term' alias—adjust references to module, hostLogic, and the isTermsLoaded variable within ensureModule accordingly.
87-100:attachStaticssilently skips name clashes and has a loose descriptor heuristic.Two small concerns:
Line 89 bails on
Object.hasOwn(TempoClass, key)without any diagnostic. In practice this means a plugin re-declaring an existing static (e.g., two plugins both definingTempoClass.ticker) gets silently dropped instead of throwing — the opposite of the "Core Module clash" policy a few lines down indefineInterpreterModule. Consider at least alogErrorpath for parity.Line 91's descriptor sniff (
isFunction(val.get) || isFunction(val.set)) will false-positive for any ordinary object that happens to exposeget/setmethods (e.g., aMap-like utility). If an accessor descriptor is the intended contract, require it explicitly (e.g., aSymbol.for('tempo/descriptor')brand, or insist on the caller passing a realPropertyDescriptor).Non-blocking — the attach path is currently only consumed by
extend.ticker.ts, so blast radius is small today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/plugin.util.ts` around lines 87 - 100, attachStatics currently silently skips static name clashes and uses a loose heuristic to detect accessor descriptors; update attachStatics to (1) report name collisions instead of silently returning — when Object.hasOwn(TempoClass, key) is true call the existing logger/error path (e.g., logError or processLogger) to surface the conflict for the caller instead of continuing silently, and (2) tighten the descriptor detection by requiring an explicit descriptor shape/brand: replace the loose isDescriptor check with a stricter test that accepts only real PropertyDescriptor objects (e.g., require val to be an object that has Symbol.for('tempo/descriptor') true OR validate that it contains at least one of the PropertyDescriptor keys (get/set/value/writable/configurable/enumerable) and get/set must be functions when present), then proceed to Object.defineProperty using that validated descriptor; reference attachStatics, TempoClass and props when making these changes.packages/tempo/src/plugin/term.util.ts (1)
320-337: Year-semantics heuristic (|year| ≤ 10→ relative) is pragmatic but deserves a doc note.The magnitude-based rule correctly covers the existing term definitions (quarters use
year: 1for cross-year boundaries; fixed historical events use 4-digit years), but it's an implicit contract — any future term that legitimately wants a relative offset of e.g.year: 11, or an absolute year in[0, 10](hypothetical but not impossible), will be misclassified. Since the interpretation is load-bearing for the yearly cycle window, consider either:
- Adding a brief note to
doc/architecture.md(or theRangeJSDoc inplugin.type.ts) spelling out thatRange.yearin a cycle-window template is relative iff |year| ≤ 10, otherwise absolute, or- Moving the distinction onto an explicit flag (e.g.
yearMode?: 'relative' | 'absolute') so authors state their intent directly.Non-blocking given current usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/term.util.ts` around lines 320 - 337, The current heuristic in term.util.ts (inside the window-building loop where isNumber(itm.year) and clone.year = (itm.year >= -10 && itm.year <= 10) ? itm.year + targetYY : itm.year) treats numeric Range.year values with absolute value ≤10 as relative offsets; add explicit documentation for this semantics by updating the Range JSDoc in plugin.type.ts to state: "When Range.year is a number it is interpreted as a relative offset if |year| ≤ 10 and as an absolute year otherwise." Also add a brief inline comment next to the logic in term.util.ts referencing that JSDoc entry (or a TODO) so future authors see the contract, leaving behavior unchanged for now.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 18: The changelog line incorrectly says the exported `Match` type was
renamed to `MatchResult`; instead update the wording to state that `MatchResult`
is introduced as an alias to `Internal.Match` while the existing runtime `Match`
export remains public so users are not misled into thinking `Match` was removed;
mention the symbols `MatchResult`, `Internal.Match`, and the runtime `Match`
class to make the intent clear.
In `@packages/library/src/common/array.library.ts`:
- Around line 57-58: The comparator branch using "case isObject(valueA) &&
isObject(valueB) && typeof valueA.valueOf() === 'number':" is asymmetric and can
produce NaN if valueB.valueOf() isn't numeric; change the guard in the
comparator (the branch handling valueA/valueB and dir, near isObject, valueOf,
and the subtraction result = dir * (valueA - valueB)) to require both sides'
valueOf() return numbers (e.g., typeof valueA.valueOf() === 'number' && typeof
valueB.valueOf() === 'number') or revert to the previous homogeneous check (like
isTempo(valueA) && isTempo(valueB)) before performing the subtraction, otherwise
fall back to the non-numeric comparison path.
In `@packages/tempo/bin/core.ts`:
- Around line 4-5: Update the stale comment above the global assignment to
accurately list the current REPL globals: remove the mention of "Token" and
instead reference the actual symbols being exposed (Tempo, getType, stringify,
objectify, enumify, Pledge, enums). Edit the comment that precedes
Object.assign(globalThis, { Tempo, getType, stringify, objectify, enumify,
Pledge, enums }); so it no longer mentions Token and clearly states the current
pre-loaded globals for the core REPL.
In `@packages/tempo/doc/releases/v2.x.md`:
- Around line 17-22: The release dates are out of order: the v2.2.3 entry
currently shows 2026-04-20 while v2.2.4 shows 2026-04-19; update the v2.2.3 date
under the "## [v2.2.3]" heading to 2026-04-19 (or reorder the entries to
maintain descending chronological order) so it matches CHANGELOG.md and keeps
releases in correct descending date order.
In `@packages/tempo/src/plugin/plugin.util.ts`:
- Around line 154-166: registerPlugin currently calls rt.addPlugin(plugin) which
guards against falsy plugins, but then unconditionally pushes into
rt.extensions, causing divergence; fix by applying the same validation to the
extensions store: either call a validated runtime helper (e.g., addExtension on
TempoRuntime) instead of pushing directly, or replicate the guard used in
TempoRuntime.addPlugin (if (!plugin) return) before checking/doing
rt.extensions.push(plugin); update registerPlugin to use the same validated path
(preferred: implement and call rt.addExtension(plugin) parallel to addPlugin) so
pluginsDb.plugins and rt.extensions remain in sync.
In `@packages/tempo/src/plugin/term.util.ts`:
- Around line 259-291: resolveCycleWindow is passing a caller-owned array (list)
into sortKey which mutates in-place; make a defensive copy of list before
sorting to avoid mutating the caller's template (affecting module-level group
arrays like in term.quarter.ts and term.zodiac.ts). Locate resolveCycleWindow
and before the call to sortKey(list, ...) replace it with creating a shallow
copy (e.g., list = [...list] or list = list.slice()) and then call sortKey on
that copy so the original template/record values remain unchanged.
In `@packages/tempo/src/support/tempo.register.ts`:
- Around line 53-57: registryReset() currently clears rt.pluginsDb.terms and
rt.extensions but leaves rt.pluginsDb.plugins populated (so plugins added via
rt.addPlugin() persist); update the reset logic in tempo.register (the block
that sets rt.pluginsDb.terms.length = 0 and rt.extensions.length = 0) to also
clear the plugin list by resetting rt.pluginsDb.plugins (e.g.,
rt.pluginsDb.plugins.length = 0 or rt.pluginsDb.plugins = []), ensuring
rt.modules and rt.installed behavior remains unchanged.
In `@packages/tempo/src/support/tempo.runtime.ts`:
- Around line 111-129: getRuntime currently creates an unpinned TempoRuntime
when globalThis[sym.$Bridge] exists but lacks sym.$RuntimeBrand, causing
multiple different runtimes; fix by detecting an unbranded existing value in
getRuntime and either (1) if the global slot is configurable
(Object.getOwnPropertyDescriptor(globalThis, sym.$Bridge)?.configurable !==
false) overwrite it with a new, branded TempoRuntime and pin it with
Object.defineProperty, or (2) if the slot is non-configurable, cache the created
TempoRuntime in a module-local variable (e.g., localFallbackRuntime) and return
that on subsequent calls; ensure you reference getRuntime, TempoRuntime,
sym.$Bridge and sym.$RuntimeBrand when implementing and add a regression test
asserting getRuntime() === getRuntime() after injecting a non-branded value into
the bridge slot.
---
Outside diff comments:
In `@packages/tempo/README.md`:
- Around line 83-135: Update the browser examples to actually load the Temporal
polyfill: modify the Import Maps and Script Tag examples to include a real
reference to `@js-temporal/polyfill` and ensure it's imported before using the
Tempo class (reference symbols: Tempo, `@js-temporal/polyfill`, importmap). For
the Import Maps example add an imports entry for "@js-temporal/polyfill" and
import it in the <script type="module"> before importing Tempo; for the Script
Tag example include a separate script tag that loads the UMD/polyfill bundle
before the tempo.bundle.js so Temporal is available when constructing new Tempo
instances.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 632-658: The extend() TermPlugin branch currently pushes directly
into Tempo.#terms (Tempo.#terms.push(config)) after Tempo.#termMap is cleared by
init(), which causes duplicate entries in the persisted runtime array
(pluginsDb.terms) on re-init/HMR; change the logic so extend() does not directly
mutate the persisted array—either perform a proper dedupe check against the
runtime store before pushing or, better, route additions through the runtime API
(e.g., call getRuntime().addTerm(config) or rt.addTerm(config)) and only update
Tempo.#termMap/Tempo.#terms references as needed; keep registerTerm(config) for
side-effects but remove or guard the direct Tempo.#terms.push to prevent
duplicate inserts into pluginsDb.terms.
In `@packages/tempo/src/tempo.type.ts`:
- Around line 233-241: The Discovery interface rename drops support for the
legacy `term` key and will silently ignore existing configs; update the runtime
code that consumes Discovery (e.g., in getRuntime() / the
bootstrap/plugin-discovery path that reads globalThis[$Tempo]) to normalize
legacy input by checking for a `term` property and merging its value into
`terms` (ensuring TermPlugin | TermPlugin[] semantics match), and emit a single
dev-mode warning when `term` is present recommending the new `terms` key; ensure
this normalization happens before any calls to Tempo.addTerm() / Tempo.extend()
so both JS/JSON consumers keep working during the transition.
---
Duplicate comments:
In `@packages/tempo/src/plugin/module/module.mutate.ts`:
- Around line 180-186: The match entry pushed in module.mutate.ts currently uses
{ type: 'ZonedDateTime', value: zdt.toString(), match: zdt } which diverges from
ParseEngine.result conventions; change the pushed shape to align with parse
results (use type: 'Temporal.ZonedDateTime', value: zdt (raw ZonedDateTime),
match: a string descriptor) OR explicitly mark it as a mutation (e.g., type:
'Mutation') so consumers can discriminate; update the matches.push call and the
subsequent new (this.constructor as any)(...) call's sym.$Internal.matches
payload to use the chosen shape so downstream code that reads ParseEngine.result
sees the consistent format.
---
Nitpick comments:
In `@packages/tempo/src/plugin/plugin.util.ts`:
- Around line 16-32: The ensureModule function's current isTermsLoaded heuristic
treats either 'term' or 'TermsModule' plus any registered terms as proof the
module is loaded, which can mask the case where rt.modules['TermsModule'] is
undefined; update ensureModule so the terms fallback only applies when the
canonical module entry actually exists: change the isTermsLoaded check to
require that rt.modules['TermsModule'] (or hostLogic for 'TermsModule') is
defined AND rt.pluginsDb.terms.length > 0 (i.e., treat registered terms as a
bonus signal, not a replacement), or alternatively standardize callers to use
only 'TermsModule' and remove the 'term' alias—adjust references to module,
hostLogic, and the isTermsLoaded variable within ensureModule accordingly.
- Around line 87-100: attachStatics currently silently skips static name clashes
and uses a loose heuristic to detect accessor descriptors; update attachStatics
to (1) report name collisions instead of silently returning — when
Object.hasOwn(TempoClass, key) is true call the existing logger/error path
(e.g., logError or processLogger) to surface the conflict for the caller instead
of continuing silently, and (2) tighten the descriptor detection by requiring an
explicit descriptor shape/brand: replace the loose isDescriptor check with a
stricter test that accepts only real PropertyDescriptor objects (e.g., require
val to be an object that has Symbol.for('tempo/descriptor') true OR validate
that it contains at least one of the PropertyDescriptor keys
(get/set/value/writable/configurable/enumerable) and get/set must be functions
when present), then proceed to Object.defineProperty using that validated
descriptor; reference attachStatics, TempoClass and props when making these
changes.
In `@packages/tempo/src/plugin/term.util.ts`:
- Around line 320-337: The current heuristic in term.util.ts (inside the
window-building loop where isNumber(itm.year) and clone.year = (itm.year >= -10
&& itm.year <= 10) ? itm.year + targetYY : itm.year) treats numeric Range.year
values with absolute value ≤10 as relative offsets; add explicit documentation
for this semantics by updating the Range JSDoc in plugin.type.ts to state: "When
Range.year is a number it is interpreted as a relative offset if |year| ≤ 10 and
as an absolute year otherwise." Also add a brief inline comment next to the
logic in term.util.ts referencing that JSDoc entry (or a TODO) so future authors
see the contract, leaving behavior unchanged for now.
In `@packages/tempo/test/discovery_security.test.ts`:
- Line 3: Replace the direct import from '#tempo/support/tempo.register.js' with
the consolidated barrel import so the test uses the public surface: update the
import statement to import { registryUpdate } from '#tempo/support' (referencing
the registryUpdate symbol used in the test) so the test exercises the
re-exported API from the support entrypoint.
In `@packages/tempo/test/repro_hang.test.ts`:
- Line 3: The test currently imports isTempo from the concrete module
support/tempo.symbol.js; change the import to use the consolidated support entry
by importing isTempo from '#tempo/support' instead (update the import statement
that references isTempo in repro_hang.test.ts to import from '#tempo/support'),
keeping the same named import and running tests to ensure the module resolves
correctly.
In `@packages/tempo/test/runtime_brand.test.ts`:
- Around line 70-96: Add an assertion that getRuntime() returns the same
singleton instance across consecutive calls when a non-branded object occupies
globalThis[sym.$Bridge]; specifically, after using trySetupBridge(fake) and
calling const rt = getRuntime(), call getRuntime() again and assert the second
result is === rt and still an instance of TempoRuntime, while keeping the
existing checks that rt !== fake to ensure the fake wasn't adopted.
In `@packages/tempo/test/ticker.patterns.test.ts`:
- Line 2: Update the import to use the consolidated support barrel: replace the
direct import from '#tempo/support/tempo.symbol.js' with an import from
'#tempo/support' and keep the same symbol name isTempo so the test in
ticker.patterns.test.ts continues to reference isTempo from the new entrypoint.
🪄 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: 192208e2-a478-4b08-967f-5f96cff57ea1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
CHANGELOG.mdpackage.jsonpackages/library/package.jsonpackages/library/src/common/array.library.tspackages/library/src/common/type.library.tspackages/tempo/README.mdpackages/tempo/bin/core.tspackages/tempo/doc/Tempo.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/package.jsonpackages/tempo/public/bundle.index.htmlpackages/tempo/public/esm.index.htmlpackages/tempo/public/script.index.htmlpackages/tempo/src/core.index.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/module/module.composer.tspackages/tempo/src/plugin/module/module.duration.tspackages/tempo/src/plugin/module/module.format.tspackages/tempo/src/plugin/module/module.lexer.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.util.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/plugin/term/standard.index.tspackages/tempo/src/plugin/term/term.index.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/plugin/term/term.timeline.tspackages/tempo/src/plugin/term/term.zodiac.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.register.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.symbol.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/discovery_security.test.tspackages/tempo/test/duration.core.test.tspackages/tempo/test/instance.result.test.tspackages/tempo/test/plugin.test.tspackages/tempo/test/plugin_registration.test.tspackages/tempo/test/repro_hang.test.tspackages/tempo/test/runtime_brand.test.tspackages/tempo/test/tempo_regexp.test.tspackages/tempo/test/ticker.patterns.test.tspackages/tempo/vitest.config.tsvitest.config.ts
💤 Files with no reviewable changes (2)
- packages/tempo/test/instance.result.test.ts
- packages/tempo/src/tempo.symbol.ts
| ### Changed | ||
| - **Consolidated Internal Storage**: Merged redundant internal term/plugin arrays into a unified, validated `pluginsDb` within `TempoRuntime`, reducing memory overhead and improving consistency. | ||
| - **Refined Year Semantics**: Normalized the `year` component in term templates to intelligently distinguish between relative offsets (e.g., year `0`) and absolute historical years (e.g., year `2000`). | ||
| - **Improved Type Safety**: Renamed the exported `Match` type to `MatchResult` to resolve naming conflicts with the `Match` runtime class. |
There was a problem hiding this comment.
Reword this as an alias, not a rename.
MatchResult is shown as an alias to Internal.Match, while Match still exists and the runtime Match export remains public. Calling this a rename can mislead users into expecting Match to be removed.
📝 Proposed wording
-- **Improved Type Safety**: Renamed the exported `Match` type to `MatchResult` to resolve naming conflicts with the `Match` runtime class.
+- **Improved Type Safety**: Added the exported `MatchResult` type alias to avoid naming conflicts with the public `Match` runtime object.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Improved Type Safety**: Renamed the exported `Match` type to `MatchResult` to resolve naming conflicts with the `Match` runtime class. | |
| - **Improved Type Safety**: Added the exported `MatchResult` type alias to avoid naming conflicts with the public `Match` runtime object. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 18, The changelog line incorrectly says the exported
`Match` type was renamed to `MatchResult`; instead update the wording to state
that `MatchResult` is introduced as an alias to `Internal.Match` while the
existing runtime `Match` export remains public so users are not misled into
thinking `Match` was removed; mention the symbols `MatchResult`,
`Internal.Match`, and the runtime `Match` class to make the intent clear.
| case isObject(valueA) && isObject(valueB) && typeof valueA.valueOf() === 'number': | ||
| result = dir * (valueA - valueB); |
There was a problem hiding this comment.
Asymmetric valueOf() check may yield NaN sort order.
The new branch only verifies valueA.valueOf() is numeric, not valueB. If the two extracted values are objects whose valueOf() results diverge (e.g., one numeric, one string/object), valueA - valueB silently produces NaN, which collapses to 0 via -1|0|1 typing and leaves ordering undefined — a regression compared to the previous isTempo(a) && isTempo(b) guard which guaranteed both sides were homogeneous.
🛡️ Proposed fix
- case isObject(valueA) && isObject(valueB) && typeof valueA.valueOf() === 'number':
+ case isObject(valueA) && isObject(valueB)
+ && typeof valueA.valueOf() === 'number'
+ && typeof valueB.valueOf() === 'number':
result = dir * (valueA - valueB);
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/library/src/common/array.library.ts` around lines 57 - 58, The
comparator branch using "case isObject(valueA) && isObject(valueB) && typeof
valueA.valueOf() === 'number':" is asymmetric and can produce NaN if
valueB.valueOf() isn't numeric; change the guard in the comparator (the branch
handling valueA/valueB and dir, near isObject, valueOf, and the subtraction
result = dir * (valueA - valueB)) to require both sides' valueOf() return
numbers (e.g., typeof valueA.valueOf() === 'number' && typeof valueB.valueOf()
=== 'number') or revert to the previous homogeneous check (like isTempo(valueA)
&& isTempo(valueB)) before performing the subtraction, otherwise fall back to
the non-numeric comparison path.
| // Pre-load Tempo and Token to the global scope for ease of use in the core REPL | ||
| Object.assign(globalThis, { Tempo, Token, Snippet, getType, stringify, objectify, enumify, enums }); | ||
| Object.assign(globalThis, { Tempo, getType, stringify, objectify, enumify, Pledge, enums }); |
There was a problem hiding this comment.
Stale comment: mentions Token which is no longer pre-loaded.
The line 4 comment still says "Pre-load Tempo and Token to the global scope" but Token was removed from the import and from Object.assign. Update the comment to reflect the current REPL globals (e.g., include Pledge, drop Token).
✏️ Proposed fix
-// Pre-load Tempo and Token to the global scope for ease of use in the core REPL
+// Pre-load Tempo and related helpers to the global scope for ease of use in the core REPL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pre-load Tempo and Token to the global scope for ease of use in the core REPL | |
| Object.assign(globalThis, { Tempo, Token, Snippet, getType, stringify, objectify, enumify, enums }); | |
| Object.assign(globalThis, { Tempo, getType, stringify, objectify, enumify, Pledge, enums }); | |
| // Pre-load Tempo and related helpers to the global scope for ease of use in the core REPL | |
| Object.assign(globalThis, { Tempo, getType, stringify, objectify, enumify, Pledge, enums }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/bin/core.ts` around lines 4 - 5, Update the stale comment
above the global assignment to accurately list the current REPL globals: remove
the mention of "Token" and instead reference the actual symbols being exposed
(Tempo, getType, stringify, objectify, enumify, Pledge, enums). Edit the comment
that precedes Object.assign(globalThis, { Tempo, getType, stringify, objectify,
enumify, Pledge, enums }); so it no longer mentions Token and clearly states the
current pre-loaded globals for the core REPL.
| ## [v2.2.4] - 2026-04-19 | ||
| ### 🛡️ Production Safety | ||
| - **Immutable Compatibility**: Added redefinition guards to `TickerModule` to prevent errors on already-frozen classes. | ||
| - **ESM Integrity**: Bundled `tslib` into granular ESM builds to resolve resolution failures in standard browser environments. | ||
|
|
||
| ## [v2.2.3] - 2026-04-20 |
There was a problem hiding this comment.
Fix the v2.2.3 release date ordering.
This page now lists v2.2.4 as 2026-04-19, followed by v2.2.3 as 2026-04-20. That conflicts with the descending release order and with CHANGELOG.md, which dates 2.2.3 as 2026-04-19.
📝 Proposed fix
-## [v2.2.3] - 2026-04-20
+## [v2.2.3] - 2026-04-19📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## [v2.2.4] - 2026-04-19 | |
| ### 🛡️ Production Safety | |
| - **Immutable Compatibility**: Added redefinition guards to `TickerModule` to prevent errors on already-frozen classes. | |
| - **ESM Integrity**: Bundled `tslib` into granular ESM builds to resolve resolution failures in standard browser environments. | |
| ## [v2.2.3] - 2026-04-20 | |
| ## [v2.2.4] - 2026-04-19 | |
| ### 🛡️ Production Safety | |
| - **Immutable Compatibility**: Added redefinition guards to `TickerModule` to prevent errors on already-frozen classes. | |
| - **ESM Integrity**: Bundled `tslib` into granular ESM builds to resolve resolution failures in standard browser environments. | |
| ## [v2.2.3] - 2026-04-19 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/doc/releases/v2.x.md` around lines 17 - 22, The release dates
are out of order: the v2.2.3 entry currently shows 2026-04-20 while v2.2.4 shows
2026-04-19; update the v2.2.3 date under the "## [v2.2.3]" heading to 2026-04-19
(or reorder the entries to maintain descending chronological order) so it
matches CHANGELOG.md and keeps releases in correct descending date order.
| export function resolveCycleWindow(source: Tempo | any, template: Range[] | Record<string, Range[]>, { anchor, groupBy = [], ...options }: resolveOptions = {}): Range[] { | ||
| // ensure we have a valid Tempo instance to work with | ||
| const t = isTempo(source) ? source : (isDefined(source) ? new (getHost(source))(source) : source); | ||
| if (!isTempo(t)) return []; |
There was a problem hiding this comment.
resolveCycleWindow silently returns [] for non-Tempo, non-object source.
For a primitive source (e.g. a string/number/bigint someone forwards in), isDefined(source) is true and getHost(source) from plugin.util.ts falls through to (source as any).constructor — i.e. String/Number/BigInt — so the branch evaluates to new String(source)(source) / similar, producing a non-Tempo wrapper that immediately trips the !isTempo(t) guard and returns [].
That’s an easy failure mode to miss in calling code, since there is no diagnostic. Either:
- Tighten the signature to
Tempo | DateTimeand reject other shapes explicitly, or - Log through
sym.$termError(as the template-missing branch at line 267 already does) when coercion fails, so debuggers get a signal instead of an empty result.
| export function resolveCycleWindow(source: Tempo | any, template: Range[] | Record<string, Range[]>, { anchor, groupBy = [], ...options }: resolveOptions = {}): Range[] { | ||
| // ensure we have a valid Tempo instance to work with | ||
| const t = isTempo(source) ? source : (isDefined(source) ? new (getHost(source))(source) : source); | ||
| if (!isTempo(t)) return []; | ||
|
|
||
| // 1. Resolve Template (supporting optional dynamic grouping) | ||
| let list: Range[] = []; | ||
| if (!isDefined(template)) { | ||
| (t.constructor as any)[sym.$termError](t.config, 'template'); | ||
| return []; | ||
| } | ||
|
|
||
| if (!Array.isArray(template) && groupBy.length > 0) { | ||
| const groupKey = groupBy | ||
| .map(key => options[key] ?? anchor?.[key] ?? t.config[key] ?? (t as any)[key] ?? '') | ||
| .join('.'); | ||
|
|
||
| list = (template as any)[groupKey] ?? []; | ||
|
|
||
| if (list.length === 0) { | ||
| const missing = groupBy.filter(k => isUndefined(options[k]) && isUndefined(anchor?.[k]) && isUndefined(t.config[k])); | ||
| const msg = missing.length > 0 ? `Missing grouping criteria: ${missing.join(', ')}` : `No ranges found for group: ${groupKey}`; | ||
| (t.constructor as any)[sym.$termError](t.config, msg); | ||
| return []; | ||
| } | ||
| } else { | ||
| list = Array.isArray(template) ? template : Object.values(template).flat() as Range[]; | ||
| } | ||
|
|
||
| if (list.length === 0) return []; | ||
|
|
||
| // Ensure chronological order for reliable anchor/window calculation | ||
| sortKey(list, 'month', 'day', 'hour', 'minute', 'second', 'millisecond', 'microsecond', 'nanosecond'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'array\.library\.(ts|js)$' --exec rg -nP -C3 '\bexport\s+(const|function)\s+sortKey\b' {}
fd -t f 'array\.library\.(ts|js)$' --exec rg -nP -B1 -A15 '\bsortKey\b' {}Repository: magmacomputing/magma
Length of output: 1246
🏁 Script executed:
#!/bin/bash
# Check the exact code at line 291
sed -n '285,295p' packages/tempo/src/plugin/term.util.tsRepository: magmacomputing/magma
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check if term.quarter.ts and term.zodiac.ts exist and if they define templates that might be affected
fd -t f 'term\.(quarter|zodiac)\.ts$'
# Also check for usage of resolveCycleWindow to see the impact
rg -n 'resolveCycleWindow' --type ts -B2 -A2Repository: magmacomputing/magma
Length of output: 4329
🏁 Script executed:
#!/bin/bash
# Check the definition of groups in these files
echo "=== term.quarter.ts groups definition ==="
sed -n '1,30p' packages/tempo/src/plugin/term/term.quarter.ts
echo -e "\n=== term.zodiac.ts groups definition ==="
sed -n '1,50p' packages/tempo/src/plugin/term/term.zodiac.tsRepository: magmacomputing/magma
Length of output: 5181
🏁 Script executed:
#!/bin/bash
# Check what defineRange returns
rg -n -A10 'export\s+(function|const)\s+defineRange' packages/tempo/src/plugin/term.util.tsRepository: magmacomputing/magma
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Check byKey implementation
rg -n -B2 -A20 'export function byKey' --type ts -g '*array*'Repository: magmacomputing/magma
Length of output: 2752
🏁 Script executed:
#!/bin/bash
# Check if resolveCycleWindow is called multiple times per session
# Look at how term.quarter and term.zodiac are used
rg -n 'resolve\(' packages/tempo/src/plugin/term/term.quarter.ts
rg -n 'resolve\(' packages/tempo/src/plugin/term/term.zodiac.ts
# Check where these resolve functions are called
rg -rn 'term\.quarter\|term\.zodiac' packages/tempo/src --type ts -A2 -B2 | head -40Repository: magmacomputing/magma
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Check how the module-level groups is being used - is it reused across calls?
# Check if resolve() is called multiple times in a session
rg -n 'defineTerm' packages/tempo/src/plugin/term/term.quarter.ts -A30 | head -50Repository: magmacomputing/magma
Length of output: 2064
sortKey at line 291 mutates the caller-supplied template array without defensive copying.
Since sortKey performs in-place mutation (as shown by the spread-copy idiom at line 50), and list frequently aliases the caller's data:
- Line 276:
list = (template as any)[groupKey] ?? [];→ retrieves a value from the caller's Record - Line 285:
list = Array.isArray(template) ? template : ...→ whentemplateis an array,list === template
The unspreaded call at line 291 mutates these references directly. The module-level groups constants in term.quarter.ts and term.zodiac.ts are affected: their arrays are reordered in-place on the first call and remain sorted for all subsequent calls, violating the expectation that authorial ordering is preserved.
Fix
- // Ensure chronological order for reliable anchor/window calculation
- sortKey(list, 'month', 'day', 'hour', 'minute', 'second', 'millisecond', 'microsecond', 'nanosecond');
+ // Ensure chronological order for reliable anchor/window calculation.
+ // Copy first — `list` may alias the caller's template array.
+ list = sortKey([...list], 'month', 'day', 'hour', 'minute', 'second', 'millisecond', 'microsecond', 'nanosecond');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resolveCycleWindow(source: Tempo | any, template: Range[] | Record<string, Range[]>, { anchor, groupBy = [], ...options }: resolveOptions = {}): Range[] { | |
| // ensure we have a valid Tempo instance to work with | |
| const t = isTempo(source) ? source : (isDefined(source) ? new (getHost(source))(source) : source); | |
| if (!isTempo(t)) return []; | |
| // 1. Resolve Template (supporting optional dynamic grouping) | |
| let list: Range[] = []; | |
| if (!isDefined(template)) { | |
| (t.constructor as any)[sym.$termError](t.config, 'template'); | |
| return []; | |
| } | |
| if (!Array.isArray(template) && groupBy.length > 0) { | |
| const groupKey = groupBy | |
| .map(key => options[key] ?? anchor?.[key] ?? t.config[key] ?? (t as any)[key] ?? '') | |
| .join('.'); | |
| list = (template as any)[groupKey] ?? []; | |
| if (list.length === 0) { | |
| const missing = groupBy.filter(k => isUndefined(options[k]) && isUndefined(anchor?.[k]) && isUndefined(t.config[k])); | |
| const msg = missing.length > 0 ? `Missing grouping criteria: ${missing.join(', ')}` : `No ranges found for group: ${groupKey}`; | |
| (t.constructor as any)[sym.$termError](t.config, msg); | |
| return []; | |
| } | |
| } else { | |
| list = Array.isArray(template) ? template : Object.values(template).flat() as Range[]; | |
| } | |
| if (list.length === 0) return []; | |
| // Ensure chronological order for reliable anchor/window calculation | |
| sortKey(list, 'month', 'day', 'hour', 'minute', 'second', 'millisecond', 'microsecond', 'nanosecond'); | |
| export function resolveCycleWindow(source: Tempo | any, template: Range[] | Record<string, Range[]>, { anchor, groupBy = [], ...options }: resolveOptions = {}): Range[] { | |
| // ensure we have a valid Tempo instance to work with | |
| const t = isTempo(source) ? source : (isDefined(source) ? new (getHost(source))(source) : source); | |
| if (!isTempo(t)) return []; | |
| // 1. Resolve Template (supporting optional dynamic grouping) | |
| let list: Range[] = []; | |
| if (!isDefined(template)) { | |
| (t.constructor as any)[sym.$termError](t.config, 'template'); | |
| return []; | |
| } | |
| if (!Array.isArray(template) && groupBy.length > 0) { | |
| const groupKey = groupBy | |
| .map(key => options[key] ?? anchor?.[key] ?? t.config[key] ?? (t as any)[key] ?? '') | |
| .join('.'); | |
| list = (template as any)[groupKey] ?? []; | |
| if (list.length === 0) { | |
| const missing = groupBy.filter(k => isUndefined(options[k]) && isUndefined(anchor?.[k]) && isUndefined(t.config[k])); | |
| const msg = missing.length > 0 ? `Missing grouping criteria: ${missing.join(', ')}` : `No ranges found for group: ${groupKey}`; | |
| (t.constructor as any)[sym.$termError](t.config, msg); | |
| return []; | |
| } | |
| } else { | |
| list = Array.isArray(template) ? template : Object.values(template).flat() as Range[]; | |
| } | |
| if (list.length === 0) return []; | |
| // Ensure chronological order for reliable anchor/window calculation. | |
| // Copy first — `list` may alias the caller's template array. | |
| list = sortKey([...list], 'month', 'day', 'hour', 'minute', 'second', 'millisecond', 'microsecond', 'nanosecond'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/src/plugin/term.util.ts` around lines 259 - 291,
resolveCycleWindow is passing a caller-owned array (list) into sortKey which
mutates in-place; make a defensive copy of list before sorting to avoid mutating
the caller's template (affecting module-level group arrays like in
term.quarter.ts and term.zodiac.ts). Locate resolveCycleWindow and before the
call to sortKey(list, ...) replace it with creating a shallow copy (e.g., list =
[...list] or list = list.slice()) and then call sortKey on that copy so the
original template/record values remain unchanged.
| rt.pluginsDb.terms.length = 0; | ||
| rt.extensions.length = 0; | ||
|
|
||
| terms.length = 0; | ||
| extensions.length = 0; | ||
|
|
||
| for (const key in modules) delete modules[key]; | ||
| internal.installed.clear(); | ||
| for (const key in rt.modules) delete rt.modules[key]; | ||
| rt.installed.clear(); |
There was a problem hiding this comment.
Clear pluginsDb.plugins during registry reset.
rt.addPlugin() records plugins in rt.pluginsDb.plugins, but registryReset() only clears pluginsDb.terms and extensions. This leaves stale plugin discovery state after reset/dispose and can leak registrations across tests or HMR cycles.
🐛 Proposed fix
rt.pluginsDb.terms.length = 0;
+ rt.pluginsDb.plugins.length = 0;
rt.extensions.length = 0;
for (const key in rt.modules) delete rt.modules[key];
rt.installed.clear();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rt.pluginsDb.terms.length = 0; | |
| rt.extensions.length = 0; | |
| terms.length = 0; | |
| extensions.length = 0; | |
| for (const key in modules) delete modules[key]; | |
| internal.installed.clear(); | |
| for (const key in rt.modules) delete rt.modules[key]; | |
| rt.installed.clear(); | |
| rt.pluginsDb.terms.length = 0; | |
| rt.pluginsDb.plugins.length = 0; | |
| rt.extensions.length = 0; | |
| for (const key in rt.modules) delete rt.modules[key]; | |
| rt.installed.clear(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tempo/src/support/tempo.register.ts` around lines 53 - 57,
registryReset() currently clears rt.pluginsDb.terms and rt.extensions but leaves
rt.pluginsDb.plugins populated (so plugins added via rt.addPlugin() persist);
update the reset logic in tempo.register (the block that sets
rt.pluginsDb.terms.length = 0 and rt.extensions.length = 0) to also clear the
plugin list by resetting rt.pluginsDb.plugins (e.g., rt.pluginsDb.plugins.length
= 0 or rt.pluginsDb.plugins = []), ensuring rt.modules and rt.installed behavior
remains unchanged.
| export function getRuntime(): TempoRuntime { | ||
| const existing = (globalThis as any)[sym.$Bridge]; | ||
| if (existing && existing[sym.$RuntimeBrand] === true) return existing; | ||
|
|
||
| const rt = new TempoRuntime(); | ||
|
|
||
| // Pin as a single hardened slot if it doesn't already exist. | ||
| // This avoids Redefinition errors if multiple bundles are loaded. | ||
| if (!existing) { | ||
| Object.defineProperty(globalThis, sym.$Bridge, { | ||
| value: rt, | ||
| enumerable: false, | ||
| configurable: false, | ||
| writable: false, | ||
| }); | ||
| } | ||
|
|
||
| return rt; | ||
| } |
There was a problem hiding this comment.
Singleton guarantee silently breaks when a non-branded value occupies the bridge slot.
If globalThis[sym.$Bridge] is truthy but lacks sym.$RuntimeBrand (e.g., residue from a test run, a colliding library, or a stale/mismatched earlier bundle), the current flow:
- Brand check fails → fallthrough.
- Creates a fresh
new TempoRuntime(). if (!existing)is false, so the new runtime is never pinned toglobalThis.- Returns the orphan rt.
Every subsequent getRuntime() call repeats the same fallthrough, yielding a different runtime each time. Consumers that rely on the singleton — e.g. Tempo.#terms (tempo.class.ts line 100) reading getRuntime().pluginsDb.terms on every access, the installed set used for plugin dedupe (tempo.class.ts lines 604/623/627), and setHook/emit wiring (tempo.class.ts line 958) — will observe inconsistent state (empty registries, re-installs, lost hooks), and plugin registration will appear to "not stick".
runtime_brand.test.ts Test 3 does not catch this because it only asserts rt instanceof TempoRuntime, not that repeated calls return the same instance.
Recommend failing fast, attempting overwrite when the slot is still configurable, or caching the fallback runtime in a module-local variable so the singleton survives.
Suggested fallback handling
export function getRuntime(): TempoRuntime {
const existing = (globalThis as any)[sym.$Bridge];
if (existing && existing[sym.$RuntimeBrand] === true) return existing;
const rt = new TempoRuntime();
- // Pin as a single hardened slot if it doesn't already exist.
- // This avoids Redefinition errors if multiple bundles are loaded.
- if (!existing) {
- Object.defineProperty(globalThis, sym.$Bridge, {
- value: rt,
- enumerable: false,
- configurable: false,
- writable: false,
- });
- }
+ // Pin as a single hardened slot. If a non-branded squatter occupies the
+ // slot and it is still configurable, overwrite; otherwise fail fast so
+ // callers don't silently observe a broken singleton.
+ const desc = Object.getOwnPropertyDescriptor(globalThis, sym.$Bridge);
+ if (!desc || desc.configurable) {
+ Object.defineProperty(globalThis, sym.$Bridge, {
+ value: rt, enumerable: false, configurable: false, writable: false,
+ });
+ } else {
+ throw new Error(
+ 'TempoRuntime: globalThis bridge slot is locked by a non-branded value; cannot install runtime.'
+ );
+ }
return rt;
}At minimum, add a regression test asserting getRuntime() === getRuntime() after a non-branded value is placed in the bridge slot.
🤖 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 111 - 129,
getRuntime currently creates an unpinned TempoRuntime when
globalThis[sym.$Bridge] exists but lacks sym.$RuntimeBrand, causing multiple
different runtimes; fix by detecting an unbranded existing value in getRuntime
and either (1) if the global slot is configurable
(Object.getOwnPropertyDescriptor(globalThis, sym.$Bridge)?.configurable !==
false) overwrite it with a new, branded TempoRuntime and pin it with
Object.defineProperty, or (2) if the slot is non-configurable, cache the created
TempoRuntime in a module-local variable (e.g., localFallbackRuntime) and return
that on subsequent calls; ensure you reference getRuntime, TempoRuntime,
sym.$Bridge and sym.$RuntimeBrand when implementing and add a regression test
asserting getRuntime() === getRuntime() after injecting a non-branded value into
the bridge slot.
|
@coderabbitai please review |
|
✅ Actions performedFull review triggered. |
|
@coderabbitai how long do you expect a full-review to take ? :30mins, perhaps ? |
|
Tip For best results, initiate chat on the files or code changes.
Since you've triggered the full review a couple of times, the latest one should already be in progress or completed. If you're not seeing results posted on the PR, it's worth checking:
If nothing appears after ~10 minutes, feel free to ping me again and I can investigate further! |
Summary
Addresses the concern about Tempo polluting the global scope. Previously, Tempo spread 7 internal bookkeeping items across
globalThis:globalThis[sym.$terms]globalThis[sym.$extends]globalThis[sym.$modules]globalThis[sym.$installed]globalThis[sym.$reset]globalThis[sym.$Plugins]globalThis[sym.$Register]All of these are now owned by a single
TempoRuntimeobject pinned under one hardenedglobalThisproperty:External code can neither enumerate, replace, nor delete the runtime object. All mutation goes through controlled methods (
addTerm,addPlugin,setRegisterHook,fireRegisterHook).Changes
src/tempo.runtime.ts(new) —TempoRuntimeclass +getRuntime()singleton factory. Includes:addTerm()/addPlugin()with input validationsetRegisterHook()/fireRegisterHook()replacing raw globalThis writesTempoRuntime.createScoped()for isolated runtimes (tests, sandboxed sub-apps)sym.$PluginssoTempo.init()finds plugins without changes to the class itselfsrc/tempo.symbol.ts—registerHook()routessym.$Registerthrough the runtime; internal-only symbols marked@deprecatedsrc/tempo.register.ts— all storage comes fromgetRuntime()instead of scatteredglobalThis[sym.*] ??=writessrc/plugin/plugin.util.ts—registerTerm/registerPluginuse runtime APIs instead of directglobalThiswritessrc/tempo.type.ts— tightenedglobalThisdeclaration; only the user-facing$Tempodiscovery slot remainstest/plugin_registration.test.ts— usesgetRuntime().pluginsDbinstead ofglobalThis[sym.$Plugins]test/duration.core.test.ts— usesgetRuntime().resetHooksinstead ofglobalThis[sym.$reset]doc/architecture.md— new TempoRuntime section explaining design rationale and multi-bundle / HMR safetyWhat does NOT change
globalThis[sym.$Tempo]/ custom discovery symbols) is intentionally preserved — that is a documented public API.tempo.class.tsor any plugin modules.On Realms
TC39 Realms would give each realm its own
globalThis, reducing cross-realm pollution further. They are viable as optional defense-in-depth for sandboxing untrusted plugins, but do not solve the core split-brain / shared-state problem on their own — and they add significant integration complexity. TheTempoRuntime.createScoped()API is Realms-ready: a realm can be given its own scoped runtime without needing a separateglobalThis.Summary by CodeRabbit
New Features
Documentation
Refactor